Repository navigation
[Engine] serve: a zero drafter ring (Metal --mtp) must not void the conversation cache or kill the engine - #4384
Draft
catalinb13 wants to merge 2 commits into
Draft
catalinb13 wants to merge 2 commits into
catalinb13 wants to merge 2 commits into
Conversation
…he conversation cache
On Apple Silicon the decode window runs the MTP draft rounds itself (metal/src/engine/window.cpp):
the session-side MtpDrafter is loaded but keeps no cells, so `mtp.kv_state()` is a default state with
max_cells 0 and its readback is all zeros. Every serve path that passed `&mtp.kv_state()` unconditionally
then asked the snapshot code to describe a draft part that does not exist: a parked or saved image failed
`conversation_snapshot_validate` ("discard invalid snapshot" -> the whole conversation re-prefills), a disk
slot read refused its own file, and `STRATA_SNAPSHOT_VERIFY` read the zero state back and exited the engine
with 1.
Gate those callsites on `mtp.kv_state().max_cells > 0` - the same predicate that answers "does this drafter
own a ring" - so the images carry no draft part and validate against a draft-less drafter; keep the draft
bounds empty in `conversation_session_read_limits` for the same state; only run the verify readback when
there is a ring to read; and dump the failing pool, its identity and the capture's own reason when the K/V
byte estimate refuses, because the estimate's 500 carried no diagnosis.
On CUDA, HIP and SYCL the drafter allocates its ring at load, so every gate passes with the true side and
the default path is unchanged. A drafter bound to the window's real buffers (the `mtp_kv_bind` route of
Niko1221#3564) reports max_cells > 0 and its draft part is described again - this is the fallback for the state
that has none, not a competing design.
Measured on an equivalent diff inside a fork Metal build (M1 Max 64 GB, huihui Qwen3.8-Flash-Next
UD-Q4_K_XL, --mtp spec 2): park 100882 tokens in 141 ms, restore in 493 ms, first follow-up reused
100724 of 100772 tokens (7.6 s against 382.7 s cold), the needle answer stayed correct, decode kept
22.9-23.1 tok/s greedy, and the K/V parity check passed at 2.0e-05 relative L2. Without the gates the
same build re-prefilled every turn and `STRATA_SNAPSHOT_VERIFY` stopped the engine.
…ft part A continuation that branches inside the saved conversation reads again from the start (a checkpoint's recurrent state exists only at its own boundary), and the draft layer's K/V is in the file only when the drafter owns a ring - on the Apple Silicon engine the decode window runs the draft rounds itself, so its files carry none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The title must start with its area: [Engine], [GUI], [TUI], [Benchmark], [Docs] or [Other], for example "[Engine] fix decode stop on long prompts" or "[GUI] keep the model list after a tab switch".
Problem and result
On an Apple Silicon box that runs
--mtp, the decode window performs the draft rounds itself(
metal/src/engine/window.cpp); the session-sideMtpDrafteris loaded but never allocates cells, somtp.kv_state()is a default state:max_cells == 0and every readback through it is zero. The servepaths that pass
&mtp.kv_state()wheneveruse_mtpis set then describe a draft part that does notexist, and the conversation cache turns against itself:
conversation_snapshot_validate->discard invalid snapshot-> the wholeconversation re-prefills on the next turn (observed on our M1 Max: 100772 tokens re-read, 382.7 s);
file never carried, so the read refuses its own file;
STRATA_SNAPSHOT_VERIFYcompares the last session K/V part against the zero drafter, printsERR verifying...to the protocol stdout andreturn 1stops the engine;at all - the estimate collapses to "invalid or overflowing K/V byte estimate".
After this change the same gates pass
nullptrfor a drafter that owns no ring, the images carry no draftpart, validate, park, borrow and restore round-trip, and the verify readback runs only where there is a
session-side ring to read. Outputs stay byte-identical to the no-park run; speculative decode is untouched
(it rides the window's own buffers).
Related discussion
not touch that capture route; it fixes what breaks on top of it whenever the drafter has no ring.
usable under
--mtp: with [Engine] metal: the conversation cache and session files keep the decode window's own K/V (#2502) #3541 alone the park/borrow/disk callsites still hand in the zero ring.mtp_kv_bind()exposes the window's real draft buffers to the session side. If thatlands first,
max_cells > 0becomes true on Metal too, every gate here passes its pointer side, and thedraft part is described again; the PR is the guard for the state that has no ring, not a competing design.
use_mtpis set hereand the drafter object exists; only its ring is elsewhere.
Approach
One predicate:
mtp.kv_state().max_cells > 0- "this drafter owns a ring" - added to every serve callsitethat passes
&mtp.kv_state()into park, borrow, restore, stage validate and the disk slot paths, plus theSTRATA_SNAPSHOT_VERIFYblock.conversation_session_read_limitskeeps the draft bounds empty for adrafter that owns no ring, because a file written this way carries no draft part and
conversation_kv_part_sizeswould refuse the empty state. The K/V byte estimate now names the failingpool, its identity (
draft/session, index) and the capture's own reason on stderr before returning therefusal; that dump is what let us find the root cause through a single HTTP 500.
Compatibility: on CUDA, HIP and SYCL the drafter allocates its ring at load, so every gate takes the true
side and those paths are unchanged line-for-line.
--mtpis optional; without ituse_mtpis false andnothing changes. Files written before this change by a CUDA engine still restore, and files written by a
Metal engine after it carry no draft part by construction.
Validation
Passed:
cmake -G Ninja -B build-metal -S metal -DMETAL_CPP_DIR=third_party/metal-cpp -DSTRATA_GGML_DIR=third_party/llama.cpp && ninja -C build-metal strataat 5e8a4cc on macOS arm64 - linksclean in 27 s (one pre-existing warning,
expert_source.hpp:1267missing-override, untouched here);./build-metal/strata --helpruns.work of [Engine] metal: the conversation cache and session files keep the decode window's own K/V (#2502) #3541's shape, so the disk path is exercised end to end): M1 Max 64 GB, huihui
Qwen3.8-Flash-Next UD-Q4_K_XL,
--mtp ... --spec 2, conversation cache with 6 slots. Parked100882 tokens in 141 ms; disk save of the same conversation 1.67 GB in 1112.9 ms; boot restore 493 ms;
first follow-up reused 100724 of 100772 tokens in 7.4 s against 382.7 s cold; the recall needle answered
correctly after restore; a full re-tokenized continuation kept its needle; greedy decode 22.9-23.1 tok/s
with
--spec 2(21.8 without MTP); the K/V parity check passed at 2.0e-05 relative L2. Without the gatesthe same build logged
discard invalid snapshoton every park and re-prefilled, andSTRATA_SNAPSHOT_VERIFY=1stopped the engine at the restore withERR verifying the drafter's K/V.Failed:
Not run:
backend-neutral and the true side of every gate reproduces the current code, but a CUDA build and a
parking_test.pyrun with--mtpon an NVIDIA box would close this gap - that is the check a maintaineror CI should ask for;
the runtime evidence above comes from the equivalent fork build;
tools/parking_test.pyon this branch as-is (it expects the disk-save capture port of [Engine] metal: the conversation cache and session files keep the decode window's own K/V (#2502) #3541, which thisPR deliberately does not duplicate).
Limits
conversation restarts speculation with a cold draft cache (observed: first decode after a full re-prefill
turn 18-20 tok/s, next request back at 20.1). [Engine] Metal: working session cache, and the 12 GiB its arena was reserving for nothing #3564 is the proper fix for parking the draft itself.
session re-reads from zero by design (a GDN recurrent state exists only at checkpoint boundaries). This
PR neither changes nor hides that: it only stops a zero ring from discarding the cache wholesale.
parking_test.pyrun with--mtpare reported.