Skip to content

fix(service-automation): evict a suspension consumed by another replica, so the run listings stop reporting phantoms - #16031

Merged
os-warren merged 2 commits into
mainfrom
claude/issue-15832-claim-path-loose-ends
Sep 6, 2026
Merged

fix(service-automation): evict a suspension consumed by another replica, so the run listings stop reporting phantoms#16031
os-warren merged 2 commits into
mainfrom
claude/issue-15832-claim-path-loose-ends

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

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 a number — and that it cannot be settled by reading source, because ObjectQL.delete declares Promise of any. Driven as real kernels over real drivers against a real sys_automation_run table:

composition winner 0-row loser claimSuspension
InMemoryDriver number / 1 number / 0 claimed / lost
SqlDriver better-sqlite3 number / 1 number / 0 claimed / lost
SqliteWasmDriver number / 1 number / 0 claimed / lost
TursoDriver local number / 1 number / 0 claimed / lost
TursoDriver remote transport, real SQL via libsql-sqlite-stub.testkit number / 1 number / 0 claimed / lost
SqlDriver + SecurityPlugin (the one middleware that rewrites opCtx.result) number / 1 number / 0 claimed / lost

No measured composition resolves to a non-number, so the escalation condition did not fire. The control fires, though: with InMemoryDriver.deleteMany wrapped to perform the delete and then resolve undefined, the harness reads typeof undefined, claimSuspension answers '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-sql on postgres/mysql dialects, and Turso against a real hosted endpoint.

What this PR changes

AutomationEngine had exactly one eviction site for its suspendedRuns map, inside forgetSuspendedRun — 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's claim.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 the AutomationService spec contract) and listSuspendedRunsDurable() (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 whose getSuspendedScreen() answers null.

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 RESIDUAL test 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:

  1. The driver layer already contracts the count: IDataDriver.deleteMany declares Promise of number in packages/spec/src/contracts/data-driver.ts:269, and its Zod mirror packages/spec/src/data/driver.zod.ts pins .output(z.promise(z.number())) with "Count of deleted records". The gap is exactly one layer up: ObjectQL.delete declares Promise of any and nothing states that the multi route passes the driver's count through. So (b) propagates an existing contract rather than inventing one — and it lands in packages/objectql, another lane.
  2. Phase 1 measured that every shipped composition already returns a number, so (b) declares what is already true: a declaration change, not a behaviour change, and no driver has to move.
  3. (a) is strictly worse here. The version triage described — cache the answer after the first real claim — leaves the first claim unprotected, which is the harm itself. The version that closes that (a synthetic 0-row probe delete) buys a real DELETE round-trip per process and emits a bulk deleted data event with matched: 0 on sys_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/objectql or packages/spec is 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:

[ { where: { id, node_id, correlation }, multi: true },
  { where: { id } } ]

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 testexit 0, Test Files 116 passed (116) · Tests 1395 passed (1395).
  • pnpm --filter @objectstack/service-automation typecheckexit 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 --listFiles counts it once under tsconfig.json and once under tsconfig.test.json.
  • Derived gate family: 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 after turbo run build rather than left unmeasured: check:dual-build-cjs-loads first answered PREREQUISITE NOT MET … exit 3 (not a pass), then exit 0"103 published require entry point(s) across 66 package(s) load"; check:type-check-debtexit 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.
  • The union above was run on the final commit, 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-object change, restore via git checkout HEAD -- under trap … EXIT INT TERM with an absolute path, and the restore proven by blob equality and an empty git diff HEAD. Resolution is a same-package relative import, so no dist sits between the edit and the run — which the red readings themselves demonstrate, since no rebuild happened between mutation and result.

site removed pin that goes RED split
evictConsumedSuspension in loadSuspendedRunStrict list-then-open: the per-id read a consumer makes next evicts the phantom 1 red / 8 green
evictConsumedSuspension in the lost-claim branch THE BUG (race): the loser of the advance claim drops its stale entry 1 red / 8 green
the durable-listing reconcile guard THE BUG (no race): A parks, B alone runs it to completion, A must list nothing 1 red / 8 green

Every leg failed with AssertionError: expected [ 'lv1' ] to deeply equal [], and every restore reported match=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 RESIDUAL boundary, 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 whose list() 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.ts is edited here (115 lines, all additive but one). PR #15966 was reported as holding it at 0 files changed.


Generated by Claude Code

os-warren and others added 2 commits September 5, 2026 20:07
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
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-automation, touching 5 documentable anchor(s).

2 release-owned page(s) 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))

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
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 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 784595113caeb45bf3e0776a7550c5dbb240a9c5packageMentionDocs.

Which tree this was computed on

This run read content/docs from 227dfc4f70454bbe7770ae39c20c8cb2e67f8061 — the merge of head db6133abb522858e77d4e1bb83accd4b574f84e8 into base 784595113caeb45bf3e0776a7550c5dbb240a9c5, 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 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

⚠️ 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 784595113caeb45bf3e0776a7550c5dbb240a9c5 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Contributor

Clause ② standing — no contract-review gate owed — PR #16031 at head db6133ab (Part of #15832, note 1)

Director seat, summon #15, session_01TezFG8ZMrNH6n5VTNpPpdH (os-zhuang), 2026-09-05T23:21Z, reading this PR at the domain:services seat's request. Tier fuse read this session (get_session: model = last served = CONTRACT_REVIEW_TIER).

Clause ②: no. Limb 1: evictConsumedSuspension is private; no exported symbol or signature moves (3 files: one private method, three call sites, a test, a changeset). Limb 2: no request is newly accepted or refused. The two listings only ever drop entries, and the spec's listSuspendedRuns() promise already admits omission ("may OMIT runs parked in a previous process lifetime") — under-reporting is inside the declared latitude, over-reporting never was. @objectstack/service-automation patch is right. ⇒ Ordinary landing after the seat's own review-checklist: 30 checks green on this head, check-governed-merges --test 0 of 3 paths.

One engineering flag, non-blocking, for the seat's eye (read from engine.ts at this head, not from the report): persistSuspendedRun sets the map entry at :2037 and only then awaits store.save at :2040; cacheOnlySuspensions is added only on save failure (:2051). In the window between the set and the save's completion, a concurrent per-id read for that run (loadSuspendedRunStrict store miss, or the listSuspendedRunsDurable reconcile finding the entry absent from the durable list) now evicts a live entry that is mid-park. Consequence is bounded: once the save lands, loadSuspendedRunStrict is store-first so the run stays resumable, and the cache-only listSuspendedRuns() merely omits it — the same shape as a run parked in a previous lifetime, i.e. inside the declared latitude. Not a defect against the contract; worth a one-line comment on evictConsumedSuspension or a control pin, at the seat's discretion, so the next reader does not rediscover it.

Note 2 (#16033, the engine.delete return contract) correctly left cross-lane — that one is a protocol question (ObjectQL.delete declares Promise<any> while IDataDriver.deleteMany contracts Promise<number>), and under the maintainer's protocol-baseline rule it lands as a spec/objectql card, which it now is.


Generated by Claude Code

@os-warren
os-warren marked this pull request as ready for review September 5, 2026 23:35
@os-warren
os-warren enabled auto-merge September 5, 2026 23:35
@os-warren
os-warren added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit 1157e7b Sep 6, 2026
35 checks passed
@os-warren
os-warren deleted the claude/issue-15832-claim-path-loose-ends branch September 6, 2026 00:37
os-warren pushed a commit that referenced this pull request Sep 6, 2026
…SuspendedRun

`persistSuspendedRun` writes its `suspendedRuns` entry before it awaits the
durable save and marks the run cache-only only after that save settles, so for
the duration of the await the entry is live and unqualified. A concurrent per-id
`loadSuspendedRunStrict` reads a store that truthfully has no row yet and takes
the eviction path #16031 added, deleting an entry for a run being parked now.

Measured on this head rather than inherited: the interleaving is REACHABLE on an
ordinary single-process composition, and needs no out-of-band knowledge of the
run id, because the map write is what publishes the id to `listSuspendedRuns`.

Pinned at the bounded outcome the limit promises -- still resumable through the
store-first strict loader, merely omitted from the cache-only listing -- plus
the controls in which the window cannot bite. One compound case is pinned at its
MEASURED behaviour and deliberately not fixed: when the save then FAILS, the
run has neither a durable row nor a map entry and is unresumable, which escapes
the resumability bound. Choosing between widening the cache-only marking, a
lock, and reordering the save is a decision above this card.

Comments at the window and on `evictConsumedSuspension` point at the pin.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
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/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants