diff --git a/plugin/appstore/supervisor.go b/plugin/appstore/supervisor.go index a71885d..64c99df 100644 --- a/plugin/appstore/supervisor.go +++ b/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 .previous with no 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 `/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 `/` 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 / is an install. pilotctl's install leaves + // .staging (from the manifest write to the swap) and + // .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) } } diff --git a/plugin/appstore/zz8_install_dirs_test.go b/plugin/appstore/zz8_install_dirs_test.go new file mode 100644 index 0000000..7cc45d2 --- /dev/null +++ b/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 /: an install-root entry that +// holds a valid manifest for id under a name that is not id, like pilotctl's +// .staging and .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 +// .staging (from the manifest write to the swap) and .previous (from +// the swap until it is retired) beside the live , each holding a valid +// manifest for the app. scanInstalled adopted both, so on a same-version +// rebuild the rescan swapped the new 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 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 / is an +// install. A dir holding a valid manifest under any other name (pilotctl's +// .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 to .previous and before +// it renamed .staging to leaves no : the app is not adopted and +// stops running. The skip line says so and how to restore it; beside a live +// 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()) + } +} diff --git a/plugin/appstore/zz9_crash_record_test.go b/plugin/appstore/zz9_crash_record_test.go new file mode 100644 index 0000000..e92cfd1 --- /dev/null +++ b/plugin/appstore/zz9_crash_record_test.go @@ -0,0 +1,112 @@ +package appstore + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// crashLoop records enough exits for id to spend its crash-loop budget. +func crashLoop(t *testing.T, sup *supervisor, id string) { + t.Helper() + for i := 0; i <= maxCrashesInWindow; i++ { + sup.recordCrash(id) + } + if !sup.isSuspended(id) { + t.Fatalf("%s not suspended after %d crashes", id, maxCrashesInWindow+1) + } +} + +// TestRescanUpgradeClearsCrashRecord: crash records are keyed by app ID, and +// the upgrade branch of rescanForNew kept s.crashes[id]. A v1 suspended for +// crash-looping made v2 report suspended from the start, and recordCrash's +// sticky suspended bit stopped v2 at its first exit. A new version starts +// with a clean record; the .suspended marker v1 left is removed when v2's +// supervise goroutine starts. +func TestRescanUpgradeClearsCrashRecord(t *testing.T) { + root := t.TempDir() + const id = "io.test.app" + appDir := writeAppDirWithVersion(t, root, id, "1.0.0") + + sup := newSupervisor(Config{ + CataloguePublisher: testCatPub, + InstallRoot: root, + RescanInterval: 20 * 1e6, + }, Deps{}, newQuietLogger(t)) + if fresh := sup.rescanForNew(); len(fresh) != 1 { + t.Fatalf("initial discovery: fresh=%d, want 1", len(fresh)) + } + crashLoop(t, sup, id) + if err := os.WriteFile(filepath.Join(appDir, suspendedMarkerName), nil, 0o600); err != nil { + t.Fatal(err) + } + + writeAppDirWithVersion(t, root, id, "2.0.0") + fresh := sup.rescanForNew() + if len(fresh) != 1 { + t.Fatalf("upgrade not applied: fresh=%d, want 1", len(fresh)) + } + if sup.isSuspended(id) { + t.Error("v2 is suspended: it inherited v1's crash record") + } + if apps := sup.Apps(); len(apps) != 1 || apps[0].AppVersion != "2.0.0" || apps[0].Suspended { + t.Errorf("Apps() = %+v, want one unsuspended 2.0.0", apps) + } + if sup.recordCrash(id) { + t.Error("v2 suspended at its first exit") + } + + // v2's supervise goroutine clears the marker as it starts. The context + // is already canceled, so it returns before spawning anything. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + sup.superviseOne(ctx, fresh[0]) + if _, err := os.Stat(filepath.Join(appDir, suspendedMarkerName)); !os.IsNotExist(err) { + t.Errorf(".suspended marker left after v2's supervisor started, stat err=%v", err) + } +} + +// TestRescanReinstallClearsCrashRecord: rescanForGone dropped an uninstalled +// app from installed but kept its crash record, so a reinstall (same ID) +// came back suspended. The record goes with the install. +func TestRescanReinstallClearsCrashRecord(t *testing.T) { + root := t.TempDir() + const id = "io.test.app" + appDir := writeAppDirWithVersion(t, root, id, "1.0.0") + + sup := newSupervisor(Config{ + CataloguePublisher: testCatPub, + InstallRoot: root, + RescanInterval: 20 * 1e6, + }, Deps{}, newQuietLogger(t)) + if fresh := sup.rescanForNew(); len(fresh) != 1 { + t.Fatalf("initial discovery: fresh=%d, want 1", len(fresh)) + } + crashLoop(t, sup, id) + + if err := os.RemoveAll(appDir); err != nil { + t.Fatal(err) + } + sup.rescanForGone() + sup.mu.RLock() + _, kept := sup.crashes[id] + sup.mu.RUnlock() + if kept { + t.Error("crash record survived the uninstall") + } + + writeAppDirWithVersion(t, root, id, "1.0.0") + if fresh := sup.rescanForNew(); len(fresh) != 1 { + t.Fatalf("reinstall not discovered: fresh=%d, want 1", len(fresh)) + } + if sup.isSuspended(id) { + t.Error("reinstalled app is suspended") + } + if apps := sup.Apps(); len(apps) != 1 || apps[0].Suspended { + t.Errorf("Apps() = %+v, want one unsuspended app", apps) + } + if sup.recordCrash(id) { + t.Error("reinstalled app suspended at its first exit") + } +}