fix: return reaper handshake errors - #3828
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
Walkthrough
ChangesReaper handshake handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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 `@reaper.go`:
- Around line 545-548: Update Connect around the r.handshake(conn) call to apply
the connection context’s deadline to conn before starting the synchronous
handshake, ensuring the existing five-second timeout bounds io.ReadFull. After a
successful handshake, clear the connection deadline before returning; preserve
the current close-and-wrapped-error behavior when the handshake fails.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: acf12f59-28b9-4f3e-9b93-27143c37a785
📒 Files selected for processing (2)
reaper.goreaper_test.go
| if err := r.handshake(conn); err != nil { | ||
| conn.Close() | ||
| return nil, fmt.Errorf("handshake reaper %s: %w", r.Endpoint, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files containing reaper.go:\n'
fd -a 'reaper\.go$' . || true
printf '\nGit status/stat:\n'
git diff --stat || true
printf '\nInspect reaper.go outline around Connect/handshake:\n'
if [ -f reaper.go ]; then
wc -l reaper.go
ast-grep outline reaper.go --match Connect --view expanded || true
ast-grep outline reaper.go --match handshake --view expanded || true
sed -n '500,580p' reaper.go | cat -n
fi
printf '\nSearch for SetDeadline and Connect context usage:\n'
rg -n "SetDeadline|DialContext|context\.(WithTimeout|WithDeadline)|func \(.*Reaper.*\) (Connect|connect|handshake)|ReadFull|Write\(" reaper.go || trueRepository: testcontainers/testcontainers-go
Length of output: 3604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspect Connect and connect usage loop:\n'
sed -n '450,536p' reaper.go | cat -n
printf '\nImports for time/io/types needed by proposed fix:\n'
sed -n '1,60p' reaper.go | cat -n
printf '\nCheck current connection deadline handling across reaper.go:\n'
rg -n "SetDeadline|SetReadDeadline|SetWriteDeadline|Deadline|context\.Context" reaper.go || true
printf '\nBehavioral probe: net.Conn deadlines are ignored by DialContext and cancel writes in a mock-like scenario is not possible here but demonstrate Go semantics with read-only source:\n'
python3 - <<'PY'
from pathlib import Path
p = Path("reaper.go")
s = p.read_text()
checks = {
"Connect creates 5s context": "context.WithTimeout(context.Background(), time.Second*5)" in s,
"connect Dial uses context": 'd.DialContext(ctx, "tcp", r.Endpoint)' in s,
"handshake follows DialContext": ") != nil" in s.split("if err := d.DialContext(ctx, \"tcp\",")[1].split("if err := r.handshake(conn)")[0],
"handshake Write unbounded": "conn.Write(filters)" in s and "SetWriteDeadline" not in s,
"handshake ReadFull unbounded": "io.ReadFull(conn, buf)" in s and "SetReadDeadline" not in s,
"current timeout applied after Dial": "SetDeadline" in s.split("conn, err := d.DialContext(ctx, \"tcp\",")[1].split("if err := r.handshake(conn)")[0].split("return terminationSignal")[0],
}
for k, v in checks.items():
print(f"{k}: {v}")
PYRepository: testcontainers/testcontainers-go
Length of output: 6809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspect newReaper retry behavior:\n'
sed -n '380,480p' reaper.go | cat -n
printf '\nBehavioral probe of source invariants:\n'
python3 - <<'PY'
from pathlib import Path
s = Path("reaper.go").read_text()
try:
connect = s[s.index("func (r *Reaper) connect(ctx context.Context)"):s.index("func (r *Reaper) handshake(conn net.Conn)", s.index("func (r *Reaper) connect(ctx context.Context)"))]
except ValueError:
connect = s[s.index("func (r *Reaper) connect(ctx context.Context)"):s.index("func (r *Reaper) handshake(conn net.Conn)", s.index("func (r *Reaper) connect(ctx context.Context)"))]
print("connect source contains Dial then handshake:", connect.find("DialContext") != -1 and connect.index("DialContext") < connect.index("handshake"))
print("connect source does not contain SetDeadline before handshake:", "SetDeadline" not in connect)
print("current timeout applied after Dial:", "SetDeadline" not in connect.split("conn, err := d.DialContext(ctx, \"tcp\", r.Endpoint)")[1].split("handshake(conn)")[0].rstrip())
connect_fn = s[s.index("func (r *Reaper) connect(ctx context.Context)"):s.index("func (r *Reaper) handshake(conn net.Conn)", s.index("func (r *Reaper) connect(ctx context.Context)") + 1)]
print("connect source line count approximately:", len(connect_fn.splitlines()))
PYRepository: testcontainers/testcontainers-go
Length of output: 4156
🌐 Web query:
Go net TCPConn SetDeadline io.ReadFull context cancellation documentation
💡 Result:
In Go, the net.TCPConn deadline methods and io.ReadFull interact in a specific way due to the design of network deadlines in the net package [1][2]. Deadlines are absolute points in time [1]. When you call SetReadDeadline or SetDeadline on a net.TCPConn, you are setting a threshold that applies to all future I/O operations and any currently-blocked I/O operations on that connection [1][2][3]. Interaction with io.ReadFull: 1. io.ReadFull is a utility function that performs multiple calls to the underlying Read method of the connection until the provided buffer is filled or an error occurs [4]. 2. Because a deadline applies to all future reads until explicitly changed or cleared, a single SetReadDeadline call effectively covers the entire duration of an io.ReadFull operation [4]. If the deadline is reached while io.ReadFull is still waiting for more data, the internal Read call will return an error that wraps os.ErrDeadlineExceeded [1][2]. 3. Deadlines do not automatically reset after an I/O operation [5][6]. If you set a deadline before calling io.ReadFull, that deadline remains in effect for any subsequent reads until you either set a new deadline or clear it by passing a zero time value (time.Time{}) [1][5][7]. Context Cancellation: The net.Conn interface (including TCPConn) does not natively support context cancellation for I/O operations [1][8]. To cancel an ongoing read operation using a context, you must implement a mechanism to monitor the context and, upon cancellation, trigger the deadline to expire immediately [8][3]. This is typically done by calling conn.SetReadDeadline(time.Now) from a separate goroutine when the context is canceled [3]. This forces any currently blocked Read call (including those inside io.ReadFull) to return immediately with a timeout error [1][3]. Summary of Best Practices: - Always set a new deadline before an operation if you want it to have a specific timeout [5][6]. - Clear the deadline (SetReadDeadline(time.Time{})) if you no longer want the connection to time out [1][7]. - To support context-based cancellation, use a select block with the context's Done channel to call SetReadDeadline(time.Now) as soon as the context is canceled [8][3].
Citations:
- 1: https://pkg.go.dev/net
- 2: https://godoc.org/net
- 3: https://groups.google.com/g/golang-nuts/c/VPVWFrpIEyo
- 4: https://groups.google.com/g/golang-nuts/c/DfuGPeeolxU
- 5: https://stackoverflow.com/questions/49358216/setdeadline-for-golang-tcp-connection
- 6: https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
- 7: https://stackoverflow.com/questions/72850929/is-golang-conn-setdeadline-for-the-next-operation-only-or-forever
- 8: https://github.com/golang/go/blob/2403e594/src/net/http/netconn_test.go
Bound the synchronous handshake by the connection timeout.
DialContext only bounds dialing; once the TCP connection is established, io.ReadFull can block indefinitely if the peer accepts the socket but never ACKs. Apply the context deadline to conn before the handshake and clear it after success so Connect respects its five-second 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 `@reaper.go` around lines 545 - 548, Update Connect around the
r.handshake(conn) call to apply the connection context’s deadline to conn before
starting the synchronous handshake, ensuring the existing five-second timeout
bounds io.ReadFull. After a successful handshake, clear the connection deadline
before returning; preserve the current close-and-wrapped-error behavior when the
handshake fails.
1 participant
Related issues
What does this PR do?
This PR makes
Reaper.connectperform the Ryuk handshake synchronously before returning a successful connection.If the handshake fails, the connection is closed and the handshake error is returned to the caller. The existing retry path can then handle the failure instead of treating the reaper connection as successful.
It also adds a regression test using a local TCP listener that returns an invalid ACK, verifying that
connectreturns an error and no termination channel.Why is it important?
Previously, handshake failures were only logged inside the connection goroutine.
Reaper.connectstill returned a non-nil termination channel and nil error, so callers could believe the reaper was connected even though Ryuk had rejected or failed the handshake.Returning the error makes reaper startup failures visible and retryable.
How to test this PR