fix(cli): wire platform.AcquireLock into upgrade + config set/init

CHANGELOG.md

@@ -157,6 +157,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   detector-level test that points `NVM_DIR` at a payload string and
   asserts no unquoted `$(touch` occurrence in the emitted script.
   Closes #43.
+- `internal/cli`: the `platform.AcquireLock`/`Release` infrastructure
+  (flock on POSIX, `LockFileEx` on Windows, stale-PID detection)
+  existed but had zero call sites anywhere in the CLI, despite the
+  README and `package` doc comments claiming concurrent runs are
+  blocked. Two parallel `nodeup upgrade` invocations could snapshot,
+  install, and migrate against the same data dir with no guard
+  between them; two parallel `nodeup config set` invocations raced
+  on read-modify-write of the YAML file (each Save is atomic on its
+  own, but the second writer's atomic rename silently clobbers the
+  first writer's in-memory changes — a lost update). `runUpgrade`,
+  `config set`, and `config init` now `defer platform.AcquireLock()`
+  around their critical sections and surface `ErrAlreadyLocked`
+  with a "another nodeup process holds the lock" hint instead of
+  silently racing. Dry-run is read-only and intentionally does NOT
+  acquire the lock (it returns before any mutation). The README's
+  stale lock-path documentation (`~/.nodeup/nodeup.lock`) is fixed
+  to point at the actual `platform.LockPath()` resolution
+  (`<DataDir>/nodeup.lock` per OS — Linux XDG, macOS
+  `~/Library/Application Support/nodeup/nodeup.lock`, Windows
+  `%APPDATA%\nodeup\nodeup.lock`). Closes #44.
 
 ## [0.0.0] - 2024-07-01
 

README.md

@@ -57,9 +57,13 @@ A few things worth knowing before you run `nodeup`:
 - **Native addons may need a rebuild** after a major Node version
   bump. If something like `node-sass` or `sharp` misbehaves, run
   `npm rebuild -g` against the new version.
-- **Concurrent runs are blocked** via a lock file at
-  `~/.nodeup/nodeup.lock`. If a run crashes mid-upgrade, the next
-  invocation offers to restore from the snapshot written at the start.
+- **Concurrent runs are blocked** via a lock file at the platform
+  `DataDir` (resolved by `platform.LockPath()` — e.g.
+  `~/.local/share/nodeup/nodeup.lock` on Linux,
+  `~/Library/Application Support/nodeup/nodeup.lock` on macOS,
+  `%APPDATA%\nodeup\nodeup.lock` on Windows). If a run crashes
+  mid-upgrade, the next invocation offers to restore from the
+  snapshot written at the start.
 
 ## Installation
 

internal/cli/config.go

@@ -1,13 +1,15 @@
 package cli
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
 	"github.com/spf13/cobra"
 	"gopkg.in/yaml.v3"
 
 	"github.com/dipto0321/nodeup/internal/config"
+	"github.com/dipto0321/nodeup/internal/platform"
 )
 
 // newConfigCmd implements `nodeup config ...` subcommands.
@@ -123,6 +125,22 @@ func newConfigSetCmd() *cobra.Command {
 				return err
 			}
 
+			// Acquire the nodeup lock so two `nodeup config set`
+			// (or `config set` + `nodeup upgrade`) invocations can't
+			// race on read-modify-write of the config file. See #44.
+			configLock, err := platform.AcquireLock()
+			if err != nil {
+				if errors.Is(err, platform.ErrAlreadyLocked) {
+					return fmt.Errorf("refusing to edit config: %w\n  (another nodeup process holds the lock)", err)
+				}
+				return fmt.Errorf("acquire config lock: %w", err)
+			}
+			defer func() {
+				if rerr := configLock.Release(); rerr != nil {
+					fmt.Fprintf(cmd.OutOrStdout(), "Warning: failed to release config lock: %v\n", rerr)
+				}
+			}()
+
 			// Load the current file (or start from defaults if absent).
 			current, _, err := config.Load(path)
 			if err != nil {
@@ -174,6 +192,22 @@ to overwrite an existing file unless --force is passed.`,
 				return err
 			}
 
+			// Acquire the nodeup lock so two `nodeup config init`
+			// invocations can't race on the existence-check /
+			// overwrite dance. See #44.
+			configLock, err := platform.AcquireLock()
+			if err != nil {
+				if errors.Is(err, platform.ErrAlreadyLocked) {
+					return fmt.Errorf("refusing to init config: %w\n  (another nodeup process holds the lock)", err)
+				}
+				return fmt.Errorf("acquire config lock: %w", err)
+			}
+			defer func() {
+				if rerr := configLock.Release(); rerr != nil {
+					fmt.Fprintf(cmd.OutOrStdout(), "Warning: failed to release config lock: %v\n", rerr)
+				}
+			}()
+
 			// Existence check is best-effort; we let Save do the real
 			// atomic-create dance.
 			if !force && fileExists(path) {

internal/cli/upgrade.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"bufio"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -13,6 +14,7 @@ import (
 	"github.com/dipto0321/nodeup/internal/detector"
 	"github.com/dipto0321/nodeup/internal/node"
 	"github.com/dipto0321/nodeup/internal/packages"
+	"github.com/dipto0321/nodeup/internal/platform"
 )
 
 // newUpgradeCmd implements `nodeup upgrade` — upgrade LTS and/or Current versions.
@@ -210,6 +212,27 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 
+	// Acquire the nodeup concurrency lock. Two `nodeup upgrade` (or
+	// `nodeup upgrade` + `nodeup config set`) invocations running in
+	// parallel would otherwise snapshot/install/migrate against the
+	// same data dir with no guard between them — see #44. We acquire
+	// AFTER the dry-run early-return because dry-run is read-only
+	// (no disk mutation, no shell-out) and we don't want a
+	// concurrent read-only query to hold a lock a real mutation is
+	// waiting on.
+	upgradeLock, err := platform.AcquireLock()
+	if err != nil {
+		if errors.Is(err, platform.ErrAlreadyLocked) {
+			return fmt.Errorf("refusing to upgrade: %w\n  (another nodeup process holds the lock; wait for it to finish, or kill the stale process)", err)
+		}
+		return fmt.Errorf("acquire upgrade lock: %w", err)
+	}
+	defer func() {
+		if rerr := upgradeLock.Release(); rerr != nil {
+			cmd.Printf("Warning: failed to release upgrade lock: %v\n", rerr)
+		}
+	}()
+
 	// Snapshot current packages. We record one snapshot per installed
 	// version so the user can manually replay against any of them via
 	// `nodeup packages restore <mgr> <version>`, but the upgrade flow

internal/platform/lock_acquire_test.go

@@ -0,0 +1,109 @@
+package platform
+
+import (
+	"errors"
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+// redirectHomeToTmp points platform.DataDir() at a per-test tempdir by
+// setting HOME (and XDG_DATA_HOME / APPDATA — whichever is checked first
+// on this OS) to the tempdir. This lets AcquireLock/LockPath work in a
+// hermetic per-test directory and never touch the developer's real
+// ~/.nodeup.
+//
+// We also clear HOME's parent cousins on each platform so they don't
+// override HOME on Linux (XDG_DATA_HOME) or Windows (APPDATA).
+func redirectHomeToTmp(t *testing.T) string {
+	t.Helper()
+	tmp := t.TempDir()
+	t.Setenv("XDG_DATA_HOME", "")
+	t.Setenv("APPDATA", "")
+	if runtime.GOOS == "windows" {
+		t.Setenv("USERPROFILE", tmp)
+		t.Setenv("HOME", tmp) // belt + braces on Windows shells
+	} else {
+		t.Setenv("HOME", tmp)
+	}
+	return tmp
+}
+
+// TestAcquireLock_RoundTrip verifies the basic happy path: acquire,
+// then release, then re-acquire. Without release in between, the second
+// AcquireLock would return ErrAlreadyLocked (covered by
+// TestAcquireLock_TwiceReturnsAlreadyLocked).
+func TestAcquireLock_RoundTrip(t *testing.T) {
+	redirectHomeToTmp(t)
+	l1, err := AcquireLock()
+	if err != nil {
+		t.Fatalf("AcquireLock (1st): %v", err)
+	}
+	if l1 == nil {
+		t.Fatal("AcquireLock returned nil LockFile")
+	}
+	if err := l1.Release(); err != nil {
+		t.Errorf("Release: %v", err)
+	}
+
+	// A second acquire after a clean release must succeed.
+	l2, err := AcquireLock()
+	if err != nil {
+		t.Fatalf("AcquireLock (2nd, after Release): %v", err)
+	}
+	if err := l2.Release(); err != nil {
+		t.Errorf("Release (2nd): %v", err)
+	}
+}
+
+// TestAcquireLock_TwiceReturnsAlreadyLocked is the regression test for
+// #44: the lock subsystem exists and is correct, but it had no call
+// sites. While the CLI wire-up is now in place (internal/cli/{upgrade,
+// config}.go), this unit test pins down the contract that the wire-up
+// relies on — a held lock must surface ErrAlreadyLocked, not a generic
+// I/O error.
+func TestAcquireLock_TwiceReturnsAlreadyLocked(t *testing.T) {
+	redirectHomeToTmp(t)
+	first, err := AcquireLock()
+	if err != nil {
+		t.Fatalf("AcquireLock (1st): %v", err)
+	}
+	t.Cleanup(func() { _ = first.Release() })
+
+	_, err2 := AcquireLock()
+	if err2 == nil {
+		t.Fatal("second AcquireLock succeeded while first holds the lock — flock is broken")
+	}
+	if !errors.Is(err2, ErrAlreadyLocked) {
+		t.Errorf("second AcquireLock error %q is not ErrAlreadyLocked — CLI callers cannot branch on it", err2)
+	}
+}
+
+// TestLockPath_UnderDataDir verifies that LockPath() returns a path
+// inside the same DataDir the rest of nodeup's files live under. On
+// each platform DataDir's terminal path component is "nodeup" (Linux,
+// Windows) or a prefix that ends in "/nodeup" (macOS, where
+// ~/Library/Application Support/nodeup is the layout). We assert
+// the lock file lives under that directory by cross-checking via
+// DataDir() — the lock MUST be on the same DataDir as the snapshots/
+// reports/ cache/ subdirs, otherwise filesystem-permission enforcement
+// of the "concurrent runs blocked" claim would not survive a refactor.
+func TestLockPath_UnderDataDir(t *testing.T) {
+	redirectHomeToTmp(t)
+
+	dataDir, err := DataDir()
+	if err != nil {
+		t.Fatalf("DataDir: %v", err)
+	}
+	lock, err := LockPath()
+	if err != nil {
+		t.Fatalf("LockPath: %v", err)
+	}
+	if filepath.Dir(lock) != dataDir {
+		t.Errorf("LockPath() directory %q is not the resolved DataDir %q — lock would not be enforceable next to snapshots/cache/reports",
+			filepath.Dir(lock), dataDir)
+	}
+	if filepath.Base(lock) != "nodeup.lock" {
+		t.Errorf("LockPath() file name = %q, want %q", filepath.Base(lock), "nodeup.lock")
+	}
+}