Flaky macOS CI: spawn-lock re-acquire and paused-time download retry test fail intermittently

[zackees]

Summary

Check (macos-latest) fails intermittently on two unrelated tests. Neither is a regression — both surfaced only once #1216/#1221 fixed the compile errors that were aborting the macOS job before it ever reached the test step.

Observed on run 30663368704, same commit, three attempts, no code changes between them:

Attempt	Result	Failing test
1	fail	fbuild-paths — daemon_ownership::tests::spawn_lock_is_exclusive_within_process
2	fail	fbuild-packages-fetch — downloader::tests::streaming_download_retries_chunk_stalls_five_times_without_output
3	pass	—

A different test failed each time and the third run went green, so this is runner-load sensitivity, not a deterministic defect. Both tests assert exact outcomes over real OS resources under timing pressure.

---

1. spawn_lock_is_exclusive_within_process

thread '...' panicked at crates/fbuild-paths/src/daemon_ownership.rs:193:9:
assertion failed: third.is_some()  // "lock must be available after release"

The test acquires the lock, confirms a second acquire is refused, drops the first, then asserts a third acquire succeeds. On the failing run the third acquire returned None.

The root problem is that we cannot tell why, because the error is discarded:

// crates/fbuild-paths/src/daemon_ownership.rs
pub fn try_acquire_spawn_lock_at(path: &Path) -> Option<SpawnLockGuard> {
    file_lock::try_acquire(path, FileLockMode::Exclusive)
        .ok()      // <-- any io::Error becomes None
        .flatten()
        .map(|_guard| SpawnLockGuard { _guard })
}

.ok() collapses a hard I/O error into the same None that means "another process holds the lock." Treating errors as "no lock available" is documented and intentional for the production caller — a broken filesystem must not gate progress — but it also erases the errno, so an actual failure is indistinguishable from routine contention in both prod and tests.

Supporting evidence that this is an error rather than a genuine failure to release: RootOwnershipGuard::try_acquire_at performs the identical acquire → contend → release → re-acquire sequence but propagates errors with ?, and it passed in the same run. If macOS were truly failing to release flock on close, both tests would fail together.

Candidate causes

a) flock() EINTR is never retried. fs2 returns the raw OS error:

// fs2-0.4.3/src/unix.rs
fn flock(file: &File, flag: libc::c_int) -> Result<()> {
    let ret = unsafe { libc::flock(file.as_raw_fd(), flag) };
    if ret < 0 { Err(Error::last_os_error()) } else { Ok(()) }
}
pub fn lock_error() -> Error { Error::from_raw_os_error(libc::EWOULDBLOCK) }

and our classifier only recognizes contention:

// crates/fbuild-core/src/file_lock.rs
fn lock_is_held(error: &io::Error) -> bool {
    error.kind() == io::ErrorKind::WouldBlock
        || error.raw_os_error() == fs2::lock_contended_error().raw_os_error()
}

EINTR is neither, so it becomes Err → .ok() → None → this exact assertion failure. flock() is interruptible by signals on macOS/BSD. (Caveat: we pass LOCK_NB, which returns immediately, making EINTR less likely than in the blocking case — plausible, not proven.)

b) fd exhaustion (EMFILE). macOS runners have a much lower default fd ceiling than Linux. Rust runs tests as threads in one process, and this suite concurrently opens lock files (including spawn_lock_serializes_concurrent_threads, which fires 16 threads at one lock) while the downloader tests bind real TCP sockets. OpenOptions::open() failing with EMFILE produces the same swallowed None.

Both are load-dependent, which matches the observed flakiness.

Suggested fix

1. Make it diagnosable first. Have try_acquire_spawn_lock_at log the errno when it converts an Err to None, keeping the "never gate progress" behavior. Right now the next occurrence gives us no more information than this one did.
2. Retry EINTR in file_lock::try_acquire (both the open and the flock), which is correct regardless of whether it is the cause here.
3. Consider having the test call an error-propagating variant so a hard error fails with the actual errno instead of a bare assertion failed.

---

2. streaming_download_retries_chunk_stalls_five_times_without_output

panicked at crates/fbuild-packages-fetch/src/downloader.rs:755:9
assert_eq!(request_count.load(Ordering::SeqCst), 5);

This test mixes tokio virtual time with real network I/O:

#[tokio::test(start_paused = true)]
async fn streaming_download_retries_chunk_stalls_five_times_without_output() {

while run_stalling_server binds a real TcpListener on 127.0.0.1:0, and the client wraps each stream.chunk() in a virtual-time tokio::time::timeout(CHUNK_READ_TIMEOUT /* 60s */) with virtual RETRY_BACKOFFS of 1/2/4/8s. The server writes headers, then tokio::time::sleep(120s).

Under start_paused, tokio auto-advances the clock whenever the runtime looks idle. But real socket readiness is driven by the OS reactor, not the virtual clock — so when every task is parked (client on chunk(), server on sleep), time can jump past a socket operation that was about to become ready. Which of the 60s client timeout and the 120s server sleep "wins" then depends on scheduling, and on a loaded runner a retry's connection may never reach accept() — where request_count is incremented — leaving the count below 5.

It is worth noting the test has already been softened once for this exact class of problem; its own comment reads:

A stalled body is retryable, as is a connection error while opening a retry. The latter can legitimately be the final transient on a busy platform, so the stable contract is exhausting all five attempts without publishing an output.

But assert_eq!(count, 5) still requires all five attempts to reach accept(), which is precisely the part that isn't stable.

Notably, this is the only test in the file combining start_paused = true with a real socket, and it is the one that flakes.

Suggested fix

Pick one of:

• Drop start_paused and use short real durations (inject the timeout/backoff constants) so virtual time never races the reactor; or
• Keep paused time but replace the real TcpListener with an in-process transport so there is no reactor to race; or
• Weaken the assertion to the property actually under test — no output file is published and the error is the stall error — and assert count >= 1 rather than exactly 5.

The first two preserve the intent; the third is the cheap stopgap.

---

Why this matters

Check (macos-latest) is not currently a required check, but a badge that goes red on its own schedule trains everyone to ignore it — which is how the four genuinely-broken things fixed in #1221 stayed broken. Re-running until green is not a fix, and I'd rather file this than paper over it.

Repro

No local repro: cross-compiling fbuild-serial to x86_64-apple-darwin from Windows fails on ring's build script needing a macOS cc, so this needs a Mac host or CI to iterate. Re-running the macOS job a few times reproduces it within a couple of attempts.

[zackees]

New data from PR #1225 that narrows part 1, worth recording separately from the PR:

The spawn-lock flake is not macOS-only, and it is not a failure to release the lock.

While validating the PR, a newly-added test asserted the post-release re-acquire through the swallowing wrapper:

drop(first);
assert!(try_acquire_spawn_lock_at(&path).is_some(), "lock must be available after release");

That reproduced this issue's exact failure on Check (ubuntu-latest) — assertion failed at the third acquire, no errno — and then passed on a re-run of the same commit with no changes. Same signature as the macOS occurrence in the original report.

Two things follow:

1. Platform. This was filed as a macOS flake. It is not. Whatever the underlying cause, Linux hits it too, which argues against the macOS/BSD-specific flock framings (including EINTR being the sole cause — the PR retries it anyway, since it is correct regardless).

2. Where it fails. It is consistently the post-release re-acquire, never the release or the contention check. That is consistent with this issue's own diagnosis — a hard I/O error being reported as routine contention — and inconsistent with "the OS failed to release flock on close." It also matches the observation in the original report that RootOwnershipGuard::try_acquire_at, which propagates with ?, passed in the same run.

Consequence for testing: any re-acquire assertion routed through try_acquire_spawn_lock_at is untestable by construction, because the wrapper collapses error and contention into the same None. PR #1225 asserts that step through the new try_acquire_spawn_lock_result_at instead, so the next occurrence fails with the actual errno rather than a bare assertion — which is what ask #1 in this issue was after.

I did not silence this by weakening the assertion or by re-running until green; the assertion still requires the re-acquire to succeed, it just now reports why when it doesn't.

The remaining unknown is the errno itself. Once the logging in #1225 is on main, the next occurrence should name it — EMFILE would confirm candidate (b) (fd exhaustion), which the Linux reproduction makes more plausible than it looked when this was believed to be macOS-only.