A panic on the model runtime thread permanently disables generation #12

New Issue

hugo · 2026-07-25T09:40:23Z

hugo commented

2026-07-25 09:40:23 +00:00

Symptom

GenerationService runs exactly one worker thread with no panic recovery and no restart. Any panic inside it — in the tokenizer, in metal.rs, in checkpoint I/O — permanently disables all generation for the rest of the process lifetime. The window stays open and responsive; every subsequent request just returns "The model runtime stopped unexpectedly." until the user quits and relaunches.

Evidence

src/runtime.rs:55-58:

thread::Builder::new()
    .name("model-runtime".into())
    .spawn(move || run(receiver, worker_metrics))

run() (src/runtime.rs:100-213) has no catch_unwind. When it unwinds, the Receiver<Command> drops, so GenerationService::generate fails at src/runtime.rs:89 and reports the message above forever. There is no supervisor and no respawn — GenerationService is created once in spawn_services and cloned.

There is no panic::catch_unwind or panic::set_hook anywhere in the codebase.

Reachable panic sites on that thread:

src/engine/metal.rs:1654, :1686, :1715, :1762, :1795, :1824 — state.indexer.as_ref().unwrap() and state.compression.as_ref().unwrap(), each guarded only by a branch condition computed earlier in the same function (ratio == 4, pos == 0, compressed_rows > shape.indexer_top_k). The invariants hold today; they are the kind that break silently during a refactor, and the surrounding code is already Result<_, String> so the unwraps are avoidable at zero cost.
src/engine/tokenizer.rs:569, :573, :620 — see the separate tokenizer issue.
Arithmetic overflow in debug builds anywhere on the path.

How ds4 handles this

The C reference does not have this failure mode, in either direction:

For engine invariant violations, ds4.c:1052 ds4_die() prints and exit(1)s — used in 251 places. A violated invariant kills the process outright. Blunt, but the user sees a message and the state is unambiguous.
For per-request failures, ds4_server.c keeps the worker alive and answers the client: prefill failures go through send_prefill_failure_response() (ds4_server.c:10495), decode failures set an err string and break out of the loop (ds4_server.c:11371+) leaving the slot workers running for the next request.
The worker pool itself is fixed and long-lived (ds4_server.c:13037, joined only at shutdown :13118).

So ds4 has exactly two outcomes: process dies loudly, or this request fails and the server continues. DS4Server has invented a third, worse one — the process lives, the UI looks fine, and generation is silently dead forever. That is the state to eliminate.

Suggested fix

Two parts, in order of value:

Make the worker survive a panic. Wrap the per-command body of run() in std::panic::catch_unwind(AssertUnwindSafe(...)). On Err, drop loaded (the Generator may be in an inconsistent state after a partial GPU command buffer), report GenerationEvent::Finished(Err("The model runtime hit an internal error and was reset.")) on that command's channel, record a metric, and continue the loop. The next request reloads the model.

Generator is not UnwindSafe (it holds NonNull GPU handles), hence AssertUnwindSafe — which is justified precisely because the recovery path drops it rather than reusing it. Say so in a comment.
Remove the avoidable panics. Convert the six unwrap()s in src/engine/metal.rs listed above into .ok_or_else(|| "...".to_owned())? — the enclosing functions already return Result<_, String>, so this is mechanical and turns a dead runtime into a failed request.

Not in scope: restarting the whole thread. Recovering in-loop is simpler and keeps GenerationService's Sender valid.

Acceptance criteria

A deliberately injected panic in the generation path (temporary panic!() behind a test-only flag, or a unit test calling the recovery wrapper directly) leaves the service usable: the triggering request gets an error, the next request succeeds after a model reload.
The six metal.rs unwrap()s are gone.
cargo clippy --all-targets --all-features -- -D warnings stays clean.

## Symptom `GenerationService` runs exactly one worker thread with no panic recovery and no restart. Any panic inside it — in the tokenizer, in `metal.rs`, in checkpoint I/O — permanently disables all generation for the rest of the process lifetime. The window stays open and responsive; every subsequent request just returns "The model runtime stopped unexpectedly." until the user quits and relaunches. ## Evidence `src/runtime.rs:55-58`: ```rust thread::Builder::new() .name("model-runtime".into()) .spawn(move || run(receiver, worker_metrics)) ``` `run()` (`src/runtime.rs:100-213`) has no `catch_unwind`. When it unwinds, the `Receiver<Command>` drops, so `GenerationService::generate` fails at `src/runtime.rs:89` and reports the message above forever. There is no supervisor and no respawn — `GenerationService` is created once in `spawn_services` and cloned. There is no `panic::catch_unwind` or `panic::set_hook` anywhere in the codebase. Reachable panic sites on that thread: - `src/engine/metal.rs:1654`, `:1686`, `:1715`, `:1762`, `:1795`, `:1824` — `state.indexer.as_ref().unwrap()` and `state.compression.as_ref().unwrap()`, each guarded only by a branch condition computed earlier in the same function (`ratio == 4`, `pos == 0`, `compressed_rows > shape.indexer_top_k`). The invariants hold today; they are the kind that break silently during a refactor, and the surrounding code is already `Result<_, String>` so the `unwrap`s are avoidable at zero cost. - `src/engine/tokenizer.rs:569`, `:573`, `:620` — see the separate tokenizer issue. - Arithmetic overflow in debug builds anywhere on the path. ## How ds4 handles this The C reference does not have this failure mode, in either direction: - For engine invariant violations, `ds4.c:1052` `ds4_die()` prints and `exit(1)`s — used in **251 places**. A violated invariant kills the process outright. Blunt, but the user sees a message and the state is unambiguous. - For per-request failures, `ds4_server.c` keeps the worker alive and answers the client: prefill failures go through `send_prefill_failure_response()` (`ds4_server.c:10495`), decode failures set an `err` string and `break` out of the loop (`ds4_server.c:11371+`) leaving the slot workers running for the next request. - The worker pool itself is fixed and long-lived (`ds4_server.c:13037`, joined only at shutdown `:13118`). So ds4 has exactly two outcomes: *process dies loudly*, or *this request fails and the server continues*. DS4Server has invented a third, worse one — *the process lives, the UI looks fine, and generation is silently dead forever*. That is the state to eliminate. ## Suggested fix Two parts, in order of value: 1. **Make the worker survive a panic.** Wrap the per-command body of `run()` in `std::panic::catch_unwind(AssertUnwindSafe(...))`. On `Err`, drop `loaded` (the `Generator` may be in an inconsistent state after a partial GPU command buffer), report `GenerationEvent::Finished(Err("The model runtime hit an internal error and was reset."))` on that command's channel, record a metric, and continue the loop. The next request reloads the model. `Generator` is not `UnwindSafe` (it holds `NonNull` GPU handles), hence `AssertUnwindSafe` — which is justified precisely *because* the recovery path drops it rather than reusing it. Say so in a comment. 2. **Remove the avoidable panics.** Convert the six `unwrap()`s in `src/engine/metal.rs` listed above into `.ok_or_else(|| "...".to_owned())?` — the enclosing functions already return `Result<_, String>`, so this is mechanical and turns a dead runtime into a failed request. Not in scope: restarting the whole thread. Recovering in-loop is simpler and keeps `GenerationService`'s `Sender` valid. ## Acceptance criteria - A deliberately injected panic in the generation path (temporary `panic!()` behind a test-only flag, or a unit test calling the recovery wrapper directly) leaves the service usable: the triggering request gets an error, the *next* request succeeds after a model reload. - The six `metal.rs` `unwrap()`s are gone. - `cargo clippy --all-targets --all-features -- -D warnings` stays clean.

hugo added the bug label 2026-07-25 09:40:23 +00:00

hugo commented

2026-07-25 11:00:43 +00:00

Fixed in 8f91a06. Each runtime command now runs behind a panic boundary; a panic fails that request, drops the loaded GPU generator, records failure/unload metrics, and leaves the worker alive for the next request. The six avoidable Metal state unwraps now return explicit errors. Added a recovery regression test that injects a panic and then succeeds on the next operation. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

Fixed in 8f91a06. Each runtime command now runs behind a panic boundary; a panic fails that request, drops the loaded GPU generator, records failure/unload metrics, and leaves the worker alive for the next request. The six avoidable Metal state unwraps now return explicit errors. Added a recovery regression test that injects a panic and then succeeds on the next operation. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

hugo closed this issue

2026-07-25 11:00:46 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hugo/DS4Server#12