Fix TraceStateBuilder.remove corrupting the builder when the same key is removed twice - #8613
Conversation
184a512 to
0b8dc2f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8613 +/- ##
============================================
- Coverage 91.64% 91.64% -0.01%
Complexity 10348 10348
============================================
Files 1013 1013
Lines 27380 27381 +1
Branches 3218 3219 +1
============================================
Hits 25092 25092
Misses 1558 1558
- Partials 730 731 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… is removed twice
0b8dc2f to
d24559d
Compare
Pull request dashboard statusMerged · refreshed 2026-07-29 22:32 UTC Status above doesn't look right?
|
|
Thank you for your contribution @thswlsqls! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
| } | ||
|
|
||
| @Test | ||
| void removeTwice() { |
There was a problem hiding this comment.
Nit: rename to removeSameKeyTwice() - this could apply to all the added tests since all the added tests are removing the same key.
There was a problem hiding this comment.
Thanks for the review. You're right that all three added tests remove the same key, so removeSameKeyTwice* reads better. Since this was already merged, I'll fold the rename into a follow-up PR.
| .put(FIRST_KEY, FIRST_VALUE) | ||
| .put(SECOND_KEY, SECOND_VALUE) | ||
| .put(THIRD_KEY, THIRD_VALUE) | ||
| .remove(FIRST_KEY) |
There was a problem hiding this comment.
Nit: just to add variety, remove the SECOND_KEY twice here.
There was a problem hiding this comment.
Good point — right now every double-remove test matches on the first entry, so removing SECOND_KEY here would exercise a later slot in the scan loop. I'll include it in the same follow-up.
Follow-up to review comments on open-telemetry#8613, which were submitted after that PR had already been merged. Rename removeTwice, removeTwice_KeepsRemainingEntry and removeTwice_KeepsRemainingEntries to removeSameKeyTwice*, since all three remove the same key and the previous names did not say so. removeSameKeyTwice_KeepsRemainingEntries now removes SECOND_KEY rather than FIRST_KEY. ArrayBasedTraceStateBuilder.remove scans entries from the front, so removing the first key always matches on the first iteration; removing a middle key covers a later slot for the first time. Test-only: no production code, public API or asserted behavior changes.
Fixes #8612
Description
ArrayBasedTraceStateBuilder.remove()decrementednumEntrieswithout checking whether the value was alreadynull, so removing a key twice drove the counter below the real count.build()trustsnumEntriesin three places, so the drift leaks a{a=null}entry, silently drops live entries, or throwsArrayIndexOutOfBoundsException.put()(same class, line 74-78), which already does this check; first removal is unchanged.TraceStateBuilderis stable public API for external instrumentation and vendor propagators. The silent data loss contradicts the no-op contract of theremove(String)Javadoc ("if it is present") and ofremoveNotPresent(), and is reachable viareuseBuilder().PooledHashMapdropping live entries).Testing done
TraceStateTest#removeTwice,#removeTwice_KeepsRemainingEntry,#removeTwice_KeepsRemainingEntriesfor one, two, and three entries. All three fail without the fix; 41 pass with it../gradlew :api:all:check— passed, 504 tests.CHANGELOG.md## Unreleased→### APIentry added.