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

[dipto0321]

Summary

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 armed — even when the post-install package restore had failed (the restore step only logs a warning and returns normally). The user was left with installed Node versions, unmigrated global packages, and a deleted "resume breadcrumb," so the documented replay flow was a dead end.

The fix gates the deferred cleanup on a new restoreSucceeded flag that only flips true when packages.RestoreFromSnapshot returns nil. A failed restore keeps the sentinel on disk so a follow-up nodeup packages restore --from <sentinel path> (the command PersistentPreRunE's hint already prints) can still find and replay it.

The companion half of the bug: a successful manual nodeup packages restore was also failing to clear the sentinel. A user who followed the printed recovery instructions to completion kept seeing "interrupted upgrade" warnings on every subsequent nodeup invocation forever. Both branches of runRestore (the --from <path> form and the <manager> <version> form) now call packages.RemoveSentinel() after success. A removal failure is logged, not returned: the user's primary goal (restored packages) is achieved and a stale sentinel is cosmetic.

CHANGELOG wording fix: the previous "prompts the user to replay" claim is replaced with a faithful description of the actual UX (two-line stderr hint naming the snapshot path and the exact nodeup packages restore --from <path> command to copy). No interactive prompt is shipped.

Closes #46.

Linked issues

• Closes bug(packages): restore aborts on first failed install; migration report never written #46

Type of change

• fix — bug fix (PATCH bump)

Checklist

• Title follows Conventional Commits (fix(scope): subject)
• I ran make ci locally and it passes (fmt, vet, test pass locally; make lint fails on the v2→v1 schema mismatch tracked in build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 — pre-existing on main, independent of this fix; CI runs golangci-lint v1.64.8 per .github/workflows/ci.yml and parses the config fine)
• I added or updated tests for the change (new internal/packages/sentinel_test.go::TestRemoveSentinel_OnlyOnRestoreSuccess regression pin: write sentinel → assert orphaned → RemoveSentinel → assert cleared; documents the contract that the cli/upgrade.go defer depends on)
• I updated relevant docs (README, docs/, inline godoc)
• No new linter warnings
• If breaking: I documented the migration path in the PR body and updated CHANGELOG.md (N/A — no CLI surface breaking; behavior change is "sentinel survives failure + clears on manual restore success")

Scope notes

• The runUpgrade defer pattern now reads as:

  defer func() {
      if sentinelArmed && restoreSucceeded {
          if err := packages.RemoveSentinel(); err != nil { ... }
      }
  }()

  sentinelArmed still flips on WriteSentinel success (the only way to ever touch the sentinel-armed flag). restoreSucceeded flips inside the if !skipMigrate && len(toInstall) > 0 block, only on the nil branch of RestoreFromSnapshot. A nil sentinel-armed + nil restoreSucceeded (the dry-run path) leaves the file untouched; a successful migration clears it; a failed migration leaves it for the user to resolve.
• The "did a previous run crash?" check on the next nodeup upgrade is intentionally not added here — that would be a separate UX feature (an actual interactive prompt + auto-replay), and the issue explicitly accepts the wording fix as one of the two acceptable resolutions. Out of scope.
• I considered adding a CLI integration test that drives a real runUpgrade (via a stub manager) and asserts sentinel state on both the success and failure paths. It would require factoring the restore logic out of runUpgrade (currently inlined as a 6-line block), which is a bigger refactor than this fix wants. The packages-level test pin + the post-fix manual verification path (nodeup upgrade → kill mid-restore → check sentinel exists → nodeup packages restore --from <path> → check sentinel gone) cover the contract.
• I also did NOT change internal/cli/root.go's warnInterruptedUpgrade — its fmt.Fprintf(os.Stderr, ...) two-line hint format is correct; the CHANGELOG claim was the wrong half of the bug, not the implementation.

Out of scope (separate issues)

• internal/cli/packages.go still uses context.Background() at line ~141 for Restore — fix(cli): context.Background() used instead of cmd.Context() in packages.go (contextcheck violation) #49.
• nodeup upgrade's restore loop does not have HTTP timeout/retry (manifest fetch) — bug(node): no HTTP timeout/retry/context on nodejs.org manifest fetch; cache write not atomic #48.
• --cleanup + --yes conflict — bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57.
• path traversal in packages restore/diff via unsanitized manager name — fix(packages): path traversal via unsanitized manager name in packages restore/diff #51.
• asdf ASDF_DIR vs ASDF_DATA_DIR inconsistency — fix(detector): asdf uses ASDF_DIR vs ASDF_DATA_DIR inconsistently between docs, detector, and system-node classifier #50.

Screenshots / output

User-visible behavior change:

• Before:

  • Scenario A (restore fails during upgrade): user runs nodeup upgrade; one of the npm install -g calls errors out; restore logs Warning: restore failed: …; function returns success; deferred RemoveSentinel() fires; next nodeup upgrade finds no sentinel; user has no documented path to retry the migration despite the snapshot file still being on disk.
  • Scenario B (manual recovery): user runs nodeup upgrade; crashes mid-upgrade; user follows the printed hint: nodeup packages restore --from /path/to/snapshot.json; packages restore completes successfully; user runs any other nodeup command; sees Detected an interrupted upgrade… warning again, repeating forever.

• After:

  • Scenario A: user runs nodeup upgrade; restore fails; next nodeup invocation prints Detected an interrupted upgrade (snapshot: …) and the suggested nodeup packages restore --from … command, allowing recovery.
  • Scenario B: user follows the printed hint, restore succeeds, sentinel is removed, subsequent nodeup invocations are silent.

Verification path: in either scenario, capture ls $(nodeup-config --datadir 2>/dev/null || echo ~/.local/share/nodeup)/upgrade-in-progress.json before/after restore to confirm file presence/absence matches the new contract.

[cocogitto-bot]

✔️ 6ff67ff - Conventional commits check succeeded.