feat(core,webapp,run-engine): stamp a shard key onto run, batch and waitpoint ids - #4788
Merged
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.
|
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 (24)
🚧 Files skipped from review as they are similar to previous changes (23)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (51)
🧰 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:
🪛 ast-grep (0.45.2)internal-packages/run-store/src/placement.proof.test.ts[warning] 42-42: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) 🔇 Additional comments (2)
WalkthroughThe PR centralizes run and batch mint-target resolution with 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 routes batch and waitpoint tag writes to generation-2 shard stores. It preserves legacy CUID and generation-1 behavior. Tests cover minting formats, shard inheritance, waitpoint creation, routing, and mint-site coverage. Merge Risk: 🔵 Low · up to This PR changes how run-related IDs are stamped for future sharding and remains inert while sharding is disabled. The current head still lacks required temporary diagnostic markers in affected paths, so it is mergeable with explicit owner awareness and follow-up before shard rollout. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the change, design, testing, dependencies, known gaps, and reviewer notes. It does not reproduce the template's issue link, checklist, changelog, or screenshots sections, but the missing sections are non-critical because the required technical and testing context is present.
✨ 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
marked this pull request as ready for review
August 26, 2026 13:12
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.
![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.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Observability mapAs of Nothing in this pull request moves the report any more. The findings an earlier push reported are gone. The 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. |
The 404 was thrown from inside the try block, and json() returns a Response, so the catch swallowed it into a 500. The log line recorded the error as an empty object, which made a missing token indistinguishable from a broken server: diagnosing one took a control experiment rather than reading the response. Rethrows a Response untouched, matching the pattern already used by the batch results route.
…-13430 # Conflicts: # apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts
@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: |
resolveShard decoded the 24-char base32hex core to recover a timestamp and then discarded it. That is an indexOf into a 32-char alphabet per character, an array, a Uint8Array and a Date, to answer a question the shape already answers: 587ns per call for a run-ops id, against 61ns for the shape check. It matters because the router calls this on every routed read and write, and because stamping waitpoint ids put it on the waitpoint-create path where it had not been before. There it more than doubled the cost of minting an id. The alphabet is [0-9a-v], so "the shape matches" and "the decode would not throw" are the same predicate. Checked against the decoding parsers over 300,000 inputs, including 40,000 adversarial 26-char strings with out-of-alphabet characters in every slot, both prefixed forms, and the store-format waitpoint shape: zero disagreements. The equivalence is pinned by tests, since drift here misroutes silently rather than erroring. The decoding parsers keep their timestamps for the callers that want them.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
Author
Manual testingExercised end to end against real Postgres databases and a real dev worker, not just unit tests. Summary of what was run and what it found. TopologiesAll four database layouts, each booted and exercised:
Shards were tested three ways: a separate database on the same Postgres instance, a separate instance, and a descriptor that aliases the existing run-ops store (no new database at all). Feature offThis is the state that matters for merging, since nothing is configured by default.
Feature on
Defects found and fixed here
Two further problems turned out to be artefacts of a stale branch rather than real: both disappeared on merging Not covered
|
… live on A tag row carries no id the router can read, and the residency hint only ever names a gen-1 store, so an environment minting gen-2 tokens wrote its tags to a different database from the tokens they describe. Reads already fan out over every store, so the row was still found: the symptom was a tag attributed to the wrong database rather than an error. The token route already resolves the environment's mint shard, so it now passes that through as an explicit hint, which takes precedence over residency. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The positive arm asserted which client object came back from a set of empty doubles, so it could not tell a correct resolution from one that resolved to a database holding no such batch. It now runs on two containers: the shard is one database, both gen-1 slots are the other, the batch is seeded only on the shard, and the assertion is that the rows committed there and the gen-1 database stayed empty. Verified to fail when the shard arm is removed. The throwing double survives for the separate "never probes the gen-1 store" assertion, where a call that must not happen is only observable if the client throws when touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The existing waitpoint census is exhaustive over id production and asks "is this id stamped with a shard?". A row with no minted id of its own is invisible to it, which is how a tag could write to a gen-1 store for a gen-2 environment while every functional test passed: reads fan out over every store, so the row was still found. This census is exhaustive over row placement instead. Every method on the RunStore interface is classified as a read or as a write, and the union is diffed against the interface, so a method added there fails until somebody classifies it. Each write records what it routes by, verbatim, and the combination that must never exist is a write which can name nothing better than the binary residency hint and whose miss is silent. Where safety is a claim rather than a mechanism, a fan-out or a residency fallback, the catalog has to argue it in prose. Of the 41 mutating methods, 40 route by an id and one takes an explicit shard key. Each of the six guards was verified by reintroducing the defect it claims to catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The residency hint can only ever name a gen-1 store, so when a waitpoint's own id names a gen-2 shard the hint is not a worse answer, it is an answer it cannot express. It was being checked first, which meant a caller passing both wrote the row to a gen-1 database while its id said otherwise. Silently: a create never misses, and the read path fans out, so the row is still found afterwards. Nothing hit this. The one call site that could withholds the hint deliberately and says why in a comment. That made correctness a convention observed at one site rather than an invariant, so the router now skips the residency arm when the id names a gen-2 shard. The owner anchor still outranks both, and gen-1 ids, cuids and the no-hint path are unchanged: four of the six new tests pass on either precedence, which is what makes them worth keeping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…abases A tag row is the one run-ops row with no id the router can read and no owning row to follow, so placement was only covered by fake-store routing assertions. These run on the four-store matrix (legacy, new, and two gen-2 shards, each its own database) and assert where the row physically landed. Two of the four fail without the shard routing; the other two pin the gen-1 path and the read fan-out, which are unchanged, so they pass either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… one store A tag row has no id the router can read, so the same logical tag gets an independent cuid on every store that ever wrote it: the unique index is (environmentId, name) and it is per-database. The read merged by id, so a tag name was returned once per store holding it. An environment that had tags before it was pinned to a shard would see the name listed twice. The merge now keys on (environmentId, name). Dropping a row is safe because nothing consumes a tag's id: a waitpoint carries its tags as a string array and this table is a name registry for listing and autocomplete. Two tests on the four-store matrix: the same name on a gen-1 store and a shard is listed once, and two environments keep their own tag of that name. The second queries without an environment filter on purpose, because a per-environment filter would pass whatever the dedupe key was. Also scopes the placement census to each method's own body. Three creates share one route expression, so a file-wide search passed when one lost its route and a sibling kept it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit replaced the tag read's id dedupe with a name dedupe, which dropped an invariant that two tests pin: drain can mirror a tag onto the new store while it keeps its id, so the same id appears on two stores and the new store's copy is authoritative. Keying only on name let a stale mirrored row survive under its old name. Tags need both keys, in order. The id pass resolves a mirrored row and keeps the duplicate alarm. The name pass then collapses the separate case, where a store that never saw the tag minted its own cuid for it, so one logical tag holds a different id per store. Restricting the name pass to rows that survived the id pass stops a stale mirror winning its name back, and filtering rather than rebuilding keeps each winner's position for callers that pass no orderBy. Each pass was verified by removing it: without the id pass the NEW-wins test fails, without the name pass the cross-store duplicate test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… arm Every production caller supplies runId, because the only entry point is the wait.duration route and that is keyed on a run friendly id. A caller that omitted it on a gen-2 environment would mint a cuid and route by residency, putting the row on a gen-1 store while the run waiting on it lives on a shard, and nothing would fail at write time. Comment only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nicktrn
approved these changes
Aug 27, 2026
Comments were 15% of the added lines. Removed the ones that restate the code, the duplicates, and the narration, and compressed the rest to the fact each one carries. Now 10%. The largest cuts: a five-line note on standaloneShardKey that appeared verbatim in three files, now once at the type; the batch-completion explanation duplicated between the resolver and its test; and the header essays on the two catalogs. Kept what is not recoverable from reading the code: that a residency hint can name only a gen-1 store, that drain can mirror a tag onto the new store while it keeps its id, that a tag has no id to route by, that the run store's write path has no stamp check, and why each census guard exists. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/run-store/src/placement.proof.test.ts (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumb coverage to the new helpers.
No
@crumbsmarker or enclosing@crumbsregion covers these helpers. Add one before merge.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd5c7358-d54f-4532-a3c4-2d01dc4b956e
📒 Files selected for processing (21)
apps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.tsapps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.tsapps/webapp/test/runEngineHandlers.test.tsinternal-packages/run-engine/src/engine/index.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.tsinternal-packages/run-store/src/placement.proof.test.tsinternal-packages/run-store/src/placementCatalog.tsinternal-packages/run-store/src/runOpsStore.nShardMatrix.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.tsinternal-packages/run-store/src/types.tspackages/build/src/package.jsonpackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/waitpointMint.ts
💤 Files with no reviewable changes (1)
- internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMint.proof.test.ts
- internal-packages/run-store/src/runOpsStore.shardMap.test.ts
- apps/webapp/app/v3/runEngineHandlersShared.server.ts
- internal-packages/run-store/src/placementCatalog.ts
- packages/core/src/v3/isomorphic/waitpointMint.ts
- internal-packages/run-store/src/runOpsStore.nShardMatrix.test.ts
- internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintSites.test.ts
- apps/webapp/test/runEngineHandlers.test.ts
- internal-packages/run-store/src/types.ts
- internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts
- packages/core/src/v3/isomorphic/waitpointMint.test.ts
- packages/core/src/v3/isomorphic/friendlyId.ts
- internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
- internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointMintCatalog.ts
- apps/webapp/app/v3/runOpsMigration/resolveRunMintTarget.server.ts
- internal-packages/run-store/src/runOpsStore.ts
- internal-packages/run-engine/src/engine/index.ts
- apps/webapp/app/v3/runOpsMigration/gen2MintInertness.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 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:
internal-packages/run-store/src/placement.proof.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-store/src/placement.proof.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/build/src/package.jsoninternal-packages/run-store/src/placement.proof.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/placement.proof.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/placement.proof.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-store/src/placement.proof.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/run-store/src/placement.proof.test.ts
🪛 ast-grep (0.45.2)
internal-packages/run-store/src/placement.proof.test.ts
[warning] 50-50: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^ {2}(?:async )?${method}(?:<[^\\n]*?>)?\\(, "gm")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (1)
packages/build/src/package.json (1)
1-3: LGTM!
Deleted rather than reworded this time. The cuts fall into three groups: facts stated in a production file and repeated in its test, field docs that restate the field's own name or return type, and test comments that repeat what the test name already says. Comment lines added by this branch: 191 down to 93. Comment-only, verified by diffing out every commented line and finding nothing left. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
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.