Fix cached-downloader bugs and implement periodic download_cache cleanup rather than waiting for the next download

[geofffranks]

• Read the Contributing document.

Summary

Fixes 4 issues:

1. When staging containers run for apps that don't specify a buildpack, many resources are downloaded + cached. This can cause the cache to exceed it's configured size limit until the next cleanup is run, which used to only happen the next time an uncached asset needed to be downloaded. Now we will check minutely as well to prune faster when over the limit.
2. Fix DownloadCachedDependencies losing late-arriving successful mounts when an earlier download fails (mountChan wasn't drained after the error path).
3. Fix unused OldEntries never being deleted from disk in FileCache.CloseDirectory.
4. Fix a failed tar extraction leaving a stale ExpandedDirectoryPath, permanently blocking retry.

Backward Compatibility

Breaking Change? no

[navinms711]

Some comments from claude, that might be worth considering.

---

Comment 2 — RecoverState + periodic runner startup race

Location: cacheddownloader.go / new cachecleanup/runner.go

RecoverState is called during executor startup (initializer_garden.go:149). The new periodic runner starts as part of the executor's ifrit Members group, which launches goroutines concurrently. If the cleanup runner fires its first MakeRoom() tick before RecoverState finishes, makeRoom iterates over a partially-initialized Entries map (or an empty one from a prior state load that isn't done yet).

makeRoom already holds lock when it runs, and RecoverState modifies c.cache.Entries without acquiring lock (it runs before the runner goroutines start). If the runner's first tick happens during or just after RecoverState completes but before the caller of RecoverState signals readiness, the timing window is narrow but real. Recommend ensuring the cleanup runner's ifrit.Runner is not added to the group until after RecoverState returns — or have the runner skip its first tick until an explicit "ready" signal is received.

---

Comment 3 — MakeRoom() with size=0 is a no-op when cache is within limit

Location: file_cache.go:385 / periodic runner

The periodic call passes size=0, so makeRoom only evicts when usedSpace > maxSizeInBytes. In normal operation (cache within its configured limit), every 1-minute tick is a full lock acquisition + usedSpace scan with zero effect. This isn't harmful at current fleet sizes, but the PR description says "faster pruning" — which implies entries are being pruned more eagerly. In practice this only helps when the cache is perpetually over the limit, which shouldn't happen in steady state if the limit is configured correctly.

If the goal is to proactively free entries that haven't been accessed recently (e.g., entries from apps that no longer run), the eviction criterion should also include an access-time threshold (TTL), not just size pressure. Otherwise the "periodic cleanup" only acts as a size-limit enforcer on a slower polling cycle, which the existing download-triggered makeRoom already handles.

---

Comment 4 — Package-level lock increases contention surface with new goroutine

Location: file_cache.go:18

var lock = &sync.Mutex{}

lock is a package-level variable, not a field on FileCache. All FileCache instances in the process share the same mutex. Adding a continuously-running goroutine that acquires lock every 60 seconds means any download operation during that window must wait. At current frequencies this is negligible, but it's worth noting if FileCache is ever instantiated more than once per process (e.g., in tests or in a future multi-tenant executor). A per-instance mutex on FileCache would eliminate the coupling entirely.

---

Comment 5 — OldEntries cleanup in CloseDirectory: verify file deletion is safe under concurrent reads

Location: file_cache.go:CloseDirectory

The fix deletes files from disk when an OldEntries entry's inUse count reaches zero. On Linux, unlink/RemoveAll on a file that has open file descriptors is safe — the file content remains accessible to existing readers until their descriptors are closed, and only the directory entry is removed. If this code runs on Windows (the PR also includes rep_windows spec changes), the behavior is different: Windows will return an error if the file is open. Confirm that the deletion path in CloseDirectory is either Linux-only or handles the Windows-open-file case explicitly.

[geofffranks]

Thanks -

Comment 2 — RecoverState + periodic runner startup race

The reviewer's mental model of the startup sequence is wrong. Tracing the actual call chain:

1. initializer_garden.go:145 — cachedDownloader.RecoverState(logger) runs synchronously inside Initialize(). If it errors, Initialize() returns early (line 147: return nil, nil, grouper.Members{}, err).
2. Initialize() returns the grouper.Members at line 307–308 — these members haven't started yet, they're just a list of ifrit.Runner structs.
3. Back in rep/cmd/rep/main.go:253 — the cache-cleanup runner is appended to those members.
4. The ifrit group is constructed and .Run() is called after Initialize() has fully returned.

RecoverState completes before Initialize() returns. The ifrit group containing the cleanup runner doesn't start until after Initialize() returns. There is no timing window — RecoverState and the runner's first tick cannot overlap. The sequence is strictly serial: RecoverState → return Members → construct group → start group → first tick (≥60s later).

Comment 3 — MakeRoom() with size=0 is a no-op when cache is within limit

The goal isn't to proactively free things that aren't recent, it's to proactively free things that are over the limit and wouldn't be cleaned up until the next LRP/Task download. Don't want to introduce new behaviors to how the eviction logic happens, just when it happens.

Comment 4 — Package-level lock increases contention surface with new goroutine

Not worried about this, we won't be making it happen multiple times per node or more frequently. the current contention happens only if downloading a new asset for a new LRP or Task is happening at the exact moment this is checking. Since those paths already called makeRoom, it's not like they see any new time from this (locking/calculating isn't the bottleneck, cleaning is).

Comment 5 — OldEntries cleanup in CloseDirectory: verify file deletion is safe under concurrent reads

This would only affect Windows, but I don't want to introduce new behaviors to how the eviction logic happens, just when it happens. We've been using this logic for many years without running into issues with this.