fix(automation): keep the stranded verdict when post-journal bookkeeping throws — a repairable strand must not report repairable: false

.changeset/strand-verdict-survives-bookkeeping-throw.md

@@ -0,0 +1,17 @@
+---
+'@objectstack/service-automation': patch
+---
+
+Keep a resume's `status: 'stranded'` verdict when the bookkeeping after the repair journal throws.
+
+`resumeInternal`'s catch arm journals the consumed suspension — the snapshot `restoreConsumedSuspension` puts back — and only then stamps `status: 'stranded'`. Two statements sat between them and could throw out of the whole arm: `recordLog`'s terminal run-summary line, and a store whose `recordTerminal` throws synchronously (the `void write.catch(...)` beneath that call only ever sees a returned promise's rejection). `failAncestors` follows them.
+
+A throw in that window left the run genuinely repairable while the verdict never shipped, and every consumer derives repairability from the verdict — `plugin-approvals` computes its operator-facing `repairable` as `status === 'stranded'` — so the approvals decision door reported `repairable: false` about a run that `restoreConsumedSuspension` answers `restored: true` for. That is a false negative on a repair instruction: it tells an operator not to attempt a repair that works.
+
+The window is now guarded. The bookkeeping may still fail — and says so loudly, at `error`, naming the run, what did not land, and the verb that repairs the strand — while the verdict still ships. Measured: with a store whose terminal write throws, `resume` now returns `{ success: false, status: 'stranded' }` instead of throwing, the door reports `repairable: true`, and the repair verb succeeds on that same run.
+
+The guard opens **after** the journal, so only a run that demonstrably has a snapshot can reach the stamp: a throw from the journal itself still propagates, every exit above the consumption point still carries no status at all, and cascade-failed ancestors — which journal nothing — are untouched and still correctly non-repairable.
+
+⚠️ This change also makes a pre-existing fault **visible** rather than creating it. The completion path's history write sits inside the same `try` as the node-failure arm, so a run that **completed** — every node succeeded — is journalled and reported `stranded` when its `completed` history row throws, and repairing such a run **re-runs the flow**. That phantom, its repair snapshot and the double run were all measurable before this change; what changes here is only that more store failures now report the verdict instead of throwing over it, so an operator can now be told to repair a completed run. Filed as #15944, with the measurement on both trees.
+
+⚠️ `repairable` remains a point-in-time fact, and this change does not make it durable: the run in the case above has no terminal history row (that write is what failed), so the repair rides on the in-memory journal and a restart loses it. The verdict reports what an operator can do now, which is exactly what was being denied.

packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts

@@ -39,10 +39,35 @@
  *  3. `'stranded'` observed AT THE DOOR, not dropped — with its reverse
  *     control, a resume failure the engine does NOT call stranded, which must
  *     report `repairable: false` rather than inheriting a default.
+ *
+ * ## The fourth pin (#15555) — the false NEGATIVE
+ *
+ * PIN 3's reverse control holds that the ABSENCE of the stamp must not be read
+ * as repairable. #15555 is the other half of the same reading: the stamp could
+ * go missing on a run that IS repairable, because the engine journalled the
+ * repair snapshot and then threw before stamping. The door then answered
+ * `repairable: false` — correctly from its own point of view, and wrong about
+ * the world — telling an operator not to attempt a repair that succeeds.
+ * PIN 4 drives that window through this same door and asserts the fix at the
+ * only place it matters: what the operator is told.
  */
 
 import { describe, it, expect, beforeEach } from 'vitest';
 import { AutomationEngine, InMemorySuspendedRunStore } from '@objectstack/service-automation';
+
+/**
+ * [#15555] A durable run-history store whose terminal write throws
+ * SYNCHRONOUSLY — the statement that sits between the engine's repair journal
+ * and its `status: 'stranded'` stamp. The engine's own `void write.catch(...)`
+ * beneath that call only ever sees a returned promise's rejection, so a
+ * synchronous throw escapes the whole catch arm.
+ */
+const TERMINAL_WRITE_FAILURE = 'run-history driver refused the terminal row';
+class SyncThrowTerminalStore extends InMemorySuspendedRunStore {
+  override recordTerminal(): Promise<void> {
+    throw new Error(TERMINAL_WRITE_FAILURE);
+  }
+}
 // [#4550] The engine doubles below route their write verbs through ObjectQL's
 // OWN dispatch predicates rather than a hand-mirrored copy — a double looser
 // than the engine it stands in for is how #4434 shipped a dead REST route with
@@ -141,8 +166,8 @@ describe('#13807 — a stranded decision publishes its facts, and keeps its stat
   let rejectBranchThrows: string | undefined;
 
   /** One live process: real engine, real approval node, real approvals service. */
-  function boot() {
-    const automation = new AutomationEngine(noopLogger as any, new InMemorySuspendedRunStore());
+  function boot(store?: InMemorySuspendedRunStore) {
+    const automation = new AutomationEngine(noopLogger as any, store ?? new InMemorySuspendedRunStore());
     registerApprovalNode(automation, service, noopLogger as any);
     automation.registerNodeExecutor({
       type: 'mark',
@@ -308,4 +333,47 @@ describe('#13807 — a stranded decision publishes its facts, and keeps its stat
     expect(lostDetails?.runId, 'and it still names the run an operator must look at')
       .toBe(lostReq.flow_run_id);
   });
+
+  it('PIN 4 — #15555: a strand whose own bookkeeping throws is STILL reported repairable', async () => {
+    // The false NEGATIVE, driven end to end through the public door. The
+    // engine consumes the pause, journals the repair snapshot, and then the
+    // durable run-history write throws out of the arm before the
+    // `status: 'stranded'` stamp is reached. Before the engine-side guard the
+    // door received a raw throw carrying no run-state discriminator at all,
+    // so `repairable = status === 'stranded'` answered FALSE — about a run the
+    // repair verb puts back successfully, asserted below on the same run.
+    rejectBranchThrows = 'the node blew up';
+    const automation = boot(new SyncThrowTerminalStore());
+    const req = await park(automation);
+    const runId = req.flow_run_id;
+
+    const err = await service
+      .decide(req.id, { decision: 'reject', actorId: 'u1' }, SYSTEM_CTX)
+      .then(() => null, (e: Error) => e);
+
+    // The door still throws and the decision still stands — this card moves
+    // neither. ⛔ The status code and the `finalized` fact are the #13807
+    // ruling's and are untouched.
+    const details = strandedDecisionDetails(err);
+    expect(details?.finalized).toBe(true);
+    expect(details?.decision).toBe('reject');
+    expect(details?.runId).toBe(runId);
+    // ⭐ The card: the operator must be told the repair is worth attempting.
+    expect(details?.repairable, 'a journalled strand is repairable even when its bookkeeping failed').toBe(true);
+
+    // The two assertions that make the old `false` a FALSE NEGATIVE rather
+    // than a conservative default: the run really is stranded, and the verb
+    // the operator was told not to bother with really does put it back.
+    expect(await automation.hasSuspendedRun(runId)).toBe(false);
+    const restored = await automation.restoreConsumedSuspension(runId, { requestedBy: 'ops' });
+    expect(restored.restored, 'the repair the caller was told not to attempt').toBe(true);
+    expect(await automation.hasSuspendedRun(runId)).toBe(true);
+
+    // And the prose names the run's OWN failure, not the history driver's:
+    // the secondary failure is an operator fact and belongs in the engine's
+    // log, never in the sentence that explains why the flow stopped.
+    expect(err?.message).toMatch(/^RESUME_FAILED/);
+    expect(err?.message).toContain(rejectBranchThrows);
+    expect(err?.message).not.toContain(TERMINAL_WRITE_FAILURE);
+  });
 });

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

@@ -5515,23 +5515,90 @@ export class AutomationEngine implements IAutomationService {
                 // ordering above is untouched. It is the evidence a repair
                 // needs, written at the only moment it still exists.
                 const consumed = this.journalConsumedSuspension(run, stepCountAtPause, errorMessage);
-                const logged = this.recordLog({
-                    id: runId,
-                    flowName: run.flowName,
-                    flowVersion: run.flowVersion,
-                    status: 'failed',
-                    startedAt: run.startedAt,
-                    completedAt: new Date().toISOString(),
-                    durationMs,
-                    trigger: buildRunTrigger(context),
-                    steps,
-                    error: errorMessage,
-                }, context, consumed.run);
-                // Subflow chain: a child failing terminally fails every
-                // ancestor awaiting it — they can never be resumed otherwise.
-                // The delegation path handles its own level (skipBubble).
-                if (!skipBubble) {
-                    await this.failAncestors(run.context, errorMessage);
+                // [#15555] From the line above, "this run is repairable" is a
+                // FACT: a snapshot exists and `restoreConsumedSuspension` puts
+                // it back. Everything from here to the `status: 'stranded'`
+                // stamp below is BOOKKEEPING ABOUT that fact — and none of it
+                // may be allowed to delete the fact by throwing.
+                //
+                // It could, and the direction is the dangerous one. The stamp
+                // is the ONLY thing that tells a consumer the run is
+                // repairable (`plugin-approvals` derives its operator-facing
+                // `repairable` from it verbatim: `status === 'stranded'`), and
+                // two statements in here throw out of the whole arm. Both live
+                // in `recordLog`'s terminal path: the run-summary line
+                // (`logger.info`, on by default) and `store.recordTerminal`,
+                // whose SYNCHRONOUS throw escapes — the `void write.catch(...)`
+                // beneath that call only ever sees a returned promise's
+                // rejection. Then `failAncestors` awaits a walk that can throw.
+                // A throw anywhere in that window replaced a truthful
+                // `repairable: true` with `false`, which does not merely lose
+                // information: it tells an operator NOT to attempt a repair
+                // that succeeds. A false negative on a repair instruction is
+                // worse than silence, and it is the opposite of the direction
+                // everybody checks for.
+                //
+                // ⛔ This is NOT "assume repairable when the failure is
+                // unknown" — that would invert the honest default and promise
+                // a repair for a lost run. The guard opens AFTER the journal,
+                // so only a run that demonstrably HAS a snapshot can reach the
+                // stamp: a throw from `journalConsumedSuspension` itself still
+                // propagates, and every exit above the consumption point is
+                // untouched and still carries no status at all.
+                //
+                // ⛔ And the journal is NOT moved down to sit beside the stamp
+                // instead. `recordLog` is what carries the snapshot into the
+                // durable row, so journalling after it would leave a
+                // `recordLog` failure with NO snapshot anywhere — converting
+                // this false negative into a TRUE one by destroying the repair
+                // rather than by reporting it.
+                let logged: ExecutionLogEntry | undefined;
+                try {
+                    logged = this.recordLog({
+                        id: runId,
+                        flowName: run.flowName,
+                        flowVersion: run.flowVersion,
+                        status: 'failed',
+                        startedAt: run.startedAt,
+                        completedAt: new Date().toISOString(),
+                        durationMs,
+                        trigger: buildRunTrigger(context),
+                        steps,
+                        error: errorMessage,
+                    }, context, consumed.run);
+                    // Subflow chain: a child failing terminally fails every
+                    // ancestor awaiting it — they can never be resumed otherwise.
+                    // The delegation path handles its own level (skipBubble).
+                    if (!skipBubble) {
+                        await this.failAncestors(run.context, errorMessage);
+                    }
+                } catch (bookkeeping) {
+                    // #4632 verdict: DURABILITY, so `error` — the caller is
+                    // told a truthful, actionable thing (the run stranded, and
+                    // it is repairable), which is exactly what makes the rest
+                    // invisible from the outside: the terminal history row
+                    // never landed and/or the ancestor cascade stopped
+                    // part-way, nothing retries either, and no envelope
+                    // carries a word about it. Consequence and fix in the
+                    // first line, per AGENTS.md. Said ONCE per stranded run,
+                    // not once per failed write.
+                    //
+                    // THIRD argument per `error(message, error?, meta?)`; the
+                    // `Error` slot stays empty on purpose (#5575), and the
+                    // thrown text goes to the structured slot rather than into
+                    // the message (#6499).
+                    this.logger.error(
+                        `[Automation] run '${runId}' of flow '${run.flowName}' is STRANDED and its ` +
+                            `post-strand bookkeeping threw, so its terminal history row never landed ` +
+                            `and/or its subflow ancestors were not failed — nothing retries either, and ` +
+                            `the run reads healthy to the Runs surfaces and the approvals sweeps. The ` +
+                            `strand itself IS reported and repairable right now: restore it with ` +
+                            `restoreConsumedSuspension('${runId}') before this process restarts, which ` +
+                            `drops the in-memory journal this repair rides on. Fix the failure in this ` +
+                            `record's meta.`,
+                        undefined,
+                        describeThrownForLog(bookkeeping),
+                    );
                 }
                 // Surface the flow's friendly error message (the raw error stays
                 // in `error` for logs/diagnostics).
@@ -5569,7 +5636,11 @@ export class AutomationEngine implements IAutomationService {
                     // worse) condition, which this stamp must not claim.
                     status: 'stranded',
                     errorMessage: flow.errorMessage,
-                    summary: logged.summary,
+                    // [#15555] Recomputed when the guard above had to abandon
+                    // `recordLog`: the same pure function of the same steps
+                    // that `recordLog`'s own first statement runs, so the two
+                    // spellings cannot disagree.
+                    summary: logged?.summary ?? summarizeRun(steps),
                 };
             }
         } finally {