EEBus: retry test server bind to survive freePort race

[andig]

follows #31617

freePort() in the eebus tests probes a free port (listen :0, read port, close) and hands it to the server, which binds it later. Another listener can take the port in that gap — a TOCTOU that surfaces as bind: address already in use and flakes TestShipPairing and TestControlBoxGridGuardHeartbeat (the now-dominant remaining flake in this package, ~1/55 locally).

An ephemeral port (Port: 0) isn't an option: ship-go announces the configured port via mDNS and rejects port 0, so the port has to be known up front.

• newServerOnFreePort retries NewServer on a fresh port when the bind loses the race; the initial server setups in both tests use it.
• Added TestNewServerOnFreePort_RetriesPastOccupiedPort: occupies the port first and asserts the helper retries past it (fails without the retry).

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The retry loop in newServerOnFreePort uses for range 5, which is not valid Go; consider a conventional counted loop like for i := 0; i < 5; i++ {} to ensure the code compiles and the intent is clear.
• Overriding the global nextFreePort in TestNewServerOnFreePort_RetriesPastOccupiedPort introduces shared mutable state that could behave incorrectly with parallel tests; consider using dependency injection or a non-global hook to avoid test interference.
• The logic in newServerOnFreePort relies on checking strings.Contains(err.Error(), "address already in use"), which is brittle; it would be more robust to detect the bind failure via a typed error or errors.Is against a well-defined sentinel error.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The retry loop in newServerOnFreePort uses `for range 5`, which is not valid Go; consider a conventional counted loop like `for i := 0; i < 5; i++ {}` to ensure the code compiles and the intent is clear.
- Overriding the global `nextFreePort` in TestNewServerOnFreePort_RetriesPastOccupiedPort introduces shared mutable state that could behave incorrectly with parallel tests; consider using dependency injection or a non-global hook to avoid test interference.
- The logic in newServerOnFreePort relies on checking `strings.Contains(err.Error(), "address already in use")`, which is brittle; it would be more robust to detect the bind failure via a typed error or `errors.Is` against a well-defined sentinel error.

## Individual Comments

### Comment 1
<location path="server/eebus/test/pairing_test.go" line_range="49" />
<code_context>
+
+		if _, err := server.Instance(); err == nil {
+			return inst
+		} else if !strings.Contains(err.Error(), "address already in use") {
+			require.NoError(t, err, "instance")
+		}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** String matching on the error message may be brittle; consider a more robust check or a test to guard against changes

`newServerOnFreePort` relies on `strings.Contains(err.Error(), "address already in use")` to detect the bind race. If the error text changes (OS differences, refactor, etc.), this will stop retrying and instead hit `require.NoError`. Prefer using `errors.Is` with a specific error type if available, or add a unit test that asserts the helper still retries when `server.Instance` returns the bind error so future changes don’t silently break this behavior.

Suggested implementation:

```golang
		if _, err := server.Instance(); err == nil {
			return inst
		}

		// if this isn't the bind-race / "address already in use" case, fail the test
		// instead of retrying on an unexpected error
		if !errors.Is(err, syscall.EADDRINUSE) {
			require.NoError(t, err, "instance")
		}

```

1. In the import block of `server/eebus/test/pairing_test.go`, add:
   - `errors`
   - `syscall`
   and remove `strings` if it is no longer used elsewhere in the file.
   For example:
   `import ("errors" "syscall" "testing" "github.com/stretchr/testify/require" ...)`
2. Add or complete a test function (e.g. `TestNewServerOnFreePort_RetriesPastOccupiedPort`) that:
   - Arranges for `server.Instance()` to fail initially with an `EADDRINUSE`-wrapped error.
   - Calls the helper (the function containing this retry loop, likely `newServerOnFreePort`).
   - Asserts that the helper ultimately returns a running server instance instead of failing.
   This test will guard against future changes that stop using `syscall.EADDRINUSE` or otherwise break the retry behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (bug_risk): String matching on the error message may be brittle; consider a more robust check or a test to guard against changes

newServerOnFreePort relies on strings.Contains(err.Error(), "address already in use") to detect the bind race. If the error text changes (OS differences, refactor, etc.), this will stop retrying and instead hit require.NoError. Prefer using errors.Is with a specific error type if available, or add a unit test that asserts the helper still retries when server.Instance returns the bind error so future changes don’t silently break this behavior.

Suggested implementation:

		if _, err := server.Instance(); err == nil {
			return inst
		}

		// if this isn't the bind-race / "address already in use" case, fail the test
		// instead of retrying on an unexpected error
		if !errors.Is(err, syscall.EADDRINUSE) {
			require.NoError(t, err, "instance")
		}

1. In the import block of server/eebus/test/pairing_test.go, add:
   • errors
   • syscall
     and remove strings if it is no longer used elsewhere in the file.
     For example:
     import ("errors" "syscall" "testing" "github.com/stretchr/testify/require" ...)
2. Add or complete a test function (e.g. TestNewServerOnFreePort_RetriesPastOccupiedPort) that:
   • Arranges for server.Instance() to fail initially with an EADDRINUSE-wrapped error.
   • Calls the helper (the function containing this retry loop, likely newServerOnFreePort).
   • Asserts that the helper ultimately returns a running server instance instead of failing.
     This test will guard against future changes that stop using syscall.EADDRINUSE or otherwise break the retry behavior.