Flaky CI: ObsConnMetrics net.connections.accepted monotonic test loses 1 of 5 connections on slow runners

[mwfj]

Summary

TestNetAcceptedIsMonotonic in test/observability/observability_connection_metrics_test.h fails intermittently on GitHub Actions shared runners with delta=4.000000 expected>=5.

Failing test

[FAIL] ObsConnMetrics: net.connections.accepted monotonic over N opens
       Error: delta=4.000000 expected>=5

Root cause

The test opens 5 TCP connections in a tight loop with immediate close + 20ms inter-connection sleep (line 484-496), then polls for up to 2 seconds for the accepted counter to reach 5.

On slow/contended CI runners, one of the connect+close cycles completes before the accept dispatcher processes the connection — the TCP RST arrives before (or nearly simultaneously with) the accept callback that increments reactor.net.connections.accepted. The counter only reaches 4.

The 20ms inter-connection sleep and 2-second poll deadline are insufficient on GitHub Actions shared runners under load.

Observed environment

• GitHub Actions ubuntu-latest shared runner
• gcc build, no sanitizers
• CI run: https://github.com/mwfj/ebbry/actions/runs/26506306129/job/78059475145
• Same binary passes locally and on most CI runs

Suggested fix options

1. Increase inter-connection sleep to 50-100ms to give the accept dispatcher more time to process each connection before the next one arrives.

2. Send an HTTP request before closing — instead of bare connect+close, send a minimal GET / HTTP/1.1\r\n\r\n and read the response. This ensures the connection was fully accepted and processed before closing, making the counter bump deterministic.

3. Increase poll timeout from 2s to 5s — though this only helps if the counter eventually reaches 5 (it may not if the accept was genuinely lost due to the immediate close).

Option 2 is the most robust since it makes the accept+count sequence deterministic rather than relying on timing.

Related

Same class of CI timing sensitivity as issue #43 (ObsConnMetrics protocol gauge race).

[mwfj]

Fix is up in #46 (branch fix-flake-unit-test-issue). Applies Option 2 from the suggested fixes — replaces the bare connect+close loop with connect → send GET /h → drain response → close so the accept callback is guaranteed to complete before the close races it. Verified 20/20 clean runs + 10/10 under 4-thread CPU stress.

[mwfj]

Regression — PR #46 fix is insufficient under heavy CI runner load

The fix from PR #46 (replacing bare connect+close with a full GET/drain/close cycle, plus extending the poll budget from 2 s → 5 s) is not enough on heavily-loaded runners. The same test failed again on a downstream consumer (mwfj/ebbry) that pulled in the PR #46 changes.

New failure evidence

• Downstream CI run: https://github.com/mwfj/ebbry/actions/runs/26515299090/job/78090627014
• Failed test: ObsConnMetrics: net.connections.accepted monotonic over N opens
• Assertion: delta=4.000000 expected>=5 — 4 of 5 expected accept-time +1s observed within the 5 s poll window
• Total run shape: 1892 / 1893 tests passed (the only failure is this single flaky test, same as before)

Why the previous fix wasn't enough

The downstream CI run that surfaced this regression was scheduling gateway-clang (this test job) alongside gateway-asan (29 m 30 s), gateway-build-test (19 m 20 s), and seven Go test shards in the same workflow. Under that level of parallel CPU contention, the accept dispatcher thread can be starved long enough that one of the 5 sequential connections gets its Counter::Add(1.0) delayed beyond the 5 s snapshot poll budget, even with the full GET/drain/close cycle in place. The PR #46 verification (20/20 clean + 10/10 stress) ran on a less-loaded environment than the real downstream CI matrix.

The accept-time bump path itself is correct — ConnectionHandler::AttachTransportObservability (server/connection_handler.cc:62) unconditionally Add(1.0)s inside the !net_active_incremented_ guard. The test is just racing the accept dispatcher's wallclock under load.

Proposed escalations (test-side only, no production change needed)

In order of increasing robustness:

1. Pace the connect loop — add sleep_for(50ms) between connects so the accept dispatcher has guaranteed wallclock space between events. Restores the explicit pacing the test had before PR Fix ci failure issue #46 (which dropped the 20 ms sleep when adding the HTTP cycle).
2. Increase N and add tolerance — bump N = 5 → N = 10 and assert delta >= N - 1. Acknowledges the inherent runner-load race while still catching real monotonicity regressions (a lost +1 for every connection would now show as ≤ 1, not ≥ 9).
3. Retry-with-hard-deadline — wrap the whole drive+poll loop in an outer retry: if the final poll shows delta < N, restart from a fresh accepted_before snapshot and try again, with an overall ~30 s hard deadline. Makes the test truly load-insensitive at the cost of more wall time on the slow path.

Option 1 alone is probably enough (cheap, low-risk), but if a future regression is suspected Option 2 gives statistical confidence and Option 3 is the belt-and-suspenders shape.

Why this matters

This test is the only failure across the entire 1893-test gateway suite on downstream CI. Reopening because the underlying flake hasn't actually been fixed, only made rarer.

[mwfj]

Reopening — see the comment above for the regression evidence.