fix(runtime): bound the worker join in teardown_workers (#2508) - #2875
fix(runtime): bound the worker join in teardown_workers (#2508)#2875gertybotbot wants to merge 1 commit into
Conversation
`teardown_workers` hard-joined every worker thread with no timeout. A worker parked in a blocking syscall never reaches its loop-top shutdown recheck, so the join was unbounded and could hang an embedding host's shutdown forever. `JoinHandle` has no join-with-timeout, so bound it with a completion signal instead: each worker decrements a `live_workers` count from a `WorkerExitGuard` that runs on every exit edge (normal break and panic unwind), notifying an `exit_cond` condvar. Teardown waits for the count to reach zero with a deadline; once it does, every outstanding `join()` is known to be non-blocking. Workers are registered in the count *before* spawn, since a fast worker can reach its exit guard before `spawn` returns. On timeout the straggler threads are detached rather than joined, and `teardown_workers` reports non-convergence. `hew_runtime_cleanup` fail-closes on that signal: it keeps the runtime installed and skips supervisor reclamation, `cleanup_all_actors`, the registry clear, and the final `take_default` free, all of which document "all workers have been joined" as their precondition. This mirrors the existing supervisor-pin-drain timeout policy in the same function, which already leaks rather than freeing beneath a live dereference. The bound is `HEW_WORKER_JOIN_TIMEOUT_MS`, defaulting to 5 s to match `shutdown::DEFAULT_DRAIN_TIMEOUT_MS`; `0` restores the historical unbounded join. The normal path is unchanged: when workers exit promptly the counter is already zero, the wait returns immediately, and every handle is joined as before. The spawn phase moves into a `spawn_workers` helper to keep `hew_sched_init` under the `too_many_lines` threshold. Refs hew-lang#2508
|
Closing as superseded by #2876, which fixes the same issue (#2508) in the same file. Both PRs came from parallel sessions of mine ~8 minutes apart; #2876 is the one I carried forward, for two reasons: 1. This PR's severity assessment is wrong. The body claims the path is "currently unreachable for a compiled native 2. It is 2.5x larger for the same outcome (+372/−33 here vs +144/−6 in #2876), because bounding the join via a No work is lost: #2508 stays covered by #2876, which is open, mergeable, and green. |
Closes #2508.
Problem
teardown_workers(hew-runtime/src/scheduler.rs) hard-joined every worker thread with no timeout. A worker genuinely parked in a blocking syscall never reaches the loop-top park-recheck, so the join was unbounded.This path is currently unreachable for a compiled native
main— codegen never emitshew_runtime_cleanup, and the actual exit path for a compiled binary goes throughshutdown_orchestrate's own bounded drain instead. This is latent hardening for the not-yet-wired host-embedder path (runtime_handle.rs), where an unbounded join would hang the embedding host's shutdown forever.Approach
std::thread::JoinHandlehas no join-with-timeout, so the join is bounded with a completion signal rather than by boundingjoin()itself:live_workers: Mutex<usize>from aWorkerExitGuardthat runs on every exit edge (normalbreakand panic unwind), notifying anexit_cond: Condvar.join()is known to be non-blocking, so the existing join loop runs unchanged.spawn, because a fast worker can reach its exit guard beforespawnreturns; registering afterwards could leave the count permanently above zero. A spawn failure rolls the registration back.The bound is
HEW_WORKER_JOIN_TIMEOUT_MS, defaulting to 5 s to matchshutdown::DEFAULT_DRAIN_TIMEOUT_MS— the drain and the join are the two halves of the same wind-down. Setting it to0restores the historical unbounded join.The leak-on-timeout tradeoff
On timeout, the straggler threads are detached (their
JoinHandles dropped) rather than joined, andteardown_workersreturnsconverged = false.Detaching is only sound if nothing the straggler touches is freed underneath it. A live worker holds a borrow of the
Scheduler(forshutdown, the parkers, the queues) and of theRuntimeInnerthat owns it, so this PR makes the non-convergence signal load-bearing:teardown_workersnever returns a detached runtime on the timeout path — it returns(false, None), so no caller can be handed aBox<RuntimeInner>to drop while a worker may still dereference it.hew_runtime_cleanupfail-closes on the signal. It skips supervisor reclamation,free_registered_supervisors,cleanup_all_actors,hew_registry_clear, and the finaltake_default()free — every one of which documents "all workers have been joined" as its precondition. It sets a last-error and returns.The tradeoff: on the timeout path the runtime, its scheduler (deques, parkers, stealers, global queue), the actor registry, and any registered supervisors are all leaked, and the straggler threads keep running. That is deliberate. The alternative — freeing on schedule — is a use-after-free under a live worker. This mirrors the policy already in the same function for the supervisor-pin-drain and destructor-owner-drain timeouts, which likewise keep the runtime and allocations installed rather than freeing memory beneath a live dereference. A host that hits this is shutting down anyway; it trades bounded memory for a bounded, non-hanging shutdown.
No change on the normal path
When workers exit promptly the counter is already zero (or reaches it immediately), the wait returns without blocking,
convergedis true, and every handle is joined exactly as before.hew_runtime_cleanupproceeds through its full sequence unchanged.Discriminating test
teardown_worker_join_is_bounded_but_control_joins_cleanlydrives the sameteardown_workerscall against the same scheduler shape twice; the only difference is whether the registered worker ever publishes its exit.live_workersthat never decrements, standing in for a thread parked in a blocking syscall. Asserts teardown returns within the bound, reportsconverged == false, hands back no runtime, and detached the handle. It also asserts the call took at least half the bound, so the assertion cannot be satisfied by teardown simply not waiting at all.WorkerExitGuard. Assertsconverged == true, the handle was joined, and the call finished in less than the bound — proving it joined rather than waiting out the deadline.The bound was proven to fire by observing the timeout path rather than assuming it: with
worker_join_timeout()mutated to always returnNone(the pre-fix unbounded behaviour), the timeout half hangs indefinitely and had to be killed at a 60 stimeout(1). With the fix, both halves complete in 0.35 s. A test that merely passed on both would not have distinguished these.worker_join_timeout_env_parsescovers the0= unbounded opt-out and the default fallback.Verification
All run bare, exit code captured on the next line:
cargo clippy --all-targets -- -D warnings(workspace)cargo test -p hew-runtimecargo fmt --all --check