diff --git a/.changeset/approvals-inspector-sees-failed-mid-resume.md b/.changeset/approvals-inspector-sees-failed-mid-resume.md new file mode 100644 index 0000000000..5b1c6f5946 --- /dev/null +++ b/.changeset/approvals-inspector-sees-failed-mid-resume.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(plugin-approvals): the stranded-request inspection now sees a run that FAILED mid-resume (#13909) + +`ApprovalService.inspectStrandedRequests` was structurally blind to the shape +#13909 owns, and reported `0` for it — the one shape an operator most needs to +see. + +**The mechanism.** `AutomationEngine.resumeInternal` consumes the suspension +*before* running the downstream nodes: `forgetSuspendedRun(run, 'resumed')` +precedes `traverseNext`. A downstream node that merely THREW therefore threw +with the pause already gone, the catch arm recorded the run `failed`, and +nothing can resume it again (`resume` answers `RUN_NOT_FOUND`, `cancelRun` is a +no-op). The decision is durable and the flow stopped half-way. + +**Why the inspection could not see it.** Its second oracle was +`if (terminal) continue` — the existence of ANY run-history row ended the check, +on the reading "the run ran to a terminal state, it is not dangling". But the +terminal row here is written BY the failure that stranded the request, so the +evidence of the defect was read as evidence of health. `releaseDeadRunRequests` +cannot see it either: it scans `status: 'pending'`, and the decision is what +took the row out of `pending`. + +**The widening, and its limits.** The second oracle now classifies the run +instead of merely detecting it. A `failed` run is reported; `completed`, +`cancelled` and `paused` are each still skipped, one named reason at a time, and +a status this code does not recognise is skipped too — the spec's +`ExecutionStatus` vocabulary is wider than the four statuses the engine writes, +and a future status must not become a silent false positive. `paused` in +particular stays skipped because "the suspension is gone but no terminal row is +written yet" is exactly what a resume IN FLIGHT looks like. The first oracle is +unchanged: a run the suspension store still holds is alive, and an unreadable +store is still counted `undetermined`, never condemned. + +Reported rows now carry `runState: 'missing' | 'failed'` (new exported type +`StrandedRunState`), because the two shapes need different remedies: a `missing` +run has no history to read, a `failed` one has a step log and an error naming +the node that threw. The sweep's own warning splits its counts the same way. +`StrandedApprovalRequest` is an output-only reporting shape the service +produces; the added field is not constructed by any caller in this repo. + +**Still read-only, and still not a census.** No status is changed and no run is +cancelled — the decision genuinely happened. This makes the condition *visible* +in a deployment; how many runs are already in it can only be answered against +that deployment's own tables. Nothing here changes the resume ordering, which +is #13909's own next slice. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index b2150be331..f78fe50e1d 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -145,7 +145,7 @@ The largest single consumer — **20 of the 109 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:333` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:440` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:850`, `:959`, `:2916`, `:3062`, `:3229`, `:3300`, `:3489`, `:3529` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:931`, `:1040`, `:2997`, `:3143`, `:3310`, `:3381`, `:3570`, `:3610` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` | diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3fbf4ebae9..d016b8bf9f 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -247,6 +247,76 @@ const TERMINAL_RUN_STATUSES: ReadonlySet = new Set([ */ const STRANDABLE_REQUEST_STATUSES = ['approved', 'rejected', 'returned'] as const; +/** + * The second oracle's verdict: which unrecoverable shape this run is in, or + * `undefined` for every run that must NOT be reported (#13909). + * + * Written as an explicit switch, not `status !== 'completed'`, because the + * negatives are the load-bearing half — a widening that reports everything the + * old `if (terminal) continue` used to skip would bury the finding it exists to + * surface. Each skip below is a distinct reason, and each is pinned by its own + * test. + * + * The engine writes exactly four run statuses (`paused`, `completed`, `failed`, + * `cancelled`); the spec's `ExecutionStatus` vocabulary is wider (`timed_out`, + * `retrying`, …) and nothing in the engine writes the rest today. The `default` + * arm therefore stays SILENT rather than reporting: a status this code does not + * recognise is not evidence a decision was stranded, and condemning on it would + * make every future status a false positive until someone noticed. + */ +function classifyStrandedRunState(run: { status?: string } | null | undefined): StrandedRunState | undefined { + // No history row at all — the #4469 shape this inspection was built for. + if (!run) return 'missing'; + switch (run.status) { + // The resume consumed the pause and a downstream node threw. Reported. + case 'failed': + return 'failed'; + + // ── The negatives, each for its own reason ────────────────────────────── + // The decision advanced the flow and the flow finished. Healthy. + case 'completed': + return undefined; + // Deliberately terminated by an operator (`cancelRun`, ADR-0044). The run + // stopping is the intended outcome, exactly as `recalled` is on the request + // side — reporting it would bury the real findings under expected ones. + case 'cancelled': + return undefined; + // The history's last row says `paused` while the suspension store says no + // live pause. That is AMBIGUOUS, and the ambiguity is not resolvable from + // one scan: a resume in flight right now has consumed the suspension and + // not yet written its terminal row, and reads exactly like a process that + // died in the same window. Condemning it would name every concurrently + // resuming approval — so this stays SKIPPED, the conservative arm this + // whole method is built on. + case 'paused': + return undefined; + default: + return undefined; + } +} + +/** + * WHY a terminal request's run is unrecoverable — the two shapes the inspection + * reports, which have different causes and different remedies (#13909). + * + * - `missing` — `getRun` finds no history row at all (#4469's original shape): + * the run was lost before it could record anything, typically a pause that + * never reached a durable store and did not survive a restart. + * - `failed` — the run DID record a terminal `failed` row. The engine consumes + * a suspension *before* running the downstream nodes + * (`AutomationEngine.resumeInternal`: `forgetSuspendedRun(run, 'resumed')` + * precedes `traverseNext`), so a downstream node that merely THREW threw with + * the pause already gone — the catch arm recorded `failed` and there is no + * suspension left to resume. The decision is durable, the flow stopped + * mid-continuation, and no verb moves the run out of that state. + * + * ⚠️ This names the shapes for the REPORT only. It is not a run state: the + * engine's own vocabulary is still `'completed' | 'paused' | 'failed'` + * (`AutomationResult.status`) and nothing persists or queries "stranded". + * Giving the condition a platform-level name is #13909's own deliverable. + */ +export type StrandedRunState = 'missing' | 'failed'; + /** * One terminal request whose owning flow run is unrecoverable (#4469) — the * decision was recorded and the flow never moved. Reporting shape only: the @@ -257,8 +327,19 @@ export interface StrandedApprovalRequest { requestId: string; /** Terminal status the request reached — the decision that WAS recorded. */ status: string; - /** The `flow_run_id` that resolves to neither a suspension nor a run history row. */ + /** + * The `flow_run_id` that resolves to no live suspension and no recoverable + * run — see `runState`: no history row at all (`missing`), or a terminal + * `failed` row (`failed`). + */ runId: string; + /** + * Which unrecoverable shape this is — see {@link StrandedRunState}. Carried + * because the two need different remedies: a `missing` run has no history to + * read, while a `failed` one has a step log and an error message naming the + * node that threw. + */ + runState: StrandedRunState; flowName?: string; /** Approval node the run should have continued from. */ nodeId?: string; @@ -3679,9 +3760,35 @@ export class ApprovalService implements IApprovalService { * live pause exists. It THROWS when the store cannot be read, and that * case is SKIPPED, never counted as dead: an unreadable store means * "unknown", and a storage outage must not be published as a lost run. - * - `getRun(runId) == null` — no terminal history row either (the `run_` - * prefixed rows in `sys_automation_run`). A run that merely finished is - * not stranded; a request whose run neither waits nor ever completed is. + * - `classifyStrandedRunState` over `getRun(runId)` — the run's own + * history row (the `run_` prefixed rows in `sys_automation_run`). A run + * that merely finished is not stranded; a request whose run neither waits + * nor completed is. + * + * **The second oracle was widened (#13909), and this is the whole point of + * that card's first slice.** It used to be `if (terminal) continue` — the + * existence of ANY history row ended the check, on the reading "the run ran to + * a terminal state, it is not dangling". That is true of a run that COMPLETED + * and false of one that FAILED: the engine consumes a suspension *before* + * running the downstream nodes (`AutomationEngine.resumeInternal` calls + * `forgetSuspendedRun(run, 'resumed')` and only then `traverseNext`), so a + * downstream node that merely threw threw with the pause already gone, and the + * catch arm wrote a terminal `failed` row. The decision is durable, the + * continuation stopped half-way, `resume` answers `RUN_NOT_FOUND` and + * `cancelRun` is a no-op — and the terminal row this oracle used to read as + * health is written BY the very failure that stranded it. So this inspection + * reported `0` for the one shape an operator most needs to see. + * + * ⚠️ The widening does NOT reverse the conservatism: `completed`, `cancelled` + * and `paused` are each still skipped, for reasons named one at a time in + * `classifyStrandedRunState`, and an unrecognised status is skipped too. + * What the widening buys is that a `failed` run is now reported with + * `runState: 'failed'` instead of counted as healthy. + * + * ⚠️ **What this can and cannot size.** It makes the condition *visible* in a + * deployment; it is not itself a census, and it says nothing about this + * repository. How many runs are already in this state can only be answered + * against a real deployment's tables — see the card. * * **Reports; never rewrites.** No status is changed and no run is cancelled. * The decision genuinely happened — a human approved or rejected — and @@ -3748,10 +3855,15 @@ export class ApprovalService implements IApprovalService { }); continue; } - if (terminal) continue; // the run ran to a terminal state — it is not dangling - - // Neither suspended nor ever finished: the run this decision was supposed - // to advance is genuinely gone. + // #13909 — the widened verdict. `undefined` means "not a shape this + // reports": healthy, deliberate, or unresolvable. See + // `classifyStrandedRunState` for which, and why each one. + const runState = classifyStrandedRunState(terminal); + if (!runState) continue; + + // Neither suspended nor recoverable: the run this decision was supposed to + // advance is gone (`missing`) or terminally failed mid-continuation with + // its pause already consumed (`failed`). const config = parseJson( raw.node_config_json, { approvers: [], behavior: 'first_response' } as any, ); @@ -3770,6 +3882,7 @@ export class ApprovalService implements IApprovalService { requestId: String(raw.id), status: raw.status, runId, + runState, flowName: typeof raw.process_name === 'string' ? raw.process_name.replace(/^flow:/, '') : undefined, nodeId: raw.flow_node_id ?? raw.current_step ?? undefined, objectName: raw.object_name, @@ -3782,9 +3895,14 @@ export class ApprovalService implements IApprovalService { } if (stranded.length || undetermined) { - this.logger?.warn?.('[approvals] stranded terminal requests (decision recorded, flow run gone)', { + // The two shapes are counted separately: they have different causes and + // different remedies, and an operator reading one number could not tell a + // pre-existing #4469 zombie from a run that failed mid-resume (#13909). + this.logger?.warn?.('[approvals] stranded terminal requests (decision recorded, flow run unrecoverable)', { scanned: rows.length, stranded: stranded.length, undetermined, - requests: stranded.map(s => `${s.requestId}@${s.nodeId ?? '?'} → run ${s.runId}`), + runMissing: stranded.filter(s => s.runState === 'missing').length, + runFailed: stranded.filter(s => s.runState === 'failed').length, + requests: stranded.map(s => `${s.requestId}@${s.nodeId ?? '?'} → run ${s.runId} (${s.runState})`), }); } return { scanned: rows.length, stranded, undetermined }; diff --git a/packages/plugins/plugin-approvals/src/index.ts b/packages/plugins/plugin-approvals/src/index.ts index 5790c2ee3e..c440249004 100644 --- a/packages/plugins/plugin-approvals/src/index.ts +++ b/packages/plugins/plugin-approvals/src/index.ts @@ -25,6 +25,8 @@ export { type ApprovalNodeAutoOutcome, // #4469 — the read-only stranded-request inspection's report shape. type StrandedApprovalRequest, + // #13909 — which unrecoverable shape a reported row is in. + type StrandedRunState, } from './approval-service.js'; export { ApprovalsServicePlugin, diff --git a/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts b/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts index 3bdfba01de..a975886e0f 100644 --- a/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts +++ b/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts @@ -17,6 +17,22 @@ * * So the inspection uses BOTH oracles and reports only rows that fail both, * skipping (never condemning) anything the stores could not answer for. + * + * ── #13909: the second oracle was too narrow ──────────────────────────────── + * + * `if (terminal) continue` read the mere EXISTENCE of a history row as health. + * The engine consumes a suspension before running the downstream nodes + * (`forgetSuspendedRun(run, 'resumed')` precedes `traverseNext`), so a node that + * merely threw threw with the pause already gone and the catch arm wrote a + * terminal `failed` row — the decision durable, the continuation stopped + * half-way, nothing able to resume it. The row this inspection read as "it + * finished, it is not dangling" is written BY the failure that stranded it, so + * the one shape an operator most needs was reported as `0`. + * + * The widening is deliberately narrow, and the second half of this file pins + * that: `completed`, `cancelled`, `paused` and any status this code does not + * recognise are each STILL skipped, one test per reason. A widening that + * reported everything would bury the finding it exists to surface. */ import { describe, it, expect, beforeEach } from 'vitest'; @@ -131,6 +147,8 @@ describe('stranded terminal request inspection (#4469)', () => { requestId: 'areq_1', status: 'approved', runId: 'run_1', + // #13909 — WHICH shape: no history row at all, the original #4469 zombie. + runState: 'missing', nodeId: 'co_sign', flowName: 'deal_approval', objectName: 'opportunity', @@ -157,7 +175,7 @@ describe('stranded terminal request inspection (#4469)', () => { expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); }); - it('does NOT report a request whose run ran to a terminal state — it finished, it is not dangling', async () => { + it('does NOT report a request whose run COMPLETED — the decision advanced the flow', async () => { engine._tables['sys_approval_request'] = [requestRow()]; svc.attachAutomation(automation({ history: { run_1: { status: 'completed' } } })); expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); @@ -239,3 +257,151 @@ describe('stranded terminal request inspection (#4469)', () => { expect(await svc.inspectStrandedRequests()).toEqual({ scanned: 0, stranded: [], undetermined: 0 }); }); }); + + +/** + * The widening (#13909) — and, in equal measure, everything it must NOT widen. + * + * The positive is one test; the negatives are five, because "it now reports the + * bad one" says nothing about whether it started reporting the good ones too. + */ +describe('stranded inspection sees a run that FAILED mid-resume (#13909)', () => { + let engine: ReturnType; + let svc: ApprovalService; + + beforeEach(() => { + engine = makeFakeEngine(); + svc = new ApprovalService({ engine: engine as any }); + }); + + it('reports a terminal request whose run recorded a terminal `failed` row', async () => { + // The shape the card owns: the resume consumed the pause, a downstream node + // threw, the catch arm recorded `failed`. `hasSuspendedRun` is false because + // the suspension really is gone — that is the defect, not a healthy state. + engine._tables['sys_approval_request'] = [requestRow({ status: 'rejected' })]; + engine._tables['opportunity'] = [{ id: 'opp1', approval_status: 'pending' }]; + svc.attachAutomation(automation({ history: { run_1: { status: 'failed' } } })); + + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(1); + expect(out.undetermined).toBe(0); + expect(out.stranded).toHaveLength(1); + expect(out.stranded[0]).toMatchObject({ + requestId: 'areq_1', + status: 'rejected', + runId: 'run_1', + runState: 'failed', + nodeId: 'co_sign', + objectName: 'opportunity', + recordId: 'opp1', + }); + // The operator-facing symptom is carried for this shape too: the record's + // mirror still reads what it read before the decision. + expect(out.stranded[0].mirroredStatus).toBe('pending'); + }); + + it('the OLD oracle would have skipped it — the terminal row is written BY the failure', async () => { + // Pins the mechanism rather than the outcome: `getRun` DOES answer for this + // run, which is exactly why `if (terminal) continue` reported all clear. + const auto = automation({ history: { run_1: { status: 'failed' } } }); + expect(await auto.getRun('run_1')).not.toBeNull(); + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(auto); + expect((await svc.inspectStrandedRequests()).stranded).toHaveLength(1); + }); + + // ── The negatives, one reason per test ───────────────────────────────────── + + it('does NOT report a run that was CANCELLED — stopping it was the intent', async () => { + // `cancelRun` (ADR-0044) is an operator deliberately ending the run, the + // run-side twin of a `recalled` request. Reporting these would bury the + // real findings under expected ones. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ history: { run_1: { status: 'cancelled' } } })); + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(1); + expect(out.stranded).toEqual([]); + expect(out.undetermined).toBe(0); + }); + + it('does NOT report a run whose last history row says `paused` — that is ambiguous, not stranded', async () => { + // A resume in flight has already consumed the suspension and not yet + // written its terminal row: `hasSuspendedRun` false + history `paused` reads + // identically to a process that died in that window. Condemning it would + // name every concurrently resuming approval. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ history: { run_1: { status: 'paused' } } })); + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(1); + expect(out.stranded).toEqual([]); + expect(out.undetermined).toBe(0); + }); + + it('does NOT report a status it does not recognise — a new run state is not evidence of a strand', async () => { + // The spec's `ExecutionStatus` vocabulary is wider than the four statuses + // the engine writes (`timed_out`, `retrying`, …). The default arm stays + // silent so a future status cannot become a silent false positive. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ history: { run_1: { status: 'timed_out' } } })); + expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); + }); + + it('does NOT report a FAILED run that is still suspended — the first oracle still gates', async () => { + // A run re-parked at a later node after an earlier failed leg is alive and + // resumable; the suspension oracle short-circuits before the run state is + // ever classified. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ + suspended: { run_1: true }, history: { run_1: { status: 'failed' } }, + })); + expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); + }); + + it('still SKIPS a failed-run row whose suspension store threw — an outage stays unknown', async () => { + // The widening must not turn an unreadable store into a verdict: the + // undetermined counter, not the stranded list, is where this belongs. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ suspendedThrows: true, history: { run_1: { status: 'failed' } } })); + const out = await svc.inspectStrandedRequests(); + expect(out.stranded).toEqual([]); + expect(out.undetermined).toBe(1); + }); + + it('separates the two shapes in one mixed population — and reports only those two', async () => { + // The aggregate pin: four terminal requests, four different run states, and + // exactly the two unrecoverable ones come back, each labelled. + engine._tables['sys_approval_request'] = [ + requestRow({ id: 'areq_missing', flow_run_id: 'run_missing' }), + requestRow({ id: 'areq_failed', flow_run_id: 'run_failed' }), + requestRow({ id: 'areq_done', flow_run_id: 'run_done' }), + requestRow({ id: 'areq_cancelled', flow_run_id: 'run_cancelled' }), + ]; + svc.attachAutomation(automation({ + history: { + run_failed: { status: 'failed' }, + run_done: { status: 'completed' }, + run_cancelled: { status: 'cancelled' }, + // `run_missing` deliberately absent — `getRun` answers null for it. + }, + })); + + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(4); + expect(out.stranded.map(s => [s.requestId, s.runState])).toEqual([ + ['areq_missing', 'missing'], + ['areq_failed', 'failed'], + ]); + }); + + it('NEVER rewrites a failed-run row either — the decision really happened', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + engine._tables['opportunity'] = [{ id: 'opp1', approval_status: 'pending' }]; + svc.attachAutomation(automation({ history: { run_1: { status: 'failed' } } })); + const before = JSON.stringify(engine._tables); + + await svc.inspectStrandedRequests(); + + expect(JSON.stringify(engine._tables)).toBe(before); + expect(engine._tables['sys_approval_action'] ?? []).toHaveLength(0); + }); +});