From bc1875280e475b8ef878e76c11939d0c8b34a16f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 14:39:28 +0000 Subject: [PATCH] osm lance: remove a stale dataset before reconversion, never overwrite in place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap the fragment-split fix (#124) left open: WriteMode::Overwrite only writes a new manifest VERSION — Lance keeps the previous version's fragment files on disk. Reconverting the pre-fix three-fragment Berlin dataset in place would therefore leave data/ holding FOUR .lance files, and locate_row_column's sole-data-file precondition would fall back to the raw .soa forever — the map would render, but Lance serving would silently never engage, with 1.3 GB of dead fragments squatting on volume01. The dataset is a derived cache of the slab; version history has no value and the slab itself is the recovery path. So a stale dataset is now removed whole (remove_stale_dataset) before write_lance runs, fail-closed: if the removal fails, nothing is written into the half-cleared directory (exactly how a 4-file data/ would happen) and the .soa keeps serving unchanged. Bonus: peak disk during reconversion drops from .soa + old + new (~3.9 GB) to .soa + new (~2.6 GB). Falsifier: remove_stale_dataset_clears_old_fragments_entirely lays out the pre-fix three-fragment shape plus Lance bookkeeping dirs and asserts the directory is GONE. Tests: osm_lance 11/11 passing; cargo check clean. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw --- crates/cockpit-server/src/osm_lance.rs | 73 ++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/crates/cockpit-server/src/osm_lance.rs b/crates/cockpit-server/src/osm_lance.rs index bc7129b2e9..eb3cb21b59 100644 --- a/crates/cockpit-server/src/osm_lance.rs +++ b/crates/cockpit-server/src/osm_lance.rs @@ -133,6 +133,29 @@ pub async fn ensure_lance_local(slab_path: &Path) -> Option { return Some(warm); } + // A stale dataset is REMOVED before reconversion, not overwritten in + // place. `WriteMode::Overwrite` only writes a new manifest VERSION — + // Lance keeps the previous version's fragment files on disk, so + // overwriting the pre-fix three-fragment Berlin dataset would leave + // `data/` holding 4 `.lance` files, and [`locate_row_column`]'s + // sole-data-file precondition would then fall back to the raw `.soa` + // forever (plus 1.3 GB of dead fragments squatting on the volume). + // The dataset is a derived cache of the slab — version history has no + // value here; the slab itself is the recovery path. Fail-closed: if + // the stale directory cannot be removed, do NOT write into it (that + // is exactly how a 4-file `data/` happens) — keep serving from the + // `.soa` unchanged. + if dest.exists() { + if let Err(e) = remove_stale_dataset(&dest) { + tracing::error!( + path = %dest.display(), error = %e, + "osm lance: cannot remove the stale dataset; refusing to write \ + into it — the raw .soa slab keeps serving the map" + ); + return None; + } + } + // Cold path only: the write genuinely needs an owned Vec (adopted by // `Buffer::from_vec`), so the slab-sized allocation is paid exactly once // per conversion, never per boot. @@ -150,6 +173,19 @@ pub async fn ensure_lance_local(slab_path: &Path) -> Option { write_lance(&dest, bytes, rows, &digest_hex, slab_path).await } +/// Remove a stale dataset directory in full, so the follow-up write starts +/// from nothing and `data/` ends up holding EXACTLY the one fragment file +/// the write produces. Split out for the falsifier below; guarded at the +/// call site by `dest.exists()`. +fn remove_stale_dataset(dest: &Path) -> std::io::Result<()> { + tracing::info!( + path = %dest.display(), + "osm lance: removing the stale dataset before reconversion \ + (version history has no value for a derived cache)" + ); + std::fs::remove_dir_all(dest) +} + /// `Some(dest)` iff a dataset already exists at `dest` with exactly `rows` /// rows and a `soa:slab_digest` header matching `digest_hex` — the ONLY two /// facts that must hold before skipping the write. Any other outcome @@ -200,10 +236,14 @@ async fn reopen_if_warm(dest: &Path, rows: usize, digest_hex: &str) -> Option, @@ -478,6 +518,31 @@ mod tests { assert!(search_head_for_probe(&file_path, &probe).is_none()); } + /// A stale dataset is removed WHOLE — including the prior version's + /// fragment files that `WriteMode::Overwrite` would have left behind. + /// This is the falsifier for the 4-files-in-`data/` failure: overwrite + /// the pre-fix three-fragment dataset in place and `locate_row_column`'s + /// sole-data-file check falls back to the raw `.soa` forever. + #[test] + fn remove_stale_dataset_clears_old_fragments_entirely() { + let dir = tempfile::tempdir().expect("tempdir"); + let dataset_dir = dir.path().join("region.lance"); + let data_dir = dataset_dir.join("data"); + std::fs::create_dir_all(&data_dir).expect("mkdir"); + // The pre-fix shape: three fragment files plus Lance's own + // bookkeeping directories. + for name in ["frag-0.lance", "frag-1.lance", "frag-2.lance"] { + std::fs::write(data_dir.join(name), vec![0x77u8; 1024]).expect("write"); + } + std::fs::create_dir_all(dataset_dir.join("_versions")).expect("mkdir"); + + remove_stale_dataset(&dataset_dir).expect("removal succeeds"); + assert!( + !dataset_dir.exists(), + "the stale dataset directory must be GONE, so the rewrite starts from nothing" + ); + } + /// A synthetic slab whose rows are DISTINGUISHABLE — row `i`'s bytes are /// a function of `i` — so a tail anchor genuinely proves the last row is /// where it is claimed to be, rather than matching any repeated filler.