From 3acb9da31bffbfffef9c9fc604b933569b609a76 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Mon, 6 Jul 2026 05:15:45 +0000 Subject: [PATCH] Repair report and version indexes and clean up stale Litestream files Add a migration that rebuilds all indexes on the report and version tables. Long-lived databases can carry index corruption (wrong number of entries in an index) that silently drops rows from index-driven queries. REINDEX is a no-op when the indexes are already consistent. The migration also removes report_benchmark rows (and their dependent metric, boundary, and alert rows) orphaned by deletes performed before ON DELETE CASCADE was enforced (see 2023-01-15-185835_perf_cascade). Migrations run with foreign_keys = OFF, so the delete chain is explicit and bottom-up, using temp tables to scan each unindexed table only once. At API server startup, remove stale Litestream files: the generations state from Litestream 0.3 that 0.5+ no longer reads, and leftover restore output files from an interrupted litestream restore. --- Cargo.lock | 1 + .../down.sql | 1 + .../up.sql | 58 ++++++++++ services/api/Cargo.toml | 3 + services/api/src/lib.rs | 3 + services/api/src/main.rs | 104 ++++++++++++++++++ 6 files changed, 170 insertions(+) create mode 100644 lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/down.sql create mode 100644 lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/up.sql diff --git a/Cargo.lock b/Cargo.lock index 455909bbb..1f5447a36 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1099,6 +1099,7 @@ dependencies = [ "sentry", "serde_yaml", "slog", + "tempfile", "thiserror 2.0.18", "tokio", "tokio-rustls", diff --git a/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/down.sql b/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/down.sql new file mode 100644 index 000000000..0d63c68ed --- /dev/null +++ b/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/down.sql @@ -0,0 +1 @@ +-- This update is not reversible. diff --git a/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/up.sql b/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/up.sql new file mode 100644 index 000000000..b40d791ed --- /dev/null +++ b/lib/bencher_schema/migrations/2026-07-06-120000_reindex_report_version/up.sql @@ -0,0 +1,58 @@ +-- Rebuild all indexes on the report and version tables. +-- Long-lived databases can carry index corruption (wrong # of entries in index) +-- that silently drops rows from index-driven queries. +-- REINDEX is a no-op when the indexes are already consistent. +REINDEX report; +REINDEX version; +-- Remove report_benchmark rows orphaned by deletes performed before +-- ON DELETE CASCADE was enforced (see 2023-01-15-185835_perf_cascade), +-- along with their dependent rows. +-- Migrations run with foreign_keys = OFF, so ON DELETE CASCADE does not +-- fire here and the delete chain must be explicit, bottom-up: +-- alert -> boundary -> metric -> report_benchmark. +-- metric.report_benchmark_id and boundary.metric_id are not indexed, so +-- collect the orphaned ids once into temp tables to scan each table only once. +CREATE TEMPORARY TABLE orphan_report_benchmark AS +SELECT id +FROM report_benchmark +WHERE benchmark_id NOT IN ( + SELECT id + FROM benchmark + ); +CREATE TEMPORARY TABLE orphan_metric AS +SELECT id +FROM metric +WHERE report_benchmark_id IN ( + SELECT id + FROM orphan_report_benchmark + ); +CREATE TEMPORARY TABLE orphan_boundary AS +SELECT id +FROM boundary +WHERE metric_id IN ( + SELECT id + FROM orphan_metric + ); +DELETE FROM alert +WHERE boundary_id IN ( + SELECT id + FROM orphan_boundary + ); +DELETE FROM boundary +WHERE id IN ( + SELECT id + FROM orphan_boundary + ); +DELETE FROM metric +WHERE id IN ( + SELECT id + FROM orphan_metric + ); +DELETE FROM report_benchmark +WHERE id IN ( + SELECT id + FROM orphan_report_benchmark + ); +DROP TABLE orphan_boundary; +DROP TABLE orphan_metric; +DROP TABLE orphan_report_benchmark; diff --git a/services/api/Cargo.toml b/services/api/Cargo.toml index b14f14ab9..5179d4efc 100644 --- a/services/api/Cargo.toml +++ b/services/api/Cargo.toml @@ -91,6 +91,9 @@ thiserror.workspace = true tokio = { workspace = true, features = ["fs", "rt-multi-thread"] } tokio-rustls.workspace = true +[dev-dependencies] +tempfile.workspace = true + [lints] workspace = true diff --git a/services/api/src/lib.rs b/services/api/src/lib.rs index 58b6f4410..aa5a8d50e 100644 --- a/services/api/src/lib.rs +++ b/services/api/src/lib.rs @@ -17,6 +17,9 @@ use slog as _; use thiserror as _; use tokio as _; use tokio_rustls as _; +// Needed for binary tests +#[cfg(test)] +use tempfile as _; // Needed for distroless builds use libsqlite3_sys as _; diff --git a/services/api/src/main.rs b/services/api/src/main.rs index 485194dcf..7eff676fb 100644 --- a/services/api/src/main.rs +++ b/services/api/src/main.rs @@ -231,6 +231,7 @@ fn run_litestream( .map_err(LitestreamError::Database)? .join(&config.database.file) }; + cleanup_stale_litestream_files(log, &db_path); #[cfg(debug_assertions)] let config_path = PathBuf::from("etc/litestream.yml"); #[cfg(not(debug_assertions))] @@ -282,6 +283,56 @@ fn run_litestream( })) } +// Remove stale Litestream files that current versions no longer read. +// Litestream 0.3 kept its state in a `generations/` directory and a `generation` +// pointer file; Litestream 0.5+ only reads `ltx/`, so 0.3 state left behind by an +// upgrade is dead weight on the data volume. An interrupted `litestream restore` +// can also leave `.tmp*` output files behind. This runs before Litestream is +// spawned, so nothing else has these files open. Failures are logged and ignored: +// cleanup must never block server startup. +#[cfg(feature = "plus")] +fn cleanup_stale_litestream_files(log: &Logger, db_path: &std::path::Path) { + let Some(db_dir) = db_path.parent() else { + return; + }; + let Some(db_file_name) = db_path.file_name().and_then(std::ffi::OsStr::to_str) else { + return; + }; + + let state_dir = db_dir.join(format!(".{db_file_name}-litestream")); + let generations_dir = state_dir.join("generations"); + if generations_dir.is_dir() { + info!( + log, + "Removing legacy Litestream 0.3 state: {generations_dir:?}" + ); + if let Err(e) = std::fs::remove_dir_all(&generations_dir) { + slog::warn!( + log, + "Failed to remove legacy Litestream 0.3 state ({generations_dir:?}): {e}" + ); + } + } + + let stale_files = [ + state_dir.join("generation"), + db_dir.join(format!("{db_file_name}.tmp")), + db_dir.join(format!("{db_file_name}.tmp-wal")), + db_dir.join(format!("{db_file_name}.tmp-shm")), + ]; + for stale_file in stale_files { + if stale_file.is_file() { + info!(log, "Removing stale Litestream file: {stale_file:?}"); + if let Err(e) = std::fs::remove_file(&stale_file) { + slog::warn!( + log, + "Failed to remove stale Litestream file ({stale_file:?}): {e}" + ); + } + } + } +} + async fn create_api_server(config: Config) -> Result, ApiError> { #[cfg(feature = "otel")] bencher_otel::ApiMeter::increment(bencher_otel::ApiCounter::ServerStartup); @@ -292,3 +343,56 @@ async fn create_api_server(config: Config) -> Result, Api .await .map_err(ApiError::ConfigTxError) } + +#[cfg(all(test, feature = "plus"))] +mod tests { + fn discard_logger() -> slog::Logger { + slog::Logger::root(slog::Discard, slog::o!()) + } + + #[test] + fn cleanup_stale_litestream_files() { + let tmp_dir = tempfile::tempdir().unwrap(); + let dir = tmp_dir.path(); + let db_path = dir.join("bencher.db"); + std::fs::write(&db_path, b"db").unwrap(); + + // Legacy Litestream 0.3 state + let state_dir = dir.join(".bencher.db-litestream"); + let generation_dir = state_dir.join("generations/fa74c771d99d6b04/wal"); + std::fs::create_dir_all(&generation_dir).unwrap(); + std::fs::write(generation_dir.join("00000001.wal.lz4"), b"wal").unwrap(); + std::fs::write(state_dir.join("generation"), b"fa74c771d99d6b04").unwrap(); + + // Live Litestream 0.5+ state + let ltx_dir = state_dir.join("ltx/0"); + std::fs::create_dir_all(<x_dir).unwrap(); + let ltx_file = ltx_dir.join("0000000000000001-0000000000000001.ltx"); + std::fs::write(<x_file, b"ltx").unwrap(); + + // Interrupted restore leftovers + std::fs::write(dir.join("bencher.db.tmp-wal"), b"").unwrap(); + std::fs::write(dir.join("bencher.db.tmp-shm"), b"shm").unwrap(); + + super::cleanup_stale_litestream_files(&discard_logger(), &db_path); + + // Stale state is removed + assert!(!state_dir.join("generations").exists()); + assert!(!state_dir.join("generation").exists()); + assert!(!dir.join("bencher.db.tmp-wal").exists()); + assert!(!dir.join("bencher.db.tmp-shm").exists()); + // The database and live state are untouched + assert!(db_path.exists()); + assert!(ltx_file.exists()); + } + + #[test] + fn cleanup_stale_litestream_files_noop() { + let tmp_dir = tempfile::tempdir().unwrap(); + let db_path = tmp_dir.path().join("bencher.db"); + + super::cleanup_stale_litestream_files(&discard_logger(), &db_path); + + assert!(!db_path.exists()); + } +}