fix(models): validate mmproj GGUF integrity on download and move-failure#18
Merged
Conversation
Guard multimodal projector (mmproj) sidecars against corrupt/partial files: - Add GGUF magic-byte validation (hasGgufMagic) alongside the size check. A read that throws or returns a short string is inconclusive (iOS RNFS.read has a known NSInteger bridging bug), so a misread never destroys a valid file. - Pre-download gate (checkMmProjExists) deletes a present-but-invalid sidecar so it re-downloads clean. - mmproj move-failure recovery validates the existing target READ-ONLY (mmProjFileValid): reuse if valid, downgrade to text-only if not, but never delete it - the target may be shared with another model or be valid-but-misread, and llama.rn re-validates on load. Ports the reliability logic from upstream off-grid-ai#316 (dropping its stale Kotlin and website churn); the shared-mmproj delete guard it also proposed is already present in modelManager. Adds move-failure coverage for valid / bad-magic / short-read / size-mismatch / missing-target paths (parallelMmproj: 42 tests).
fl4p
added a commit
that referenced
this pull request
Jul 4, 2026
#23) Two defects in the mmproj GGUF integrity validation (#18), found in post-merge review: 1. The catch-up guard (mmproj completes natively before the onComplete listener registers) only checked target existence on move-failure, silently keeping a corrupt-but-present sidecar as a vision projector. It now runs the same read-only validation as the onComplete handler. Both paths share a new reconcileMmProjMoveFailure() so they can't drift again. 2. mmProjFileValid / checkMmProjExists skipped the size check when expectedSize was 0/undefined (reachable on crash-restore), so a 0-byte/truncated stub with an inconclusive magic read passed as valid. Added an absolute MIN_MMPROJ_FILE_SIZE (1 KB) floor, independent of expectedSize, mirroring llmSafetyChecks. Tests: catch-up now downgrades-without-deleting on a corrupt present target, and rejects a sub-1KB stub when expectedSize is unknown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Multimodal vision models ship a separate mmproj (multimodal projector) sidecar next to the main GGUF. A corrupt, truncated, or wrong sidecar makes the model load as broken-vision. This hardens the sidecar lifecycle so a bad file is caught and a good one is never destroyed.
Changes (
src/services/modelManager/download.ts)hasGgufMagic) added alongside the existing size check. A read that throws or returns a short/empty string is treated as inconclusive (iOSRNFS.readhas a known NSInteger bridging bug that can manifest either way), so a misread never deletes a valid file.checkMmProjExists): a present-but-invalid sidecar (missing / partial / bad magic) is deleted so it re-downloads clean. This path exclusively owns the file.mmProjFileValid) — reuse it if valid, downgrade to text-only if not, but never delete it.llama.rnre-validates on load.Scope note
This ports only the useful reliability logic from upstream off-grid-ai#316. Dropped from that PR: its stale
DownloadManagerModule.ktrewrite (would regress main's newer download-id/observer refactor) and unrelated website churn. Its shared-mmproj delete guard is already present inmodelManager(index.ts), so it's not re-added here.Tests
__tests__/unit/services/parallelMmproj.test.ts— added move-failure recovery coverage: valid GGUF (reuse), bad magic (downgrade, no delete), size mismatch + good magic (downgrade, no delete), read throws / short read (inconclusive → keep vision), missing target (downgrade).npm testfor the download suites: 202 passing; eslint clean.