fix: bound the reaper connect dial so the spawner backoff can retry - #3839
fix: bound the reaper connect dial so the spawner backoff can retry#3839ghaering wants to merge 1 commit into
Conversation
connect() dialed with the caller's context, which has no deadline. With dropped SYNs (conntrack/DNAT race under container churn) the dial sits in the kernel's ~2min connect timeout. One attempt eats the whole 20s backoff budget, so the ETIMEDOUT retry never happens. Bound the dial to 5s like Connect() does. The retry dials from a fresh source port and gets a fresh conntrack entry.
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
Walkthrough
ChangesReaper dial timeout
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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_dial_bound_test.go`:
- Around line 15-30: Make the connect timeout regression test deterministic by
injecting a dialer or dial function into Reaper.connect that blocks until the
derived child context expires, rather than dialing 192.0.2.1. Assert that the
returned error is a context deadline or timeout error, retain the retryError
classification check, and tighten the elapsed-time assertion to five seconds
plus a small scheduling margin.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 60d5251c-1cb1-4c92-90a9-00f22d2b5075
📒 Files selected for processing (2)
reaper.goreaper_dial_bound_test.go
| r := &Reaper{Endpoint: "192.0.2.1:8080"} // TEST-NET-1, not routed | ||
| start := time.Now() | ||
| _, err := r.connect(context.Background()) | ||
| elapsed := time.Since(start) | ||
|
|
||
| if err == nil { | ||
| t.Fatal("expected dial error") | ||
| } | ||
| if elapsed > 10*time.Second { | ||
| t.Fatalf("dial not bounded: took %v", elapsed) | ||
| } | ||
| s := &reaperSpawner{} | ||
| rerr := s.retryError(err) | ||
| var perm *backoff.PermanentError | ||
| if errors.As(rerr, &perm) { | ||
| t.Fatalf("timeout classified permanent, backoff would stop: %v", rerr) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the timeout regression test deterministic and enforce the five-second bound.
192.0.2.1 does not guarantee a SYN timeout on every CI host. The dial can return ENETUNREACH or ECONNREFUSED immediately. reaper.go Lines 237-248 can classify ENETUNREACH as permanent, and an immediate ECONNREFUSED lets an implementation without the new timeout pass this test.
Inject the dialer or dial function and block it until the child context expires. Assert a deadline or timeout error. Set the elapsed limit to five seconds plus a small scheduling margin instead of ten seconds.
🤖 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_dial_bound_test.go` around lines 15 - 30, Make the connect timeout
regression test deterministic by injecting a dialer or dial function into
Reaper.connect that blocks until the derived child context expires, rather than
dialing 192.0.2.1. Assert that the returned error is a context deadline or
timeout error, retain the retryError classification check, and tighten the
elapsed-time assertion to five seconds plus a small scheduling margin.
1 participant
What does this PR do?
Bound the dial in
Reaper.connect()to 5s, same asConnect(). Add aregression test.
Why is it important?
The spawner's connect check dials with the caller's context, which has
no deadline. With dropped SYNs (we hit a conntrack/DNAT race under
container churn) the dial sits in the kernel's ~2min connect timeout.
One attempt eats the whole 20s backoff budget, so the ETIMEDOUT retry
never happens. With a 5s bound the retry dials from a fresh source port
(fresh conntrack entry) and connects.
Related issues
How to test this PR
Dials 192.0.2.1:8080 (TEST-NET-1, not routed), checks the dial fails
within the bound and is classified retryable. Without the fix it takes
~2min and fails. On networks that answer for TEST-NET-1 the dial fails
instantly, so the test passes without exercising the timeout path.
Follow-ups
Later maybe: blackhole locally with a backlog-0 listener (Linux only)
instead of relying on TEST-NET-1.