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.
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:1052ds4_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.
## 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
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Symptom
GenerationServiceruns exactly one worker thread with no panic recovery and no restart. Any panic inside it — in the tokenizer, inmetal.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:run()(src/runtime.rs:100-213) has nocatch_unwind. When it unwinds, theReceiver<Command>drops, soGenerationService::generatefails atsrc/runtime.rs:89and reports the message above forever. There is no supervisor and no respawn —GenerationServiceis created once inspawn_servicesand cloned.There is no
panic::catch_unwindorpanic::set_hookanywhere in the codebase.Reachable panic sites on that thread:
src/engine/metal.rs:1654,:1686,:1715,:1762,:1795,:1824—state.indexer.as_ref().unwrap()andstate.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 alreadyResult<_, String>so theunwraps are avoidable at zero cost.src/engine/tokenizer.rs:569,:573,:620— see the separate tokenizer issue.How ds4 handles this
The C reference does not have this failure mode, in either direction:
ds4.c:1052ds4_die()prints andexit(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.ds4_server.ckeeps the worker alive and answers the client: prefill failures go throughsend_prefill_failure_response()(ds4_server.c:10495), decode failures set anerrstring andbreakout of the loop (ds4_server.c:11371+) leaving the slot workers running for the next request.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()instd::panic::catch_unwind(AssertUnwindSafe(...)). OnErr, droploaded(theGeneratormay be in an inconsistent state after a partial GPU command buffer), reportGenerationEvent::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.Generatoris notUnwindSafe(it holdsNonNullGPU handles), henceAssertUnwindSafe— 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 insrc/engine/metal.rslisted above into.ok_or_else(|| "...".to_owned())?— the enclosing functions already returnResult<_, 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'sSendervalid.Acceptance criteria
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.metal.rsunwrap()s are gone.cargo clippy --all-targets --all-features -- -D warningsstays clean.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.