# Answer key: proxy cache reclamation | # | tier | summary | severity | Category | | :-- | :--- | :--------------------------------------------------------------------------- | :--------- | :-------------- | | 1 | 1 | Sorting ascending by age evicts the newest blobs, not the oldest | issue | defect | | 2 | 1 | A failed unlink still counts its bytes as reclaimed | issue | defect | | 3 | 1 | `>=` against the ceiling removes a blob from a store that is inside it | suggestion | defect | | 4 | 2 | `fs::metadata` follows links, so the sweep deletes outside the store | blocking | security | | 5 | 2 | A future mtime becomes an age of zero, so those blobs are never evicted | issue | defect | | 6 | 2 | The pass is never handed to `spawn_blocking`, so it holds a runtime worker | issue | performance | | 7 | 2 | `tokio::time::interval` fires immediately, so every restart sweeps | issue | defect | | 8 | 2 | `MissedTickBehavior::Delay` leaves no gap after a sweep that overran | suggestion | performance | | 9 | 2 | Every blob in the store is materialised into one `Vec` before any deletion | suggestion | performance | | 10 | 2 | A malformed `dry_run` is rejected before auth, outside the error contract | suggestion | security | | 11 | 3 | The route calls the unguarded `sweep`, bypassing the exclusivity lock | issue | defect | | 12 | 3 | A dry run does not delete blobs but does delete their index entries | blocking | defect | | 13 | 3 | The two new metric counters are rendered under each other's names | issue | defect | | 14 | 3 | Age is the write time, so eviction is by insertion order and not by use | issue | defect | | 15 | 3 | The three new variables are absent from `docker-compose.yml` | issue | maintainability | | 16 | 3 | The runbook recommends the dry run, which is the one call that corrupts | suggestion | maintainability | | 17 | 4 | Clearing `incoming` unlinks partials that live fetches and `link` are using | blocking | defect | | 18 | 4 | The unreferenced prune deletes a blob committed but not yet linked | blocking | defect | | 19 | 4 | Two proxies on one blob directory over-reclaim and double-count | issue | defect | | 20 | 4 | Eviction leaves `versions.cached_at` set, so the dashboard lies | issue | defect | ## 1. Sorting ascending by age evicts the newest blobs **Tier 1** · `services/proxy/src/sweep.rs:102` · `issue` · `defect` The comment above the sort says oldest first. `Candidate::age` is how long ago the blob was written, so ascending order puts the smallest age - the blob written most recently - at the front, and the loop consumes the vector from the front. **Trigger:** a store over its ceiling holding one blob written a week ago and one written a minute ago. The minute-old one is deleted and the week-old one stays. The cache throws away what it has just fetched and keeps what nobody has wanted for a month, so the hit rate gets worse the harder the proxy is working. **What a good review says:** reverse the ordering, or sort on the modification time rather than on the age derived from it. **Why it is hard:** `sort_by_key(|candidate| candidate.age)` is what you write when you mean "sort by age". Catching it needs the reader to hold in mind that a larger age means an older blob, against a comment that has already told them the code is doing the right thing. ## 2. A failed unlink still counts its bytes as reclaimed **Tier 1** · `services/proxy/src/sweep.rs:125-137` · `issue` · `defect` `remove_file` failing is logged at debug and then the four lines that account for the removal run anyway: `remaining` drops, `bytes` grows, `removed` grows, and the digest goes onto `gone`. **Trigger:** a blob directory remounted read-only, or one whose leaf directories have lost write permission. Every unlink fails with `EROFS` or `EACCES`, `remaining` still falls below the ceiling so the loop stops taking candidates, the route answers with a byte total, and nothing was freed. `cairn_proxy_sweep_bytes_reclaimed_total` counts bytes that are still on disk, so the one metric an operator would watch is the one hiding it. **What a good review says:** only account for the removal when `remove_file` succeeded, treating `NotFound` as success. **Why it is hard:** the comment gives a real and common reason for the error - a purge, or another proxy got there first - and for that reason ignoring it is correct. It is the accounting sitting after the `if let Err` rather than inside the success path that is wrong, and nothing draws the eye to it. ## 3. The ceiling comparison removes a blob from a store already inside it **Tier 1** · `services/proxy/src/sweep.rs:120` · `suggestion` · `defect` `config.rs:63` documents `cache_max_bytes` as "the most the blob store may hold", so a store holding exactly that much is inside its limit. The loop removes while `remaining >= self.max_bytes`. **Trigger:** a store holding exactly `CAIRN_CACHE_MAX_BYTES`. One blob is deleted that should have stayed. **What a good review says:** compare with `>`, so the ceiling is a size the store is allowed to reach. **Why it is hard:** it is one character, in a condition that also carries the age check, and the cost of being wrong is one blob. Worth finding, not worth blocking on. ## 4. `fs::metadata` follows links, so the sweep deletes outside the store **Tier 2** · `services/proxy/src/sweep.rs:160-169` · `blocking` · `security` The comment above the call says a link is counted at the size of the link and not of what it points at. `fs::metadata` is the call that follows a symlink; `symlink_metadata` is the one that does not. The size accounting is the least of it: `meta.is_dir()` on the followed metadata is true for a symlink to a directory, so `collect` recurses through the link and out of the store. Every file it finds out there has a name that is not a digest in the index, so `!index.contains_key(hex)` is true, `remove` is true, and `fs::remove_file` unlinks it. Age never enters the unreferenced branch, so there is no grace period to save it. **Trigger:** one symlink to a directory anywhere under `blobs/sha256`. The comment names the case itself - a store moved between volumes with blobs linked in rather than copied - so linking a shard in from the old volume during exactly that migration means the first sweep deletes files on the old volume. A link pointing somewhere else entirely, say `/etc`, is a sweep that deletes files under `/etc`; a link pointing at an ancestor of the blob directory is unbounded recursion until the stack goes. The link itself is skipped by the `continue` after recursing, so it survives and the next sweep does it again. Verified: a store with one symlink to a directory holding two files reported `scanned: 3, removed: 3`, and both files outside the store were gone. **What a good review says:** use `symlink_metadata`, and skip anything that is not a regular file. **Why it is hard:** the two calls differ by one word, and the comment directly above asserts the behaviour of the one that was not used. The reasoning step is small even though the consequence is large, which is why this stays at tier 2 - a reviewer who spots the wrong call has the whole defect. Score a review that reports only the size accounting as a partial find; the deletion outside the store is the part that matters. **Hard links, for completeness:** the comment is wrong about those too - a hard link reports the full size of the shared inode, so the total double-counts and unlinking one link frees nothing until the last one goes. `symlink_metadata` does not fix that, because a hard link is the file; only tracking inodes or `st_nlink` would. A review raising hard links has this entry, but the recommended fix does not cover them, so do not require it. ## 5. A future mtime becomes an age of zero, so those blobs are never evicted **Tier 2** · `services/proxy/src/sweep.rs:185-190` · `issue` · `defect` `duration_since` returns an error when the timestamp is ahead of the clock, and `unwrap_or_default()` turns that error into `Duration::ZERO`. A zero age never satisfies `candidate.age >= self.min_age` for any non-zero minimum, so those blobs cannot be evicted by the ceiling rule at all. **Trigger:** a blob directory on a volume whose server clock runs ahead of the proxy's - an NFS mount a few seconds out, or a container host that has just stepped its clock. Every freshly written blob is exempt while the clock is ahead; on a volume minutes ahead it is the whole store. Verified: with both blobs' mtimes set 60 seconds into the future and a one-second minimum age, a sweep over a store at four times its ceiling reported `removed: 0`. The sweep runs on schedule, reports nothing reclaimed, and the disk fills. **What a good review says:** treat an unreadable or future mtime as old rather than new, so a clock problem cannot make a blob permanently exempt. **Why it is hard:** the doc comment argues the case for flattening the error, and the argument is sound - a sweep that refuses to run over one odd timestamp is worse than one that copes. The bug is the direction it flattens in, which the comment never mentions, and the symptom is a sweep that quietly does nothing rather than one that fails. ## 6. The pass is never handed to `spawn_blocking`, so it holds a runtime worker **Tier 2** · `services/proxy/src/sweep.rs:14-18`, `services/proxy/src/routes/admin.rs:76`, `services/proxy/src/main.rs:116` · `issue` · `performance` Every filesystem call in the module is `std::fs`, and both entry points are reached from async contexts with no await point anywhere inside the pass. The synchronous calls are not themselves the defect - see *Not defects* - but nothing moves the pass off the async workers. **Trigger:** a sweep over a populated store while requests are in flight. A walk of `blobs/sha256` plus every index entry is tens of thousands of `readdir` and `stat` calls, and one tokio worker is blocked for all of it, so every request scheduled onto that worker stalls. On the on-demand route the caller's own request is the one holding the thread. **What a good review says:** wrap the pass in `spawn_blocking`, which keeps the cheap synchronous calls and takes them off the async workers. **Why it is hard:** the module documentation makes a real argument against `tokio::fs`, and it is correct as far as it goes - dispatching each call to the blocking pool individually would be worse. It presents two options and picks the better of them, and the right answer is a third the comment never raises. ## 7. `tokio::time::interval` fires immediately, so every restart sweeps **Tier 2** · `services/proxy/src/main.rs:101`, `services/proxy/src/main.rs:108` · `issue` · `defect` The doc comment says the first tick is one interval away, and gives the reason: a proxy that has just started should not sweep a cache it has not begun to fill. `tokio::time::interval` completes its first tick immediately. **Trigger:** any process start. Verified against the built binary: with `CAIRN_SWEEP_INTERVAL=1h`, the log line "swept the blob store" lands about two milliseconds after "proxy starting". A proxy that is crash-looping or being rolled sweeps once per restart regardless of the configured interval, and each of those passes takes the store to the ceiling and drops the index entries for what it evicted. **What a good review says:** use `interval_at` with a start instant one period out, or await the period before the first pass. **Why it is hard:** it is a property of the constructor rather than of any line in the diff, the comment states the opposite with a reason attached, and the `set_missed_tick_behavior` call on the next line makes the block read as though somebody had already thought carefully about the timer. ## 8. `MissedTickBehavior::Delay` leaves no gap after a sweep that overran **Tier 2** · `services/proxy/src/main.rs:109-112` · `suggestion` · `performance` The comment has two claims and the second is wrong. `Delay` does suppress the catch-up burst, which is the first claim. But it does not make "the next one start a full interval after this one finished": `tick` returns ready the moment it is polled with an elapsed deadline, and only then schedules the next one at `now + period`. So when a sweep runs longer than the interval, the deadline has already passed by the time the loop comes back round, and the next sweep starts immediately. **Trigger:** a sweep that takes longer than `CAIRN_SWEEP_INTERVAL`, which is the large store this feature exists for. Verified with a 100 ms period and 250 ms of work under `Delay`: the gaps between one pass finishing and the next starting were 2.0, 2.6 and 2.1 microseconds. Sweeps then run back to back for as long as the store stays big, which turns defect 6 from a stall into a permanently occupied worker thread. **What a good review says:** `Skip` gets closer to the comment, and a `sleep(period)` at the end of the loop body is what actually gives a gap measured from when the pass finished. **Why it is hard:** the first half of the comment is true and checkable, which lends the second half credibility. Distinguishing them means knowing that `Delay` reschedules from the moment of the late tick rather than deferring it, and the difference only shows up under an overrun. ## 9. Every blob in the store is materialised into one `Vec` before any deletion **Tier 2** · `services/proxy/src/sweep.rs:94-102`, `services/proxy/src/sweep.rs:153` · `suggestion` · `performance` `collect` pushes a `PathBuf`, a `u64` and a `Duration` for every blob in the store, with no cap, and the sort needs the whole vector before the loop can start. `referenced` holds a second structure over every index entry, and both are alive at the same time. **Trigger:** a store at the 32 GiB default ceiling full of small npm tarballs - on the order of several hundred thousand blobs, so roughly a hundred megabytes of paths and metadata, plus the index map beside it. The service is careful about exactly this elsewhere: the fetch path streams in 64 KiB chunks and `store.rs` says why, so that a hundred concurrent downloads do not each hold a megabyte. **What a good review says:** bound it - keep only the oldest N candidates in a heap, or sweep one `blobs/sha256` shard at a time. **Why it is hard:** it is a scaling defect with no wrong output, it only shows up on a store far larger than any test would build, and materialising does look necessary because the candidates have to be sorted. ## 10. A malformed `dry_run` is rejected before auth and outside the error contract **Tier 2** · `services/proxy/src/routes/admin.rs:51-56`, `services/proxy/src/routes/admin.rs:61-66` · `suggestion` · `security` `Query` runs as an extractor, so its rejection is returned before the handler body executes - which means before `authorise`. The rejection is axum's own, not this service's. **Trigger:** `POST /v1/admin/cache/sweep?dry_run=1` with a valid token answers `400` with `content-type: text/plain` and the body `Failed to deserialize query string: dry_run: provided string was not 'true' or 'false'`. Every other failure in the service is `application/problem+json` carrying a status, a stable code and a request id, which is what `error.rs` exists to guarantee. Verified against the running binary, including that the same request with no token also answers `400` rather than `401`, so the parameter's name and type are readable without a credential. **What a good review says:** take the flag as `Option` and parse it in the handler, or attach a rejection handler that renders the same problem document, so this route's failures look like the rest of the service's. **Why it is hard:** the handler looks complete and `authorise` is its first line, so the ordering only becomes visible once you know extractors run before the body. A reviewer who reports either half - the wrong error shape, or the parameter being reachable before the credential check - has this one. ## 11. The route bypasses the lock that makes a sweep exclusive **Tier 3** · `services/proxy/src/sweep.rs:65-67`, `services/proxy/src/sweep.rs:82-90`, `services/proxy/src/routes/admin.rs:76` · `issue` · `defect` `Sweeper::run` takes the `running` guard, and the field's own comment says the lock is what keeps the one-at-a-time promise from being something callers have to remember. The route does not call `run`. It calls `sweep` directly, because `run` takes no `dry_run` and gives the handler nothing to vary. **Trigger:** `POST /v1/admin/cache/sweep` issued while the interval's sweep is in progress - on a 15 minute interval over a large store that is a wide window, and it is exactly when an operator watching free space fall would reach for the route. Both passes compute `held` from the same starting total and both remove that much, so the store lands well under the ceiling and both count the same bytes. **What a good review says:** put the guard inside `sweep`, or give `run` the `dry_run` argument and make `sweep` private. **Why it is hard:** neither line is wrong alone. The lock is real, `run` uses it correctly, and the route's call is the only signature that fits what the route needs. What makes it wrong is a promise made in a field comment sixty lines away, and there is no comment at the call site to contradict. ## 12. A dry run leaves the blobs and deletes their index entries **Tier 3** · `services/proxy/src/sweep.rs:125`, `services/proxy/src/sweep.rs:140`, `services/proxy/src/sweep.rs:249` · `blocking` · `defect` `dry_run` guards `remove_file` on the blob and is honoured in `clear_partials`. `forget` never receives it. `gone` holds every digest the pass decided to remove - in a dry run, every digest it *would* have removed - so `forget` unlinks the index entries for blobs that are still on disk. **Trigger:** `POST /v1/admin/cache/sweep?dry_run=true` against a store over its ceiling. Verified: the blobs all survive and the index entry naming the evicted digest is gone. The cache then misses on coordinates whose bytes it is holding, and every one of those misses is a fetch from an upstream, so a caller trying a low ceiling drops most of the index and sends the proxy's whole fetch volume upstream - which is the amplification `admin.rs` opens by naming as the reason these routes need a credential. **What a good review says:** pass `dry_run` into `forget` and return early, or move the index pruning inside the branch that actually removed the blob. **Why it is hard:** the flag is threaded through and honoured in two of the three places that delete something, so a reader checking that dry run is wired up finds it wired up. `forget`'s own doc comment is a well-argued justification for pruning index entries, and it is correct - about the non-dry-run case, which is the only one it discusses. ## 13. The two new counters are rendered under each other's names **Tier 3** · `services/proxy/src/metrics.rs:50-57`, `services/proxy/src/metrics.rs:94-95` · `issue` · `defect` `render` zips `COUNTERS` with `values()` positionally. `COUNTERS` lists `blobs_removed` then `bytes_reclaimed`; `values()` loads `sweep_bytes_reclaimed` then `sweep_blobs_removed`, following the struct's field order. **Trigger:** any sweep. Verified: `record_sweep(3, 4096)` renders `cairn_proxy_sweep_blobs_removed_total 4096` and `cairn_proxy_sweep_bytes_reclaimed_total 3`. Every dashboard and alert on either number reads the other, and `docs/operations.md` tells operators to watch `cairn_proxy_sweep_bytes_reclaimed_total` by name. **What a good review says:** put the two arrays in the same order - the existing comment above `COUNTERS` already says that is the invariant. **Why it is hard:** each array is internally consistent and each ordering is defensible alone; `values()` matches the struct and `COUNTERS` matches the order somebody would describe the counters in. The coupling lives in `render`, which the diff does not touch. The comment naming this exact failure is four lines above one of the two arrays, which makes it findable and also makes it read as already handled. ## 14. Age is the write time, so eviction is by insertion order and not by use **Tier 3** · `services/proxy/src/sweep.rs:186`, `services/proxy/src/sweep.rs:100-101`, `services/proxy/src/sweep.rs:10-12` · `issue` · `defect` `age_of` reads `meta.modified()`, which is when the blob was written. Serving a blob does not touch it - `BlobStore::open_blob` opens the file read-only and reads a length - so nothing on the serve path moves an mtime. Verified: opening a blob and reading every byte of it leaves `modified()` unchanged. The module documentation and the sort comment both describe the ordering as use: "the blobs nothing has wanted for longest", "the package somebody fetched this morning stays". The pull request body says the same. What the code implements is eviction by insertion order. **Trigger:** a dependency fetched a year ago and served on every build since, against one fetched yesterday and never read again. The year-old one is evicted first, and it is the one every build needs. On a long-lived proxy that inverts the policy across the whole working set. **What a good review says:** record a use time - touch the blob, or keep an access record, when it is served - or stop describing this as least-recently-used and accept it as first-in-first-out. **Why it is hard:** mtime is the obvious thing to reach for and it *is* the age of the blob, so the code is not wrong about what it computes, only about what that means. Finding it means noticing that the diff claims use, then checking that nothing on the serve path updates the field the claim rests on. ## 15. The three new variables are absent from `docker-compose.yml` **Tier 3** · `.env.example:41`, `.env.example:43-44`, `docs/operations.md:51` · `issue` · `maintainability` The diff adds `CAIRN_CACHE_MAX_BYTES`, `CAIRN_CACHE_MIN_AGE` and `CAIRN_SWEEP_INTERVAL` to `.env.example` and writes a runbook telling an operator to set the ceiling. `docker-compose.yml` enumerates the proxy's settings one by one in its `environment:` block at lines 78 to 89, and there is no `env_file` anywhere in the file, so a container gets only what that block lists. **Trigger:** set `CAIRN_CACHE_MAX_BYTES=1GiB` in `.env`, run `mise run up`, and the proxy sweeps to 32 GiB anyway, because the variable never reaches the container. Following the documentation this change adds has no effect on the stack the documentation is written for. `CAIRN_LOG_LEVEL: ${CAIRN_LOG_LEVEL:-info}` two lines below shows the pattern that would have worked. **What a good review says:** add the three variables to the proxy service's `environment:` block, interpolated with defaults the way `CAIRN_SERVICE_TOKEN` and the log settings are. **Why it is hard:** the Rust is correct and reading it tells you nothing. It needs knowing that compose does not pass a whole `.env` into a container, and then noticing that a file the diff does not touch is the reason a feature the diff documents is not configurable. ## 16. The runbook recommends the dry run, which is the one call that corrupts **Tier 3** · `docs/operations.md:51` · `suggestion` · `maintainability` The runbook this change adds says to try a new ceiling with `?dry_run=true` before writing it into the environment. Because of defect 12 that is the call which leaves every blob in place and deletes the index entries for the ones it would have evicted, so following the runbook is how an operator would trigger the worst behaviour in the patch. **Trigger:** an operator pasting the recommended request on a store over its ceiling. Nothing is reclaimed, so the space problem is unchanged, and the index loses an entry per blob the pass selected, so the next build refetches all of them. **Scoring:** this is not independent of defect 12 - it exists only because `forget` ignores the flag. Score it separately only when a review flags the runbook line without identifying that `forget` is unguarded. A review that reports 12 has already covered the mechanism, so do not count both against a single observation. **Why it is hard:** the sentence is good advice about a correct implementation, and the documentation is the last place a reviewer looks for the consequence of a bug in the Rust. ## 17. Clearing `incoming` unlinks partials that live fetches and `link` are using **Tier 4** · `services/proxy/src/sweep.rs:219-236` · `blocking` · `defect` `clear_partials` removes every file in `incoming` with no age check at all. `incoming` is where `BlobStore::temp_path` puts every in-progress write, and there are two of those, not one. **Trigger, the download:** `BlobWriter` holds its partial open for the whole of a fetch, so a sweep during any download unlinks it. Unlinking an open file does not fail the writes, so the fetch continues normally and then `BlobWriter::commit` renames from a path that no longer exists and fails with `NotFound`. Verified: unlinking an open file, writing more to the handle, syncing and then renaming returns `NotFound`. For a 200 MiB artifact over a slow upstream that window is minutes wide, so on a busy proxy it is several fetches lost per sweep, every `CAIRN_SWEEP_INTERVAL`. **Trigger, the index write:** `BlobStore::link` also goes through `incoming` - `store.rs:147-152` writes the encoded entry to `temp_path()` and renames it into the index. A sweep between those two calls fails the rename, so a fetch that downloaded, hashed, registered and verified correctly returns a 500 at the last step. That window is short, but it is on every single cache miss. **What a good review says:** only remove a partial older than some generous threshold - longer than `CAIRN_FETCH_TIMEOUT` - so no write in progress can be one of them. **Why it is hard:** the doc comment argues that a partial needs no age check, and it is nearly right: a partial carries no digest and nothing can look one up. The step it misses is that the process holding it open is about to give it a name, and that fact lives in `store.rs` rather than anywhere in this diff. The `link` window is harder still, because nothing about the name `incoming` suggests index entries pass through it. A review that finds either window has this entry; finding both is a strong review. ## 18. The unreferenced prune deletes a blob committed but not yet linked **Tier 4** · `services/proxy/src/sweep.rs:114-120` · `blocking` · `defect` A blob is removed when no index entry names it, and the comment is explicit that age does not enter that decision. But `routes/cache.rs::fetch` commits the blob at line 151, calls `register_version` at 155, verifies the digest at 161, and only then calls `store.link` at 163. Between the commit and the link the blob is on disk with nothing pointing at it. **Trigger:** a sweep landing in that window, which is a whole registry round trip wide. It is wider than that, and this is the part that makes it a mark-and-sweep bug rather than a plain race: `sweep` builds the index map at line 92 *before* listing the blobs at line 95, so a blob whose `link` completed after the snapshot but before the listing is still treated as unreferenced and deleted. `fetch` then fails at the `open_blob` on line 90 with the error already sitting there for this case: "a blob committed by this request is already missing from the store". That message was written for an operator deleting by hand; this change makes the proxy do it to itself. **What a good review says:** exempt blobs younger than `cache_min_age` from the unreferenced prune too, or re-read the index entry immediately before unlinking. **Why it is hard:** the reasoning in the comment is sound for every blob that is not being written right now, and unreachable-so-remove is the correct rule for a content-addressed store. Seeing the hole means opening `cache.rs`, noticing that commit and link are separated by a network call, and recognising that the existing error string is a description of this bug. ## 19. Two proxies over one blob directory over-reclaim and double-count **Tier 4** · `services/proxy/src/sweep.rs:10-12`, `services/proxy/src/sweep.rs:65-67` · `issue` · `defect` The module documentation states the hazard - two sweeps deciding what to remove from a total each is changing take the store far below the ceiling - and answers it with an in-process `Mutex`. `store.rs::temp_path` says two proxies sharing a blob directory is a deployment the store is built for, and its temp names carry the process id so that it works. A mutex cannot span processes. **Trigger:** two proxies on one volume, both on the default 15 minute interval, sweeping a store at 1.5 times the ceiling. Each reads `held`, each removes `held - ceiling`, and the store lands at `2 × ceiling - held` - half the intended cache. Both also call `record_sweep` for the same bytes, so the fleet's reclaimed total is roughly double the truth. **What a good review says:** take a lock the filesystem can enforce - a lock file or a lease under the blob directory - or recheck the size as the loop goes rather than deciding everything up front. **Why it is hard:** the code names the risk and appears to have handled it, and for a single proxy it has. What makes the guard insufficient is a supported deployment shape recorded in a comment in a different file, and the symptom is a cache smaller than configured rather than anything that errors. ## 20. Eviction leaves `versions.cached_at` set, so the dashboard lies **Tier 4** · `services/proxy/src/sweep.rs:125-137`, `docs/operations.md:53` · `issue` · `defect` Nothing in the sweep tells the registry that a version's bytes are gone. `versions.cached_at` is what the dashboard reads to decide an artifact is cached: `web/browser/src/routes/artifacts/[id]/+page.svelte:102-104` renders `cached` whenever `version.cached_at` is set. After a sweep, every evicted version still carries a timestamp and still shows the pill, so the dashboard reports a cache that does not hold what it claims. **Trigger:** any sweep that evicts anything, then loading the artifact page for one of those versions. The registry sets `cached_at = now()` on re-registration - `services/registry/internal/store/artifacts.go:219`, `DO UPDATE SET cached_at = now()` - so the drift heals for anything somebody fetches again, and persists forever for everything nobody asks for again. That second group is exactly the population a sweep creates, since it evicts what has been sitting longest. **What a good review says:** have the sweep tell the registry what it removed, so `cached_at` is cleared for those versions - or stop treating that column as ground truth for the pill. **Why it is hard:** the evidence that this was known is in a sentence this diff deletes. The manual procedure in `docs/operations.md` ended with "Set `cached_at` to null for the rows you removed so the dashboard stops claiming they are cached", and the change removes that line while automating the deletion it belonged to. Finding it means noticing the removed step, or knowing the dashboard well enough to ask what renders the pill. Nothing in the Rust hints at it, since the proxy has no database connection by design and the registry client has no call for this. ## Not defects These are intended trade-offs. A reviewer raising one is right about the facts, so do not score it as a finding either way. - **The sweep route sits outside the `TimeoutLayer`.** Noticing that a destructive endpoint has no request deadline is correct; the router comment gives the reason, which is that a sweep of a large store legitimately runs longer than ten seconds and a gateway timeout would report failure for a pass that ran. - **The pass uses `std::fs` rather than `tokio::fs`.** Intended, and the module documentation argues it. The keyed defect is 6, the missing `spawn_blocking` - a review arguing that every call should instead go through `tokio::fs` is arguing against the intended choice. - **`sweep.rs` restates the three directory names instead of reaching them through `BlobStore`.** Intended, with the reason in the comment above the constants; `store.rs` remains the definition of the layout. - **The sweep task is not part of the graceful shutdown.** Intended, with the reason in `spawn_sweeps`: a pass is a sequence of independent unlinks, and stopping one part-way leaves the store as consistent as finishing it would. - **Nothing cross-validates `CAIRN_CACHE_MAX_BYTES` against `CAIRN_MAX_ARTIFACT_BYTES`.** `config.rs` cross-validates no pair of values today - every check is one value against zero or against its parser - so not comparing these two is consistent with the file rather than an oversight here. - **There is no test over the new code.** A reviewer asking for one is right about this repository's convention, which covers pure logic as a matter of course. The pull request body answers for most of the module - the reclamation loop needs a populated store on a real filesystem, which is why the store paths are untested too - but `age_of`, the ceiling comparison and the ordering are pure and testable, so a review that asks for a test on the eviction order is making a fair point the body does not answer. - **Index entries are left behind by `BlobStore::remove`.** Pre-existing and documented there; the sweep's own index pruning is new and is covered by defect 12.