Guard Windows Studio installs against active runtimes - #7764
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: 2b88f5a98d
ℹ️ 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".
|
|
||
| $studioRuntimeMutex = $null | ||
| try { | ||
| if ($StudioRedirectMode -eq 'legacy') { |
There was a problem hiding this comment.
The installer never assigns StudioRedirectMode the value legacy; its only values are env, profile, and default at lines 301-309. Consequently, default/Tauri installs never acquire the mutex shared with start_backend, so a backend can still start after the process scan and while the environment is being moved or restored. The desktop-process check at line 1945 is disabled for the same reason.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 807c3d0: the installer now acquires the reachable SID-scoped Global runtime guard for the physical Tauri-managed root.
| } | ||
| } | ||
|
|
||
| $runningVenvProcesses = @(Get-RunningStudioVenvProcesses -VenvPath $VenvDir) |
There was a problem hiding this comment.
On an upgrade where the active backend was launched from the supported old .venv layout, this scan only examines the new unsloth_studio directory. The backend resolver explicitly falls back to .venv in studio/src-tauri/src/process.rs:455-472, but the installer subsequently moves that directory at lines 1982-2010 and installs into it without detecting the running process; the same omission applies to the CWD-relative environment migrated at lines 2013-2023. Scan every migration source before renaming or modifying it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 807c3d0: the installer scans and deduplicates the target plus both legacy migration sources before any move.
| Exit-StudioInstallMutex -Mutex $studioRuntimeMutex | ||
| Exit-StudioInstallMutex -Mutex $studioInstallMutex |
There was a problem hiding this comment.
For a manual interactive install, both mutexes are released before the user is prompted at lines 3260-3265. While that prompt is open, a second installer can acquire the locks, pass its process scan, and begin replacing the environment; accepting the first installer's prompt then launches unsloth studio from that environment without participating in the Tauri mutex, recreating the startup-versus-replacement race this change is intended to prevent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 807c3d0: both locks remain held through the prompt and process creation, then the installer waits outside the critical section.
| $script:StudioVenvRollbackTarget = $VenvDir | ||
| $script:StudioVenvRollbackActive = $false | ||
|
|
||
| $script:StudioManagedRuntimeMutexName = "Local\UnslothStudioManagedEnvironment" |
There was a problem hiding this comment.
The Local\ namespace scopes this runtime mutex—and the install mutex returned at line 1714—to one Windows logon session, while the protected environment is stored in the user's shared profile. If the same account has Studio or an installer active in a disconnected RDP session and starts an install from another session, each process can acquire a different mutex; two installers can then replace the same directory concurrently, or a backend can start after the other session's process scan. Use a cross-session locking mechanism for this per-profile resource.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 807c3d0: runtime locking is Global and user-SID scoped, while install locking is Global and path scoped.
| $commandLineMatch = ( | ||
| $process.CommandLine -and | ||
| $process.CommandLine.IndexOf($venvNeedle, [System.StringComparison]::OrdinalIgnoreCase) -ge 0 | ||
| ) |
There was a problem hiding this comment.
The command-line fallback performs an unrestricted substring search for the venv path. Thus, when the managed directory is C:\...\unsloth_studio, an unrelated process whose command line references C:\...\unsloth_studio_backup\worker.py is reported as using the managed environment and every install is rejected until that process exits, even though none of its files will be modified. Match a directory boundary after the venv path rather than any shared textual prefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 807c3d0: command-line detection now requires a path boundary, rejecting sibling prefixes while preserving exact and descendant matches.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd9aa2b26
ℹ️ 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".
| $leftFull = [System.IO.Path]::GetFullPath($Left).TrimEnd('\', '/') | ||
| $rightFull = [System.IO.Path]::GetFullPath($Right).TrimEnd('\', '/') |
There was a problem hiding this comment.
When $StudioHome reaches the Windows profile through a directory junction or symbolic-link alias—such as a redirected USERPROFILE that points back to the physical profile—Path.GetFullPath only normalizes path syntax and leaves the reparse-point alias intact. The comparison therefore returns false even though both paths address the same environment, causing $studioUsesTauriManagedRoot to remain false and skipping the runtime mutex at line 1995; a Tauri backend can then start after the process scan while that physical environment is being replaced. Compare resolved filesystem identities/final paths rather than lexical absolute paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d7114f. Installer mutex hashing and managed-root classification now use GetFinalPathNameByHandleW so junction and symlink aliases resolve to the same filesystem path. Unknown identity acquires the runtime lock without enabling legacy migrations. Added junction and unknown-identity regressions for pwsh and Windows PowerShell 5.1.
| foreach ($candidateVenv in @($venvPathsToScan | Select-Object -Unique)) { | ||
| foreach ($process in @(Get-RunningStudioVenvProcesses -VenvPath $candidateVenv)) { |
There was a problem hiding this comment.
When a user starts unsloth studio from a terminal after this one-time scan completes, that supported launcher does not acquire the new named mutex: the Windows paths in unsloth_cli/commands/studio.py:1596-1600 and :1622-1644 spawn or run the managed backend directly. It can therefore enter the environment while the installer is renaming or rebuilding it, causing the backend to load a mixture of the rollback and replacement trees or to lock files needed by the install. Make CLI backend admission participate in the same runtime gate rather than relying only on a snapshot of already-running processes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d7114f. Terminal plain-server and run paths now join the same Global SID-scoped mutex around backend admission. The run re-exec uses a one-shot child-only environment handoff so the parent holds the gate until the managed child exists. Added native contention, mutex-name, handoff, and launch-boundary regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b428aa33
ℹ️ 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".
| #[cfg(windows)] | ||
| let _runtime_launch_guard = acquire_studio_runtime_launch_guard()?; |
There was a problem hiding this comment.
On Windows, the mutex is acquired only by start_backend, while the initial desktop_preflight still launches the same managed executable through run_cli_probe and probe_cli_capability in studio/src-tauri/src/preflight/managed.rs:305-379 without acquiring it. If a second desktop instance opens after a --tauri installer has completed its process scan, these probes can start while that installer is renaming or rebuilding the environment; Tauri installs deliberately skip the desktop-process check, so the second app does not otherwise block the mutation. Require every Tauri subprocess launched from the managed environment, including preflight and auth provisioning, to participate in this gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ea1c24. All managed Tauri child launches now use the SID-scoped runtime gate. Read-only probes hold it through spawn; update performs a post-lock managed-process scan and holds it through the full mutation. Backend and installer autostart use a one-shot handoff to avoid self-contention. Added Win32 mutex, process-scan, and ordering regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7114f1d0
ℹ️ 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 sys.platform != "win32" or inherited or not uses_tauri_managed_root(studio_home): | ||
| yield False | ||
| return |
There was a problem hiding this comment.
When Windows users set UNSLOTH_STUDIO_HOME or STUDIO_HOME to a custom root, this condition makes every terminal launch guard a no-op; the installer likewise skips its runtime mutex when that root differs from the Tauri-managed root and relies only on its one-time process scan. A supported unsloth studio launch can therefore begin after the scan while the custom environment is being renamed or rebuilt. The fresh evidence beyond the earlier terminal-launch report is this attempted fix's explicit custom-root early return; custom roots need a path-scoped runtime gate shared with the installer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d7b07f: custom roots now use the same path-scoped runtime mutex in PowerShell and Python, with exact UTF-8 hashing and dual SID/path locking when default-root identity is unknown.
| $matchIndex = $CommandLine.IndexOf($Path, $searchFrom, [System.StringComparison]::OrdinalIgnoreCase) | ||
| if ($matchIndex -lt 0) { return $false } |
There was a problem hiding this comment.
When a launcher hands off to a base Python executable and supplies a managed script using Windows-valid forward slashes, such as C:/Users/me/.unsloth/studio/unsloth_studio/worker.py, $Path has been normalized by GetFullPath to backslashes and this literal IndexOf never matches it. Because the executable itself is outside the venv, the earlier executable-path scan also misses the process, so installation can rename or rebuild the environment while that process is consuming it. Normalize both path spellings, or search equivalent slash variants, before applying the boundary check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d7b07f: the installer normalizes forward slashes in both the command line and managed path before applying the path-boundary check, with a forward-slash regression test.
| $studioAutoStartProcess = Start-Process -FilePath $UnslothExe ` | ||
| -ArgumentList @("studio", "-p", "8888") ` | ||
| -NoNewWindow -PassThru |
There was a problem hiding this comment.
On a default Windows install, the installer still owns $studioRuntimeMutex when this process is created, while the new CLI immediately uses a zero-timeout acquisition of that same mutex before starting its server. If the child is scheduled through its startup checks before Start-Process returns and the finally at line 3466 releases the lock, it exits with “installation is modifying the managed environment,” so accepting the installer’s launch prompt can sporadically fail despite a successful install. The fresh evidence beyond the earlier auto-start thread is the newly added CLI gate conflicting with the lock that the proposed fix deliberately retains through process creation; pass a one-shot handoff or release only the runtime gate before spawning while retaining installer serialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ea1c24: installer auto-start now passes a one-shot handoff so the guarded parent releases its mutex before the spawned Studio process acquires the long-lived runtime guard.
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: 4441d929db
ℹ️ 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".
| // Unlike read-only probes, update mutates the managed environment for its | ||
| // entire lifetime. This function is synchronous, so the thread-owned Win32 | ||
| // mutex is acquired and released on the same thread without crossing await. | ||
| let result = crate::process::with_studio_runtime_launch_guard(|| { |
There was a problem hiding this comment.
This serializes only updates launched through the desktop. The supported terminal path unsloth studio update still reaches _run_setup_script() in unsloth_cli/commands/studio.py without acquiring the runtime mutex, so if a user starts a terminal update and then opens Studio, start_backend can acquire the free mutex and spawn from the venv while setup is replacing it. Wrap the CLI update mutation in the same gate as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1c18b4. Direct CLI updates now consume the one-shot handoff, acquire the shared runtime gate before the idle scan, and retain it through setup and verification. Tauri passes the handoff marker so its already-gated update child cannot self-block.
| if let Some(image) = process_image_path(entry.th32ProcessID) { | ||
| if let Ok(image_key) = normalized_existing_windows_path(&image) { | ||
| if windows_path_is_within(&image_key, &venv_key) { |
There was a problem hiding this comment.
This idle check only considers the process image path. If an existing consumer was started with a system Python and a script/module path under the managed venv, image_key is outside venv_key, so the desktop update proceeds and rewrites files underneath it; the installer handles this same class with a command-line fallback, but Tauri updates do not. Include equivalent command-line scanning before allowing the update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1c18b4. The Tauri idle scan now checks command lines with component boundaries, resolves existing path arguments, and uses the actual per-process working directory for relative arguments. The process query fails closed before mutation.
| } | ||
| } | ||
|
|
||
| $venvPathsToScan = @($VenvDir) |
There was a problem hiding this comment.
This scan only checks $VenvDir and legacy venv roots, but the installer also exposes the supported $StudioHome\bin\unsloth.exe shim/hardlink. When a server was started through that shim before the installer reaches this point, the process image and command line can reference bin\unsloth.exe without containing $VenvDir, so the scan returns empty and the installer can roll the managed venv while that server is still serving from it. The fresh evidence beyond the earlier terminal-launch gate is that the new pre-mutation scan still excludes the shim path it creates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1c18b4. The exact StudioHome\bin\unsloth.exe shim is now protected by installer, Tauri, and direct-update scans for both executable images and command-line references.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c6f191025
ℹ️ 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 ($LASTEXITCODE -ne 0 -or -not $json) { return @{} } | ||
| $decoded = $json | ConvertFrom-Json -ErrorAction Stop | ||
| } catch { | ||
| return @{} |
There was a problem hiding this comment.
When repairing a damaged environment whose managed Python cannot run this probe or import psutil, this branch silently returns an empty CWD map. A live consumer launched through a base interpreter with a relative command such as python worker.py and its working directory inside the managed venv then has neither an executable path nor a command-line path matching the venv, so the subsequent scan misses it and the installer can rename files it is using. Treat probe failure as fatal or obtain CWDs independently of the environment being repaired.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentionally not used as a blocking signal. A working directory beneath the Studio root does not prove that a process is executing the managed runtime, and failing closed when CWD inspection is unavailable would stop installs for unrelated terminals or tools. The guard is deliberately limited to confirmed executable identities and the exact outer shim; the focused tests preserve that non-blocking behavior.
| $_tauriOverride = [System.IO.Path]::GetFullPath($_tauriOverride) | ||
| } catch {} | ||
| $_legacyTauriRoot = Join-Path $env:USERPROFILE ".unsloth\studio" | ||
| $_legacyTauriRoot = Join-Path $tauriProfile ".unsloth\studio" |
There was a problem hiding this comment.
When USERPROFILE or an explicit UNSLOTH_STUDIO_HOME reaches the physical Windows profile through a junction or symlink, this now compares the override against the profile API's lexical spelling using only GetFullPath. Two paths that identify the same Tauri-managed root therefore compare unequal and --tauri aborts as if a custom root were requested. The fresh gap beyond the later managed-root alias fix is this earlier validation, which still runs before Get-StudioFinalPath is defined or used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Tauri override validation now resolves both the override and legacy root through Get-StudioFinalPath before comparing them, so junction or symlink aliases of the managed root are accepted. A live Windows junction regression test covers this path.
|
|
||
| # Resolve existing path arguments so a command line using a junction or | ||
| # symlink spelling still maps to the physical managed environment. | ||
| foreach ($token in [UnslothStudioFinalPath]::ParseCommandLine($CommandLine)) { |
There was a problem hiding this comment.
When a base PowerShell process consumes a managed file through a junction alias inside a command payload such as powershell -Command "& 'C:\Alias\worker.ps1'; Start-Sleep 30", CommandLineToArgvW returns the entire payload as one token and this loop treats that whole expression as a filesystem path. The raw scan cannot match the alias to the physical venv and the expression does not resolve as a path, so the live consumer is missed and the installer can rename its environment. The fresh gap beyond the attached-option parsing fix is that shell command-language arguments remain opaque after argv parsing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is outside the detector's intentional evidence boundary. Parsing arbitrary shell-language command strings would be heuristic and can flag text that is not an executing image. The guard blocks only confirmed managed executable images and the exact installed shim; command-line text remains deliberately non-blocking and is covered by the focused contract tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d796c995e
ℹ️ 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".
| with _studio_runtime_launch_guard(inherited = runtime_gate_handoff): | ||
| run_server(**run_kwargs) |
There was a problem hiding this comment.
When a user starts the supported unsloth studio command from a terminal after the installer has acquired the mutex and completed its process scan, the executable and Python interpreter have already started loading files from the managed environment before execution can reach this guard. The installer can therefore rename or rebuild the environment concurrently with interpreter startup, causing launch failures, locked-file rollback failures, or mixed-version imports. Fresh evidence beyond the earlier terminal-launch report is that the attempted fix acquires the mutex only inside studio.py, rather than in the native launcher before managed Python starts; move admission into the launcher or another pre-interpreter component.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The direct terminal path is intentionally a snapshot check in this PR: it blocks when an existing managed process is detected before installation, but it does not introduce a stable native pre-interpreter launcher. Closing the post-scan launch window would require that larger launcher redesign. The PR description now states this boundary explicitly; Tauri native launches still participate in atomic mutex coordination.
Preserve upstream managed-Python validation while moving the shared lock and process preflight ahead of dependency setup. Block only confirmed managed executable images and the exact shim; remove command-line/CWD heuristics and the unrelated Playwright delta.
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: 1cc45d5813
ℹ️ 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".
|
|
||
| function Get-StudioRuntimeMutexNameForPath { | ||
| param([Parameter(Mandatory = $true)][string]$Path) | ||
| return "Global\UnslothStudioManagedEnvironmentPath-$(Get-StudioRuntimePathHash -Path $Path)" |
There was a problem hiding this comment.
When UNSLOTH_STUDIO_HOME/STUDIO_HOME points at a custom root, or when default-root identity resolution returns unknown, Get-StudioRuntimeMutexNames calls this path-scoped mutex helper before execution has reached the later Get-StudioRuntimePathHash definition. PowerShell does not hoist nested function definitions ahead of execution, so these installs fail while creating the runtime lock instead of acquiring the custom-root gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Get-StudioRuntimePathHash is now defined before Get-StudioRuntimeMutexNameForPath and before the custom-root lock acquisition can call it. A source-order regression test enforces hash helper, mutex helper, then acquisition ordering.
| parent_pid = int(descendant.get("ParentProcessId") or -1) | ||
| if parent_pid <= 0 or parent_pid in excluded_pids: | ||
| break | ||
| excluded_pids.add(parent_pid) |
There was a problem hiding this comment.
When a still-running managed process launches unsloth studio update as a child, this loop adds that parent's PID to excluded_pids before the executable-path check, so a parent whose image is under STUDIO_HOME\unsloth_studio is skipped and setup can replace the environment while that parent is still consuming it. Exclude only the current updater or a verified launcher shim/shell, not every ancestor in the process tree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Ancestor suppression now continues only through a parent whose canonical executable is a verified protected launcher. It stops before a managed backend parent, which remains visible and blocks replacement. The regression suite now covers updater -> verified launcher -> managed backend ancestry.
Etherll
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9629c7835e
ℹ️ 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".
|
|
||
|
|
||
| def _canonical_windows_path(path: Path) -> str: | ||
| return _resolved_windows_path(path).casefold() |
There was a problem hiding this comment.
When the profile and a custom Studio root contain distinct names with the same Unicode case fold (for example, C:\Users\Straße\... and C:\Users\Strasse\...), casefold() classifies the custom root as Tauri-managed even though Windows ordinal comparison—and the installer's Test-StudioPathEqual—treats them as different paths. The CLI then takes the SID mutex while the installer takes the path-hash mutex, allowing a launch during mutation of that custom environment. Use the same Windows ordinal case-insensitive comparison as the installer rather than full Unicode case folding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Tauri-root classification now compares resolved paths with Windows CompareStringOrdinal in case-insensitive mode, matching the installer's OrdinalIgnoreCase semantics without Unicode case-fold expansions. The regression distinguishes Straße from Strasse while retaining ordinary case-only equality.
| if (-not (Test-Path -LiteralPath $fullPath)) { | ||
| return $fullPath | ||
| } |
There was a problem hiding this comment.
When .unsloth\studio does not yet exist beneath a junction-aliased USERPROFILE, this early return preserves the alias spelling instead of resolving the existing parent. An installer using that alias therefore takes a path-scoped runtime mutex and alias-derived install mutex, while another process using the physical profile path classifies the same destination as Tauri-managed and takes the SID mutex plus a different install mutex; both first-install processes can then concurrently create and mutate the same directory. Fresh evidence beyond the earlier alias fix is that its resolver explicitly skips GetFinalPathNameByHandleW for the absent child; resolve the deepest existing ancestor and append the missing suffix before comparing or hashing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Get-StudioFinalPath now walks to the deepest existing ancestor, resolves that physical identity, and appends the missing suffix. A live junction regression keeps the child absent and verifies alias/physical path equality, identical install mutexes, and the same SID runtime mutex family.
| # process has Studio files open and should not create a false abort. | ||
| foreach ($process in @(Get-Process -ErrorAction SilentlyContinue)) { | ||
| $executable = $null | ||
| try { $executable = $process.Path } catch { continue } |
There was a problem hiding this comment.
When the installer is run from 32-bit Windows PowerShell while an existing managed Studio executable is 64-bit, accessing Process.Path can fail during the cross-bitness MainModule lookup. This catch silently skips that process, so a legacy or otherwise pre-gate backend does not appear in the pre-mutation scan and the installer can roll its live environment. Query the image with a cross-bitness API such as QueryFullProcessImageNameW (as the Rust side already does) instead of treating every path-access failure as an unrelated process.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Process images are now queried through OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) plus QueryFullProcessImageNameW instead of Process.Path. A dedicated test runs 32-bit Windows PowerShell against a 64-bit managed probe, alongside the existing live process-detection tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99aed4438a
ℹ️ 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 (-not $TauriMode -and $studioUsesLegacyLayout) { | ||
| $runningDesktopApps = @(Get-Process -Name "unsloth-studio" -ErrorAction SilentlyContinue) |
There was a problem hiding this comment.
On multi-user or RDP Windows hosts, this unfiltered query also returns another logged-in user's unsloth-studio process, even though that user's Studio environment and the newly SID-scoped runtime mutex are separate. PowerShell documents that, without parameters restricting ownership, Get-Process gets all processes on the local computer (Get-Process documentation); consequently the installer can demand that the current user close a process they neither own nor can stop, indefinitely preventing installation. Filter the results by the current user SID before treating them as a conflict.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The legacy desktop fallback now reuses the current Windows SID and filters Win32_Process candidates through GetOwnerSid. Different-user and unreadable owners no longer block, while same-user processes across RDP sessions remain protected; the regression covers current, other, and unavailable owner results and explicitly excludes SessionId filtering.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44cc2acb8e
ℹ️ 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".
|
|
||
| function Get-StudioFinalPath { | ||
| param([Parameter(Mandatory = $true)][string]$Path) | ||
| $fullPath = [System.IO.Path]::GetFullPath($Path).TrimEnd('\', '/') |
There was a problem hiding this comment.
When a supported custom root is a drive root such as UNSLOTH_STUDIO_HOME=D:\, trimming the trailing separator turns it into D:, which Windows interprets as the drive-relative current directory rather than the root. Get-StudioFinalPath can consequently resolve and hash D:\work while Python hashes D:, so the installer and launcher acquire different path-scoped runtime mutexes; separate installers launched with different per-drive working directories can also acquire different install mutexes while mutating the same D:\unsloth_studio. Preserve the separator for filesystem roots before resolving or hashing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. PowerShell now preserves a filesystem root separator before alias resolution and after final-path resolution, and the Python path identity helper does the same before hashing. The regression uses the Windows drive root and verifies the resolved path plus installer and path-scoped runtime mutex names across both pwsh and Windows PowerShell.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60aab6a42a
ℹ️ 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".
| candidate_key = candidate.rstrip("\\/").replace("/", "\\").casefold() | ||
| root_key = root.rstrip("\\/").replace("/", "\\").casefold() |
There was a problem hiding this comment.
Although managed-root classification now uses CompareStringOrdinal, this separate scan still applies Unicode casefold() here and in _canonical_windows_path. On Windows, distinct custom roots such as D:\Straße\studio and D:\Strasse\studio can therefore collapse to the same key, causing an executable in the unrelated sibling installation to be reported as using the environment and blocking unsloth studio update until it exits. Preserve the resolved spelling and use Windows ordinal case-insensitive equality and component-prefix comparisons for this scan as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The idle scan now preserves resolved path spelling and uses CompareStringOrdinal for both component-prefix containment and exact protected-executable matches. The Windows regression verifies that Straße matches case-only straße but remains distinct from Strasse; the focused suite passes 116 tests.
1 participant
Summary
Guard Windows Studio installs against processes that are already using the managed environment.
Before rollback or replacement starts, the installer now acquires its safety locks, checks confirmed executable identities, and stops with a clear warning when Studio or another managed process is active. If no managed executable is active, installation proceeds normally. Tauri desktop backend starts and updates use the same native Windows mutex, so they cannot enter during installation.
Motivation
The desktop app and manual installer both operate on
%USERPROFILE%\.unsloth\studio\unsloth_studio. Replacing that environment while its Python backend is active can leave the original path incomplete while the healthy environment remains in a rollback directory.uv --cleardoes not solve files that are still in use.Changes
bin\unsloth.exeshim.Scope
The direct terminal decision is intentionally snapshot-based: a process already active when installation begins is rejected. A new
unsloth studiocommand launched after that scan is not atomically gated before managed Python starts; closing that separate edge would require replacing the installed console shim with a stable native launcher. This PR does not add that launcher. Tauri desktop starts are atomically coordinated because their launcher is already native.Non-Windows behavior is unchanged.
Validation
python -m pytest tests/python/test_studio_runtime_gate.py tests/python/test_windows_installer_concurrency_guard.py tests/python/test_windows_python_venv_hardening.py unsloth_cli/tests/test_studio_run_parallel_flag.py -q: 118 passed.cargo test --manifest-path studio/src-tauri/Cargo.tomlwith the test frontend override: 115 passed.git diff --check: passed.