server: tell a streaming client when its slot is parked and restored - #190
server: tell a streaming client when its slot is parked and restored#190danielhanchen wants to merge 25 commits into
Conversation
A slot parked by the preemption path produces nothing until its cells come back, and to a client that is indistinguishable from a hung server: the stream goes silent, read timeouts fire, and a chat that was merely waiting for room is torn down as broken. Push a small out-of-band result to the task's response queue when a streaming slot is parked and when it is restored. The HTTP layer writes it as an SSE comment, ": preempted" and ": resumed", which is legal SSE that every existing client ignores, so the body of the response is unchanged by preemption. While parked the ping runs every 2 s as ": preempt-keepalive" regardless of --sse-ping, so proxies and client read timeouts survive a wait that is long by design. Notices that arrive before the first real result (a slot parked while it was still processing its prompt) are sent in front of it. Non-streaming requests see nothing. Harness test: forced parks every 8 tokens on /completion and /v1/chat/completions carry the comments in park/resume order and generate the same tokens as the unparked run; a non-streaming request is untouched; two streams that overflow the pool together both finish and the parked one says so.
47767ad to
65f8f54
Compare
…is kept alive Found by putting #190 and #192 together and then looking for the keepalive that #190 promises. It never arrives. Live, the 4B on one B200, two streaming completions that do not fit together so one is parked until the other finishes, every SSE line timestamped as it arrives: 6.19s B : preempted 16.82s B : resumed A 10.63 s silence on a stream whose whole point is that it says ": preempt- keepalive" every 2 s. Four-chat runs at -c 8192 and -c 4096 show the same: parks of up to 14.59 s by the server's own "resumed after" line, and not one keepalive on any stream in any run. server_response::send() notify_all()s a single condition variable for every result of every task, and server_response::recv_with_timeout() waited with wait_for(), which restarts on every wakeup. A reader waiting on a task that is producing nothing is therefore woken by every token every other task produces, and its wait_for() never elapses. On a server with any traffic at all the timeout is not a timeout: whoever waits for a quiet task waits indefinitely. A parked slot is the worst possible case for this, because a slot is only ever parked while the others are busy, so the keepalive was unreachable by construction. The same applies to the ordinary --sse-ping, which likewise only fired on an otherwise idle server, and to the should_stop polling in server_response_reader::next(), whose own comment says it happens every polling_interval_seconds and did not. Compute the deadline once and wait_until() it. Spurious wakeups then re-check the queue and go back to waiting for the same instant, which is what every caller already reads the argument as meaning. After, the same two streams: 6.26s B : preempted 9.26s B : preempt-keepalive 12.26s B : preempt-keepalive 14.26s B : preempt-keepalive 16.75s B : resumed Three keepalives across a 10.49 s park, at the 2 s period plus the reader's 1 s polling granularity. The probe is scripts/integ_keepalive_probe.py. No harness test: stories260K generates several hundred tokens a second, and at the context sizes the harness uses a park lasts two or three seconds, which is the keepalive period itself. Every sizing I tried either parked for milliseconds at a time as the pool oscillated around full, or did not park at all. A test that straddles the period it is testing would be worse than none, so the regression is pinned by the live probe above.
|
One more commit,
Measured with two streaming completions on the 4B at Cost is none measurable: nothing extra happens per result, and on the branch this was found on the same four-chat control gives 139.6 tok/s before the change and 139.2 after. There is no harness test for it, because at the context sizes the test model runs at a park lasts about as long as the keepalive period itself, and a test straddling the period it is testing would be worse than none, so this is pinned by the probe above. |
The last resort parks through the same call as the planner but did not send the stream comment the planner sends, so a client that shows the pause from that comment showed nothing for a slot parked this way.
|
Merged #184 up to 86845c1 (the retry ladder's last resort) and added one line for this branch: the last resort parks through the same call as the planner but did not send the stream comment the planner sends, so Studio, which shows the pause from that comment, showed nothing for a slot parked that way. Found by a four-chat 35B GUI drive with the planner off: four of four finished, nine resumes, zero pauses shown. With the notice wired the same drive shows the pauses. Suites: 9 preemption and 4 notify, 13 passed; on the merged #197 binary the same drive shows the pause on 4 of 4 chats, 12 parks and 12 resumes, 0 errors. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A request with several prompts streams them through one reader, and one flag stood for all of them: a prompt resuming cleared it while another was still parked, which switched the parked keepalive off for the one still waiting. The reader now keeps the set of parked prompts and runs the keepalive while any of them is parked. The comment names the prompt it is about for prompts after the first; prompt 0 keeps the bare form a single-prompt client matches on.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…; the rotation park is announced A request parked while still processing its prompt has no token to send. The route used to hold the response until the first ordinary result, so the client saw nothing, not even the headers, until the slot resumed, and the parked keepalive never ran. The stream now starts on the first notice; the data-less signal a prompt sends before its first token is skipped once the stream is open, since it has nothing to add. The head-of-line rotation parks a resident through the same preempt_save() as any other park but never announced it, so that stream stayed silent while parked and later carried an unmatched resume. It is announced now. Two tests: the notice is the first thing on the wire and arrives while the other stream still runs; both streams of a rotation carry paired notices.
…eused slot before it is sized or parked
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…s and waiting children
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…at lets the head in
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…round; a rotation counts the head's bytes as leaving
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…eemption; the rotation holds both states at once
…s from the prefix it keeps
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ed for While parked the keepalive ran every two seconds whatever --sse-ping said, which lengthened the silence for a client that had asked for a ping every second, exactly while nothing else was coming. The parked interval is now the shorter of the two.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… resident cycling past the rotation delay
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ntext; the recurrent flag assigned on every load
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…at/server-side-preemption-notify
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A slot just given a task has not yet passed the prompt checks the STARTED block runs, and the planner could park it first. The park notice opens the stream, so a prompt the checks reject came back as HTTP 200 with an in-stream error where the non-stream 4xx belongs. The checks are one helper now, slot_prompt_rejected(), run by the STARTED block as before and asked by the planner before a started slot can be chosen: a request about to be errored is never given a notice ahead of its error.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90a5094d3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (first_result != nullptr && dynamic_cast<server_task_result_preempt_notice*>(first_result.get()) != nullptr) { | ||
| // [TAG_PREEMPT] parked before any token exists. The stream starts now, with the | ||
| // notice, so the parked keepalive runs through the wait instead of the client | ||
| // seeing nothing until the slot resumes; the first ordinary result follows in | ||
| // the stream, an error included, since the response has already begun. |
There was a problem hiding this comment.
When a streaming request contains multiple prompts, this opens the HTTP 200 response as soon as any task's preemption notice arrives, before checking whether another task has produced an initial error. For example, with an oversized prompt first and a valid shorter prompt second under KV pressure, the oversized task can remain the leader while the valid task is parked; its notice is dequeued here before the oversized-prompt error, converting the expected non-stream 4xx into a 200 response with an in-stream error. Buffer notices until the first ordinary result/error for every task has been resolved, or prevalidate all prompts before allowing any notice to start the response.
Useful? React with 👍 / 👎.
1 participant
Stacked on #184. Only the last commit is new; the rest is the swap branch.
Summary
A streaming client is told when its slot is parked and when it is restored, as SSE comments (
: preempted,: resumed), with a: preempt-keepaliveevery 2 s while parked. Comments are legal SSE and invisible to every existing client, so the body is unchanged by preemption. Non-streaming requests see nothing. A request parked before its first token starts its stream on the notice, so the headers and the keepalive go out through the wait instead of after it.Without this a parked slot is a silent connection: the client cannot tell "waiting for cells" from "slow" or "dead", and any client-side read timeout or proxy idle timeout fires during a wait that is long by design. Unsloth Studio uses the comments to show a paused line on the chat that was parked and to stand its own preemption down when the server can do it.
Policy
Unchanged. The notice is emitted at the two park sites (the KV-full victim and the
LLAMA_SERVER_PREEMPT_EVERYtest knob) and at the restore site, through aserver_task_result_preempt_noticepushed to the task's result queue for streaming tasks only. Notices that precede the first result (a slot parked while still processing its prompt) are sent in front of it. While parked the ping period is 2 s regardless of--sse-ping.Results
Raw stream under
LLAMA_SERVER_PREEMPT_EVERY=64: park and resume pairs every 64 tokens, body identical to the unparked run. Through Unsloth Studio on Qwen3.5-4B with two MTP drafts at-c 8192and four slots: four concurrent chats 4 of 4 with the pause shown in the GUI on every chat that was parked, zero errors.Exactness
The generated tokens with forced parks equal the unparked run token for token (harness test), and through Studio both 1200 token prompts are byte-identical across 18 parks each (seed 0, temperature 0).
Cost when it does not fire
None. No notice is created unless a slot is parked, and the keepalive is the existing
--sse-pingpath with one extra branch.Tests
tools/server/tests/unit/test_preempt_notify.py: comments arrive in order and the body is unchanged on/completion, the same on/v1/chat/completions, a non-streaming request is untouched, two overflowing streams both finish with the parked one saying so, a stream parked before its first token starts with the notice and the notice arrives while the other stream still runs, and a resident rotated out for a parked head announces it with every notice paired. 17 passed together with the existing preemption tests.Limitations
No keepalive before the first result, so a park during prompt processing is covered by the client's first-token deadline rather than the 2 s ping.
Follow-up commit
The keepalive promised above had never fired. The server's result queue notifies one condition variable for every result of every task, and the reader's timed wait restarted on each wakeup, so on any busy server the timeout never elapsed; the same wait serves the ordinary
--sse-ping. The wait now targets a fixed deadline. Paired on this branch against an isolated build of the previous head, a 13.9 s park carried zero keepalives before and four after, spaced at the 2 s period plus the reader's 1 s polling granularity, none outside the park.Carried from #184 since: a pure recurrent cache is served without preemption (no notices either, nothing is parked), and a rotation refused for the RAM cap is logged once per park; the head's notices are unchanged, it is restored when a resident finishes.
Also from #184: a started slot's reservation counts from the prefix it keeps, as its used count already did.
While parked, a shorter ping interval the request asked for is kept: the parked keepalive is the shorter of the configured interval and the forced two seconds.
From #184 again: a started slot is counted by the cells it holds, and trimmed to the prefix its request keeps when nothing fits.
A started slot is validated before it can be parked. The prompt checks of the STARTED block (logits without memory, a prompt past the physical batch or the context) are one helper now, run there as before and asked by the planner before a started slot can be chosen, so a request the checks would reject is never given a park notice ahead of its error: the notice opens the stream, and the client would have got 200 plus an in-stream error where the non-stream 4xx belongs.