fix(cli): wire platform.AcquireLock into upgrade + config set/init#79
Merged
Merged
Conversation
`platform.AcquireLock`/`Release` (flock on POSIX, LockFileEx on Windows, stale-PID detection) had been implemented but had zero call sites anywhere in `internal/cli` or `internal/config`. The README and package doc comments advertised "concurrent runs are blocked via a lock file" — that claim was false. Two parallel `nodeup upgrade` invocations could snapshot/install/ migrate against the same data dir with no guard between them. Two parallel `nodeup config set` invocations raced on read-modify-write of the YAML file (Save itself is atomic via temp-file+rename, but the second writer's atomic rename silently clobbers the first writer's in-memory changes — a lost update). `runUpgrade`, `config set`, and `config init` now defer `platform.AcquireLock()` around their critical sections (after the dry-run early-return, because dry-run is read-only), and surface `ErrAlreadyLocked` with an actionable hint instead of silently racing. A new package-level test (`lock_acquire_test.go`) pins down the acquire/release/`ErrAlreadyLocked` contract the wire-up depends on and asserts that `LockPath()` always lives under `DataDir()` — so a future refactor that splits them cannot quietly break the filesystem-permission enforcement story. Fixes an incidental doc bug: README claimed the lock lives at `~/.nodeup/nodeup.lock`, but the real path is `<DataDir>/nodeup.lock` (Linux XDG, macOS `~/Library/Application Support/...`, Windows `%APPDATA%\...`). The README now spells out the platform resolution explicitly. Refs #44 Co-Authored-By: puku-ai-2.8 <noreply@puku.sh>
|
✔️ 9d75644 - Conventional commits check succeeded. |
This was referenced Jul 3, 2026
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
1 participant
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
platform.AcquireLock/Release(flock on POSIX, LockFileEx on Windows, stale-PID detection) existed but had zero call sites. The README and package doc comments advertised "concurrent runs are blocked via a lock file" — that claim was false.This PR wires the lock into
runUpgrade,config set, andconfig init(each wraps the critical section indefer platform.AcquireLock()), surfacesErrAlreadyLockedwith an actionable hint instead of silently racing, and corrects an incidental doc bug: the README claimed the lock lives at~/.nodeup/nodeup.lock, but the real path is<DataDir>/nodeup.lock(Linux XDG, macOS~/Library/Application Support/..., Windows%APPDATA%\...).Closes #44.
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 will parse the config fine)docs/, inline godoc)Scope notes
runUpgradedoes NOT acquire the lock. Dry-run is read-only (no shell-out, no disk mutation); a concurrentnodeup checkshould be able to run while another invocation is in dry-run without contention. The lock is acquired immediately after dry-run returns, before any snapshot/manifest/install mutation.~/.nodeup/config.yamlreferences in README anddocs/configuration.md/docs/managers.md/ CHANGELOG. Those are different from the lock file's path — the config file genuinely does live at~/.nodeup/config.yaml(perconfig.DefaultPath()), while the lock lives at<DataDir>/nodeup.lock. The two having different parents is intentional: the config follows the user's home directory across platforms, while the lock follows the OS-aware XDG/AppData layout.runUpgradereturnsErrAlreadyLocked. The contract is already covered by the newTestAcquireLock_TwiceReturnsAlreadyLocked; full CLI integration is overkill for this fix size.internal/node: HTTP timeout/retry/context on nodejs.org manifest fetch — bug(node): no HTTP timeout/retry/context on nodejs.org manifest fetch; cache write not atomic #48.ltsJSON union bug (internal/node/dist.go) — already fixed in PR feat(cli): implement end-to-end upgrade command #20.Screenshots / output
User-visible behaviour change:
nodeup upgrade(ornodeup config set) silently raced against the first; users had no warning that two invocations were stomping each other.nodeup upgradereturns:nodeup config set/config init:Verification path: in two terminals, run
nodeup upgradein one andnodeup config set manager fnmin the other — the second one now fast-fails with the message above instead of corrupting state.