From 74ea45e8057c1d9bbea203fc2e7d093492bd6b0a Mon Sep 17 00:00:00 2001 From: Ben Kearns <35475+bkearns@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:31:13 -0700 Subject: [PATCH] fix(storage): a missing SSTable component is a defect, not a later problem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup repair checked whether a critical component was zero-byte and quarantined the generation if so, but let a MISSING component fall through: Err(_) => { // Missing component — will fail in open_sstable_from_dir. } It does fail there. What reaches the operator is storage: I/O error: No such file or directory (os error 2) naming no table, no generation and no path — and it fails EVERY read of that table rather than skipping the one generation. Missing is strictly worse than empty, and was the one case not caught. Observed on the ferrosa-memory native cluster: agent_memory .mobile_control_cursor_state and .knowledge_by_state were unreadable on all three nodes. The cursor allocator reverted to an older surviving value and regressed ~10,600 cursors, which surfaced as a mobile client refusing a high-water below the one it had already committed. The client guard was right; the store had moved backwards under it. The decision moves to sstable_health.rs as a pure function over probed components, because the judgement needed no engine, manifest or disk — only the walk around it did. Missing, zero-byte and unreadable all withhold the generation now. Rows.db stays out of the critical set: the writer legitimately emits it empty for simple partitions, and treating it as critical would quarantine healthy SSTables. Both startup log lines name the component and why it is unusable, so the next occurrence does not need someone reading generation directories on disk to find out which file is gone. Tests: 8 cases in sstable_health::tests, written against the old behaviour first — a_missing_data_file_is_unusable and the_first_defect_in_order_is_the_one_reported both failed before the fix, the latter reporting a zero-byte Partitions.db while stepping over a missing Data.db. Full crate suite 1089 passed, 0 failed, 0 ignored; fmt and clippy -D warnings clean. Not addressed: the manifest still references the quarantined generation, so it is re-probed and re-quarantined on every boot, and what removed the files in the first place is still unknown. Both recorded in ST-18. --- ferrosa-storage/README.md | 9 ++ ferrosa-storage/specs/fmea.md | 1 + ferrosa-storage/src/engine.rs | 45 +++--- ferrosa-storage/src/lib.rs | 1 + ferrosa-storage/src/sstable_health.rs | 197 ++++++++++++++++++++++++++ 5 files changed, 234 insertions(+), 19 deletions(-) create mode 100644 ferrosa-storage/src/sstable_health.rs diff --git a/ferrosa-storage/README.md b/ferrosa-storage/README.md index 8b5f1128..064cd8d3 100644 --- a/ferrosa-storage/README.md +++ b/ferrosa-storage/README.md @@ -202,6 +202,15 @@ data through this crate, almost always via the `Arc` indirection found at flush/replay are written to a durable `quarantine/*.jsonl` sidecar instead of crashing; the self-heal controller detects corrupt SSTables and quarantines them under a safety rail. +- **Startup SSTable health** (`sstable_health.rs`) — decides whether a + generation on disk can serve reads before it is loaded. A critical component + (`Data.db`, `Partitions.db`) that is **missing**, **zero-byte**, or + **unreadable** withholds the generation; `Rows.db` is excluded because the + writer legitimately emits it empty for simple partitions. The judgement is a + pure function so it is testable without a disk or a cluster, and the log line + names the component and the reason — a manifest entry pointing at a file that + is gone used to surface only as `No such file or directory (os error 2)` with + no table, generation or path, failing every read of that table. - **Accord** (`accord/`) — per-shard conflict index + protocol log for strict-serializable transactions. Also defines `TransactionCommitter` (ADR-021): the front-end-facing seam CQL/Postgres `BEGIN`/`COMMIT` call to commit a diff --git a/ferrosa-storage/specs/fmea.md b/ferrosa-storage/specs/fmea.md index a50e6a2b..06da0e2a 100644 --- a/ferrosa-storage/specs/fmea.md +++ b/ferrosa-storage/specs/fmea.md @@ -29,6 +29,7 @@ the top of the scale: a defect here is silent data loss or corruption. | ST-15 | **Concurrent schema replays replace a live table store** — two registrations observe an absent table, then the later builder overwrites the first store after it has accepted writes | Read-after-write and secondary-index queries return empty results until a later write or flush rebuilds state | 8 | 3 | 6 | 144 | **Mitigated**: table installation is compare-and-install under the table-map write lock. A losing replay merges its index declarations into the winning store and never replaces active memtable/index postings. Guarded by `engine::tests::late_table_registration_cannot_replace_live_memtable`. | | ST-16 | **`ALTER TABLE ADD` shifts storage and index cell ordinals** — regular columns are ordered by the column-name comparator, so adding a column that sorts before an existing column re-numbers cell ordinals while existing rows and index declarations were written under the old layout | Post-ALTER reads could misattribute pre-ALTER row cells to the wrong column, and index declarations/backfills could extract postings from the wrong cell, serving false empty or wrong-column results (`agent_memory.entity_store` phonetic lookups after ddl/020) | 8 | 4 | 7 | 224 → 32 | **Fixed**: `TableStore::update_schema` remaps every positional index declaration (scalar `indexed_columns`, `fulltext_indexes`, `vector_index_configs`, filtered-index predicate clauses) through the old schema's column name before swapping the new schema; an unmappable ordinal fails loud (error log) instead of silently indexing the wrong cell. `StorageEngine::update_table_schema` now flushes dirty pre-ALTER memtable/replay rows under the old schema while holding the table-map write lock, then swaps schemas and discards the covered commit-log position. Reads already use each SSTable `SerializationHeader` to remap physical ordinals by column name, and index backfills now map current ordinals and filtered predicates through each SSTable header before building sidecars. Tests: `store::update_schema_remaps_index_ordinal_when_added_column_sorts_first`, `store::index_backfill_maps_current_ordinals_to_legacy_sstable_source_ordinals`, `engine::update_table_schema_preserves_unflushed_pre_alter_row_ordinals`, and `ferrosa-cql` `router::tests::phonetic_keyed_equality_matches_when_fulltext_shares_the_column`. | | ST-17 | **Storage flush replaces the registry snapshot with an incompatible JSON array** — two components wrote different formats to `schema.json` | Next boot can register storage tables while the CQL registry starts empty, making intact SSTables unreachable | 10 | 3 | 8 | 240 → 10 | **Fixed**: `SchemaSnapshotStore` is the sole crash-safe publisher of discriminated `schema.json`; storage-only registrations use `storage-schema.json`. Reads and writes are capped at 64 MiB and stream without an intermediate JSON buffer. Corrupt, unknown, oversized, and legacy-array registry inputs are quarantined and returned as startup errors. File locking, verified staging, fsync, atomic rename, directory fsync, and three retained generations cover concurrent writers and crash boundaries. | +| ST-18 | **Startup repair ignores a MISSING SSTable component** — the zero-byte check quarantined an empty `Data.db`/`Partitions.db` but let an absent one fall through, on the reasoning that it would "fail in `open_sstable_from_dir`" | It does fail there, as `storage: I/O error: No such file or directory (os error 2)` naming no table, generation or path — and it fails EVERY read of that table rather than skipping the one generation. Observed on the ferrosa-memory native cluster: `agent_memory.mobile_control_cursor_state` and `knowledge_by_state` unreadable on all three nodes, the cursor allocator silently reverting to an older value and regressing ~10,600 cursors | 8 | 4 | 8 | 256 → 24 | **Fixed**: `sstable_health.rs` makes the decision a pure function over probed components — missing, zero-byte and unreadable all withhold the generation, `Rows.db` stays excluded because the writer legitimately emits it empty. Both startup log lines now name the component and the reason. Tests: `sstable_health::tests` (8 cases, including `a_missing_data_file_is_unusable` and `the_first_defect_in_order_is_the_one_reported`). Residual: the manifest entry itself is not reconciled, so the generation is re-probed and re-quarantined on every boot. | ## Top risks to act on diff --git a/ferrosa-storage/src/engine.rs b/ferrosa-storage/src/engine.rs index 76a935bc..e7d9f790 100644 --- a/ferrosa-storage/src/engine.rs +++ b/ferrosa-storage/src/engine.rs @@ -4385,29 +4385,34 @@ impl StorageEngine { // a per-partition row index (ferrosa-sstable/src/writer.rs:212). // A 0-byte Rows.db is the expected output, not corruption; the // reader treats a missing/empty Rows.db as "no row index". - // Only Data.db and Partitions.db being zero-byte is unrecoverable. - let critical_components = ["Data.db", "Partitions.db"]; - let mut quarantine = false; - for comp in &critical_components { - let path = Self::generation_component_path(table_dir, &gen_str, comp) - .unwrap_or_else(|| table_dir.join(format!("{gen_str}-{comp}"))); - match std::fs::metadata(&path) { - Ok(meta) if meta.len() == 0 => { - quarantine = true; - } - Err(_) => { - // Missing component — will fail in open_sstable_from_dir. - } - _ => {} - } - } - if quarantine { + // Only Data.db and Partitions.db are critical. + // + // A MISSING component counts, not only a zero-byte one. This used + // to fall through on the reasoning that it "will fail in + // open_sstable_from_dir" -- and it does, as + // `storage: I/O error: No such file or directory (os error 2)` + // naming no table, no generation and no path, failing every read + // of that table rather than the one generation. Missing is + // strictly worse than empty and is caught in the same place. + let probes: Vec<(&str, crate::sstable_health::ComponentProbe)> = + crate::sstable_health::CRITICAL_COMPONENTS + .iter() + .map(|comp| { + let path = Self::generation_component_path(table_dir, &gen_str, comp) + .unwrap_or_else(|| table_dir.join(format!("{gen_str}-{comp}"))); + (*comp, crate::sstable_health::probe_component(&path)) + }) + .collect(); + let defect = crate::sstable_health::first_unusable_component(&probes); + if let Some((component, reason)) = defect { match repair_mode { StartupSstableRepairMode::Quarantine => { tracing::error!( gen, dir = %table_dir.display(), - "storage-engine: startup repair quarantining SSTable with zero-byte critical component" + component, + reason = reason.describe(), + "storage-engine: startup repair quarantining SSTable with an unusable critical component" ); let quarantine_dir = table_dir.join("quarantine"); let _ = std::fs::create_dir_all(&quarantine_dir); @@ -4422,7 +4427,9 @@ impl StorageEngine { gen, dir = %table_dir.display(), mode = ?repair_mode, - "storage-engine: startup repair excluded SSTable with zero-byte critical component from active readers; files remain in place for salvage" + component, + reason = reason.describe(), + "storage-engine: startup repair excluded SSTable with an unusable critical component from active readers; files remain in place for salvage" ); excluded_count += 1; } diff --git a/ferrosa-storage/src/lib.rs b/ferrosa-storage/src/lib.rs index 196553e7..5fba571c 100644 --- a/ferrosa-storage/src/lib.rs +++ b/ferrosa-storage/src/lib.rs @@ -40,6 +40,7 @@ pub mod schema_snapshot; pub mod self_heal; pub mod snapshot; pub mod spill_budget; +pub(crate) mod sstable_health; pub mod store; pub mod subscription_observer; #[cfg(feature = "test-generators")] diff --git a/ferrosa-storage/src/sstable_health.rs b/ferrosa-storage/src/sstable_health.rs new file mode 100644 index 00000000..c1d6efdb --- /dev/null +++ b/ferrosa-storage/src/sstable_health.rs @@ -0,0 +1,197 @@ +//! Module: Decide whether an SSTable generation on disk can serve reads. +//! +//! Correctness: Correct when every way a critical component can fail to serve +//! — absent, empty, or unreadable — withholds the generation from readers, and +//! when a component that is legitimately empty does not. +//! +//! Last revised: 2026-08-28 +//! Last changed: New. +//! +//! # Why this is a pure function and not an fs call +//! +//! The decision lives inside `load_existing_sstables_and_sidecars`, which walks +//! a data directory and needs an engine, a manifest and a disk to run at all. +//! The judgement it makes — is this generation servable — needs none of those, +//! and is the part that was wrong. Separated, it is testable without a cluster. + +/// What probing one component on disk found. +/// +/// `Missing` and `Unreadable` are distinct because they mean different things +/// to an operator: one is a file that is gone, the other is a file that is +/// there and cannot be stat'd. Both withhold the generation; only the message +/// differs. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ComponentProbe { + /// The component exists and holds this many bytes. + Present(u64), + /// The path does not exist. + Missing, + /// The path exists but could not be inspected. + Unreadable, +} + +/// Why a critical component cannot serve reads. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ComponentDefect { + /// Zero bytes. For a critical component this is unrecoverable. + Empty, + /// The file the manifest points at is not on disk. + Missing, + /// Present, but its metadata could not be read. + Unreadable, +} + +impl ComponentDefect { + /// How this reads in a log line an operator has to act on. + pub(crate) fn describe(self) -> &'static str { + match self { + Self::Empty => "zero-byte", + Self::Missing => "missing from disk", + Self::Unreadable => "unreadable", + } + } +} + +/// The components a generation cannot serve reads without. +/// +/// Rows.db is deliberately absent. The SSTable writer emits a zero-byte +/// Rows.db for simple partitions that need no per-partition row index, and the +/// reader treats a missing or empty Rows.db as "no row index". Treating it as +/// critical would quarantine healthy SSTables. +pub(crate) const CRITICAL_COMPONENTS: [&str; 2] = ["Data.db", "Partitions.db"]; + +/// The first critical component that cannot serve reads, if any. +/// +/// Returns the first rather than all of them because the caller's decision is +/// binary — withhold the generation or do not — and naming one concrete file +/// is what makes the log line actionable. Order follows `probes`, so the +/// message is deterministic when several components are defective. +pub(crate) fn first_unusable_component<'a>( + probes: &[(&'a str, ComponentProbe)], +) -> Option<(&'a str, ComponentDefect)> { + probes + .iter() + .find_map(|(name, probe)| component_defect(*probe).map(|defect| (*name, defect))) +} + +/// Whether one probed component can serve reads. +const fn component_defect(probe: ComponentProbe) -> Option { + match probe { + ComponentProbe::Present(0) => Some(ComponentDefect::Empty), + ComponentProbe::Present(_) => None, + // A component the manifest references but disk does not have cannot be + // read, and deferring that to the read path costs an operator a whole + // table: the failure that reaches them is a bare ENOENT naming no + // file, no generation and no table. + ComponentProbe::Missing => Some(ComponentDefect::Missing), + ComponentProbe::Unreadable => Some(ComponentDefect::Unreadable), + } +} + +/// Probe one component path, mapping io failures onto the states above. +pub(crate) fn probe_component(path: &std::path::Path) -> ComponentProbe { + match std::fs::metadata(path) { + Ok(meta) => ComponentProbe::Present(meta.len()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => ComponentProbe::Missing, + Err(_) => ComponentProbe::Unreadable, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn probes( + data: ComponentProbe, + partitions: ComponentProbe, + ) -> Vec<(&'static str, ComponentProbe)> { + vec![("Data.db", data), ("Partitions.db", partitions)] + } + + /// The ordinary healthy generation. + #[test] + fn a_generation_whose_components_all_hold_bytes_is_servable() { + let probed = probes(ComponentProbe::Present(4_096), ComponentProbe::Present(512)); + assert_eq!(first_unusable_component(&probed), None); + } + + /// Pins the behaviour that already existed: a zero-byte critical component + /// is unrecoverable and must be withheld. + #[test] + fn a_zero_byte_data_file_is_unusable() { + let probed = probes(ComponentProbe::Present(0), ComponentProbe::Present(512)); + assert_eq!( + first_unusable_component(&probed), + Some(("Data.db", ComponentDefect::Empty)) + ); + } + + /// The defect this module exists for. + /// + /// Startup repair used to treat a MISSING component as benign while + /// treating a zero-byte one as fatal, on the reasoning that it "will fail + /// in open_sstable_from_dir". It does — as `storage: I/O error: No such + /// file or directory (os error 2)` with no table, no generation and no + /// path, failing every read of that table. Missing is strictly worse than + /// empty and must be caught in the same place. + #[test] + fn a_missing_data_file_is_unusable() { + let probed = probes(ComponentProbe::Missing, ComponentProbe::Present(512)); + assert_eq!( + first_unusable_component(&probed), + Some(("Data.db", ComponentDefect::Missing)) + ); + } + + /// The other critical component, so the rule is not accidentally + /// Data.db-only. + #[test] + fn a_missing_partitions_file_is_unusable() { + let probed = probes(ComponentProbe::Present(4_096), ComponentProbe::Missing); + assert_eq!( + first_unusable_component(&probed), + Some(("Partitions.db", ComponentDefect::Missing)) + ); + } + + /// Present but unstat-able is still unservable, and says so differently. + #[test] + fn an_unreadable_component_is_unusable() { + let probed = probes(ComponentProbe::Unreadable, ComponentProbe::Present(512)); + assert_eq!( + first_unusable_component(&probed), + Some(("Data.db", ComponentDefect::Unreadable)) + ); + } + + /// Deterministic reporting: with two defects the message names the first, + /// so the same broken generation does not log a different file each boot. + #[test] + fn the_first_defect_in_order_is_the_one_reported() { + let probed = probes(ComponentProbe::Missing, ComponentProbe::Present(0)); + assert_eq!( + first_unusable_component(&probed), + Some(("Data.db", ComponentDefect::Missing)) + ); + } + + /// Rows.db is legitimately zero-byte for simple partitions, so it must not + /// be in the critical set — including it would quarantine healthy tables. + #[test] + fn rows_db_is_not_a_critical_component() { + assert!(!CRITICAL_COMPONENTS.contains(&"Rows.db")); + assert_eq!(CRITICAL_COMPONENTS, ["Data.db", "Partitions.db"]); + } + + /// The defects an operator reads are distinguishable. + #[test] + fn each_defect_describes_itself_distinctly() { + let all = [ + ComponentDefect::Empty, + ComponentDefect::Missing, + ComponentDefect::Unreadable, + ]; + let described: std::collections::BTreeSet<_> = all.iter().map(|d| d.describe()).collect(); + assert_eq!(described.len(), all.len(), "two defects read the same"); + } +}