Uh oh!
There was an error while loading. Please reload this page.
fix(service-automation): keep a dying loop's completed body steps in the run log - #14185
Conversation
…the run log
A `loop` that died mid-sweep discarded its body's `childSteps` wholesale: the
engine splices them into the run's step list only after a SUCCESSFUL node
result, and a loop whose body throws never produces a result at all. A run that
genuinely performed 3 flag writes and 2 notifies before dying reported
`{selected: 5, acted: 0}` and kept no step record of writes already committed.
The danger is the direction, not the arithmetic: `acted: 0` on a failed sweep
reads as "nothing happened, safe to re-run", which for a non-idempotent body
invites double-execution.
A dying container now carries its completed body steps out on the thrown error
-- a non-enumerable symbol brand, the idiom #3863 already uses to mark
un-routable guard refusals on this same throw path -- and the engine's catch
path folds them into the run log where the success path splices `childSteps`.
The error is rethrown unchanged, so failure propagation, `fault`-edge routing,
`$error` and the run's reported error are all untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs📓 Docs Drift Check6 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run. 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 bfe2575a0eca030611b87310217e96d824541ca8 && git checkout bfe2575a0eca030611b87310217e96d824541ca8
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 45b9051248f86f362b042fa9de63295a8c224073 dbc1d661a112f353bb9e05132f5bcb00e144821a && git checkout -B drift-repro 45b9051248f86f362b042fa9de63295a8c224073 && git merge --no-ff dbc1d661a112f353bb9e05132f5bcb00e144821a
node scripts/docs-audit/affected-docs.mjs --json 45b9051248f86f362b042fa9de63295a8c224073 |
os-steve
commented
Sep 1, 2026
PM 复核:通过 —— 落地前检已过,CI 全绿即转 ready + auto-merge落地前检按最终 5 条路径当场重跑: ⭐ 本轮最值钱的不是修复,是你证伪了两条候选修法派发令说「三条路都未考察,自己量、自己选、写明为什么是这条」。你真的量了,而且量出了反直觉的结果:
⭐ 而被否掉的第四条,理由是语义安全,不是风格
⇒ 一个「记录级」修复若顺手把 loop 改成返回失败,就会把不可路由的守卫拒绝变成可路由的。这是行为面的改变,而本卡是记录面的卡。看出这一点需要读 #3863 为什么存在,不是读 loop 的代码就能得到。 证据形状,逐条对上
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#13803
A
loopthat dies mid-sweep discarded its body's completed steps wholesale. The enginesplices a container's
childStepsinto the run's step list only after a successfulnode result, and a
loopwhose body throws never produces a result at all — the throwunwinds straight past the splice, taking the accumulated array with the stack frame.
Reproduced on this branch's base (
682d03ba7), not from the card's43e18f51cite, withthe real
AutomationEngineand the card's own five-row shape (row 3 has a null owner, sonotifyfails):The direction is the defect, not the arithmetic. An operator reading
acted: 0on afailed sweep reasonably concludes "nothing happened, safe to re-run" — but the writes did
happen. For a non-idempotent body (notifications, counters, external calls) that misread
invites double-execution, and for auditing the rows exist in the data with no step record
to explain them. The run summary is the platform's own honesty instrument (#4354), and on
this path it was wrong in the one direction that causes harm.
Why this fix and not the other two
The card named three unexamined candidates. All three were measured before choosing.
A summary-side repair — rejected on evidence.
summarizeRunis a pure fold over theflat step log, and on the failing path that log holds three entries:
start,query, andthe loop's own
failurestep, which carries no metrics. The counts are notunder-aggregated, they are absent. A summary-side repair could therefore only invent
numbers it has no record of, or degrade to reporting the sweep as unmeasured — and the
honest version of the latter needs a field on the summary, which is
packages/spec/src/contracts/automation-service.tsand the clause-② path limb. Rejectedwithout writing code: the data has to be preserved, not reinterpreted.
Splice
childStepsbefore result grading — measured, and it is a no-op here. This onewas implemented in isolation and run, because the reasoning is easy to get wrong. Adding
the
childStepssplice to the engine'sif (!result.success)branch changes nothing onthis path: the loop does not return a failing result, it throws, so the engine's
catch (execErr)arm runs instead and there is noresultobject to splice from. Themeasured summary after that change was still
{ selected: 5, acted: 0 }, and the step logwas still three entries. The tell is visible in the log without the experiment: the
loop's step carries
EXECUTION_ERROR, which only the catch arm emits. The mutation wasconfirmed on disk by blob hash and marker count before the run, and reverted with
git diff HEADproven empty afterwards.Flush per-iteration — the right instinct, in the wrong place. For the loop to write
body steps into the run log as it goes, it needs the run's step array, and
NodeExecutor.execute(node, variables, context)does not receive it. Reaching it meanswidening the executor contract for every executor to serve one container.
What shipped keeps the per-iteration instinct but uses the seam #7546 already built.
runRegiongrew apartialStepssink for exactly this — surfacing a failed region'spartial steps — and wired only
try_catchto it, noting that "callers that do not pass asink (
loop,parallel) are unaffected". So:loopnow passes itschildStepsaccumulator as that sink, which captures thefailing iteration's steps too (success returns them instead, so nothing is counted
twice), and
non-enumerable symbol brand, which the engine's catch arm folds into the run log in the
same position the success path splices
childSteps— behind the container's own step,ahead of any
faulthandler's.The brand is not a new idea in this package: #3863's
markGuardRefusalalready carries"this failure is un-routable" out through this identical throw path the same way.
#7546 declined the exception channel for
runRegionbecause it "would either change whatcallers catch or require a bespoke error type" — neither cost lands here, since this is
the opposite direction (executor back to engine) and the error object is passed through
untouched rather than wrapped.
What it means under partial failure
The run still fails. Nothing about accept/reject behaviour moves — that was the deciding
constraint, and it is why the losing alternative lost. Having
loopswallow the failureand return
{ success: false, childSteps }would have made the engine's failure branchwork, but it would also rewrite the run's error text (
Node 'notify' failed: ...becomesNode 'each' failed: Node 'notify' failed: ...), change the container step's code fromEXECUTION_ERRORtoNODE_FAILURE, set$errorwhere it previously stayed unset, and —decisively — make a guard refusal raised inside a loop body routable by a
faultedge onthe loop. That is precisely the one-edge switch #3863 exists to prevent. A record fix must
not move accept/reject behaviour, so the error is rethrown with its identity, message and
guard-refusal marking intact.
What changes is only the record, and only for iterations that actually ran:
Rows 4 and 5 were never entered and no step claims they were. The per-node breakdown now
lists the body nodes that ran (
flagruns 3 acted 3;notifyruns 3 failures 1 acted 2)instead of only the container. Every folded step carries a
parentNodeIdset byrunRegion's tagger, so the ADR-0044 runaway guard — which counts only top-level visits —cannot see them either.
Nesting is handled rather than assumed: as a failure unwinds through nested containers each
one re-brands the same error with its own accumulator, and because an outer container's
sink has already absorbed the inner one's steps, the last write is a superset of every
earlier one and each step object still reaches the log exactly once. The brand is writable
for this reason — a non-writable one would make the second container to unwind throw a
TypeError, turning a nested-loop failure into an engine crash. There is a test for it.Verification
All readings below are at
dbc1d661a, the branch head.The directional assertion is the pin.
expectNeverUnderReportsasserts the reportedactedis never lower than the writes the store actually holds, rather than that itequals 3 or 5 — the direction survives a refactor, the literal does not. Under ablation it
reads:
Reverse control. A sweep whose body's first node fails writes nothing and must still
report
acted: 0, because there that is the honest answer. It does, and it stays greenunder the ablation — it is insensitive to this fix by construction, which is exactly what
stops the repair degenerating into copying
selectedintoacted. A second reverse casepins the boundary: failing on the first element after one write reports
acted: 1, not 0and not
selected.Controls, declared as controls and not offered as ablation evidence. An all-succeeding
sweep and the
try_catch-contained path (#13681's face) were captured before and after thechange and are byte-identical — the measured lines diff clean, sha
6b1579d5...and7bf71866...respectively. The failing run's propagation fields (success,error,run.status) are byte-identical too.Ablation. Direction predicted first: removing the engine-side fold should redden the
six tests that read the recovered record and leave the four that do not. Measured exactly
that — 6 failed, 4 passed, and the four greens were the predicted ones. The mutation was
confirmed on disk before the run (deleted-text count 1 to 0, injected marker 0 to 1, blob
b5e4b3f5...to19682f89...), never from an editor's exit code, and the restore wasproven by state afterwards (
git diff HEADempty, blob back tob5e4b3f5...), not by atrap firing. No rebuild leg applies: the subject is reached through relative same-package
imports (
../engine.js,./loop-node.js), so vitest resolves source, neverdist—confirmed empirically, since editing only
service-automation/srcwithout rebuilding thatpackage changed the measured result.
Suites and gates.
pnpm --filter @objectstack/service-automation exec vitest run --maxWorkers=2— 95files, 1133 tests, all pass.
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, both sections readwhole: 30 path-derived plus 7 convention-triggered. 34 of 35 pass. The one exception
is
scripts/check-test-completeness.mjs, which exits 3 = PREREQUISITE NOT MET locallybecause it grades a tee'd
turbo run testlog that only CI produces; its own output saysto record it as NOT MEASURED and that it "is not a red". Recorded as NOT MEASURED, not as
a pass. Every exit code was captured before any pipe.
pnpm check:type-check-debt(the--re-measureratchet) passes with the workspaceclosure built, so the ledgered count did not drift.
pnpm lint(eslint . --no-inline-config, whole repo) — exit 0, 64s. No narrowing.pnpm check:nul-bytes— clean over 7741 files; the diff was also scanned directly forraw control bytes.
tsc --listFileslists all four touched sources, the new test file included, and theonly errors reported are the three pre-existing ledgered
TS2341innested-region-parity.test.ts, a file this PR does not touch.Scope
Deliberately not addressed here, and each remains open: #13681's ruled B-branch is the
contained path's docs and lint visibility, whereas this is the uncontained path's
summary under-reporting; the resume ordering,
forgetSuspendedRunandtraverseNextare#13937's decision surface in the same file; and
restoreConsumedSuspensionis untouched.No file under
packages/spec/is modified, so the declared shapes ofAutomationResult,the run summary and
stepsare unchanged —StepLogEntryis engine-local.One adjacent finding went to its own card, #14184, and remains open:
try_catchwith nocatchregion returns a failing result and deliberately withholds itschildSteps, citingthe same engine asymmetry. This PR closes that asymmetry for the throw path only and leaves
the engine's returned-failure branch alone, since no executor returns
childStepstheretoday and a fold for zero producers would be speculative — so a
try_catchwith no handlerstill loses its try-region record.
Generated by Claude Code
Generated by Claude Code