Explain the Windows unsloth.exe lock instead of reporting it as permissions - #7740
Explain the Windows unsloth.exe lock instead of reporting it as permissions#7740danielhanchen wants to merge 25 commits into
Conversation
|
@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: e9ddc7c002
ℹ️ 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".
| if exe_lock_err is not None: | ||
| # setup.ps1 reports this as a permissions problem, which it is not. | ||
| _explain_self_exe_locked(exe_lock_err) |
There was a problem hiding this comment.
When the CLI is launched from the locked Scripts\unsloth.exe, every exception from _run_setup_script is now attributed to that lock, including failures that occur before pip touches the executable (for example, a missing setup script, a dependency download failure, or a user interrupt). _explain_self_exe_locked then replaces the original exception with typer.Exit(1) and directs the user to retry through the launcher even though that cannot fix the actual failure. Only convert the failure when there is evidence that setup failed while replacing this executable; otherwise re-raise the original exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this was the worse half of the bug. Setup output is streamed rather than captured, so nothing in this process can tell whether pip ever reached unsloth.exe. A missing setup script or a dead download would have been reported as a lock, and the real error was discarded along with its exit code.
It is now a note printed next to the original exception, which is re-raised in every case, and the wording is conditional: it says what to check ("if the failure above mentions WinError 32, a sharing violation or file permissions") rather than asserting the cause. test_the_note_is_conditional_not_a_diagnosis pins that.
| # turns this into the explanation, because pip hits the same lock and reports | ||
| # it as a permissions problem, which is what sent people to the installer. | ||
| print(f"[update] could not rename {exe.name} -> {stale.name}: {e}") | ||
| return e |
There was a problem hiding this comment.
For the explicitly supported case where the locked executable cannot be renamed but setup is a no-op and succeeds, this branch still prints [update] could not rename ... before returning the error. Consequently successful updates continue to emit the same alarming warning that this change is intended to defer until a real setup failure. Return the error without printing here and let the failure path render the explanation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. An update where the package is already current never touches that file and succeeds from here, so the common case was a warning printed for a run that then worked, which is the noise this PR is supposed to remove.
The release path is silent now and only returns the error; the failure path renders it if it turns out to matter. test_a_locked_exe_is_reported_but_does_not_abort asserts both halves: the error reaches the caller, and stdout stays empty.
| typer.echo("Run it through the launcher on PATH instead:", err = True) | ||
| typer.echo("", err = True) | ||
| typer.echo(" unsloth studio update", err = True) |
There was a problem hiding this comment.
The existence of STUDIO_HOME\bin\unsloth.exe does not guarantee that an unqualified unsloth command resolves to it. If the venv's Scripts directory precedes the shim directory on PATH—which is precisely how a user can reach this lock—the suggested command launches the same locked executable again and repeats the failure. Print a PowerShell command that invokes shim directly, or verify PATH resolution before recommending the unqualified command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes. Reaching this lock at all is evidence the venv Scripts directory may come first on PATH, in which case a bare unsloth resolves straight back to the copy that is locked, and the advice loops.
It prints the launcher by full path now: & '<STUDIO_HOME>\bin\unsloth.exe' studio update on Windows, shell-quoted elsewhere.
| err = True, | ||
| ) | ||
| typer.echo("", err = True) | ||
| typer.echo(" irm https://unsloth.ai/install.ps1 | iex", err = True) |
There was a problem hiding this comment.
When the missing-launcher branch is reached for a custom UNSLOTH_STUDIO_HOME, this pasted command runs in a new shell without the environment exported by _ensure_studio_env_exported, so it creates or repairs the default ~\.unsloth\studio installation and leaves the actual custom installation without a launcher. It also drops the recorded no-torch mode, causing a GGUF-only installation to pull in the PyTorch stack. Format this recovery command with the custom root and recorded install mode, as _fail_if_install_damaged already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the copy in the note had already drifted from the one in _fail_if_install_damaged: it dropped both the custom UNSLOTH_STUDIO_HOME and the recorded no-torch mode. A pasted command from a custom root would have built a fresh default install beside the real one and left it untouched, and on a GGUF-only install it would have pulled the whole PyTorch stack back in.
Both call one _reinstall_command() now. test_without_a_launcher_the_note_carries_the_install_config covers the custom-root case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88b0fefc79
ℹ️ 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".
| # the venv Scripts dir may come first on PATH, in which case an unqualified | ||
| # name resolves straight back to the locked copy. | ||
| if platform.system() == "Windows": | ||
| typer.echo(f" & '{str(shim)}' studio update", err = True) |
There was a problem hiding this comment.
When the failed invocation used studio update --local, this recovery command retries a different operation: it omits both --local and the STUDIO_LOCAL_REPO value established above. The launcher therefore updates from PyPI instead of installing the requested checkout, and may report success while the local changes that triggered the executable replacement remain uninstalled. Build the suggested command from the original update options and carry the resolved local repository into the new shell.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is the worse of the two failure modes here because it fails quietly. The retry runs in a new shell that inherits neither the flag nor the STUDIO_LOCAL_REPO this function resolved, so a --local update would come back as a PyPI update, report success, and leave the checkout that prompted the replacement uninstalled.
repo_root is now passed in from update -- it is already the resolved path, and it is None exactly when the update was not local -- and both the assignment and the flag are rendered only in that case. Covered both ways by test_a_local_update_is_retried_as_a_local_update and test_a_pypi_update_is_not_retried_as_a_local_one.
| # the venv Scripts dir may come first on PATH, in which case an unqualified | ||
| # name resolves straight back to the locked copy. | ||
| if platform.system() == "Windows": | ||
| typer.echo(f" & '{str(shim)}' studio update", err = True) |
There was a problem hiding this comment.
For a supported custom Studio root containing an apostrophe, such as C:\Users\O'Brien\Studio, embedding shim directly in a single-quoted PowerShell string produces an invalid command, so the prescribed recovery cannot be pasted successfully. Other PowerShell paths in this module already double embedded apostrophes; apply the same escaping before rendering this launcher command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes. There is now one _ps_single_quote doing the doubling, and _reinstall_command calls it instead of carrying its own copy of the rule, since that was where the escaping already existed and the drift started.
test_an_apostrophe_in_the_path_does_not_break_the_powershell_line runs it under C:\Users\O'Brien\Studio and checks both that the apostrophe is doubled and that the quotes in the rendered line pair up.
| ls -la "$SCRIPTS"/*.deleteme | ||
| exit 1 | ||
| fi | ||
| "$PY" -c "import unsloth" || { echo "::error::unsloth unimportable after replacement"; exit 1; } |
There was a problem hiding this comment.
In the inspected studio-windows-update-smoke.yml job, the installation is deliberately created with --no-torch, and the preceding assertion requires import torch to fail. On Windows, import unsloth follows the GPU path in unsloth/__init__.py, where _gpu_init.py raises when PyTorch is absent, so this newly added check fails on every healthy GGUF-only installation and prevents the workflow from reaching the later boot test. Validate the distribution or CLI without importing the torch-dependent top-level package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it did fail the job on exactly this. Two separate mistakes in one line: the install is --no-torch and the step above it asserts torch is absent, so import unsloth cannot succeed on a healthy install; and running from the repo root, the name resolved to the checked-out source tree at D:\a\unsloth\unsloth\unsloth\__init__.py rather than the installed copy, so it was not testing the replacement at all.
It now runs the replaced Scripts/unsloth.exe itself -- the file pip rewrote and the one the cleanup could have deleted, so existence alone is not enough -- and asserts studio desktop-capabilities --json reports studio_install_ok. That path needs neither torch nor the source tree.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1db4ce3608
ℹ️ 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".
| if shim.is_file(): | ||
| typer.echo("", err = True) | ||
| typer.echo("Re-run through the launcher, a separate entry to the same", err = True) | ||
| typer.echo("binary, which leaves this copy replaceable:", err = True) |
There was a problem hiding this comment.
When another running command was launched from Scripts\unsloth.exe, _release_self_exe_lock_windows() receives the same sharing violation even if this updater was already launched through the shim. Re-running through the launcher therefore leaves the other process's lock in place and repeats the failure; the OSError alone does not establish that the current updater owns the lock. Advise closing processes using the venv executable, or recommend the shim only when the current invocation is known to be the lock holder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fair, and the distinction matters: the OSError says the entry is locked, not who holds it. A second process launched from the same copy holds it just as well, and re-running through the shim then changes nothing, because it is pip that has to replace the file.
I did not make the shim advice conditional, because there is no reliable way from here to tell whether this process is the only holder, and suppressing the retry on that uncertainty would withhold the fix from the common case to hedge the rarer one. Instead the note keeps the retry and adds the other possibility after it, naming the copy to close. Worth noting the Studio backend itself runs from that same entry, so a running Studio is the most likely second holder.
| if repo_root is not None: | ||
| prefix = f"$env:STUDIO_LOCAL_REPO = {_ps_single_quote(str(repo_root))}; " | ||
| suffix = " --local" if repo_root is not None else "" | ||
| typer.echo(f" {prefix}& {_ps_single_quote(str(shim))} studio update{suffix}", err = True) |
There was a problem hiding this comment.
When studio update --package X hits this lock, the displayed retry omits --package X, so the new process resets STUDIO_PACKAGE_NAME to unsloth and performs a different update. This can also record the wrong package in the install manifest, making subsequent verification follow unsloth rather than the package the user was testing. Pass the original package option through to the recovery command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. update exports it as STUDIO_PACKAGE_NAME, so the retry would have reset to unsloth, updated a different package, and then recorded that one in the manifest for later verification to follow.
Carried through now, shell-quoted, and omitted when it is the default so the common case stays short. Both directions covered by test_a_custom_package_survives_into_the_retry and test_the_default_package_is_not_spelled_out.
| os.replace(exe, stale) | ||
| except OSError as e: | ||
| # Not fatal; setup.ps1 retries from a sibling process. | ||
| print(f"[update] could not rename {exe.name} -> {stale.name}: {e}") | ||
| # Windows locks the directory entry an image was launched from, not the file |
There was a problem hiding this comment.
On Windows, the actual Tauri update path still obtains unsloth_studio\Scripts\unsloth.exe in studio/src-tauri/src/process.rs:362-375 and executes it from studio/src-tauri/src/update.rs:253, so this rename always hits the updater's own image lock. Whenever an update must replace the package, the code merely returns the error and lets pip encounter the same lock, meaning in-app update and repair remain unable to install that release; the new smoke only passes because it explicitly launches through bin\unsloth.exe. Make the desktop updater use that shim or re-exec through it before setup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against the Rust and you are right, so this PR now changes it.
update.rs resolves with process::find_unsloth_binary(), which returns unsloth_studio\Scripts\unsloth.exe on Windows, and spawns studio update from it. Both in-app update and repair go through the same body, and nothing under studio/src-tauri resolved the shim at all -- find_unsloth_binary_in_studio_dir has a base.join("bin") arm, but that is the Unix venv unsloth_studio/bin/unsloth, one level below the <studio>/bin shim, which is easy to misread as already handling it. git log -L on that function shows it arrived unchanged with the original Tauri commit and no comment justifying the venv copy, so it was not a deliberate choice.
So the desktop app could not install any release that replaces the unsloth package, and you are also right that the smoke passed only because it drives bin\unsloth.exe explicitly.
New find_unsloth_updater() prefers the shim and falls back to the venv copy, so an install whose shim was never created is no worse off than before. It checks non-empty rather than existence, because install.ps1 downgrades a shim it could not write to a warning and carries on, and the Copy-Item fallback can leave a truncated file. The long-lived backend at process.rs keeps find_unsloth_binary on purpose: launching that from the shim would hold the shim entry open for as long as Studio runs, which is the same bug aimed at a different file.
The workflow now also drives a package-replacing update from Scripts\unsloth.exe. It is not asserted to succeed, since that is the failure this explains rather than removes; it asserts either outcome leaves a working CLI, which exercises the restore path nothing else covered, and that a failure names its cause.
| "while this process is running from it.", | ||
| err = True, | ||
| ) | ||
| if shim.is_file(): |
There was a problem hiding this comment.
If bin\unsloth.exe exists but is zero-byte or otherwise unreadable, is_file() still selects the launcher retry branch. This is a realistic recovery case because the user may have invoked the venv copy precisely because the damaged shim cannot start; after the update fails on the self-lock, the only printed recovery command then points back to that unusable file and the reinstall fallback is hidden. Verify that the launcher is at least a nonempty readable executable before recommending it, otherwise print the reinstall command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and the realistic-case argument is the right one: a shim that does not start is a reason to have run the venv copy directly in the first place, so the note pointed back at the broken file and hid the reinstall fallback exactly when it was needed.
_is_usable_launcher now requires a non-empty readable file. This matters beyond the note too: install.ps1 downgrades a shim it could not create to a warning and continues, so an install can legitimately be missing a usable one.
1db4ce3 to
50174bd
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
unsloth/unsloth_cli/commands/studio.py
Lines 3304 to 3306 in 50174bd
Keep the fallback executable when restoration fails
If setup succeeds without creating a replacement executable and _restore_self_exe_lock_windows() cannot move .deleteme back—for example because another process has created or locked the destination—the cleanup continues and unconditionally attempts to delete .deleteme. The source entry can be deletable even when replacement of the locked destination is not, leaving no working recovery copy; only unlink the stale file after confirming that unsloth.exe was restored or that a usable replacement exists.
ℹ️ 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".
| suffix = local_flag | ||
| if custom_package: | ||
| suffix += f" --package {_ps_single_quote(package)}" | ||
| typer.echo(f" {prefix}& {_ps_single_quote(str(shim))} studio update{suffix}", err = True) |
There was a problem hiding this comment.
When the failed invocation was studio update --no-verify, this recovery line retries without that option and silently re-enables the post-update damage scan. Users who explicitly disabled verification because of known unmanaged or damaged files can therefore complete setup through the shim but still receive a failing retry, even though the original operation would have succeeded; pass the original verification setting into _note_self_exe_locked and append --no-verify when needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the same family as --local and --package. Turning the scan off is a deliberate choice made because the install has files it reports and cannot repair, so a retry that silently turns it back on fails after completing the update it was meant to complete. verify is threaded through and rendered only when it is off.
| match std::fs::metadata(&shim) { | ||
| Ok(meta) if meta.is_file() && meta.len() > 0 => Some(shim), | ||
| _ => None, |
There was a problem hiding this comment.
The newly added Rust selector treats every nonempty regular file as a usable launcher. A partially copied/corrupt Windows shim, or a non-executable Unix shim, is therefore preferred over a working venv binary and makes desktop update and repair fail during spawn() instead of using the fallback. This is fresh evidence beyond the earlier CLI recommendation issue: the desktop path has its own weaker validation and should reject launchers that cannot be executed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and the failure mode is worse than the bug the ordering fixes: preferring an unrunnable shim turns a working venv binary into a spawn failure, so the fallback never gets a chance.
The execute bit on Unix, and on Windows the MZ image header, since there is no execute bit there and the loader decides -- that also rejects the half-written file a Copy-Item fallback can leave. The unit test now covers both: a 0o644 shim is skipped and a 0o755 one is chosen, and on Windows a non-MZ file is skipped.
Worth noting this is stricter than the Python side deliberately. _is_usable_launcher only prints a suggestion, so a false negative there just costs a better hint; here a false positive breaks the update outright.
|
One consequence of routing the desktop update through the shim, recorded here rather than fixed, because it cannot be fixed from inside the process this PR touches.
In practice it keeps working: the console-script stub only embeds the interpreter path and the The fix does not belong in this process. Re-linking the shim means deleting the file the updater is currently running from, which is the same sharing violation this PR is about, just pointed the other way. It has to happen after the child exits, so the natural owner is the Tauri side in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5790e785bf
ℹ️ 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".
| """ | ||
| if platform.system() != "Windows": | ||
| return | ||
| _restore_self_exe_lock_windows() |
There was a problem hiding this comment.
When setup succeeds without producing a usable unsloth.exe but _restore_self_exe_lock_windows() cannot move the backup back—for example, because antivirus or another process temporarily locks the missing/zero-byte destination—the helper catches the OSError and returns, after which this cleanup immediately unlinks the still-valid .deleteme file. That leaves the user with no working CLI even though the backup survived; only remove the stale file after confirming restoration succeeded or that a usable replacement exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is a real bug in the PR rather than a hardening note, so thank you for it.
The restore reports rather than raises, and it can fail for entirely ordinary transient reasons. Setup succeeding does not imply pip wrote a usable unsloth.exe either. So the one outcome with no way back was reachable: the update ends with no CLI at all while a working copy was sitting right there under .deleteme.
It now removes the backup only once the destination is a non-empty file. A surviving .deleteme is harmless by comparison, since the next update renames over it anyway, so the asymmetry is the right way round. Two tests: one where os.replace raises, one where the destination is still zero-byte.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e4a69f66d
ℹ️ 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".
| SCRIPTS="$HOME/.unsloth/studio/unsloth_studio/Scripts" | ||
| SHIM="$HOME/.unsloth/studio/bin/unsloth.exe" | ||
| test -f "$SHIM" || { echo "::error::no launcher at $SHIM"; exit 1; } | ||
| "$PY" -m pip install --no-deps --force-reinstall --no-build-isolation . 2>&1 | tail -5 |
There was a problem hiding this comment.
When this Windows smoke runs in the freshly-created Studio venv, this forced reinstall is executed with --no-build-isolation, so pip expects the repo's build backend to already be installed in that venv. The project declares setuptools.build_meta plus setuptools-scm as build-system requirements in pyproject.toml, but those are not runtime deps of the --no-torch install, so this command can fail before it ever recreates Scripts/unsloth.exe and the new lock coverage never runs; the identical reinstall in the next step has the same problem. Use normal build isolation here or explicitly install the build requirements first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, though the mechanism is not quite the one described, and the difference matters for the fix.
setuptools-scm is declared in build-system.requires but never imported: the version comes from [tool.setuptools.dynamic] version = {attr = "unsloth.models._utils.__version__"} (pyproject.toml:44-45), and there is no [tool.setuptools_scm] section. pip also does not verify build-system.requires under --no-build-isolation; it just calls the backend and fails on ImportError. So the only real requirement is that setuptools itself is importable in the Studio venv. I reproduced the command in a venv pinned to what the real Studio venv actually has (pip 26.2, setuptools 78.1.0, no setuptools-scm) and it built and installed cleanly.
The concern still stands in its stronger form: setuptools is in that venv only because something pulled it in, not because anything installs it deliberately, and uv venv does not provide it. That is exactly the kind of thing a dependency change silently removes, and the failure mode would be this step erroring before it recreates Scripts\unsloth.exe, so the lock coverage below never runs and nothing reports why. Not worth resting on, for a couple of seconds of isolated build.
Both reinstalls now use normal build isolation. Verified the isolated build from this checkout too.
Fixed in d913ad1.
| # Through the launcher, which is a second link to the same binary. Windows | ||
| # locks the entry an image was launched from, not the file, so this leaves | ||
| # the venv copy replaceable. | ||
| "$SHIM" studio update --local 2>&1 | tee logs/update_replace.log |
There was a problem hiding this comment.
If the forced reinstall above succeeds, it replaces the editable install with a normal site-packages copy, so studio update --local can no longer derive the checkout from __file__; the update() guard falls back to the installed package location and exits with “needs an Unsloth checkout” before this launcher-path test reaches the lock-handling path. The later venv-copy step already exports STUDIO_LOCAL_REPO for this reason, and this shim step needs the same export before invoking --local.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I had the asymmetry backwards when I wrote the two steps: I reasoned about it for the venv step and not for this one, even though the same reinstall precedes both.
Confirmed the premise rather than assuming it. install.ps1 --local overlays the checkout as an editable install (studio/install_python_stack.py:3065-3074), which is why the two earlier no-op updates resolve the checkout from __file__ and pass. The forced reinstall in this step replaces that with a site-packages copy, so Path(__file__).resolve().parents[2] becomes site-packages, pyproject.toml is not there, and update() exits 2 at unsloth_cli/commands/studio.py:3033-3056.
One correction to the diagnosis: with defaults.run.shell: bash the step runs under -eo pipefail, so that exit 2 would fail the step loudly rather than let the later assertions pass on nothing. Either way it never reaches the lock, which is the whole point of the step.
Exported STUDIO_LOCAL_REPO via cygpath -w before the shim update, with the same checkout sanity test, and added the "the update never started" grep guard that the venv step already had, so a future regression to the argument guard reports itself instead of looking like a pass.
Worth noting these two steps have never actually executed: the job has been failing earlier, at the llama.cpp prebuilt assertion, since before they were added. So this was caught by reading rather than by CI.
Fixed in d913ad1.
There was a problem hiding this comment.
Correction to my reply above, and it makes this a worse bug than either of us described.
I wrote that these two steps have never executed. That is wrong for this one: it has run four times (two failures, then two passes at 1db4ce360 and 5d7829f14). Only the venv step is unexercised. I should have checked the step-level history before saying that rather than inferring it from the job-level failure.
Checking it turned up the real problem. In run 30751891659 this step passed, and the update inside it printed:
deps overlaying local repo (editable):
C:\Users\runneradmin\.unsloth\studio\unsloth_studio\Lib\site-packages
Against D:\a\unsloth\unsloth in the two no-op updates earlier in the same job. So repo_root fell back to Path(__file__).resolve().parents[2], landed on site-packages, and --local overlaid the venv onto itself instead of the checkout. It did not exit at the argument guard, so your predicted symptom is not what happens on Windows: the guard passed, and the step went green while installing the wrong thing.
That also means the guard I first added, a grep for "needs an Unsloth checkout to install from", was doubly useless: dead under -eo pipefail as you would expect, and grepping for a message that provably does not appear here.
Replaced with a positive assertion that the path the update overlaid is the checkout, case- and separator-folded so a resolve() that canonicalises either does not fail the step over a spelling. Checked it against three fixtures: the real logged site-packages line fails it, the checkout line passes, and an empty log fails.
04dff15ea.
There was a problem hiding this comment.
Both of these steps now run green on a real Windows runner, and the first one passes for the right reason rather than the wrong one.
Verified on staging (danielhanchen/unsloth-staging-2, branch off 04dff15ea) so as not to keep re-rolling the org queue. Two runs, one pull_request and one workflow_dispatch. All 18 steps green in both, including the venv-refusal step, which had only ever executed once before and failed.
The evidence that matters is inside the step, not the green tick. Where the org run printed
deps overlaying local repo (editable):
C:\Users\runneradmin\.unsloth\studio\unsloth_studio\Lib\site-packages
it now prints the checkout, on all three updates in the job. The new assertion is what would have caught the old behaviour, and it did not fire.
The venv step exercised the real thing rather than a stand-in: a genuine [WinError 32] on Scripts\unsloth.exe, the refusal with "Stopping before the install is changed. Nothing has been removed.", the note carrying $env:STUDIO_LOCAL_REPO and --local into the retry command, studio_install_ok: true afterwards, and the launcher route then completing the update that was refused.
One correction to what I said earlier about the llama.cpp prebuilt 429. The pull_request run cleared that step with HF_TOKEN empty, so it is a transient per-runner-IP window, not something every tokenless PR run hits. The three *-update-smoke workflows are still the only Studio workflows without the hf-download-with-retry.sh prewarm the other nine use, but that is a pre-existing gap and not this PR's to close.
Staging PR closed, not merged.
|
Staging CI reproduced the lock end to end on a real Windows runner, and it showed this issue is worse than the PR assumed. The failure is not a failed update, it is a destroyed install: pip uninstalls before it installs. So it removed That is the state a user is in today after running The rename failing is the signal, and at that moment nothing has been removed yet. The update now re-runs itself through the launcher, which is a separate directory entry to the same binary and can therefore move this copy aside, and exits with the child status. One hop only, guarded by an env var, or a launcher that resolved back to the same entry would recurse forever. It returns rather than raising whenever there is nothing better to do (no usable launcher, already re-executed, launcher will not start), so a broken hand-off cannot become a second failure on top of the one being avoided, and the note still gets printed. The note and the restore logic all stay: they are what covers the case where there is no usable launcher to hand over to. The smoke step now asserts the hand-off happened and the update succeeded, instead of tolerating either outcome. It also fails if the log does not show the hand-off, which catches the case where the rename stops failing and the step quietly stops covering the lock at all. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cce34d5e5
ℹ️ 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".
| typer.echo(f"Re-running through the launcher instead: {shim}", err = True) | ||
| typer.echo("", err = True) | ||
| try: | ||
| result = subprocess.run(argv, env = env) |
There was a problem hiding this comment.
In the venv-copy path where _release_self_exe_lock_windows() already failed, waiting for the launcher child keeps this original process alive from Scripts\unsloth.exe; that parent process continues to hold the entry the child’s pip must replace, so the child repeats the same sharing violation. This is fresh evidence beyond the earlier other-process warning: the code itself creates the still-running locker via subprocess.run, so spawn/exec the launcher and terminate this process before setup runs instead of waiting here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is the reasoning that killed the hand-off. subprocess.run keeps this process alive for the whole child run, and this process is the one holding the entry, so the child hit the identical WinError 32. I confirmed that on a real Windows runner before removing it.
The alternative you suggest, spawn and exit before setup runs, is what I looked at next and rejected: exiting first abandons the exit status and the streamed setup output, and the desktop updater reads both, so an update would report success no matter what happened.
So there is no hand-off any more. The venv-launched path now refuses at _refuse_update_that_would_break_the_install() before pip runs (unsloth_cli/commands/studio.py:3187), on the grounds that going on is not a failed update but a destroyed one, since pip uninstalls before it installs. The reasoning is recorded in that docstring so the hand-off does not get reinvented.
Already addressed as of 0aa12c4; the line this comment points at no longer exists.
|
Correcting my previous comment: the hand-off through the launcher does not work, and staging CI caught it on the next run. This process stays alive waiting for the child, and this process is the one holding the entry, so the child hit the identical The finding it was built on still stands, and it is the important part: continuing is not a failed update, it is a destroyed install. pip uninstalls before it installs, so it removes The update now refuses at that point instead. The rename failing is the evidence that going on would destroy the install, and nothing has been removed yet, so it stops and says so. Only when there is a usable launcher to refuse towards. With none there is no better path to send anyone down and stopping would leave them unable to update at all, so it goes on and takes its chances with the note. The trade, stated plainly: running the venv copy directly used to work for a no-op update and destroy the install for anything else. It now fails for both, with the one command that works printed alongside. Anyone whose PATH has the launcher, which The smoke step now asserts the install survives rather than that the update succeeds: no uninstall line in the log at all, the CLI still runs and reports |
…ssions Closes #7697. Windows locks the directory entry an image was launched from rather than the file behind it. Measured on windows-latest: with a process launched from Scripts\unsloth.exe that copy cannot be renamed, WinError 32, while the hardlinked bin\unsloth.exe can; launch through the launcher instead and it is the other way round. Quiescent, both rename fine. So _release_self_exe_lock_windows can never move the copy it is running out of, and its failure was a print. pip then reaches the same file, dies mid-uninstall, and reports a permissions problem, which is what sent people back to the full installer. The rename failure is now returned rather than printed and discarded. It stays non-fatal, because an update with no package change never touches the file and still works from there, and only turns into a message if setup actually fails, naming the launcher on PATH that avoids the lock, or the installer when no launcher exists. The existing Windows update job asserts the update is a no-op, so pip never rewrote unsloth.exe and none of this was reachable. It now also forces a replacement and drives it through the launcher, which is the supported path.
for more information, see https://pre-commit.ci
The update renames unsloth.exe aside so pip can replace it, and clears the .deleteme afterwards. But setup succeeding does not mean pip rewrote the exe: a dependency pass that finds the package already at the right version reinstalls nothing, and then the renamed copy is the only one there is. Clearing it leaves no CLI at all, which is what the new Windows job hit -- the update finished and then reported its own unsloth.exe missing. _restore_self_exe_lock_windows already knows how to decide this, restoring only when the exe is absent or zero-byte, so cleanup consults it before removing the orphan rather than deleting unconditionally. The restore is a no-op once pip has written a fresh binary. The job now also asserts a usable unsloth.exe and no surviving .deleteme after an update that had to replace the package.
for more information, see https://pre-commit.ci
Review on this PR raised four things and all four were right. The note replaced the setup failure with `Cannot update: ... is in use by this process` and exited 1. Setup output is streamed rather than captured, so nothing here can tell whether pip ever reached unsloth.exe: a missing setup script or a dead download would have been reported as a lock. It now prints alongside the original exception, which is re-raised either way, and the wording is conditional -- it says what to check rather than what happened. The rename failure printed on every run. An update with no package change never touches that file and succeeds from there, so the common case was a warning for a run that then worked. The release path is silent now and the failure path renders the error if it turns out to matter. The re-run line was a bare `unsloth studio update`. Reaching this lock at all is evidence the venv Scripts directory may come first on PATH, in which case that name resolves straight back to the locked copy. It names the launcher by full path. The reinstall command was duplicated between _fail_if_install_damaged and this note, and the copy in the note dropped both the custom UNSLOTH_STUDIO_HOME and the recorded no-torch mode -- so a pasted command would have built a fresh default install next to the real one, and pulled the whole PyTorch stack into a GGUF-only install. Both now call one _reinstall_command. Tests cover the release, the note and the cleanup: nine cases, each failing without the change it pins.
for more information, see https://pre-commit.ci
…rt from CI Three more from review, all correct. The retry command dropped `--local` and the resolved STUDIO_LOCAL_REPO. It runs in a new shell that inherits neither, so a local update would have been retried as a PyPI one: it reports success while the checkout that prompted the replacement stays uninstalled. Both are carried through now. A custom Studio root is user-chosen and can contain an apostrophe. Embedding it in a single-quoted PowerShell string ended the string early and made the prescribed recovery unpasteable. One _ps_single_quote doubles them, and _reinstall_command uses it too rather than keeping its own copy of the rule. The new smoke step ended with `import unsloth`, which failed the job. That install is --no-torch by design and the step above it asserts torch is absent, so the package refuses to import on a perfectly healthy install; and from the repo root the name resolved to the checked-out source tree rather than the installed copy, so it was not testing the replacement at all. It now runs the replaced Scripts/unsloth.exe itself and asserts desktop-capabilities reports studio_install_ok, which needs neither torch nor the source tree.
…ess case Three more, all correct. --package was dropped from the retry. update exports it as STUDIO_PACKAGE_NAME, so the retry would have reset to `unsloth` and updated a different package, then recorded that one in the manifest for later verification to follow. It is carried through, quoted, and omitted when it is the default. is_file() accepts a zero-byte or unreadable shim. That is not hypothetical here: a damaged launcher is a reason to have run the venv copy directly in the first place, and the note then pointed back at the file that already does not start while hiding the reinstall fallback. _is_usable_launcher requires non-empty and readable. The OSError says the entry is locked, not who holds it. A second process launched from the same copy holds it just as well, and then re-running through the shim changes nothing, because it is pip that has to replace the file. The note now says so and names the copy to close.
The reviewer is right and I checked it against the Rust myself. The Tauri update and repair both resolve the binary with process::find_unsloth_binary, which returns unsloth_studio\Scripts\unsloth.exe on Windows (process.rs), and spawn `studio update` from it (update.rs). Nothing in studio/src-tauri resolves the shim at all. So the in-app update was launched from the exact entry pip has to replace, and any release that replaces the unsloth package could not install from the desktop app. Without this the PR explains that failure instead of preventing it, and the smoke only passed because it drives the shim explicitly. find_unsloth_updater prefers <studio>/bin/unsloth.exe and falls back to the venv copy, so an install whose shim was never created is no worse off than before. install.ps1 downgrades a shim it could not write to a warning and carries on, and the Copy-Item fallback can leave a truncated file, so it checks non-empty rather than existence. The long-lived backend keeps using find_unsloth_binary deliberately: launching it from the shim would hold that entry open for as long as Studio runs, which is the same bug aimed at the other file. The workflow now also drives a package-replacing update from the venv copy. It is not asserted to succeed -- that is the failure this explains rather than removes. It asserts that either outcome leaves a working CLI, which exercises _restore_self_exe_lock_windows on the failure path where nothing else did, and that a failure names its cause instead of surfacing as bare permissions.
It failed, and it was the step that was wrong, not the code. By the time it runs, the step before it has reinstalled unsloth non-editably, so the CLI is running out of site-packages and cannot derive the checkout from __file__. `--local` exited at the argument guard with "needs an Unsloth checkout to install from" and never got near the rename it exists to exercise. STUDIO_LOCAL_REPO is now set explicitly, which is what that guard tells users to do. Through cygpath: MSYS rewrites arguments that look like paths but not environment variables, so the native python would not have resolved /d/a/... . It also now fails if the update did not start, instead of quietly passing every later assertion without testing any of them. That is the same way the first cut of the neighbouring step passed for the wrong reason, so it is worth an explicit guard rather than trusting the ordering to stay put.
…shim runs Three more from review, all correct, and the first is a real bug in this PR. _cleanup_self_exe_lock_windows deleted the .deleteme unconditionally after calling the restore. But the restore reports rather than raises, and it can fail for ordinary transient reasons -- antivirus or another process holding the destination for a moment. Setup succeeding does not imply pip wrote a usable unsloth.exe either. So the one path with no way back was reachable: no CLI left at all while a working copy sat right there. It now removes the backup only once the destination is a non-empty file. A surviving .deleteme is harmless by comparison, since the next update's rename overwrites it. --no-verify was dropped from the retry, like --local and --package before it. Turning the scan off is deliberate -- the install has files it reports and cannot repair -- so a retry that turns it back on fails after the update it was meant to complete has already succeeded. The Rust selector accepted any non-empty regular file, so a non-executable Unix shim or a half-written Windows one would have been preferred over a working venv binary and turned the update into a spawn failure, which is worse than the bug the ordering fixes. It checks the execute bit on Unix, and on Windows the MZ image header, since there is no execute bit and the loader decides.
for more information, see https://pre-commit.ci
… install
Staging CI reproduced the lock end to end and showed the failure is worse than
this PR assumed. pip uninstalls before it installs, so a venv-launched update
removes unsloth_cli and only then hits the locked stub:
Uninstalling unsloth-2026.7.6:
ERROR: Could not install packages due to an OSError: [WinError 32] ...
scripts\unsloth.exe
leaving an unsloth.exe that starts and raises ModuleNotFoundError: No module
named 'unsloth_cli'. So this was not a failed update, it was a destroyed install,
and explaining it after the fact is not enough.
The rename failing is the signal, and at that point nothing has been removed yet.
The update now re-runs itself through the launcher, which is a separate directory
entry to the same binary and can therefore move this copy aside, and exits with
the child's status. One hop only, guarded by an env var, or a launcher that
resolved back to the same entry would recurse forever. It returns rather than
raising whenever there is nothing better to do -- no usable launcher, already
re-executed, the launcher will not start -- so a broken hand-off cannot become a
second failure on top of the one being avoided, and the caller still reaches the
note.
The smoke step now asserts the hand-off happens and the update succeeds, rather
than tolerating either outcome. It fails if the log does not show the hand-off,
which also catches the case where the rename stops failing and the step silently
stops covering the lock.
for more information, see https://pre-commit.ci
…ncher The hand-off in the previous commit does not work, and staging CI showed it. This process stays alive waiting for the child, and this process is the one holding the entry, so the child hits the identical lock and fails the same way. Nothing short of exiting first releases it, and exiting first means abandoning the exit status and the streamed output the desktop reads. So it refuses instead. The rename failing is the evidence that going on would destroy the install rather than merely fail, and at that point nothing has been removed, so it stops there and says so. Only when there is a usable launcher to refuse towards. With none there is no better path to send anyone down and stopping would leave them unable to update at all, so it goes on and takes its chances with the note. UNSLOTH_ALLOW_LOCKED_UPDATE overrides, because this does stop updates that would have succeeded: one with no package change never touches the file. That is the trade this makes, deliberately. Running the venv copy directly used to work for a no-op update and destroy the install for any other. It now fails for both, with the one command that works. Anyone whose PATH has the launcher -- which install.ps1 sets up -- never reaches it, and the desktop no longer does either after the Rust change. The smoke step asserts the install survives: no uninstall line in the log, the CLI still runs and reports studio_install_ok, and the launcher route it points people at actually completes the update it refused.
for more information, see https://pre-commit.ci
12d9886 to
5d7829f
Compare
Upstream CI failed here, and the step was wrong rather than the code. The refusal worked: the note printed, nothing was removed, the CLI still ran. Then the step asserted studio_install_ok and got false. The raw force-reinstall this step does first moves the installed version without re-running setup, so the manifest is legitimately out of date afterwards, and the update that would refresh it is the one just refused. Asserting it there was asserting that a refusal repaired something, which is the opposite of refusing. The neighbouring step gets away with the same assertion only because its update succeeds and does the repair. Staging missed it because the installed and checkout versions matched, so the reinstall changed no version and the manifest stayed current. What matters after a refusal is that the install was not destroyed, and the CLI answering at all is what proves it: the wreckage this prevents is a stub that starts and cannot import unsloth_cli. So it asserts the CLI runs and returns usable JSON, and studio_install_ok moved to after the launcher route completes the update, where being whole again is the actual claim.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa12c428d
ℹ️ 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".
| try: | ||
| if not shim.is_file() or shim.stat().st_size <= 0: | ||
| return False | ||
| return os.access(shim, os.R_OK) |
There was a problem hiding this comment.
When bin\unsloth.exe is a readable, nonempty but truncated or corrupt file, this predicate accepts it, so _refuse_update_that_would_break_the_install() aborts the intact venv-copy update and _note_self_exe_locked() directs the user to a launcher that Windows cannot execute while hiding the reinstall fallback. Fresh evidence beyond the earlier zero-byte concern is that the final tree now checks the MZ header in the Rust launcher selector, while this CLI predicate still only calls os.access; apply at least the same Windows image-header check here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and the inconsistency you point at is the argument: two selectors in the same PR disagreeing about which shims count as runnable is a bug regardless of which one is right.
_is_usable_launcher() now applies the same image-header check on Windows as find_unsloth_launcher_in_studio_dir (studio/src-tauri/src/process.rs:414-425), so a nonempty readable file that is not a PE image no longer counts, and the refusal does not fire towards it. Windows-only, since Unix has an execute bit and the shim there is not an image.
Three tests, in unsloth_cli/tests/test_studio_update_exe_lock.py: a truncated launcher (b"PK\x03\x04...", so only the header check can reject it, not the size or readability checks that were already there), a real MZ image still accepted, and the header check not applied off Windows. Falsifiability-checked by removing the header check with __pycache__ cleared: exactly the truncated case flips to failing, and the other two keep passing, so they are pinning behaviour rather than the new code.
Fixed in d913ad1.
There was a problem hiding this comment.
Follow-up: the fix I pushed for this had a regression in it, and the regression was the mirror image of the bug.
Making the predicate stricter widens the path it guards. _refuse_update_that_would_break_the_install() only refused when the launcher was usable, so every file the new header check newly rejects now falls through to the destructive update instead. A half-written Copy-Item fallback (install.ps1:2921), or an antivirus holding the shim so the open() raises, went from "refuse, install survives" to "proceed, install destroyed" and silently, since _note_self_exe_locked was only reached after the gate. Recovery is impossible on that path: the rename failed, so there is no .deleteme for _restore_self_exe_lock_windows to put back.
The premise behind the gate does not hold either. "Stopping would leave them unable to update at all" is only true if going on leaves them able to, and it does not, it destroys the install and they reinstall anyway without having been told to.
So the predicate now decides which message to print, not whether to stop. Windows refuses unconditionally, _note_self_exe_locked already prints the reinstall command when there is no usable launcher, and UNSLOTH_ALLOW_LOCKED_UPDATE=1 is still there for anyone who would rather take the chance. Two tests that asserted the fall-through now assert the refusal.
04dff15ea.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04dff15eaa
ℹ️ 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".
| _manifest = _studio_deps.load_install_manifest_module() | ||
| no_torch = _manifest is not None and _manifest.recorded_no_torch() is True | ||
| except Exception: | ||
| no_torch = False |
There was a problem hiding this comment.
When a GGUF-only user follows the new UNSLOTH_ALLOW_LOCKED_UPDATE=1 escape hatch with a missing or unusable shim, setup removes the manifest and pip can uninstall the package—including studio/install_manifest.py—before failing on the locked executable. This function is called only afterward, so load_install_manifest_module() returns None, the surviving .unsloth-no-torch marker is never read, and the printed reinstall command omits UNSLOTH_NO_TORCH, causing recovery to install the multi-GB PyTorch stack. Fresh evidence beyond the earlier recovery-command comment is the newly added override path, which now allows setup to destroy the helper before this command is constructed; snapshot the recorded mode before running setup or read the durable marker without dynamically loading the removed package.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the reachability argument is right too: making the refusal unconditional turned UNSLOTH_ALLOW_LOCKED_UPDATE=1 into the only way past it, so this path went from incidental to the documented one.
Traced it rather than taking it on trust. _note_self_exe_locked is called a second time at unsloth_cli/commands/studio.py:3082, inside the except around _run_setup_script, so by then pip has uninstalled the package. studio/install_manifest.py ships inside it (studio is a top-level package in the wheel, alongside unsloth and unsloth_cli), so load_install_manifest_module() returns None, recorded_no_torch is never consulted, and the else branch prints a recovery command without UNSLOTH_NO_TORCH. A GGUF-only user pastes it and gets the full PyTorch stack.
Took the first of your two suggestions. The mode is now snapshotted immediately before _run_setup_script, while the reader still exists, and _reinstall_command prefers the snapshot. I did not go for reading the marker directly: that would duplicate venv_root() and NO_TORCH_MARKER in a second place, and the two would drift.
The snapshot only records when the module actually loads, so an unreadable manifest leaves it unset and a later call still gets its chance, and is True is preserved throughout, so recorded_no_torch returning None does not become a claim that torch was wanted.
Two tests. One drives the real sequence, snapshot while intact, then the loader returning None as pip leaves it, and asserts UNSLOTH_NO_TORCH survives into the command. One asserts an unknown mode is not invented, since the opposite error reinstalls a torch user without torch. Falsifiability-checked with __pycache__ cleared: reverting to the live read fails exactly the first and leaves the other 29 passing.
Fixed in 68e407a.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3867afcc04
ℹ️ 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 _RECORDED_NO_TORCH is None: | ||
| _snapshot_recorded_no_torch() | ||
| no_torch = _RECORDED_NO_TORCH is True |
There was a problem hiding this comment.
When _reinstall_command() is called more than once in the same Python process, the first snapshot is reused forever. In the existing test_studio_update_verify.py order, the custom-root tests call this before the no-torch test and cache the ambient False/unknown value, so the later monkeypatched no-torch manifest is never read and the recovery command omits UNSLOTH_NO_TORCH; the reverse order can also leak a previous True into a torch install. Reset/cache this per invocation or per install root instead of skipping _snapshot_recorded_no_torch() after any previous call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it was already broken, not merely fragile. I ran only test_studio_update_exe_lock.py after the last change instead of the suite; running unsloth_cli/tests/ shows my commit turned two pre-existing tests red:
FAILED test_studio_update_verify.py::test_a_no_torch_install_keeps_that_mode_in_the_reinstall[Linux-| UNSLOTH_NO_TORCH=1 sh]
FAILED test_studio_update_verify.py::test_a_no_torch_install_keeps_that_mode_in_the_reinstall[Windows-$env:UNSLOTH_NO_TORCH = '1'; irm]
2 failed, 859 passed
Exactly the order you describe. Apologies for shipping that.
The framing in your last sentence is the fix: the snapshot should never have been consulted first. It is a fallback for the case where nothing can answer, not a cache of the answer. So _reinstall_command now reads live every time and falls back only when the read cannot answer at all, which is the destroyed-install case it exists for. Split into _live_recorded_no_torch() returning Optional[bool], so "the reader is gone" stays distinct from "torch was wanted" and neither collapses into the other.
Per-invocation rather than per-root: keying on the root would still be a cache, and the mode can change under a fixed root when an update migrates out of no-torch mode.
Added a test that pins the ordering directly: two _note_self_exe_locked calls in one process against different manifests, asserting the second answers for its own install. Falsifiability-checked with __pycache__ cleared -- restoring the cache-first form fails that test and both of the ones above, and the live-first form passes all 862.
Also merged main in, so this is no longer 26 commits behind.
Fixed in cc829a9.
|
@codex review |
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: ebcdd5dac5
ℹ️ 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".
| typer.echo("", err = True) | ||
| typer.echo("The launcher that avoids this is unusable; reinstall to restore it:", err = True) | ||
| typer.echo("", err = True) | ||
| typer.echo(f" {_reinstall_command()}", err = True) |
There was a problem hiding this comment.
When this missing-launcher branch is hit from a custom Windows install in a fresh shell, the reinstall hint can still target the default root: _resolve_studio_home() only infers a custom root from sys.prefix when share/studio.conf or bin\unsloth.exe exists, and the Windows installer does not create studio.conf, so a missing bin\unsloth.exe leaves no sentinel and STUDIO_HOME falls back to ~\.unsloth\studio. Fresh evidence beyond the earlier recovery-command issue is that this final path now explicitly handles the missing shim, but it renders _reinstall_command() after the custom root has already been lost, so the pasted command repairs a different install while the custom one remains broken.
Useful? React with 👍 / 👎.
| find_unsloth_launcher_in_studio_dir(&studio) | ||
| .or_else(|| find_unsloth_binary_in_studio_dir(&studio)) |
There was a problem hiding this comment.
When the desktop update runs on Windows with a missing or unusable shim, this fallback starts unsloth_studio\Scripts\unsloth.exe; the CLI now immediately refuses that venv-launched path before setup can determine that the update is a no-op, and start_backend_update only returns the update error rather than falling back to the installer. In that damaged-shim-but-working-venv state, the desktop app can still pass managed preflight via find_unsloth_binary() but every normal backend update fails solely because the selector chose the self-locking entry; return no updater or invoke the installer/repair path instead of falling back to the venv copy on Windows.
Useful? React with 👍 / 👎.
| except OSError as e: | ||
| # Not fatal; setup.ps1 retries from a sibling process. | ||
| print(f"[update] could not rename {exe.name} -> {stale.name}: {e}") | ||
| # Windows locks the directory entry an image was launched from, not the file |
There was a problem hiding this comment.
This catches every os.replace(exe, stale) failure and feeds it into the new pre-setup refusal, but an existing unsloth.exe.deleteme can be the locked path rather than the current Scripts\unsloth.exe entry—for example after a prior aborted update while antivirus or another process temporarily holds the stale backup. In that case a shim-launched update would have left Scripts\unsloth.exe replaceable and pip does not need the .deleteme file, but the new refusal blocks the update before setup; distinguish a destination-backup failure from evidence that the updater is running from the file pip must replace.
Useful? React with 👍 / 👎.
|
Full CI for this PR run on a staging replica, to keep it off the org queue, which was 133 runs deep against 5 concurrent runners when I started. 17 of 17 green on The one that matters most here, One failure on the way, worth recording since it is not this PR: Remaining on the org queue are the four this replica does not reproduce: Lint CI, Wheel CI, Local Agent Guides CI, Core. Staging PR closed, not merged. |
|
One thing to be clear about before anyone reads the check list on this PR: the red entries are cancellations I made, not failures.
All 17 green, listed in the comment above. I am deliberately not re-running them here. The queue was 133 deep against 5 concurrent runners when I started and is 250 deep now, so putting 17 runs back would cost everyone else hours to turn a documented green into a displayed green. If you would rather see them run natively before merge, say so and I will re-run them, or just re-run from the Actions tab. The genuinely native checks on this PR are Lint CI, Wheel CI, Local Agent Guides CI and Core, which staging does not reproduce. Those are the ones to read normally. |
1 participant
Closes #7697.
What is actually going on
Windows locks the directory entry an image was launched from, not the file behind it. Measured on
windows-latest, with the hardlinked launcher (nlink: 2, same inode, so genuinely one file):Scripts\unsloth.exerenameable?bin\unsloth.exerenameable?Scripts\unsloth.exebin\unsloth.exe(launcher)So
_release_self_exe_lock_windowscan never move the copy it is itself running out of. Its failure was aprint, the run continued, and pip then reached the same file, died mid-uninstall, and reported it as a permissions problem:That is the mechanism behind telling Windows users to re-run the full
irm ... | iexinstaller: the installer runs from a separate process and holds nothing.Change
The rename failure is returned instead of printed and discarded, and it is turned into a message only if setup actually fails. It names the launcher on PATH, or the installer when no launcher exists.
It deliberately stays non-fatal. Aborting on the failed rename was my first cut and it is wrong: an update with no package change never touches
unsloth.exeand completes fine from the venv copy today, so failing early would break a path that currently works, to pre-empt a failure that would not have happened.Why CI never caught it
studio-windows-update-smoke.ymlasserts both of its updates are no-ops:A no-op update never makes pip rewrite
Scripts\unsloth.exe, so the lock is unreachable and the whole class of failure was invisible. The job now also forces a replacement and drives it through the launcher, which is the supported path, and fails if that path ever hits the lock. It asserts the log is non-empty, because an earlier version of this check passed only becauseunslothwas not on PATH and the command never ran.Verification
The table above is from a diagnostic run on a real
windows-latestrunner, not from reasoning about NTFS.838 passedinunsloth_cli/tests. Three of the seven new tests fail with the change reverted; the other four are the guards that it stays non-fatal, that other platforms are untouched, and that an unrelated setup failure is not blamed on this.