feat(run-store): make the run-ops router correct at N shards - #4771
Conversation
…ounts a pending wait
…pinned legacy store
|
|
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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (7)We use vitest exclusively. **Never mock anything** - use testcontainers instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging. Mark lines with📄 CodeRabbit inference engine (AGENTS.md) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🧠 Learnings (1)📚 Learning: 2026-08-21T14:26:14.909ZApplied to files:
🔇 Additional comments (4)
WalkthroughThe run store now supports configurable gen-2 shard routing with shard aliases and distinct-database fan-out. It deduplicates cross-shard results, applies deterministic precedence, partitions waitpoint queries, and records duplicate and fallback metrics. Gen-2 waitpoints route directly to their named shard, while invalid co-location and missing routing identifiers now raise errors. Idempotency matches include run IDs and creation times. New PostgreSQL fixtures and tests validate four-store routing, aggregation, completion, aliases, and pagination. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the objectives, key implementation changes, safety considerations, testing, and issue reference. It omits the template checklist, explicit Closes line, and screenshots section, but it provides sufficient detail for review. ✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/testcontainers/src/nShardFixture.test.ts (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumb markers to the new test blocks.
The added test code has no
//@Crumbsmarker or `// `#region` `@crumbsblock. Add markers while developing these tests and strip them before merge.Proposed change
+// `@crumbs` // Booting the PG14 + PG17 containers and cloning four databases on a cold runner far exceeds // vitest's 5s default (this package sets no global testTimeout), so pass a generous per-test one. nShardTest( "builds 4 distinct databases (legacy + new + 2 gen-2 shards)", async ({ legacyUri, newUri, shardUris }) => { expect(shardUris).toHaveLength(2); const all = [legacyUri, newUri, ...shardUris]; expect(new Set(all).size).toBe(4); }, 120_000 ); +// `@crumbs` nShardTest( "each gen-2 clone carries the run-ops subset schema", async ({ shardPrismas }) => {As per coding guidelines:
**/*: Add crumbs as you write code with//@Crumbsor `// `#region` `@crumbs, then strip them withagentcrumbs stripbefore merge.Also applies to: 18-18
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52a9ae21-41a5-4c86-a63b-b3e7040c24a5
📒 Files selected for processing (1)
internal-packages/testcontainers/src/nShardFixture.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (33)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/testcontainers/src/nShardFixture.test.ts
🔇 Additional comments (1)
internal-packages/testcontainers/src/nShardFixture.test.ts (1)
8-16: LGTM!Also applies to: 19-27
… onto a gen-2 shard
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal-packages/run-store/src/runOpsStore.ts (3)
100-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject alias cycles.
A self-alias or cycle passes this validation because each target exists in
configured.#distinctStoresthen removes every cycle member. Unrouted reads and fan-out writes omit that database.Reject self-aliases and cycles, or resolve every alias to a non-aliased root before building
#distinctStores.
759-764: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClassify idempotency candidates by
row.id.If shard
"a"aliases"new", fan-out returns an"a"run through the"new"leg. This condition then treats that gen-2 candidate as gen-1 and selects the last database result instead of the earliestcreatedAt. The idempotency lookup can select the wrong existing run.Determine gen-2 participation from
this.#shardKeyOfSafe(row.id). Keep the leg key only for gen-1 mirror precedence.
98-136: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd an approved crumb to the new shard-routing flow.
Add a
//@Crumbsmarker or `// `#region` `@crumbsblock for the new shard configuration and alias resolution path.AGENTS.mddoes not listinternal-packages/run-store, so obtain an approved namespace before adding the marker. Do not invent a namespace.Source: Coding guidelines
internal-packages/run-store/src/runOpsStore.shardMap.test.ts (1)
86-116: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftReplace the fake store with a testcontainers-backed fixture.
This fake reports
presentIdsonly for pending waitpoints. It cannot represent a completed but present waitpoint. The routing tests can therefore miss an incorrect fallback probe.Use the N-shard testcontainers fixture for these cases instead of extending
FakeStore.As per coding guidelines, “We use vitest exclusively. Never mock anything - use testcontainers instead.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cea49659-9f09-47d9-9b5d-2caeed4fe352
📒 Files selected for processing (2)
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (27)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
|
Thanks — worked through the three merge-risk points. One was real and is fixed; the other two I believe are false positives, detail below. 1. Alias cycles/chains omitting a database — fixed (623c61c). Correct. The constructor validated that 2. Idempotency selecting the wrong run under an alias — I don't think this can occur. 3. Creates failing when routing identifiers are omitted — intended, and verified safe. These are the fail-loud throws the ticket licenses: All changes are inert with two configured stores ( |
…mirror counts once
Manual testingBooted the webapp on this branch against all three run-ops database topologies and exercised the run hot path — create → block on a waitpoint → complete → resume — in each, via the SDK test-suite canary (the
In every mode: the app started cleanly (no FATAL, no sentinel-assertion failures), the boot interlocks resolved correctly (the coresidency sentinel flips as expected between 2-DB and 3-DB), a routed run-detail read rendered, and the canary was driven to completion end to end. Key result: the canary produced identical results across all three topologies. The router path (2-DB and 3-DB) behaves the same as the no-router path (single-DB), and every waitpoint primitive passed in every mode — confirming the rewritten completion/count hot path ( Gen-2 N-way sharding stays dormant here (no id minting, no shard config wired), so it was not exercised by this manual pass — that path is covered by the four-store container matrix in this PR and will be validated live via the alias soak in a follow-up. |
…s-n-tri-13427 # Conflicts: # internal-packages/run-store/src/index.ts
What
Makes
RoutingRunStorecorrect when the run-ops layer routes across more than two Postgres stores. Today it routes between a gen-1newdedicated database and alegacycontrol-plane database; this generalizes every routing policy to N shards while keeping the two-store behaviour byte-identical.The change sets the four routing decisions that were implicit in code order, and fixes one hazard that failed silently:
resolveShardinstead of the binary residency classifier, so a gen-2 id reaches its own shard through the keyed map.#distinctStores(one entry per physical database, aliases excluded by a declaredaliasOf) drives every sum, probe, and merge;#shardsdrives routing. An aliased shard can no longer make a sum count one database twice.{new, legacy}stays silent (the known drain-mirror case); any other cross-shard duplicate incrementsrunops_shard_duplicate_id_totaland logs at error level.countPendingWaitpointsand the waitpoint collector now partition absent ids by shard and union by id rather than summing counts. A drain-mirrored waitpoint on both gen-1 stores is counted once, so a blocked run can no longer hang forever on a double-counted pending waitpoint.new. An id resolving to an unconfigured shard throws instead of being dropped.Two new counters are exported:
runops_shard_duplicate_id_totalandrunops_waitpoint_probe_fallback_total.Why it is safe to merge
With only
{new, legacy}configured every generalized rule reduces to today's behaviour.resolveShardreturns exactly what the old classifier returned for every id shape that exists today, and no gen-2 id is minted yet. The only intentional behaviour change is the fail-loud create throw; an enumeration of production call sites confirmed no caller trips it.Testing
runOpsStore.nShardMatrix.test.tsruns a four-store matrix (legacy + new + two gen-2 shards) against real Postgres containers: the disjoint-sum union, the alias topology, cross-tree completion, pagination merges, and mixed-id hydration.makeNShardRunOpsPostgresTest(k)fixture in@internal/testcontainers.Notes
.server-changesfile: internal routing infrastructure, no user-visible behaviour change.