Repository navigation
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: block SIGTERM and SIGINT, wait for them with sigwait on a dedicated thread, and route the first signal through Control::request_stop(). request_stop locks and notifies the wake, so calling it from another thread is safe. std and portable-pty clear the signal mask before exec, so children behave as before. I couldn't build or run this here because the sandbox has no Rust or Zig toolchain, so it's reviewed from the code only.
Public API: unchanged. This only affects how the daemon handles signals internally.
One ordering problem, see the inline comment: the signals are blocked after Runtime::bind, which has already started the sweeper thread. That leaves a window where the daemon can still die the old way.
| // Created after bind so bind I/O does not consume the acquire window. | ||
| let control = Arc::new(Control::new(wake, Ownership::new(Instant::now()))); | ||
| #[cfg(unix)] | ||
| stop_on_signals(&control)?; |
There was a problem hiding this comment.
platform::Runtime::bind already starts a thread on Unix: the stale-runtime sweeper (thread::spawn(move || super::sweep::stale_runtimes(&directory)) in daemon/unix.rs). That thread keeps the original signal mask, so the doc comment's "blocked on this thread before any worker exists" isn't true. While the sweeper runs (longer when there are many stale runtimes), the kernel can deliver SIGTERM or SIGINT to it. The default action then kills the process and leaves the runtime directory, socket and service.json behind. The registration is already written by then, so other processes can find the daemon and signal it during that window.
Fix: block the signals at the very top of run, before Wake::new()/bind, and only start the sigwait thread once Control exists. A signal that arrives during bind stays pending and is picked up by the waiter.
There was a problem hiding this comment.
The earlier ordering point is fixed: SIGTERM and SIGINT are now blocked at the very top of run, before bind starts the sweeper thread or writes service.json, so every later thread inherits the block. The rest of the new code also reads correctly: the libc::sigwait loop with its EINTR retry, the reset to the default action after blocking, the 1 s grace before a second signal calls _exit, and removing the runtime files when the signal thread can't start. I found nothing new that needs changing.
This is from reading the code only. This environment has no Rust or Zig toolchain, so I couldn't build the daemon or run its tests.
Public API: unchanged.
Issue for this PR
Closes #
Type of change
What does this PR do?
The PTY daemon had no handler for SIGTERM or SIGINT, so either signal killed it on the spot: its terminals were left to die on SIGHUP and its runtime directory, socket and registration stayed behind until a later sweep. Once the daemon also runs non-PTY commands (next PRs), those children don't get SIGHUP at all and would keep running.
runfirst blocks SIGTERM and SIGINT, beforebindstarts the sweeper thread or publishesservice.json, so every later thread inherits the mask. It then resets both to their default action (macOS never delivers an ignored signal tosigwait, and a launcher may leave SIGINT ignored).libc::sigwait(retrying onEINTR). The first signal callsControl::request_stop(), so the daemon goes through its normal stop: kill every terminal, join workers, remove the runtime files. A second signal more than 1 s later means the stop is stuck and calls_exit(128 + signal); one within 1 s (Ctrl-C plus a wrapper's SIGTERM) is part of the same stop.libcdirectly rather than nix'sSigSet::wait, which treatssigwait's positive error return as success and reads an uninitialized signal.exec. portable-pty resets it for PTY children, so terminals are unaffected; the new test checks that a terminal's process still dies from each signal. Anything else the daemon spawns must reset the mask itself (the pipe-mode PR does). Windows is unchanged.How did you verify your code works?
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warningsand the macOS test suite inpackages/process-daemon; clippy forx86_64-pc-windows-msvcandx86_64-unknown-linux-gnu. New teststop_signals_stop_terminals_and_clean_up(against the real binary, for both signals): a terminal's child still receives the signal, and after the daemon gets the signal it exits successfully, removes its runtime directory and socket, and its terminal process is gone.Checklist