Fix s_currentTweener++ in AnimationExtensions is not thread-safe, producing duplicate animation IDs

[HarishwaranVijayakumar]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Issue Details

• s_currentTweener++ at lines 67 and 87 of src/Controls/src/Core/AnimationExtensions.cs is a non-atomic read-modify-write operation. When multiple threads call AnimationExtensions.Add() or Insert() concurrently (which happens via finalizer thread access as documented in the code comments), two threads can read the same value and produce duplicate IDs. These duplicates overwrite entries in s_tweeners (a ConcurrentDictionary), causing silently lost animations.

Description of Change

Thread safety improvements:

• Replaced the use of s_currentTweener++ with Interlocked.Increment(ref s_currentTweener) in both the Add and Insert methods of AnimationExtensions to ensure unique IDs are generated safely when accessed by multiple threads.
• Added using System.Threading; to support atomic operations.

Testing enhancements:

• Introduced a new test suite AnimationExtensionsThreadSafetyTests that verifies Add and Insert produce unique IDs even under high concurrency, preventing regressions in thread safety. This includes a minimal NoOpAnimationManager implementation for testing purposes.

Reference - https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/DataTemplate.cs#L22

Issues Fixed

Fixes #35837

Tested the behaviour in the following platforms

• - Windows
• - Android
• - iOS
• - Mac

Before	After

[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 -- 35838

Or

• Run remotely in PowerShell:

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

[MauiBot]

AI Review Summary

@HarishwaranVijayakumar — new AI review results are available based on this last commit: 85ad2fc. To request a fresh review after new comments or commits, comment /review rerun.

Review Sessions — click to expand

Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: main · Merge base: 32d494c2

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 AnimationExtensionsThreadSafetyTests AnimationExtensionsThreadSafetyTests	✅ FAIL — 113s	✅ PASS — 105s

🔴 Without fix — 🧪 AnimationExtensionsThreadSafetyTests: FAIL ✅ · 113s

  Determining projects to restore...
  Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 4.69 sec).
  Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 8.71 sec).
  Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 3.62 sec).
  Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 6 ms).
  Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 11 ms).
  Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 57 ms).
  Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 32 ms).
  Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 34 ms).
  Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 76 ms).
  1 of 10 projects are up-to-date for restore.
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
  Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
  TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
  Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.15]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.25]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.26]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.34]       Assert.Equal() Failure: Values differ
[xUnit.net 00:00:01.34]       Expected: 10000
[xUnit.net 00:00:01.34]       Actual:   9903
[xUnit.net 00:00:01.34]       Stack Trace:
[xUnit.net 00:00:01.34]     Add_ConcurrentCalls_ProducesUniqueIds [FAIL]
[xUnit.net 00:00:01.34]         /_/src/Controls/tests/Core.UnitTests/AnimationExtensionsThreadSafetyTests.cs(41,0): at Microsoft.Maui.Controls.Core.UnitTests.AnimationExtensionsThreadSafetyTests.Add_ConcurrentCalls_ProducesUniqueIds()
[xUnit.net 00:00:01.34]            at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:01.34]            at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:01.35]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Failed Add_ConcurrentCalls_ProducesUniqueIds [28 ms]
  Error Message:
   Assert.Equal() Failure: Values differ
Expected: 10000
Actual:   9903
  Stack Trace:
     at Microsoft.Maui.Controls.Core.UnitTests.AnimationExtensionsThreadSafetyTests.Add_ConcurrentCalls_ProducesUniqueIds() in /_/src/Controls/tests/Core.UnitTests/AnimationExtensionsThreadSafetyTests.cs:line 41
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
  Passed Insert_ConcurrentCalls_ProducesUniqueIds [10 ms]

Test Run Failed.
Total tests: 2
     Passed: 1
     Failed: 1
 Total time: 1.9904 Seconds

🟢 With fix — 🧪 AnimationExtensionsThreadSafetyTests: PASS ✅ · 105s

  Determining projects to restore...
  All projects are up-to-date for restore.
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
  Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
  Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14422398
  Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
  TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
  Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.61]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:03.78]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:03.79]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:03.98]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed Add_ConcurrentCalls_ProducesUniqueIds [60 ms]
  Passed Insert_ConcurrentCalls_ProducesUniqueIds [26 ms]

Test Run Successful.
Total tests: 2
     Passed: 2
 Total time: 5.4881 Seconds

📁 Fix files reverted (1 files)

• src/Controls/src/Core/AnimationExtensions.cs

---

UI Tests

Full UI test matrix will run (no specific categories detected from PR changes).

---

Pre-Flight — Context & Validation

Issue: #unknown - unavailable (GitHub CLI authentication unavailable)
PR: #35838 - unavailable (local squashed review commit: thread-safe AnimationExtensions tweener IDs)
Platforms Affected: all platforms; testing platform requested: android
Files Changed: 1 implementation, 1 test

Key Findings

• GitHub context gathering was blocked because gh is not authenticated in this environment; continued with local checked-out PR diff as required.
• The implementation changes AnimationExtensions.Add and AnimationExtensions.Insert from non-atomic s_currentTweener++ to atomic ID generation.
• Added xUnit unit coverage exercises concurrent Add and Insert calls and asserts generated IDs are unique.
• No UI-test categories are directly impacted; this is shared animation infrastructure with unit-test coverage.

Code Review Summary

Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 0 | Suggestions: 0

Key code review findings:

• No inline findings warranted from the local MAUI expert review.
• CI and PR discussion/review reconciliation could not be verified because GitHub CLI authentication is unavailable, so confidence is capped at low.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #35838	Use Interlocked.Increment(ref s_currentTweener) in Add and Insert to generate unique tweener IDs under concurrency.	✅ PASSED (Gate)	src/Controls/src/Core/AnimationExtensions.cs, src/Controls/tests/Core.UnitTests/AnimationExtensionsThreadSafetyTests.cs	Original PR fix; gate result was provided as passed and was not re-run.

---

Code Review — Deep Analysis

Code Review — PR #35838

Independent Assessment

What this changes: The PR replaces unsynchronized s_currentTweener++ operations in AnimationExtensions.Add and AnimationExtensions.Insert with Interlocked.Increment(ref s_currentTweener), and adds unit tests that call both APIs concurrently to assert ID uniqueness.
Inferred motivation: s_currentTweener++ is a read/modify/write sequence and can produce duplicate IDs under concurrent animation registration. Duplicate IDs can overwrite entries in the shared tweener dictionary and cause the returned ticker IDs to refer to the wrong animation or to an animation that was already replaced.

Reconciliation with PR Narrative

Author claims: Unavailable; gh pr view failed because GitHub CLI authentication is not configured.
Agreement/disagreement: Local code analysis supports the inferred bug fix. The implementation addresses concurrent ID generation at the shared counter where both affected entry points obtain IDs.

Prior Review Reconciliation

Prior ❌ Error Finding	Source	Status	Evidence
N/A	N/A	N/A	Prior PR reviews could not be queried because GitHub CLI authentication is unavailable. No local prior ❌ Error findings were available.

Blast Radius Assessment

• Runs for all instances: yes — Add and Insert are shared animation/tweener registration helpers used by animation infrastructure.
• Startup impact: no direct startup execution; code runs when animations/tweeners are registered.
• Static/shared state: yes — the fix changes access to the static s_currentTweener counter.

CI Status

• Required-check result: undetermined
• Classification: undetermined
• Action taken: GitHub CLI was unauthenticated, so CI could not be checked; confidence capped at low.

Findings

No ❌ Error, ⚠️ Warning, or 💡 Suggestion findings.

Failure-Mode Probing

• Concurrent calls to Add: the PR's atomic increment prevents duplicate IDs for concurrent callers.
• Concurrent calls to Insert: the same atomic increment covers kinetic/tweener registration.
• Existing consumers expecting opaque IDs: IDs remain int values and continue to be used only as handles into s_tweeners; the first generated value changes relative to the old post-increment sequence, but callers should not depend on exact numeric values.
• Counter overflow: unchanged existing risk for a long-lived static int; not introduced by this PR.

Verdict: NEEDS_DISCUSSION

Confidence: low
Summary: The local code review found no correctness issues in the PR diff, and the approach is a direct fix for a real race in shared animation ID generation. The verdict cannot be LGTM because GitHub PR metadata, prior reviews, and CI status are unavailable in this environment.

---

Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	try-fix-1	Private lock-protected helper serializes s_currentTweener++ for Add and Insert.	✅ PASS	1 file	Correct and preserves original post-increment sequence, but not better than PR; monitor lock is heavier than Interlocked for a simple shared counter.
2	try-fix-2	Use ConcurrentDictionary.TryAdd as an active-ID reservation loop and retry on collisions.	✅ PASS	1 file	Focused tests pass, but expert/self-review found a major semantic weakness: completed animations can free an ID before a racing duplicate reserves it, allowing returned ID reuse.
PR	PR #35838	Use Interlocked.Increment(ref s_currentTweener) in AnimationExtensions.Add and Insert.	✅ PASSED (Gate)	2 files	Original PR; simplest correct process-wide atomic allocation. Gate result was provided as passed and was not re-run.

Cross-Pollination

Model	Round	New Ideas?	Details
maui-expert-reviewer	1	Yes	Suggested/testing-approved lock-serialized allocation as a valid but less preferable alternative to Interlocked.
maui-expert-reviewer	2	Yes	Suggested/testing-approved dictionary reservation loop as meaningfully different, while warning it may be weaker in completion/removal scenarios.
maui-expert-reviewer	3	No	NO NEW IDEAS: CompareExchange is a verbose reimplementation of Interlocked.Increment; moving allocation to AnimationManager is architectural/API churn; per-thread ranges still need atomic global reservation and add complexity; tests-only does not fix the race.

Candidate Narratives

try-fix-1 — Lock-Serialized Tweener ID Allocation

Approach: Add a private static lock and helper to serialize s_currentTweener++ in AnimationExtensions.Add and AnimationExtensions.Insert.

Diff: See ../try-fix-1/fix.diff.

Test Results: ✅ PASS — dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~AnimationExtensionsThreadSafetyTests" passed 2/2 tests.

Failure Analysis: Not a test failure. Rejected because it is not demonstrably better than the PR fix: it adds monitor locking where Interlocked.Increment is simpler, lock-free, and idiomatic.

try-fix-2 — ConcurrentDictionary Reservation Loop

Approach: Leave s_currentTweener++ non-atomic, but use s_tweeners.TryAdd(id, animation) as the active-ID reservation mechanism and retry on collisions. Commit only after reservation succeeds.

Diff: See ../try-fix-2/fix.diff.

Test Results: ✅ PASS — dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "FullyQualifiedName~AnimationExtensionsThreadSafetyTests" passed 2/2 tests.

Failure Analysis: Rejected despite passing tests. It prevents active dictionary overwrites but does not guarantee unique returned IDs if an animation completes and removes the dictionary entry before another racing caller with the same generated ID reaches TryAdd.

Exhausted: Yes
Selected Fix: PR's fix — it is the simplest correct option. It directly makes the shared counter allocation atomic, passes the provided gate, avoids lock overhead, avoids dictionary-reservation edge cases, and has the clearest correctness argument.

---

Report — Final Recommendation

Comparative Fix Report — PR #35838

Inputs

• pr: PR #35838 raw fix, changing tweener ID allocation in AnimationExtensions.Add and AnimationExtensions.Insert to Interlocked.Increment(ref s_currentTweener).
• pr-plus-reviewer: PR fix plus expert reviewer feedback applied in a sandbox candidate. The expert reviewer found no actionable feedback, so this candidate is identical to pr.
• try-fix-1: Lock-serialized tweener ID allocation helper. Focused regression tests passed.
• try-fix-2: ConcurrentDictionary.TryAdd reservation loop. Focused regression tests passed, but STEP 5a identified a semantic weakness when completed animations free IDs during races.

Gate result for the PR fix was provided as passed: tests fail without the fix and pass with the fix. The gate was not re-run.

Candidate Ranking

Rank	Candidate	Regression Result	Assessment
1	pr	✅ Passed gate	Best candidate. It fixes the root race at the shared counter with the simplest atomic primitive, keeps the existing lifecycle unchanged, avoids lock overhead, and avoids dictionary-reservation edge cases.
2	pr-plus-reviewer	✅ Same as PR	No actionable reviewer feedback was found, so this is identical to pr. It is not selected separately because it contributes no additional improvement over the raw PR fix.
3	try-fix-1	✅ Passed focused regression tests	Correct, but uses monitor locking for a simple integer allocation. It is safe but heavier and less idiomatic than Interlocked.Increment.
4	try-fix-2	✅ Passed focused regression tests	Weaker than the PR despite passing tests. It prevents active dictionary overwrites but can still reuse a returned ID if an animation completes and removes its dictionary entry before another racing caller with the same non-atomically generated ID reserves it.

No candidate failed its recorded regression tests, so ranking is based on correctness strength, simplicity, and risk.

Winning Candidate

Winner: pr

The raw PR fix is the single best candidate because it fixes the actual shared-state race at the source using Interlocked.Increment, has the clearest correctness argument, and preserves the existing animation commit/removal behavior. pr-plus-reviewer is equivalent but not a distinct improvement, try-fix-1 is correct but heavier, and try-fix-2 has a real semantic hole despite passing the focused tests.

---

Future Action — review latest findings

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