Repository navigation
fix(engine): governor threads and drain-free spawn survive a failed spawn (#1122) - #1460
Conversation
…pawn (#1122) Root cause. The release profile is panic = "abort", and a thread spawn can fail in two ways that both end the node: * Builder::spawn returns Err on EAGAIN (thread or memory limit), and std::thread::spawn / Scope::spawn turn that Err into a panic; * std panics inside the NEW thread when it cannot mmap the thread's alternate signal stack under memory exhaustion -- the caller never sees an Err at all. That is the `xerj-drain-free` abort in #1122. The first is fixed by handling the Err; the second can only be avoided by not spawning while memory is exhausted, which the governor's thread_spawn_safe() (rc.81) already answers -- but only memtable.rs asked it. What this adds. xerj-common::thread_spawn -- one place for the pattern every crate needs: * try_spawn(name, f): never panics on a failed spawn; on failure the closure comes BACK in SpawnFailure::job (Builder::spawn drops it), so the caller can run it inline, retry, or drop it on purpose; * a process-wide spawn gate (install_spawn_gate / spawn_allowed) that refuses a spawn up front, with the same hand-back; * spawn_with_retry(name, attempts, backoff, f) for threads that must exist; * a Spawn trait (StdSpawn in production) so the failure paths are unit tested with an injected spawner instead of an exhausted machine. xerj-engine: * governor::init installs thread_spawn_safe as that gate, so storage, server and the rest consult the same RSS-watermark check without depending on the engine. * engine.rs spawn_resource_sampler: the three governor threads (xerj-mem-drain, xerj-mem-sampler, xerj-memtable-sampler) were `let _ = Builder::spawn(..)` -- a failed spawn was dropped silently and the node ran on with no parent memory breaker. They now go through spawn_governor_thread: 3 attempts with 50 ms doubling back-off, then an ERROR naming the thread and that the breaker is degraded. The drain request channel (MEMORY_DRAIN_TX) is published only when the drain worker actually started. * memtable.rs MemtableClaim::retire (xerj-drain-free): goes through try_spawn; on refusal or failure the returned job drops the dead generations inline -- the fallback the code always intended, now explicit. Same gate as before (governor::thread_spawn_safe). Behaviour changes only on the spawn-failure path. With no governor installed (unit tests, library embedding) spawning is unrestricted, exactly as before. Tests: 6 in xerj-common (failed spawn returns the job, closed gate never calls the spawner, success runs once on the named thread, retry rides out transient EAGAIN, retry gives up and hands the job back, gate re-sampled per attempt) and 2 in xerj-engine (a governor thread that cannot spawn reports false instead of aborting; a closed gate is respected). Advances #1122.
Review follow-up. spawn_governor_thread consulted the process-wide spawn gate, which governor::init now installs as thread_spawn_safe: refuse while the breaker is engaged or live RSS is at the watermark. That gate is right for xerj-drain-free, which has an inline fallback, but wrong for the sampler and drain threads. They are spawned once, at boot, and they are what samples RSS, engages the breaker and drains memory. A node that boots above its watermark (a large WAL replay under a low XERJ_MAX_PROCESS_MEMORY_MB) had all 3 attempts refused within ~150 ms and then ran with no memory breaker, no 429 admission and no drain for the life of the process. Before this PR those threads always started. Before: gate closed at boot -> xerj-mem-sampler, xerj-mem-drain and xerj-memtable-sampler are never started (ERROR logged). After: the soft watermark no longer blocks them. A real pthread_create failure is still retried with back-off, then logged instead of aborting. The gated drain-free path in memtable.rs is unchanged. File: engine/crates/xerj-engine/src/engine.rs (spawn_governor_thread). Verified: cargo test -p xerj-engine --lib (759 passed), cargo test -p xerj-common thread_spawn (6 passed), cargo clippy -p xerj-common -p xerj-engine --all-targets -D warnings.
|
Independent review: approve (after one follow-up fix, pushed) Checked:
Defect fixed in 86f1934: The failure mode: a node that boots above its watermark (for example after a large WAL replay under a low Verified:
"Advances #1122" is correct. Storage, server and the rayon pools are still open. |
…when it cannot spawn (xerj-org#1122) Advances xerj-org#1122. Why --- Release builds are `panic = "abort"`. `pool.rs` built the phase-A scan pool with `ThreadPoolBuilder::build().expect(..)`, and `build()` returns `Err` when `pthread_create` fails with `EAGAIN` (thread or memory limit). Under memory exhaustion that `.expect` aborted the whole autoindex run. It is the first item in xerj-org#1481's "Still open on xerj-org#1122 after this series". The second item, the rayon global-pool users in `content.rs`, `watch.rs` and `lib.rs`, turned out to be the same site. On main every one of their `par_iter`s already runs inside `crate::pool::install` (xerj-org#240), so none of them reaches rayon's global pool while the scan pool exists. They would reach it only if `install` fell through without a pool. That is why the fallback below does not just call `f()`: a `par_iter` on a thread that is not a rayon worker goes to the lazily built global pool, which panics when it cannot start its threads. What ---- `pool.rs`: - Scan-pool construction is fallible. A failed `build()` is reported on stderr and is NOT cached: `SCAN_POOL` stays empty and the next phase tries again, at the width `configure` asked for (new `REQUESTED`), not at the machine's. A mutex serialises construction so two first callers do not both start a pool's worth of threads. - `install` with no pool runs `f` through `sequential`, which builds a width-1 `use_current_thread()` pool. That builder spawns nothing; it registers the calling thread as the pool's only worker, so `f`'s `par_iter`s run inline on it. rayon-core 1.13 uses the same construction as its own fallback when threads are unsupported (`registry.rs`, `default_global_registry`). If the caller is already a rayon worker, `f` runs on that worker's pool instead (a thread can belong to one pool only). - `scan_pool()` now returns `Option<&'static ThreadPool>`. The new `width()` (1 when degraded) replaces the three `scan_pool().current_num_threads()` callers (`lib.rs` progress width, two tests). Behaviour changes only on the failure path. When the pool builds, it is built at the same width, at the same moment, with the same thread names as before. Tests ----- The injectable `Spawn` hook from xerj-org#1460 is not on main yet (xerj-org#1460 is open), and it targets std threads anyway. These tests inject the failure through rayon's own `ThreadPoolBuilder::spawn_handler`, which returns an EAGAIN-style `io::Error`, so `build()` takes its real error path. - `a_pool_that_cannot_spawn_degrades_to_the_calling_thread`: a failed build returns `None` and leaves the slot empty. A 1,000-item `par_iter` through the fallback returns the full, ordered result, every item on the calling thread, with `current_num_threads() == 1`. A second phase on the same thread works. Once spawning succeeds, the next call builds and caches the real pool. - `the_fallback_inside_a_rayon_worker_stays_on_that_pool`: the fallback inside an existing worker runs on that worker's pool, with no registration error. Still open on xerj-org#1122 (not in this PR) ------------------------------------ - std panics inside a freshly spawned thread when it cannot map its sigaltstack. A rayon worker spawned while building the scan pool can still hit this. Only a spawn gate avoids it, which is xerj-org#1460's `install_spawn_gate`, and autoindex does not consult one yet. - `objsource.rs` `fetch_objects` already handles a failed fetch-pool build with `?`, which fails the object run with an error rather than aborting. It does not degrade to sequential. - The engine's lazily built rayon pools still `.build().expect(..)` (xerj-engine `lib.rs` ingest/background/merge pools, `index_guard.rs`), as xerj-org#1460 lists. - Dependencies that use rayon's global pool from outside `pool::install` were not audited.
|
Thanks for this contribution — we'd like to merge it. Before we can, we need a signed Contributor License Agreement from: @cursoragent It is a one-time thing per contributor, not per pull request. Read CLA.md, then open a small pull request adding your GitHub username to Once it is merged, comment |
main now includes #1596 (honour disk.threshold_enabled=false). The memory/disk sampler in engine.rs conflicted with this branch's spawn-failure path. The resolved loop still starts xerj-mem-sampler through spawn_governor_thread, and each sample still applies cluster.routing.allocation.disk.threshold_enabled. governor.rs auto-merged: the spawn gate and the disk-threshold flag both remain. Co-authored-by: xerj-org <xerj-org@users.noreply.github.com>
004f70f to
24945dd
Compare
Advances #1122.
Why
The release profile is
panic = "abort". A thread spawn fails in two ways, and both used to take the node down:Builder::spawnreturnsErronEAGAIN(thread or memory limit).std::thread::spawnandScope::spawnturn thatErrinto a panic.Err. This is thexerj-drain-freeabort in engine: node dies SIGABRT under memory exhaustion — std thread-spawn sigaltstack panic hits panic="abort" #1122.Handling the
Errfixes (1). The only way to avoid (2) is to not spawn while memory is exhausted. The governor'sthread_spawn_safe()(rc.81) already gives that answer, butmemtable.rswas the only caller that checked it.What
xerj-common::thread_spawnis the shared helper the follow-up PRs use:try_spawn(name, f)never panics on a failed spawn. On failure the closure comes back inSpawnFailure::job(Builder::spawndrops it), so the caller can run it inline, retry, or drop it on purpose.install_spawn_gate/spawn_allowed) refuses a spawn up front and hands the closure back the same way.spawn_with_retry(name, attempts, backoff, f)is for threads that must exist.Spawntrait (StdSpawnin production) lets the tests inject a failing spawner, so the failure paths are tested without exhausting a real machine.xerj-enginegovernor::initinstallsthread_spawn_safeas the gate. Storage, server and other crates then use the same RSS-watermark check without depending on the engine.Call sites covered
engine.rsspawn_resource_sampler:xerj-mem-drainlet _ = Builder::spawn(..): a failure was dropped silently and the node ran without a breaker drainMEMORY_DRAIN_TXis published only if the worker startedengine.rs:xerj-mem-samplerengine.rs:xerj-memtable-samplermemtable.rsMemtableClaim::retire:xerj-drain-freelet _ = spawntry_spawn, which uses the same gate. On refusal or failure the returned job drops the dead generations inline, explicitlyBehaviour changes only on the spawn-failure path. With no governor installed (unit tests, library embedding), spawning is unrestricted, as before.
Tests
xerj-common(6): a failed spawn returns the job; a closed gate never calls the spawner; a successful spawn runs the job once on the named thread; retry rides out a transientEAGAIN; retry gives up and hands the job back; the gate is re-sampled on every attempt.xerj-engine(2): a governor thread that cannot spawn reportsfalseinstead of aborting; a closed gate is respected.cargo clippy -p xerj-common -p xerj-engine --all-targets -D warningsis clean, andcargo test -p xerj-commonpasses.index::exact_scan_hydration_tests::exact_scan_is_bit_identical_to_the_clone_everything_scanfails locally on unmodifiedorigin/mainas well (different assertion each run). This PR does not touch it.Not in this PR
Storage (
merge.rs.expect, the WAL fsync worker using the gate), server, autoindex, and the engine's lazily built rayon pools (.build().expect(..)) are handled in follow-up PRs or listed on #1122.🤖 Generated with Claude Code