Repository navigation
fix(server): make audio synthesis panic-safe in release via panic=unwind with explicit core-thread abort - #383
Merged
Conversation
The release profile set `panic = "abort"`, which defeats the `catch_unwind` request-isolation backstop PR #374 added to the audio worker (`run_guarded` in src/server/audio_worker.rs). Under abort a synthesis panic does not unwind, so the boundary never runs and the whole server process aborts instead of the offending request degrading to a per-request error. The same setting silenced the pipeline stage-isolation `catch_unwind` in production. Flip `[profile.release]` to `panic = "unwind"` so those deliberate containment boundaries work in the shipped binary as they already do under `cargo test` (the test harness always unwinds). The unwinding-table size and runtime cost are negligible for an MLX-linked inference server whose decode hot path does not panic. Unwind alone would let a panic on a core generation thread silently kill that thread and leave the server alive but unable to generate, so re-impose fail-fast explicitly where it is wanted: add `run_core_thread_or_abort`, a narrow `catch_unwind` + `process::abort()` wrapper that logs without request content, and wrap the two core generation worker threads (batched and legacy sequential) in src/server/model_worker.rs. The batch scheduler, the DiffusionGemma batch-1 loop, and the disaggregated serving-role loop all run inside those two thread bodies, so the two wrappers cover every core generation path. The audio worker `run_guarded` and the pipeline stage boundaries deliberately contain panics and are not wrapped. There is no global abort panic hook on purpose: a hook runs before unwinding and would defeat those backstops. The default panic hook (prints panic + backtrace) is kept. An MLX C++ FFI exception still becomes `std::terminate` rather than a Rust panic and terminates the process; that residual vector is unchanged here and tracked as issue #382. Adds a happy-path unit test for `run_core_thread_or_abort` (the abort path terminates the test runner and is verified manually), documents that the existing `worker_survives_engine_panic_and_keeps_serving` test now represents release behavior, and records the decision and rejected options in docs/adr/0003.
inureyes
added
type:security
Release `panic = "unwind"` (issue #375) made a panic on the remote pipeline stage service thread (`RemoteStageService::run` in src/distributed/pipeline/remote_service.rs) unwind and silently kill the thread while `serve_remote_pipeline_stage` stays parked on `ctrl_c`, leaving a zombie stage process a supervisor will not restart. Under the previous `panic = "abort"` such a panic aborted the stage cleanly, so the unwind flip was a fail-fast regression for a core inference thread that this PR had not covered. Relocate `run_core_thread_or_abort` from src/server/model_worker.rs to a shared crate-level module (src/worker_failfast.rs) reachable from both the server workers and the distributed pipeline, generalizing it to forward the body's return value so a recoverable `Err` propagates unchanged and only a panic aborts. Wrap the remote stage service thread with it; the `JoinHandle<Result<()>>` type and the `shutdown()` join handling are unchanged on the non-panic path. The local batched and legacy `--no-batch` workers keep their existing wrapping, no global panic hook is added, and the audio worker `run_guarded` containment is untouched. Correct the ADR 0003, the Cargo.toml profile comment, and architecture.md, which claimed the pipeline stage "stays contained" with its own `catch_unwind`. There is no production `catch_unwind` under src/distributed/ (the only one is `#[cfg(test)]`). The audio worker is now described as the only deliberately contained boundary, all core inference threads (batched, legacy, and remote stage) fail-fast explicitly through `run_core_thread_or_abort`, and pipeline faults are handled at the coordinator by `Result` propagation plus stage timeout and health probing. The MLX FFI `std::terminate` residual (#382) and the always-unwind `cargo test` notes are kept.
The existing happy-path test exercises the void return case. This adds a test with a Result-returning body that returns Err, confirming the wrapper forwards a recoverable error unchanged and does not abort.
Member
Author
PR FinalizationDocumentationAll four docs verified against the final design:
No em dashes in any of the new content. No stale or drifted phrasing found. Tests
Lint / Format
running 17 tests test result: ok. 17 passed; 0 failed; 0 ignored; 0 measured; 3532 filtered out; finished in 0.02s: 17 passed.running 7 tests test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 3542 filtered out; finished in 0.92s: 7 passed. |
be-free-snowboy
pushed a commit
to freestylejs/snowbrain-mlxcel
that referenced
this pull request
Oct 27, 2026
…ind with explicit core-thread abort (lablup#383) * fix(server): make audio synthesis panic-safe in release via unwind The release profile set `panic = "abort"`, which defeats the `catch_unwind` request-isolation backstop PR lablup#374 added to the audio worker (`run_guarded` in src/server/audio_worker.rs). Under abort a synthesis panic does not unwind, so the boundary never runs and the whole server process aborts instead of the offending request degrading to a per-request error. The same setting silenced the pipeline stage-isolation `catch_unwind` in production. Flip `[profile.release]` to `panic = "unwind"` so those deliberate containment boundaries work in the shipped binary as they already do under `cargo test` (the test harness always unwinds). The unwinding-table size and runtime cost are negligible for an MLX-linked inference server whose decode hot path does not panic. Unwind alone would let a panic on a core generation thread silently kill that thread and leave the server alive but unable to generate, so re-impose fail-fast explicitly where it is wanted: add `run_core_thread_or_abort`, a narrow `catch_unwind` + `process::abort()` wrapper that logs without request content, and wrap the two core generation worker threads (batched and legacy sequential) in src/server/model_worker.rs. The batch scheduler, the DiffusionGemma batch-1 loop, and the disaggregated serving-role loop all run inside those two thread bodies, so the two wrappers cover every core generation path. The audio worker `run_guarded` and the pipeline stage boundaries deliberately contain panics and are not wrapped. There is no global abort panic hook on purpose: a hook runs before unwinding and would defeat those backstops. The default panic hook (prints panic + backtrace) is kept. An MLX C++ FFI exception still becomes `std::terminate` rather than a Rust panic and terminates the process; that residual vector is unchanged here and tracked as issue lablup#382. Adds a happy-path unit test for `run_core_thread_or_abort` (the abort path terminates the test runner and is verified manually), documents that the existing `worker_survives_engine_panic_and_keeps_serving` test now represents release behavior, and records the decision and rejected options in docs/adr/0003. * fix(server): wrap remote pipeline stage thread for fail-fast parity Release `panic = "unwind"` (issue lablup#375) made a panic on the remote pipeline stage service thread (`RemoteStageService::run` in src/distributed/pipeline/remote_service.rs) unwind and silently kill the thread while `serve_remote_pipeline_stage` stays parked on `ctrl_c`, leaving a zombie stage process a supervisor will not restart. Under the previous `panic = "abort"` such a panic aborted the stage cleanly, so the unwind flip was a fail-fast regression for a core inference thread that this PR had not covered. Relocate `run_core_thread_or_abort` from src/server/model_worker.rs to a shared crate-level module (src/worker_failfast.rs) reachable from both the server workers and the distributed pipeline, generalizing it to forward the body's return value so a recoverable `Err` propagates unchanged and only a panic aborts. Wrap the remote stage service thread with it; the `JoinHandle<Result<()>>` type and the `shutdown()` join handling are unchanged on the non-panic path. The local batched and legacy `--no-batch` workers keep their existing wrapping, no global panic hook is added, and the audio worker `run_guarded` containment is untouched. Correct the ADR 0003, the Cargo.toml profile comment, and architecture.md, which claimed the pipeline stage "stays contained" with its own `catch_unwind`. There is no production `catch_unwind` under src/distributed/ (the only one is `#[cfg(test)]`). The audio worker is now described as the only deliberately contained boundary, all core inference threads (batched, legacy, and remote stage) fail-fast explicitly through `run_core_thread_or_abort`, and pipeline faults are handled at the coordinator by `Result` propagation plus stage timeout and health probing. The MLX FFI `std::terminate` residual (lablup#382) and the always-unwind `cargo test` notes are kept. * test(worker): add Err-forwarding test for run_core_thread_or_abort The existing happy-path test exercises the void return case. This adds a test with a Result-returning body that returns Err, confirming the wrapper forwards a recoverable error unchanged and does not abort.
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.
Summary
Release builds set
[profile.release] panic = "abort", which defeats thecatch_unwindrequest-isolation backstop PR #374 added to the audio worker (run_guarded). Under abort a synthesis panic does not unwind, so a single bad/v1/audio/speechrequest aborts the whole server instead of degrading to a per-request error. This flips the release profile topanic = "unwind"so that backstop (and the pipeline stage-isolationcatch_unwind) work in the shipped binary, and re-imposes fail-fast explicitly on the core generation worker threads so a broken core invariant still aborts cleanly for a supervised restart.What changed
Cargo.toml:[profile.release]panic = "abort"becomespanic = "unwind", with a comment explaining the decision. This is the only profile that setspanic; no member crate overrides it.src/server/model_worker.rs: addsrun_core_thread_or_abort, a narrowcatch_unwind+std::process::abort()wrapper that logs without request content, and wraps the two core generation worker thread bodies (the batched worker and the legacy--no-batchsequential worker). The batch scheduler, the DiffusionGemma batch-1 loop, and the disaggregated serving-role loop all run inside those two thread bodies, so the two wrappers cover every core generation path. The wrapped-body diff is large because rustfmt re-indents the moved blocks;git diff --ignore-all-spaceshows the logical change is just the helper plus the two wrappers.src/server/model_worker_tests.rs: adds a happy-path unit test forrun_core_thread_or_abort(a non-panicking body runs to completion and its side effect is observed). The abort path terminates the test runner and is documented as manually/subprocess-verified rather than unit-tested in-process.src/server/audio_worker.rs: adds a comment onworker_survives_engine_panic_and_keeps_servingnoting it now represents release-build behavior, because release uses unwind to match the always-unwind test profile. The test is kept unchanged.docs/adr/0003-release-panic-unwind-with-core-thread-abort.md(new) anddocs/adr/README.md: record the decision, the rejected options (keep abort and document only; full Kokoro-to-Resultrefactor as disproportionate), the no-global-abort-hook trap, thatcargo testalways unwinds, and the residual MLX-FFIstd::terminatevector.docs/audio-api.mdanddocs/architecture.md: short notes that synthesis panics are now contained as per-request errors in release, core generation threads abort on panic by design, and MLX FFI exceptions still terminate.Design notes / traps respected
std::panic::set_hookthat aborts: the hook runs before unwinding and would kill the audio workerrun_guardedand the pipeline stage containment this change exists to restore. Only targeted per-threadcatch_unwind+abortwrappers are used. The default panic hook (prints panic + backtrace) is kept.run_guarded, the pipeline runtimecatch_unwind, and Tokio async request handlers are deliberately left contained and are not wrapped.AssertUnwindSafein the wrapper is sound because the wrapper aborts (never continues) on a caught panic, so no torn state is observed.Residual abort vector
An MLX C++ FFI exception thrown through a non-
Resultcxxop becomesstd::terminate, not a Rust panic, so neither thecatch_unwindbackstops nor the unwind policy intercept it. This change does nothing for that path; it was filed separately as #382 and is referenced in the ADR.Thread spawn audit
Production long-lived threads found (
thread::spawn/thread::Builder), and the disposition of each:src/server/model_worker.rs:188(batched model worker, core generation): wrapped.src/server/model_worker.rs:714(legacy sequential model worker, core generation): wrapped.src/server/audio_worker.rs:165(audio worker): left contained; it has its ownrun_guardedcatch_unwindrequest isolation, which is exactly the boundary this change restores.src/distributed/pipeline/remote_service.rs:597(remote pipeline stage service): left contained as a pipeline stage-isolation boundary (outside thesrc/server/src/lib/mlxcel-core/srcaudit scope; a stage fault is contained to its request/connection).src/server/diffusion_worker.rsspawns no thread of its own;run_diffusion_worker_loopruns on the model-worker thread, so it is already covered by the two wrappers. The remainingthread::spawnhits in the tree are#[cfg(test)]stubs and test helpers.Test plan
cargo check --lib --testscargo check --binscargo test --lib server::audio_worker(7 passed, includingworker_survives_engine_panic_and_keeps_serving)cargo test --lib run_core_thread_or_abort(1 passed)cargo clippy --lib --tests -- -D warnings(clean)cargo fmt --check(clean)python3 scripts/ci/check_cross_repo_refs.py(OK)Closes #375