[Android] Map: Stop reloading a pin's icon on every cluster pass

[kevin68]

Fixes #37773

Description of Change

ReclusterPins re-runs AddPins on every zoom change, and AddPinAsync reloaded Pin.ImageSource unconditionally each time — a Glide fetch plus a decode and rescale for every un-clustered pin, on every pass. The MarkerOptions it writes the icon to already outlive the markers built from them, so the work was redundant as soon as the source had not changed.

MapPinHandler now records which image source produced the icon currently on its MarkerOptions, and AddPinAsync skips the load while it still matches.

The key comes from GetClusterIconCacheKey, the helper the cluster icon cache already uses, rather than a reference comparison. That matters twice:

• it compares by value, so a new but equivalent ImageSource instance is recognised instead of forcing a reload;
• it returns null for a source that opted out of caching — a UriImageSource with CachingEnabled false, or a non-positive CacheValidity — and those always reload rather than being frozen for the handler's lifetime. The helper's own comment states that rule for the cluster cache; this reuses it rather than restating it.

The record is invalidated by the ImageSource mapper, which is the one hook that always runs when the icon on the MarkerOptions goes stale — on every SetVirtualView, including a handler reconnected to a different pin, and on every ImageSource change.

The source is captured before the await and re-checked after it before the icon and the key are written: UpdatePinImageSourceAsync guards its own write the same way, and without the symmetric guard the two writers race and a marker can be published with the older icon while the change notification that would correct it has already been consumed. UpdatePinImageSourceAsync writes the icon to the MarkerOptions as well as the live marker and updates the key, reaching the platform element through the non-throwing accessor since it resumes after an await.

A cleared ImageSource now takes the previous icon off the reused MarkerOptions (SetIcon(null), Android's "use the default marker") rather than leaving the pin rendering an image its source no longer names.

Skipping the load also removes the await before Map.AddMarker on the common path, so an un-clustered pin with an unchanged icon is added synchronously instead of disappearing until its image resolves.

Note on ordering

Split out of #37769 so the bug fix and this caching behaviour stay separately reviewable. It applies independently, but #37769 is the one that fixes broken behaviour — merge that first.

Verification

Physical device (Samsung SM-X230, Android 15), clustering gallery using pins whose icon is a PNG written to FileSystem.CacheDirectory, handler instrumented at each boundary. Three zoom-in/zoom-out cycles over 25 clustered custom pins, merged with #37769: 19 image loads for 86 markers instead of one load per marker, every de-clustered pin keeping its icon, no Glide asserts, no recycled-bitmap errors, no crashes. Re-measured after adding the mapper-based invalidation, to confirm it does not fire on the recluster path — ReclusterPins reuses the existing handler with an unchanged VirtualView, so the mapper does not re-run there.

Issues Fixed

Fixes #37773

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37774

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37774"

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Hey there @@kevin68! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[dotnet-policy-service]

Hey there @kevin68! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[MauiBot]

Expert Review — 6 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness — AddPinAsync writes both the icon and the new AppliedImageSourceKey after the await without re-checking that pin.ImageSource is still requestedSource. UpdatePinImageSourceAsync (line 972) deliberately guards the symmetric write with ReferenceEquals(pin.ImageSource, requestedSource); this path does not, so the two writers can land out of order.

Concrete scenario (clustering enabled, pin has a live marker from the previous pass):

1. Recluster triggers AddPins → AddPinAsync(pin) captures requestedSource = A and awaits LoadPinIconAsync(A).
2. App sets pin.ImageSource = B. PinOnPropertyChanged finds the still-live marker and starts UpdatePinImageSourceAsync, which completes first and applies B to marker, to mapPinHandler.PlatformView and sets AppliedImageSourceKey = keyB.
3. The A continuation resumes and unconditionally overwrites markerOptions.SetIcon(A) and AppliedImageSourceKey = keyA.
4. Map.AddMarker(markerOptions) (line 938) then publishes a marker rendering A while pin.ImageSource is B, and nothing re-runs until the next zoom/recluster — the ImageSource change notification has already been consumed.

This also breaks the invariant the new code documents ("the key recorded below has to name the source the icon actually came from"): the key names A, but the last intended source is B. Suggested fix: after the await, bail out (or skip the SetIcon/key write) when !ReferenceEquals(pin.ImageSource, requestedSource), matching the guard already used in UpdatePinImageSourceAsync.

[kevin68]

Fixed — this was real, and the asymmetry with UpdatePinImageSourceAsync was the tell. Your step 4 is the part that makes it stick: the marker is published rendering A while the change notification that would have corrected it has already been consumed, so nothing re-runs until the next recluster.

AddPinAsync now re-checks after the await, matching the existing guard:

if (icon != null && ReferenceEquals(pin.ImageSource, requestedSource))
{
    markerOptions.SetIcon(icon);
    if (mapPinHandler is not null)
        mapPinHandler.AppliedImageSourceKey = requestedKey;
}

A stale continuation now drops both writes rather than half of them, so the key keeps naming the source whose icon is actually on the MarkerOptions.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — The requestedSource is not null gate skips the whole block, so a cleared ImageSource never clears the icon already set on the reused MarkerOptions, and AppliedImageSourceKey is left pointing at the removed source.

Concrete scenario (clustering enabled): pin P was standalone with ImageSource = A, so markerOptions holds icon A and AppliedImageSourceKey = "file:a.png". User zooms out and P becomes part of a multi-pin cluster — no marker exists for P, so PinOnPropertyChanged returns early at the marker is null check (line 811) and a subsequent pin.ImageSource = null is dropped. MapPinHandler.MapImageSource is a no-op, so nothing else observes the change. Zoom back in: AddRegularPin → AddPinAsync with requestedSource == null skips everything, and Map.AddMarker(markerOptions) recreates the marker still rendering icon A even though ImageSource is null.

The new key state makes this cheap to close: when requestedSource is null && mapPinHandler?.AppliedImageSourceKey is not null, call markerOptions.SetIcon(null) (Android's documented "use default marker" value, already relied on at line 974) and reset the key to null.

[kevin68]

Fixed. You are right that the new key state makes this cheap to close, and it is a genuine wrong-icon case rather than a cosmetic one — the pin comes back rendering an image its ImageSource no longer names.

AddPinAsync now handles the cleared source explicitly instead of skipping the block:

if (requestedSource is null)
{
    markerOptions.SetIcon(null);
    if (mapPinHandler is not null)
        mapPinHandler.AppliedImageSourceKey = null;
}
else { ... }

I set the icon unconditionally rather than gating on AppliedImageSourceKey is not null, because the key is also cleared by the ImageSource mapper now (see the MapPinHandler thread), so it is not a reliable signal that an icon is still on the reused options. SetIcon(null) is cheap and idempotent for the pins that never had one.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Handler Mapper and Property Patterns — AppliedImageSourceKey is handler-lifetime state with no invalidation hook on the handler itself. MapImageSource is a no-op, so the mapper path never resets it, and ElementHandler.SetVirtualView reuses a non-null PlatformView (ElementHandlerState.Reconnecting) when the handler is re-attached to a different IMapPin — the reused MarkerOptions keeps its icon and this key keeps describing the previous pin's source. Combined with the requestedSource is not null gate in MapHandler.AddPinAsync (line 922), a reattached pin with a null ImageSource inherits the previous pin's icon.

Invalidation currently depends entirely on MapHandler.PinOnPropertyChanged finding a live Marker, which is not the case for pins inside a cluster. Reset the key where the platform element's icon state is (re)established — e.g. clear it in DisconnectHandler/on reconnect, or make MapImageSource invalidate it — so the cache key can never outlive the MarkerOptions icon it describes.

[kevin68]

Fixed. The reconnect case is the one I had not accounted for: ElementHandlerState.Reconnecting keeps the PlatformView, so both the icon and the key survive onto a different pin.

MapImageSource is no longer a no-op — it clears AppliedImageSourceKey. That is the right home for the invalidation because it is the one hook that always runs when the icon on the MarkerOptions goes stale: on every SetVirtualView, including a reconnect, and on every ImageSource change. The icon itself is still applied in AddPinAsync, since it has to be on the options before Map.AddMarker, and the mapper comment now spells that split out.

The cache still holds afterwards — I re-measured on device to be sure the invalidation does not fire on the hot path: 19 image loads for 86 markers across three zoom-in/zoom-out cycles. ReclusterPins reuses the existing handler with an unchanged VirtualView, so the mapper does not re-run there.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Async and Threading Safety — pin.Handler is MapPinHandler mapPinHandler is evaluated after the await, and ElementHandler<TVirtualView, TPlatformView>.PlatformView throws InvalidOperationException("PlatformView cannot be null here") rather than returning null (src/Core/src/Handlers/Element/ElementHandlerOfT.cs:21). The existing post-await guard re-checks ct, pin.ImageSource and pin.MarkerId, but not that the pin handler is still connected — the new write is the only statement here that can throw. Because this runs under .FireAndForget() (line 832) the exception is swallowed and the marker update is silently half-applied (marker.SetIcon done, MarkerOptions/key not). Use ((IElementHandler)mapPinHandler).PlatformView is MarkerOptions options (the non-throwing accessor) or check handler connectivity before touching PlatformView.

[kevin68]

Fixed — now reached through the non-throwing accessor:

if (pin.Handler is MapPinHandler mapPinHandler &&
    ((IElementHandler)mapPinHandler).PlatformView is MarkerOptions handlerOptions)

Your framing of the consequence is what makes it worth fixing rather than arguing about reachability: under .FireAndForget() the throw is swallowed and the update lands half-applied — marker.SetIcon done, MarkerOptions and key not — which is precisely the state the key is supposed to rule out.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Performance-Critical Path — GetClusterIconCacheKey is invoked before the requestedSource is not null test, and for IFontImageSource it builds an interpolated string from seven components (MapHandler.cs:119). AddPins re-runs for every pin on every zoom change (the exact hot path this PR is optimizing), so each pass now allocates one key string per font-image pin even when the subsequent comparison is a cache hit. Cheap conditions first: skip the key computation entirely when requestedSource is null, and consider caching the computed key alongside AppliedImageSourceKey when the IImageSource instance is reference-identical to the previously keyed one.

[kevin68]

Fixed. Restructuring for the cleared-source thread moved the key computation into the else branch, so it is no longer computed at all when requestedSource is null.

I did not add the second half — caching the computed key against a reference-identical IImageSource. It would need another field on the handler to hold the instance the key was derived from, and the allocation it saves only occurs on the miss path, which is bounded by how often the source actually changes. If font-image pins turn out to be a measurable cost on this path I would rather do it with a number in hand.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — This PR changes when a pin icon is loaded and applied (new skip path, new cross-writer state on MapPinHandler) and ships with no accompanying test: the diff is two production files only. The behaviors introduced here are testable without a device — AppliedImageSourceKey gating (second AddPins pass must not re-load a keyable source), the null-key path (uncacheable/CachingEnabled == false sources must reload every pass), and the UpdatePinImageSourceAsync → MarkerOptions write-back. Please add at least a regression test covering "icon survives a recluster pass" and "a source that opted out of caching is not frozen", since a silent regression here surfaces only as a wrong-image pin at a particular zoom level.

[kevin68]

Acknowledged, and the same constraint applies as on #37769: the Maps assembly exposes internals only to Microsoft.Maui.Controls.Core.UnitTests, which builds for $(_MauiDotNetTfm) and never compiles MapHandler.Android.cs, and the Android device tests for Map are excluded over the Google Maps API key. Neither AddPinAsync nor MapPinHandler.AppliedImageSourceKey is reachable from a test project today.

Two of the three behaviours you list are reachable in principle, though, and the seam already exists — I used it in #37770 to unit-test MapHandler.ShouldRecluster after extracting it into the shared handler. The keying rules in particular are already covered there: GetClusterIconCacheKey has tests in MapTests.cs pinning value equality and the CachingEnabled == false null result, which is exactly the "must not be frozen" behaviour this PR relies on. What is not covered is the gating itself, and that needs the Android handler.

In the meantime the behaviours are verified on device and the numbers are in the PR description. If you would like the Android Map device tests unblocked as a separate piece of work, I am happy to look at what it would take — that seems the real fix for all three of these PRs.

[kubaflo]

Could you please check the ai's suggestions?

[kevin68]

Changed direction on the caching here, after reviewing this alongside #37769 and #37770 as a set rather than on its own.

What changed

AddPinAsync now goes through the icon cache the handler already owned, instead of the per-pin AppliedImageSourceKey this PR originally added.

That cache was already there for cluster markers — bounded LRU, in-flight coalescing, expiry, generation invalidation — and AddClusterMarkerAsync was already keying it with the same GetClusterIconCacheKey this PR was calling. AppliedImageSourceKey was effectively a second, weaker cache sitting beside it: it deduped across passes for one pin, but not across pins. With 500 pins sharing one image, the first pass still decoded 500 times, and again after every MapPins. Routing both marker kinds through one GetIconAsync makes that a single decode.

What it removes

• AppliedImageSourceKey and the new internal API on MapPinHandler.
• The MapImageSource mapper change — MapPinHandler.Android.cs is back to its net11.0 state.
• The invalidation coupling that came with it. The key's only invalidation hook lived in MapPinHandler.MapImageSource, and MapPinHandler.Mapper is public static. Replacing that entry — the documented customization idiom — silently deleted the hook, after which a handler reconnected to a different pin would keep serving the previous pin's icon. There is nothing left to keep honest.

Renaming

The cache is no longer cluster-specific, so the names no longer say it is: ClusterIconCache → IconCache, GetClusterIconCacheKey/Expiry → GetIconCacheKey/Expiry, plus the fields and the tests. It backs pin markers too on Android; iOS still uses it for clusters only.

Kept from the previous round

The cleared-ImageSource reset of the reused MarkerOptions, the post-await ReferenceEquals guard, and the write-back through the non-throwing PlatformView accessor.

Test coverage

I said earlier that the gating needed the Android handler and was therefore out of reach. Moving it onto the shared cache makes the two behaviours this now rests on testable on the shared TFM, and neither had coverage before:

• a keyed source is not reloaded on a later pass — the recluster case this PR exists for;
• a null key always reloads, which is what keeps a CachingEnabled == false source from being frozen (pairs with the existing GetIconCacheKeyForUriImageSourceDependsOnCachingEnabled).

11 IconCache tests, 100/100 in MapTests.

Correction to an earlier reply

When I declined to memoize the key string, I said the allocation it saves "only occurs on the miss path". That was wrong: the key has to be computed before the lookup, so it is allocated on every pass, cache hit included. That is unchanged here — GetIconAsync computes it per call — and I still think it is worth leaving alone, but now it buys a cache that also dedupes across pins, rather than the reason I actually gave.

Trade-off worth flagging

IconCache is bounded at 64 entries and is now shared between pin and cluster images. Above ~64 distinct images the LRU thrashes and pins reload every pass — no worse than the behaviour on net11.0 today, but the per-pin key would not have had that ceiling. Happy to raise the bound if that is a realistic pin count.

Merge note

This now conflicts with #37770 in MapHandler.cs — a single comment block, where the reworded cache-key comment sits next to ShouldRecluster. Resolution is to take both, recluster pair first; whichever lands second carries it.

[MauiBot]

Expert Review — 6 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — The post-await guard drops the icon write, but the comment's assumption that UpdatePinImageSourceAsync will supply the newer icon does not hold during a recluster pass. ReclusterPins removes every marker and sets _markers = null before calling AddPins, while pin.MarkerId still names the removed marker. If pin.ImageSource changes from A to B while this await GetIconAsync(...) is in flight, PinOnPropertyChanged runs, fails to find a matching entry in the freshly rebuilt _markers list, and returns without scheduling UpdatePinImageSourceAsync. This method then resumes, ReferenceEquals(pin.ImageSource, requestedSource) is false, so nothing is written — and the reused MarkerOptions still carries icon A from the previous pass. Map.AddMarker(markerOptions) publishes the marker with the stale icon A and no further notification arrives to correct it. Consider re-reading pin.ImageSource and loading/clearing the current source instead of silently skipping the write.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Performance-Critical Path — The 64-entry LRU is now shared between cluster icons and every un-clustered pin icon, but the capacity was not revisited. On a map with more than 64 distinct pin ImageSources (a common clustering scenario), a single AddPins pass touches more distinct keys than the cache can hold, so entries are evicted in the same pass that inserted them: pin icons still decode every recluster, and the cluster icons that the cache originally existed to protect are now evicted by pin traffic on each zoom step. That is a regression of the pre-existing cluster-icon caching in exactly the workload this PR targets. Either size the cache relative to the pin count or keep cluster icons in a separate partition/reserved segment.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Performance-Critical Path — This Clear() fires whenever the cluster image version changes, and it now discards all cached pin icons as well, not just cluster icons. An app that recomputes its cluster image (bumping the version) while zooming forces every pin icon to be decoded and rescaled again on the next pass — the exact cost this PR set out to remove. Scoping the invalidation to cluster-image keys (e.g. a key prefix or a separate cache instance for cluster images) would keep the pin entries alive.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Async and Threading Safety — For any keyable source the load is started with CancellationToken.None, so LoadPinIconAsync -> imageSource.GetPlatformImageAsync(mauiContext) can no longer be aborted. Previously only cluster images had this property; it now applies to every pin icon. A slow or hanging IUriImageSource load started just before DisconnectHandler/AddPins cancellation keeps running with the captured IMauiContext until it completes, and the result is stored in the handler cache afterwards. The callers still re-check ct after the await, so there is no incorrect write — but the underlying work and the context capture outlive the cancellation. A linked token that is cancelled only when all awaiters have gone away would preserve both sharing and cancellability.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[minor] Complexity Reduction — This new write pushes the descriptor produced by the direct LoadPinIconAsync call in this method onto the handler's reused MarkerOptions, bypassing GetIconAsync/_iconCache. Consequences: (1) setting a pin's ImageSource to an image already in the cache still triggers a full decode/rescale, and (2) the descriptor stored on MarkerOptions is a different instance from the one AddPinAsync will hand the same key on the next pass, so the two paths do not actually converge on one decoded bitmap. Routing this method's load through GetIconAsync as well would make the caching invariant stated in the IconCache comment true for both writers.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The two added tests exercise only the IconCache<T> primitive (keyed reuse, null-key bypass), which already existed and is not where the reported bug lives. None of the actual behavior changes in this PR are covered: the reused MarkerOptions being cleared when ImageSource becomes null, UpdatePinImageSourceAsync writing back to the handler's MarkerOptions so a later cluster pass does not restore the previous icon, and the post-await stale-source guard in AddPinAsync. Those are the changes that could regress #37773, and they only fail-without-fix here because of the type rename, not because of behavior. A device/UI test on Android that sets a pin ImageSource, forces a zoom-driven recluster, and asserts the icon survives (and that clearing ImageSource reverts to the default marker) would give this fix real regression protection.

[MauiBot]

AI Review Summary

@kevin68 — new AI review results are available based on commit 5ef7f9d.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: net11.0 · Merge base: cbb52ef0

✅ Verified (new API / feature) — this PR adds new API and a test that references it in the same project, so reverting the fix un-compiles the test: there is no valid "fails without the fix" baseline to establish (a compile-coupled baseline). The gate instead verified the fix by a clean build + pass with the fix, so this is a real PASS rather than a non-committal INCONCLUSIVE.

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 MapTests MapTests	🛠️ BUILD ERROR	✅ PASS — 34s

🔴 Without fix — 🧪 MapTests: 🛠️ BUILD ERROR · 81s

Error-relevant lines (filtered from the build log):

/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1451,31): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1466,32): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1467,33): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1484,27): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1492,25): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1514,32): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1515,29): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1538,32): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1539,33): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1551,27): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1552,27): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1558,20): error CS0246: The type or namespace name 'IconCache<>' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1581,20): error CS0246: The type or namespace name 'IconCache<>' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1603,20): error CS0246: The type or namespace name 'IconCache<>' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]
/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1627,20): error CS0246: The type or namespace name 'IconCache<>' could not be found (are you missing a using directive or an assembly reference?) [/home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj]

🟢 With fix — 🧪 MapTests: PASS ✅ · 34s

(no coded error found; showing last 1200 chars)

oStatic [5 ms]
  Passed MapClickedAndLongClickedCanCoexist [< 1 ms]
  Passed MapLongClickedEventArgsContainsLocation [6 ms]
  Passed LastUserLocation_UpdatedOnLocationUpdate [< 1 ms]
  Passed HideInfoWindowDoesNotThrowWhenPinHasNoParent [< 1 ms]
  Passed MapElementZIndexDefaultIsZero [< 1 ms]
  Passed SettingSameClusterImageProviderMethodGroupDoesNotRebuildPins [< 1 ms]
  Passed TracksRemove [4 ms]
  Passed ItemMove [< 1 ms]
  Passed IconCacheEvictsLeastRecentlyUsedEntry [8 ms]
  Passed ClusterImageSourceInheritsBindingContext [< 1 ms]
  Passed MoveToRegionRequestProperties [1 ms]
  Passed TracksAdd [< 1 ms]
  Passed ClusterInfoConstructorThrowsOnNullArguments [2 ms]
  Passed GetClusterImagePrefersProviderOverStatic [< 1 ms]
  Passed ClusterImageSourceDefaultIsNull [< 1 ms]
  Passed GetClusterImageFallsBackToStaticWhenProviderThrows [2 ms]
  Passed MapLongClickedMultipleHandlersAllFire [< 1 ms]
  Passed MapElementIsVisibleDefaultIsTrue [< 1 ms]
  Passed TracksReplace [3 ms]
[xUnit.net 00:00:05.99]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed LastUserLocation_IsNullByDefault [< 1 ms]
Test Run Successful.
Total tests: 100
     Passed: 100
 Total time: 7.4828 Seconds

⚠️ Failure Details

• 🛠️ MapTests without fix: build failed before tests could run
  • /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/MapTests.cs(1450,30): error CS0117: 'MapHandler' does not contain a definition for 'GetIconCacheKey' [/home/vsts/work/1/s/src/Controls/tests/Core....

📁 Fix files reverted (3 files)

• src/Core/maps/src/Handlers/Map/MapHandler.Android.cs
• src/Core/maps/src/Handlers/Map/MapHandler.cs
• src/Core/maps/src/Platform/iOS/MauiMKMapView.cs

---

📋 Pre-Flight — Context & Validation

PR #37774 Pre-Flight

Context

• PR: [Android] Map: Stop reloading a pin's icon on every cluster pass
• Issue: #37773, Android Map.IsClusteringEnabled repeatedly decodes and rescales each unclustered pin's ImageSource whenever zooming triggers ReclusterPins -> AddPins -> AddPinAsync. The await also leaves the pin absent until the image resolves.
• Base: net11.0
• Materialized review commit: 2f114ddc21
• Platform: Android
• Gate: Already passed. Do not rerun fail-without-fix verification and do not modify gate/content.md.

Current PR Approach

The current fix broadens the existing cluster-image cache into a handler-wide bounded IconCache<T> shared by cluster and regular pin markers:

1. GetIconCacheKey keys stable file, URI, and font sources by value; unkeyable or cache-disabled sources bypass caching.
2. GetIconAsync shares keyed loads without tying them to one caller's cancellation token.
3. AddPinAsync reuses the cached descriptor, clears reused MarkerOptions when ImageSource becomes null, and rejects a stale post-await write when the source changed.
4. UpdatePinImageSourceAsync updates both the live marker and the handler's reused MarkerOptions through the non-throwing IElementHandler.PlatformView accessor.
5. Cache state is cleared on handler cleanup and cluster image version changes. The shared cache type/helper rename also updates the iOS cluster-image caller.

This replaced an earlier per-MapPinHandler applied-source-key proposal. Review of that earlier proposal found stale post-await writes, failure to clear a removed source, state surviving handler reuse, and unsafe typed PlatformView access; the current PR addresses those cases with cache reuse and explicit guards.

Committed Diff

• src/Core/maps/src/Handlers/Map/MapHandler.Android.cs — uses the shared cache for pin and cluster icons and synchronizes live marker/MarkerOptions updates.
• src/Core/maps/src/Handlers/Map/MapHandler.cs — generalizes stable cache-key/expiry helpers and ClusterIconCache<T> into IconCache<T>.
• src/Core/maps/src/Platform/iOS/MauiMKMapView.cs — updates existing iOS cluster cache references to the generalized internal names.
• src/Controls/tests/Core.UnitTests/MapTests.cs — renames existing cache tests and adds keyed-reuse and null-key-bypass coverage.

Only the three production files are reverted by EstablishBrokenBaseline.ps1 and therefore form the try-fix modification allow-list. The test file remains at the PR version, so an alternative must retain compatible internal cache helper/type behavior for the compile-coupled tests or honestly report failure/blockage.

Targeted Test

Run only the detected primary test class, which includes the new cache tests and the existing map regressions:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~MapTests"

No additional mandatory regression test was supplied for STEP 5a. Do not run the full repository suite, UI tests, device tests, or the gate verification.

Attempt Constraints

• Each candidate must use a root-cause mechanism different from the current shared handler-wide icon cache and from any earlier candidate.
• One implementation/test pass is allowed, plus at most one focused correction/retest.
• Preserve all pre-existing unrelated working-tree changes and use only pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore for restoration.

---

🔬 Code Review — Deep Analysis

Expert Evaluation — PR #37774

Verdict: LGTM with non-blocking warnings

Confidence: medium

Independent Assessment

The PR makes the shared icon cache available to Android pin icons, clears reused MarkerOptions when a pin image is removed, and keeps MarkerOptions synchronized when a live pin image changes. The value-based cache-key handling, post-await source check, non-throwing platform-view access, and iOS internal-symbol rename are directionally sound. The submitted implementation addresses the reported repeated image loading without introducing a verified blocking regression.

Findings

1. ⚠️ Moderate — The post-await guard does not guarantee correction during reclustering (src/Controls/Maps/src/Handlers/Map/MapHandler.Android.cs:930). While reclustering, the old marker has already been removed and the replacement may not yet exist. If ImageSource changes during the load, the property-change path can find no marker, while AddPinAsync drops the completed load; the reused MarkerOptions can therefore retain the prior icon with no remaining notification to correct it. This is an incomplete race mitigation rather than a regression from the pre-PR behavior.
2. ⚠️ Moderate — Pin and cluster icons now share the existing 64-entry cache (MapHandler.Android.cs:52). A map with many distinct pin images can evict cluster entries and vice versa, potentially causing cache thrash in the workload this change is intended to optimize.
3. 💡 Minor — Cluster-version invalidation now clears pin icons too (MapHandler.Android.cs:417), increasing reload work after a renderer/version change.
4. 💡 Minor — Keyed image loads use CancellationToken.None (MapHandler.Android.cs:992), so abandoned loads continue after map state changes.
5. 💡 Minor — The property-change write-back bypasses the shared cache (MapHandler.Android.cs:982), allowing the update and add paths to produce separate descriptors for the same source.
6. ⚠️ Moderate — Tests exercise the cache primitive rather than the Android pin behavior (src/Controls/Maps/tests/DeviceTests/Map/MapTests.cs:1601). They do not directly cover unchanged-source reclustering, image removal, handler reuse, or the image-change race.

The raw inline findings are persisted in PRAgent/inline-findings.json.

Failure-Mode and Blast-Radius Assessment

The behavioral change is confined primarily to Android Maps, with iOS receiving internal renames only and no public API change. No new subscriptions or static/shared state were introduced. Cleanup invalidation and in-flight generation handling are symmetric. The main unresolved edge case is an ImageSource change while AddPinAsync is awaiting during reclustering; the cache-capacity concern is workload-dependent rather than a correctness failure.

Reconciliation

The code-first assessment agrees with the PR narrative's root cause and intended optimization. Earlier concerns recorded in pre-flight about clearing removed images, handler reuse, stale post-await writes, and safe platform access are substantially addressed. The residual recluster race above remains narrower than the earlier stale-write defect.

Validation Evidence

The trusted Gate passed: tests fail without the submitted fix and pass with it. The expert reviewer could not retrieve required-check status from GitHub and did not rerun the Gate.

---

🛠️ Try-Fix — Analysis & Comparison

PR #37774 — Alternative Fix Candidates

Candidate 1 — Incremental reclustering with marker reuse (claude-opus-5)

Result: Pass
Candidate narrative and full diff: ../try-fix-1/content.md
Attempt artifacts: attempt-1/

Rather than caching pin icons or recording which source was applied, this candidate removes the cause of the repeated load: ReclusterPins retains live markers for pins that remain standalone at the new zoom and removes/recreates only markers whose clustering outcome changed. Reused markers never re-enter AddPinAsync, so they neither decode their image again nor disappear during an awaited load. A handler-lifetime pin-image cancellation token keeps property-driven icon updates alive across recluster passes; null sources clear reused MarkerOptions. The existing cluster-cache internals are renamed only to satisfy the PR's compile-coupled tests and remain unused for regular pin icons.

This differs mechanistically from both the PR's shared handler cache and the rejected per-pin applied-key design: those accept full marker teardown and preserve decoded/applied state across an async rebuild, while this candidate preserves the marker itself and eliminates the rebuild/await for unchanged standalone pins.

Changed files: MapHandler.Android.cs (+139/−26), MapHandler.cs (+4/−4), and MauiMKMapView.cs (+3/−3), all within the baseline allow-list.

Targeted test:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~MapTests"
Passed: 100; Failed: 0; Skipped: 0

The requested test compiles only the shared net11.0 maps target, so it validates the compile-coupled API rename and map regressions but does not compile or execute the Android implementation.

Inline self-review: 3 findings (0 critical, 0 major, 2 moderate, 1 minor): a low-risk CTS/disconnect race, missing Android coverage for null-icon clearing, and the narrower benefit when a pin repeatedly changes between clustered and standalone.

Restoration: EstablishBrokenBaseline.ps1 -Restore reported Restored True; no attempt-created source changes remain.

Candidate 2 — Pin-lifecycle image preparation (gpt-5.6-sol)

Result: Pass
Candidate narrative and full diff: ../try-fix-2/content.md
Attempt artifacts: attempt-2/

This candidate decouples regular-pin image preparation from marker construction. AddPinAsync becomes a synchronous AddPin that immediately adds the marker from the handler's existing MarkerOptions. Decode/rescale starts only when the pin first enters its subscribed standalone lifecycle or when ImageSource changes. A successful load writes the descriptor to the durable MarkerOptions and then the pin's current live marker, with map, source, virtual-view, and pin-membership guards rejecting stale continuations. Reclustering still tears down and recreates every marker, but it no longer initiates or awaits image loading; the already-prepared native options make reconstruction synchronous.

Unlike the PR, regular-pin descriptors never enter a keyed shared cache. Unlike the rejected per-handler design, there is no applied-source key or state consulted during reconstruction. Unlike Candidate 1, live markers are not retained and no handler-lifetime CTS is introduced. The causal shift is from reconstruction-time image resolution to pin-lifecycle/source-change image preparation.

Changed files: MapHandler.Android.cs (+52/−32), MapHandler.cs (+4/−4), and MauiMKMapView.cs (+3/−3), all within the baseline allow-list.

Targeted test:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~MapTests"
Passed: 100; Failed: 0; Skipped: 0

The test ran once without a correction round. As with Candidate 1, this net11.0 target does not compile or execute MapHandler.Android.cs, so the Pass validates shared compile coupling and existing map tests rather than Android runtime behavior.

Inline self-review: 2 moderate findings (0 critical/major): a failed initial icon load is not retried on later zooms until another lifecycle/source event, and the permitted test provides no Android implementation coverage.

Restoration: EstablishBrokenBaseline.ps1 -Restore reported Restored True; no attempt-created target-file changes remain and pre-existing harness changes were preserved.

Bounded Outcome

Exactly two candidates were attempted in the required model order. Both passed the only permitted targeted test, but neither Android implementation was compiled or behaviorally exercised by that test. Candidate 1 removes repeated work by retaining unchanged standalone markers; Candidate 2 retains full marker rebuilding but moves image preparation out of that path. Their full saved diffs and self-review findings are linked above for STEP 5b assessment.

---

📝 PR Finalize — Recommended Title & Description

Assessment: ✏️ Recommend updating — the title is accurate, but the description still documents the superseded per-MapPinHandler applied-source record, mapper invalidation, and old GetClusterIconCacheKey name instead of the shared IconCache<T> implementation in the submitted PR HEAD.

Recommended title

[Android] Map: Stop reloading a pin's icon on every cluster pass

Recommended description

Fixes #37773

### Description of Change

`ReclusterPins` re-runs `AddPins` on every zoom change, and `AddPinAsync` previously reloaded `Pin.ImageSource` unconditionally each time — a Glide fetch plus a decode and rescale for every un-clustered pin, on every pass. The `MarkerOptions` that receives the icon already outlives the markers built from it, so repeating that work is unnecessary while the source is unchanged.

The Android `MapHandler` now routes regular pin icons through the existing bounded cluster-icon cache, generalized from `ClusterIconCache<T>` to `IconCache<T>`. `AddPinAsync` uses `GetIconAsync`, so markers naming the same stable image key reuse one decoded `BitmapDescriptor`, including equivalent `ImageSource` instances and concurrent loads.

The key comes from `GetIconCacheKey`, the generalized helper used by both regular pin and cluster icons. It compares stable sources by value rather than reference. It returns `null` for a source that cannot be keyed stably or has opted out of caching — including streams and a `UriImageSource` with `CachingEnabled` false or a non-positive `CacheValidity` — so those sources continue to reload instead of being frozen for the handler lifetime.

The source is captured before each await and re-checked afterward before the icon is written. `UpdatePinImageSourceAsync` applies a changed icon to both the live marker and its reused `MarkerOptions`, reaching the platform element through the non-throwing accessor because the method resumes after an await. This prevents a later cluster pass from restoring the previous icon.

A cleared `ImageSource` now removes the previous icon from the reused `MarkerOptions` with `SetIcon(null)`, which restores Android's default marker instead of rendering an image the source no longer names.

On the common unchanged-source path, the cache hit completes without asynchronous image work before `Map.AddMarker`, so an un-clustered pin no longer disappears while its image is fetched and decoded on every zoom pass.

The cache is cleared during handler cleanup and when the cluster-image owner or version changes. The iOS change only updates references to the generalized internal cache and helper names; regular pin-icon caching behavior changes only on Android.

### Note on ordering

Split out of #37769 so the bug fix and this caching behavior stay separately reviewable. It applies independently, but #37769 is the one that fixes broken behavior — merge that first.

### Verification

`MapTests` cover cache-key behavior, concurrent load coalescing, LRU eviction, keyed reuse across later passes, and null-key bypass.

Physical device (Samsung SM-X230, Android 15), clustering gallery using pins whose icon is a PNG written to `FileSystem.CacheDirectory`, handler instrumented at each boundary. Three zoom-in/zoom-out cycles over 25 clustered custom pins, merged with #37769: **19 image loads for 86 markers** instead of one load per marker, every de-clustered pin keeping its icon, no Glide asserts, no recycled-bitmap errors, and no crashes.

### Issues Fixed

Fixes #37773

---

🏁 Report — Final Recommendation

✅ Final Recommendation: APPROVE

Winner: pr

The raw submitted PR is the strongest candidate. The trusted Gate passed, the single expert review found no blocking correctness defect or unresolved discussion item, and its implementation directly addresses repeated pin-icon decoding while preserving cache opt-out semantics. The review surfaced a residual source-change race, shared-cache pressure, and a behavior-test gap, but the race is an incomplete mitigation rather than a regression and the performance risks are workload-dependent.

Comparative Ranking

Rank	Candidate	Validation	Assessment
1	pr	Gate passed: tests fail without the fix and pass with it	Smallest proven change. It reuses value-based keys, handles cache-disabled/unkeyable sources correctly, clears removed images, synchronizes live markers with reused MarkerOptions, and has documented Android device verification. Expert findings are non-blocking.
2	try-fix-1	Passed: 100 MapTests	Incremental marker reuse removes reloads and marker disappearance for pins that remain standalone, including unkeyable sources. It is substantially more invasive, does not help pins that repeatedly cross the cluster boundary, introduces a handler-lifetime CTS race concern, and its Android implementation was neither compiled nor behaviorally exercised by the test target.
3	try-fix-2	Passed: 100 MapTests	Moving image preparation to the pin lifecycle makes recluster reconstruction synchronous without a shared pin cache. It retains full marker churn, does not retry a failed initial image load on later zooms, and its Android implementation was neither compiled nor behaviorally exercised.
4	pr-plus-reviewer	Failed: candidate-caused CS8632 compile errors	The consolidated patch improves cache isolation/capacity and closes the expert's residual recluster race, but its required targeted validation failed in the added test. It must rank below every passing candidate and was not repaired or retested.

Expert Review Reconciliation

The code-first assessment agreed with the submitted PR's root cause and design. The stable cache key is value-based and deliberately returns null for streams and cache-disabled URI sources; cleanup and cache-generation invalidation are symmetric; iOS changes are internal renames only; and no public API, subscription, startup, or global-state regression was introduced.

The main warning is a narrow timing window in which ImageSource changes while AddPinAsync awaits during reclustering: no live marker exists for the notification, and the stale load is dropped without loading the replacement. This can temporarily publish reused options with the prior icon, but it does not make pre-PR behavior worse. The 64-entry cache shared with cluster icons can also thrash for maps with many distinct sources, while cluster-version invalidation clears pin entries. These are valid follow-up concerns, not blockers established by the supplied evidence.

Why the Alternatives Do Not Win

Both STEP 5a alternatives passed the only permitted test, so they outrank the failed reviewer candidate. That test compiles only the shared net11.0 maps target, however, and does not compile either alternative's Android implementation. Each alternative also carries a broader or more concrete behavioral limitation than the submitted PR: Candidate 1 changes reclustering ownership and cancellation across a much larger surface, while Candidate 2 can permanently retain the default icon after a transient initial load failure until another lifecycle or source event.

The raw PR therefore remains the only candidate combining a passing trusted Gate, a bounded and reviewable implementation, direct physical-device evidence, and no blocking expert finding.

---

📱 UI Tests — Button,Label,Layout

Detected UI test categories: Button,Label,Layout

✅ Deep UI tests — 360 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
Button	71/73 (2 skipped) ✓	—
Label	97/99 (2 skipped) ✓	—
Layout	192/195 (3 skipped) ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

---

🧭 Next Steps — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.