Durable KV prefix cache: agent prefixes survive eviction, restart, and cold nodes

[michaelneale]

Closes #1226 (partially — W0/W2/W2b/W4 plus a dense bridge; W1/W3/W5/W6/W7 not done).

What you get

A prefix computed once survives the process that computed it. With the opt-in
disk tier, the first request after a node restart mmaps the KV page a previous
process wrote instead of prefilling it again.

Measured on an M4 Max, release host plus Metal native runtime built from this
branch, Qwen3-8B Q4_K_M layer package, --ctx-size 16384, 8 GiB tier, ~8.6k-token
agent prompt. Cold runs start from an empty cache directory.

Scenario	Time	cached_tokens
Cold, empty tier	7.88s	0
New session, same system prompt, different tail	7.30s	768
Third distinct tail	6.48s	2688
First request after process restart	0.58s	8576 of 8609

The restart case is the result to look at: 13.6x, reproduced across two
separate verified restarts (0.58s and 0.58s). 8576 of 8609 tokens were restored
from disk.

On the cross-session rows. They hit, but modestly — a few hundred to a few
thousand tokens of an 8.6k prompt, worth a fraction of a second. An earlier
revision of this description claimed 12.65s saved cross-session; that number
came from a configuration I could not reproduce at the shipped defaults, and it
has been removed rather than restated. The durable, reproducible win here is the
restart case, and the honest summary is that this PR makes prefixes survive
time on a node — not that it makes cross-session sharing dramatically faster
within one process.

Verifying a restart actually happened matters. During this measurement a
pkill silently failed to match, the replacement process died on
Address already in use, and the "post-restart" request was served by the
original process — which manufactures a perfect-looking cache hit. Every restart
number above is from a kill verified by an empty lsof -ti:9337 before the new
process started.

The record ladder

Cross-session sharing already worked — prefix_hash_with_namespace contains no
session_id. It was recording that was throttled: a policy that always keeps
the full length meant an 8000-token prompt recorded only [8000, 7936], both in
the request's own tail, the least shareable part of it. Lookup probed dozens of
lengths including 2048; nothing was ever stored there.

The ladder keeps the exact length and the near-tail candidate (same-session
continuation) and reaches down into the shared system-prompt region.

A second defect in that ladder was found and fixed while validating this
branch, and it is worth reading before reviewing the policy. The ladder charged
its two unconditional slots — exact and near-tail, the longest candidates by
construction — against the resident token budget before considering any shared
rung. For a 12288-token prompt those two pin ~24k tokens, more than the entire
budget on any context below roughly 48k, so the first shared rung was
unaffordable and the loop exited immediately. Composed over the shipped config
chain (n_ctx → max_entries → record_limit → resident budget):

before:  ctx=8192 -> [12288, 12160]     ctx=16384 -> [12288, 12160]
after:   ctx=8192 -> [12288, 12160, 896] ctx=16384 -> [12288, 12160, 3328, 896]

Only a 131k context recorded anything shareable beforehand. The budget now
governs the optional rungs only; the mandatory slots are already committed by
the time it is consulted. record_candidate_token_counts has a regression test
over the composed shipped configuration, because every hand-picked-policy test
missed this.

This fix is not load-bearing for any number in this PR, and I want that on the
record. A live A/B against SKIPPY_KV_CACHE_SHARED_RECORD_LIMIT=2 (the old
effective behaviour) was identical on every row within noise — cold 7.88s vs
7.89s, restart 0.58s vs 0.60s, both tiers ending with one on-disk entry. The
archive selector caps one page per request, so ladder depth never reaches disk,
and the resident-path cross-session hits occur identically at limit 2. The fix
makes recorded behaviour match documented intent; it does not produce a measured
speedup here.

Two findings worth your attention

1. topology_id made persistence impossible — and removing it removed a safety net.

Local serving derives it as topology-mesh-skippy-{unix_nanos}, so it is unique per process. While it was hashed, every restart produced fresh page_ids and no persistent cache could ever be read back. Removing it is required for any disk tier.

But it was also, accidentally, the thing guaranteeing no stale page could ever be misread. model_id is a display name, not a content digest — two runs can serve genuinely different tensors under the same alias (different quant, repacked layer package, GGUF swapped under the same path). So identity now hashes manifest_sha256, source_model_sha256, package_ref and load_mode explicitly. This is the change I'd most like a second opinion on, because the failure mode is silent numerical corruption that persists across restarts, not a crash.

2. Archiving the longest candidate is useless. When I capped archiving to one page per request, I initially picked the longest — which is the request's own tail, which nothing else ever asks for. Cross-restart hits vanished until it was changed to the lowest (most shareable) candidate. Caught by measurement, not by tests.

Safety

The tier's failure mode is wrong numbers, not a crash, so:

• Payload bytes are checksummed and verified before import; a mismatch quarantines the entry rather than being treated as a miss.
• The KV page descriptor is persisted with the bytes — without it, archived pages survive and are silently unusable (a real bug found by running a live model, now covered by a regression test).
• Descriptor token ranges are cross-checked against the looked-up identity before import; a failed import stops probing rather than layering a second page onto a dirtied session.
• Cache directories take an exclusive lock. A second instance on the same directory declines the tier instead of sharing it, because the index is last-writer-wins and orphan reclaim would delete the other instance's live, mapped files.
• Mapped payloads own no blob-store blocks, so promote/re-evict cannot double-release. Covered by a test.

Default path

The disk tier is off unless SKIPPY_KV_DISK_TIER_MIB / SKIPPY_KV_DISK_TIER is set. With it off, the only behaviour changes are the identity hash and the ladder depth.

Architecture

• skippy-cache/src/disk_tier.rs — mmap-backed store: atomic rename, checksums, LRU, directory lock, orphan reclaim.
• CacheBytesRepr::Mapped borrows a mapped range instead of allocating. Block-deduping on disk was rejected: as_cow() on a Blocks payload concatenates the whole thing, which for a multi-GB page is gigabytes of copying immediately before the runtime copies again.
• skippy-server/src/kv_integration/dense_disk.rs — dense (ResidentKv) families have no serialized form, so without this bridge the tier would help only hybrid/recurrent models and do nothing for llama/qwen/gemma. Archiving happens at record time, not eviction: eviction runs on the decode hot path, and deferring it would mean the llama.cpp sequence cannot drop until the export completes (use-after-drop, or a cell leak re-triggering the 502 wedge max_resident_tokens exists to prevent).
• Ladder depth is bounded by a token budget, not a slot count — the entry-count cap assumes ~min_tokens-sized entries, which the two longest slots violate badly.

Protocol

Node-local only. No wire-format, gossip, or plugin-protocol changes. Skippy ABI unchanged (0.1.35). Disk format is versioned and self-invalidating.

RuntimeKvPageDesc gains serde derives — plain data mirror, layout unaffected.

Validation

cargo fmt --all --check clean; clippy -D warnings clean on skippy-cache, skippy-server, skippy-runtime, mesh-llm-host-runtime, mesh-llm. Tests: skippy-cache 105 + 7 integration, skippy-server 442, mesh-llm-host-runtime 1932, skippy-runtime 69 — all passing. Full CI green on the rebased head.

Live verification on a real dense model as described above, including a genuine process restart against a warm cache directory.

Two-node split serving (validated)

Measured on a 2-node loopback split, Qwen3-8B Q4_K_M layer package, ~16.9k-token agent prompt, 10 GB VRAM budget and an 8 GiB disk tier per stage:

Scenario	Time
Cold, both stage caches empty	31.02s
Cross-session, same prefix, new tail	1.29s / 0.91s
First request after restarting both nodes	1.54s

Restart reuse across a split now works — a 20x win. An earlier 0.5B run showed no restart gain because the downstream stage archived only its 512-token floor candidate and the all-or-nothing veto cancelled stage 0's deeper archive. Both stages now archive the 16768-token shared bulk:

stage 0 : [16768]                                   <- one page, the shared bulk
stage 1 : [16768, 16640, 16512, ...] (88 entries)   <- full ladder

Fixed by e1c4af42 (stop min_record_tokens starving the archive selector on a warm restore; archive on every stage, not only stages with a downstream) and b32dec97 (drop the resident-admission gate on stage-0 full prefill, which declined every rung of a prompt over max_resident_tokens).

Two physical machines over LAN (validated)

The table above is a loopback split. Repeated across an M4 Pro and a Mac mini on a LAN (13-14ms direct QUIC), Qwen3-8B Q4_K_M layer package, --max-vram 4 --ctx-size 16384 per node, 8 GiB tier per node, stage 0 layers 0-22 on one machine and stage 1 layers 22-36 on the other:

Scenario	Time	cached_tokens
Cold, both caches empty	12.45s	0
Cross-session, same prefix, new tail	1.49s / 1.53s	4096
First request after restarting both machines' nodes	1.72s	4096

Both nodes independently persisted format_version: 1 indexes and reloaded them into a freshly negotiated topology, so restored page ids agreed across hosts. Stage 1 also exercised budget eviction (disk_evictions 0 → 3 → 5 against its 2 GiB share) while still serving hits — a path the loopback run never reached.

Re-run under a pinned topology (--split --split-topology-lock, stage 0 layers 0-32 on the M4 Pro, stage 1 layers 32-36 on the mini, --ctx-size 8192), verified via GET /api/runtime/stages rather than /v1/models:

Scenario	Time	cached_tokens
Cold, both caches empty	5.48s	0
Cross-session, same prefix, new tail	1.39s / 1.29s	4096
First request after restarting both nodes	1.48s	4096

This replaces an earlier run that reached a split by tuning --max-vram, which is unreliable — at --max-vram 5 one node silently served all 36 layers solo while still looking like a healthy two-node mesh. docs/skippy/KV_RETENTION_PLAN.md now records the lock recipe and the traps.

Agent harness (validated)

AGENTS.md requires a harness run for changes on this surface. Goose was run
against the local proxy with tool calling, on the same node and tier as above.

Check	Result
Goose, model=auto, file-creation task with tool calls	completed, hello.txt written
Goose, model=<explicit model id>, shell tool call	completed, 39.9s
Goose, model=auto, after a verified node restart	completed, 35.2s
Multi-turn tool-call loop, assistant tool_calls + role=tool reply	completed, no reducer or schema errors

The disk tier stayed healthy across the harness: four archived entries
(4992/5120/5632/4992 tokens), 1.5 GB, no corruption or quarantine events. A
5708-token harness prompt returned cached_tokens=5632 at 1.80s after a restart,
against 5.78s cold — the restart win holds under a real agent workload, not just
scripted requests.

model=mesh was not exercised: on a single-node mesh the proxy returns
model 'mesh' not found (no local or remote host serving this model), which is
correct behaviour for that topology rather than a defect. It is covered by the
two-node runs above.

Not validated: WAN split, very large MoE models, OpenCode and Pi harnesses.

Observability, tests, and format spec (review round 2)

Follow-up to @i386's review, in f320fa27:

• Hit attribution. Exact-state hits carry skippy.exact_cache.hit_source (ram | disk).
• Archive outcomes. archive_dense_prefix previously returned Result<bool>, so "policy declined" and "the archive failed" collapsed into the same silent false — and two of the four call sites discarded it with let _ =. It now returns an explicit outcome and every site reports skippy.kv.archive_status (archived / skipped_too_short / skipped_already_archived / skipped_tier_disabled / failed_export / failed_write / failed_error), plus archive_bytes, archive_export_ms, archive_write_ms.
• Disk-tier counters exported. disk_demotions, disk_promotions, disk_evictions, disk_corrupt_entries, disk_verifications, disk_verifications_skipped, disk_entries, disk_bytes, disk_max_bytes, disk_tier_enabled. Without these, a tier that has silently stopped storing anything is indistinguishable from one that is never probed.
• Payload-kind coverage. RecurrentOnly had no disk round-trip at all; it now has demote → restart → restore. Cross-kind rejection went from 1 of 6 pairs to all 6, plus a test that a kind mismatch keeps the entry quarantined against a later correct lookup.
• Format spec. docs/skippy/KV_DISK_TIER_FORMAT.md — directory layout, index schema, lifecycle, atomic-write and crash-recovery contract, both checksummed regions, full corruption-response table, identity contract, versioning rules. The JSON index is retained deliberately: metadata only, read once at startup, entry counts in the hundreds. A binary index should follow a profile, not precede one.
• User docs. website/src/docs/pages/kv-disk-cache.md plus a nav entry: how to enable, when it declines to enable and why, safety model, how to clear it.
• DISK_TIER_FORMAT_VERSION reset 3 → 1 before landing.

Open questions for review

1. Is the weight-identity set (manifest_sha256 + source_model_sha256 + package_ref + load_mode) sufficient? Should the tier refuse to open when both digests are absent?
2. n_gpu_layers and backend device are hashed conservatively, but auto-offload can shift between runs on the same host — that will silently self-invalidate the tier. Right tradeoff?
3. Archive-at-record costs one KV export per request even when nothing later reads it. Worth gating behind observed hit rate (i.e. land W2 export first, then decide)?

Summary by CodeRabbit

• New Features

  • Added optional persistent KV-prefix caching with disk-backed storage and memory-mapped restores.
  • Dense KV prefixes can be archived and restored after memory eviction or process restarts.
  • Added cache statistics and miss classifications.

• Improvements

  • Shared-prefix selection adapts to cache capacity and token budgets.
  • Cache identities distinguish incompatible runtime configurations while supporting restart reuse.
  • Restoration falls back across exact, resident, and archived prefixes.
  • Added CLI and documentation for disk-cache configuration.

• Bug Fixes

  • Improved handling of corrupted or incompatible cached data.
  • Added validation for short activation frames during forwarding.

---

Review round 3

Architecture and byte order are now bound into page identity. Reviewer asked for explicit byte-order and architecture identifiers. update_layout_identity hashed cache dtypes, flash-attn, GPU layer split, and backend_device — but not CPU arch, endianness, or pointer width. The identity hash deliberately encodes its own integers little-endian (correct for a portable id), which meant two hosts of different native endianness computed the same page id for the same tokens.

Harmless in the default machine-local dir; not harmless once a dir is shared or copied, because SKIPPY_KV_DISK_TIER_DIR takes any path, the stage directory key holds only model/stage shape, and backend_device does not separate x86_64 CUDA from aarch64 CUDA, nor two CPU-only hosts both recording <no-selected-device>. Checksums would confirm the bytes arrived intact and the runtime would import them as native — a silent misread. update_platform_identity now hashes ARCH + endianness tag + pointer width, so a foreign page is a miss, never a wrong hit.

Chosen over a .kvp magic header: no change to component offsets or file sizing, and mismatches become different ids rather than mutual quarantine between platforms sharing a directory. No format bump — v1 has not shipped, so no on-disk directory claiming it exists in any build a user could have run; the spec now says the bump rule applies from the first released version onward.

Retention policy documented, no TTL added. Entries are content-bound: age alone does not make one wrong, and they become unreachable rather than incorrect when model or config changes. Idle entries consume no more than their existing allowance, so expiry only forfeits future hits. A TTL would be a privacy/hygiene control, not correctness. The real gap is different and now recorded: stage directories abandoned on a model_id or stage-shape change fall outside any tier subsequently opened, which wants a base-directory quota rather than expiry inside active caches.

Rebased onto main (13 commits, including the regenerated llama.cpp patch queue). That bumped the Skippy ABI to 0.1.38, so all earlier live measurements were re-run against a freshly built native runtime at the merged head 85f0060d: 7.57s cold → 0.62s after a genuine process restart, cached_tokens=8320, hit_source":"disk". Full CI is green on the merged head.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This change adds adaptive shared-prefix recording, complete cache identities, mmap-backed disk retention, miss tracking, dense KV archival, disk restoration fallbacks, runtime configuration, and non-first-stage activation-frame validation.

Changes

KV prefix retention

Layer / File(s)	Summary
Identity, payload, and candidate selection crates/mesh-llm-host-runtime/..., crates/skippy-cache/src/config.rs, crates/skippy-cache/src/identity.rs, crates/skippy-cache/src/payload/*, crates/skippy-runtime/src/types.rs	Cache identities include runtime layout, platform, backend, and model artifact data. Candidate recording uses geometric targets and resident-token limits. Payloads support mapped bytes and resident KV archives.
Disk tier and cache state crates/skippy-cache/src/disk_tier.rs, crates/skippy-cache/src/exact_state.rs, crates/skippy-cache/src/miss_reason.rs, crates/skippy-cache/tests/agentic_retention.rs	The cache persists evicted payloads in a bounded, locked, checksum-validated mmap tier. Exact-state lookup restores disk entries and records miss classifications and cache statistics.
Dense archive configuration and lifecycle crates/skippy-server/src/kv_integration/*, crates/mesh-llm-cli/src/parser/commands.rs, crates/mesh-llm-host-runtime/src/runtime/*, crates/mesh-llm/src/lib.rs	Runtime and CLI options configure the disk tier. Server configuration applies content-digest validation, node budgets, stage shares, archive selection, and validated dense archive restoration.
Server restoration and transport validation crates/skippy-server/src/binary_transport/*, crates/skippy-server/src/frontend/*	Request paths add disk-archive fallbacks and archive telemetry. Non-first stages reject short activation frames when token counts differ from the input.
Public wiring and retention documentation crates/skippy-cache/src/lib.rs, crates/skippy-server/src/telemetry.rs, docs/skippy/*, website/src/docs/pages/kv-disk-cache.md	The cache exports disk and miss-tracking APIs. Telemetry caches stderr configuration. Documentation specifies the disk format, retention status, configuration, supported models, and cleanup behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Mergeability Score: 🟡 Moderate · up to 9928b

The opt-in persistent KV cache currently has concrete correctness and availability risks: stale metadata can accumulate without bound, and some archives can be reported as successful while becoming unusable after restart; relative cache directories can also receive an incorrect disk budget. These issues should be fixed or explicitly accepted before merging.

Possibly related issues

• KV prefix retention: durable disk-backed prefix cache so agent prefixes survive eviction, restart, and cold nodes #1226 — Directly covers durable shared-prefix retention, identity completeness, miss tracking, mmap persistence, and dense-family archival.
• Persist KV prefixes for sliding-window attention models (composite base + SWA page) #1264 — Covers the durable KV disk tier, including unsupported sliding-window model handling and archive rejection.
• Harden prefix-cache identity, residency accounting, and staged-runtime observability #1097 — Covers related identity, residency, eviction, miss telemetry, and runtime observability changes.

Possibly related PRs

• Mesh-LLM/mesh-llm#1041 — Overlaps with prefill recording, prefix-candidate handling, and binary transport paths.
• Mesh-LLM/mesh-llm#1118 — Overlaps with FamilyPolicy KV-cache policy derivation.
• Mesh-LLM/mesh-llm#1146 — Relates to exact-state token-count discovery and identity lookup for retained prefixes.

Suggested labels: experimental

Suggested reviewers: i386, ndizazzo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary change: durable KV prefix retention across eviction, restart, and cold-node operation.
Linked Issues check	✅ Passed	The changes address durable retention, partial-prefix reuse, identity validation, mmap restoration, telemetry, eviction handling, and solo or split serving.
Out of Scope Changes check	✅ Passed	The changes remain centered on durable KV retention, including implementation, configuration, validation, telemetry, and documentation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/kv-prefix-retention

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review.

[michaelneale]

@i386 review please when you get a chance.

Two things I'd most like your eyes on:

1. Weight identity in skippy-cache/src/identity.rs. I removed topology_id from the prefix hash — it's derived from unix_nanos per process, so while it was hashed no persistent cache could ever be read back after a restart. But it was also, accidentally, the thing making collisions impossible. model_id is a display name, not a content digest, so I added manifest_sha256 / source_model_sha256 / package_ref / load_mode to the hash. If that set is incomplete, the failure mode is silent numerical corruption that survives restarts rather than a crash — so it's worth a sceptical read.

2. Whether archive-at-record earns its keep. Every request with a recorded prefix pays one KV export even if nothing ever reads it back. It might be better to land the miss-reason metrics export first and let the hit-rate data decide.

Draft because the multi-node / split-topology / agent-harness validation from AGENTS.md isn't done, and the only live numbers are from a 0.5B dense model on one box. The disk tier is off by default (SKIPPY_KV_DISK_TIER_MIB), so the default serving path only sees the identity change and the deeper record ladder.

[michaelneale]

Followed up on the "surely this saves a ton on a big model" question — it does, and my first numbers undersold it badly.

Re-measured on Qwen3-8B Q4_K_M (layer package / staged) with a ~12.4k-token agent prompt, cold control = same binary against an empty cache dir:

Scenario	Cold	Warm	Saved
Cross-session, same prefix, new tail	21.50s	8.85s	12.65s
First request after full process restart	21.60s	8.85s	12.75s (2.44×)

The archived page is 0.96 GB for 12288 tokens.

The detail I think is the actual result: the post-restart number (8.85s) is identical to the cross-session number (8.85s). Restoring a ~1 GB page via mmap costs the same whether this process computed it or a previous one did. That is the plan's ratio argument holding — restore is bounded by bytes, prefill is superlinear in tokens, so the bigger the reusable bulk the better this looks.

The 0.5B numbers I led with earlier (0.31s → 0.25s) are the worst case, not the representative one. I've kept them in the PR for honesty but they were misleading as the headline.

Caveat unchanged: this is single-node. Split topologies are the interesting open question, since per-stage pages are smaller but every stage has to hit for the pipeline to benefit — one cold stage negates the rest.

[michaelneale]

Splits now covered — with one honest gap

Agreed splits are where the payoff is, so I dug in. Findings:

The hard problem I expected wasn't there. I assumed per-stage restore would need a new negotiation protocol (stage 0 restores N tokens, stage 1 must agree or attention goes wrong). It already exists: try_restore_embedded_split_prefill has stage 0 state its restore length on the wire, and any stage's miss vetoes the whole attempt, with misses propagating back through middle stages. So this is a third tier under an existing gate, not a redesign.

What I added: disk restore in restore_binary_prefix (one site covering both binary lookup branches) and in split stage 0's frontend path, plus archive-on-record in all four recorders. A disk hit is deliberately indistinguishable from a resident hit so the veto keeps working unchanged.

A real bug the measurement caught. My first archive heuristic stored the lowest ladder candidate. For a 2129-token prompt that's a 256-token page — 12% of the prefill, pure noise. ArchiveCandidate now picks the longest prefix that still excludes the request's tail (2048 here, 96%), which is the actual shared system-prompt bulk. Unit-tested.

Safety. Promoted an undocumented invariant to an explicit assertion: a non-first stage must execute its full token range. Suffix-only execution is legal only on the stage owning layer 0 — anywhere else the next stage attends over a prefix it never received and emits plausible but wrong tokens. Previously this was caught only as a side effect of a payload-size check in the encoder. Now named, asserted, and tested both ways.

Measured, 2-node loopback split (0.5B, ~2.1k tokens)

Scenario	Cold	Warm
Cross-session, new tail	1.34s	0.57s
First request after restarting both nodes	1.33s	1.32s

Cross-session split reuse works. Restart reuse does not pay off yet. The archive index shows why: stage 0 archives its full ladder including the 2048 bulk, the downstream stage archives only 512. Because of the veto, one stage's shallow archive negates the other's good one. I'd rather flag that than quote the flattering cross-session number alone.

ci-two-node-split-smoke.sh passes; the new invariant assertion never fires.

MoE

Not a special case. MoE families (qwen3moe, glm4_moe, deepseek3, openai_moe, llama4, hunyuan_moe...) map to the same dense resident_kv policy — MoE changes FFN weights, not KV shape. Only hybrid/recurrent families take the other path. So the large MoE models that motivate splits are covered by the dense path already.

Still unmeasured: large MoE on a real multi-machine split, which is where the solo 8B result (2.44×) suggests the payoff should be biggest.

[coderabbitai]

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/skippy-server/src/binary_transport/binary_kv.rs (1)

726-757: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Archive prefixes when the stage has no downstream peer.

Line 726 gates the archive operation on config.downstream.is_some() and output. A terminal binary stage still records resident prefixes, but it never writes its selected archive_candidate to disk. This removes restart reuse for that stage.

Move lines 746-757 after the activation-recording conditional. Keep activation recording conditional, but archive whenever archive_candidate.take() returns a candidate.

Proposed fix

-    if config.downstream.is_some()
-        && let Some(output) = output
-    {
+    if config.downstream.is_some()
+        && let Some(output) = output
+    {
         // activation recording
-        if let Some(identity) = archive_candidate.take() {
-            let mut runtime = runtime.lock().expect("runtime lock poisoned");
-            if let Ok(true) = kv.archive_dense_prefix(&mut runtime, session_id, &identity) {
-                attrs.insert(
-                    "skippy.kv.archived_tokens".to_string(),
-                    json!(identity.identity.token_count),
-                );
-            }
-        }
+    }
+    if let Some(identity) = archive_candidate.take() {
+        let mut runtime = runtime.lock().expect("runtime lock poisoned");
+        if let Ok(true) = kv.archive_dense_prefix(&mut runtime, session_id, &identity) {
+            attrs.insert(
+                "skippy.kv.archived_tokens".to_string(),
+                json!(identity.identity.token_count),
+            );
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-server/src/binary_transport/binary_kv.rs` around lines 726 -
757, Move the archive_candidate handling block out of the
config.downstream/output conditional so it runs for terminal stages as well.
Keep record_resident_activation and add_binary_activation_records conditional,
but always call archive_dense_prefix when archive_candidate.take() yields a
candidate and preserve the existing archived-token attribute update.

🧹 Nitpick comments (9)

crates/skippy-cache/src/lib.rs (1)

10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Drop the new crate-root re-exports and import from the owning modules.

disk_tier and miss_reason are already pub, so consumers can use skippy_cache::disk_tier::PrefixDiskTier and skippy_cache::miss_reason::PrefixMissReason directly. The added crate-root aliases create a second public path for the same types.

♻️ Proposed change

 pub use config::{PrefixCandidatePolicy, ResidentCacheConfig};
-pub use disk_tier::{DiskLoad, DiskTierStats, PrefixDiskTier};
 pub use exact_state::{
     ExactStateCache, ExactStateCacheStats, ExactStateLookup, ExactStateRecordOutcome,
 };
 pub use identity::{
     NATIVE_KV_DTYPE, NATIVE_KV_RUNTIME_ABI_VERSION, PrefixIdentity, activation_page_id,
     prefix_hash, prefix_hash_with_namespace, prefix_identity, prefix_identity_with_namespace,
 };
-pub use miss_reason::{PrefixGapBucket, PrefixMissReason, PrefixMissStats, PrefixMissTracker};

Update the importing call sites in crates/skippy-server accordingly.

As per coding guidelines: "Minimize crate-root re-exports. New code should import from the owning module directly, and transitional compatibility re-exports should be removed after call sites migrate."

Also applies to: 18-18

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/src/lib.rs` at line 10, Remove the crate-root re-exports
for disk-tier and miss-reason types from lib.rs, then update skippy-server call
sites to import PrefixDiskTier, DiskLoad, DiskTierStats, and PrefixMissReason
through skippy_cache::disk_tier and skippy_cache::miss_reason. Preserve existing
usage while eliminating the duplicate public paths.

Source: Coding guidelines

crates/skippy-cache/src/identity.rs (1)

73-76: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Device identity uses backend_device only, not stable_id.

backend_device is an index-based label such as CUDA0. On a machine with more than one GPU model, the device behind CUDA0 can change between runs after a driver or enumeration change. Persisted pages then restore under a different device than the one that produced them. StageDevice::stable_id is available and is the field that survives re-enumeration. Consider hashing stable_id when it is present, with the absent case tagged distinctly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/src/identity.rs` around lines 73 - 76, Update the
selected-device branch in the identity hashing logic to hash
StageDevice::stable_id instead of backend_device, preserving the
no-selected-device marker for None and using a distinct fallback marker when
stable_id is absent. Ensure device identities remain stable across
re-enumeration.

crates/skippy-cache/src/config.rs (1)

213-275: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider treating the two mandatory slots as part of the pinned budget check.

pinned starts at the sum of the exact and near-tail lengths, so those two slots always bypass max_resident_tokens_hint. On a small pool the ladder can therefore pin far more than the budget, as ladder_is_bounded_by_the_resident_token_budget shows (8000 + 7936 against a 4096 hint). The current behavior is intentional and documented, so this is only a design note: if the hint is meant as a hard ceiling, drop the near-tail slot when it alone exceeds the budget.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/src/config.rs` around lines 213 - 275, Apply
max_resident_tokens_hint to the mandatory near-tail selection in the
ladder-building logic: retain the exact-length slot, but only add near_tail when
the combined pinned length stays within the budget, dropping it when it alone
would exceed the hint. Keep the existing unconstrained behavior when the hint is
zero and preserve later candidate checks.

crates/skippy-cache/Cargo.toml (1)

18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use workspace versions for libc and tempfile; gate libc to unix.

crates/skippy-cache/Cargo.toml keeps local version pins while the root uses memmap2, serde, and serde_json from [workspace.dependencies]. libc.workspace = true requires adding libc to [workspace.dependencies], then moving it to [target.'cfg(unix)'.dependencies] because libc is only used in crates/skippy-cache/src/disk_tier.rs behind #[cfg(unix)]. tempfile is also only used in crates/skippy-cache test/dev paths, so it can use shared tempfile.workspace = true from [workspace.dependencies] if present before application.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/Cargo.toml` around lines 18 - 24, The dependency
declarations in skippy-cache should use workspace versions: add libc and ensure
tempfile is available in workspace dependencies, replace their local version
pins with workspace references, and move libc under target cfg(unix)
dependencies to match its cfg-gated use in disk_tier.rs. Keep tempfile as a
workspace dev-dependency.

crates/skippy-cache/src/disk_tier.rs (1)

431-445: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Re-verify the checksum once per mapping, not on every load.

load hashes every component on every call, including when the mapping is already present in self.mappings. The mapping is only established after a full verification, and entries are never modified in place, so repeated hashing of a live mapping adds no safety.

The cost is proportional to payload size on the restore hot path. The plan records a 0.96 GB archived page (docs/skippy/KV_RETENTION_PLAN.md, Line 493), so each repeat hit pays a full-payload BLAKE3 pass over ~1 GB.

Track verified page ids alongside mappings and skip the hash when the mapping was already verified in this process.

♻️ Proposed refactor sketch

-        let mmap = match self.mappings.get(page_id) {
-            Some(mmap) => mmap.clone(),
+        let (mmap, already_verified) = match self.mappings.get(page_id) {
+            Some(mmap) => (mmap.clone(), true),
             None => {

                 let mmap = Arc::new(mmap);
                 self.mappings.insert(page_id.to_string(), mmap.clone());
-                mmap
+                (mmap, false)
             }
         };
 
         let mut parts = Vec::with_capacity(entry.components.len());
         for component in &entry.components {
             let bytes = CacheBytes::mapped(mmap.clone(), component.offset, component.len)?;
-            // Verify before handing bytes to the runtime. This faults the
-            // pages in, which a restore is about to do anyway.
-            let actual = blake3::hash(bytes.as_cow()?.as_ref()).to_hex().to_string();
-            if actual != component.checksum {
-                self.quarantine(page_id);
-                self.stats.corrupt_entries = self.stats.corrupt_entries.saturating_add(1);
-                return Err(anyhow!(
-                    "KV disk entry failed checksum verification for page {page_id}"
-                ));
+            // Verify once per mapping, before the bytes first reach the
+            // runtime. Entries are published by atomic rename and never
+            // modified in place, so a live mapping stays verified.
+            if !already_verified {
+                let actual = blake3::hash(bytes.as_cow()?.as_ref()).to_hex().to_string();
+                if actual != component.checksum {
+                    self.quarantine(page_id);
+                    self.stats.corrupt_entries = self.stats.corrupt_entries.saturating_add(1);
+                    return Err(anyhow!(
+                        "KV disk entry failed checksum verification for page {page_id}"
+                    ));
+                }
             }
             parts.push(bytes);
         }

Note that corrupted_entry_is_rejected_and_quarantined (Line 652) already clears tier.mappings before reloading, so it keeps passing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/src/disk_tier.rs` around lines 431 - 445, Track page IDs
whose mappings have completed checksum verification alongside self.mappings, and
in load skip per-component hashing for already verified mappings. Mark a page
verified only after every component passes validation; preserve quarantine,
corruption stats, and error behavior on failure, and clear the verification
state whenever the corresponding mapping is removed or reset.

docs/skippy/KV_RETENTION_PLAN.md (1)

264-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale line reference.

as_cow() now lives at payload/bytes.rs:103-137 after this change. The cited range payload/bytes.rs:60-79 points at other code.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/skippy/KV_RETENTION_PLAN.md` around lines 264 - 268, Update the
`CacheBytes::as_cow()` reference in the deduped-blocks discussion to point to
its current location at `payload/bytes.rs:103-137`, leaving the surrounding
explanation unchanged.

crates/skippy-cache/tests/agentic_retention.rs (1)

336-382: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add coverage for the new ResidentKvArchive payload kind.

This test archives with ExactStatePayloadKind::KvRecurrent and two components, while its doc comment describes the dense-archive scenario. The ResidentKvArchive kind added in crates/skippy-cache/src/payload/mod.rs has no test in the reviewed files, so its one-component contract and its from_disk_components reconstruction are unverified.

A test that stores one component under ResidentKvArchive and reads it back would also surface the kind() asymmetry raised on crates/skippy-cache/src/payload/mod.rs: the restored payload reports KvRecurrent, not ResidentKvArchive.

💚 Proposed test

/// A dense archive carries attention KV only. It must round-trip under its
/// own payload kind, and the restored payload must not present a fabricated
/// recurrent component as real state.
#[test]
fn dense_archive_round_trips_under_its_own_payload_kind() {
    let dir = tempfile::tempdir().unwrap();
    let kv_bytes = vec![0x7Eu8; 16 * 1024];

    {
        let disk = PrefixDiskTier::open(dir.path(), 64 << 20).unwrap();
        let mut cache = ExactStateCache::<()>::new(4, 0).with_disk_tier(disk);
        assert!(cache.store_on_disk(
            "dense-a",
            2048,
            ExactStatePayloadKind::ResidentKvArchive,
            &[&kv_bytes],
            (),
        ));
    }

    let disk = PrefixDiskTier::open(dir.path(), 64 << 20).unwrap();
    let mut cache = ExactStateCache::<()>::new(4, 0).with_disk_tier(disk);
    let restored = cache
        .lookup_disk_only("dense-a", ExactStatePayloadKind::ResidentKvArchive)
        .unwrap()
        .expect("dense archive should be importable after a restart");

    assert_eq!(
        restored.payload.kv_bytes().unwrap().unwrap().as_ref(),
        &kv_bytes[..]
    );
    // A dense archive has no recurrent state. Reading one back must not
    // succeed with an empty buffer that a caller could import as real state.
    assert!(restored.payload.recurrent_state_bytes().is_err());
}

The final assertion fails against the current implementation. That is the defect raised on crates/skippy-cache/src/payload/mod.rs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/tests/agentic_retention.rs` around lines 336 - 382, Add a
test alongside archived_kv_page_metadata_survives_a_restart named
dense_archive_round_trips_under_its_own_payload_kind that stores one component
using ExactStatePayloadKind::ResidentKvArchive, restores it with
lookup_disk_only, and verifies the KV bytes round-trip. Assert that the restored
payload reports ResidentKvArchive through kind() and that
recurrent_state_bytes() returns an error, confirming no fabricated recurrent
component is exposed.

crates/skippy-cache/src/miss_reason.rs (1)

155-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for re-eviction of a page that already has a tombstone.

Line 167 removes the stale by_sequence key when tombstones.insert replaces an existing entry. Without that line by_sequence would retain a dangling sequence, and trim_to_capacity would then drop a page id that no longer maps to that sequence, silently shrinking the table below its bound. No current test exercises the replacement path.

💚 Proposed test

/// Re-evicting a page must not leave a dangling insertion-order key, or
/// trimming drops the wrong entry and the table shrinks below its bound.
#[test]
fn re_eviction_replaces_the_tombstone_without_leaking_order_keys() {
    let mut tracker = PrefixMissTracker::new(4);
    tracker.note_evicted("page-a", 64, 0);
    tracker.note_evicted("page-a", 128, 10);

    assert_eq!(tracker.stats().tombstones, 1);
    assert_eq!(tracker.by_sequence.len(), 1);
    // The second eviction wins, so the gap is measured from it.
    assert_eq!(
        tracker.note_miss("page-a", 20),
        PrefixMissReason::EvictedRecently
    );
    assert_eq!(tracker.stats().evicted_miss_tokens, 128);
    assert_eq!(
        tracker.stats().evicted_misses_in(PrefixGapBucket::UnderMinute),
        1
    );
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-cache/src/miss_reason.rs` around lines 155 - 170, Add a unit
test for PrefixMissTracker::note_evicted covering re-eviction of the same page,
asserting the replacement leaves one tombstone and one by_sequence entry, and
that note_miss uses the newer token count and eviction timestamp while reporting
EvictedRecently.

crates/skippy-server/src/kv_integration/mod.rs (1)

21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import ArchiveCandidate from its owning module.

Line 22 adds a new crate-root re-export. Make dense_disk visible within the crate and update callers to import crate::kv_integration::dense_disk::ArchiveCandidate directly. As per coding guidelines, “Minimize crate-root re-exports. New code should import from the owning module directly.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/skippy-server/src/kv_integration/mod.rs` around lines 21 - 22, Remove
the crate-root re-export of ArchiveCandidate in kv_integration and expose the
dense_disk module within the crate as needed. Update all callers to import
ArchiveCandidate directly from crate::kv_integration::dense_disk, preserving
existing behavior while minimizing crate-root re-exports.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/skippy-cache/src/disk_tier.rs`:
- Around line 123-129: Move the size-bounded, mmap-backed store documentation
from the doc comment immediately before DirectoryLock to PrefixDiskTier, leaving
DirectoryLock documented only with its exclusive advisory lock behavior. Ensure
PrefixDiskTier has the complete tier description directly before its
declaration.
- Around line 132-151: Update DirectoryLock::acquire to provide mutual exclusion
on non-Unix targets before PrefixDiskTier::open can activate the tier: use a
platform-specific exclusive lock such as Windows LockFileEx or
exclusive-share-mode file access, and return an error when ownership cannot be
acquired. If cross-platform locking is not supported, gate the disk tier so it
declines on those targets instead of returning Ok after merely creating
owner.lock.

In `@crates/skippy-cache/src/exact_state.rs`:
- Around line 384-404: Update demote_to_disk to persist entry.extra with the
disk entry so KvRecurrent pages remain importable after restart, propagating the
necessary serialization bounds through remove_entry, evict_until_within_limits,
and record. Update lookup_with_disk to prefer persisted metadata from
disk_extras or load.extra before invoking the fallback closure, and make
store_on_disk treat metadata serialization failure as an unsuccessful archive
rather than storing None. Add a restart round-trip test for a demoted
KvRecurrent entry and its descriptor, alongside
archived_kv_page_metadata_survives_a_restart.
- Around line 114-171: Update lookup_with_disk so the initial RAM probe does not
classify a miss immediately; preserve the RAM hit behavior, but defer miss
classification until after the disk tier is consulted. Record a miss only when
both RAM and disk fail, and record a hit for a disk-served result without also
incrementing miss statistics. Leave lookup unchanged so
miss_reasons_distinguish_eviction_from_never_seen retains its existing behavior.

In `@crates/skippy-cache/src/identity.rs`:
- Around line 98-127: Update the disk-tier setup around open_disk_tier and
update_weight_identity so every disk-backed weight identity includes a resolved
content digest. Prefer gating disk-tier use when neither source_model_sha256 nor
manifest_sha256 is available; otherwise add a reliable fallback based on the
resolved model path’s file size and modification time, ensuring byte changes
cannot reuse the same alias while preserving existing digest-based identities.

In `@crates/skippy-cache/src/payload/mod.rs`:
- Around line 61-64: Correct the doc comment for disk_components to state that
it returns an error when a component cannot be borrowed contiguously, replacing
the inaccurate reference to None while preserving the documented component
ordering.
- Around line 88-116: Add a distinct ExactStatePayload::ResidentKvArchive
variant and update from_disk_components to construct it directly from the KV
component. Update recurrent_state_bytes(), is_mapped(), and serialization/kind
selection for this variant so it has no recurrent state, reports mapping based
on its KV bytes, and is persisted as ResidentKvArchive rather than KvRecurrent.

In `@crates/skippy-server/src/binary_transport/forwarding.rs`:
- Around line 381-397: Update first_stage_may_emit_a_short_activation_frame so
incoming.token_count is greater than 1 while preserving the one-token output
frame, ensuring the test exercises the first-stage short-activation exception.

In `@crates/skippy-server/src/frontend/prefix_cache.rs`:
- Around line 540-547: Update the archive selection around the
identities/records iteration to construct an ArchiveCandidate from only
successfully stored resident records, rather than unconditionally using
identities.last(). After the loop, archive the selected candidate while
preserving the existing runtime lock and error handling.

In `@docs/skippy/KV_RETENTION_PLAN.md`:
- Around line 86-89: Update docs/skippy/KV_RETENTION_PLAN.md at lines 86-89 and
the W7 bullet at line 343 to remove claims that topology_id is part of the
prefix hash or page identity, while retaining the layer-range invalidation
point. In crates/skippy-cache/src/disk_tier.rs lines 27-35, revise the “Safety
of reuse across restarts” module documentation to remove topology from the
identity components.
- Around line 468-471: Update the finding in the retention plan so it documents
the final ArchiveCandidate heuristic: select the longest shareable prefix that
excludes the request’s own tail. Retain the lowest-candidate approach only as an
intermediate attempt that was later identified as incorrect, and align the
wording with the split-serving section.

---

Outside diff comments:
In `@crates/skippy-server/src/binary_transport/binary_kv.rs`:
- Around line 726-757: Move the archive_candidate handling block out of the
config.downstream/output conditional so it runs for terminal stages as well.
Keep record_resident_activation and add_binary_activation_records conditional,
but always call archive_dense_prefix when archive_candidate.take() yields a
candidate and preserve the existing archived-token attribute update.

---

Nitpick comments:
In `@crates/skippy-cache/Cargo.toml`:
- Around line 18-24: The dependency declarations in skippy-cache should use
workspace versions: add libc and ensure tempfile is available in workspace
dependencies, replace their local version pins with workspace references, and
move libc under target cfg(unix) dependencies to match its cfg-gated use in
disk_tier.rs. Keep tempfile as a workspace dev-dependency.

In `@crates/skippy-cache/src/config.rs`:
- Around line 213-275: Apply max_resident_tokens_hint to the mandatory near-tail
selection in the ladder-building logic: retain the exact-length slot, but only
add near_tail when the combined pinned length stays within the budget, dropping
it when it alone would exceed the hint. Keep the existing unconstrained behavior
when the hint is zero and preserve later candidate checks.

In `@crates/skippy-cache/src/disk_tier.rs`:
- Around line 431-445: Track page IDs whose mappings have completed checksum
verification alongside self.mappings, and in load skip per-component hashing for
already verified mappings. Mark a page verified only after every component
passes validation; preserve quarantine, corruption stats, and error behavior on
failure, and clear the verification state whenever the corresponding mapping is
removed or reset.

In `@crates/skippy-cache/src/identity.rs`:
- Around line 73-76: Update the selected-device branch in the identity hashing
logic to hash StageDevice::stable_id instead of backend_device, preserving the
no-selected-device marker for None and using a distinct fallback marker when
stable_id is absent. Ensure device identities remain stable across
re-enumeration.

In `@crates/skippy-cache/src/lib.rs`:
- Line 10: Remove the crate-root re-exports for disk-tier and miss-reason types
from lib.rs, then update skippy-server call sites to import PrefixDiskTier,
DiskLoad, DiskTierStats, and PrefixMissReason through skippy_cache::disk_tier
and skippy_cache::miss_reason. Preserve existing usage while eliminating the
duplicate public paths.

In `@crates/skippy-cache/src/miss_reason.rs`:
- Around line 155-170: Add a unit test for PrefixMissTracker::note_evicted
covering re-eviction of the same page, asserting the replacement leaves one
tombstone and one by_sequence entry, and that note_miss uses the newer token
count and eviction timestamp while reporting EvictedRecently.

In `@crates/skippy-cache/tests/agentic_retention.rs`:
- Around line 336-382: Add a test alongside
archived_kv_page_metadata_survives_a_restart named
dense_archive_round_trips_under_its_own_payload_kind that stores one component
using ExactStatePayloadKind::ResidentKvArchive, restores it with
lookup_disk_only, and verifies the KV bytes round-trip. Assert that the restored
payload reports ResidentKvArchive through kind() and that
recurrent_state_bytes() returns an error, confirming no fabricated recurrent
component is exposed.

In `@crates/skippy-server/src/kv_integration/mod.rs`:
- Around line 21-22: Remove the crate-root re-export of ArchiveCandidate in
kv_integration and expose the dense_disk module within the crate as needed.
Update all callers to import ArchiveCandidate directly from
crate::kv_integration::dense_disk, preserving existing behavior while minimizing
crate-root re-exports.

In `@docs/skippy/KV_RETENTION_PLAN.md`:
- Around line 264-268: Update the `CacheBytes::as_cow()` reference in the
deduped-blocks discussion to point to its current location at
`payload/bytes.rs:103-137`, leaving the surrounding explanation unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d41d3c2d-c5ab-4c24-a6b5-aaf5bcd1e670

📥 Commits

Reviewing files that changed from the base of the PR and between f4d530c and c104b99.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (24)

• Cargo.toml
• crates/mesh-llm-host-runtime/src/inference/skippy/family_policy.rs
• crates/skippy-cache/Cargo.toml
• crates/skippy-cache/src/config.rs
• crates/skippy-cache/src/disk_tier.rs
• crates/skippy-cache/src/exact_state.rs
• crates/skippy-cache/src/identity.rs
• crates/skippy-cache/src/lib.rs
• crates/skippy-cache/src/miss_reason.rs
• crates/skippy-cache/src/payload/blob_store.rs
• crates/skippy-cache/src/payload/bytes.rs
• crates/skippy-cache/src/payload/mod.rs
• crates/skippy-cache/tests/agentic_retention.rs
• crates/skippy-runtime/src/types.rs
• crates/skippy-server/src/binary_transport/binary_kv.rs
• crates/skippy-server/src/binary_transport/forwarding.rs
• crates/skippy-server/src/frontend/local_generation/token_generation.rs
• crates/skippy-server/src/frontend/prefix_cache.rs
• crates/skippy-server/src/kv_integration/config.rs
• crates/skippy-server/src/kv_integration/dense_disk.rs
• crates/skippy-server/src/kv_integration/exact_state.rs
• crates/skippy-server/src/kv_integration/identity.rs
• crates/skippy-server/src/kv_integration/mod.rs
• docs/skippy/KV_RETENTION_PLAN.md

[michaelneale]

Status after two independent expert reviews

Ran two reviews in parallel: one on split/staged correctness, one on the disk tier and cache identity.

Split correctness — clean

All five questions came back safe:

• The new non-first-stage assertion cannot false-positive on MTP/speculative decode, chunked prefill, or batched lanes.
• A disk hit keeps the cross-stage veto sound (agreement depends on hit/miss, not provenance).
• resident_seq_id: None on the disk path is benign — telemetry-only, and the pre-existing exact path already does it.
• A partial disk restore cannot propagate downstream: the clamp gives <= len, the hit gate requires >= len, so only a full restore reports a hit.
• Token-range mismatch is rejected before import, not silently truncated.

Disk tier — five real issues, all fixed

Finding	Fix
Metadata not checksummed. Only the payload was. extra carries the KV descriptor (layer range, ggml types, row strides), so a corrupted-but-valid-JSON index could apply correct bytes under a wrong layout — silent corruption on a path reporting success.	extra_checksum per entry, verified before the payload. Format v3. Test included.
Unanchored weights could alias. Two different GGUFs under one model_id collide on disk across a restart.	Tier refuses to open without a content digest (manifest_sha256/source_model_sha256/package_ref), and logs why.
No lock on non-Unix, running silently unprotected.	Fails closed with an explicit error.
Crash-left .tmp files never reclaimed — a page's bytes leaked per crash, unbounded.	Reclaimed at open, safe under the exclusive lock. Test included.
Debug eprintln! left in committed code.	Removed — my mistake, caught on re-read.

Two review claims I checked and rejected: page_id is a full BLAKE3 digest (not truncated to 64 bits), and the index already commits atomically via write-temp-then-rename. Both were artifacts of the reviewer seeing an excerpt rather than the file.

Re-measured after hardening (solo 0.5B, ~2.1k tokens)

	Cold	Warm
Cross-session, new tail	0.42s	0.13s
First request after restart	0.42s	0.21s

Two-node split smoke passes. Full suite green: skippy-cache 93, skippy-server 395, skippy-runtime 69, host-runtime 1932.

Confidence

• High on default-path safety — tier is opt-in, off by default; the only always-on change is the identity hash.
• High on solo retention — measured repeatedly, including 2.44× on an 8B package.
• Medium on split retention — cross-session works and is measured; restart still shows no win because the downstream stage archives a shallower ladder than stage 0, and the all-or-nothing veto means one shallow archive negates the other. Known, documented, unfixed.
• Untested: large MoE across real multi-machine splits; Windows (now explicitly refuses rather than running unlocked).

@i386 — the split gap above is the main thing I'd value a second opinion on.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/skippy-server/src/kv_integration/config.rs`:
- Around line 86-108: Update the disk-tier eligibility check around the
manifest_sha256, source_model_sha256, and package_ref fields to require at least
one non-empty, valid SHA-256 digest in manifest_sha256 or source_model_sha256.
Do not treat package_ref alone as sufficient, and retain the existing
disabled-tier message and return behavior when no validated digest is available.

In `@docs/skippy/KV_RETENTION_PLAN.md`:
- Around line 591-593: Update the disk-retention enablement gate to require a
validated immutable content digest, such as manifest_sha256 or
source_model_sha256, before opening the tier. Do not accept package_ref by
presence alone; if it is the only identity, derive its content digest and
validate it before allowing retention, otherwise fail closed with the existing
explanatory error path.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c3a4266e-01b0-4c5c-ae8f-defc59877c26

📥 Commits

Reviewing files that changed from the base of the PR and between c104b99 and 96564de.

📒 Files selected for processing (5)

• crates/skippy-cache/src/disk_tier.rs
• crates/skippy-server/src/binary_transport/binary_kv.rs
• crates/skippy-server/src/kv_integration/config.rs
• crates/skippy-server/src/kv_integration/dense_disk.rs
• docs/skippy/KV_RETENTION_PLAN.md

💤 Files with no reviewable changes (1)

• crates/skippy-server/src/binary_transport/binary_kv.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/skippy-server/src/kv_integration/dense_disk.rs
• crates/skippy-cache/src/disk_tier.rs

[i386]

Review findings:

1. P1 — Stage-0 full-prefill archives the smallest candidate
   crates/skippy-server/src/frontend/prefix_cache.rs:540-546 uses identities.last(). Identities are longest-first, so this archives the least useful page—or nothing when it falls below the 512-token floor. Use ArchiveCandidate here too.

2. P1 — Recurrent KV demotions lose their descriptor
   crates/skippy-cache/src/exact_state.rs:396-404 stores demoted entries with extra=None. KvRecurrent pages need kv_desc; after eviction/restart they cannot restore the KV portion and are silently treated as misses.

3. P1 — Disk tier can enable with an unsafe identity
   crates/skippy-server/src/kv_integration/config.rs:97-100 accepts any package_ref, including mutable paths, or invalid/empty digest strings. Require a validated 64-hex manifest_sha256 or source_model_sha256; package_ref alone is not immutable content identity.

4. P1 — Every disk hit hashes the entire mapped payload
   crates/skippy-cache/src/disk_tier.rs:493-499 re-runs BLAKE3 for every component on every promotion. For the documented ~0.96 GB page, this happens on every hit while cache/runtime locks are held. Cache verified mapping IDs and invalidate them when mappings are removed.

5. P2 — Disk hits are counted as both misses and hits
   crates/skippy-cache/src/exact_state.rs:150-162 calls lookup(), recording a miss before consulting disk, then records a hit. Retention telemetry and recoverable-miss ratios are inflated.

The SHA-256 concern is not new to this PR: source-model SHA-256 computation and its sidecar cache already exist on the base. The new hot-path concern is the repeated full-page BLAKE3 verification.

Local validation passed: skippy-cache 93 unit + 7 integration tests, and 19 targeted skippy-server tests.

[i386]

@michaelneale thanks for the follow-up. My read of the current state:

• Weight identity: the identity fields were added, but the disk-tier gate is not fully fail-closed yet. A package_ref alone, or an invalid/empty digest, can still enable retention. This remains a P1.
• Archive-at-record cost: the new measurements justify keeping it, but it is still unconditional rather than gated by observed hit-rate telemetry. That is an explicit trade-off, not fully addressed in code.
• Split/restart behavior: cross-session split reuse works, but restart reuse still has the documented gap where a shallow downstream archive causes the all-or-nothing restore veto.
• Disk hardening: metadata checksums, non-Unix locking, temporary-file reclaim, and the debug output appear addressed. The immutable content-digest gate remains incomplete.

I also found three additional issues in the current head: stage-0 full-prefill can still select the smallest candidate, demoted recurrent KV entries lose their descriptor, and every disk hit re-hashes the full mapped payload.

Could you confirm the intended disposition for these remaining items—especially the digest gate, stage-0 candidate selection, and split restart gap—before merge?

[micspiral]

Your bandwidth point is the biggest thing in this issue — and I can now show why

I chased all three. The second one changes how I'd frame the whole feature.

1. Prefix commonality — agreed, and it's a retention-time argument

A fleet on one agent sends near-identical system prompt + tool schemas forever. High hit frequency, full-cold-prefill value per hit. The goal you describe — never warm up again — is exactly the disk tier's job: survive eviction, restart, and redeploy. Disk "can't hurt" is right, and it's cheap per node on splits because per-stage pages are smaller.

2. Prefill bandwidth — this is the finding

You're right that it's ~3 GB between stages, and it turns out a chain restore removes essentially all of it. embedded_prefix_cache_message (frontend/wire_messages.rs:264-288) builds the restore message with activation: Vec::new() — it carries only token IDs. So the restored span costs 4 bytes/token instead of width x dtype:

width	dtype	tokens	cold	warm	ratio
4096	f32	128k	2.10 GB	0.5 MB	4096x
4096	f16	128k	1.05 GB	0.5 MB	2048x
8192	f16	128k	2.10 GB	0.5 MB	4096x

Per boundary, per request. That reframes things: prefill compute parallelizes across stages, boundary transfer does not. On a thin or shared link the bandwidth saving may dominate the compute saving — and it is completely invisible in the single-node wall-clock numbers I've been quoting. My loopback benchmarks are the worst possible place to observe your most valuable effect.

Corollary: the all-or-nothing veto is more expensive than I credited. One stage missing doesn't just lose that stage's compute — it forces full activation traffic across every boundary for the whole prompt. That raises the priority of the stage-1 gap below.

3. Pre-seeding — a consequence of work already done, not new machinery

The tier is already a content-addressed, checksummed, identity-anchored page store, so seeding is a write into it. What makes it plausible now is that identity no longer contains topology_id or any per-process value, so a page is valid for any process with matching weights/layer range/KV dtype/backend. What still blocks it: layer range and stage id are hashed, so a seed is only valid for an identical split — seeding has to happen after topology is chosen, or be keyed by topology and matched at plan time. Worth deciding deliberately, since re-splitting invalidates everything.

Root cause of the stage-1 gap

Found it while looking at this. maybe_record_binary_prefill is called with min_record_tokens = restored_tokens (prefill_recording.rs:67-81), and the recorder skips every candidate <= min_record_tokens (binary_kv.rs:632). On a warm chain restore the downstream stage therefore declines to record exactly the shared-bulk candidates it just served — its ladder can only ever get shallower. That's why stage 0 archives 2048 and stage 1 archives 512.

Not fixed in this PR — it's a behavioural change to the record path and I'd rather land it separately with its own evidence.

Documented in docs/skippy/KV_RETENTION_PLAN.md. @i386 the bandwidth table is the part I'd most like a sanity check on.

[micspiral]

Yes — and it already works, no new machinery needed. Measured.

Your instinct was right. Sending the canonical prefix through the node once as an ordinary request populates the disk tier, because recording happens on the normal serving path.

Solo: measured

Seed run = the system prompt with no user turn. Then restart the process (cold RAM, warm disk) and send a request with a tail the seed never saw:

First real request on a fresh process
Unseeded (empty disk)	0.41s
Seeded (one prior prefix-only run)	0.21s

The seed run archived a single 2048-token page — the shared bulk — and an unrelated later session hit it. That's the "never warm up again" property you described: the node is useful on its first ever real request.

Why no new code was needed: identity contains no session_id and no per-process value, so a page recorded by a warmup is valid for any later session or process with matching weights, layer range, KV dtype, backend. And ArchiveCandidate prefers the longest partial candidate, which happens to be exactly right for seeding — a seed prompt's full length is the shared prefix.

Durability is the part that makes it worth doing: the seed survives eviction and restart, so you pay the warmup once per node, not once per process.

Splits: no gain yet, and I found the exact cause

Same experiment on 2 nodes: 1.27s seeded vs ~1.30s cold. Nothing. Per-stage archives show why:

stage 0 : [2048, 1920, 1792, ... 512]   <- full ladder
stage 1 : [512]                          <- floor only

512 is exactly MIN_ARCHIVE_TOKENS — stage 1 is archiving the smallest page it's permitted to and nothing more. With the all-or-nothing veto, that cancels stage 0's good page.

Root cause: record_completed_prefill passes min_record_tokens = restored_tokens (prefill_recording.rs:67-81) and the recorder skips every candidate <= min_record_tokens (binary_kv.rs:632). So on a warm chain restore the downstream stage declines to record precisely the shared-bulk candidates it just served. The ladder ratchets shallower with every warm request.

That gate is correct for its original job — don't re-record what you just restored into the resident cache — and wrong for the archive, which wants the shared bulk specifically.

I've deliberately not fixed it here. It's a behaviour change on the record path, it needs per-stage archive depth as its evidence, and this PR is already large. It's the clear next piece of work, and it unblocks both split retention and split seeding — which, given the 2048–4096× bandwidth effect, is where the real payoff is.

So: seeding is real and works today, solo only.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/skippy/KV_RETENTION_PLAN.md`:
- Around line 696-699: Update the fenced code block containing the stage 0 and
stage 1 ladder output to specify the text language identifier (`text`) on its
opening fence, preserving the block contents unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b7c1295-3183-44a3-b06b-5245349a5561

📥 Commits

Reviewing files that changed from the base of the PR and between 80f49f8 and de08c50.

📒 Files selected for processing (1)

• docs/skippy/KV_RETENTION_PLAN.md

[i386]

Re-review of 42a9eb4001ec27095f5faac1753ef21de074c189:

• All 11 CodeRabbit findings from the prior review are addressed and their threads are resolved, including the outdated threads.
• Disk-hit accounting now records one outcome per lookup, so recoverable-miss metrics no longer count a disk hit as a RAM miss.
• Resident dense-KV archives now retain their own payload kind and cannot be misread as empty recurrent state.
• Stage-0 archiving selects the longest successfully resident shareable candidate, and content-addressed disk retention now requires a valid 64-hex SHA-256 digest.
• Documentation and the first-stage forwarding regression test are updated.

Validation on the pushed head: cargo fmt --all --check, cargo check -p skippy-cache, cargo check -p skippy-server, Clippy for both touched crates with warnings denied, 98 cache unit tests, 7 cache retention integration tests, 412 server unit tests, and just build all pass. No new actionable findings in this re-review. GitHub’s platform/runtime and Clippy jobs are still pending because this PR remains draft.

[i386]

@micspiral could you think more carefully about the disk-cache lifecycle/design here?

I traced the current behavior:

• RAM pressure demotes exact-state entries to disk; dense resident-KV entries are proactively archived at record time.
• Disk eviction is only triggered after a write, or during tier startup, and removes disk LRU entries until the byte budget is satisfied.
• There is no TTL, periodic maintenance, or age-based cleanup. A disk hit refreshes LRU metadata but does not promote the entry back into the RAM cache.
• On restart, the index is reloaded, orphan/temp files are removed, the budget is reapplied, and payload checksums are verified lazily on first use.

This is bounded and internally consistent, but it raises a design question: should retention have an explicit age/TTL or maintenance policy, and should archive admission account more directly for write cost, restore cost, and model churn? In particular, if the workload stops writing, old entries remain indefinitely within the byte budget. Would appreciate your view on whether this lifecycle is the right policy before we consider it settled.

[i386]

@micspiral on the binary packing: the current layout is efficient for a same-platform local cache. Payload components are concatenated into one contiguous .kvp file with no per-component padding, and restores use mmap without copying. The JSON index carries only metadata, offsets, lengths, and BLAKE3 checksums; page checksums use BLAKE3 rather than SHA-256.

The index integers are endian-neutral because they are JSON text. The .kvp payload is opaque runtime KV bytes and does not currently contain a packed integer header, so there is no immediate little-/big-endian header mismatch. However, the payload format implicitly assumes a compatible runtime/platform; endianness is not explicitly recorded or validated, so big-endian portability is not guaranteed.

As part of a versioned page schema, I suggest an explicit magic/header with schema version and byte order—probably enforce little-endian and reject unsupported byte order—plus runtime/architecture payload identifiers.

[i386]

The page payload is already binary; only the metadata index is JSON.

A binary index could reduce startup parsing and repeated full-index serialization, but it would not speed normal cache hits: the index is parsed only when the tier opens, while lookups use in-memory metadata and mmap the payload directly.

The likely improvement is a versioned index.bin with:

• magic and schema version
• explicit little-endian encoding
• entry count and length-prefixed fields
• payload kind, offsets, lengths, checksums, and metadata
• CRC or equivalent index integrity check

I’d keep a JSON diagnostic/export tool, but switch the runtime index to binary if profiling shows startup or index rewrites matter. The page .kvp files do not need another serialization layer.

[i386]

@micspiral I agree with the direction here.

For the current design, the .kvp page data is already raw binary and mmap-friendly; the JSON index is metadata (entries, offsets, lengths, checksums, and eviction timestamps). JSON parsing is primarily a startup/index-commit cost, so I’d keep it for now and only move to a binary index if profiling shows that cost matters. Any binary-index follow-up should define a magic header, schema version, explicit byte order, and compatibility behavior.

We should add a documented file-format spec in Markdown and on the website covering the directory layout, index/page schemas, lifecycle and eviction, atomic writes, checksums, versioning, endian assumptions, and corruption/compatibility behavior. Agent guidance should require the implementation, tests, canonical Markdown spec, and website copy to stay synchronized whenever the format changes.

One important release detail: please reset the on-disk DISK_TIER_FORMAT_VERSION to v1 when this PR lands.

[michaelneale]

note disk tier is not off by default I believe