Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions __tests__/unit/services/modelResidency.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)', () => {
Expand Down
19 changes: 16 additions & 3 deletions src/services/modelResidency/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,19 +237,32 @@
// genuinely low, or is the app footprint bloated?
const availMB = Math.round(hardwareService.getAvailableMemoryGB() * 1024);
const totalMB = Math.round(hardwareService.getTotalMemoryGB() * 1024);
logger.log(`[MEM-SM] makeRoomFor ${spec.key} sizeMB=${spec.sizeMB} dirty=${!!spec.dirtyMemory} budgetMB=${budgetMB} os_procAvailMB=${availMB} totalMB=${totalMB} residents=[${residents.map(r => `${r.key}:${r.sizeMB}${r.pinned ? '(pinned)' : ''}`).join(',')}] fits=${plan.fits} evict=[${plan.evict.map(e => e.key).join(',')}]`);

Check warning on line 240 in src/services/modelResidency/index.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not use nested template literals.

See more on https://sonarcloud.io/project/issues?id=off-grid-ai_off-grid-ai-mobile&issues=AZ9KqklRBli2ezftOhOs&open=AZ9KqklRBli2ezftOhOs&pullRequest=454
if (!plan.fits) {
// Won't fit even after the planned evictions — DON'T evict (otherwise we'd
// 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 };
Comment on lines +246 to +265

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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

}

async ensureResident(
Expand Down