Refactor to use httputil.ReverseProxy#342
Merged
mostlygeek merged 3 commits intomostlygeek:mainfrom Oct 13, 2025
Merged
Conversation
dwrz
changed the title
WalkthroughValidate model Proxy URLs during config load; add an httputil.ReverseProxy to Process and delegate ProxyRequest to it with a ModifyResponse hook to disable buffering for SSE; add a test response recorder to support CloseNotify and adjust tests for upstream kill/streaming behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant P as Process
participant RP as ReverseProxy
participant U as Upstream
Note over P: Process initialized with ReverseProxy (ModifyResponse hook)
C->>P: HTTP request for proxied model
P->>RP: ServeHTTP(w, r)
RP->>U: Forward request upstream
U-->>RP: Response (may be streaming SSE)
alt Content-Type == text/event-stream
Note over RP: ModifyResponse sets X-Accel-Buffering: no
end
RP-->>C: Streamed/regular response proxyed back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
proxy/config/config.go(2 hunks)proxy/config/model_config.go(2 hunks)proxy/process.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/config/config.goproxy/config/model_config.goproxy/process.go
🧬 Code graph analysis (1)
proxy/process.go (1)
proxy/config/model_config.go (1)
ModelConfig(11-40)
🪛 GitHub Actions: Linux CI
proxy/config/config.go
[error] Command 'go test -count=1 ./proxy/...' failed during 'make test-all'.
proxy/config/model_config.go
[error] Command 'go test -count=1 ./proxy/...' failed during 'make test-all'.
proxy/process.go
[error] 447-447: TestProcess_AutomaticallyStartsUpstream failed with panic: runtime error: invalid memory address or nil pointer dereference during ProxyRequest (nil pointer dereference in reverse proxy).
[error] Command 'go test -count=1 ./proxy/...' failed during 'make test-all'.
🪛 GitHub Actions: Windows CI
proxy/process.go
[error] 447-447: Runtime panic: invalid memory address or nil pointer dereference in reverse proxy handling during TestProcess_AutomaticallyStartsUpstream. (Test failure in 'make test-all')
Refactor manual HTTP proxying logic in Process.ProxyRequest to use the standard library's httputil.ReverseProxy.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/config/config.go(2 hunks)proxy/process.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/process.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/config/config.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (1)
proxy/config/config.go (1)
6-6: LGTM!The
net/urlimport is correctly added to support URL validation.
Comment on lines
+346
to
+351
| // Validate the proxy URL. | ||
| if _, err := url.Parse(modelConfig.Proxy); err != nil { | ||
| return Config{}, fmt.Errorf( | ||
| "model %s: invalid proxy URL: %w", modelId, err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify if stricter URL validation is needed.
The validation correctly catches URL parse errors. However, url.Parse is permissive and accepts empty strings, relative URLs, and URLs without scheme or host. Since the PR introduces httputil.ReverseProxy, which typically requires complete URLs with scheme and host, verify whether additional validation is needed.
Run the following script to check how the reverse proxy handles the parsed URL:
🏁 Script executed:
#!/bin/bash
# Description: Check if reverse proxy validates URL structure (scheme/host)
# Expected: Find validation or error handling for incomplete URLs
# Search for reverse proxy setup and URL usage
ast-grep --pattern 'reverseProxy = &httputil.ReverseProxy{$$$}'
# Search for proxy URL validation in process.go
rg -n --type=go -C5 'modelConfig.Proxy|ProxyURL' proxy/process.goLength of output: 0
🏁 Script executed:
#!/bin/bash
echo "Searching for NewSingleHostReverseProxy usage..."
rg -nP 'NewSingleHostReverseProxy' -tgo -C3
echo "Searching for ReverseProxy struct literal..."
rg -nP '&httputil.ReverseProxy' -tgo -C3
echo "Searching for modelConfig.Proxy usage..."
rg -n 'modelConfig.Proxy' -tgo -C5Length of output: 4449
Enforce complete proxy URL validation
After parsing, ensure both u.Scheme and u.Host are non-empty (e.g.
u, err := url.Parse(modelConfig.Proxy)
if err != nil || u.Scheme == "" || u.Host == "" {
return Config{}, fmt.Errorf("model %s: proxy must include scheme and host", modelId)
}
```)
<details>
<summary>🤖 Prompt for AI Agents</summary>In proxy/config/config.go around lines 346 to 351, the URL validation only
checks parse error and may accept incomplete URLs; update the logic to parse the
proxy into u and treat it as invalid if err != nil or u.Scheme == "" or u.Host
== "", returning a formatted error like "model %s: proxy must include scheme and
host" with modelId and the underlying error when present; ensure you use the
parsed u variable (not the raw string) for these checks and preserve existing
error-wrapping semantics.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
Update to handle behavior with httputil.ReverseProxy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/process_test.go (1)
439-441: LGTM with optional suggestion for test robustness.The assertion correctly verifies that the full message was not delivered due to the upstream kill. The approach is reasonable given the non-deterministic timing of when the kill occurs during the response.
Optional: Consider adding an additional assertion to verify the response is either empty or a prefix of "12345" to more precisely capture the expected partial-response behavior:
// Upstream may be killed mid-response. // Assert an incomplete or partial response. assert.NotEqual(t, "12345", w.Body.String()) +// Verify it's either empty or a valid prefix +body := w.Body.String() +if body != "" { + assert.True(t, strings.HasPrefix("12345", body), + "partial response should be a prefix of '12345', got: %s", body) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/process_test.go(1 hunks)proxy/proxymanager_test.go(26 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/proxymanager_test.goproxy/process_test.go
🔇 Additional comments (2)
proxy/proxymanager_test.go (2)
24-48: Well-structured test utility for streaming support.The
TestResponseRecordercorrectly implementshttp.CloseNotifierto enable testing of streaming behavior withhttputil.ReverseProxy. The buffered channel and helper methods are appropriately designed.Note: While
http.CloseNotifierwas deprecated in Go 1.7 in favor ofRequest.Context()cancellation, it remains necessary for compatibility withhttputil.ReverseProxy's internal checks for this interface. Based on learnings.
66-66: LGTM! Tests properly updated for streaming support.All test functions have been correctly migrated to use
CreateTestResponseRecorder()instead ofhttptest.NewRecorder(). This ensures that the tests can properly handle streaming scenarios that rely on theCloseNotifyinterface, which is required by the newhttputil.ReverseProxyintegration.Also applies to: 103-103, 145-145, 188-188, 241-241, 340-340, 416-416, 505-505, 510-510, 548-548, 556-556, 600-600, 618-618, 624-624, 676-676, 711-711, 745-745, 813-813, 841-841, 849-849, 870-870, 898-898, 929-929, 964-964, 996-996, 1017-1017, 1109-1109, 1138-1138
Contributor
Author
|
@mostlygeek -- I gave the tests a quick look. It looks like the refactor to I ran tests with the Go race detector enabled: Here's the output I get on the |
Owner
Owner
|
Thanks, looks good! Also all data races fixed in #349 |
mostlygeek
added a commit
that referenced
this pull request
Oct 26, 2025
Switching to use httputil.ReverseProxy in #342 introduced a possible panic if a client disconnects while streaming the body. Since llama-swap does not use http.Server the recover() is not automatically there. - introduce a recover() in Process.ProxyRequest to recover and log the event - add TestProcess_ReverseProxyPanicIsHandled to reproduce and test the fix fixes: #362
mostlygeek
added a commit
that referenced
this pull request
Oct 26, 2025
Switching to use httputil.ReverseProxy in #342 introduced a possible panic if a client disconnects while streaming the body. Since llama-swap does not use http.Server the recover() is not automatically there. - introduce a recover() in Process.ProxyRequest to recover and log the event - add TestProcess_ReverseProxyPanicIsHandled to reproduce and test the fix fixes: #362
0uep
pushed a commit
to lynxai-team/llama-swap
that referenced
this pull request
Nov 21, 2025
Switching to use httputil.ReverseProxy in mostlygeek#342 introduced a possible panic if a client disconnects while streaming the body. Since llama-swap does not use http.Server the recover() is not automatically there. - introduce a recover() in Process.ProxyRequest to recover and log the event - add TestProcess_ReverseProxyPanicIsHandled to reproduce and test the fix fixes: mostlygeek#362
rohitpaul
pushed a commit
to rohitpaul/llama-swap
that referenced
this pull request
Mar 29, 2026
* Refactor to use httputil.ReverseProxy Refactor manual HTTP proxying logic in Process.ProxyRequest to use the standard library's httputil.ReverseProxy. * Refactor TestProcess_ForceStopWithKill test Update to handle behavior with httputil.ReverseProxy. * Fix gin interface conversion panic
rohitpaul
pushed a commit
to rohitpaul/llama-swap
that referenced
this pull request
Mar 29, 2026
Switching to use httputil.ReverseProxy in mostlygeek#342 introduced a possible panic if a client disconnects while streaming the body. Since llama-swap does not use http.Server the recover() is not automatically there. - introduce a recover() in Process.ProxyRequest to recover and log the event - add TestProcess_ReverseProxyPanicIsHandled to reproduce and test the fix fixes: mostlygeek#362
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
2 participants
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.
Refactor manual HTTP proxying logic in Process.ProxyRequest to use the standard library's httputil.ReverseProxy.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests