diff --git a/.changeset/approval-dead-run-ordering-invariant.md b/.changeset/approval-dead-run-ordering-invariant.md new file mode 100644 index 0000000000..74b600f024 --- /dev/null +++ b/.changeset/approval-dead-run-ordering-invariant.md @@ -0,0 +1,11 @@ +--- +--- + +test(approvals): pin the ordering invariant the dead-run sweep rests on (#3456 +follow-up) — releases nothing. + +Tests plus one comment reference; no runtime behaviour changes, so no package +needs a version bump. The new coverage asserts that every in-band approval +transition moves its request out of `pending` before it hands the run back — +the unenforced premise `releaseDeadRunRequests` relies on to tell an orphaned +request from a live one. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index c8e638e22b..e9a7821e04 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -2390,3 +2390,124 @@ describe('ApprovalService — participant visibility (#3590)', () => { expect(res.request.status).toBe('approved'); }); }); + +// ── The ordering invariant the dead-run sweep rests on (#3456) ───────── +// +// `releaseDeadRunRequests` recalls a PENDING request whose owning run has +// reached a TERMINAL state, on the premise that such a pair can only be an +// orphan. That premise is not self-evident — it holds only because every +// in-band transition moves the request OUT of `pending` before it hands the run +// back. Resume first and a run that finishes promptly afterwards would be +// indistinguishable from an orphan, so the sweep would cancel a LIVE approval — +// precisely the one failure mode it is built never to have. +// +// Nothing enforces that ordering: it is a convention spread across four public +// methods and seven resume/cancelRun call sites, any of which a refactor could +// reorder without a single existing test going red. So pin the invariant +// itself rather than the call order of any one method — at the instant the run +// is handed back, no request owned by that run may still be `pending`. +describe('in-band transitions finalise before they resume (#3456 invariant)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + /** One entry per hand-back, with any still-pending requests owned by the run. */ + let handoffs: Array<{ hook: string; stillPending: string[] }>; + + /** A flow whose approval node declares the `revise` out-edge send-back needs. */ + const REVISE_FLOW = { + name: 'deal_approval', + edges: [{ id: 'e_rev', source: 'approve_step', target: 'wait_revision', label: 'revise' }], + }; + + function recordHandoff(hook: string) { + const rows = (engine._tables['sys_approval_request'] ?? []) as any[]; + handoffs.push({ + hook, + stillPending: rows + .filter(r => String(r.flow_run_id ?? '') === 'run_1' && r.status === 'pending') + .map(r => String(r.id)), + }); + } + + /** Assert every hand-back this scenario made was clean. */ + function expectCleanHandoffs() { + expect( + handoffs.length, + 'the run was never handed back — this scenario did not exercise the invariant', + ).toBeGreaterThan(0); + for (const h of handoffs) { + expect( + h.stillPending, + `${h.hook}() handed run_1 back while it still owned a pending request — ` + + 'the dead-run sweep would treat that as an orphan and cancel a live approval', + ).toEqual([]); + } + } + + beforeEach(async () => { + engine = makeFakeEngine(); + n = 0; + handoffs = []; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + svc.attachAutomation({ + async resume() { recordHandoff('resume'); }, + async cancelRun() { recordHandoff('cancelRun'); }, + async getFlow() { return REVISE_FLOW; }, + } as any); + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100 }]; + }); + + const open = (configExtra: Record = {}) => + svc.openNodeRequest(openInput(['u9'], {}, configExtra), CTX); + + it('decide(approve) finalises before resuming', async () => { + const req = await open(); + await svc.decide(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + expectCleanHandoffs(); + }); + + it('decide(reject) finalises before resuming', async () => { + const req = await open(); + await svc.decide(req.id, { decision: 'reject', actorId: 'u9' }, SYS); + expectCleanHandoffs(); + }); + + it('recall finalises before resuming', async () => { + const req = await open(); + await svc.recall(req.id, { actorId: 'u1' }, CTX); + expectCleanHandoffs(); + }); + + it('sendBack finalises before resuming', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9', comment: 'fix the totals' }, CTX); + expectCleanHandoffs(); + }); + + it('sendBack past the revision budget auto-rejects before resuming', async () => { + // `maxRevisions: 0` takes the ADR-0044 loop-guard branch on the first + // send-back — a separate resume site from the normal path above. + const req = await open({ maxRevisions: 0 }); + const out = await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + expect(out.autoRejected, 'expected the auto-reject branch').toBe(true); + expectCleanHandoffs(); + }); + + it('recall inside the revise window cancels the run without a pending request', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + handoffs = []; // isolate the recall's own hand-back + await svc.recall(req.id, { actorId: 'u1' }, CTX); + expect(handoffs.map(h => h.hook)).toContain('cancelRun'); + expectCleanHandoffs(); + }); + + it('resubmit re-enters the node without leaving the old request pending', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + handoffs = []; // isolate the resubmit's own hand-back + await svc.resubmit(req.id, { actorId: 'u1' }, CTX); + expectCleanHandoffs(); + }); +}); diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 33bde71881..3bf8adafb6 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -55,13 +55,14 @@ export function resolveRunDataContext(context: AutomationContext | undefined): R if (context?.runAs === 'system') { 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. + // NOTE (#3456 residual, tracked as #3712): 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 = {