Skip to content

fix: eliminate port TOCTOU race and framed test compile failure breaking CI (#767)#774

Merged
strawgate merged 1 commit into
masterfrom
fix/diagnostics-test-port-toctou
Apr 3, 2026
Merged

fix: eliminate port TOCTOU race and framed test compile failure breaking CI (#767)#774
strawgate merged 1 commit into
masterfrom
fix/diagnostics-test-port-toctou

Conversation

@strawgate
Copy link
Copy Markdown
Owner

Summary

Fixes two test failures on macOS/Linux CI introduced by the nextest switch in #761:

1. diagnostics.rsAddress already in use on macOS

free_port() + TEST_LOCK assumed shared process state, but nextest runs each test as its own process. The static AtomicU16 resets to 19100 per process, so concurrent test processes pick the same starting port.

Fix: DiagnosticsServer::start() now returns (JoinHandle<()>, SocketAddr) so the actual OS-assigned port is available without a separate probe step. Tests use "127.0.0.1:0" and extract the port from the returned SocketAddr. Removes free_port(), TEST_LOCK, and all per-test port management — no more TOCTOU window.

2. framed.rs — test compile failure

FormatProcessor::Passthrough became a struct variant in #755 but 3 EOF-flush tests were not updated. Tests now use FormatProcessor::passthrough(stats.clone()).

Test plan

  • cargo test -p logfwd-io passes locally (previously failed to compile)
  • CI green on both macOS and Linux
  • No change to production behavior — DiagnosticsServer API is internal to the binary

🤖 Generated with Claude Code

…dr from start()

Two test failures on macOS CI, both introduced by switching to nextest (#761):

1. diagnostics.rs — free_port() + TEST_LOCK failed because nextest runs each
   test as its own process: the static AtomicU16 resets to 19100 per process,
   so concurrent tests pick the same starting port → 'Address already in use'.
   Fix: DiagnosticsServer::start() now returns (JoinHandle<()>, SocketAddr).
   Tests use "127.0.0.1:0" and read the OS-assigned port from the result.
   Removes free_port(), TEST_LOCK, and all per-test port management.

2. framed.rs — FormatProcessor::Passthrough became a struct variant in #755
   but 3 EOF-flush tests were not updated → test compile failure on master.
   Fix: use FormatProcessor::passthrough(stats.clone()).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 3, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1e4c0d98-f345-4568-a3ff-f7877460fd41

📥 Commits

Reviewing files that changed from the base of the PR and between 9f43073 and 7e2af91.

📒 Files selected for processing (3)
  • crates/logfwd-io/src/diagnostics.rs
  • crates/logfwd-io/src/framed.rs
  • crates/logfwd/src/main.rs

Walkthrough

The pull request updates DiagnosticsServer::start to return a tuple containing both the JoinHandle and the actual bound SocketAddr instead of only the handle. This enables callers to retrieve the server's bound address after startup. The diagnostics test infrastructure was refactored to use ephemeral port binding (port 0) rather than manually selecting and locking TCP ports via a free_port() function. Additionally, FramedInput unit tests were updated to use the new FormatProcessor::passthrough() API, and the call site in main.rs was adjusted to destructure the new return value.

Possibly related PRs


Comment @coderabbitai help to get the list of available commands and usage tips.

@strawgate strawgate merged commit 383c4fe into master Apr 3, 2026
6 of 9 checks passed
@strawgate strawgate deleted the fix/diagnostics-test-port-toctou branch April 3, 2026 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant