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"); + } +}