FeatureFlagBuffer.merged() can throw and drop events under concurrency

[runningcode]

sentry/src/main/java/io/sentry/featureflags/FeatureFlagBuffer.java

Bug

merged() claims to snapshot the incoming buffers:

// Capture references to avoid inconsistencies from concurrent modifications
final @Nullable CopyOnWriteArrayList<FeatureFlagEntry> globalFlags = globalBuffer.flags;

That comment isn't true. flags is assigned only in the three constructors and never reassigned, so capturing the reference yields the same live list, not a snapshot. CopyOnWriteArrayList makes each individual size()/get(i) atomic, but gives nothing across the ~maxSize calls the merge loop makes.

add() mutates in three separate steps — remove(i), add(...), remove(0) — so the list transiently shrinks. A merging thread that reads size() == n and then calls get(n-1) while a writer sits between its remove and its add gets an ArrayIndexOutOfBoundsException. Indexing a shifting list can also duplicate or skip entries.

This is reachable: the global-scope buffer is shared across all threads, and merged() runs on the error-capture path (SentryClient#applyScope → CombinedScopeView#getFeatureFlagBuffer). Scopes.captureEventInternal catches Throwable, so it won't crash the host app — but the event is silently dropped, including events from the uncaught-exception handler.

clone() and getFeatureFlags() are fine: the CopyOnWriteArrayList(Collection) constructor and the COW iterator both take a real atomic snapshot. Only merged() indexes.

Related cleanup in the same class

• volatile on flags is dead weight — the field is never reassigned. It costs a fence per read and, worse, signals a snapshot-swap design that isn't there (likely how the misleading comment above arose).
• maxSize is neither final nor volatile despite only being set in constructors. As written, a thread obtaining a FeatureFlagBuffer through a data race can legally observe maxSize == 0. final gives the freeze guarantee for free — same for flags.
• CopyOnWriteArrayList + lock is redundant: every mutation already holds the lock, so COW's write-side atomicity is never needed. We pay for it three times per add() — with the default maxFeatureFlags = 100, that's three 100-element array copies per flag evaluation.
• FeatureFlagEntry.nanos is a boxed @NotNull Long — an extra allocation per entry, unboxed twice per merge-loop comparison. Should be a primitive long.
• @SuppressWarnings("UnusedVariable") on nanos is stale; merged() reads it.
• getFeatureFlags() is annotated @Nullable but can never return null.
• Merge tie-break favors GLOBAL > ISOLATION > CURRENT (strict >, so whichever is tested first wins equal timestamps). The more specific scope should win; changing isolation's and current's comparison to >= fixes it.

Proposed fix

Make the snapshot real: an immutable list behind a volatile field, swapped under the existing lock — the design the current code already gestures at.

private volatile @NotNull List<FeatureFlagEntry> flags = Collections.emptyList();
private final int maxSize;

@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 @NotNull List<FeatureFlagEntry> updated = new ArrayList<>(flags.size() + 1);
    for (final @NotNull FeatureFlagEntry entry : flags) {
      if (!entry.flag.equals(flag)) {
        updated.add(entry);
      }
    }
    updated.add(new FeatureFlagEntry(flag, result, System.nanoTime()));
    if (updated.size() > maxSize) {
      updated.remove(0);
    }
    flags = Collections.unmodifiableList(updated);
  }
}

What it buys:

• merged() becomes correct — one volatile read per buffer yields a list that can never change, so the index arithmetic is trivially safe and the existing comment becomes accurate.
• clone() gets cheaper than today: new FeatureFlagBuffer(other.maxSize, other.flags) shares the immutable list with zero copying. Since the class doc calls clone the optimized path, this is the one that matters.
• add() drops from three array copies to one.
• clear() becomes flags = Collections.emptyList(); CopyOnWriteArrayList disappears from the class entirely.

Invariant to preserve: correctness now rests on every write holding the lock (they are read-modify-write), while reads need no lock at all.

Add a test that hammers add() from one thread while another calls merged().

Notes

• Left AutoClosableReentrantLock alone — it is the repo idiom.
• Deliberately not swapping the list for a LinkedHashMap: that would make add() O(1) but force clone() to copy, trading away the property this class exists to optimize.
• Optional: replace System.nanoTime() with a static AtomicLong sequence to get a total order across buffers with no ties and no dependence on clock granularity.

[linear-code]

JAVA-704