From 94f9604fe6463f76f139eb479a848b0c723ce872 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 15:09:14 +0000 Subject: [PATCH 1/2] fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record lock keys on a pending `sys_approval_request` and could not tell the run that OWNS that request from an unrelated user editing the record. A flow that touched its own target record while its own approval was still pending — a manual `resume` with no decision, or a node writing the record between opening the approval and the decision — died on its own `RECORD_LOCKED`, leaving the record locked behind the dead run. Prevention: the automation engine stamps `flowRunId` onto the run context at setup, alongside `runAs`, and it travels with every data node's ObjectQL context into `ctx.session`; the lock hook exempts a write whose `flowRunId` matches the pending request's `flow_run_id`. Keyed on run identity rather than elevation on purpose — a `runAs:'user'` run stays RLS-scoped while it writes. The field is pure provenance: server-constructed like `isSystem`, never client-supplied, read by no security middleware, and it permits exactly one write — to the record its own run already holds a pending request against. Recovery: a sweep on the existing approvals clock finalizes a pending request whose owning run is terminal (`completed`/`failed`/`cancelled`/`timed_out`) as `recalled`, releasing the lock, audited under the reserved actor `system:dead-run`. This covers the case no in-band handler can — a run killed by a process crash. Fail-safe by construction: it acts only on an explicit terminal status from a closed set, so `paused` (a live approval), `running`, an unknown status, an unreadable run and a deployment with no automation engine are all read as alive. Also fixes `AutomationEngine.getRun`, which returned the FIRST log entry for a run id. A run that pauses then finishes records two entries under one id, so every suspend-then-finish run — every approval, screen and wait flow — reported itself as `paused` forever, on the Runs surface and to this sweep alike. Residual, deliberate: a `runAs:'user'` run with no trigger user (a schedule) passes no ObjectQL context at all, so it carries no `flowRunId` and is still subject to the lock. Manufacturing a context to carry the run id would flip that run from its documented unscoped fail-open (#1888) to baseline-member RLS — a separate, larger change. The sweep recovers that shape. Tests: plugin-approvals 229, service-automation 365, objectql 1092, lint 390 — all green. Each new guard was mutation-verified: reverting the exemption, the closed terminal set, or the getRun fix turns the matching tests red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv --- .changeset/approval-dead-run-record-lock.md | 57 ++++++ packages/objectql/src/engine.ts | 6 + .../src/approval-service.test.ts | 166 ++++++++++++++++++ .../plugin-approvals/src/approval-service.ts | 145 +++++++++++++++ .../plugin-approvals/src/approvals-plugin.ts | 27 ++- .../plugin-approvals/src/lifecycle-hooks.ts | 16 ++ .../services/service-automation/src/engine.ts | 31 +++- .../src/run-history.test.ts | 66 +++++++ ...runas-grant-resolution.integration.test.ts | 43 +++++ .../src/runtime-identity.ts | 18 +- .../spec/src/contracts/automation-service.ts | 16 ++ packages/spec/src/data/hook.zod.ts | 1 + .../spec/src/kernel/execution-context.zod.ts | 16 ++ 13 files changed, 597 insertions(+), 11 deletions(-) create mode 100644 .changeset/approval-dead-run-record-lock.md diff --git a/.changeset/approval-dead-run-record-lock.md b/.changeset/approval-dead-run-record-lock.md new file mode 100644 index 0000000000..a9da2514f6 --- /dev/null +++ b/.changeset/approval-dead-run-record-lock.md @@ -0,0 +1,57 @@ +--- +"@objectstack/service-automation": patch +"@objectstack/plugin-approvals": patch +"@objectstack/objectql": patch +"@objectstack/spec": patch +--- + +fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456) + +The record lock is keyed on a **pending** `sys_approval_request`, and it could +not tell *the run that owns that request* from *an unrelated user editing the +record*. So a flow that touched its own target record while its own approval was +still pending — a manual `resume` with no decision, or a node that writes the +record between opening the approval and the decision — died on its own +`RECORD_LOCKED`, and the record stayed locked behind the dead run. Recovery +existed (#3424 lets an admin `recall`/`reject` to release it) but nothing made it +self-healing. + +Both halves are now closed. + +**Prevention — the owning run may write its own record.** The automation engine +stamps `flowRunId` onto the run context at setup, alongside `runAs`, and it +travels with every data node's ObjectQL context into `ctx.session`. The lock hook +exempts a write whose `flowRunId` matches the pending request's `flow_run_id`. +It is keyed on run identity rather than elevation on purpose: a `runAs:'user'` +run stays fully RLS-scoped while it writes. `flowRunId` is pure provenance — +server-constructed like `isSystem`, never client-supplied, evaluated by no +security middleware, and the only write it permits is to the one record its own +run already holds a pending request against. + +**Recovery — a sweep releases records held by runs that died anyway.** A pending +request whose owning run has reached a terminal state (`completed`, `failed`, +`cancelled`, `timed_out`) can never be decided, so it is finalised as `recalled` +— releasing the lock — and audited under the reserved actor `system:dead-run` +with the run and its status in the comment, so it is never mistaken for a +submitter's withdrawal. It runs on the existing approvals sweep clock, which also +covers the case no in-band handler can: a run killed by a process crash. + +The sweep is fail-safe by construction. It acts only on an explicit terminal +status from a closed set; `paused` (the normal state of a live approval), +`running`, an unrecognised status, an unknown run, a `getRun` that throws, and a +deployment with no automation engine are all read as "still alive". The failure +mode is "a dead run's lock survives until an admin recalls it" — today's +behaviour — never "a live approval is destroyed". + +Also fixes `AutomationEngine.getRun`, which returned the **first** log entry for +a run id rather than the latest. A run that pauses and later finishes records two +entries under one id, so every suspend-then-finish run — every approval, screen +and wait flow — reported itself as `paused` forever, both on the Runs +observability surface and to this sweep. + +Residual, deliberately not changed here: a `runAs:'user'` run with no trigger +user (a schedule) passes no ObjectQL context at all, so it carries no +`flowRunId` and is still subject to the lock. Manufacturing a context just to +carry the run id would flip that run from its documented unscoped fail-open +(#1888) to baseline-member RLS — a separate, larger change. The sweep is what +recovers that shape. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 01d62a82b3..acf3a8ce45 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -778,6 +778,12 @@ export class ObjectQL implements IDataEngine { // Propagate system-elevated flag so hooks can distinguish engine // self-writes (e.g. approval status mirror) from genuine user writes. ...((execCtx as any).isSystem ? { isSystem: true } : {}), + // Propagate the owning flow run so a hook can recognize writes made BY a + // run it already knows about — the approvals record lock lets the run that + // opened a pending approval write its own target record (#3456). Pure + // provenance: it grants nothing, and unlike `isSystem` it does not widen + // the write's authorization, so a `runAs:'user'` run stays RLS-scoped. + ...((execCtx as any).flowRunId ? { flowRunId: String((execCtx as any).flowRunId) } : {}), // Propagate the automation-suppression flag so the record-change trigger // can skip flow dispatch for seed/bulk writes (ADR: seed loads end-state // data, not user events). `skipAutomations` implies `skipTriggers` — diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index fe4b419aca..c8e638e22b 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -1561,12 +1561,178 @@ describe('record-lock hook (node era)', () => { ).resolves.toBeUndefined(); }); + // ── #3456 prevention half: the lock must not kill the run that owns it ── + + it('allows the OWNING run to write its own target record', async () => { + await expect( + engine.fire('beforeUpdate', { + object: 'opportunity', + input: { id: 'opp1', data: { amount: 200 } }, + // Neither elevated nor admin — the exemption rides on run identity + // alone, so a `runAs:'user'` run stays RLS-scoped while it writes. + session: { isSystem: false, positions: [], userId: 'u1', flowRunId: 'run_1' }, + }), + ).resolves.toBeUndefined(); + }); + + it('still blocks a DIFFERENT run writing the locked record', async () => { + await expect( + engine.fire('beforeUpdate', { + object: 'opportunity', + input: { id: 'opp1', data: { amount: 200 } }, + session: { isSystem: false, positions: [], userId: 'u1', flowRunId: 'run_other' }, + }), + ).rejects.toThrow(/RECORD_LOCKED/); + }); + + it('does not exempt anyone when the pending request carries no run id', async () => { + // A request with no owning run has nothing to match against — a stray + // `flowRunId` must not become a skeleton key. + engine._tables['sys_approval_request'][0].flow_run_id = null; + await expect( + engine.fire('beforeUpdate', { + object: 'opportunity', + input: { id: 'opp1', data: { amount: 200 } }, + session: { isSystem: false, positions: [], userId: 'u1', flowRunId: 'run_1' }, + }), + ).rejects.toThrow(/RECORD_LOCKED/); + }); + it('unbindAllHooks removes the lock hook', () => { expect(unbindAllHooks(engine as any)).toBe(1); expect(engine._hooks['beforeUpdate']).toHaveLength(0); }); }); +// ── #3456 recovery half: release records held by a dead approval run ── +// +// The prevention half above stops a run from dying on its own lock. This sweep +// covers the runs that die anyway — including a process crash, which no in-band +// handler can clean up because the process that would run it is gone. +// +// The load-bearing property is what it must NOT do: a run merely *paused* on its +// approval is the normal state of every live request, so anything short of an +// explicit terminal status has to be read as "alive". +describe('ApprovalService — dead-run release (#3456)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + /** Attach an automation surface whose `getRun` answers with `status`. */ + const withRunStatus = (status: string | null) => + svc.attachAutomation({ getRun: async () => (status == null ? null : { status }) } as any); + + const requestRow = () => engine._tables['sys_approval_request'][0]; + + beforeEach(async () => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + bindApprovalLockHook(engine as any); + await svc.openNodeRequest(openInput(['u9'], {}, { approvalStatusField: 'approval_status' }), CTX); + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100 }]; + }); + + it('releases a pending request whose owning run failed', async () => { + withRunStatus('failed'); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 1 }); + expect(requestRow().status).toBe('recalled'); + expect(requestRow().pending_approvers).toBeNull(); + expect(requestRow().completed_at).toBeTruthy(); + }); + + it('audits the release as a dead-run abandonment, not a submitter recall', async () => { + withRunStatus('failed'); + await svc.releaseDeadRunRequests(); + const action = engine._tables['sys_approval_action'].find((a: any) => a.actor_id === 'system:dead-run'); + expect(action).toBeTruthy(); + expect(action.action).toBe('recall'); + expect(action.comment).toMatch(/run_1/); + expect(action.comment).toMatch(/failed/); + }); + + it('actually unlocks the record — a plain user edit succeeds afterwards', async () => { + // The end-to-end point of the whole sweep. + const edit = () => engine.fire('beforeUpdate', { + object: 'opportunity', + input: { id: 'opp1', data: { amount: 200 } }, + session: { isSystem: false, positions: [], userId: 'u1' }, + }); + await expect(edit()).rejects.toThrow(/RECORD_LOCKED/); // held by the dead run + withRunStatus('failed'); + await svc.releaseDeadRunRequests(); + await expect(edit()).resolves.toBeUndefined(); // released + }); + + it('mirrors the configured status field on release', async () => { + withRunStatus('failed'); + await svc.releaseDeadRunRequests(); + expect(engine._tables['opportunity'][0].approval_status).toBe('recalled'); + }); + + it('leaves a PAUSED run alone — that is a live approval', async () => { + withRunStatus('paused'); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it.each([ + ['an unknown run (null)', null], + ['an unrecognised status', 'reticulating_splines'], + ['a still-running run', 'running'], + ])('leaves the request pending for %s', async (_label, status) => { + withRunStatus(status as any); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it('leaves the request pending when getRun throws', async () => { + svc.attachAutomation({ getRun: async () => { throw new Error('engine unreachable'); } } as any); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it('is a no-op with no automation engine attached', async () => { + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 0, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it('is a no-op when the surface has no getRun (older engine)', async () => { + svc.attachAutomation({ resume: async () => undefined } as any); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 0, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it('skips a request with no owning run', async () => { + requestRow().flow_run_id = null; + withRunStatus('failed'); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 0 }); + expect(requestRow().status).toBe('pending'); + }); + + it.each(['completed', 'cancelled', 'timed_out'])( + 'releases on the other terminal status %s', async (status) => { + // A terminal run can never decide its request, whatever ended it — a + // `completed` one means someone resumed the run out of band. + withRunStatus(status); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 1, released: 1 }); + expect(requestRow().status).toBe('recalled'); + }, + ); + + it('one unreadable request does not stop the sweep', async () => { + await svc.openNodeRequest( + { ...openInput(['u9']), recordId: 'opp2', runId: 'run_2' } as any, CTX, + ); + let call = 0; + svc.attachAutomation({ + getRun: async () => { call++; if (call === 1) throw new Error('boom'); return { status: 'failed' }; }, + } as any); + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 2, released: 1 }); + }); +}); + // ── Out-of-office auto-skip (#1322 M1/M4) ───────────────────────────── // // When a resolved individual approver has declared an active OOO delegation, diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 9aa825114c..3732d5d3f8 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -73,6 +73,19 @@ export interface ApprovalResumeSurface { * which has no reject edge to resume down. */ cancelRun?(runId: string, reason?: string): Promise; + /** + * Look up a run's recorded outcome (#3456). Used by the dead-run sweep to ask + * "is the run behind this pending request still alive?". + * + * The contract that makes the sweep safe is the answer for a run that is + * merely SUSPENDED (the normal state of a run waiting on an approval): the + * engine writes no execution-log entry until a run reaches a terminal state, + * so a suspended run resolves to `null`, never to a status. The sweep + * therefore acts only on an explicit terminal-failure status and treats + * `null` — unknown run, evicted log, no durable store, no automation engine — + * as "still alive". + */ + getRun?(runId: string): Promise<{ status?: string } | null>; } /** @@ -101,6 +114,24 @@ export const ESCALATION_JOB_NAME = 'approvals-sla-escalation'; export const ESCALATION_SCAN_INTERVAL_MS = 5 * 60 * 1000; /** Reserved actor id for machine decisions made by the SLA scanner. */ export const SLA_ACTOR_ID = 'system:sla'; +/** Reserved actor id for requests abandoned because their run died (#3456). */ +export const DEAD_RUN_ACTOR_ID = 'system:dead-run'; +/** + * Run statuses that mean "this run will never resume", so a request still + * pending on it is orphaned (#3456). A CLOSED set, deliberately: the dead-run + * sweep treats every other answer — `paused` (a run waiting on its approval, + * the normal case), `running`, an unknown status, or no answer at all — as + * alive, so an unrecognised state can never cost someone a live approval. + * + * `completed` belongs here with the failure states. The approval node only + * writes a request row on the path where it also suspends the run, and every + * in-band transition (decide / recall / send-back / resubmit) finalises the + * request *before* it resumes the run — so a completed run with a still-pending + * request means the run was resumed out of band and left the request behind. + */ +const TERMINAL_RUN_STATUSES: ReadonlySet = new Set([ + 'completed', 'failed', 'cancelled', 'timed_out', +]); /** Default lifetime of an actionable-link token (ADR-0043). */ export const ACTION_TOKEN_TTL_MS = 72 * 60 * 60 * 1000; @@ -2195,6 +2226,120 @@ export class ApprovalService implements IApprovalService { return { scanned: rows.length, escalated }; } + // ── Dead-run release (#3456) ────────────────────────────────── + + /** + * One dead-run sweep: a pending request whose owning flow run has reached a + * TERMINAL state can never be decided — nothing is left to resume — so the + * request is finalised as `recalled` and, with `lockRecord`, the record it was + * holding is released. + * + * This is the recovery half of #3456. The prevention half is the record lock's + * owning-run exemption (`lifecycle-hooks.ts`), which stops a run from killing + * itself on its own lock in the first place; this sweep cleans up the runs that + * still die — for any reason, including a process crash, which no in-band + * handler can catch because the process that would have run it is gone. + * + * **Fail-safe by construction.** It acts only on an explicit terminal status + * from a closed set. Every other answer — `paused` (the normal state of a run + * waiting on its approval), `running`, an unrecognised status, `null` (unknown + * run, evicted log, no durable store), a `getRun` that throws, or no automation + * engine at all — is read as "still alive" and left strictly alone. The failure + * mode is therefore "a dead run's lock survives until an admin recalls it" + * (today's behaviour, #3424), never "a live approval is destroyed". + * + * `recalled` is the finalisation because it is the platform's existing terminal + * state for *a live request that ended without a decision*; the audit row names + * the real cause and {@link DEAD_RUN_ACTOR_ID} the real actor, so a dead-run + * release is never mistaken for a submitter's withdrawal. + */ + async releaseDeadRunRequests(): Promise<{ scanned: number; released: number }> { + // No liveness oracle → no basis to declare anything dead. + if (typeof this.automation?.getRun !== 'function') return { scanned: 0, released: 0 }; + + let rows: any[] = []; + try { + rows = await this.engine.find('sys_approval_request', { + where: { status: 'pending' }, limit: 500, context: SYSTEM_CTX, + }) ?? []; + } catch (err: any) { + this.logger?.warn?.('[approvals] dead-run sweep failed to list requests', { + error: err?.message ?? String(err), + }); + return { scanned: 0, released: 0 }; + } + + let released = 0; + for (const raw of rows) { + try { + const runId = raw?.flow_run_id ? String(raw.flow_run_id) : ''; + if (!runId) continue; // not node-driven — no run owns it, nothing to check + + let status: string | undefined; + try { + const run = await this.automation.getRun!(runId); + status = typeof run?.status === 'string' ? run.status : undefined; + } catch (err: any) { + // Unknown liveness is NOT death — leave the request pending. + this.logger?.warn?.('[approvals] dead-run sweep could not read run status', { + request: raw?.id, run: runId, error: err?.message ?? String(err), + }); + continue; + } + if (!status || !TERMINAL_RUN_STATUSES.has(status)) continue; + + await this.abandonForDeadRun(raw, runId, status); + released++; + } catch (err: any) { + // One bad row never stops the sweep (mirrors runEscalations). + this.logger?.warn?.('[approvals] dead-run release failed for request', { + request: raw?.id, error: err?.message ?? String(err), + }); + } + } + if (released > 0) { + this.logger?.info?.('[approvals] dead-run sweep', { scanned: rows.length, released }); + } + return { scanned: rows.length, released }; + } + + /** + * Finalise one pending request whose owning run is terminal. Mirrors the + * shape of {@link recall} — audit row first (so a crash mid-release leaves a + * trace of the intent), then the status transition, approver-index sync and + * the optional status-field mirror. No resume/cancel of the run: it is already + * terminal, which is precisely why we are here. + */ + private async abandonForDeadRun(raw: any, runId: string, runStatus: string): Promise { + const org = raw.organization_id ?? null; + const nodeId: string | null = raw.flow_node_id ?? raw.current_step ?? null; + const now = this.clock.now().toISOString(); + + await this.engine.insert('sys_approval_action', { + id: uid('aact'), request_id: raw.id, organization_id: org, + step_name: nodeId, step_index: 0, action: 'recall', + actor_id: DEAD_RUN_ACTOR_ID, + comment: `owning flow run ${runId} is ${runStatus} — request abandoned and record lock released`, + created_at: now, + }, { context: SYSTEM_CTX }); + + await this.engine.update('sys_approval_request', { + id: raw.id, status: 'recalled', pending_approvers: null, completed_at: now, updated_at: now, + }, { context: SYSTEM_CTX }); + await this.syncApproverIndex(raw.id, [], org, now); + + const config = parseJson( + raw.node_config_json, { approvers: [], behavior: 'first_response' } as any, + ); + if (config.approvalStatusField) { + await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, 'recalled'); + } + + this.logger?.warn?.('[approvals] released a record held by a dead approval run', { + request: raw.id, run: runId, runStatus, object: raw.object_name, record: raw.record_id, + }); + } + /** Execute the configured escalation action for one overdue request. */ private async escalateRequest(raw: any, esc: any): Promise { const action: string = esc.action ?? 'notify'; diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 2ca815626e..f83b08bcdd 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -161,12 +161,29 @@ export class ApprovalsServicePlugin implements Plugin { if (!jobs || typeof jobs.schedule !== 'function' || !this.service) return; const svc = this.service; const intervalMs = this.options.escalationScanIntervalMs ?? ESCALATION_SCAN_INTERVAL_MS; - await jobs.schedule(ESCALATION_JOB_NAME, { type: 'interval', intervalMs }, async () => { - await svc.runEscalations(); - }); + // Both sweeps ride this one clock: they walk the same `pending` set, and + // the dead-run release (#3456) is reconciliation with the same "catch up + // after a restart" requirement — a run killed BY the restart is exactly + // the shape no in-band handler can clean up. + // Genuinely independent — an escalation failure must not strand locked + // records, and vice versa, so neither can short-circuit the other. + const sweep = async () => { + const results = await Promise.allSettled([ + svc.runEscalations(), + svc.releaseDeadRunRequests(), + ]); + for (const r of results) { + if (r.status === 'rejected') { + ctx.logger.warn?.('[approvals] periodic sweep leg failed', { + error: (r.reason as any)?.message ?? String(r.reason), + }); + } + } + }; + await jobs.schedule(ESCALATION_JOB_NAME, { type: 'interval', intervalMs }, sweep); this.escalationJobScheduled = true; - void svc.runEscalations().catch((err: any) => { - ctx.logger.warn?.('[approvals] boot escalation sweep failed', { error: err?.message }); + void sweep().catch((err: any) => { + ctx.logger.warn?.('[approvals] boot sweep failed', { error: err?.message }); }); ctx.logger.info('ApprovalsServicePlugin: SLA escalation scan scheduled', { intervalMs }); } catch { /* job service not installed */ } diff --git a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts index caa038916f..2cc86796fd 100644 --- a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts +++ b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts @@ -93,6 +93,22 @@ export function bindApprovalLockHook(engine: MinimalEngine, logger?: MinimalLogg const pending = await pendingRequestFor(engine, object, id); if (!pending) return; + // The run that OPENED this approval may still write its own target record + // (#3456). Without this the lock cannot tell "the run that owns this pending + // request" from "an unrelated user edit", so a flow that touches the record + // between opening the approval and the decision — or a manual `resume` with + // no decision — dies on its own `RECORD_LOCKED` and leaves the record locked + // behind it. + // + // Keyed on run identity, NOT on elevation: a `runAs:'user'` run must stay + // RLS-scoped, so widening it to `isSystem` would be the wrong tool. The + // automation engine stamps `flowRunId` into the server-built ExecutionContext + // (never client-supplied, like `isSystem`) and it grants nothing by itself — + // the only write it permits is to the one record this very run already holds + // a pending request against. + const writerRun = (ctx?.session as any)?.flowRunId; + if (writerRun && pending.flow_run_id && String(writerRun) === String(pending.flow_run_id)) return; + const config = parseJson(pending.node_config_json, {}); if (config?.lockRecord === false) return; diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index b4857fd92f..a83506fe27 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -1543,7 +1543,16 @@ export class AutomationEngine implements IAutomationService { } async getRun(runId: string): Promise { - const inMem = this.executionLogs.find(l => l.id === runId); + // LAST entry wins, not the first: a run that pauses and later finishes + // records TWO entries under the same run id ('paused', then + // 'completed'/'failed'/'cancelled'). Scanning forwards returned the stale + // 'paused' one, so every suspend-then-finish run — i.e. every approval, + // screen and wait flow — reported itself as still paused forever, both on + // the Runs surface and to the approvals dead-run sweep (#3456). + let inMem: ExecutionLogEntry | undefined; + for (let i = this.executionLogs.length - 1; i >= 0; i--) { + if (this.executionLogs[i].id === runId) { inMem = this.executionLogs[i]; break; } + } if (inMem) return inMem; // Durable fallback: after a restart (or ring-buffer eviction) the run's // terminal history row still answers "what happened, at which node?". @@ -1578,8 +1587,17 @@ export class AutomationEngine implements IAutomationService { * `runAs:'system'` to make scheduled elevation explicit (the build-time lint * `flow-schedule-runas-unscoped` flags the same shape earlier). */ - private async resolveRunContext(flow: FlowParsed, context?: AutomationContext): Promise { - const runContext: AutomationContext = { ...(context ?? {}), runAs: flow.runAs ?? 'user' }; + private async resolveRunContext(flow: FlowParsed, context?: AutomationContext, runId?: string): Promise { + // `flowRunId` is stamped alongside `runAs` because it shares that field's + // lifetime and its single construction point: set once here, copied into + // every data node's ObjectQL context by `resolveRunDataContext`, and + // persisted with a suspended run so it survives pause/resume — including a + // cold resume after a restart (#3456). Provenance, not authorization. + const runContext: AutomationContext = { + ...(context ?? {}), + runAs: flow.runAs ?? 'user', + ...(runId ? { flowRunId: runId } : {}), + }; // #3356 (follow-up to #1888) — a `runAs:'user'` run must enforce its data // ops as the TRIGGERING user's real authorization. Most trigger surfaces @@ -1767,7 +1785,9 @@ export class AutomationEngine implements IAutomationService { // elevation is scoped to this run and the caller's identity is restored // when execute() returns). Surfaces the user-less fail-open (see helper) // and resolves the triggering user's real grants for `runAs:'user'` (#3356). - const runContext = await this.resolveRunContext(flow, context); + // Also stamps `flowRunId` so this run's data writes are attributable to it + // (#3456). + const runContext = await this.resolveRunContext(flow, context, runId); try { // Find the start node @@ -3116,7 +3136,8 @@ export class AutomationEngine implements IAutomationService { // ADR-0049 / #1888 — establish the run's effective execution identity // from flow.runAs (see execute() / resolveRunContext); threaded below. - const runContext = await this.resolveRunContext(flow, context); + // `flowRunId` is stamped here too (#3456). + const runContext = await this.resolveRunContext(flow, context, runId); try { const startNode = flow.nodes.find(n => n.type === 'start'); diff --git a/packages/services/service-automation/src/run-history.test.ts b/packages/services/service-automation/src/run-history.test.ts index 43273be924..21a9b4779f 100644 --- a/packages/services/service-automation/src/run-history.test.ts +++ b/packages/services/service-automation/src/run-history.test.ts @@ -120,6 +120,72 @@ describe('automation run history (durable observability)', () => { expect(await engine.getRun('run_nope')).toBeNull(); }); + // A run that pauses and later finishes records TWO log entries under the + // same run id ('paused', then the terminal one). getRun scanned forwards and + // returned the stale 'paused' entry, so every suspend-then-finish run — i.e. + // every approval / screen / wait flow — reported itself as still paused + // forever, on the Runs surface and to the #3456 dead-run sweep alike. + describe('getRun after a pause (latest entry wins)', () => { + function pausingFlow(name: string, tail: string) { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 's' }, + { id: 'hold', type: 'hold', label: 'h' }, + { id: 'tail', type: tail, label: 't' }, + { id: 'end', type: 'end', label: 'e' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'hold' }, + { id: 'e2', source: 'hold', target: 'tail' }, + { id: 'e3', source: 'tail', target: 'end' }, + ], + }; + } + + const holdExecutor = { + type: 'hold', + async execute() { return { success: true, suspend: true, correlation: 'held' }; }, + } as never; + + it('reports the terminal status once a paused run completes', async () => { + const engine = new AutomationEngine(silent, new InMemorySuspendedRunStore()); + engine.registerNodeExecutor(holdExecutor); + engine.registerNodeExecutor({ type: 'noop', async execute() { return { success: true }; } } as never); + engine.registerFlow('held_ok', pausingFlow('held_ok', 'noop') as never); + + const paused = await engine.execute('held_ok', { event: 'test' } as AutomationContext); + expect(paused.status).toBe('paused'); + const runId = paused.runId!; + // Accurate while it really is paused — this is the signal the sweep + // reads as "alive", so it must not regress either. + expect((await engine.getRun(runId))!.status).toBe('paused'); + + expect((await engine.resume(runId)).success).toBe(true); + await flush(); + expect((await engine.getRun(runId))!.status).toBe('completed'); + }); + + it('reports failed once a paused run dies after resuming', async () => { + const engine = new AutomationEngine(silent, new InMemorySuspendedRunStore()); + engine.registerNodeExecutor(holdExecutor); + engine.registerNodeExecutor({ + type: 'boom', async execute() { throw new Error('kaboom'); }, + } as never); + engine.registerFlow('held_bad', pausingFlow('held_bad', 'boom') as never); + + const paused = await engine.execute('held_bad', { event: 'test' } as AutomationContext); + const runId = paused.runId!; + expect(paused.status).toBe('paused'); + + expect((await engine.resume(runId)).success).toBe(false); + await flush(); + // The exact shape #3456's sweep needs: a dead approval run must be + // recognisable as dead, not as forever-paused. + expect((await engine.getRun(runId))!.status).toBe('failed'); + }); + }); + it('caps terminal history per flow (retention stop-gap, #2585)', async () => { const store = new InMemorySuspendedRunStore({ maxTerminalRunsPerFlow: 2 }); const engine = new AutomationEngine(silent, store); diff --git a/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts b/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts index 6bc6556c96..ba50477788 100644 --- a/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts +++ b/packages/services/service-automation/src/runas-grant-resolution.integration.test.ts @@ -124,3 +124,46 @@ describe("AutomationServicePlugin bridges the runAs:'user' grant resolver (#3356 await kernel.shutdown(); }); }); + +// The provenance band #3456 rides on: a run's data writes must be attributable +// to the run that made them, all the way down to the ObjectQL context — without +// it the approvals record lock cannot tell the owning run from a stranger. +describe('a run stamps its own id onto its data ops (#3456)', () => { + it.each([ + ["runAs:'user'", 'user'], + ["runAs:'system'", 'system'], + ] as const)('%s carries flowRunId matching the run id', async (_label, runAs) => { + const { engine: ql, crud } = fakeObjectQl(AUTHZ_TABLES); + const kernel = await bootWithObjectQl(ql); + const automation = kernel.getService('automation'); + automation.registerFlow(`stamp_${runAs}`, updateFlow(`stamp_${runAs}`, runAs) as never); + + const res = await automation.execute(`stamp_${runAs}`, { userId: 'u1', params: { noteId: 'n1' } }); + expect(res.success, `run failed: ${JSON.stringify(res)}`).toBe(true); + + // `result.runId` is only surfaced for a PAUSED run, so cross-check against + // the engine's own run log — the id a later `getRun` would be asked about, + // which is exactly the join the dead-run sweep depends on. + const [logged] = await automation.listRuns(`stamp_${runAs}`, { limit: 1 }); + const update = crud.find((c) => c.op === 'update' && c.obj === 'runas_thing'); + expect(update!.ctx.flowRunId, 'the data op carried no run provenance').toBeTruthy(); + expect(update!.ctx.flowRunId).toBe(logged.id); + + await kernel.shutdown(); + }); + + it('does not leak the stamp back onto the caller-supplied context', async () => { + const { engine: ql } = fakeObjectQl(AUTHZ_TABLES); + const kernel = await bootWithObjectQl(ql); + const automation = kernel.getService('automation'); + automation.registerFlow('nomutate', updateFlow('nomutate', 'user') as never); + + // resolveRunContext copies rather than mutates, so a caller reusing one + // context object across runs can never inherit a stale run id. + const caller: any = { userId: 'u1', params: { noteId: 'n1' } }; + await automation.execute('nomutate', caller); + expect(caller.flowRunId).toBeUndefined(); + + await kernel.shutdown(); + }); +}); diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 85bfec6c2c..33bde71881 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -20,6 +20,13 @@ export interface RunDataContext { permissions: string[]; /** Acting user's tenant/org id. */ tenantId?: string; + /** + * The run performing this operation (#3456). Provenance only — it is not part + * of the identity the security middleware evaluates, so it neither widens nor + * narrows what the run may touch. Hooks use it to recognize a run's writes to + * state that run itself opened (the approvals record lock). + */ + flowRunId?: string; } /** @@ -44,9 +51,17 @@ export interface RunDataContext { * by every data-touching node so the policy can't drift between node types. */ export function resolveRunDataContext(context: AutomationContext | undefined): RunDataContext | undefined { + const flowRunId = context?.flowRunId; if (context?.runAs === 'system') { - return { isSystem: true, positions: [], permissions: [] }; + return { isSystem: true, positions: [], permissions: [], ...(flowRunId ? { flowRunId } : {}) }; } + // NOTE (#3456): the identity-less case below returns `undefined`, so a + // schedule-triggered `runAs:'user'` run with no user carries NO context at all + // — and therefore no `flowRunId` either, leaving it subject to the approvals + // record lock on its own target record. Manufacturing a context here just to + // carry the run id would flip that run from the documented unscoped fail-open + // (#1888) to baseline-member RLS — a separate, larger behavior change. The + // dead-run sweep in plugin-approvals is what recovers this shape. if (!context?.userId) return undefined; // `context` is now narrowed to a defined AutomationContext with a userId. const out: RunDataContext = { @@ -56,6 +71,7 @@ export function resolveRunDataContext(context: AutomationContext | undefined): R permissions: Array.isArray(context.permissions) ? context.permissions : [], }; if (context.tenantId) out.tenantId = context.tenantId; + if (flowRunId) out.flowRunId = flowRunId; return out; } diff --git a/packages/spec/src/contracts/automation-service.ts b/packages/spec/src/contracts/automation-service.ts index 6152c5b13a..9a93193ce0 100644 --- a/packages/spec/src/contracts/automation-service.ts +++ b/packages/spec/src/contracts/automation-service.ts @@ -64,6 +64,22 @@ export interface AutomationContext { * Callers do NOT set this — the engine derives it from the flow definition. */ runAs?: 'system' | 'user'; + /** + * Id of the run this context belongs to, stamped by the engine at run setup + * alongside {@link runAs} and carried into every data node's ObjectQL + * `context` (see `resolveRunDataContext` in @objectstack/service-automation). + * It is persisted with a suspended run, so it survives a pause/resume round + * trip — including a cold resume after a process restart. + * + * Provenance, not authorization: it grants nothing and no security + * middleware keys on it. A hook uses it to recognize writes made BY a given + * run — the approvals record lock (#3456) lets the run that opened a pending + * approval write its own target record, which it otherwise cannot tell apart + * from an unrelated user's edit. + * + * Callers do NOT set this — the engine derives it, exactly like {@link runAs}. + */ + flowRunId?: string; /** Additional contextual data */ params?: Record; } diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index 8c94d9f033..9dcf322745 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -228,6 +228,7 @@ export const HookContextSchema = lazySchema(() => z.object({ roles: z.array(z.string()).optional(), accessToken: z.string().optional(), isSystem: z.boolean().optional().describe('True when the call was made with an elevated system context (engine self-writes)'), + flowRunId: z.string().optional().describe('Id of the automation flow run performing this write, when it originates from a flow data node. Provenance only — grants nothing, no security middleware keys on it. Lets a hook tell the run that OWNS some externalized state (e.g. a pending approval) from an unrelated caller (#3456).'), skipTriggers: z.boolean().optional().describe('True when record-change automation (flow triggers) must be suppressed for this write — e.g. package seed replay. Lifecycle hooks still run.'), skipAutomations: z.boolean().optional().describe('True when metadata-bound automation hooks must be suppressed for this write — e.g. data import with "run automations" unchecked, or import undo. Implies skipTriggers; code-registered system hooks (audit, security) still run.'), }).optional().describe('Current session context'), diff --git a/packages/spec/src/kernel/execution-context.zod.ts b/packages/spec/src/kernel/execution-context.zod.ts index 655b738a89..580bfd52f5 100644 --- a/packages/spec/src/kernel/execution-context.zod.ts +++ b/packages/spec/src/kernel/execution-context.zod.ts @@ -192,6 +192,22 @@ export const ExecutionContextSchema = lazySchema(() => z.object({ /** Whether this is a system-level operation (bypasses permission checks) */ isSystem: z.boolean().default(false), + /** + * Id of the automation flow RUN performing this operation, when the write + * originates from a flow's data node. Server-constructed by the automation + * engine at run setup and threaded through `resolveRunDataContext` — never + * client-supplied, exactly like {@link isSystem} (this envelope is built from + * the authenticated session, not from request input). + * + * Provenance, NOT authorization: it grants nothing on its own and no security + * middleware keys on it. It exists so a hook can tell "the run that owns this + * externalized state" from "an unrelated caller" — the approvals record lock + * (#3456) uses it to let the run that opened a pending approval still write + * its own target record, which `isSystem` cannot express for a + * `runAs:'user'` run without elevating it. + */ + flowRunId: z.string().optional(), + /** * Suppress record-change AUTOMATION (autolaunched flow triggers: * record-after-insert / -update / -delete) for writes made under this From a52fde257bd6a389cf4dbef62dc40f6a9d87c762 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 15:12:55 +0000 Subject: [PATCH 2/2] docs(approvals): regenerate the ExecutionContext reference; document the lock's two new behaviours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `flowRunId` field added to `ExecutionContextSchema` left the GENERATED `content/docs/references/kernel/execution-context.mdx` stale, failing the spec `check:docs` gate. Regenerated via the prescribed path (`gen:schema && gen:docs`) — a one-row addition to the field table. Also updates the hand-written approvals guide, which described the record lock as absolute and predated any automatic recovery: - the lock now exempts the run that opened the request, and the exemption is keyed on run identity rather than elevation, so a `runAs:'user'` run stays RLS-scoped while it writes; - a dead run's lock is released by the sweep, with the note that it acts only on a positively-confirmed terminal run — a paused run, an unknown run, or an unreachable engine all count as alive. `check:docs` and `check:doc-authoring` both green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv --- content/docs/automation/approvals.mdx | 20 +++++++++++++++++++ .../references/kernel/execution-context.mdx | 1 + 2 files changed, 21 insertions(+) diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index b803113df9..c946dd18a9 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -210,6 +210,12 @@ The node writes a `sys_approval_request` row: `status: 'pending'`, record is **locked** against edits while pending (`lockRecord`, default `true`), and the flow run parks until a decision arrives. +The lock applies to everyone *except the run that opened the request*. A flow may +still write its own target record while its own approval is pending, so it can +never deadlock against itself. The exemption is keyed on run identity rather than +elevation, so a `runAs:'user'` run stays row-level-security scoped while it +writes — it does not become a system write. + Only `approvers` is required on the node; everything else has a default (`behavior: 'first_response'`, `lockRecord: true`, `maxRevisions: 3`). @@ -383,6 +389,20 @@ and is audited under the admin's own id. Prefer a guaranteed-staffed fallback approver so the set is never empty in the first place. + +**A dead run releases its own lock.** If the flow run that opened an approval +reaches a terminal state without a decision — it failed, was cancelled, timed +out, or the process hosting it crashed — nothing is left to decide the request, +so a periodic sweep finalizes it as `recalled` and releases the record. The +audit row records the actor `system:dead-run` and names the run and its status, +so it reads distinctly from a submitter's own recall. + +The sweep only ever acts on a run it can positively confirm is terminal: a +paused run (the normal state of a live approval), an unknown run, or an +unreachable automation engine all count as *alive* and are left untouched. It +frees orphaned records; it never cancels a live approval. + + ### Progress and notification deep links A pending multi-approver request also carries a **server-computed diff --git a/content/docs/references/kernel/execution-context.mdx b/content/docs/references/kernel/execution-context.mdx index ebe6e4378f..095159246a 100644 --- a/content/docs/references/kernel/execution-context.mdx +++ b/content/docs/references/kernel/execution-context.mdx @@ -66,6 +66,7 @@ const result = ExecutionContext.parse(data); | **accessible_org_ids** | `string[]` | optional | | | **rlsMembership** | `Record` | optional | | | **isSystem** | `boolean` | ✅ | | +| **flowRunId** | `string` | optional | | | **skipTriggers** | `boolean` | optional | | | **skipAutomations** | `boolean` | optional | | | **seedReplay** | `boolean` | optional | |