Retry transient key-value storage failures in every backend

[kmatasfp]

The defect

A brief key-value backend outage aborted the worker executor. WorkerService::set_assignment_tracking panicked at both call sites when its write failed, and the failure reaching it was not a rejected query — it was a connection-pool acquisition timeout. Any stall on the pool longer than that timeout took the process down.

That matters more than an ordinary crash. An executor holding live agents forces every one of them to replay from oplog or snapshot when it dies, and that replay lands on the storage layer that has just finished recovering. Surviving a blip avoids the storm entirely.

Underneath, the two backends did not behave alike. Nothing in storage/keyvalue/ retried anything. Redis only looked like it retried, because fred is built with a ReconnectPolicy — that re-establishes connections, it does not re-run operations. The SQL backends had no equivalent at all.

Design

Retry is now a property of the KeyValueStorage interface rather than of any one backend.

A typed error. The trait returned Result<T, String>, which cannot be classified without matching message text. It now returns KeyValueStorageError with three variants describing retryability:

• NotAttempted — failed before reaching the backend (pool acquire timeout, refused dispatch). Cannot have been applied.
• Transient — may or may not have been applied (mid-operation I/O error, response timeout, dropped connection).
• Other — never retried.

SQL backends map via RepoError, Redis via fred::error::ErrorKind. No string matching. The labelled wrappers still hand callers a String, so service-layer call sites were unaffected.

A decorator, applied outermost. RetryingKeyValueStorage wraps Arc<dyn KeyValueStorage> and applies one RetryConfig to all 17 methods, installed above NamespaceRoutedKeyValueStorage. The guarantee is a property of the interface, so at the outermost edge a namespace routed to Redis and one routed to PostgreSQL get the same policy by construction — no backend, present or future, can opt out. Below the router, "identical" would be a thing to re-verify each time a backend is added.

One cost of that placement: the router records record_worker_kv_cache_value_size before delegating, so a retried write records its size more than once. That skews a histogram under fault conditions only.

The retry budget

Sized to outlast an AWS-side switchover, because those are the outages this exists to hide. Aurora promotes a reader to writer in high single-digit to mid-tens of seconds, worst case around a minute; ElastiCache Multi-AZ promotes a replica in around thirty. key_value_storage_retry defaults to 15 attempts, 200ms minimum, ×2, capped at 10s — roughly 93 seconds of backoff, up to ~106s with jitter. The cap keeps the tail responsive once the new writer starts answering.

The budget only works with the other half. A retry loop cannot make progress while a single attempt is parked waiting for a connection, and sqlx defaults that wait to 30s — longer than the failover being waited on. DbPostgresConfig::acquire_timeout is therefore new, as Option<Duration>:

• Unset leaves sqlx's default untouched, so every other pool in the workspace behaves exactly as before. Pools whose callers do not retry are better off riding out a load spike than failing six times sooner.
• The key-value pool alone falls back to 5s, because it is the one with a retry policy above it.

A failover refuses connections quickly, so backoff dominates; a partition blackholes them, so the acquire timeout dominates. The budget has to work for both shapes.

Retrying a non-idempotent operation

Every method was classified before being wrapped, since retrying blindly is how an availability fix becomes a correctness bug. Reads, set/set_many, del/del_many, and the set and sorted-set mutators are all idempotent: their results carry no "did it already exist" bit.

set_if_not_exists is the exception — its bool means "this call performed the write" — and it is deliberately retried anyway. That decision is the one place here trading an observable inaccuracy for availability, and it is argued in full in a dedicated comment with the evidence behind it. In short: no caller branches on the flag in a way the inaccuracy changes, the API already cannot promise an accurate flag because worker-service retries the whole call, and refusing to retry aborted the executor on the commonest failure class of one of the two backends.

Idempotence::NonIdempotent is retained with no users, as the vocabulary for a future method that genuinely cannot tolerate a repeated apply.

Panics

services/promise.rs now has zero panic! calls. services/worker.rs has three, none reachable from a key-value failure: component metadata, enrich_with_type, and "malformed oplog without create oplog entry" — the last deliberately kept, because a corrupted invariant is not an unreachable dependency.

Converting them moved several signatures to Result: PromiseService::create, and WorkerService::{get, get_agent_mode, read_status_checkpoint, get_running_workers_in_shards, remove, remove_cached_status, update_cached_status}. Worker::get_latest_metadata moved with them, and the change reaches golem-debugging-service, which consumes the trait but implements none of it.

One case is deliberately still fatal: the recovery scan reached during WorkerExecutorImpl::new. An executor that cannot read its running-worker index does not know which workers to resume, so it refuses to start rather than serve with an unknown recovery set, and the restart policy retries it. That costs nothing precisely because it is holding no agents — which is the same reasoning that makes the failure non-fatal once agents are running. The rationale is recorded at the call site.

Failures that must not be swallowed

Review found three places that logged a storage failure and carried on. Each sits where the recovery guarantee actually lives, so each now reports it.

WorkerService::update_cached_status returned Ok when the RunningWorkers index write failed. Both of its callers go on to make the worker runnable, so the call could hand back a running agent that no crash or reshard would resume, and the blob write that followed made it look accounted for. The index is written first now and its error returned.

WorkerActivator::activate_worker returned (), logging a failed metadata read and a failed activation alike. The scheduler read that as success and acknowledged the action, deleting the only thing left that would have woken the agent. It returns Result now, and both ScheduledAction::Resume and ScheduledAction::CompletePromise fail the action instead, so the lease expires and it is claimed again. A metadata read returning Ok(None) stays a success: no oplog means the worker was deleted, there is nothing to resume, and retrying it every tick forever would be the worse failure.

MultiSqliteKeyValueStorage creates each namespace's database on the path of the first operation that needs it, through a cache typed with String. Every initialization failure therefore reached the decorator as Other and was never retried. SqliteKeyValueStorage::configured and migrate return KeyValueStorageError now, classified by the same sqlx::Error predicate the query path uses. A transient cause is NotAttempted rather than Transient, because initialization runs before the operation: nothing the caller asked for was applied, so even set_if_not_exists is safe to retry through it.

Allocation

The decorator clones its by-value arguments per attempt, because each attempt consumes its copy and PostgresPool::with_tx's for<'f> FnOnce(&'f mut Tx) -> BoxFuture<'f, _> bound forbids the future from borrowing caller data. Cloning only on retry is not expressible: to have a value after a move you must copy before it, without knowing whether the attempt will fail.

So the payloads are refcounted instead, making the per-attempt copy free rather than conditional: Arc<AgentId> and Arc<str> inside KeyValueStorageNamespace, and Arc<[String]> for the batch key lists. Trait signatures are unchanged, so the higher-ranked bound is untouched and PostgreSQL still receives owned, 'static payloads.

This matters most on the guest-facing path. wasi:keyvalue get-many/delete-many accept a guest-supplied key list, and the UserDefined namespace is keyed by environment and bucket with no agent id — so it is many agents against one shared namespace. That path previously made three full copies of the key list per call (durability record, durability-loop iteration, storage attempt); it now makes one, the durability record, which needs an owned Vec to serialize into the oplog.

Tests

Unit tests drive a fake backend failing a configurable number of attempts: transient failure succeeding on retry, retry exhaustion returning rather than panicking, non-transient errors not retried, set_if_not_exists retried after a possibly-applied write, and a test pinning the price of that decision — a retry reporting false for its own write.

Coverage was then driven by mutation testing rather than judgement. retrying.rs went from 26 surviving mutants to 1 (a Debug::fmt mutant). Separately, cargo-mutants cannot generate arm-deletion mutants where there is no catch-all, so the error classification was invisible to it; 12 hand-crafted mutants across is_retryable, From<RepoError>, From<RedisError> and is_pool_timeout all survived before and are all killed now.

Verified: cargo check --workspace --tests; cargo test -p golem-worker-executor --lib (534 passed); cargo test -p golem-service-base --lib (77 passed); cargo clippy --tests -- --no-deps -Dwarnings; cargo fmt --all -- --check; and the container-backed key_value integration target (242 passed) against real Redis and PostgreSQL, which is what exercises the backend bodies and the fred key conversion.

Also included

A MoonBit toolchain version bump, unrelated to the above. The pinned release artifact 403s on download, failing build-golem-moonbit and it-cli on this branch and on 1.5.x alike.

Not fixed here

Tracked separately, each pre-existing and none made worse by this change:

• set_assignment_tracking has no test coverage, including an ephemeral-mode comparison whose inversion would put the wrong workers in the recovery index. Killing those mutants needs a DefaultWorkerService fixture with service doubles that do not exist in-crate.
• remove deletes the oplog before the key-value state, so a failed delete leaks orphaned entries. Reordering is not a safe swap: get writes on a cache miss, so a concurrent get would re-create exactly what was deleted. Note that the dangerous half is fixed here — an orphaned index entry used to panic the recovery scan on every start.
• The promise registry pre-registration in create is dead: the registry holds a Weak whose only strong reference is discarded immediately. The comment is corrected here; the behaviour is not.
• Nothing bounds the guest-supplied key list on the batch key-value API. The refcounting removes the retry multiplier, not the single-call ceiling.
• indexed/multi_sqlite.rs creates its databases lazily the same way, and SqliteIndexedStorage::configured still returns String, which init_storage maps straight to IndexedStorageError::Other. The identical hole, in a storage this change does not touch.

[kmatasfp]

Why set_if_not_exists now retries on Transient

The second commit reverses a deliberate decision from the first, so here is the reasoning in full. This is the one classification in the change that trades an observable inaccuracy for availability — please push back if you read it differently.

What changed

set_if_not_exists was classified NonIdempotent, so it retried only on NotAttempted. It is now Idempotent and retries on Transient like every other method. Idempotence::NonIdempotent is kept, with no users, as the vocabulary for a future method that genuinely needs it.

What that costs

A retry after an attempt that did write, but whose response was lost, reports false for a write this call performed. That is real, and a_retried_set_if_not_exists_reports_false_for_its_own_write pins it so it stays visible rather than becoming folklore.

Why it is worth paying

Nothing consumes the flag in a way the inaccuracy changes. PromiseService::create discards it outright — the statement ends in ;. PromiseService::complete uses it to choose between its own payload and a read-back of the stored one, and on a spurious false that read-back returns the payload this very call wrote. What is left is the completed bool on one gRPC response, and a skipped gauge decrement.

The API cannot promise an accurate flag anyway. worker-service retries complete_promise against the executor under its own worker_executor_retries. A response lost one layer up already produces exactly this false, today, without this PR. Declining to retry here defends an invariant that was never held.

The cost of not retrying is the defect this PR exists to remove. Connection loss classifies as Transient — see From<RedisError>, where every connectivity error kind except Backpressure lands there. So refusing Transient here meant both promise call sites aborted the executor on the commonest failure class of one of the two backends, and aborted it identically whether or not anyone read the flag.

How this was checked

Not by reading alone. A throwaway harness drove the real DefaultPromiseService through the real RetryingKeyValueStorage, against a backend that applies the write and then reports failure:

Call site	Classified as	Outcome
create()	Transient	executor aborted
complete()	Transient	executor aborted
create()	NotAttempted	survived, Ok(true)
complete()	NotAttempted	survived, Ok(false)

In that last row the waiter still received the correct payload, byte-identical to what was sent. The create() rows are the sharpest part: it discards the flag entirely, yet the old policy still aborted the executor to protect it.

What this does not fix

(Updated — this section originally said ~18 other panic! calls remained reachable from a key-value failure. Later commits on this branch fixed them, so it no longer applies.)

services/promise.rs now has zero panic! calls, and services/worker.rs has three, none of them reachable from a key-value failure:

• worker.rs:806 — component metadata, from the component service
• worker.rs:819 — enrich_with_type, pure
• worker.rs:895 — "Encountered malformed oplog without create oplog entry", deliberately kept: a corrupted invariant is not an unreachable dependency, and continuing would fold a status from an oplog with no Create entry

The recovery scan reached during WorkerExecutorImpl::new still returns Err and startup still fails when the key-value cluster is unreachable. That is deliberate rather than unfinished: an executor that cannot read its recovery index does not know which workers to resume, and refusing to start costs nothing because it is holding no agents. The restart policy retries it. The reasoning is recorded at that call site.

[vigoo]

We should not skip anything - I think not recovering a running worker is worse than retrying here forever, or something like that.

• if the entry is missing completely that means that the database is inconsistent and indeed i'd skip it (as there is nothing to resume)
• but if the entry is not missing, just unreadable due to a transient network issue, that's should not be taken the same way. if we don't resume it, the agent will remain suspended and we fail to deliver one of our primary properties - that resharding / executor crashes do not stop a running worker.
• if the error is transient then enough retries should fix it. if the error is not transient, then nothing else will work anyway, that's why I was originally panicking in these cases (better start a new executor that may work than have a disfunctional one)

[kmatasfp]

You're right, and the distinction you draw is the one the code was missing. Fixed in 607ce90.

Ok(None) still skips — no oplog means the index entry outlived the worker and there is nothing to resume. Err now fails the scan instead of skipping, because it is not evidence the worker is gone, only that we could not find out. Skipping stranded a running agent for as long as this executor owned the shard, silently.

Your point about transient-vs-not is what makes failing the right answer rather than a compromise: a transient cause is now absorbed by the storage retry budget before this line is ever reached, and a cause that outlives the budget leaves the executor unable to do its job — so replacing it beats carrying on with an unknown number of agents stranded. That is the same reasoning as your original panic; it just gets a longer runway first.

Worth noting the test suite passed unchanged when this was wrong, so there was no coverage of it at all. Added recovery_scan_fails_rather_than_stranding_a_worker_it_cannot_read, and confirmed it fails against the previous behaviour with Ok([]) — the empty list being exactly the silent stranding you describe.

[vigoo]

I feel like this is also wrong due to what I wrote in the previous comment. For me not being able to track the running agents seems to be fatal

[kmatasfp]

Agreed — reverted to fatal in fa12614.

My reasoning was that aborting drops every in-flight invocation and leaves the index just as stale, so logging was the lesser harm. That weighs the wrong things: a stale index is not merely equal harm, it is invisible harm, and it breaks the one property the executor exists to uphold. A crash is at least loud and recoverable.

What has changed is when it fires, not whether. Previously the panic came from the first pool acquire timeout, roughly 30 seconds in. Now the storage retry budget absorbs a transient blip first, so reaching this line means the write kept failing for the whole budget — at which point this executor genuinely cannot maintain its own recovery index, and your "better to start a new one that may work" applies squarely.

I have left update_cached_status returning Result rather than panicking, since both of its callers are in Result bodies and propagate — the failure is reported rather than swallowed there. Say the word if you would rather that were fatal too.

[vigoo]

Not sure about this one (and not even new code) but worth checking:

RepoError::is_transient recognizes only sqlx::Error::PoolTimedOut and sqlx::Error::Io. Backend error responses remain sqlx::Error::Database and are never retried. This excludes, for example:

• PostgreSQL shutdown/failover responses such as SQLSTATE 57P01–57P03
• connection exception class 08
• serialization/deadlock errors such as 40001/40P01
• SQLite BUSY/LOCKED

In particular, an Aurora switchover can terminate an established connection with a PostgreSQL FATAL response, so the advertised ~93-second failover budget may not be used at all.

[kmatasfp]

This was the important one — you're right, and it would have made the rest of the PR largely decorative. Fixed in 98875e3.

is_transient matched only PoolTimedOut | Io(_), so every sqlx::Error::Database fell through to "never retry". Your Aurora example is exactly the hole: the old writer answers with a FATAL and then closes the connection, so it arrives as a server response with a SQLSTATE rather than as a transport error. The ~93-second budget would not have been spent on the one event it was sized for.

is_transient now also accepts, for Error::Database:

• Class 08 in full — the connection-exception family.
• 57P01 / 57P02 / 57P03 — admin shutdown, crash shutdown, cannot-connect-now. All three are what a failover looks like from the client.
• 40001 / 40P01 — serialization failure and deadlock. Rolled back rather than half-applied, so a retry starts from the state the first attempt saw.
• SQLite BUSY / LOCKED, including extended codes, by masking to the primary result code.

One trap worth recording, because my own tests caught it and I would have shipped it otherwise. My first version discriminated SQLite from PostgreSQL by "does the code parse as a number". Plenty of SQLSTATEs are all digits, so 08000 was read as SQLite code 8000 and rejected — 57P01 only passed because the P made parsing fail. The discriminator is now length: a SQLSTATE is always exactly five characters.

is_pool_timeout is deliberately unchanged. It is the gate for retrying a non-idempotent operation, and only a pool timeout carries the "never reached the backend" guarantee — a server-reported error means the statement did reach it. There is a test pinning that widening one did not widen the other.

Six tests added against a fake DatabaseError, covering each class above plus the rejected-statement cases (23505, 42601, 42P01, 22001, and SQLite CONSTRAINT) that must stay non-transient.

Not new code, as you say — so this hole predates the PR and would have applied to the scheduler and indexed-oplog retry paths too, which use the same classification.

[vigoo]

I think this still violates the recovery guarantee from my previous comment. If tracking fails, we log it and can still return success after writing the status. This can activate a running worker that is missing from RunningWorkers and will not be recovered after a crash or reshard. This error should not be swallowed.

[kmatasfp]

You're right, and the log-and-continue here is a leftover from when this whole call was best effort. Fixed in a6751e8.

update_cached_status now updates the index first and returns its error. The blob write no longer runs after a failed index update, so nothing is left behind that makes the worker look accounted for. Both callers reach this from a path that returns Result and propagate it: the cold path in DefaultWorkerService::get where a recomputed status is reconciled, and worker creation in Worker::get_or_create_worker. The operation fails instead of handing back a worker that RunningWorkers does not list.

I also corrected the doc comment on set_assignment_tracking, which still said the index was not worth aborting over. That was the reasoning you rejected in the flusher, and it had no business surviving in the trait docs.

Tests: the existing test only pinned the swallow, so it now asserts the opposite (Err, tracking attempted once, blob not written). Added a second one for the blob write failure, because both failures now leave through the same return value and only one of them was covered.

[vigoo]

I think ScheduledAction::Resume below needs the same error handling. activate_worker logs metadata and activation failures and returns (), then the scheduler returns true, so it acknowledges and deletes the resume action even when activation fails. This can leave the worker suspended forever.

[kmatasfp]

Agreed, fixed in 85fe973.

WorkerActivator::activate_worker returns Result<(), WorkerExecutorError> now and SchedulerWorkerAccess mirrors it. A Resume whose activation failed returns false, so the action is never acknowledged: the lease expires and it is claimed again.

One distinction carried over from the recovery scan. activate_worker still succeeds when the metadata read returns Ok(None), because no oplog means the worker was deleted and no number of retries brings it back. A read that failed now propagates rather than being logged. Without that split, a resume for a deleted agent would be retried on every tick forever.

I applied the same handling to the CompletePromise branch above, which had the identical shape: the promise is completed, the activation failure is logged, the action is acknowledged, and whoever was waiting on the wakeup stays suspended. The retry re-runs complete against an already completed promise, which returns Ok(false) and writes nothing, and then attempts activation again. Say the word if you would rather I kept this to the branch you commented on.

What it costs: an action whose worker cannot be activated for a permanent reason, a component that no longer loads for example, is now retried every tick instead of being dropped. The scheduler has no dead letter path, and Invoke and ArchiveOplog already behave this way, so I followed them rather than inventing one here.

Tests: failed_resume_is_not_acknowledged (the action is still claimable once the lease expires), failed_activation_after_promise_completion_is_not_acknowledged, and successful_resume_is_acknowledged so the acknowledge path stays pinned too. Confirmed both failure tests fail against the previous behaviour, the resume one with an empty claim list, which is exactly the acknowledged and deleted resume you describe. The third passes either way, which is the point of having it.

[vigoo]

I think the lazy initialization here still bypasses the retry policy. storage_by_namespace returns String, which is converted to KeyValueStorageError::Other, so transient SQLite initialization failures are never retried. Can we preserve the typed error through initialization?

[kmatasfp]

You're right, and this was the one backend where initialization sits on the operation path. Fixed in a013464.

SqliteKeyValueStorage::configured and migrate return KeyValueStorageError now, and the cache in MultiSqliteKeyValueStorage is typed with it, so nothing round trips through String on the way to the decorator.

The classification reuses the query path's predicate rather than growing a second one. RepoError::is_transient now delegates to is_transient_sqlx_error(&sqlx::Error), which can also be applied to a sqlx error that never became a RepoError, which is what a pool or migration failure is. RepoError::is_transient itself behaves exactly as before.

A transient initialization failure is classified NotAttempted, not Transient. Initialization runs before the operation, so nothing the caller asked for was applied and even set_if_not_exists is safe to retry through it. Anything not known to be transient stays Other, so a bad path or a corrupt database fails at once instead of after the whole ~93 second budget.

Worth flagging one detail, because the obvious version of this is wrong. The classifier walks the whole error chain instead of calling downcast_ref: a migration failure arrives as a sqlx::migrate::MigrateError carrying the sqlx error as its source, and downcast_ref::<sqlx::Error>() returns None for it. transient_cause_is_found_through_a_wrapping_error asserts that None first and then the classification, so the shortcut cannot come back unnoticed.

Also worth recording that the retry has something to retry: Cache::get_or_insert caches only Ok and removes the pending entry on Err (failed_insert_does_not_cache_allows_retry in golem-common), so the second attempt re-runs initialization rather than replaying a cached failure.

Tests: three on the classification, plus lazy_initialization_failure_keeps_its_classification in multi_sqlite, which drives a real initialization failure through get (a directory sitting where the database file belongs, so SQLite cannot open it) and asserts the caller sees Other and no retry.

One more of the same shape, pre-existing and left alone: indexed/multi_sqlite.rs initializes lazily too, and SqliteIndexedStorage::configured still returns String, which init_storage maps straight to IndexedStorageError::Other. Same hole, different storage. I have added it to the PR description rather than widening this change.