feat(webapp,run-engine): Redis waitpoint coordinator arm behind a per-org mint flag - #4793
Draft
d-cs wants to merge 33 commits into
Draft
Conversation
…h coordinator arms
The resume path only has id, status, type and completedAfter per edge, which is nine
fields short of a completion envelope. Add one coordinator method that sources the rest,
implemented by both arms so the record build never branches on residency.
The store arm reads wp:{id} alone: both halves live under that key, so one pipelined
HMGET per id needs no run-scoped key and cannot span two cluster slots. An id with no
record, or a record with no completion, is omitted rather than defaulted.
One record per distinct id. The ordered id list carries multiplicity and holds only batch-indexed ids, so the record set is what says which waitpoints completed. The output variant is chosen, never copied: an offloaded value stays a reference, a plain RUN output becomes a marker re-read from TaskRun.output, a BATCH output is omitted because the runtime discards it at source, and everything else rides inline under the pre-existing thresholds. No new cap and no completion-time spill. A RUN error and an orphaned RUN both stay inline. TaskRun.error is jsonb and does not round-trip, and the completing-run back-reference nulls on delete.
Rebuilds CompletedWaitpoint[] from a wait cycle's ordered id list and records, field-for- field equivalent to the existing snapshot hydration, which is what the executor consumes. It iterates the records, never the order. The order holds only batch-indexed ids, so iterating it would drop every index-less wait: each wait.for, each single triggerAndWait and each token. The equivalence suite pins that, and fails on 10 of 12 cases if the iteration is inverted. The coverage check is the fail-loud rule. The id classifier is total and never throws, so an unrecognised shape would otherwise classify as legacy, find no row, and vanish from the resumed run's completed set. An id that no half resolves throws, and so does an id that both halves claim.
…at the resume appends Carries an envelope per distinct id from the resume path into the wait cycle's key, filling the hole the snapshot store left for this lane. The records ride the mint only: a copy-forward writes no key and needs none. continueRunIfUnblocked builds the set once and passes it at both appends. The build is gated on id shape, so a wait with no store-resident half supplies no records and a Postgres-resident resume is byte-identical to before. Nothing mints a store-format waitpoint yet, so every live path supplies none today. The existing waitpoint corpus passes unmodified.
The base branch gained a refusal path: when the store declines an untrustworthy cycle pointer it mints a replacement inside the same call, from the refs the caller carried. That replacement needs the records too. A cycle holding ids with no records makes the resolver's coverage check reject a legitimate resume, because every distinct id must resolve through exactly one half. Also pins the no-refs case, where writing no pointer at all stays correct.
… envelope Coverage check now runs over the whole membership, not the order. The order omits every index-less wait by construction, so an order-scoped check could not see an index-less id whose record was missing — the exact loss the resolver exists to prevent. Adds distinctIds to the resolver args and updates the jointly-owned freeze pin. A refused copy-forward no longer mints a records-less cycle. Copy-forward appends carry no records of their own, and the append script can refuse a pointer and mint a replacement from the carried refs, so the decorator reads the surviving cycle's records and carries those. A deriveFromRun record whose run output is gone now fails loud instead of resolving to an empty output. Postgres does not lose it: the back-reference nulls on delete but the stored output stays, so returning undefined would resolve a triggerAndWait with silently wrong data. The legacy arm passes the routing hint it was dropping, so a resume reads the run's own store instead of fanning out across every run-ops database, and reuses the chunked fetch rather than reading a large fan-in whole. The envelope read issues one command per id concurrently rather than as a pipeline. Each id is its own hash tag, so N ids are N cluster slots and a pipeline spanning them is rejected under cluster mode — which a single-node test server would never surface. Also: shares one row-to-source mapper between the legacy arm and the equivalence suite, so a bug in the arm can no longer hide from the oracle; pins the deliberate BATCH-output divergence and corrects the comment that gave the wrong reason for it; gates the record build on id format rather than claiming residency; and builds the record set inside the two branches that append rather than before the statuses that return without appending.
|
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: Team 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: 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.) Full details: Description checkExplanation The description explains the implementation, routing design, correctness safeguards, regression risk, and testing strategy. It omits some template sections, such as the issue closure, checklist, changelog, and screenshots, but it provides sufficient technical and testing detail. ✨ 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.
The store arm cannot return a pending waitpoint, because a pending one has no completion to read. The legacy arm read rows by id with no status filter, so it could hand back an envelope for a PENDING waitpoint with completedAt defaulted to now. The resolver's coverage check reads an omission as "fail loud", so the arms disagreeing there would turn a pending waitpoint into a resumable one. Filters to COMPLETED. Also states why the ref branch precedes the RUN branch, which is the opposite order to the reference implementation in the freeze test. Both are byte-identical at read time by that reference's own reasoning, and this order needs no Postgres read to recover a string already in hand — and keeps an offloaded RUN success resolvable when the completing run row is gone, which now refuses rather than resolving empty. Adds the offloaded-RUN-success case that both suites were missing.
@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
Replaces the hand-written run-output callbacks with a real Postgres read. The branch's premise is that TaskRun.output holds the same string the waitpoint carried, and only a real row can settle that — a callback returning a literal asserted that the callback was called. Adds createRunOutputReader, the production reader over the store, so the read routes to the run's owning database. The dependency is now optional, because most cycles carry no deriveFromRun record; one that does with no reader wired throws, since that is a wiring error rather than a data condition. The equivalence suite runs against seeded child runs whose output matches each RUN row, so the parity claim is now checked end to end rather than against a value the test supplied twice. The pure suite keeps every case that performs no read and is built with no reader at all. One wrapper remains, and delegates to the real reader: it counts reads to pin one query per record rather than one per batch index, which the resolved output cannot show.
…-tri-13441' into feat/waitpoint-mint-flag-wiring-tri-13442
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 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.
Each case in the waitpoint suite now runs twice, once against Postgres and once against the store, with the mint kind and the run id shape following the arm. Three things had to change for the store arm to assert anything real. A store RUN waitpoint derives its id from its anchor run, so a literal legacy run id would mint a legacy waitpoint and the case would prove nothing. Waitpoint reads go through a helper that reads whichever system holds the waitpoint, since there is no row for the store to read. And block-edge reads union both systems rather than switching, because a run can hold one edge in each at once and a test seeing half of them would report the wrong count.
…reated it A repeated idempotency key returns the waitpoint the first call created, and the projection write ran on that path too, so it tried to insert a row that already existed and failed the primary key. The waitpoint itself was fine, but every cached hit logged an error and counted a projection failure. Only the creating call writes now. Also marks the two assertions that cannot hold on the store arm yet. Executor visible completed waitpoints are hydrated from the snapshot entry's record set for a store-resident waitpoint, and the hook that reads it back belongs to the snapshot lane. The condition is written into the test with the reason, rather than the case being skipped, so the rest of it still runs on both arms.
…coordinators Extends the two-arm parameterization to the RUN and BATCH waitpoint suites, so triggerAndWait and batchTriggerAndWait both exercise the store path end to end rather than only the Postgres one. Same three adjustments as the waitpoint suite: run ids take a shape the anchor derive can work from, waitpoint reads go through the arm-aware helper, and block edge reads union both systems.
The helper dropped batchId, so assertions comparing an edge's batch read undefined and failed on both arms. Both arms carry it now.
d-cs
force-pushed
the
feat/waitpoint-envelope-resolver-tri-13441
branch
from
September 2, 2026 09:24
5b4b060 to
9e8dc9d
Compare
…aitpoint-mint-flag-wiring-tri-13442 The base picked up the gen-2 shard work, which overlaps this branch in six files. Resolutions worth knowing about: Waitpoint ids for BATCH now come from the shard-aware mint rather than a bare generate. That mint moved with the create when it went onto the coordinator seam, so the legacy arm carries it and the catalog entry moves with it. anchorRunId is required on the seam now, which is the base's stricter contract. The router's derive check no longer has an absent case to handle. The standalone shard hint and the waitpoint mint kind are both parameters on the two standalone create paths; neither replaces the other. The trigger path keeps this branch's shape, where the decision to block the parent comes from a ref minted before the run is created rather than from the Prisma relation. Base still read the relation, which the store path never writes. Run-id mint resolution moved into the base's own helper, so two imports here were genuinely dead rather than dropped by the merge.
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 and the per-organization flag that decides where a new waitpoint is minted. With the flag unset, which is the default, no store arm is constructed at all and every waitpoint behaves exactly as before.
Stacked on #4779.
Design
A router now sits in the coordinator slot and decides which system owns a waitpoint. It applies two deliberately different rules. 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. A create routes on the caller's mint kind, and a store mint with no store configured falls back with a logged error, because there is no id yet to misroute and refusing would turn one badly configured process into a trigger outage for every flipped organization.
A run can hold one waitpoint in each system at once, and neither arm can see the other's. So the reads fan out to both and the pending counts sum. Tests cover both completion orders.
Three rules in the store arm carry the correctness weight:
An edge in neither the run's pending nor its delivered set reports
PENDINGand increments a counter. The store keeps every edge in exactly one of the two, so being in neither means the run shard lost state, and reading that as "complete" would resume a run whose waitpoint never completed. This is deliberately not a rule about completion envelopes: a waitpoint can beCOMPLETEDcarrying none, and treating that as unresolved would block a healthy run forever.The trigger path mints the RUN waitpoint's identity before the run is created and keys the parent-block step off that, never off the Prisma relation. Gating on the relation would skip the block entirely on the store path, and
triggerAndWaitwould return without waiting. The waitpoint itself is created after the run commits, on an id derived from the run, so a retry recomputes the same id. If the process dies in that window the parent's register throws rather than resuming.A lockless batch absorb refuses to write item edges unless the parent's
BATCHwaitpoint is present and still pending. Note the scope honestly: this is a preflight detector, not a barrier. It reads the run shard, then the absorb writes separately, so a completion landing between the two is detected on the next call rather than prevented. Closing that window means moving the assertion inside the absorb script, which is called out in an open review thread.A bug this found
_completedWaitpointshas a foreign key toWaitpoint, and the resume was offering it ids for waitpoints held outside Postgres. That insert shares the resume's transaction, so the violation took the resume down and the run never continued. Every existing test passed, because none of them had a waitpoint outside Postgres to link. Fixed with a regression test.Testing
Two-arm coverage for all four create types, mixed mode in both directions, the router, and the store arm. The correctness rules are checked by mutation rather than assumed: inverting each one fails its test and nothing else.
Two test guards ship with the factory, because the most plausible failure here is a test that runs green on both arms while the store arm quietly did nothing. One refuses a store-arm run triggered with a legacy id, which would mint a legacy waitpoint and assert nothing. The other asserts a waitpoint really is store resident where one is expected.
The pre-existing waitpoint suite is parameterized too:
waitpoints,triggerAndWaitandbatchTriggerAndWaiteach run every case twice. Three files stay on Postgres only and say why in place, because they pin residency machinery or a Postgres row that the store path does not have.Getting there needed three things in the shared factory. Run ids take a shape the anchor derive can work from, or a store-arm case mints a legacy waitpoint and proves nothing while still passing. Waitpoint reads go through a helper that reads whichever system holds it. Block-edge reads union both systems rather than switching, because a run can hold one edge in each at once.