From 293d1c6ff99b0b4a40c500c37082e76f6a6a6fc6 Mon Sep 17 00:00:00 2001 From: alichherawalla Date: Sat, 25 Jul 2026 02:54:58 +0530 Subject: [PATCH] fix(B5/G1): a transient FS error must not drop a valid model from the registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadDownloadedModels() prunes any model whose file it can't find and PERSISTS the pruned registry (saveModelsList). Its existence probe was a bare `RNFS.exists`, which rejects on a transient FS hiccup (I/O blip, app-container path not yet mounted). That rejection either aborted the whole load (models list blanks until relaunch) or — via the resolve fallback — could read as "file absent" and drop the row, so a filesystem hiccup made the user's fully-downloaded model disappear. Fix: `probeExists()` distinguishes "provably absent" (RNFS resolved false) from "couldn't verify" (RNFS threw). validateAndResolveModels keeps a model when it exists OR when its absence couldn't be verified; only a PROVABLY-gone file is pruned + persisted. Real orphan cleanup (exists cleanly false) is unchanged. Note: the ORIGINAL G1 mechanism (validateModelFile's RNFS.stat throwing -> valid:false -> RNFS.unlink the file) no longer exists on main — that path was refactored to this exists-based registry prune. This closes the residual transient-FS gap in that current implementation; no file is ever unlinked here. Test drives the real loadDownloadedModels/validateAndResolveModels code and only faults the third-party FS boundary (the sole way to reproduce a transient FS error): a valid model survives a throwing existence check with the registry untouched, while a provably absent model is still pruned and a present one is left alone. --- .../modelManager/storageTransientFsG1.test.ts | 70 +++++++++++++++++++ src/services/modelManager/storage.ts | 45 +++++++++--- 2 files changed, 105 insertions(+), 10 deletions(-) create mode 100644 __tests__/unit/services/modelManager/storageTransientFsG1.test.ts diff --git a/__tests__/unit/services/modelManager/storageTransientFsG1.test.ts b/__tests__/unit/services/modelManager/storageTransientFsG1.test.ts new file mode 100644 index 000000000..5dcbf8552 --- /dev/null +++ b/__tests__/unit/services/modelManager/storageTransientFsG1.test.ts @@ -0,0 +1,70 @@ +/** + * B5 / G1 — a transient filesystem error must NEVER unlink a valid, fully-downloaded model. + * + * loadDownloadedModels() prunes any model whose file it can't find, then PERSISTS the pruned + * registry (saveModelsList). The regression: the existence probe (RNFS.exists) treated a + * transient rejection (I/O blip, container not yet mounted) the same as "file absent", so a + * momentary FS hiccup silently dropped the user's multi-GB model from storage — permanent loss. + * + * These tests drive the REAL seam (the actual loadDownloadedModels + validateAndResolveModels + * code) and only fault the third-party FS boundary (RNFS), which is the only way to reproduce a + * transient FS error deterministically. We assert OUR behaviour: the model survives and the + * registry is not rewritten to exclude it — while a PROVABLY absent file is still pruned. + */ +import RNFS from 'react-native-fs'; +import AsyncStorage from '@react-native-async-storage/async-storage'; +import { loadDownloadedModels } from '../../../../src/services/modelManager/storage'; + +const mockedRNFS = RNFS as jest.Mocked; +const mockedAsyncStorage = AsyncStorage as jest.Mocked; + +const MODELS_DIR = '/data/models'; +const VALID_MODEL = { + id: 'unsloth/gemma-4-E2B-it-GGUF/gemma-4-E2B-it-Q4_K_M.gguf', + fileName: 'gemma-4-E2B-it-Q4_K_M.gguf', + filePath: `${MODELS_DIR}/gemma-4-E2B-it-Q4_K_M.gguf`, + engine: 'llama', +}; + +beforeEach(() => { + jest.clearAllMocks(); + mockedAsyncStorage.getItem.mockResolvedValue(JSON.stringify([VALID_MODEL])); + mockedAsyncStorage.setItem.mockResolvedValue(undefined as any); +}); + +describe('loadDownloadedModels — transient FS error (B5/G1)', () => { + it('KEEPS a valid model when the existence check throws (transient), and does not rewrite the registry', async () => { + // RNFS.exists rejects — a transient I/O error, NOT proof the file is gone. + mockedRNFS.exists.mockRejectedValue(new Error('EIO: i/o error, stat')); + + const models = await loadDownloadedModels(MODELS_DIR); + + // The model is retained — no silent data loss. + expect(models).toHaveLength(1); + expect(models[0].filePath).toBe(VALID_MODEL.filePath); + // And the stored registry is NOT rewritten to a pruned list (nothing was provably removed). + expect(mockedAsyncStorage.setItem).not.toHaveBeenCalled(); + }); + + it('still PRUNES and persists when the file is provably absent (exists cleanly resolves false)', async () => { + // Clean "false" everywhere — the file is genuinely gone; registry cleanup must still happen. + mockedRNFS.exists.mockResolvedValue(false); + + const models = await loadDownloadedModels(MODELS_DIR); + + expect(models).toHaveLength(0); + // The pruned (empty) registry is persisted — real orphan cleanup is preserved. + expect(mockedAsyncStorage.setItem).toHaveBeenCalledTimes(1); + const [, persisted] = mockedAsyncStorage.setItem.mock.calls[0]; + expect(JSON.parse(persisted as string)).toHaveLength(0); + }); + + it('KEEPS a present model untouched (exists resolves true — no rewrite)', async () => { + mockedRNFS.exists.mockResolvedValue(true); + + const models = await loadDownloadedModels(MODELS_DIR); + + expect(models).toHaveLength(1); + expect(mockedAsyncStorage.setItem).not.toHaveBeenCalled(); + }); +}); diff --git a/src/services/modelManager/storage.ts b/src/services/modelManager/storage.ts index 57bd9ead6..892d1e470 100644 --- a/src/services/modelManager/storage.ts +++ b/src/services/modelManager/storage.ts @@ -64,18 +64,36 @@ export async function saveImageModelsList(models: ONNXImageModel[]): Promise { + try { + return { exists: await RNFS.exists(path), verifiable: true }; + } catch (e) { + logger.warn(`[ModelManagerStorage] existence check could not be verified for ${path} — keeping model (transient)`, e); + return { exists: false, verifiable: false }; + } +} + async function tryResolveTextModelPath( model: DownloadedModel, modelsDir: string, -): Promise<{ exists: boolean; updated: boolean }> { +): Promise<{ exists: boolean; updated: boolean; verifiable: boolean }> { const resolved = resolveStoredPath(model.filePath, modelsDir); - if (!resolved || resolved === model.filePath) return { exists: false, updated: false }; - const exists = await RNFS.exists(resolved); + if (!resolved || resolved === model.filePath) return { exists: false, updated: false, verifiable: true }; + const { exists, verifiable } = await probeExists(resolved); if (exists) { model.filePath = resolved; - return { exists: true, updated: true }; + return { exists: true, updated: true, verifiable }; } - return { exists: false, updated: false }; + return { exists: false, updated: false, verifiable }; } async function tryResolveMmProjPath( @@ -103,12 +121,12 @@ async function validateAndResolveModels( let pathsUpdated = false; const existenceChecks = await Promise.all( - models.map(m => RNFS.exists(m.filePath)) + models.map(m => probeExists(m.filePath)) ); const modelsToResolve: Array<{ model: DownloadedModel; idx: number }> = []; for (let i = 0; i < models.length; i++) { - if (!existenceChecks[i]) { + if (!existenceChecks[i].exists) { modelsToResolve.push({ model: models[i], idx: i }); } } @@ -124,7 +142,7 @@ async function validateAndResolveModels( const modelsToCheckMmProj: Array<{ model: DownloadedModel; idx: number }> = []; for (let i = 0; i < models.length; i++) { - const mainExists = existenceChecks[i]; + const mainExists = existenceChecks[i].exists; if (!mainExists) { const idx = modelsToResolve.findIndex(m => m.idx === i); if (idx >= 0 && resolutionResults[idx].exists) { @@ -144,15 +162,22 @@ async function validateAndResolveModels( } for (let i = 0; i < models.length; i++) { - const mainExists = existenceChecks[i]; + const { exists: mainExists, verifiable: mainVerifiable } = existenceChecks[i]; let exists = mainExists; + // The model is confirmably gone only when EVERY probe cleanly resolved "absent". + // If any probe couldn't be verified (RNFS threw), we don't know — and must not drop it. + let verifiable = mainVerifiable; if (!mainExists) { const idx = modelsToResolve.findIndex(m => m.idx === i); if (idx >= 0) { exists = resolutionResults[idx].exists; + verifiable = verifiable && resolutionResults[idx].verifiable; } } - if (exists) { + // Keep the model if it exists OR if we couldn't verify its absence. Pruning (and the + // saveModelsList persist that follows) is reserved for files PROVABLY gone — a transient + // filesystem error must never silently unlink a valid, fully-downloaded model (G1). + if (exists || !verifiable) { validModels.push(models[i]); } }