Fix pluginsd cleanup race with active collector - #22207
Merged
stelfrag merged 2 commits intoApr 14, 2026
Merged
Conversation
- Prevent lifecycle violations by improving thread ownership checks. - Address race condition when releasing PRD dimension references under active collector threads. - Refactor stale `collector_tid` handling and clarify comments.
Contributor
There was a problem hiding this comment.
No issues found across 1 file
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Svc as Service/Cleanup Thread
participant Coll as Collector Thread
participant RRD as RRDSET (Chart Metadata)
participant Mem as PRD Dimension Memory
Note over Svc,Mem: Cleanup Request Flow (rrdset_pluginsd_receive_unslot_and_cleanup)
Svc->>RRD: atomic_load(collector_tid)
RRD-->>Svc: returns active_tid
alt NEW: collector_tid != current_tid (Race Prevention)
Note right of Svc: Another thread owns the chart.
Svc->>Svc: Log warning (Skipping cleanup)
Note over Svc,RRD: CHANGED: We no longer rely on<br/>RRDSET_FLAG_COLLECTION_FINISHED<br/>as it was prone to races.
else CHANGED: collector_tid == current_tid (Collector teardown)
Svc->>Svc: internal_fatal() (if NETDATA_INTERNAL_CHECKS)
Svc->>RRD: NEW: atomic_store(collector_tid, 0)
Svc->>Mem: Free PRD dimension references
else collector_tid == 0 (Safe State)
Svc->>Mem: Free PRD dimension references
end
Note over Coll,Mem: Active Collection Flow (concurrent)
Coll->>RRD: Access chart dimensions
opt collector_tid != 0
Coll->>Mem: Dereference pointers
Note left of Mem: PRD references remain valid because<br/>Service Thread skipped cleanup.
end
stelfrag
marked this pull request as ready for review
April 14, 2026 09:41
stelfrag
marked this pull request as draft
April 14, 2026 09:41
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race in pluginsd slot cleanup where PRD dimension references could be released while an active collector thread may still be dereferencing cached pointers, by tightening collector thread ownership checks during cleanup.
Changes:
- Skip
rrdset_pluginsd_receive_unslot_and_cleanup()whencollector_tidis set and owned by a different thread (no longer relying onRRDSET_FLAG_COLLECTION_FINISHEDas a gate). - When cleanup is invoked from the collector thread, explicitly log/assert and force-clear
collector_tidbefore proceeding. - Remove the prior “stale collector_tid handoff” path and clarify inline comments to avoid lifecycle violations.
Comments suppressed due to low confidence (1)
src/database/rrdset-slots.c:229
- This warning is emitted when cleanup is skipped due to an active collector, but it doesn't include chart/host identification, which can make it hard to action (especially if this can happen during shutdown/finalization across many charts). Consider including
rrdhost_hostname(st->rrdhost)andrrdset_id(st)(or similar identifiers) in the log fields/message.
nd_log_limit(&erl, NDLS_DAEMON, NDLP_WARNING,
"PLUGINSD: attempted cleanup while collector (tid %d) is still active on chart, skipping",
collector_tid);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stelfrag
marked this pull request as ready for review
April 14, 2026 10:04
thiagoftsm
approved these changes
Apr 14, 2026
thiagoftsm
left a comment
Contributor
There was a problem hiding this comment.
PR is running as expected during few hours. LGTM!
nedi-app Bot
pushed a commit
that referenced
this pull request
Apr 24, 2026
* Fix cleanup logic for collector thread in RRDSET - Prevent lifecycle violations by improving thread ownership checks. - Address race condition when releasing PRD dimension references under active collector threads. - Refactor stale `collector_tid` handling and clarify comments. * Clarify collector teardown comment in `rrdset-slots.c` for improved readability
Merged
Ferroin
pushed a commit
that referenced
this pull request
Jul 15, 2026
* Fix cleanup logic for collector thread in RRDSET - Prevent lifecycle violations by improving thread ownership checks. - Address race condition when releasing PRD dimension references under active collector threads. - Refactor stale `collector_tid` handling and clarify comments. * Clarify collector teardown comment in `rrdset-slots.c` for improved readability (cherry picked from commit 5419eec)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
collector_tidhandling and clarify comments.Summary by cubic
Fixes a race where pluginsd cleanup could free PRD dimension refs while a collector thread still used them. Enforces strict ownership: cleanup only proceeds in the collector thread; otherwise it is skipped.
collector_tidis set and not the current thread; stop relying onRRDSET_FLAG_COLLECTION_FINISHED.collector_tid=0before proceeding; clarify that teardown clears it inrrdhost_pluginsd_receive_chart_slots_free().collector_tidhandoff path and tighten teardown comments/logging to prevent lifecycle violations.Written for commit 4a2f22a. Summary will update on new commits.