Skip to content

Commit 9e0a756

Browse files
glasstigerclaude
andcommitted
Fix WS per-endpoint token pull and getToken doc
QwpWebSocketSender.buildAndConnect resolved the Authorization header inside its per-endpoint walk, so a token provider was queried once per endpoint per reconnect round. A throwing provider (a failed silent refresh, or not signed in) was then caught as a per-endpoint transport error, retried across every endpoint, and surfaced as "all endpoints unreachable" - masking the real auth failure and re-hammering the token endpoint with the same dead credential. Resolve the header once per (re)connect round, before the endpoint walk, and reuse it across a failover (a token is cluster-wide). A provider throw now propagates to connectWithRetry, which retries it within the reconnect budget - the documented streaming model - and surfaces the provider's own message. A close that races the round aborts before the possibly blocking token pull. This mirrors QwpQueryClient, which likewise resolves the credential once before its walk. Add a WebSocketTokenProviderTest case that proves the provider is queried once (not per endpoint) and its error surfaces instead of "all endpoints unreachable"; it fails against the pre-fix code. Also correct the OidcDeviceAuth.getToken() javadoc: the silent refresh is not bounded by httpTimeoutMillis end to end. That timeout bounds the send, response wait and body parse, but the preceding connection phase (DNS, TCP connect, TLS handshake) is bounded by the OS, so an unreachable token endpoint can stall the refresh for the OS TCP-connect timeout (~2 minutes), not 30s. The class already documented this for the token-store lock; the getToken() contract now matches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2ce1ca0 commit 9e0a756

3 files changed

Lines changed: 77 additions & 15 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/auth/OidcDeviceAuth.java

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,9 @@
8787
* lifetime (up to 30 minutes), so a concurrent {@link #signIn()} or {@link #clearCache()} blocks
8888
* behind it - but {@link #getToken()} never waits behind an interactive sign-in: it fails fast with an
8989
* {@link OidcAuthException} rather than stall a request/flush path (a needed silent refresh still runs,
90-
* bounded by {@link Builder#httpTimeoutMillis(int)} plus, with a coordinating {@link TokenStore}, a brief
91-
* cross-process lock wait - see {@link #getToken()}). To abort a waiting sign-in, call
90+
* each HTTP round-trip phase bounded by {@link Builder#httpTimeoutMillis(int)} - though an unreachable
91+
* endpoint's connect is bounded by the OS, not by it - plus, with a coordinating {@link TokenStore}, a brief
92+
* cross-process lock wait; see {@link #getToken()}). To abort a waiting sign-in, call
9293
* {@link #close()} from another thread; it signals the flow to stop, which then fails with an
9394
* {@link OidcAuthException} rather than polling until the device code expires. Cancellation is seen
9495
* between polls (within ~100ms while waiting out an interval); a poll already in flight is not
@@ -468,12 +469,16 @@ public String getAuthorizationHeaderValue() {
468469
* It does not wait behind an interactive {@link #signIn()} running on another thread (which would stall
469470
* the flush for the whole device-code lifetime): if such a sign-in holds the lock it fails fast, and the
470471
* caller should retry once the sign-in completes. It is not, however, instantaneous - when the cached
471-
* token has expired it makes one synchronous refresh round-trip to the token endpoint, bounded by
472-
* {@link Builder#httpTimeoutMillis(int)} (30s by default); when a {@link TokenStore} coordinates the
473-
* refresh across processes it may first wait briefly to acquire the store's per-identity lock (a few
474-
* seconds at most for {@link FileTokenStore}, then it proceeds without the lock) before that round-trip.
475-
* That is the "quick silent refresh" the {@code HttpTokenProvider} contract permits on the flush path,
476-
* not an unbounded interactive wait.
472+
* token has expired it makes one synchronous refresh round-trip to the token endpoint (and, with a
473+
* coordinating {@link TokenStore}, may first wait briefly to acquire the store's per-identity lock - a few
474+
* seconds at most for {@link FileTokenStore}, then it proceeds without the lock - before that round-trip).
475+
* The send, response wait and body parse of that round-trip are each bounded by
476+
* {@link Builder#httpTimeoutMillis(int)} (30s by default); the connection phase that precedes them - DNS
477+
* resolution, the TCP connect and the TLS handshake - is bounded by the OS, not by httpTimeoutMillis, so an
478+
* unreachable (black-holed) token endpoint can stall this refresh for the OS TCP-connect timeout (commonly
479+
* ~2 minutes on Linux) rather than 30s. That is the "quick silent refresh" the {@code HttpTokenProvider}
480+
* contract permits on the flush path, not an unbounded interactive wait - but a producer sizing backpressure
481+
* against this call should expect that OS-bounded connect stall, not a hard 30s cap.
477482
*
478483
* @return a non-null, non-empty token
479484
* @throws OidcAuthException if no token has been obtained yet, if the cached token expired and could

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

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,9 @@ public class QwpWebSocketSender implements Sender {
139139
// Yields the Authorization header value presented on each WebSocket upgrade. A constant for a
140140
// fixed token or Basic credential; for an httpTokenProvider it pulls a freshly refreshed token,
141141
// so the initial connect and every reconnect re-handshake carry the current token. May be null
142-
// when no auth is configured. Evaluated inside buildAndConnect's per-endpoint try, so a throwing
143-
// provider (e.g. a failed silent refresh) is handled as a connect failure rather than escaping.
142+
// when no auth is configured. Evaluated once per (re)connect round in buildAndConnect, before the
143+
// endpoint walk (not once per endpoint); a throwing provider propagates to the connectWithRetry
144+
// reconnect wrapper, which retries it within the reconnect budget and surfaces the provider's message.
144145
private final Supplier<String> authorizationHeaderSupplier;
145146
private final int autoFlushBytes;
146147
private final long autoFlushIntervalNanos;
@@ -2433,6 +2434,22 @@ private synchronized WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
24332434
HttpClientException terminalUpgradeError = null;
24342435
QwpIngressRoleRejectedException lastRoleReject = null;
24352436
Endpoint lastEndpoint = null;
2437+
// Honor a close/stop that raced this (re)connect before doing any work - a token pull can make a
2438+
// blocking network call - mirroring the per-endpoint check at the top of the walk below.
2439+
if (cursorSendLoop == null ? closed : !cursorSendLoop.isRunning()) {
2440+
throw new LineSenderException("sender closed during connect");
2441+
}
2442+
// Resolve the Authorization header ONCE per (re)connect round, before the endpoint walk. For an
2443+
// httpTokenProvider this queries the provider a single time - a fresh token per handshake round, as the
2444+
// javadoc documents - NOT once per endpoint: a token-provider failure is cluster-wide (a failed silent
2445+
// refresh, or not signed in), not a per-endpoint transport fault, so re-querying it per endpoint would
2446+
// hammer the token endpoint with the same dead credential and mislabel the failure as "all endpoints
2447+
// unreachable". A throw here propagates to the connectWithRetry reconnect wrapper, which retries it
2448+
// within the reconnect budget like any other connect failure and surfaces the provider's own message,
2449+
// so a transient failed refresh recovers and only a persistent one terminates the sender (this
2450+
// transport's documented reconnect model). Mirrors QwpQueryClient, which likewise resolves the
2451+
// credential once before its endpoint walk.
2452+
final String authHeader = authorizationHeaderSupplier == null ? null : authorizationHeaderSupplier.get();
24362453
while (true) {
24372454
if (cursorSendLoop == null ? closed : !cursorSendLoop.isRunning()) {
24382455
throw new LineSenderException("sender closed during connect");
@@ -2451,11 +2468,9 @@ private synchronized WebSocketClient buildAndConnect(ReconnectSupplier ctx) {
24512468
newClient.setQwpRequestDurableAck(requestDurableAck);
24522469
newClient.connect(ep.host, ep.port);
24532470
int upgradeTimeoutMs = (int) Math.min(authTimeoutMs, Integer.MAX_VALUE);
2454-
// Pull the current Authorization header for this handshake. For an httpTokenProvider
2455-
// this re-queries the provider, so a reconnect presents a freshly refreshed token. A
2456-
// provider that throws here (a failed silent refresh) is caught below as a connect
2457-
// failure for this endpoint and retried within the reconnect budget.
2458-
String authHeader = authorizationHeaderSupplier == null ? null : authorizationHeaderSupplier.get();
2471+
// Present the header resolved once above for this handshake. On a failover to a later endpoint
2472+
// the same round's token is reused (a token is cluster-wide), so the provider is queried once
2473+
// per reconnect round, not once per endpoint.
24592474
newClient.upgrade(WRITE_PATH, upgradeTimeoutMs, authHeader);
24602475
} catch (HttpClientException e) {
24612476
HttpClientException classified = QwpUpgradeFailures.classify(newClient, ep.host, ep.port, e);

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/WebSocketTokenProviderTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
package io.questdb.client.test.cutlass.qwp.client;
2626

2727
import io.questdb.client.Sender;
28+
import io.questdb.client.cutlass.auth.OidcAuthException;
2829
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
2930
import org.junit.Assert;
3031
import org.junit.Test;
@@ -135,6 +136,47 @@ public void testStaticTokenStillSuppliedOverWebSocket() throws Exception {
135136
});
136137
}
137138

139+
@Test
140+
public void testThrowingProviderResolvedOncePerConnectRound() throws Exception {
141+
assertMemoryLeak(() -> {
142+
// A token-provider failure (a failed silent refresh, or not signed in) is cluster-wide, not a
143+
// per-endpoint transport fault. The credential is resolved once before the endpoint walk, so the
144+
// provider is queried exactly once per connect round even across a multi-endpoint failover, and the
145+
// provider's own error reaches the caller instead of being masked as "all endpoints unreachable".
146+
AtomicInteger calls = new AtomicInteger();
147+
try (TestWebSocketServer server = new TestWebSocketServer(new AckHandler())) {
148+
int port = server.getPort();
149+
server.start();
150+
Assert.assertTrue(server.awaitStart(5, TimeUnit.SECONDS));
151+
152+
// two endpoints at the same reachable server (distinct host strings, so not rejected as
153+
// duplicates) - a pre-fix per-endpoint pull would query the provider twice for one connect
154+
try {
155+
Sender.builder(Sender.Transport.WEBSOCKET)
156+
.address("localhost:" + port)
157+
.address("127.0.0.1:" + port)
158+
.httpTokenProvider(() -> {
159+
calls.incrementAndGet();
160+
throw new OidcAuthException("no token has been obtained yet; call signIn()");
161+
})
162+
.build();
163+
Assert.fail("expected build() to fail when the token provider throws");
164+
} catch (OidcAuthException e) {
165+
// the provider's own error surfaces directly, not wrapped as a transport failure
166+
String msg = e.getMessage();
167+
Assert.assertTrue("expected the provider's message, got: " + msg,
168+
msg.contains("no token has been obtained yet"));
169+
Assert.assertFalse("a provider failure must not be mislabeled as unreachable, got: " + msg,
170+
msg.contains("unreachable"));
171+
} catch (Exception e) {
172+
Assert.fail("expected the provider's OidcAuthException to surface, got: " + e);
173+
}
174+
// queried once per connect round, not once per endpoint (pre-fix this would be 2)
175+
Assert.assertEquals(1, calls.get());
176+
}
177+
});
178+
}
179+
138180
@Test
139181
public void testUsernamePasswordStillSuppliedOverWebSocket() throws Exception {
140182
assertMemoryLeak(() -> {

0 commit comments

Comments
 (0)