fix(daemon): own the session-metadata outbox per member - #1068
Conversation
There was a problem hiding this comment.
Requesting changes for one blocking liveness bug in the new ownership handoff. A fresh session-metadata row owned by a retiring member is excluded from the successor's drain scope; the gain replay only resumes ownerless rows, and the scheduler does not arm a wake for the future owner-lease expiry. That can leave a terminal session snapshot unsynced indefinitely during the graceful rollout path this patch is intended to fix. The ownership/CAS direction otherwise looks coherent.
I verified the trusted synthetic merge has exactly the supplied base and head as parents, and the changed worktree blobs match the PR diff. Focused daemon tests were not runnable because this isolated checkout has no installed dependencies.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| if (!this.shared) return { sql: '', params: {} } | ||
| const scope = idScope('agentId', agentIds) | ||
| return { | ||
| sql: ` AND (ownerId = @ownerId OR ((ownerId IS NULL OR COALESCE(claimedAt, 0) <= @staleBefore)${scope.sql}))`, |
There was a problem hiding this comment.
[P1] Keep a wake for fresh rows owned by the previous holder. On a graceful handoff, A can persist a final snapshot, enter shutdown (which sets draining and clears the metadata retry timer), and release the duty before it parks the row. B then gains the agent while ownerId = A and claimedAt is still fresh. replayGainedSessionMetadata() only resets ownerId IS NULL rows, this scope hides the row from B, and schedulePendingSessionMetadataDrain() uses the same scope, so it arms no timer for claimedAt + SHARED_OUTBOX_LEASE_MS. When the claim eventually lapses, nothing wakes B unless an unrelated snapshot or reconnect happens, leaving the metadata unsynced indefinitely. Please either transfer/release rows for newly gained agents under a safe fence or include the owner-lease expiry in the next-attempt scheduling.
|
Good catch on the graceful handoff — it was a real hole, and the first push had no way out of it. Fixed in
Tests (all three fail on the previous commit):
Re-ran |
|
The original handoff hole is fixed, but
Consequently, when a pool row is pending, the scheduler query fails instead of arming the lease-expiry/retry timer—the no-duty-change recovery path can still remain stuck. Please use a portable sent by |
|
@review-bot continue |
There was a problem hiding this comment.
Requesting changes for one blocking backend-portability defect on 3d2792241. The new duty-gain reclaim and shutdown release address the original graceful-handoff hole, but the lease-expiry scheduler uses SQLite's scalar two-argument MAX syntax. The production shared pool runs this LocalStore query through PostgreSQL, whose worker does not rewrite that expression, so the query fails instead of arming the fallback timer. The no-duty-change lease-expiry path can therefore still remain stuck.
The checkout ref matches the trusted head exactly. The new focused tests exercise DatabaseSync/SQLite and do not cover this PostgreSQL query path.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| nextSessionMetadataAttemptAt(ownerId?: string, agentIds?: readonly string[]): number | undefined { | ||
| const scope = this.sessionMetadataWorkScope(ownerId, agentIds) | ||
| const lapse = this.shared | ||
| ? `MAX(COALESCE(nextAttemptAt, 0), CASE WHEN ownerId IS NOT NULL AND ownerId <> @ownerId |
There was a problem hiding this comment.
[P1] Keep this lease-expiry expression portable to the real shared-store backend. SQLite accepts MAX(x, y) as a scalar maximum, but PostgreSQL's MAX is a one-argument aggregate, and postgres-store-worker.js#rewrite does not translate this expression. Consequently, once hasPendingSessionMetadata() finds a pool row, nextSessionMetadataAttemptAt() throws, the refill check only logs a warning, and no lease-expiry timer is armed—the exact no-duty-change recovery path added here stays stuck. Please use a portable CASE expression or explicitly rewrite this to PostgreSQL GREATEST, and exercise the query through PostgresSyncDatabase.
Fixes #1023. On a daemon pool the session-metadata outbox is one shared table, but an `event/session-sync` frame is scoped by the agent's organization, which only a member serving that agent can resolve. Every member drained every row, and a snapshot whose organization it could not resolve was raised locally as a non-retryable SCOPE_DENIED, deferred, and retried forever on every member. The outbox now carries the same ownership model as the hook-completion outbox and the purge receipts: `ownerId` / `claimedAt` (schema v10 plus its migration step), a member is offered its own rows and unowned or lapsed rows only for the agents it serves, and it claims before every emit. A snapshot this member cannot scope is parked — the claim is released for whichever member serves the agent, the body and the failure count survive, and a short backoff keeps it out of this member's next pass instead of head-of-line blocking the rows behind it. Gaining a duty re-arms the parked snapshots of the gained agents and re-runs the drain, so a takeover replays them at once. A local single-daemon store is unchanged: it lists and settles unfenced, the claim is a no-op, and parking is refused so the existing retry/defer path still applies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on #1023. A graceful handoff could strand a snapshot: member A persists it, enters shutdown (draining, retry timer cleared) and releases the duty before parking the row, so the row still names A with a fresh claim. The successor's scope hid it, the duty-gain replay only re-armed unowned rows, and nothing was scheduled for the moment the claim lapsed. Three ways out, all of them cheap: - `reclaimSessionMetadataSnapshots` now releases a previous holder's claim for the gained agents, not just the parked rows. The duty ledger has already proved that member no longer serves the agent; this member's own claims are left alone, since only it knows whether one is in flight. - `nextSessionMetadataAttemptAt` and `hasPendingSessionMetadata` read a wider work scope — own rows plus every row of a served agent — and the wake is armed at `claimedAt + SHARED_OUTBOX_LEASE_MS` for a row a peer still holds, so the drain runs when the claim lapses even without a duty change. - Shutdown releases every claim this member still holds, once its drain has been joined and before the store is closed, so a successor does not wait out a lease nobody will renew. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on #1023. `nextSessionMetadataAttemptAt` reached for SQLite's scalar two-argument `max(x, y)`. The same statement text runs through PostgreSQL on a pool, where `MAX` is only an aggregate, so the query raised `function max(bigint, bigint) does not exist`. The refill check only logs a failed read, so the lease-expiry wake was never armed there — exactly the recovery path the previous commit added. The later-of is expressed as one CASE instead, which both engines parse the same way. No rewrite rule was added to the worker: `MAX(` → `GREATEST(` would also rewrite every legitimate one-argument aggregate in this file. `postgres-pool-store.int.test.ts` gains a session-metadata case that drives the lease, the claim CAS, the park, the reclaim and this query through `PostgresSyncDatabase`. It fails with the two-argument form and passes with the CASE. The stale `getCronLastRun` assertion in the first case is repaired in passing — that method was renamed to `cronRun` and the suite only runs with a database, so nothing caught it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3d27922 to
3afa7cc
Compare
|
Right, and it reproduced exactly as described. Fixed in The bug — driving Since the refill check only logs a failed read, the lease-expiry wake was never armed on a pool — the one recovery path the previous commit added. The fix — the later-of is one CASE WHEN ownerId IS NOT NULL AND ownerId <> @ownerId
AND COALESCE(claimedAt, 0) + @lease > COALESCE(nextAttemptAt, 0)
THEN COALESCE(claimedAt, 0) + @lease ELSE COALESCE(nextAttemptAt, 0) ENDI deliberately did not add a worker rewrite: Exercised against a real engine —
I also exercised the in-place migration on PostgreSQL rather than only the fresh-schema path: dropped Rest of the diff audited for engine-specific SQL ( One drive-by in the same file: the first case called Re-ran |
There was a problem hiding this comment.
Approved on 3afa7cce. Both prior blocking findings are resolved:
- Graceful handoff now reclaims fresh peer-owned rows when duty is gained, keeps a wake armed for a live peer claim's lease expiry, and releases this member's remaining claims during shutdown.
- The lease-expiry calculation now uses a SQLite/PostgreSQL-portable
CASEexpression, with a real PostgreSQL pool-store integration test covering claim isolation, lapse, takeover, parking, reclaim, release, and settlement.
I inspected the complete rebased base-to-head diff. The checkout ref matches the trusted head exactly, and every changed worktree blob matches the PR diff. I could not rerun the suites locally because this isolated checkout has no installed dependencies.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
|
Confirmed—the sent by |
* test(daemon): run the store suites on real PostgreSQL The daemon pool opens `LocalStore` over `PostgresSyncDatabase`, but every store test ran on SQLite only, so a SQLite-only construct reached a cluster before anything noticed. A `store-postgres` Vitest project now re-runs the store suites with `LocalStore` opened over the real pool store against a Testcontainers `postgres:16-alpine` — one container per run, one database per worker, a schema-wide sweep between tests. `test/store-postgres/backend.ts` is the seam: unarmed it leaves the SQLite run exactly as it was, armed it hands the suites `openPostgresLocalStore()`. `local-store-sql-portability.test.ts` is the cheap half: it reads the SQL text out of `local-store.ts` and fails on constructs the pool worker does not rewrite (two-argument `MAX`/`MIN`, `IFNULL`, `IIF`, `datetime()`, `INSERT OR REPLACE`, `GROUP_CONCAT`, `printf`, `TYPEOF`, `||`, comma `LIMIT`), so a statement no suite covers still fails fast. The PostgreSQL run caught one: the activation rendezvous joined its composite key and transcript coordinates with NUL, which PostgreSQL TEXT rejects outright, so every paired-activation write threw on the pool. The separator is now the unit separator; those rows are TTL-bounded, so no stored key outlives the change. Refs #1073, #1068 * ci: gate the daemon store suites on real PostgreSQL A dedicated Docker-bearing runner for the `store-postgres` project, off the critical path the way the control-plane integration shards are. The suites take about half a minute after the container boots, so the pool's SQL stops reaching a cluster untested.
Summary
Fixes #1023. On a daemon pool the session-metadata outbox (
session_metadata_outbox)lives in the shared data-plane store, but an
event/session-syncframe is scoped by theagent's organization, and the daemon resolves that organization from its own agent
registry — which carries only the agents this member serves. The drain had no owner and
no agent filter, so every member picked up every row, and a row it could not scope was
raised locally as a non-retryable
SCOPE_DENIED, deferred, and retried forever.The outbox now gets the ownership model the hook-completion outbox (#1035) and the purge
receipts (#1032) already use —
SHARED_OUTBOX_LEASE_MS, a CAS claim before every emit,an owner-fenced ACK — plus one thing those two do not need: a row this member cannot
scope is parked for the member that can, instead of counting as a failure.
Failure scenario
outbox. The rollout then moves X's duty to member B (or A is retired).
row, oldest first.
CpClient.scopedFrameneeds anorgIdfor a non-install-wide frame and derives itfrom the payload's agent id. A member that does not serve X has no spec for X, so
orgForAgentyields nothing and the client throwsSCOPE_DENIEDlocally, beforeanything is sent.
retryable: falseis read as "permanently rejected": the row is failure-counted anddeferred, logging
event/session snapshot deferred after N failures for session <id>on every member, with N climbing forever. The snapshot is never persisted by the CP,
and because the drain is one row at a time it also delays every row behind it.
Fix
Ownership in the store (
packages/daemon/src/store/local-store.ts)session_metadata_outboxgainsownerId/claimedAt(schema v10 plus itsSCHEMA_MIGRATIONSstep, so an existing store upgrades in place).saveSessionMetadataSnapshotstamps the writing member — the daemon that produced thesnapshot is serving the agent, so it is the one that can scope the frame.
nextSessionMetadataSnapshot,hasPendingSessionMetadataandnextSessionMetadataAttemptAtshare one scope: this member's own rows, plus unowned orlease-lapsed rows for agents it currently serves. Unlike the hook outbox, an
unowned row is deliberately not offered install-wide — the frame carries the agent's
organization, so a parked snapshot must wait for the member that has it rather than
circling the pool. The scheduler reads the same scope, so a foreign row can no longer
keep re-arming the retry timer.
claimSessionMetadataSnapshotis the CAS taken before every emit (revision-fenced, soa newer snapshot written mid-flight is not claimed by the older attempt).
parkSessionMetadataSnapshotreleases the claim and applies a short backoff withouttouching the body or the failure count;
acknowledgeSessionMetadataSnapshotandrecordSessionMetadataSnapshotFailureare fenced to the claim holder, so a member cannever destroy or fail a peer's row.
resumeSessionMetadataSnapshotsre-arms the parked rows of a set of agents.Drain and duty handover (
packages/daemon/src/daemon.ts)runSessionMetadataDrainreads with the owner + served-agent scope, claims before itemits, and skips (never
returns on) a row a peer took over between the read and theclaim.
SCOPE_DENIEDraised while emitting. Neither incrementsfailedAttemptsor logs thedefer warning; the row simply waits for its serving member.
settleDutyChangere-arms the parked snapshots ofagentsGainedand re-runs the drain,next to the purge-receipt replay added in fix(daemon): scope the session TTL/GC sweeps to the duty holder and lease purge receipts per member #1065, so a takeover replays them immediately
rather than after the backoff a departed holder wrote.
A local single-daemon store is unchanged: it lists and settles unfenced, the claim is a
no-op, and parking is refused so the pre-existing retry/defer path still applies.
Test plan
packages/daemon/test/daemon-session-metadata-outbox-pool.test.ts(new) — twomembers over one shared store: B drains only its own row and leaves A's pending and
unfailed; a row for an agent nobody here serves is parked (claim released, backoff
set,
failedAttemptsstill 0, no defer warning) and the peer leaves it alone; Bgaining the duty replays that row exactly once, scoped to the agent's org, and a
second drain emits nothing; a served row whose organization is momentarily
unresolvable is parked, not failure-counted, and drains once the scope resolves; a
single daemon on its own store drains every row unfenced. The first four fail on
main.packages/daemon/test/local-store.test.ts— the v1/v5/v7 upgrade paths land onschema v10 with
ownerId/claimedAtpresent on the outbox.pnpm --filter @agentconnect.md/daemon typechecklocal-store,daemon-session-metadata-outbox-pool,daemon-smoke,durable-inbox,daemon-lifecycle,daemon-session-sweeps-pool,postgres-migrations,daemon-duty-*,orchestrationsuitespnpm lint,pnpm format:check