From b5b48918adf58f9cb9bbee35a08142d7f005b202 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 4 Aug 2026 20:34:13 -0400 Subject: [PATCH 1/2] fix(tracking): honor tracking.history_days instead of the hardcoded default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `history_days` was declared on TrackingConfig, given a default, written into the generated config.toml and documented in src/core/README.md — but never read. `cleanup_old` used the DEFAULT_HISTORY_DAYS constant directly, so the retention window was fixed at 90 days and the setting was inert. Resolve the window once at construction and store it on Tracker rather than loading the config inside cleanup_old, which runs on every record() and record_parse_failure() and must not touch the filesystem per call. A non-positive value falls back to the default instead of being honored: a typo'd 0 would otherwise delete the entire history on the next write. The same fallback covers an unparseable config, including a [tracking] section that omits history_days — the field has no serde default, so a partial section fails the whole parse. The pruning test is asserted in both directions on purpose. A hardcoded 90 still passes the "retained within window" half, so only the "pruned beyond window" half catches the regression, and only the pair shows the field is what drives the cutoff. --- src/core/tracking.rs | 147 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 144 insertions(+), 3 deletions(-) diff --git a/src/core/tracking.rs b/src/core/tracking.rs index 59069018d6..138935f0a9 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -90,6 +90,9 @@ use super::constants::{DEFAULT_HISTORY_DAYS, HISTORY_DB, RTK_DATA_DIR}; /// ``` pub struct Tracker { conn: Connection, + /// Retention window in days, resolved once at construction. `cleanup_old` runs on + /// every record, so this must not re-read the config file per call. + history_days: i64, } /// Individual command record from tracking history. @@ -338,14 +341,20 @@ impl Tracker { restrict_db_files(&db_path); - Ok(Self { conn }) + Ok(Self { + conn, + history_days: configured_history_days(), + }) } /// 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, + history_days: configured_history_days(), + }; tracker.init_schema()?; Ok(tracker) } @@ -452,7 +461,7 @@ impl Tracker { } fn cleanup_old(&self) -> Result<()> { - let cutoff = Utc::now() - chrono::Duration::days(DEFAULT_HISTORY_DAYS); + let cutoff = Utc::now() - chrono::Duration::days(self.history_days); self.conn.execute( "DELETE FROM commands WHERE timestamp < ?1", params![cutoff.to_rfc3339()], @@ -1253,6 +1262,29 @@ fn db_sidecars(db_path: &std::path::Path) -> Vec { .collect() } +/// Retention window for `cleanup_old`, from `tracking.history_days`. +/// +/// Falls back to the default on any config problem, including a `[tracking]` section +/// that omits `history_days` (the field has no serde default, so a partial section +/// fails the whole parse). A non-positive value also falls back rather than being +/// honored: a typo'd `0` would otherwise delete the entire history on the next write. +fn configured_history_days() -> i64 { + resolve_history_days( + crate::core::config::Config::load() + .ok() + .map(|c| c.tracking.history_days), + ) +} + +/// Clamping half of [`configured_history_days`], split out so it is testable without +/// touching the real config file. +fn resolve_history_days(configured: Option) -> i64 { + configured + .map(i64::from) + .filter(|d| *d > 0) + .unwrap_or(DEFAULT_HISTORY_DAYS) +} + pub(crate) fn get_db_path() -> Result { // Priority 1: Environment variable RTK_DB_PATH if let Ok(custom_path) = std::env::var("RTK_DB_PATH") { @@ -1683,6 +1715,115 @@ mod tests { assert!(summary.recovery_rate >= 0.0 && summary.recovery_rate <= 100.0); } + /// Backdated tracker: one `commands` and one `parse_failures` row 10 days old, + /// with an explicit retention window. + #[cfg(test)] + fn tracker_with_10_day_old_rows(history_days: i64) -> Tracker { + let mut t = Tracker::new_in_memory().expect("in-memory tracker"); + t.history_days = history_days; + let old = (Utc::now() - chrono::Duration::days(10)).to_rfc3339(); + t.conn + .execute( + "INSERT INTO commands + (timestamp, original_cmd, rtk_cmd, input_tokens, output_tokens, + saved_tokens, savings_pct, exec_time_ms, project_path) + VALUES (?1, 'old cmd', 'rtk old cmd', 100, 20, 80, 80.0, 0, '')", + params![old], + ) + .expect("insert backdated command"); + t.conn + .execute( + "INSERT INTO parse_failures + (timestamp, raw_command, error_message, fallback_succeeded) + VALUES (?1, 'old raw', 'old err', 1)", + params![old], + ) + .expect("insert backdated parse failure"); + t + } + + // Regression: cleanup_old hardcoded DEFAULT_HISTORY_DAYS, so tracking.history_days + // was declared, defaulted, written into config.toml, documented — and never read. + // Both directions are asserted deliberately: a hardcoded 90 still passes the + // "retained" half, so only the "pruned" half catches the regression, and only the + // pair proves the field is what drives the cutoff. + #[test] + fn test_cleanup_old_prunes_beyond_configured_history_days() { + let t = tracker_with_10_day_old_rows(3); + + t.record("git status", "rtk git status fresh", 100, 20, 5) + .expect("record triggers cleanup_old"); + + let stale: i64 = t + .conn + .query_row( + "SELECT COUNT(*) FROM commands WHERE rtk_cmd = 'rtk old cmd'", + [], + |r| r.get(0), + ) + .expect("count commands"); + assert_eq!( + stale, 0, + "10-day-old command should be pruned at 3-day retention" + ); + + let stale_pf: i64 = t + .conn + .query_row( + "SELECT COUNT(*) FROM parse_failures WHERE raw_command = 'old raw'", + [], + |r| r.get(0), + ) + .expect("count parse_failures"); + assert_eq!( + stale_pf, 0, + "10-day-old parse failure should be pruned at 3-day retention" + ); + } + + #[test] + fn test_cleanup_old_retains_within_configured_history_days() { + let t = tracker_with_10_day_old_rows(30); + + t.record("git status", "rtk git status fresh", 100, 20, 5) + .expect("record triggers cleanup_old"); + + let stale: i64 = t + .conn + .query_row( + "SELECT COUNT(*) FROM commands WHERE rtk_cmd = 'rtk old cmd'", + [], + |r| r.get(0), + ) + .expect("count commands"); + assert_eq!( + stale, 1, + "10-day-old command should survive 30-day retention" + ); + } + + #[test] + fn test_resolve_history_days() { + assert_eq!( + resolve_history_days(Some(30)), + 30, + "honors a configured value" + ); + // A missing/unparseable config (including a [tracking] section that omits + // history_days, which fails the whole parse) must not change retention. + assert_eq!( + resolve_history_days(None), + DEFAULT_HISTORY_DAYS, + "absent config falls back to the default" + ); + // A typo'd 0 would otherwise delete the entire history on the next write. + assert_eq!( + resolve_history_days(Some(0)), + DEFAULT_HISTORY_DAYS, + "zero falls back rather than wiping history" + ); + } + #[test] fn test_reset_all_clears_both_tables() { let tracker = Tracker::new_in_memory().expect("Failed to create in-memory tracker"); From 79635f9446524f02cfec531c9b09c84a4a02a686 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 4 Aug 2026 20:45:24 -0400 Subject: [PATCH 2/2] fix(tracking): return pruned space to the filesystem These databases have no auto_vacuum, so retention only marks pages reusable and the file never shrinks. Shortening history_days deletes millions of rows and leaves the file at its old size indefinitely: on one box, dropping 90 to 30 pruned 3.09M rows and left 1.9GB on disk with 445,466 free pages. cleanup_old now counts what it deleted and, when anything went, vacuums if enough of the file is free pages. VACUUM cannot run unconditionally here. cleanup_old runs once per tracked command, and VACUUM rewrites the whole database under an exclusive lock, which would both blow the startup budget and stall concurrent rtk processes sharing the DB. Gating on the free-page ratio keeps it dormant in steady state, where inserts reuse freed pages, and lets it fire on the bulk deletes that actually strand space. Measured on a 196MB database: 20 consecutive tracked commands leave the freelist at 1-4 pages of 50,067, so the gate stays closed and page_count is unchanged. Injecting 300k out-of-window rows takes it to 320MB, and the next tracked command prunes and reclaims back to 196MB with quick_check ok. Failure is ignored. VACUUM needs scratch space near the size of the live data and cannot run inside a transaction; neither is a reason to fail a user's command. --- src/core/tracking.rs | 137 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 131 insertions(+), 6 deletions(-) diff --git a/src/core/tracking.rs b/src/core/tracking.rs index 138935f0a9..81f3074096 100644 --- a/src/core/tracking.rs +++ b/src/core/tracking.rs @@ -462,17 +462,50 @@ impl Tracker { fn cleanup_old(&self) -> Result<()> { let cutoff = Utc::now() - chrono::Duration::days(self.history_days); - self.conn.execute( - "DELETE FROM commands WHERE timestamp < ?1", - params![cutoff.to_rfc3339()], - )?; - self.conn.execute( + let cutoff = cutoff.to_rfc3339(); + let mut deleted = self + .conn + .execute("DELETE FROM commands WHERE timestamp < ?1", params![cutoff])?; + deleted += self.conn.execute( "DELETE FROM parse_failures WHERE timestamp < ?1", - params![cutoff.to_rfc3339()], + params![cutoff], )?; + if deleted > 0 { + self.reclaim_if_fragmented(); + } Ok(()) } + /// Return freed pages to the filesystem when enough of the file is empty. + /// + /// These databases have no `auto_vacuum`, so a delete only marks pages reusable: + /// the file never shrinks on its own. Shortening `history_days` therefore prunes + /// millions of rows and leaves the file at its old size until something vacuums. + /// + /// `VACUUM` rewrites the whole database and takes an exclusive lock, so it must not + /// run on every `cleanup_old` (that is once per tracked command). Gating on the free + /// ratio keeps it near-free in steady state, where inserts reuse freed pages and the + /// freelist stays flat, and lets it fire on the bulk deletes that actually strand + /// space. Failure is ignored: reclaiming disk must never fail a user's command. + fn reclaim_if_fragmented(&self) { + let free: i64 = match self + .conn + .query_row("PRAGMA freelist_count", [], |r| r.get(0)) + { + Ok(v) => v, + Err(_) => return, + }; + let total: i64 = match self.conn.query_row("PRAGMA page_count", [], |r| r.get(0)) { + Ok(v) => v, + Err(_) => return, + }; + if should_reclaim(free, total) { + // Cannot run inside a transaction, and needs scratch space roughly the size + // of the live data. Both are why this stays best-effort. + let _ = self.conn.execute_batch("VACUUM;"); + } + } + /// Delete all tracked data (commands + parse_failures), resetting all stats to zero. pub fn reset_all(&self) -> Result<()> { self.conn @@ -1276,6 +1309,21 @@ fn configured_history_days() -> i64 { ) } +/// Free pages below this never justify a rewrite, whatever the ratio. At SQLite's 4 KiB +/// default this is ~16 MB, so small databases are left alone. +const VACUUM_MIN_FREE_PAGES: i64 = 4096; +/// Percentage of the file that must be free pages before a rewrite pays for itself. +const VACUUM_MIN_FREE_PCT: i64 = 25; + +/// Decision half of [`Tracker::reclaim_if_fragmented`], split out so the thresholds are +/// testable without building a database large enough to cross them. +fn should_reclaim(free_pages: i64, total_pages: i64) -> bool { + if total_pages <= 0 || free_pages < VACUUM_MIN_FREE_PAGES { + return false; + } + free_pages.saturating_mul(100) / total_pages >= VACUUM_MIN_FREE_PCT +} + /// Clamping half of [`configured_history_days`], split out so it is testable without /// touching the real config file. fn resolve_history_days(configured: Option) -> i64 { @@ -1802,6 +1850,83 @@ mod tests { ); } + #[test] + fn test_should_reclaim_thresholds() { + // Steady state: inserts reuse freed pages, so the freelist stays flat and a + // per-command VACUUM would be pure waste. + assert!(!should_reclaim(0, 100_000), "no free pages"); + assert!( + !should_reclaim(VACUUM_MIN_FREE_PAGES - 1, VACUUM_MIN_FREE_PAGES), + "tiny database, high ratio: not worth a rewrite" + ); + assert!( + !should_reclaim(VACUUM_MIN_FREE_PAGES * 2, 1_000_000), + "plenty of free pages but a small share of the file" + ); + // The bulk-delete case this exists for: shortening history_days strands most + // of the file. + assert!( + should_reclaim(450_000, 500_000), + "90% free after a retention drop should reclaim" + ); + assert!(should_reclaim( + VACUUM_MIN_FREE_PAGES, + VACUUM_MIN_FREE_PAGES * 4 + )); + // Guards against a divide-by-zero on an empty or unreadable database. + assert!(!should_reclaim(10, 0), "zero pages must not divide"); + } + + #[test] + fn test_vacuum_returns_space_after_bulk_delete() { + let t = Tracker::new_in_memory().expect("in-memory tracker"); + let old = (Utc::now() - chrono::Duration::days(400)).to_rfc3339(); + for i in 0..4000 { + t.conn + .execute( + "INSERT INTO commands + (timestamp, original_cmd, rtk_cmd, input_tokens, output_tokens, + saved_tokens, savings_pct, exec_time_ms, project_path) + VALUES (?1, ?2, ?2, 100, 20, 80, 80.0, 0, '')", + params![old, format!("padding row {} {}", i, "x".repeat(400))], + ) + .expect("insert bulk row"); + } + let before: i64 = t + .conn + .query_row("PRAGMA page_count", [], |r| r.get(0)) + .expect("page_count"); + + t.conn + .execute("DELETE FROM commands", []) + .expect("bulk delete"); + let freed: i64 = t + .conn + .query_row("PRAGMA freelist_count", [], |r| r.get(0)) + .expect("freelist_count"); + assert!( + freed > 0, + "delete alone should strand pages, not return them" + ); + + t.conn.execute_batch("VACUUM;").expect("vacuum"); + let after: i64 = t + .conn + .query_row("PRAGMA page_count", [], |r| r.get(0)) + .expect("page_count"); + assert!( + after < before, + "VACUUM should shrink the database ({} -> {} pages)", + before, + after + ); + let leftover: i64 = t + .conn + .query_row("PRAGMA freelist_count", [], |r| r.get(0)) + .expect("freelist_count"); + assert_eq!(leftover, 0, "VACUUM should leave no free pages"); + } + #[test] fn test_resolve_history_days() { assert_eq!(