Repository navigation
Conversation
…owing Nothing ever deleted from the npm download cache at ~/.staged/packages/npm-prefix/cache/_cacache. Every floating bridge install added the new version's tarballs beside all the old ones, and npm left an orphaned body behind for each package document it revalidated, so the cache grew by one to two gigabytes a month. The reconcile epilogue now wipes `_cacache` whole once it holds more than 1 GiB. The cache has no correctness role, so the only cost is one re-download of the bridges' tarballs on the next pass. - Add `managed_acp_tools/cache.rs` with the single definition of the cache path, the 1 GiB cap and `prune_npm_cache_at`. The prune removes `_cacache.trash-*` leftovers from an interrupted run, sums file sizes without following symlinks, and renames an oversized `_cacache` aside before deleting it so npm never sees a half-deleted cache. It is best-effort and logs failures at warn level. - Call it from `finish_reconcile_at` inside the locked scope, after the outcome is recorded, on the blocking pool. It runs on every pass, not only successful ones, so a bridge that keeps failing cannot switch the cleanup off. - Use the cache path helper in the bridge installer and in the managed npm env pairs. - Lock Doctor's npm-backed fixes and updates, and the deprecated-bridge fix, with a new `NpmWriteLock` that takes the tool-install mutex and then the packages flock. It is taken only after the managed Node runtime ensure returns, because that call takes the same flock. A fix can now wait behind another instance's in-flight install. - The `npm view` and registry probes stay unlocked: one that races a wipe fails once and already reports the latest version as unknown. - Describe the cap in the `managed_acp_tools` layout comment and the reconciler header. No fields are added to `state.json`, and `node/`, `tools/`, `_logs` and `npm-prefix/lib` are left alone. Tests cover a cache under, at and over the cap, survival of everything else under the packages root, leftover trash removal, a missing cache, a `_cacache` that is a plain file or a symlink, symlinks inside the cache, pruning on a failed pass, and the epilogue waiting for a second descriptor's flock before it prunes. All use temporary directories. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…nside the packages tree Follow-up to fca2ddb, addressing the review of that commit. Restricted builds never capped the cache. The reconcile loop returns before its first pass when the managed set is empty, so with the `no-managed-acp-tools` feature, with `STAGED_ACP_TOOLS_DIR` set, or on an unsupported target the epilogue's prune never ran, while Doctor's `npm install -g` fixes and updates kept filling the same cache. - `NpmWriteLock` now remembers its packages root and gains `prune_npm_cache`, which applies the 1 GiB cap while the lock is held. - Doctor's npm-backed fixes and updates, and the deprecated-bridge fix, end in a new `finish_npm_fix`. It prunes under the still-held lock, after a failed command as well as a successful one, and returns the fix's own result untouched. The prune is best-effort and returns nothing, so it cannot fail a fix. - The spawn-blocking wrapper around the prune moves into `cache.rs` as `prune_npm_cache_on_blocking_pool`, shared with the reconcile epilogue. The prune could leave the packages tree. Only the final `_cacache` component was checked for a symlink, so a `cache` or `npm-prefix` linked elsewhere sent the leftover sweep and the wipe to the link target, for example a `~/.npm` that the user's own npm writes without any Staged lock. `prune_npm_cache_at` now stops, with a warning, unless both are real directories. A missing component stays a silent no-op, and a symlinked packages root is still capped because the whole tree and its lock file sit behind it. The wait for the npm write lock was silent. `prepare_npm_fix` now logs at info level when it starts waiting and when it has the lock, with the check id and the elapsed time. The first line is written before the managed Node runtime ensure, because that call can queue behind the same flock. Locking behaviour is unchanged and no timeout is added. The reconciler header, the `managed_acp_tools` layout comment and the cache module docs now say that "silent when there is nothing to manage" includes the cache cap, and that Doctor's fixes cover it there. Tests, all in temporary directories: - a symlinked `cache`, a symlinked `npm-prefix` and a plain-file `cache` are left alone, including the linked cache's trash leftover - a symlinked packages root is still capped - a `_cacache.trash-*` symlink is unlinked and its target survives - the write lock prunes an oversized cache and still holds both locks afterwards, and leaves a cache within the cap alone - `finish_npm_fix` prunes at the real cap for a passing and a failing fix and returns each result unchanged, leaves a cache exactly at the cap, and passes a result through when no lock is held. A sparse file stands in for a cache over 1 GiB. `lock_npm_writes_at` becomes `pub(crate)` and is re-exported under `cfg(test)` so the Doctor tests can lock a temporary root. The one new field is the in-memory `packages_root` on the lock guard; nothing is added to `state.json` or any stored model. Not covered by a test: that `run_doctor_fix` and `run_doctor_update` call `finish_npm_fix`. Those commands resolve the real packages root and spawn real commands, so that wiring is checked by reading only. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Summary
Nothing ever deleted from the private npm download cache at
~/.staged/packages/npm-prefix/cache/_cacache. Every floating bridge install added the new version's tarballs beside the old ones, and npm left an orphaned body behind for each revalidated package document, so the cache grew by one to two gigabytes a month.managed_acp_tools/cache.rswith the single definition of the cache path, a 1 GiB cap andprune_npm_cache_at. The prune sweeps_cacache.trash-*leftovers from an interrupted run, sums file sizes without following symlinks, and renames an oversized_cacacheaside before deleting it so npm never sees a half-deleted cache. It is best-effort and logs failures at warn level.NpmWriteLock(tool-install mutex, then the packages flock). Each fix ends infinish_npm_fix, which prunes under the still-held lock whether the command passed or failed, so builds that manage no bridges (and never reconcile) are still capped.npm-prefixandnpm-prefix/cacheare real directories, so a symlinked cache (for example one pointing at~/.npm) is never sized or wiped. A symlinked packages root is still capped.No fields are added to
state.jsonor any stored model; the only new field is the in-memorypackages_rooton the lock guard.node/,tools/,_logsandnpm-prefix/libare left alone.Tests
All in temporary directories: caches under, at and over the cap; survival of everything else under the packages root; leftover trash removal (including a symlinked trash entry); a missing, plain-file or symlinked
_cacache; symlinkedcacheandnpm-prefixleft alone; a symlinked packages root still capped; pruning on a failed pass; the epilogue waiting for another descriptor's flock; the write lock pruning while holding both locks; andfinish_npm_fixpruning for passing and failing fixes while returning each result unchanged.Not covered by a test: that
run_doctor_fixandrun_doctor_updatecallfinish_npm_fix, since those spawn real commands against the real packages root.🤖 Generated with Claude Code