fix(B9/G4): don't double-count an already-resident model on reload - #616
Conversation
makeRoomFor's dirty-ceiling check summed every non-evicted dirty resident AND added the incoming spec.sizeMB — but on a reload the incoming REPLACES its own same-key resident, so that entry is the SAME RAM. Reloading an already-resident dirty model (e.g. an image model re-selected after a thread change) therefore charged 2x its footprint and was refused for memory even though it already occupied that RAM. planEviction already handles this: alreadyResident charges the incoming 0 and never evicts the same-key resident. The separate dirty-ceiling check in makeRoomFor didn't mirror that — now it excludes the incoming's own same-key entry from keptDirtyMB, so the footprint is counted once. The real physics guard is untouched: a fresh dirty model whose OWN footprint exceeds the balanced ceiling is still refused. Test drives the REAL manager (registers an already-resident dirty image model, reloads the same key) sized so 1x fits the balanced dirty ceiling but 2x (the double-count) exceeds it — red before the fix (fits:false), green after; plus a companion proving a genuinely-oversized model is still refused (no over-correction). 66 residency + 126 adjacent memory tests pass.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesReload dirty-memory accounting
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|



Problem
makeRoomFor's dirty-ceiling check summed every non-evicted dirty resident and added the incomingspec.sizeMB— but on a reload the incoming replaces its own same-key resident, so that entry is the same RAM. Reloading an already-resident dirty model (e.g. an image model re-selected after a thread change) therefore charged 2× its footprint and was refused for memory even though it already occupied that RAM.Fix
planEvictionalready handles this (itsalreadyResidentrule charges the incoming 0 and never evicts the same-key resident). The separate dirty-ceiling check inmakeRoomFordidn't mirror it — now it excludes the incoming's own same-key entry fromkeptDirtyMB, so the footprint is counted once. The real physics guard is untouched: a fresh dirty model whose own footprint exceeds the balanced ceiling is still refused.Test — real manager, red→green
Drives the real
modelResidencyManager(registers an already-resident dirty image model, reloads the same key), sized so 1× fits the balanced dirty ceiling but 2× (the double-count) exceeds it:fits:false, double-counted; verified by reverting), green after;66 residency + 126 adjacent memory tests pass; eslint/tsc/depcruise clean.
On-device note (honest)
B9's precise fault — the double-count crossing the memory ceiling on reload — can't be staged on a physical device: it needs an image model that fits budget at 1× but double-counts past the ceiling. SDXL was the only candidate on the test iPhone, and it's override-only (7.3 GB > 4.9 GB budget, so it bypasses the dirty-ceiling check entirely) and has since been removed. The rigorous proof is the real-manager test above; there's no distinguishing device repro to run — same class as the B5/B8 memory-accounting fixes.
Summary by CodeRabbit
Bug Fixes
Tests