T-1900 — Cancelled SVG detail reload can overwrite the current viewer state. Adds LoadGeneration, a shared monotonic token, and routes every loadState write in WebImageDetailSheet (iOS) and ImageDetailWindow (macOS) through a commitLoadState guard. PR #393.
grep -n 'loadState' over both views returns exactly one assignment each — WebImageDetailSheet.swift:168 and ImageDetailWindow.swift:431, both inside commitLoadState. No unguarded write survives.begin() is provably on the MainActor before the first await. The app target sets SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor (project.pbxproj:424, :478), and SwiftUI's .task(id:) takes an @_inheritActorContext sending @isolated(any) closure formed in @MainActor body. No data race on the @State mutation.SVGRenderer is @MainActor (SVGRenderer.swift:19) and suspends at try await Task.sleep(for: .milliseconds(100)) on the MainActor (:64). A cancelled reload resumes with no hop into loadSVG's generic catch; if SwiftUI cancels-then-creates, the old token is still current and the stale failure commits.error.localizedDescription, so a leaked CancellationError renders as “The operation couldn't be completed. (Swift.CancellationError error 1.)” — unlocalised, and a CLAUDE.md localisation violation in spirit. The raster path yields “Network error: Cancelled”.LoadGeneration.swift:11-16 claims the cancelled task “does not observe that cancellation” and that Task.checkCancellation() “does not close this”. SVGRenderer.swift:65 calls Task.checkCancellation() explicitly; ImageLoader and SVGSourceLoader both map cancellation to .networkError("Cancelled"). The pipeline is cancellation-aware.WebImageDetailSheet or ImageDetailWindow. A future loadState = .failed(…) silently restores T-1900 — the exact failure class CLAUDE.md records for T-1943. ProductionSourceScan already exists for this.NotesManager.loadGeneration, DocumentSession.parseGeneration, RemoteRefreshFlow.currentRequestID, RemoteContentCoordinator, RawSourceViewModel.linesGeneration, FootnotePopoverWebPage.renderGeneration. The new type's name also collides with NotesManager.loadGeneration, which is a counter value, not a counter object.Needs fixes
The change is correct, well-scoped and a strict improvement: every loadState write in both views now goes through the guard (verified by grep — exactly one assignment per file), begin() is provably on the MainActor before the first suspension, the token-0 change is unreachable in production, and no stuck-spinner path is introduced. Both builds pass warning-free, SwiftLint reports 0 violations, the WebKit isolation guard passes, and LoadGenerationTests executes 5/5 green on the real test host.
But it implements only half of the fix the ticket itself prescribed, and the omitted half is the one that deterministically covers the scenario in the ticket's title. T-1900 says: “Capture the reload key/generation at task start and only commit loadState if it is still current; treat CancellationError as a silent return.” The generation half shipped; the cancellation half did not. On the SVG render path — the path the ticket is named after — SVGRenderer is @MainActor and its cancellation window is a MainActor Task.sleep, so a cancelled reload's CancellationError reaches commitLoadState with no actor hop. If SwiftUI's .task(id:) cancels before it creates, the stale task's token is still current and the guard passes, committing a failure over the newer result — exactly the reported symptom. The generation counter closes this probabilistically; catch is CancellationError { return } closes it by definition, and is the pattern RemoteRefreshFlow and RemoteContentCoordinator already use alongside their own generation counters.
Add the one-line cancellation guard, correct the LoadGeneration doc comment (which asserts a mechanism the pipeline contradicts), and soften the CHANGELOG claim. Everything else is follow-up material.
178524f7 Fix T-1900: guard SVG/image detail reloads against a superseded commit bd921ac9 Fix T-1900: rewrite race test to exercise the live generation counter working-tree No fixes applied — this review is editorial only When you tap an image in Prism to view it fullscreen, the app has to fetch and draw that image. If the picture is an SVG (a drawing described in text rather than stored as dots), the app has to redraw it whenever the appearance changes — switching between light and dark mode, or on a Mac, dragging the window to a screen with a different sharpness.
Each of those changes kicks off a fresh load. The problem was that the old load didn't stop cleanly. It carried on in the background, finished late, and wrote its result over the top of the newer one — so you could end up looking at the picture for the appearance you just left, or at an error message saying the load was cancelled, even though a perfectly good image had already arrived.
It's a visible wrongness in the viewer: you switch to dark mode and get a stale light-mode drawing, or an error where a picture should be. Nothing is lost or corrupted — but what's on screen stops matching what you asked for, and there's no obvious way to make it right again short of closing and reopening.
The ticket-number scheme is correctly built and correctly applied everywhere it needs to be. But that last point — the cancelled attempt reporting an error — still has a narrow gap, on exactly the SVG path this bug was reported against. Closing it takes one more line.
Both detail viewers reload through .task(id:) keyed on (colorScheme, displayScale). On a re-key SwiftUI cancels the in-flight task and starts a replacement, but the two tasks are still racing to write the same @State, and arrival order is not start order.
The fix introduces LoadGeneration, a Sendable value type wrapping a monotonic UInt64:
mutating func begin() -> UInt64 // supersede, hand out a token
func isCurrent(_ token: UInt64) -> Bool // token != 0 && token == currentEach view holds it as @State, calls begin() as the first statement of its load function (before any suspension), threads the returned token down through the SVG sub-path, and funnels every state write through commitLoadState(_:token:), which drops the write when the token is stale.
NotesManager, DocumentSession, RemoteRefreshFlow, RemoteContentCoordinator, RawSourceViewModel, FootnotePopoverWebPage) — it's just the first time it has been given a name. StoreManager's beginEntitlementObservation() / commitEntitlement(_:observedAt:) is the closest structural twin.commitLoadState re-reads the live counter rather than a value captured earlier — and the rewritten test deliberately mirrors that, which is what makes it a real race test rather than a restatement of two synchronous begin() calls.imageCache/snapshotCache. Because keys are derived from resolved identity plus appearance, a stale load can only write its own key — so the write is harmless and the newer load may benefit from it.token: UInt64 is now a parameter on the SVG sub-path purely to reach the commit helper, and there are ~13 call sites across the two views. Having the load functions return a LoadState and committing once at the top would delete the parameter and make “exactly one write” structural rather than a matter of discipline.The app target sets SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor in both configurations (project.pbxproj:424, :478) with SWIFT_APPROACHABLE_CONCURRENCY = YES and SWIFT_VERSION = 5.0; SWIFT_STRICT_CONCURRENCY is unset and there is no .xcconfig. So LoadGeneration, both load functions and both commitLoadState helpers are @MainActor by default isolation. Independently, SwiftUI's .task(id:) takes @_inheritActorContext _ action: sending @escaping @isolated(any) () async -> Void, so a closure formed in @MainActor body starts its Task on the MainActor. begin() is the first statement in both load functions, before any suspension. The @State mutation is single-threaded by construction.
Two consequences worth naming. First, : Sendable on LoadGeneration is redundant — a global-actor-isolated value type is implicitly Sendable. Second, prismTests does not set SWIFT_DEFAULT_ACTOR_ISOLATION (project.pbxproj:645-648, :670-673), so the synchronous nonisolated test bodies call a @MainActor type across isolation. Silent under -swift-version 5; a diagnostic under Swift 6. Latent migration debt, matching the asymmetry already recorded in project memory. It is not a race — the value type is copied per test.
The guard wins only if T_new.begin() runs before T_old's resumption reaches commitLoadState. On the SVG-render path that ordering is genuinely in doubt:
@MainActor final class SVGRenderer { // SVGRenderer.swift:19-20
func render(...) async throws -> PlatformImage {
try await Task.sleep(for: .milliseconds(100)) // :64 <- MainActor suspension
try Task.checkCancellation() // :65Cancelling T_old fires Task.sleep's cancellation handler, enqueuing T_old's resumption on the MainActor. If SwiftUI's internal _TaskValueModifier cancels before it creates, that resumption is enqueued first; both tasks are .userInitiated, so FIFO runs T_old first. render and loadSVG are both MainActor, so the CancellationError propagates with no hop into the generic catch and calls commitLoadState while current is still the old token. Guard passes; stale failure committed; T_new then overwrites it — a visible flash.
_TaskValueModifier is @usableFromInline internal with no inlinable body in the .swiftinterface, so the ordering cannot be read from source. It is unknown, not benign. The ImageLoader/SVGSourceLoader paths are narrower because both are actors, so cancellation observed inside them needs an extra hop back that T_new will almost certainly beat.
Two strings, both bad. ImageLoader.load is async throws(ImageLoadError), so cancellation is mapped at four sites to .networkError("Cancelled") → “Network error: Cancelled”, mislabelled as a network failure. SVGRenderer.render is untyped async throws, so a bare CancellationError reaches the generic catch, which uses error.localizedDescription rather than displayMessage → “The operation couldn't be completed. (Swift.CancellationError error 1.)”. Unlocalised, and precisely the string T-1900 exists to stop showing.
Cancellation is definitionally true for the loser regardless of scheduling. guard !Task.isCancelled else { return } at the top of commitLoadState, or catch is CancellationError { return } per chain, closes the residual completely. This is the house pattern: RemoteRefreshFlow.swift:117/121/127/134 and RemoteContentCoordinator.swift:89/97/100 both combine catch is CancellationError and a request-id check; DocumentScrollContent.swift:279 uses the cancellation half alone via DocumentLoadingIndicatorPolicy.mayClearFlag(wasCancelled:). This branch is the only place using the generation half alone — and its doc comment declares the other half insufficient, which contradicts DocumentScrollContent's documented T-1744 rationale one layer down.
Dismissal: when the view goes away nothing bumps the generation, so the current token stays current and the failure is committed — unchanged from before, and unobservable because the view is gone. The begin()/commit pairing is total: every return path either commits or tail-calls a function that commits on all five branches. The newest token is current by definition, so no stuck spinner is introduced.
Token 0: isCurrent has exactly two production callers, both commitLoadState, both fed from begin(), which returns current += 1 starting at 0 — i.e. always ≥ 1. The token != 0 branch is unreachable in production and exercised only by the artificial test. Harmless; the test should not be read as covering a real path. += 1 traps on overflow where every existing counter in the repo uses &+= 1 — unreachable at ~1.8×1019 loads, but a silent divergence from house convention.
T-2142 (media caches survive reloads): no adverse interaction. Cache writes sit before the guard, but keys are resolved-identity-plus-appearance derived, so a superseded load writes only its own key. The scenario where that matters — bytes changing at the same URL after a document reload — is pre-existing, unchanged by this branch, and exactly what T-2142 tracks. Guarding the cache writes (which the ticket's suggested fix mentions) would not help T-2142 and would lose a free cache warm.
LoadGeneration.swift
Why it matters. This is the whole mechanism. It is small, pure, Sendable, and correct — begin() supersedes and hands out a token, isCurrent() answers whether that token is still newest. Reviewer interest: it is the seventh instance of this idea in the repo and the first with a name, so it either becomes the shared primitive or it becomes a one-off whose name collides with NotesManager.loadGeneration.
What to look at. prism/Services/LoadGeneration.swift:24-42
WebImageDetailSheet.swift
Why it matters. The correctness claim of this PR is 'every loadState write goes through the guard'. That claim holds: grep over both views returns exactly one assignment each, both inside the helper. This is the finding a reviewer most needs to confirm, and it confirms cleanly.
WebImageDetailSheet.swift
Why it matters. MAJOR. T-1900's suggested fix reads: 'Capture the reload key/generation at task start and only commit loadState if it is still current; treat CancellationError as a silent return.' The first clause shipped, the second did not — and the second is the one that deterministically covers the scenario in the ticket's title. SVGRenderer is @MainActor and its cancellation window is a MainActor Task.sleep (SVGRenderer.swift:64-65), so a cancelled reload's error reaches commitLoadState with no actor hop; if SwiftUI cancels before it creates, the stale token is still current and the guard passes.
LoadGeneration.swift
Why it matters. MINOR, but this repo is meticulous about comments describing the mechanism that actually runs. LoadGeneration.swift:11-16 asserts the cancelled task 'does not observe that cancellation — it keeps running', and that 'Task.checkCancellation() does not close this'. SVGRenderer.swift:65 calls Task.checkCancellation() explicitly; ImageLoader.swift:317 and SVGSourceLoader.swift:164 both catch CancellationError and map it to .networkError("Cancelled"); ImageLoader.swift:40 uses withTaskCancellationHandler. The pipeline is thoroughly cancellation-aware.
What to look at. prism/Services/LoadGeneration.swift:11-16
LoadGenerationTests.swift
Why it matters. The round-1 version took a frozen snapshot of LoadGeneration before either branch ran, so its outcome was fixed by two synchronous begin() calls — it duplicated laterBeginSupersedesEarlierToken and exercised no race. The replacement is genuinely deterministic: a @MainActor harness mirroring the views' state pair, plus a Gate actor that blocks until the stale load has provably reached its suspension point.
What to look at. prismTests/LoadGenerationTests.swift:58-135 (LoadCommitHarness, Gate, supersededLoadCannotWinCommitRace)
LoadGenerationTests.swift
Why it matters. MAJOR. The suite tests the primitive and a stand-in harness that mirrors the views. It never observes WebImageDetailSheet or ImageDetailWindow. A future edit writing loadState = .failed(…) directly restores T-1900 with the whole suite green — the exact failure class CLAUDE.md records for T-1943 ('a direct-invocation test cannot see missing wiring, that is exactly how T-1943 survived the cutover and every review').
What to look at. prismTests/LoadGenerationTests.swift — and the absent scan rule; compare prismTests/ImageMaterializationChokepointTests.swift
Stated in LoadGeneration.swift:10-16 and the first commit message. The counter does not depend on SwiftUI's internal cancel/create ordering and matches six existing in-repo precedents, so it is the right primary mechanism.
The stated grounds, however, do not hold: the comment claims the cancelled task never observes cancellation, which is false for both the raster and SVG paths. The decision is right; the justification needs rewriting, and it should say that the cancellation check is complementary, not superseded.
Added in commit bd921ac9 after the first real test-host run surfaced isCurrent(0) reading as true on a fresh generation. Explicitly documented as having no production effect — confirmed: isCurrent has exactly two callers, both fed from begin(), which returns current += 1 starting at 0, so a 0 token cannot exist. It closes a pure contract gap.
Worth noting so nobody mistakes the coverage: noTokenIsCurrentBeforeFirstBegin pins an unreachable branch, not a real path.
T-1900's suggested fix says to validate “immediately before every state/cache commit”. The branch guards state commits only; imageCache.set and snapshotCache.set still run on the superseded path.
This is the right call and should be stated as one. Keys are derived from resolved identity plus colorScheme/displayScale, so a superseded load can only write its own key — harmless, and a free cache warm the newer load may hit. Guarding them would discard correct work. No note records the divergence, so a future reader comparing branch to ticket will read it as an omission.
Smallest diff that touches neither control flow nor the two views' differing LoadState payloads. The cost is that “exactly one write” is per-call-site discipline across ~13 sites rather than a structural property — which is the same class of invariant that failed in the first place.
Services/ already holds pure dependency-free helpers (LevenshteinDistance, ICUWhitespace, MonospaceMetrics, CodeFenceHelper, PrismURL), and Models/ is clearly document-domain. There is no prism/Concurrency/. Services/ is the least-bad existing home — no change needed.
Checked against the last eight merges to main: three shipped a specs/bugfixes/ folder, five did not. There is no firm convention, so its absence is not a finding — noted only so it is not raised again.
| Severity | Area | Finding | Resolution |
|---|---|---|---|
| major | WebImageDetailSheet.swift:155-161, ImageDetailWindow.swift:418-424 | The cancellation half of T-1900's own suggested fix ('treat CancellationError as a silent return') was not implemented. SVGRenderer is @MainActor and its cancellation window is a MainActor Task.sleep (SVGRenderer.swift:64-65), so a cancelled reload's CancellationError reaches commitLoadState with no actor hop. If SwiftUI's .task(id:) cancels before it creates, the stale task's token is still current, the guard passes, and a failure state is committed over the newer result — the reported symptom, on the SVG path the ticket is named after. The internal _TaskValueModifier body is not in the .swiftinterface, so the ordering cannot be read from source: this is unknown, not benign. | Recommended (not applied — this review is editorial): add `guard !Task.isCancelled else { return }` at the top of commitLoadState in both views, or `catch is CancellationError { return }` per chain. Cancellation is definitionally true for the loser regardless of scheduling, so this closes the residual by construction. It is the house pattern — RemoteRefreshFlow.swift:117/121/127/134 and RemoteContentCoordinator.swift:89/97/100 both combine it with their own generation counters. |
| major | prismTests/LoadGenerationTests.swift | No test observes WebImageDetailSheet or ImageDetailWindow. The suite covers the LoadGeneration primitive and a LoadCommitHarness stand-in that mirrors the views' logic. A future edit writing `loadState = .failed(…)` directly restores T-1900 with the suite green — the failure class CLAUDE.md records for T-1943, and the harness being a copy means it stays green while the original drifts. | Recommended: a ProductionSourceScan.Rule banning a bare `loadState = ` outside commitLoadState in those two files (~15 lines, reusing the machinery already shared by ImageMaterializationChokepointTests and URLChokepointAdoptionTests). Largely subsumed if the load functions are refactored to return a LoadState, leaving one write site. |
| minor | prism/Services/LoadGeneration.swift:11-16 | The doc comment asserts the cancelled task 'does not observe that cancellation — it keeps running', and that 'Task.checkCancellation() does not close this'. Both are false for these call paths: SVGRenderer.swift:65 calls Task.checkCancellation(), ImageLoader.swift:40 uses withTaskCancellationHandler, and ImageLoader.swift:317 / SVGSourceLoader.swift:164 both map CancellationError to .networkError("Cancelled"). It also contradicts DocumentScrollContent.swift:279, which uses the dismissed idiom with a documented T-1744 rationale. | Recommended: rewrite to say the counter is chosen because it does not depend on SwiftUI's cancel/create ordering and covers the never-cancelled slow-load half, and that the cancellation check is complementary rather than superseded. As written the comment forecloses the fix above. |
| minor | CHANGELOG.md | The entry claims a cancelled reload 'can no longer overwrite the current image with a stale one or a "Cancelled" error'. The stale-image half holds. The 'Cancelled' error half is order-dependent on the SVG-render path, and what the user would actually see there is not 'Cancelled' but the unlocalised bridge string "The operation couldn't be completed. (Swift.CancellationError error 1.)" — the SVG catch uses error.localizedDescription, not displayMessage. The raster path yields 'Network error: Cancelled'. | Recommended: soften to the stale-image guarantee, or land the cancellation guard and keep the claim as written. The latter is preferable — the claim is the right one to be able to make. |
| minor | Repo-wide: seven copies of one mechanism | LoadGeneration does not duplicate an existing reusable type — none exists — but it is the seventh hand-rolled instance of the monotonic-epoch pattern and adopts none of them: NotesManager.swift:88 (loadGeneration, T-1556), DocumentSession.swift:148 (parseGeneration, T-718), RemoteRefreshFlow.swift:71 (currentRequestID, T-1805), RemoteContentCoordinator.swift:69 (T-862), RawSourceViewModel.swift:88 (linesGeneration, T-1759), FootnotePopoverWebPage.swift:51 (renderGeneration, T-1979), plus StoreManager's begin/commit observation clock, which is the closest structural twin. The new type's name also collides with NotesManager.loadGeneration, which is a counter value rather than a counter object. (WebDocumentController's processGeneration / BridgeGeneration is a genuinely different mechanism — a multi-field tag matched across a process boundary — and should not be migrated.) | Out of scope for a bugfix. Recommended: a follow-up ticket to migrate NotesManager, DocumentSession and RemoteRefreshFlow onto LoadGeneration, or the type stays a one-off with a colliding name. |
| minor | WebImageDetailSheet.swift:177-180, ImageDetailWindow.swift:451-454 | ReloadKey and ImageReloadKey are byte-identical apart from the name — both `Hashable { colorScheme; displayScale }`. Both private and platform-exclusive so they never coexist in a build, but they are one concept. Relatedly, the two load pipelines are mirrors of each other (WebImageDetailSheet.swift:11 says so in a comment), which is why this fix had to be written twice, symmetrically. | Out of scope. Recommended follow-up: a shared ImageAppearanceKey and a shared DetailImageLoader returning Result<PlatformImage, ImageLoadFailure>, which would have made the T-1900 guard a single edit. |
| minor | WebImageDetailSheet.swift:106-169, ImageDetailWindow.swift:356-432 | token: UInt64 is threaded into loadSVG/loadSVGImage purely to reach commitLoadState, giving ~13 guarded call sites across the two views. 'Every write is guarded' is now per-call-site discipline — the same class of invariant that failed originally. | Recommended: have the load functions return a LoadState and commit once — `commitLoadState(await resolvedState(), token: token)`. Deletes the token: parameter and makes the single-write-point structural. Both functions already end in a cache-hit early return or a do/catch with a value per branch, so it is mechanical. |
| nit | prism/Services/LoadGeneration.swift:31 | begin() uses `current += 1`, which traps on overflow. Every existing counter in the repo uses wrapping `&+= 1` (DocumentSession.swift:581, NotesManager.swift:287, RemoteRefreshFlow.swift:96, RawSourceViewModel.swift:257). Unreachable at ~1.8e19 loads, but a silent divergence from house convention. | Recommended: use `&+= 1` for consistency. Note that wrapping would also make the token != 0 guard load-bearing rather than unreachable. |
| nit | prism/Services/LoadGeneration.swift:24 | `: Sendable` is redundant. The app target sets SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor, so LoadGeneration is a global-actor-isolated value type and therefore implicitly Sendable. Harmless, but it reads as a claim about cross-actor use that the type does not actually make. | Recommended: drop it, or keep it with a note that it is deliberate future-proofing. |
| nit | prismTests target vs app target isolation | prismTests does not set SWIFT_DEFAULT_ACTOR_ISOLATION (project.pbxproj:645-648, :670-673), so its default is nonisolated while LoadGeneration is @MainActor. The synchronous nonisolated test bodies at LoadGenerationTests.swift:18-56 therefore call a @MainActor type across isolation. Silent under -swift-version 5; a warning under Swift 6. Not a race — the value type is copied per test — but latent migration debt, matching the target-asymmetry trap already in project memory. The LoadCommitHarness test is correctly built (@MainActor class driven from async tests). | No action needed on this branch. Worth knowing when the project moves to Swift 6 language mode; the whole test target will need the same treatment, and this file is not special. |
| nit | ImageDetailWindow.swift:349 | Pre-existing, not introduced here: the comment says 'same resolution pipeline as ImageBlockView', but ImageBlockView.swift no longer exists — it was removed in the T-1542 WebKit cutover. CLAUDE.md's Source Structure still lists it, along with MermaidPreviewCard.swift. | Recommended as a drive-by only if the file is being touched anyway. The CLAUDE.md Source Structure drift is a separate, larger cleanup. |
| nit | WebImageDetailSheet.swift:106, ImageDetailWindow.swift:356 | Each begin() mutates a @State struct, so every load invalidates the view and costs one extra body evaluation. Verified this cannot loop: neither ReloadKey nor ImageReloadKey reads loadGeneration or loadState, so the recomputed key is identical and .task(id:) does not restart. | No action. Any counter held in @State has the same cost. Holding it on a reference type — as DocumentSession.isCurrentSaveAttempt (T-2213) and RemoteRefreshFlow.currentRequestID already do — would avoid it, but that is a style choice, not a fix. |
Click to expand.
diff --git a/prism/Services/LoadGeneration.swift b/prism/Services/LoadGeneration.swiftnew file mode 100644index 00000000..0a05ea88--- /dev/null+++ b/prism/Services/LoadGeneration.swift@@ -0,0 +1,42 @@+//+// LoadGeneration.swift+// prism+//++import Foundation++/// Guards an async load's state write against being superseded by a newer load.+///+/// SwiftUI's `.task(id:)` cancels the in-flight task when its id changes and starts a+/// replacement, but a detached `await` inside the cancelled task does not observe that+/// cancellation — it keeps running and can still resume and write state after the+/// replacement task's own result has already been committed (T-1900). `Task.checkCancellation()`+/// does not close this: the two tasks are still racing to write the same state, and checking+/// cancellation only catches the case where the runtime happened to notice — it does nothing+/// for a slow remote fetch or render that never revisits a suspension point until after a+/// faster, newer load has already resolved.+///+/// `begin()` hands out a token for one load attempt, superseding any attempt already in+/// flight. `isCurrent(_:)` — called immediately before every state write that follows an+/// `await` — answers whether that token is still the most recent one. A stale token means a+/// newer load has since started (or already finished); its result must be discarded, not+/// written, cancellation or no cancellation.+struct LoadGeneration: Sendable {+ private var current: UInt64 = 0++ /// Starts a new load attempt, superseding any attempt already in flight, and+ /// returns the token that identifies it.+ mutating func begin() -> UInt64 {+ current += 1+ return current+ }++ /// True when `token` is still the most recently started attempt.+ ///+ /// `token != 0` excludes the pre-`begin()` state: a fresh generation's `current`+ /// is `0`, which is otherwise indistinguishable from a token — without this guard+ /// `isCurrent(0)` reads as current before any load has ever started.+ func isCurrent(_ token: UInt64) -> Bool {+ token != 0 && token == current+ }+}diff --git a/prism/Views/WebImageDetailSheet.swift b/prism/Views/WebImageDetailSheet.swiftindex 2b70f455..f9d0577d 100644--- a/prism/Views/WebImageDetailSheet.swift+++ b/prism/Views/WebImageDetailSheet.swift@@ -37,6 +37,8 @@ struct WebImageDetailSheet: View { @Environment(\.dismiss) private var dismiss @State private var loadState: LoadState = .loading+ /// Guards `loadState` writes against a reload superseded by a newer one (T-1900).+ @State private var loadGeneration = LoadGeneration() @State private var showFolderPicker = false enum LoadState {@@ -98,6 +100,10 @@ struct WebImageDetailSheet: View { } private func load() async {+ // Captured before any await: a reload started while this one is still in+ // flight bumps the generation, so every commit below is checked against+ // the token taken here (T-1900).+ let token = loadGeneration.begin() let resolved = ImagePathResolver.resolve( source: request.source, baseURL: imageBaseURL,@@ -105,12 +111,12 @@ struct WebImageDetailSheet: View { ) if ImagePathResolver.isSVG(source: request.source, resolved: resolved) {- await loadSVG(resolved: resolved)+ await loadSVG(resolved: resolved, token: token) return } if let key = resolved.cacheKey, let cached = imageServices.imageCache.get(key) {- loadState = .loaded(cached)+ commitLoadState(.loaded(cached), token: token) return } @@ -119,20 +125,20 @@ struct WebImageDetailSheet: View { if let key = resolved.cacheKey { imageServices.imageCache.set(key, image: image) }- loadState = .loaded(image)+ commitLoadState(.loaded(image), token: token) } catch {- loadState = .failed(message: error.displayMessage, canGrantAccess: isAccessError(error))+ commitLoadState(.failed(message: error.displayMessage, canGrantAccess: isAccessError(error)), token: token) } } - private func loadSVG(resolved: ResolvedImageSource) async {+ private func loadSVG(resolved: ResolvedImageSource, token: UInt64) async { // Keyed on the resolved absolute identity, not the raw markdown-relative // path string, so two documents that both reference e.g. "./diagram.svg" // don't collide on one cache entry (T-1866). Nil when resolution failed // (no stable identity) — caching is skipped in that case. let svgKey = SnapshotCache.svgKey(for: resolved, colorScheme: colorScheme, displayScale: displayScale) if let svgKey, let cached = imageServices.snapshotCache.get(svgKey) {- loadState = .loaded(cached)+ commitLoadState(.loaded(cached), token: token) return } do {@@ -145,16 +151,23 @@ struct WebImageDetailSheet: View { if let svgKey { imageServices.snapshotCache.set(svgKey, image: snapshot) }- loadState = .loaded(snapshot)+ commitLoadState(.loaded(snapshot), token: token) } catch let error as ImageLoadError {- loadState = .failed(message: error.displayMessage, canGrantAccess: isAccessError(error))+ commitLoadState(.failed(message: error.displayMessage, canGrantAccess: isAccessError(error)), token: token) } catch let error as SVGRenderError {- loadState = .failed(message: error.localizedDescription, canGrantAccess: false)+ commitLoadState(.failed(message: error.localizedDescription, canGrantAccess: false), token: token) } catch {- loadState = .failed(message: error.localizedDescription, canGrantAccess: false)+ commitLoadState(.failed(message: error.localizedDescription, canGrantAccess: false), token: token) } } + /// Writes `state` only if `token` is still the most recently started load (T-1900).+ /// A superseded reload silently loses the race instead of overwriting a newer result.+ private func commitLoadState(_ state: LoadState, token: UInt64) {+ guard loadGeneration.isCurrent(token) else { return }+ loadState = state+ }+ /// A sandbox/folder-access failure is the one a folder grant can fix. private func isAccessError(_ error: ImageLoadError) -> Bool { if case .sandboxRestricted = error { return true }diff --git a/prism/Views/ImageDetailWindow.swift b/prism/Views/ImageDetailWindow.swiftindex 3ca6f250..4ba028e6 100644--- a/prism/Views/ImageDetailWindow.swift+++ b/prism/Views/ImageDetailWindow.swift@@ -32,6 +32,8 @@ struct ImageDetailWindow: View { // MARK: - State @State private var loadState: ImageWindowLoadState = .loading+ /// Guards `loadState` writes against a reload superseded by a newer one (T-1900).+ @State private var loadGeneration = LoadGeneration() @State private var scale: CGFloat = 1.0 @State private var offset: CGSize = .zero @State private var lastScale: CGFloat = 1.0@@ -348,6 +350,10 @@ struct ImageDetailWindow: View { /// Loads the image using the same resolution pipeline as ImageBlockView. private func loadImage() async {+ // Captured before any await: a reload started while this one is still in+ // flight bumps the generation, so every commit below is checked against+ // the token taken here (T-1900).+ let token = loadGeneration.begin() let sourceType = DocumentSourceType.from(rawValue: data.sourceType) let resolved = ImagePathResolver.resolve( source: data.imageSource,@@ -359,13 +365,13 @@ struct ImageDetailWindow: View { // SVGs are checked first because their cache is color-scheme-aware and // must re-render on theme change (task re-triggers via id: colorScheme). if ImagePathResolver.isSVG(source: data.imageSource, resolved: resolved) {- await loadSVGImage(resolved: resolved)+ await loadSVGImage(resolved: resolved, token: token) return } // Raster images: check image cache first (theme-independent) if let key = cacheKey(for: resolved), let cached = imageServices.imageCache.get(key) {- loadState = .loaded(cached)+ commitLoadState(.loaded(cached), token: token) return } @@ -375,14 +381,14 @@ struct ImageDetailWindow: View { if let key = cacheKey(for: resolved) { imageServices.imageCache.set(key, image: image) }- loadState = .loaded(image)+ commitLoadState(.loaded(image), token: token) } catch {- loadState = .failed(error.displayMessage)+ commitLoadState(.failed(error.displayMessage), token: token) } } /// Loads an SVG image by checking the snapshot cache, then falling back to source loading + rendering.- private func loadSVGImage(resolved: ResolvedImageSource) async {+ private func loadSVGImage(resolved: ResolvedImageSource, token: UInt64) async { // Keyed on the resolved absolute identity (not the raw markdown-relative // path string), so two documents that both reference e.g. "./diagram.svg" // don't collide on one cache entry (T-1866). Display scale is included so@@ -394,7 +400,7 @@ struct ImageDetailWindow: View { // Check snapshot cache if let svgKey, let cached = imageServices.snapshotCache.get(svgKey) {- loadState = .loaded(cached)+ commitLoadState(.loaded(cached), token: token) return } @@ -408,16 +414,23 @@ struct ImageDetailWindow: View { if let svgKey { imageServices.snapshotCache.set(svgKey, image: snapshot) }- loadState = .loaded(snapshot)+ commitLoadState(.loaded(snapshot), token: token) } catch let error as ImageLoadError {- loadState = .failed(error.displayMessage)+ commitLoadState(.failed(error.displayMessage), token: token) } catch let error as SVGRenderError {- loadState = .failed(error.localizedDescription)+ commitLoadState(.failed(error.localizedDescription), token: token) } catch {- loadState = .failed(error.localizedDescription)+ commitLoadState(.failed(error.localizedDescription), token: token) } } + /// Writes `state` only if `token` is still the most recently started load (T-1900).+ /// A superseded reload silently loses the race instead of overwriting a newer result.+ private func commitLoadState(_ state: ImageWindowLoadState, token: UInt64) {+ guard loadGeneration.isCurrent(token) else { return }+ loadState = state+ }+ /// Generates a cache key from a resolved image source. private func cacheKey(for resolved: ResolvedImageSource) -> String? { resolved.cacheKeydiff --git a/prismTests/LoadGenerationTests.swift b/prismTests/LoadGenerationTests.swiftnew file mode 100644index 00000000..0c601fa8--- /dev/null+++ b/prismTests/LoadGenerationTests.swift@@ -0,0 +1,140 @@+//+// LoadGenerationTests.swift+// prismTests+//+// Regression coverage for T-1900: WebImageDetailSheet and ImageDetailWindow reload+// images via .task(id:) on colour-scheme/display-scale changes. SwiftUI cancels the+// superseded task, but a detached await inside it ignores that cancellation and can+// still resume and overwrite a newer, already-committed loadState. LoadGeneration is+// the pure token/counter that both views now check immediately before every state+// write, so a stale load loses the race regardless of arrival order.+//++import Foundation+import Testing+@testable import prism++@Suite("LoadGeneration")+struct LoadGenerationTests {+ @Test("first begin() token is current")+ func firstTokenIsCurrent() {+ var generation = LoadGeneration()+ let token = generation.begin()+ #expect(generation.isCurrent(token))+ }++ @Test("a later begin() supersedes an earlier token")+ func laterBeginSupersedesEarlierToken() {+ var generation = LoadGeneration()+ let first = generation.begin()+ let second = generation.begin()++ #expect(!generation.isCurrent(first))+ #expect(generation.isCurrent(second))+ }++ @Test("tokens from repeated begin() calls are all distinct")+ func repeatedBeginProducesDistinctTokens() {+ var generation = LoadGeneration()+ let tokens = (0..<5).map { _ in generation.begin() }++ #expect(Set(tokens).count == tokens.count)+ // Only the last one issued is current.+ for token in tokens.dropLast() {+ #expect(!generation.isCurrent(token))+ }+ #expect(generation.isCurrent(tokens.last!))+ }++ @Test("a fresh generation has no current token before begin() is called")+ func noTokenIsCurrentBeforeFirstBegin() {+ let generation = LoadGeneration()+ // Token 0 never gets handed out (begin() starts counting at 1), so a+ // caller cannot accidentally pass an "unstarted" token and have it read+ // as current.+ #expect(!generation.isCurrent(0))+ }++ /// A small stand-in for the MainActor state a view owns in production:+ /// `generation` is mutated in place by every `beginLoad()`, and `commit`+ /// re-reads it — not a snapshot taken before the race — at write time,+ /// mirroring `ImageDetailWindow`/`WebImageDetailSheet`'s `loadGeneration`/+ /// `loadState` `@State` pair and their `commitLoadState(_:token:)` guard.+ @MainActor+ private final class LoadCommitHarness {+ private(set) var generation = LoadGeneration()+ private(set) var loadState: String?++ func beginLoad() -> UInt64 {+ generation.begin()+ }++ func commit(token: UInt64, state: String) {+ guard generation.isCurrent(token) else { return }+ loadState = state+ }+ }++ /// A one-shot rendezvous that forces a specific interleaving without any+ /// timing assumption: the stale load suspends in `wait()` until the test+ /// explicitly calls `signal()`, and `waitForReady()` lets the test block+ /// until the stale load has actually reached that suspension point. No+ /// `Task.sleep`, no reliance on scheduler ordering (T-2017/T-2095).+ private actor Gate {+ private var continuation: CheckedContinuation<Void, Never>?+ private var readyContinuation: CheckedContinuation<Void, Never>?+ private var isReady = false++ func waitForReady() async {+ if isReady { return }+ await withCheckedContinuation { readyContinuation = $0 }+ }++ func wait() async {+ await withCheckedContinuation { continuation in+ self.continuation = continuation+ isReady = true+ readyContinuation?.resume()+ readyContinuation = nil+ }+ }++ func signal() {+ continuation?.resume()+ continuation = nil+ }+ }++ /// End-to-end race against the *live* generation counter, not a frozen copy —+ /// exactly what `commitLoadState` re-reads in production. The stale load+ /// begins first (capturing the older token) and then suspends; only while+ /// it's suspended does the current load begin and commit; only then does the+ /// stale load resume and attempt its own commit. If a stale write could still+ /// win, `loadState` would end up "stale" instead of "current".+ @Test("a superseded load cannot win the commit race")+ func supersededLoadCannotWinCommitRace() async {+ let harness = await LoadCommitHarness()+ let gate = Gate()++ let staleLoad = Task {+ let token = await harness.beginLoad()+ await gate.wait()+ await harness.commit(token: token, state: "stale")+ }++ // Block until the stale load has captured its token and suspended —+ // without this, the current load below could race ahead of the stale+ // load's beginLoad() and the ordering the test relies on wouldn't be+ // pinned at all.+ await gate.waitForReady()++ let currentToken = await harness.beginLoad()+ await harness.commit(token: currentToken, state: "current")++ await gate.signal()+ await staleLoad.value++ let finalState = await harness.loadState+ #expect(finalState == "current")+ }+}diff --git a/CHANGELOG.md b/CHANGELOG.mdindex bb517458..b0d42411 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- A cancelled reload of the fullscreen image viewer (iOS) or image detail window (macOS) can no longer overwrite the current image with a stale one or a "Cancelled" error after a rapid appearance or display-scale change (T-1900). - An image that is tiny as a file but enormous as a picture can no longer exhaust memory or terminate Prism, whether it comes from the web, from a file beside the document, or written directly into the markdown (T-2132, T-2149, T-2151, T-1867). A picture is stored compressed, and a plain-coloured one compresses at about a thousand to one — so a 400 KB download can be a 20,000 by 20,000 image that needs about 400 MB the moment anything tries to display it, and four times that if it is in colour. Prism's limits were all written on the wrong side of that: a 50 MB cap on the download said nothing about the picture inside it, and the 2 MB cap on a local SVG was applied only after the whole file had already been read, so a very large one could freeze the app on its way to being refused. The limits that did exist covered only images fetched from the web; the same image referenced from a file next to your document, or embedded inline in the markdown, went straight to the renderer unchecked. Prism now reads the picture's dimensions from its header — a few bytes, before anything is decoded — and decides from that. An ordinary image is displayed as before. A very large one referenced from the web or from a file is scaled down to fit. One beyond any reasonable size is refused outright and shows the usual "Image failed to load" placeholder, rather than being handed to a decoder that would have to build the whole thing first. How large a picture is now also accounts for how much detail each dot of it carries: most pictures store one byte per colour, but some store two or four, and Prism previously assumed the smaller size for all of them and so under-counted the deep ones by half or three quarters. One consequence you may see: a very large deep-colour photograph that used to display at full size is now scaled down, because its true size was always above the limit and is now measured as such. Files are now read up to their limit instead of read whole and then measured — including the copy Prism keeps of a document you have not saved yet, which is restored when the app reopens. How much decoding happens at once is limited by how much memory those pictures actually need rather than by how many of them there are, so a page full of large images no longer overruns while appearing to stay within its bounds. Two things behave differently, both deliberately. An image whose file does not say how big it is, or what kind of dots it stores, now shows the "Image failed to load" placeholder instead of being displayed — there is no way to know what it would cost until it has already cost it. And an image written directly into the markdown is treated more strictly than the same image kept in a file beside the document: it is either small enough to display as it is or refused, never scaled down. That difference is about memory rather than effort. Scaling a picture that is written into the markdown means rebuilding it and writing the smaller version back into the page, where it then stays for as long as the document is open — which costs more memory, for longer, than not showing it. A picture in a file has somewhere else to keep its smaller version, so it can be scaled instead of refused. Animated images are unaffected in either case: they play as before, however many frames they have. - A verification scan that starts during the app's initial entitlement bootstrap can no longer publish a stale result while a newer scan is still in flight (T-2152). While `entitlementState` was still `.loading`, any scan's result was accepted regardless of whether a more recent scan — for example one started right after `AppStore.sync()` — was still reading the world; the older scan finishing first could briefly flip the paywall to locked (or unlocked) ahead of the newer, more current answer. An older result that arrives while a newer scan is still outstanding is now held back rather than published. If the newer scan goes on to answer, its fresher result is published and the held-back one is simply dropped; if instead it is cancelled without ever answering, the held-back result is released, so a cancelled scan cannot leave the paywall stranded on `.loading`. The trade is that the brief loading state now ends when the last overlapping scan answers rather than the first, so it can last marginally longer; every control it gates is disabled meanwhile, so nothing silently does nothing. - Saving a pasted document to a file no longer disturbs whatever document you opened next (T-2213). A save finishes in two parts: the file is written straight away, but the document only becomes that file once its notes have been moved across, and on a slow iCloud connection that second part can still be running after you have closed the document or opened another one. When it finished late, it acted on the document then on screen instead of the one it had saved: the pasted text of that other document was deleted from the place Prism keeps unsaved documents — so it could no longer be recovered after a relaunch — its entry in Recent Files was labelled with the wrong document's title, and an action you had queued behind its own Save prompt could run without you confirming it. A save that failed to move its notes also raised an alert naming a file you were no longer looking at. Each of these now belongs to the document that was actually saved, and the document on screen is left alone. Its Recent Files entry is labelled with its own title rather than the other document's. Where that other document had itself started saving in the meantime, the late save no longer takes over the shortcut that document had prepared for its own file, which can leave the saved file without a Recent Files entry of its own. The file is saved either way, and can be opened from the Files app.grep -n 'loadState' prism/Views/WebImageDetailSheet.swift prism/Views/ImageDetailWindow.swift returns exactly one assignment per file (:168 and :431), both inside commitLoadState. Every other hit is the declaration, a doc comment, or a read (switch loadState, if case .loaded, guard case .loaded). ImageWindowLoadState appears nowhere else in the codebase. No unguarded write survives.
let token = loadGeneration.begin() is the first statement of both load functions (WebImageDetailSheet.swift:106, ImageDetailWindow.swift:356), ahead of ImagePathResolver.resolve and every await. MainActor isolation is established two independent ways: the app target's SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor, and SwiftUI's .task(id:) taking an @_inheritActorContext sending @isolated(any) closure formed in @MainActor body.
When the sheet or window goes away, .task is cancelled with no replacement, so nothing bumps the generation. The token stays current, isCurrent returns true, and the failure is committed — identical to pre-branch behaviour, and unobservable because the view is gone.
The begin()/commit pairing is total: every return path either commits or tail-calls a function that commits on all five branches (cache hit, success, ImageLoadError, SVGRenderError, generic). The newest token is current by definition, so the newest task's commit is never dropped. Re-presentation gets fresh @State on both platforms — iOS via .sheet(item:) (MediaZoomPresenter.swift:46), macOS via WindowGroup(for:) (prismApp.swift:218-220).
Cache writes sit before the guard, so a superseded load still populates imageCache/snapshotCache. That is benign and mildly useful: keys are derived from resolved identity plus colorScheme/displayScale, so a superseded load writes only its own key, never the current load's, and the newer load may hit what it warmed.
The one scenario where an unguarded cache write bites — bytes changing at the same URL after a document reload — is exactly T-2142, is pre-existing, and is unchanged by this branch. Guarding these writes would not help it and would lose the cache warm. Land T-2142 independently; nothing here constrains it.
Whether SwiftUI's _TaskValueModifier cancels-then-creates or creates-then-cancels on a .task(id:) re-key. The public .task(id:) is @_alwaysEmitIntoClient and readable, but it forwards to @usableFromInline internal modifiers with no inlinable body in the .swiftinterface.
Finding 1's severity rests on that unknown. If SwiftUI creates first, the guard already wins and the residual is theoretical. If it cancels first, the residual is real on the SVG-render path. The one-line cancellation guard makes the question moot, which is the argument for adding it rather than for investigating further.
make lint — 0 violations, 0 serious, 557 files.make verify-test-isolation — OK; 43 self-tests pass.make build-macos, make build-ios — both exit 0. A dedicated warning grep over the macOS app-target build produced nothing.xcodebuild ... -only-testing:prismTests/LoadGenerationTests on the real test host — 5/5 passed, repeated across four parallel workers.build-for-testing warnings are all pre-existing macro-expansion diagnostics across unrelated suites; none names LoadGeneration.