Stop the nav-data decode from blocking the main thread - #15
Merged
Conversation
Fixes SF50-TOLD-26: "App Hang Fully Blocked", 184 events / 43 users, still firing on 3.6.1+49 with a unimodal duration histogram (52 of 58 events between 2.4s and 5.7s). This is executor contention, not store-lock contention. The seven prior fixes (c15f8ca, 5093289, d229f61, d13ad53, 908e298, 40c0270, ff37690) all addressed the main context waiting on the persistent store coordinator while the importer committed, and ff37690's separate import container removed that class of stall entirely. The remaining hang reaches the same performBlockAndWait frame through task *scheduling* rather than a fetch: a @Modelactor's serial executor is its NSManagedObjectContext's dispatch queue, and SwiftData enqueues jobs onto it with performBlockAndWait, so enqueueing onto a busy @Modelactor blocks the enqueueing thread. No store lock is involved. decompress(data:) was a synchronous actor method doing an LZMA inflate plus a whole-dataset PropertyListDecoder.decode, owning that queue for seconds without suspending, while progressTask polled `await loader.state` every 0.25s from the MainActor. Each poll blocked the main thread until the decode finished. - Move the inflate and decode into a nonisolated @Concurrent static function with a local PropertyListDecoder, so the actor suspends and its executor stays free. The stored `decoder` property is gone with its only caller. - Push load progress through an AsyncStream the loader yields into, instead of polling the actor, following the NOTAM precedent from d13ad53/908e298. Yielding is nonblocking, so no MainActor -> @Modelactor hop occurs for the duration of the import. The stream carries the current state as its first element and finishes when load() returns, and the consumer still skips .idle so the UI never regresses to the consent screen. - Coarsen download progress to 0.5% steps. The download reports once per 8 KB chunk, which a pull-based consumer sampled at 4 Hz but a push-based one would deliver in full. - Build the import container and loader on a @Concurrent static function so opening the second store coordinator no longer does filesystem work on the main thread when the user taps Load. A failure there now returns the UI to .idle, since re-entry is blocked before the container is opened. - Document the executor invariant on NavDataLoader. Every prior fix recorded its reasoning; this rule was the unstated one, which is why a polling progressTask looked safe to add. Preserved from the prior fixes: batched saves with inter-batch pauses, the bounded batch deletes, no mainContext access from the view model, the separate import container, the re-entry guard, and the state poll that stops once data is loaded. Verified by instrumenting a real end-to-end load on an iOS 26.5 simulator (real LZMA plist from GitHub), measuring how long a MainActor enqueue onto the loader took during a ~2.6-2.8s decode: - decompress on the actor: one enqueue blocked 2.686905 s (decode 2.61 s) - decompress @Concurrent: 28 consecutive samples over the 2.75 s decode all 11-31 us; worst 52 ms, and that sample landed after the decode, during a bounded batch delete Also verified: swift-format and swiftlint clean on both files; app builds warning-free; SF50 Shared Unit Tests 304/304 passing; the progress UI advances through download -> extracting -> loading with the accessibility tree responding promptly throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes SF50-TOLD-26 — "App Hang Fully Blocked", 184 events / 43 users, still firing on 3.6.1+49.
This is not the eighth attempt at the store-lock theory
Seven prior commits (
c15f8ca,5093289,d229f61,d13ad53,908e298,40c0270,ff37690) fixed persistent-store-lock contention: the main context blocking inperformBlockAndWaitwhile waiting for the store coordinator that the importer held mid-commit. Those were correct, andff37690'smakeImportContainerfinished the job — with the importer on its own container/coordinator and WAL journaling, main-context reads are no longer blocked by importer writes at all.The hang that remains bottoms out in the same
performBlockAndWaitframe but arrives there by a different route:context.fetch→performBlockAndWaitswift_task_enqueueOnExecutor→performBlockAndWaitThe mechanism: a
@ModelActor's serial executor is itsNSManagedObjectContext's dispatch queue, and SwiftData enqueues jobs onto it via-[NSManagedObjectContext performBlockAndWait:]. So enqueueing onto a busy@ModelActorblocks the enqueueing thread — no store lock required, and a separate coordinator does nothing to help.decompress(data:)was a synchronous actor method doing an LZMA inflate plus a whole-datasetPropertyListDecoder.decode, holding that queue for seconds without ever suspending. MeanwhileprogressTaskpolledawait loader.stateevery 0.25 s from the MainActor (a bareTask {}in a@MainActormethod inherits MainActor isolation). Every poll blocked the main thread until the decode finished — which matches the histogram exactly: unimodal, 52 of 58 events between 2.4 s and 5.7 s, i.e. the decode duration, not a lock-wait distribution.Changes
decompress(data:)becomes anonisolated@concurrentstaticfunction with a localPropertyListDecoder,awaited so the actor genuinely suspends. It touched nomodelContext, andAirportDataCodableis alreadyCodable, Sendable, so the result crosses isolation cleanly. The storeddecoderproperty went with its only caller.Stateinto anAsyncStream(bufferingNewest(1)) instead of callers polling it, following the NOTAM precedent fromd13ad53/ Observe NOTAM changes instead of polling on the main actor #11 and908e298. Yielding is nonblocking, so there is no MainActor →@ModelActorhop for the duration of the import. The stream's first element is the current state and it finishes whenload()returns or throws; the consumer still skips.idleso the UI never regresses to the consent screen.makeImportLoader(matching:)is a@concurrentstatic function, so opening the second store coordinator no longer does filesystem work on the main thread at the moment the user taps Load. TheSentrySDK.capturetag/fingerprint and the in-memory-store special case for UI tests are unchanged.## Executor Constraintssection onNavDataLoader. Every prior fix documented its reasoning in a comment; this rule was the unstated one, which is exactly why adding a pollingprogressTasklooked safe.Preserved from the prior fixes: batched saves with inter-batch pauses, the bounded batch deletes, no
mainContextaccess from the view model, the separate import container, theload()re-entry guard, and the launch-time state poll that stops once data is loaded.Verified
Instrumented a real end-to-end load on an iOS 26.5 simulator (iPhone 17 Pro), pulling the real LZMA plist from GitHub, with a temporary probe measuring how long a MainActor enqueue onto the loader took. The instrumentation is not in this commit; the only variable between the two runs was
decompress's isolation.decompressnonisolated@concurrent(this PR)The one outlier in the fixed run was 52 ms, and that sample landed after the decode returned, during a bounded batch delete — the shape
40c0270intentionally produced.Also verified:
swift format lint --strictandswiftlint --strictclean on both changed files (the pre-existingswitch_case_alignmentconfig warning is unrelated noise).SF50 TOLDscheme builds for an iOS 26 simulator with zero warnings.SF50 Shared Unit Tests(the plan CI runs): 304 tests, 0 failures, 0 skipped, 56.3 s.Not verified / reviewer's attention
writeCycles,Defaults[.schemaVersion], loader dismissal) are unchanged in behavior but were not observed finishing.NavDataLoaderlives in the app target, which has no unit-test target attached to any scheme —SF50 TOLDTests/is empty with an orphaned test plan, andSF50 Shared Unit Testscannot import the app target. There is also no honest unit test for "an enqueue onto this actor does not block," which is why this was measured against the real import instead.progressReportingStep). The download reports once per 8 KB chunk — the old 4 Hz poll discarded most of those, but a push consumer would deliver every one, waking the MainActor a few hundred times a second to move a ring by a pixel. 0.5% keeps the ring smooth at ~14 updates/s on the observed download. Import-phase progress is left alone, since the batch cadence already bounds it at ~20/s.importContainerfailure now resetsstateto.idle. It has to, because re-entry is blocked before the container is opened — otherwise a failure would strand the user on the progress screen with the Load button unreachable. Previously the state was never advanced in that path.🤖 Generated with Claude Code