fix: atomic plugin dir replacement to prevent stale executables on upgrade#645
Merged
Conversation
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Add preparePluginStagingDir/commitPluginStagingDir helpers that extract/copy into a sibling .installing directory and atomically rename it into place. This prevents stale executables after upgrades because: 1. The old directory is removed before the new one is moved in 2. ensurePluginBinary can no longer find the old binary in the staging dir Also add verifyInstalledVersion to confirm plugin.json version matches the manifest after a registry install. Tests added: - TestInstallPluginFromManifest_UpgradeReplacesStaleBinary - TestInstallFromURL_UpgradeReplacesStaleBinary - TestInstallFromLocal_UpgradeReplacesStaleBinary - TestInstallPluginFromManifest_StagingCleanedUpOnFailure Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/37d41012-d306-495a-af1f-9b948158f6e6 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix wfctl plugin install to update stale executable
fix: atomic plugin dir replacement to prevent stale executables on upgrade
May 13, 2026
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates wfctl plugin install to stage plugin installs before replacing the final plugin directory, aiming to prevent stale binaries during upgrades.
Changes:
- Adds staging directory preparation, commit, and version verification helpers.
- Updates registry, URL, and local install paths to install into staging first.
- Adds regression tests for upgrade replacement and staging cleanup behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cmd/wfctl/plugin_install.go |
Implements staged plugin install flow across install sources. |
cmd/wfctl/plugin_install_test.go |
Adds upgrade and failure-path tests for staged installs. |
Comments suppressed due to low confidence (2)
cmd/wfctl/plugin_install.go:321
- When the verify hook is enabled, this tarball is written under the staging directory but the deferred remove still references the staging path. After
commitPluginStagingDirrenames staging to the final directory, the deferred removal becomes a no-op and the.tar.gzis left in the installed plugin directory.
tarballPath := filepath.Join(stagingDir, pluginName+".tar.gz")
if writeErr := os.WriteFile(tarballPath, data, 0600); writeErr != nil {
return fmt.Errorf("write tarball for verify hook: %w", writeErr)
}
defer os.Remove(tarballPath) //nolint:errcheck
cmd/wfctl/plugin_install_test.go:668
installFromLocalalso updates.wfctl-lock.yamlin the process current directory, but this test only isolates the plugin install directory. It should run from a temporary cwd or otherwise isolate the lockfile so the test does not mutate the package checkout or depend on an existing lockfile.
if err := installFromLocal(makeLocalSrc("1.0.0", oldBinary), pluginDir); err != nil {
t.Fatalf("first install: %v", err)
}
binaryPath := filepath.Join(pluginDir, pluginName, pluginName)
gotOld, err := os.ReadFile(binaryPath)
if err != nil {
t.Fatalf("read v1 binary: %v", err)
}
if !bytes.Equal(gotOld, oldBinary) {
t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary)
}
// Upgrade.
if err := installFromLocal(makeLocalSrc("2.0.0", newBinary), pluginDir); err != nil {
Comment on lines
+1426
to
+1436
| // commitPluginStagingDir atomically replaces destDir with stagingDir. It | ||
| // removes the previous destDir content and renames the staging directory into | ||
| // its place. On success stagingDir no longer exists on disk; the deferred | ||
| // cleanup returned by preparePluginStagingDir becomes a harmless no-op. | ||
| func commitPluginStagingDir(stagingDir, destDir string) error { | ||
| if err := os.RemoveAll(destDir); err != nil { | ||
| return fmt.Errorf("remove existing plugin dir %s: %w", destDir, err) | ||
| } | ||
| if err := os.Rename(stagingDir, destDir); err != nil { | ||
| return fmt.Errorf("install plugin dir %s: %w", destDir, err) | ||
| } |
Comment on lines
+341
to
344
| pluginJSONPath := filepath.Join(stagingDir, "plugin.json") | ||
| if writeErr := writeInstalledManifest(pluginJSONPath, manifest); writeErr != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: could not write plugin.json: %v\n", writeErr) | ||
| } |
|
|
||
| // Attempt upgrade with a failing URL. | ||
| badManifest := makeTestManifest(pluginName, srv.URL+"/nonexistent.tar.gz", strings.Repeat("0", 64)) | ||
| _ = installPluginFromManifest(pluginDir, pluginName, badManifest, nil, false) // expected to fail |
Comment on lines
+603
to
+617
| if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(oldTar), false); err != nil { | ||
| t.Fatalf("first install: %v", err) | ||
| } | ||
| binaryPath := filepath.Join(pluginDir, pluginName, pluginName) | ||
| gotOld, err := os.ReadFile(binaryPath) | ||
| if err != nil { | ||
| t.Fatalf("read v1 binary: %v", err) | ||
| } | ||
| if !bytes.Equal(gotOld, oldBinary) { | ||
| t.Fatalf("after first install, binary = %q, want %q", gotOld, oldBinary) | ||
| } | ||
|
|
||
| // Upgrade. | ||
| serveData = newTar | ||
| if err := installFromURL(srv.URL+"/p.tar.gz", pluginDir, sha256sum(newTar), false); err != nil { |
Contributor
|
@copilot apply changes based on the comments in this thread And improve test coverage #645 (comment) |
…lity - commitPluginStagingDir: rename existing dir to .uninstalling before moving staging in place; restore on failure, remove on success. This prevents the old install from being destroyed if the final rename fails. - installPluginFromManifest: make writeInstalledManifest failure a hard error instead of a warning so registry-only metadata (capabilities, CLI commands) cannot be silently dropped and an archive-supplied plugin.json cannot bypass verifyInstalledVersion. - TestInstallPluginFromManifest_StagingCleanedUpOnFailure: assert the failing install actually returns an error (was discarding it with _). - TestInstallFromURL_UpgradeReplacesStaleBinary, TestInstallFromLocal_UpgradeReplacesStaleBinary, TestInstallFromURL_WithExpectedSHA256_Correct, TestInstallFromURL_SkipChecksum_NonGitHub: add t.Chdir(t.TempDir()) so updateLockfileWithChecksum writes .wfctl-lock.yaml to a temp dir rather than the package checkout, preventing lockfile pollution and test interdependence. Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/6bc57919-fc52-426b-94ba-1545ffcd1537 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all four review fixes in commit
|
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.
wfctl plugin installreported success after an upgrade but left the old binary in place, because extraction wrote into an existing directory andensurePluginBinaryshort-circuited whendestDir/<name>already existed — meaning a GoReleaser-named tarball entry was extracted but never renamed over the stale binary.Root cause
All three install paths (
installPluginFromManifest,installFromURL,installFromLocal) calledos.MkdirAllthen extracted on top of the existing directory, so the stale binary survived every upgrade.Changes
preparePluginStagingDir(destDir)— creates a freshdestDir + ".installing"sibling (same filesystem, atomic-rename safe) and returns a cleanup func invoked on failure.commitPluginStagingDir(stagingDir, destDir)— removes the olddestDirthen renames staging into its place; old install is untouched if commit fails.verifyInstalledVersion(dir, expectedVersion)— asserts the stagedplugin.jsonversion matches the manifest before committing; catches silent write failures.ensurePluginBinaryalways runs against an empty staging dir so the stale-binary early-return can never fire.Tests
TestInstallPluginFromManifest_UpgradeReplacesStaleBinaryTestInstallFromURL_UpgradeReplacesStaleBinary--urlpathTestInstallFromLocal_UpgradeReplacesStaleBinary--localpathTestInstallPluginFromManifest_StagingCleanedUpOnFailure