Uh oh!
There was an error while loading. Please reload this page.
fix(service-automation): seed a retry attempt's variable environment through the same chokepoint as attempt 1 - #9888
Conversation
…through the same chokepoint as attempt 1 `executeWithoutRetry()` — the method `retryExecution` re-runs the flow through on every retry attempt — seeded only the flow's declared variables and `$record`, while `execute()` also binds `record` plus the trigger record's flattened fields, `previous`, `$runId`, `$flowName` and `$flowLabel`. A retry attempt therefore ran in a strictly smaller variable environment than the first. Under strict CEL an unbound name ABORTS the predicate rather than yielding false (#4697), so a start condition or edge predicate reading `previous` (#3427) or a bare record field failed on the retry for a reason attempt 1 never hit, and a pausing node on a retry attempt had no `$runId` to map its external state back to this run with (ADR-0019). Both methods now seed through one private `seedRunVariables` helper, the `buildRunTrigger` chokepoint pattern. First-attempt behaviour is unchanged: the helper is `execute()`'s own block verbatim, and the run id is now minted before the seeding (order-safe — `nextRunId()` is a stateless random id). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
…pair Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
📓 Docs Drift CheckThis PR changes 1 package(s): ⛔ 2 release-owned page(s) name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin dd9f3c24483e1b31398d5cd1b648dc1b744d0962 && git checkout dd9f3c24483e1b31398d5cd1b648dc1b744d0962
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4f994de82c376102025f2e64d3c4205665094859 0dc1f8406aee472b40d551814110d75e0a984105 && git checkout -B drift-repro 4f994de82c376102025f2e64d3c4205665094859 && git merge --no-ff 0dc1f8406aee472b40d551814110d75e0a984105
node scripts/docs-audit/affected-docs.mjs --json 4f994de82c376102025f2e64d3c4205665094859
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9704
The defect
executeWithoutRetry()— the methodretryExecutionre-runs the flow through on every retry attempt — seeded onlyseedDeclaredVariables(flow, context)and$record.execute()seeds all of that plusrecord(a friendlier alias), the trigger record's fields flattened to top-level names,previous,$runId,$flowNameand$flowLabel.So a retry attempt ran in a strictly smaller variable environment than attempt 1. Conditions are strict CEL, where reading an unbound name ABORTS the predicate (
Unknown variable: X) rather than yieldingfalse(the mechanism #4697 documents), which makes the divergence user-visible exactly where retry is most used —errorHandling.strategy: 'retry'on a record-change flow:previous(the create-vs-update discriminator record_change flow start node binds to a single lifecycle event — no create-OR-update in one flow #3427 binds deliberately, always, even tonull) aborted — the retry failed for a reason attempt 1 never hit, reading as a flaky flow rather than a defect;status/budgetreference to a triggering-record field aborted for the same reason, since the flattening wasexecute()-only;$runIdwas undefined for a pausing node on a retry attempt, so an approval request row minted there carried no run id and the external record could not point back at the (since bug(service-automation): a retry attempt that PAUSES is recorded as failed and its suspension is never persisted —executeWithoutRetryhas noisSuspendSignalarm #9510, durably stored) pause — the ADR-0019 contract.Premise re-derivation
The card measured at
ed4ca5999; the repair for #9510 (PR #9785) landed onengine.tsafterwards. Re-derived fresh against mergedmain(55d29935b): the divergence is intact and complete — the five seedings named are exactly the delta, with no additions and none already repaired. Only the line anchors moved.The repair
Per the dispatching seat's route ruling (shared helper hoist, over passing the seeded map from the caller), following the
buildRunTriggerchokepoint precedent and the four prior drift cards on these two methods (#9378, #9415, #9414, #9510 — all in the same direction: the copy that is notexecute()is the one a repair forgets):seedRunVariables(flow, flowName, context, runId)holdsexecute()'s seeding block verbatim, in the same order (declared variables first, so the record flattening cannot shadow a flow input);execute()andexecuteWithoutRetry()call it;$runIdis part of the environment. Order-safe:nextRunId()is a stateless random id with no counter to advance, and nothing between the old and new call sites reads or mints one. The caller still owns the id (it keys the log row, the continuation and the returned envelope) rather than the helper minting a second one that would name no resumable run.First-attempt behaviour is unchanged by construction and by measurement (see below).
The parity pin flips substantively
retry-attempt-pause.test.ts(from #9510) pinned this divergence explicitly so it could not be repaired silently, and its own comment named the correct edit. The threenot.toContainassertions are replaced by assertions of the new semantics, not merely deleted:$runIdnormalized), the same discipline the result envelope above it already used;$runIdis asserted by value against the run id each route returned — a snapshot merely carrying some$runIdis the ADR-0019 mapping hole this card is about, and a presence-only check cannot see it;$flowName,$flowLabel,previous(viatoHaveProperty, since absent andnullboth read asnullthrough?.),recordand the flattenedamount/idare pinned by value on the retry route — because the parity comparison alone is equally satisfied if both routes lose them, which is the shape a later "simplification" of the shared helper would take.Every genuinely-illegal-shape assertion in the file is untouched, including the retry-accounting pins (
still burns the full budget when attempts genuinely FAIL) and the resumed-run budget answer.Verification
All at
0dc1f8406.Red before green. With only the test flipped and
engine.tsunrepaired:AssertionError: expected undefined to be 'run_c27e4d29-...'— the retry snapshot carried no$runIdat all. After the engine fix: 6/6 pass.First-attempt behaviour unchanged. Full package suite is 82 files / 988 tests passing, identical file and test counts to the pre-change baseline run on
origin/main.Ablation (both legs, red then byte-identically restored). The suite resolves
engine.tsfrom source — this is an intra-package relative import (./engine.js), not a dependency resolved throughexportstodist/, so no rebuild gates the measurement. That is not assumed: the red-before-green pair above proves it directly, since editing only the test went red and editing onlyengine.tswent green with no build in between.variables.set('previous', ...)from the helper:AssertionError: expected { '$record': ..., ...(7) } to have property "previous" with value null;recordalias and the flattening loop:AssertionError: expected undefined to deeply equal { id: 'ord_9510', amount: 500 }— and note this failed on the by-value assertion, not the parity comparison, which is exactly the guard those assertions were added for.Both restored with
git checkout HEAD -- src/engine.ts, verified byte-identical by blob hash (git hash-object=2d61de4d9ceccd18b687490206f19da5bac9ac36=git rev-parse HEAD:...) with clean porcelain, and the green run re-taken from the committed state.Typecheck.
npx tsc --noEmit -p tsconfig.jsonin the package: 3 errors, all pre-existing innested-region-parity.test.ts(TS2341, privateflowsaccess), matching the debt ledger's recordederrors: 3for this package — none in either changed file. Notepnpm --filter @objectstack/service-automation typecheckwas deliberately not used as evidence: the package declares notypecheckscript, so that invocation matches zero scripts and exits 0 without running anything.Gates. Named by the dispatch, then re-derived against the actual diff with
node scripts/pm/dispatch-gates.mjs(which derives its own change set from the merge base) and everything it added was run:check:cross-package-test-inputsOK: 12 package(s) read outside themselves, all declaredcheck:test-source-aliasOK — 72 packages with tests scannedcheck:type-source-resolutionOK — 76 packages with a tsconfig.json scannedscripts/check-cross-package-test-inputs.mjsOK: 12 package(s) read outside themselvesscripts/docs-audit/check-affected-docs.mjs✓ affected-docs self-test: 242 cases pass.check:changeset-gate-self-testscheck:objectui-changeset✓ objectui-range --self-test: all checks passedscripts/check-adr-0087-registration.mjs✓ this PR adds no declared-breaking changesetscripts/check-changeset-no-major.mjs✓ This diff introduces no major bump.scripts/check-empty-changeset.mjs✓ No empty-frontmatter changeset introducedcheck:query-options-erasure✓ ratchet holds ... none newcheck:engine-double-contractOK — 321 pinned, 133 in the DEBT ledger, 2 exemptcheck:where-matcher✓ conformance holds: 258 matcher(s) ... none newcheck:type-check-coverageOK — 64/77 workspace packages type-checkedcheck:type-check-debt--re-measure: OK — 33 ledger entr(ies) re-measured in 351.9s, 1926 raw tsc error(s) total, none above its recorded number.check:nul-bytesOK (scanned 6282 text file(s) ... no raw ASCII control bytes)check:type-check-debtrefuses to measure without a built closure (it would otherwise measure a different world), so the full 70-package workspace build was run first, exactly aslint.ymldoes.Changeset
@objectstack/service-automationis a published package (noprivate: true,publishConfig.access: public) and this is a user-visible behaviour change, so a patch changeset is included.Repricing note for the serialized sibling #9705
Unaffected.#9705 is the resume path (
resumeInternalnever readsflow.errorHandlingand never entersretryExecution), while this change touches only how the two execution entry points seed their variable map; the pin answering that question (a resumed run does not retry, on either route) is untouched and still green.Generated by Claude Code