fix(service-automation): a completed run is no longer answered failed — or re-executed under strategy: retry — by its own history write - #17565

Fixes #16274

A completion-path recordLog({ status: 'completed' }) was called from inside the try whose catch exists for node failures, on both initial-execution paths. So a throw out of a history write, on a run that had already finished, was handled as though a node had thrown. This is the guard PR #16273 landed on resumeInternal's completion path, on the two sites that card deliberately scoped out.

⭐ The measurement came first, and it moved the card's grade

Triage attached a re-grade trigger and asked for it to be driven before any code was written — 「若它能重试一个已经完成的 run，本卡立刻从 p3 变成 p2 甚至更高」. It was driven on origin/main (9540590ba) with the doubles PR #16273 already shipped, and it can. Posted on the card first: #16274 (comment)

M3 against M5 and M6 is the finding. Same flow, same maxRetries: 2, same counter: healthy sinks run the node once; a genuine node failure runs it three times (retry working correctly); and a run whose every node SUCCEEDED also runs three times — because the false failed is handed to retryExecution by the strategy branch, and that loop reads result.success. Three failed rows for one logical success, the node's side effects three times, unattended, inside one execute() call. It needs no store at all: the run-summary line is on by default (this.runSummaryLog = options?.runSummaryLog ?? 'info') and calls a host-injected Logger.

⇒ That is the double run #15944 was graded sharp for, and on one axis sharper: #15944's re-run needed an operator to restore and resume. ⛔ The priority:p3 -> priority:p2 flip is the triage/PM seat's to apply, not this seat's.

The two questions the card left unmeasured, both answered. The throw reaches the generic failure arm at both sites: the thrown value is a plain Error, so isSuspendSignal(err) is false and err instanceof InputSchemaViolationError is false — and the envelope proves both negatives, carrying status: 'failed' with NO code where the input-schema arm returns code: 'FLOW_INPUT_SCHEMA_INVALID' and the suspend arm returns status: 'paused'. And nothing downstream distinguishes it from a genuine node failure: compare M3 with M6 field by field — identical shape, identical discriminator, identical HTTP arm. The only difference is the content of error, which is a message regex away from being readable and therefore not a distinction (PD #12).

⚠️ Three completion sites in the tree; the card named two

Re-located by structure (the card's line numbers were from 48427620a and it said to), on origin/main at 9540590ba:

The card's "the retry path" site is executeWithoutRetry(), not retryExecution() (:9851): the latter holds the loop and the backoff and calls the former once per attempt, which is where that attempt's completion write and its node-failure catch live. ⚠️ Fixing execute() alone would not have closed it — a flow under strategy: 'retry' whose attempt 2 completes leaves through executeWithoutRetry()'s exit and never crosses execute()'s again.

What changed

1. fix(service-automation) — each completion-path recordLog is guarded at its own call site, the summary is recomputed with the same pure function (logged?.summary ?? summarizeRun(steps)), and the swallowed failure is reported once at error with its consequence and fix. The precedent's shape, twice; ⛔ no third design. ⛔ No catch arm's meaning is widened: a genuine node failure still lands in the node-failure arm, still records failed, still carries the node's own text, and is still retried the full 1 + maxRetries times — pinned by two controls.
2. chore(gates) ×2 — AGENTS.md's rule for check:durability-log-level is "it cannot discover a new seam, only stop known ones from regressing; found a new one, add it to DURABILITY_CRITICAL_CALLEES in the same PR that fixes it." recordLog is that seam. Measured before landing it: the population goes 29 -> 33 catch seams (this card's two guards plus the two the resume path already landed), all four judged LOUD, gate green, zero unrecognised constructs, the shrink-only baseline still empty. The paired by-value copy in measure-durability-swallow-family.mjs then reddened on drift exactly as designed (declared by the gate, missing from the copy: recordLog) and was carried over, its own remedy text followed.

⚠️ The level question this lane carries, and which side of #13398 this site is on

The card's suggested remedy says report at error, and both precedents (#16273, #15555) landed that level — but this lane carries the standing ruling that a site reporting through a published sink shape must not be raised to error. Established, not assumed:

• the ruling's own criterion, as the landed [Decision] plugin-sharing's refused-backfill report lands at warn where AGENTS.md puts it at error — and the card that was supposed to carry the level is CLOSED #13398-class sites state it, is that what is forbidden is raising a site to error where doing so means GROWING error? onto a published sink that lacks it (its option B). A sink that already declares error widens nothing — share-link-service.ts says so in those words and takes error on that basis;
• this site's sink is Logger from @objectstack/spec/contracts, where error(message, error?, meta?) is a REQUIRED member. Only fatal?, child? and withTrace? are optional. Nothing is grown, no warn fallback branch is owed, and no sink type changes in this diff;
• corroborated mechanically: pnpm check:optional-error-sink (exit 0) censuses 50 sink types declaring error under packages/**, of which 14 declare it REQUIRED — "nothing to guarantee". Logger is in that 14, which is also why the two precedents on this same sink could land error.

⇒ This site is OUTSIDE #13398's class, so the precedent's shape AND its level are both copied. Reported here rather than resolved silently, as the dispatch required; it is a reading, not a judgement call.

Clause-②: no

Both limbs measured against the diff, not recalled. ① Accept/reject behaviour — no Zod schema, parse path or accept set is touched; AutomationResult.status already declares 'completed' | 'paused' | 'failed' and this change emits no new value, it emits the ABSENT status + success: true that the healthy completion path already returns. ② Public surface — no exported symbol, no new error code, no new key on any published payload; the change is a try/catch around an existing call plus one logger.error, and summarizeRun was already imported in the file. The two gate-script edits are repo tooling, not published (files[] is ["dist", "README.md", "CHANGELOG.md"]). This matches the seat's claim-time declaration.

Verification

RED before GREEN, proven by on-disk state. The fix was committed first, then ablated out of committed state (git checkout 9540590ba -- packages/services/service-automation/src/engine.ts), with the mutation proven by hash and by occurrence counts on the exact text touched, and restored by state afterwards:

HEAD blob       : 8223765305ab4f72b82329c3d1611d9a9addf46c
on-disk BEFORE  : 8223765305ab4f72b82329c3d1611d9a9addf46c   (equal -> tree really was at HEAD)
counts BEFORE   : catch (bookkeeping)=4   marker [#16274]=4   summary recompute=4
on-disk AFTER   : 7f77362b8733e494107e2f31ca809c73a54d6cba   (= the pre-fix blob; != BEFORE -> not a no-op)
counts AFTER    : catch (bookkeeping)=2   marker [#16274]=0   summary recompute=2
restore         : git checkout HEAD -- THE FILE ; `git diff HEAD` EMPTY, `git status --porcelain` EMPTY,
                  on-disk hash back to 8223765305ab4f72b82329c3d1611d9a9addf46c

The ablation script carried trap restore EXIT INT TERM with an absolute git rev-parse --show-toplevel path, and an empty hash was treated as FAILURE rather than as "nothing to compare".

The direction OBSERVED (not the one predicted): 5 pins failed, 3 controls passed — the controls passing on the ablated tree is correct, since they assert behaviour this change does not move. The two readings that matter, quoted from the run:

PIN 3  AssertionError: a completed run is NEVER re-attempted: expected 3 to be 1
PIN 1  AssertionError: a history write must never break the run that produced it: expected 'threw' to be 'returned'

⚠️ No dist/ could have served stale bytes on that leg: packages/services/service-automation/dist did not exist at ablation time (checked and printed by the script — dist present? : NO; only the dependency closure had been built), and the test resolves ./engine.js relatively to the TypeScript source rather than through the package's exports. The mutation changing the verdict is itself the proof that source is what ran.

Runs — every heavy run through scripts/pm/os-verify-lock.sh, quoting its own verdict line:

Gates — derived from the actual diff with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, re-derived after the scripts/** edits (61 commands -> 82), and reconciled. All exit 0, with two earned the hard way:

• pnpm check:dual-build-cjs-loads first returned exit 3 = PREREQUISITE NOT MET = NOT MEASURED ("this gate reads built output, and some package has no dist/"). ⛔ Not rounded up: the prerequisite was satisfied with the CI-shaped build above and the gate re-run to a real exit 0.
• pnpm check:type-check-debt likewise: exit 3 -> prerequisite built -> exit 0 (5 ledger entries re-measured, 55 raw tsc errors, none above its recorded number).
• pnpm check:swallow-census-controls returned a genuine exit 1 on the drift described above, and is exit 0 after the paired copy was carried over.
• The four artifact-roster families whose roster sits in a directory one of these paths is in — where the derivation says its silence is evidence in neither direction — were read and run anyway: check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity, all exit 0. So were the gate script's own suites (--self-test: 63 + 57 cases) and the siblings that read it (check:ratchet-remedy-authority, check:cross-package-test-inputs, check-scripts-symbol-anchors, check-declaration-mirrors, check-self-test-wired).

NOT MEASURED, each with its reason — all of them CI-owned, none of them a skipped local obligation: the 5 families whose argv takes a value only a CI run supplies (check-shard-attestation, check-test-completeness); the 5 CI jobs scheduled by these paths whose 27 steps have no local invocation (Test Core shards, Temporal Conformance on live PG/MySQL, the two Dogfood jobs, Build Core); and the 11 families the derivation marks as declaring a population too wide to place — of which the two that actually bear on this diff were run anyway (check:optional-error-sink, check:durability-log-level), the rest being unrelated roots.

Out-of-scope finding — reported, ⛔ not fixed here

Filed as #17562, bare, with a reproduction and a firing control. The failure-arm recordLog({ status: 'failed' }) on these same two paths is unguarded, so on the tree this PR produces, a genuine node failure plus a synchronously throwing store makes execute() reject instead of answering the declared envelope:

store = SYNC-THROW        -> {"kind":"threw","error":"run-history driver refused the terminal row"}
store = HEALTHY (control) -> {"kind":"returned","status":"failed","error":"work blew up"}

A different site with a different consequence (the run really did fail, so nothing misleads an operator — what is lost is the SHAPE), which is why it is a separate card and is deliberately not pinned here: freezing a known defect in this PR's test file would be worse than naming it. #17562 is not addressed by this PR.

⛔ Untouched, as the card and the dispatch both fence: resumeInternal's already-guarded site, restoreConsumedSuspension, inspectStrandedRequests (#15358), suspended-run-store.ts and sys-automation-run.object.ts (held by the #15336 dev in flight), and packages/spec/**.

Authored by Claude Code in session session_01ToDPcx9AESFubJkDiFMtKW — attribution in prose deliberately, because a PR body's trailing rule line plus footer block is eaten by the platform on write while the call still reports success.

Generated by Claude Code

Generated by Claude Code