From 6ff67ff9f3ecc40bc997dd9fc165d3a4306f9db4 Mon Sep 17 00:00:00 2001 From: dipto0321 Date: Fri, 3 Jul 2026 08:33:13 +0600 Subject: [PATCH] fix(packages): keep upgrade sentinel on restore failure; clear on manual restore success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upgrade-in-progress sentinel had a broken lifecycle in both directions (issue #46). On the `nodeup upgrade` side, the deferred `RemoveSentinel()` fired unconditionally once the sentinel had been armed, even when the post-install package restore had failed (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" for the manual `nodeup packages restore --from ` workflow that `PersistentPreRunE` advertises. The defer now gates on `restoreSucceeded`: only a successful restore clears the sentinel, a failed restore keeps it so the user can recover via the documented replay command. On the `nodeup packages restore` side — the exact command PersistentPreRunE's hint tells the user to run after an interrupted upgrade — a successful restore now calls `packages.RemoveSentinel()`. Without this, a user who followed the printed instructions to recover from a crash kept seeing "interrupted upgrade" warnings on every subsequent `nodeup` invocation forever. Sentinel-clearing failure there is logged, not returned: the user's primary goal (restored packages) is already achieved and a stale sentinel is cosmetic. Also corrects the CHANGELOG claim that the next run "prompts the user to replay" the migration — the implementation only prints a two-line stderr hint naming the snapshot path and the exact `nodeup packages restore --from ` command to copy. No interactive prompt is shown. Closes #46. Co-Authored-By: puku-ai-2.8 --- CHANGELOG.md | 30 +++++++++++-- internal/cli/packages.go | 20 +++++++++ internal/cli/upgrade.go | 12 ++++- internal/packages/sentinel_test.go | 70 ++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 5 deletions(-) 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) {