fix(automation,approvals): 审批决策不能在流程原地不动的情况下"成功" (#4420)

.changeset/approval-decision-survives-restart.md

@@ -0,0 +1,64 @@
+---
+"@objectstack/spec": minor
+"@objectstack/service-automation": minor
+"@objectstack/plugin-approvals": minor
+"@objectstack/rest": patch
+"@objectstack/runtime": patch
+---
+
+fix(automation,approvals): an approval decision can no longer succeed while its flow stays parked (#4420)
+
+A flow paused at an `approval` node, a deploy, then an approver clicking
+Approve: the request row flipped to `approved`, the UI toasted success — and
+the flow never moved. No next-stage request, no error, the record's mirrored
+status frozen mid-workflow. Approval flows pause for days by design, so a
+restart mid-flight is the normal case: every release could quietly zombify
+every in-flight approval, with the approvers none the wiser.
+
+Durable suspended runs (#1518) had shipped and were not the missing piece. Two
+other things were.
+
+**The wiring could enable a store over a table nobody had created.** Object
+registration and store activation resolve different services in different
+phases — `manifest` at `init()`, `objectql` at `start()` — and the plugin
+declared no ordering. Composed ahead of ObjectQL, `init()` found no `manifest`,
+warned, and continued; `start()` then attached the DB-backed store anyway. Every
+suspend failed with `no such table: sys_automation_run` into a log line nobody
+read, pauses silently stayed in memory, and the next restart lost them all.
+Now: `AutomationServicePlugin` declares `optionalDependencies:
+['com.objectstack.engine.objectql']` (order-if-present, per ADR-0116 — an
+engine-less kernel must still boot); a registration missed at `init()` is
+retried at `start()`, which still lands before ObjectQL's schema sync; the
+store is never attached when registration did not happen, and says so at
+**error** level instead of warning; the table is probed once at boot so a
+broken setup surfaces there rather than one failed write at a time; and a
+failed durable write of a paused run is logged at error — it is data loss in
+waiting, not a warning.
+
+**A reported resume failure read as success.** `AutomationEngine.resume()`
+answers a lost run by *returning* `{ success: false }`, never by throwing.
+`ApprovalService` discarded that return value, and `decide()` counted only a
+thrown error as failure — so a decision against a dead run came back
+`resumed: true`, HTTP 200. Resume failures are now classified
+(`RUN_NOT_FOUND`, `STORE_UNAVAILABLE`, `RESUME_IN_PROGRESS`, joining
+`PERMISSION_DENIED` / `INVALID_SIGNAL`), so a run that is gone for good is
+distinguishable from a store that is merely unreachable, and the raw resume
+route maps them to 404 / 503 / 409.
+
+Approvals acts on them. A new `AutomationEngine.hasSuspendedRun(runId)` — which
+reads the suspension store, unlike `getRun()`, and throws rather than answering
+`false` when the store is unreadable — pre-flights every flow-advancing
+operation (`decide`, `sendBack`, `resubmit`) **before its first write**, so the
+zombie half-state is never created rather than merely reported: the decision
+fails with `RESUME_TARGET_LOST` (HTTP 409) and the request stays actionable. A
+resume that fails after the decision is durable can no longer be undone, but it
+now throws `RESUME_FAILED` (HTTP 500) naming the stranded run instead of
+reporting success. A concurrent duplicate resume stays benign — the engine's
+idempotency guard is doing its job — and reports through the new optional
+`resumeError` field. Recall and revise-window cancellation stay non-fatal by
+design (they abandon the request), but log at error with the reason instead of
+swallowing it. Compositions with no automation engine attached are unaffected.
+
+Existing zombie requests from affected deployments (already `approved`, run
+stranded) are not repaired by this change — `releaseDeadRunRequests` only
+sweeps requests that are still `pending`.

content/docs/references/api/error-code-ledger.mdx

@@ -280,13 +280,17 @@ const result = ErrorCode.parse(data);
 * `REQUEST_NOT_FOUND`
 * `RESEED_NO_ROWS`
 * `RESEED_SKIPPED`
+* `RESUME_FAILED`
+* `RESUME_IN_PROGRESS`
+* `RESUME_TARGET_LOST`
 * `ROUTE_NOT_FOUND`
 * `RULE_DEFINE_FAILED`
 * `RULE_DELETE_FAILED`
 * `RULE_EVALUATE_FAILED`
 * `RULE_GET_FAILED`
 * `RULE_LIST_FAILED`
 * `RULE_NOT_FOUND`
+* `RUN_NOT_FOUND`
 * `SAML_REGISTER_FAILED`
 * `SCHEDULES_LIST_FAILED`
 * `SCHEDULE_DELETE_FAILED`
@@ -303,6 +307,7 @@ const result = ErrorCode.parse(data);
 * `SIGN_IN_REQUIRED`
 * `SSO_REGISTER_FAILED`
 * `SSO_REGISTER_FORBIDDEN`
+* `STORE_UNAVAILABLE`
 * `SUGGESTION_CONFIRM_FAILED`
 * `SUGGESTION_DISMISS_FAILED`
 * `SUGGESTION_LIST_FAILED`

packages/plugins/plugin-approvals/src/approval-restart-resume.test.ts

@@ -0,0 +1,254 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Approval decisions across a process restart (#4420).
+ *
+ * The reported failure: a flow parks at an `approval` node in process A, the
+ * process restarts, and the approver clicks Approve in process B. The request
+ * row flips to `approved`, the UI toasts success — and the flow never moves.
+ * The next stage's request is never opened, the record's mirrored status
+ * freezes mid-workflow, and nothing anywhere logs an error. Approval flows
+ * pause for days by design, so a deploy in the middle is the normal case, not
+ * the edge one: every release could silently zombify every in-flight approval.
+ *
+ * Two independent defects produced it, and both are pinned here:
+ *
+ *  1. The run state has to SURVIVE the restart (#1518's durable store, driven
+ *     end to end through the approvals surface for the first time — the store's
+ *     own suite proves the engine half, not that a decision can cross it).
+ *  2. When it did not survive, every layer read the failure as success:
+ *     `engine.resume()` REPORTS `{ success: false }` rather than throwing,
+ *     `serviceResume` discarded that return value, and `decide()` counted only
+ *     a thrown error as failure — so it answered `resumed: true`, HTTP 200.
+ *
+ * The fix refuses the decision instead: the run is checked BEFORE the decision
+ * is written, so the zombie half-state (recorded decision + stranded run) is
+ * never created rather than merely reported.
+ */
+
+import { describe, it, expect, beforeEach } from 'vitest';
+import { AutomationEngine, InMemorySuspendedRunStore } from '@objectstack/service-automation';
+import { ApprovalService } from './approval-service.js';
+import { registerApprovalNode } from './approval-node.js';
+
+const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any;
+const noopLogger = { info() {}, warn() {}, error() {}, debug() {} };
+
+/** In-memory ObjectQL stand-in — the approvals tables outlive the "restart". */
+function makeFakeEngine() {
+  const tables = new Map<string, any[]>();
+  const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!));
+  const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => {
+    if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]);
+    if (v && typeof v === 'object' && '$ne' in (v as any)) return row[k] !== (v as any).$ne;
+    return row[k] === v;
+  });
+  return {
+    tables,
+    async find(object: string, opts: any = {}) {
+      const where = opts.where ?? opts.filter ?? {};
+      let out = rows(object).filter(r => matches(r, where));
+      if (opts.limit) out = out.slice(0, opts.limit);
+      return out.map(r => ({ ...r }));
+    },
+    async insert(object: string, data: any) { rows(object).push({ ...data }); return { ...data }; },
+    async update(object: string, idOrData: any) {
+      const row = rows(object).find(r => r.id === idOrData.id);
+      if (row) Object.assign(row, idOrData);
+      return row ? { ...row } : null;
+    },
+    async delete(object: string, opts: any = {}) {
+      const list = rows(object);
+      for (let i = list.length - 1; i >= 0; i--) if (matches(list[i], opts.where ?? {})) list.splice(i, 1);
+      return { affected: 1 };
+    },
+  };
+}
+
+function registerDecisionFlow(engine: AutomationEngine) {
+  engine.registerFlow('deal_approval', {
+    name: 'deal_approval',
+    label: 'Deal Approval',
+    type: 'autolaunched',
+    nodes: [
+      { id: 'start', type: 'start', label: 'Start' },
+      { id: 'approve_step', type: 'approval', label: 'Manager Approval', config: { approvers: [{ type: 'user', value: 'u1' }] } },
+      { id: 'on_approved', type: 'mark', label: 'Approved' },
+      { id: 'on_rejected', type: 'mark', label: 'Rejected' },
+      { id: 'end', type: 'end', label: 'End' },
+    ],
+    edges: [
+      { id: 'e1', source: 'start', target: 'approve_step' },
+      { id: 'e2', source: 'approve_step', target: 'on_approved', label: 'approve' },
+      { id: 'e3', source: 'approve_step', target: 'on_rejected', label: 'reject' },
+      { id: 'e4', source: 'on_approved', target: 'end' },
+      { id: 'e5', source: 'on_rejected', target: 'end' },
+    ],
+  } as never);
+}
+
+describe('approval decisions across a process restart (#4420)', () => {
+  let data: ReturnType<typeof makeFakeEngine>;
+  let service: ApprovalService;
+  let marks: string[];
+
+  /**
+   * One process lifetime: a fresh engine over the shared approvals tables,
+   * optionally sharing a durable suspended-run store with earlier lifetimes.
+   * Nothing carries over in memory — that is the whole point.
+   */
+  function boot(store?: InMemorySuspendedRunStore) {
+    const automation = new AutomationEngine(noopLogger as any, store);
+    registerApprovalNode(automation, service, noopLogger as any);
+    automation.registerNodeExecutor({
+      type: 'mark',
+      async execute(node: any) { marks.push(node.id); return { success: true }; },
+    });
+    registerDecisionFlow(automation);
+    service.attachAutomation(automation);
+    return automation;
+  }
+
+  const pendingRequest = async () =>
+    (await data.find('sys_approval_request', { where: { status: 'pending' } }))[0];
+
+  beforeEach(() => {
+    marks = [];
+    data = makeFakeEngine();
+    service = new ApprovalService({ engine: data as any, logger: noopLogger });
+  });
+
+  it('approves a run that paused in a previous process, and the flow advances', async () => {
+    // #1518's promise, exercised the way a user reaches it: submit in one
+    // process, decide in the next.
+    const store = new InMemorySuspendedRunStore();
+    const processA = boot(store);
+    const paused = await processA.execute('deal_approval', {
+      object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter',
+    });
+    expect(paused.status).toBe('paused');
+    const request = await pendingRequest();
+
+    // ── restart ── nothing of process A survives except the two stores.
+    const processB = boot(store);
+    expect(processB.listSuspendedRuns(), 'no in-memory state carried over').toHaveLength(0);
+
+    const out = await service.decide(request.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX);
+
+    expect(out).toMatchObject({ finalized: true, decision: 'approve', resumed: true });
+    expect(marks, 'the flow continued down the approve branch').toEqual(['on_approved']);
+    expect((await data.find('sys_approval_request', { where: { id: request.id } }))[0].status).toBe('approved');
+  });
+
+  it('refuses the decision, writing nothing, when the run did not survive the restart', async () => {
+    // Process A keeps its pauses in memory only — the 17.0.0-rc.1 deployment.
+    const processA = boot();
+    await processA.execute('deal_approval', {
+      object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter',
+    });
+    const request = await pendingRequest();
+
+    // ── restart ── the suspension is gone for good.
+    boot();
+
+    await expect(
+      service.decide(request.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX),
+    ).rejects.toThrow(/RESUME_TARGET_LOST/);
+
+    // The half-state is not merely reported — it is never created. The request
+    // is still actionable once an operator sorts the run out, and the audit
+    // trail does not claim an approval that had no effect.
+    const after = (await data.find('sys_approval_request', { where: { id: request.id } }))[0];
+    expect(after.status, 'still pending, not a zombie "approved"').toBe('pending');
+    expect(after.pending_approvers).toContain('u1');
+    expect(
+      await data.find('sys_approval_action', { where: { request_id: request.id, action: 'approve' } }),
+      'no approval was audited for a decision that could not take effect',
+    ).toHaveLength(0);
+    expect(marks).toEqual([]);
+  });
+
+  it('names the stranded run when the resume fails after the decision was written', async () => {
+    // The residual race: the run passes the pre-flight and dies before the
+    // resume. The decision IS durable by then, so this cannot be undone — but
+    // it must not read as success, and it must name what needs rescuing.
+    const processA = boot();
+    await processA.execute('deal_approval', {
+      object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter',
+    });
+    const req = await pendingRequest();
+
+    service.attachAutomation({
+      hasSuspendedRun: async () => true,
+      resume: async () => ({ success: false, code: 'RUN_NOT_FOUND', error: `No suspended run 'run_x'` }),
+    } as any);
+
+    const err = await service
+      .decide(req.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX)
+      .then(() => null, (e: Error) => e);
+
+    expect(err?.message).toMatch(/^RESUME_FAILED/);
+    expect(err?.message, 'says which run an operator has to rescue').toMatch(
+      /could not be resumed and is now stranded/,
+    );
+    // The decision itself stands — it is durable, and pretending otherwise
+    // would put the row and the audit trail out of step.
+    expect((await data.find('sys_approval_request', { where: { id: req.id } }))[0].status).toBe('approved');
+  });
+
+  it('treats a concurrent duplicate resume as benign, not as a failure', async () => {
+    const processA = boot();
+    await processA.execute('deal_approval', {
+      object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter',
+    });
+    const req = await pendingRequest();
+
+    // The engine's own idempotency guard: another caller is already advancing
+    // this run. Surfacing that as an error would turn a working safeguard into
+    // a user-visible failure.
+    service.attachAutomation({
+      hasSuspendedRun: async () => true,
+      resume: async () => ({ success: false, code: 'RESUME_IN_PROGRESS', error: `Run 'run_x' is already being resumed` }),
+    } as any);
+
+    const out = await service.decide(req.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX);
+    expect(out.finalized).toBe(true);
+    expect(out.resumed).toBe(false);
+    expect(out.resumeError).toMatch(/already being resumed/);
+  });
+
+  it('does not block a decision when the suspended-run store is merely unreachable', async () => {
+    const processA = boot();
+    await processA.execute('deal_approval', {
+      object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter',
+    });
+    const req = await pendingRequest();
+    const realResume = service['automation']!.resume!.bind(service['automation']);
+
+    // A transient outage means "unknown", not "dead". Failing closed here would
+    // reject every decision in the tenant for the duration of a blip.
+    service.attachAutomation({
+      hasSuspendedRun: async () => { throw new Error('connection refused'); },
+      resume: realResume,
+    } as any);
+
+    const out = await service.decide(req.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX);
+    expect(out).toMatchObject({ finalized: true, resumed: true });
+    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 });
+    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,
+    }, SYSTEM_CTX);
+
+    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);
+  });
+});

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -916,6 +916,43 @@ describe('ApprovalService (node era)', () => {
     expect(out.resumed).toBe(false);
   });
 
+  // ── #4420: a REPORTED resume failure is a failure ────────────────
+  //
+  // The engine answers a lost run with `{ success: false }` rather than by
+  // throwing, so every one of these used to come back as `resumed: true`.
+
+  it('decide: refuses before writing anything when the run is already gone', async () => {
+    svc.attachAutomation({
+      async hasSuspendedRun() { return false; },
+      async resume() { return { success: true }; },
+    } as any);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+
+    await expect(svc.decide(req.id, { decision: 'approve', actorId: 'u9' }, SYS))
+      .rejects.toThrow(/RESUME_TARGET_LOST/);
+    const [row] = await engine.find('sys_approval_request', { where: { id: req.id } });
+    expect(row.status, 'nothing recorded against a run that cannot advance').toBe('pending');
+  });
+
+  it('decide: fails loudly when a resume reports failure without throwing', async () => {
+    svc.attachAutomation({
+      async resume() { return { success: false, code: 'RUN_NOT_FOUND', error: `No suspended run 'run_1'` }; },
+    } as any);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+
+    await expect(svc.decide(req.id, { decision: 'approve', actorId: 'u9' }, SYS))
+      .rejects.toThrow(/RESUME_FAILED/);
+  });
+
+  it('decide: a resume that returns nothing still counts as success', async () => {
+    // The historical shape: an engine (or a test double) that reports nothing
+    // is reporting no failure, and must not be read as one.
+    svc.attachAutomation({ async resume() { /* returns undefined */ } });
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    const out = await svc.decide(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
+    expect(out.resumed).toBe(true);
+  });
+
   // ── read API ────────────────────────────────────────────────────
 
   it('listRequests: filters by approver and status', async () => {