Skip to content

libbeat/diskqueue: reuse decoder buffer when capacity matches size#49530

Open
github-actions[bot] wants to merge 3 commits intomainfrom
ai/diskqueue-buffer-reuse-49519-15510806f9ab52ff
Open

libbeat/diskqueue: reuse decoder buffer when capacity matches size#49530
github-actions[bot] wants to merge 3 commits intomainfrom
ai/diskqueue-buffer-reuse-49519-15510806f9ab52ff

Conversation

@github-actions
Copy link
Contributor

Summary

  • fix eventDecoder.Buffer in libbeat/publisher/queue/diskqueue/serialize.go to reuse the existing slice when cap(d.buf) == n
  • changed the capacity check from cap(d.buf) > n to cap(d.buf) >= n

Why

The reader loop calls decoder.Buffer(int(header.eventSize)) on a hot path. With the previous strict > check, equal-capacity buffers were unnecessarily reallocated.

Additional audit requested in issue

I looked for the same pattern (cap(...) > n followed by make([]byte, n) in reuse/allocation branches) in hot-path areas.

Findings:

  • libbeat/publisher/queue/diskqueue/serialize.go (this PR): real issue, fixed.
  • No other matches in libbeat/publisher/**.
  • One extra repository-wide cap(...) > ... match exists in libbeat/common/streambuf/streambuf.go, but it is a different condition (retainable && cap(data) > newCap) and not the equal-capacity reuse bug pattern.

Validation

  • go test ./libbeat/publisher/queue/diskqueue
  • go test ./libbeat/publisher/queue/diskqueue -run '^$' -bench 'BenchmarkAsync1k$' -benchmem -count=1

Both commands pass in this workspace.


What is this? | From workflow: Mention in Issue

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions bot added the ai label Mar 17, 2026
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 17, 2026
@mergify
Copy link
Contributor

mergify bot commented Mar 17, 2026

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @github-actions[bot]? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@strawgate strawgate added the backport-skip Skip notification from the automated backport with mergify label Mar 18, 2026
@strawgate strawgate marked this pull request as ready for review March 18, 2026 13:40
@strawgate strawgate requested a review from a team as a code owner March 18, 2026 13:40
@strawgate
Copy link
Contributor

/ai can you run and share results from impacted benchmarks please, feel free to also run and report on benchmarks that might be impacted

@coderabbitai
Copy link

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 85407cd3-a3cc-42f2-877d-639f928ef0ce

📥 Commits

Reviewing files that changed from the base of the PR and between 9ab67b9 and 27f5282.

📒 Files selected for processing (1)
  • libbeat/publisher/queue/diskqueue/serialize.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • libbeat/publisher/queue/diskqueue/serialize.go

📝 Walkthrough

Walkthrough

The change modifies eventDecoder.Buffer to reuse the existing buffer when cap(d.buf) >= n instead of only when cap(d.buf) > n, avoiding reallocation when capacity equals the requested size. It also adds validation in decodeJSONAndCBOR that to.Flags is within 0–255 and converts it to the event Flags (with a nolint for gosec G115). No exported function signatures or public types were changed.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed PR meets all objectives: buffer reuse logic changed from > to >= [#49519], validated by tests and benchmarks (~4.28% faster, ~10.99% lower bytes/op), code audit completed.
Out of Scope Changes check ✅ Passed All changes align with PR scope: one-line buffer condition fix plus gosec directive comment. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ai/diskqueue-buffer-reuse-49519-15510806f9ab52ff
  • 🛠️ Update Documentation: Commit on current branch
  • 🛠️ Update Documentation: Create PR
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor Author

TL;DR

golangci-lint failed on all OS jobs due to G115 at libbeat/publisher/queue/diskqueue/serialize.go:184 (uint32uint8 conversion via publisher.EventFlags). Add an explicit bounds check (or justified suppression) before casting, then re-run lint.

Remediation

  • In decodeJSONAndCBOR, validate to.Flags before conversion:
    • if to.Flags > math.MaxUint8, return an error (invalid serialized event flags);
    • otherwise cast safely to publisher.EventFlags(uint8(to.Flags)).
  • Re-run the same check locally/CI (golangci-lint for the touched file/package) to confirm G115 is resolved.
Investigation details

Root Cause

The decoder reads entry.Flags as uint32 but directly converts it to publisher.EventFlags (uint8 underlying type). gosec flags this as potential truncation (G115) in a whole-file lint run.

Evidence

  • Workflow: https://github.com/elastic/beats/actions/runs/23247564451
  • Jobs/step:
    • lint (ubuntu-latest) → step golangci-lint
    • lint (windows-latest) → step golangci-lint
    • lint (macos-latest) → step golangci-lint
  • Key log excerpt:
    • libbeat/publisher/queue/diskqueue/serialize.go:184:30: G115: integer overflow conversion uint32 -> uint8 (gosec)
    • Flags: publisher.EventFlags(to.Flags),
    • issues found

Validation

  • Additional local commands were not run in this detective pass (analysis based on workflow job logs and current PR diff context).

Follow-up

  • If you prefer not to add a runtime guard, an alternative is a narrowly scoped //nolint:gosec with rationale explaining why serialized Flags is guaranteed to fit in uint8.

What is this? | From workflow: PR Actions Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Our encoder only writes EventFlags (uint8) values, so to.Flags is always
0-255. Use nolint:gosec instead of runtime bounds check to avoid hot-path
overhead.

Made-with: Cursor
@strawgate strawgate force-pushed the ai/diskqueue-buffer-reuse-49519-15510806f9ab52ff branch from 9ab67b9 to 27f5282 Compare March 18, 2026 15:19
Copy link
Contributor

@VihasMakwana VihasMakwana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd appreciate another review from @faec as she worked on the diskqueue implementation.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Mar 19, 2026
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 19, 2026
@VihasMakwana VihasMakwana requested a review from faec March 19, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai backport-skip Skip notification from the automated backport with mergify skip-changelog Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[performance-profiler] Reuse diskqueue decoder buffer when capacity equals requested size

4 participants