Cross-OS validation: Studio Ctrl+C shutdown ordering (unsloth #6566) - #215
danielhanchen wants to merge 2 commits into
Conversation
Validates install.sh + run.py shutdown fixes on Linux, macOS, Windows, WSL. Sweeps the staging workflow bloat and adds one focused workflow per OS that runs shell syntax checks, py_compile, the AST regression tests, and pty-based behavioural tests (prompt decline, trap ordering, uvicorn thread wait, bounded join, force-quit). pty tests auto-skip on native Windows where install.ps1 is the relevant path.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust shutdown handling for Unsloth Studio, ensuring that the terminal shell prompt does not return while the background uvicorn thread is still writing shutdown logs. It adds a bounded wait for the server thread, restores default signal handlers on the first interrupt to allow a second Ctrl+C to force-quit, and implements Strix Halo ROCm-on-WSL rerouting to Ubuntu 24.04. Additionally, comprehensive contract and behavioral tests are added to validate these fixes across different operating systems. The review feedback highlights a critical issue where using trap '' INT in the installer script causes child processes to inherit the ignored signal, preventing graceful shutdown; it suggests using trap ':' INT instead and updating the corresponding tests. Other recommendations include using time.monotonic() instead of time.time() to avoid system clock adjustment issues, and killing child process groups in tests to prevent potential hangs during os.waitpid.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # the shell to die parsing the now-truncated tail (`unexpected fi`). | ||
| # Ignore Ctrl+C so this shell waits for studio's own graceful | ||
| # shutdown instead of dying first and racing the prompt over its logs. | ||
| trap '' INT |
There was a problem hiding this comment.
Using trap '' INT ignores the SIGINT signal (Ctrl+C) in the installer shell script. However, in POSIX-compliant shells, child processes spawned by the shell inherit ignored signals as ignored (SIG_IGN). This means that the Python unsloth studio process will also ignore SIGINT and will not raise KeyboardInterrupt or execute its graceful shutdown handler when the user presses Ctrl+C.
To fix this while still preventing the installer shell from exiting prematurely, use trap ':' INT. This catches the signal in the shell (running the dummy : command) but restores the default signal action (SIG_DFL) in child processes, allowing Python to receive SIGINT and shut down gracefully.
| trap '' INT | |
| trap ':' INT |
|
|
||
| def test_install_sh_traps_int_before_launch(): | ||
| txt = _INSTALL_SH.read_text(encoding="utf-8") | ||
| i_trap = txt.find("trap '' INT") |
There was a problem hiding this comment.
Update the test assertion to match the change from trap '' INT to trap ':' INT in install.sh.
| i_trap = txt.find("trap '' INT") | |
| i_trap = txt.find("trap ':' INT") |
| while True: time.sleep(0.2) | ||
| ''')) | ||
| installer = tmp_path / "installer.sh" | ||
| installer.write_text(f'trap \'\' INT\n{sys.executable} {child} </dev/null\necho EXITED\n') |
There was a problem hiding this comment.
Update the test helper to use trap ':' INT instead of trap '' INT to match the actual implementation and verify that the shell still waits for the child process correctly.
| installer.write_text(f'trap \'\' INT\n{sys.executable} {child} </dev/null\necho EXITED\n') | |
| installer.write_text(f'trap \':\' INT\n{sys.executable} {child} </dev/null\necho EXITED\n') |
| end = time.time() + max_wait | ||
| while time.time() < end: | ||
| rl, _, _ = select.select([master], [], [], 0.1) | ||
| if master in rl: | ||
| try: c = os.read(master, 4096) | ||
| except OSError: break | ||
| if not c: break | ||
| out += c | ||
| if not typed and (ready_marker is None or ready_marker.encode() in out): | ||
| time.sleep(settle) | ||
| if type_after is not None: | ||
| os.write(master, type_after) | ||
| else: | ||
| os.write(master, b"\x04") # EOF | ||
| typed = True | ||
| t_type = time.time() | ||
| if done_markers and any(m.encode() in out for m in done_markers): | ||
| break | ||
| elapsed = (time.time() - t_type) if t_type else None |
There was a problem hiding this comment.
Using time.time() for measuring timeouts and elapsed time can lead to bugs if the system clock is adjusted (e.g., via NTP). It is highly recommended to use time.monotonic() instead.
| end = time.time() + max_wait | |
| while time.time() < end: | |
| rl, _, _ = select.select([master], [], [], 0.1) | |
| if master in rl: | |
| try: c = os.read(master, 4096) | |
| except OSError: break | |
| if not c: break | |
| out += c | |
| if not typed and (ready_marker is None or ready_marker.encode() in out): | |
| time.sleep(settle) | |
| if type_after is not None: | |
| os.write(master, type_after) | |
| else: | |
| os.write(master, b"\x04") # EOF | |
| typed = True | |
| t_type = time.time() | |
| if done_markers and any(m.encode() in out for m in done_markers): | |
| break | |
| elapsed = (time.time() - t_type) if t_type else None | |
| end = time.monotonic() + max_wait | |
| while time.monotonic() < end: | |
| rl, _, _ = select.select([master], [], [], 0.1) | |
| if master in rl: | |
| try: c = os.read(master, 4096) | |
| except OSError: break | |
| if not c: break | |
| out += c | |
| if not typed and (ready_marker is None or ready_marker.encode() in out): | |
| time.sleep(settle) | |
| if type_after is not None: | |
| os.write(master, type_after) | |
| else: | |
| os.write(master, b"\x04") # EOF | |
| typed = True | |
| t_type = time.monotonic() | |
| if done_markers and any(m.encode() in out for m in done_markers): | |
| break | |
| elapsed = (time.monotonic() - t_type) if t_type else None |
| try: os.close(master) | ||
| except OSError: pass | ||
| try: os.waitpid(pid, 0) | ||
| except OSError: pass |
There was a problem hiding this comment.
If the child process group hangs or ignores the EOF on the master pty, os.waitpid(pid, 0) will block indefinitely, hanging the entire test suite. To make the test harness more robust, explicitly terminate the child process group using os.killpg before waiting.
| try: os.close(master) | |
| except OSError: pass | |
| try: os.waitpid(pid, 0) | |
| except OSError: pass | |
| try: os.close(master) | |
| except OSError: pass | |
| try: | |
| import signal | |
| os.killpg(pid, signal.SIGKILL) | |
| except OSError: | |
| pass | |
| try: os.waitpid(pid, 0) | |
| except OSError: pass |
| t0 = time.time() | ||
| os.write(master, b"\x03") | ||
| if double_ctrl_c: | ||
| time.sleep(0.2); os.write(master, b"\x03") | ||
| ret = None | ||
| end = time.time() + max_wait | ||
| while time.time() < end: | ||
| rl, _, _ = select.select([master], [], [], 0.1) | ||
| if master in rl: | ||
| try: c = os.read(master, 4096) | ||
| except OSError: break | ||
| if not c: break | ||
| out += c | ||
| if ret is None and b"INTERACTIVE_PROMPT>" in bytes(out[cmd_end:]): | ||
| ret = time.time() - t0 | ||
| break |
There was a problem hiding this comment.
Using time.time() for measuring timeouts and elapsed time can lead to bugs if the system clock is adjusted (e.g., via NTP). It is highly recommended to use time.monotonic() instead.
| t0 = time.time() | |
| os.write(master, b"\x03") | |
| if double_ctrl_c: | |
| time.sleep(0.2); os.write(master, b"\x03") | |
| ret = None | |
| end = time.time() + max_wait | |
| while time.time() < end: | |
| rl, _, _ = select.select([master], [], [], 0.1) | |
| if master in rl: | |
| try: c = os.read(master, 4096) | |
| except OSError: break | |
| if not c: break | |
| out += c | |
| if ret is None and b"INTERACTIVE_PROMPT>" in bytes(out[cmd_end:]): | |
| ret = time.time() - t0 | |
| break | |
| t0 = time.monotonic() | |
| os.write(master, b"\x03") | |
| if double_ctrl_c: | |
| time.sleep(0.2); os.write(master, b"\x03") | |
| ret = None | |
| end = time.monotonic() + max_wait | |
| while time.monotonic() < end: | |
| rl, _, _ = select.select([master], [], [], 0.1) | |
| if master in rl: | |
| try: c = os.read(master, 4096) | |
| except OSError: break | |
| if not c: break | |
| out += c | |
| if ret is None and b"INTERACTIVE_PROMPT>" in bytes(out[cmd_end:]): | |
| ret = time.monotonic() - t0 | |
| break |
| try: os.close(master) | ||
| except OSError: pass | ||
| try: os.waitpid(pid, 0) | ||
| except OSError: pass |
There was a problem hiding this comment.
If the interactive bash shell or any of its children hang, os.waitpid(pid, 0) will block indefinitely. Explicitly terminate the child process group using os.killpg to ensure the test suite never hangs.
| try: os.close(master) | |
| except OSError: pass | |
| try: os.waitpid(pid, 0) | |
| except OSError: pass | |
| try: os.close(master) | |
| except OSError: pass | |
| try: | |
| import signal | |
| os.killpg(pid, signal.SIGKILL) | |
| except OSError: | |
| pass | |
| try: os.waitpid(pid, 0) | |
| except OSError: pass |
…strengthen tests
|
Cross-OS validation for unsloth unslothai#6566 is green on all four targets (final commit, install.sh subshell INT reset + run.py SIGBREAK restore):
Behavioural coverage includes: prompt decline (n/N/EOF do not launch, Enter does), the subshell trap reset delivering Ctrl+C to a child that relies on the default SIGINT (not swallowed by an inherited SIG_IGN), prompt-after-logs ordering, the bounded 5s join, and force-quit on a second Ctrl+C. Closing as a throwaway staging PR. The fix lives in unsloth unslothai#6566. |
1 participant
Throwaway staging PR to run the unsloth unslothai#6566 shutdown-ordering fixes (install.sh + run.py) across Linux, macOS, Windows, and WSL. One workflow per OS: shell syntax, py_compile, AST regression tests, and pty behavioural tests (prompt decline, trap ordering, uvicorn thread wait, bounded join, force-quit). Not for merge.