Skip to content

ECOHERENCE shares 503 with not-applied errors, so retrying double-applies a write #175

Description

@Hazzng

Summary

ECOHERENCE and ELOCKTIMEOUT/ELOCKLOST all surface as 503, but they mean opposite things about durability:

  • ELOCKTIMEOUT / ELOCKLOSTnothing was applied (the breaker throws before fn runs; F2-L1 rolls back on lease loss). Safe to retry.
  • ECOHERENCEthe write was applied, only the version publish failed. Retrying a non-idempotent script double-applies it.

A client seeing 503 cannot tell them apart without parsing code, and the two pieces of documentation disagree about which behaviour is correct.

Proven, not inferred

Made only the version INCR fail (version key set to a non-integer, lock evals left healthy). Each probe ran echo L >> /counter.txt: 1 healthy probe (200) + 6 poisoned probes (all 503 ECOHERENCE). After repairing the key:

wc -l < /counter.txt  ->  7

Every 503-ECOHERENCE exec had already committed its mutation, and the client got no stdout back.

The contradiction

  • src/api/session-manager.ts:1105 says: "ECOHERENCE: write committed but version publish failed; client should retry"
  • src/api/openapi-spec.ts:72 says: "Never causes retry on 503 ECOHERENCE for write execs"

One of these is wrong, and the message is the dangerous one — it tells clients to do the thing that double-applies.

Two amplifiers found alongside it

  1. A pure read can receive it. publishVersionIfDirty is reached on the exclusive path, and session-manager.ts:1085 is if (!dirty && !session.publishPending) return; — so once publishPending is set, a turn that mutated nothing still attempts the INCR and 503s. A GET /files then returns "write committed but version publish failed", which is false for that request.
  2. A Postgres load spike exactly when Redis is degraded. :1099 sets lastSeenVersion = -1, so every subsequent request on that session does a full loadAllPaths + content prewarm. Confirmed by path_snapshot_miss + content_prewarm_ok on every poisoned request.

publishPending does clear on the next successful INCR (it is not sticky past recovery), but for a persistently failing INCR it never clears and the sandbox is wedged indefinitely.

Recommended fix

  1. Split the contract. Keep 503 + ELOCKTIMEOUT/ELOCKLOST as retryable-not-applied. Give ECOHERENCE a distinct, documented "applied, do not blindly retry" semantic, and reconcile openapi-spec.ts:72 with it. Fix the message so it stops instructing a retry.
  2. Never 503 a request that mutated nothing. When !dirty && publishPending, attempt the INCR but on failure leave it to the background drainer instead of throwing.
  3. Recover from a poisoned key. Treat a non-transient INCR error (ERR value is not an integer, WRONGTYPE) as poisoned: reset it or tear the session down, rather than wedging the sandbox forever. A fresh session reloads from Postgres and is guaranteed-correct.

Provenance

Found during pre-merge load testing of #162. Not introduced by that PR. Closely related to #167 (the connection coupling that makes the INCR fail in the first place).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:distributedDistributed locking / caching / coherencebugSomething isn't workingseverity:highHigh severity

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions