Skip to content

Retry retryable ack failures instead of abandoning the work item (redux) - #72

Open
Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
mainfrom
pinodeca/retry-retryable-ack-failures
Open

Thom Chiovoloni (thomcc-work) wants to merge 6 commits into
mainfrom
pinodeca/retry-retryable-ack-failures

Conversation

@thomcc-work

@thomcc-work Thom Chiovoloni (thomcc-work) commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #41. Taking over for #42.

The worker dispatcher previously abandoned a work item on every acknowledgement error, including retryable provider failures. Abandonment discards the completed activity's result and redelivers the item, potentially running the handler again. That is permitted by at-least-once execution, but is avoidable for transient acknowledgement failures.

Acknowledgement behavior

  • Activity success and application-error completions: Retry retryable acknowledgement errors in place, using the same lock token and completion payload. There are at most five total attempts, with waits of 100, 200, 400, and 800 milliseconds. This is 1.5 seconds of cumulative backoff, excluding provider-call duration—not a wall-clock deadline.
  • A permanent error on the first attempt, or five retryable failures: Fall back to the existing abandonment path.
  • A permanent error after a retryable completion-acknowledgement error: Report an uncertain outcome, increment duroxide_activity_ack_uncertain_total once, and do not attempt abandonment or count a confirmed activity infrastructure failure. An earlier attempt may have committed, or the lock may have been lost. If uncommitted work remains, recovery relies on lock expiry; redelivery is not guaranteed.
  • Cancellation acknowledgements (ack_work_item(token, None)): Make a single attempt, without in-place retries. On failure, fall back to abandonment.

Lock renewal remains active while a normal completion's acknowledgement is pending. Retries do not extend the shutdown grace period: the default one-second grace can interrupt the retry sequence.

Activities still require at-least-once-safe behavior. Crashes, lost locks, and exhausted retries can cause re-execution. Poison-message handling, Provider trait signatures, persisted schemas, and SQLite pool configuration are unchanged.

Test coverage

The fault-injection tests check exact acknowledgement/abandonment sequences, lock tokens, completion payloads, and handler execution counts:

  • Success on the fifth and final attempt without re-executing the handler.
  • Five retryable failures followed by abandonment and successful redelivery.
  • A first-attempt permanent error followed immediately by abandonment.
  • Retried application-error completions without re-executing the handler.
  • Single-attempt cancellation acknowledgement with no completion payload.
  • Redelivery after a transient renewal failure followed by a transient cancellation-acknowledgement failure.
  • Uncertain outcomes both after a committed acknowledgement and before commit, with no abandonment. The latter case recovers through lock expiry.
  • An uncertainty count of one for each uncertain outcome, and zero for retried success and both confirmed-failure paths.

The reusable provider validation test_ack_work_item_duplicate_rejected acknowledges the same token twice and verifies a non-retryable second error, exactly one queued completion, and removal of the worker item. This checks the queue directly rather than relying on replay deduplication.

The acknowledgement fault-injection tests use file-backed SQLite so shared-cache deadlocks do not interfere with exact call-count assertions. The existing in-memory, multi-worker session test remains a separate regression check.

Checklist

  • Tests added/updated
  • Regular tests pass locally (cargo nt) — focused nextest selection; commands and results below
  • Doc tests pass locally (cargo test --doc --all-features)
  • Docs updated
    • Existing docs updated where behavior changed
    • New doc added under docs/ if introducing a new area — not applicable
    • Linked from docs/README.md — not applicable; existing documentation updated

Links / Design notes

Provider requirements

An acknowledgement must atomically remove the locked worker item and enqueue its completion. If it committed but its response was lost, repeating the acknowledgement with the same token must fail non-retryably without enqueueing another completion.

The provider guides and 0.2.0 provider notes document this requirement and the new validation helper. SQLite runs the validation here; external providers should register it after upgrading their Duroxide dependency.

SQLite diagnosis

The original issue and PR description attributed the shared-cache failure to SQLx not using SQLite unlock notifications. That explanation is incorrect for SQLx 0.8.6: its statement execution loop already calls sqlite3_unlock_notify, and the dependency enables unlock-notification support.

SQLite emits the reported database is deadlocked error when unlock-notification registration detects a wait cycle. Waiting longer cannot resolve that cycle. SQLite documents rollback and later retry of the transaction as the recovery mechanism. For this failure, the acknowledgement transaction is rolled back and the next provider call opens a fresh transaction.

This supports retrying the complete acknowledgement operation rather than rerunning the activity. The same retry policy applies to other providers' retryable failures, including connection failures whose commit outcome may be unknown.

Validation

Refreshed locally at 1e883a3:

./run-tests.sh \
  --test worker_reliability_test \
  --test sqlite_provider_validations \
  --test observability_tests \
  -E 'binary(worker_reliability_test) | test(/ack_work_item|ack_uncertain|metrics_snapshot/)'

Passed: 15 tests with all features and 15 without explicit feature flags. This was a focused run, not a fresh full-suite run.

cargo test --doc --all-features

Passed: 46 doctests; 74 ignored; none failed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation matches the documented retry policy and has comprehensive deterministic regression coverage.

0 open findings

What changed in this PR

Adds bounded retries for transient activity acknowledgement failures, reducing unnecessary activity re-execution while preserving at-least-once semantics.

Changes:

  • Retries acknowledgements up to five times with exponential backoff.
  • Adds fault-injection coverage for success, failure, cancellation, exhaustion, and uncertain commits.
  • Documents acknowledgement retry and atomicity requirements.
File Description
src/​runtime/​dispatchers/​worker.rs Implements acknowledgement retries.
tests/​common/​fault_injection.rs Adds retryable failure injection and call tracking.
tests/​worker_reliability_test.rs Adds acknowledgement retry scenarios.
tests/​session_e2e_tests.rs Aligns assertion with at-least-once execution.
docs/​provider-implementation-guide.md Documents retry and commit semantics.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The worker dispatcher abandoned an activity work item on any ack_work_item
error, which redelivers the item and re-runs the handler. The dispatcher poll
loop already branches on ProviderError::is_retryable() and backs off before
retrying fetch_work_item; the ack path ignored the classification entirely.

Add ack_work_item_with_retry: up to 5 attempts with exponential backoff
(100ms..1s, ~1.5s total) for retryable errors, then fall back to the existing
abandon path. Permanent errors still abandon immediately, so poison handling
and the existing failure tests are unaffected.

This is reachable from any provider that reports transient failures. It is
easiest to observe with SqliteProvider::new_in_memory, which uses
sqlite::memory:?cache=shared. Shared-cache mode uses table-level locks and
returns SQLITE_LOCKED on a read-to-write upgrade collision without invoking
the busy handler, so PRAGMA busy_timeout does not cover it.

Also relax test_multi_worker_heterogeneous_config, which asserted
`a + b == 3`. Activities are documented as at-least-once, so any redelivery
makes equality wrong; assert `a + b >= 3` instead.

Fixes #41

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Softened the uncertain-ack log so it no longer promises redelivery.
- Added duroxide_activity_ack_uncertain_total, counted once per uncertain outcome without inflating failure counters.
- Updated metric documentation and added an Unreleased entry in CHANGELOG.md.
- Restored MetricsSnapshot’s original fields and added MetricsProvider::activity_ack_uncertain(). Updated callers and documentation. A new exhaustive struct-literal test reproduced E0063 before the fix and now passes.
- Added the missing telemetry specification entry and assertions that both confirmed-failure paths leave uncertainty at zero.
@thomcc-work
Thom Chiovoloni (thomcc-work) force-pushed the pinodeca/retry-retryable-ack-failures branch from 9a6a321 to 71032ce Compare November 2, 2026 18:44

@tjgreen42 Todd J. Green (tjgreen42) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by GPT-6.1 Sol.

No actionable findings in this revision. Reviewed acknowledgement retry and lock-renewal lifetimes, cancellation/shutdown handling, duplicate acknowledgements, and uncertain commit outcomes against the provider contract. Leaving uncertain acknowledgements for lock-expiry recovery is an explicit documented/tested policy, rather than an accidental abandonment regression.

Validation: isolated nextest worker-reliability tests passed (10/10), and the duplicate-acknowledgement provider validation passed. Full suite not run. Reviewed head 1e883a3.

@tjgreen42

Copy link
Copy Markdown
Contributor

Review by GPT-6.1 Sol.

Please update the PR description to match the current implementation:

  • Cancellation acknowledgements (ack_work_item(token, None)) are attempted only once, not retried. On failure, the worker falls back to abandonment. The test summary should likewise describe single-attempt cancellation acknowledgement and redelivery after transient renewal/cancellation-ack failures.
  • A permanent error after a retryable completion-acknowledgement error is treated as an uncertain outcome: the worker records duroxide_activity_ack_uncertain_total and does not attempt abandonment. An earlier attempt may have committed; if uncommitted work remains, recovery relies on lock expiry. The current blanket statement that permanent errors use the abandonment path should distinguish first-attempt permanent errors from this case.

The code, tests, and provider documentation already describe these policies; this is a PR-description alignment request.

@tjgreen42 Todd J. Green (tjgreen42) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Worker abandons work items on retryable ack failures, causing redundant activity re-execution

4 participants