fix(balloon): keep a guest reserve in the idle target instead of the floor (CORE-45) - #518
fix(balloon): keep a guest reserve in the idle target instead of the floor (CORE-45)#518AprilNEA wants to merge 7 commits into
Conversation
…floor (CORE-45) The idle target was `used + 256MB`, which for an idle guest (used ≈ 0) collapsed onto the 384MB floor. Measured 2026-07-29 on a 16 GB VM, the descent then asked for 15.8 GB of the guest's 15.96 GB available and held MemAvailable at literally 0 for ~98 s per cycle — an OOM hazard for any running container, and ~14 cores of reclaim spin to produce it. The module's own invariant says the target is never an unconditional constant; the formula violated it in exactly the idle case it exists for. The target is now the higher of two floors: `used + 1GiB`, so the guest keeps a working reserve, and `total - available/2`, so a single entry reclaims at most half the slack rather than walking the guest to the edge in one descent. A reclaim smaller than 512MB is not worth its guest-side cost and keeps the balloon instead. The 1GiB reserve strictly dominates the old 384MB floor, so the floor is removed rather than left as unreachable code — which also drops a latent `clamp(min, max)` panic for a configured size below 384MB. Policy is inert today (`reclaim_capable` is false on every macOS backend, arcbox#504); this is the precondition for re-enabling shrinking anywhere.
|
Run failed. View the logs →
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecced88fea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThe PR replaces the idle balloon floor with bounded reclaim policy and makes failed restores persistently recoverable.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported memory-domain, stale-shrink, test-oracle, and active restore-retry issues are addressed by the current code and regression coverage.
|
| Filename | Overview |
|---|---|
| app/arcbox-core/src/vm_lifecycle/actor.rs | Restricts configured-memory reporting to running machines so stopped VMs terminate restore bookkeeping cleanly. |
| app/arcbox-core/src/vm_lifecycle/balloon/controller.rs | Unifies shrink state, restores before idle sizing, and retries failed restores across active and idle lifecycle transitions. |
| app/arcbox-core/src/vm_lifecycle/balloon/mod.rs | Replaces the absolute floor policy with reserve- and share-bounded reclaim calculated in the configured-memory domain. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Enter idle] --> B{Shrink already booked?}
B -- Yes --> C[Attempt restore]
C -- Fails --> D[IdleUnshrunk retry loop]
C -- Succeeds --> E[Read fresh guest stats]
B -- No --> E
E --> F{Worthwhile reclaim?}
F -- No --> D
F -- Yes --> G[Apply staged shrink]
G --> H[Watch guest pressure]
H -- Pressure or activity --> I[Attempt restore]
I -- Succeeds --> J[Active, no shrink owed]
I -- Fails --> K[Active with booked shrink]
K --> L[Retry every 30 seconds]
L --> I
I -- VM stopped or gone --> J
Reviews (7): Last reviewed commit: "test(balloon): cover the IdleUnshrunk ex..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…le shrinks Three review findings, all verified against the code first. The guest's MemTotal sits below the configured size (kernel reservations: a 16384 MB VM reports 15.6 GiB, measured today). Deriving the target from MemTotal and applying it against the configured full size reclaimed that ~400MB gap on top of the intended share, eating into the very reserve the policy exists to protect. The policy is now expressed as a reclaim subtracted from full, with available used only as a delta — domain-free — and clamped to the guest's own total so an inconsistent reading cannot inflate it. A restore that exhausts its retries deliberately leaves the shrunk bookkeeping set, and its doc comment claimed the next idle entry would notice. Nothing ever read it, so a later entry deciding Keep stranded the guest at the stale target indefinitely. Entry is now the retrier: it restores before probing whenever a shrink is still applied, which the sizing invariant (balloon-empty stats) demanded regardless. The staged-descent test derives its expectation from the policy helpers, so it now also pins the sequence shape independently — first move is one SHRINK_STEP, every move is a full step but the clamping last, and it ends exactly at the policy target.
…entry A fail-open restore that exhausts its attempts lands in Active with the shrink still applied. Retrying only on the next idle entry is not a retry for an active VM, which may never go idle again — and that is the worst case to strand, since fail-open usually follows guest memory pressure: the guest is working, under pressure, and squeezed. Active now carries a 30s retry whenever a shrink is still applied, and no timer at all otherwise.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new stale-shrink retrier only fires on idle re-entry, so a continuously busy VM never reaches it and stays squeezed for the daemon's lifetime.
Reviewed changes
- Re-expressed
idle_targetas a reclaim subtracted fromfull, so the configured and guest-visible memory domains can no longer be mixed —availableis used only as a delta and is clamped tomin(stats.total, full)so an inconsistent reading cannot inflate the reclaim. - Made
enter_idlethe retrier for a stale shrink: it restores before the stats probe wheneverappliedis set, which also keeps the sizing invariant (entry stats are only meaningful while the balloon is empty). - Pinned the derived descent oracle in
staged_descent_steps_on_settled_frameswith three shape assertions that hold independently of the helpers that produced the sequence. - Added
reclaim_ignores_the_memtotal_to_configured_gapandstale_shrink_is_restored_on_next_entry.
The domain rework is the right shape. Both guarantees in the doc comment now hold for every input, not just the ones the tests pass: the reclaim is capped at available - IDLE_MIN_RESERVE, so the guest always keeps its 1 GiB, and target >= full - available + reserve >= used + reserve keeps the target above usage. I swept adversarial inputs (available > total, available > full, full smaller than the reserve, zero available) looking for a violating case and there isn't one. The previous round's two concerns — MIN_WORTHWHILE_RECLAIM having no distinguishing test, and the reserve being short by full - MemTotal — are both closed by this commit.
ℹ️ Coverage sequencing
Nothing above unit tests exercises the descent. tests/e2e/tests/idle_balloon.rs is VZ-pinned and asserts the balloon never moves, which is correct forever for VZ (guest RAM lives in Apple's XPC process, so it can never become reclaim-capable) but means it can never cover a shrink. With reclaim_capable() still false on both backends, CORE-44's HV re-enable would be this formula's first execution against a real guest. Worth landing an HV-gated e2e alongside that flip rather than after it.
ℹ️ Nitpicks
The headroom/floor vocabulary outlives the constants this PR deleted:
controller.rs:127—PRESSURE_MIN_AVAILABLEis documented as "Safely below the entry headroom (256 MiB)". The margin it sits below is now the 1 GiB reserve, so the claim is more true than before, but it names a constant that no longer exists.controller.rs:384— the shrink log still says"idle balloon shrunk to guest usage + headroom".mod.rs:163— thestats.total == 0guard's comment reasons about"used + headroom"degenerating to "the floor". The guard is also now redundant:availableis clamped tostats.total, so a zero total gives a zero reclaim andentry_decisionreturnsKeepon the worthwhile check anyway. Keeping it as a cheap early-out is fine; the justification needs rewording either way.guest/arcbox-agent/src/memory_pressure.rs:11carries the same stale wording.controller.rs:760— the assertion message "every move is a full step except the clamping last one" describes a case the test never reaches.final_target()is exactlyFULL - 3 * SHRINK_STEP, so the sequence is[14, 12, 10]GiB and every move is a full step. The escape hatch in the condition is unexercised; a non-divisiblefinal_targetin the fixture would make it real.
Claude Opus ｜ 𝕏
fail_open notes activity, so the resulting ExitIdle arrives immediately after a failed restore. Retrying there turns a transient backend failure into a same-instant recovery instead of a wait for the 30s timer, which stays as the backstop for the case where no further edge ever arrives.
There was a problem hiding this comment.
Important
The new 30s retry closes the stranding hole, but it never terminates when the VM is stopped rather than deleted — the guest stays squeezed and the daemon logs 4 lines every 30s for the rest of its life.
Reviewed changes
- Added
STRANDED_RESTORE_RETRY(30s) and atokio::select!timer arm toMode::Active, armed only whenapplied.is_some()andstd::future::pending()otherwise, so an active VM that never goes idle again still retries. - Made
ExitIdleinMode::Activeretry the restore instead of being a no-op, so the activity edgefail_openitself emits recovers a transient failure immediately rather than after up to 30s. - Added
stranded_shrink_is_retried_while_activeandstranded_shrink_is_retried_on_the_activity_edge.
The topology is now complete in the sense the previous thread asked for: with the enter_idle restore from 4abf2ff, the ExitIdle edge, and the timer, there is no mode that can hold applied without a scheduled retry. I checked the two ways this could have been quietly non-functional and it is neither: Harness::settle() is 50 yield_now().awaits with no clock advance, so the activity-edge test genuinely cannot be rescued by the timer and the while-active test genuinely needs its explicit tokio::time::advance — both new tests are falsifiable. And the let stranded = self.applied.is_some() snapshot cannot go stale, since nothing mutates applied between the snapshot and either arm body within one loop iteration. Repeated EnterIdle/ExitIdle cannot starve the timer either, because both of those paths restore on their own.
What the retry lacks is a stopping condition. restore() clears applied on exactly two paths — the machine record being deleted, or set_balloon_target succeeding — and a stopped VM satisfies neither, so the loop runs forever. Details inline.
ℹ️ Nitpicks
controller.rs:252 — the Mode::IdleUnshrunk ExitIdle arm still comments "Nothing was shrunk; nothing to restore". Since 4abf2ff, enter_idle can return IdleUnshrunk with applied still set (that is exactly the failed-restore path), so the premise no longer holds. The arm is still correct, because the new Active timer picks the retry up, but for a different reason than the comment gives.
Claude Opus ｜ 𝕏
Architectural review of the subsystem, addressing the shape that produced this PR's three P1 findings rather than the findings alone. The controller held applied/final_target/step_applied_at as three parallel Options that only ever moved together, and restore() signalled failure by leaving applied set — an implicit protocol that five call sites had to know about and three had to re-check by hand. Both P1s were a call site not knowing. They are now one Option<ShrinkState>, whose Some means exactly "the guest is owed memory", and restore() returns a #[must_use] bool so the compiler asks the question at every call site instead of the reviewer. The ExitIdle arm was also copied across all five modes with subtly different behaviour, and my own stale-shrink fix had already made one copy lie: IdleUnshrunk can now hold a failed restore, but its arm still read "Nothing was shrunk; nothing to restore." All five now route through one exit_idle() that hands back whatever is owed, which fixes that by construction and makes IdleUnshrunk recover on the edge rather than waiting out the Active timer. Also drops two stale docs: PRESSURE_MIN_AVAILABLE cited the long-gone 256 MiB headroom, and the shrink log still described the old usage-plus-headroom formula. No behaviour change beyond the IdleUnshrunk recovery above; all 176 tests pass untouched, which is itself the evidence that none of this leaked past the module boundary.
There was a problem hiding this comment.
ℹ️ Nothing new blocks this — the delta is a clean refactor that makes the "guest is owed memory" invariant hard to misread. The
[!IMPORTANT]finding from the previous round (the stranded-restore retry never terminating for a stopped VM) is untouched by these commits and its thread stays open.
Reviewed changes
- Collapsed
applied/final_target/step_applied_atinto oneShrinkState, soshrink.is_some()is the single reading of "the guest is owed memory" — the old shape let a caller consultappliedalone and miss that a restore had failed. - Made
restore()returnboolunder#[must_use], and rewroteenter_idle's stale-shrink block to branch on it rather than re-probingappliedafterwards. - Extracted
exit_idle()and routed every mode'sExitIdlethrough it — which also givesMode::IdleUnshrunka restore it previously skipped. - Refreshed the stale
headroomvocabulary the last round flagged:PRESSURE_MIN_AVAILABLE's doc now namesIDLE_MIN_RESERVE, the shrink log reads"idle balloon shrinking", andrestore()'s doc plus its terminalerror!no longer claim recovery waits for the next idle cycle.
I traced the consolidation against every reachable state and it is behaviour-preserving apart from the one IdleUnshrunk change noted inline. Watching/Dwelling/Polling are only entered downstream of self.shrink = Some(..) (via open_watch / continue_descent), so exit_idle's new shrink.is_some() guard is equivalent to unconditional there. apply_step's new let Some(state) = self.shrink.as_mut() else { return } is unreachable for the same reason — its only caller comes through next_pending_step(), which requires shrink to be Some. And all four restore() call sites land in a mode that has a scheduled retry, so no false is silently dropped: line 374 checks it explicitly, and the three let _ sites (the Active timer, exit_idle, fail_open) all return into Active, which re-arms the 30s timer.
The old three-Option shape also never produced a partially-populated state, so nothing depended on reading one field without the others — the consolidation loses no expressiveness.
ℹ️ Nitpicks
controller.rs:238 / controller.rs:446 — unifying the ExitIdle paths means every restore now logs reason = "idle exit", including the Mode::Active stranded retry that previously logged "stale shrink on activity". In Active the controller is not exiting idle at all, so the one log line that distinguished the activity-edge retrier from an ordinary idle exit is gone.
Claude Opus ｜ 𝕏
…addressable The 30s retry I added had no terminal condition for a *stopped* VM, which is the ordinary case rather than an edge one: MachineManager::stop keeps the machine record, so full_memory_bytes kept returning a size, while VmManager::set_balloon_target rejects any VM that is not Running. Every attempt failed, forever — three warns and an error every 30s for the daemon's remaining lifetime. That is a worse shape than the silent one-shot failure the retry replaced. full_memory_bytes now answers the question the controller is actually asking — is there a balloon I can address — by gating on the machine running, and the trait documents None as a terminal answer rather than a transient one. restore() already treated None as settled; that is now correct for a stop as well as a removal, and it is the honest invariant: a stopped guest is unsqueezed by the stop itself and a fresh boot starts with an empty balloon, so nothing is owed. Regression asserts both halves: no further set attempts once the VM stops, and none after it comes back either, so a new guest does not inherit a shrink it never had.
Routing every ExitIdle through exit_idle() also made IdleUnshrunk recover on the edge instead of waiting out the Active timer — a behaviour change that no test reached. IdleUnshrunk can hold a booked shrink because enter_idle parks there when the stale restore fails. The new case gets there by that exact route and leaves idle without advancing the clock, so neither 30s timer can be what rescues the guest. It also pins the precondition rather than assuming it: exactly one failed restore and no stats probe during the re-entry is the enter_idle early return, which is the only way into IdleUnshrunk holding a shrink.
There was a problem hiding this comment.
ℹ️ Both remaining findings are closed.
1c7633d4gives the stranded retry a real terminal condition ande73292cccovers the one mode no test reached — the only thing left is that one of the new test's two precondition assertions can't fail.
Reviewed changes
RealBalloonDeps::full_memory_bytesnow.filter(|info| info.state == MachineState::Running), so a stopped VM answersNoneexactly like a deleted one.restore()already bookedNoneas settled, so that single line is the whole terminal condition. The trait doc now statesNoneis terminal and why, which is the right place for it — the fake has to honour the same contract.FakeDeps.fullbecameMutex<Option<u64>>so a test can stop the VM mid-flight, plusstranded_shrink_stops_retrying_once_the_vm_stops.stranded_shrink_is_restored_on_exit_from_idle_unshrunk— theIdleUnshrunkgap from the last round.
I chased the two ways full_memory_bytes could now answer None for a guest that is still alive, and neither is reachable. graceful_stop sets Stopping and rollback_stopping (machine.rs:1156-1164) reverts to Running on Ok(false)/Err, which would clear a booking for a guest that never went down — but run_stop (boot.rs:40-110) always falls back to the force mm.stop(&name) after a failed or timed-out graceful stop, so the VM genuinely stops. The Starting window (machine.rs:599 sets Running after VmManager already flipped) is harmless in the other direction: a fresh boot has an empty balloon, so there is nothing booked to lose. Both new tests are falsifiable — settle() advances no clock, so ..._on_exit_from_idle_unshrunk's final restore cannot be the 30 s timer, and the stop test's advance(STRANDED_RESTORE_RETRY * 4) would fire four retries if the filter were absent.
The .filter(..) itself has no direct test — the new controller test drives the pre-existing restore() None branch through the fake. For a one-line field filter on a thin adapter that seems like the right trade, so I'm noting it rather than asking for it.
Claude Opus ｜ 𝕏
| // Exactly one failed restore (3 attempts) and no stats probe: that is | ||
| // the `enter_idle` early return, i.e. we are parked in IdleUnshrunk. | ||
| assert_eq!(h.deps.set_attempts.load(Ordering::SeqCst) - before_entry, 3); | ||
| assert_eq!(h.deps.stats.lock().unwrap().len(), 0); |
There was a problem hiding this comment.
This assertion cannot fail, so only half of the stated precondition is actually pinned.
The test pushes exactly one stats entry (line 1146) and the first EnterIdle consumes it — FakeDeps::guest_stats pops from the front (controller.rs:631-641) and returns None once empty. So the queue is already empty before the second EnterIdle, and len() == 0 holds whichever branch that entry takes. "Zero stats probes consumed" is what you want to assert, but with an empty queue a probe and no probe are indistinguishable.
The attempt-count assertion on the line above does carry weight, so the test still fails if the enter_idle early return goes away and entry falls through to a probe-then-descend — it just fails on targets() at the end rather than here.
Making it falsifiable is two lines: push a second entry so a probe would be observable as a decrease.
deps.push_stats(Some(idle_stats()));
deps.push_stats(Some(idle_stats()));and then
assert_eq!(h.deps.stats.lock().unwrap().len(), 1);The first EnterIdle still consumes exactly one (the descent parks in Watching without advancing the clock, which assert_eq!(h.targets(), vec![FIRST_STEP]) already pins), so a re-entry that probed would drain it to 0 and trip the assertion.
1 participant
Problem
idle_target = used + 256MBdegenerates for an idle guest:MemAvailable ≈ total⇒used ≈ 0⇒ the target collapses onto the 384MB floor. The module's own invariant says the target is "never an unconditional constant" — the formula violated it in exactly the idle case it exists for.Measured 2026-07-29 (VZ, 16 GB VM, before #504 disabled the balloon): the descent asked the guest for 15.8 GB of its 15.96 GB available, drove
MemAvailableto literally 0 for ~98 s per cycle, then pressure-restored and repeated every ~8.5 min — ~2.8 cores averaged while "idle". During the zero-available window any running container is an OOM candidate.Change
The target is now the higher of two floors:
used + IDLE_MIN_RESERVE(1 GiB) — the guest keeps a working reserve;total - available / IDLE_RECLAIM_DIVISOR(2) — one entry reclaims at most half the slack, so no single descent walks the guest to the edge.A reclaim below
MIN_WORTHWHILE_RECLAIM(512 MB) is not repaid by its guest-side cost, so the balloon is kept instead.IDLE_BALLOON_FLOORis removed: the 1 GiB reserve strictly dominates the 384 MB floor, so it was unreachable code. Dropping it also removes a latentclamp(min, max)panic for any configured size below 384 MB.For the 16 GB idle guest above, the target moves from 384 MB to ~8 GB — the host still gets ~8 GB back, without the cliff.
Notes
BalloonDeps::reclaim_capableisfalseon every macOS backend (fix(core): disable the idle balloon — no macOS backend reclaims ballooned memory #504). This is the precondition for re-enabling shrinking anywhere (CORE-44 flips HV back only after this lands).final_target()callsidle_targetand the expected step sequence is derived fromnext_step, so a future policy change cannot silently desync the test.idle_guest_is_never_walked_to_the_floor,planned_reclaim_always_leaves_the_reserve(sweeps availability 0–16 GiB),entry_keeps_when_the_reclaim_is_not_worth_it,idle_target_never_exceeds_a_tiny_configured_size.Validation
cargo test -p arcbox-core --lib— 172 passed.cargo clippy -p arcbox-core --all-targets— zero warnings.cargo fmt --check— clean. No e2e run: the shrink path this governs cannot execute whilereclaim_capableis false, soidle_balloon(which pins the never-shrinks contract) is unaffected.Closes CORE-45.