# Compaction Stale DiskLoc

**Date:** 2026-05-16
**Status:** Approved design
**Scope:** `VarTree`, `VarMap` read paths without the shard lock (`get`, `first`, iterators, `migrate` traversal); `Shard::read_block` including its encrypted-decode second-lock pass; `ShardInner::read_value_from_disk_locked`; `read_value_cached_inner` block helpers (`get_or_read_block`, `extract_from_block`)
**Origin:** issue #9 in `armdb/docs/reviews/codex-vartree-varmap.md`; "Follow-Up Work" in `armdb/docs/superpowers/specs/26-05-15-var-collections-critical-correctness.md`
**Review:** `armdb/docs/superpowers/reviews/26-05-16-compaction-stale-diskloc.md` (GPT-5 Codex) — spec revised to address tests/visibility/encrypted-path/multi-block/guard-lifetime concerns.

## Context

`Bitcask` compaction in `armdb/src/compaction.rs` removes obsolete immutable data files in three observable phases under the shard mutex:

1. New compacted immutable file is added to `inner.immutable` (critical section 1).
2. Index entries are CAS-updated from `old_loc` to `new_loc` in batches of 256 (`update_if_match`).
3. Old file IDs are removed from `inner.immutable` (critical section 2), then on-disk files are deleted outside the lock.

Phases 2 and 3 each take and release the shard lock. Lock-free readers (`VarTree::get`, `VarTree::iter`/`first`/`last`, `VarMap::get`) follow the pattern:

```rust
let disk = *node.load_disk();        // T1: copy stale DiskLoc (F_old)
self.read_value_cached(&disk)         // T4: try to read F_old, locks shard
```

Or for `VarMap`:

```rust
let disk = {
    let index = sync::lock(&self.indexes[shard]);
    *index.get(key)?
};                                    // T1: copy stale DiskLoc (F_old)
self.read_value_cached(&disk)         // T4: try to read F_old
```

Compaction can interleave between T1 and T4:

- T2: compaction holds the shard lock, runs `update_if_match` for this key → the in-memory index now points to `F_new`.
- T3: compaction holds the shard lock, retains `inner.immutable` without `F_old`.
- T4: reader acquires the shard lock inside `Shard::read_block` (or `ShardInner::read_value_from_disk_locked`), looks up `F_old` in `inner.immutable`, fails.

`Shard::read_block` currently maps this to `DbError::Io(NotFound)`. `read_value_cached` swallows it and returns `None`. The public `get()` then returns `None` for a key whose value is still on disk and still indexed.

Paths that hold the shard lock end-to-end (`VarTree::cas`, `update`, `fetch_update`, `delete` with `H::NEEDS_OLD_VALUE`, `VarShard::get`, mirror for `VarMap`) are not affected: compaction's phases 2 and 3 each take the same lock, so the reader either sees the pre-compaction or fully-post-compaction state — never a mid-flight one. This is confirmed by reading the index *after* acquiring the lock, which is what `cas`/`update` already do.

## Goals

1. The lock-free `get` for a live key must always return `Some(value)` when compaction races a read.
2. Iteration over `VarTree` (`iter`, `prefix_iter`, `range`, `first`, `last`) and the `VarTree::migrate` / `VarMap::migrate` traversal must not silently drop a live key because compaction removed its source file from the immutable view.
3. Introduce no new on-disk artefacts and no change to the observable phases of the existing compaction algorithm.
4. Add targeted regression tests that demonstrate the race deterministically.

## Non-Goals

- Lifecycle of immutable files (no deferred retirement, no `retiring` vec, no seize-managed `Arc<ImmutableFile>`).
- Compaction algorithm changes. No `compact_shard_inner` split, no new phase boundaries, no test hooks added to the compaction module.
- Block-cache invalidation timing (`invalidate_blocks` after critical section 2 stays as is).
- Retry under the shard lock (out of scope per the rationale above).
- `ConstTree` / `TypedTree` / `ZeroTree` reads — values are inlined into SkipList nodes, no on-disk lookup at read time.
- `FixedStore` — no compaction exists for that backend.
- Strengthening weakly-consistent iteration semantics beyond the "live key → Some" contract (keys inserted after the iterator starts may still be missed).
- Recovery semantics if compaction itself aborts mid-flight.
- Concurrent compactions on the same shard (engine already serializes).
- `DiskLoc.file_id` width (`u16` overflow) — separate spec.
- `migrate` hook semantics (when `on_write`/`on_init` should fire) — separate spec. This spec only fixes the traversal's read path, not the hook contract.
- Multi-block large-value cache (>2 blocks) — separate spec. This spec keeps the existing two-block boundary; it only ensures that `StaleDiskLoc` propagates correctly through both block reads.
- `VarTree::warmup` / `VarMap::warmup` retry behavior. Warmup is best-effort and currently propagates `read_block` errors via `?`. After this spec it will propagate `DbError::StaleDiskLoc` instead of `DbError::Io(NotFound)`. Callers that already ignore warmup errors are unaffected; callers that handle them can choose to retry or ignore on `StaleDiskLoc`. No retry loop is added to warmup itself.

## Design

### Stale signal

Add a discrete error variant in `armdb/src/error.rs`:

```rust
/// A copied DiskLoc points to a file that has just been removed from
/// inner.immutable by compaction. The data is logically still live;
/// the caller should retry with a fresh DiskLoc from the index.
#[error("stale DiskLoc - file removed by compaction")]
StaleDiskLoc,
```

(ASCII hyphen in the message text — keep stable charset across log/grep paths.)

`status_code()` and `to_resp()` keep their current shape; `StaleDiskLoc` falls into the default `500` branch. In practice the only call sites that surface it are `Shard::read_block` (used by `read_value_cached_inner` and `warmup`) and `ShardInner::read_value_from_disk_locked` (used only under shard lock, where Stale is logically unreachable — see Error Handling).

Sources of the variant:
- `Shard::read_block`: `inner.immutable.iter().find(...)` returns `None` for the requested `file_id` AND `inner.active.file_id != file_id`.
- `Shard::read_block` encrypted-decode path: the captured immutable Arc's `tag_file` is `None` while a cipher is configured (see below).
- `ShardInner::read_value_from_disk_locked`: same condition as `read_block` for the immutable lookup.

### `Shard::read_block`

Two changes are required. Both happen inside the same function (`armdb/src/shard.rs:401`).

**Change A — Stale signal on the primary immutable lookup.**

Replace the `NotFound` `io::Error` constructor with `DbError::StaleDiskLoc`:

```rust
let arc = inner
    .immutable
    .iter()
    .find(|f| f.file_id == file_id)
    .ok_or(DbError::StaleDiskLoc)?
    .clone();
```

The active-file branch keeps its current behavior: when `inner.active.file_id == file_id`, no lookup is needed.

**Change B — Eliminate the second lock for the encrypted-decode tag-file lookup.**

The current encrypted path (lines 448-466 today) takes the shard lock a second time and re-runs `inner.immutable.iter().find(|f| f.file_id == file_id).and_then(|f| f.tag_file.as_ref())`. Between the first lock (which captured `immutable_arc`) and this second lock, compaction can complete the immutable-retain phase, after which the second lookup returns `None`. The current code then silently skips decryption and returns ciphertext as if it were plaintext, and if `is_full_block` was true, the ciphertext is cached. This is a pre-existing data-correctness bug, not just a "transient `None`" issue, and is in scope here.

Refactor: capture the tag-file decision atomically with the immutable Arc inside lock 1. Concretely:

- The active-file branch returns the `Arc<File>` and (under `#[cfg(feature = "encryption")]`) the active `tag_file` reference, captured while the lock is held. Because `TagFile` is not `Clone`, wrap each `TagFile` in `Arc` at the point it is stored (`ActiveFile::tag_file: Option<Arc<TagFile>>` and `ImmutableFile::tag_file: Option<Arc<TagFile>>`) so a cheap `.clone()` of the `Arc<TagFile>` inside lock 1 is sufficient. This `Arc` rewrap is a small ripple change at the three to four construction sites (open, rotate, compaction add, recovery); no on-disk format change.
- The immutable-file branch captures `(arc, total, tag_file_arc_opt)` in one shot; subsequent decode uses `tag_file_arc_opt` without re-locking.
- Remove the second `sync::lock(&self.inner)` block entirely. Decryption uses the captured `Arc<TagFile>` directly.
- If the encrypted database is configured (`cipher_ref.is_some()`) but the captured `tag_file_arc_opt` is `None` for an immutable file that the first lookup found, return `DbError::StaleDiskLoc`. This case is unreachable for correctly-recovered databases but defends against the silent-skip-decryption pre-existing bug.
- Under `#[cfg(not(feature = "encryption"))]` the change is purely about the first lookup (Change A); no second lock exists.

Caching invariant after Change B: the block is only `insert`ed into `BlockCache` after a successful decryption (or no-cipher path), so it can never contain ciphertext.

If `file_id` is neither active nor in immutable, the new variant is returned. This includes the legitimate compaction race and any future programming error; the latter would be visible because callers convert `StaleDiskLoc` into either a bounded retry or a propagated error, never into silent `None`.

### `ShardInner::read_value_from_disk_locked`

Mirror the change in `armdb/src/shard.rs::ShardInner::read_value_from_disk_locked` (added in spec `26-05-15-var-collections-critical-correctness`). Replace the `DbError::Io(NotFound)` it currently returns for an unknown immutable file with `DbError::StaleDiskLoc`.

`VarTree::read_value_locked` and `VarMap::read_value_locked` keep their `Option<ByteView>` return type. The disk-read branch now distinguishes Stale from genuine I/O failure:

```rust
let bytes = match inner.read_value_from_disk_locked(disk) {
    Ok(b) => b,
    Err(DbError::StaleDiskLoc) => {
        tracing::error!(
            file_id = disk.file_id,
            shard_id = disk.shard_id,
            "stale DiskLoc under shard lock — programming bug"
        );
        return None;
    }
    Err(_) => return None,
};
Some(ByteView::new(&bytes))
```

Under the shard lock, `StaleDiskLoc` is impossible by design (compaction takes the same lock for its retain phase). The branch exists so the assumption is not silent: if it ever fires, the log records it.

### `VarTree::read_value_cached`

Current signature and behavior:

```rust
fn read_value_cached(&self, disk: &DiskLoc) -> Option<ByteView>
```

Split into two parts:

```rust
const MAX_STALE_RETRIES: usize = 3;

/// Lock-free read that propagates errors. Called only from `read_value_cached`
/// (which adds the retry loop). `read_value_locked` does NOT call this; it has
/// its own body that already handles the write-buffer / cache / disk dispatch.
fn read_value_cached_inner(&self, disk: &DiskLoc) -> DbResult<Option<ByteView>> {
    // 1. Block cache (lock-free).
    // 2. Shard::read_block — now returns DbError::StaleDiskLoc on missing file_id
    //    (Change A above) or on captured-tag-file-missing (Change B).
    // 3. extract_from_block / get_or_read_block — for multi-block values these
    //    helpers MUST propagate DbResult so the SECOND block read's
    //    DbError::StaleDiskLoc is not swallowed (see below).
    // 4. ByteView construction.
    // Returns:
    //   Ok(Some(v))         — value retrieved
    //   Ok(None)             — value genuinely not present (e.g. cache returned partial
    //                          block at end-of-file)
    //   Err(StaleDiskLoc)    — compaction race; retry with fresh DiskLoc
    //   Err(other)           — I/O / decryption / corruption
}

fn read_value_cached(&self, node: &VarNode<K>, guard: &seize::LocalGuard<'_>) -> Option<ByteView> {
    let _ = guard; // explicit anchor: caller must keep guard alive across the call
    for _ in 0..MAX_STALE_RETRIES {
        let disk = *node.load_disk();
        match self.read_value_cached_inner(&disk) {
            Ok(Some(v)) => return Some(v),
            Ok(None) => return None,
            Err(DbError::StaleDiskLoc) => {
                metrics::counter!("armdb.read.stale_retry", "tree" => "var_tree")
                    .increment(1);
                continue;
            }
            Err(_) => return None,
        }
    }
    None
}
```

**Guard lifetime.** The retry loop calls `node.load_disk()` and reads through the raw `*mut VarNode<K>`. Both require an active `seize::LocalGuard` to keep the node from being retired between iterations. The signature takes the guard by reference so the call site cannot drop it before the loop completes. Concrete call-site fixes:

- `VarTree::get` (`armdb/src/var_tree.rs:206-211`): the existing guard already lives until end of method; pass it as `&guard`. No structural change.
- `VarTree::first` (`armdb/src/var_tree.rs:429-448`): the current code contains `let _ = guard;` immediately before the call, which moves `guard` into a `_` binding and drops it at end of statement. This drop happens before the read returns and before `node.key` is read again, leaving the raw-pointer access unsynchronized with seize epochs. Remove the `let _ = guard;` line. Pass `&guard` to `read_value_cached`. After the call, the guard naturally drops at end of method.
- `VarTree::migrate` (`armdb/src/var_tree.rs:700-737`): the guard is held across the whole traversal (`let _guard = ...`); pass `&_guard` (or rename to `guard`).
- `VarIter::next` / `next_back` (`armdb/src/var_tree.rs:1206`, `1250`): `VarIter` stores `_guard: seize::LocalGuard<'a>` in the iterator. Pass `&self._guard`. The `_` prefix is misleading — keep the rename to `guard` if convenient, or use `&self._guard` directly.

All current callers of `read_value_cached` in `var_tree.rs` already have a `&VarNode<K>` (or `*mut VarNode<K>` they immediately deref under `unsafe`) in scope: `get`, `first`, `migrate`, `VarIter::next`, `VarIter::next_back`. They pass `node` instead of `&disk` and `&guard` instead of dropping it. `VarTree::migrate` already iterates level-0 with a `node` reference.

### Block helpers must propagate `DbResult`

`VarTree::get_or_read_block` (`armdb/src/var_tree.rs:798`) and the symmetric `VarMap::get_or_read_block` (`armdb/src/var_map.rs:566`) currently swallow `Shard::read_block` errors and return `Option<Arc<AlignedBuf>>` (logging and yielding `None`). `VarTree::extract_from_block` / `VarMap::extract_from_block` then call `next_block: impl FnOnce() -> Option<Arc<AlignedBuf>>` to read the second block when the value spans a 4096-byte boundary.

After this spec, `Shard::read_block` can return `Err(DbError::StaleDiskLoc)`. With the current `Option` shape, either the first or the second block read would convert `Stale` into `None`, defeating the retry signal. Refactor:

```rust
fn get_or_read_block(
    &self,
    shard_id: u8,
    file_id: u32,
    block_offset: u64,
) -> DbResult<Arc<AlignedBuf>> {
    // cache hit → Ok(cached)
    // miss → match shard.read_block(...) {
    //   Ok((buf, is_full_block)) => { insert if full, Ok(Arc::new(buf)) }
    //   Err(e) => Err(e),   // includes DbError::StaleDiskLoc
    // }
}

fn extract_from_block(
    block: &AlignedBuf,
    start: usize,
    len: usize,
    next_block: impl FnOnce() -> DbResult<Arc<AlignedBuf>>,
) -> DbResult<ByteView> {
    if start + len <= 4096 {
        Ok(ByteView::new(&block[start..start + len]))
    } else {
        let next = next_block()?;
        // ... existing combine logic ...
        Ok(ByteView::from_vec(combined))
    }
}
```

`read_value_cached_inner` calls them and propagates the `DbResult`:

```rust
let block = self.get_or_read_block(disk.shard_id, disk.file_id as u32, block_offset)?;
let view = Self::extract_from_block(&block, start, len, || {
    self.get_or_read_block(disk.shard_id, disk.file_id as u32, block_offset + 4096)
})?;
Ok(Some(view))
```

Now both block reads surface `DbError::StaleDiskLoc` to the retry loop. The non-stale `Err` cases (I/O, decryption) are propagated by `read_value_cached_inner` and turned into `None` by the outer `read_value_cached` wrapper, preserving the historic public behavior for those.

The cache-hit fast path stays `Ok(cached)` — no I/O occurs, so no race against compaction is possible inside the hit branch itself (the worry is the next-block path).

### `VarMap::read_value_cached`

`VarMap` has no SkipList node; the per-shard `Mutex<HashMap<K, DiskLoc>>` is the index. The retry re-acquires the shard's index lock and re-reads the `DiskLoc`. The public `get()` becomes:

```rust
pub fn get(&self, key: &K) -> Option<ByteView> {
    let shard_id = self.shard_for(key);
    for _ in 0..MAX_STALE_RETRIES {
        let disk = {
            let index = sync::lock(&self.indexes[shard_id]);
            match index.get(key) {
                Some(d) => *d,
                None => return None,
            }
        };
        match self.read_value_cached_inner(&disk) {
            Ok(Some(v)) => return Some(v),
            Ok(None) => return None,
            Err(DbError::StaleDiskLoc) => {
                metrics::counter!("armdb.read.stale_retry", "tree" => "var_map")
                    .increment(1);
                continue;
            }
            Err(_) => return None,
        }
    }
    None
}
```

`read_value_cached_inner` is the existing read-block-plus-decode body, refactored to return `DbResult<Option<ByteView>>` instead of `Option<ByteView>`. The shape mirrors `VarTree::read_value_cached_inner`.

The retry path in `VarMap` performs an extra short-held mutex acquisition; this is acceptable because the race is rare (counted by the metric).

### `MAX_STALE_RETRIES`

`3` is a const at the top of each file (or in a shared module if convenient). Justification:

- A single compaction touching the same key produces at most one transition from `F_old` to `F_new`. After one retry the node's `disk` (or the HashMap's `DiskLoc`) points to `F_new`, which is in `inner.immutable`.
- Successive compactions of `F_new` into `F_newer` require time on the order of the compaction throughput; three consecutive races on the same `get` call is implausible without unrelated bugs.
- A persistent `StaleDiskLoc` therefore indicates a programming bug; returning `None` after three failed attempts preserves the historic behavior for that case and avoids unbounded looping.

The metric `armdb.read.stale_retry` (with a `"tree"` label set to `"var_tree"` or `"var_map"`) makes the rate of legitimate races observable. Histogram or counter is fine; counter is simpler.

### Test placement

Tests live as unit tests inside the affected crate modules, NOT in `armdb/tests/` integration tests. Reasoning:

- Integration tests can only access `pub` API; the variant tests need to call `read_value_cached_inner`, build fake `DiskLoc` values, and access `pub(crate)` cache fields.
- No `#[cfg(test)] pub(crate)` test-hooks are added to `compaction.rs`. The compaction algorithm runs unchanged.
- The deterministic property of the regression tests does not depend on compaction-phase-by-phase control. `compact_shard` is called synchronously from the test thread; the stale snapshot is captured *before* the call, the assertions run *after* it. The race window we care about (post-section-2, pre-step-5) collapses to "after `compact_shard` returns": the immutable Vec no longer contains `F_old`, the index points to `F_new`, and the cache has been invalidated. A snapshot of `F_old`'s `DiskLoc` captured before the call is exactly the stale value a real reader would have.

Concretely the tests live at:

- `armdb/src/shard.rs#[cfg(test)] mod tests` — Stale signal from `Shard::read_block` and `ShardInner::read_value_from_disk_locked`.
- `armdb/src/var_tree.rs#[cfg(test)] mod tests` — `read_value_cached_inner` stale, `read_value_cached` retry, `get_or_read_block` stale, `extract_from_block` propagation.
- `armdb/src/var_map.rs#[cfg(test)] mod tests` — `read_value_cached_inner` stale, `get` retry, `get_or_read_block` stale.

Existing integration tests in `armdb/tests/var_correctness_tests.rs` are NOT extended for this spec; they remain the test surface for the previous slice's behaviors (PR #8).

## Error Handling

- `DbError::StaleDiskLoc` is a real `DbError` variant, not a private signal. It can appear on the following public surfaces and the spec defines the behavior for each:
  - `Shard::read_block` (public): returns the variant directly. Internal callers consume it; external callers (none today inside the workspace) should treat it like any other transient `DbError`.
  - `Shard::read_value` (public, currently unused by VarTree/VarMap): the variant can surface here through the immutable lookup; behavior unchanged otherwise.
  - `VarTree::warmup` / `VarMap::warmup` (public): propagate the variant via `?`. Pre-spec, the same code path returned `DbError::Io(NotFound)`. See Non-Goals.
- For lock-free `get` / `first` / iteration / `migrate` traversal, the retry loop in `read_value_cached` consumes the variant. Callers see the historic `Option<ByteView>` shape; persistent stale (3 retries) maps to `None`.
- I/O errors, decryption errors, and corruption keep mapping to `None` for cached reads, mirroring current behavior.
- Under the shard lock, `Stale` is logically impossible because compaction takes the same lock for the immutable retain phase. The signature of `read_value_locked` stays `Option<ByteView>` and the implementation handles the disk branch as:

  ```rust
  let bytes = match inner.read_value_from_disk_locked(disk) {
      Ok(b) => b,
      Err(DbError::StaleDiskLoc) => {
          tracing::error!(
              file_id = disk.file_id,
              shard_id = disk.shard_id,
              "stale DiskLoc under shard lock - programming bug",
          );
          return None;
      }
      Err(_) => return None,
  };
  ```

  Current behavior (None on disk error) is preserved; the log gives the bug a visible signal if the assumption ever breaks.
- `DbError` is not `#[non_exhaustive]` today. Adding a variant is technically a breaking change for any external matcher that omits a `_` arm. Reviewing the workspace, both `armdb` internal call sites and `armdb`/`armour-rpc` external consumers use catch-all arms, so no fanout. Adding `#[non_exhaustive]` to `DbError` itself is left out of scope but called out as a future cleanup.

## Tests

All tests are `#[cfg(test)] mod tests` unit tests inside the relevant source files; for any test that requires `feature = "var-collections"` the test function (or the whole module) is additionally gated on `#[cfg(feature = "var-collections")]`. Tests requiring encryption are gated on `#[cfg(feature = "encryption")]`.

In `armdb/src/shard.rs`:

1. `read_block_returns_stale_for_unknown_file_id`. Open a `Shard` directly (test-only helper or the existing `Shard::open` API), construct `DiskLoc::new(9999, 0, 0)`. Call `shard.read_block(9999, 0)`. Expect `Err(DbError::StaleDiskLoc)`.
2. `read_value_from_disk_locked_returns_stale_for_unknown_file_id`. Same setup; acquire `shard.lock()`; call `inner.read_value_from_disk_locked(&fake_disk)`; assert the same variant.
3. `read_block_encrypted_uses_captured_tag_file`. With `feature = "encryption"`, open an encrypted shard, write a value, force flush + rotation so the value is in immutable. Snapshot the immutable `Arc<ImmutableFile>` (or the `Arc<TagFile>` from it) via the test-only accessor on `ShardInner`. Run compaction to remove the immutable. Call `shard.read_block(F_old, off)` using the snapshot's `file_id`. Expect `Err(DbError::StaleDiskLoc)` (the primary lookup short-circuits — there is no longer a second lock). Optionally, also assert the cache was never populated with this `(F_old, off)` block.

In `armdb/src/var_tree.rs`:

4. `var_tree_read_value_cached_inner_returns_stale_after_compaction`. Open `VarTree<[u8;8]>` with `max_file_size=4096`, `compaction_threshold=0.0`. Insert and overwrite N keys so multiple immutable files accumulate dead bytes. Snapshot a `DiskLoc` via the index: `let snap = *index.get(key.as_bytes(), &guard).unwrap().load_disk();`. Call `armdb::compaction::compact_shard::<_, _>(shard, index, threshold)` synchronously. Assert `matches!(tree.read_value_cached_inner(&snap), Err(DbError::StaleDiskLoc))`.
5. `var_tree_read_value_cached_returns_some_after_compaction`. Same setup as (4). Capture `node = index.get(...)`. Run compaction. Call `tree.read_value_cached(node, &guard)`. Expect `Some(value)` matching the post-compaction value. This test verifies the outer wrapper still produces `Some` once compaction has finished. Note: this does NOT exercise the *retry-after-stale* path on its own — after `compact_shard` returns, `node.load_disk()` already yields the fresh `DiskLoc`, so the loop's first iteration succeeds. The retry signal itself is verified by test (4); the loop body is short and statically obvious from the implementation. Combined coverage:
   - Stale signal: (4).
   - Outer wrapper returns Some after compaction: (5), (6), (7).
   - Retry-limit termination: optional test (14).
6. `var_tree_get_during_compaction_returns_some`. Same setup; call public `tree.get(&key)` after `compact_shard` returns. Expect `Some`. Also assert `armdb.read.stale_retry` counter incremented once (or use a test-friendly counter wrapper, optional).
7. `var_tree_iter_during_compaction_yields_all_live_keys`. Same setup; instead of `get`, iterate via `tree.iter()` and assert every original key appears with its current value.
8. `var_tree_get_or_read_block_returns_stale_for_unknown_file_id`. Direct unit test of the helper: call `tree.get_or_read_block(shard_id, 9999, 0)`. Expect `Err(DbError::StaleDiskLoc)`.
9. `var_tree_extract_from_block_propagates_next_block_error`. Pure function test on `VarTree::extract_from_block`. Build a 4096-byte `AlignedBuf`, pick `start, len` such that `start + len > 4096`. Pass a `next_block: || Err(DbError::StaleDiskLoc)` closure. Assert the function returns `Err(DbError::StaleDiskLoc)`.
10. `var_tree_extract_from_block_multi_block_first_cached_second_stale`. End-to-end multi-block path with cache:
    - Write a single >4072-byte value (spans two blocks).
    - Snapshot `DiskLoc`.
    - Run compaction (invalidates cache for old blocks).
    - Manually re-insert ONLY the first block into the cache via `tree.cache.insert(...)` using the stale `(F_old, block_offset)` key and an arbitrary-but-known `AlignedBuf` (the value's first-block content).
    - Call `tree.read_value_cached_inner(&snap)`. First block hits the cache; second block triggers `get_or_read_block` → `read_block(F_old, ...)` → `Err(StaleDiskLoc)`. Assert the variant propagates.

In `armdb/src/var_map.rs`:

11. `var_map_read_value_cached_inner_returns_stale_after_compaction`. Mirror of (4) for `VarMap`. Snapshot is obtained as `let snap = *sync::lock(&map.indexes[shard_id]).get(key).unwrap();` then drop the lock before compaction.
12. `var_map_get_during_compaction_returns_some`. Mirror of (6).
13. `var_map_get_or_read_block_returns_stale_for_unknown_file_id`. Mirror of (8).

Retry-limit smoke test (optional, lives in `armdb/src/var_tree.rs`):

14. `var_tree_retry_limit_returns_none_on_persistent_stale`. Construct a setup where every `read_value_cached_inner` invocation returns `Err(StaleDiskLoc)`:
    - Open the tree, insert a key, snapshot the node pointer.
    - Under `shard.lock()`, manually clear `inner.immutable` so `node.disk`'s `file_id` is no longer reachable.
    - Call `tree.read_value_cached(node, &guard)`. Assert `None` is returned after the loop bound.
    - Optionally also assert `armdb.read.stale_retry` counter incremented `MAX_STALE_RETRIES` times.
    This proves the loop terminates and does not hang on permanently stale state. Private-field access via the `mod tests` block makes the manual setup possible without a public test API.

Tests 4-7 and 10-12 use `compact_shard` (or `compact_shard_inner` with a known `min_replicated_gsn = u64::MAX`) as the synchronous trigger — no test-only compaction API is added.

## Verification

1. `cargo fmt`.
2. `cargo clippy -p armdb --features 'var-collections typed-tree encryption' -- -D warnings`.
3. `cargo nextest run -p armdb --features 'var-collections'`. New unit tests in `shard.rs`, `var_tree.rs`, `var_map.rs` run as part of `cargo nextest run -p armdb` (they are `#[cfg(test)] mod tests`, picked up automatically).
4. `cargo nextest run -p armdb --features 'var-collections encryption'` — must keep the same delta as today: pre-existing failures `test_encrypted_no_key_fails` and `test_unencrypted_with_key_fails` remain (unrelated to this spec); all other tests must pass including the new ones.
5. Confirm the new tests fail before the implementation and pass after, run-by-run, to verify they actually exercise the race-relevant paths and aren't accidentally green on the unfixed code.

## Rollout

- One PR, one feature branch (`compaction-stale-diskloc`).
- No on-disk format change. No compaction algorithm change.
- Internal API change: `read_value_cached(&self, disk: &DiskLoc)` → `read_value_cached(&self, node: &VarNode<K>, guard: &seize::LocalGuard)` in `VarTree`; equivalent inner-function refactor in `VarMap` with `read_value_cached_inner` returning `DbResult<Option<ByteView>>`. All call sites are inside the same crate; both methods are private (`fn`).
- `get_or_read_block` / `extract_from_block` signature changes from `Option` to `DbResult` (private methods, internal only).
- `ActiveFile::tag_file` / `ImmutableFile::tag_file` rewrap to `Option<Arc<TagFile>>`. These fields are `pub(crate)`. All construction and consumption sites in `shard.rs`, `recovery.rs`, `compaction.rs` updated accordingly; the rewrap is mechanical (`Some(TagFile::open_*(...)?)` → `Some(Arc::new(TagFile::open_*(...)?))`, accessors deref via `Arc<TagFile>`).
- `Shard::read_block` is `pub` today. Its error variant set widens with `DbError::StaleDiskLoc`. Downstream code is expected to use catch-all matching on `DbError` and is not affected.
- `DbError::StaleDiskLoc` is a new variant on the public `DbError` enum. `DbError` is not `#[non_exhaustive]` today; see Error Handling above for fanout analysis.
- Side effect (pre-existing bug fixed): on encrypted databases, a compaction race no longer allows ciphertext to be returned (or cached) as plaintext. The fix is structural — by removing the second-lock-pass that lost the file-Arc reference.
- Side effect (pre-existing bug fixed): `VarTree::first` no longer drops its `seize::LocalGuard` (via `let _ = guard;`) before reading through the raw node pointer, eliminating an existing UB window.

## Follow-Up Work

Out of this slice, separate specs to consider next:

- `migrate` / hook semantics mismatch for `VarTree` and `VarMap` (review issue #7).
- Multi-block cached reads for large variable values (review issue #4).
- `DiskLoc.file_id` `u16` overflow on long-running databases (review issue #8).
