gsadmin/AutopilotMonitor
1
0
Fork 0
mirror of https://github.com/okieselbach/AutopilotMonitor.git synced 2026-10-24 12:42:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(backend/backup): perform retryable rollback under lease, with strict state guard

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-05-26 15:53:05 +02:00
parent c89f33be54
commit 149f447281
1 changed files with 43 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Backend/AutopilotMonitor.Functions/Services/Backup/Queue/CriticalTableBackupQueueWorker.cs
+43 -16
View File
@@ -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) ──