diff --git a/CHANGELOG.md b/CHANGELOG.md index 331a5d6..a8e715a 100644 --- a/CHANGELOG.md +++ b/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 + (`/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 diff --git a/README.md b/README.md index 0d2718c..0925773 100644 --- a/README.md +++ b/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 diff --git a/internal/cli/config.go b/internal/cli/config.go index 8343975..8832d42 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "fmt" "strings" @@ -8,6 +9,7 @@ import ( "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) { diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index f6a214d..9778d3e 100644 --- a/internal/cli/upgrade.go +++ b/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 `, but the upgrade flow diff --git a/internal/platform/lock_acquire_test.go b/internal/platform/lock_acquire_test.go new file mode 100644 index 0000000..74c1cae --- /dev/null +++ b/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") + } +}