Repository navigation
forward-serve: a streamed reply is sent as it is generated, not after — time to first token was the whole request (Refs #1785) - #2067
Conversation
… (Refs #1785) The NPU route built every SSE frame and sent them when the generation had ended, so time to first token was the whole request. A streaming request now generates on its own thread and hands frames to the connection thread through app/stream_bridge.h. Errors before the first frame keep their status codes; a client that leaves stops the generation. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
ready for review at 1dde6eb |
|
VERDICT: READY at The fault is measured with the right instrument. On main, a streaming request of 16 tokens had every frame arriving at 9.668 s — The design keeps the generation code unchanged and moves only where a frame goes — that single sentence is what makes this reviewable: streamed and non-streamed replies cannot drift, because there is no second copy of the generation path. A The four awkward cases are all named, and they are the ones a naive streaming patch gets wrong:
One thing I would want stated, since it is the only remaining lifetime question: the ticket. The measurement says "one |
|
VERDICT: APPROVE at The user-visible fixA streamed reply that is assembled and then sent is not a stream: the client sees one chunk after the whole generation, which reads as "the server is slow" rather than "the server is streaming". That is the arm I would write first, because it is the only one that distinguishes "delivers in order" (which a batch-at-end implementation also satisfies) from "delivers as it goes". The arm that catches the hangA condition-variable The rest are the ordering properties (
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1dde6eb
The measurement is the right one for a streaming route, and it is stated as the failure it is: "Refs #1785 (F5: the bar is a time to first token, and on this route there was no such thing)" — a streamed reply that is sent after generation completes is not a slow stream, it is not a stream: every token arrives in one burst and the user watches a spinner for the whole generation. On the NPU route that is the difference between a usable chat surface and one that looks hung.
And it is measured on the device with the real container (FastFlowLM's Qwen3.5-2B, 16-token streaming request, one npu-hold ticket), so the number is the route's number rather than a host-side simulation — which matters because the whole point of F5's bar is what a user sees.
Two asks, both about making the fix checkable later:
- assert the pattern, not only the total time — a TTFT number passes if the reply is sent in two chunks (first token, then the rest), which is better than nothing and still not streaming. An arm that counts chunk arrivals (≥ N distinct events for an N-token reply) pins the property; the timing follows from it;
- record the TTFT before and after on the ticket, since F5's bar is a threshold and "was unbounded, now X ms" is the sentence the release note needs.
No other asks.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1dde6eb — A streamed reply sent as it is generated rather than buffered: for a chat front end this is the difference between a first token in milliseconds and one after the whole answer, so it is a user-visible correctness property, not an optimisation — and the forward_serve path is where it belongs rather than in a client.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 1dde6eb
F5's bar is a time to first token, and a route that sends the whole reply at the end has no TTFT to measure — so this is not a nicety, it is the precondition for the row. Fixing it before taking the figure is the right order, and it is the same discipline as #1953's drafter row (the tool mode shipped with the number) and #1949's GDN row (re-measure the premise first).
Three asks:
- Does the first token arrive before the model has finished the first chunk, or does this buffer one chunk and then stream? Those are different TTFTs and only the first satisfies F5. If the sampler emits per token, the flush point should be per token or per N tokens with N stated — say which, because "streamed" covers both and the bar does not.
- Does streaming preserve the stop condition from #2064? A turn end that is stripped after the fact is fine when the whole reply is assembled; streamed, the client has already seen
im_endarrive as text unless the stop check happens before the flush. That interaction is the one thing about this change that can regress a just-fixed bug, and it is worth one sentence in the body. - Is there a measured number now? TTFT before/after on the same container under one ticket is what the F5 row wants, and it is also what proves the change did anything: a streamed reply and a buffered reply have identical tok/s, so TTFT is the only observable that moves.
Refs #1785 rather than Fixes is right while the row's figure is still owed — and this PR is what makes that figure meaningful.
|
VERDICT: READY at Why this is the right shape
The two things that would make it reviewable rather than plausible
Nothing else — the |
|
VERDICT: READY at this head. The bite check is the property itself, which is the best kind: removing the restored → The before/after is the whole point of #1785 and it is measured on the device, same container, one ticket each: So time to first token went from the whole request to 1.87 s of a 10.26 s reply, and the frames after it are evenly spaced — which is the detail that proves they are being generated, not replayed: a burst at the end with the same total would have passed a "streaming works" test that only checked ordering. Three design decisions I rate, in order:
Two things I would add to the body. The |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1dde6eb — the before/after pair is a textbook measurement of a streaming defect, and the hang-up arm is the one that proves the cancellation actually works.
The fault was measured, not described: on main, all 14 chunks arrive at 9.668 s — chunk arrivals (s): [9.668, 9.668, …] — so the client's time-to-first-token was the whole request. After, [1.867, 2.809, 3.28, …, 10.2]: first chunk at 1.867 s of 10.258 s, spread over 7.789 s. Same container, same ticket, binary named. That is the difference between "streaming implemented" and "streaming measured".
The five PASS lines are the right five, and each closes a way this can be wrong:
- "the first chunk arrives after the prompt is done and within a few tokens of it" (
1867 ms,prompt_ms 1023) — a stream that starts before prefill is a lie about latency; - "the stream is well formed: event-stream, a finish reason (stop), a usage tail,
[DONE]— the terminal frame is the part clients block on, and a missing[DONE]hangs an SDK rather than erroring; - "the streamed reply equals the non-streamed one" — the strongest arm here: it makes streaming a transport change rather than a behaviour change, which is the only way it can be trusted for a decision route;
- *HTTP 400 for an over-context prompt and for a malformed field, not a stream — the error path has to decide before the headers go out, because after that the status line is already sent and an error can only be a frame;
- the hang-up arm: "left after the first chunk at 4.45s; the next request (2 tokens) returned in 2.78s" — so a client that walks away stops the generation instead of letting a 200-token answer finish into a dead socket. This is the arm that makes
is_connection_closed()meaningful, and it is the one most streaming implementations omit.
The code matches the arms on the property I care most about: the frame loop breaks on req.is_connection_closed() || (bridge && bridge->cancelled()), so the device work is bounded by the client's interest rather than by max_tokens. On this box that is not a nicety — an orphaned generation is the same class of resource burn as the 14 h llama-server.
Two things the body states that a reader would otherwise mis-infer, both appreciated: the first chunk lands ~0.8 s after prompt_ms because the answer splitter holds the opening markup (so nobody reads 1867 as prefill latency), and the <|im_end|> visible in the reply is #2064's fault, not this branch's — with the branch cut from main without it.
One question, non-blocking: what happens if the generation errors after the first frame — is there an error frame (or a finish_reason that says so), or does the stream just end? A client cannot tell "ended" from "broke" unless one of them is emitted, and the arms cover well-formed success and pre-stream errors but not the mid-stream failure. If it is already handled, a line saying which frame carries it closes the gap for good.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 1dde6eb — I built the bridge and its test at this head and ran them twice: once plainly, once under ThreadSanitizer, 40 consecutive runs, zero warnings. For a two-thread hand-off that is the check that matters, and it is clean.
$ g++ -O2 -std=c++17 -I app tests/stream_bridge_test.cpp -o sb -pthread && ./sb -> stream_bridge: ok
$ g++ -O1 -g -fsanitize=thread -I app … && for i in $(seq 1 40); do ./sbtsan; done -> 40/40 clean, 0 TSan warnings
The subtlest part is right, and I checked it rather than assuming: the worker writes *early while the handler thread reads it, and the code paths are ordered so that cannot race — wait_first() returns false only after the worker has called finish(), which is after early->status/set_content, and the handler then join()s before reading early. Taking the status from early only on the no-frame path, and pushing the error into the stream when started() is true ("a status cannot change once a stream has started"), is exactly the right split.
Four notes, none blocking.
- A streaming request now costs two threads, not one. Before, the generation ran on the connection thread; now it runs on its own, and the connection thread blocks in
next()for the length of the reply. Under httplib's pool that is2Nthreads andNin-flight generations forNconcurrent streams. That is the price of streaming correctly, not a defect — but it is a capacity change, and it belongs in the doc paragraph this PR adds, since the first thing a reader will ask is where the second thread comes from. push()'s bool is ignored at every call site. The header says it exists "so the producer can stop early"; the producer instead pollscancelled()once per token, which is the check that actually stops it. Both work, but a reader has to notice that the documented mechanism is unused — either use the return insend()or say in the header that the token loop'scancelled()is the stop, andpush's bool is for callers that push in a burst.wait_first()'s predicate ispushed_ > 0 || finished_—cancelled_is not in it. Harmless today (only the consumer cancels, and it cannot be insidewait_first()and callingcancel()at once), but if the releaser ever runs while the handler is still waiting — an aborted response settled from another thread —wait_first()would block until the producer finishes rather than returning. One clause in the header, orcancelled_in the predicate with a "false on cancel" meaning.res.status = early->status > 0 ? early->status : 500makes "the implementation never set a status" indistinguishable from "it failed". That is true on today's paths (a successful stream always pushes[DONE], so no-frame means failure), but a future stream route that legitimately pushes nothing would be answered 500. Settingearly->status = 200before the worker starts makes the fallback mean only what it says.
The measurement is the right one to have wanted: 14 chunks all arriving at 9.668 s of a 9.668 s reply is a client-side TTFT of the whole request, and this is the change that fixes the cause (the frames were built during generation and sent after, because httplib writes only after the handler returns) rather than the symptom. The non-streamed path is untouched — bridge == nullptr still collects into chunks and sends on return — so an ordinary request cannot regress.
Refs #1785 (F5: the bar is a time to first token, and on this route there was no such thing).
The fault, measured on the NPU
1bit serve --device npu --flashon main, FastFlowLM's Qwen3.5-2B container, a streaming chat request of 16 tokens, onenpu-holdticket:Every frame arrives when the reply is finished.
app/forward_serve.cppbuilt the SSE frames into a vector during the generation and sent them from the content provider afterwards (its own comment said so: "Run the forward here too, so it is not invoked from httplib's write path"). A client's time to first token was the whole request.The change
app/stream_bridge.h: a small producer/consumer hand-off (push, finish, wait for the first frame, next, cancel). Nothing in it knows about HTTP.handle_generateanswers a non-streaming request in place, as before. A streaming one runs the same generation code on its own thread, which pushes each frame as it exists; the connection thread waits for the first frame or the end and only then chooses between an ordinary response and a stream.data:frame.After, same container, binary built from this branch (
1dde6eb), one ticketThe first chunk comes about 0.8 s after
prompt_ms, not at it: the model's opening markup is held by the answer splitter until it can be classified, so the first visible piece is a token or two in. The box was at load 31 for this run; the times are for the shape, not for a speed claim.The
<|im_end|>in the reply above is the separate fault #2064 fixes; this branch is cut from main without it.Tests
tests/stream_bridge_test.cpp(cteststream_bridge, built in CI as a dependency ofnpu_sampling_test): a frame is readable when pushed and not when the producer finishes (timed); a producer that ends with no frame reports "no stream"; frames queued beforefinish()survive it and a push after it is dropped;cancel()releases a blocked reader and is seen by the producer; 20,000 frames cross in order with none lost.stream_bridge: ok.tools/copyright.py --checkrc 0;ci_targets_check.py --check"baseline 9, and none new". Docs:docs/npu.md, "Streaming" under "Serving".Limits and what I did not do
logprobsstreams go through the samesend()but were not exercised on the device.🤖 Generated with Claude Code