Deflake concurrent_transactions_same_state by allowing retries - #152
Merged
Conversation
Current Aviator status
This PR was merged using Aviator (commit 0c670ad).
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
changed the title
tests: rendezvous on driver names, not on arrival countsconcurrent_transactions_same_state by allowing retries
rjhuijsman
marked this pull request as ready for review
September 3, 2026 08:43
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The mirrored implementations correctly preserve concurrency validation while tolerating legal transaction retries.
Pull request overview
Deflakes mirrored transaction-concurrency tests by tracking unique callers across retries.
Changes:
- Replace arrival counters with caller-ID sets.
- Pass driver IDs into nested transactions.
- Update assertions and retry documentation.
File summaries
| File | Description |
|---|---|
tests/reboot/zod/concurrent_transactions_same_state/test.ts |
Checks unique Zod callers. |
tests/reboot/zod/concurrent_transactions_same_state/servicer.ts |
Tracks and forwards Zod driver IDs. |
tests/reboot/zod/concurrent_transactions_same_state/servicer_api.ts |
Adds the Zod driver-ID request field. |
tests/reboot/pydantic/concurrent_transactions_same_state/test.py |
Checks unique Pydantic callers. |
tests/reboot/pydantic/concurrent_transactions_same_state/servicer.py |
Tracks and forwards Pydantic driver IDs. |
tests/reboot/pydantic/concurrent_transactions_same_state/servicer_api.py |
Adds the Pydantic driver request model. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
`test_nested_transactions_on_one_state_are_parallel` failed intermittently on the MacOS arm64 CI runner, and always by overshooting: `40 != 20` (every driver counted twice), `59 !== 20`, and once a 300s timeout. It never reproduced on Linux. The `Rendezvous` that the test met at counted invocations of `inner()`. Reboot guarantees that a transaction happens once, not that the code inside it is invoked once: when a root transaction aborts, the `ExternalContext` stub retries it, re-running `call_inner` and with it the nested `inner()` on the shared `COUNTER_ID`. On a slow runner such an abort is easy to come by, whether from a lock acquire that exceeds `LOCK_ACQUIRE_DEADLINE_DEFAULT` or from a participant raising `TransactionShouldRetryWithoutBackoff`, so the count climbed past `CONCURRENCY`. Counting invocations was also a weaker check than it looked: a driver counted twice can stand in for one that has not arrived at all, opening the meeting point while only 19 transactions really overlap. `Rendezvous` now collects the names of the drivers that have arrived and opens once it holds `expected` distinct ones, so a retried transaction arrives idempotently and the assertion stays exact. `inner` takes the driver's name in its request, because it runs on the peer's state, which is the same state for every driver, and so cannot work out who called it from its own context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTP67vX9mZusQwMrNCZw1r
rjhuijsman
force-pushed
the
rjh.concurrent-transactions-rendezvous
branch
from
September 3, 2026 09:03
a2fb129 to
868aba3
Compare
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
rjhuijsman
marked this pull request as draft
September 3, 2026 09:19
rjhuijsman
marked this pull request as ready for review
September 3, 2026 11:52
onelxj
approved these changes
Sep 3, 2026
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.
tests/reboot/pydantic/concurrent_transactions_same_stateand itszodtwin were failing intermittently on the MacOS arm64 CI job. This PR fixes that flake.The failure on MacOS only is due to the Mac runners being highly loaded, therefore very slow, and therefore they accidentally exercise Reboot's retry mechanisms. In that environment our tests must allow some (legal, correct) retries; if they don't, we get flakes.
In this test, the subtest
test_nested_transactions_on_one_state_are_parallelcounts the number of time aninnerfunction is run - there are 20 parallel callers, so it expectsinnerto be run 20 times. That's incorrect in the face of retries, in two ways:innerwill be run at least 20 times, but can be run more times.innerwere run exactly 20 times, that doesn't prove the desired property that 20 parallel callers ran - it may have been any number of callers up to 20, since any of them may retry any number of times.This PR fixes that by changing how we count callers: by counting unique names of callers (rather than just call count) we build a picture that's robust in the face of retries.
TESTED: the test still passes locally on Linux. That does not verify the flake is fixed, since the flake only occurs on MacOS arm64 runners (due to their heavy load) and don't repro locally or on Linux CI. The claim that the flake is fixed is based on log+code inspection