Update the vendored nats-io client to fix a stalled NATS subscription and a JetStream fetch deadlock - #119867
Conversation
Bump contrib/nats-io from cf441828d30f to cb4edba96601, the head of the ClickHouse/v3.9.2 fork branch that master's pin already tracks. It is a fast-forward (ahead 4, behind 0) and brings three fixes: * 37532c32 (ClickHouse/nats.c#5). The libuv adapter let a reconnect's natsLibuv_Attach install a new socket off the loop thread while a poll removal queued earlier by _evStopPolling was still pending. When the loop thread later drained that older removal, uvPollUpdate saw events == 0 and called natsConnection_ProcessCloseEvent on the fd the attach had just installed. The connection then reported itself attached and never polled again, so a NATS table silently received nothing after its broker restarted. ThreadSanitizer reported it as a data race in natsLibuv_Attach. Poll state is now owned by the loop thread: the socket travels inside the attach event and is assigned in uvAsyncAttach, and natsLibuv_Attach gained the queue-jump guard its three sibling callbacks already had. No new lock, because a lock would have hidden the report while leaving the socket close live. * b41317ae (ClickHouse/nats.c#6). js_maybeFetchMore published its pull request while holding the subscription lock, so DROP DATABASE over a database holding a NATS table with a JetStream consumer blocked for the full 600 second query timeout. * 3e3a3f10 (by @ alexey-milovidov). uvPollUpdate dereferenced a released poll handle when _evStopPolling ran twice, crashing the server on a NATS table whose credentials the broker rejects. Both of my commits have cf441828 as their only parent and were merged as two merge commits on top of 3e3a3f10, so no commit on the branch carries them without also carrying 3e3a3f10. The branch head is therefore the only pin that delivers all three. 3e3a3f10 is self-contained: two guards inside the library, and ClickHouse names none of the internals it touches. Validated with a standalone ThreadSanitizer harness that links the real adapter header against the real contrib/libuv and drives the interleaving from the CI report. At cf441828 the ordering defect reproduces 25 of 25 runs and the race is reported in natsLibuv_Attach; at 3e3a3f10 alone it still reproduces 25 of 25, which is why bumping only to that commit is not enough; at cb4edba9 it is 0 of 25 with the close path still exercised, so the zero is measured rather than an unexercised probe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
groeneai
added
can be tested
Internal second-model review: adjudication log (click to expand)Pre-publication review by an independent model (engine: codex; 2 findings; bounded: 1 full pass).
Severity: ❌ blocker / Session id: cron:clickhouse-review-slot-10:20260913-232400 |
|
Workflow [PR], commit [549dda0] Summary: ❌
AI ReviewSummaryThis PR moves the Findings❌ Blockers
Final Verdict
LLVM Coverage ReportMeasured on commit 549dda0.
Changed lines: Uncovered code analysis did not run: No coverable C/C++ source files changed (contrib/ is excluded from coverage). Newly covered: +187 lines in 53 files (-122 lines lost coverage) · Details |
clickhouse-gh
Bot
added
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| @@ -1 +1 @@ | |||
| Subproject commit cf441828d30fdd5de12d9da319e88d2586fdeeba | |||
| Subproject commit cb4edba96601711e82085153e11b512448f8b7b5 | |||
There was a problem hiding this comment.
This gitlink-only replacement drops the ClickHouse-side half of the auth-rotation fix. The bumped nats.c still closes a connection for good after the same authorization error on two reconnect attempts (_processAuthError is unchanged), but StorageNATS::createConsumersConnection still returns as soon as consumers_connection is non-null (src/Storages/NATS/StorageNATS.cpp:344-350), and threadFunc only reacts to closed subscriptions, not to a permanently closed connection (src/Storages/NATS/StorageNATS.cpp:726-739).
That means the event-loop exception is gone, but once the password/token is accepted again the table still has no path to build a fresh NATSConnection: core NATS stays dead and keeps losing messages, and JetStream stays stalled, until DETACH/ATTACH.
If this PR is superseding #119853, it still needs the StorageNATS closed-connection recovery from that PR (drop consumers, reset the closed connection, and reinitialize) rather than only the submodule bump.
There was a problem hiding this comment.
Both code claims are correct as written, and this PR changes neither site: createConsumersConnection returns at StorageNATS.cpp:346-347 whenever consumers_connection is non-null, and the only closed thing threadFunc reacts to is a subscription (consumersNeedResubscribe() at line 728).
The conditional the finding rests on is false: this PR does not supersede #119853 and is not a substitute for it. Measured on this branch:
3e3a3f10(that PR's pin) is an ancestor ofcb4edba9(this pin), and the four commits between them touch onlysrc/adapters/libuv.h,src/js.c,src/natsp.handtest/.src/conn.cis byte-identical between the two pins, so this bump carries its_closereleased-handle guard verbatim and leaves_processAuthErrorexactly as master has it. Closing for good after repeated auth rejections is therefore neither introduced nor widened here; it is what master does today atcf441828.- The recovery being asked for is that PR's, and its hunks cover precisely the two sites cited above: an
isClosed()drop-consumers-and-reset block inserted increateConsumersConnection, and a reinitialize branch at the top ofthreadFunc. Carrying it here means copying 62 source lines plus a 193-line fake broker and a 211-line rotation test out of an open PR, which would be taking over its author's change.
So the split is: this PR is the gitlink only, for the stall after a broker restart, the JetStream fetch lock-order deadlock, and the released-handle crash it inherits from #119853's commit; #119853 owns getting the table consuming again after the credentials are accepted, and its test. Either merge order works, because the one-line gitlink conflict resolves to cb4edba9, the other pin being its ancestor.
If a single PR is preferred, moving #119853's pin to cb4edba9 subsumes this one and I will close it. The description now states the split rather than leaving it to be inferred.
clickhouse-gh
Bot
added
the
comp-external-dependencies
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units43 translation units recompiled, 9 s compile time in total, 43 of them have a recent master baseline. |
CI finish ledger - 549dda0Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task whose
CIDB breadth behind the RabbitMQ rowSELECT count() AS rows, uniqExact(pull_request_number) AS prs,
countIf(head_ref = 'master' AND pull_request_number = 0) AS true_master
FROM default.checks
WHERE check_start_time > now() - INTERVAL 30 DAY
AND test_name = 'test_storage_rabbitmq/test.py::test_rabbitmq_queue_consume'
AND test_status IN ('FAIL', 'ERROR')Grouped by pull request: 100185 four rows, 116050 two, 110180 one, this branch one. For precedent rather than ownership: the identical The 171 Session id: cron:clickhouse-maint-slot-12:20260914-044300 |
2 participants
Related: #110321
Related: #119853
Related: #118449
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Updated the vendored
nats-ioclient, fixing three defects: aNATStable silently received nothing after its broker restarted;DROP DATABASEhung for 600 seconds on a database holding aNATStable with a JetStream consumer; and the server crashed on aNATStable whose credentials the broker rejects.Description
Asked for by @ alexey-milovidov in #110321 (comment).
contrib/nats-iostill pins a commit from April 2025, so master builds the library without three fixes merged onClickHouse/v3.9.2, the branch it tracks. This moves the pin to that head,cb4edba96601.37532c32(libuv adapter: do not let an attach overtake a queued poll removal nats.c#5): a reconnect'snatsLibuv_Attachinstalled a new socket off the loop thread, ahead of a poll removal_evStopPollinghad queued. Draining that older removal madeuvPollUpdateseeevents == 0and close the fd just installed, while the connection considered itself attached, so it never polled again. Poll state is now loop-thread-owned and the attach has the queue-jump guard its siblings already had. No new lock.b41317ae(Backport upstream fix for the JetStream fetch lock-order inversion nats.c#6):js_maybeFetchMorepublished its pull request while holding the subscription lock, inverting the client's lock order.3e3a3f10, by @ alexey-milovidov:uvPollUpdatedereferenced a released poll handle.Both of my commits sit on top of the third, so only the head delivers all three. #119853 pins
3e3a3f10, an ancestor ofcb4edba9, so a conflict resolves to keepingcb4edba9; itsStorageNATSrecovery of a closed connection is not duplicated here. #119507 moves instead toClickHouse/v3.13.0, which still writesnle->socketinline innatsLibuv_Attach, so it leaves the stall in place.A standalone ThreadSanitizer harness linking the adapter against
contrib/libuvreproduces the ordering defect 25 of 25 runs at the old pin and at3e3a3f10alone, and 0 of 25 atcb4edba9. Two broker-restart integration tests pass on the bumped build.Seen on true master in
Integration tests (amd_tsan, 2/6): the race at60f0a24d70ac, the hang atcd52baad3efe.Post Hooksis red by construction: the two defects I fix are races in the client, so this diff has no test; keepingBug Fixis the reviewer's call.Workflow [PR]
Sync PR [sync-upstream/pr/119867]