state_managers: fix two read-only transaction participant bugs

[rjhuijsman]

Important

Written unattended by a robot (Claude Opus 5), and not yet reviewed by a human. The design was agreed with @rjhuijsman first; the code was not. It needs the author's review before it goes to peer review.

Two independent bugs in how read-only transaction participants reach a terminal outcome. Each one strands a participant holding its state's shared lock, and each has its own commit.

Bug 1: a re-sent Prepare is answered "no such transaction"

Two behaviours that are each correct on their own, and don't compose.

A read-only participant elides its prepare/commit. On the first Prepare it marks the transaction prepared and committed in memory, drops its participant entry and releases its shared lock immediately. It has nothing to persist, and the coordinator skips it during the commit phase, so it never expects to be contacted again. That is the whole point — it stops holding a read lock across the rest of two-phase commit.

The coordinator retries Prepare indefinitely on any RPC-level error. Also deliberate: a transport error says nothing about whether the participant prepared. Concluding "abort" from one would be wrong, because the participant may be sitting there prepared and a later recovery could commit the whole transaction. So definitive outcomes travel only in response fields, never in gRPC status codes.

Together: a Prepare whose response is lost gets re-sent to a participant that has forgotten the transaction precisely because it succeeded. The participant answers abort=True — which the coordinator does treat as definitive — and a transaction that prepared perfectly becomes an aborted one.

The codebase already knows this hazard in one place. The recovery re-prepare passes skip_read_only=True, with the comment "by then read-only participants may have already forgotten the transaction." Same problem, fixed on the recovery path only; the live per-participant retry loop had no equivalent.

Bug 1's fix

The coordinator tells each participant, in PrepareRequest, whether it recorded that participant as read-only. A participant asked about a transaction it no longer holds may then answer prepared — but only after its own restart detection confirms it has not restarted since the transaction began, which is the one way it could have lost the transaction rather than completed it.

Deliberately not done on the coordinator side by reinterpreting the response: abort=True keeps meaning exactly one thing, and prepare() needs no new logic. The party that owns each fact supplies it — the coordinator knows the read-only classification, the participant is the only one that can know whether it lost state.

Rolling upgrades degrade to today's behaviour in both directions, following the pattern abort_via_response and read_only_aware already set: an old participant ignores the new field and answers abort=True, which every coordinator already treats as definitive; an old coordinator never sets it, so a new participant behaves as it does today.

Bug 2: a participant is told to commit a transaction it never prepared

A coordinator writes its participant list to disk and fans Prepare out concurrently, so it can crash with the list durably recorded and a read-only participant's Prepare never sent. That participant stays joined, unprepared, holding its shared lock. The recovered coordinator re-prepares with skip_read_only=True — read-only participants may already have elided and forgotten the transaction — and then answers this one's Watch with "committed".

The database persists a participant transaction only once it is prepared and refuses to commit one that is not, so the commit fails: "Missing transaction for state type ..." with restart detection, "Txn not prepared" on the legacy path. The watch loop treats that as transient, backs off, asks again, gets the same answer, and repeats forever — so _complete_participant_transaction() is never reached and the lock is never released.

The fix is to abort instead of committing when the transaction was never prepared. That is the terminal outcome such a participant can still reach, and it is safe because a read-only participant has nothing to apply; one that did elide is already finished(), which makes the abort a no-op for it. Doing it participant-side rather than changing what Watch answers also keeps it working against an un-upgraded coordinator, and avoids relying on every member of Participants.read_only() being elidable — retain_as_read_only() can move a previously-committing participant into that set.

Tests

Five, in StateManagerTestCase. One reproduces the bug — it fails on main with AssertionError: True is not false. The other four pin down behaviour that must not change, and pass both before and after:

Case	Expected
First Prepare on a read-only participant	elides: entry dropped, lock released, abort=False
Re-sent Prepare after eliding	abort=False — bug 1's fix
Unprepared participant told to commit	aborts: entry dropped, lock released — bug 2's fix
Abort of an already-elided participant	no-op; stays committed, lock stays released
Re-sent Prepare, coordinator did not set read_only	abort=True
Re-sent Prepare, coordinator not read_only_aware	abort=True
Re-sent Prepare after a restart	abort=True, restart_detected=True
Re-sent Prepare with no restart detection (UUIDv4)	abort=True

Scope and what this does not settle

These came out of the concurrent_transactions_same_state MacOS-arm64 investigation; #158 is a third, separate bug and is independent (they touch different parts of state_managers.py and do not conflict).

The evidence that this is the mechanism behind that hang is strong but circumstantial, and the hang is not reproduced here: the 18 failing transactions in run 34212468575 are all UUIDv7-stamped in the same millisecond and their failed Prepares land nine seconds later, which is a retry ladder rather than a first attempt; and those RPCs cross a local Envoy (the test log shows execute program 'envoy'), so one transport event fails all 18 in-flight responses at once. The hang only reproduces on MacOS arm64 at roughly 1 in 3, and no MacOS runner was available here.

One residual limit is called out in a comment at the new branch: the restart check is per process. Shards are fixed when a state manager is constructed, so a shard changes owner only via a new process, which the recovery timestamp catches. Reassigning a shard to an already-running server would need a per-shard recovery timestamp.

🤖 Generated with Claude Code

https://claude.ai/code/session_014L8UDXfhAJNKDLuEHN42AX

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue-ready label.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.