Port #3774 to main: retry transient key-value storage failures in every backend

[kmatasfp]

Ports #3774 to main.

Differences from the 1.5.x change

main already had a narrower version of this. retry_on_pool_timeout in storage/keyvalue/mod.rs retried connection-pool acquisition timeouts inside each SQL backend. RetryingKeyValueStorage supersedes it: it covers every backend, classifies more than pool timeouts, and sits above the namespace router, so it is removed here along with its three tests. scheduler_storage_retry and indexed_storage_retry are main's own, cover different subsystems, and are untouched.

main has an 18th KeyValueStorage method, compare_and_set_many, that 1.5.x has never seen. It is wrapped and retried, on the same footing as set_if_not_exists and for a reason of the same shape rather than because it is idempotent. A retry after an attempt that applied but lost its response reports false where true was the truth, because the comparison row now holds what this call itself wrote.

The argument for retrying it anyway is in a comment at the wrapper, and rests on its only caller. StreamSessionIndex::catch_up_inner discards the flag and loops, reloading the index after every attempt, with the comment "Reload after either winning the CAS or observing another executor's progress". A spurious false therefore takes the same branch a genuine loss takes, re-reads state this call had already written, and finds nothing left to do. Refusing to retry would instead surface a brief backend outage to the durable-stream index as a hard failure, which is the defect this change exists to remove.

Op::ALL in the decorator's test matrix is a hardcoded array; it goes from 17 to 18 so the new method is actually covered rather than silently skipped by the "every operation" tests.

One panic stays that 1.5.x removed, because main had already replaced it with something better: WorkerService::get keeps main's "the Create entry still proves the worker exists" branch. Worker::get_latest_metadata propagates a status that cannot be recomputed (1.5.x panicked there; main used to fold it into "no agent", see the review follow-ups below).

Not ported: remove_legacy_cached_status and legacy_status_key. main has no legacy cached-status key to remove.

golem-shard-manager/tests/persistence.rs was deleted on main by #3824, so its acquire_timeout: None hunk is dropped; the equivalent literal in tests/etcd_backed/persistence.rs gets it instead.

Review follow-ups

Four findings from review, none of which 1.5.x has either; they will need porting back.

Recovery no longer acknowledges an assignment whose activations failed. on_shard_assignment_changed used to log and skip a worker whose status could not be computed or whose restart failed, so a key-value outage that outlived the retry budget left a running agent stopped, unrecorded, until an unrelated invocation arrived. Both failures now fail the assignment. Ok(None) from the status computation (the oplog is gone, a delete raced the index read) still skips that one agent, the same distinction enum_workers_at_key draws. Failing the assignment is what hands the retry to something: the shard manager retries a failed assign_shards, and at startup the failure is fatal and the restart policy retries. Recovery is idempotent, so a re-run after a partial success is safe.

The test for it, shard_assignment_fails_when_a_recovered_worker_cannot_be_activated, fails only activation's read of the worker record while enumeration's succeeds. That needed two harness additions: Bootstrap::wrap_key_value_storage (identity by default) so a test can place a fault-injecting decorator above the retry decorator, and TestWorkerExecutor::retire_unloaded_worker, because an unloaded shell that is still cached in ActiveAgents is reused by activation without any storage read. FaultInjectingKeyValueStorage lives in storage::keyvalue::fault_injecting so the harness and the unit tests can share it; nothing configures it in production. It replaces the two hand-written 18-method doubles the branch had grown (FlakyKeyValueStorage in the decorator's tests, UnreachableKeyValueStorage in the worker service's), which are gone.

The per-worker decision is recovered_status, a pure function with its own unit tests, because the only thing that makes the status computation fail is a durable-stream payload download, which the harness cannot fault at the service level.

On the retry budget: every storage read on the recovery path goes through RetryingKeyValueStorage first, whose default of 15 attempts over roughly 93 seconds is sized to outlast an Aurora reader-to-writer or ElastiCache replica-to-primary promotion. Failing the assignment is what happens to an outage that outlived that budget. That is the same shape the oplog layers have (a bounded retry, then a fatal failure) at a lower cost, since a failed assignment is one RPC rather than a process exit.

A promise handle is not handed out until it is hydrated. poll registered the handle before reading the completion, so a concurrent poll could take the fast path and retain a handle that, if the first read then failed, nothing would ever hydrate. PromiseHandleInner gains a hydrated flag, set by a local completion or by a successful storage read; the fast path returns only hydrated handles, and an unhydrated one sends the poll through to its own read. concurrent_poll_survives_a_failed_initial_result_read pauses the first poll inside its result read, polls again while it is paused, then releases the first to fail.

Redis failover replies are retried. fred 10.1.0 folds READONLY, LOADING, MASTERDOWN and TRYAGAIN into ErrorKind::Unknown, which was never retried. Two parts: the workspace now enables fred's custom-reconnect-errors, so with its default set (CLUSTERDOWN, LOADING, READONLY) the client reconnects and replays the buffered command up to max_command_attempts before the caller sees anything; and what still reaches the storage layer is classified by reply prefix as NotAttempted, since a refusal means the server did not execute the command. Every other Unknown stays Other. The tests feed the exact reply strings Redis emits; fred's protocol module is private, so pretty_error itself cannot be called from a test.

Method validation propagates a failed lookup. validate_method_invocation folded a failed get_latest_metadata into None, which selects the deployed component revision, so a key-value outage could validate an agent pinned to an older revision against the wrong signatures and reject a method it has. method_validation_revision now returns Result and only KnownFresh or a real Ok(None) select the deployed revision. get_latest_metadata had the same fold one layer down (a status that could not be recomputed became Ok(None)) and now propagates it too, keeping Ok(None) for the oplog-gone case.

Verification

Workspace type-checks with --all-targets; cargo clippy -D warnings is clean on golem-worker-executor and golem-worker-executor-test-utils. golem-worker-executor lib tests: 1847 passed, 0 failed, 6 ignored (before the second round; the 49 tests in the modules that round touched pass, and clippy is still clean). The new integration test and the promise integration tests pass locally; each new test was run red against the code without its fix before the fix was put back.

All twelve config files are cargo make generate-configs output.

🤖 Generated with Claude Code

[netlify]

✅ Deploy Preview for golemcloud canceled.

Name	Link
🔨 Latest commit	0fe2f9c
🔍 Latest deploy log	https://app.netlify.com/projects/golemcloud/deploys/6aa2fad98407fd000871d0f4

[vigoo]

Agent review on behalf of vigoo. Four findings from reviewing the complete diff and relevant callers. Verification: 27 key-value tests and 49 related tests passed; an isolated Rust reproducer confirmed the promise-poll race. No live Redis/Postgres failover experiment was run.

[vigoo]

[P1] Keep failed recovery activations pending instead of acknowledging the shard

Returning this KV read failure now feeds the existing log-and-skip branch in on_shard_assignment_changed (durable_host/mod.rs:6024–6047). If the initial recovery scan succeeds but this second metadata read exhausts retries during get_or_create_running, recovery logs the failure and returns Ok(()). Startup/shard assignment therefore succeeds without restarting the worker, and nothing retries it until another external invocation. Previously this KV failure panicked under the production abort policy.

Retain and retry failed activations, or propagate the failure from recovery. Add a test where enumeration succeeds but activation's metadata lookup fails; the new recovery tests only exercise enumeration failures.

[kmatasfp]

Agreed, fixed in 3f50f16. Recovery now propagates both failures: a status that cannot be computed and an activation that fails. Ok(None) from the status computation (oplog gone, a delete raced the index read) still skips that one agent, the same distinction enum_workers_at_key already draws. Propagating fails the assignment, which is what hands the retry to something: the shard manager retries a failed assign_shards (WorkerExecutionError is retriable in its client), and at startup the failure is fatal and the restart policy retries. Recovery is idempotent, so re-running the scan after a partial success is safe.

Test: shard_assignment_fails_when_a_recovered_worker_cannot_be_activated in tests/api.rs. It needed two harness pieces: a Bootstrap::wrap_key_value_storage hook (identity by default) so a test can put a fault-injecting decorator above the retry decorator, and retire_unloaded_worker, because after a revoke the unloaded shell is still cached in ActiveAgents and activation reuses it with no storage read at all. The first run of the test found exactly that. The fault is keyed on the read_cached_agent_mode label and skips the first read (enumeration) so only activation's fails; the assertion checks the failure text is the restart's, not the scan's. Red without the fix (acknowledged: Some(Success(Empty))), green with it.

[vigoo]

[P1] Do not leave a published promise handle unhydrated after a read failure

The handle is inserted into the registry before this fallible read. While the read is pending, a concurrent poll can obtain and retain that handle through the fast path at lines 398–401. If this read then errors, subsequent polls keep returning the retained, incomplete handle without rereading storage—even when the promise already has a durable completion. The waiter can remain pending indefinitely. An isolated Rust reproducer using this PR's unchanged poll and registry implementations confirmed that three subsequent polls performed no storage reads after the failure.

Make initial hydration shared and retryable, and prevent fast-path success until it succeeds. Merely removing the registry entry on error would not repair handles already retained by other callers. Add a concurrent-poll test with a failed initial result read.

[kmatasfp]

Fixed in 3f50f16. PromiseHandleInner gains a hydrated flag, set by a local completion or by a poll whose storage read succeeded. The fast path returns a registered handle only if it is hydrated; otherwise the poll falls through to its own read. Concurrent polls still share one handle, whichever read succeeds first hydrates it, and a failed read leaves nothing behind that a later poll would trust: the failure is reported to that caller alone. Removing the registry entry on error would not have helped, for the reason you gave.

Test: concurrent_poll_survives_a_failed_initial_result_read. It completes the promise through a second service over the same storage, pauses the first poll inside its result read with a gate, runs a second poll while it is paused (which must come back ready), then releases the first to fail and checks a third poll. Red without the gate, green with it.

[vigoo]

[P2] Retry Redis server responses that represent failover

Fred 10.1.0's pretty_error maps READONLY, LOADING, and TRYAGAIN to ErrorKind::Unknown, so this fallback makes those transient responses permanently non-retryable. The workspace does not enable Fred's custom-reconnect-errors feature (it is not enabled by default), so READONLY/LOADING are not intercepted for reconnect/replay either. A demoted or loading Redis node can therefore cause an immediate hard failure instead of using the new failover retry budget.

Recognize these transient server responses and reconnect where required, without treating every Unknown error as transient. Add tests using actual server-response classification rather than only manually selected ErrorKind values.

[kmatasfp]

Fixed in 3f50f16, in two parts. The workspace now enables fred's custom-reconnect-errors, so with its default set (CLUSTERDOWN, LOADING, READONLY) the client treats those replies as a reconnect trigger and replays the buffered command up to max_command_attempts before the caller sees anything. What still reaches the storage layer is classified by reply prefix: READONLY, LOADING, MASTERDOWN and TRYAGAIN under ErrorKind::Unknown become NotAttempted, since each is a refusal (the server did not execute the command) and a retry cannot duplicate a write. Every other Unknown stays Other.

Tests feed the exact reply strings Redis emits (READONLY You can't write against a read only replica. and so on) rather than picked kinds. fred's protocol module is private, so pretty_error itself cannot be called from a test; the test says so. A negative test covers ERR, NOSCRIPT, EXECABORT, an internal channel error, and prefix lookalikes.

[vigoo]

[P2] Propagate metadata-read failures instead of validating against the deployed revision

.ok().flatten() discards the new storage error and turns it into the same None used for an absent agent. ComponentService::get_metadata(..., None) then selects the currently deployed component revision. For an existing inactive agent pinned to an older revision, a KV outage can consequently cause a valid method or signature to be rejected as a ProtocolError against the wrong revision, before execution is attempted.

Propagate the lookup error through method_validation_revision; use the deployed revision only for KnownFresh or an actual Ok(None). Add a failed-lookup test where the existing and deployed revisions have different method signatures.

[kmatasfp]

Fixed in 3f50f16. method_validation_revision returns Result<Option<_>> and the loader's error is propagated; only KnownFresh or a real Ok(None) select the deployed revision. One layer down, Worker::get_latest_metadata had the same fold (a status that could not be recomputed became Ok(None)), so a checkpoint read failure would still have ended at the deployed revision. It now propagates that error too and keeps Ok(None) for the oplog-gone case; the PR description is updated accordingly.

Test: may_exist_method_validation_propagates_a_failed_lookup. I stopped at the revision-selection seam rather than building a validate_method_invocation harness with two revisions of differing signatures: past that seam it is ComponentService::get_metadata(_, None) returning the deployed revision, which a failed lookup can no longer reach. Happy to build the end-to-end version if you want it.