fix: TrackedEnv drop must close the heed env via prepare_for_closing

AGENTS.md

@@ -103,5 +103,6 @@ Enforced by the `pre-commit` hook (root-md allowlist guard — see `.githooks/RE
   - **Take the channel as a parameter, not as a field the handler fills in.** A `warnings` field on a response struct is the obvious fix and the weaker one: the handler stays free to pass `None`, and a test that builds the struct itself cannot see it happen. Round 8 proved this — the round-7 defect was reintroduced at the `get_chunk` success path and all 630 tests still passed. Use `respond_with_items()` / `respond_with_object()`, which cannot be called without the channel.
   - **Before claiming a test pins a fix, reintroduce the defect and confirm it fails.** A green suite over a restored defect is the only proof that matters, and a test named after an acceptance criterion that constructs the response itself is testing serde, not the handler. Note `serde_json::Map` is a `BTreeMap` here (no `preserve_order`), so a `to_value` round-trip silently re-sorts keys — a healthy path must serialize the struct directly.
   - **A caller-facing literal wrapped across lines needs a `\` continuation**, or the next line's indentation becomes part of the message. Enforced by `tests/caller_facing_literals.rs`, not by review: three commits shipped this defect through reviews that were explicitly hunting it, because the mangled text still satisfies every `contains(...)` assertion. A detector that only runs by hand gets skipped on exactly the commit that needs it.
+- **LMDB rule — a dropped `TrackedEnv` must close via `prepare_for_closing()`, never a plain drop.** heed 0.20's `OPENED_ENV` cache keeps a strong `Env` clone inside its entry, so dropping the last user-side `Env` leaves the Arc count at 1 (the entry's) and `mdb_env_close` NEVER runs — on Windows `data.mdb`/`lock.mdb` stay locked for the process lifetime (the deterministic `index rm` os-error-32 failure, todo issue #76), on POSIX it's an invisible fd/mmap leak. `TrackedEnv::drop` in `src/lmdb_registry.rs` handles this; never clone a raw `heed::Env` out of a `TrackedEnv` (that breaks the "last reference" assumption the close depends on).
 - **Counter-then-teardown races.** A background task that tears down state guarded by an in-flight counter (idle-checker closing a connection, a reaper dropping a handle, a GC sweep clearing a slot) must take the state's write lock *before* checking the counter, and hold that lock across both the check and the clear. Checking the counter first and taking the write lock afterwards — even with no other statement between them — leaves a window in which a consumer can still acquire the resource and have it torn down mid-use once the write lock lands. The fix composes because of how consumers are structured: every consumer increments the counter (e.g. via an RAII guard created at function entry) *before* it takes the read lock to acquire the resource. That means a consumer already holding the resource has necessarily already incremented — so the checker seeing `counter == 0` under its own write lock proves no such consumer exists — and a consumer that has not yet read will simply block on the held write lock until the teardown (or the "already gone" check) has completed. Found in the MCP proxy's idle-disconnect feature (`src/mcp/mod.rs`, the `idle_ticker.tick()` arm in `run_mcp_client`): the original version read `in_flight`/`peer_state` before acquiring the write lock; fixed by moving the acquisition first, per the "counter-then-teardown races" review lesson.
 - **Tooling:** never use the bundled `codesearch` binary to investigate this repo (it's the project under development). Use codesearch **MCP tools first** for discovery (server verified working; this repo indexed as `codesearch-git`). `grep`/`Glob`/`Read` stay correct for a specific git ref / fetched PR head (codesearch only indexes the on-disk working tree), exact literal matching, or when MCP returns nothing.

CHANGELOG.md

@@ -14,6 +14,12 @@ more PRs land; when the release is actually tagged, the same section is
 finalized in place with a date — no renaming/migration step needed.
 -->
 
+## [1.3.4]
+
+### Fixed
+
+- **`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
 
 ### Added

src/index/manager.rs

@@ -2489,6 +2489,35 @@ mod tests {
     use crate::cache::FileMetaStore;
     use tempfile::tempdir;
 
+    /// Dropping the last `Arc<SharedStores>` must release every handle inside
+    /// the DB directory — the precondition `index rm`'s delete path depends on.
+    ///
+    /// Pins the Windows manifestation of a heed 0.20 leak fixed in
+    /// `TrackedEnv::drop`: the `OPENED_ENV` cache entry held a strong `Env`
+    /// clone, so `mdb_env_close` never ran after a plain drop and
+    /// `data.mdb`/`lock.mdb` stayed locked for the life of the process —
+    /// `serve::tests::index_rm_deletes_db_while_serve_holds_real_lmdb_env`
+    /// failed deterministically on os error 32 through the whole 60 s retry
+    /// budget with an EMPTY LMDB registry (the holder was invisible to it).
+    /// This is the serve-free, instant version of that acceptance test.
+    #[test]
+    fn sharedstores_drop_releases_db_dir_for_deletion() {
+        let tmp = tempdir().unwrap();
+        let db = tmp.path().join(".codesearch.db");
+        let stores = SharedStores::new(&db, 384).expect("open SharedStores");
+        assert!(
+            !crate::lmdb_registry::open_holders_under(&db).is_empty(),
+            "holder must be visible while SharedStores lives"
+        );
+        drop(stores);
+        assert!(
+            crate::lmdb_registry::open_holders_under(&db).is_empty(),
+            "registry must drain after the last Arc<SharedStores> drops"
+        );
+        std::fs::remove_dir_all(&db)
+            .expect("db dir must be deletable after SharedStores drops (no leaked LMDB handles)");
+    }
+
     /// Helper: create metadata.json in db_path with given dimensions
     fn create_metadata_json(db_path: &Path, dimensions: usize) {
         let metadata = serde_json::json!({

src/lmdb_registry.rs

@@ -186,10 +186,12 @@ impl TrackedEnv {
 
 impl Drop for TrackedEnv {
     fn drop(&mut self) {
-        // Ordering here is load-bearing. heed maintains its OWN process-global
-        // registry of opened environments (`OPENED_ENV`), keyed by canonical
-        // path, that outlives a `heed::Env` until its last strong ref drops.
-        // If we `unregister()` from our registry FIRST and let the field drop
+        // Ordering here is load-bearing, twice over.
+        //
+        // (1) The env must be closed BEFORE we free our own registry slot.
+        // heed maintains its OWN process-global registry of opened
+        // environments (`OPENED_ENV`), keyed by canonical path. If we
+        // `unregister()` from our registry FIRST and let the field drop
         // afterwards (the default Rust drop order: body, then fields), there is
         // a window where our slot is free but heed's env is still alive. A
         // concurrent `TrackedEnv::open` on the same path — e.g. the idle reaper
@@ -198,18 +200,37 @@ impl Drop for TrackedEnv {
         // rejects with the cryptic "an environment is already opened with
         // different options" (once a prior MDB_MAP_FULL resize left the live
         // env's recorded map_size differing from the reopen's resolved size).
+        // Closing the env before `unregister()` enforces the invariant
+        // "our slot free ⟹ heed's slot free": a concurrent open either sees
+        // our slot still occupied (clear "double-open prevented" + retry) or
+        // sees both free (clean reopen). It can never observe the inconsistent
+        // state that produces heed's raw error.
         //
-        // Dropping the `heed::Env` BEFORE `unregister()` enforces the invariant
-        // "our slot free ⟹ heed's slot free": a concurrent open either sees our
-        // slot still occupied (clear "double-open prevented" + retry) or sees
-        // both free (clean reopen). It can never observe the inconsistent state
-        // that produces heed's raw error.
+        // (2) A plain drop of the `heed::Env` does NOT close the environment.
+        // heed 0.20's `OPENED_ENV` entry itself holds a strong `Env` clone
+        // (`EnvEntry { env: Some(env.clone()), .. }` — inserted at open, used
+        // to hand out further clones on re-open). With our wrapper as the only
+        // user-side reference, dropping it leaves the Arc count at exactly 1:
+        // the entry's own clone. `EnvInner::drop` — and with it
+        // `mdb_env_close` — therefore NEVER runs. On POSIX that leaks an fd
+        // and an mmap silently; on Windows it locks `data.mdb`/`lock.mdb`
+        // against deletion for the lifetime of the process, which is why
+        // `index rm` against a running serve could not delete the DB dir
+        // (deterministic os error 32 after the whole retry budget, LMDB
+        // registry long empty — the holder is invisible to it).
+        // `prepare_for_closing()` is heed's one real close path: it takes the
+        // entry's reference out and drops the last one synchronously
+        // (entry removed, `mdb_env_close` called, waiters signalled) before
+        // returning. It is correct here because nothing in this codebase
+        // clones the `heed::Env` out of a `TrackedEnv` (the deref only lends
+        // `&Env`; `TrackedEnv` itself is not `Clone`), so this wrapper holds
+        // the last user-side reference.
         //
-        // SAFETY: `inner` is dropped exactly once, here, and never accessed
-        // again (the surrounding `TrackedEnv` is being destroyed).
-        unsafe {
-            ManuallyDrop::drop(&mut self.inner);
-        }
+        // SAFETY: `inner` is taken exactly once, here, and the `ManuallyDrop`
+        // slot is never touched again afterwards (the surrounding
+        // `TrackedEnv` is being destroyed).
+        let env = unsafe { ManuallyDrop::take(&mut self.inner) };
+        env.prepare_for_closing();
         unregister(&self.canonical);
     }
 }
@@ -288,7 +309,6 @@ mod tests {
         assert!(err.contains("double-open prevented"));
         assert!(err.contains("test-1"));
     }
-
     #[test]
     fn test_registry_allows_reopen_after_drop() {
         let dir = TempDir::new().unwrap();
@@ -304,6 +324,49 @@ mod tests {
         let _env2 = unsafe { TrackedEnv::open(&opts, path, "test-2").unwrap() };
     }
 
+    /// Dropping the last `TrackedEnv` must REALLY close the heed environment.
+    ///
+    /// heed 0.20's `OPENED_ENV` entry holds a strong `Env` clone of its own,
+    /// so a plain drop of the user-side `Env` leaves the Arc count at 1 (the
+    /// entry's) and `mdb_env_close` never runs — the env stays open invisibly:
+    /// `env_closing_event` keeps answering `Some`, and on Windows `data.mdb`/
+    /// `lock.mdb` stay locked against deletion for the life of the process
+    /// (the deterministic `index rm` os-error-32 failure this test pins).
+    /// `TrackedEnv::drop` therefore closes via `prepare_for_closing()`.
+    ///
+    /// Cross-platform: the `env_closing_event` assert fails everywhere when
+    /// the close path regresses; the directory-delete assert is the
+    /// Windows-visible consequence of the same leak and guards it directly.
+    #[test]
+    fn drop_really_closes_heed_env_and_releases_the_files() {
+        let dir = TempDir::new().unwrap();
+        let db_path = dir.path().join("db");
+        std::fs::create_dir(&db_path).unwrap();
+        let opts = make_opts();
+
+        {
+            let _env = unsafe { TrackedEnv::open(&opts, &db_path, "close-on-drop-test").unwrap() };
+            assert!(
+                heed::env_closing_event(&db_path).is_some(),
+                "while the TrackedEnv lives, heed must report the env open"
+            );
+        }
+
+        // The env must now be gone from heed's own registry too — not just
+        // from ours (`open_holders_under` is vacuous here, it tracks
+        // TrackedEnvs only).
+        assert!(
+            heed::env_closing_event(&db_path).is_none(),
+            "heed's OPENED_ENV entry must be removed on TrackedEnv drop; \
+             a surviving entry means mdb_env_close never ran"
+        );
+
+        // ...which is what makes the DB directory deletable on Windows.
+        std::fs::remove_dir_all(&db_path)
+            .expect("db dir must be deletable after the last TrackedEnv drops");
+        assert!(!db_path.exists());
+    }
+
     #[test]
     fn test_different_paths_both_allowed() {
         let dir1 = TempDir::new().unwrap();