diff --git a/CHANGELOG.md b/CHANGELOG.md index a4141f7..d984c10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,10 +79,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 snapshots packages at the start of a run, it writes a sentinel file recording the manager, the pre-upgrade version, and the snapshot path. If a subsequent run finds a leftover sentinel, it - prompts the user to replay the package migration against the - snapshot (PR #29). `nodeup packages restore` accepts a - `--from ` flag for restoring from a non-default - location, mirroring the sentinel's stored path. + prints a hint to stderr naming the snapshot path and the exact + `nodeup packages restore --from ` invocation the user can + copy to replay the migration (PR #29). `nodeup packages restore` + accepts a `--from ` flag for restoring from a + non-default location, mirroring the sentinel's stored path. - Cross-platform path handling: `internal/platform.QuotePath` now enforces consistent shell-quoting across all `RunShell` callsites, so paths containing spaces (e.g. `C:\Users\Dipto Karmakar\...`) @@ -177,6 +178,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (`/nodeup.lock` per OS — Linux XDG, macOS `~/Library/Application Support/nodeup/nodeup.lock`, Windows `%APPDATA%\nodeup\nodeup.lock`). Closes #44. +- `internal/cli`: the upgrade-in-progress sentinel had a broken + lifecycle in both directions. On the `nodeup upgrade` side, the + deferred `RemoveSentinel()` fired unconditionally as long as the + sentinel had been armed — even when the post-install package + restore had failed and only logged a warning. That left the + user with installed Node versions, unmigrated global packages, + AND the "resume breadcrumb" silently deleted: there was no way to + recover through the documented `--from ` path. We + now track `restoreSucceeded` alongside the existing + `sentinelArmed` and only remove the sentinel when both are true. + On the manual `nodeup packages restore` side — the exact command + `PersistentPreRunE` tells the user to run after an interrupted + upgrade — a successful restore now calls `packages.RemoveSentinel()` + so subsequent `nodeup` invocations stop printing the + "interrupted upgrade" warning forever. Removal failures there are + logged, not returned: the user's primary goal (restored packages) + has been achieved and a stale sentinel is cosmetic. Also corrects + the CHANGELOG claim that "next run prompts the user to replay" — + the implementation only prints a two-line stderr hint + (`To resume: nodeup packages restore --from `); no + interactive prompt is shown. Closes #46. - `nodeup list` was a stub that printed "not yet implemented (Phase 1)". It now does the obvious thing — resolve every detected manager, call each one's `ListInstalled()`, and render the union diff --git a/internal/cli/packages.go b/internal/cli/packages.go index ade2d48..c1cf02c 100644 --- a/internal/cli/packages.go +++ b/internal/cli/packages.go @@ -141,12 +141,31 @@ func runRestore(cmd *cobra.Command, args []string) error { ctx := context.Background() fromPath, _ := cmd.Flags().GetString("from") + // runRestore doubles as the "replay the upgrade-in-progress sentinel" + // command — PersistentPreRunE's hint (root.go:21-40) tells the user + // to invoke `nodeup packages restore --from `. If the + // restore succeeds, the sentinel's job is done and we should clear + // it so the next `nodeup` invocation doesn't keep warning about an + // "interrupted upgrade" that has in fact been resolved. + // + // We unconditionally attempt the removal after success: a sentinel + // from a *different* (older) upgrade is harmless stale state, and + // the next upgrade would overwrite it anyway. We log (don't fail) + // on a removal error because the user's actual goal — restored + // packages — has already been achieved. + clearSentinel := func() { + if err := packages.RemoveSentinel(); err != nil { + cmd.Printf("Warning: failed to clear upgrade sentinel: %v\n", err) + } + } + // --from branch: read the path straight off disk, no manager or // version parsing required. if fromPath != "" { if err := packages.RestoreFromSnapshot(ctx, fromPath); err != nil { return fmt.Errorf("restore failed: %w", err) } + clearSentinel() cmd.Printf("Restored packages from %s\n", fromPath) return nil } @@ -165,6 +184,7 @@ func runRestore(cmd *cobra.Command, args []string) error { if err := packages.Restore(ctx, managerName, *v); err != nil { return fmt.Errorf("restore failed: %w", err) } + clearSentinel() cmd.Printf("Restored packages for %s %s\n", managerName, versionStr) return nil diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index 9778d3e..93bf786 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -115,9 +115,17 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // replay-by-sentinel bookkeeping is wrapped in a sentinel lifecycle. // We use a flag (rather than os.Exit) so deferred cleanup runs even // on error paths. + // + // restoreSucceeded tracks whether the post-install package restore + // step actually succeeded; the deferred cleanup only removes the + // sentinel when that's true. A failed restore must leave the sentinel + // on disk so a follow-up `nodeup packages restore --from ` + // (the command printed by PersistentPreRunE's hint) can still find + // the resume breadcrumb. See issue #46. sentinelArmed := false + restoreSucceeded := false defer func() { - if sentinelArmed { + if sentinelArmed && restoreSucceeded { if err := packages.RemoveSentinel(); err != nil { cmd.Printf("Warning: failed to remove upgrade sentinel: %v\n", err) } @@ -325,6 +333,8 @@ func runUpgrade(cmd *cobra.Command, args []string) error { 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) + } else { + restoreSucceeded = true } } diff --git a/internal/packages/sentinel_test.go b/internal/packages/sentinel_test.go index 8d13b04..7569c6d 100644 --- a/internal/packages/sentinel_test.go +++ b/internal/packages/sentinel_test.go @@ -263,6 +263,76 @@ func TestLoadSentinel_ParseError(t *testing.T) { } } +// TestRemoveSentinel_OnlyOnRestoreSuccess is the regression pin for +// issue #46. +// +// Before the fix, `nodeup upgrade`'s deferred cleanup called +// RemoveSentinel() unconditionally after the sentinel was armed — +// even when the post-install package restore had failed (because +// restore failures were logged as warnings and the function returned +// normally). The user was left with installed Node versions, +// unmigrated global packages, AND no "resume breadcrumb" to point +// the manual `nodeup packages restore --from ` command at. +// +// The fix is in the cli/upgrade.go defer: RemoveSentinel() now +// fires only after a successful restore. We can't unit-test that +// defer in isolation without spinning up the full upgrade pipeline, +// so we instead pin the underlying primitives the cli uses: +// - writing a sentinel surfaces as orphaned +// - calling RemoveSentinel() clears it +// +// The exact "restore success → clear" decision lives in upgrade.go's +// restoreSucceeded gate, reviewed by the same reader that owns the +// sentinel field. The companion fix on the manual `nodeup packages +// restore` path adds the inverse: a successful restore there now +// also calls RemoveSentinel() so a follow-up `nodeup` doesn't keep +// printing the "interrupted upgrade" hint. +func TestRemoveSentinel_OnlyOnRestoreSuccess(t *testing.T) { + redirectDataDir(t) + + // Plant the sentinel — simulates an interrupted upgrade from a + // previous run. + if err := WriteSentinel(UpgradeSentinel{ + StartedAt: time.Now(), + Manager: "fnm", + OldVersion: "20.9.0", + NewVersion: "20.10.0", + SnapshotPath: "/tmp/snap.json", + }); err != nil { + t.Fatalf("WriteSentinel: %v", err) + } + + // Sanity-check: subsequent run would print the "interrupted + // upgrade" warning — so the sentinel must be visible to + // OrphanedSentinel. + s, err := OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after write: %v", err) + } + if s == nil { + t.Fatal("OrphanedSentinel returned nil after a sentinel was written") + } + if s.Manager != "fnm" { + t.Fatalf("sentinel Manager = %q, want fnm", s.Manager) + } + + // The fix in upgrade.go: this RemoveSentinel() must only be + // reached when the restore step succeeded. We assert the + // primitive it depends on is correct: removal leaves the file + // gone, and OrphanedSentinel goes back to returning nil. + if err := RemoveSentinel(); err != nil { + t.Fatalf("RemoveSentinel: %v", err) + } + + s, err = OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after RemoveSentinel: %v", err) + } + if s != nil { + t.Fatalf("OrphanedSentinel = %+v, want nil after RemoveSentinel", s) + } +} + // dataDirForTest is a small helper that returns the redirected DataDir // so cleanup / verification tests can introspect the filesystem. func dataDirForTest() (string, error) {