diff --git a/server/src/server/http_server.cpp b/server/src/server/http_server.cpp index a909344e8..3ed8ca7e7 100644 --- a/server/src/server/http_server.cpp +++ b/server/src/server/http_server.cpp @@ -3392,11 +3392,17 @@ HttpServer::GenerationCacheState HttpServer::prepare_generation_cache( // requests prefer the reusable system/tool boundary; otherwise an // enabled exact full-prompt cache retains its existing priority. auto prepare_inline = [&]() { + // Never let the new snapshot land in the slot this request restores + // from: the guard below would cancel it, pinning the restore point at + // the deepest slot on linearly-growing conversations. + const int restore_source_slot = + cache.using_restore ? cache.cache_slot : -1; const auto prepared_snapshot = prefix_cache_.prepare_inline_snap( effective_prompt, cache.using_restore ? logical_prefix_len : 0, prefer_tools_boundary, - forced_cut); + forced_cut, + restore_source_slot); cache.snap_slot = prepared_snapshot.first; cache.snap_cut = prepared_snapshot.second; }; @@ -3660,7 +3666,9 @@ void HttpServer::remember_agent_turn( const int canonical_end = (int) canonical_tokens.size(); const auto pending = prefix_cache_.prepare_inline_snap( - canonical_tokens, source_pos, false, canonical_end); + canonical_tokens, source_pos, false, canonical_end, source_slot); + // No safe victim (only the restore source and/or protected pins remain) + // or no useful boundary: nothing to replay into. if (pending.first < 0 || pending.second != canonical_end) return; const int slot = pending.first; diff --git a/server/src/server/prefix_cache.cpp b/server/src/server/prefix_cache.cpp index 8bb67aac6..dfc67af99 100644 --- a/server/src/server/prefix_cache.cpp +++ b/server/src/server/prefix_cache.cpp @@ -171,35 +171,63 @@ static bool is_strict_prefix(const std::vector & a, } int select_inline_evict_victim(const std::vector *> & ids_lru, - const std::vector * protected_lru) { + const std::vector * protected_lru, + int skip_index) { const int n = (int)ids_lru.size(); if (n <= 0) return 0; auto is_protected = [&](int i) { return protected_lru && i >= 0 && i < (int)protected_lru->size() && (*protected_lru)[(size_t)i]; }; - // Oldest-first scan: prefer an unprotected leaf so sticky tools pins survive. - int oldest_protected_leaf = -1; - for (int i = 0; i < n; i++) { - bool is_ancestor = false; + auto is_ancestor = [&](int i) { for (int j = 0; j < n; j++) { if (j == i) continue; - if (is_strict_prefix(*ids_lru[i], *ids_lru[j])) { is_ancestor = true; break; } + if (is_strict_prefix(*ids_lru[i], *ids_lru[j])) return true; } - if (is_ancestor) continue; + return false; + }; + // Oldest-first scan: prefer an unprotected leaf so sticky tools pins + // survive. skip_index (the in-flight restore source) is never a victim. + int oldest_protected_leaf = -1; + for (int i = 0; i < n; i++) { + if (i == skip_index) continue; + if (is_ancestor(i)) continue; if (!is_protected(i)) return i; // oldest unprotected leaf if (oldest_protected_leaf < 0) oldest_protected_leaf = i; } + if (skip_index >= 0) { + // No unprotected leaf outside the restore source — e.g. a linearly + // growing conversation whose only leaf is the restore source itself. + // Evict the shallowest non-protected ancestor (its KV is subsumed by + // every deeper entry) so the new snapshot lands in a different slot + // and the restore point can slide forward. Never the protected tools + // pin, never the restore source. + int shallowest_ancestor = -1; + for (int i = 0; i < n; i++) { + if (i == skip_index || is_protected(i)) continue; + if (!is_ancestor(i)) continue; + if (shallowest_ancestor < 0 || + ids_lru[i]->size() < ids_lru[(size_t)shallowest_ancestor]->size()) { + shallowest_ancestor = i; + } + } + if (shallowest_ancestor >= 0) return shallowest_ancestor; + // Only the restore source and/or protected pins remain: destroying + // either would throw away the stable tools head or the in-flight + // restore, so there is no safe victim. + return -1; + } if (oldest_protected_leaf >= 0) return oldest_protected_leaf; return 0; // unreachable (the longest entry is always a leaf); pure-LRU fallback } int select_inline_evict_victim(const std::vector> & ids_lru, - const std::vector * protected_lru) { + const std::vector * protected_lru, + int skip_index) { std::vector *> ptrs; ptrs.reserve(ids_lru.size()); for (const auto & v : ids_lru) ptrs.push_back(&v); - return select_inline_evict_victim(ptrs, protected_lru); + return select_inline_evict_victim(ptrs, protected_lru, skip_index); } int select_inline_snapshot_boundary(const std::vector & boundaries, @@ -341,7 +369,8 @@ std::pair PrefixCache::prepare_inline_snap( const std::vector & prompt_ids, int restored_prefix_len, bool prefer_tools_boundary, - int forced_cut) { + int forced_cut, + int restore_source_slot) { if (disabled_) return {-1, 0}; auto candidates = find_all_boundaries(prompt_ids, markers_); @@ -372,7 +401,10 @@ std::pair PrefixCache::prepare_inline_snap( if ((int)entries_.size() >= cap_) { // At capacity — reserve a slot without evicting yet. Prefix-aware: prefer // the oldest leaf so shared ancestor prefixes (reused by later branches) - // stay resident. Skip protected tools pins when an unprotected leaf exists. + // stay resident. Skip protected tools pins when an unprotected leaf + // exists. The in-flight restore source is never a victim, so the new + // snapshot lands in a different slot and the restore point can slide + // forward past the deepest slot. std::vector *> ids_lru; std::vector protected_lru; ids_lru.reserve(entries_.size()); @@ -381,7 +413,23 @@ std::pair PrefixCache::prepare_inline_snap( ids_lru.push_back(&e.ids); protected_lru.push_back(e.protect); } - int victim = select_inline_evict_victim(ids_lru, &protected_lru); + int skip_index = -1; + if (restore_source_slot >= 0) { + for (int i = 0; i < (int)entries_.size(); i++) { + if (entries_[i].slot == restore_source_slot) { + skip_index = i; + break; + } + } + } + int victim = select_inline_evict_victim(ids_lru, &protected_lru, skip_index); + if (victim < 0) { + // Nothing safe to evict (only the restore source and/or protected + // pins remain). Skip this snapshot; the restore point stays put + // rather than being destroyed. + pending_protect_ = false; + return {-1, 0}; + } pending_evict_key_ = entries_[victim].hash; has_pending_evict_ = true; slot = entries_[victim].slot; @@ -393,8 +441,16 @@ std::pair PrefixCache::prepare_inline_snap( entries_[victim].ids.size(), entries_.front().ids.size()); } } else { + // Skip the in-flight restore source so the new snapshot lands in a + // different slot (the http_server/agent-replay guards would cancel + // an unlucky collision, leaving the restore point pinned). With a + // vacancy and cap >= 2 there is always a non-restore slot to take; + // cap == 1 keeps the old guard behavior. slot = next_slot_; - next_slot_ = (next_slot_ + 1) % cap_; + if (slot == restore_source_slot && cap_ > 1) { + slot = (slot + 1) % cap_; + } + next_slot_ = (slot + 1) % cap_; has_pending_evict_ = false; } diff --git a/server/src/server/prefix_cache.h b/server/src/server/prefix_cache.h index af32359e0..f857e008b 100644 --- a/server/src/server/prefix_cache.h +++ b/server/src/server/prefix_cache.h @@ -45,23 +45,28 @@ std::vector find_all_boundaries(const std::vector & ids, using PrefixHash = std::array; PrefixHash hash_prefix(const int32_t * ids, int count); -// Prefix-aware inline eviction policy. Given the cached prefixes in LRU order -// (index 0 = oldest), return the index of the eviction victim: the oldest entry -// whose ids are NOT a strict prefix of any other entry's ids (a "leaf"). Keeping -// shared ancestor prefixes resident avoids re-prefilling them for later branches. -// Returns 0 (pure-LRU fallback) when ids_lru is empty or, impossibly, no leaf -// is found. Pure and model-free so it can be unit-tested without a PrefixCache. -// The pointer overload is the core (the caller passes pointers into its own -// entries so no token vectors are copied); the value overload is a convenience -// wrapper for tests. +// Prefix-aware inline eviction: given cached prefixes in LRU order (0 = oldest), +// return the index of the oldest "leaf" — an entry that is not a strict prefix +// of any other — so shared ancestors stay resident. Pointer overload is the +// core (no token copies); the value overload is for tests. // -// When `protected_lru` is non-null and same-sized, entries with -// `(*protected_lru)[i] == true` are skipped unless every leaf is protected -// (then the oldest protected leaf is chosen as a last resort). +// protected_lru (optional, same size): entries marked true are skipped. +// Without skip_index, if every leaf is protected, the oldest protected leaf +// is the last resort. With skip_index set, protected entries stay ineligible +// and the function may return -1 instead (see skip_index below). +// skip_index (default -1): the in-flight restore source, never a victim; if it +// is the only unprotected leaf, evict the shallowest non-protected ancestor +// instead so the restore point can slide. The protected pin is never evicted. +// +// Returns the victim index [0, n-1]; -1 if skip_index is set and only the +// restore source and/or protected pins remain; 0 if ids_lru is empty or, +// impossibly, no leaf exists. int select_inline_evict_victim(const std::vector *> & ids_lru, - const std::vector * protected_lru = nullptr); + const std::vector * protected_lru = nullptr, + int skip_index = -1); int select_inline_evict_victim(const std::vector> & ids_lru, - const std::vector * protected_lru = nullptr); + const std::vector * protected_lru = nullptr, + int skip_index = -1); // Pick the inline snapshot boundary for a request. // Default: boundary before the current user turn (second-to-last marker), @@ -121,12 +126,17 @@ class PrefixCache { // `prefer_tools_boundary` selects the system/tools head first (see // select_inline_snapshot_boundary). When `forced_cut` > restored, that // cut is used instead (PPP pin_end, including mid-message LCP cuts). + // `restore_source_slot` (default -1) is the slot this request restores + // from; at capacity it is never chosen as the eviction victim, so the new + // snapshot lands in a different slot and the restore point can slide + // forward past the deepest slot. // Returns (slot, target_cut) or (-1, 0). std::pair prepare_inline_snap( const std::vector & prompt_ids, int restored_prefix_len = 0, bool prefer_tools_boundary = false, - int forced_cut = 0); + int forced_cut = 0, + int restore_source_slot = -1); // Confirm after daemon successfully saved the snapshot. // `protect` marks the entry non-evictable by unprotected traffic (tool pin). diff --git a/server/test/test_server_unit.cpp b/server/test/test_server_unit.cpp index 01dad8dcc..31805a2ab 100644 --- a/server/test/test_server_unit.cpp +++ b/server/test/test_server_unit.cpp @@ -2456,6 +2456,203 @@ TEST_CASE(ServerUnitFixture, test_evict_all_protected_falls_back) { TEST_ASSERT(select_inline_evict_victim(ids, &protect) == 0); } +// ── Restore-source-aware eviction (prefix-cache slide) ───────────────── + +// (a) Linear chain at capacity: the new snapshot must land in a different +// slot than the restore source, so the restore point can slide forward past +// the deepest slot. +TEST_CASE(ServerUnitFixture, test_slide_evicts_ancestor_not_restore_source) { + const std::string path = write_deepseek_marker_tokenizer_fixture(); + Tokenizer tokenizer; + TEST_ASSERT(tokenizer.load_from_gguf(path.c_str())); + PrefixCache cache(4, tokenizer); + TEST_ASSERT(!cache.disabled()); + + // Linear chain: each prompt strictly extends the previous one. + std::vector p1 = {1, 100, 4, 101}; + std::vector p2 = p1; + p2.insert(p2.end(), {3, 102}); + std::vector p3 = p2; + p3.insert(p3.end(), {4, 103}); + std::vector p4 = p3; + p4.insert(p4.end(), {3, 104}); + + auto fill = [&](const std::vector & p) { + const auto prepared = cache.prepare_inline_snap( + p, 0, false, (int) p.size()); + TEST_ASSERT(prepared.first >= 0); + TEST_ASSERT(prepared.second == (int) p.size()); + cache.confirm_inline_snap(prepared.first, prepared.second, p); + return prepared.first; + }; + const int s1 = fill(p1); + const int s2 = fill(p2); + const int s3 = fill(p3); + const int s4 = fill(p4); + TEST_ASSERT(s1 != s2 && s2 != s3 && s3 != s4 && s4 != s1); + TEST_ASSERT(s4 == 3); // deepest slot, like the BUG.md repro + + // Turn 5: restore from the deepest slot and extend the conversation. + std::vector p5 = p4; + p5.insert(p5.end(), {3, 105}); + const auto hit = cache.lookup(p5); + TEST_ASSERT(hit.first == s4 && hit.second == (int) p4.size()); + + const auto snap = cache.prepare_inline_snap( + p5, hit.second, false, (int) p5.size(), hit.first); + TEST_ASSERT(snap.first >= 0); + TEST_ASSERT(snap.first != s4); // different slot: the restore source + // was not the victim + TEST_ASSERT(snap.second == (int) p5.size()); + cache.confirm_inline_snap(snap.first, snap.second, p5); + + // The restore point slid forward: the new, deeper prefix now matches. + const auto after = cache.lookup(p5); + TEST_ASSERT(after.first == snap.first); + TEST_ASSERT(after.second == (int) p5.size()); + // The old deepest entry survived the eviction. + std::vector p4b = p4; + p4b.insert(p4b.end(), {7, 7}); + const auto kept = cache.lookup(p4b); + TEST_ASSERT(kept.first == s4 && kept.second == (int) p4.size()); + TEST_ASSERT(cache.stats().in_use == 4); + unlink(path.c_str()); +} + +// (b) The in-flight restore source is never the eviction victim, at any LRU +// position, whether it is the only leaf (linear chain) or not. +TEST_CASE(ServerUnitFixture, test_slide_restore_source_never_evicted) { + std::vector> ids = { + {9}, {9, 1}, {9, 1, 2}, {9, 1, 2, 3}, + }; + for (int skip = 0; skip < 4; ++skip) { + const int victim = select_inline_evict_victim(ids, nullptr, skip); + TEST_ASSERT(victim >= 0 && victim != skip); + } + // Linear chain whose only leaf is the restore source: evict the + // shallowest unprotected ancestor instead of cancelling everything. + TEST_ASSERT(select_inline_evict_victim(ids, nullptr, 3) == 0); + // With a free leaf the usual leaf preference still applies. + TEST_ASSERT(select_inline_evict_victim(ids, nullptr, 0) == 3); +} + +// (c) The protected tools pin is never evicted, even when it is the +// shallowest ancestor and the only other entry besides the restore source. +TEST_CASE(ServerUnitFixture, test_slide_protected_pin_never_evicted) { + std::vector> ids = { + {9}, {9, 1}, {9, 1, 2}, {9, 1, 2, 3}, + }; + std::vector protect = {true, false, false, false}; + TEST_ASSERT(select_inline_evict_victim(ids, &protect, 3) == 1); + + // Only the protected pin and the restore source remain: no safe victim, + // so no snapshot is reserved instead of destroying the pin. + std::vector> two = {{9}, {9, 1}}; + std::vector two_prot = {true, false}; + TEST_ASSERT(select_inline_evict_victim(two, &two_prot, 1) == -1); + + const std::string path = write_deepseek_marker_tokenizer_fixture(); + Tokenizer tokenizer; + TEST_ASSERT(tokenizer.load_from_gguf(path.c_str())); + PrefixCache cache(2, tokenizer); + TEST_ASSERT(!cache.disabled()); + + std::vector pin = {1, 100, 4, 101}; + std::vector deep = pin; + deep.insert(deep.end(), {3, 102}); + auto prepared = cache.prepare_inline_snap(pin, 0, true, (int) pin.size()); + TEST_ASSERT(prepared.first == 0); + cache.confirm_inline_snap(prepared.first, prepared.second, pin, true); + prepared = cache.prepare_inline_snap(deep, 0, false, (int) deep.size()); + TEST_ASSERT(prepared.first == 1); + cache.confirm_inline_snap(prepared.first, prepared.second, deep); + + std::vector deeper = deep; + deeper.insert(deeper.end(), {4, 103}); + const auto hit = cache.lookup(deeper); + TEST_ASSERT(hit.first == 1 && hit.second == (int) deep.size()); + // At capacity the only other entry is the protected pin: refuse rather + // than evict it. + const auto refused = cache.prepare_inline_snap( + deeper, hit.second, false, (int) deeper.size(), hit.first); + TEST_ASSERT(refused.first == -1 && refused.second == 0); + const auto kept = cache.lookup(deep); + TEST_ASSERT(kept.first == 1 && kept.second == (int) deep.size()); + TEST_ASSERT(cache.stats().in_use == 2); + unlink(path.c_str()); +} + +// (d) Branching conversations are unchanged: with two leaves, the oldest +// non-restore-source leaf is still the victim. +TEST_CASE(ServerUnitFixture, test_slide_branching_oldest_leaf_unchanged) { + // [9] is a shared root; leaves are idx 1 ([9,1]) and idx 2 ([9,2]). + std::vector> ids = {{9}, {9, 1}, {9, 2}}; + TEST_ASSERT(select_inline_evict_victim(ids) == 1); // original behavior + // Restore source is the newer leaf: the older leaf is still the victim. + TEST_ASSERT(select_inline_evict_victim(ids, nullptr, 2) == 1); + // Restore source is the older leaf: the remaining leaf is the victim. + TEST_ASSERT(select_inline_evict_victim(ids, nullptr, 1) == 2); + // A protected leaf is never evicted: with the restore source skipped and + // the only remaining leaf protected, the shallowest unprotected ancestor + // is the victim instead. + std::vector protect = {false, true, false}; + TEST_ASSERT(select_inline_evict_victim(ids, &protect, 2) == 0); +} + +// (e) Free-slot path (not at capacity) must skip the restore source too, so +// the http_server / agent-replay guards do not cancel the reservation and the +// restore point can advance. +TEST_CASE(ServerUnitFixture, test_slide_free_slot_skips_restore_source) { + const std::string path = write_deepseek_marker_tokenizer_fixture(); + Tokenizer tokenizer; + TEST_ASSERT(tokenizer.load_from_gguf(path.c_str())); + PrefixCache cache(4, tokenizer); + TEST_ASSERT(!cache.disabled()); + + // Two entries with cap 4: vacancy exists, so prepare_inline_snap takes + // the free-slot path. Round-robin has next_slot_ at 2. + std::vector p1 = {1, 100, 4, 101}; + std::vector p2 = p1; + p2.insert(p2.end(), {3, 102}); + + auto fill = [&](const std::vector & p) { + const auto prepared = cache.prepare_inline_snap( + p, 0, false, (int) p.size()); + TEST_ASSERT(prepared.first >= 0); + TEST_ASSERT(prepared.second == (int) p.size()); + cache.confirm_inline_snap(prepared.first, prepared.second, p); + return prepared.first; + }; + const int s1 = fill(p1); // slot 0 + const int s2 = fill(p2); // slot 1 + TEST_ASSERT(s1 == 0 && s2 == 1); + + // Drive the round-robin so next_slot_ lands exactly on s2 (the restore + // source we will pass in). Three abort-burn steps from slot 2 → 3 → 0 → 1. + for (int i = 0; i < 3; ++i) { + std::vector scratch = p2; + scratch.push_back(7); + scratch.push_back(7 + i); + const auto prep = cache.prepare_inline_snap( + scratch, 0, false, (int) scratch.size()); + TEST_ASSERT(prep.first >= 0); + // Burn the round-robin step without committing an entry. + cache.cancel_inline_snap(prep.first); + } + + // Restore source is s2 = 1. Free slots are 2 and 3. The next free-slot + // allocation must skip s2 (== 1) and pick a non-restore slot. + std::vector p3 = p2; + p3.insert(p3.end(), {4, 103}); + const auto hit = cache.lookup(p3); + TEST_ASSERT(hit.first == s2); + const auto snap = cache.prepare_inline_snap( + p3, hit.second, false, (int) p3.size(), hit.first); + TEST_ASSERT(snap.first >= 0); + TEST_ASSERT(snap.first != hit.first); + unlink(path.c_str()); +} + // ═══════════════════════════════════════════════════════════════════════ // PFlash config tests (model-free) // ═══════════════════════════════════════════════════════════════════════