fix: share the per-repo SCIP LMDB env; clear frozen C# index error state

CHANGELOG.md

@@ -18,6 +18,8 @@ finalized in place with a date — no renaming/migration step needed.
 
 ### Fixed
 
+- **SCIP symbol index: every query/rebuild opened its own LMDB env on `db_path/scip` — overlapping operations failed each other with `LMDB double-open prevented`, then rendered as a frozen red `C#!` in the TUI.** LMDB permits one open env per directory per process, and the C# and TypeScript adapters share that directory, so a watcher-triggered rebuild starting while a lazy `find-refs` call held its env (minutes on large Roslyn workspaces), or any C#↔TS overlap on the same repo, tripped the double-open guard and the loser failed outright. Observed in production as `C# symbol rebuild failed: LMDB double-open prevented … (SCIP(…), opened 35.2s ago)` on an actively-used repo — with the error state then frozen on the TUI for days, because `repo_statuses_lightweight()` prefers the cached status over its on-disk probe and a closed repo has no watcher left to retry a rebuild and flip it back to Ready. Two fixes shipped together. (1) **Shared per-directory env**: all SCIP opens now route through `get_or_open_shared_env` (a Weak-ref cache in `lmdb_registry`), handing every concurrent user the same `Arc<TrackedEnv>` — writers serialise on LMDB's single-writer mutex, readers never block, and the C#/TS adapters share one env (5 named DBs pre-created once per env session). The cache never keeps an env alive, so `remove_repo`'s holder-drain is unaffected: drop of the last Arc still closes and frees the registry slot (strengthened by the `prepare_for_closing` fix above). (2) **Error state lifecycle**: idle eviction and force-reindex close now clear `csharp_index_status`/`csharp_index_error` for the alias, letting the on-disk probe (helper available + index exists → Ready) restore the truth instead of a permanent red `!`. Also hoists the `scip_meta` DB name to `constants::SCIP_META_DB_NAME` (previously a duplicated literal in both adapters).
+
 - **`TrackedEnv` drop never actually closed the heed environment — the real cause of `index rm`'s deterministic Windows failure (os error 32).** heed 0.20's process-global `OPENED_ENV` cache holds a strong `Env` clone inside its entry, so dropping the last user-side `Env` leaves the Arc count at exactly 1 (the entry's own) and `mdb_env_close` never runs. On POSIX that silently leaks an fd + mmap; on Windows it locks `data.mdb`/`lock.mdb` against deletion for the life of the process — which made `serve::tests::index_rm_deletes_db_while_serve_holds_real_lmdb_env` fail deterministically through the whole 60 s retry budget with an *empty* LMDB registry (the holder is invisible to it: the registry tracks `TrackedEnv`s, not heed-internal Arc clones), and would equally block a real `codesearch index rm` against a running serve from ever deleting the DB dir. Diagnosed with a minimal serve-free repro (open `SharedStores` → drop → `data.mdb` still locked, `env_closing_event` still `Some`). `TrackedEnv::drop` now closes via `Env::prepare_for_closing()` — heed's one real close path, which takes the entry's reference out and closes synchronously — before freeing our own registry slot (the existing slot-ordering invariant is preserved and now actually holds). Regression tests pin both levels: `drop_really_closes_heed_env_and_releases_the_files` (registry level: heed's `OPENED_ENV` entry must be gone after drop, db dir deletable) and `sharedstores_drop_releases_db_dir_for_deletion` (production shape, serve-free, instant). The previously failing acceptance test now passes 3/3 in ~0.6 s instead of failing in ~62 s; full lib suite 612/612.
 
 ## [1.3.3] - 2026-08-18

src/constants.rs

@@ -611,6 +611,9 @@ pub const SCIP_CSHARP_DEBOUNCE_MS: u64 = 60_000; // 60 seconds
 /// LMDB database name for the SCIP symbols table.
 pub const SCIP_SYMBOLS_DB_NAME: &str = "scip_symbols";
 
+/// LMDB database name for the SCIP per-repo metadata table.
+pub const SCIP_META_DB_NAME: &str = "scip_meta";
+
 /// LMDB metadata key for the last rebuild timestamp.
 pub const SCIP_REBUILD_TIMESTAMP_KEY: &str = "last_rebuild_ts";
 

src/lmdb_registry.rs

@@ -14,7 +14,7 @@ use dashmap::DashMap;
 use std::mem::ManuallyDrop;
 use std::ops::Deref;
 use std::path::{Path, PathBuf};
-use std::sync::OnceLock;
+use std::sync::{Arc, OnceLock, Weak};
 use std::time::Instant;
 
 use crate::cache::safe_canonicalize;
@@ -140,6 +140,77 @@ pub fn open_holders_under(path: &Path) -> Vec<String> {
     }
 }
 
+// ── Shared-env cache ────────────────────────────────────────────
+
+/// Process-wide cache of LMDB environments that multiple components must use
+/// CONCURRENTLY (queries, rebuilds, per-language adapters on the same
+/// directory). Holds only [`Weak`] references: the cache never keeps an
+/// environment alive, it just hands out the live one when it exists. When the
+/// last user drops their `Arc`, the [`TrackedEnv`] drops, its registry slot
+/// frees, and the stale cache entry is reaped on the next lookup.
+static SHARED_ENVS: OnceLock<DashMap<PathBuf, Weak<TrackedEnv>>> = OnceLock::new();
+
+/// Open the environment at `path`, or return the already-open shared instance.
+///
+/// LMDB permits exactly one open environment per directory per process, so
+/// callers that may overlap in time (a rebuild vs. an in-flight query, the C#
+/// vs. TypeScript adapters on the same `db_path/scip`) must not each open
+/// their own — the second [`TrackedEnv::open`] trips the double-open guard and
+/// one side fails outright. This getter makes the collision impossible: the
+/// first caller opens (running `init` once to create the named databases) and
+/// everyone else receives a clone of the same `Arc`.
+///
+/// `build_opts` configures the [`heed::EnvOpenOptions`] and `init` runs once
+/// per environment lifetime, right after the open, before the handle is
+/// published to other threads. Writers then serialise on LMDB's own
+/// single-writer mutex and readers never block.
+///
+/// Both closures run while the cache's shard lock is held: they must not
+/// re-enter `get_or_open_shared_env` (a path hashing to the same shard
+/// self-deadlocks) and should stay cheap.
+///
+/// The env-var lookups inside `build_opts`/`init` run only when the directory
+/// is opened for the first time in this process — later override changes do
+/// not affect an already-shared environment.
+pub fn get_or_open_shared_env(
+    path: &Path,
+    description: &str,
+    build_opts: impl FnOnce(&mut heed::EnvOpenOptions),
+    init: impl FnOnce(&TrackedEnv) -> Result<()>,
+) -> Result<Arc<TrackedEnv>> {
+    let canonical = safe_canonicalize(path)
+        .with_context(|| format!("Cannot canonicalize LMDB path: {}", path.display()))?;
+    let cache = SHARED_ENVS.get_or_init(DashMap::new);
+
+    loop {
+        use dashmap::mapref::entry::Entry;
+        match cache.entry(canonical.clone()) {
+            Entry::Occupied(occupied) => {
+                if let Some(env) = occupied.get().upgrade() {
+                    return Ok(env);
+                }
+                // Last Arc dropped but the entry survived — reap and retry so
+                // the Vacant arm below performs a fresh open.
+                occupied.remove();
+            }
+            Entry::Vacant(vacant) => {
+                let mut opts = heed::EnvOpenOptions::new();
+                build_opts(&mut opts);
+                // SAFETY: caller contract — same as `TrackedEnv::open`. The
+                // registry slot guards against any concurrent direct open.
+                let tracked = unsafe { TrackedEnv::open(&opts, path, description)? };
+                // Run init before publishing: the `?` early-return drops
+                // `tracked`, freeing the registry slot, so a failed init
+                // leaves the path openable.
+                init(&tracked)?;
+                let env = Arc::new(tracked);
+                vacant.insert(Arc::downgrade(&env));
+                return Ok(env);
+            }
+        }
+    }
+}
+
 // ── TrackedEnv wrapper ──────────────────────────────────────────
 
 /// Wrapper around [`heed::Env`] that prevents double-open panics.
@@ -377,6 +448,104 @@ mod tests {
         let _env2 = unsafe { TrackedEnv::open(&opts, dir2.path(), "test-2").unwrap() };
     }
 
+    fn shared_opts(opts: &mut heed::EnvOpenOptions) {
+        opts.map_size(1024 * 1024).max_dbs(4);
+        // Every test open carries the baseline flags per the AGENTS.md rule.
+        unsafe { opts.flags(BASE_ENV_FLAGS) };
+    }
+
+    /// Direct-open options IDENTICAL to `shared_opts`. heed refuses to reopen
+    /// a path with different options (max_dbs included) even after the prior
+    /// env dropped — the AGENTS.md "same options on every open" rule — so the
+    /// direct opens in the shared-env tests must not reuse `make_opts`
+    /// (max_dbs 1).
+    fn shared_compatible_opts() -> heed::EnvOpenOptions {
+        let mut opts = heed::EnvOpenOptions::new();
+        shared_opts(&mut opts);
+        opts
+    }
+
+    /// Two callers of the shared getter receive the SAME environment — this is
+    /// the property whose absence made a watcher rebuild fail with
+    /// `LMDB double-open prevented` while a lazy find-refs held its own env.
+    #[test]
+    fn shared_env_returns_live_instance_to_concurrent_callers() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().to_path_buf();
+
+        let env1 = get_or_open_shared_env(&path, "shared-1", shared_opts, |_| Ok(())).unwrap();
+        let env2 = get_or_open_shared_env(&path, "shared-2", shared_opts, |_| Ok(())).unwrap();
+        assert!(Arc::ptr_eq(&env1, &env2));
+
+        // While the shared env is alive, a DIRECT open on the same path must
+        // still trip the guard — the shared env genuinely occupies the slot.
+        let direct = unsafe { TrackedEnv::open(&shared_compatible_opts(), &path, "direct") };
+        let err = direct.unwrap_err().to_string();
+        assert!(err.contains("double-open prevented"));
+        assert!(err.contains("shared-1"));
+    }
+
+    /// The cache holds only weak refs: once the last user drops their Arc the
+    /// registry slot frees (a direct open succeeds) and the next shared caller
+    /// transparently reopens.
+    #[test]
+    fn shared_env_reopens_after_all_arcs_drop() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().to_path_buf();
+
+        {
+            let _env = get_or_open_shared_env(&path, "shared-1", shared_opts, |_| Ok(())).unwrap();
+        }
+
+        // Slot freed after the last Arc dropped.
+        {
+            let _direct =
+                unsafe { TrackedEnv::open(&shared_compatible_opts(), &path, "direct").unwrap() };
+        }
+
+        // And the shared getter opens fresh again.
+        let _again = get_or_open_shared_env(&path, "shared-2", shared_opts, |_| Ok(())).unwrap();
+    }
+
+    /// An `init` failure must not leak the registry slot: the error propagates
+    /// and the env is dropped, leaving the path openable.
+    #[test]
+    fn shared_env_init_failure_frees_slot() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().to_path_buf();
+
+        let result = get_or_open_shared_env(&path, "shared-1", shared_opts, |_| {
+            Err(anyhow::anyhow!("boom"))
+        });
+        assert!(result.is_err());
+
+        let _direct =
+            unsafe { TrackedEnv::open(&shared_compatible_opts(), &path, "direct").unwrap() };
+    }
+
+    /// N threads racing the FIRST open all succeed and all hold the same env —
+    /// no thread sees the double-open error the per-caller open produced.
+    #[test]
+    fn shared_env_concurrent_first_open_is_safe() {
+        let dir = TempDir::new().unwrap();
+        let path = Arc::new(dir.path().to_path_buf());
+
+        let handles: Vec<_> = (0..8)
+            .map(|_| {
+                let p = Arc::clone(&path);
+                std::thread::spawn(move || {
+                    get_or_open_shared_env(&p, "shared-race", shared_opts, |_| Ok(()))
+                })
+            })
+            .collect();
+
+        let envs: Vec<_> = handles
+            .into_iter()
+            .map(|h| h.join().expect("thread panicked").expect("open failed"))
+            .collect();
+        assert!(envs.windows(2).all(|w| Arc::ptr_eq(&w[0], &w[1])));
+    }
+
     /// `open_holders_under` reports every live env at or under the queried
     /// path — the precondition check `remove_repo`'s lock-class retry waits
     /// on. Must see (a) an env whose path IS the queried path and (b) an env

src/serve/mod.rs

@@ -1509,9 +1509,28 @@ impl ServeState {
         None
     }
 
+    /// Drop cached C# symbol-index status/error for `alias`.
+    ///
+    /// `repo_statuses_lightweight()` prefers these cached entries over its
+    /// on-disk probe, so a cached `Error` outlives the repo itself: a closed
+    /// repo has no watcher left to retry a rebuild or emit `Succeeded`, and
+    /// the red `C#!` it causes in the TUI freezes forever (observed on a repo
+    /// whose rebuild lost a one-shot LMDB double-open race days earlier).
+    /// Remove the entries letting the probe (helper available + index
+    /// exists → Ready) restore the on-disk truth. Called from idle eviction
+    /// and `close_repo` (force-reindex reopen). `remove_repo` deliberately
+    /// does NOT call this: once the alias is unregistered the entries are
+    /// display-unreachable (`repo_statuses_lightweight` iterates registered
+    /// repos only), so a clear there would be dead code.
+    fn clear_csharp_index_state(&self, alias: &str) {
+        self.csharp_index_status.remove(alias);
+        self.csharp_index_error.remove(alias);
+    }
+
     /// Remove a repo from the DashMap, dropping its stores and releasing
     /// LMDB file handles. Used before force-reindex reopen.
     fn close_repo(&self, alias: &str) {
+        self.clear_csharp_index_state(alias);
         if self.repos.remove(alias).is_some() {
             tracing::info!(
                 "Closed repo '{}' (dropped stores, released LMDB handles)",
@@ -2945,6 +2964,10 @@ impl ServeState {
             // deleted (the repo can be re-opened on the next query), so we
             // don't need to await — the cancelled task drains on its own.
             self.fsw_tasks.remove(alias);
+            // Cached C# symbol-index state must not outlive the repo (see
+            // clear_csharp_index_state): without this, an Error entry frozen
+            // from a lost double-open race renders red forever.
+            self.clear_csharp_index_state(alias);
             match self.repos.remove(alias) {
                 Some((_, RepoState::Write { cancel_token, .. })) => {
                     cancel_token.cancel();

src/serve/tests.rs

@@ -2261,3 +2261,43 @@ async fn index_rm_deletes_db_while_serve_holds_real_lmdb_env() {
         "expected a clean Unknown-alias error, got: {err:#}"
     );
 }
+
+/// A cached C# symbol-index Error must NOT outlive the repo it belongs to.
+///
+/// `repo_statuses_lightweight()` prefers the cached entry over its on-disk
+/// probe, so an Error left behind by idle eviction renders a red `C#!` in the
+/// TUI forever — a closed repo has no watcher left to retry a rebuild and
+/// flip the state to Ready. Regression guard for the frozen-`!` fix observed
+/// on a repo whose rebuild lost a one-shot LMDB double-open race days
+/// earlier.
+#[serial_test::serial]
+#[test]
+fn evicting_idle_repo_clears_frozen_csharp_error_state() {
+    let _env = crate::testing::EnvRestore::set(&[(crate::constants::REPO_IDLE_TIMEOUT_ENV, "1")]);
+    let state = ServeState::new(ReposConfig::default(), None);
+
+    // Simulate the poisoned state: an Error + message cached by a failed
+    // watcher rebuild, and a last-access old enough to be evicted.
+    state
+        .csharp_index_status
+        .insert("frozen".to_string(), CSharpIndexStatus::Error);
+    state.csharp_index_error.insert(
+        "frozen".to_string(),
+        "LMDB double-open prevented".to_string(),
+    );
+    state.last_access.insert(
+        "frozen".to_string(),
+        std::time::Instant::now() - std::time::Duration::from_secs(5),
+    );
+
+    state.evict_idle_repos();
+
+    assert!(
+        !state.csharp_index_status.contains_key("frozen"),
+        "eviction must clear the cached C# status — a frozen Error renders red forever otherwise"
+    );
+    assert!(
+        !state.csharp_index_error.contains_key("frozen"),
+        "eviction must clear the cached C# error message along with the status"
+    );
+}