Windows: guard the source-build and whisper.cpp probes on an unreadable install tree - #7757
Conversation
…le install tree Follow-up to #7735, which routed the prebuilt llama.cpp probes through three-state path probing but left two gaps. Phase 4 read $LlamaServerBin with a bare Test-Path under "Stop". A forced compile, a pinned PR or a custom llama source skips Phase 3.4 entirely, so on those routes this was the first probe inside the tree and a denied build\ aborted with the raw "Test-Path : Access is denied" the merged PR set out to remove. It now probes three-state, and the CMakeCache.txt read below it is guarded too: a listed file can still deny the read, which the probe cannot see. The probe is skipped for a linked UNSLOTH_LOCAL_LLAMA_CPP_DIR, where it would read through the junction into the user's own checkout, and the denial reports -OwnershipUnverified under a custom home, where nothing on this route has proven the tree is ours. The whisper.cpp phase promises failure is never fatal, but under a custom UNSLOTH_STUDIO_HOME an unreadable tree exited the whole run, taking llama.cpp inference down with it. Assert-StudioOwnedOrAbsent gains a -NonFatal mode that hands the denial back instead; an unowned tree still stops. The check stays behind the installer-exists gate it used to sit inside, so a tree without install_whisper_prebuilt.py remains the no-op it was. Backend: _is_runnable let Path.is_file() propagate EACCES. Now that setup leaves a denied whisper.cpp in place, that turned into a 500 out of /api/inference/audio/stt/status, the one endpoint reporting both dictation engines, so the setup message promising Transformers dictation still works was not true. It reads as engine-unavailable instead.
![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: 393d16f8b9
ℹ️ 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".
| $CmakeCacheFile = Join-Path $BuildDir "CMakeCache.txt" | ||
| if (Test-Path -LiteralPath $CmakeCacheFile) { | ||
| $cachedCuda = Select-String -LiteralPath $CmakeCacheFile -Pattern 'GGML_CUDA:BOOL=ON' -Quiet | ||
| if (Test-PathQuiet $CmakeCacheFile "Leaf") { |
There was a problem hiding this comment.
On Windows, when CMakeCache.txt alone has a deny ACE that causes Test-Path to throw, Test-PathQuiet converts Denied to False, so the guarded Select-String block never runs. $NeedRebuild consequently remains false, and with an existing release binary and any requested tag other than master (including the default latest), a forced source-build route can report "already built" and retain a CPU/CUDA binary that does not match the current GPU mode. Probe this file with Get-PathState and handle Denied explicitly before treating it as absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise does not hold on Windows: a deny ACE scoped to a single file whose parent is still listable does not make Test-Path throw. The suite already builds that exact state (icacls on a marker file, parent readable) and gates on whether the bare probe throws; on windows-latest it reports "this host would not deny the marker file" and skips, so Test-PathQuiet returns true, the block runs, and the Select-String catch below produces the actionable denial.
The shape where Test-Path does throw is an unlistable parent, and a deny on llama.cpp\ or build\ is inherited, so $LlamaServerBin probes Denied and line 4497 exits before $CmakeCacheFile is computed. Reaching line 4504 with the binary Present needs a non-propagating deny on build\ that spares build\bin\Release\llama-server.exe, which is a hand-built ACL rather than anything a lockdown produces.
For reference this line was a bare Test-Path under Stop before, which aborted the run outright in the throwing shape, so the change is not a new silent path.
| try: | ||
| return p.is_file() and (sys.platform == "win32" or os.access(p, os.X_OK)) | ||
| except OSError: |
There was a problem hiding this comment.
When setup's new non-fatal path is triggered because only UNSLOTH_WHISPER_PREBUILT_INFO.json is ACL-denied while whisper-server remains accessible, this catch does not run because the binary probe succeeds. is_available() then calls slim_runtime_intact(), whose lookup_marker() performs an unguarded Path.is_file() on that same marker, allowing EACCES to escape and leaving the STT status endpoint returning 500. The availability check needs to treat failures from marker and linked-runtime probing as unavailable too, rather than guarding only _is_runnable().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The described trigger cannot produce the 500. Two things block it.
The marker read is already guarded: _parse_marker in studio/backend/utils/prebuilt/whisper_layout.py catches OSError, so a marker that stats fine but denies the read yields (None, invalid=True) and slim_runtime_intact returns normally.
And there is no state where the binary probe succeeds while the marker probe raises. Measured against the real lookup_marker and the real _is_runnable:
marker mode 000, root readable runnable=True lookup_marker=ok
root mode 000 (no traverse/list) runnable=False lookup_marker=PermissionError
root mode 111 (traverse, no list) runnable=True lookup_marker=ok
root 111 + marker 000 runnable=True lookup_marker=ok
The only raising row is the one where _is_runnable already returns False, so find_whisper_server_binary never hands that path to slim_runtime_intact. Same on Windows: the inherited (OI)(CI) deny that trips setup's non-fatal branch covers the binary too, and a deny scoped to the marker alone leaves the parent listable, so os.stat falls back through the parent and returns normally.
Mutation testing found six ways to reintroduce the bugs this branch fixes while the tests stayed green. Assert-StudioOwnedOrAbsent: the -NonFatal returns were counted, not ordered. Moving one below its Exit-PathAccessDenied makes it dead code and the whisper phase fatal again; hoisting one above the custom-home gate reports a fresh install as unreadable. Each return is now pinned immediately above the exit it pre-empts, with no unpaired return allowed. The whisper denial branch had no assertion scoped to its own body. Both phrases it was checked for already occur elsewhere in the phase, so the branch could be turned back into an Exit-SetupFailure and stay green. The branch is now sliced out and checked for step/Yellow, both phrases, and the absence of any exit. The installer gate was checked for presence, not for being a conjunct, so -or-joining or negating it reopened the installer-less tree the test is named for. The denial subject was unpinned, so it could name llama-server.exe and tell the user to move aside one file instead of the tree. Slice terminators are now asserted through one helper: an unasserted terminator does not fail, it silently widens the window to end-of-file and makes everything inside it near-vacuous. The whisper binary probe test gated its only behavioural case on geteuid() == 0, which silently drops it in any root container. It probes for a real denial instead. Two pre-existing exact counts in the ownership guard tests become floors: this branch consumed the last of their headroom, so the next legitimate route added there would break two tests that say nothing about it.
for more information, see https://pre-commit.ci
The control probed a marker file that did not exist. Windows reports a missing child of a denied directory as absent rather than throwing, so the control read as "this host cannot deny" and failed the suite on windows-latest while passing under chmod on Linux. It now probes a file that exists inside the locked tree, matching the control the suite already uses. That difference also splits the routes by platform for a tree with no ownership marker, which is the fresh custom-home case: Linux catches it on the marker probe, Windows has to catch it on the adoptable-state read. Added a case that accepts either route and rejects anything but Denied, so the Windows one is exercised for the first time.
Staging CI on windows-latest caught this. Get-StudioAdoptableState decided "denied" only from probes of two marker files inside the tree, but Windows reports a MISSING child of an unreadable directory as absent rather than throwing. A denied tree holding neither marker therefore returned "No", and Assert-StudioOwnedOrAbsent fell through to "path is not an Unsloth-owned install" and exited: the wrong cause, and fatal, on the only platform any of this runs on. It also defeated the whisper -NonFatal path, since an unowned tree is still fatal by design. Listing the directory itself distinguishes "no markers here" from "cannot look", so that is the fallback when neither probe reported a denial. A readable tree with no markers still returns "No" as before, and the catch swallows anything that is not a denial because this helper must not throw. This also corrects the message a denied custom-home llama.cpp tree produced on Windows, which reported the same wrong cause. chmod 000 blocks the child probes outright, so it never reached the new code. chmod 111 allows stat of a named child while forbidding a listing, which is exactly the Windows shape, so the test now covers both and the negative control fires only on the 111 case.
|
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". |
…else's (#7771) * Linux/macOS: tell an unreadable llama.cpp install apart from someone else's setup.ps1 learned this in #7735 and #7757; setup.sh never did. A tree that is ours, carrying our own marker, reads as somebody else's the moment it stops being searchable, because every probe inside it reports the marker absent. The ownership guard then said the folder is not an Unsloth-owned install and told the user to move it aside or pick an empty UNSLOTH_STUDIO_HOME, when the actual fix is a permission change. Search (+x) is what those probes need, not read (+r): a directory can be readable and unsearchable (mode 444) or searchable and unreadable (mode 111), so the new probe tests search. The two destructive replaces also ran blind. Under errexit a failing rm -rf aborts on a raw permission error with no [TAURI:ERROR], so the desktop app shows a bare exit code and the freshly built llama.cpp is left in the temp directory with nothing saying where. Both sites now tolerate the failed remove, check that the directory is actually gone, and report either the permission problem or where the new build was left, exiting 3 as the Windows side does for a build that succeeded but could not be installed. Neither message tells the user to delete a folder Unsloth cannot prove is its own, matching the rule the Windows guard follows. * Keep the rm error when a replace fails The redirect this PR added swallowed the one line that names which subtree refused to go. rm reports the deepest failing path, llama.cpp/build say, while the message below it can only name the install root, so a user whose build directory alone is locked lost the part that told them where to look. || true is what keeps errexit from aborting; the redirect was never doing that job, so stderr stays. * Tighten the comments on the denied-install-tree guards
1 participant
Follow-up to #7735. That PR routed the llama.cpp prebuilt probes through three-state path probing (
Present/Absent/Denied) so an ACL-denied%USERPROFILE%\.unsloth\llama.cppreports an actionable error instead of aborting with a rawTest-Path : Access is deniedunder$ErrorActionPreference = "Stop". Two reachable gaps were left open, and I kept them out of that PR to avoid widening it further.1. The source build read inside the tree with a bare probe
Phase 4 probed
$LlamaServerBinwith a bareTest-Pathbefore deciding whether to rebuild. Phase 3.4's denial guard only covers the prebuilt path, andUNSLOTH_LLAMA_FORCE_COMPILE=1,UNSLOTH_LLAMA_PRor a custom$LlamaSourceskip that path entirely, so on those routes this was the first read inside the tree. A deniedbuild\aborted with exactly the raw error #7735 set out to remove.It now probes three-state. Two things came out of reviewing my own first attempt:
UNSLOTH_LOCAL_LLAMA_CPP_DIRis linked.$LlamaCppDiris a junction onto the user's own checkout there, so the probe read through it into their tree and the denial told them to delete it and runtakeown /Ragainst their build. That is what the-UserSuppliedrule at the candidate loop already forbids. Nothing this block computes is consumed on that path anyway, since the linked branch skips both the download and the build.-OwnershipUnverified:$StudioHomeIsCustom. Nothing on the forced-compile route has run the ownership guard, so under a customUNSLOTH_STUDIO_HOMEthe tree cannot be proven ours and the "delete it, we reinstall it" advice does not apply.The
CMakeCache.txtread below it is guarded too.Test-PathQuietonly proves the entry is listed, so a deny ACE on the file itself left the probe returning true andSelect-Stringthrew, reintroducing the same crash one line under the fix.2. whisper.cpp was fatal on a denied tree
The phase header promises "Failure is never fatal: local dictation falls back to Transformers STT", but under a custom
UNSLOTH_STUDIO_HOMEan unreadablewhisper.cpphitAssert-StudioOwnedOrAbsentand exited the whole run, taking llama.cpp inference down with it.Assert-StudioOwnedOrAbsentgains a-NonFatalswitch that hands the denial back instead of exiting. Only that mode returns a value, so the other callers are unchanged, and an unowned tree still stops exactly as before. The check stays behind the installer-exists gate it used to sit inside, so a tree withoutinstall_whisper_prebuilt.pyremains the no-op it was rather than becoming newly fatal.3. The backend then had to survive what setup now leaves behind
Because setup no longer aborts, the backend became the first thing to touch the denied directory.
_is_runnableletPath.is_file()propagateEACCES(_IGNORED_ERRNOScoversENOENT,ENOTDIR,EBADF,ELOOP, notEACCES), and neitherstt_statusnor_resolve_serving_stt_enginewraps it. That turned into a 500 out of/api/inference/audio/stt/status, the one endpoint that reports both dictation engines, so the setup message promising browser and Transformers dictation still work would not have been true. An unreadable install now reads as engine-unavailable, matching the module's existing "never a crash at load" contract.Verification
Behavioural, against real denials rather than only source matching:
Test-Pathcrashbuild\CMakeCache.txtread-deniedtests/studio/test_path_probe_access_denied.ps1now exercises-NonFatalfor real against achmod/icaclsdenial, covering both the root-probe and marker-read routes, with the existing negative control that fails the suite if the host cannot actually produce a denial.tests/studio/test_whisper_binary_probe_denied.pyruns the real_is_runnableagainst a live denied directory.Every new assertion was mutation-tested: each is confirmed red under the bug it exists to catch, and green under a legitimate change (renumbering the phase header, wrapping a long line, adding a comment, hardening another probe, adding a further denial route). Counts are floors rather than exact matches, and the slice anchors assert before indexing so a moved anchor fails as an assertion instead of an
IndexError.Suites:
setup.ps1parses, 12 PowerShell suites pass, 30 shell suites run with only the pre-existingtest_install_host_defaults.shfailures (identical on a cleanmain, and already skipped bytests/run_all.sh), and 3040 Python tests pass with only the pre-existingtest_negative_control_no_tokenizersfailure, which reproduces unchanged onmain.No behaviour changes on readable trees.
setup.shis untouched, so Linux, macOS and WSL are unaffected.