From 963aa31363d39d055099fbfd021bc42b6b282313 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Mon, 2 Nov 2026 14:22:01 +0600 Subject: [PATCH] fix(packages): the upgrade marker outlives its snapshot and warns forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Detected an interrupted upgrade" warning could fire on every nodeup invocation, pointing at a file that no longer existed and telling the user to run a command that was guaranteed to fail. The marker (/upgrade-in-progress.json) is deliberately left behind when a migration fails, so the user can replay it — that is the #46 contract. 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 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: - snapshot still present → genuine interrupted upgrade. Warning and resume hint unchanged; the marker is NOT cleared. - snapshot 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 window can never have it deleted out from under it — the fresh marker is put back untouched. Compare-then-delete would have a TOCTOU window there; the claim closes it. `nodeup packages cleanup --stale-sentinel` clears a stale marker on demand. It refuses to clear one whose snapshot still exists: that represents a genuine interrupted migration, and discarding it would lose the only breadcrumb telling the user how to replay their globals. Also fixes a test-hygiene wart this change exposed. A CLI test that built a root command ran against the developer's real $HOME, so it probed the real marker — and could now delete it. Running the suite silently wiped the marker on this machine. 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. Closes #141 --- CHANGELOG.md | 48 ++++ README.md | 33 ++- SPEC.md | 3 +- internal/cli/cleanup_snapshots.go | 68 ++++- internal/cli/hermeticity_test.go | 109 ++++++++ internal/cli/root.go | 69 ++++- internal/cli/stale_sentinel_test.go | 296 ++++++++++++++++++++++ internal/cli/version_test.go | 10 + internal/packages/sentinel.go | 181 +++++++++++++ internal/packages/stale_sentinel_test.go | 309 +++++++++++++++++++++++ 10 files changed, 1119 insertions(+), 7 deletions(-) create mode 100644 internal/cli/hermeticity_test.go create mode 100644 internal/cli/stale_sentinel_test.go create mode 100644 internal/packages/stale_sentinel_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 94d21bf..4149fac 100644 --- a/CHANGELOG.md +++ b/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* + (`/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 diff --git a/README.md b/README.md index 1e3544a..7dfc4db 100644 --- a/README.md +++ b/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: diff --git a/SPEC.md b/SPEC.md index 16c2e14..b12c9da 100644 --- a/SPEC.md +++ b/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: `/snapshots/-.json` - file: `/reports/migration-.json` (one per upgrade, success or partial) -- file: `/upgrade-in-progress.json` (sentinel) +- file: `/upgrade-in-progress.json` (sentinel; cleared automatically once + the snapshot it points at is gone) - file: `/nodeup.lock` - env: `NODEUP_MANAGER`, `NODEUP_TRACK_LTS`, `NODEUP_TRACK_CURRENT`, `NODEUP_PACKAGES_MIGRATE`, `NODEUP_PACKAGES_STRATEGY`, diff --git a/internal/cli/cleanup_snapshots.go b/internal/cli/cleanup_snapshots.go index cc092af..7dc05d0 100644 --- a/internal/cli/cleanup_snapshots.go +++ b/internal/cli/cleanup_snapshots.go @@ -46,12 +46,18 @@ 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, } @@ -59,6 +65,7 @@ Examples: 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 /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 +} diff --git a/internal/cli/hermeticity_test.go b/internal/cli/hermeticity_test.go new file mode 100644 index 0000000..b145d1b --- /dev/null +++ b/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 +// /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 /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 "" +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 68fa3f7..9b230f6 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -29,15 +29,76 @@ import ( // "no sentinel" are deliberately swallowed: a corrupted sentinel file // is a cosmetic issue and should not prevent the user's actual command // from running. +// +// # THE SENTINEL CAN OUTLIVE ITS SNAPSHOT +// +// The sentinel's contract is "there is a migration you can replay; the +// payload for it lives at SnapshotPath". That contract only holds while +// the snapshot survives — and two things can delete it out from under +// the sentinel: the upgrade flow's own auto-reclaim (#128) and +// `nodeup packages cleanup`. +// +// When the snapshot is gone, the warning below becomes actively +// harmful. It points at a dead file and tells the user to run a +// command that cannot possibly succeed: +// +// ⚠ Detected an interrupted upgrade (snapshot: .../fnm-26.10.0.json, ...) +// ⚠ To resume: `nodeup packages restore --from .../fnm-26.10.0.json` +// Error: restore failed: read snapshot ...: no such file or directory +// +// And since the sentinel lives at /upgrade-in-progress.json — +// outside the snapshots directory — `nodeup packages cleanup` never +// touched it, so the warning fired on every invocation forever with no +// way to dismiss it. +// +// So we validate the sentinel before acting on it: +// +// - Snapshot still present → genuine interrupted upgrade. Print the +// warning and the resume hint, exactly as before. (This is the +// #46 contract: a failed restore MUST leave the sentinel armed so +// the migration can be replayed.) +// - Snapshot gone → the sentinel is stale. Say so plainly, clear it, +// and do NOT print a resume hint that cannot work. func warnInterruptedUpgrade(w ui.Writer) { s, err := packages.OrphanedSentinel() if err != nil || s == nil { return } - w.Warn(fmt.Sprintf("Detected an interrupted upgrade (snapshot: %s, started: %s).", - s.SnapshotPath, s.StartedAt.Format("2006-01-02T15:04:05Z07:00"))) - w.Warn(fmt.Sprintf("To resume: `nodeup packages restore --from %s`", - s.SnapshotPath)) + + // The snapshot is still there → this is a real interrupted upgrade. + // Print the resume hint. Nothing is cleared: the sentinel is the + // breadcrumb that lets the user replay the migration. + if packages.SentinelSnapshotExists(s) { + w.Warn(fmt.Sprintf("Detected an interrupted upgrade (snapshot: %s, started: %s).", + s.SnapshotPath, s.StartedAt.Format("2006-01-02T15:04:05Z07:00"))) + w.Warn(fmt.Sprintf("To resume: `nodeup packages restore --from %s`", + s.SnapshotPath)) + return + } + + // The sentinel points at a snapshot that no longer exists. Clear it + // so the warning stops firing, and tell the user why. + // + // We use the compare-and-delete variant so we cannot clobber a + // FRESH sentinel armed by a concurrent `nodeup upgrade` between our + // read and our delete. + removed, rerr := packages.RemoveSentinelIfUnchanged(s) + if rerr != nil { + // Best-effort. A failure to remove the file is cosmetic — the + // user's command still runs. We stay quiet here rather than + // printing a second warning on top of the one below, which + // would be noise; the stale sentinel will be retried on the + // next invocation. + return + } + if !removed { + // The sentinel on disk changed since we read it — a concurrent + // upgrade armed a fresh one. Leave it entirely alone. + return + } + + w.Warn(fmt.Sprintf("Cleared a stale upgrade marker (the snapshot it pointed at no longer exists, so there was nothing to resume). It was last used on %s.", + s.StartedAt.Format("2006-01-02"))) } // NewRootCmd builds the root `nodeup` command with all subcommands attached. diff --git a/internal/cli/stale_sentinel_test.go b/internal/cli/stale_sentinel_test.go new file mode 100644 index 0000000..119d009 --- /dev/null +++ b/internal/cli/stale_sentinel_test.go @@ -0,0 +1,296 @@ +package cli + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/dipto0321/nodeup/internal/packages" + "github.com/dipto0321/nodeup/internal/ui" +) + +// These tests cover the CLI half of the stale-sentinel fix (#141). +// +// The bug: the "Detected an interrupted upgrade" warning fired on every +// nodeup invocation whenever a sentinel file existed — even after the +// snapshot it pointed at had been deleted. The warning pointed at a +// dead file and told the user to run a command that could not succeed, +// and `nodeup packages cleanup` never touched the sentinel, so there +// was no way to dismiss it. +// +// The fix: the warning validates its own advice. A sentinel whose +// snapshot is gone is cleared and reported as stale; a sentinel whose +// snapshot is alive keeps the existing behavior exactly. + +// redirectSentinelDataDir points nodeup's data directory at a temp dir +// so these tests never see (or touch) the developer's real sentinel. +// Mirrors redirectCleanupDataDir in cleanup_snapshots_test.go — the +// helpers aren't shared because the packages package has its own copy +// and the cli package can't import a test helper. +func redirectSentinelDataDir(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + + t.Setenv("XDG_DATA_HOME", "") + t.Setenv("APPDATA", "") + t.Setenv("USERPROFILE", tmp) + t.Setenv("HOME", tmp) + return tmp +} + +// writeSentinelAt plants an upgrade marker pointing at the given +// snapshot path and returns the sentinel. +func writeSentinelAt(t *testing.T, snapshotPath string) packages.UpgradeSentinel { + t.Helper() + return writeSentinelFor(t, "fnm", snapshotPath) +} + +// writeSentinelFor plants an upgrade marker for a specific manager. +func writeSentinelFor(t *testing.T, manager, snapshotPath string) packages.UpgradeSentinel { + t.Helper() + s := packages.UpgradeSentinel{ + StartedAt: time.Now().UTC().Truncate(time.Second), + Manager: manager, + OldVersion: "26.10.0", + NewVersion: "26.12.1", + SnapshotPath: snapshotPath, + } + if err := packages.WriteSentinel(s); err != nil { + t.Fatalf("WriteSentinel: %v", err) + } + return s +} + +// newWarningWriter builds a PlainMode writer over the two buffers and +// returns it plus the buffers. +func newWarningWriter() (ui.Writer, *bytes.Buffer, *bytes.Buffer) { + var out, errBuf bytes.Buffer + return ui.NewWriter(ui.PlainMode, &out, &errBuf), &out, &errBuf +} + +// TestWarnInterruptedUpgrade_StaleSentinelIsClearedAndNotWarned is the +// core regression test for #141. +// +// A sentinel whose snapshot no longer exists must NOT produce the +// "Detected an interrupted upgrade" warning, and it must be removed +// from disk so the warning does not come back on the next invocation. +func TestWarnInterruptedUpgrade_StaleSentinelIsClearedAndNotWarned(t *testing.T) { + redirectSentinelDataDir(t) + + // The snapshot path deliberately points at a file that isn't there. + missing := filepath.Join(t.TempDir(), "fnm-26.10.0.json") + writeSentinelAt(t, missing) + + w, out, errBuf := newWarningWriter() + warnInterruptedUpgrade(w) + + combined := out.String() + errBuf.String() + if strings.Contains(combined, "Detected an interrupted upgrade") { + t.Errorf("stale sentinel still produced the interrupted-upgrade warning:\n%s", combined) + } + if strings.Contains(combined, "packages restore --from") { + t.Errorf("stale sentinel still produced a resume hint that cannot work:\n%s", combined) + } + if !strings.Contains(combined, "stale") { + t.Errorf("expected the stale marker to be reported, got:\n%s", combined) + } + + // And the sentinel must actually be gone from disk. + after, err := packages.OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after warning: %v", err) + } + if after != nil { + t.Errorf("stale sentinel = %+v, want nil — it should have been cleared", after) + } +} + +// TestWarnInterruptedUpgrade_LiveSentinelStillWarns is the +// non-regression pin. +// +// When the snapshot IS still on disk, the sentinel represents a genuine +// interrupted migration. The warning and the resume hint must still +// appear, and the sentinel must NOT be cleared — the #46 contract says +// a failed restore leaves the breadcrumb armed so the user can replay +// the migration. +func TestWarnInterruptedUpgrade_LiveSentinelStillWarns(t *testing.T) { + redirectSentinelDataDir(t) + + // A real snapshot file on disk. + dir := t.TempDir() + snap := filepath.Join(dir, "fnm-26.10.0.json") + if err := os.WriteFile(snap, []byte(`{"manager":"fnm","packages":[]}`), 0o644); err != nil { + t.Fatalf("write snapshot: %v", err) + } + writeSentinelAt(t, snap) + + w, out, errBuf := newWarningWriter() + warnInterruptedUpgrade(w) + + combined := out.String() + errBuf.String() + if !strings.Contains(combined, "Detected an interrupted upgrade") { + t.Errorf("live sentinel did not produce the warning:\n%s", combined) + } + if !strings.Contains(combined, "packages restore --from") { + t.Errorf("live sentinel did not produce the resume hint:\n%s", combined) + } + if !strings.Contains(combined, snap) { + t.Errorf("resume hint does not mention the snapshot path %q:\n%s", snap, combined) + } + + // The sentinel must still be there — this is the resume breadcrumb. + after, err := packages.OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel: %v", err) + } + if after == nil { + t.Error("live sentinel was cleared — the resume breadcrumb was lost") + } +} + +// TestWarnInterruptedUpgrade_NoSentinelIsSilent covers the quiet case: +// no sentinel at all means no output whatsoever. +func TestWarnInterruptedUpgrade_NoSentinelIsSilent(t *testing.T) { + redirectSentinelDataDir(t) + + w, out, errBuf := newWarningWriter() + warnInterruptedUpgrade(w) + + if combined := out.String() + errBuf.String(); combined != "" { + t.Errorf("expected no output with no sentinel, got:\n%s", combined) + } +} + +// TestWarnInterruptedUpgrade_DoesNotClobberAFreshSentinel guards against the +// fix being too eager: if the sentinel on disk changed since the +// warning inspected it (a concurrent upgrade armed a fresh one), the +// fresh one must survive. +// +// The deterministic version of this race — the one where the swap lands +// exactly between our read and our claim — lives in +// internal/packages/stale_sentinel_test.go, where the claim seam is. +// This test covers the CLI's half: it must not delete a sentinel whose +// snapshot is alive. +func TestWarnInterruptedUpgrade_DoesNotClobberAFreshSentinel(t *testing.T) { + redirectSentinelDataDir(t) + + // A fresh sentinel whose snapshot IS alive. The warning must not + // clear it. + live := filepath.Join(t.TempDir(), "nvm-20.0.0.json") + if err := os.WriteFile(live, []byte(`{"manager":"nvm","packages":[]}`), 0o644); err != nil { + t.Fatalf("write snapshot: %v", err) + } + writeSentinelFor(t, "nvm", live) + + w, _, _ := newWarningWriter() + warnInterruptedUpgrade(w) + + after, err := packages.OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel: %v", err) + } + if after == nil { + t.Fatal("the live sentinel was deleted — the warning clobbered it") + } + if after.Manager != "nvm" { + t.Errorf("sentinel Manager = %q, want nvm", after.Manager) + } +} + +// TestCleanupCmd_StaleSentinelFlagClearsIt covers the explicit +// recovery path: `nodeup packages cleanup --stale-sentinel`. +// +// The sentinel lives at /upgrade-in-progress.json — outside +// the snapshots and reports directories the command otherwise works +// on — so without an explicit flag there was no way for the user to +// dismiss a stale marker by hand. +func TestCleanupCmd_StaleSentinelFlagClearsIt(t *testing.T) { + redirectSentinelDataDir(t) + + missing := filepath.Join(t.TempDir(), "fnm-26.10.0.json") + writeSentinelAt(t, missing) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--stale-sentinel"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --stale-sentinel: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "Cleared the stale upgrade marker") { + t.Errorf("output %q does not confirm the marker was cleared", out) + } + + after, err := packages.OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after: %v", err) + } + if after != nil { + t.Errorf("sentinel = %+v, want nil after --stale-sentinel", after) + } +} + +// TestCleanupCmd_StaleSentinelFlagRefusesLiveSentinel is the safety +// pin: a sentinel whose snapshot still exists represents a genuine +// interrupted migration. Clearing it on request would throw away the +// only breadcrumb telling the user how to replay their globals. +func TestCleanupCmd_StaleSentinelFlagRefusesLiveSentinel(t *testing.T) { + redirectSentinelDataDir(t) + + dir := t.TempDir() + snap := filepath.Join(dir, "fnm-26.10.0.json") + if err := os.WriteFile(snap, []byte(`{"manager":"fnm","packages":[]}`), 0o644); err != nil { + t.Fatalf("write snapshot: %v", err) + } + writeSentinelAt(t, snap) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--stale-sentinel"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --stale-sentinel: %v", err) + } + + out := buf.String() + if !strings.Contains(out, "genuine interrupted upgrade") { + t.Errorf("output %q does not explain why the live marker was kept", out) + } + + after, err := packages.OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after: %v", err) + } + if after == nil { + t.Fatal("the live sentinel was cleared — the resume breadcrumb was lost") + } +} + +// TestCleanupCmd_StaleSentinelFlagNoMarkerIsQuiet covers the +// already-clean case. +func TestCleanupCmd_StaleSentinelFlagNoMarkerIsQuiet(t *testing.T) { + redirectSentinelDataDir(t) + + cmd := newCleanupCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--stale-sentinel"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("cleanup --stale-sentinel: %v", err) + } + + if out := buf.String(); !strings.Contains(out, "No upgrade marker found") { + t.Errorf("output %q does not report that there was no marker", out) + } +} diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index 27f3e39..ca99b36 100644 --- a/internal/cli/version_test.go +++ b/internal/cli/version_test.go @@ -129,6 +129,16 @@ func TestVersionCmd_CheckFlagIsNoOpNotError(t *testing.T) { // — in PlainMode that's a direct passthrough, so the strings are // byte-for-byte identical to the legacy test above. func TestVersionCmd_UsesInjectedWriter(t *testing.T) { + // This test builds a ROOT command, so PersistentPreRunE runs — which + // means it probes the real /upgrade-in-progress.json + // sentinel. Without this redirect the test would read (and, since + // the stale-sentinel fix, DELETE) the developer's own sentinel. + // + // That's not hypothetical: before the redirect, running the test + // suite wiped the real upgrade marker on this machine. Any test + // that constructs a root command MUST redirect the data dir. + redirectSentinelDataDir(t) + out := &bytes.Buffer{} errOut := &bytes.Buffer{} diff --git a/internal/packages/sentinel.go b/internal/packages/sentinel.go index 50c9065..7027e21 100644 --- a/internal/packages/sentinel.go +++ b/internal/packages/sentinel.go @@ -128,7 +128,17 @@ func LoadSentinel() (*UpgradeSentinel, error) { if err != nil { return nil, err } + return loadSentinelFrom(path) +} +// loadSentinelFrom reads and parses a sentinel file at an explicit +// path. +// +// Split out from LoadSentinel so the claim-by-rename path in +// RemoveSentinelIfUnchanged can parse the claimed file without going +// back through SentinelPath (which always resolves to the canonical +// location). +func loadSentinelFrom(path string) (*UpgradeSentinel, error) { data, err := os.ReadFile(path) if err != nil { if errors.Is(err, os.ErrNotExist) { @@ -163,6 +173,133 @@ func RemoveSentinel() error { return nil } +// claimSentinelFile is the package-level seam used by +// RemoveSentinelIfUnchanged to atomically claim the sentinel file. +// +// It renames `path` to `claimPath` — an atomic operation on POSIX and +// on Windows (os.Rename replaces the destination). Tests overwrite it +// to simulate a concurrent `nodeup upgrade` arming a fresh sentinel in +// the window between our read and our claim. Production code never +// reassigns it. +// +// This mirrors the `whichNode` seam in internal/detector/system_node.go +// — the project's established way of making a filesystem-timing +// dependency deterministically testable. +var claimSentinelFile = os.Rename + +// RemoveSentinelIfUnchanged deletes the sentinel ONLY if it still +// describes the same upgrade the caller inspected. +// +// This is the compare-and-delete primitive the stale-sentinel cleanup +// needs. Without it there is a race: the warning fires in +// PersistentPreRunE on EVERY nodeup invocation, so between reading the +// sentinel and deleting it, a concurrent `nodeup upgrade` could arm a +// FRESH sentinel whose snapshot is alive. An unconditional delete would +// clobber that live breadcrumb, and the user would lose the resume +// hint for a migration that genuinely needs replaying. +// +// # WHY CLAIM-BY-RENAME, NOT COMPARE-THEN-DELETE +// +// The obvious implementation — read the sentinel, compare it, then +// os.Remove it — has a TOCTOU window: a concurrent writer can replace +// the file between the compare and the delete, and we would then delete +// the NEW file, not the one we judged. Re-reading immediately before the +// delete narrows the window but does not close it. +// +// Instead we CLAIM the file first: os.Rename(path, claimPath) atomically +// moves whatever is currently at `path` to a unique name. Whatever we +// claimed, we now own exclusively — a concurrent writer can no longer +// touch it. We then read the claimed file: +// +// - It matches the sentinel we judged stale → delete the claim. Done. +// - It does NOT match → a concurrent upgrade armed a fresh sentinel +// after we read ours, and we accidentally claimed THAT one. Put it +// back and report that we removed nothing. +// +// The compare now happens on a file we exclusively own, so there is no +// window between "decide" and "act". +func RemoveSentinelIfUnchanged(target *UpgradeSentinel) (removed bool, err error) { + if target == nil { + return false, nil + } + + path, err := SentinelPath() + if err != nil { + return false, err + } + + // Claim the file. The claim path is unique per process so two + // concurrent cleanups can't collide on it. + claimPath := fmt.Sprintf("%s.claim-%d-%d", path, os.Getpid(), time.Now().UnixNano()) + if err := claimSentinelFile(path, claimPath); err != nil { + if errors.Is(err, os.ErrNotExist) { + // Already gone — someone else cleaned it up. The desired + // end state holds, so report success without an error. + return false, nil + } + return false, fmt.Errorf("claim sentinel: %w", err) + } + + // From here on the claimed file is ours. Every early return must + // either delete it or put it back — never leave it stranded under + // the claim name, or the sentinel would vanish from the user's + // view entirely. + claimed, lerr := loadSentinelFrom(claimPath) + if lerr != nil { + // We can't tell what we claimed. Put it back rather than + // destroying an upgrade breadcrumb we don't understand. + _ = os.Rename(claimPath, path) + if errors.Is(lerr, ErrNoSentinel) { + return false, nil + } + return false, fmt.Errorf("read claimed sentinel: %w", lerr) + } + + if !sameSentinel(claimed, target) { + // The file we claimed is NOT the one the caller inspected — a + // concurrent upgrade armed a fresh sentinel in the meantime. + // Put it back exactly where it came from and leave it alone. + if rerr := os.Rename(claimPath, path); rerr != nil { + return false, fmt.Errorf("restore concurrently-written sentinel: %w", rerr) + } + return false, nil + } + + // It's the sentinel we judged stale. Delete the claim. + if err := os.Remove(claimPath); err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + // We own the file but couldn't delete it. Put it back so the + // user still sees their marker and can retry. + _ = os.Rename(claimPath, path) + return false, fmt.Errorf("delete stale sentinel: %w", err) + } + return true, nil +} + +// sameSentinel reports whether two sentinels describe the same upgrade. +// +// We compare the fields that identify the upgrade rather than the whole +// struct: StartedAt (when it began), Manager, and SnapshotPath (the +// payload it points at). NewVersion and OldVersion are included for +// completeness — a sentinel that differs in any of these is a +// different upgrade. +// +// StartedAt is compared with Equal rather than == because it round-trips +// through JSON, which preserves the instant but not the monotonic clock +// reading and location pointer. +func sameSentinel(a, b *UpgradeSentinel) bool { + if a == nil || b == nil { + return false + } + return a.StartedAt.Equal(b.StartedAt) && + a.Manager == b.Manager && + a.OldVersion == b.OldVersion && + a.NewVersion == b.NewVersion && + a.SnapshotPath == b.SnapshotPath +} + // OrphanedSentinel returns the parsed sentinel if one exists, or nil if // not. This is the "did the last run get interrupted?" check that // PersistentPreRunE uses on every `nodeup` invocation. Errors other than @@ -179,3 +316,47 @@ func OrphanedSentinel() (*UpgradeSentinel, error) { } return s, nil } + +// SentinelSnapshotExists reports whether the snapshot a sentinel points +// at is still on disk. +// +// This is what makes the interrupted-upgrade warning self-validating. +// +// The sentinel's contract is "there is a migration you can replay; +// the payload for it lives at SnapshotPath". That contract only holds +// while the snapshot survives. Two things can break it: +// +// - The upgrade flow's own auto-reclaim, which deletes the snapshot +// files once the migration has replayed them (see #128). +// - `nodeup packages cleanup`, which lets the user delete any +// snapshot by hand. +// +// When either happens, the sentinel outlives its payload: the warning +// keeps firing on every invocation, it points at a file that no longer +// exists, and the recovery command it suggests is guaranteed to fail +// with "read snapshot ...: no such file or directory". The user is +// left with a warning they cannot act on and no way to dismiss it. +// +// A sentinel with an EMPTY SnapshotPath is treated as stale: there is +// nothing to replay, so there is nothing to warn about. (The sentinel +// is only armed when there is something to migrate — see the +// `len(toInstall) > 0` gate in internal/cli/upgrade.go — so an empty +// path means the file was written by something else, or hand-edited.) +// +// The check is a plain os.Stat. A snapshot that exists but is +// unparseable is still "present" as far as this function is concerned +// — the restore path will report the corruption, and a corruption is a +// different problem from a missing file. +func SentinelSnapshotExists(s *UpgradeSentinel) bool { + if s == nil || s.SnapshotPath == "" { + return false + } + info, err := os.Stat(s.SnapshotPath) + if err != nil { + return false + } + // A directory at the snapshot path is not a snapshot. Treat it as + // missing so the warning doesn't point the user at something the + // restore command cannot read. + return !info.IsDir() +} diff --git a/internal/packages/stale_sentinel_test.go b/internal/packages/stale_sentinel_test.go new file mode 100644 index 0000000..35d01e4 --- /dev/null +++ b/internal/packages/stale_sentinel_test.go @@ -0,0 +1,309 @@ +package packages + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// These tests cover the "stale sentinel" class of bug: the upgrade +// marker outliving the snapshot it points at. +// +// The sentinel's contract is "there is a migration you can replay; the +// payload for it lives at SnapshotPath". Two things can delete the +// payload out from under it: +// +// - the upgrade flow's own auto-reclaim, which removes the snapshot +// files once the migration has replayed them (#128) +// - `nodeup packages cleanup`, which lets the user delete any +// snapshot by hand +// +// When that happens the marker keeps firing an "interrupted upgrade" +// warning that points at a dead file and tells the user to run a +// command that cannot succeed. See issue #141. + +// writeSnapshotAt drops a snapshot file at an absolute path and returns +// it. The package already has a writeSnapshotFile helper, but that one +// always writes into the snapshots directory; these tests need to place +// a file at an arbitrary path (and to place a file at a path that does +// NOT exist), so we need the absolute-path variant. +func writeSnapshotAt(t *testing.T, path string) string { + t.Helper() + if err := os.WriteFile(path, []byte(`{"manager":"fnm","packages":[]}`), 0o644); err != nil { + t.Fatalf("write snapshot %s: %v", path, err) + } + return path +} + +// TestSentinelSnapshotExists_TrueWhenPresent is the happy path: a +// sentinel pointing at a snapshot that IS on disk reports true. This is +// the genuine interrupted-upgrade case, and the caller must keep +// warning about it — the #46 contract depends on it. +func TestSentinelSnapshotExists_TrueWhenPresent(t *testing.T) { + redirectDataDir(t) + + dir := t.TempDir() + snap := writeSnapshotAt(t, filepath.Join(dir, "fnm-20.10.0.json")) + + s := &UpgradeSentinel{Manager: "fnm", SnapshotPath: snap} + if !SentinelSnapshotExists(s) { + t.Errorf("SentinelSnapshotExists = false, want true (snapshot is on disk)") + } +} + +// TestSentinelSnapshotExists_FalseWhenMissing is the core regression +// test for #141: the snapshot is gone, so the sentinel is stale and the +// caller must NOT print a resume hint. +func TestSentinelSnapshotExists_FalseWhenMissing(t *testing.T) { + redirectDataDir(t) + + dir := t.TempDir() + snap := filepath.Join(dir, "fnm-20.10.0.json") + + s := &UpgradeSentinel{Manager: "fnm", SnapshotPath: snap} + if SentinelSnapshotExists(s) { + t.Errorf("SentinelSnapshotExists = true, want false (snapshot does not exist)") + } +} + +// TestSentinelSnapshotExists_FalseWhenPathEmpty covers the sentinel +// with no recorded snapshot path. There is nothing to replay, so there +// is nothing to warn about. +func TestSentinelSnapshotExists_FalseWhenPathEmpty(t *testing.T) { + redirectDataDir(t) + + if SentinelSnapshotExists(&UpgradeSentinel{Manager: "fnm"}) { + t.Errorf("SentinelSnapshotExists = true, want false (no snapshot path recorded)") + } +} + +// TestSentinelSnapshotExists_FalseWhenNil guards the nil case — the +// caller must not have to nil-check before calling. +func TestSentinelSnapshotExists_FalseWhenNil(t *testing.T) { + redirectDataDir(t) + + if SentinelSnapshotExists(nil) { + t.Errorf("SentinelSnapshotExists(nil) = true, want false") + } +} + +// TestSentinelSnapshotExists_FalseWhenPathIsDirectory covers the +// "a directory sits where the snapshot should be" case. The restore +// command cannot read a directory, so the hint would still be dead. +func TestSentinelSnapshotExists_FalseWhenPathIsDirectory(t *testing.T) { + redirectDataDir(t) + + dir := t.TempDir() + sub := filepath.Join(dir, "fnm-20.10.0.json") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + s := &UpgradeSentinel{Manager: "fnm", SnapshotPath: sub} + if SentinelSnapshotExists(s) { + t.Errorf("SentinelSnapshotExists = true, want false (path is a directory)") + } +} + +// TestRemoveSentinelIfUnchanged_RemovesMatching pins the happy path: +// when the file on disk still describes the same upgrade the caller +// inspected, it is deleted. +func TestRemoveSentinelIfUnchanged_RemovesMatching(t *testing.T) { + redirectDataDir(t) + + written := UpgradeSentinel{ + StartedAt: time.Now().UTC().Truncate(time.Second), + Manager: "fnm", + OldVersion: "26.10.0", + NewVersion: "26.12.1", + SnapshotPath: "/gone/fnm-26.10.0.json", + } + if err := WriteSentinel(written); err != nil { + t.Fatalf("WriteSentinel: %v", err) + } + + // Read it back the way the CLI does, so we compare against exactly + // what a caller would hold. + loaded, err := OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel: %v", err) + } + if loaded == nil { + t.Fatal("OrphanedSentinel returned nil after write") + } + + removed, rerr := RemoveSentinelIfUnchanged(loaded) + if rerr != nil { + t.Fatalf("RemoveSentinelIfUnchanged: %v", rerr) + } + if !removed { + t.Fatal("RemoveSentinelIfUnchanged = false, want true (sentinel unchanged)") + } + + // And the file must actually be gone. + after, err := OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after removal: %v", err) + } + if after != nil { + t.Errorf("OrphanedSentinel = %+v, want nil after removal", after) + } +} + +// TestRemoveSentinelIfUnchanged_LeavesDifferentSentinel is the +// race-safety pin. +// +// The warning fires on EVERY nodeup invocation, so between a caller +// reading the sentinel and deleting it, a concurrent `nodeup upgrade` +// could arm a FRESH sentinel whose snapshot is alive. An unconditional +// delete would clobber that live breadcrumb. +// +// The compare-and-delete must detect the change and leave the new +// sentinel in place. +func TestRemoveSentinelIfUnchanged_LeavesDifferentSentinel(t *testing.T) { + redirectDataDir(t) + + // The stale one the caller inspects. + stale := &UpgradeSentinel{ + StartedAt: time.Now().UTC().Truncate(time.Second), + Manager: "fnm", + OldVersion: "26.10.0", + SnapshotPath: "/gone/fnm-26.10.0.json", + } + if err := WriteSentinel(*stale); err != nil { + t.Fatalf("WriteSentinel: %v", err) + } + + // A concurrent upgrade arms a FRESH sentinel, overwriting the stale + // one. This one has a live snapshot. + fresh := UpgradeSentinel{ + StartedAt: time.Now().UTC().Truncate(time.Second).Add(time.Minute), + Manager: "nvm", + OldVersion: "20.0.0", + NewVersion: "22.0.0", + SnapshotPath: "/alive/nvm-20.0.0.json", + } + if err := WriteSentinel(fresh); err != nil { + t.Fatalf("WriteSentinel (fresh): %v", err) + } + + removed, rerr := RemoveSentinelIfUnchanged(stale) + if rerr != nil { + t.Fatalf("RemoveSentinelIfUnchanged: %v", rerr) + } + if removed { + t.Error("RemoveSentinelIfUnchanged = true, want false (sentinel changed on disk)") + } + + // The FRESH sentinel must survive — its snapshot is alive, so the + // resume breadcrumb is still valid. + after, err := OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after: %v", err) + } + if after == nil { + t.Fatal("the fresh sentinel was deleted — the compare-and-delete clobbered it") + } + if after.Manager != "nvm" || after.SnapshotPath != fresh.SnapshotPath { + t.Errorf("sentinel = %+v, want the fresh one (%+v)", after, fresh) + } +} + +// TestRemoveSentinelIfUnchanged_ConcurrentUpgradeWins is the +// deterministic race test. +// +// The window is narrow — between our read of the sentinel and our +// claim of it — so triggering it by timing would be flaky. The +// `claimSentinelFile` seam lets us reproduce it exactly: the seam +// fires at the moment of the claim, which is precisely where a +// concurrent `nodeup upgrade` would arm its fresh sentinel. +// +// The fresh sentinel MUST survive. Clobbering it would destroy the +// resume breadcrumb for a migration the user genuinely needs to +// replay. +func TestRemoveSentinelIfUnchanged_ConcurrentUpgradeWins(t *testing.T) { + redirectDataDir(t) + + stale := UpgradeSentinel{ + StartedAt: time.Now().UTC().Truncate(time.Second), + Manager: "fnm", + OldVersion: "26.10.0", + SnapshotPath: "/gone/fnm-26.10.0.json", + } + if err := WriteSentinel(stale); err != nil { + t.Fatalf("WriteSentinel: %v", err) + } + + fresh := UpgradeSentinel{ + StartedAt: stale.StartedAt.Add(time.Minute), + Manager: "nvm", + OldVersion: "20.0.0", + NewVersion: "22.0.0", + SnapshotPath: "/alive/nvm-20.0.0.json", + } + + // Swap the seam for the duration of this test. The replacement + // writes the fresh sentinel (simulating the concurrent upgrade + // winning the race) and then performs the real rename — so we + // CLAIM the fresh sentinel, not the stale one. + orig := claimSentinelFile + t.Cleanup(func() { claimSentinelFile = orig }) + + claimSentinelFile = func(path, claimPath string) error { + if err := WriteSentinel(fresh); err != nil { + return err + } + return os.Rename(path, claimPath) + } + + removed, rerr := RemoveSentinelIfUnchanged(&stale) + if rerr != nil { + t.Fatalf("RemoveSentinelIfUnchanged: %v", rerr) + } + if removed { + t.Error("RemoveSentinelIfUnchanged = true, want false (we claimed a different sentinel)") + } + + // The FRESH sentinel must be back in place — put back, not + // destroyed. This is the property that matters: the concurrent + // upgrade's breadcrumb must not be lost. + after, err := OrphanedSentinel() + if err != nil { + t.Fatalf("OrphanedSentinel after: %v", err) + } + if after == nil { + t.Fatal("the fresh sentinel was deleted — the concurrent upgrade lost its breadcrumb") + } + if after.Manager != "nvm" || after.SnapshotPath != fresh.SnapshotPath { + t.Errorf("sentinel = %+v, want the fresh one (%+v)", after, fresh) + } +} + +// TestRemoveSentinelIfUnchanged_NoOpWhenAbsent covers the case where +// the sentinel is already gone (someone else cleaned it up). The +// desired end state holds, so it is not an error. +func TestRemoveSentinelIfUnchanged_NoOpWhenAbsent(t *testing.T) { + redirectDataDir(t) + + removed, rerr := RemoveSentinelIfUnchanged(&UpgradeSentinel{Manager: "fnm"}) + if rerr != nil { + t.Fatalf("RemoveSentinelIfUnchanged on missing file: %v", rerr) + } + if removed { + t.Error("RemoveSentinelIfUnchanged = true, want false (file was already absent)") + } +} + +// TestRemoveSentinelIfUnchanged_NilTarget covers the nil guard. +func TestRemoveSentinelIfUnchanged_NilTarget(t *testing.T) { + redirectDataDir(t) + + removed, rerr := RemoveSentinelIfUnchanged(nil) + if rerr != nil { + t.Fatalf("RemoveSentinelIfUnchanged(nil): %v", rerr) + } + if removed { + t.Error("RemoveSentinelIfUnchanged(nil) = true, want false") + } +}