[libbeat] Fix a shutdown race in the memory queue

[faec]

Fix a race in the memory queue where an event that was reported as unpublished during queue shutdown could still be enqueued, ingested, and have its acknowledgment callback triggered. In specific circumstances this could cause a panic during Filebeat shutdown, see #47246.

A deterministic test is impossible since the behavior depended on precise timing of an internal select statement with multiple valid paths, but the accompanying unit test fails 90% of the time on the old code, and passes 100% of the time with this change.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in ./changelog/fragments using the changelog tool.

Related issues

• Fixes Memory queue producer race can cause panic on Filebeat shutdown #47246

[elasticmachine]

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

[github-actions]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @faec? 🙏.
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.

[leehinman]

I love that this removes the buffered channel. I don't think this will make performance worse, but any chance you could add a benchmark test to verify?

Otherwise LGTM.

[mauri870]

LGTM, thanks for fixing this! I tested it with the stresstest script and confirmed it's stable.

[faec]

I love that this removes the buffered channel.

I'm increasingly of the opinion that controller logic goroutines should always have unbuffered input channels, there's often some sense that there should be a buffer for performance, "just in case," but it makes the synchronization logic so much less stable, this specific channel buffer has caused years worth of incredibly subtle races.

I don't think this will make performance worse, but any chance you could add a benchmark test to verify?

I don't think so either (Publish already blocks on a channel for every event since the refactor last year, so the buffer seems vestigial), but I will look into confirming.

[faec]

Sigh, the buffered channel is sticking around after all 😔

Per @leehinman's request I added a benchmark to confirm this wasn't hurting performance, and found that it actually did slow down performance by 10-15%. So I've revised the fix to keep the buffered channel but to handle shutdown more carefully in the producers so they still handle a pending response even if the queue shutdown reaches them first. (Response channels already had a one-element buffer, and after the closing channel is triggered the response buffer is guaranteed to be filled for all requests that were actually handled.) This version is more self-evidently performance-neutral, but the benchmark test confirms it and I left the it in the PR just for reference.

[github-actions]

@Mergifyio backport 8.19 9.0 9.1 9.2

[mergify]

backport 8.19 9.0 9.1 9.2

✅ Backports have been created

Details

• #47327 [8.19](backport #47248) [libbeat] Fix a shutdown race in the memory queue has been created for branch 8.19
• #47329 [9.0](backport #47248) [libbeat] Fix a shutdown race in the memory queue has been created for branch 9.0 but encountered conflicts
• #47328 [9.1](backport #47248) [libbeat] Fix a shutdown race in the memory queue has been created for branch 9.1
• #47330 [9.2](backport #47248) [libbeat] Fix a shutdown race in the memory queue has been created for branch 9.2