fix(runtime): do not report a poison disposition that never committed - #49
Open
waldemort-auto[bot] wants to merge 1 commit into
Open
fix(runtime): do not report a poison disposition that never committed#49waldemort-auto[bot] wants to merge 1 commit into
waldemort-auto[bot] wants to merge 1 commit into
Conversation
When a message exceeds max_attempts the runtime marks it as poison. The poison marking, the backoff, and the attempt-count increment all commit through a single ack keyed on the message's current lock token. If that lease has expired the ack fails and all three are lost together, so the message is never terminated -- it becomes visible again and is redelivered for as long as the provider keeps handing out dead leases. The runtime reported that outcome as a success: - The ack result was discarded with `let _ = ...`, so a failed disposition was invisible. Operators saw an 'exceeded max attempts, marking as poison' warning followed by a separate, transient-looking 'Invalid lock token' warning, with nothing connecting them or saying the message was still circulating. - record_orchestration_poison() was then called unconditionally, so duroxide_orchestration_poison_total counted poison *attempts* rather than messages actually taken out of circulation. In the incident behind #46 that meant tens of thousands of increments while zero messages were ever poisoned. Both poison paths (corrupted-history and normal) now route their ack result through record_poison_disposition, which records the poison metric only when the ack committed and otherwise logs at ERROR -- naming the instance, the attempt count, the underlying error, the fact that the orchestration was NOT terminated, and where to look (a provider handing out leases that are expired on arrival). A new duroxide_orchestration_poison_failed_total counter, surfaced as MetricsSnapshot::orch_poison_failed, makes the condition alertable. The runtime cannot force a terminal write without a valid lease: both ack_orchestration_item and abandon_orchestration_item require one by contract. Closing that gap would need a lease-independent provider operation, which is an API decision for the Provider trait and is left out of this change deliberately. tests/poison_disposition_tests.rs covers both directions: a disposition that cannot commit must not be counted as a poisoning, and one that does commit must be counted exactly once and report no failure. The second test exists so the first cannot be satisfied by simply never recording the metric. PoisonInjectingProvider gains expire_orchestration_lease(), which makes every orchestration ack fail with a non-retryable 'Invalid lock token' -- the condition a provider produces when the lease it handed out was already dead. Verified with the repository's own two-pass suite (./run-tests.sh): 1116 tests run, 1116 passed, 0 failed. With the fix reverted, the failed-disposition test fails as expected. Refs #47, #46 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
@waldemort-auto[bot] please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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
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.
Refs #47. Core half of the livelock analysed in #46.
Problem
When a message exceeds
max_attemptsthe runtime marks it as poison. The poison marking, the backoff, and the attempt-count increment all commit through a single ack keyed on the message's current lock token. If that lease has expired, the ack fails and all three are lost together, so the message is never terminated — it becomes visible again and is redelivered for as long as the provider keeps handing out dead leases.The runtime reported that outcome as a success:
The ack result was discarded (
let _ = ...), so a failed disposition was invisible. Operators sawTwo adjacent lines at the same level, with nothing connecting them. The first reads as "the runtime is handling it"; the second reads as a transient. Together they mean permanently unrecoverable, and nothing said so.
record_orchestration_poison()was called unconditionally afterwards, soduroxide_orchestration_poison_totalcounted poison attempts rather than messages actually taken out of circulation. Withattempt_count=78874againstmax_attempts=10, that counter read tens of thousands while zero messages were ever poisoned. A dashboard showing a large poison count for messages that are still circulating is worse than no metric at all.Changes
Both poison paths — corrupted-history and normal — now route their ack result through
record_poison_disposition:duroxide_orchestration_poison_total, as beforeduroxide_orchestration_poison_failed_total, do not record a poisoningThe error log names the instance, the attempt count, the underlying error, the fact that the orchestration was not terminated and will be redelivered, and where to look — a provider handing out leases that are already expired on arrival. Actual output from the test suite:
MetricsSnapshotgainsorch_poison_failedso the condition is assertable in tests and alertable in production.What this deliberately does not do
It does not make terminal disposition survive a dead lease. It cannot: both
ack_orchestration_itemandabandon_orchestration_itemtake a lock token, and the trait requires providers to reject an invalid one (providers/mod.rs, "Invalid lock tokens MUST return an error"). Forcing termination would need a lease-independent provider operation — aProvidertrait addition affecting every implementation, including third-party ones. That is an API decision for the maintainers, not something to slip into a bug fix, so this PR makes the failure honest and visible and leaves the trait alone.Worth noting what that means in practice: with a provider that returns live leases, this path is already self-correcting. Each redelivery carries a fresh valid token, so the poison disposition commits on the next attempt. The unbounded case only arises when a provider persistently hands out expired leases — which is the defect fixed in microsoft/duroxide-pg#22.
Tests
tests/poison_disposition_tests.rscovers both directions:failed_poison_disposition_is_not_counted_as_poisoned— every ack fails with a non-retryableInvalid lock token; assertsorch_poison == 0andorch_poison_failed > 0.successful_poison_disposition_is_counted_once— healthy lease; asserts the orchestration endsFailed,orch_poison >= 1, andorch_poison_failed == 0.The second test exists so the first cannot be satisfied by simply never recording the metric.
PoisonInjectingProvidergainsexpire_orchestration_lease(), which makes every orchestration ack fail as a provider does when the lease it handed out was already dead. Combined with the existing attempt-count injection, this reproduces the #46 steady state in-process, with SQLite and no external database.Verification — the repository's own two-pass suite (
./run-tests.sh, matching CI):With the fix reverted (metric recorded unconditionally, result discarded),
failed_poison_disposition_is_not_counted_as_poisonedfails and the other passes — so the test demonstrates the defect rather than merely accompanying the change.Relationship to #46
#46 is one failure with two owners. The provider side — a lease computed from a pre-wait timestamp, plus a blocking advisory lock that made the wait long enough to matter — is microsoft/duroxide-pg#22. This is the core side. Neither alone is the whole story: the provider fix stops expired leases being produced, and this stops the runtime from silently mis-reporting when it is handed one anyway, by any provider.