feat(core,webapp,run-engine): stamp a shard key onto run, batch and waitpoint ids - #4788
feat(core,webapp,run-engine): stamp a shard key onto run, batch and waitpoint ids#4788d-cs wants to merge 14 commits into
Conversation
Adds mintWaitpointIdForShard(key) and mintWaitpointIdFor(anchorId). A gen-2 shard key produces a 26-char body carrying that shard char at index 24 and version "2"; a reserved key, or no anchor, keeps today's cuid. The core is always freshly minted rather than derived from the anchor: a derived body would share the anchor's core, shard char and version char, so it would be byte-identical to the run's own id. Both the webapp and the run engine mint through this one function. They have to agree byte-for-byte, because the routing store refuses a waitpoint whose id is not stamped for the shard it is being written to. Kept separate from friendlyId.ts because it needs resolveShard, and runOpsResidency.ts already imports friendlyId.ts.
…tance
resolveInheritedMintKind now returns { kind, shardChar? } instead of a bare
kind, and mintFriendlyIdForKind takes that object. A gen-2 parent hands its
own shard char to its children, so a run tree never splits across shards.
The shard char and the region both occupy index 24 of a run-ops id, so they
travel in one object rather than as two independent optional parameters: a
caller cannot set two competing sources for one slot, and the gen-2 arm
simply ignores the region.
mintAnchoredRunFriendlyId keeps its signature, its keying on the batch id
shape, and its synchronous form. Callers of the batch mint still break at
this commit; the next two commits repair them.
…default Adds resolveRunMintTarget: a parent means inherit by id-shape, no parent means resolve the org's mint kind and then, only on the run-ops path, the environment's mint shard. Three services carried this branch separately and one had already drifted, so it now lives in one function with an injectable deps parameter for tests. resolveMintShard gains an early return when no shard descriptor is configured. It matters for more than speed: the flag read happens before the routable-key bound is applied, so without this guard, merging would add a control-plane replica query to the root trigger path on every deployment that has no shards. With it, an unconfigured deployment takes a literally unchanged path — no query, no cache write, no log line. Both knip suppressions for that module are dropped now that it has real importers.
triggerFailedTask duplicated the mint branch inline rather than calling the shared helper, and it had drifted: it dropped the caller's region, and once gen-2 ids exist it would mint a gen-1 id for a child of a gen-2 parent. The router would then write that child to the gen-1 store while its parent lives on a shard, splitting one run tree across two databases. Both trigger services now call resolveRunMintTarget. triggerTask's behaviour is unchanged. The pre-minted runFriendlyId pass-through stays ahead of the resolver: batchTrigger and runEngineHandlers hand in an id already minted from the batch, and re-resolving it would move the item off its batch's shard. Added a container test for that, since no pure test can reach the guard and a typecheck will not notice if it moves below the resolver.
batchIdForMintKind and resolveBatchMintKind now take and return the mint target, so a child batch carries its parent run's shard char and a root batch mints by the environment's policy. Batch-anchored item minting needs no change: it already keys on the shape of the batch id. This is where the type change actually bites. resolveBatchMintKind declared Promise<RunIdMintKind>, so the inheritance change makes it a compile error, and the obvious repair -- comparing kind.kind -- would compile while silently dropping the shard char. The rewritten tests cover both arms, including the two that pin the rule that the flag resolver is never consulted for a child. batchTriggerV3.mintChildFriendlyId keeps its own branch and its injected resolveMintKind. Its root arm is unreachable in production and that injection point is what lets a test drive it without a database.
Enumerates every site that creates a Postgres waitpoint row, and asserts no scanned source still mints an id with the un-stamped helper. This commit is deliberately RED: five textual uses remain, so the drift assertion fails until the last mint site is converted. That is the point of landing it first -- the guard proves it can fail without anyone having to break a working site to demonstrate it. The four following commits each remove one or more of those uses. The guard walks the coordinator directory rather than a fixed file list, so a mint in a new coordinator file cannot hide from it, and it counts the waitpoint write calls too -- a site that omits the id entirely lets Prisma's cuid default fire after the write, which no stamp check can see. Scope includes the run store's two physical writers of the associated waitpoint row, read as text only. Those are the writes that bypass the routing store's stamp check, so they are exactly the ones a census must see.
…hor's shard Both sites already receive the owning run id, which is what they use to co-locate the row, so the mint just uses the same anchor. A gen-1 or legacy anchor keeps a cuid. The MANUAL retry loop re-evaluates the mint on every attempt, as it did before. The anchor does not change between attempts, so a retry lands on the same shard with a fresh id. Census guard: 4 textual uses of the un-stamped helper drop to 1.
… shard This is the one waitpoint site whose write is not covered by the routing store's stamp check: the row goes in as part of createRun, written inside the run store on the client the run itself routed to. An unstamped id there lands on a gen-2 shard, the completion fallback probes only the gen-1 pair, and the parent run waits forever with nothing logged. mintAssociatedWaitpointData had no run id to stamp from, so anchorRunId is now a required parameter on the coordinator interface. Required rather than optional on purpose: a caller that forgets it is a compile error instead of a silent cuid. All three callers already had the id to hand. Census guard: the last coordinator use is gone, leaving one in the engine.
Stamped from the batch id rather than the blocked run's. The create passes only completedByBatchId, so the routing store resolves the owner from the batch and checks the stamp against the batch's shard; stamping from the run would make that check throw. The two are the same shard in practice, and structurally so rather than by luck: all three callers mint the batch from the parent run id they then block, in the same request. A test pins that. Census guard: the last un-stamped mint is gone, so the drift assertion added four commits ago is now green.
…ironment's shard A token has no owning run, so the environment's mint shard decides where it lands. The id is minted inside the coordinator, so the shard key travels with the call rather than being resolved at the route. The gen-2 arm passes no residency hint at all. That hint outranks the id shape in the routing store and can only name a gen-1 store, so keeping it would write the row to the gen-1 store while its completion routed to the shard -- every run blocked on that token would then wait forever. Without a hint the stamped id routes the write, and the id-less dedup read probes across shards exactly as a gen-1 token's read does today. The gen-1 arm keeps the hint and its current behaviour. Resolving the shard at the route costs no query: the org flags it reads are already loaded on the authenticated environment.
… is off One named test per mint path -- root run, child run, root and child batch, batch item, all four waitpoint sites, standalone token -- asserting each produces the id it produced before gen-2 existed. This is the merge test as an executable claim rather than a paragraph. Also picks up an indentation fix the formatter made to the coordinator types.
…ke the census site-granular An adversarial review found the census guard was file-granular where the requirement is site-granular, and that no test bound a create site to its anchor. Both were real: a fifth mint added inside an already-catalogued file passed, and swapping any site's anchor for undefined passed every test on the branch while silently reverting that site to a cuid. The catalog now records the exact mint expression per site, and the proof test counts each one per file. It walks the whole engine tree rather than the coordinator directory alone, so a mint moved back into systems/ -- where they all lived before the coordinator seam -- is visible. Test-support trees are excluded explicitly, since a helper writing through raw Prisma never reaches the routing store. Both holes were confirmed closed by reintroducing them and watching the guard fail. The site tests now drive the real create sites through a capturing run store rather than calling the mint helper with a hand-written literal, including the standalone-token arms and the precedence of an owning run over the environment shard. Also: deletes a test that duplicated another file while claiming to guard the failed-run path it never imported; adds the missing gen-2 batch-anchor case for batch items; corrects the standaloneShardKey contract text, which stated a rule its only caller does not follow; and corrects the BATCH comment, which claimed stamping from the run "would throw" when on the normal path both stamps agree and it would not.
|
WalkthroughThe PR centralizes run and batch mint-target resolution with support for generation-2 shard inheritance. It updates run and batch ID generation to use structured targets. It adds shard-aware waitpoint minting for anchored and standalone waitpoints, including explicit standalone shard routing. It preserves legacy CUID and generation-1 behavior. New tests cover minting formats, shard inheritance, feature-flag changes, waitpoint creation paths, and catalogued mint sites. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed, relevant context and testing information, but it does not follow the repository template. It omits the issue closure line, checklist, explicit Testing section, Changelog section, and Screenshots section. Resolution Add the required template sections. Include a valid "Closes #<issue>" reference, complete the checklist, move test details into a Testing section, add a concise Changelog entry, and provide screenshots or state that screenshots are not applicable.
✨ 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 |
Consolidating the mint branch dropped the region on the inherited arm. The previous code passed it on both arms, so a child run stamped whatever region the caller asked for; without it a child of an unsharded parent stamped the default character instead. Ids for every existing deployment have to be unchanged, so this is a regression rather than a cosmetic slip. A shard character still outranks the region, since both occupy the same slot and only one of them can be authoritative. The inertness suite missed it by asserting the version character but not the region character. Both are now asserted, for an inherited parent with and without a shard.
Minting gen-2 batch ids broke batch waits. The batch-completion writer was resolved by a binary probe: look for the row on the new store, otherwise assume legacy. A gen-2 batch lives on neither, so the probe fell through to legacy, the update found no row and threw, the callback died before tryCompleteBatch, the batch waitpoint stayed pending, and the parent run waited forever with nothing logged as a hang. Found by running it: a gen-2 batchTriggerAndWait parent never resumed, while the same task on a gen-1 batch completed in twenty seconds. A gen-2 batch id names its own shard, so it now routes by that and never probes. An id naming an unconfigured shard throws rather than guessing a store, because guessing is precisely what strands the run. Both new tests fail without this change, the first on a fake client that throws if the new store is probed at all.
| timeout, | ||
| tags: bodyTags, | ||
| standaloneResidency: residency, | ||
| standaloneShardKey, |
There was a problem hiding this comment.
🔍 Standalone waitpoint tags not shard-aware
A standalone token now mints its id onto a gen-2 shard via standaloneShardKey, but its tags are still created with only the coarse NEW/LEGACY residency. upsertWaitpointTag routes tags to the gen-1 NEW store while the waitpoint row lands on the shard, so once a shard is configured the tag rows and the waitpoint row diverge across databases. Inert today because resolveMintShard returns "new" while RUN_OPS_SHARDS is empty. Confirm the sharded tag path accounts for this.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8e345e8-ca22-4245-9b46-77cb8e04adc9
📒 Files selected for processing (3)
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.ts
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/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/runEngineHandlers.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/test/runEngineHandlers.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/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.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/test/runEngineHandlers.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/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/runEngineHandlers.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.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:
apps/webapp/app/v3/runEngineHandlers.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/runEngineHandlers.test.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:
apps/webapp/test/runEngineHandlers.test.ts
| const shardKey = resolveShard(batchId); | ||
| if (shardKey !== "new" && shardKey !== "legacy") { | ||
| const shard = deps.shards?.find((s) => s.key === shardKey); | ||
| if (!shard) { | ||
| // Writing to a guessed store is what strands a run. Fail loud instead. | ||
| throw new Error( | ||
| `resolveBatchRunOpsWriter: batch "${batchId}" names shard "${shardKey}", which is not configured` | ||
| ); | ||
| } | ||
| return shard.writer; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a crumb for shard writer selection.
Record the batch ID and resolved shard key before this branch selects the writer. This new branch changes the database destination and has no temporary routing crumb.
As per coding guidelines, “Add crumbs as you write code” and use an existing namespace or ask before creating one.
Source: Coding guidelines
| it("a gen-2 batch resolves to its own shard writer", async () => { | ||
| const shardWriter = {} as never; // identity is the whole assertion; no database is touched | ||
| const gen2BatchId = `${"a".repeat(24)}a2`; | ||
|
|
||
| const writer = await resolveBatchRunOpsWriter(gen2BatchId, { | ||
| newReplica: { | ||
| batchTaskRun: { | ||
| findFirst: async () => { | ||
| throw new Error("a gen-2 batch id must never probe the NEW store"); | ||
| }, | ||
| }, | ||
| } as never, | ||
| newWriter: {} as never, | ||
| legacyWriter: {} as never, | ||
| shards: [{ key: "a", writer: shardWriter as never }], | ||
| }); | ||
|
|
||
| expect(writer).toBe(shardWriter); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move these resolver tests next to the source file.
Place these tests in apps/webapp/app/v3/runEngineHandlersShared.server.test.ts. The tested export is in apps/webapp/app/v3/runEngineHandlersShared.server.ts.
As per coding guidelines, “Test files go next to source files.”
Also applies to: 518-527
Source: Coding guidelines
| const writer = await resolveBatchRunOpsWriter(gen2BatchId, { | ||
| newReplica: { | ||
| batchTaskRun: { | ||
| findFirst: async () => { | ||
| throw new Error("a gen-2 batch id must never probe the NEW store"); | ||
| }, | ||
| }, | ||
| } as never, | ||
| newWriter: {} as never, | ||
| legacyWriter: {} as never, | ||
| shards: [{ key: "a", writer: shardWriter as never }], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace the database client doubles with testcontainers.
newReplica, newWriter, legacyWriter, and shardWriter are hand-built mocks. Use real run-ops clients backed by testcontainers. Configure the new store so an accidental probe selects a different writer, then assert that the configured shard writer is returned.
As per coding guidelines, “Never mock anything - use testcontainers instead.”
Also applies to: 520-524
Source: Coding guidelines
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. |
Summary
Adds the id-minting half of sharding run data across several databases. Every entity that co-locates with a run now carries the run's shard key inside its own id, so its row is routable on its own instead of needing a directory table or a scatter across shards.
Nothing changes for users yet. With no shard descriptors configured, every mint path produces exactly the ids it produces today, and the trigger path issues no extra query.
Design
A run's mint target travels as a single object carrying the kind and, when sharded, the shard character. The shard and the caller's region both occupy index 24 of a run-ops id, so passing them together makes it impossible for a caller to set two competing sources for one slot.
A child run, a batch and a batch item read the shard from their parent's id rather than resolving a fresh one, so a run tree never splits across databases. Three services carried that branch separately, and one had already drifted, so it now lives in one function.
Waitpoints mint through one shared pure function used by both the webapp and the run engine. They have to agree byte for byte, because the routing store refuses a waitpoint whose id is not stamped for the shard it is being written to:
The core is always freshly minted rather than derived from the anchor, since a derived body would be byte-identical to the run's own id.
One latent bug fixed on the way: the failed-run path duplicated the mint branch inline and had drifted, so a child of a sharded parent would have been written to a different database from its parent.
Guarding the create sites
The expensive failure here is a waitpoint minted without its anchor's shard: one of the five create sites writes through a path that has no stamp check, so a miss there strands a blocked run with nothing logged. An enumerated census plus a source scan fails when a new create site appears, when an existing one stops passing its anchor, or when a site is added to a file the scan does not yet cover.
The census was written before any site was converted, so it went red on the first commit and green as the last site landed. Both holes an earlier draft had, a file-granular count and a scan that missed the directory these mints used to live in, were confirmed closed by reintroducing them and watching the guard fail.
Before enabling a shard
Merging this is inert: with the mint list empty the resolver returns before it reads anything, and
ids are identical to a measured
mainbaseline. Verified against a live shard locally, includingthat the resolver issues no query across thirty triggers with no shard configured.
Enabling is gated on two other pull requests, both open, both by the same author, each of which owns
the file involved:
at all: the wait route resolves the waitpoint through read-through, which is shard-blind, so the
wait fails. Do not set the mint list before it merges.
physical database as the gen-1 store boots without complaint, which voids the disjointness the
fan-out sums rely on.
Testing also turned up a silent read-path gap that neither pull request covers: the paths that
hydrate runs from ClickHouse through a fixed pair of Postgres clients drop gen-2 rows on the floor,
so the runs list would show fewer rows than its own count with nothing logged. That needs its own
change before a shard carries real traffic, and it is filed as such.
Notes for reviewers
Four commits in the middle of the stack do not typecheck in isolation: a signature change and its call-site repairs are separate commits, so bisecting inside the stack needs care. Commit
845ab06also understates itself, since it rewrites the primary trigger path's mint alongside the failed-run path it names.No changeset and no server-changes entry: every path is inert while the feature is off, so there is nothing to tell users yet.