fix(ci): de-flake the two intermittent macOS test failures (#1222)

[zackees]

Closes #1222.

Both tests, both fixed at the root rather than by weakening assertions. Neither was a regression — they only became visible once #1216/#1221 stopped the macOS job aborting before it reached the test step.

1. spawn_lock_is_exclusive_within_process

The issue's key observation was that we couldn't tell why it failed, and that's the part fixed first:

file_lock::try_acquire(path, FileLockMode::Exclusive)
    .ok()      // <-- any io::Error becomes None
    .flatten()

.ok() collapsed a hard I/O error into the same None that means "another process holds the lock."

• try_acquire_spawn_lock_result_at is a new error-propagating variant. try_acquire_spawn_lock_at keeps the swallowing behavior — a broken filesystem must never gate daemon spawn — but now logs the errno and kind before returning None. Production contract unchanged; the failure is just no longer invisible.
• The test uses the propagating variant, so a hard error reports the real errno instead of assertion failed: third.is_some().
• A new test pins the swallowing wrapper's happy/contended behavior, since only its error path changed.

EINTR is now retried in file_lock::try_acquire, around both the open and the flock (candidate cause (a) in the issue). flock() is interruptible by signals on macOS/BSD, fs2 returns the raw OS error, and lock_is_held only recognizes EWOULDBLOCK/lock_contended_error — so an EINTR became a hard error → .ok() → None → that exact assertion. Retrying is correct whether or not it was the cause here. Bounded at 8 attempts so a continuous signal source can't spin forever.

Candidate (b), fd exhaustion, is not separately addressed — but it now announces itself as EMFILE in the log instead of looking like contention.

2. streaming_download_retries_chunk_stalls_five_times_without_output

As the issue notes, this was the only test combining start_paused = true with a real TcpListener, and it was the one that flaked. Paused time auto-advances whenever the runtime looks idle, but socket readiness comes from the OS reactor, not the virtual clock — so with the client parked on chunk() and the server on sleep(120s), the clock could jump past a connection about to reach accept(), leaving request_count below 5.

I took the first suggested option, not the stopgap: the retry durations move into a RetryTiming struct, so the test drives the same retry logic on millisecond durations in real time.

const FAST_RETRY_TIMING: RetryTiming = RetryTiming {
    chunk_read_timeout: Duration::from_millis(150),
    backoffs: &[/* 10ms x4 */],
};

No start_paused, no reactor race, and assert_eq!(count, 5) stays exact rather than being weakened to >= 1 — the retry budget is the actual contract under test, so keeping it asserted is the point. production_retry_timing_matches_the_constants pins the seam to CHUNK_READ_TIMEOUT / RETRY_BACKOFFS so the test seam can't silently drift into a behavior change.

Verification

Local repro of the macOS host isn't possible (per the issue: cross-compiling to x86_64-apple-darwin from Windows fails on ring), so this leans on CI plus what is checkable locally:

• The chunk-stall test passes 5/5 consecutive runs, ~1s each (was ~90s of virtual time).
• soldr cargo test -p fbuild-packages-fetch -p fbuild-paths -p fbuild-core --lib green.
• soldr cargo clippy clean on all four crates with -D warnings.

fbuild-paths gains a tracing dependency for the errno log line.

Worth watching this PR's Check (macos-latest) specifically — that's the real test of the fix.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved resilience when acquiring file and process locks by retrying interrupted operations.
  • Lock contention continues to be handled safely without being reported as an unexpected error.
  • Improved download retry and stalled-transfer handling for more reliable package downloads.
  • Added clearer diagnostic logging when lock acquisition encounters an I/O failure.
• Tests
  • Expanded coverage for interrupted operations, lock behavior, retry timing, and stalled downloads.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: cd4bd263-1f70-44fc-baa3-6cf593a7f3ff

📥 Commits

Reviewing files that changed from the base of the PR and between 8f512c8 and 1cf17b1.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

• crates/fbuild-core/src/file_lock.rs
• crates/fbuild-packages-fetch/src/downloader.rs
• crates/fbuild-paths/Cargo.toml
• crates/fbuild-paths/src/daemon_ownership.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• crates/fbuild-paths/src/daemon_ownership.rs
• crates/fbuild-paths/Cargo.toml
• crates/fbuild-core/src/file_lock.rs
• crates/fbuild-packages-fetch/src/downloader.rs

---

📝 Walkthrough

Walkthrough

The PR retries interrupted file-lock operations, injects downloader retry timing for production and tests, and adds error-preserving spawn-lock acquisition with logging for swallowed I/O failures.

Changes

Reliability changes

Layer / File(s)	Summary
Interrupted file-lock retries crates/fbuild-core/src/file_lock.rs	File opening and lock acquisition retry interrupted system calls up to eight times. Lock contention remains Ok(None).
Injected downloader retry timing crates/fbuild-packages-fetch/src/downloader.rs	Retry delays and chunk-read deadlines use RetryTiming. Streaming downloads expose an internal timed helper. Stall tests use short real-time durations and validate production timing.
Spawn-lock error handling crates/fbuild-paths/Cargo.toml, crates/fbuild-paths/src/daemon_ownership.rs	Spawn-lock acquisition exposes an error-preserving API. The existing wrapper logs I/O failures and converts them to None. Tests cover successful and contended acquisition.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• FastLED/fbuild#1168: Modifies the same file-lock and spawn-lock code for acquisition and error handling.
• FastLED/fbuild#1189: Modifies downloader retry tests and retry-exhaustion assertions.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the CI flake fixes for the two intermittent macOS test failures.
Linked Issues check	✅ Passed	The changes address both flaky tests by adding EINTR retries, error diagnostics, error propagation, and injected real-time download retry timing.
Out of Scope Changes check	✅ Passed	All reviewed changes support the linked issue objectives and preserve the documented production behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1222-deflake-macos-ci

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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