feat(run-engine,run-store): completed-waitpoint envelope, read-time resolver, and the fail-loud coverage check - #4779
Conversation
…h coordinator arms
The resume path only has id, status, type and completedAfter per edge, which is nine
fields short of a completion envelope. Add one coordinator method that sources the rest,
implemented by both arms so the record build never branches on residency.
The store arm reads wp:{id} alone: both halves live under that key, so one pipelined
HMGET per id needs no run-scoped key and cannot span two cluster slots. An id with no
record, or a record with no completion, is omitted rather than defaulted.
One record per distinct id. The ordered id list carries multiplicity and holds only batch-indexed ids, so the record set is what says which waitpoints completed. The output variant is chosen, never copied: an offloaded value stays a reference, a plain RUN output becomes a marker re-read from TaskRun.output, a BATCH output is omitted because the runtime discards it at source, and everything else rides inline under the pre-existing thresholds. No new cap and no completion-time spill. A RUN error and an orphaned RUN both stay inline. TaskRun.error is jsonb and does not round-trip, and the completing-run back-reference nulls on delete.
Rebuilds CompletedWaitpoint[] from a wait cycle's ordered id list and records, field-for- field equivalent to the existing snapshot hydration, which is what the executor consumes. It iterates the records, never the order. The order holds only batch-indexed ids, so iterating it would drop every index-less wait: each wait.for, each single triggerAndWait and each token. The equivalence suite pins that, and fails on 10 of 12 cases if the iteration is inverted. The coverage check is the fail-loud rule. The id classifier is total and never throws, so an unrecognised shape would otherwise classify as legacy, find no row, and vanish from the resumed run's completed set. An id that no half resolves throws, and so does an id that both halves claim.
…at the resume appends Carries an envelope per distinct id from the resume path into the wait cycle's key, filling the hole the snapshot store left for this lane. The records ride the mint only: a copy-forward writes no key and needs none. continueRunIfUnblocked builds the set once and passes it at both appends. The build is gated on id shape, so a wait with no store-resident half supplies no records and a Postgres-resident resume is byte-identical to before. Nothing mints a store-format waitpoint yet, so every live path supplies none today. The existing waitpoint corpus passes unmodified.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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.