Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .changeset/approvals-stranded-request-inspection.md
Original file line number Diff line number Diff line change
@@ -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).
42 changes: 42 additions & 0 deletions .changeset/verify-harness-durable-suspended-runs.md
Original file line number Diff line number Diff line change
@@ -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.
25 changes: 25 additions & 0 deletions content/docs/automation/approvals.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
</Callout>

<Callout type="info">
Expand All @@ -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.
</Callout>

### Progress and notification deep links
Expand Down
44 changes: 43 additions & 1 deletion content/docs/automation/flows.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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)` |

Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Loading