test(service-automation): pin the node comparison with a correlation-less parking, and correct the comment that claimed it was already pinned

[os-warren]

Fixes #14956

Test-only. No behaviour changes — suspended-run-store.ts and engine.ts are untouched.

The gap

InMemorySuspendedRunStore.claimSuspension compares two things, and only one of them was falsifiable. Both existing CONDITION cases in concurrent-replica-resume-race.test.ts re-park with a new correlation (req_lv1→req_lv2, map:item_1→map:item_2), so the correlation comparison rejects the stale claim first and masks the node comparison entirely. Deleting if (run.nodeId !== parkedAt.nodeId) return 'lost'; alone left the whole three-file population green — on the comparison the maintainer's ruling names verbatim ("delete only if still parked at node N"), on the path whose purpose is stopping two replicas resuming one run twice.

And the file's own CONDITION comment asserted the opposite: "one per comparison, so a mutation that deletes only one of them still reddens." False for the node half. A comment describing a red that does not exist is worse than none.

Re-derived bench — the card's numbers reproduce exactly

Card's numbers were taken at feb213d42. Re-driven here on origin/main d4f9b2a9d — population unchanged at 61, every leg identical, zero delta:

mutation to InMemorySuspendedRunStore.claimSuspension	card (feb213d42)	re-derived (d4f9b2a9d)
baseline	61 passed (61)	61 passed (61)
both comparisons deleted	2 red	2 failed | 59 passed (61)
correlation alone	1 red	1 failed | 60 passed (61)
node alone	0 red	61 passed (61) — 0 red

The pin, driven and mutation-checked here

Reconstructed rather than accepted as given — the point of the card is that an unverified claim about a red was wrong.

THE CONDITION (node, correlation-less): a pausing executor that mints no correlation — the shape SuspensionParkedAt documents as legitimate ("a row persisted with no correlation has nothing to compare, and the node condition still holds") — with replica B's claim held on a latch until replica A has advanced, fired notify, and re-parked at lv2. The claim B then presents carries no correlation key at all, so nodeId is the only field the store can test.

Green at HEAD; under the node-only deletion it fails at concurrent-replica-resume-race.test.ts:661 with:

AssertionError: expected true to be false // Object.is equality
- Expected  false
+ Received  true

B's stale claim granted, and led.fired becomes ['notify','notify'] — the action fired twice.

With the pin, node-alone is 1 failed | 61 passed (62), and the one red is this case.

Every ablation leg mutated the source on disk, proved it landed (anchored grep -Fc counts plus a git hash-object delta against the HEAD blob) before reading any result, restored from HEAD inside a trap … EXIT INT TERM with absolute paths, and proved the restore by an empty git diff HEAD and a blob hash equal to HEAD's. One leg (C2) initially reported MUTATION NOT CONFIRMED because my on-disk check anchor (multi: true) was not unique in the file; the harness refused to measure, I narrowed the anchor and re-ran. That refusal working is why the numbers below are trustworthy.

The comment, corrected to the measured truth

The false sentence is replaced by the measurement, both readings labelled so neither can be misread:

//                       WITHOUT the third case   WITH it (today)
//   both comparisons       2 red / 61 tests         3 red / 62 tests
//   correlation alone      1 red                    1 red
//   node alone          ⛔ 0 RED                     1 red

The file's REVERT-PROOF ledger, re-measured in full

Adding a case to the population invalidates every total the header quotes, and this file's whole subject is a measurement claim that had gone false — so all five legs were re-driven rather than adjusted on paper, on the 62-test population:

leg	measured
(E) the engine stops asking	10 failed | 52 passed (62) — 8 here, 2 in suspended-run-store.test.ts; staleness stays 8/8
(C) both comparisons	3 failed | 59 passed (62) — the three CONDITION cases, nothing else
(C-node) node alone	1 failed | 61 passed (62) — new row
(C-corr) correlation alone	1 failed | 61 passed (62) — new row
(C2) ObjectStore loses multi: true	7 failed | 55 passed (62) — all in suspended-run-store.test.ts

Splitting (C) into its halves is what exposed the unfalsifiable one, so the two new rows stay.

⚠️ Deliberately not touched: the ObjectStore side needs no such pin — dropping node_id from its where is already caught by that suite's exact-shape assertions (leg C2 above, 7 red). This gap was specific to the in-memory store.

Verification

Head e60ca587d (includes a merge of origin/main b2375921b; the closure was rebuilt after that merge — before it, a stale @objectstack/spec dist reported two phantom TS2305 errors from sibling PR #15792's new exports).

• pnpm --filter @objectstack/service-automation exec vitest run → 110 passed (110) files, 1317 passed (1317) tests
• pnpm --filter @objectstack/service-automation typecheck → exit 0, and the edited file is proven inside the program (tsc -p tsconfig.test.json --listFiles names it)
• Gate family re-derived from the actual changed path with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (45 owed): 43 exit 0, 2 at exit 3 = PREREQUISITE NOT MET = NOT MEASURED (check:dual-build-cjs-loads, check:type-check-debt — both read built output of the whole workspace, which this worktree does not have). ⛔ Neither is a pass; CI measures them.

Also owed by the card, and it did not exist

The card flagged as NOT VERIFIED that a grouped follow-up for the prior review's §5 notes 1 and 5 could not be found. I searched rather than assuming: a targeted issue search returned only #14956 itself, #15222 and an unrelated domain:skills card, and a local grep found nothing. It did not exist, so it is now filed bare (no labels — routing is the PM's) as #15832, with both notes re-verified against current main rather than carried over on the review's word. Out of scope here: #15832 is not addressed by this PR.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see

• a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json ec0a6e7bd90f11334967c85f37ee489a3808c0c8 → packageMentionDocs.

[os-warren]

Clause-② contract review — PR #15836 (card #14956) — verdict: PASS

Head reviewed: e60ca587d · merge-base b2375921b · 1 file (+166/−20), test-only · dedicated detached worktree (/home/user/objectstack-rev15836), tree provably clean after every mutation and re-proven after an interruption before this was posted: git status --porcelain empty, git diff HEAD 0 lines, suspended-run-store.ts 03b860aa3… / engine.ts 3a44f8da7… / the test file 45d33e234… all equal to their HEAD blobs.
Tier line: CONTRACT_REVIEW_TIER = 'claude-fable-5-1' (scripts/pm/dispatch-gates.mjs:8659) — override + self-report: the PM seat attests an explicit model: fable override on this Agent call; this reviewer self-reports identity claude-fable-5-1. get_session not used. This is not an "exact match" reading.
Independence pair: Implemented-by: claude/issue-14956-pin-node-comparison (mode:subagent dev) · Reviewed-by: context-isolated review subagent of the domain:services PM seat, PM session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909 (session_01XpTx2tbq3pZRYAdoGt6E6Y). Different subagent contexts under the same dispatching session; not treated as the SELF-REVIEW case — the PM seat rules if it reads otherwise.

Already verified by the PM and not redone here: head match, merge-base, exactly one file changed, 0 non-test source touched, ADR-0087 exit 0.

How this was weighed. A test-only PR whose entire content is claims about which mutations go red, on a card that exists because a comment in this very file asserted a red that did not exist. So the numbers were re-derived, not read. Every leg below was driven on the head tree by my own harness (ablate-rev15836.sh, scratchpad, never committed): an anchored edit that is refused unless the anchor occurs exactly once, blob-hash delta proven before vitest ran, restore from HEAD inside a trap … EXIT INT TERM, restore proven by blob == HEAD and an empty porcelain. Population: the three files the ledger names; --maxWorkers=1; under scripts/pm/os-verify-lock.sh (slot rev15836). Closure built at 10:31Z from this worktree's own source at the head (10 packages, --workspace-concurrency=1, exit 0); all runs 10:32–10:40Z; each leg has its own log.

1 ⭐ The bench, both halves — reproduces exactly, and the pin does its job

Pre-pin legs swap in the merge-base blob of the test file (fec1f3d61, proven by hash; the third case's anchor count 0) on the otherwise identical head tree. Since b2375921b..HEAD is that one file, this is the pre-pin population.

mutation to InMemorySuspendedRunStore.claimSuspension	pre-pin (61) — claimed	measured	post-pin (62) — claimed	measured
baseline	61 passed	61 passed (61)	62 passed	62 passed (62)
both comparisons deleted	2 failed	2 failed | 59 passed (61) (:457, :544)	3 failed	3 failed | 59 passed (62) (:497, :584, :682)
correlation alone	1 failed	1 failed | 60 passed (61) (:544)	1 failed	1 failed | 61 passed (62) (:584)
node alone	0 red	61 passed (61) — ZERO RED	1 failed	1 failed | 61 passed (62) — the correlation-less case, AssertionError: expected true to be false at :682:33

Node-only flips 0 → 1 with the pin present, and the one red is the new case. That is the card's requirement, met. (The dev's bench was driven pre-merge at 4daf36125/063dd2cf2 on base d4f9b2a9d; the head then merged a main that changed engine.ts (+40, #15792). Every figure holds on the merged head.)

2 The pin — it is the right pin, and not a tautology

• The correlation really is absent. uncorrelatedReplica (:603) returns { success: true, suspend: true } from the pausing executor (:619) — no correlation; the case asserts the persisted row has correlation === undefined (:656) and that the claim B presents is [{ nodeId: 'lv1' }] (:673, delayedClaimClient recording { ...parkedAt }). claimAdvance spreads the key out entirely when run.correlation === undefined (engine.ts:2215), so the store's correlation test (suspended-run-store.ts:165, parkedAt.correlation !== undefined && …) is skipped and the node test (:162) is the only one consulted. The masking mechanism that made the original comment false cannot operate here.
• B's stale claim is genuinely granted under the mutation. Scratch probe (same scenario, values printed, file deleted afterwards): at HEAD bSuccess=false bCode=RESUME_IN_PROGRESS fired=['notify'] opened=['lv1','lv2']; under node-only deletion bSuccess=true bStatus=paused fired=['notify','notify'] opened=['lv1','lv2','lv2'] — the claim granted, notify fired twice, lv2 opened twice. The PR's description of the red is exact.
• The corrected comment (:410-413) matches the measurement on every cell — both readings, labelled.
• Nit, non-blocking: expect(seen).toEqual([{ nodeId: 'lv1' }]) — toEqual ignores undefined-valued keys, so "no correlation key at all — not an empty one" is pinned against '' but not against correlation: undefined. Semantically irrelevant to the store's !== undefined predicate, and the engine spreads the key out anyway; toStrictEqual would say what the comment says.

3 ⭐ The re-measured REVERT-PROOF ledger — every total verified on the head

leg	ledger claims	measured
(E) engine stops asking (claim pinned to unguarded)	10 failed / 52 passed — 8 here, 2 in the store test; staleness 8/8	10 failed | 52 passed (62) — race file 12 tests / 8 failed, suspended-run-store.test.ts 42 / 2 failed (:1009, :1030), multi-replica-resume-staleness.test.ts 8/8 green
(C) both	3 failed / 59	3 failed | 59 passed (62)
(C-node)	1 failed / 61	1 failed | 61 passed (62)
(C-corr)	1 failed / 61	1 failed | 61 passed (62)
(C2) ObjectStore loses multi: true	7 failed / 55, all in the store test	7 failed | 55 passed (62), all 7 in suspended-run-store.test.ts, on the producer's own refusal ("Delete names one row by primary key, but options.where also carries predicate keys 'node_id', 'correlation'…")

No total is stale. The ledger this card was filed to fix is right on the 62-test population.

4 The aborted C2 leg — the refusal path exists and works

• In the dev's harness (issue-14956/ablate2.sh) the python step's assert n == 1 runs before any write, so a non-unique anchor exits non-zero with nothing on disk; the shell then prints MUTATION NOT CONFIRMED: python failed and exits 1 inside the restoring trap. The vitest log is only written after MUTATION CONFIRMED ON DISK, so an aborted run leaves no measurement to read — which also means there is no artifact of the abort to cite; the mechanism is what is checkable, and the dev's account is consistent with it.
• The anchor really was non-unique: multi: true occurs 3 times in suspended-run-store.ts (:339, :345 in the docblock, :390 in code). The narrowed anchor occurs once.
• Demonstrated with my harness's C2bad leg (the original anchor): anchor 'multi: true' occurs 3 time(s) … not writing the file … MUTATION NOT CONFIRMED — REFUSING TO MEASURE, exit 93, no vitest run, no log created, restore proven by blob == HEAD.

5 #15832 — filed bare (0 labels, 0 assignees, confirmed); Note 5 holds; ⭐ Note 1's severity is UNDERSTATED

• Note 5 — verified by reading and by run. suspended-run-store.ts:390 await this.engine.delete(…, { where, multi: true, … }) precedes the typeof affected !== 'number' refusal at :391 → 'unsupported' at :396; claimAdvance maps 'unsupported' → unguarded (engine.ts:2234); resumeInternal then calls forgetSuspendedRun(run, 'resumed', claim.kind === 'claimed') = false (:5409), whose store.delete(runId) is unconditional (:2114). Probe with an engine double whose multi-delete resolves undefined: deletes issued during one resume = [{ where: { id, node_id: 'lv1', correlation: 'req_lv1' }, multi: true }, { where: { id } }] — the compare-and-set went out first (and removed the row), its verdict was discarded, the store warned "resolved undefined, not an affected-row count", the engine warned "no cross-replica advance guarantee (the attached suspended-run store answered 'unsupported')", and a second unconditional delete followed. Exactly as filed.

• Note 1 — the leak is real, but "the stale entry is not read back" is false. The issue's argument covers only the by-id loader: loadSuspendedRunStrict is store-first (:4988-4999) and the 'lost' branch returns (:5379) before the one suspendedRuns.delete (:2107) — all confirmed. But engine.ts has two more readers of the map: listSuspendedRuns() (:6383-6384, synchronous and cache-only by design, documented as backing a "pending approvals" view, and per the engine's own comment the one listing that is on the AutomationService spec contract) and listSuspendedRunsDurable() (:6403, which at :6474 deliberately appends map entries the durable list lacks). Probe, two engines over one in-memory store, A parks and loses a latched claim to B:

  • after A's 'lost': store row at lv2; a.listSuspendedRuns() → [{ nodeId: 'lv1', correlation: 'req_lv1' }] (stale node); a.listSuspendedRunsDurable() → lv2 (durable wins); a.hasSuspendedRun() → true.
  • after B completes the run (store row gone): a.listSuspendedRuns() → still lv1; a.listSuspendedRunsDurable() → lv1 appended — a completed run listed as suspended; a.hasSuspendedRun() → false; getSuspendedScreen() → null.
  • the no-race variant (A parks, only B ever resumes, A never tries) leaks identically — so the eviction site the issue points at ('lost') is not where the leak lives; it is any run parked by one process and consumed by another.

  So the by-id and resume paths are correct (the issue is right there, and the doubled side effect service-automation: two concurrent resumes of one run on two replicas can both advance it — the idempotency guard is per-process #14333 exists to prevent cannot come from them), while the two listing surfaces hand the stale entry back, one of them a spec-contract method. Severity should read "unbounded per-process leak plus stale/phantom rows on the suspended-run listings", and the remedy needs more than evicting on 'lost' (e.g. evict on a store miss in loadSuspendedRunStrict for a non-cacheOnlySuspensions run, and/or a sweep). Not this PR's to fix; a correction on service-automation: two loose ends on the #14333 claim path — the loser's hot-cache entry is never evicted, and the 'unsupported' branch answers only after its conditional delete has already been issued #15832 is owed.

6 skip-changeset

Label present (size/m, tests, skip-changeset); CI Check Changeset green. Precedent real: f501453b9 is the merge of PR #15782 (four *.test.ts files, +26, tests + skip-changeset, merged by the maintainer). Publishes nothing: package.json files = ["dist", "README.md", "CHANGELOG.md"]; no .changeset/ file in the diff.

7 Honesty audit

• STALE TREE, not reported. gates.err (09:57:43Z, at 063dd2cf2): "at least 5 commit(s) behind origin/main, and 22 file(s) it derives from CHANGED". git rev-list --count d4f9b2a9d..b2375921b = 5. gates2.err (at e60ca587d): no stale line. gate-results.txt: 45 rows, 43 × EXIT=0, rows 29 (check:dual-build-cjs-loads) and 42 (check:type-check-debt) EXIT=3. My own derivation on the head worktree: the same 45 commands — with a new stale-tree warning because origin/main has since moved on (2 derivation-source files changed); CI's Lint & Repo Gates on the merge into current main is green, which is the reading that counts.
• The narrowing is honest and, for this package, more than sufficient. check:type-check-debt = check-type-check-coverage.mjs --re-measure, which refreshes the ledgered packages' built closure over the whole workspace and exits 3 (PREREQUISITE NOT MET — nothing measured) without it. @objectstack/service-automation graduated from that ledger (tooling(services): service-automation has no typecheck script — its test layer is compiled by no tsc program (#14062 family, sibling of #14181) #15048, check-type-check-coverage.mjs:771; no test-typecheck-debt.json beside its tsconfig.test.json), so --re-measure has no entry to re-measure for it; the structural mode (no --re-measure) exits 0 on this head. The package's own typecheck re-run here: exit 0, check:test-typecheck: OK … 0 file(s) / 0 error(s); tsc -p tsconfig.test.json --listFiles: 564-file program, the test file named once, 0 errors. check:dual-build-cjs-loads reads emitted dist/**/*.cjs, which a *.test.ts cannot change. CI: Type Check · debt ledger (hosts check:type-check-debt, lint.yml:5066) and Build Core (hosts check:dual-build-cjs-loads, ci.yml:1592) both green on this head.
• Stale-dist trap, diagnosed not reported. typecheck.log: exactly two TS2305 — structuralConditionRefusal (engine.ts) and STRUCTURAL_CONDITION_SHAPE_REFUSAL (structural-condition-shape.test.ts), both "no exported member" of @objectstack/spec/automation. Those are precisely the two exports service-automation: refuse a structural flow condition that is neither CEL text nor an expression #15792 added to packages/spec/api-surface/automation.json; service-automation: refuse a structural flow condition that is neither CEL text nor an expression #15792 is in the 5-commit merged range and adds the engine.ts:33 import; typecheck2.log after the rebuild is clean. Phantoms, correctly named.
• CI on the head: 36 check runs; Build Core, Lint & Repo Gates, all four Type Check jobs, Check Changeset, Test Core shards 2–6 green; shard 1/6 still in progress at posting time; none failed.
• Nit, non-blocking: the PR body cites the red at :661; that is the line at the pin commit 4daf36125 — the ledger commit 063dd2cf2 added 21 header lines and the assertion is at :682 on the head. The file's own comment carries no line number, so the file is right; the body is one commit stale.

Contract judgment (①②③)

① Test-only; no accept/reject behaviour changes, no public surface widened; suspended-run-store.ts and engine.ts untouched (0 non-test source, PM-verified; my mutations restored to HEAD blobs). ② No semver question — publishes nothing. ③ Boundary flag: #15832 filed bare, out of scope and correctly not addressed here; its Note 1 needs the severity correction in §5.

Verdict: PASS. The node-only leg flips 0 → 1 with the pin present; the pin is the correlation-less shape the card asked for and reds on the node comparison alone (claim granted, notify fired twice); all five ledger totals hold on the merged head; the harness refusal path exists and works; skip-changeset rests on real precedent; and the honesty audit finds the STALE TREE / exit-3 / phantom-TS2305 account accurate. Two non-blocking nits for the PR body or a follow-up commit (:661 → :682; toStrictEqual on seen). Owed elsewhere, not by this PR: a severity correction on #15832 (§5).

---

Generated by Claude Code

---

Generated by Claude Code