Repository navigation
fix: release mlx thread state at thread exit; lift the mlx pin - #30
Merged
Merged
Conversation
The 0.32.1 segfault pinned in the previous fix is upstream working-as-intended: mlx PR #4248 removed automatic per-thread cleanup, and ml-explore/mlx#4327 (closed 2026-08-18) states the contract — every thread that touched mlx must call mx.clear_streams() before exiting. An exiting thread that skips it takes the whole process down in the dyld TLS finalizer (CompileCache teardown reaching _Py_Dealloc without the GIL). sous's per-generation threads die after every turn, which is why the daemon crashed mid-task. release_mlx_thread_state() now runs in a finally at every mlx-touching thread boundary: the generation thread (after the result hand-off, so the caller is never delayed), the server_status memory probe, and the auto-context memory snapshot. The mlx ceiling is lifted — compliance with the contract is what lets it float. Verified live on mlx 0.32.1 (0-for-2 before the fix): three consecutive delegated tasks done/completed, no new crash reports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds explicit MLX thread-state cleanup to prevent daemon crashes and removes the MLX 0.32.1 ceiling.
Changes:
- Introduces
release_mlx_thread_state(). - Adds cleanup to generation and memory-probe paths.
- Relaxes the MLX dependency constraint and adds regression tests/documentation.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CLAUDE.md |
Documents the MLX thread cleanup requirement. |
pyproject.toml |
Removes the MLX upper bound. |
uv.lock |
Synchronizes dependency metadata. |
src/sous/engine/base.py |
Adds the cleanup helper. |
src/sous/worker.py |
Cleans generation-thread state. |
src/sous/server.py |
Cleans status-probe state. |
src/sous/context.py |
Cleans memory-probe state. |
tests/test_worker.py |
Tests generation cleanup. |
tests/test_server.py |
Tests status cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review round: the worker-loop thread also touches mlx (model load, idle unload) and exits on the non-signal shutdown path while the process is tearing down — same ml-explore/mlx#4327 segfault shape. An outer finally releases on the way out. The reviewer's companion suggestion to join the worker from main() is deliberately NOT taken: the shutdown handler documents that the worker is never joined (a task can run for minutes), and the ordinary SIGTERM path os._exits before any join could run. The helper itself was only ever exercised through monkeypatched call sites, so an API typo inside it would be swallowed by its own except while every test stayed green. Two unit tests now drive it against a fake mlx.core: clear_streams is actually reached, and a raising clear_streams never propagates out of a dying thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
krcm0209
added a commit
that referenced
this pull request
Aug 18, 2026
Bumps `pyproject.toml`, `server.json`, and `server.json`/`packages[0]` to **0.3.0** (all three, because CI's tag gate compares the tag against each and a mismatch is only caught *after* the tag exists), plus `uv.lock`. **0.3.0, not 0.2.2:** the line renames the MCP-facing tool, which is breaking on the wire, and adds five features. Pre-1.0, a breaking change rides a minor bump. ## What's in it **Discovery — sous is now selected without the user naming it** - #22 — server-level MCP `instructions`: the one part of the surface clients place in the model's context unconditionally, even where tool schemas are deferred to names-only. Carries when to delegate and why, under Claude Code's 2KB truncation limit (test-enforced). - #23 — `delegate_task` → **`delegate_to_local_model`**, so the name alone carries its trigger.⚠️ breaking on the wire. **Delegation economics and friction** (both calibrated from live runs, not theory) - #24 — the netting rule (a short spec must yield a large diff), trust-the-worker spec guidance, `uv run <tool>` allowlist defaults, worker `PATH` resolved from the user's login shell (so allowlisted verify commands resolve under launchd), and the new **`sous wait <task-id>`** command — blocks until a task finishes or needs approval, so agents stop tight-polling or reading `tasks.db` by hand. - #26 — lean post-review output: a clean diff earns a brief acceptance; misses get a narrower self-contained follow-up. - #31 — plan-execution guidance: batch adjacent mechanical plan tasks, keep judgment tasks inline, and block on `sous wait` in the foreground when the next step needs the result. **Auto-sized context** - #25 — `[context] mode = "auto"` sizes the worker's window **per task** from live memory headroom (the tighter of the Metal working-set ceiling and available system RAM × `fraction`), clamped to `[min_tokens, the model's native max]`. Per-token KV cost is read from the model's own config, counting only full-attention layers on hybrid architectures. Fails safe to the fixed window with a warning; every report records the window and the reason. Default stays `fixed`. **Fixes** - #29 / #30 — mlx ≥ 0.32.1 requires every thread that touched mlx to call `mx.clear_streams()` before exiting ([ml-explore/mlx#4327](ml-explore/mlx#4327)); without it the daemon segfaults mid-task. sous now complies at every mlx-touching thread boundary, and the temporary `mlx<0.32.1` ceiling from #29 is **lifted**. ## Upgrade notes - Clients see `delegate_to_local_model`; anything hard-coding the old tool name needs updating (MCP clients discover tools dynamically, so configuration is unaffected). - No plist regeneration needed — `PATH` resolution happens in the daemon. - `[context] mode` defaults to `fixed`; auto sizing is opt-in. ## Verification All four checks green locally (282 passed / `ty` / `ruff` + format / `uv lock --check`). `uv build` produces `sous_mcp-0.3.0`, and CI's tag-gate script run locally against `v0.3.0` agrees across all three declared versions: ``` tag=0.3.0 pyproject=0.3.0 server.json=0.3.0 server.json/packages=0.3.0 TAG-CHECK WOULD PASS ``` Beyond the checks, this line has been exercised end to end on real hardware today: delegated tasks completing on the merged build with auto-sized windows (~190k–262k tokens), zero approval round-trips, and a plan-execution run where a fresh session delegated the mechanical tasks unprompted. **No tag pushed** — tagging triggers the PyPI publish, so that stays a maintainer action: merge this, then `git tag v0.3.0 && git push origin v0.3.0`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Why
The mlx 0.32.1 segfault pinned in #29 turned out to be upstream working-as-intended, not a bug they'll fix: mlx PR #4248 removed automatic per-thread cleanup, and the maintainers' contract (ml-explore/mlx#4327, closed this morning) is that every thread that touched mlx must call
mx.clear_streams()before it exits. sous violates that in three places: the per-generation thread in_generate_with_timeout(dies after every turn — this is what killed the daemon mid-task), and the MCP worker threads running_mlx_memory_gb/context._live_memory.What
engine.base.release_mlx_thread_state()— callsmx.clear_streams()(present in 0.32.0 too), swallows everything (cleanup must never raise out of a dying thread), no-op without mlx.finallyat all three sites; in the generation thread it runs after the result-queue put so the waiting caller is never delayed by cleanup.mlx>=0.31,<0.32.1→mlx>=0.31, with the comment now documenting the contract that allows it to float.Verification
TDD: two new tests (RED first) pin that the release runs once per generation in the generation thread, and on the
server_statuspath. 279 passing + all four checks.Live soak on the previously-crashing version (
mlx 0.32.1 / mlx-vlm 0.6.14, resolver's own pick after the unpin): 3 consecutive delegated tasks, alldone/completed, interleaved with status calls, zero new crash reports — against 0-for-2 before the fix on the same version.Supersedes the #29 ceiling; no upstream issue filed (it would duplicate the closed-by-design #4327).
🤖 Generated with Claude Code