diff --git a/.changeset/approvals-engineless-resume-gap.md b/.changeset/approvals-engineless-resume-gap.md new file mode 100644 index 0000000000..be4ed4e2f5 --- /dev/null +++ b/.changeset/approvals-engineless-resume-gap.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): an approval decision can no longer strand a flow run silently when no automation engine is attached (#4420) + +#4420's fix closed every path by which a decision could be recorded while its +flow stayed parked — except one, and it is the one where none of the new guards +could run. Every guard it added (`assertRunResumable`'s pre-flight, the +`RESUME_TARGET_LOST` refusal, the `RESUME_FAILED` throw) hangs off the +automation engine. In a process where **no engine is attached**, all of them +were skipped by the same `typeof this.automation?.resume === 'function'` +condition that wrapped the resume itself — so the decision was written, the +mirrored status field advanced, and the call answered HTTP 200 with +`resumed: false` and **nothing logged at all**. That is #4420's reported +symptom exactly, reproduced in the one composition its fix could not see. + +The composition is reachable the same way the original bug was: a flow parks at +an `approval` node in a process that has the automation service, and the +decision arrives in one that does not (the plugin failed to init, or the host +was recomposed between releases). The request row still carries a +`flow_run_id` — which is the row's own declaration that a run is parked on this +decision. + +**What changes.** The decision still stands. Rolling it back is not on the +table (a human really decided, and the row is durable by then), and refusing +every such call would break the standalone approvals compositions the +pre-flight deliberately protects — so `finalized` and `resumed` are unchanged +for every existing caller. What changes is that the gap is no longer silent: + +- it is logged at **`error`**, per the durability rule in `AGENTS.md` — + persisted state and runtime state disagree while nothing looks broken from + the outside, which is the class that rule exists for; +- the response carries **`resumeError`**, so `resumed: false` arrives with its + reason and the stranded run's id instead of leaving the caller to guess + whether a resume was even attempted. + +It reuses the already-registered `RESUME_FAILED` code and the existing resume +message shape rather than introducing a new vocabulary — the fact being +reported (an outcome recorded whose run did not advance) is the same one. + +Applied at all five sites that resume a recorded outcome: `decide`, the +revision-limit auto-rejection, `sendBack`, `resubmit`, and both branches of +`recall` (whose revise-window path needs `cancelRun` rather than `resume`). + +A request that names **no** run is unaffected and stays quiet — there is +nothing parked on it, and reporting one there would be the mirror-image +failure that trains operators to skim `error`. diff --git a/packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts b/packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts index cfd80dba73..b0d3319f71 100644 --- a/packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts @@ -237,11 +237,25 @@ describe('approval decisions across a process restart (#4420)', () => { expect(marks).toEqual(['on_approved']); }); - it('leaves a composition with no automation engine exactly as it was', async () => { - // Approvals also runs with no engine attached — the request row still - // names a run, but there is nothing here that could resume it. The - // pre-flight must stay out of the way rather than invent a failure. - const standalone = new ApprovalService({ engine: data as any, logger: noopLogger }); + it('still records the decision with no automation engine attached — but never silently', async () => { + // Approvals runs with no engine attached, and the decision must still + // stand: a human really decided, the row is durable, and refusing every + // such call would break the standalone compositions the pre-flight + // deliberately protects. So `finalized` / `resumed` are unchanged. + // + // What changed is the silence. The request row NAMES a run, which is its + // own declaration that a flow is parked on this decision — and every guard + // the #4420 fix added hangs off an engine that is not there, so this was + // the one path left answering 200 / `resumed: false` with nothing logged: + // #4420's exact reported symptom, in the one composition its fix could not + // see. The gap is now reported at `error` (persisted state and runtime + // state disagree, and nothing looks broken) and carried on `resumeError`. + const logs: Array<{ level: string; msg: string }> = []; + const capturing = { + info() {}, warn() {}, debug() {}, + error(msg: any) { logs.push({ level: 'error', msg: String(msg) }); }, + }; + const standalone = new ApprovalService({ engine: data as any, logger: capturing as any }); const opened = await standalone.openNodeRequest({ object: 'crm_deal', recordId: 'd1', runId: 'run_from_another_process', nodeId: 'approve_step', config: { approvers: [{ type: 'user', value: 'u1' }] } as any, @@ -250,5 +264,43 @@ describe('approval decisions across a process restart (#4420)', () => { const out = await standalone.decide((opened as any).id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX); expect(out.finalized).toBe(true); expect(out.resumed).toBe(false); + + // The divergence is machine-visible, not a bare `resumed: false` the + // caller has to guess about, and it names the run that stayed parked. + expect(out.resumeError, 'the composition gap must reach the caller').toBeTruthy(); + expect(out.resumeError).toMatch(/RESUME_FAILED/); + expect(out.resumeError).toMatch(/run_from_another_process/); + + // …and it is loud in the log at `error`, per AGENTS.md's durability rule. + expect(logs.some(l => /no automation engine to advance/.test(l.msg)), + `expected a loud error log, got: ${JSON.stringify(logs)}`).toBe(true); + }); + + it('reports the same gap on a request that names no run — by staying quiet', async () => { + // The counterpart that keeps the rule honest: a standalone approvals + // request with NO `flow_run_id` has nothing parked on it, so there is no + // divergence to report. Reporting one here would be the mirror-image + // failure — training operators to skim `error`, which is what made #4420's + // original `warn` unreadable in the first place. + const logs: string[] = []; + const capturing = { + info() {}, warn() {}, debug() {}, error(msg: any) { logs.push(String(msg)); }, + }; + const standalone = new ApprovalService({ engine: data as any, logger: capturing as any }); + const opened = await standalone.openNodeRequest({ + object: 'crm_deal', recordId: 'd1', runId: 'run_x', + nodeId: 'approve_step', config: { approvers: [{ type: 'user', value: 'u1' }] } as any, + }, SYSTEM_CTX); + // `openNodeRequest` requires a run, so the only way a stored row names none + // is the one the `if (!runId)` guards are written for: a legacy or + // externally-created request that no flow node ever owned. Model it here. + data.tables.get('sys_approval_request')! + .find((r: any) => r.id === (opened as any).id).flow_run_id = null; + + const out = await standalone.decide((opened as any).id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX); + expect(out.finalized).toBe(true); + expect(out.resumed).toBe(false); + expect(out.resumeError, 'nothing was parked — nothing to report').toBeUndefined(); + expect(logs, `no run named, so no degradation: ${JSON.stringify(logs)}`).toEqual([]); }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 027b6a49af..a154c4c869 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1966,6 +1966,58 @@ export class ApprovalService implements IApprovalService { } } + /** + * The run named by a recorded outcome cannot be advanced at all, because no + * automation engine in THIS process implements the capability it needs + * (#4420). Returns the reason, or `undefined` when the capability is there + * and the caller should proceed. + * + * **Why this is not simply "not our problem".** Approvals is legitimately + * usable with no engine attached, and {@link assertRunResumable} deliberately + * stays out of the way for that reason. But "no engine is attached" and "no + * run is waiting" are different facts, and only the second is benign: a + * `flow_run_id` on the row is the request's OWN declaration that a run is + * parked on this decision. Deciding it in a process that cannot resume it + * reproduces #4420's reported half-state exactly — a durable decision, a + * mirrored status field frozen mid-workflow, a flow parked forever — while + * the caller is answered HTTP 200. The engine-less composition is the one + * path the #4420 fix left silent, because every guard it added hangs off an + * engine that is not there. + * + * **So the outcome stands, but it is never silent.** Rolling the decision + * back is not on the table (a human really did decide, and the row is + * already durable), and refusing every such call would break the standalone + * compositions the pre-flight protects. What is owed is the report: `error` + * level per AGENTS.md's durability rule — persisted state and runtime state + * disagree and nothing looks broken from the outside — plus a `resumeError` + * on the response, so `resumed: false` carries its reason instead of leaving + * the caller to guess whether a resume was even attempted. + * + * Reuses the registered `RESUME_FAILED` code (ADR-0112 ledger) and + * {@link serviceResume}'s message shape: the fact being reported — an + * outcome recorded whose run did not advance — is the same one, and this + * needs no new vocabulary of its own. + */ + private missingRunCapability( + runId: string, + requestId: string, + what: string, + capability: 'resume' | 'cancelRun', + ): string | undefined { + const fn = capability === 'resume' ? this.automation?.resume : this.automation?.cancelRun; + if (typeof fn === 'function') return undefined; + this.logger?.error?.( + '[approvals] no automation engine to advance the recorded outcome — the run is stranded', + { request: requestId, run: runId, outcome: what, capability }, + ); + return ( + `${capability} of run '${runId}' failed [RESUME_FAILED]: ${what} was recorded on request ${requestId}, ` + + `but no automation engine in this process can ${capability} its flow run — the run stays parked and the ` + + `record's mirrored status will not advance. Compose the automation service in this host, or recall the ` + + `request to release the record.` + ); + } + /** * Resume the run behind an outcome that has ALREADY been written down, and * fail loudly when it cannot be (#4420). @@ -1977,7 +2029,10 @@ export class ApprovalService implements IApprovalService { * everything it catches never reaches a write. * * `RESUME_IN_PROGRESS` is the exception — a concurrent resume is already - * advancing the run, so the outcome stands and only `resumed` is false. + * advancing the run, so the outcome stands and only `resumed` is false. So + * is a composition with no engine at all ({@link missingRunCapability}), + * which cannot throw without breaking every standalone deployment — it + * reports through `resumeError` instead. * * @param what - how the recorded outcome reads in the error, e.g. * `"the approve decision"`. @@ -1988,6 +2043,8 @@ export class ApprovalService implements IApprovalService { what: string, signal: { output?: Record; branchLabel?: string }, ): Promise<{ resumed: boolean; resumeError?: string }> { + const missing = this.missingRunCapability(runId, requestId, what, 'resume'); + if (missing) return { resumed: false, resumeError: missing }; try { await this.serviceResume(runId, signal); return { resumed: true }; @@ -2032,7 +2089,11 @@ export class ApprovalService implements IApprovalService { let resumed = false; let resumeError: string | undefined; - if (result.finalized && result.runId && typeof this.automation?.resume === 'function') { + // No `typeof this.automation?.resume === 'function'` guard here (#4420): + // skipping the call when no engine is attached is precisely how a decision + // against a parked run returned 200 / `resumed: false` with nothing logged. + // `resumeRecordedOutcome` reports that composition gap instead of hiding it. + if (result.finalized && result.runId) { const branchLabel = result.decision === 'approve' ? APPROVAL_BRANCH_LABELS.approve : APPROVAL_BRANCH_LABELS.reject; @@ -2134,29 +2195,35 @@ export class ApprovalService implements IApprovalService { if (inReviseWindow) { // ADR-0044: the run is paused at the revise wait node, which has no // reject out-edge to resume down — terminally cancel it instead. - if (runId && typeof this.automation?.cancelRun === 'function') { + if (runId) { + resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'cancelRun'); + if (!resumeError) { + try { + await this.automation!.cancelRun!(runId, `approval request ${requestId} recalled during revision`); + } catch (err: any) { + resumeError = err?.message ?? String(err); + this.logger?.error?.('[approvals] cancelRun after revise-window recall failed — the run may be stranded', { + request: requestId, run: runId, error: resumeError, + }); + } + } + } + } else if (runId) { + resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'resume'); + if (!resumeError) { try { - await this.automation.cancelRun(runId, `approval request ${requestId} recalled during revision`); + await this.serviceResume(runId, { + branchLabel: APPROVAL_BRANCH_LABELS.reject, + output: { decision: 'recall', requestId }, + }); + resumed = true; } catch (err: any) { resumeError = err?.message ?? String(err); - this.logger?.error?.('[approvals] cancelRun after revise-window recall failed — the run may be stranded', { + this.logger?.error?.('[approvals] resume after recall failed — the run may be stranded', { request: requestId, run: runId, error: resumeError, }); } } - } else if (runId && typeof this.automation?.resume === 'function') { - try { - await this.serviceResume(runId, { - branchLabel: APPROVAL_BRANCH_LABELS.reject, - output: { decision: 'recall', requestId }, - }); - resumed = true; - } catch (err: any) { - resumeError = err?.message ?? String(err); - this.logger?.error?.('[approvals] resume after recall failed — the run may be stranded', { - request: requestId, run: runId, error: resumeError, - }); - } } const fresh = await this.readBackRequest(requestId, context); @@ -2240,7 +2307,7 @@ export class ApprovalService implements IApprovalService { } let resumed = false; let resumeError: string | undefined; - if (runId && typeof this.automation?.resume === 'function') { + if (runId) { const outcome = await this.resumeRecordedOutcome( runId, requestId, 'the auto-rejection', { @@ -2281,7 +2348,7 @@ export class ApprovalService implements IApprovalService { let resumed = false; let resumeError: string | undefined; - if (runId && typeof this.automation?.resume === 'function') { + if (runId) { const outcome = await this.resumeRecordedOutcome( runId, requestId, 'the send-back', { @@ -2370,7 +2437,7 @@ export class ApprovalService implements IApprovalService { let resumed = false; let resumeError: string | undefined; - if (runId && typeof this.automation?.resume === 'function') { + if (runId) { const outcome = await this.resumeRecordedOutcome( runId, requestId, 'the resubmit', {