Skip to content

fix(service-automation): a restored suspension carries the pause, not the failed attempt - #16396

Merged
os-warren merged 2 commits into
mainfrom
claude/issue-15660-suspend-snapshot-aliasing
Sep 6, 2026
Merged

fix(service-automation): a restored suspension carries the pause, not the failed attempt#16396
os-warren merged 2 commits into
mainfrom
claude/issue-15660-suspend-snapshot-aliasing

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #15660

The card was filed as a reading, not a measurement, and its first job was to decide whether it reproduces at all. It reproduces — but not by the mechanism the card named. Both halves are below.

It reproduces

Driven end to end against the real engine, exactly the path the card named as the only exposure (the #13937 consumed-suspension restore):

reading value
the durable row at the pause started: 1
what restoreConsumedSuspension put back started: 99
what the node was handed on the repair resume started: 99

An operator repairing a stranded run got a snapshot stamped as "the pause" that carried state written by the attempt that failed.

The mechanism is NOT the one the card named

The card attributed the aliasing to "the in-memory suspended-run store keeps the object by identity rather than serialising it". It does not. InMemorySuspendedRunStore JSON round-trips on both save and load — pinned in this PR, because the next reader should not re-derive a fix from a premise that is false.

The aliasing is minted on the resume, not at the suspend. resumeInternal rebuilds the flow scope as new Map(Object.entries(run.variables)): that copies the keys and shares every value object with the parked snapshot. journalConsumedSuspension then shallow-copies run and journals it as the pause "verbatim" — a guarantee it delivered for top-level keys only. So an executor that keeps state in the scope and updates it in place writes straight through into the snapshot the operator exit hands back.

map is the concrete instance (nodeId.$mapState) and, by a census re-derived here rather than recalled, still the only executor in-repo that does this.

Both proposed fix placements are refuted by that same measurement

The card offered three candidates and asked that the choice be made after the repro, with cost measured. Two of them are refuted:

  • "deep copy at snapshot time" — with a store configured the snapshot already is a private deep copy before a single downstream node runs, and it reproduced anyway. It cannot fix a link that is minted later.
  • "copy at restore time" — the node mutates and then throws, so the journal is already corrupt when it is written. Anything copied at or after that point copies the corruption.
  • "node state is never mutated in place" — the remaining candidate, and it is a convention the engine cannot verify. A third-party executor reintroduces the defect silently, and the failure mode is an operator told a run is repairable and handed a snapshot that misstates its progress. Rejected for that reason, not on cost.

The copy has to be taken before the failed attempt runs. That is the line that already captures stepCountAtPause, for the same reason, so the fix sits beside the code that had the same problem and solved it.

The cost the card flagged NOT MEASURED

scope one clone
typical (8 node outputs) 5.8 us
map mid-run (100 results) 32 us
map worst case (10,000 results = MAX_MAP_ITEMS) 3.3 ms

Measured against what the same path already spends: one suspend+resume round trip against a configured store already performs four full clones of this same payload (save x1, load x2, recordTerminal x1 — counted, not read). This adds a fifth, on a path whose production cost is a durable round trip. The existing objection to a JSON clone was that it would run inside a catch arm already handling a failure, where a circular value would throw the operator's repair away; on the happy path it is just a value we decline to copy, and the fallback is the previous behaviour with a warning.

Verification

git rev-parse --short HEAD = d3ab067 — every result below is from that tree.

  • Ablation, red before green. With the fix reverted to origin/main (mutation confirmed on disk: blob hash equals the origin/main blob, marker count 3 to 0), the new pin is 2 failed | 27 passed; both control arms and all 24 tests of the existing consumed-suspension-restore.test.ts stay green. With the fix: 29 passed. Restore proved by an empty git diff HEAD and disk hash equal to the HEAD blob.
  • Controls in the file, not in a scratch buffer. Each in-place arm is paired with a replace-and-set arm that must stay green. Without them, a harness that reported the right value for a trivial reason would read exactly like a fixed engine.
  • Package suite: 1431 passed (122 files). Typecheck: green, and both changed files confirmed present in the tsc program via --listFiles (1 hit each) rather than assumed.
  • Gate family derived mechanically on this head — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands — and all 56 run. Two returned exit 3 PREREQUISITE NOT MET (check:dual-build-cjs-loads, check:type-check-debt); both were re-run after turbo run build and returned real greens.

Scope

No contract surface: no packages/spec/**, no exported type or signature changed. The resume ordering is untouched, and a running flow sees exactly the scope it saw before — only what gets journalled changes.

Not folded in, per the card's boundaries: #15616 / PR #15648 (the map state lifetime) and #15646 (the runRegion suspend-arm residue) remain open and are not addressed here.


🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

…d attempt's

`resumeInternal` rebuilt the flow scope as `new Map(Object.entries(
run.variables))` — keys copied, value objects SHARED — and
`journalConsumedSuspension` then shallow-copied `run` and journalled it as the
pause "VERBATIM". An executor that keeps state in the scope and updates it in
place (`map`'s `<nodeId>.$mapState`) therefore wrote through into the snapshot
`restoreConsumedSuspension` hands an operator, so the exit re-armed a pause
carrying state that belonged to the failed attempt.

Measured: the durable row held `started: 1` at the pause and the restore put
back `started: 99`.

The copy is taken before the failed attempt runs — the same line that already
captures `stepCountAtPause`, for the same reason. No later placement works: the
node mutates and THEN throws, so a copy taken at journal or restore time copies
the corruption.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 4 documentable anchor(s).

2 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx (via AutomationEngine (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 4998efa71773154561c471075f4ef12566ecc455packageMentionDocs.

Which tree this was computed on

This run read content/docs from 97670b10f2693ea2b8738d9019ba5c44a4cd2b25 — the merge of head d3ab0677673554881ad1a7742f021de4ec30a8ae into base 4998efa71773154561c471075f4ef12566ecc455, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 97670b10f2693ea2b8738d9019ba5c44a4cd2b25 && git checkout 97670b10f2693ea2b8738d9019ba5c44a4cd2b25
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4998efa71773154561c471075f4ef12566ecc455 d3ab0677673554881ad1a7742f021de4ec30a8ae && git checkout -B drift-repro 4998efa71773154561c471075f4ef12566ecc455 && git merge --no-ff d3ab0677673554881ad1a7742f021de4ec30a8ae

node scripts/docs-audit/affected-docs.mjs --json 4998efa71773154561c471075f4ef12566ecc455

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 4998efa71773154561c471075f4ef12566ecc455 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants