diff --git a/src/embedding.rs b/src/embedding.rs index 85143533..0bab72b7 100644 --- a/src/embedding.rs +++ b/src/embedding.rs @@ -962,13 +962,31 @@ impl VectorStore { /// [`crate::storage::read_sidecar`] — otherwise a permanently /// unreadable sidecar pays a full re-embed every residency with /// nothing in the logs to say why. + /// + /// A thin wrapper over [`VectorStore::load_checked`] for a test + /// reading a sidecar back to assert its contents, which does not + /// care whether an empty result would mean "nothing written yet" + /// or "a read failed" — production code always goes through + /// `load_checked` directly (`AppState::entry_vectors`), the caller + /// that does care. + #[cfg(test)] pub fn load(path: &Path) -> Self { - match crate::storage::read_sidecar(path, "vector store") { - Some(bytes) => Self::from_bytes(&bytes).unwrap_or_else(|| { + Self::load_checked(path).unwrap_or_default() + } + + /// Like [`VectorStore::load`], but reports a genuine read failure + /// as `Err(())` instead of folding it into the same empty default + /// a cold, never-embedded context also returns — the distinction + /// [`AppState::entry_vectors`](crate::registry::AppState) needs to + /// quarantine a disk hiccup (issue #677 item 3) instead of caching + /// its empty answer as if the context were genuinely bare. + pub fn load_checked(path: &Path) -> Result { + match crate::storage::read_sidecar_checked(path, "vector store")? { + Some(bytes) => Ok(Self::from_bytes(&bytes).unwrap_or_else(|| { tracing::warn!("ignoring corrupt vector store at {}", path.display()); Self::default() - }), - None => Self::default(), + })), + None => Ok(Self::default()), } } @@ -1302,9 +1320,29 @@ impl PassageVectorStore { /// [`crate::storage::read_sidecar`] — otherwise a permanently /// unreadable sidecar pays a full re-embed every residency with /// nothing in the logs to say why. + /// + /// A thin wrapper over [`PassageVectorStore::load_checked`] for a + /// test reading a sidecar back to assert its contents, which does + /// not care whether an empty result would mean "nothing written + /// yet" or "a read failed" — production code always goes through + /// `load_checked` directly (`AppState::entry_passage_vectors`), the + /// caller that does care. + #[cfg(test)] pub fn load(path: &Path) -> Self { - match crate::storage::read_sidecar(path, "passage vector store") { - Some(bytes) => Self::from_bytes(&bytes).unwrap_or_else(|| { + Self::load_checked(path).unwrap_or_default() + } + + /// Like [`PassageVectorStore::load`], but reports a genuine read + /// failure as `Err(())` instead of folding it into the same empty + /// default a cold, never-embedded context also returns — the + /// distinction + /// [`AppState::entry_passage_vectors`](crate::registry::AppState) + /// needs to quarantine a disk hiccup (issue #677 item 3) instead of + /// caching its empty answer as if the context genuinely had no + /// passages embedded. + pub fn load_checked(path: &Path) -> Result { + match crate::storage::read_sidecar_checked(path, "passage vector store")? { + Some(bytes) => Ok(Self::from_bytes(&bytes).unwrap_or_else(|| { if bytes.get(..8) == Some(LEGACY_PASSAGE_VECTOR_MAGIC.as_slice()) { tracing::info!( "passage vectors at {} predate doc2query; re-embedding", @@ -1317,8 +1355,8 @@ impl PassageVectorStore { ); } Self::default() - }), - None => Self::default(), + })), + None => Ok(Self::default()), } } diff --git a/src/registry.rs b/src/registry.rs index 0ca558fe..491ad0bd 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -465,6 +465,15 @@ pub struct Entry { /// by refresh, cleared by eviction, and counted against the cache /// budget. Lock order: `inner` before `vectors`, never the reverse. vectors: Mutex>>, + /// The vector sidecar's last failed load, while it is being + /// remembered — `passages_load_failure`'s counterpart, so a + /// transient disk hiccup at load time quarantines and later + /// retries (`AppState::entry_vectors`) instead of caching an empty + /// store into `vectors` above for the rest of this residency + /// (issue #677 item 3). Only ever locked while `vectors` is held + /// (or alone by the test aging helper), so it adds no lock-order + /// edge. + vectors_load_failure: Mutex>, /// Serializes whole gloss-embedding refreshes — the same reason as /// `passage_refresh` below: a diff computed outside the entry lock /// (provider round trips can take seconds) races on which of two @@ -511,6 +520,9 @@ pub struct Entry { /// vector lane's mirror of `vectors`, in its own slot so resolve's /// small hot gloss store never shares a fate with this big one. passage_vectors: Mutex>>, + /// `vectors_load_failure`'s mirror for `passage_vectors` — see that + /// field's doc. + passage_vectors_load_failure: Mutex>, /// Set when a passage store/retract lands, cleared when a passage /// embedding refresh claims it — the auto-refresh ticker's signal /// (passage writes do not mark the GRAPH dirty, so the flush list @@ -645,12 +657,14 @@ impl Entry { flushing: AtomicBool::new(false), last_touch: AtomicU64::new(0), vectors: Mutex::new(None), + vectors_load_failure: Mutex::new(None), vectors_refresh: Mutex::new(()), vectors_save_pending: AtomicBool::new(false), passages: Mutex::new(None), bm25: RwLock::new(None), bm25_dirty: AtomicBool::new(false), passage_vectors: Mutex::new(None), + passage_vectors_load_failure: Mutex::new(None), passages_embed_dirty: AtomicBool::new(false), passage_refresh: Mutex::new(()), usage: UsageCounters::seeded(&usage), @@ -2848,18 +2862,37 @@ impl AppState { /// The paragraph vector sidecar, loaded on first use and held until /// refresh replaces it or eviction clears it. + /// + /// Mirrors `entry_vectors`'s quarantine (issue #677 item 3, see + /// that function's doc): a genuine read failure is not cached into + /// `passage_vectors`, only handed back as an uncached empty + /// default, so the next call retries the disk once + /// `LOAD_FAILURE_RETRY` has passed instead of serving a stale + /// "empty" answer for the rest of this residency. fn entry_passage_vectors(&self, entry: &Entry, stem: &str) -> Arc { let mut cached = entry.passage_vectors.lock(); - match &*cached { - Some(store) => Arc::clone(store), - None => { - let store = Arc::new(PassageVectorStore::load(&pvectors_path( - &self.0.data_dir, - stem, - ))); + if let Some(store) = &*cached { + return Arc::clone(store); + } + { + let failure = entry.passage_vectors_load_failure.lock(); + if let Some(failed_at) = &*failure + && still_quarantined(failed_at) + { + return Arc::new(PassageVectorStore::default()); + } + } + match PassageVectorStore::load_checked(&pvectors_path(&self.0.data_dir, stem)) { + Ok(store) => { + *entry.passage_vectors_load_failure.lock() = None; + let store = Arc::new(store); *cached = Some(Arc::clone(&store)); store } + Err(()) => { + *entry.passage_vectors_load_failure.lock() = Some(std::time::Instant::now()); + Arc::new(PassageVectorStore::default()) + } } } @@ -2986,9 +3019,9 @@ impl AppState { Ok(result) } - /// Test-only: rewinds any remembered load failure (graph image and - /// passage store both) so the quarantine window can elapse without - /// the test sleeping through it. + /// Test-only: rewinds any remembered load failure (graph image, + /// passage store, and both vector sidecars) so the quarantine + /// window can elapse without the test sleeping through it. #[cfg(test)] pub fn age_load_failures(&self, name: &str, by: std::time::Duration) { let entry = self.lookup(name).expect("the context must be registered"); @@ -3002,6 +3035,16 @@ impl AppState { .checked_sub(by) .expect("test ages within the Instant range"); } + if let Some(failed_at) = &mut *entry.vectors_load_failure.lock() { + *failed_at = failed_at + .checked_sub(by) + .expect("test ages within the Instant range"); + } + if let Some(failed_at) = &mut *entry.passage_vectors_load_failure.lock() { + *failed_at = failed_at + .checked_sub(by) + .expect("test ages within the Instant range"); + } } /// Runs a mutating operation on one context, loading it first if diff --git a/src/registry/embeddings.rs b/src/registry/embeddings.rs index 9b8c7c75..35be965d 100644 --- a/src/registry/embeddings.rs +++ b/src/registry/embeddings.rs @@ -13,7 +13,7 @@ use crate::hash::fnv1a; use super::{ AccessError, AppState, EmbeddingsStatus, Entry, GlossLaneReport, GlossSidecarStatus, PassageRefreshOutcome, PassageSidecarStatus, SEMANTIC_RESOLVE_LIMIT, - dispatch_chunks_concurrently, file_stem, pvectors_path, vectors_path, + dispatch_chunks_concurrently, file_stem, pvectors_path, still_quarantined, vectors_path, }; /// Rows per provider call, shared by both the gloss (`embed_stale`) and @@ -1190,15 +1190,40 @@ impl AppState { /// The entry's vector store, loaded from its sidecar on first use /// and held until refresh replaces it or eviction clears it. + /// + /// A genuine read failure (not simply "nothing embedded yet") is + /// quarantined exactly like `entry_passages`' load failure (issue + /// #677 item 3): the failed attempt is NOT cached into `vectors` — + /// only an empty, uncached default is handed back, and the next + /// call retries the disk once `LOAD_FAILURE_RETRY` has passed. + /// Before this, a transient disk hiccup at load time cached an + /// empty store forever (until an explicit refresh or eviction), so + /// semantic search silently found nothing for the rest of that + /// residency even after the disk recovered. fn entry_vectors(&self, entry: &Entry, stem: &str) -> Arc { let mut cached = entry.vectors.lock(); - match &*cached { - Some(store) => Arc::clone(store), - None => { - let store = Arc::new(VectorStore::load(&vectors_path(&self.0.data_dir, stem))); + if let Some(store) = &*cached { + return Arc::clone(store); + } + { + let failure = entry.vectors_load_failure.lock(); + if let Some(failed_at) = &*failure + && still_quarantined(failed_at) + { + return Arc::new(VectorStore::default()); + } + } + match VectorStore::load_checked(&vectors_path(&self.0.data_dir, stem)) { + Ok(store) => { + *entry.vectors_load_failure.lock() = None; + let store = Arc::new(store); *cached = Some(Arc::clone(&store)); store } + Err(()) => { + *entry.vectors_load_failure.lock() = Some(std::time::Instant::now()); + Arc::new(VectorStore::default()) + } } } } diff --git a/src/registry/embeddings/gloss_tests.rs b/src/registry/embeddings/gloss_tests.rs index ed96d77f..87610a53 100644 --- a/src/registry/embeddings/gloss_tests.rs +++ b/src/registry/embeddings/gloss_tests.rs @@ -2682,4 +2682,83 @@ mod tests { let _ = fs::remove_dir_all(dir); } + + /// Issue #677 item 3: a genuine read failure on the vector sidecar + /// must not be cached as if the context were simply empty — that + /// would silently degrade semantic search for the rest of this + /// residency even after the disk recovers. Mirrors + /// `a_failed_passage_load_is_quarantined_like_the_image` + /// (`registry/passages.rs`), the same TTL quarantine + /// `entry_passages` already had. + #[test] + fn a_failed_vector_load_is_quarantined_then_recovers() { + let dir = scratch_dir("vectors-quarantine"); + { + let calls = Arc::new(AtomicUsize::new(0)); + let embedder = + Some(Arc::new(MockEmbeddings::fruity(&calls)) as Arc); + let state = AppState::boot(dir.clone(), usize::MAX, embedder).unwrap(); + state + .create("fruit", ContextMeta::default()) + .map_err(|_| "create") + .unwrap(); + state + .write_context("fruit", |context| { + context.associate("りんご", "l", "アップル", 1.0).unwrap(); + }) + .map_err(|_| "write") + .unwrap(); + state + .refresh_embeddings("fruit", Deadline::unbounded()) + .unwrap() + .unwrap(); + state.flush_dirty(); + } + + // A directory at the sidecar's path is unreadable as a file + // (`fs::read` fails with `IsADirectory`/`ENOENT`-adjacent + // errors) regardless of the running process's own privileges — + // unlike a permission bit, which root or `CAP_DAC_OVERRIDE` + // (common in CI containers) simply ignores, making that + // approach non-deterministic (CodeRabbit, PR #689). + let path = vectors_path(&dir, &file_stem("fruit")); + let healthy = fs::read(&path).unwrap(); + fs::remove_file(&path).unwrap(); + fs::create_dir(&path).unwrap(); + + let state = AppState::boot(dir.clone(), usize::MAX, None).unwrap(); + let entry = state.lookup("fruit").unwrap(); + + // Unreadable: an empty store, degraded rather than a panic or + // a propagated error — but NOT cached, unlike a genuinely + // empty context. + let degraded = state.entry_vectors(&entry, &file_stem("fruit")); + assert!(degraded.concepts.is_empty() && degraded.labels.is_empty()); + assert!( + entry.vectors.lock().is_none(), + "a load failure must not be cached as if the context were genuinely empty" + ); + + fs::remove_dir(&path).unwrap(); + fs::write(&path, &healthy).unwrap(); + + // Still within the quarantine window: the disk is not re-read + // yet even though it has already recovered — same posture as + // `entry_passages`. + let still_degraded = state.entry_vectors(&entry, &file_stem("fruit")); + assert!(still_degraded.concepts.is_empty() && still_degraded.labels.is_empty()); + + state.age_load_failures("fruit", crate::registry::LOAD_FAILURE_RETRY); + let recovered = state.entry_vectors(&entry, &file_stem("fruit")); + assert!( + !recovered.concepts.is_empty() || !recovered.labels.is_empty(), + "the quarantine window passing must trigger a real retry, not stay stuck empty forever" + ); + assert!( + entry.vectors.lock().is_some(), + "a genuinely successful load must be cached like any other" + ); + + let _ = fs::remove_dir_all(&dir); + } } diff --git a/src/registry/embeddings/passage_tests.rs b/src/registry/embeddings/passage_tests.rs index aaba76f7..a9c395b7 100644 --- a/src/registry/embeddings/passage_tests.rs +++ b/src/registry/embeddings/passage_tests.rs @@ -1135,4 +1135,83 @@ mod tests { let _ = fs::remove_dir_all(dir); } + + /// Issue #677 item 3, passage side — mirrors gloss_tests.rs's + /// `a_failed_vector_load_is_quarantined_then_recovers`: a genuine + /// read failure on the paragraph vector sidecar must not be cached + /// as if the context had no passages embedded, or the vector lane + /// stays silently empty for the rest of this residency even after + /// the disk recovers. + #[test] + fn a_failed_passage_vector_load_is_quarantined_then_recovers() { + let dir = scratch_dir("pvec-quarantine"); + { + let calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let state = + boot_for_passage_embedding(&dir, Arc::new(MockEmbeddings::fruity(&calls)), 20_000); + state + .create("sake", ContextMeta::default()) + .map_err(|_| "create") + .unwrap(); + let mut passages = BTreeMap::new(); + passages.insert("doc-a".to_string(), "りんごの段落。".to_string()); + state + .store_passages("sake", plain(passages)) + .unwrap() + .unwrap(); + state + .refresh_passage_embeddings("sake", Deadline::unbounded()) + .unwrap() + .unwrap(); + state.flush_dirty(); + } + + // A directory at the sidecar's path is unreadable as a file + // regardless of the running process's own privileges — unlike + // a permission bit, which root or `CAP_DAC_OVERRIDE` (common + // in CI containers) simply ignores, making that approach + // non-deterministic (CodeRabbit, PR #689). + let path = pvectors_path(&dir, &file_stem("sake")); + let healthy = fs::read(&path).unwrap(); + fs::remove_file(&path).unwrap(); + fs::create_dir(&path).unwrap(); + + let state = boot_for_passage_embedding( + &dir, + Arc::new(MockEmbeddings::fruity(&Arc::new( + std::sync::atomic::AtomicUsize::new(0), + ))), + 20_000, + ); + let entry = state.lookup("sake").unwrap(); + + let degraded = state.entry_passage_vectors(&entry, &file_stem("sake")); + assert_eq!(degraded.len(), 0); + assert!( + entry.passage_vectors.lock().is_none(), + "a load failure must not be cached as if the context were genuinely empty" + ); + + fs::remove_dir(&path).unwrap(); + fs::write(&path, &healthy).unwrap(); + + // Still within the quarantine window: the disk is not re-read + // yet even though it has already recovered. + let still_degraded = state.entry_passage_vectors(&entry, &file_stem("sake")); + assert_eq!(still_degraded.len(), 0); + + state.age_load_failures("sake", crate::registry::LOAD_FAILURE_RETRY); + let recovered = state.entry_passage_vectors(&entry, &file_stem("sake")); + assert_eq!( + recovered.len(), + 1, + "the quarantine window passing must trigger a real retry, not stay stuck empty forever" + ); + assert!( + entry.passage_vectors.lock().is_some(), + "a genuinely successful load must be cached like any other" + ); + + let _ = fs::remove_dir_all(dir); + } } diff --git a/src/registry/replication.rs b/src/registry/replication.rs index f30140a7..0fee5338 100644 --- a/src/registry/replication.rs +++ b/src/registry/replication.rs @@ -129,6 +129,8 @@ impl AppState { *entry.passage_vectors.lock() = None; *entry.vectors.lock() = None; *entry.passages_load_failure.lock() = None; + *entry.passage_vectors_load_failure.lock() = None; + *entry.vectors_load_failure.lock() = None; if inner.meta.pinned { if let Err(error) = ensure_hot( &self.0.data_dir, diff --git a/src/storage.rs b/src/storage.rs index 5e9fc56f..5943b054 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -371,9 +371,25 @@ pub(crate) fn sidecar_read_worth_warning(error: &io::Error) -> bool { /// (which the caller already warns on for its own bytes), a read that /// never got bytes at all was silent everywhere before this: `what` /// names the sidecar kind for [`sidecar_read_worth_warning`]'s warning. +/// +/// A thin wrapper over [`read_sidecar_checked`] that discards the +/// missing-vs-failed distinction that function keeps — the right +/// choice for a caller that already treats "nothing to load" and "an +/// unreadable file" the same way (a derived cache rebuilds either way). pub(crate) fn read_sidecar(path: &Path, what: &str) -> Option> { + read_sidecar_checked(path, what).unwrap_or(None) +} + +/// Like [`read_sidecar`], but keeps the one distinction that function +/// discards: `Ok(None)` is the benign case (nothing written yet — +/// nothing worth remembering), `Err(())` is a genuine read failure, +/// already warned about here, that a caller wanting to QUARANTINE +/// against (rather than silently re-treat as "empty" on every access) +/// needs to tell apart — see `AppState::entry_vectors`/ +/// `entry_passage_vectors` (issue #677 item 3). +pub(crate) fn read_sidecar_checked(path: &Path, what: &str) -> Result>, ()> { match fs::read(path) { - Ok(bytes) => Some(bytes), + Ok(bytes) => Ok(Some(bytes)), Err(error) => { if sidecar_read_worth_warning(&error) { // Never a bare `error` field — see the ADR 0008 note on @@ -383,8 +399,10 @@ pub(crate) fn read_sidecar(path: &Path, what: &str) -> Option> { read_error = %error, "{what} sidecar unreadable; costing a full rebuild every residency until fixed", ); + Err(()) + } else { + Ok(None) } - None } } }