fix(residency): a failed eviction unload must not over-commit memory (OOM guard)#454
Closed
alichherawalla wants to merge 1 commit into
Closed
fix(residency): a failed eviction unload must not over-commit memory (OOM guard)#454alichherawalla wants to merge 1 commit into
alichherawalla wants to merge 1 commit into
Conversation
makeRoomFor swallowed a rejecting native unload (.catch) and deleted the resident anyway, then returned fits based on the pre-unload PLAN. So a victim whose unload failed (still holding its RAM) was treated as freed → the caller (activeModelService, which honors fits at index.ts:169) loaded the incoming model on top → OOM/jetsam. Now: only delete + count a victim as evicted when its unload actually resolves. On the first unload rejection, keep that resident and return fits:false, so the caller surfaces a clean 'insufficient-memory' error instead of over-committing and crashing. Test (fails-before/passes-after): a rejecting unload keeps the resident and returns fits:false; with two victims, the one that truly unloaded is reported evicted and the failed one stays resident. Follow-up (noted, not in this PR): ensureResident and whisperStore call makeRoomFor without honoring fits — a separate pre-existing gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Self-auditSOLID / abstraction
Tests — no false green
Provit (on-device E2E)
Platform parity
Standards
|
📝 WalkthroughWalkthroughThe eviction loop in ModelResidencyManager.makeRoomFor now treats native unload failures as fatal, halting eviction, retaining the failed resident, and returning fits: false with only successfully unloaded victims. Two regression tests were added covering single and multi-victim unload failure scenarios. ChangesEviction Failure Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ModelResidencyManager
participant ResidentUnload
Caller->>ModelResidencyManager: makeRoomFor(plan)
loop for each planned victim
ModelResidencyManager->>ResidentUnload: unload()
alt unload succeeds
ResidentUnload-->>ModelResidencyManager: success
ModelResidencyManager->>ModelResidencyManager: add to evicted, delete resident
else unload throws
ResidentUnload-->>ModelResidencyManager: error
ModelResidencyManager->>ModelResidencyManager: keep resident, log error
ModelResidencyManager-->>Caller: return { evicted, fits: false }
end
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoPrevent memory over-commit when residency eviction unload fails
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/services/modelResidency/index.ts`:
- Around line 246-265: `makeRoomFor` now preserves the residency invariant by
only deleting after a successful `unload()`, but `handleMemoryWarning` and
`reclaimSttForGeneration` still bypass that behavior by deleting on unload
failure. Centralize the eviction state transition in a shared helper on
`ModelResidency` that attempts `unload()`, deletes only on success, and returns
the same “kept resident/refuse to over-commit” outcome on failure. Then update
`makeRoomFor`, `handleMemoryWarning`, and `reclaimSttForGeneration` to use that
helper so every residency-owned eviction/reclaim path follows the same
unload/delete rule.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3852102e-9a9c-423c-9062-dd2bd9c7c54e
📒 Files selected for processing (2)
__tests__/unit/services/modelResidency.test.tssrc/services/modelResidency/index.ts
Comment on lines
+246
to
+265
| const evicted: string[] = []; | ||
| for (const victim of plan.evict) { | ||
| const reg = this.residents.get(victim.key); | ||
| if (!reg) continue; | ||
| await reg.unload().catch(err => logger.log(`[ModelResidency] unload ${victim.key} failed:`, err)); | ||
| this.residents.delete(victim.key); | ||
| try { | ||
| await reg.unload(); | ||
| this.residents.delete(victim.key); | ||
| evicted.push(victim.key); | ||
| } catch (err) { | ||
| // Native unload FAILED — the model may still be holding its RAM. Deleting it | ||
| // here (the old behaviour) made the budget believe memory was freed when it | ||
| // wasn't, so the caller loaded the incoming model on top → OOM/jetsam. Keep it | ||
| // resident and report room as NOT made; the caller refuses/defers the load | ||
| // instead of over-committing and crashing. (Successfully-evicted victims stay | ||
| // evicted — they are genuinely gone.) | ||
| logger.log(`[ModelResidency] unload ${victim.key} failed — keeping it resident, refusing to over-commit:`, err); | ||
| return { evicted, fits: false }; | ||
| } | ||
| } | ||
| return { evicted: plan.evict.map(e => e.key), fits: plan.fits }; | ||
| return { evicted, fits: plan.fits }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply this unload/delete invariant to every residency-owned eviction path.
makeRoomFor now only deletes after unload() resolves, but handleMemoryWarning and reclaimSttForGeneration still catch unload failures and delete anyway. A failed native unload in those paths can still leave RAM allocated while the budget map undercounts it, so a later load can over-commit.
Consider centralizing this into one helper and using it from all eviction/reclaim paths.
Suggested direction
+ private async unloadAndRelease(key: string, context: string): Promise<boolean> {
+ const reg = this.residents.get(key);
+ if (!reg) return true;
+ try {
+ await reg.unload();
+ this.residents.delete(key);
+ return true;
+ } catch (err) {
+ logger.log(`[ModelResidency] ${context} unload ${key} failed — keeping it resident:`, err);
+ return false;
+ }
+ }Then have makeRoomFor, memory-warning reclaim, and STT reclaim all rely on this single state transition.
As per coding guidelines, “Fix the seam instead of patching around missing abstractions, and migrate paths through the owning service without bypasses.”
🤖 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 `@src/services/modelResidency/index.ts` around lines 246 - 265, `makeRoomFor`
now preserves the residency invariant by only deleting after a successful
`unload()`, but `handleMemoryWarning` and `reclaimSttForGeneration` still bypass
that behavior by deleting on unload failure. Centralize the eviction state
transition in a shared helper on `ModelResidency` that attempts `unload()`,
deletes only on success, and returns the same “kept resident/refuse to
over-commit” outcome on failure. Then update `makeRoomFor`,
`handleMemoryWarning`, and `reclaimSttForGeneration` to use that helper so every
residency-owned eviction/reclaim path follows the same unload/delete rule.
Source: Coding guidelines
Code Review by Qodo
Context used✅ Compliance rules (platform):
25 rules 1. No integration test for makeRoomFor
|
|
Collaborator
Author
|
Closing — bundling the fix into #510 (refactor/parse-once-boundary). A falsifying red-flow test for this bug is committed on that branch (integration-level, mocks only the native boundary; verified red on HEAD, flips green when the fix lands). |
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.
Problem (audit A2)
makeRoomFordidawait reg.unload().catch(log); this.residents.delete(key)and returnedfitsfrom the pre-unload plan. A victim whose native unload rejected (still holding RAM) was deleted from the budget map and counted as freed → the main load path (activeModelService/index.ts:169, which honorsfits) loaded the incoming model on top → OOM/jetsam. Same 'failure treated as success' class as the KB embedding bug.Fix
Only delete + count a victim as evicted when its
unload()actually resolves. On the first rejection, keep that resident and returnfits:false→ the caller surfaces the clean 'insufficient-memory' error instead of over-committing.Tests (fails-before/passes-after)
fits:false(was: deleted +fits:true).evicted; the failed one stays resident.Provit
Pending — exercised by the memory/OOM journeys (KUF: Model routing); runs when the oracle's back. Self-audit below.
Follow-up (not here)
ensureResidentandwhisperStorecallmakeRoomForwithout honoringfits— a separate pre-existing gap.🤖 Generated with Claude Code
Summary by CodeRabbit