release(provider): v0.6.23 — MLX 0.32.0 (M5 NAX) + thread-stream crash fix + B1 (cache-deferred)#474
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR correctly flips the B=1 greedy fast-path to default-ON and patches a real concurrency race in the admission fence, with two security-relevant consequences that need explicit acknowledgement: the fast-path's eligibility gate now always fires for prefix-cache-enabled providers (correctly deflecting to the engine), and Trust boundaries touched
Threat analysisT-007 — Provider serves manipulated model outputs ℹ️ Neutral The weight-hash enforcement gap (SEC-007, fail-open at the coordinator) is unchanged. This PR only touches scheduling logic, not hash advertisement or coordinator admission. No regression here. T-028 — Residual inference data in GPU memory ℹ️ Neutral The T-041 — Cross-tenant prefix-cache timing oracle (SEC-035) ✅ Strengthens (partially)
New attack surface / concerns not covered by an existing threat1.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12226a109e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let v = env["DARKBLOOM_GEMMA_B1_FAST_PATH"], off.contains(v.lowercased()) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
With the gate now defaulting to true, an unconfigured provider can enter this path even though submitTokenized still has a reentrant window before fastPathTasks is populated: the first eligible request inserts its bridge and awaits reserveKVForRequest, while a second request arriving then sees activeBridgeCount > 0 so it uses the engine path, but the concurrency guard only checks fastPathTasks.isEmpty. When the first request resumes, runGreedyFastPath starts and both paths can share GPU/KV concurrently, violating the single-exclusive assumption and risking OOM or incorrect scheduling; please keep the default off or mark/fence the fast path before the first await.
Useful? React with 👍 / 👎.
| // tell the two indistinguishable gpt-oss 500 modes apart. Wire-compatible: | ||
| // `error_reason` is an optional inference-error field, omitted when nil. | ||
| public static let version = "0.6.17" | ||
| public static let version = "0.6.23" |
There was a problem hiding this comment.
This bumps ProviderCore.version to 0.6.23, but coordinator/api/server.go still hardcodes LatestProviderVersion = "0.6.11" for the /api/version and latestReleasedVersion() fallback path. In coordinators without a release row (dev/in-memory setups or an empty release table), clients will still be told 0.6.11 is latest, so existing providers will not be prompted to install the v0.6.23 bundle; please keep the coordinator fallback in sync with this version bump.
Useful? React with 👍 / 👎.
| if let v = env["DARKBLOOM_GEMMA_B1_FAST_PATH"], off.contains(v.lowercased()) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
With this returning true by default, normal unscoped Gemma requests (no prompt_cache_key/user, so cacheScope == "") now take the fast path even when the checkpoint prefix cache is enabled, which is the provider default. The fast path bypasses planRestoredCheckpoint/finalizeRestore and the engine capture hooks, so those requests no longer populate or hit the prefix cache; the live HybridCheckpoint tests that assert stores/hits exercise exactly this unscoped path. Please make cache-enabled schedulers defer to the engine, not only requests with a non-empty scope.
Useful? React with 👍 / 👎.
| if let v = env["DARKBLOOM_GEMMA_B1_FAST_PATH"], off.contains(v.lowercased()) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
Once this gate is on by default, any direct submitTokenized caller that has already rendered tools into the prompt but doesn't pass allowFastPath: false can take the fast path, because the eligibility check has no tool metadata beyond that optional flag. The production OpenAI path opts tool requests out, but existing direct users such as GemmaToolCallLiveTests call scheduler.submitTokenized(...) with the default flag and expect raw tool-call text from the engine; the fast-path runner explicitly treats .toolCall as unsupported and errors instead. Please make direct tokenized tool prompts stay on the engine path or require an explicit fast-path opt-in for submitTokenized.
Useful? React with 👍 / 👎.
|
Found 1 test failure on Blacksmith runners: Failure
|
P1 fast-path admission race: a fast-path request inserted its bridge then awaited KV reservation BEFORE runGreedyFastPath populated fastPathTasks; a concurrent request arriving in that window saw fastPathTasks empty, took the engine path, and ran alongside the fast path (single-exclusivity violation). Add a synchronous `fastPathAdmitting` fence set the instant the fast path is chosen (before any await); both engine concurrency gates now reject while it is set; cleared on every fast-path exit (KV failure, engine-superseded, launch) and defensively in cancelAllFastPathTasks/waitForFastPathTasks. P2 prefix-cache bypass: default-on routed unscoped greedy Gemma requests to the fast path, bypassing the checkpoint prefix cache (planRestoredCheckpoint/capture) and breaking HybridCheckpoint live tests. b1FastPathEligiblePure gains a prefixCacheEnabled param; eligibility now defers to the engine whenever the checkpoint or engine-tier prefix cache is active, not only on a non-empty scope. P2 direct tokenized tool prompts: submitTokenized now defaults allowFastPath: false so direct callers (e.g. GemmaToolCallLiveTests) stay on the engine; the production consumer passes allowFastPath: toolHandler == nil explicitly. P2 coordinator fallback: LatestProviderVersion 0.6.11 -> 0.6.23. Tests: eligibility helper gains prefixCacheEnabled; new prefix-cache-enabled ineligibility case; B1 A/B bench opts in with allowFastPath: true.
Gajesh2007
changed the title
…ump version 0.6.23
mlx-swift-lm@791c17f verified to compile against 0.32.0 (darkbloom build green). Host C++ + metallib both source from libs/mlx-swift/Source/Cmlx/mlx (d5a24040), matching libs/mlx. NAX auto-activates on M5+macOS26.2.
…fix)
Pins the fix for MLX 0.32's thread-local Metal command encoders that crashed
every async inference request ('There is no Stream(gpu, N) in current thread').
mlx-swift#10 (process-global default stream) + mlx-c#3 (new_thread_unsafe_stream
binding). Validated on M5 Max: gemma-4-26b-8bit clean, 79.4 tok/s decode,
prefill up to 3796 tok/s (NAX).
P1 fast-path admission race: a fast-path request inserted its bridge then awaited KV reservation BEFORE runGreedyFastPath populated fastPathTasks; a concurrent request arriving in that window saw fastPathTasks empty, took the engine path, and ran alongside the fast path (single-exclusivity violation). Add a synchronous `fastPathAdmitting` fence set the instant the fast path is chosen (before any await); both engine concurrency gates now reject while it is set; cleared on every fast-path exit (KV failure, engine-superseded, launch) and defensively in cancelAllFastPathTasks/waitForFastPathTasks. P2 prefix-cache bypass: default-on routed unscoped greedy Gemma requests to the fast path, bypassing the checkpoint prefix cache (planRestoredCheckpoint/capture) and breaking HybridCheckpoint live tests. b1FastPathEligiblePure gains a prefixCacheEnabled param; eligibility now defers to the engine whenever the checkpoint or engine-tier prefix cache is active, not only on a non-empty scope. P2 direct tokenized tool prompts: submitTokenized now defaults allowFastPath: false so direct callers (e.g. GemmaToolCallLiveTests) stay on the engine; the production consumer passes allowFastPath: toolHandler == nil explicitly. P2 coordinator fallback: LatestProviderVersion 0.6.11 -> 0.6.23. Tests: eligibility helper gains prefixCacheEnabled; new prefix-cache-enabled ineligibility case; B1 A/B bench opts in with allowFastPath: true.
51c5fd7 to
4e9ba6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e9ba6dd2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard engineStillCurrent(submitEpoch, engine), let container = self.modelContainer else { | ||
| // Fast-path exit: clear the admission fence (no task was launched). | ||
| fastPathAdmitting = false |
There was a problem hiding this comment.
If a fast-path admission is suspended in reserveKVForRequest, a cancel/timeout path can drop activeBridges[id] before this post-await guard resumes; because the guard only checks the engine/container, the request still launches runGreedyFastPath after its local state was removed. In that case recordFinish sees no bridge and returns zero without releasing any late KV reservation, and the off-engine generate can continue untracked alongside later work; mirror the engine path’s dropped-bridge check before starting the fast-path task.
Useful? React with 👍 / 👎.
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Submit.swift:53— Default allowFastPath changed from true to false without clear security justification- Suggestion: Document the security implications of this default change or revert if unintentional
Performance — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Submit.swift:117— Race condition in fast path admission fence - concurrent requests can slip through during KV reservation- Suggestion: Move fastPathAdmitting = true before any await points in the fast path decision logic to prevent concurrent requests from taking the engine path during KV reservation
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
2 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Submit.swift:53— Default allowFastPath changed from true to false without clear security justification- Suggestion: Document the security implications of this default change or revert if unintentional
Performance — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+B1FastPath.swift:55-62— Environment variable lookup performed on every request- Suggestion: Cache the environment variable lookup result at startup or use a lazy static property to avoid repeated ProcessInfo.processInfo.environment access on the hot path
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
2 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Submit.swift:53— Default value changed from true to false for allowFastPath parameter- Suggestion: Consider maintaining backward compatibility or documenting this breaking change clearly
Performance — 1 finding(s) (1 blocking)
- 🔴 [CRITICAL]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Submit.swift:117-119— Race condition in fast path admission gate allows concurrent engine execution- Suggestion: Move fastPathAdmitting = true before any await points and ensure atomic check-and-set semantics to prevent concurrent requests from bypassing the exclusivity guarantee
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
2 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| requestId: String? = nil, | ||
| cacheScope: String = "", | ||
| allowFastPath: Bool = true | ||
| allowFastPath: Bool = false |
There was a problem hiding this comment.
🟡 [MEDIUM] 🔒 Default value changed from true to false for allowFastPath parameter
💡 Suggestion: Consider maintaining backward compatibility or documenting this breaking change clearly
📊 Score: 3×4 = 12 · Category: breaking-change
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: COMMENT
Security — ✅ No issues found
Performance — ✅ No issues found
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
✅ All four passes clean. No issues found.
🤖 Automated review by Centaur · DAR-186
2 participants
Summary — provider v0.6.23
Three real wins + the correctness work to make them safe:
libs/mlx-swift→df1fdc5f(MLX 0.32.0). NAX auto-activates on M5 + macOS 26.2 — measured prefill up to 3,796 tok/s on an M5 Max (~3.2× compute-bound quantized matmul vs M4). Decode is memory-bound so unchanged (~79–95 tok/s).async/await+ the actor engine hop threads, so every inference aborted withThere is no Stream(gpu, N) in current thread. Fixed by using a single process-global "thread-unsafe" default stream (restores MLX 0.31 semantics):Layr-Labs/mlx-c#3+Layr-Labs/mlx-swift#10(both merged).DARKBLOOM_PREFIX_CACHE=0(then ~+21% decode for single greedy Gemma requests). This is the intentional decision (no prefix-cache regression); a nuanced "B1 on cache-miss" policy is a follow-up.Codex review fixes (all addressed)
fastPathAdmittingactor flag set synchronously before the KV-reserve await; both concurrency gates reject on!fastPathTasks.isEmpty || fastPathAdmitting; cleared on every exit. Prevents engine+fast-path running concurrently.submitTokenized(allowFastPath:)now defaultsfalse(production engine passes it explicitly).LatestProviderVersion 0.6.11 → 0.6.23.Before / After
flowchart LR subgraph Before [v0.6.22 / MLX 0.31.1] A1[async inference] --> B1[OK on 0.31 single global stream] A2[Gemma B=1 greedy] --> B2[batched engine ~63 tok/s] A3[M5 prefill] --> B3[no NAX path] end subgraph After [v0.6.23 / MLX 0.32.0] A4[async inference] --> B4[global thread-unsafe stream: no crash] A5[Gemma B=1 greedy] --> B5{prefix cache on?} B5 -- yes default --> B6[engine + prefix cache] B5 -- DARKBLOOM_PREFIX_CACHE=0 --> B7[B1 fast path ~76 tok/s] A6[M5 prefill] --> B8[NAX ~3796 tok/s] endValidation
gemma-4-26b-8bit, MLX 0.32): clean — greedy (coherent Rayleigh-scattering answer), streaming (SSE), concurrent ×3 batched (no crash), 79–95 tok/s decode, 3,796 tok/s prefill.2908b669); on the fix commit a single flakyparse:mean<=1mslatency assertion (1.6 ms) tripped under CI load — unrelated to these changes (re-running).Known gaps (tracked, not blocking the crash fix)