Repository navigation
fix(runtime): do not report a poison disposition that never committed (redux) - #73
Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
Conversation
e7057e6 to
378fb18
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Poison failures propagated through orchestration execution are still counted before their acknowledgement succeeds, and compatibility/documentation issues remain.
3 open findings
What changed in this PR
Corrects orchestration poison metrics so acknowledgement failures are not reported as confirmed poison dispositions.
Changes:
- Records confirmed and failed poison acknowledgements separately.
- Adds structured error logging and regression coverage.
- Documents the new metrics and acknowledgement uncertainty.
| File | Description |
|---|---|
src/runtime/dispatchers/orchestration.rs |
Records poison metrics from acknowledgement results. |
src/runtime/observability.rs |
Adds the failed-poison counter. |
src/runtime/mod.rs |
Exposes failed-poison metric recording. |
tests/poison_disposition_tests.rs |
Tests success, failure, recovery, logs, and exports. |
tests/common/fault_injection.rs |
Adds acknowledgement fault injection. |
tests/reliability_tests.rs |
Makes duplicate-event batching deterministic. |
docs/observability-guide.md |
Documents poison acknowledgement counters. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
Outcome accounting is consistent, compatibility is preserved, and focused regression coverage supports the change; tests were not executed during this review.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
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>
6750323 to
d3081e7
Compare
Use 5s lease, with bounded 30s wait.
Todd J. Green (tjgreen42)
left a comment
Todd J. Green (tjgreen42)
left a comment
There was a problem hiding this comment.
Review by GPT-6.1 Sol.
No actionable findings in this revision. Reviewed poison-disposition accounting across direct and propagated poison, acknowledgement retries, lost acknowledgement responses, fallback acknowledgement, and redelivery. The current revision addresses the earlier compatibility, accounting, and reliability-test concerns.
Scope: static review of the diff and relevant runtime/provider/observability code and regression tests at ed64587. I did not run tests locally.
|
Review note from GPT-6.1 Sol: the PR description accurately explains the core acknowledgement-outcome reporting, but it does not fully reflect the current diff.
The description’s statements about single-threaded 1×1 execution and bounded condition-based waits remain accurate. The latest commit retains the production lease duration and extends the bounded wait to 30 seconds to avoid unrelated acknowledgement failures under load. |
Summary
Taking over from #49.
Correct orchestration poison reporting so that an acknowledgement error is not counted as a successful poisoning. Previously, poison accounting could happen before acknowledgement or regardless of its result.
The shared acknowledgement path now records the final result after retry handling, covering:
duroxide_orchestration_poison_totalduroxide_orchestration_poison_failed_totaland log at ERROR with the instance, poison attempt count, configured maximum, and provider errorEach final acknowledgement result increments one counter, not once per retry. A transient failure followed by a successful retry counts only as confirmed. A failed poison acknowledgement followed by a successful infrastructure-failure fallback does not count as confirmed poisoning.
The diagnostic deliberately says termination was not confirmed, rather than asserting that it did not occur. A write may have committed before its response was lost, or another dispatcher may have completed the work after lease expiry. These counters describe observed acknowledgement outcomes, not an exact ledger of durable terminations.
The public
MetricsSnapshotshape remains unchanged. The new failed count is available separately throughMetricsProvider::orchestration_poison_failed()on the existing observability handle.Tests and reliability
The new poison-disposition suite contains 18 tests, covering:
MetricsSnapshotstruct-literal compatibility.Async poison tests run in single-threaded 1×1 mode. They retain the production five-second lease and use bounded, condition-based waits of up to 30 seconds, avoiding unrelated lease-expiry failures under load.
The reliability tests also make duplicate-event batching deterministic. The crash-recovery case explicitly fetches a batch and leaves its lease unacknowledged before restarting, verifies that fetch did not append history, then checks that recovery records both arrivals and exactly one completion.
Both metrics references and the changelog are updated.
Checklist
cargo nt)cargo test --doc --all-features)docs/if introducing a new areadocs/README.mdLinks / Design notes