supervisor: adopt only <id>, not swap dirs; a fresh crash record per install

plugin/appstore/supervisor.go

@@ -15,6 +15,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"runtime"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -170,11 +171,12 @@ type supervisor struct {
 	crashes   map[string]*crashRecord       // app_id → sliding-window crash counter
 	appCancel map[string]context.CancelFunc // app_id → cancel its per-app context (used to stop a supervise goroutine on detected uninstall)
 
-	// sigMu guards sigFails. Kept separate from mu because the
-	// signature-failure bookkeeping happens inside scanInstalled, which
-	// must not hold the main lock while walking the filesystem.
+	// sigMu guards sigFails and misnamed. Kept separate from mu because
+	// this bookkeeping happens inside scanInstalled, which must not hold
+	// the main lock while walking the filesystem.
 	sigMu    sync.Mutex
 	sigFails map[string]*sigFailRecord // install-dir name → manifest-signature failure state
+	misnamed map[string]bool           // install-dir names skipped (and logged) because they are not their manifest's app ID
 }
 
 func newSupervisor(cfg Config, deps Deps, logger *log.Logger) *supervisor {
@@ -191,13 +193,14 @@ func newSupervisor(cfg Config, deps Deps, logger *log.Logger) *supervisor {
 		crashes:   map[string]*crashRecord{},
 		appCancel: map[string]context.CancelFunc{},
 		sigFails:  map[string]*sigFailRecord{},
+		misnamed:  map[string]bool{},
 	}
 }
 
 // crashLoopWindow + maxCrashesInWindow define when an app is judged to
 // be stuck in a crash-loop. Exceed the cap and the supervisor stops
-// respawning until either the daemon restarts or a future
-// pilotctl-driven "appstore restart" command clears the suspended bit.
+// respawning until the daemon restarts, or the app is upgraded or
+// reinstalled (the rescan then starts it with a clean record).
 const (
 	crashLoopWindow    = 60 * time.Second
 	maxCrashesInWindow = 5
@@ -399,10 +402,46 @@ func (s *supervisor) clearSignatureFailure(dirName, appID string) {
 	}
 }
 
+// noteMisnamed logs the install dirs one scan skipped because their name is
+// not their manifest's app ID (skipped maps dir name → app ID; present holds
+// every dir name in the scan). Each dir is logged once, not on every rescan
+// tick; a name absent from this scan is forgotten, so a dir that comes back
+// later (the next install's staging dir) is logged again.
+//
+// An <id>.previous with no <id> beside it is an install interrupted mid-swap:
+// the app is no longer running, so that line says how to restore it.
+func (s *supervisor) noteMisnamed(skipped map[string]string, present map[string]bool) {
+	s.sigMu.Lock()
+	var fresh []string
+	for name := range skipped {
+		if !s.misnamed[name] {
+			fresh = append(fresh, name)
+		}
+	}
+	s.misnamed = make(map[string]bool, len(skipped))
+	for name := range skipped {
+		s.misnamed[name] = true
+	}
+	s.sigMu.Unlock()
+
+	sort.Strings(fresh)
+	for _, name := range fresh {
+		id := skipped[name]
+		if strings.HasPrefix(name, id+".previous") && !present[id] {
+			s.logger.Printf("skip %s: app %s is not installed, and this is its previous install, left by an install that did not finish — run `pilotctl appstore install %s` to restore it", name, id, id)
+			continue
+		}
+		s.logger.Printf("skip %s: dir name is not its app id %s (an install's staging or replaced dir) — not adopting", name, id)
+	}
+}
+
 // scanInstalled walks InstallRoot, reads each `<app>/manifest.json`, and
 // returns the verified-by-syntax set. Sha256 verification is per-launch
 // (in run()), not per-scan, so a corrupted binary surfaces at the right
 // time.
+//
+// Only `<InstallRoot>/<id>` is an install: a dir whose name is not its
+// manifest's ID is skipped (see the check after Validate).
 func (s *supervisor) scanInstalled() ([]*installedApp, error) {
 	entries, err := os.ReadDir(s.cfg.InstallRoot)
 	if err != nil {
@@ -412,6 +451,7 @@ func (s *supervisor) scanInstalled() ([]*installedApp, error) {
 		return nil, err
 	}
 	var out []*installedApp
+	misnamed := map[string]string{} // dir name → app ID, for noteMisnamed
 	for _, e := range entries {
 		if !e.IsDir() {
 			continue
@@ -432,6 +472,18 @@ func (s *supervisor) scanInstalled() ([]*installedApp, error) {
 			s.logger.Printf("skip %s: invalid manifest: %v", e.Name(), errs[0])
 			continue
 		}
+		// Only <InstallRoot>/<id> is an install. pilotctl's install leaves
+		// <id>.staging (from the manifest write to the swap) and
+		// <id>.previous (from the swap until it is retired) beside it, both
+		// holding a valid manifest for the same app. Adopting one gave the
+		// app a second supervisor: on a rescan it "rebuilt" the app from
+		// the old copy, the two overwrote each other's appCancel entry and
+		// reaped each other's process until the app was suspended, and at
+		// daemon start it ran beside the live install.
+		if e.Name() != m.ID {
+			misnamed[e.Name()] = m.ID
+			continue
+		}
 		// Sideload detection: presence of `.sideloaded` in the install
 		// dir flips the trust regime. Sideloaded apps skip the
 		// publisher-signature check (they were never signed by the
@@ -513,6 +565,11 @@ func (s *supervisor) scanInstalled() ([]*installedApp, error) {
 			Sideloaded: sideloaded,
 		})
 	}
+	present := make(map[string]bool, len(entries))
+	for _, e := range entries {
+		present[e.Name()] = true
+	}
+	s.noteMisnamed(misnamed, present)
 	return out, nil
 }
 
@@ -683,6 +740,12 @@ func (s *supervisor) rescanForNew() []*installedApp {
 				BinaryAt: a.BinaryPath,
 			})
 		}
+		// A new install or version starts with a clean crash-loop record.
+		// Records are keyed by app ID, so keeping it would leave v2
+		// suspended because v1 crash-looped (recordCrash's suspended bit is
+		// sticky). The .suspended marker is removed by superviseOne when it
+		// starts.
+		delete(s.crashes, a.Manifest.ID)
 		s.installed[a.Manifest.ID] = a
 		fresh = append(fresh, a)
 		s.logger.Printf("rescan: discovered new app id=%s dir=%s", a.Manifest.ID, a.Dir)
@@ -717,6 +780,9 @@ func (s *supervisor) rescanForGone() {
 		}
 		delete(s.installed, id)
 		delete(s.ready, id)
+		// The crash record goes with the install, so a reinstall under the
+		// same ID does not come back suspended.
+		delete(s.crashes, id)
 		s.logger.Printf("rescan: app id=%s removed from disk; supervise goroutine canceled", id)
 	}
 }

plugin/appstore/zz8_install_dirs_test.go

@@ -0,0 +1,176 @@
+package appstore
+
+import (
+	"bytes"
+	"log"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// writeAppDirAt writes an app dir for id (writeAppDirWithVersionSHA, in a
+// scratch root) and moves it to <root>/<dirName>: an install-root entry that
+// holds a valid manifest for id under a name that is not id, like pilotctl's
+// <id>.staging and <id>.previous.
+func writeAppDirAt(t *testing.T, root, dirName, id, version, binSHA string) string {
+	t.Helper()
+	src := writeAppDirWithVersionSHA(t, t.TempDir(), id, version, binSHA)
+	dst := filepath.Join(root, dirName)
+	if err := os.Rename(src, dst); err != nil {
+		t.Fatal(err)
+	}
+	return dst
+}
+
+// TestRescanIgnoresStagingAndPreviousDirs: pilotctl's install leaves
+// <id>.staging (from the manifest write to the swap) and <id>.previous (from
+// the swap until it is retired) beside the live <id>, each holding a valid
+// manifest for the app. scanInstalled adopted both, so on a same-version
+// rebuild the rescan swapped the new <id> in and then "rebuilt" the app again
+// from .previous and .staging: several supervise goroutines for one app, each
+// overwriting appCancel[id] and reaping the others' process, until the app
+// was suspended. Only <id> may be adopted, and the next tick must be a no-op.
+func TestRescanIgnoresStagingAndPreviousDirs(t *testing.T) {
+	root := t.TempDir()
+	const id = "io.test.app"
+	shaA := strings.Repeat("a", 64)
+	shaB := strings.Repeat("b", 64)
+	shaC := strings.Repeat("c", 64)
+	live := writeAppDirWithVersionSHA(t, root, id, "1.0.0", shaB)
+	writeAppDirAt(t, root, id+".previous", id, "1.0.0", shaA)
+	writeAppDirAt(t, root, id+".staging", id, "1.0.0", shaC)
+
+	sup := newSupervisor(Config{
+		CataloguePublisher: testCatPub,
+		InstallRoot:        root,
+		RescanInterval:     20 * 1e6,
+	}, Deps{}, newQuietLogger(t))
+
+	// In memory: the install from before the rebuild (sha A).
+	entry := &installedApp{
+		Dir:        live,
+		Manifest:   parseDummyManifest(t, id),
+		BinaryPath: filepath.Join(live, "bin/x"),
+	}
+	entry.Manifest.AppVersion = "1.0.0"
+	entry.Manifest.Binary.SHA256 = shaA
+	sup.mu.Lock()
+	sup.installed[id] = entry
+	sup.mu.Unlock()
+
+	fresh := sup.rescanForNew()
+	var dirs []string
+	for _, a := range fresh {
+		dirs = append(dirs, filepath.Base(a.Dir))
+	}
+	if len(fresh) != 1 || fresh[0].Dir != live {
+		t.Fatalf("rescanForNew started %v, want exactly [%s]", dirs, id)
+	}
+	sup.mu.RLock()
+	got := sup.installed[id]
+	sup.mu.RUnlock()
+	if got.Dir != live || got.Manifest.Binary.SHA256 != shaB {
+		t.Errorf("installed[%s] = %s pinned %s, want %s pinned %s",
+			id, filepath.Base(got.Dir), shortSHA(got.Manifest.Binary.SHA256), id, shortSHA(shaB))
+	}
+	// Nothing changed on disk: the next tick must not restart the app again.
+	if again := sup.rescanForNew(); len(again) != 0 {
+		t.Errorf("unchanged install root restarted %d apps on the next tick, want 0", len(again))
+	}
+}
+
+// TestScanInstalled_SkipsDirNotNamedForItsApp: only <InstallRoot>/<id> is an
+// install. A dir holding a valid manifest under any other name (pilotctl's
+// <id>.previous, a hand-made copy) is skipped. Daemon start runs every scanned
+// dir, so adopting one ran a second copy of the app beside the live one. The
+// skip is logged once per dir, not on every rescan, and again only if the dir
+// goes away and comes back.
+func TestScanInstalled_SkipsDirNotNamedForItsApp(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+	const id = "io.test.app"
+	sha := strings.Repeat("a", 64)
+	live := writeAppDirWithVersionSHA(t, root, id, "1.0.0", sha)
+	writeAppDirAt(t, root, id+".previous", id, "1.0.0", sha)
+	copyDir := writeAppDirAt(t, root, "copy-of-test-app", id, "1.0.0", sha)
+
+	var logs bytes.Buffer
+	sup := newSupervisor(Config{InstallRoot: root, CataloguePublisher: testCatPub}, Deps{}, log.New(&logs, "", 0))
+	scan := func() {
+		t.Helper()
+		apps, err := sup.scanInstalled()
+		if err != nil {
+			t.Fatal(err)
+		}
+		var dirs []string
+		for _, a := range apps {
+			dirs = append(dirs, filepath.Base(a.Dir))
+		}
+		if len(apps) != 1 || apps[0].Dir != live {
+			t.Fatalf("scanInstalled adopted %v, want exactly [%s]", dirs, id)
+		}
+	}
+	logged := func(name string) int {
+		return strings.Count(logs.String(), "skip "+name+":")
+	}
+
+	for i := 0; i < 3; i++ {
+		scan()
+	}
+	for _, name := range []string{id + ".previous", "copy-of-test-app"} {
+		if n := logged(name); n != 1 {
+			t.Errorf("%s logged %d times over 3 scans, want once:\n%s", name, n, logs.String())
+		}
+	}
+
+	// Gone, then back: a new occurrence is logged again.
+	if err := os.RemoveAll(copyDir); err != nil {
+		t.Fatal(err)
+	}
+	scan()
+	writeAppDirAt(t, root, "copy-of-test-app", id, "1.0.0", sha)
+	scan()
+	if n := logged("copy-of-test-app"); n != 2 {
+		t.Errorf("copy-of-test-app logged %d times after it came back, want 2:\n%s", n, logs.String())
+	}
+}
+
+// An install interrupted after it renamed <id> to <id>.previous and before
+// it renamed <id>.staging to <id> leaves no <id>: the app is not adopted and
+// stops running. The skip line says so and how to restore it; beside a live
+// <id> it stays the plain skip line.
+func TestScanInstalled_PreviousWithoutLiveSaysHowToRestore(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+	const id = "io.test.app"
+	sha := strings.Repeat("a", 64)
+	writeAppDirAt(t, root, id+".previous", id, "1.0.0", sha)
+	writeAppDirAt(t, root, id+".staging", id, "1.0.1", sha)
+
+	var logs bytes.Buffer
+	sup := newSupervisor(Config{InstallRoot: root, CataloguePublisher: testCatPub}, Deps{}, log.New(&logs, "", 0))
+	if apps, err := sup.scanInstalled(); err != nil || len(apps) != 0 {
+		t.Fatalf("scanInstalled = %d apps, %v; want none", len(apps), err)
+	}
+	restore := "run `pilotctl appstore install " + id + "` to restore it"
+	if !strings.Contains(logs.String(), "skip "+id+".previous: app "+id+" is not installed") || !strings.Contains(logs.String(), restore) {
+		t.Errorf("no restore hint for a .previous without its app:\n%s", logs.String())
+	}
+	if strings.Count(logs.String(), restore) != 1 {
+		t.Errorf("restore hint not once (the staging dir must not get it):\n%s", logs.String())
+	}
+
+	// With the live dir back, a later .previous is the ordinary skip.
+	logs.Reset()
+	root2 := t.TempDir()
+	writeAppDirWithVersionSHA(t, root2, id, "1.0.0", sha)
+	writeAppDirAt(t, root2, id+".previous", id, "1.0.0", sha)
+	sup2 := newSupervisor(Config{InstallRoot: root2, CataloguePublisher: testCatPub}, Deps{}, log.New(&logs, "", 0))
+	if _, err := sup2.scanInstalled(); err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(logs.String(), "restore it") || !strings.Contains(logs.String(), "not adopting") {
+		t.Errorf("a .previous beside its live app got the restore hint:\n%s", logs.String())
+	}
+}