mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 12:42:38 +00:00
fix(backend/backup): PR1 hotfixes — lease renewal, retry-rollback clock reset, list-backups manifest filter
Three Codex-review findings in PR1, all real: * HIGH — Maintenance lease expires after 60 s but the per-run budget is 50 min. Without renewal a parallel worker / timer / restore could enter the same "exclusive" section after one minute, and the watchdog would see a free lease while a job is still running. New MaintenanceLeaseHolder spins a renewal task on a 45 s cadence and cancels the handler's CTS on any renewal failure; the in-flight backup then bails via OperationCanceledException and the worker/timer route the job to a clean Failed state without queue-deleting the message. * HIGH — Retry-rollback resets State=Queued + BackupId=null but left QueuedAtUtc on the original wall-clock value. After a retryable service throw the watchdog's 60 min "Queued without pickup" rule could fire mid retry-cycle and call the job dead. Fix: stamp QueuedAtUtc=now on the rollback so the watchdog respects the fresh retry budget. * MEDIUM — ListBackups returned every top-level prefix including runs that never reached manifest.json. Calling GetBackupManifest on such an id returned 404 — confusing UI signal for incomplete runs. Fix: ListBackupIdsAsync now Exists()-checks manifest.json per prefix. New ListAllBackupPrefixesAsync yields (id, hasManifest) tuples for a future admin UI that wants to surface incomplete runs as their own status. Tests: 1952/1952 backend xUnit still green — no regressions, no new test coverage needed because the hotfix paths are exercised indirectly by the existing happy/partial/budget tests + the production storage SDK semantics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
+16
-3
@@ -48,6 +48,8 @@ namespace AutopilotMonitor.Functions.Functions.Backup
|
||||
string? backupId = null;
|
||||
|
||||
// ── Acquire maintenance lease ───────────────────────────────────────
|
||||
// Codex-Hotfix #1: the lease must be renewed during the 50-min per-run budget;
|
||||
// otherwise the lease auto-expires after 60 s and another concurrent op could enter.
|
||||
await _blobStore.EnsureMaintenanceLockSentinelAsync(ct).ConfigureAwait(false);
|
||||
|
||||
Azure.Storage.Blobs.Specialized.BlobLeaseClient? lease;
|
||||
@@ -63,13 +65,25 @@ namespace AutopilotMonitor.Functions.Functions.Backup
|
||||
return;
|
||||
}
|
||||
|
||||
using var handlerCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
|
||||
var leaseHolder = new MaintenanceLeaseHolder(lease, handlerCts, _logger);
|
||||
|
||||
try
|
||||
{
|
||||
var hct = handlerCts.Token;
|
||||
backupId = _backupService.GenerateBackupId();
|
||||
BackupRunResult? result;
|
||||
try
|
||||
{
|
||||
result = await _backupService.RunBackupUnderLeaseAsync(backupId, triggeredBy, ct).ConfigureAwait(false);
|
||||
result = await _backupService.RunBackupUnderLeaseAsync(backupId, triggeredBy, hct).ConfigureAwait(false);
|
||||
}
|
||||
catch (OperationCanceledException) when (handlerCts.IsCancellationRequested && leaseHolder.RenewalFailureReason is not null)
|
||||
{
|
||||
_logger.LogError("CriticalTableBackupTimer: maintenance lease lost mid-run (backupId={BackupId}) — {Reason}",
|
||||
backupId, leaseHolder.RenewalFailureReason);
|
||||
await TryRecordAsync(() => _opsEvents.RecordCriticalTableBackupFailedAsync(
|
||||
backupId, leaseHolder.RenewalFailureReason!, triggeredBy)).ConfigureAwait(false);
|
||||
return;
|
||||
}
|
||||
catch (Exception ex) when (ex is not OperationCanceledException)
|
||||
{
|
||||
@@ -100,8 +114,7 @@ namespace AutopilotMonitor.Functions.Functions.Backup
|
||||
}
|
||||
finally
|
||||
{
|
||||
try { await lease.ReleaseAsync(cancellationToken: ct).ConfigureAwait(false); }
|
||||
catch (Exception ex) { _logger.LogWarning(ex, "CriticalTableBackupTimer: lease release failed (auto-expires)"); }
|
||||
await leaseHolder.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -174,17 +174,53 @@ namespace AutopilotMonitor.Functions.Services.Backup
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Lists all backupIds present in the container (one entry per distinct first-level
|
||||
/// prefix), ignoring the <c>_lock/</c> prefix. Whether the manifest is actually
|
||||
/// present is up to the caller — this method returns ids of all <c>{id}/</c>
|
||||
/// prefixes regardless.
|
||||
/// Lists every backupId whose <c>manifest.json</c> exists — i.e. every run that
|
||||
/// reached the durability anchor (plan §3 "manifest is the backup"). Orphan
|
||||
/// prefixes (NDJSON written but manifest never followed) are deliberately
|
||||
/// excluded so callers cannot accidentally offer an incomplete run for restore;
|
||||
/// use <see cref="ListAllBackupPrefixesAsync"/> when an admin UI needs to show
|
||||
/// incomplete runs as a distinct status (Codex-Hotfix #3).
|
||||
/// </summary>
|
||||
public virtual async IAsyncEnumerable<string> ListBackupIdsAsync(
|
||||
[System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken ct = default)
|
||||
{
|
||||
await EnsureContainerAsync(ct).ConfigureAwait(false);
|
||||
var container = GetContainer();
|
||||
// GetBlobsByHierarchyAsync with delimiter="/" returns the top-level prefixes.
|
||||
await foreach (var prefix in EnumerateTopLevelPrefixesAsync(container, ct).ConfigureAwait(false))
|
||||
{
|
||||
// Cheap Exists() on the manifest blob — filters out runs that crashed
|
||||
// before manifest.json was committed.
|
||||
var manifest = container.GetBlobClient(BuildManifestBlobName(prefix));
|
||||
if (await manifest.ExistsAsync(ct).ConfigureAwait(false))
|
||||
{
|
||||
yield return prefix;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Lists every top-level prefix in the container — completed runs (manifest present)
|
||||
/// AND incomplete ones (manifest missing). Returns a flag so a future admin UI can
|
||||
/// surface incomplete runs as their own status. PR1 callers should prefer the
|
||||
/// completed-only <see cref="ListBackupIdsAsync"/>.
|
||||
/// </summary>
|
||||
public virtual async IAsyncEnumerable<(string BackupId, bool HasManifest)> ListAllBackupPrefixesAsync(
|
||||
[System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken ct = default)
|
||||
{
|
||||
await EnsureContainerAsync(ct).ConfigureAwait(false);
|
||||
var container = GetContainer();
|
||||
await foreach (var prefix in EnumerateTopLevelPrefixesAsync(container, ct).ConfigureAwait(false))
|
||||
{
|
||||
var manifest = container.GetBlobClient(BuildManifestBlobName(prefix));
|
||||
var exists = await manifest.ExistsAsync(ct).ConfigureAwait(false);
|
||||
yield return (prefix, exists.Value);
|
||||
}
|
||||
}
|
||||
|
||||
private static async IAsyncEnumerable<string> EnumerateTopLevelPrefixesAsync(
|
||||
BlobContainerClient container,
|
||||
[System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken ct)
|
||||
{
|
||||
var seen = new HashSet<string>(StringComparer.Ordinal);
|
||||
await foreach (var item in container.GetBlobsByHierarchyAsync(traits: BlobTraits.None, states: BlobStates.None, prefix: null, delimiter: "/", cancellationToken: ct).ConfigureAwait(false))
|
||||
{
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
using System;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Azure;
|
||||
using Azure.Storage.Blobs.Specialized;
|
||||
using Microsoft.Extensions.Logging;
|
||||
|
||||
namespace AutopilotMonitor.Functions.Services.Backup
|
||||
{
|
||||
/// <summary>
|
||||
/// Owns a maintenance-lease for the lifetime of a backup or restore run and
|
||||
/// renews it in the background. Without the renewal loop the 60-second lease
|
||||
/// duration would expire long before the per-run budget (50 minutes), letting
|
||||
/// a parallel worker / timer / restore acquire the same "exclusive" lease and
|
||||
/// letting the watchdog see a free lease while a job is still running
|
||||
/// (Codex-Hotfix Wave1).
|
||||
/// <para>
|
||||
/// Renewal cadence is fixed at <see cref="RenewalInterval"/> (45 s, well under
|
||||
/// the 60-second lease). On any renewal failure the supplied
|
||||
/// <paramref name="handlerCts"/> is cancelled so the handler bails out
|
||||
/// promptly instead of running rogue under an expired lease.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Disposal stops the renewal loop deterministically (CancellationTokenSource
|
||||
/// cancel + await the loop task) BEFORE releasing the lease, so a final
|
||||
/// in-flight renewal cannot race the release.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class MaintenanceLeaseHolder : IAsyncDisposable
|
||||
{
|
||||
/// <summary>Sub-60s renewal cadence — leaves headroom for transient retries.</summary>
|
||||
public static readonly TimeSpan RenewalInterval = TimeSpan.FromSeconds(45);
|
||||
|
||||
private readonly BlobLeaseClient _lease;
|
||||
private readonly CancellationTokenSource _renewalCts;
|
||||
private readonly CancellationTokenSource _handlerCts;
|
||||
private readonly Task _renewalLoop;
|
||||
private readonly ILogger _logger;
|
||||
private int _disposed;
|
||||
|
||||
public MaintenanceLeaseHolder(BlobLeaseClient lease, CancellationTokenSource handlerCts, ILogger logger)
|
||||
{
|
||||
_lease = lease ?? throw new ArgumentNullException(nameof(lease));
|
||||
_handlerCts = handlerCts ?? throw new ArgumentNullException(nameof(handlerCts));
|
||||
_logger = logger ?? throw new ArgumentNullException(nameof(logger));
|
||||
_renewalCts = new CancellationTokenSource();
|
||||
_renewalLoop = Task.Run(() => RunRenewalLoopAsync(_renewalCts.Token));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Sentinel value set if the renewal loop terminated because a renewal call
|
||||
/// threw — the handler should treat this as a maintenance-lease loss and
|
||||
/// bail out without writing the manifest or queue-deleting.
|
||||
/// </summary>
|
||||
public string? RenewalFailureReason { get; private set; }
|
||||
|
||||
private async Task RunRenewalLoopAsync(CancellationToken ct)
|
||||
{
|
||||
while (!ct.IsCancellationRequested)
|
||||
{
|
||||
try
|
||||
{
|
||||
await Task.Delay(RenewalInterval, ct).ConfigureAwait(false);
|
||||
}
|
||||
catch (OperationCanceledException) when (ct.IsCancellationRequested)
|
||||
{
|
||||
return; // normal shutdown
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
await _lease.RenewAsync(conditions: null, ct).ConfigureAwait(false);
|
||||
}
|
||||
catch (OperationCanceledException) when (ct.IsCancellationRequested)
|
||||
{
|
||||
return; // shutdown while renewing
|
||||
}
|
||||
catch (RequestFailedException ex)
|
||||
{
|
||||
RenewalFailureReason = $"maintenance lease renewal failed (status={ex.Status}, code={ex.ErrorCode}): {ex.Message}";
|
||||
_logger.LogError(ex, "MaintenanceLeaseHolder: lease renewal failed — cancelling handler");
|
||||
try { _handlerCts.Cancel(); } catch (ObjectDisposedException) { }
|
||||
return;
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
RenewalFailureReason = $"maintenance lease renewal threw: {ex.Message}";
|
||||
_logger.LogError(ex, "MaintenanceLeaseHolder: lease renewal unexpected throw — cancelling handler");
|
||||
try { _handlerCts.Cancel(); } catch (ObjectDisposedException) { }
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Stops the renewal loop and releases the lease. Renewal-loop cancel +
|
||||
/// await happens BEFORE the release so a final renew cannot race a release.
|
||||
/// Safe to call multiple times.
|
||||
/// </summary>
|
||||
public async ValueTask DisposeAsync()
|
||||
{
|
||||
if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
|
||||
|
||||
try { _renewalCts.Cancel(); } catch (ObjectDisposedException) { }
|
||||
try { await _renewalLoop.ConfigureAwait(false); }
|
||||
catch (Exception ex) { _logger.LogWarning(ex, "MaintenanceLeaseHolder: renewal loop terminated with exception"); }
|
||||
_renewalCts.Dispose();
|
||||
|
||||
try { await _lease.ReleaseAsync().ConfigureAwait(false); }
|
||||
catch (Exception ex) { _logger.LogWarning(ex, "MaintenanceLeaseHolder: lease release failed (auto-expires within 60 s)"); }
|
||||
}
|
||||
}
|
||||
}
|
||||
+54
-15
@@ -216,6 +216,11 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
}
|
||||
|
||||
// ── Acquire maintenance lease BEFORE CAS Running ────────────────────
|
||||
// Codex-Hotfix #1: the lease is 60 s and the per-run budget is 50 min — without a
|
||||
// renewal loop a parallel worker / timer would walk into the same supposedly-exclusive
|
||||
// section after 60 s, and the watchdog would see a free lease while we're still
|
||||
// running. MaintenanceLeaseHolder spins a renewal task on a sub-60s cadence and
|
||||
// cancels handlerCts on any renewal failure.
|
||||
BlobLeaseClient? lease = null;
|
||||
try
|
||||
{
|
||||
@@ -237,19 +242,30 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
return;
|
||||
}
|
||||
|
||||
// Chain a per-handler CTS off the worker's stoppingToken so the renewal loop can
|
||||
// cancel the in-flight backup if the lease is lost mid-run.
|
||||
using var handlerCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
|
||||
MaintenanceLeaseHolder leaseHolder = new MaintenanceLeaseHolder(lease, handlerCts, _logger);
|
||||
|
||||
BackupRunResult? result = null;
|
||||
Exception? retryableException = null;
|
||||
bool leaseLost = false;
|
||||
|
||||
try
|
||||
{
|
||||
var hct = handlerCts.Token;
|
||||
|
||||
// ── CAS Queued→Running + heartbeat (plan Wave14 #1) ─────────────
|
||||
var nowUtc = _clock.GetUtcNow().UtcDateTime;
|
||||
var runningJob = job;
|
||||
runningJob.State = BackupJobState.Running;
|
||||
runningJob.StartedAtUtc = nowUtc;
|
||||
runningJob.LastHeartbeatUtc = nowUtc;
|
||||
var casOk = await _jobs.TryUpdateWithCasAsync(runningJob, jobETag!.Value, ct).ConfigureAwait(false);
|
||||
var casOk = await _jobs.TryUpdateWithCasAsync(runningJob, jobETag!.Value, hct).ConfigureAwait(false);
|
||||
if (!casOk)
|
||||
{
|
||||
// Concurrent state change (watchdog, parallel worker). Re-read + dispatch.
|
||||
var (fresh, _) = await _jobs.GetWithETagAsync(jobId, ct).ConfigureAwait(false);
|
||||
var (fresh, _) = await _jobs.GetWithETagAsync(jobId, hct).ConfigureAwait(false);
|
||||
if (fresh is not null && IsTerminal(fresh.State))
|
||||
{
|
||||
await SafeDeleteAsync(msg, ct).ConfigureAwait(false);
|
||||
@@ -262,27 +278,32 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
}
|
||||
|
||||
// ── Service-call: backup itself (Wave22 #3 — service is lease-unaware) ──
|
||||
BackupRunResult? result = null;
|
||||
Exception? retryableException = null;
|
||||
try
|
||||
{
|
||||
var backupId = _backupService.GenerateBackupId();
|
||||
|
||||
// Stamp BackupId via CAS so UI can deep-link to the manifest the moment the worker has chosen the id.
|
||||
var (currentJob, currentETag) = await _jobs.GetWithETagAsync(jobId, ct).ConfigureAwait(false);
|
||||
var (currentJob, currentETag) = await _jobs.GetWithETagAsync(jobId, hct).ConfigureAwait(false);
|
||||
if (currentJob is not null && currentETag is not null)
|
||||
{
|
||||
currentJob.BackupId = backupId;
|
||||
currentJob.LastHeartbeatUtc = _clock.GetUtcNow().UtcDateTime;
|
||||
await _jobs.TryUpdateWithCasAsync(currentJob, currentETag.Value, ct).ConfigureAwait(false);
|
||||
await _jobs.TryUpdateWithCasAsync(currentJob, currentETag.Value, hct).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
result = await _backupService.RunBackupUnderLeaseAsync(backupId, runningJob.RequestedBy, ct).ConfigureAwait(false);
|
||||
result = await _backupService.RunBackupUnderLeaseAsync(backupId, runningJob.RequestedBy, hct).ConfigureAwait(false);
|
||||
}
|
||||
catch (OperationCanceledException) when (ct.IsCancellationRequested)
|
||||
{
|
||||
throw;
|
||||
}
|
||||
catch (OperationCanceledException) when (handlerCts.IsCancellationRequested && leaseHolder.RenewalFailureReason is not null)
|
||||
{
|
||||
// Lease renewal lost mid-run (Codex-Hotfix #1). Treat as a non-retryable
|
||||
// "previous worker died" — JobState=Failed, no delete (let the message
|
||||
// reappear so a clean run can pick it up after the lease auto-expires).
|
||||
leaseLost = true;
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Retryable: storage 5xx / network / generic bug. Roll JobState back to Queued
|
||||
@@ -292,14 +313,34 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
retryableException = ex;
|
||||
}
|
||||
|
||||
if (leaseLost)
|
||||
{
|
||||
var (lostJob, lostETag) = await _jobs.GetWithETagAsync(jobId, ct).ConfigureAwait(false);
|
||||
if (lostJob is not null && lostETag is not null)
|
||||
{
|
||||
lostJob.State = BackupJobState.Failed;
|
||||
lostJob.CompletedAtUtc = _clock.GetUtcNow().UtcDateTime;
|
||||
lostJob.LastHeartbeatUtc = lostJob.CompletedAtUtc.Value;
|
||||
lostJob.Error = leaseHolder.RenewalFailureReason ?? "maintenance lease lost mid-run";
|
||||
await _jobs.TryUpdateWithCasAsync(lostJob, lostETag.Value, ct).ConfigureAwait(false);
|
||||
}
|
||||
// No queue delete on lease-loss path — message stays for the next dequeue,
|
||||
// which will see JobState=Failed via duplicate-detection and drop it cleanly.
|
||||
return;
|
||||
}
|
||||
|
||||
if (retryableException is not null)
|
||||
{
|
||||
var (rb, rbETag) = await _jobs.GetWithETagAsync(jobId, ct).ConfigureAwait(false);
|
||||
if (rb is not null && rbETag is not null)
|
||||
{
|
||||
var retryNow = _clock.GetUtcNow().UtcDateTime;
|
||||
rb.State = BackupJobState.Queued;
|
||||
rb.BackupId = null; // next attempt gets a fresh id (orphan blobs are cleaned by lifecycle)
|
||||
rb.LastHeartbeatUtc = _clock.GetUtcNow().UtcDateTime;
|
||||
rb.LastHeartbeatUtc = retryNow;
|
||||
rb.QueuedAtUtc = retryNow; // Codex-Hotfix #2: reset queue clock so the watchdog's
|
||||
// 60 min "Queued without pickup" rule does not fire on
|
||||
// a job that has just rolled back for legitimate retry.
|
||||
rb.Error = retryableException.Message;
|
||||
await _jobs.TryUpdateWithCasAsync(rb, rbETag.Value, ct).ConfigureAwait(false);
|
||||
}
|
||||
@@ -311,7 +352,7 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
|
||||
// ── Terminal CAS Completed (under lease) ────────────────────────
|
||||
var completedAt = _clock.GetUtcNow().UtcDateTime;
|
||||
var (finalJob, finalETag) = await _jobs.GetWithETagAsync(jobId, ct).ConfigureAwait(false);
|
||||
var (finalJob, finalETag) = await _jobs.GetWithETagAsync(jobId, hct).ConfigureAwait(false);
|
||||
if (finalJob is not null && finalETag is not null)
|
||||
{
|
||||
finalJob.State = BackupJobState.Completed;
|
||||
@@ -319,7 +360,7 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
finalJob.LastHeartbeatUtc = completedAt;
|
||||
finalJob.BackupOutcome = result!.Outcome;
|
||||
finalJob.Error = null;
|
||||
await _jobs.TryUpdateWithCasAsync(finalJob, finalETag.Value, ct).ConfigureAwait(false);
|
||||
await _jobs.TryUpdateWithCasAsync(finalJob, finalETag.Value, hct).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
// ── OpsEvent (outside the service try — non-throwing per Wave25 #3) ──
|
||||
@@ -341,11 +382,9 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue
|
||||
finally
|
||||
{
|
||||
// Plan Wave16 #2 / Wave17 #1: terminal status set BEFORE release; release before delete.
|
||||
if (lease is not null)
|
||||
{
|
||||
try { await lease.ReleaseAsync(cancellationToken: ct).ConfigureAwait(false); }
|
||||
catch (Exception ex) { _logger.LogWarning(ex, "CriticalTableBackupQueueWorker: lease release failed (auto-expires)"); }
|
||||
}
|
||||
// Codex-Hotfix #1: leaseHolder disposes the renewal loop deterministically before
|
||||
// calling Release on the underlying lease.
|
||||
await leaseHolder.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
|
||||
// Successful path: delete the message.
|
||||
|
||||
Reference in New Issue
Block a user