Repository navigation
fix(mongodb): a retried commit finishes the writes an interrupted one left behind - #659
Conversation
|
[review] 3 findings: 1 Important, 1 Nit, 1 Pre-existing (Deep, per REVIEW.md)
No weakened tests: 117 lines added, none removed. |
…status of a newer one (#656) Red: the replay puts Succeeded back over the newer run's Failed.
|
[review] Follow-up on finding 1 (Important): fixed, not just documented. The earlier claim that a guard needed a schema change was wrong: |
|
|
||
| // The recurring status belongs to the latest run: a retry that arrives after a newer run of the same | ||
| // recurring job must not put the older run's status back. A purged job has no CreatedAt to compare. | ||
| var recurringJobId = await HasNewerRunAsync(storedJob, result.RecurringJobId, ct).ConfigureAwait(false) |
There was a problem hiding this comment.
[review] Important — a late duplicate commit could overwrite the recurring job's LastExecutionStatus written by a newer run.
There was a problem hiding this comment.
Fixed in 37911b0 (guard in HasNewerRunAsync, red test then green, 171 Mongo tests pass). My first answer, that it needed a schema change, was wrong: JobDocument already stores RecurringJobId. Not guarded: a job removed by PurgeOnSuccess, which has no CreatedAt to compare; stated in delivery-guarantees.md.
| : result.RecurringJobId; | ||
|
|
||
| // A purged success leaves no document, so a missing job counts as Succeeded. | ||
| if (result.Succeeded && (storedStatus is null or JobStatus.Succeeded)) |
There was a problem hiding this comment.
[review] Nit — result.Succeeded && storedStatus is null or ... had correct precedence but was easy to misread.
There was a problem hiding this comment.
Fixed: the condition is parenthesized.
| await ApplyFailureTailAsync(result, result.RecurringJobId, ct).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private async Task ApplyFailureTailAsync(JobExecutionResult result, string? recurringJobId, CancellationToken ct) |
There was a problem hiding this comment.
[review] Pre-existing — ApplyRetryAsync / ApplyFailureAsync: does the same early-return gap exist outside the success path?
There was a problem hiding this comment.
Resolved, no change needed for the retry path: ApplyRetryAsync leaves the job Scheduled (not terminal) with one write, so the early return never applies. ApplyFailureAsync had the same gap and is fixed here (ApplyFailureTailAsync). Dead-letter runs in the executor, outside storage.
|



Summary
Fixes #656.
CommitJobResultAsyncreturned early when the job was already terminal, so a retry after an interrupted commit skipped the continuation release and the recurring status. It now runs those idempotent tails when the stored status matches the result outcome (Succeeded or missing job for a success, Failed for a final failure).Testing
Checked, no change needed
ApplyRetryAsync: leaves the job Scheduled (not terminal), one write, the early return never applies.ApplyFailureAsync: same gap, fixed here. Dead-letter runs in the executor, not in storage.Risk and rollback