[staging CI] unslothai/unsloth#8170 - #217
danielhanchen wants to merge 6 commits into
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
…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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2b41006ad
ℹ️ 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 Studio is killed on macOS during a Python or shell tool call, the child continues running because _python_exec and _shell_exec spawn at tools.py:10332 and tools.py:10457 without calling adopt_pid. Consequently this sweep has no record of those processes and cannot reap them on the next launch; a sleeping or file-holding tool payload can therefore survive indefinitely despite the new abnormal-exit cleanup.
Useful? React with 👍 / 👎.
| tmp = path.with_suffix(".tmp") | ||
| tmp.write_text(json.dumps(payload), encoding = "utf-8") | ||
| tmp.replace(path) |
There was a problem hiding this comment.
When two worker threads call adopt_pid or forget_pid concurrently, both writes use the same .tmp path without a lock. An older _tracked_pids snapshot can be written and replaced after the newer snapshot, or one replace can consume the other thread's temp file; the exception is then swallowed. On macOS this can omit a live child from the only crash-survivable record, so a subsequent startup will leave that child orphaned.
Useful? React with 👍 / 👎.
| del CTRL_C_EVENT, CTRL_BREAK_EVENT | ||
| return False |
There was a problem hiding this comment.
On Windows, Ctrl+C and Ctrl+Break take this non-close branch, but del CTRL_C_EVENT, CTRL_BREAK_EVENT makes both names local to the callback and raises UnboundLocalError before return False. Because this runs through a ctypes console-control callback, the exception is reported/ignored at the callback boundary instead of intentionally passing the event onward to Python's signal handler, making Ctrl-event handling unreliable.
Useful? React with 👍 / 👎.
1 participant
Disposable CI run for unslothai/unsloth#8170. Do not merge; closed after CI.