Fix two transaction hangs, and other CI flakes - #134
Open
rjhuijsman wants to merge 4 commits into
Open
Conversation
Current Aviator status
This PR is not ready to merge (currently in state pending): this PR has not been approved. Pending Status Checks
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.
|
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
3 times, most recently
from
August 18, 2026 15:55
c504154 to
a4ba4d0
Compare
rjhuijsman
changed the title
state_managers: don't hand a lock hold to a waiter that gave up
rjhuijsman
changed the title
rjhuijsman
marked this pull request as ready for review
August 18, 2026 16:01
rjhuijsman
requested review from
benh
and
a balanced review from Copilot
and removed request for
benh
August 18, 2026 16:01
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes transaction hangs caused by cancelled lock waiters and interrupted coordinator aborts.
Changes:
- Safely discards abandoned lock waiters and restores interrupted upgrades.
- Publishes coordinator aborts before cancellable cleanup.
- Adds lock regression tests and increases a flaky test’s timeout class.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
reboot/aio/state_managers.py |
Updates lock and transaction-abort handling. |
tests/reboot/state_manager_tests.py |
Adds cancellation regression tests. |
tests/reboot/zod/concurrent_transactions_same_state/BUILD.bazel |
Increases the test size to large. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
from
August 19, 2026 12:07
02800df to
0cb9773
Compare
In the state manager, callers take `Lock`s per state - but before this commit it was possible for a caller to be granted a `Lock` after the caller was cancelled. In that case the `Lock` was never released again, and the state it guarded became unusable for the life of the process. That caused CI flakes. Specifically, before this change, `Lock._maybe_grant_next()` would grant the lock to the waiter at the head of its queue without checking whether that waiter was cancelled or not. Cancellation of callers is routine, so there were many ways CI runs could flake - the most common flake was the new `//tests/reboot/pydantic/concurrent_transactions_same_state:test_py` which flaked ~80% on MacOS (oof). This commit makes it so that all forms of lock-taking will avoid granting locks to waiters that have been cancelled. Additionally, the `zod` version of `concurrent_transactions_same_state` would regularly time out even without hitting the flake, so this commit also bumps its timeout. TESTED: five new unit tests cover all cases that used to be broken.
A participant only ever learns that its transaction aborted from the coordinator, by watching it. If that news never arrives the participant blocks forever, holds its state's lock forever, and any retry reusing the same idempotency key blocks behind it. Before this change, `_transaction_coordinator_abort()` resolved the `_coordinator_participants` future last, after awaiting a database cleanup and a fan-out of best-effort `Abort` RPCs whose own handler re-raises `CancelledError`. A coordinator cancelled during either await -- which is what happens when its caller goes away -- left the future pending with nothing left to resolve it, blocking every participant forever. This commit reorders the abort to first resolve the future and drop its entry, ahead of anything that can be interrupted. Answering before the database cleanup lands is safe because an abort is never something the coordinator records: the durable outcomes are "preparing" and "absent", and recovery re-drives a "preparing" transaction into the same abort.
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
2 times, most recently
from
August 19, 2026 12:21
26a92b8 to
80f5843
Compare
rjhuijsman
commented
Aug 19, 2026
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
from
August 19, 2026 12:36
013a472 to
fac8d23
Compare
Our MacOS runners are the slowest and most expensive ones we have, and over-saturated runners cause frequent timeouts in our largest tests. This commit removes several redundant large tests from its coverage, reducing run time by ~51 minutes and hopefully reducing over-saturated-runner timeout flakes. Before this change, the MacOS job ran every test under `//tests/...` that wasn't explicitly excluded — 219 targets, 28 of them `large` or carrying a `long` timeout. Many of those tests don't tell us anything that the Linux runners don't already tell us: they type-check TypeScript, exercise pure application logic on top of the Reboot runtime. Other `large` tests drive a toolchain (`uv sync`, `rbt generate`, `npm install`, `rbt dev run`) that another test running on MacOS drives too. These useless tests add up to ~51 minutes of MacOS runner usage per run. This commit adds a `platform-independent` tag, which indicates tests that don't add meaningful coverage for the MacOS platform. the MacOS job's `bazel query` now excludes these.
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
from
August 19, 2026 12:37
fac8d23 to
a77547e
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Before this commit, the `agent-wiki`'s test asked for a `large` test; we don't think it actually needs that. It claims it's for `uv sync`, but we've not seen any recent evidence of a fully cold `uv sync` taking more than ~1.2s. The target runs in 35-71s across the last 22 MacOS jobs, comfortably inside a `medium` budget. The timeouts it _did_ have look more like the hangs the previous commits fixed. This commit therefore removes the `large` test request for `agent-wiki`.
rjhuijsman
force-pushed
the
rjh.deflake-concurrent-transactions-same-state
branch
from
August 19, 2026 13:08
cca1867 to
061176e
Compare
Contributor
Author
|
@benh can you bump this to the top of your queue? These flakes make CI unpassable for all other PRs, and you're clearly the appropriate reviewer for transaction logic changes. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
rjhuijsman
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI is currently ~unpassable because of CI flakes. Most notably
//tests/reboot/pydantic/concurrent_transactions_same_state:test_pyis ~80% flaky. An agent identified two hangs in the transaction code from those flakes. These each have their own commit to fix them, please see the individual commit descriptions for details.Additionally, we address timeouts and flakes in MacOS CI that are due to overload by reducing its workload, removing tests that don't add meaningful coverage.
Note that while the first commit's case is proven with unit tests, the second is hard to repro so needs some human reasoning to validate.