Skip to content

Commit 2acba17

Browse files
glasstigerclaude
andcommitted
Fix OIDC blank-refresh gate and correct docs
tryRefresh's hasRequiredToken gated the served kind on length() > 0 while storeTokens folds a whitespace-only token to null via Chars.isBlank. A non-conformant 2xx refresh with a blank access/id token therefore reported success, cached a token storeTokens then nulled, and made signIn() throw "no access_token" instead of falling back to the interactive device flow. Gate hasRequiredToken on !Chars.isBlank so the refresh gate and the cache agree, and add testBlankTokenFromRefreshFallsBackToInteractiveFlow, which fails without the change with the exact "no access_token" error. Correct three stale docs left by the SF-drainer-terminal fix: - QwpCredentialUnavailableException's javadoc described a reconnectMaxDurationMillis-bounded terminate that no path implements; the running store-and-forward drainer retries a credential-unavailable failure indefinitely under Invariant B, and only the foreground/SYNC initial connect fails fast. - Sender.httpTokenProvider's javadoc claimed a WebSocket sender terminates on a sustained token outage; a running SF-backed sender retries token-pull failures indefinitely and recovers, as testPersistentlyThrowingProviderOnReconnect... already proves. - FileTokenStore's createLockFile/acquireLock/stealIfStale comments claimed a single atomic create+stamp with no gap; writeNewFile opens then writes separately, so a GC pause can land in the empty window, which EMPTY_LOCK_STEAL_GRACE_MILLIS covers. Add testProviderTokenReResolvedOnFailoverReconnect, which drives a real QwpQueryClient failover reconnect and asserts reconnectViaTracker re-resolves the token provider so a rotated token reaches the reconnect upgrade, closing the one untested cross-context provider path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent b7bb36d commit 2acba17

6 files changed

Lines changed: 177 additions & 27 deletions

File tree

core/src/main/java/io/questdb/client/Sender.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2111,9 +2111,13 @@ public LineSenderBuilder httpToken(String token) {
21112111
* started, then once per flush. Over WebSocket the initial connection handshake runs during
21122112
* {@code build()} and queries the provider once for it, then again once per reconnect handshake - so a
21132113
* refreshed token is presented each time the link is (re)established; an already-established WebSocket
2114-
* is not re-authenticated mid-stream. The two transports differ on a sustained token outage: over HTTP
2115-
* a failed pull is retried on the next row, but over WebSocket a pull that keeps failing past the
2116-
* reconnect budget terminates the sender for good, like any persistent reconnect failure.
2114+
* is not re-authenticated mid-stream. The two transports differ in mechanism but both keep the producer
2115+
* alive across a sustained token outage: over HTTP a failed pull leaves the request token-pending and is
2116+
* retried on the next row; over WebSocket the token must be obtainable when {@code build()} runs (the
2117+
* initial handshake fails fast otherwise), after which a pull that keeps failing on later reconnects is
2118+
* retried indefinitely, with the buffered rows held in store-and-forward, until a token is available
2119+
* again. A token outage does not terminate a running WebSocket sender, just as a persistent transport
2120+
* reconnect failure does not (store-and-forward Invariant B).
21172121
* <br>
21182122
* Over HTTP the token is pulled once per request and written into the request buffer ahead of the
21192123
* buffered rows, so a token refresh is picked up on the next new batch after a successful flush. A

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

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -352,11 +352,13 @@ long getLockStaleMillis() {
352352
}
353353

354354
private static void createLockFile(Path lock, String nonce) throws IOException {
355-
// Exclusively create the lock (O_CREAT|O_EXCL via CREATE_NEW) AND write the owner nonce in a single
356-
// open, so there is NO create->stamp gap for a GC/safepoint pause (or a cross-machine clock skew) to
357-
// straddle and make our freshly-created lock look empty-and-stale to a peer. FileAlreadyExists means a
358-
// peer already holds it. releaseLock and stealIfStale verify this nonce before deleting. Keep the
359-
// owner-only perms (and the non-POSIX fallback) to match the store's other files.
355+
// Exclusively create the lock (O_CREAT|O_EXCL via CREATE_NEW), then write the owner nonce into that same
356+
// open channel before closing it. The file exists empty only for the tiny window between the create and
357+
// the stamp; a GC/safepoint pause (or a cross-machine clock skew) CAN land in that window, so what keeps
358+
// our freshly-created lock from being stolen as empty-and-stale is EMPTY_LOCK_STEAL_GRACE_MILLIS sitting
359+
// well above it, not the absence of the window. FileAlreadyExists means a peer already holds it.
360+
// releaseLock and stealIfStale verify this nonce before deleting. Keep the owner-only perms (and the
361+
// non-POSIX fallback) to match the store's other files.
360362
final byte[] bytes = nonce.getBytes(StandardCharsets.UTF_8);
361363
try {
362364
writeNewFile(lock, bytes, FILE_ATTRS);
@@ -701,8 +703,9 @@ private String acquireLock(Path lock) {
701703
final long deadline = System.currentTimeMillis() + lockAcquireBudgetMillis;
702704
while (true) {
703705
try {
704-
// atomic exclusive-create + stamp in one call: no create->stamp gap, so a GC/safepoint pause
705-
// can no longer make our freshly-created lock look empty-and-stale to a peer mid-acquisition
706+
// exclusive-create then stamp on the same open channel: the empty-file window between the two is
707+
// tiny and covered by EMPTY_LOCK_STEAL_GRACE_MILLIS, so a GC/safepoint pause mid-acquisition
708+
// cannot get our freshly-created lock stolen as empty-and-stale
706709
createLockFile(lock, nonce);
707710
return nonce;
708711
} catch (FileAlreadyExistsException e) {
@@ -792,11 +795,12 @@ private void stealIfStale(Path lock) {
792795
return;
793796
}
794797
} else if (!isOlderThan(lock, Math.min(EMPTY_LOCK_STEAL_GRACE_MILLIS, lockStaleMillis))) {
795-
// an empty/unreadable lock is never a validly-held lock: acquireLock creates the lock and writes
796-
// the owner nonce in ONE atomic call (createLockFile via CREATE_NEW), so a live lock always carries
797-
// a stamp. An empty lock therefore means a crash mid-write - the exclusive create succeeded but the
798-
// nonce write did not - a rare, narrow window with NO Java-level create->stamp gap for a GC/safepoint
799-
// pause to straddle (the pause would have to land inside the single write call). Steal it on the
798+
// an empty/unreadable lock is almost never a validly-held lock: acquireLock creates the lock and
799+
// stamps the owner nonce onto the same open channel (createLockFile via CREATE_NEW), so a live lock
800+
// carries its stamp within the tiny create->stamp window. An empty lock therefore means either a
801+
// crash mid-write (the exclusive create succeeded but the nonce write did not) or a peer momentarily
802+
// caught in that narrow window - a GC/safepoint pause CAN land there, which is exactly why the grace
803+
// exists. Steal it on the
800804
// short empty-lock grace rather than the full staleness window, so a crash orphan stops wedging peers
801805
// for the whole window; the capture-verify below still confirms the lock is unchanged before
802806
// completing the steal. (A cross-machine clock skew wider than the grace could still pre-empt such a

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1601,12 +1601,15 @@ private boolean tryRefresh() {
16011601
return false;
16021602
}
16031603
// succeed only on a clean 2xx (no OAuth error) returning the token getToken() actually serves (the
1604-
// id token when groups are encoded in it, the access token otherwise). A refresh that omits the id
1605-
// token - which RFC 6749 permits and many providers do - or carries an error or a non-2xx status
1606-
// must fall back to the interactive flow rather than be cached (and later fail in selectToken())
1604+
// id token when groups are encoded in it, the access token otherwise). A refresh that omits the served
1605+
// kind - which RFC 6749 permits and many providers do - or returns it blank/whitespace-only, or carries
1606+
// an error or a non-2xx status, must fall back to the interactive flow rather than be cached. Test the
1607+
// served kind with Chars.isBlank, the SAME contract storeTokens/adopt use to fold a blank token to null:
1608+
// gating on length() > 0 here would pass a whitespace-only token, which storeTokens then nulls, so
1609+
// tryRefresh would report success while selectToken() throws "no token" instead of falling back.
16071610
boolean hasRequiredToken = (groupsInToken
1608-
? tokenParser.idToken.length() > 0
1609-
: tokenParser.accessToken.length() > 0)
1611+
? !Chars.isBlank(tokenParser.idToken)
1612+
: !Chars.isBlank(tokenParser.accessToken))
16101613
&& isHttpStatusSuccess()
16111614
&& tokenParser.error.length() == 0;
16121615
if (hasRequiredToken) {

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

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,15 @@
3232
* returning a token -- a failed silent refresh, or no sign-in yet.
3333
* <p>
3434
* Distinct from {@link QwpAuthFailedException}, which means the server rejected a
35-
* credential the client did present. Neither is a transport outage, so neither heals
36-
* by retrying alone: a reconnect loop cannot conjure a credential it is unable to
37-
* acquire. The cursor send loop therefore retries this class only for as long as a
38-
* transient refresh could still recover -- bounded by {@code reconnectMaxDurationMillis}
39-
* -- and then terminates the sender with the provider's own message, rather than
40-
* reconnect-looping forever against a dead credential (Invariant B, which governs
41-
* genuine transport outages, stays untouched).
35+
* credential the client did present (a terminal auth failure). A credential the client
36+
* cannot ACQUIRE is instead handled by connection phase, exactly like a transport outage:
37+
* the RUNNING store-and-forward drainer retries it indefinitely with capped backoff under
38+
* Invariant B -- the IdP becomes reachable again, or the user completes an interactive
39+
* sign-in -- holding the un-acked rows in SF meanwhile, and NEVER bounds it by
40+
* {@code reconnectMaxDurationMillis} nor latches a terminal (either would drop a producer
41+
* store-and-forward promised to keep alive). Only the foreground/SYNC initial connect
42+
* fails fast, because a connectivity error is the caller's to see during initialization,
43+
* not after the drainer is running.
4244
* <p>
4345
* This is an internal marker that carries the provider's own exception: it exists so
4446
* the send loop can tell "the provider failed" apart from "the network failed". A

core/src/test/java/io/questdb/client/test/cutlass/auth/OidcDeviceAuthTest.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1775,6 +1775,41 @@ public void testBlankServedTokenFromWireIsNotServed() throws Exception {
17751775
});
17761776
}
17771777

1778+
@Test(timeout = 30_000)
1779+
public void testBlankTokenFromRefreshFallsBackToInteractiveFlow() throws Exception {
1780+
assertMemoryLeak(() -> {
1781+
// a non-conformant IdP answers a SILENT REFRESH with a 2xx carrying a whitespace-only access token.
1782+
// The refresh gate (hasRequiredToken) must treat it as absent with the same Chars.isBlank contract
1783+
// storeTokens uses, so tryRefresh() reports failure and signIn() falls back to the interactive device
1784+
// flow - rather than caching a token storeTokens then nulls, which would make signIn() throw "no
1785+
// access_token" while a fresh interactive sign-in was still possible.
1786+
AtomicInteger deviceCodePolls = new AtomicInteger();
1787+
MockOidcServer.Handler handler = (method, path, body) -> {
1788+
if (DEVICE_PATH.equals(path)) {
1789+
return MockOidcServer.json(200, deviceAuthorizationJson(1, 300));
1790+
}
1791+
if (body.contains("grant_type=refresh_token")) {
1792+
// blank served token on refresh: the gate must fall back, not cache-and-serve it
1793+
return MockOidcServer.json(200, tokenJson(" ", null, null, 3600));
1794+
}
1795+
// the device-code grant: the first poll mints the initial short-lived token; the second is the
1796+
// interactive fallback after the blank refresh and mints a fresh, usable one
1797+
return deviceCodePolls.incrementAndGet() == 1
1798+
? MockOidcServer.json(200, tokenJson("ACCESS-1", null, "REFRESH-1", 60))
1799+
: MockOidcServer.json(200, tokenJson("ACCESS-FALLBACK", null, "REFRESH-2", 3600));
1800+
};
1801+
try (MockOidcServer server = new MockOidcServer(handler);
1802+
OidcDeviceAuth auth = newAuth(server, false, noopPrompt())) {
1803+
Assert.assertEquals("ACCESS-1", auth.signIn());
1804+
expireCachedToken(auth); // force the silent-refresh path on the next sign-in
1805+
// with the blank-refresh gate fixed, signIn() falls back to the device flow instead of throwing
1806+
Assert.assertEquals("ACCESS-FALLBACK", auth.signIn());
1807+
Assert.assertEquals("the interactive device flow must run again as the fallback",
1808+
2, deviceCodePolls.get());
1809+
}
1810+
});
1811+
}
1812+
17781813
@Test(timeout = 30_000)
17791814
public void testGroupsInTokenButNoIdTokenFails() throws Exception {
17801815
assertMemoryLeak(() -> {

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,28 @@
2626

2727
import io.questdb.client.cutlass.http.client.HttpClientException;
2828
import io.questdb.client.cutlass.line.LineSenderException;
29+
import io.questdb.client.cutlass.qwp.client.QwpColumnBatch;
30+
import io.questdb.client.cutlass.qwp.client.QwpColumnBatchHandler;
31+
import io.questdb.client.cutlass.qwp.client.QwpEgressMsgKind;
2932
import io.questdb.client.cutlass.qwp.client.QwpQueryClient;
33+
import io.questdb.client.cutlass.qwp.protocol.QwpConstants;
34+
import io.questdb.client.test.cutlass.qwp.websocket.TestWebSocketServer;
3035
import org.junit.Assert;
3136
import org.junit.Test;
3237

38+
import java.io.IOException;
3339
import java.io.OutputStream;
3440
import java.net.InetAddress;
3541
import java.net.ServerSocket;
3642
import java.net.Socket;
43+
import java.nio.ByteBuffer;
44+
import java.nio.ByteOrder;
3745
import java.nio.charset.StandardCharsets;
3846
import java.util.ArrayList;
3947
import java.util.Collections;
4048
import java.util.List;
49+
import java.util.concurrent.TimeUnit;
50+
import java.util.concurrent.atomic.AtomicInteger;
4151

4252
/**
4353
* Unit coverage for {@link QwpQueryClient#withBearerTokenProvider}: header
@@ -53,6 +63,20 @@
5363
*/
5464
public class QwpQueryClientTokenProviderTest {
5565

66+
private static final QwpColumnBatchHandler NOOP_BATCH_HANDLER = new QwpColumnBatchHandler() {
67+
@Override
68+
public void onBatch(QwpColumnBatch batch) {
69+
}
70+
71+
@Override
72+
public void onEnd(long totalRows) {
73+
}
74+
75+
@Override
76+
public void onError(byte status, String message) {
77+
}
78+
};
79+
5680
@Test
5781
public void testProviderConflictsWithBasicAuth() {
5882
try (QwpQueryClient c = QwpQueryClient.newPlainText("localhost", 9000).withBearerTokenProvider(() -> "tok")) {
@@ -126,6 +150,54 @@ public void testProviderSynthesizesBearerHeader() {
126150
}
127151
}
128152

153+
@Test(timeout = 20_000)
154+
public void testProviderTokenReResolvedOnFailoverReconnect() throws Exception {
155+
// The failover reconnect path (reconnectViaTracker) resolves the Authorization header once before its
156+
// endpoint walk, exactly as connect() does, so a rotating token reaches the reconnect upgrade. This
157+
// pins that a regression dropping the re-resolve from the reconnect path would be caught: bind endpoint
158+
// A on the initial connect (capturing tok-0), drop it, then run a query - the failover reconnect to
159+
// endpoint B must upgrade with a FRESHLY resolved token, not the stale connect-time one.
160+
AtomicInteger calls = new AtomicInteger();
161+
TestWebSocketServer a = new TestWebSocketServer(new TestWebSocketServer.WebSocketServerHandler() {
162+
});
163+
a.setSendServerInfo(true);
164+
TestWebSocketServer b = new TestWebSocketServer(new ExecDoneQueryServer());
165+
b.setSendServerInfo(true);
166+
try {
167+
a.start();
168+
b.start();
169+
Assert.assertTrue(a.awaitStart(5, TimeUnit.SECONDS));
170+
Assert.assertTrue(b.awaitStart(5, TimeUnit.SECONDS));
171+
172+
try (QwpQueryClient client = QwpQueryClient.fromConfig(
173+
"ws::addr=localhost:" + a.getPort() + ",localhost:" + b.getPort() + ";auth_timeout_ms=2000;")
174+
.withBearerTokenProvider(() -> "tok-" + calls.getAndIncrement())) {
175+
client.connect();
176+
Assert.assertTrue("client must bind the first endpoint on connect", client.isConnected());
177+
String aHeader = a.pollAuthorizationHeader(5, TimeUnit.SECONDS);
178+
Assert.assertEquals("the initial connect upgrade must carry the first resolved token",
179+
"Bearer tok-0", aHeader);
180+
181+
// drop endpoint A so the next execute() cannot use its connection and must fail over
182+
a.close();
183+
184+
// the query fails on the dead A connection, drives the failover loop -> reconnectViaTracker,
185+
// which re-resolves the header and upgrades B; B answers EXEC_DONE so execute() returns
186+
client.execute("SELECT 1", NOOP_BATCH_HANDLER, false);
187+
188+
String bHeader = b.pollAuthorizationHeader(5, TimeUnit.SECONDS);
189+
Assert.assertNotNull("the failover reconnect must upgrade endpoint B", bHeader);
190+
Assert.assertTrue("the reconnect upgrade must carry a Bearer token, was: " + bHeader,
191+
bHeader.startsWith("Bearer tok-"));
192+
Assert.assertNotEquals("the failover reconnect must RE-RESOLVE the provider, not reuse the "
193+
+ "connect-time token", aHeader, bHeader);
194+
}
195+
} finally {
196+
a.close();
197+
b.close();
198+
}
199+
}
200+
129201
@Test(timeout = 15_000)
130202
public void testProviderTokenSentOnRealUpgrade() throws Exception {
131203
// drive the REAL connect path (connect() -> resolveAuthorizationHeader -> runUpgradeWithTimeout),
@@ -235,4 +307,34 @@ public void testThrowingProviderFailsConnect() throws Exception {
235307
}
236308
}
237309
}
310+
311+
private static byte[] buildExecDone(byte[] queryRequest) {
312+
int bodyLen = 1 + 8 + 1 + 1; // msg_kind + request_id + op_type + rows_affected varint
313+
byte[] frame = new byte[QwpConstants.HEADER_SIZE + bodyLen];
314+
ByteBuffer bb = ByteBuffer.wrap(frame).order(ByteOrder.LITTLE_ENDIAN);
315+
bb.put((byte) 'Q').put((byte) 'W').put((byte) 'P').put((byte) '1');
316+
bb.put((byte) 1); // version
317+
bb.put((byte) 0); // flags
318+
bb.putShort((short) 0); // table_count
319+
bb.putInt(bodyLen); // payload_length
320+
bb.put(QwpEgressMsgKind.EXEC_DONE);
321+
bb.put(queryRequest, 1, 8); // echo request_id verbatim
322+
bb.put((byte) 0); // op_type
323+
bb.put((byte) 0); // rows_affected = 0
324+
return frame;
325+
}
326+
327+
private static final class ExecDoneQueryServer implements TestWebSocketServer.WebSocketServerHandler {
328+
@Override
329+
public void onBinaryMessage(TestWebSocketServer.ClientHandler client, byte[] data) {
330+
if (data.length == 0 || data[0] != QwpEgressMsgKind.QUERY_REQUEST) {
331+
return;
332+
}
333+
try {
334+
client.sendBinary(buildExecDone(data));
335+
} catch (IOException e) {
336+
// best-effort: a failed reply surfaces to the client as a transport error
337+
}
338+
}
339+
}
238340
}

0 commit comments

Comments
 (0)