fix(#9562): teardown restarts a stopped server past a taken HTTP port and no longer hides a failed restart - #9578

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of PR 9578

Overall this is a solid, well-scoped test-fixture fix. The root-cause analysis is convincing, and the two changes (widen the restart port to the rest of the range, and stop the comparison failure from replacing the restart failure) match the stated invariant. No blocking issues.

What looks good

• restartHttpPortSetting is a small pure static function with clear edge cases: never bound, single port, last port of the range, and past the range. The unit tests cover each of them.
• compareWithoutMasking keeps the real cause as the primary exception and attaches the comparison failure as suppressed. It handles RuntimeException | Error and rethrows unchanged when there was no restart failure.
• The IT reproduces the issue's exact signature on the unfixed code. It also asserts that the restart really moved to another port, so it cannot pass vacuously.
• Style follows CLAUDE.md: assertThat(...) syntax, final usage, no fully qualified names, and no new dependencies. The IT binds the stranger socket on a known port, which is legitimate here because the port it takes is the one the fixture bound.

Suggestions (non-blocking)

1. Peers only learn the moved port in BaseRaftHATest. The PR says BaseRaftHATest.startServer patches the HTTP-address maps. Other BaseGraphServerTest subclasses that restart a server and then use a cached or derived URL will now see a different port in the rare collision case. This is still better than a guaranteed failure, but a short note in the Javadoc would help the next reader. Check that no other subclass caches the port before endTest.
2. Malformed settings in restartHttpPortSetting. Integer.parseInt(configured.substring(dash + 1).trim()) throws NumberFormatException for an unexpected value, for example a list-style setting. If that happens inside endTest's try, it is recorded as a "restart failure" and hides the real teardown. A guarded parse that falls back to String.valueOf(boundPort) would be more robust, and it is cheap to add. A test case would be useful too.
3. Restart failure bookkeeping. restartFailure is only set for exceptions thrown inside the restart try. That block also includes the post-restart wait and alignment code that follows the restart. That is probably fine, since those are also "restart failed" causes, but the Javadoc says "restart failure" and could say "restart or realignment failure".
4. IT stranger socket. It binds 127.0.0.1 only. If the fixture's server binds the wildcard or IPv6 address, the collision may not reproduce on every platform. The IT passing on CI suggests it does, so this is just a note. The empty catch has a comment, which is fine.
5. Small nit. The first test method in the IT does only setup and no assertions; its real assertions live in the endTest override. It works, but a one-line comment at the test method saying the verification happens in teardown would make that clearer.

Performance and security

Test-only change, so there is no production impact and no security concern.

Test coverage

Good: 7 unit tests plus a failing-before/passing-after IT. Adding the malformed-setting case from point 2 would round it out.

LGTM, modulo the optional hardening above.

Merge Risk: 🔵 Low · up to 3b970

The new regression test can fail because of port allocation rather than a product defect. The PR is mergeable with awareness of that bounded test risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly describes the primary changes: allowing teardown restarts to move past a taken HTTP port and preserving the original restart failure.
Description check		The description provides the motivation, linked issue, implementation details, test plan, risks, and validation results. It does not use all template headings and does not explicitly confirm `mvn clea…
Linked Issues check		Issue #9562 requires preventing teardown from masking a stopped-server restart failure and preventing the single old HTTP port from causing that failure when a later configured port is available. `Bas…
Out of Scope Changes check		The changes stay within issue #9562. The production-test-harness changes implement the teardown restart and exception-handling fix. The added unit and integration tests directly verify port selection,…

---

---

• Fix all pre-merge checks with AI

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Review: solid, well-scoped test-fixture fix. The diagnosis is clear and both changes (range fallback for the restart port, and not masking the restart failure) are small and targeted. No blocking issues.

Good:

• restartHttpPortSetting is a pure static function and is unit tested for range, end of range, single port, unreadable and never-bound cases.
• compareWithoutMasking keeps the root cause as the propagating exception and attaches the comparison failure as suppressed.
• The IT reproduces the issue signature by holding the victim old port, and fails without the fix.
• Style follows CLAUDE.md (final, imports, AssertJ form).

Minor suggestions (non-blocking):

1. Add a short comment in endTest noting that restartFailure is read in the finally.
2. restartHttpPortSetting with a bound port below the range start (e.g. 2490-2495, bound 2481) yields 2481-2495, widening below the configured start. Unlikely in practice, but a start <= boundPort guard would make it exact.
3. The IT stranger binds only 127.0.0.1. That blocks the server on Linux; on macOS/Windows SO_REUSEADDR semantics differ. The restartedOnPort != oldHttpPort assertion would then fail loudly rather than pass silently, which is acceptable.
4. If the 3-node Raft IT turns out to be long-running, consider @tag("slow") per CLAUDE.md.
5. Non-Raft BaseGraphServerTest subclasses that cache HTTP addresses could still see a moved port. The Javadoc already warns about this.

No security or performance concerns (test fixtures only). Test coverage is appropriate.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.37%. Comparing base (4289679) to head (3b9700d).
⚠️ Report is 7 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9578      +/-   ##
============================================
+ Coverage     77.35%   77.37%   +0.01%     
+ Complexity     4276     4275       -1     
============================================
  Files          2159     2159              
  Lines        222118   222118              
  Branches      47590    47590              
============================================
+ Hits         171822   171866      +44     
+ Misses        32062    32022      -40     
+ Partials      18234    18230       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.