diff --git a/.changeset/approvals-stranded-request-inspection.md b/.changeset/approvals-stranded-request-inspection.md new file mode 100644 index 0000000000..53e01ced41 --- /dev/null +++ b/.changeset/approvals-stranded-request-inspection.md @@ -0,0 +1,48 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): find the zombie requests nothing was looking at (#4469) + +#4460 stopped new zombies being produced; the rows already stuck had no mechanism +to find or release them. The failure shape (#4420) is a request flipped to +`approved` / `rejected` / `returned` whose `flow_run_id` points at a run that no +longer exists — the decision landed, the flow never moved. Any deployment on +17.0.0-rc.1 that hit the wiring hole and crossed a restart mid-approval can be +carrying these rows. + +`releaseDeadRunRequests` could not see them, and the reason is worth stating +plainly: it scans `status: 'pending'`, and the very step that zombifies a request +is the one that takes it OUT of `pending`. The act of breaking it removed it from +the only sweeper's field of view — a large part of why this class of failure +stayed silent. It could not have answered the question even if it had looked: its +liveness oracle is `getRun`, which reads the execution LOG and returns `null` for +a perfectly ALIVE suspended run after a restart. It treats `null` as alive +(conservative, and correct for what it does) — which is exactly why it has no way +to say "this run is really gone". + +Adds `ApprovalService.inspectStrandedRequests()`, which uses BOTH oracles and +reports only rows that fail both: + +- `hasSuspendedRun(runId) === false` — the suspension store itself says no live + pause exists. It THROWS when the store cannot be read, and that case is + SKIPPED and counted as `undetermined`, never condemned: an unreadable store + means "unknown", and a storage outage must not be published as a lost run. +- `getRun(runId) == null` — no terminal history row either. A run that merely + finished is not stranded; a request whose run neither waits nor ever completed + is. + +**It reports; it never rewrites.** No status is changed and no run is cancelled. +The decision genuinely happened — a human approved or rejected — and silently +rolling it back would make the audit trail disagree with the facts. The report +carries what an operator needs to decide: which requests are stuck at which step, +and what the mirrored status field on the business record still reads (usually +the stale value the user is staring at). Whether to re-run the downstream actions +or re-open the approval is a judgement call this cannot make. + +It rides the existing escalation/dead-run sweep clock, so the finding surfaces in +the logs without an operator knowing to go looking for it. `recalled` is +deliberately out of scope: a recall abandons its run on purpose, and reporting +those would bury the real findings under expected ones. + +New export: `StrandedApprovalRequest` (the report row shape). diff --git a/.changeset/verify-harness-durable-suspended-runs.md b/.changeset/verify-harness-durable-suspended-runs.md new file mode 100644 index 0000000000..c31b7a02dc --- /dev/null +++ b/.changeset/verify-harness-durable-suspended-runs.md @@ -0,0 +1,42 @@ +--- +"@objectstack/verify": minor +--- + +fix(verify): stop the harness pinning `suspendedRunStore: 'memory'` (#4470) + +`bootStack` hardcoded `suspendedRunStore: 'memory'` when it registered +`@objectstack/service-automation`. That made the DB-backed suspended-run store +**structurally unreachable** from every dogfood/e2e fixture — not under-tested, +untestable. The coverage map had a clean seam nothing crossed: + +- unit tests covered ENGINE-side persistence (`suspended-run-store.test.ts` + drives suspend → restart → resume against a fake table); +- e2e covered the BUSINESS chain (approvals), but single-process and wholly in + memory; +- the ASSEMBLY between them — is `sys_automation_run` registered, is its table + created, is the store actually attached to the engine — was covered by + neither. + +#4420 grew in precisely that seam: the store hung off a table that was never +created, every write failed into a `warn` nobody read, the pause reported +success, and the run died at the next restart. #4460 added assembly unit tests; +this makes the e2e half possible. + +The harness now boots the plugin's own `'auto'` default — the same wiring +`objectstack dev` / `serve` get — so fixtures exercise the real assembly. Two +new knobs: + +- `automation` accepts `{ suspendedRunStore: 'auto' | 'memory' }` as well as + `true`, so a fixture that wants the old in-memory behaviour asks for it + explicitly rather than getting it by default. +- `databaseFile` backs the in-process SQLite database with a file instead of + `:memory:`, so state can outlive a kernel. + +Answering the question the issue raised — was `'memory'` pinned for speed or +because persistence could not run there? **Speed/simplicity.** The durable path +works in this harness: the accompanying dogfood proof boots with it, and the +whole existing dogfood suite passes on it unchanged (38 files, 239 tests). Note +`databaseFile` does not yet deliver a true cold boot: a second `bootStack` over +the same file reads a database whose tables exist but whose rows are gone — +ordinary records do not survive it either, so it is a harness/driver persistence +gap rather than anything to do with suspended runs, and it is filed as #4518. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index d348da984d..6696828f1a 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -477,6 +477,19 @@ approver, or **recall** it — releasing the lock. An admin decision is authoritative: it finalizes the node even under `unanimous`/`quorum`/`per_group`, 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. + +Note the rule is **"the actor is an admin"**, not "the slate is unstaffed" — so +an admin can also act on a request whose slate *is* properly staffed, bypassing +the people on it. That is why the decision records **which door it came +through**: `sys_approval_action.via_override` is `true` when the actor was +admitted *only* by this privileged path, holding no slot themselves. An admin who +is also a designated approver is approving normally and records `false` — the +flag is about the branch that authorized the call, not about who holds admin +rights. A row written before the column existed carries no value at all, which +reads as *not recorded* rather than as *not an override*. Without it, an +override and an ordinary approval were byte-for-byte identical, and the only +trace that a slate had been bypassed was the designated approver's later +`409 INVALID_STATE` — if they happened to try. @@ -491,6 +504,18 @@ 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. + +That sweep scans **pending** requests, which leaves one shape outside it: a +request already *decided* — `approved`, `rejected` or `returned` — whose run has +since vanished. The decision landed and the flow never moved, and flipping the +request out of `pending` is precisely what removed it from the sweep's view. A +second, **read-only** inspection rides the same clock for those: it reports a +terminal request only when the suspension store says no live pause exists **and** +no terminal run record exists either, skipping (never condemning) any row whose +store could not be read. It deliberately **does not rewrite** them — the decision +really happened, and rolling it back automatically would put the audit trail at +odds with the facts — so it names the stuck requests, their step, and the stale +mirrored status for an operator to act on. ### Progress and notification deep links diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index eda67e7f47..b3ccc5b681 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -468,7 +468,7 @@ POST /api/v1/automation/{flow}/runs/{runId}/resume | Pausing node | Suspends until… | Resumed by | | :--- | :--- | :--- | | `approval` | a human decision | the approvals service (`POST /api/v1/approvals/requests/:id/approve\|reject`) — resumes down the matching `approve` / `reject` edge. **Decide through the approvals API**; the resume route above **refuses** an approval pause outright (see below). | -| `screen` | a user submits the form | the UI runner posting the collected `inputs`; a `paused` response carrying the next `screen` chains multi-step wizards under one stable `runId` | +| `screen` | a user submits the form | the UI runner posting the collected `inputs` — **validated server-side against the screen's declared `fields`** (see below); a `paused` response carrying the next `screen` chains multi-step wizards under one stable `runId` | | `wait` (timer) | an ISO-8601 duration elapses | **automatically** — a one-shot job resumes the run; after a cold boot the engine re-arms pending timers from the durable store (overdue timers resume immediately) | | `wait` (signal) | a named external event | any caller invoking `resume(runId)` | @@ -513,6 +513,48 @@ key. A reserved name answers **400**, nothing is applied (not even legitimate keys sent alongside it), and the run stays parked. Ordinary author variables are unaffected, `$` mid-name (`price$`) included. +### A `screen` resume is checked against the declared fields + +A screen node's `config.fields` is a **contract**, not just a rendering hint: +the author declares which keys are collected, which are `required`, and — via +`visibleWhen` — when a field is even asked for. `resume` enforces all of it +server-side, so skipping the dialog and posting to the route directly is not a +way around what the author declared: + +``` +POST /api/v1/automation/{flow}/runs/{runId}/resume +{ "inputs": { "kind": "escalate" } } + +400 Invalid screen input: Screen field "escalation_reason" is required + — declared fields: 'kind', 'escalation_reason' +``` + +Two conditions are refused, both reported at once and both with +`code: 'INVALID_SCREEN_INPUT'`: + +- a **`required` field the caller was actually asked for** is missing (an empty + or blank string counts as missing); +- a key the screen **never declared** was sent. + +`visibleWhen` is evaluated against the **submitted values** first, so a hidden +field's `required` never fires — enforcing it would dead-end the run at a field +the user was never shown. A predicate that cannot be evaluated is treated as +hidden (and logged), because the client is the authority on what was rendered. + +Like the `$`-namespace rule above, the refusal happens **before** the suspension +is consumed: nothing is applied, the run stays parked, and the corrected +submission still lands. + +Three shapes declare no contract and so keep the pass-through — the same way an +action with no `params` is untouched: + +- an **object-form** screen (`config.objectName`), whose flat `fields` list is + empty by construction; the client persists the record through the normal write + path, which enforces that object's own `required` fields; +- a **message-only** screen (`waitForInput: true` with no fields); +- `signal.output`, which is the node-*output* namespace of the approval-style + resume envelope rather than the screen's collected-values channel. + Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on its descriptor when the decision to continue belongs to your service rather than to whoever holds the run id. diff --git a/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts index cb855b0dd1..206a70a9e3 100644 --- a/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts +++ b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts @@ -32,7 +32,7 @@ // but whose ROWS are gone, so it fails for a reason with nothing to do with // suspended runs. Ordinary records do not survive it either, which is what // identifies it as a harness/driver persistence gap rather than a defect in the -// suspended-run store. Filed separately; when it is fixed, the natural next test +// suspended-run store. Filed as #4518; when it is fixed, the natural next test // here is the one this file was originally written around. import { describe, it, expect, beforeAll, afterAll } from 'vitest';