add_docker_metadata: fix data races in lazy cgroup cache init by orestisfl · Pull Request #51688 · elastic/beats

orestisfl · 2026-07-02T11:53:34Z

Proposed commit message

add_docker_metadata: fix data races in lazy cgroup cache init

add_docker_metadata can run as a beat-level (global) processor, in
which case a single instance is shared by every pipeline client and
its Run method is invoked concurrently. Two data races existed on
that path, both reachable by default because match_pids defaults to
["process.pid", "process.parent.pid"].

lazyCgroupCacheInit guarded the cache with a plain
"if d.cgroups == nil" check-then-set. Concurrent Run calls raced on
the pointer, and two callers could each build a cache and start its
own janitor goroutine, leaking a goroutine and orphaning a cache.
Replace the field with an atomic.Pointer[common.Cache] initialized
exactly once through sync.Once, and load it everywhere it is read.

common.Cache.Get held only the read lock, but get updates the
element's last access time on access-expiry caches (the cgroup cache
is one), so concurrent Get calls mutated the same element under a
shared lock. Take the exclusive lock in Get.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works. Where relevant, I have used the stresstest.sh script to run them under stress conditions and race detector to verify their stability.
I have added an entry in ./changelog/fragments using the changelog tool.

Disruptive User Impact

None.

How to test this PR locally

Regression test

Run the new test with the stress test script:

./script/stresstest.sh --race ./libbeat/processors/add_docker_metadata '^TestMatchPIDsConcurrent$' -p 32

End-to-end reproduction

A running Docker daemon is required, otherwise Run returns early before reaching the cgroup cache.

Create input files so several harvesters publish concurrently at startup:

mkdir -p /tmp/dmrepro/logs /tmp/dmrepro/out
for i in $(seq 1 24); do yes "line" | head -n 400 > /tmp/dmrepro/logs/f${i}.log; done

Write /tmp/dmrepro/filebeat.yml with 24 filestream inputs (each a distinct id), an add_fields that sets process.pid and add_docker_metadata as a global processor:

Config

filebeat.inputs:
  - type: filestream
    id: gen-1
    paths: ["/tmp/dmrepro/logs/f1.log"]
  - type: filestream
    id: gen-2
    paths: ["/tmp/dmrepro/logs/f2.log"]
  - type: filestream
    id: gen-3
    paths: ["/tmp/dmrepro/logs/f3.log"]
  - type: filestream
    id: gen-4
    paths: ["/tmp/dmrepro/logs/f4.log"]
  - type: filestream
    id: gen-5
    paths: ["/tmp/dmrepro/logs/f5.log"]
  - type: filestream
    id: gen-6
    paths: ["/tmp/dmrepro/logs/f6.log"]
  - type: filestream
    id: gen-7
    paths: ["/tmp/dmrepro/logs/f7.log"]
  - type: filestream
    id: gen-8
    paths: ["/tmp/dmrepro/logs/f8.log"]
  - type: filestream
    id: gen-9
    paths: ["/tmp/dmrepro/logs/f9.log"]
  - type: filestream
    id: gen-10
    paths: ["/tmp/dmrepro/logs/f10.log"]
  - type: filestream
    id: gen-11
    paths: ["/tmp/dmrepro/logs/f11.log"]
  - type: filestream
    id: gen-12
    paths: ["/tmp/dmrepro/logs/f12.log"]
  - type: filestream
    id: gen-13
    paths: ["/tmp/dmrepro/logs/f13.log"]
  - type: filestream
    id: gen-14
    paths: ["/tmp/dmrepro/logs/f14.log"]
  - type: filestream
    id: gen-15
    paths: ["/tmp/dmrepro/logs/f15.log"]
  - type: filestream
    id: gen-16
    paths: ["/tmp/dmrepro/logs/f16.log"]
  - type: filestream
    id: gen-17
    paths: ["/tmp/dmrepro/logs/f17.log"]
  - type: filestream
    id: gen-18
    paths: ["/tmp/dmrepro/logs/f18.log"]
  - type: filestream
    id: gen-19
    paths: ["/tmp/dmrepro/logs/f19.log"]
  - type: filestream
    id: gen-20
    paths: ["/tmp/dmrepro/logs/f20.log"]
  - type: filestream
    id: gen-21
    paths: ["/tmp/dmrepro/logs/f21.log"]
  - type: filestream
    id: gen-22
    paths: ["/tmp/dmrepro/logs/f22.log"]
  - type: filestream
    id: gen-23
    paths: ["/tmp/dmrepro/logs/f23.log"]
  - type: filestream
    id: gen-24
    paths: ["/tmp/dmrepro/logs/f24.log"]

# Beat-level (global) processors: built once, shared by every input's pipeline client
processors:
  - add_fields:
      target: process
      fields:
        pid: 1
  - add_docker_metadata:
      match_source: false   # force the match_pids path (lazy cgroup cache)

output.file:
  path: "/tmp/dmrepro/out"
  filename: out

queue.mem:
  events: 4096
  flush.min_events: 1

logging.level: debug
logging.selectors: ["add_docker_metadata"]

Build Filebeat with the race detector and run it.

cd filebeat && go build -race -o /tmp/dmrepro/filebeat.race . && cd -
GORACE="halt_on_error=1" /tmp/dmrepro/filebeat.race run -e --strict.perms=false \
  -c /tmp/dmrepro/filebeat.yml --path.home=/tmp/dmrepro/home

On main the run aborts within a second with a WARNING: DATA RACE (exit code 66) on the shared processor's cgroup cache:

WARNING: DATA RACE
Write at 0x... by goroutine 148:
  common.(*element).UpdateLastAccessTime()   libbeat/common/cache.go:54
  common.(*Cache).get / Get                  libbeat/common/cache.go:284 / 183
  add_docker_metadata.(*addDockerMetadata).lookupContainerIDByPID()  add_docker_metadata.go:381
  add_docker_metadata.(*addDockerMetadata).Run()                     add_docker_metadata.go:278
  processors.(*SafeProcessor).Run() -> processing.(*group).Run()     (global processor)
  pipeline.(*client).publish() <- filestream harvester
Previous read at 0x... by goroutine 155:
  common.(*element).IsExpired()              libbeat/common/cache.go:48
  ... same stack ...

lazyCgroupCacheInit did an unguarded nil-check-and-write of d.cgroups while concurrent Run calls read it, racing on the pointer and able to start multiple janitor goroutines; the path is on by default via match_pids and reachable whenever one instance runs events from multiple goroutines, for example as a beat-level global processor. Guard the init with sync.Once and publish the cache through atomic.Pointer. common.Cache.Get held only the read lock while updating the element's last access time on access-expiry caches; take the exclusive lock. Both races were confirmed with -race before the fix; the new concurrent-Run test passes 600+ stress runs after it.

github-actions · 2026-07-02T11:53:43Z

🤖 GitHub comments

Just comment with:

run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)
/test : Run the Buildkite pipeline.

infra-vault-gh-plugin-prod · 2026-07-02T11:54:35Z

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

coderabbitai · 2026-07-02T11:55:29Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: d59c82a9-ab16-40ef-8eec-5e8c862d08ea

📥 Commits

Reviewing files that changed from the base of the PR and between 5acbbac and 24e1926.

📒 Files selected for processing (4)

changelog/fragments/1783070200-fix-add-docker-metadata-cgroup-cache-races.yaml
libbeat/common/cache.go
libbeat/processors/add_docker_metadata/add_docker_metadata.go
libbeat/processors/add_docker_metadata/add_docker_metadata_test.go

👮 Files not reviewed due to content moderation or server errors (4)

libbeat/common/cache.go
libbeat/processors/add_docker_metadata/add_docker_metadata_test.go
libbeat/processors/add_docker_metadata/add_docker_metadata.go
changelog/fragments/1783070200-fix-add-docker-metadata-cgroup-cache-races.yaml

📝 Walkthrough

[!WARNING]

Walkthrough skipped

File diffs could not be summarized.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

🛠️ Update Documentation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

This PR fixes two data races that can occur when add_docker_metadata is used as a beat-level (shared) processor and Run() is invoked concurrently, by making cgroup-cache initialization and cache reads/writes safe under concurrency.

Changes:

Make the PID→container-ID cgroup cache lazily initialized in a concurrency-safe way using sync.Once + atomic.Pointer.
Fix a race in libbeat/common.Cache.Get() by taking an exclusive lock when Get() may update an element’s last-access time.
Add a regression test that runs concurrent Run() calls to validate the fix under -race, and add a changelog fragment.

Reviewed changes

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

File	Description
`libbeat/processors/add_docker_metadata/add_docker_metadata.go`	Concurrency-safe lazy init and access for the cgroup cache used by PID matching.
`libbeat/processors/add_docker_metadata/add_docker_metadata_test.go`	New concurrent regression test covering shared-processor `Run()` calls.
`libbeat/common/cache.go`	Use exclusive locking in `Cache.Get()` to prevent races when updating last-access time.
`changelog/fragments/1783070200-fix-add-docker-metadata-cgroup-cache-races.yaml`	Changelog entry for the race fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

leehinman · 2026-07-06T19:29:19Z

-		if d.cgroups != nil {
-			d.cgroups.StopJanitor()
+		if cgroups := d.cgroups.Load(); cgroups != nil {
+			cgroups.StopJanitor()


I think we have a problem if Close is ever called before cgroupCache. We would get a Startjanitor goroutine leak. I had Claude code up a test with a potential fix commented out. Let me know what you think.

It is possible this never happens but we have had enough bugs with Start/Stop ordering and races I think it is at least worth looking at.

close_before_cgroup_cache.patch

khushijain21

aside from Lee's comment, this looks good to me

khushijain21 · 2026-07-07T09:07:10Z

 func (c *Cache) Get(k Key) Value {
-	c.RLock()
-	defer c.RUnlock()
+	// Exclusive lock because get can update the element's last access time.


[nit]

It's just that while reading it sounded bizzare at first

Suggested change

// Exclusive lock because get can update the element's last access time.

// Exclusive lock because Get() can update the element's last access time.

orestisfl added bug Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-active-all Automated backport with mergify to all the active branches labels Jul 2, 2026

orestisfl self-assigned this Jul 2, 2026

botelastic Bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 2, 2026

orestisfl marked this pull request as ready for review July 2, 2026 11:54

orestisfl requested a review from a team as a code owner July 2, 2026 11:54

orestisfl requested review from khushijain21 and leehinman July 2, 2026 11:54

orestisfl requested a review from Copilot July 2, 2026 11:54

orestisfl changed the title ~~libbeat/add_docker_metadata: fix data races in lazy cgroup cache init~~ add_docker_metadata: fix data races in lazy cgroup cache init Jul 2, 2026

Copilot started reviewing on behalf of orestisfl July 2, 2026 11:57 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

leehinman reviewed Jul 6, 2026

View reviewed changes

khushijain21 approved these changes Jul 7, 2026

View reviewed changes

khushijain21 reviewed Jul 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

add_docker_metadata: fix data races in lazy cgroup cache init#51688

add_docker_metadata: fix data races in lazy cgroup cache init#51688
orestisfl wants to merge 1 commit into
elastic:mainfrom
orestisfl:add-docker-metadata-races

orestisfl commented Jul 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

infra-vault-gh-plugin-prod Bot commented Jul 2, 2026

Uh oh!

coderabbitai Bot commented Jul 2, 2026

Walkthrough skipped

Uh oh!

Copilot AI left a comment

Uh oh!

leehinman Jul 6, 2026

Uh oh!

khushijain21 left a comment

Uh oh!

khushijain21 Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// Exclusive lock because get can update the element's last access time.
	// Exclusive lock because Get() can update the element's last access time.

Uh oh!

Conversation

orestisfl commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

Disruptive User Impact

How to test this PR locally

Regression test

End-to-end reproduction

Uh oh!

github-actions Bot commented Jul 2, 2026

🤖 GitHub comments

Uh oh!

infra-vault-gh-plugin-prod Bot commented Jul 2, 2026

Uh oh!

coderabbitai Bot commented Jul 2, 2026

Walkthrough skipped

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

leehinman Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

khushijain21 left a comment

Choose a reason for hiding this comment

Uh oh!

khushijain21 Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

orestisfl commented Jul 2, 2026 •

edited

Loading