# VarTree / VarMap Review Validation - Codex

**Date:** 2026-05-15  
**Reviewer model:** Codex / GPT-5  
**Scope:**
- `armdb/docs/design.md`
- `armdb/docs/reviews/hy3-vartree.md`
- `armdb/docs/reviews/glm5.1-vartree.md`
- `armdb/docs/reviews/kimi-k2.6-vartree.md`
- `armdb/docs/reviews/deepseek-vartree.md`
- `armdb/src/var_tree.rs`
- `armdb/src/var_map.rs`
- `armdb/src/skiplist/node.rs`
- `armdb/src/skiplist/mod.rs`

This document validates the existing AI reviews against the current code, classifies confirmed issues, rejects stale or incorrect findings, and adds additional issues found during the validation.

## Executive Summary

The external reviews contain useful findings, but several high-confidence claims are stale or based on a different implementation. The most important confirmed problems in the current code are not purely API edge cases - they include unsafe `Send` implementations around `seize::LocalGuard`, unsafe pointer tagging implementation details, large-value read/write limitations, encrypted `VarTree`/`VarMap` read paths, and `migrate` hook semantics.

Highest-priority current issues:

1. `VarIter` is unsafely marked `Send` while it contains `seize::LocalGuard`, which is explicitly thread-local and `!Send`.
2. SkipList node deletion marks reinterpret `AtomicPtr<T>` as `AtomicUsize`; this is not a robust Rust layout/aliasing contract.
3. `VarTree::insert_locked` ignores `InsertResult::Exists`.
4. `VarTree` and `VarMap` cached reads support at most two 4096-byte blocks; larger values can panic or be truncated by logic assumptions.
5. `append_entry` can overflow the write buffer for entries larger than `write_buffer_size`.
6. Encrypted `VarTree`/`VarMap` `cas`/`update`/old-value paths use raw `pread_value` instead of the encrypted read path.
7. `VarTree::migrate` and `VarMap::migrate` fire `on_write`, while docs say migrate should not fire `on_write`; docs also contradict themselves about whether VarTree/VarMap support `on_init`.
8. `DiskLoc.file_id` is `u16`, while internal file IDs are `u32`; after 65535 files per shard this can alias.
9. A concurrent compaction can make a reader that already copied an old `DiskLoc` miss a still-live value after the old immutable file is removed from `inner.immutable`.

## Model Usefulness Ranking

| Model | Usefulness | Notes |
|---|---:|---|
| Kimi K2.6 | 7/10 | Best unsafe/concurrency review. Correctly called out `VarIter: Send` with `LocalGuard`, `AtomicPtr` as `AtomicUsize`, and prefix length panic. Some claims were overstated or invalid. |
| GLM 5.1 | 6/10 | Useful for `insert_locked`, `migrate`, and spanning-value read paths. It overstates the public same-key insert race because shard locking usually serializes that path. |
| DeepSeek | 6/10 | Good validation around `insert_locked` and correctly rejects a future-only SkipList marked-node race. The spanning cache/write-buffer bug is mostly invalid as described because active file blocks are not cached. |
| Hy3 | 2/10 | Mostly stale or mismatched to current code. It references nonexistent sequence-based CAS and tombstone-shaped `DiskLoc` semantics. Only the broad compaction/read-safety concern remains directionally useful. |

## Confirmed Issues

### 1. `VarIter` is unsafely `Send` with `seize::LocalGuard`

**Severity:** critical  
**Files:** `armdb/src/var_tree.rs`, also same pattern in `ConstIter` and `TypedIter`  
**Source review:** Kimi K2.6

`VarIter` stores a `seize::LocalGuard<'a>`:

```rust
pub struct VarIter<'a, K: Key, H: WriteHook<K> = NoHook> {
    ...
    _guard: seize::LocalGuard<'a>,
}

unsafe impl<K: Key, H: WriteHook<K>> Send for VarIter<'_, K, H> {}
```

`seize::LocalGuard` is intentionally thread-local. In `seize-0.5.1`, it stores the current thread reservation and includes `_unsend: PhantomData<*mut ()>` with a comment that `LocalGuard` must not be `Send` or `Sync` because it is tied to the current thread.

Moving `VarIter` to another thread can drop or refresh the guard on the wrong thread. This breaks `seize`'s safety invariant.

**Assessment:** valid and high priority. Remove the unsafe `Send` impl, or redesign iterators to use `OwnedGuard` if cross-thread iteration is required. The same pattern should be checked in `ConstIter` and `TypedIter`.

### 2. Node `mark()` reinterprets `AtomicPtr<T>` as `AtomicUsize`

**Severity:** high / critical portability and unsafe-code concern  
**Files:** `armdb/src/skiplist/node.rs`  
**Source review:** Kimi K2.6

`mark()` uses:

```rust
let atomic_usize: &AtomicUsize =
    unsafe { &*(self.tower(0) as *const AtomicPtr<Self> as *const AtomicUsize) };
let old = atomic_usize.fetch_or(1, Ordering::AcqRel);
```

This relies on layout and aliasing compatibility between `AtomicPtr<Self>` and `AtomicUsize`. It also relies on pointer tagging with the low bit. Heap node alignment likely makes the low-bit tag workable on supported targets, but the type pun is not a strong Rust contract.

**Assessment:** valid. Prefer representing tower entries as an explicit atomic integer/tagged-pointer type, or store a separate atomic mark bit. This impacts `ConstNode`, `VarNode`, and `TypedNode`.

### 3. `VarTree::insert_locked` ignores `InsertResult::Exists`

**Severity:** medium to high  
**File:** `armdb/src/var_tree.rs`  
**Source reviews:** GLM 5.1, DeepSeek

Current code:

```rust
if self.index.get(key.as_bytes(), guard).is_some() {
    return Err(DbError::KeyExists);
}

let (disk_loc, _gsn) = inner.append_entry(...)?;
let node_ptr = VarNode::alloc(*key, disk_loc, height);
self.index.insert(node_ptr, guard);
Ok(())
```

If `insert()` returns `InsertResult::Exists`, the newly allocated node is not freed and the method still returns `Ok(())`.

Important nuance: for normal public `insert`, same-key operations route through `shard_for(key)` and hold the same shard lock, so the common "two public inserts of the same key" race is mostly serialized. The finding is still valid because `insert_locked` is also used by `VarShard`, and future or replication/internal paths should not rely on this unchecked invariant. `put_locked` already handles `Exists` correctly.

**Assessment:** valid, but the likelihood is lower than some reviews claimed. Match on `InsertResult`, deallocate the rejected node, and return `DbError::KeyExists`.

### 4. Cached variable-value reads only support up to two blocks

**Severity:** high for large values  
**Files:** `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`  
**Source reviews:** GLM 5.1 and DeepSeek pointed near this area, but did not identify the full issue

`extract_from_block` handles either:

- value fully in the current 4096-byte block
- value spanning into exactly one next block

```rust
let next = next_block()?;
let first_part = &block[start..];
let second_len = len - first_part.len();
combined.extend_from_slice(first_part);
combined.extend_from_slice(&next[..second_len]);
```

If `second_len > 4096`, slicing `next[..second_len]` panics. Therefore values larger than roughly `4096 - start + 4096` are not safely supported by the cached read path.

`read_value_locked` uses `direct::pread_value` and can read larger values from disk, but normal `get()` and iterators use `read_value_cached`.

**Assessment:** valid. Either document and enforce a max variable value size, or implement multi-block assembly.

### 5. Entries larger than the write buffer can overflow `WriteBuffer::append`

**Severity:** high for large values  
**Files:** `armdb/src/shard.rs`, `armdb/src/entry.rs`  
**Source review:** additional Codex finding

`append_entry` serializes the whole entry, flushes if the current buffer cannot fit it, then appends:

```rust
if self.write_buf.is_full(buf.len()) {
    self.flush_write_buf()?;
}

let entry_offset = self.write_buf.append(&buf);
```

`WriteBuffer::append` then copies into `self.buf[self.len..self.len + data.len()]` without checking whether `data.len()` exceeds the buffer capacity. A single serialized entry larger than `write_buffer_size` will panic.

**Assessment:** valid. Add validation in `append_entry` or `WriteBuffer::append`, or support a direct-write path for oversized values.

### 6. Encrypted `VarTree`/`VarMap` `read_value_locked` uses unencrypted `pread_value`

**Severity:** high when `feature = "encryption"` and old-value/CAS/update paths are used  
**Files:** `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`, `armdb/src/shard.rs`, `armdb/src/io/direct.rs`  
**Source review:** additional Codex finding

`VarTree::read_value_locked` and `VarMap::read_value_locked` are used by:

- `put` when `H::NEEDS_OLD_VALUE`
- `delete` when `H::NEEDS_OLD_VALUE`
- `cas`
- `update` / `fetch_update`
- shard-handle `get`

These functions read disk via:

```rust
let bytes = direct::pread_value(file, disk.offset as u64, len).ok()?;
```

The block-cache path goes through `Shard::read_block`, which decrypts pages when encryption is enabled. There is also `direct::pread_value_encrypted`, but `read_value_locked` does not use it.

If encrypted data has already been flushed to disk, these old-value/CAS/update paths can read ciphertext and treat it as plaintext. Basic encrypted `VarTree::get()` tests do not cover this path because `get()` uses block reads.

**Assessment:** valid. Reuse a shard-level value read method that handles encryption, or add encryption-aware logic to `read_value_locked`.

### 7. `migrate` hook semantics are wrong or at least contradictory

**Severity:** medium / high depending on hook users  
**Files:** `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`, `armdb/docs/hooks.md`  
**Source reviews:** GLM 5.1 and DeepSeek noticed related symptoms; Codex identified the contract mismatch

Docs say:

- `migrate()` should not fire `on_write`.
- `migrate()` `Keep` and `Update` should fire `on_init` when `NEEDS_INIT`.
- Later in the same hooks doc, VarTree/VarMap are listed as not supporting `on_init`.

Current implementation for `VarTree`:

```rust
MigrateAction::Keep => {
    self.hook.on_write(&node.key, None, Some(&value));
}
MigrateAction::Update(new_value) => {
    self.put(&node.key, &new_value)?;
}
MigrateAction::Delete => {
    self.delete(&node.key)?;
}
```

`Update` and `Delete` call public methods, so they also fire `on_write`. `VarMap` has the same pattern.

This differs from `ConstTree`/`ConstMap`/`TypedTree`/`TypedMap`, where migration is hook-free for writes and fires `on_init` for kept/updated values.

**Assessment:** valid. Decide the intended API:

- If VarTree/VarMap should support `on_init`, implement migrate/replay behavior consistently and avoid `on_write` during migration.
- If VarTree/VarMap intentionally do not support `on_init`, update the docs and avoid fake `on_write` for `Keep`.

### 8. `DiskLoc.file_id` truncates internal `u32` file IDs to `u16`

**Severity:** medium now, high for long-running databases  
**Files:** `armdb/src/disk_loc.rs`, `armdb/src/shard.rs`, `armdb/src/compaction.rs`, `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`  
**Source review:** additional Codex finding

`DiskLoc` stores:

```rust
pub file_id: u16,
```

But shards use `u32` file IDs internally and repeatedly cast:

```rust
self.active.file_id as u16
new_file_id as u16
```

After 65535 files on a shard, file IDs alias in `DiskLoc`. This can read from the wrong file or fail recovery/compaction assumptions. Current config validation limits `max_file_size` to 4 GiB because `offset` is `u32`, but there is no equivalent guard for `file_id`.

**Assessment:** valid. Either make `DiskLoc.file_id` `u32`, enforce a hard maximum file ID, or implement a generation/epoch scheme.

### 9. Concurrent compaction can make stale copied `DiskLoc` reads return `None`

**Severity:** medium  
**Files:** `armdb/src/var_tree.rs`, `armdb/src/var_map.rs`, `armdb/src/compaction.rs`, `armdb/src/shard.rs`  
**Source review:** Hy3 had a broad compaction-safety concern, but not this exact mechanism

Read path copies `DiskLoc` from the index under a guard, then reads the file:

```rust
let disk = *node.load_disk();
self.read_value_cached(&disk)
```

Compaction can:

1. Add the new compacted immutable file.
2. Update the in-memory index via `update_if_match`.
3. Remove old files from `inner.immutable`.
4. Delete old files on disk.

If a reader copied the old `DiskLoc` before step 2, but reaches `read_block` or `read_value_locked` after step 3, `Shard::read_block` cannot find the old file in `inner.immutable` and returns an error. The public `get()` path maps this to `None`.

The code keeps `Arc<ImmutableFile>` alive only if `Shard::read_block` captured it before removal. A stale `DiskLoc` alone does not protect the immutable file.

**Assessment:** plausible and important. The current weak-consistency docs allow stale values, but returning `None` for a live key during compaction may be too weak for expected semantics. Possible fixes include deferred immutable-file retirement, generation-protected file handles, retry on `NotFound` by re-reading the index, or keeping old immutable files readable until no readers can hold stale `DiskLoc`s.

## Partially Valid / Lower Priority Findings

### `read_value_locked` does not use block cache for spanning values

`read_value_locked` checks the write buffer first and then only uses block cache for values within a single block. For spanning values it falls back to `direct::pread_value`.

This is not a correctness bug for unencrypted data, but it can cause disk I/O under the shard lock for larger values. GLM correctly identified the performance issue, but overstated it as a correctness issue. The more serious correctness bug is the encrypted `pread_value` path and the two-block limit in `read_value_cached`.

### `read_value_cached` first-block cache hit skips write-buffer check for the second block

DeepSeek described a potential stale read if the first block is cached and the second block is still in the write buffer. In current `Shard::read_block`, active file blocks are never cached, so this exact scenario should not occur for unflushed active data. The concern would become valid if active blocks were cached in the future.

### Long `migrate` holds a `seize` guard

`VarTree::migrate` keeps a guard during the whole level-0 traversal. This delays reclamation while migration runs. This is a resource/latency issue, not immediate unsafety. `VarMap::migrate` avoids this pattern by copying shard keys into a `Vec`, but that has its own memory spike.

### `prefix_to_end_bound` panics if prefix is longer than key size

This is valid for `VarTree::prefix_bounds`: it copies `prefix` into `K::zeroed()` without checking `prefix.len() <= size_of::<K>()`. This should be guarded or documented. It is a lower-priority API hardening issue.

### `shard_for` uses `K::zeroed()` for partial-byte prefix hashing

This is just overhead and code smell. It is not a bug. Note that `VarTree` copies `full_bytes + 1`, while `VarMap` copies `full_bytes` then sets the masked byte; both result in the same hashed prefix bytes.

### `resolve_front_asc` and `resolve_front_rev` are identical

True, but this is duplication, not a correctness problem. It may be intentional separation for future changes.

## Invalid or Stale Findings

### Hy3: `VarNode` is missing from code

Invalid. `VarNode` exists in `armdb/src/skiplist/node.rs` behind `feature = "var-collections"`.

### Hy3: tombstones are represented as `DiskLoc` in the live index

Invalid for current code. Deletes append tombstone entries to disk and remove the node from the in-memory index. Public `get()` sees absence via `index.get() == None`, not via a tombstone `DiskLoc`.

### Hy3: CAS is sequence-number based

Invalid for current code. `VarTree::cas` compares current value bytes, not sequence numbers.

### Hy3: `shard_count` must be a power of two for `shard_for`

Invalid. Current `shard_for` uses modulo `self.engine.shards().len()`, not a bit mask. `Config::validate` only requires `1..=255`.

### GLM / DeepSeek: public same-key `insert` race is common across shards

Mostly invalid as stated. Same key always maps to the same shard, and public `insert` holds that shard lock before calling `insert_locked`. The ignored `InsertResult` is still a real robustness bug, but the common public race scenario is overstated.

### Kimi: separate `DiskLoc` retire makes `load_disk()` references dangling

Not valid as described for current use. Readers enter a `seize` guard before obtaining the node, then copy `DiskLoc` immediately:

```rust
let disk = *node.load_disk();
```

Old `DiskLoc` pointers are retired via the same collector, so active guards protect them until readers leave. The API returning `&DiskLoc` is still easy to misuse in future code, but current call sites generally copy the value immediately.

### Kimi: `ByteView` relaxed clone refcount is a data race

Invalid. For immutable refcounted data, `fetch_add(Relaxed)` on clone and `fetch_sub(AcqRel)` on drop is a standard pattern. There is no mutation of the pointed-to bytes after construction. The more relevant checks are overflow of `AtomicU32` and whether all `ByteView` copies go through `Clone`; current type is not `Copy`.

### Kimi: `SkipList::height` race causes search correctness/performance issues

Not valid under the current `write_lock` model. Inserts are serialized; readers may observe a higher height before all links are populated, but null at an upper level just falls through to lower levels. This is acceptable for correctness. If the write lock is removed in the future, the insert algorithm needs a full CAS-based redesign.

### GLM: all-`0xFF` prefix returning `Unbounded` is incorrect

Invalid. For ascending lexicographic prefix bounds, a prefix consisting only of `0xFF` bytes has no finite exclusive upper bound in the same key space. Returning `Unbounded` is mathematically correct. It may scan more than expected only if callers misunderstand the ordering, but it should not include keys that are lexicographically outside the prefix range for fixed-size keys starting with all `0xFF`.

## VarMap-Specific Notes

Most variable-value storage concerns apply to both `VarTree` and `VarMap`:

- two-block limit in `read_value_cached`
- oversized write buffer panic
- encrypted `read_value_locked` path
- `migrate` hook contract mismatch
- `DiskLoc.file_id` truncation
- stale copied `DiskLoc` during compaction

`VarMap` does not share the SkipList-specific issues:

- no `VarIter: Send`
- no pointer-tagged tower mark
- no `VarTree::insert_locked` ignored `InsertResult`

`VarMap::insert_locked` is simpler and currently safe against duplicate insert under the held per-shard `HashMap` lock:

```rust
if index.contains_key(key) {
    return Err(DbError::KeyExists);
}
let (disk_loc, _gsn) = inner.append_entry(...)?;
index.insert(*key, disk_loc);
```

## Recommended Fix Order

1. Remove unsafe `Send` impls for iterators carrying `seize::LocalGuard`, or switch them to `OwnedGuard`.
2. Replace `AtomicPtr` to `AtomicUsize` aliasing in node marking with a representation that is explicit and justified.
3. Fix encrypted `read_value_locked` paths for `VarTree` and `VarMap`.
4. Enforce or implement large-value support:
   - validate max entry size against `write_buffer_size`
   - make cached reads support arbitrary multi-block values, or document a hard max
5. Fix `VarTree::insert_locked` to handle `InsertResult::Exists`.
6. Decide and implement VarTree/VarMap migration hook semantics consistently with docs.
7. Address `DiskLoc.file_id` overflow.
8. Decide whether weak consistency permits transient `None` during compaction; if not, add retry/deferred file retirement.
9. Add API validation for prefix length.

