refactor(run-engine): extract a WaitpointCoordinator seam around the Postgres waitpoint implementation (#4753)
Extracts every Postgres waitpoint and edge operation out of
`WaitpointSystem` into a `WaitpointCoordinator` seam with one Postgres
implementation, so a different coordination backend can be plugged in
later without any caller changing.
Pure refactor. Zero behaviour change, and zero test-file diffs — the
existing engine corpus is the characterisation test.
## What moved
`WaitpointCoordinator` (`waitpointCoordinator/types.ts`, declared with
`type`) has nine members: `clearRunBlockState`, `readRunBlockState`,
`registerBlocks`, `registerBlocksLockless`, `complete`,
`createDateTimeWaitpoint`, `createManualWaitpoint`,
`mintAssociatedWaitpointData`, `createAssociatedWaitpoint`.
`LegacyPostgresWaitpointCoordinator` implements them against the run-ops
store. Its dependencies are `{ runStore, prisma, logger }` only, so it
structurally cannot reach the run lock, the worker, or the event bus —
orchestration stays in `WaitpointSystem`, which keeps all ten public
signatures, all six `worker.enqueue` sites, the racepoints, the snapshot
transitions, and the event emissions.
Two register methods rather than one with a flag, so "the batch path
issues no extra query" is structural instead of conditional. Both share
one private edge-write helper.
## Six notes for reviewers — please read before "simplifying" any of
these
1. **`nanoid(24)` is called twice with different values on purpose**, in
each create path: once for the upsert `where` key, once for
`create.data`. Hoisting either to a shared constant makes the where-key
match the create-key, turning a guaranteed-miss upsert into a possible
update. In `createManualWaitpoint` both calls plus
`WaitpointId.generate()` stay *inside* the retry loop so each attempt
tries a fresh key.
2. **The two enqueue conditions are deliberately asymmetric.** DATETIME
enqueues `finishWaitpoint` unconditionally after a non-cached create,
with `availableAt: completedAfter`. MANUAL enqueues only when `timeout`
is set. That is existing behaviour, not an oversight. The coordinator
returns a discriminated union on `kind` rather than a boolean so the
enqueue is structurally unreachable on the cached path.
3. **One false clause was deleted from a moved comment.** The old
comment on the full-clear delete claimed the caller's `tx` is not
forwarded. The code does forward it, and `PostgresRunStore` uses `tx ??
this.prisma`, so a single store joins the caller's transaction — only
the routing store strips it. The rest of that comment is unchanged.
4. **The MANUAL timeout enqueue now sits outside the P2002 retry loop.**
Safe because the worker is Redis-backed and cannot raise
`Prisma.PrismaClientKnownRequestError`, so the loop never retried on it.
**If a Postgres-backed enqueue is ever swapped in, that equivalence
breaks silently.**
5. **The coordinator caches `runStore`/`prisma`/`logger` at
construction**, where the old code read `this.$.*` per call. Equivalent
only because nothing reassigns them: one assignment at
`engine/index.ts`, and the `resources` object is a `const` that is never
mutated.
6. **Two comments in other files are now stale and were left alone** —
`engine/index.ts` and `completeWaitpointCrossSeamGuard.test.ts` both
describe routing as the first statement of
`waitpointSystem.completeWaitpoint`. Both tests still pass, because that
guard sits in `index.ts` before the delegation. Left untouched to keep
this diff to three files.
## Preserved verbatim
The `unnest` edge CTE rather than a `Waitpoint` join; the pending count
as a separate statement after the edge write (READ COMMITTED needs its
own snapshot); completion's `findWaitpointOnPrimary` re-read through the
*resolved handle* while the blocked-run fan-out goes back through the
*router*; the residency and colocate hints, with colocation objects
built only in the Postgres arm and the count keeping its `runId`
argument; `ON CONFLICT DO NOTHING` and the `(taskRunId, waitpointId,
batchIndex)` multi-index edge semantics; the unread `batchId` select,
which rides inside two `logger.debug` payloads.
`internal-packages/run-store/` is untouched, so the CTE and the conflict
semantics never moved.
## Verification
| Check | Result |
| --- | --- |
| Engine corpus | 61/61 files, 353 passed, 1 skipped, **0 failed**
(baseline: 352 passed, 1 failed) |
| Test-file diffs | **empty** |
| `run-engine` typecheck | `tsc --noEmit -p tsconfig.build.json` exits 0
|
| `webapp` typecheck | 146 errors on this branch, **146 identical errors
at baseline** — pre-existing, none added |
The webapp typecheck does not pass. The failures are pre-existing
(`PrismaPg` not assignable to `never`; missing `@trigger.dev/rbac`
exports) and the sorted error lists are byte-identical to the merge
base, so this branch adds none — but the criterion is genuinely unmet
and needs a separate fix.
No changeset and no `.server-changes` note: internal refactor with no
user-visible change.
## Follow-ups this surfaced
- The dominant RUN waitpoint is still created outside the seam —
`buildRunAssociatedWaitpoint` now mints through the coordinator, but the
row is inserted nested inside `createRun`/`createFailedRun`. That needs
its own packet before a second backend lands, or the commonest waitpoint
gets split across two of them.
- `clearRunBlockState` overloads opposite outcomes on `undefined` versus
`[]`: `undefined` clears every edge, `[]` clears none. Both callers are
correct today; worth splitting when the file is next touched.
- A stray non-`.sql` entry in `internal-packages/clickhouse/schema/`
breaks every `containerTest` in the repo, because the testcontainers
migration reader `readFile`s every `readdir` entry without filtering
despite a comment claiming it filters. Hit this during setup; unrelated
to this change and left for a separate fix. D
Daniel Sutton committed
0205feda393c6c05be44a20c940a59b46d1fd116
Parent: 73f86c7
Committed by GitHub <noreply@github.com>
on 8/24/2026, 11:22:33 AM