Repository navigation
fix(qwen4exp): validate prefix snapshot budget - #942
gaurang2456 wants to merge 4 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/qwen4exp/qwen4exp_backend.cpp">
<violation number="1" location="server/src/qwen4exp/qwen4exp_backend.cpp:280">
P2: This minimum is applied to single-sequence snapshots too, so a short valid prefix capture is disabled whenever a 4096-token checkpoint does not fit. Preserve the single-sequence path by applying this minimum only to the concurrent budget, or derive its minimum from the actual single-sequence capture boundary.</violation>
<violation number="2" location="server/src/qwen4exp/qwen4exp_backend.cpp:280">
P2: `min_snapshot` undercounts the checkpoint size that `snapshot_fits()` actually enforces. When the spare memory falls between the raw cache-copy size and the full estimate, this leaves a nonzero allowance but every checkpoint capture fails; calculate the minimum with the same complete checkpoint estimate used by `snapshot_fits()`.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
| plan->layer_bytes = (size_t) slots * layer; | ||
| if (plan->required > available) return 0; // the context fit moves on without searching smaller chunks | ||
| } | ||
| const size_t min_snapshot = snapshot_budget |
There was a problem hiding this comment.
P2: This minimum is applied to single-sequence snapshots too, so a short valid prefix capture is disabled whenever a 4096-token checkpoint does not fit. Preserve the single-sequence path by applying this minimum only to the concurrent budget, or derive its minimum from the actual single-sequence capture boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At server/src/qwen4exp/qwen4exp_backend.cpp, line 280:
<comment>This minimum is applied to single-sequence snapshots too, so a short valid prefix capture is disabled whenever a 4096-token checkpoint does not fit. Preserve the single-sequence path by applying this minimum only to the concurrent budget, or derive its minimum from the actual single-sequence capture boundary.</comment>
<file context>
@@ -277,8 +277,10 @@ int qwen4exp_select_chunk(ggml_backend_t backend, const Qwen4ExpWeights & w,
plan->layer_bytes = (size_t) slots * layer;
if (plan->required > available) return 0; // the context fit moves on without searching smaller chunks
}
+ const size_t min_snapshot = snapshot_budget
+ ? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, 4096)) : 0;
const int chunk = qwen4exp_fit_chunk(cache.max_ctx, available, fixed, chunk_workspace, snapshot_budget,
</file context>
| plan->layer_bytes = (size_t) slots * layer; | ||
| if (plan->required > available) return 0; // the context fit moves on without searching smaller chunks | ||
| } | ||
| const size_t min_snapshot = snapshot_budget |
There was a problem hiding this comment.
P2: min_snapshot undercounts the checkpoint size that snapshot_fits() actually enforces. When the spare memory falls between the raw cache-copy size and the full estimate, this leaves a nonzero allowance but every checkpoint capture fails; calculate the minimum with the same complete checkpoint estimate used by snapshot_fits().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At server/src/qwen4exp/qwen4exp_backend.cpp, line 280:
<comment>`min_snapshot` undercounts the checkpoint size that `snapshot_fits()` actually enforces. When the spare memory falls between the raw cache-copy size and the full estimate, this leaves a nonzero allowance but every checkpoint capture fails; calculate the minimum with the same complete checkpoint estimate used by `snapshot_fits()`.</comment>
<file context>
@@ -277,8 +277,10 @@ int qwen4exp_select_chunk(ggml_backend_t backend, const Qwen4ExpWeights & w,
plan->layer_bytes = (size_t) slots * layer;
if (plan->required > available) return 0; // the context fit moves on without searching smaller chunks
}
+ const size_t min_snapshot = snapshot_budget
+ ? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, 4096)) : 0;
const int chunk = qwen4exp_fit_chunk(cache.max_ctx, available, fixed, chunk_workspace, snapshot_budget,
</file context>
davide221
left a comment
davide221
left a comment
There was a problem hiding this comment.
Code review
Found 1 issue.
Copy to agent
Use the copy button in the top-right corner of the block, then paste into your agent.
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.
Repository: Luce-Org/lucebox
Reviewed commit: c2b17aa6bef9e63089302d55fff288fac135259c
Verify the findings against the current code, run relevant checks for any fixes, and explain any findings you reject.
<file name="server/src/qwen4exp/qwen4exp_backend.cpp">
<violation number="1" line="280" end_line="283">
min_snapshot sized at the 4096-token spacing zeroes allowances that could still hold shorter checkpoints
`min_snapshot` is `qwen4exp_snapshot_bytes(..., min(max_ctx, 4096))`, i.e. the cost of a checkpoint at a full restore-point spacing. But restore points are not all ≥4096 tokens: `spaced_restore_points()` keeps the first boundary wherever it falls, and the server also reserves cuts at PPP pins and chat boundaries (a system/tools head of a few hundred tokens is a first-class checkpoint, see `reserve_inline_snap`'s `forced_cut`/`restore_point_spacing` handling). On a memory-tight device where `spare` lands between `bytes(short head)` and `bytes(4096)`, the old code granted `allowance = spare`, and captures of short heads succeeded (the seq engine checks each capture against its actual token count, `estimate_prefix_store_bytes(ticket.checkpoint.tokens)`). With this change the reservation is rejected, `snapshot_budget_` becomes 0, `snapshot_allowance_bytes()` returns 0, and the scheduler disables the prefix cache entirely — including the single-sequence path (slots=1, 3 snapshots requested in `plan_chunk`), which the PR claims to preserve. Caching that previously worked for short prefixes is silently turned off. The minimum viable checkpoint should be derived from the smallest boundary the server can actually cut at (or the check should accept an allowance that holds any restorable checkpoint), not from the spacing constant.
<suggestion>
const size_t min_snapshot = snapshot_budget
? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, restore_point_spacing())) : 0;
</suggestion>
</violation>
</file>
Issue overview
- min_snapshot sized at the 4096-token spacing zeroes allowances that could still hold shorter checkpoints · likely (
server/src/qwen4exp/qwen4exp_backend.cpp:280-283)
Reviewed c2b17aa6 · 2 candidate issues checked, kept at ≥70% confidence · add the code-review label to review again
| const size_t min_snapshot = snapshot_budget | ||
| ? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, 4096)) : 0; | ||
| const int chunk = qwen4exp_fit_chunk(cache.max_ctx, available, fixed, chunk_workspace, snapshot_budget, | ||
| floor_rows, max_rows); | ||
| floor_rows, max_rows, min_snapshot); |
There was a problem hiding this comment.
min_snapshot sized at the 4096-token spacing zeroes allowances that could still hold shorter checkpoints · likely
min_snapshot is qwen4exp_snapshot_bytes(..., min(max_ctx, 4096)), i.e. the cost of a checkpoint at a full restore-point spacing. But restore points are not all ≥4096 tokens: spaced_restore_points() keeps the first boundary wherever it falls, and the server also reserves cuts at PPP pins and chat boundaries (a system/tools head of a few hundred tokens is a first-class checkpoint, see reserve_inline_snap's forced_cut/restore_point_spacing handling). On a memory-tight device where spare lands between bytes(short head) and bytes(4096), the old code granted allowance = spare, and captures of short heads succeeded (the seq engine checks each capture against its actual token count, estimate_prefix_store_bytes(ticket.checkpoint.tokens)). With this change the reservation is rejected, snapshot_budget_ becomes 0, snapshot_allowance_bytes() returns 0, and the scheduler disables the prefix cache entirely — including the single-sequence path (slots=1, 3 snapshots requested in plan_chunk), which the PR claims to preserve. Caching that previously worked for short prefixes is silently turned off. The minimum viable checkpoint should be derived from the smallest boundary the server can actually cut at (or the check should accept an allowance that holds any restorable checkpoint), not from the spacing constant.
Checked (78%): The mechanism checks out: restore points can be much shorter than 4096 tokens (spaced_restore_points keeps the first boundary wherever it falls, prefix_cache.cpp:329-336, and unspaced forced_cut cuts are added at :346-348), captures are charged by actual token count (qwen4exp_seq_engine.cpp:320), and min_snapshot is sized at min(max_ctx, 4096) (qwen4exp_backend.cpp:280-281). When spare memory lands between bytes(short head) and bytes(4096), qwen4exp_chunk.h:48-51 zeroes the allowance that the old code granted, making snapshot_allowance_bytes() 0 (qwen4exp_backend.h:74) so the scheduler disables the prefix cache entirely (scheduler.cpp:110) and supports_prefix_store() goes false (qwen4exp_seq_engine.h:59), killing the slots=1 path too. The only assumption left is that such an in-between memory state arises in practice; the PR's own stated intent is only to reject allowances that cannot hold a viable checkpoint, and short-head checkpoints were viable before this change.
| const size_t min_snapshot = snapshot_budget | |
| ? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, 4096)) : 0; | |
| const int chunk = qwen4exp_fit_chunk(cache.max_ctx, available, fixed, chunk_workspace, snapshot_budget, | |
| floor_rows, max_rows); | |
| floor_rows, max_rows, min_snapshot); | |
| const size_t min_snapshot = snapshot_budget | |
| ? qwen4exp_snapshot_bytes(backend, cache, std::min(cache.max_ctx, restore_point_spacing())) : 0; |
|
@gaurang2456 thanks for the contribution. Can you validate bugs and resolve conflicts |
CI failed on
|
|
Hi @davide221 Davide, I’ve addressed the review comments and the CI compilation issues you pointed out.
|
CI failed on
|
Summary
Fixes #894, where Qwen4Exp prefix caching could silently become ineffective under concurrent workloads because the available snapshot budget was insufficient to hold a viable checkpoint.
Changes
qwen4exp_fit_chunk().qwen4exp_snapshot_bytes(), accounting for restore-point spacing and context length.Testing
test_qwen4exp_chunk.cppandtest_server_unit.cpp.git diff --checkpasses.CI validation is needed for native compilation and test execution.