cloudflare / cloudflare/pingora

closes_only_unclaimed_fds is racy: it asserts a closed fd number is invalid, but other test threads reuse it

Open
#963 0 comments 0 reactions 1 assignee Claimed by @andrewhavck View on GitHub
Accepted bug
Dominant language
Rust
Stars
27.4k
Forks
1.7k
Avg merge
6h 22m
Merged PRs (30d)
3

Description

## Describe the bug

`close_unclaimed_tests::closes_only_unclaimed_fds` in
`pingora-core/src/server/transfer_fd/mod.rs` is racy against any other test in the same binary that
opens a file descriptor.

The test closes `drop_fd` through `Fds::close_unclaimed()` and then asserts that the descriptor
**number** has become invalid:

```rust
let closed = fds.close_unclaimed(&keep);
...
assert_eq!(unsafe { libc::fcntl(drop_fd, libc::F_GETFD) }, -1);
assert_eq!(std::io::Error::last_os_error().raw_os_error(), Some(libc::EBADF));
```

File descriptor numbers are process-wide, and `open`/`socket` hand back the lowest free number. Rust
runs the tests of one binary in parallel threads, and several tests in this same file create sockets
(`test_send_receive_fds`, `test_serde_via_socket`, `test_send_fds_to_respects_configurable_timeout`,
`empty_keep_set_closes_everything`, …). If any of them allocates a descriptor between
`close_unclaimed()` and the `fcntl` check, it gets `drop_fd`'s number back and the assertion fails —
even though `close_unclaimed()` did exactly the right thing.

## Pingora info

**Pingora version**: `main` at `0046038` (pristine clone, `git status` clean)
**Rust version**: `cargo 1.97.1 (c980f4866 2026-06-30)`
**Operating system version**: Debian GNU/Linux 13 (trixie), 12 cores, kernel 6.18.33 (WSL2)

## Steps to reproduce

```bash
git clone --depth 1 https://github.com/cloudflare/pingora
cd pingora
for i in $(seq 1 20); do
cargo test -p pingora-core --lib server::transfer_fd 2>&1 \
| grep -E "closes_only_unclaimed_fds \.\.\. (ok|FAILED)"
done
```

## Expected results

20 passes.

## Observed results

**7 failures out of 20** in the environment above. The failure is always the same assertion:

```
thread 'server::transfer_fd::close_unclaimed_tests::closes_only_unclaimed_fds' panicked at
pingora-core/src/server/transfer_fd/mod.rs:592:9:
assertion `left == right` failed
left: 0
right: -1
```

`left: 0` means `fcntl(drop_fd, F_GETFD)` succeeded — i.e. that descriptor number was already valid
again, because another thread had taken it.

It reproduces less often when the machine is otherwise idle, and I have not seen it on this
repository's CI, so it is load- and core-count dependent rather than universal.

## Additional context

The behaviour under test is fine; only the way it is observed is racy. Ideas, in the order I would
rank them:

- Drop the `fcntl`/`EBADF` pair. The assertions above it (`closed` contains the right key, the
entry is gone from `Fds`) already cover what `close_unclaimed()` promises, and they are not racy.
- Or serialize the descriptor-sensitive tests in this file behind a shared mutex, so no other test
can allocate a descriptor inside that window.

Checking the number is inherently unreliable in a multi-threaded process, so I do not think there is
a variant of the current assertion that is both meaningful and stable.

I noticed this while working on #959 / #960, which touch the same function; it is pre-existing and
unrelated to that change (measured at the same rate with and without it). Happy to send a PR for
either direction if you have a preference.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.