Harden model download integrity (pre-release audit, wave 2)#536
Conversation
Every curated catalog model ships with nil expectedSizeBytes/sha256, so those validators no-op and the only integrity gate was the HTTP 2xx check. A body the server truncated while ending the transfer cleanly (HTTP/2 reset, proxy close) was therefore promoted as a complete model and later failed to load with an opaque llama.cpp error. - Add ModelFileValidator.validateCompleteness, comparing the staged file's size to the server's declared Content-Length (no-op when unknown). This catches truncation without needing catalog metadata. - Clean up the rescued temp file on every pre-staging failure (cancel, non-2xx, staging-move) and the staged file on every validation/promotion failure; both previously leaked into the temp/runtime directory. - Promote atomically with replaceItemAt when a model already exists, so a failure mid-promotion can never destroy the existing good model before the replacement lands (the old delete-then-move could leave nothing installed).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if fileManager.fileExists(atPath: destinationURL.path) { | ||
| try fileManager.removeItem(at: destinationURL) | ||
| _ = try fileManager.replaceItemAt(destinationURL, withItemAt: stagingURL) | ||
| } else { | ||
| try fileManager.moveItem(at: stagingURL, to: destinationURL) | ||
| } |
There was a problem hiding this comment.
promoteStagedFile: fileExists is checked, then moveItem is called in the else branch. If another process or a concurrent download creates the destination between those two calls, moveItem throws "file already exists," the staging file is cleaned up, and the download is surfaced as failed — even though the data is fully valid. FileManager.replaceItemAt handles both the "destination exists" and "destination absent" cases atomically, so the fileExists branch is unnecessary.
| if fileManager.fileExists(atPath: destinationURL.path) { | |
| try fileManager.removeItem(at: destinationURL) | |
| _ = try fileManager.replaceItemAt(destinationURL, withItemAt: stagingURL) | |
| } else { | |
| try fileManager.moveItem(at: stagingURL, to: destinationURL) | |
| } | |
| _ = try fileManager.replaceItemAt(destinationURL, withItemAt: stagingURL) |
| } | ||
| } | ||
|
|
||
| func test_validateCompleteness_noOpsWhenLengthUnknown() throws { |
There was a problem hiding this comment.
validateCompleteness rejects any size mismatch (not just truncation), but the test suite only covers the under-size case. A file that is larger than the declared Content-Length — e.g., a response with appended garbage bytes — also triggers sizeMismatch via validateSize, and having an explicit test documents that guarantee and guards against a future refactor that changes the comparison direction.
| func test_validateCompleteness_noOpsWhenLengthUnknown() throws { | |
| func test_validateCompleteness_throwsWhenFileLargerThanDeclaredLength() throws { | |
| let url = try makeFixture(contents: Data(repeating: 0xAB, count: 120)) | |
| XCTAssertThrowsError(try ModelFileValidator.validateCompleteness(of: url, declaredContentLength: 100)) { error in | |
| guard case ModelFileValidator.ValidationError.sizeMismatch = error else { | |
| XCTFail("Expected sizeMismatch, got \(error)") | |
| return | |
| } | |
| } | |
| } | |
| func test_validateCompleteness_noOpsWhenLengthUnknown() throws { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Wave 2 of the pre-release audit: model-download integrity. Every curated catalog model ships with nil
expectedSizeBytes/sha256, sovalidateSize/validateSHA256no-op and the only integrity gate was the HTTP 2xx check. A body the server truncated while ending the transfer "cleanly" (an HTTP/2 stream reset, a proxy closing the connection) was therefore promoted as a complete model and later failed to load with an opaque llama.cpp error, with no way for the user to tell it was a bad download.ModelFileValidator.validateCompletenesscompares the staged file's byte size to the server's declaredContent-Length(no-op when unknown). Catches truncation without needing catalog metadata.replaceItemAtwhen a model already exists, so a failure mid-promotion can never destroy the existing good model before the replacement lands (the old delete-then-move could leave nothing installed).Validation
The download manager's filesystem/network flow is not unit-tested (the repo pattern); the restructure was verified by trace, and the new validator is unit-tested.
Linked issues
None. Pre-release hardening, wave 2.
Risk / rollout notes
Content-Length; HuggingFace's CDN does, so truncated downloads are now caught before promotion. A legitimately complete download is unaffected.replaceItemAtworks because staging and the install location share the runtime directory (same volume).fileExistscheck), so it blocks redownload; with this PR truncated files no longer get promoted in the first place, but recovering an already-bad install (e.g. exposing delete for the selected model) is a separate UX change.Greptile Summary
This PR hardens the model download pipeline across three fronts: a
validateCompletenesscheck that catches HTTP-cleanly-truncated bodies by comparing on-disk size toContent-Length, deterministic cleanup of both the temp and staging files on every failure path, and an atomicreplaceItemAt-based promotion that prevents a crash from leaving the install location empty.ModelFileValidator.validateCompletenessdelegates to the existingvalidateSize, no-ops whenContent-Lengthis absent or zero, and is backed by three new unit tests.performSingleFileDownloadcovers temp-file leaks on pre-staging failures (user cancel, non-2xx response, move error) and staging-file leaks on validation or promotion failures.promoteStagedFilereplaces the old delete-then-move withreplaceItemAtwhen the destination exists andmoveItemwhen it does not, though the conditional introduces a TOCTOU thatreplaceItemAtalone would eliminate.Confidence Score: 4/5
Safe to merge; the cleanup and completeness-check logic is correct, and the only actionable finding is a narrow race in promoteStagedFile that causes a spurious download failure rather than data loss.
The promotion path has a fileExists-then-moveItem check that can fail if the destination is created between the two calls; the consequence is a failed download the user must retry, not a corrupted install. No data loss path was identified. The missing over-size test in ModelFileValidatorTests is the only other gap.
The promoteStagedFile method in ModelDownloadManager.swift and the validateCompleteness test block in ModelFileValidatorTests.swift would benefit from a second look.
Important Files Changed
Sequence Diagram
sequenceDiagram participant DM as ModelDownloadManager participant Delegate as ModelDownloadSessionDelegate participant FS as FileSystem participant Validator as ModelFileValidator DM->>Delegate: download(from: url) Delegate-->>DM: DownloadResult(temporaryURL, response) alt Task cancelled or non-2xx response DM->>FS: removeItem(temporaryURL) [cleanup] DM-->>DM: throw error end DM->>FS: moveItem(temporaryURL → stagingURL) alt Move fails DM->>FS: removeItem(temporaryURL) [cleanup] DM-->>DM: throw error end DM->>Validator: validateCompleteness(stagingURL, Content-Length) DM->>Validator: validateSize(stagingURL, expectedSizeBytes) DM->>Validator: validateSHA256(stagingURL, sha256) alt Any validation fails DM->>FS: removeItem(stagingURL) [cleanup] DM-->>DM: throw error end alt Destination exists DM->>FS: replaceItemAt(destinationURL, withItemAt: stagingURL) else Destination absent DM->>FS: moveItem(stagingURL → destinationURL) end alt Promotion fails DM->>FS: removeItem(stagingURL) [cleanup] DM-->>DM: throw error end DM-->>DM: "modelState = .downloaded"Reviews (1): Last reviewed commit: "Harden model download integrity (pre-rel..." | Re-trigger Greptile