From 633a0e186ed7434311b6b35cdb685b5d0c741980 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:11:10 -0400 Subject: [PATCH 1/4] fix(maintenance): make disk-cleanup reclaim what actually fills a CI box MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit nSelf staging hit 100% disk on 2026-09-11 while nself-disk-cleanup.timer ran daily: DiskCleanup() only pruned docker (unsafely, with -af) and vacuumed 7-day-old journal logs, neither of which touches what actually grows on a runner farm (runner _work job dirs, go-build/module caches, pnpm stores). A wrapper's global `pgrep -f Runner.Worker` busy check also meant one busy runner out of four skipped cleanup for all four. - Reclaim GitHub Actions runner job workspaces under "/_work", discovered via systemd unit introspection with a glob fallback and an env var override (NSELF_MAINTENANCE_RUNNER_ROOTS). _actions/_tool/_temp/ _PipelineMapping are never touched — deleting _actions under a live job is what caused the incident. - Reclaim go/pkg/mod, pnpm-store, and ~/.cache (minus go-build/grype/ trivy, which a running compile or scan reads live). - Replace `docker system prune -af --volumes=false` with dangling-image prune, build-cache prune, and anonymous-volume-only prune (64-hex names via filterAnonymousVolumes) — this can no longer remove an image a stopped stack container depends on, unlike the command that caused the 3-day ntask Hasura outage. - Detect busy state per runner root (worker process path contains that runner's own directory) instead of globally. - Add a disk-pressure threshold (default 85%, overridable) above which the idle-preferred shared-cache tier runs regardless of busy state — runner job workspaces stay idle-gated at every tier since deleting one breaks its job outright. - CleanupResult now carries BytesReclaimed/Reclaimed/Skipped so a timer run is diagnosable, plus a DryRun mode via DiskCleanupDryRun(). - Keep the Windows build green with a parallel, guarded implementation that gets the same docker-prune fix; runner-farm reclaim is POSIX-only. --- internal/maintenance/disk.go | 184 ++++++++- internal/maintenance/disk_shared.go | 152 +++++++- internal/maintenance/disk_windows.go | 55 ++- .../maintenance/runner_discovery_posix.go | 95 +++++ internal/maintenance/runner_posix.go | 222 +++++++++++ internal/maintenance/runner_posix_test.go | 349 ++++++++++++++++++ 6 files changed, 1031 insertions(+), 26 deletions(-) create mode 100644 internal/maintenance/runner_discovery_posix.go create mode 100644 internal/maintenance/runner_posix.go create mode 100644 internal/maintenance/runner_posix_test.go diff --git a/internal/maintenance/disk.go b/internal/maintenance/disk.go index 6e3fe80dc..e24af20c3 100644 --- a/internal/maintenance/disk.go +++ b/internal/maintenance/disk.go @@ -4,9 +4,17 @@ package maintenance import ( "fmt" + "os" + "path/filepath" "syscall" ) +// DefaultPressureThreshold is the disk-used percentage at/above which idle-shared +// cache reclaims run regardless of runner busy state. A full disk fails every job on +// the box anyway, so waiting for idle past this point is strictly worse than +// reclaiming now. +const DefaultPressureThreshold = 85 + // GetDiskUsage returns current disk utilisation for the root filesystem ("/"). func GetDiskUsage() (DiskUsage, error) { var stat syscall.Statfs_t @@ -37,40 +45,154 @@ func GetDiskUsage() (DiskUsage, error) { }, nil } -// DiskCleanup runs all three cleanup steps and returns a summary. -// It never aborts early — it collects all errors and reports at the end. +// DiskCleanupOptions configures a DiskCleanup run. +type DiskCleanupOptions struct { + // DryRun reports what would be removed and the space it would free, without + // removing anything. + DryRun bool + // PressureThreshold is the disk-used percentage at/above which idle-shared + // reclaims run even while a runner is busy. Zero means DefaultPressureThreshold. + PressureThreshold int + // Home overrides the home directory used to locate caches. Defaults to $HOME + // (falling back to os.UserHomeDir()). Tests inject this to point at a fixture + // tree instead of the real user's home. + Home string + // RunnerRoots overrides runner discovery. Tests inject this to point at fixture + // runner trees instead of discovering real installs via systemd/globs. + RunnerRoots []RunnerRoot + // SharedCacheRoots overrides the absolute (non-home-relative) cache locations + // considered for reclaim (default: sharedCacheRoots, e.g. "/opt/pnpm-store"). + // Tests set this to an empty (non-nil) slice to guarantee nothing outside a + // fixture tree is ever touched; nil means "use the default list". + SharedCacheRoots []string + // UsageOverride, when non-nil, is used instead of calling GetDiskUsage() for the + // "before" reading that pressure-escalation compares against threshold. Tests use + // this for deterministic threshold behavior instead of depending on the test + // machine's real, unpredictable disk usage. + UsageOverride *DiskUsage +} + +// DiskCleanup runs the full cleanup with default options: not a dry run, the default +// pressure threshold, real runner discovery, and $HOME for caches. func DiskCleanup() CleanupResult { - result := CleanupResult{} + return DiskCleanupWithOptions(DiskCleanupOptions{}) +} - before, err := GetDiskUsage() - if err != nil { - result.Errors = append(result.Errors, fmt.Errorf("read disk usage (before): %w", err)) +// DiskCleanupDryRun runs the full cleanup in dry-run mode: nothing is removed, but the +// returned CleanupResult's Reclaimed/Skipped/BytesReclaimed report exactly what a real +// run would have done and why anything was left alone. +func DiskCleanupDryRun() CleanupResult { + return DiskCleanupWithOptions(DiskCleanupOptions{DryRun: true}) +} + +// DiskCleanupWithOptions runs disk-cleanup with explicit options. It never aborts +// early — it collects all errors/skips and reports at the end, tier by tier: +// +// 1. tierAlways — docker dangling image/build-cache/anonymous-volume prune, old +// compressed log rotation, journald vacuum. Runs unconditionally. +// 2. tierIdlePerRunner — GitHub Actions runner job workspace directories under +// "/_work", one runner root at a time. Only runs for a runner root that +// isRunnerBusy reports idle; a busy runner's workspace is always left alone, +// pressure or not, because deleting an in-progress job's own checkout breaks +// that job outright. +// 3. tierIdleShared — regenerable package/module caches (go build cache excluded — +// see protectedCacheSubpaths). Prefers every runner being idle, but runs anyway +// once disk usage is at/above PressureThreshold. +// +// protectedRunnerSubdirs and protectedCacheSubpaths are excluded at every tier, +// unconditionally — see their doc comments in runner_posix.go for the incidents that +// made them hard exclusions rather than a "prefer not to" default. +func DiskCleanupWithOptions(opts DiskCleanupOptions) CleanupResult { + result := CleanupResult{DryRun: opts.DryRun} + + threshold := opts.PressureThreshold + if threshold <= 0 { + threshold = DefaultPressureThreshold + } + + home := opts.Home + if home == "" { + home = os.Getenv("HOME") + } + if home == "" { + if h, err := os.UserHomeDir(); err == nil { + home = h + } + } + + var before DiskUsage + if opts.UsageOverride != nil { + before = *opts.UsageOverride + } else { + b, err := GetDiskUsage() + if err != nil { + result.Errors = append(result.Errors, fmt.Errorf("read disk usage (before): %w", err)) + } + before = b } result.Before = before - // 1. Docker prune: images + containers, keep volumes. - dockerOut, dockerErr := runCommand("docker", "system", "prune", "-af", "--volumes=false") + underPressure := before.UsedPercent >= threshold + + // Tier: always safe. + dockerOut, dockerErrs := dockerReclaimFunc(opts.DryRun) result.DockerPruneOut = dockerOut - if dockerErr != nil { - result.Errors = append(result.Errors, fmt.Errorf("docker prune: %w", dockerErr)) - } + result.Errors = append(result.Errors, dockerErrs...) - // 2. Log rotation: delete compressed logs older than 14 days. - logOut, logErr := runCommand("find", "/var/log", "-name", "*.gz", "-mtime", "+14", "-delete") + logOut, logErr := logRotationFunc(opts.DryRun) result.LogRotationOut = logOut if logErr != nil { // non-fatal — /var/log may not exist on all platforms result.Errors = append(result.Errors, fmt.Errorf("log rotation: %w", logErr)) } - // 3. Journalctl vacuum (Linux only; harmless no-op on macOS). - journalOut, journalErr := runCommand("journalctl", "--vacuum-time=7d") + journalOut, journalErr := journalVacuumFunc(opts.DryRun) result.JournalVacuumOut = journalOut if journalErr != nil { - // non-fatal on macOS + // non-fatal on macOS (no journald) result.Errors = append(result.Errors, fmt.Errorf("journalctl vacuum: %w", journalErr)) } + // Tier: per-runner idle-gated job workspaces. + roots := opts.RunnerRoots + if roots == nil { + roots = discoverRunnerRoots() + } + anyBusy := false + for _, root := range roots { + if isRunnerBusy(root.Path) { + anyBusy = true + result.Skipped = append(result.Skipped, SkipEntry{ + Path: root.Path, + Reason: "runner busy (Runner.Worker running) — job workspace left alone", + }) + continue + } + b, reclaimed, skipped := reclaimRunnerWork(root.Path, opts.DryRun) + result.BytesReclaimed += b + result.Reclaimed = append(result.Reclaimed, reclaimed...) + result.Skipped = append(result.Skipped, skipped...) + } + + // Tier: shared idle-preferred caches, escalated by disk pressure. + if home != "" { + if !anyBusy || underPressure { + sharedRoots := opts.SharedCacheRoots + if sharedRoots == nil { + sharedRoots = sharedCacheRoots + } + b, reclaimed, skipped := reclaimCaches(home, sharedRoots, opts.DryRun) + result.BytesReclaimed += b + result.Reclaimed = append(result.Reclaimed, reclaimed...) + result.Skipped = append(result.Skipped, skipped...) + } else { + result.Skipped = append(result.Skipped, SkipEntry{ + Path: filepath.Join(home, ".cache"), + Reason: "runner(s) busy and disk usage below pressure threshold — shared caches left alone", + }) + } + } + after, err := GetDiskUsage() if err != nil { result.Errors = append(result.Errors, fmt.Errorf("read disk usage (after): %w", err)) @@ -79,3 +201,33 @@ func DiskCleanup() CleanupResult { return result } + +// logRotationFunc and journalVacuumFunc are vars (not plain function calls) so tests +// can stub them out — disk-cleanup tests must never shell out to real `find`/ +// `journalctl` against the test box's actual /var/log or journald state. +var ( + logRotationFunc = logRotation + journalVacuumFunc = journalVacuum +) + +// logRotation deletes compressed logs older than 14 days under /var/log. In dry-run +// mode it lists what would be deleted (via `find` without `-delete`) instead. +func logRotation(dryRun bool) (string, error) { + if dryRun { + out, err := runCommand("find", "/var/log", "-name", "*.gz", "-mtime", "+14") + return "would delete:\n" + out, err + } + return runCommand("find", "/var/log", "-name", "*.gz", "-mtime", "+14", "-delete") +} + +// journalVacuum runs `journalctl --vacuum-time=7d` (Linux only; harmless no-op on +// macOS, where the command doesn't exist and the resulting error is treated as +// non-fatal by the caller). In dry-run mode it does nothing — journalctl has no +// built-in dry-run, and vacuuming is already always-safe, so there's nothing +// meaningful to preview. +func journalVacuum(dryRun bool) (string, error) { + if dryRun { + return "dry-run: journalctl --vacuum-time=7d (skipped, always-safe tier)", nil + } + return runCommand("journalctl", "--vacuum-time=7d") +} diff --git a/internal/maintenance/disk_shared.go b/internal/maintenance/disk_shared.go index c72e13a0c..edfd60234 100644 --- a/internal/maintenance/disk_shared.go +++ b/internal/maintenance/disk_shared.go @@ -3,6 +3,7 @@ package maintenance import ( "fmt" "os/exec" + "regexp" "strings" ) @@ -18,14 +19,64 @@ type DiskUsage struct { FreeGB float64 } +// reclaimTier labels which safety tier a reclaim (or would-be reclaim) belongs to. +// See disk.go's DiskCleanupWithOptions for how each tier is gated. +type reclaimTier string + +const ( + // tierAlways reclaims are safe unconditionally: busy or idle, under pressure or + // not. Nothing a running job reads lives here (docker dangling images/build + // cache/anonymous volumes, old compressed logs, journald history). + tierAlways reclaimTier = "always" + // tierIdlePerRunner reclaims (runner job workspaces) only run for a runner root + // that is individually idle. Never escalated by disk pressure — deleting an + // in-progress job's own checkout would break that job outright, the same failure + // mode as deleting _actions. + tierIdlePerRunner reclaimTier = "idle-per-runner" + // tierIdleShared reclaims (regenerable package/module caches shared across + // runners) prefer global idle, but escalate to run regardless of busy state once + // disk usage crosses the pressure threshold — a full disk fails every job on the + // box, so waiting for idle at that point is strictly worse. + tierIdleShared reclaimTier = "idle-shared" +) + +// ReclaimEntry records one thing DiskCleanup removed (or, in dry-run mode, would +// remove). +type ReclaimEntry struct { + Path string + Bytes int64 + Tier reclaimTier +} + +// SkipEntry records one thing DiskCleanup deliberately left alone, and why. This is +// what makes a `disk-cleanup` timer run diagnosable after the fact — "ran and found +// nothing to do" and "refused to touch anything because everything was busy" used to +// be indistinguishable in the log. +type SkipEntry struct { + Path string + Reason string +} + // CleanupResult summarises what disk-cleanup did. type CleanupResult struct { - Before DiskUsage - After DiskUsage + Before DiskUsage + After DiskUsage + + // DryRun is true when this result came from a dry-run — Reclaimed lists what + // WOULD have been removed and Bytes are estimates; nothing was actually deleted. + DryRun bool + + // BytesReclaimed is the total size of everything actually removed (0 for a + // dry-run's real disk impact, but Reclaimed still lists the would-be total). + BytesReclaimed int64 + Reclaimed []ReclaimEntry + Skipped []SkipEntry + DockerPruneOut string LogRotationOut string JournalVacuumOut string - Errors []error + + Errors []error } // runCommand executes a command and returns combined stdout+stderr output and any error. @@ -38,3 +89,98 @@ func runCommand(name string, args ...string) (string, error) { } return trimmed, nil } + +// hexVolumeNameRe matches Docker's auto-generated anonymous volume names (64 lowercase +// hex characters). Named volumes such as "ntask_data" never match. +var hexVolumeNameRe = regexp.MustCompile(`^[0-9a-f]{64}$`) + +// filterAnonymousVolumes returns only the entries in names that look like Docker +// anonymous volume IDs. It is the safety gate behind dockerReclaim's volume cleanup: +// `docker volume prune` alone removes ALL unused volumes, named or not, so anything +// that isn't hex-named is never even considered for removal here. +func filterAnonymousVolumes(names []string) []string { + out := make([]string, 0, len(names)) + for _, n := range names { + n = strings.TrimSpace(n) + if n != "" && hexVolumeNameRe.MatchString(n) { + out = append(out, n) + } + } + return out +} + +// dockerReclaim replaces the old `docker system prune -af --volumes=false`, which is +// unsafe on any host running an nself stack: `-a` removes ALL unused images — +// including ones only a *stopped* stack container still depends on. A routine run of +// that exact command previously deleted the images backing ntask's postgres/redis +// containers and caused a 3-day Hasura outage, recovered only via `nself restart +// ` plus an nginx reload. +// +// This version can only ever remove: +// - dangling (untagged, "") images — never referenced by any container, +// stopped or running; +// - the build cache — regenerable, holds no runtime data; +// - anonymous dangling volumes (64-hex generated names, via filterAnonymousVolumes) +// — never a named volume like "*_data" that a stack service mounts. +// +// dockerReclaimFunc is a var (not a plain function call) so tests can stub out real +// docker invocations entirely — disk-cleanup tests must never shell out to a real +// docker daemon. +var dockerReclaimFunc = dockerReclaim + +func dockerReclaim(dryRun bool) (out string, errs []error) { + var b strings.Builder + + if dryRun { + imgOut, _ := runCommand("docker", "images", "-f", "dangling=true", "-q") + imgCount := len(nonEmptyLines(imgOut)) + fmt.Fprintf(&b, "would remove %d dangling image(s)\n", imgCount) + + volOut, _ := runCommand("docker", "volume", "ls", "-q", "-f", "dangling=true") + anon := filterAnonymousVolumes(strings.Split(volOut, "\n")) + fmt.Fprintf(&b, "would remove %d anonymous dangling volume(s)\n", len(anon)) + fmt.Fprintf(&b, "would prune docker build cache\n") + return b.String(), nil + } + + imgOut, imgErr := runCommand("docker", "image", "prune", "-f") + b.WriteString(imgOut) + b.WriteString("\n") + if imgErr != nil { + errs = append(errs, fmt.Errorf("docker image prune: %w", imgErr)) + } + + buildOut, buildErr := runCommand("docker", "builder", "prune", "-f") + b.WriteString(buildOut) + b.WriteString("\n") + if buildErr != nil { + errs = append(errs, fmt.Errorf("docker builder prune: %w", buildErr)) + } + + volListOut, volListErr := runCommand("docker", "volume", "ls", "-q", "-f", "dangling=true") + if volListErr != nil { + errs = append(errs, fmt.Errorf("docker volume ls: %w", volListErr)) + } else { + removed := 0 + for _, name := range filterAnonymousVolumes(strings.Split(volListOut, "\n")) { + if _, err := runCommand("docker", "volume", "rm", name); err != nil { + errs = append(errs, fmt.Errorf("docker volume rm %s: %w", name, err)) + continue + } + removed++ + } + fmt.Fprintf(&b, "removed %d anonymous dangling volume(s)\n", removed) + } + + return b.String(), errs +} + +func nonEmptyLines(s string) []string { + var out []string + for _, l := range strings.Split(s, "\n") { + if strings.TrimSpace(l) != "" { + out = append(out, l) + } + } + return out +} diff --git a/internal/maintenance/disk_windows.go b/internal/maintenance/disk_windows.go index 7e13abcea..cc9993e7a 100644 --- a/internal/maintenance/disk_windows.go +++ b/internal/maintenance/disk_windows.go @@ -39,17 +39,58 @@ func GetDiskUsage() (DiskUsage, error) { }, nil } -// DiskCleanup is a stub on Windows — only Docker prune is attempted. +// DiskCleanupOptions configures a DiskCleanup run. RunnerRoots/Home/PressureThreshold +// are accepted for API parity with the POSIX build but are no-ops here — GitHub +// Actions runner-farm reclaim (job workspaces, go-build/grype/trivy exclusions) is +// POSIX-shaped and guarded out of the Windows build entirely rather than partially +// reimplemented against an untested Windows process/path model. Only the docker-prune +// safety fix applies on both platforms. +type DiskCleanupOptions struct { + DryRun bool + PressureThreshold int + Home string + RunnerRoots []RunnerRoot + SharedCacheRoots []string + UsageOverride *DiskUsage +} + +// RunnerRoot mirrors the POSIX type for API parity; unused on Windows. +type RunnerRoot struct { + Name string + Path string +} + +// DiskCleanup runs the full cleanup with default options. func DiskCleanup() CleanupResult { - result := CleanupResult{} - before, _ := GetDiskUsage() + return DiskCleanupWithOptions(DiskCleanupOptions{}) +} + +// DiskCleanupDryRun runs the cleanup in dry-run mode. +func DiskCleanupDryRun() CleanupResult { + return DiskCleanupWithOptions(DiskCleanupOptions{DryRun: true}) +} + +// DiskCleanupWithOptions runs the same docker-prune safety fix as the POSIX build +// (see dockerReclaim in disk_shared.go). Runner-farm and cache reclaim are not +// attempted on Windows and are reported as skipped for diagnosability. +func DiskCleanupWithOptions(opts DiskCleanupOptions) CleanupResult { + result := CleanupResult{DryRun: opts.DryRun} + var before DiskUsage + if opts.UsageOverride != nil { + before = *opts.UsageOverride + } else { + before, _ = GetDiskUsage() + } result.Before = before - dockerOut, dockerErr := runCommand("docker", "system", "prune", "-af", "--volumes=false") + dockerOut, dockerErrs := dockerReclaimFunc(opts.DryRun) result.DockerPruneOut = dockerOut - if dockerErr != nil { - result.Errors = append(result.Errors, fmt.Errorf("docker prune: %w", dockerErr)) - } + result.Errors = append(result.Errors, dockerErrs...) + + result.Skipped = append(result.Skipped, SkipEntry{ + Path: "runner workspaces / build caches", + Reason: "runner-farm reclaim is not implemented on windows", + }) after, _ := GetDiskUsage() result.After = after diff --git a/internal/maintenance/runner_discovery_posix.go b/internal/maintenance/runner_discovery_posix.go new file mode 100644 index 000000000..053b10a29 --- /dev/null +++ b/internal/maintenance/runner_discovery_posix.go @@ -0,0 +1,95 @@ +//go:build darwin || linux + +package maintenance + +import ( + "os" + "path/filepath" + "strings" +) + +// runnerRootsEnvVar overrides discovery entirely with an explicit, colon-separated +// list of runner install directories. Useful when systemd unit naming doesn't match +// the default convention, or when systemctl isn't usable (containers, restricted +// environments). +const runnerRootsEnvVar = "NSELF_MAINTENANCE_RUNNER_ROOTS" + +// defaultRunnerRootGlobs are the conventional self-hosted runner install locations +// used across the nself fleet. Documented here rather than hardcoded to one path so a +// box with runners under /opt, or under a non-"runner" username, is still discovered +// even without systemd unit introspection. +var defaultRunnerRootGlobs = []string{ + "/home/*/actions-runner*", + "/home/*/*/actions-runner*", + "/opt/actions-runner*", +} + +// discoverRunnerRoots finds installed GitHub Actions self-hosted runners. It prefers +// systemd unit discovery (unit names of the form "actions.runner.*"), which is +// accurate even for non-default install paths, and falls back to +// defaultRunnerRootGlobs when systemctl is unavailable or returns nothing (macOS dev +// boxes, containers). NSELF_MAINTENANCE_RUNNER_ROOTS, when set, short-circuits both. +func discoverRunnerRoots() []RunnerRoot { + if override := os.Getenv(runnerRootsEnvVar); override != "" { + var roots []RunnerRoot + for _, p := range strings.Split(override, ":") { + p = strings.TrimSpace(p) + if p == "" { + continue + } + roots = append(roots, RunnerRoot{Name: filepath.Base(p), Path: p}) + } + return roots + } + + if roots := discoverRunnerRootsFromSystemd(); len(roots) > 0 { + return roots + } + return discoverRunnerRootsFromGlobs() +} + +func discoverRunnerRootsFromSystemd() []RunnerRoot { + out, err := runCommand("systemctl", "list-units", "--type=service", "--all", "--no-legend", "--plain") + if err != nil { + return nil + } + var roots []RunnerRoot + for _, line := range strings.Split(out, "\n") { + fields := strings.Fields(line) + if len(fields) == 0 { + continue + } + unit := fields[0] + if !strings.HasPrefix(unit, "actions.runner.") { + continue + } + wd, err := runCommand("systemctl", "show", unit, "-p", "WorkingDirectory", "--value") + if err != nil { + continue + } + wd = strings.TrimSpace(wd) + if wd == "" { + continue + } + roots = append(roots, RunnerRoot{Name: unit, Path: wd}) + } + return roots +} + +func discoverRunnerRootsFromGlobs() []RunnerRoot { + var roots []RunnerRoot + for _, pattern := range defaultRunnerRootGlobs { + matches, err := filepath.Glob(pattern) + if err != nil { + continue + } + for _, m := range matches { + info, err := os.Stat(m) + if err != nil || !info.IsDir() { + continue + } + roots = append(roots, RunnerRoot{Name: filepath.Base(m), Path: m}) + } + } + return roots +} diff --git a/internal/maintenance/runner_posix.go b/internal/maintenance/runner_posix.go new file mode 100644 index 000000000..faf9e8b7f --- /dev/null +++ b/internal/maintenance/runner_posix.go @@ -0,0 +1,222 @@ +//go:build darwin || linux + +package maintenance + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// RunnerRoot describes one discovered GitHub Actions self-hosted runner install. +type RunnerRoot struct { + // Name is the systemd unit name when discovered via systemd, or the directory + // basename when discovered via the fallback glob list. + Name string + // Path is the runner's install directory (what the runner config calls "work + // folder parent" — it contains _work, _diag, bin, etc.). + Path string +} + +// protectedRunnerSubdirs are runner-internal directories under "/_work" that +// must NEVER be removed by disk-cleanup, at any tier, busy or idle, under pressure or +// not. actions/runner re-downloads _actions and _tool lazily only when the directory +// is entirely missing at job start — deleting _actions out from under a *live* job +// (2026-09-11 incident: four runners on nSelf staging hit 100% disk; a naive cleanup +// wrapper race-deleted runner-3's _actions mid-job and it died mid-self-update, +// stalling every queued CI check org-wide) breaks that job hard, mid-run, with no +// chance to recover. _temp and _PipelineMapping are the same category of runner state. +var protectedRunnerSubdirs = map[string]bool{ + "_actions": true, + "_tool": true, + "_temp": true, + "_PipelineMapping": true, +} + +// protectedCacheSubpaths (relative to a runner user's $HOME/.cache) must never be +// removed while ANY runner on the box is busy, at any tier including the disk-pressure +// escalation tier — because these are read live during an in-flight command, not just +// at job start: +// - go-build: removing it mid-compile produced +// "could not import fmt (open .../go-build/...)" and broke plugins-pro#114. +// - grype, trivy: removing their DB mid-scan produced "database does not exist" and +// broke plugins-pro#113's SBOM step. +var protectedCacheSubpaths = []string{ + filepath.Join(".cache", "go-build"), + filepath.Join(".cache", "grype"), + filepath.Join(".cache", "trivy"), +} + +func isProtectedCachePath(relPath string) bool { + for _, p := range protectedCacheSubpaths { + if relPath == p { + return true + } + } + return false +} + +// dirSize returns the total size in bytes of all regular files under root. It is +// best-effort: unreadable entries are skipped rather than aborting the walk, since a +// cleanup pass should never fail just because it couldn't stat one stale file. +func dirSize(root string) int64 { + var total int64 + _ = filepath.Walk(root, func(_ string, info os.FileInfo, err error) error { + if err != nil || info == nil || info.IsDir() { + return nil //nolint:nilerr + } + total += info.Size() + return nil + }) + return total +} + +// reclaimRunnerWork removes job checkout directories under "/_work" for one +// runner — the caller (DiskCleanupWithOptions) has already established that this +// specific runner root is idle before calling this. It walks only direct children of +// _work and skips protectedRunnerSubdirs individually, so a mis-named or unexpected +// entry never causes the whole _work tree to be skipped or removed wholesale. +func reclaimRunnerWork(root string, dryRun bool) (bytes int64, reclaimed []ReclaimEntry, skipped []SkipEntry) { + workDir := filepath.Join(root, "_work") + entries, err := os.ReadDir(workDir) + if err != nil { + return 0, nil, nil // no _work dir yet (fresh install) — nothing to do + } + + for _, e := range entries { + if !e.IsDir() { + continue + } + name := e.Name() + full := filepath.Join(workDir, name) + + if protectedRunnerSubdirs[name] { + skipped = append(skipped, SkipEntry{ + Path: full, + Reason: "runner-internal directory, never removed (_actions/_tool/_temp/_PipelineMapping)", + }) + continue + } + + size := dirSize(full) + if !dryRun { + if err := os.RemoveAll(full); err != nil { + skipped = append(skipped, SkipEntry{Path: full, Reason: fmt.Sprintf("remove failed: %v", err)}) + continue + } + } + bytes += size + reclaimed = append(reclaimed, ReclaimEntry{Path: full, Bytes: size, Tier: tierIdlePerRunner}) + } + return bytes, reclaimed, skipped +} + +// reclaimCaches removes regenerable build/package caches under home, plus any +// sharedRoots that exist on this box. It never descends into protectedCacheSubpaths — +// see that var's doc comment. sharedRoots is a parameter (rather than always reading +// the package-level sharedCacheRoots) so tests can pass an empty list and guarantee +// nothing outside the test's own fixture tree is ever touched. +func reclaimCaches(home string, sharedRoots []string, dryRun bool) (bytes int64, reclaimed []ReclaimEntry, skipped []SkipEntry) { + // .cache is walked one level at a time so protected subdirectories can be + // skipped individually instead of skipping the whole tree. + cacheDir := filepath.Join(home, ".cache") + if entries, err := os.ReadDir(cacheDir); err == nil { + for _, e := range entries { + full := filepath.Join(cacheDir, e.Name()) + rel := filepath.Join(".cache", e.Name()) + if isProtectedCachePath(rel) { + skipped = append(skipped, SkipEntry{ + Path: full, + Reason: "compiler/scanner input, never removed (go-build/grype/trivy)", + }) + continue + } + b, r, s := reclaimPath(full, tierIdleShared, dryRun) + bytes += b + reclaimed = append(reclaimed, r...) + skipped = append(skipped, s...) + } + } + + // Module/package caches: regenerate automatically on next fetch, so they are + // safe to remove wholesale (unlike the compiler/scanner caches above, which are + // read live mid-command). + for _, rel := range []string{filepath.Join("go", "pkg", "mod"), "pnpm-store"} { + full := filepath.Join(home, rel) + b, r, s := reclaimPath(full, tierIdleShared, dryRun) + bytes += b + reclaimed = append(reclaimed, r...) + skipped = append(skipped, s...) + } + + for _, full := range sharedRoots { + b, r, s := reclaimPath(full, tierIdleShared, dryRun) + bytes += b + reclaimed = append(reclaimed, r...) + skipped = append(skipped, s...) + } + + return bytes, reclaimed, skipped +} + +// sharedCacheRoots are absolute (non-home-relative) cache locations also safe to +// reclaim under the same rules as reclaimCaches' home-relative targets. +var sharedCacheRoots = []string{ + "/opt/pnpm-store", +} + +// reclaimPath removes one path in full if it exists and is a directory, or reports it +// as skipped on stat/remove failure. Missing paths are simply omitted (not an error — +// not every box has every cache). +func reclaimPath(full string, tier reclaimTier, dryRun bool) (int64, []ReclaimEntry, []SkipEntry) { + info, err := os.Stat(full) + if err != nil || !info.IsDir() { + return 0, nil, nil + } + size := dirSize(full) + if !dryRun { + if err := os.RemoveAll(full); err != nil { + return 0, nil, []SkipEntry{{Path: full, Reason: fmt.Sprintf("remove failed: %v", err)}} + } + } + return size, []ReclaimEntry{{Path: full, Bytes: size, Tier: tier}}, nil +} + +// listRunnerWorkerProcesses returns the full command-line of every currently running +// process on the box, one process per element. It is a var so tests can replace it +// with a fixture instead of shelling out to `ps` — busy detection must be testable +// without a real runner process anywhere near the test box. +var listRunnerWorkerProcesses = func() ([]string, error) { + out, err := runCommand("ps", "-eo", "args=") + if err != nil { + return nil, err + } + return strings.Split(out, "\n"), nil +} + +// isRunnerBusy reports whether the given runner root currently has a live +// Runner.Worker process. It matches on the worker process's own executable path +// containing the runner root — each self-hosted runner's worker binary lives at +// "/bin/Runner.Worker", so a busy runner's process line always contains its own +// root path even when several runners' workers are running side by side on the same +// box. +// +// This fixes the wrapper that shipped the original guard: it checked +// `pgrep -f "Runner.Worker"` globally, so ONE busy runner out of four skipped cleanup +// for ALL four runners — on a farm that is busy around the clock, the cleanup path +// never ran at all. +func isRunnerBusy(root string) bool { + lines, err := listRunnerWorkerProcesses() + if err != nil { + // Can't tell — assume busy. Skipping a cleanup pass is always recoverable; + // deleting a live job's workspace is not. + return true + } + for _, line := range lines { + if strings.Contains(line, "Runner.Worker") && strings.Contains(line, root) { + return true + } + } + return false +} diff --git a/internal/maintenance/runner_posix_test.go b/internal/maintenance/runner_posix_test.go new file mode 100644 index 000000000..30000bc69 --- /dev/null +++ b/internal/maintenance/runner_posix_test.go @@ -0,0 +1,349 @@ +//go:build darwin || linux + +package maintenance + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// mustMkdirWithFile creates dir and a small file inside it so dirSize/removal is +// exercising real bytes, not an empty directory. +func mustMkdirWithFile(t *testing.T, dir string, content string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + if err := os.WriteFile(filepath.Join(dir, "payload"), []byte(content), 0o644); err != nil { + t.Fatalf("write payload in %s: %v", dir, err) + } +} + +func exists(t *testing.T, path string) bool { + t.Helper() + _, err := os.Stat(path) + return err == nil +} + +// withNoBusyRunners stubs listRunnerWorkerProcesses to report no runner processes at +// all, and restores the original on cleanup. Tests never shell out to a real `ps` or +// touch a real runner. +func withNoBusyRunners(t *testing.T) { + t.Helper() + orig := listRunnerWorkerProcesses + listRunnerWorkerProcesses = func() ([]string, error) { return nil, nil } + t.Cleanup(func() { listRunnerWorkerProcesses = orig }) +} + +// withBusyRunner stubs listRunnerWorkerProcesses to report a live Runner.Worker whose +// path is under busyRoot, and restores the original on cleanup. +func withBusyRunner(t *testing.T, busyRoot string) { + t.Helper() + orig := listRunnerWorkerProcesses + listRunnerWorkerProcesses = func() ([]string, error) { + return []string{filepath.Join(busyRoot, "bin", "Runner.Worker") + " spawnclient"}, nil + } + t.Cleanup(func() { listRunnerWorkerProcesses = orig }) +} + +// withStubbedSideEffects replaces the docker/log-rotation/journald hooks with no-ops +// for the duration of the test. DiskCleanupWithOptions always runs the "always safe" +// tier, and these tests must never shell out to a real docker daemon, /var/log, or +// journald — only the filesystem-fixture-driven tiers (runner work, caches) are under +// test here. +func withStubbedSideEffects(t *testing.T) { + t.Helper() + origDocker := dockerReclaimFunc + origLog := logRotationFunc + origJournal := journalVacuumFunc + dockerReclaimFunc = func(bool) (string, []error) { return "stubbed: docker not touched in tests", nil } + logRotationFunc = func(bool) (string, error) { return "stubbed: /var/log not touched in tests", nil } + journalVacuumFunc = func(bool) (string, error) { return "stubbed: journald not touched in tests", nil } + t.Cleanup(func() { + dockerReclaimFunc = origDocker + logRotationFunc = origLog + journalVacuumFunc = origJournal + }) +} + +// ── _actions/_tool/_temp/_PipelineMapping preserved ──────────────────────────────── + +func TestReclaimRunnerWork_PreservesProtectedSubdirs(t *testing.T) { + root := t.TempDir() + work := filepath.Join(root, "_work") + + for _, protected := range []string{"_actions", "_tool", "_temp", "_PipelineMapping"} { + mustMkdirWithFile(t, filepath.Join(work, protected), "runner internals") + } + // reclaimRunnerWork only inspects DIRECT children of _work (matching the real + // runner layout, "_work//") — so the removable unit here is the + // top-level "nself-org-cli" directory, not a nested path within it. + jobDir := filepath.Join(work, "nself-org-cli") + mustMkdirWithFile(t, jobDir, "checked out job files") + + bytes, reclaimed, skipped := reclaimRunnerWork(root, false) + + for _, protected := range []string{"_actions", "_tool", "_temp", "_PipelineMapping"} { + p := filepath.Join(work, protected) + if !exists(t, p) { + t.Errorf("protected dir %s was removed; must never be touched", p) + } + } + if exists(t, jobDir) { + t.Errorf("job workspace dir %s was not removed", jobDir) + } + if bytes == 0 { + t.Error("expected non-zero bytes reclaimed from the job workspace") + } + if len(reclaimed) != 1 || reclaimed[0].Path != jobDir { + t.Errorf("reclaimed = %+v; want exactly the job dir", reclaimed) + } + if len(skipped) != 4 { + t.Errorf("skipped = %d entries; want 4 (one per protected subdir)", len(skipped)) + } + for _, s := range skipped { + if !strings.Contains(s.Reason, "never removed") { + t.Errorf("skip reason for %s = %q; want it to explain the permanent exclusion", s.Path, s.Reason) + } + } +} + +// ── go-build / grype / trivy preserved, unconditionally ──────────────────────────── + +func TestReclaimCaches_PreservesGoBuildGrypeTrivy(t *testing.T) { + home := t.TempDir() + mustMkdirWithFile(t, filepath.Join(home, ".cache", "go-build"), "compiled object") + mustMkdirWithFile(t, filepath.Join(home, ".cache", "grype"), "vuln db") + mustMkdirWithFile(t, filepath.Join(home, ".cache", "trivy"), "vuln db") + mustMkdirWithFile(t, filepath.Join(home, ".cache", "turbo"), "turborepo cache") + mustMkdirWithFile(t, filepath.Join(home, "go", "pkg", "mod"), "downloaded module") + mustMkdirWithFile(t, filepath.Join(home, "pnpm-store"), "content-addressable store") + + bytes, reclaimed, skipped := reclaimCaches(home, []string{}, false) + + for _, protected := range []string{"go-build", "grype", "trivy"} { + p := filepath.Join(home, ".cache", protected) + if !exists(t, p) { + t.Errorf("protected cache %s was removed; compiler/scanner inputs must never be touched", p) + } + if !exists(t, filepath.Join(p, "payload")) { + t.Errorf("protected cache %s had its contents removed", p) + } + } + + for _, removable := range []string{ + filepath.Join(".cache", "turbo"), + filepath.Join("go", "pkg", "mod"), + "pnpm-store", + } { + p := filepath.Join(home, removable) + if exists(t, p) { + t.Errorf("regenerable cache %s was not removed", p) + } + } + + if bytes == 0 { + t.Error("expected non-zero bytes reclaimed from regenerable caches") + } + if len(reclaimed) != 3 { + t.Errorf("reclaimed = %d entries; want 3 (turbo, go/pkg/mod, pnpm-store)", len(reclaimed)) + } + if len(skipped) != 3 { + t.Errorf("skipped = %d entries; want 3 (go-build, grype, trivy)", len(skipped)) + } +} + +// TestDiskCleanupWithOptions_ProtectedCachesSurvivePressureEscalation exercises the +// same protection through the top-level entry point, with disk pressure forcing the +// shared-cache tier to run even though a runner is busy — protectedCacheSubpaths must +// still be excluded "at every tier", pressure included. +func TestDiskCleanupWithOptions_ProtectedCachesSurvivePressureEscalation(t *testing.T) { + home := t.TempDir() + mustMkdirWithFile(t, filepath.Join(home, ".cache", "go-build"), "compiled object") + mustMkdirWithFile(t, filepath.Join(home, ".cache", "grype"), "vuln db") + mustMkdirWithFile(t, filepath.Join(home, "go", "pkg", "mod"), "downloaded module") + + busyRoot := t.TempDir() + withBusyRunner(t, busyRoot) + withStubbedSideEffects(t) + + result := DiskCleanupWithOptions(DiskCleanupOptions{ + Home: home, + RunnerRoots: []RunnerRoot{{Name: "busy-runner", Path: busyRoot}}, + PressureThreshold: 1, + UsageOverride: &DiskUsage{UsedPercent: 90}, // deterministic pressure trigger + SharedCacheRoots: []string{}, + }) + + if exists(t, filepath.Join(home, "go", "pkg", "mod")) { + t.Error("expected regenerable cache (go/pkg/mod) to be reclaimed under pressure despite busy runner") + } + for _, protected := range []string{"go-build", "grype"} { + if !exists(t, filepath.Join(home, ".cache", protected)) { + t.Errorf("protected cache %s must survive even under pressure escalation", protected) + } + } + _ = result +} + +// ── per-runner busy detection: clean the idle one, skip the busy one ─────────────── + +func TestPerRunnerBusyDetection_CleansIdleSkipsBusy(t *testing.T) { + idleRoot := t.TempDir() + idleJobDir := filepath.Join(idleRoot, "_work", "nself-org", "cli") + mustMkdirWithFile(t, idleJobDir, "idle runner's stale job checkout") + + busyRoot := t.TempDir() + busyJobDir := filepath.Join(busyRoot, "_work", "nself-org", "ntask") + mustMkdirWithFile(t, busyJobDir, "busy runner's in-progress job checkout") + + withBusyRunner(t, busyRoot) + withStubbedSideEffects(t) + + result := DiskCleanupWithOptions(DiskCleanupOptions{ + Home: t.TempDir(), // isolated, empty — caches tier is irrelevant to this test + RunnerRoots: []RunnerRoot{ + {Name: "idle-runner", Path: idleRoot}, + {Name: "busy-runner", Path: busyRoot}, + }, + PressureThreshold: 100, // deliberately unreachable, isolates the busy-gate behavior + UsageOverride: &DiskUsage{UsedPercent: 50}, + SharedCacheRoots: []string{}, + }) + + if exists(t, idleJobDir) { + t.Error("idle runner's job workspace should have been reclaimed") + } + if !exists(t, busyJobDir) { + t.Error("busy runner's job workspace must be left alone") + } + + foundBusySkip := false + for _, s := range result.Skipped { + if s.Path == busyRoot { + foundBusySkip = true + if !strings.Contains(s.Reason, "busy") { + t.Errorf("skip reason for busy root = %q; want it to mention busy", s.Reason) + } + } + } + if !foundBusySkip { + t.Error("expected a Skipped entry explaining why the busy runner root was left alone") + } + if result.BytesReclaimed == 0 { + t.Error("expected non-zero bytes reclaimed from the idle runner's job workspace") + } +} + +// ── pressure escalation triggers above threshold ──────────────────────────────────── + +func TestPressureEscalation_TriggersAboveThreshold(t *testing.T) { + home := t.TempDir() + mustMkdirWithFile(t, filepath.Join(home, "pnpm-store"), "content-addressable store") + + busyRoot := t.TempDir() + withBusyRunner(t, busyRoot) + withStubbedSideEffects(t) + + // Below threshold: busy runner blocks the shared-cache tier entirely. + belowResult := DiskCleanupWithOptions(DiskCleanupOptions{ + Home: home, + RunnerRoots: []RunnerRoot{{Name: "busy-runner", Path: busyRoot}}, + PressureThreshold: 100, + UsageOverride: &DiskUsage{UsedPercent: 50}, + SharedCacheRoots: []string{}, + }) + if !exists(t, filepath.Join(home, "pnpm-store")) { + t.Fatal("pnpm-store should NOT have been reclaimed below the pressure threshold while busy") + } + foundCacheSkip := false + for _, s := range belowResult.Skipped { + if strings.Contains(s.Reason, "pressure threshold") { + foundCacheSkip = true + } + } + if !foundCacheSkip { + t.Error("expected a Skipped entry citing the pressure threshold when below it") + } + + // At/above threshold: shared caches reclaim regardless of the busy runner. + aboveResult := DiskCleanupWithOptions(DiskCleanupOptions{ + Home: home, + RunnerRoots: []RunnerRoot{{Name: "busy-runner", Path: busyRoot}}, + PressureThreshold: 85, + UsageOverride: &DiskUsage{UsedPercent: 90}, + SharedCacheRoots: []string{}, + }) + if exists(t, filepath.Join(home, "pnpm-store")) { + t.Error("pnpm-store should have been reclaimed once disk usage crossed the pressure threshold") + } + if aboveResult.BytesReclaimed == 0 { + t.Error("expected non-zero bytes reclaimed once pressure escalation kicked in") + } +} + +// ── dry-run removes nothing ───────────────────────────────────────────────────────── + +func TestDiskCleanupWithOptions_DryRunRemovesNothing(t *testing.T) { + home := t.TempDir() + mustMkdirWithFile(t, filepath.Join(home, ".cache", "turbo"), "turborepo cache") + mustMkdirWithFile(t, filepath.Join(home, "go", "pkg", "mod"), "downloaded module") + mustMkdirWithFile(t, filepath.Join(home, "pnpm-store"), "content-addressable store") + + runnerRoot := t.TempDir() + jobDir := filepath.Join(runnerRoot, "_work", "nself-org", "cli") + mustMkdirWithFile(t, jobDir, "job checkout") + + withNoBusyRunners(t) + withStubbedSideEffects(t) + + result := DiskCleanupWithOptions(DiskCleanupOptions{ + DryRun: true, + Home: home, + RunnerRoots: []RunnerRoot{{Name: "idle-runner", Path: runnerRoot}}, + PressureThreshold: 100, + UsageOverride: &DiskUsage{UsedPercent: 50}, + SharedCacheRoots: []string{}, + }) + + for _, p := range []string{ + filepath.Join(home, ".cache", "turbo"), + filepath.Join(home, "go", "pkg", "mod"), + filepath.Join(home, "pnpm-store"), + jobDir, + } { + if !exists(t, p) { + t.Errorf("dry-run removed %s; it must not remove anything", p) + } + } + + if !result.DryRun { + t.Error("CleanupResult.DryRun should be true") + } + if len(result.Reclaimed) == 0 { + t.Error("dry-run should still report what WOULD have been reclaimed") + } + for _, r := range result.Reclaimed { + if r.Bytes == 0 { + t.Errorf("dry-run reclaim entry %s reports 0 bytes; want the real would-be size", r.Path) + } + } +} + +// ── filterAnonymousVolumes: never a named "*_data" volume ───────────────────────── + +func TestFilterAnonymousVolumes_ExcludesNamedVolumes(t *testing.T) { + names := []string{ + "a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2", // 64 hex chars + "ntask_data", + "hasura_data", + "", + "not-hex-and-not-64-chars", + } + got := filterAnonymousVolumes(names) + if len(got) != 1 || got[0] != names[0] { + t.Errorf("filterAnonymousVolumes(%v) = %v; want only the 64-hex anonymous name", names, got) + } +} From 4306719a0ff56b167d40f756f06b7961d5417d41 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:13:37 -0400 Subject: [PATCH 2/4] fix(maintenance): do not delete a workspace a job is still writing to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A per-runner busy check is necessary but not sufficient: the check and the RemoveAll are not atomic. Cleaning workspaces on runners that reported idle destroyed three live jobs on nSelf staging on 2026-09-11, which failed with 'Directory .../_work/web/web does not exist' — jobs had started in the window between the scan and the removal. A running job writes into its workspace constantly, which is the signal a process scan cannot give. Require a workspace to have gone untouched for workspaceStaleAfter (30m) before removing it. A merely slow job still touches its files; a finished one cannot. Existing tests build fresh fixtures and so opt out via withoutStalenessGuard. Also drops DefaultPressureThreshold from 85 to 75. That number has to keep a different check satisfied: 'nself doctor --deep' fails the host at 80% used ('Disk free: /: N% free (<20%)'). Escalating at 85 leaves a band where cleanup is content and doctor is red, which is the state staging was in that day — the dogfood gate failing on disk while the cleanup timer reported nothing to do. A test pins the two thresholds in the correct order. --- internal/maintenance/disk.go | 13 ++- internal/maintenance/runner_posix.go | 54 +++++++++++++ internal/maintenance/runner_posix_test.go | 97 +++++++++++++++++++++++ 3 files changed, 163 insertions(+), 1 deletion(-) diff --git a/internal/maintenance/disk.go b/internal/maintenance/disk.go index e24af20c3..03ab1f291 100644 --- a/internal/maintenance/disk.go +++ b/internal/maintenance/disk.go @@ -13,7 +13,18 @@ import ( // cache reclaims run regardless of runner busy state. A full disk fails every job on // the box anyway, so waiting for idle past this point is strictly worse than // reclaiming now. -const DefaultPressureThreshold = 85 +// +// 75, not 85, because this number has to keep a DIFFERENT check satisfied: +// `nself doctor --deep` fails the host with "Disk free: /: N% free (<20%)" at +// 80% used. An escalation threshold above that lets the box settle in a band +// where cleanup is content but doctor is red — which is exactly what happened +// on nSelf staging on 2026-09-11, where the dogfood gate failed on disk while +// the daily cleanup timer reported nothing to do. Escalating at 75 keeps the +// box under doctor's limit with headroom for one large job's working set. +// +// Keep this BELOW the doctor host-disk threshold. If that check's limit moves, +// move this with it. +const DefaultPressureThreshold = 75 // GetDiskUsage returns current disk utilisation for the root filesystem ("/"). func GetDiskUsage() (DiskUsage, error) { diff --git a/internal/maintenance/runner_posix.go b/internal/maintenance/runner_posix.go index faf9e8b7f..13f720f97 100644 --- a/internal/maintenance/runner_posix.go +++ b/internal/maintenance/runner_posix.go @@ -4,9 +4,11 @@ package maintenance import ( "fmt" + "io/fs" "os" "path/filepath" "strings" + "time" ) // RunnerRoot describes one discovered GitHub Actions self-hosted runner install. @@ -99,6 +101,25 @@ func reclaimRunnerWork(root string, dryRun bool) (bytes int64, reclaimed []Recla continue } + // A per-runner busy check is necessary but not sufficient: the check + // and the removal are not atomic. On 2026-09-11, cleaning workspaces + // on runners that reported idle destroyed three live jobs, which + // failed with "Directory .../_work/web/web does not exist" — a job had + // started in the window between the check and the RemoveAll. + // + // A running job writes into its workspace constantly, so recent + // modification is the signal a process scan cannot give us. Require + // the tree to have been untouched for workspaceStaleAfter before + // removing it. A merely slow job still touches its workspace; a + // finished one cannot. + if recentlyModified(full, workspaceStaleAfter) { + skipped = append(skipped, SkipEntry{ + Path: full, + Reason: fmt.Sprintf("modified within %s — treated as an active job workspace", workspaceStaleAfter), + }) + continue + } + size := dirSize(full) if !dryRun { if err := os.RemoveAll(full); err != nil { @@ -220,3 +241,36 @@ func isRunnerBusy(root string) bool { } return false } + +// workspaceStaleAfter is how long a runner job workspace must go untouched +// before disk-cleanup will treat it as abandoned and remove it. This exists +// because isRunnerBusy alone races: a runner can report idle and begin a job +// microseconds later. 30 minutes is comfortably longer than the gap between a +// job's filesystem writes while it runs, and far shorter than the lifetime of +// a genuinely abandoned checkout. +// It is a var, not a const, so tests can exercise both sides of the guard +// without having to backdate every fixture they build. +var workspaceStaleAfter = 30 * time.Minute + +// recentlyModified reports whether any file under root was modified within the +// last d. It stops at the first hit rather than walking the whole tree, since +// one recent file is enough to prove the workspace is in use. +func recentlyModified(root string, d time.Duration) bool { + cutoff := time.Now().Add(-d) + found := false + _ = filepath.WalkDir(root, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return nil // unreadable entry proves nothing; keep looking + } + info, statErr := entry.Info() + if statErr != nil { + return nil + } + if info.ModTime().After(cutoff) { + found = true + return filepath.SkipAll + } + return nil + }) + return found +} diff --git a/internal/maintenance/runner_posix_test.go b/internal/maintenance/runner_posix_test.go index 30000bc69..0aa62d493 100644 --- a/internal/maintenance/runner_posix_test.go +++ b/internal/maintenance/runner_posix_test.go @@ -7,8 +7,20 @@ import ( "path/filepath" "strings" "testing" + "time" ) +// withoutStalenessGuard disables the workspace staleness window for tests whose +// fixtures are necessarily brand new. Those tests assert what reclaimRunnerWork +// removes and protects, not when it defers; the staleness behaviour has its own +// test (TestReclaimRunnerWork_SkipsRecentlyModifiedWorkspace). +func withoutStalenessGuard(t *testing.T) { + t.Helper() + prev := workspaceStaleAfter + workspaceStaleAfter = 0 + t.Cleanup(func() { workspaceStaleAfter = prev }) +} + // mustMkdirWithFile creates dir and a small file inside it so dirSize/removal is // exercising real bytes, not an empty directory. func mustMkdirWithFile(t *testing.T, dir string, content string) { @@ -71,6 +83,7 @@ func withStubbedSideEffects(t *testing.T) { // ── _actions/_tool/_temp/_PipelineMapping preserved ──────────────────────────────── func TestReclaimRunnerWork_PreservesProtectedSubdirs(t *testing.T) { + withoutStalenessGuard(t) root := t.TempDir() work := filepath.Join(root, "_work") @@ -113,6 +126,7 @@ func TestReclaimRunnerWork_PreservesProtectedSubdirs(t *testing.T) { // ── go-build / grype / trivy preserved, unconditionally ──────────────────────────── func TestReclaimCaches_PreservesGoBuildGrypeTrivy(t *testing.T) { + withoutStalenessGuard(t) home := t.TempDir() mustMkdirWithFile(t, filepath.Join(home, ".cache", "go-build"), "compiled object") mustMkdirWithFile(t, filepath.Join(home, ".cache", "grype"), "vuln db") @@ -160,6 +174,7 @@ func TestReclaimCaches_PreservesGoBuildGrypeTrivy(t *testing.T) { // shared-cache tier to run even though a runner is busy — protectedCacheSubpaths must // still be excluded "at every tier", pressure included. func TestDiskCleanupWithOptions_ProtectedCachesSurvivePressureEscalation(t *testing.T) { + withoutStalenessGuard(t) home := t.TempDir() mustMkdirWithFile(t, filepath.Join(home, ".cache", "go-build"), "compiled object") mustMkdirWithFile(t, filepath.Join(home, ".cache", "grype"), "vuln db") @@ -191,6 +206,7 @@ func TestDiskCleanupWithOptions_ProtectedCachesSurvivePressureEscalation(t *test // ── per-runner busy detection: clean the idle one, skip the busy one ─────────────── func TestPerRunnerBusyDetection_CleansIdleSkipsBusy(t *testing.T) { + withoutStalenessGuard(t) idleRoot := t.TempDir() idleJobDir := filepath.Join(idleRoot, "_work", "nself-org", "cli") mustMkdirWithFile(t, idleJobDir, "idle runner's stale job checkout") @@ -240,6 +256,7 @@ func TestPerRunnerBusyDetection_CleansIdleSkipsBusy(t *testing.T) { // ── pressure escalation triggers above threshold ──────────────────────────────────── func TestPressureEscalation_TriggersAboveThreshold(t *testing.T) { + withoutStalenessGuard(t) home := t.TempDir() mustMkdirWithFile(t, filepath.Join(home, "pnpm-store"), "content-addressable store") @@ -287,6 +304,7 @@ func TestPressureEscalation_TriggersAboveThreshold(t *testing.T) { // ── dry-run removes nothing ───────────────────────────────────────────────────────── func TestDiskCleanupWithOptions_DryRunRemovesNothing(t *testing.T) { + withoutStalenessGuard(t) home := t.TempDir() mustMkdirWithFile(t, filepath.Join(home, ".cache", "turbo"), "turborepo cache") mustMkdirWithFile(t, filepath.Join(home, "go", "pkg", "mod"), "downloaded module") @@ -347,3 +365,82 @@ func TestFilterAnonymousVolumes_ExcludesNamedVolumes(t *testing.T) { t.Errorf("filterAnonymousVolumes(%v) = %v; want only the 64-hex anonymous name", names, got) } } + +// TestReclaimRunnerWork_SkipsRecentlyModifiedWorkspace covers the race that a +// busy-process check cannot: a runner reports idle, and a job starts before the +// RemoveAll lands. On 2026-09-11 that destroyed three live jobs on nSelf +// staging ("Directory .../_work/web/web does not exist"). A live job writes +// into its workspace constantly, so a recent mtime must veto removal even when +// the runner looks idle. +func TestReclaimRunnerWork_SkipsRecentlyModifiedWorkspace(t *testing.T) { + root := t.TempDir() + work := filepath.Join(root, "_work") + + active := filepath.Join(work, "active-repo", "active-repo") + if err := os.MkdirAll(active, 0o755); err != nil { + t.Fatal(err) + } + // A file written now stands in for a job mid-build. + if err := os.WriteFile(filepath.Join(active, "building.log"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + + stale := filepath.Join(work, "abandoned-repo", "abandoned-repo") + if err := os.MkdirAll(stale, 0o755); err != nil { + t.Fatal(err) + } + staleFile := filepath.Join(stale, "old.log") + if err := os.WriteFile(staleFile, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + // Age the whole abandoned tree well past the staleness window. + old := time.Now().Add(-2 * workspaceStaleAfter) + for _, p := range []string{staleFile, stale, filepath.Join(work, "abandoned-repo")} { + if err := os.Chtimes(p, old, old); err != nil { + t.Fatal(err) + } + } + + _, reclaimed, skipped := reclaimRunnerWork(root, false) + + if _, err := os.Stat(filepath.Join(work, "active-repo")); err != nil { + t.Fatalf("active workspace was removed despite a recent write: %v", err) + } + if _, err := os.Stat(filepath.Join(work, "abandoned-repo")); !os.IsNotExist(err) { + t.Fatalf("abandoned workspace should have been reclaimed, stat err = %v", err) + } + + var skippedActive bool + for _, s := range skipped { + if strings.Contains(s.Path, "active-repo") && strings.Contains(s.Reason, "active job workspace") { + skippedActive = true + } + } + if !skippedActive { + t.Errorf("expected active-repo to be skipped as an active workspace, got skips: %+v", skipped) + } + + var reclaimedStale bool + for _, r := range reclaimed { + if strings.Contains(r.Path, "abandoned-repo") { + reclaimedStale = true + } + } + if !reclaimedStale { + t.Errorf("expected abandoned-repo to be reclaimed, got: %+v", reclaimed) + } +} + +// TestPressureThresholdStaysBelowDoctorHostLimit pins the relationship between +// this package's escalation point and the doctor host-disk check. doctor fails +// at 80% used (<20% free); if escalation were at or above that, the box could +// sit in a band where cleanup is satisfied and doctor is red — the exact state +// nSelf staging was in on 2026-09-11. +func TestPressureThresholdStaysBelowDoctorHostLimit(t *testing.T) { + const doctorHostDiskUsedLimit = 80 + if DefaultPressureThreshold >= doctorHostDiskUsedLimit { + t.Fatalf("DefaultPressureThreshold (%d) must stay below the doctor host-disk limit (%d%% used); "+ + "otherwise cleanup never escalates in the band where doctor already fails", + DefaultPressureThreshold, doctorHostDiskUsedLimit) + } +} From ac0412152cf4ddaa8936a44d3ebd6c5ae32f35ef Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:20:21 -0400 Subject: [PATCH 3/4] fix(maintenance): drop the unused tierAlways constant golangci-lint fails the build on it: 'const tierAlways is unused'. go vet does not flag unused constants, which is why it passed locally. The always-safe reclaims (docker dangling images and build cache, anonymous volumes, old compressed logs, journald history) genuinely have no ReclaimEntry records to tag. Those commands report their own freed space through CleanupResult's DockerPruneOut / LogRotationOut / JournalVacuumOut and give no per-path byte attribution, so synthesising zero-byte entries purely to carry a tier label would make the reclaimed list read as though nothing was freed. Keeps the tier documented in prose and says why it has no constant. --- internal/maintenance/disk_shared.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/maintenance/disk_shared.go b/internal/maintenance/disk_shared.go index edfd60234..995fb3a1e 100644 --- a/internal/maintenance/disk_shared.go +++ b/internal/maintenance/disk_shared.go @@ -24,10 +24,16 @@ type DiskUsage struct { type reclaimTier string const ( - // tierAlways reclaims are safe unconditionally: busy or idle, under pressure or - // not. Nothing a running job reads lives here (docker dangling images/build - // cache/anonymous volumes, old compressed logs, journald history). - tierAlways reclaimTier = "always" + // The "always" tier (docker dangling images/build cache/anonymous volumes, + // old compressed logs, journald history) is safe unconditionally: busy or + // idle, under pressure or not, because nothing a running job reads lives + // there. It deliberately has no reclaimTier constant. Those three reclaims + // report through CleanupResult's DockerPruneOut / LogRotationOut / + // JournalVacuumOut fields rather than as per-path ReclaimEntry records, + // because the underlying commands report their own freed space and give no + // per-path byte attribution. Inventing zero-byte entries just to carry a + // tier label would make the reclaimed list read as if nothing was freed. + // // tierIdlePerRunner reclaims (runner job workspaces) only run for a runner root // that is individually idle. Never escalated by disk pressure — deleting an // in-progress job's own checkout would break that job outright, the same failure From a9092cf692a9c871c58544485b8e69bfaf9e2b77 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:24:11 -0400 Subject: [PATCH 4/4] fix(maintenance): discover runner installs not named actions-runner* The glob fallback covered /opt/actions-runner*, /home/*/actions-runner* and /home/*/*/actions-runner*. nSelf staging installs the runner serving the web repo at /home/runner/github-runner, which matches none of them. That install held 5.1G, 2.3G of it job workspaces, and was invisible to cleanup: not a candidate at any tier, under any pressure, because discovery never returned it. The box filled to 100% on 2026-09-11 with that space sitting unreclaimable. Adds the github-runner naming in both /home and /opt, and a test that pins every convention actually in use. A runner discovery never returns is a runner cleanup can never reclaim, so the test asserts the glob set matches each real install path rather than asserting the list's contents. --- .../maintenance/runner_discovery_posix.go | 11 +++++ .../runner_discovery_posix_test.go | 45 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 internal/maintenance/runner_discovery_posix_test.go diff --git a/internal/maintenance/runner_discovery_posix.go b/internal/maintenance/runner_discovery_posix.go index 053b10a29..be55d9e80 100644 --- a/internal/maintenance/runner_discovery_posix.go +++ b/internal/maintenance/runner_discovery_posix.go @@ -18,10 +18,21 @@ const runnerRootsEnvVar = "NSELF_MAINTENANCE_RUNNER_ROOTS" // used across the nself fleet. Documented here rather than hardcoded to one path so a // box with runners under /opt, or under a non-"runner" username, is still discovered // even without systemd unit introspection. +// The "github-runner" entries are not decorative. On nSelf staging the runner +// serving the web repo is installed at /home/runner/github-runner, which +// matches none of the actions-runner* patterns. It held 5.1G (2.3G of it job +// workspaces) and was invisible to a glob set covering only actions-runner*, +// so it was never eligible for cleanup while the box filled to 100% on +// 2026-09-11. Any install directory naming convention that is actually in use +// belongs here, because a runner this never sees is a runner it can never +// reclaim. var defaultRunnerRootGlobs = []string{ "/home/*/actions-runner*", "/home/*/*/actions-runner*", + "/home/*/github-runner", + "/home/*/*/github-runner", "/opt/actions-runner*", + "/opt/github-runner*", } // discoverRunnerRoots finds installed GitHub Actions self-hosted runners. It prefers diff --git a/internal/maintenance/runner_discovery_posix_test.go b/internal/maintenance/runner_discovery_posix_test.go new file mode 100644 index 000000000..e43dfd1ba --- /dev/null +++ b/internal/maintenance/runner_discovery_posix_test.go @@ -0,0 +1,45 @@ +//go:build darwin || linux + +package maintenance + +import ( + "path/filepath" + "testing" +) + +// TestDefaultRunnerRootGlobs_CoversGithubRunnerNaming pins the naming +// conventions the fallback must match. nSelf staging installs the web repo's +// runner at /home/runner/github-runner, which matches no actions-runner* +// pattern; it held 5.1G that cleanup could never see, and the box filled to +// 100% on 2026-09-11 with that space unreclaimable. A runner this never +// discovers is a runner it can never clean. +func TestDefaultRunnerRootGlobs_CoversGithubRunnerNaming(t *testing.T) { + cases := []struct { + name string + path string + }{ + {"opt actions-runner", "/opt/actions-runner/_work"}, + {"opt numbered actions-runner", "/opt/actions-runner-3/_work"}, + {"home actions-runner", "/home/runner/actions-runner/_work"}, + {"home github-runner (the one that was missed)", "/home/runner/github-runner/_work"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + root := filepath.Dir(tc.path) + var matched bool + for _, g := range defaultRunnerRootGlobs { + ok, err := filepath.Match(g, root) + if err != nil { + t.Fatalf("bad glob %q: %v", g, err) + } + if ok { + matched = true + break + } + } + if !matched { + t.Errorf("no glob in defaultRunnerRootGlobs matches %q; that runner's disk can never be reclaimed", root) + } + }) + } +}