fix: expire cache entries unused for a week after a cut by aozmekik · Pull Request #346 · canonical/chisel

aozmekik · 2026-11-04T12:35:44Z

Have you signed the CLA?

Fixes #261.

Nothing calls Cache.Expire, so the cache only grows. Commands that fill the cache now expire it once they are done with it: cut after a successful cut, and debug check-release-archives after fetching the packages. Cache.Open already refreshes an entry's mtime, so only entries nothing has read for a while go away. A failed expiry is logged as a warning.

CHISEL_CACHE_EXPIRY sets the expiry in days, 7 by default, and 0 disables it. Any other value logs a warning and leaves the cache alone.

Expire only removes files chisel wrote: entries named after their digest, and temporary files left by unfinished writes. It carries on past an entry it can't remove, and skips entries that disappear while it runs, so concurrent expiries don't fail on each other. Open no longer fails when an entry is expired right after it was opened.

Tested with go test ./... (and -race -count=30 for the cache and cmd packages), golangci-lint, and the tests/cache-expiry spread task in an ubuntu:24.04 container: it passes on this branch and fails on main.

The cache grows forever since nothing ever calls Cache.Expire. Call it after a successful cut with a one week timeout. Opening an entry refreshes its mtime, so this only removes entries no cut has read in that time, and a failure to expire is logged rather than failing the cut. Expire now also skips entries that disappear while it runs, so two expiries over the same cache don't fail on each other's removals. Fixes canonical#261

lczyk

hi. cheers for the pr. :) let me kick things of by doing a first-pass review.

lczyk · 2026-11-04T18:15:59Z

 			err = os.Remove(filepath.Join(digestKindDir, finfo.Name()))
-			if err != nil {
+			if err != nil && !os.IsNotExist(err) {
 				return fmt.Errorf("cannot expire cache entry: %v", err)


if an entry can't be removed it stops the rest of expiry. e.g. a stale non-empty dir that sorts first gives cannot expire cache entry: ... directory not empty and every stale file after it stays unreclaimed.

Good catch. It now carries on past an entry it can't remove and returns the first error once it has tried everything, so one bad entry doesn't keep the rest around (791b62b). With the change from the other thread, directories aren't considered at all anymore.

lczyk · 2026-11-04T18:15:59Z

 			if finfo.ModTime().After(expired) {
 				continue
 			}
 			err = os.Remove(filepath.Join(digestKindDir, finfo.Name()))


this removes anything old under sha*/, not just what chisel wrote. we know exactly what files we wrote so maybe its better to be more selective here(?)

Agreed. It only looks at regular files named like a digest of that kind now, plus <digest>.tmp and tmp.N, the leftovers of a write that didn't finish, since those are ours too and nothing else ever cleans them up. Anything else under sha*/ is left alone (791b62b).

lczyk · 2026-11-04T18:15:59Z

+	// Reading an entry refreshes its timestamp, so this only drops what no
+	// cut has used for a while. Failing to clean up is not a failed cut.
+	cc := &cache.Cache{Dir: cacheDir}
+	if err := cc.Expire(cacheExpiry); err != nil {


note. this is the first time cut touches files outside of --root. kinda obvious but also could be detrimental for some use cases (e.g. pre-warming cache). i would personally very much like something like CHISEL_CACHE_EXPIRY to e.g. control the expiry time and CHISEL_CACHE_EXPIRY=0 to disable (and i guess for now safest is to ignore others aka ignore anything thats not positive integer or 0).

Makes sense, added CHISEL_CACHE_EXPIRY in a6f1417: a number of days, 7 when unset, and 0 disables it. For any other value I read "ignore" as "don't act on it": it logs a warning and skips expiry instead of falling back to 7 days, so someone who sets e.g. off to protect a pre-warmed cache doesn't lose it. Happy to fall back to the default instead if you prefer.

It's also mentioned in chisel help cut. I can add it to chisel-docs once the name and behaviour are settled.

lczyk · 2026-11-04T18:15:59Z

 			return fmt.Errorf("cannot list cache directory: %v", err)
 		}
 		for _, entry := range entries {
+			// Entries may vanish while listing, e.g. renamed from .tmp by


small race: Open does os.Open and then Chtimes. thich gives a timing window for a race from a parallel process between the two and can lead to the cut dying with cannot update cached file timestamp. likely around us window, and i have not been able to reproduce it, but its technically there and so noting it here as part of my ongoing attempt to make chisel more concurency-friendly.

Right, the open file is still fine to read if the entry gets removed between os.Open and Chtimes, so Open now ignores ENOENT from Chtimes. It also closes the file on the other errors, which it didn't before (c053a58). I didn't add a test for this one, I couldn't hit that window without adding a hook to Open just for it.

lczyk · 2026-11-04T18:15:59Z

+		c.Assert(err, IsNil)
+	}
+
+	// Expiries running at the same time remove entries the others have


this is probabilistic since a slow runner can schedule gorutines differently.

Yeah. Replaced it with TestCacheExpireVanishedEntry, which removes the entry itself between listing and stat, and between stat and remove, so nothing depends on scheduling (791b62b).

lczyk · 2026-11-04T18:15:59Z

+	}
+
+	// Reading an entry refreshes its timestamp, so this only drops what no
+	// cut has used for a while. Failing to clean up is not a failed cut.


Suggested change

// cut has used for a while. Failing to clean up is not a failed cut.

// Reading an entry refreshes its timestamp, so this only drops what no

// cut has used for a while.

nit: second sentence restates the logf below.

Done. The comment moved into the shared expireCache helper with the call, so it now says "no command" (a6f1417).

lczyk · 2026-11-04T18:15:59Z

 }

+// cacheExpiry is how long a cache entry can go unused before a cut removes it.
+const cacheExpiry = 7 * 24 * time.Hour


hmmm... find / info / debug check-release-archives fill the cache too and never expire it. if we decide to expire the cache should we have that expiry hook in every command which touches it...?

Good point for debug check-release-archives: it fetches every package of the release, so it now calls the same helper once it has them (a6f1417). find and info only go through FetchRelease, which keeps one releases/<name> directory per release and replaces it in place, so they never add to sha*/ and there's nothing for them to expire.

Expire removed anything old under the digest directories, and the first entry it could not remove stopped the whole run. It now only looks at regular files named like a digest of that kind, or like the temporary files of an unfinished write, and returns the first failure after trying every entry. The concurrent expiry test depended on scheduling. It is replaced by a test that removes an entry between listing and removal on purpose.

Open refreshes the timestamp of an entry after opening it. If another process expires the entry in between, Chtimes fails and so did the fetch, although the open file is still readable. Ignore that case, and close the file on the other failures.

CHISEL_CACHE_EXPIRY sets how many days a cache entry can go unused, 7 by default, and 0 disables expiry. Any other value logs a warning and leaves the cache alone, since deleting is the one thing that can't be undone. debug check-release-archives fetches every package of a release, so it now expires the cache too. find and info only refresh the release directory, which is replaced in place and does not grow. The command tests point XDG_CACHE_HOME at a temporary directory, so they never expire the cache of whoever runs them.

aozmekik · 2026-11-04T19:26:54Z

Thanks for the review! I pushed three commits for it and replied inline:

791b62b: Expire only touches files chisel wrote and keeps going past failures. The probabilistic test is replaced with a deterministic one.
c053a58: Open no longer fails when an entry is expired right after it's opened.
a6f1417: CHISEL_CACHE_EXPIRY, and debug check-release-archives expires too. The command tests now use a temporary XDG_CACHE_HOME, so they never touch the real cache.

Tested with go test ./..., -race -count=30 on the cache and cmd packages, golangci-lint, and the spread task in ubuntu:24.04 (it passes here and fails on main).

aozmekik added 2 commits November 4, 2026 15:10

test: look for the deb in every digest directory

27e0a3a

lczyk suggested changes Nov 4, 2026

View reviewed changes

aozmekik added 3 commits November 4, 2026 22:14

	// cut has used for a while. Failing to clean up is not a failed cut.
	// Reading an entry refreshes its timestamp, so this only drops what no
	// cut has used for a while.

Conversation

aozmekik commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aozmekik commented Nov 4, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aozmekik commented Nov 4, 2026 •

edited

Loading