From 265e533ad3c531eddf8691db367ddac57eb8749d Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Tue, 8 Sep 2026 21:59:58 -0500 Subject: [PATCH] scanner: reclaim scan directories a killed process left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildOCILayout already removes its scan dir on every error path, and syft.go defers the stereoscope generator's Cleanup. What neither can do is clean up after a process that dies mid-scan: the deferred call never runs, and nothing afterwards ever looks at what was left. Every restart therefore leaks the in-flight layout and extraction permanently, and a restart is routine — a deploy is one. On seamark-hold that reached 8.8 GB of orphaned scan-*, syft-scan-* and syft-cataloger-* directories under a 20 GB disk, at which point the disk was 97% full and scans began failing on it: failed to load OCI image: unable to populate layer cache dir="/var/lib/seamark/scanner/tmp/syft-scan-1546187834/..." : no space left on device failed to download layer 5: failed to write blob: write /var/lib/seamark/scanner/tmp/scan-4160414849/blobs/sha256/... : no space left on device The leaked directories cluster at the scanner's restart timestamps, which is what identifies the killed process rather than the error paths as the source. Manual removal reclaimed 8.8 GB and took the disk from 97% to 50%. Startup is where this belongs: it is the one moment the previous process is known to be gone, and it is immediately after the event that caused the leak. The sweep runs in WorkerPool.Start after TMPDIR is set and before any worker can dequeue, so nothing it removes can be work in progress here. Three constraints shape what it will touch: - Only the three per-job prefixes, only as direct children, only directories. The Grype database lives beside the tmp dir at /vulndb and go-getter unpacks into grype-dl underneath it; both are state the scanner needs and neither matches a prefix. The prefixes now have one definition each, used by both the creator and the sweeper, so renaming a directory cannot silently take it out of the sweep's scope. - An age threshold, vuln.sweep_max_age, default 1h. A second scanner sharing the directory has an in-flight scan-* dir that is minutes old, and scanner.job_timeout is 8m, so an hour clears both with room to spare. 0 disables the sweep rather than removing a peer's live work. - Nothing is fatal. A stat or removal failure is a WARN and the sweep moves on, so a permission problem in the tmp dir cannot keep the scanner from starting. The sweep only runs at startup, so a scanner that is killed twice between deploys carries the first leak until its next restart. That is the tradeoff for never racing a live peer; a periodic sweep would be the follow-up if processes ever live long enough for it to matter. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TA9D4DjaLZTvzQ7dJbu4eg --- deploy/upcloud/configs/scanner.yaml.tmpl | 6 + docs/SBOM_SCANNING.md | 6 + scanner/internal/config/config.go | 8 ++ scanner/internal/scan/extractor.go | 2 +- scanner/internal/scan/sweep.go | 130 ++++++++++++++++++++ scanner/internal/scan/sweep_test.go | 148 +++++++++++++++++++++++ scanner/internal/scan/syft.go | 2 +- scanner/internal/scan/worker.go | 6 + 8 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 scanner/internal/scan/sweep.go create mode 100644 scanner/internal/scan/sweep_test.go diff --git a/deploy/upcloud/configs/scanner.yaml.tmpl b/deploy/upcloud/configs/scanner.yaml.tmpl index 59845a7..eca1469 100644 --- a/deploy/upcloud/configs/scanner.yaml.tmpl +++ b/deploy/upcloud/configs/scanner.yaml.tmpl @@ -47,3 +47,9 @@ vuln: # reached 561 MiB RSS with the memory limit applied. Images above this are # rejected before any blob is downloaded. max_image_size: 536870912 + # Reclaim scan directories left behind by a scanner that was killed + # mid-scan (a restart or a deploy). Those never run their own cleanup, and + # on this host they reached 8.8 GB on a 20 GB disk before scans started + # failing with "no space left on device". Above the 8m job timeout, so a + # scan in flight elsewhere in the same directory is never touched. + sweep_max_age: 1h diff --git a/docs/SBOM_SCANNING.md b/docs/SBOM_SCANNING.md index 3011312..0119ff2 100644 --- a/docs/SBOM_SCANNING.md +++ b/docs/SBOM_SCANNING.md @@ -110,6 +110,7 @@ a YAML file or pure env vars with the `SCANNER_` prefix. Run with | `vuln.db_path` | `SCANNER_VULN_DB_PATH` | `/var/lib/atcr-scanner/vulndb` | Directory for the Grype vulnerability database. | | `vuln.tmp_dir` | `SCANNER_VULN_TMP_DIR` | `/var/lib/atcr-scanner/tmp` | Directory for layer extraction and DB download. Also exported as `TMPDIR`; point it at a large partition, **not** tmpfs. | | `vuln.max_image_size`| `SCANNER_VULN_MAX_IMAGE_SIZE`| `2147483648` (2 GiB) | Max total compressed image size. Larger images are skipped with an error. `0` = no limit. | +| `vuln.sweep_max_age`| `SCANNER_VULN_SWEEP_MAX_AGE` | `1h` | Age threshold for the startup sweep of `vuln.tmp_dir`. Leftover `scan-*`, `syft-scan-*` and `syft-cataloger-*` directories older than this are removed before workers start; a scan killed by a restart never cleans up after itself. Keep it above the longest scan so a second scanner sharing the directory keeps its live work. `0` disables the sweep. | | `server.addr` | `SCANNER_SERVER_ADDR` | `:9090` | Listen address for the scanner's health endpoint. | Both `hold.url` and `hold.secret` are required; `LoadConfig` errors out if either is @@ -420,6 +421,11 @@ admin endpoint `POST /admin/api/scan-backfill` instead. - **Layer extraction or Grype DB download fails mid-process.** `vuln.tmp_dir` is too small or on tmpfs. Point it at a large persistent partition; the scanner sets `TMPDIR` to this directory. +- **`vuln.tmp_dir` fills up with `scan-*` / `syft-scan-*` directories.** These are + scans the process was killed in the middle of, which never ran their own cleanup. + The startup sweep reclaims them on the next restart; if it is not doing so, check + that `vuln.sweep_max_age` is not `0` and look for `Swept scanner tmp dir` in the + startup logs. - **SBOM present but no vulnerability counts.** `vuln.enabled` is false on the scanner, or the Grype DB failed to initialize (check startup logs). - **Helm/attestation artifacts show "scanning isn't applied".** Expected — these are diff --git a/scanner/internal/config/config.go b/scanner/internal/config/config.go index ff717f2..e839fe1 100644 --- a/scanner/internal/config/config.go +++ b/scanner/internal/config/config.go @@ -68,6 +68,13 @@ type VulnConfig struct { // Maximum total compressed image size in bytes. Images exceeding this are skipped. 0 = no limit. MaxImageSize int64 `yaml:"max_image_size" comment:"Maximum total compressed image size in bytes. 0 = no limit. Default: 2 GiB."` + + // Age threshold for the startup sweep of tmp_dir. A scan killed mid-flight + // (a restart, a deploy) never runs its own cleanup, so its layout and + // extraction directories stay on disk forever; the sweep reclaims the ones + // older than this. It must stay well above the longest a scan can take, so + // that a second scanner sharing the directory keeps its live work. + SweepMaxAge time.Duration `yaml:"sweep_max_age" comment:"Age threshold for the startup sweep of stale scan directories in tmp_dir (scan-*, syft-scan-*, syft-cataloger-*), which a scan interrupted by a restart leaves behind. Must stay above the longest scan so a second scanner sharing the directory keeps its live work. 0 disables the sweep. Default: 1h."` } // setScannerDefaults registers all default values on the given Viper instance. @@ -92,6 +99,7 @@ func setScannerDefaults(v *viper.Viper) { v.SetDefault("vuln.db_path", "/var/lib/atcr-scanner/vulndb") v.SetDefault("vuln.tmp_dir", "/var/lib/atcr-scanner/tmp") v.SetDefault("vuln.max_image_size", 2*1024*1024*1024) // 2 GiB + v.SetDefault("vuln.sweep_max_age", "1h") // Log shipper defaults v.SetDefault("log_shipper.batch_size", 100) diff --git a/scanner/internal/scan/extractor.go b/scanner/internal/scan/extractor.go index 914ca9f..95c5ef3 100644 --- a/scanner/internal/scan/extractor.go +++ b/scanner/internal/scan/extractor.go @@ -62,7 +62,7 @@ type ociIndex struct { // waiting out the HTTP client's own per-request timeout, and every exit from // here removes the scan directory — the deadline path included. func buildOCILayout(ctx context.Context, job *scanner.ScanJob, tmpDir, secret string, maxBytes int64) (string, func(), error) { - scanDir, err := os.MkdirTemp(tmpDir, "scan-*") + scanDir, err := os.MkdirTemp(tmpDir, scanDirPrefix+"*") if err != nil { return "", nil, fmt.Errorf("failed to create temp directory: %w", err) } diff --git a/scanner/internal/scan/sweep.go b/scanner/internal/scan/sweep.go new file mode 100644 index 0000000..c2759b4 --- /dev/null +++ b/scanner/internal/scan/sweep.go @@ -0,0 +1,130 @@ +package scan + +import ( + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" + "time" +) + +// The three directory shapes a scan leaves directly under the configured tmp +// dir. Everything created here is per-job and disposable: +// +// scan-* buildOCILayout's downloaded OCI layout +// syft-scan-* stereoscope's extraction root (syftTempDirName) +// syft-cataloger-* Syft's own cataloging scratch, created inside itself +// +// Only these are ever swept. The Grype database lives beside the tmp dir at +// /vulndb and go-getter unpacks its downloads into a grype-dl +// directory underneath it; both are state the scanner needs and neither +// matches a prefix here. +const ( + scanDirPrefix = "scan-" + // syftTempDirName is the root prefix handed to stereoscope's + // TempDirGenerator in syft.go, which appends "-" and a random suffix. + syftTempDirName = "syft-scan" + // syftCatalogerPrefix is Syft's, not ours: it opens its own generator + // during cataloging. Named here because the sweep is the only thing that + // reclaims one after a crash. + syftCatalogerPrefix = "syft-cataloger-" +) + +// sweepPrefixes is the full set, defined once so the creators above and the +// sweeper below cannot drift apart. +var sweepPrefixes = []string{scanDirPrefix, syftTempDirName + "-", syftCatalogerPrefix} + +// sweepTmpDir removes scan leftovers older than maxAge from tmpDir. +// +// The per-job error paths already clean up after themselves; what they cannot +// do is clean up after a process that dies mid-scan. A restart — a deploy +// included — permanently orphans whatever was in flight, and on seamark-hold +// that reached 8.8 GB on a 20 GB disk before scans started failing with "no +// space left on device". Nothing else ever reclaims these, so startup does. +// +// Only direct children matching sweepPrefixes are considered, and only +// directories: the tmp dir is shared with state the scanner needs. maxAge is +// what makes this safe to run while another scanner shares the directory — +// its in-flight scan-* dir is minutes old, not hours. A maxAge of zero or less +// disables the sweep entirely rather than removing a peer's live work. +// +// Nothing here is fatal. Every failure is logged and the sweep moves on, so a +// permission problem in the tmp dir cannot stop the scanner from starting. +func sweepTmpDir(tmpDir string, maxAge time.Duration) { + if tmpDir == "" || maxAge <= 0 { + return + } + + entries, err := os.ReadDir(tmpDir) + if err != nil { + // A tmp dir that does not exist yet is the normal first-boot state, + // not something to warn about. + if !errors.Is(err, fs.ErrNotExist) { + slog.Warn("Failed to sweep scanner tmp dir", "dir", tmpDir, "error", err) + } + return + } + + cutoff := time.Now().Add(-maxAge) + var removed int + var reclaimed int64 + for _, entry := range entries { + if !entry.IsDir() || !sweepable(entry.Name()) { + continue + } + info, err := entry.Info() + if err != nil { + slog.Warn("Failed to stat stale scan directory", "name", entry.Name(), "error", err) + continue + } + if info.ModTime().After(cutoff) { + continue + } + + // Measured before the removal, because afterwards there is nothing + // left to measure. Best effort: a directory that cannot be walked is + // still worth removing, it just goes unaccounted for. + path := filepath.Join(tmpDir, entry.Name()) + size := dirSize(path) + if err := os.RemoveAll(path); err != nil { + slog.Warn("Failed to remove stale scan directory", "dir", path, "error", err) + continue + } + removed++ + reclaimed += size + } + + slog.Info("Swept scanner tmp dir", + "dir", tmpDir, + "removed", removed, + "bytes", reclaimed, + "older_than", maxAge) +} + +// sweepable reports whether a directory name is one of ours. +func sweepable(name string) bool { + for _, prefix := range sweepPrefixes { + if strings.HasPrefix(name, prefix) { + return true + } + } + return false +} + +// dirSize sums the file sizes under path, ignoring anything it cannot read. +// It exists only to put a number in the sweep's log line. +func dirSize(path string) int64 { + var total int64 + _ = filepath.WalkDir(path, func(_ string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil //nolint:nilerr // an unreadable subtree is skipped, not fatal + } + if info, err := d.Info(); err == nil { + total += info.Size() + } + return nil + }) + return total +} diff --git a/scanner/internal/scan/sweep_test.go b/scanner/internal/scan/sweep_test.go new file mode 100644 index 0000000..e339c48 --- /dev/null +++ b/scanner/internal/scan/sweep_test.go @@ -0,0 +1,148 @@ +package scan + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// staleAge is comfortably past the maxAge every case below sweeps with, so a +// "stale" fixture is stale regardless of filesystem timestamp granularity. +const staleAge = 3 * time.Hour + +// mkStaleDir creates a directory under parent holding one file of the given +// size, then backdates it so the sweep sees it as old. +func mkStaleDir(t *testing.T, parent, name string, size int, age time.Duration) string { + t.Helper() + + path := filepath.Join(parent, name) + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", path, err) + } + if size > 0 { + if err := os.WriteFile(filepath.Join(path, "blob"), make([]byte, size), 0o644); err != nil { + t.Fatalf("write blob in %s: %v", path, err) + } + } + when := time.Now().Add(-age) + if err := os.Chtimes(path, when, when); err != nil { + t.Fatalf("chtimes %s: %v", path, err) + } + return path +} + +// TestSweepTmpDirRemovesOnlyStaleScanDirs is the whole contract of the sweep: +// the three shapes a dead scan leaves behind go, and everything else in the +// tmp dir stays. The preserved entries are not hypothetical — the Grype +// database lives at /vulndb and go-getter unpacks into grype-dl under +// the tmp dir, so a sweep that took either would cost a full DB download on +// every restart. +func TestSweepTmpDirRemovesOnlyStaleScanDirs(t *testing.T) { + tests := []struct { + name string + dir string + age time.Duration + wantGone bool + }{ + {name: "orphaned OCI layout", dir: "scan-1234567890", age: staleAge, wantGone: true}, + {name: "orphaned stereoscope extraction", dir: "syft-scan-1546187834", age: staleAge, wantGone: true}, + {name: "orphaned syft cataloging scratch", dir: "syft-cataloger-99", age: staleAge, wantGone: true}, + {name: "scan a peer may still be running", dir: "scan-inflight", age: time.Minute}, + {name: "grype download dir", dir: "grype-dl", age: staleAge}, + {name: "vulndb alongside the tmp dir", dir: "vulndb", age: staleAge}, + {name: "unrelated directory", dir: "cache", age: staleAge}, + } + + tmpDir := t.TempDir() + for _, tt := range tests { + mkStaleDir(t, tmpDir, tt.dir, 512, tt.age) + } + // A file, not a directory, that matches a swept prefix. Only directories + // are ours; a stray file is somebody else's business. + stray := filepath.Join(tmpDir, "scan-notes.txt") + if err := os.WriteFile(stray, []byte("hello"), 0o644); err != nil { + t.Fatalf("write %s: %v", stray, err) + } + when := time.Now().Add(-staleAge) + if err := os.Chtimes(stray, when, when); err != nil { + t.Fatalf("chtimes %s: %v", stray, err) + } + + sweepTmpDir(tmpDir, time.Hour) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := os.Stat(filepath.Join(tmpDir, tt.dir)) + switch { + case tt.wantGone && err == nil: + t.Errorf("%s survived the sweep; nothing else ever reclaims it", tt.dir) + case !tt.wantGone && err != nil: + t.Errorf("%s was removed by the sweep: %v", tt.dir, err) + } + }) + } + + if _, err := os.Stat(stray); err != nil { + t.Errorf("scan-notes.txt was removed by the sweep: %v", err) + } + if _, err := os.Stat(tmpDir); err != nil { + t.Errorf("the tmp dir itself was removed: %v", err) + } +} + +// TestSweepTmpDirDisabledAndMissing covers the two ways the sweep is asked to +// do nothing. Neither may touch anything, and neither may fail: this runs on +// the startup path, before any worker exists to report an error. +func TestSweepTmpDirDisabledAndMissing(t *testing.T) { + t.Run("zero max age disables the sweep", func(t *testing.T) { + tmpDir := t.TempDir() + stale := mkStaleDir(t, tmpDir, "scan-old", 16, staleAge) + + sweepTmpDir(tmpDir, 0) + + if _, err := os.Stat(stale); err != nil { + t.Errorf("sweep ran with max age 0 and removed %s: %v", stale, err) + } + }) + + t.Run("tmp dir does not exist", func(t *testing.T) { + sweepTmpDir(filepath.Join(t.TempDir(), "never-created"), time.Hour) + }) + + t.Run("empty tmp dir path", func(t *testing.T) { + sweepTmpDir("", time.Hour) + }) +} + +// TestSweepTmpDirContinuesPastAnUnremovableEntry is the "never fatal" half of +// the contract. One directory the process cannot remove must not cost the +// sweep the rest of the reclaim, and must not stop the scanner from starting. +func TestSweepTmpDirContinuesPastAnUnremovableEntry(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } + + tmpDir := t.TempDir() + + // A stale scan dir made read-only, so RemoveAll cannot unlink what is + // inside it and fails partway through, the way a root-owned leftover or a + // busy mount would. + locked := mkStaleDir(t, tmpDir, "scan-locked", 32, staleAge) + if err := os.Chmod(locked, 0o555); err != nil { + t.Fatalf("chmod %s: %v", locked, err) + } + // Restore write permission or t.TempDir's own cleanup fails too. + t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) + + removable := mkStaleDir(t, tmpDir, "syft-scan-removable", 32, staleAge) + + sweepTmpDir(tmpDir, time.Hour) + + if _, err := os.Stat(locked); err != nil { + t.Errorf("expected the unremovable dir to still be there, got: %v", err) + } + if _, err := os.Stat(removable); err == nil { + t.Error("the sweep stopped at the unremovable entry instead of continuing") + } +} diff --git a/scanner/internal/scan/syft.go b/scanner/internal/scan/syft.go index be7dc31..2ae67ec 100644 --- a/scanner/internal/scan/syft.go +++ b/scanner/internal/scan/syft.go @@ -29,7 +29,7 @@ func generateSBOM(ctx context.Context, ociLayoutDir, sourceRef string) (*sbom.SB slog.Info("Generating SBOM with Syft", "ociLayout", ociLayoutDir, "source", sourceRef) // Create stereoscope OCI directory provider - tmpGen := file.NewTempDirGenerator("syft-scan") + tmpGen := file.NewTempDirGenerator(syftTempDirName) defer tmpGen.Cleanup() provider := oci.NewDirectoryProvider(tmpGen, ociLayoutDir) diff --git a/scanner/internal/scan/worker.go b/scanner/internal/scan/worker.go index df5cb06..b5b7ca4 100644 --- a/scanner/internal/scan/worker.go +++ b/scanner/internal/scan/worker.go @@ -61,6 +61,12 @@ func (wp *WorkerPool) Start(ctx context.Context) { os.Setenv("TMPDIR", wp.cfg.Vuln.TmpDir) } + // Reclaim what earlier processes left behind before any worker starts + // filling the directory again. A scan that is interrupted by a restart + // never runs its own cleanup, so without this every restart leaks the + // in-flight layout and extraction permanently. + sweepTmpDir(wp.cfg.Vuln.TmpDir, wp.cfg.Vuln.SweepMaxAge) + // Initialize vuln database on startup if enabled if wp.cfg.Vuln.Enabled { go func() {