Retain the connection for the duration of an event loop attachment

[groeneai]

Backport of upstream nats-io/nats.c@b89b44d07 (resolves nats-io#888) and nats-io/nats.c@655c8ea54, neither on this branch. Red in ClickHouse CI now, on Integration tests (amd_asan_ubsan, db disk, 4/8): https://github.com/ClickHouse/ClickHouse/actions/runs/35728383336/job/106765149366 . The abort kills the server container, so 18 of that shard's 33 test_nats_core.py cases go red at once. Five such jobs on five PRs since 2026-09-20; 0 true-master rows.

The adapter keeps the connection as a raw, never-cleared nle->nc, and the library held no reference for it. When the last user reference is dropped off the loop thread, _close only queues the poll removals and the detach, so that release can bring the count to zero while a read event is in flight. The guard atop natsConnection_ProcessReadEvent exists for exactly such a late event, but it needs a live connection to be readable at all. The library now retains once at the very first attach, and an adapter calls the new natsConnection_ProcessDetachedEvent when it can no longer dereference nle->nc.

Abridged ASan report from that job (the freed-by stack is the whole root cause)

==8==ERROR: AddressSanitizer: heap-use-after-free on address 0x7cd73af46d80
READ of size 8 at 0x7cd73af46d80 thread T204 (ThreadPool)
    #0 natsConnection_ProcessReadEvent  src/conn.c:4105:5        <- natsConn_Lock(nc)
    #1 natsLibuvPoll                    src/adapters/libuv.h:197:9
    #2 uv__io_poll   #3 uv_run          #4 DB::NATSHandler::runLoop()

freed by thread T204 (ThreadPool) here:
    #1 _freeConn                        src/conn.c:211:5
    #2 natsConn_release                 src/conn.c:244:9
    #3 natsConnection_ProcessReadEvent  src/conn.c:4145:5        <- its OWN trailing release
    #4 natsLibuvPoll                    src/adapters/libuv.h:197:9

previously allocated by thread T204 (ThreadPool) here:
    #1 natsConn_create                  src/conn.c:3238:14
    #2 natsConnection_Connect           src/conn.c:3324:13

Validation, same sanitizer setup, driving the destroy off the loop thread:

tree / injection	result
this branch, unpatched	reproduces, 4 of 4 seeds, within 8-415 destroys
with the change	0 use-after-free over 6000 destroys, LSan clean, nothing left registered in the loop
change minus the library's retain	use-after-free returns within 5-7 destroys
change minus the adapter's release	no use-after-free, but every connection leaks (natsConn_create in the LSan stack)
uv_poll_start failed on the very first attach	clean, later connects fine, nothing left registered; unpatched leaves a poll handle behind
the reconnect's scheduling call failed	reconnects on a later attempt; unpatched never comes back up
the teardown's write removal failed	clean, nothing left registered; unpatched reads freed memory from natsLibuvPoll

src/natsp.h, src/conn.c and src/nats.h are upstream's hunks unchanged. Three adapter-side departures, because this branch's adapters are not upstream's:

• Upstream's third hunk, uv_poll_stop(nle->handle) atop natsLibuv_Read, is not taken: it would mutate loop->watchers from whatever thread closes the connection and deref an nle->handle that is legitimately NULL there. Nor is it load-bearing: with the retain a late poll event lands on a live connection, which the existing guard refuses.
• Upstream's else if (created) natsLibuvEvents_free(nle, false) raw-frees a uv_async_t already linked into loop->handle_queue and loop->async_handles. Here a failed attach closes what libuv knows about and frees only storage it never saw, uvAsyncAttach undoes its own poll handle, reference ownership travels in a new releaseConnOnFree flag, and a failing uv_mutex_init frees and nulls nle->lock so a non-NULL lock always means an initialized mutex. That created gate also repairs a pre-existing defect the refcount contract makes mandatory: a successive attach failing used to free nle unconditionally, leaving nc->el.data dangling for the next reconnect.
• The detach closes a poll handle the stop-polling request failed to retire: _evStopPolling asks for the read removal only if the write removal succeeded and _close discards the result, so off the loop thread an allocation failure can leave it armed past the free of its nle.

test_EventLoopDestroyWhileAttached is a new registered case, deterministic both ways, and the four existing EventLoop* cases still pass against nats-server v2.14.6. It covers the library half; the adapter half is covered by the driver above, since nothing here compiles an adapter header.

Related, not a substitute: ClickHouse#119507 would bump this submodule to v3.13.0, which has both commits; it is open and master pins v3.9.2.

[groeneai]

Internal second-model review: adjudication log (click to expand)

Pre-publication review by an independent model over four rounds (engine: codex; 3, then 2, then 1,
then 0 findings; base 26daa919, full scope every time). Three blockers were accepted and fixed
before this branch was pushed, one of them a defect the previous round's own fix introduced. The
fourth round reviewed the final content and returned nothing.

#	Sev	Finding	Verdict	Evidence / action
1	❌	A failed queued poll removal lets detach free the adapter and release the connection while the poll handle is still armed (src/adapters/libuv.h, uvAsyncDetach)	AGREE, fixed	_evStopPolling issues the write removal first and only reaches the read removal on success, uvScheduleToEventLoop returns NATS_NO_MEMORY when its event allocation fails, and the library discards that status, so the handle was never retired before the free. uvAsyncDetach now closes a residual nle->handle with uvHandleClosedCb, which does not dereference nle, so close-callback ordering is irrelevant. New injection arm covers both directions.
2	❌	The libevent sibling still crashes or reports success when read/write event attachment fails (src/adapters/libevent.h, natsLibevent_Attach)	DISAGREE	The observations are accurate but every cited line is outside this diff, and byte-identical to the base branch. They also break nothing this change asserts: nle is calloc-zeroed, releaseConnOnFree is set only on a successful first attach, natsLibevent_Detach releases exactly when it is set, a failed first attach is compensated in the library, and on a successive attach s can never become non-OK because every assignment to it sits inside the if (nle == NULL) branch. Hardening it here would add unreproducible OOM-path defensive code to a sibling adapter and widen a two-commit backport. Reported, not fixed.
3	⚠️	The regression test does not exercise either bundled adapter's ownership transfer or release path (test/test.c)	AGREE in part	The claim is correct: grep -c 'natsLibuv|natsLibevent' test/test.c is 0, so no committed case would redden if releaseConnOnFree or the natsConnection_ProcessDetachedEvent call were removed from an adapter. The remedy as asked is not available here: CMakeLists.txt:41-42 default the libuv and libevent examples to OFF, :70-71 require a caller-supplied LIBUV_DIR, and libuv and libevent appear zero times across all six workflow files, so nothing in CI compiles an adapter header. The committed case therefore covers the library half, and the adapter half is covered by the out-of-tree sanitizer driver whose arms are summarised in the description, including the two mutation arms that remove the retain and remove the release. Stated rather than papered over.
4	❌	On Windows a failed uv_poll_init_socket can leave a freed poll handle linked in libuv's loop (src/adapters/libuv.h, uvAsyncAttach)	AGREE, fixed	Correct, and it is this change's own claim to uphold. src/win/poll.c calls uv__handle_init (which inserts into loop->handle_queue) and only then does a fallible getsockopt, so a non-zero return does not imply the loop never saw the handle. src/unix/poll.c returns every error before uv__handle_init, so the old test was exact on every platform ClickHouse builds but not on Windows. The handle is now allocated zeroed and its own type decides: storage the loop never saw is freed, anything it linked is closed. uv_async_init needs nothing: it cannot fail after linking on either platform.
5	❌	The residual-handle detach fix still leaks the connection socket when queued poll removal fails (src/adapters/libuv.h, uvAsyncDetach)	DISAGREE	The leak is real and pre-existing: _evStopPolling invalidates nc->sockCtx.fd only when both removals succeeded, _close discards its status, and _freeConn never closes the descriptor. What does not hold is the prescribed pairing this rests on. nle->socket is written at the top of every uvAsyncAttach and cleared only in uvPollUpdate's both-removed branch, so when a reconnect attach fails inside uvAsyncAttach - no allocation failure needed, a uv_poll_start failure does it, which is one of the injection arms - the teardown nulls nle->handle but leaves nle->socket holding that attempt's descriptor, and _close then takes its non-event-loop branch and closes that same descriptor itself. An unconditional ProcessCloseEvent in the detach would close an already-closed descriptor whose number another thread may have re-bound, which is worse than the leak and reachable without allocation failure. The diff touches neither nle->socket, _evStopPolling, _freeConn nor uvPollUpdate's socket branch, and descriptor ownership is not what this change contracts. It is also asymmetric in three distinct failure paths, not one, so a close added in one of them patches an instance instead of establishing an invariant: the safe shape is to gate it on nle->handle != NULL, which excludes every path where the attach failed, together with a descriptor-count oracle. That is its own change, not a line in a backport.
6	❌	The first fix for #4 left that same handle orphaned instead: neither closed nor freed, and the only pointer to it discarded (src/adapters/libuv.h, uvAsyncAttach)	AGREE, fixed	Right, and it exposes a second error in #4's reasoning. uv_poll_init_socket on Windows can fail on either side of the step that links the handle, an early ioctlsocket(FIONBIO) before it and the getsockopt after it, so a success flag cannot separate "never linked" from "linked"; only the handle's own type can. Closing a linked handle whose init did not finish was refused first time round on the grounds that uv_close has no contract for that state, which the pinned sources contradict: uv_close dispatches on type to uv__poll_close, which zeroes events, calls uv__handle_closing (needs only the loop and flags that uv__handle_init wrote) and then, since a zeroed handle that failed at getsockopt has both submitted_events_* counters at 0, takes the early uv__want_endgame return without reading socket, peer_socket, the slow-poll flag or either request; uv__poll_endgame asserts exactly those two counters and hands the handle to the close callback. So the flag is dropped and the choice is made on type alone: freed when the loop never saw it, closed when it did. That is one local and one branch smaller than the shape it replaces and, because Unix returns every init error before linking, provably identical on every platform this branch can build, which is why the existing injection arms revalidate it unchanged. Landed: the flag and its two writes are gone, the declaration line and the whole uv_poll_init block are back to the base's text byte-for-byte, and the compiled comparison against the handle's type at offset 0x10 is visible in both the driver binary and ClickHouse's own NATSHandler.cpp.o. The fourth review round read that content and raised nothing.
7	⚠️	The same finding also asked for a Windows post-link initialization-failure test	DISAGREE	Not writable here, and the limitation is stated rather than papered over. The only Windows job in this repository configures with -DNATS_BUILD_STREAMING=OFF and installs no libuv, and CMakeLists.txt:41-42,70-71 leave the libuv example off unless the caller supplies LIBUV_DIR, so src/adapters/libuv.h is not compiled on Windows at all, let alone exercised. Reaching the branch also requires getsockopt(SO_PROTOCOL_INFOW) to fail on a socket that has just connected, i.e. fault injection inside libuv, for which this test harness has no seam; such a test would measure the injection. The justification shipped instead is libuv's own source ordering plus the fact that the change is a no-op on every buildable platform.

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are
terminal per finding. Findings on hunks unchanged by the fix round are auto-dropped.

Also noticed and deliberately not changed, none of them reachable from any input a user can run:
after a failed uvAsyncAttach on a queued reconnect attach, that attempt's socket is left open,
because the library has already been told the attach succeeded and uvPollUpdate returns early on a
NULL handle; a detach whose own queueing allocation fails never runs, so the adapter object and the
connection reference it holds are both pinned; and because the release is now made from the loop
thread, a connection destroyed after its loop has already stopped leaks instead of being freed with a
dangling adapter pointer, which is measurably not the ordering StorageNATS shuts down in and is a
strict improvement on a use-after-free in any case.

Session id: cron:clickhouse-review-slot-9:20260923-072700

[groeneai]

Pre-PR validation (a-i)

#	Question	Answer
a	Deterministic repro?	Yes, two. An ASan driver linking the real library and real libuv reproduces in 4 of 4 seeds within 8-415 connect/destroy cycles; and test_EventLoopDestroyWhileAttached reproduces with no race at all, every run, on the unpatched branch.
b	Root cause explained?	Yes. The adapter holds the connection as a raw, never-cleared nle->nc and the library took no reference for the attachment, while _close only queues the poll removals and the detach; so the last release can land inside a poll callback with the handle still armed, and the late-event guard needs a live connection to be readable.
c	Fix matches the root cause?	Yes. One ownership invariant at the layer that lacked it (while the adapter object exists, refs >= 1), which is upstream's own fix and what v3.13.0 ships. Not a guard at the crash site, not a NULL check on nle->nc (still racy).
d	Test intent preserved / tests added?	Yes. No assertion weakened; upstream's own test hunks carried, the fork-only EventLoopParserResetOnDisconnect given the same treatment, and one new registered case added. All six EventLoop-family cases pass against nats-server v2.14.6.
e	Both directions demonstrated?	Yes. Base reddens 4/4 seeds, fix clean over 6000 destroys on the same seeds; the committed test reddens on the unpatched branch with the reported signature and passes with the fix. The detach-side hardening has its own pair: with the teardown's write-event removal forced to fail, the tree without it reddens heap-use-after-free in natsLibuvPoll on 2 of 2 seeds, and the tree with it is clean with nothing left registered in the loop. One sub-case is stated rather than papered over: on Unix uv_poll_init never links a handle into the loop and then fails, so the uv_close side of the type test is reached there only from a failing uv_poll_start, which is the measured arm above; the linked-then-failed-init case is Windows-only, and it rests on libuv's own source ordering rather than on a measurement claimed here.
f	General across code paths?	Yes. Read and write events, first attach and reconnect, attach failing in the library and inside the adapter, _close's own retain/detach/release triple and its already-closed early return, every write site of the adapter's poll-handle field (all loop-thread-only, each uv_close followed immediately by a NULL so there is no double close), and the libevent adapter (this branch's only synchronous free site, which would otherwise leak the connection forever, and which has no twin of the detach-side hazard because it frees its events synchronously) all walked.
g	Generalizes across inputs?	Yes, for the relevant space. A refcount lifetime fix has no type or value surface; the equivalent axis is state, and the seven arms plus the committed test span first attach vs reconnect, success vs failure in library vs adapter, success vs failure of the teardown's own poll removal, destroy on vs off the loop thread, read vs write vs late event, and both adapters.
h	Backward compatible?	Yes. No setting, format or default changes. One new symbol, natsConnection_ProcessDetachedEvent, with upstream's name and signature, so a later v3.13.0 bump converges rather than conflicts; no new caller outside the adapters.
i	Invariants and contracts preserved?	Yes. Refcount balance walked on every path including error and early-return; the release precedes the free that would invalidate the pointer it reads; a handle libuv knows about is closed and never freed, storage it never saw is freed and never closed (measured, and the unpatched branch fails it), with that choice made on the handle's own type witness rather than on the init's return value, because uv_poll_init sets type in the same step that links the handle and can fail on either side of that step; a non-NULL adapter lock now always means an initialized mutex; no new lock, so no lock ordering change.

Two mutation arms are the non-vacuity proof: removing only the library's retain brings the
use-after-free back within 5-7 destroys, and removing only the adapter's release leaves no
use-after-free but leaks every connection, with the connection's own allocation stack in the
LeakSanitizer report. Each half is therefore shown necessary by reddening a different oracle.