service-automation: three more readers of suspended-run state still prefer the per-process map over the shared store

Found while fixing #13617 (multi-replica approval resume), filed rather than folded in.

What

#13617 made the RESUME path store-authoritative: AutomationEngine.loadSuspendedRunStrict
now reads the shared sys_automation_run row and consults the per-process map only for a run
whose durable save failed. Three other readers of the same state still prefer the per-process
map and were deliberately left alone, because they are the same class but not the same fix:

AutomationEngine.cancelRun — this.suspendedRuns.get(runId) ?? await this.store.load(runId).
On a replica holding a stale entry, the cancel acts on the stale SuspendedRun object: the
row deletion is still by id and therefore correct, but forgetSuspendedRun notifies the
WRONG node executor that its pause is over, so whatever that node armed is not the thing
torn down.
AutomationEngine.failAncestors — the same ?? chain while walking the $parentRunId
chain. This one has the harm shape of automation/approvals: 多副本集群下审批流每级节点(除首级)被重复创建 —— approve 后恢复读到滞后一拍的流运行态,同级要批两次(单副本零重复) #13617 itself: a stale parent means failing an
ancestor at a node it has already left.
AutomationEngine.listSuspendedRunsDurable — merges the durable list with the in-process
map under the comment In-memory entries win — they are the freshest copy, which is exactly
backwards once several replicas share one store. Lowest harm of the three: its own docblock
records that it has no in-repo production consumer.

Why it was not fixed in that PR

Sites 1 and 2 carry bespoke degradation contracts with their own recorded #4632/#6299 verdicts
in long docblocks — the posture at each was a deliberate judgment, so routing them through the
shared loader changes documented logging and degradation behaviour rather than being mechanical.
That failed the in-scope test for a bounded drive-by fix.

Suggested shape

One answer to where is this run parked, not four: route all of them through
loadSuspendedRun / loadSuspendedRunStrict and keep each site’s existing degradation posture
by choosing the degrading or strict loader deliberately, rather than by re-deriving the read.
The getSuspendedScreen docblock already states the principle (one loader, two callers).

Repro shape is the two-engines-over-one-shared-store harness added in
packages/services/service-automation/src/multi-replica-resume-staleness.test.ts.

Generated by Claude Code

Triage (R+90, triage seat, session session_019kDRpB7D2XzVzkaLp57T5D): pm:queue · priority:p2 · domain:services · type Bug.

Lands in: packages/services/service-automation/src/engine.ts — the three readers (cancelRun, failAncestors, listSuspendedRunsDurable). Ruled shape is the card's own: one answer to "where is this run parked" — route all three through loadSuspendedRun / loadSuspendedRunStrict, choosing the degrading or strict loader deliberately per site so each site's recorded #4632 / #6299 degradation posture is preserved by choice rather than re-derived; the getSuspendedScreen docblock already states the principle. p2 because site 2 (failAncestors) carries #13617's exact harm shape — failing an ancestor at a node it has already left — and site 1 tears down the wrong node executor's pause. ⛔ Site 3's "in-memory entries win" comment must be corrected in the same PR, not left as a true-sounding sentence that is backwards once several replicas share one store; its own docblock records it has no in-repo production consumer, so it is the cheapest of the three to make honest.

Family: #13617 (resume path, landed) · #14333 (the concurrent guard, queued this round) — same file; the services seat answers fold-or-serial for all three.

Size/model suggestion: M, opus; the harness is multi-replica-resume-staleness.test.ts.

Generated by Claude Code

Claim: domain:services PM seat, session session_01AUF1NoViznQK32gqpK8wS8 (GitHub os-sales), 2026-09-02 ~16:16Z.

Session: session_01AUF1NoViznQK32gqpK8wS8
Branch: claude/issue-14332-suspended-run-readers-store-authoritative
Worktree: /home/user/objectstack-issue-14332 (dev-created, worktree-first)
Domain: domain:services (packages/services/service-automation)
File surface: packages/services/service-automation/src/engine.ts — the three readers named by triage (cancelRun, failAncestors, listSuspendedRunsDurable) and their docblocks, plus the loadSuspendedRun / loadSuspendedRunStrict pair only if a site needs a loader variant that does not exist yet; tests under packages/services/service-automation/src/*.test.ts (the harness of record is multi-replica-resume-staleness.test.ts); one changeset (@objectstack/service-automation). ⛔ Not touched: packages/spec/**, content/docs/releases/**, skills/**, the subflow delegation block landed by PR fix(service-automation): answer a delegated subflow child refusal as a refusal, not a terminal failure #14567, the store implementation, service-automation: two concurrent resumes of one run on two replicas can both advance it — the idempotency guard is per-process #14333's concurrent-resume guard (queued separately, same file — see the serial note).
Serial constraints cleared: every open PR branch diffed against origin/main at ~16:14Z — no open PR touches packages/services/service-automation; engine.ts was released when PR fix(service-automation): answer a delegated subflow child refusal as a refusal, not a terminal failure #14567 (service-automation: a subflow parent resume treats the delegated child screen's RETRYABLE refusal (INVALID_SCREEN_INPUT) as a terminal child failure — the parent is failed, the still-paused child is orphaned, and the corrected retry answers RUN_NOT_FOUND #14379) merged at 13:21Z. The next card in the same file, service-automation: two concurrent resumes of one run on two replicas can both advance it — the idempotency guard is per-process #14333, stays in the queue until this one lands — one at a time, per the seat's hot-file rule (this claim holds the file).
Container & model: in-process os-dev subagent, model opus (triage's own suggestion: M, opus). node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --tier packages/services/service-automation/src/engine.ts on 13bf05d3f: "Model tier — no path-derived mandate: the surface hits none of the 3 declared glob(s) … The tier stays the PM's per-card judgment call (floor sonnet · default opus · ceiling fable). Clause ② is NOT reachable from paths … This line is a FLOOR, never a clearance."
Clause-②: expected no (a services implementation face; the three readers keep their signatures and their published behaviour except where the staleness is the defect). The dev re-declares from the ACTUAL diff, and hangs needs:contract-review on both carriers if it lands yes.
Ruling of record: no maintainer ruling is owed — triage graded the card dispatchable and fixed its shape in 14332#issuecomment-5503565404: one answer to "where is this run parked", all three sites routed through loadSuspendedRun / loadSuspendedRunStrict with the degrading-or-strict choice made deliberately per site so each recorded [convention] best-effort 降级导致"看起来正常、实则不持久"时不应记 warn——把 #4460 的点状修复定成规则 #4632 / finding(service-automation): engine.ts 还剩三处同形的 warn message 拼接 —— forgetSuspendedRun / cancelRun / listSuspendedRunsDurable,是 #5912+#6230 之后该文件的最后一批 #6299 degradation posture is preserved by choice rather than re-derived; ⛔ site 3's "In-memory entries win — they are the freshest copy" comment must be corrected in the same PR.

Race re-read follows this comment before the dev is launched.

Generated by Claude Code

os-dev-report

{
  "issue": 14332,
  "status": "done",
  "branch": "claude/issue-14332-suspended-run-readers-store-authoritative",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/14650",
  "head_sha": "89f5f8d15b (tree 514564fde37f75fd96c8f58e09ae139bc5ce0801 — byte-identical to da4db89f5, the commit the gate union and every measurement ran on; `git diff --stat da4db89f5 89f5f8d15` prints nothing. The second commit exists only because the branch was already pushed when a type-only import was corrected, and force-push is forbidden, so the amend was re-landed as a child commit.)",
  "premise_still_valid": true,
  "summary": "All three readers now take one answer to 'where is this run parked', through the existing loadSuspendedRun / loadSuspendedRunStrict pair, with the loader chosen deliberately per site. cancelRun takes the STRICT loader (its degradation decides a write and its #4632 DURABILITY record is at `error`; the degrading loader would have downgraded that verdict to its own best-effort `warn`), failAncestors takes the DEGRADING loader (it runs inside the catch arm already handling a failure, so it must not throw, and 'degrade to null and stop the walk' is exactly what its bare .catch(() => null) did), and listSuspendedRunsDurable gets the merge direction plus the corrected comment. Diff is 3 files: engine.ts, one new pin file, one patch changeset. No export, signature or return-shape change.",
  "per_site_judgment": {
    "cancelRun": "STRICT (loadSuspendedRunStrict). Not found -> false ('already terminal / unknown', its documented idempotent success), unchanged. Store unreadable -> throws out of the loader into this site's own catch, which keeps its #4632 DURABILITY record at `error` — the degrading loader would have answered null under its own `warn`. Named consequence: while a store is configured this process's map is no longer an answer, so a store outage reaches that `error` record even for a run this replica holds, where the old cache-first read cancelled from the local snapshot. That snapshot is the defect — the row delete is by id and right either way, but forgetSuspendedRun notified the executor of the node recorded on the SNAPSHOT.",
    "failAncestors": "DEGRADING (loadSuspendedRun). Not found -> ends the walk, unchanged. Store unreadable -> reads as 'no ancestor here' and ends the walk, never propagates — the posture the bare .catch(() => null) had, and required because this walk runs inside resumeInternal's catch arm. One thing gained: that silent swallow is now recorded, at the loader's declared best-effort `warn`. No new error-level site.",
    "listSuspendedRunsDurable": "NEITHER LOADER — merge direction plus the corrected comment. The durable row now wins an id collision. Map entries the durable listing does not carry are still included, deliberately: a LIST is not a per-id answer. store.list() is a capped best-effort enumeration (ObjectStoreSuspendedRunStore reads at most 1000 `paused` rows) and the same merge is reached on the DEGRADED path where the enumeration failed outright, so 'absent from the list' is not the 'the store answered and has no row' that loadSuspendedRunStrict rests on. Applying the strict qualifier here would let a truncated or failed enumeration silently drop live runs. Both the inline comment ('In-memory entries win — they are the freshest copy') and the docblock sentence that repeated it ('The in-memory cache takes precedence on id collisions') are corrected."
  },
  "premise_checks": "P1 HOLDS (cancelRun map-first), P2 HOLDS (failAncestors ?? chain), P3 HOLDS (list merged under the map with the 'freshest copy' comment), P4 HOLDS (the #13617 pair unreshaped, getSuspendedScreen still states 'one loader, two callers'), P5 all three sites defective — reds measured per site by ablation on the committed tree, no partially false premise, P6 HOLDS (packages/spec/** and content/docs/releases/** untouched). Verified on origin/main at 13bf05d3f before the first edit.",
  "hypotheses": {
    "H1": "holds — no third loader; the per-site choice is a judgment from each site's own docblock and is now stated in the code, one sentence per site.",
    "H2": "holds, with two named consequences rather than silent changes. No log site moved, no level changed, no thrown/returned shape changed, no row write changed; the #6299 byte-level pins for all three seams stay green untouched. Named: (a) cancelRun's existing `error` record now also covers the map-hit case under a store outage; (b) failAncestors' silently swallowed store failure is now recorded at the loader's `warn`. No new error-level site (#13398 class).",
    "H3": "holds, verified with positive controls. `git grep -n listSuspendedRunsDurable -- packages/** examples/** apps/** :!*.test.ts :!*.spec.ts` returns only engine.ts's own definition/comments plus CHANGELOG prose. Positive controls on the same command shape do find consumers: hasSuspendedRun -> packages/plugins/plugin-approvals/src/approval-service.ts:2704 (non-test), listSuspendedRuns -> packages/spec/src/contracts/automation-service.ts:589.",
    "H4": "holds — Clause-2 is `no`. From the actual diff: `git diff -U0 origin/main...HEAD | grep export` matches exactly one line and it is prose inside the changeset, not an export statement. Changeset is patch. No needs:contract-review hung.",
    "H5": "holds — #14333's concurrent-resume guard region untouched; the engine.ts diff is confined to the three readers and their docblocks."
  },
  "tests": "New pins: packages/services/service-automation/src/multi-replica-suspended-run-readers.test.ts (11 tests), a SIBLING of the #13617 harness — reason in its docblock: multi-replica-resume-staleness.test.ts carries a REVERT-PROOF ledger for one named mutation with measured 4 red / 4 green counts, and adding these cases would falsify those counts and merge two mutations' revert-proofs into one statement. Every site-1/site-2 pin asserts on the onSuspensionReleased NOTIFICATION (which node executor was told its pause is over), never on the row — the row delete is by id and correct either way. Runs (all under scripts/pm/os-verify-lock.sh, exit captured before any pipe, on the measured tree): (1) full package suite `pnpm --filter @objectstack/service-automation test` -> 'Test Files 100 passed (100) / Tests 1182 passed (1182)', 'os-verify-lock: VERDICT command-exit 0'. (2) Type-check: the package has NO typecheck script (a shrink-only DEBT ledger entry frozen at 3), so `pnpm --filter ... typecheck` would have matched zero scripts and exited 0 having measured nothing; measured instead with the dependency closure built (`pnpm --filter '@objectstack/service-automation^...' build`, the world the ledger uses) via `tsc --noEmit -p tsconfig.json --listFiles` -> exactly 3 errors, all pre-existing TS2341 in src/nested-region-parity.test.ts at 95/151/180, none in the changed files; --listFiles confirms the new test file IS in the program. That run is why a fourth error (TS2459, a type imported from the wrong module) was caught and fixed before the head was final. (3) Repo-wide lint, no narrowing needed: `node --stack-size=4000 node_modules/eslint/bin/eslint.js . --no-inline-config` -> EXIT=0, 'VERDICT command-exit 0 · held the lock 116s'.",
  "gates": "Derived on the final tree, not hand-written: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` (--repo asserted and held against this checkout's origin) -> 36 commands over the 3 changed paths. Result: 33 green, 0 red, 3 NOT MEASURED. The three NOT MEASURED are prerequisite refusals at exit code 3, which each script's own verdict text distinguishes from a finding's 1: check-test-completeness.mjs ('PREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named'), check:dual-build-cjs-loads ('PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/'), check:type-check-debt ('PREREQUISITE NOT MET ... --re-measure cannot run: 35 workspace dependenc(ies) ... have no built type entry point on disk'). Its sibling check:type-check-coverage ran GREEN ('check-type-check-coverage: OK — 69/79 workspace packages type-checked ... 10 in the DEBT ledger'), and the targeted tsc reading above answers the debt gate's substantive question for the one package this PR touches, in the built-closure world the ledger uses. Also green outside the derived family: `pnpm check:nul-bytes` ('OK (scanned 7992 text file(s) ... no raw ASCII control bytes)') plus a direct control-byte grep of both changed files. Merge preflight before opening the PR: `git merge-tree --write-tree --name-only origin/main HEAD` listed no conflicting paths, so content/docs/permissions/system-context.mdx needs no regeneration.",
  "ablation": "Three ablations, one per site, on the COMMITTED tree. Each mutation restores that site's map-first read, is line-neutral, is proven on disk by anchored occurrence counts BEFORE the run, and is restored with `git checkout HEAD -- ABSOLUTE_PATH` inside a `trap ... EXIT INT TERM`, the restore proven by `git hash-object` equal to the HEAD blob (f2c33d8b57cea0dce4054de3b8f1ee0ac9b88e6a) AND an empty `git diff HEAD`. No rebuild is interposed because the pins import ./engine.js relatively (vitest resolves that to src/engine.ts, not the package's dist/ through exports); the reds themselves are the proof the mutation reached the code under test. Measured, not predicted: site 1 (`run = this.suspendedRuns.get(runId) ?? await this.loadSuspendedRunStrict(runId)`) -> 2 red / 9 green (THE BUG: the teardown names lv1 while the run is parked at lv2; plus NEW REACH, since a map hit means the unreadable store is never read). Site 2 (`const parent = this.suspendedRuns.get(parentId) ?? await this.loadSuspendedRun(parentId)`) -> 2 red / 9 green (the ancestor is failed at lv1, a node it has already left; plus the unreadable-ancestor control, for site 1's reason). Site 3 (drop the `byId.has(r.runId)` guard) -> 1 red / 10 green (the listing reports lv1, one level stale). TWO READINGS WERE VOIDED RATHER THAN REPORTED: the first site-1 mutation used a one-line anchor occurring twice in engine.ts (the other in resumeInternal) and the guard refused at anchor count 2; a second attempt was voided by the on-disk proof before any test ran. Both are named because a silently re-run ablation is the defect one layer up. The test-file docblock's REVERT-PROOF section was rewritten to the measured counts after these runs — my pre-measurement prediction ('only that site's THE BUG goes red') was wrong for sites 1 and 2.",
  "clause_2": "no",
  "mcp_calls": "2 — mcp__github__issue_read x2 (get, get_comments). Everything else went through repo-scoped REST (probed 200 in this container) and git.",
  "open_questions": [],
  "out_of_scope_findings": []
}

Generated by Claude Code

PM ACCEPT — PR #14650 at `89f5f8d15` (Clause-② `no`, verified on the tree)

Collected the os-dev-report and verified the load-bearing claims against origin/main and the branch, not against the report.

Triage's shape, met. The three readers now route through the existing loadSuspendedRun / loadSuspendedRunStrict pair, and the per-site choice is argued rather than mechanical — cancelRun strict (its degradation decides a write and its #4632 record is at error; the degrading loader would have downgraded that to its own warn), failAncestors degrading (it runs inside the catch arm already handling a failure, so it must not throw — the posture the bare .catch(() => null) had, now with the swallow recorded), listSuspendedRunsDurable neither (a list is not a per-id answer: store.list() is a capped best-effort enumeration and the same merge is reached on the degraded path, so "absent from the list" is not the "the store answered and has no row" the strict loader rests on). Site 3's "in-memory entries win — they are the freshest copy" comment and the docblock sentence that repeated it are corrected, as triage required.

Seat verification (measured).

Non-comment delta in engine.ts is exactly the three sites: cancelRun's map-first read replaced by loadSuspendedRunStrict inside the site's own catch (the error record's text unchanged), failAncestors' ?? chain replaced by loadSuspendedRun, and the byId.has(r.runId) guard that flips the merge direction. Nothing else in that file moved — service-automation: two concurrent resumes of one run on two replicas can both advance it — the idempotency guard is per-process #14333's region is untouched, so the two cards can land in sequence.
File surface: 3 files (engine.ts, one new pin file, one patch changeset).
Clause-② no: git diff -U0 origin/main...HEAD | grep -E '^[+-].*\bexport\b' matches exactly one line, and it is changeset prose ("No signature, export or return-shape change on any of the three."), not an export statement. No new exported symbol, no signature change.
Head identity: the report rests its measurements on da4db89f5 while the head is 89f5f8d15. Checked rather than taken on trust — both commits carry tree 514564fde37f75fd96c8f58e09ae139bc5ce0801 and git diff --stat between them prints nothing, so the readings carry to the head.
git merge-tree --write-tree --name-only origin/main HEAD lists no file. At dispatch time (16:14Z) no open PR touched packages/services/service-automation; CI's own No other open PR may claim the same single-writer path is the mechanical check on the current queue.
Log levels: no log site moved, no level changed, no new error-level site. Two consequences are stated in the docblocks and the changeset instead of left to be discovered: cancelRun's existing error record now also covers a map-hit under a store outage (that local snapshot is the defect), and failAncestors' previously silent store failure is recorded at the loader's warn.

Worth noting for the record: the dev voided two ablation attempts rather than reporting them — the first site-1 mutation anchored on a line occurring twice in engine.ts, and the guard refused at anchor count 2. It also recorded that its own pre-measurement prediction for sites 1 and 2 was wrong (two pins red each, not one) and rewrote the test file's revert-proof to the measured counts. Both are the right handling.

Landing: on all checks green at 89f5f8d15 the seat flips the PR to ready, arms auto-merge and posts landing provenance; on MERGED the card closes, pm:dispatched comes off, and engine.ts is released to #14333 — the next card in the same file, held in the queue behind this one.

Generated by Claude Code

Navigation Menu

Repository navigation

service-automation: three more readers of suspended-run state still prefer the per-process map over the shared store #14332

What

Why it was not fixed in that PR

Suggested shape

PM ACCEPT — PR #14650 at `89f5f8d15` (Clause-② `no`, verified on the tree)

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

service-automation: three more readers of suspended-run state still prefer the per-process map over the shared store #14332

Description

What

Why it was not fixed in that PR

Suggested shape

Activity

huangyiirene commented on Sep 2, 2026

os-sales commented on Sep 2, 2026

claude commented on Sep 2, 2026

os-sales commented on Sep 2, 2026

PM ACCEPT — PR #14650 at 89f5f8d15 (Clause-② no, verified on the tree)

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

PM ACCEPT — PR #14650 at `89f5f8d15` (Clause-② `no`, verified on the tree)