fix(server): slide the prefix-cache restore point past the deepest slot - #665
fix(server): slide the prefix-cache restore point past the deepest slot#665jkyamog wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
With --prefix-cache-slots 4 and a long linearly-growing tool-heavy conversation, all slots form one chain and the only leaf is the deepest slot = the current restore source. select_inline_evict_victim() evicted that leaf and prepare_inline_snap() assigned the new snapshot to the same slot; the prepare_generation_cache guard (snap_slot == cache_slot) then cancelled it, pinning the restore point at the deepest slot while per-turn prefill grew unbounded (observed 1s -> 60s on the 3090s). Thread the restore-source slot into victim selection: - select_inline_evict_victim(..., int skip_index): the in-flight restore source is never a victim. - prepare_inline_snap(..., int restore_source_slot): http_server passes cache.cache_slot when using_restore, else -1. - Fallback: if skipping the restore source leaves no unprotected leaf, evict the shallowest non-protected ancestor instead (its KV is subsumed by every deeper entry); never the protected tools pin, never the restore source. If nothing safe remains, skip the snapshot, and clear pending_protect_ on the refusal path so it cannot leak into a later confirm_inline_snap. The new snapshot now lands in a different slot, so the restore point slides forward each turn. Adds four unit tests: linear-chain eviction picks an ancestor and commits to a different slot; restore source never evicted; protected pin never evicted; branching victim selection unchanged.
deb0631 to
cabce2f
Compare
There was a problem hiding this comment.
1 issue found across 4 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
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.
<file name="server/src/server/prefix_cache.h">
<violation number="1" location="server/src/server/prefix_cache.h:53">
P3: When `skip_index` is set and every leaf is protected, the implementation returns `-1` rather than the oldest protected leaf; qualify this fallback as applying only without a restore source.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // protected_lru (optional, same size): entries marked true are skipped unless | ||
| // every leaf is protected (then the oldest protected leaf is the last resort). |
There was a problem hiding this comment.
P3: When skip_index is set and every leaf is protected, the implementation returns -1 rather than the oldest protected leaf; qualify this fallback as applying only without a restore source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/prefix_cache.h, line 53:
<comment>When `skip_index` is set and every leaf is protected, the implementation returns `-1` rather than the oldest protected leaf; qualify this fallback as applying only without a restore source.</comment>
<file context>
@@ -45,23 +45,26 @@ std::vector<int> find_all_boundaries(const std::vector<int32_t> & ids,
-// 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 unless
+// every leaf is protected (then the oldest protected leaf is the last resort).
+// skip_index (default -1): the in-flight restore source, never a victim; if it
</file context>
| // protected_lru (optional, same size): entries marked true are skipped unless | |
| // every leaf is protected (then the oldest protected leaf is the 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, protected entries remain ineligible and the function may return -1. |
Symptom & root cause (two parts)
Symptom: with
--prefix-cache-slots 4and a long linearly-growing tool-heavy conversation, per-turn prefill grew unbounded (observed 1s → 60s on the 3090s; 63–70s climbing in the pre-fix repro).Root cause, part 1 — restore point pins at the deepest slot. All slots form one chain; the only leaf is the deepest slot, which is also the current restore source.
select_inline_evict_victim()evicted that leaf, andprepare_inline_snap()assigned the new snapshot to the same slot.Root cause, part 2 — the guard cancels the snap. The
prepare_generation_cacheguard (snap_slot == cache_slot) then cancelled the snapshot, so the restore point never advanced past the deepest slot and every turn re-prefilled the growing tail.The fix
Thread the restore-source slot into victim selection:
select_inline_evict_victim(..., int skip_index)— the in-flight restore source is never a victim.prepare_inline_snap(..., int restore_source_slot)—http_serverpassescache.cache_slotwhenusing_restore, else-1.Tests
test_slide_*unit tests inserver/test/test_server_unit.cpp(e.g.test_slide_evicts_ancestor_not_restore_source,test_slide_restore_source_never_evicted): restore source never evicted; protected pin never evicted; branching victim selection unchanged; snap lands in a different slot than the restore source.ctest -R server_unit: 441/441 pass.Verification
lookupprefix_lenadvancing turn over turn.cached_prefixadvancing (72192 → 95744 over 38/39 turns).