test: bind ephemeral ports in test_init_and_defaults - #172
Merged
Conversation
windows-2025 Release failed on a documentation-only PR:
check failed: ( client ), function test_init_and_defaults, netcode.c:6947
netcode_client_create returned NULL because something else on the runner held
127.0.0.1:50000. The test does not care which port it gets -- it only checks that a
zeroed config yields working defaults, and it never connects -- so the fixed port buys
nothing and costs a flake. Port 0 asks the OS for an ephemeral one. The socket layer
already calls getsockname after bind, so the machinery for this existed and was unused:
37 fixed-port sites in the test suite, zero ephemeral.
REPRODUCED DETERMINISTICALLY rather than assumed. Holding 127.0.0.1:50000 from another
process and running the suite gives exactly the CI failure at 6947. With this change the
failure moves PAST test_init_and_defaults, which is the proof that this test is fixed.
WHAT IS NOT FIXED, stated so the flake is not reported as gone:
- test_client_create_error still needs a fixed port BY DESIGN -- it binds one and asserts
that a second bind on the same port returns NULL. Ephemeral would make both binds
succeed and silently destroy the test. The hermetic version binds :0, reads back the
assigned port, and re-binds that; it needs an API to read the bound address and is a
bigger change than this one.
- test_runtime_guards fails the same way if 127.0.0.1:40000 is held.
So the suite is still vulnerable to external port contention in two places. This change
fixes the one that actually broke CI and does not claim more.
Verified: clean rebuild, full suite passes normally, and passes test_init_and_defaults
with :50000 held.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
windows-2025 Releasefailed on a documentation-only PR:netcode_client_createreturned NULL because something else on the runner held127.0.0.1:50000. The test does not care which port it gets — it only checks that a zeroed config yields working defaults, and it never connects. So the fixed port buys nothing and costs a flake.The socket layer already calls
getsocknameafter bind, so the machinery for ephemeral ports existed and was unused: 37 fixed-port sites in the test suite, zero ephemeral.Reproduced deterministically, not assumed
Holding
127.0.0.1:50000from another process and running the suite reproduces exactly the CI failure at 6947. With this change, the failure moves pasttest_init_and_defaults— which is the proof that this test is fixed rather than the symptom being masked.What this does NOT fix
Stated so the flake is not reported as gone:
test_client_create_errorneeds a fixed port by design — it binds one and asserts a second bind on the same port returns NULL. Ephemeral would make both binds succeed and silently destroy the test. The hermetic version binds:0, reads back the assigned port and re-binds that; it needs an API to read the bound address, and is a bigger change than this one.test_runtime_guardsfails the same way if127.0.0.1:40000is held.The suite is still vulnerable to external port contention in two places. This fixes the one that actually broke CI and claims nothing more.