From d98269fed1aa3de63adeed2efbb403f931616ec9 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 25 Aug 2026 10:30:54 +0200 Subject: [PATCH 1/2] fix(core): Make feature flag buffer merging thread-safe (JAVA-704) 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) --- .../featureflags/FeatureFlagBuffer.java | 131 +++++++++--------- .../featureflags/FeatureFlagBufferTest.kt | 50 +++++++ 2 files changed, 114 insertions(+), 67 deletions(-) diff --git a/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java b/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java index fc696b5948f..5a02a911ace 100644 --- a/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java +++ b/sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java @@ -8,8 +8,9 @@ import io.sentry.util.AutoClosableReentrantLock; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; +import java.util.Map; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -23,63 +24,63 @@ *
  • Performance of scope cloning is optimized here *
  • Supports merging across scope types (GLOBAL, ISOLATION, CURRENT) * + * + *

    {@link #flags} always holds an immutable list. Writers hold {@link #lock} and swap in a new + * list; readers take a single volatile read and are then free to iterate or index into a list that + * can no longer change. This is what makes {@link #clone()} free: the copy shares the list rather + * than duplicating it. */ @ApiStatus.Internal public final class FeatureFlagBuffer implements IFeatureFlagBuffer { - private volatile @NotNull CopyOnWriteArrayList flags; + private volatile @NotNull List flags; private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); - private int maxSize; + private final int maxSize; - private FeatureFlagBuffer(int maxSize) { - this.maxSize = maxSize; - this.flags = new CopyOnWriteArrayList<>(); + private FeatureFlagBuffer(final int maxSize) { + this(maxSize, Collections.emptyList()); } - private FeatureFlagBuffer( - int maxSize, final @NotNull CopyOnWriteArrayList flags) { + private FeatureFlagBuffer(final int maxSize, final @NotNull List flags) { this.maxSize = maxSize; this.flags = flags; } - private FeatureFlagBuffer(@NotNull FeatureFlagBuffer other) { - this.maxSize = other.maxSize; - this.flags = new CopyOnWriteArrayList<>(other.flags); - } - @Override public void add(final @Nullable String flag, final @Nullable Boolean result) { if (flag == null || result == null) { return; } try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - final int size = flags.size(); - for (int i = 0; i < size; i++) { - final @NotNull FeatureFlagEntry entry = flags.get(i); - if (entry.flag.equals(flag)) { - flags.remove(i); - break; + final @NotNull List current = flags; + final @NotNull List updated = new ArrayList<>(current.size() + 1); + for (final @NotNull FeatureFlagEntry entry : current) { + if (!entry.flag.equals(flag)) { + updated.add(entry); } } - flags.add(new FeatureFlagEntry(flag, result, System.nanoTime())); + updated.add(new FeatureFlagEntry(flag, result, System.nanoTime())); - if (flags.size() > maxSize) { - flags.remove(0); + if (updated.size() > maxSize) { + updated.remove(0); } + + flags = Collections.unmodifiableList(updated); } } @Override public void clear() { try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - flags.clear(); + flags = Collections.emptyList(); } } @Override public @Nullable FeatureFlags getFeatureFlags() { - List featureFlags = new ArrayList<>(); - for (FeatureFlagEntry entry : flags) { + final @NotNull List snapshot = flags; + final @NotNull List featureFlags = new ArrayList<>(snapshot.size()); + for (final @NotNull FeatureFlagEntry entry : snapshot) { featureFlags.add(entry.toFeatureFlag()); } return new FeatureFlags(featureFlags); @@ -87,7 +88,7 @@ public void clear() { @Override public @NotNull IFeatureFlagBuffer clone() { - return new FeatureFlagBuffer(this); + return new FeatureFlagBuffer(maxSize, flags); } public static @NotNull IFeatureFlagBuffer create(final @NotNull SentryOptions options) { @@ -123,6 +124,9 @@ public void clear() { *

    If a duplicate is found we skip it since we're iterating in reverse order and we already * have the latest entry. * + *

    Entries carrying the same timestamp are resolved in favour of the most specific scope, so + * CURRENT wins over ISOLATION, which in turn wins over GLOBAL. + * * @param maxSize max number of feature flags * @param globalBuffer buffer from global scope * @param isolationBuffer buffer from isolation scope @@ -135,12 +139,13 @@ public void clear() { final @Nullable FeatureFlagBuffer isolationBuffer, final @Nullable FeatureFlagBuffer currentBuffer) { - // Capture references to avoid inconsistencies from concurrent modifications - final @Nullable CopyOnWriteArrayList globalFlags = + // One volatile read each pins an immutable list, so concurrent writers cannot shift these + // out from under the index arithmetic below + final @Nullable List globalFlags = globalBuffer == null ? null : globalBuffer.flags; - final @Nullable CopyOnWriteArrayList isolationFlags = + final @Nullable List isolationFlags = isolationBuffer == null ? null : isolationBuffer.flags; - final @Nullable CopyOnWriteArrayList currentFlags = + final @Nullable List currentFlags = currentBuffer == null ? null : currentBuffer.flags; final int globalSize = globalFlags == null ? 0 : globalFlags.size(); @@ -166,8 +171,7 @@ public void clear() { FeatureFlagEntry currentEntry = currentFlags == null || currentIndex < 0 ? null : currentFlags.get(currentIndex); - final @NotNull java.util.Map uniqueFlags = - new java.util.LinkedHashMap<>(maxSize); + final @NotNull Map uniqueFlags = new LinkedHashMap<>(maxSize); // check if there is still room and remaining items to check while (uniqueFlags.size() < maxSize @@ -177,66 +181,59 @@ public void clear() { @Nullable ScopeType selectedBuffer = null; // choose newest entry across all buffers - if (globalEntry != null && (entryToAdd == null || globalEntry.nanos > entryToAdd.nanos)) { + if (globalEntry != null) { entryToAdd = globalEntry; selectedBuffer = ScopeType.GLOBAL; } if (isolationEntry != null - && (entryToAdd == null || isolationEntry.nanos > entryToAdd.nanos)) { + && (entryToAdd == null || isolationEntry.nanos >= entryToAdd.nanos)) { entryToAdd = isolationEntry; selectedBuffer = ScopeType.ISOLATION; } - if (currentEntry != null && (entryToAdd == null || currentEntry.nanos > entryToAdd.nanos)) { + if (currentEntry != null && (entryToAdd == null || currentEntry.nanos >= entryToAdd.nanos)) { entryToAdd = currentEntry; selectedBuffer = ScopeType.CURRENT; } - if (entryToAdd != null) { - // no need to update existing entries since we already have the latest - if (!uniqueFlags.containsKey(entryToAdd.flag)) { - uniqueFlags.put(entryToAdd.flag, entryToAdd); - } - - // decrement only index of buffer that was selected - if (ScopeType.CURRENT.equals(selectedBuffer)) { - currentIndex--; - currentEntry = - currentFlags != null && currentIndex >= 0 ? currentFlags.get(currentIndex) : null; - } else if (ScopeType.ISOLATION.equals(selectedBuffer)) { - isolationIndex--; - isolationEntry = - isolationFlags != null && isolationIndex >= 0 - ? isolationFlags.get(isolationIndex) - : null; - } else if (ScopeType.GLOBAL.equals(selectedBuffer)) { - globalIndex--; - globalEntry = - globalFlags != null && globalIndex >= 0 ? globalFlags.get(globalIndex) : null; - } - } else { - // no need to look any further since lists are sorted and we could not find any newer - // entries anymore + if (entryToAdd == null) { break; } + + // no need to update existing entries since we already have the latest + if (!uniqueFlags.containsKey(entryToAdd.flag)) { + uniqueFlags.put(entryToAdd.flag, entryToAdd); + } + + // decrement only index of buffer that was selected + if (selectedBuffer == ScopeType.CURRENT) { + currentIndex--; + currentEntry = + currentFlags != null && currentIndex >= 0 ? currentFlags.get(currentIndex) : null; + } else if (selectedBuffer == ScopeType.ISOLATION) { + isolationIndex--; + isolationEntry = + isolationFlags != null && isolationIndex >= 0 + ? isolationFlags.get(isolationIndex) + : null; + } else if (selectedBuffer == ScopeType.GLOBAL) { + globalIndex--; + globalEntry = globalFlags != null && globalIndex >= 0 ? globalFlags.get(globalIndex) : null; + } } // Convert to list in reverse order (oldest first, newest last) final @NotNull List resultList = new ArrayList<>(uniqueFlags.values()); Collections.reverse(resultList); - return new FeatureFlagBuffer(maxSize, new CopyOnWriteArrayList<>(resultList)); + return new FeatureFlagBuffer(maxSize, Collections.unmodifiableList(resultList)); } private static class FeatureFlagEntry { private final @NotNull String flag; private final boolean result; + private final long nanos; - @SuppressWarnings("UnusedVariable") - @NotNull - private final Long nanos; - - public FeatureFlagEntry( - final @NotNull String flag, final boolean result, final @NotNull Long nanos) { + public FeatureFlagEntry(final @NotNull String flag, final boolean result, final long nanos) { this.flag = flag; this.result = result; this.nanos = nanos; diff --git a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt index 8ec18ce02b8..ad690634bd6 100644 --- a/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt +++ b/sentry/src/test/java/io/sentry/featureflags/FeatureFlagBufferTest.kt @@ -1,6 +1,9 @@ package io.sentry.featureflags +import com.google.common.truth.Truth.assertThat import io.sentry.SentryOptions +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -347,4 +350,51 @@ class FeatureFlagBufferTest { val featureFlags = buffer.featureFlags assertNotNull(featureFlags) } + + @Test + fun `clone does not share later writes with the original`() { + val buffer = FeatureFlagBuffer.create(SentryOptions().also { it.maxFeatureFlags = 5 }) + buffer.add("a", true) + + val clone = buffer.clone() + clone.add("b", true) + buffer.add("c", true) + + assertThat(buffer.featureFlags!!.values.map { it!!.flag }).containsExactly("a", "c").inOrder() + assertThat(clone.featureFlags!!.values.map { it!!.flag }).containsExactly("a", "b").inOrder() + } + + @Test + fun `merging is safe while another thread adds flags`() { + val options = SentryOptions().also { it.maxFeatureFlags = 100 } + val globalBuffer = FeatureFlagBuffer.create(options) + val isolationBuffer = FeatureFlagBuffer.create(options) + val currentBuffer = FeatureFlagBuffer.create(options) + + val stop = AtomicBoolean(false) + val writerFailure = AtomicReference(null) + // more distinct names than maxFeatureFlags, so adds mix evictions in with refreshes + val writer = Thread { + try { + var i = 0 + while (!stop.get()) { + globalBuffer.add("flag${i++ % 150}", true) + } + } catch (e: Exception) { + writerFailure.set(e) + } + } + + writer.start() + try { + repeat(10_000) { + FeatureFlagBuffer.merged(options, globalBuffer, isolationBuffer, currentBuffer).featureFlags + } + } finally { + stop.set(true) + writer.join() + } + + assertThat(writerFailure.get()).isNull() + } } From fa382369ec5c39b94555ec4d91620bf57a53154b Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 25 Aug 2026 10:31:41 +0200 Subject: [PATCH 2/2] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02e1a8a906e..9f374f2c4de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - Prevent duplicated breadcrumbs on tombstone-merged native crash events ([#5888](https://github.com/getsentry/sentry-java/pull/5888)) +- Prevent events from being dropped when feature flags are added while an event is being captured ([#5989](https://github.com/getsentry/sentry-java/pull/5989)) - Prevent a class of Session Replay deadlocks by confining lifecycle state changes to Android's main thread ([#5965](https://github.com/getsentry/sentry-java/pull/5965)) ### Performance