From 2a2e5c3592e5a0616674572c2710a492be8d9cb3 Mon Sep 17 00:00:00 2001 From: jam-cai Date: Thu, 23 Apr 2026 14:56:35 -0400 Subject: [PATCH 1/3] feat: Add cancel control to in-flight model downloads (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires Swift Task cancellation through to URLSession so users can stop a long model download from the UI. Until now, Task.cancel() only flipped Task.isCancelled — the URLSession download kept running until natural completion, wasting bytes and silently ignoring the user's intent. Changes: - ModelDownloadManager.cancel(filename:) — public, idempotent. - ModelDownloadSessionDelegate.download(from:) wraps the continuation in withTaskCancellationHandler. The onCancel block calls .cancel() on the retained URLSessionDownloadTask, which fires didCompleteWithError with URLError.cancelled and resumes the continuation. - New pure helper DownloadOutcomeClassifier distinguishes user-cancel (CancellationError or URLError.cancelled) from real failures, so performDownload's catch routes the two correctly: - cancel -> restore .idle (or .downloaded if a prior copy is on disk) - failure -> .failed(message), shown to the user - DownloadableModelRow renders an xmark.circle.fill cancel button next to the percentage during .downloading; tooltip explains. Tests (7 new, 32 total): - DownloadOutcomeClassifierTests covers cancel surfaces (CancellationError, URLError.cancelled), real failure surfaces (timedOut, notConnected, badServerResponse, generic NSError), and the LlamaRuntimeError case (must NOT be misclassified as cancel — would silently roll back a real failure to .idle). Verified locally: xcodebuild test -project tabby.xcodeproj -scheme tabby \ -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO # ** TEST SUCCEEDED ** 32 tests 0 failures First of three stacked PRs against #6: 1. (this PR) cancel + cleanup 2. size + SHA-256 validation 3. retry UX polish Refs #6 --- tabby.xcodeproj/project.pbxproj | 13 ++-- .../Utilities/DownloadOutcomeClassifier.swift | 32 +++++++++ .../Utilities/ModelDownloadManager.swift | 65 ++++++++++++++++--- tabby/UI/DownloadableModelCatalogView.swift | 37 ++++++++--- .../DownloadOutcomeClassifierTests.swift | 50 ++++++++++++++ 5 files changed, 168 insertions(+), 29 deletions(-) create mode 100644 tabby/Services/Utilities/DownloadOutcomeClassifier.swift create mode 100644 tabbyTests/DownloadOutcomeClassifierTests.swift diff --git a/tabby.xcodeproj/project.pbxproj b/tabby.xcodeproj/project.pbxproj index 7470fb39..f31fcb7c 100644 --- a/tabby.xcodeproj/project.pbxproj +++ b/tabby.xcodeproj/project.pbxproj @@ -7,6 +7,7 @@ objects = { /* Begin PBXBuildFile section */ + 8B6282F0C1CCA0746D96B914 /* DownloadOutcomeClassifierTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 562F89255AF340C15A0554BE /* DownloadOutcomeClassifierTests.swift */; }; A1C3E0012F90000100AAA001 /* LlamaSwift in Frameworks */ = {isa = PBXBuildFile; productRef = A1C3E0002F90000100AAA001 /* LlamaSwift */; }; A1C3E0112F90000100AAA001 /* Sparkle in Frameworks */ = {isa = PBXBuildFile; productRef = A1C3E0102F90000100AAA001 /* Sparkle */; }; A404828463CADB2ECDAE7AF3 /* LlamaPromptRendererTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F29623C5C0A67B992D383A3C /* LlamaPromptRendererTests.swift */; }; @@ -27,6 +28,7 @@ /* Begin PBXFileReference section */ 193741492F81DE7000BEC04F /* tabby.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = tabby.app; sourceTree = BUILT_PRODUCTS_DIR; }; 3FBFA92FA44AA317135426FB /* tabbyTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = tabbyTests.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; + 562F89255AF340C15A0554BE /* DownloadOutcomeClassifierTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = DownloadOutcomeClassifierTests.swift; sourceTree = ""; }; 5A39EC1A44E9160E13E2AE77 /* SuggestionAvailabilityEvaluatorTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SuggestionAvailabilityEvaluatorTests.swift; sourceTree = ""; }; 8B1121FFDAD30C7F62E15289 /* SuggestionRequestFactoryTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SuggestionRequestFactoryTests.swift; sourceTree = ""; }; F29623C5C0A67B992D383A3C /* LlamaPromptRendererTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = LlamaPromptRendererTests.swift; sourceTree = ""; }; @@ -67,7 +69,6 @@ children = ( 1937414B2F81DE7000BEC04F /* tabby */, 1937414A2F81DE7000BEC04F /* Products */, - 98425D3A1DB21E10B0F3BFA6 /* Frameworks */, 38079AF58386B5C2E9373742 /* tabbyTests */, ); sourceTree = ""; @@ -87,18 +88,11 @@ F29623C5C0A67B992D383A3C /* LlamaPromptRendererTests.swift */, 5A39EC1A44E9160E13E2AE77 /* SuggestionAvailabilityEvaluatorTests.swift */, 8B1121FFDAD30C7F62E15289 /* SuggestionRequestFactoryTests.swift */, + 562F89255AF340C15A0554BE /* DownloadOutcomeClassifierTests.swift */, ); - name = tabbyTests; path = tabbyTests; sourceTree = ""; }; - 98425D3A1DB21E10B0F3BFA6 /* Frameworks */ = { - isa = PBXGroup; - children = ( - ); - name = Frameworks; - sourceTree = ""; - }; /* End PBXGroup section */ /* Begin PBXNativeTarget section */ @@ -215,6 +209,7 @@ A404828463CADB2ECDAE7AF3 /* LlamaPromptRendererTests.swift in Sources */, B053492719F6106E48290C32 /* SuggestionAvailabilityEvaluatorTests.swift in Sources */, ADEFEE12C197DB6C990E3812 /* SuggestionRequestFactoryTests.swift in Sources */, + 8B6282F0C1CCA0746D96B914 /* DownloadOutcomeClassifierTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/tabby/Services/Utilities/DownloadOutcomeClassifier.swift b/tabby/Services/Utilities/DownloadOutcomeClassifier.swift new file mode 100644 index 00000000..ba67d303 --- /dev/null +++ b/tabby/Services/Utilities/DownloadOutcomeClassifier.swift @@ -0,0 +1,32 @@ +import Foundation + +/// File overview: +/// Classifies download errors into "user pressed cancel" vs "something genuinely +/// went wrong." Used by `ModelDownloadManager` to decide whether a failed +/// download should restore the prior state (cancel) or surface as `.failed`. +/// +/// Why this is its own type: +/// Swift's `Task.cancel()` triggers two distinct error shapes by the time we +/// catch them downstream: +/// +/// 1. `CancellationError` — when `Task.checkCancellation()` runs *before* the +/// URLSession download even starts. +/// 2. `URLError(.cancelled)` — when the URLSession download is in flight and +/// our `withTaskCancellationHandler` aborts it via +/// `URLSessionDownloadTask.cancel()`. +/// +/// Without this classification, case (2) would route through the catch-all +/// failure path and the user would see "The operation couldn't be completed" +/// despite having pressed Cancel themselves. This helper makes the +/// distinction testable in isolation, with no URLSession or Task setup. +enum DownloadOutcomeClassifier { + static func isUserCancellation(_ error: Error) -> Bool { + if error is CancellationError { + return true + } + if let urlError = error as? URLError, urlError.code == .cancelled { + return true + } + return false + } +} diff --git a/tabby/Services/Utilities/ModelDownloadManager.swift b/tabby/Services/Utilities/ModelDownloadManager.swift index 0a4387fb..f9faf525 100644 --- a/tabby/Services/Utilities/ModelDownloadManager.swift +++ b/tabby/Services/Utilities/ModelDownloadManager.swift @@ -126,6 +126,27 @@ final class ModelDownloadManager: ObservableObject { downloadTasks[model.filename] = task } + /// User-initiated cancel of an in-flight model download. Idempotent — + /// calling it on a filename that isn't downloading is a safe no-op. + /// + /// Cancellation flow: + /// 1. `Task.cancel()` flips `Task.isCancelled` and triggers the + /// `withTaskCancellationHandler` block in the delegate. + /// 2. That block calls `URLSessionDownloadTask.cancel()`, which aborts + /// the in-flight download. + /// 3. The delegate receives `didCompleteWithError(URLError.cancelled)` + /// and resumes the continuation throwing. + /// 4. `performDownload`'s catch routes the error through + /// `DownloadOutcomeClassifier`, sees a user cancel, and restores + /// `.idle` (or `.downloaded` if a prior copy is on disk) — never + /// `.failed`, since the user pressed Cancel deliberately. + func cancel(filename: String) { + guard let task = downloadTasks[filename] else { + return + } + task.cancel() + } + func openModelsDirectory() { do { try ensureRuntimeDirectoryExists() @@ -191,14 +212,18 @@ final class ModelDownloadManager: ObservableObject { modelStates[model.filename] = .downloaded onModelDirectoryChanged?() - } catch is CancellationError { - if isInstalled(model: model) { - modelStates[model.filename] = .downloaded + } catch { + // A user-initiated cancel surfaces here as either CancellationError + // (cancelled before URLSession ran) or URLError.cancelled + // (cancelled while in flight). Both should restore the prior + // visible state, not show a failure — the user already knows what + // they did. DownloadOutcomeClassifier owns the discrimination so + // the rule is unit-tested in isolation. + if DownloadOutcomeClassifier.isUserCancellation(error) { + modelStates[model.filename] = isInstalled(model: model) ? .downloaded : .idle } else { - modelStates[model.filename] = .idle + modelStates[model.filename] = .failed(error.localizedDescription) } - } catch { - modelStates[model.filename] = .failed(error.localizedDescription) } } @@ -250,6 +275,11 @@ private final class ModelDownloadSessionDelegate: NSObject, URLSessionDownloadDe private var downloadedFileURL: URL? private var response: URLResponse? private var hasCompleted = false + // Held so `withTaskCancellationHandler` can call .cancel() on it when the + // surrounding Swift Task is cancelled. Without this, Task.cancel() would + // only flip Task.isCancelled — the URLSession download would keep running + // until natural completion, wasting bytes and ignoring the user's intent. + private var activeDownloadTask: URLSessionDownloadTask? init(progressHandler: @escaping @Sendable (Double?) -> Void) { self.progressHandler = progressHandler @@ -260,10 +290,25 @@ private final class ModelDownloadSessionDelegate: NSObject, URLSessionDownloadDe configuration.requestCachePolicy = .reloadIgnoringLocalCacheData let session = URLSession(configuration: configuration, delegate: self, delegateQueue: nil) - return try await withCheckedThrowingContinuation { continuation in - self.continuation = continuation - let task = session.downloadTask(with: url) - task.resume() + // withTaskCancellationHandler bridges Swift Task cancellation into the + // URLSession world. When `Task.cancel()` runs upstream (e.g., from + // ModelDownloadManager.cancel(filename:)), the onCancel block fires and + // aborts the URLSession download task. The delegate then receives + // didCompleteWithError(URLError.cancelled), which resumes the + // continuation throwing — and the catch in performDownload routes it + // through DownloadOutcomeClassifier as a user cancel rather than a + // hard failure. + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + self.continuation = continuation + let task = session.downloadTask(with: url) + self.activeDownloadTask = task + task.resume() + } + } onCancel: { [weak self] in + // URLSessionDownloadTask.cancel() is thread-safe by Apple's docs, + // so calling it from arbitrary cancellation contexts is fine. + self?.activeDownloadTask?.cancel() } } diff --git a/tabby/UI/DownloadableModelCatalogView.swift b/tabby/UI/DownloadableModelCatalogView.swift index a39b68c9..74bfb8f2 100644 --- a/tabby/UI/DownloadableModelCatalogView.swift +++ b/tabby/UI/DownloadableModelCatalogView.swift @@ -18,7 +18,8 @@ struct DownloadableModelCatalogView: View { DownloadableModelRow( model: model, state: modelDownloadManager.state(for: model), - onDownload: { modelDownloadManager.download(model) } + onDownload: { modelDownloadManager.download(model) }, + onCancel: { modelDownloadManager.cancel(filename: model.filename) } ) } @@ -58,6 +59,7 @@ private struct DownloadableModelRow: View { let model: DownloadableRuntimeModel let state: ModelDownloadState let onDownload: () -> Void + let onCancel: () -> Void var body: some View { VStack(alignment: .leading, spacing: 8) { @@ -100,15 +102,30 @@ private struct DownloadableModelRow: View { .buttonStyle(.borderedProminent) .controlSize(.small) case .downloading(let progress): - if let progress { - Text("\(Int((progress * 100).rounded()))%") - .font(.system(size: 11, weight: .medium, design: .monospaced)) - .foregroundStyle(.blue) - .frame(width: 40, alignment: .trailing) - } else { - ProgressView() - .controlSize(.small) - .frame(width: 40) + HStack(spacing: 6) { + if let progress { + Text("\(Int((progress * 100).rounded()))%") + .font(.system(size: 11, weight: .medium, design: .monospaced)) + .foregroundStyle(.blue) + .frame(width: 40, alignment: .trailing) + } else { + ProgressView() + .controlSize(.small) + .frame(width: 40) + } + // Plain SF Symbol button keeps the row compact and matches + // the "Get"/"Retry" button affordance scale. Cancel is the + // affirmative action while downloading, so it gets the same + // visual weight as Get does in the idle state. + Button { + onCancel() + } label: { + Image(systemName: "xmark.circle.fill") + .font(.system(size: 16)) + .foregroundStyle(.secondary) + } + .buttonStyle(.plain) + .help("Cancel download") } case .downloaded: Image(systemName: "checkmark.circle.fill") diff --git a/tabbyTests/DownloadOutcomeClassifierTests.swift b/tabbyTests/DownloadOutcomeClassifierTests.swift new file mode 100644 index 00000000..ad6f1f12 --- /dev/null +++ b/tabbyTests/DownloadOutcomeClassifierTests.swift @@ -0,0 +1,50 @@ +import XCTest +@testable import tabby + +/// Tests for the rule that decides whether a download error is "the user +/// pressed Cancel" or "something went genuinely wrong." +/// +/// This classification matters because the two cases produce different errors +/// at runtime (CancellationError vs URLError.cancelled depending on whether +/// the URLSession download had started yet) but both should restore the prior +/// state, never surface as a user-visible failure. +final class DownloadOutcomeClassifierTests: XCTestCase { + + // MARK: - cancellation surfaces + + func test_isUserCancellation_trueForCancellationError() { + XCTAssertTrue(DownloadOutcomeClassifier.isUserCancellation(CancellationError())) + } + + func test_isUserCancellation_trueForURLErrorCancelled() { + XCTAssertTrue(DownloadOutcomeClassifier.isUserCancellation(URLError(.cancelled))) + } + + // MARK: - real failures + + func test_isUserCancellation_falseForURLErrorTimedOut() { + XCTAssertFalse(DownloadOutcomeClassifier.isUserCancellation(URLError(.timedOut))) + } + + func test_isUserCancellation_falseForURLErrorNotConnected() { + XCTAssertFalse(DownloadOutcomeClassifier.isUserCancellation(URLError(.notConnectedToInternet))) + } + + func test_isUserCancellation_falseForURLErrorBadServerResponse() { + XCTAssertFalse(DownloadOutcomeClassifier.isUserCancellation(URLError(.badServerResponse))) + } + + func test_isUserCancellation_falseForGenericNSError() { + let error = NSError(domain: "TestDomain", code: 42, userInfo: nil) + XCTAssertFalse(DownloadOutcomeClassifier.isUserCancellation(error)) + } + + /// Important: a domain-level runtime error (model unavailable, etc.) must + /// NOT be misclassified as a cancellation. Otherwise a real failure would + /// silently roll back to .idle and the user would never see the problem. + func test_isUserCancellation_falseForLlamaRuntimeError() { + XCTAssertFalse(DownloadOutcomeClassifier.isUserCancellation( + LlamaRuntimeError.unavailable("Model download failed with status code 500.") + )) + } +} From d2f1f492de40465609a9e0c9fe69170ead1c8b7d Mon Sep 17 00:00:00 2001 From: jam-cai Date: Thu, 23 Apr 2026 15:03:13 -0400 Subject: [PATCH 2/3] feat: Validate model downloads against expected size and SHA-256 (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds integrity checks for curated model downloads. After a download finishes, the file is moved to a staging URL, validated against expected size + SHA-256, and only then swapped into the install location. If validation fails, the staged file is removed and the prior install (if any) stays in place — a corrupt download can't take out a working copy. Validation strategy: both size and SHA-256. - Size is a fast pre-check that catches truncated downloads. - SHA-256 is the integrity guarantee against silent corruption (bit flips, partial writes, mirror substitution). - Either alone is insufficient; together they're cheap. Catalog now carries `expectedSizeBytes` and `sha256` for the three curated models. Values were captured from HuggingFace's `x-linked-size` and `x-linked-etag` response headers — refresh procedure documented in the catalog. Both fields are Optional; validators no-op when nil so future catalog additions don't have to wait on metadata to land. Streams the SHA-256 in 1 MB chunks so the largest curated model (~4.5 GB) doesn't get loaded entirely into memory. Tests (11 new, 43 total): - validateSize: match, mismatch (both directions), nil expected, missing file - validateSHA256: known-good hash, uppercase tolerance, mismatch, nil expected, missing file - streaming guard: 3 MB fixture exercises the multi-chunk read loop so a regression to single-shot read would fail the test Verified locally: xcodebuild test -project tabby.xcodeproj -scheme tabby \ -destination 'platform=macOS' CODE_SIGNING_ALLOWED=NO # ** TEST SUCCEEDED ** 43 tests 0 failures Stacked on #60 (download cancel). Folds into #6. Refs #6 --- tabby.xcodeproj/project.pbxproj | 4 + tabby/Models/LlamaRuntimeModels.swift | 31 +++- .../Utilities/ModelDownloadManager.swift | 33 ++++- .../Utilities/ModelFileValidator.swift | 113 ++++++++++++++ tabbyTests/ModelFileValidatorTests.swift | 138 ++++++++++++++++++ 5 files changed, 314 insertions(+), 5 deletions(-) create mode 100644 tabby/Services/Utilities/ModelFileValidator.swift create mode 100644 tabbyTests/ModelFileValidatorTests.swift diff --git a/tabby.xcodeproj/project.pbxproj b/tabby.xcodeproj/project.pbxproj index f31fcb7c..5da0ba02 100644 --- a/tabby.xcodeproj/project.pbxproj +++ b/tabby.xcodeproj/project.pbxproj @@ -12,6 +12,7 @@ A1C3E0112F90000100AAA001 /* Sparkle in Frameworks */ = {isa = PBXBuildFile; productRef = A1C3E0102F90000100AAA001 /* Sparkle */; }; A404828463CADB2ECDAE7AF3 /* LlamaPromptRendererTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F29623C5C0A67B992D383A3C /* LlamaPromptRendererTests.swift */; }; ADEFEE12C197DB6C990E3812 /* SuggestionRequestFactoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8B1121FFDAD30C7F62E15289 /* SuggestionRequestFactoryTests.swift */; }; + AF0F4C853CCA8B86BB5E28CD /* ModelFileValidatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F9D35DB9E86506B9FAE1CFE9 /* ModelFileValidatorTests.swift */; }; B053492719F6106E48290C32 /* SuggestionAvailabilityEvaluatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5A39EC1A44E9160E13E2AE77 /* SuggestionAvailabilityEvaluatorTests.swift */; }; /* End PBXBuildFile section */ @@ -32,6 +33,7 @@ 5A39EC1A44E9160E13E2AE77 /* SuggestionAvailabilityEvaluatorTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SuggestionAvailabilityEvaluatorTests.swift; sourceTree = ""; }; 8B1121FFDAD30C7F62E15289 /* SuggestionRequestFactoryTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = SuggestionRequestFactoryTests.swift; sourceTree = ""; }; F29623C5C0A67B992D383A3C /* LlamaPromptRendererTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = LlamaPromptRendererTests.swift; sourceTree = ""; }; + F9D35DB9E86506B9FAE1CFE9 /* ModelFileValidatorTests.swift */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.swift; path = ModelFileValidatorTests.swift; sourceTree = ""; }; /* End PBXFileReference section */ /* Begin PBXFileSystemSynchronizedRootGroup section */ @@ -89,6 +91,7 @@ 5A39EC1A44E9160E13E2AE77 /* SuggestionAvailabilityEvaluatorTests.swift */, 8B1121FFDAD30C7F62E15289 /* SuggestionRequestFactoryTests.swift */, 562F89255AF340C15A0554BE /* DownloadOutcomeClassifierTests.swift */, + F9D35DB9E86506B9FAE1CFE9 /* ModelFileValidatorTests.swift */, ); path = tabbyTests; sourceTree = ""; @@ -210,6 +213,7 @@ B053492719F6106E48290C32 /* SuggestionAvailabilityEvaluatorTests.swift in Sources */, ADEFEE12C197DB6C990E3812 /* SuggestionRequestFactoryTests.swift in Sources */, 8B6282F0C1CCA0746D96B914 /* DownloadOutcomeClassifierTests.swift in Sources */, + AF0F4C853CCA8B86BB5E28CD /* ModelFileValidatorTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/tabby/Models/LlamaRuntimeModels.swift b/tabby/Models/LlamaRuntimeModels.swift index 4883f55d..3553d6e4 100644 --- a/tabby/Models/LlamaRuntimeModels.swift +++ b/tabby/Models/LlamaRuntimeModels.swift @@ -44,6 +44,15 @@ struct DownloadableRuntimeModel: Equatable, Hashable, Sendable, Identifiable { let displayName: String let downloadURL: URL let approximateSizeInGigabytes: Double + /// Exact byte count of the served file. Optional so future catalog entries + /// can land while metadata is still being filled in. When non-nil, the + /// download manager runs `ModelFileValidator.validateSize` against it + /// before promoting the staged file into the install location. + let expectedSizeBytes: Int64? + /// Lowercase SHA-256 hex string for the served file. Same nullability + /// rationale as `expectedSizeBytes`. HuggingFace exposes this as the + /// `x-linked-etag` response header on its CDN URLs. + let sha256: String? let alternateFilenames: [String] var id: String { filename } @@ -59,12 +68,16 @@ struct DownloadableRuntimeModel: Equatable, Hashable, Sendable, Identifiable { displayName: String, downloadURL: URL, approximateSizeInGigabytes: Double, + expectedSizeBytes: Int64? = nil, + sha256: String? = nil, alternateFilenames: [String] = [] ) { self.filename = filename self.displayName = displayName self.downloadURL = downloadURL self.approximateSizeInGigabytes = approximateSizeInGigabytes + self.expectedSizeBytes = expectedSizeBytes + self.sha256 = sha256 self.alternateFilenames = alternateFilenames } } @@ -84,6 +97,12 @@ enum RuntimeModelCatalog { } /// Canonical downloadable model list shown in Welcome and menu UI. + /// + /// `expectedSizeBytes` and `sha256` were captured from HuggingFace's CDN + /// response headers (`x-linked-size` and `x-linked-etag` respectively). + /// To refresh after a model is updated upstream: + /// + /// curl -sIL "" | grep -iE "^(x-linked-size|x-linked-etag):" static let downloadableModels: [DownloadableRuntimeModel] = [ DownloadableRuntimeModel( filename: "gemma-3-1b-it-Q4_K_M.gguf", @@ -92,7 +111,9 @@ enum RuntimeModelCatalog { string: "https://huggingface.co/unsloth/gemma-3-1b-it-GGUF/resolve/main/gemma-3-1b-it-Q4_K_M.gguf?download=true" )!, - approximateSizeInGigabytes: 0.8 + approximateSizeInGigabytes: 0.8, + expectedSizeBytes: 806_058_272, + sha256: "8270790f3ab69fdfe860b7b64008d9a19986d8df7e407bb018184caa08798ebd" ), DownloadableRuntimeModel( filename: "Qwen3-0.6B-Q4_K_M.gguf", @@ -101,7 +122,9 @@ enum RuntimeModelCatalog { string: "https://huggingface.co/unsloth/Qwen3-0.6B-GGUF/resolve/main/Qwen3-0.6B-Q4_K_M.gguf?download=true" )!, - approximateSizeInGigabytes: 0.4 + approximateSizeInGigabytes: 0.4, + expectedSizeBytes: 396_705_472, + sha256: "ac2d97712095a558e31573f62f466a3f9d93990898b0ec79d7c974c1780d524a" ), DownloadableRuntimeModel( filename: "gemma-3n-E4B-it-Q4_K_M.gguf", @@ -110,7 +133,9 @@ enum RuntimeModelCatalog { string: "https://huggingface.co/unsloth/gemma-3n-E4B-it-GGUF/resolve/main/gemma-3n-E4B-it-Q4_K_M.gguf?download=true" )!, - approximateSizeInGigabytes: 3.5 + approximateSizeInGigabytes: 3.5, + expectedSizeBytes: 4_539_054_208, + sha256: "43b489bb77a81bda85180e7c490d40ad7f1d5c2ce654c9b05e15e104bd3c777e" ), ] } diff --git a/tabby/Services/Utilities/ModelDownloadManager.swift b/tabby/Services/Utilities/ModelDownloadManager.swift index f9faf525..1bb66717 100644 --- a/tabby/Services/Utilities/ModelDownloadManager.swift +++ b/tabby/Services/Utilities/ModelDownloadManager.swift @@ -204,11 +204,40 @@ final class ModelDownloadManager: ObservableObject { try validate(response: downloadResult.response) let fileManager = FileManager.default + + // Stage-validate-swap so a corrupt download can't take out a + // working previous install. If validation throws, the staged + // file is removed and the existing destinationURL (if any) + // stays untouched. + let stagingURL = runtimeDirectoryURL.appendingPathComponent( + "\(model.filename).staging-\(UUID().uuidString)", + isDirectory: false + ) + try fileManager.moveItem(at: downloadResult.temporaryURL, to: stagingURL) + + do { + try ModelFileValidator.validateSize( + of: stagingURL, + expectedBytes: model.expectedSizeBytes + ) + try ModelFileValidator.validateSHA256( + of: stagingURL, + expectedSHA256: model.sha256 + ) + } catch { + // Don't leave a partial or corrupt file in the runtime + // directory where the locator might pick it up later. + try? fileManager.removeItem(at: stagingURL) + throw error + } + + // Validation passed — atomically swap the new file in. The + // existing copy is removed only at this point, so any failure + // before here leaves the prior install intact. if fileManager.fileExists(atPath: destinationURL.path) { try fileManager.removeItem(at: destinationURL) } - - try fileManager.moveItem(at: downloadResult.temporaryURL, to: destinationURL) + try fileManager.moveItem(at: stagingURL, to: destinationURL) modelStates[model.filename] = .downloaded onModelDirectoryChanged?() diff --git a/tabby/Services/Utilities/ModelFileValidator.swift b/tabby/Services/Utilities/ModelFileValidator.swift new file mode 100644 index 00000000..38a8ba01 --- /dev/null +++ b/tabby/Services/Utilities/ModelFileValidator.swift @@ -0,0 +1,113 @@ +import Foundation +import CryptoKit + +/// File overview: +/// Verifies a downloaded GGUF file against expected size and SHA-256 metadata +/// before the download manager promotes it to the installed location. +/// +/// Why both size and checksum: +/// Size is a fast pre-check that catches truncated downloads (cancelled mid- +/// transfer, server hung up). SHA-256 is the real integrity guarantee against +/// silent corruption — bit flips, mirror-side substitution, partial writes +/// that happen to land at the right byte count. Either alone is insufficient: +/// matching size with wrong contents is plausible for any transport bug, and +/// computing SHA-256 first is wasteful when bytes-on-disk is wrong. +/// +/// Why both validators take optional expected values: +/// `DownloadableRuntimeModel.expectedSizeBytes` and `.sha256` are optional so +/// future catalog entries can land without metadata immediately. When the +/// expected value is nil, the validator no-ops — better than failing every +/// install just because metadata wasn't filled in yet. The catalog is the +/// authoritative source of "what should be true about this file." +enum ModelFileValidator { + + enum ValidationError: LocalizedError { + case sizeMismatch(expected: Int64, actual: Int64) + case checksumMismatch(expected: String, actual: String) + case fileUnreadable(URL) + + var errorDescription: String? { + switch self { + case let .sizeMismatch(expected, actual): + return "Downloaded file is \(actual) bytes; expected \(expected). The download may have been truncated." + case let .checksumMismatch(expected, actual): + let expectedShort = String(expected.lowercased().prefix(16)) + let actualShort = String(actual.lowercased().prefix(16)) + return "Downloaded file's checksum (\(actualShort)…) doesn't match the expected (\(expectedShort)…). The file may be corrupt." + case let .fileUnreadable(url): + return "Couldn't read \(url.lastPathComponent) for validation." + } + } + } + + /// Throws if the file's byte size differs from `expectedBytes`. + /// No-op when `expectedBytes` is nil. + static func validateSize( + of url: URL, + expectedBytes: Int64?, + fileManager: FileManager = .default + ) throws { + guard let expectedBytes else { + return + } + let attributes: [FileAttributeKey: Any] + do { + attributes = try fileManager.attributesOfItem(atPath: url.path) + } catch { + throw ValidationError.fileUnreadable(url) + } + guard let actualNumber = attributes[.size] as? NSNumber else { + throw ValidationError.fileUnreadable(url) + } + let actualBytes = actualNumber.int64Value + if actualBytes != expectedBytes { + throw ValidationError.sizeMismatch(expected: expectedBytes, actual: actualBytes) + } + } + + /// Throws if the file's SHA-256 differs from `expectedSHA256` (case-insensitive). + /// No-op when `expectedSHA256` is nil. + /// + /// Streams the file in 1 MB chunks so the largest curated GGUF (~4.5 GB) + /// doesn't get fully loaded into memory. + static func validateSHA256(of url: URL, expectedSHA256: String?) throws { + guard let expectedSHA256 else { + return + } + let actualHex = try sha256Hex(of: url) + // Lowercase comparison: HuggingFace returns hex in lowercase, but we + // accept either case in the catalog so a hand-pasted upper-case + // checksum won't silently fail every download. + if actualHex.lowercased() != expectedSHA256.lowercased() { + throw ValidationError.checksumMismatch(expected: expectedSHA256, actual: actualHex) + } + } + + /// Computes the SHA-256 of the file as a lowercase hex string. + /// Streams the file in chunks so memory stays bounded regardless of size. + private static func sha256Hex(of url: URL) throws -> String { + let handle: FileHandle + do { + handle = try FileHandle(forReadingFrom: url) + } catch { + throw ValidationError.fileUnreadable(url) + } + defer { try? handle.close() } + + var hasher = SHA256() + let chunkSize = 1024 * 1024 // 1 MB — small enough to stay out of large allocations, large enough to keep syscall overhead negligible. + while true { + let chunk: Data + do { + chunk = try handle.read(upToCount: chunkSize) ?? Data() + } catch { + throw ValidationError.fileUnreadable(url) + } + if chunk.isEmpty { + break + } + hasher.update(data: chunk) + } + return hasher.finalize().map { String(format: "%02x", $0) }.joined() + } +} diff --git a/tabbyTests/ModelFileValidatorTests.swift b/tabbyTests/ModelFileValidatorTests.swift new file mode 100644 index 00000000..e9346581 --- /dev/null +++ b/tabbyTests/ModelFileValidatorTests.swift @@ -0,0 +1,138 @@ +import XCTest +import CryptoKit +@testable import tabby + +/// Tests for the size + SHA-256 validation that runs against staged downloads +/// before they're promoted to the install location. +/// +/// The streaming SHA-256 path is exercised against a multi-chunk fixture so +/// the chunked-read loop is actually executed by at least one test — without +/// that, the implementation could regress to a single-shot read and pass the +/// happy-path tests anyway. +final class ModelFileValidatorTests: XCTestCase { + + private var fixtures: [URL] = [] + + override func tearDown() { + fixtures.forEach { try? FileManager.default.removeItem(at: $0) } + fixtures.removeAll() + super.tearDown() + } + + // MARK: - validateSize + + func test_validateSize_passesWhenSizeMatches() throws { + let url = try makeFixture(contents: Data(repeating: 0xAB, count: 100)) + XCTAssertNoThrow(try ModelFileValidator.validateSize(of: url, expectedBytes: 100)) + } + + func test_validateSize_throwsSizeMismatchWhenLargerThanExpected() throws { + let url = try makeFixture(contents: Data(repeating: 0xAB, count: 200)) + XCTAssertThrowsError(try ModelFileValidator.validateSize(of: url, expectedBytes: 100)) { error in + guard case ModelFileValidator.ValidationError.sizeMismatch(let expected, let actual) = error else { + XCTFail("Expected sizeMismatch, got \(error)") + return + } + XCTAssertEqual(expected, 100) + XCTAssertEqual(actual, 200) + } + } + + func test_validateSize_throwsSizeMismatchWhenSmallerThanExpected() throws { + let url = try makeFixture(contents: Data(repeating: 0xAB, count: 50)) + XCTAssertThrowsError(try ModelFileValidator.validateSize(of: url, expectedBytes: 100)) + } + + func test_validateSize_isNoOpWhenExpectedNil() throws { + let url = try makeFixture(contents: Data()) + XCTAssertNoThrow(try ModelFileValidator.validateSize(of: url, expectedBytes: nil)) + } + + func test_validateSize_throwsFileUnreadableWhenFileMissing() { + let phantom = FileManager.default.temporaryDirectory + .appendingPathComponent("validator-test-phantom-\(UUID().uuidString)") + XCTAssertThrowsError(try ModelFileValidator.validateSize(of: phantom, expectedBytes: 100)) { error in + guard case ModelFileValidator.ValidationError.fileUnreadable = error else { + XCTFail("Expected fileUnreadable, got \(error)") + return + } + } + } + + // MARK: - validateSHA256 + + func test_validateSHA256_passesForKnownChecksum() throws { + // SHA-256 of "hello" — pinned so the test would fail if the + // implementation accidentally hashed something different (e.g., + // appended a trailing newline). + let url = try makeFixture(contents: Data("hello".utf8)) + XCTAssertNoThrow(try ModelFileValidator.validateSHA256( + of: url, + expectedSHA256: "2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824" + )) + } + + func test_validateSHA256_acceptsUppercaseExpected() throws { + let url = try makeFixture(contents: Data("hello".utf8)) + XCTAssertNoThrow(try ModelFileValidator.validateSHA256( + of: url, + expectedSHA256: "2CF24DBA5FB0A30E26E83B2AC5B9E29E1B161E5C1FA7425E73043362938B9824" + )) + } + + func test_validateSHA256_throwsChecksumMismatchOnBadHash() throws { + let url = try makeFixture(contents: Data("hello".utf8)) + XCTAssertThrowsError(try ModelFileValidator.validateSHA256( + of: url, + expectedSHA256: String(repeating: "0", count: 64) + )) { error in + guard case ModelFileValidator.ValidationError.checksumMismatch = error else { + XCTFail("Expected checksumMismatch, got \(error)") + return + } + } + } + + func test_validateSHA256_isNoOpWhenExpectedNil() throws { + let url = try makeFixture(contents: Data("anything".utf8)) + XCTAssertNoThrow(try ModelFileValidator.validateSHA256(of: url, expectedSHA256: nil)) + } + + /// Streaming guard — exercises the multi-chunk read loop. Without this, + /// a regression to a single read(upToCount:) of the entire file would + /// pass every other test in this suite. + func test_validateSHA256_handlesFileLargerThanChunkSize() throws { + // 3 MB file (>1 MB chunk) of repeating bytes. Computed via CryptoKit + // here so the test is self-checking — the goal isn't to verify + // CryptoKit, it's to verify the streaming read produces the same + // hash as a single-shot computation. + let bytes = Data(repeating: 0x42, count: 3 * 1024 * 1024) + let url = try makeFixture(contents: bytes) + let expected = SHA256.hash(data: bytes).map { String(format: "%02x", $0) }.joined() + XCTAssertNoThrow(try ModelFileValidator.validateSHA256(of: url, expectedSHA256: expected)) + } + + func test_validateSHA256_throwsFileUnreadableWhenFileMissing() { + let phantom = FileManager.default.temporaryDirectory + .appendingPathComponent("validator-test-phantom-\(UUID().uuidString)") + XCTAssertThrowsError(try ModelFileValidator.validateSHA256( + of: phantom, + expectedSHA256: String(repeating: "a", count: 64) + )) { error in + guard case ModelFileValidator.ValidationError.fileUnreadable = error else { + XCTFail("Expected fileUnreadable, got \(error)") + return + } + } + } + + // MARK: - Helpers + + private func makeFixture(contents: Data) throws -> URL { + let url = FileManager.default.temporaryDirectory + .appendingPathComponent("validator-test-\(UUID().uuidString)") + try contents.write(to: url) + fixtures.append(url) + return url + } +} From d4e7e3c021b137d7d5bd030bf17fafa284d9f22c Mon Sep 17 00:00:00 2001 From: jam-cai Date: Sun, 26 Apr 2026 16:10:44 -0400 Subject: [PATCH 3/3] feat: Make download failure messages legible in the model catalog (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failure messages now have their own wrapping row below the row body, with a warning icon. Validation errors include exact byte counts and partial checksum prefixes — fitting them inline with the size label clipped them on smaller windows. Changes: - New `failureMessageRow(message:)` renders the localized error in red with `exclamationmark.triangle.fill`. Pinned to fixedSize(vertical:) so SwiftUI allocates space for the wrap rather than truncating. - Metadata text in `.failed` state now shows "Download failed" instead of the full statusText — the full message lives in the dedicated row. - Retry button gets `arrow.counterclockwise` symbol; pairs with the warning row to reinforce "something went wrong, try again." No new tests — pure SwiftUI tweak. Verified the row still builds: xcodebuild build -project tabby.xcodeproj -scheme tabby \ -configuration Debug -destination 'platform=macOS' \ CODE_SIGNING_ALLOWED=NO # ** BUILD SUCCEEDED ** And the existing 43 tests still pass. Stacked on #61 (validation) and #60 (cancel). Closes the last acceptance criterion of #6 — failed downloads can be retried from the same UI with a legible error. Refs #6 --- tabby/UI/DownloadableModelCatalogView.swift | 44 +++++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/tabby/UI/DownloadableModelCatalogView.swift b/tabby/UI/DownloadableModelCatalogView.swift index 74bfb8f2..508bb22b 100644 --- a/tabby/UI/DownloadableModelCatalogView.swift +++ b/tabby/UI/DownloadableModelCatalogView.swift @@ -85,6 +85,14 @@ private struct DownloadableModelRow: View { if state.isDownloading { downloadProgressBar } + + // Failure messages get their own wrapping row. Validation errors + // include exact byte counts and partial checksum prefixes, which + // would clip the size label or get truncated to one line if we + // tried to fit them inline with the metadata. + if case .failed(let message) = state { + failureMessageRow(message: message) + } } .padding(.vertical, 6) .padding(.horizontal, 10) @@ -94,6 +102,23 @@ private struct DownloadableModelRow: View { ) } + /// Renders the localized error from `.failed(message)` as wrapping red + /// text below the row body. Pinned to `fixedSize(vertical:)` so SwiftUI + /// allocates enough vertical space for the wrap rather than clipping. + @ViewBuilder + private func failureMessageRow(message: String) -> some View { + HStack(alignment: .top, spacing: 6) { + Image(systemName: "exclamationmark.triangle.fill") + .font(.system(size: 11)) + .foregroundStyle(.red) + Text(message) + .font(.system(size: 11)) + .foregroundStyle(.red) + .fixedSize(horizontal: false, vertical: true) + .frame(maxWidth: .infinity, alignment: .leading) + } + } + @ViewBuilder private var modelActionButton: some View { switch state { @@ -132,9 +157,16 @@ private struct DownloadableModelRow: View { .foregroundStyle(.green) .font(.system(size: 16)) case .failed: - Button("Retry") { onDownload() } - .buttonStyle(.bordered) - .controlSize(.small) + // Refresh symbol gives the button a visual cue that it's not just + // any "tap to do something"; pairs with the warning row below to + // reinforce "something went wrong, try again." + Button { + onDownload() + } label: { + Label("Retry", systemImage: "arrow.counterclockwise") + } + .buttonStyle(.bordered) + .controlSize(.small) } } @@ -148,7 +180,11 @@ private struct DownloadableModelRow: View { case .downloaded: installationStatus = "Installed" case .failed: - installationStatus = state.statusText + // Terse here; the full message lives in `failureMessageRow` below + // where it has room to wrap. Mixing a multi-line error into the + // size-label line would either truncate or push the size off + // screen on smaller windows. + installationStatus = "Download failed" } return "\(installationStatus) • \(model.approximateSizeLabel)"