state_managers: shield a transaction's outcome from its waiters - #158
Conversation
Current Aviator status
This PR was merged using Aviator (commit 0f567bf).
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.
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
4d1373c to
78a85c9
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
This pull request failed to merge: merge conflict detected, please resolve manually and requeue. After you have resolved the problem, you should remove the Additional debug info: Failed to rebase this PR onto the latest changes from its base branch. You will probably need to rebase it manually and resolve the conflicts. |
78a85c9 to
5910f88
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Keeps two phase commit's invariant that a participant transaction is
resolved exactly once, so a participant always releases its state's
lock and drops its participant entry instead of abandoning both.
Before this change, `StateManager.Transaction.__await__` awaited the
shared `_committed` future directly. `asyncio` cancels the future a
task is suspended on when that task itself is cancelled, so any
cancelled waiter -- `_load()` waiting on an ongoing prepared
transaction, or a duplicate idempotent call waiting on the transaction
it matched -- took the transaction's own outcome down with it.
`finished()` then reported the transaction done while `commit()` and
`abort()` could no longer resolve it, and the next `set_result()`
raised from inside a section the framework marks exception-intolerant:
##### WOW! YOU'VE FOUND A BUG IN REBOOT! #####
Raised exception in critical exception-intolerant Abort section:
<class 'asyncio.exceptions.InvalidStateError'>: 'invalid state'
That raise lands before `_complete_participant_transaction()`, so the
participant entry stayed in `_participant_transactions` and the state's
`Lock` kept a holder for the life of the process, leaving every later
exclusive acquisition on that state waiting forever.
Awaiting `asyncio.shield(self._committed)` keeps a cancellation with
the waiter it belongs to, which is what the coordinator's participants
future already does.
- Add `TransactionTest`, covering both the cancelled waiter and the
waiters alongside it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014L8UDXfhAJNKDLuEHN42AX
Extends the previous commit's reasoning to the transaction's other shared future, so that neither of the two futures a concurrent call may be parked on can be resolved by a party that was only reading it. Before this change, a call that found another call in the same transaction already started on this state awaited `transaction.acquired_lock` directly. That future is shared with every other concurrent call on this state, and is resolved by `_transaction_participant_start()` once the per-state lock has been acquired (or with the acquire's exception). A cancelled waiter therefore cancelled it for everybody, and the resolution that followed raised `InvalidStateError` from a path with no handling for it. This is the same hazard as `_committed`, reached through a different future: the regression tests for that one cover the mechanism, and this call site has no seam to exercise it through in isolation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014L8UDXfhAJNKDLuEHN42AX
5910f88 to
0942143
Compare
|
This pull request failed to merge: some required checks failed. After you have resolved the problem, you should remove the Failed checks: Build public repo for MacOS arm64 / Build Reboot for MacOS, Test Manylinux arm64 / Manylinux runner, Check Code Style / GitHub-hosted Runner, Build public repo for Linux x86_64 / GitHub-hosted Runner, Build public repo for Linux arm64 / GitHub-hosted Runner, Test React Native mobile app (Maestro) / GitHub-hosted Runner, Test Manylinux x86_64 / Manylinux runner |
These commits fix a bug that caused infinitely-locked states.
A participant's
StateManager.Transactionpublishes its outcome — committed or aborted — through a singleasyncio.Future. Everyone who needs that outcome learns it by awaiting the transaction object, which awaited the future directly. If you cancel anasynciotask, that also cancels the future a task is suspended on. So a cancelled waiter did not merely stop waiting: it cancelled the transaction's outcome for everybody.After such an error, attempts to resolve the transaction would raise
InvalidStateErrorin exception-intolerant code, which causes Reboot to print##### WOW! YOU'VE FOUND A BUG IN REBOOT! #####. The state'sLockthen keeps a holder for the rest of the process's life, and every later attempt to claim the lock blocks forever.The fix is simply to
awaitthe outcome throughasyncio.shield, so a cancellation stays with the waiter it belongs to. The coordinator's participants future is already awaited that way, for the same reason.A second commit applies the same shield to
transaction.acquired_lock, the transaction's other shared future: concurrent calls in the same transaction park on it while the first of them acquires the per-state lock, so a cancelled waiter could cancel it the same way. That call site has no seam to exercise it through in isolation, so it relies on the tests for_committedand on the reasoning being identical.TESTED: new unit tests.