fix(core): Make feature flag buffer merging thread-safe - #5989
Open
runningcode wants to merge 2 commits into
Open
fix(core): Make feature flag buffer merging thread-safe#5989runningcode wants to merge 2 commits into
runningcode wants to merge 2 commits into
Conversation
FeatureFlagBuffer.merged() read size() and then indexed into the live CopyOnWriteArrayList held by each buffer. add() mutates in three steps (remove, add, remove), so the list transiently shrinks and a merging thread could index past its end. Scopes.captureEventInternal swallows the resulting ArrayIndexOutOfBoundsException, so the event is silently dropped, including events from the uncaught exception handler. Capturing the list reference did not help: the field was never reassigned, so every reader saw the same live list. Hold an immutable list behind the volatile field instead and swap it under the existing lock, so one volatile read yields a snapshot that can no longer change. This also makes clone() free, since it now shares the list rather than copying it, and cuts add() from three full array copies down to one. CopyOnWriteArrayList is no longer needed and maxSize becomes final, which it always should have been. Entries carrying identical timestamps now resolve in favour of the most specific scope, CURRENT over ISOLATION over GLOBAL, rather than the reverse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a416a65 | 333.78 ms | 410.37 ms | 76.59 ms |
| 2195398 | 319.02 ms | 342.38 ms | 23.36 ms |
| 62b579c | 349.26 ms | 426.26 ms | 77.00 ms |
| bbc35bb | 324.88 ms | 425.73 ms | 100.85 ms |
| e59e22a | 374.68 ms | 442.14 ms | 67.46 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| 62b579c | 312.88 ms | 361.57 ms | 48.70 ms |
| 8687935 | 332.52 ms | 362.23 ms | 29.71 ms |
| 1edbdfa | 364.77 ms | 450.29 ms | 85.52 ms |
| 2195398 | 322.52 ms | 361.91 ms | 39.39 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 2195398 | 0 B | 0 B | 0 B |
| 62b579c | 0 B | 0 B | 0 B |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| e59e22a | 1.58 MiB | 2.20 MiB | 635.34 KiB |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| 62b579c | 0 B | 0 B | 0 B |
| 8687935 | 1.58 MiB | 2.19 MiB | 619.17 KiB |
| 1edbdfa | 1.58 MiB | 2.20 MiB | 635.34 KiB |
| 2195398 | 0 B | 0 B | 0 B |
runningcode
marked this pull request as ready for review
August 25, 2026 10:11
runningcode
requested review from
0xadam-brown,
adinauer,
markushi and
romtsn
as code owners
August 25, 2026 10:11
9 tasks
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
1 participant
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.
📜 Description
When we merge the feature flags in merge(), we iterate through the lists without copying them or obtaining a lock.
the add() method obtains a lock, and does the add by first removing an existing flag (if exists) then adding it again and then doing another remove if the list is too long.
That means that if we call merge and then another thread calls add , there's no guarantee that anything will work or that it won't throw an exception.
The fix holds an immutable list behind the volatile field, swapped under the existing lock. One volatile read now yields a snapshot that can no longer change, so the index arithmetic in
merged()is safe and the comment becomes true.Falling out of that:
clone()is now free — it shares the list rather than copying it. Since the class doc calls scope cloning the optimized path, this is the one that matters.add()drops from three full array copies to one.CopyOnWriteArrayListdisappears from the class;clear()becomes an empty-list swap.maxSizebecomesfinal, which it always should have been — as written, a thread obtaining a buffer through a data race could legally observemaxSize == 0.FeatureFlagEntry.nanosbecomes a primitivelonginstead of a boxed@NotNull Long, and its stale@SuppressWarnings("UnusedVariable")is gone.>meant whichever buffer was tested first won equal timestamps.getFeatureFlags()is left annotated@Nullableeven though it never returns null, to stay consistent with theIFeatureFlagBuffersignature.💡 Motivation and Context
💚 How did you test it?
Added a test that merges in a loop while another thread adds flags. Against the old implementation it fails with
ArrayIndexOutOfBoundsException: Index 100 out of bounds for length 100; against the new one it passes. Also added a test pinningclone()independence, since clone changed from copying the list to sharing it. Full:sentry:testsuite passes andapiDumpproduces no changes.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
JAVA-704 also notes an optional follow-up: replacing
System.nanoTime()with a staticAtomicLongsequence, which would give a total order across buffers with no ties at all and no dependence on clock granularity. Left out here to keep the change small.