From dc8403e2e894fce427d53351a6f4ef43713fb61c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 22 Aug 2026 15:35:58 +0100 Subject: [PATCH] fix(plugins): prune superseded plugin versions after a confirmed start (#2921) Bounds plugin disk use: installing never removed the version it replaced, so ~/.kandev/plugins// accumulated a full copy per install forever (4.2GB on one machine, 3.74GB of it superseded versions). A plugin now keeps its active version plus one rollback target, pruned only once the new version is confirmed running. Co-authored-by: Kandev Agent Co-Authored-By: Claude Opus 5 (1M context) --- apps/backend/internal/plugins/service.go | 12 + .../internal/plugins/service_install.go | 70 ++- .../internal/plugins/service_lifecycle.go | 9 + .../backend/internal/plugins/service_prune.go | 185 +++++++ .../internal/plugins/service_prune_test.go | 517 ++++++++++++++++++ .../2026-08-22-plugin-version-retention.md | 82 +++ docs/decisions/INDEX.md | 1 + docs/public/plugins-authoring.md | 7 +- docs/public/plugins.md | 27 + 9 files changed, 908 insertions(+), 2 deletions(-) create mode 100644 apps/backend/internal/plugins/service_prune.go create mode 100644 apps/backend/internal/plugins/service_prune_test.go create mode 100644 docs/decisions/2026-08-22-plugin-version-retention.md diff --git a/apps/backend/internal/plugins/service.go b/apps/backend/internal/plugins/service.go index d6e87a2466..dbd6ee0e34 100644 --- a/apps/backend/internal/plugins/service.go +++ b/apps/backend/internal/plugins/service.go @@ -75,6 +75,18 @@ type Service struct { // insertion one atomic catalog mutation across different plugin IDs. agentToolInstallMu sync.Mutex + // extractingMu guards extractingPaths and, crucially, is held across the + // pkgtar extraction that registers into it: a version directory must never + // be observable on disk before it is marked in flight, or a concurrent + // prune could delete it in that gap. See extractPackage. + extractingMu sync.Mutex + // extractingPaths counts, per version directory, the installs that have + // extracted it but have not yet finished. Install extracts before it can + // know the plugin id (and therefore before it can take that id's lifecycle + // lock), so this is what tells a prune running under the lock that a + // directory belongs to an install still waiting for it. + extractingPaths map[string]int + pluginsDir string store store.Store registry *Registry diff --git a/apps/backend/internal/plugins/service_install.go b/apps/backend/internal/plugins/service_install.go index 92c75d1cd5..850b967fce 100644 --- a/apps/backend/internal/plugins/service_install.go +++ b/apps/backend/internal/plugins/service_install.go @@ -45,10 +45,11 @@ const downloadTimeout = 60 * time.Second // survive) and restarts the previous version's process, so a failed upgrade // attempt never destroys a previously working install. func (s *Service) Install(ctx context.Context, r io.Reader) (*store.Record, error) { - result, err := pkgtar.Install(r, s.pluginsDir) + result, err := s.extractPackage(r) if err != nil { return nil, err } + defer s.releaseExtraction(result.InstallPath) if err := s.checkMinKandevVersion(result.Manifest.MinKandevVersion); err != nil { _ = os.RemoveAll(result.InstallPath) return nil, err @@ -123,6 +124,14 @@ func (s *Service) Install(ctx context.Context, r io.Reader) (*store.Record, erro catalogLocked = false activateErr := s.activate(rec) + if activateErr == nil { + // Only now can the new version be confirmed running, which is the + // point at which the versions it superseded stop being rollback + // targets. Pruning any earlier would delete what a failed upgrade + // falls back to; pruneSupersededVersions re-checks that the process is + // actually up before deleting anything. + s.pruneSupersededVersions(rec.ID, rec.Version, previousVersion(oldRec, hadOldRec)) + } s.notifyDeliverer() s.notifyAgentToolCatalogChanged() @@ -133,6 +142,65 @@ func (s *Service) Install(ctx context.Context, r io.Reader) (*store.Record, erro return installed, activateErr } +// extractPackage runs pkgtar.Install and registers the extracted version +// directory as an in-flight install, both under extractingMu so the directory +// is never visible on disk without being marked. Install cannot take the +// per-plugin lifecycle lock any earlier than this — the id is only known once +// the package's manifest has been parsed — so without the mark, two +// overlapping installs of the same id would both extract, and whichever +// acquired the lock first would prune the other's fresh directory and leave it +// activating an InstallPath that no longer exists. +// +// The mutex serializes extraction across every plugin, not just one id. That +// is deliberate and cheap: an install is a rare operator- or poller-driven +// action, and the alternative (marking after pkgtar returns) leaves exactly +// the gap this exists to close. +func (s *Service) extractPackage(r io.Reader) (*pkgtar.InstallResult, error) { + s.extractingMu.Lock() + defer s.extractingMu.Unlock() + + result, err := pkgtar.Install(r, s.pluginsDir) + if err != nil { + return nil, err + } + if s.extractingPaths == nil { + s.extractingPaths = make(map[string]int) + } + s.extractingPaths[result.InstallPath]++ + return result, nil +} + +// releaseExtraction drops one in-flight mark for path, deferred by Install so +// it runs however that install ends (rejected package, failed persist, or a +// completed activation). +func (s *Service) releaseExtraction(path string) { + s.extractingMu.Lock() + defer s.extractingMu.Unlock() + if s.extractingPaths[path] > 1 { + s.extractingPaths[path]-- + return + } + delete(s.extractingPaths, path) +} + +// extractionInFlight reports whether some install has extracted path and not +// yet finished with it. +func (s *Service) extractionInFlight(path string) bool { + s.extractingMu.Lock() + defer s.extractingMu.Unlock() + return s.extractingPaths[path] > 0 +} + +// previousVersion returns the version an in-place upgrade replaced, or "" when +// this install had no existing record to replace (a first install, or one over +// a plugin whose record was lost) and therefore knows of no rollback target. +func previousVersion(oldRec *store.Record, hadOldRec bool) string { + if !hadOldRec || oldRec == nil { + return "" + } + return oldRec.Version +} + // DevKandevVersion is the version string an un-stamped local build carries // (cmd/kandev's `Version` default, mirrored by internal/system/updates' // devVersion). It sorts meaninglessly against real semver, and a developer diff --git a/apps/backend/internal/plugins/service_lifecycle.go b/apps/backend/internal/plugins/service_lifecycle.go index 720a709d1c..49e3474d9c 100644 --- a/apps/backend/internal/plugins/service_lifecycle.go +++ b/apps/backend/internal/plugins/service_lifecycle.go @@ -305,7 +305,16 @@ func (s *Service) StartActivePlugins(ctx context.Context) { s.notifyDeliverer() s.notifyAgentToolCatalogChanged() } + continue } + // Boot is the only trigger that reaches an instance which already + // accumulated superseded versions but installs nothing new (a plugin + // already at its latest version, or one whose auto-update is off), so + // it prunes too — under the same confirmed-start precondition as + // Install, which is what keeps disabled plugins, sideloads registered + // disabled, and plugins that fail to spawn untouched. Unlike Install, + // this loop holds no lifecycle lock, so the prune takes one itself. + s.pruneUnderLifecycleLock(rec.ID, rec.Version, "") } } diff --git a/apps/backend/internal/plugins/service_prune.go b/apps/backend/internal/plugins/service_prune.go new file mode 100644 index 0000000000..f8e1ac3e5c --- /dev/null +++ b/apps/backend/internal/plugins/service_prune.go @@ -0,0 +1,185 @@ +package plugins + +import ( + "os" + "path/filepath" + "strings" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/plugins/manifest" +) + +// pluginDataDirName is the plugin-owned writable directory the runtime +// manager creates beside a plugin's version directories +// (//data — see internal/plugins/runtime's pluginCommand). It +// is durable state that survives every upgrade and is removed only by +// Uninstall, so the prune below excludes it by name as well as by the +// manifest check every retained candidate has to pass. +const pluginDataDirName = "data" + +// pruneSupersededVersions removes every superseded version directory under +// //, keeping activeVersion plus one rollback target. +// Installing never used to remove the version it replaced, so a plugin's +// directory grew by a full copy (~20MB, ~95MB for a package shipping all five +// platform binaries) on every install — and the auto-update poller installs +// with no user action at all, which is how a single plugin reached 46 +// superseded copies and one machine reached 3.74GB of them. +// +// Retention is deliberately fixed at "current + 1 previous" rather than +// configurable: the second directory exists so a failed upgrade still has a +// working process to fall back on (see rollbackFailedInstall), which is a +// safety invariant rather than an operator preference, and any larger number +// re-creates the unbounded growth this exists to stop. +// +// Nothing is deleted unless id's process is confirmed running, which is the +// precondition that makes this safe: pruning before the new version starts +// would delete the very directory a failed upgrade needs. The check lives here +// rather than at each call site so a future caller cannot forget it — note +// that activate returns nil without starting anything when no runtime is +// wired, so "activate succeeded" alone is not the same guarantee. +// +// Every failure is logged and swallowed: a plugin that installed and started +// correctly must never be reported as failed because cleanup could not delete +// a directory. Callers hold id's lifecycle lock, so no concurrent +// install/enable/uninstall for the same id can race the scan. +// +// rollbackVersion names the version the caller knows was previously in use; +// when it is empty (or no longer on disk) the semver-greatest remaining +// version is retained instead. +func (s *Service) pruneSupersededVersions(id, activeVersion, rollbackVersion string) { + if s.pluginsDir == "" || id == "" || activeVersion == "" { + return + } + if s.runtime == nil || !s.runtime.Running(id) { + return + } + + // Every read and delete below goes through one os.Root handle opened on + // the plugin directory, so a path validated by isInstalledVersionDir is + // removed within the same confined tree: a symlink or rename planted + // mid-prune cannot redirect the removal outside //. + root, err := os.OpenRoot(filepath.Join(s.pluginsDir, id)) + if err != nil { + if !os.IsNotExist(err) { + s.log.Warn("plugins: could not open plugin directory for pruning", + zap.String("plugin_id", id), zap.Error(err)) + } + return + } + defer func() { _ = root.Close() }() + + versions, err := installedVersionDirs(root, id) + if err != nil { + s.log.Warn("plugins: could not scan installed versions for pruning", + zap.String("plugin_id", id), zap.Error(err)) + return + } + + keep := retainedVersions(versions, activeVersion, rollbackVersion) + for _, version := range versions { + if keep[version] { + continue + } + // A concurrent Install extracts before it can take id's lifecycle + // lock, so a version directory that exists here may belong to an + // install still waiting on that lock. Deleting it would strand that + // caller with an InstallPath pointing at nothing. + if s.extractionInFlight(filepath.Join(s.pluginsDir, id, version)) { + continue + } + if err := root.RemoveAll(version); err != nil { + s.log.Warn("plugins: could not remove superseded plugin version", + zap.String("plugin_id", id), zap.String("version", version), zap.Error(err)) + continue + } + s.log.Info("plugins: removed superseded plugin version", + zap.String("plugin_id", id), zap.String("version", version)) + } +} + +// pruneUnderLifecycleLock is pruneSupersededVersions for a caller that does +// not already hold id's lifecycle lock (the boot activation loop). Install +// holds it across activate and the prune that follows, and the prune relies on +// that exclusion, so the boot path takes the same lock rather than depending +// on nothing else being able to run yet. +func (s *Service) pruneUnderLifecycleLock(id, activeVersion, rollbackVersion string) { + lock := s.lifecycleLocks.lockFor(id) + lock.Lock() + defer lock.Unlock() + s.pruneSupersededVersions(id, activeVersion, rollbackVersion) +} + +// retainedVersions returns the set of version directory names to keep: +// active, plus one rollback target — rollback when the caller named a version +// that is still on disk, otherwise the semver-greatest of the rest. +func retainedVersions(versions []string, active, rollback string) map[string]bool { + keep := map[string]bool{active: true} + fallback := "" + for _, version := range versions { + if version == active { + continue + } + if version == rollback { + keep[version] = true + return keep + } + if fallback == "" || manifest.CompareVersions(version, fallback) > 0 { + fallback = version + } + } + if fallback != "" { + keep[fallback] = true + } + return keep +} + +// installedVersionDirs lists the entries directly under root (a handle on +// //) that are installed versions of id. The bar is +// deliberately high, because everything it returns is a deletion candidate: +// the entry must be a real directory (a symlink reports as a link here and is +// skipped), must not be the plugin's data directory or any dot-prefixed name +// (which covers pkgtar's in-flight ".tmp-*" staging directories and any other +// hidden entry), and must hold a manifest.yaml naming exactly this id and this +// directory as its version — which is what pkgtar.Install writes and what +// nothing else on that path produces. The plugin's store record (".yml") +// and config (".config.yml") live one level up, beside the plugin +// directory rather than inside it, so the root handle cannot reach them. +func installedVersionDirs(root *os.Root, id string) ([]string, error) { + dir, err := root.Open(".") + if err != nil { + return nil, err + } + defer func() { _ = dir.Close() }() + + entries, err := dir.ReadDir(-1) + if err != nil { + return nil, err + } + var versions []string + for _, entry := range entries { + name := entry.Name() + if !entry.IsDir() || name == pluginDataDirName || strings.HasPrefix(name, ".") { + continue + } + if !isInstalledVersionDir(root, id, name) { + continue + } + versions = append(versions, name) + } + return versions, nil +} + +// isInstalledVersionDir reports whether /manifest.yaml under root +// parses and declares exactly this id and version. +func isInstalledVersionDir(root *os.Root, id, version string) bool { + data, err := root.ReadFile(filepath.Join(version, manifestFileName)) + if err != nil { + return false + } + m, err := manifest.Parse(data) + if err != nil { + return false + } + return m.ID == id && m.Version == version +} diff --git a/apps/backend/internal/plugins/service_prune_test.go b/apps/backend/internal/plugins/service_prune_test.go new file mode 100644 index 0000000000..d03dd8a4fd --- /dev/null +++ b/apps/backend/internal/plugins/service_prune_test.go @@ -0,0 +1,517 @@ +package plugins + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + goruntime "runtime" + "sort" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/plugins/store" +) + +const prunePluginID = "kandev-plugin-slack" + +// seedVersionDir writes a version directory shaped exactly like one +// pkgtar.Install extracts (a manifest.yaml whose id/version match the +// directory name, plus a payload file), so the prune scan sees a real +// superseded install rather than an empty directory. +func seedVersionDir(t *testing.T, pluginsDir, id, version string) string { + t.Helper() + dir := filepath.Join(pluginsDir, id, version) + if err := os.MkdirAll(filepath.Join(dir, "server"), 0o755); err != nil { + t.Fatalf("MkdirAll(%q): %v", dir, err) + } + platformKey := goruntime.GOOS + "-" + goruntime.GOARCH + manifestYAML := fmt.Sprintf(` +id: %s +api_version: 1 +version: %s +display_name: Test Plugin +runtime: + type: binary + executables: + %s: server/plugin +`, id, version, platformKey) + if err := os.WriteFile(filepath.Join(dir, manifestFileName), []byte(manifestYAML), 0o644); err != nil { + t.Fatalf("WriteFile(manifest.yaml): %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "server", "plugin"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatalf("WriteFile(server/plugin): %v", err) + } + return dir +} + +// versionDirsOnDisk lists every directory entry under //, +// sorted, so a test can assert the exact surviving set (including entries a +// prune must never touch, like "data"). +func versionDirsOnDisk(t *testing.T, pluginsDir, id string) []string { + t.Helper() + entries, err := os.ReadDir(filepath.Join(pluginsDir, id)) + if err != nil { + t.Fatalf("ReadDir(%q): %v", filepath.Join(pluginsDir, id), err) + } + var names []string + for _, entry := range entries { + if entry.IsDir() { + names = append(names, entry.Name()) + } + } + sort.Strings(names) + return names +} + +func assertVersionDirs(t *testing.T, pluginsDir, id string, want ...string) { + t.Helper() + got := versionDirsOnDisk(t, pluginsDir, id) + sort.Strings(want) + if len(got) != len(want) { + t.Fatalf("version dirs on disk = %v, want %v", got, want) + } + for i := range got { + if got[i] != want[i] { + t.Fatalf("version dirs on disk = %v, want %v", got, want) + } + } +} + +// TestServiceInstallPrunesSupersededVersions pins the retention rule: after a +// successful install whose new version started cleanly, only the newly active +// version and the immediately-previous one (the rollback target) survive. +// Every install used to leave its predecessor behind forever — 46 copies of a +// single plugin, 3.74GB of superseded versions on a real machine. +func TestServiceInstallPrunesSupersededVersions(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err != nil { + t.Fatalf("Install(1.1.0): %v", err) + } + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.2.0", false)); err != nil { + t.Fatalf("Install(1.2.0): %v", err) + } + + assertVersionDirs(t, dir, prunePluginID, "1.1.0", "1.2.0") +} + +// TestServiceInstallPrunesAccumulatedVersions covers the already-broken +// instance: many superseded versions predate the fix, and one successful +// install must collapse them to the retained pair. +func TestServiceInstallPrunesAccumulatedVersions(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0, active + for _, v := range []string{"0.1.0", "0.2.0", "0.9.9"} { + seedVersionDir(t, dir, prunePluginID, v) + } + + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err != nil { + t.Fatalf("Install(1.1.0): %v", err) + } + + assertVersionDirs(t, dir, prunePluginID, "1.0.0", "1.1.0") +} + +// TestServiceInstallPruneKeepsRollbackTarget pins that the first upgrade +// prunes nothing: the previous version is the rollback target a failed +// restart falls back to. +func TestServiceInstallPruneKeepsRollbackTarget(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err != nil { + t.Fatalf("Install(1.1.0): %v", err) + } + + assertVersionDirs(t, dir, prunePluginID, "1.0.0", "1.1.0") +} + +// TestServiceInstallPruneKeepsDataDirectory pins the hard constraint: the +// plugin's writable data directory is durable state that survives every +// upgrade and is removed only on uninstall. +func TestServiceInstallPruneKeepsDataDirectory(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + + marker := filepath.Join(dir, prunePluginID, "data", "marker.txt") + if err := os.MkdirAll(filepath.Dir(marker), 0o755); err != nil { + t.Fatalf("MkdirAll(data): %v", err) + } + if err := os.WriteFile(marker, []byte("keep me"), 0o644); err != nil { + t.Fatalf("WriteFile(marker): %v", err) + } + + for _, v := range []string{"1.1.0", "1.2.0"} { + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, v, false)); err != nil { + t.Fatalf("Install(%s): %v", v, err) + } + } + + if _, err := os.Stat(marker); err != nil { + t.Fatalf("plugin data dir marker was removed by the prune: %v", err) + } + assertVersionDirs(t, dir, prunePluginID, "1.1.0", "1.2.0", "data") +} + +// TestServiceInstallPruneNeverRemovesDataDirWithManifest pins the data +// directory's exclusion by name, independently of the manifest check that +// also happens to cover it. The directory is plugin-writable, so its contents +// are not something the host gets to reason about: a plugin that writes its +// own manifest.yaml there must not be able to talk the prune into deleting +// its durable state. +func TestServiceInstallPruneNeverRemovesDataDirWithManifest(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + seedVersionDir(t, dir, prunePluginID, pluginDataDirName) + marker := filepath.Join(dir, prunePluginID, pluginDataDirName, "marker.txt") + if err := os.WriteFile(marker, []byte("keep me"), 0o644); err != nil { + t.Fatalf("WriteFile(marker): %v", err) + } + + for _, v := range []string{"1.1.0", "1.2.0"} { + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, v, false)); err != nil { + t.Fatalf("Install(%s): %v", v, err) + } + } + + if _, err := os.Stat(marker); err != nil { + t.Fatalf("plugin data dir was removed by the prune: %v", err) + } +} + +// TestServiceInstallPruneSkipsNonVersionDirectories pins that the prune only +// removes directories that actually parse as an installed version: an +// operator's stray directory, a directory whose manifest names a different +// version, and pkgtar's in-flight ".tmp-*" staging directory are all left +// alone. +func TestServiceInstallPruneSkipsNonVersionDirectories(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + seedVersionDir(t, dir, prunePluginID, "0.9.0") + + pluginDir := filepath.Join(dir, prunePluginID) + if err := os.MkdirAll(filepath.Join(pluginDir, "notes"), 0o755); err != nil { + t.Fatalf("MkdirAll(notes): %v", err) + } + // pkgtar extracts into a ".tmp-*" dir under the plugin directory before + // renaming it into place, so a concurrent install has a fully populated + // staging dir here that the prune must not delete out from under it. + staging := seedVersionDir(t, dir, prunePluginID, "1.3.0") + if err := os.Rename(staging, filepath.Join(pluginDir, ".tmp-inflight")); err != nil { + t.Fatalf("Rename(.tmp-inflight): %v", err) + } + // A directory whose manifest declares a different version than its own + // name is not something the installer produced; leave it to the operator. + mismatched := seedVersionDir(t, dir, prunePluginID, "0.8.0") + if err := os.Rename(mismatched, filepath.Join(pluginDir, "misnamed")); err != nil { + t.Fatalf("Rename(misnamed): %v", err) + } + + for _, v := range []string{"1.1.0", "1.2.0"} { + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, v, false)); err != nil { + t.Fatalf("Install(%s): %v", v, err) + } + } + + assertVersionDirs(t, dir, prunePluginID, "1.1.0", "1.2.0", "notes", ".tmp-inflight", "misnamed") +} + +// TestServiceInstallSpawnFailurePrunesNothing pins that pruning happens only +// after the new version is confirmed started. A failed spawn must leave every +// other version on disk, because the operator's way out is the previous +// version. +func TestServiceInstallSpawnFailurePrunesNothing(t *testing.T) { + svc, dir, _, rt := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + seedVersionDir(t, dir, prunePluginID, "0.9.0") + + rt.setStartErr(prunePluginID, errors.New("spawn failed")) + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err == nil { + t.Fatal("Install() expected an error when the new version fails to spawn") + } + + assertVersionDirs(t, dir, prunePluginID, "0.9.0", "1.0.0", "1.1.0") +} + +// TestServiceInstallSaveFailurePrunesNothingAndRestartsPrevious pins the +// other failure mode: rollbackFailedInstall removes only the just-extracted +// version and restarts the previous one, and the prune never runs. +func TestServiceInstallSaveFailurePrunesNothingAndRestartsPrevious(t *testing.T) { + dir := t.TempDir() + fsStore := store.NewFSStore(dir) + failing := &failingSaveStore{Store: fsStore} + svc := NewService(failing, NewRegistry(), nil, testLogger(t)) + svc.SetPluginsDir(dir) + rt := newFakeRuntime() + svc.SetRuntime(rt) + + installTestPlugin(t, svc, prunePluginID) // 1.0.0, active + running + seedVersionDir(t, dir, prunePluginID, "0.9.0") + + failing.armFailNextSave() + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err == nil { + t.Fatal("Install() expected an error from the simulated Save failure") + } + + assertVersionDirs(t, dir, prunePluginID, "0.9.0", "1.0.0") + if !rt.Running(prunePluginID) { + t.Fatal("the previous version's process was not restarted after the failed upgrade") + } +} + +// TestServiceInstallPruneFailureDoesNotFailInstall pins that cleanup is +// best-effort: a plugin that installed and started correctly is never +// reported as failed because a superseded directory could not be removed. +func TestServiceInstallPruneFailureDoesNotFailInstall(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + seedVersionDir(t, dir, prunePluginID, "0.9.0") + + requireUnwritableVersionDir(t, filepath.Join(dir, prunePluginID), "0.9.0") + + rec, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)) + if err != nil { + t.Fatalf("Install() failed because the prune could not delete a directory: %v", err) + } + if rec.Status != StatusActive { + t.Fatalf("Install() Status = %q, want %q", rec.Status, StatusActive) + } +} + +// requireUnwritableVersionDir makes pluginDir/version reject removal of its +// contents, skipping the test when the current executor does not enforce the +// permission bit (root, or a filesystem/OS that ignores it). The version is a +// parameter rather than a constant so the coupling to what the caller seeded +// is visible at the call site: a caller that seeded a different version would +// otherwise get a silent no-op chmod. +func requireUnwritableVersionDir(t *testing.T, pluginDir, version string) { + t.Helper() + probeParent := filepath.Join(t.TempDir(), "probe") + if err := os.MkdirAll(filepath.Join(probeParent, "child"), 0o755); err != nil { + t.Fatalf("MkdirAll(probe): %v", err) + } + if err := os.Chmod(probeParent, 0o555); err != nil { + t.Skipf("chmod unsupported on this platform: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(probeParent, 0o755) }) + if err := os.RemoveAll(filepath.Join(probeParent, "child")); err == nil { + t.Skip("this executor ignores the directory write bit (running as root?)") + } + + // The install itself must still be able to create the new version + // directory, so make only the superseded version's own contents + // undeletable rather than the plugin directory. + stale := filepath.Join(pluginDir, version) + if err := os.Chmod(stale, 0o555); err != nil { + t.Skipf("chmod unsupported on this platform: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(stale, 0o755) }) +} + +// TestServiceInstallWithoutRuntimePrunesNothing pins the precondition against +// the one path where activate reports success without starting anything: with +// no runtime wired, activate only persists StatusActive, so there is no +// confirmed start and no version may be discarded. +func TestServiceInstallWithoutRuntimePrunesNothing(t *testing.T) { + dir := t.TempDir() + svc := NewService(store.NewFSStore(dir), NewRegistry(), nil, testLogger(t)) + svc.SetPluginsDir(dir) + + for _, v := range []string{"1.0.0", "1.1.0", "1.2.0"} { + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, v, false)); err != nil { + t.Fatalf("Install(%s): %v", v, err) + } + } + + assertVersionDirs(t, dir, prunePluginID, "1.0.0", "1.1.0", "1.2.0") +} + +// TestPruneSkipsInFlightExtraction pins the guard against a concurrent +// install's freshly extracted directory. Install extracts before it can know +// the plugin id, so it cannot hold that id's lifecycle lock yet; a prune +// running under the lock must still leave that directory alone, and must +// reclaim it once the install that extracted it is done. +func TestPruneSkipsInFlightExtraction(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0 + if _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)); err != nil { + t.Fatalf("Install(1.1.0): %v", err) + } + + // Exactly what a competing install has done by the time it blocks on the + // lifecycle lock: extracted and marked in flight, nothing else. + inFlight, err := svc.extractPackage(testPackage(t, prunePluginID, "1.2.0", false)) + if err != nil { + t.Fatalf("extractPackage(1.2.0): %v", err) + } + + svc.pruneSupersededVersions(prunePluginID, "1.1.0", "1.0.0") + assertVersionDirs(t, dir, prunePluginID, "1.0.0", "1.1.0", "1.2.0") + + svc.releaseExtraction(inFlight.InstallPath) + svc.pruneSupersededVersions(prunePluginID, "1.1.0", "1.0.0") + assertVersionDirs(t, dir, prunePluginID, "1.0.0", "1.1.0") +} + +// TestConcurrentInstallsKeepBothExtractions drives the interleaving end to +// end: the first install is held inside its store write, holding the lifecycle +// lock, while a second install for the same id extracts a different version +// behind it. The first must not prune the second's package out from under it. +func TestConcurrentInstallsKeepBothExtractions(t *testing.T) { + dir := t.TempDir() + barrier := &armedBarrierStore{Store: store.NewFSStore(dir)} + svc := NewService(barrier, NewRegistry(), nil, testLogger(t)) + svc.SetPluginsDir(dir) + svc.SetRuntime(newFakeRuntime()) + + // 1.0.0 must be a real installed record, so the first racing install + // treats it as its rollback target and 1.2.0 becomes a prune candidate. + installTestPlugin(t, svc, prunePluginID) + + entered, release := barrier.arm() + errs := make(chan error, 2) + go func() { + _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.1.0", false)) + errs <- err + }() + <-entered // 1.1.0 extracted, lifecycle lock held, prune still ahead + + go func() { + _, err := svc.Install(context.Background(), testPackage(t, prunePluginID, "1.2.0", false)) + errs <- err + }() + waitForVersionDir(t, dir, prunePluginID, "1.2.0") // extracted, now blocked on the lock + + close(release) + if err := <-errs; err != nil { + t.Fatalf("concurrent install error: %v", err) + } + if err := <-errs; err != nil { + t.Fatalf("concurrent install error: %v", err) + } + + for _, version := range []string{"1.1.0", "1.2.0"} { + if _, statErr := os.Stat(filepath.Join(dir, prunePluginID, version)); statErr != nil { + t.Fatalf("version %s was pruned out from under a concurrent install: %v", version, statErr) + } + } + rec, err := svc.Get(prunePluginID) + if err != nil { + t.Fatalf("Get(): %v", err) + } + if _, statErr := os.Stat(rec.InstallPath); statErr != nil { + t.Fatalf("the winning record's InstallPath %q does not exist: %v", rec.InstallPath, statErr) + } +} + +// armedBarrierStore pauses one chosen Save, so a test can pin an install +// inside the lifecycle-locked region of Install (its record write) while a +// competing install runs behind it. Unarmed saves pass straight through, which +// keeps ordinary setup installs out of the barrier. +type armedBarrierStore struct { + store.Store + mu sync.Mutex + armed bool + entered chan struct{} + release chan struct{} +} + +// arm makes the next Save block; entered closes once that Save is reached and +// the returned release channel unblocks it when closed. +func (s *armedBarrierStore) arm() (entered <-chan struct{}, release chan struct{}) { + s.mu.Lock() + defer s.mu.Unlock() + s.entered = make(chan struct{}) + s.release = make(chan struct{}) + s.armed = true + return s.entered, s.release +} + +func (s *armedBarrierStore) Save(rec *store.Record) error { + s.mu.Lock() + armed := s.armed + s.armed = false + entered, release := s.entered, s.release + s.mu.Unlock() + if armed { + close(entered) + <-release + } + return s.Store.Save(rec) +} + +// waitForVersionDir blocks until a version directory appears, so a test can +// synchronize on a competing install's extraction (a real side effect) rather +// than on a sleep. +func waitForVersionDir(t *testing.T, pluginsDir, id, version string) { + t.Helper() + dir := filepath.Join(pluginsDir, id, version) + deadline := time.NewTimer(10 * time.Second) + defer deadline.Stop() + tick := time.NewTicker(time.Millisecond) + defer tick.Stop() + for { + if _, err := os.Stat(dir); err == nil { + return + } + select { + case <-tick.C: + case <-deadline.C: + t.Fatalf("version dir %q never appeared", dir) + } + } +} + +// TestStartActivePluginsPrunesAfterHealthyStart pins boot-time recovery: an +// instance that already accumulated superseded versions collapses them on the +// next restart, without waiting for an install that may never come. +func TestStartActivePluginsPrunesAfterHealthyStart(t *testing.T) { + svc, dir, _, rt := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0, active + for _, v := range []string{"0.7.0", "0.8.0", "0.9.0"} { + seedVersionDir(t, dir, prunePluginID, v) + } + rt.Stop(prunePluginID) // simulate a fresh process: registered active, not running + + svc.StartActivePlugins(context.Background()) + + if !rt.Running(prunePluginID) { + t.Fatal("StartActivePlugins() did not start the active plugin") + } + assertVersionDirs(t, dir, prunePluginID, "0.9.0", "1.0.0") +} + +// TestStartActivePluginsDoesNotPruneWhenStartFails pins the same +// confirmed-start precondition at boot. +func TestStartActivePluginsDoesNotPruneWhenStartFails(t *testing.T) { + svc, dir, _, rt := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0, active + seedVersionDir(t, dir, prunePluginID, "0.8.0") + seedVersionDir(t, dir, prunePluginID, "0.9.0") + rt.Stop(prunePluginID) + rt.setStartErr(prunePluginID, errors.New("spawn failed")) + + svc.StartActivePlugins(context.Background()) + + assertVersionDirs(t, dir, prunePluginID, "0.8.0", "0.9.0", "1.0.0") +} + +// TestStartActivePluginsDoesNotPruneDisabledPlugin pins that a plugin nobody +// started keeps every version an operator staged for it. +func TestStartActivePluginsDoesNotPruneDisabledPlugin(t *testing.T) { + svc, dir, _, _ := newTestServiceWithDir(t) + installTestPlugin(t, svc, prunePluginID) // 1.0.0, active + seedVersionDir(t, dir, prunePluginID, "0.8.0") + seedVersionDir(t, dir, prunePluginID, "0.9.0") + if err := svc.Disable(prunePluginID); err != nil { + t.Fatalf("Disable(): %v", err) + } + + svc.StartActivePlugins(context.Background()) + + assertVersionDirs(t, dir, prunePluginID, "0.8.0", "0.9.0", "1.0.0") +} diff --git a/docs/decisions/2026-08-22-plugin-version-retention.md b/docs/decisions/2026-08-22-plugin-version-retention.md new file mode 100644 index 0000000000..659c6a2503 --- /dev/null +++ b/docs/decisions/2026-08-22-plugin-version-retention.md @@ -0,0 +1,82 @@ +# ADR-2026-08-22-plugin-version-retention: Keep Exactly One Superseded Plugin Version + +**Status:** accepted +**Date:** 2026-08-22 +**Area:** backend + +## Context + +Every plugin install extracts a full package into `~/.kandev/plugins///`, and no +install path ever removed the version it replaced. Keeping the predecessor during the upgrade is +deliberate: `Service.Install` stops the running plugin before swapping, and the old directory is +what `rollbackFailedInstall` restarts when the new version fails. Nothing, however, removed it +afterwards. + +All four install routes (marketplace/URL, auto-update poller, sideload upload, dropped tarball) +converge on that same code, and the auto-update poller installs with no user action at all, so the +growth is unattended. A version directory is roughly 20MB, or 95MB for a package shipping all five +platform binaries. On one developer machine the plugins directory had reached 4.2GB, of which +3.74GB was 58 superseded version directories, 46 of them belonging to a single auto-updating +plugin. + +## Decision + +A plugin converges on two extracted versions: the active one and the version it replaced. The +retained count is fixed in code, not configurable. It is a convergent state rather than an enforced +cap, because every precondition below can legitimately leave more versions on disk. + +Deletion is gated on a **confirmed running process**, never on the install alone. `Install` prunes +after `activate` returns successfully and boot (`StartActivePlugins`) prunes after `runtime.Start` +succeeds, but in both cases `pruneSupersededVersions` itself re-checks `runtime.Running(id)` before +deleting anything — `activate` returns nil without starting a process when no runtime is wired, so +"activate succeeded" is not the same guarantee. A failed install, a failed spawn, a disabled plugin, +a sideload registered disabled, and a plugin in `error` therefore all keep every version they have, +as does a plugin whose deletion failed. `rollbackFailedInstall` is unchanged, and remains the one +path that deletes a version directory without a prune: it removes the newly extracted version, and +only that one, when the record cannot be persisted. + +Extraction is serialized and tracked. `Install` cannot take a plugin's lifecycle lock until it has +parsed the manifest, so two overlapping installs of the same id both extract before either is +serialized; the extraction and its in-flight registration happen under one mutex, and a prune skips +any directory still marked in flight. Without that, the first install to acquire the lock would +delete the second's fresh package and strand it activating a path that no longer exists. + +A directory is a deletion candidate only if it is a real directory (never a symlink), is not +`data/` or any dot-prefixed name (which covers pkgtar's `.tmp-*` staging directories), and contains +a `manifest.yaml` declaring exactly that plugin id and that directory name as its version. Listing, +manifest reads, and removal all go through a single `os.Root` handle on the plugin directory, so a +path validated by the scan is removed inside the same confined tree and the `.yml` / +`.config.yml` records one level up are unreachable. + +Pruning is best-effort: every failure is logged and swallowed, because a plugin that installed and +started correctly must not be reported as failed because cleanup could not delete a directory. + +## Consequences + +- Disk use per plugin becomes bounded by two versions instead of growing with every install. +- Rollback keeps working: the version a failed upgrade restarts is always still on disk. +- Boot-time pruning lets an instance that already accumulated versions recover on its next restart, + including plugins that are already at their latest version and would never trigger an install. +- The prune deletes durable on-disk state, so its preconditions are the safety contract and any + change to them needs regression coverage (`service_prune_test.go`). +- Rolling back further than one version now requires reinstalling the wanted package. + +## Alternatives Considered + +- **Make the retained count configurable.** Rejected as speculative. The second directory exists to + serve a specific mechanism (rollback of a failed upgrade), which is a safety invariant rather than + an operator preference, and any larger configured value re-creates the unbounded growth this + exists to stop. A knob would also need UI, docs, and validation for no demonstrated need. +- **Keep only the active version.** Rejected because it deletes the rollback target + `rollbackFailedInstall` depends on, converting a failed upgrade from recoverable into an outage. +- **Prune immediately after extraction, before the new version starts.** Rejected for the same + reason: it removes the fallback exactly when it is most likely to be needed. +- **Age-based retention (delete versions older than N days).** Rejected because it neither bounds + disk use for a frequently-updating plugin nor guarantees a rollback target exists for a rarely + updated one. +- **Install-time pruning only.** Rejected because the instances that motivated this fix are already + full of superseded versions, and a plugin at its latest version, or one with auto-update off, + would never install again and never recover. +- **A separate operator-triggered cleanup action.** Rejected as a worse default: the growth is + unattended, so the recovery should be too, and an operator who has not noticed 3.74GB of stale + packages will not press a button about them. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index b3cc42204a..035e520963 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -187,3 +187,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-19-repository-qualified-comparison-targets | [Qualify Git Comparison Targets by Repository](2026-08-19-repository-qualified-comparison-targets.md) | accepted | backend, agentctl, frontend, protocol, GitHub, GitLab | 2026-08-19 | | 2026-08-20-startup-configuration-source-parity | [Startup Configuration Uses One Typed Source Model](2026-08-20-startup-configuration-source-parity.md) | accepted | backend, agentctl, frontend, cli, security, operations | 2026-08-20 | | 2026-08-20-acp-client-non-underscore-extension-methods | [Route Non-Underscore Inbound Client Methods to the Extension Handler](2026-08-20-acp-client-non-underscore-extension-methods.md) | accepted | backend, protocol | 2026-08-20 | +| 2026-08-22-plugin-version-retention | [Keep Exactly One Superseded Plugin Version](2026-08-22-plugin-version-retention.md) | accepted | backend | 2026-08-22 | diff --git a/docs/public/plugins-authoring.md b/docs/public/plugins-authoring.md index 947a23eb6e..6bc480b5ee 100644 --- a/docs/public/plugins-authoring.md +++ b/docs/public/plugins-authoring.md @@ -44,7 +44,12 @@ flowchart LR Installation validates the manifest, archive paths, checksums, managed runtime, and the current host executable before extraction. Kandev then supervises the -declared subprocess and injects a Host connection. The UI bundle is static +declared subprocess and injects a Host connection. Installing a new version +does not remove the one it replaces: that version stays on disk as the rollback +target, and only the versions before it are deleted, once the new one is +confirmed running. Two extracted versions is therefore the steady state for a +plugin that runs; a plugin that is disabled or failed to start keeps every +version it has, and `data/` is never part of that cleanup. The UI bundle is static package content loaded by the browser; it does not run inside the backend subprocess. On disable or uninstall, the host calls destroy when present and bulk-revokes registrations, styles, routes, handlers, and navigation. Reloads and diff --git a/docs/public/plugins.md b/docs/public/plugins.md index 93d7c2b85a..d049d4e7ae 100644 --- a/docs/public/plugins.md +++ b/docs/public/plugins.md @@ -127,6 +127,14 @@ Either path runs the same pipeline: Before persistence, credential-like values such as PATs, bearer tokens, labeled secrets/API keys, and the host home path are redacted; plugin stdout is not stored verbatim. +6. Once the new version is confirmed running, delete the plugin's older + extracted versions, keeping the version now running plus the one it + replaced as a rollback target (see "Version retention on disk"). This step + is skipped entirely when the install or the spawn fails, and a deletion that + fails is logged without failing the install. Skipping it is not the same as + deleting nothing: an install that extracted successfully but could not be + recorded is rolled back, which removes that one new version directory and + restarts the previous one. A successful install that failed to spawn returns HTTP 201 with a `warning` field rather than failing outright. The package is installed, @@ -247,6 +255,25 @@ host-managed runtime fields, including `signed`, `last_error`, and `last_error_at`. The diagnostic fields are empty until a runtime failure is recorded and are cleared after successful recovery. +### Version retention on disk + +Two extracted versions is the steady state for a plugin that runs: the one +currently running and the one it replaced, which is what a failed upgrade falls +back to. Older versions are deleted once a newer one is confirmed running, +either right after the install that superseded them or at the next backend +start. The retained count is fixed and not configurable. + +It is a steady state rather than a hard cap, because the cleanup is +conservative and never insists. Kandev deletes a version directory only after +confirming that the plugin's process is up, so a plugin that is disabled, +errored, or was never started keeps every version it has, and a deletion that +fails leaves that version in place until the next attempt. It never +touches `data/`, a directory whose name does not match the version declared by +the `manifest.yaml` inside it, or anything outside `~/.kandev/plugins//`. +Because an upgrade previously left its predecessor behind forever, an instance +that has been auto-updating for a long time can reclaim a substantial amount of +disk on its first restart after upgrading to this version. + ## Security posture - **Auth is the spawn relationship.** Kandev spawns the plugin subprocess