Retry retryable ack failures instead of abandoning the work item - #42
Open
pinodeca wants to merge 1 commit into
Open
Retry retryable ack failures instead of abandoning the work item#42pinodeca wants to merge 1 commit into
pinodeca wants to merge 1 commit into
Conversation
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
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.
Fixes #41.
Problem
The worker dispatcher abandoned an activity work item on any
ack_work_itemerror, ignoring the error's retry classification. Abandoning redelivers the item and re-runs the handler, so a few milliseconds of database contention cost a full redundant activity execution.The dispatcher poll loop in the same file already gets this right for
fetch_work_item— it branches one.is_retryable()and backs off. The ack path did not.Changes
src/runtime/dispatchers/worker.rs— addack_work_item_with_retry: up to 5 attempts with exponential backoff (100ms → 1s, ~1.5s total budget) for retryable errors, then fall back to the existing abandon path. Used by the success, application-error, and cancellation completion paths.Permanent errors still abandon immediately, so poison handling and
worker_abandon_on_ack_failure_enables_retryare unaffected.tests/session_e2e_tests.rs—test_multi_worker_heterogeneous_configasserteda + b == 3, i.e. exactly-once activity execution. Activities are documented as at-least-once (docs/durable-futures-internals.md), so any redelivery makes equality wrong. Relaxed toa + b >= 3; the existingb >= 1assertion still carries the test's actual intent (overflow sessions reach worker B).tests/common/fault_injection.rs—FailingProvidercould only inject permanent ack failures. Addedfail_next_ack_work_item_retryable(n)plusremaining_retryable_ack_failures()so tests can distinguish "abandon" from "retry in place".tests/worker_reliability_test.rs— newworker_retries_retryable_ack_failure_without_reexecuting_activity. Injects 3 retryable ack failures, then asserts the orchestration completes, all 3 injections were consumed, and the activity ran exactly once.worker_lock_timeoutis 30s so a redelivery could not come from lock expiry.Verified non-vacuous — reverting only the
worker.rschange fails it:Why this showed up as a flake
SqliteProvider::new_in_memoryusessqlite::memory:?cache=shared. Per the SQLite docs, shared-cache mode uses table-level locks and returnsSQLITE_LOCKEDon a read→write upgrade collision — without invoking the busy handler ("If a required table lock cannot be obtained, the query fails andSQLITE_LOCKEDis returned to the caller"). So the provider'sPRAGMA busy_timeout = 60000gives zero protection against this specific error; recovering properly needssqlite3_unlock_notify, which sqlx doesn't wire up.test_multi_worker_heterogeneous_configruns 2 runtimes × (2 worker + 2 orchestration) dispatchers = 8 concurrent pollers against one shared cache with a 5-connection pool, so it hit this readily: 2/8 failures in-suite and 1/5 in isolation before this change.Note the shared-cache path is only used by
new_in_memory— file-backed deployments use WAL and getSQLITE_BUSY, which is covered bybusy_timeout. TheSQLITE_LOCKEDtrigger is therefore test-infrastructure-only, but the abandon-on-retryable-error behavior this PR fixes applies to any provider.I did not change the shared-cache configuration here. SQLite documents shared-cache as obsolete and discourages it, so that's worth revisiting, but it's a larger change to the provider's concurrency model and doesn't belong in this fix.
Testing
./run-tests.sh— both passes green (1109 with--all-features, 1106 without), exit 0cargo test --doc --all-features— 46 passedcargo clippy --all-targets --all-features— no new warningsMeasuring the runtime fix specifically
Simply re-running the suite after this change proves little, because relaxing the assertion to
>= 3makes that test unable to fail from a redelivery. To isolate theworker.rsfix, I kept the strictassert_eq!(a + b, 3)(exactly-once) assertion, applied only the runtime change, and ran 20 iterations with--success-output immediate:SQLITE_LOCKEDevents741
SQLITE_LOCKEDevents across the 20 runs confirms the contention was still occurring, and the new retry path fired 19 times — each one a work item that would previously have been abandoned and re-executed. Against the pre-fix rate of 3/14 (21%), 0/20 gives p ≈ 0.008.The relaxed
>= 3assertion is still the correct one to commit, since at-least-once means the runtime cannot guarantee equality under lock expiry. The strict version was used only as a measurement probe.