feat(core,webapp,run-engine): stamp a shard key onto run, batch and waitpoint ids - #4788
Open
d-cs wants to merge 17 commits into
Open
feat(core,webapp,run-engine): stamp a shard key onto run, batch and waitpoint ids#4788d-cs wants to merge 17 commits into
d-cs wants to merge 17 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.
|
Contributor
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 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly covers the change, design, testing, rollout dependencies, risks, and changelog rationale. It does not include the template checklist, issue reference, or screenshots section, but these omissions do not prevent the description from being 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
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.
Contributor
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
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29a8ccc1-f29a-4539-b923-dbe679126e2c
📒 Files selected for processing (3)
packages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.tspackages/core/src/v3/isomorphic/waitpointMint.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (50)
- GitHub Check: report
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: sdk-compat / Node.js 24.18 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 20.20 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 26.4 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - npm)
- GitHub Check: sdk-compat / Node.js 22.23 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (10)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.ts
**Import subpaths only** (never root).
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
Never import the root package (`@trigger.dev/core`). Always use subpath imports such as `@trigger.dev/core/v3`, `@trigger.dev/core/v3/utils`, `@trigger.dev/core/logger`, or `@trigger.dev/core/schemas`
📄 CodeRabbit inference engine (packages/core/CLAUDE.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.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:
packages/core/src/v3/isomorphic/waitpointMint.test.tspackages/core/src/v3/isomorphic/friendlyId.tspackages/core/src/v3/isomorphic/runOpsResidency.ts
🔇 Additional comments (4)
packages/core/src/v3/isomorphic/runOpsResidency.ts (1)
1-1: LGTM!packages/core/src/v3/isomorphic/waitpointMint.test.ts (3)
78-88: LGTM!Also applies to: 98-117
3-9: LGTM!
89-96: 🎯 Functional CorrectnessNo change needed.
SHARD_CHAR_PATTERNaccepts all lowercase letters and digits, so every loop character is valid.
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
|
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
1 participant
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.