From 9a6fe693f1a5b635c28e0eaed4ddf77abef4d13f Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:33:48 +0300 Subject: [PATCH 1/4] updater: cap updater.log the way the daemon caps daemon.log install.sh's launchd job points the updater's stdout and stderr at ~/.pilot/updater.log. launchd opens that file and never rotates it, and nothing else does: one laptop's held every line since the June install (2,205 lines, 310 KB), and each failed GitHub check adds a ~400-byte line an hour. The updater now runs internal/logcap on its own stderr, as the daemon does for daemon.log (#464): copy-truncate into gzipped updater.log.pilot.N.gz once the log passes -log-max-size MB (default 10; 0 disables), keeping -log-max-backups generations (default 3). Only a log inside ~/.pilot (or $PILOT_HOME/.pilot) is capped unless -log-max-size is given explicitly. Under systemd the updater logs to journald and this is a no-op. Tests: an over-cap ~/.pilot/updater.log opened O_APPEND is truncated and its bytes land in updater.log.pilot.1.gz; a log outside ~/.pilot is left alone unless the flag is explicit; -log-max-size=0 disables. End to end in a Linux container, the built binary rotated an 11 MB updater.log to a 236-byte log plus updater.log.pilot.1.gz. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/updater/logcap.go | 77 +++++++++++++++++++++ cmd/updater/main.go | 6 ++ cmd/updater/zz_logcap_test.go | 125 ++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+) create mode 100644 cmd/updater/logcap.go create mode 100644 cmd/updater/zz_logcap_test.go diff --git a/cmd/updater/logcap.go b/cmd/updater/logcap.go new file mode 100644 index 00000000..d0479a46 --- /dev/null +++ b/cmd/updater/logcap.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "context" + "flag" + "os" + "path/filepath" + "time" + + "github.com/pilot-protocol/pilotprotocol/internal/logcap" +) + +// install.sh's launchd job points the updater's stdout and stderr at +// ~/.pilot/updater.log (StandardOutPath/StandardErrorPath). launchd opens +// that file and never rotates it, and nothing else does either: one +// laptop's updater.log held every line since the June install (2,205 +// lines, 310 KB), and each failed GitHub check adds a ~400-byte line an +// hour. The daemon caps its own daemon.log from the inside +// (cmd/daemon/logrotation.go); the updater now does the same with the same +// package, keeping gzipped .pilot.N.gz generations. Under systemd the +// updater logs to journald and this is a no-op: stderr is not a regular +// file there. + +const ( + // defaultUpdaterLogMaxMB is -log-max-size's default. The updater writes + // a few lines an hour, so this is years of history. + defaultUpdaterLogMaxMB = 10 + // defaultUpdaterLogBackups is -log-max-backups's default, the same as + // the daemon's. + defaultUpdaterLogBackups = 3 + // updaterLogCheckInterval is how often the log's size is checked. + updaterLogCheckInterval = 5 * time.Minute +) + +// updaterLogDirs returns the directories the updater's log is in when +// Pilot set it up, where the cap applies by default: ~/.pilot (install.sh's +// launchd updater.log), and $PILOT_HOME/.pilot when that is set. A log an +// operator redirected elsewhere may be rotated by other means, so it is +// left alone unless -log-max-size is set explicitly. +func updaterLogDirs() []string { + var dirs []string + if home, err := os.UserHomeDir(); err == nil { + dirs = append(dirs, filepath.Join(home, ".pilot")) + } + if h := os.Getenv("PILOT_HOME"); h != "" { + dirs = append(dirs, filepath.Join(h, ".pilot")) + } + return dirs +} + +// capUpdaterLog starts capping the log f writes to (the updater's +// stderr) at maxMB megabytes, keeping backups gzipped generations, until +// ctx is done. anywhere lifts the ~/.pilot scope. It reports whether +// capping started: false when maxMB <= 0, f is not a regular file, or the +// log is out of scope. +func capUpdaterLog(ctx context.Context, f *os.File, maxMB, backups int, anywhere bool, interval time.Duration) bool { + return logcap.Watch(ctx, f, logcap.Options{ + MaxBytes: int64(maxMB) << 20, + MaxBackups: backups, + Anywhere: anywhere, + Within: updaterLogDirs(), + }, interval) +} + +// flagSetOnCommandLine reports whether flag name was given on the command +// line. +func flagSetOnCommandLine(name string) bool { + set := false + flag.Visit(func(f *flag.Flag) { + if f.Name == name { + set = true + } + }) + return set +} diff --git a/cmd/updater/main.go b/cmd/updater/main.go index 49cd7099..6a04297d 100644 --- a/cmd/updater/main.go +++ b/cmd/updater/main.go @@ -78,6 +78,8 @@ func main() { // stays on. Mirrors the --state-path pattern with an env fallback. skipAttestation := flag.Bool("skip-attestation", envBool("PILOT_UPDATER_SKIP_ATTESTATION"), "skip SLSA attestation verification (default off); for test/air-gapped use only — production verifies in-process, no `gh` needed") + logMaxSize := flag.Int("log-max-size", defaultUpdaterLogMaxMB, "rotate the updater's log once it exceeds this many MB (copy-truncate into gzipped .pilot.N.gz backups). By default only a log inside ~/.pilot is rotated (install.sh's launchd updater.log); a log elsewhere only when this is set explicitly. 0 disables") + logMaxBackups := flag.Int("log-max-backups", defaultUpdaterLogBackups, "gzipped generations kept by -log-max-size rotation (.pilot.1.gz ... .pilot.N.gz); 0 keeps none") flag.Parse() if *showVersion { @@ -91,6 +93,10 @@ func main() { } setupLogging(*logLevel, *logFormat) + // launchd never rotates ~/.pilot/updater.log; cap it from the inside + // (logcap.go). Lives for the process's lifetime. + capUpdaterLog(context.Background(), os.Stderr, *logMaxSize, *logMaxBackups, + flagSetOnCommandLine("log-max-size"), updaterLogCheckInterval) u := updater.New(updater.Config{ CheckInterval: *interval, diff --git a/cmd/updater/zz_logcap_test.go b/cmd/updater/zz_logcap_test.go new file mode 100644 index 00000000..625d8880 --- /dev/null +++ b/cmd/updater/zz_logcap_test.go @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "bytes" + "compress/gzip" + "context" + "io" + "os" + "path/filepath" + "testing" + "time" +) + +// openLog creates the log at path the way launchd does (O_APPEND) and +// fills it with size bytes. +func openLog(t *testing.T, path string, size int) (*os.File, []byte) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = f.Close() }) + data := bytes.Repeat([]byte("time=x level=ERROR msg=\"failed to fetch latest release\"\n"), size/56+1)[:size] + if _, err := f.Write(data); err != nil { + t.Fatal(err) + } + return f, data +} + +// TestUpdaterLogIsCappedInsidePilotDir: install.sh's launchd job writes the +// updater's stderr to ~/.pilot/updater.log, which nothing rotated. Past the +// cap it is truncated and its content kept as updater.log.pilot.1.gz. +func TestUpdaterLogIsCappedInsidePilotDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("PILOT_HOME", "") + logPath := filepath.Join(home, ".pilot", "updater.log") + f, data := openLog(t, logPath, 1<<20+4096) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if !capUpdaterLog(ctx, f, 1, defaultUpdaterLogBackups, false, time.Hour) { + t.Fatal("capping did not start for ~/.pilot/updater.log") + } + + backup := logPath + ".pilot.1.gz" + deadline := time.Now().Add(10 * time.Second) + for { + fi, err := os.Stat(logPath) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(backup); err == nil && fi.Size() == 0 { + break + } + if time.Now().After(deadline) { + t.Fatalf("log not rotated: size=%d", fi.Size()) + } + time.Sleep(20 * time.Millisecond) + } + + gz, err := os.Open(backup) + if err != nil { + t.Fatal(err) + } + defer gz.Close() + zr, err := gzip.NewReader(gz) + if err != nil { + t.Fatal(err) + } + got, err := io.ReadAll(zr) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, data) { + t.Fatalf("backup holds %d bytes, want the %d the log had", len(got), len(data)) + } +} + +// TestUpdaterLogElsewhereIsLeftAlone: a log an operator redirected outside +// ~/.pilot is rotated only when -log-max-size is set explicitly. +func TestUpdaterLogElsewhereIsLeftAlone(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("PILOT_HOME", "") + if err := os.MkdirAll(filepath.Join(home, ".pilot"), 0o700); err != nil { + t.Fatal(err) + } + f, _ := openLog(t, filepath.Join(t.TempDir(), "var", "updater.log"), 16) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if capUpdaterLog(ctx, f, 1, 3, false, time.Hour) { + t.Fatal("capping started for a log outside ~/.pilot without an explicit -log-max-size") + } + if !capUpdaterLog(ctx, f, 1, 3, true, time.Hour) { + t.Fatal("capping did not start for an explicitly capped log") + } +} + +// TestUpdaterLogCapDisabled: -log-max-size=0 turns the cap off. +func TestUpdaterLogCapDisabled(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + f, _ := openLog(t, filepath.Join(home, ".pilot", "updater.log"), 16) + if capUpdaterLog(context.Background(), f, 0, 3, true, time.Hour) { + t.Fatal("capping started with -log-max-size=0") + } +} + +// TestUpdaterLogDirs follows HOME and PILOT_HOME. +func TestUpdaterLogDirs(t *testing.T) { + t.Setenv("HOME", "/h") + t.Setenv("PILOT_HOME", "/p") + got := updaterLogDirs() + want := []string{filepath.Join("/h", ".pilot"), filepath.Join("/p", ".pilot")} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Fatalf("updaterLogDirs() = %v, want %v", got, want) + } +} From a81920fc12bae8bb1fa22ba040f403d9c35de411 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:34:02 +0300 Subject: [PATCH 2/4] updater: remove abandoned bundle downloads from the temp dir `pilotctl appstore install `, and `appstore upgrade`, which the updater runs every hour as `upgrade --all`, download each bundle to $TMPDIR/pilot-bundle-.tar.gz and unpack it into $TMPDIR/pilot-bundle-unpack-. The unpacked copy is never removed after the install consumes it, and the tarball survives a pilotctl killed mid-download. The updater library's own release dir, pilot-update-, survives a kill mid-download too. One laptop's $TMPDIR held 272 of them (the launchd updater shares the user's $TMPDIR). macOS purges $TMPDIR only after days without access; a Linux /tmp that is not a tmpfs is often never cleaned. The updater now sweeps them at start and every -interval, whether or not auto-update is enabled. It removes an entry only when it matches one of those three names, is the expected type (dir or regular file; never a symlink), is owned by the updater's user, and was last modified over 24 h ago (an install finishes in minutes). Directories an unpacked bundle made read-only are made owner-writable first so the removal completes. The root cause, the install not removing its unpack dir, is in cmd/pilotctl/appstore*.go and is left to a follow-up; this also clears the leftovers already on disk and those a killed pilotctl leaves. Tests: abandoned leftovers of all three names go; fresh ones, other names, wrong types and a backdated symlink (and its target) stay; another user's leftover stays (root-only); the loop sweeps $TMPDIR at once and stops when told. Mutating out the age, type or symlink check fails the test. End to end in a Linux container, the built binary removed two 3-day-old leftovers (301,000 bytes) and kept a fresh one. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/updater/main.go | 4 + cmd/updater/owner_other.go | 11 ++ cmd/updater/owner_unix.go | 17 +++ cmd/updater/tmpsweep.go | 158 +++++++++++++++++++++++++++ cmd/updater/zz_tmpsweep_test.go | 184 ++++++++++++++++++++++++++++++++ 5 files changed, 374 insertions(+) create mode 100644 cmd/updater/owner_other.go create mode 100644 cmd/updater/owner_unix.go create mode 100644 cmd/updater/tmpsweep.go create mode 100644 cmd/updater/zz_tmpsweep_test.go diff --git a/cmd/updater/main.go b/cmd/updater/main.go index 6a04297d..283a9cb7 100644 --- a/cmd/updater/main.go +++ b/cmd/updater/main.go @@ -130,6 +130,9 @@ func main() { } else { slog.Info("app auto-upgrade opted out (PILOT_APP_UPDATE_OPT_OUT); apps stay at their installed versions, pilot binaries still update") } + // Remove abandoned bundle downloads from the temp dir (tmpsweep.go). + stopSweep := make(chan struct{}) + go tempSweepLoop(*interval, stopSweep) if *pin != "" { slog.Info("version pinned", "tag", *pin) } @@ -142,6 +145,7 @@ func main() { <-sig slog.Info("shutting down") + close(stopSweep) u.Stop() } diff --git a/cmd/updater/owner_other.go b/cmd/updater/owner_other.go new file mode 100644 index 00000000..a9fcc224 --- /dev/null +++ b/cmd/updater/owner_other.go @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build !unix + +package main + +import "os" + +// ownedByCurrentUser cannot tell the owner here, so the temp sweep removes +// nothing. +func ownedByCurrentUser(os.FileInfo) bool { return false } diff --git a/cmd/updater/owner_unix.go b/cmd/updater/owner_unix.go new file mode 100644 index 00000000..dca1f58c --- /dev/null +++ b/cmd/updater/owner_unix.go @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build unix + +package main + +import ( + "os" + "syscall" +) + +// ownedByCurrentUser reports whether fi belongs to the user this process +// runs as. +func ownedByCurrentUser(fi os.FileInfo) bool { + st, ok := fi.Sys().(*syscall.Stat_t) + return ok && int(st.Uid) == os.Getuid() +} diff --git a/cmd/updater/tmpsweep.go b/cmd/updater/tmpsweep.go new file mode 100644 index 00000000..e2486281 --- /dev/null +++ b/cmd/updater/tmpsweep.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package main + +import ( + "io/fs" + "log/slog" + "os" + "path/filepath" + "time" +) + +// Stale download leftovers in the system temp directory. +// +// `pilotctl appstore install `, and `appstore upgrade`, which +// appUpgradeLoop runs every hour as `upgrade --all`, download each bundle to +// $TMPDIR/pilot-bundle-.tar.gz and unpack it into +// $TMPDIR/pilot-bundle-unpack-. The unpacked copy is not removed once the +// install has consumed it, and the tarball survives a pilotctl that is +// killed mid-download. The updater's own release download dir, +// $TMPDIR/pilot-update-, is removed when the update returns but not when +// the process is killed mid-download. One laptop's $TMPDIR held 272 of +// these. Nothing else removes them: macOS purges $TMPDIR only after days +// without access, and a Linux /tmp that is not a tmpfs is often never +// cleaned. +// +// sweepStaleTemps removes them once they are clearly abandoned. It only +// touches entries that: +// - match one of these Pilot-specific names, +// - are the expected type (a directory for the two dir patterns, a +// regular file for the tarball; symlinks never match), +// - are owned by the user the updater runs as, and +// - were last modified more than staleTempAge ago. An install or update +// finishes in minutes (pilotctl's download timeout is 60 s, the +// updater's app-upgrade run is capped at 10 minutes), so a live one is +// never near that age. + +// staleTempAge is how old a leftover must be before it is removed. +const staleTempAge = 24 * time.Hour + +// staleTempPattern is one name the sweep removes, and the type of entry it +// must be. +type staleTempPattern struct { + glob string + dir bool +} + +// staleTempPatterns are the temp names pilotctl (appstore_catalogue.go, +// fetchAndUnpackBundle) and the updater library (applyUpdate) create with +// os.CreateTemp/os.MkdirTemp in the system temp directory. Keep in step +// with them. +var staleTempPatterns = []staleTempPattern{ + {glob: "pilot-bundle-unpack-*", dir: true}, + {glob: "pilot-bundle-*.tar.gz", dir: false}, + {glob: "pilot-update-*", dir: true}, +} + +// sweepResult summarises one sweepStaleTemps run. +type sweepResult struct { + Removed int + Bytes int64 + Failed int +} + +// sweepStaleTemps removes the abandoned leftovers in dir (see above) last +// modified before now-maxAge. +func sweepStaleTemps(dir string, now time.Time, maxAge time.Duration) sweepResult { + var res sweepResult + cutoff := now.Add(-maxAge) + for _, p := range staleTempPatterns { + matches, err := filepath.Glob(filepath.Join(dir, p.glob)) + if err != nil { + continue // only ErrBadPattern, and the patterns are constant + } + for _, path := range matches { + fi, err := os.Lstat(path) + if err != nil { + continue + } + if p.dir != fi.IsDir() || (!p.dir && !fi.Mode().IsRegular()) { + continue + } + if !fi.ModTime().Before(cutoff) || !ownedByCurrentUser(fi) { + continue + } + size := treeSize(path) + if err := removeTree(path); err != nil { + res.Failed++ + slog.Warn("temp sweep: could not remove stale download leftover", "path", path, "err", err) + continue + } + res.Removed++ + res.Bytes += size + } + } + return res +} + +// treeSize is the total size of the regular files under path (path itself +// when it is a file). Best effort, for the log line only. +func treeSize(path string) int64 { + var total int64 + _ = filepath.WalkDir(path, func(_ string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + if d.Type().IsRegular() { + if info, err := d.Info(); err == nil { + total += info.Size() + } + } + return nil + }) + return total +} + +// removeTree removes path and everything under it. A bundle can unpack +// directories without write permission, whose entries os.RemoveAll then +// cannot delete, so on failure it makes every directory in the tree +// writable by its owner and tries again. WalkDir does not follow symlinks, +// so only entries inside the tree are changed. +func removeTree(path string) error { + if err := os.RemoveAll(path); err == nil { + return nil + } + _ = filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { + if err == nil && d.IsDir() { + _ = os.Chmod(p, 0o700) // #nosec G302 -- owner-only, on a dir about to be removed + } + return nil + }) + return os.RemoveAll(path) +} + +// tempSweepLoop runs sweepStaleTemps on the system temp directory now and +// then every interval, until stop is closed. It runs whether or not +// automatic updates are enabled: an interactive `pilotctl appstore install` +// leaves the same leftovers. +func tempSweepLoop(interval time.Duration, stop <-chan struct{}) { + sweep := func() { + res := sweepStaleTemps(os.TempDir(), time.Now(), staleTempAge) + if res.Removed > 0 || res.Failed > 0 { + slog.Info("temp sweep: removed stale download leftovers", + "dir", os.TempDir(), "removed", res.Removed, "bytes", res.Bytes, "failed", res.Failed) + } + } + sweep() + t := time.NewTicker(interval) + defer t.Stop() + for { + select { + case <-stop: + return + case <-t.C: + sweep() + } + } +} diff --git a/cmd/updater/zz_tmpsweep_test.go b/cmd/updater/zz_tmpsweep_test.go new file mode 100644 index 00000000..5c982507 --- /dev/null +++ b/cmd/updater/zz_tmpsweep_test.go @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build unix + +package main + +import ( + "os" + "path/filepath" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// mkTree creates path as a dir holding a file of size bytes and a +// read-only subdir with another file, the shape an unpacked bundle can +// have, then backdates it. +func mkTree(t *testing.T, path string, size int, mtime time.Time) { + t.Helper() + if err := os.MkdirAll(filepath.Join(path, "bin"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "manifest.json"), make([]byte, size), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(path, "bin", "app"), []byte("x"), 0o500); err != nil { + t.Fatal(err) + } + if err := os.Chmod(filepath.Join(path, "bin"), 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(filepath.Join(path, "bin"), 0o700) }) + if err := os.Chtimes(path, mtime, mtime); err != nil { + t.Fatal(err) + } +} + +func mkFile(t *testing.T, path string, size int, mtime time.Time) { + t.Helper() + if err := os.WriteFile(path, make([]byte, size), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(path, mtime, mtime); err != nil { + t.Fatal(err) + } +} + +func exists(path string) bool { + _, err := os.Lstat(path) + return err == nil +} + +// TestSweepRemovesOnlyAbandonedPilotDownloads: abandoned bundle downloads +// and update dirs go; anything fresh, of the wrong type, a symlink, or not +// named like ours stays. +func TestSweepRemovesOnlyAbandonedPilotDownloads(t *testing.T) { + dir := t.TempDir() + now := time.Now() + old := now.Add(-2 * staleTempAge) + fresh := now.Add(-time.Minute) + + // Removed. + mkTree(t, filepath.Join(dir, "pilot-bundle-unpack-111"), 1000, old) + mkFile(t, filepath.Join(dir, "pilot-bundle-222.tar.gz"), 500, old) + mkTree(t, filepath.Join(dir, "pilot-update-333"), 200, old) + + // Kept: an install or update still in progress. + mkTree(t, filepath.Join(dir, "pilot-bundle-unpack-444"), 10, fresh) + mkFile(t, filepath.Join(dir, "pilot-bundle-555.tar.gz"), 10, fresh) + // Kept: not our names. + mkTree(t, filepath.Join(dir, "other-unpack-666"), 10, old) + mkFile(t, filepath.Join(dir, "pilot-bundle-notes.txt"), 10, old) + // Kept: our names, wrong type. + mkFile(t, filepath.Join(dir, "pilot-bundle-unpack-777"), 10, old) + mkTree(t, filepath.Join(dir, "pilot-bundle-888.tar.gz"), 10, old) + // Kept: a symlink with our name, and what it points at. + target := filepath.Join(t.TempDir(), "precious") + mkTree(t, target, 10, old) + link := filepath.Join(dir, "pilot-bundle-unpack-999") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + // Backdate the link itself (os.Chtimes would follow it), so only its + // type keeps it. + tv := []unix.Timeval{unix.NsecToTimeval(old.UnixNano()), unix.NsecToTimeval(old.UnixNano())} + if err := unix.Lutimes(link, tv); err != nil { + t.Fatal(err) + } + + res := sweepStaleTemps(dir, now, staleTempAge) + + for _, gone := range []string{"pilot-bundle-unpack-111", "pilot-bundle-222.tar.gz", "pilot-update-333"} { + if exists(filepath.Join(dir, gone)) { + t.Errorf("%s was not removed", gone) + } + } + for _, kept := range []string{ + "pilot-bundle-unpack-444", "pilot-bundle-555.tar.gz", + "other-unpack-666", "pilot-bundle-notes.txt", + "pilot-bundle-unpack-777", "pilot-bundle-888.tar.gz", + "pilot-bundle-unpack-999", + } { + if !exists(filepath.Join(dir, kept)) { + t.Errorf("%s was removed", kept) + } + } + if !exists(filepath.Join(target, "manifest.json")) { + t.Error("the symlink's target was touched") + } + if res.Removed != 3 || res.Failed != 0 { + t.Errorf("result = %+v, want 3 removed, 0 failed", res) + } + if want := int64(1000 + 1 + 500 + 200 + 1); res.Bytes != want { + t.Errorf("bytes = %d, want %d", res.Bytes, want) + } +} + +// TestSweepEmptyDir is a no-op. +func TestSweepEmptyDir(t *testing.T) { + if res := sweepStaleTemps(t.TempDir(), time.Now(), staleTempAge); res != (sweepResult{}) { + t.Fatalf("result = %+v, want zero", res) + } +} + +// TestTempSweepLoopSweepsTempDirAndStops: the loop sweeps $TMPDIR right +// away and returns once stopped. +func TestTempSweepLoopSweepsTempDirAndStops(t *testing.T) { + dir := t.TempDir() + t.Setenv("TMPDIR", dir) + stale := filepath.Join(dir, "pilot-bundle-unpack-1") + mkTree(t, stale, 10, time.Now().Add(-2*staleTempAge)) + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + tempSweepLoop(time.Hour, stop) + close(done) + }() + deadline := time.Now().Add(5 * time.Second) + for exists(stale) { + if time.Now().After(deadline) { + t.Fatal("stale leftover not swept") + } + time.Sleep(10 * time.Millisecond) + } + close(stop) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("tempSweepLoop did not return after stop") + } +} + +func TestOwnedByCurrentUser(t *testing.T) { + p := filepath.Join(t.TempDir(), "f") + mkFile(t, p, 1, time.Now()) + fi, err := os.Lstat(p) + if err != nil { + t.Fatal(err) + } + if !ownedByCurrentUser(fi) { + t.Fatal("a file this process created is not reported as its own") + } +} + +// TestSweepKeepsOtherUsersLeftovers: on a shared /tmp another user's +// downloads are not ours to remove. Needs root to create them. +func TestSweepKeepsOtherUsersLeftovers(t *testing.T) { + if os.Getuid() != 0 { + t.Skip("needs root to create a file owned by another user") + } + dir := t.TempDir() + old := time.Now().Add(-2 * staleTempAge) + theirs := filepath.Join(dir, "pilot-bundle-unpack-1") + mkTree(t, theirs, 10, old) + if err := os.Lchown(theirs, 65534, 65534); err != nil { + t.Fatal(err) + } + _ = os.Chtimes(theirs, old, old) + if res := sweepStaleTemps(dir, time.Now(), staleTempAge); res.Removed != 0 || !exists(theirs) { + t.Fatalf("another user's leftover was removed (result %+v)", res) + } +} From 269717209ea551f0d2900df0ee7a6248b70ff8e7 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:34:04 +0300 Subject: [PATCH 3/4] tests: goroutine gate for a long-lived daemon under peer churn TestDaemonShutdownStopsGoroutines checks that Stop reaps everything; no test checked a daemon that stays up while peers come and go. TestPeerChurnDoesNotLeakGoroutines keeps one daemon running while eight short-lived peers each open a stream to its echo port, round-trip a message, exchange datagrams both ways and then shut down for good. The goroutine count after the last peer must be within rounds/2 of the count after a warm-up round. Today it holds (macOS and Linux: delta -1 after 8 peers). With one goroutine leaked per TunnelManager.AddPeer it fails (delta 15). Added to the architecture workflow's goroutine-reaping step. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/architecture.yml | 2 +- tests/zz_peer_churn_goroutine_test.go | 115 ++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 tests/zz_peer_churn_goroutine_test.go diff --git a/.github/workflows/architecture.yml b/.github/workflows/architecture.yml index 4cefd213..d858100b 100644 --- a/.github/workflows/architecture.yml +++ b/.github/workflows/architecture.yml @@ -53,4 +53,4 @@ jobs: run: go test -race -count=1 -timeout 10m -run TestConcurrentDialEncryptDecrypt ./tests/... - name: P4/P9 - goroutine reaping - run: go test ./tests -run "TestDaemonShutdown|TestStreamLifecycleGoroutines|TestPluginShutdown" + run: go test ./tests -run "TestDaemonShutdown|TestStreamLifecycleGoroutines|TestPluginShutdown|TestPeerChurnDoesNotLeakGoroutines" diff --git a/tests/zz_peer_churn_goroutine_test.go b/tests/zz_peer_churn_goroutine_test.go new file mode 100644 index 00000000..29dc3b2d --- /dev/null +++ b/tests/zz_peer_churn_goroutine_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package tests + +import ( + "context" + "io" + "runtime" + "testing" + "time" + + "github.com/pilot-protocol/common/protocol" + "github.com/pilot-protocol/pilotprotocol/pkg/daemon" +) + +// TestPeerChurnDoesNotLeakGoroutines is the running-daemon counterpart of +// TestDaemonShutdownStopsGoroutines: one long-lived daemon talks to a +// stream of short-lived peers, each of which then goes away for good, and +// the goroutine count must not grow with the number of peers it has seen. +// +// Every round starts a peer, opens a stream to its echo port and +// round-trips a message, exchanges datagrams both ways, closes the stream +// and stops the peer (drivers, plugins, daemon). A goroutine per peer or +// per connection that outlived its peer (a retx loop, a keepalive or +// per-peer timer, a dial left pending) would add up round after round. +// The baseline is taken after a warm-up round, so goroutines the +// long-lived daemon starts lazily on first contact are not counted. +func TestPeerChurnDoesNotLeakGoroutines(t *testing.T) { + if testing.Short() { + t.Skip("requires full daemon lifecycle") + } + const rounds = 8 + + env := NewTestEnv(t) + // A short TIME_WAIT so a closed stream's state is gone before the count. + a := env.AddDaemon(func(c *daemon.Config) { c.TimeWaitDuration = 100 * time.Millisecond }) + + round := func(i int) { + t.Helper() + b := env.AddDaemon() + bIdx := len(env.daemons) - 1 + + conn, err := a.Driver.DialAddrTimeout(b.Daemon.Addr(), protocol.PortEcho, 10*time.Second) + if err != nil { + t.Fatalf("round %d: dial echo: %v", i, err) + } + msg := []byte("churn") + if _, err := conn.Write(msg); err != nil { + t.Fatalf("round %d: write: %v", i, err) + } + got := make([]byte, len(msg)) + _ = conn.SetReadDeadline(time.Now().Add(10 * time.Second)) + if _, err := io.ReadFull(conn, got); err != nil { + t.Fatalf("round %d: read echo: %v", i, err) + } + _ = conn.Close() + for j := 0; j < 3; j++ { + _ = a.Daemon.SendDatagram(b.Daemon.Addr(), protocol.PortEcho, []byte("ping")) + _ = b.Daemon.SendDatagram(a.Daemon.Addr(), protocol.PortEcho, []byte("pong")) + } + time.Sleep(100 * time.Millisecond) + + stopPeer(t, env, bIdx) + } + + settle := func() int { + time.Sleep(500 * time.Millisecond) // TIME_WAIT (100ms) + FIN handling + runtime.GC() + return runtime.NumGoroutine() + } + + round(0) + baseline := settle() + t.Logf("baseline after warm-up round: %d goroutines", baseline) + + for i := 1; i <= rounds; i++ { + round(i) + } + + // Allow stragglers (plugin HTTP clients, ticker drains) to exit, as + // TestDaemonShutdownStopsGoroutines does. A per-peer leak of even one + // goroutine shows as >= rounds over baseline and trips the bound. + const slack = rounds / 2 + deadline := time.Now().Add(10 * time.Second) + var final int + for { + final = settle() + if final <= baseline+slack || time.Now().After(deadline) { + break + } + } + t.Logf("after %d more peers: %d goroutines (delta %d)", rounds, final, final-baseline) + if final > baseline+slack { + buf := make([]byte, 1<<20) + n := runtime.Stack(buf, true) + t.Fatalf("goroutines grew with peer churn: baseline=%d final=%d after %d peers\n%s", + baseline, final, rounds, buf[:n]) + } +} + +// stopPeer shuts down env's daemon idx the way env.Close does (driver, +// plugins, daemon). It stays in env's lists, so the next AddDaemon gets a +// fresh index and with it a fresh identity (a reused index would reload +// this peer's identity file and come back as the same node); env.Close +// stopping it again is a no-op. +func stopPeer(t *testing.T, env *TestEnv, idx int) { + t.Helper() + _ = env.drivers[idx].Close() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + if err := env.runtimes[idx].StopPlugins(ctx); err != nil { + t.Logf("peer %d plugin shutdown: %v", idx, err) + } + cancel() + _ = env.daemons[idx].Stop() +} From ca64cdc37d98b16978e12c7dac135cc27d9ca40c Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 10:59:36 +0300 Subject: [PATCH 4/4] updater: temp sweep removes with os.RemoveAll alone The sweep made every directory in a leftover owner-writable before a second os.RemoveAll, in case a bundle unpacked read-only directories. None do: pilotctl's untarUnder creates every directory 0755 and the updater's release staging 0755, and a read-only file does not block removal. The chmod ran inside a filepath.WalkDir callback on a path that could be swapped for a symlink mid-walk (gosec G122, CWE-367), so drop it rather than harden code that is never needed. gosec v2.29.0 on cmd/updater: 1 issue (G122, tmpsweep.go:128) before, 0 after. The test tree now has the shape bundles really unpack to (0755 dirs, a read-only executable). Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/updater/tmpsweep.go | 24 +++++------------------- cmd/updater/zz_tmpsweep_test.go | 12 ++++-------- 2 files changed, 9 insertions(+), 27 deletions(-) diff --git a/cmd/updater/tmpsweep.go b/cmd/updater/tmpsweep.go index e2486281..02068a6d 100644 --- a/cmd/updater/tmpsweep.go +++ b/cmd/updater/tmpsweep.go @@ -84,7 +84,11 @@ func sweepStaleTemps(dir string, now time.Time, maxAge time.Duration) sweepResul continue } size := treeSize(path) - if err := removeTree(path); err != nil { + // os.RemoveAll does not follow symlinks inside the tree. Every + // directory in these trees is created owner-writable (0755/0700: + // pilotctl's untarUnder, the updater's staging), so nothing in + // them blocks removal. + if err := os.RemoveAll(path); err != nil { res.Failed++ slog.Warn("temp sweep: could not remove stale download leftover", "path", path, "err", err) continue @@ -114,24 +118,6 @@ func treeSize(path string) int64 { return total } -// removeTree removes path and everything under it. A bundle can unpack -// directories without write permission, whose entries os.RemoveAll then -// cannot delete, so on failure it makes every directory in the tree -// writable by its owner and tries again. WalkDir does not follow symlinks, -// so only entries inside the tree are changed. -func removeTree(path string) error { - if err := os.RemoveAll(path); err == nil { - return nil - } - _ = filepath.WalkDir(path, func(p string, d fs.DirEntry, err error) error { - if err == nil && d.IsDir() { - _ = os.Chmod(p, 0o700) // #nosec G302 -- owner-only, on a dir about to be removed - } - return nil - }) - return os.RemoveAll(path) -} - // tempSweepLoop runs sweepStaleTemps on the system temp directory now and // then every interval, until stop is closed. It runs whether or not // automatic updates are enabled: an interactive `pilotctl appstore install` diff --git a/cmd/updater/zz_tmpsweep_test.go b/cmd/updater/zz_tmpsweep_test.go index 5c982507..96cda940 100644 --- a/cmd/updater/zz_tmpsweep_test.go +++ b/cmd/updater/zz_tmpsweep_test.go @@ -13,12 +13,12 @@ import ( "golang.org/x/sys/unix" ) -// mkTree creates path as a dir holding a file of size bytes and a -// read-only subdir with another file, the shape an unpacked bundle can -// have, then backdates it. +// mkTree creates path as a dir holding a file of size bytes and a bin/ +// subdir with a read-only executable, the shape an unpacked bundle has, +// then backdates it. func mkTree(t *testing.T, path string, size int, mtime time.Time) { t.Helper() - if err := os.MkdirAll(filepath.Join(path, "bin"), 0o700); err != nil { + if err := os.MkdirAll(filepath.Join(path, "bin"), 0o755); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(path, "manifest.json"), make([]byte, size), 0o600); err != nil { @@ -27,10 +27,6 @@ func mkTree(t *testing.T, path string, size int, mtime time.Time) { if err := os.WriteFile(filepath.Join(path, "bin", "app"), []byte("x"), 0o500); err != nil { t.Fatal(err) } - if err := os.Chmod(filepath.Join(path, "bin"), 0o500); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = os.Chmod(filepath.Join(path, "bin"), 0o700) }) if err := os.Chtimes(path, mtime, mtime); err != nil { t.Fatal(err) }