test(monitoring): fix flaky TCP connection-detection test (retry connect+detect)#615
Merged
Merged
Conversation
…etection test test_suspicious_tcp_port_detected intermittently failed with 'Expected network threat for TCP:4444, got none' under CI load. The suspicious connection is fire-and-forget, so a transiently failed connect() (server race / slow python3 startup) leaves no ESTABLISHED socket to detect, and the host-side /proc-net poller can also miss a single detection window. Re-issue the connection and poll again up to 3 times (fresh connect each attempt); any detection passes. No change to what the test asserts. Prior commits widened the poll window (40s->75s) and added a wait-for-listen; this addresses the residual fire-and-forget/poll-window race directly.
Applies the /code-review findings on the retry hardening: - Verify the in-container connect() actually reached ESTABLISHED (ss ESTAB on :4444) before polling for detection. This distinguishes the two failure modes the blind retry conflated: a failed connect is retried; an ESTABLISHED-but- undetected connection now fails as a real detector miss with a precise message (findings #1, #5). - Re-ensure the port-4444 server is listening each attempt, restarting it if it died between attempts (finding #4). - Track the fire-and-forget connect processes and terminate them in `finally` so up to three `incus exec ... sleep(120)` sessions don't linger (finding #3). - Per-attempt poll trimmed to 30s (3x40s worst case unchanged from the retry version) so runtime doesn't regress (finding #2). Extracted small helpers (_start_http_server_4444 / _wait_port4444_listening / _port4444_listening / _port4444_established) shared by the initial setup and the retry loop. Test-only; no product code touched.
…upgrade Applies the max-effort /code-review findings on the prior commit: - Anchored port match (#1/#7): replace `grep ':4444'` / `grep 'ESTAB.*:4444'` substring checks with exact `ss` state+port filters (`ss -Htln 'sport = :PORT'`, `ss -Htn state established '( sport = :PORT or dport = :PORT )'` + `grep -q .`). The old substring matched ephemeral ports 44440-44449 and IPv6 4444 hextets, which could flip established=True on a connect flake and misfire the "detector miss" assertion. One idiom now (grep -q + returncode) for both predicates. - TimeoutExpired safety (#2): `_incus_bash` catches subprocess.TimeoutExpired and returns None ("not ready"); predicates guard for it. The establish gate no longer adds an uncaught-timeout hard-ERROR surface to the detection path. - Establish/poll split (#3/#4): Phase 1 retries only the CONNECT (bounded) and confirms ESTABLISHED; Phase 2 polls ONCE with the full 75s budget. Drops the sticky `established` misdiagnosis and stops connect-retries from shrinking the detection-latency window below the old 75s. - Skip-vs-fail consistency (#5): an in-loop server restart that fails now pytest.skips (environmental), matching the initial start. - Parameterize + reuse (#6): helpers take (container, port); the metadata sibling drops its condemned `time.sleep(2)` for the same wait-for-listening. - Efficiency (#8/#9/#10): server-start unified into the loop (no redundant back-to-back probe); establish-wait polls at 2s (monitor's interval); break on first establish (no redundant extra connects). Pre-existing, out of scope: _poll_network_threats returning on the first network event, and the `"4444" in json.dumps` substring assert.
mensfeld
added a commit
to technicalpickles/code-on-incus
that referenced
this pull request
Jul 17, 2026
…only-home-mount
4 tasks
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
1 participant
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.
What
Hardens
test_suspicious_tcp_port_detected(tests/integration/test_network_connection_detection.py), which intermittently fails in CI with:Why it flakes
The test opens a suspicious TCP connection inside the container as fire-and-forget (
subprocess.Popen, stderr discarded) and then polls the host-side threat detector for the event. Two independent races make it miss:connect()transiently fails (the port-4444 server just started listening, or python3's startup lags under CI load), there's no ESTABLISHED socket, so the poller correctly detects nothing — but the failure is invisible (no wait, stderr=DEVNULL)./proc/<pid>/net/tcppoller can miss a single detection window.The two sibling tests (
udp,metadata) pass consistently; only this one flakes. Prior commits already widened the poll window (40s→75s,91bd859) and added a wait-for-listen (51b55c5) — this addresses the residual fire-and-forget/poll race directly.Fix
Re-issue the connection and poll again up to 3 times (a fresh connect each attempt — re-polling the same one wouldn't help), 40s per attempt; any detection passes. No change to what the test asserts.
Notes