feat(daemon): add retention sweeps + incremental auto_vacuum [#2339] - #2360
feat(daemon): add retention sweeps + incremental auto_vacuum [#2339]#2360lsm wants to merge 5 commits into
Conversation
Extend the daily job_queue.cleanup handler with configurable retention sweeps for terminal external/github events (+deliveries), mcp_audit_log, and space_goal_events, plus incremental_vacuum(500) page reclamation. - Retention deletion is OFF by default (HYPERNEO_RETENTION_ENABLED=1); each TTL is independently configurable. Only terminal-state rows are pruned; in-flight events are always kept so an active pipeline never loses work. - Enable PRAGMA auto_vacuum = INCREMENTAL on fresh DBs; convert existing auto_vacuum=NONE DBs via opt-in migration 169 (HYPERNEO_DB_VACUUM_MIGRATION), since a full VACUUM on a multi-GB DB is a long operation to schedule deliberately. incremental_vacuum then shrinks the file as pages are freed by retention + normal deletes. sdk_messages retention is intentionally deferred (out of scope for this pass; flagged for a separate policy decision).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…on-jobs-sdk-messages-events-audit-enable # Conflicts: # packages/daemon/src/storage/schema/migrations.ts
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES (posting as COMMENT only because the PR author and the review token are the same GitHub account — self-review rejects REQUEST_CHANGES.)
Overall this is solid work: the off-by-default policy, the gated one-time VACUUM migration, incremental-vacuum page reclamation, and the terminal-state-only pruning are all the right design. Tests are comprehensive (511 relevant tests pass — retention, cleanup-handler, db-core, all 459 migration tests) and oxlint/tsc/knip/db-schema-parity are green. The sdk_messages/sessions deferral is correctly scoped as a separate policy decision.
Two findings before merge, both small:
P2 — retention delete queries are full table scans on 3 of 5 tables (undermines the perf goal).
retention.ts states "the age columns are indexed", but that is only true for 2 of the 5 swept tables:
- ✅
space_external_events→idx_space_external_events_state (state, updated_at)(recreated by M124) - ✅
space_external_event_deliveries→idx_space_external_event_deliveries_state_updated (state, updated_at)(M165) - ❌
space_github_events→ only(task_id, occurred_at)and(space_id, repo_owner, repo_name, pr_number); no index leads withstate/updated_at. (Also usesupdated_atwhile the only time index is onoccurred_at.) - ❌
mcp_audit_log→(space_id, timestamp),(task_id, timestamp),(session_id, timestamp);timestampis never the leading column. - ❌
space_goal_events→(goal_id, created_at DESC),(space_id, created_at DESC),(source_task_id, created_at DESC);created_atnever leading.
The prune() helper also does COUNT-then-DELETE, so each unindexed sweep is two full scans. mcp_audit_log is one row per MCP tool call and is one of the largest tables on the 15GB DB this PR exists to fix — once HYPERNEO_RETENTION_ENABLED=1 is set, the daily sweep will hold a write lock while scanning it end-to-end. Recommend adding covering indexes (e.g. mcp_audit_log(timestamp), space_github_events(state, updated_at), space_goal_events(created_at)) — or at minimum correct the comment and chunk the deletes (e.g. DELETE ... WHERE rowid IN (SELECT rowid FROM t WHERE ... LIMIT 1000)).
P3 — dead enum values in EXTERNAL_EVENT_TERMINAL_STATES.
The array still lists 'delivery_failed' and 'ambiguous', but migration 124 collapsed space_external_events.state to 4 values (published, delivered, failed, ignored) and migrated those two away — they now match no rows under the CHECK constraint. Harmless, but the code reads as if it targets the pre-M124 schema. Trim to ['delivered', 'failed', 'ignored'].
Minor note (not a blocker): migration 170's VACUUM path is only exercised on an in-memory (rollback-journal) DB; production runs it against a WAL file. VACUUM is WAL-compatible, but the production path is untested. Also, if VACUUM fails mid-run (e.g. <2× free disk), runMarkedMigration propagates the throw → daemon boot fails every restart until freed. The 2× disk requirement is documented; consider catch-and-skip so a mis-set maintenance flag can't take the daemon down.
…m, VACUUM boot-safety [#2339] - P2: add covering indexes (mcp_audit_log(timestamp), space_github_events(state, updated_at), space_goal_events(created_at)) via migration 171 so the retention sweep is an indexed range scan, not a full scan under a write lock; correct the prune() doc comment. - P3: trim EXTERNAL_EVENT_TERMINAL_STATES to delivered/failed/ignored — M124 collapsed the enum and migrated delivery_failed/ambiguous away. - Non-blocking: wrap the M170 VACUUM registration in try/catch so a failed VACUUM (e.g. <2x free disk) can't take the daemon down; left unmarked so it retries next boot.
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…ty [#2339] CI's DB schema parity check (check-db-schema-parity) compares the fully-migrated production schema against tests/unit/helpers/space-test-db.ts. Migration 171 added covering indexes to production; mirror mcp_audit_log(timestamp) and space_goal_events(created_at) here so parity passes. (space_github_events isn't in the helper, so its index needs no mirror.)
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: REQUEST_CHANGES (COMMENT only — self-review, same GitHub account.)
Round 2 — the previous P2/P3 fixes are verified correct:
- P3 ✓
EXTERNAL_EVENT_TERMINAL_STATEStrimmed to['delivered','failed','ignored']. Good call leaving'ambiguous'inGITHUB_EVENT_TERMINAL_STATES—space_github_events.statewas never collapsed by M124, so its CHECK still allows it. - P2 ✓ Migration 171 adds
mcp_audit_log(timestamp),space_github_events(state, updated_at),space_goal_events(created_at). I bootstrapped a fresh DB viaDatabaseCore.initialize()and confirmed empirically that all three indexes land on both fresh and existing DBs. Theprune()comment is corrected. 462 migration tests pass; targeted tests pass. - VACUUM boot-safety ✓ The try/catch around the M170 registration is correct —
runMarkedMigrationskipsmarkMigrationwhenmigration()throws, so a failed VACUUM stays unmarked and retries next boot without taking the daemon down.
P1 — CI is red because a required file change is uncommitted.
The fix commit 5fd65947 added migration 171 but did not include the matching change to the test helper. Three CI checks failed on this commit — Lint, Knip, Format & Type Check (DB schema parity: failure), Daemon Unit Tests (0-shared-handlers-workflow) (schema-parity-check.test.ts), and the aggregate All Tests Pass. The schema-parity-check.test.ts diff is explicit:
space-test-db helper index mismatch for mcp_audit_log: Missing indexes
space-test-db helper index mismatch for space_goal_events: Missing indexes
Root cause: packages/daemon/tests/unit/helpers/space-test-db.ts has an uncommitted edit (present in this shared worktree) that adds exactly those two indexes to createSpaceTables:
db.exec(`CREATE INDEX IF NOT EXISTS idx_mcp_audit_log_timestamp ON mcp_audit_log (timestamp)`);
...
db.exec(`CREATE INDEX IF NOT EXISTS idx_space_goal_events_created ON space_goal_events(created_at)`);With that edit the local parity test passes (2/2); without it (the pushed commit) CI fails. The change is already written — it just needs git add + commit + push. (space_github_events is correctly absent from the helper — it doesn't create that table, so no third index is needed there.)
This is the only blocker. Once the helper is committed, all three checks should go green.
Non-blocking note. Migration 171 runs ungated at first boot after upgrade, and CREATE INDEX on mcp_audit_log (one of the largest tables on the 15GB DB) will hold a write lock for the duration of the build — a one-time startup cost that could surprise an operator. It's strictly cheaper than VACUUM and is a one-off, so not gating it is defensible, but worth being aware of for the production rollout (the same "schedule deliberately" reasoning that gated M170 applies here, just smaller).
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE (COMMENT only — self-review, same GitHub account; GitHub rejects APPROVE.)
Round 3 — all findings resolved, zero open issues.
- P1 (CI red / parity) ✓ Fixed in
69a42f49— thespace-test-db.tshelper now mirrorsidx_mcp_audit_log_timestampandidx_space_goal_events_createdincreateSpaceTables. CI on69a42f49is fully green: all three previously-failing checks pass (Lint/Knip/Format/Type Check withdb-schema-parity, Daemon Unit Tests (0-shared-handlers-workflow) withschema-parity-check.test.ts, and the aggregate All Tests Pass) — 32 success, 0 fail/pending. - P2 (retention scans) ✓ Migration 171 covering indexes verified present on both fresh and existing DBs (bootstrapped via
DatabaseCore.initialize()empirically);prune()comment corrected. - P3 (dead enum) ✓ Trimmed to
['delivered','failed','ignored'];'ambiguous'correctly retained in the github-events terminal list (its CHECK was never collapsed). - VACUUM boot-safety ✓ try/catch correctly leaves M170 unmarked on failure → retries next boot.
On the M171-gating note — I concede, ungated is correct. CREATE INDEX is materially different from VACUUM (one table scan, no 2× disk, can't brick the DB), and ungated index migrations are established precedent on this DB (M168/M169/M164/M165). Gating would defeat the purpose. Withdrawn.
Verified locally: targeted tests pass (retention/cleanup/db-core/M170/M171), full migration suite 462/462, parity test + script pass, oxlint/tsc/kip green. Both review threads resolved; PR is open and mergeable. Ship it.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What
Extends the daily
job_queue.cleanuphandler with configurable retention sweeps and incremental page reclamation, addressing the monotonic DB growth root cause in #2339.HYPERNEO_RETENTION_ENABLED=1): prune terminal-statespace_external_events/deliveries,space_github_events,mcp_audit_log, andspace_goal_eventsolder than their TTL. Each TTL is independently configurable; in-flight states are always kept.incremental_vacuum(500)runs every cleanup cycle to reclaim pages freed by retention + the existing 7-day job_queue/worktree reapers.auto_vacuum = INCREMENTALon fresh DBs (DatabaseCore); existingauto_vacuum=NONEDBs convert via opt-in migration 170 (HYPERNEO_DB_VACUUM_MIGRATION) — gated because VACUUM on a multi-GB DB is long and should be scheduled deliberately.sdk_messagesretention was intentionally deferred —is_terminalonly marksresultmessages, so "prune non-terminal" would delete entire conversations; flagged for a separate policy decision.Verify
packages/daemon/src/lib/job-handlers/retention.ts+ tests; migration-170 + db-coreauto_vacuumtests.