From 4341c8d68d8e9a2910957570548ee934e6382dd9 Mon Sep 17 00:00:00 2001 From: alichherawalla Date: Sat, 4 Jul 2026 13:51:33 +0530 Subject: [PATCH] fix(residency): a failed eviction unload must not over-commit memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../unit/services/modelResidency.test.ts | 39 +++++++++++++++++++ src/services/modelResidency/index.ts | 19 +++++++-- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/__tests__/unit/services/modelResidency.test.ts b/__tests__/unit/services/modelResidency.test.ts index dede7e830..ba317e151 100644 --- a/__tests__/unit/services/modelResidency.test.ts +++ b/__tests__/unit/services/modelResidency.test.ts @@ -398,6 +398,45 @@ describe('ModelResidencyManager', () => { expect(evicted).toEqual([]); expect(unloadImg).not.toHaveBeenCalled(); }); + + it('refuses (fits:false) and KEEPS the resident when a planned eviction UNLOAD fails (no over-commit → OOM)', async () => { + // The plan wants to evict 'image' to fit 'text', but its native unload REJECTS — + // so RAM was not actually freed. Old behaviour swallowed the error, deleted the + // resident, and returned fits:true → the caller loaded 'text' on top of a still- + // resident 'image' → OOM/jetsam. Now: keep it resident and report room NOT made. + modelResidencyManager.setBudgetOverrideMB(1000); + jest.spyOn(hardwareService, 'refreshMemoryInfo').mockResolvedValue(undefined as never); + const failingUnload = jest.fn().mockRejectedValue(new Error('native unload failed')); + modelResidencyManager.register({ key: 'image', type: 'image', sizeMB: 800 }, failingUnload, 1); + + const { evicted, fits } = await modelResidencyManager.makeRoomFor({ key: 'text', type: 'text', sizeMB: 800 }); + + expect(failingUnload).toHaveBeenCalled(); + expect(fits).toBe(false); // FAILS before (was true) + expect(evicted).toEqual([]); // nothing was genuinely freed + expect(modelResidencyManager.isResident('image')).toBe(true); // FAILS before (was deleted) + }); + + it('reports the victims it DID free before an unload failure, then refuses', async () => { + // Two victims; the first unloads fine, the second rejects. The first is genuinely + // gone (reported evicted); the run then refuses rather than over-committing. + // Budget 900 so the plan fits after evicting BOTH (used 1100 → 0, +800 ≤ 900), + // and both unloads are actually attempted (500 would bail as "won't fit empty"). + modelResidencyManager.setBudgetOverrideMB(900); + jest.spyOn(hardwareService, 'refreshMemoryInfo').mockResolvedValue(undefined as never); + const okUnload = jest.fn().mockResolvedValue(undefined); + const failUnload = jest.fn().mockRejectedValue(new Error('boom')); + // Lowest priority (sidecar) evicted first and succeeds; image second and fails. + modelResidencyManager.register({ key: 'stt', type: 'whisper', sizeMB: 300 }, okUnload, 1); + modelResidencyManager.register({ key: 'image', type: 'image', sizeMB: 800 }, failUnload, 2); + + const { evicted, fits } = await modelResidencyManager.makeRoomFor({ key: 'text', type: 'text', sizeMB: 800 }); + + expect(fits).toBe(false); + expect(evicted).toEqual(['stt']); // the one that truly freed + expect(modelResidencyManager.isResident('stt')).toBe(false); + expect(modelResidencyManager.isResident('image')).toBe(true); // failed unload → kept + }); }); describe('canEvict veto (residency ↔ audio seam)', () => { diff --git a/src/services/modelResidency/index.ts b/src/services/modelResidency/index.ts index 84b72d009..c7532168f 100644 --- a/src/services/modelResidency/index.ts +++ b/src/services/modelResidency/index.ts @@ -243,13 +243,26 @@ class ModelResidencyManager { // strand the device with nothing). The caller blocks the load. return { evicted: [], fits: false }; } + 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 }; } async ensureResident(