[OpenTelemetry] Metrics - Raise min CircularBufferBuckets capacity to 2

[ysolomchenko]

Fixes # N/A
Design discussion issue # N/A

Found by Codex/security scans.

Changes

Base2ExponentialBucketHistogram bucket initialization now rejects invalid maxBuckets values smaller than 2, preventing a possible hang during metrics recording.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.74%. Comparing base (53d590c) to head (af3c762).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7078      +/-   ##
==========================================
+ Coverage   88.72%   88.74%   +0.02%     
==========================================
  Files         270      270              
  Lines       12928    12928              
==========================================
+ Hits        11470    11473       +3     
+ Misses       1458     1455       -3     

Flag	Coverage Δ
unittests-Project-Experimental	88.67% <100.00%> (+<0.01%)	⬆️
unittests-Project-Stable	88.70% <100.00%> (+0.06%)	⬆️

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

Files with missing lines	Coverage Δ
src/OpenTelemetry/Metrics/CircularBufferBuckets.cs	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[reyang]

preventing a possible hang during metrics recording

@ysolomchenko details?

[reyang]

It is unclear what was the problem, and how the change proposed in this PR will solve the problem.

[ysolomchenko]

It is unclear what was the problem, and how the change proposed in this PR will solve the problem.

The problem is that CalculateScaleReduction can enter an infinite loop for a valid input: capacity = 1.

If the range spans negative to non-negative values (e.g., -1 and 0), the loop converges to begin = -1 and end = 0. Further right shifts don’t change these values, so diff stays 1 and the loop condition remains true forever.

The PR fixes this by ensuring that this non-converging case cannot occur (e.g., by disallowing capacity = 1), thus preventing the infinite loop in TryIncrement.

For end users, nothing changes because existing usage already enforces capacity >= 2.

This PR simply adds an extra safeguard to handle the edge case (capacity = 1) and prevent a potential infinite loop, even though it is not reachable in current usage.

[reyang]

It is unclear what was the problem, and how the change proposed in this PR will solve the problem.

The problem is that CalculateScaleReduction can enter an infinite loop for a valid input: capacity = 1.

If the range spans negative to non-negative values (e.g., -1 and 0), the loop converges to begin = -1 and end = 0. Further right shifts don’t change these values, so diff stays 1 and the loop condition remains true forever.

The PR fixes this by ensuring that this non-converging case cannot occur (e.g., by disallowing capacity = 1), thus preventing the infinite loop in TryIncrement.

For end users, nothing changes because existing usage already enforces capacity >= 2.

This PR simply adds an extra safeguard to handle the edge case (capacity = 1) and prevent a potential infinite loop, even though it is not reachable in current usage.

Thanks @ysolomchenko! I have two suggestions:

1. In the PR description or issue, explain the exact problem, following the spirit of https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/.github/ISSUE_TEMPLATE/bug_report.yml and be specific about - how to reproduce the issue, what's the expected result and what's the actual result.
2. Add a test case which would fail before the change and would pass after the change, so we avoid the regression in the future.

[martincostello]

In the PR description or issue, explain the exact problem, following the spirit of https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/.github/ISSUE_TEMPLATE/bug_report.yml and be specific about - how to reproduce the issue, what's the expected result and what's the actual result.

I think there's some intentional vagueness being applied to pull requests being raised at the moment ("Found by Codex/security scans.").

[reyang]

In the PR description or issue, explain the exact problem, following the spirit of https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/.github/ISSUE_TEMPLATE/bug_report.yml and be specific about - how to reproduce the issue, what's the expected result and what's the actual result.

I think there's some intentional vagueness being applied to pull requests being raised at the moment ("Found by Codex/security scans.").

I disagree wearing my SIG security maintainer hat.
If this is indeed a security issue, we should use security advisory and follow https://github.com/open-telemetry/sig-security/blob/main/security-response.md, and depending on how sensitive it is, the fix might need to happen in a private fork.

[martincostello]

I can't speak to that specifically - the first I heard about this particular PR was when it was opened.

[reyang]

I can't speak to that specifically - the first I heard about this particular PR was when it was opened.

No worries, I'm here to help.

Here is my recommendation:

1. If this seems to be a real security issue, we (maintainers + folks who reported the issue + folks who has the best context to provide a fix) should understand the exact problem, determine the severity (e.g. CVSS score) and decide how to fix it (publicly, privately) and communicate it (publish CVE directly, catch a monthly release train without declaring CVE, work with major customers to apply a private fix before making it public).
2. If this is not a real security issue, but is an improvement for general goodness, we should be very explicit about it in PR/issue/changelog.

[Kielek]

@reyang, it is not a security issue. It was detected during the Codex/security scans.
All production constructors usage are Guarded by 2 (except maybe some reflection stuff, but such scenarios are not supported).

opentelemetry-dotnet/src/OpenTelemetry/Metrics/MetricPoint/Base2ExponentialBucketHistogram.cs

Lines 90 to 95 in 8934056

	Guard.ThrowIfOutOfRange(maxBuckets, min: 2);
	this.Scale = scale;
	this.PositiveBuckets = new CircularBufferBuckets(maxBuckets);
	this.NegativeBuckets = new CircularBufferBuckets(maxBuckets);
	}

IMO tests after changes are good enough, as it cover -1,0,1 with exception expectations.. It should prevent us with any unihtentional changes in the future.

@ysolomchenko, could you please extend description with such information? With this, we could proceed and merge changes IMO.

[reyang]

@reyang, it is not a security issue. It was detected during the Codex/security scans. All production constructors usage are Guarded by 2 (except maybe some reflection stuff, but such scenarios are not supported).

opentelemetry-dotnet/src/OpenTelemetry/Metrics/MetricPoint/Base2ExponentialBucketHistogram.cs

Lines 90 to 95 in 8934056

	Guard.ThrowIfOutOfRange(maxBuckets, min: 2);
	this.Scale = scale;
	this.PositiveBuckets = new CircularBufferBuckets(maxBuckets);
	this.NegativeBuckets = new CircularBufferBuckets(maxBuckets);
	}

IMO tests after changes are good enough, as it cover -1,0,1 with exception expectations.. It should prevent us with any unihtentional changes in the future.

@ysolomchenko, could you please extend description with such information? With this, we could proceed and merge changes IMO.

Got it, thank you very much!

[reyang]

LGTM.

[github-actions]

Thank you for your contribution @ysolomchenko! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.