Studio: fix Ctrl+C shutdown ordering (installer shell + uvicorn thread wait) - #6566
Conversation
…own logs ordered
The `curl | sh` Studio auto-start prompt had two issues on Linux/macOS/WSL
(install.sh). install.ps1 already gates on input redirection, so Windows is
unaffected.
1. Typing n, or any closed/EOF /dev/tty, still launched Studio. The read
fallbacks defaulted to "y" (read failure, and the no-tty branch), so any
answer other than a cleanly delivered y/n line auto-started a blocking
foreground server. Default those to "n"; a real Enter still counts as yes
via ${_reply:-y}.
2. On Ctrl+C the shell prompt printed in the middle of Studio's shutdown logs.
The non-interactive installer shell took the default SIGINT action and died
before the child finished its graceful shutdown, so the prompt raced ahead
of "All subprocesses cleaned up". trap '' INT in the installer shell so it
waits for Studio's own graceful shutdown.
…rl+C Builds on #6565 by @Imagineer99. The studio server runs uvicorn in a daemon thread, so on Ctrl+C the process could return to the shell while that thread was still writing its shutdown logs, interleaving them with the prompt. Retain the uvicorn thread and join it (flushing stdout/stderr) before terminal entrypoints return, from run.py's main shutdown path and the CLI shutdown paths. Refinements over #6565: - Bound the join at 5s (_SERVER_SHUTDOWN_JOIN_TIMEOUT, matching the existing _graceful_shutdown subprocess timeouts) so a stalled uvicorn shutdown cannot hang the terminal; the timeout warning branch is now reachable. - Restore SIG_DFL for SIGINT/SIGTERM at the start of the signal handler so a second Ctrl+C force-quits, and drop the redundant in-handler wait (the post-loop wait already covers the signal path). Co-authored-by: Lee Jackson <130007945+Imagineer99@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
for more information, see https://pre-commit.ci
![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 improves the terminal shutdown behavior of Unsloth Studio by ensuring that the shell prompt does not return while the background uvicorn server thread is still writing shutdown logs. It introduces a bounded wait for the server thread, restores default signal handlers upon the first interrupt to allow a force-quit on a second interrupt, and adds regression tests for these behaviors. The review feedback highlights two important improvements: resetting the SIGINT trap in a subshell in install.sh to prevent child processes from inheriting an ignored signal disposition, and restoring SIGBREAK to its default handler on Windows to ensure force-quits work reliably there as well.
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.
| trap '' INT | ||
| "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null |
There was a problem hiding this comment.
When a shell script ignores a signal using trap '' INT, any child process executed by that shell inherits the ignored signal disposition (SIG_IGN). This means unsloth studio and any of its spawned subprocesses (such as llama-server, cloudflared, or training workers) will start with SIGINT ignored. While Python can override this, other non-Python subprocesses may not, leading to orphaned processes or broken graceful shutdowns upon pressing Ctrl+C.
To prevent this, reset the SIGINT trap to its default behavior within a subshell before executing the command.
| trap '' INT | |
| "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null | |
| trap '' INT | |
| (trap - INT; exec "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null) |
| signal.signal(signal.SIGINT, signal.SIG_DFL) | ||
| signal.signal(signal.SIGTERM, signal.SIG_DFL) |
There was a problem hiding this comment.
On Windows, some terminals send SIGBREAK instead of or in addition to SIGINT when Ctrl+C or Ctrl+Break is pressed. Since SIGBREAK is registered to this handler on Windows, we should also restore SIGBREAK to its default disposition (SIG_DFL) to ensure a second Ctrl+Break successfully force-quits the process if the graceful shutdown stalls.
| signal.signal(signal.SIGINT, signal.SIG_DFL) | |
| signal.signal(signal.SIGTERM, signal.SIG_DFL) | |
| signal.signal(signal.SIGINT, signal.SIG_DFL) | |
| signal.signal(signal.SIGTERM, signal.SIG_DFL) | |
| if hasattr(signal, "SIGBREAK"): | |
| signal.signal(signal.SIGBREAK, signal.SIG_DFL) |
|
@codex review |
![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: d116d7a417
ℹ️ 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".
| trap '' INT | ||
| "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null |
There was a problem hiding this comment.
When the user accepts the auto-start prompt, this trap '' INT is inherited by the foreground unsloth studio process on POSIX shells, so Python starts with SIGINT ignored rather than raising KeyboardInterrupt. The CLI path only catches KeyboardInterrupt and does not install its own SIGINT handler, so Ctrl+C no longer requests Studio's graceful shutdown in the installer-launched server; users are left with a foreground server that ignores Ctrl+C until they close the terminal or send another signal.
Useful? React with 👍 / 👎.
| signal.signal(signal.SIGINT, signal.SIG_DFL) | ||
| signal.signal(signal.SIGTERM, signal.SIG_DFL) |
There was a problem hiding this comment.
On Windows, the handler is also registered for SIGBREAK just below, and the existing comment notes that some terminals deliver Ctrl+C/Ctrl+Break that way. This restore block leaves SIGBREAK pointing at _signal_handler, so in those terminals a second Ctrl+C during a stalled graceful shutdown re-enters the cleanup path instead of taking the default action, defeating the new force-quit escape hatch for that environment.
Useful? React with 👍 / 👎.
- install.sh: run studio in a subshell that resets INT to default (trap - INT; exec ...) so the foreground child does not inherit the installer shell's ignored SIGINT, which would otherwise swallow the studio process's own Ctrl+C and graceful shutdown. - run.py: also restore SIGBREAK to SIG_DFL in the signal handler so a second Ctrl+Break force-quits on Windows, matching SIGINT/SIGTERM.
|
Addressed both review findings in 7e25c27:
Validated on Linux, macOS, Windows, and WSL via CI. |
…strengthen tests
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the shutdown behavior of Unsloth Studio to ensure the shell prompt does not return while the background server thread is still writing shutdown logs. It introduces a bounded wait for the uvicorn thread, flushes standard streams, and restores default signal handlers on the first interrupt to allow a second interrupt to force-quit. Additionally, install.sh is updated to run the studio in a subshell while ignoring INT in the parent shell. Feedback points out a critical issue in install.sh where set -e is active; if the subshell exits with a non-zero status, the script will terminate immediately instead of capturing the exit code in _LAUNCH_EXIT. A suggestion is provided to safely capture the exit status.
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.
| trap '' INT | ||
| (trap - INT; exec "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null) | ||
| _LAUNCH_EXIT=$? |
There was a problem hiding this comment.
Because set -e is active in install.sh, if the subshell exits with a non-zero status, the shell will terminate immediately and never reach the _LAUNCH_EXIT=$? assignment or the subsequent if block. This prevents the migration warning from being printed when _MIGRATED is true.
Using || _LAUNCH_EXIT=$? prevents set -e from triggering on failure and correctly captures the exit status.
| trap '' INT | |
| (trap - INT; exec "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null) | |
| _LAUNCH_EXIT=$? | |
| trap '' INT | |
| _LAUNCH_EXIT=0 | |
| (trap - INT; exec "$VENV_DIR/bin/unsloth" studio -p 8888 </dev/null) || _LAUNCH_EXIT=$? |
|
Codex Review: Didn't find any major issues. 🚀 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". |
… hint still prints
|
Good catch on the set -e interaction. install.sh runs under set -e, so a non-zero studio exit from the subshell would abort before |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the terminal shutdown behavior of Unsloth Studio by ensuring the uvicorn server thread is properly joined with a timeout, and restoring default signal handlers on interrupt to allow force-quitting. It also updates the installer to default to a non-interactive 'no' when the TTY is unreadable. The review feedback suggests avoiding broad, silent exception handling when flushing streams, and refactoring duplicated shutdown calls in the CLI commands into finally blocks for better maintainability.
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.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Avoid using silent exception handlers. Instead of silently ignoring the exception with pass, catch the specific exceptions and log them (at least at a debug level) to aid in troubleshooting and future debugging.
| except Exception: | |
| pass | |
| except (OSError, ValueError) as e: | |
| logger.debug("Failed to flush stream: %s", e) |
References
- Avoid using broad, silent exception handlers like
except Exception: pass. Instead, log the exception, even if at a debug level, to aid in future debugging.
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | ||
| except KeyboardInterrupt: | ||
| run_mod._graceful_shutdown(run_mod._server) | ||
| typer.echo("\nShutting down...") | ||
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() |
There was a problem hiding this comment.
The call to _wait_for_server_shutdown is duplicated for both the normal shutdown path and the KeyboardInterrupt handler. You can simplify this by using a finally block to ensure this cleanup logic is always executed upon exiting the try block, regardless of how it exits. This will make the code cleaner and more maintainable. Additionally, accessing the shutdown function via getattr ensures the cleanup path is tolerant of partially-constructed or uninitialized modules/attributes.
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | |
| except KeyboardInterrupt: | |
| run_mod._graceful_shutdown(run_mod._server) | |
| typer.echo("\nShutting down...") | |
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | |
| except KeyboardInterrupt: | |
| run_mod._graceful_shutdown(run_mod._server) | |
| typer.echo("\nShutting down...") | |
| finally: | |
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() |
References
- Ensure that cleanup or teardown paths tolerate partially-constructed objects or uninitialized attributes by accessing them using getattr instead of direct access.
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | ||
| except KeyboardInterrupt: | ||
| run_mod._graceful_shutdown(run_mod._server) | ||
| typer.echo("\nShutting down...") | ||
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() |
There was a problem hiding this comment.
Similar to another part of this file, the call to _wait_for_server_shutdown is duplicated for the normal shutdown path and the KeyboardInterrupt handler. Using a finally block here would avoid repetition and ensure the shutdown logic is always called, improving code clarity and maintainability. Additionally, accessing the shutdown function via getattr ensures the cleanup path is tolerant of partially-constructed or uninitialized modules/attributes.
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | |
| except KeyboardInterrupt: | |
| run_mod._graceful_shutdown(run_mod._server) | |
| typer.echo("\nShutting down...") | |
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() | |
| except KeyboardInterrupt: | |
| run_mod._graceful_shutdown(run_mod._server) | |
| typer.echo("\nShutting down...") | |
| finally: | |
| getattr(run_mod, "_wait_for_server_shutdown", lambda: None)() |
References
- Ensure that cleanup or teardown paths tolerate partially-constructed objects or uninitialized attributes by accessing them using getattr instead of direct access.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
Applied the Left |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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
Summary
On
Ctrl+C, Studio's shutdown logs and the shell prompt interleave, and the auto-start prompt can launch even when declined. This consolidates the two halves of the fix. It supersedes #6564 (installer) and #6565 (server thread) and folds in the run.py-side work from #6565 by @Imagineer99 with two refinements.There are two independent causes, on Linux / macOS / WSL:
curl | sh) dies onSIGINTbefore the studio child finishes, so the outer shell prompt prints in the middle of the shutdown logs.Both are needed for a clean shutdown.
1. Installer (
install.sh)The auto-start prompt also had a second bug: declining still launched. The read fallbacks defaulted to yes (
read ... || _reply="y"and the no-tty branch), so any answer that was not a cleanly deliveredy/nline auto-started a blocking foreground server (closed/EOF tty, or antyped early and consumed by the earlier package-accept prompt).n; a real Enter still counts as yes via${_reply:-y}.trap '' INTbefore launching Studio so the installer shell waits for Studio's own graceful shutdown instead of dying first and racing the prompt over its logs. The child still receivesSIGINTfrom the terminal.2. Server thread (
studio/backend/run.py,unsloth_cli/commands/studio.py)From #6565 by @Imagineer99: retain the uvicorn thread (
_server_thread) and add_wait_for_server_shutdown(), which joins it and flushes stdout/stderr. Terminal entrypoints call it after requesting shutdown (run.py's main shutdown path and the CLI shutdown/KeyboardInterrupt/BaseExceptionpaths) so the prompt cannot return while the thread still owns the streams.Two refinements on top of #6565:
_SERVER_SHUTDOWN_JOIN_TIMEOUT, matching the per-subprocess timeouts in_graceful_shutdown). The original joined withtimeout=Noneat every call site, which could hang the terminal forever if uvicorn shutdown stalled and left the existingis_alive()warning branch unreachable.SIG_DFLforSIGINT/SIGTERMat the start of the signal handler so a secondCtrl+Cforce-quits, and drop the redundant in-handler wait (the post-loop wait already covers the signal path). This also keeps the wait out of the signal handler.Validation
bash -n,dash -n,shellcheckclean oninstall.sh.python -m py_compileandast.parseclean onrun.py,studio.py, the test.pytest tests/test_studio_shutdown_thread_wait.pypasses (6 source-level AST tests, including new ones for the bounded join and theSIG_DFLrestore).n/N/EOF) no longer launches; Enter still launches; the prompt prints after all shutdown logs.INFO: Shutting downprints before the prompt (without it, the line is lost). A stalled uvicorn thread exits via the 5s bound (no infinite hang); a secondCtrl+Cforce-quits in ~0.2s.Notes
install.ps1gates the prompt onUserInteractive -and (-not [Console]::IsInputRedirected)and runs Studio in process via& $UnslothExe, so neither issue applies on Windows. No change there.