Studio: Stop every running Unsloth server, not just the last one recorded - #7577
Conversation
for more information, see https://pre-commit.ci
![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: e5aec0aa05
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 306915c031
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e47426a2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b2861df99
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fbd60fa5f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e897c025c9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b53bc251
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfcb99196e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 279afd1024
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7db3ee073c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdd0cf3835
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07f2f468a1
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
… server Follow-up on the per-port PID files. Each item below is a case where the new code either lost a server the old code could still stop, or stopped something that was not ours. All were reproduced against real Studio servers. studio/backend/run.py - Write the per-port record and the legacy studio.pid independently. They shared one try, so a studio root that could not take a new directory entry left the server recorded nowhere at all and unstoppable from the CLI; the old code still recorded it in studio.pid, which is an overwrite of an existing path and can still succeed. _remove_pid_file now also checks studio.pid when the per-port write failed. - Write the record through a temp file and os.replace. `stop` reads these concurrently and treats a truncated read as a corrupt record. - A failed Windows tasklist probe now means "alive", matching the CLI. Treating it as dead pruned a live server's record and let the next launch fall back past it, which is the orphan this work exists to fix. - Guard the unlink in _own_studio_on_port. Pruning is a courtesy and must not abort startup. - Extract _resolve_port so the requested-port abort is reachable from a test. Deleting that abort previously left the whole suite green. - Keep the plain fallback for api-only callers. The desktop app hardcodes 8888 and documents its reliance on the 8888-8908 range, and it reports a non-zero backend exit to the user as "Server stopped unexpectedly". It reads the bound port back from TAURI_PORT, as `studio run` does from app.state.server_port, so a fallback there is harmless and both servers are still recorded and stoppable. The interactive path prints the requested port, so it still aborts. - isdigit() is not enough to gate int(): a superscript two passes it and the ValueError escaped into every caller of _read_pid_record. unsloth_cli/commands/studio.py - An untimed record no longer cancels a timed one for the same PID. Every current server writes both a timed per-port record and an untimed studio.pid, so the start-time check was inert exactly where it mattered, and after a crash plus a PID reuse `stop` sent SIGTERM to whatever unrelated process had inherited the PID. - Distinguish an unreadable record from an invalid one. A root-owned record, or one caught mid-write, still belongs to a live server, and deleting it stranded that server. - Route every PID-file removal through _unlink_quietly. One undeletable record raised PermissionError and left the remaining live servers running. - Same isdigit()/int() guard as the backend. Tests - The requested-port abort, the recorded bind address, and the api-only fallback are now covered; all three previously survived deletion. - tests/studio/test_studio_pid_file_contract.py pins run.py's filename scheme to the CLI's glob and keeps studio.pid parseable by an older CLI. It lives under tests/studio because unsloth_cli/tests is not run by any workflow. - test_cli_studio_stop_windows.py now checks _signal_stop as well as stop. The kill moved into _signal_stop, so the os.kill(pid, 0) guard passed vacuously.
# Conflicts: # studio/backend/run.py
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: 52d671f3a2
ℹ️ 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".
| # kill(0) signals our whole process group; kill(1) is init. Never either. | ||
| if pid < 2: | ||
| return None |
There was a problem hiding this comment.
In containerized launches where Studio is the container entrypoint, the server can legitimately run as PID 1 and write studio-<port>-1.pid; this parser now treats that valid record as corrupt, so unsloth studio stop unlinks/ignores it instead of signalling the server, and the mirrored backend parser also misses it for duplicate-start detection. Only PID 0 has the process-group signalling semantics this guard is avoiding, so PID 1 should be verified like any other recorded Studio PID rather than rejected outright.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. Three reasons, in order of weight.
A PID is namespace-scoped and the record does not say which namespace it came from, so 1 is the value most likely to be cross-namespace garbage: with a bind-mounted or shared studio home, a record written as studio-8888-1.pid inside a container would make the host's unsloth studio stop signal host PID 1. That is the failure mode the guard exists for.
The verification that would have to carry the safety does not hold. _pid_is_studio_server(1, [None]) returns True today: an untimed record is trusted unconditionally, and the legacy studio.pid is always untimed. So the pid < 2 check is the only thing between a 1 in a PID file and os.kill(1, SIGTERM), and Studio commonly runs as root on cloud GPU boxes, so EPERM is not a backstop either.
The scenario also is not one we ship: there is no Dockerfile, compose file or container CI in the tree, and the unsloth/unsloth image in the README is a Jupyter/SSH dev container where Studio is a child of the session, never PID 1. Even as an entrypoint it would stop correctly through docker stop, since uvicorn installs its own SIGTERM handler; reaching unsloth studio stop would take a deliberate docker exec.
Rejecting PID 0 (your earlier comment on this line) stays, and is unaffected.
| # app.state.server_port for `studio run`), so a fallback there is harmless and | ||
| # is what the desktop app's 8888-8908 range expects. The interactive path | ||
| # prints the requested port, so falling back is what strands a server. | ||
| port = _resolve_port(host, port, avoid_own_studio = not api_only) |
There was a problem hiding this comment.
studio run on the fallback path
When unsloth studio run is invoked without --api-only and another recorded Studio server already holds the requested/default port, this passes avoid_own_studio=True and _resolve_port() exits instead of falling back. That path is not the plain interactive banner path: the CLI stores app = run_server(...) and then uses actual_port = getattr(app.state, "server_port", port) or port, so it can follow a fallback safely; this regresses multi-model experiments that previously started on 8889/8890 and printed the correct URL. Please keep the own-server abort only for callers that cannot read back the port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct that studio run is not the bare-banner path: it reads app.state.server_port back at studio.py:2245 and uses it for the health wait, the model load and the printed URL, so gating on api_only broke starting a second model while the first was up. Fixed in c854b2d, though not by extending the exemption list. run_server now takes an explicit abort_if_own_studio (defaulting to the previous not api_only, so the exec'd run.py path is unchanged) and studio run opts out.
The exemption also reopened the orphan from the other side, which is the part worth flagging: _write_pid_file overwrote studio.pid unconditionally, and a pre-upgrade server is recorded there and nowhere else, so any exempt launch falling back past one erased its only record. That file is now taken over only when it is free, already ours, or held by a dead PID, which makes a fallback non-destructive for every caller rather than only for the ones we remember to exempt.
…er take studio.pid from a live server Two problems with keying the own-server abort on api_only. `unsloth studio run` is not the bare-banner path: it stores `app = run_server(...)` and reads `app.state.server_port` back, then uses it for the health wait, the model load and the printed base URL. Gating on api_only aborted it, so starting a second model while the first was up stopped working, where before it landed on the next port and printed the right URL. Replace the proxy with an explicit abort_if_own_studio, defaulting to the old api_only behaviour so the exec'd `run.py` path is unchanged, and have `studio run` opt out. The api_only exemption also reopened the orphan from the other side. _write_pid_file overwrote studio.pid unconditionally, and a pre-upgrade server is recorded there and nowhere else, so an exempt launch falling back past one erased its only record. Take the file over only when it is free, already ours, or held by a dead PID. Also resync _pid_is_studio_backend with the CLI copy: an untimed record next to a timed one carried no information but cancelled the start-time check, which is what let a reused PID be treated as ours. Tests: 51 backend, 26 CLI, 9 under tests/studio. Real Studio servers still abort the bare same-port relaunch, still fall back past a foreign listener, and one `unsloth studio stop` still stops every server in all five scenarios.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c854b2dc80
ℹ️ 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".
| prior = _read_pid_record(_PID_FILE) if _PID_FILE.is_file() else None | ||
| if prior is None or prior[0] == os.getpid() or not _pid_alive(prior[0]): | ||
| _PID_FILE.write_text(str(os.getpid()), encoding = "utf-8") |
There was a problem hiding this comment.
When two current backends share STUDIO_HOME, this guard lets only the first one own studio.pid. If that server later shuts down normally, _remove_pid_file() deletes studio.pid while the second server remains live, and nothing promotes the second server's per-port record; an older unsloth CLI, which reads only studio.pid, then reports no running server and cannot stop the remaining instance. The fresh evidence relative to the earlier legacy-write concern is that this revision now preserves a live owner but implements no ownership handoff; transfer the legacy pointer to another verified live per-port record when its owner exits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 287eb0c. _remove_pid_file now hands studio.pid to a live sibling instead of deleting it: _legacy_heir() scans the per-port records, skips our own and any PID that fails the liveness plus start-time check, and writes the survivor's PID into studio.pid. Covered by test_the_legacy_pointer_moves_to_a_live_sibling and test_the_legacy_pointer_is_not_handed_to_a_dead_sibling.
| except (OSError, UnicodeDecodeError) as e: | ||
| # Unreadable is not the same as invalid. A root-owned record, or one | ||
| # caught mid-write, still belongs to a live server, and deleting it | ||
| # strands that server -- the bug this command exists to fix. | ||
| typer.echo(f"Cannot read PID file {path.name}: {e}", err = True) | ||
| continue |
There was a problem hiding this comment.
When the only live server's PID file is unreadable—for example, the server was started with sudo under the same STUDIO_HOME—this branch prints an error but omits the record from entries. stop() then takes the empty-list path, exits successfully, and says no server is running even though the server and its preserved PID file remain; track unreadable records as failures so the command does not report a successful stop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 287eb0c. _pid_file_entries now collects unreadable paths into an out-parameter, and stop reports them and exits 1 on every path instead of printing "No running Unsloth server found". Covered by test_stop_does_not_claim_success_when_the_only_record_is_unreadable and test_stop_reports_failure_when_one_record_is_unreadable_but_another_stops. The record itself is still preserved, since deleting it is what strands the server.
… records Two follow-ups from review of the previous commit. Only one backend owns studio.pid at a time. When that server exited it deleted the file, so an older CLI, which reads nothing else, could no longer stop a sibling that was still serving. _remove_pid_file now hands the pointer to a live sibling instead of dropping it. _pid_file_entries skipped records it could not read, for instance one written by a server started under sudo. When that was the only record, stop printed "No running Unsloth server found" and exited 0 while the server kept serving. Unreadable records are now reported and make stop exit 1, so a partial stop is never mistaken for a complete one.
for more information, see https://pre-commit.ci
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
3 participants
The port fallback quietly starts a second server when the requested port is taken, but there was only one
studio.pidso the second launch overwrote the first entry.stopthen killed the newer server, printed "Unsloth server stopped.", and deleted the PID file, leaving the older one serving forever with no way to stop it from the CLI:Writes one PID file per port (
studio-<port>.pid), sostopfinds and stops every server and reports only what actually exited. The legacystudio.pidis still read, so a server from an older build can still be stopped. Also refuses to fall back past our own server_get_pid_on_portalready.