Retry the socket wait when poll() is interrupted by a signal

[groeneai]

natsSock_WaitReady reports every poll() failure as NATS_IO_ERROR, including EINTR, so one asynchronous signal delivered during a connect or handshake wait fails the whole connection attempt.

poll is never restarted after a signal handler runs, whatever SA_RESTART says (signal(7)), so EINTR here says nothing about the socket. Any embedder that delivers periodic per-thread signals reaches this. ClickHouse does by default: it arms a 10 s per-thread profiler timer (SIGUSR1, SIGEV_THREAD_ID) on every thread from its global pool, its first fire placed at random inside that first period, and it runs the NATS event loop on such a thread. When the signal lands in the connect window, natsSock_ConnectTcp discards the fd and CREATE TABLE ... ENGINE = NATS fails with

Cannot connect to Nats last error: (unix/sock.c:56): poll error: 4

errno 4 is EINTR. ClickHouse CI hit it three times in 3h35m on 2026-09-14, in both tests of a new integration module and on three different builds: on master in an amd_asan_ubsan run, and on two pull requests under arm_binary and amd_llvm_coverage, the third green only because a retry passed, so it is neither sanitizer- nor x86-specific. That module sets nats_startup_connect_tries=1, which turns one interrupted poll into a failed DDL rather than a retried connect.

Fix: the poll now sits in a loop that recomputes natsDeadline_GetTimeout(deadline) on each iteration and continues on EINTR. The remaining time bounds it: an inactive deadline yields -1 (wait forever, the requested semantics) and an expired one 0, so poll returns 0 and the pre-existing NATS_TIMEOUT arm fires as before. Nothing changes on a path that does not see EINTR. This is the only poll or select in the non-Windows sources, and it is reached both for connect completion and for the handshake read.

Test: two arms in the existing test_natsWaitReady, both waiting under a storm thread that pthread_kills SIGALRM at the waiting thread about every millisecond, and both asserting a delivery counter, so a run that received no signal cannot pass. One requires NATS_OK inside the existing 450-600 ms no-deadline bound, which pins progress across ~460 interruptions; the other requires NATS_TIMEOUT inside 40-100 ms, which pins a recomputed rather than re-armed deadline. Both fail before this change on the same poll error: 4 leaf and pass 10/10 after. On a real ClickHouse server against a silent listener, the message appears in exactly one of four cells: unpatched, profiler on.

The same function is unpatched upstream in nats-io/nats.c main. ClickHouse's pin needs a bump to pick this up; ClickHouse/ClickHouse#119867 already moves it to this commit's parent.

What I deliberately did not change, and levers for you

SSL_do_handshake (conn.c:749) is the one sibling structurally reachable with a blocking fd, since _makeTLSConn sets blocking at :677 and restores it at :760. Different call site, different retry contract, not reproduced, so left alone.

natsSock_Read/natsSock_Write also report an EINTR from recv/send as a fatal NATS_IO_ERROR. Their SSL arms cannot see one, because the TLS blocking window above contains no natsSock_Read/Write call. Their plain arms can, but only in the mode ClickHouse does not use: _processConnInit (conn.c:1983) switches the fd to blocking when writeDeadline <= 0, and _spinUpSocketWatchers then runs _readLoop on it. With an external event loop, which ClickHouse always sets, :1992 restores non-blocking and no _readLoop is started, so a transfer returns EWOULDBLOCK into the function fixed here instead. Not reproduced either, so also left alone. Say the word if you want either covered.

ClickHouse/v3.13.0 carries the identical unpatched function; I can open the same PR against that branch if you want both lines fixed.

On the ClickHouse side, dropping nats_startup_connect_tries=1 from the two rotation fixtures would restore the shipped default of 5 attempts, and since the profiler interval is 10 s while five attempts complete in milliseconds, one interrupt cannot reach more than one of them. I have not done it, because it papers over this bug and drops coverage of a real setting value.

natsSock_Flush's fsync has no callers.

[groeneai]

Internal second-model review

An independent reviewer (different model, fresh context) and a separate cold code review ran over
this change before it was published, four times over four rounds. Verdicts below. The reviewer's
change map had to be assembled from the ClickHouse superproject against its contrib/nats-io pin,
which is four commits behind this PR's base, so it also saw two already merged fork PRs; findings
are attributed by hunk for that reason.

❌ No positive-path oracle for the retry (agreed, fixed). Every assertion in the first version of
the new test_natsWaitReady case was NATS_TIMEOUT on a socket kept unreadable, so nothing proved
that an interrupted wait still observes readiness and returns NATS_OK. An arm that waits under the
signal storm and succeeds was added.

❌ The signal storm was process-directed (agreed, fixed). ITIMER_REAL delivers SIGALRM to any
unblocked thread and nats_Sleep is usleep() with no EINTR loop, so a signal landing on the
test's fake-server thread shortened its sleep. The storm now targets the waiting thread with
pthread_kill, and each storm arm asserts that the handler actually ran, so a failed setup cannot
make the case pass as a duplicate of the unsignalled one.

❌ The storm's stop flag was unsynchronised (agreed, fixed). It was a volatile bool written by
the test thread and polled by the storm thread. test/CMakeLists.txt arms
TSAN_OPTIONS=...halt_on_error=1 for every test under NATS_SANITIZE, so that would abort
natsWaitReady in a sanitizer build of the suite. It is now guarded by a natsMutex, matching the
rest of this file. Reverting only that guard reproduces the race 5/5 under -fsanitize=thread.

⚠️ The storm outlived the wait, so the failure it exists to catch was a stall rather than an
assertion failure (agreed, fixed). Measured both ways: with the storm's own 1500 ms budget, an
implementation that re-arms the full 50 ms on each retry fails the deadline arm on duration at
1549 ms; without the budget the same implementation stalls to 9949 ms and then returns NATS_OK
when the fake server tears the connection down, so the arm would have failed on the wrong term.
The budget keeps it an assertion about the deadline.

⚠️ Two comments claimed more than the code does (agreed, reworded). One described the constant
as stopping a wait that never returns, which the counterfactual above disproves; the other said a
re-arming retry never returns under a continuous storm. Both now state the measured bound.

⚠️ The third CI occurrence cited in the description is a green check (agreed, disclosed). Its
recorded context still carries the poll error: 4 leaf, but the integration runner's retry passed,
so the description and the commit message now say so rather than leaving a reviewer to click into an
apparently green run. The first two occurrences are genuine reds.

💡 Redundant pfd.revents = 0 inside the retry loop (agreed, removed). poll() writes revents
on every call and this function never reads it.

✅ "The submodule range ships the libuv and JetStream fixes too, so wait for the pin bump"
(disagreed three times, with the ancestry measured each time). This PR publishes no gitlink change
and does not depend on the ClickHouse-side pin bump: 37532c3246461 and b41317aede9e2 are both
ancestors of this PR's base branch tip, so those two merges are in the base and cannot appear in
this diff. rev-list --count <base>..<head> is 1, the diff is two files, +140/-3, and it contains
no js.c and no libuv.h. In the superproject, rev-list --count origin/master..HEAD is 0.

The reviewer's remaining findings are against src/js.c and src/adapters/libuv.h content that is
already merged in this fork and sits in this PR's base rather than in its diff, plus one against a
PR template this repository does not have. I have not verified the former and am not restating them
as defects here; I recorded their pointers against the pin-bump work instead of folding unrelated
concurrency changes into this PR.

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. ctest -R '^natsWaitReady$' in an out-of-tree build of this repo fails on the unpatched library with Error: 3 - IO Error - (unix/sock.c:56): poll error: 4, the verbatim leaf all three CI occurrences carry (3/3 runs, both storm arms).
b	Root cause explained?	Yes. A periodic per-thread signal (ClickHouse's default 10 s profiler timer, SIGUSR1 via SIGEV_THREAD_ID, first fire uniformly random inside the first period) lands during the connect or handshake wait; poll is never restarted after a handler runs, so it returns EINTR; natsSock_WaitReady reports any poll failure as NATS_IO_ERROR; natsSock_ConnectTcp discards the fd, and with nats_startup_connect_tries=1 the embedder's DDL fails.
c	Fix matches root cause?	Yes. It makes the interrupted wait continue against the remaining deadline, at the function that misclassifies EINTR. Not a guard on the caller, not a change to the test, not a change to the embedder's thread type. The cheap test-side lever is offered to the maintainer in the description instead of being taken silently.
d	Test intent preserved / new tests added?	Yes. Two arms inside the existing test_natsWaitReady rather than a new file, one per scenario: an interrupted wait must still make PROGRESS (no deadline, returns NATS_OK at 500 ms after some 460 delivered signals), and it must still honour its DEADLINE (50 ms, NATS_TIMEOUT inside 40-100 ms under some 46). Both assert a signal-receipt counter, so a failed sigaction, a failed mutex or thread create, or undelivered signals fail the case instead of degrading it to an unsignalled wait. The storm stops by itself after 1500 ms, so the arm stays an assertion rather than becoming a stall against a wrong implementation. Pre-existing cases still pass, and Windows keeps the upstream unsignalled case verbatim.
e	Both directions demonstrated?	Yes, five ways. Library: both arms FAIL unpatched with the leaf above while the unsignalled case still passes as a control, and PASS 10/10 patched. Mutation on the storm: with pthread_kill removed, both arms FAIL on the receipt counter alone, so neither can pass vacuously. Mutation on the fix: with the deadline recompute hoisted back out of the loop, so every retry re-arms the full 50 ms, the deadline arm FAILS on duration at 1549 ms, bounded, which is the specific wrong implementation this arm exists to reject. Sanitizer: natsWaitReady is clean 5/5 under -fsanitize=thread with the project's own halt_on_error=1 options, and reverting only the mutex reproduces a _testStormStop data race 5/5 that kills the case, so that direction is not vacuous either. End to end against a real ClickHouse server and a silent listener: a 4-cell matrix over (patched, unpatched) x (profiler 1 ms, profiler off) in which poll error: 4 appears in exactly one cell, with the build id reported by each running server and the NATS_TIMEOUT source line differing per binary.
f	Fix is general across code paths?	Yes. This is the only poll, select, ppoll or epoll_wait in the non-Windows sources. The TLS arms of natsSock_Read/Write are unreachable with a blocking fd (the blocking window conn.c:677 to :760 contains no such call); their plain arms see a blocking fd only in the no-event-loop mode (conn.c:1983) that the embedder never uses, so here they run non-blocking and funnel EWOULDBLOCK into the function fixed here; natsSock_Flush has no callers; win/sock.c's select is not built by the embedder and WSAEINTR does not arise from signal delivery. SSL_do_handshake is the one structurally reachable sibling and is named in the description as a deliberate, not reproduced, decline.
g	Fix generalizes across inputs?	Yes. The retry sits after the waitMode switch, so WAIT_FOR_READ, WAIT_FOR_WRITE and WAIT_FOR_CONNECT behave alike. The real input dimension is the deadline, and both values are permanent test arms: active (expires at 50 ms under about 46 signals) and inactive, timeout = -1 (returns NATS_OK at 500 ms with the elapsed time unchanged, so the infinite wait neither hangs nor returns early). An already expired deadline gives natsDeadline_GetTimeout = 0, so poll returns 0 and the pre-existing NATS_TIMEOUT arm fires; the loop cannot spin.
h	Backward compatible?	Yes, and there is nothing to be compatible with: no new symbol, no signature change, no option, no default, no wire or file format. The only observable difference is that a wait interrupted by a signal continues instead of failing, which no caller can have relied on.
i	Invariants and contracts preserved?	Yes. Same three return statuses, same total wait, NATS_TIMEOUT still exactly at deadline expiry, because the timeout is recomputed each iteration rather than re-armed. The loop is bounded by the deadline it was given. No lock, no shared state, no allocation, no new early-return path; pfd and the deadline pointer are per call. The test helper's own concurrency contract is explicit: the storm's stop flag is natsMutex-guarded, the thread is joined before its mutex is destroyed, and a failed mutex create becomes the arm's status instead of a crash in the stop path.

Session id: cron:clickhouse-impl-slot-4:20260915-012300

[groeneai]

Superseded: the retry landed on this branch as b88704df, and the test it carried is now on its own in #10, rebased onto that commit. Closing this one.