Skip to content

updater: cap updater.log, sweep abandoned bundle downloads; peer-churn goroutine gate - #472

Open
TeoSlayer wants to merge 4 commits into
mainfrom
fix/cleanup-gaps
Open

TeoSlayer wants to merge 4 commits into
mainfrom
fix/cleanup-gaps

Conversation

@TeoSlayer

Copy link
Copy Markdown
Collaborator

Closes two cleanup gaps found in a sweep of Pilot's on-disk and process leftovers, and adds a regression gate. It touches only cmd/updater/, one new file in tests/, and one line of .github/workflows/architecture.yml. No file here is touched by #470, #465, #464 or #467.

1. updater.log is now capped (like daemon.log)

install.sh's launchd job writes the updater's stdout and stderr to ~/.pilot/updater.log. launchd never rotates that file. On one laptop it held every line since the June install: 2,205 lines, 310 KB. Each failed GitHub check (API rate limit exceeded) adds a ~400-byte line an hour.

The updater now runs internal/logcap on its own stderr, the same way the daemon does for daemon.log (#464):

  • It copy-truncates into gzipped updater.log.pilot.N.gz.
  • New flags: -log-max-size (MB, default 10, 0 disables) and -log-max-backups (default 3).
  • By default only a log inside ~/.pilot (or $PILOT_HOME/.pilot) is capped. A log elsewhere is capped only when -log-max-size is set explicitly.
  • Under systemd it's a no-op, because stderr goes to journald.

2. Abandoned bundle downloads are removed from the temp dir

pilotctl appstore install <catalogue id> and appstore upgrade both do this: the updater runs upgrade --all hourly. They download to $TMPDIR/pilot-bundle-<n>.tar.gz and unpack into $TMPDIR/pilot-bundle-unpack-<n>, and the unpacked copy is never removed after the install. The tarball also survives a pilotctl killed mid-download, and the updater library's pilot-update-<n> survives a kill mid-download. One laptop had 265-272 of these. Most came from cmd/pilotctl test runs, which leak about 9 per run into the real $TMPDIR; one came from a real install on Sep 22.

The updater now sweeps them at start and on every -interval. It removes an entry only when all of these hold:

  • it matches one of those three names;
  • it is the expected type (a dir or a regular file, never a symlink);
  • it is owned by the updater's user;
  • it was last modified more than 24 h ago.

The launchd updater shares the user's per-user $TMPDIR (checked on the live process), so it sees interactive leftovers too.

The root cause, the install not removing its unpack dir, lives in cmd/pilotctl/appstore*.go, which this PR stays out of. A tested patch is ready for a follow-up:

  • it removes the unpack dir after a catalogue install and on a failed unpack;
  • it never removes a sideload's source dir;
  • it isolates the one leaking test.

With that patch the appstore pilotctl tests add 0 temp dirs per run, against 9 on main. This sweep is still needed after that fix: fatalHint exits the process, which skips deferred cleanup, and it clears what is already on disk.

3. Goroutine gate for peer churn

TestPeerChurnDoesNotLeakGoroutines keeps one daemon up while 8 short-lived peers each:

  • open a stream to its echo port and round-trip a message;
  • exchange datagrams both ways;
  • shut down.

The goroutine count must stay within rounds/2 of a post-warm-up baseline.

  • Today: delta -1 after 8 peers, on macOS and Linux.
  • With one goroutine leaked per TunnelManager.AddPeer: delta 15, fails.

It's added to the architecture workflow's "goroutine reaping" step and takes ~40 s.

Verification

  • go test ./cmd/updater passes on macOS arm64, and on Linux arm64 as root and as uid 1000. The root-only case checks that another user's leftover is kept.
  • Mutation checks: removing the sweep's age, type or symlink check fails TestSweepRemovesOnlyAbandonedPilotDownloads.
  • End to end, the built pilot-updater was run in a Linux container with stderr appended to ~/.pilot/updater.log (11 MB):
    • log after start: 236 bytes, plus updater.log.pilot.1.gz;
    • temp sweep: removed stale download leftovers removed=2 bytes=301000;
    • a fresh unpack dir was kept.
  • check-layers: OK: layered architecture clean. go vet is clean.

Related, outside this repo / this PR

🤖 Generated with Claude Code

teovl and others added 3 commits September 24, 2026 10:33
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) <noreply@anthropic.com>
`pilotctl appstore install <catalogue id>`, and `appstore upgrade`,
which the updater runs every hour as `upgrade --all`, download each
bundle to $TMPDIR/pilot-bundle-<n>.tar.gz and unpack it into
$TMPDIR/pilot-bundle-unpack-<n>. 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-<n>,
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Comment thread cmd/updater/tmpsweep.go Fixed
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) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants