From e259016a005e029f22c8730eacc0e9b6761c2cd9 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Mon, 20 Jul 2026 20:32:18 +0200 Subject: [PATCH] checkpoint the WAL after the vacuum, not before Verified against a live install: vacuum reported "34.3 MiB freed (170.3 MiB -> 136.0 MiB), 0 WAL frame(s) checkpointed" while the WAL grew from 67 MiB to 143 MiB. Net disk use barely moved, and the frame count was zero because the checkpoint ran before the VACUUM, when there was nothing to fold. Reordering is strictly better, but the commit message should not overstate it: the live inflation happens because the daemon's other connections hold read transactions and a TRUNCATE checkpoint waits on readers, so under a busy daemon this can still move little. journal_size_limit stays the backstop -- the live WAL did fall back to its 64 MiB cap on its own afterwards. The accompanying test deliberately does not assert on WAL size or frame count. Measured both ways, a single-connection test reports frames=0 and a 0-byte WAL under either ordering, because SQLite folds the log itself without a concurrent reader to block it; an assertion there would pass on the bug and be decoration. It asserts what is reliably true instead -- that a file-backed vacuum reclaims space and leaves the database sound -- and the ordering rationale plus the residual uncertainty are documented on vacuum() itself. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01JFzNQyb4TWrCRWcnegqWxY --- crates/pdfs-core/src/db/maintenance.rs | 79 ++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 4 deletions(-) diff --git a/crates/pdfs-core/src/db/maintenance.rs b/crates/pdfs-core/src/db/maintenance.rs index 012600d..f62dc97 100644 --- a/crates/pdfs-core/src/db/maintenance.rs +++ b/crates/pdfs-core/src/db/maintenance.rs @@ -154,22 +154,35 @@ impl Db { /// Fold the WAL back into the database and compact the file. /// - /// Checkpointing first keeps the `VACUUM` from having to contend with a - /// large WAL. - /// /// `VACUUM` rewrites the whole database into a fresh file, so it needs room /// for a second copy and takes a write lock for the duration. That makes it /// a deliberate, user-invoked operation rather than something the daemon /// does on a timer. + /// + /// The checkpoint runs *after* the vacuum. Checkpointing only beforehand + /// folds a log that the vacuum is about to rewrite anyway, and leaves the + /// rewrite itself sitting in the WAL: measured on a live install, a 170 MiB + /// database vacuumed to 136 MiB while its WAL grew 67 MiB → 143 MiB, + /// erasing the reclaim in net disk terms and reporting `0 frames` + /// checkpointed because it ran when there was nothing to fold. + /// + /// Ordering it afterwards is strictly better but is *not* a proven fix for + /// that inflation. The live case inflates because the daemon's other + /// connections hold read transactions, and a `TRUNCATE` checkpoint waits on + /// readers — so under a busy daemon this can still return having moved + /// little. `journal_size_limit` remains the backstop that caps the file. + /// Confirming the fix needs a measurement against a live daemon, not a + /// single-connection test, which folds the WAL on its own either way. pub fn vacuum(&self) -> Result { let before = self.stats()?.total_bytes(); - let wal_frames_checkpointed = self.checkpoint_wal()?; { let conn = self.conn.lock(); conn.execute_batch("VACUUM")?; } + let wal_frames_checkpointed = self.checkpoint_wal()?; + let after = self.stats()?.total_bytes(); Ok(VacuumOutcome { before_bytes: before, @@ -260,4 +273,62 @@ mod tests { db.vacuum().unwrap(); assert!(db.integrity_check().unwrap().is_empty()); } + + /// A unique temp directory removed on drop; avoids a dev-dependency, as in + /// [`crate::cache`]'s tests. Needed here because an in-memory database has + /// no WAL file to measure. + struct TempDir(std::path::PathBuf); + impl TempDir { + fn new() -> Self { + use std::sync::atomic::{AtomicU64, Ordering}; + static N: AtomicU64 = AtomicU64::new(0); + let p = std::env::temp_dir().join(format!( + "pdfs-maint-{}-{}", + std::process::id(), + N.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&p).unwrap(); + TempDir(p) + } + } + impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + /// A file-backed vacuum reclaims space and leaves the file sound. + /// + /// Note what this does *not* pin: the WAL-inflation behaviour that motivated + /// the checkpoint ordering in [`Db::vacuum`]. Measured here, both orderings + /// report `frames=0` and leave a 0-byte WAL, because a single-connection + /// test lets SQLite fold the log itself. The live case inflated only because + /// the daemon holds concurrent readers that block a checkpoint, and + /// reproducing that needs a second connection parked in a read transaction. + /// An assertion on frame count or WAL size here would pass under both + /// orderings and so would be decoration — see the note in `vacuum`. + #[test] + fn a_file_backed_vacuum_reclaims_space_and_stays_sound() { + let dir = TempDir::new(); + let path = dir.0.join("cache.db"); + let db = Db::open(&path).unwrap(); + + for i in 0..2000 { + db.set_state_str(&format!("k{i}"), &"v".repeat(256)).unwrap(); + } + let full = db.stats().unwrap().total_bytes(); + for i in 0..2000 { + db.set_state_str(&format!("k{i}"), "").unwrap(); + } + + let outcome = db.vacuum().unwrap(); + + assert!( + outcome.after_bytes < full, + "vacuum did not reclaim: {} → {} (peak {full})", + outcome.before_bytes, + outcome.after_bytes + ); + assert!(db.integrity_check().unwrap().is_empty()); + } }