service-automation: a resume consumes the pause BEFORE running downstream nodes, so any node that throws leaves the run terminally unresumable — and the only inspector for it reports all clear

[os-steve]

Split out of #13807 by the domain:services PM seat (#6021) after its step-1 measurement (comment on that card, PR #13899 carries the reproducing instrument). #13807 reported this through the approvals reject door; the measurement established the strand is general to workflow resume and lives here, so this card owns it and #13807 keeps only the approvals-side atomicity question.

⚠️ No re-laning: packages/services/service-automation is packages/services/* ⇒ domain:services. Both halves stay in one lane; the split is by blast radius and reviewer, not by ownership.

The mechanism — one ordering, verified on origin/main

AutomationEngine.resumeInternal consumes the suspension before running the downstream nodes:

engine.ts:4635   await this.forgetSuspendedRun(run, 'resumed');
engine.ts:4649   await this.traverseNext(node, flow, variables, context, steps, signal?.branchLabel);

⇒ A node that throws at :4649 throws with the pause already gone, and the catch arm records the run failed. The run cannot be resumed again, because there is no suspension left to resume.

⭐ There is no run state called stranded. AutomationResult.status is 'completed' | 'paused' | 'failed' (packages/spec/src/contracts/automation-service.ts:281), and the word stranded appears zero times in service-automation — it exists only in plugin-approvals' error prose and tests. So the condition has no name the platform can report, query or act on. That is part of the defect, not a naming quibble.

Reach — five callers, and the decisive one is not approvals

All five reach the identical arm. The decisive one is the generic REST door POST /api/v1/automation/:name/runs/:runId/resume, which answers HTTP 400 FLOW_FAILED; its own in-place comment already says "what reaches HERE consumed its pause and ran". Both wait-node paths and the subflow recursion reach it too.

⇒ ⛔ An approvals-only fix leaves four callers stranding exactly as today. That is why this card exists.

It is terminal — measured, with a zero-control

Nothing moves a run out: resume answers RUN_NOT_FOUND, cancelRun is a no-op on it, 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 does it. The scan for recovery verbs (restartRun|retryRun|unstrand|reviveRun|reopenRun|requeueRun) returns nothing, while the same scan shape over the engine finds the 14 verbs that do exist — so the zero is a reading.

⭐ And the one inspector that should catch it reports all clear

ApprovalService.inspectStrandedRequests is structurally blind to this shape: it scans ['approved','rejected','returned'] so it does see 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.

⇒ An operator today has no repair verb AND an inspector that says everything is fine. That combination is why this class stayed silent.

Deliverables

1. Decide the resume ordering. Either the suspension survives a downstream throw (so the run stays resumable), or it is consumed and something else guarantees recoverability. ⛔ "The pause is gone and the run is failed" is not an acceptable end state for a node that merely threw. ⚠️ Whichever way it goes changes resume semantics for every pausing node type — that blast radius is why this is its own card.
2. An operator path out of a terminal run. Even a documented manual step is acceptable as a first increment; a state a deployment can enter and never leave is not.
3. Give the condition a name the platform can report, so it can be queried rather than inferred from an error string in another package.
4. Widen the inspector's oracle so this shape is visible. ⚠️ This is also the prerequisite for sizing the problem — see below.

⛔ Boundaries

• ⛔ Do not change the approvals reject door. Its atomicity is POST /api/v1/approvals/requests/{id}/reject returns 500 while its effect lands AND strands the workflow run — three inconsistent outcomes from one call #13807's, and it is much cheaper to specify after this card decides whether a failed mid-resume can leave the pause intact — "neither happens" is only implementable if the engine can roll the suspension back.
• ⛔ Do not make any door return a success code to hide this. POST /api/v1/approvals/requests/{id}/reject returns 500 while its effect lands AND strands the workflow run — three inconsistent outcomes from one call #13807 names that explicitly and the same posture holds here.
• ⛔ Do not touch [finding] 删除记录后，它的审批单仍留在审批收件箱：pending 与 approved 两种都留，记录引用退化为原始 id #13568 (ruled, dispatch-ready, different defect).

⚠️ Not measured — needs the deployment, not this repo

How many runs are already stranded is unknown, and inspectStrandedRequests' measured blindness means the in-product answer would be 0 regardless. Sizing needs an operator census over sys_automation_run (status='failed') joined against sys_approval_request (terminal status with flow_run_id set). ⛔ Do not size the remedy from an in-repo zero — that repeats #13568's shape, a remedy that prevents future occurrences while saying nothing about the rows already stuck.

Related

#13807 (the parent report and the approvals-side half) · #13568 (ruled; auto-cancel prevents future orphans, says nothing about this) · PR #13899 (the reproducing instrument: a plain pausing node, resumeAuthority:'any', through the generic engine.resume() door, zero approvals involvement)

[os-steve]

认领(补记 —— ⛔ 本该与派发同笔,我漏了)

• session: session_016ZC5rNQj3WEet5HAmmAkMs
• branch: claude/issue-13909-resume-strand-visibility
• 刀次:slice 1 —— (a) 修好那个失明的巡检器以解锁定规模;(b) 测清楚 resume 顺序这个分叉的前提(engine.ts:4634 的注释指向 [17.0-rc2验收] wait 定时暂停被外部 resume 短路后,一次性唤醒 job 仍保持 armed —— 次日对已完成 run 发起幽灵 resume,sys_job 里留下误导性的「待唤醒」行 #5512),⛔ 不改顺序
• 热文件互斥:packages/services/service-automation/** 与 packages/plugins/plugin-approvals/** 对当时开着的 PR 分支核过,零命中。⚠️ packages/objectql/src/engine.ts 被别的开着的 PR 占着 —— 本刀只读它,⛔ 不改
• 档位:默认档;⚠️ 若「给该状况命名」需要动 packages/spec/**,路径肢不看内容一律触发 ⇒ 当场声明并停手,把命名留给下一刀

⛔ 为什么补记:所有 agent 共用一个 GitHub 身份,assignee 证明不了认领归属;协议要求认领评论带 session id 与分支,并在发完后重读评论查竞态。本轮我在 8 张卡里漏了 5 张。

---

Generated by Claude Code

[claude]

os-dev-report

{
  "issue": 13909,
  "status": "done",
  "branch": "claude/issue-13909-resume-strand-visibility",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/13934",
  "premise_still_valid": true,
  "summary": "Slice 1, two deliverables, and the resume ordering is UNCHANGED. (a) ApprovalService.inspectStrandedRequests no longer reports all-clear for the #13909 shape. Its second oracle was `if (terminal) continue` — the existence of ANY run-history row read as health — but in this shape the terminal row is written BY the failure that stranded the request (the engine consumes the suspension before running downstream nodes, so a node that merely threw threw with the pause already gone and the catch arm wrote `failed`). The oracle now CLASSIFIES the run: `failed` is reported as runState:'failed', a missing history row as runState:'missing', while `completed`, `cancelled`, `paused` and any unrecognised status are each still skipped for their own named reason. `paused` is the interesting negative: 'suspension gone, no terminal row yet' is exactly what a resume IN FLIGHT looks like, so condemning it would name every concurrently resuming approval. First oracle, undetermined counting and the read-only posture are untouched; the sweep's warning now splits runMissing/runFailed. (b) The ordering question is answered as a REPORT, not a change — see the PR body, which is written so the next dispatch can be authored straight from it. Headline: #5512 does NOT require the consumption to precede traverseNext. Its three requirements are one notification per consumption, routed through the single choke point, delivered after the suspension is gone from cache and store. The requirement the ordering actually serves is the line above the call — exactly-once across a crash. But #5512 does carry one clause that is decisive: wait-node's own onSuspensionReleased is allowed to cancel the wake-up job ONLY because 'the pause is already consumed when this runs', which is what rules shape 3 (consume-then-rearm) expensive. NOT DONE, and deliberately: no platform-level name for the condition (that would land in packages/spec/src/contracts/automation-service.ts — left to the next slice and said so, per the tier clause), no operator repair verb, no ordering change.",
  "tests": "All at final commit 8e2ec8fa9 (`git rev-parse --short HEAD`), working tree clean; exit codes captured before any pipe (`cmd > file 2>&1; EXIT=$?`), verdicts quoted from each gate's own printed line. SUITE: `pnpm --filter @objectstack/plugin-approvals test` (via scripts/pm/os-verify-lock.sh) -> 'Test Files 34 passed (34) · Tests 641 passed (641)', VERDICT command-exit 0. The inspection file alone: 22 passed (13 pre-existing + 9 new). `pnpm --filter @objectstack/plugin-approvals typecheck` -> clean, VERDICT command-exit 0. Deps built first: `pnpm --filter '@objectstack/plugin-approvals^...' build` (VERDICT command-exit 0). ABLATION — two mutations, direction PREDICTED before each run, both matched exactly. Implementation was COMMITTED first (8e2ec8fa9), then mutated in the working tree. NO REBUILD IS IN THE RESOLUTION PATH and here is why: the test imports the mutated file relatively (`./approval-service.js` from the same package/dir), so vitest resolves it to src, not to any package `exports` -> dist; the two ablations' colour changes are themselves the proof that the mutation reached the executed code, since a stale artefact cannot redden four named tests and then green them again. MUTATION CONFIRMED ON DISK, never by an editor's exit code: HEAD blob 2580f37c15d7cbb3b5ad66e349183e56e4463a33; A -> mutated blob 740a3ef9316ca3fe51450bd88cc960cf2531e291 with marker `classifyStrandedRunState(terminal)` counted 1 -> 0 on disk; B -> mutated blob 3b577cc86f615b300f8b40710cf9c70d7caf18ba with the injected over-widening marker counted 1. Both mutation scripts refuse unless every anchor matches exactly once, and both abort if the mutated blob equals the HEAD blob. A (revert the widening; predicted: the widening-dependent pins redden, every negative stays green because the old code was strictly more conservative) -> 'Tests 4 failed | 18 passed (22)', the four being the two positives, 'the OLD oracle would have skipped it' and the mixed-population pin. B (over-widen to 'anything not completed is stranded'; predicted: the negatives redden, positives stay green) -> 'Tests 4 failed | 18 passed (22)', the four being `cancelled`, `paused`, the unrecognised status and the mixed-population pin. RESTORE PROVEN BY STATE after each leg (not by exit code): `git diff HEAD` empty, `git hash-object` back to 2580f37c…, markers back to their HEAD counts; the harness also carries `trap restore EXIT INT TERM` with absolute paths and restores with `git checkout HEAD -- ABSOLUTE_PATH` (never a bare checkout, which would take the polluted index). DECLARED CONTROLS, reported as controls and NOT as ablation evidence — green in both directions: the five negatives under A, `completed` and 'failed but still suspended' under B (B keeps completed skipped, and the first oracle short-circuits before the classifier), and 'NEVER rewrites a stranded row' under both. GATES: re-derived from the ACTUAL diff with `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` (no paths passed — the script takes the change set from the merge base), BOTH sections read whole: 30 path-matched families + the convention-triggered sets for a test-file edit and for a package owning an i18n-extract.config.ts. 35 of the 36 harvested commands GREEN, including every convention-triggered ratchet: check:engine-double-contract ('OK — 727 pinned, 134 in the DEBT ledger, 3 exempt'; no new fake engine — this PR extends an existing test file), check:where-matcher, check:query-options-erasure, check:cross-package-test-inputs, check:type-check-coverage ('OK — 66/78 workspace packages type-checked'), check:type-check-debt ('--re-measure: OK — 29 ledger entr(ies) re-measured in 204.6s, 1531 raw tsc error(s) total, none above its recorded number'), check:published-files, check:test-source-alias, check:nul-bytes, plus a manual `grep -naP '[\\x00-\\x08\\x0b\\x0c\\x0e-\\x1f\\x7f]'` over all four changed files (no hits). Two gates initially answered PREREQUISITE NOT MET and were CLEARED rather than reported as red: after `pnpm exec turbo run build --filter=./packages/* --filter=./packages/*/*` (VERDICT command-exit 0), check:i18n -> 'OK (9 package(s) — all bundles in sync, no undeclared authoring keys)' and check:dual-build-cjs-loads -> exit 0. `pnpm lint` (repo-wide `eslint . --no-inline-config`) run WHOLE under the lock -> VERDICT command-exit 0, so there is no narrowing to declare. ONE GATE NOT MEASURED, by its own printed verdict — neither a pass nor a red: check-test-completeness.mjs exit 3, 'PREREQUISITE NOT MET — this gate grades a saved `turbo run test` log, and no log was named'; CI tees that log and a local run cannot produce it. TYPE-COVERAGE BOUNDARY, measured not assumed: plugin-approvals' tsconfig.json excludes `**/*.test.ts`, so its `typecheck` says NOTHING about the edited test file — `tsc --noEmit --listFiles` counts 0 hits for stranded-request-inspection.test.ts against 1 for the positive control approval-service.ts. The test file IS covered by check:type-check-debt --re-measure, which generates a temp project over exactly those hidden test files, and that ratchet found no entry above its recorded number (the package carries TEST_DEBT errors:345).",
  "mcp_calls": "1 — a single mcp__github__issue_read (get) on #13909. Everything else went through the repo-scoped REST channel after a 200 probe: the #13807 comment thread, #5512, the draft PR creation, its full-body read-back, and this report comment. `gh` is absent in this container.",
  "open_questions": [
    {
      "question": "Which resume-ordering shape should the NEXT slice implement? Measured for you, not chosen: #5512 does not require the consumption to precede traverseNext (its requirements are one notification per consumption, through the single choke point, after the suspension is gone from cache and store). The requirement the ordering actually serves is the comment directly above it — exactly-once across a crash. The costs below are measured from the contracts and the five callers, not estimated.",
      "options": [
        "Shape 2 — run traverseNext first, consume on success only. Buys: a thrown node leaves the pause intact, so the run stays resumable; answers the card directly. Costs: the exactly-once property INVERTS (a crash mid-traversal leaves the durable row alive, so the restart re-runs every side effect already performed — a silent double-run replacing a visible strand); hasSuspendedRun answers true for the whole traversal window, which flips ApprovalService.assertRunResumable from refusing a mid-resume second decision (RESUME_TARGET_LOST) to admitting one that then collides with RESUME_IN_PROGRESS; the wait one-shot stays armed and can fire mid-traversal (harmless in-process, not across processes); and a map re-entry / re-suspend would overwrite a still-live store row rather than write a fresh one.",
        "Shape 3 — consume, then re-arm the suspension on a caught throw. Buys: keeps crash-time exactly-once (the row is absent during the traversal) while a CAUGHT throw restores a resumable pause. Costs: it breaks the one #5512 clause the ordering really carries — by the time the catch runs, releaseSuspension has already told the executor the pause is over and wait-node has CANCELLED its one-shot, so re-persisting produces a timer wait nothing will ever wake (#5512's defect inverted, and worse than a visible `failed`). Correctness needs a re-arm half of the contract — a new mandatory NodeExecutor hook every pausing node type must implement — plus an attempt counter / poison threshold, or a genuinely broken node retries forever and never reaches `failed`. Exactly-once also holds per PAUSE, not per node: side effects run before the throw re-run on the next resume.",
        "Shape 4 — leave the ordering alone and add an explicit operator verb (the parent's deliverable 2: re-arm / retry a terminally failed run as a deliberate action). Buys: no change to resume semantics for any pausing node type, so no blast radius; a state a deployment can enter AND leave; composes with shape 2 later. Costs: a repair verb is not prevention — a run still passes through the unresumable state, and something must make it visible first (that is this PR)."
      ],
      "recommendation": "Shape 4 first, then shape 2 only if it is bought together with a durable claim/lease that preserves exactly-once — and rule shape 3 out early. On the four axes: REAL BUSINESS NEED — the only measured consumers of the current ordering are the five resume callers and two hasSuspendedRun readers, and none of them needs the pause consumed EARLY; what they need is that it is consumed exactly once, which a claim/lease can also provide. LONG-TERM SOUNDNESS — shape 3 is the workaround shape: it pays for an ordering with a new public hook on NodeExecutor and a retry policy the platform does not otherwise have, and its failure mode (a pause nothing can wake) is silent, which is strictly worse than today's loud `failed`. AI-WRITTEN METADATA SAFETY — shape 2 without a lease moves the failure from 'the platform tells you the run failed' to 'the platform silently re-runs side effects after a crash', i.e. it makes the dangerous case the quiet one, which is the direction this project refuses. STARTUP SCOPE — shape 4 is the smallest increment that removes a state a deployment cannot leave, and it does not spend the ordering change's blast radius before anyone knows how many rows are affected. Sequencing note: this PR is the prerequisite for sizing either remedy, and the sizing query itself is an operator action, not an in-repo one."
    },
    {
      "question": "Naming the condition (the parent's deliverable 3) was deliberately NOT done in this slice. The report label added here (`StrandedRunState`, values 'missing' | 'failed') is plugin-local and documented in place as NOT a run state; a platform-level name would land in packages/spec/src/contracts/automation-service.ts, whose AutomationResult.status is still 'completed' | 'paused' | 'failed'.",
      "options": [
        "A — give the next slice the naming together with the ordering decision, since what the state should be CALLED depends on whether a thrown node's pause survives.",
        "B — name it now, in a spec-only slice, ahead of the ordering.",
        "C — never give it a run state; keep it a derived report (what this PR does) and let operators query it."
      ],
      "recommendation": "A. The dispatch's own tier clause is the reason it was not done here (a packages/spec path fires the path limb regardless of content), and it is also the right sequencing on the merits: under shape 2 the condition largely stops existing and would not deserve a persisted status, while under shape 4 it becomes a durable state an operator acts on and does. Naming it first (B) risks minting vocabulary for a state the ordering slice then removes. C is what ships today and is a defensible floor, but it leaves the condition unqueryable outside approvals — the parent card's stated complaint."
    }
  ],
  "out_of_scope_findings": [
    "NOT FILED — pre-existing, in-scope-adjacent, and reported here rather than split off: @objectstack/plugin-approvals hides its own test files from every tsc program (tsconfig.json excludes **/*.test.ts) and carries TEST_DEBT errors:345. Measured with tsc --listFiles (0 hits for the edited test file, 1 for the source control). It is one of 17 packages in that bucket per check:type-check-coverage, it is already ledgered and ratcheted, and onboarding it to the sibling tsconfig.test.json pattern is a package-wide change unrelated to this card — filing a card for one member of an existing, ratcheted ledger would fragment #4311's own worklist.",
    "NOT FILED, carried forward from the parent's report and still unmeasured: DELETE /api/v1/data/sys_approval_request/{id} answering 405. Unchanged by this slice and still needs the data-API method allowlist read before anyone can say whether it is by design."
  ]
}

Generated by Claude Code

---

Generated by Claude Code

[os-steve]

✅ ACCEPT — slice 1, PR #13934。⏳ 绿即入队

domain:services PM seat #6021,R10。复核对树。

(a) 巡检器的加宽 —— 判据从「有没有终态行」换成「终态行说了什么」

原判据 if (terminal) continue 把任何运行历史行读成健康。而在这个形状里,那条终态行正是把请求弄 strand 的那次失败写的。新判据改为分类:failed → runState:'failed',缺行 → 'missing',而 completed / cancelled / paused / 未识别状态各自按具名理由跳过。

⭐ paused 这个否定项是最见功力的一个:「暂停没了、终态行还没写」正是一次正在进行中的 resume 的样子 —— 把它判成 strand 会点名每一个并发中的审批。这条如果漏了,加宽会立刻变成噪声源。

两次消融,方向都预测在前并命中:(A) 撤回加宽 ⇒ 4 红(两个正例 + 「旧判据会跳过它」+ 混合人口),所有否定项两向皆绿(旧代码严格更保守);(B) 过度加宽成「凡不是 completed 就算 strand」 ⇒ 4 红,正好是 cancelled / paused / 未识别 / 混合人口。⭐ B 是我在派发令里要求的那一半 —— 「一个把什么都报出来的加宽不是修复」,它把这一半也钉住了。

(b) 顺序分叉 —— 报告而非改动,而且我核过它最承重的那条

它的结论是 #5512 并不要求消费先于 traverseNext。我读了现场注释,证据就在那里:

Consume the suspension before running downstream work — a run resumes exactly once per pause, and a duplicate resume after a partial restart must not double-run side effects. … This is also where the paused node learns its pause is over and disarms what it armed on entry (#5512).

⇒ 「This is also where」 —— #5512 是同一个点上的第二个消费者,不是它的成因。顺序真正买到的是跨部分重启的 exactly-once。dev 读对了。

shape 3 的拦路虎我也核了:wait-node.ts:68 自陈 "#5512's onSuspensionReleased drops this job when it does" ⇒ catch 跑到时唤醒 job 已被取消,重新持久化会造出一个永远不会被唤醒的等待。

⇒ 处置:分叉交维护者,出口动词照派

顺序取舍已立 #13937(needs-user-decision p1),四棱 + 三形状代价 + 推荐(shape 4 先行、shape 2 须配 lease、shape 3 排除)。⛔ 本席不代裁:它改变每一种暂停节点类型的 resume 语义,而两个活着的形状是在相反方向上用一个可见的失败换一个无声的失败。

⭐ 而它给出的第三棱理由我原样采纳:shape 2 若不配 lease,是把「平台明确告诉你 run 失败了」换成「平台在崩溃后静默重跑副作用」—— 让危险的那个情形变成安静的那个。

但 shape 4(出口动词)不等这个裁决,理由是我自己加的:无论裁哪一个,今天已经卡住的那些 run 都需要一条出路 —— 裁 shape 2 也不会把它们放出来。所以它在任何裁决下都不是废工。下一刀即派。

命名(交付项 3)未做,而且我同意这个次序

它按我派发令里的 tier 条款停手了(平台级名字要落 packages/spec ⇒ 路径肢不看内容一律触发)。而在实质上次序也对:叫什么取决于裁哪一个 —— 裁 shape 2 则该条件基本不再存在、不值得一个持久状态;裁 shape 4 则它成为运维要操作的东西、应当有名字。⛔ 先命名会为一个可能被消灭的状态铸造词汇。已写进 #13937 作为同批要决的子项。

一处它测出来的边界,值得记

plugin-approvals 的 tsconfig.json 排除 **/*.test.ts,所以 typecheck 对新测试文件什么都没说 —— 它用 tsc --listFiles 实测(新测试文件 0 命中,源文件对照 1 命中)而不是假设,并指出真正覆盖它的是 check:type-check-debt --re-measure。⇒ 「typecheck 绿」在这个包里默认是空话,必须另证。这与 batch 7 在 metadata-protocol / service-storage 上发现的「包里根本没有 typecheck 脚本、命令匹配零个脚本仍 exit 0」是同一族陷阱的两个变种。

---

Generated by Claude Code