feat(run-engine): Redis waitpoint store coordinator, Lua protocol, and waitpoint ids - #4761
Conversation
Thread runId into #observeSizes so all three high-water logger.warn payloads name the run, per spec. Adds a capturing-logger test proving the warning fires with the run id above the mark, and stays silent under a high threshold.
Record actual byte values instead of booleans and partition per append so a mis-wired metric can't hide behind a flat toContain. Cover the succeeding direction of expectedCur: "" against a genuinely unset cur, assert recordCycleMismatch fires, pin the CRC16 helper against a known vector plus a negative control, bound the prefixed cycle-key TTL, prove cur is untouched by a stale CAS, and add a matching-environment getSince with a non-empty window.
A retried append whose write already succeeded advanced cur to its own id, so the CAS above the duplicate guard saw its own id as a stale expectedCur and reported forked instead of duplicate. Snapshot ids are unique per append, so checking duplicate first is always correct.
Adds the reachable-in-tests, unreachable-in-prod case where the Lua- chosen head is dropped by the TS env filter. It surfaced a real bug: headOrder stayed attached to whatever row ended up last after filtering, donating the dropped head's waitpoints to it. Track whether the actual head row survives and only then attach its order.
… order Implements the spec's read-side check that was previously unwritten: a sentinel problem (an empty order string meant both "read as empty" and "not read for this row" in getSince's tail rows) blocked it. #decode now takes an explicit orderKnown flag, runs the count-vs-length check only when the order was actually read, and never sets completedWaitpointIds on a row whose order wasn't read -- which also removes the need to delete it again afterward.
…is-snapshot-store
parseWaitpointId no longer strips an arbitrary <prefix>_ before classifying a body, so a run_ or batch_ id can never be misread as a waitpoint id. deriveWaitpointIdFromAnchor keeps its own prefix-agnostic stripping, since its input is always a known run/batch anchor.
…ard-test coverage - runAbsorbBlockers now computes pendingOfRequested once after every write in the batch lands, as distinct requested ids with no entry in done, instead of incrementing during the loop — the incremental count was order-dependent and could report a waitpoint as both pending and delivered. - runAbsorbBlockers, runClear and wpIdemReserve reject a bad arity/expiry before their first write, so a caller mistake cannot half-apply a script. - wpRegisterOrReport now uses HSETNX for the watcher write, matching the edge's ON CONFLICT DO NOTHING semantics: the first registration wins. - assertKeysForTest now delegates through the private #call funnel instead of calling assertSingleSlot directly, so its own test fails if the guard inside #call is ever removed. - createIfAbsent decodes the record and status fields explicitly instead of relying on ?? against a Lua '' sentinel, and throws a diagnosable error naming the waitpoint id if the record blob is unexpectedly missing. - Adds direct-Lua coverage for the two straddle orderings and the three arity guards, a decode-correctness test for an absent completion field, and fixes a lexicographic sort in an existing assertion.
…verage - scripts.ts's header rule 3 stated that a Lua false or nil truncates the reply array. Measured against a live Redis: a missing HGET returns Lua false, not nil, and false does not truncate anything after it — only a genuine Lua nil does. The or '' coercion exists to give absent values one decoded shape, not to prevent truncation. Corrected the comment so it states what was actually measured. - Adds four more direct-Lua cases for runAbsorbBlockers's pendingOfRequested/pend count: two distinct unreported ids, one reported plus one unreported, the same unreported id passed twice, and an id already in done passed unreported. The round 1 fix only had straddle-ordering coverage; these round out the behaviour that was hand-verified but unguarded. - assertKeysForTest still delegates through #call so a mutation removing the guard from #call fails its own test, but no longer returns or awaits that call's promise: a valid-key invocation's eventual settlement is swallowed instead of risking an unhandled rejection. - Renames the direct-Lua describe block to name all three scripts it covers.
…nt, drop dead ?? on Lua reply
… edge-field split
…number The RunBlockEdge comment pointed at stale waitpointSystem.ts line numbers that no longer match the file after this branch shrank it. Name the continueRunIfUnblocked method instead so the citation can't drift again.
`BlockedRun` is only named inside types.ts, by CompleteResult. The repo's knip gate rejects unused exports, so drop the export keyword rather than add a knip.json exception — nothing outside this file needs the name yet.
…eam-tri-13373' into feat/waitpoint-store-coordinator-tri-13440
…coordinator-tri-13440
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (45)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (1)
WalkthroughThe changes add typed waitpoint ID generation, derivation, and parsing. They add Redis key builders, field encoders, hash-tag validation, and atomic coordination scripts. Merge Risk: 🔵 Low · up to The new Redis waitpoint functionality is not connected to live traffic, so it does not change current production behavior. Merge is reasonable with owner awareness that required diagnostic instrumentation is still missing from the new key and coordinator paths and should be added or explicitly accepted before relying on this code operationally. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…g regex
The tag scan reproduced the first NON-empty brace pair, where Redis stops at
the first pair and treats an empty one as no tag at all. The two disagreed
about the slot for a key like wp:{}{a}. The pattern also backtracked
quadratically on a key made of many opening braces, which CodeQL flagged.
Replaces it with a two-indexOf scan that mirrors keyHashSlot exactly.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…coordinator-tri-13440
The loser-discard deletes this call's own record, which is only safe for a freshly minted id that was never handed out. A RUN or BATCH id is derived from its anchor, so any caller can recompute it and register a watcher on it, and discarding one could delete a record already in use. Also documents that `created` means this call won the reservation, not that the id is new: a retry by the original creator loses to its own reservation and reports false.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts (1)
220-537: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required crumb markers.
The new coordinator and integration-test paths have no
//@Crumbsmarker or `// `#region` `@crumbsblock. Mark the new work before merge.
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts#L220-L537: Add crumb markers around the new waitpoint lifecycle and block-state operations.internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts#L704-L838: Add crumb markers around the new idempotency integration-test flow.As per coding guidelines, “Add crumbs as you write code” and mark lines with
//@Crumbsor `// `#region` `@crumbs.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ba4ce1f-e859-4394-a853-a4d9677afaea
📒 Files selected for processing (2)
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (46)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: typecheck / typecheck
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - npm)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: sdk-compat / Node.js 20.20 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - pnpm)
- GitHub Check: sdk-compat / Node.js 22.23 (warp-ubuntu-latest-x64-4x)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: sdk-compat / Node.js 24.18 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 26.4 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.tsinternal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
**/*.{test,spec}.{ts,tsx}: We use vitest exclusively. Never mock anything - use testcontainers instead.
Test files go next to source files (e.g.,MyService.ts->MyService.test.ts).
Files:
internal-packages/run-engine/src/engine/waitpointCoordinator/storeCoordinator.test.ts
…ordinator Add tests for the winner's own retry in createWithIdempotencyKey, a COMPLETED record round-tripping through createIfAbsent, absorbBlockers reading back a stored delivery envelope rather than its flag, and a new genuine-concurrency suite (real Promise.all races, no mocks) covering complete, registerOrReport, createWithIdempotencyKey, and registerBlocks under contention. Each was proven against its mutant and restored clean.
…coordinator-tri-13440
3 participants
Builds the Redis-backed half of the waitpoint coordinator, beside the Postgres coordinator that #4753 extracted. Adds the coordination protocol as Lua scripts, the run-ops-format waitpoint id scheme, and the key layout. No caller wires any of it up.
Refs TRI-13440.
Inert by construction
Merging this changes nothing observable. 3180 insertions, zero deletions, nine new or additively-edited files.
WaitpointStoreCoordinatoris never constructed outside its own tests and the benchmark.redisOptionsas a constructor argument.waitpointSystem.tsis untouched. Every live waitpoint operation still runs on Postgres through the coordinator merged in refactor(run-engine): extract a WaitpointCoordinator seam around the Postgres waitpoint implementation #4753..server-changesnote — nothing here is user-facing yet.Deploying this needs no Redis or MemoryDB instance. That becomes a prerequisite when a later change routes traffic onto the store behind a per-organisation flag.
What's here
Nine Lua scripts, each atomic on one hash tag. Seven mutate state — create-if-absent, register-or-report, complete, idempotency reserve, absorb, deliver, clear. One reads state (
runReadBlockState) and is separate because the pending, delivered and edge sets must be read as one consistent view. One discards an idempotency loser.Two hash tags, deliberately.
wp:{waitpointId}holds a waitpoint's record, status, completion envelope and watcher hash.wp:run:{runId}:*holds one run's pending set, delivered set and edge set. A waitpoint has N watchers, so it cannot live under any single run's tag.Waitpoint ids reuse the run-ops body layout: a 24-char base32hex core, a type char (
r/b/d/m), and version charw. RUN and BATCH ids derive from their anchor's core, so create-if-absent is idempotent with no lock.parseWaitpointIdis total and never throws.The single-slot guard. Every script invocation goes through one private wrapper that asserts all keys share a hash tag. A single-node test server accepts what a real cluster rejects, so this assertion is the only enforcement — and it is mutation-tested: removing it fails a test.
Measured
Against the same population of real Postgres rows:
Both are lower bounds: the benchmark charges Postgres a
COUNT(*), while the resume-time read is a join with a partial select plus filtering in JavaScript.Store-only paths, no Postgres counterpart: block+complete+deliver 0.88 ms p50; 100-watcher fan-out 13.8 ms; a 1001-edge fan-in 149.8 ms, flat at 0.15 ms per edge and round-trip bound rather than algorithmic.
The benchmark lives in
*.bench.test.tsand is excluded from the default suite.Review notes
types.ts(from refactor(run-engine): extract a WaitpointCoordinator seam around the Postgres waitpoint implementation #4753) carries the coordinator interface;storeCoordinator.tsdeclares its own operation types because this was built in parallel. The wiring change reconciles them.Verification
79 tests in the coordinator suite, 58 in the id suite.
typecheckon run-engine and webapp,buildon core,knip,oxfmtandoxlintall clean. The engine corpus passes 82/82.Every invariant is mutation-tested rather than merely asserted. A whole-branch review ran 14 mutants and killed 12; the two survivors were fixed with their own mutation checks.
🤖 Generated with Claude Code