Repository navigation
fix(server): release the resident reranker or embedder before loading its replacement, then drain MLX's cache - #133
Merged
Merged
Conversation
… its replacement, then drain MLX's cache Partially addresses #130. ensureRerankerLoaded and ensureEmbedderLoaded loaded the new engine and only then dropped the old one, so both models were resident for the duration of the load. Measured on a real server: a second Qwen3-Reranker-4B (bf16) took the peak to 14.98 GB against 7.66 GB for one copy; with the 0.6B 4-bit checkpoint a swap peaked at 639 MB where one copy is 320 MB. Both functions now release the current engine first and drain MLX's buffer cache through a new EngineMemory helper (the server keeps no MLX import), then load. Same probe after the change: the peak stays at 591 MB across the swaps, and after the last one the cache holds 5 MB instead of 932 MB. A failed load now leaves no engine resident; the next request for the previous model reloads it. The engines are still outside ModelPool's byte budget, which is the remaining half of #130.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1f2c9c5. Configure here.
| if embeddingEngine != nil { | ||
| embeddingEngine = nil | ||
| EngineMemory.releaseCachedBuffers() | ||
| } |
There was a problem hiding this comment.
Swap 500s concurrent same-model requests
Medium Severity
Releasing embeddingEngine or rerankEngine before the replacement is assigned opens a nil window. A concurrent same-model request can pass the already-loaded check on a local current handle, then the handler re-reads the now-nil property and returns 500 load_failed for a model that was already resident.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1f2c9c5. Configure here.
…ding the slot Bugbot on #133: the already-loaded check awaits current.loadedModel, which lets the actor interleave another request's swap; that swap now empties the slot before its replacement is assigned, so a same-model request that passed the check could re-read the slot, find nil and answer 500 for a model it had just verified resident. The old order had the mirror image of the race: the re-read could pick up a different model's freshly assigned engine and score against the wrong model. ensureEmbedderLoaded and ensureRerankerLoaded now return the engine for this request (the verified current one, or the one they loaded) and the handlers use that instance; ensureEmbedderOr404 carries it in an EmbedderSwap result. The returned reference also keeps the model alive for the duration of the request if a concurrent swap drops it from the slot, so a response always comes from the model that was asked for.
…k, test the swap order on real checkpoints Second review round on #133. - The three handlers acquire the generation lock before the cold swap, as the chat model's handler always has (SRV-2). Swaps now serialize with each other and with generation: a request arriving during a swap waits instead of loading its own copy or holding a reference to the engine being replaced, and two swaps to different models can no longer load at once. Every early return after the acquire releases the lock. - Gated smoke (ms-marco under two ids, then Qwen3-Reranker-0.6B-4bit, then back): the swap's peak stays below one and a half copies where the old order peaked near two, and after the swap back fewer than 64 MB remain in MLX's cache where the undrained path parks about 600 MB. - CHANGELOG: the Changed entry states what was measured (serial requests; the 4B run was not repeated) and gains a Fixed entry for the wrong-model race the first round closed, present since v0.5.3 (embedder) and v0.9.0 (cross-encoder). - EngineMemory is internal; its comment no longer claims MLX never trims its cache. The POOL-5 analogy names what the pool has on top. The misplaced ensureEmbedderLoaded doc comment is back on its function. - No drain on a failed load: a load that fails before touching MLX must not be the first thing to initialize Metal, which the SPM test job cannot do.
…ract of auxiliary swaps Third review round on #133. - /v1/embeddings resolves the model and checks its kind before taking the generation lock, as the reranker path already did, so an unknown model (404) or a chat model (400) is answered at once even while a generation runs; ensureEmbedderLoaded takes the resolved target and only swaps. The cosine branch of /v1/rerank reuses the target it had already resolved. One loadFailureResponse maps a failed swap for all three handlers. - A failed load drains MLX's cache only when this call had just released a resident engine, which proves Metal is up; a load that fails before touching MLX still never initializes it. - Three pure tests pin the lock contract: a swap on /v1/rerank and on /v1/embeddings queues while a generation holds the lock and releases it after a failed load; the embeddings 404 and 400 do not wait for the lock. Four mutations each turn the expected test red (swap moved before the acquire, either failure path leaking the lock, the kind gate moved under the lock). A regression here is a server-wide deadlock. - CHANGELOG: a chat request on the server's single-stream path is what waits behind a swap; the swap runs under the lock as the chat model's does.
# Conflicts: # CHANGELOG.md # MacMLXCore/Tests/MacMLXCoreTests/Engine/RerankEngineSmokeTests.swift # MacMLXCore/Tests/MacMLXCoreTests/Server/HummingbirdServerEmbeddingsTests.swift
…e and the NLI smoke
…; tidy two formatting nits The third lock-held failure path of /v1/rerank (an .embedder model whose load fails) now has the same queue-then-release test as the other two; a missing release there would deadlock every later generation. Also drops a doubled blank line and re-wraps one long CHANGELOG line.
magicnight
added a commit
that referenced
this pull request
Oct 4, 2026
… call, read the lock state in tests Review of the first round found that taking the lock before the load also put a model's first download under it: a multi-GB speech model fetched on first use would have stalled every chat, embeddings and rerank request for minutes, with nothing in the logs saying why. The handlers now fetch the files first (AudioEngine.prepareSTT/prepareTTS, upstream's own resolve-or- download step, files only) outside the lock, and only load and run under it. A repo the Hub cannot deliver fails with 500 load_failed before the lock is touched. Concurrent fetches of one repo share a single download, because upstream clears a directory it finds incomplete and two downloads would delete each other's files. The app's view models cancel a superseded task and start the next one at once, so the superseded call can still be inside the engine; the GUI's AudioServicing now goes through the model-bound transcribe(model:) and synthesize(model:) too, and the slot-reading transcribe(audioURL:) and synthesize(text:) are gone. A swap drains MLX's buffer cache between the two models, as the embedder and reranker swaps do (#130), which the "same gate" comment claimed and the code did not do. The SpeechModelBox doc now says what actually serializes synthesis (the generation lock on the server; nothing in the app yet). The Metal CI job runs the suite in parallel on a slow runner, where a request round trip took close to 20 s and two "does not wait for the lock" tests (one from #133) failed on a fixed 2 s bound. The tests now read the server's lock state directly instead of racing an acquire against a timer, and the bounded waits are 60 s with the lock held throughout, so only a real wait-for-lock regression can fail them.
This was referenced Oct 4, 2026
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.


Partially addresses #130 (the cheap half; the pool-budget half stays open).
ensureRerankerLoadedandensureEmbedderLoadedloaded the replacement engine and only then let the old one go, so both models were resident for the duration of the load. Both now release the current engine first, drain MLX's buffer cache through a newEngineMemory.releaseCachedBuffers()(soHummingbirdServer.swiftkeeps noMLXimport), then load. A failed load therefore leaves no reranker or embedder resident; the next request for the previous model reloads it.Measured
Same probe as in #130 (real
HummingbirdServer,Memory.snapshot()after each/v1/rerankcall), two model ids pointing at the sameQwen3-Reranker-0.6B-4bitweights, then a swap to ms-marco:The 639 MB peak is the two 320 MB copies side by side; after the change the same serial probe never rises above its first request's 591 MB high-water mark. The 4B bf16 numbers from #130 (7.66 GB for one copy, 14.98 GB during a swap, 15.5 GB left in the cache) show the old order at scale; the 4B run was not repeated after the change.
Review rounds
ensureEmbedderLoaded/ensureRerankerLoadedreturn the engine for the request and the handlers never re-read the slot. This also closes the mirror racemainhad since v0.5.3 (embedder) and v0.9.0 (cross-encoder): a re-read after another request's swap finished could pick up the other model's engine and answer under the requested model's name. Recorded as a Fixed entry.Third round
swift test): a swap on/v1/rerankand on/v1/embeddingsqueues while a generation holds the lock and releases it after a failed load; the embeddings 404 and 400 do not wait for the lock. Four mutations each turn the expected test red: swap moved before the acquire, either failure path leaking the lock, the kind gate moved under the lock. A regression here is a server-wide deadlock, which is why the critic asked for these before merging./v1/embeddingsresolves the model and checks its kind before the lock, as the reranker path already did, so an unknown model or a chat model is answered at once even during a generation;ensureEmbedderLoadedtakes the resolved target, the cosine branch reuses the one it had, and oneloadFailureResponsemaps a failed swap for all three handlers.main(fix(detect): classify sequence-classification heads by what MLXRerankers can score, never as embedders #132): both### Fixedentries sit under one header; the two test files keep both sides' additions. On the merged head the full pure suite is 980/102 green and all five gated smokes pass (ms-marco 1.9e-6 vs PyTorch, NLI 7.4e-6 relative, Qwen3 ordering, endpoint, swap order 100.2 MB peak against 90.9 MB one copy and 9.4 MB left in the cache).