Fix flaky tests - #555
Merged
ameowlia merged 13 commits intoApr 16, 2026
Merged
Conversation
The background goroutine that keeps an endpoint alive during pruning was not fully drained before the spec ended. This caused a race with the next spec's BeforeEach overwriting the shared `configObj` variable. Wait for the goroutine to exit via a `stoppedChan` before returning.
… timeout The router and integration test suites relied on Gomega's default Eventually timeout of 1s, which is too short under CI load with parallel test procs. Set it to 5s to match other test suites.
The `testHealthCheckEndpoint` helper used an HTTP client with no timeout. If a TCP connection hangs instead of being immediately refused, the `Consistently` callback blocks indefinitely, causing the test proc to stall and Ginkgo to report a suite-level timeout.
These assertions verify that something does NOT happen, which is exactly what Consistently is for. Using Eventually would wait up to the full timeout (10s) before passing, causing unnecessary slowdown.
Increase EndpointTimeout from 500ms to 2s for the HTTP keep-alive tests. The 500ms timeout left only 125ms of margin for request processing between sleeps, which is insufficient under CI load. Add a 10s read deadline on the TCP connection in the 100-Continue test to prevent hanging if the gorouter is slow to respond.
cf-foundation-community-automation
Bot
moved this to Inbox
in Application Runtime Platform Working Group
hoffmaen
marked this pull request as ready for review
April 14, 2026 19:32
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Application Runtime Platform Working Group
kart2bc
previously approved these changes
Apr 15, 2026
kart2bc
dismissed
their stale review
April 15, 2026 16:30
There are few unit failures in router_test.go been obeserved.
The new requestTimeout value needs to be updated in the necessary BeforeEach section.
The keep-alive BeforeEach only updated backendIdleTimeout but not requestTimeout. Since initializeRouter passes requestTimeout to the proxy config, the proxy still used the default 500ms timeout while the slow app slept for (2s*3)/4 = 1.5s, causing every request to time out with a 502.
plowin
previously approved these changes
Apr 16, 2026
plowin
left a comment
plowin
left a comment
Contributor
There was a problem hiding this comment.
Check if this re-triggers validation
The println("Error", err.Error()) calls unconditionally dereference
err before the Expect assertion. If TLS certificate validation races
and err is nil, this causes a nil pointer panic that crashes the
parallel proc and triggers "Ginkgo timed out waiting for all parallel
procs to report back".
HTTP clients without timeouts can hang indefinitely on stalled connections, crashing the parallel proc and causing "Ginkgo timed out waiting for all parallel procs to report back". - Set http.DefaultClient.Timeout = 10s in SynchronizedBeforeSuite - Add Timeout: 10s to all 17 custom http.Client instances in router_test.go and health_listener_test.go
net.Dial err.Error() was called without checking err != nil first. Under parallel test execution, port reuse can cause the dial to succeed unexpectedly, making err nil and triggering a panic that crashes the proc.
The cleanup order stopped NATS before the router, causing the subscriber's ClosedCB to fire log.Fatal → os.Exit(1), which kills the test proc without reporting results back to Ginkgo. Reversing the order ensures the router and its subscriber disconnect gracefully before the NATS server shuts down.
The debug server address was hardcoded to 127.0.0.1:17017, causing port collisions when multiple parallel Ginkgo procs each start a gorouter binary. This led to "bind: address already in use" failures in the integration suite, and cascading proc crashes in the router suite.
Every test suite ran in a fresh process, resetting the static port
counter to the same base (25000 + procID). When suites ran sequentially,
ports from a prior suite (e.g. integration) could still be in TCP
TIME_WAIT, causing net.Listen to fail in the next suite (e.g. router).
This triggered log.Fatal → os.Exit(1), crashing the Ginkgo proc.
Replace the static interleaved port scheme with OS-assigned ports via
net.Listen("tcp", "127.0.0.1:0"). The OS guarantees the port is free
and avoids recently-used ports in TIME_WAIT.
ameowlia
approved these changes
Apr 16, 2026
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Application Runtime Platform Working Group
rkoster
added a commit
that referenced
this pull request
Apr 20, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
Apr 20, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
Apr 23, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
May 4, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
May 12, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
May 13, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
May 27, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
Jun 4, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
rkoster
added a commit
that referenced
this pull request
Jun 10, 2026
This prevents the subscriber's ClosedCB from firing log.Fatal when NATS is stopped first, which was causing the test process to exit prematurely and leading to port binding conflicts in parallel test runs. The cleanup order is now: 1. Terminate gorouter session 2. Stop NATS server 3. Clean up test files This matches the fix from upstream PR #555 (commit b2bf830) which resolved similar issues in router/router_test.go.
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
4 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.
Summary
Fix multiple sources of test flakiness across the gorouter test suites (router, integration, registry). These fixes address intermittent failures observed under CI load with 7 parallel Ginkgo nodes.
Changes
BeforeEachoverwriting the sharedconfigObj. Fixed with astoppedChanpattern to drain the goroutine before the spec exits.SetDefaultEventuallyTimeout(10s)globally inrouter_suite_test.goandintegration/init_test.go. Both suites relied on Gomega's 1s default, which is too short under CI load.Timeout: 500msto health check HTTP client intestHealthCheckEndpoint— Anhttp.Client{}with no timeout can hang indefinitely, causing Ginkgo to report "timed out waiting for all parallel procs".Eventually(...).ShouldNotwithConsistently(...).ShouldNotinhealth_listener_test.goandintegration/main_test.go—Eventually(...).ShouldNot(...)waits the full timeout (now 10s) before passing, whileConsistentlyis the semantically correct choice for "this should never happen" assertions.EndpointTimeoutfrom 500ms to 2s in the HTTP keep-alive context (only 125ms margin was left for request processing), and add a 10s read deadline on the raw TCP connection in the 100-Continue test.Backward Compatibility
Breaking Change? No
Note on AI usage
Parts of this code and tests were developed with assistance from Claude Code (claude-opus-4-20250514).