diff --git a/__tests__/unit/services/parallelMmproj.test.ts b/__tests__/unit/services/parallelMmproj.test.ts index 13067c9e9..66c258c0f 100644 --- a/__tests__/unit/services/parallelMmproj.test.ts +++ b/__tests__/unit/services/parallelMmproj.test.ts @@ -1190,5 +1190,94 @@ describe('Parallel mmproj download', () => { mmProjFileName: 'mmproj.gguf', })); }); + + // Regression: the catch-up guard previously only checked existence, so a + // corrupt-but-present sidecar was silently kept as a vision projector. It + // now runs the same read-only validation as the onComplete handler. + it('catch-up: downgrades WITHOUT deleting when the already-present target is corrupt', async () => { + stubStartDownload(['42', '43']); + const completeCbs = captureCompleteCallbacks(); + mockService.getActiveDownloads.mockResolvedValue([ + { downloadId: '43', status: 'completed' } as any, + ]); + mockService.moveCompletedDownload.mockImplementation((id: string) => + id === '43' + ? Promise.reject(new Error('catch-up move failed: target exists')) + : Promise.resolve(`${MODELS_DIR}/vision.gguf`), + ); + const onComplete = jest.fn(); + + await performBackgroundDownload({ + modelId: 'test/model', + file: visionFile(), + modelsDir: MODELS_DIR, + backgroundDownloadContext: bgContext, + backgroundDownloadMetadataCallback: metadataCallback, + }); + + // A corrupt file (bad GGUF magic, large enough to clear the size floor) already + // sits at the target when the catch-up move fails. + mockedRNFS.exists.mockResolvedValue(true); + mockedRNFS.stat.mockResolvedValue({ size: 500_000_000 } as any); + (mockedRNFS.read as jest.Mock).mockResolvedValue('XXXX'); + + watchBackgroundDownload({ + downloadId: '42', + modelsDir: MODELS_DIR, + backgroundDownloadContext: bgContext, + backgroundDownloadMetadataCallback: metadataCallback, + onComplete, + }); + + await new Promise(resolve => setImmediate(resolve)); + await completeCbs['42']?.({ downloadId: '42', fileName: 'vision.gguf' }); + await new Promise(resolve => setImmediate(resolve)); + + expect(onComplete.mock.calls[0][0].mmProjPath).toBeUndefined(); + expect(mockedRNFS.unlink).not.toHaveBeenCalledWith(`${MODELS_DIR}/vision-mmproj.gguf`); + }); + + // Regression: with no expected size (0/undefined, as on crash-restore) the size + // check used to be skipped entirely, so a truncated stub with an inconclusive + // magic read passed as valid. The absolute floor now rejects it. + it('catch-up: rejects a sub-1KB stub even when expectedSize is unknown', async () => { + stubStartDownload(['42', '43']); + const completeCbs = captureCompleteCallbacks(); + mockService.getActiveDownloads.mockResolvedValue([ + { downloadId: '43', status: 'completed' } as any, + ]); + mockService.moveCompletedDownload.mockImplementation((id: string) => + id === '43' + ? Promise.reject(new Error('catch-up move failed: target exists')) + : Promise.resolve(`${MODELS_DIR}/vision.gguf`), + ); + const onComplete = jest.fn(); + + await performBackgroundDownload({ + modelId: 'test/model', + file: visionFile(4_000_000_000, 0), // mmproj expected size unknown + modelsDir: MODELS_DIR, + backgroundDownloadContext: bgContext, + backgroundDownloadMetadataCallback: metadataCallback, + }); + + mockedRNFS.exists.mockResolvedValue(true); + mockedRNFS.stat.mockResolvedValue({ size: 100 } as any); // truncated stub + (mockedRNFS.read as jest.Mock).mockResolvedValue(''); // inconclusive read + + watchBackgroundDownload({ + downloadId: '42', + modelsDir: MODELS_DIR, + backgroundDownloadContext: bgContext, + backgroundDownloadMetadataCallback: metadataCallback, + onComplete, + }); + + await new Promise(resolve => setImmediate(resolve)); + await completeCbs['42']?.({ downloadId: '42', fileName: 'vision.gguf' }); + await new Promise(resolve => setImmediate(resolve)); + + expect(onComplete.mock.calls[0][0].mmProjPath).toBeUndefined(); + }); }); }); diff --git a/src/services/modelManager/download.ts b/src/services/modelManager/download.ts index c900746a1..b23e05d25 100644 --- a/src/services/modelManager/download.ts +++ b/src/services/modelManager/download.ts @@ -60,6 +60,11 @@ export async function performBackgroundDownload(opts: PerformBackgroundDownloadO } const GGUF_MAGIC = 'GGUF'; +// Absolute floor, independent of any externally-supplied expectedSize (which can +// be 0/undefined for downloads reconstructed on crash-restore). Mirrors the +// MIN_GGUF_FILE_SIZE floor in llmSafetyChecks so a 0-byte/truncated stub can +// never pass as a valid projector even when a short magic read is inconclusive. +const MIN_MMPROJ_FILE_SIZE = 1024; // 1 KB async function hasGgufMagic(path: string): Promise { // Returns true if GGUF magic confirmed, false if confirmed invalid, null if @@ -81,17 +86,40 @@ async function hasGgufMagic(path: string): Promise { async function mmProjFileValid(path: string, expectedSize?: number): Promise { try { if (!(await RNFS.exists(path))) return false; - if (expectedSize) { - const stat = await RNFS.stat(path); - const actualSize = typeof stat.size === 'string' ? Number.parseInt(stat.size, 10) : stat.size; - if (actualSize < expectedSize) return false; - } + const stat = await RNFS.stat(path); + const actualSize = typeof stat.size === 'string' ? Number.parseInt(stat.size, 10) : stat.size; + // Absolute floor first, so a 0-byte/truncated stub is rejected even when + // expectedSize is unknown (0/undefined) and the magic read is inconclusive. + if (actualSize < MIN_MMPROJ_FILE_SIZE) return false; + if (expectedSize && actualSize < expectedSize) return false; return (await hasGgufMagic(path)) !== false; } catch { return false; } } +// Shared recovery for a failed mmproj move (both the onComplete handler and the +// catch-up guard hit this): a file may already sit at the target (a re-download, +// or another model sharing the path). Validate it READ-ONLY — reuse if valid, +// downgrade to text-only if not — and never delete, so a misread or a shared +// file can't be destroyed here (llama.rn re-validates on load). Kept in one place +// so the two call sites can't drift (the catch-up guard previously only checked +// existence, silently keeping a corrupt sidecar). +async function reconcileMmProjMoveFailure( + ctx: Extract, + moveErr: unknown, +): Promise { + const valid = ctx.mmProjLocalPath + ? await mmProjFileValid(ctx.mmProjLocalPath, ctx.file.mmProjFile?.size) + : false; + if (valid) { + logger.log('[ModelManager] mmproj move failed but existing target is valid, reusing:', moveErr); + } else { + logger.warn('[ModelManager] mmproj move failed and target invalid, continuing without vision:', moveErr); + ctx.mmProjLocalPath = null; + } +} + // Pre-download gate: a present-but-invalid file is deleted so it re-downloads clean. // (This path owns the file exclusively — unlike the move-failure recovery path, // which uses the read-only mmProjFileValid and never deletes.) @@ -100,14 +128,13 @@ async function checkMmProjExists(path: string | null, expectedSize?: number): Pr const exists = await RNFS.exists(path); if (!exists) return false; try { - if (expectedSize) { - const stat = await RNFS.stat(path); - const actualSize = typeof stat.size === 'string' ? Number.parseInt(stat.size, 10) : stat.size; - if (actualSize < expectedSize) { - logger.warn(`[ModelManager] mmproj partial (${actualSize}/${expectedSize}), re-downloading`); - await RNFS.unlink(path).catch(() => {}); - return false; - } + const stat = await RNFS.stat(path); + const actualSize = typeof stat.size === 'string' ? Number.parseInt(stat.size, 10) : stat.size; + // Absolute floor first (expectedSize may be 0/undefined on crash-restore). + if (actualSize < MIN_MMPROJ_FILE_SIZE || (expectedSize && actualSize < expectedSize)) { + logger.warn(`[ModelManager] mmproj too small (${actualSize}/${expectedSize || MIN_MMPROJ_FILE_SIZE}), re-downloading`); + await RNFS.unlink(path).catch(() => {}); + return false; } if ((await hasGgufMagic(path)) === false) { logger.warn(`[ModelManager] mmproj failed GGUF magic check, re-downloading: ${path}`); @@ -577,21 +604,7 @@ export function watchBackgroundDownload(opts: WatchDownloadOpts): void { try { await backgroundDownloadService.moveCompletedDownload(event.downloadId, ctx.mmProjLocalPath!); } catch (moveErr) { - // Move can fail legitimately when a valid mmproj already sits at this path - // (a re-download, or another model sharing the path). Validate the existing - // file READ-ONLY: reuse it if valid, downgrade to text-only if not — but - // never delete it, so a misread or a file another model references can't be - // destroyed here (llama.rn re-validates on load). - const expectedSize = ctx.file.mmProjFile?.size; - const valid = ctx.mmProjLocalPath - ? await mmProjFileValid(ctx.mmProjLocalPath, expectedSize) - : false; - if (valid) { - logger.log('[ModelManager] mmproj move failed but existing target is valid, reusing:', moveErr); - } else { - logger.warn('[ModelManager] mmproj move failed and target invalid, continuing without vision:', moveErr); - ctx.mmProjLocalPath = null; - } + await reconcileMmProjMoveFailure(ctx, moveErr); } ctx.mmProjCompleted = true; await tryFinalize(); @@ -651,11 +664,7 @@ export function watchBackgroundDownload(opts: WatchDownloadOpts): void { try { await backgroundDownloadService.moveCompletedDownload(ctx.mmProjDownloadId!, ctx.mmProjLocalPath!); } catch (moveErr) { - const targetExists = ctx.mmProjLocalPath ? await RNFS.exists(ctx.mmProjLocalPath) : false; - if (!targetExists) { - logger.warn('[ModelManager] mmproj catch-up move failed, continuing without vision:', moveErr); - ctx.mmProjLocalPath = null; - } + await reconcileMmProjMoveFailure(ctx, moveErr); } ctx.mmProjCompleted = true; await tryFinalize();