test(rdns): fix flaky thread leak across analytics rDNS tests - #211
Merged
Conversation
…y leak) main went red on test_analytics_rdns.py::test_repeated_calls_dedup_targets_ to_bound_budget — its `calls` list contained 9.9.9.x IPs from the PRIOR test (test_saturated_pool_does_not_poison_unpulled_queue). Root cause: a test-isolation thread leak, not a product bug. warm_rdns runs lookups on daemon `rdns-*` workers and abandons any black-holed past its wall-clock budget (intentional — survives a hung gethostbyaddr). test_saturated_pool leaves 2 such workers hung on its gate; the `finally: gate.set()` revives them, and they drain the remaining queue items — but by then the next test has monkeypatched _rdns_lookup, so the leaked workers append to ITS calls list. It only surfaces when xdist schedules the two tests back-to-back on one worker (timing-dependent — green on the PR run, red on main, same tree; #210's added test files shifted the distribution). Production is unaffected: each warm_rdns call owns its queue and _rdns_lookup isn't swapped at runtime. Fix: the rdns_state fixture now joins any lingering `rdns-*` thread (timeout) on teardown, before the next test. The test's monkeypatch is still active during teardown (set up before rdns_state, torn down after), so the joined workers call their own test's lookup and exit cleanly. Verified: the file passes 5x sequential + 3x under -n auto; full light suite 1840 passed; ruff clean. (Tests-only change — slow deploy suite unaffected.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018i8zX4iJDjh6nwLCYRVaRf
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
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.
Why
mainwent red right after #210 merged, ontest_analytics_rdns.py::test_repeated_calls_dedup_targets_to_bound_budget— itscallslist unexpectedly contained9.9.9.xIPs from the previous test (test_saturated_pool_does_not_poison_unpulled_queue). This is a test-isolation thread leak, not a product bug, and unrelated to the LI-COR change that surfaced it.Root cause
warm_rdnsresolves PTRs on daemonrdns-*workers and abandons any that black-hole past its wall-clock budget (intentional — a hunggethostbyaddrmust not pin the process).test_saturated_poolleaves 2 such workers hung on itsgate; itsfinally: gate.set()revives them, and they drain the remaining queue items. By then the next test has monkeypatched_rdns_lookup, so the leaked workers append to itscallslist.It only manifests when xdist schedules the two tests back-to-back on one worker — timing-dependent, which is why the identical tree was green on #210's PR run but red on main (the merge added test files, shifting
-n autodistribution). Production is unaffected: eachwarm_rdnscall owns its queue, and_rdns_lookupis never swapped at runtime.Fix
The
rdns_statefixture now joins any lingeringrdns-*thread (with a timeout) on teardown, before the next test runs. The test'smonkeypatchis still active during teardown (it's set up beforerdns_state, so torn down after), so the joined workers call their own test's lookup and exit cleanly — nothing leaks forward. Test-only change.Verification
-n auto(CI mode).(Also re-ran the red main CI run to unblock
mainimmediately; this PR is the durable fix so it stops recurring.)— Claude · Opus-4.8(1M) · x-high
🤖 Generated with Claude Code