Skip to content

Commit 9fd80d2

Browse files
bluestreak01claude
andcommitted
fix(qwp): report connect_timeout, not auth_timeout, on a connect-phase timeout
QwpQueryClient.runUpgradeWithTimeout wrapped connect() and upgrade() in one try block, so a connect_timeout overage -- the timeout-flagged HttpClientException from doConnect()'s CONNECT_TIMEOUT path -- was caught by the isTimeout() branch meant for upgrade() and rewritten as "WebSocket upgrade to host:port exceeded auth_timeout=<authTimeoutMs>ms". A user with connect_timeout=500 and auth_timeout_ms=15000 saw, after ~500ms, an error blaming a 15000ms auth timeout (wrong phase and wrong value). Move connect() outside the upgrade try so the auth_timeout rewrite only applies to genuine upgrade-phase timeouts; connect-phase failures propagate with their own "connect timed out ..." message. The failover walk is unchanged (the exception is still a transport error and the next endpoint is tried). The ingest side (QwpWebSocketSender) was already correct -- it routes through QwpUpgradeFailures.classify, which leaves the connect-timeout exception unmodified. Add QwpQueryClientConnectTimeoutTest: a TEST-NET-1 blackhole connect with connect_timeout < auth_timeout must report connect_timeout, not auth_timeout. It skips gracefully when the runner has no route to the blackhole, mirroring NetConnectTimeoutTest. Verified it fails on the pre-fix code with the exact misreported message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 7491d95 commit 9fd80d2

2 files changed

Lines changed: 98 additions & 1 deletion

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpQueryClient.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1771,12 +1771,21 @@ private void reconnectViaTracker() {
17711771
}
17721772

17731773
private void runUpgradeWithTimeout(Endpoint ep) {
1774+
// Connect first, OUTSIDE the upgrade try. A connect-phase failure --
1775+
// including a connect_timeout overage flagged via flagAsTimeout() -- must
1776+
// keep its own message ("connect timed out ...") and must NOT be relabeled
1777+
// as an auth_timeout overage below. doConnect() tears down its own socket
1778+
// on failure; the failover walker treats the propagated HttpClientException
1779+
// as a transport error and moves on to the next endpoint.
1780+
webSocketClient.connect(ep.host, ep.port);
1781+
17741782
int timeoutMs = (int) Math.min(authTimeoutMs, Integer.MAX_VALUE);
17751783
try {
1776-
webSocketClient.connect(ep.host, ep.port);
17771784
webSocketClient.upgrade(DEFAULT_ENDPOINT_PATH, timeoutMs, authorizationHeader);
17781785
} catch (HttpClientException ex) {
17791786
if (ex.isTimeout()) {
1787+
// Reachable only for an upgrade/auth-phase timeout now, so the
1788+
// auth_timeout attribution is accurate.
17801789
HttpClientException timeout = new HttpClientException("WebSocket upgrade to ")
17811790
.put(ep.host).put(':').put(ep.port)
17821791
.put(" exceeded auth_timeout=").put(authTimeoutMs).put("ms");
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
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;
26+
27+
import io.questdb.client.cutlass.http.client.HttpClientException;
28+
import io.questdb.client.cutlass.qwp.client.QwpQueryClient;
29+
import org.junit.Assert;
30+
import org.junit.Assume;
31+
import org.junit.Test;
32+
33+
public class QwpQueryClientConnectTimeoutTest {
34+
35+
/**
36+
* A connect-phase timeout must be reported as a connect_timeout failure, not
37+
* relabeled as an "exceeded auth_timeout" overage.
38+
* <p>
39+
* {@code QwpQueryClient.runUpgradeWithTimeout} used to wrap the {@code connect()}
40+
* and {@code upgrade()} calls in one try block, so the timeout-flagged exception
41+
* thrown by the (in-diff) connect_timeout path was caught by the {@code isTimeout()}
42+
* branch intended for upgrade() and rewritten with the (much larger, and wrong)
43+
* auth_timeout value -- e.g. a connect that bailed after 500 ms reported
44+
* "exceeded auth_timeout=15000ms". The ingest side never had this because it
45+
* routes through {@code QwpUpgradeFailures.classify}, which leaves the
46+
* connect-timeout exception unmodified.
47+
*/
48+
@Test(timeout = 30_000)
49+
public void testConnectTimeoutNotReportedAsAuthTimeout() {
50+
// 192.0.2.0/24 is TEST-NET-1 (RFC 5737): on a normal network the SYN is
51+
// silently dropped, so the TCP connect stalls and our application-level
52+
// connect_timeout (500 ms) fires -- long before auth_timeout_ms (15000 ms).
53+
// The WebSocket upgrade phase is never reached.
54+
try (QwpQueryClient client = QwpQueryClient.fromConfig(
55+
"ws::addr=192.0.2.1:9009;connect_timeout=500;auth_timeout_ms=15000;failover=off;target=any;")) {
56+
long start = System.currentTimeMillis();
57+
try {
58+
client.connect();
59+
Assert.fail("expected connect to fail");
60+
} catch (HttpClientException ex) {
61+
long elapsed = System.currentTimeMillis() - start;
62+
String msg = ex.getMessage();
63+
64+
// The connect_timeout path is only exercised when the runner routes
65+
// TEST-NET-1 into a black hole (dropped SYN). Skip -- rather than
66+
// flake -- on the other two outcomes:
67+
// - no route: a fast ENETUNREACH surfaces as "could not connect".
68+
// - (rare) the host accepts the connect: the upgrade then runs the
69+
// full auth_timeout, so elapsed ~ auth_timeout (>5 s).
70+
// Neither gate keys on the connect-vs-auth label, so neither can mask
71+
// the regression: a black-holed connect always bails at ~500 ms with
72+
// a message that is "connect timed out" (fixed) or "...auth_timeout..."
73+
// (the bug) -- both reach the assertions below.
74+
Assume.assumeFalse("no route to TEST-NET-1 black hole on this runner: " + msg,
75+
msg.contains("could not connect"));
76+
Assume.assumeTrue("TEST-NET-1 is not a black hole on this runner (elapsed=" + elapsed + "ms): " + msg,
77+
elapsed < 5_000);
78+
79+
// It bailed at connect_timeout=500 ms, nowhere near auth_timeout=15000 ms.
80+
// Regression: name the connect phase, never auth_timeout.
81+
Assert.assertFalse("connect-phase timeout misreported as auth_timeout: " + msg,
82+
msg.contains("auth_timeout"));
83+
Assert.assertTrue("expected a connect-timeout diagnostic, got: " + msg,
84+
msg.contains("connect timed out"));
85+
}
86+
}
87+
}
88+
}

0 commit comments

Comments
 (0)