Add OIDC token-provider tests and cleanups

Address minor code-review findings on the OIDC device flow: fill test
gaps around the token provider and tidy comments and dead code. No
production behavior change beyond simplifying unreachable branches.

Tests:
- Add testRefreshedTokenWithControlCharFallsBackToInteractiveFlow: a
  refresh whose 200 response carries a served token with an escaped
  control char (JsonLexer now decodes it into a real CR) is rejected by
  validateTokenChars, and tryRefresh must fall back to the interactive
  flow rather than propagate. Guards the tryRefresh storeTokens catch;
  verified to fail without it.
- Add testThrowingProviderOnReconnectIsRetriedAndRecovers and
  testPersistentlyThrowingProviderOnReconnectTerminatesTheSender: a
  token provider that throws on a WebSocket reconnect (the background
  I/O thread) is retried within the reconnect budget and recovers on a
  transient failure, or terminates the sender once the budget is
  exhausted on a persistent one.
- Replace the vacuous testMalformedEndpointDoesNotLeakNativeMemory
  (which fed "not-a-url", rejected before the lexer is allocated) with
  testRejectedBuildDoesNotLeakNativeMemory (a parseable-but-rejected
  endpoint) and testSuccessfulBuildAndCloseDoNotLeakNativeMemory - the
  path that actually allocates and frees the native lexer. The new
  guard fails if close() stops freeing the lexer.
- Add @test(timeout=30_000) to
  testShortAllDigitStatusNotTreatedAsTransientOrTerminal so a guard
  regression fails fast instead of polling to the device-code deadline.

Comments and dead code:
- Reword the writeLockHolder and acquireLock comments to match
  releaseLock's honest framing: the read-then-write and
  size-check-then-delete narrow, but do not close, the peer-stamp
  clobber window; the residual degrades to a double-refresh, never a
  torn credential.
- Reduce pathOnly() to return Endpoint.parse(url).path and drop the
  now-unreachable ?/# arms in Endpoint.parse's authority terminator and
  path construction (Endpoint.parse rejects ? and # up front).
- Expand the refreshUnderLock comment: the equals(refreshToken,
  lastPersistedRefreshToken) guard no longer strictly means "unsaved
  newer token" once adopt() keeps a live token the file lacked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: glasstiger

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

@@ -647,11 +647,14 @@ private static boolean writeLockHolder(Path lock, String nonce) {
             // acquireLock created this lock empty; if it already carries a stamp, a peer judged it stale and
             // stole+restamped it in the create->stamp gap (a long GC/suspend pause between the two syscalls, or
             // a cross-machine clock skew wider than the empty-lock grace). A plain WRITE|TRUNCATE_EXISTING has no
-            // exclusivity and would overwrite the peer's stamp, leaving two processes each believing they hold
-            // the lock. Refuse instead - honouring releaseLock's own-stamp ownership rule - so acquireLock
-            // degrades to a lock-free refresh (the documented best-effort residual) rather than clobber a live
-            // peer's stamp. A readLockHolder that throws (our file was moved away during the peer's steal) is
-            // caught below and likewise fails the stamp.
+            // exclusivity and would overwrite that stamp, leaving two processes each believing they hold the
+            // lock, so refuse when a stamp is already present - honouring releaseLock's own-stamp ownership rule.
+            // This read-then-write is two syscalls, so like releaseLock's own read-then-delete it only NARROWS
+            // the clobber window (a steal landing between the read and the write is still overwritten); it does
+            // not close it. There is no atomic "write-only-if-still-empty" primitive to close it with. The
+            // residual degrades to at most the documented double-refresh (a re-prompt on a rotating IdP), never
+            // a torn or forged credential - Layer-1's atomic rename holds regardless. A readLockHolder that
+            // throws (our file was moved away during the peer's steal) is caught below and likewise fails the stamp.
             if (readLockHolder(lock) != null) {
                 return false;
             }
@@ -679,7 +682,10 @@ private String acquireLock(Path lock) {
                 // created and degrade to a lock-free refresh rather than hold an unverifiable lock. Remove it
                 // only while it is still the empty file we created: writeLockHolder also returns false when a
                 // peer stole and restamped this path in the create->stamp gap, and deleting that peer's non-empty
-                // live lock by bare path would admit a third holder (mirrors releaseLock's own-stamp rule).
+                // live lock by bare path would admit a third holder (mirrors releaseLock's own-stamp rule). Like
+                // writeLockHolder's read-then-write, this size-check-then-delete is two syscalls, so it narrows
+                // but does not fully close that window; the residual degrades to the documented double-refresh,
+                // never a torn credential.
                 try {
                     if (Files.size(lock) == 0) {
                         Files.deleteIfExists(lock);

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

@@ -855,16 +855,10 @@ private static int parseIntOrZero(CharSequence value) {
     }
 
     private static String pathOnly(String url) {
-        // the path component only (drop any ?query / #fragment); a ;matrix parameter stays part of the path,
-        // so a traversal hidden in it (.../token;..%2f..) is still scanned
-        String path = Endpoint.parse(url).path;
-        for (int i = 0, n = path.length(); i < n; i++) {
-            char c = path.charAt(i);
-            if (c == '?' || c == '#') {
-                return path.substring(0, i);
-            }
-        }
-        return path;
+        // the path component only; Endpoint.parse rejects a url carrying a ?query or #fragment up front, so the
+        // returned path never contains one. A ;matrix parameter, by contrast, stays part of the path, so a
+        // traversal hidden in it (.../token;..%2f..) is still scanned by the issuer-path check.
+        return Endpoint.parse(url).path;
     }
 
     private static String percentDecodeOnce(String s) {
@@ -1334,11 +1328,17 @@ private boolean refreshUnderLock() {
         // entry and skip the network when it already yields a valid token; otherwise refresh with the freshest
         // known refresh token (the one just adopted, so a rotated token is not replayed).
         //
-        // Only re-read when the in-memory refresh token still matches what we last persisted. If they differ,
-        // a previous save failed (persistence is best-effort), so the in-memory token is newer than the
-        // on-disk one; re-adopting would regress it to the stale - and, on a rotating identity provider,
-        // already-revoked - on-disk token and force a needless re-prompt. In that case keep the in-memory
-        // token and refresh with it.
+        // Only re-read when the in-memory refresh token still matches what we last persisted. A mismatch no
+        // longer strictly means "in-memory is a newer unsaved token": it covers two cases, and re-adopting
+        // would regress in both, so keep the in-memory token and refresh with it. (1) A previous save failed
+        // (persistence is best-effort), so the in-memory token is genuinely newer than the on-disk one;
+        // re-adopting would regress it to the stale - and, on a rotating identity provider, already-revoked -
+        // on-disk token and force a needless re-prompt. (2) adopt() kept a live in-memory token that the loaded
+        // file did not carry (a cross-language peer that never received a refresh_token), leaving
+        // lastPersistedRefreshToken null; here the trade-off is that if a rotating-IdP peer has since revoked
+        // our token and written a fresher one, we skip that fresher on-disk token this round and fall back to an
+        // interactive re-prompt. Both are benign (never a stale/wrong served token; the pre-fix alternative in
+        // case 2 was an uncaught urlEncode(null) NPE) and cross-process-only.
         if (Objects.equals(refreshToken, lastPersistedRefreshToken)) {
             PersistedToken fresh;
             try {
@@ -1982,27 +1982,20 @@ static Endpoint parse(String url) {
                 throw new OidcAuthException().put("invalid url, expected http or https [url=").put(url).put(']');
             }
             int hostStart = schemeEnd + 3;
-            // the authority ([userinfo@]host[:port]) ends at the first '/', '?' or '#'; splitting only on
-            // '/' (as before) folded a query/fragment - or userinfo - into the host on a path-less url
+            // the authority ([userinfo@]host[:port]) ends at the first '/', or at the end of the url for a
+            // path-less endpoint. A ?query or #fragment was already rejected above, so neither can fold into the
+            // host or the path here (this used to also split on '?'/'#' to guard that, now handled up front).
             int authorityEnd = url.length();
             for (int i = hostStart, n = url.length(); i < n; i++) {
-                char c = url.charAt(i);
-                if (c == '/' || c == '?' || c == '#') {
+                if (url.charAt(i) == '/') {
                     authorityEnd = i;
                     break;
                 }
             }
             String hostPort = url.substring(hostStart, authorityEnd);
-            // a path-less url uses '/'; a query/fragment with no path is prefixed with '/' so the request
-            // line stays well-formed (a '/'-terminated authority already carries its own leading slash)
-            String path;
-            if (authorityEnd == url.length()) {
-                path = "/";
-            } else if (url.charAt(authorityEnd) == '/') {
-                path = url.substring(authorityEnd);
-            } else {
-                path = "/" + url.substring(authorityEnd);
-            }
+            // a path-less url uses '/'; otherwise the authority is '/'-terminated and the path starts at that
+            // slash, which already carries its own leading slash
+            String path = authorityEnd == url.length() ? "/" : url.substring(authorityEnd);
             if (hostPort.indexOf('@') >= 0) {
                 // userinfo (user[:pass]@host) is unsupported: the HTTP layer would connect to the literal
                 // "user@host". Reject it clearly rather than mis-resolve it or surface a misleading port error

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

@@ -2043,24 +2043,49 @@ public void testPlaintextSettingsWithAdvertisedEndpointsRequiresPin() throws Exc
     }
 
     @Test(timeout = 30_000)
-    public void testMalformedEndpointDoesNotLeakNativeMemory() {
-        // build() parses the endpoints up front (for the co-location / issuer-pin checks) and throws on
-        // this malformed url before the constructor allocates the native JSON lexer, so the never-returned
-        // instance cannot leak it. Measure the parser tag directly - the module's assertMemoryLeak does not
-        // flag a single-tag growth.
+    public void testRejectedBuildDoesNotLeakNativeMemory() {
+        // A build rejected during validation must not leak. build() parses and validates every endpoint
+        // BEFORE the constructor runs, and the constructor allocates the native JSON lexer LAST (after
+        // urlEncode and the TokenStoreKey build, either of which can throw), so a rejected build never
+        // allocates the lexer and the never-returned instance cannot be closed to free it. Use a
+        // parseable-but-rejected config - endpoints that parse cleanly but fail the https requirement - so the
+        // rejection lands AFTER endpoint parsing, exercising more of build() than a syntactically bad url
+        // would. testSuccessfulBuildAndCloseDoNotLeakNativeMemory covers the complementary lexer-allocated
+        // path. Measure the parser tag directly; the module's assertMemoryLeak does not flag a single-tag growth.
         long parserMemBefore = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_TEXT_PARSER_RSS);
         try (OidcDeviceAuth ignored = OidcDeviceAuth.builder()
                 .clientId("c")
-                .deviceAuthorizationEndpoint("not-a-url")
+                .deviceAuthorizationEndpoint("http://idp.example/device") // parses fine, but plaintext http to a non-loopback host
                 .tokenEndpoint("https://idp.example/token")
-                .allowInsecureTransport(true)
+                .allowInsecureTransport(false)
                 .build()
         ) {
-            Assert.fail("expected Endpoint.parse to reject the malformed url");
+            Assert.fail("expected the https requirement to reject the plaintext device endpoint");
         } catch (OidcAuthException e) {
-            Assert.assertTrue(e.getMessage(), e.getMessage().contains("expected a scheme"));
+            Assert.assertTrue(e.getMessage(), e.getMessage().contains("use an https url"));
         }
-        Assert.assertEquals("the JSON lexer native buffer leaked",
+        Assert.assertEquals("a rejected build must not leak the JSON lexer native buffer",
+                parserMemBefore, Unsafe.getMemUsedByTag(MemoryTag.NATIVE_TEXT_PARSER_RSS));
+    }
+
+    @Test(timeout = 30_000)
+    public void testSuccessfulBuildAndCloseDoNotLeakNativeMemory() {
+        // The complement to the rejected-build case: a SUCCESSFUL build is the only path that allocates the
+        // native JSON lexer, so this is the block that actually exercises a lexer-allocated instance, and
+        // close() must free it. Loop a few build->close cycles so any per-cycle leak accrues, then assert the
+        // parser tag returns to its baseline. build() does no network I/O (discovery is separate), so valid
+        // co-located https endpoints construct offline.
+        long parserMemBefore = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_TEXT_PARSER_RSS);
+        for (int i = 0; i < 4; i++) {
+            try (OidcDeviceAuth auth = OidcDeviceAuth.builder()
+                    .clientId("c")
+                    .deviceAuthorizationEndpoint("https://idp.example/device")
+                    .tokenEndpoint("https://idp.example/token")
+                    .build()) {
+                Assert.assertNotNull(auth);
+            }
+        }
+        Assert.assertEquals("close() must free the JSON lexer native buffer allocated by a successful build",
                 parserMemBefore, Unsafe.getMemUsedByTag(MemoryTag.NATIVE_TEXT_PARSER_RSS));
     }
 
@@ -2502,6 +2527,45 @@ public void testRefreshErrorFallsBackToInteractiveFlow() throws Exception {
         });
     }
 
+    @Test(timeout = 30_000)
+    public void testRefreshedTokenWithControlCharFallsBackToInteractiveFlow() throws Exception {
+        assertMemoryLeak(() -> {
+            // A silent refresh whose 200 response carries a served token with a control character - here an
+            // escaped \r that JsonLexer now decodes into a real CR byte - must be rejected by storeTokens ->
+            // validateTokenChars, and tryRefresh must SWALLOW that rejection and fall back to the interactive
+            // device flow rather than let it propagate out of signIn()/getToken(). Guards the tryRefresh
+            // storeTokens try/catch: without it, this signIn() throws instead of returning the fallback token.
+            AtomicInteger deviceCalls = new AtomicInteger();
+            AtomicInteger deviceCodeGrants = new AtomicInteger();
+            MockOidcServer.Handler handler = (method, path, body) -> {
+                if (DEVICE_PATH.equals(path)) {
+                    deviceCalls.incrementAndGet();
+                    return MockOidcServer.json(200, deviceAuthorizationJson(1, 300));
+                }
+                if (body.contains("grant_type=refresh_token")) {
+                    // valid-JSON 200, but the access_token carries an escaped CR (\r on the wire); the served
+                    // kind is validated, so validateTokenChars must reject it before it is cached
+                    return MockOidcServer.json(200, tokenJson("ACCESS\\r2", null, "REFRESH-2", 3600));
+                }
+                // the initial device-code grant uses a short TTL so the next signIn() triggers a refresh
+                if (deviceCodeGrants.getAndIncrement() == 0) {
+                    return MockOidcServer.json(200, tokenJson("ACCESS-1", null, "REFRESH-1", 1));
+                }
+                // the fallback interactive grant, after the poisoned refresh is rejected
+                return MockOidcServer.json(200, tokenJson("ACCESS-3", null, "REFRESH-3", 3600));
+            };
+            try (MockOidcServer server = new MockOidcServer(handler);
+                 OidcDeviceAuth auth = newAuth(server, false, noopPrompt())) {
+                Assert.assertEquals("ACCESS-1", auth.signIn());
+                expireCachedToken(auth);
+                // the refresh returns a control-char token -> rejected -> fall back to a fresh interactive sign-in
+                Assert.assertEquals("ACCESS-3", auth.signIn());
+                Assert.assertEquals("the interactive flow must run twice (initial + fallback after the rejected refresh)",
+                        2, deviceCalls.get());
+            }
+        });
+    }
+
     @Test(timeout = 30_000)
     public void testRefreshKeepsExistingRefreshTokenWhenOmitted() throws Exception {
         assertMemoryLeak(() -> {
@@ -3306,7 +3370,7 @@ public void testShortAllDigitStatusIsNotTreatedAsSuccess() throws Exception {
         });
     }
 
-    @Test
+    @Test(timeout = 30_000)
     public void testShortAllDigitStatusNotTreatedAsTransientOrTerminal() throws Exception {
         // a real HTTP status is exactly 3 digits. A malformed 1-digit "5" must not be read as a transient 5xx
         // (which would poll on to the device-code deadline), nor a 1-digit "4" as a terminal 4xx, by the leading