From 1acab9e32f813bd615e0255c1599dabdcdd4d7b2 Mon Sep 17 00:00:00 2001 From: badbread Date: Sat, 8 Aug 2026 10:46:56 -0700 Subject: [PATCH 1/2] fix(recorder): make storage seeding path-idempotent + name->path lookups Boot seeding (recorder main + the seed binary) upserted the two configured storages by NAME only. After an operator renames a storage in the console (PUT /config/storages/{id}), no row carries the configured *_STORAGE_NAME, so the next boot INSERTs a second, empty row for the same directory: the duplicate Live/Archive rows that showed up next to the real 2TB NVMe / 16TB Spinner rows. PR #557 added a warn-only detector (duplicate_path_under_other_name) and deliberately did nothing else; its revisit trigger (the duplicates actually materialize and confuse the operator) has now fired. The ghost rows are also not inert: the recorder resolves runtime defaults by config NAME (archive.rs free-space-floor fallback, reconcile.rs live/archive stage labelling), so once the real rows are renamed those lookups miss and silently degrade. - Promote the detector from warn to a GATE. Both seed paths now share one decision function, db::seed_storage_path_idempotent: retarget a same-named row's path; else if a row already covers the path under another name, SKIP the insert (never create a duplicate); else insert (fresh install). It never renames or deletes the operator's row, and folds the resolved row into the in-pass set so a shared live==archive directory resolves to one row. - Add db::get_storage_by_path (deterministic: oldest created_at wins on a shared path) + db::get_storage_by_name_or_path (name first, then path), and use the name-or-path variant at the two runtime sites so a renamed install keeps full behavior and, after the empty rows are cleaned up, the paths still resolve to the real rows. - Path comparison normalizes trailing slashes (normalize_storage_path). - No migration, no UNIQUE(path): existing DBs already carry duplicate paths and a shared live==archive layout is legal. Tests (DB-backed, gated on TEST_DATABASE_URL): the exact #557 scenario run twice (still 2 rows, operator names untouched, no growth), fresh-install two rows, same-name retarget, shared-layout one row, get_storage_by_path oldest-wins + trailing-slash, and the name->path free-floor/labelling fallback; plus pure tests for normalize_storage_path and the trailing-slash-aware detector. Cannot orphan footage: existing segments keep their storage_id, the default policy keeps live_storage_id, ensure_default_policy wires by PATH, and the fix only ever skips creating a NEW empty row. DECISIONS.md entry added (supersedes #557's detect-never-fix). The one-time cleanup of the two already-present empty rows remains a maintainer DB action, sequenced after this deploys. Signed-off-by: badbread --- docs/DECISIONS.md | 84 ++++++ services/common/src/db.rs | 406 ++++++++++++++++++++++++++++- services/recorder/src/archive.rs | 12 +- services/recorder/src/bin/seed.rs | 39 ++- services/recorder/src/main.rs | 95 +++---- services/recorder/src/reconcile.rs | 21 +- 6 files changed, 580 insertions(+), 77 deletions(-) diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 970bdfdf..2c698ce1 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -8,6 +8,90 @@ revisit. --- +## 2026-08-08, Boot storage seeding is PATH-idempotent (skip a name whose directory is already covered) + runtime name lookups fall back to path — supersedes #557's "detect, never fix" + +**Context.** `db::upsert_storage` is idempotent by NAME only +(`INSERT … ON CONFLICT (name) DO UPDATE SET path`). The recorder seeds the two +configured storages (`LIVE_STORAGE_NAME` / `ARCHIVE_STORAGE_NAME`, defaulting to +the compose values `Live` / `Archive`) on every boot, and the `seed` binary does +the same at container start. When an operator renames a storage in the console +(`PUT /config/storages/{id}` — e.g. `Live` → `2TB NVMe`), no row carries the +configured name any more, so the next boot's upsert-by-name INSERTs a SECOND, +empty row for the same directory. On prod this produced two ghost rows +(`Live` → `/data/live`, `Archive` → `/data/archive`) sitting next to the real +`2TB NVMe` / `16TB Spinner` rows. + +PR #557 (`b70a311`, 2026-08-07) added `db::duplicate_path_under_other_name` +as a *detector*: it WARNed at seed time and deliberately did nothing else, on the +reasoning that a boot-time adopt-by-rename would undo a deliberate operator +rename, and duplicate rows were "tolerated" because reconcile keys duplicates by +root path. That entry's revisit trigger — "the duplicates actually materialize +and confuse the operator" — has now fired. The ghost rows are also not fully +inert: the recorder resolves several runtime defaults by config NAME +(`archive.rs` free-space-floor fallback when a policy's `live_storage_id` is +NULL; `reconcile.rs` live/archive stage labelling), so once the real rows are +renamed those lookups miss and the code silently degrades (byte-cap floor, +fewer disks labelled). + +**Decision.** + +- **Seed by path-idempotence, not by name alone.** Boot seeding + (`main.rs::seed_storages`) and `bin/seed.rs` now share ONE decision function, + `db::seed_storage_path_idempotent(pool, known, name, path)`: + 1. a row named `name` exists → `upsert_storage` (retarget its path — the + supported "repoint a named storage via env" behavior, unchanged); + 2. else a row already covers `path` under a DIFFERENT name → SKIP the insert + (log `info!`, never create a second row) and return the covering row; + 3. else → insert (fresh install, unchanged). + The resolved row is folded into the in-pass `known` set, so a shared + `live == archive` directory now resolves to ONE row instead of two. The + operator's row is NEVER renamed or deleted (that could undo a deliberate + rename, or orphan real footage). +- **Runtime name lookups fall back to path.** New `db::get_storage_by_path` + (deterministic: OLDEST `created_at` wins when a path is shared) and + `db::get_storage_by_name_or_path` (name first, then path). The two runtime + sites — `archive.rs` free-floor fallback and `reconcile.rs` live/archive + labelling — call the name-or-path variant, so a renamed install keeps full + behavior, and once the empty ghost rows are cleaned up the configured paths + still resolve to the real renamed rows. +- `duplicate_path_under_other_name` is kept and promoted from detector to gate; + path comparison normalizes trailing slashes (`normalize_storage_path`). +- **No migration, no `UNIQUE(path)`** — see rejected. + +**Rejected / not done.** + +- *Boot-time adopt-by-rename.* Still rejected, for #557's original reason: + `PUT /config/storages/{id}` is a deliberate operator action, and silently + renaming their row back on the next boot would fight them. Skipping the + duplicate insert achieves the goal (one row per directory) without mutating + their row. +- *A `UNIQUE(path)` constraint.* Rejected: existing prod DBs already carry + duplicate paths (the very rows this fixes), so the migration would fail to + apply; and a shared `live == archive` layout expressed as two rows is legal + (`seed.rs` §6.5). The gate prevents NEW duplicates without breaking old DBs. +- *Deleting the existing ghost rows in code.* Out of scope and unsafe to do at + boot. A referenced row can't be deleted anyway (`segments.storage_id` FK is + `ON DELETE RESTRICT`); the one-time cleanup of the two already-present empty + rows is a maintainer DB action, sequenced AFTER this deploys. + +**Tests.** `seed_is_path_idempotent_against_renamed_rows` (the exact #557 +scenario, run twice → still 2 rows, operator names untouched, no growth); +`seed_fresh_db_creates_both_defaults`; `seed_retargets_existing_name_in_place`; +`seed_shared_layout_yields_one_row`; `get_storage_by_path_prefers_oldest` +(determinism + trailing-slash); `get_storage_by_name_or_path_falls_back_to_path` +(the free-floor/labelling fallback); plus pure-function tests for +`normalize_storage_path` and the trailing-slash-aware detector. + +**Revisit triggers.** + +- If a future need arises to actively MERGE duplicate storage rows (reassign + segments off a ghost row so it can be deleted), that belongs in an explicit, + guarded admin/maintenance operation — not the boot seed — and would get its + own entry. +- If storages ever gain a real path-uniqueness requirement (e.g. per-path + accounting that duplicates would corrupt), reconsider `UNIQUE(path)` behind a + data-cleanup migration. + ## 2026-08-07, Timeline motion-intensity is bucketed in SQL (GROUP BY over a `generate_series`-expanded range), not by fetching every segment to Rust **Context.** The desktop Playback/clip timeline "intensity ribbon" took roughly diff --git a/services/common/src/db.rs b/services/common/src/db.rs index d4a983b8..3e878d8f 100644 --- a/services/common/src/db.rs +++ b/services/common/src/db.rs @@ -279,13 +279,17 @@ pub async fn upsert_storage(pool: &Pool, name: &str, path: &str) -> Result &str { + let trimmed = path.trim_end_matches('/'); + if trimmed.is_empty() { + path + } else { + trimmed + } +} + +/// Fetch the storage row whose root PATH matches `path` (after trailing-slash +/// normalization). +/// +/// When several rows share a path (a tolerated duplicate condition on +/// already-affected databases — see [`duplicate_path_under_other_name`]) the +/// OLDEST by `created_at` wins, so resolution is deterministic and prefers the +/// operator's original row over any later duplicate. Returns `None` when no row +/// covers the path. +/// +/// # Errors +/// +/// Returns an error if the database query fails. +pub async fn get_storage_by_path(pool: &Pool, path: &str) -> Result> { + let want = normalize_storage_path(path); + let client = get_conn(pool).await?; + let rows = client + .query( + "SELECT id, name, path, total_bytes, icon, created_at FROM storages ORDER BY created_at", + &[], + ) + .await + .context("get_storage_by_path")?; + Ok(rows + .iter() + .map(storage_from_row) + .find(|s| normalize_storage_path(&s.path) == want)) +} + +/// Resolve a storage by NAME first, then fall back to PATH. +/// +/// The recorder resolves several runtime defaults by the configured +/// `*_STORAGE_NAME` (the free-space-floor fallback in `archive.rs`, and +/// reconcile's live/archive stage labelling). An operator who renamed a storage +/// via `PUT /config/storages/{id}` no longer has a row under the configured +/// name, so a name-only lookup returns `None` and the caller silently degrades +/// (falls back to the byte cap / labels fewer disks). Falling back to the +/// configured PATH recovers the real row — the rename never changes the path — +/// and, once the empty seed-duplicates are cleaned up, keeps those paths +/// resolving to the correct storage. +/// +/// # Errors +/// +/// Returns an error if the database query fails. +pub async fn get_storage_by_name_or_path( + pool: &Pool, + name: &str, + path: &str, +) -> Result> { + if let Some(s) = get_storage_by_name(pool, name).await? { + return Ok(Some(s)); + } + get_storage_by_path(pool, path).await +} + +/// Seed one storage row PATH-IDEMPOTENTLY and return the resolved row. +/// +/// The single decision path shared by the recorder boot seed +/// (`main.rs::seed_storages`) and the `seed` binary, so both behave identically: +/// +/// * a row named `name` already exists → [`upsert_storage`] (retarget its path, +/// the supported "retarget a named storage via env" behavior); +/// * else a row already covers `path` under a DIFFERENT name → SKIP the insert +/// (never create a second, empty row for the same directory) and return that +/// existing row; +/// * else → insert (fresh install). +/// +/// The resolved row is folded into `known` so a later call in the same seed pass +/// (archive after live) sees it — a shared `live == archive` directory then +/// resolves to ONE row, not two. This never renames or deletes the operator's +/// row: a boot-time rename would silently undo a deliberate +/// `PUT /config/storages/{id}` rename, and a delete could orphan real footage. +/// No footage is ever affected — existing segments keep their `storage_id`, and +/// the runtime name lookups fall back to path +/// ([`get_storage_by_name_or_path`]). +/// +/// # Errors +/// +/// Returns an error if a database query fails. +pub async fn seed_storage_path_idempotent( + pool: &Pool, + known: &mut Vec<(String, String)>, + name: &str, + path: &str, +) -> Result { + let resolved = if let Some(other) = + duplicate_path_under_other_name(known.iter().cloned(), name, path) + { + tracing::info!( + path = %path, + existing_name = %other, + configured_name = %name, + "storage path already covered by an existing row under another name; not \ + seeding the configured name (it would duplicate the directory). Runtime \ + lookups resolve this path via the existing row." + ); + match get_storage_by_path(pool, path).await? { + Some(s) => s, + // The covering row vanished between the snapshot and this lookup + // (an operator deletion racing the seed). Fall back to a plain + // upsert so seeding still completes; it stays idempotent. + None => upsert_storage(pool, name, path).await?, + } + } else { + upsert_storage(pool, name, path).await? + }; + // Reflect the resolved row so the next call's gate sees it; drop any stale + // entry for the same name first (the retarget case updated its path). + known.retain(|(n, _)| n != &resolved.name); + known.push((resolved.name.clone(), resolved.path.clone())); + Ok(resolved) +} + /// Fetch a storage row by its UUID. /// /// Returns `None` if the row does not exist. @@ -12972,6 +13102,266 @@ mod tests { ); } + #[test] + fn normalize_storage_path_trims_trailing_slashes() { + assert_eq!(normalize_storage_path("/data/live"), "/data/live"); + assert_eq!(normalize_storage_path("/data/live/"), "/data/live"); + assert_eq!(normalize_storage_path("/data/live///"), "/data/live"); + // A root / all-slash path never collapses to the empty string. + assert_eq!(normalize_storage_path("/"), "/"); + assert_eq!(normalize_storage_path(""), ""); + } + + #[test] + fn duplicate_path_detector_ignores_a_trailing_slash() { + // A stored `/data/live/` and a configured `/data/live` are the same + // directory, so the gate must still fire (skip the duplicate insert). + let rows = vec![("2TB NVMe".to_owned(), "/data/live/".to_owned())]; + assert_eq!( + duplicate_path_under_other_name(rows, "Live", "/data/live"), + Some("2TB NVMe".to_owned()), + ); + } + + // ── path-idempotent storage seeding (issue: duplicate Live/Archive rows) ── + // + // The DB-backed tests below prove `seed_storage_path_idempotent` (the ONE + // decision path shared by `main.rs::seed_storages` and `bin/seed.rs`) never + // creates a second storage row for a directory that already has one under a + // different name — the exact mechanism that spawned empty "Live"/"Archive" + // ghosts next to operator-renamed rows — while still creating the defaults + // on a fresh install and retargeting a same-named row's path. + + /// Create just the `storages` table (schema-identical to migration 0001's + /// relevant columns) in the fixture schema. + async fn create_storages_table(pool: &Pool) { + get_conn(pool) + .await + .expect("get_conn") + .batch_execute( + r" + CREATE TABLE storages ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + name text NOT NULL UNIQUE, + path text NOT NULL, + total_bytes bigint, + icon text, + created_at timestamptz NOT NULL DEFAULT now() + ); + ", + ) + .await + .expect("create storages table"); + } + + /// Run one full boot-shaped seed pass (live then archive), exactly as + /// `seed_storages` / `bin/seed.rs` do: snapshot the rows, then thread + /// `known` through the two `seed_storage_path_idempotent` calls. + async fn seed_pass(pool: &Pool, live: (&str, &str), archive: (&str, &str)) { + let mut known: Vec<(String, String)> = list_storages(pool) + .await + .expect("list_storages") + .into_iter() + .map(|s| (s.name, s.path)) + .collect(); + seed_storage_path_idempotent(pool, &mut known, live.0, live.1) + .await + .expect("seed live"); + seed_storage_path_idempotent(pool, &mut known, archive.0, archive.1) + .await + .expect("seed archive"); + } + + /// The #557 scenario: the DB already has the operator-renamed rows + /// ("2TB NVMe" → /data/live, "16TB Spinner" → /data/archive). Seeding the + /// compose default names (Live/Archive) must NOT add duplicate rows, must + /// leave the operator's names untouched, and must be idempotent across + /// repeated boots. + #[tokio::test] + async fn seed_is_path_idempotent_against_renamed_rows() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + // Operator's real, renamed rows. + upsert_storage(&fx.pool, "2TB NVMe", "/data/live") + .await + .expect("seed operator live row"); + upsert_storage(&fx.pool, "16TB Spinner", "/data/archive") + .await + .expect("seed operator archive row"); + + // Two boots' worth of seeding with the compose defaults. + for _ in 0..2 { + seed_pass(&fx.pool, ("Live", "/data/live"), ("Archive", "/data/archive")).await; + } + + let all = list_storages(&fx.pool).await.expect("list"); + assert_eq!(all.len(), 2, "no duplicate rows should be created"); + let names: std::collections::HashSet<&str> = + all.iter().map(|s| s.name.as_str()).collect(); + assert!( + names.contains("2TB NVMe") && names.contains("16TB Spinner"), + "operator names must be untouched, got {names:?}", + ); + assert!( + !names.contains("Live") && !names.contains("Archive"), + "no ghost default-named rows, got {names:?}", + ); + } + + /// A genuinely-fresh install still gets both default storage rows. + #[tokio::test] + async fn seed_fresh_db_creates_both_defaults() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + seed_pass(&fx.pool, ("Live", "/data/live"), ("Archive", "/data/archive")).await; + + let all = list_storages(&fx.pool).await.expect("list"); + assert_eq!(all.len(), 2, "fresh install seeds two rows"); + let by_name: std::collections::HashMap<&str, &str> = + all.iter().map(|s| (s.name.as_str(), s.path.as_str())).collect(); + assert_eq!(by_name.get("Live"), Some(&"/data/live")); + assert_eq!(by_name.get("Archive"), Some(&"/data/archive")); + } + + /// A same-named row's path is still retargeted (the supported + /// "retarget a named storage via env" behavior), with no new row. + #[tokio::test] + async fn seed_retargets_existing_name_in_place() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + upsert_storage(&fx.pool, "Live", "/data/old-disk") + .await + .expect("seed old-path Live row"); + + let mut known: Vec<(String, String)> = list_storages(&fx.pool) + .await + .expect("list") + .into_iter() + .map(|s| (s.name, s.path)) + .collect(); + let seeded = seed_storage_path_idempotent(&fx.pool, &mut known, "Live", "/data/live") + .await + .expect("seed"); + assert_eq!(seeded.path, "/data/live", "path retargeted"); + + let all = list_storages(&fx.pool).await.expect("list"); + assert_eq!(all.len(), 1, "retarget must not add a row"); + assert_eq!(all[0].path, "/data/live"); + } + + /// A shared live == archive directory resolves to ONE row (the archive seed + /// is gated because the just-seeded live row already covers the path). + #[tokio::test] + async fn seed_shared_layout_yields_one_row() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + seed_pass(&fx.pool, ("Live", "/data"), ("Archive", "/data")).await; + + let all = list_storages(&fx.pool).await.expect("list"); + assert_eq!(all.len(), 1, "shared layout must not duplicate the directory"); + assert_eq!(all[0].name, "Live"); + } + + /// When several rows share a path, `get_storage_by_path` deterministically + /// returns the OLDEST by `created_at` (the operator's original, not a later + /// duplicate). + #[tokio::test] + async fn get_storage_by_path_prefers_oldest() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + get_conn(&fx.pool) + .await + .expect("get_conn") + .batch_execute( + r" + INSERT INTO storages (name, path, created_at) VALUES + ('Original', '/data/live', now() - interval '1 hour'), + ('GhostDup', '/data/live', now()); + ", + ) + .await + .expect("insert two rows sharing a path"); + + let resolved = get_storage_by_path(&fx.pool, "/data/live") + .await + .expect("query") + .expect("a row covers the path"); + assert_eq!(resolved.name, "Original", "oldest row wins"); + // Trailing-slash normalization still resolves the same row. + let resolved2 = get_storage_by_path(&fx.pool, "/data/live/") + .await + .expect("query") + .expect("a row covers the path"); + assert_eq!(resolved2.name, "Original"); + } + + /// The runtime free-floor / labelling fallback: `get_storage_by_name_or_path` + /// resolves by NAME first, then falls back to PATH — so a renamed install + /// (no row under the configured name) still resolves to the real row instead + /// of silently degrading. + #[tokio::test] + async fn get_storage_by_name_or_path_falls_back_to_path() { + let Some(url) = test_db_url() else { + eprintln!("skipping: TEST_DATABASE_URL not set"); + return; + }; + let fx = setup_schema(&url).await; + create_storages_table(&fx.pool).await; + + // Renamed install: only "2TB NVMe" at /data/live exists; no "Live" row. + upsert_storage(&fx.pool, "2TB NVMe", "/data/live") + .await + .expect("seed renamed row"); + assert!( + get_storage_by_name(&fx.pool, "Live") + .await + .expect("by name") + .is_none(), + "precondition: no row named Live", + ); + let resolved = get_storage_by_name_or_path(&fx.pool, "Live", "/data/live") + .await + .expect("query") + .expect("path fallback resolves"); + assert_eq!(resolved.name, "2TB NVMe", "resolves to the real row via path"); + + // NAME still wins when a row under the configured name exists, even if + // that row's path differs from the configured one. + upsert_storage(&fx.pool, "Live", "/data/somewhere-else") + .await + .expect("seed a same-named row"); + let name_wins = get_storage_by_name_or_path(&fx.pool, "Live", "/data/live") + .await + .expect("query") + .expect("resolves"); + assert_eq!(name_wins.name, "Live", "name lookup takes precedence"); + } + #[test] fn levenshtein_treats_confusable_chars_as_free() { // An OCR O/0, I/1, B/8, S/5, Z/2, D/0, G/6 flip costs nothing, so a diff --git a/services/recorder/src/archive.rs b/services/recorder/src/archive.rs index 8106f5bb..721b55c5 100644 --- a/services/recorder/src/archive.rs +++ b/services/recorder/src/archive.rs @@ -2138,10 +2138,14 @@ pub async fn policy_size_eviction_sweep( // byte cap. let live_storage_for_floor: Option = match policy.live_storage_id { Some(sid) => db::get_storage(pool, sid).await.ok().flatten(), - None => db::get_storage_by_name(pool, &config.live_storage_name) - .await - .ok() - .flatten(), + None => db::get_storage_by_name_or_path( + pool, + &config.live_storage_name, + &config.live_storage_path, + ) + .await + .ok() + .flatten(), }; // `deficit` is how many bytes we must free to get back above the floor; // 0 when above the floor or free space can't be read. The per-policy diff --git a/services/recorder/src/bin/seed.rs b/services/recorder/src/bin/seed.rs index 2cdc1c1f..42081752 100644 --- a/services/recorder/src/bin/seed.rs +++ b/services/recorder/src/bin/seed.rs @@ -120,11 +120,29 @@ async fn main() -> Result<()> { .context("schema assertion: storages.name must have a UNIQUE constraint")?; // ── 1. Storage rows ──────────────────────────────────────────────────────── - - let live_storage = - db::upsert_storage(&pool, &config.live_storage_name, &config.live_storage_path) - .await - .context("upserting live storage")?; + // + // Seed PATH-IDEMPOTENTLY: never INSERT a second row for a directory that + // already has a storage row under a different name (the operator-rename + // case — see `db::seed_storage_path_idempotent`, the ONE decision path + // shared with the recorder boot seed). `known` is threaded through so the + // archive seed sees a just-seeded live row (a shared live==archive directory + // then resolves to one row, not two). + + let mut known: Vec<(String, String)> = db::list_storages(&pool) + .await + .context("listing storages before seeding")? + .into_iter() + .map(|s| (s.name, s.path)) + .collect(); + + let live_storage = db::seed_storage_path_idempotent( + &pool, + &mut known, + &config.live_storage_name, + &config.live_storage_path, + ) + .await + .context("seeding live storage")?; info!( id = %live_storage.id, name = %live_storage.name, @@ -148,9 +166,14 @@ async fn main() -> Result<()> { (config.archive_storage_name.clone(), archive_path) }; - let archive_storage = db::upsert_storage(&pool, &archive_name, &archive_path_effective) - .await - .context("upserting archive storage")?; + let archive_storage = db::seed_storage_path_idempotent( + &pool, + &mut known, + &archive_name, + &archive_path_effective, + ) + .await + .context("seeding archive storage")?; info!( id = %archive_storage.id, name = %archive_storage.name, diff --git a/services/recorder/src/main.rs b/services/recorder/src/main.rs index bebc27ed..f4e97514 100644 --- a/services/recorder/src/main.rs +++ b/services/recorder/src/main.rs @@ -1071,65 +1071,50 @@ impl RecorderSupervisor { /// requiring a separate `seed` run for storage rows. Camera rows still /// require the `seed` binary. async fn seed_storages(&self) -> Result<()> { + // Seed the two named storage rows PATH-IDEMPOTENTLY: never INSERT a + // second row for a directory that already has a storage row under + // another name. + // // The `*_STORAGE_NAME` code defaults track docker-compose.yml (`Live` / - // `Archive`). A deployment that predates that and does NOT get the names - // from compose still has rows under the old defaults, so the upsert - // below would INSERT a second row for the same directory. That is - // tolerated (reconcile keys duplicates by root path) and loses no - // footage — existing segments keep their `storage_id` and the default - // policy keeps its `live_storage_id` — but it must not happen silently. - match db::list_storages(&self.pool).await { - Ok(existing) => { - let rows: Vec<(String, String)> = - existing.into_iter().map(|s| (s.name, s.path)).collect(); - for (name, path) in [ - ( - &self.config.live_storage_name, - &self.config.live_storage_path, - ), - ( - &self.config.archive_storage_name, - &self.config.archive_storage_path, - ), - ] { - if let Some(other) = - db::duplicate_path_under_other_name(rows.iter().cloned(), name, path) - { - warn!( - path = %path, - existing_name = %other, - configured_name = %name, - "DUPLICATE STORAGE PATH: an existing storage row already points at \ - this path under a different name, so seeding the configured name \ - adds a SECOND row for the same directory. No footage is affected \ - (existing segments keep their storage). To merge them, either set \ - LIVE_STORAGE_NAME / ARCHIVE_STORAGE_NAME to the existing name, or \ - rename the existing storage in the console." - ); - } - } - } + // `Archive`). An operator who renamed a storage via + // `PUT /config/storages/{id}` (e.g. "2TB NVMe") no longer has a row + // under the configured name, so a plain upsert-by-name would INSERT a + // second, empty row for the same directory on every boot (the duplicate + // reported in the console). We never rename or delete the operator's row + // (a boot-time rename would silently undo a deliberate console rename); + // we simply SKIP seeding a name whose path is already covered. The + // runtime name lookups fall back to path (`get_storage_by_name_or_path`) + // so a renamed install keeps full behavior. A genuinely-fresh install + // (no rows yet) still creates the defaults. + // + // No footage can be affected either way: existing segments keep their + // `storage_id`, the default policy keeps its `live_storage_id`, and + // `ensure_default_policy` wires the default policy by PATH (below). + let mut known: Vec<(String, String)> = match db::list_storages(&self.pool).await { + Ok(existing) => existing.into_iter().map(|s| (s.name, s.path)).collect(), Err(e) => { - // Advisory only — never block the seed (and therefore recording). - warn!(error = %e, "could not list storages to check for duplicate paths before seeding"); + // Advisory only — never block the seed (and therefore + // recording). An empty view degrades to the fresh-install path + // (plain upsert-by-name), which upsert would merge anyway. + warn!(error = %e, "could not list storages before seeding; proceeding with upsert-by-name"); + Vec::new() } - } - - db::upsert_storage( - &self.pool, - &self.config.live_storage_name, - &self.config.live_storage_path, - ) - .await - .context("upserting live storage")?; + }; - db::upsert_storage( - &self.pool, - &self.config.archive_storage_name, - &self.config.archive_storage_path, - ) - .await - .context("upserting archive storage")?; + for (name, path) in [ + ( + &self.config.live_storage_name, + &self.config.live_storage_path, + ), + ( + &self.config.archive_storage_name, + &self.config.archive_storage_path, + ), + ] { + db::seed_storage_path_idempotent(&self.pool, &mut known, name, path) + .await + .with_context(|| format!("seeding storage '{name}'"))?; + } info!( live = %self.config.live_storage_name, diff --git a/services/recorder/src/reconcile.rs b/services/recorder/src/reconcile.rs index a7ea30eb..005f3993 100644 --- a/services/recorder/src/reconcile.rs +++ b/services/recorder/src/reconcile.rs @@ -1071,7 +1071,18 @@ async fn run_background(pool: Pool, config: Config, shutdown: CancellationToken) // conservative recording stage, exactly as before. let mut archive_storage_ids: HashSet = HashSet::new(); let mut live_storage_ids: HashSet = HashSet::new(); - match db::get_storage_by_name(&pool, &config.archive_storage_name).await { + // Resolve the configured defaults by NAME first, then fall back to PATH: + // an operator who renamed a storage in the console (or an install whose + // empty seed-duplicates were cleaned up) no longer has a row under the + // configured `*_STORAGE_NAME`, and a name-only lookup would leave the disk + // unlabelled (safe, but wrong). See `db::get_storage_by_name_or_path`. + match db::get_storage_by_name_or_path( + &pool, + &config.archive_storage_name, + &config.archive_storage_path, + ) + .await + { Ok(opt) => { if let Some(s) = opt { archive_storage_ids.insert(s.id); @@ -1082,7 +1093,13 @@ async fn run_background(pool: Pool, config: Config, shutdown: CancellationToken) warn!(error = %e, "reconcile phase 2: cannot resolve archive-default storage"); } } - match db::get_storage_by_name(&pool, &config.live_storage_name).await { + match db::get_storage_by_name_or_path( + &pool, + &config.live_storage_name, + &config.live_storage_path, + ) + .await + { Ok(opt) => { if let Some(s) = opt { live_storage_ids.insert(s.id); From 312a272f220b509bca6afe7742877b32ea93f106 Mon Sep 17 00:00:00 2001 From: badbread Date: Sat, 8 Aug 2026 11:02:14 -0700 Subject: [PATCH 2/2] style: rustfmt the storage-seed changes Signed-off-by: badbread --- services/common/src/db.rs | 75 +++++++++++++++++++------------ services/recorder/src/bin/seed.rs | 12 ++--- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/services/common/src/db.rs b/services/common/src/db.rs index 3e878d8f..14810f99 100644 --- a/services/common/src/db.rs +++ b/services/common/src/db.rs @@ -425,27 +425,26 @@ pub async fn seed_storage_path_idempotent( name: &str, path: &str, ) -> Result { - let resolved = if let Some(other) = - duplicate_path_under_other_name(known.iter().cloned(), name, path) - { - tracing::info!( - path = %path, - existing_name = %other, - configured_name = %name, - "storage path already covered by an existing row under another name; not \ - seeding the configured name (it would duplicate the directory). Runtime \ - lookups resolve this path via the existing row." - ); - match get_storage_by_path(pool, path).await? { - Some(s) => s, - // The covering row vanished between the snapshot and this lookup - // (an operator deletion racing the seed). Fall back to a plain - // upsert so seeding still completes; it stays idempotent. - None => upsert_storage(pool, name, path).await?, - } - } else { - upsert_storage(pool, name, path).await? - }; + let resolved = + if let Some(other) = duplicate_path_under_other_name(known.iter().cloned(), name, path) { + tracing::info!( + path = %path, + existing_name = %other, + configured_name = %name, + "storage path already covered by an existing row under another name; not \ + seeding the configured name (it would duplicate the directory). Runtime \ + lookups resolve this path via the existing row." + ); + match get_storage_by_path(pool, path).await? { + Some(s) => s, + // The covering row vanished between the snapshot and this lookup + // (an operator deletion racing the seed). Fall back to a plain + // upsert so seeding still completes; it stays idempotent. + None => upsert_storage(pool, name, path).await?, + } + } else { + upsert_storage(pool, name, path).await? + }; // Reflect the resolved row so the next call's gate sees it; drop any stale // entry for the same name first (the retarget case updated its path). known.retain(|(n, _)| n != &resolved.name); @@ -13196,13 +13195,17 @@ mod tests { // Two boots' worth of seeding with the compose defaults. for _ in 0..2 { - seed_pass(&fx.pool, ("Live", "/data/live"), ("Archive", "/data/archive")).await; + seed_pass( + &fx.pool, + ("Live", "/data/live"), + ("Archive", "/data/archive"), + ) + .await; } let all = list_storages(&fx.pool).await.expect("list"); assert_eq!(all.len(), 2, "no duplicate rows should be created"); - let names: std::collections::HashSet<&str> = - all.iter().map(|s| s.name.as_str()).collect(); + let names: std::collections::HashSet<&str> = all.iter().map(|s| s.name.as_str()).collect(); assert!( names.contains("2TB NVMe") && names.contains("16TB Spinner"), "operator names must be untouched, got {names:?}", @@ -13223,12 +13226,19 @@ mod tests { let fx = setup_schema(&url).await; create_storages_table(&fx.pool).await; - seed_pass(&fx.pool, ("Live", "/data/live"), ("Archive", "/data/archive")).await; + seed_pass( + &fx.pool, + ("Live", "/data/live"), + ("Archive", "/data/archive"), + ) + .await; let all = list_storages(&fx.pool).await.expect("list"); assert_eq!(all.len(), 2, "fresh install seeds two rows"); - let by_name: std::collections::HashMap<&str, &str> = - all.iter().map(|s| (s.name.as_str(), s.path.as_str())).collect(); + let by_name: std::collections::HashMap<&str, &str> = all + .iter() + .map(|s| (s.name.as_str(), s.path.as_str())) + .collect(); assert_eq!(by_name.get("Live"), Some(&"/data/live")); assert_eq!(by_name.get("Archive"), Some(&"/data/archive")); } @@ -13278,7 +13288,11 @@ mod tests { seed_pass(&fx.pool, ("Live", "/data"), ("Archive", "/data")).await; let all = list_storages(&fx.pool).await.expect("list"); - assert_eq!(all.len(), 1, "shared layout must not duplicate the directory"); + assert_eq!( + all.len(), + 1, + "shared layout must not duplicate the directory" + ); assert_eq!(all[0].name, "Live"); } @@ -13348,7 +13362,10 @@ mod tests { .await .expect("query") .expect("path fallback resolves"); - assert_eq!(resolved.name, "2TB NVMe", "resolves to the real row via path"); + assert_eq!( + resolved.name, "2TB NVMe", + "resolves to the real row via path" + ); // NAME still wins when a row under the configured name exists, even if // that row's path differs from the configured one. diff --git a/services/recorder/src/bin/seed.rs b/services/recorder/src/bin/seed.rs index 42081752..fad5586b 100644 --- a/services/recorder/src/bin/seed.rs +++ b/services/recorder/src/bin/seed.rs @@ -166,14 +166,10 @@ async fn main() -> Result<()> { (config.archive_storage_name.clone(), archive_path) }; - let archive_storage = db::seed_storage_path_idempotent( - &pool, - &mut known, - &archive_name, - &archive_path_effective, - ) - .await - .context("seeding archive storage")?; + let archive_storage = + db::seed_storage_path_idempotent(&pool, &mut known, &archive_name, &archive_path_effective) + .await + .context("seeding archive storage")?; info!( id = %archive_storage.id, name = %archive_storage.name,