feat(minvmd): scheduled guest maintenance — cache sweep + fstrim - #1002
Draft
norrietaylor wants to merge 3 commits into
Draft
feat(minvmd): scheduled guest maintenance — cache sweep + fstrim#1002norrietaylor wants to merge 3 commits into
norrietaylor wants to merge 3 commits into
Conversation
Nothing reclaims guest state today: `cache/built` grows monotonically and every build leaves sandbox/task/temp dirs behind. Deleting them does not shrink the host image either — the state volume is mounted without `discard`, so `data-vol.raw` is a high-water mark of every block ever written. The two halves are ordered and both required: the sweep frees blocks inside ext4, the trim returns them to the host image. A timer in the `minvmd` supervisor drives the in-VM `minimald` through a new `Maintenance` RPC every `maintenance-interval-secs` (default 6h): sweep entries unused for longer than `maintenance-older-than-secs` (default 14d, matching `mip cache clean`), reap dirs whose owning pid is gone, then `syncfs` + `FITRIM`. Both knobs persist in `config.toml` with the same env/config/default precedence as the resource parameters. The schedule lives on the host, matching the `Shutdown` precedent: policy on the host, execution in the guest. A cycle is skipped when a build is in flight (FITRIM takes ext4 block-group locks) or the host is on battery. Correctness: `mip cache clean` is project-scoped, so its protection rule does not transfer. The daemon instead protects the union of packages every session's workspace references, read from the store — not via `get_session`, which spawns a dormant session's actor as a side effect. If that union cannot be computed the cache is left untouched, but the dead-dir reap and the trim still run, since neither can evict anything a session needs; the skip is reported rather than raised so one unresolvable session degrades a cycle instead of disabling maintenance for the life of the VM. Session-tree reclaim is deliberately out of scope: destroyed sessions already free their trees crash-safely via the store's tombstone protocol, and the remaining cost is duplication between live sessions, tracked separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both rested on a signal that does not hold. `sandbox2` names each sandbox directory after the pid of the process that *created* it (`std::process::id()`), not the leader whose lifetime it tracks. In the guest the creator is `minimald` — pid 1 — so every directory is stamped `-1` and `/proc/1` exists by definition, including across restarts. Observed on a booted VM: every cycle declined with `a task is in flight (build-1784239277-0-1)`, so maintenance never ran at all, and the reap could never have removed anything either. Rather than guess with a heuristic, ship the two halves that are sound — the session-aware cache sweep and the `fstrim` — and leave directory reclaim to the namespace-ownership fix tracked in gominimal/inbox#368. The constraint is recorded in the module doc and the CLI reference so the gap is legible rather than silent. Also promotes the remaining skip paths from `debug!` to `info!`. A cycle that declines is exactly what a reader is trying to diagnose, and at DEBUG it was indistinguishable from a timer that never fired — which is how the pid-1 defect stayed invisible through two boots. `MaintenanceResponse` collapses into `Errorable<MaintenanceReport>`: with nothing left to defer, a response variant nothing can emit would be misleading. `stale_dirs_removed` / `stale_dir_bytes_deleted` go with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds configurable daily guest maintenance with session-aware cache sweeping and Linux state-volume trimming, wiring settings from ChangesMaintenance configuration and CLI
Guest maintenance execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant minvmd
participant VM
participant minimald
participant SessionManager
participant Cache
participant StateVolume
minvmd->>VM: pass maintenance environment tokens
VM->>minimald: boot with schedule and retention
minimald->>SessionManager: request maintenance inputs
SessionManager-->>minimald: workspaces and daemon directories
minimald->>Cache: sweep stale unprotected entries
minimald->>StateVolume: syncfs and FITRIM
StateVolume-->>minimald: discarded byte count
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
The schedule was on the host because the issue judged the guest clock untrustworthy: it sets from the RTC at boot and advances only while the VM is scheduled, so a sleeping host left it hours behind. That is no longer true. `minvmd`'s timekeep bridge pushes the host wall clock into the guest every minute and immediately after a suspend, which removes the only argument for scheduling from outside. So `minvmd` now configures rather than drives: a time of day and a retention, handed over on the kernel command line at boot, and the in-VM `minimald` runs its own daily timer from them. `minvmd config set --maintenance-at HH:MM` replaces `--maintenance-interval-secs`; a wall-clock time is what a nightly schedule actually wants, and an interval never expressed "run at 3am". `HH:MM` is fixed-width and whitespace-free because it travels on the kernel command line, which the kernel splits on whitespace — so it is validated at `config set` rather than silently dropped a boot later. UTC, because a microVM rootfs carries no timezone database. The guest waits in short slices against the wall clock rather than one long sleep: a single monotonic deadline would not advance across a host suspend and the schedule would slip by exactly the time spent asleep, which is the failure timekeep exists to prevent. A clock that jumps forward over several missed occurrences runs once and resumes the cadence instead of firing a backlog. Removed with the host timer: the `Maintenance` RPC and its whole wire contract, now that nothing triggers cycles from outside, and the battery probe, which the guest cannot see. A 03:00 schedule already lands when the machine is most likely idle and on mains, which was most of what the probe bought. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
norrietaylor
marked this pull request as ready for review
July 28, 2026 23:07
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
docs/reference/cli-minvmd.md (1)
72-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
Static analysis flags this block for missing a language identifier (MD040).
📝 Proposed fix
-``` +```text minvmd config set [--vcpus <N>] [--ram-mib <N>] [--maintenance-at <HH:MM>] [--maintenance-older-than-secs <N>]</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/reference/cli-minvmd.mdaround lines 72 - 76, Update the fenced code
block containing the minvmd config command to include the text language
identifier, preserving the command content and formatting.</details> <!-- cr-comment:v1:2cb1948f8434c1559d106d2b --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>crates/minvmd/src/vm.rs (2)</summary><blockquote> `538-549`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Redundant `.replace()` call.** Rust's line-continuation (`\` at end of a string literal line) already strips the following line's leading whitespace at compile time, so `.replace(" ", "")` has nothing to remove here. Harmless, but reads as if whitespace still needs stripping. <details> <summary>♻️ Proposed simplification</summary> ```diff assert_eq!( line, "console=hvc0 MINIMAL_MAINTENANCE_AT=03:00 \ - MINIMAL_MAINTENANCE_OLDER_THAN_SECS=1209600" - .replace(" ", "") + MINIMAL_MAINTENANCE_OLDER_THAN_SECS=1209600" );🤖 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/minvmd/src/vm.rs` around lines 538 - 549, Remove the redundant replace call from kernel_cmdline_forwards_the_maintenance_schedule and compare directly against the continued string literal. Preserve the expected kernel command line content and formatting.
236-266: 🩺 Stability & Availability | 🔵 TrivialRun VM/daemon integration coverage for this boot-line change.
This modifies the guest kernel command line construction used at VM boot. As per path instructions, changes to VM or daemon paths should run
just e2eand/orjust test-vm, and should not rely only on unit tests for VM/networking behavior.Based on path instructions: "When changing VM or daemon paths, run the relevant integration coverage:
just e2eand/orjust test-vm." and "Do not rely only on unit tests for VM/networking behavior; preserve and run the applicable integration and root-integration harnesses."Also applies to: 384-427
🤖 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/minvmd/src/vm.rs` around lines 236 - 266, Run the applicable VM/daemon integration coverage after updating the boot-line construction in Vm::apply, using just e2e and/or just test-vm as required by the repository path instructions. Do not rely solely on unit tests; preserve and execute the relevant integration and root-integration harnesses.Source: Path instructions
crates/minimald/src/server.rs (1)
441-446: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider gating the maintenance scheduler by
in_microvm.
spawn_scheduleronly checks forMAINTENANCE_AT_ENV; it isn't gated by thein_microvmflag already available a few lines above.trim()self-gates on a mounted state volume, but the cachesweep()insiderun_cycledoes not — it acts ondaemon_ctx.local_cache()unconditionally. If this env var were ever present in a native (non-guest) minimald process, sessions' real cache would be swept with no guest-only safeguard.🛡️ Proposed guard
- let _maintenance = crate::maintenance::spawn_scheduler(state.clone()); + #[cfg(target_os = "linux")] + let _maintenance = in_microvm.then(|| crate::maintenance::spawn_scheduler(state.clone()));🤖 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 441 - 446, Gate the maintenance scheduler initialization around spawn_scheduler with the existing in_microvm flag, so it is created only for guest processes; keep the current scheduler lifetime binding and unscheduled no-op behavior unchanged.crates/lcache/src/lib.rs (1)
289-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate hash→subpath logic with
dir().
entry_pathre-implements the samehash_hex[0..2]/hash_hex[2..]split already used bydir()(crates/lcache/src/fs.rs-adjacent method at lines 169-183). Consider extracting a shared private helper (e.g.fn hash_subpath(hash: &SpecHash) -> PathBuf) used by both, so the on-disk layout stays defined in exactly one place.♻️ Proposed refactor
+fn hash_subpath(hash: &SpecHash) -> PathBuf { + let hash_hex = hash.0.to_hex(); + [&hash_hex.as_str()[0..2], &hash_hex.as_str()[2..]] + .iter() + .collect() +} + pub fn entry_path(&self, hash: &SpecHash) -> PathBuf { - let hash_hex = hash.0.to_hex(); - // Entries on disk are at <root>/<first byte as hex>/<remaining bytes as hex> - let subpath: PathBuf = [&hash_hex.as_str()[0..2], &hash_hex.as_str()[2..]] - .iter() - .collect(); - - self.inner().fs.path().join(subpath) + self.inner().fs.path().join(hash_subpath(hash)) }🤖 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/lcache/src/lib.rs` around lines 289 - 303, Extract the duplicated hash-to-subpath construction into a shared private helper, such as hash_subpath, and update both entry_path and dir to use it. Preserve the existing <root>/<first two hex characters>/<remaining hex characters> layout while keeping each method’s current behavior unchanged.
🤖 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 119-123: Update the retention-value parsing in the maintenance
configuration flow around MAINTENANCE_OLDER_THAN_ENV to distinguish a missing
variable from an invalid or zero value. Preserve the default only when the
variable is absent; when parsing fails or the value is zero, log a warning using
the same approach as the MAINTENANCE_AT_ENV handling, then apply the default.
In `@crates/minvmd/src/cmd/mod.rs`:
- Around line 251-274: Update effective_maintenance_at and
effective_maintenance_older_than_secs to validate each source before applying
precedence, so invalid environment values fall back to valid persisted values
and then defaults. Trim maintenance-at values before returning them, ensuring
the validated value is the one passed to kernel_cmdline. Add appropriate logging
for rejected environment or persisted maintenance settings so operators can
identify fallback decisions.
---
Nitpick comments:
In `@crates/lcache/src/lib.rs`:
- Around line 289-303: Extract the duplicated hash-to-subpath construction into
a shared private helper, such as hash_subpath, and update both entry_path and
dir to use it. Preserve the existing <root>/<first two hex
characters>/<remaining hex characters> layout while keeping each method’s
current behavior unchanged.
In `@crates/minimald/src/server.rs`:
- Around line 441-446: Gate the maintenance scheduler initialization around
spawn_scheduler with the existing in_microvm flag, so it is created only for
guest processes; keep the current scheduler lifetime binding and unscheduled
no-op behavior unchanged.
In `@crates/minvmd/src/vm.rs`:
- Around line 538-549: Remove the redundant replace call from
kernel_cmdline_forwards_the_maintenance_schedule and compare directly against
the continued string literal. Preserve the expected kernel command line content
and formatting.
- Around line 236-266: Run the applicable VM/daemon integration coverage after
updating the boot-line construction in Vm::apply, using just e2e and/or just
test-vm as required by the repository path instructions. Do not rely solely on
unit tests; preserve and execute the relevant integration and root-integration
harnesses.
In `@docs/reference/cli-minvmd.md`:
- Around line 72-76: Update the fenced code block containing the minvmd config
command to include the text language identifier, preserving the command content
and formatting.
🪄 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: 390c7493-ea9e-45b4-aeae-e4884e6a6852
📒 Files selected for processing (14)
crates/lcache/src/lib.rscrates/mctx/src/lib.rscrates/minimald/src/guest.rscrates/minimald/src/lib.rscrates/minimald/src/maintenance.rscrates/minimald/src/server.rscrates/minimald/src/sessions.rscrates/minvmd/src/cmd/config.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/config.rscrates/minvmd/src/main.rscrates/minvmd/src/vm.rscrates/minvmd/tests/config_cli_integration.rsdocs/reference/cli-minvmd.md
Comment on lines
+119
to
+123
| let older_than_secs = std::env::var(MAINTENANCE_OLDER_THAN_ENV) | ||
| .ok() | ||
| .and_then(|v| v.trim().parse::<u64>().ok()) | ||
| .filter(|&secs| secs > 0) | ||
| .unwrap_or(DEFAULT_OLDER_THAN_SECS); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Silent fallback on an invalid/zero retention value.
An unparseable MINIMAL_MAINTENANCE_OLDER_THAN_SECS (or 0) silently falls back to DEFAULT_OLDER_THAN_SECS, unlike the sibling MAINTENANCE_AT_ENV parse failure a few lines above, which logs a warning. A typo here would go unnoticed instead of surfacing the same way.
🩹 Proposed fix
let older_than_secs = std::env::var(MAINTENANCE_OLDER_THAN_ENV)
.ok()
- .and_then(|v| v.trim().parse::<u64>().ok())
- .filter(|&secs| secs > 0)
- .unwrap_or(DEFAULT_OLDER_THAN_SECS);
+ .map(|v| {
+ v.trim().parse::<u64>().ok().filter(|&secs| secs > 0).unwrap_or_else(|| {
+ tracing::warn!(
+ env = MAINTENANCE_OLDER_THAN_ENV,
+ value = v,
+ "ignoring an invalid maintenance retention; using the default",
+ );
+ DEFAULT_OLDER_THAN_SECS
+ })
+ })
+ .unwrap_or(DEFAULT_OLDER_THAN_SECS);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let older_than_secs = std::env::var(MAINTENANCE_OLDER_THAN_ENV) | |
| .ok() | |
| .and_then(|v| v.trim().parse::<u64>().ok()) | |
| .filter(|&secs| secs > 0) | |
| .unwrap_or(DEFAULT_OLDER_THAN_SECS); | |
| let older_than_secs = std::env::var(MAINTENANCE_OLDER_THAN_ENV) | |
| .ok() | |
| .map(|v| { | |
| v.trim().parse::<u64>().ok().filter(|&secs| secs > 0).unwrap_or_else(|| { | |
| tracing::warn!( | |
| env = MAINTENANCE_OLDER_THAN_ENV, | |
| value = v, | |
| "ignoring an invalid maintenance retention; using the default", | |
| ); | |
| DEFAULT_OLDER_THAN_SECS | |
| }) | |
| }) | |
| .unwrap_or(DEFAULT_OLDER_THAN_SECS); |
🤖 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/maintenance.rs` around lines 119 - 123, Update the
retention-value parsing in the maintenance configuration flow around
MAINTENANCE_OLDER_THAN_ENV to distinguish a missing variable from an invalid or
zero value. Preserve the default only when the variable is absent; when parsing
fails or the value is zero, log a warning using the same approach as the
MAINTENANCE_AT_ENV handling, then apply the default.
Comment on lines
+251
to
+274
| pub fn effective_maintenance_at() -> Option<String> { | ||
| let value = std::env::var(MAINTENANCE_AT_ENV) | ||
| .ok() | ||
| .or_else(|| persisted_resource_config().maintenance_at) | ||
| .unwrap_or_else(|| DEFAULT_MAINTENANCE_AT.to_string()); | ||
| is_valid_maintenance_at(&value).then_some(value) | ||
| } | ||
|
|
||
| /// The effective sweep retention in seconds, resolved as | ||
| /// `env override ?? persisted config ?? default` (R9.7). | ||
| /// | ||
| /// A zero retention would make every cache entry eligible on the next run, so | ||
| /// it is rejected in favour of the default rather than honoured — the schedule | ||
| /// is unattended, and "delete the whole cache every night" is never what a | ||
| /// stray `0` in the environment meant. | ||
| #[must_use] | ||
| pub fn effective_maintenance_older_than_secs() -> u64 { | ||
| std::env::var(MAINTENANCE_OLDER_THAN_ENV) | ||
| .ok() | ||
| .and_then(|v| v.trim().parse::<u64>().ok()) | ||
| .or_else(|| persisted_resource_config().maintenance_older_than_secs) | ||
| .filter(|&secs| secs > 0) | ||
| .unwrap_or(DEFAULT_MAINTENANCE_OLDER_THAN_SECS) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Precedence resolution doesn't fall back per-level on invalid values.
Both effective_maintenance_at and effective_maintenance_older_than_secs pick the first present source (env, then persisted, then default) and validate only the combined result — they don't validate at each precedence level before falling back. Consequences:
effective_maintenance_at: an env override that is empty/malformed short-circuits toNone(maintenance off) even if the persisted config has a perfectly valid schedule, contradicting the documentedenv override ?? persisted config ?? defaultsemantics.effective_maintenance_older_than_secs: an env value of0bypasses persisted config entirely and jumps straight to the hard-coded default, instead of falling back to persisted config first.is_valid_maintenance_attrims for validation, but the returned (untrimmed) value can still contain whitespace, whichkernel_cmdlineinvm.rswill then silently reject, so a value that passed validation may never actually reach the guest.
None of this is logged, so an operator has no way to tell why maintenance silently isn't running.
🐛 Proposed fix: filter for validity at each precedence level
pub fn effective_maintenance_at() -> Option<String> {
- let value = std::env::var(MAINTENANCE_AT_ENV)
- .ok()
- .or_else(|| persisted_resource_config().maintenance_at)
- .unwrap_or_else(|| DEFAULT_MAINTENANCE_AT.to_string());
- is_valid_maintenance_at(&value).then_some(value)
+ std::env::var(MAINTENANCE_AT_ENV)
+ .ok()
+ .map(|v| v.trim().to_string())
+ .filter(|v| is_valid_maintenance_at(v))
+ .or_else(|| {
+ persisted_resource_config()
+ .maintenance_at
+ .map(|v| v.trim().to_string())
+ .filter(|v| is_valid_maintenance_at(v))
+ })
+ .or_else(|| Some(DEFAULT_MAINTENANCE_AT.to_string()))
}
pub fn effective_maintenance_older_than_secs() -> u64 {
std::env::var(MAINTENANCE_OLDER_THAN_ENV)
.ok()
.and_then(|v| v.trim().parse::<u64>().ok())
- .or_else(|| persisted_resource_config().maintenance_older_than_secs)
- .filter(|&secs| secs > 0)
+ .filter(|&secs| secs > 0)
+ .or_else(|| {
+ persisted_resource_config()
+ .maintenance_older_than_secs
+ .filter(|&secs| secs > 0)
+ })
.unwrap_or(DEFAULT_MAINTENANCE_OLDER_THAN_SECS)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn effective_maintenance_at() -> Option<String> { | |
| let value = std::env::var(MAINTENANCE_AT_ENV) | |
| .ok() | |
| .or_else(|| persisted_resource_config().maintenance_at) | |
| .unwrap_or_else(|| DEFAULT_MAINTENANCE_AT.to_string()); | |
| is_valid_maintenance_at(&value).then_some(value) | |
| } | |
| /// The effective sweep retention in seconds, resolved as | |
| /// `env override ?? persisted config ?? default` (R9.7). | |
| /// | |
| /// A zero retention would make every cache entry eligible on the next run, so | |
| /// it is rejected in favour of the default rather than honoured — the schedule | |
| /// is unattended, and "delete the whole cache every night" is never what a | |
| /// stray `0` in the environment meant. | |
| #[must_use] | |
| pub fn effective_maintenance_older_than_secs() -> u64 { | |
| std::env::var(MAINTENANCE_OLDER_THAN_ENV) | |
| .ok() | |
| .and_then(|v| v.trim().parse::<u64>().ok()) | |
| .or_else(|| persisted_resource_config().maintenance_older_than_secs) | |
| .filter(|&secs| secs > 0) | |
| .unwrap_or(DEFAULT_MAINTENANCE_OLDER_THAN_SECS) | |
| } | |
| pub fn effective_maintenance_at() -> Option<String> { | |
| std::env::var(MAINTENANCE_AT_ENV) | |
| .ok() | |
| .map(|v| v.trim().to_string()) | |
| .filter(|v| is_valid_maintenance_at(v)) | |
| .or_else(|| { | |
| persisted_resource_config() | |
| .maintenance_at | |
| .map(|v| v.trim().to_string()) | |
| .filter(|v| is_valid_maintenance_at(v)) | |
| }) | |
| .or_else(|| Some(DEFAULT_MAINTENANCE_AT.to_string())) | |
| } | |
| /// The effective sweep retention in seconds, resolved as | |
| /// `env override ?? persisted config ?? default` (R9.7). | |
| /// | |
| /// A zero retention would make every cache entry eligible on the next run, so | |
| /// it is rejected in favour of the default rather than honoured — the schedule | |
| /// is unattended, and "delete the whole cache every night" is never what a | |
| /// stray `0` in the environment meant. | |
| #[must_use] | |
| pub fn effective_maintenance_older_than_secs() -> u64 { | |
| std::env::var(MAINTENANCE_OLDER_THAN_ENV) | |
| .ok() | |
| .and_then(|v| v.trim().parse::<u64>().ok()) | |
| .filter(|&secs| secs > 0) | |
| .or_else(|| { | |
| persisted_resource_config() | |
| .maintenance_older_than_secs | |
| .filter(|&secs| secs > 0) | |
| }) | |
| .unwrap_or(DEFAULT_MAINTENANCE_OLDER_THAN_SECS) | |
| } |
🤖 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/minvmd/src/cmd/mod.rs` around lines 251 - 274, Update
effective_maintenance_at and effective_maintenance_older_than_secs to validate
each source before applying precedence, so invalid environment values fall back
to valid persisted values and then defaults. Trim maintenance-at values before
returning them, ensuring the validated value is the one passed to
kernel_cmdline. Add appropriate logging for rejected environment or persisted
maintenance settings so operators can identify fallback decisions.
norrietaylor
marked this pull request as draft
July 28, 2026 23:51
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.
Closes part of #990.
Adds a periodic maintenance cycle to
minvmd: a configurable host-side timerdrives the in-VM
minimaldthrough a session-aware cache sweep followed byfstrim, so the guest filesystem and the host's backing raw image stay boundedover a long-lived VM's life.
The two halves are ordered and both required. The sweep frees blocks inside
ext4; the trim returns them to the host image, which is mounted without
discardand is otherwise a high-water mark of every block ever written.What's here
minimald/src/maintenance.rs— daily scheduler, protected-set computation, cache sweeptrim_state_volume—syncfs+FITRIM(minimald/src/guest.rs)MaintenanceInputsmessage (minimald/src/sessions.rs)config.tomlknobs, CLI flags, kernel-command-line delivery (vm.rs), docsDefaults: 03:00 UTC daily, 14 d retention (matching
mip cache clean --older-than). Both persist inconfig.tomlwith the sameenv ?? config ?? defaultprecedence as the resource knobs. No newdependencies.
Design notes worth reviewing
The schedule lives in the guest (changed in the third commit, after review).
minvmdconfigures it — a time of day and a retention, handed over on the kernelcommand line at boot — and the in-VM
minimaldruns its own timer.This is only sound because the guest clock is now trustworthy. It advances only
while the VM is scheduled, so a sleeping host used to leave it hours behind, and
that drift was the issue's stated reason for scheduling from the host.
minvmd'stimekeep bridge has since landed on
mainand pushes the host wall clock inevery minute and immediately after a suspend, which removes the argument.
Two consequences worth checking in review. The guest waits in short slices
against the wall clock rather than one long sleep, because a single monotonic
deadline would not advance across a host suspend and the schedule would slip by
exactly the time spent asleep. And a clock that jumps forward over several missed
occurrences runs once and resumes the daily cadence rather than firing a backlog
of housekeeping nobody missed.
HH:MMis fixed-width and whitespace-free because the kernel splits its commandline on whitespace; it is validated at
config setrather than silently droppeda boot later. UTC, because a microVM rootfs carries no timezone database.
The sweep reads the store, not session handles.
ManagerHandle::get_sessionspawns a dormant session's actor as a side effect, so building the protected set
that way would wake every session on every cycle.
MaintenanceInputswalks thestore instead.
Fail-closed, but degrading rather than aborting. If any session's packages
cannot be enumerated the cache is left entirely alone — a partial protected set
would look complete to the sweep, and the missing half is exactly what would be
deleted. The trim still runs, since discarding already-free blocks cannot evict
anything, and the skip is reported as
cache_sweep_skippedrather than raised.One unresolvable session should cost a cycle its sweep, not disable maintenance
for the life of the VM.
Protection beats ageing, not the other way round. A just-built entry has no
recorded read yet, so ageing alone would class the freshest artifact in the cache
as the stalest thing in it.
is_sweepablechecks membership first, and thatordering is what the unit tests pin.
What is deliberately not here
The second commit removes the sandbox-directory reap and the build-aware
deferral that the issue calls for. Both rested on a signal that does not hold:
sandbox2names each sandbox directory after the pid of the process thatcreated it, not the leader whose lifetime it tracks. In the guest that creator
is
minimald— pid 1 — so every directory is stamped-1and/proc/1existsby definition, including across restarts.
Observed on a booted VM: every cycle declined with
a task is in flight (build-1784239277-0-1), so maintenance never ran at all,and the reap could never have removed anything either.
Rather than substitute a heuristic, this ships the two halves that are sound and
leaves directory reclaim to the namespace-ownership fix tracked in
#1001. The
constraint is recorded in the module doc and the CLI reference so the gap is
legible rather than silent.
Session-tree reclaim is also out of scope: destroyed sessions already free their
trees crash-safely via the store's tombstone protocol, and the remaining cost is
duplication between live sessions, tracked in
gominimal/inbox#367.
Verification
just cion macOS: green (534 tests).cargo clippy -p minvmd --all-targets -- -D warnings: clean.minimaldundercross(aarch64 musl): 186 lib tests pass, including theschedule arithmetic (roll-to-tomorrow, strictly-after, forward-jump, midnight)
and the sweep predicate.
minvmd: 147 lib tests + 7config_cli_integrationtests, covering the newflag, persistence, source reporting, and the malformed-schedule rejection.
to the guest end-to-end. That run is how the pid-1 defect above was found.
Not verified: the
FITRIMioctl has not executed against a real kernel. Everycycle on the VM deferred before reaching it (the defect the second commit fixes),
and re-testing afterwards was not possible without stopping a stack that had a
live attached session. The request code is hand-derived
(
_IOWR('X', 121, struct fstrim_range)→0xC0185879) and should be exercised ona VM before merge — now by setting
MINVMD_MAINTENANCE_ATto a minute or twoahead and watching for
guest maintenance reclaimed state.Also not automated: the issue's host-side proof that
st_blocks * 512fordata-vol.rawmeasurably drops after a sweep. That needs aMINVMD_E2E-gatedharness in the style of
volume_quiesce_integration.rs.Pre-existing, not from this branch
just test-crossis red onmainat its clippy stage —libc::time_tisdeprecated under musl and
-D warningspromotes it. Reproduced identically onorigin/main. Likewise twocrates/minimal/tests/bug.rstests fail undercross(host network/power collectors absent in the container), also identicalon
origin/main.🤖 Generated with Claude Code
Summary by CodeRabbit
Note
Add scheduled guest maintenance with cache sweep and fstrim to
minimald--maintenance-atand--maintenance-older-than-secsCLI flags onminvmd.FITRIMioctl after asyncfs, reporting reclaimed bytes.Macroscope summarized bba82a6.