Repository navigation
[API server][Security] Keep worker and decoder error text out of client responses - #646
Conversation
Automated reviewThe PR adds Findings: none. I checked these paths:
The description gives test commands and results, and says what was not tested and why. Nothing is missing for a change of this size. Advisory, not a merge gate. Rules: |
8c60efa to
0e11f83
Compare
merceod
left a comment
There was a problem hiding this comment.
Checked 8c60efa served on klone and read 0e11f83 (the rebase over #362 gives register_failure a walk argument used in the log line, adds the KV retrieve error text and quoted paths to the scrub, and adjusts the cosmos3 test stub; the served paths are unchanged, not re-served). Injected worker and decoder errors come back as internal error (error id ...) on all five surfaces (SSE, NDJSON, websocket, non-streamed, bridge), ValueError paths are cut to base names, and all 21 ids a client received are in the server log. The ordinary 400s (bad JSON, missing model, bad sample rate) keep their text.
One thing I checked because it looked wrong on the first head: /generate/ws and the bridge turned the server's own HTTPExceptions ("Request timed out", a fatal 503) into the generic text. The current heads keep #519's HTTPException branch first and the timeout frame is {"error":"Request timed out","status":500}, the same as main. Fine as is.
Outside this PR: chat finish_reason is hard-coded to "stop", so a max_tokens cut reports "stop", and a streamed /generate cut by --timeout ends without an error line, on both arms.
Live test: [API server][Security] Keep worker and decoder error text out of client responsesWorks: base sends the client the upload path and server user name for audio decode errors; the PR sends an error id and logs the full text under it. 1x H200 per server, Qwen3.5 0.8B and Whisper large-v3, Python and Rust frontends, SHM transport · PR head Claims
Other
|
|
Thanks for the served check.
Yes, that branch stays first on purpose: an HTTPException is the server's own message with its own status, so it goes out unchanged, and only other exceptions get the error id.
Confirmed on main and filed as #709. In the code, No code change since the approval. |
|
Thanks for the run. The current head is the retested one, so the rows above still hold.
No code change since the approval. |
0e11f83 to
39aa7f7
Compare
39aa7f7 to
a7ac147
Compare
|
@Gaurav-Shah05 could you take a look when you get a chance, and merge if it looks good? @merceod approved it and CI's green. it keeps worker and decoder error text out of client responses. |
a7ac147 to
141060f
Compare
|
@merceod heads up, since your approval the OpenAI route catch-alls and the conductor's step-failure text go through the same error-id scrubbing too, and a stray ValueError on speech is now a 400 like the other routes. mind another look? |
merceod
left a comment
There was a problem hiding this comment.
Approve again on 141060f. Since my last look the error-id policy reached the OpenAI routes and the conductor. All six catch-alls go through _exception_error (router.py lines 66 to 87), an HTTPException keeps its status and detail, a ValueError or TypeError is a 400 with paths cut to base names, anything else a 500 with an error id and the full error in the log, a class with its own status_code keeps it, and _fail_after_error sends the error-id text too. The new cases in test_client_error_text.py, test_conductor_tick_exception.py and the speech-status case fail on the head I approved and pass here, 194 over the ten touched files.
Two behaviour changes worth a line in the description. A ValueError or TypeError on /v1/audio/speech was a 500 and is a 400 now, and other 500s on the OpenAI routes show only an error id. Held for a second approval as before, and it conflicts with main in conductor.py and worker.py, so a rebase is due.
One helper, mstar.utils.client_errors.client_error_text, turns an exception into the text a client sees: a ValueError or TypeError keeps its message with absolute paths cut to base names, anything else becomes "internal error (error id <id>)". The full error and traceback go to the server log under that id. Used by the data worker's _fail_request, the worker's ingest and main-loop failures, the engine's per-request stage failures, the conductor's ingest failures, the /generate and /generate/ws catch-alls and the Rust-frontend bridge. Fixes mstar-project#592
A failed KV retrieve put its exception text, server path included, into the admit refusal clients see; it now gets the same error id as other internal failures. A quoted absolute path is now cut to its base name whole, so directories with spaces or brackets no longer leak.
…o it has mstar-project#471's drain state
The OpenAI routes' catch-alls sent str(e) for any error a handler raised, so a transcription whose frontend decode failed could hand the client the decoder's message with the upload path. They now go through client_error_text: an HTTPException keeps its detail, a ValueError or TypeError is a 400 with paths cut to base names, and anything else is a 500 with an error id and the full error in the server log. The conductor's per-request failure text after an exception gets the same treatment.
mstar-project#832's test pinned the conductor's ingest RuntimeError text in the client message. It is a model bug, so the client now gets an error id and the test finds the reason in the log under it.
mstar-project#419 fails a request whose input or output tensor read failed. The output side goes through the data worker's _fail_request, which now sends an error id; the worker's input side sent the read's exception text, so it gets an error id too. Both log one line without a traceback, since the tensor manager already logged the read's error where it failed.
141060f to
016d546
Compare
|
@Gaurav-Shah05 rebased on main, CI's green and merceod's approval stands. would you mind merging when you get a chance? @merceod FYI one commit on top since your approval: #419's worker input-read failure sent the raw error text to the client, so it now goes through the same error id, w/ #419's test asserting it. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Gaurav-Shah05
left a comment
There was a problem hiding this comment.
The log change can come in a follow-up if you'd rather merge this first.
| status, detail = getattr(e, "status_code", None), getattr(e, "detail", None) | ||
| if isinstance(status, int) and detail is not None: | ||
| return _error(status, str(detail)) | ||
| text, _ = client_error_text(e, f"{route} request failed", log=logger, type_name=False) |
There was a problem hiding this comment.
client_error_text logs every exception at ERROR with its traceback, so every 400 for a bad request, such as a chat request that lists tools, now writes a traceback to the log where main wrote nothing. The 400's text doesn't carry the error id, so I think _exception_error should log only the 500s.
Conflicts: - mstar/model/base.py: kept this branch's ClientChunk; dropped PrefixStream, which main replaced with KVLabel/PrefixKeys (#832/#834). - mstar/api_server/data_worker.py: imports take ClientChunk plus main's PrefixKeys/keyed_labels; this branch's _load_media refactor kept with main's _prefix_streams -> _keyed_labels rename; _finish_reading drains when the rid is already draining (this branch) or still has a failed read's transfers in flight (main #419), docstring merged. - mstar/api_server/entrypoint.py: PendingRequest takes main's output_totals (#844), which counts outputs as they are accepted; this branch's `delivered` counter and _count_outputs, the same fix for the same leak, are deleted. Stream drain keeps this branch's last_activity read with the swap-out; the done path clears req.chunks as main does. /generate/ws: main's empty-id-as-None and check_request_id (#618) move into the receive loop beside this branch's isinstance check, since live-call routing reads the id there; the error reply keeps this branch's call/turn release with main's client_error_text scrubbing (#646), under this branch's `payload` name. - mstar/conductor/conductor.py: imports from both; worker graphs from main's validate_deployment (#640), then this branch's _find_client_streams. - test/modular/test_kv_prefix_request_keys.py: kept this branch's get_partition_topology stub and main's kv_labels. Adapted to main (no conflict markers): - test/modular/test_client_stream_api.py: stub renamed prefix_key_streams -> kv_labels; the tensor-manager fake gains take_failed_reads; the bad-append test expects #646's "internal error" text in place of the model's exception; test_a_streaming_request_lets_go_of_the_chunks_it_has_delivered dropped, as main's test_api_result_delivery.py covers the release and the totals. - test/modular/test_request_id_limit.py: the fake server gains takes_stream_input, submission_key and abort_submission, which this branch's /generate/ws reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failure forwarding a client chunk (conductor) or feeding a ws append (API server) sent the raw exception text to the client. Route both through #646's client_error_text like main's other error sites: a ValueError keeps its (path-scrubbed) message and the 400, anything else reaches the client as an internal error with an error id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (full duplex 3/6) (#593) * Client streams: live input from the client into a running request A model can declare a stream the client feeds while its request runs (Connection(from_partition=CLIENT, ...)). /generate/ws opens a live call, takes append and end_input messages for it, and aborts a closed socket's calls; the preprocess thread turns each append into stream items with Model.process_stream_chunk and sends CLIENT_STREAM_INPUT to the conductor, which routes the chunks to the consuming partition's worker and holds them for a call still waiting for a slot. The preprocess queue stays FIFO, as on main: a pass takes up to MAX_APPENDS_PER_PASS items in arrival order and stops at the first new request. Before the thread processes a queued opening or append it asks the API server whether that submission is still pending (APIServer.is_pending); if not, it drops the item unsent and deletes an append's files. An abort that lands after the check goes out behind what the thread just sent, from the same thread and outbox, and the conductor tears the request down as usual. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: an append carries text, files and its own model_kwargs An append over /generate/ws was files only, grouped by modality, and the model hook saw the opening's model_kwargs. It now mirrors the opening: - Wire: {"request_id", "append": {"text"?, "files"?: [{"name", "data", "stream"?}], "model_kwargs"?}}. "append": [files] stays valid as short for {"files": [files]}, so existing clients need no change. Routing is still by the top-level shape; what a record holds never opens a request, and text, files or model_kwargs beside the record are refused. Null fields inside the record count as absent, the input budget counts append text as well as files, and refusals answer as before. - A file's "stream" names the client stream it is for; a name the model doesn't have is refused (400). - Model.process_stream_chunk(chunk: ClientChunk) -> NameToTensorList, where ClientChunk carries text, the loaded tensors and their metadata, each file's target stream, the append's model_kwargs and the opening's. - An append with text that yields no stream item is refused (400) rather than dropped, so a text-only append to a model that takes no text is an error the client sees. Docs: docs/clients.rst (both forms), docs/adding_models.rst (the hook), docs/serving.rst (the budget counts append text). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: several per partition, and end_input can name them - The conductor's startup rule that a partition takes at most one client stream is gone. Its reason (chunks and ends sharing one connection state) no longer holds: connection state is keyed per (from, to, edge). Each client stream still needs exactly one consumer. - end_input may be true (every client stream of the call) or a list of client stream names (only those), on an append or on the opening message. ClientAppend.ends and ClientStreamInput.ended (a typed list[str], so the wire codec needs no pickle) carry the streams that end. The API server tracks each call's open streams: an append naming a stream already ended is answered as late, a name the model doesn't have is refused (400), and the call takes input until every stream has ended. - The conductor ends only the named connections, with per-edge producer_done / producer_done_items; _send_producer_done takes the edges to end, every edge between the two partitions by default. - Worker._process_new_inputs ends only the edges producer_done_items counts, or every stream from the producer when it counts none. Every producer_done sender is the conductor's _send_producer_done, and a partition's own end counts each edge into the consumer (0 items included), so partition-to-partition streams end as before. The one difference: two consumer partitions colocated on one worker and fed by one producer no longer have each other's streams ended, with a count of 0, by the other's signal. - Rust parity: the Rust graph runtime only encodes worker-to-worker INPUT_SIGNALS, always with an empty producer_done and no producer_done_items (frames.rs), and nothing in rust/ reads either field or ClientStreamInput, so there is nothing to change there. Docs: docs/clients.rst (end_input lists), docs/adding_models.rst (several client streams per partition). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: per-stream limits, enforced by the client-stream layer A client Connection may declare ClientStreamLimits (mstar.streaming.topology). Each is optional, and a deployment may set one the model leaves open or lower one it declares under the serving config's client_streams: {<edge>: {...}}, but not raise or lift it: a declared bound is what the model sized its consumer for (nemotron_duplex sizes its session buffers and KV pools from max_session_s). - max_items: what the stream may carry over a call's life. Each item counts item_size(item) (1 by default), so a stream of variable-length chunks can count its frames or samples. The data worker charges every item process_prompt or process_stream_chunk returns for the stream, before storing it, and an item past the limit fails the call with a 400 that names the limit. The counts are per-request state, dropped with the rest of the request's. - max_pending_items: items the consumer has been sent and not consumed, from the conductor's token_count and the consumer's consumed_count (reported each step, so a step late). Past it, pending_overflow "refuse" (the default) ends the call with a 400 naming the limit; "drop_oldest" holds the newest item in the conductor until the consumer reports room, replacing (and freeing at once) an older one waiting there; the stream's end sends the waiting item first. The conductor decides for every rank, so a lockstep consumer sees one sequence of items. No per-chunk host sync and no new per-chunk message: the data worker counts tensors it already holds, and the conductor compares counters it already keeps. max_call_input_mb stays the byte budget for a call's uploads. _remove_request takes a failure status, so a refused call gets its 400. Docs: docs/serving.rst (client_streams), docs/adding_models.rst (declaring limits), docs/clients.rst (the 400). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: refuse a max_pending_items below the stream's chunk window at load A consumer held to fewer pending items than its chunk policy waits for never forms a chunk, so it never reports consumption: drop_oldest held the call forever and refuse failed every call. client_stream_limits now fails loading when max_pending_items is below the widest window the stream's policy ever waits for, naming the stream and both numbers. ChunkPolicy.max_window_size() gives that window over the stream's life (window_size() is only the next chunk's): the window itself for fixed policies, chunk + left_context for LeftContextChunkPolicy, the capped size for RampChunkPolicy (None when it grows without a cap), and the steady-state window for ScheduledLeftContextChunkPolicy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: answer items routed to an ended stream as late; check the opening's streams An append whose files name no stream reached the model's process_stream_chunk, which could route them to a client stream the client had already ended: the data worker charged them to max_items and the conductor dropped them silently. The data worker now keeps each call's ended streams (it sees every end, in order) and drops items for them before charging or sending them; the append is answered as late through ClientAppend.on_late, which the /generate/ws handler sets to send its late reply from the socket's loop. A file's stream on the opening message was ignored without a check. It is now checked as an append's is (submit_request's file_streams): a stream the model lacks gets a 400. process_prompt still routes the opening's files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Worker: drop the uncounted branch of a per-edge producer_done Every producer_done comes from the conductor's _send_producer_done, which counts each edge it ends (0 items included), so the branch that closed every stream from a producer when the signal counted none could not run. The worker now ends exactly the edges a signal counts. The tests that sent an uncounted end now send the conductor's shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: rename pending_overflow drop_oldest to keep_newest Past max_pending_items the policy sends the items that fit, holds the newest of the rest in one slot (replacing and freeing any item held there) and drops those in between: it keeps the newest, it does not drop the oldest. Renamed in the code, config validation, docs and tests; drop_oldest is now an unknown value that fails loading. docs/serving.rst describes the policy precisely and notes that max_pending_items counts items, not max_items' units. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Docs: say what an append's text and model_kwargs actually do Only an append of text alone that yields no stream item is refused; text sent with files reaches process_stream_chunk, and is dropped without an error if the model ignores it (the data worker can't tell which items it yielded). An append's model_kwargs reach only process_stream_chunk, so the example no longer sets a sampling temperature per append. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams tests: match on_late, main's preprocess thread, and the close's abort - test_generate_ws: stream_input now also takes the append's on_late; the test checks it is a callable rather than expecting the old keyword set. - test_client_stream_api: the fake model declares no prefix-key streams, which main's PreprocessWorkerThread reads when a model_config is set. - The late-append test checks the call is not aborted when the late reply goes out; the socket's close after it is what aborts the call, by design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams test: close the socket after the call finishes, not when its last append lands The record-and-shorthand test closed its socket once the sixth call reached the fake server. Under load the call had not finished by then, so the close aborted it, as a close does, and the abort showed up as an extra call. It now waits for the call's finish reply, and checks that reply is all the socket sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: report paced consumption while the consumer's loop runs The conductor sizes a client's max_pending_items room from the consumer's consumed count, which only reached it on WORKER_GRAPHS_DONE. A consumer reading its client stream inside a dynamic loop (nemotron_duplex's encode_loop) sends that only when the loop ends, so a well-behaved live call was refused (or, under keep_newest, starved) once it had sent max_pending_items items in total. The worker now sends a CLIENT_STREAM_CONSUMED message after a step whose consumed count moved, for client streams that have a max_pending_items bound only, and skips rids whose WORKER_GRAPHS_DONE already carried the count this step. One columnar message per step (request ids, edge names, running counts), from the lockstep leader only; the count last reported lives on the StreamBuffer, so there is no new handle-keyed state. Models without a paced client stream pay one check per step. The send sits in Worker._stream_and_send, above the graph runtime, so the Python and Rust runtimes behave the same and the WORKER_GRAPHS_DONE frame Rust builds is unchanged. The conductor folds the report in through the same path as WORKER_GRAPHS_DONE's counts, and sends a keep_newest item held for room. Tests drive a real graph runtime (Python, and Rust when built) through a looped consumer and the real Worker._stream_and_send into the real Conductor: a call that sends three times its limit while being consumed is not refused, and a consumer that stops reading still gets its client refused. The existing limit tests now use the per-step report rather than a WORKER_GRAPHS_DONE that finished the worker graph on every step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * generate/ws: an append waits on the call it named; a refused message passes its turn first Two fixes to the merge of main's ordered submission (#787) with live calls: - An append task looked up live[request_id] when it ran, not when it was read, so an append queued behind a refused opening raised KeyError once the opening's task had removed the call (no reply), and one queued across a call's end could feed a new call reusing the id. Each append now waits on the call it named when it was read. - A refused message passed its turn only after sending its error, which waits on the socket: a client not reading yet held every later message (test_refusals_never_hold_up_reading[failed-call-append]). The turn is now released before the send, as the call's future already was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Client streams: refuse a live call at max_concurrent_requests; drop max_pending_items At the cap, a live call held up to 256 appends (20 s at 80 ms) until it got a slot, so it started, and stayed, that far behind its client. The opening now carries its own client input on NEW_REQUEST (its first chunk and the streams it ends), so the conductor can tell a live call from a one-shot when it arrives: a call that leaves any stream open gets a 503, and a one-shot queues as before. Nothing is held for a queued request any more, so the held-chunk map and its cap go. No model or config sets max_pending_items, so refuse, keep_newest, the held waiting item, the per-step CLIENT_STREAM_CONSUMED reports and the chunk policies' max_window_size check go until a model needs a bound on pending input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * R1.1: ack an unbuffered client stream under the wire request id ack_unread_tensors takes the wire id since #557 and puts it in TensorReceived; passing the worker-local handle named the wrong (or no) request on the producer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * R1.2: report client-input and append failures through client_error_text A failure forwarding a client chunk (conductor) or feeding a ws append (API server) sent the raw exception text to the client. Route both through #646's client_error_text like main's other error sites: a ValueError keeps its (path-scrubbed) message and the 400, anything else reaches the client as an internal error with an error id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * R1.3: check client-stream settings in validate_deployment A bad client_streams: block, max_call_input_mb or client-stream declaration was raised alone, by the conductor, the API server's init or the data worker's thread, before or beside #640's collected report. check_client_streams (mstar/validation/client_streams.py) now returns them as problems from validate_deployment. Conductor._find_client_streams, client_stream_limits and _call_input_limit only read; the API server and data worker read theirs on first use, after the conductor has accepted the deployment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * P3b: nemotron_duplex configs: a live call past the cap gets a 503 Since d566acd a call with open input is refused at max_concurrent_requests; only one-shot requests queue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * R2.2: refuse a client's bad ws append with a warning, not an error A ValueError from an append is the client's fault: answer it with its message, paths scrubbed, and log one WARNING line, as the conductor's ingest path does. Anything else still goes through client_error_text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * R3.1: log the ws append ValueError refusal at INFO, like the sibling refusals Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test fakes: the fields #593's API server and conductor now read - test_client_error_text.py: _FailingServer gains takes_stream_input, submission_key and abort_submission, which /generate/ws reads. - test_whisper_kv_sizing.py: the conductor's fake request body gains client_input=None, which _ingest_request reads. - api_server_helpers.py: make_test_api_server sets _model_config (None) instead of writing max_call_input_bytes, now a cached_property of it that a real server leaves unset until first use (test_api_server_helpers.py compares the two field sets). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Since #646 an engine failure that is not a ValueError or TypeError shows the client only an error id, with the reason logged under it. The context-cap and MTP greedy-only refusals in prepare_inputs are RuntimeErrors, backstops behind process_prompt's 400s, so the two tests that read the reason from failed_requests now find an error id there and the reason in the log.
…r with an error id Since #646 every route handler goes through _exception_error: a ValueError or TypeError keeps its message with server paths cut to base names, and any other error shows the client only an error id, with the full error logged under it. /v1/messages, /v1/messages/count_tokens and /v1/responses still sent the exception's own text, so they now take the same path and join #646's route tests.
A ref_audio storage failure is now main's generic 500 with an error id (mstar-project#646), so its test checks for that and that no server path leaks.
Main's #646 made every route answer a failure through _exception_error: server paths cut from a 400's text, a 500 naming only an error id. The LoRA admin routes kept their own copy of that mapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #592
What does this PR do?
Worker, engine and decoder exceptions reached API clients verbatim. That is a security risk (#592): any client, with no credentials (#506), sending a bad upload or hitting a server fault could read server paths, the server's user name and arena segment names with pids, and one engine error could show a client text about another request in its batch. Now deliberate refusals keep their message with paths cut to base names; anything else becomes an error id, and the server log keeps the full error under it.
client_error_text(exc, what) -> (text, error_id)andscrub_pathsinmstar/utils/client_errors.py: one place for client error text; /v1/realtime:languageandpromptskip validation, and a failed step echoes them whole into the error event and the log #499 and Harden remote media fetch when MSTAR_ALLOW_REMOTE=1 (follow-up to #411) #424 can reuse it.data_worker.py): decoder errors named the upload; status stays 400/500.worker.py,engine.py): worker ingest and main loop, and engine per-request stages;Error in worker:carried arena and tensor-file paths. A failed tensor read ([Mooncake][Worker] Fail only the request whose async tensor read failed, at TP>1 too #419) gets an error id on both the worker and the data worker side.conductor.py): ingest failures, the same leak one hop earlier (ValueError stays a 400), and a request failed after a conductor step raised./generateand/generate/wscatch-alls inentrypoint.pyand the six OpenAI route catch-alls inopenai/router.pyechoedstr(e); a transcription decoded in the frontend (a header past the model's window) could send the decoder's message with the upload path. Deliberate messages are unchanged. The Rust frontend bridge inrust_frontend.pysentrepr(e); the Rust code itself only relays that text, plus its own refusals and I/O errors, which name no path.How was it tested?
On 016d546 (head, rebased on main 6430016, umask 002):
ruff check .is clean. CPU Core selection: 6,941 passed, 879 skipped, 4 failed; the same 4 fail on main 6430016 here (6,903 passed), because this venv lacksprometheus_clientand LingBot'sftfy. TP lockstep sim: 11 passed. The rebase kept #419's one-line log for a failed tensor read (client_error_texttakestraceback=Falsefor it) and sends an error id for that read on the worker side too; the GPU runs below are from before the rebase.python -m pytest test/modular/test_client_error_text.py(new): 37 passed (31 fail on main; 14 fail on the branch before the OpenAI route change). The changed test files (test_conductor_tick_exception.py,test_openai_router.py,test_request_failure_propagation.py,test_preplan_error_paths.py,test_teardown_barrier.py,test_ingest_rollback.py,test_kv_layout_refusals.py): 131 passed, 3 skipped.python -m pytest test/rust/against a releasemstar-serverbuilt from main (this PR changes no Rust): 463 passed, 5 skipped, 1 failed (test_runtime_lockstep.py::test_only_the_tp_leader_counts_new_tokens[1], which fails on main here too).Each OpenAI route gets a handler error carrying an upload path and a fake API key: the client sees only an error id, the log has both. Hostile transcriptions (random bytes, a truncated header, headers claiming 4 h and 600 s over a 1 s body, a 40 s FLAC with noise in the middle) get the engine's answer or, for the FLAC that fails in the frontend decode, a 500 with an error id; no path, no upload left, RSS within 6 MiB.
e2e matrix on one H200, main against this PR (
e2e-matrix run --base main --head 08d6077dd --models whisper_large --rp --parallel 1): no regressions, 28/28 request-path cases the same.e2e matrix table
e2e matrix details and chart
No regressions: 3 rows across 1 models, every one starts, keeps its outputs, and stays within 5% (or within noise) at c=1 and c=8.
Served through
mstar serveon one H200 per model, base and head back to back on the same VM (order alternated per model), 3 measured passes per point, 1 server session(s) per arm. Deltas are signed so that positive means head is better (lower latency, higher throughput).Measured on 08d6077dd against main 294cbab; the commits since add #807, which both arms then have, and a test-only change. The request-path cases include garbage audio, an empty file, a 513 MiB upload and malformed multipart.
Per-row numbers (mean +- std over passes), correctness detail, extra metrics
Startup, peak VRAM, errors
Startup uses warm compile caches shared by both arms, so the first arm can pay compiles the second reuses; it is reported, not judged.
Setup
82375ad11), head: 08d6077ddad9dc95d2fa74c9e18300301620f299 (08d6077dd)e2e-h200-4)Not tested
The Rust frontend served on a GPU: the matrix's only Rust frontend model is Qwen3-Omni (
qwen3_omni_rustfe), which is large and has no request-path cases. Its error text comes from the Python bridge, whichtest_the_rust_frontend_bridge_answers_with_an_error_idcovers.Not here: remote-fetch errors (#424) and the
/v1/realtimeserver_errorecho (#499) will useclient_error_text.Depends on and merge order
Depends on: none.
git merge-treeagainst #618, #617, #645 and #807 is clean for each pair, and the four still open merged onto main 82375ad (#807 has since merged) pass the CPU Core selection (6,854 passed, 845 skipped, the same 4 environment failures),test/rust/(464 passed, 5 skipped) andcargo test --releaseinrust/server(75 passed).With #617: its upload-dir refusal (
PermissionError, which names the directory) reaches clients through these catch-alls as an error id on the Python side; the Rust frontend still sends that text in its 500.Design
ValueErrororTypeError, the rule the data worker already used to pick 400 over 500, so [Model] Cap the decoded size of request images and videos #510's media refusals and model validation messages read as before; only absolute paths change, to base names (a directory path becomes<path>).HTTPException's status and detail as before. Anything else goes through the helper:ValueError/TypeErroris a 400, an exception class that names its ownstatus_codekeeps it (AudioTooLongErrorstays a 413), and the rest is a 500 with an error id. Speech used to answer a strayValueErrorwith a 500; it is now a 400 like the other routes./generate,/generate/ws, the OpenAI routes and the bridge passtype_name=False, so their kept messages stay byte-identical to main. The data worker, worker and conductor keep their<stage> failed: <Type>: <message>shape.<what> [error id <id>]: <Type>: <message>with the traceback, so the engine's three per-stagelogger.exceptioncalls moved intoregister_failurerather than logging twice./generatekeeps status 500 for aValueErrorfromsubmit_request, as on main. Scheduler admit refusals carry the resource's own message; the one that held an exception's text, a failed KV retrieve (its message named the snapshot path), now carries the error id instead (kv/manager.py).Tests in detail
test_client_error_text.py: a real PIL decoder failure on an upload under a per-user dir, a deliberate decoderValueErrornaming that upload, an arena I/O error, a worker main-loop exception, a per-request stage failure,/generate,/generate/wsand the Rust bridge each answer with no/tmp,/dev/shm,mstar_arenaor user name, and the server log holds the full error (under the client's error id for the generic ones). Plus a guard that/generatekeeps a deliberate message byte-identical, and 8scrub_pathscases (URLs, ratios and relative paths untouched; a quoted path is cut whole, so directories with spaces or brackets do not leak). A failed KV retrieve reaches the admit refusal as an error id, with the path only in the log.RuntimeErrorthat carries an upload path and a fake API key answers 500 with only an error id (the log has both), and aValueErrornaming the upload answers 400 with the base name. A transcription with a 40 s corrupt FLAC reaches the frontend decode (the test counts the call), whose fallback raises with the saved upload's path: the client gets an error id and the upload is deleted. Five hostile uploads get a clean answer with bounded RSS, and anAudioTooLongErrorkeeps its 413.test_conductor_tick_exception.py: the two cases that pinnedConductor error ...: RuntimeError: <text>now expect the error id, and one of them finds the text in the log under that id.ValueErrorkeeps its 400 without the path, and the speech test'sOSErrorcase expects an error id instead of theOSErrortext.git checkout upstream/main -- <the source files>, remove the new helper, run, restore. Of the 37 cases intest_client_error_text.py, the 14 route and frontend-decode cases fail on the previous head (with the other leaks fixed), and the 4 other hostile-upload cases pass there too: they guard the paths that already answered cleanly.Checklist
ruff check .passesAI assistance: written with AI coding agent