fix(registry): quarantine vector-sidecar load failures like passages (#677 item 3)

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<Self, ()> {
+        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<Self, ()> {
+        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()),
         }
     }
 

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<Option<Arc<VectorStore>>>,
+    /// 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<Option<std::time::Instant>>,
     /// 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<Option<Arc<PassageVectorStore>>>,
+    /// `vectors_load_failure`'s mirror for `passage_vectors` — see that
+    /// field's doc.
+    passage_vectors_load_failure: Mutex<Option<std::time::Instant>>,
     /// 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<PassageVectorStore> {
         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

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<VectorStore> {
         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())
+            }
         }
     }
 }

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