cloudflare / cloudflare/pingora
current_handle() can return a NoStealRuntime that has already shut down
- Dominant language
- Rust
- Stars
- 27.4k
- Forks
- 1.7k
- Avg merge
- 6h 22m
- Merged PRs (30d)
- 3
Description
## Describe the bug
`current_handle()` can hand a thread a runtime that has already shut down.
A `NoStealRuntime` built after an earlier one has shut down can start life holding the dead runtime's handles. Tasks its workers spawn through `current_handle()` are cancelled the instant they are spawned, on a runtime that is alive, idle, and otherwise healthy. The caller sees `task N was cancelled` with nothing in it naming the dead runtime, so it reads as a spurious cancellation.
`CURRENT_HANDLE` is a process-wide `ThreadLocal` keyed by an id that the `thread_local` crate recycles on thread exit, by design. From `thread_local-1.1.9/src/thread_id.rs`:
```rust
/// Thread ID manager which allocates thread IDs. It attempts to aggressively
/// reuse thread IDs where possible ...
fn alloc(&mut self) -> usize {
if let Some(id) = self.free_list.as_mut().and_then(|heap| heap.pop()) {
id.0
} else { ... }
}
```
`free` runs from a `ThreadGuard` destructor on thread exit and `alloc` pops the lowest free id first. `ThreadLocal` never clears the entry, so a registration outlives the worker that made it and reappears under whichever thread is handed the same id next. The safety comment in `current_handle()` holds for the thread that made the registration, and not for that thread's successor.
There are two sides to it:
- **Registration.** When a recycled id lands on a worker of a newer `NoStealRuntime`, the `get_or` in `init_pools()` finds the stale entry, does not run its closure, and leaves the new worker holding the old runtime's pools. This is what the test below drives.
- **Read.** When a recycled id lands on an ordinary application thread that was never part of any `NoStealRuntime`, `current_handle()` finds a registration under its id and returns the dead runtime instead of falling through to `Handle::current()`. Whether a thread is exposed depends on the `thread_local` version: through 1.1.9 `get` allocates an id for the caller, so any thread calling `current_handle()` qualifies; 1.1.10 changed it to `try_get`, so the thread must already hold an id, which it does if it has touched any other `ThreadLocal`. `pingora-runtime` depends on `thread_local = "1"`, so both are in range of a normal build.
Worth noting that `get_pools()` recycles ids on its own, with no help from application code. The losing side of the race spawns a full set of workers and then drops the controls:
```rust
Err((p, _my_pools)) => p,
```
Dropping the controls drops the `Sender`s, the workers see `Err(_)` from `rt.block_on(rx)` and exit, having already registered. The `TODO` just above that `match` describes the same situation from the cost side.
## Pingora info
**Pingora version**: reproduced against `pingora-runtime` 0.8.1 from crates.io. `current_handle()` is unchanged since `8797329` (v0.1.0), so 0.1.0 through 0.8.1 all carry it.
**Rust version**: `cargo 1.95.0 (f2d3ce0bd 2026-03-21)`
**Operating system version**: macOS 26.5.2, arm64. The mechanism is thread-id allocation order, so we would expect it everywhere.
## Steps to reproduce
The test below depends on nothing but `pingora-runtime` and `std`. In an empty crate with `pingora-runtime = "0.8"`, drop it at `tests/repro.rs` and run `cargo test`. In a pingora checkout, copy it to `pingora-runtime/tests/` and run `cargo test -p pingora-runtime --test `.
It builds a two-worker `Runtime::new_no_steal`, reads its handle so the pools and registrations exist, and shuts it down. Joining the workers returns their ids to the free list. It then builds a one-worker `Runtime::new_no_steal` whose worker is handed a recycled id, and from a task on that second runtime calls `current_handle()` and spawns through it.
It is deterministic rather than lucky, because it is the only thing in its process allocating a `thread_local` id: the first runtime's workers take the lowest ids including 0, each leaves a registration under the id it took, `shutdown_timeout` returns exactly those ids to a heap that pops lowest first, and the second runtime gets a single worker so the one handle `get_handle()` can return belongs to the thread holding the lowest. No sleeps, no timing assumptions, no patched build. That is also why the file holds exactly one `#[test]`: a second one runs on another thread and competes for the same ids.
## Expected results
A task spawned through `current_handle()` from a worker of a live `NoStealRuntime` runs on that runtime and returns its value.
## Observed results
300 runs out of 300 against `pingora-runtime` 0.8.1, on `thread_local` 1.1.9 and 1.1.10 alike:
```
test a_no_steal_worker_spawns_onto_its_own_runtime ... FAILED
thread 'a_no_steal_worker_spawns_onto_its_own_runtime' panicked at tests/repro.rs:110:21:
a worker of the live second runtime spawned onto the shut-down first one: task 2 was cancelled
```
Both `thread_local` versions, 300 out of 300 each, so the registration side does not depend on the 1.1.10 change.
For completeness, and so you know what we did not find: we also wrote a variant driving the read side, where a thread that was never part of any `NoStealRuntime` calls `current_handle()`. That one is 300 out of 300 on 1.1.9 and **0 out of 300 on 1.1.10**, which is the `try_get` change doing its job. So if you reproduce this on a lockfile with 1.1.10, expect the registration case to fail and the foreign-thread case to pass. We nearly reported the read-side case alone from a checkout that happened to resolve 1.1.9, which would have been unreproducible for you.
## Additional context
We hit this in a fork and have been running the following fix. Happy to open a PR with it and the test if you want it in this shape.
Store the owning `ThreadId` beside the pools and check it on read. `std::thread::ThreadId` is never reused for the life of the process, so the comparison is exact rather than probabilistic:
```rust
type Registration = RefCell>;
static CURRENT_HANDLE: Lazy> = Lazy::new(ThreadLocal::new);
```
On registration, claim the slot by assignment rather than `get_or`, so a worker inheriting a recycled id overwrites the stale entry instead of silently adopting it:
```rust
*CURRENT_HANDLE.get_or_default().borrow_mut() =
Some((thread::current().id(), pools_ref));
```
On read, fall through to `Handle::current()` unless the registration belongs to the calling thread:
```rust
if *owner == thread::current().id() { ... }
```
Two side effects worth mentioning. The assignment drops the stale `Arc` that `get_or` used to preserve, so this is strictly less retained memory, not more. And the residual is bounded: a slot whose id is never handed out again keeps its handles for the process lifetime, which is a startup-time constant rather than something that grows.
We also removed the `unwrap()` on the pools `OnceCell` in `current_handle()`, since `get_pools()` fills it after `init_pools()` returns and `init_pools()` is what spawned the reader.
The reproduction test (depends only on pingora-runtime and std)
```rust
//! `current_handle()` hands a fresh no-steal worker a shut-down runtime.
//!
//! `CURRENT_HANDLE` is a `thread_local::ThreadLocal`. Each
//! `NoStealRuntime` worker registers its own runtime's pools there in
//! `init_pools()`, and `current_handle()` reads them back. The map is
//! keyed by an id the `thread_local` crate allocates from a free list and
//! recycles the moment a thread exits, so a registration outlives the
//! thread that made it and reappears under whichever thread is handed
//! the same id next.
//!
//! When that next thread is a worker of a newer `NoStealRuntime`, the
//! `get_or` in `init_pools()` finds the old entry, does not run its
//! closure, and leaves the new worker holding a handle to the runtime
//! that already shut down. Every task the new worker spawns through
//! `current_handle()` is cancelled on arrival, on a runtime that is
//! healthy and has nothing wrong with it.
//!
//! # Running it
//!
//! Drop this file into `pingora-runtime/tests/` in a pingora checkout
//! and run:
//!
//! ```text
//! cargo test -p pingora-runtime --test upstream-current-handle-repro
//! ```
//!
//! Or, with no checkout at all, in an empty crate whose `Cargo.toml` is
//!
//! ```toml
//! [package]
//! name = "current-handle-repro"
//! version = "0.0.0"
//! edition = "2021"
//!
//! [dependencies]
//! pingora-runtime = "0.8"
//! ```
//!
//! put this file at `tests/repro.rs` and run `cargo test`. It needs
//! nothing but `pingora-runtime` and `std`.
//!
//! # Why it is deterministic
//!
//! Not timing, and no sleeps. It rests on being the only thing in its
//! process that allocates a `thread_local` id, which is why it is a test
//! file of its own with exactly one `#[test]` in it:
//!
//! 1. The first runtime's workers are the first threads in the process
//! to ask for a `thread_local` id, so they take the lowest ids, id 0
//! among them, and each leaves a registration under the id it took.
//! 2. `shutdown_timeout` joins those threads, which is what returns
//! their ids to the free list. The registrations stay where they are.
//! 3. The free list is a `BinaryHeap>` and pops the
//! lowest id first, so the next thread to ask is handed one of them.
//! 4. The second runtime is built with a single worker, so that one
//! worker is the next thread to ask, and the single handle
//! `get_handle()` can return is that worker's.
//!
//! Add a second `#[test]` to this file and libtest will run it on
//! another thread that competes for the same ids, and step 4 stops
//! holding.
use std::sync::mpsc;
use std::time::Duration;
use pingora_runtime::{current_handle, Runtime};
/// The runtime that shuts down. Any thread count works: whatever ids its
/// workers take, id 0 is one of them, and every id they take is left
/// pointing at this runtime's pools.
const FIRST_THREADS: usize = 2;
/// The runtime that outlives it, with exactly one worker, so the single
/// handle `get_handle()` can return belongs to the thread that was given
/// the lowest recycled id.
const SECOND_THREADS: usize = 1;
#[test]
fn a_no_steal_worker_spawns_onto_its_own_runtime() {
// A no-steal runtime, used and then shut down. Reading the handle is
// what builds the pools and spawns the worker threads, and each
// worker registers this runtime's pools against its own thread id
// before it starts driving its runtime.
let first = Runtime::new_no_steal(FIRST_THREADS, "first");
let _ = first.get_handle();
// Joins the worker threads. That is what puts their thread ids back
// on the free list. The registrations they left behind stay.
first.shutdown_timeout(Duration::from_secs(10));
// A second no-steal runtime, built after the first one is gone. Its
// worker is handed a recycled id, and with it the first runtime's
// registration.
let second = Runtime::new_no_steal(SECOND_THREADS, "second");
// Ask the worker of the second runtime, from a task running on it,
// to spawn through the public entry point. The second runtime is
// alive and idle, so the task has to run.
let (tx, rx) = mpsc::channel();
second.get_handle().spawn(async move {
let spawned = current_handle().spawn(async { 7u32 }).await;
let _ = tx.send(spawned.map_err(|e| e.to_string()));
});
let outcome = rx
.recv_timeout(Duration::from_secs(30))
.expect("the worker of the second runtime polls the probe task");
match outcome {
Ok(value) => assert_eq!(value, 7, "the probe task returns its own value"),
// Observed: "task 2 was cancelled".
Err(err) => panic!(
"a worker of the live second runtime spawned onto the shut-down first one: {err}"
),
}
}
```
Contributor guide
Research direction
Start with init_pools() and current_handle() in pingora-runtime, then copy the supplied reproduction into pingora-runtime/tests/ and run cargo test -p pingora-runtime --test . Trace how worker registration survives shutdown and thread-id reuse. Done means the test passes and a live NoStealRuntime's current_handle() spawns work on that runtime rather than the shut-down one.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100