fix(daemon): scope the loop-guard trip to the member and make its counters atomic - #1069
Conversation
…nters atomic On a daemon pool the loop guard was global in its destruction and local in its enforcement, and its counters lost increments under concurrent members. `purgeLoopScopeInbox` scanned the whole install-wide inbox and deleted every row in the scope, including rows queued on peers whose in-memory state this process cannot see; the interruption half that followed only walked this process's own maps. So a trip on one member destroyed a peer's durable backlog without ever stopping the peer's live turns. The purge now skips rows whose agent this member does not serve, exactly like the sweeps and the replay path, and a member stops its own turns once per latch on the first admission its open circuit refuses. The counters were a JS read-modify-write followed by an absolute upsert, so two members charging the same conversation both read n and both wrote n + 1 — the undercount is worst exactly when the loop is fastest. Both the charge and the latch are now single relative, window-aware statements with `RETURNING`, so the verdict is computed from what was actually stored and the latch is a CAS that elects exactly one owner for the trip's side effects. That also removes the path's dependence on an exclusive writer, which the shared store cannot give it: the Postgres facade rewrites `BEGIN IMMEDIATE` to a plain `BEGIN`. The remaining transaction buys atomicity between the charge and the inbox marker only, and the rewrite now says why a shared-store statement has to be a CAS or a relative write. Local single-daemon behavior is unchanged: with no duty enforcement every agent is served here, so the purge still clears the whole conversation backlog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed head 162231126f9f420057b42c6d7cf1396225f39d8d against base 52e7754a2e672a4c464709e27d86af10f8b8bcf0.
One blocking pool-concurrency gap remains. The relative counter updates and member-scoped inbox purge look sound, but the new structural-trip CAS makes overlapping callers on losing members return trippedNow: false. The malformed-DM dispatch branch handles that result by purging the member’s inbox only; it does not call enforceLatchedLoopScope, so existing ACP turns on each losing member continue until some later admission happens to encounter the open latch. This directly undercuts the PR’s member-local interruption guarantee and is amplified by this change because overlapping structural trips previously let both callers report trippedNow: true and run their local interruption side effects.
Please enforce the already-latched scope on the structural-trip loser path as well. No additional tests are required for this review.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| ) | ||
| .run({ ...row, trippedAt: now, reason }) | ||
| .get({ scopeKey, now, reason }) as { totalCount: number; automaticCount: number } | undefined | ||
| if (!latched) return this.latchedLoopGuardVerdict(scopeKey) |
There was a problem hiding this comment.
This CAS correctly elects one side-effect owner, but it changes overlapping structural trips so every losing member returns trippedNow: false. In Daemon.dispatch’s malformed-DM branch, that result only calls purgeLoopScopeInbox; unlike the normal counter-latch path, it never calls enforceLatchedLoopScope. The winner can interrupt only its own in-memory turns, so losers leave their live ACP turns running until another admission arrives. Please enforce the latch in that false branch too (alongside the member-scoped purge).
The trip's CAS gives overlapping callers on losing members `trippedNow: false`, and the malformed-DM branch answered that with a member-scoped purge alone. So a member that lost the race kept its live ACP turns running until some later admission happened to hit the open latch. It now enforces the latch there as well, exactly like the counter-latch path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch — fixed in 34f9187. The structural trip's CAS gives overlapping callers on losing members New case in Re-ran the loop-guard, duty, sweep-pool, durable-inbox, serial-gate, hook, interrupt-safety |
There was a problem hiding this comment.
Reviewed head 34f9187c657b966dbd11ba26218becc355bd1bea against base 52e7754a2e672a4c464709e27d86af10f8b8bcf0.
Approved. The follow-up closes the prior blocking race: when a member loses the structural-trip CAS, the malformed-DM path now performs its member-scoped inbox purge and calls enforceLatchedLoopScope, so the elected winner remains the sole warning owner while every member still interrupts its own live ACP turns. The focused pool regression test exercises that loser path. I found no remaining blocking regressions in the relative counter updates, latch election, or member-scoped purge behavior.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
|
Confirmed—I re-reviewed sent by |
Summary
Fixes #1038. On a daemon pool the agent loop guard was global in its destruction and
local in its enforcement, and its counters lost increments under concurrent members.
Three defects, one seam:
purgeLoopScopeInboxscanned the whole install-wide inbox and deleted every row in the scope, including
rows queued on members whose in-memory state this process cannot see. The
interruption half that followed only walked this process's own maps.
a conversation, not an agent, so two agents in one channel held by two members
charged concurrently: both read
n, both wroten + 1, and one turn went uncounted.facade rewrites
BEGIN IMMEDIATEto a plainBEGIN, so the enclosing transactionbought no exclusivity — the second writer merely blocked on the row lock and then
overwrote with its stale-derived value.
Failure scenario
A runaway conversation involves two agents held by two members. The counters undercount
exactly when the loop is fastest, because that is when concurrency is highest, so the
circuit trips late or not at all. When it finally trips on member A, A deletes member B's
durable backlog rows out from under B's in-memory queue, but B's live ACP turns are never
interrupted and keep running. The durable latch does block new admissions on B, so the
outcome is a partial, confusing failure: work disappears, the loop continues, and the two
members' logs tell different stories.
Fix
The trip acts only on what this member serves.
purgeLoopScopeInboxskips rows whoseagent this member does not hold, through the same
servesAgentgate that already scopescrons, deadlines, the sandbox sweep and — since #1065 — the session TTL/GC sweeps. A peer's
queued row is left for its holder, never destroyed. The interrupt half is split out as
interruptLoopScopeTurns, which is member-local by construction, and a member that meets acircuit a peer latched stops its own turns once per latch, on the first admission it
refuses. The trip's operator warning still has exactly one owner.
The counters are relative and window-aware in SQL. Both the charge and the structural
trip are single statements with
RETURNING, identical on SQLite and Postgres: an upsertwhose
DO UPDATEincrements fromloop_guard.totalCount(guarded byWHERE loop_guard.trippedAt IS NULL, so a latched scope is never recharged and returns norow), and a guarded
UPDATEfor the latch itself. The verdict is therefore computed fromwhat was actually stored, and the latch is a CAS that elects exactly one member to run the
trip's side effects.
That removes the need for the lock rather than faking one. The remaining transaction in
recordLoopGuardTurnForInboxbuys atomicity between the charge and the inbox marker only —no
SELECT … FOR UPDATEpath was added, because nothing reads-then-writes any more. TheBEGIN IMMEDIATErewrite in the Postgres worker now states why a shared-store statement hasto be a CAS or a relative write, instead of leaving each caller to assume a writer lock it
never gets.
Local single-daemon behavior is unchanged: with no duty enforcement every agent is served
here, so the purge still clears the whole conversation backlog. No schema change.
Out of scope, per the issue: the k8s driver, the sweeps (#1065), and the outboxes (#1023).
Test plan
New
packages/daemon/test/daemon-loop-guard-pool.test.ts— two members over one store:cancelled; B then cancels its own head and purges its own row when it enforces the
latch, and does so once per latch, not on every subsequent refusal;
New cases in
packages/daemon/test/loop-guard.test.ts— twoLocalStoremembers over oneSQLite file, with a peer's charge interleaved at the exact moment the member's own write is
about to execute:
trippedNowowner, and so do two concurrent structural trips;
Both new store cases and the pool case fail on
mainand pass here.Run:
pnpm --filter @agentconnect.md/daemon typecheck, the loop-guard, duty, sweep-pool,durable-inbox, serial-gate, hook, interrupt-safety and local-store suites (389 tests),
pnpm lint,pnpm format:check.