Skip to content

fix(runtime): bound the worker join in teardown_workers (#2508) - #2875

Closed
gertybotbot wants to merge 1 commit into
hew-lang:mainfrom
gertybotbot:bound-teardown-worker-join
Closed

fix(runtime): bound the worker join in teardown_workers (#2508)#2875
gertybotbot wants to merge 1 commit into
hew-lang:mainfrom
gertybotbot:bound-teardown-worker-join

Conversation

@gertybotbot

Copy link
Copy Markdown
Contributor

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 emits hew_runtime_cleanup, and the actual exit path for a compiled binary goes through shutdown_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::JoinHandle has no join-with-timeout, so the join is bounded with a completion signal rather than by bounding join() itself:

  • Each worker decrements a live_workers: Mutex<usize> 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 reaches zero, every outstanding join() is known to be non-blocking, so the existing join loop runs unchanged.
  • Workers are registered in the count before spawn, because a fast worker can reach its exit guard before spawn returns; registering afterwards could leave the count permanently above zero. A spawn failure rolls the registration back.
  • A caller that is itself one of the counted workers discounts itself from the wait target, matching the existing self-join skip in the join loop.

The bound is HEW_WORKER_JOIN_TIMEOUT_MS, defaulting to 5 s to match shutdown::DEFAULT_DRAIN_TIMEOUT_MS — the drain and the join are the two halves of the same wind-down. Setting it to 0 restores the historical unbounded join.

The leak-on-timeout tradeoff

On timeout, the straggler threads are detached (their JoinHandles dropped) rather than joined, and teardown_workers returns converged = false.

Detaching is only sound if nothing the straggler touches is freed underneath it. A live worker holds a borrow of the Scheduler (for shutdown, the parkers, the queues) and of the RuntimeInner that owns it, so this PR makes the non-convergence signal load-bearing:

  • teardown_workers never returns a detached runtime on the timeout path — it returns (false, None), so no caller can be handed a Box<RuntimeInner> to drop while a worker may still dereference it.
  • hew_runtime_cleanup fail-closes on the signal. It skips supervisor reclamation, free_registered_supervisors, cleanup_all_actors, hew_registry_clear, and the final take_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, converged is true, and every handle is joined exactly as before. hew_runtime_cleanup proceeds through its full sequence unchanged.

Discriminating test

teardown_worker_join_is_bounded_but_control_joins_cleanly drives the same teardown_workers call against the same scheduler shape twice; the only difference is whether the registered worker ever publishes its exit.

  • Timeout half — a worker registered in live_workers that never decrements, standing in for a thread parked in a blocking syscall. Asserts teardown returns within the bound, reports converged == 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.
  • Control half — a worker that exits immediately through a real WorkerExitGuard. Asserts converged == 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 return None (the pre-fix unbounded behaviour), the timeout half hangs indefinitely and had to be killed at a 60 s timeout(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_parses covers the 0 = unbounded opt-out and the default fallback.

Verification

All run bare, exit code captured on the next line:

Command rc
cargo clippy --all-targets -- -D warnings (workspace) 0
cargo test -p hew-runtime 0 (2242 lib tests passed, 0 failed)
cargo fmt --all --check 0

`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
@gertybotbot

Copy link
Copy Markdown
Contributor Author

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 main — codegen never emits hew_runtime_cleanup". That is true of hew_runtime_cleanup, but it is not the only caller. hew_sched_shutdown also reaches teardown_workers directly (hew-runtime/src/scheduler.rs:710:715), and codegen emits hew_sched_shutdown straight into native main for supervisor programs — the emit_immediate_shutdown_epilogue gate at hew-codegen-rs/src/llvm.rs:30438, emitted at :26036. So the unbounded join is reachable from a compiled binary today, not merely latent for the unwired embedder path. Merging this text would put an understated severity into the record.

2. It is 2.5x larger for the same outcome (+372/−33 here vs +144/−6 in #2876), because bounding the join via a live_workers count plus WorkerExitGuard and an exit_cond Condvar adds a second synchronization structure to the scheduler. #2876 polls is_finished() against one deadline shared by the whole worker set and detaches rather than force-killing a worker still parked in a syscall, since it may still touch scheduler-owned memory — bounding shutdown must not trade a hang for a use-after-free.

No work is lost: #2508 stays covered by #2876, which is open, mergeable, and green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

teardown_workers joins parked workers with no timeout (unreachable for compiled main today; blocks a future host-embedder path)

1 participant