Leave a trace for every lost capture claim - #2067
Merged
Merged
Conversation
After #2058, AgentRunLogCaptureRecoveryService still had three paths that dropped the cause of a claim it could not settle. RecoverClaimAsync turned any exception from the recovery step into a typed retry, recovery-operation-exception, and discarded the exception. A deterministic fault then repeats on every retry until the intent is exhausted into ExternalStateIndeterminate, and nothing says why. It is now logged as a warning in the shape #2058 uses: run, intent, the exception, and the SQLSTATE and message text of any PostgresException in its chain, plus the retry the step's outcome became. The line is written before the settlement runs, so it names that outcome without promising it: on the last allowed attempt the settlement exhausts the intent instead, a changed worker fence supersedes it, and a lost lease writes nothing, and the line says so rather than "recovered again once the retry falls due". SettleAsync returned a lost lease with no log line when its lease re-check failed and when its write matched no row (DbUpdateConcurrencyException). Each exit now logs a warning naming the run, the intent, the outcome it discarded, and why: lease-expired, reclaimed, row-version-changed, or claim-row-missing (which the ON DELETE RESTRICT key and the DELETE guard make unreachable today). The concurrency exit names the write it discarded, which the settlement may already have replaced with an exhausted or superseded outcome, not the outcome it observed. The level is Warning, not Information: the constructor makes a lease outlive both bounded steps of a claim, so a live worker loses one only when a step overruns the bound its cancellation sets, the process stalls, or the row changes under the settlement's own FOR UPDATE lock. None of that is routine. Each loss costs a re-claim and another recovery attempt, and for a legacy intent that attempt counts toward exhaustion. It is not an Error either, because the fence keeps the loss safe. Nothing read the LostLease tally, because the recurring job discards the reconcile summary. ReconcileAsync now logs the summary at Information when its wave claimed anything, as AgentRunReconcilerService and AgentRunSpoolReaper log theirs, so the job and its handler stay thin dispatchers. Every path still returns the same settlement or retry as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RecoverClaimAsyncturned any exception from the recovery step into arecovery-operation-exceptionretry and dropped the exception. It is now logged as a warning with the run, the intent, the exception and any SQLSTATE and message text. The line comes before the settlement runs. So it names the step's outcome without promising it: the settlement may exhaust the intent (recovery-exhaustedon the last allowed attempt), supersede it (worker fence changed), or not settle it at all.SettleAsyncused to return a lost lease without logging anything, both when its lease re-check failed and when its write matched no row. Each exit now logs a warning with the run, the intent, the outcome it discarded and a cause:lease-expired,reclaimed,row-version-changedorclaim-row-missing. Therow-version-changedline names the write it discarded (possibly already exhausted or superseded), not the outcome it observed.LostLeasetally.ReconcileAsyncnow logs the summary at Information when its wave claimed anything, asAgentRunReconcilerServiceandAgentRunSpoolReaperdo. Every path still returns the same settlement or retry as before.Test plan
A_recovery_step_that_fails_outside_the_database_..._its_settlement_decides_the_retryis a Theory overMaxAttempts8 and 1. In both cases the intent settles asExpected/recovery-operation-exceptionorExternalStateIndeterminate/recovery-exhausted, and the single line names the step's outcome without promising a retry.A_settlement_whose_row_changed_under_its_lock_is_logged_with_the_write_it_discarded_...is a Theory overMaxAttempts8 and 1. It fails if the concurrency catch logs the observed outcome instead of the write it discarded (checked by mutation).AgentRunLogCaptureRecoveryFlowTests32/32).