atelet: simplify the file cache internals per review - #1605
Closed
Dmitry Berkovich (dberkov) wants to merge 7 commits into
Closed
atelet: simplify the file cache internals per review#1605Dmitry Berkovich (dberkov) wants to merge 7 commits into
Dmitry Berkovich (dberkov) wants to merge 7 commits into
Conversation
Introduce cmd/atelet/internal/filecache, the foundation of a node-local artifact cache: opaque entry keys (content-addressed sha256 and immutable-URI forms), the entries/tmp on-disk layout, a startup sweep for crash debris (unfinished fetches, interrupted evictions), and byte accounting for a GC budget. Golden snapshot restores download their files per actor with no reuse, and sandbox-asset fetches race concurrent downloads of the same asset; this package is the shared cache that will back both paths. Retrieval (singleflight fetch, atomic publication, hardlink-out) and eviction build on this skeleton in follow-up changes.
GetFileTo materializes a cached artifact at a destination path via hard link, fetching it on a miss. Concurrent callers for one key share a single fetch (singleflight), and the fetch runs detached from the callers' contexts bounded by the store's fetch timeout, so one canceled caller never aborts a download other callers are waiting on. There is no negative caching: a failed fetch reaches every waiting caller and the next call starts fresh. A fetch lands in tmp/, must produce a regular file, is made read-only (0444) so a consumer's in-place write fails loudly instead of corrupting the shared copy, and is published with one atomic rename. A hit links out and touches the entry's last-use clock under a shared lock that eviction will hold exclusively, closing the hit-vs-evict window. Destinations must not exist and must be absolute paths on the cache's mount; cross-filesystem destinations fail with a dedicated error rather than a silent copy.
copyFile and its hole-preserving machinery lived in package main, usable only by atelet's own checkpoint staging. The filecache package is about to need the same copy (its copy-out mode hands consumers a private, hole-preserved copy of a cached artifact), so move the code where both can import it. Mechanical move, with one seam added: Copy(src, dst *os.File) exposes the engine on caller-owned handles, for callers that must open the source before its name can vanish or create the destination with O_EXCL. CopyFile keeps its os.Create semantics for the existing caller.
GetFileTo serves hits as read-only hard links, which is only safe for consumers that never write the staged file in place. GetFileCopyTo serves the same read-through cache as a private copy instead: the caller owns the resulting inode outright (mode 0600) and may mutate it freely, holes are preserved, and the destination may live on any filesystem. The copy reads a handle opened under the hit lock, so an eviction racing the copy retires only the entry's name — the bytes survive until the copy completes. Fetch dedup is unchanged: concurrent calls for one key share a single flight.
EvictUnused frees cache space least-recently-used first until a byte target is met, with a two-phase retire: inside the key's singleflight and the hit lock, a victim is re-verified (a moved last-use clock or an in-flight fetch vetoes) and renamed to a .rm-* dir, making it invisible to lookups; the slow physical deletion runs after all retires, outside the locks the hot path contends, so hits and fetches never wait on it. Entries younger than the store's min age are never touched, covering the window between publication and a consumer's first link. Entries whose data a consumer still hard-links may be retired but count as pending rather than freed bytes - the kernel returns that space when the last consumer link goes - so eviction can only ever cost a re-download, never break a consumer. FreedBytes is credited per entry only after its physical removal succeeds; a failed removal leaves the bytes in a .rm-* dir for the startup sweep and out of the freed count.
State the package's consumer-protection contracts in the package doc (link-out immunity, min-age sizing, the read-only shared-bytes rule, and key immutability), and pin them with a race-detector stress test: getters and evictors hammer the same keys concurrently, and every get must succeed with intact content - eviction may force refetches but can never fail a caller, corrupt a served file, or leave half-states in the store.
Remove the createDestFile test seam and the sparseDest indirection: CopyFile creates its destination directly, copySparse writes real files, and the userspace fallback is tested through the public API by copying across filesystems (copy_file_range fails with EXDEV on a /dev/shm destination). Name getTo's serving-mode parameter (serveHit) so the shared loop reads as the strategy split it is, not a test seam. Eviction passes now retry .rm-* leftovers a previous pass failed to remove, so those bytes no longer wait for the next restart's sweep; tmp/ stays startup-swept only, since a later sweep would delete in-flight fetches' working directories. Split filecache.go into doc.go, store.go, and sweep.go, moving the sizing helpers next to their consumer in evict.go, and shorten the package's comments.
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the readability review on #1517, kept as its own PR so that branch stays exactly as reviewed. Stacked on #1517: review only the top commit (
atelet: simplify the file cache internals per review).What it does, mapped to the review comments:
createDestFileseam — removed entirely, along with thesparseDestinterface and theFd()type-assertion it forced:CopyFileopens withos.Create,copySparsewrites real*os.Files. The userspace fallback is now tested through the public API:CopyFilefrom disk to/dev/shm, wherecopy_file_rangegenuinely fails with EXDEV (hard-link probe verifies the mounts differ; skips where no second filesystem exists — platforms without a kernel copy path exercise userspace in the plain holes test already). The close-error test is deleted with the seam; it only existed because the seam made it possible.getTo's parameters — clarified rather than restructured, because neither is a test seam:fetchis the caller's downloader (the production API; M2 binds the GCS fetcher through it), and the second parameter is the link-vs-copy serving strategy shared by the two public methods. It now has a named, documented type (serveHit) so the loop reads as that split..rm-*leftovers a previous pass failed to remove (race-free underevictMu; skipped in dry-run; test included), so stranded bytes no longer wait for a restart.tmp/stays startup-swept only — a later sweep would delete in-flight fetches' working directories — andSweepDebris's doc now says both things explicitly.filecache.gosplit intodoc.go(package contract),store.go(store lifecycle),sweep.go; the sizing helpers moved next to their consumer inevict.go.Kept as-is:
FileFetcherremains a func type — it is the production injection point (call sites bind a GCS client and object URI via closure), and single-method func types are the stdlib idiom for exactly this (http.HandlerFunc,filepath.WalkFunc).Tested: full atelet suite; filecache and sparsefile under
-race -count=3; gofmt, golangci-lint, boilerplate clean.🤖 Generated with Claude Code