Skip to content

fix(service-automation): a conditional advance claim on SuspendedRunStore, so two replicas cannot both advance one run - #14712

Merged
os-project-manager merged 9 commits into
mainfrom
claude/issue-14333-concurrent-resume-store-guard
Sep 3, 2026
Merged

fix(service-automation): a conditional advance claim on SuspendedRunStore, so two replicas cannot both advance one run#14712
os-project-manager merged 9 commits into
mainfrom
claude/issue-14333-concurrent-resume-store-guard

Conversation

@claude

@claude claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #14333

Two concurrent resumes of one run on two replicas could both advance it, running
every downstream side effect twice. The idempotency guard was this.resuming, an
in-process Set — a complete guard for exactly one deployment shape, a single
process. #13617 closed the sequential half of this family; it deliberately did not
close the concurrent one.

Ruling of record

Triage 14333#issuecomment-5503564326, quoted verbatim:

Not a decision card, and the reason matters: "two replicas both advance one run and its side effects run twice" is a broken exactly-once invariant, and restoring an invariant is explicitly not an escalation class — the finding carries its own decision. What the card actually asks is which remedy, and that is ruled here so the dispatch is bounded:

Measure first, then the store-side guard. ① Size the real exposure: two decisions on one run inside one resume window — drive it on the two-engines-over-one-shared-store harness (multi-replica-resume-staleness.test.ts), with parallel / any-of approvers and automated approve calls, which the card names as the reachable shapes. A single-approver-per-level flow that measures unreachable is a complete answer and gets recorded, not patched. ② If reachable, take the conditional advance on SuspendedRunStore ("delete only if still parked at node N") over the optimistic-version column: the guarantee belongs where the shared state lives, an implementation that cannot express it must say so loudly rather than silently offering none, and it touches no stored-data shape. ⛔ The sys_automation_run version column is not the lane's to take — a platform-object schema change is the maintainer's floor; if the measurement shows the store interface cannot express the guarantee, that is a fork report to the decision inbox, not a unilateral migration.

① The measurement, first — the race is REACHABLE

Measured on the two-engines-over-one-shared-store harness before any remedy
existed, on the merge base's engine. The instrument is a SIBLING file,
packages/services/service-automation/src/concurrent-replica-resume-race.test.ts,
not more cases inside multi-replica-resume-staleness.test.ts: that file carries a
REVERT-PROOF ledger keyed to its own mutation ("4 red / 4 green", case by named
case), and adding cases to it would silently invalidate those counts. Its ledger is
untouched and still true.

"Both advanced" is asserted on OBSERVABLE EFFECTS, never on internal bookkeeping.
The flow puts a real side-effect node between two approval levels — start to
lv1 (pauses) to notify (fires) to lv2 (pauses) to end — so one advance past
lv1 fires notify once and opens lv2 once. The defect's signature is notify
in the fired ledger twice and lv2 opened twice: an action fired twice and a node
executed twice, which is the duplicated approval request the family was reported as.

shape reachable? measured before the remedy
parallel / any-of approvers (two approvers, one level, same instant, two replicas) YES fired = [ 'notify', 'notify' ] where [ 'notify' ] is correct; opened = [ 'lv1', 'lv2', 'lv2' ]
automated approve calls (one automated decision delivered twice, two replicas) YES identical: [ 'notify', 'notify' ]
SIZED, 25 raced runs YES, every time { trials: 25, doubled: 25, extraOpens: 25 } — every raced run advanced twice and re-opened its next level
single approver per level, sequential (the card's "not obviously reachable") NO passes unchanged before and after; there is no window in which two resumes overlap
single replica, two concurrent approves NO the in-process resuming set already refuses one, RESUME_IN_PROGRESS

Both shapes the card named as reachable ARE reachable, deterministically. The shape
it called not obviously reachable measures unreachable and is kept as the negative
control.

② The remedy — the conditional advance on SuspendedRunStore

SuspendedRunStore gains claimSuspension(runId, parkedAt): consume the durable
record only if it is still parked at the node the caller read, and — when the caller
has one — still carrying that correlation, as ONE atomic operation. It answers
'claimed' / 'lost' / 'unsupported'.

  • The winner advances. The loser is refused and runs nothing.
  • this.resuming stays as the cheap first gate; it is not replaced, and it
    still refuses a same-process duplicate with no store round-trip.
  • The loser's code is the EXISTING RESUME_IN_PROGRESS, deliberately: the
    observation differs (another process, not this one) but the remedy is identical,
    plugin-approvals already branches on it that way in resumeRecordedOutcome
    ("a concurrent resume is already advancing the run, so the outcome stands"), and
    the transport already maps it to 409. A distinct code would be vocabulary nothing
    reads.
  • No platform-object schema change. node_id and correlation are columns
    sys_automation_run has always carried, so the condition is expressible against
    the row exactly as it stands. The version column stays untaken.

Three values rather than a boolean, because "I cannot express this condition" is a
different fact from "you lost": a store answering false for it would stall every
resume, one answering true would offer no guarantee while claiming one.

Both shipped implementations owe it, and both pay:

  • InMemorySuspendedRunStore — tests and removes with no await between the two,
    so on a single JavaScript thread it IS one atomic operation. Spelling it load()
    then delete() would put a microtask boundary in the middle and reopen the window.
  • ObjectStoreSuspendedRunStore — one DELETE ... WHERE id = ? AND node_id = ?
    (plus correlation when present) through the data engine's own documented
    compare-and-set route: multi: true with a full where, which
    engine-delete-dispatch.ts states dispatches to driver.deleteMany with the
    composed AST, whose result is contracted to be an affected COUNT. A pure-id
    where would take the by-id route and silently discard the condition — the one
    shape that must never happen here.

Nothing silently offers no guarantee. Four cases never reach the store and each
is a deliberate, documented unguarded:

case why announced?
no store attached nothing is shared, so there is no second replica to race; resuming already IS the whole guarantee no — there is no cross-replica guarantee to weaken
a run the store never accepted (cacheOnlySuspensions, #13617) the store holds no row, so a compare-and-set would answer lost for a run that is legitimately resumable in this process no — it would convert persistSuspendedRun's documented degradation into an unresumable run
a store with no claimSuspension (a third-party store predating it) it cannot express the condition yes, once per engine
a store answering 'unsupported' (no engine delete, or a non-count result) it cannot decide the condition yes — this one is emitted by the STORE, deduped once per store instance

Log levels, chosen deliberately.

  • The missing-guarantee declaration is warn, and it is said ONCE. The engine's
    own line (a store with no claimSuspension) is once per engine instance; the
    store's line (a composition that answers 'unsupported') is emitted by
    ObjectStoreSuspendedRunStore and deduped once per STORE instance, which is the
    narrower carrier and never louder than once per engine. [convention] best-effort 降级导致"看起来正常、实则不持久"时不应记 warn——把 #4460 的点状修复定成规则 #4632's
    question: after the degradation, does the system look normal while something it
    claims is persisted has not landed? No — nothing claimed-persisted fails to land;
    what is smaller than advertised is a GUARANTEE, and the line says exactly what is
    weaker and how to close it. It is byte-for-byte the call
    AutomationEngine.claim()'s missing-ledger branch makes one screen up, for the
    same reason. ⛔ Deliberately not error: no new error-level site through a
    published sink shape.
  • The loser learning it lost is debug. It is an ORDINARY outcome — the guard
    doing its job — and the caller is told in the result. At warn a busy any-of
    level would emit a steady stream of records describing correct behaviour.
  • A claimSuspension that THROWS is handed to the caller as STORE_UNAVAILABLE
    with the cause, and gets no log at all: AGENTS.md's third legal answer, "a failure
    handed to the CALLER is not a degradation at all". The message states only what
    this seam KNOWS: this resume did not continue the run, and whether the store
    consumed the suspension is UNKNOWN — a claim throws just as readily after a
    committed delete (a transport drop post-commit) as before one. ⛔ It deliberately
    does NOT say "the suspension was not consumed"; only the STRICT LOAD's failure one
    screen up may say that, because it fails before anything is consumed. A retry
    settles the rest: one that finds the run still parked continues it, one that finds
    the suspension gone answers RUN_NOT_FOUND ([automation/approvals] 进程重启后审批决策静默失效:挂起 flow run 仍只存内存(#1518 标记 COMPLETED 但 17.0.0-rc.1 未生效),approve 落库却永不推进且零报错 #4420).

Contract-first re-read, re-taken here

Triage's conditional — if SuspendedRunStore is declared in packages/spec, the
interface half goes to the spec seat — does not fire, re-measured in this
worktree rather than trusted:

$ git grep -n "SuspendedRunStore" -- packages/spec
packages/spec/src/data/object.zod.ts:624: *      `ObjectStoreSuspendedRunStore` in `service-automation` (`serialize()`

One hit, a docblock reference, not a declaration. The interface is declared at
packages/services/service-automation/src/engine.ts and nowhere else. No spec-seat
handoff; packages/spec is untouched by this PR.

Clause-② — YES, declared from the actual diff

The exported SuspendedRunStore interface gains a member, and two new exported
types (SuspensionParkedAt, SuspensionClaimOutcome) reach the barrel. That is a
public-surface widening. needs:contract-review is hung on this PR and on #14333.

The member is optional, so no existing implementation is compile-broken and no
test double in the repo needed a line changed. Changeset is minor (a new member on
a published interface is a surface change, not a patch), and it declares no breaking
change, so no ADR-0087 disposition marker is owed — check-adr-0087-registration
passes.

Ablation — three mutations, all re-measured on the pushed head feb213d42

Population: concurrent-replica-resume-race.test.ts, suspended-run-store.test.ts
and multi-replica-resume-staleness.test.ts61 tests, green baseline
Tests 61 passed (61).

No build or dist step is owed or was run, and that is a property of the imports
rather than a convenience: both test files import ./engine.js and
./suspended-run-store.js by RELATIVE path inside their own package, so vitest
resolves the TYPESCRIPT SOURCE, not the package exports to dist; the package's
vitest.config.ts aliases only @objectstack/platform-objects. Each mutation
going red is itself the proof the edit reached the runtime.

Every mutation was confirmed ON DISK before a single result was read — anchored
counts AND the blob hash, never an editor's exit code — and every restore was
proven by OBSERVATION (git diff HEAD empty AND the blob hash equal to HEAD's),
run inside trap ... EXIT INT TERM with absolute paths. The injected markers are
real statements, not // comments.

(E) the engine stops asking. Replace the claimAdvance call in
resumeInternal with the unconditional await this.forgetSuspendedRun(run, 'resumed') it had before this card.

engine.ts  HEAD_BLOB=cc6fbf5a8b3649454cce9acd6274dd1e040629ed
ON-DISK: deleted-claimAdvance-call=0  injected-marker=1  restored-unconditional-consume=1
         MUT_BLOB=514f7d018b11750cb7215b9a6d7ad2b5c52977be   -- MUTATION CONFIRMED ON DISK --
Tests  9 failed | 52 passed (61)
RESTORE: POST_BLOB=cc6fbf5a8b3649454cce9acd6274dd1e040629ed (== HEAD)  git diff HEAD -> empty

Seven in the race file (SHAPE A, SHAPE B, SIZED, both CONDITION cases, the loser's
debug trace and the declared degradation) and two in suspended-run-store.test.ts
(the two-engines race over the durable store, and the throwing claim).
multi-replica-resume-staleness.test.ts stays 8/8 — the mutation is targeted,
and #13617's own ledger is untouched.

(C) the condition stops being a condition — the isolated review's own ablation,
and BLOCKING finding 1. Delete BOTH comparisons from
InMemorySuspendedRunStore.claimSuspension, leaving an existence-only consume.

suspended-run-store.ts  HEAD_BLOB=0ad54b2841b39f1bda50ab3d96c427c9430a2b91
ON-DISK: deleted-node-cmp=0  deleted-corr-cmp=0  injected-marker=1
         MUT_BLOB=1bd1f8c06fffecf9e75b0b03d92f9e951f077b2d   -- MUTATION CONFIRMED ON DISK --
Tests  2 failed | 59 passed (61)
  x THE CONDITION (node): a claim landing after the winner RE-PARKED is lost, not granted
      concurrent-replica-resume-race.test.ts:457  expect(bResult.success).toBe(false)
      AssertionError: expected true to be false
  x THE CONDITION (correlation): a re-entry at the SAME node with a new correlation is lost
      concurrent-replica-resume-race.test.ts:544  expect(bResult.success).toBe(false)
      AssertionError: expected true to be false
RESTORE: POST_BLOB=0ad54b2841b39f1bda50ab3d96c427c9430a2b91 (== HEAD)  git diff HEAD -> empty

Exactly the two CONDITION cases and nothing else, which is the point of them. Every
other race in the file lets the loser lose by finding NO row at all — a condition an
existence check satisfies too — so before those two existed this mutation was
measured GREEN across the whole branch.

(C2) the production store loses its predicate — the review's second ablation,
and BLOCKING finding 2. Delete multi: true from the one delete call in
ObjectStoreSuspendedRunStore.claimSuspension.

suspended-run-store.ts  HEAD_BLOB=0ad54b2841b39f1bda50ab3d96c427c9430a2b91
ON-DISK: deleted-multi-call=0  injected-marker=1  mutated-call=1
         MUT_BLOB=11f686cdc070be12601d12e2965036454482c4ce   -- MUTATION CONFIRMED ON DISK --
Tests  7 failed | 54 passed (61)
  x spells the compare-and-set the way ObjectQL dispatches to deleteMany, not the by-id route
  x omits `correlation` from the condition when the caller has none, and still takes the multi route
  x maps the affected-row COUNT to the outcome: 1 is claimed, 0 is lost
  x a row that MOVED to another node is lost, not claimed -- the row still exists
  x a row re-parked at the SAME node with a new correlation is lost (the map re-entry shape)
  x two engines over ONE durable store advance a raced run exactly once
  x drives a suspend -> restart -> resume through the DB-backed store
RESTORE: POST_BLOB=0ad54b2841b39f1bda50ab3d96c427c9430a2b91 (== HEAD)  git diff HEAD -> empty

All seven in suspended-run-store.test.ts, every one failing with the PRODUCER's
own refusal rather than a token match: "Delete names one row by primary key, but
options.where also carries predicate keys 'node_id', 'correlation' ... For a
conditional (compare-and-set) write, declare the predicate path, which honours
EVERY where key". Against a running server that spelling throws, and claimAdvance
turns it into STORE_UNAVAILABLE on EVERY resume; before that suite existed, this
one-token regression was measured GREEN across the whole branch.

The three figures above are byte-identical to the REVERT-PROOF ledger recorded in
the pin file's docblock, re-measured independently on this head rather than read
back from it.

Verification, on the FINAL head feb213d42

All heavy runs through scripts/pm/os-verify-lock.sh; exit codes captured before
any pipe; verdicts quoted from each gate's own line.

  • Package suitepnpm --filter @objectstack/service-automation test:
    Test Files 101 passed (101) · Tests 1202 passed (1202), SUITE_EXIT=0,
    os-verify-lock: VERDICT command-exit 0.
  • The pin file, the store suite and the automation/approvals: 多副本集群下审批流每级节点(除首级)被重复创建 —— approve 后恢复读到滞后一拍的流运行态,同级要批两次(单副本零重复) #13617 harness — the ablation population: Tests 61 passed (61).
  • Package typecheck — measured properly, and the plain reading would have said
    nothing.
    This package declares NO typecheck script, so a
    pnpm --filter ... typecheck would match zero scripts and exit 0 having measured
    NOTHING. The real reading is tsc --noEmit -p on the package tsconfig, whose
    include: [ "src" ] DOES cover **/*.test.ts — verified with --listFiles: the
    new pin file is in the program, 101 test files in total, along with
    suspended-run-store.test.ts, engine.ts, suspended-run-store.ts and
    index.ts — re-verified on this head, all five edited files present, so this
    reading really does cover the new tests. Result on the final head: TSC_EXIT=2,
    TSC_ERROR_COUNT=3, all three TS2341 Property 'flows' is private in
    src/nested-region-parity.test.ts at 95/151/180 — byte-for-byte the ledgered
    TEST_DEBT entry
    for this package in scripts/check-type-check-coverage.mjs
    (errors: 3, note itemising those exact lines). Zero errors in any file this PR
    touches, and the same three measured on the merge base with these sources
    reverted, so this PR adds none. pnpm check:type-check-coverage passes green.
  • Gate family, re-derived on the final head from the actual change set
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
    (the script derives the change set itself from the merge base; no hand-built
    path list is fed to it). It reports 10 paths and 67 commands — 63 named by
    PATH plus 7 named by change KIND, 3 of them reached both ways. All 67 were run,
    plus check:nul-bytes. 64 green. Three are NOT MEASURED, each by its own
    prerequisite verdict line and none of them a red:
    check-test-completeness (exit 3, "grades a saved turbo run test log, and no
    log was named"), check:dual-build-cjs-loads (exit 3, "PREREQUISITE NOT MET —
    this gate reads built output, and some package has no dist/"), and
    check:type-check-debt (exit 3, "PREREQUISITE NOT MET"). A fourth,
    check:skill-examples, exits 1 on the same class of prerequisite
    ("packages/client-react/dist holds no .d.ts declarations — the package is not
    built"), which is an unbuilt-workspace fact and not a finding about this diff:
    its scan surface is code blocks, and this PR's docs diff changes only digits in
    prose and tables. CI builds the closure and runs all four for real. Exit codes
    were captured BEFORE any pipe (cmd > log 2>&1; EXIT=$?), and each verdict above
    is quoted from the gate's own line rather than read off a bare $?.
  • Trial mergegit merge-tree --write-tree --name-only origin/main HEAD
    against origin/main at 4d0d9445a: exit 0, one line of output (the tree oid),
    no conflict paths. The branch already carries a merge of that commit.

The census re-certification is part of this diff, and its delta is attributed

ObjectStoreSuspendedRunStore.claimSuspension adds exactly ONE write call site on
the application surface, so the shrink-only tenant-audit census moves 217 to 218.
Regenerated with node scripts/tenant-audit-census.mjs --write, and the nine
hand-written prose figures the generator does not own updated by hand.

That the delta is mine and only mine was MEASURED, not assumed: with these three
sources reverted to the merge base and the census restored to HEAD, the gate is
green at 217 (✓ check-tenant-audit-census: OK -- 217 write call sites certified).
On this branch it is green at 218 (✓ check-tenant-audit-census: OK -- 218 write call sites certified (146 decidable; 9 tenancy-enabled sites PROVABLY carry no tenant context, 32 more unreadable), 23 prose figures held to the census).

Out of scope

None found.

Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8

🤖 Generated with Claude Code

https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8


Generated by Claude Code


Generated by Claude Code

…re (#14333)

Two concurrent resumes of one run on two replicas could both advance it: the
idempotency guard was `this.resuming`, an in-process Set, so each replica
passed its own check, both read the same fresh row from the shared store, both
consumed it and both traversed forward — every downstream side effect twice.

Measured first, on the two-engines-over-one-shared-store harness: 25/25 raced
runs advanced twice, for both shapes the report named (parallel / any-of
approvers and duplicated automated approve calls); a single approver per level
deciding sequentially does not race and is pinned as the negative control.

`SuspendedRunStore` gains an optional `claimSuspension(runId, parkedAt)` — the
compare-and-set put where the shared state lives: consume the row only if it is
still parked at the node the caller read. The winner advances, the loser is
refused RESUME_IN_PROGRESS and runs nothing, and `this.resuming` stays the
cheap first gate. Both shipped stores implement it; `ObjectStoreSuspendedRunStore`
uses the data engine's documented compare-and-set route with the columns
`sys_automation_run` already carries, so no platform-object schema changes.
A store that cannot express the condition is announced once at `warn`, never
silently offered no guarantee.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
The REVERT-PROOF block carried a predicted 5 red / 6 green. Measured on the
committed tree, with the mutation confirmed on disk by anchored counts and the
blob hash, it is 4 red / 4 green — with the failing values named case by case,
so the ledger describes the file that exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…ll sites (#14333)

`ObjectStoreSuspendedRunStore.claimSuspension` adds exactly one write call
site on the application surface — the conditional `delete` that decides the
cross-replica advance — so the shrink-only census the gate holds the tree to
moves 217 -> 218. Regenerated with `node scripts/tenant-audit-census.mjs
--write`, and the nine hand-written prose figures the generator does not own
updated by hand to match.

Measured pre-existing state, so the delta is attributable: with these three
sources reverted to the merge base and the census restored to HEAD, the gate
is green at 217 — this PR moves it by exactly one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 18 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/services/service-automation/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via nodeId (symbol, a field of interface SuspensionParkedAt))
  • content/docs/automation/flows.mdx (via nodeId (symbol, a field of interface SuspensionParkedAt))
  • content/docs/kernel/cluster.mdx (via nodeId (symbol, a field of interface SuspensionParkedAt))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via AutomationEngine (symbol, a top-level class))
  • content/docs/releases/v17.mdx (via AutomationEngine (symbol, a top-level class), nodeId (symbol, a field of interface SuspensionParkedAt))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/services/service-automation/src/index.ts) — pages documenting those are invisible to this run
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 5 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 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6packageMentionDocs.

Which tree this was computed on

This run read content/docs from 3d41ae325c5454ee3d5ce05ba52d5eb52bb62723 — the merge of head feb213d42be0172ccb30b4d63bb5a2c817502de3 into base 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 3d41ae325c5454ee3d5ce05ba52d5eb52bb62723 && git checkout 3d41ae325c5454ee3d5ce05ba52d5eb52bb62723
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6 feb213d42be0172ccb30b4d63bb5a2c817502de3 && git checkout -B drift-repro 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6 && git merge --no-ff feb213d42be0172ccb30b4d63bb5a2c817502de3

node scripts/docs-audit/affected-docs.mjs --json 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 4d0d9445a8ed0240e7ca6a393bbe7f4c637e6bd6 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

os-sales commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

In-seat Clause-② contract review — VERDICT: FAIL. Patch round 1 opens.

Reviewer: isolated subagent, fed only the card, the ruling and this PR, with no access to the author's reasoning. Model served, verified from the transcript rather than from self-report — grep -o '"model":"[^"]*"' over the run: 62 of 62 claude-fable-5-1, no other model present. The verdict is therefore adoptable and is adopted verbatim below. ⛔ Nothing in it is softened, re-ordered or paraphrased by this seat.


VERDICT: FAIL

① Held. The diff is seven paths (git diff --stat origin/main...origin/claude/issue-14333-concurrent-resume-store-guard): the changeset, engine.ts, index.ts, suspended-run-store.ts, the new pin file, and two census pages. Nothing under packages/spec/**, content/docs/releases/**, or sys-automation-run.object.ts. The ⛔ holds in substance, not just by path: the condition is spelled against node_id and correlation, both already declared on sys_automation_run (sys-automation-run.object.ts:116, :146) and already written by serialize() (suspended-run-store.ts:591, :596); the version column is untaken. Triage's spec conditional does not fire: git grep SuspendedRunStore -- packages/spec returns one docblock reference (object.zod.ts:624) and a CHANGELOG line, no declaration. The measurement was taken and I reproduced it rather than reading it: in a scratch bench at the PR head with engine.ts and suspended-run-store.ts replaced by their origin/main blobs (claimSuspension count 0/0 on disk), the pin file goes 4 red / 4 green with expected [ 'notify', 'notify' ] to deeply equal [ 'notify' ] (SHAPE A, SHAPE B), { trials: 25, doubled: 25, ... } (SIZED), and expected [] to have a length of 1 (the degradation case); the sequential single-approver control is green before and after. Both ruled shapes (any-of approvers, duplicated automated approve) are present. The instrument is a sibling of multi-replica-resume-staleness.test.ts using the same harness shape (two AutomationEngines over one InMemorySuspendedRunStore); the named file's own revert-proof ledger (its lines 40-44) is untouched and stays 8/8 under the author's ablation in my bench. The two census pages are gate-owned: node scripts/check-tenant-audit-census.mjs is green at 218 on the PR head. Measurement and remedy landed in one commit (d9282c376), which is a sequencing of commits, not of reasoning; the values stand.

② Derived surface: git diff -U0 ... | grep -E '^\+\s*export ' yields exactly two symbols, export interface SuspensionParkedAt and export type SuspensionClaimOutcome (both engine.ts); both are added to the barrel's export type { ... } from './engine.js'. Not visible to that grep: a new optional member claimSuspension?(runId, parkedAt: SuspensionParkedAt): Promise<SuspensionClaimOutcome> on the already-exported SuspendedRunStore, and a new public method claimSuspension on each of the two barrel-exported classes InMemorySuspendedRunStore and ObjectStoreSuspendedRunStore. Module-private and correctly so: the AdvanceClaim fold and forgetSuspendedRun's new third parameter. Judgment: the two types are the parameter and return of a member a host must implement, so they are genuinely public and the barrel is the right module (same barrel already does this for SuspensionRestoreResult / SuspensionRestoreRefusal, #13909); they are not in-package-only exports. Changeset @objectstack/service-automation: minor is the right level: a new member on a published interface plus two new exported types, additive, no compile break. ADR-0087: the gate keys off a declared-breaking changeset (check-adr-0087-registration.mjs header, lines 51-66); this changeset declares none, so no marker is owed. The optional shape is defensible and is what the ruling's "must say so loudly rather than silently offering none" asks for: absence is announced once at warn, a required member would compile-break third-party stores for a guarantee they may be unable to express. The three-valued vocabulary is public vocabulary, not an escaped detail: 'unsupported' is a fact distinct from 'lost' (a false would stall every resume) and from a throw (which the engine maps to the transient STORE_UNAVAILABLE), and ObjectStoreSuspendedRunStore has two genuine runtime cases (no engine.delete, a non-count result) that are only knowable at call time, so omitting the member cannot express them. The engine keeps its own four-way fold private. Log levels from the diff: engine warn (once per instance) and debug (loser), store warn through the existing MinimalLogger warn? sink; no error site added, no published sink shape raised. The #13592 constraint holds.

③ The condition is the right one: the compare-and-set is against (id, node_id[, correlation]) taken from the store row the caller loaded (loadSuspendedRunStrict reads the store first, engine.ts 4825-4841), so the claim asks whether the shared row still says what this replica read. The production route is real: { where: { id, node_id[, correlation] }, multi: true } resolves to { kind: 'multi' } in resolveEngineDeleteDispatch (scalar id, unhonoured keys, multi set; pinned as the #11009 case in ENGINE_DELETE_DISPATCH_CASES), ObjectQL.delete then calls driver.deleteMany (engine.ts:12941) whose contract is Promise<number> (spec/src/contracts/data-driver.ts:233); the 'objectql' service the plugin takes is the raw ObjectQL instance (objectql/src/plugin.ts:400), and all five shipped drivers have deleteMany (sqlite-wasm inherits it from SqlDriver). In-memory: get, compare, delete with no await between, atomic on one thread. The window cannot be reopened by re-parking: detectCycles throws, so a run is a DAG and after A consumes node N the row can only be re-parked at a different node or, for map re-entry, at the same node with a different map:<childRunId> correlation. The unguarded cases are genuinely unguardable: no store means nothing is shared; a cache-only run has no row anywhere another replica can read, and resuming already covers the only process that can resume it. The loser path goes through finally and clears resuming. Where it does not hold up is discrimination. Two ablations the author did not run, in my bench on the committed tree, with restores confirmed by git status: (C) removing BOTH the node and the correlation comparisons from InMemorySuspendedRunStore.claimSuspension, leaving an existence-only delete, keeps the pin file, the staleness harness and suspended-run-store.test.ts at 49/49 green; nothing distinguishes "delete only if still parked at node N", the remedy the ruling named, from "delete if the row exists", because no test lets B's claim land after A has re-parked. (C2) removing multi: true from the ObjectStore spelling also keeps 49/49 green: the only double that reaches ObjectStoreSuspendedRunStore (the fake engine in suspended-run-store.test.ts) deletes by id and returns true, which the store reads as 'unsupported' and the engine as unguarded; against the real ObjectQL that spelling is the reject verdict, which throws, which claimAdvance turns into STORE_UNAVAILABLE on every resume. A one-token regression that would refuse every production resume is green. The author's ablation (engine call site) reproduces 4 red / 12 green and discriminates only "the engine calls the store".

TESTS: The pin file pins, on observable effects, that two engines over one InMemorySuspendedRunStore fire notify once and open lv2 once for any-of approvers and for a duplicated automated approve; that exactly one caller wins with status: 'paused' and the other gets RESUME_IN_PROGRESS with no status; that both replicas still see the run parked afterwards; 25/25 sized trials; the sequential single-approver control; the single-replica concurrent control (this.resuming, error text "already being resumed") and sequential-to-completion control; that no store yields no advance guarantee line; and that a store lacking the member gets exactly one line, starting warn , naming claimSuspension, and still resumes. It does not pin: the node or correlation condition (C); ObjectStoreSuspendedRunStore.claimSuspension at all, neither its multi: true predicate spelling nor its count-to-claimed/lost mapping (C2); 'lost' from a stale node or correlation on any store; either 'unsupported' branch; a throwing claimSuspension mapping to STORE_UNAVAILABLE; the debug line on the loser; once-per-engine for the 'unsupported' case (only the missing-member case is pinned). Beyond the three files I ran (49 tests) the package suite, the gate family and the trial merge are NOT MEASURED here.

BLOCKING:

  1. The compare-and-set condition is unpinned: with the nodeId and correlation comparisons deleted from InMemorySuspendedRunStore.claimSuspension (existence-only consume), every test on the branch stays green (49/49 measured). The ruling's remedy is "delete only if still parked at node N"; no test can tell that from "delete if present", so the window the condition closes (a loser whose claim lands after the winner has re-parked) has no red.
  2. The production implementation ObjectStoreSuspendedRunStore.claimSuspension has no test: with multi: true removed from its delete call, every test stays green (49/49 measured), while against the real ObjectQL that spelling throws the unhonoured-predicate reject and claimAdvance maps it to STORE_UNAVAILABLE on every resume. Nothing drives two engines over one ObjectStoreSuspendedRunStore, nothing pins the predicate spelling or the count-to-outcome mapping, nothing pins either 'unsupported' branch; the counting multi fake engine in suspended-run-store.test.ts already exists as the harness.

§5 NOTES:

  1. On 'lost' the loser's hot-cache entry in this.suspendedRuns is not evicted; resume stays correct because loadSuspendedRunStrict reads the store first, but any direct reader of the map sees a parking another replica already consumed (the service-automation: three more readers of suspended-run state still prefer the per-process map over the shared store #14332 family).
  2. The STORE_UNAVAILABLE message on a thrown claimSuspension asserts "the suspension was NOT consumed"; a throw after a committed DELETE (transport drop post-commit) makes that false. Same ambiguity as any store write, but the text claims more than is known.
  3. A driver without deleteMany is not an 'unsupported' answer: ObjectQL.delete throws ENGINE_DELETE_REJECT_MESSAGE on the predicate path, so such a composition gets STORE_UNAVAILABLE on every resume. No shipped driver lacks it (memory, sql, mongodb, turso implement it; sqlite-wasm inherits from SqlDriver), so this is third-party exposure the docblock's "two ways this store cannot express the guarantee" does not name.
  4. The store-level 'unsupported' warn fires per call; the engine dedups its own line but not the store's, so a permanently non-counting engine logs one store line per resume. Consistent with the existing delete() no-delete warn, but weaker than the once-per-engine the PR body promises.
  5. On the non-count 'unsupported' branch the DELETE has already executed before 'unsupported' is reported; the engine then treats the advance as unguarded and re-deletes in forgetSuspendedRun. Harmless and pre-PR behaviour on that path, not a regression.
  6. tsc in my bench: zero errors in engine.ts, suspended-run-store.ts, index.ts and the new test; the remaining errors (3 ledgered in nested-region-parity.test.ts, 11 across five integration tests) are identical with the touched sources at merge base, so not attributable to this PR. The bench borrowed a sibling tree's built spec, so a same-commit type check is NOT MEASURED.

The seat's dispositions

The two blocking findings are the patch round, and neither is a preference. Both are the same defect in two places: the guard is real but the tests cannot tell it from a guard that isn't there. A one-token regression that would refuse every production resume passing 49/49 is not a coverage gap to note, it is the suite failing at its job. Patch round 1 must make the reviewer's own ablations (C) and (C2) go red. ⛔ Nothing else about the remedy is reopened — ①, ② and the substance of ③ are adopted as passing, and the design is not to be revisited.

§5, disposed one by one rather than deferred as a block. Notes 2, 3 and 4 are about text this PR itself introduces — an error message that claims more than is known, a docblock that does not name a third exposure, and a PR-body promise (once per engine) the store-level line does not keep. Correcting your own new text is not widening a PR, so they ride patch round 1. Notes 1 and 5 describe behaviour that predates this PR on paths it does not change; they become a grouped finding after landing, ⛔ not one card each and ⛔ not a rider here. Note 6 is a boundary declaration, not a finding: no action.

needs:contract-review stays hung on this PR and on #14333. This PR stays draft. Neither carrier is cleared until a delta review — run by resuming the same reviewer, on the patch head — comes back PASS.


Generated by Claude Code

os-sales commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

CI red on 97ac4afcf — the run-dev-unbuilt-workspace flake again, not this PR's

Test Core (1/6) failed on this PR's own branch CI. Read from the job log rather than from the check summary:

FAIL  integration  test/run-dev-unbuilt-workspace.e2e.test.ts
  > the mirror direction: a reader that is never coming back
  > gives up and exits instead of waiting forever
AssertionError: expected 'SIGKILL' to be null
 ❯ test/run-dev-unbuilt-workspace.e2e.test.ts:317:27
Test Files  1 failed | 230 passed (231)
      Tests  1 failed | 2659 passed (2660)

Not this PR's, on the same three readings that disposed of it on #14528 and #14687: the failing test is in @objectstack/cli while this diff is packages/services/service-automation plus two gate-owned census pages; the same test file is the tracked flake anchored at #14648 (priority:p1, domain:cli, dispatched, dev on claude/issue-14648-run-dev-unbuilt-workspace-flake since 18:15Z); and the asserted value is the harness's own kill signal — its setTimeout(… 'SIGKILL', UNREAD_HARD_CAP_MS) firing — which is a stopwatch, not a product behaviour.

A reading worth carrying to #14648: this ejection was branch CI, not a merge-queue batch, so the flake is not confined to queue shards. The shard's own numbers support the contention reading — Duration 989.98s (import 394.67s, tests 2529.18s) for 2660 tests against a cap derived from a measured ~22 s uncontended run.

No re-run is being spent and nothing is being pushed for it. Patch round 1 is already in flight on this branch (the contract review returned FAIL; the verdict and the seat's dispositions are at 14712#issuecomment-5515933381), so the head will move and CI re-runs on its own. Spending a manual re-run on a head that is about to be replaced would buy nothing.

@objectstack/service-automation's own suite was green on this head: Test Files 101 passed (101) · Tests 1190 passed (1190).


Generated by Claude Code

…roduction store (#14333)

Patch round 1 for the isolated contract review's two BLOCKING findings. Both
were the same defect in two places: the guard was real but no test could tell
it from a guard that is not there.

BLOCKING 1 — the condition was unpinned. Every earlier race let the loser lose
by finding NO row, which an existence-only consume satisfies too. Two new tests
hold the loser's claim until after the winner has advanced and RE-PARKED, one
per comparison: a re-park at a different node, and a `map` re-entry at the same
node with a new correlation. Both assert the parking the loser read (so the
precondition is measured, not assumed) and that the winner's live suspension
SURVIVES — the half a doubled effect alone would not catch, since an
existence-only consume strands the run by deleting the parking another replica
is standing on.

BLOCKING 2 — `ObjectStoreSuspendedRunStore.claimSuspension` had no test at all.
`createFakeEngine` now dispatches through the producer's own predicate
(`assertEngineDeleteDispatch`), so the double cannot accept a call
`ObjectQL.delete` refuses, and a new suite pins the predicate spelling (read
back through that same predicate, never by matching the literal token), the
count-to-outcome mapping, `'lost'` from a moved node and from a moved
correlation, both `'unsupported'` branches with their once-per-store line, two
engines racing over ONE durable store, and a throwing claim mapping to
STORE_UNAVAILABLE. Also pins the loser's `debug` line.

§5 notes 2/3/4, all of them this PR's own new text:
- the STORE_UNAVAILABLE message no longer asserts "the suspension was NOT
  consumed" — a throw can arrive after a committed delete, so it now states
  only that this resume did not continue the run and hands the ambiguity to a
  retry;
- the store docblock names the THIRD exposure: a driver with no `deleteMany`
  makes ObjectQL.delete throw on the predicate path, so such a composition gets
  STORE_UNAVAILABLE on every resume rather than `'unsupported'`;
- the store-level `'unsupported'` warn is deduped to once per store instance,
  keeping the once-per-engine promise the PR body makes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…14333)

The pin file's REVERT-PROOF block described a two-file, eight-test population
that no longer exists. Re-measured on the committed tree, mutation proven on
disk and restore proven byte-identical for each:

  (E)  engine stops asking            9 failed | 52 passed (61)
  (C)  condition stops being one      2 failed | 59 passed (61)
  (C2) store loses its predicate      7 failed | 54 passed (61)

(C) and (C2) are the review's own ablations, both measured GREEN across the
whole branch before this round.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
…T row (#14333)

`createFakeEngine` in `suspended-run-store.test.ts` now dispatches through
`assertEngineDeleteDispatch`, which is exactly what that file's DEBT entry named
as its close condition ("replace the mirrored `if` with
assertEngineDeleteDispatch(options) — the devDependency is already declared").

Both halves, as `check:engine-double-contract` prescribes: the pinned ledger
learns about the new coverage (`--write`, one row added, none lost) and the
closed DEBT row is deleted in the PR that fixed it. The ratchet moves in the
shrinking direction — 754 pinned / 134 DEBT becomes 755 pinned / 133 DEBT.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8
@github-actions github-actions Bot added size/xl and removed size/l labels Sep 2, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review September 2, 2026 23:50
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit 4368411 Sep 3, 2026
42 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-14333-concurrent-resume-store-guard branch September 3, 2026 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

service-automation: two concurrent resumes of one run on two replicas can both advance it — the idempotency guard is per-process

3 participants