feat(run-engine,run-store): completed-waitpoint envelope, read-time resolver, and the fail-loud coverage check - #4779
feat(run-engine,run-store): completed-waitpoint envelope, read-time resolver, and the fail-loud coverage check#4779d-cs wants to merge 9 commits into
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe run engine now reads completed waitpoint envelopes from Postgres or Redis and converts them into deduplicated completed-waitpoint records. Resolver logic reconstructs executor-compatible waitpoints, including outputs, metadata, indexes, and validation errors. Resume and enqueue paths forward these records into execution snapshots. The run store accepts the expanded snapshot inputs, and the Redis snapshot decorator persists records across staged, carried-forward, and replacement cycles. Tests cover equivalence, output handling, validation, Redis reads, and snapshot persistence. Merge Risk: 🟡 Moderate · up to This PR adds completed-waitpoint persistence and read-time reconstruction, but the current head is not merge-ready because it depends on 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on-topic, and documents the implementation, testing, stacking dependency, known limitations, follow-up work, and verification results. It omits the template checklist, explicit changelog, screenshots section, and issue-closing line, but it provides the critical review information.
✨ 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 |
…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.
7154c3c to
7f9da73
Compare
@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: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)
156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion cannot fail; express it against the crash count.
Line 146 already asserts
pgRun.attemptNumberis exactly 1, so1 - 1 <= 1always holds. The comment says the line proves the per-crash bound, but it proves nothing. The last test in this file states the same property correctly againstfaults.fired(...).♻️ Proposed change
- // The bound: one crash costs at most one attempt number. - expect(pgRun.attemptNumber! - 1).toBeLessThanOrEqual(1); + // The bound: pgAttempt - maxLoggedAttempt <= crashCount. + expect((pgRun.attemptNumber ?? 0) - 1).toBeLessThanOrEqual( + faults.fired("afterPgBeforeRedis") + );
383-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name claims two crashes, but the assertion accepts one.
expect(crashes).toBeGreaterThanOrEqual(1)passes when only one fault fires. The two-crash scenario named in the title and in the comment on Line 390 is then never exercised, and the bound assertion on Line 391 degrades to the single-crash case. This is the same silent-pass failure mode thefired()guards elsewhere in this file exist to prevent.Assert the exact expected count.
♻️ Proposed change
const crashes = faults.fired("afterPgBeforeRedis"); - expect(crashes).toBeGreaterThanOrEqual(1); + expect(crashes).toBe(2);
197-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree dequeue sites index
dequeued[0]!without the length assertion this file uses elsewhere. The first and third tests assertexpect(dequeued.length).toBe(1)before indexing. The other three do not, so an empty queue produces aTypeErrorinstead of the intended assertion failure.
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L197-L204: addexpect(dequeued.length).toBe(1)after the dequeue call in theafterRedisBirthBeforePgtest.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L315-L320: add the same assertion after the dequeue call in the stale-snapshot test.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L364-L371: add the same assertion after the dequeue call in the two-crash test.internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)
232-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail if the birth omits the completion TTL.
The comment states the invariant: a born-terminal run never transitions again, so the birth itself must apply the completion TTL. The assertions only check that both keyspaces are readable. They pass whether or not any expiry was set, and the non-terminal run exists only as an unused comparison.
Assert the expiry directly with a raw Redis client, as
taskRunExecutionSnapshotStore.waitpointCycles.test.tsdoes for cycle keys: expect a positivepttlon the terminal run's key and-1on the non-terminal run's key.internal-packages/run-store/src/redisSnapshotStore.ts (1)
459-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared row-decode and head-resolution block.
getSinceandgetSinceCreatedAtnow carry the same loop, the sameheadSurvivedtracking, the samerows.reverse(), and the same head attribution. The offsets (i = 3, stride 4) and the reply layout must stay identical in both, so a future change to the Lua reply shape must be applied twice. Extract one private helper that takes the reply and returns{ entries, headWaitpointIds }.♻️ Sketch
`#decodeSinceReply`( reply: string[], environmentId: string | undefined, runId: string ): { entries: SnapshotRead[]; headWaitpointIds: WaitpointIds } { const headOrder = reply[1] ?? ""; const headDistinct = reply[2] ?? ""; const rows: SnapshotRead[] = []; let headSurvived = false; for (let i = 3; i + 3 < reply.length; i += 4) { const decoded = this.#decode( [reply[i], reply[i + 1], reply[i + 2], reply[i + 3], ""], environmentId, runId, false ); if (decoded) { rows.push(decoded); if (i === 3) headSurvived = true; } } rows.reverse(); const head = headSurvived ? rows[rows.length - 1] : undefined; const headWaitpointIds = decodeWaitpointIds( head !== undefined, head ? headOrder : "", head ? headDistinct : "" ); if (head) { head.completedWaitpointIds = headWaitpointIds; if (head.cycle) { this.#checkCycleMismatch(runId, head.cycle.count, headWaitpointIds.order.length); } } return { entries: rows, headWaitpointIds }; }Also applies to: 503-557
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2802deac-da94-48dd-8065-ab7bc7c0da5c
📒 Files selected for processing (46)
internal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/types.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 (9)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/snapshotEntry.test.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-store/src/snapshotEntry.test.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/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-store/src/snapshotEntry.test.ts
Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
📄 CodeRabbit inference engine (internal-packages/run-engine/CLAUDE.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.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/executionSnapshotSystem.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/snapshotEntry.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-store/src/snapshotEntry.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-store/src/snapshotEntry.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-store/src/snapshotEntry.test.ts
🧠 Learnings (5)
📚 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:
internal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/types.tsinternal-packages/run-store/src/delegatingRunStore.ts
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
internal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts
📚 Learning: 2026-08-15T17:58:37.120Z
Learnt from: 1stvamp
Repo: triggerdotdev/trigger.dev PR: 4628
File: internal-packages/run-engine/src/run-queue/tests/ckWildcardKey.test.ts:0-0
Timestamp: 2026-08-15T17:58:37.120Z
Learning: In internal-packages/run-engine test files, use Vitest's established global test API when Vitest globals are enabled. Do not import describe from node:test, because it shadows Vitest's global describe and registers test blocks with Node's test runner; remove the node:test import rather than replacing it with a Vitest import.
Applied to files:
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
📚 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:
internal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.ts
🪛 ast-grep (0.45.2)
internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts
[warning] 168-168: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(return this\\.delegate\\.${name}\\(([^;]*)\\);)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.26.0)
internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts
[ERROR] 117-117: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 139-139: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 169-169: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (41)
internal-packages/run-store/src/snapshotOrphanSweeper.ts (8)
25-58: LGTM!
92-111: LGTM!
116-144: LGTM!
184-207: LGTM!
210-229: LGTM!
247-266: LGTM!
272-338: LGTM!
149-163: 🗄️ Data Integrity & IntegrationNo change needed.
RunStore.findRunsByIdsreturns an ID-keyedMap, and its implementation keys entries by the internalid.rows.get(runId)uses the correct key.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)
32-58: LGTM!
60-104: LGTM!
226-279: LGTM!internal-packages/run-store/src/delegatingRunStore.ts (1)
51-750: LGTM!internal-packages/run-store/src/delegatingRunStore.test.ts (1)
76-88: 🎯 Functional CorrectnessNo change needed.
runStoreMethodNames.tschecks both directions againstkeyof RunStore, so missing or extra member names cause a TypeScript error.internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1)
13-13: LGTM!Also applies to: 452-452, 474-474, 497-497
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.ts (1)
12-26: LGTM!Also applies to: 49-124, 126-149
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.ts (1)
9-28: LGTM!Also applies to: 30-80, 82-178, 180-259, 264-332
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts (1)
20-123: LGTM!Also applies to: 125-330
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.ts (1)
8-20: LGTM!internal-packages/run-store/src/PostgresRunStore.snapshotId.test.ts (1)
10-149: LGTM!internal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.ts (1)
16-96: LGTM!Also applies to: 98-202, 204-235, 237-311
internal-packages/run-store/src/snapshotOrphanSweeper.test.ts (1)
21-34: LGTM!Also applies to: 37-117, 119-194, 196-272, 274-352, 354-396, 398-429
internal-packages/run-store/src/testFixtures/snapshotIdFixture.ts (1)
8-77: LGTM!Also applies to: 79-149, 151-167
internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.ts (1)
25-109: LGTM!Also applies to: 111-143, 147-161, 165-184, 186-218
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts (1)
1839-1975: LGTM!internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts (1)
509-563: LGTM!Also applies to: 596-629
internal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.ts (1)
113-132: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.ts (1)
79-420: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts (1)
96-544: LGTM!internal-packages/run-engine/src/engine/tests/helpers/decoratedStore.ts (1)
31-75: LGTM!internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts (1)
104-149: 🗄️ Data Integrity & IntegrationDo not add the
COMPLETEDpredicate.
WaitpointSystemcalls this method only afterreadRunBlockStatereports all blockers asCOMPLETED. Both reads pass the writer client, which routes to the owning primary, so a pending row cannot reach this mapping through the caller path.internal-packages/run-store/src/redisSnapshotStore.ts (1)
32-44: LGTM!Also applies to: 286-319, 426-426, 581-601, 617-623, 643-645, 680-680, 750-750, 762-762, 775-837, 867-900, 923-923, 949-957
internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
118-138: LGTM!Also applies to: 157-221, 227-257, 263-425, 447-566, 584-627, 636-651, 666-795, 804-866, 872-911, 913-962
internal-packages/run-store/src/snapshotFaultInjection.ts (1)
9-45: LGTM!internal-packages/run-store/src/index.ts (1)
6-10: LGTM!internal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.ts (1)
11-41: LGTM!Also applies to: 43-193
internal-packages/run-store/src/snapshotReadShapes.ts (1)
13-27: LGTM!Also applies to: 29-51, 53-95
internal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.ts (1)
10-38: LGTM!Also applies to: 40-100
internal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.ts (2)
20-65: LGTM!Also applies to: 67-251
253-285: 📐 Maintainability & Code QualityNo change needed.
PostgresRunStore.forWaitpointCompletionignores the waitpoint ID and context and returnsthis, so an unknown waitpoint ID does not fail before the assertions.internal-packages/run-store/src/snapshotReadShapes.test.ts (1)
7-17: LGTM!Also applies to: 19-151
internal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.ts (1)
26-134: LGTM!Also applies to: 136-454
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (5)
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)
156-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion cannot fail; express it against the crash count.
Line 146 already asserts
pgRun.attemptNumberis exactly 1, so1 - 1 <= 1always holds. The comment says the line proves the per-crash bound, but it proves nothing. The last test in this file states the same property correctly againstfaults.fired(...).♻️ Proposed change
- // The bound: one crash costs at most one attempt number. - expect(pgRun.attemptNumber! - 1).toBeLessThanOrEqual(1); + // The bound: pgAttempt - maxLoggedAttempt <= crashCount. + expect((pgRun.attemptNumber ?? 0) - 1).toBeLessThanOrEqual( + faults.fired("afterPgBeforeRedis") + );
383-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name claims two crashes, but the assertion accepts one.
expect(crashes).toBeGreaterThanOrEqual(1)passes when only one fault fires. The two-crash scenario named in the title and in the comment on Line 390 is then never exercised, and the bound assertion on Line 391 degrades to the single-crash case. This is the same silent-pass failure mode thefired()guards elsewhere in this file exist to prevent.Assert the exact expected count.
♻️ Proposed change
const crashes = faults.fired("afterPgBeforeRedis"); - expect(crashes).toBeGreaterThanOrEqual(1); + expect(crashes).toBe(2);
197-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree dequeue sites index
dequeued[0]!without the length assertion this file uses elsewhere. The first and third tests assertexpect(dequeued.length).toBe(1)before indexing. The other three do not, so an empty queue produces aTypeErrorinstead of the intended assertion failure.
internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L197-L204: addexpect(dequeued.length).toBe(1)after the dequeue call in theafterRedisBirthBeforePgtest.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L315-L320: add the same assertion after the dequeue call in the stale-snapshot test.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts#L364-L371: add the same assertion after the dequeue call in the two-crash test.internal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.ts (1)
232-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test cannot fail if the birth omits the completion TTL.
The comment states the invariant: a born-terminal run never transitions again, so the birth itself must apply the completion TTL. The assertions only check that both keyspaces are readable. They pass whether or not any expiry was set, and the non-terminal run exists only as an unused comparison.
Assert the expiry directly with a raw Redis client, as
taskRunExecutionSnapshotStore.waitpointCycles.test.tsdoes for cycle keys: expect a positivepttlon the terminal run's key and-1on the non-terminal run's key.internal-packages/run-store/src/redisSnapshotStore.ts (1)
459-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared row-decode and head-resolution block.
getSinceandgetSinceCreatedAtnow carry the same loop, the sameheadSurvivedtracking, the samerows.reverse(), and the same head attribution. The offsets (i = 3, stride 4) and the reply layout must stay identical in both, so a future change to the Lua reply shape must be applied twice. Extract one private helper that takes the reply and returns{ entries, headWaitpointIds }.♻️ Sketch
`#decodeSinceReply`( reply: string[], environmentId: string | undefined, runId: string ): { entries: SnapshotRead[]; headWaitpointIds: WaitpointIds } { const headOrder = reply[1] ?? ""; const headDistinct = reply[2] ?? ""; const rows: SnapshotRead[] = []; let headSurvived = false; for (let i = 3; i + 3 < reply.length; i += 4) { const decoded = this.#decode( [reply[i], reply[i + 1], reply[i + 2], reply[i + 3], ""], environmentId, runId, false ); if (decoded) { rows.push(decoded); if (i === 3) headSurvived = true; } } rows.reverse(); const head = headSurvived ? rows[rows.length - 1] : undefined; const headWaitpointIds = decodeWaitpointIds( head !== undefined, head ? headOrder : "", head ? headDistinct : "" ); if (head) { head.completedWaitpointIds = headWaitpointIds; if (head.cycle) { this.#checkCycleMismatch(runId, head.cycle.count, headWaitpointIds.order.length); } } return { entries: rows, headWaitpointIds }; }Also applies to: 503-557
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2802deac-da94-48dd-8065-ab7bc7c0da5c
📒 Files selected for processing (46)
internal-packages/run-engine/src/engine/systems/enqueueSystem.tsinternal-packages/run-engine/src/engine/systems/executionSnapshotSystem.tsinternal-packages/run-engine/src/engine/systems/waitpointSystem.tsinternal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.tsinternal-packages/run-engine/src/engine/tests/helpers/decoratedStore.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.tsinternal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/types.tsinternal-packages/run-store/src/PostgresRunStore.snapshotId.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.tsinternal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.tsinternal-packages/run-store/src/PostgresRunStore.tsinternal-packages/run-store/src/delegatingRunStore.forwarding.test.tsinternal-packages/run-store/src/delegatingRunStore.test.tsinternal-packages/run-store/src/delegatingRunStore.tsinternal-packages/run-store/src/index.tsinternal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.tsinternal-packages/run-store/src/redisSnapshotStore.tsinternal-packages/run-store/src/runStoreMethodNames.tsinternal-packages/run-store/src/snapshotEntry.parity.test.tsinternal-packages/run-store/src/snapshotEntry.test.tsinternal-packages/run-store/src/snapshotEntry.tsinternal-packages/run-store/src/snapshotFaultInjection.tsinternal-packages/run-store/src/snapshotOrphanSweeper.test.tsinternal-packages/run-store/src/snapshotOrphanSweeper.tsinternal-packages/run-store/src/snapshotReadShapes.test.tsinternal-packages/run-store/src/snapshotReadShapes.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.births.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.tsinternal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.tsinternal-packages/run-store/src/testFixtures/snapshotIdFixture.tsinternal-packages/run-store/src/types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
🔇 Additional comments (41)
internal-packages/run-store/src/snapshotOrphanSweeper.ts (8)
25-58: LGTM!
92-111: LGTM!
116-144: LGTM!
184-207: LGTM!
210-229: LGTM!
247-266: LGTM!
272-338: LGTM!
149-163: 🗄️ Data Integrity & IntegrationNo change needed.
RunStore.findRunsByIdsreturns an ID-keyedMap, and its implementation keys entries by the internalid.rows.get(runId)uses the correct key.internal-packages/run-engine/src/engine/tests/snapshotStoreChaos.test.ts (3)
32-58: LGTM!
60-104: LGTM!
226-279: LGTM!internal-packages/run-store/src/delegatingRunStore.ts (1)
51-750: LGTM!internal-packages/run-store/src/delegatingRunStore.test.ts (1)
76-88: 🎯 Functional CorrectnessNo change needed.
runStoreMethodNames.tschecks both directions againstkeyof RunStore, so missing or extra member names cause a TypeScript error.internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts (1)
13-13: LGTM!Also applies to: 452-452, 474-474, 497-497
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.ts (1)
12-26: LGTM!Also applies to: 49-124, 126-149
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.ts (1)
9-28: LGTM!Also applies to: 30-80, 82-178, 180-259, 264-332
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts (1)
20-123: LGTM!Also applies to: 125-330
internal-packages/run-engine/src/engine/tests/finalRunStatusParity.test.ts (1)
8-20: LGTM!internal-packages/run-store/src/PostgresRunStore.snapshotId.test.ts (1)
10-149: LGTM!internal-packages/run-store/src/PostgresRunStore.snapshotWrites.test.ts (1)
16-96: LGTM!Also applies to: 98-202, 204-235, 237-311
internal-packages/run-store/src/snapshotOrphanSweeper.test.ts (1)
21-34: LGTM!Also applies to: 37-117, 119-194, 196-272, 274-352, 354-396, 398-429
internal-packages/run-store/src/testFixtures/snapshotIdFixture.ts (1)
8-77: LGTM!Also applies to: 79-149, 151-167
internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointRecords.test.ts (1)
25-109: LGTM!Also applies to: 111-143, 147-161, 165-184, 186-218
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts (1)
1839-1975: LGTM!internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts (1)
509-563: LGTM!Also applies to: 596-629
internal-packages/run-store/src/PostgresRunStore.snapshotTimestamps.test.ts (1)
113-132: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.reads.test.ts (1)
79-420: LGTM!internal-packages/run-store/src/taskRunExecutionSnapshotStore.waitpointCycles.test.ts (1)
96-544: LGTM!internal-packages/run-engine/src/engine/tests/helpers/decoratedStore.ts (1)
31-75: LGTM!internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts (1)
104-149: 🗄️ Data Integrity & IntegrationDo not add the
COMPLETEDpredicate.
WaitpointSystemcalls this method only afterreadRunBlockStatereports all blockers asCOMPLETED. Both reads pass the writer client, which routes to the owning primary, so a pending row cannot reach this mapping through the caller path.internal-packages/run-store/src/redisSnapshotStore.ts (1)
32-44: LGTM!Also applies to: 286-319, 426-426, 581-601, 617-623, 643-645, 680-680, 750-750, 762-762, 775-837, 867-900, 923-923, 949-957
internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
118-138: LGTM!Also applies to: 157-221, 227-257, 263-425, 447-566, 584-627, 636-651, 666-795, 804-866, 872-911, 913-962
internal-packages/run-store/src/snapshotFaultInjection.ts (1)
9-45: LGTM!internal-packages/run-store/src/index.ts (1)
6-10: LGTM!internal-packages/run-store/src/redisSnapshotStore.sinceCreatedAt.test.ts (1)
11-41: LGTM!Also applies to: 43-193
internal-packages/run-store/src/snapshotReadShapes.ts (1)
13-27: LGTM!Also applies to: 29-51, 53-95
internal-packages/run-store/src/taskRunExecutionSnapshotStore.off.test.ts (1)
10-38: LGTM!Also applies to: 40-100
internal-packages/run-store/src/taskRunExecutionSnapshotStore.staging.test.ts (2)
20-65: LGTM!Also applies to: 67-251
253-285: 📐 Maintainability & Code QualityNo change needed.
PostgresRunStore.forWaitpointCompletionignores the waitpoint ID and context and returnsthis, so an unknown waitpoint ID does not fail before the assertions.internal-packages/run-store/src/snapshotReadShapes.test.ts (1)
7-17: LGTM!Also applies to: 19-151
internal-packages/run-store/src/taskRunExecutionSnapshotStore.transitions.test.ts (1)
26-134: LGTM!Also applies to: 136-454
🛑 Comments failed to post (8)
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts (3)
490-497: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add OTEL instrumentation for the completed-waitpoint propagation flow.
The new RunEngine flow reads completion envelopes and creates a snapshot without tracer or meter instrumentation. Add a trace span and bounded outcome metrics. Use only bounded attributes such as coordinator arm, snapshot status, and success or failure. Do not add run IDs, waitpoint IDs, or record counts as metric attributes.
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L490-L497: instrument envelope collection and record-build outcomes.internal-packages/run-engine/src/engine/systems/enqueueSystem.ts#L96-L117: instrument the completed-waitpoint snapshot handoff.As per coding guidelines, “Integrate OpenTelemetry tracer and meter instrumentation in RunEngine systems for observability.”
📍 Affects 2 files
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L490-L497(this comment)internal-packages/run-engine/src/engine/systems/enqueueSystem.ts#L96-L117Source: Coding guidelines
744-773: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add approved crumbs to the new snapshot propagation flow.
The new completion-envelope, record-building, and Redis-entry paths contain no crumb markers. Add
//@Crumbsmarkers or `#region `@crumbsblocks with an approved namespace. Request a namespace before adding one because the provided guideline table is unavailable.
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L744-L773: mark envelope classification and coordinator reads.internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts#L12-L60: mark record and output-variant selection.internal-packages/run-store/src/snapshotEntry.ts#L135-L159: mark execution-snapshot entry construction.As per coding guidelines, “Add crumbs as you write code” and “Do not invent new namespaces.”
📍 Affects 3 files
internal-packages/run-engine/src/engine/systems/waitpointSystem.ts#L744-L773(this comment)internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts#L12-L60internal-packages/run-store/src/snapshotEntry.ts#L135-L159Source: Coding guidelines
767-772: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- applicable repository conventions ---' find /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019 -type f -name '*.md' -print printf '%s\n' '--- waitpointSystem structure and target ---' ast-grep outline internal-packages/run-engine/src/engine/systems/waitpointSystem.ts sed -n '1,90p;700,785p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts printf '%s\n' '--- coordinator definitions and relevant methods ---' rg -n --glob '*.{ts,tsx}' 'class (LegacyPostgresWaitpointCoordinator|StoreCoordinator)|readCompletionEnvelopes|LegacyPostgresWaitpointCoordinator' internal-packagesRepository: triggerdotdev/trigger.dev
Length of output: 15028
🏁 Script executed:
printf '%s\n' '--- applicable conventions and learnings ---' cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/conventions/internal-packages-run-engine-src-engine-systems.md cat /tmp/coderabbit-repo-knowledge/triggerdotdev-trigger-dev-0bdd0019/learnings/internal-packages-run-engine-src-engine-systems.md printf '%s\n' '--- coordinator contract and implementations ---' cat -n internal-packages/run-engine/src/engine/waitpointCoordinator/types.ts sed -n '1,145p;490,585p' internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts sed -n '1,90p;495,575p' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts printf '%s\n' '--- callers of the private read path and waitpoint ID parsing ---' rg -n -A18 -B18 '`#completedWaitpointRecordsFor`|completedWaitpointRecordsFor|parseWaitpointId|b32hexW' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts internal-packages/run-engine/src/engine/waitpointCoordinator internal-packages packagesRepository: triggerdotdev/trigger.dev
Length of output: 50381
🏁 Script executed:
printf '%s\n' '--- completion record builder and resume consumption ---' cat -n internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts sed -n '450,620p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts printf '%s\n' '--- StoreWaitpointCoordinator construction and usage ---' rg -n -A12 -B8 'new Store|StoreWaitpoint|StoreCoordinator|storeCoordinator|WaitpointCoordinator' internal-packages/run-engine/src/engine --glob '*.ts' printf '%s\n' '--- exact legacy envelope body ---' sed -n '90,165p' internal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsRepository: triggerdotdev/trigger.dev
Length of output: 38971
🏁 Script executed:
printf '%s\n' '--- all uses of completedWaitpointRecords in the resume path ---' rg -n -A12 -B12 'completedWaitpointRecords' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts printf '%s\n' '--- store record read contract and coordinator capabilities ---' rg -n -A20 -B10 'readCompletionEnvelopes|readRunBlockState|complete\\(|implements WaitpointCoordinator|export class WaitpointStoreCoordinator' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts internal-packages/run-engine/src/engine/waitpointCoordinator/*.test.ts printf '%s\n' '--- resources relevant to Redis/store routing ---' rg -n -A8 -B8 'redisOptions|waitpoint.*Store|WaitpointStoreCoordinator|SystemResources' internal-packages/run-engine/src/engine/systems internal-packages/run-engine/src/engine --glob '*.ts'Repository: triggerdotdev/trigger.dev
Length of output: 50382
🏁 Script executed:
sed -n '560,710p' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts printf '%s\n' '--- targeted record propagation ---' rg -n -A10 -B10 'completedWaitpointRecords' internal-packages/run-engine/src/engine/systems/waitpointSystem.ts printf '%s\n' '--- store coordinator interface declaration ---' sed -n '145,205p' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts rg -n 'readCompletionEnvelopes' internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsRepository: triggerdotdev/trigger.dev
Length of output: 12481
🏁 Script executed:
printf '%s\n' '--- completed waitpoint record contract and resolver ---' rg -l 'CompletedWaitpointRecord|completedWaitpointRecords' internal-packages packages --glob '*.{ts,tsx}' | head -80 printf '%s\n' '--- relevant declarations and consumers ---' rg -n -A18 -B12 'completedWaitpointRecords|CompletedWaitpointRecord' internal-packages/run-store packages internal-packages/run-engine/src --glob '*.{ts,tsx}' --glob '!**/*.test.ts' --glob '!**/*.bench.test.ts' | head -500Repository: triggerdotdev/trigger.dev
Length of output: 50381
🏁 Script executed:
sed -n '1,130p' internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.ts sed -n '584,670p' internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts rg -n -A18 -B12 'resolveCompletedWaitpoints|CompletedWaitpointResolver|completedWaitpointRecords' internal-packages/run-store/src internal-packages/run-engine/src --glob '!**/*.test.ts' --glob '!**/*.bench.test.ts'Repository: triggerdotdev/trigger.dev
Length of output: 50381
Read store-format envelopes with
WaitpointStoreCoordinator.When a completed waitpoint has format
b32hexW,waitpointSystem.ts:767uses the fixedLegacyPostgresWaitpointCoordinator. A Redis-only waitpoint is omitted, and the resolver can throwUnresolvableWaitpointIdwith reasonno-source. Keep legacy operations on Postgres, but route this envelope read toWaitpointStoreCoordinator. Add an integration test for a Redis-onlyb32hexWwaitpoint.internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts (1)
273-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash # Description: Resolve the effective tsconfig strictness for the run-engine package. set -euo pipefail fd -t f 'tsconfig*.json' internal-packages/run-engine --exec cat {} \; fd -t f 'tsconfig*.json' --max-depth 1 --exec cat {} \;Repository: triggerdotdev/trigger.dev
Length of output: 391
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- target test excerpt ---' sed -n '250,305p' internal-packages/run-engine/src/engine/tests/snapshotStoreReadGate.test.ts printf '%s\n' '--- package tsconfig files ---' fd -t f 'tsconfig*.json' internal-packages/run-engine -x sh -c 'echo "--- $1"; cat "$1"' sh {}Repository: triggerdotdev/trigger.dev
Length of output: 2395
🏁 Script executed:
#!/bin/bash set -euo pipefail for file in \ internal-packages/run-engine/tsconfig.src.json \ internal-packages/run-engine/tsconfig.test.json \ tsconfig.json do if [ -f "$file" ]; then echo "--- $file" cat "$file" else echo "--- $file (missing)" fi done echo '--- tracked TypeScript configuration paths near run-engine ---' git ls-files '*tsconfig*.json' | grep -E '(^|/)run-engine/|(^|/)tsconfig\.json$' || trueRepository: triggerdotdev/trigger.dev
Length of output: 3848
🌐 Web query:
TypeScript TS2454 variable used before being assigned assignment inside try finally definite assignment💡 Result:
The TypeScript error TS2454, "Variable '...' is used before being assigned," occurs within
try-finallyblocks because TypeScript’s control flow analysis (CFA) must account for the possibility that code inside thetryblock could throw an exception at any point [1][2][3]. Consequently, the compiler cannot guarantee that an assignment statement inside atryblock successfully executed before reaching thefinallyblock or subsequent code [1][4][5]. ### Why This Happens When you declare a variable (e.g.,let x: string;) without an initial value, TypeScript treats it as unassigned [3][5]. Even if you assign a value inside atryblock, the compiler assumes that an error could occur before that assignment is reached [1][6]. Because thefinallyblock or subsequent code might execute regardless of whether thetryblock completed, TypeScript flags the potential usage of an uninitialized variable as an error to prevent runtimeundefinedissues [3][5]. ### Recommended Solutions 1. Initialize at Declaration (Preferred) The most reliable way to resolve this is to provide an initial value at the point of declaration [3][7].typescript // Fix: Provide a default value let x: string = ''; try { x = fetchData(); } finally { console.log(x); // Now guaranteed to be assigned }2. Use a Union Type withundefinedIf a value is not immediately available, you can explicitly define the variable as potentiallyundefined[1][6]. This informs the compiler that the variable might not have a value, forcing you to handle theundefinedcase safely [1][6].typescript let x: string | undefined; try { x = fetchData(); } finally { if (x !== undefined) { console.log(x); } }3. Definite Assignment Assertion If you are certain that the variable will be assigned before it is used—and you want to bypass the compiler check—you can use the definite assignment assertion operator (!) [3][5]. Use this sparingly, as it tells the compiler to trust you, potentially hiding actual bugs [7].typescript let x!: string; try { x = fetchData(); } finally { console.log(x); // Compiler accepts this }In summary, TS2454 is a safeguard against unpredictable control flow [3]. By initializing variables or explicitly handlingundefinedstates, you align your code with TypeScript's safety requirements [3][7].Citations:
- 1: microsoft/TypeScript#40290
- 2: microsoft/TypeScript#32836
- 3: https://deverrors.com/errors/ts-variable-used-before-assigned
- 4: https://stackoverflow.com/questions/70526138/typescript-error-variable-reswithstatus-is-used-before-being-assigned-ts245
- 5: https://akousa.net/error-codes/ts-2454
- 6: microsoft/TypeScript#15499
- 7: https://deadends.dev/typescript/ts2454-variable-used-before-assigned/
Initialize
runIdbefore thetryblock.The test project enables
"strict": true. TypeScript can report TS2454 becauserunIdis assigned insidetryand read afterfinally. Initialize it or restructure the code so the assignment is definite.internal-packages/run-store/src/delegatingRunStore.forwarding.test.ts (1)
11-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a crumb marker for this new source-level test.
Add
//@Crumbsor a `#region `@crumbsblock near the source parsing setup. This file currently has no required crumb instrumentation.As per coding guidelines, “Add crumbs as you write code — not just when debugging.”
Source: Coding guidelines
internal-packages/run-store/src/PostgresRunStore.ts (1)
2033-2059: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash # Description: List TaskRunExecutionSnapshot scalar columns and compare with the echoed literal. set -euo pipefail fd -t f 'schema.prisma' --exec rg -n -A 60 'model TaskRunExecutionSnapshot\b' {} \; fd -t f 'snapshotEntry.ts' internal-packages/run-store/src --exec cat -nRepository: triggerdotdev/trigger.dev
Length of output: 11760
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- PostgresRunStore context ---' sed -n '1970,2085p' internal-packages/run-store/src/PostgresRunStore.ts printf '%s\n' '--- SnapshotEntryInput and related hydrator definitions ---' rg -n -C 8 'type SnapshotEntryInput|interface SnapshotEntryInput|SnapshotEntryInput|lastHeartbeatAt|entryFromCreateExecutionSnapshot' internal-packages/run-store/srcRepository: triggerdotdev/trigger.dev
Length of output: 50381
Add
lastHeartbeatAt: nullto the Redis-only snapshot echo.
TaskRunExecutionSnapshotdeclares this nullable column, and the Redis hydrator returnsnullfor it. The echo omits it and bypasses type checking, so callers receiveundefinedinstead ofnull.internal-packages/run-store/src/taskRunExecutionSnapshotStore.readCohort.test.ts (1)
20-24: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the mocked
RunStore.The empty-object cast is a mocked
RunStore. Extract the cohort predicate into a pure function and test that function, or construct the store with a testcontainer-backed RunStore.As per coding guidelines, “We use vitest exclusively. Never mock anything - use testcontainers instead.”
Source: Coding guidelines
internal-packages/run-store/src/taskRunExecutionSnapshotStore.ts (1)
746-754: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
One extra Redis round trip per non-head row in the window read.
getSinceCreatedAtattachescompletedWaitpointIdsto the head row only. Every other row therefore falls into theawait this.redis.getSnapshotWaitpointIds(runId, read.id)branch in#hydrate, so a window oftakerows costs up totake - 1extra Redis calls where Postgres served the same window with one query. The comment on Line 749 states that each row carries its own order, which the store does not currently provide.Two options: return per-row order from the Lua script, or accept an empty order for non-head rows and say so in the comment. If the engine only reads
completedWaitpointOrderfrom the head row, the second option removes the fan-out entirely.Also applies to: 820-826
… 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3ed0a8c-d4ef-4714-85a3-cd91d97326b5
📒 Files selected for processing (5)
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.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/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.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/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointRecords.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/legacyPostgresCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completionEnvelopeSource.test.ts
🧠 Learnings (1)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
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.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fcadf9d-bbb1-477a-945d-67eb8a9740f4
📒 Files selected for processing (5)
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/testFixtures/childRun.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.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/testFixtures/childRun.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.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/testFixtures/childRun.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/testFixtures/childRun.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/testFixtures/childRun.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/testFixtures/childRun.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.runOutput.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointResolver.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/completedWaitpointEquivalence.test.ts
1 participant
Stacked on #4765 — base is
feat/snapshot-store-decorator-tri-13449, notmain, because the frozen record types and the test fixture only exist there. Not mergeable until #4765 lands. The diff is the 4 commits on top.What this adds
The snapshot store left an explicit hole for this lane:
This fills it. Three new units plus one coordinator method:
readCompletionEnvelopesonWaitpointCoordinator, implemented by both arms. The resume path only hadid,status,typeandcompletedAfterper edge — nine fields short of an envelope. The store arm readswp:{id}alone: both halves live under that one key, so one pipelinedHMGETper id needs no run-scoped key and cannot span two cluster slots. The legacy arm reads the rows. Both return the same shape, so the record build never branches on residency.buildCompletedWaitpointRecords— one record per distinct id. The output variant is chosen, never copied: an offloaded value stays a reference, a plain RUN output becomes a marker re-read fromTaskRun.output, a BATCH output is omitted because the runtime discards it at source, everything else rides inline under the pre-existing thresholds. No new cap, no completion-time spill.createCompletedWaitpointResolver— rebuildsCompletedWaitpoint[]from a cycle's ordered id list and records, field-for-field equivalent toenhanceExecutionSnapshotWithWaitpoints.continueRunIfUnblockedbuilds the set once and passes it at both appends.Three things worth reviewing closely
1. The resolver iterates records, not the order. The order holds only batch-indexed ids, because its positions are the indexes. Iterating it drops every index-less wait — each
wait.for, each singletriggerAndWait, each token — and resumes the run without their results.completedWaitpointEquivalence.test.tspins this: invert the iteration and 10 of its 12 cases fail.2. A RUN error and an orphaned RUN must stay inline.
TaskRun.erroris jsonb and does not round-trip to the same string, and the completing-run back-reference isonDelete: SetNull. Only a non-error RUN with a livecompletedByTaskRunIdcan carry thederiveFromRunmarker.3. The fail-loud rule is a coverage check, not a classification.
parseWaitpointIdis documented "Total: never throws" and returns the legacy verdict for any unrecognised shape, so classification alone can never fail loud — a corrupt versioned id would route to Postgres, find no row, and vanish silently. Instead every distinct id must resolve through exactly one half; neither or both throwsUnresolvableWaitpointId.Testing
The equivalence suite's oracle is
enhanceExecutionSnapshotWithWaitpointsitself, not a hand-written literal — a literal cannot catch a drift inenhance.completedWaitpointEquivalence.test.ts— 12 cases, all four types, both index shapescompletedWaitpointRecords.test.ts— 16 cases on the output policycompletedWaitpointResolver.test.ts— 22 cases including the coverage checkstoreCoordinator.test.ts— 6 appended cases on the envelope readtaskRunExecutionSnapshotStore.waitpointRecords.test.ts— 4containerTestcases on the writeGreen:
waitpointCoordinator148 tests, run-store 595 tests across 88 files, typecheck both packages, oxfmt/oxlint/knip.waitpointSystem.test.tspasses 25/25 unmodified.Not run locally: the full run-engine suite (container-heavy). Leaving that to CI.
Both mutation-checked, so neither suite is vacuous: inverting the resolver's iteration kills 10 of 12 equivalence cases, and removing the decorator plumbing kills 3 of 4 write cases.
Inert if merged alone
#completedWaitpointRecordsForgates on the store id format. Nothing mints that format yet, so every live resume supplies no records and a Postgres-resident resume is byte-identical to before. Test diff is 1182 insertions and zero deletions — no existing assertion was edited.No changeset and no
.server-changesnote: this ships dark.Follow-up this does not do
No
recordsreader exists in the store yet — the snapshot lane owns where the hook is called. Whoever wires it must passresolvedElsewherefrom the row fetch, or the coverage check cannot see the legacy half and will reject a mixed snapshot.Review pass (applied)
An adversarial review found two real defects. Both are fixed in
7011b57bc; the notes above describe the state before it.The coverage check was order-scoped. The rule is "every distinct id resolves through exactly one half", but the loop ran over
order— which by this PR's own headline argument omits every index-less wait. So an index-less waitpoint with a missing record resolved to an empty set silently: the guard was blind to the exact loss it exists to prevent. It now runs over the union ofdistinctIdsandorder.That required adding
distinctIdstoResolveCompletedWaitpointsArgs. The freeze test caught it, which is what it is for — an added optional field breaks no compilation, so the pin exists to stop either lane widening the type quietly. The pin and its three construction sites are updated. The field is required: optional-defaulting-to-[]would restore the hole silently. No behavioural impact, since nothing constructs these args yet.The carry-forward fix was incomplete. It covered the resume appends only. Eight copy-forward appends (3 in
dequeueSystem, 3 incheckpointSystem, 1 inrunAttemptSystem, 1 inindex.ts) re-pass the id set with no records, and a refused pointer then minted a replacement withHDELon the records field — ids nothing could resolve, permanently. A copy-forward legitimately has no records of its own, so#resolveCyclenow reads the surviving cycle's records for the refusal branch. One read, on the rare branch, rather than an envelope read on every dequeue and checkpoint.Residual limit: if the head entry is lost entirely there is no cycle to read records from, so they are unrecoverable. The coverage-check fix means that now fails loud instead of resuming with a silently empty set.
Also fixed: the legacy arm was dropping the
runIdrouting hint (findManyWaitpointstakes it third), so every resume fanned out across every run-ops database — and it now reuses the chunked fetch rather than reading a large fan-in whole. AderiveFromRunrecord whose run output is gone now throws instead of resolving atriggerAndWaitwith no output. The envelope read is one concurrent command per id, not a pipeline, since N ids are N cluster slots. The BATCH-output drop is unchanged and correct, but the comment gave the wrong reason and a test now pins the divergence as intentional. The legacy arm's row mapping had no coverage and the equivalence suite hand-rolled a duplicate; both now share one mapper, which has its own suite.Verification: 182 tests across 7 run-engine files including the freeze test; typecheck both packages; lint; knip.
One local-only artifact: a full run-store suite run on my machine showed 7 failures in
runOpsStore.presentersRunReadView.replicaLag.test.ts, untouched by this branch. It passes alone here, alone on main, and beside this branch's new files — and CI's Internal unit tests pass on this tree. Local docker contention against a deliberately frozen replica is the likely cause; not attributed to this change.