Studio: stop leaking child processes on an abnormal exit - #8170
Conversation
A Windows user could not update Studio until they killed a stray python by hand: the tool sandbox runs its payload under a shell wrapper, the kill path reaped only the wrapper, and `unsloth studio update` then refused to run because a process still held the managed environment. - taskkill /T on Windows, so a tool payload cannot outlive its wrapper - a console-close handler, since CTRL_CLOSE_EVENT never becomes a Python signal and the graceful shutdown was skipped entirely when the window was closed - the desktop updater drains the app job before standing down crash cleanup, and re-arms it when the install never happens - children are recorded on disk and swept at the next startup, which is the only reaper macOS has after a crash or a force quit - the job status is logged instead of failing silently Also fixes a liveness probe that used os.kill(pid, 0); on Windows that is TerminateProcess, so it killed the process it was asking about.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…d per owner, drop the job drain - the console handler ran _signal_handler on the thread Windows creates for the event, where signal.signal raises, so closing the window did no cleanup at all - bound that work to the ~5s Windows allows before it kills the process - one record file per owner pid: two Studios can share a home, and a single file let the second erase the first's children - add a Windows process identity (creation time) and refuse to signal a pid that cannot be verified - drop the whole-job drain: it would also terminate the WebView2 hosts, and cleanup_child_processes already taskkills the backend tree
A record that is not an object, or whose children are not dicts, raised out of the startup sweep and would have stopped Studio from starting. Pair the Linux start time with the command name as well, since start time alone has 10ms granularity.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5185a542af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if sys.platform == "darwin": | ||
| assert survived, "macOS now reaps orphans -- update this repro" | ||
| else: | ||
| assert not survived |
There was a problem hiding this comment.
On Linux this new contrast test fails for the same shell-wrapped shape it builds: child_popen_kwargs() installs PR_SET_PDEATHSIG only in the direct bash wrapper, and when _run_case() SIGKILLs the parent, bash dies but the Python payload it spawned remains alive. Update the expectation or make the Linux path actually bind/kill the whole shell tree; otherwise the new test suite encodes a guarantee the implementation does not provide.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this one and it does not reproduce: the test passes on Linux. _get_shell_cmd produces bash -c '"python" -c "..."', a single simple command, so bash execs it rather than forking. Measured it: the Popen pid and the payload's os.getpid() are the same process, and PR_SET_PDEATHSIG survives execve, so there is no grandchild for the guarantee to miss. The assertion is right as written.
|
Follow-up verification, plus a fix pushed to this branch. What happened before, and what happens now
Real or notReal, and both sides were proved on a Windows runner rather than argued: with kill-on-close in force the child did not survive the parent, and with it disabled (which is exactly what the updater path does) it did. Does merging break anythingNo new hardware path, and none of this is GPU or device specific. The risk in a reaper is killing something it should not, so that is what the tests are aimed at:
Fix in this pushA record that was not an object, or whose Testing
|
Ctrl+C on Windows raised UnboundLocalError inside the console callback, where the BOOL result is then undefined, so the event could be reported as handled and Studio would not stop. The updater no longer re-arms kill-on-close before relaunching, which would have made the old process kill the replacement it just started, and it resets the exit-cleanup guard so a retry after a failed installer still reaps the backend. The RAG embedder and cloudflared are recorded like the other sidecars, a llama-server that survived a failed kill stays recorded, the Windows kill path checks the captured creation time before taskkill, and record writes are serialised.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The delayed Windows kill skips a captured pid it cannot verify, since the job object still takes the tree at exit. An owner whose identity cannot be read counts as live rather than gone, so a momentary ps failure no longer costs a running Studio its sidecars, and that lookup pins TZ so a timezone change does not read as a different process. A child that outlived terminate_all keeps its record instead of losing the only handle on it, with zombies told apart from survivors. The whole relaunch handoff is inside the recovery scope, so any path that leaves this process running re-arms cleanup.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
terminate_all now applies the same test the startup sweep does: a pid whose identity cannot be read is left alone and kept in the record for the next launch to retry, rather than signalled on the chance it is still ours. The startup reaper re-checks liveness before dropping a record, so a kill that did not take stays reapable, and the breadcrumb unlink happens under the record lock so a concurrent adopt cannot have its record deleted from under it.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6755e699d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # crashed left its sidecars running. Sweep before spawning anything: a | ||
| # leftover holds VRAM, a port, and the files an update has to replace. | ||
| try: | ||
| reaped = reap_recorded_children() |
There was a problem hiding this comment.
When the macOS/Linux desktop app is force-killed with SIGKILL, the new sweep never fixes the leak: studio/src-tauri/src/process.rs only makes the Python backend a process-group leader, so killing Tauri does not terminate that backend, and this sweep runs inside the still-alive backend rather than the replacement desktop process. Even if another backend starts, utils/process_lifetime.py::_reap_one_record explicitly skips records whose Python owner is alive, leaving the backend and its llama/cloudflared children consuming the port and GPU after the Force Quit scenario this change is intended to cover. Bind the backend itself to the desktop parent's lifetime or have desktop startup identify and terminate the abandoned owned backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this and I do not think it is a gap. A backend outliving a force-quit desktop is deliberate: the app writes owner metadata (token, backend_pid, port) to disk, and preflight.rs probes and verifies that on the next launch, then adopt_verified_backend reclaims it. Binding the backend to the desktop's lifetime would break adoption, which exists so a desktop restart does not have to reload the model. The sweep skipping records whose owner is alive is the same deliberate rule that stops a second Studio killing a running one's sidecars, and there is a test for it. The residual is a backend running until the next launch reclaims it, which is the design rather than something this PR left behind.
Linux startup probes prctl with the read-only PR_GET_PDEATHSIG before reporting the parent-death signal as in force, so a seccomp or container policy that blocks it is reported as such rather than as a guarantee nothing keeps. The backstop sweep takes its snapshot under the lock the writes already hold.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The leader-only fallback runs when the job object was unavailable, which is exactly when the record is the only handle on those workers. Both callers read the dead leader as the tree being gone and dropped it, so anything that survived became unreachable. The tree kill now reports whether it took, and a failure keeps the pid tracked and its record on disk for the next launch.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The installer announced its validation server as stopped as soon as the group leader exited, which drops the record while a child that ignored the SIGTERM is still holding the GPU. It now waits for the group to empty, escalates to SIGKILL, and only announces the stop once nothing is left. An installer timeout killed the installer alone and left the announced server for a sweep that never runs while this process lives; those children are now terminated with it. The diffusion group id is kept from the spawn, so a shim that exited before the kill path (a failed health check, a crash before a reload) no longer leaves its visual server with nothing able to reach it.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # studio/src-tauri/src/main.rs
The timeout path took them; a nonzero exit or a stream that ended mid-line left them running. This process stays up after an update failure, and its own live record shields those pids from a sweep that would not run anyway, so the cleanup now happens in the finally that covers every way out.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
terminate_pid falls back to the recorded process group when the leader has already exited, keeps the record when a Windows tree kill could not be confirmed, drains the announced children under a lock so the watchdog and the reader thread cannot race, and the installer arms the parent-death signal on the validation server it puts in a session of its own.
for more information, see https://pre-commit.ci
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…dentity before a single-pid kill The desktop stops this backend by signalling its process group and force-kills it five seconds later, so a runner in a session of its own survives a backend that is slow to shut down and keeps the GPU until the next launch sweeps it. Put it back in that group, as the component installer already is, and reach the visual server by walking the runner's children instead of killpg. The cached group id goes with it: a pid is reusable once nothing holds the number as a process group any more, so an id kept past its group eventually names a stranger. terminate_pid signalled on the pid alone. An announced validation server can exit without the line that clears it, so run the same identity test terminate_all does before either termination branch.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The server was started in a session of its own everywhere, but only Linux can pair that with a parent-death signal. On macOS it left the group Studio force-kills while the only record of it is the announcement the backend has yet to read, so a kill in that window orphaned it with nothing able to find it. It stays in the inherited group there, and the kill path only reaches for killpg when the server actually leads a group, so a shared group is never signalled.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
A Windows user reported that shutting Studio down to update it left errors behind and the update would not run until they killed a stray python by hand. The chain is real and reproduces on a Windows runner:
cmd /corbash -c)_capture_process_groupreturnedNoneon Windows and_kill_process_treefell back toproc.kill(), so only the wrapper died and the payload, usually the venv's own python, kept runningensure_managed_environment_is_idlethen refuses the update while any process image under<studio home>\unsloth_studiois alive:The managed Studio environment is in use by python.exe (PID 9008). Stop that process, then retry the update.Changes
Windows process trees.
taskkill /T /Fin_kill_process_tree, and_capture_process_groupcaptures the wrapper pid (tagged) rather than returningNone, so a payload that outlives its wrapper is still reachable from_killpg_captured.Console close. Closing the console window raises
CTRL_CLOSE_EVENT, which Python never turns into a signal, so neither the signal handler noratexitran and the cooperative cleanup was skipped completely._install_windows_console_handlerruns_graceful_shutdownfor close, logoff and shutdown events, and passes Ctrl+C and Ctrl+Break through to the existing signal path rather than shutting down twice.Desktop updater.
suspend_for_update_installerclearedKILL_ON_JOB_CLOSEand nothing ever restored it, so from that point the app ran with no reaper. It now drains the job first, since clearing the flag removes the last backstop and anything the cooperative stop missed would outlive the app and hold the venv open against the installer. A newresume_desktop_update_cleanupcommand re-arms the flag when the install fails or is cancelled, called from thefinallyinuse-tauri-update.macOS. No
PR_SET_PDEATHSIG, no job objects, so a crash or a force quit left llama-server and the other sidecars running. Tracked children are now recorded under the studio home and swept at the next startup, before anything new spawns. llama-server is tracked throughadopt_pid/forget_pidalongside its existing pidfile. Ownership is decided by start-time identity rather than pid alone, because a pid is recycled quickly enough on a busy machine to make a dead owner look alive.Visibility.
_install_windows_jobfailed silently in four places. The outcome is recorded and logged, and readable throughwindows_job_status().Also fixes a real bug found while testing this: the liveness probe used
os.kill(pid, 0), which on Windows isTerminateProcess(handle, 0), so it killed the process it was asking about. It usesOpenProcessplusWaitForSingleObjectnow, treatingACCESS_DENIEDas alive.Tests
test_orphaned_children.pyandtest_child_lifetime_boundary.py, driving real processes rather than mocks. They cover the shell payload dying with its wrapper, the update gate firing on a single orphan, the console handler, the job status, the record and its sweep, a live owner never being reaped, and the liveness probe not killing its subject. The boundary tests pin both sides on Windows: with the job in force a grandchild is reaped, without it the grandchild survives, which is what the updater drain exists to prevent.Validated on
ubuntu-latest,windows-latestandmacos-14, pluscargo check --all-targetsand the threewindows_jobtests on a real Windows toolchain. End to end against an installed Studio: kill the server outright, and where macOS previously left llama-server running, the next launch logsReaped 1 orphaned child process(es) left by a previous Studioand nothing survives.