Fix TraceStateBuilder.remove double-counting a repeated removal

[TimurRakhmatullin86]

Problem

ArrayBasedTraceStateBuilder.remove(key) decrements numEntries unconditionally, even when the key's slot is already a null tombstone from a previous remove of the same key. put() guards the symmetric update (if (currentValue == null) { numEntries++; }); remove() has no mirror guard, so a repeated remove of an already-removed key double-decrements the count.

Because build() trusts numEntries, a repeated removal corrupts the result three different ways:

// 1) silent data loss — a valid entry disappears
TraceState.builder().put("a","1").put("b","2").remove("a").remove("a").build();
// numEntries: 2 -> 1 -> 0  =>  build() returns empty();  get("b") == null

// 2) null-valued entry — violates the non-null value contract
TraceState.builder().put("a","1").remove("a").remove("a").build();
// numEntries: 1 -> 0 -> -1 => size()==2 fast path returns [a, null]; size()==1, get("a")==null

// 3) crash
TraceState.builder().put("a","1").put("b","2").put("c","3").remove("a").remove("a").build();
// numEntries under-counts => entries[] is too small => ArrayIndexOutOfBoundsException

The remove javadoc says it removes the entry "if it is present", so a second remove of a key that is no longer present must be a no-op — the same as removing a key that was never added (which removeNotPresent already covers).

Fix

Only account for the removal (set the tombstone and decrement) when the entry is still present, mirroring the guard in put().

Tests

Added three cases in TraceStateTest for the data-loss, null-value, and ArrayIndexOutOfBoundsException paths. All three fail against the current code and pass with the fix; the existing TraceStateTest cases stay green.

[opentelemetry-pr-dashboard]

Pull request dashboard status

Waiting on reviewers · refreshed 2026-09-02 14:36 UTC

Review the latest changes.

Also blocked by: Merge conflicts.

Status above doesn't look right?

• Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
• Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

[jack-berg]

This test is largely redundant with removeAlreadyRemovedKeyKeepsOtherEntries. Let's drop it.

[jack-berg]

Nit: this comment can probably be tightened. The symmetry with put() is visible a few lines up, and the failure modes are documented in the PR description / tests. Something like:

// Mirror the tombstone guard in put(): skip already-removed entries so numEntries stays accurate.

[TimurRakhmatullin86]

Thanks for the review, @jack-berg! Going to address your comments I realized this is a duplicate: the same if (reversedEntries.get(i + 1) != null) guard in remove() already landed in #8613 back in July, along with the removeSameKeyTwice* regression tests that cover the same scenarios as the ones here — my branch predated that and I didn't catch the overlap. So there's nothing left for this to add. Closing as redundant; sorry for the noise, and thanks for taking the time to look.