From cf9c75d4b53a2e2411be6f335d0542c0ba8d30e0 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 07:06:51 +0600 Subject: [PATCH] fix(cli): replay snapshot from latest-installed version, not new version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runUpgrade` snapshots each *previously installed* Node version's global npm packages, then called `packages.Restore(ctx, mgr, *v)` for each **newly installed** version. Since snapshots are keyed by `-.json` (e.g., `fnm-20.10.0.json`) and `Restore` looks up by the exact same key, the restore step read a file that didn't yet exist (`fnm-20.11.0.json`) — its wrapped "read snapshot: open …: no such file or directory" was swallowed by the loop's `cmd.Printf("Warning: …")`, silently no-op'ing the package migration on every upgrade. Resolve the snapshot path the same way the sentinel block does (latest-installed-version key, kept in a single local variable shared between the sentinel arm and the restore step) and replay via `packages.RestoreFromSnapshot(ctx, restoreSnapshotPath)`. The user's global packages now carry forward to the new default exactly once. Refs #42 Co-Authored-By: puku-ai-2.8 --- CHANGELOG.md | 17 ++++ internal/cli/upgrade.go | 72 +++++++++------ internal/packages/snapshot_test.go | 138 +++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 27 deletions(-) create mode 100644 internal/packages/snapshot_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 550b619..fd341c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `chore(ci): replace issue-workflow.sh shell script with an AI skill` for the full rationale. +### Fixed +- `internal/cli`: upgrade loop's restore step was looking up its + snapshot by the **new** installed Node version + (`packages.Restore(ctx, mgr, *v)` where `v` came from `toInstall`), + but the upgrade flow only writes snapshots for the **previously** + installed versions. The restore then read a non-existent + `/snapshots/-.json`, hit + `read snapshot: open …: no such file or directory`, and the loop's + blanket `cmd.Printf("Warning: restore failed: …")` swallowed the + error — silently no-op'ing the package migration for every newly + installed Node. The restore step now resolves the snapshot path the + same way the sentinel does (latest-installed-version key) and + replays via `packages.RestoreFromSnapshot(ctx, snapshotPath)`, + carrying the user's globals forward to the new default exactly + once. Added a regression test pinning the + `RestoreFromSnapshot`-reads-source-path contract. Closes #42. + ## [0.0.0] - 2024-07-01 ### Added diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 4188f3a..f6a214d 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -210,7 +210,19 @@ func runUpgrade(cmd *cobra.Command, args []string) error { return nil } - // Snapshot current packages + // 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 + // itself only replays from the latest installed version (below) — + // the snapshots for older installed versions remain on disk for + // the user's reference. + // + // While we're at it, resolve the conventional snapshot path for + // the latest installed version — this is the path both the + // sentinel arms and the restore step reads from, so we compute it + // once and pass it down. + var restoreSnapshotPath string + var oldVersion string if !skipMigrate { ctx := cmd.Context() for _, v := range installedVersions { @@ -218,6 +230,18 @@ func runUpgrade(cmd *cobra.Command, args []string) error { cmd.Printf("Warning: snapshot failed for %s: %v\n", v, err) } } + if len(installedVersions) > 0 { + last := installedVersions[len(installedVersions)-1] + oldVersion = last.String() + // Resolve the snapshot path the conventional way. We + // tolerate a failure here — without a snapshot path the + // sentinel is still useful (it tells the user a + // migration was in flight) and we don't want to abort + // the upgrade over a path-resolution glitch. + if p, perr := packages.SnapshotPath(m.Name(), oldVersion); perr == nil { + restoreSnapshotPath = p + } + } } // Arm the sentinel AFTER snapshots are on disk but BEFORE any @@ -225,35 +249,19 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // cleanup, the sentinel is the "this upgrade was interrupted" // breadcrumb that the next `nodeup` invocation will pick up. // - // We point the sentinel at the latest installed version's snapshot - // since that is the most likely one we want to replay against — - // it contains the package set the user had right before we started - // installing the new versions. + // The sentinel records the snapshot path so that + // `nodeup packages restore --from ` can replay + // against exactly the same package set we were about to install. if !skipMigrate { newVersion := "" if len(toInstall) > 0 { newVersion = toInstall[len(toInstall)-1].String() } - oldVersion := "" - var snapshotPath string - if len(installedVersions) > 0 { - last := installedVersions[len(installedVersions)-1] - oldVersion = last.String() - // Resolve the snapshot path the conventional way so the - // warning message can hand it to the user verbatim. We - // tolerate a failure here — without a snapshot path the - // sentinel is still useful (it tells the user a migration - // was in flight) and we don't want to abort the upgrade - // over a path-resolution glitch. - if p, perr := packages.SnapshotPath(m.Name(), oldVersion); perr == nil { - snapshotPath = p - } - } if err := packages.WriteSentinel(packages.UpgradeSentinel{ Manager: m.Name(), OldVersion: oldVersion, NewVersion: newVersion, - SnapshotPath: snapshotPath, + SnapshotPath: restoreSnapshotPath, }); err != nil { cmd.Printf("Warning: failed to write upgrade sentinel: %v\n", err) } else { @@ -277,13 +285,23 @@ func runUpgrade(cmd *cobra.Command, args []string) error { } } - // Restore packages + // Restore packages under the NEW default. We replay the snapshot + // of the latest previously-installed Node version's global npm + // packages against the freshly-set-default Node — the user's + // globals carry forward to the new install exactly once. + // + // We deliberately do NOT loop per-new-version here: each newly + // installed Node has its own `npm install -g` environment, and + // the right package set is the one from the user's most-recent + // active Node, not from any freshly-installed (empty) one. The + // sentinel already records this snapshot path for + // replay-after-interrupt (`nodeup packages restore --from + // `); here we read the same path directly. if !skipMigrate && len(toInstall) > 0 { - ctx := cmd.Context() - for _, v := range toInstall { - if err := packages.Restore(ctx, m.Name(), *v); err != nil { - cmd.Printf("Warning: restore failed: %v\n", err) - } + if restoreSnapshotPath == "" { + cmd.Printf("Warning: no snapshot path available to restore from; skipping package migration\n") + } else if err := packages.RestoreFromSnapshot(cmd.Context(), restoreSnapshotPath); err != nil { + cmd.Printf("Warning: restore failed: %v\n", err) } } diff --git a/internal/packages/snapshot_test.go b/internal/packages/snapshot_test.go new file mode 100644 index 0000000..21f2bb1 --- /dev/null +++ b/internal/packages/snapshot_test.go @@ -0,0 +1,138 @@ +package packages + +import ( + "context" + "encoding/json" + "errors" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Masterminds/semver/v3" +) + +// TestSnapshot_StoredVersionKeyMatchesArg verifies the on-disk path +// convention used by Snapshot() / SnapshotPath(): a snapshot taken for +// Node 20.10.0 lives at /snapshots/fnm-20.10.0.json — the +// file name is keyed on the version argument, not on the new version +// the upgrade flow is about to install. +// +// This test pins down the contract that the upgrade-loop fix in #42 +// relies on: the upgrade flow snapshots the *previously installed* +// versions, then on restore uses RestoreFromSnapshot() rather than looking up a snapshot file keyed on the +// newly-installed version (which would silently miss and no-op the +// migration). +func TestSnapshot_StoredVersionKeyMatchesArg(t *testing.T) { + redirectDataDir(t) + + // No installed npm, no packages — Snapshot() reads + // `npm ls -g --json --depth=0` and will fail in a test + // environment. We bypass Snapshot() and call saveSnapshot() + // directly with a known SnapshotData struct so the test stays + // hermetic. + snap := SnapshotData{ + Manager: "fnm", + NodeVersion: "20.10.0", + Packages: []Package{}, + } + if err := saveSnapshot(snap); err != nil { + t.Fatalf("saveSnapshot: %v", err) + } + + // Conventional path should exist for the OLD-installed version. + wantPath, err := SnapshotPath("fnm", "20.10.0") + if err != nil { + t.Fatalf("SnapshotPath old-version: %v", err) + } + if _, err := os.Stat(wantPath); err != nil { + t.Fatalf("snapshot file not created at conventional path %s: %v", wantPath, err) + } + + // The version-key the upgrade loop used to look up (BEFORE the + // #42 fix) was the NEW installed version (20.11.0). That key + // deliberately misses — verifying the miss here pins down the + // regression that the fix prevents. + newVerPath, err := SnapshotPath("fnm", "20.11.0") + if err != nil { + t.Fatalf("SnapshotPath new-version: %v", err) + } + if _, statErr := os.Stat(newVerPath); statErr == nil { + t.Fatalf("new-version snapshot file should NOT exist at %s — if it does, the version-key mismatch regression has resurfaced", newVerPath) + } +} + +// TestRestore_LooksUpByArgVersion covers the existing behavior of +// Restore(ctx, manager, version): it loads the conventional +// /snapshots/-.json file. We don't exercise +// installPackages' actual `npm install -g` call (that needs a working +// npm binary), but verify the read-and-parse path returns a wrapped +// "read snapshot" error when the file does not exist — this is the +// failure mode that used to silently no-op the upgrade migration +// when the upgrade-loop called Restore with the new (not-yet-existing) +// version key. +func TestRestore_LooksUpByArgVersion(t *testing.T) { + redirectDataDir(t) + + // There is no snapshot for fnm 99.99.99 — Restore must surface + // that as a wrapped read error, not a silent success. + v, err := semver.NewVersion("99.99.99") + if err != nil { + t.Fatalf("parse version: %v", err) + } + restoreErr := Restore(context.Background(), "fnm", *v) + if restoreErr == nil { + t.Fatal("expected error for missing snapshot, got nil") + } + if !strings.Contains(restoreErr.Error(), "read snapshot") { + t.Errorf("error %q does not mention read snapshot failure", restoreErr) + } + // The user-facing error should be wrapped — `read snapshot:` is + // the human-readable prefix and the underlying fs.PathError must + // remain accessible via errors.Is so callers can branch on + // fs.ErrNotExist. Stdlib's *PathError satisfies this naturally, + // so we don't add an Is() method on our error type. + if !errors.Is(restoreErr, fs.ErrNotExist) { + t.Errorf("error %q does not unwrap to fs.ErrNotExist; callers cannot branch on missing-file", restoreErr) + } +} + +// TestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersion is the +// regression test for the upgrade-loop fix: RestoreFromSnapshot reads +// the snapshot at the path it's given regardless of the Node version +// key in the filename. This is what the upgrade flow relies on — +// the snapshot path comes from the sentinel, which records the +// latest-INSTALLED-version's snapshot (e.g., fnm-20.10.0.json), not a +// fresh-version snapshot keyed on the just-installed Node. +func TestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersion(t *testing.T) { + redirectDataDir(t) + + // Write a snapshot at an arbitrary path (does not follow the + // `-.json` convention — RestoreFromSnapshot is + // explicitly path-based). + tmp := t.TempDir() + arbitraryPath := filepath.Join(tmp, "old-installed-set.json") + snap := SnapshotData{ + Manager: "fnm", + NodeVersion: "20.10.0", // the OLD installed version's key + Packages: []Package{}, // no packages → install loop is a no-op + } + data, err := json.MarshalIndent(snap, "", " ") + if err != nil { + t.Fatalf("marshal: %v", err) + } + if err := os.WriteFile(arbitraryPath, data, 0o644); err != nil { + t.Fatalf("write: %v", err) + } + + // RestoreFromSnapshot should succeed: the install loop iterates + // over zero packages and exits cleanly. This proves that the + // path-based restore does NOT depend on the version-key in the + // filename matching some "new version" expectation — it just + // reads what's at the path. + if err := RestoreFromSnapshot(context.Background(), arbitraryPath); err != nil { + t.Errorf("RestoreFromSnapshot: %v", err) + } +}