Harden token-store cross-process lock edge cases

The FileTokenStore lock-file protocol had four edge cases where the
cross-process mutual exclusion could degrade. Each degrades gracefully
(at worst a re-prompt on a rotating-refresh-token IdP, never a torn or
forged credential), but the fixes are cheap and the comments overstated
the guarantees.

sweepStaleTempFiles now skips names containing ".lock.": a steal
captures a stale lock by renaming it to <hash>.lock.<uuid>.tmp, and
ATOMIC_MOVE preserves the stale mtime, so a concurrent save's temp sweep
would judge the in-flight capture old and delete it - destroying a lock
the stealer may be about to restore to its live owner.

stealIfStale now reclaims an empty/unstamped lock after a short grace
(EMPTY_LOCK_STEAL_GRACE_MILLIS) instead of the full staleness window. A
holder that crashes between the exclusive create and the stamp leaves an
empty lock with a fresh mtime that the staleness check protected for the
whole window, wedging peers into lock-free refreshes. The grace dwarfs
the create-to-stamp gap, so a peer mid-stamp is never pre-empted (which
would steal a lock its rightful owner is about to hold). isStale becomes
the parameterized isOlderThan.

The restore branch now documents honestly that a multi-actor race can
momentarily admit two holders - inherent to stealing with a lock file,
since a filesystem offers no atomic compare-and-delete.

The comments in FileTokenStore, OidcDeviceAuth, and the design doc
claimed the under-lock refresh hold is bounded by 4x httpTimeoutMillis;
they now note the connection phase (DNS + TCP connect + TLS handshake)
is not bounded by httpTimeoutMillis, so lockStaleMillis must clear that
on top of the refresh I/O. The default 600s window already absorbs it.

Adds regression tests for the empty-lock grace steal and the
sweep-skips-capture behaviour; both fail on the unfixed code.

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

@@ -91,14 +91,26 @@ public final class FileTokenStore implements TokenStore {
     // treat a lock older than this as abandoned by a crashed holder and steal it. Must stay comfortably
     // above the longest a live holder can hold it (one refresh under the lock) so a live holder is never
     // stolen from. That refresh runs send + await + parse, plus a body drain on a parse failure, each
-    // separately bounded by the client's HTTP timeout, so up to ~4x it; OidcDeviceAuth caps that timeout at
-    // 120s, so the worst-case hold is ~480s and this 10-minute window stays safely above it
+    // separately bounded by the client's HTTP timeout (so up to ~4x it; OidcDeviceAuth caps that timeout at
+    // 120s, hence ~480s), PLUS the connection phase (DNS + TCP connect + TLS handshake), which the HTTP
+    // timeout does NOT bound and which the OS bounds instead (a black-holed connect is ~tcp-connect-timeout,
+    // commonly ~2 minutes on Linux). This 10-minute window leaves ample headroom above ~480s + a typical
+    // connection stall; a pathological DNS/connection hang longer than that headroom can still let a peer
+    // steal a live holder's lock, degrading (best-effort) to a re-prompt on a rotating-refresh-token IdP
     private static final long DEFAULT_LOCK_STALE_MILLIS = 600_000L;
     private static final FileAttribute<Set<PosixFilePermission>> DIR_ATTRS =
             PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------"));
     // the same owner-only directory permissions as DIR_ATTRS, in the form setPosixFilePermissions wants, so
     // ensureDirectory can re-assert them on a directory that already exists with looser permissions
     private static final Set<PosixFilePermission> DIR_PERMS = PosixFilePermissions.fromString("rwx------");
+    // steal an empty/unstamped lock once it has existed at least this long. A validly held lock always
+    // carries an owner stamp (acquireLock stamps it immediately after the exclusive create); an empty lock is
+    // therefore either a peer momentarily between its create and its stamp - microseconds, far below this
+    // grace - or one a holder abandoned by crashing in that tiny window. Stealing on this short grace instead
+    // of the full staleness window keeps a post-crash empty lock from wedging peers (into lock-free refreshes)
+    // for the whole staleness window, while the grace stays well above the create->stamp gap so a peer
+    // mid-stamp is never pre-empted (which would force the rightful holder to degrade)
+    private static final long EMPTY_LOCK_STEAL_GRACE_MILLIS = 5_000L;
     private static final FileAttribute<Set<PosixFilePermission>> FILE_ATTRS =
             PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------"));
     private static final char[] HEX = "0123456789abcdef".toCharArray();
@@ -141,14 +153,20 @@ public FileTokenStore(Path directory) {
      *                                 and at most 30_000 (30s): {@code getToken()} can wait it out on the
      *                                 latency-sensitive flush path, so it is kept short
      * @param lockStaleMillis          a lock older than this is treated as abandoned by a crashed holder and
-     *                                 stolen. It MUST exceed the longest a live holder can hold the lock: one
-     *                                 refresh under the lock runs send + await + parse plus a body drain, each
-     *                                 bounded by the {@code OidcDeviceAuth} httpTimeoutMillis, so up to ~4x it
-     *                                 (~480s at the 120s timeout cap). Set it below that and a peer can steal a
-     *                                 live holder's lock mid-refresh, reopening the cross-process refresh race
-     *                                 this lock exists to prevent. The store cannot see the client's timeout,
-     *                                 so sizing this correctly is the caller's responsibility; the default is
-     *                                 600_000 (safely above the ~480s worst case).
+     *                                 stolen. It MUST exceed the longest a live holder can hold the lock, which
+     *                                 is the under-lock refresh PLUS the connection phase that precedes it: the
+     *                                 refresh runs send + await + parse plus a body drain, each bounded by the
+     *                                 {@code OidcDeviceAuth} httpTimeoutMillis (so up to ~4x it, ~480s at the
+     *                                 120s timeout cap), but establishing the connection - DNS resolution, the
+     *                                 TCP connect, and the TLS handshake - is NOT bounded by httpTimeoutMillis;
+     *                                 the OS bounds it instead (a black-holed connect runs to the OS TCP-connect
+     *                                 timeout, commonly ~2 minutes). Size this window above ~4x httpTimeoutMillis
+     *                                 plus a generous connection-stall allowance, or a peer can judge a live but
+     *                                 connection-stalled holder stale and steal its lock mid-refresh, reopening
+     *                                 the cross-process refresh race this lock exists to prevent. The store
+     *                                 cannot see the client's timeout, so sizing this correctly is the caller's
+     *                                 responsibility; the default is 600_000 (~480s worst-case refresh plus
+     *                                 ample headroom for a typical connection stall).
      */
     public FileTokenStore(Path directory, long lockAcquireBudgetMillis, long lockStaleMillis) {
         if (directory == null) {
@@ -591,7 +609,7 @@ private static boolean writeLockHolder(Path lock, String nonce) {
         // stamp the lock with the owner nonce. Unlike the staleness mtime (which only needs to be recent),
         // this content is what releaseLock checks before deleting, so it must be written reliably; report a
         // failure so acquireLock drops an unverifiable lock rather than hold one it cannot safely release.
-        // Writing also refreshes the mtime, which is what isStale reads
+        // Writing also refreshes the mtime, which is what the staleness age check reads
         try {
             Files.write(lock, nonce.getBytes(StandardCharsets.UTF_8), StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
             return true;
@@ -663,10 +681,10 @@ private void ensureDirectory() throws IOException {
         }
     }
 
-    private boolean isStale(Path lock) {
+    private boolean isOlderThan(Path lock, long thresholdMillis) {
         try {
             FileTime modified = Files.getLastModifiedTime(lock);
-            return System.currentTimeMillis() - modified.toMillis() > lockStaleMillis;
+            return System.currentTimeMillis() - modified.toMillis() > thresholdMillis;
         } catch (IOException e) {
             return false; // cannot determine the age; do not steal
         }
@@ -685,16 +703,27 @@ private void stealIfStale(Path lock) {
         // NoSuchFileException and fall back to the wait), then verify what we captured carries the same stamp
         // we judged stale. If a peer had already replaced it with a live lock we grabbed that instead, so we
         // put it back rather than steal it. This mirrors releaseLock's own-stamp check and shrinks the
-        // residual race from the whole isStale->delete gap to the gap between the two renames.
+        // residual race from the whole age-check->delete gap to the gap between the two renames.
         final byte[] before;
         try {
             before = readLockHolder(lock);
         } catch (IOException e) {
             return; // gone or unreadable; nothing to steal here - the create attempt or a peer settles it
         }
-        // read the stamp first, then check staleness: if a peer replaces the stale lock with a fresh one in
-        // between, isStale reads the fresh mtime and returns false, so we never proceed against a live lock
-        if (!isStale(lock)) {
+        // read the stamp first, then check age: if a peer replaces the lock with a fresh one in between, the
+        // age check reads the fresh mtime and returns false, so we never proceed against a live lock.
+        if (before != null) {
+            // a stamped lock is a (claimed) live holder: steal only once it outlives the full staleness window
+            if (!isOlderThan(lock, lockStaleMillis)) {
+                return;
+            }
+        } else if (!isOlderThan(lock, Math.min(EMPTY_LOCK_STEAL_GRACE_MILLIS, lockStaleMillis))) {
+            // an empty/unreadable lock is never a validly-held lock (a holder stamps right after creating): it
+            // is a peer mid-create/stamp (recovers on its own in microseconds) or one a crash orphaned in that
+            // gap. Steal it on the short empty-lock grace rather than the full staleness window, so a crash
+            // orphan stops wedging peers for the whole window; the grace dwarfs the create->stamp gap, so a
+            // peer mid-stamp is never pre-empted. The capture-verify below still confirms the lock is unchanged
+            // before completing the steal.
             return;
         }
         final Path captured = lock.resolveSibling(lock.getFileName().toString() + '.' + UUID.randomUUID() + ".tmp");
@@ -722,8 +751,13 @@ private void stealIfStale(Path lock) {
         // we captured a live lock a peer recreated in the gap: put it back rather than steal it. Use a plain
         // move (not ATOMIC_MOVE, which maps to rename(2) and would replace the target): if a third party
         // claimed the now-free path during our capture window, FileAlreadyExistsException leaves their lock
-        // intact and we drop our captured copy rather than clobber it; a stray .tmp is reclaimed by
-        // sweepStaleTempFiles on a later save.
+        // intact and we drop our captured copy rather than clobber it. That drop loses the recreating peer's
+        // lock file while it still believes it holds the lock, so for that one refresh two holders can run
+        // concurrently - the inherent residual of stealing with a lock file: a filesystem has no atomic
+        // "delete/rename only if the content is still X", so the capture-verify shrinks the window to this
+        // multi-actor race (our steal, a peer recreating, AND a third party claiming the freed path, all
+        // overlapping) but cannot close it. Best-effort by design: it degrades to one extra refresh - a
+        // re-prompt on a rotating-refresh-token IdP - never a torn or forged credential (Layer 1 still holds).
         try {
             Files.move(captured, lock);
         } catch (IOException e) {
@@ -740,6 +774,16 @@ private void sweepStaleTempFiles(String hashPrefix) {
         try (DirectoryStream<Path> stream = Files.newDirectoryStream(directory, hashPrefix + "*.tmp")) {
             final long now = System.currentTimeMillis();
             for (Path tmp : stream) {
+                // never sweep a steal-captured lock (<hash>.lock.<uuid>.tmp): it is a cross-process steal in
+                // progress, not an orphaned write temp, and ATOMIC_MOVE preserves the stale lock's old mtime
+                // onto it, so the age guard below would judge an in-flight capture sweepable and delete it -
+                // destroying a lock the stealer may be about to restore to its live owner. A save write temp is
+                // <hash><random>.tmp and never contains ".lock.", so this only excludes captures. A capture
+                // orphaned by a crash mid-steal is rare and harmless (the canonical lock path is left free), so
+                // it is deliberately not reclaimed here.
+                if (tmp.getFileName().toString().contains(".lock.")) {
+                    continue;
+                }
                 try {
                     if (now - Files.getLastModifiedTime(tmp).toMillis() > lockStaleMillis) {
                         Files.deleteIfExists(tmp);

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

@@ -132,21 +132,25 @@ public class OidcDeviceAuth implements QuietCloseable {
     // tokens fail to parse with "String is too long".
     private static final int JSON_LEXER_CACHE_SIZE = 1024;
     private static final int JSON_LEXER_MAX_VALUE_BYTES = 1 << 20;
-    // the worst case a coordinated refresh can hold a FileTokenStore cross-process lock, as a multiple of
-    // httpTimeoutMillis: one refresh under the lock runs send + await + parse, plus a body drain on a parse
-    // failure, each separately bounded by httpTimeoutMillis. build() rejects a FileTokenStore whose
-    // lock-staleness window does not exceed this, so a peer never judges a live holder's lock stale mid-refresh
+    // the I/O portion of a coordinated refresh, as a multiple of httpTimeoutMillis: the refresh under the lock
+    // runs send + await + parse, plus a body drain on a parse failure, each separately bounded by
+    // httpTimeoutMillis. NOTE this multiple does NOT cover the connection phase that precedes the send - DNS
+    // resolution, the TCP connect, and the TLS handshake are NOT bounded by httpTimeoutMillis (the OS bounds the
+    // connect instead). build() requires the FileTokenStore staleness window to exceed this multiple as a floor;
+    // the default window adds ample headroom for a typical connection stall on top of it (see build())
     private static final int LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE = 4;
     // upper bound on the device code lifetime (the device authorization response's expires_in), so a
     // hostile or buggy provider cannot make the client poll for an absurd duration; matches the Python client
     private static final int MAX_DEVICE_CODE_TTL_SECONDS = 1800;
     // upper bound on the token cache lifetime (the token response's expires_in), so an absurd or hostile
     // value cannot overflow the timing arithmetic or make the client trust a token for absurdly long
     private static final int MAX_EXPIRES_IN_SECONDS = 3600;
-    // upper bound on the configurable HTTP request timeout. A token-endpoint round-trip never needs longer,
-    // and bounding it keeps a refresh held under the FileTokenStore cross-process lock (send + await + parse,
-    // plus a body drain on a parse failure - each separately bounded by this, so up to ~4x this) safely
-    // shorter than that store's lock-staleness window, so a slow refresh's live lock is not stolen by a peer
+    // upper bound on the configurable HTTP request timeout. A token-endpoint round-trip never needs longer, and
+    // bounding it keeps the I/O portion of a refresh held under the FileTokenStore cross-process lock (send +
+    // await + parse, plus a body drain on a parse failure - each separately bounded by this, so up to ~4x this)
+    // a known, bounded multiple, so the store's staleness window can be sized to dominate it. The connection
+    // phase (DNS + TCP connect + TLS) is bounded by the OS, not by this, and the default staleness window
+    // leaves headroom for it (see Builder.build())
     private static final int MAX_HTTP_TIMEOUT_MILLIS = 120_000;
     // upper bound on the poll interval, both the initial value and the growth after a slow_down or 429, so
     // a hostile or buggy provider cannot stall the poll loop; matches the Python client
@@ -1585,11 +1589,15 @@ public OidcDeviceAuth build() {
             validateEndpointOrigins(parsedTokenEndpoint, deviceEndpoint, issuerEndpoint);
             ClientTlsConfiguration tls = tlsConfig != null ? tlsConfig : defaultTlsConfig();
             // a FileTokenStore steals a lock older than its staleness window, presuming a crashed holder; that
-            // window must exceed the worst-case time a live refresh holds the lock (the refresh under the lock is
-            // bounded by httpTimeoutMillis, up to LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE times it), or a peer could steal
-            // a live holder's lock mid-refresh and reopen the rotating-refresh-token race the lock prevents. The
-            // store cannot see this client's timeout, so enforce the invariant here, where both are known, rather
-            // than leave the caller to size it by hand. A non-coordinating TokenStore is exempt - it takes no lock.
+            // window must exceed the worst-case time a live refresh holds the lock, or a peer could steal a live
+            // holder's lock mid-refresh and reopen the rotating-refresh-token race the lock prevents. Enforce the
+            // bounded part of that worst case here, where both values are known: the refresh I/O under the lock is
+            // up to LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE x httpTimeoutMillis. This is a FLOOR, not the whole story - the
+            // connection phase (DNS + TCP connect + TLS) that precedes the send is bounded by the OS, not by
+            // httpTimeoutMillis, so the staleness window must also clear a connection stall on top of this floor.
+            // The default 600s window leaves ~120s of headroom over the floor even at the 120s timeout cap, which
+            // covers a typical connection stall; a caller raising httpTimeoutMillis should raise lockStaleMillis
+            // to keep that headroom. A non-coordinating TokenStore is exempt - it takes no lock.
             if (tokenStore instanceof FileTokenStore) {
                 long minStaleMillis = (long) LOCK_HOLD_HTTP_TIMEOUT_MULTIPLE * httpTimeoutMillis;
                 long staleMillis = ((FileTokenStore) tokenStore).getLockStaleMillis();