Stop the node_fold tests racing the real :4243 — the flake that cost three shard re-runs (#1166) - #1170
Open
emooreatx wants to merge 2 commits into
Open
Stop the node_fold tests racing the real :4243 — the flake that cost three shard re-runs (#1166)#1170emooreatx wants to merge 2 commits into
emooreatx wants to merge 2 commits into
Conversation
…cing the real :4243 `test_refuses_a_live_node_this_process_does_not_own` failed on three unrelated PRs -- #1162 shard 6, #1164 shard 2, #1169 shard 7 -- each costing an 18-minute shard re-run on a diff it could not see (#1169's was requirements.txt plus four version strings). CIRISAgent#1166. Cause: the node port was a bare literal in four places in node_fold, so a test exercising the ownership logic had to bind the REAL 4243 and then raced everything else on the box that touches it -- the sibling `own_listener` in another xdist worker, a leftover node, a TIME_WAIT socket with SO_REUSEADDR. The flaky-retry plugin re-ran into the same shared environment, so retries rarely helped. Worse than flaky: when 4243 was already held, two of the four tests SKIPPED ("port 4243 already in use") -- silently not checked, which is the shape this very file was written to eliminate. Now `NODE_FOLD_PORT` names it once; the liveness probe, the ownership check, the HTTP identity probe and the two operator-facing messages all read it, so the messages name the port actually probed. An autouse fixture points it at an ephemeral port the kernel guarantees is unused. Proven against the failure condition rather than by passing once: with a real foreign HTTP server squatting on 4243, the old file skipped 2 of 4 tests while the new one passes all 4 -- serially three times and under `-n 8`. All 44 tests across the five node_fold-touching files pass; mypy on node_fold is clean (the two remaining errors are the documented wallet import-untyped ones, identical on main). No version bump: #1169 is already taking 2.11.4 and edits the same constants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J45bNgkggVC1ntnmp6DqQA
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Sonar put this PR's new-code coverage at 71.4% against the 80% gate: of the lines the port-constant change touched, two sat in `stop_node_fold`'s "the wheel cannot free the port" arm, which had no test. That arm is worth one. `stop_node_fold` returns None for BOTH "this process never started a node" and "this wheel predates shutdown_node", so the warning is the only thing separating a clean no-op from a node that will still hold :4243 when the next boot tries to bind it -- the EADDRINUSE the five-platform gate's post-reset port check exists to catch (CIRISAgent#1102). Both paths are now asserted, including that the clean one stays quiet. 9 of the 11 changed lines are now covered. The remaining two are inside the post-boot compose region (node_fold 515-708), which has no test coverage at all today and is not something this PR introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J45bNgkggVC1ntnmp6DqQA
|
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.
Closes #1166.
test_refuses_a_live_node_this_process_does_not_ownfailed on three unrelated PRs — #1162 shard 6, #1164 shard 2, #1169 shard 7 — each costing an ~18-minute shard re-run. #1169's diff wasrequirements.txtplus four version strings; the test cannot see any of it.Cause
The node port was a bare literal in four places in
node_fold.py(liveness probe, ownership check, HTTP identity probe, two messages). A test exercising the ownership logic therefore had to bind the real 4243, and then raced everything else on the box that touches it: the siblingown_listenerin another xdist worker, a leftover node from an earlier test, a TIME_WAIT socket withSO_REUSEADDR. The flaky-retry plugin re-ran into the same shared environment, which is why retries rarely helped.Worse than flaky. With 4243 already held, two of the four tests skipped —
port 4243 already in use— silently not checked. That is precisely the shape this file's own docstring was written to eliminate.Fix
NODE_FOLD_PORTnames it once and all four functional uses read it (so the operator-facing messages now name the port actually probed, instead of asserting:4243while probing something else). An autouse fixture points it at an ephemeral port the kernel guarantees is unused.Proven against the failure condition, not by passing once
With a real foreign HTTP server squatting on 4243:
:4243occupiedport 4243 already in use-n 8All 44 tests across the five
node_fold-touching files pass. mypy onnode_fold.pyis clean (the two remaining errors are the documented walletimport-untypedones, identical onmain).No version bump — #1169 is already taking 2.11.4 and edits the same constants; this rides whatever merges.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J45bNgkggVC1ntnmp6DqQA