fix(fleet): per-provider circuit breaker + 5xx reclassifier (coordinator) + v0.6.16 #408 wedge & KV-pin recovery (provider) — DAR-335/336/337/338#413
Conversation
…s->429 vs fault->5xx (DAR-336)
…route fault-503 nodes, fail-open (DAR-335)
…ess watchdog & KV-pin recovery (DAR-338, DAR-337)
QuickCapacityCheck excluded breaker-open providers, so an all-breaker-open fleet reported 0 candidates / 0 capacity-rejections and the consumer hard-503'd "no_provider" BEFORE dispatch's fail-open valve could serve a probe — the exact model-wide outage the valve exists to prevent during a bad fleet-wide rollout. The preflight now ignores the provider breaker (every other gate, incl. the shape-keyed inference-error cooldown, still honored). Adds a regression test.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR introduces a backend-liveness watchdog and a per-provider circuit breaker that are net-positive for availability and marginally improve security posture, with no regressions against the threat model and two new attack surfaces that require brief scrutiny. Trust boundaries touched
Threat assessment
New attack surface not covered by existing threats1.
2.
SEC-* findings resolved by this PRNone — no open SEC-* findings are directly closed by this change. The circuit-breaker (SEC-034 adjacent, pre-auth WebSocket flooding) is not addressed here; the Minor notes
🔐 Threat model: |
![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: a1bc73a2b3
ℹ️ 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".
| } | ||
|
|
||
| let longestStall = longestAdmittedZeroTokenStallSeconds(now: now) | ||
| let hasDemand = !activeBridges.isEmpty || pendingRequestCount > 0 |
There was a problem hiding this comment.
When the KV budget collapses to the floor, normal requests whose promptTokens + maxTokens exceed that floor fail at the early requestBudget <= budgetMax guards in submitTokenized/submit before any bridge is inserted, so activeBridges and pendingRequestCount remain zero on watchdog ticks. In that exact pinned-pool scenario (the one this watchdog is meant to recover from), hasDemand stays false and the policy never returns .pinned, leaving the provider to keep returning token_budget_exhausted without self-restarting; track recent admission rejections as demand or insert a demand marker before the early token-budget reject.
Useful? React with 👍 / 👎.
| "request rejected", | ||
| "queue full", | ||
| "server busy", | ||
| "service temporarily unavailable", |
There was a problem hiding this comment.
Including the broad service temporarily unavailable marker makes coordinator-side failures match the provider-capacity classifier too: dispatchOneProvider returns "service temporarily unavailable — please retry" on reserveAdditionalForProvider DB errors, and the dispatch exhaustion path reclassifies any matching 5xx to a 429. In that billing/store outage scenario clients and uptime accounting see a capacity/rate-limit response instead of the real coordinator 503, so this marker needs to be narrowed to provider-originated overload text or the reclassification needs to be limited to actual provider errors.
Useful? React with 👍 / 👎.
| "request timed out waiting for capacity", | ||
| "queue full", | ||
| "server busy", | ||
| "service temporarily unavailable", |
There was a problem hiding this comment.
The API failure classifier explicitly treats "request rejected" as overload/backpressure capacity, but this breaker’s capacity marker list omits that same string, so a provider returning 503 request rejected under transient backpressure is counted as a node fault and trips the provider-wide breaker after five such sheds. In that load-shedding scenario the change deroutes a healthy busy node instead of ignoring the shed; add the marker here or otherwise distinguish the genuine-fault variant before feeding the breaker.
Useful? React with 👍 / 👎.
| if w2, cc2, cr2, tl2, vr2, tr2, bt2, _ := r.selectBestCandidateScanLocked(model, pr, true, excludeIDs...); w2 != nil { | ||
| return w2, cc2, cr2, tl2, vr2, tr2, bt2 |
There was a problem hiding this comment.
When pass 1 finds healthy providers only rejected for capacity/TTFT and also sees a breaker-open provider, this unconditional second scan bypasses the breaker and can select the quarantined node instead of returning the capacity signal that would queue or 429. In a mixed fleet where good nodes are simply busy but one bad node is idle, requests are sent to the known-failing provider; only fail open when the breaker is the sole reason no route exists, not when healthy providers are available but temporarily full or slow.
Useful? React with 👍 / 👎.
|
Found 1 test failure on Blacksmith runners: Failure
|
…ned KV pool self-restarts
- Fail-open valve now triggers ONLY when the node-health breaker is the sole reason a request has no route; if a healthy provider is merely busy (capacity) or too slow (TTFT), surface that signal so the request queues/429s instead of routing to a known-bad, breaker-open node. - Breaker treats a 503 'request rejected' as a capacity/backpressure shed (consistent with the api reclassifier and the inference-error breaker), so a healthy-but-busy node is no longer quarantined. - Drop 'service temporarily unavailable' from the api capacity table: the coordinator emits it on its own store/DB errors, which must stay 5xx rather than be hidden as an uptime-neutral 429.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6f558dfc4
ℹ️ 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".
| // Bad weights / metallib: a real load fault, NOT a capacity miss. MUST stay | ||
| // here (see the "not loaded" caveat in capacityClassMarkers above). | ||
| "model load failed", |
There was a problem hiding this comment.
When a provider accepts a request but ensureModelLoaded fails for transient capacity, ProviderLoop sends the coordinator "model load failed: ..." with a 503 (for example the InferenceError.modelLoadFailed memory/headroom paths and slot-cap invalidModelDirectory paths in ProviderLoop.swift). Because this marker is checked before "insufficient memory", "insufficient kv", and the slot-cap predicate, those capacity failures now stay as raw 503s instead of being reclassified to the uptime-neutral 429, so overloaded/cold-load providers can still penalize reliability rather than trigger normal failover.
Useful? React with 👍 / 👎.
| case 500, 502, 504: | ||
| return true |
There was a problem hiding this comment.
When older/provider paths surface capacity rejects such as token_budget_exhausted, insufficient KV, or context overflow as 500/502/504, the dispatch path can still recognize them as capacity later via isCapacityClassProviderError(statusCode >= 500), but this branch records them as provider faults first. Five long-prompt or near-miss capacity rejects from an otherwise healthy node can therefore open the provider-wide breaker and deroute it, even though the final client response is meant to be an uptime-neutral 429; check the capacity markers before treating these status codes as unconditional faults.
Useful? React with 👍 / 👎.
| // slot_state ("crashed"/"reloading" instead of a lying "idle"), and | ||
| // self-restarts the engine/model slot to recover. Wire-compatible: only | ||
| // existing slot_state string values are emitted; no protocol changes. | ||
| public static let version = "0.6.16" |
There was a problem hiding this comment.
This bumps the Swift binary to 0.6.16, but coordinator/api/server.go still falls back to LatestProviderVersion = "0.6.11" when no release row is registered. In in-memory/dev coordinators or release-table bootstrap scenarios, update checks and provider metadata will advertise 0.6.11 even though the built provider is 0.6.16, so operators may not be prompted to install the wedge/liveness fix shipped here.
Useful? React with 👍 / 👎.
| let hash = currentWeightHash | ||
| isReloadingForRecovery = true | ||
| livenessLogger.error("self-restarting model \(id, privacy: .public) to recover backend liveness") | ||
| await loadModel(container: container, modelId: id, weightHash: hash) |
There was a problem hiding this comment.
During a liveness self-restart, this loadModel call first runs stopCurrentEngine, which clears the scheduler's modelId while the ProviderLoop still keeps the old modelSlots[modelId] entry. Any heartbeat in that reload window reports a reloading slot for model:"" rather than for the actual model, so the coordinator can treat the provider as an eligible cold/unknown candidate for the real model; ensureModelLoaded then returns because the stale slot entry exists and the request reaches a scheduler with engine == nil, producing No model loaded 500s instead of being derouted until reload completes.
Useful? React with 👍 / 👎.
| // fault-503ing ~all of its requests gets quarantined fleet-wide. errStr lets | ||
| // the breaker tell a capacity-503 (ignored) from a fault-503 (counted). Both | ||
| // breakers coexist. | ||
| if opened, _ := s.registry.RecordProviderOutcome(providerID, false, statusCode, errStr); opened { |
There was a problem hiding this comment.
This feeds the node-health breaker with only providerID, so five genuine 500/502/504 failures from a single corrupt model or request shape open a provider-wide breaker and providerPassesRoutingGatesLockedEx then rejects the same node for unrelated healthy models too. The shape-keyed RecordInferenceError already quarantines the failing provider/model/shape; the node-wide breaker should require cross-model/all-request evidence (or otherwise include model scope) before derouting the whole provider.
Useful? React with 👍 / 👎.
…3-cap) A request now keeps failing over to fresh healthy providers until one succeeds, candidates are exhausted (each failed provider is excluded, so dispatchPrimary returns outcomeFailFast once none remain), or the request's deadline/context fires (run() checks r.Context() each attempt). maxDispatchAttempts becomes a high safety ceiling (64) against a pathological hot loop rather than the normal stopping point; retries never re-queue, so failover stays fast. Exhaustion messages now report the actual attempt count. Adds a regression test proving failover reaches a healthy provider sitting behind six failing ones (the old 3-attempt cap gave up before reaching it).
…breaker - inference_failure_class: a provider 'model load failed: <capacity reason>' (insufficient memory/KV, slot-cap) now reclassifies to an uptime-neutral 429 instead of staying 5xx — capacity markers are checked before the model-load-failed wrapper; bad-weights/opaque load failures still stay 5xx. - provider_breaker: ignore a capacity-shaped 5xx regardless of status code, so a 500/502/504 carrying a capacity/backpressure message no longer trips the node-health breaker (some/older provider paths surface capacity as a non-503 5xx; the dispatch reclassifier already 429s them). Renames the breaker's capacity helper/markers to drop the 503-specific name.
…veness self-restart — no nil-engine 500s
- KV-quant + benchmark files (master == merged foundation): keep ours (foundation + gate changes) — verified no review-time divergence. - BatchScheduler.swift: 3-way merge on the foundation base to combine the concurrency cap-fix (seed dynamicMaxConcurrentRequests to the ceiling) with master's #413 backend-liveness/wedge tracking. - Submodule already at merged engine main (a9e0ee1). Builds clean; cap/KVQuant/#413-liveness/config/budget suites green.
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)
- 🔵 [INFO]
coordinator/api/dispatch.go:1810-1812— Context deadline check may leak timing information- Suggestion: Consider using a constant-time check or adding jitter to prevent timing-based inference about request processing state
Performance — 4 finding(s) (2 blocking)
- 🟡 [MEDIUM]
coordinator/api/dispatch.go:1800-1812— Context cancellation check only after first attempt may allow unbounded retries- Suggestion: Move context check before the loop or add it inside the loop body to ensure timely cancellation
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:135-155— Opportunistic sweep runs on every RecordProviderOutcome call when maps exceed 1024 entries- Suggestion: Add probabilistic triggering (e.g., 1% chance) or use a separate periodic cleanup to avoid repeated expensive map iterations
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:119-133— longestAdmittedZeroTokenStallSeconds iterates all active bridges on every watchdog tick- Suggestion: Cache the longest stall time and update incrementally when bridges change state, or limit iteration frequency
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/GlobalKVCacheBudget.swift:172-174— Fire-and-forget Task creation in reclaimForShortfall without bounds- Suggestion: Use a TaskGroup or limit concurrent reclaim tasks to prevent unbounded Task spawning under high load
Type_diligence — 2 finding(s)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800— Loop variableattemptrelies on implicit zero initialization- Suggestion: Make initialization explicit:
for attempt := 0; attempt < maxDispatchAttempts; attempt++
- Suggestion: Make initialization explicit:
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:358— Function parameter uses bareanytype- Suggestion: Define a specific type for the word parameter or use a more constrained interface
Additive_complexity — 6 finding(s) (2 blocking)
- 🔵 [INFO]
coordinator/api/dispatch.go:63-77— maxDispatchAttempts increased from 3 to 64 with extensive comment justification- Suggestion: Consider if 64 is truly necessary or if a smaller value like 16 would suffice. The comment suggests this is a 'safety ceiling' but such a high value may mask underlying issues.
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:1-366— Entire new file adds per-provider circuit breaker with significant complexity- Suggestion: Consider if this functionality could be integrated into existing error handling rather than a separate subsystem. The overlap with inference error cooldown suggests potential consolidation opportunities.
- 🔵 [INFO]
coordinator/api/inference_failure_class.go:2-152— Function renamed and logic duplicated between api and registry packages- Suggestion: Extract shared classification logic to a common package to avoid maintaining two similar implementations of capacity vs fault classification.
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BackendLivenessProbe.swift:1-128— Pure policy struct for simple threshold checks- Suggestion: Consider if this level of abstraction is needed for what are essentially threshold comparisons. The policy could be simplified to direct threshold checks.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/KVPoolReclaimer.swift:1-113— New actor to move blocking operations off main actor- Suggestion: While solving a real problem, this adds significant architectural complexity. Consider if the blocking operations could be made non-blocking instead of adding another actor layer.
- 🔴 [CRITICAL]
coordinator/registry/scheduler.go:278-629— Complex fail-open logic with two-pass selection and bypass mechanisms- Suggestion: The two-pass selection with breaker bypass adds significant complexity to the hot path. Consider if this could be simplified or if the fail-open behavior could be achieved with less intricate logic.
13 finding(s) total, 4 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 — ✅ No issues found
Performance — 4 finding(s) (3 blocking)
- 🟡 [MEDIUM]
coordinator/api/dispatch.go:1800-1812— Context cancellation check only after first attempt may allow unbounded retries- Suggestion: Move the context check before the attempt loop or add it inside the loop to ensure timely cancellation
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler.swift:1712-1716— Fire-and-forget KV reclaim may still block on actor boundary- Suggestion: Verify that kvBudget.reclaimForShortfall is truly non-blocking or consider using Task.detached for complete isolation
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:358-375— Linear search through capacity shed markers on every provider error- Suggestion: Convert capacityShedMarkers to a map or use a more efficient string matching approach for hot path performance
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:119-133— Inefficient O(n) scan of activeBridges on every liveness assessment- Suggestion: Maintain a separate index of zero-token bridges or cache the longest stall to avoid repeated full scans
Type_diligence — 3 finding(s)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800— range over maxDispatchAttempts without checking bounds- Suggestion: Add explicit bounds check or use
for attempt := 0; attempt < maxDispatchAttempts; attempt++instead of range
- Suggestion: Add explicit bounds check or use
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:300— Using map[string]any for capacityShedMarkers slice- Suggestion: Define a typed slice
[]stringinstead of relying on string matching patterns
- Suggestion: Define a typed slice
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:320— Using map[string]any for faultClassMarkers slice- Suggestion: Define a typed slice
[]stringinstead of relying on string matching patterns
- Suggestion: Define a typed slice
Additive_complexity — 4 finding(s) (1 blocking)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800-1812— deadline check on every retry attempt adds unnecessary complexity- Suggestion: Move context deadline check to the beginning of run() method only, or use a select with context.Done() in the main dispatch loop instead of checking on every iteration
- 🟡 [MEDIUM]
coordinator/registry/provider_breaker.go:1-366— capacity classification logic duplicated between api and registry packages- Suggestion: Extract the capacity vs fault classification into a shared package to avoid maintaining two similar implementations of isCapacityClassProviderError and isCapacityShedError
- 🔵 [INFO]
coordinator/api/inference_failure_class.go:108-152— complex string classification with overlapping marker arrays- Suggestion: Consider a single classification function with clear precedence rules instead of separate marker arrays and multiple classification paths
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:1-182— liveness watchdog logic mixed into scheduler instead of separate component- Suggestion: Extract liveness monitoring into a dedicated LivenessWatchdog class that observes scheduler state rather than embedding the logic directly in BatchScheduler
11 finding(s) total, 4 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 — ✅ No issues found
Performance — 5 finding(s) (2 blocking)
- 🟡 [MEDIUM]
coordinator/api/dispatch.go:1800-1812— Context cancellation check inside hot retry loop may cause unnecessary overhead- Suggestion: Move context check outside the loop or use a select statement to avoid repeated Context().Err() calls on every iteration
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:95-112— Nested loop over outcomes array for windowed stats calculation- Suggestion: Consider maintaining a separate time-ordered structure or using a more efficient sliding window algorithm to avoid O(n) scan on every assessment
- 🟡 [MEDIUM]
coordinator/registry/provider_breaker.go:140-156— Opportunistic map cleanup triggers on >1024 entries with full iteration- Suggestion: Consider using a background cleanup goroutine or more efficient data structure to avoid blocking the hot path when maps grow large
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:119-133— Linear scan through all active bridges to find longest stall on every watchdog tick- Suggestion: Maintain a priority queue or sorted structure of bridges by admission time to avoid O(n) scan every 2 seconds
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/GlobalKVCacheBudget.swift:172-178— Fire-and-forget task spawning in nonisolated functions may accumulate under high load- Suggestion: Consider using a bounded task queue or coalescing mechanism to prevent unbounded task creation during admission pressure spikes
Type_diligence — 1 finding(s)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800— range over maxDispatchAttempts without checking bounds- Suggestion: Add explicit bounds check or use a typed constant instead of bare range
Additive_complexity — 6 finding(s) (4 blocking)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800-1812— Deadline check on every retry attempt adds unnecessary complexity- Suggestion: Move the deadline check inside dispatchPrimary() or use a simpler timeout mechanism. The current approach checks context cancellation on every loop iteration when maxDispatchAttempts is already a safety ceiling.
- 🟡 [MEDIUM]
coordinator/api/inference_failure_class.go:2-152— Complex string classification with overlapping responsibilities- Suggestion: Split into separate classifiers for capacity vs fault detection, or use a simple map-based approach. The current implementation has complex precedence rules and duplicate logic between api and registry packages.
- 🟡 [MEDIUM]
coordinator/registry/provider_breaker.go:1-366— Entire new circuit breaker system duplicates existing error handling- Suggestion: Consider extending the existing inference error cooldown system instead of adding a parallel breaker. The two systems have overlapping responsibilities and similar state management patterns.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BackendLivenessProbe.swift:1-128— Pure policy class for simple threshold checks- Suggestion: Replace with direct threshold comparisons in the scheduler. The policy abstraction adds indirection for what are essentially simple numeric comparisons.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/KVPoolReclaimer.swift:1-113— Separate actor for rate-limited cache clearing- Suggestion: Use a simple debounced function or timer instead of a full actor. The reclaimer mainly wraps rate-limiting around a single operation.
- 🔵 [INFO]
coordinator/api/consumer.go:218-257— Provider outcome recording duplicated across multiple functions- Suggestion: Create a single recordProviderOutcome helper that handles both breakers consistently, reducing the repeated pattern of calling both RecordInferenceError and RecordProviderOutcome.
12 finding(s) total, 6 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 — ✅ No issues found
Performance — 4 finding(s) (3 blocking)
- 🟡 [MEDIUM]
coordinator/api/dispatch.go:1800-1812— Context cancellation check only after first attempt may allow unbounded retries- Suggestion: Move context check before the loop or add it inside the loop body to ensure timely cancellation
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler.swift:1712-1716— Fire-and-forget KV reclaim may still block on actor boundary- Suggestion: Verify that kvBudget.reclaimForShortfall is truly non-blocking or use Task.detached to ensure no actor blocking
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:315-366— String processing repeated for each provider outcome classification- Suggestion: Pre-compute lowercased error strings or cache classification results to avoid repeated string operations
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:119-133— O(n) scan of activeBridges on every liveness assessment- Suggestion: Maintain a separate index of zero-token bridges or use more efficient data structure for this lookup
Type_diligence — 3 finding(s)
- 🔵 [INFO]
coordinator/api/dispatch.go:1800— range over maxDispatchAttempts without checking bounds- Suggestion: Add explicit bounds check or use
for attempt := 0; attempt < maxDispatchAttempts; attempt++to avoid potential range issues
- Suggestion: Add explicit bounds check or use
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:289— map[string]any used in providerOutcomes map- Suggestion: Replace map[string]any with a typed struct for providerOutcomes entries to improve type safety
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:290— map[string]any used in providerBreakerOpenUntil map- Suggestion: Replace map[string]any with a typed struct for providerBreakerOpenUntil entries to improve type safety
Additive_complexity — 8 finding(s) (5 blocking)
- 🔵 [INFO]
coordinator/api/consumer.go:66-79— maxDispatchAttempts increased from 3 to 64 with extensive comment justification- Suggestion: Consider if 64 is truly necessary or if a smaller value like 16 would suffice. The extensive comment suggests uncertainty about the right value.
- 🟡 [MEDIUM]
coordinator/api/dispatch.go:1800-1812— Context cancellation check duplicates existing timeout handling- Suggestion: The request context timeout is already handled by the dispatch state machine. Consider if this additional check adds meaningful value or creates redundant timeout paths.
- 🟡 [MEDIUM]
coordinator/api/inference_failure_class.go:2-152— Complex dual-bucket classification system with extensive marker arrays- Suggestion: Consider simplifying to a single classification function with clear rules rather than maintaining two separate marker arrays and complex precedence logic.
- 🔵 [INFO]
coordinator/registry/provider_breaker.go:1-366— Reimplements circuit breaker pattern alongside existing error_cooldown.go- Suggestion: Consider if the existing inference error cooldown could be extended rather than implementing a parallel circuit breaker system.
- 🔵 [INFO]
provider-swift/Sources/ProviderCore/Inference/BackendLivenessProbe.swift:1-128— Pure policy struct for simple boolean decision- Suggestion: The policy abstraction may be overkill for what appears to be straightforward threshold checks. Consider inlining the logic.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/KVPoolReclaimer.swift:1-113— Dedicated actor for rate-limited cache clearing- Suggestion: Consider if this complexity is justified over simpler rate-limiting within the existing budget actor, given the single responsibility.
- 🔴 [CRITICAL]
coordinator/registry/scheduler.go:341-400— Two-pass selection with fail-open logic adds significant complexity- Suggestion: The dual-pass selection with breaker bypass creates complex control flow. Consider if a simpler approach like weighted selection could achieve the same fail-open behavior.
- 🟡 [MEDIUM]
provider-swift/Sources/ProviderCore/Inference/BatchScheduler+Liveness.swift:1-182— Scheduler now responsible for self-restart and liveness detection- Suggestion: Consider if liveness monitoring belongs in a separate component rather than expanding the scheduler's responsibilities.
15 finding(s) total, 8 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
2 participants
Summary
Post-swap (coordinator
0d4e5a63, fleet v0.6.15), 9 nodes produced 83% of all gpt-oss errors while staying fully routable — one box failed 99/99 requests for 15+ min and kept receiving traffic — capping gpt-oss OpenRouter-formula uptime at ~88% ("degraded").This PR bundles both layers of the fix:
#408GPU-sync admission wedge and the un-recoverable pinned KV pool, so we can stop relying on the breaker.Linear: DAR-335, DAR-336, DAR-337, DAR-338.
Architecture: before → after
Before — bad nodes stay routable and admission wedges
flowchart TD subgraph CB["COORDINATOR — before"] A["Consumer request"] --> B["Preflight + scheduler"] B --> C{"Pick provider<br/>warm-idle looks ideal"} C --> D["Bad node<br/>serial D6C3FH4PJ4"] D -->|"503 / hang, ttft=0"| E["admitted_but_failed = 5xx"] E --> F[("OpenRouter uptime ~88%<br/>DEGRADED")] D -. "still idle+warm — never derouted" .-> C end subgraph PB["PROVIDER — before · PR 408 / v0.6.15"] G["commit() on<br/>GlobalKVCacheBudget actor"] --> H["reclaimForShortfall()"] H --> I[["BLOCKING MLX synchronize()<br/>+ clearCache() ON the actor"]] I -->|"actor stalled,<br/>all reservations queue"| J["request timed out<br/>waiting for capacity"] J --> K["engine loaded, not crashed"] end K -. "heartbeat lies: slot_state=idle" .-> DAfter — breaker deroutes faults, reclassifier protects uptime, admission never blocks
flowchart TD subgraph CA["COORDINATOR — after · DAR-335 + DAR-336"] A2["Consumer request"] --> P{"Per-provider<br/>breaker open?"} P -->|"yes"| X["Derouted<br/>re-probed after cooldown"] P -->|"no"| C2{"Pick provider"} P -. "ALL tripped → fail-open probe" .-> C2 C2 --> H1["Healthy node serves ✅"] C2 --> D2["Node returns 503"] D2 --> R{"capacity-class?"} R -->|"yes"| Q["reclassify → 429<br/>uptime-neutral, OR fails over"] R -->|"no — crash / fault"| S["stays 5xx → trips breaker"] S --> P H1 --> U[("OpenRouter uptime ≥95%")] Q --> U end subgraph PA["PROVIDER — after · v0.6.16 · DAR-338 + DAR-337"] G2["commit() on actor"] --> N["decide on snapshot<br/>NEVER blocks on GPU"] N -. "fire-and-forget" .-> RC["off-actor KVPoolReclaimer<br/>coalesced GPU flush"] N --> ADM["admit / reject instantly ✅"] W["Liveness watchdog"] -->|"wedged or pinned KV"| HB["truthful heartbeat<br/>slot_state = crashed / reloading"] W --> RS["self-restart engine<br/>clears pinned pool"] end HB -. "coordinator deroutes via breaker" .-> PCoordinator (ship-today mitigation)
DAR-335 — per-provider node-health circuit breaker
Every existing failure-derived deroute path ignores generic fault-503 (the inference-error breaker counts only 500/502/504; reputation skips 503; dispatch-load cooldown only fires on model-load failures; within-request retry has no cross-request memory). A node returning 503 for ~100% of requests advertises
slot_state=idle+warm, so the scheduler treats it as an ideal instant-TTFT target and keeps feeding it.coordinator/registry/provider_breaker.go: breaker keyed by provider ID only (node crashes affect every model). Trips on 5 consecutive faults OR (≥20 reqs / 120s AND >80% fail); cooldown60s × 2^tripscapped at 5m; half-open re-probe; auto-closes on a success.token_budget_exhausted,kv headroom,insufficient memory,draining); counts genuine faults (500/502/504, and 503 with crash/opaque/request rejected). Local classifier in-package (registry can't import api).providerPassesRoutingGatesLocked(one chokepoint covering selection, admit re-check, preflight, queued + cold dispatch).selectBestCandidateScanLocked/ReserveProviderEx: if every candidate is tripped, routing still serves (a bad fleet-wide rollout can never deroute everyone).noteInferenceError/noteInferenceSuccess(error string threaded through the dispatch terminals); telemetryrouting.provider_breaker_open/_closed. Cleared onDisconnect.DAR-336 — split provider-5xx reclassification
isUnservableProviderError→isCapacityClassProviderErrorwith two extendable tables:draining, slot-cap, overload/backpressure (request rejected,queue full,server busy,service temporarily unavailable,request timed out waiting for capacity), cold/not-loaded misses.internal error,model load failed, the opaque Foundation string; default is fault so crashes stay visible.Extra fix found in review — preflight fail-open (DAR-335)
The fail-open valve lived only at dispatch-selection, but the public preflight (
QuickCapacityCheck) still excluded breaker-open nodes — so an all-breaker-open fleet reported 0 candidates / 0 capacity-rejections and the consumer hard-503'dno_providerbefore dispatch's fail-open could run. That re-introduced the model-wide outage the valve exists to prevent (and they just had a fleet-wide v0.6.15 regression). The preflight now fails open on the breaker; every other gate (incl. the shape-keyed inference-error cooldown) is still honored. Regression test added.Provider v0.6.16 (the cure)
DAR-338 —
#408KV self-heal blocking GPU sync wedged admission#408put a blockingMLX...synchronize()inside theGlobalKVCacheBudgetactor (commit()/token-gate →reclaimForShortfall→clearCache). Under load every near-miss stalled the actor; all reservations serialized behind it; requests timed out (request timed out waiting for capacity) while the node kept advertising healthy → fleet-wide wedge on 6+ machines, ~38% of residual errors. The shipped 1/sec rate-limit was insufficient.KVPoolReclaimer(dedicated actor) owns the GPU flush; the admission path triggers a coalesced, rate-limited, fire-and-forget background reclaim and decides on the current snapshot.reserve/commit/reclaimForShortfall/token-gate execution ever blocks/awaits a GPU sync on the budget actor. Proven by a test (reservereturns <250 ms against a 0.5 s flush).clearCache(off the admission path) and the#408engine-detach race fix are preserved.DAR-337 — pinned/collapsed KV pool can't recover
A node's KV budget collapses to ~1024 tokens and 503s every request for 15+ min; only a reload clears it.
BackendLivenessPolicywatchdog detects wedge (admitted request, 0 tokens past threshold) and pinned (budget collapsed + demand + 0 successes for a sustained window).slot_state→crashed/reloading, no longer a lyingidle/warm) so the coordinator deroutes, and the engine self-restarts (cooldown-guarded) to clear the pinned pool.LatestProviderVersionleft untouched — that's a deploy decision).Testing
go test ./...green, incl.registry+apiwith-race. New breaker suite (consecutive/rate trip, half-open recovery, capacity-503 ignored, fault-503 trips, fail-open viaReserveProviderEx, preflight fail-open, Disconnect cleanup, >1024 sweep) + DAR-336 bucket tests with real observed strings.swift buildgreen;swift test --skip Live --skip Performance --skip ThroughputSweep→ 877 tests / 53 suites pass, incl. rewritten self-heal tests, newKVPoolReclaimerandBackendLivenesssuites.dispatch.go(touched by both coordinator tickets) auto-merged and the combined tree was re-verified green.Deploying
coordinator/Dockerfileand blue-green deploys; no fleet update needed. The breaker + reclassifier take effect immediately and restore OR-uptime toward ≥95% under the same fleet conditions.release-swift.yml), upload the bundle, and bumpLatestProviderVersionin a follow-up so the fleet converges. Because this is one PR, coordinator + provider land together on merge — if you need to decouple, we can split into two PRs instead.Risks / follow-ups
#408false-429 it originally fixed).🤖 Implemented via parallel sub-agents in isolated worktrees.