Conversation
Signed-off-by: Costa Tsaousis <costa@netdata.cloud>
Signed-off-by: Costa Tsaousis <costa@netdata.cloud>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
Thanks for chasing this down. The idea at the core of this PR is right: a listed manifest must never lose the last copy of a page it needs. We reproduced the bug on the current beta with mrweiner's reproducer (GLM-5.3-Flash Spark TP2, L1 24 GB / L2 128 GB, on-evict). R1 (supersede + clean restart) fails with the exact fingerprint, We are not merging this PR as it stands. Besides the fix, the commit changes a lot of shared storage-engine behaviour for ordinary KV and every adapter. Our review reproduced several regressions from that part:
The volume of disk writes is a separate question. What we merged instead: #102, a focused fix for the loss you, hashspamjam and mrweiner reported.
If you still see listed-but-unreadable checkpoints after that fix, the ownership guard (retire the dependent manifests before deleting a page's last copy) would be welcome as its own small PR. The store-queue, pinning and adapter changes should come in separate PRs, each with its own tests, so they can be judged on their own. We are happy to review them. 🤖 Generated with Claude Code |
|
Thanks. Two of this PR's core ideas are now merged in #104:
#104 also keeps the checkpoint that a side request or sub-agent fork extends. It does this without the storage-engine changes from our review:
It also does not write retained branches on eviction, which measured about as much as write-through. End to end on GLM Spark TP2, the beta vs #104 (details in #104):
Closing in favour of #104. If you still see 🤖 Generated with Claude Code |
A newer recurrent checkpoint can supersede an older branch while the older manifest remains discoverable. With deferred writes, RAM eviction can then discard the older checkpoint's unique state pages before they reach disk. A later restore finds the attention pages but not the state, falls back through incomplete checkpoints and recomputes the prompt.
Keep checkpoint publication, payload ownership and eviction consistent across the RAM/disk lifecycle. Supersession becomes an eviction preference rather than permission to discard an unwritten retained branch. Cache loss remains allowed under bounded resource pressure: retire affected generations before deliberately reclaiming their last known copy, so they become ordinary misses rather than advertised incomplete restores.
The implementation covers the consequences of that ownership rule:
Dependencies and review scope:
65254d20cc1a05e4a196a0efe998a6dd3b6e4922; its two existing admission commits remain separate. Until [vLLM] Wait for recurrent checkpoint restore capacity instead of recomputing #100 merges, GitHub's branch comparison includes that dependency. Review the new cache-integrity commit separately. Both existing admission PRs are unchanged.b12x-checkpoint-shutdown-order.Validation:
on-evicttraffic crossed RAM and disk eviction thresholds. By the 10:25 UTC observation, 34,076 on-evict page writes had persisted with zero write timeouts. Complete mixed-tier retrievals included 44/44 pages: 12 RAM + 32 disk, taking 117–118 ms. The 10:01–10:25 window had no missing-page/failed-restore errors or warnings, and no new service restart.Draft qualification limits: a full production restore with every page read from disk, orderly restart with active GPU transfers, and sustained physical SSD-write reduction remain unqualified. A roughly 40-second zero-output interval cleared without intervention during the workload; its cause is unresolved. A nearby warned deletion completed before that interval, so no causal attribution is made. These live observations support the fixes but are not a completed production acceptance gate.