Skip to content

fix(service-automation): persist the terminal run status distinction (cancelled / timed_out survive a restart) - #17008

Merged
yinlianghui merged 3 commits into
mainfrom
claude/issue-15223-persist-terminal-status-distinction
Sep 9, 2026
Merged

fix(service-automation): persist the terminal run status distinction (cancelled / timed_out survive a restart)#17008
yinlianghui merged 3 commits into
mainfrom
claude/issue-15223-persist-terminal-status-distinction

Conversation

@os-trump

@os-trump os-trump commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #15223

Clause-②: yes

The durable run-history row records the terminal status a run actually reached, so a restart no longer changes a run's answer. Gate union reconciled on 0ddabd202.

🛑 The census came back POSITIVE — this may be a priority:p1

Triage wrote the regrade trigger down: 若发现任何消费者(平台内或应用侧)依赖持久的 cancelled / timed_out 做分支,升 p1. It fires. I have applied no label — the seat regrades.

Method. Two passes, joined. (1) Enumerate every read that a durable row can feed — the callers of getRun / listRuns / loadTerminal / listHistory across packages/, apps/, examples/, scripts/ — then read each for a per-terminal-member branch. (2) Grep every 'cancelled' / 'timed_out' literal over the same trees plus the two reachable consuming apps (objectstack-ai/ats, objectstack-ai/hotcrm), and triage all 36 hits by which status vocabulary they belong to.

Positive control (it fires). The method has to find a durable-fed cancelled branch that is already independently measured, or its zero is NOT MEASURED. It finds AutomationEngine.restoreConsumedSuspension's RUN_CANCELLED arm — the one triage measured on stranded-run-status.test.ts — at engine.ts. For the app repos the control is a term known present (sys_automation_run, 2 hits; automation, 8+ files): the grep can return non-zero there, and for a status branch it returns zero.

Result — two platform consumers branch on a persistent cancelled, and the second one was not accounted for.

consumer read path branch consequence of the fold
restoreConsumedSuspension refusal ladder (engine.ts) getRunloadTerminal logged.status === 'cancelled'RUN_CANCELLED the refusal triage already measured — honest, but less than the truth
ApprovalService.inspectStrandedRequestsclassifyStrandedRunState (approval-service.ts) automation.getRun(runId)loadTerminal case 'cancelled': return undefined (SKIP) vs case 'failed': return 'failed' (REPORT) a deliberately cancelled run is reported to an operator as a stranded, unrepairable approval request
releasePendingForTerminalRuns dead-run sweep same flat TERMINAL_RUN_STATUSES set unaffected — the exclusion the card already named, confirmed

The second row is the sharp one, and it is not the sweep the card excluded — that one is releasePendingForTerminalRuns and it treats every terminal state alike. classifyStrandedRunState branches per member, and its own comment states why: "Deliberately terminated by an operator (cancelRun, ADR-0044). The run stopping is the intended outcome … reporting it would bury the real findings under expected ones." On any replica that did not itself see the cancel, the folded row says failed, the skip does not fire, and the row is reported — then handed to the third oracle, which on a foreign replica holds no snapshot and grades it unrepairable.

The strongest single piece of evidence is an existing green test. packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts, "does NOT report a run that was CANCELLED — stopping it was the intent", feeds history: { run_1: { status: 'cancelled' } } and asserts stranded: []. It pins a history-row state that, before this PR, the persistence layer could not produce. It passes only because its fake store does not fold. That is the whole shape of the card: the consumer's pinned contract and the row's real content had diverged, and no gate could see it.

App-side: zero. Both consumers are in-platform.

🛑 Backfill: rows already stored are left as failed, and there is nothing to recover them from

Stated out loud because silence is not an answer. Rows written before this release had their distinction destroyed at write timerecordLog mapped cancelled and timed_out to failed before the row was ever built, and the row carries no other column that discriminates. A backfill would need a source, and there is none: the ring buffer that still knew is per-process and long gone, and nothing else on the row (error, node_id, steps_json) distinguishes an operator's cancellation from a failure — cancelRun's reason lands in the same error column a thrown node writes to.

So: no backfill, no migration, no "undecidable" marking. Marking the old rows undecidable was considered and rejected — it would need a write over the whole history table to replace one wrong answer (failed) with a differently wrong one (unknown), and it would break the two consumers above in the opposite direction: classifyStrandedRunState's default arm skips an unrecognised status, so every genuinely failed historical strand would silently stop being reported. Rows written from this release forward carry the distinction; the ones before it do not, and the PR says so rather than letting a reader assume they heal.

The narrowing question: INHERITED, not deliberate — and now written beside the declaration

RunRecord.status declared two members while recordLog's own terminal predicate admitted four and ExecutionStatus declared them all. Neither file explained it. It was inherited:

  • Nothing was paying for it. The column is a Field.select that stores the string whatever its width — there was no storage cost to buy, which was the hypothesis worth testing.
  • It is simply older than what it had to carry. The durable history row (Automation run observability — follow-ups (retention, discoverable Runs surface, durable run detail) #2585) predates cancelRun (ADR-0044), and timed_out has been in the spec vocabulary the whole time.
  • Neither the write fold nor the read fold carried a comment, an issue reference or a test asserting the narrowing as intended — the fold is spelled as a defensive coercion at both ends, which is what an inherited shape looks like.

The reason is now recorded at the declaration itself (RunRecord.status), with a ⛔ against re-narrowing it to make a downstream switch exhaustive.

What changed

  • One vocabulary. TERMINAL_RUN_STATUSES / TerminalRunStatus / isTerminalRunStatus (engine.ts). Three sites had a private copy of the list — the writer's predicate, the store's row gate, listHistory's filter — and a fourth lives in the object schema.
  • Write side. recordLog resolves the status once into the const that also decides whether a row is written, and stores it. The fold is gone; a cast that would let it back is impossible by construction.
  • Read side. loadTerminal / listHistory resolve the row's status in the gate that already decides whether the row is terminal, and hand the member to deserializeTerminal, which no longer re-reads or folds it — and has no unreachable fallback pretending to guard an arm that cannot occur.
  • listHistory's filter. The second two-member copy. Left alone, widening the writer would have replaced a wrong status with a missing row — cancelled runs would have vanished from the Runs list entirely.
  • Stored column. sys_automation_run.status accepts cancelled / timed_out, and lifecycle.retention.onlyWhen counts them as terminal — a widened writer over a two-member sweep scope would have left those rows never ageing out, on a table whose whole retention posture (ADR-0057) is that history is telemetry.
  • refused deliberately NOT added. ExecutionStatus declares it (A flow cannot REFUSE with per-record text: the only channel that interpolates is a screen description, and a message-only screen still renders Submit and toasts "completed" #14945) but no engine path produces it, and no recordLog terminal arm admits it. An option nothing can write is declared-but-inert metadata (ADR-0078). Noted below rather than fixed here.

Verification

The restart pin (suspended-run-store.test.ts, "the persisted terminal status distinction (#15223)"). Every assertion reads through a second store over the same rows, because the defect is invisible in-process: getRun prefers the ring entry and has always said cancelled. Three tests: a cancelled run read from a process that never saw the cancel (getRun, listRuns, and the wire's ?status= filter, #7359); all four members round-tripping through the row and surviving listHistory's gate; and the ladder reading below.

Ablation — the two folds are independent, and each is proven separately. Anchor counts and git hash-object vs the HEAD blob on both legs; restored under trap … EXIT INT TERM with git checkout HEAD -- ABSPATH, proven by blob equality and an empty git diff HEAD. The suite resolves ./engine.js in-package, so vitest reads src/ — no dist/ sits between the mutation and the assertion.

leg mutation pin verdict
write-side status: terminalStatusentry.status === 'completed' ? 'completed' : 'failed' RED · 2 failed / 1 passed — expected 'failed' to be 'cancelled', expected 'NO_CONSUMED_SUSPENSION' to be 'RUN_CANCELLED'
read-side status (the parameter) → row.status === 'failed' ? 'failed' : 'completed' RED · 3 failed — expected 'completed' to be 'cancelled' ×2, expected 'RUN_COMPLETED' to be 'RUN_CANCELLED'

The store-level round-trip stays GREEN under the write leg and turns RED under the read leg: the two sites really are independent, and a fix to either alone would have masked the other. The read leg also shows the read fold's own signature — it makes the ladder answer RUN_COMPLETED for a cancelled run, which is worse than today's NO_CONSUMED_SUSPENSION.

Reverse validation of the cross-package type change (proves the dependent reads the rebuilt .d.ts, not a cache): a probe in plugin-approvals assigning RunRecord['status'] to the four-member union compiles GREEN; the same probe assigning to the OLD 'completed' | 'failed' is RED with TS2322: Type '"completed" | "failed" | "cancelled" | "timed_out"' is not assignable to type '"completed" | "failed"'. Probe deleted, git status --porcelain empty. No out-of-package source names RunRecord or TerminalRunStatus today.

Runs.

  • pnpm --filter @objectstack/service-automation test1496 passed / 127 files (1493 before; +3).
  • pnpm --filter @objectstack/service-automation typecheck — green, including the test layer (check:test-typecheck: OK — 0 file(s) / 0 error(s)).
  • Downstream consumer sweep, dependents direction: turbo run build --filter='...@objectstack/service-automation'68 successful, 68 total.
  • Derived gate union: scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack → 58 families, all run, all green. --ran reconciliation: ✓ 58 derived famil(ies) accounted for — 58 run, 0 NOT-MEASURED.
  • Two families first returned exit 3 PREREQUISITE NOT MET and were re-run to a real reading, never reported as passes: check:dual-build-cjs-loads (built the five packages it named, then green — 104 require entry points across 67 packages) and check:type-check-debt (OOM under a 4096 MB ceiling; re-run at 9216 MB, green — 5 ledger entries re-measured, 55 raw errors, none above its recorded number).
  • check:route-envelope (dispatch-gates: a whole-tree-walk gate whose workflow names: lists only its CURRENT members is placed Silent, so it is never derived for the card that adds a new member — measured on check:route-envelope / PR #16730 #16828, outside the derived union) is not owed: no file in this diff writes c.json(…) or res.json(…) — measured, 0 hits across all six.
  • Repo-wide pnpm lint (eslint . --no-inline-config) ran in full and passed. No narrowing to declare.
  • check:adr-0087-registration first went RED on this changeset: the body explained why no breaking-change banner is owed and spelled the token to say so, which its detector reads as the declaration itself (token-based, blind to the negation — the closing-keyword trap in another costume). Reworded to state the same conclusion without the token; green.

The reading triage asked for: does the ladder now answer RUN_CANCELLED on a foreign replica?

Half yes, and the half that did not move is worth naming. ⛔ The ladder is unchanged by this PR — triage ruled it honest and the row the defect.

  • A replica with no ring entry for the run (a genuinely restarted process, or after an eviction): YES. It now answers RUN_CANCELLED, pinned in the new test. Before: NO_CONSUMED_SUSPENSION.
  • A replica holding its own stale failed ring entry (the shape stranded-run-status.test.ts measures): still NO_CONSUMED_SUSPENSION. The ladder tests cancelled against getRun, which is ring-first, while it consults the durable row for completed alone. The row can support the real answer now; that arm does not read it. Recorded in the test's comment so the assertion is not mistaken for a fix — the reason it holds has narrowed from two to one.

Dispositions

  • ADR-0087 migration entry: not owed. ADR-0087 governs authorable metadata shapes on sys_metadata and the conversion chain replayed at rehydration. sys_automation_run is an engine-owned system data table with no authorable surface, objectstack migrate meta has nothing to rewrite for it, and every value already stored stays valid under the widened option set. No column added, no type changed.
  • **BREAKING** banner: not owed. The published contract — IAutomationService.getRun / listRuns return ExecutionLog, whose status is ExecutionStatus — has declared all four members since before this row existed. The implementation stops under-reporting one the contract already promised; a consumer written against the declared contract is unaffected. The in-repo readers that would have been narrowed by the widening (listHistory, the retention scope, the row gate) are all widened in this same change.
  • Changeset: @objectstack/service-automation: minor — an additive widening of a published package's surface, and the level the clause-② declaration requires.
  • Single-writer: the fix needed no packages/spec/src/** path, so the fix(analytics): ask the object-level read grant before serving an inline dataset — one admission verdict on every driver #16860 collision the dispatch fenced did not arise. ExecutionStatus already declared everything needed.

验收备注 (acceptance notes — observations, not filed)


🤖 Generated with Claude Code

https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37


Generated by Claude Code

…our members

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
`RunRecord.status` declared two members while `recordLog`'s terminal
predicate admitted four and `ExecutionStatus` declares them all. Both ends
of the durable store folded to match the narrower declaration, so a
cancelled or timed-out run's distinction was destroyed at write time: the
same run read `cancelled` in-process and `failed` after a restart.

- write side: `recordLog` records the status its own terminal predicate
  admitted, resolved once through the newly declared `TERMINAL_RUN_STATUSES`
- read side: the row's status is resolved once in the terminal gate and
  handed to `deserializeTerminal`, which no longer folds it; `listHistory`'s
  second copy of the two-member list now asks the same predicate
- stored column: `sys_automation_run.status` options and the retention
  `onlyWhen` scope carry all four terminal members
- pins: the distinction survives a fresh store over the same rows, on
  `getRun`, `listRuns` (with its `?status=` filter) and `listHistory`

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
…ation token

The body explained why no breaking-change banner is owed, and spelled the
token to say so — which `check:adr-0087-registration` reads as the
declaration itself (its detector is token-based and blind to the negation,
the same shape as the closing-keyword trap). The reasoning is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

13 anchor(s) derived from 1 changed package(s); no hand-written page names any of them. ⚠️ 1 changed file(s) yielded no anchor (packages/services/service-automation/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/index.ts) — pages documenting those are invisible to this run
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 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; 100 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 — 6 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 dd2fd2003485df584092d3a35d774d3543c70686packageMentionDocs.

Which tree this was computed on

This run read content/docs from 745a37e065652fc69c6a0bf0a08cc82b09871d81 — the merge of head 0ddabd202d6273ac02c45ebc232215794316c9e5 into base dd2fd2003485df584092d3a35d774d3543c70686, 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 745a37e065652fc69c6a0bf0a08cc82b09871d81 && git checkout 745a37e065652fc69c6a0bf0a08cc82b09871d81
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin dd2fd2003485df584092d3a35d774d3543c70686 0ddabd202d6273ac02c45ebc232215794316c9e5 && git checkout -B drift-repro dd2fd2003485df584092d3a35d774d3543c70686 && git merge --no-ff 0ddabd202d6273ac02c45ebc232215794316c9e5

node scripts/docs-audit/affected-docs.mjs --json dd2fd2003485df584092d3a35d774d3543c70686

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

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 9, 2026
@yinlianghui
yinlianghui marked this pull request as ready for review September 9, 2026 02:38
@yinlianghui
yinlianghui added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit 775e5ec Sep 9, 2026
41 of 42 checks passed
@yinlianghui
yinlianghui deleted the claude/issue-15223-persist-terminal-status-distinction branch September 9, 2026 03:04
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

3 participants