Keep split-serving KV retention from decaying on warm requests - #1237
Keep split-serving KV retention from decaying on warm requests#1237michaelneale wants to merge 1 commit into
Conversation
A downstream stage in a split chain lost its disk-tier retention over time: each warm request archived a shallower prefix than the last, until only the 512-token floor candidate survived and a restart restored almost nothing. Two gates caused it, both in the chunked recorder: - `min_record_tokens` (the restore length) skipped candidates at or below the restored prefix. Correct for the resident cache -- they are resident already -- but the archive was fed only from candidates that passed that gate, so on a warm restore the recorder declined to archive precisely the shared bulk it had just served. Candidates below the restore point are now still offered to the archive selector while resident re-recording stays skipped. - Archival was nested inside the `config.downstream.is_some()` branch, so the last stage of a chain -- which has no downstream -- never archived at all. Archival now runs for every stage. Adds `candidate_already_resident` as a named predicate so the resident-cache and archive decisions are visibly distinct, plus tests covering both. Co-authored-by: Michael Neale <14976+michaelneale@users.noreply.github.com> Signed-off-by: Michael Neale <14976+michaelneale@users.noreply.github.com>
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
1 participant
Stacked on #1228 (
feat/kv-prefix-retention). Draft, like its base.What changes for users
On a split mesh, a stage's durable KV prefix cache no longer decays. Before this, each warm request archived a shallower prefix than the last, so a node that had been serving happily for a while would restart and restore almost nothing — the disk tier looked like it worked and then quietly stopped mattering. This is the one blocker called out in issue #1226 for split retention and split seeding.
The two gates
Both in the chunked recorder (
binary_kv.rs,maybe_record_binary_prefill):min_record_tokensvetoed archival, not just recording. It is the restore length; candidates at or below it are resident already, so skipping the resident re-record is right. But the archive selector was only fed from candidates that passed that gate, so on a warm restore the recorder declined to archive exactly the shared bulk it had just served. Candidates below the restore point are now offered toArchiveCandidatewhile resident re-recording stays skipped.candidate_already_residentis a named predicate so the two decisions read as distinct.config.downstream.is_some(). The last stage of a chain has no downstream, so it never archived anything. Archival now runs once per request for every stage, unchanged in cost (still at most one export per request, under the runtime lock).Validation
cargo test -p skippy-server --lib— 399 passed, 0 failedcargo clippy -p skippy-server --all-targets -- -D warnings— cleancargo fmt --all --check— cleanrecord_gate_tests::restored_candidates_are_not_re_recorded_into_the_resident_cache,record_gate_tests::restored_candidates_are_still_offered_for_archivalNot yet done, and the evidence #1226 asks for: a real split run showing per-stage archive depth roughly symmetric across stages, plus a split restart with a measured gain. Unit tests prove the gate logic; they do not prove the retention win.