fix(tracking): honor tracking.history_days instead of the hardcoded default

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<Self> {
         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,18 +461,51 @@ impl Tracker {
     }
 
     fn cleanup_old(&self) -> Result<()> {
-        let cutoff = Utc::now() - chrono::Duration::days(DEFAULT_HISTORY_DAYS);
-        self.conn.execute(
-            "DELETE FROM commands WHERE timestamp < ?1",
-            params![cutoff.to_rfc3339()],
-        )?;
-        self.conn.execute(
+        let cutoff = Utc::now() - chrono::Duration::days(self.history_days);
+        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
@@ -1253,6 +1295,44 @@ fn db_sidecars(db_path: &std::path::Path) -> Vec<PathBuf> {
         .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),
+    )
+}
+
+/// 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<u32>) -> i64 {
+    configured
+        .map(i64::from)
+        .filter(|d| *d > 0)
+        .unwrap_or(DEFAULT_HISTORY_DAYS)
+}
+
 pub(crate) fn get_db_path() -> Result<PathBuf> {
     // Priority 1: Environment variable RTK_DB_PATH
     if let Ok(custom_path) = std::env::var("RTK_DB_PATH") {
@@ -1683,6 +1763,192 @@ 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_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!(
+            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");