From a1b3de5de589bd7c3696ed56ae04df5348b901c7 Mon Sep 17 00:00:00 2001 From: Chris Kelsey Date: Tue, 22 Sep 2026 15:47:40 -0700 Subject: [PATCH] feat(engine-manager): publish installed engine CLIs on the user's PATH When PAIR installs Ollama or LM Studio, add that engine's command-line directory to the installing user's persistent PATH on that device, and give back only what PAIR recorded when the engine or the application is removed. Open a new terminal after either operation. Local installs only. An install driven by a cluster peer deliberately skips the PATH step, because rewriting the login shell of whoever is sitting at the target node is not a paired peer's decision. Ownership is recorded outside the engine directory and before the PATH is touched, so a crash mid-update is retryable and an uninstall can delete exactly what PAIR added. Existing entries, user-edited shell snippets, and engines the user installed themselves are preserved. The record also notes that PAIR ran the installer, which is what lets a reinstall re-adopt an engine whose vendor owns its location: LM Studio writes ~/.lmstudio, so the path is no evidence of who put it there. A record that exists but cannot be parsed fails an uninstall rather than reporting a clean one. Windows persists through HKCU\Environment; the other platforms append a self-identifying block to the login shell's profiles. Both run under a cross-process lock that covers the whole read-modify-write, bounded so a hung peer times out instead of wedging the operation, and both publish through a temporary file and an atomic rename. A profile rewrite re-checks the file's fingerprint before renaming, so a concurrent editor's save is refused rather than silently discarded. The platform uninstallers release the entries before deleting the data directory that holds the records, and keep that data when the release fails so a reinstall can finish the cleanup. Two changes are adaptations to this tree rather than part of the feature. runCommand takes a variadic environment for install commands that declare overrides, so the launch closure in lifecycle.go accepts and ignores it; a launch carries its own environment separately. writeJSONAtomic keeps this tree's unique temporary name and gains the file flush and directory flush that the ownership records need, since a rename alone says nothing about bytes reaching stable storage. Signed-off-by: Chris Kelsey --- desktop/electron-builder.config.ts | 11 +- desktop/scripts/build/installer.nsh | 50 +- desktop/scripts/build/linux/before-remove.sh | 61 ++ desktop/scripts/build/macos/uninstall.sh | 39 + docs/engine-lifecycle.mdx | 17 + docs/getting-started.mdx | 60 +- docs/known-issues.mdx | 7 - scripts/wipe-app-data.ps1 | 10 + scripts/wipe-app-data.sh | 10 + services/nvpair-engine-manager/MANIFEST.md | 15 + services/nvpair-engine-manager/README.md | 49 +- .../nvpair-engine-manager/controlstream.go | 4 +- services/nvpair-engine-manager/e2e_test.go | 21 +- services/nvpair-engine-manager/executor.go | 9 + .../nvpair-engine-manager/executor_test.go | 6 +- services/nvpair-engine-manager/install.go | 155 +++- .../install_path_test.go | 352 +++++++++ services/nvpair-engine-manager/lifecycle.go | 6 +- services/nvpair-engine-manager/live_test.go | 37 +- services/nvpair-engine-manager/main.go | 24 + .../manifests/lmstudio.json | 1 + .../nvpair-engine-manager/pathlock_unix.go | 28 + .../nvpair-engine-manager/pathlock_windows.go | 32 + .../nvpair-engine-manager/pathownership.go | 312 ++++++++ .../pathownership_test.go | 668 ++++++++++++++++++ services/nvpair-engine-manager/registry.go | 26 + services/nvpair-engine-manager/setport.go | 32 +- services/nvpair-engine-manager/spec.md | 23 +- .../nvpair-engine-manager/syncdir_unix.go | 37 + .../nvpair-engine-manager/syncdir_windows.go | 8 + .../testdata/fakeengine/main.go | 27 +- services/nvpair-engine-manager/userpath.go | 58 ++ .../nvpair-engine-manager/userpath_shell.go | 231 ++++++ .../nvpair-engine-manager/userpath_test.go | 238 +++++++ .../nvpair-engine-manager/userpath_unix.go | 222 ++++++ .../userpath_unix_test.go | 79 +++ .../nvpair-engine-manager/userpath_windows.go | 103 +++ .../userpath_windows_entries.go | 80 +++ 38 files changed, 3063 insertions(+), 85 deletions(-) create mode 100644 desktop/scripts/build/linux/before-remove.sh create mode 100644 services/nvpair-engine-manager/install_path_test.go create mode 100644 services/nvpair-engine-manager/pathlock_unix.go create mode 100644 services/nvpair-engine-manager/pathlock_windows.go create mode 100644 services/nvpair-engine-manager/pathownership.go create mode 100644 services/nvpair-engine-manager/pathownership_test.go create mode 100644 services/nvpair-engine-manager/syncdir_unix.go create mode 100644 services/nvpair-engine-manager/syncdir_windows.go create mode 100644 services/nvpair-engine-manager/userpath.go create mode 100644 services/nvpair-engine-manager/userpath_shell.go create mode 100644 services/nvpair-engine-manager/userpath_test.go create mode 100644 services/nvpair-engine-manager/userpath_unix.go create mode 100644 services/nvpair-engine-manager/userpath_unix_test.go create mode 100644 services/nvpair-engine-manager/userpath_windows.go create mode 100644 services/nvpair-engine-manager/userpath_windows_entries.go diff --git a/desktop/electron-builder.config.ts b/desktop/electron-builder.config.ts index 1f713d47..612bb7be 100644 --- a/desktop/electron-builder.config.ts +++ b/desktop/electron-builder.config.ts @@ -10,6 +10,7 @@ // signing and notarization live outside this repository, so anything built here // is unsigned. Released builds come from NVIDIA's own signed pipeline. import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' import type { Configuration } from 'electron-builder' import electronPkg from 'electron/package.json' import pkg from './package.json' @@ -390,7 +391,15 @@ const config: Configuration = { }, deb: { afterInstall: 'scripts/build/linux/after-install.sh', - afterRemove: 'scripts/build/linux/after-remove.sh' + afterRemove: 'scripts/build/linux/after-remove.sh', + // prerm has no dedicated option, so it goes through the raw fpm passthrough. + // It has to be prerm rather than postrm because dpkg deletes the package's + // files before postrm runs, and the PATH cleanup runs a binary from /opt — + // see before-remove.sh. Unlike afterInstall/afterRemove, fpm arguments are + // forwarded verbatim: no ${macro} expansion, and the path is resolved + // against fpm's working directory rather than this file, so pass an + // absolute one. + fpm: [`--before-remove=${join(__dirname, 'scripts/build/linux/before-remove.sh')}`] }, mac: { executableName: APP_EXECUTABLE_NAME, diff --git a/desktop/scripts/build/installer.nsh b/desktop/scripts/build/installer.nsh index 1517393e..eba4a41f 100644 --- a/desktop/scripts/build/installer.nsh +++ b/desktop/scripts/build/installer.nsh @@ -78,6 +78,45 @@ ClearErrors !macroend +; Release the PATH entries this user's engines own, before the data directory +; that records them is deleted. +; +; Engine-manager writes its ownership receipts under the data root +; (engine-bin\engine-path\), while the entries themselves live in +; HKCU\Environment\Path — outside everything pairRemoveUserData touches. Deleting +; the receipts first would strand those entries with no record left to remove +; them by, pointing at engine directories this uninstall is about to delete. +; +; Runs the shipped binary rather than editing the registry here, so one +; implementation owns the format and the ownership rules. It must run before the +; template's RMDir /r $INSTDIR, which is why this lives in customUnInstall. +; nsExec::ExecToLog never aborts the uninstaller. +; +; The exit code is kept in $7 because the caller has to act on it. A failed +; release is not a no-op: the binary removes what it can and reports the rest, +; so some entries may be gone and some may remain — and the records that could +; still identify the remaining ones are inside the data root the caller is about +; to delete. +; +; The child inherits this process's environment and user, so it resolves the +; same profile pairRemoveUserData does. An elevated uninstall authenticated as a +; different administrator targets that account instead — the same limitation the +; data removal above already has. +!macro pairReleaseEnginePathEntries + DetailPrint "Releasing engine PATH entries..." + nsExec::ExecToLog '"$INSTDIR\resources\cli-bin\nvpair-engine-manager.exe" --remove-user-path' + Pop $7 +!macroend + +; The release failed, so the ownership records are the only thing that can still +; identify the entries it left behind. Deleting the data root would strand them +; permanently — the outcome the whole ordering above exists to avoid — so the +; data stays and the user is told why. A reinstall retries the cleanup. +!macro pairWarnPathEntriesRemain + DetailPrint "Could not release every engine PATH entry; keeping user data so a reinstall can retry." + MessageBox MB_OK|MB_ICONEXCLAMATION "Personal AI Router could not remove every engine entry from your PATH.$\n$\nYour data has been kept so that reinstalling can finish the cleanup. If you delete it by hand, remove those PATH entries yourself as well — nothing else will be able to identify them." /SD IDOK +!macroend + ; Best-effort: when the user opts to remove data, stop any process whose ; executable lives UNDER one of the data roots (e.g. an engine like Ollama ; running from %LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\) @@ -314,9 +353,14 @@ pairDataDone: ${endif} ${if} $8 == "1" - !insertmacro pairKillProcessesInDataDirs - !insertmacro pairRemoveUserData - !insertmacro pairWarnIfDataRemains + !insertmacro pairReleaseEnginePathEntries + ${if} $7 == "0" + !insertmacro pairKillProcessesInDataDirs + !insertmacro pairRemoveUserData + !insertmacro pairWarnIfDataRemains + ${else} + !insertmacro pairWarnPathEntriesRemain + ${endif} ${endif} ${endif} !macroend diff --git a/desktop/scripts/build/linux/before-remove.sh b/desktop/scripts/build/linux/before-remove.sh new file mode 100644 index 00000000..cfbfaa1c --- /dev/null +++ b/desktop/scripts/build/linux/before-remove.sh @@ -0,0 +1,61 @@ +#!/bin/bash +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# Personal AI Router Debian pre-remove. Releases the PATH entries this user's +# engines own. No `set -e`, and a trailing `exit 0`, so a best-effort step never +# fails the removal. +# +# Unlike after-install.sh and after-remove.sh, this one reaches fpm through the +# raw passthrough in electron-builder.config.ts, so ${macro} is NOT expanded +# here. It finds its own paths at runtime instead. +# +# WHY prerm AND NOT postrm. Engine-manager records what it added to PATH under +# the user's data root (engine-bin/engine-path/), while the entries themselves +# live in the login shell's profiles, which no maintainer script touches. +# after-remove handles the data root on `purge` -- but dpkg deletes the +# package's files before postrm runs, so by then the binary that understands +# those records is gone. prerm is the last point at which both still exist. +# +# This runs on `apt remove` as well as `apt purge`, so a plain remove gives up +# its PATH entries even though it keeps the engines. That is the recoverable +# direction: the drain releases each entry but keeps the record's note that PAIR +# installed the engine, so a reinstall re-adopts it and republishes. Deleting +# the record outright is what made this unrecoverable for an engine whose CLI +# lives outside PAIR's install directory -- nothing else tells it apart from an +# engine the user installed. The alternative -- entries no tool can identify, +# pointing into a directory a later purge deletes -- is worse either way. +# +# dpkg calls prerm with "upgrade" during an update, and "failed-upgrade" when +# recovering from one. Those keep the installation, so leave PATH alone. +case "${1:-}" in + upgrade|failed-upgrade) + exit 0 + ;; +esac + +# Ask dpkg where it put the binary rather than rebuilding /opt//... +# from a name this script cannot be told at build time. +command -v dpkg-query >/dev/null 2>&1 || exit 0 +engine_manager="$(dpkg-query -L "${DPKG_MAINTSCRIPT_PACKAGE:-}" 2>/dev/null \ + | grep -E '/cli-bin/nvpair-engine-manager$' | head -n 1)" +[ -n "$engine_manager" ] && [ -x "$engine_manager" ] || exit 0 + +# prerm runs as root, while the records, the dotfiles, and $XDG_CONFIG_HOME all +# belong to the user who ran the app. Resolve that user the same way +# after-remove resolves the data root on purge, and run as them so the binary +# reads the environment it wrote under. Best-effort: on a multi-user box, other +# users' entries are left for their own reinstall to reclaim. +real_user="${SUDO_USER:-}" +if [ -z "$real_user" ] && command -v logname >/dev/null 2>&1; then + real_user="$(logname 2>/dev/null || true)" +fi +[ -n "$real_user" ] && [ "$real_user" != root ] || exit 0 + +if command -v runuser >/dev/null 2>&1; then + runuser -u "$real_user" -- "$engine_manager" --remove-user-path >/dev/null 2>&1 || true +else + su -s /bin/sh -c "'$engine_manager' --remove-user-path" "$real_user" >/dev/null 2>&1 || true +fi + +exit 0 diff --git a/desktop/scripts/build/macos/uninstall.sh b/desktop/scripts/build/macos/uninstall.sh index ec948cc8..585531a7 100644 --- a/desktop/scripts/build/macos/uninstall.sh +++ b/desktop/scripts/build/macos/uninstall.sh @@ -98,6 +98,45 @@ if [ -x "$CTL" ]; then fi fi +# Release the PATH entries this user's engines own, while the binary that owns +# the ownership records still exists. +# +# Engine-manager records what it added under the data root +# (engine-bin/engine-path/), while the entries themselves live in the login +# shell's profiles — which the purge below never touches. Removing the records +# first would strand those entries with no way left to identify them, pointing +# at engine directories this script is about to delete. Only needed when data is +# going away: keeping it keeps the engines, the records, and a reinstall's +# ability to clean up later. +# +# Runs as the invoking user for the same reason as the helper above: the +# profiles and the records are theirs, not root's. +# +# Unlike every other step here, a failure is not shrugged off. The binary +# removes what it can and reports the rest, and the records that could still +# identify whatever it left are inside the data root the purge is about to +# delete — so a failed release cancels the purge rather than making those +# entries unidentifiable. The app bundle still goes; a reinstall retries. +if [ "$PURGE_DATA" = "1" ]; then + EM="$APP_PATH/Contents/Resources/cli-bin/nvpair-engine-manager" + if [ -x "$EM" ]; then + echo "Releasing engine PATH entries..." + # `|| released=$?` rather than a bare call: set -e is on, so a failure would + # otherwise abort before the check below could keep the data. + released=0 + if [ -n "$real_user" ] && [ "$real_user" != "root" ]; then + sudo -u "$real_user" "$EM" --remove-user-path >/dev/null 2>&1 || released=$? + else + "$EM" --remove-user-path >/dev/null 2>&1 || released=$? + fi + if [ "$released" -ne 0 ]; then + echo "Warning: could not release every engine PATH entry." >&2 + echo "Keeping user data so a reinstall can finish the cleanup; re-run with --purge afterwards." >&2 + PURGE_DATA=0 + fi + fi +fi + echo "Removing $APP_PATH ..." rm -rf "$APP_PATH" 2>/dev/null || true diff --git a/docs/engine-lifecycle.mdx b/docs/engine-lifecycle.mdx index 0d492277..8131a89f 100644 --- a/docs/engine-lifecycle.mdx +++ b/docs/engine-lifecycle.mdx @@ -70,6 +70,23 @@ Refer also to [Getting started](getting-started.mdx). You can install a supported engine during first-run setup or later from **Engine settings**. +PAIR automatically adds the installed engine's command-line tools (`ollama` or +`lms`) to your PATH. Open a new terminal after installation; on Windows, you may +need to restart your terminal app. + +PATH setup only ever changes the account that asked for the install. Installing +an engine on another node from this one leaves that node's PATH alone, and an +engine that was already on the machine before PAIR keeps whatever its own +installer set up. + +If PATH setup fails, PAIR reports it as a dismissible warning. The install still +succeeds, the engine still starts, and it works normally through its full path. + +On uninstall, PAIR removes only the PATH entries it recorded adding for an +engine it installed. Pre-existing entries and user-edited shell blocks are +preserved. Installations without an ownership record are left unchanged. +Uninstalling PAIR itself releases any entries its engines still own. + When you install an engine, consider the following: - Choosing install makes it an **NVPAIR-installed** engine. PAIR downloads it, diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index cb7e7e34..e975025a 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -351,51 +351,59 @@ current URL. ### Using an Engine's Own Command Line An engine PAIR installed for you is a normal installation, and you can drive it -with its own command-line interface (CLI). Two things make that less obvious: +with its own command-line interface (CLI). -- The binaries are not on your `PATH` yet. -- The engine is not on the port its CLI expects by default. +When PAIR installs an engine, it adds that engine's command directory to your +user `PATH`, so `ollama` or `lms` works by name. **Open a new terminal +afterwards** — a shell that was already running keeps the `PATH` it started with. +Uninstalling the engine removes the entry again. If PAIR could not update your +`PATH`, it tells you so and the engine still works; use the full path below. + +One thing still catches people out: the engine is not on the port its CLI expects +by default. Refer to [Set the port explicitly](#set-the-port-explicitly) below. **Ollama** is installed inside PAIR's own data directory: -| Platform | Path | -| --- | --- | -| Windows | `%LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe` | -| Linux | `~/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama/bin/ollama` | +```powershell +$env:OLLAMA_HOST = "127.0.0.1:11435" +ollama list +``` -On Linux it needs its bundled libraries on the library path: +On Linux, Ollama also needs its bundled libraries on the library path: ```bash ENGINE="$HOME/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama" -LD_LIBRARY_PATH="$ENGINE/lib/ollama" OLLAMA_HOST=127.0.0.1:11435 "$ENGINE/bin/ollama" list -``` - -```powershell -$ollama = "$env:LOCALAPPDATA\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe" -$env:OLLAMA_HOST = "127.0.0.1:11435" -& $ollama list +LD_LIBRARY_PATH="$ENGINE/lib/ollama" OLLAMA_HOST=127.0.0.1:11435 ollama list ``` **LM Studio** installs to its own standard location instead, because PAIR runs its -official installer: `~/.lmstudio/bin/lms`, or -`%USERPROFILE%\.lmstudio\bin\lms.exe` on Windows. +official installer: ```bash -~/.lmstudio/bin/lms status +lms status ``` -**Set the port explicitly.** Set `OLLAMA_HOST` to the engine's own port. -Otherwise, the CLI connects to `11434`, which is PAIR's proxy, and `ollama list` -returns the cluster's view instead of the local machine's. Use the **Server** -value under **Engine settings > Ports** to address the local engine directly. -It defaults to `11435` for Ollama and `1235` for LM Studio. +If you need the full path — because `PATH` setup failed, or you are scripting +against a specific installation — these are the locations: + +| Engine | Platform | Path | +| --- | --- | --- | +| Ollama | Windows | `%LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe` | +| Ollama | Linux | `~/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama/bin/ollama` | +| LM Studio | Windows | `%USERPROFILE%\.lmstudio\bin\lms.exe` | +| LM Studio | Linux, macOS | `~/.lmstudio/bin/lms` | + +#### Set the Port Explicitly + +Set `OLLAMA_HOST` to the engine's own port. Otherwise, the CLI connects to +`11434`, which is PAIR's proxy, and `ollama list` returns the cluster's view +instead of the local machine's. Use the **Server** value under +**Engine settings > Ports** to address the local engine directly. It defaults to +`11435` for Ollama and `1235` for LM Studio. Use the proxy port to check what your cluster can serve. Use the engine port to check what is installed on the local machine. -PAIR does not put these binaries on `PATH` yet. Until then, use the full path or -add an alias yourself. - ## Use PAIR with Existing Applications Any client that lets you set a base URL and a model name can use PAIR — Hermes, diff --git a/docs/known-issues.mdx b/docs/known-issues.mdx index 59aa0cdf..52827467 100644 --- a/docs/known-issues.mdx +++ b/docs/known-issues.mdx @@ -81,13 +81,6 @@ This matters most for a genuinely headless deployment, where those operations ha no other route on that machine. Refer to [What the Terminal Interface Cannot Do](terminal-interface.mdx#what-the-terminal-interface-cannot-do). -## Engine Binaries Are Not on Your PATH - -PAIR does not add the engines it installs to your `PATH`, so running an engine's -CLI means using the full path. Refer to -[Using an engine's own command line](getting-started.mdx#using-an-engines-own-command-line) -for the locations. This is on the roadmap. - ## Platform Limits Worth Knowing - **The Linux desktop installer is a `.deb` only.** On RPM-based distributions, diff --git a/scripts/wipe-app-data.ps1 b/scripts/wipe-app-data.ps1 index 4f2caeb7..0a958745 100644 --- a/scripts/wipe-app-data.ps1 +++ b/scripts/wipe-app-data.ps1 @@ -122,6 +122,16 @@ $Tmp = $env:TEMP if (-not $Tmp) { $Tmp = $env:TMP } $ControlDir = Join-Path $Tmp "nvpair-$Scope" +# The engine PATH entry in HKCU\Environment is deliberately NOT listed. Wiping +# the data root deletes engine-manager's ownership records along with the +# engines they describe, so nothing here could identify the entry to remove; the +# real uninstaller drains them first instead (desktop/scripts/build/installer.nsh). +# Recovery on the next install is partial, so prefer the real uninstaller. An +# engine PAIR installed into its own directory is recognized by the executable's +# location and reclaims the entry it finds already present. One whose vendor +# owns its location has no such evidence once the record is gone, so its entry +# is left orphaned until the user removes it. +# # Append-only target list. Never remove entries — only append. $Targets = @( @{ Path = $CurrentRoot; Reason = 'Current shared Electron + backend app data root' }, diff --git a/scripts/wipe-app-data.sh b/scripts/wipe-app-data.sh index 659393d2..b042268e 100755 --- a/scripts/wipe-app-data.sh +++ b/scripts/wipe-app-data.sh @@ -125,6 +125,16 @@ RUNTIME_BASE="${XDG_RUNTIME_DIR:-${TMPDIR:-/tmp}}" RUNTIME_BASE="${RUNTIME_BASE%/}" CONTROL_DIR="$RUNTIME_BASE/nvpair-$SCOPE" +# The engine PATH blocks in the user's shell profiles are deliberately NOT +# listed. Wiping the data root deletes engine-manager's ownership records along +# with the engines they describe, so nothing here could identify the blocks to +# remove; the real uninstallers drain them first instead (desktop/scripts/build). +# Recovery on the next install is partial, so prefer the real uninstallers. An +# engine PAIR installed into its own directory is recognized by the executable's +# location and re-adopts the block it finds already present. One whose vendor +# owns its location — LM Studio writes ~/.lmstudio — has no such evidence once +# the record is gone, so its block is left orphaned until the user removes it. +# # Append-only target list (path|reason). Never remove entries — only append. TARGETS=() TARGETS+=("$CURRENT_ROOT|Current shared Electron + backend app data root") diff --git a/services/nvpair-engine-manager/MANIFEST.md b/services/nvpair-engine-manager/MANIFEST.md index a5f13515..12b77c99 100644 --- a/services/nvpair-engine-manager/MANIFEST.md +++ b/services/nvpair-engine-manager/MANIFEST.md @@ -144,6 +144,21 @@ and recovery. Editing `args`/`start` directly remains trusted manifest authoring | `run` | string[] | no | Argv to execute after download (e.g. run the installer, extract the archive). Placeholders resolved; OS env refs expanded. Requires a `fetch` (the artifact it unpacks). | | `script` | string[] | no | **Escape hatch** for vendors that only ship a script installer. Runs **without** checksum verification (logged as unpinned) and replaces `fetch`+`run`. Prefer `fetch`+`run` whenever the vendor publishes a script or artifact: download it first, then execute the local file. **Make failures loud:** a piped bootstrap such as `curl … \| bash` can mask a failed fetch, while a separate fetch prevents the run and reports the error. | | `mode` | string | no | `"user"` (default) or `"admin"`. The runner **refuses** `"admin"` (engine-manager is user-mode only); it is a deliberate, flagged exception, not a default. | +| `env` | object | no | Literal `KEY: "value"` overrides layered onto the inherited environment of the **installer subprocess only** — not this service's environment and not the engine's runtime (that is `runtime.env`). Values are used verbatim: no placeholders, no OS env expansion. | + +`install.env` exists so a vendor quirk stays in the manifest. LM Studio's +installer edits the user's PATH unless `LMS_NO_MODIFY_PATH=1` is set, and PAIR +publishes that directory itself with an ownership record it can later remove — +two writers, one removable. Declaring it keeps the runner engine-agnostic, so a +third-party engine with the same quirk needs no Go change: + +```json +"install": { + "env": { "LMS_NO_MODIFY_PATH": "1" }, + "fetch": { "url": "https://lmstudio.ai/install.sh" }, + "run": ["bash", "{download}"] +} +``` ### Runtime diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index b401c883..9dc35fb5 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -53,6 +53,36 @@ Requests (caller → service): `EngineStatus` = `{ engine, display_name, installed, running, healthy, port }`. +A locally-initiated install publishes the engine's CLI directory on this user's +PATH. Windows updates `HKCU\Environment\Path`; Unix appends a block to the login +shell's profiles, read from the passwd database rather than the inherited +environment. Existing entries are preserved and repeated requests do not +duplicate PAIR's entries. New terminals pick up the change. + +`InstallForPeer` — the path a cluster peer's remote install takes — skips this +step entirely. A PATH failure is a dismissible warning, never an install error, +so the engine stays installed and the caller's optional start step still runs. + +The directory comes from the manifest's `runtime.cli`. An engine that declares +none falls back to its detected executable, and only from inside the directory +PAIR installed into: `detect` deliberately matches vendor layouts PAIR does not +own. An installation PAIR did not place gets no PATH entry and no warning. + +Ownership is recorded under `engine-bin/engine-path/.json` in the user +data directory, written before PATH is touched and guarded by a lock file in the +same directory so a concurrent engine-manager — `nvpair-tui` starts its own — +cannot clobber it. After a successful uninstall PAIR removes only recorded +entries or unchanged shell snippets; pre-existing entries and unrecorded +installations are preserved. Cleanup failures keep the receipt for a later +uninstall retry, even after the executable is gone. + +Those receipts live inside the tree the application uninstaller deletes, while +the PATH entries do not, so `--remove-user-path` drains every one of them and +exits. The uninstaller runs it before removing the data directory. + +The LM Studio installer runs with `LMS_NO_MODIFY_PATH=1`, declared as +`install.env` in its manifest, so every PATH change is one PAIR can remove. + Notifications (service → caller): `engine:ready{version}`, `engine:state-changed{EngineStatus}`, `engine:models-changed{engine, models}` — pushed when an engine's set of @@ -238,6 +268,8 @@ its own. | `--reserved-port ` | `0` (off) | Refuse local or remote engine starts and persisted port changes on a parent-owned proxy alias; the broker configures this from `OLLAMA_HOST` | | `--cluster-dir ` | _(none)_ | Cluster identity/pin directory; gates the `ec` surface on and supplies the leaf/pins used to serve it and to dial peers | | `--loaded-poll-interval ` | `5` | Seconds between loaded-model polls that drive `engine:models-changed`; `0` disables the watcher | +| `--user-path` | `true` | Publish an installed engine's CLI directory on the current user's PATH. `--user-path=false` leaves `HKCU\Environment` and the shell profiles alone; used by live tests so a run cannot edit the developer's own account | +| `--remove-user-path` | `false` | Release every PATH entry this user's engines own, then exit. Not a debug affordance: the Windows, macOS, and Debian uninstallers all invoke it before deleting the data directory that holds the ownership records, and gate that deletion on it succeeding | | `--log-level ` | _(env `NVPAIR_LOG_LEVEL` or `info`)_ | `debug` \| `info` \| `warn` \| `error` | | `--version` | | Print version and exit | @@ -285,9 +317,20 @@ with a loud warning. ## Cross-platform One binary compiles and runs on Windows, Linux, and macOS × amd64/arm64. -Per-OS variance lives in the manifest first; OS primitives (process -termination, console hiding) are the only build-tagged Go -(`proc_windows.go` / `proc_unix.go`). +Per-OS variance lives in the manifest first; the build-tagged Go is limited to +OS primitives: + +| Pair | Primitive | +|---|---| +| `proc_windows.go` / `proc_unix.go` | Process termination, console hiding | +| `userpath_windows.go` / `userpath_unix.go` | User PATH persistence — the registry vs. the login shell's profiles | +| `pathlock_windows.go` / `pathlock_unix.go` | Cross-process file locking | +| `syncdir_windows.go` / `syncdir_unix.go` | Flushing a directory entry after a rename | + +`userpath.go` states the two-function contract those PATH pairs implement. +`userpath_shell.go` and `userpath_windows_entries.go` are deliberately +**untagged** so the profile-editing and PATH-parsing rules stay testable on +every platform, not only on the one CI runs. ## Shutdown diff --git a/services/nvpair-engine-manager/controlstream.go b/services/nvpair-engine-manager/controlstream.go index 13bd584d..f4b554dd 100644 --- a/services/nvpair-engine-manager/controlstream.go +++ b/services/nvpair-engine-manager/controlstream.go @@ -78,7 +78,9 @@ func (s *controlServer) handleInstall(w http.ResponseWriter, r *http.Request) { return } s.streamOp(w, r, req.OpID, req.Engine, "install", func(ctx context.Context) (streamFrame, error) { - if err := s.exec.Install(ctx, req.Engine); err != nil { + // A pinned peer may install here; it may not rewrite this user's login + // shell configuration. See Executor.InstallForPeer. + if err := s.exec.InstallForPeer(ctx, req.Engine); err != nil { return streamFrame{}, err } if req.Start { diff --git a/services/nvpair-engine-manager/e2e_test.go b/services/nvpair-engine-manager/e2e_test.go index e02187a0..bcd3853c 100644 --- a/services/nvpair-engine-manager/e2e_test.go +++ b/services/nvpair-engine-manager/e2e_test.go @@ -7,6 +7,7 @@ import ( "bufio" "encoding/json" "io" + "maps" settings "nvpair-shared/enginesettings" "os" "os/exec" @@ -34,7 +35,7 @@ func TestE2EOverStdio(t *testing.T) { } cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) + cmd.Env = sandboxedEnv(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -244,7 +245,7 @@ type e2eManager struct { func startE2EManager(t *testing.T, cfg, home string) *e2eManager { t.Helper() cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) + cmd.Env = sandboxedEnv(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -308,6 +309,22 @@ func writeE2EManifest(t *testing.T, dir string, manifest *Manifest) { } } +// sandboxedEnv isolates a spawned manager from the developer's own account. +// +// Installing an engine publishes its CLI directory on the user's PATH, which +// means writing the profiles under HOME and choosing them from SHELL. Without +// this, an opt-in live run would append a block to the real ~/.zshrc pointing at +// a directory the test deletes on the way out — and any t.Fatalf before the +// uninstall step would leave it there for good. Explicit values win, so a caller +// that already points HOME at a temporary directory keeps its own. +func sandboxedEnv(t *testing.T, over map[string]string) []string { + t.Helper() + sandbox := t.TempDir() + env := map[string]string{"HOME": sandbox, "USERPROFILE": sandbox, "SHELL": "/bin/sh"} + maps.Copy(env, over) + return overrideEnv(env) +} + // overrideEnv returns the current environment with the given keys // replaced (case-insensitively, for Windows %AppData%). func overrideEnv(over map[string]string) []string { diff --git a/services/nvpair-engine-manager/executor.go b/services/nvpair-engine-manager/executor.go index de6bcfdd..fd444e9a 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -93,6 +93,9 @@ type Executor struct { // detectTimeout bounds the post-install/uninstall detect poll // (installers finish their file work asynchronously). Overridable. detectTimeout time.Duration + // addToPath persists the CLI directory in the current user's PATH. + addToPath func(string, *pathReceipt, func() error) error + removeFromPath func(*pathReceipt) error // actionTimeout bounds a single engine:action call (HTTP or CLI) so a // hung engine can't park the goroutine or starve the caller forever. actionTimeout time.Duration @@ -124,6 +127,8 @@ func NewExecutor(reg *Registry, reporter *Reporter, emit func(string, any), base baseDir: baseDir, desired: newDesiredStateStore(baseDir), detectTimeout: 30 * time.Second, + addToPath: addToUserPath, + removeFromPath: removeUserPath, actionTimeout: 30 * time.Minute, loadedPollInterval: defaultLoadedPollSeconds * time.Second, loadedPoke: make(chan struct{}, 1), @@ -272,6 +277,10 @@ func expandPathForOS(s, goos string) string { func installFailedID(engine string) string { return "engine-manager:install-failed:" + engine } +// pathFailedID is deliberately separate from install-failed: the engine did +// install, so the two states have to be able to coexist and clear independently. +func pathFailedID(engine string) string { return "engine-manager:path-failed:" + engine } + func uninstallFailedID(engine string) string { return "engine-manager:uninstall-failed:" + engine } func pullFailedID(engine, model string) string { diff --git a/services/nvpair-engine-manager/executor_test.go b/services/nvpair-engine-manager/executor_test.go index 40ebe249..c719a9ff 100644 --- a/services/nvpair-engine-manager/executor_test.go +++ b/services/nvpair-engine-manager/executor_test.go @@ -56,7 +56,10 @@ func newTestExecutor(t *testing.T, m *Manifest) *Executor { t.Helper() reg := NewRegistry() reg.engines[m.Engine] = m - return NewExecutor(reg, NewReporter(nil), func(string, any) {}, t.TempDir()) + ex := NewExecutor(reg, NewReporter(nil), func(string, any) {}, t.TempDir()) + // Lifecycle tests must not modify the developer's persistent PATH. + ex.addToPath = func(string, *pathReceipt, func() error) error { return nil } + return ex } func responseHeaderTimeout(t *testing.T, client *http.Client) time.Duration { @@ -403,6 +406,7 @@ func TestInstallAdoptsExternalServiceWithoutDownloading(t *testing.T) { reg.engines[m.Engine] = m var methods []string ex := NewExecutor(reg, NewReporter(nil), func(method string, _ any) { methods = append(methods, method) }, t.TempDir()) + ex.addToPath = func(string, *pathReceipt, func() error) error { return nil } if err := ex.Install(context.Background(), m.Engine); err != nil { t.Fatalf("install should adopt the external service: %v", err) diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 268bd0b9..d38d0a5c 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -7,6 +7,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "fmt" "io" "log/slog" @@ -15,13 +16,31 @@ import ( "os" "os/exec" "path" + "path/filepath" "strings" "time" ) -// Install obtains the engine in user mode: download (checksum-verified) -// then run the declared command. No-op if already detected. +// Install obtains the engine in user mode for a request that originated on this +// machine: download (checksum-verified), run the declared command, then publish +// the CLI directory on this user's PATH. func (e *Executor) Install(ctx context.Context, engine string) error { + return e.install(ctx, engine, true) +} + +// InstallForPeer installs on behalf of a paired cluster node. +// +// It is the same install with the PATH step withheld. A peer may put an engine +// on this machine, but editing this user's login shell configuration and +// HKCU\Environment is a different kind of change: the pairing PIN is a +// convenience code, cluster membership is not proof of a vetted peer, and +// nothing in the remote-install flow tells the person sitting at this machine +// that their shell startup files are about to be rewritten. +func (e *Executor) InstallForPeer(ctx context.Context, engine string) error { + return e.install(ctx, engine, false) +} + +func (e *Executor) install(ctx context.Context, engine string, setUserPath bool) error { st, err := e.state(engine) if err != nil { return err @@ -29,6 +48,9 @@ func (e *Executor) Install(ctx context.Context, engine string) error { st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); ok { + if setUserPath { + e.installPath(engine, st, false) + } e.reporter.clear(installFailedID(engine)) e.emitInstallProgress(engine, "already-installed", 100) return nil @@ -65,6 +87,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { } vars := map[string]string{"install_dir": st.installDir} + installEnv := inst.environ() if len(inst.Script) > 0 { // Escape hatch: vendor-script install with no checksum. Logged @@ -79,7 +102,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { for i := range argv { argv[i] = expandPath(argv[i]) } - if err := e.runCommand(ctx, argv); err != nil { + if err := e.runCommand(ctx, argv, installEnv...); err != nil { werr := fmt.Errorf("script install failed: %w", err) e.reportInstallFailed(engine, werr) return werr @@ -106,7 +129,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { for i := range args { args[i] = expandPath(args[i]) } - if err := e.runCommand(ctx, args); err != nil { + if err := e.runCommand(ctx, args, installEnv...); err != nil { werr := fmt.Errorf("install command failed: %w", err) e.reportInstallFailed(engine, werr) return werr @@ -119,12 +142,92 @@ func (e *Executor) Install(ctx context.Context, engine string) error { e.reportInstallFailed(engine, err) return err } + if setUserPath { + e.installPath(engine, st, true) + } e.reporter.clear(installFailedID(engine)) e.emitInstallProgress(engine, "done", 100) e.emitState(engine) return nil } +// installPath publishes the engine's command-line directory on the user's PATH. +// +// A PATH problem is a warning, never an install failure. The engine is +// installed and fully usable without it, and the caller gates the optional +// start step on install succeeding — so failing here would leave a working +// engine stopped, carrying an error card, behind a retry that clears nothing. +func (e *Executor) installPath(engine string, st *engineState, installedNow bool) { + dir, err := e.updateInstallPath(engine, st, installedNow) + if err != nil { + e.reportPathFailed(engine, st.manifest.DisplayName, dir, err) + return + } + if dir == "" { + // An external install PAIR declined to touch. Nothing was published, so + // there is nothing to report cleared: clearing here would retract a + // standing warning about an entry still missing from the user's PATH. + return + } + e.reporter.clear(pathFailedID(engine)) +} + +// pathCLIDir resolves the directory to publish on PATH. +// +// The manifest's declared runtime.cli is authoritative. A detect entry is only +// a fallback and only from inside the engine's managed install directory: +// detect deliberately matches vendor installs PAIR does not own (Ollama's +// darwin list starts at /Applications/Ollama.app), and its first hit is a +// discovery result, not a statement about where the CLI lives. +func pathCLIDir(st *engineState) (string, error) { + var cli string + if declared := st.plat.Runtime.CLI; declared != "" { + // {install_dir} is a documented runtime placeholder that detect and + // runtime.bin both resolve, so PATH has to resolve it too. Without this + // a manifest written to MANIFEST.md produced a non-absolute path and + // published nothing but a warning. + resolved, err := resolvePlaceholders(declared, map[string]string{"install_dir": st.installDir}) + if err != nil { + return "", err + } + cli = expandPath(resolved) + } else { + if !managedCLI(st) { + return "", fmt.Errorf("its command-line executable is outside the directory PAIR installs into") + } + // Detect has already expanded binPath; expanding it again would corrupt + // a directory that legitimately contains a $ or %. + st.mu.Lock() + cli = st.binPath + st.mu.Unlock() + } + if !filepath.IsAbs(cli) { + // Anchoring this to the daemon's working directory would produce a PATH + // entry that means nothing; a manifest has to declare an absolute path. + return "", fmt.Errorf("the manifest resolves its command-line executable to a relative path") + } + // An unreadable executable is not an absent one. Collapsing the two told the + // user to reinstall over a permission or I/O error, which fails identically. + if _, err := os.Stat(cli); err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("its command-line executable was not found") + } + return "", fmt.Errorf("its command-line executable could not be read: %w", err) + } + return filepath.Dir(cli), nil +} + +// managedCLI reports whether the detected executable is one PAIR placed. An +// engine whose vendor installer owns its own location (LM Studio writes +// ~/.lmstudio) is never managed by this test, so a lost receipt there is +// indistinguishable from an external install and is left alone. +func managedCLI(st *engineState) bool { + st.mu.Lock() + bin := st.binPath + st.mu.Unlock() + return isManagedInstallPath(bin, st.installDir) +} + // Uninstall runs the manifest's uninstall command (user-mode), stopping // the engine first. No-op if the engine isn't currently detected. func (e *Executor) Uninstall(ctx context.Context, engine string) error { @@ -135,7 +238,10 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); !ok { - return e.setDesiredEnabled(engine, false) // already gone + // Joined rather than sequenced: this branch is the documented retry + // after the files are already gone, so a desired-state write that keeps + // failing must not be what makes PATH cleanup unreachable. + return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) } un := st.plat.Uninstall if un == nil || len(un.Run) == 0 { @@ -204,9 +310,11 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { st.mu.Lock() st.binPath = "" st.mu.Unlock() - e.reporter.clear(uninstallFailedID(engine)) e.emitState(engine) - return e.setDesiredEnabled(engine, false) + // The engine is gone either way, so both steps run and both errors travel. + // Short-circuiting here left PATH published and the previous attempt's + // error card standing whenever the desired-state write failed. + return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) } // maxDownloadBytes caps a single engine download (engine installers / @@ -309,11 +417,14 @@ func (e *Executor) download(ctx context.Context, engine string, f *Fetch) (strin // runCommand executes a manifest-declared argv (an install or uninstall // step), hiding the console window on Windows; on failure it returns the // combined output for diagnostics. -func (e *Executor) runCommand(ctx context.Context, argv []string) error { +func (e *Executor) runCommand(ctx context.Context, argv []string, env ...string) error { if len(argv) == 0 { return nil } cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) + if len(env) > 0 { + cmd.Env = append(os.Environ(), env...) + } configureSysProcAttr(cmd) // hide the console window on Windows out, err := cmd.CombinedOutput() if err != nil { @@ -322,6 +433,34 @@ func (e *Executor) runCommand(ctx context.Context, argv []string) error { return nil } +// reportPathFailed surfaces a PATH problem as a dismissible warning telling the +// user what to do instead. +// +// The reported text is streamed to a remote install's initiator and push-synced +// to cluster peers, so the home directory is folded back to "~" before it +// leaves. That keeps the account name off the wire while still naming the +// profile the user has to edit. The full paths stay in this node's log. +func (e *Executor) reportPathFailed(engine, displayName, dir string, err error) { + slog.Warn("could not add an engine's command-line tools to PATH", + "engine", engine, "directory", dir, "err", err) + message := fmt.Sprintf( + "%s is installed and ready, but its command-line tools could not be added to your PATH (%s). Run the engine using its full path, or add its directory to your PATH yourself — the service log names the directory.", + displayName, redactHome(err.Error())) + e.reporter.report(serviceError{ + ID: pathFailedID(engine), Message: message, + Severity: "warning", Action: "dismiss", EngineType: engine, Operation: "install", + }) +} + +// redactHome keeps the user's home directory out of text that leaves this node. +func redactHome(text string) string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return text + } + return strings.ReplaceAll(text, home, "~") +} + func (e *Executor) reportInstallFailed(engine string, err error) { e.notify("engine:install-progress", map[string]any{"engine": engine, "stage": "failed", "percent": -1, "error": err.Error()}) e.progress.publish(ProgressEvent{Engine: engine, Op: "install", Stage: "failed", Percent: -1, Message: err.Error()}) diff --git a/services/nvpair-engine-manager/install_path_test.go b/services/nvpair-engine-manager/install_path_test.go new file mode 100644 index 00000000..24171b1c --- /dev/null +++ b/services/nvpair-engine-manager/install_path_test.go @@ -0,0 +1,352 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// pathInstallExecutor builds an engine whose detect path and declared CLI are +// deliberately different files inside one managed install directory. Detect is a +// discovery hint that matches whatever layout a vendor ships; only runtime.cli +// names the directory PAIR is entitled to publish on PATH. +func pathInstallExecutor(t *testing.T, engine, mode string) (*Executor, string) { + t.Helper() + installDir := filepath.Join(t.TempDir(), "Engine Tools") + detected := filepath.Join(installDir, engine+exeExt()) + cli := filepath.Join(installDir, "bin", engine+exeExt()) + if err := os.MkdirAll(filepath.Dir(cli), 0o755); err != nil { + t.Fatal(err) + } + m := &Manifest{ + Engine: engine, DisplayName: engine, ManifestVersion: 1, + Platforms: map[string]Platform{runtime.GOOS + "/" + runtime.GOARCH: { + Detect: []string{detected}, + Install: &Install{Run: []string{fakeEngineBin, "touch", detected, cli}}, + Runtime: Runtime{Mode: mode, Bin: detected, CLI: cli}, + }}, + } + ex := newTestExecutor(t, m) + engineStateForTest(t, ex, engine).installDir = installDir + return ex, cli +} + +func engineStateForTest(t *testing.T, ex *Executor, engine string) *engineState { + t.Helper() + st, err := ex.state(engine) + if err != nil { + t.Fatal(err) + } + return st +} + +// pathWarning returns the reported PATH warning for an engine, or "". +func pathWarning(ex *Executor, engine string) string { + for _, e := range ex.reporter.snapshot() { + if e.ID == pathFailedID(engine) { + return e.Message + } + } + return "" +} + +func TestInstallAutomaticallyAddsCLIToPath(t *testing.T) { + test := func(name, engine, mode string) { + t.Run(name, func(t *testing.T) { + ex, cli := pathInstallExecutor(t, engine, mode) + var added []string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { + if !fileExists(cli) { + t.Error("PATH changed before CLI was installed") + } + added = append(added, dir) + return nil + } + if err := ex.Install(context.Background(), engine); err != nil { + t.Fatal(err) + } + if !fileExists(cli) { + t.Fatal("engine was not installed") + } + if len(added) != 1 || added[0] != filepath.Dir(cli) { + t.Fatalf("added directories = %v", added) + } + }) + } + test("Ollama CLI directory", "ollama", "process") + test("LM Studio CLI directory", "lmstudio", "command") +} + +// A PATH failure must not fail the install. Failing it would also skip the +// caller's start step, so a working engine would sit stopped behind an error. +func TestPathFailureStillCompletesInstallAndWarns(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + var announced EngineStatus + ex.emit = func(method string, params any) { + if method != "engine:state-changed" { + return + } + data, err := json.Marshal(params) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(data, &announced); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + return errors.New("profile is read only") + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatalf("install error = %v, want success with a PATH warning", err) + } + installed, err := ex.Detect("ollama") + if err != nil || !installed { + t.Fatalf("installed = %v, error = %v", installed, err) + } + if !announced.Installed { + t.Fatal("UI was not told the engine remains installed") + } + warning := pathWarning(ex, "ollama") + if !strings.Contains(warning, "profile is read only") || !strings.Contains(warning, "full path") { + t.Fatalf("PATH warning = %q, want the reason and the manual alternative", warning) + } + + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("retry added %q", added) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("retry left the warning in place: %q", warning) + } +} + +// The warning crosses the wire to a remote initiator and is push-synced to +// cluster peers, so it must not carry the user's home directory. +func TestPathWarningOmitsTheUsersHomeDirectory(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil || home == "" { + t.Skip("no home directory to redact") + } + ex, _ := pathInstallExecutor(t, "ollama", "process") + ex.addToPath = func(string, *pathReceipt, func() error) error { + return errors.New("open " + filepath.Join(home, ".zshrc") + ": permission denied") + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + warning := pathWarning(ex, "ollama") + if strings.Contains(warning, home) { + t.Fatalf("PATH warning leaked the home directory: %q", warning) + } + if !strings.Contains(warning, ".zshrc") { + t.Fatalf("PATH warning dropped the profile name: %q", warning) + } +} + +// A detect entry can match a vendor layout PAIR does not own, so it is never +// enough on its own to claim a PATH directory. PAIR having just installed the +// engine is what separates this from an ordinary pre-existing installation. +func TestPathRefusesAnUndeclaredCLIOutsideTheInstallDirectory(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + st.plat.Runtime.CLI = "" + st.installDir = t.TempDir() + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("claimed a PATH directory PAIR does not own") + return nil + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if !fileExists(cli) { + t.Fatal("engine was not installed") + } + if warning := pathWarning(ex, "ollama"); !strings.Contains(warning, "outside the directory PAIR installs into") { + t.Fatalf("PATH warning = %q", warning) + } +} + +// A user who already had the engine before PAIR gets no PATH entry and no +// warning: the vendor's installer owns that location and its PATH. +func TestPreexistingEngineProducesNoPathWarning(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + st.plat.Runtime.CLI = "" + st.installDir = t.TempDir() + for _, path := range append([]string{cli}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("vendor install"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("claimed a PATH directory PAIR does not own") + return nil + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("warned about a PATH entry PAIR was never going to add: %q", warning) + } +} + +// A relative path is meaningless as a PATH entry: it would resolve against the +// daemon's working directory, which is not where the engine lives. +func TestPathRejectsARelativeManifestCLI(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + t.Chdir(filepath.Dir(cli)) + st.plat.Runtime.CLI = filepath.Base(cli) + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("published a PATH entry from a relative manifest path") + return nil + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if warning := pathWarning(ex, "ollama"); !strings.Contains(warning, "relative path") { + t.Fatalf("PATH warning = %q", warning) + } +} + +// The runner applies whatever a manifest declares, so a third-party engine can +// suppress its installer's own PATH edits without a code change. +func TestInstallAppliesManifestDeclaredInstallerEnvironment(t *testing.T) { + t.Setenv("PAIR_TEST_INSTALL_ENV", "inherited") + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + record := filepath.Join(filepath.Dir(cli), "installer-env") + st.plat.Install.Env = map[string]string{"PAIR_TEST_INSTALL_ENV": "declared"} + st.plat.Install.Run = []string{fakeEngineBin, "write-env", "PAIR_TEST_INSTALL_ENV", record} + st.plat.Detect = []string{record} + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + value, err := os.ReadFile(record) + if err != nil { + t.Fatal(err) + } + if string(value) != "declared" { + t.Fatalf("installer saw %q, want the manifest override", value) + } + if os.Getenv("PAIR_TEST_INSTALL_ENV") != "inherited" { + t.Fatal("the override escaped into the manager's own environment") + } +} + +// The bundled LM Studio manifest — not a branch in the runner — is what keeps +// the vendor installer from writing PATH entries PAIR cannot clean up. +func TestBundledLMStudioManifestSuppressesVendorPathEdits(t *testing.T) { + reg := NewRegistry() + if err := reg.LoadFS(bundledManifests, "manifests"); err != nil { + t.Fatal(err) + } + m, ok := reg.Get("lmstudio") + if !ok { + t.Fatal("lmstudio manifest is not bundled") + } + for key, plat := range m.Platforms { + if plat.Install == nil { + continue + } + if plat.Install.Env["LMS_NO_MODIFY_PATH"] != "1" { + t.Errorf("%s install env = %v", key, plat.Install.Env) + } + } +} + +func TestFailedInstallDoesNotChangePath(t *testing.T) { + ex, _ := pathInstallExecutor(t, "ollama", "process") + st, err := ex.state("ollama") + if err != nil { + t.Fatal(err) + } + st.plat.Install.Run = []string{filepath.Join(t.TempDir(), "missing-installer")} + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("PATH changed after failed install") + return nil + } + if err := ex.Install(context.Background(), "ollama"); err == nil { + t.Fatal("expected install failure") + } +} + +func TestInstallRPCAutomaticallyAddsCLIToPath(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + var out bytes.Buffer + m := NewManager(NewCodec(&out), ex, nil) + id := json.RawMessage("1") + m.runOp(context.Background(), &Message{JSONRPC: "2.0", ID: &id, Method: "engine:install", + Params: json.RawMessage(`{"engine":"ollama"}`)}) + var response struct { + Result EngineStatus `json:"result"` + Error *json.RawMessage `json:"error"` + } + if err := json.Unmarshal(out.Bytes(), &response); err != nil { + t.Fatal(err) + } + if response.Error != nil { + t.Fatalf("install returned an error: %s", *response.Error) + } + if !response.Result.Installed { + t.Error("install reported the engine as not installed") + } + if added != filepath.Dir(cli) { + t.Errorf("published %q on PATH, want the CLI directory %q", added, filepath.Dir(cli)) + } +} + +// A pinned peer may install an engine here, but rewriting this user's login +// shell configuration is a different kind of change and stays local-only. +func TestRemoteInstallDoesNotChangeTheTargetUsersPath(t *testing.T) { + ex, cli := pathInstallExecutor(t, "lmstudio", "command") + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("a remote install changed the local user's PATH") + return nil + } + s := &controlServer{exec: ex} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, controlInstallPath, strings.NewReader(`{"opId":"path-test","engine":"lmstudio"}`)) + s.handleInstall(rec, req) + frames := decodeFrames(t, rec.Body.String()) + if len(frames) == 0 { + t.Fatal("missing install result") + } + last := frames[len(frames)-1] + if rec.Code != http.StatusOK { + t.Errorf("HTTP status = %d, want %d", rec.Code, http.StatusOK) + } + if last.Type != "result" { + t.Fatalf("terminal frame type = %q, want %q: %+v", last.Type, "result", last) + } + if last.Status == nil { + t.Fatal("the terminal result frame carried no status") + } + if !last.Status.Installed { + t.Error("the remote install reported the engine as not installed") + } + if !fileExists(cli) { + t.Fatal("remote install did not install the engine") + } + if _, err := os.Stat(ex.pathReceiptFile("lmstudio")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("remote install claimed PATH ownership: %v", err) + } +} diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index cea7aa16..753f52f5 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -328,7 +328,11 @@ func (e *Executor) bringUpCommand(ctx context.Context, st *engineState, engine s argv := append([]string{launch.Bin}, launch.Args...) run := e.runCommand if rt.LaunchArgs != nil || rt.LaunchEnv != nil || len(launch.Env) > 0 { - run = func(ctx context.Context, argv []string) error { + // The variadic environment is runCommand's, for install commands + // that declare overrides. A launch carries its own in launch.Env, + // so the sole call below passes none and this ignores the parameter + // rather than pretending to merge two sources. + run = func(ctx context.Context, argv []string, _ ...string) error { return runPrivateLaunchCommand(ctx, argv, launch.Env, launchDiagnosticArgs(rt)) } // A vendor command may spawn its daemon and then exit unsuccessfully. diff --git a/services/nvpair-engine-manager/live_test.go b/services/nvpair-engine-manager/live_test.go index 6b84db8d..c952f2b4 100644 --- a/services/nvpair-engine-manager/live_test.go +++ b/services/nvpair-engine-manager/live_test.go @@ -223,10 +223,15 @@ func TestLiveLMStudioCleanRoom(t *testing.T) { // startManager spawns the manager binary with env overrides and returns // a frame stream, its stdin, and a cleanup func. It logs every frame. +// +// These tests install real engines, so PATH publishing is switched off: +// HKCU\Environment is machine state no temporary directory can contain, and a +// live run that fails partway must not leave the developer's account pointing at +// the temp directory it installed into. sandboxedEnv covers the dotfile half. func startManager(t *testing.T, env map[string]string) (chan frame, io.WriteCloser, func()) { t.Helper() - cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(env) + cmd := exec.Command(managerBin, "--user-path=false") + cmd.Env = sandboxedEnv(t, env) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -270,20 +275,34 @@ func startManager(t *testing.T, env map[string]string) (chan frame, io.WriteClos return frames, stdin, cleanup } +// The vendor/product segments shared/appdir appends to the platform base +// directory, which is where the manager looks for override manifests. +const ( + appOrgDir = "Nvidia Corporation" + appProductDir = "Personal AI Router" +) + // startManagerWithManifest writes m into a temp config dir, then spawns // the manager pointed at it (so the override manifest shadows bundled). func startManagerWithManifest(t *testing.T, m Manifest) (chan frame, io.WriteCloser, func()) { t.Helper() - cfg := t.TempDir() - engdir := filepath.Join(cfg, configSubdir, "engines") - if err := os.MkdirAll(engdir, 0o755); err != nil { + cfg, home := t.TempDir(), t.TempDir() + data, err := json.MarshalIndent(m, "", " ") + if err != nil { t.Fatal(err) } - data, _ := json.MarshalIndent(m, "", " ") - if err := os.WriteFile(filepath.Join(engdir, m.Engine+".json"), data, 0o644); err != nil { - t.Fatal(err) + // appdir picks a different base per platform — %LocalAppData%, $XDG_CONFIG_HOME, + // or ~/Library/Application Support — so write to each rather than guess. + for _, base := range []string{cfg, filepath.Join(home, "Library", "Application Support")} { + engdir := filepath.Join(base, appOrgDir, appProductDir, "engines") + if err := os.MkdirAll(engdir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(engdir, m.Engine+".json"), data, 0o644); err != nil { + t.Fatal(err) + } } - return startManager(t, map[string]string{"APPDATA": cfg, "XDG_CONFIG_HOME": cfg}) + return startManager(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) } func sha256File(t *testing.T, path string) string { diff --git a/services/nvpair-engine-manager/main.go b/services/nvpair-engine-manager/main.go index 4a9b5765..fda77bdd 100644 --- a/services/nvpair-engine-manager/main.go +++ b/services/nvpair-engine-manager/main.go @@ -37,6 +37,11 @@ func main() { reservedPort := flag.Int("reserved-port", 0, "local engine port reserved by the parent proxy; 0 disables the reservation") clusterDir := flag.String("cluster-dir", "", "cluster identity/pin directory; when set and this node holds a cluster identity, the ec remote-control surface (--control-port) turns on with pin-based mTLS") loadedPollSec := flag.Int("loaded-poll-interval", defaultLoadedPollSeconds, "seconds between loaded-model polls that drive engine:models-changed pushes; 0 disables the watcher") + userPath := flag.Bool("user-path", true, "publish an installed engine's command-line directory on the current user's PATH; --user-path=false leaves HKCU\\Environment and the shell profiles alone") + // Not named removeUserPath: that is the package-level function this flag + // ultimately reaches, and shadowing it for the whole of main() sets a trap + // for the next person who needs the function here. + releaseUserPath := flag.Bool("remove-user-path", false, "release every PATH entry this user's engines own, then exit; the application uninstaller runs this before deleting the data directory that holds the ownership records") showVersion := flag.Bool("version", false, "print version and exit") resolveLevel := applog.RegisterFlag(nil, slog.LevelInfo) flag.Parse() @@ -48,6 +53,18 @@ func main() { applog.Init("nvpair-engine-manager", resolveLevel()) + if *releaseUserPath { + _, installBase := userPaths() + if installBase == "" { + log.Fatal("no user data directory: cannot locate the PATH ownership records") + } + if err := removeAllUserPaths(installBase); err != nil { + log.Fatalf("release user PATH entries: %v", err) + } + log.Print("released the PATH entries owned by this user's engines") + return + } + var transport io.ReadWriteCloser if *ipcPath != "" { conn, err := dialIPC(*ipcPath) @@ -90,6 +107,13 @@ func main() { // engine:set-port persists the chosen port as a manifest override in the // same per-user engines/ dir that buildRegistry overlays. exec.overrideDir = manifestDir + if !*userPath { + // The opt-in live tests drive a real install against a temporary home, + // so they must not reach the developer's registry value or dotfiles. + exec.addToPath = func(string, *pathReceipt, func() error) error { return nil } + exec.removeFromPath = func(*pathReceipt) error { return nil } + log.Print("user PATH updates are disabled (--user-path=false)") + } // Loaded-model watcher cadence. Integer seconds keeps parity with the other // flags, so the smallest positive interval is 1s; <=0 disables the watcher. if *loadedPollSec <= 0 { diff --git a/services/nvpair-engine-manager/manifests/lmstudio.json b/services/nvpair-engine-manager/manifests/lmstudio.json index 964a7cc8..5b110621 100644 --- a/services/nvpair-engine-manager/manifests/lmstudio.json +++ b/services/nvpair-engine-manager/manifests/lmstudio.json @@ -5,6 +5,7 @@ "manifest_version": 1, "detect": ["~/.lmstudio/bin/lms"], "install": { + "env": { "LMS_NO_MODIFY_PATH": "1" }, "fetch": { "url": "https://lmstudio.ai/install.sh" }, "run": ["bash", "{download}"], "mode": "user" diff --git a/services/nvpair-engine-manager/pathlock_unix.go b/services/nvpair-engine-manager/pathlock_unix.go new file mode 100644 index 00000000..09a25e83 --- /dev/null +++ b/services/nvpair-engine-manager/pathlock_unix.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "errors" + "os" + "syscall" +) + +// tryLockExclusive takes the lock if it is free and reports false without +// waiting if another process holds it. The lock is released by unlockExclusive +// or by the process exiting, so a crash mid-update cannot wedge the next one — +// but a hung peer can, which is why the caller bounds its own wait. +func tryLockExclusive(f *os.File) (bool, error) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if errors.Is(err, syscall.EWOULDBLOCK) { + return false, nil + } + return err == nil, err +} + +func unlockExclusive(f *os.File) error { + return syscall.Flock(int(f.Fd()), syscall.LOCK_UN) +} diff --git a/services/nvpair-engine-manager/pathlock_windows.go b/services/nvpair-engine-manager/pathlock_windows.go new file mode 100644 index 00000000..82fcf9d2 --- /dev/null +++ b/services/nvpair-engine-manager/pathlock_windows.go @@ -0,0 +1,32 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLockExclusive takes the lock if it is free and reports false without +// waiting if another process holds it. Windows releases a byte-range lock when +// the handle closes, including on an abnormal exit, so a crash mid-update +// cannot wedge the next one — but a hung peer can, which is why the caller +// bounds its own wait. +func tryLockExclusive(f *os.File) (bool, error) { + err := windows.LockFileEx( + windows.Handle(f.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, 1, 0, new(windows.Overlapped), + ) + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return false, nil + } + return err == nil, err +} + +func unlockExclusive(f *os.File) error { + return windows.UnlockFileEx(windows.Handle(f.Fd()), 0, 1, 0, new(windows.Overlapped)) +} diff --git a/services/nvpair-engine-manager/pathownership.go b/services/nvpair-engine-manager/pathownership.go new file mode 100644 index 00000000..d97a4c4f --- /dev/null +++ b/services/nvpair-engine-manager/pathownership.go @@ -0,0 +1,312 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" + "time" +) + +// A receipt is created only after PAIR installs an engine. PATH changes are +// recorded before writing them, so a failed write or process restart is retryable. +// Keep it outside the engine directory, which the vendor uninstaller deletes. +type pathReceipt struct { + // Dir is the directory PAIR published on the user's PATH. + Dir string `json:"dir,omitempty"` + WindowsEntry string `json:"windowsEntry,omitempty"` + ShellBlocks []pathBlock `json:"shellBlocks,omitempty"` + // Installed records that PAIR ran this engine's installer, which is a + // longer-lived fact than any individual PATH entry. It outlives the entries + // themselves: releasing PATH when the application is uninstalled keeps this + // flag, so a reinstall re-adopts an engine PAIR placed instead of mistaking + // it for one the user installed. Only an engine uninstall clears it, by + // deleting the whole record. + // + // The directory is not a usable substitute. An engine whose vendor installer + // owns its location — LM Studio writes ~/.lmstudio — is indistinguishable + // from an external install by path alone. + Installed bool `json:"installed,omitempty"` +} + +type pathBlock struct { + Profile string `json:"profile"` + Text string `json:"text"` +} + +// pathReceiptDir keeps ownership independent of the removable engine files. +func (e *Executor) pathReceiptDir() string { + return pathReceiptDir(e.baseDir) +} + +func pathReceiptDir(baseDir string) string { + return filepath.Join(baseDir, "engine-path") +} + +func (e *Executor) pathReceiptFile(engine string) string { + return filepath.Join(e.pathReceiptDir(), engine+".json") +} + +// errUnreadableReceipt marks a record that exists but cannot be parsed. +// +// The two directions need opposite things from it, so it is a distinct error +// rather than an empty record. Install must proceed: refusing would leave no +// way to clear the file short of the user finding and deleting it, and writing +// a fresh claim over it is both safe and self-repairing. Uninstall must not, +// because an empty record makes it skip the removal and then report the entries +// released — clearing the retry along with the warning, on evidence it never +// read. The entry outlives the only record that could identify it. +var errUnreadableReceipt = errors.New("PATH ownership record is unreadable") + +// loadPathReceipt returns an empty record for a file that is absent, and +// errUnreadableReceipt alongside one for a file that is present but corrupt. +// An absent record and an unreadable one are not the same fact, and callers +// that treat them alike report success they cannot back up. +func loadPathReceipt(file string) (*pathReceipt, error) { + data, err := os.ReadFile(file) + if errors.Is(err, os.ErrNotExist) { + return &pathReceipt{}, nil + } + if err != nil { + return nil, err + } + var receipt pathReceipt + if err := json.Unmarshal(data, &receipt); err != nil { + slog.Warn("PATH ownership record is unreadable", "file", file, "err", err) + return &pathReceipt{}, fmt.Errorf("%w: %s", errUnreadableReceipt, filepath.Base(file)) + } + return &receipt, nil +} + +// savePathReceipt atomically persists ownership before PATH is modified. +func savePathReceipt(file string, receipt *pathReceipt) error { + if err := os.MkdirAll(filepath.Dir(file), 0o700); err != nil { + return err + } + return writeJSONAtomic(file, receipt) +} + +// lockUserPath serializes a whole read-modify-write cycle — the receipt, the +// registry value or shell profiles, and the receipt again — within this process +// and across every process sharing the user's data directory. +// +// One mutex is not enough: nvpair-tui starts its own broker and its own +// engine-manager, so a terminal install can run concurrently with a desktop +// install against the same HKCU\Environment\Path, the same dotfiles, and the +// same receipts. Unsynchronized, the second write discards the first, and two +// shell installs can append duplicate blocks with only one removable record. +func lockUserPath(baseDir string) (func(), error) { + userPathMu.Lock() + release := func() { userPathMu.Unlock() } + dir := pathReceiptDir(baseDir) + if err := os.MkdirAll(dir, 0o700); err != nil { + release() + return nil, err + } + f, err := os.OpenFile(filepath.Join(dir, "lock"), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + release() + return nil, err + } + if err := awaitExclusive(f, userPathLockTimeout); err != nil { + _ = f.Close() + release() + return nil, err + } + return func() { + _ = unlockExclusive(f) + _ = f.Close() + release() + }, nil +} + +// userPathLockTimeout bounds the wait for another process's PATH update. +// +// The engine's operation mutex is held across this wait, so waiting forever +// turns one wedged peer — a network home directory that stops responding, not +// a crash, which releases the lock — into every later install and uninstall +// for that engine hanging behind it. PATH work already degrades to a warning +// everywhere else, and the shell probes are bounded for the same reason. +const userPathLockTimeout = 10 * time.Second + +// awaitExclusive polls rather than blocking so the wait has an end. The timeout +// is a parameter rather than a package variable the test can reassign: the +// suite runs under -race, and a global swapped around a goroutine that reads it +// is a data race waiting for the one run where the timing lines up. +func awaitExclusive(f *os.File, timeout time.Duration) error { + for deadline := time.Now().Add(timeout); ; { + locked, err := tryLockExclusive(f) + if err != nil { + return err + } + if locked { + return nil + } + if time.Now().After(deadline) { + return fmt.Errorf("another process has held the PATH lock for over %s", timeout) + } + time.Sleep(50 * time.Millisecond) + } +} + +// pairOwnsInstall reports whether PAIR placed the engine already on disk, which +// is what entitles it to publish a PATH entry and later withdraw one. +// +// Any one of three facts establishes it: +// +// - a recorded directory, the ordinary case; +// - the Installed flag, which outlives the application uninstaller releasing +// every entry, so a reinstall re-adopts what PAIR placed; +// - an executable inside the engine's managed install directory, which covers +// a first install whose receipt never landed. +// +// The last is not sufficient on its own. An engine whose vendor installer owns +// its location never satisfies it, so testing only that made a lost record +// permanent for exactly the engine whose vendor PATH edit PAIR suppresses. +func pairOwnsInstall(receipt *pathReceipt, st *engineState) bool { + return receipt.Dir != "" || receipt.Installed || managedCLI(st) +} + +// updateInstallPath records fresh PAIR installs and retries only owned installs. +// It returns the directory it resolved so a failure can be logged against it. +func (e *Executor) updateInstallPath(engine string, st *engineState, installedNow bool) (string, error) { + unlock, err := lockUserPath(e.baseDir) + if err != nil { + return "", fmt.Errorf("lock PATH ownership: %w", err) + } + defer unlock() + file := e.pathReceiptFile(engine) + receipt, err := loadPathReceipt(file) + // A corrupt record is no claim for install's purposes: the fresh one written + // below replaces it, which is the only way the file ever gets repaired. + if err != nil && !errors.Is(err, errUnreadableReceipt) { + return "", fmt.Errorf("read PATH ownership: %w", err) + } + if !installedNow && !pairOwnsInstall(receipt, st) { + // An external installation is not ours to modify, and not ours to + // complain about either — the vendor's own installer handles its PATH. + return "", nil + } + // Resolved after the external check so a vendor layout PAIR cannot publish + // never produces a warning about a directory PAIR was never going to touch. + dir, err := pathCLIDir(st) + if err != nil { + return "", err + } + if receipt.Dir != "" && receipt.Dir != dir { + // The CLI moved — a manifest update, or a vendor that relocated it. + // Reaching here means PAIR owns the recorded entry, so release it before + // claiming the new one. Leaving it would point the user at a directory + // that no longer holds the engine, with the record still claiming PAIR + // put it there. + if err := e.removeFromPath(receipt); err != nil { + return dir, err + } + receipt = &pathReceipt{} + } + receipt.Dir = dir + receipt.Installed = true + save := func() error { return savePathReceipt(file, receipt) } + if err := save(); err != nil { + return dir, fmt.Errorf("save PATH ownership: %w", err) + } + return dir, e.addToPath(dir, receipt, save) +} + +// uninstallPath keeps the receipt on failure so cleanup can be retried even +// when the executable has already been removed by the vendor uninstaller. +func (e *Executor) uninstallPath(engine string) error { + unlock, lockErr := lockUserPath(e.baseDir) + if lockErr != nil { + return fmt.Errorf("lock PATH ownership: %w", lockErr) + } + defer unlock() + file := e.pathReceiptFile(engine) + // An unreadable record is a failure here, not an absent claim. Skipping the + // removal and reporting the entries released would strand them and clear the + // retry that is the only route back. + receipt, err := loadPathReceipt(file) + if err == nil && receipt.Dir != "" { + err = e.removeFromPath(receipt) + if err == nil { + err = os.Remove(file) + } + } + if err != nil { + err = fmt.Errorf("%s is uninstalled, but its PATH entries could not be cleaned up: %w", engine, err) + // redactHome for the same reason the install side does it: nvpair-errors + // push-syncs this message to every peer, and the wrapped error carries + // the absolute path of a file in this user's home directory. + e.reporter.report(serviceError{ID: uninstallFailedID(engine), Message: redactHome(err.Error()), Severity: "error", Action: "retry", EngineType: engine, Operation: "uninstall"}) + return err + } + e.reporter.clear(uninstallFailedID(engine)) + e.reporter.clear(pathFailedID(engine)) + return nil +} + +// removeAllUserPaths releases every PATH entry the engines under baseDir own. +// +// The receipts live inside the data directory the application uninstaller +// deletes, and nothing in HKCU\Environment or the user's dotfiles is under it. +// Without this step, uninstalling PAIR — or uninstalling it without first +// uninstalling each engine — orphans those entries with no record left to +// remove them by. +func removeAllUserPaths(baseDir string) error { + return drainUserPaths(baseDir, removeUserPath) +} + +// drainUserPaths takes the remover as a parameter so the platform the tests run +// on does not decide which half of a receipt they can check. +func drainUserPaths(baseDir string, remove func(*pathReceipt) error) error { + unlock, err := lockUserPath(baseDir) + if err != nil { + return err + } + defer unlock() + dir := pathReceiptDir(baseDir) + entries, err := os.ReadDir(dir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + var failures []error + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { + continue + } + file := filepath.Join(dir, entry.Name()) + receipt, err := loadPathReceipt(file) + if err != nil { + failures = append(failures, err) + continue + } + if err := remove(receipt); err != nil { + failures = append(failures, err) + continue + } + // The entries are gone; the installation is not. Retaining the Installed + // flag is what lets a later reinstall re-adopt an engine PAIR placed. + // Deleting the record outright made that unrecoverable for an engine + // whose CLI lives outside the managed install directory, because nothing + // else distinguishes it from one the user installed. + if receipt.Installed { + if err := savePathReceipt(file, &pathReceipt{Installed: true}); err != nil { + failures = append(failures, err) + } + continue + } + if err := os.Remove(file); err != nil { + failures = append(failures, err) + } + } + return errors.Join(failures...) +} diff --git a/services/nvpair-engine-manager/pathownership_test.go b/services/nvpair-engine-manager/pathownership_test.go new file mode 100644 index 00000000..befa7377 --- /dev/null +++ b/services/nvpair-engine-manager/pathownership_test.go @@ -0,0 +1,668 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func pathLifecycleExecutor(t *testing.T, engine, mode string) (*Executor, string, string) { + t.Helper() + ex, cli := pathInstallExecutor(t, engine, mode) + st := engineStateForTest(t, ex, engine) + // The uninstaller clears both the detected marker and the declared CLI, so + // the post-uninstall detect poll actually goes negative. + st.plat.Uninstall = &Uninstall{Run: append([]string{fakeEngineBin, "remove", cli}, st.plat.Detect...)} + platform := st.manifest.Platforms[hostKey()] + platform.Uninstall = st.plat.Uninstall + st.manifest.Platforms[hostKey()] = platform + home := t.TempDir() + ex.addToPath = func(dir string, receipt *pathReceipt, save func() error) error { + return addToShellPath(home, "sh", "", "", dir, receipt, save) + } + ex.removeFromPath = removeShellPath + return ex, cli, filepath.Join(home, ".profile") +} + +func TestUninstallRemovesOwnedPathAfterRestart(t *testing.T) { + test := func(engine, mode string) { + t.Run(engine, func(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, engine, mode) + original := "export PATH=\"$PATH:/user/tools\"\n" + if err := os.WriteFile(profile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + if err := ex.Install(context.Background(), engine); err != nil { + t.Fatal(err) + } + // A fresh executor must use the persisted ownership, not memory. + restarted := NewExecutor(ex.reg, NewReporter(nil), func(string, any) {}, ex.baseDir) + restarted.removeFromPath = removeShellPath + engineStateForTest(t, restarted, engine).installDir = engineStateForTest(t, ex, engine).installDir + if err := restarted.Uninstall(context.Background(), engine); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(data) != original { + t.Errorf("uninstall changed the user's profile to %q, want %q", data, original) + } + if fileExists(cli) { + t.Errorf("uninstall left the CLI behind at %q", cli) + } + if _, err := os.Stat(ex.pathReceiptFile(engine)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("ownership receipt still exists: %v", err) + } + }) + } + test("ollama", "process") + test("lmstudio", "command") +} + +func TestDetectedExternalEngineDoesNotAcquirePathOwnership(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "lmstudio", "command") + st := engineStateForTest(t, ex, "lmstudio") + // The vendor owns this location; PAIR never installed here. + st.installDir = t.TempDir() + for _, path := range append([]string{cli}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("external CLI"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("modified external engine PATH") + return nil + } + ex.removeFromPath = func(*pathReceipt) error { + t.Error("cleaned up an unowned PATH") + return nil + } + if err := ex.Install(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + if err := ex.Uninstall(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } +} + +// A receipt that never landed must not make the next install report success +// while silently leaving PATH alone. +func TestManagedInstallWithNoReceiptStillClaimsPath(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if err := os.Remove(ex.pathReceiptFile("ollama")); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + // Already detected, so this install short-circuits before any download. + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("added %q, want the lost ownership to be re-recorded for %q", added, filepath.Dir(cli)) + } +} + +// A manifest update or a vendor relocation moves the CLI. The recorded entry +// still belongs to PAIR, so it has to move too rather than being left pointing +// at a directory the engine is no longer in. +func TestMovedCLIMigratesTheOwnedPathEntry(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(before), filepath.Dir(cli)) { + t.Fatalf("install did not publish the original directory: %q", before) + } + + st := engineStateForTest(t, ex, "ollama") + moved := filepath.Join(st.installDir, "tools", filepath.Base(cli)) + if err := os.MkdirAll(filepath.Dir(moved), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Rename(cli, moved); err != nil { + t.Fatal(err) + } + st.plat.Runtime.CLI = moved + + // Already detected, so this is the short-circuit path. + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(after), filepath.Dir(cli)) { + t.Fatalf("stale entry survived the move: %q", after) + } + if !strings.Contains(string(after), filepath.Dir(moved)) { + t.Fatalf("new directory was not published: %q", after) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("unexpected warning: %q", warning) + } +} + +// Wiping PAIR's data directory deletes the receipts but leaves the profile +// blocks, so a reinstall has to re-adopt a block it demonstrably authored. +func TestReinstallReadoptsItsOwnOrphanedProfileBlock(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + orphaned, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(orphaned) == 0 { + t.Fatal("install wrote no PATH block") + } + // Simulate the data wipe: the receipts are inside the deleted tree, the + // shell profile is not. + if err := os.RemoveAll(ex.pathReceiptDir()); err != nil { + t.Fatal(err) + } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(orphaned) { + t.Fatalf("reinstall duplicated the block: %q", after) + } + if err := ex.Uninstall(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + remaining, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(remaining) != 0 { + t.Fatalf("uninstall left the re-adopted block behind: %q", remaining) + } +} + +// The application uninstaller runs this before deleting the data directory the +// receipts live in. +func TestDrainReleasesThePathBlock(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(contents) == 0 { + t.Fatal("install wrote no PATH block, so the drain would prove nothing") + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(after) != 0 { + t.Fatalf("drain left a PATH block: %q", after) + } +} + +// Draining releases the entries but not the knowledge that PAIR installed the +// engine. Deleting the record outright is what made a reinstall unable to +// republish PATH for an engine whose CLI lives outside PAIR's install +// directory, because nothing else distinguishes it from an external install. +func TestDrainKeepsTheInstalledFlagSoReinstallCanReadopt(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + receipt, err := loadPathReceipt(ex.pathReceiptFile("ollama")) + if err != nil { + t.Fatal(err) + } + if !receipt.Installed { + t.Error("drain dropped the installed flag, so a reinstall cannot re-adopt the engine") + } + if receipt.Dir != "" { + t.Errorf("drain kept a PATH claim it had already released: %q", receipt.Dir) + } + if len(receipt.ShellBlocks) != 0 { + t.Errorf("drain kept %d shell block claims it had already released", len(receipt.ShellBlocks)) + } +} + +// Draining an engine PAIR never installed leaves nothing behind at all. +func TestDrainRemovesARecordWithNoInstallBehindIt(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + file := ex.pathReceiptFile("ollama") + if err := savePathReceipt(file, &pathReceipt{Dir: t.TempDir()}); err != nil { + t.Fatal(err) + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(file); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("drain kept a record with no installation behind it: %v", err) + } +} + +// LM Studio installs into ~/.lmstudio, outside the directory PAIR installs +// into, so an executable-location test can never recognize it as PAIR's. The +// record has to carry that fact instead: the application uninstaller releases +// every PATH entry while leaving the engine installed, and without the retained +// flag a reinstall reads the engine as an external install and never republishes +// its CLI — permanently, because PAIR also suppresses the vendor's own PATH edit. +func TestReinstallRepublishesPathForAnEngineOutsideTheInstallDirectory(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "lmstudio", "command") + engineStateForTest(t, ex, "lmstudio").installDir = t.TempDir() + if err := ex.Install(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + // The application uninstaller: release the entries, leave the engine. + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("reinstall published %q, want the engine's CLI directory %q", added, filepath.Dir(cli)) + } +} + +// Declining to touch an external install is not the same as succeeding at it. +// Clearing the warning there retracts a standing report about a CLI that is +// still missing from the user's PATH. +func TestSkippingAnExternalInstallLeavesTheWarningStanding(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "lmstudio", "command") + st := engineStateForTest(t, ex, "lmstudio") + st.installDir = t.TempDir() + for _, path := range append([]string{}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("external CLI"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.reporter.report(serviceError{ID: pathFailedID("lmstudio"), Message: "an earlier attempt failed", Severity: "warning"}) + if err := ex.Install(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + if pathWarning(ex, "lmstudio") == "" { + t.Error("a skipped external install cleared a warning it never addressed") + } +} + +// A record PAIR cannot parse is not a record of no claim. Reporting the entries +// released would strand them and clear the retry that is the only route back. +func TestUninstallWithAnUnreadableReceiptReportsFailure(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + file := ex.pathReceiptFile("ollama") + if err := os.WriteFile(file, []byte("{ this is not json"), 0o600); err != nil { + t.Fatal(err) + } + err := ex.Uninstall(context.Background(), "ollama") + if !errors.Is(err, errUnreadableReceipt) { + t.Fatalf("uninstall returned %v, want the unreadable record to surface", err) + } + if _, statErr := os.Stat(file); statErr != nil { + t.Errorf("uninstall deleted the record it could not read: %v", statErr) + } +} + +// The lock file is what carries this across processes: nvpair-tui starts its own +// broker and its own engine-manager against the same data directory, and the +// application uninstaller drains PATH from a third. Two independent handles +// stand in for those separate processes — an in-process mutex alone would let +// the second update overwrite the first. +func TestPathLockIsExclusiveAcrossHandles(t *testing.T) { + file := filepath.Join(t.TempDir(), "lock") + open := func() *os.File { + t.Helper() + f, err := os.OpenFile(file, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = f.Close() }) + return f + } + held, other := open(), open() + + locked, err := tryLockExclusive(held) + if err != nil { + t.Fatal(err) + } + if !locked { + t.Fatal("could not take a lock nothing else held") + } + locked, err = tryLockExclusive(other) + if err != nil { + t.Fatal(err) + } + if locked { + t.Error("a second handle took a lock the first already held") + } + if err := unlockExclusive(held); err != nil { + t.Fatal(err) + } + locked, err = tryLockExclusive(other) + if err != nil { + t.Fatal(err) + } + if !locked { + t.Error("the lock stayed held after being released") + } +} + +// The engine's operation mutex is held across this wait, so a peer that hangs +// rather than crashing must not park every later install behind it. +func TestPathLockWaitIsBounded(t *testing.T) { + baseDir := t.TempDir() + unlock, err := lockUserPath(baseDir) + if err != nil { + t.Fatal(err) + } + defer unlock() + + // A separate handle stands in for the second process; the in-process mutex + // is already held by this goroutine's lock, so the wait under test is the + // file lock's. + waited := make(chan error, 1) + go func() { + f, openErr := os.OpenFile(filepath.Join(pathReceiptDir(baseDir), "lock"), os.O_CREATE|os.O_RDWR, 0o600) + if openErr != nil { + waited <- openErr + return + } + defer f.Close() + waited <- awaitExclusive(f, 150*time.Millisecond) + }() + select { + case err := <-waited: + if err == nil { + t.Fatal("took a lock another handle held") + } + case <-time.After(5 * time.Second): + t.Fatal("the wait for a held lock never ended") + } +} + +// A corrupt receipt must not wedge install for that engine: the fresh claim +// written over it is the only way the file is ever repaired. +func TestUnreadableReceiptIsTreatedAsNoClaim(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(ex.pathReceiptFile("ollama"), []byte("{ truncated"), 0o600); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatalf("install refused to proceed past a corrupt receipt: %v", err) + } + if added != filepath.Dir(cli) { + t.Fatalf("added %q, want %q", added, filepath.Dir(cli)) + } + if err := ex.Uninstall(context.Background(), "ollama"); err != nil { + t.Fatalf("uninstall refused to proceed past a corrupt receipt: %v", err) + } +} + +func TestFailedUninstallPreservesPathOwnership(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama"); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + st, err := ex.state("ollama") + if err != nil { + t.Fatal(err) + } + // The command returns successfully but leaves the executable in place. + st.plat.Uninstall.Run = []string{fakeEngineBin, "echo", "still installed"} + ex.detectTimeout = time.Millisecond + if err := ex.Uninstall(context.Background(), "ollama"); err == nil { + t.Fatal("expected failed uninstall") + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("failed uninstall changed PATH") + } + if _, err := os.Stat(ex.pathReceiptFile("ollama")); err != nil { + t.Fatalf("lost ownership after failed uninstall: %v", err) + } +} + +func TestPathCleanupCanRetryAfterEngineRemoval(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, "lmstudio", "command") + if err := ex.Install(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + wantErr := errors.New("profile is read only") + ex.removeFromPath = func(*pathReceipt) error { return wantErr } + if err := ex.Uninstall(context.Background(), "lmstudio"); !errors.Is(err, wantErr) { + t.Fatalf("uninstall error = %v", err) + } + if fileExists(cli) { + t.Fatal("engine was not removed before cleanup") + } + ex.removeFromPath = removeShellPath + if err := ex.Uninstall(context.Background(), "lmstudio"); err != nil { + t.Fatal(err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(contents) != 0 { + t.Fatalf("retry left PATH block: %q", contents) + } +} + +// The published PATH and the recorded claim are asserted directly, not just the +// round trip through removal. A no-op addWindowsPath leaves next unchanged and +// the receipt empty, and removeWindowsPath returns an unclaimed PATH untouched — +// so every round trip here would still match its expectation with the append +// deleted entirely. +func TestWindowsPathOwnershipPreservesExistingEntries(t *testing.T) { + test := func(name, current, wantPublished, wantClaim, wantAfterCleanup string) { + t.Run(name, func(t *testing.T) { + receipt := &pathReceipt{} + expand := func(s string) string { return strings.ReplaceAll(s, "%ENGINE%", `C:\Engine`) } + next, err := addWindowsPath(current, `C:\Engine`, expand, receipt, func() error { return nil }) + if err != nil { + t.Fatal(err) + } + if next != wantPublished { + t.Errorf("published PATH = %q, want %q", next, wantPublished) + } + if receipt.WindowsEntry != wantClaim { + t.Errorf("recorded claim = %q, want %q", receipt.WindowsEntry, wantClaim) + } + if got := removeWindowsPath(next, receipt.WindowsEntry); got != wantAfterCleanup { + t.Errorf("after cleanup = %q, want %q", got, wantAfterCleanup) + } + }) + } + test("new entry", `C:\UserTools`, `C:\UserTools;C:\Engine`, `C:\Engine`, `C:\UserTools`) + // Appending after a trailing separator would turn a harmless trailing empty + // element into a searched interior one, so the separator is dropped instead. + test("trailing empty entry", `C:\UserTools;`, `C:\UserTools;C:\Engine`, `C:\Engine`, `C:\UserTools`) + // Already on PATH: nothing is appended, and nothing is claimed. Recording an + // entry the user put there would let uninstall delete it. + test("existing entry", `C:\Engine;C:\UserTools`, `C:\Engine;C:\UserTools`, "", `C:\Engine;C:\UserTools`) + test("existing case variant", `c:\ENGINE;C:\UserTools`, `c:\ENGINE;C:\UserTools`, "", `c:\ENGINE;C:\UserTools`) + test("existing environment reference", `%ENGINE%;C:\UserTools`, `%ENGINE%;C:\UserTools`, "", `%ENGINE%;C:\UserTools`) +} + +// Addition compares case-insensitively after normalizing, so removal has to as +// well: otherwise a PATH editor that re-cased or re-slashed the entry would make +// it unremovable, and uninstall would silently leave it pointing at a deleted +// directory. +func TestWindowsCleanupMatchesRespelledEntries(t *testing.T) { + test := func(name, current, want string) { + t.Run(name, func(t *testing.T) { + if got := removeWindowsPath(current, `C:\Engine`); got != want { + t.Fatalf("after cleanup = %q, want %q", got, want) + } + }) + } + test("recased", `C:\UserTools;c:\engine`, `C:\UserTools`) + test("trailing backslash", `C:\UserTools;C:\Engine\`, `C:\UserTools`) + test("quoted", `"C:\Engine";C:\UserTools`, `C:\UserTools`) + test("forward slashes", `C:/Engine;C:\UserTools`, `C:\UserTools`) +} + +// PAIR appends, and an installer that wants precedence prepends, so a duplicate +// is somebody else's copy in front of PAIR's. Take PAIR's and leave theirs +// where they put it, rather than silently demoting a directory another tool +// deliberately promoted. +func TestWindowsCleanupRemovesTheAppendedDuplicate(t *testing.T) { + current := `C:\Engine;C:\UserTools;C:\Engine` + if got, want := removeWindowsPath(current, `C:\Engine`), `C:\Engine;C:\UserTools`; got != want { + t.Fatalf("after cleanup = %q, want %q", got, want) + } +} + +func TestWindowsPathOwnershipSavedBeforeChangingPath(t *testing.T) { + wantErr := errors.New("cannot save receipt") + current := `C:\UserTools` + got, err := addWindowsPath(current, `C:\Engine`, func(s string) string { return s }, &pathReceipt{}, func() error { return wantErr }) + if !errors.Is(err, wantErr) || got != current { + t.Fatalf("PATH = %q, error = %v", got, err) + } +} + +func TestWindowsCleanupPreservesUserModifiedEntry(t *testing.T) { + current := `C:\UserTools;%ENGINE%` + if got := removeWindowsPath(current, `C:\Engine`); got != current { + t.Fatalf("modified user entry was removed: %q", got) + } +} + +func TestShellOwnershipSaveFailureLeavesProfileUntouched(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + const original = "# user configuration\n" + if err := os.WriteFile(profile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + wantErr := errors.New("cannot save ownership") + err := addToShellPath(home, "sh", "", "", "/engine/bin", &pathReceipt{}, func() error { return wantErr }) + if !errors.Is(err, wantErr) { + t.Fatalf("error = %v", err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(contents) != original { + t.Fatalf("profile changed before ownership was saved: %q", contents) + } +} + +// Once a user edits the block, PAIR can no longer prove it wrote what is there, +// so cleanup leaves it alone rather than guessing at the boundaries. +func TestShellCleanupPreservesAUserEditedBlock(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + if err := os.WriteFile(profile, []byte("export PATH=\"$PATH:/engine/bin\"\n"), 0o644); err != nil { + t.Fatal(err) + } + receipt := &pathReceipt{} + if err := addToShellPath(home, "sh", "", "", "/engine/bin", receipt, func() error { return nil }); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + before = []byte(strings.ReplaceAll(string(before), "PAIR engine", "user customized engine")) + if err := os.WriteFile(profile, before, 0o644); err != nil { + t.Fatal(err) + } + if err := removeShellPath(receipt); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("cleanup modified unowned profile content") + } +} + +// Finding the exact block already present means an earlier PAIR install wrote it +// and lost the receipt. Recording ownership anyway is what lets the next +// uninstall clean it up instead of orphaning it forever. +func TestShellAddAdoptsAnIdenticalBlockItDidNotRecord(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + const userLine = "export PATH=\"$PATH:/user/tools\"\n" + if err := os.WriteFile(profile, []byte(userLine), 0o644); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "sh", "", "", "/engine/bin"); err != nil { + t.Fatal(err) + } + orphaned, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + receipt := &pathReceipt{} + if err := addToShellPath(home, "sh", "", "", "/engine/bin", receipt, func() error { return nil }); err != nil { + t.Fatal(err) + } + readopted, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(readopted) != string(orphaned) { + t.Fatalf("adoption duplicated the block: %q", readopted) + } + if err := removeShellPath(receipt); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != userLine { + t.Fatalf("after cleanup = %q, want only the user's own line", after) + } +} diff --git a/services/nvpair-engine-manager/registry.go b/services/nvpair-engine-manager/registry.go index b6a79ad6..5d7b7a94 100644 --- a/services/nvpair-engine-manager/registry.go +++ b/services/nvpair-engine-manager/registry.go @@ -82,6 +82,32 @@ type Install struct { // an explicit, rarely-used exception the runner surfaces loudly // rather than silently escalating. Mode string `json:"mode,omitempty"` + // Env holds literal environment overrides for the installer subprocess + // only — not the manager's environment and not the engine's runtime. It + // keeps a vendor quirk (LM Studio's installer edits PATH unless + // LMS_NO_MODIFY_PATH is set) in the manifest, so the runner stays + // engine-agnostic and a third-party engine can declare the same thing + // without a code change. + Env map[string]string `json:"env,omitempty"` +} + +// environ renders Env as the KEY=VALUE overrides runCommand layers onto the +// inherited environment. Deterministically ordered so a failing install command +// reproduces identically. +func (i *Install) environ() []string { + if len(i.Env) == 0 { + return nil + } + keys := make([]string, 0, len(i.Env)) + for k := range i.Env { + keys = append(keys, k) + } + sort.Strings(keys) + out := make([]string, 0, len(keys)) + for _, k := range keys { + out = append(out, k+"="+i.Env[k]) + } + return out } // Uninstall removes a user-mode install by running the engine's own diff --git a/services/nvpair-engine-manager/setport.go b/services/nvpair-engine-manager/setport.go index 66f0b283..8cfeb945 100644 --- a/services/nvpair-engine-manager/setport.go +++ b/services/nvpair-engine-manager/setport.go @@ -218,28 +218,36 @@ func setOverrideObject(parent map[string]any, key string, object map[string]any) } // writeJSONAtomic marshals v and writes it to path via a tmp file + rename so -// a crash mid-write can't leave a truncated manifest behind. +// a crash mid-write can't leave a truncated manifest behind. The temporary file +// is flushed before the rename and the directory after it: rename makes the +// swap atomic for a concurrent reader, but on its own guarantees nothing about +// the bytes reaching stable storage, so a power loss could publish an empty file. func writeJSONAtomic(path string, v any) error { data, err := json.MarshalIndent(v, "", " ") if err != nil { return err } - file, err := os.CreateTemp(filepath.Dir(path), ".override-*.tmp") + // Named by file, not by caller: this also persists PATH ownership records, + // and a failed receipt write reported as "write override" sends the reader + // looking at port overrides. + name := filepath.Base(path) + file, err := os.CreateTemp(filepath.Dir(path), "."+name+"-*.tmp") if err != nil { - return fmt.Errorf("create override: %w", err) + return fmt.Errorf("write %s: %w", name, err) } tmp := file.Name() defer os.Remove(tmp) - if _, err := file.Write(data); err != nil { - _ = file.Close() - return fmt.Errorf("write override: %w", err) - } - if err := file.Close(); err != nil { - return fmt.Errorf("close override: %w", err) + // Closes the file on every path. 0600 is asserted by + // TestSettingsOverrideRestrictsExistingPermissions and is stated rather + // than inherited from CreateTemp, because the rename has to *restrict* a + // pre-existing group- or world-readable file, not merely avoid widening it. + // Every caller writes a per-user file: a launch environment that can hold + // credentials, a desired-state record, or a PATH ownership receipt. + if err := writeAndSync(file, data, 0o600); err != nil { + return fmt.Errorf("write %s: %w", name, err) } if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return fmt.Errorf("rename override: %w", err) + return fmt.Errorf("rename %s: %w", name, err) } - return nil + return syncDir(filepath.Dir(path)) } diff --git a/services/nvpair-engine-manager/spec.md b/services/nvpair-engine-manager/spec.md index ba2e6324..8afe3e92 100644 --- a/services/nvpair-engine-manager/spec.md +++ b/services/nvpair-engine-manager/spec.md @@ -33,11 +33,14 @@ A declarative, config-driven control plane for **local inference engines** (Olla - **The node's error list** — owned by `nvpair-errors`, which holds it as in-memory session state; this service only emits `errors:report` / `errors:clear`. ## 3. Key Use Cases -- **Install an engine, user-mode**: `engine:install {engine:"ollama"}` downloads the per-OS user-scoped package (Windows/Linux standalone archive extracted into a user dir; macOS app bundle — never an elevated `Setup.exe` or `curl | sh`), checksum-verifies, extracts, re-detects. +- **Install an engine, user-mode**: `engine:install {engine:"ollama"}` downloads the per-OS user-scoped package (Windows/Linux standalone archive extracted into a user dir; macOS app bundle — never an elevated `Setup.exe` or `curl | sh`), checksum-verifies, extracts, re-detects, then publishes the engine's CLI directory on this user's PATH. +- **Publish the CLI on PATH**: the directory comes from `runtime.cli`, or from the detected executable when the manifest declares none and that executable is inside the directory PAIR installed into. Ownership is recorded before the change, under a cross-process lock, so uninstall removes exactly what PAIR added. A remote install from a cluster peer (`InstallForPeer`) skips this step: editing the login shell of whoever is sitting at the target node is not something a paired peer decides. - **Run lifecycle**: `engine:start` / `engine:stop` / `engine:restart` / `engine:status`, with readiness and health probes against the engine's loopback port. - **Run a declared action**: `engine:action {engine, action, params}` → the manifest-declared HTTP call to the engine's loopback control API (e.g. `127.0.0.1:{port}/api/pull`). (Methods, notifications, and UI events all use the colon form `engine:*`, matching the POC UI and the `errors:*` notifications.) - **Onboard a new engine (no code)**: a vendor adds `engines/.json`; the generic runner exposes their lifecycle + actions immediately. -- **Edge case — already installed**: detect short-circuits install (idempotent). +- **Edge case — already installed**: detect short-circuits the download and the install command. The PATH step still runs, so an engine whose ownership record was lost — the data directory was wiped, or an earlier save failed — reacquires it instead of reporting success with nothing on PATH. Recovery does not depend on where the CLI lives: the record's `installed` flag survives the application uninstaller's release, and a wiped record is recovered from the executable's location only when that location is inside PAIR's install directory. It is idempotent: an entry PAIR already owns is left as it is. If the CLI has moved — a manifest update, or a vendor that relocated it — the recorded entry is released and the new directory claimed, so the stale one does not outlive the engine it pointed at. +- **Edge case — PATH setup fails**: reported as a dismissible warning, never an install failure. The engine is installed and usable through its full path, and the caller's optional start step still runs. +- **Edge case — engine already present before PAIR**: no PATH entry and no warning. Its own installer owns that location. - **Edge case — checksum mismatch**: install fails before `run`, is reported, and never executes an unverified payload. - **Edge case — readiness timeout / crash**: failed readiness returns to Stopped; a later crash flips health and is reported. @@ -112,8 +115,8 @@ Requests (caller → service): | `engine:get-installed` | — | `{ engines: [EngineStatus] }` | | `engine:describe` | `{ engine }` | the engine's manifest | | `engine:status` | `{ engine }` | `EngineStatus` | -| `engine:install` | `{ engine }` | `EngineStatus` (after install) | -| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal) | +| `engine:install` | `{ engine }` | `EngineStatus` (after install and PATH publication) | +| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal and PATH cleanup) | | `engine:start` | `{ engine }` | `EngineStatus` (after readiness) | | `engine:stop` | `{ engine }` | `EngineStatus` | | `engine:restart` | `{ engine }` | `EngineStatus` | @@ -158,14 +161,15 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t - **External**: engine vendors' download URLs (HTTPS; checksum-pinned when a `sha256` is set, else HTTPS-only with a warning); first-party `nvpair-shared/applog` (logging + `log/set-level`) and `nvpair-shared/clustertrust` (pin-based mTLS). The `nvpair-shared/errors` wire shape is mirrored locally with identical JSON tags. No third-party runtime services. ## 9. Data Ownership -- **Owned**: the in-memory engine registry (parsed manifests + per-engine runtime state) and per-engine log/error ring buffers — transient only. -- **Source of truth**: no — `nvpair-errors` owns the node's error list (in memory, for the session); model inventories belong to the engines; manifests on disk are authored elsewhere. -- **Storage**: in-memory; manifests read from the per-user data dir's `engines/*.json` (`%LocalAppData%\Nvidia Corporation\Personal AI Router` on Windows, `~/.config/Nvidia Corporation/Personal AI Router` on Linux, `~/Library/Application Support/Nvidia Corporation/Personal AI Router` on macOS) plus bundled `manifests/*.json`. No database. +- **Owned**: the in-memory engine registry (parsed manifests + per-engine runtime state) and per-engine log/error ring buffers — transient. Also the durable PATH ownership records under the per-user data dir's `engine-bin/engine-path/.json`, which are **not** transient: they are the only thing that can identify what PAIR added to the user's PATH, and they have to outlive both a restart and the engine's own files. +- **Source of truth**: yes, for what PAIR published on this user's PATH — nothing else records it. Otherwise no: `nvpair-errors` owns the node's error list (in memory, for the session); model inventories belong to the engines; manifests on disk are authored elsewhere. +- **Storage**: in-memory, plus the PATH records above. Manifests read from the per-user data dir's `engines/*.json` (`%LocalAppData%\Nvidia Corporation\Personal AI Router` on Windows, `~/.config/Nvidia Corporation/Personal AI Router` on Linux, `~/Library/Application Support/Nvidia Corporation/Personal AI Router` on macOS) plus bundled `manifests/*.json`. No database. +- **PATH record lifecycle**: written before the PATH change, so a crash mid-update is retryable. An engine uninstall deletes the record. The application uninstaller (`--remove-user-path`) releases the entries but **keeps** each record's note that PAIR installed the engine, so a reinstall re-adopts it — an engine whose CLI lives outside PAIR's install directory has no other evidence distinguishing it from one the user installed. A record that exists but cannot be parsed is a failure for uninstall, not an absent claim; treating the two alike reported the entries released while stranding them. ## 10. Design Constraints - **Performance**: control plane, not inference; sub-second RPCs except install (network-bound) and start (bounded by the readiness timeout). - **Scalability**: a handful of engines per node; one managed instance per engine in v1. -- **Reliability**: best-effort; readiness + health probes; automatic restart on crash is planned but **not yet implemented** (see §4); install is one-shot and idempotent (detect short-circuits). +- **Reliability**: best-effort; readiness + health probes; automatic restart on crash is planned but **not yet implemented** (see §4); install is one-shot and idempotent — detect short-circuits the download, and the PATH step re-runs but changes nothing it already owns. - **Security**: **user mode only — no admin/sudo at runtime** (escalation reserved for product install time); both optional LAN listeners terminate pin-based mTLS and reject unpinned peers — the read-only model-list listener (`em`) because a node's model inventory is cluster data, and the `ec` control listener because its routes are privileged; `em` additionally serves plaintext on loopback only, for this node's own scanner; engines bind loopback by default, but a manifest's `runtime.bind` may open an inference engine to the LAN (Ollama defaults to `0.0.0.0`, overridable per-call); downloads are HTTPS-only (plain HTTP only from loopback) and checksum-verified before execution when the manifest pins a `sha256` (an unpinned fetch is HTTPS-only with a loud warning, like a `script` install). - **Compliance**: no PII; payloads carry engine/model identifiers and error messages only. @@ -177,6 +181,9 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t ## 12. Failure Modes and Mitigations - **Download / checksum failure on install**: engine stays NotInstalled. → Fail fast before `run`; `engine:install-progress` error + `errors:report` (`engine-manager:install-failed:`); never execute an unverified payload. +- **PATH publication fails on install**: the engine is installed and usable, but not by name. → Never fails the install, so the caller's start step still runs; `errors:report` (`engine-manager:path-failed:`) as a dismissible **warning** naming the manual alternative, cleared by a later install or uninstall that succeeds. Reported text folds the home directory to `~`, because a remote install streams it to the initiator and `nvpair-errors` push-syncs it to peers. +- **PATH cleanup fails on uninstall**: the executable is gone but the entry remains. → The receipt is kept and `errors:report` (`engine-manager:uninstall-failed:`) offers `retry`; a repeat `engine:uninstall` on an already-removed engine runs cleanup alone. Uninstalling the application drains every remaining receipt through `--remove-user-path`, since they live in the directory that uninstall deletes and the PATH entries do not. +- **Corrupt or lost PATH ownership record**: neither install nor uninstall can wedge on it. → An unparseable receipt is logged and treated as no claim; a managed install with no receipt reacquires PATH rather than reporting a silent success. - **Engine fails its readiness probe**: start never reaches Running. → Bounded ready timeout → back to Stopped; structured error; no half-started state. - **Engine process crashes**: engine unavailable. → A watcher flips state, emits `engine:state-changed` + `errors:report` (`engine-manager:exited:`); the parent's supervisor reports if the *manager itself* dies. (Automatic restart is planned — see §4 — not yet implemented.) - **`nvpair-errors` unavailable**: failures absent from the node's error list. → The Broker no-ops the forward; local applog + ring buffers still hold everything. diff --git a/services/nvpair-engine-manager/syncdir_unix.go b/services/nvpair-engine-manager/syncdir_unix.go new file mode 100644 index 00000000..74c257c2 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_unix.go @@ -0,0 +1,37 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "errors" + "log/slog" + "os" + "syscall" +) + +// syncDir flushes a directory entry so a rename into it survives a crash. +// os.Rename is atomic with respect to readers, but nothing guarantees the new +// entry has reached stable storage; ext4's auto_da_alloc and APFS hide that, +// XFS, btrfs, and networked home directories do not. +// +// A filesystem that rejects the flush outright is not a failure to report. The +// rename has already committed by the time this runs, so the file on disk is +// correct; turning EINVAL into an error would surface a PATH warning on install +// or a cleanup error on uninstall for a write that succeeded. CIFS and 9p do +// exactly that — network home directories and WSL interop paths, which is the +// case the flush above is aimed at. +func syncDir(dir string) error { + f, err := os.Open(dir) + if err != nil { + return err + } + syncErr := f.Sync() + if errors.Is(syncErr, syscall.EINVAL) || errors.Is(syncErr, syscall.ENOTSUP) { + slog.Debug("filesystem does not support flushing a directory", "dir", dir, "err", syncErr) + syncErr = nil + } + return errors.Join(syncErr, f.Close()) +} diff --git a/services/nvpair-engine-manager/syncdir_windows.go b/services/nvpair-engine-manager/syncdir_windows.go new file mode 100644 index 00000000..906c86b0 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_windows.go @@ -0,0 +1,8 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +// syncDir is a no-op on Windows. There is no directory handle to flush: +// FlushFileBuffers rejects one, and NTFS journals the rename's metadata itself. +func syncDir(string) error { return nil } diff --git a/services/nvpair-engine-manager/testdata/fakeengine/main.go b/services/nvpair-engine-manager/testdata/fakeengine/main.go index 9f12ba82..af128e21 100644 --- a/services/nvpair-engine-manager/testdata/fakeengine/main.go +++ b/services/nvpair-engine-manager/testdata/fakeengine/main.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "os" + "path/filepath" "strconv" "strings" "sync" @@ -108,9 +109,29 @@ func main() { os.Exit(1) } return - case "touch": // write a marker file so a test can assert the command ran - if len(os.Args) > 2 { - _ = os.WriteFile(os.Args[2], []byte("ok"), 0o644) + case "remove": // delete every named file, standing in for an uninstaller + for _, path := range os.Args[2:] { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Fatal(err) + } + } + return + case "write-env": // record an environment variable's value, to prove the + // installer subprocess received a manifest-declared override + if len(os.Args) < 4 { + fmt.Fprintln(os.Stderr, "write-env: need ") + os.Exit(2) + } + if err := os.WriteFile(os.Args[3], []byte(os.Getenv(os.Args[2])), 0o644); err != nil { + log.Fatal(err) + } + return + case "touch": // write marker files so a test can assert the command ran + for _, path := range os.Args[2:] { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + log.Fatal(err) + } + _ = os.WriteFile(path, []byte("ok"), 0o644) } return case "echo": // print args to stdout so a cmd-action can capture output diff --git a/services/nvpair-engine-manager/userpath.go b/services/nvpair-engine-manager/userpath.go new file mode 100644 index 00000000..0f01a0c7 --- /dev/null +++ b/services/nvpair-engine-manager/userpath.go @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" +) + +// The OS-neutral entry point for user PATH changes, and the contract each +// platform implements. +// +// Two functions, both build-tagged, one pair per platform: +// +// persistUserPath(dir, receipt, save) error — publish dir on the user's PATH +// removeUserPath(receipt) error — withdraw what the receipt claims +// +// Windows implements them in userpath_windows.go against HKCU\Environment\Path; +// everything else in userpath_unix.go against the login shell's profiles. +// Both are always called with the user PATH lock held (see lockUserPath), which +// covers the whole read-modify-write, not just the write. +// +// Ownership rules the two share: +// +// - Record before writing, so a crash or a failed write is retryable. +// - Never claim an entry PAIR did not put there. Uninstall deletes exactly +// what the receipt names, so a wrong claim deletes a user's own entry. +// +// Where they legitimately differ: Windows records only when it actually +// appends, because an entry already present is indistinguishable from the +// user's own. Unix records even when the profile already holds a byte-identical +// block, because PAIR's block is self-identifying — it is demonstrably the +// author, and re-adopting is what lets a reinstall clean up after a lost +// receipt. +// +// A third platform implements the two functions and nothing else. The shell +// editing in userpath_shell.go and the Windows entry parsing in +// userpath_windows_entries.go are deliberately untagged so their rules stay +// testable everywhere, not because either is portable. + +// Both engines can finish installing together. Serialize user PATH updates +// across engines so one read/modify/write cannot discard the other's entry. +// This covers one process only; lockUserPath extends it across the several +// engine-managers a machine can run at once. +var userPathMu sync.Mutex + +// addToUserPath validates the directory before changing the current user's PATH. +// The caller holds the user PATH lock through both ownership and environment writes. +func addToUserPath(dir string, receipt *pathReceipt, save func() error) error { + if !filepath.IsAbs(dir) || strings.ContainsAny(dir, "\r\n\x00"+string(os.PathListSeparator)) { + return fmt.Errorf("invalid command-line directory %q", dir) + } + return persistUserPath(dir, receipt, save) +} diff --git a/services/nvpair-engine-manager/userpath_shell.go b/services/nvpair-engine-manager/userpath_shell.go new file mode 100644 index 00000000..f75afcc3 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_shell.go @@ -0,0 +1,231 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +// Keep profile editing independent of the host OS so tests can use temporary +// homes on every platform, without touching the developer's shell configuration. +func addToShellPath(home, shell, zdotdir, configHome, dir string, receipt *pathReceipt, save func() error) error { + quote := "'" + strings.ReplaceAll(dir, "'", "'\"'\"'") + "'" + block := "\n# PAIR engine command-line tools\ncase \":$PATH:\" in\n *:" + quote + ":*) ;;\n *) export PATH=\"${PATH:+$PATH:}\"" + quote + " ;;\nesac\n" + var profiles []string + switch filepath.Base(shell) { + case "zsh": + if zdotdir == "" { + zdotdir = home + } + profiles = []string{filepath.Join(zdotdir, ".zprofile"), filepath.Join(zdotdir, ".zshrc")} + case "bash": + login := filepath.Join(home, ".profile") + for _, name := range []string{".bash_profile", ".bash_login"} { + candidate := filepath.Join(home, name) + if _, err := os.Stat(candidate); err == nil { + login = candidate + break + } else if !errors.Is(err, os.ErrNotExist) { + return err + } + } + profiles = []string{login, filepath.Join(home, ".bashrc")} + case "fish": + if configHome == "" { + configHome = filepath.Join(home, ".config") + } + profiles = []string{filepath.Join(configHome, "fish", "conf.d", "nvpair-path.fish")} + quote = "'" + strings.NewReplacer(`\`, `\\`, "'", `\'`).Replace(dir) + "'" + block = "\n# PAIR engine command-line tools\nif not contains -- " + quote + " $PATH\n set -gx PATH $PATH " + quote + "\nend\n" + case "sh", "dash", "ksh", "ash", "busybox": + profiles = []string{filepath.Join(home, ".profile")} + default: + // tcsh, nushell, elvish, xonsh and friends each need their own syntax. + // The caller reports this as a warning with the directory to add, so an + // unrecognized shell costs the user one manual step, not the install. + return fmt.Errorf("automatic PATH setup is not supported for the %s shell", filepath.Base(shell)) + } + for _, profile := range profiles { + profile, err := filepath.Abs(profile) + if err != nil { + return err + } + if err := appendPathBlock(profile, block, receipt, save); err != nil { + return fmt.Errorf("update %s: %w", filepath.Base(profile), err) + } + } + return nil +} + +// appendPathBlock records the snippet as ours before writing it. +// +// Ownership is recorded even when the profile already carries a byte-identical +// block, because PAIR is demonstrably the author: a reinstall after the receipt +// was lost — wiping the data directory deletes it — must be able to re-adopt +// the block, or the next uninstall reports success while leaving it behind. +func appendPathBlock(profile, block string, receipt *pathReceipt, save func() error) error { + // Recorded before the profile is read rather than between reading it and + // rewriting it. save() is two fsyncs, and every one of them sat inside the + // window in which a concurrent editor's save to the same profile would be + // discarded by the rewrite below. + owned := pathBlock{Profile: profile, Text: block} + recorded := false + for _, previous := range receipt.ShellBlocks { + recorded = recorded || previous == owned + } + if !recorded { + receipt.ShellBlocks = append(receipt.ShellBlocks, owned) + if err := save(); err != nil { + return err + } + } + contents, before, err := readProfile(profile) + if err != nil { + return err + } + if strings.Contains(string(contents), block) { + return nil + } + return writeShellProfile(profile, append(contents, block...), before) +} + +// removeShellPath removes exact recorded snippets and preserves other content. +func removeShellPath(receipt *pathReceipt) error { + for _, block := range receipt.ShellBlocks { + contents, before, err := readProfile(block.Profile) + if err != nil { + // Named by basename: this message reaches cluster peers. + return fmt.Errorf("read %s: %w", filepath.Base(block.Profile), err) + } + // A user-edited block is no longer ours to delete. + if block.Text == "" || !strings.Contains(string(contents), block.Text) { + continue + } + next := strings.Replace(string(contents), block.Text, "", 1) + if err := writeShellProfile(block.Profile, []byte(next), before); err != nil { + return fmt.Errorf("update %s: %w", filepath.Base(block.Profile), err) + } + } + return nil +} + +// profileSnapshot fingerprints a profile so a rewrite can tell whether anything +// else changed it since it was read. +type profileSnapshot struct { + size int64 + modTime time.Time + absent bool +} + +func statProfile(profile string) (profileSnapshot, error) { + info, err := os.Stat(profile) + if errors.Is(err, os.ErrNotExist) { + return profileSnapshot{absent: true}, nil + } + if err != nil { + return profileSnapshot{}, err + } + return profileSnapshot{size: info.Size(), modTime: info.ModTime()}, nil +} + +// errProfileChanged reports that someone else wrote to the profile mid-update. +// Both callers surface it as a retryable failure, which is the right answer: +// the next attempt reads the user's new contents and appends to those. +var errProfileChanged = errors.New("the file changed while PAIR was updating it") + +// readProfile fingerprints a profile before reading it. Taking the stat first +// means a write that races the read fails the later comparison instead of +// passing it — the worst case is a spurious retry, never a silent overwrite. +func readProfile(profile string) ([]byte, profileSnapshot, error) { + before, err := statProfile(profile) + if err != nil { + return nil, profileSnapshot{}, err + } + contents, err := os.ReadFile(profile) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, profileSnapshot{}, err + } + return contents, before, nil +} + +// writeShellProfile replaces a profile in one step: write a sibling temporary +// file, flush it, then rename it over the original. Both directions go through +// here. An append cannot use O_APPEND, because a short write — ENOSPC, a quota — +// would leave the user a profile truncated mid-statement that every new shell +// then fails to parse, and that PAIR could neither match nor repair afterwards. +// The flush matters for the same reason: rename swaps the directory entry +// atomically but says nothing about the data reaching stable storage, and PAIR +// did not create these files so it has no copy to restore. +// +// Symlinks are resolved so a dotfile linked into a dotfiles repository is +// updated in place rather than replaced by a regular file. +// +// The cost is inode identity: the replacement carries the permission bits over +// but not ACLs, extended attributes, or additional hard links. That is accepted +// deliberately. Preserving them means writing through the original inode, which +// is exactly the short-write hazard above, and the alternative — copying each +// class of metadata — is platform-specific, incomplete, and guards a dotfile +// arrangement far rarer than a full disk. Do not turn this back into an +// in-place write. +// +// since is the fingerprint readProfile took, re-checked immediately before the +// rename. Replacing the whole file from a copy read earlier would discard an +// edit that landed in between, and the PAIR lock cannot prevent that one — the +// competing writer is the user's editor, not another engine-manager. +func writeShellProfile(profile string, contents []byte, since profileSnapshot) error { + target := profile + perm := os.FileMode(0o644) + switch resolved, err := filepath.EvalSymlinks(profile); { + case err == nil: + info, statErr := os.Stat(resolved) + if statErr != nil { + return statErr + } + target, perm = resolved, info.Mode().Perm() + case !errors.Is(err, os.ErrNotExist): + return err + } + dir := filepath.Dir(target) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + f, err := os.CreateTemp(dir, ".pair-path-*") + if err != nil { + return err + } + defer os.Remove(f.Name()) + if err := writeAndSync(f, contents, perm); err != nil { + return err + } + switch now, err := statProfile(profile); { + case err != nil: + return err + case now != since: + return fmt.Errorf("%w: %s", errProfileChanged, filepath.Base(profile)) + } + if err := os.Rename(f.Name(), target); err != nil { + return err + } + return syncDir(dir) +} + +// writeAndSync fills an open file and closes it durably, so the caller's rename +// publishes the bytes rather than an empty file. +func writeAndSync(f *os.File, contents []byte, perm os.FileMode) error { + if err := f.Chmod(perm); err != nil { + return errors.Join(err, f.Close()) + } + if _, err := f.Write(contents); err != nil { + return errors.Join(err, f.Close()) + } + if err := f.Sync(); err != nil { + return errors.Join(err, f.Close()) + } + return f.Close() +} diff --git a/services/nvpair-engine-manager/userpath_test.go b/services/nvpair-engine-manager/userpath_test.go new file mode 100644 index 00000000..e4db1b27 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_test.go @@ -0,0 +1,238 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestAppendWindowsPath(t *testing.T) { + expand := func(s string) string { return strings.ReplaceAll(s, "%USERPROFILE%", `C:\Users\Test`) } + test := func(name, current, dir, want string) { + t.Run(name, func(t *testing.T) { + if got := appendWindowsPath(current, dir, expand); got != want { + t.Fatalf("PATH = %q, want %q", got, want) + } + }) + } + test("first entry", "", `C:\Engine Tools`, `C:\Engine Tools`) + test("preserves order and references", `%USERPROFILE%\bin;C:\Tools`, `C:\Engine`, `%USERPROFILE%\bin;C:\Tools;C:\Engine`) + test("case insensitive existing directory", `C:\TOOLS\;D:\Other`, `c:\tools`, `C:\TOOLS\;D:\Other`) + test("existing expanded directory", `%USERPROFILE%\bin`, `C:\Users\Test\bin`, `%USERPROFILE%\bin`) + test("existing quoted directory", `"C:\Engine Tools"`, `C:\Engine Tools`, `"C:\Engine Tools"`) + // Windows ignores a trailing empty element but searches an interior one, and + // has historically resolved it against the current directory. + test("trailing separator", `C:\Tools;`, `C:\Engine`, `C:\Tools;C:\Engine`) + test("only separators", `;;`, `C:\Engine`, `C:\Engine`) + test("different directory with same prefix", `C:\Engine-old`, `C:\Engine`, `C:\Engine-old;C:\Engine`) +} + +func TestShellPathNamesAnUnsupportedShell(t *testing.T) { + err := testAddToShellPath(t.TempDir(), "/usr/local/bin/nu", "", "", "/opt/bin") + if err == nil || !strings.Contains(err.Error(), "nu") { + t.Fatalf("error = %v, want the shell named", err) + } +} + +// Every shell PAIR claims to support has to actually get a profile written. +func TestShellPathSupportsPosixShellFamilies(t *testing.T) { + for _, shell := range []string{"sh", "dash", "ksh", "ash", "busybox"} { + t.Run(shell, func(t *testing.T) { + home := t.TempDir() + if err := testAddToShellPath(home, "/bin/"+shell, "", "", "/opt/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(home, ".profile")); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestShellPathProfilesPreserveContentAndAreIdempotent(t *testing.T) { + test := func(name, shell string, profiles []string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + dir := "/opt/Engine Tools/bin" + const original = "# existing user configuration\n" + for _, profile := range profiles { + filename := filepath.Join(home, profile) + if err := os.MkdirAll(filepath.Dir(filename), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filename, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + } + if err := testAddToShellPath(home, shell, "", "", dir); err != nil { + t.Fatal(err) + } + first := make(map[string]string) + for _, profile := range profiles { + data, err := os.ReadFile(filepath.Join(home, profile)) + if err != nil { + t.Fatal(err) + } + first[profile] = string(data) + if !strings.HasPrefix(string(data), original) || !strings.Contains(string(data), dir) { + t.Fatalf("profile content = %q", data) + } + } + if err := testAddToShellPath(home, shell, "", "", dir); err != nil { + t.Fatal(err) + } + for _, profile := range profiles { + data, err := os.ReadFile(filepath.Join(home, profile)) + if err != nil { + t.Fatal(err) + } + if string(data) != first[profile] { + t.Fatalf("repeat changed %s", profile) + } + } + }) + } + test("bash login and interactive terminals", "/bin/bash", []string{".profile", ".bashrc"}) + test("bash honors existing login profile", "/bin/bash", []string{".bash_profile", ".bashrc"}) + test("zsh login and interactive terminals", "/bin/zsh", []string{".zprofile", ".zshrc"}) + test("fish configuration", "/usr/bin/fish", []string{filepath.Join(".config", "fish", "conf.d", "nvpair-path.fish")}) +} + +func TestShellPathUsesConfiguredProfileDirectories(t *testing.T) { + home, zdir, config := t.TempDir(), t.TempDir(), t.TempDir() + if err := testAddToShellPath(home, "zsh", zdir, "", "/opt/ollama/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(zdir, ".zshrc")); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "fish", "", config, "/opt/lms/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(config, "fish", "conf.d", "nvpair-path.fish")); err != nil { + t.Fatal(err) + } + entries, err := os.ReadDir(home) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("wrote to home instead of configured directories: %v", entries) + } +} + +func TestShellPathReportsUnwritableProfile(t *testing.T) { + home := t.TempDir() + if err := os.Mkdir(filepath.Join(home, ".profile"), 0o755); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "bash", "", "", "/opt/bin"); err == nil { + t.Fatal("expected a profile write failure") + } +} + +func TestShellPathPreservesLiteralDirectoriesAtRuntime(t *testing.T) { + shell, err := exec.LookPath("sh") + if err != nil { + t.Skip("POSIX shell unavailable") + } + test := func(name, dir string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + if err := testAddToShellPath(home, "sh", "", "", dir); err != nil { + t.Fatal(err) + } + profile, err := os.ReadFile(filepath.Join(home, ".profile")) + if err != nil { + t.Fatal(err) + } + // Source twice to check runtime deduplication, not just file deduplication. + command := "PATH=/existing\n" + string(profile) + string(profile) + "printf '%s' \"$PATH\"" + out, err := exec.Command(shell, "-c", command).CombinedOutput() + if err != nil { + t.Fatalf("source profile: %v: %s", err, out) + } + if string(out) != "/existing:"+dir { + t.Fatalf("PATH = %q, want literal directory %q appended once", out, dir) + } + }) + } + test("spaces", "/opt/Engine Tools/bin") + test("quotes and shell metacharacters", "/opt/it's $(printf injected) [engine]/bin") +} + +// Profile-only tests use temporary homes and do not need a persistent receipt. +func testAddToShellPath(home, shell, zdotdir, configHome, dir string) error { + return addToShellPath(home, shell, zdotdir, configHome, dir, &pathReceipt{}, func() error { return nil }) +} + +// A dotfile is often a symlink into a dotfiles repository. Replacing the link +// with a regular file would detach the user's profile from the repository that +// manages it, so the write has to go through to the target. +func TestShellProfileWriteFollowsASymlink(t *testing.T) { + home := t.TempDir() + store := t.TempDir() + target := filepath.Join(store, "profile") + if err := os.WriteFile(target, []byte("# managed elsewhere\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(home, ".profile") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if err := testAddToShellPath(home, "sh", "", "", "/opt/engine/bin"); err != nil { + t.Fatal(err) + } + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Error("the profile symlink was replaced by a regular file") + } + contents, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(contents), "/opt/engine/bin") { + t.Errorf("the symlink target was not updated: %q", contents) + } + if !strings.Contains(string(contents), "# managed elsewhere") { + t.Errorf("the symlink target lost its original contents: %q", contents) + } +} + +// The rewrite replaces the whole file from a copy read earlier, and the PAIR +// lock cannot cover the competing writer here — it is the user's editor. An +// edit that lands in the gap has to be noticed rather than discarded. +func TestShellProfileWriteRefusesToDiscardAConcurrentEdit(t *testing.T) { + profile := filepath.Join(t.TempDir(), ".profile") + if err := os.WriteFile(profile, []byte("original\n"), 0o644); err != nil { + t.Fatal(err) + } + contents, before, err := readProfile(profile) + if err != nil { + t.Fatal(err) + } + edited := "original\nadded by the user's editor\n" + if err := os.WriteFile(profile, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + err = writeShellProfile(profile, append(contents, "# PAIR\n"...), before) + if !errors.Is(err, errProfileChanged) { + t.Fatalf("write returned %v, want the concurrent edit to be refused", err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != edited { + t.Errorf("the user's edit was overwritten: %q", after) + } +} diff --git a/services/nvpair-engine-manager/userpath_unix.go b/services/nvpair-engine-manager/userpath_unix.go new file mode 100644 index 00000000..96b092ec --- /dev/null +++ b/services/nvpair-engine-manager/userpath_unix.go @@ -0,0 +1,222 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "bufio" + "context" + "errors" + "log/slog" + "os" + "os/exec" + "os/user" + "path/filepath" + "runtime" + "strings" + "time" +) + +// persistUserPath selects the installing user's shell configuration. +func persistUserPath(dir string, receipt *pathReceipt, save func() error) error { + home, err := os.UserHomeDir() + if err != nil { + return err + } + shell := loginShell() + // Resolved only for zsh. An unreadable .zshenv has to fail rather than fall + // back, and a bash user's install must not be the thing it fails. + zdotdir := "" + if filepath.Base(shell) == "zsh" { + if zdotdir, err = zshDotDir(home); err != nil { + return err + } + } + return addToShellPath(home, shell, zdotdir, os.Getenv("XDG_CONFIG_HOME"), dir, receipt, save) +} + +// removeUserPath uses recorded profiles, even if the user has changed shells. +func removeUserPath(receipt *pathReceipt) error { + return removeShellPath(receipt) +} + +// loginShell reports the shell the user actually logs in with, from the account +// database rather than $SHELL. +// +// Engine-manager is spawned by the broker, which the desktop application +// spawns, so its environment is the GUI session's. $SHELL is usually right +// there but is not guaranteed to be set at all, and writing to the wrong +// shell's profiles succeeds silently while leaving nothing on PATH. +func loginShell() string { + u, err := user.Current() + if err != nil { + slog.Debug("no account record for the current user", "err", err) + return environmentShell() + } + if shell := accountShell(u); shell != "" { + return shell + } + return environmentShell() +} + +// accountShell reads the login shell out of the passwd database. On Linux the +// file covers ordinary users; on macOS it holds only system accounts, so a real +// account has to come from Directory Services. +func accountShell(u *user.User) string { + if runtime.GOOS == "darwin" { + return directoryServiceShell(u.Username) + } + if shell := passwdFileShell("/etc/passwd", u.Username); shell != "" { + return shell + } + // NSS-backed accounts (LDAP, SSSD) are absent from the file. + getent, err := exec.LookPath("getent") + if err != nil { + return "" + } + return passwdEntryShell(probeAccountDatabase(getent, "passwd", u.Username), u.Username) +} + +func directoryServiceShell(username string) string { + out := probeAccountDatabase("/usr/bin/dscl", ".", "-read", "/Users/"+username, "UserShell") + _, value, ok := strings.Cut(out, ":") + if !ok { + return "" + } + return strings.TrimSpace(value) +} + +// accountProbeTimeout bounds a single account-database lookup. +// +// getent blocks on an unreachable LDAP or SSSD backend and dscl on a directory +// server that is not answering, both of which are ordinary on a laptop off the +// corporate network. This runs inside an install whose PATH step is supposed to +// degrade to a warning, so a hung probe has to become a miss rather than an +// install that never returns. +const accountProbeTimeout = 3 * time.Second + +// probeAccountDatabase returns the command's trimmed output, or "" if it fails +// or outlasts its budget. The caller falls back to the environment. +func probeAccountDatabase(name string, args ...string) string { + ctx, cancel := context.WithTimeout(context.Background(), accountProbeTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, name, args...) + // Killing the probe closes the pipe Output is reading only if nothing it + // spawned still holds the other end. WaitDelay caps that too, so the + // deadline is on returning rather than on the process alone. + cmd.WaitDelay = accountProbeTimeout + out, err := cmd.Output() + if err != nil { + slog.Debug("account database lookup failed", "command", name, "err", err) + return "" + } + return strings.TrimSpace(string(out)) +} + +func passwdFileShell(file, username string) string { + f, err := os.Open(file) + if err != nil { + slog.Debug("could not read the passwd database", "file", file, "err", err) + return "" + } + defer f.Close() + scanner := bufio.NewScanner(f) + for scanner.Scan() { + if shell := passwdEntryShell(scanner.Text(), username); shell != "" { + return shell + } + } + // "Not listed" and "stopped looking" are different facts. The caller falls + // back either way, but only one of them explains a profile written to the + // wrong shell, so it has to reach the log. + if err := scanner.Err(); err != nil { + slog.Debug("stopped reading the passwd database early", "file", file, "err", err) + } + return "" +} + +// passwdEntryShell returns field 7 of a passwd line belonging to username. +func passwdEntryShell(line, username string) string { + fields := strings.Split(line, ":") + if len(fields) < 7 || fields[0] != username { + return "" + } + return strings.TrimSpace(fields[6]) +} + +// environmentShell is the last resort when the account database is unreadable. +func environmentShell() string { + if shell := os.Getenv("SHELL"); shell != "" { + return shell + } + if runtime.GOOS == "darwin" { + return "zsh" + } + return "bash" +} + +// zshDotDir resolves where zsh looks for .zprofile and .zshrc. +// +// ZDOTDIR is conventionally assigned in ~/.zshenv, which zsh reads before +// anything else, so it is essentially never visible in this process's inherited +// environment. Missing it writes two stray dotfiles into $HOME that the user's +// zsh never reads, and reports success. +func zshDotDir(home string) (string, error) { + if dir := os.Getenv("ZDOTDIR"); dir != "" { + return dir, nil + } + return zshenvDotDir(home) +} + +// zshenvDotDir scans ~/.zshenv for a plain ZDOTDIR assignment. Only a literal +// value is honoured: anything built from a command substitution or a conditional +// cannot be evaluated here, and guessing at it would be worse than falling back +// to $HOME. +func zshenvDotDir(home string) (string, error) { + data, err := os.ReadFile(filepath.Join(home, ".zshenv")) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + if err != nil { + // Falling back to $HOME on a permission or I/O error would write two + // dotfiles the user's zsh never reads and report success — the exact + // outcome this function exists to prevent, on evidence it never got. + return "", err + } + found := "" + for _, line := range strings.Split(string(data), "\n") { + statement := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "export ")) + value, ok := strings.CutPrefix(statement, "ZDOTDIR=") + if !ok { + continue + } + value = strings.TrimSpace(value) + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { + value = value[1 : len(value)-1] + } else if cut := strings.IndexAny(value, " \t"); cut >= 0 { + // Unquoted, so zsh ends the word at the first space and the rest is + // a trailing comment. Without this, `ZDOTDIR=$HOME/.config/zsh # tidy` + // cleared every guard below and PAIR created a directory named + // `zsh # tidy`. + value = value[:cut] + } + value = expandHomePrefix(home, value) + if !filepath.IsAbs(value) || strings.ContainsAny(value, "$`(){}") { + continue + } + found = value // A later assignment wins, as it would in zsh. + } + return found, nil +} + +// expandHomePrefix resolves the only references a static read can resolve. +func expandHomePrefix(home, value string) string { + for _, prefix := range []string{"$HOME/", "${HOME}/", "~/"} { + if rest, ok := strings.CutPrefix(value, prefix); ok { + return filepath.Join(home, rest) + } + } + return value +} diff --git a/services/nvpair-engine-manager/userpath_unix_test.go b/services/nvpair-engine-manager/userpath_unix_test.go new file mode 100644 index 00000000..8d7531d8 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_unix_test.go @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// Getting ZDOTDIR wrong is not a cosmetic error: PAIR writes .zprofile and +// .zshrc into the directory this returns, so a wrong answer leaves two stray +// dotfiles the user's zsh never reads and reports the PATH entry published. +func TestZshenvDotDir(t *testing.T) { + test := func(name, zshenv, want string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + if err := os.WriteFile(filepath.Join(home, ".zshenv"), []byte(zshenv), 0o644); err != nil { + t.Fatal(err) + } + got, err := zshenvDotDir(home) + if err != nil { + t.Fatal(err) + } + if want == "$HOME/.config/zsh" { + want = filepath.Join(home, ".config", "zsh") + } + if got != want { + t.Errorf("ZDOTDIR = %q, want %q", got, want) + } + }) + } + test("absent assignment", "# nothing here\n", "") + test("plain assignment", "ZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + test("exported assignment", "export ZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + test("double quoted", "export ZDOTDIR=\"$HOME/.config/zsh\"\n", "$HOME/.config/zsh") + test("single quoted", "export ZDOTDIR='$HOME/.config/zsh'\n", "$HOME/.config/zsh") + // zsh ends an unquoted word at the first space, so the comment is not part + // of the directory. Treating it as one made PAIR create `zsh # keep tidy`. + test("trailing comment", "export ZDOTDIR=$HOME/.config/zsh # keep tidy\n", "$HOME/.config/zsh") + // A later assignment wins, as it would in zsh. + test("reassigned", "ZDOTDIR=$HOME/first\nZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + // Anything PAIR cannot evaluate the way zsh would is declined rather than + // guessed at: the fallback to $HOME is the safe answer. + test("command substitution", "export ZDOTDIR=$(dirname /a/b)\n", "") + test("parameter expansion", "export ZDOTDIR=${XDG_CONFIG_HOME}/zsh\n", "") + test("relative path", "export ZDOTDIR=.config/zsh\n", "") +} + +// An unreadable .zshenv is not an absent one. Falling back to $HOME on a +// permission error produces exactly the stray-dotfile outcome above, while +// reporting success. +func TestZshenvDotDirDistinguishesUnreadableFromAbsent(t *testing.T) { + t.Run("absent", func(t *testing.T) { + got, err := zshenvDotDir(t.TempDir()) + if err != nil { + t.Fatalf("an absent .zshenv is not an error: %v", err) + } + if got != "" { + t.Errorf("ZDOTDIR = %q, want the caller to fall back to $HOME", got) + } + }) + t.Run("unreadable", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads a 0o000 file regardless of its mode") + } + home := t.TempDir() + zshenv := filepath.Join(home, ".zshenv") + if err := os.WriteFile(zshenv, []byte("export ZDOTDIR=$HOME/.config/zsh\n"), 0o000); err != nil { + t.Fatal(err) + } + if _, err := zshenvDotDir(home); err == nil { + t.Error("an unreadable .zshenv was reported as absent") + } + }) +} diff --git a/services/nvpair-engine-manager/userpath_windows.go b/services/nvpair-engine-manager/userpath_windows.go new file mode 100644 index 00000000..ffd0ab13 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_windows.go @@ -0,0 +1,103 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "log/slog" + "runtime" + "unsafe" + + "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" +) + +// Resolved once. Building the lazy DLL per call re-resolves the module and +// leaks a reference for every PATH update. +var ( + user32 = windows.NewLazySystemDLL("user32.dll") + procSendMessageTimeoutW = user32.NewProc("SendMessageTimeoutW") +) + +// persistUserPath appends a user PATH entry only after recording its ownership. +func persistUserPath(dir string, receipt *pathReceipt, save func() error) error { + return updateWindowsPath(func(current string) (string, error) { + return addWindowsPath(current, dir, expandPath, receipt, save) + }) +} + +// removeUserPath removes only the registry entry recorded for this installation. +func removeUserPath(receipt *pathReceipt) error { + if receipt.WindowsEntry == "" { + return nil + } + return updateWindowsPath(func(current string) (string, error) { + return removeWindowsPath(current, receipt.WindowsEntry), nil + }) +} + +// updateWindowsPath preserves the registry value type and notifies new processes. +func updateWindowsPath(update func(string) (string, error)) error { + key, err := registry.OpenKey(registry.CURRENT_USER, `Environment`, registry.QUERY_VALUE|registry.SET_VALUE) + if err != nil { + return err + } + defer key.Close() + current, kind, err := key.GetStringValue("Path") + if errors.Is(err, registry.ErrNotExist) { + current, kind, err = "", registry.EXPAND_SZ, nil + } + if err != nil { + return err + } + next, err := update(current) + if err != nil { + return err + } + if next == current { + return nil + } + if kind == registry.EXPAND_SZ { + err = key.SetExpandStringValue("Path", next) + } else { + err = key.SetStringValue("Path", next) + } + if err != nil { + return err + } + broadcastEnvironmentChange() + return nil +} + +// broadcastEnvironmentChange tells Explorer that future processes should pick up +// the new user PATH. Failure is logged, never returned: SMTO_ABORTIFHUNG makes +// SendMessageTimeoutW return 0 whenever any top-level window is not pumping +// messages — routine on a busy desktop, and certain in a session with no window +// station — while the registry value it is announcing is already authoritative +// for every shell started from here on. +func broadcastEnvironmentChange() { + name, err := windows.UTF16PtrFromString("Environment") + if err != nil { + slog.Warn("skipped the environment change broadcast", "err", err) + return + } + const ( + hwndBroadcast = 0xffff + wmSettingChange = 0x001a + smtoAbortIfHung = 0x0002 + timeoutMs = 5000 + ) + // The final lpdwResult is optional and nothing here reads it, so pass NULL + // rather than a pointer into a variadic call that offers no lifetime guarantee. + delivered, _, callErr := procSendMessageTimeoutW.Call( + hwndBroadcast, wmSettingChange, 0, uintptr(unsafe.Pointer(name)), + smtoAbortIfHung, timeoutMs, 0, + ) + runtime.KeepAlive(name) + if delivered == 0 { + // callErr is a syscall.Errno and is never nil, so log it as the last + // error rather than as proof anything went wrong. + slog.Warn("PATH was saved, but the environment change broadcast did not complete; new terminals still inherit it", "last_error", callErr) + } +} diff --git a/services/nvpair-engine-manager/userpath_windows_entries.go b/services/nvpair-engine-manager/userpath_windows_entries.go new file mode 100644 index 00000000..1138459f --- /dev/null +++ b/services/nvpair-engine-manager/userpath_windows_entries.go @@ -0,0 +1,80 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "strings" + +// Windows PATH string algebra: splitting HKCU\Environment\Path on ';' and +// deciding what one entry means. Deliberately untagged, like userpath_shell.go, +// so the parsing rules are testable on every platform rather than only on the +// one CI happens to run. userpath_windows.go holds the registry access these +// feed, and that file is the Windows-only half. + +// Remove only the entry we wrote; differently spelled or expanded user entries +// are preserved. Comparison matches the one appendWindowsPath used to decide +// the entry was missing, so an external tool that re-cased the entry or gave it +// a trailing separator cannot make it unremovable. +// +// Unlike appendWindowsPath, no expansion is applied: the receipt records the +// literal entry PAIR wrote, and expanding a user's %VAR% here could match an +// entry PAIR never added. +// +// The search runs from the end because appendWindowsPath appends. The receipt +// records a directory, not a position, so when something else has since added +// the same directory the last match is the better guess at PAIR's own — and +// installers conventionally prepend. Exactly one goes, leaving whatever the +// other tool wanted where it wanted it. +func removeWindowsPath(current, owned string) string { + if owned == "" { + return current + } + entries := strings.Split(current, ";") + for i := len(entries) - 1; i >= 0; i-- { + if strings.EqualFold(normalizeWindowsPathEntry(entries[i]), normalizeWindowsPathEntry(owned)) { + return strings.Join(append(entries[:i:i], entries[i+1:]...), ";") + } + } + return current +} + +// addWindowsPath records newly appended entries but never claims existing ones. +// Ownership is recorded only when the PATH actually changed, which is the +// Windows half of the rule stated in userpath.go. +func addWindowsPath(current, dir string, expand func(string) string, receipt *pathReceipt, save func() error) (string, error) { + next := appendWindowsPath(current, dir, expand) + if next != current { + receipt.WindowsEntry = dir + if err := save(); err != nil { + return current, err + } + } + return next, nil +} + +// appendWindowsPath preserves existing entries and their order, including +// expandable environment references. Windows directory comparisons ignore case. +// +// A trailing separator is dropped rather than carried through: appending after +// one turns a harmless trailing empty element into an interior one, and Windows +// executable and DLL search has historically resolved an empty PATH element +// against the current directory. +func appendWindowsPath(current, dir string, expand func(string) string) string { + for _, entry := range strings.Split(current, ";") { + if strings.EqualFold(normalizeWindowsPathEntry(expand(entry)), normalizeWindowsPathEntry(dir)) { + return current + } + } + current = strings.TrimRight(current, ";") + if current == "" { + return dir + } + return current + ";" + dir +} + +// normalizeWindowsPathEntry makes two spellings of one directory comparable: +// surrounding quotes and spaces, slash direction, and a trailing separator are +// all cosmetic on Windows. +func normalizeWindowsPathEntry(value string) string { + return strings.TrimRight(strings.ReplaceAll(strings.Trim(value, " \""), "/", `\`), `\`) +}