Functions: pre-existing follow-ups from the function-mechanism refactor (#23532)

Tracking issue for the pre-existing conditions surfaced by the function-mechanism refactor (#23532) and its review rounds. None are regressions of that PR — each was verified to exist at the base commit. They are bundled here for triage; any of them can graduate to its own issue when picked up.

Last consolidated: 2026-08-20, after a CodeRabbit review of the PR added item 8 and expanded item 1's streaming-sender bullet. Items 3 and 4 are fixed by the nRPC descriptor work on the PR branch (registry destroy barrier 83166d7153, immutable method descriptors 2f67328aef); items 1, 2, 5, 6, 7 and 8 remain open. Every status below was verified by reading the code, not carried forward from earlier write-ups.

1. Inflight-machinery warts (lazy-GC bundle) — OPEN

• Wait-mode timeout sends no FUNCTION_CANCEL: when a caller waiting on a function times out, the plugin is not told to stop until the next function submission triggers the garbage collector — the plugin keeps working on an abandoned request. (After item 3's fix this became a safer, ~2-line follow-up in nrpc_call_async_wait(): cancel dispatch no longer races the registration swap.)
• GC-write interleave on the result buffer: the garbage collector can write the timeout response while another thread writes the (late) real response into the same buffer.
• Inline-sync-on-stream-thread scheduling fork: sync methods execute inline on the calling thread, which for streaming is the stream thread — unlike every other execution path.
• inflight_stream_function.sender is a raw pointer with no lifetime tie to the sender it names — re-surfaced by the CodeRabbit review of 2026-08-20 and traced in full then. When a parent asks a child to run a function, the child stores a bare struct sender_state * in the callback record and starts the call with wait = false; the result and progress callbacks later dereference it to write the answer back up the wire. Nothing binds the two lifetimes.
  • The sender-thread join does not cover it. The completion callback does not run on the sender thread — it runs on whichever thread completes the function, or on the garbage collector that fires the timeout response for a call nobody answered. Joining the sender thread says nothing about either.
  • Cancellation is not a mitigation. nrpc_call_cancel() asks the handler to stop; it does not detach the result callback, and completing is exactly what dereferences the pointer. A teardown that cancelled every outstanding transaction would shorten the window, not close it.
  • The trigger is host cleanup, not disconnects. sender_state is freed from exactly one place, rrdhost_cleanup_data_collection_and_health(), so ordinary reconnect churn leaves the structure alive. What is required is a host being archived or torn down — orphan reaper, node removal, shutdown — while a parent-initiated function is still in flight, which puts this in the same neighbourhood as item 5.
  • The window is wider than the free itself. That teardown destroys the sender's circular buffer and nulls it several lines before freez(host->sender), so a callback landing in between commits into a nulled buffer. One accidental mitigation exists — the callback tests rrdhost_can_stream_metadata_to_parent() first and skips the write once the flag clears — but that test reads through the same pointer, so it helps only while the structure is still valid.
  • Verified pre-existing: the struct, both callbacks, the raw assignment and the no-wait mode are byte-identical on master. The refactor only replaced the direct call with a call spec.
  • Both honest fixes are design work rather than a patch: either give the callback record a counted reference on the sender so teardown drains outstanding calls, or stop referencing the sender at all and re-resolve the host at callback time under a lock. The first changes teardown ordering in a path that already contains an unbounded thread join; the second changes what the callback is allowed to assume.

2. Redundant nrpc_serving_started() registrations — OPEN, narrowed

The proc and windows plugin threads still carry "just in case" calls that the per-thread serving model made verifiably unnecessary. Each is a small, independently verifiable deletion.

The additional in-component instance previously noted here (the registry's insert callback calling it immediately before an acquire that calls it again) no longer exists: the registration callbacks were replaced by descriptor construction (2f67328aef), which takes the serving handle exactly once.

3. Lock-free method-state reads vs. conflict-callback swap — FIXED

Originally filed as the serving-handle read on the cancel/progress paths racing the conflict callback's swap-and-release (a freed-handle acquire in a microsecond window), plus the read-once collector-ownership check in unregister (TOCTOU). An adversarial verification pass during the descriptor planning sharpened it into a class:

• cancel and progress re-read the serving pointer between their acquire and their release, so a swap in between pairs them on different handles — a refcount fatal on one, or a leaked dispatcher ref on the other that hangs that serving thread's drain forever (free-independent: this bites even without the UAF);
• a fifth unlocked reader sat in the catalog traversal, not excluded by the traversal lock;
• several scalar fields were read without the leaf lock the swaps used — sharpest: sync at dispatch time, so a concurrent swap could send a call down the SYNC execution path with an async registration's handler;
• unregister's ownership check could delete a method a different thread had just re-registered, and a conflict could swallow the delete-window tombstone while the delete still removed the fresh registration.

Fixed as a class by 2f67328aef: the method value is now an immutable, refcounted descriptor swapped as one unit (identity stays in the dictionary slot; state is one registration event). Readers pin the descriptor once and read it lock-free; call records hold the descriptor of the registration whose handler actually runs, so executors, the sync/async fork, and cancel/progress dispatch can no longer observe a different registration; unregister verifies under the slot lock that it is deleting the exact descriptor it authorized, and reads its del verdicts (dyncfg, manifest membership) from that same descriptor.

Accepted residual (deliberate, exact parity with the old code): between unregister's tombstone and its key-based delete, a re-registration can land and still be removed. The window is value-blind by lock-order necessity, self-heals on the next full FUNCTION re-list (the re-list is ground truth, DELs are a change-log), and is pinned by a test as present, not claimed fixed.

Deliberate behavior changes recorded with the fix (both wire-observable only in racing/restart windows):

• a re-registration with priority 0 now normalizes to the default (100) like a first registration — the old split behavior was the bug;
• cancel/progress now route to the serving thread of the call's own registration instead of whatever currently owns the name — the old live re-read made an in-flight call uncancellable after a takeover if the new registrant died, and dispatched into dead transports if the original one died.

4. Registry lifetime barrier — FIXED

Originally: non-sender consumers of the function registry could race the registry destroy during orphan cleanup — HTTP/dyncfg/ACLK paths reached the inner dictionaries with no lifetime synchronization, and destroy freed them without any barrier (the receiver's bounded ~2s give-up on a stalled parser thread being one way to get there).

Fixed by 83166d7153, in exactly the shape this item asked for — block new entry, drain active users: the registry embeds a dispatcher-counted gate; every operation that reaches the inner dictionaries through a registry handle brackets its use in one acquire-or-fail (a failed acquire answers what an absent registry answers), and destroy disarms, unlinks, retires the gate (mark dead + drain), and only then destroys the inner dictionaries. A destroy-vs-reader race test drives 2000 full init/register/destroy cycles against the catalog renderer and the availability probe under ASAN.

The receiver's bounded give-up itself remains, but a parser thread that outlives it can no longer reach freed registry memory — its registry operations now acquire-or-fail. What remains of that give-up is host-lifecycle exposure, which is item 5's territory.

5. Un-archive can overlap host teardown — OPEN

nrpc_registry_init() runs from two places. The create-side call is under rrd_wrlock(). The un-archive call is not: rrdhost_find_or_create() releases rrd_wrlock() before calling rrdhost_update(), and rrdhost_update() takes only host->rrdhost_update_lock. The orphan reaper in svc_rrdhost_cleanup_orphan_hosts() holds rrd_wrlock() plus the host's metadata_lifetime_lock for write, and neither excludes that path. So a child reconnecting to a long-archived host can un-archive it at the same moment the reaper tears it down — rrdhost_should_be_cleaned_up() still passes at that instant, because the receiver only clears ORPHAN / sets COLLECTOR_ONLINE after rrdhost_find_or_create() returns.

Three consequences, in increasing order of seriousness:

1. The host can come out of the un-archive live and streaming with no function registry, so its functions stay invisible until its next archive/un-archive cycle.
2. Component-level, found in the descriptor-work review round (2026-08-19, pre-existing — verified present before both descriptor commits): nrpc_registry_init()'s post-set ownership check dereferences the stored entry after dictionary_set has dropped the item reference. A concurrent destroy for the same owner can delete the entry, drop its own reference and garbage-collect the value in that window — a use-after-free read. Candidate fix independent of the wider race: use dictionary_set_and_acquire_item() and do the check under the held reference.
3. Most importantly, and nothing to do with functions: rrdhost_update() is writing into a host that rrdhost_free_unlinked() may be freeing.

Candidate fix for the overlap itself, using machinery that already exists: have the un-archive path take host->metadata_lifetime_lock for read across rrdhost_update(), in the shape commands.c already uses (rrd read lock → try the per-host lock → drop the rrd lock → work → release). The reaper's write attempt then fails and it skips the host — behaviour it already handles. rrdhost_find_or_create() itself already takes that lock for write in its discard-archived-state branch, so the precedent is in the same function. Open question: what the un-archive does when the try fails — every existing user gives up, and giving up here means a reconnecting child retries.

6. A queued FUNCTION_DEL can be dropped in a flag-skew window — OPEN

From the post-PR six-reviewer round, driver-verified (filed as High, traced down to Medium): both places that flush the pending-removal queue only discard it when the parent cannot accept removals and metadata is flowing. The common "sender not ready" case returns early and keeps the queue — nothing lost there. The real gap is a narrow window where the collector goes offline but the ready-for-metrics flag has not cleared yet: a queued removal is dropped, the changed-flag is cleared, and the parent keeps advertising a function that now answers 503. It clears itself when the child reconnects or disconnects, so it is not permanent. Medium.

7. Owner-callback snapshot-then-invoke window — OPEN (mostly a documentation decision)

The registry's owner vtable callbacks are snapshotted under the entry's lock and invoked after releasing it, leaving a window to call into an owner that is tearing down. Pre-existing on master in the same shape; the destroy-side disarm narrows it (snapshots taken after disarm are no-ops) but snapshots taken just before remain exposed, bounded by the owner's thread lifecycle. Deliberately NOT covered by item 4's gate (the changed callback fires outside it, to keep gated sections non-blocking). Follow-up: keep the current comments honest about "narrowed, not closed", and decide whether closing it for real is worth new machinery.

8. The availability probe looks up an unsanitized key — OPEN

The function registry is keyed by sanitized names, but the availability probe looks names up raw. Three of the four operations that reach the registry by name — register, unregister, and call/authorize — sanitize; the probe alone does not. (rrd_function_available() on master, nrpc_method_available() on the PR branch; unchanged in shape between them.)

The function-name sanitizer maps " to '. dyncfg_is_valid_id() rejects whitespace and the single quote, but permits the double quote — so a config id containing " passes validation, is pasted verbatim into the record's function name, and registers under a key that differs from the name the probe later asks for. The probe misses, and dyncfg marks a live config as an orphan. The single-quote clause was added deliberately in fe06e8495ff (2024-05-16), which excluded the character the sanitizer produces while allowing the one it consumes.

Consequences, in increasing severity:

1. A healthy config displays as orphaned. The probe runs inside the sweep over every dyncfg node, so merely rendering the config tree stamps the status onto the live record.
2. get / test / userconfig stop being forwarded to the owning plugin. The caller gets 404 plus a log line blaming a plugin that is in fact running — actively misleading.
3. remove reinterprets orphan status as "reap a dead record": it deletes the in-memory node and the on-disk config file, returns 200, and never tells the plugin, which keeps running the job it was never asked to stop. This is the consequence worth acting on.

Calls keep working throughout, because the call path does sanitize. That is what makes the symptom read as "the config vanished" rather than "the function is broken", and what would make it miserable to diagnose from a bug report.

Reachability. Not reachable from go.d: the Go netdataapi layer rejects ', ", = and \ in a CONFIG id before the wire. It *is* reachable from producers that validate with dyncfg_is_valid_id() alone — the functions_evloop dyncfg helper (used by systemd-journal.plugin and by third-party plugins speaking plugins.d directly), and health alert prototypes, whose function name is built with a plain snprintfz from the alert name. The pluginsd line splitter preserves a double quote that appears mid-word (a quote only opens a quoted region at a word boundary), so such an id survives the wire intact.

Verified pre-existing. Confirmed present on master at 8f91e637b6 by an independent adversarial audit that set out to refute it: every link checked in source, and the two most easily misread — the sanitizer transform and the splitter's quote handling — confirmed by compiling and executing the repository's own code rather than reasoning about it. The core links date to f2b250a1f53 (2024-01-11). Six attempts to refute failed; the seventh narrowed the blast radius to non-Go producers, as recorded above.

Candidate fix. Sanitize in the probe the way the other three paths do, releasing the allocation on every exit, plus a regression test registering and probing an id that contains a double quote. Without the test the raw lookup will read as correct to the next person who touches it.

---

Origin: refactor PR #23532; findings from its 4-model chain review, the post-PR external review round (2026-08-17), the descriptor-work planning/review rounds (2026-08-19), and a CodeRabbit review of the PR (2026-08-20).