feat(webapp,run-store): gen-2 shard arms in read-through and idempotency - #4781
Merged
Conversation
|
Contributor
WalkthroughThe change adds shard-keyed replica and writer maps for run operations. Read-through requests now use explicit identifier kinds and resolve gen-2 IDs to dedicated shard clients. Batch hydration, API presenters, waitpoint resolution, and idempotency lookups use shard-aware routing. Results now expose structured Merge Risk: 🔵 Low · up to The PR adds shard-aware read and idempotency routing while remaining inactive when shard configuration is unset; the supplied tests and checks pass, and no concrete user-impacting defect is identified. It is mergeable with owner awareness that required instrumentation and test-convention follow-ups remain open. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly covers the implementation, behavior changes, merge safety, testing, and known out-of-scope findings. It does not reproduce the checklist or template headings, but it provides the required substantive information and is mostly complete.
✨ 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 |
![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.
d-cs
force-pushed
the
feat/gen2-readthrough-idempotency-tri-13431
branch
from
August 25, 2026 17:05
ad425cc to
8455b57
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
Route read-through by `resolveShard` instead of the binary residency classifier. A gen-2 id takes one read on its own shard's replica and probes no other store; a gen-1 v1 id still reads new only. Callers now declare `idKind`, because a cuid gives no way to tell a run id from a waitpoint id and the two must route differently. A legacy run id reads the legacy replica only, since there is no cuid run migration and the new-store probe cannot find it. A cuid waitpoint keeps the new-first pair probe, which is load-bearing because a cuid waitpoint can be co-located with its run on the new store. `ReadThroughResult` now carries `found` structurally. `source` is an open-ended union once shards exist, so a consumer testing found-ness by listing hit sources would read a gen-2 hit as a miss; discriminating on `found` turns that class of bug into a compile error. An id resolving to an unconfigured shard key logs an error and returns not-found rather than throwing. These ids arrive from callers (a URL param on the waitpoint route) and any base32hex core plus `[a-z0-9]` plus "2" parses as gen-2, so a throw would be a 500 any client could induce by guessing a shard char. Throwing stays correct on the router path, where ids are minted rather than received. The read-your-writes fallback for a gen-2 waitpoint now reads that shard's own writer. The gen-1 new writer is a different database, so reading it would miss and silently disable read-your-writes for the freshly minted token that fallback exists to serve. Inert while RUN_OPS_SHARDS is unset: the shard maps are empty, so every gen-2 arm is unreachable.
…e shard map
Idempotency resolved its client with a binary `ownerEngine(id) === "NEW"`
ternary, which cannot name a gen-2 shard. Both call sites now resolve
through one shard-keyed map, so they cannot disagree about which store
owns an id. An absent key takes an explicit logged branch to the
fallback rather than a silent legacy default.
`clientForShardKey` is the single place an id becomes a client.
`ShardKey` collapses to `string`, so the compiler cannot catch a wrong
key here; the `classify` seam is retyped to return a `ShardKey` so a
`Residency` value ("NEW") can no longer be fed into a shard-key
parameter, which the two differ from each other only by case.
`resolveIdempotencyDedupClient` keeps its policy. The mint-kind branch
has no id to decode and still resolves to the gen-1 pair.
Delete the `isMigrated` branch. Nothing implements it, and the one
production comment recorded that omitting it was deliberate.
The two cross-seam batch hydration sites classified with the binary
`ownerEngine` too. A gen-2 id joined the gen-1 `new` group, missed
there, and — classifying dedicated-family — never reached the legacy
probe either, so it was dropped from a bulk-action page and from batch
results with no error. Both now partition ids by shard key and read each
configured shard once.
`PostgresRunStore._residency` widens to `ShardKey` for call-site
consistency. It stays unused; the store stays unaware of its siblings.
Inert while RUN_OPS_SHARDS is unset: the shard map holds only the two
reserved gen-1 keys, so every partition falls through to today's paths.
…d read The baseline is keyed by line number, so partitioning the batch-results read by shard shifted the four pre-existing entries in ApiBatchResultsPresenter and added one. The new entry is the shard read beside the two gen-1 reads already in the baseline: the same explicitly-passed replica handle, the same read pattern. Net baselined violations in this file go from four to five, all reads, none new in kind.
Building it at module scope dereferenced the db.server run-ops handles at import time. This module is imported by triggerTask, so any test that mocks `~/db.server` without those two exports failed to collect — runEngineBatchTriggerStoreRouting and routesBatchGetReplicaLag both did. Before this change those handles were only read inside the two methods that use them. Restore that: resolve the map on first use and memoise it, since the trigger path is the hottest in the system. No test behaviour changes; this is the inert-by-default property the change was supposed to have in the first place.
… the gen-1 read Raised in review. When the shard map was non-empty but lacked the id's shard key, the member fell into the gen-1 group. The new replica is a different database so it missed, and a dedicated-family id never reaches the legacy probe, so the member vanished from the results with no error — the exact silent drop this change set out to remove. Report it and drop it, matching hydrateRunsAcrossSeam. The gen-2 tests now use the N-shard fixture that arrived with main, so the shard is a real separate database and a member seeded there is genuinely absent from the gen-1 store. That removes the handle-shadowing proxy the earlier version of the test needed to discriminate.
…uard Replacing the ownerEngine try/catch with resolveShard removed the only caller of the seam's `logger.warn`. That branch was already unreachable: classification has been total since the classifier stopped throwing. The dep now asks for `error` alone rather than carrying a hook nothing calls. The guard baseline is keyed by line number, so the three-way partition in ApiBatchResultsPresenter shifted four of its five entries. Still five, all reads, in the one file, none new in kind.
d-cs
force-pushed
the
feat/gen2-readthrough-idempotency-tri-13431
branch
from
August 26, 2026 08:59
1821f29 to
70fd5d5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two read paths answered 500 for input a caller controls. Found while validating the gen-2 shard arms against a local instance. The waitpoint wait route threw `json(404)` from inside its own `try`, so the catch-all below caught that Response and re-threw it as a 500. Every not-found waitpoint answered 500, including a plain absent cuid. Re-throw a Response untouched: it carries a deliberate status, not a failure. `RoutingRunStore.#shardStore` carried a comment claiming its throw was unreachable with the two-store compat constructor. It is reachable. The constructor defaults to the real `resolveShard`, which is pure id-shape, so any gen-2 shaped id names a shard char even when no shard is configured — and these ids arrive as URL parameters. Correct the comment and pin the reachability with a test that uses the real resolver rather than an injected one, plus a test that gen-1 shapes still route on the compat pair. The throw itself is left alone: failing loud rather than reading the wrong database is the intended design. What needs changing is the status the boundary reports, and that needs a typed error to match on instead of the current plain Error — filed separately rather than done here.
…nt id Two read paths answered 500 for input a caller controls. Found while validating the gen-2 shard arms against a local instance. `resolveShard` is pure id-shape, so any base32hex core plus [a-z0-9] plus "2" parses as gen-2 and names a shard. The routing store throws for a shard key it has no store for, and that throw reached the API boundary as a 500 — so any caller holding an API key could induce one by guessing a shard char, of which there are 36. A 5xx on a read also trips canary rollbacks on deploy. The throw stays: failing loud beats reading the wrong database. What changes is the status the boundary reports. `#shardStore` now throws a typed `UnknownShardKey` carrying the key and the configured set, and the API boundary maps it to 404 — the same answer an absent gen-1 or cuid id already gives. The boundary still logs at error level with the key, so a shard dropped from a config meant to be append-only still alarms. Separately, the waitpoint wait route threw `json(404)` from inside its own try, so the catch-all caught that Response and re-threw it as a 500. Every not-found waitpoint answered 500, including a plain absent cuid. Re-throw a Response untouched: it carries a deliberate status, not a failure. `#shardStore` also claimed its throw was unreachable with the two-store compat constructor. It is reachable, because that constructor defaults to the real resolver. Comment corrected and the reachability pinned by a test that uses the real resolver rather than an injected one. Measured on a local instance: all four cases that returned 500 now return 404, absent gen-1 and cuid ids still 404, and a real run still 200.
…utable id not The two 404s in the previous commits are not the same kind of miss, and the SDK only retries what it is told to. A missing waitpoint can be replica lag. resolveWaitpointThroughReadThrough deliberately never reads the legacy primary, so it relies on the caller retrying a miss. The SDK does not retry a plain 404, so returning one would turn a transient miss into a permanent failure — worse than the 500 it replaced. Send `x-should-retry: true`. An id naming a shard with no configured store is not transient: no number of retries makes a topology grow a store. Send `x-should-retry: false` so that stays true if SDK defaults ever change. Measured: the wait route answers 404 retry=true for both a forged gen-2 id and an absent cuid; an unroutable id answers 404 retry=false; an absent run still answers 404 retry=true; a real run still answers 200.
Contributor
Observability mapAs of 20/100 over 449 measured of 467 entry points (base 19, up 1) What this PR changed
FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
d-cs
marked this pull request as ready for review
August 26, 2026 12:01
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
… a cached map The map merged two module constants with runOpsShardWriters, which is already keyed, so it was a second structure over static references. The memo around it traded a trivial allocation for mutable module state that can leak between tests. A plain resolver is lazy by construction and holds nothing. Laziness is the part that matters: reading the db.server handles at module scope breaks import for any test that mocks the module without them, and triggerTask imports this file. The dependency changes from a map to `clientFor`, which injects the same way. The error log drops the configured key list, since boot already prints the shard table.
…n fast path Raised in review: `#shardStore` was the only unconfigured-shard guard throwing the typed error. `#partitionAbsentIds` and `#fanOutPartitioned` still threw a bare Error, and `findRunsByIds` reaches the latter through `findRuns` — so those paths would still answer 500 where the boundary now gives a non-retryable 404. Both now throw `UnknownShardKey`, which gains an optional subject so the messages stay as specific as before. Tests cover both guards by type, not just by message. Also cover the gen-1 run fast path in `readRunForEvent`, which until now was the only live behaviour change on this branch with no test and no manual verification. The observable property is the read COUNT: one read on the fast path, two on the old new-then-legacy pair probe. The new-side read has to genuinely miss for the two to be distinguishable — a pair probe whose first read hits short-circuits and looks identical — so the test points the new side at a replica that misses the row. Verified by reverting the arm: the count goes to two.
Collaborator
Author
Manual verification performedEverything below was measured against a locally-booted webapp, not inferred. Automated coverage is in the diff; this is what was exercised by hand, because the gaps it closes are ones tests structurally could not reach. SetupTwo configurations, both against real Postgres:
I deliberately did not use With
|
| Check | Result |
|---|---|
| boot | healthcheck 200, no fatal, no sentinel failure |
| shard table absent from the boot log | confirmed — it is only emitted when a shard is configured |
| split genuinely live | a new run present on :5434, absent from :5432 |
| idempotency reuse | two triggers on one key → one run id, second isCached: true |
| cuid waitpoint token | created and retrieved — the frozen new-first pair-probe arm |
| bulk action CANCEL | 3/3, and all three rows confirmed CANCELED in the database |
| batch results | all 3 members returned |
| legacy run read | a cuid run resolves, and is legacy-resident only |
With one real shard configured — the path no test covers
Every gen-2 test injects the shard map. The production path from RUN_OPS_SHARDS through runOpsShardHandles to the read sites had no coverage at all.
| Check | Result |
|---|---|
| boot | {"key":"a","fingerprint":"localhost:5436/postgres","role":"shard"} logged; run-ops-shard-a-writer pool opened |
| sentinels | no distinct-database failure, no replication failure, no zod rejection |
| a gen-2 run reads from its own shard | a run present on :5436 only — absent from :5432 and :5434 — was returned by the API |
| a gen-2 id on an unconfigured shard key | degrades, does not throw; see status table |
| an unreachable shard at boot | healthcheck 200, one logged connect failure, no crash |
Status and retry semantics
Measured on a booted instance, before and after the fixes in this PR:
| Case | Before | After | x-should-retry |
|---|---|---|---|
GET /api/v3/runs/<gen-2> |
500 | 404 | false |
GET /api/v1/waitpoints/tokens/<gen-2> |
500 | 404 | false |
POST …/waitpoints/tokens/<gen-2>/wait |
500 | 404 | true |
POST …/waitpoints/tokens/<cuid>/wait |
500 | 404 | true |
| absent gen-1 v1 run | 404 | 404 | true |
| absent cuid run | 404 | 404 | true |
| a real run | 200 | 200 | — |
The two 404s are deliberately different. An absent waitpoint may be replica lag, and resolveWaitpointThroughReadThrough never reads the legacy primary — its own comment says it relies on the caller retrying — so that one must stay retryable. An id naming an unconfigured shard cannot become routable by retrying, so that one is explicitly not.
The boundary still logs Unroutable id at error level with the shard key, so a shard dropped from a config meant to be append-only still alarms while the caller gets a 404.
No collateral damage from the boundary change
apiBuilder sits on every API route's error path, so its blast radius was checked directly: malformed body still 400, missing auth still 401, idempotency dedup, run reads and waitpoint reads all still behave. The match is narrow by construction — it only claims UnknownShardKey — and a test asserts it declines unrelated errors, so a genuine failure still reaches the 500 path.
What manual testing did NOT cover
- Real gen-2 minting. It does not exist yet (TRI-13430), so every gen-2 id used here was hand-forged into a database. That is the best available today.
- An SDK honouring the retryable 404 end to end. The header was verified; the SDK's
shouldRetrybehaviour was read in code, not exercised. - A gen-2 batch. Verified only as a negative: a gen-2 batch row 404s, because the batch row lookup has no shard arm. See the note below.
Two findings outside this PR's scope
Both were found by this exercise and are recorded for whoever picks them up.
ApiBatchResultsPresenterresolves the batch row via new-then-legacy only, with no shard arm. A gen-2 batch row placed on a shard returned 404. The member-partition fix in this PR is correct but unreachable for a real gen-2 batch until that lookup is fixed. This matters to gen-2 minting.BatchTaskRunItem.taskRunIdcarries a foreign key toTaskRunwithin each store. Postgres refused an item pointing at a run on another database, so a batch's items and its member runs must be co-resident: a batch cannot span shards. That is a constraint the sharding design should state explicitly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts (1)
36-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required crumb instrumentation.
Add
//@Crumbsmarkers or approved `#region `@crumbsblocks for the changed shard-routing and test paths.
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts#L36-L43: Add crumbs for shard-key client resolution.apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.ts#L27-L27: Add crumbs for the changed resolver fixture.apps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts#L28-L36: Add crumbs for configured-client fallback.internal-packages/run-store/src/runOpsStore.ts#L263-L263: Add crumbs for unconfigured-shard rejection.internal-packages/run-store/src/runOpsStore.shardMap.test.ts#L363-L369: Add crumbs for the partitioned-read error case.apps/webapp/test/readRunForEvent.replicaLag.test.ts#L204-L269: Add crumbs for the legacy fast-path test.As per coding guidelines, “Add crumbs as you write code — not just when debugging.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c26d6f3-65f3-4aed-bf6a-c584c0ae319c
📒 Files selected for processing (6)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsinternal-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; 5 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (37)
- GitHub Check: report
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: internal / 🧪 Unit Tests: Internal
- 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: 🛡️ E2E Auth Tests (full)
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: audit
🧰 Additional context used
📓 Path-based instructions (13)
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.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.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts
🧠 Learnings (1)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
internal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.ts
ericallam
approved these changes
Aug 26, 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.
Gives read-through and idempotency their gen-2 shard arms, so an id that names its own shard is read there and nowhere else.
#4764 has landed, so this now targets
maindirectly and no longer depends on an unmerged branch. It builds on what that PR supplied:resolveShard,runOpsShardHandlesand the keyed router.TRI-13431
What changes
Read-through routes by
resolveShard, not by the binary residency classifier. A gen-2 id reads its own shard's replica once and probes no other store. A gen-1 v1 id still reads new only.Callers now declare
idKind. A cuid gives no way to tell a run id from a waitpoint id, and the two must route differently:There is no default, because a default would pick one of those arms silently. The field
runIdis renamed toid, since it carried both kinds already.ReadThroughResultcarriesfound.sourceis an open-ended union once shards exist, so a consumer testing found-ness by listing the hit sources reads a gen-2 hit as a miss. One consumer did exactly that. Discriminating onfoundmakes that class of bug a compile error rather than something a reviewer has to spot.Idempotency resolves its client through one shard-keyed map. Both call sites go through
clientForShardKey, so they cannot disagree about which store owns an id. An absent key takes an explicit logged branch to the fallback, not a silent legacy default. Theclassifyseam is retyped to return aShardKey:Residency("NEW") and the reserved shard keys ("new") differ only by case, andShardKeycollapses tostring, so the compiler would not have caught feeding one into the other.The dead
isMigratedbranch is deleted. Nothing implemented it, and the one production comment recorded that omitting it was deliberate.PostgresRunStore._residencywidens toShardKey. Still unused; the store stays unaware of its siblings.Two behaviour fixes found while doing the above
An unconfigured shard key logs and returns not-found instead of throwing. The waitpoint route takes the id from a URL parameter, and any base32hex core plus
[a-z0-9]plus"2"parses as gen-2. The route turns a throw into a 500, so throwing here would let any authenticated client generate 500s and error logs by guessing shard chars, of which there are 36. An error-logged not-found is neither silent nor a misroute. Throwing stays correct on the router path, where ids are minted rather than received.The two cross-seam batch hydration sites were gen-2 blind.
hydrateRunsAcrossSeamandApiBatchResultsPresenterclassified with the binaryownerEngine, so a gen-2 run id joined the gen-1newgroup, missed there, and — classifying dedicated-family — never reached the legacy probe either. The id was dropped from a bulk-action page and from batch results with no error. Both now partition ids by shard key and read each configured shard once.Also: a gen-2 waitpoint that missed its shard replica fell back to the gen-1 new writer, a different database, silently disabling read-your-writes for the freshly minted token that fallback exists to serve. It now falls back to its own shard's writer.
Merge safety
Inert while
RUN_OPS_SHARDSis unset: the shard maps are empty, so every gen-2 arm is unreachable, and gen-2 minting is not live yet.The one live change is the gen-1 run arm, and it removes work rather than adding it.
RoutingRunStore.findRunnever forwards the caller's client object — it routes by id and reads only the client's presence and replica brand — soreadRunForEvent's "new" closure already resolved a legacy-classified run id to the legacy store. The arm removes a duplicated read of the legacy replica. A test pins this, because a future caller passing a raw client and a run id would lose the pre-cutover 27-char case, which is new-resident but classifies legacy.Testing
14 tests added, testcontainers throughout, no mocks. 22 affected test files pass; typecheck, lint, format and knip are clean.
Both arms were verified by neutralising them and confirming the new tests fail. The batch-results test needed rewriting after that check: the first version passed with the fix neutralised, because it used one container as both the gen-1 new client and the shard replica, so it was not testing what it claimed.
Note for review: run testcontainer suites in small batches. Sixteen at once starves Docker and everything times out at 60 seconds.
The run-ops legacy-guard baseline is refreshed in its own commit. The baseline is keyed by line number, so partitioning the batch-results read shifted four pre-existing entries and added one. Baselined violations in that file go from four to five, all reads; the new one is the shard read beside two gen-1 reads already there.
No changeset and no
.server-changesentry: a user notices nothing while the flag is unset.