fix(db): auto-clean conversation_turn_nodes and orphaned conversations - #12548
Conversation
insoln
left a comment
There was a problem hiding this comment.
Review: schema, semantics, pipeline, SQL, tests — verified locally
Ran tests/unit/db-cleanup-conversation-nodes-12453.test.ts (4/4 pass, ~3 s), npm run lint (clean), and reproduced the reported scale from #12453 in a scratch DB with migration 156's exact schema (1,155,364 rows, 758.9 MB). The core fix is correct: both migration-comment claims check out (155 does index agentic_conversations.last_seen_at; 156 has no last_seen_at index), every writer of both tables' last_seen_at uses toISOString() so the string < comparisons are sound, the NOT EXISTS probe is a covering-index seek (idx_turn_nodes_conversation, measured 0.9 s for 36k conversations), and all readers (reconnect walk, tree route, conversation list) already degrade gracefully to the documented anchor-miss path. Scheduler wiring is fine: startCleanupScheduler() runs runAutoCleanup() at startup+6h with autoCleanupEnabled defaulting to true, and the VACUUM in the scheduler does reclaim the disk (measured 758.9 → 71.8 MB).
Blocking
1. Unbatched 1M-row DELETE freezes the whole server for ~36 s (measured).
cleanupConversationTurnNodes → deleteFromTableBefore (src/lib/db/cleanup/usagePurge.ts:50-52) is one synchronous DELETE ... WHERE last_seen_at < ? with SCAN conversation_turn_nodes as the query plan. On a replica of the reporter's table (1,155,364 rows / 758.9 MB, WAL, pragmas as in src/lib/db/core.ts): 1,039,828 rows deleted in 36.0 s with the Node event loop stalled the entire 36.0 s (a 10 ms timer fired zero times during the delete). This runs 30 s after startup, so a first deploy onto the reporter's already-bloated DB is 36 s of total outage — every API request, SSE chunk, and :1276) documents that better-sqlite3 writes park the event loop. Suggested fix: chunk it, e.g. loop /health probe hangs. core.ts's own comment (DELETE FROM conversation_turn_nodes WHERE rowid IN (SELECT rowid FROM conversation_turn_nodes WHERE last_seen_at < ? LIMIT 10000) with setImmediate yields between chunks, summing changes. (A last_seen_at index doesn't help — it's a delete-most-of-table scan either way; batching is the right lever. Note the follow-up VACUUM is another synchronous stall, but that's pre-existing scheduler behavior.)
Should-fix
2. "Clear all" doesn't clear these tables (issue item 3). RESET_TARGETS (src/lib/db/cleanup.ts:695-737) lists 12 tables; conversation_turn_nodes and agentic_conversations are absent. A user with the reported bloat who clicks Reset usage data — All wipes call_logs (the nodes' display-content anchor) but keeps the 775 MB of nodes — doubly dead data that then waits up to 30 days for the retention sweep. The PR body declares this out of scope; I'd argue the "Clear all" path is exactly where it belongs since that's the documented escape hatch for the reported state. Happy to be overruled if a follow-up is planned.
3. CALL_LOG_RETENTION_DAYS does not control this window — the reporter's setting has no effect here. The new functions read getUserDatabaseSettings().retention.callLogs (dashboard DB setting, default 30 days; src/types/databaseSettings.ts:120). The env var is consumed only by the compliance path (src/lib/compliance/index.ts:496) and never reaches getUserDatabaseSettings (zero process.env reads in databaseSettings.ts). #12453's reporter runs CALL_LOG_RETENTION_DAYS=3 and will still get a 30-day node window (~5–6 GB steady-state at their ~190 MB/day) unless they also change the dashboard setting. Pre-existing coupling for call_logs itself, but this PR makes it the only knob for two more tables without documenting it — worth at least a Reviewer Notes line or a docs touch in docs/reference/ENVIRONMENT.md next to the CALL_LOG_RETENTION_DAYS row (issue item 4).
Nits
cleanupAgenticConversations' doc comment credits theNOT EXISTSguard with protecting a freshly-created root, but a root written moments ago haslast_seen_at = nowand can't matchlast_seen_at < cutoffanyway — thelast_seen_atpredicate alone excludes it. The guard is fine; the stated reason isn't quite the real one.- The
tableExistsearly-return incleanupAgenticConversations(cleanup.ts:489-491) is unique new logic with no test — a cheapDROP TABLEsubtest would pin it. - Test nit:
OLD = -(R+10),RECENT = -1silently assumes retention ≥ 2 days.
Everything else — typing of runAutoCleanup results (untyped record, totals include the new keys), scheduler defaults, dbstat-based size display picking the tables up automatically, test isolation (mkdtempSync override beats the harness default) — verified fine.
Verdict: the retention semantics are correct and well-tested; the event-loop freeze in finding 1 is the one thing I'd want addressed before this merges onto a production DB of the reported size.
conversation_turn_nodes and agentic_conversations (migrations 155/156) had no retention path: nothing ever deleted from them, so storage.sqlite grew without bound (1.15M node rows, ~775 MB in four days on one busy coding-agent workload). The nodes are identity-only and resolve their display content from the call_logs row last_correlation_id points at, so once call-log retention purges that row the node is dead weight. Both tables now follow the existing retention.callLogs window inside runAutoCleanup: nodes older than the window are deleted, then agentic_conversations rows past the window that no longer have any node are swept with a NOT EXISTS probe bounded by the indexed last_seen_at column. Closes diegosouzapw#12453
3b7afe0 to
43156d0
Compare
02884ed
into
diegosouzapw:release/v3.8.51
The first copy already batches via DELETE_BATCH_SIZE and cutoffValue. The second copy was stacked on rebase against diegosouzapw#12548 and is a compile error (Identifier already declared). Webpack fails next build. Fixes the production-build break found on deploy/v3.8.51-20260912. Signed-off-by: Minxi Hou <houminxi@gmail.com>
diegosouzapw#12548) Two tables with no retention path at all and 775 MB of a 1.1 GB database is a real operational failure. Tying them to the existing `retention.callLogs` window rather than inventing a knob is right, and the reasoning is what makes it safe: once `cleanupCallLogs` purges the row `last_correlation_id` points at, the node can never render again. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded with the rest of this batch — zero conflicts between the 19 PRs. - `typecheck:core` clean; `check:dashboard-typecheck` OK (206 pre-existing, all within the frozen baseline); `check:changelog-integrity` OK - complexity 2802 / baseline 3218 and cognitive-complexity 1267 / baseline 1437 — both under baseline - 226 of 228 focused assertions green across the batch's 23 test files. The 2 remaining belong to diegosouzapw#12551, which is held separately. Two batch-owned defects were found and fixed in flight, both pure base drift: `173_xp_action_counts.sql` collided with `173_call_logs_video_content_removed.sql` (renumbered to 176 on diegosouzapw#12651 — it aborted every DB open, which is what 53 of the first run's failures were), and the feature-flag catalog was missing the `SERVER_OWNED_TOOL_LOOP_ENABLED` row the base gained after diegosouzapw#12552 was written.⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` reproduce on the pure tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098, which this batch does not touch). Thanks @pacocartones — the `file:line` citations and the explicit out-of-scope notes on every one of these made a 19-PR batch reviewable in one pass.
diegosouzapw#12548) Two tables with no retention path at all and 775 MB of a 1.1 GB database is a real operational failure. Tying them to the existing `retention.callLogs` window rather than inventing a knob is right, and the reasoning is what makes it safe: once `cleanupCallLogs` purges the row `last_correlation_id` points at, the node can never render again. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded with the rest of this batch — zero conflicts between the 19 PRs. - `typecheck:core` clean; `check:dashboard-typecheck` OK (206 pre-existing, all within the frozen baseline); `check:changelog-integrity` OK - complexity 2802 / baseline 3218 and cognitive-complexity 1267 / baseline 1437 — both under baseline - 226 of 228 focused assertions green across the batch's 23 test files. The 2 remaining belong to diegosouzapw#12551, which is held separately. Two batch-owned defects were found and fixed in flight, both pure base drift: `173_xp_action_counts.sql` collided with `173_call_logs_video_content_removed.sql` (renumbered to 176 on diegosouzapw#12651 — it aborted every DB open, which is what 53 of the first run's failures were), and the feature-flag catalog was missing the `SERVER_OWNED_TOOL_LOOP_ENABLED` row the base gained after diegosouzapw#12552 was written.⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` reproduce on the pure tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098, which this batch does not touch). Thanks @pacocartones — the `file:line` citations and the explicit out-of-scope notes on every one of these made a 19-PR batch reviewable in one pass.
Summary
conversation_turn_nodesandagentic_conversations(migrations 155/156, feat(dashboard): agentic conversation tracking — v4, decoupled + storage-architecture concern resolved #10263) had no retention path at all:src/lib/db/cleanup.tsnever referenced either table, there is no cascade FK, and nothing else deletes from them. The reporter's numbers in fix(backend): conversation_turn_nodes / agentic_conversations have no retention — unbounded storage.sqlite growth #12453 (1.15M node rows, ~775 MB of a 1.1 GBstorage.sqliteafter four days, ~190 MB/day) are the same failure class as feat(backend): auto-cleanup/upsert for telemetry tables to bound storage.sqlite #6848 / fix: startup cleanup ignores dashboard data-retention setting (always deletes logs older than 7d) #4354 / fix(db): pre-migration backups are never pruned — db_backups grew to 204 GB / 49k files #10421.call_logsrow thatlast_correlation_idpoints at (156_conversation_turn_nodes.sqlheader). OncecleanupCallLogspurges that row the node can never render again, so both tables now follow the existingretention.callLogswindow (src/types/databaseSettings.ts) instead of getting a knob of their own, as the issue proposes. No new dashboard setting, no new environment variable, anddocs/reference/ENVIRONMENT.mdis untouched because there is nothing new to document there.runAutoCleanup:cleanupConversationTurnNodes()deletes nodes withlast_seen_atbefore the cutoff through the shareddeleteFromTableBeforehelper (src/lib/db/cleanup/usagePurge.ts, so a database that predates migration 156 is a no-op rather than an error);cleanupAgenticConversations()then sweeps roots past the same cutoff that have no remaining node, withWHERE last_seen_at < ? AND NOT EXISTS (SELECT 1 FROM conversation_turn_nodes n WHERE n.conversation_id = agentic_conversations.id). Thelast_seen_atguard uses the index from migration 155 and keeps a root thatcreateConversationwrote moments ago, before the same request inserted its nodes.resolveConversationId. Deliberately out of scope: the "Clear all" /resetUsageHistorypaths (a separate, user-triggered contract), and alast_seen_atindex onconversation_turn_nodes. The node DELETE is a table scan today because migration 156 defined no index on that column; it runs once per cleanup cycle, and I can send the index as a follow-up migration if the maintainer wants it.Related Issues
Validation
tests/unit/db-cleanup-conversation-nodes-12453.test.ts+telemetry-auto-cleanup-6848+db-cleanup+db-cleanup-xp-audit-log23/23,npm run check:db-rulesOK,node scripts/check/check-complexity-ratchets.mjs --base-ref origin/release/v3.8.51OK (0 violations),npm run check:changelog-integrityOK,npm run typecheck:core0 errorsnpm run lintrelease/v3.8.51; focused checks rerun afterwardTests Added Or Updated
tests/unit/db-cleanup-conversation-nodes-12453.test.ts(new, 4 cases, real SQLite adapter in amkdtempDATA_DIR, same pattern astelemetry-auto-cleanup-6848.test.ts): nodes older thanretention.callLogsdeleted and recent ones kept; stale orphan root swept while a stale-but-anchored root and a fresh root without nodes both stay; a chain whose nodes expire is swept in the same pass, and never before its nodes are gone;runAutoCleanupreportsconversationTurnNodesandagenticConversationswith the expected counts. Red on the base (4/4 fail:cleanupConversationTurnNodes is not a function,cleanupAgenticConversations is not a function,conversationTurnNodes missing from results), green with the change (4/4).Coverage Notes
src/lib/db/cleanup.ts:cleanupConversationTurnNodes,cleanupAgenticConversationsand their registration inrunAutoCleanupare exercised end-to-end by the new test file; the existingtests/unit/db-cleanup.test.tsandtelemetry-auto-cleanup-6848.test.tsstay green.Reviewer Notes
cleanup.ts: two new functions inserted beforerunAutoCleanupand two new entries at the end of itsresultsmap. feat(teams): billing cost centers, soft budgets and dashboard #10409 also edits this file, but in different hunks (cleanupMcpAudit,cleanupA2aEvents,resetUsageHistory), so the two should merge cleanly in either order. feat(dashboard): parent-link, genuine-continuation badge, and modal perf fixes #12448 editssrc/lib/db/agenticConversations.ts, which this PR does not touch.retention.callLogs(30 days), conversations idle for longer than that lose their reconnect anchor and start a new conversation id on resume. Operators who set a short call-log retention (the reporter runs 3 days) get the same window for anchors, which is the trade-off the issue asks for.conversation_turn_nodes(nolast_seen_atindex in migration 156). On the reporter's 1.15M-row table that is one pass per cleanup cycle; if that is a concern I will add the index as a follow-up migration rather than widen this PR.