From 41f48e93db4df5a97241c1001d8eabb559674b35 Mon Sep 17 00:00:00 2001 From: imkp1 Date: Sat, 8 Aug 2026 15:26:26 +0530 Subject: [PATCH] fix(tracking): reclaim history.db pages after retention cleanup cleanup_old() deleted rows past the 90-day window but nothing returned the freed pages, so history.db stayed at its high-water mark for good. Retention bounded the row count, not the file size. New databases are now created with auto_vacuum=INCREMENTAL and give the pages back after every cleanup that removed rows. A database created before this change is converted once, by the first cleanup that frees pages, and reclaims its accumulated slack at the same time. Reclamation is best-effort: a database that cannot be shrunk right now must never fail the command being recorded. --- src/core/tracking.rs | 203 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 193 insertions(+), 10 deletions(-) diff --git a/src/core/tracking.rs b/src/core/tracking.rs index 59069018d6..389042cb28 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -33,8 +33,9 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use rusqlite::{params, Connection}; use serde::Serialize; +use std::cell::Cell; use std::ffi::OsString; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::time::Instant; // ── Project path helpers ── // added: project-scoped tracking support @@ -90,6 +91,9 @@ use super::constants::{DEFAULT_HISTORY_DAYS, HISTORY_DB, RTK_DATA_DIR}; /// ``` pub struct Tracker { conn: Connection, + /// Set when the database was created before incremental auto_vacuum was + /// enabled, so the next cleanup that frees pages converts it with a VACUUM. + needs_full_vacuum: Cell, } /// Individual command record from tracking history. @@ -247,7 +251,11 @@ impl Tracker { /// # Ok::<(), anyhow::Error>(()) /// ``` pub fn new() -> Result { - let db_path = get_db_path()?; + Self::open(&get_db_path()?) + } + + /// Open a tracker on a specific database path. + fn open(db_path: &Path) -> Result { if let Some(parent) = db_path.parent() { crate::core::utils::create_private_dir(parent)?; } @@ -256,7 +264,7 @@ impl Tracker { // an already-private DB instead of the umask. crate::core::utils::open_private( std::fs::OpenOptions::new().write(true).create(true), - &db_path, + db_path, ) .with_context(|| { format!( @@ -265,11 +273,21 @@ impl Tracker { ) })?; - let conn = Connection::open(&db_path)?; + let conn = Connection::open(db_path)?; + // A database that already holds pages but was created without + // auto_vacuum predates incremental reclaim. Read this before the PRAGMA + // below, which reports INCREMENTAL as soon as it is set even though the + // file is only converted by a VACUUM. + let needs_full_vacuum = Cell::new(is_legacy_non_auto_vacuum(&conn)); + // auto_vacuum goes first: switching to WAL writes the header, and after + // that a new database can no longer change its vacuum mode. Setting it + // here makes a new database incremental from birth; on an existing one it + // only takes effect once a VACUUM converts the file. // WAL mode + busy_timeout for concurrent access (multiple Claude Code instances). // Non-fatal: NFS/read-only filesystems may not support WAL. let _ = conn.execute_batch( - "PRAGMA journal_mode=WAL; + "PRAGMA auto_vacuum=INCREMENTAL; + PRAGMA journal_mode=WAL; PRAGMA busy_timeout=5000;", ); conn.execute( @@ -336,16 +354,22 @@ impl Tracker { [], )?; - restrict_db_files(&db_path); + restrict_db_files(db_path); - Ok(Self { conn }) + Ok(Self { + conn, + needs_full_vacuum, + }) } /// Create an isolated in-memory tracker for tests. #[cfg(test)] pub fn new_in_memory() -> Result { let conn = Connection::open_in_memory().context("Failed to open in-memory DB")?; - let tracker = Self { conn }; + let tracker = Self { + conn, + needs_full_vacuum: Cell::new(false), + }; tracker.init_schema()?; Ok(tracker) } @@ -453,17 +477,46 @@ impl Tracker { fn cleanup_old(&self) -> Result<()> { let cutoff = Utc::now() - chrono::Duration::days(DEFAULT_HISTORY_DAYS); - self.conn.execute( + let mut deleted = self.conn.execute( "DELETE FROM commands WHERE timestamp < ?1", params![cutoff.to_rfc3339()], )?; - self.conn.execute( + deleted += self.conn.execute( "DELETE FROM parse_failures WHERE timestamp < ?1", params![cutoff.to_rfc3339()], )?; + if deleted > 0 { + self.reclaim_free_pages(); + } Ok(()) } + /// Hand the pages freed by a delete back to the filesystem. + /// + /// Deleting rows only moves their pages to the freelist, so without this the + /// database keeps its high-water mark for good. Best-effort by design: a + /// database that cannot be shrunk right now must never fail the command that + /// was being recorded. + fn reclaim_free_pages(&self) { + if self.needs_full_vacuum.get() { + // The database predates auto_vacuum. `open` already requested + // INCREMENTAL, so this VACUUM both reclaims the accumulated pages and + // converts the file, which is why it only ever runs once. + if self.conn.execute_batch("VACUUM;").is_ok() { + self.needs_full_vacuum.set(false); + } + return; + } + // incremental_vacuum frees pages one step at a time and reports success + // either way, so it has to be driven to completion rather than run as a + // batch, which would return Ok having reclaimed nothing. + if let Ok(mut stmt) = self.conn.prepare("PRAGMA incremental_vacuum") { + if let Ok(mut rows) = stmt.query([]) { + while matches!(rows.next(), Ok(Some(_))) {} + } + } + } + /// Delete all tracked data (commands + parse_failures), resetting all stats to zero. pub fn reset_all(&self) -> Result<()> { self.conn @@ -474,6 +527,7 @@ impl Tracker { COMMIT;", ) .context("Failed to reset tracking database")?; + self.reclaim_free_pages(); Ok(()) } @@ -1232,6 +1286,23 @@ fn categorize_command(rtk_cmd: &str) -> String { .to_string() } +/// Whether the file behind this connection was created before incremental +/// auto_vacuum was enabled. +/// +/// A brand-new database has no pages yet and reports `auto_vacuum = 0` too, so +/// the page count is what separates the two. Both pragmas are read before +/// `open` sets `auto_vacuum`, which starts reporting INCREMENTAL immediately +/// even though the file itself is only converted by a VACUUM. +fn is_legacy_non_auto_vacuum(conn: &Connection) -> bool { + let pages: i64 = conn + .query_row("PRAGMA page_count", [], |row| row.get(0)) + .unwrap_or(0); + let auto_vacuum: i64 = conn + .query_row("PRAGMA auto_vacuum", [], |row| row.get(0)) + .unwrap_or(0); + pages > 0 && auto_vacuum == 0 +} + /// SQLite appends `-wal`/`-shm` to the whole filename, so these are siblings /// rather than extension swaps. Concatenate on `OsString`, not `PathBuf::push`, /// which would append a component and silently target `history.db/-wal`. @@ -1757,4 +1828,116 @@ mod tests { assert_eq!(mode, 0o600, "expected 0600 on {}", p.display()); } } + + // Space reclaim after retention cleanup. + + fn pragma(conn: &Connection, name: &str) -> i64 { + conn.query_row(&format!("PRAGMA {}", name), [], |row| row.get(0)) + .unwrap_or_else(|e| panic!("PRAGMA {} failed: {}", name, e)) + } + + /// Insert rows dated past the retention window, so the next cleanup deletes + /// them. The payload is what gives the database enough pages for the freed + /// space to be visible. + fn insert_expired_rows(conn: &Connection, count: usize) { + let stale = (Utc::now() - chrono::Duration::days(DEFAULT_HISTORY_DAYS + 1)).to_rfc3339(); + let payload = "x".repeat(4096); + let tx = conn.unchecked_transaction().expect("transaction"); + for _ in 0..count { + tx.execute( + "INSERT INTO commands (timestamp, original_cmd, rtk_cmd, project_path, input_tokens, output_tokens, saved_tokens, savings_pct, exec_time_ms) + VALUES (?1, ?2, ?2, '', 100, 20, 80, 80.0, 1)", + params![stale, payload], + ) + .expect("insert expired row"); + } + tx.commit().expect("commit"); + } + + #[test] + fn test_new_database_uses_incremental_auto_vacuum() { + let tmp = tempfile::tempdir().expect("tempdir"); + let tracker = Tracker::open(&tmp.path().join("history.db")).expect("open tracker"); + + assert_eq!( + pragma(&tracker.conn, "auto_vacuum"), + 2, + "a new database should be created with auto_vacuum=INCREMENTAL" + ); + } + + #[test] + fn test_cleanup_reclaims_free_pages() { + let tmp = tempfile::tempdir().expect("tempdir"); + let tracker = Tracker::open(&tmp.path().join("history.db")).expect("open tracker"); + + insert_expired_rows(&tracker.conn, 200); + let pages_before = pragma(&tracker.conn, "page_count"); + + tracker.cleanup_old().expect("cleanup"); + + assert_eq!( + pragma(&tracker.conn, "freelist_count"), + 0, + "cleanup should hand the pages it freed back, not park them on the freelist" + ); + assert!( + pragma(&tracker.conn, "page_count") < pages_before, + "page count should drop once expired rows are removed" + ); + } + + #[test] + fn test_legacy_database_is_converted_on_cleanup() { + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("history.db"); + + // A database as it looked before this change: rows past the retention + // window and no auto_vacuum. + { + let legacy = Tracker::open(&db_path).expect("open tracker"); + legacy + .conn + .execute_batch("PRAGMA auto_vacuum=NONE; VACUUM;") + .expect("drop auto_vacuum"); + insert_expired_rows(&legacy.conn, 200); + assert_eq!(pragma(&legacy.conn, "auto_vacuum"), 0); + } + + let tracker = Tracker::open(&db_path).expect("reopen tracker"); + let pages_before = pragma(&tracker.conn, "page_count"); + + tracker.cleanup_old().expect("cleanup"); + + assert_eq!( + pragma(&tracker.conn, "auto_vacuum"), + 2, + "the one-time vacuum should also convert the file to incremental" + ); + assert!( + pragma(&tracker.conn, "page_count") < pages_before, + "an existing database should give back its accumulated pages" + ); + assert!( + !tracker.needs_full_vacuum.get(), + "the conversion should not be repeated on later cleanups" + ); + } + + #[test] + fn test_reset_all_reclaims_free_pages() { + let tmp = tempfile::tempdir().expect("tempdir"); + let tracker = Tracker::open(&tmp.path().join("history.db")).expect("open tracker"); + + insert_expired_rows(&tracker.conn, 200); + let pages_before = pragma(&tracker.conn, "page_count"); + + tracker.reset_all().expect("reset"); + + assert_eq!(pragma(&tracker.conn, "freelist_count"), 0); + assert!( + pragma(&tracker.conn, "page_count") < pages_before, + "resetting stats to zero should also return the space" + ); + } }