Repository navigation
feat(server): persist agent sessions on SSD with a disk budget and TTL - #839
Conversation
… validated restores Keep each conversation's newest snapshot and shared heads on disk when RAM evicts them, and flush resident sessions on graceful shutdown. Enable the cache by default with a hard LRU budget, a TTL, bounded asynchronous writes, checksums, exact token keys and model/config identity validation. Restore deeper disk states ahead of shallow RAM hits, load paged snapshots without blocking decoding, and replay tool-call text after restart. Validate Qwen snapshot layout and every disk tensor's type, alignment, dimensions and payload bounds before adoption or allocation; corrupt files become misses. Includes persistence, restart, eviction, corruption and identity regression tests. Compressed prompts do not receive raw disk keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
53b8c22 to
8dcdee6
Compare
davide221
left a comment
There was a problem hiding this comment.
Code review
Found 8 issues.
Copy to agent
Use the copy button in the top-right corner of the block, then paste into your agent.
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
Repository: Luce-Org/lucebox
Reviewed commit: 8dcdee6461ec8fb6ea60c744d0aee54ada2d7f56
Verify the findings against the current code, run relevant checks for any fixes, and explain any findings you reject.
<file name="server/src/server/disk_prefix_cache.cpp">
<violation number="1" line="616" end_line="629">
Exclusive one-server-per-directory lock is POSIX-only; on Windows two servers share the cache and delete each other's fi
The PR's stated safety contract is "One server per directory, enforced by an exclusive lock." The lock block in `init()` is compiled only under `#if !defined(_WIN32)` and there is no `#else` fallback: on Windows, `init()` proceeds with no lock at all and no warning. The rest of the file is explicitly written to support Windows (`_fseeki64` in `seek_to`, `_commit` in `sync_file`, the `(void)dir` branch in `sync_directory`), so a Windows build is a supported configuration.
Failure sequence: start two luce_server instances on Windows with the same `--kv-cache-dir`. The second instance's `scan_all()` (lines 749-757) deletes every `*.dkv.tmp` it finds — including the temporary file the first server's writer thread is actively writing (writes go to `<file>.tmp` and are renamed only after `fdatasync`). The first server's rename then fails, so its evicted-session snapshot is lost (logged as a write failure). Both instances also independently run `make_room_locked` LRU eviction and in-place `last_used` rewrites over the same files, so each deletes files the other still has indexed.
Fix: add a Windows equivalent of the exclusive lock (e.g. `CreateFile` on `<dir>/.lock` with `dwShareMode = 0`, or `LockFileEx(..., LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, ...)`), with the same fail-off-and-log behavior as the POSIX path. Fact I could not check: whether the server is officially shipped for Windows; if it is not, the Windows-specific code paths elsewhere in this file suggest it is, and the lock should match.
<suggestion>
#if !defined(_WIN32)
const std::string lock_path = config_.cache_dir + "/.lock";
lock_fd_ = ::open(lock_path.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0644);
if (lock_fd_ < 0 || ::flock(lock_fd_, LOCK_EX | LOCK_NB) != 0) {
std::fprintf(stderr,
"[disk-cache] disabled: %s is in use by another server "
"(pass --kv-cache-dir <other dir> to give this one its own)\n",
config_.cache_dir.c_str());
if (lock_fd_ >= 0) ::close(lock_fd_);
lock_fd_ = -1;
config_.cache_dir.clear();
return true;
}
#else
// Windows: exclusive hold on .lock via a share-mode-0 handle.
const std::string lock_path = config_.cache_dir + "/.lock";
lock_handle_ = CreateFileA(lock_path.c_str(), GENERIC_READ | GENERIC_WRITE,
0, nullptr, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
if (lock_handle_ == INVALID_HANDLE_VALUE) {
std::fprintf(stderr,
"[disk-cache] disabled: %s is in use by another server "
"(pass --kv-cache-dir <other dir> to give this one its own)\n",
config_.cache_dir.c_str());
config_.cache_dir.clear();
return true;
}
#endif
</suggestion>
</violation>
<violation number="2" line="1153" end_line="1158">
Duplicate-save touch updates last_used only in memory, so TTL/LRU after a restart ignore recent use
`save_async()`'s contract (like `load()`'s) is that using an entry refreshes its `last_used`, and the header comment at the `load()` touch site states the file field is rewritten in place precisely so that "LRU order and the TTL survive a restart." When a snapshot is saved but its key is already on disk (line 1154 finds the entry), the code updates `entries_[existing].last_used` in memory only and never rewrites `DISK_CACHE_LAST_USED_OFFSET` in the file. The same pattern exists in `save()` (lines 1213-1217). Concretely: a shared system/tools head or a session snapshot that is written once and then repeatedly re-saved (every later eviction of the same prefix hits the duplicate branch — including the graceful-shutdown flush, which also goes through `save_async`) keeps its original on-disk `last_used` forever. After a restart, `scan_all()` restores that stale timestamp, and `expire_locked()` at startup (line 633) or `make_room_locked()` LRU eviction will delete the file as "unused for ttl_seconds" (default 72 h) even though it was actively used right up to the shutdown. Fix: mirror the `load()` touch path and persist the new `last_used` to the file when the entry is not pending.
<suggestion>
const int existing = find_entry(hash);
if (existing >= 0) {
DiskEntry & e = entries_[(size_t)existing];
e.last_used = now();
if (!e.pending) {
// Persist like load() does, so LRU order and the TTL survive a restart.
if (FILE * f = std::fopen(e.path.c_str(), "r+b")) {
if (std::fseek(f, DISK_CACHE_LAST_USED_OFFSET, SEEK_SET) == 0) {
write_u64(f, e.last_used);
}
std::fclose(f);
}
}
return true;
}
</suggestion>
</violation>
</file>
<file name="server/src/server/prefix_cache.cpp">
<violation number="1" line="991" end_line="993">
Reclaimed (superseded) bytes are double-counted by concurrent reservations
The new `credited_slots` mechanism exists so that "two captures never count the same bytes": slots an in-flight reservation already counts as freed are pushed into `credited_slots` (here and at lines 998-999) and `plan_several` skips them via `slot_credited()` (line 937-938). But the `reclaimed` computation above (lines 830-837), which credits this reservation with the bytes of the entries it supersedes, only skips `slot_reserved(...)` — it never checks `slot_credited(...)`. Sequence that breaks the invariant: reservation A on one conversation thread computes `reclaimed` containing ancestor entry X and pushes X's slot into its `credited_slots`; reservation B (a sibling/deeper prompt sharing ancestor X, two concurrent requests) then computes its own `reclaimed` including X again, because X is not `slot_reserved`. Both reservations are admitted on the assumption that X's bytes are freed twice. The budget check `fits()`/`room()` therefore admits captures whose real post-commit resident bytes exceed `max_resident_bytes_`; the overshoot is only corrected later by `enforce_resident_budget()` evicting entries that were never planned (possibly including a restore source or the shared head that the plan was careful to keep). Fix: in the `reclaimed` loop, also skip entries whose slot is `slot_credited(entries_[i].slot)` (and conversely skip already-reclaimed slots), so a superseded ancestor is credited to exactly one in-flight reservation.
<suggestion>
for (size_t i = 0; i < reclaimed.size(); ++i) {
// Skip slots another in-flight reservation already credits: the
// reclaimed loop above only filters slot_reserved, so without
// this two concurrent captures can count the same ancestor's
// bytes as freed.
if (reclaimed[i] && !slot_credited(entries_[i].slot))
credited_slots.push_back(entries_[i].slot);
}
</suggestion>
</violation>
</file>
<file name="server/src/server/http_server.cpp">
<violation number="1" line="4668" end_line="4670">
Agent-turn snapshots get a disk key even when the request's disk scope is not full
`save_generated_turn()` unconditionally records `slot_tokens_[slot]` and calls `note_session_slot(slot)` for the saved generated turn. `slot_tokens_` is the disk key: `persist_evicted_slot()` (line 4359) writes any slot that has a key to disk on RAM eviction or shutdown, and its own comment says a missing key means "a compressed view, a non-`full` disk scope". The inline-snapshot path in `finalize_generation_cache()` (lines 4293-4299) gates exactly this keying on `!prepared.compressed && cache.disk_policy.mode == DiskPrefixCacheMode::Full`, but the agent-turn path here (and the identical one in `remember_agent_turn()`, lines 4816-4818, which also has `cache` in scope via the caller) skips that check. A client that sends a request with disk-cache scope `off`/`auto`/`fixed` (parsed into `req.disk_cache_policy`) while the server-level disk cache is enabled will still have its generated agent turn written to the on-disk KV cache when the snapshot is evicted or the server shuts down — persisting a conversation the request explicitly scoped out of the disk cache. Gate the `slot_tokens_` assignment and `note_session_slot()` on `cache.disk_policy.mode == DiskPrefixCacheMode::Full` (and `!disk_cache_.disabled()`) the same way `finalize_generation_cache()` does.
<suggestion>
if (cache.disk_policy.mode == DiskPrefixCacheMode::Full) {
slot_tokens_[slot] = std::vector<int32_t>(
canonical.begin(), canonical.begin() + saved_pos);
note_session_slot(slot);
}
</suggestion>
</violation>
</file>
<file name="server/test/unit/test_server_unit.cpp">
<violation number="1" line="10106" end_line="10106">
Unguarded ::geteuid() breaks the Windows build of the test file
The new `test_disk_cache_write_failure_is_not_fatal` test is compiled in every configuration (it sits in the unguarded region lines 9757-10696), yet line 10106 calls `::geteuid()`, a POSIX-only function declared in `<unistd.h>`. This same diff shows the project treats Windows as a supported build for this test file: the paged-scheduler tests directly below (lines 10698-10982) are wrapped in `#if !defined(_WIN32)` precisely because they use `socketpair`/`AF_UNIX`/`close`. On an MSVC build `::geteuid` is not declared, so the unit-test target fails to compile. Even where it compiles, the root check is meant to skip the permission-based assertions when the write can't fail; the surrounding `fs::permissions(owner_read|owner_exec)` trick also has different semantics on Windows (it maps to the read-only attribute), so the whole block should be platform-guarded. What I could not check: whether the test file (or a test support header) defines a `geteuid` compatibility shim for `_WIN32`; if it does, the compile concern is void but the permission-based assertions would still silently not exercise the failure path on Windows.
<suggestion>
#if !defined(_WIN32)
if (::geteuid() != 0) {
TEST_ASSERT(cache.stats().write_failures == 1);
TEST_ASSERT(cache.entry_count() == 1);
TEST_ASSERT(dkv_files(dir).size() == 1);
TEST_ASSERT(!cache.lookup(disk_test_key(40, 800), 1));
}
#endif
</suggestion>
</violation>
</file>
<file name="docs/specs/props-endpoint.md">
<violation number="1" line="390" end_line="391">
New full_cache disk fields (disk_enabled, disk_budget_mb, disk_ttl_hours) are not documented in the /props spec
This PR adds `disk_enabled`, `disk_budget_mb` and `disk_ttl_hours` to the `full_cache` object reported by `/props` (per the PR description and the http_server.cpp wiring reviewed in part 4), and §4.7 of this spec is the normative description of `full_cache` semantics — it already documents `capacity`, `max_resident_bytes`, `resident_bytes` and `budget_skips` in the paragraph this diff edits. The diff only updates the `budget_skips` wording and never adds the three new fields, so the spec now under-documents the endpoint: a client written against docs/specs/props-endpoint.md has no way to know these keys exist, what units they use (MB vs bytes — note `disk_budget_mb` vs `max_resident_bytes`), or what values mean the disk cache is disabled (`disk_enabled` false vs a `0` budget, which this same paragraph defines as 'disabled'/`unlimited' for other fields). Add a sentence to §4.7 documenting the three fields, their units and their disabled-state values. I could not verify the exact wire names/units emitted by http_server.cpp beyond the PR description, so the fix should be checked against that code.
</violation>
</file>
<file name="server/src/server/scheduler.cpp">
<violation number="1" line="139" end_line="145">
Dropped disk loads leak unjoined reader threads until server shutdown
`drop_disk_load()` moves a still-joinable reader `std::thread` into `finished_readers`, and that vector is only joined in the scheduler teardown path (lines 1334-1336). A joinable thread that has already exited keeps its stack and thread-control resources allocated until it is joined or detached (pthreads semantics), so every dropped load leaks on the order of a full thread stack (typically 8 MB) for the remaining process lifetime. Drops happen in normal use: line 516 drops a pending load whenever a different job is admitted, and a retry whose probe no longer qualifies (a deeper RAM entry was committed meanwhile, or the disk entry expired/was evicted) drops it too. On a long-running server with the disk cache on by default, repeated drops accumulate leaked stacks unboundedly. Fix: reap finished readers periodically in the scheduler loop (join and erase any whose `result->done` is set), rather than only at shutdown — joining inside `drop_disk_load()` itself is not an option because the reader may still be blocked in `disk_cache_.load()`, and detaching would let a reader outlive `disk_cache_` at shutdown.
</violation>
</file>
<file name="server/src/server/server_main.cpp">
<violation number="1" line="429" end_line="435">
Default disk-cache directory is empty on Windows, silently disabling the on-by-default cache
The PR makes the disk session cache on by default via `default_kv_cache_dir()`, which only consults `XDG_CACHE_HOME` and `HOME` and returns an empty string otherwise, and `parse_model_options()` assigns that empty result to `sconfig.disk_cache_dir` (line 459), which is the disabled sentinel. On Windows — a build configuration this repository supports (`#if defined(_WIN32)` paths throughout the server) — `XDG_CACHE_HOME` is never set and `HOME` is normally unset, so every Windows launch gets the cache silently disabled, contradicting the documented default (on, 128 GB) with no log line telling the user it is off. Use a Windows fallback (e.g. `%LOCALAPPDATA%` / `%USERPROFILE%` under `lucebox\kv-cache`) so the default is actually on there, or at least log that the cache defaulted to off.
</violation>
</file>
Issue overview
-
Exclusive one-server-per-directory lock is POSIX-only; on Windows two servers share the cache and delete each other's fi · likely (
server/src/server/disk_prefix_cache.cpp:616-629) -
Duplicate-save touch updates last_used only in memory, so TTL/LRU after a restart ignore recent use · likely (
server/src/server/disk_prefix_cache.cpp:1153-1158) -
Reclaimed (superseded) bytes are double-counted by concurrent reservations · likely (
server/src/server/prefix_cache.cpp:991-993) -
Agent-turn snapshots get a disk key even when the request's disk scope is not full · likely (
server/src/server/http_server.cpp:4668-4670) -
Unguarded ::geteuid() breaks the Windows build of the test file · confirmed (
server/test/unit/test_server_unit.cpp:10106) -
New full_cache disk fields (disk_enabled, disk_budget_mb, disk_ttl_hours) are not documented in the /props spec · likely (
docs/specs/props-endpoint.md:390-391) -
Dropped disk loads leak unjoined reader threads until server shutdown · likely (
server/src/server/scheduler.cpp:139-145) -
Default disk-cache directory is empty on Windows, silently disabling the on-by-default cache · likely (
server/src/server/server_main.cpp:429-435)
Reviewed 8dcdee64 · 11 candidate issues checked, kept at ≥70% confidence · add the code-review label to review again
…ion-only writes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Allow large captures to evict several eligible entries transactionally. Respect request scopes, refresh duplicate-save TTL metadata, disable failed cache initialization, and reap asynchronous readers during serving. Use an exclusive Windows directory lock and native cache defaults. Cover dense Qwen disk adoption, failed restores, shared capture reservations, and compressed-state exclusion with model-free regressions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8dcdee6 to
d6774ae
Compare
Prerequisite PR stack from the branch, F milestones M1-M9 with tests and 3090 gates against the Modal R1b F rows, multi-round compaction answer, risks (Luce-Org#839 disk, kernel bias on CUDA/HIP, ragged pool vs Luce-Org#652 verify views), and the origin/main merge strategy.
What this does
A long coding-agent session can lose its RAM snapshots in three ways:
luce_server now keeps those sessions on disk, and the disk cache is on by default. A session whose RAM snapshot is gone restores its raw KV from disk and prefills only the new suffix. This works both while the server runs and after a restart.
Defaults and flags
--kv-cache-dir <path>|off$XDG_CACHE_HOME/lucebox/kv-cache, else~/.cache/lucebox/kv-cacheoff(or--no-kv-cache) disables the cache.--kv-cache-budget <MB>131072(128 GB)--kv-cache-ttl <hours>720= never.--kv-cache-shutdown-flush <s>120-1= no limit.The other disk flags are unchanged:
--kv-cache-min-tokens,--kv-cache-interval,--kv-cache-cold-maxand--disk-prefix-cache(defaultfull)./props→full_cachereportsdisk_enabled,disk_budget_mbanddisk_ttl_hours.Dashboard: Luce-Org/luce_box#391 (branch
feat/session-cache-ssd-setting) still proposes 16 GB, off by default. Its default should change to match this PR: on, 128 GB. That repo is not touched here.When something is written: only when it leaves RAM
Nothing is written after a turn. A snapshot is written in two cases:
--kv-cache-shutdown-flush. One log line gives the sessions, edit backups not written, files, MB and seconds, plus anything the limit dropped.Caveat: a crash,
kill -9or power loss loses whatever was only in RAM. States written before that still restore.An active session that stays resident writes nothing. That matters because a 90-120K-token Qwen3.8-27B session with q4_0 KV is 1.5-2.2 GB per snapshot. The 30 s idle write-behind of earlier revisions is gone.
What is persisted
Edit-backup rule. Since #835, a new turn keeps the previous turn's snapshot in RAM so that an edit of the last message can resume there.
PrefixCache::persist_on_evict). It only serves an edit, which the deeper file cannot, and writing it would double the bytes per conversation.Compressed prompts (PFlash, FlowKV) are not raw history and never get a disk key.
How it works
Writes. Eviction hands the snapshot buffer to a writer thread without a copy, and the request continues at once.
<file>.tmp, isfdatasynced and renamed, and the directory is synced.Restore. With the
fullpolicy, a lookup probes three keys: the whole prompt, every chat boundary, and the deepest stored key that is a prefix of the prompt. These are index lookups; only a hit reads a file.Safety: fail closed.
--max-ctx, the chat template and the draft model;LUCE_DISK_CACHE_DIGEST=1logs an XXH64 of every restored state, taggedssdorram.A large capture evicts several older entries (own commit). Upstream skipped a capture when no single evictable entry freed enough room. A long session that arrived after many short ones (subagent threads) was then never captured, so it reached neither RAM nor disk. Now a capture that fits the budget at all plans several evictions, least recently used first: edit backups first, then leaves. The rule is the same on the single-sequence and paged paths.
budget_skips).Pre-existing bug fixed (qwen35).
snapshot_adopt()never set the snapshot layout, so every Qwen disk restore was rejected. The caller then decoded on an empty cache, which produced garbage output. Adopted snapshots are now dense, and a failed restore fails the request.Commits
Folded into exactly 3 commits on
main(5f402c61f), including the review follow-ups. The fold preserves the verified final tree.5ee22b5745f744cd83d6774ae46Review fixes and validation (13 Oct)
Corrupt tensor metadata is rejected before GGML tensor construction: unsupported types, invalid quantized dimensions, overflowing shapes and out-of-file payloads become logged misses. The original corruption regression reproduced a floating-point exception; four malformed metadata cases now pass during live lookup and restart.
The follow-up fixes also disable a failed cache initialization so RAM serving continues without an unstarted writer queue; refresh duplicate-save TTL timestamps on disk; respect request disk scope for agent-turn saves and concurrent reads/writes; reap completed abandoned reader threads during serving; and provide an exclusive Windows directory lock and native Windows default cache directory.
/props.full_cachedocuments the effective disk enablement, budget and TTL. The disk write-failure regression is portable to Windows and root-owned test runs.The reported
snapshot_adopt/restore_and_generate_implfailure is fixed here: adopted Qwen snapshots are dense, and restore failure returns an error before decoding from cleared KV. A CPU-backed test uses the real Qwen backend to load a.dkv, adopt it, restore exact tensor values, and reject an incompatible restore without generating tokens.The shared-ancestor reservation concern did not reproduce: two capture orders are tested at tight budgets. Reservations retain each capture's full pending bytes, so the shared ancestor is not credited twice. No accounting workaround was added.
Validation on the final tree:
luce_server,test_server_unit,test_recurrent_snapshot.CUDA and Windows builds and R9700/Strix GPU CI passed on the preceding published head; the DGX job was cancelled before running steps. CI is rerunning for these follow-ups. This review did not run a new live GPU workload; the hardware measurements below are the earlier gates.
Numerics
SSD restore vs RAM restore of the same state: byte-identical.
A RAM restore and a fresh prefill can still differ, because prefill runs over different chunk boundaries. That is a prefix-cache property, unchanged here. In the gate, every answer that differs from the reference belongs to a request restored at a different depth.
Gate results: Modal L40S
Setup:
44K: 22.97 / 1.04 / 1.10 / 0.91 s (1124 MB, 0.32 s)
88K: 64.30 / 0.77 / 2.03 / 1.60 s (1895 MB, 0.61 s)
c4 (fix): median 1.003, but one request is over the 1.15 line, so the pre-registered label is STALL-SUSPECT. That request is B3-probe at 0.99 vs 0.86 s. The 70K B3 state, skipped before the fix, is now captured: it pushes out 1.6 GB, and the next probe pushes it out again (1.6 GB written while that probe prefills).
The gate found one bug. On the single-sequence path, a RAM hit on the shared 512-token head ended the restore search. A session evicted to disk was therefore never restored while the server ran: 0 SSD restores in the first run. It is fixed in this PR, with a unit test that fails without the fix. After the fix, the same group made 9 SSD restores, all byte-identical where paired.
Unit tests:
test_server_unit679 passed, 0 failed, 2 skipped on this branch (HIP build for gfx1201, GPU hidden). The suite covers:R9700 restart check
One AMD Radeon AI PRO R9700, 12 Oct, built with the PFlash release branch merged in (release defaults on). A 45K-token
agent session (public MIT ggml tool calls and results): turn 1, graceful shutdown, restart, turn 2.
Paths not covered
kill -9. Loses what was only in RAM, by design. Not exercised.--paged-attentionand the concurrentqwen4expengine. They keep their checkpoints outside the backend's snapshot slots and run without a disk cache (logged at startup).PFlash integration limit. The combined #839 + #746 unit suite passes, but compressed PFlash views are not persisted to SSD. This PR excludes shortened snapshots from disk keys; it restores raw session states and ToolMemory replay metadata. Disk-aware PFlash admission and compressed-view restart persistence require a separate integration change.
🤖 Generated with Claude Code