Uh oh!
There was an error while loading. Please reload this page.
fix(service-automation): a try_catch whose catch region itself fails keeps both regions' step record - #14813
Conversation
…ion fails `try_catch` returns a failure from three sites. #14184 taught the engine's returned-failure branch to fold `childSteps` and taught the no-`catch` producer to supply them; the `catch`-present-and-failing return was left unfolded and still discarded the whole step record. It is the worst of the three for an operator, because two regions ran: the try region may have written rows before it failed and the handler may have written more before IT failed, yet the run log kept a step for neither, so the #4354 summary reported `acted: 0` over writes that had landed. The catch region now gets the same `partialSteps` sink the try region already had (`runRegion`'s fifth argument) and the failing return carries `[...failedAttemptSteps, ...catchAttemptSteps]` — failed try attempts first, matching the successful-catch return's ordering. `runRegion`'s tagger already supplies `regionKind` on its failure path, so no tagging is added here and no engine change is needed. The pin that recorded the old boundary ("carries no try steps") is inverted in place with its comment rewritten to say what it used to assert and what moved it, rather than deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…y-catch-failing-catch-steps
…y-catch-failing-catch-steps
…y-catch-failing-catch-steps
📓 Docs Drift Check1 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 d47c1bb46f8b7c97c8cd341481fa762e8ffeaf7e && git checkout d47c1bb46f8b7c97c8cd341481fa762e8ffeaf7e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 5a5336b399db2ef18dd4700f97d579a328197dda 20aa3b0d963eb00258967370ad7b493295da9d8c && git checkout -B drift-repro 5a5336b399db2ef18dd4700f97d579a328197dda && git merge --no-ff 20aa3b0d963eb00258967370ad7b493295da9d8c
node scripts/docs-audit/affected-docs.mjs --json 5a5336b399db2ef18dd4700f97d579a328197dda |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#14222
A
try_catchwhose catch region itself fails now returns the step record of both regions instead of discarding everything.try_catchreturns failure from three sites. #13803 taught the engine to fold a dying container's carried steps off the THROW channel; #14184 taught the returned-failure branch (if (!result.success)) the same and taught the first producer — atry_catchwith nocatchregion — to supply them. The second producer was left unfolded. When acatchregion is present and the handler itself fails, the return droppedchildStepsentirely, so the run log kept a step for neither region and the #4354 summary reportedacted: 0over writes from two regions that had genuinely landed.acted: 0on a failed run reads as "nothing happened, safe to re-run".The half that was genuinely missing was the catch region's own steps: the failed try attempts were already in scope, but the catch region ran without a
partialStepssink, so when the handler threw its completed steps unwound with the stack. The catch region now receives the same sink the try region already had (runRegion's fifth argument), and the failing return carries[...failedTryAttempts, ...catchAttempts]— failed try attempts first, the ordering the successful-catch return has always used.Ruling of record
issuecomment-5503054554(triage, 2026-09-02 01:40:34Z). Comments read to the last page: 2 — the triage ruling and the dispatching seat's claim comment. The ruling settles the design the card asked for, as a restore-invariant: "the run log must carry every step that ran, whichever region ran it." This PR implements exactly that shape — the sink asrunRegion's fifth argument,[...failedAttemptSteps, ...catchAttemptSteps]with failed try attempts first,regionKindsupplied byrunRegion's existing tagger, and noengine.tschange. The standing fence onengine.tswas not approached: the #14184 fold that reads these steps is already in place atengine.ts:7141.The pre-existing pin "a failing
catchregion still fails with the catch error and carries no try steps" is inverted in place with its comment rewritten, not deleted — what it used to assert is written out in the new comment, per the ruling.Ablation — two legs, both red, restore proven
Subject resolution stated up front: the pin suite lives in
packages/services/service-automation/src/and imports its subject relatively (../engine.js,./try-catch-node.js), so vitest resolves it to source, neverdist/. The package's only vitest alias is for@objectstack/platform-objects, unrelated. No rebuild ofservice-automationis in the ablation's path, and the mutation is live in the next run; it is proven on disk by blob hash and anchor-occurrence counts, never by a comment marker.All four runs below are at
20aa3b0d9, withHEADblob oftry-catch-node.ts=66eac7ad9.66eac7ad9== HEADTest Files 1 passed (1)/Tests 17 passed (17)childStepsfrom the failing-catch return43360a513!= HEADTest Files 1 failed (1)/Tests 2 failed | 15 passed (17)childSteps)a2b6b440b!= HEADTest Files 1 failed (1)/Tests 2 failed | 15 passed (17)git checkout HEAD -- ABSOLUTE_PATH66eac7ad9== HEAD;git diff HEADemptyTest Files 1 passed (1)/Tests 17 passed (17)The mutation script carries
trap RESTORE_FN EXIT INT TERMwith absolute paths derived fromgit rev-parse --show-toplevel; restore is proven by blob equality against theHEADblob (an empty hash is treated as failure), not by the trap having fired and not by an exit code.The two legs discriminate the two halves, which is the point. Leg A removes the whole record:
acted=0, grouped steps[]. Leg B keepschildStepsbut removes the sink — this is precisely the one-liner #14184could have written — and it lands atacted=2over 3 writes that happened, withcatch:cw1:success/catch:handler:failuremissing from the log:Leg B is the one that matters for scope: it proves the sink — the genuinely new seam, the part the card said was not a mechanical mirror of #14184 — is load-bearing and not decoration.
In both legs exactly 2 tests fail and 15 pass. The 15 include every control that must not move: "still fails the run, with the same error text and step code", "still routes down the
faultedge, with the same$errorcontents", the contained-path controls, the honest-zero reverse controls, and the three nesting/double-count pins. That is the measured evidence that this change is additive to the record only.Adversarial reading of the 48-line diff
Asked directly whether the diff does only what the card asks. It does. Specifically:
runRegionpushes intopartialStepsonly inside its owncatcharm (engine.ts:tag(); partialSteps?.push(...regionSteps);then rethrow). On success the sink is never written andregionStepsis returned as before. The successful-catch return still uses the returnedcatchSteps, so passing the fifth argument changes nothing a caller observes when the handler succeeds — and no step can reach the log twice by this route. The suite pins that independently (expect(new Set(steps).size).toBe(steps.length)).runRegionstill rethrows,catchErris still caught at the same place, and the error string is byte-identical (try_catch '${node.id}': catch region failed — ${catchMsg}).success: falseis unchanged.[...failedAttemptSteps, ...catchAttemptSteps]— the ruling's rule, and the same one the successful-catch return above it already used.regionKind: 'try'/'catch'andparentNodeIdcome fromrunRegion's existing tagger, which runs on its failure path as well as its success path.runRegioncall from 3 lines to 8 to take a fifth argument, plus a ~22-line comment block, accounts for the bulk. The behavioural delta is two things: one newconst catchAttemptSteps: StepLogEntry[] = [], passed as the fifth argument, and one newchildSteps:key on the failing return.git diff | grep '^+.*export'→ none), andengine.tsis not in the diff at all.I found nothing that reaches beyond the card. The one behavioural consequence worth naming explicitly is intended and is the fix itself: the run summary's
actedfor this path changes from an under-count to the true count (0→3 in the new pin). That is a corrected value in an existing field, not a new field.Clause ② —
noNo consumer-visible payload key is added and no accept/reject behaviour moves.
childSteps?: StepLogEntry[]is a pre-existing optional field onNodeExecutionResult(engine.ts:349), already produced by both success returns in this same executor and by the no-catch failing return since #14184; this return starts populating it, it does not introduce it. The element typeStepLogEntrygains no field —engine.tsis untouched, which is the mechanical check the dispatch asked for. Accept/reject is unmoved: samesuccess: false, byte-identical error text, sameNODE_FAILUREstep, same$errorwrite, samefault-edge routing — all of which live inengine.tsand are pinned green through both ablation legs.Changeset
.changeset/try-catch-failing-catch-step-record.md→"@objectstack/service-automation": patch. Confirmed rather than assumed: no exported symbol is added, no type is widened, nopackage.json/exportsmoves, and the only change is that an existing optional field is now populated on a path where it was absent. The direct precedent agrees — #14184, the same defect one path over in the same file, landed aspatch(.changeset/trycatch-returned-failure-step-record.mdonmainat7d3b1b79c).Gates
Re-derived on the final diff with
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack CHANGED_PATHS→ 35 commands. Exit codes captured by redirecting first (cmd > log 2>&1; EXIT=$?), never across a pipe. 32 green, 3 NOT MEASURED — each of the three exits 3, and each prints its own verdict text declaring the prerequisite unmet:node scripts/check-test-completeness.mjs— "PREREQUISITE NOT MET — this gate grades a savedturbo run testlog, and no log was named… ⛔ It is not a red." CI tees the log and passes the path.pnpm check:dual-build-cjs-loads— "PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/… ⛔ This is NOT a pass: nothing was measured." Needs a fullpnpm build. My diff touches nopackage.json, noexports, and no build config, so the dual-build surface is untouched by construction.pnpm check:type-check-debt— "PREREQUISITE NOT MET… ⛔ This is NOT a pass and NOT a finding." Needs the built dependency closure. Its self-test passed (48+68+43+28+19+18 cases) and the non-re-measurepnpm check:type-check-coverageran green.Also run and green outside the derived set:
pnpm check:nul-bytes(self-test 75 assertions; 8071 files scanned, clean), plus a direct control-byte scan of the three changed files (grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]'→ no match).Package verification, all at
20aa3b0d9:pnpm --filter '@objectstack/service-automation^...' build— dependency closure built first.pnpm --filter @objectstack/service-automation build— green; DTS emit succeeds (2/2 declared declaration file(s) present), which type-checks the source.pnpm --filter @objectstack/service-automation test—Test Files 101 passed (101)/Tests 1203 passed (1203).ESLint — a declared narrowing, not a skip. Ran
eslint --no-inline-configon the changed paths rather than the whole repo, with all three pieces of evidence:File ignored because no matching configuration was supplied, i.e. it places 2 of the 3 changed files in the lint population.--format json— 3 entries reported, 2 linted, 0 errors, 0 real warnings.eslint.config.mjs:326-329states this repo "runs oneeslint.config.mjs, which never enables type-aware linting (noparserOptions.project, no typed@typescript-eslintrules) for ANY file, test or not", corroborated by everyparserOptionsin the config carrying onlyecmaVersion/sourceType. With no cross-file type program there is no mechanism by which an edit underservice-automation/src/builtin/can move any untouched file's verdict.pnpm lintiseslint . --no-inline-config— the same binary, flag and config I invoked.The repo-wide
pnpm lintrun itself is CI's, and CI runs the farm exactly once regardless.Not encountered
The known repo-wide flake at
packages/cli/test/run-dev-unbuilt-workspace.e2e.test.ts:317did not appear — nothing in this card's verification touchespackages/cli.Provenance
This branch was pushed by an earlier agent that died before opening a PR or reporting. Its commit
860269286was verified rather than trusted: the diff was re-read line by line, the ablation above was authored and run fresh, and every number here was measured in this session.origin/mainwas merged twice during verification; the final tree is20aa3b0d9.🤖 Generated with Claude Code
https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
Generated by Claude Code