feat(webapp,run-engine): Redis waitpoint coordinator arm behind a per-org mint flag - #4793
Draft
d-cs wants to merge 20 commits into
Draft
Conversation
…ncy (#4781) Gives read-through and idempotency their gen-2 shard arms, so an id that names its own shard is read there and nowhere else. #4764 has landed, so this now targets `main` directly and no longer depends on an unmerged branch. It builds on what that PR supplied: `resolveShard`, `runOpsShardHandles` and the keyed router. TRI-13431 ## What changes **Read-through routes by `resolveShard`, not by the binary residency classifier.** A gen-2 id reads its own shard's replica once and probes no other store. A gen-1 v1 id still reads new only. **Callers now declare `idKind`.** A cuid gives no way to tell a run id from a waitpoint id, and the two must route differently: - a legacy-classified **run** id reads the legacy replica only — there is no cuid run migration, so the new-store probe cannot find it; - a cuid **waitpoint** keeps the new-first pair probe, which is load-bearing because a cuid waitpoint can be co-located with its run on the new store. There is no default, because a default would pick one of those arms silently. The field `runId` is renamed to `id`, since it carried both kinds already. **`ReadThroughResult` carries `found`.** `source` is an open-ended union once shards exist, so a consumer testing found-ness by listing the hit sources reads a gen-2 hit as a miss. One consumer did exactly that. Discriminating on `found` makes that class of bug a compile error rather than something a reviewer has to spot. **Idempotency resolves its client through one shard-keyed map.** Both call sites go through `clientForShardKey`, so they cannot disagree about which store owns an id. An absent key takes an explicit logged branch to the fallback, not a silent legacy default. The `classify` seam is retyped to return a `ShardKey`: `Residency` (`"NEW"`) and the reserved shard keys (`"new"`) differ only by case, and `ShardKey` collapses to `string`, so the compiler would not have caught feeding one into the other. The dead `isMigrated` branch is deleted. Nothing implemented it, and the one production comment recorded that omitting it was deliberate. **`PostgresRunStore._residency` widens to `ShardKey`.** Still unused; the store stays unaware of its siblings. ## Two behaviour fixes found while doing the above **An unconfigured shard key logs and returns not-found instead of throwing.** The waitpoint route takes the id from a URL parameter, and any base32hex core plus `[a-z0-9]` plus `"2"` parses as gen-2. The route turns a throw into a 500, so throwing here would let any authenticated client generate 500s and error logs by guessing shard chars, of which there are 36. An error-logged not-found is neither silent nor a misroute. Throwing stays correct on the router path, where ids are minted rather than received. **The two cross-seam batch hydration sites were gen-2 blind.** `hydrateRunsAcrossSeam` and `ApiBatchResultsPresenter` classified with the binary `ownerEngine`, so a gen-2 run id joined the gen-1 `new` group, missed there, and — classifying dedicated-family — never reached the legacy probe either. The id was dropped from a bulk-action page and from batch results with no error. Both now partition ids by shard key and read each configured shard once. Also: a gen-2 waitpoint that missed its shard replica fell back to the gen-1 new writer, a different database, silently disabling read-your-writes for the freshly minted token that fallback exists to serve. It now falls back to its own shard's writer. ## Merge safety Inert while `RUN_OPS_SHARDS` is unset: the shard maps are empty, so every gen-2 arm is unreachable, and gen-2 minting is not live yet. The one live change is the gen-1 run arm, and it removes work rather than adding it. `RoutingRunStore.findRun` never forwards the caller's client object — it routes by id and reads only the client's presence and replica brand — so `readRunForEvent`'s "new" closure already resolved a legacy-classified run id to the legacy store. The arm removes a duplicated read of the legacy replica. A test pins this, because a future caller passing a raw client and a run id would lose the pre-cutover 27-char case, which is new-resident but classifies legacy. ## Testing 14 tests added, testcontainers throughout, no mocks. 22 affected test files pass; typecheck, lint, format and knip are clean. Both arms were verified by neutralising them and confirming the new tests fail. The batch-results test needed rewriting after that check: the first version passed with the fix neutralised, because it used one container as both the gen-1 new client and the shard replica, so it was not testing what it claimed. Note for review: run testcontainer suites in small batches. Sixteen at once starves Docker and everything times out at 60 seconds. The run-ops legacy-guard baseline is refreshed in its own commit. The baseline is keyed by line number, so partitioning the batch-results read shifted four pre-existing entries and added one. Baselined violations in that file go from four to five, all reads; the new one is the shard read beside two gen-1 reads already there. No changeset and no `.server-changes` entry: a user notices nothing while the flag is unset.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds waitpoint system configuration and a validated organization feature flag. It resolves mint kinds with global defaults, organization overrides, bounded caching, replica fallback, and fail-safe legacy behavior. It extends coordinator contracts for batch waitpoints and mint kinds. It adds Redis-backed waitpoint coordination, status handling, completion delivery, idempotency, batch guards, and database projections. Existing batch creation now uses the coordinator while preserving duplicate-key behavior. It also adds shard-aware read, hydration, idempotency, and unroutable-ID handling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on-topic, and covers the design, behavior, and verification results. It does not use every template section, but it provides equivalent testing and change details, so the description is mostly complete. Full details: Docstring CoverageExplanation Docstring coverage is 34.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 37 files. (1 skipped: 1 unsupported.) ✨ 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 |
The org's waitpointSystem flag decides where a NEW waitpoint is minted; WAITPOINT_SYSTEM_DEFAULT is the fallback and defaults to legacy. A flag-read failure mints legacy, matching computeRunIdMintKind's fail-safe. No flip-grace machinery: every operation after a mint routes by the waitpoint's id shape and never re-reads the flag, so a flip can never split one waitpoint across the two systems. Nothing consumes this yet.
…nator seam blockRunWithCreatedBatch built its waitpoint with runStore.createWaitpoint directly, so it had no arm to route to. It now goes through the coordinator. The P2002 catch moves to the legacy arm, where it belongs: it is the duplicate-batch contract for a unique index, and it is dead against a store that reports a duplicate through NX instead. Leaving it wrapped around a store create would read a genuine store error as a duplicate batch. The block step keeps its own P2002 catch. The previous shape wrapped the create and the block in one try, so a P2002 from either returned null; narrowing that here would be a behaviour change smuggled into an extraction. Seam also gains the mint kind on the create params and batchWaitpointId on the lockless params. Both are pinned to their legacy values at every call site, so behaviour is unchanged.
The coordinator seam returns Prisma Waitpoint, and callers read its columns directly, but a store-resident waitpoint has no row. This maps the store's record, status and completion onto that shape. Every column is listed explicitly rather than spread. A missed non-null column would surface as undefined in a consumer far from here that had no reason to guard, and the type checker catches an omission here instead. An absent idempotency key throws rather than synthesizing one: the column is non-null and half of the (environmentId, idempotencyKey) unique index, so an invented value could collide with a real one.
Implements the coordinator seam against the Redis store, so waitpoint state can live there instead of Postgres. Unreachable until a mint routes to it. Three rules carry the correctness weight: An edge that is in neither the run's pending nor its delivered set reports PENDING and increments a counter. The store keeps every edge in exactly one of those sets, so being in neither means the run shard lost state. Reading that as "not pending, therefore complete" would resume a run whose waitpoint never completed. Note this is deliberately not a rule about completion envelopes: a waitpoint can be COMPLETED carrying none, and treating that as unresolved would block a healthy run forever. A lockless absorb refuses to write item edges unless the parent's BATCH waitpoint is present and still pending. Absorbing items without the run lock is only safe while that waitpoint holds the pending set open, otherwise a concurrent completion can see an empty set mid-absorb and resume the parent early. The MANUAL projection row is written after the store commit and never read back for coordination. A failed projection write is logged and counted rather than thrown: the waitpoint already exists and is already coordinating, so failing the create would report failure for work that succeeded. Also adds a single-key record read to the store client. The seam returns the Postgres row shape and only the immutable record carries the columns that shape needs.
d-cs
force-pushed
the
feat/waitpoint-mint-flag-wiring-tri-13442
branch
from
August 26, 2026 16:07
fd52adc to
c4e21e6
Compare
…lag-wiring-tri-13442
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5ef88f7-7af6-48fe-91ac-762eef3cff06
📒 Files selected for processing (14)
apps/webapp/app/env.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsapps/webapp/vitest.config.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/storeArm.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
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/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.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/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsapps/webapp/app/env.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.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/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsapps/webapp/app/env.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.tsapps/webapp/app/env.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.tsapps/webapp/app/env.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsapps/webapp/app/env.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.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/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsapps/webapp/app/env.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.tsapps/webapp/app/env.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.tsapps/webapp/app/env.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.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-engine/src/engine/waitpointCoordinator/waitpointShape.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.tsapps/webapp/vitest.config.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/index.tsapps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/waitpointShape.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.test.tsapps/webapp/app/env.server.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.ts
🧠 Learnings (1)
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
apps/webapp/app/v3/waitpointMigration/waitpointMintKind.server.test.ts
Comment on lines
+170
to
+185
| async #assertBatchWaitpointPending(params: RegisterBlocksLocklessParams): Promise<void> { | ||
| if (!params.batchWaitpointId) { | ||
| return; | ||
| } | ||
|
|
||
| const state = await this.store.readBlockState(params.runId); | ||
| if (state.pendingIds.includes(params.batchWaitpointId)) { | ||
| return; | ||
| } | ||
|
|
||
| this.batchGuardViolations.add(1); | ||
| throw new Error( | ||
| `Lockless absorb for run ${params.runId} requires the parent BATCH waitpoint ` + | ||
| `${params.batchWaitpointId} to be present and pending on the run shard` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the parent-pending guard atomic with item-edge absorption.
Lines 175-177 only perform a preflight read. A concurrent completion can remove the parent BATCH entry after this read and before registerBlocks() stores item edges. The parent run can then observe no pending waitpoints and resume before the item edges exist.
Move the parent-state assertion into the run-shard operation that absorbs the item edges.
Collaborator
Author
There was a problem hiding this comment.
Valid, and I am leaving this thread open for a human call rather than closing it.
You are right that the check does not close the window. It reads the run shard, then the absorb writes in a separate operation, so a completion landing between the two is detected on a later call rather than prevented. It is a detector, not a barrier. 8c4c6af states that in the code instead of letting the previous comment imply atomicity, and makes a missing parent id throw rather than skip, but neither of those makes it atomic.
Closing it properly means moving the pending-set assertion inside the absorb script so the check and the edge write share one atomic action. That edits the Lua the earlier coordinator work owns, which is why I am not doing it unilaterally here.
Worth noting the plan this work follows anticipated the gap: it assigns the in-code assertion to this stage and the race test to a later one, on the grounds that this variant is unmodeled in both verification campaigns. So the current state is what was specified. Whether specified is good enough is the question I am putting to a reviewer.
Contributor
There was a problem hiding this comment.
@d-cs, agreed. The current code detects the unsafe state but does not prevent the race.
The finding remains open because #assertBatchWaitpointPending() and store.registerBlocks() are separate run-shard operations. A complete fix must perform the parent pending-set check and item-edge absorption in one atomic Lua operation.
The stated staged plan explains the current implementation. It does not remove the correctness risk. A human reviewer can decide whether to accept that deferred work for this PR.
(ᵔᴥᵔ)
✏️ Learnings added
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4793
File: internal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.ts:176-197
Timestamp: 2026-08-26T16:34:03.944Z
Learning: In `internal-packages/run-engine/src/engine/waitpointCoordinator/storeArm.ts`, `StoreWaitpointCoordinatorArm.#assertBatchWaitpointPending()` is intentionally a non-atomic preflight detector. It reads run-shard state separately from `store.registerBlocks()`, so it cannot prevent a concurrent completion between the check and item-edge absorption. To close this race, the pending-set assertion and absorption must execute in the same run-shard Lua operation. The migration plan assigns the in-code detector to the current stage and defers the race test to a later stage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
@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: |
…the batch guard Four fixes from review. A completed MANUAL waitpoint left its Postgres projection row PENDING. The token API and the dashboard read status, output and completedAt from that row, so a finished token reported as still waiting with no output. The completion now writes through to the projection, best effort like the create-time write. A lockless absorb that arrives with no parent BATCH waitpoint id now throws instead of returning early. Skipping silently meant an unwired caller would disable the pending-set guard rather than fail, which is the exact failure the guard exists to catch. mintAssociatedWaitpointData gains anchorRunId on the coordinator contract. The store arm derives a RUN waitpoint id from the run's own id body, so without the anchor on the shared type the two arms disagreed about the call shape. The mint-kind resolver splits into a pure module and an env-bound wrapper, so its test no longer loads env.server through the import chain. Test import time drops from 2.7s to 7ms, which is the chain being gone rather than a speedup. Also states plainly in the code that the batch guard is a preflight detector and not a barrier: it reads the run shard, then the absorb writes separately, so a completion landing between the two is detected next call, not prevented. Closing that window means moving the assertion inside the absorb script.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts (1)
22-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required crumbs for the new shard-routing paths.
Add
//@Crumbsmarkers or an approved `#region `@crumbsblock for the new routing decisions. If no approved namespace applies, ask before adding one.
apps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts#L22-L40: add crumbs for configured-shard selection and fallback selection.apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts#L36-L44: add crumbs for the local shard-to-client mapping.apps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.ts#L15-L38: add crumbs for the new shard-routing test setup.As per coding guidelines, “Add crumbs as you write code” and do not invent a namespace.
Source: Coding guidelines
apps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts (1)
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMint the gen-2 fixture ID with
generateRunOpsIdV2.The hand-built string encodes the gen-2 layout as a comment. If the ID format changes,
resolveShardreclassifies this constant asneworlegacy, and the shard tests keep passing while exercising the gen-1 path instead. The sibling testapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsalready uses the generator.♻️ Proposed change
-// 26-char gen-2 body: shard char at index 24, version "2" at index 25. -const SHARD_A_RUN_ID = "run_" + "c".repeat(24) + "a2"; +const SHARD_A_RUN_ID = generateRunOpsIdV2("a");Add the import:
import { generateRunOpsIdV2 } from "`@trigger.dev/core/v3/isomorphic`";
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e327c8f-55cc-49f0-bbba-70d9938eef5b
📒 Files selected for processing (24)
apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/runOpsMigration/track1-baseline.jsonapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsinternal-packages/run-store/src/runOpsStore.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
Use Remix flat-file route conventions with dot-separated segments; for example, `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts
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/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.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/services/routeBuilders/unroutableId.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.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/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.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/services/routeBuilders/unroutableId.server.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/track1-baseline.jsonapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.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/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/unroutableIdStatus.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/services/routeBuilders/unroutableId.server.tsapps/webapp/test/unroutableIdStatus.test.tsinternal-packages/run-store/src/PostgresRunStore.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.test.tsapps/webapp/test/readRunForEvent.replicaLag.test.tsapps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.tsapps/webapp/app/v3/runEngineHandlersShared.server.tsapps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.tsapps/webapp/app/v3/runOpsMigration/shardHandles.server.tsinternal-packages/run-store/src/runOpsStore.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsinternal-packages/run-store/src/runOpsStore.shardMap.test.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.test.tsapps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.tsapps/webapp/app/services/routeBuilders/apiBuilder.server.tsapps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.tsapps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.tsapps/webapp/app/v3/runOpsMigration/readThrough.server.test.tsapps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
🧠 Learnings (2)
📚 Learning: 2026-08-21T14:26:14.909Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 4752
File: internal-packages/run-store/src/runOpsStore.shardMap.test.ts:5-11
Timestamp: 2026-08-21T14:26:14.909Z
Learning: For these RoutingRunStore unit tests, use an instrumented fakeStore() with a shared ordered call log when verifying routing algebra such as sequential probe order and merge precedence. Use testcontainer-backed tests separately for database behavior, including mixed residency and replica-lag scenarios.
Applied to files:
internal-packages/run-store/src/runOpsStore.shardMap.test.ts
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.tsapps/webapp/app/runEngine/concerns/idempotencyResidency.server.ts
🔇 Additional comments (11)
apps/webapp/app/presenters/v3/ApiBatchResultsPresenter.server.ts (1)
2-2: LGTM!Also applies to: 26-27, 188-238
apps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.ts (1)
34-36: 🗄️ Data Integrity & IntegrationNo caller-contract issue is present. The only call sites are in
apps/webapp/app/v3/services/bulk/BulkActionV2.batchReadThrough.server.test.ts; eachreadNewforwardsclient, and the provided logger defineserror. Other callers omit the optional logger.apps/webapp/test/apiBatchResultsPresenter.dedicatedSeam.test.ts (1)
6-6: 📐 Maintainability & Code QualityNo fixture change is needed.
makeNShardRunOpsPostgresTestis exported and provideslegacyPrisma,newPrisma, andshardPrismas.apps/webapp/app/v3/runOpsMigration/readThrough.server.ts (1)
6-17: LGTM!Also applies to: 26-76, 78-139
apps/webapp/app/v3/runOpsMigration/readThrough.server.test.ts (1)
16-30: LGTM!Also applies to: 46-46, 57-58, 69-69, 78-79, 91-98, 110-110, 126-127, 138-138, 152-198, 200-257, 259-308
apps/webapp/app/v3/runOpsMigration/waitpointTokenResolve.server.test.ts (1)
141-142: LGTM!Also applies to: 154-154
apps/webapp/app/v3/runEngineHandlersShared.server.ts (1)
38-39: LGTM!Also applies to: 51-51
apps/webapp/app/runEngine/concerns/resolveWaitpointThroughReadThrough.server.ts (1)
1-20: LGTM!Also applies to: 31-41, 57-90
apps/webapp/test/resolveWaitpointThroughReadThrough.readthrough.test.ts (1)
4-4: LGTM!Also applies to: 289-323, 325-360, 362-389
apps/webapp/app/routes/engine.v1.runs.$runFriendlyId.waitpoints.tokens.$waitpointFriendlyId.wait.ts (1)
41-48: LGTM!Also applies to: 65-69
internal-packages/run-store/src/PostgresRunStore.ts (1)
33-33: LGTM!Also applies to: 2761-2761
…-tri-13441' into feat/waitpoint-mint-flag-wiring-tri-13442
Contributor
Observability mapAs of 20/100 over 449 measured of 467 entry points (base 19, up 1) What this PR changed
FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
The mint-kind resolver and the shared mint-kind type are both dead code until the commits that wire them up land. Knip is right to flag them. The webapp module joins the ignore list beside runOpsMintShard.server.ts, which sits there for the same reason. The engine type takes a @knipignore tag, since that package has no ignore block. Both come back out when their consumers land.
Adds the router that sits in the coordinator slot and decides which arm owns a waitpoint. It holds no store or database client of its own: every method is a partition followed by delegation. Two rules, deliberately different. An operation routes on the id's shape, because the id exists and its residency is a fact. A store-shaped id with no store configured rejects rather than guessing, since guessing would operate on the wrong system silently. A create routes on the caller's mint kind, and a store mint with no store configured falls back to legacy with a logged error. There is no id yet, so nothing can be misrouted, and refusing would turn one badly configured process into a trigger outage for every organization with the flag set. A run blocked by one waitpoint of each kind is why the reads fan out to both arms and the pending counts sum. That sum is the dual pending check. One trap worth naming: clearing block state treats an omitted edge list as "clear the whole run" and an empty list as a no-op. So a partition that comes out empty sends the empty list, never an omission, or clearing a mixed run would wipe the other arm's edges. A test pins it. Nothing constructs this yet.
…re arm Puts the router in the coordinator slot. WaitpointSystem stops building its own Postgres arm and receives one, so the engine decides the topology. Adds waitpointStore to the engine options. Absent, which is the default, means no store arm is constructed and the store path cannot be reached at all: every id classifies legacy and every mint pins legacy, so this changes no behaviour. The store client joins the shutdown sequence so it cannot leak a connection. The gate for this commit is that the existing corpus passes with no test-file diffs. A test that needed changing here would mean the router is not the pass-through it claims to be.
The two standalone types are the first creates that can reach the store. Both engine entry points take the mint kind the caller resolved from the org flag, and default to legacy when it is absent, so every existing caller is unchanged. Tested against both arms: the minted id classifies to the expected system, a repeated idempotency key returns the cached waitpoint either way, and the two directions that matter for rollout are pinned. A legacy mint stays legacy even where a store is configured, which is the reversibility claim. A store mint on a process with no store configured falls back to legacy rather than failing, which keeps one bad configuration from breaking triggers for a flipped org.
…anchor A store RUN waitpoint has no Postgres row, so the trigger path can no longer decide whether to block the parent by looking for one. It now mints the waitpoint's identity before the run is created and keys the block step off that, which keeps the decision independent of where the waitpoint lives. Getting this wrong is quiet rather than loud: gating on the absent relation would skip the block entirely, and triggerAndWait would return without waiting on every store-path trigger. The test asserts the parent reaches SUSPENDED, so a parent that was never blocked fails it. The store waitpoint is created after the run commits, on an id derived from the run, so a retry recomputes the same id and the create is idempotent. If the process dies in that window the parent's register step throws rather than resuming, which a test covers by deleting the record before the register. A run whose own id is legacy shaped keeps a legacy waitpoint even where the flag is on, since the derivation needs a run-ops anchor. The router owns that fallback and counts it: an org with no run-ops runs mints no store waitpoints, and a rollout gate reading health off an empty sample measures nothing.
…uard The batch entry point now carries the mint kind, so a flipped organization's BATCH waitpoint lands in the store. Its id derives from the batch row id, which production already mints as a run-ops id for a run-ops environment. The item-absorb path passes the parent's BATCH waitpoint id down to the lockless register. That id is derived rather than looked up, so it costs nothing, and it gives the store arm the subject its pending-set assertion needs. Without it the guard had nothing to check and skipped itself. Tested on both arms: the parent reaches SUSPENDED rather than staying QUEUED, a duplicate batch answers null through a unique index on one arm and create-if- absent on the other, and the parent stays blocked after each of three items absorb, which is the invariant the guard protects.
Eight call sites now resolve the organization's flag and pass it to the engine: the trigger path, the failed-run path, the three batch sites, the token route, the two stream wait routes, and the duration wait route. Until this commit the flag existed but nothing consulted it, so every mint was legacy. The trigger path resolves once per trigger and passes the org flags the authenticated environment already carries, so the hot path issues no extra query. Routes that already load the environment do the same. Also drops the knip ignore added while the resolver had no consumers. It has consumers now, so the entry would be stale config rather than a real exemption. Deliberately not adding a compile-time assertion that the webapp's mint-kind union matches the engine's. Exporting the engine's type for that purpose degraded module resolution across the webapp, and the check is redundant: every call site passes this value into an engine method, so a drift already fails there, closer to whatever broke.
The related-runs list is built from a Postgres table that only the Postgres block-edge write fills in. A waitpoint whose edges live elsewhere would answer an empty list, and an empty list reads as "no run is blocked on this token", which is a false statement about the token rather than an honest gap. The presenter now reports whether the list is available, and the page says so instead of rendering an empty table. Nothing changes for a token whose edges are in Postgres.
Adds the factory the two-arm parameterization needs, and moves the six waitpoint-subject files onto it. The arm defaults to legacy, so this commit changes no behaviour and every one of these tests still asserts what it did. The factory takes the arm at construction rather than per call, because an engine with no store configured cannot reach the store path at all, which is what an unflipped deployment actually looks like. Two helpers ship with it. freshRunFriendlyId keeps a store-arm test from triggering with a legacy run id, which would mint a legacy waitpoint and assert nothing about the store. assertStoreResident is the same guard stated at the assertion site, for tests that are supposed to mint into the store.
The _completedWaitpoints join has a foreign key to "Waitpoint". A waitpoint held outside Postgres has no row there, so offering its id to the insert violates the constraint. That insert shares the resume's transaction, so the violation took the whole resume down and the run never continued. Those ids are now dropped before the statement runs. Their snapshot link travels with the snapshot entry instead, which is where a non-Postgres waitpoint keeps it, so nothing is lost by leaving them out here. Found by running the waitpoint suite against the store arm, which is the class of defect that parameterization exists to surface: every existing test passed, because none of them had a waitpoint outside Postgres to link. Also consolidates the test-side arm helpers onto the shared factory.
Parameterizes the waitpoint suite over the two arms. Each case now runs twice, once against Postgres and once against the store, with the mint kind and the run id shape following the arm. Two things had to change for the store arm to mean anything. A store RUN waitpoint derives its id from the anchor run, so a literal legacy run id would mint a legacy waitpoint and the case would assert nothing about the store. And assertions that read the waitpoint row directly go through a helper that reads whichever system holds it, since there is no row to read on the store side.
…rrectly A run can hold one waitpoint in Postgres and one in the store at the same time, and neither arm can see the other's. The resume therefore has to read both, or it would release the run while half its blockers are still outstanding. Three cases: completing the Postgres one first, completing the store one first, and confirming the resume clears both arms' edges rather than stranding one. Verified the suite bites. Dropping the store half from the resume read releases the run as soon as the Postgres waitpoint completes, and the legacy-first case fails, which is exactly the defect these tests exist to catch. Also worth recording: an earlier draft of these tests never dequeued or started the run, so it asserted resume behaviour on a run that had no attempt to continue. It failed identically with both waitpoints in Postgres, which is what showed the fault was in the test rather than in mixed mode.
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 Redis arm of the waitpoint coordinator, plus the per-organization flag that decides which arm mints a new waitpoint. Nothing routes to the new arm yet: the flag defaults to unset,
WAITPOINT_SYSTEM_DEFAULTdefaults tolegacy, and every call site still pins the legacy value. Behaviour is unchanged.Stacked on #4779. Draft while the remaining wiring lands, and open early so CI runs the container suites.
Design
The coordinator seam already existed, with one Postgres implementation behind it. This adds a second implementation and the machinery to choose between them at mint time. Every operation after a mint routes by the waitpoint id's own shape, never by re-reading the flag, so a flag flip can only change where the next waitpoint is born. That is also why the flag needs no grace window: there is no interval in which one waitpoint could end up split across the two systems.
Three rules in the new arm carry the correctness weight.
An edge that is in neither the run's pending set nor its delivered set reports
PENDING, and increments a counter. The store keeps every edge in exactly one of the two, so being in neither means the run shard lost state. Reading that as "not pending, therefore complete" would resume a run whose waitpoint never completed. This is deliberately not a rule about completion envelopes: a waitpoint can beCOMPLETEDwhile carrying none, and treating that as unresolved would block a healthy run forever.A lockless batch absorb refuses to write item edges unless the parent's
BATCHwaitpoint is present and still pending. Absorbing items without the run lock is only safe while that waitpoint holds the pending set open; otherwise a concurrent completion can observe an empty pending set mid-absorb and resume the parent early.The
MANUALprojection row is written to Postgres after the store commit, and no coordination path reads it back. A failed projection write is logged and counted rather than thrown, because the waitpoint already exists and is already coordinating.The
BATCHwaitpoint create also moves onto the seam. ItsP2002catch moves with it to the Postgres arm, where it belongs: that catch is a unique-index contract, and against a store that reports duplicates throughSET NXit would read a genuine store error as a duplicate batch. The block step keeps its ownP2002catch, because the previous shape wrapped both the create and the block in onetry, and narrowing that here would be a behaviour change smuggled into an extraction.Verification
Run locally before the machine ran out of container capacity: the new store-arm suite (10 cases), the shape mapper (6), the mint-kind resolver (6), and the existing waitpoint suites that pin the batch contract (46 across
waitpointPublicRouter,batchTriggerAndWait,batchTwoPhase,waitpointSystem) with no test-file diffs. Typecheck clean for both packages.Not yet re-run locally: a mutation check on the two correctness rules above. Leaving that to CI here and repeating it locally before this leaves draft.