fix(packages): keep upgrade sentinel on restore failure; clear on manual restore success

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 <snapshot-path>` 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 <path>` invocation the user can
+  copy to replay the migration (PR #29). `nodeup packages restore`
+  accepts a `--from <snapshot-path>` 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
   (`<DataDir>/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 <sentinel path>` 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 <path>`); 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

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 <sentinel path>`. 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

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 <path>`
+	// (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
 		}
 	}
 

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 <path>` 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) {