fix(sdk/metric): keep concurrent float sums consistent

[jstar0]

Fixes #8779

The float64 metric sum counter keeps whole and fractional values in separate atomics. Cumulative collection can combine values from different points in time and export a sum that was never observed.

The generation validation added in the earlier revision rejects fractional ABA snapshots, but waiting for all fractional writers to become idle can prevent collection from completing while measurements continue. This update keeps the lock-free snapshot fast path. After three unsuccessful attempts, a float64 load serializes fallback collectors, freezes new fractional writes, waits only for writes that had already entered, captures a stable fractional value with the integer atomic, and then releases writers. Whole-number additions retain the existing integer atomic fast path.

Tests:

• make test/./sdk/metric ARGS=-race TIMEOUT=60
• go test -timeout 60s -race ./internal/aggregate -run '^TestAtomicCounterLoadMakesProgressWithFractionalContention$' -count=100 from sdk/metric
• go vet ./internal/aggregate from sdk/metric
• make precommit completes generation, module tidy, lint, README, and module verification locally. Its test stage stops at bridge/opentracing TestBridgeTracer_ExtractAndInject_gRPC with a gRPC server-preface EOF; the same failure is reproducible on the prior upstream baseline and is unrelated to this change.

Benchmarking on Apple M4 Pro with GOMAXPROCS=8 reports 0 B/op and 0 allocs/op for all existing atomic-counter benchmarks. The added concurrent fractional-add benchmark also reports 0 B/op and 0 allocs/op across ten samples.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.5%. Comparing base (5bac29f) to head (8d0ff1f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #8848   +/-   ##
=====================================
  Coverage   88.5%   88.5%           
=====================================
  Files        331     331           
  Lines      21012   21037   +25     
=====================================
+ Hits       18602   18626   +24     
- Misses      2410    2411    +1     

Files with missing lines	Coverage Δ
sdk/metric/internal/aggregate/atomic.go	94.8% <100.0%> (+1.4%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MrAlias]

The PR adds retry logic around concurrent fractional writes and a regression test for the positive-only sequence, but the snapshot validation remains vulnerable when fractional updates return to a prior bit pattern.

[jstar0]

Addressed in 7d19e3b. Added an atomic fractional-write generation that increments after every successful fractional CAS and is validated alongside nFloatBits and writerState during load. This rejects ABA snapshots where a negative fractional write restores the prior bit pattern. Added TestAtomicCounterLoadConcurrentSnapshotCancellation for the .5, 1, 1, -.5 sequence, and updated the subnormal fixture to model its three individual writes. Local verification: go test ./... in sdk/metric, targeted aggregate tests, targeted aggregate -race, go vet, and git diff --check all pass. The new head has restarted hosted CI; please take another look when convenient.

[jstar0]

Follow-up in 74fbb54 bounds the cancellation stress loop to 1,000,000 sequences. The previous 10,000,000 iteration count exceeded the repository's 60s per-test timeout under hosted -race; the hosted failure was a timeout, not a race report. The same Makefile target now passes locally: make test/./sdk/metric ARGS=-race TIMEOUT=60, and the full sdk/metric module race suite passes. The ABA coverage remains unchanged.

[MrAlias]

The generation change resolves the ABA issue from my earlier review. I missed a separate progress constraint in the original approach: cumulative collection still needs to complete while measurements continue.

[jstar0]

Follow-up for the collection-progress feedback:

• The bounded lock-free snapshot path remains unchanged for uncontended loads.
• After three failed attempts, fallback collectors are serialized; a low-bit freeze marker prevents new fractional writes from entering, while the remaining bits count already-entered writers. Collection waits only for that count to drain, then reads the stable fractional and integer portions.
• Added TestAtomicCounterLoadMakesProgressWithFractionalContention, synchronized so both writers have started, plus BenchmarkAtomicCounterFractionalAdd.
• The branch includes current main via a signed merge commit and the changelog entry is back under Unreleased; the released changelog guard now passes.

Local evidence: make test/./sdk/metric ARGS=-race TIMEOUT=60, the progress regression under -race -count=100, go vet ./internal/aggregate, and targeted snapshot/ABA tests pass. make precommit reaches the full race test stage; the only local failure is the existing bridge/opentracing gRPC server-preface EOF, reproduced on the prior upstream baseline.

[jstar0]

Resolved the CHANGELOG.md conflict with current main via signed merge commit 7a1b3fb5d. Both Unreleased entries are kept, no implementation files changed, and ./verify_released_changelog.sh main passes. The branch is mergeable again.

[dashpole]

I have concerns with the writer-side spinning (runtime.Gosched()) in the fallback path. Under high concurrent load, writers that arrive while a snapshot is frozen will repeatedly execute atomic increments, rollbacks, and yields. This causes cache-line bouncing on fractionalWriteState, delays in-flight writes from finishing, and risks P99 latency spikes on application recording threads.

Alternative: Follow cumulativeHistogram with hotColdWaitGroup

Instead of embedding freeze bits and generation tracking inside atomicCounter, we could follow how cumulativeHistogram works by using a per-series hotColdWaitGroup and [2] buffers only for float64 sums:

• measure(): hotIdx := hcwg.start(); s.counters[hotIdx].add(val); hcwg.done(hotIdx)
• collect(): readIdx := hcwg.swapHotAndWait(), read cold value, merge delta into hot buffer (s.counters[hotIdx].add(val)), and reset cold buffer.

Tradeoffs:

• Writer Predictability: Writers never spin or yield—swapHotAndWait() is one-sided and only the background collector waits.
• Fractional Writes (0.5): hotColdWaitGroup is marginally (~12%) faster on the hot path (176 ns/op vs 200 ns/op for me) because it uses 1 fewer atomic operation per write.
• Whole-Number Floats (1.0): The current PR is significantly faster (22 ns/op vs 52 ns/op), but users typically choose Float64 instruments specifically for fractional measurements.
• int64 Sums: Completely unaffected (retains single atomic.Int64 fast path with zero hcwg overhead).
• Simplicity: Reuses our existing hotColdWaitGroup architecture rather than maintaining a bespoke atomic state machine.

[jstar0]

Thanks for the detailed performance analysis. I agree that yielding on an application-recording path is an undesirable tradeoff.

I checked the current ownership before changing the design: atomicCounter is also used for exponential-histogram sums, whose cumulative collection can call load concurrently with record. The proposed per-series hot/cold layout is a good fit for cumulative sums, but applying it only there would leave that shared exponential-histogram path on the old split-counter snapshot behavior. Moving the layout into the shared counter instead would broaden the change beyond the current sum-focused issue.

Could you confirm which scope you prefer?

1. Rework cumulative float sums around hotColdWaitGroup, and handle exponential-histogram snapshot consistency separately; or
2. Keep the shared counter correctness boundary, but replace the writer-freeze/yield mechanism with a design that leaves writers non-blocking across all current users.

I will keep the current approved branch unchanged until that boundary is clear, rather than replace the hot path with a broader design without confirmation.