feat(minimald): automatic cache cleaning - #1156
Conversation
📝 WalkthroughWalkthroughThe change adds shared package-resolution APIs, a cache-cleaning operation, daemon maintenance scheduling, and streaming ChangesCache Cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CleanCacheRPC
participant MaintenanceActor
participant SessionManager
participant CleanCache
Client->>CleanCacheRPC: submit CleanCache request
CleanCacheRPC->>MaintenanceActor: request cleanup
MaintenanceActor->>SessionManager: needed_packages()
SessionManager-->>MaintenanceActor: required package hashes
MaintenanceActor->>CleanCache: run cleanup with keep set
CleanCache-->>MaintenanceActor: progress events and report
MaintenanceActor-->>CleanCacheRPC: relay updates
CleanCacheRPC-->>Client: stream updates and terminal result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
083d17b to
af31d5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
crates/minimald/src/sessions.rs (2)
675-699: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider resolving sessions concurrently.
The loop awaits
session.needed_packages()one session at a time. Each call runs a full nickel evaluation plus a graph build (per the doc comment onSessionHandle::needed_packages). With several active sessions, this serializes their resolution cost on every maintenance cycle and on every on-demandCleanCacheRPC.Resolve sessions concurrently instead, for example with
futures::future::try_join_allorFuturesUnordered, after collecting the live session handles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/sessions.rs` around lines 675 - 699, Update Sessions::needed_packages to collect live active session handles first, then resolve their packages concurrently using try_join_all or FuturesUnordered instead of awaiting each SessionHandle::needed_packages sequentially. Preserve missing-session handling, error context with the session ID, and union all successful package sets into the existing output HashSet.
675-699: 🩺 Stability & Availability | 🔵 TrivialA single failing session blocks cache reclamation for the whole daemon.
session.needed_packages()'s error propagates through?and fails the entire union. Downstream, the maintenance actor'sclean()treats anyneeded_packages()failure as a reason to skip the whole cache clean, for every session, not just the failing one. This is a deliberate safety choice (an incomplete keep-set must not drive deletions), but it means one persistently broken session — aminimal.tomledited into an invalid state after activation, for example — can silently and indefinitely disable cache cleanup for the entire daemon, with unbounded cache growth as the consequence.Consider adding a metric or alert on repeated
needed_packages/clean failures, so an operator notices before disk usage becomes a problem, and consider whether a session that consistently fails resolution should be identifiable in logs by name/id (it already is, viaSessionsError::other(format!("session {}: {e}", info.id))) to speed up remediation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/sessions.rs` around lines 675 - 699, Add observability for repeated failures in Sessions::needed_packages and the maintenance actor’s clean flow, while preserving the fail-safe behavior that skips cache deletion when the keep-set is incomplete. Record a metric or alert for each failed session resolution, including its session ID (and name if available), and ensure repeated failures remain detectable rather than silently disabling cleanup.crates/minimald/src/test_harness.rs (1)
62-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
server::test_configand dropping the actor with the harness.Two small points for this new block:
crates/minimald/src/server.rslines 830-845 addtest_config, documented as the shared way for sibling modules to build aServerStateHandle.TestServer::newstill buildsConfigby hand just above, so the two constructions can drift.- The
CancellationTokencreated on line 66 is dropped immediately and never cancelled, so the actor task outlives everyTestServer. Storing theMaintenanceHandle(or the token) onTestServerand aborting it on drop keeps the test runtime free of leaked tasks.Neither affects correctness of the current tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/test_harness.rs` around lines 62 - 67, Update TestServer::new to reuse server::test_config when constructing the ServerStateHandle instead of duplicating Config setup. Store the returned MaintenanceHandle or its CancellationToken on TestServer, and cancel or abort it during TestServer cleanup/drop so the maintenance actor cannot outlive the harness.crates/minimald/src/server.rs (1)
313-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale
#[allow(dead_code)]and correct the doc comment.
crates/minimald/src/rpc.rsline 674 callss.maintenance()inclean_cache_stream. The accessor is therefore used, and the note "No manual trigger is wired to this yet" is no longer accurate. Keeping the attribute hides a future genuine dead-code warning.♻️ Proposed change
/// The housekeeping actor, for a caller that wants a cache clean run. /// `None` on a state whose server never started it (unit tests). /// /// Going through the actor is the point: it is the one place a clean /// starts, so a requested one queues behind the periodic one instead of /// racing it. - #[allow(dead_code)] // No manual trigger is wired to this yet. pub(crate) async fn maintenance(&self) -> Option<crate::maintenance::MaintenanceHandle> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/server.rs` around lines 313 - 322, Remove the stale #[allow(dead_code)] attribute from the maintenance method and update its documentation to reflect that callers such as clean_cache_stream use this accessor; do not retain the inaccurate note claiming no manual trigger is wired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/maintenance.rs`:
- Around line 157-161: Update Maintenance::abort and the related shutdown
documentation to accurately state that an in-flight clean continues until
completion because spawn_blocking work cannot be aborted. Ensure the comments
around the clean/shutdown ordering and abort guarantee no longer claim that
abort stops active cleaning, while preserving cancellation behavior between
clean operations.
In `@crates/minimald/src/rpc.rs`:
- Around line 729-744: The channel request readers must enforce a shared checked
maximum body size to prevent unbounded buffering. Update read_channel_request
and the inline request-reading logic in ServeOneshot::handle_channel to reject
data once the accumulated bytes exceed that limit, returning the same error used
by serve_clean_cache for oversized extended data; preserve normal parsing for
bodies within the limit.
In `@crates/mip/src/cmd_cache.rs`:
- Around line 46-74: Update the cache-clean command around CleanCache::run to
execute the synchronous filesystem operation via tokio::task::spawn_blocking,
moving both the CleanCache operation and local cache handle into that closure.
Keep event rendering and result propagation intact, and handle
ctx.needed_packages() outside the blocking task because mctx::Error is not Send.
In `@crates/op/src/cache_clean.rs`:
- Around line 128-132: Update the cache cleanup flow around the stale-entry
filter in `cache_clean.rs` so newly finalized entries without an access-log
record are not evicted before their first read. Prefer recording access during
`PendingDir::finalize`; otherwise, use the entry creation time when
`atimes.last_read(hash)` returns no record and only delete entries older than
`cutoff`.
- Around line 155-187: Update sweep_dir to treat a missing directory from
read_dir as empty, while propagating other read errors; handle removal failures
as best effort by continuing to later entries and incrementing removed only
after success. Move the CleanEvent::Swept emission in sweep_dir to occur after
common::remove_dir_all succeeds.
---
Nitpick comments:
In `@crates/minimald/src/server.rs`:
- Around line 313-322: Remove the stale #[allow(dead_code)] attribute from the
maintenance method and update its documentation to reflect that callers such as
clean_cache_stream use this accessor; do not retain the inaccurate note claiming
no manual trigger is wired.
In `@crates/minimald/src/sessions.rs`:
- Around line 675-699: Update Sessions::needed_packages to collect live active
session handles first, then resolve their packages concurrently using
try_join_all or FuturesUnordered instead of awaiting each
SessionHandle::needed_packages sequentially. Preserve missing-session handling,
error context with the session ID, and union all successful package sets into
the existing output HashSet.
- Around line 675-699: Add observability for repeated failures in
Sessions::needed_packages and the maintenance actor’s clean flow, while
preserving the fail-safe behavior that skips cache deletion when the keep-set is
incomplete. Record a metric or alert for each failed session resolution,
including its session ID (and name if available), and ensure repeated failures
remain detectable rather than silently disabling cleanup.
In `@crates/minimald/src/test_harness.rs`:
- Around line 62-67: Update TestServer::new to reuse server::test_config when
constructing the ServerStateHandle instead of duplicating Config setup. Store
the returned MaintenanceHandle or its CancellationToken on TestServer, and
cancel or abort it during TestServer cleanup/drop so the maintenance actor
cannot outlive the harness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 011d265f-06fd-466d-a80b-d81cd825d405
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/mctx/src/lib.rscrates/minimald-rpc/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/lib.rscrates/minimald/src/maintenance.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/test_harness.rscrates/mip/src/cmd_cache.rscrates/op/src/cache_clean.rscrates/op/src/lib.rs
| /// Reads a JSON request body off `c`, draining until the client half-closes. | ||
| /// The streaming handlers' equivalent of what | ||
| /// [`ServeOneshot::handle_channel`] does inline. | ||
| async fn read_channel_request<T: serde::de::DeserializeOwned>( | ||
| c: &mut RuChannel<Msg>, | ||
| ) -> Result<T, ConnectionError> { | ||
| let mut buf = Vec::with_capacity(1024); | ||
| while let Some(msg) = c.wait().await { | ||
| match msg { | ||
| russh::ChannelMsg::Data { data } => buf.extend_from_slice(&data), | ||
| russh::ChannelMsg::Eof | russh::ChannelMsg::Close => break, | ||
| _ => {} | ||
| } | ||
| } | ||
| Ok(serde_json::from_slice(&buf)?) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare with the oneshot handler's inline request read.
rg -nP -C10 'fn handle_channel' crates/minimald/src/Repository: gominimal/minimal
Length of output: 4704
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- oneshot handler ---'
sed -n '40,85p' crates/minimald/src/rpc.rs
printf '%s\n' '--- streaming helper and call sites ---'
sed -n '700,770p' crates/minimald/src/rpc.rs
rg -n -C4 'read_channel_request|serve_clean_cache|serve_' crates/minimald/src/rpc.rs
printf '%s\n' '--- error definitions and channel types ---'
rg -n -C5 'enum ConnectionError|struct ConnectionError|type Msg|type RuChannel|RuChannel<' crates/minimald/src/Repository: gominimal/minimal
Length of output: 42849
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- clean-cache error path ---'
sed -n '636,660p' crates/minimald/src/rpc.rs
printf '%s\n' '--- request-size and timeout controls ---'
rg -n -i -C3 'max.*(request|body|bytes)|request.*(limit|cap|size)|body.*(limit|cap|size)|timeout|REQUEST_TIMEOUT' crates/minimald/src crates/minimald/Cargo.toml
printf '%s\n' '--- all inline channel reads ---'
rg -n -C3 'ChannelMsg::Data|from_slice\\(&buf\\)|Vec::with_capacity\\(1024\\)' crates/minimald/srcRepository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
text = Path("crates/minimald/src/rpc.rs").read_text()
def body(name):
start = text.index(name)
brace = text.index("{", start)
depth = 0
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
return text[brace:i + 1]
raise AssertionError(f"unterminated body: {name}")
oneshot = body("async fn handle_channel")
helper = body("async fn read_channel_request")
clean = body("async fn serve_clean_cache")
for name, code in [("ServeOneshot::handle_channel", oneshot),
("read_channel_request", helper)]:
assert "ChannelMsg::Data { data }" in code
assert "buf.extend_from_slice(&data)" in code
assert not re.search(r"MAX_.*(REQUEST|BODY)|data\.len\(\).*buf\.len|buf\.len\(\).*data\.len", code)
print(f"{name}: unbounded Data accumulation")
assert "read_channel_request(c).await?" in text
assert "c.extended_data_bytes(1, e.to_string()).await" in clean
print("clean-cache request errors are sent on extended data")
PYRepository: gominimal/minimal
Length of output: 316
Bound JSON request bodies in both channel handlers.
read_channel_request and ServeOneshot::handle_channel append all ChannelMsg::Data bytes until EOF. An authenticated client can force unbounded memory growth. Apply one checked maximum to both paths and return an error when exceeded. serve_clean_cache already sends this error on extended data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimald/src/rpc.rs` around lines 729 - 744, The channel request
readers must enforce a shared checked maximum body size to prevent unbounded
buffering. Update read_channel_request and the inline request-reading logic in
ServeOneshot::handle_channel to reject data once the accumulated bytes exceed
that limit, returning the same error used by serve_clean_cache for oversized
extended data; preserve normal parsing for bodies within the limit.
| let stale: Vec<SpecHash> = cache | ||
| .iter_entries() | ||
| .filter(|hash| !self.keep.contains(hash)) | ||
| .filter(|hash| atimes.last_read(hash).is_none_or(|last| last < cutoff)) | ||
| .collect(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the access-log writer and the finalize path in lcache.
fd -t f -e rs . crates/lcache | xargs rg -n -C4 'fn atimes|fn last_read|fn record_read|fn finalize|alog'Repository: gominimal/minimal
Length of output: 10181
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- cache_clean.rs ---'
sed -n '1,220p' crates/op/src/cache_clean.rs
printf '%s\n' '--- lcache lib write/finalize paths ---'
sed -n '1,180p' crates/lcache/src/lib.rs
sed -n '330,365p' crates/lcache/src/lib.rs
printf '%s\n' '--- record_read call sites ---'
rg -n -C4 'record_read|EntryWriter|\.finalize\(' crates --glob '*.rs'
printf '%s\n' '--- cache-clean tests and metadata definitions ---'
rg -n -C5 'cache_clean|EntryMeta|mtime|modified|created|last_read' crates/op crates/lcache --glob '*.rs'Repository: gominimal/minimal
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- lcache access recording and read APIs ---'
sed -n '150,215p' crates/lcache/src/lib.rs
sed -n '300,345p' crates/lcache/src/lib.rs
printf '%s\n' '--- cache-clean tests ---'
sed -n '220,360p' crates/op/src/cache_clean.rs
printf '%s\n' '--- metadata implementation ---'
sed -n '1,220p' crates/lcache/src/entry_meta.rs
printf '%s\n' '--- static behavior check ---'
python3 - <<'PY'
from pathlib import Path
src = Path("crates/lcache/src/lib.rs").read_text()
start = src.index("pub fn finalize(self, meta: EntryMeta)")
end = src.index("\n }\n}", start) + len("\n }")
finalize = src[start:end]
print("finalize_calls_record_access:", "record_access" in finalize or "record_read" in finalize)
print("finalize_writes_metadata:", "meta.write" in finalize)
print("cache_has_access_recording:", "fn record_access" in src)
print("cache_has_mtime_fallback_api:", "modified()" in src or "metadata(" in src)
PYRepository: gominimal/minimal
Length of output: 14798
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- all record_access call sites ---'
rg -n -C3 'record_access|record_read' crates/lcache/src --glob '*.rs'
printf '%s\n' '--- lcache public cache operations ---'
rg -n -C5 'pub fn (get|open|read|dir|entry|invalidate)' crates/lcache/src/lib.rs
printf '%s\n' '--- cache construction and tracker availability ---'
sed -n '220,335p' crates/lcache/src/lib.rsRepository: gominimal/minimal
Length of output: 12761
Protect newly finalized cache entries from premature eviction. PendingDir::finalize does not record a read; record_access runs only from Cache::read_dir. A newly finalized entry with no access-log record can therefore satisfy is_none_or(|last| last < cutoff) and be deleted before its first read. Record an access at finalization, or compare the no-record case with the entry creation time before deleting it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/op/src/cache_clean.rs` around lines 128 - 132, Update the cache
cleanup flow around the stale-entry filter in `cache_clean.rs` so newly
finalized entries without an access-log record are not evicted before their
first read. Prefer recording access during `PendingDir::finalize`; otherwise,
use the entry creation time when `atimes.last_read(hash)` returns no record and
only delete entries older than `cutoff`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/sessions.rs`:
- Around line 675-698: Coordinate needed_packages with session activation and
cache cleanup so the keep-set snapshot remains valid through cache deletion. Add
a shared retention/lifecycle guard around Sessions::needed_packages and the
maintenance::clean/CleanCache path, ensuring activation cannot complete without
registering hashes while cleanup is in progress. Add a race test that finalizes
a session during cleanup and verifies its cache entry is retained, then run the
applicable just test recipe (and just test-cross on macOS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f68b4c12-bf82-479d-bf41-81cd3ada09af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
crates/mctx/src/lib.rscrates/minimal/tests/cli.rscrates/minimald-rpc/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/lib.rscrates/minimald/src/maintenance.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/session.rscrates/minimald/src/sessions.rscrates/minimald/src/test_harness.rscrates/mip/src/cmd_cache.rscrates/op/src/cache_clean.rscrates/op/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- crates/minimald/Cargo.toml
- crates/minimald/src/test_harness.rs
- crates/minimald/src/lib.rs
- crates/minimald/src/session.rs
- crates/minimal/tests/cli.rs
- crates/op/src/lib.rs
- crates/mip/src/cmd_cache.rs
- crates/minimald/src/maintenance.rs
- crates/mctx/src/lib.rs
- crates/minimald/src/rpc.rs
- crates/op/src/cache_clean.rs
- crates/minimald-rpc/src/lib.rs
- crates/minimald/src/server.rs
| /// The union of what every active session needs: the spec hash of each package | ||
| /// reachable from any session's tasks, stack, or `[session]` block. | ||
| /// | ||
| /// Resolving a session brings its actor up if it isn't running, exactly as | ||
| /// [`get_session`](Self::get_session) does. | ||
| pub async fn needed_packages(&self) -> Result<HashSet<SpecHash>, SessionsError> { | ||
| let mut out = HashSet::new(); | ||
| for info in self.list().await? { | ||
| if info.status != sessions::SessionStatus::Active { | ||
| continue; | ||
| } | ||
| // Deleted between the list and the lookup: gone, so needs nothing. | ||
| let Some(session) = self.get_session(SessionKeyPredicate::Id(info.id)).await? else { | ||
| continue; | ||
| }; | ||
| let pkgs = session | ||
| .needed_packages() | ||
| .await | ||
| .map_err(|e| SessionsError::other(format!("session {}: {e}", info.id)))?; | ||
| out.extend(pkgs); | ||
| } | ||
|
|
||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize session activation with cache retention.
needed_packages takes a non-atomic snapshot. A session can become Active after list() returns but before maintenance::clean passes keep to CleanCache. Its hashes are then absent from the keep set, and cleanup can remove an old cache entry that mctx::Env::build later reads when that session attaches.
Coordinate cleanup with session lifecycle transitions. Hold a shared retention guard through keep-set collection and deletion, or atomically register required hashes before a session becomes Active. Add a race test that finalizes a session during cleanup. Run the test with the applicable just recipe; on macOS, run just test-cross.
Also applies to: 1524-1551
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/minimald/src/sessions.rs` around lines 675 - 698, Coordinate
needed_packages with session activation and cache cleanup so the keep-set
snapshot remains valid through cache deletion. Add a shared retention/lifecycle
guard around Sessions::needed_packages and the maintenance::clean/CleanCache
path, ensuring activation cannot complete without registering hashes while
cleanup is in progress. Add a race test that finalizes a session during cleanup
and verifies its cache entry is retained, then run the applicable just test
recipe (and just test-cross on macOS).
Source: Coding guidelines
Summary
Testing
Checklist
BREAKING CHANGE:footer present if this is a breaking changeNote
Add automatic cache cleaning to minimald with an SSH RPC and background maintenance actor
maintenanceactor inminimaldthat periodically sweeps the local cache, removing stale entries and directories; it also handles on-demand clean requests serialized through an mpsc channel.CleanCacheop in theopcrate that computes a keep-set from live sessions, deletes unused cache entries older than a threshold, sweeps stale sandbox/task/temp directories by checking/procfor dead PIDs, and emits structuredCleanEventprogress updates.CLEAN_CACHE_SUBSYSTEMSSH RPC that streams newline-delimitedCleanCacheUpdateJSON (per-removalRemovedlines, then a terminalDoneorFailed) to clients viaminimald-rpc.mip'scmd_cacheto delegate all deletion and sweep logic toop::CleanCache, usingctx.needed_packages()to protect packages required by active sessions.Macroscope summarized ff428ec.
Summary by CodeRabbit