bugfix: hardening late July - #212
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change updates model matching, endpoint balancing, circuit-breaker probes, proxy model metrics, request-size handling, provider routing, response handling, ETag generation, logging, and configuration defaults. Tests cover the revised behaviour across these areas. ChangesModel matching and endpoint balancing
Shared circuit-breaker resilience
Resolved proxy model metrics
Request and provider handling
Observability and configuration defaults
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapter/proxy/sherpa/service_retry.go (1)
76-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the translated model before request construction.
Line 78 can record a failure before Lines 93-96 copy
ContextModelKeyintostats.Model. If request construction fails after translation, the model statistic uses the client model instead of the resolved backend model.Set
stats.Modeland deriveresolvedModelbeforehttp.NewRequestWithContext. Keep the model-header assignment after request construction. Add a regression test for this failure path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/proxy/sherpa/service_retry.go` around lines 76 - 104, In the retry flow around http.NewRequestWithContext, apply ContextModelKey to stats.Model and derive resolvedModel before constructing proxyReq so construction failures record the translated backend model. Keep setting the translated model header on proxyReq after successful construction, reuse the precomputed resolvedModel for all success/failure records, and add a regression test covering request-construction failure after translation.
🧹 Nitpick comments (7)
internal/app/middleware/logging_gate_test.go (1)
246-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the test comment to its reason.
Lines 246-258 repeat the handler implementation and assertion mechanics. Keep the reason for the regression test. This makes the test easier to maintain.
Proposed change
-// TestCombinedLogging_PathMutatingHandlerStaysQuiet pins the fix for the -// quiet-gate fail-open: handler_proxy.go's dispatchToEndpoints strips the -// route prefix by mutating r.URL.Path in place (r.URL.Path = pr.targetPath) -// before forwarding upstream, so the post-request console line and the -// access log must not re-read r.URL.Path after next.ServeHTTP returns - they -// would see the backend's target path instead of the original route and lose -// the quiet-poll classification entirely. CombinedLoggingMiddleware now -// captures path once, up front, and reuses it throughout. -// -// The stand-in handler mutates r.URL.Path to "/v1/chat/completions" - a -// vLLM-style target path with neither an "/olla/" substring nor an "/api/" -// prefix, so a regression here cannot hide behind Ollama's coincidentally -// "/api/"-prefixed target paths. +// TestCombinedLogging_PathMutatingHandlerStaysQuiet protects quiet-route logging +// because proxy forwarding mutates r.URL.Path before upstream dispatch.As per coding guidelines, comments in
**/*.gomust state why, not what, and be concise and direct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/middleware/logging_gate_test.go` around lines 246 - 258, Shorten the comment above TestCombinedLogging_PathMutatingHandlerStaysQuiet to state only why the regression test exists: the handler mutates r.URL.Path, so middleware must preserve the original route for quiet-poll classification. Remove implementation details, target-path examples, and assertion mechanics.Source: Coding guidelines
internal/adapter/balancer/least_connection_test.go (1)
443-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert consecutive tie selections differ.
The 5/5 aggregate assertion accepts clustered selections that do not alternate. Record the previous selection and fail when the next tied selection has the same endpoint.
Proposed test update
- counts := make(map[string]int) + counts := make(map[string]int) + var previous string for range 10 { selected, err := selector.Select(ctx, endpoints) if err != nil { t.Fatalf("Select failed: %v", err) } + if selected.Name == previous { + t.Fatalf("expected consecutive tied selections to alternate, got %q twice", selected.Name) + } + previous = selected.Name counts[selected.Name]++ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/balancer/least_connection_test.go` around lines 443 - 460, Update the sequential tie-selection test around selector.Select to track the previously selected endpoint and fail whenever consecutive selections have the same Name. Retain the existing 10-selection loop and aggregate 5/5 assertions, adding the consecutive-selection check after each successful selection.internal/adapter/balancer/least_connections.go (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce implementation detail in Go comments.
Keep each comment concise and explain why the code exists. Do not describe the cursor mechanics or test steps.
internal/adapter/balancer/least_connections.go#L15-L21: State that tie rotation prevents recovered endpoints from starvation.internal/adapter/balancer/least_connections.go#L54-L57: State that all minimum-count endpoints are retained to prevent registration-order bias.internal/adapter/balancer/least_connection_test.go#L427-L433: State that the test prevents starvation during persistent ties.internal/adapter/balancer/least_connection_test.go#L463-L466: State that the test preserves strict minimum-count selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/balancer/least_connections.go` around lines 15 - 21, Shorten the comments at internal/adapter/balancer/least_connections.go:15-21 to state that tie rotation prevents recovered endpoints from starvation; at internal/adapter/balancer/least_connections.go:54-57 to state that retaining all minimum-count endpoints prevents registration-order bias; at internal/adapter/balancer/least_connection_test.go:427-433 to state that the test prevents starvation during persistent ties; and at internal/adapter/balancer/least_connection_test.go:463-466 to state that the test preserves strict minimum-count selection. Keep each comment focused on why the behavior exists and remove cursor mechanics and test-step details.Source: Coding guidelines
internal/adapter/health/circuit_breaker_test.go (1)
293-298: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the atomic load in the
t.Fatalfarguments.Both conditions load the field atomically, then the format argument reads
state.isOpenandstate.lastAttemptdirectly. These two tests run no concurrent goroutines, so-racewill not report anything today. The direct read is still inconsistent with every other access tocircuitState, and it is a copy-paste hazard for the concurrent tests in this file.♻️ Proposed change
- if atomic.LoadInt32(&state.isOpen) != 0 { - t.Fatalf("isOpen = %d after RecordSuccess, want closed (0)", state.isOpen) - } - if atomic.LoadInt64(&state.lastAttempt) != 0 { - t.Fatalf("lastAttempt = %d after RecordSuccess, want reset to 0", state.lastAttempt) - } + if isOpen := atomic.LoadInt32(&state.isOpen); isOpen != 0 { + t.Fatalf("isOpen = %d after RecordSuccess, want closed (0)", isOpen) + } + if lastAttempt := atomic.LoadInt64(&state.lastAttempt); lastAttempt != 0 { + t.Fatalf("lastAttempt = %d after RecordSuccess, want reset to 0", lastAttempt) + }Line 321 has the same pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/health/circuit_breaker_test.go` around lines 293 - 298, Update the failure arguments in the RecordSuccess assertions to reuse atomic.LoadInt32(&state.isOpen) and atomic.LoadInt64(&state.lastAttempt) instead of reading the fields directly. Apply the same correction to the matching pattern around line 321, keeping the existing conditions and messages unchanged.internal/adapter/health/client.go (1)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the retry loop variable to remove the shadowing of
attempt.Line 59 binds
attemptto the circuit-breaker probe token (int64). Line 76 declares a secondattempt(int) as the retry index. The two names carry different meanings in the same function.The current code is correct. The retry variable's scope ends at Line 101, so Lines 111-119 read the probe token. The type difference also means a wrong binding would fail to compile.
The risk is future edits. If anyone moves a recording call into the loop body, it silently binds the retry index instead of the probe token, and the correlation guard in
RecordSuccessdrops the result. Rename the loop variable to make the distinction explicit.♻️ Proposed rename
- for attempt := 0; attempt <= maxRetries; attempt++ { - if attempt > 0 { + for retry := 0; retry <= maxRetries; retry++ { + if retry > 0 { // Calculate exponential backoff delay - delay := calculateBackoffDelay(attempt) + delay := calculateBackoffDelay(retry)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/health/client.go` around lines 56 - 59, Rename the retry-loop variable in the health-check function to distinguish it from the int64 circuit-breaker probe token assigned by IsOpen. Update all references within the retry loop and preserve the existing probe-token uses in the subsequent RecordSuccess/RecordFailure calls.internal/adapter/health/circuit_breaker.go (1)
143-163: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAttempt tokens derived from
time.Now().UnixNano()can collide.
IsOpenuses the wall-clock nanosecond value as the attempt token. On a platform with coarse clock resolution, a stale-handover replacement can receive the same token value as the probe it supersedes. The correlation guard then compares equal, and the superseded probe's late result still applies. The test at Lines 124-128 ofinternal/adapter/health/circuit_breaker_test.goalready works around this exact clock behaviour.A monotonic counter removes the collision entirely and keeps the same CAS structure.
lastAttemptwould then hold the token, and staleness would need a separate stamp field.Given the extra field, this is a follow-up rather than a merge blocker.
♻️ Sketch: separate the token from the timestamp
type circuitState struct { failures int64 lastFailure int64 lastAttempt int64 + attemptID int64 // monotonic probe token; never reused isOpen int32 }Assign
attemptIDwithatomic.AddInt64(&state.attemptID, 1)when a probe is admitted, and correlate on that value instead of the timestamp.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/health/circuit_breaker.go` around lines 143 - 163, Replace wall-clock-derived attempt tokens in IsOpen with a per-endpoint monotonic counter, adding an attemptID field to the endpoint state and assigning it via atomic.AddInt64 when a probe is admitted. Store and compare lastAttempt using this unique counter token, while keeping any timestamp needed for staleness in a separate field; update RecordSuccess and RecordFailure correlation to use the new token.internal/app/handlers/handler_proxy_provider_scope_test.go (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce repetitive test comments.
Keep the regression reason in each comment. Let the test name, setup, and assertions describe the tested behaviour.
internal/app/handlers/handler_proxy_provider_scope_test.go#L14-L21: Reduce this to the fail-closed regression reason.internal/app/handlers/handler_proxy_provider_scope_test.go#L46-L50: Reduce this to the inclusive-routing regression reason.internal/app/handlers/handler_proxy_provider_scope_test.go#L75-L79: Remove repeated request and response details.internal/app/handlers/handler_proxy_provider_scope_test.go#L146-L151: Retain only why the inspector is necessary.internal/app/handlers/handler_proxy_provider_scope_test.go#L164-L171: Retain only why the registry mock differs from a simple endpoint filter.internal/app/handlers/handler_proxy_provider_scope_test.go#L189-L202: Reduce the diagnostic-masking explanation to the failure cause.As per coding guidelines, “Comment on why, not what. Concise and direct.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/handlers/handler_proxy_provider_scope_test.go` around lines 14 - 21, In internal/app/handlers/handler_proxy_provider_scope_test.go:14-21, shorten the comment to only the fail-closed regression reason for TestFilterEndpointsByProfile_FailClosedOnNoMatch. At 46-50, retain only the inclusive-routing regression reason; at 75-79, remove repeated request and response details. At 146-151, explain only why the inspector is necessary; at 164-171, explain only why the registry mock differs from a simple endpoint filter; and at 189-202, reduce the diagnostic-masking comment to the failure cause.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/handlers/dashboard/access.go`:
- Around line 176-183: Update the Warn logging in reject to include the
applicable dashboard.access_policy.allowed_hosts or allowed_cidrs configuration
guidance, matching the operator-facing 403 body; preserve the existing reason,
client_ip, and host fields.
In `@internal/app/handlers/handler_proxy_chunked_oversize_test.go`:
- Around line 77-138: Update TestSizeValidator_ChunkedProxyBody_EndToEnd to use
a configured proxy engine and send requests through the actual proxy
route/request flow instead of the inline io.ReadAll handler. Preserve the
separate direct handleProxyError coverage as a unit test, while asserting the
configured engine produces 413 for oversized chunked bodies and 200 for bodies
under the cap.
In `@internal/app/handlers/handler_proxy.go`:
- Around line 564-565: Replace the Content-Type check in
writeNoRoutableEndpoints with a genuine response-commit check, using the
existing response-writer abstraction or a wrapper that tracks whether
WriteHeader or Write has occurred. Ensure handleProxyError still runs when
headers were only prepared, while preserving the early return after an actual
response commit.
In `@internal/app/handlers/handler_status_etag_test.go`:
- Around line 210-216: Update the inverse-test comment near
TestEndpointStatusResponse_ETagStableAcrossRelativeTimeChurn to reference
TestEndpointStatusResponse_ETagChangesOnStatusFlip instead of
TestStatusResponse_ETagChangesOnRealDataChange, without changing the tests.
---
Outside diff comments:
In `@internal/adapter/proxy/sherpa/service_retry.go`:
- Around line 76-104: In the retry flow around http.NewRequestWithContext, apply
ContextModelKey to stats.Model and derive resolvedModel before constructing
proxyReq so construction failures record the translated backend model. Keep
setting the translated model header on proxyReq after successful construction,
reuse the precomputed resolvedModel for all success/failure records, and add a
regression test covering request-construction failure after translation.
---
Nitpick comments:
In `@internal/adapter/balancer/least_connection_test.go`:
- Around line 443-460: Update the sequential tie-selection test around
selector.Select to track the previously selected endpoint and fail whenever
consecutive selections have the same Name. Retain the existing 10-selection loop
and aggregate 5/5 assertions, adding the consecutive-selection check after each
successful selection.
In `@internal/adapter/balancer/least_connections.go`:
- Around line 15-21: Shorten the comments at
internal/adapter/balancer/least_connections.go:15-21 to state that tie rotation
prevents recovered endpoints from starvation; at
internal/adapter/balancer/least_connections.go:54-57 to state that retaining all
minimum-count endpoints prevents registration-order bias; at
internal/adapter/balancer/least_connection_test.go:427-433 to state that the
test prevents starvation during persistent ties; and at
internal/adapter/balancer/least_connection_test.go:463-466 to state that the
test preserves strict minimum-count selection. Keep each comment focused on why
the behavior exists and remove cursor mechanics and test-step details.
In `@internal/adapter/health/circuit_breaker_test.go`:
- Around line 293-298: Update the failure arguments in the RecordSuccess
assertions to reuse atomic.LoadInt32(&state.isOpen) and
atomic.LoadInt64(&state.lastAttempt) instead of reading the fields directly.
Apply the same correction to the matching pattern around line 321, keeping the
existing conditions and messages unchanged.
In `@internal/adapter/health/circuit_breaker.go`:
- Around line 143-163: Replace wall-clock-derived attempt tokens in IsOpen with
a per-endpoint monotonic counter, adding an attemptID field to the endpoint
state and assigning it via atomic.AddInt64 when a probe is admitted. Store and
compare lastAttempt using this unique counter token, while keeping any timestamp
needed for staleness in a separate field; update RecordSuccess and RecordFailure
correlation to use the new token.
In `@internal/adapter/health/client.go`:
- Around line 56-59: Rename the retry-loop variable in the health-check function
to distinguish it from the int64 circuit-breaker probe token assigned by IsOpen.
Update all references within the retry loop and preserve the existing
probe-token uses in the subsequent RecordSuccess/RecordFailure calls.
In `@internal/app/handlers/handler_proxy_provider_scope_test.go`:
- Around line 14-21: In
internal/app/handlers/handler_proxy_provider_scope_test.go:14-21, shorten the
comment to only the fail-closed regression reason for
TestFilterEndpointsByProfile_FailClosedOnNoMatch. At 46-50, retain only the
inclusive-routing regression reason; at 75-79, remove repeated request and
response details. At 146-151, explain only why the inspector is necessary; at
164-171, explain only why the registry mock differs from a simple endpoint
filter; and at 189-202, reduce the diagnostic-masking comment to the failure
cause.
In `@internal/app/middleware/logging_gate_test.go`:
- Around line 246-258: Shorten the comment above
TestCombinedLogging_PathMutatingHandlerStaysQuiet to state only why the
regression test exists: the handler mutates r.URL.Path, so middleware must
preserve the original route for quiet-poll classification. Remove implementation
details, target-path examples, and assertion mechanics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 229856ca-0697-43b7-a23f-67f2a3fa2e16
📒 Files selected for processing (39)
config/models.yamlinternal/adapter/balancer/least_connection_test.gointernal/adapter/balancer/least_connections.gointernal/adapter/health/checker_test.gointernal/adapter/health/circuit_breaker.gointernal/adapter/health/circuit_breaker_test.gointernal/adapter/health/client.gointernal/adapter/health/client_classify_test.gointernal/adapter/health/client_ratelimit_test.gointernal/adapter/health/types.gointernal/adapter/proxy/core/base.gointernal/adapter/proxy/core/base_test.gointernal/adapter/proxy/core/model_rewrite.gointernal/adapter/proxy/core/retry.gointernal/adapter/proxy/olla/service.gointernal/adapter/proxy/olla/service_retry.gointernal/adapter/proxy/proxy_benchmark_test.gointernal/adapter/proxy/proxy_model_stats_test.gointernal/adapter/proxy/proxy_olla_test.gointernal/adapter/proxy/sherpa/service_retry.gointernal/adapter/security/request_size_limit.gointernal/adapter/security/request_size_limit_test.gointernal/adapter/unifier/metadata_extractor_test.gointernal/adapter/unifier/model_config.gointernal/app/handlers/dashboard/access.gointernal/app/handlers/dashboard/access_test.gointernal/app/handlers/handler_provider_common.gointernal/app/handlers/handler_proxy.gointernal/app/handlers/handler_proxy_chunked_oversize_test.gointernal/app/handlers/handler_proxy_provider_scope_test.gointernal/app/handlers/handler_status.gointernal/app/handlers/handler_status_endpoints.gointernal/app/handlers/handler_status_etag_test.gointernal/app/middleware/logging.gointernal/app/middleware/logging_gate_test.gointernal/core/domain/routing.gotest/manual/config.sticky.yamltest/validate/config.validate.limits.yamltest/validate/config.validate.yaml
…le etag test comment reference
…writes instead of a content-type check
…stead of a fake inline handler
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adapter/proxy/core/retry.go`:
- Around line 73-76: Update ResponseStartedWriter to mark started when Flush is
invoked through http.NewResponseController, while preserving the underlying
flusher behavior exposed by Unwrap. Add a regression test that flushes the
wrapper via http.NewResponseController before invoking handleProxyError, and
verify no error response is appended after the response is committed.
In `@internal/app/handlers/handler_proxy_chunked_oversize_test.go`:
- Around line 265-267: Strengthen the under-cap request assertion in the
relevant chunked-body test so it verifies the request reached the upstream, not
merely that Sherpa returned http.StatusOK. Assert the expected upstream response
body or use the existing upstream request-recording mechanism, while preserving
the current status check and test setup.
In `@internal/app/handlers/handler_proxy.go`:
- Around line 89-100: Update core.ResponseStartedWriter to add FlushError and
Flush methods that mark the response started before delegating to the underlying
writer, including support for http.NewResponseController behavior. Preserve
existing WriteHeader, Write, and Unwrap semantics, and add a regression test
covering a flush followed by handleProxyError to ensure no error response is
appended after commitment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1ef3555-4588-4866-b29a-4cfed4051b41
📒 Files selected for processing (6)
internal/adapter/proxy/core/retry.gointernal/adapter/proxy/core/retry_safety_test.gointernal/app/handlers/dashboard/access.gointernal/app/handlers/handler_proxy.gointernal/app/handlers/handler_proxy_chunked_oversize_test.gointernal/app/handlers/handler_status_etag_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/app/handlers/dashboard/access.go
- internal/app/handlers/handler_status_etag_test.go
http.NewResponseController(rw).Flush() could walk past the wrapper via Unwrap() straight to the underlying writer, leaving started=false even though bytes had already reached the client. Flush/FlushError now mark started before delegating, so handleProxyError can't append an error onto a response that was committed by a flush with no prior write.
The 200 check alone would also pass if something short-circuited before the real upstream, since both look identical on status code. Checking the body proves the response genuinely came through.
…silience Olla's proxy and health packages had each hand-rolled a near-identical closed/open/half-open breaker with single-flight probe gating and ABA-safe attempt tokens, and the fixes for that kept landing in one copy and missing the other. Pulls the state machine into one type, with its own test suite covering the full machine independently of either adapter.
Keeps the endpoint map and per-endpoint CheckTimeout-derived staleness here; the state machine itself now delegates to resilience.Breaker.
circuitBreaker is now a local alias for resilience.Breaker; this package still owns the endpoint map, the cleanup sweep and its own halfOpenStaleness constant (now exported as HalfOpenStaleness since IsOpen takes it as a call parameter). Exposes the same behaviour as before, including the persisted half-open state that lets a sub-threshold half-open failure retry immediately rather than forcing a fresh full recovery wait.
Review caught that the doc comment presented the sub-threshold half-open retry as observable behaviour - it isn't reachable via the public API (failures only resets on success, so every non-closed state already has failures >= threshold). States plainly that this mirrors olla's original mechanics as future-proofing, not a load-bearing path.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
internal/adapter/resilience/circuit_breaker.go (2)
214-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
SetOpen(true)does not stamplastFailure.
SetOpen(true)storesstateOpenbut leaveslastFailureuntouched. IflastFailureis still 0, the nextIsOpencall evaluates the recovery timeout against the Unix epoch, finds it elapsed, and transitions straight to half-open. A caller that expectsSetOpen(true)to block traffic forOpenDurationgets the opposite result.NewTrippedavoids this because it sets both fields.Either stamp
lastFailureinsideSetOpen(true), or state the coupling in the doc comment so callers pair it withSetLastFailureNanos.♻️ Proposed doc clarification
// SetOpen forces the breaker to the open state (v true) or the closed state // (v false). Exported for test construction of specific scenarios; // production callers should go through RecordSuccess/RecordFailure, which -// own the state's lifecycle. +// own the state's lifecycle. +// +// SetOpen(true) does not stamp lastFailure. A breaker forced open with +// lastFailure still 0 measures its recovery timeout from the Unix epoch, so +// the next IsOpen call transitions it straight to half-open. Pair this with +// SetLastFailureNanos, or use NewTripped, when the open state must persist. func (b *Breaker) SetOpen(v bool) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/resilience/circuit_breaker.go` around lines 214 - 224, Update SetOpen so enabling the open state also stamps lastFailure, matching NewTripped and ensuring the breaker remains open for OpenDuration; leave the closed-state behavior unchanged.
132-143: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a monotonic counter for the attempt token.
The attempt token is
time.Now().UnixNano(). On platforms with a coarse wall clock, two successive admissions can produce the same value. If a superseded probe and its replacement share a token, the correlation guard inRecordSuccessandRecordFailureaccepts the stale result. The test files already work around this by backdatinglastAttemptinstead of chaining two realIsOpencalls, which is direct evidence that collisions are possible.An
atomic.Int64sequence counter removes the collision class entirely and keeps the 0 sentinel. The staleness comparison would then need a separate timestamp field, so this is a deliberate trade-off rather than a drop-in change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/resilience/circuit_breaker.go` around lines 132 - 143, Replace the time-based attempt token in the circuit-breaker admission logic with a strictly increasing atomic.Int64 sequence that preserves 0 as the unset sentinel, ensuring each probe receives a unique token. Add a separate timestamp field for probe-staleness checks, and update IsOpen plus RecordSuccess/RecordFailure to use the sequence token for correlation and the timestamp for expiration.internal/adapter/proxy/olla/circuit_breaker_test.go (1)
216-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider trimming the duplicated state-machine tests now that
circuitBreakeris an alias.
circuitBreakeris now a type alias forresilience.Breaker, so this package adds no wrapper code between the test and the state machine.TestCircuitBreaker_ABARace_SupersededProbeResultDroppedis therefore a near-copy ofTestBreaker_ABARace_SupersededProbeResultDroppedininternal/adapter/resilience/circuit_breaker_test.goand exercises the same implementation.The health copy is different:
health.CircuitBreakeris a real wrapper that adds endpoint keying and derivesprobeStalenessfromCheckTimeout, so its tests cover behaviour the shared tests cannot.Keeping the olla copy means a future state-machine change needs three test updates instead of one, which works against the consolidation this PR performs. Consider reducing the olla tests to what is olla-specific, such as
GetCircuitBreakerconstruction, theHalfOpenStalenessvalue, and the cleanup sweep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/proxy/olla/circuit_breaker_test.go` around lines 216 - 296, Remove the duplicated state-machine test TestCircuitBreaker_ABARace_SupersededProbeResultDropped from the olla package, since circuitBreaker aliases resilience.Breaker and the shared resilience test already covers this behavior. Retain or add only olla-specific coverage around GetCircuitBreaker construction, HalfOpenStaleness, and cleanup-sweep behavior.internal/adapter/proxy/proxy_olla_test.go (1)
87-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the sleep; it does not affect the assertion.
The open duration is 30 seconds. A 10 millisecond sleep cannot change the result of the following
IsOpencall, so the assertion is identical with or without it. The sleep only adds wall-clock time to the suite.If the intent is to prove the breaker stays open until the recovery timeout elapses, backdate
lastFailurewithSetLastFailureNanosinstead. That tests the boundary and stays deterministic.♻️ Proposed change
- // Should remain open for timeout period - time.Sleep(10 * time.Millisecond) + // Should remain open until the recovery timeout elapses if open, _ := cb.IsOpen(olla.HalfOpenStaleness); !open { t.Error("Circuit breaker should remain open during timeout") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/proxy/proxy_olla_test.go` around lines 87 - 91, Remove the 10-millisecond time.Sleep from the circuit-breaker assertion in the relevant test. Keep the existing IsOpen check, or if testing timeout behavior is intended, use SetLastFailureNanos to backdate lastFailure and make the boundary deterministic.internal/adapter/proxy/olla/service.go (1)
235-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a proxy-local constant for the open duration.
GetCircuitBreakertakesOpenDurationfromhealth.DefaultCircuitBreakerTimeout. That names a health-check default, but it now configures the recovery window for proxied inference requests. The two workloads have different budgets, so a later change to the health default silently changes proxy behaviour.The value carries over from the previous implementation, so this is not a regression. Since this PR consolidates breaker configuration, a proxy-local constant alongside
HalfOpenStalenesswould make the intent explicit and decouple the two subsystems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/proxy/olla/service.go` around lines 235 - 243, Define a proxy-local constant for the circuit breaker open duration alongside HalfOpenStaleness, using the current timeout value, and update Service.GetCircuitBreaker to use it instead of health.DefaultCircuitBreakerTimeout. Keep the existing resilience configuration and behavior unchanged.internal/adapter/resilience/circuit_breaker_test.go (1)
301-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a floor assertion so the race test cannot degrade to a no-op.
The test only asserts
_ = b.Tripped(). If a future change makesIsOpenalways return open, every goroutine hitscontinue,RecordSuccessandRecordFailurenever run, and the test still passes. The race coverage would then be silently lost.Count the admitted calls and assert the count is greater than zero.
♻️ Proposed floor assertion
b := New(testConfig(5)) + var admitted int64 var wg sync.WaitGroup wg.Add(goroutines) for i := range goroutines { go func(id int) { defer wg.Done() for j := range iterations { open, attempt := b.IsOpen(testProbeStaleness) if open { continue } + atomic.AddInt64(&admitted, 1) if (id+j)%3 == 0 { b.RecordFailure(attempt) } else { b.RecordSuccess(attempt) } } }(i) } wg.Wait() + // Guards against the loop degrading to a no-op that races nothing. + if atomic.LoadInt64(&admitted) == 0 { + t.Fatal("no call was admitted, so the concurrent record paths were never exercised") + } + // No panic and the breaker is still in a legal state (open or closed). _ = b.Tripped()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapter/resilience/circuit_breaker_test.go` around lines 301 - 328, Add an atomic or mutex-protected counter in TestBreaker_Concurrent_MixedTrafficNoRace, incremented whenever IsOpen returns closed and the goroutine proceeds to RecordFailure or RecordSuccess. After waiting for all goroutines, assert that this admitted-call count is greater than zero before the existing Tripped check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adapter/proxy/core/retry.go`:
- Around line 94-101: The FlushError method should restore rw.started to false
when ResponseController.Flush returns an error matching http.ErrNotSupported, so
handleProxyError can still write a proxy error response; preserve the committed
state for other flush outcomes. Update the related coverage in
internal/adapter/proxy/core/retry_safety_test.go at lines 350-360 to verify
unsupported flushes remain uncommitted.
---
Nitpick comments:
In `@internal/adapter/proxy/olla/circuit_breaker_test.go`:
- Around line 216-296: Remove the duplicated state-machine test
TestCircuitBreaker_ABARace_SupersededProbeResultDropped from the olla package,
since circuitBreaker aliases resilience.Breaker and the shared resilience test
already covers this behavior. Retain or add only olla-specific coverage around
GetCircuitBreaker construction, HalfOpenStaleness, and cleanup-sweep behavior.
In `@internal/adapter/proxy/olla/service.go`:
- Around line 235-243: Define a proxy-local constant for the circuit breaker
open duration alongside HalfOpenStaleness, using the current timeout value, and
update Service.GetCircuitBreaker to use it instead of
health.DefaultCircuitBreakerTimeout. Keep the existing resilience configuration
and behavior unchanged.
In `@internal/adapter/proxy/proxy_olla_test.go`:
- Around line 87-91: Remove the 10-millisecond time.Sleep from the
circuit-breaker assertion in the relevant test. Keep the existing IsOpen check,
or if testing timeout behavior is intended, use SetLastFailureNanos to backdate
lastFailure and make the boundary deterministic.
In `@internal/adapter/resilience/circuit_breaker_test.go`:
- Around line 301-328: Add an atomic or mutex-protected counter in
TestBreaker_Concurrent_MixedTrafficNoRace, incremented whenever IsOpen returns
closed and the goroutine proceeds to RecordFailure or RecordSuccess. After
waiting for all goroutines, assert that this admitted-call count is greater than
zero before the existing Tripped check.
In `@internal/adapter/resilience/circuit_breaker.go`:
- Around line 214-224: Update SetOpen so enabling the open state also stamps
lastFailure, matching NewTripped and ensuring the breaker remains open for
OpenDuration; leave the closed-state behavior unchanged.
- Around line 132-143: Replace the time-based attempt token in the
circuit-breaker admission logic with a strictly increasing atomic.Int64 sequence
that preserves 0 as the unset sentinel, ensuring each probe receives a unique
token. Add a separate timestamp field for probe-staleness checks, and update
IsOpen plus RecordSuccess/RecordFailure to use the sequence token for
correlation and the timestamp for expiration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56176105-2eb7-4c27-ab33-f61ead9bca4c
📒 Files selected for processing (17)
config/models.yamlinternal/adapter/health/circuit_breaker.gointernal/adapter/health/circuit_breaker_test.gointernal/adapter/proxy/benchmark_refactor_test.gointernal/adapter/proxy/core/retry.gointernal/adapter/proxy/core/retry_safety_test.gointernal/adapter/proxy/olla/circuit_breaker_test.gointernal/adapter/proxy/olla/service.gointernal/adapter/proxy/olla/service_leak_test.gointernal/adapter/proxy/olla/service_retry.gointernal/adapter/proxy/proxy_integration_test.gointernal/adapter/proxy/proxy_olla_test.gointernal/adapter/resilience/circuit_breaker.gointernal/adapter/resilience/circuit_breaker_test.gointernal/adapter/unifier/metadata_extractor_test.gointernal/adapter/unifier/model_config.gointernal/app/handlers/handler_proxy_chunked_oversize_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/adapter/unifier/model_config.go
- internal/adapter/proxy/olla/service_retry.go
- config/models.yaml
- internal/adapter/health/circuit_breaker_test.go
- internal/adapter/unifier/metadata_extractor_test.go
1 participant
Various bug fixes and consistency updates from July from Forge testing.
Summary by CodeRabbit
Bug Fixes
Improvements