Uh oh!
There was an error while loading. Please reload this page.
fix(service-automation): a conditional advance claim on SuspendedRunStore, so two replicas cannot both advance one run - #14712
Conversation
…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
📓 Docs Drift CheckThis PR changes 1 package(s): 3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # 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
|
os-sales
commented
Sep 2, 2026
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 — VERDICT: FAIL ① Held. The diff is seven paths ( ② Derived surface: ③ The condition is the right one: the compare-and-set is against TESTS: The pin file pins, on observable effects, that two engines over one BLOCKING:
§5 NOTES:
The seat's dispositionsThe 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 (
Generated by Claude Code |
…ncurrent-resume-store-guard
os-sales
commented
Sep 2, 2026
CI red on |
…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
…ncurrent-resume-store-guard
…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
…ncurrent-resume-store-guard
Uh oh!
There was an error while loading. Please reload this page.
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, anin-process
Set— a complete guard for exactly one deployment shape, a singleprocess. #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:
① 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 aREVERT-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 —
starttolv1(pauses) tonotify(fires) tolv2(pauses) toend— so one advance pastlv1firesnotifyonce and openslv2once. The defect's signature isnotifyin the fired ledger twice and
lv2opened twice: an action fired twice and a nodeexecuted twice, which is the duplicated approval request the family was reported as.
fired=[ 'notify', 'notify' ]where[ 'notify' ]is correct;opened=[ 'lv1', 'lv2', 'lv2' ][ 'notify', 'notify' ]{ trials: 25, doubled: 25, extraOpens: 25 }— every raced run advanced twice and re-opened its next levelresumingset already refuses one,RESUME_IN_PROGRESSBoth 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
SuspendedRunStoreSuspendedRunStoregainsclaimSuspension(runId, parkedAt): consume the durablerecord 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'.this.resumingstays as the cheap first gate; it is not replaced, and itstill refuses a same-process duplicate with no store round-trip.
RESUME_IN_PROGRESS, deliberately: theobservation differs (another process, not this one) but the remedy is identical,
plugin-approvalsalready branches on it that way inresumeRecordedOutcome("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.
node_idandcorrelationare columnssys_automation_runhas always carried, so the condition is expressible againstthe 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
falsefor it would stall everyresume, one answering
truewould offer no guarantee while claiming one.Both shipped implementations owe it, and both pay:
InMemorySuspendedRunStore— tests and removes with noawaitbetween 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— oneDELETE ... WHERE id = ? AND node_id = ?(plus
correlationwhen present) through the data engine's own documentedcompare-and-set route:
multi: truewith a fullwhere, whichengine-delete-dispatch.tsstates dispatches todriver.deleteManywith thecomposed AST, whose result is contracted to be an affected COUNT. A pure-
idwherewould take the by-id route and silently discard the condition — the oneshape that must never happen here.
Nothing silently offers no guarantee. Four cases never reach the store and each
is a deliberate, documented
unguarded:resumingalready IS the whole guaranteecacheOnlySuspensions, #13617)lostfor a run that is legitimately resumable in this processpersistSuspendedRun's documented degradation into an unresumable runclaimSuspension(a third-party store predating it)'unsupported'(no enginedelete, or a non-count result)Log levels, chosen deliberately.
warn, and it is said ONCE. The engine'sown line (a store with no
claimSuspension) is once per engine instance; thestore's line (a composition that answers
'unsupported') is emitted byObjectStoreSuspendedRunStoreand deduped once per STORE instance, which is thenarrower 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 thesame reason. ⛔ Deliberately not
error: no newerror-level site through apublished sink shape.
debug. It is an ORDINARY outcome — the guarddoing its job — and the caller is told in the result. At
warna busy any-oflevel would emit a steady stream of records describing correct behaviour.
claimSuspensionthat THROWS is handed to the caller asSTORE_UNAVAILABLEwith 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
SuspendedRunStoreis declared inpackages/spec, theinterface half goes to the spec seat — does not fire, re-measured in this
worktree rather than trusted:
One hit, a docblock reference, not a declaration. The interface is declared at
packages/services/service-automation/src/engine.tsand nowhere else. No spec-seathandoff;
packages/specis untouched by this PR.Clause-② — YES, declared from the actual diff
The exported
SuspendedRunStoreinterface gains a member, and two new exportedtypes (
SuspensionParkedAt,SuspensionClaimOutcome) reach the barrel. That is apublic-surface widening.
needs:contract-reviewis 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 ona 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-registrationpasses.
Ablation — three mutations, all re-measured on the pushed head
feb213d42Population:
concurrent-replica-resume-race.test.ts,suspended-run-store.test.tsand
multi-replica-resume-staleness.test.ts— 61 tests, green baselineTests 61 passed (61).No build or
diststep is owed or was run, and that is a property of the importsrather than a convenience: both test files import
./engine.jsand./suspended-run-store.jsby RELATIVE path inside their own package, so vitestresolves the TYPESCRIPT SOURCE, not the package
exportstodist; the package'svitest.config.tsaliases only@objectstack/platform-objects. Each mutationgoing 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 HEADempty AND the blob hash equal to HEAD's),run inside
trap ... EXIT INT TERMwith absolute paths. The injected markers arereal statements, not
//comments.(E) the engine stops asking. Replace the
claimAdvancecall inresumeInternalwith the unconditionalawait this.forgetSuspendedRun(run, 'resumed')it had before this card.Seven in the race file (SHAPE A, SHAPE B, SIZED, both CONDITION cases, the loser's
debugtrace and the declared degradation) and two insuspended-run-store.test.ts(the two-engines race over the durable store, and the throwing claim).
multi-replica-resume-staleness.test.tsstays 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.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: truefrom the onedeletecall inObjectStoreSuspendedRunStore.claimSuspension.All seven in
suspended-run-store.test.ts, every one failing with the PRODUCER'sown 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
claimAdvanceturns it into
STORE_UNAVAILABLEon EVERY resume; before that suite existed, thisone-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
feb213d42All heavy runs through
scripts/pm/os-verify-lock.sh; exit codes captured beforeany pipe; verdicts quoted from each gate's own line.
pnpm --filter @objectstack/service-automation test:Test Files 101 passed (101) · Tests 1202 passed (1202),SUITE_EXIT=0,os-verify-lock: VERDICT command-exit 0.Tests 61 passed (61).nothing. This package declares NO
typecheckscript, so apnpm --filter ... typecheckwould match zero scripts and exit 0 having measuredNOTHING. The real reading is
tsc --noEmit -pon the package tsconfig, whoseinclude: [ "src" ]DOES cover**/*.test.ts— verified with--listFiles: thenew pin file is in the program, 101 test files in total, along with
suspended-run-store.test.ts,engine.ts,suspended-run-store.tsandindex.ts— re-verified on this head, all five edited files present, so thisreading really does cover the new tests. Result on the final head:
TSC_EXIT=2,TSC_ERROR_COUNT=3, all threeTS2341 Property 'flows' is privateinsrc/nested-region-parity.test.tsat 95/151/180 — byte-for-byte the ledgeredTEST_DEBTentry for this package inscripts/check-type-check-coverage.mjs(
errors: 3, note itemising those exact lines). Zero errors in any file this PRtouches, and the same three measured on the merge base with these sources
reverted, so this PR adds none.
pnpm check:type-check-coveragepasses green.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 ownprerequisite verdict line and none of them a red:
check-test-completeness(exit 3, "grades a savedturbo run testlog, and nolog 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/distholds no .d.ts declarations — the package is notbuilt"), 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 aboveis quoted from the gate's own line rather than read off a bare
$?.git merge-tree --write-tree --name-only origin/main HEADagainst
origin/mainat4d0d9445a: 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.claimSuspensionadds exactly ONE write call site onthe application surface, so the shrink-only tenant-audit census moves 217 to 218.
Regenerated with
node scripts/tenant-audit-census.mjs --write, and the ninehand-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