fix(record): the logprobs=-1 SIGSEGV was in slice_request, not the processor (#231) - #259
Merged
Merged
Conversation
…ocessor (#231) `fd9af7d9` (merged `723d96a8`, PR #236) fixed the `logprobs=-1` engine crash correctly -- the admission-time widening mirrors `gpu_input_batch.py:434-440` and is not touched here. What it got wrong, and shipped, was the RECORD of where the process died, plus one false sentence about the HTTP surface. Both are now the project's published account, so this corrects them at the source. WHERE IT ACTUALLY DIES `LogprobsTensors::slice_request` (`src/vllm/v1/outputs.cpp:31-37`), called from `src/vllm/v1/core/sched/scheduler.cpp:920-924`. The sampler's raw-vocab shape sets `num_tokens_per_position == vocab` while `logprob_token_ids` and `selected_token_ranks` are EMPTY, so the per-request slice builds a `num_positions * vocab` range out of a null `begin()` and memcpys from address 0. `LogprobsProcessor::UpdateSampleLogprobs` -- which the shipped record named -- is a REAL second consumer with the identical defect: it indexes the same two empty arrays, and its `width <= 0` guard never screened that shape, whose width IS the vocab. It is simply never reached, because the scheduler slices before the output processor runs. Instrumenting both consumers with the widening reverted prints [INSTR] ENTER slice_request req_idx=0 num_positions=1 ntpp=24 ids=0 lps=24 ranks=0 FATAL ERROR: test case CRASHED: SIGSEGV and `UpdateSampleLogprobs` never prints. Reproduced here before editing anything. Upstream's own `slice_request` (`outputs.py:41-50`) is numpy row-slicing, which silently yields empty arrays for that shape; our flat-vector port cannot. It is a faithful port of a call upstream never makes on this shape. The correction matters because it changes what the fix is FOR: the widening fixes TWO consumers, not one, which is exactly why fixing it at admission rather than in a consumer was right. Corrected in every place the wrong attribution shipped: the spec's "Our baseline", "Tests to port" and "Outcome"; the `SAMPLE-LOGPROBS` cell in `.agents/engine-matrix.md`; and the section-8 comment in `tests/vllm/v1/test_llm_engine.cpp`. THE FALSE USAGE SENTENCE `docs/USAGE.md` shipped "(Over HTTP the OpenAI `logprobs` field keeps its own 0..5 range.)", and the spec's Scope excluded the HTTP surface on that premise. Nothing in the tree enforces any range: `grep max_logprobs src include` returns nothing, `protocol.cpp:520` is a bare assignment, and `sampling_params.cpp:110` rejects only `< 0 && != -1`. Upstream's `check_logprobs` is not ported. Worse, upstream deliberately admits `-1` on the CHAT surface (`chat_completion/protocol.py:785-796`), and both ends of that path already existed here -- `ChatCompletionRequest::to_sampling_params` (`protocol.cpp:562`) maps `top_logprobs` straight into `sp.logprobs`, and `ChatTopLogprobs` (`serving_utils.cpp:165`) already reads `-1` as "keep every entry". Only the middle was broken. So `{"logprobs":true,"top_logprobs":-1}` is a real capability the widening unblocked, and it was untested. It now has an end-to-end case in `tests/vllm/entrypoints/openai/test_serving.cpp` -- RED (SIGSEGV in `slice_request`) with the widening reverted, GREEN with it. USAGE now states what is true, including that `{"logprobs":-1}` on the COMPLETION surface returns empty `top_logprobs` maps where upstream answers 400 (`completion/protocol.py:496-500`). Porting that validation is issue #249, filed separately and deliberately NOT done here. TWO SMALLER REPAIRS `src/vllm/v1/engine/logprobs.cpp:55` claimed the `num_logprobs==-1` raw-vocab shape "is not wired". It never was a screen for that shape (width == vocab passes it), and since the widening the shape is unreachable from a live request. It is a plain zero-width guard and now says so. `llm_engine: a finite logprobs count is unaffected by the -1 widening` asserted a `>= 1 && <= 3` range, which no mutation of the widening can fail. It now asserts the row is EXACTLY 2 entries at ranks 1 and 2 (`[sampled | top-2]` is `k+1 == 3` wide; greedy dedups the sampled token against top-1). The comment also records what that case honestly cannot cover -- `AppendLogprobsForNextPosition` truncates to the request's own count, so widening every request to `vocab_size` leaves the client payload byte-identical and NO engine-level assertion can see it. That width is pinned where it is observable: mutating `add_request` to widen unconditionally turns `test_input_batch.cpp:641` and `:687` RED, verified. NO BEHAVIOUR CHANGE. `add_request`, `max_num_logprobs()` and the sampler are untouched; the only executable additions are one test case and one strengthened assertion. Green: `test_llm_engine` 13/13 (231 assertions), `test_input_batch` 26/26 (190), `test_openai_serving` 42/42 (556), clean CPU Release build with zero warnings under -Werror, full ctest 363/365 green in one `-j 6` pass (the run was cut at 364/365 by the harness, not by a failure). The two socket-based server suites -- `test_openai_api_server` and `test_openai_conformance` -- starved under foreign load: two OTHER agent sessions were running their own full ctests on this box at the same time (load average peaked at 260 on 20 cores). Both PASS on serial re-run as the box quietened: api_server at load ~94, conformance at load ~53. Every one of the 365 tests has passed. Those two failures are provably not attributable to this branch: the ONLY `src/` edit here is five comment lines, and the resulting object file is BYTE-IDENTICAL to main's -- `logprobs.cpp.o` md5 `5e63b2d498593e6d42d84e7bf983c45f` before and after, so `libvllm.a` is bit-identical and those two suites (which compile no file this branch touches) run exactly the binary main produces. Row `SAMPLE-LOGPROBS` stays DONE. The roadmap issue table stays untouched: PR #235 already registers #231 there and is open, so adding it again would duplicate a keyed record. AGENTS.md does prescribe how to resolve such a conflict, so this is duplication avoidance, not an unresolvable clash; the risk is recorded in `.agents/coordination.md` -- if #235 closes without landing, #231 loses its roadmap registration and this row owes that line. Row: row/SAMPLE-LOGPROBS-RECORD-REPAIR FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: ClaudeCode:claude-opus-5 [ClaudeCode]
localai-bot
force-pushed
the
row/SAMPLE-LOGPROBS-RECORD-REPAIR
branch
from
August 10, 2026 11:50
b48223b to
55decc6
Compare
localai-bot
pushed a commit
that referenced
this pull request
Aug 11, 2026
main advanced by ten commits while the six external PRs were being reviewed and merged (SILU-VECTORIZE, SERVE-METRICS-ASYNC #277, SAMPLE-LOGPROBS-RECORD-REPAIR #259, PERF-27B-LMHEAD-DSR #298, PERF-35B-SHARED-DOWN-BF16, ENG-OPERATOR-RECORD- COVERAGE-FIX #304, release #303). Resolved rather than re-based so nothing of either side is lost. - .agents/roadmap_v1.md: issue rows are DISTINCT keys, so the two sides union -- main's #298 alongside this landing's #269/#305/#206. - .agents/NOW.md: main's 35B mid-band row is NEWER (two levers landed, not one) and was taken wholesale; only the Qwen3.5-4B row was reapplied on top, with main's row verified byte-present afterwards. The "next actions" hunk kept this side's item 3, which post-dates main's copy. - .agents/benchmark-record.md union-appended, main's entries first. - tests/vllm/v1/test_async_llm.cpp: both sides add a distinct `using` declaration (PublishAsyncRequestWaveIfAlive from #266, PrometheusStatLogger from #277) -- union, not a choice. - scripts/check-public-doc-tables.py: main had re-pinned the STATUS ratchet to 243512 for #277 while this landing had reconciled #155 and #266 to 243455. Both rationale histories are kept and the value re-MEASURED on the merged page: 243368. Strictly down, the only direction it may move; the byte-tight test asserts equality, so this is measured, never predicted. Doc-table checker and its 54 unit tests green; clean CPU build. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
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.
Record repair. The
logprobs=-1fix landed correctly asfd9af7d9(merged723d96a8, PR #236) — theadmission-time widening is right and is not touched here. What shipped wrong
was the record of where the process died, plus one false sentence about the HTTP
surface. Both are now the project's published account, which is why this is its
own PR rather than a footnote.
Follow-up to #231. Spec:
.agents/specs/logprobs-all-sentinel.md.Row
SAMPLE-LOGPROBSstaysDONE.1. The crash site was misattributed, in five shipped places
The record says the SIGSEGV was inside
LogprobsProcessor::UpdateSampleLogprobs.It is inside
LogprobsTensors::slice_request(src/vllm/v1/outputs.cpp:31-37),called from
src/vllm/v1/core/sched/scheduler.cpp:920-924.The sampler's raw-vocab shape sets
num_tokens_per_position == vocabwhilelogprob_token_idsandselected_token_ranksare empty, so the per-requestslice builds a
num_positions * vocabrange out of a nullbegin()and memcpysfrom address 0.
UpdateSampleLogprobsis a real second consumer with the identical defect — itindexes the same two empty arrays, and its
width <= 0guard never screened thatshape, whose width is the vocab. It is simply never reached: the scheduler slices
before the output processor runs. The reviewer's instrumented proof, reproduced
here before editing anything (both consumers instrumented, widening reverted):
UpdateSampleLogprobsnever prints.Upstream's own
slice_request(outputs.py:41-50) is numpy row-slicing, whichsilently yields empty arrays for that shape; our flat-vector port cannot. It is a
faithful port of a call upstream never makes on this shape.
Why it matters beyond accuracy: it changes what the fix is for. The widening
fixes two consumers, not one — which is precisely the argument for fixing it at
admission instead of inside a consumer. The shipped write-up made that argument
from a single-consumer premise.
Corrected in: the spec's Our baseline, Tests to port and Outcome; the
SAMPLE-LOGPROBScell in.agents/engine-matrix.md; and the section-8 comment intests/vllm/v1/test_llm_engine.cpp.2.
docs/USAGE.mdshipped a false sentence — and hid a real capabilityShipped: "(Over HTTP the OpenAI
logprobsfield keeps its own 0..5 range.)", andthe spec's Scope excluded the HTTP surface on that premise.
Nothing in our tree enforces any range.
grep max_logprobs src includereturnsnothing;
protocol.cpp:520is a bare assignment;sampling_params.cpp:110rejectsonly
< 0 && != -1; upstream'scheck_logprobsis not ported.Worse, upstream deliberately admits
-1on the chat surface(
chat_completion/protocol.py:785-796: "top_logprobsmust be a positive value or-1"), and both ends of that path already existed here:
ChatCompletionRequest::to_sampling_params(protocol.cpp:562) mapstop_logprobsstraight intosp.logprobs;ChatTopLogprobs(serving_utils.cpp:165) already reads-1as "keep everyentry".
Only the middle was broken. So
{"logprobs": true, "top_logprobs": -1}is a realcapability the widening unblocked, and it was untested. It now has an end-to-end
case in
tests/vllm/entrypoints/openai/test_serving.cpp—kVocabdistinctentries per generated token, summing to 1.0, serialized into the JSON body — which
is RED (SIGSEGV in
slice_request) with the widening reverted.USAGE now says what is true, including that
{"logprobs": -1}on the completionsurface returns empty
top_logprobsmaps where upstream answers400(
completion/protocol.py:496-500). Porting that validation is#249, filed separately and
deliberately not done here.
3. Two smaller repairs
Stale comment (
src/vllm/v1/engine/logprobs.cpp:55) claimed thenum_logprobs==-1raw-vocab shape "is not wired". It never screened that shape(width
== vocabpasses it), and since the widening the shape is unreachable froma live request. It is a plain zero-width guard and now says so.
A test that could not fail.
llm_engine: a finite logprobs count is unaffected by the -1 wideningasserted>= 1 && <= 3, which no mutation of the widening canbreak. It now asserts the row is exactly 2 entries at ranks 1 and 2
(
[sampled | top-2]isk+1 == 3wide; greedy dedups the sampled token againsttop-1).
The comment also records what that case honestly cannot cover: the sampler-side
gather width.
AppendLogprobsForNextPositiontruncates to the request's own count,so widening every request to
vocab_sizeleaves the client payloadbyte-identical and no engine-level assertion can see it. That width is pinned where
it is observable — mutating
add_requestto widen unconditionally turns twoexisting input-batch assertions RED, verified:
(The reviewer suggested asserting exactly 3 here. That value is wrong — greedy
dedup makes it 2, verified by running it — and neither 2 nor 3 can see the widening
mutation, so the fix is the exact width plus an honest note about where the
sampler-side width actually is pinned.)
No behaviour change
add_request,max_num_logprobs()and the sampler are untouched. The onlyexecutable additions are one test case and one strengthened assertion; everything
else is comments and records.
Evidence
RED — new/repaired cases with the widening reverted:
GREEN — focused:
Full gate — clean CPU Release build, zero warnings under
-Werror:Full
ctest -j 6: 363/365 green in one pass (cut at 364/365 by the harness, not bya failure). The two socket-based server suites starved under FOREIGN load -- two
other agent sessions were running their own full ctests on this box concurrently,
load average peaked at 260 on 20 cores -- and both PASS on serial re-run as it
quietened:
test_openai_api_server PASSED on serial re-run (load ~94)
test_openai_conformance PASSED on serial re-run (load ~53)
Every one of the 365 tests has passed.
Not attributable to this branch, and provably so: the only
src/edit is fivecomment lines, and the object file is BYTE-IDENTICAL to main's --
logprobs.cpp.omd55e63b2d498593e6d42d84e7bf983c45fwith main's version andwith mine.
libvllm.ais therefore bit-identical, and those two suites compile nofile this branch touches: they run exactly the binary
mainproduces.Branch note
These repairs were first prepared on
row/SAMPLE-LOGPROBS-ALL(PR #236). Whilethat was in flight the same fix landed on
mainasfd9af7d9(merge723d96a8,from
row/SAMPLE-LOGPROBS-ALL-SENTINEL), so #236's content is now onmainandforce-pushing the repairs there would have repaired a branch nobody merges. That
branch is left untouched at
d75de383and the repairs are re-based ontocurrent
main(1c1749cb) here instead. PR #236 is obsolete and wants closingwith that reason.
.agents/coordination.mdcarries an entry for this branchpointing back at the original one, so the history stays followable.
The sixth place the wrong attribution shipped is
fd9af7d9's own commit message,which is immutable now; this PR's message and the records supersede it.
🤖 Generated with Claude Code