Skip to content

fix(models): close mmproj integrity gaps (catch-up move + size floor) #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fl4p merged 1 commit into from
Jul 4, 2026
Merged

fix(models): close mmproj integrity gaps (catch-up move + size floor) #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 89 additions & 0 deletions __tests__/unit/services/parallelMmproj.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
});
75 changes: 42 additions & 33 deletions src/services/modelManager/download.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean | null> {
// Returns true if GGUF magic confirmed, false if confirmed invalid, null if
Expand All @@ -81,17 +86,40 @@ async function hasGgufMagic(path: string): Promise<boolean | null> {
async function mmProjFileValid(path: string, expectedSize?: number): Promise<boolean> {
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<BackgroundDownloadContext, { mmProjCompleted: boolean }>,
moveErr: unknown,
): Promise<void> {
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.)
Expand All @@ -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}`);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down