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

.changeset/persist-terminal-run-status-distinction.md

@@ -0,0 +1,15 @@
+---
+"@objectstack/service-automation": minor
+---
+
+A run's durable history row records the terminal status the run actually reached — `completed`, `failed`, `cancelled` or `timed_out` — instead of folding all four into two. A restart no longer changes a run's answer.
+
+`RunRecord.status` declared two members (`'completed' | 'failed'`) while `AutomationEngine.recordLog`'s own terminal predicate admitted four and `ExecutionStatus` (`@objectstack/spec`) has declared them all along. Both ends of the store folded to match the narrower declaration: the write mapped everything that was not `completed` to `failed`, and the read mapped everything that was not `failed` back to `completed`. The distinction was therefore not hidden — it was **destroyed at write time**, so no later change could recover it for a row already stored. The cost was that one run answered differently depending on where you read it: `getRun` prefers the in-memory ring entry and said `cancelled`, while after a restart or a ring-buffer eviction the durable row answered, and it said `failed`.
+
+- **The write side.** `recordLog` writes the status its own terminal predicate admitted, resolved once into a `const` that also decides whether a row is written at all. The predicate is now the single declared vocabulary, `TERMINAL_RUN_STATUSES` (`engine.ts`) — three sites had a copy of that list and only one of them was ever going to be updated together with the writer.
+- **The read side.** `ObjectStoreSuspendedRunStore` resolves the row's status once in the gate that already decided whether the row is terminal at all and hands the member to `deserializeTerminal`, which no longer re-reads or folds it. `listHistory`'s filter was the second copy of the two-member list — left alone it would have replaced a wrong status with a *missing row*, dropping cancelled runs out of the Runs list entirely.
+- **The stored column.** `sys_automation_run.status` accepts the two added members, and the retention scope (`lifecycle.retention.onlyWhen`) counts them as terminal — a widened writer over a two-member sweep scope would have left `cancelled` and `timed_out` history rows never ageing out, on a table whose whole retention posture (ADR-0057) is that history is telemetry. `refused` is deliberately not added: `ExecutionStatus` declares it (#14945) but no engine path produces it, and an option nothing can write is declared-but-inert metadata (ADR-0078).
+- **Rows already stored keep reading `failed`.** The information they lost is not recoverable and this change does not pretend otherwise — there is no backfill, because there is nothing to backfill *from*. Rows written from this release forward carry the distinction.
+- **`TerminalRunStatus`** is exported for the same reason `ConsumedSuspensionDropNotice` is: `RunRecord` is barrel-reachable, and a host store implementing `recordTerminal` / `loadTerminal` has to be able to name the field it round-trips.
+
+Not a breaking change, and deliberately carries no breaking-change banner: the published contract (`IAutomationService.getRun` / `listRuns` return `ExecutionLog`, whose `status` is `ExecutionStatus`) has declared all four members since before this row existed. What changes is that the implementation stops under-reporting one the contract already promised — a consumer written against the declared contract is unaffected. Also no ADR-0087 migration entry: that ADR governs authorable metadata shapes on `sys_metadata`, and this is an engine-owned system data table whose existing values stay valid under the widened option set.

packages/services/service-automation/src/engine.ts

@@ -1213,11 +1213,58 @@ export interface SuspendedRun {
  * A terminal run summary persisted as durable run history (completed / failed)
  * for the "Runs" observability surface — distinct from a live {@link SuspendedRun}.
  */
+/**
+ * [#15223] The terminal states a run can be RECORDED in — the durable
+ * run-history vocabulary, declared ONCE because three sites have to agree on
+ * it: the writer ({@link AutomationEngine.recordLog}'s terminal predicate),
+ * the reader (`ObjectStoreSuspendedRunStore`'s row gate) and the stored
+ * column (`sys_automation_run.status`, whose `Field.select` options and
+ * retention `onlyWhen` scope enumerate the same four). A second copy of this
+ * list is how a widened writer ends up with rows a reader filters away.
+ *
+ * These are exactly the four `ExecutionStatus` members (`@objectstack/spec`)
+ * that mean "this run has stopped and will not resume". `paused`,
+ * `running`, `pending` and `retrying` are live states with no history row;
+ * `refused` is declared by the spec but no engine path produces it today, so
+ * adding it here would enumerate a value nothing can write.
+ */
+export const TERMINAL_RUN_STATUSES = ['completed', 'failed', 'cancelled', 'timed_out'] as const;
+
+/** One member of {@link TERMINAL_RUN_STATUSES}. */
+export type TerminalRunStatus = (typeof TERMINAL_RUN_STATUSES)[number];
+
+/** Whether `status` is one of {@link TERMINAL_RUN_STATUSES}. */
+export function isTerminalRunStatus(status: unknown): status is TerminalRunStatus {
+    return (TERMINAL_RUN_STATUSES as readonly unknown[]).includes(status);
+}
+
 export interface RunRecord {
     runId: string;
     flowName: string;
     flowVersion?: number;
-    status: 'completed' | 'failed';
+    /**
+     * The terminal state this run reached, as the engine observed it.
+     *
+     * [#15223] This declared `'completed' | 'failed'` — TWO members — while
+     * {@link AutomationEngine.recordLog}'s own terminal predicate admitted
+     * FOUR and `ExecutionStatus` (`@objectstack/spec`) declared them all.
+     * Both ends folded: the write mapped everything that was not `completed`
+     * to `failed`, and the read mapped everything that was not `failed` to
+     * `completed`. The information was not hidden by that, it was DESTROYED
+     * at write time, so the same run read `cancelled` in-process (`getRun`
+     * prefers the ring entry) and `failed` after a restart or a ring
+     * eviction.
+     *
+     * ⛔ The narrowing was INHERITED, not chosen — recorded here so the next
+     * reader does not re-derive it. Nothing was paying for it: the column is a
+     * `Field.select` that stores the string whatever its width, so there was
+     * no storage cost to buy, and neither file stated a reason. It is simply
+     * older than what it had to carry — the durable history row (#2585)
+     * predates `cancelRun` (ADR-0044), and `timed_out` was in the spec's
+     * vocabulary the whole time. ⛔ Do not re-narrow it to make a downstream
+     * `switch` exhaustive; widen the switch.
+     */
+    status: TerminalRunStatus;
     startedAt: string;
     startTime?: number;
     /** When the run reached its terminal state. */
@@ -4190,7 +4237,11 @@ export class AutomationEngine implements IAutomationService {
             id: r.runId,
             flowName: r.flowName,
             flowVersion: r.flowVersion,
-            status: r.status, // 'completed' | 'failed' — both valid ExecutionLog statuses
+            // [#15223] All four {@link TERMINAL_RUN_STATUSES} members, each a
+            // valid `ExecutionLog` status — the schema has declared the whole
+            // vocabulary since before this row existed, and it was the
+            // persistence layer, not the contract, that reported only two.
+            status: r.status,
             startedAt: r.startedAt,
             completedAt: r.finishedAt,
             durationMs: r.durationMs,
@@ -7374,11 +7425,13 @@ export class AutomationEngine implements IAutomationService {
         // store so "did it run / fail, and why?" survives a restart and the
         // in-memory ring-buffer eviction. Best-effort + fire-and-forget: a
         // history write must NEVER block or break the run that produced it.
-        const terminal =
-            entry.status === 'completed' ||
-            entry.status === 'failed' ||
-            entry.status === 'cancelled' ||
-            entry.status === 'timed_out';
+        // [#15223] ONE vocabulary, not a fourth copy of the list: this
+        // predicate decides both WHETHER a history row is written and WHAT its
+        // `status` says. Keeping the narrowed value in a `const` is what makes
+        // the record below type-check without a cast — and a cast is precisely
+        // how the fold this card is about survived four members for two.
+        const terminalStatus = isTerminalRunStatus(entry.status) ? entry.status : undefined;
+        const terminal = terminalStatus !== undefined;
 
         // The MVP of #4354, and the half that needs no console: one structured
         // line per terminal run. `selected=30 acted=0` in a log file is the
@@ -7413,13 +7466,20 @@ export class AutomationEngine implements IAutomationService {
             else this.logger.info(line, meta);
         }
 
-        if (terminal && this.store?.recordTerminal) {
+        if (terminalStatus && this.store?.recordTerminal) {
             const lastStep = entry.steps[entry.steps.length - 1];
             const record: RunRecord = {
                 runId: entry.id,
                 flowName: entry.flowName,
                 flowVersion: entry.flowVersion,
-                status: entry.status === 'completed' ? 'completed' : 'failed',
+                // [#15223] The status the run actually reached. This used to be
+                // `entry.status === 'completed' ? 'completed' : 'failed'` — a
+                // fold applied at WRITE time, so a cancelled or timed-out run's
+                // distinction was not merely unshown, it was never stored and
+                // could not be recovered afterwards. ⛔ Never re-introduce a
+                // conditional here: whatever the terminal predicate above
+                // admits is what the row must carry.
+                status: terminalStatus,
                 startedAt: entry.startedAt,
                 finishedAt: entry.completedAt,
                 durationMs: entry.durationMs,

packages/services/service-automation/src/index.ts

@@ -58,6 +58,12 @@ export type {
     // host store implementing `recordTerminal` / `loadTerminal` writes and
     // reads; unnameable, the field would be writable only by structural luck.
     ConsumedSuspensionDropNotice,
+    // [#15223] The type of `RunRecord.status`, exported for exactly the reason
+    // above: `RunRecord` is barrel-reachable and a host store implementing
+    // `recordTerminal` / `loadTerminal` has to name the field it round-trips.
+    // It is also the set to switch over — a terminal run's four states exist
+    // precisely to be told apart, which is the whole of what this card fixed.
+    TerminalRunStatus,
     // [#15358] The read-only repairability verdict
     // (`AutomationEngine.inspectConsumedSuspension`), for the same reason as
     // `SuspensionRestoreResult` above: the method is barrel-reachable, so a

packages/services/service-automation/src/stranded-run-status.test.ts

@@ -38,7 +38,10 @@
  *  6. **The recorded `ExecutionStatus` stays `failed`** — the ruling widened
  *     the RESULT vocabulary; the run-row vocabulary is `@objectstack/spec`'s
  *     (`automation/execution.zod.ts`) and is untouched, in the log and in the
- *     durable history row.
+ *     durable history row. [#15223] Still true of a STRANDED run, which is
+ *     recorded `failed`. What changed under this file is a different run: a
+ *     CANCELLED one, whose durable row used to be folded to `failed` on the
+ *     way in and now carries `cancelled`. See case 4's second test.
  */
 
 import { describe, it, expect } from 'vitest';
@@ -334,10 +337,23 @@ describe('#13937 — a re-armed run is not double-runnable', () => {
 
         const stale = await a.restoreConsumedSuspension(runId);
         expect(stale.restored).toBe(false);
-        // A's own log still says `failed` for this run and the durable row
-        // records a cancelled run as `failed` too, so A cannot name the
-        // cancellation — what it CAN say, honestly, is that no snapshot is
-        // held any more. ⛔ Never `RUN_SUSPENDED`, and never `restored: true`.
+        // A's own log still says `failed` for this run, and `getRun` prefers
+        // the ring entry — so A cannot name the cancellation and says,
+        // honestly, that no snapshot is held any more. ⛔ Never
+        // `RUN_SUSPENDED`, and never `restored: true`.
+        //
+        // [#15223] ⚠️ The REASON narrowed here, and the assertion is kept to
+        // pin the half that did not move. It used to hold for two reasons —
+        // A's stale ring entry AND a durable row that recorded every
+        // cancellation as `failed`. The row carries `cancelled` now
+        // (`suspended-run-store.test.ts`, "the persisted terminal status
+        // distinction"), and a replica with NO ring entry for this run answers
+        // `RUN_CANCELLED` from it. What still produces `NO_CONSUMED_SUSPENSION`
+        // is only A's own stale hot copy shadowing the row: the ladder tests
+        // `cancelled` against `getRun` (ring first) while consulting the
+        // durable row for `completed` alone. ⛔ Deliberately NOT changed by
+        // #15223 — triage ruled the ladder honest and the row the defect; this
+        // is the measured residue, recorded so it is not mistaken for a fix.
         expect(stale.refusal).toBe('NO_CONSUMED_SUSPENSION');
         expect(await store.list()).toHaveLength(0);
         expect((await a.resume(runId)).code).toBe('RUN_NOT_FOUND');

packages/services/service-automation/src/suspended-run-store.test.ts

@@ -1051,3 +1051,116 @@ describe('#14333 ObjectStoreSuspendedRunStore.claimSuspension — the production
         expect(dataEngine.rows.get(runId).node_id).toBe('lv1');
     });
 });
+
+/**
+ * #15223 — the persisted terminal-status distinction.
+ *
+ * `RunRecord.status` declared two members (`'completed' | 'failed'`) while
+ * `recordLog`'s own terminal predicate admitted four and `ExecutionStatus`
+ * (`@objectstack/spec`) declared them all. Both ends of this store folded to
+ * match the narrower declaration — the write mapped everything that was not
+ * `completed` to `failed`, the read mapped everything that was not `failed`
+ * to `completed` — so a cancelled run's distinction was not merely unshown:
+ * it was **destroyed at write time**, and no later fix could recover it for a
+ * row already stored.
+ *
+ * ⭐ The property, and the reason every assertion below reads through a
+ * SECOND store over the same rows: the defect is invisible in-process.
+ * `getRun` prefers the in-memory ring entry, which has always said
+ * `cancelled`; only a restart (or a ring eviction) makes the row answer. An
+ * in-process assertion cannot see this and would have stayed green throughout.
+ *
+ * The three surfaces a restart moved, pinned here together because the fold
+ * lived at one site and surfaced at all three: `getRun`, `listRuns` (including
+ * its wire-exposed `?status=` filter, #7359) and `listHistory`.
+ */
+describe('ObjectStoreSuspendedRunStore — the persisted terminal status distinction (#15223)', () => {
+    /** Land the fire-and-forget `recordTerminal` off the terminal `recordLog`. */
+    const settle = () => new Promise((r) => setImmediate(r));
+
+    it('⭐ a CANCELLED run still reads `cancelled` from a process that never saw the cancel', async () => {
+        const table = createFakeEngine();
+        const freshStore = () => new ObjectStoreSuspendedRunStore(table, createTestLogger());
+
+        // Replica A parks the run, then an operator ends it deliberately
+        // (`cancelRun`, ADR-0044).
+        const a = pausableEngine(freshStore());
+        const paused = await a.execute('approval_flow');
+        const runId = paused.runId!;
+        expect(await a.cancelRun(runId, 'submitter withdrew')).toBe(true);
+        await settle();
+
+        // In-process this has always worked — it is the ring entry answering.
+        expect((await a.getRun(runId))?.status).toBe('cancelled');
+
+        // ⭐ The ROW, which is the whole of what a restart leaves behind. This
+        // said `failed` before the write-side fold was removed, and nothing
+        // downstream could have recovered the cancellation from it.
+        expect(table.rows.get(`run_${runId}`)?.status).toBe('cancelled');
+
+        // ⭐ …and a fresh process over the same rows — empty ring, new store —
+        // now answers what the operator actually did, on both read surfaces.
+        const b = pausableEngine(freshStore());
+        expect((await b.getRun(runId))?.status).toBe('cancelled');
+        expect((await b.listRuns('approval_flow')).find(r => r.id === runId)?.status).toBe('cancelled');
+
+        // The wire's `?status=` filter (#7359) reads the same resolved status,
+        // so it stops answering the opposite of the truth: the cancelled run
+        // used to be what `?status=failed` returned and `?status=cancelled`
+        // could not return at all.
+        expect((await b.listRuns('approval_flow', { status: 'cancelled' })).map(r => r.id)).toEqual([runId]);
+        expect(await b.listRuns('approval_flow', { status: 'failed' })).toEqual([]);
+    });
+
+    it('all four terminal members round-trip through the ROW — and none is filtered out of the history', async () => {
+        const table = createFakeEngine();
+        const writer = new ObjectStoreSuspendedRunStore(table, createTestLogger());
+        // `timed_out` has no engine path producing it today, so the store's own
+        // contract is where it can be measured at all — which is exactly why
+        // the vocabulary is declared once and asserted here rather than
+        // inferred from whatever the engine happens to emit.
+        const members = ['completed', 'failed', 'cancelled', 'timed_out'] as const;
+        for (const [i, status] of members.entries()) {
+            await writer.recordTerminal(terminalRecord(i + 1, { status, flowName: 'four_flow' }));
+        }
+
+        // The stored bytes carry the distinction — one row per member.
+        expect(members.map(s => [...table.rows.values()].filter(r => r.status === s).length)).toEqual([1, 1, 1, 1]);
+
+        // A FRESH store over the same rows reads each one back unchanged. The
+        // read-side fold made this collapse to `completed` for two of them.
+        const reader = new ObjectStoreSuspendedRunStore(table, createTestLogger());
+        for (const [i, status] of members.entries()) {
+            expect((await reader.loadTerminal(`r${i + 1}`))?.status).toBe(status);
+        }
+
+        // ⛔ And the list-side gate admits all four. This filter used to spell
+        // its own two-member list — a SECOND copy of the vocabulary — so
+        // widening only the writer would have replaced a wrong status with a
+        // missing row, which is worse: a cancelled run would have vanished
+        // from the Runs list entirely.
+        const history = await reader.listHistory('four_flow', 10);
+        expect(history.map(r => r.status).sort()).toEqual([...members].sort());
+    });
+
+    it('the refusal ladder answers RUN_CANCELLED to a replica that never saw the cancel', async () => {
+        // The reading triage asked for, and it is only half the story — see the
+        // companion pin in `stranded-run-status.test.ts`, where a replica
+        // holding its OWN stale `failed` ring entry still answers
+        // `NO_CONSUMED_SUSPENSION`. ⛔ The ladder is deliberately unchanged
+        // here; what moved is the row it reads.
+        const table = createFakeEngine();
+        const a = pausableEngine(new ObjectStoreSuspendedRunStore(table, createTestLogger()));
+        const paused = await a.execute('approval_flow');
+        const runId = paused.runId!;
+        expect(await a.cancelRun(runId, 'submitter withdrew')).toBe(true);
+        await settle();
+
+        const b = pausableEngine(new ObjectStoreSuspendedRunStore(table, createTestLogger()));
+        const refused = await b.restoreConsumedSuspension(runId);
+        expect(refused.restored).toBe(false);
+        // Was `NO_CONSUMED_SUSPENSION` — honest, but everything the folded row
+        // could support. The row can support the real reason now.
+        expect(refused.refusal).toBe('RUN_CANCELLED');
+    });
+});