[Kielek]

Follow-up to #7779.

Changes

Fixes a TOCTOU race in the metric cardinality-limit fast paths.

For delta aggregation, a recorder could miss a tag mapping while another
thread dequeued the final available MetricPoint and published that same
mapping. The first recorder could then observe an empty queue and route its
measurement to otel.metric.overflow without performing the authoritative
locked dictionary lookup.

The cumulative path identified in #7779 as having a similar early return had
an analogous stale-lookup race once its metric-point index reached the full
sentinel.

This change treats the unlocked capacity checks as hints. When a store appears
full, the tag mapping and capacity are rechecked under the existing creation
lock before the measurement is sent to overflow. The normal non-capacity path
is unchanged.

Adds deterministic concurrency coverage for:

• Delta and cumulative aggregation.
• Single-tag and multi-tag lookups.
• long and double measurements.

No public API changes.

Testing

CI

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated
• Appropriate CHANGELOG.md
• [ ] Changes in public API reviewed (not applicable)

[Kielek]

@richardcocks, @martincostello as you were involved in previous PR.

Detected by internal Splunk/Cisco Codex scans.

[opentelemetry-pr-dashboard]

Pull request dashboard status

Merged · refreshed 2026-10-05 12:45 UTC

Status above doesn't look right?

• Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

[richardcocks]

I was aware of this, but it seemed equivalent to racing the lock in the first place, the effect is that it would see it as full, so while strictly a TOCTOU it fails safe ( and fails equivalent to winning the race to the lock in the first place).

Is my understanding not correct, or is avoiding a harmless TOCTOU in principle more important than avoiding a second check on every invocation?

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the multi-tag coverage gap and replace the banned Meter(string) constructor.

Review effort: Lite
Findings: None

What changed in this PR

This pull request fixes TOCTOU races in metric cardinality-limit checks.

Changes:

• Adds synchronized mapping and capacity rechecks.
• Adds deterministic concurrency regression tests.
• Documents the fix in the changelog.

File	Summary
test/​OpenTelemetry.Tests/​Metrics/​MetricPointReclaimTests.cs	Adds concurrency regression coverage.
src/​OpenTelemetry/​Metrics/​AggregatorStore.cs	Adds locked mapping and capacity rechecks.
src/​OpenTelemetry/​CHANGELOG.md	Records the race-condition fix.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.21%. Comparing base (f9dd754) to head (3c8320d).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7804      +/-   ##
==========================================
+ Coverage   92.19%   92.21%   +0.01%     
==========================================
  Files         363      363              
  Lines       20850    20851       +1     
==========================================
+ Hits        19222    19227       +5     
+ Misses       1628     1624       -4     

Flag	Coverage Δ
unittests-Project-Experimental	92.32% <100.00%> (+0.01%)	⬆️
unittests-Project-Stable	92.28% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/OpenTelemetry/Metrics/AggregatorStore.cs	92.08% <100.00%> (+1.32%)	⬆️

[martincostello]

So does this effectively undo #7779, or is it fixing an existing bug that the change from that PR surfaced?

[richardcocks]

@martincostello

It's not quite either, a second check I hope can be put inside without undoing most the performance benefits, but I'll need to re-run benchmarks to confirm that.

Additionally, this may just need a comment explaining why a second check isn't carried out, if the TOCTOU can be reasoned as harmless as I suspect.

I'll try to draw some sequence diagrams to better communicate my understanding, and then we can see if we can all agree on the best solution pending the performance impact of a second check, which may be minimal.

Keeping the first check outside the lock should still improve performance in the majority of cases, but I will try to set up racy benchmarks to confirm.

[richardcocks]

I've now had the chance to better analyze the PR, and I understand the race condition better, it's a race that can occur trying to write the same tag set, where one succeeds and one fails, ending up with a split behaviour of one thread writing to a slot and another failing to do so.

Given that, I agree it's a genuine correctness bug. I'm trying to write benchmarks to see the impact on the delta path, there will still be reduced allocation from #7779, this doesn't reverse that gain, but it will no longer be an entirely lock-free rejection.

On the cumulative path, which #7779 didn't change, this PR will introduce a lock to a previously lock-free rejection, so I will also attempt to benchmark the impact of this on that path too.

[richardcocks]

I am finding it difficult to produce reliable benchmarks that aren't very noisy, especially on the cumulative path.

I'm seeing some runs where the lock contention is reducing throughput by 5-10%, which might be worth the correctness, but then I'm seeing other runs where the throughput is hammered by 25% on a quieter machine and possibly even up to 75% reduction in throughput on a noisy machine, which is concerning but hard to reproduce.

My benchmarking machine is my desktop, so it's not a clean uncontended box, and due to the nature of this testing of the contended results, it relies on some specific multi-thread and noisy core behaviour, so it's difficult to reproduce clean numbers, this isn't something that will surface from a well-targeted BDN micro-benchmark.

I have also run BDN benchmarks to confirm that allocations don't regress with this fix, but that isn't where the source of the potential slow-down would be, nor would it be the single-threaded lock acquisition time, which is a handful of nanoseconds.

Benchmark results below when from when I re-ran while otherwise not using my computer. Note that the delta-path is still an improvement over the pre #7779 main, because it has similar behaviour without the allocations, but the tail behaviour of the cumulative path needs a more rigorous benchmark by someone more qualified, as well as a product call about the consequences and likelihood of hitting this race condition, because introducing a lock into a previously lock-free path can have severe cost penalties.

I am looking at whether we can use a lock-free recheck to avoid the worst lock-convey behaviour which is the suspect behind some runs coming out dramatically worse throughput than others, while still passing the tests added in this PR.

Benchmark results below

Please treat theses as slightly suspect or indicative, rather than hard-numbers.

Single-thread BenchmarkDotNet (Mean ns, base 44d841f; launchCount 7, warmup 10, iter 15, MemoryDiagnoser)

Path	A: pre-#7779	B: #7779	C: #7804	C − B	Alloc A / B / C
Delta — existing tag (control)	62.9 ns	62.7 ns	63.1 ns	~0	0 / 0 / 0
Delta — unseen, 1 tag	141.5 ns	100.2 ns	108.5 ns	+8.2 ns	40 B / 0 / 0
Delta — unseen, 2 tags	340.2 ns	208.9 ns	214.0 ns	+5.1 ns	112 B / 0 / 0
Cumulative — existing (control)	50.6 ns	51.0 ns	51.0 ns	~0	0 / 0 / 0
Cumulative — unseen, 1 tag	92.6 ns	96.3 ns	104.9 ns	+8.6 ns	0 / 0 / 0
Cumulative — unseen, 2 tags	200.0 ns	199.0 ns	210.3 ns	+11.3 ns	0 / 0 / 0

StdDev ≤ 8–9 ns on every overflow row. A = pre-#7779 (no pre-check), B = #7779 (unlocked pre-check), C = #7804 (pre-check + locked recheck).

Contention throughput, Delta (Mops/s, median of 6× 8s runs, High priority; higher = better)

Tags	Threads	A pre-#7779	B #7779	C #7804	C vs B
1	1	7.19	10.30	8.75	−15%
1	2	9.95	13.94	11.81	−15%
1	4	11.71	13.82	12.96	−6%
1	6	7.60	11.61	10.38	−11%
1	8	10.53	12.03	10.97	−9%
2	1	3.00	4.94	4.73	−4%
2	2	5.28	8.42	7.68	−9%
2	4	9.11	13.57	11.59	−15%
2	6	7.50	10.37	7.51	−28%
2	8	9.35	11.30	10.34	−9%

Delta allocation: A allocates per dropped measurement; B and C both 0 MB.

Contention throughput, Cumulative (Mops/s, median of 6× 8s runs, High priority; higher = better)

Tags	Threads	A pre-#7779	B #7779	C #7804	C vs B
1	1	11.44	11.59	9.84	−15%
1	2	15.09	15.05	11.71	−22%
1	4	14.59	14.71	14.32	−3%
1	6	12.09	12.02	11.11	−8%
1	8	11.64	11.72	8.85	−25%
2	1	5.25	5.25	4.84	−8%
2	2	8.88	8.71	7.81	−10%
2	4	13.45	13.48	12.56	−7%
2	6	10.84	11.01	10.20	−7%
2	8	10.11	10.05	8.13	−19%

The cumulative overflow path was lock-free in both A and B; #7804 (C) adds a lock to it. Every cell regresses.

Tail behaviour at 8 threads, 2 tags (Mops/s, 12 runs; higher = better)

Arm	Path	Mean	Median	Min	Max	Perf Collapses (<6Mop/s)
A pre-#7779	Delta	8.03	8.47	3.44	10.66	2 / 12 (GC pauses — A allocates)
B #7779	Delta	11.66	11.12	10.85	15.20	0 / 12
C #7804	Delta	9.02	9.96	3.42	10.99	2 / 12
A pre-#7779	Cumulative	9.20	9.11	8.75	10.36	0 / 12
B #7779	Cumulative	9.68	9.22	8.90	12.79	0 / 12
C #7804	Cumulative	7.07	7.68	3.63	9.03	2 / 12

C (#7804) vs B (#7779): Delta mean −22.7% / median −10.4% ; Cumulative mean −27.0% / median −16.7%.

[Kielek]

@richardcocks, I have tried to find lock-free mechanism. Unfortunately, no cusses. I know that current performance is worse then #7779, but we are in better place than before your changes (for most cases). I think that it can be reasonable to fix this, but looking for any other feedback you can share.

[richardcocks]

I'm happy whatever the outcome here, and whether the lock is worth the performance trade-off for correctness isn't a call I can make.

Thank you, and I apologise if my PR has kicked up some latent issues.

Although following Martin's advice in his talk a couple of weeks ago, I shouldn't feel guilty! Great talk, I very much enjoyed it.