fix(packages): the upgrade marker outlives its snapshot and warns forever

CHANGELOG.md

@@ -7,7 +7,48 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+- The "Detected an interrupted upgrade" warning could fire forever,
+  pointing at a file that no longer existed. The upgrade *marker*
+  (`<DataDir>/upgrade-in-progress.json`) is deliberately left behind
+  when a migration fails, so the user can replay it — but the
+  *snapshot* it points at does not necessarily survive: v1.2.2's
+  auto-reclaim (#130) deletes it once the migration has replayed the
+  packages, and `nodeup packages cleanup` lets the user delete any
+  snapshot by hand.
+
+  When that happened the marker outlived its payload, and every
+  `nodeup` invocation printed a warning whose own recovery command was
+  guaranteed to fail:
+
+  ```
+  ⚠ Detected an interrupted upgrade (snapshot: .../fnm-26.10.0.json, started: 2026-10-22T10:28:49+06:00).
+  ⚠ To resume: `nodeup packages restore --from .../snapshots/fnm-26.10.0.json`
+  • No snapshot files found.
+  ```
+
+  The marker lives outside the snapshots directory, so
+  `nodeup packages cleanup` never touched it and the user had no way
+  to dismiss it.
+
+  The warning now validates its own advice: if the snapshot is still
+  on disk, this is a genuine interrupted upgrade and the warning and
+  resume hint are unchanged (the #46 contract — a failed restore MUST
+  leave the marker armed so the migration can be replayed). If the
+  snapshot is gone, the marker is stale; nodeup says so and clears it
+  instead of printing a hint that cannot work.
+
+  Clearing is race-safe. The marker is claimed with an atomic rename
+  before it is inspected, so a concurrent `nodeup upgrade` that arms a
+  fresh marker in the meantime can never have it deleted out from
+  under it — the fresh marker is put back untouched.
+
 ### Added
+- `nodeup packages cleanup --stale-sentinel` clears a stale upgrade
+  marker on demand. It refuses to clear a marker whose snapshot still
+  exists: that one represents a genuine interrupted migration, and
+  throwing it away would lose the only breadcrumb telling the user how
+  to replay their global npm packages.
 - `docs/release-checklist.md` now documents the full verification
   procedure for the npm wrapper's install path: the published-metadata
   check, `npm audit` against the published package, the npm 12
@@ -17,6 +58,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `publish-npm` job going green does not mean the version is visible
   on npmjs.com yet.
 
+### Fixed (tests)
+- A CLI test that built a root command ran against the developer's
+  real `$HOME`, so it probed — and, after the marker fix above, could
+  delete — the real upgrade marker. The test now redirects the data
+  dir, and a new AST-based guard test fails any future test that
+  constructs a root command without doing so.
+
 ## [1.2.3] - 2026-11-02
 
 A wrapper-only release. The Go binary is unchanged from v1.2.2 — this

README.md

@@ -63,7 +63,9 @@ A few things worth knowing before you run `nodeup`:
   `~/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.
+  snapshot written at the start. If that snapshot is later deleted,
+  the marker is cleared automatically rather than warning forever —
+  see [The interrupted-upgrade marker](#the-interrupted-upgrade-marker).
 
 ## Installation
 
@@ -230,6 +232,35 @@ Pass `--keep-snapshots` to `nodeup upgrade` (or set
 `packages.keep_snapshots: true` in the config) if you'd rather keep the
 rollback payloads around for manual replays.
 
+### The interrupted-upgrade marker
+
+When `nodeup upgrade` starts a migration it writes an *upgrade marker*
+(`~/.nodeup/upgrade-in-progress.json`) so an interrupted run can be
+detected and replayed. While the marker's snapshot is still on disk, every
+`nodeup` command tells you how to resume:
+
+```
+⚠ Detected an interrupted upgrade (snapshot: .../fnm-26.10.0.json, started: 2026-10-22T10:28:49+06:00).
+⚠ To resume: `nodeup packages restore --from .../fnm-26.10.0.json`
+```
+
+If the snapshot is deleted — by the auto-reclaim after a successful
+migration, or by `nodeup packages cleanup` — the marker is *stale*: there
+is nothing left to resume, and the hint it prints cannot work. nodeup
+detects that and clears the marker on the next run, so the warning stops
+rather than repeating forever.
+
+You can also clear one by hand:
+
+```bash
+nodeup packages cleanup --stale-sentinel
+```
+
+That only ever clears a **stale** marker. A marker whose snapshot still
+exists represents a genuine interrupted migration, and nodeup refuses to
+throw it away — replay it with the `nodeup packages restore --from ...`
+command the warning prints.
+
 ## How it works
 
 In short:

SPEC.md

@@ -50,7 +50,8 @@ manual steps, single static binary, no Node runtime dependency.
 - file: `~/.nodeup/config.yaml` (override: `$NODEUP_CONFIG`; home override: `$NODEUP_HOME`)
 - file: `<DataDir>/snapshots/<manager>-<node-version>.json`
 - file: `<DataDir>/reports/migration-<timestamp>.json` (one per upgrade, success or partial)
-- file: `<DataDir>/upgrade-in-progress.json` (sentinel)
+- file: `<DataDir>/upgrade-in-progress.json` (sentinel; cleared automatically once
+  the snapshot it points at is gone)
 - file: `<DataDir>/nodeup.lock`
 - env: `NODEUP_MANAGER`, `NODEUP_TRACK_LTS`, `NODEUP_TRACK_CURRENT`,
   `NODEUP_PACKAGES_MIGRATE`, `NODEUP_PACKAGES_STRATEGY`,

internal/cli/cleanup_snapshots.go

@@ -46,19 +46,26 @@ the rollback payload the "interrupted upgrade" warning points at — plus
 the ones from Node versions you have since uninstalled. This command
 lists what's on disk and lets you choose what to delete.
 
+The upgrade marker (upgrade-in-progress.json) lives outside the
+snapshots directory, so it is not listed above. A marker whose snapshot
+no longer exists is stale and is cleared automatically; use
+--stale-sentinel to clear one explicitly.
+
 Examples:
   nodeup packages cleanup              Select snapshots to delete
   nodeup packages cleanup --all        Delete every snapshot (asks first)
   nodeup packages cleanup --all --yes  Delete every snapshot, no prompt
   nodeup packages cleanup --reports    Clean up migration reports instead
-  nodeup packages cleanup --list       Show what would be deleted, delete nothing`,
+  nodeup packages cleanup --list       Show what would be deleted, delete nothing
+  nodeup packages cleanup --stale-sentinel  Clear a stale upgrade marker`,
 		RunE: runCleanup,
 	}
 
 	cmd.Flags().Bool("all", false, "delete every snapshot instead of selecting individually")
 	cmd.Flags().Bool("reports", false, "clean up migration reports instead of snapshots")
 	cmd.Flags().Bool("list", false, "list the files that would be deleted and exit")
 	cmd.Flags().Bool("yes", false, "skip the confirmation prompt (implies --all)")
+	cmd.Flags().Bool("stale-sentinel", false, "clear a stale upgrade marker (one whose snapshot no longer exists) and exit")
 
 	return cmd
 }
@@ -70,6 +77,21 @@ func runCleanup(cmd *cobra.Command, args []string) error {
 	reports, _ := cmd.Flags().GetBool("reports")
 	listOnly, _ := cmd.Flags().GetBool("list")
 	yes, _ := cmd.Flags().GetBool("yes")
+	staleSentinel, _ := cmd.Flags().GetBool("stale-sentinel")
+
+	// --stale-sentinel is its own narrow operation: clear the upgrade
+	// marker if it's stale, and do nothing else. It exists because the
+	// sentinel lives at <DataDir>/upgrade-in-progress.json — OUTSIDE
+	// the snapshots and reports directories this command otherwise
+	// works on — so without an explicit flag there was no way for the
+	// user to dismiss a stale marker by hand.
+	//
+	// We handle it BEFORE the snapshot/report listing so it works on a
+	// machine with no snapshots at all (which is exactly the state a
+	// user hits after the snapshot was reclaimed).
+	if staleSentinel {
+		return clearStaleSentinel(w)
+	}
 
 	// --yes implies --all: an unattended run can't answer a
 	// multi-select question, so it has to mean "delete them all".
@@ -214,3 +236,47 @@ func formatBytes(n int64) string {
 	}
 	return fmt.Sprintf("%.1f %cB", float64(n)/float64(div), "KMGTPE"[exp])
 }
+
+// clearStaleSentinel implements the `--stale-sentinel` path: it removes
+// the upgrade marker IF the snapshot it points at is gone.
+//
+// We deliberately refuse to clear a LIVE sentinel. A sentinel whose
+// snapshot still exists represents a genuine interrupted migration —
+// the user's global npm packages may not have made it across to the new
+// Node version, and the marker is the only breadcrumb telling them
+// how to replay it. Wiping that on request would be data loss, not
+// cleanup.
+//
+// The compare-and-delete primitive (RemoveSentinelIfUnchanged) means we
+// cannot clobber a fresh sentinel armed by a concurrent `nodeup upgrade`
+// between our read and our delete.
+func clearStaleSentinel(w ui.Writer) error {
+	s, err := packages.OrphanedSentinel()
+	if err != nil {
+		return fmt.Errorf("read upgrade marker: %w", err)
+	}
+	if s == nil {
+		w.Info("No upgrade marker found.")
+		return nil
+	}
+
+	if packages.SentinelSnapshotExists(s) {
+		w.Warn(fmt.Sprintf("The upgrade marker still has a snapshot (%s) — that is a genuine interrupted upgrade, not stale state.", s.SnapshotPath))
+		w.Warn(fmt.Sprintf("Replay it with: `nodeup packages restore --from %s`", s.SnapshotPath))
+		return nil
+	}
+
+	removed, rerr := packages.RemoveSentinelIfUnchanged(s)
+	if rerr != nil {
+		return fmt.Errorf("clear upgrade marker: %w", rerr)
+	}
+	if !removed {
+		// The marker changed under us — a concurrent upgrade armed a
+		// fresh one. Don't touch it.
+		w.Info("The upgrade marker changed while we were reading it — left it alone.")
+		return nil
+	}
+
+	w.Success("Cleared the stale upgrade marker.")
+	return nil
+}

internal/cli/hermeticity_test.go

@@ -0,0 +1,109 @@
+package cli
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestRootCommandTestsRedirectDataDir is a hermeticity guard.
+//
+// Constructing a root command runs PersistentPreRunE, which probes
+// <DataDir>/upgrade-in-progress.json. Before the stale-sentinel fix
+// (#141) that probe was read-only, so a test that built a root command
+// against the developer's real $HOME was merely reading their state.
+//
+// After the fix the same probe can DELETE a stale sentinel. That turned
+// a pre-existing test-hygiene wart into a real hazard: running
+// `go test ./...` on a machine with a stale upgrade marker silently
+// removed that marker — state the user might have wanted to inspect.
+//
+// The fix is that any test constructing a root command must redirect
+// the data dir first. This guard enforces that mechanically, so the
+// next person to add such a test finds out immediately rather than
+// discovering it by deleting someone's file.
+//
+// We parse the AST rather than grepping the source: a textual check
+// would be satisfied by the word "redirectSentinelDataDir" appearing
+// in a comment or an error message, which is exactly the kind of false
+// negative that lets a real violation through.
+func TestRootCommandTestsRedirectDataDir(t *testing.T) {
+	entries, err := os.ReadDir(".")
+	if err != nil {
+		t.Fatalf("read cli package dir: %v", err)
+	}
+
+	for _, entry := range entries {
+		name := entry.Name()
+		if entry.IsDir() || !strings.HasSuffix(name, "_test.go") {
+			continue
+		}
+		// This file is the guard itself; it names NewRootCmd in the
+		// strings it searches for.
+		if name == "hermeticity_test.go" {
+			continue
+		}
+
+		fset := token.NewFileSet()
+		file, err := parser.ParseFile(fset, filepath.Join(".", name), nil, 0)
+		if err != nil {
+			t.Fatalf("parse %s: %v", name, err)
+		}
+
+		for _, decl := range file.Decls {
+			fn, ok := decl.(*ast.FuncDecl)
+			if !ok || fn.Body == nil {
+				continue
+			}
+
+			buildsRoot := false
+			redirects := false
+
+			ast.Inspect(fn.Body, func(n ast.Node) bool {
+				call, ok := n.(*ast.CallExpr)
+				if !ok {
+					return true
+				}
+				switch calleeName(call.Fun) {
+				case "NewRootCmd":
+					buildsRoot = true
+				case "redirectSentinelDataDir", "redirectCleanupDataDir":
+					redirects = true
+				}
+				return true
+			})
+
+			if buildsRoot && !redirects {
+				t.Errorf("%s:%d: %s constructs a root command (NewRootCmd) but never redirects the data dir.\n"+
+					"PersistentPreRunE reads <DataDir>/upgrade-in-progress.json and, since the\n"+
+					"stale-sentinel fix, can DELETE it. Add redirectSentinelDataDir(t) at the\n"+
+					"top of the test so it never touches the developer's real nodeup state.",
+					name, fset.Position(fn.Pos()).Line, fn.Name.Name)
+			}
+		}
+	}
+}
+
+// calleeName returns the bare name of a called function, or "" for
+// anything that isn't a plain identifier call (a method call, a
+// selector on a package, etc.).
+func calleeName(fun ast.Expr) string {
+	switch f := fun.(type) {
+	case *ast.Ident:
+		return f.Name
+	case *ast.SelectorExpr:
+		// pkg.Func() — the interesting case is a method on a value
+		// (cmd.Func()), where the selector name alone would be a
+		// false positive. We only accept a selector whose receiver is
+		// a package-ish identifier we don't recognize as a value.
+		if id, ok := f.X.(*ast.Ident); ok {
+			return id.Name + "." + f.Sel.Name
+		}
+		return f.Sel.Name
+	}
+	return ""
+}