fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456)

.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.

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.
 </Callout>
 
+<Callout type="info">
+**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.
+</Callout>
+
 ### Progress and notification deep links
 
 A pending multi-approver request also carries a **server-computed

content/docs/references/kernel/execution-context.mdx

@@ -66,6 +66,7 @@ const result = ExecutionContext.parse(data);
 | **accessible_org_ids** | `string[]` | optional | |
 | **rlsMembership** | `Record<string, string[]>` | optional | |
 | **isSystem** | `boolean` | ✅ | |
+| **flowRunId** | `string` | optional | |
 | **skipTriggers** | `boolean` | optional | |
 | **skipAutomations** | `boolean` | optional | |
 | **seedReplay** | `boolean` | optional | |

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` —

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<typeof makeFakeEngine>;
+ 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,