test(tunnel): wait for listener shutdown after context cancellation - #1256
Conversation
The test asserted an asynchronous effect with a fixed 100ms sleep: after cancel(), the listener is closed by a separate goroutine watching the context, so under -race on a loaded macOS runner the close can land after the dial and the connection succeeds. Poll until the port refuses connections (bounded at 2s) instead, matching the existing health-check shutdown test. Signed-off-by: Samuel K <skevetter@pm.me>
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
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 |
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
|
@greptileai review |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
The loop returns on a dial error before checking the two-second deadline. If the deadline fires while DialTimeout is still running and the listener closes afterward, the test passes even though shutdown exceeded the intended bound. Check the deadline or elapsed time before treating the dial failure as success.
|
On the Greptile P2 (dial failure bypasses deadline) - non-blocking per its own summary, so explaining rather than applying, per the review workflow: The deadline is a failure bound, not a latency contract. Normal shutdown completes in microseconds after cancel(); the 2s exists only to turn "shutdown never happens" into a test failure instead of a hang. The overshoot the finding describes is bounded by a single dial cycle: the last in-flight DialTimeout can land at most ~50ms (its own timeout, loopback RTT is sub-millisecond) past the deadline. Asserting elapsed < deadline on the success path would make the test strictly enforce 2.000s vs 2.05s, which is not a property the tunnel contract has. The loop also deliberately mirrors the existing structure of TestLocalTunnel_HealthCheckShutdown in the same file, which has the same shape. If a strict bound is ever wanted, the fix is a three-line select/default on the deadline channel before the success return - happy to add it on request. |
|
Tick the box to add this pull request to the merge queue (same as
|
1 participant
Root cause
TestLocalTunnel_ContextCancellationflaked on the macOS Build CLI Binary run on #1248 (29bd1b4) withexpected connection to be refused after context cancellation, then passed on the same code ~2h earlier.The test asserted an asynchronous effect with a fixed 100ms sleep:
cancel()cancels the parent context. Propagation to the tunnel's child context is synchronous, so by the timecancel()returns,tunnelCtx.Done()is closed.<-tunnelCtx.Done(); listener.Close()inNewLocalTunnel). That goroutine must be scheduled before the port stops accepting.DialTimeoutsucceeded, and the test failed.The macOS "Build CLI Binary" job runs
go test ./... -race -short(goreleaser pre-build hook): race-instrumented test binaries from many packages run in parallel on a 3-vCPU runner, where a >100ms scheduling delay for one goroutine is entirely plausible. Same code passing 2h earlier is consistent with load-dependent scheduling, not a code regression (#1248 does not touchpkg/tunnel).Evidence
local_listener_test.go:189).-overlay, no source change), the unmodified test fails 100% with the exact CI message. At 0ms artificial delay it passes, including 400 iterations under CPU saturation on 2 cores with-race.Close()is not involved in this test.Fix
Test-side only: replace the fixed sleep with polling until the port refuses connections, bounded at 2s, matching the pattern the same file already uses in
TestLocalTunnel_HealthCheckShutdown. The deadline is a failure bound, not a wait - the test passes as soon as shutdown is observed (normally immediate). No blanket retry, no inflated timeout: the scheduling assumption is removed rather than widened.The product contract is unchanged and correct: context cancellation shuts the tunnel down promptly but asynchronously, so the test must observe the effect rather than assume a delay.
Verification
-race.pkg/tunnelsuite passes with-race -short.gofmtclean.