fix(cli): thread cmd.Context() through packages snapshot/restore#83
Merged
Conversation
`internal/cli/packages.go`'s runSnapshot (line 55) and runRestore (line 141) were cobra RunE functions with a live cmd.Context() available, but both called context.Background() directly instead. Ctrl-C during `nodeup packages snapshot` or `nodeup packages restore` could not cancel the in-flight `npm ls -g` / `npm install -g` subprocess; the child process kept running after the user aborted. This is the same contextcheck violation `internal/cli/upgrade.go` and `check.go` already addressed as part of #48 (the manifest fetch fix). `packages.go` was the last RunE file in the tree still using context.Background(). Fix: replace both call sites with cmd.Context(), drop the now-unused "context" import. Closes #49. Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
|
✔️ 564fc90 - Conventional commits check succeeded. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/cli/packages.go'srunSnapshotandrunRestoreare cobraRunEfunctions with a livecmd.Context()available, but both calledcontext.Background()directly. Ctrl-C duringnodeup packages snapshotornodeup packages restorecould not cancel the in-flightnpm ls -g/npm install -gsubprocess — the child process kept running after the user aborted.This is the same contextcheck violation
internal/cli/upgrade.goandcheck.goalready addressed as part of #48 (the manifest fetch fix).packages.gowas the lastRunEfile in the tree still usingcontext.Background(). Fix: replace both call sites withcmd.Context(), drop the now-unused"context"import.Closes #49.
Linked issues
Type of change
fix— bug fix (PATCH bump)Checklist
fix(scope): subject)make cilocally and it passes (fmt, vet, test pass locally;make lintfails 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 onmain, independent of this fix; CI runs golangci-lint v1.64.8 per.github/workflows/ci.ymland parses the config fine)cmd.Context()integration withcontextcheck. A focused test would require mockingpackages.Snapshot/packages.Restore, which is a larger refactor than this fix wants. The pre-existinginternal/packages/snapshot_test.go::TestSnapshot_StoredVersionKeyMatchesArgandTestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersioncover the contract pieces the cli calls.)docs/, inline godoc)make lintoutput but not in CI — once build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 is fixed, this would have been the only remaining offender)Scope notes
runSnapshotwas a single-line change:packages.Snapshot(context.Background(), m.Name(), version)→packages.Snapshot(cmd.Context(), m.Name(), version).runRestorewas a two-line change:ctx := context.Background()→ctx := cmd.Context(); the rest of the function already used the localctx, so only the source needed updating."context"import is now unused and was removed.cmd.Context()throughgetCurrentVersion(the helper at line 63) because it doesn't shell out — it just callsm.ListInstalled(), which is an in-memory slice operation. There's no Ctrl-C behavior to expose there.runRestoreto be unit-testable with acmd.Context()injection seam. Same reasoning as bug(packages): restore aborts on first failed install; migration report never written #46: a stubbedplatform.RunShellseam is a larger change than this three-liner wants, and the rest of the test surface (snapshot_test.go + restore_test.go) already validates the package-level Snapshot/Restore contract that runRestore relies on.Out of scope (separate issues)
--cleanup/--yesprecedence conflict — bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57.packages restore/diffvia unsanitized manager name — fix(packages): path traversal via unsanitized manager name in packages restore/diff #51.asdf ASDF_DIR vs ASDF_DATA_DIRinconsistency — fix(detector): asdf uses ASDF_DIR vs ASDF_DATA_DIR inconsistently between docs, detector, and system-node classifier #50.Current()failure leaves active Node unprotected from cleanup — bug(cli): Current() failure leaves the active Node version unprotected from cleanup deletion #58.Current()relies on unverifiedn currentsubcommand — fix(detector): n manager's Current() relies on unverified 'n current' subcommand #59.Screenshots / output
User-visible behavior change:
nodeup packages snapshot, hits Ctrl-C during the in-flightnpm ls -gstep. The cobra command returns immediately but the underlyingnpmchild process keeps running until completion (or its own timeout). The newcmd.Context()plumbing makes Ctrl-C now propagate down throughRunShelltoexec.CommandContext, killing the child.nodeup packages snapshotornodeup packages restorecancels both the cobra command AND the in-flightnpmsubprocess.Verification path: start
nodeup packages snapshot, watch the in-flightnpmPID viaps, send SIGINT to the parent nodeup process, confirm the childnpmPID disappears within ~ms.