refactor(run-store): hold RoutingRunStore's stores in a keyed shard map - #4752
Merged
Conversation
RoutingRunStore held two named store fields and took its routing policy from the order the statements happened to run in. It now holds a Map<ShardKey, RunStore>, and the three policies are readable data: - #probeOrder — sequential probe for a lookup with no routable id; the first non-null result wins and the last entry owns the not-found throw - #precedence — ascending authority for a merge, so the highest-authority shard wins a duplicate id - #idlessRouteShard / #idlessWaitpointShard — the two id-less defaults, which differ by role and were previously two unrelated literals The two orders are the reverse of one another, which is why they are separate fields: some sites observe the result-array order, others only the winner of a duplicate. Four helpers absorb the twenty-six hand-written fan-outs (#probeFirst, #fanOut, #fanOutPartitioned, #shardsExcept), and #shardKeyOf replaces the inline residency-to-store ternaries. The constructor keeps its exact options type, so every existing call site compiles unchanged and the @ts-expect-error onLegacyRead lock keeps firing. One behaviour changes. findManyTaskRunWaitpoints merged its edge rows NEW-first into a last-wins dedupe, so a duplicate edge id resolved to the LEGACY row — the opposite of the rule every sibling merge follows, and of what dedupeEdgesById's own comment claimed. It now resolves NEW-wins, with a test pinning it. The case needs one edge id present on both stores at once, which is the drain-mirror window; the drain seam is removed, so it is unreachable in practice. Also refreshes the sixteen stale runOpsStore.ts line references in runOpsStore.mixedResidency.test.ts.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (32)
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ocstring The docstring said "In single-DB both stores are the same, so routing is a no-op passthrough". That has not been true since the injecting seam started returning a bare PostgresRunStore when the split is off — this class is not constructed at all in single-DB. The stale sentence made the counting fan-outs look like they could double count one store. State the actual invariant instead: every shard is a distinct database, enforced by the seam and by the fail-closed boot sentinel, and two keys resolving to one store would double count. Comment-only. Also re-anchors the line references in runOpsStore.mixedResidency.test.ts, which the docstring shifted.
…comments The comments cited runOpsStore.ts line numbers. They rotted three times inside this PR alone — a docstring edit or a formatter pass moves every one of them, and a stale pointer is worse than none. Keeps the symbol names, which are what actually locate the code and survive edits. Matches the convention unblockRouteCatalog states for itself: anchored by method name, never a line number. Comment-only.
d-cs
marked this pull request as ready for review
August 21, 2026 15:55
0ski
approved these changes
Aug 21, 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
2 participants
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.
What
RoutingRunStoreheld two named store fields,#newand#legacy, and took its routing policy from the order the statements happened to run in. It now holds aMap<ShardKey, RunStore>, and the three policies that were implicit are readable data:#probeOrder(new→legacy) — the sequential probe for a lookup with no routable id. The first non-null result wins, and the last entry owns the canonical not-found throw.#precedence(legacy→new) — ascending authority for a merge, so the highest-authority shard wins a duplicate id.#idlessRouteShard(new) and#idlessWaitpointShard(legacy) — the two id-less defaults, which differ by role and were previously two unrelated literals in unrelated methods.The two orders are the reverse of each other, which is why they are separate fields rather than one ordering. Nine sites observe the result-array order and must iterate
#probeOrder; five decide a value by which shard wins a duplicate and must iterate#precedence. Five more sum counts and are order-independent, because addition commutes.Four helpers absorb the twenty-six hand-written fan-outs —
#probeFirst,#fanOut(order, fn),#fanOutPartitioned,#shardsExcept— and#shardKeyOfreplaces the inline residency-to-store ternaries.#fanOuttakes its order as an argument so every call site states which policy it uses.The constructor keeps its exact options type. No union arm, no
shardsmember: that would loosen the excess-property check and silently retire the@ts-expect-error onLegacyReadlock in the test corpus. N-way construction is a later change.One behaviour change
findManyTaskRunWaitpointsmerged its edge rows NEW-first into a last-wins dedupe, so a duplicate edge id resolved to the legacy row — the opposite of the rule the other four merges follow, and the opposite of whatdedupeEdgesById's own comment claimed. No test pinned it in either direction.It now resolves NEW-wins, consistent with every sibling merge, and a new test pins the winner so it cannot drift back silently.
Reaching this case needs one edge id present on both stores at the same time, with no routable
taskRunId. That only arises from drain mirroring. The drain seam is removed (runOpsStore.test.ts, "fan-out spans NEW+LEGACY with no drain seam"), so no new duplicates can be created — but removing the code does not delete rows it previously wrote, and this class still carries comments treating mirrored rows as a live data condition. Whether any historical duplicate edge rows persist is an empirical question about production data, not something this diff settles.If such a row is hit, the two copies either agree — in which case the winner is immaterial — or they have diverged, in which case NEW is the authoritative copy by the router's own precedence rule. So the corrected behaviour is at least as correct as the old one in every reachable case.
Everything else is behaviour-preserving.
How it was verified
internal-packages/run-store: 69 files, 379 tests pass. The corpus is the regression gate for this refactor. 67 of the 68 pre-existing test files are byte-identical; the one that differs (runOpsStore.mixedResidency.test.ts) changes only//comments.internal-packages/run-engine: 12 files, 69 tests pass — every file that constructs the router, exercised at runtime.@ts-expect-error onLegacyReadlock still fires.tsconfig.build.jsonexcludes*.test.ts, so a green typecheck does not cover it. A scratch probe confirmedtscstill reportsTS2353foronLegacyReadand no error for the three real options.tsconfig.check.jsonalso excludes*.test.ts, so the 25 webapp test files were checked with the test exclusion dropped and compared against the same check on the base commit: 614 errors before, 614 after, zero present in one and not the other. Those 614 are pre-existing in never-typechecked test files.typecheckpasses forrun-store,run-engineandwebapp.knipreports nothing inrun-store.Also
Refreshes the sixteen stale
runOpsStore.tsline references inrunOpsStore.mixedResidency.test.ts, each verified against the symbol it names.Notes for the reviewer
readonly ShardKey[]. The five#precedencesites are#findRunsOpen,findRunsByIdempotencyKeys,#collectManyWaitpoints,findManyTaskRunWaitpointsandfindManyWaitpointTags. Those are the lines worth the closest read.home === this.#new ? ...). They now compare keys. The two are equivalent: in single-database mode both keys map to the same store object, and when the stores are distinct, identity and key comparison agree..server-changesnote: the package is internal and the one behaviour change is unreachable in production, so a release note would tell a user nothing.lint(~16 unknownreact/*rules make.oxlintrc.jsonfail to parse, which disables oxlint entirely — including the twotrigger-runopsfences) andknip(unrun, an unused devDependency on the default branch). Both want their own fix.