Skip to content

Commit 4368411

Browse files
claude[bot]claude
andauthored
fix(service-automation): a conditional advance claim on SuspendedRunStore, so two replicas cannot both advance one run (#14712)
* fix(service-automation): conditional advance claim on SuspendedRunStore (#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 * test(service-automation): record the measured ablation ledger (#14333) 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 * docs(permissions): re-certify the tenant-audit census at 218 write call 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 * test(service-automation): pin the compare-and-set condition and the production 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 * test(service-automation): record the three measured ablation ledgers (#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 * chore(gates): record the newly pinned engine double and close its DEBT 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a7a7390 commit 4368411

10 files changed

Lines changed: 1332 additions & 41 deletions
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@objectstack/service-automation": minor
3+
---
4+
5+
fix(service-automation): make the resume advance a conditional claim on the suspended-run store, so two replicas cannot both advance one run (#14333)
6+
7+
`AutomationEngine.resumeInternal` guarded a duplicate resume with `this.resuming`,
8+
an in-process `Set`. That is a complete guard for exactly one deployment shape: a
9+
single process. Behind a load balancer, two decisions on one run arriving in the
10+
same instant on two replicas each passed their **own** `resuming` check, both read
11+
the same fresh row out of the shared store, both consumed it, and both traversed
12+
forward — so every downstream side effect ran twice. #13617 closed the sequential
13+
half of this family (a replica resuming from a snapshot it had gone stale on); it
14+
deliberately did not close the concurrent one.
15+
16+
Measured before the fix on the two-engines-over-one-shared-store harness, at
17+
`packages/services/service-automation/src/concurrent-replica-resume-race.test.ts`:
18+
**25 of 25** raced runs advanced twice — one action fired twice and one approval
19+
level opened twice per run — for both reachable shapes the report named (parallel /
20+
any-of approvers, and duplicated automated approve calls). A single approver per
21+
level deciding sequentially does **not** race, and is pinned as the negative
22+
control.
23+
24+
`SuspendedRunStore` therefore gains `claimSuspension(runId, parkedAt)`: consume the
25+
durable record **only** if it is still parked at the node the caller read (and, when
26+
the caller has one, still carrying that correlation), atomically, answering
27+
`'claimed'` / `'lost'` / `'unsupported'`. The winner advances; the loser is refused
28+
`RESUME_IN_PROGRESS` — the existing code, because the remedy is identical to the
29+
in-process refusal's and `plugin-approvals` already branches on it that way — and
30+
runs nothing. The per-process `resuming` set stays as the cheap first gate; it is
31+
not replaced, and the single-replica path is unchanged.
32+
33+
Both shipped stores implement it: `InMemorySuspendedRunStore` tests and removes
34+
with no `await` between the two, and `ObjectStoreSuspendedRunStore` issues one
35+
`DELETE … WHERE id = ? AND node_id = ?` through the data engine's documented
36+
compare-and-set route (`multi: true` with a full `where`), reading the affected-row
37+
count. ⛔ No platform-object schema change: `node_id` and `correlation` are columns
38+
`sys_automation_run` already carried.
39+
40+
The member is **optional**, so no existing implementation is broken, and its absence
41+
is a declared degradation rather than a silent one: an engine whose store cannot
42+
express the condition says once, at `warn`, that resume idempotency is in-process
43+
only and what that costs — the same posture `AutomationEngine.claim()` already takes
44+
when no persisted flow-dispatch ledger is attached.

content/docs/permissions/tenant-audit-census.mdx

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ are reported as `undecidable` rather than assumed either way.
9898

9999
The same holds twice over for the context. An options argument spelled as a
100100
literal can be read; one spelled `options`, `{ ...opts }`, or handed through a
101-
forwarding shim cannot, and **67 of the 217 sites are spelled that way**. A
101+
forwarding shim cannot, and **67 of the 218 sites are spelled that way**. A
102102
context resolved from an inline literal or a local `const` can be tested for
103103
`isSystem`; one arriving from a helper call cannot.
104104

@@ -147,10 +147,10 @@ reproduce them. Where it disagrees, it disagrees on the page:
147147

148148
| carried figure | where it survives | this census |
149149
| :--- | :--- | ---: |
150-
| 175 write call sites | quoted in the merged changeset | **217** |
150+
| 175 write call sites | quoted in the merged changeset | **218** |
151151
| 24 carrying no tenant context | quoted in the merged changeset | **9** provable and tenancy-enabled; **32** more whose options argument is unreadable |
152-
| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **145 of 217** decidable, **72** undecidable |
153-
| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 99 decidably elevated, 0 decidably not, 101 undecidable |
152+
| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **146 of 218** decidable, **72** undecidable |
153+
| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 100 decidably elevated, 0 decidably not, 101 undecidable |
154154
| 141 and 132, two independent re-derivations | the card that filed this work ||
155155

156156
**The differences are not reconciled, and deliberately so.** The old census's
@@ -161,17 +161,17 @@ at any commit.
161161

162162
Two structural facts do plausibly widen this reading against any hand or regex
163163
one, and both are counted in the generated tables below: the 45 sites reached
164-
through an erased (`any`) receiver, and the 37 that name their object through a
164+
through an erased (`any`) receiver, and the 38 that name their object through a
165165
`const` rather than inline. An instrument that read either the way a person does
166166
would report a smaller number and would not say so.
167167

168168
The fourth row is the one worth flagging to anyone citing it. **The 135 / 77%
169169
figure has no surviving corroboration anywhere in the tree.** This census reads
170-
99 of 217 (45%) as decidably elevated, with 101 more whose elevation is a
170+
100 of 218 (46%) as decidably elevated, with 101 more whose elevation is a
171171
run-time fact — so the claim is neither confirmed nor refuted, and the honest
172172
answer is that a static reading cannot settle it.
173173

174-
**Cite `9 / 217`, and say what it is**: the sites whose options argument was
174+
**Cite `9 / 218`, and say what it is**: the sites whose options argument was
175175
READ and holds no tenant context, against a decidably tenancy-enabled object.
176176
That is the control's provable yield surface. ⛔ Do not cite it as "the sites
177177
without tenant context" — **32 further sites** have an options argument this
@@ -183,29 +183,29 @@ cannot read, and they are neither in nor out.
183183

184184
| what | count |
185185
| :--- | ---: |
186-
| write call sites on the application surface | **217** |
187-
| …whose object name is statically decidable | 145 |
186+
| write call sites on the application surface | **218** |
187+
| …whose object name is statically decidable | 146 |
188188
| …whose object name is chosen at run time | 72 |
189-
| …against an object with tenancy ENABLED | 145 |
189+
| …against an object with tenancy ENABLED | 146 |
190190
| …against an object that declares tenancy off | 0 |
191-
| threading a tenant context | 133 |
191+
| threading a tenant context | 134 |
192192
| PROVABLY carrying none (options read, no context key) | **17** |
193193
| …of those, against a decidably tenancy-enabled object | **9** |
194194
| options argument UNREADABLE — may or may not carry one | 67 |
195195
| …of those, against a decidably tenancy-enabled object | 32 |
196-
| threading a decidably ELEVATED (`isSystem`) context | 99 |
196+
| threading a decidably ELEVATED (`isSystem`) context | 100 |
197197
| threading a context that is decidably NOT elevated | 0 |
198198
| threading a context whose elevation is a run-time fact | 101 |
199199

200200
| how the instrument reached the site | count |
201201
| :--- | ---: |
202-
| receiver carried a readable engine type | 172 |
202+
| receiver carried a readable engine type | 173 |
203203
| receiver erased, placed by the object NAME | 19 |
204204
| receiver erased, placed by an `object: string` PARAMETER | 15 |
205205
| receiver erased, placed by an `UNTYPED_RECEIVERS` row | 11 |
206206

207207
| object name spelled inline | 108 |
208-
| object name spelled through a `const` | 37 |
208+
| object name spelled through a `const` | 38 |
209209
| object name is an `object: string` parameter | 19 |
210210
| object name is some other run-time expression | 53 |
211211

@@ -224,13 +224,13 @@ holds still. They are required to be HERE and to say WHEN they were true;
224224
their values are not compared. The reasoning, and the measurement behind it,
225225
are in `scripts/check-tenant-audit-census.mjs`.
226226

227-
Measured on 2026-09-01 at `d3ebf3b55`.
227+
Measured on 2026-09-02 at `5daab8df0`.
228228

229229
| corpus scale (not enforced) | count |
230230
| :--- | ---: |
231-
| tracked non-test sources scanned | 534 |
231+
| tracked non-test sources scanned | 539 |
232232
| engine-shaped types recognised | 56 |
233233
| declared objects in the registry | 297 |
234-
| same-named calls subtracted as non-engine | 119 |
234+
| same-named calls subtracted as non-engine | 130 |
235235

236236
{/* END GENERATED: tenant-audit-census */}

docs/audits/2026-08-tenant-audit-write-call-sites.counts.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,17 @@ silent, and `node scripts/tenant-audit-census.mjs --write` is the resolution.
2929

3030
| Measure | Value |
3131
|---|---:|
32-
| Write call sites | 217 |
33-
| Object name statically decidable | 145 |
32+
| Write call sites | 218 |
33+
| Object name statically decidable | 146 |
3434
| Object name chosen at run time | 72 |
35-
| Against a tenancy-enabled object | 145 |
35+
| Against a tenancy-enabled object | 146 |
3636
| Against an object declaring tenancy off | 0 |
37-
| Threading a tenant context | 133 |
37+
| Threading a tenant context | 134 |
3838
| Provably carrying none | 17 |
3939
| …and decidably tenancy-enabled | 9 |
4040
| Options argument unreadable | 67 |
4141
| …and decidably tenancy-enabled | 32 |
42-
| Threading a decidably elevated context | 99 |
42+
| Threading a decidably elevated context | 100 |
4343
| Threading a decidably non-elevated context | 0 |
4444
| Threading a context of undecidable elevation | 101 |
4545

@@ -52,14 +52,14 @@ holds still. They are required to be HERE and to say WHEN they were true;
5252
their values are not compared. The reasoning, and the measurement behind it,
5353
are in `scripts/check-tenant-audit-census.mjs`.
5454

55-
Measured on 2026-09-01 at `d3ebf3b55`.
55+
Measured on 2026-09-02 at `5daab8df0`.
5656

5757
| corpus scale (not enforced) | count |
5858
| :--- | ---: |
59-
| tracked non-test sources scanned | 534 |
59+
| tracked non-test sources scanned | 539 |
6060
| engine-shaped types recognised | 56 |
6161
| declared objects in the registry | 297 |
62-
| same-named calls subtracted as non-engine | 119 |
62+
| same-named calls subtracted as non-engine | 130 |
6363

6464
## Every site
6565

@@ -158,7 +158,7 @@ Measured on 2026-09-01 at `d3ebf3b55`.
158158
| `packages/services/service-automation/src/builtin/crud-nodes.ts` | `insert` | `objectName` | undecidable | context, elevation undecidable | 1 |
159159
| `packages/services/service-automation/src/builtin/crud-nodes.ts` | `update` | `objectName` | undecidable | context, elevation undecidable | 1 |
160160
| `packages/services/service-automation/src/flow-dispatch-store.ts` | `insert` | `sys_flow_dispatch` | enabled | elevated | 1 |
161-
| `packages/services/service-automation/src/suspended-run-store.ts` | `delete` | `sys_automation_run` | enabled | elevated | 2 |
161+
| `packages/services/service-automation/src/suspended-run-store.ts` | `delete` | `sys_automation_run` | enabled | elevated | 3 |
162162
| `packages/services/service-automation/src/suspended-run-store.ts` | `insert` | `sys_automation_run` | enabled | elevated | 2 |
163163
| `packages/services/service-automation/src/suspended-run-store.ts` | `update` | `sys_automation_run` | enabled | elevated | 2 |
164164
| `packages/services/service-datasource/src/datasource-admin-plugin.ts` | `delete` | `sys_metadata` | enabled | PROVABLY NONE | 1 |

0 commit comments

Comments
 (0)