From 149f44728146c57dbee4c2d468cd28052ec44869 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 26 May 2026 15:53:05 +0200 Subject: [PATCH] fix(backend/backup): perform retryable rollback under lease, with strict state guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex-review finding: after the previous hotfix the retryable Running→Queued rollback ran AFTER leaseHolder.DisposeAsync(), i.e. AFTER the maintenance lease was released. That opened a window of "Running + free lease + stale heartbeat" in which the watchdog could legitimately flip the job to Failed; the rollback then re-read the fresh row, took the latest ETag, and unconditionally re-set Queued — silently undoing the watchdog's decision. Fix: * Move the retryable rollback INSIDE the leased section, right next to the success-CAS that produces the Completed terminal state. Both paths now run while the lease is still held + being renewed, so the watchdog cannot race. * Defence-in-depth: re-read the row before writing Queued and only do the state transition if the row is still Running. If a parallel actor (in theory: another worker that somehow stole the lease, or the watchdog acting on a transient probe) shifted the state, the rollback skips + logs instead of clobbering. Belt-and-braces — under a held lease the predicate should always be true. * Outer-block retry-handling shrinks to "log + rethrow"; the throw is what drives the visibility-timeout retry of the queue message. Tests: 1952/1952 still green. The race fix is structural (ordering relative to lease release), not test-locked; the existing in-memory fakes don't model lease semantics so a regression would have to be caught in a future watchdog race-condition test. Co-Authored-By: Claude Opus 4.7 --- .../Queue/CriticalTableBackupQueueWorker.cs | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Backup/Queue/CriticalTableBackupQueueWorker.cs b/src/Backend/AutopilotMonitor.Functions/Services/Backup/Queue/CriticalTableBackupQueueWorker.cs index 6be5d21b..9e0c4377 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Backup/Queue/CriticalTableBackupQueueWorker.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Backup/Queue/CriticalTableBackupQueueWorker.cs @@ -321,9 +321,17 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue retryableException = ex; } - // ── Terminal CAS Completed (only on success path) ────── + // ── Terminal CAS, still UNDER LEASE ───────────────────── + // Codex-Hotfix Wave3 #1: BOTH the success-CAS (Running→Completed) + // AND the retryable-rollback (Running→Queued) must run before the + // lease is released. Otherwise the post-lease window + // "Running + free lease + stale heartbeat" lets the watchdog flip the + // job to Failed, and our subsequent rollback then re-reads the fresh + // ETag and unconditionally re-sets Queued, silently undoing the + // watchdog's decision. if (retryableException is null && result is not null) { + // Success path. var completedAt = _clock.GetUtcNow().UtcDateTime; var (finalJob, finalETag) = await _jobs.GetWithETagAsync(jobId, hct).ConfigureAwait(false); if (finalJob is not null && finalETag is not null) @@ -336,6 +344,35 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue await _jobs.TryUpdateWithCasAsync(finalJob, finalETag.Value, hct).ConfigureAwait(false); } } + else if (retryableException is not null) + { + // Retryable-rollback under lease. + var (rb, rbETag) = await _jobs.GetWithETagAsync(jobId, hct).ConfigureAwait(false); + if (rb is not null && rbETag is not null) + { + // Defence-in-depth state guard (Codex-Hotfix Wave3 #1): we + // expect the row to still be Running (we set it that way and + // hold the maintenance lease throughout). If a parallel actor + // somehow shifted the state, leave it alone instead of + // clobbering a non-Running decision back to Queued. + if (rb.State == BackupJobState.Running) + { + var retryNow = _clock.GetUtcNow().UtcDateTime; + rb.State = BackupJobState.Queued; + rb.BackupId = null; + rb.LastHeartbeatUtc = retryNow; + rb.QueuedAtUtc = retryNow; + rb.Error = retryableException.Message; + await _jobs.TryUpdateWithCasAsync(rb, rbETag.Value, hct).ConfigureAwait(false); + } + else + { + _logger.LogWarning( + "CriticalTableBackupQueueWorker: skipping retry rollback for jobId {JobId} — row no longer Running (now {State})", + jobId, rb.State); + } + } + } } } catch (OperationCanceledException) when (ct.IsCancellationRequested) @@ -389,23 +426,13 @@ namespace AutopilotMonitor.Functions.Services.Backup.Queue 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 = retryNow; - rb.QueuedAtUtc = retryNow; // Codex-Hotfix Wave1 #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); - } + // Rollback already done UNDER LEASE inside the leased section + // (Codex-Hotfix Wave3 #1). Just log + rethrow so the outer poll loop + // applies visibility-timeout retry semantics. _logger.LogWarning(retryableException, - "CriticalTableBackupQueueWorker: jobId {JobId} backup attempt failed — rolling to Queued for retry (dequeue {N})", + "CriticalTableBackupQueueWorker: jobId {JobId} backup attempt failed — rolled back to Queued for retry (dequeue {N})", jobId, msg.DequeueCount); - throw retryableException; // outer loop applies visibility-timeout retry + throw retryableException; } // ── OpsEvent on success (non-throwing per Wave25 #3) ──