Backport upstream fix for the JetStream fetch lock-order inversion

[groeneai]

What breaks

A pull-async JetStream subscription can deadlock the client threads: once it fires, nothing on
that connection completes again. ClickHouse hits it when a NATS JetStream table subscribes a
second consumer, or subject, on one connection while the first is fetching.

Root cause

The library's lock order is nc->mu, nc->subsMu, then sub->mu (taken with the
subscription's dispatcher lock). js_maybeFetchMore inverted it by publishing the pull request
while still holding the subscription and dispatcher locks, so sub->mu was held while
natsConn_publish took nc->mu. That closes a cycle with natsConn_subscribeImpl and
natsConn_processMsg, which take an outer lock first.
js_PullSubscribeAsync produced the same edge a second way, calling
js_maybeFetchMore while already holding both locks; natsMutex is recursive, so breaking that
function's contract (js.c:2907) was silent.

The change

A cherry-pick of upstream 7220e9f6e081 (nats-io#834, fixes nats-io#823),
released in v3.10.0. This branch is cut from v3.9.2, so it was never inherited.
The src/ hunks are byte-identical to upstream; the one dropped hunk reorders
_test(SSLVerificationCallback), absent from this fork.
The new test is extended so reverting either half of the fix reddens it.

The publish moves outside the locks and js_PullSubscribeAsync releases them before the first
fetch, so the inverted edge is removed, not guarded. The jsFetch::replySubject field
goes with it: its fixed 61 bytes assumed the default inbox prefix, which
natsOptions_SetCustomInboxPrefix does not bound, so a long prefix truncated the reply
subject.

Left unfixed, all pre-existing. Custom inbox prefixes stay partly broken:
_fetchIDFromSubject (js.c:1705) parses the fetch id at a fixed default-prefix offset, so
fetch status messages go unrecognized; the synchronous _fetch path
keeps the same 61-byte buffer. applyNewSID drops only sub->mu before taking
nc->subsMu, a different pair needing a shared dispatcher and ordered consumers.

Two windows come with the patch, both still open in upstream main, so this adopts upstream's
state: the lock is dropped across the publish, so the
fetch->requestedMsgs update is no longer atomic with the fetch id bump and the next request can
be sized from a stale counter; and the initial fetch runs unlocked, so a callback destroying the
subscription before the call returns could free it mid-publish. Neither is reachable from
ClickHouse, which leaves MaxMessages and MaxBytes at 0 and sets ManualAck.

Validation (three sanitizer arms, a deadlock oracle, a sizing mutant)

test_JetStream_GH823, extended here to count deliveries per subscription and to wait for a
second batch on each, under ThreadSanitizer with NATS_BUILD_NO_SPIN=ON (an uncontended
natsMutex_Lock spins on trylock, which the detector cannot see). Five runs per arm.

Arm	inversions reported	second-batch assertion
unpatched	5 of 5 runs	fails 5 of 5
this fix minus the js_PullSubscribeAsync hunk	5 of 5 runs	fails 5 of 5
this fix	0 of 5 runs	passes 5 of 5

The middle arm is the sensitivity control: it shows the second hunk is load-bearing rather than
cosmetic. The assertion failure is a genuine stall, not a tight wait, since it still fails with
the wait widened to 30 seconds. With spinning left on, the same sanitizer build wedges the
unpatched tree on 4 of 5 runs and this one on 0 of 5.

Without a sanitizer the inversion is only a potential deadlock, so no functional assertion
reddens reliably there. That is what test_JetStream_GH823CustomInboxPrefix covers: it fails
5 of 5 on a tree differing from this one only by restoring the fixed-size field, while
test_JetStream_GH823 still passes there.

The thirteen other JetStream tests pass unchanged on both trees, the two new tests pass 70 of 70
runs on this tree across three build configurations, and the patched library compiles clean in
ClickHouse.

The ClickHouse-side contrib/nats-io bump follows in a separate pull request.

Reported on: ClickHouse/ClickHouse#96487 (comment)
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=96487&sha=3d1580985ee545bb52169f392baa8a3bc643d4fe&name_0=PR&name_1=Integration%20tests%20%28amd_tsan%2C%205%2F6%29
Related: nats-io#834

[groeneai]

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

Pre-publication review by an independent model over four rounds (engine: codex; 3 findings, then
4, then 2, then 2), plus my own cold review of the resulting code in each round. Every finding
below is either fixed or carries recorded evidence for the verdict.

Rows 2 and 6 were raised in three separate rounds. My first refutation of each was wrong and the
rows record the correction, not the original verdict: I accept both mechanisms and decline only
the remedy, because src/ here is byte-identical to upstream 7220e9f6e081 and both windows are
open in upstream main today, so closing them in this fork would mean shipping concurrency
changes upstream has not reviewed. Both are stated in the description instead.

#	Sev	Finding	Verdict	Evidence / action
1	⚠️	The justification for the one dropped hunk named SSLSocketLeakWithEventLoop and said this fork lacks it	AGREE, fixed	The hunk reorders _test(SSLVerificationCallback), which the fork does lack; SSLSocketLeakWithEventLoop is present (test/list_test.txt:263) and was only a context line. Commit message and description corrected
2	❌	Publishing before recording requestedMsgs can over-fetch and permanently discard messages from an AckNone pull consumer	AGREE on the mechanism, remedy declined, disclosed	Raised twice. My earlier refutation rested on the server rejecting AckNone for pull consumers (error 10084), and that is false: the rejection is gated on work-queue retention, jsStreamConfig_Init defaults Retention to js_LimitsPolicy (src/jsm.c:1274), and this suite already asserts NATS_OK for a pull-async subscription with js_AckNone. The window is real: the publish at js.c:2945 precedes the increment at js.c:2947-2950, _autoNextFetchRequest sizes the next request from the stale counter (js.c:2976), and the surplus is destroyed undelivered (dispatch.c:147-153, :373-375). I am not changing src/ for it, because upstream main still has the identical structure today, so closing it here would put unreviewed concurrency semantics into a vendored copy, and because it is not reproducible, which is its own reason not to patch speculatively. It is stated in the commit message and the description instead
3	⚠️	The custom-inbox change still cannot recognize fetch status subjects for a non-default prefix	AGREE on the claim, contract narrowed	Correct: _fetchIDFromSubject (js.c:1705) parses at a fixed default-prefix offset. It is untouched by this diff and identically broken before it, so I narrowed the description rather than widening the patch: only the truncation is fixed, the rest is disclosed as pre-existing
4	⚠️	The regression test can pass even if async fetching silently stops	AGREE, fixed @ cc9ae6d5a426f	Raised twice, and right both times: the callback discarded messages without counting and the test asserted only the two creation statuses. It now counts deliveries per subscription and waits for a sixth message on each, which can only arrive from a second pull request, so a fetch that publishes once and dies no longer passes. src/ stayed byte-identical to upstream
5	⚠️	The reply-subject sizing change is not exercised by any test	AGREE, fixed @ cc9ae6d5a426f	Correct: the test connected with default options, so restoring the deleted 61-byte field would have left it green. A custom-inbox-prefix arm now pins it, bounded to one message so it does not depend on the status-parsing non-goal in row 3. It fails 5 of 5 on a tree differing from this one only by restoring the field
6	❌	The unlocked first fetch has no reference protecting sub and fetch from callback-driven destruction	AGREE on the mechanism, remedy declined, disclosed	Raised twice. My earlier refutation counted the caller's reference as unavailable to callbacks, which double-counts it: there is one caller reference and natsSubscription_Destroy from a callback consumes exactly that one. Correcting the finding too, since a use-after-free needs more than a callback calling Destroy: the dispatcher holds an independent reference from sub.c:396 that user code cannot drop, released only when that thread exits (dispatch.c:591), so the free has to beat the few remaining instructions in js_maybeFetchMore. Narrow, but real, and new for this path. Declined for the same reasons as row 2, and because the suggested regression would exercise undefined usage: destroying a subscription the constructor has not returned yet
7	💡	The list of remaining custom-prefix limitations omits the synchronous pull path's identical fixed buffer	AGREE, fixed	Verified at both revisions: _fetch keeps char rply[NATS_DEFAULT_INBOX_PRE_LEN + NUID_BUFFER_LEN + 32] and the truncating snprintf, and this diff touches no rply[ line. The description now covers both custom-prefix holes. The commit message's "only the truncation is fixed" is narrower than that, naming just the status-parsing hole; noted, not blocking
8	❌	My own review: the description dismissed the row 2 window with the false AckNone claim, and carried three stale measurements	AGREE, fixed @ b41317aede9e2	The false sentence is gone. Also corrected: a 2-of-10 deadlock rate that described an earlier version of the test (the shipped test is green 4 of 5 on a plain build, and wedges 4 of 5 under the sanitizer with spinning on), "fourteen existing tests pass on both trees" (it is thirteen others, and the two backported tests are now supposed to fail on the base tree), and a "verbatim cherry-pick" claim that was only ever true of src/
9	💡	sub->subject and jsi->nxtMsgSubj are read outside the subscription lock	DISAGREE	Both are immutable for any subscription reaching this code. sub->subject is reassigned only by the ordered-consumer reset (js.c:3505), whose own comment scopes it to a simple push subscriber, and nxtMsgSubj only during _subscribeMulti before the subscription is returned
10	💡	Upstream's new test is not compiled by ClickHouse at all	DISAGREE	True and expected: contrib/nats-io-cmake/CMakeLists.txt lists src/*.c explicitly and no test/
11	⚠️	My own review: both the description and the commit message framed the row 2 window as an ordering change the patch introduces	AGREE, description fixed	The publish already preceded the increment before this patch (js.c:2938 then :2941 at the base), so "now updated after the request is on the wire" described nothing the diff changed, and natsConn_publish (pub.c:86) buffers rather than writing, so "on the wire" was wrong in both trees. What the patch actually changes is atomicity: the lock is dropped across the publish, so the requestedMsgs update is no longer atomic with the fetch id bump and a reply can now interleave. The description says that. The commit message still carries the looser "now publishes ... before it adds" clause; its conclusion is correct and I left it rather than spend a third message-only round on wording

Severity: ❌ blocker / ⚠️ major / 💡 nit. DISAGREE verdicts carry recorded evidence and are
terminal per finding.

Neither window in rows 2 and 6 is reachable from ClickHouse, which is why this backport is still
the right trade against a deadlock that ThreadSanitizer reports on 5 of 5 runs: its JetStream
consumer leaves MaxMessages and MaxBytes at zero, so the over-limit discard is dead code
there, it sets ManualAck, and it destroys no subscription from a callback.

Session id: cron:clickhouse-review-slot-9:20260902-035900

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. testsuite JetStream_GH823 on a ThreadSanitizer build with NATS_BUILD_NO_SPIN=ON reports lock-order-inversion on 5 of 5 unpatched runs. NATS_NO_SPIN is needed only because natsMutex_Lock spins on pthread_mutex_trylock for up to gLockSpinCount (default 2000) attempts before taking a real lock, and the detector builds lock-order edges only from real acquisitions. Proved with a two-sided control: the same 3-thread cycle over recursive mutexes reports 2 inversions at spin count 0 and 0 at spin count 2000.
b	Root cause explained?	Yes. Order is nc->mu, then nc->subsMu, then sub->mu. js_maybeFetchMore published the pull request while holding the subscription and dispatcher locks, so sub->mu was held while natsConn_publish took nc->mu, closing a cycle against natsConn_subscribeImpl (nc->mu then the subscription lock) and natsConn_processMsg (nc->subsMu then the subscription lock). js_PullSubscribeAsync made the same edge a second way by calling js_maybeFetchMore while already holding both locks; natsMutex is PTHREAD_MUTEX_RECURSIVE, so that contract violation was silent.
c	Fix matches root cause?	Yes. The inverted edge is deleted, not guarded: the publish moves outside the locks and the subscribe path releases before fetching. No new mutex, no lock widening, no defensive check at the failure site, and no ClickHouse-side workaround. It is a verbatim cherry-pick of the upstream fix 7220e9f6e081 (nats-io#834, fixes nats-io#823).
d	Test intent preserved / new tests added?	Yes. Upstream's test_JetStream_GH823 is added and registered in test/list_test.txt, extended so it counts delivered messages instead of discarding them, and a second test test_JetStream_GH823CustomInboxPrefix pins the reply-subject sizing half of the fix. No existing test is removed, weakened or retimed. The thirteen other JetStream tests, covering the rest of the pull and pull-async family, pass unchanged on both trees.
e	Both directions demonstrated?	Yes, on three oracles with distinct binaries by sha256. Sanitizer with spinning off: unpatched 2 inversions on 5 of 5 runs, fixed 0 on 5 of 5. The extended test's own assertion: unpatched fails sum >= 6 on 5 of 5, and still fails it when the wait is widened from 2000 ms to 30000 ms, so the refetch there does not merely run late. Sanitizer with spinning on: unpatched deadlocks on 4 of 5 runs, fixed on 0 of 5. A partial-fix arm is included as a sensitivity control and still reports the inversion 5 of 5, so the clean result is a property of the fix rather than of the oracle going quiet. Without a sanitizer the deadlock is probabilistic, so there the extended test reddens on 1 of 5 unpatched runs while the custom-prefix test reddens on all of them.
f	Fix is general across code paths?	Yes. A sweep of all 54 subscription-lock critical sections in src/ found exactly two members of this class, js_maybeFetchMore and js_PullSubscribeAsync, and both are fixed, which is why the second hunk is not optional. One inversion of a different class is left alone deliberately: applyNewSID drops only sub->mu before taking nc->subsMu, but that is a different mutex pair, arises only for a shared dispatcher, and only for ordered push consumers, which ClickHouse does not use. The stan layer has similar edges and is not compiled into ClickHouse.
g	Fix generalizes across inputs?	Yes, and the backport is broader than a minimal patch here. Replacing the fixed-size jsFetch::replySubject with a buffer sized from the subscription's subject also fixes a latent truncation: the old size assumed the default inbox prefix, but natsOptions_SetCustomInboxPrefix accepts any valid prefix with no length bound, so a long prefix made snprintf silently truncate the reply subject and fetched messages went to a subject the subscription does not match. The new unlocked strlen(sub->subject) is safe on this path because the only post-creation writer is the ordered-consumer reset, and ordered consumers are structurally rejected in pull mode. test_JetStream_GH823CustomInboxPrefix covers it with a 96 character prefix, which nats_IsSubjectValid accepts because it applies no length bound: it passes on the fixed tree 5 of 5 and fails 5 of 5 on a tree that differs only by restoring the fixed-size field, where test_JetStream_GH823 still passes.
h	Backward compatible?	Yes. No setting, no serialization format, no experimental gate, so no SettingsChangesHistory.cpp entry. jsFetch lives in the private, non-installed src/natsp.h, so removing a field breaks no ABI for library users, and _publishPullRequest is static. No deliberate break, so no maintainer sign-off is required.
i	Invariants and contracts preserved?	Yes. js_maybeFetchMore's own documented contract that neither lock is held is now honoured instead of silently violated. jsi->inFetch and jsi->fetchID are still set before the request is published, so the dispatcher can never see a reply for a fetch id it has not recorded. fetch->requestedMsgs has one writer and two readers and stays lock-protected on every path, so the widened publish-to-increment window introduces no race. The reads now taken outside the lock are sub->conn and jsi->nxtMsgSubj, both effectively immutable once a fetch can run. On the error path the allocation failure returns before any lock is taken, and natsSubscription_Destroy on the subscribe failure path now runs with the lock released, which is strictly safer. All state here is in memory, so there is no durability or restart surface.

Session id: cron:clickhouse-impl-slot-5:20260902-010900

[groeneai]

A shorter cycle through the same inverted edge, now firing on ClickHouse master

The reachability argument in the description is a table subscribing a second consumer or subject while the first is fetching. Since 2026-09-06 the same edge closes a two-lock cycle on a single-consumer, single-subject table, and it fires on master rather than on a PR arm.

• Arm B, the violator this PR removes: js_maybeFetchMore takes nats_lockSubAndDispatcher(sub) (js.c:2930) and holds it across _sendPullRequest (js.c:2938), which reaches natsConn_Lock (pub.c:86). The dispatcher calls it at dispatch.c:379 after every delivered message that is not the last of a fetch, and js_PullSubscribeAsync always installs a NextHandler when the caller passes none (js.c:3051), so the early return never applies to ClickHouse.
• Arm A, order-conforming: natsSubscription_DrainTimeout reaches natsConn_unsubscribe, which takes natsConn_Lock (conn.c:3151) and, still holding it, calls natsSub_startDrain (conn.c:3204), which takes natsSub_Lock (sub.c:936).

So one table that unsubscribes while the broker is delivering is enough. Both threads block on a plain natsMutex_Lock, so the 5 s drain timeout never applies.

What it costs, from Integration tests (amd_msan, 4/8) on 261c959b483c (job 7329 s against a 7200 s bound):

18:30:01.739  x8  the dispatcher NAKs into a finished queue; last NATS line the server ever writes
18:31:02.856      DROP DATABASE test reaches the NATS table and never returns
18:41:03 ... 20:11:10   ten further DROP DATABASE IF EXISTS test SYNC, one per 600 s client timeout
20:21:10          job killed; the server stayed wedged for 110 minutes

That is 11 of 11 failures on the shard, NATS taking 44.1 % of its test seconds against a 12 to 16 % baseline on 14 other commits of the same day, and 6 tests never run. Integration tests (amd_tsan, 2/6) on cbbd5c1e4f0f has the same shape: 12 hangs, 41.0 %, 10 tests never run. Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=261c959b483c89e63dc75aad77bb1b7ec813ef9d&name_0=MasterCI

_flushAndDrain is not a third arm: it releases sub->mu before natsConnection_Flush (sub.c:869), so removing the edge in js_maybeFetchMore closes this cycle as well.