fix: preserve model cache when first load is cancelled#749
Conversation
DownloadUtils.loadModels treats any first-load failure as cache corruption and deletes the repo folder before re-downloading. A cancelled load (CancellationError, NSURLErrorCancelled -999, or NSUserCancelledError, including in the underlying-error chain) is not corruption: cancelling mid-load during app teardown wiped fully downloaded multi-hundred-MB model repos. Classify cancellation via isCancellationError and rethrow without touching the cache. Offline mode behaviour is unchanged.
There was a problem hiding this comment.
Pull request overview
This PR prevents DownloadUtils.loadModels from treating cancellations as cache corruption, avoiding deletion of a valid on-disk model repository when the initial load is cancelled (e.g., app teardown / task cancellation).
Changes:
- Add
DownloadUtils.isCancellationError(_:)to classify common cancellation shapes (SwiftCancellationError,NSURLErrorCancelled-999,NSUserCancelledError, plus underlying-error chains). - Gate the delete-and-redownload fallback in
loadModelsso cancellations rethrow without wiping the cache. - Add a focused XCTest suite covering positive/negative cancellation classification cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Sources/FluidAudio/DownloadUtils.swift | Adds cancellation classification and skips cache deletion on cancelled first loads. |
| Tests/FluidAudioTests/Shared/DownloadUtilsCancellationTests.swift | Adds unit tests to validate cancellation detection behavior and non-cancellation negatives. |
| /// `true` when `error` represents cancellation rather than a corrupted | ||
| /// cache: Swift `CancellationError`, `NSURLErrorCancelled` (-999), or | ||
| /// `NSUserCancelledError` — including anywhere in the underlying-error | ||
| /// chain (`NSUnderlyingErrorKey`). |
There was a problem hiding this comment.
Done in 7c4d206 — doc now states the 8-link depth cap and the rationale (cycle protection; real-world wrapping is 1–2 levels deep).
| func testUnderlyingChainCycleTerminates() { | ||
| // Self-referential underlying chain must not loop forever; the | ||
| // walker is depth-capped. | ||
| let inner = NSError(domain: "com.example.a", code: 1) | ||
| let outer = NSError( | ||
| domain: "com.example.b", code: 2, | ||
| userInfo: [NSUnderlyingErrorKey: inner]) | ||
| XCTAssertFalse(DownloadUtils.isCancellationError(outer)) | ||
| } |
There was a problem hiding this comment.
Done in 7c4d206 — renamed to testNonCancellationUnderlyingChainNotClassified (it never built a cycle), and added testDepthCapTerminatesLongChain which actually exercises the cap on a 12-link chain.
- isCancellationError doc now states the 8-link depth cap and why (cycle protection; real wrapping is 1-2 levels). - Rename testUnderlyingChainCycleTerminates to testNonCancellationUnderlyingChainNotClassified (it never built a cycle) and add testDepthCapTerminatesLongChain covering the cap on a 12-link chain.
| } | ||
| } | ||
|
|
||
| /// `true` when `error` represents cancellation rather than a corrupted |
There was a problem hiding this comment.
the comment is abit too verbose , if users or agents want more context they will use commit history as context reference.
There was a problem hiding this comment.
Trimmed in 243a591 — doc comment is now three lines; the depth-cap rationale moved onto the constant itself.
|
|
||
| var current: NSError? = error as NSError | ||
| var depth = 0 | ||
| while let nsError = current, depth < 8 { |
There was a problem hiding this comment.
why specifically 8 , also ideally this should be a constant somewhere
There was a problem hiding this comment.
Done in 243a591 — extracted maxUnderlyingErrorDepth = 8 with a one-line rationale (cycle guard; real-world wrapping is 1–2 levels deep; 8 leaves generous headroom).
| /// Used by ``loadModels(_:modelNames:directory:computeUnits:variant:progressHandler:)`` | ||
| /// to skip the delete-cache-and-redownload fallback for cancelled loads. | ||
| /// Guards the `NSUnderlyingErrorKey` walk against pathological or cyclic | ||
| /// error chains; real-world wrapping is 1–2 levels deep. |
There was a problem hiding this comment.
so why do we need 8 then ? why not just 2
There was a problem hiding this comment.
The depth limit was pretty arbitrary. In reality, NSError chains are usually only 1–2 levels deep and a plain NSError can't even form a cycle because userInfo is immutable
…tionError The NSUnderlyingErrorKey walk was depth-capped at an arbitrary 8 links. Neither 8 nor any other constant is principled: real chains are 1-2 deep, and a plain NSError cannot even form a cycle (userInfo is fixed at init). Track visited errors by identity instead. This removes the magic number, terminates on any genuine cycle, and walks legit-but-deep acyclic chains to the end -- so a cancellation buried at any depth is still detected (the safe direction: never wipe a good cache). Tests: real self-referential cycle via NSError subclass (plain NSError can't cycle); deep-buried-cancellation now asserts detected.
…tionError Mirror of PR FluidInference#749 fix (fork commit e44eff7) onto the integration branch. The NSUnderlyingErrorKey walk was depth-capped at an arbitrary 8 links; neither 8 nor any constant is principled (real chains are 1-2 deep, and a plain NSError can't even form a cycle). Track visited errors by identity instead: no magic number, terminates on any genuine cycle, and a cancellation buried at any depth is still detected (safe direction).
Problem
DownloadUtils.loadModelstreats any first-load failure as cache corruption: it deletes the whole repo cache folder and re-downloads. But cancellation is not corruption — if the load is cancelled (app teardown, user abort,Taskcancellation propagating asCancellationErrororNSURLErrorCancelled-999), the fallback wipes a perfectly valid, fully-downloaded model cache. We hit this in production: cancelling a Sortformer load during app stop deleted a 246 MB downloaded model, forcing a full re-download on next launch.Fix
Add an
isCancellationErrorclassifier and check it before the delete-and-redownload fallback. Cancelled loads rethrow without touching the cache. Detected shapes:CancellationErrorNSURLErrorDomaincodeNSURLErrorCancelled(-999)NSCocoaErrorDomaincodeNSUserCancelledErrorNSUnderlyingErrorKeychain (depth-capped)Offline-mode behaviour (
enforceOffline) is unchanged — it already skipped the wipe.Tests
Tests/FluidAudioTests/Shared/DownloadUtilsCancellationTests.swift— 10 cases covering all detected cancellation shapes plus negatives (timeout, corrupt-file error, -999 in a non-URL domain, chain termination).swift build+swift test --filter DownloadUtilsCancellationTestsgreen on macOS.🤖 Generated with Claude Code