Skip to content

Commit 0277a32

Browse files
committed
fix(qwp): cursor I/O loop dies unlatched when the reconnect machinery throws
A RuntimeException escaping fail()/connectLoop pierced ioLoop's catch-all and killed the I/O thread with running still true and no terminal latched: checkError() stayed clean and the producer kept buffering into SF against a dead loop -- the exact silent stall Invariant B exists to prevent. The concrete trigger was the retry-loop jitter: ThreadLocalRandom.nextLong throws IAE on a bound <= 0, and the loop constructors (unlike the builder) do not validate the backoff params, so direct construction with a non-positive backoff turned the first backoff into an unlatched thread death. Fixed twice over: - clamp both jitter bounds with Math.max(1L, ...), mirroring BackgroundDrainer's sweep-loop guard; - guard the fail(t) call in ioLoop's catch: any secondary failure now latches a terminal (latch-and-die, like the Error arm) with the original failure attached as suppressed, instead of escaping. recordFatal is first-writer-wins, so a terminal already latched inside connectLoop is preserved. CursorWebSocketSendLoopZeroBackoffTest is red pre-fix on both counts: the I/O thread died after 2 attempts (IAE: bound must be positive) with no terminal latched, and connectWithRetry surfaced the raw IAE instead of budget exhaustion.
1 parent 104b1bb commit 0277a32

2 files changed

Lines changed: 171 additions & 3 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,10 @@ public static WebSocketClient connectWithRetry(
505505
lastLogNanos = now;
506506
}
507507
}
508-
long jitter = ThreadLocalRandom.current().nextLong(backoffMillis);
508+
// Math.max guards nextLong's bound>0 contract (IAE on <= 0)
509+
// against non-builder callers -- builder-validated config keeps
510+
// this > 0. Mirrors BackgroundDrainer's sweep-loop jitter guard.
511+
long jitter = ThreadLocalRandom.current().nextLong(Math.max(1L, backoffMillis));
509512
long sleepMillis = backoffMillis + jitter;
510513
long remainingMillis = (deadlineNanos - System.nanoTime()) / 1_000_000L;
511514
if (remainingMillis <= 0) {
@@ -1171,7 +1174,13 @@ private void connectLoop(Throwable initial, String phase) {
11711174
}
11721175
}
11731176
if (running) {
1174-
long jitter = ThreadLocalRandom.current().nextLong(backoffMillis);
1177+
// Math.max guards nextLong's bound>0 contract (IAE on <= 0):
1178+
// an IAE here would climb through fail() into ioLoop's
1179+
// secondary-failure guard and latch a terminal -- correct but
1180+
// needlessly fatal for a jitter computation. Builder-validated
1181+
// config keeps this > 0; the guard covers direct constructor
1182+
// use. Mirrors BackgroundDrainer's sweep-loop jitter guard.
1183+
long jitter = ThreadLocalRandom.current().nextLong(Math.max(1L, backoffMillis));
11751184
long sleepMillis = backoffMillis + jitter;
11761185
LockSupport.parkNanos(sleepMillis * 1_000_000L);
11771186
backoffMillis = Math.min(backoffMillis * 2, reconnectMaxBackoffMillis);
@@ -1382,7 +1391,28 @@ private void ioLoop() {
13821391
recordFatal(t);
13831392
throw (Error) t;
13841393
}
1385-
fail(t);
1394+
try {
1395+
fail(t);
1396+
} catch (Throwable secondary) {
1397+
// fail()/connectLoop itself threw. Without this guard the
1398+
// secondary failure escapes ioLoop's catch and kills the I/O
1399+
// thread with `running` still true and no terminal latched:
1400+
// checkError() stays clean and the producer keeps buffering
1401+
// into SF against a dead loop -- the exact silent stall
1402+
// Invariant B exists to prevent. Latch-and-die like the Error
1403+
// arm above instead; the finally still counts down the
1404+
// shutdown latch, so close() cannot hang. recordFatal is
1405+
// first-writer-wins, so a terminal already latched inside
1406+
// connectLoop (e.g. a throw from dispatchError after
1407+
// recordFatal) is preserved.
1408+
if (secondary != t) {
1409+
secondary.addSuppressed(t);
1410+
}
1411+
recordFatal(secondary);
1412+
if (secondary instanceof Error) {
1413+
throw (Error) secondary;
1414+
}
1415+
}
13861416
} finally {
13871417
// Last act of the I/O thread: dispose of whatever client it
13881418
// holds. This is the airtight half of the close()-vs-reconnect
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
/*+*****************************************************************************
2+
* ___ _ ____ ____
3+
* / _ \ _ _ ___ ___| |_| _ \| __ )
4+
* | | | | | | |/ _ \/ __| __| | | | _ \
5+
* | |_| | |_| | __/\__ \ |_| |_| | |_) |
6+
* \__\_\\__,_|\___||___/\__|____/|____/
7+
*
8+
* Copyright (c) 2014-2019 Appsicle
9+
* Copyright (c) 2019-2026 QuestDB
10+
*
11+
* Licensed under the Apache License, Version 2.0 (the "License");
12+
* you may not use this file except in compliance with the License.
13+
* You may obtain a copy of the License at
14+
*
15+
* http://www.apache.org/licenses/LICENSE-2.0
16+
*
17+
* Unless required by applicable law or agreed to in writing, software
18+
* distributed under the License is distributed on an "AS IS" BASIS,
19+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
20+
* See the License for the specific language governing permissions and
21+
* limitations under the License.
22+
*
23+
******************************************************************************/
24+
25+
package io.questdb.client.test.cutlass.qwp.client.sf.cursor;
26+
27+
import io.questdb.client.cutlass.line.LineSenderException;
28+
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
29+
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop;
30+
import org.junit.Assert;
31+
import org.junit.Rule;
32+
import org.junit.Test;
33+
import org.junit.rules.TemporaryFolder;
34+
35+
import java.io.IOException;
36+
37+
/**
38+
* Regression guard for the I/O-thread silent-death hazard in
39+
* {@link CursorWebSocketSendLoop}: a {@code RuntimeException} escaping
40+
* {@code fail()}/{@code connectLoop} pierced {@code ioLoop}'s catch-all and
41+
* killed the I/O thread with {@code running} still {@code true} and no
42+
* terminal latched — the producer kept buffering into SF against a dead loop,
43+
* the exact silent stall Invariant B exists to prevent.
44+
* <p>
45+
* The concrete trigger was the retry-loop jitter:
46+
* {@code ThreadLocalRandom.nextLong(backoffMillis)} throws
47+
* {@code IllegalArgumentException} when the bound is {@code <= 0}. The
48+
* builder validates {@code reconnect_initial_backoff_millis > 0}, but the
49+
* loop constructors are public and do not — direct construction with a
50+
* non-positive backoff (as this test does) turned the very first backoff
51+
* into an unlatched thread death. Fixed twice over: the jitter bounds are
52+
* clamped with {@code Math.max(1L, …)} (mirroring {@code BackgroundDrainer}'s
53+
* sweep loop), and {@code ioLoop}'s catch guards the {@code fail(t)} call so
54+
* any residual secondary failure latches a terminal (latch-and-die) instead
55+
* of escaping.
56+
*/
57+
public class CursorWebSocketSendLoopZeroBackoffTest {
58+
59+
@Rule
60+
public final TemporaryFolder sfDir = TemporaryFolder.builder().assureDeletion().build();
61+
62+
/**
63+
* Pins the static bounded-connect variant ({@code connectWithRetry},
64+
* user-thread blocking initial connect): a non-positive initial backoff
65+
* must exhaust the budget and surface the real (transport) failure as a
66+
* {@link LineSenderException} — not blow up in the jitter computation
67+
* with an {@code IllegalArgumentException}.
68+
*/
69+
@Test(timeout = 30_000)
70+
public void connectWithRetryToleratesNonPositiveBackoff() {
71+
try {
72+
CursorWebSocketSendLoop.connectWithRetry(
73+
() -> {
74+
throw new IOException("connection refused (test)");
75+
},
76+
50L, // budget: fail fast
77+
0L, // non-positive initial backoff: pre-fix IAE in nextLong
78+
1L,
79+
"test-connect");
80+
Assert.fail("expected LineSenderException on budget exhaustion");
81+
} catch (LineSenderException expected) {
82+
Assert.assertTrue(
83+
"budget exhaustion should carry the attempt summary, got: "
84+
+ expected.getMessage(),
85+
expected.getMessage().contains("failed after"));
86+
}
87+
}
88+
89+
/**
90+
* Pre-fix: the I/O thread walked the async-initial-connect retry loop,
91+
* hit {@code nextLong(0)} on the first backoff, and the IAE — re-thrown
92+
* once more from the {@code fail(t)} re-entry — escaped {@code ioLoop}'s
93+
* catch. The thread died after at most two recorded attempts with
94+
* {@code isRunning() == true} and {@code getTerminalError() == null}:
95+
* invisible to the producer. Post-fix the clamped jitter keeps the loop
96+
* retrying indefinitely (Invariant B), so attempts keep climbing and no
97+
* terminal is latched.
98+
*/
99+
@Test(timeout = 30_000)
100+
public void zeroBackoffConnectFailuresKeepRetryingInsteadOfKillingIoThread() throws Exception {
101+
try (CursorSendEngine engine = new CursorSendEngine(
102+
sfDir.getRoot().getAbsolutePath(), 16_384)) {
103+
CursorWebSocketSendLoop loop = new CursorWebSocketSendLoop(
104+
null, engine, 0, 1_000_000L,
105+
() -> {
106+
// Plain connect failure: retried forever under
107+
// Invariant B — never terminal, never latched.
108+
throw new IOException("connection refused (test)");
109+
},
110+
0,
111+
0, // non-positive initial backoff: pre-fix IAE in nextLong
112+
1);
113+
try {
114+
loop.start();
115+
// Post-fix the zero-backoff loop records attempts at a very
116+
// high rate; pre-fix the count freezes at <= 2 when the IAE
117+
// kills the thread, and this poll times out.
118+
long deadlineNanos = System.nanoTime() + 20_000_000_000L;
119+
while (loop.getTotalReconnectAttempts() < 5
120+
&& System.nanoTime() < deadlineNanos) {
121+
Thread.yield();
122+
}
123+
Assert.assertTrue(
124+
"I/O thread stopped retrying after "
125+
+ loop.getTotalReconnectAttempts()
126+
+ " attempts -- it died without latching a terminal "
127+
+ "(the Invariant B silent stall)",
128+
loop.getTotalReconnectAttempts() >= 5);
129+
Assert.assertTrue("loop must still be running", loop.isRunning());
130+
Assert.assertNull(
131+
"plain connect failures must not latch a terminal (Invariant B)",
132+
loop.getTerminalError());
133+
} finally {
134+
loop.close();
135+
}
136+
}
137+
}
138+
}

0 commit comments

Comments
 (0)