`POST /api/v1/approvals/requests/{id}/reject` returns 500 while its effect lands AND strands the workflow run — three inconsistent outcomes from one call

[os-warren]

Split out of #13568 by the triage seat. #13568 was ruled by the maintainer at 2026-08-31T02:20Z (director batch #5, 「同意」). The evidence below was added to that card at 02:54Z and 03:57Z — after the ruling — and describes a defect the ruling does not reach: #13568's remedy (auto-cancel pending approvals when a record is deleted) prevents future orphans; it says nothing about a write path that returns an error while applying its effect, and nothing about the runs already stranded.

The measurement (from #13568's follow-up comments, reporter yinlianghui, re-read from transcript by the filing session — not recalled)

One call against an orphaned sys_approval_request (its record_id points at a deleted crm_leave_request row):

POST /api/v1/approvals/requests/{approvalRequestId}/reject     body {"comment": "<text>"}
  → HTTP 500

and afterwards three states coexist:

1. the caller received 500;
2. the request's status is rejected, and it has disappeared from the pending inbox — the effect landed;
3. the workflow run is stranded — unrecoverable.

The full error text (the first quote of it in #13568 was truncated; the reporter corrected it in the third comment, which is the version to trust):

…ow run 'run_8380f743-ac5b-4c27-a976-9ac5e71f6a00' could not be resumed and is
now stranded: resume of run 'run_8380f743-ac5b-4c27-a976-9ac5e71f6a00' failed:
Node 'mark_rejected' failed: update_record(crm_leave_request) failed:
Record 9SEmlyRfw8D9-J7Z not found in crm_leave_request

Controls from the same session: POST …/approve on a healthy request → 200. DELETE /api/v1/data/sys_approval_request/{id} → 405 (so the orphan cannot be cleaned up through the data API either).

Why this is p1 and separate from #13568

• The 500 lies about the outcome. A caller — human, script, or agent — reads 500 as "the rejection did not happen" and retries or escalates. It did happen. This is the error/effect divergence class, and it is worse than a plain failure because the honest-looking response is the wrong one.
• stranded is unrecoverable and has no operator story on this card. [finding] 删除记录后，它的审批单仍留在审批收件箱：pending 与 approved 两种都留，记录引用退化为原始 id #13568's ruling produces no mechanism for runs already in that state, because it did not know about them.
• The trigger generalises past approvals. mark_rejected fails because a workflow node writes back to a record that no longer exists. Any workflow node doing update_record on a since-deleted row plausibly reaches the same strand. ⚠️ Not measured — that generalisation is a hypothesis, and establishing its reach is this card's first step, not its premise.

First deliverables, in order

1. Establish the reach. Does any workflow node failing mid-resume strand the run, or is this specific to the approval resume path? That decides whether the fix is in plugin-approvals or in the workflow engine, and it may re-anchor this card's lane.
2. Decide the atomicity posture for the reject door: either the status write and the workflow resume succeed together, or neither does. ⛔ Returning 500 after a committed status write is not an option to preserve.
3. An operator path out of stranded — even if it is only a documented manual step. A state a deployment can enter and never leave is not acceptable as an end state, and the 405 on the data API means there is currently no way out at all.

⛔ Boundaries

• ⛔ Do not fold this back into [finding] 删除记录后，它的审批单仍留在审批收件箱：pending 与 approved 两种都留，记录引用退化为原始 id #13568. That card is ruled and dispatch-ready; this one is unruled and its first step is a measurement. Merging them would put a settled card behind an open question.
• ⛔ Do not "fix" this by making /reject return 200. The effect landing while a run strands is the defect; a green status code would only hide it.
• The lockRecord editing bind ([finding] 删除记录后，它的审批单仍留在审批收件箱：pending 与 approved 两种都留，记录引用退化为原始 id #13568 item 4, "改不了只能删重建") is the amplifier that produces orphans in the first place, and the ruling explicitly declines to solve it here. Same posture on this card.

Related

• [finding] 删除记录后，它的审批单仍留在审批收件箱：pending 与 approved 两种都留，记录引用退化为原始 id #13568 — the ruled parent (orphaned approval requests; auto-cancel + tombstone).
• finding: an approver routed a request can be unable to open the record it concerns — the inbox's record link dead-ends on "Record not found" #7345 — adjacent symptom, different cause (record exists, approver lacks read access); closed not_planned.
• Approval routed to an empty position permanently locks the record (no admin override, no recovery) #3424 / Approval: a dead approval run can leave the record RECORD_LOCKED (#3424 follow-up, expected-behavior 3) #3456 — approval routed to an empty position permanently locks the record; the lock cause, not this.

[claude]

Step 1 — the reach of stranded, measured

Measurement slice only; no repair, and /reject still returns 500. Instrument: draft PR #13899 (packages/services/service-automation/src/resume-node-failure-strands-run.test.ts), 3 tests green at 69d9b14f8.

0. A vocabulary correction that reframes the card

There is no run state named stranded. A repo-wide scan (excluding dist/ and CHANGELOGs) finds the word as a run outcome in exactly one file: packages/plugins/plugin-approvals/src/approval-service.ts — the thrown message could not be resumed and is now stranded and one sibling log line. AutomationResult.status (packages/spec/src/contracts/automation-service.ts) is 'completed' | 'paused' | 'failed'; the persisted engine statuses add 'cancelled'. Nothing anywhere persists, queries or filters on "stranded".

So "stranded" is approvals' word for an engine condition, and the engine condition is the thing to reason about:

the suspension has been consumed, the run is recorded failed, and the downstream work it was resumed to do never happened.

That matters immediately: an operator cannot search for stranded runs, because no field says so.

1. Every path that reaches it, with controls

The condition is produced in one place — AutomationEngine.resumeInternal (packages/services/service-automation/src/engine.ts). The whole defect is an ordering:

await this.forgetSuspendedRun(run, 'resumed');   // the suspension is consumed HERE
...
await this.traverseNext(node, flow, variables, context, steps, signal?.branchLabel);
} catch (err) { ...  recordLog({ status: 'failed' })  ... }

A downstream node throws with the pause already gone. The catch arm records failed and returns; nothing puts the suspension back.

Callers reaching that body:

#	Caller	What the failure looks like from outside
1	plugin-approvals serviceResume / resumeRecordedOutcome	the report: RESUME_FAILED, HTTP 500, decision already durable
2	Generic REST door POST /api/v1/automation/:name/runs/:runId/resume (packages/runtime/src/domains/automation.ts:1641)	HTTP 400 FLOW_FAILED — same engine state, different status code
3	wait-node.ts:115 — the timer job firing	logged, job disarmed
4	wait-node.ts:398 — rearmSuspendedWaitTimers on boot, overdue run	logged at error
5	engine-internal recursion — subflow delegation (engine.ts:4553) and bubbleToParent (engine.ts:4913)	the parent/child chain fails with it

Path 2 is the decisive one, and the route's own comment already states the fact:

"Every arm above is a REFUSAL that left the suspension intact and can be retried; what reaches HERE consumed its pause and ran."

Controls. The same scan shape (git grep '\.resume(' across packages/, apps/, examples/) is not a zero-finder: it returns the five call sites above and unrelated process.stdin.resume() hits in cli and mcp — the noise floor that proves it was not over-filtered. And the instrument's own two reverse controls prove the probes measure rather than return constants: a resume refused before the consumption point (INVALID_SIGNAL) leaves the pause intact and re-resumable, and a clean resume also ends unsuspended — so the strand is the failed status, not merely the missing pause.

2. Verdict: GENERAL to workflow resume — and the card should be re-laned

Measured, not inferred: the new test reproduces the strand through a plain pausing node declaring resumeAuthority: 'any', continued through the generic door, with zero approvals involvement. The hypothesis in the card's third bullet is confirmed.

But the reported defect decomposes into two halves that live in two packages, and this is the part the next dispatch needs:

• The strand — service-automation. A property of resumeInternal's ordering, reached by all five callers. Deliverables 1 and 3 are engine work.
• The error/effect divergence — plugin-approvals. decide() calls decideNode() (which writes the decision row and mirrors the status field onto the business record) and only then resumeRecordedOutcome, with no compensating transaction. The generic REST door has no prior durable side-write, which is exactly why it produces a strand but not the three-coexisting-states shape. Deliverable 2 (atomicity posture) is approvals' own.

Implication for laning: an approvals-only fix would close the 500-with-landed-effect and leave paths 2 through 5 stranding exactly as they do today. Recommend the engine half leads and the approvals half follows it, rather than the reverse.

3. Is stranded terminal? Yes — no path out, anywhere

Surface	Result
engine.resume(runId) again	RUN_NOT_FOUND — measured
engine.cancelRun(runId)	false, a no-op (it needs a suspended run to consume) — measured
REST run surface	GET /runs, GET /runs/:id, POST /runs/:id/resume, GET /runs/:id/screen — no cancel route, no retry route
CLI	no run commands at all (packages/cli/src/commands/ has none)
Engine public API	14 public async methods; none moves a run out

Control for the zero. git grep 'restartRun|retryRun|unstrand|reviveRun|reopenRun|requeueRun' returns nothing in code — but the same scan shape over the engine (grep '^ async [a-zA-Z]*(') does find the 14 verbs that exist: claim, listFlows, getFlow, hydrateFlowActivations, toggleFlow, listRuns, getRun, execute, resume, hasSuspendedRun, cancelRun, listSuspendedRunsDurable, getSuspendedScreen, runRegion. The zero is a fact about the vocabulary, not a scan artifact.

Additional finding — the existing stranded-request inspector is structurally BLIND to this shape. ApprovalService.inspectStrandedRequests scans ['approved','rejected','returned'], so it does see the rejected row. But its second oracle is:

if (terminal) continue;    // the run ran to a terminal state — it is not dangling

where terminal = await getRun(runId). In this shape the failed resume wrote a terminal failed log row (measured: getRun(runId).status === 'failed'), so the row is skipped as healthy. That is correct for the #4469 target it was built for (a run that never ran) and wrong for this one (a run that ran and failed mid-resume). releaseDeadRunRequests cannot see it either — it scans status: 'pending', and the rejection is what took the row out of pending.

So today an operator has neither a repair verb nor a report that names the row. Deliverable 3 is not just "no escape hatch"; it is "no escape hatch and the one existing inspector reports all clear".

4. Measured / NOT MEASURED boundary

MEASURED in-repo (vitest at 69d9b14f8): the engine strand and its generalisation past approvals; getRun().status === 'failed'; re-resume yields RUN_NOT_FOUND; cancelRun yields false; the absence of any recovery verb, with its control.

Code reading resting on a measured fact (not executed): the inspectStrandedRequests blindness. The branch is if (terminal) continue; and terminal is truthy because of the measured failed row. I did not instantiate an ApprovalService against a datasource.

NOT MEASURED — needs a running deployment, and who must run it:

1. That the reporter's run took the traverseNext-throw arm rather than the no-engine arm. The quoted text matches resumeRecordedOutcome's throw verbatim, so this is near-certain — but it is still an inference from a log line. Reporter yinlianghui, or anyone with that deployment's logs.
2. The 405 on DELETE /api/v1/data/sys_approval_request/{id}. I did not measure the data-API method allowlist for platform objects, so I cannot say whether that 405 is by design or a second gap. Needs a data-API dispatch reading or a live probe — worth its own card if it is not by design.
3. How many runs are already stranded in that deployment. Nothing in-repo can count them, and per section 3 inspectStrandedRequests will report 0 for this shape. The only census is a deployment-side query over sys_automation_run (status = 'failed') joined against sys_approval_request (terminal status with flow_run_id set). The deployment operator must run it — and deliverable 3's remedy should be sized from that number, not from zero.

Generated by Claude Code

---

Generated by Claude Code

[claude]

os-dev-report

{
  "issue": 13807,
  "status": "done",
  "branch": "claude/issue-13807-strand-reach-measurement",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/13899",
  "premise_still_valid": true,
  "summary": "Step 1 measurement only; no repair, /reject still returns 500. VERDICT: the strand is GENERAL to workflow resume, not approval-path-specific. There is no run state named 'stranded' anywhere — AutomationResult.status is 'completed'|'paused'|'failed' and the word lives only in two plugin-approvals error strings; it is approvals' prose for an engine condition. That condition is produced in ONE place, AutomationEngine.resumeInternal, by an ordering: forgetSuspendedRun(run,'resumed') consumes the suspension BEFORE traverseNext runs downstream nodes, so a node that throws throws with the pause already gone and the catch arm records the run 'failed'. Five callers reach it; the decisive one is the GENERIC REST door POST /api/v1/automation/:name/runs/:runId/resume, which hits the identical arm and answers HTTP 400 FLOW_FAILED (its own comment already says 'what reaches HERE consumed its pause and ran'). The reported defect therefore decomposes across TWO packages: the strand belongs to service-automation (reached by all five callers), while the error/effect divergence belongs to plugin-approvals (decide() writes the decision row and mirrors the status field, then resumes, with no compensating transaction — the generic door has no prior durable side-write, which is why it strands but does not produce three coexisting states). This is the re-laning input: an approvals-only fix leaves the other four callers stranding as today. 'stranded' IS terminal — resume yields RUN_NOT_FOUND, cancelRun is a no-op, the REST run surface has no cancel or retry route, the CLI has no run commands, and none of the engine's 14 public methods moves a run out. Additional finding inside deliverable 3: ApprovalService.inspectStrandedRequests is structurally BLIND to this shape — it scans ['approved','rejected','returned'] so it sees the row, but its oracle 'if (terminal) continue' skips it because the failed resume wrote a terminal 'failed' log row; releaseDeadRunRequests scans status:'pending' only. So an operator today has no repair verb AND the one existing inspector reports all clear.",
  "tests": "Instrument added: packages/services/service-automation/src/resume-node-failure-strands-run.test.ts — reproduces the strand through a plain pausing node with resumeAuthority:'any' via the generic engine.resume() door, ZERO approvals involvement. `pnpm --filter @objectstack/service-automation exec vitest run --maxWorkers=2 src/resume-node-failure-strands-run.test.ts` (via scripts/pm/os-verify-lock.sh) → 'Test Files 1 passed (1) · Tests 3 passed (3)', VERDICT command-exit 0, re-run at final commit 69d9b14f8. Two REVERSE CONTROLS in the file: (a) a resume refused before the consumption point (INVALID_SIGNAL) leaves hasSuspendedRun true and still resumable — so the probes measure rather than return constants; (b) a clean resume also ends unsuspended, so the strand is the FAILED status not the missing pause. Enumeration control: git grep '\\.resume(' returns the five real call sites AND unrelated process.stdin.resume() hits in cli/mcp — the noise floor proving the scan was not over-filtered. Zero-control for recovery verbs: grep restartRun|retryRun|unstrand|reviveRun|reopenRun|requeueRun → nothing, while the same scan shape over the engine finds the 14 verbs that do exist. GATES: re-derived from the ACTUAL diff via `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack`, both sections read whole — 22 path-matched families + the convention-triggered set for adding a test file. Union run at 69d9b14f8: 23 green (exit codes captured before any pipe), including every convention-triggered ratchet (check:engine-double-contract, check:where-matcher, check:cross-package-test-inputs, check:query-options-erasure, check:type-check-coverage, check:test-source-alias, check:published-files) plus check:nul-bytes OK and a manual control-byte grep (no hits). check:type-check-debt: the ledger entry for @objectstack/service-automation reads errors:3; tsc --noEmit measures exactly 3 at HEAD, all three the ledgered TS2341s in nested-region-parity.test.ts, 0 from the new file — and tsc --listFiles confirms the new test IS in the program (1 hit, control file 4), so this is a measurement and not an exclusion reading. TWO GATES NOT MEASURED (exit 3, by their own printed verdict lines — neither a pass nor a red): check-test-completeness.mjs needs a saved `turbo run test` log that CI tees and a local run cannot produce; check:dual-build-cjs-loads printed 'PREREQUISITE NOT MET' (49 packages have no dist/, needs a full pnpm build). Both are CI's to run.",
  "mcp_calls": "2 — both mcp__github__issue_read on #13807 (get, get_comments). Every other GitHub operation went through the repo-scoped REST channel after a 200 probe; gh is absent in this container.",
  "open_questions": [
    {
      "question": "Should #13807 be re-laned, and how should the two halves be sequenced? The measurement splits the defect across two packages: the strand is service-automation's (resumeInternal ordering, reached by all five resume callers) and the error/effect divergence is plugin-approvals' (durable decision write before an uncompensated resume).",
      "options": [
        "A — split into two cards: an engine card owning deliverables 1 and 3 (the strand and the operator path out), and an approvals card owning deliverable 2 (the atomicity posture for the reject door), with the engine card landing first.",
        "B — keep one card but re-lane it to the workflow-engine lane, with the approvals atomicity fix as a follow-up item inside it.",
        "C — leave it in the approvals lane and fix only the reject door."
      ],
      "recommendation": "A. C is measurably insufficient: the generic REST door, both wait-node paths and the subflow recursion reach the identical arm today, so an approvals-only fix leaves four callers stranding and the card's own deliverable 1 unmet. Between A and B, A is preferred because the two halves have different blast radii and different reviewers — the engine half changes resume semantics for every pausing node type, while the approvals half is a local transaction-ordering question — and because deliverable 3 (the operator path out of a terminal run) is engine work that also serves the four non-approvals callers. Sequencing matters: the approvals atomicity fix is much cheaper to specify once the engine has decided whether a failed mid-resume can leave the pause intact, because 'neither happens' is only implementable if the engine can roll the suspension back."
    },
    {
      "question": "Deliverable 3 asks for an operator path out of a terminal run, but the census that should size it cannot be taken in-repo. inspectStrandedRequests will report 0 for this shape (measured blindness), so how many runs are already stranded in the reporter's deployment is unknown.",
      "options": [
        "A — ask the deployment operator to run a census over sys_automation_run (status='failed') joined against sys_approval_request (terminal status with flow_run_id set) before sizing the remedy.",
        "B — size the remedy from zero and ship a documented manual step only.",
        "C — widen inspectStrandedRequests' oracle first so the census becomes a supported in-product query, then size."
      ],
      "recommendation": "A, and then C as part of the engine card. A is cheap and unblocks sizing immediately; C is the durable answer because the inspector's blindness is itself part of why this class stayed silent, and the same oracle widening is what would let the platform report the condition at all. B risks repeating #13568's shape — a remedy that prevents future occurrences while saying nothing about the rows already stuck."
    }
  ],
  "out_of_scope_findings": [
    "NOT FILED, needs measurement first: DELETE /api/v1/data/sys_approval_request/{id} answering 405 (the card's own control). I did not measure the data-API method allowlist for platform objects, so I cannot say whether the 405 is by design or a second gap — filing blind would be a guess. Reported in the issue comment under NOT MEASURED with who must measure it. PM may want a card once someone reads the data-API dispatch or probes a live deployment.",
    "NOT FILED, inside this card's scope: ApprovalService.inspectStrandedRequests is structurally blind to the #13807 shape (its 'if (terminal) continue' oracle skips the row because the failed resume wrote a terminal 'failed' log entry), and releaseDeadRunRequests scans status:'pending' only. This is deliverable 3's own subject matter rather than a separate defect, so it is written into the issue comment instead of split off — splitting it would fragment an unruled card."
  ]
}

---

Generated by Claude Code