fix(service-automation): evict a suspension consumed by another replica, so the run listings stop reporting phantoms - #16031
Conversation
A run parked by one process and consumed by another left the parking process's `suspendedRuns` map holding it for the life of the process. `forgetSuspendedRun` is the one eviction site and it runs in whichever replica CONSUMES the suspension, which in a multi-replica deployment is routinely not the one that parked it. The retained snapshot is not only memory: `listSuspendedRuns()` (cache-only, and the one listing on the AutomationService spec contract) and `listSuspendedRunsDurable()` (which deliberately appends map entries the durable list lacks) both hand it back, so a completed run is published as suspended and `getSuspendedScreen()` answers null for it. Evict on a store-authoritative per-id "no row" answer — the strict loader's store miss, the lost advance claim, and a bounded per-id reconcile for the map-only entries of the durable listing. Never on a store that threw, never for a cache-only run, never when no store is attached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift CheckThis PR changes 1 package(s): ⛔ 2 release-owned page(s) 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 227dfc4f70454bbe7770ae39c20c8cb2e67f8061 && git checkout 227dfc4f70454bbe7770ae39c20c8cb2e67f8061
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 784595113caeb45bf3e0776a7550c5dbb240a9c5 db6133abb522858e77d4e1bb83accd4b574f84e8 && git checkout -B drift-repro 784595113caeb45bf3e0776a7550c5dbb240a9c5 && git merge --no-ff db6133abb522858e77d4e1bb83accd4b574f84e8
node scripts/docs-audit/affected-docs.mjs --json 784595113caeb45bf3e0776a7550c5dbb240a9c5
|
Clause ② standing — no contract-review gate owed — PR #16031 at head
|
Part of #15832 — note 1 only. Note 2 is judged cross-lane and is deliberately left open here; see the lane judgement below.
Phase 1 first: the measurement that gated everything
Triage ruled that the escalation to p1 turns on whether any published composition's
engine.delete(object, { multi: true, … })resolves to something other than anumber— and that it cannot be settled by reading source, becauseObjectQL.deletedeclaresPromiseofany. Driven as real kernels over real drivers against a realsys_automation_runtable:claimSuspensionInMemoryDrivernumber/1number/0claimed/lostSqlDriverbetter-sqlite3number/1number/0claimed/lostSqliteWasmDrivernumber/1number/0claimed/lostTursoDriverlocalnumber/1number/0claimed/lostTursoDriverremote transport, real SQL vialibsql-sqlite-stub.testkitnumber/1number/0claimed/lostSqlDriver+SecurityPlugin(the one middleware that rewritesopCtx.result)number/1number/0claimed/lostNo measured composition resolves to a non-number, so the escalation condition did not fire. The control fires, though: with
InMemoryDriver.deleteManywrapped to perform the delete and then resolveundefined, the harness readstypeof undefined,claimSuspensionanswers'unsupported'for a genuine winner and for a replica that actually lost, and the store logs "resolved undefined, not an affected-row count". NOT MEASURED:driver-mongodb(the mongod binary download is refused by the egress proxy —curl: (56) CONNECT tunnel failed, response 403),driver-sqlon postgres/mysql dialects, and Turso against a real hosted endpoint.What this PR changes
AutomationEnginehad exactly one eviction site for itssuspendedRunsmap, insideforgetSuspendedRun— and that runs in whichever process consumes the suspension. In a multi-replica deployment that is routinely not the process that parked it, and there is no invalidation channel between them.The card located the leak on
resumeInternal'sclaim.kind === 'lost'branch. That branch does leak, but the no-race variant leaks identically — A parks, only B ever resumes, A never attempts a claim and there is no'lost'anywhere in the sequence — so an eviction hung on'lost'alone would have left the ordinary deployment untouched. This is built to that reading, not to the card body.The retained snapshot is not only memory.
listSuspendedRuns()(synchronous, cache-only, and the one listing on theAutomationServicespec contract) andlistSuspendedRunsDurable()(which deliberately appends map entries the durable list lacks) both hand it back, so once the other replica completes the run both publish a phantom whosegetSuspendedScreen()answersnull.An entry is now dropped whenever this process holds a store-authoritative, per-id "no row" answer for it: the strict loader's store miss, a lost advance claim, and a bounded per-id reconcile for the map-only entries of the durable listing.
The promise relied on — stated, because it is the whole question
The fix only ever removes entries. The spec says
listSuspendedRuns()lists "the currently suspended (paused) runs awaiting a resume"; the engine's own docblock adds only that it may OMIT runs (those parked in a previous process lifetime), because it reads the cache alone. Under-reporting is therefore already inside the declared latitude, and over-reporting was never inside the promise. Neither listing becomes store-backed and the synchronous one stays synchronous, so the cache-only contract is not moved or reinterpreted in either direction.Residual, pinned rather than described
Eviction is demand-driven: a phantom clears when this process next obtains the per-id answer for that run. A process that never looks at the run again keeps the entry. Closing that needs a background sweep or a store-backed listing, both decisions above this card — so a
RESIDUALtest pins the boundary instead of leaving it to be discovered.Note 2 — the lane judgement, and why it is (b)
Triage offered (a) an in-store one-shot capability probe (stays in
domain:services) or (b) declaring "multi delete returns the affected-row count" as a contract (cross-lane). This is (b). Reasoning, with what Phase 1 added:IDataDriver.deleteManydeclaresPromiseofnumberinpackages/spec/src/contracts/data-driver.ts:269, and its Zod mirrorpackages/spec/src/data/driver.zod.tspins.output(z.promise(z.number()))with "Count of deleted records". The gap is exactly one layer up:ObjectQL.deletedeclaresPromiseofanyand nothing states that themultiroute passes the driver's count through. So (b) propagates an existing contract rather than inventing one — and it lands inpackages/objectql, another lane.deleteddata event withmatched: 0onsys_automation_run— a fabricated write event to answer a typing question. Both keep the store guessing about a shape the layer beneath it already promises, which is the contract-first refusal: the producer's declaration is weaker than its behaviour, so the producer is where it is repaired.⛔ No file under
packages/objectqlorpackages/specis touched by this PR.Note 5 confirmed by run, not quoted
Same recorded shape as filed. Observed delete sequence on a composition whose multi-delete resolves to a non-count:
with the store warning "the data engine's multi-row delete resolved undefined, not an affected-row count" and the engine warning "no cross-replica advance guarantee … resume idempotency is IN-PROCESS ONLY".
Verification
Every command run bare, exit code captured before any pipe.
pnpm --filter @objectstack/service-automation test— exit 0,Test Files 116 passed (116) · Tests 1395 passed (1395).pnpm --filter @objectstack/service-automation typecheck— exit 0;check:test-typecheck: OK — the test layer compiles under tsconfig.test.json. Confirmed the new test file is genuinely in both programs rather than excluded:tsc --listFilescounts it once undertsconfig.jsonand once undertsconfig.test.json.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack— 55 families (43 by path + 7 by kind + 7 whole-tree). All 55 exit 0. Two needed the built workspace and were re-run afterturbo run buildrather than left unmeasured:check:dual-build-cjs-loadsfirst answeredPREREQUISITE NOT MET … exit 3(not a pass), thenexit 0— "103 published require entry point(s) across 66 package(s) load";check:type-check-debt—exit 0, "12 ledger entr(ies) re-measured … none above its recorded number".pnpm lint(whole repo,eslint . --no-inline-config) — exit 0. Run in full, so no narrowing is claimed.db6133ab.Mutation proof — one leg per fix site
Each: the implementation committed first, the mutation proven on disk by an anchored occurrence count plus a
git hash-objectchange, restore viagit checkout HEAD --undertrap … EXIT INT TERMwith an absolute path, and the restore proven by blob equality and an emptygit diff HEAD. Resolution is a same-package relative import, so nodistsits between the edit and the run — which the red readings themselves demonstrate, since no rebuild happened between mutation and result.evictConsumedSuspensioninloadSuspendedRunStrictevictConsumedSuspensionin the lost-claim branchEvery leg failed with
AssertionError: expected [ 'lv1' ] to deeply equal [], and every restore reportedmatch=YES diff-HEAD-empty=YES.Pin populations — what each actually covers
Nine tests over two engines and one shared store. Three cover the defect (no-race, race, list-then-open); one is the
RESIDUALboundary, which asserts the entry survives with no reconcile, so it is a statement of the limit and not of the fix. Five are controls that must stay green in both directions: no store attached (the map is the authority), a run whose durable save failed (cacheOnlySuspensions— the store's silence is not an answer), a store read that throws (unknown is not gone), a store whoselist()throws (a failed enumeration triggers no per-id reconcile), and a live run parked in this process surviving every reconcile. A passing control proves only the shape it drives; none of them speaks for the engine's other suspension paths, which the 1395-test package suite covers instead.packages/services/service-automation/src/engine.tsis edited here (115 lines, all additive but one). PR #15966 was reported as holding it at 0 files changed.Generated by Claude Code