[michaeltrefry]

sc-12959: thread-safe-eval — enforce the fork's threading model (eval lock, synced encoder maps, process-global default stream)

Shortcut story sc-12959 — closes the three hazards the sc-12937 thread-shared-streams.patch (PR #20) left open, making the fork's threading model coherent: streams visible everywhere, all evaluation serialized, O(devices) streams.

Why now (audit evidence)

A read-only audit of ~/Repos/inference confirmed both open questions the sc-12937 review raised:

• Eval safety is discipline-only today. Every MLX-facing contract trait is !Send + !Sync and the docs say "the worker serializes jobs per thread" — but nothing enforces it. Two independently-built engine instances on two threads can eval concurrently; with streams now globally visible that races a shared command encoder into a Metal SIGABRT instead of the old catchable exception.
• The encoder leak is structurally baked in. The !Send contract forces the worker to build-and-run each MLX model inside a spawn_blocking task; tokio blocking-pool threads die on idle, and each dead thread stranded one CommandEncoder + MTLCommandQueue in the global map for process lifetime.

The patch (mlx-sys/patches/thread-safe-eval.patch, 5th in the apply chain, required=true)

1. Process-global eval lock — detail::eval_mutex() (recursive, leaked against static-destruction races) taken for the body of eval_impl and mlx::core::synchronize(Stream). Every encode path funnels through these two (verified: item/tostring/save/gguf → eval(); async_eval → eval_impl; the mlx-c metal surface never touches encoders), so concurrent cross-thread eval now serializes instead of aborting. eval()'s host-side completion wait runs outside the lock; synchronize drains under it by design. Verified deadlock-free: Metal completion handlers and cpu StreamThread tasks never acquire the mutex; lock order eval → default-stream-storage → all_streams → encoder-map has no reverse edge.
2. Synchronized global encoder maps — shared_mutex per backend: shared lock on the fallback read in get_command_encoder (element reference bound inside the locked scope — references survive rehash, iterators don't), exclusive lock on registration and clear. Closes the read-during-rehash UB that every thread's first MLX op could trigger.
3. Process-global default stream — default_stream_storage is static (was thread_local) behind its own mutex. Stream/encoder count is now O(devices) instead of O(threads-that-ever-touched-MLX): the spawn_blocking churn leak is gone structurally. Safe because of (1). ThreadLocalStream keeps per-thread semantics; set_default_stream is now process-global (matches the fork's single-eval-thread contract).

Tests

Two new deterministic tests in mlx-tests/tests/test_cross_thread_stream.rs (now 6):

• test_default_stream_shared_across_threads — two threads see the same default stream index. Red without this patch (per-thread defaults yield distinct indices — this is the leak mechanism).
• test_concurrent_eval_from_many_threads — 8 threads × 25 evals contending on the shared default stream with per-thread result assertions; exercises the eval lock in the worst case (all threads on one stream).

Verification

• Clean from-scratch FetchContent rebuild: all 5 patches apply in order; thread-safe-eval.patch also applies standalone to pristine v0.32.0 (contexts are pure upstream) and passes the reverse-apply idempotency check.
• Full suite cargo test --all --no-fail-fast -- --test-threads=1: 972 passed / 0 failed. cargo fmt --all --check and cargo clippy -- -D warnings clean.
• Adversarial review (fresh-context subagent): verdict FIX-FIRST → all actionable findings fixed (iterator-deref-after-unlock UB sliver, stale git rebase state, overstated lock-scope comment, mutex static-destruction hardening). Accepted-as-documented: recursive_mutex is insurance (no re-entry path exists today); metal clear_streams remains teardown-only (no callers); the cuda backend got no mutex treatment (unreachable in this Metal-only fork).

🤖 Generated with Claude Code