fix(metrics): export-only clamp for buffer statistics (#5303)

[VedantMadane]

Which issue(s) this PR fixes:
Fixes #5303

What this PR does / why we need it:
Buffer size gauges (stage_byte_size, queue_byte_size, and derived total_queued_size) can go transiently negative when Fluentd core under/over-subtracts during concurrent stage/queue transitions (the deferred @stage_size_metrics.add after chunk unlock vs enqueue_chunk's sub — see #2712 / #2734). Those values are mirrored by the Prometheus plugin as fluentd_output_status_buffer_total_bytes / fluentd_output_status_buffer_stage_byte_size, which is what #5303 reports.

Clamping the gauge store on sub/dec is the wrong fix: it turns a self-correcting transient negative into a permanent over-count, so Buffer#storable? (which reads the raw gauge) eventually refuses every write. Thanks @Watson1978 for catching that.

This PR takes an export-only approach:

1. Leave LocalMetrics gauge sub/dec/set semantics unchanged (negatives still allowed so the deferred-add race can self-heal).
2. Clamp stage/queue sizes to >= 0 only when building statistics (the path Prometheus and the monitor agent consume).
3. Clamp available_buffer_space_ratios to [0, 100] when counters overshoot total_limit_size, and treat total_limit_size == 0 as 0% free without dividing (no NaN / no dead NaN guard).

Docs Changes:
None

General Checklist:

• I have read and followed CONTRIBUTING.md
• tests pass (CI)
• DCO signed-off

Tests:

• test/plugin/test_buffer.rb #statistics: negative underlying gauges export as 0; overshoot clamps ratio to 0; total_limit_size == 0 does not raise and ratio stays finite.

[Watson1978]

Thanks for digging into #5303. However, I think clamping at zero is the wrong direction for the buffer counters: it converts a transient, self-healing negative into a permanent over-count.

Buffer#write intentionally defers the add. As part of the #2712 fix, staged_bytesizes_by_chunk is accumulated while each chunk is locked, but @stage_size_metrics.add(bytesize) only runs after the chunk locks are released (chunk.mon_exit at lib/fluent/plugin/buffer.rb:386 / :400, and the deferred add at :416).

That leaves a window in which the enqueue thread can run enqueue_chunk → @stage_size_metrics.sub(chunk.bytesize) (buffer.rb:504) while stage_size is still 0.

Take a 100-byte chunk in that window:

• Before this PR: the store goes to -100, then the pending add(100) brings it back to the correct 0. The negative reading is transient and self-correcting — ugly in the export, but the accounting converges.
• After this PR: sub clamps the store to 0, then the pending add(100) leaves stage_size == 100 while the real staged size is 0. The +100 never decays.

Under load this repeats on every flush cycle and accumulates. Note that storable? (buffer.rb:300) reads the raw .get, not the clamped statistics value:

@total_limit_size > @stage_size_metrics.get + @queue_size_metrics.get

So an effectively empty buffer eventually fails this check and the output starts raising BufferOverflowError permanently. That would turn a cosmetic metrics problem into a back-pressure / data-loss problem.

Two related points:

1. The negative value is currently the only signal operators have that add/sub pairing broke. Clamping silently removes the alert without removing the drift — an over-subtraction that does not cross zero was never visible to begin with, and after this change nothing is left to alert on.
2. set_gauge is not clamped, so buffer.stage_size = -1 still stores a negative. statistics would report 0 while storable? computes with -1, i.e. the exported number and the number the buffer actually acts on disagree.

I would suggest fixing the add/sub pairing in buffer.rb (the deferred add at :416 versus enqueue_chunk's sub at :504) rather than clamping the generic gauge type. If a defensive clamp for alternate metrics backends is still wanted, doing it once in the stage_size / queue_size accessors (buffer.rb:204-218) would cover statistics, storable? and in_monitor_agent in one place, and it should emit a log.warn so the underlying race stays diagnosable.

One more thing unrelated to the design question: @store = 0 if @store < 0 is now the last expression of the synchronize block, so dec_gauge / sub_gauge return nil on the non-clamped path. m.set(10); m.dec returned 9 before and returns nil now, while inc / add / set still return the value (and the existing tests assert that contract, e.g. assert_equal 1, @m.inc). Worth an explicit @store at the end of the block, plus a return-value assertion in the new tests.

[Copilot]

Pull request overview

This PR addresses negative buffer size gauge values that can surface under concurrent buffer stage/queue transitions, ensuring exported buffer metrics do not report negative byte sizes (notably impacting Prometheus consumers).

Changes:

• Clamp LocalMetrics gauge sub/dec operations so gauge values do not fall below zero.
• Clamp buffer statistics export for stage_byte_size/queue_byte_size to non-negative values.
• Add unit tests to verify gauge sub/dec never produce negative values.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
lib/fluent/plugin/metrics_local.rb	Floors gauge decrements/subtractions at zero to prevent negative gauges in the default local metrics backend.
lib/fluent/plugin/buffer.rb	Ensures exported buffer statistics never publish negative stage/queue byte sizes by clamping at export time.
test/plugin/test_metrics_local.rb	Adds unit tests validating non-negative behavior for gauge sub/dec.

Suppressed comments (1)

lib/fluent/plugin/buffer.rb:925

• available_buffer_space_ratios is described as a ratio of available space, but buffer_space can still go below 0.0 (or above 1.0) if stage/queue counters drift upward beyond @total_limit_size. This would export negative (or >100) ratios; consider clamping the computed ratio to [0.0, 1.0] before publishing.

        buffer_space = 1.0 - ((stage_size + queue_size * 1.0) / @total_limit_size)

---

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

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[VedantMadane]

Addressed in 3fe8d18: no longer uses Array#max/min on the ratio. Zero capacity takes the else branch (0.0); positive denom uses Numeric#clamp on a non-NaN value.

[VedantMadane]

Addressed remaining Copilot note on NaN when total_limit_size is 0: compute the ratio only when denom > 0, clamp with comparisons (not Array#max/min on NaN), and treat zero/invalid capacity as 0.0 free-space ratio. Added a unit test that export does not raise and the ratio is finite.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

lib/fluent/plugin/metrics_local.rb:68

• dec_gauge no longer reliably returns the updated gauge value because the last expression is the conditional clamp (@store = 0 if ...), which returns nil when no clamp happens. This changes behavior vs inc (and the previous dec_gauge implementation) and could break any callers that use the return value. Consider explicitly returning @store after clamping.

This issue also appears on line 80 of the same file.

          # Buffer size / length gauges must never go negative even if a race
          # causes sub/dec to run more times than add/inc (see #5303).
          @store -= 1
          @store = 0 if @store < 0

lib/fluent/plugin/metrics_local.rb:83

• sub_gauge has the same return-value issue as dec_gauge: the clamp line is the last expression, so the method returns nil when no clamp happens. Returning @store keeps gauge operations consistent and avoids surprising behavior changes for callers.

          # Prevent negative values that leak into Prometheus buffer metrics
          # (fluentd_output_status_buffer_total_bytes, etc.). See #5303 / #2712.
          @store -= value
          @store = 0 if @store < 0

[Watson1978]

Clamping sub_gauge/dec_gauge at zero discards a transient negative excursion that used to self-correct, so stage_size stays permanently inflated; Buffer#storable? reads that raw gauge, so the buffer refuses every further write even when it is empty.

[VedantMadane]

Addressed in 3fe8d18: reverted LocalMetrics sub/dec zero-clamp entirely so the deferred-add vs enqueue_chunk-sub race can still self-correct. Export-only clamp remains in Buffer#statistics.

[Watson1978]

Clamping sub_gauge/dec_gauge at zero discards a transient negative excursion that used to self-correct, so stage_size stays permanently inflated; Buffer#storable? reads that raw gauge, so the buffer refuses every further write even when it is empty.

[VedantMadane]

Addressed in 3fe8d18: same as sibling — LocalMetrics clamp removed; raw gauge semantics restored.

[Watson1978]

@store = 0 if @store < 0 is the last expression in the synchronize block, and a false guard evaluates to nil, so dec/sub return nil on the normal (non-clamped) path instead of the new store value.

[VedantMadane]

Addressed in 3fe8d18: clamp (and the bad last-expression return) removed with the revert of LocalMetrics flooring.

[Watson1978]

@store = 0 if @store < 0 is the last expression in the synchronize block, and a false guard evaluates to nil, so dec/sub return nil on the normal (non-clamped) path instead of the new store value.

[VedantMadane]

Addressed in 3fe8d18: clamp (and the bad last-expression return) removed with the revert of LocalMetrics flooring.

[Watson1978]

The denom > 0.0 branch already covers the case this comment describes, so the NaN
guard inside it is dead code.

• total_limit_size == 0 no longer reaches the division at all — it takes the else branch.
• Inside the if, stage_size and queue_size are already floored at 0 by the
  [..., 0].max calls above, and denom is positive and finite. So
  (stage_size + queue_size).to_f / denom is always >= 0 and never NaN, which means
  neither buffer_space.nan? nor buffer_space > 1.0 can ever be true.

I'd drop both and keep just the lower bound:

denom = @total_limit_size.to_f
buffer_space = denom > 0.0 ? (1.0 - (stage_size + queue_size) / denom).clamp(0.0, 1.0) : 0.0

[VedantMadane]

Addressed in 3fe8d18: simplified to your suggested form — denom > 0.0 ? (...).clamp(0.0, 1.0) : 0.0; dead NaN guard dropped.

[VedantMadane]

Thanks @Watson1978 — you were right about the gauge-store clamp.

Addressed in 3fe8d18:

1. Reverted LocalMetrics sub/dec zero-clamping entirely. Flooring the store discarded the self-correcting transient negative from the deferred stage_size add vs enqueue_chunk sub race, leaving a permanent over-count that Buffer#storable? (raw .get) would eventually treat as full. Internal gauge semantics are unchanged again; the nil return-value bug on the non-clamped path is gone with the clamp.

2. Export-only clamp remains in Buffer#statistics via [metrics.get, 0].max for stage_byte_size / queue_byte_size / total_queued_size, so Prometheus still never sees negatives.

3. available_buffer_space_ratios simplified per your suggestion:

   denom = @total_limit_size.to_f
   buffer_space = denom > 0.0 ? (1.0 - (stage_size + queue_size).to_f / denom).clamp(0.0, 1.0) : 0.0

   Dead NaN guard inside the denom > 0 branch removed.

4. Dropped the LocalMetrics floor unit tests; kept / clarified the statistics export tests (negative gauges, overshoot ratio, total_limit_size == 0).

I did not move the clamp into the stage_size / queue_size accessors or fix the deferred-add pairing in this PR — export-only keeps the cosmetic metrics fix without changing storable? behavior. Happy to follow up on the race itself in a separate change if that is preferred.