Skip to content

Commit 99e2ecd

Browse files
mtopolnikclaude
andcommitted
Prove sender-pool unwind in build-failure test
testQueryPoolBuildFailureUnwindsSenderPool previously asserted only that build() threw when the query pool could not connect. That cannot catch the bug it targets: if build() threw and also leaked the already-built sender pool's connected senders, the test still passed. TestWebSocketServer now tracks connections from the server's view. It increments a live counter when a handshake completes and decrements it when that connection's read thread exits (the client closed its socket), and keeps a monotonic handshake counter. The read loop runs inside a try/finally so the decrement fires on every exit path. The test now proves the unwind two ways: the server saw two ingest handshakes (the senders connected, so the next check is not vacuous), and the live connection count returns to zero after the failed build (the unwind closed every sender). The live count is polled because the server observes the client-side close asynchronously. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 4d75c79 commit 99e2ecd

2 files changed

Lines changed: 83 additions & 27 deletions

File tree

core/src/test/java/io/questdb/client/test/QuestDBBuilderTest.java

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.junit.Test;
3232

3333
import java.util.concurrent.TimeUnit;
34+
import java.util.function.BooleanSupplier;
3435

3536
public class QuestDBBuilderTest {
3637

@@ -274,9 +275,21 @@ public void testQueryPoolBuildFailureUnwindsSenderPool() throws Exception {
274275
.close();
275276
Assert.fail("expected build to fail when query pool cannot connect");
276277
} catch (RuntimeException expected) {
277-
// The exact exception comes from QwpQueryClient.connect(); the
278-
// build failing tells us the sender-pool unwind ran.
278+
// The exact exception comes from QwpQueryClient.connect(). The
279+
// build failing only proves the query pool gave up; the
280+
// assertions below prove the unwind closed the senders the
281+
// sender pool had already connected, rather than leaking them.
279282
}
283+
// The sender pool eagerly warmed senderPoolSize(2), so the server
284+
// saw two ingest handshakes (proving the senders connected and the
285+
// assertion below is not vacuous)...
286+
awaitTrue("sender pool should have connected two ingest senders",
287+
() -> ingest.handshakeCount() >= 2);
288+
// ...and the failed build() must have closed every one of them, so
289+
// no sender connection is left live on the server. The server
290+
// observes the client-side socket close asynchronously, so poll.
291+
awaitTrue("failed build() must close the already-built sender pool, leaving no live connection",
292+
() -> ingest.liveConnectionCount() == 0);
280293
}
281294
}
282295

@@ -396,4 +409,15 @@ private static void assertSchemaRejected(Runnable action) {
396409
Assert.assertTrue(e.getMessage(), e.getMessage().contains("ws or wss"));
397410
}
398411
}
412+
413+
private static void awaitTrue(String message, BooleanSupplier condition) throws InterruptedException {
414+
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5);
415+
while (System.nanoTime() < deadline) {
416+
if (condition.getAsBoolean()) {
417+
return;
418+
}
419+
Thread.sleep(20);
420+
}
421+
Assert.assertTrue(message, condition.getAsBoolean());
422+
}
399423
}

core/src/test/java/io/questdb/client/test/cutlass/qwp/websocket/TestWebSocketServer.java

Lines changed: 57 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import java.util.concurrent.CountDownLatch;
4747
import java.util.concurrent.TimeUnit;
4848
import java.util.concurrent.atomic.AtomicBoolean;
49+
import java.util.concurrent.atomic.AtomicInteger;
4950

5051
/**
5152
* A simple WebSocket server for client integration testing.
@@ -57,10 +58,19 @@ public class TestWebSocketServer implements Closeable {
5758
private final List<ClientHandler> clients = new CopyOnWriteArrayList<>();
5859
private final boolean emitDurableAckHeader;
5960
private final WebSocketServerHandler handler;
61+
// Count of WebSocket connections currently live from the server's view:
62+
// incremented when a handshake completes, decremented when that connection's
63+
// read thread exits (the client closed its socket). Lets a test assert that a
64+
// client-side pool actually closed the connections it opened.
65+
private final AtomicInteger liveConnections = new AtomicInteger();
6066
private final int port;
6167
private final AtomicBoolean running = new AtomicBoolean(false);
6268
private final ServerSocket serverSocket;
6369
private final CountDownLatch startLatch = new CountDownLatch(1);
70+
// Monotonic count of completed handshakes over the server's lifetime. Unlike
71+
// liveConnections it never decrements, so a test can confirm how many clients
72+
// connected even after they have all disconnected.
73+
private final AtomicInteger totalHandshakes = new AtomicInteger();
6474
private Thread acceptThread;
6575
// X-QuestDB-Role value to emit on handshake responses. null = omit the
6676
// header (legacy behavior for tests written before role-aware failover).
@@ -164,6 +174,22 @@ public int getPort() {
164174
return port;
165175
}
166176

177+
/**
178+
* Number of handshakes the server has completed over its lifetime
179+
* (monotonic; never decreases when clients disconnect).
180+
*/
181+
public int handshakeCount() {
182+
return totalHandshakes.get();
183+
}
184+
185+
/**
186+
* Number of WebSocket connections currently live from the server's view.
187+
* Drops back to zero once every client has closed its socket.
188+
*/
189+
public int liveConnectionCount() {
190+
return liveConnections.get();
191+
}
192+
167193
/**
168194
* Replaces the advertised role for subsequent handshakes (live update).
169195
*/
@@ -536,35 +562,41 @@ void start() {
536562
LOG.error("Handshake failed");
537563
return;
538564
}
565+
totalHandshakes.incrementAndGet();
566+
liveConnections.incrementAndGet();
539567

540-
if (sendServerInfo) {
541-
sendBinary(buildServerInfoFrame(roleByte(advertisedRole)));
542-
}
543-
544-
byte[] readBuf = new byte[8192];
545-
546-
while (running.get() && !isClosed) {
547-
int read;
548-
try {
549-
read = in.read(readBuf);
550-
} catch (SocketTimeoutException e) {
551-
continue;
552-
}
553-
if (read <= 0) {
554-
break;
568+
try {
569+
if (sendServerInfo) {
570+
sendBinary(buildServerInfoFrame(roleByte(advertisedRole)));
555571
}
556572

557-
// append to recvBuffer
558-
recvBuffer.compact();
559-
if (recvBuffer.remaining() < read) {
560-
// should not happen with 64k buffer in tests
561-
LOG.error("Receive buffer overflow");
562-
break;
573+
byte[] readBuf = new byte[8192];
574+
575+
while (running.get() && !isClosed) {
576+
int read;
577+
try {
578+
read = in.read(readBuf);
579+
} catch (SocketTimeoutException e) {
580+
continue;
581+
}
582+
if (read <= 0) {
583+
break;
584+
}
585+
586+
// append to recvBuffer
587+
recvBuffer.compact();
588+
if (recvBuffer.remaining() < read) {
589+
// should not happen with 64k buffer in tests
590+
LOG.error("Receive buffer overflow");
591+
break;
592+
}
593+
recvBuffer.put(readBuf, 0, read);
594+
recvBuffer.flip();
595+
596+
handleRead();
563597
}
564-
recvBuffer.put(readBuf, 0, read);
565-
recvBuffer.flip();
566-
567-
handleRead();
598+
} finally {
599+
liveConnections.decrementAndGet();
568600
}
569601
} catch (IOException e) {
570602
if (running.get()) {

0 commit comments

Comments
 (0)