fix(plugins,skills): make reinstall transactions atomic - #762
Conversation
Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
There was a problem hiding this comment.
Pull request overview
This PR introduces a cross-process filesystem transaction layer for plugin and skill reinstalls so replacements are staged and validated before touching the active install, and lockfile updates are serialized and published atomically to prevent lost entries under concurrency.
Changes:
- Add
internal/installtxnto stage installs, take an interprocess lock (Unix + Windows), swap directories with rollback, and atomically publish lockfiles. - Update plugin and skill install/remove flows to stage first, then commit content swap + lockfile update under the shared lock.
- Add regression tests covering rollback behavior and concurrent installs preserving all lockfile entries.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/skills/install.go | Stage SKILL.md before locking; commit dir swap + lockfile write atomically; lock Remove and write lockfile atomically. |
| internal/skills/install_test.go | Add concurrent install regression test to ensure lockfile entries aren’t lost. |
| internal/plugins/install.go | Stage full plugin tree before locking; commit dir swap + lockfile write atomically; lock Remove and write lockfile atomically. |
| internal/plugins/install_test.go | Add concurrent install regression test to ensure lockfile entries aren’t lost. |
| internal/installtxn/lock_windows.go | Implement Windows interprocess lock + atomic file replace via Win32 APIs. |
| internal/installtxn/lock_unix.go | Implement Unix interprocess lock via flock + atomic file replace via rename. |
| internal/installtxn/installtxn.go | New transaction helpers: lock, stage, commit with rollback, removal with rollback, atomic file write. |
| internal/installtxn/installtxn_test.go | Add tests for rollback and workspace cleanup behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "path/filepath" | ||
| ) | ||
|
|
||
| const lockFileName = ".zero-install.lock" |
WalkthroughAdds cross-process locking, staged directory swaps, rollback handling, and atomic file replacement for install operations. Plugins and skills now use these primitives for transactional installation, removal, lockfile publication, and concurrent updates. ChangesInstall transaction coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PluginInstall
participant installtxn
participant Filesystem
participant Lockfile
PluginInstall->>installtxn: StageDir and prepare plugin content
PluginInstall->>installtxn: Lock install root
PluginInstall->>installtxn: CommitDir staged plugin
installtxn->>Filesystem: Swap target directory
installtxn->>Lockfile: Publish updated lockfile
Lockfile-->>installtxn: Failure or success
installtxn->>Filesystem: Restore previous directory on failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/installtxn/lock_unix.go (1)
27-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSync parent directory after rename for full durability.
The Windows implementation of
replaceFileusesMOVEFILE_WRITE_THROUGHto ensure the updated directory entry is immediately persisted. To achieve equivalent durability guarantees on Unix and prevent the atomic file replacement from being lost or corrupted during a sudden power loss, you shouldfsyncthe parent directory after renaming.♻️ Proposed refactor to add directory sync
Add
"path/filepath"to the imports, then apply this diff:-func replaceFile(source string, target string) error { - return os.Rename(source, target) -} +func replaceFile(source string, target string) error { + if err := os.Rename(source, target); err != nil { + return err + } + if dir, err := os.Open(filepath.Dir(target)); err == nil { + _ = dir.Sync() + _ = dir.Close() + } + return nil +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/installtxn/lock_unix.go` around lines 27 - 29, Update replaceFile to fsync the parent directory after os.Rename succeeds: derive the target’s parent with filepath.Dir, open that directory, call Sync, and close it while propagating any errors. Preserve the rename error path and ensure the directory is always closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/installtxn/lock_unix.go`:
- Around line 27-29: Update replaceFile to fsync the parent directory after
os.Rename succeeds: derive the target’s parent with filepath.Dir, open that
directory, call Sync, and close it while propagating any errors. Preserve the
rename error path and ensure the directory is always closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae91b4f2-e90e-4178-8910-c0ae28302560
📒 Files selected for processing (8)
internal/installtxn/installtxn.gointernal/installtxn/installtxn_test.gointernal/installtxn/lock_unix.gointernal/installtxn/lock_windows.gointernal/plugins/install.gointernal/plugins/install_test.gointernal/skills/install.gointernal/skills/install_test.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the core mechanism is correct and does what it claims: I confirmed TestConcurrentInstallsPreserveEveryLockEntry fails on the merge base (only 1–3 of 12 lock entries survive across 5 runs, plus a torn plugins.lock that fails to parse) and passes here. A 30-iteration same-name concurrent install/remove stress run under -race produced no lockfile/disk inconsistency and no leaked workspaces. Everything below is an edge case or an incidental permission regression, not a failure of the design.
What's genuinely well done
Several non-obvious hazards are already handled correctly, and it's worth saying so explicitly:
- Staging inside the target install dir (
internal/installtxn/installtxn.go:31) means the commit rename can never hitEXDEV. - The extra
staged/previousnesting level is what keeps transient workspaces invisible to the loaders, which look exactly one level down fordir/<entry>/plugin.json(internal/plugins/plugins.go:253) anddir/<entry>/SKILL.md(internal/skills/skills.go:307). A flatter workspace layout would have made every in-flight install briefly visible as a broken extension. - Renaming the old target out before renaming the new one in (
installtxn.go:48then:56) is precisely what makes the swap work on Windows, where rename-over-an-existing-directory fails. - The network fetch is deliberately kept outside the lock, and re-reading the lockfile after acquiring it (
internal/plugins/install.go:157-159,internal/skills/install.go:153-155) is the detail that actually closes the lost-update window. That's the crux of the fix. cleanupWorkspace's refusal to delete a workspace still holdingprevious(installtxn.go:114-119) is the right call, and the comment explains why.
Findings
[Minor] WriteFileAtomically resets the lockfile to 0644, discarding umask and any user-hardened mode
internal/installtxn/installtxn.go:131
temp.Chmod(perm) applies the literal perm argument (0o644 from internal/plugins/install.go:271 and internal/skills/install.go:332) before the rename, so the published lockfile always ends up world-readable regardless of process umask or the mode the existing lockfile had. os.WriteFile on the base only applied perm at creation, so this is a behavior change.
Confirmed empirically against the base: (1) after chmod 600 ~/.zero/skills/skills.lock, the base preserves 0600 across a reinstall, this PR republishes it as 0644 (-rw------- -> -rw-r--r--); (2) under umask 077 the base creates it 0600, this PR creates it 0644.
This matters because the lockfile stores the source string verbatim (internal/skills/install.go:166, internal/plugins/install.go:169) and remote sources are not rewritten — canonicalSource returns non-local sources unchanged (internal/skills/install.go:428-440). A private repo installed via https://user:token@host/repo puts that token into a file whose permissions were just silently widened on a multi-user host.
Fix: stat the destination first — if it exists, reuse existing.Mode().Perm() so an operator-hardened mode survives; if it doesn't, drop the explicit Chmod and let CreateTemp's 0600 stand (or apply the umask explicitly) rather than forcing the literal perm.
[Minor] CommitDir uses os.Stat, so installing over a dangling symlink now fails instead of replacing it
internal/installtxn/installtxn.go:47
os.Stat(target) follows symlinks, so a dangling symlink at the install target reports ErrNotExist, sets hadPrevious=false, and leaves the symlink in place. The subsequent os.Rename(staged, target) at :56 then fails, because renaming a directory onto an existing non-directory returns ENOTDIR.
Confirmed empirically: with ~/.zero/skills/probe a symlink pointing at a nonexistent path, skills.Install succeeds on the merge base (the old RemoveAll unlinked the dangling symlink) and records the lock entry. On this PR the same call returns publish staged install: rename .../staged .../probe: not a directory, the lockfile stays empty, and the dangling symlink is left behind — the skill can never be installed until the user manually deletes the link. Same path applies to internal/plugins/install.go:170.
Fix: use os.Lstat(target) for the hadPrevious probe, so any existing entry — symlink, dangling or not — is renamed into the workspace before the staged directory moves into place. Worth also considering the matching os.Stat presence checks in Remove (internal/plugins/install.go:215, internal/skills/install.go:210).
[Nit] Remove creates the install dir and a lock file as a side effect of a lookup that then fails
internal/installtxn/installtxn.go:18
Lock unconditionally MkdirAlls the install root and opens .zero-install.lock inside it, and Remove takes the lock (internal/plugins/install.go:203, internal/skills/install.go:198) before checking whether the item exists. On a fresh machine with no ~/.zero/skills, zero skill remove foo now creates ~/.zero/skills/ and ~/.zero/skills/.zero-install.lock before returning skill "foo" is not installed. Verified: after Remove(dir, "nope") against a nonexistent dir, the directory exists and contains the lock file. Nothing was created on the base.
Fix: do the cheap ReadLock/os.Stat existence check before taking the lock and return the not-installed error early, or give Lock a non-creating variant that returns a no-op unlock when the install root doesn't exist.
[Nit] Lockfile write failures during Remove are reported as directory-removal failures
internal/plugins/install.go:228, internal/skills/install.go:224
RemoveDir returns the publish callback's error verbatim (internal/installtxn/installtxn.go:92), but both call sites wrap everything it returns in remove plugin dir: / remove skill dir:. If the directory rename succeeds and writeLock then fails, the rollback restores the directory correctly — but the user sees remove plugin dir: write plugins.lock: ..., pointing at a directory removal that in fact succeeded and was rolled back. It sends anyone debugging toward directory permissions rather than the lockfile.
Fix: have RemoveDir distinguish its own failures from the callback's (typed or sentinel-wrapped publish error), or drop the blanket wrap at the call sites and let the already-descriptive inner errors surface.
Checked and cleared
Three things looked like defects on first read and are not — flagging them so they don't get re-raised:
- Stranded
previousworkspaces. Looks like silent data hiding, but it's reported and recoverable: theos.Renamefailure is a*os.LinkErrorwrapped with%watinstalltxn.go:104and surfaced to stderr (internal/cli/distribution.go:83and:204), printing the full path to the strandedpreviousdirectory — I forced the state and observed it. It's a plain directory recoverable withmv. The behavior is deliberate, documented atinstalltxn.go:110-113, and pinned byinstalltxn_test.go:66. More importantly it's strictly better than the base, which didos.RemoveAll(target)beforecopyTree, i.e. destroyed the previous install unrecoverably. Note the cited Windows mechanism doesn't apply anyway: an open file undertargetmakesos.RemoveAllat:99fail and return at:100, so:103is never reached on that path. - Missing fsync on the staged tree / parent dir. Not a regression — the base had zero syncs anywhere in the install path (
grep '\.Sync()'on both trees);installtxn.go:139is the only one and this PR adds it. Also,temp.Sync()runs beforereplaceFileat:146, and on ext4 that fsync forces the running journal transaction which already contains theos.Rename(staged, target)issued at:56— so the ordering concern is backwards. And the package docs (installtxn.go:1-3) promise cross-process mutual exclusion, not power-loss durability. Staged file data is still unsynced (internal/plugins/install.go:397-411), but that's a pre-existing codebase-wide hardening opportunity, not something introduced here. - Concurrency tests asserting only
len(entries). Accurate description, but not a real gap. I mutation-tested it: no-op'ing theos.Renameatinstalltxn.go:57does let the concurrency test pass alone, but fails 8 other tests in the samego test ./internal/plugins/ ./internal/skills/invocation (TestInstallCopiesLocalPluginAndRecordsHash,TestInstallCopiesEntireTree,TestInstallFromRealLocalGitRepo, +5 in skills). Deleting the retained-previousguard atinstalltxn.go:110-113immediately fails the PR's ownTestCleanupWorkspacePreservesRetainedPreviousInstall. Entry count is the correct and only assertion that discriminates fixed-from-broken for the bug this PR targets; on-disk content is covered elsewhere.
Build & tests
gofmt -l . clean repo-wide, go build ./... clean, go vet clean on all three touched packages. internal/installtxn, internal/plugins, internal/skills all pass under -race -count=5 with no flakiness and no race reports.
No test failure is attributable to this PR. There are failures in internal/tools, internal/tui, and internal/cli (sandbox-escape and doctor tests), but I re-ran each of them in a worktree at the true merge base and the failure sets are byte-for-byte identical on both sides. Root cause is environmental — the sandbox backend reports {"name":"unavailable","available":false,"platform":"windows","fallback":true} while running on darwin, so workspace-escape writes aren't blocked and doctor exits 3. Pre-existing, not yours.
Downstream check: internal/cli, internal/tools, internal/tui, internal/zerocommands all import the changed packages; full suites run on both sides with identical results, so the plugins/skills API changes broke no consumer.
Cross-platform: lock_windows.go is behind //go:build windows and never compiled by the darwin or linux toolchain, so I verified it explicitly — GOOS=windows GOARCH=amd64 go build and go vet are both clean, as is GOOS=linux. Flagging that the Windows path (LockFileEx/UnlockFileEx, MoveFileEx with MOVEFILE_REPLACE_EXISTING|MOVEFILE_WRITE_THROUGH) has no test coverage — installtxn's tests only exercise the unix implementation, so it's build-verified only. Worth a manual smoke test on Windows before this ships, given the whole point is a cross-process lock.
One housekeeping note: this branch is based on ce4a996, which is behind current main — a rebase before merge would avoid a surprising diff.
Merge is kevin's call per the program gate.
Summary
Fixes #754
Verification
make fmt-checkgo vet ./...go test ./...go test -race ./internal/installtxn ./internal/plugins ./internal/skills-count=10)go run ./cmd/zero-release buildgo run ./cmd/zero-release smokego run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...git diff HEAD --checkThe pinned advisory golangci-lint command reports the repository's existing 36-item backlog in unrelated files; it reports no finding in the files changed here.
Summary by CodeRabbit
Bug Fixes
Tests