fix: resolve proxy transport false-positives - #3900
Conversation
aheritier
added
area/providers/gemini
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The proxy transport logic fix is well-motivated and the guard correctly preserves the main code paths (Unix socket and Windows HTTP CONNECT). One edge case remains: bare-TCP proxy connection failures (without the proxyconnect tcp prefix) are now silently classified as target-host errors rather than proxy failures, because "connect: connection refused" was removed from proxyErrorPatterns. See inline comment for details.
|
|
||
| // Check for common proxy socket failure patterns | ||
| // Target TCP connection errors are target host errors, not proxy socket failures. | ||
| if strings.Contains(errStr, "dial tcp") && !strings.Contains(errStr, "proxyconnect tcp") { |
There was a problem hiding this comment.
[medium] Removal of "connect: connection refused" pattern may leave bare-TCP proxy failures undetected
The PR removes "connect: connection refused" from proxyErrorPatterns (previously the safety net for the case where a socket exists but nothing is listening). The replacement relies on two assumptions: (1) every genuine proxy connection failure will contain "proxyconnect tcp", and (2) every "dial tcp … connect: connection refused" without "proxyconnect tcp" is a target-host error.
Assumption (1) holds for standard Go net/http HTTP-CONNECT proxy paths — those always produce a proxyconnect tcp: … prefix. However if the proxy transport is configured as a direct TCP dial (not an HTTP CONNECT tunnel), Go wraps the failure without that prefix, yielding a bare "dial tcp 127.0.0.1:<port>: connect: connection refused" error. That string hits the new early-return guard at line 152 and returns false, so disableProxy() is never called and the agent keeps retrying against the dead proxy.
In practice this codebase appears to go through HTTP CONNECT, so the risk is low — but the gap is real and there is no test covering the bare-TCP proxy case. Consider re-adding the pattern with a tighter guard (e.g. only match when the error also contains "proxyconnect tcp" or "dial unix"), or add a comment documenting why bare-TCP proxy failures cannot occur here.
| Confidence | Score |
|---|---|
| 🟡 moderate | 60/100 |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The proxy transport fix (isProxySocketError) correctly distinguishes target-host TCP dial errors from upstream proxy tunnel failures using the "dial tcp" / "proxyconnect tcp" guard. The three new test cases cover the key boundary conditions. Windows linter suppressions (nolint:gosec, nolint:noctx) are appropriate for the contexts. The %w error-wrapping changes are valid under Go 1.26.5.
Lower-confidence findings (not posted inline)
-
[medium]
pkg/desktop/transport/transport.go:152—"connect: connection refused"removed fromproxyErrorPatterns— may affect proxy fallback when Unix socket exists but is not listening (confidence: 🟠 weak 52/100)The PR removes the
"connect: connection refused"pattern. For Unix socket errors, Go's net package typically formats them as"dial unix /path/to/socket: connect: connection refused", which still matches the remaining"dial unix"pattern — so on Linux/macOS this is likely safe. However, the safety of the removal depends on Go runtime error-message formatting across all supported platforms; if an OS or wrapper ever produces a bare"connect: connection refused"without the"dial unix"prefix, proxy socket outages during startup (socket file present, process not yet listening) would now fail hard rather than falling back to direct transport. Worth a manual cross-platform spot-check before merging.
17ddb86 to
5c9d16d
Compare
Sayt-0
left a comment
Sayt-0
left a comment
There was a problem hiding this comment.
The core fix is sound: the proxy is only ever reached through a Unix socket or a Windows named pipe (pkg/desktop/socket), so a genuine proxy failure surfaces as dial unix or proxyconnect tcp, never as a bare dial tcp. The new guard correctly stops target-host refusals from disabling the proxy for 30 seconds.
Blocking points before merge:
| # | Item | Where |
|---|---|---|
| 1 | PR title and description do not match the diff | description |
| 2 | Duplicate test case | transport_test.go |
| 3 | Misleading pattern comment | transport.go |
On point 1: the description lists changes to pkg/selfupdate/exec_windows.go, pkg/tools/builtin/backgroundjobs/cmd_windows.go and pkg/tools/builtin/shell/cmd_windows.go (%w wrapping, nolint:gosec, nolint:noctx), but none of these files are part of the diff. It also states "3 new unit test boundary cases" while 5 were added. Title and description need to be rewritten to describe only what the PR actually changes: the transport guard, the new tests, and the staticcheck SA5011 fix in the Gemini test.
| // Target TCP connection errors are target host errors, not proxy socket failures. | ||
| // Note: If the agent were configured to use a bare-TCP direct dial proxy (rather than | ||
| // HTTP CONNECT), a proxy connection failure would produce a "dial tcp" error without | ||
| // "proxyconnect tcp", which would be falsely classified as a target host error here. | ||
| // In practice, Docker Agent only uses HTTP CONNECT proxies or Unix sockets, so this | ||
| // early return correctly protects target host outages from disabling the proxy. |
There was a problem hiding this comment.
This comment block is longer than the convention used in this file. The hypothetical bare-TCP proxy case can be stated in two lines:
| // Target TCP connection errors are target host errors, not proxy socket failures. | |
| // Note: If the agent were configured to use a bare-TCP direct dial proxy (rather than | |
| // HTTP CONNECT), a proxy connection failure would produce a "dial tcp" error without | |
| // "proxyconnect tcp", which would be falsely classified as a target host error here. | |
| // In practice, Docker Agent only uses HTTP CONNECT proxies or Unix sockets, so this | |
| // early return correctly protects target host outages from disabling the proxy. | |
| // A bare "dial tcp" error is a target host failure, not a proxy socket failure: | |
| // the proxy is only reached via Unix socket or named pipe, never plain TCP. |
| proxyErrorPatterns := []string{ | ||
| "no such file or directory", // Socket file deleted | ||
| "connect: connection refused", // Socket exists but no listener | ||
| "connect: connection refused", // Socket exists but no listener (Unix or Windows named pipe) |
There was a problem hiding this comment.
Windows named pipe dial failures (winio) do not produce connect: connection refused; they surface as messages like "The system cannot find the file specified". The parenthetical is misleading:
| "connect: connection refused", // Socket exists but no listener (Unix or Windows named pipe) | |
| "connect: connection refused", // Socket exists but no listener |
| { | ||
| name: "target TCP connection refused", | ||
| errStr: "dial tcp 127.0.0.1:8080: connect: connection refused", | ||
| expected: false, | ||
| }, |
There was a problem hiding this comment.
Duplicate of the "bare-TCP proxy failure" case above (same errStr, same expected). One of the two should be removed:
| { | |
| name: "target TCP connection refused", | |
| errStr: "dial tcp 127.0.0.1:8080: connect: connection refused", | |
| expected: false, | |
| }, |
| // "proxyconnect tcp", which would be falsely classified as a target host error here. | ||
| // In practice, Docker Agent only uses HTTP CONNECT proxies or Unix sockets, so this | ||
| // early return correctly protects target host outages from disabling the proxy. | ||
| if strings.Contains(errStr, "dial tcp") && !strings.Contains(errStr, "proxyconnect tcp") { |
There was a problem hiding this comment.
Behavioral note worth confirming: previously, a target refusal surfaced through the proxy triggered a direct-transport retry, which could succeed when the proxy resolves loopback differently from the agent process. With the guard, the error is now returned as-is with no direct fallback. This appears to be the intended trade-off, but it deserves an explicit mention in the PR description.
Piyush0049
changed the title
|
@Sayt-0 I have updated the title and description according to the changes. |
|
There was a ci error from my side. I have fixed it. @Sayt-0 Please re-trigger the ci tests whenever you get time. |
4 participants
Description
This pull request addresses a critical logic bug in the Docker Desktop proxy transport and resolves a
staticcheckwarning in the Gemini tests.Proxy Error Classification Fix
Before:
The
isProxySocketErrorfunction used a broad string match for"connect: connection refused". When an agent attempted to reach an offline target service (e.g.dial tcp 127.0.0.1:8080: connect: connection refused), the transport misclassified this target host failure as a dead Docker Desktop proxy socket. This falsely triggered a 30-second global disablement of the proxy, routing all subsequent outbound agent traffic directly and bypassing Docker Desktop networking.After:
Added an explicit guard to exclude direct target TCP dial errors. The function now correctly distinguishes between target host connection failures (
dial tcp) and upstream proxy tunnel failures (proxyconnect tcp), ensuring the proxy is only disabled during genuine proxy socket outages.Note: With this guard, a target refusal surfaced through the proxy is returned as-is with no direct fallback.
Linter Fixes
require.NoErrorandrequire.NotNilto satisfy thestaticcheck SA5011control-flow analyzer.Testing
transport_test.goto explicitly verify target TCP refusal and proxy tunnel refusal logic.golangci-lint runpasses with 0 issues across the entire repository.go test ./...passes cleanly on all modified packages.