From dc2f29bfcc491abb0cea1b2090959e1be3d5e439 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Thu, 8 Oct 2026 01:32:15 +0100 Subject: [PATCH 1/4] Find the hang-up watch's parent without listing every process Arming the watch at the start of a shimmed command with a piped stdin took a snapshot of every process on the machine, twice, to find the parent's pid. Measured on Windows on 2026-10-08, one snapshot took 19 to 27 ms (three runs of 40 calls). The kernel keeps the parent's pid in the process's own record, and NtQueryInformationProcess returns it in about 0.4 us a call (three runs of 2000). The snapshot stays as the fallback, and the pid found for the handle is now reused for the log line instead of being looked up again. Measured through the shim on 2026-10-08 on a busy machine, node --version took a median of 0.300 s with stdin a pipe and 0.221 s with stdin from NUL (21 interleaved runs each). With this change it takes 0.103 s and 0.101 s. --- internal/nvx/sandbox_parent_watch_windows.go | 69 ++++++++++++++++--- .../nvx/sandbox_parent_watch_windows_test.go | 55 +++++++++++++++ 2 files changed, 116 insertions(+), 8 deletions(-) diff --git a/internal/nvx/sandbox_parent_watch_windows.go b/internal/nvx/sandbox_parent_watch_windows.go index 7f1f8ac7..bc0e1bdb 100644 --- a/internal/nvx/sandbox_parent_watch_windows.go +++ b/internal/nvx/sandbox_parent_watch_windows.go @@ -89,14 +89,13 @@ func watchStdinForHangup(nvxHome string, onHangup func()) { // process even after it exits, so a recycled PID cannot later be mistaken // for our parent still running -- or worse, an unrelated new process's exit // mistaken for ours. - parent, ok := openParentProcess() + parent, ppid, ok := openParentProcessWithID() if !ok { // Cannot tell a finished pipeline from a departed client; do nothing. noteHangupWatch(nvxHome, "not-armed", "the parent process could not be identified or opened") return } - ppid, _ := parentProcessID() noteHangupWatch(nvxHome, "armed", fmt.Sprintf("watching stdin pipe against parent pid %d", ppid)) // Read once, here, rather than inside the goroutine. The goroutine outlives @@ -192,11 +191,19 @@ func noteHangupWatch(nvxHome, state, reason string) { // openParentProcess returns a handle to the process that started this one. func openParentProcess() (syscall.Handle, bool) { + h, _, ok := openParentProcessWithID() + return h, ok +} + +// openParentProcessWithID is openParentProcess that also says which pid it +// opened, so a caller that records the pid does not look it up a second time. +func openParentProcessWithID() (syscall.Handle, uint32, bool) { ppid, ok := parentProcessID() if !ok { - return 0, false + return 0, 0, false } - return openIfOlderThanSelf(ppid) + h, ok := openIfOlderThanSelf(ppid) + return h, ppid, ok } // openIfOlderThanSelf opens pid to wait on, but only if that process was created @@ -237,12 +244,58 @@ func processCreationTime(h syscall.Handle) (uint64, bool) { return uint64(created.HighDateTime)<<32 | uint64(created.LowDateTime), true } -// parentProcessID finds this process's parent via a process snapshot. +// parentProcessID finds this process's parent. // -// Toolhelp rather than NtQueryInformationProcess: the parent PID is not exposed -// by the Go standard library on Windows, and of the two ways to get it this one -// is documented and stable. +// It is asked of the kernel, which keeps it in this process's own record. The +// answer used to come from a snapshot of every process on the machine, taken +// twice to arm the watch. Measured 2026-10-07 on the development machine, one +// snapshot took 19 to 27 ms (three runs of 40 calls). Arming runs at the start +// of every shimmed command whose stdin is a pipe, as an agent harness or an MCP +// client leaves it. The snapshot stays as the fallback for a kernel that does +// not answer. func parentProcessID() (uint32, bool) { + if pid, ok := parentProcessIDFromKernel(); ok { + return pid, true + } + return parentProcessIDFromSnapshot() +} + +var ( + modNtdll = syscall.NewLazyDLL("ntdll.dll") + procNtQueryInformationProcess = modNtdll.NewProc("NtQueryInformationProcess") +) + +// processBasicInfo is PROCESS_BASIC_INFORMATION. Every field is pointer sized +// because ExitStatus and BasePriority, which are 32 bits wide, are padded to the +// next pointer on 64-bit Windows. +type processBasicInfo struct { + exitStatus uintptr + pebAddress uintptr + affinityMask uintptr + basePriority uintptr + processID uintptr + parentID uintptr +} + +// parentProcessIDFromKernel reads InheritedFromUniqueProcessId, the pid the +// snapshot reports as the parent. A variable so a test can show that arming the +// watch does not take the snapshot. +var parentProcessIDFromKernel = func() (uint32, bool) { + const processBasicInformation = 0 + var info processBasicInfo + self, _, _ := procGetCurrentProcess.Call() + status, _, _ := procNtQueryInformationProcess.Call(self, processBasicInformation, + uintptr(unsafe.Pointer(&info)), unsafe.Sizeof(info), 0) + // An NTSTATUS of zero is success. Anything else is not an answer. + if status != 0 || info.parentID == 0 { + return 0, false + } + return uint32(info.parentID), true +} + +// parentProcessIDFromSnapshot finds the parent in a snapshot of the process +// list. It is the documented route, and the slow one. +var parentProcessIDFromSnapshot = func() (uint32, bool) { const th32csSnapProcess = 0x00000002 snap, _, _ := procCreateToolhelp32Snapshot.Call(uintptr(th32csSnapProcess), 0) if snap == uintptr(syscall.InvalidHandle) || snap == 0 { diff --git a/internal/nvx/sandbox_parent_watch_windows_test.go b/internal/nvx/sandbox_parent_watch_windows_test.go index 37deefc7..c9d360b9 100644 --- a/internal/nvx/sandbox_parent_watch_windows_test.go +++ b/internal/nvx/sandbox_parent_watch_windows_test.go @@ -185,3 +185,58 @@ func TestHangupWatchRecordsWhyItDidNotArm(t *testing.T) { t.Errorf("the reason must name what stopped it arming, got %q", e["reason"]) } } + +// Finding the parent must not list every process on the machine. Arming the +// watch took a process snapshot twice, 19 to 27 ms a call on the development +// machine, at the start of every shimmed command whose stdin is a pipe. The +// kernel keeps the parent's pid in the process's own record, so the snapshot is +// there only for a kernel that gives no answer. +func TestFindingTheParentDoesNotListEveryProcess(t *testing.T) { + orig := parentProcessIDFromSnapshot + t.Cleanup(func() { parentProcessIDFromSnapshot = orig }) + parentProcessIDFromSnapshot = func() (uint32, bool) { + t.Error("the process list was snapshotted to find the parent") + return 0, false + } + + if _, ok := parentProcessID(); !ok { + t.Fatal("no parent found") + } + h, pid, ok := openParentProcessWithID() + if !ok { + t.Fatal("could not open the parent") + } + defer syscall.CloseHandle(h) + if pid == 0 { + t.Error("openParentProcessWithID opened a parent and reported pid 0") + } +} + +// The two ways of asking must agree, or the watchdog would wait on the wrong +// process. +func TestTheKernelAndTheProcessListNameTheSameParent(t *testing.T) { + fromKernel, ok := parentProcessIDFromKernel() + if !ok { + t.Fatal("the kernel gave no parent") + } + fromList, ok := parentProcessIDFromSnapshot() + if !ok { + t.Fatal("the process list gave no parent") + } + if fromKernel != fromList { + t.Errorf("the kernel says the parent is %d and the process list says %d", fromKernel, fromList) + } +} + +// A kernel that does not answer leaves the process list to do it, so the +// watchdog still arms. +func TestTheProcessListAnswersWhenTheKernelDoesNot(t *testing.T) { + origKernel, origList := parentProcessIDFromKernel, parentProcessIDFromSnapshot + t.Cleanup(func() { parentProcessIDFromKernel, parentProcessIDFromSnapshot = origKernel, origList }) + parentProcessIDFromKernel = func() (uint32, bool) { return 0, false } + parentProcessIDFromSnapshot = func() (uint32, bool) { return 4242, true } + + if pid, ok := parentProcessID(); !ok || pid != 4242 { + t.Errorf("parentProcessID() = %d, %v, want the process list's 4242", pid, ok) + } +} From a717687d109650c8b8938ee1afb5e796b60370c4 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Thu, 8 Oct 2026 01:32:27 +0100 Subject: [PATCH 2/4] Take the housekeeping sweep off the end of every command reclaimStaleSandboxes ran after each shimmed command and the command did not exit until it finished. Measured on Windows on 2026-10-08 with 638 package profiles on the machine, the package-profile sweep alone took 83 to 91 ms a call (three runs of 20). With one guest home of 12,000 files left by a killed install, node -e 0 through the shim took 3.50 s (median of 5 interleaved runs) because the command waited for the home to be deleted. The sweeps that go by age (package profiles, rescued logs, staged command copies) find something only once it is a week or two old. They now run when the last full look is an hour old, and sooner while one of them is still working through a backlog. The sweep runs in a goroutine and the command waits for it for at most 100 ms, leaving what is unfinished to the next command. Package profile deletions stay on the command's own goroutine, so the process is never cut off in the middle of one. One sweep runs at a time. The lock is the operating system's (flock on Unix, LockFileEx on Windows), so a sweep that dies does not leave it held. After this change node --version through the shim took a median of 0.101 s against 0.221 s before (21 interleaved runs each, stdin from NUL, node alone 0.052 s), and node -e 0 with the leftover home took 0.25 s against 3.50 s. --- internal/nvx/env.go | 5 +- internal/nvx/file_lock_unix.go | 13 ++ internal/nvx/file_lock_windows.go | 19 ++ internal/nvx/main_test.go | 4 + internal/nvx/reclaim.go | 161 ++++++++++++++ internal/nvx/reclaim_packages_other_test.go | 9 + internal/nvx/reclaim_packages_windows_test.go | 81 +++++++ internal/nvx/reclaim_test.go | 210 ++++++++++++++++++ internal/nvx/sandbox.go | 33 +-- 9 files changed, 504 insertions(+), 31 deletions(-) create mode 100644 internal/nvx/reclaim.go create mode 100644 internal/nvx/reclaim_packages_other_test.go create mode 100644 internal/nvx/reclaim_packages_windows_test.go create mode 100644 internal/nvx/reclaim_test.go diff --git a/internal/nvx/env.go b/internal/nvx/env.go index 994612ee..fc2d9ca4 100644 --- a/internal/nvx/env.go +++ b/internal/nvx/env.go @@ -889,8 +889,9 @@ func runShim(cmdName string, args []string, nvxHome string) int { // Housekeeping after the command, never before it: leftovers from processes // that were killed cannot clean up after themselves, and waiting for someone // to type `nvx cleanup` meant 91 guest homes on the machine this was found - // on. Bounded and silent -- see reclaimStaleSandboxes. - reclaimStaleSandboxes(nvxHome) + // on. Bounded and silent -- see reclaimStaleSandboxes. The command does not + // wait for it for long -- see reclaim.go. + reclaimAfterCommand(nvxHome) return code } diff --git a/internal/nvx/file_lock_unix.go b/internal/nvx/file_lock_unix.go index 8ac5359e..bee4cc70 100644 --- a/internal/nvx/file_lock_unix.go +++ b/internal/nvx/file_lock_unix.go @@ -16,3 +16,16 @@ func lockFileExclusive(f *os.File) error { func unlockFile(f *os.File) error { return syscall.Flock(int(f.Fd()), syscall.LOCK_UN) } + +// tryLockFileExclusive takes the lock lockFileExclusive does without waiting for +// it. It reports false and no error when another file holds the lock. +func tryLockFileExclusive(f *os.File) (bool, error) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if err == nil { + return true, nil + } + if err == syscall.EWOULDBLOCK { + return false, nil + } + return false, err +} diff --git a/internal/nvx/file_lock_windows.go b/internal/nvx/file_lock_windows.go index 514c4d43..a803374a 100644 --- a/internal/nvx/file_lock_windows.go +++ b/internal/nvx/file_lock_windows.go @@ -34,3 +34,22 @@ func unlockFile(f *os.File) error { } return nil } + +const ( + lockfileFailImmediately = 0x00000001 + errorLockViolation = 33 +) + +// tryLockFileExclusive takes the lock lockFileExclusive does without waiting for +// it. It reports false and no error when another handle holds the lock. +func tryLockFileExclusive(f *os.File) (bool, error) { + var ov syscall.Overlapped + r, _, err := procLockFileEx.Call(f.Fd(), lockfileExclusiveLock|lockfileFailImmediately, 0, 1, 0, uintptr(unsafe.Pointer(&ov))) + if r != 0 { + return true, nil + } + if errno, ok := err.(syscall.Errno); ok && errno == errorLockViolation { + return false, nil + } + return false, err +} diff --git a/internal/nvx/main_test.go b/internal/nvx/main_test.go index 4e2aa1b6..6daed3f0 100644 --- a/internal/nvx/main_test.go +++ b/internal/nvx/main_test.go @@ -62,6 +62,10 @@ func TestMain(m *testing.M) { // would otherwise print whatever an old setup left on the host running the // suite. reportSetupLeftoversFn = func(string) {} + // A command waits only briefly for the housekeeping sweep and leaves the rest + // to the next one. A test that ends while it is still running would see its + // temporary directory deleted under it, so the suite waits for all of it. + reclaimExitGrace = 0 code := m.Run() cleanupProbeChildBinary() diff --git a/internal/nvx/reclaim.go b/internal/nvx/reclaim.go new file mode 100644 index 00000000..5f752332 --- /dev/null +++ b/internal/nvx/reclaim.go @@ -0,0 +1,161 @@ +package nvx + +import ( + "os" + "path/filepath" + "time" +) + +// When the automatic sweep runs, and how long a command waits for it. +// +// reclaimStaleSandboxes ran after every shimmed command and the command did not +// finish until it had. Measured 2026-10-08 on Windows with 638 package profiles +// on the machine, the package-profile sweep alone took 83 to 91 ms a call (three +// runs of 20). With one guest home of 12,000 files left by a killed install, +// `node -e 0` through the shim took 3.50 s (median of 5), because it waited for +// the home to be deleted. +// +// Two changes. The sweeps that go by age (package profiles, rescued logs, staged +// command copies) only find something once it is a week or two old, so looking +// on every command found what the last look found. They run when the last full +// look is an hour old. And the command waits for the sweep only briefly. What it +// has not finished, the next command finishes. Each step is bounded, can be run +// again, and is safe to leave half done, which is what a killed process leaves +// anyway. +// +// One sweep runs at a time. Two commands that end together used to sweep +// together, each trying to delete the same guest home. The lock is the operating +// system's, so a process that dies holding it does not leave it held. + +// retentionSweepInterval is how long a full look at the age-based leftovers +// stays good. +const retentionSweepInterval = time.Hour + +// reclaimExitGrace is how long the end of a command waits for the sweep. Zero +// waits for all of it, which is what the tests want. A variable for them. +var reclaimExitGrace = 100 * time.Millisecond + +// reclaimGuestHomes is the step that runs on every sweep. A variable so a test +// can make it slow, to show that a command does not wait for it. +var reclaimGuestHomes = func(nvxHome string) { + cleanupStaleSandboxes(nvxHome, reclaimBudgetPerRun) +} + +// reclaimAfterCommand is the sweep a command runs on its way out. It returns +// when the sweep has finished or reclaimExitGrace has passed. +func reclaimAfterCommand(nvxHome string) { + reclaim(nvxHome, reclaimExitGrace) +} + +// reclaim sweeps once and waits for it at most grace, or for all of it when +// grace is zero. +func reclaim(nvxHome string, grace time.Duration) { + if nvxHome == "" { + return + } + release, ok := lockReclaim(nvxHome) + if !ok { + // Another command is sweeping. What it leaves, a later command takes. + return + } + retention := retentionSweepDue(nvxHome) + backlog := false + if retention { + // AppContainer package profiles, on Windows. Swept here rather than only + // from `nvx cleanup` for the same reason guest homes are. A command nobody + // runs reclaims nothing. One profile is registered per project nvx has ever + // contained. The first version of this swept only on an explicit cleanup, + // and only when no session at all was running. On a machine with a couple + // of long-lived MCP servers that is never. + // + // Safe unprompted on the same terms. A package held by a live session is + // skipped, and one used inside the retention window is left alone so the + // common case never pays to re-register a profile it is about to use again. + // + // This runs here and not in the goroutine below. Deleting a profile is a + // call into Windows that this process should not be cut off in the middle + // of, and the process may leave while the goroutine is still working. + backlog = sweepOrphanedSandboxPackages(nvxHome, reclaimBudgetPerRun) >= reclaimBudgetPerRun + } + + done := make(chan struct{}) + go func() { + defer close(done) + defer release() + reclaimGuestHomes(nvxHome) + if !retention { + return + } + // Logs rescued from failed runs. They had no sweep at all, so they + // accumulated for the life of the installation. That was 3,146 directories + // and 181 MB on the development machine, which `nvx cleanup` also left alone. + // + // A larger budget than the sweeps above, because the work is not + // comparable. A guest home may hold a large tree and a package profile + // costs a registry write. These are small directories of log files. At + // eight per run a three-thousand-folder backlog needs some four hundred + // commands to clear, which is not a reclaim so much as a rumour of one. + if sweepRescuedLogs(nvxHome, rescuedLogBudgetPerRun) >= rescuedLogBudgetPerRun { + backlog = true + } + // Staged command copies whose command has changed. One per run: each is a + // whole directory, up to tens of thousands of files. + if pruneStaleCommandCopies(nvxHome, 1) >= 1 { + backlog = true + } + // A step that used its whole budget may have more to do, so the next + // command looks again instead of waiting out the hour. + if !backlog { + markRetentionSweep(nvxHome) + } + }() + + if grace <= 0 { + <-done + return + } + timer := time.NewTimer(grace) + defer timer.Stop() + select { + case <-done: + case <-timer.C: + } +} + +// reclaimLockPath is the file the sweep's lock is taken on. +func reclaimLockPath(nvxHome string) string { return filepath.Join(nvxHome, "reclaim.lock") } + +// lockReclaim takes the right to sweep, and reports false when another process +// holds it. release gives it back. A lock the file system cannot give is not a +// reason to skip the sweep, which is housekeeping and was safe to run twice. +func lockReclaim(nvxHome string) (release func(), ok bool) { + f, err := os.OpenFile(reclaimLockPath(nvxHome), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return func() {}, true + } + locked, err := tryLockFileExclusive(f) + if err == nil && !locked { + _ = f.Close() + return nil, false + } + // Closing the file releases the lock, and so does the process ending. + return func() { _ = f.Close() }, true +} + +func retentionStampPath(nvxHome string) string { return filepath.Join(nvxHome, "reclaim.stamp") } + +// retentionSweepDue reports whether the age-based sweeps have not run in the +// last retentionSweepInterval. A stamp dated in the future, from a clock that +// was put back, counts as due. +func retentionSweepDue(nvxHome string) bool { + info, err := os.Stat(retentionStampPath(nvxHome)) + if err != nil { + return true + } + age := time.Since(info.ModTime()) + return age < 0 || age >= retentionSweepInterval +} + +func markRetentionSweep(nvxHome string) { + _ = os.WriteFile(retentionStampPath(nvxHome), nil, 0o600) +} diff --git a/internal/nvx/reclaim_packages_other_test.go b/internal/nvx/reclaim_packages_other_test.go new file mode 100644 index 00000000..e90da038 --- /dev/null +++ b/internal/nvx/reclaim_packages_other_test.go @@ -0,0 +1,9 @@ +//go:build !windows + +package nvx + +import "testing" + +// isolatePackageSweep is for the Windows package sweep. There is nothing to +// isolate elsewhere. +func isolatePackageSweep(t *testing.T) { t.Helper() } diff --git a/internal/nvx/reclaim_packages_windows_test.go b/internal/nvx/reclaim_packages_windows_test.go new file mode 100644 index 00000000..c44c1df3 --- /dev/null +++ b/internal/nvx/reclaim_packages_windows_test.go @@ -0,0 +1,81 @@ +//go:build windows + +package nvx + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// isolatePackageSweep points the package sweep at an empty folder, so a test of +// the sweep as a whole does not list or delete the profiles of this machine. +func isolatePackageSweep(t *testing.T) { + t.Helper() + root := tempDir(t) + prevRoot, prevDelete := packagesRoot, deleteSandboxPackage + packagesRoot = func() string { return root } + deleteSandboxPackage = func(string) {} + t.Cleanup(func() { packagesRoot, deleteSandboxPackage = prevRoot, prevDelete }) +} + +// Listing the package folder is what the sweep costs. Over this machine's 638 +// profiles it took 83 to 91 ms a call, and a profile is only reclaimed a week +// after its last use. It is listed once an hour. +func TestThePackageFolderIsListedOnceAnHour(t *testing.T) { + root := tempDir(t) + nvxHome := tempDir(t) + prevRoot, prevDelete := packagesRoot, deleteSandboxPackage + listed := 0 + packagesRoot = func() string { listed++; return root } + deleteSandboxPackage = func(string) {} + t.Cleanup(func() { packagesRoot, deleteSandboxPackage = prevRoot, prevDelete }) + + reclaimStaleSandboxes(nvxHome) + if listed != 1 { + t.Fatalf("the first sweep listed the package folder %d times, want 1", listed) + } + reclaimStaleSandboxes(nvxHome) + reclaimStaleSandboxes(nvxHome) + if listed != 1 { + t.Fatalf("the package folder was listed %d times in three sweeps within the hour, want 1", listed) + } + + old := time.Now().Add(-retentionSweepInterval - time.Minute) + if err := os.Chtimes(retentionStampPath(nvxHome), old, old); err != nil { + t.Fatal(err) + } + reclaimStaleSandboxes(nvxHome) + if listed != 2 { + t.Fatalf("the package folder was listed %d times after the hour, want 2", listed) + } +} + +// A package sweep that reached its budget has more to delete, so the next +// command lists again. +func TestAPackageBacklogIsNotWaitedOutForAnHour(t *testing.T) { + root := tempDir(t) + nvxHome := tempDir(t) + prevRoot, prevDelete := packagesRoot, deleteSandboxPackage + packagesRoot = func() string { return root } + deleteSandboxPackage = func(name string) { _ = os.RemoveAll(filepath.Join(root, name)) } + t.Cleanup(func() { packagesRoot, deleteSandboxPackage = prevRoot, prevDelete }) + + old := time.Now().Add(-30 * 24 * time.Hour) + const total = reclaimBudgetPerRun + 3 + for i := 0; i < total; i++ { + stagePackage(t, root, nvxHome, nvxPackagePrefix+string(rune('a'+i))+"aaaaaaaaaaaaaaa", old) + } + + reclaimStaleSandboxes(nvxHome) + left, _ := os.ReadDir(root) + if len(left) != total-reclaimBudgetPerRun { + t.Fatalf("%d profiles left after one sweep, want %d", len(left), total-reclaimBudgetPerRun) + } + reclaimStaleSandboxes(nvxHome) + left, _ = os.ReadDir(root) + if len(left) != 0 { + t.Fatalf("%d profiles left after the second sweep, want none: a backlog waited out the hour", len(left)) + } +} diff --git a/internal/nvx/reclaim_test.go b/internal/nvx/reclaim_test.go new file mode 100644 index 00000000..7b07d8a2 --- /dev/null +++ b/internal/nvx/reclaim_test.go @@ -0,0 +1,210 @@ +package nvx + +import ( + "fmt" + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" +) + +// oldRescuedLog makes a rescued log folder past the retention window. +func oldRescuedLog(t *testing.T, nvxHome, name string) string { + t.Helper() + dir := filepath.Join(nvxHome, "logs", name) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + old := time.Now().Add(-rescuedLogRetention - time.Hour) + if err := os.Chtimes(dir, old, old); err != nil { + t.Fatal(err) + } + return dir +} + +func pathExists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// The sweeps that go by age find something only once it is a week or two old, so +// they look once an hour and not after every command. Measured 2026-10-08 on +// Windows, the package sweep alone took 83 to 91 ms a call. +func TestAgeBasedLeftoversAreLookedForOnceAnHour(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + + first := oldRescuedLog(t, nvxHome, "first") + reclaimStaleSandboxes(nvxHome) + if pathExists(first) { + t.Fatal("the first sweep left a rescued log past its retention window") + } + + second := oldRescuedLog(t, nvxHome, "second") + reclaimStaleSandboxes(nvxHome) + if !pathExists(second) { + t.Fatal("a sweep a moment after the last full look looked again") + } + + old := time.Now().Add(-retentionSweepInterval - time.Minute) + if err := os.Chtimes(retentionStampPath(nvxHome), old, old); err != nil { + t.Fatal(err) + } + reclaimStaleSandboxes(nvxHome) + if pathExists(second) { + t.Fatal("a sweep an hour after the last full look did not look again") + } +} + +// A clock put back leaves a stamp dated in the future, and the age-based sweeps +// must not wait for the clock to catch up with it. +func TestAStampFromTheFutureDoesNotHoldBackTheSweep(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + markRetentionSweep(nvxHome) + future := time.Now().Add(48 * time.Hour) + if err := os.Chtimes(retentionStampPath(nvxHome), future, future); err != nil { + t.Fatal(err) + } + log := oldRescuedLog(t, nvxHome, "stuck") + reclaimStaleSandboxes(nvxHome) + if pathExists(log) { + t.Fatal("a stamp dated in the future stopped the sweep") + } +} + +// A sweep that used its whole budget may have more to do. The next command looks +// again, and the hour starts when a look finds the end. +func TestABacklogIsNotWaitedOutForAnHour(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + const total = rescuedLogBudgetPerRun + 5 + for i := 0; i < total; i++ { + oldRescuedLog(t, nvxHome, fmt.Sprintf("log%03d", i)) + } + + count := func() int { + entries, _ := os.ReadDir(filepath.Join(nvxHome, "logs")) + return len(entries) + } + reclaimStaleSandboxes(nvxHome) + if got := count(); got != total-rescuedLogBudgetPerRun { + t.Fatalf("%d folders left after a sweep of budget %d from %d, want %d", got, rescuedLogBudgetPerRun, total, total-rescuedLogBudgetPerRun) + } + if pathExists(retentionStampPath(nvxHome)) { + t.Fatal("a sweep that stopped at its budget recorded a full look") + } + reclaimStaleSandboxes(nvxHome) + if got := count(); got != 0 { + t.Fatalf("%d folders left after the second sweep, want none", got) + } + if !pathExists(retentionStampPath(nvxHome)) { + t.Fatal("a sweep that reached the end recorded no full look") + } +} + +// Two commands that end together used to sweep together, each trying to delete +// the same guest home. One sweeps and the other leaves it, and the lock is the +// operating system's, so a sweep that is killed does not leave it held. +func TestOnlyOneSweepRunsAtATime(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + dead := makeGuestHome(t, nvxHome, "dead-session", deadPID(t), 0) + + holder, err := os.OpenFile(reclaimLockPath(nvxHome), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + locked, err := tryLockFileExclusive(holder) + if err != nil || !locked { + t.Fatalf("could not take the sweep lock for the test: %v, %v", locked, err) + } + + reclaimStaleSandboxes(nvxHome) + if !pathExists(dead) { + t.Fatal("a second sweep ran while another held the lock") + } + + _ = holder.Close() + reclaimStaleSandboxes(nvxHome) + if pathExists(dead) { + t.Fatal("the sweep did not run once the lock was free") + } +} + +// lockIsFree reports whether nothing holds the sweep lock. +func lockIsFree(t *testing.T, nvxHome string) bool { + t.Helper() + probe, err := os.OpenFile(reclaimLockPath(nvxHome), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + defer probe.Close() + locked, _ := tryLockFileExclusive(probe) + return locked +} + +// The command waits for the sweep only briefly. Measured 2026-10-08 on Windows, +// an unrelated command waited 3.50 s (median of 5) for a guest home of 12,000 +// files, left by a killed install, to be deleted. +func TestACommandDoesNotWaitLongForASlowSweep(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + + release := make(chan struct{}) + var finished atomic.Bool + orig := reclaimGuestHomes + t.Cleanup(func() { reclaimGuestHomes = orig }) + reclaimGuestHomes = func(string) { + <-release + finished.Store(true) + } + + start := time.Now() + reclaim(nvxHome, 50*time.Millisecond) + if took := time.Since(start); took > 5*time.Second { + t.Fatalf("the command waited %s for a sweep that was still running", took) + } + if finished.Load() { + t.Fatal("the sweep finished, so the test did not show that the command left it running") + } + + // The sweep goes on holding the lock, so a second command does not start + // another one on top of it. + if lockIsFree(t, nvxHome) { + t.Error("the lock was free while the sweep was still running") + } + + close(release) + deadline := time.Now().Add(10 * time.Second) + for !lockIsFree(t, nvxHome) { + if time.Now().After(deadline) { + t.Fatal("the sweep never released its lock") + } + time.Sleep(10 * time.Millisecond) + } + if !finished.Load() { + t.Error("the lock was released before the sweep finished") + } +} + +// With no time limit the command waits for the whole sweep, which is what the +// tests and reclaimStaleSandboxes rely on. +func TestWithoutATimeLimitTheCommandWaitsForTheSweep(t *testing.T) { + isolatePackageSweep(t) + nvxHome := tempDir(t) + + var finished atomic.Bool + orig := reclaimGuestHomes + t.Cleanup(func() { reclaimGuestHomes = orig }) + reclaimGuestHomes = func(string) { + time.Sleep(100 * time.Millisecond) + finished.Store(true) + } + + reclaim(nvxHome, 0) + if !finished.Load() { + t.Fatal("reclaim returned before a sweep with no time limit had finished") + } +} diff --git a/internal/nvx/sandbox.go b/internal/nvx/sandbox.go index fc47bcb4..25397393 100644 --- a/internal/nvx/sandbox.go +++ b/internal/nvx/sandbox.go @@ -775,34 +775,9 @@ const reclaimBudgetPerRun = 8 // `nvx cleanup` still does that, where nothing is mid-launch. // // Runs after the command rather than before, so it never delays what was typed. +// The steps and when they run are in reclaim.go. This one runs all of them to +// the end, which is what the tests and anything that wants the work done need. +// A command uses reclaimAfterCommand, which does not wait for the sweep for long. func reclaimStaleSandboxes(nvxHome string) { - if nvxHome == "" { - return - } - cleanupStaleSandboxes(nvxHome, reclaimBudgetPerRun) - // AppContainer package profiles, on Windows. Swept here rather than only - // from `nvx cleanup` for the same reason guest homes are: a command nobody - // runs reclaims nothing. One profile is registered per project nvx has ever - // contained, and the first version of this swept only on an explicit cleanup - // AND only when no session at all was running -- which on a machine with a - // couple of long-lived MCP servers is never. - // - // Safe unprompted on the same terms: a package held by a live session is - // skipped, and one used inside the retention window is left alone so the - // common case never pays to re-register a profile it is about to use again. - sweepOrphanedSandboxPackages(nvxHome, reclaimBudgetPerRun) - // Logs rescued from failed runs. They had no sweep at all, so they - // accumulated for the life of the installation -- 3,146 directories and - // 181 MB on the development machine, which `nvx cleanup` also left alone. - // - // A larger budget than the sweeps above, because the work is not comparable: a - // guest home may hold a large tree and a package profile costs a registry - // write, while these are small directories of log files. At eight per run a - // three-thousand-folder backlog needs some four hundred commands to clear, - // which is not a reclaim so much as a rumour of one. - sweepRescuedLogs(nvxHome, rescuedLogBudgetPerRun) - // Staged command copies whose command has changed. One per run: each is a - // whole directory, up to tens of thousands of files, and this runs before - // the prompt comes back. - pruneStaleCommandCopies(nvxHome, 1) + reclaim(nvxHome, 0) } From bacb0c02dd58d73b1e6db057f4e61deefaff718d Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Thu, 8 Oct 2026 01:32:39 +0100 Subject: [PATCH 3/4] Install from the lockfile npm's resolver wrote nvx runs npm twice for an install that brings in new packages. The first run resolves the tree and nvx checks what it wrote. The second run resolved the tree again from the registry and installed that, so a version published between the runs was installed without being checked, and the second run repeated the work of the first. The second run now starts from the lockfile the first one wrote. For a bare install that is the lockfile as it stands. For an install that names packages, each name is first made exact from the lockfile (foo and foo@latest become foo@1.2.3), because npm asks the registry again about any package it is told to install. A range, an alias, a URL or a git source is not pinned, nor is a flag nvx does not know takes no value, and those installs run as they did. npm update and npm dedupe run as they did, since they ask for newer versions by design. Only a lockfile of hashed tarballs is handed over, the entries the checks hold to the registry's record. With --verbose the run says why it resolved again. The pass runs from the project with a folder inside it as its prefix. For an install that names nothing, npm 7 to 10 then install the project into that folder as a file: dependency, and the lockfile gains the project's own files, a link to them and a dependency on them. Measured on 2026-10-08, npm 7.24.2, 8.19.4, 9.9.4, 10.9.3 and 10.9.9 do this and 11.11.0 does not. That is removed before the lockfile is handed over. With 10.9.9 and the 321-package project the rest of the lockfile was the same as one resolved on its own. The lockfile is written into the project before the install and taken out again when npm did not write it itself. That is what a run that stops before npm saves leaves, and so do a project that sets package-lock=false and an install with --no-save. The new tests run the real npm against a fake registry that publishes new versions between the two runs, and compare package.json, package-lock.json and the installed versions with a plain npm run. A bare install and an install of a name, a tag and -D pass with npm 7.24.2, 8.19.4, 9.9.4, 10.9.3, 10.9.9 and 11.11.0 on Windows, and with 10.9.8 on Linux under the race detector. With the handoff switched off the same test installs the new versions and fails. Through the contained shim on Windows with Node 22.23.3 (npm 10.9.9), the 321-package project ended with the same package-lock.json as plain npm. Measured on Windows on 2026-10-08 on a busy machine, median of 5 interleaved runs through the contained shim. The 321-package project with no lockfile took 88.2 s and now takes 64.8 s. An install of one package took 8.7 s and takes 8.8 s. In a Linux container the 321-package project took 89.8 s and now takes 61.4 s, and one package took 2.47 s and takes 2.39 s. --- SECURITY.md | 7 +- internal/nvx/env.go | 9 + internal/nvx/npm_fake_registry_test.go | 183 ++++++++ internal/nvx/npm_resolve.go | 32 +- internal/nvx/npm_resolved_install.go | 438 ++++++++++++++++++ internal/nvx/npm_resolved_install_npm_test.go | 367 +++++++++++++++ .../npm_resolved_install_perm_unix_test.go | 30 ++ .../npm_resolved_install_project_link_test.go | 131 ++++++ internal/nvx/npm_resolved_install_test.go | 367 +++++++++++++++ internal/nvx/verify_targets.go | 3 + site/src/content/docs/docs/index.md | 8 +- 11 files changed, 1566 insertions(+), 9 deletions(-) create mode 100644 internal/nvx/npm_fake_registry_test.go create mode 100644 internal/nvx/npm_resolved_install.go create mode 100644 internal/nvx/npm_resolved_install_npm_test.go create mode 100644 internal/nvx/npm_resolved_install_perm_unix_test.go create mode 100644 internal/nvx/npm_resolved_install_project_link_test.go create mode 100644 internal/nvx/npm_resolved_install_test.go diff --git a/SECURITY.md b/SECURITY.md index 78feafc7..d8f99d4b 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -66,7 +66,12 @@ install and run scripts. Its defenses are layered: `--package-lock-only --ignore-scripts`, contained like the install, on a scratch copy of `package.json` and the lockfile. Every package in the tree it writes is checked, dependencies included, apart from those already - installed at the same version, which npm leaves alone. For `npm ci`, `npm rebuild` and + installed at the same version, which npm leaves alone. An `npm install` + then starts from the lockfile that resolver wrote, so it installs the tree + that was checked even if the registry changes in between. `npm update`, + `npm dedupe` and an install nvx cannot pin to that lockfile (one that names + a version range, an alias or a git source) run npm's resolver a second time + instead. For `npm ci`, `npm rebuild` and an `npm install` whose lockfile matches `package.json`, every lockfile entry for this platform is checked. A lockfile entry's `resolved` URL and `integrity` hash must match the registry's record for its name and version, and an entry that diff --git a/internal/nvx/env.go b/internal/nvx/env.go index fc2d9ca4..9699cc81 100644 --- a/internal/nvx/env.go +++ b/internal/nvx/env.go @@ -968,13 +968,16 @@ func runShimTraced(trace *runTrace, cmdName string, args []string, nvxHome strin toolName = tool } + var resolved *resolvedInstall switch strings.ToLower(pmCmd) { case "npm", "yarn", "pnpm", "npx", "bun", "bunx": + resolved = &resolvedInstall{} // Returning rather than exiting here is what lets runShim record the // abort. A blocked or refused install is a run, and the one a later // review most wants to find. if code, reason, label := verifyBeforeRun(verifyRequest{ pmCmd: pmCmd, pmArgs: pmArgs, nvxHome: nvxHome, contain: contain, + resolved: resolved, launch: SandboxConfig{ NvxHome: nvxHome, FilesystemProvider: opts.filesystemProvider, @@ -991,6 +994,12 @@ func runShimTraced(trace *runTrace, cmdName string, args []string, nvxHome strin return verifyExitCode(code, reason) } } + // An install that was resolved for the checks starts from the lockfile they + // ran on, and not from a second resolution that could differ. See adopt. + if run, finish := resolved.adopt(cmdName, args, pmCmd, pmArgs); finish != nil { + args = run + defer finish() + } if cmdName == "npm" || cmdName == "yarn" || cmdName == "pnpm" { if cwd, err := os.Getwd(); err == nil { if root := findProjectRoot(cwd); root != "" { diff --git a/internal/nvx/npm_fake_registry_test.go b/internal/nvx/npm_fake_registry_test.go new file mode 100644 index 00000000..e71aae56 --- /dev/null +++ b/internal/nvx/npm_fake_registry_test.go @@ -0,0 +1,183 @@ +package nvx + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "crypto/sha1" // #nosec G505 -- the registry's shasum field is SHA-1 + "crypto/sha512" + "encoding/base64" + "encoding/hex" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" +) + +// fakeNpmRegistry is a registry real npm can install from. It serves packuments +// and tarballs for what a test publishes, and logs what was asked, so a test can +// change what the registry holds between two runs of npm and see what each asked. +type fakeNpmRegistry struct { + t *testing.T + srv *httptest.Server + + mu sync.Mutex + pkgs map[string]*fakeNpmPackage + log []fakeNpmRequest +} + +type fakeNpmPackage struct { + versions map[string]fakeNpmVersion + tags map[string]string +} + +type fakeNpmVersion struct { + deps map[string]string + scripts map[string]string + tgz []byte +} + +// fakeNpmRequest is one request, with the program that made it. +type fakeNpmRequest struct { + path, agent string +} + +// byNpm reports a request that npm itself made, as opposed to nvx's own checks. +func (r fakeNpmRequest) byNpm() bool { return strings.HasPrefix(r.agent, "npm/") } + +// forPackument reports a request for a package's metadata rather than a tarball. +func (r fakeNpmRequest) forPackument() bool { return !strings.Contains(r.path, "/-/") } + +func newFakeNpmRegistry(t *testing.T) *fakeNpmRegistry { + t.Helper() + r := &fakeNpmRegistry{t: t, pkgs: map[string]*fakeNpmPackage{}} + r.srv = httptest.NewServer(http.HandlerFunc(r.serve)) + t.Cleanup(r.srv.Close) + return r +} + +func (r *fakeNpmRegistry) url() string { return r.srv.URL + "/" } + +// publish adds a version and makes it the latest. +func (r *fakeNpmRegistry) publish(name, version string, deps map[string]string) { + r.t.Helper() + r.publishWith(name, version, deps, nil) +} + +func (r *fakeNpmRegistry) publishWith(name, version string, deps, scripts map[string]string) { + r.t.Helper() + manifest := map[string]any{"name": name, "version": version} + if len(deps) > 0 { + manifest["dependencies"] = deps + } + if len(scripts) > 0 { + manifest["scripts"] = scripts + } + body, err := json.Marshal(manifest) + if err != nil { + r.t.Fatal(err) + } + var tgz bytes.Buffer + gz := gzip.NewWriter(&tgz) + tw := tar.NewWriter(gz) + for _, f := range []struct{ name, body string }{ + {"package/package.json", string(body)}, + {"package/index.js", "module.exports = " + fmt.Sprintf("%q", version) + ";\n"}, + } { + if err := tw.WriteHeader(&tar.Header{Name: f.name, Mode: 0o644, Size: int64(len(f.body)), ModTime: time.Unix(0, 0)}); err != nil { + r.t.Fatal(err) + } + if _, err := tw.Write([]byte(f.body)); err != nil { + r.t.Fatal(err) + } + } + if err := tw.Close(); err != nil { + r.t.Fatal(err) + } + if err := gz.Close(); err != nil { + r.t.Fatal(err) + } + + r.mu.Lock() + defer r.mu.Unlock() + p := r.pkgs[name] + if p == nil { + p = &fakeNpmPackage{versions: map[string]fakeNpmVersion{}, tags: map[string]string{}} + r.pkgs[name] = p + } + p.versions[version] = fakeNpmVersion{deps: deps, scripts: scripts, tgz: tgz.Bytes()} + p.tags["latest"] = version +} + +// requests returns what has been asked so far. +func (r *fakeNpmRegistry) requests() []fakeNpmRequest { + r.mu.Lock() + defer r.mu.Unlock() + return append([]fakeNpmRequest(nil), r.log...) +} + +func (r *fakeNpmRegistry) serve(w http.ResponseWriter, req *http.Request) { + path := req.URL.EscapedPath() + r.mu.Lock() + r.log = append(r.log, fakeNpmRequest{path: path, agent: req.UserAgent()}) + r.mu.Unlock() + + name, tarball, isTarball := strings.Cut(strings.TrimPrefix(req.URL.Path, "/"), "/-/") + r.mu.Lock() + defer r.mu.Unlock() + p := r.pkgs[name] + if p == nil { + http.NotFound(w, req) + return + } + if isTarball { + for version, v := range p.versions { + if tarball == name+"-"+version+".tgz" { + w.Header().Set("Content-Type", "application/octet-stream") + _, _ = w.Write(v.tgz) + return + } + } + http.NotFound(w, req) + return + } + + versions := map[string]any{} + times := map[string]string{"created": "2020-01-01T00:00:00.000Z", "modified": "2020-01-01T00:00:00.000Z"} + for version, v := range p.versions { + sum := sha512.Sum512(v.tgz) + sha := sha1.Sum(v.tgz) // #nosec G401 -- the registry's shasum field is SHA-1 + manifest := map[string]any{ + "name": name, "version": version, + "dist": map[string]string{ + "tarball": r.srv.URL + "/" + name + "/-/" + name + "-" + version + ".tgz", + "integrity": "sha512-" + base64.StdEncoding.EncodeToString(sum[:]), + "shasum": hex.EncodeToString(sha[:]), + }, + } + if len(v.deps) > 0 { + manifest["dependencies"] = v.deps + } + if len(v.scripts) > 0 { + manifest["scripts"] = v.scripts + } + versions[version] = manifest + times[version] = "2020-01-02T03:04:05.000Z" + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "name": name, "dist-tags": p.tags, "versions": versions, "time": times, + }) +} + +// reset forgets every package and every request. +func (r *fakeNpmRegistry) reset() { + r.mu.Lock() + defer r.mu.Unlock() + r.pkgs = map[string]*fakeNpmPackage{} + r.log = nil +} diff --git a/internal/nvx/npm_resolve.go b/internal/nvx/npm_resolve.go index 95eca2cb..9d71b9d6 100644 --- a/internal/nvx/npm_resolve.go +++ b/internal/nvx/npm_resolve.go @@ -20,6 +20,8 @@ import ( // node_modules/.cache, so the project's own files are not touched. The // lockfile it writes lists every package at the version npm chose, with its // tarball URL and hash, and the checks run on what is not installed already. +// The install then starts from that same lockfile and does not resolve again. +// See npm_resolved_install.go. // // A copy of those files cannot stand in for a project whose package.json names // workspaces or local folders, so those are not resolved, and the run says so. @@ -209,8 +211,19 @@ func resolveNpmInstall(req verifyRequest, root string, platform nodePlatform) ([ } defer func() { _ = os.RemoveAll(scratch) }() + // What the pass is given is kept as well, so that the install can tell + // whether the project changed while the checks ran. See adopt. + given := map[string][]byte{} for _, name := range []string{"package.json", "package-lock.json", "npm-shrinkwrap.json", ".npmrc"} { - if err := copyFileIfExists(filepath.Join(root, name), filepath.Join(scratch, name)); err != nil { + data, err := readIfExists(filepath.Join(root, name)) + if err != nil { + return nil, fmt.Errorf("could not copy %s: %w", name, err) + } + if data == nil { + continue + } + given[name] = data + if err := os.WriteFile(filepath.Join(scratch, name), data, 0o600); err != nil { return nil, fmt.Errorf("could not copy %s: %w", name, err) } } @@ -251,6 +264,8 @@ func resolveNpmInstall(req verifyRequest, root string, platform nodePlatform) ([ if targets == nil { targets = []verifyTarget{} } + // The install starts from the lockfile that was just checked. + req.resolved.record(root, scratch, given) return targets, nil } @@ -322,13 +337,18 @@ func missingDirs(dir string) []string { } } -func copyFileIfExists(src, dst string) error { - data, err := os.ReadFile(src) +// readIfExists returns the file's bytes, nil when there is no such file, and an +// empty slice that is not nil for a file with nothing in it. +func readIfExists(path string) ([]byte, error) { + data, err := os.ReadFile(path) if errors.Is(err, os.ErrNotExist) { - return nil + return nil, nil } if err != nil { - return err + return nil, err + } + if data == nil { + data = []byte{} } - return os.WriteFile(dst, data, 0o600) + return data, nil } diff --git a/internal/nvx/npm_resolved_install.go b/internal/nvx/npm_resolved_install.go new file mode 100644 index 00000000..6546f6ca --- /dev/null +++ b/internal/nvx/npm_resolved_install.go @@ -0,0 +1,438 @@ +package nvx + +import ( + "bytes" + "encoding/json" + "errors" + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "time" +) + +// Installing what was checked. +// +// npm ran twice. The first run resolved the tree and nvx checked what it wrote. +// The second run resolved the tree again and installed that. The two trees agree +// only while the registry does. A version published between the runs was +// installed without ever being checked, and the second run did all the work of +// the first a second time. +// +// The second run now starts from the lockfile the first one wrote. npm installs +// a lockfile that matches package.json as it stands and asks the registry for +// nothing but the tarballs, so what is installed is what was checked. The +// lockfile is put in the project before the install, and taken out again if npm +// does not end up using it, so the project's own lockfile is what npm would have +// left. See adopt. +// +// This is not done for every command npm resolves, only where the second run +// would otherwise be a fresh resolution of something already resolved: +// +// - A bare install, with or without a lockfile. The lockfile is all it needs. +// - An install that names packages, when every name can be pinned to the +// version the lockfile holds without changing what npm saves in package.json. +// npm treats a named package as a request and asks the registry about it +// again however the lockfile reads, so the name is made exact first. +// - Not update or dedupe. They ask the registry for something newer than the +// lockfile holds, which is what they are for. A lockfile cannot stand in for +// that, and installing instead of updating would also run the project's own +// install scripts, which npm update does not. +// - Not an install that names a range, an alias, a URL or anything else pinning +// would change the meaning of, nor one with a flag nvx does not know takes no +// value. Those run as they did. + +// adoptCheckedLockfile turns the handoff off. A variable so a test can show what +// an install gets without it. +var adoptCheckedLockfile = true + +// resolvedInstall is what npm's resolution pass was given and wrote. +type resolvedInstall struct { + root string + // lock is the lockfile the pass wrote, and lockfile what it says. + lock []byte + lockfile packageLockFile + // The project's own files as the pass was given them, nil for a file that + // did not exist. If they differ when the install starts, the project changed + // while the checks ran and the lockfile no longer describes it. + manifest, projectLock []byte +} + +// record keeps what the pass wrote, when it is something the install can start +// from. scratch is where the pass ran, given the project's files by name. +func (r *resolvedInstall) record(root, scratch string, given map[string][]byte) { + if r == nil { + return + } + *r = resolvedInstall{} + // A shrinkwrap is what npm reads first, and the pass wrote to it. The + // project's lockfile is not the one the install would use. + if given["npm-shrinkwrap.json"] != nil { + LogDetail("The project has an npm-shrinkwrap.json, so the install resolves again.") + return + } + lock, err := os.ReadFile(filepath.Join(scratch, "package-lock.json")) + if err != nil { + return + } + written, ok := readManifestDeps(scratch) + if !ok { + return + } + lock, written = withoutProjectLink(lock, written, root, scratch) + var parsed packageLockFile + if json.Unmarshal(lock, &parsed) != nil { + return + } + // The lockfile must describe the package.json the pass ended with, which + // carries the packages the command named. One that does not is a lockfile npm + // would resolve further, and so not one to install from. + if !lockMatchesManifest(parsed, written) { + LogDetail("The lockfile npm wrote does not match the package.json it ended with, so the install resolves again.") + return + } + if path := firstUnhashedEntry(parsed); path != "" { + LogDetail("The lockfile npm wrote holds %s, which is not a tarball with a hash, so the install resolves again.", path) + return + } + *r = resolvedInstall{ + root: root, lock: lock, lockfile: parsed, + manifest: given["package.json"], projectLock: given["package-lock.json"], + } +} + +// withoutProjectLink takes out of the lockfile and the package.json the pass +// wrote what npm 7 to 10 add to an install that names no package, when it is run +// from the project with another folder as its prefix, which is how the pass runs. +// They install the project into that folder as a file: dependency, so the +// lockfile gains the project's own files as an entry, a link to it, and a +// dependency on it. Measured 2026-10-08, npm 7.24.2, 8.19.4, 9.9.4, 10.9.3 and +// 10.9.9 do this and 11.11.0 does not. With 10.9.9 and a project of 321 packages +// the rest of the lockfile was the same as one resolved on its own. The project +// is found by its path, the one the dependency's file: spec names. Nothing else +// is changed, and a lockfile without it comes back as it was. +func withoutProjectLink(lock []byte, written manifestDeps, root, scratch string) ([]byte, manifestDeps) { + rel, err := filepath.Rel(scratch, root) + if err != nil { + return lock, written + } + rel = filepath.ToSlash(rel) + name := "" + for dep, spec := range written.Dependencies { + if spec == "file:"+rel { + name = dep + } + } + if name == "" { + return lock, written + } + + var top map[string]json.RawMessage + var packages map[string]json.RawMessage + var rootEntry map[string]json.RawMessage + var rootDeps map[string]string + if json.Unmarshal(lock, &top) != nil || json.Unmarshal(top["packages"], &packages) != nil || + json.Unmarshal(packages[""], &rootEntry) != nil || json.Unmarshal(rootEntry["dependencies"], &rootDeps) != nil || + rootDeps[name] != "file:"+rel { + return lock, written + } + // The link is what npm writes for it. Anything else by that name is not this. + if raw, there := packages["node_modules/"+name]; there { + var link struct { + Link bool `json:"link"` + Resolved string `json:"resolved"` + } + if json.Unmarshal(raw, &link) != nil || !link.Link || link.Resolved != rel { + return lock, written + } + } + delete(rootDeps, name) + delete(packages, rel) + delete(packages, "node_modules/"+name) + var ok bool + if rootEntry["dependencies"], ok = marshalRaw(rootDeps); !ok { + return lock, written + } + if packages[""], ok = marshalRaw(rootEntry); !ok { + return lock, written + } + if top["packages"], ok = marshalRaw(packages); !ok { + return lock, written + } + // lockfileVersion 2 repeats the tree in the older form npm 6 reads. + var legacy map[string]json.RawMessage + if json.Unmarshal(top["dependencies"], &legacy) == nil { + delete(legacy, name) + if top["dependencies"], ok = marshalRaw(legacy); !ok { + return lock, written + } + } + out, err := json.MarshalIndent(top, "", " ") + if err != nil { + return lock, written + } + + clean := written + clean.Dependencies = map[string]string{} + for dep, spec := range written.Dependencies { + if dep != name { + clean.Dependencies[dep] = spec + } + } + return out, clean +} + +func marshalRaw(v any) (json.RawMessage, bool) { + b, err := json.Marshal(v) + return b, err == nil +} + +// firstUnhashedEntry returns the path of a package the lockfile lists that is not +// a tarball fetched over HTTP with a hash to check it against, or "" when every +// one is. Those are the entries the checks hold to the registry's record of +// their name and version. A link, a git source, a local folder and a package +// bundled inside another have no such record, so a lockfile with one is left +// for npm to resolve as it did. +func firstUnhashedEntry(lock packageLockFile) string { + var first string + for path, e := range lock.Packages { + if path == "" { + continue + } + resolved := strings.ToLower(e.Resolved) + if e.Link || e.Integrity == "" || !(strings.HasPrefix(resolved, "https://") || strings.HasPrefix(resolved, "http://")) { + // The least path, so that the same lockfile always names the same one. + if first == "" || path < first { + first = path + } + } + } + return first +} + +// versions maps each package at the top of the lockfile's tree to its version, +// leaving out links and aliases, whose names do not say what they install. +func (r *resolvedInstall) versions() map[string]string { + out := map[string]string{} + for path, e := range r.lockfile.Packages { + name, ok := strings.CutPrefix(path, "node_modules/") + if !ok || strings.Contains(name, "node_modules/") || e.Link || e.Name != "" || e.Version == "" { + continue + } + out[name] = e.Version + } + return out +} + +// adopt puts the resolved lockfile in the project and returns the command to run +// in place of args, with a function to call once it has finished. When the +// command is not one the lockfile can stand in for, or the project is not as the +// pass found it, it returns args and a nil function and nothing has changed. +// +// finish takes the lockfile out again if npm did not write it. A lockfile npm +// ends up using is rewritten by npm whatever happens to the install, so a file +// still dated as it was left is a file npm did not use. That is what a run that +// stops before npm saves leaves, and so do a project whose settings turn the +// lockfile off and an install with --no-save. Native npm leaves the lockfile +// alone in all of them, and so does this. +func (r *resolvedInstall) adopt(cmdName string, args []string, pmCmd string, pmArgs []string) ([]string, func()) { + if !adoptCheckedLockfile || r == nil || len(r.lock) == 0 { + return args, nil + } + // The command nvx resolved is the command it will run. A package manager + // reached through corepack or its entry script has not been read the same way. + if !strings.EqualFold(cmdName, "npm") || !strings.EqualFold(pmCmd, "npm") || !slices.Equal(args, pmArgs) { + return args, nil + } + read, _ := readCommand("npm", args) + verb := commandVerbIndex(read, npmResolveVerbs...) + if verb < 0 || len(read) != len(args) { + return args, nil + } + if v := strings.ToLower(read[verb]); v != "install" && v != "install-test" { + LogDetail("npm %s resolves the tree itself, so it does that again after the checks.", v) + return args, nil + } + run, ok := pinInstallArgs(args, verb, r.versions()) + if !ok { + LogDetail("npm resolves this install again after the checks, because it names a package or a flag that cannot be pinned to the checked lockfile.") + return args, nil + } + if !r.projectUnchanged() { + LogDetail("The project changed while its install was being checked, so npm resolves it again.") + return args, nil + } + s, ok := stageLockfile(filepath.Join(r.root, "package-lock.json"), r.lock) + if !ok { + return args, nil + } + LogDetail("Installing from the lockfile the checks were run on, so what is installed is what was checked.") + return run, s.finish +} + +// projectUnchanged reports whether the project's package.json and lockfile are +// the bytes the resolution pass was given, and there is still no shrinkwrap. +func (r *resolvedInstall) projectUnchanged() bool { + same := func(name string, given []byte) bool { + now, err := os.ReadFile(filepath.Join(r.root, name)) + if errors.Is(err, os.ErrNotExist) { + return given == nil + } + return err == nil && given != nil && bytes.Equal(now, given) + } + return same("package.json", r.manifest) && same("package-lock.json", r.projectLock) && same("npm-shrinkwrap.json", nil) +} + +// stagedLockfile is a lockfile put in the project before an install. +type stagedLockfile struct { + path string + content []byte + // modTime is the date the file was left with, which only npm writing it can + // change. + modTime time.Time + // What was there before. + existed bool + prev []byte + mode os.FileMode + prevMod time.Time +} + +// stagedLockfileDate is the date a staged lockfile is left with. A fixed, plain, +// old date, so that a file npm has written cannot be mistaken for one it has not. +var stagedLockfileDate = time.Date(2001, time.January, 1, 0, 0, 0, 0, time.UTC) + +// stageLockfile writes content to path, over a lockfile that is there already, +// and leaves it dated stagedLockfileDate. +func stageLockfile(path string, content []byte) (*stagedLockfile, bool) { + s := &stagedLockfile{path: path, content: content, mode: 0o644} + // Lstat, so that a lockfile which is a link is left to npm as it was. + if info, err := os.Lstat(path); err == nil { + prev, rerr := os.ReadFile(path) + if rerr != nil || !info.Mode().IsRegular() { + return nil, false + } + s.existed, s.prev, s.mode, s.prevMod = true, prev, info.Mode().Perm(), info.ModTime() + } else if !errors.Is(err, os.ErrNotExist) { + return nil, false + } + if err := os.WriteFile(path, content, s.mode); err != nil { + return nil, false + } + if err := os.Chtimes(path, stagedLockfileDate, stagedLockfileDate); err != nil { + s.restore() + return nil, false + } + info, err := os.Stat(path) + if err != nil { + s.restore() + return nil, false + } + s.modTime = info.ModTime() + return s, true +} + +// finish puts the project's lockfile back as it was if npm did not write the +// staged one. A file changed by anything else is left alone. +func (s *stagedLockfile) finish() { + info, err := os.Stat(s.path) + if err != nil || !info.ModTime().Equal(s.modTime) || info.Size() != int64(len(s.content)) { + return + } + if now, err := os.ReadFile(s.path); err != nil || !bytes.Equal(now, s.content) { + return + } + s.restore() +} + +// restore puts back what was there before the staged file, or removes it. +func (s *stagedLockfile) restore() { + if !s.existed { + _ = os.Remove(s.path) + return + } + if err := os.WriteFile(s.path, s.prev, s.mode); err == nil { + _ = os.Chtimes(s.path, s.prevMod, s.prevMod) + } +} + +var ( + // A package name as the registry has accepted them for years. Anything else + // is left for npm to read. + pinnableName = regexp.MustCompile(`^(@[a-z0-9][a-z0-9._~-]*/)?[a-z0-9][a-z0-9._~-]*$`) + exactVersion = regexp.MustCompile(`^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$`) + // A dist-tag such as latest or next. A tag with a digit in it could be read + // as a range, so it is not one this pins. + distTagName = regexp.MustCompile(`^[a-z][a-z-]+$`) +) + +// npmInstallFlagsWithoutValue are the install flags that take no value, so that +// the token after one is a package and not the flag's argument. A flag that is +// not here and is not written --flag=value might swallow the next token, and +// then nvx cannot tell what the packages are. +var npmInstallFlagsWithoutValue = map[string]bool{ + "-D": true, "--save-dev": true, "-P": true, "--save-prod": true, "-O": true, + "--save-optional": true, "--save-peer": true, "-E": true, "--save-exact": true, + "-B": true, "--save-bundle": true, "-S": true, "--save": true, "--no-save": true, + "--audit": true, "--no-audit": true, "--fund": true, "--no-fund": true, + "-f": true, "--force": true, "--legacy-peer-deps": true, "--strict-peer-deps": true, + "--ignore-scripts": true, "--foreground-scripts": true, "--prefer-offline": true, + "--prefer-online": true, "--offline": true, "--no-optional": true, "--production": true, + "--no-bin-links": true, "--install-links": true, "--global-style": true, + "--legacy-bundling": true, "--no-progress": true, "--no-update-notifier": true, + "--silent": true, "-s": true, "--quiet": true, "-q": true, "--verbose": true, + "-d": true, "-dd": true, "-ddd": true, "--no-color": true, "--dry-run": true, + "--no-package-lock": true, +} + +// pinInstallArgs returns args with each package it names written as an exact +// version taken from versions, and whether that could be done for all of them. +// verb is where npm's command sits in args. +// +// npm saves a package named without a version, or with a tag, as a range around +// the version it installed, and saves an exact version the same way. Naming the +// version the lockfile holds therefore saves what the original command would +// have saved. A range is saved as it was typed, which pinning would change. +func pinInstallArgs(args []string, verb int, versions map[string]string) ([]string, bool) { + out := slices.Clone(args) + for i := verb + 1; i < len(args); i++ { + a := args[i] + switch { + case a == "--": + return nil, false + case strings.HasPrefix(a, "-"): + if !strings.Contains(a, "=") && !npmInstallFlagsWithoutValue[a] { + return nil, false + } + default: + pinned, ok := pinSpec(a, versions) + if !ok { + return nil, false + } + out[i] = pinned + } + } + return out, true +} + +// pinSpec writes one named package as name@version, or reports that it cannot. +func pinSpec(spec string, versions map[string]string) (string, bool) { + name, rest := spec, "" + if at := strings.LastIndex(spec, "@"); at > 0 { + name, rest = spec[:at], spec[at+1:] + } + if !pinnableName.MatchString(name) { + return "", false + } + version, ok := versions[name] + if !ok { + return "", false + } + switch { + case rest == "" || distTagName.MatchString(rest): + return name + "@" + version, true + case exactVersion.MatchString(rest) && rest == version: + return spec, true + } + return "", false +} diff --git a/internal/nvx/npm_resolved_install_npm_test.go b/internal/nvx/npm_resolved_install_npm_test.go new file mode 100644 index 00000000..71e81093 --- /dev/null +++ b/internal/nvx/npm_resolved_install_npm_test.go @@ -0,0 +1,367 @@ +package nvx + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" +) + +// What npm installs, against a registry that changes between its two runs. +// +// These run the real npm that is on the machine, uncontained, against the fake +// registry in npm_fake_registry_test.go. The registry has app-dep 1.0.0 when nvx +// runs npm's resolver and the checks, and after the resolver has finished it +// publishes app-dep, leaf-dep and extra-dep 1.0.1, the way a registry changes +// while a person waits. The tree that was checked holds 1.0.0 of each. An +// install that resolves again gets 1.0.1. + +// realNodeDir finds a directory holding a real node and npm, skipping any +// directory nvx's own shims are linked in, which would run the wrong program. +func realNodeDir(t *testing.T) string { + t.Helper() + npm, node := "npm", "node" + if runtime.GOOS == "windows" { + npm, node = "npm.cmd", "node.exe" + } + for _, dir := range filepath.SplitList(os.Getenv("PATH")) { + if dir == "" { + continue + } + if _, err := os.Stat(filepath.Join(dir, nvxExecutableName())); err == nil { + continue + } + if _, err := os.Stat(filepath.Join(dir, npm)); err != nil { + continue + } + if _, err := os.Stat(filepath.Join(dir, node)); err != nil { + continue + } + return dir + } + t.Skip("no node and npm outside nvx's own shims on PATH") + return "" +} + +// isolateNpm gives npm nothing of this machine's: its own config files, cache and +// proxy settings, and a PATH that holds only node and npm. +func isolateNpm(t *testing.T, nodeDir string) { + t.Helper() + t.Setenv("PATH", nodeDir) + // Two files, because npm refuses to read one as both. + configs := tempDir(t) + for _, name := range []string{"user.npmrc", "global.npmrc"} { + if err := os.WriteFile(filepath.Join(configs, name), nil, 0o600); err != nil { + t.Fatal(err) + } + } + t.Setenv("npm_config_userconfig", filepath.Join(configs, "user.npmrc")) + t.Setenv("npm_config_globalconfig", filepath.Join(configs, "global.npmrc")) + t.Setenv("npm_config_cache", tempDir(t)) + t.Setenv("npm_config_update_notifier", "false") + t.Setenv("npm_config_audit", "false") + t.Setenv("npm_config_fund", "false") + for _, k := range []string{"NPM_CONFIG_PREFIX", "NPM_CONFIG_REGISTRY", "HTTP_PROXY", "HTTPS_PROXY", "http_proxy", "https_proxy", "ALL_PROXY"} { + unsetEnv(t, k) + } + t.Setenv("NO_PROXY", "127.0.0.1") + unsetEnv(t, nvxActiveEnvVar) +} + +// An install project: a folder holding these files and an .npmrc that sends npm +// to the fake registry, followed by any .npmrc the files hold. +func writeInstallProject(t *testing.T, reg *fakeNpmRegistry, files map[string]string) string { + t.Helper() + dir := tempDir(t) + files = mergeFiles(files, map[string]string{".npmrc": "registry=" + reg.url() + "\n" + files[".npmrc"]}) + for name, body := range files { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + return dir +} + +func mergeFiles(a, b map[string]string) map[string]string { + out := map[string]string{} + for k, v := range a { + out[k] = v + } + for k, v := range b { + out[k] = v + } + return out +} + +// publishFirstVersions is what the registry holds while the install is checked. +func publishFirstVersions(reg *fakeNpmRegistry) { + reg.reset() + reg.publish("leaf-dep", "1.0.0", nil) + reg.publish("app-dep", "1.0.0", map[string]string{"leaf-dep": "^1.0.0"}) + reg.publish("extra-dep", "1.0.0", nil) +} + +// publishNewVersions is what it holds once the checks are done. +func publishNewVersions(reg *fakeNpmRegistry) { + reg.publish("leaf-dep", "1.0.1", nil) + reg.publish("app-dep", "1.0.1", map[string]string{"leaf-dep": "^1.0.0"}) + reg.publish("extra-dep", "1.0.1", nil) +} + +// installedVersions reads the version of each named package in the project's +// node_modules, "" for one that is not there. +func installedVersions(t *testing.T, dir string, names ...string) map[string]string { + t.Helper() + out := map[string]string{} + for _, name := range names { + data, err := os.ReadFile(filepath.Join(dir, "node_modules", name, "package.json")) + if err != nil { + out[name] = "" + continue + } + var m struct{ Version string } + if err := json.Unmarshal(data, &m); err != nil { + t.Fatalf("%s/package.json: %v", name, err) + } + out[name] = m.Version + } + return out +} + +// installOutcome is what a run of npm left in the project. +type installOutcome struct { + exit int + manifest string + lock string + versions map[string]string + output string +} + +var installedNames = []string{"app-dep", "leaf-dep", "extra-dep"} + +func readOutcome(t *testing.T, dir string, exit int, output string) installOutcome { + t.Helper() + read := func(name string) string { + data, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil { + return "" + } + return string(data) + } + return installOutcome{exit: exit, manifest: read("package.json"), lock: read("package-lock.json"), + versions: installedVersions(t, dir, installedNames...), output: output} +} + +// runNativeNpm runs npm as it is, with nvx nowhere, in a project made from files +// against the registry as it is now. +func runNativeNpm(t *testing.T, nodeDir string, reg *fakeNpmRegistry, files map[string]string, args ...string) installOutcome { + t.Helper() + dir := writeInstallProject(t, reg, files) + npm := "npm" + if runtime.GOOS == "windows" { + npm = filepath.Join(nodeDir, "npm.cmd") + } + cmd := exec.Command(npm, args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + exit := 0 + if err != nil { + exit = 1 + if ee, ok := err.(*exec.ExitError); ok { + exit = ee.ExitCode() + } + } + return readOutcome(t, dir, exit, string(out)) +} + +// runInstallThroughNvx runs the same command through runShim, which checks it +// first and then installs it. After npm's resolver has finished and before the +// install starts, the registry publishes its new versions. It returns the +// outcome and what npm asked the registry for once the new versions were out. +func runInstallThroughNvx(t *testing.T, reg *fakeNpmRegistry, files map[string]string, args ...string) (installOutcome, []fakeNpmRequest) { + t.Helper() + home := tempDir(t) + if err := os.WriteFile(filepath.Join(home, "policy.json"), []byte(`{"isolation":{"enabled":false},"typosquatting":{"enabled":false}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(home, "popular_packages.json"), []byte(`["react","lodash"]`), 0o600); err != nil { + t.Fatal(err) + } + dir := writeInstallProject(t, reg, files) + inProjectDir(t, dir) + t.Setenv("NVX_NONINTERACTIVE", "1") + t.Setenv("NVX_YES", "") + packuments.Range(func(k, _ any) bool { packuments.Delete(k); return true }) + + published := -1 + orig := launchNpmResolution + t.Cleanup(func() { launchNpmResolution = orig }) + launchNpmResolution = func(cfg SandboxConfig, contain bool) int { + code := orig(cfg, contain) + if code == 0 { + publishNewVersions(reg) + published = len(reg.requests()) + } + return code + } + + var code int + var out string + stderr := captureStderrHere(t, func() { + out = captureStdout(t, func() { code = runShim("npm", args, home) }) + }) + if published < 0 { + t.Fatalf("npm's resolver never ran, so the registry never changed:\n%s", stderr) + } + var after []fakeNpmRequest + for _, r := range reg.requests()[published:] { + if r.byNpm() { + after = append(after, r) + } + } + return readOutcome(t, dir, code, out+stderr), after +} + +type installScenario struct { + name string + files map[string]string + args []string + // packuments is how many metadata requests npm may make once the registry + // has changed. A bare install makes none. Each named package is asked + // about once, because npm reads a package it was told to install as a + // request, whatever the lockfile says. + packuments int +} + +func installScenarios() []installScenario { + app := `{"name":"app","version":"1.0.0","dependencies":{"app-dep":"^1.0.0"}}` + return []installScenario{ + { + name: "bare install with no lockfile", + files: map[string]string{"package.json": app}, + args: []string{"install"}, + }, + { + name: "install of a name, a tag and -D", + files: map[string]string{"package.json": `{"name":"app","version":"1.0.0"}`}, + args: []string{"install", "-D", "app-dep@latest", "extra-dep"}, + packuments: 2, + }, + } +} + +// The tree npm installs is the tree that was checked, and the project is left +// the way npm would have left it. The registry changes between npm's resolver and +// the install, so an install that resolved again would get the new versions, and +// the project's package.json and lockfile would say so. +func TestAnInstallGetsTheTreeThatWasChecked(t *testing.T) { + nodeDir := realNodeDir(t) + for _, sc := range installScenarios() { + t.Run(sc.name, func(t *testing.T) { + isolateNpm(t, nodeDir) + reg := newFakeNpmRegistry(t) + + // What npm leaves when the registry holds the first versions throughout. + publishFirstVersions(reg) + native := runNativeNpm(t, nodeDir, reg, sc.files, sc.args...) + if native.exit != 0 { + t.Fatalf("npm on its own failed:\n%s", native.output) + } + + publishFirstVersions(reg) + got, asked := runInstallThroughNvx(t, reg, sc.files, sc.args...) + if got.exit != 0 { + t.Fatalf("the install exited %d:\n%s", got.exit, got.output) + } + if got.manifest != native.manifest { + t.Errorf("package.json is not what npm leaves:\n%s\nnpm leaves:\n%s", got.manifest, native.manifest) + } + if got.lock != native.lock { + t.Errorf("package-lock.json is not what npm leaves:\n%s\nnpm leaves:\n%s", got.lock, native.lock) + } + for name, want := range native.versions { + if got.versions[name] != want { + t.Errorf("%s is installed at %q, and the tree that was checked holds %q", name, got.versions[name], want) + } + } + var packuments int + for _, r := range asked { + if r.forPackument() { + packuments++ + } + } + if packuments > sc.packuments { + t.Errorf("npm asked the registry for %d packages' metadata after the checks, want at most %d: %v", packuments, sc.packuments, asked) + } + }) + } +} + +// A control for the test above. With the lockfile not handed over, the same +// install resolves again and gets what the registry holds by then, so the test +// above can fail, and does when the install resolves afresh. +func TestWithoutTheHandoffAnInstallGetsTheNewVersions(t *testing.T) { + nodeDir := realNodeDir(t) + isolateNpm(t, nodeDir) + reg := newFakeNpmRegistry(t) + + orig := adoptCheckedLockfile + t.Cleanup(func() { adoptCheckedLockfile = orig }) + adoptCheckedLockfile = false + + for _, sc := range installScenarios() { + t.Run(sc.name, func(t *testing.T) { + publishFirstVersions(reg) + got, _ := runInstallThroughNvx(t, reg, sc.files, sc.args...) + if got.exit != 0 { + t.Fatalf("the install exited %d:\n%s", got.exit, got.output) + } + if got.versions["app-dep"] != "1.0.1" { + t.Fatalf("app-dep is installed at %q: the registry changed and the install did not see it, so the test above shows nothing\n%s", got.versions["app-dep"], got.output) + } + }) + } +} + +// A project whose settings leave the lockfile alone, or a command that does not +// save, ends up as it would with npm alone, and the lockfile nvx handed over is +// not left behind. +func TestTheHandedOverLockfileIsNotLeftWhereNpmWouldNotWriteOne(t *testing.T) { + nodeDir := realNodeDir(t) + app := `{"name":"app","version":"1.0.0","dependencies":{"app-dep":"^1.0.0"}}` + cases := []struct { + name string + files map[string]string + args []string + }{ + // npm reads no lockfile and writes none. + {"package-lock=false in .npmrc", map[string]string{"package.json": app, ".npmrc": "package-lock=false\n"}, []string{"install"}}, + // npm reads the lockfile and writes none. + {"--no-save", map[string]string{"package.json": app}, []string{"install", "--no-save"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + isolateNpm(t, nodeDir) + reg := newFakeNpmRegistry(t) + publishFirstVersions(reg) + native := runNativeNpm(t, nodeDir, reg, tc.files, tc.args...) + if native.exit != 0 { + t.Fatalf("npm on its own failed:\n%s", native.output) + } + + publishFirstVersions(reg) + got, _ := runInstallThroughNvx(t, reg, tc.files, tc.args...) + if got.exit != 0 { + t.Fatalf("the install exited %d:\n%s", got.exit, got.output) + } + if got.manifest != native.manifest { + t.Errorf("package.json is not what npm leaves:\n%s\nnpm leaves:\n%s", got.manifest, native.manifest) + } + if got.lock != native.lock { + t.Errorf("package-lock.json is not what npm leaves:\n%q\nnpm leaves:\n%q", got.lock, native.lock) + } + }) + } +} diff --git a/internal/nvx/npm_resolved_install_perm_unix_test.go b/internal/nvx/npm_resolved_install_perm_unix_test.go new file mode 100644 index 00000000..8582cc5b --- /dev/null +++ b/internal/nvx/npm_resolved_install_perm_unix_test.go @@ -0,0 +1,30 @@ +//go:build !windows + +package nvx + +import ( + "os" + "testing" +) + +// A lockfile that is there keeps its permissions through staging and back. +// Windows keeps no permission bits to keep, so this is not run there. +func TestStagingKeepsThePermissionsOfALockfileThatIsThere(t *testing.T) { + root := tempDir(t) + if err := os.WriteFile(lockfileAt(root), []byte(`{}`), 0o640); err != nil { + t.Fatal(err) + } + s, ok := stageLockfile(lockfileAt(root), []byte(`{"staged":true}`)) + if !ok { + t.Fatal("not staged") + } + info, _ := os.Stat(lockfileAt(root)) + if info.Mode().Perm() != 0o640 { + t.Errorf("staged lockfile is %v, want 0640", info.Mode().Perm()) + } + s.finish() + info, _ = os.Stat(lockfileAt(root)) + if info.Mode().Perm() != 0o640 { + t.Errorf("restored lockfile is %v, want 0640", info.Mode().Perm()) + } +} diff --git a/internal/nvx/npm_resolved_install_project_link_test.go b/internal/nvx/npm_resolved_install_project_link_test.go new file mode 100644 index 00000000..9a3756ed --- /dev/null +++ b/internal/nvx/npm_resolved_install_project_link_test.go @@ -0,0 +1,131 @@ +package nvx + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// The pass runs from the project with a folder inside it as its prefix. For an +// install that names nothing, npm 7 to 10 then install the project into that +// folder as a file: dependency, and the lockfile it writes lists the project's own +// files, a link to them and a dependency on them. None of it belongs in the +// lockfile the install starts from, where the folder is not there. Measured +// 2026-10-08 with 7.24.2, 8.19.4, 9.9.4, 10.9.3 and 10.9.9. 11.11.0 does not do it. + +const ( + linkedManifest = `{"name":"app","version":"1.0.0","dependencies":{"app":"file:../../..","foo":"^1.0.0"}}` + // What npm 10 wrote for it, lockfileVersion 3. + linkedLockV3 = `{"name":"app","version":"1.0.0","lockfileVersion":3,"requires":true,"packages":{` + + `"":{"name":"app","version":"1.0.0","dependencies":{"app":"file:../../..","foo":"^1.0.0"}},` + + `"../../..":{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}},` + + `"node_modules/app":{"resolved":"../../..","link":true},` + + `"node_modules/foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA"}}}` + // And npm 7 and 8, lockfileVersion 2, which repeats the tree the old way. + linkedLockV2 = `{"name":"app","version":"1.0.0","lockfileVersion":2,"requires":true,"packages":{` + + `"":{"name":"app","version":"1.0.0","dependencies":{"app":"file:../../..","foo":"^1.0.0"}},` + + `"../../..":{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}},` + + `"node_modules/app":{"resolved":"../../..","link":true},` + + `"node_modules/foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA"}},` + + `"dependencies":{"app":{"version":"file:../../..","requires":{"foo":"^1.0.0"}},` + + `"foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA"}}}` +) + +// linkedFixture puts the scratch folder where the pass puts it, inside the project. +func linkedFixture(t *testing.T, project, written map[string]string) *resolvedFixture { + t.Helper() + f := newResolvedFixture(t, project, nil) + f.scratch = filepath.Join(f.root, "node_modules", ".cache", "nvx-resolve-test") + if err := os.MkdirAll(f.scratch, 0o755); err != nil { + t.Fatal(err) + } + for name, body := range written { + if err := os.WriteFile(filepath.Join(f.scratch, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + return f +} + +func TestTheProjectIsLeftOutOfTheLockfileTheInstallStartsFrom(t *testing.T) { + for name, lock := range map[string]string{"lockfileVersion 3": linkedLockV3, "lockfileVersion 2": linkedLockV2} { + t.Run(name, func(t *testing.T) { + f := linkedFixture(t, map[string]string{"package.json": `{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}}`}, + map[string]string{"package.json": linkedManifest, "package-lock.json": lock}) + r := f.recorded() + if len(r.lock) == 0 { + t.Fatal("the lockfile was not kept") + } + for _, gone := range []string{"../../..", "node_modules/app", "file:"} { + if strings.Contains(string(r.lock), gone) { + t.Errorf("the lockfile still holds %q:\n%s", gone, r.lock) + } + } + var parsed struct { + Packages map[string]struct { + Dependencies map[string]string `json:"dependencies"` + Version string `json:"version"` + } `json:"packages"` + } + if err := json.Unmarshal(r.lock, &parsed); err != nil { + t.Fatalf("the lockfile does not read: %v\n%s", err, r.lock) + } + if got := parsed.Packages[""].Dependencies; len(got) != 1 || got["foo"] != "^1.0.0" { + t.Errorf("the project's dependencies are %v, want only foo ^1.0.0", got) + } + if got := parsed.Packages["node_modules/foo"].Version; got != "1.2.3" { + t.Errorf("foo is at %q, want 1.2.3", got) + } + if got := r.versions(); got["foo"] != "1.2.3" || len(got) != 1 { + t.Errorf("versions = %v, want only foo 1.2.3", got) + } + + // A lockfile with the project taken out is one a bare install can start from. + run, finish := r.adopt("npm", []string{"install"}, "npm", []string{"install"}) + if finish == nil { + t.Fatal("not adopted") + } + defer finish() + if len(run) != 1 { + t.Errorf("ran %v", run) + } + }) + } +} + +// A lockfile npm 11 wrote has nothing to take out and is kept as it is. +func TestALockfileWithoutTheProjectIsKeptAsItIs(t *testing.T) { + clean := `{"name":"app","version":"1.0.0","lockfileVersion":3,"requires":true,"packages":{` + + `"":{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}},` + + `"node_modules/foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA"}}}` + f := linkedFixture(t, map[string]string{"package.json": `{"name":"app"}`}, + map[string]string{"package.json": `{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}}`, "package-lock.json": clean}) + if got := string(f.recorded().lock); got != clean { + t.Fatalf("the lockfile was changed:\n%s", got) + } +} + +// A dependency on a folder that is not the project is not this, and neither is a +// package of the same name that is not a link. Those lockfiles are not kept. +func TestOnlyTheProjectItselfIsTakenOut(t *testing.T) { + for name, tc := range map[string]struct{ manifest, lock string }{ + "a file: dependency on another folder": { + `{"name":"app","version":"1.0.0","dependencies":{"other":"file:../elsewhere","foo":"^1.0.0"}}`, + strings.NewReplacer(`"app":"file:../../.."`, `"other":"file:../elsewhere"`, `"node_modules/app"`, `"node_modules/other"`).Replace(linkedLockV3), + }, + "a package of that name that is not a link": { + linkedManifest, + strings.Replace(linkedLockV3, `{"resolved":"../../..","link":true}`, `{"version":"1.0.0","resolved":"https://registry.npmjs.org/app/-/app-1.0.0.tgz","integrity":"sha512-BBBB"}`, 1), + }, + } { + t.Run(name, func(t *testing.T) { + f := linkedFixture(t, map[string]string{"package.json": `{"name":"app"}`}, + map[string]string{"package.json": tc.manifest, "package-lock.json": tc.lock}) + if r := f.recorded(); len(r.lock) != 0 { + t.Fatalf("kept a lockfile that was not the project's alone:\n%s", r.lock) + } + }) + } +} diff --git a/internal/nvx/npm_resolved_install_test.go b/internal/nvx/npm_resolved_install_test.go new file mode 100644 index 00000000..2101383b --- /dev/null +++ b/internal/nvx/npm_resolved_install_test.go @@ -0,0 +1,367 @@ +package nvx + +import ( + "bytes" + "os" + "path/filepath" + "reflect" + "testing" + "time" +) + +// resolvedFixture is a project with a resolution pass already run on it: the +// project's own files, and a scratch folder holding what the pass wrote. +type resolvedFixture struct { + root, scratch string + given map[string][]byte +} + +func newResolvedFixture(t *testing.T, project map[string]string, written map[string]string) *resolvedFixture { + t.Helper() + f := &resolvedFixture{root: tempDir(t), scratch: tempDir(t), given: map[string][]byte{}} + for name, body := range project { + if err := os.WriteFile(filepath.Join(f.root, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + f.given[name] = []byte(body) + } + for name, body := range written { + if err := os.WriteFile(filepath.Join(f.scratch, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + return f +} + +func (f *resolvedFixture) recorded() *resolvedInstall { + r := &resolvedInstall{} + r.record(f.root, f.scratch, f.given) + return r +} + +// A lock for package.json {"dependencies":{"foo":"^1.0.0"}} holding foo and, one +// level down, the leaf it brings in. +const ( + fooManifest = `{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}}` + fooLock = `{"name":"app","version":"1.0.0","lockfileVersion":3,"requires":true,"packages":{` + + `"":{"name":"app","version":"1.0.0","dependencies":{"foo":"^1.0.0"}},` + + `"node_modules/foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA","dependencies":{"leaf":"^2.0.0"}},` + + `"node_modules/leaf":{"version":"2.1.0","resolved":"https://registry.npmjs.org/leaf/-/leaf-2.1.0.tgz","integrity":"sha512-BBBB"},` + + `"node_modules/@s/aliased":{"name":"real","version":"3.0.0","resolved":"https://registry.npmjs.org/real/-/real-3.0.0.tgz","integrity":"sha512-CCCC"},` + + `"node_modules/foo/node_modules/nested":{"version":"9.9.9","resolved":"https://registry.npmjs.org/nested/-/nested-9.9.9.tgz","integrity":"sha512-DDDD"}}}` +) + +func TestOnlyTheTopOfTheTreeCanBePinned(t *testing.T) { + f := newResolvedFixture(t, map[string]string{"package.json": `{"name":"app"}`}, + map[string]string{"package.json": fooManifest, "package-lock.json": fooLock}) + got := f.recorded().versions() + want := map[string]string{"foo": "1.2.3", "leaf": "2.1.0"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("versions = %v, want %v (an alias, whose name is not what it installs, and a nested copy are not pinnable)", got, want) + } +} + +func TestWhichInstallsCanBePinned(t *testing.T) { + versions := map[string]string{"foo": "1.2.3", "bar": "0.4.0", "@scope/baz": "5.0.0-beta.1"} + cases := []struct { + name string + args []string + want []string // nil when the command cannot be pinned + }{ + {"bare install", []string{"install"}, []string{"install"}}, + {"bare install with flags", []string{"i", "--omit=dev", "--no-audit"}, []string{"i", "--omit=dev", "--no-audit"}}, + {"a name", []string{"install", "foo"}, []string{"install", "foo@1.2.3"}}, + {"a tag", []string{"install", "foo@latest"}, []string{"install", "foo@1.2.3"}}, + {"a tag on a scoped name", []string{"add", "@scope/baz@next", "-E"}, []string{"add", "@scope/baz@5.0.0-beta.1", "-E"}}, + {"the exact version the lockfile holds", []string{"install", "foo@1.2.3"}, []string{"install", "foo@1.2.3"}}, + {"-D and two names", []string{"install", "-D", "foo", "bar"}, []string{"install", "-D", "foo@1.2.3", "bar@0.4.0"}}, + {"a flag with its value attached", []string{"install", "--registry=https://r.example/", "foo"}, []string{"install", "--registry=https://r.example/", "foo@1.2.3"}}, + + // A range is saved as it was typed, and pinning it would save something else. + {"a caret range", []string{"install", "foo@^1.0.0"}, nil}, + {"a partial version", []string{"install", "foo@1"}, nil}, + {"another exact version", []string{"install", "foo@1.2.4"}, nil}, + // npm reads these as other things than the name. + {"an alias", []string{"install", "foo@npm:bar@1"}, nil}, + {"a git source", []string{"install", "foo@github:someone/foo"}, nil}, + {"a path", []string{"install", "./foo"}, nil}, + {"a URL", []string{"install", "https://example.invalid/foo.tgz"}, nil}, + {"a tag with a digit", []string{"install", "foo@next-14"}, nil}, + {"a name with capitals", []string{"install", "Foo"}, nil}, + {"a package the lockfile does not hold", []string{"install", "unknown"}, nil}, + // A flag that might take the next token as its value leaves the packages unclear. + {"a flag with a separate value", []string{"install", "--registry", "https://r.example/", "foo"}, nil}, + {"an unknown flag", []string{"install", "--made-up", "foo"}, nil}, + {"after --", []string{"install", "--", "foo"}, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + verb := commandVerbIndex(tc.args, npmResolveVerbs...) + got, ok := pinInstallArgs(tc.args, verb, versions) + if tc.want == nil { + if ok { + t.Fatalf("pinned %v to %v, want it left alone", tc.args, got) + } + return + } + if !ok || !reflect.DeepEqual(got, tc.want) { + t.Fatalf("pinned %v to %v (ok=%v), want %v", tc.args, got, ok, tc.want) + } + }) + } +} + +func TestPinningLeavesTheTypedArgumentsAlone(t *testing.T) { + args := []string{"install", "foo"} + if _, ok := pinInstallArgs(args, 0, map[string]string{"foo": "1.2.3"}); !ok { + t.Fatal("not pinned") + } + if !reflect.DeepEqual(args, []string{"install", "foo"}) { + t.Fatalf("the caller's arguments were changed to %v", args) + } +} + +func TestOnlyAnInstallTheLockfileCanStandInForIsAdopted(t *testing.T) { + project := map[string]string{"package.json": `{"name":"app","version":"1.0.0"}`} + written := map[string]string{"package.json": fooManifest, "package-lock.json": fooLock} + + cases := []struct { + name string + cmd string + args []string + pmCmd string + pmArgs []string + adopted bool + want []string + }{ + {"npm install foo", "npm", []string{"install", "foo"}, "npm", []string{"install", "foo"}, true, []string{"install", "foo@1.2.3"}}, + {"an abbreviated verb", "npm", []string{"inst", "foo"}, "npm", []string{"inst", "foo"}, true, []string{"inst", "foo@1.2.3"}}, + {"npm install-test", "npm", []string{"it", "foo"}, "npm", []string{"it", "foo"}, true, []string{"it", "foo@1.2.3"}}, + // update and dedupe ask the registry for something newer than the lockfile. + {"npm update", "npm", []string{"update"}, "npm", []string{"update"}, false, nil}, + {"npm up foo", "npm", []string{"up", "foo"}, "npm", []string{"up", "foo"}, false, nil}, + {"npm dedupe", "npm", []string{"dedupe"}, "npm", []string{"dedupe"}, false, nil}, + {"npm rebuild", "npm", []string{"rebuild"}, "npm", []string{"rebuild"}, false, nil}, + {"npm ci", "npm", []string{"ci"}, "npm", []string{"ci"}, false, nil}, + {"a range", "npm", []string{"install", "foo@^1"}, "npm", []string{"install", "foo@^1"}, false, nil}, + // The command that runs is not the command that was resolved. + {"npm through corepack", "corepack", []string{"npm", "install"}, "npm", []string{"install"}, false, nil}, + {"pnpm", "pnpm", []string{"install"}, "pnpm", []string{"install"}, false, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f := newResolvedFixture(t, project, written) + r := f.recorded() + run, finish := r.adopt(tc.cmd, tc.args, tc.pmCmd, tc.pmArgs) + if finish != nil { + defer finish() + } + if (finish != nil) != tc.adopted { + t.Fatalf("adopted = %v, want %v", finish != nil, tc.adopted) + } + if tc.adopted && !reflect.DeepEqual(run, tc.want) { + t.Fatalf("ran %v, want %v", run, tc.want) + } + if !tc.adopted { + if !reflect.DeepEqual(run, tc.args) { + t.Fatalf("ran %v, want the arguments as typed", run) + } + if _, err := os.Stat(filepath.Join(f.root, "package-lock.json")); err == nil { + t.Fatal("a lockfile was put in the project for a command that does not use it") + } + } + }) + } +} + +// Only entries the checks can hold to the registry's record are handed over. A +// link, a git source, a local folder, a package bundled in another and an entry +// with no hash have no such record. +func TestOnlyALockfileOfHashedTarballsIsKept(t *testing.T) { + lockWith := func(extra string) string { + return `{"lockfileVersion":3,"packages":{"":{"dependencies":{"foo":"^1.0.0"}},` + + `"node_modules/foo":{"version":"1.2.3","resolved":"https://registry.npmjs.org/foo/-/foo-1.2.3.tgz","integrity":"sha512-AAAA"},` + extra + `}}` + } + project := map[string]string{"package.json": `{"name":"app"}`} + for name, tc := range map[string]struct { + lock string + kept bool + }{ + "hashed tarballs": {lockWith(`"node_modules/bar":{"version":"1.0.0","resolved":"http://mirror.example/bar/-/bar-1.0.0.tgz","integrity":"sha512-BBBB"}`), true}, + "a link": {lockWith(`"node_modules/bar":{"resolved":"../bar","link":true}`), false}, + "a git source": {lockWith(`"node_modules/bar":{"version":"1.0.0","resolved":"git+ssh://git@github.com/u/bar.git#abc","integrity":"sha512-BBBB"}`), false}, + "a local folder": {lockWith(`"node_modules/bar":{"version":"1.0.0","resolved":"file:../bar","integrity":"sha512-BBBB"}`), false}, + "a bundled package": {lockWith(`"node_modules/bar":{"version":"1.0.0","inBundle":true}`), false}, + "an entry with no hash": {lockWith(`"node_modules/bar":{"version":"1.0.0","resolved":"https://registry.npmjs.org/bar/-/bar-1.0.0.tgz"}`), false}, + } { + t.Run(name, func(t *testing.T) { + f := newResolvedFixture(t, project, map[string]string{"package.json": fooManifest, "package-lock.json": tc.lock}) + if kept := len(f.recorded().lock) != 0; kept != tc.kept { + t.Fatalf("kept = %v, want %v", kept, tc.kept) + } + }) + } +} + +// A lockfile that does not describe the package.json the pass ended with is one +// npm would resolve further, so it is not one to install from. +func TestALockfileThatDoesNotMatchIsNotKept(t *testing.T) { + project := map[string]string{"package.json": `{"name":"app"}`} + mismatched := `{"lockfileVersion":3,"packages":{"":{"dependencies":{"other":"^1.0.0"}},"node_modules/other":{"version":"1.0.0"}}}` + for name, written := range map[string]map[string]string{ + "a lockfile for other dependencies": {"package.json": fooManifest, "package-lock.json": mismatched}, + "no lockfile": {"package.json": fooManifest}, + "an unreadable lockfile": {"package.json": fooManifest, "package-lock.json": "{"}, + "an old lockfile": {"package.json": fooManifest, "package-lock.json": `{"lockfileVersion":1,"dependencies":{"foo":{"version":"1.2.3"}}}`}, + } { + t.Run(name, func(t *testing.T) { + f := newResolvedFixture(t, project, written) + if r := f.recorded(); len(r.lock) != 0 { + t.Fatalf("kept %s", r.lock) + } + }) + } +} + +// A shrinkwrap is what npm reads first, and the pass wrote to that. The +// project's lockfile is not what the install would use. +func TestAShrinkwrapIsNotReplacedByALockfile(t *testing.T) { + f := newResolvedFixture(t, + map[string]string{"package.json": `{"name":"app"}`, "npm-shrinkwrap.json": `{"lockfileVersion":3,"packages":{"":{}}}`}, + map[string]string{"package.json": fooManifest, "package-lock.json": fooLock}) + if r := f.recorded(); len(r.lock) != 0 { + t.Fatal("kept a lockfile for a project that has a shrinkwrap") + } +} + +// What the project held when the pass ran is what the lockfile describes. If it +// changed while the checks ran, the install is left to resolve for itself. +func TestAProjectThatChangedWhileItWasCheckedIsNotAdopted(t *testing.T) { + for name, change := range map[string]func(root string) error{ + "package.json edited": func(root string) error { + return os.WriteFile(filepath.Join(root, "package.json"), []byte(`{"name":"app","private":true}`), 0o600) + }, + "a lockfile appeared": func(root string) error { + return os.WriteFile(filepath.Join(root, "package-lock.json"), []byte(`{}`), 0o600) + }, + "a shrinkwrap appeared": func(root string) error { + return os.WriteFile(filepath.Join(root, "npm-shrinkwrap.json"), []byte(`{}`), 0o600) + }, + } { + t.Run(name, func(t *testing.T) { + f := newResolvedFixture(t, map[string]string{"package.json": `{"name":"app"}`}, + map[string]string{"package.json": fooManifest, "package-lock.json": fooLock}) + r := f.recorded() + if err := change(f.root); err != nil { + t.Fatal(err) + } + run, finish := r.adopt("npm", []string{"install"}, "npm", []string{"install"}) + if finish != nil { + finish() + t.Fatal("adopted a lockfile for a project that was not as the pass found it") + } + if !reflect.DeepEqual(run, []string{"install"}) { + t.Fatalf("ran %v", run) + } + }) + } +} + +func lockfileAt(root string) string { return filepath.Join(root, "package-lock.json") } + +// The staged lockfile is taken out again unless npm wrote it, whatever the exit +// code. A run that stops before npm saves leaves the project's lockfile alone, so +// this does too. A run that fails after npm saved leaves the saved one. +func TestAStagedLockfileIsTakenOutUnlessNpmWroteIt(t *testing.T) { + staged := []byte(`{"staged":true}`) + earlier := []byte(`{"earlier":true}`) + earlierDate := time.Date(2024, time.March, 5, 12, 0, 0, 0, time.UTC) + + t.Run("none before, npm did not write", func(t *testing.T) { + root := tempDir(t) + s, ok := stageLockfile(lockfileAt(root), staged) + if !ok { + t.Fatal("not staged") + } + if got, _ := os.ReadFile(lockfileAt(root)); !bytes.Equal(got, staged) { + t.Fatalf("staged file holds %q", got) + } + s.finish() + if _, err := os.Stat(lockfileAt(root)); err == nil { + t.Fatal("a lockfile npm never wrote was left in the project") + } + }) + + t.Run("one before, npm did not write", func(t *testing.T) { + root := tempDir(t) + if err := os.WriteFile(lockfileAt(root), earlier, 0o640); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(lockfileAt(root), earlierDate, earlierDate); err != nil { + t.Fatal(err) + } + s, ok := stageLockfile(lockfileAt(root), staged) + if !ok { + t.Fatal("not staged") + } + s.finish() + got, _ := os.ReadFile(lockfileAt(root)) + if !bytes.Equal(got, earlier) { + t.Fatalf("the project's lockfile holds %q, want what it held", got) + } + if info, _ := os.Stat(lockfileAt(root)); !info.ModTime().Equal(earlierDate) { + t.Errorf("the project's lockfile is dated %v, want %v", info.ModTime(), earlierDate) + } + }) + + t.Run("npm wrote it", func(t *testing.T) { + root := tempDir(t) + s, ok := stageLockfile(lockfileAt(root), staged) + if !ok { + t.Fatal("not staged") + } + // What npm does: write the file, which dates it now. The same length, so + // that only the date can say it was written. + written := bytes.ToUpper(staged) + if err := os.WriteFile(lockfileAt(root), written, 0o600); err != nil { + t.Fatal(err) + } + s.finish() + if got, _ := os.ReadFile(lockfileAt(root)); !bytes.Equal(got, written) { + t.Fatalf("npm's lockfile was replaced by %q", got) + } + }) + + t.Run("someone else changed it", func(t *testing.T) { + root := tempDir(t) + s, ok := stageLockfile(lockfileAt(root), staged) + if !ok { + t.Fatal("not staged") + } + edited := []byte(`{"edited":"by hand"}`) + if err := os.WriteFile(lockfileAt(root), edited, 0o600); err != nil { + t.Fatal(err) + } + s.finish() + if got, _ := os.ReadFile(lockfileAt(root)); !bytes.Equal(got, edited) { + t.Fatalf("an edit made during the install was replaced by %q", got) + } + }) +} + +func TestALockfileThatIsALinkIsLeftAlone(t *testing.T) { + root := tempDir(t) + target := filepath.Join(root, "real-lock.json") + if err := os.WriteFile(target, []byte(`{"real":true}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, lockfileAt(root)); err != nil { + t.Skipf("creating symlinks on Windows needs privilege or Developer Mode: %v", err) + } + if _, ok := stageLockfile(lockfileAt(root), []byte(`{"staged":true}`)); ok { + t.Fatal("staged over a link") + } + if got, _ := os.ReadFile(target); string(got) != `{"real":true}` { + t.Fatalf("the linked file holds %q", got) + } +} diff --git a/internal/nvx/verify_targets.go b/internal/nvx/verify_targets.go index 5b484e3e..7dd8615c 100644 --- a/internal/nvx/verify_targets.go +++ b/internal/nvx/verify_targets.go @@ -93,6 +93,9 @@ type verifyRequest struct { // npm's resolution step runs the same way. contain bool launch SandboxConfig + // resolved, when set, is filled with what npm's resolution pass wrote, for + // the install that follows to start from. See npm_resolved_install.go. + resolved *resolvedInstall } // verifyBeforeRun runs the pre-install checks for a package-manager command. diff --git a/site/src/content/docs/docs/index.md b/site/src/content/docs/docs/index.md index 388373a1..9b0107b1 100644 --- a/site/src/content/docs/docs/index.md +++ b/site/src/content/docs/docs/index.md @@ -44,8 +44,12 @@ Before an install runs, nvx checks what it is about to fetch. An npm install that brings in new packages runs npm twice. The first run only resolves versions, contained, so each package can be checked before the second -run installs it. An `npm install` whose lockfile already matches -`package.json`, and `npm ci`, run npm once. +run installs it. The second run installs the lockfile the first one wrote, so a +version published in between is not installed unchecked. `npm update`, +`npm dedupe` and an install nvx cannot pin to that lockfile, such as one that +names a version range, an alias or a git source, resolve again in the second +run. An `npm install` whose lockfile already matches `package.json`, and +`npm ci`, run npm once. Each check has its own exemption list in the [policy file](/docs/policy/#reference). None of them certifies a package, which is why containment is the backstop. From f8e105fddb995e8ad761b9c8a990cd0e3310e913 Mon Sep 17 00:00:00 2001 From: Felix Stubner Date: Thu, 8 Oct 2026 01:53:53 +0100 Subject: [PATCH 4/4] Add changelog entries for the install, sweep and hang-up watch changes Each entry carries the numbers measured for it on Windows and in a Linux container on 2026-10-08. --- CHANGELOG.md | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08eea3bc..8aa0f017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,6 +188,61 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and Administrators full control (and you, for your profile). Otherwise it says why and changes nothing. `nvx doctor` points at `nvx setup` for this. +* **An npm install now installs the lockfile nvx checked.** An install that + brings in new packages runs npm twice. The first run resolves the tree and + nvx checks what it wrote. The second run used to resolve the tree again from + the registry, so a version published between the two runs was installed + without a check, and it repeated the first run's work. It now starts from the + lockfile the first run wrote, and npm asks the registry for nothing but the + tarballs. An install that names packages first makes each name exact from + that lockfile, so `foo` and `foo@latest` become `foo@1.2.3`, because npm + asks the registry again about any package it is told to install. Your + `package.json` and `package-lock.json` come out as npm leaves them. Against a + fake registry that publishes new versions between the two runs, a bare + install and an install of a name, a tag and `-D` left both files byte for byte + as a plain npm run does, with npm 7.24.2, 8.19.4, 9.9.4, 10.9.3, 10.9.9 and + 11.11.0. If npm does not write the lockfile itself, as with + `package-lock=false` in `.npmrc` or `--no-save`, nvx takes the one it put + there out again. `npm update`, `npm dedupe` and an install that names a + version range, an alias, a URL or a git source resolve again in the second + run, as before, and `--verbose` says why. pnpm, Yarn and Bun have no + resolving run and are unchanged. Measured 2026-10-08 as the median of 5 + interleaved runs through the contained shim, on machines busy with other + work. The 321-package project with no lockfile took 88.2 s on Windows and + now takes 64.8 s. In a Linux container it took 89.8 s and now takes 61.4 s. + An install of one package did not change, 8.7 s and 8.8 s on Windows and + 2.47 s and 2.39 s on Linux. + +* **A command no longer waits for nvx's housekeeping.** After a shimmed + command, nvx deletes what killed runs left behind, and the command did not + exit until it had finished. On a Windows machine with 638 package profiles, + the sweep of those alone took 83 to 91 ms a call, and `node -e 0` took 3.50 s + with one guest home of 12,000 files left behind, because it waited for the + home to be deleted (medians of 5 interleaved runs). Package profiles, rescued + logs and staged command copies only count once they are a week or two old, + so they are now looked at when the last full look is an hour old, and sooner + while one of them is working through a backlog. The sweep runs in the + background, and the command waits for it for 100 ms at most and leaves the + rest to the next command. One sweep runs at a time, under a lock that + Windows or Linux releases if the sweep dies. Profile deletions stay on the + command's own thread, so leaving never stops one halfway. On Windows + `node --version` through the shim took a median of 0.221 s and now takes + 0.101 s, against 0.052 s for node alone (21 interleaved runs, stdin from + NUL), and `node -e 0` with the leftover home took 3.50 s and now takes + 0.25 s. On Linux the same home took 0.46 s and now takes 0.18 s, and + `node --version` through the shim stayed at 0.018 s. + +* **On Windows, a command whose stdin is a pipe no longer lists every process + as it starts.** An agent harness or an MCP client leaves stdin as a pipe, + and nvx then watches for the program that started it to go away. Finding that + program meant a snapshot of every process on the machine, taken twice. One + snapshot took 19 to 27 ms (three runs of 40 calls). nvx now asks Windows for + the parent's process id, which took about 0.4 microseconds a call (three runs + of 2000), and falls back to the snapshot only if that fails. Measured as the + median of 21 interleaved runs of `node --version` through the shim, it took + 0.300 s with stdin a pipe and 0.221 s with stdin from NUL before, and takes + 0.103 s and 0.101 s now. + ### Security * **Windows: in 0.7.0 a contained launch could, rarely, start the command