feat(core): OIDC sign-in via device flow (RFC 8628)

[glasstiger]

Adds interactive OIDC sign-in to the Java client using the OAuth 2.0 Device Authorization Grant (RFC 8628). A process with no local browser — a remote notebook kernel, a container, a headless job — can sign a human in against QuestDB Enterprise: the user authorizes on any device (laptop or phone) while the process only makes outbound calls to the identity provider.

On first use it prints a verification URL and a short code (and, by default, also tries to open the URL in a local browser); once the user authorizes, the token is cached in memory and refreshed silently on later calls.

import io.questdb.client.Sender;
import io.questdb.client.cutlass.auth.OidcDeviceAuth;

// Discover the client id, scope and endpoints from the QuestDB server's /settings:
try (OidcDeviceAuth auth = OidcDeviceAuth.fromQuestDB("https://questdb.example.com:9000")) {
    auth.signIn(); // sign in once: prompts on first use, then caches and refreshes

    // Pass a token provider, not a fixed string: the sender pulls a freshly refreshed token on each
    // request, so a long-lived sender keeps working as the token rotates. getToken() refreshes
    // silently and never prompts on the flush path.
    try (Sender sender = Sender.builder(Sender.Transport.HTTP)
            .address("questdb.example.com:9000")
            .enableTls()
            .httpTokenProvider(auth::getToken)
            .build()) {
        sender.table("trades")
                .symbol("symbol", "ETH-USD")
                .doubleColumn("price", 2615.54)
                .atNow();
    }
}

What's new

OidcDeviceAuth (io.questdb.client.cutlass.auth) — runs the flow and owns the token:

• OidcDeviceAuth.fromQuestDB(url) discovers the client id, scope, audience and IdP endpoints from the server's unauthenticated /settings; OidcDeviceAuth.fromQuestDB(url, DiscoveryOptions) adds an identity-provider pin (.issuer(...)), a TLS config, an allowInsecureTransport opt-in, and the prompt (see Discovery and trust below); OidcDeviceAuth.builder() configures the identity provider explicitly.
• signIn() signs in interactively on first use, then serves a cached token and refreshes it silently; getToken() never prompts and never waits behind an interactive sign-in (safe on a request/flush path); getAuthorizationHeaderValue() returns the full Bearer … value; clearCache() drops the cached token so the next signIn() re-signs-in; close() cancels an in-flight sign-in (observed between polls, so it can take up to one HTTP request timeout to return). Calls are serialized by a ReentrantLock; getToken() uses tryLock and fails fast rather than wait behind an interactive sign-in. Token state is in-memory only by default; pass a TokenStore to persist it across restarts (see Token persistence below).

Sender integration — new HttpTokenProvider interface and Sender.builder(...).httpTokenProvider(auth::getToken). The sender pulls a freshly refreshed token on every request, so a long-lived sender keeps working as the token rotates — unlike a fixed httpToken(...), which is captured once and eventually starts returning 401s. Mutually exclusive with httpToken/httpUsernamePassword. Supported over HTTP and WebSocket transport (a WebSocket sender re-queries the provider on every (re)connect/upgrade); rejected for TCP and UDP. The two transports differ in mechanism but both keep the producer alive across a sustained token outage: over HTTP a failed pull leaves the request token-pending and is retried on the next row; over WebSocket the token must be obtainable when build() runs (the initial handshake fails fast otherwise), after which a pull that keeps failing on later reconnects is retried indefinitely, with the buffered rows held in store-and-forward, until a token is available again — a token outage does not terminate a running WebSocket sender, just as a persistent transport reconnect failure does not (store-and-forward Invariant B). The first pull is deferred off the build path to the first row, so the documented construct → signIn() → send ordering works and a provider that throws leaves the request retriable instead of corrupting the sender.

QWP egress query client — QwpQueryClient.withBearerTokenProvider(HttpTokenProvider) accepts the same on-demand provider, so OidcDeviceAuth::getToken plugs into the egress query path as well as ingress. The provider is queried at every WebSocket upgrade — the initial connect() and each failover reconnect — so a long-lived query client follows token rotation; each returned token is validated before it reaches the header, and a provider that throws fails that connection attempt (matching the ingress sender). Mutually exclusive with withBearerToken/withBasicAuth.

DeviceCodePrompt / DeviceAuthorizationChallenge — how the verification URL and user code are shown. The default, DeviceCodePrompt.openBrowser(), prints the instructions to System.out and also tries to open the verification URL in the local default browser; the browser open is best-effort (skipped on a headless JVM, without the java.desktop module, or for a non-http(s) URL, and disabled by -Dquestdb.client.oidc.open.browser=false) and never blocks or fails sign-in. Use DeviceCodePrompt.SYSTEM_OUT to print only, or supply your own to render a clickable link or a QR code, e.g. in a notebook.

audience — builder().audience(...) / discovered from acl.oidc.audience. When set, the audience parameter is sent on the device-authorization and refresh requests, for providers that require it to stamp the aud claim QuestDB expects.

The token can be presented to QuestDB over any auth path the server already validates:

• REST / ingestion — Authorization: Bearer <token>.
• PG-wire — connect as _sso with the token as the password (requires acl.oidc.pg.token.as.password.enabled=true on the server).

Discovery and trust

fromQuestDB(...) takes the IdP endpoints from the server's unauthenticated /settings, so by default it trusts that server to designate where the user signs in: a spoofed, compromised, or man-in-the-middled server could otherwise redirect the sign-in — and the long-lived refresh token — to an attacker-controlled identity provider. An optional DiscoveryOptions.issuer(...) pin addresses this, and also covers servers that do not advertise a device-authorization endpoint. The pin separates two sources of endpoints and trusts them differently — an endpoint the untrusted /settings advertised is constrained to the issuer, while an endpoint read from the identity provider's own .well-known is trusted wherever the provider hosts it:

• .well-known discovery fallback. Current servers do not advertise the device-authorization endpoint. When it (and/or the token endpoint) is missing, a pinned issuer reads it from {issuer}/.well-known/openid-configuration. The discovery origin comes only from the caller-supplied issuer, never from a /settings-supplied value, so a tampered /settings cannot choose where discovery — and the credential POSTs it resolves — are aimed. Without a pin, discovery is refused rather than guessed.
• Co-location pin. validateEndpointOrigins, enforced on every construction path (discovery and the explicit builder()), requires the token and device-authorization endpoints to share one origin (RFC 8628 co-locates them on a single authorization server), so a tampered /settings or discovery document cannot siphon one of the two credential POSTs off to a different origin.
• /settings-advertised endpoints are pinned to the issuer. An endpoint the untrusted /settings response supplied must sit on the pinned issuer's origin, and — when the issuer has a path — under that path (compared segment by segment, rejecting ./.., percent-encoded traversal, and a percent-encoded path separator such as %2f or %5c, at every decode level). The path check matters for a path-based provider that shares one origin per tenant (e.g. a Keycloak realm path /realms/<realm>), where the origin check alone cannot stop a tampered /settings from steering credentials to a sibling tenant. The issuer is supplied out of band and cannot be forged.
• IdP-discovered endpoints are trusted where the issuer hosts them. An endpoint read from the issuer's own .well-known is neither origin-pinned nor path-scoped: that document is fetched from the pinned issuer origin and is authoritative for wherever the provider hosts its endpoints. This is deliberate — some providers (e.g. Google, Azure AD) serve their token and device endpoints from a different origin or path than the issuer, and discovery against them signs in normally. The co-location pin above still applies.
• Plaintext-channel pin. A /settings response fetched over plaintext http to a non-loopback host (only reachable with allowInsecureTransport) is MITM-able, so its advertised endpoints are not trusted to route credentials without an issuer pin.

Without a pin, the behaviour against an https server that advertises its endpoints is unchanged: that server is trusted, as before.

Security

• https is required by default for both the QuestDB server and the IdP endpoints; http is rejected unless the caller opts in with allowInsecureTransport(true). That opt-in relaxes only the QuestDB /settings link — the IdP device-authorization and token endpoints always require https (loopback excepted), so the device code and refresh token never cross the network in cleartext (matching the Python client).
• Tokens never leak into logs or exceptions. Only the HTTP status of a token/device response is captured; the body — which carries access, id and refresh tokens — is never retained or surfaced in a message. The captured status is validated to be exactly three digits, so a malformed or hostile status line cannot splice ANSI/control bytes into a later [httpStatus=…] echo, nor can a short all-digit status (2, 5) be misread as a 2xx/5xx class — a malformed-length status falls through to the terminal reject path.
• Untrusted IdP text is sanitized before it is shown in a prompt or an exception message: per-code-point stripping of control characters, ANSI escapes, CR/LF, and bidirectional / zero-width / Unicode-format characters (including supplementary-plane "tag" characters that arrive as surrogate pairs, and unpaired surrogates), so an attacker-influenced field cannot reorder, hide, or forge what a human reads — e.g. a right-to-left override that makes the displayed verification URL differ from the one the browser opens. A user code or verification URL that is non-empty on the wire but sanitizes to nothing is rejected (or, for verification_uri_complete, treated as absent) rather than shown as a blank line or handed to the browser launcher.
• The token itself is validated before use. The token QuestDB will actually receive — the id token when the server encodes groups in the token, the access token otherwise — is rejected if it carries a control or non-ASCII character (outside 0x20–0x7e) before it is cached, placed in the Authorization: Bearer header, or used as the PG-wire password — so a tampered or hostile identity provider cannot smuggle a CR/LF into the request the client then sends to the trusted QuestDB server. Only the served kind is checked; a stray character in the unused token kind, which never reaches the wire, no longer aborts an otherwise usable grant. (The JsonLexer change below decodes JSON escapes, which is what turns a \r/\n in a token into a real byte rather than two literal characters.)
• URLs are validated up front. Endpoint.parse rejects control characters, whitespace and display-unsafe code points anywhere in the url (so a tampered endpoint cannot inject a CR/LF into the request line or a bidi char into a log line), rejects bracketed IPv6 literals rather than mis-parsing them, rejects userinfo (user@host) — which the HTTP layer would otherwise try to connect to literally — terminates the authority at the first /, ? or # so a query or fragment is never folded into the host, and range-checks the port to 1..65535.
• Bounded against a hostile or stalled server. Response reads are capped by a 4 MiB byte limit and a wall-clock deadline that bounds the whole read — covering both a chunked response whose chunk-size line is dribbled a byte at a time and a Content-Length body dribbled through stalled TLS records, either of which previously could keep a single read running well past the deadline. After such a bounded-read abort the half-read poll connection is dropped so the next poll reconnects on a clean socket, rather than the loop spinning on the stalled response's leftover bytes until the device code expires. The device-code lifetime, the poll interval, and the token TTL are all clamped (defaults applied for absent/zero values, hard caps for absurd ones). A 429 with no OAuth error is treated as a transient back-off (a 429 that also carries a terminal error such as access_denied still aborts on the error); a transient transport failure or 5xx during polling keeps polling until the device-code deadline rather than failing the sign-in (RFC 8628; matches the Python client), while a definitive OAuth error or a terminal 4xx aborts immediately. The trade-off is that a persistently flaky network is no longer cut short by a separate error budget — it polls to the device-code deadline.

Token persistence (opt-in)

By default token state is in-memory only, so a restarted process re-runs the interactive device flow. Passing a TokenStore persists it, so the restarted process resumes from the saved refresh token (one silent token-endpoint round-trip) instead of re-prompting — getToken() then even works as the first call, with no explicit signIn().

• TokenStore SPI (io.questdb.client.cutlass.auth) — load/save/clear keyed by a non-secret TokenStoreKey (endpoints, client id, scope, audience, groups-in-token mode), plus an optional inLock hook for cross-process coordination. Wire it in with builder().tokenStore(...) or DiscoveryOptions.tokenStore(...). Persistence is best-effort: a store failure warns to System.err and the in-memory token is used regardless.
• FileTokenStore (the default) — one plaintext JSON file per identity under ${user.home}/.questdb/oidc-tokens/ (override with questdb.client.oidc.token.store.dir), the refresh token protected at rest by file permissions (0600 file, 0700 directory on POSIX) rather than encryption — the same approach gcloud, aws and gh take. The file name is a SHA-256 of the identity, so it leaks neither endpoint nor client id and several identities coexist. FileTokenStore.atDefaultLocation() / FileTokenStore.at(dir).
• Treated as untrusted on load. A persisted file is attacker-writable, so on load it is size-bounded, parsed defensively (a corrupt/oversized/garbage file is ignored, not fatal), its fingerprint re-checked against the live config (a token minted for one identity is never served for another), and the served token re-validated for control/non-ASCII characters before it can reach a header — the same CR/LF / non-ASCII rejection the device flow applies to IdP responses. A tampered far-future expiry is clamped, not trusted. A bad file degrades to a refresh or an interactive sign-in.
• Integrity and cross-process coordination. Each update is written to a temp file then atomically renamed, so a concurrent reader never sees a half-written credential. When the IdP rotates the refresh token on each refresh, the read-refresh-write is serialized across processes with an O_CREAT|O_EXCL lock file (not an OS advisory lock, which Java FileLock and Python flock cannot share); a process that cannot acquire the lock degrades to a lock-free refresh rather than stall.
• The on-disk format is a frozen cross-language contract (design/oidc-token-persistence.md): the file name, JSON schema, atomic-write and lock-file protocols are specified so the Python client (and others) can share one file.

Supporting changes

• JsonLexer now resolves JSON string escape sequences (\", \\, \/, \b \f \n \r \t, \uXXXX; lenient on malformed input), so string values arrive fully decoded. This also reaches the existing ILP error-response parser, which now sees decoded message/code/line/errorId fields.
• Response.recv(int timeout) (both the Content-Length and chunked implementations) bounds the whole read to the timeout in total, not per socket read, so a server that dribbles the body — the chunk-size line of a chunked response, or Content-Length bytes behind stalled TLS records — cannot keep a single read running past the caller's deadline. A non-positive timeout keeps the legacy unbounded behaviour. The existing ILP flush reads go through the no-arg recv(), which passes the configured client timeout (positive by default), so they now bound the whole body read to that timeout instead of re-arming it per socket read: a response that completes within the timeout is unaffected, while one that legitimately dribbles a single fragment for longer than the timeout — previously tolerated as long as each socket read made progress — now aborts.
• AbstractLineHttpSender plumbs the token provider through with a deferred, retriable per-request pull (a throwing or blank-returning provider leaves the request token-pending for the next row instead of corrupting the half-built request), so the very first send already carries a provider-sourced token. Its error rendering now routes every untrusted server-supplied string through putAsPrintable — the decoded JSON error body, the [http-status=…] field, and the line-protocol-version detection probe body — escaping control and Unicode format characters (bidi overrides, zero-width joiners, the BOM), so a hostile or proxied endpoint cannot reorder, hide, or forge the text shown in a LineSenderException (or spliced into a log line or terminal).
• QwpWebSocketSender sources its auth header from the token provider too, re-querying it on every connect/reconnect so a rotating token keeps a long-lived WebSocket sender authenticated.

Tradeoffs and limitations

• The origin pin behaves differently on the two construction paths. fromQuestDB(...) discovery trusts an endpoint read from the issuer's .well-known wherever the provider hosts it, so an off-origin provider (e.g. Google) signs in normally through a pinned issuer; only an endpoint the /settings response itself advertised is held to the issuer's origin and path. The explicit builder().issuer(...) pin is stricter — a plain sanity check that both supplied endpoints sit on the issuer origin — so an off-origin provider configured that way must have its issuer omitted, or its endpoints supplied to match. This matches the Python client.
• A failed token pull is handled differently per transport, but neither drops buffered rows: over HTTP it leaves the request token-pending and retries on the next row; over WebSocket the initial handshake must obtain a token at build() (it fails fast otherwise), after which a pull that keeps failing on later reconnects is retried indefinitely with the buffered rows held in store-and-forward, until a token is available again — a token outage does not terminate a running WebSocket sender (store-and-forward Invariant B). A getToken() provider that fails only transiently recovers on both transports; a long WebSocket outage grows store-and-forward (and eventually applies backpressure) rather than ending the sender.
• The co-location check requires the token and device-authorization endpoints to share an origin. A test that previously pointed the token endpoint at a dead second port to simulate an unreachable endpoint was reworked to drop a co-located connection instead (MockOidcServer.dropConnection()).
• The plaintext-channel pin's firing path is exercised end to end by reaching the loopback mock through a short-form 127.x address that the loopback classifier deliberately rejects as non-loopback. That trick relies on the OS resolver expanding the short form (BSD inet_aton, on Linux/macOS), which Windows getaddrinfo does not do, so that one end-to-end test is skipped on Windows; the loopback classifier itself is covered cross-platform.
• Persistence writes a long-lived refresh token to disk in plaintext, protected only by file permissions — anyone who can read the file holds a credential until the IdP expires or revokes it. This is why persistence is opt-in; for at-rest encryption, supply a TokenStore backed by an OS keychain or a secrets manager instead of FileTokenStore. On Windows POSIX permissions cannot be enforced, so the file currently relies on the user-profile directory's default ACL (owner-only ACL hardening is a follow-up); the client warns once to System.err.
• A store-coordinated getToken() may briefly wait to acquire the cross-process lock before a silent refresh (a few seconds at most for FileTokenStore — the acquire budget is capped — then it proceeds without the lock). It still never waits behind an interactive sign-in; this is a quick silent refresh, not an interactive wait.
• The Response.recv(int) whole-read bound (see Supporting changes) also tightens existing, non-OIDC ILP flushes. The no-arg recv() now bounds the whole flush-response body read to the configured client timeout instead of re-arming it per socket read, so a response that legitimately dribbles a single fragment for longer than that timeout — previously tolerated as long as each socket read made progress — now aborts. A healthy response is unaffected: the abort surfaces as a retryable HttpClientException that flush0 catches and retries like any transport failure, so the only observable effect is that a pathologically slow single fragment is retried rather than tolerated indefinitely, marginally widening the pre-existing ILP-over-HTTP at-least-once window.

Tests & docs

OidcDeviceAuthTest (~107 cases) + MockOidcServer, BrowserLauncherTest, LineHttpSenderTokenProviderTest, WebSocketTokenProviderTest + TestWebSocketServer, SenderBuilderErrorApiTest, JsonLexerTest, LineHttpSenderErrorResponseTest, DisplaySafeTest, ChunkedResponseTest/ResponseTest, QwpQueryClientTokenProviderTest, FileTokenStoreTest, OidcDeviceAuthPersistenceTest; runnable OidcDeviceFlowExample / OIDCAuthExample; and README "OIDC Sign-In (Device Flow)" and "Persisting the Token Across Restarts" sections. Coverage includes:

• .well-known discovery via a pinned issuer; a discovery document that omits the device-authorization endpoint
• the co-location check rejecting split-origin endpoints; the builder().issuer(...) origin pin rejecting off-origin endpoints; a /settings-advertised endpoint rejected when off the issuer origin; an endpoint discovered from the issuer's .well-known accepted even when off the issuer origin (the Google case)
• issuer path scoping: endpoints under the issuer path accepted; a sibling realm, percent-encoded traversal, an encoded path separator (%2f), and a percent-encoded backslash (%5c) rejected
• the plaintext-channel pin requiring an issuer pin for advertised endpoints over http (firing path skipped on Windows)
• the audience parameter discovered from /settings and sent on the device and refresh requests
• token-provider support over HTTP and WebSocket (initial upgrade and re-queried per reconnect; static token / username-password still work over WebSocket); rejected for TCP/UDP; mutual exclusion with other auth; null/empty provider token rejected; deferred build-time pull; no sender corruption when the provider throws after a flush; the QWP egress query client's token provider — header synthesis, per-resolve re-query, token validation, and mutual exclusion
• a token with a control or non-ASCII character rejected rather than sent (the served kind validated); tokens never echoed in messages
• bounded reads: a stalled body and an oversized body aborting on the deadline / 4 MiB cap; a chunked body read aborting when the chunk-size line is dribbled; a bounded-read abort dropping the dirty poll connection so the next poll reconnects and signs in
• the poll model: a 429 and a transient 5xx/transport failure keep polling to the deadline; a terminal 4xx and an OAuth error fail fast (including a 429 that also carries a terminal error); slow_down growth and the 60 s interval clamp; device-code-lifetime and clock-skew clamps
• Endpoint.parse rejecting a malformed url: userinfo (user@host), a bracketed IPv6 literal, an out-of-range port, and control/whitespace/display-unsafe characters
• a malformed HTTP status code rejected on both the device-authorization and the token-poll path: a non-numeric status, and a short all-digit status (2, 5) that must not be read as a 2xx/5xx class
• display sanitizing: bidi/zero-width, lone-surrogate and supplementary-plane format characters stripped from the challenge and OAuth error; a server's JSON error body, its HTTP status field, and the protocol-version probe body with such characters escaped, not rendered raw; a user code or verification URL that sanitizes to empty rejected, and a verification_uri_complete that sanitizes to empty treated as absent
• JsonLexer escape decoding, including a \uXXXX escape split across two parse fragments, and the lenient/exotic escape arms
• getToken() failing fast while another thread holds the lock in an interactive sign-in or a silent refresh; native-memory cleanup on the error/rejection construction paths
• token persistence: round-trip save/load with 0600/0700 permissions and control-char JSON escaping; a corrupt, empty, oversized, schema-version-mismatched, or per-field fingerprint-mismatched file ignored; a tampered far-future expiry clamped and a CR/LF served token rejected on load; a restart serving a valid persisted token (or silently refreshing an expired one) without re-running the device flow; rotating vs non-rotating refresh write behaviour; a swallowed save not replaying a revoked token; the cross-process lock-file protocol (acquire, mutual exclusion, stale-steal, degrade); getToken() degrading to a lock-free refresh when a peer holds the lock; the HTTP-timeout cap; the file-name hash pinned as a cross-language contract

Review follow-ups

A level-3 review of this branch surfaced the issues below; all are fixed here, each production fix with a regression test proven to fail without it (see the commit history for detail).

Confirmed defects:

• Blank served token: adopt()/storeTokens() accepted a whitespace-only served token (it passed isEmpty()/hasOnlyTokenChars() vacuously), so signIn() reported success and getToken() served a blank Bearer header the server only answers with 401, never falling back. Now rejected via Chars.isBlank; a blank served kind is folded to absent so selectToken() surfaces the actionable error.
• getToken() lock contention: the unconditional tryLock() failed fast on any lock hold, so concurrent callers sharing one OidcDeviceAuth threw on every token refresh. It now waits briefly behind a peer's silent refresh (bounded by httpTimeoutMillis) and fails fast only behind an interactive sign-in.
• SYNC initial connect: a token-provider failure was treated as a transport outage and retried for the whole reconnect budget (5 min default) then wrapped. It now fails fast with the provider's own exception, matching the OFF-mode and background-reconnect paths.

Hardening and coverage:

• Discovery parsers reject array-wrapped JSON, so {"config":[{...}]} can no longer surface fields at the trusted config depth.
• The cross-process lock file is created with its owner stamp in one atomic exclusive open, removing the create-then-stamp gap a GC pause could straddle (design doc updated; the Python client must mirror it).
• The ILP token-provider request is built once per flush rather than twice. (validateToken still re-scans every pulled token by design - a provider may mutate a reused buffer between flushes - but that scan is O(token length), runs once per flush, and is dwarfed by the network round-trip.)
• Added regression tests for previously untested load-bearing guards: the parseHex4 non-ASCII guard, the raw .. issuer-path reject, the FileTokenStore size caps, the store-and-forward credential-timer reset, and the ILP flush whole-read timeout bound.
• Docs: HttpTokenProvider.getToken() now discloses the OS-bounded connect stall; TokenStore.inLock documents its no-reentrancy contract; FileTokenStore states the concurrent-refresh residual (token-family revocation on a reuse-detecting IdP) instead of understating it.

Tandem

OSS: questdb/questdb#7331
Ent: https://github.com/questdb/questdb-enterprise/pull/1090

[mtopolnik]

[PR Coverage check]

😍 pass : 1764 / 1961 (89.95%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/line/http/LineHttpSenderV1.java	0	2	00.00%
🔵	io/questdb/client/cutlass/http/client/HttpClient.java	0	2	00.00%
🔵	io/questdb/client/cutlass/line/http/LineHttpSenderV2.java	0	2	00.00%
🔵	io/questdb/client/cutlass/auth/BrowserLauncher.java	12	22	54.55%
🔵	io/questdb/client/cutlass/qwp/protocol/QwpTableBuffer.java	2	3	66.67%
🔵	io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java	21	26	80.77%
🔵	io/questdb/client/cutlass/auth/FileTokenStore.java	360	435	82.76%
🔵	io/questdb/client/cutlass/qwp/client/QwpCredentialUnavailableException.java	5	6	83.33%
🔵	io/questdb/client/cutlass/auth/DeviceCodePrompt.java	22	24	91.67%
🔵	io/questdb/client/cutlass/auth/OidcDeviceAuth.java	922	1009	91.38%
🔵	io/questdb/client/cutlass/line/http/AbstractLineHttpSender.java	65	71	91.55%
🔵	io/questdb/client/cutlass/auth/TokenStoreKey.java	37	39	94.87%
🔵	io/questdb/client/cutlass/qwp/client/QwpQueryClient.java	30	31	96.77%
🔵	io/questdb/client/Sender.java	29	30	96.67%
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java	37	37	100.00%
🔵	io/questdb/client/cutlass/auth/TokenStore.java	1	1	100.00%
🔵	io/questdb/client/std/str/DirectUtf8Sink.java	9	9	100.00%
🔵	io/questdb/client/std/str/Utf16Sink.java	12	12	100.00%
🔵	io/questdb/client/cutlass/qwp/client/QwpUdpSender.java	1	1	100.00%
🔵	io/questdb/client/cutlass/auth/DeviceAuthorizationChallenge.java	12	12	100.00%
🔵	io/questdb/client/cutlass/http/client/AbstractChunkedResponse.java	10	10	100.00%
🔵	io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java	32	32	100.00%
🔵	io/questdb/client/cutlass/json/JsonLexer.java	65	65	100.00%
🔵	io/questdb/client/cutlass/auth/PersistedToken.java	12	12	100.00%
🔵	io/questdb/client/cutlass/auth/OidcAuthException.java	33	33	100.00%
🔵	io/questdb/client/cutlass/http/client/AbstractResponse.java	8	8	100.00%
🔵	io/questdb/client/std/str/DisplaySafe.java	20	20	100.00%
🔵	io/questdb/client/HttpTokenProvider.java	7	7	100.00%

[glasstiger]

Code review — OIDC device flow (independent level-3 pass)

Reviewed at 7a95bb3a (base bc722d4f), 60 files, ~+8500/−130. Authoritative diff taken from local git (gh pr diff serves a stale cache for this branch); source read from the working tree. This is a fresh, independent pass — 10 parallel agents across all 13 review dimensions (correctness, concurrency, performance, resources, cross-context callers, tests ×3, code quality, metadata, plus a fresh-context "find what's wrong" agent), with every high-severity claim re-verified against source. It deliberately targets the newest, least-reviewed surface — the store-and-forward drainer's rotating-credential auth logic (BackgroundDrainer / CursorWebSocketSendLoop / QwpWebSocketSender), which the prior review passes predate.

Verdict: approve. No Critical or blocking issues.

The store-and-forward data-loss invariants hold: NACK/close tiering is correctly tiered (UNKNOWN fails open, no watermark advance past a NACKed frame), a transient class cannot drive a wrong quarantine (it only inflates the wall clock = more JWKS convergence time), the rotating-credential ride-out uses counters separate from the cap-gap budget, and the close()/cancel() credential-pull interrupt is a correct Dekker volatile handshake. The security surface (endpoint origin+path pinning, token-char validation, display sanitizing, hostile-file defense, response DoS bounds) held under adversarial tracing. Prior-review items are genuinely fixed (verified below).

Moderate

M-1 — Hostile chunk-size guard is incomplete: a 17-digit hex size wraps past size < 0 → silent truncation / desync. (in-diff)
AbstractChunkedResponse.java:150-159. This PR adds the if (size < 0) guard (and its comment reasons about parseHexLong's unchecked val << 4), but only handles the negative wrap. Numbers.parseHexLong has no digit-count/overflow bound, so "10000000000000000" (17 hex digits) wraps to size = 0 → hits the size == 0 chunk terminator → silent body truncation, and "...005" → size = 5 → chunk desync. Both are >= 0 and bypass the guard. Reachable from the untrusted-IdP threat model this PR introduces. Impact is bounded (no spin — that is fixed; no injection; the 4 MiB body cap + deadline backstop it), so not Critical, but it's a real gap in hardening code the author meant to close. Fix: reject a chunk-size line >16 hex digits (or that overflows), or cap size at the max body size, before the size < 0 check.

M-2 — getToken() can stall the ILP flush hot path on an unbounded in-process lock. (in-diff)
FileTokenStore.java:288 (via OidcDeviceAuth.getToken():531 → tryRefreshCoordinated():1677 → inLock). getToken() bounds the per-instance lock (tryLock) and the cross-process file lock (degrades after a budget), but the in-process PROCESS_LOCKS entry is taken with a plain unbounded processLock.lock() — deliberately not degradable (to prevent two same-identity instances double-POSTing a rotating refresh token). So two OidcDeviceAuth instances for one identity in a JVM (the documented ILP Sender + QwpQueryClient sharing a FileTokenStore): if instance B holds the lock during a black-holed token-endpoint connect (OS-bounded, ~2 min), instance A's getToken() on the flush thread blocks the whole time. The ~2 min worst case is disclosed, but the HttpTokenProvider javadoc attributes the sub-refresh wait to the cross-process lock and omits this unbounded in-process path. Fix: bound the in-process acquire in the getToken path with a tryLock(budget) that fails fast to token-pending (the flush path tolerates that), or at least correct the javadoc. A naive unconditional bound reopens the family-revocation race, so fail-fast-to-pending is the right framing.

M-3 — recv(int) whole-call bound can turn a committed 2xx flush into a retry → duplicate rows. (effect out-of-diff; cause in-diff)
AbstractLineHttpSender.java:718 → :746. On the flush success branch, consumeChunkedResponse(response, actualTimeoutMillis) runs outside any local try; if the new recv(int) bound makes it throw while draining an already-committed 2xx chunked body, it propagates to the retry catch at :746, which re-sends the same buffer (pendingRows isn't reset until after the break) → duplicate rows. Not reachable against a real QuestDB server — ILP success is 204 No Content and consumeChunkedResponse early-returns on !isChunked() (:642); it needs an intermediary rewriting success into a slow (>30 s/fragment) chunked 2xx. Already disclosed in the PR's Tradeoffs as a marginal at-least-once widening. Verified real, but narrow. Fix (optional): on the success path, treat a recv timeout while draining an already-2xx body as non-retryable.

M-4 — The marquee rotating-401 fix is under-pinned by its regression tests. (test efficacy)
BackgroundDrainerDurableAckRetryTest (:324, :326-328, :352-353), BackgroundDrainerMidDrainAuthRejectTest:161. newDrainer uses a 60 s reconnect budget, so across the few-ms of fast attempts elapsed < budget is always true and masks the attempts < 6 clause — assertEquals(DEFAULT+1, attempts()) rides 6 failures and succeeds on the 7th for any threshold (even 1); assertTrue(attempts() >= DEFAULT) passes for 5/6/7 alike. A clause-deletion mutant on the wall-clock floor survives, the instanceof QwpAuthFailedException type-guard has no test throwing a non-421 WebSocketUpgradeException with a dynamic credential, and none of these tests carries @Test(timeout=…) — an unbounded-ride-out regression hangs the suite rather than failing. Fix: a test with a trivially-met budget asserting the exact attempts() == 6 at quarantine; a floor test with a large attempt cap; a non-QwpAuthFailedException upgrade-reject case; add @Test(timeout=…) to all ride-out tests.

M-5 — PR description contradicts the code (and its own javadoc) on WebSocket token-outage durability. (PR body)
The description says a sustained WebSocket token outage "terminates the sender for good, like any persistent reconnect failure." The code does the opposite — a running WS sender treats a credential-unavailable as transient under Invariant B and retries forever, holding rows in SF (CursorWebSocketSendLoop.java:1928-1962), and the in-code Sender.httpTokenProvider javadoc agrees ("A token outage does not terminate a running WebSocket sender"). The code is the SF-correct, data-safe behavior; the description is the outlier. Since the PR body becomes release notes and shapes the operator's data-at-risk mental model, correct it to match the code.

Minor (grouped)

Liveness / worker-pinning (all data-safe):

• BackgroundDrainer.java:388-390: the rotating-auth ride-out quarantines only on attempts>=6 AND elapsed>=budget. Under a non-default unbounded reconnect budget (reconnectMaxDurationMillis = Long.MAX_VALUE, which the builder allows), elapsed>=budget is never true, so a genuinely-revoked credential is retried forever and the orphan slot never quarantines — contradicting the DEFAULT_MAX_DYNAMIC_CREDENTIAL_AUTH_ATTEMPTS javadoc ("reaches a human … rather than pinning … forever") and diverging from the cap-gap sibling's OR escape (:465-467). Give it a hard attempt-cap OR term. (Default 300 s budget works; observable via the error handler regardless.)
• An adversarial server (accept-with-valid-token → drop-before-ACK → 401-on-reconnect, repeated) resets the ride-out each recycle and can pin a worker; an always-throwing provider is retried forever by design. Consider a progress guard on the recycle.

Defense-in-depth (nil reachability today):

• Loaded refreshToken is not char-validated in OidcDeviceAuth.adopt() (:1140, :1162) — only the served kind is. Safe today (sole egress URL-encodes it); a future non-encoding sink turns a hostile file into CRLF injection. Add hasOnlyTokenChars.
• fetchJson discovery parses the /settings and .well-known body regardless of HTTP status (:714). Not exploitable (endpoints stay pinned); check 2xx for clearer failures.

Doc / comment cluster:

• Three stale System.err references where the code uses SLF4J: OidcDeviceAuth.warnPersistence comment (:1681), design/oidc-token-persistence.md:537, and the persistence warning path.
• Misleading recv-bound comment AbstractLineHttpSender.java:712-717 ("bounds each read, not the whole body cumulatively" → it bounds each recv() call, re-armed per fragment).
• Sender.httpTokenProvider javadoc (:2327) says a WS token "must be obtainable when build() runs" — false for ASYNC initial-connect mode; and (:40) attributes the sub-refresh wait solely to the cross-process lock (see M-2).
• firstDynamicCredentialAuthFailureNanos uses 0L as the "no failure" sentinel while the analogous cap-gap field deliberately uses -1L because nanoTime() may legitimately be 0; match the hardened convention.

Naming / metadata:

• Boolean fields lack is/has (groupsInToken, closed, interactiveSignInInProgress, storeLoadAttempted, warnedNoPosixPerms) — getters are correctly prefixed; pervasive idiom, low priority.
• PR title has no leading verb → feat(core): add OIDC sign-in via device flow (RFC 8628); optionally add the documentation label.

Integration nits:

• QwpQueryClient.withBearerTokenProvider doesn't reject a double-set (:1152), unlike its ingress twin.
• A fully auth-gated /settings blocks the unauthenticated protocol-version probe for a token-provider HTTP sender (AbstractLineHttpSender.java:284) — likely moot since QuestDB serves /settings unauthenticated; document that such users set protocol_version explicitly.
• stampTokenIfPending leaves isTokenPending=true if authToken()/withContent() throw mid-stamp (needs a multi-MB token) → double-write on retry; clear the flag right after validateToken.
• Interrupt-flag hygiene leak: a ClosedByInterruptException during a FileChannel read leaves the interrupt flag set past the shield (busy-spins one parkNanos); low impact (only fires in the stop protocol).

Test efficacy / coverage / quality:

• testRejectedBuildDoesNotLeakNativeMemory is still tautological — the rejection (build():1750) precedes construction (:1777, lexer allocated last at :245), so the rejected path never allocates the lexer; the RSS-equality assertion holds unconditionally. The real leak path is covered by two sibling tests; this one only guards a construct-before-validate reorder. Restructure or drop as redundant.
• Vacuous assertions: 7 dead assertNotEquals in OidcDeviceAuthPersistenceTest (after an assertEquals pins the value), DisplaySafeTest isDisplaySafe/isUnsafeForDisplay pairs (the latter is return !isDisplaySafe), testHttpTokenProviderAcceptedForWebSocket (asserts only the absence of one wording), no-assertion smoke tests (HttpTokenProviderTest:35, BrowserLauncherTest:43).
• Coverage gaps: the WS supplier's validateToken reject branch (Sender.java:3442) is untested (deletable, whole WS suite still passes); cross-process file-lock exclusion has no executable guard (passes solely via PROCESS_LOCKS); clamp-boundary tests use values far from their edges (off-by-one survives); the chunk-size zero/small-positive wrap (M-1) is untested.
• Concurrency-test correctness: testConcurrentStealContentionDegradesCleanly has no barrier (threads serialize on PROCESS_LOCKS, never actually contend); two inLock worker tests swallow spawned-thread exceptions.
• Test-code quality: systemic multi-paragraph @Test rationale bloat (worst in LineHttpSenderTokenProviderTest, WebSocketTokenProviderTest); QwpQueryClientTokenProviderTest:249 hand-rolls a raw ServerSocket instead of reusing MockOidcServer; SEGMENT_SIZE_BYTES = 16384L should be 16_384L; MockOidcServer dribble()/stall() factories break the alphabetical run.

Checked and cleared (so they don't get re-raised)

• Long.MIN_VALUE serializes as bare null — fixed: putLongMember uses Numbers.append(sink, value, false) (FileTokenStore.java:531); the three golden SHA-256 filename hashes recompute to an exact match.
• Array-wrapped JSON surfaces fields at config depth — fixed: all four OIDC parsers gate every read on arrayDepth == 0.
• Builder.httpTimeoutMillis unvalidated — fixed: now rejects <= 0 and > 120_000.
• Blank served token reaches a Bearer header — fixed on all served paths (adopt/storeTokens/tryRefresh/selectToken via Chars.isBlank).
• Clock-skew collapse for stored tokens — fixed (uses the stored issued lifetime, not the shrinking remaining span).
• acquireLock deletes a peer's lock by bare path (:568) — fixed (degrades lock-free instead).
• close()/cancel() misses a cancellation — false positive: a correct Dekker volatile handshake (cancel writes cancelled then reads credentialPullThread; the I/O thread publishes then reads cancelled), verified both ways; cancellation cannot be missed.
• testSaveFailureThenRefreshDoesNotReplayRevokedToken is flaky (Thread.sleep(1_200) vs 1 s TTL) — refuted: the skew cap expires the token at issue+500 ms, a deterministic ~700 ms margin.
• Endpoint-pinning bypass / token→header injection / identity confusion / hostile-file DoS — all traced clean (origin+path pin with multi-decode traversal defense, 0x20–0x7e token gate, byte-exact fingerprint re-check, 1 MiB cap + iterative lexer).

Summary

5 Moderate, ~20 Minor verified; ~9 draft findings dropped as already-fixed or false positives (including the flaky-sleep worry, refuted, and the cancel() handshake, verified correct both ways). Every Moderate is in-diff or in the PR body except M-3, whose effect lands at the out-of-diff ILP flush success path (cause in-diff). The cross-context pass walked every out-of-diff caller of the changed shared symbols (putAsPrintable/DisplaySafe, JsonLexer, Response.recv(int), DirectUtf8Sink, the QWP name-escaping paths) and confirmed them byte-identical for the ASCII/healthy-server cases the existing ILP tests exercise — 0 out-of-diff breaks. Worth doing before merge: M-1 (complete the chunk-size guard) and M-4 (pin the rotating-401 thresholds + add @Test(timeout)); M-2 / M-3 / M-5 are judgment calls best made explicitly rather than by omission.

_{🤖 Generated with Claude Code}

[glasstiger]

Code review — OIDC device flow (RFC 8628), independent level-3 pass

Reviewed at head 7a95bb3a (base bc722d4f), 56 files, +16,151/−210. Client-only (the tandem OSS #7331 / Ent #1090 were not examined). Method: independent verification of the five open Moderate findings against source, plus 10 parallel dimension agents (parsing security, store-and-forward invariants, concurrency, resources, cross-context callers, tests ×3, code quality, metadata, and a fresh-context adversarial pass). Every finding was re-verified against working-tree source; conflicting agent claims were resolved both ways.

This pass deliberately targeted the newest, least-reviewed surface — the store-and-forward rotating-credential drainer (BackgroundDrainer / CursorWebSocketSendLoop / QwpWebSocketSender).

Verdict: approve. No Critical; the one Moderate (M-5) has been addressed during this review; everything else is Minor.

The store-and-forward data-loss invariants hold: NACK never advances the ack watermark, UNKNOWN fails open, transient reconnect has no deadline, no transport error reaches a running producer, and the fixed-vs-dynamic credential gate is correct. The security core (endpoint origin+path pinning, served-kind token-char validation on every egress, display sanitizing, hostile-file defense, bounded reads) and the concurrency/resource surface (lock discipline, no jsonLexer use-after-free, a correct Dekker cancel handshake, no native leak on error/cancel paths) all held under adversarial tracing. The cross-context pass cleared every out-of-diff caller of the seven changed shared symbols — 0 out-of-diff defects.

Committed-binary gate: pass — no added/modified file is binary.

---

Moderate — addressed

M-5 (PR body) — the description contradicted the code on WebSocket token-outage durability. ✅ Fixed in the PR description during this review.

The body previously stated that over WebSocket "a pull that keeps failing past the sender's reconnect budget terminates the sender for good" and "a long outage ends a WebSocket sender." The shipped code does the opposite, and correctly so: in the running drainer, CursorWebSocketSendLoop.java:1928-1962 treats a QwpCredentialUnavailableException as transient under Invariant B — "retry indefinitely with capped backoff — NEVER bound by a wall-clock budget and NEVER latch a terminal" — holding the un-acked rows in store-and-forward. The in-code Sender.httpTokenProvider javadoc agrees ("A token outage does not terminate a running WebSocket sender"). The behavior the body described would itself have been a Critical store-and-forward violation (a reconnect budget dropping a producer SF promised to keep alive). Because the body becomes release notes and shapes the operator's data-at-risk model, both passages were corrected to match the code/javadoc: a running WS sender is not terminated — it retries indefinitely and buffers to SF, and a long outage grows SF/disk (eventually backpressure) rather than dropping the sender. The correct SYNC initial-connect fail-fast note was left intact.

---

Minor (none blocking)

1. M-1 (in-diff) — the hostile chunk-size guard is incomplete, and its comment is factually wrong. AbstractChunkedResponse.java:150-159 adds only if (size < 0), but Numbers.parseHexLong (Numbers.java:346-360) has no digit-count bound and no overflow check (val = (val<<4)+digit). So 10000000000000000 (17 hex digits) wraps to size == 0 → chunk terminator → silent body truncation, and ...0005 wraps to size == 5 → chunk desync — both bypass the guard. Severity is Minor: the guard's anti-spin purpose is met, the reachable path (untrusted IdP body) fails closed behind parseBody's 4 MiB cap + wall-clock deadline + parseLast() mid-JSON rejection, and the attacker already controls the whole body (no new capability). The in-code comment (:152-157) claiming "16 or more hex digits … wraps to a negative value" is false (7fffffffffffffff stays positive; 17 digits wrap to arbitrary non-negative values). Fix: reject a chunk-size token longer than 16 hex digits (or cap at max body size) before the guard, correct the comment, and add the two missing tests — the existing testOverflowingChunkSizeIsRejectedRatherThanSpun only exercises the negative-wrap 8000000000000000.

2. M-2 (in-diff) — getToken() on the flush path can stall on an unbounded in-process lock. The per-instance lock is bounded (acquireForGetToken, OidcDeviceAuth.java:1068), but FileTokenStore.inLock then takes processLock.lock() (FileTokenStore.java:288) — a plain unbounded ReentrantLock shared across two OidcDeviceAuth instances of one identity. If a peer holds it during a black-holed token-endpoint connect (bounded by the OS TCP-connect timeout, not httpTimeoutMillis), the flush thread blocks the whole time. Liveness/latency only, no data loss — and it is fully documented in the getToken() javadoc (:498-503, naming the exact ILP-Sender + QwpQueryClient scenario), so this is a disclosed tradeoff. Optional: bound the in-process acquire on the getToken path with tryLock(budget) degrading to token-pending (the flush path tolerates that), keeping signIn() blocking.

3. M-4 (test efficacy) — the rotating-401 regression tests leave the load-bearing clauses unpinned. BackgroundDrainer.java:388-390 quarantines only when attempts >= 6 AND elapsed >= budget. Under the 60 s FAST_RECONNECT_MAX_DURATION_MILLIS, elapsed < budget always holds, so deleting the attempt-cap clause survives every test (the wall-clock floor is tested — the earlier "floor mutant survives" note had this backwards). The instanceof QwpAuthFailedException type-guard is also unpinned (no test throws a non-421 upgrade reject with a dynamic credential), and the three direct-call rotating tests carry no @Test(timeout=…), so an unbounded-ride-out regression hangs the suite rather than failing (the MidDrain tests are guarded by a join(20s)+fail). Fix: a small-budget/large-backoff test that exhausts the wall clock in fewer than 6 attempts and asserts attempts() >= 6; a dynamic-credential non-421 upgrade-reject case; and @Test(timeout) on the direct-call tests.

4. Orphan-drainer liveness cluster (all data-safe; worth an explicit design decision). The orphan BackgroundDrainer javadoc promises a genuinely-broken credential "reaches a human … rather than pinning the slot and a drainer-pool worker forever." Three paths defeat that (rows stay safe on disk in every case; the risk is pool-worker starvation → backpressure):

• (a) AND-vs-OR asymmetry — :388-390 requires both the attempt cap and the wall-clock floor to quarantine, but under the intended "effectively unbounded" reconnect budget (Long.MAX_VALUE, contemplated at :338-348; validated only > 0 at Sender.java:2769) elapsed >= budget is never true, so it never quarantines. The cap-gap sibling (:467-468) uses OR and does not have this. Fix: cap attempts independently of the budget.
• (b) client-side credential failure never quarantines — QwpCredentialUnavailableException (provider throws / validateToken throws) misses every typed catch and lands in catch (Throwable) at :500, classified transient → retried forever. A permanently-broken provider pins the worker — asymmetric with the server-side-401 ride-out.
• (c) ride-out counter resets per recycle — a mid-drain 401 re-invokes connectWithDurableAckRetry() at :931, resetting the per-drain locals at :327/:330, so a flapping credential never accumulates to the threshold. The "Never reset" comment (:322-326) overstates the invariant (true within one invocation, false across recycles).

These are Invariant-B-adjacent (favor data safety over worker liveness) and may be intentional — but the javadoc promise and the server-vs-client asymmetry make them worth an explicit sign-off, or a javadoc correction if by design.

5. API parity (in-diff) — QwpQueryClient.withBearerTokenProvider accepts a silent double-set. QwpQueryClient.java:1152-1162 guards provider == null and cross-exclusion with withBearerToken/withBasicAuth, but not tokenProvider != null, so a second call silently overwrites (last-wins) — unlike its ingress twin Sender.httpTokenProvider (:2345), which throws. Add the tokenProvider != null guard for parity.

6. Test-code quality (in-diff).

• Tautological OidcDeviceAuthTest.testRejectedBuildDoesNotLeakNativeMemory — the build() rejection precedes the constructor's lexer alloc (allocated last), so the RSS-equality assertion holds unconditionally; the sibling success test is the real leak guard.
• QwpQueryClientPostConnectGuardTest leaks a native QwpQueryClient on each of 19 assertRejects calls (no close(), no assertMemoryLeak — it would fail today if wrapped) and reaches connected state via setAccessible reflection against the class's documented *ForTest no-reflection convention. Largely pre-existing (~2 diff lines here), but worth fixing.
• 7 dead assertNotEquals in OidcDeviceAuthPersistenceTest (:642,669,724,748,779,804,831) — each under an assertEquals that already pins the value.
• DirectUtf8SinkTest allocates off-heap but lacks assertMemoryLeak (its AbstractTest.tearDown() only logs).
• testTruncatedJsonReturnsNull is trivial-pass (fingerprint check returns null before the truncation guard runs); testConcurrentStealContentionDegradesCleanly never actually contends (all threads serialize on PROCESS_LOCKS).
• Minor: DisplaySafe mirror-pair redundant assertions; absence-only assertions in SenderBuilderErrorApiTest; WebSocketTokenProviderTest:644 Thread.sleep(50) write-before-close sync; magic numbers ≥5 digits without _ (QwpUdpSenderTest, SEGMENT_SIZE_BYTES = 16384L).

7. Metadata & misc.

• Title lacks a leading verb: feat(core): OIDC sign-in via device flow (RFC 8628) → feat(core): add OIDC sign-in via device flow (RFC 8628) (the description is copied to release notes and must read on its own). Commit 86d65ffb's title carries a feat(core): CC prefix commit titles should not (moot after squash).
• OIDCAuthExample.java:69-70 prints the raw server message to System.err — modeling the exact unsanitized-display anti-pattern the PR defends against.
• OidcAuthException.getOauthError() returns the raw, unsanitized error (only the message is sanitized).
• Boolean fields without is/has (interactiveSignInInProgress, the groupsInToken field); firstDynamicCredentialAuthFailureNanos's 0L sentinel (benign — errs toward more retrying).

---

Downgraded (verified false positives / non-defects)

• M-3 (recv-timeout → duplicate rows) — the throw-during-2xx-drain → retry → resend structure is pre-existing (the old no-arg recv() already threw on its default timeout; the PR only changes which timeout the success drain uses), it is unreachable against a real QuestDB server (ILP success is 204 No Content, never chunked, so consumeChunkedResponse early-returns), and it is within ILP's inherent at-least-once contract and disclosed under Tradeoffs. Not a defect this PR introduces.
• testOversizedFileReturnsNull / testOversizedStaleLockIsStolen — not trivial-pass; both guards are load-bearing (proven by the save+load-then-oversize setup and the lock-steal decline).
• QwpQueryClientTokenProviderTest "missing assertMemoryLeak" — all 12 tests wrap it.
• BrowserLauncherTest "no assertion" — has assertNull(invokeSafeHttpUri(...)).
• inLock "swallowed spawned-thread assertion" — asserting tests run inLock on the main thread; spawned-thread tests capture via AtomicReference and re-assert after join().
• DeviceCodePrompt plain put() — safe; challenge fields are pre-sanitized at construction (OidcDeviceAuth.java:1512-1524).
• putAsPrintable→DisplaySafe at ILP callers — byte-identical for the previously-escaped set, strictly more escaping otherwise; all callers are error renderers.

---

Summary

1 Moderate (M-5) — addressed in the PR description during this review — plus ~15 Minor; ~10 draft findings dropped as false positives / non-defects after source verification. Every confirmed finding is in-diff or in the PR body; 0 out-of-diff defects. The recv(int) ILP-flush tightening is disclosed and correctly framed; the orphan-drainer liveness cluster is a data-safety-vs-worker-liveness tradeoff worth an explicit call. Cheap follow-ups worth doing: M-1 (complete the chunk-size guard + fix its false comment + 2 tests), M-4 (pin the attempt-cap clause + @Test(timeout)), the QwpQueryClient double-set guard, and a decision on the orphan-drainer "reach a human" holes. After eight prior review passes plus this one, no reachable data-loss, credential-leak, injection, crash, or corruption path survives.

_{🤖 Generated with Claude Code}