refactor(daemon): one rule table for store retention, plus the pool's orphan proof - #1085
Conversation
There was a problem hiding this comment.
Approved — no blocking findings in cf934835c02fa1bb8ad393601e1c451c4dc4bcca.
I traced the shared-store sweep through candidate selection, batched agent/exists, timestamp-fenced deletes, dry-run/failure exit behavior, and the control-plane placement/duty predicate. The implementation fails closed when the existence read is unavailable and preserves rows reclaimed between list and delete.
Non-blocking warning: removing the former 30-day session_purges pruning while intentionally skipping local stores means a local daemon whose control plane never accepts purge receipts can retain those rows indefinitely. The PR calls out the older-control-plane case; under the repository's active-development compatibility policy, I do not consider that a blocker.
Verification was static against the trusted synthetic merge (parents exactly the supplied base and head) and the exact PR diff. I could not rerun the listed tests because this isolated checkout has no installed dependencies and its sandbox denies /dev/null, which prevents the normal Git/pnpm shims from starting.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
cf93483 to
442c732
Compare
|
Second pass: the reaper is now a declarative rule table ( The sweep now has two callers: the daemon's own hourly tick runs the rules age-only, so a local install keeps exactly the retention it had, and Routines that are lifecycle rather than retention are listed in the body with the reason each stayed — state transitions, lease recoveries, and the per-write caps that bound a UI surface with no age and no owner. The per-routine prune tests are replaced by rule-table tests that also run every declared rule against real PostgreSQL. Rebased onto main after #1081/#1086/#1087; a deferred managed distill waits as |
There was a problem hiding this comment.
Changes requested for 442c7325bac9bf9997ed71abec71d93dc607d1c6.
The consolidated retention engine no longer preserves the runtime-catalog startup ordering: stale cache rows are hydrated and can be advertised in the initial Control Plane facts before the first age sweep. The inline finding describes the required startup fence.
Non-blocking warning under the project's active-development compatibility policy: the session-purge rule uses COALESCE(claimedAt, purgedAt) even though the removed 30-day prune aged by purgedAt. Every report attempt refreshes claimedAt, including an unsupported response or a failing request, so a permanently unaccepted receipt can still be retained indefinitely despite the new local sweep. If exact prior retention is intended, the rule needs separate age and CAS clocks; I am not making that compatibility/failure-mode difference an additional blocker here.
Verification was static against the trusted synthetic merge whose parents are exactly the supplied base and head, the new commit delta, and the complete current PR patch. I could not rerun the listed tests because this isolated checkout has no installed dependencies and its sandbox denies /dev/null, preventing the normal Git/pnpm shims from starting.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| void this.drainSessionPurges() | ||
| // Every table's row retention, from the one rule table (store/retention.ts). Age only: | ||
| // the agent-existence proof needs a control-plane read that only `reconcile --once` has. | ||
| void this.storeRetention.sweep() |
There was a problem hiding this comment.
Run the age sweep before hydrating the runtime catalog cache. hydrateRuntimeCatalogCache() executes during startup before Control Plane registration, but this is the daemon's only storeRetention.sweep() call and startup resets lastSessionRetentionSweepAt, so it cannot run until the first hourly interval (and never runs when idleSweepMs <= 0). A catalog older than the 30-day rule—which the removed gcRuntimeCatalog deleted synchronously—is therefore loaded and advertised in the initial runtime facts, potentially offering models/capabilities the runtime no longer has. Please invoke the age-only sweep before hydrateRuntimeCatalogCache() and keep its cadence independent of disabling idle reclamation.
|
Thanks—the current PR head is still Non-blocking: sent by |
…le job The Kubernetes half of the orphan reconciler collects sandbox objects nobody will ever remove. The same leak exists one layer down: a pool member writes a row into a per-member outbox on the shared data-plane store, dies, and no peer can drain it. Rather than one retention rule per table, `store/orphan-reaper.ts` sweeps them all inside the existing `agentconnect-daemon reconcile --once` CronJob, on the same batched `agent/exists` answer the cluster half already asked for. Four tables, one predicate: `inbox` hook-completion reports, `session_metadata_outbox`, `session_purges`, and `webchat_mcp_grant_ledger` — everything carrying an `ownerId`/`claimedAt` lease. A row is collected when the control plane no longer knows its agent (nobody can ever report it), or when nothing has written it for the single 7-day horizon (a live owner renews its claim on every drain attempt, so an untouched claim means its owner is gone). Every delete carries the clock the row was judged on as its CAS, so a row a member claimed between the read and the delete is left alone. Dry run by default, opt in with `AC_STORE_ORPHAN_DELETE`, one summary line per run. A local single-daemon store is skipped outright: it has one owner forever, so its rows are its own. The store methods are plain portable SQL and the suite runs in the `store-postgres` project as well as on SQLite. On the control plane, `PoolMemberReaper` gains the counterpart. Retiring a member no longer cascades its agents' webchat MCP delegations away now that the row is agent-keyed, so each sweep revokes the ones nothing serves. The predicate is exactly `PlacementResolver.servingDaemons(agent) === []` — neither placement column names a target and no unexpired duty lease holds the agent. No dry run: such a row is already inert, since the live authority check answers `placement_mismatch` on every use, so the write only records what is true.
…r replaces `drainSessionPurges` ran its own 30-day prune of `session_purges` on every sweep, with its own constant, its own warn line and its own idea of how long an unreportable receipt is worth keeping. The reaper now answers that question for every per-member outbox table from one horizon, so the special-purpose copy goes and retention has one home. Deliberately kept: the in-process set that holds a peer's permanently rejected hook report out of THIS daemon's drain. It reads like retention but is not — it stops a member that serves the agent from re-attempting a report the control plane can only ever refuse. The reaper bounds how long the row survives; that set bounds how often it is retried in the meantime.
…per-table prunes
The reaper sat next to the cleanup it was meant to replace. Retention across the
daemon store was a private constant and a `DELETE … WHERE … < x` wherever each
table's writer happened to live, so "how long do we keep X" had as many answers
as there were writers, and a table nobody remembered simply grew.
`store/retention.ts` is now the one home: a rule per table
(`{ table, key, clock, where?, agentColumn?, ownerColumn?, horizonMs,
foreignHorizonMs? }`), one sweep loop, one summary line. The sweep composes both
the SELECT and a re-fenced DELETE from the rule, and the clock the row was judged
on rides the DELETE, so a row anything wrote in between survives. A rule's clock
is the last write ANY process made to the row, so a live owner that renews a
claim or re-stamps a cache is never collected.
Deleted, their semantics now rule rows:
- `pruneSessionPurges` — gone in the previous commit, now a 30-day rule.
- `gcRuntimeCatalog` and its two cutoff arguments, plus the startup call: the
30-day own window and the 7-day departed-owner window are `horizonMs` and
`foreignHorizonMs` on the two catalog rules, and the per-catalog clock that
kept a phase-1 refresh from stripping its own model rows is the rule's `clock`.
- the terminal-row purge inside `expireMemoryCaptures`, with
`MEMORY_CAPTURE_TERMINAL_RETENTION_MS`, the `terminalRetentionMs` option and
the maintenance deadline behind it. What is left there is a state change with a
redaction and a metric, which is not retention.
- the settled-record delete inside `expireActivations`, with
`ACTIVATION_RETENTION_MS`.
Two callers, one table. The daemon's own hourly sweep runs the rules age-only, so
a local single-daemon install keeps exactly the retention it had — the CronJob
never ran there, and retention was never about ownership there either. Only
`reconcile --once` has the `agent/exists` answer, so only it applies the
agent-gone proof, still dry-run behind `AC_STORE_ORPHAN_DELETE`. The horizon
proof always deletes: it is the retention these rules replaced.
Left alone, because they are lifecycle rather than retention: the redaction half
of `expireMemoryCaptures`, `pruneRuntimeModelCaps` (one discovery's
set-difference), the ownership takeovers, the session TTL/GC sweeps (worktrees
and CP receipts), `loop_guard`'s cleanup (its cutoff is the caller's window, and
it rides the charge statement), and the per-agent caps on permission-request
history, acknowledged hook receipts and memory `.history` — real-time caps with
no age and no owner.
Tests follow the same move: the per-routine prune tests are gone with the
prunes, replaced by rule-table tests that drive the engine and run every declared
rule against the real schema on both SQLite and PostgreSQL.
442c732 to
7edf99e
Compare
|
Both items addressed on 7edf99e. Startup ordering and cadence (blocker). Age clock. Judged on the merits the leak is real, and it was wider than the one rule: The DELETE still re-fences on the clock, which now means something sharper: a rule whose clock moves on new work cannot collect an obligation renewed between the read and the delete. Where the clock is immutable the rule's own Rebased onto main through #1091. Daemon store/hook/inbox/catalog/lifecycle suites, the whole |
There was a problem hiding this comment.
Approved. This revision fixes the previous blocking issues: age-only retention now runs synchronously before runtime-catalog hydration, its periodic cadence is independent of idle reclamation, and outbox retention ages the underlying work timestamp rather than retry lease renewal. The complete current GitHub check suite is green, including daemon-store PostgreSQL coverage.
One non-blocking edge remains: classify gives the agent-gone proof precedence over the horizon proof. With AC_STORE_ORPHAN_DELETE left false, a row satisfying both proofs is therefore retained as an orphan dry run even though horizon retention is documented as always deleting. Checking the horizon first, or representing both proofs, would keep the switch scoped strictly to early orphan collection.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
| now: number, | ||
| live: Set<string> | undefined | ||
| ): 'agent-gone' | 'horizon' | undefined { | ||
| if (live && rule.agentColumn && row.agentId && UUID.test(row.agentId) && !live.has(row.agentId)) return 'agent-gone' |
There was a problem hiding this comment.
Non-blocking: this early return masks an already-satisfied horizon. When the agent is gone and deleteOrphans is false, collect dry-runs the row instead of applying the always-on age retention. Consider evaluating the horizon first or returning both proofs so the orphan switch only gates rows that are not yet old enough.
|
Confirmed on current head The only remaining note is non-blocking: when a row satisfies both proofs, sent by |
Summary
The store half of the orphan reconciler from #1062 — and, after the first review, the
consolidation it was supposed to be rather than a reaper sitting next to the cleanup it
was meant to replace.
Row retention across the daemon store was a private constant and a
DELETE … WHERE … < xwherever each table's writer happened to live, so "how long do we keep X" had as many
answers as there were writers, and a table nobody remembered simply grew. All of it is now
declarative data in
packages/daemon/src/store/retention.ts: one rule per table, onesweep loop, one summary line.
The sweep composes both the SELECT and a re-fenced DELETE from the rule, and the clock the
row was judged on rides the DELETE, so a row anything wrote between the read and the delete
survives. A rule's
clockis the last write any process made to the row, so a liveowner that renews a claim or re-stamps a cache is never collected. Adding retention to a
table is a rule, not a function; changing a horizon is a number, not a cutoff to find.
What is collected
Two proofs, and a rule may earn either:
it is what the deleted routines already did, so it is always on. Windows differ per rule
because they always did — 7 days for a per-member outbox, 30 for a session-purge receipt
(the only record that a transcript was deleted) and the model-catalog cache, 1 day for
terminal memory captures and settled activation records.
AC_STORE_RETENTION_SCALEmovesall of them together.
drain it. This needs the batched
agent/existsanswer the Kubernetes half already asksfor, so only
reconcile --oncecan apply it, and it ships dry-run behindAC_STORE_ORPHAN_DELETE=true.ownerColumncarries the third case: a row written by a process that is not the sweeper.An
ownerIddies with the process that minted it, so the catalog rules reclaim a departedmember's cache on the shorter
foreignHorizonMswhile a live member keeps its own on thelong one — exactly what the hand-written catalog GC expressed with two cutoff arguments.
The eight rules:
hook-report,session-metadata,session-purge,webchat-grant,memory-capture,activation,catalog-meta,catalog-models.Two callers, one table. The daemon's own hourly sweep runs the rules age-only against
its own store, so a local single-daemon install keeps exactly the retention it had — the
CronJob never ran there, and retention was never about ownership there either. The pool's
reconcile --onceCronJob runs them with the control-plane read as well, and owns no rowsitself, so every rule falls back to its conservative window.
Control plane.
PoolMemberReaper.tick()ends withrevokeUnplaced. Now thatWebchatMcpDelegationis keyed on the agent rather than on a daemon row, retiring a memberno longer cascades its agents' delegations away. The predicate is exactly
PlacementResolver.servingDaemons(agent) === []in SQL — neither placement column names atarget, and no unexpired duty lease holds the agent. No dry run there, because the rows are
provably inert: with no serving daemon,
resolveLiveWebchatMcpAuthorityreturnsplacement_mismatchbefore any grant can be issued, accepted, redeemed or revoked. Thewrite changes no reachable behaviour; it makes the ledger say what is true and lets the
existing expiry reaper count the rows correctly.
What was removed
Every routine below is deleted, not wrapped:
pruneSessionPurges+SESSION_PURGE_RECEIPT_TTL_MS+ its call and warn line indrainSessionPurgeslocal-store.ts,daemon.tssession-purgerule, 30 daysgcRuntimeCatalog(both cutoff arguments, the two-table correlated sweep) + its startup call inhydrateRuntimeCatalogCachelocal-store.ts,daemon.tscatalog-meta/catalog-modelsrules,horizonMs30d +foreignHorizonMs7d, per-catalog clockexpireMemoryCaptures, plusMEMORY_CAPTURE_TERMINAL_RETENTION_MS, theterminalRetentionMsoption, and the terminal branch ofnextMemoryCaptureMaintenanceAtlocal-store.ts,memory-plugin/outbox.tsmemory-capturerule, 1 dayexpireActivations+ACTIVATION_RETENTION_MSlocal-store.tsactivationrule, 1 dayhook-reportrule, 7 dayslistStoreOrphanCandidates/deleteStoreOrphan, the hand-written four-table version from this PR's first passlocal-store.tslistRetentionCandidates/deleteRetentionRow, rule-drivenNet effect in the files that carried the cleanup: +91 / −184. The new module (+324) and
its suite (+388) are the replacement and its tests.
Inventoried and deliberately kept
A routine is not retention when it is a state transition, a lease recovery, or a real-time
cap:
expireMemoryCaptures' remaining half — redacts a live capture and emits a metric.A state change; only the terminal row it leaves is retention.
pruneRuntimeModelCaps— the set-difference of one successful discovery(prune-on-success), inside the discovery transaction.
recoverMemoryCaptures,recoverPermissionRequests,reclaimWebchatMcpGrants,markOwnedWebchatMcpGrantsRevoking— ownership takeovers, all CAS.expireActivations' remaining halves — one reports an operational delivery failure,the other releases a claim so the next attempt is a first attempt.
loop_guard's cleanup — its cutoff is the caller'swindowMsparameter, not ahorizon, and it rides the charge statement.
prunePermissionRequestHistory,acknowledgeHookInbox'smaxAcknowledgedReceipts,memory
.historyretention — per-agent/per-file caps applied on every write to bound aUI surface. No age, no owner; moving them to a sweep would let them drift between passes.
drain — live-loop protection, not retention. Without it a member serving the agent
re-attempts a report the control plane can only refuse, once per outbox lease, until the
horizon. The rule bounds how long the row survives; that set bounds how often it is
retried meanwhile.
On the control plane, the reapers around
poolMemberReaperwere inventoried and left:CronRunReaper(and its hook-run twin) is arunning → failedtransition, not a delete;RelaySweeperdeletes and then fans a shrunk roster;WebchatMcpOperationReaperrecoversattempt-fenced operations and gates its delete on the invocation ledger;
HookRedeliveryReconcilerasks GitHub to redeliver. The per-platform pending-install TTLreapers stay behind
CpPlatformProvider— a platform module owns its own reapers bycontract, and hoisting them would break that seam.
Test plan
packages/daemon/test/store-retention.test.ts(new, replaces the per-routine prunetests): every declared rule runs against the real schema on both backends and collects
nothing fresh; each rule's own horizon fires on its own schedule with per-rule counts;
a row one millisecond short is kept;
scalemoves every window at once; a departedwriter's catalog goes on the shorter window while the sweeper's own stays; a refreshed
catalog is kept whole including the models discovery just found; a forgotten agent's
rows go on one batched control-plane read; the agent-gone proof counts without
deleting until enabled; an age-only sweep still runs where nobody can be asked; a row
written between the read and the delete survives the CAS; an unanswerable existence
read fails the sweep; env parsing.
vitest.postgres.config.ts, so every rule's composed SQL runs on realPostgreSQL — which is what proves the correlated per-catalog clock and the
key/clockcomposition are portable.
packages/daemon/test/cli-reconcile.test.ts: the store half runs in the same job onthe same control-plane answer and its counters reach the summary; a failing store
delete exits 1 however clean the cluster was.
packages/control-plane:poolMemberReaper.test.ts(revokes and logs; a failingsweep keeps the loop alive) and
webchat-mcp-delegation.repo.test.tson real Postgres(an unplaced agent's delegation is revoked and the second sweep is a no-op; a
set-placed agent whose duty a live member holds is never revoked, and goes only once
that lease lapses).
origin/mainafter feat: keep a cluster agent's managed memory on its sandbox volume #1081/chore(control-plane): drop the envelope tables #1086/docs(designs): repair pool-design drift against what shipped #1087. feat: keep a cluster agent's managed memory on its sandbox volume #1081's deferred managed distillswait in
memory_capture_outboxaspending/accepted, which thememory-capturerule never matches — it collects terminal rows only.
memory,memory-fs,managed-distill-outbox,dreamsuites re-run green.typecheckdaemon + control plane; daemon store/hook/inbox/memory/dream suites; thewhole
store-postgresproject; control planetest:unit(1726) andtest:int(1378).pnpm lint,pnpm format:checkNo schema change:
SCHEMA_VERSIONstays at 11 and no migration step was added.Refs #1062, #1044, #1057, #1065, #1068, #970.