Fix CacheCallbackCoordinator race and out-of-order cache actions

[devzahirul]

What

Fixes a race and an intermittent assertion crash in CacheCallbackCoordinator — the cache-completion coordinator used by KingfisherManager when storing image + original image.

Symptom

A flaky CI crash (Debug-only assertionFailure):

Fatal error: This case should not happen in CacheCallbackCoordinator:
originalImageCached - cacheInitiated   (KingfisherManager.swift)

Two root causes

1. Non-atomic transition. apply() read state (in the switch tuple) and wrote it back via separate stateQueue.sync calls:

func apply(_ action: Action, trigger: () -> Void) {
    switch (state, action) {                 // sync read
    …
    case (.imageCached, .cachingOriginalImage):
        state = .done                        // sync write — different critical section
        trigger()
    …
    }
}

Two apply calls (the .cachingImage and .cachingOriginalImage store callbacks run on different threads) can interleave between the read and the write — corrupting the state machine and either firing trigger twice (double completion) or never (a stuck waitForCache completion).

2. Out-of-order actions. The three actions are produced from different queues:

targetCache.store(…)      { coordinator.apply(.cachingImage)         { … } }  // async callback
originalCache.storeToDisk { coordinator.apply(.cachingOriginalImage) { … } }  // async callback
coordinator.apply(.cacheInitiated) { … }                                      // synchronous

If a store finishes before the synchronous .cacheInitiated is applied, .cacheInitiated arrives in .imageCached / .originalImageCached instead of .idle. The state machine had no case for that and hit default → assertionFailure (the CI crash). It's intermittent because it depends on store-vs-cacheInitiated timing.

Fix

• Resolve the entire transition inside one stateQueue.sync, returning whether to fire, and call trigger() after the queue is released (so user code never runs while the queue is held). This makes each transition atomic and guarantees trigger fires exactly once.
• Handle (.imageCached, .cacheInitiated) / (.originalImageCached, .cacheInitiated) identically to (.idle, .cacheInitiated) instead of trapping. The default → assertionFailure is kept as a safety net for genuinely-unreachable duplicate actions.

No public API or behavior change for the in-order path; the completion semantics are unchanged.

Tests (KingfisherManagerTests)

• testCacheCoordinatorToleratesLateCacheInitiated — deterministic reproduction of the CI crash (.cachingOriginalImage / .cachingImage then .cacheInitiated).
• testCacheCoordinatorTriggersExactlyOnceForEveryOrdering — all 6 action orderings × waitForCache, completion must fire exactly once.
• testCacheCoordinatorConcurrentActionsTriggerOnce — applies the actions concurrently 1000×, asserts exactly one trigger and stays clean under TSan.

Verification (macOS, Thread Sanitizer)

xcodebuild test -project Kingfisher.xcodeproj -scheme Kingfisher \
  -destination 'platform=macOS' \
  -only-testing:KingfisherTests/KingfisherManagerTests/testCacheCoordinatorToleratesLateCacheInitiated \
  -only-testing:KingfisherTests/KingfisherManagerTests/testCacheCoordinatorTriggersExactlyOnceForEveryOrdering \
  -only-testing:KingfisherTests/KingfisherManagerTests/testCacheCoordinatorConcurrentActionsTriggerOnce \
  -enableThreadSanitizer YES

• Before the fix: testCacheCoordinatorToleratesLateCacheInitiated crashes with Fatal error: … originalImageCached - cacheInitiated → ** TEST FAILED ** (the exact CI failure).
• After the fix: all three pass, no sanitizer report → ** TEST SUCCEEDED **.

Why it matters

This is the intermittent failure behind the "originalImageCached - cacheInitiated" crash that flakes CI. Beyond the Debug assertion, the non-atomic transition can drop a waitForCache completion in Release builds — a silently never-firing callback — which is a plausible contributor to hard-to-reproduce hang reports.

CI

The full test + build matrix passes on my fork across iOS, macOS, tvOS, and watchOS (Xcode 26.2 and 26.5).

[onevpaw]

Reviewed the coordinator transitions against the production cache paths. The serialized state transition and tolerance for a late .cacheInitiated cover the reachable synchronous cache-callback ordering while preserving single completion and avoiding callback reentrancy under the lock.

One non-blocking follow-up: parameterizing the new ordering/concurrency tests for shouldCacheOriginal: false, with assertions that waitForCache does not complete early, would strengthen regression coverage.

— an assistant to @onevcat

[onevclaw]

As onevcat's assistant, I reviewed 8cbf74148ce17d66d7f370f17d17946c8425ea39.

The state transition is now serialized and completion runs outside the critical section, which addresses the reachable callback-before-.cacheInitiated ordering without introducing a reentrancy issue.

Non-blocking test coverage suggestion: please consider adding the callback-before-.cacheInitiated permutations for shouldCacheOriginal == false, for both waitForCache values, to protect that production path as well.

I also found that swift test -q cannot discover tests because the package manifest has no test target. This appears outside this PR's diff, but should be resolved or explicitly treated as a separate repository-level issue. Visible CI is still pending, so this is a comment rather than an approval.

onevclaw - an assistant to @onevcat