Fix pluginsd cleanup race with active collector (#22207)
* 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
Stelios Fragkakis committed
Apr 14, 2026 at 21:33 UTC
5419eecee594998161e1109e93eb704585723bd9
1 file changed
+21
-25
src/database/rrdset-slots.c
+21
-25
@@ -212,12 +212,17 @@ void rrdset_pluginsd_receive_unslot_and_cleanup(RRDSET *st) {
212
pid_t collector_tid = __atomic_load_n(&st->pluginsd.collector_tid, __ATOMIC_ACQUIRE);
213
pid_t current_tid = gettid_cached();
214
if(collector_tid != 0) {
215
- if(collector_tid != current_tid &&
216
- !rrdset_flag_check(st, RRDSET_FLAG_COLLECTION_FINISHED)) {
217
- internal_fatal(true,
218
- "PRD_ARRAY: cleanup called while collector (tid %d) is still active - lifecycle violation",
219
- collector_tid);
220
-
215
+ if(collector_tid != current_tid) {
216
+ // A different thread owns this chart as collector.
217
+ // We must not release PRD dimension references while that thread may
218
+ // still be using cached rd pointers from the array.
219
+ // RRDSET_FLAG_COLLECTION_FINISHED is not a reliable signal here:
220
+ // it can be set by the cleanup caller (service thread) rather than
221
+ // by the collector itself, creating a race where we free dimensions
222
+ // that the collector is actively dereferencing.
223
+ // Legitimate teardown clears collector_tid via
224
+ // rrdhost_pluginsd_receive_chart_slots_free() after the receiver
225
+ // is fully stopped, before invoking cleanup.
226
nd_log_limit_static_global_var(erl, 1, 0);
227
nd_log_limit(&erl, NDLS_DAEMON, NDLP_WARNING,
228
"PLUGINSD: attempted cleanup while collector (tid %d) is still active on chart, skipping",
@@ -226,28 +231,19 @@ void rrdset_pluginsd_receive_unslot_and_cleanup(RRDSET *st) {
231
return;
232
}
233
229
- if(collector_tid == current_tid) {
230
- // Cleanup in the collector thread should not normally happen.
231
- // Keep this explicit so we don't mask it as a stale tid case.
234
+ // We ARE the collector thread - safe to proceed with cleanup.
235
+ // This should not normally happen; keep it explicit so we don't
236
+ // mask it as a stale tid case.
237
#ifdef NETDATA_INTERNAL_CHECKS
233
- internal_fatal(true,
234
- "PRD_ARRAY: cleanup called from collector thread (tid %d) - lifecycle violation",
235
- collector_tid);
238
+ internal_fatal(true,
239
+ "PRD_ARRAY: cleanup called from collector thread (tid %d) - lifecycle violation",
240
+ collector_tid);
241
#endif
242
238
- nd_log_limit_static_global_var(erl_collector, 1, 0);
239
- nd_log_limit(&erl_collector, NDLS_DAEMON, NDLP_WARNING,
240
- "PLUGINSD: cleanup called from collector thread (tid %d), forcing collector_tid=0",
241
- collector_tid);
242
- }
243
- else {
244
- // Finalization can hit stale collector_tid on charts that were not switched away.
245
- // Treat this as cleanup ownership handoff and continue.
246
- nd_log_limit_static_global_var(erl_finalize, 1, 0);
247
- nd_log_limit(&erl_finalize, NDLS_DAEMON, NDLP_WARNING,
248
- "PLUGINSD: cleanup forcing stale collector_tid=%d to 0 for finalized chart",
249
- collector_tid);
250
- }
243
+ nd_log_limit_static_global_var(erl_collector, 1, 0);
244
+ nd_log_limit(&erl_collector, NDLS_DAEMON, NDLP_WARNING,
245
+ "PLUGINSD: cleanup called from collector thread (tid %d), forcing collector_tid=0",
246
+ collector_tid);
247
248
__atomic_store_n(&st->pluginsd.collector_tid, 0, __ATOMIC_RELEASE);
249
}