Repository navigation
feat(qwp): add refreshing token providers for Entra ID authentication - #102
Open
bluestreak01 wants to merge 43 commits into
Open
bluestreak01 wants to merge 43 commits into
bluestreak01 wants to merge 43 commits into
Conversation
Add a cross-language specification for QWP clients that authenticate with rotating bearer tokens (Microsoft Entra ID managed identities and service principals), plus the Java findings and implementation plan. - design/qwp-token-provider-spec.md (v0.3, decisions resolved): the token-source contract, a proactively refreshing shared token cache, when clients fetch tokens, failure policy by phase (one retry after 401), connect-string keys (token_provider, azure_resource, azure_client_id), connection health, an optional authentication- outage deadline, redaction rules and conformance tests C1-C23. - design/entra-id-qwp-auth.md: findings for the Java client with code references, the design rationale, and the four-step Java plan (§10). Security-relevant server findings are reported privately and are not part of this change.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Implement the dynamic-credential specification (design/qwp-token-provider-spec.md, v0.3) and the four-step Java plan in design/entra-id-qwp-auth.md, section 10. - Token cache: RefreshingTokenProvider, ExpiringToken, TokenSource and TokenUnavailableException. Proactive jittered refresh at about half the token lifetime, a 60 s hand-out floor, single-flight cold waits bounded by cold_wait, jittered backoff honouring Retry-After, rate-limited forced refresh, and token redaction everywhere. - Client integration: one immediate same-endpoint retry after a refreshable 401 (WWW-Authenticate aware; never for a 403 or a static credential) on every ingest connect path, orphan drains included, and on egress connect and failover. SYNC startup retries a retryable provider failure within its budget (D6); any other provider exception still fails fast (D8). Egress errors name the failure class, and a query client whose failover reconnect failed reconnects on its next execute() instead of staying unusable (the suspected dead pooled worker, reproduced by a test before the fix). - Connect string: token_provider, azure_resource and azure_client_id on both clients (wss:: only; exclusive with static credentials and application-supplied providers), resolved through a ServiceLoader SPI and a process-wide ref-counted registry with a 60 s linger. Validation never fetches a token. - New optional module azure/ (org.questdb:questdb-client-azure): token_provider=azure on DefaultAzureCredential with the spec's error classification. Java 8 floor, released together with the client. - Connection health: Sender.health(), QwpQueryClient.health() and an aggregate QuestDB.health(); an optional auth_failure_max_duration_millis deadline for authentication outages. - Tests for conformance scenarios C1-C23, plus a TLS mode for the test WebSocket server (its key is generated at test time). README and design docs updated; the spec's Appendix B now names the published artifact org.questdb:questdb-client-azure.
bluestreak01
force-pushed
the
feat/qwp-entra-token-provider
branch
from
October 2, 2026 10:15
576383b to
c9f2b96
Compare
The spec classified Azure Identity's "no credential available in the chain" as permanent (section 7.5), while section 4 calls network failures and IMDS 404/410 retryable. Inside DefaultAzureCredential an unreachable IMDS produces exactly that message: the chain probes IMDS once, with a short timeout and no retries, so a transient IMDS outage failed a SYNC startup fast. Spec v0.4 follows Azure Identity's own split between fail-fast discovery and a resilient single credential: - 7.1, 7.2: a new key, azure_credential (default, managed_identity, workload_identity, environment), with its validation rules. - 7.5: library errors are classified by how the credential was selected. With one credential, configuration errors are permanent and endpoint or network failures retryable. In the discovery chain, "no credential available" is retryable and the client warns once. Errors should carry the library's innermost reason. - 4: a source that discovers its credential by probing must not treat "nothing found" as permanent on that evidence alone. - 10: conformance test C24. 12: decision D10, which records why an SMBIOS host check was rejected. Appendix B: the Java binding. Appendix D: precedents from Azure Identity, MongoDB, Google, AWS and Apache Druid. The Java implementation still follows v0.3.
Implements spec v0.4's azure_credential and aligns the spec with the code, so the two no longer disagree. - azure_credential=default|managed_identity|workload_identity|environment is accepted by the Sender, QwpQueryClient and the QuestDB facade, and validated per spec 7.2: an unknown value lists the supported ones, and azure_client_id cannot be combined with environment. default is normalized away, so existing connect strings take the same code path and share providers as before. - The other values build that credential directly. Passing AZURE_TOKEN_CREDENTIALS to DefaultAzureCredentialBuilder.configuration(), as Appendix B prescribed, does not work in azure-identity 1.18: it selects the credential but keeps the chain's one-shot IMDS probe, because each credential re-reads the selector from the process-wide configuration. A managed identity built directly retries an unreachable endpoint and rides out an outage. - A managed identity used alone reports "identity not assigned" only in a nested MSAL message. AzureTokenSource now classifies it as permanent; otherwise a sync startup retries that misconfiguration for its whole connect budget (measured: 316 s instead of ~25 s). - Retryable errors lead with their socket-level cause, such as a refused connection, which managed identity otherwise hides behind "see inner exception". Only java.net socket messages are echoed. - The discovery chain's "no credential available" stays permanent, so a host without a credential still fails fast. Instead, the provider warns once when DefaultAzureCredential starts as a chain, recommending azure_credential in production. - A workload identity missing its platform configuration no longer fails inside a client's build(): every fetch fails as permanent. The spec is amended to match (sections 4, 7.5, C24, D10, Appendix B and D), and entra-id-qwp-auth.md now points at v0.4. Tests: AzureManagedIdentityLibraryTest runs the real library in a child JVM against a local IMDS stub (MSAL reads the endpoint only from the process environment) for C24: an outage ridden out, an unreachable endpoint retryable, an unassigned identity permanent, the chain failing fast and warning once, and the platform selector silencing the warning. Plus AzureCredentialSelectionTest, new AzureTokenSourceTest cases, and TokenProviderConfigTest and config-honored coverage for the new key.
Brings in #104 (pool-wide sf_max_total_bytes budget and graceful sender close). Conflict in Sender.java: this branch moved the WebSocket build() branch into buildWebSocket(HttpTokenProvider), while #104 edited that block in place. Resolved by keeping the move and applying #104's edits to buildWebSocket: resolveSfMaxSegmentBytes()/resolveSfMaxTotalBytes(), newCursorEngine(..., sfSharedBudget, ...) for both engine constructions, and the sfSharedBudget argument on both quarantineTornSlot() calls. A token_provider sender built by the pool now charges the pool's shared segment budget like any other pooled sender.
QueryWorker.shutdown() closed the query client once its bounded 5 s join expired, even with the dispatch thread still inside execute(). A reconnect walk parked in a native connect or WebSocket upgrade does not see the shutdown interrupt, so it outlived the close: it went on to a healthy endpoint, then wrote the binds into the bind buffer close() had freed (SIGSEGV), or published an I/O thread and connection on the closed client (leak). The failover reconnect could already reach this; the reconnect on the next execute() after a failed failover made it reachable from every query on such a client. When the join expires with the dispatch thread still busy, the thread now closes its own client on its way out of runLoop(). One CAS decides which side closes, so the client is closed exactly once and never under a running execute(). Shutdown latency is unchanged and the per-query path is untouched.
executeImpl's failover-budget-exhausted exit returned without telling the connection health tracker, unlike its sibling exits. A query whose connection died on the last attempt the budget allowed, or after failover_max_duration_ms ran out, left QwpQueryClient.health() - and the QuestDB.health() aggregate - reporting CONNECTED until the next execute(). With failover_max_attempts=1 every later query fails on the same latched failure without reconnecting, so the client reported CONNECTED for its whole life while each query failed. That exit now records the loss with connectionLost(), as the other exits do: RECONNECTING, since the next execute() fails over. With a single attempt per execute() it also calls failed(): the client never reconnects, as with failover=off, so FAILED is the true state. Health is read only through health(), so query behaviour is unchanged, and the success path never reaches this code. Tests: ConnectionHealthTest covers a connection lost on the last allowed attempt (RECONNECTING, then CONNECTED on the next query) and with a single attempt (FAILED, and the next query fails without reconnecting).
The auth_failure_max_duration_millis javadoc and the ERROR log the deadline emits promised that unacknowledged rows stay in on-disk store-and-forward for a later sender or an orphan drain. That only holds with sf_dir: the option is accepted without one, memory-mode segments are malloc'd, orphan drains require sf_dir, and spec section 8.5 says memory-only data is lost when the sender closes. Document both modes in the javadoc, and make the log line name the outcome for the sender's actual backing (engine.sfDir() is null only in memory mode).
Contributor
|
Tandem: questdb/questdb#7799 (submodule bump to |
Bring the symbol-dictionary recycle API into the Entra token\nprovider branch so the OSS test suite compiles against the pinned\nclient. Preserve both the token-provider handshake and recycle setup\nin the sender builder, and carry lifetime counters into the loop.\n\nThe client and OSS test compiles pass with JDK 25; targeted client\nand OSS integration tests exercise both features together.
authOutageDeadlineFired() built its TERMINAL SenderError span from the engine's raw FSNs, while every other SenderError the loop emits adds externalFsnBase. After a symbol-dictionary recycle, which is on by default, the rebuilt engine numbers its frames from 0 again, so the span named FSNs that flushAndGetSequence() had already returned for acknowledged rows and left out the rows still waiting. The span now adds externalFsnBase like its siblings. ConnectionHealthTest gains a test that recycles the dictionary, then rejects every handshake with 401 past a short deadline, and checks the terminal span against the FSNs the sender reported. It fails without the fix (fromFsn 0 instead of 1), and also fails if the rebuilt loop is not armed with the deadline. QwpQueryClientDynamicCredentialTest gains three tests for paths that no test pinned: - connect() and a failover reconnect retry a refreshable 401 only once, even when the provider hands out a new token on every pull. Without the limit, both walks kept retrying for as long as the server rejected. - When the reconnect that the next execute() makes after a failed failover fails as well, the operation reports "reconnect failed:", and a later execute() still reconnects instead of throwing "not connected".
glasstiger
changed the title
The SYNC budget-exhaustion test in WebSocketDynamicCredentialTest asserted more than two token pulls within a 300 ms connect budget, with 10-40 ms backoff parks between pulls. On the mac-aarch64 CI runner a timed park wakes about 100 ms late: the sibling test with the same backoff spent 196-504 ms in three parks across five mac runs, against 67-72 ms on linux and windows. The budget then fit only two pulls, and the assertion failed. Running the test JVM under background QoS (taskpolicy -b) reproduces the overshoot locally: a 10 ms park takes ~105 ms, the 300 ms budget yields exactly three pulls in every run, and added CPU load drops it to one or two. A 2 s budget yields 15-17 pulls under the same load. The test now gives the connect loop 2 s. Its assertions are unchanged and the client code is untouched: the loop honours the budget and retries; only the test assumed timely wake-ups.
TestWebSocketServer counted a validator rejection only after it had flushed the 401, so a test that reads authRejectCount() as soon as the client has handled the rejection could see the count one short. That is how testPersistent401AfterTheRetryFailsStartup failed on the JDK 8 CI job (expected 2, was 1). The server now counts before it writes the response. With a 50 ms lag added after every reject, all 40 tests in the three classes that read the counter pass; without the reorder the same lag fails that test every time. testDeadlineIsNotResetOrFiredByOtherFailureClasses said that role rejects and transport failures do not reset the auth-outage clock, but it only waited for some terminal, so a loop that reset the clock in those branches still passed it. The test now parses the elapsed time from the terminal and requires it to span the role-reject and transport rounds. It fails against such a reset and passes without it. Comments and javadoc still said that an eager initial connect fails fast with the provider's own exception and that a running sender never latches a terminal on a credential outage. They now describe the SYNC retry of a retryable provider failure, the LineSenderException that reports an exhausted budget, the extra token pull after a 401 and the opt-in auth_failure_max_duration_millis deadline. The merge of main had also brought back, at the build() callsite, the recovery comment that #91 moved into constructEngineOnSlotLocked, and had dropped two #91 comments; main's text is restored there.
Contributor
Review at
|
glasstiger
marked this pull request as ready for review
October 14, 2026 13:42
ConnectionHealth and QwpConnectionHealthTracker declared lastFailure before lastConnectedAt(EpochMillis), and ConnectionHealth listed getLastFailure() before getLastConnectedAtEpochMillis(). This commit puts the fields and getters in alphabetical order, as the member sorting convention requires. WsSenderConfigHonoredTest now writes the expected auth_failure_max_duration_millis value as 600_000L, using the thousands separator for numbers of five or more digits. No behaviour changes. ConnectionHealthTest and WsSenderConfigHonoredTest pass.
Four fixes from the level-3 review of this branch. README: the auth_failure_max_duration_millis note promised that unacknowledged rows stay on disk when the deadline fires. That holds only with sf_dir. A memory-only sender, the default, loses them when it closes, as the javadoc, the deadline's log line and spec section 8.5 already say. The README now names both outcomes. RefreshingTokenProvider: a fetch whose token expired within handout_floor (60 s) of its receipt counted as a success, although the provider can never hand such a token out. A source that issues tokens that short, such as a Keycloak realm at its 1-minute minimum access-token lifespan, left every caller waiting out cold_wait for a "no usable token" error with no cause, no WARN, and a provider that reported no failures. checkResult() now treats such a result as a retryable failed fetch whose message names handout_floor, so the WARN log, getLastFailure() and the caller's error all carry it. Spec v0.5 amends section 4 and conformance rows C2 and C6 to match, and the README documents the floor and handoutFloorMillis(). azure/pom.xml: surefire ignored -Dtest.include and -Dtest.exclude, so a questdb server build with the local-client profile ran all 26 Azure tests in every CI shard: about 35 s in each of the 15 Linux shards of OSS build 279062. The module now maps both properties the way core does. Its defaults still run every test. RefreshingTokenProviderTest: the C8 step for a stale token ran within forced_min_interval, so the rate limit returned first and the test passed with the stale-token check deleted. The step now runs once the interval has passed and asserts that no fetch is scheduled. Verification: deleting the stale-token check now fails C8. Reverting the handout_floor check fails the new hand-out-floor test and the two updated C2 and C6 tests. With -Dtest.include='**/griffin/**' the azure module runs no tests (26 before), and its default selection still runs all 26.
RefreshingTokenProviderTest failed on the JDK 8 CI job with a
NullPointerException in the condition of the await() at line 298:
await(() -> scheduler.peek() != null
&& scheduler.peek().delayNanos == 0, ...)
The condition reads ManualScheduler.peek() twice while another thread
makes the provider replace its scheduled task. scheduleFetchLocked()
cancels the pending refresh before it schedules the forced fetch, and
peek() returns live tasks only, so between those two steps the
scheduler holds none: the first peek() saw the old task and the
second returned null.
ManualScheduler.isNextImmediate() now reads the task list once. The
four awaits that poll while a rejecter or cold-caller thread runs,
two in the C8 forced-refresh test and one in each hand-out-floor
test, use it. Every other peek() dereference runs after the
background call has been joined, or on the test thread alone.
With a 100 ms pause added between cancel() and schedule() in the
provider and a 20 ms pause in peek(), the old condition failed 3 of 3
runs with the CI stack trace, and the new one passed 3 of 3. Without
the pauses, RefreshingTokenProviderTest passes 5 of 5 runs.
The race dates from c9f2b96; 2c4a79b did not change these lines.
LineSenderBuilder.build() takes a TokenProviderRegistry lease before it connects a token_provider sender, and its finally block closes the lease when buildWebSocket() throws. No test covered that path: with the lease.close() call deleted, the core and azure suites still passed, while the failed build left the lease held, so the shared provider's refresher thread would keep fetching tokens for the life of the process. TokenProviderSharingTest gains testSenderBuildFailureReleasesTheLease. It builds a token_provider sender against a port nothing listens on, expects the connect failure, and checks that build() created the provider, that the lease count is back to zero, and that the provider lingers for the next attempt. With the lease.close() call deleted the test fails with "expected:<0> but was:<1>". WsSenderConfigHonoredTest now lists its TestTokenProviderFactory import after io.questdb.client.impl.Side, in alphabetical order.
The opt-in auth_failure_max_duration_millis let a running store-and-forward sender become terminal once credential-unavailable or 401/403 rounds had lasted that long. Its clock started at the first such failure and kept running through role-reject and transport rounds, so a failover between two brief credential failures could end a producer whose authentication never failed for long. A store-and-forward sender must never become terminal on a transient outage, so the deadline is removed. An established sender again retries every authentication-class failure indefinitely, and reports each one to the error handler and in its connection health. An application that prefers a hard failure closes the sender itself. The connect-string key, the builder method and the send loop's outage clock are gone, and the spec moves to v0.6: section 8.5, D7 and C22 now rule such a deadline out. ConnectionHealthTest replaces the deadline tests with testAuthenticationOutageNeverMakesTheSenderTerminal. It drives credential-unavailable, 401 and role-reject rounds, checks that the sender stays alive, and checks that it delivers the buffered row after recovery. With a terminal latched on a credential failure in the send loop, it fails with "expected:<RECONNECTING> but was:<FAILED>".
A sender whose I/O loop latched a terminal error while it was connected, such as a batch the server rejects with PARSE_ERROR, reported FAILED with outageSince NONE. CursorWebSocketSendLoop.recordFatal() reports only failed(), and failed() never set the outage start that upgraded() had cleared. ConnectionHealth documents NONE as "while connected", and the aggregate's oldest outage as NONE only when every connection is connected, so QuestDB.health() left such senders out of its oldest outage. A query client already calls connectionLost() before failed(). QwpConnectionHealthTracker.failed() now starts the outage when none is running. A client that was already without a connection keeps the start of the outage it is in. ConnectionHealthTest gains two tests for paths that no test pinned: - testSenderHealthIsFailedWithAnOutageAfterATerminalRejection: a server that rejects every batch with PARSE_ERROR moves the sender to FAILED with an outage start. It fails without the fix (outageSince -1), and with "expected:<FAILED> but was:<CONNECTED>" if recordFatal() stops reporting failed(). - testQueryClientHealthIsFailedWhenFailoverIsOffAndItLosesItsConnection: with failover=off a dropped connection leaves the query client FAILED, and the next query fails without reconnecting. It fails with "expected:<FAILED> but was:<RECONNECTING>" if the failover-off branch of executeImpl() stops reporting failed(). The new query test and the failover_max_attempts=1 test now share one helper, assertQueryClientFailsForGood().
glasstiger
added a commit
to questdb/documentation
that referenced
this pull request
Oct 14, 2026
… renewal and Java token providers Align the OIDC, RBAC and client pages with questdb/questdb-enterprise#1267, questdb/questdb#7799, questdb/ui#603 and questdb/java-questdb-client#102. - Issuer validation (QuestDB Enterprise 4.0.2): acl.oidc.issuer and acl.oidc.issuer.validation.enabled, a new "The expected issuer" section with where the issuer comes from, the issuers of Entra ID v1.0 and v2.0 tokens, the common/organizations templates, issuers that depend on the host name of the request, the opt-out and its advisory, the InvalidIssuer and MissingRequiredClaim("iss") log lines, and the upgrade notes. acl.oidc.host needs acl.oidc.issuer unless validation is disabled. PGWire tokens need the expected issuer too. - HTTP sessions: a request with a credential that QuestDB rejects, or cannot check because the provider is unreachable, gets 401 and ends its session. Sessions end with the password or REST token that opened them, or when the user is disabled or dropped, and on a replica when they are next used. acl.oidc.cache.ttl reuses only the check of a password that QuestDB verifies itself, even with OIDC disabled. - Provider outages: with the User Info endpoint, Web Console users are logged out once acl.oidc.cache.ttl has passed; with tokens validated locally, once the ID token expires; ROPC logins need the token endpoint in both modes. - Web Console: it renews the ID token by its exp claim, keeps its token while it cannot reach the provider, and without a refresh token logs the user out when the token of the login expires. Remove the stale "logged out about an hour after the login" text from the OIDC page and the Entra ID guide; Entra ID limits the refresh tokens of single-page applications to 24 hours. Its password login sends a REST token that survives a password change, so drop the user's REST tokens to log the user out. - Java client: "Rotating tokens" with token_provider=azure, the questdb-client-azure artifact, RefreshingTokenProvider and how token failures are handled, "Connection health", the token provider keys in the connect string reference, and a Java example in the Entra ID guide. - With the User Info endpoint, QuestDB treats the access token as opaque. - Changelog entries for the above. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Preserve Retry-After headers from MSAL service exceptions, including\nwrapped failures. Service-principal credentials surface throttling through\nMSAL rather than HttpResponseException, so the adapter previously lost\nthe final response's delay after the SDK exhausted its retries. Share delta-seconds parsing between Azure HTTP and MSAL response headers\nwithout changing permanent-error classification or retaining SDK causes. Add regression tests through the real ClientSecretCredential and its\ndefault retry policy for throttling, server errors, wrapped exceptions,\ncase-insensitive headers, and missing or malformed hints. Verify MSAL\nfailures without response headers still use the provider's backoff. Validation: all 30 Azure tests and 35 refreshing-provider tests pass on\nnative arm64 JDK 8 and JDK 25. The new throttling tests fail before the\nproduction fix and pass with it.
Replay a canceled scheduled callback before its forced replacement using the existing fake clock and manual scheduler. Assert that the callback neither fetches a token nor replaces the pending task, and that the forced refresh publishes the new token exactly once. All 36 provider tests pass on JDK 25 and on JDK 8 with -Xint. Removing only the scheduled-generation guard makes the new test fail on an unexpected extra fetch while the other 35 tests still pass.
Restore RECONNECTING health when a replacement connection loses its\ntransport during dictionary catch-up. Publish the loss before backoff\nor another credential pull so health polling cannot report CONNECTED\nwhile the sender waits to reconnect. Keep the retry policy unchanged.\n\nAdd a real-socket regression test that populates the symbol dictionary\nthrough the sender API, fails the reconnect catch-up send, and holds\nthe following credential pull. Check the outage state, buffered delivery,\nand recovery. The test fails before the production fix.\n\nValidation: 178 targeted Maven tests pass on native ARM64 JDK 25,\nincluding health, cursor send-loop, credential cancellation, dynamic\ncredential, and JVM-error cleanup tests. git diff --check also passes.
Package Java 11 module descriptors in both Java 8 release jars so service binding discovers the Azure provider and resolves its SDK dependencies without extra module roots. Keep root classes compatible with Java 8 and inspect the versioned descriptor in the CI smoke job. Stop forwarding arbitrary SDK exception text into token-source errors and cached failures. Retain exception types, HTTP status, known Entra configuration codes, socket details, and retry hints without retaining SDK causes or token-endpoint response bodies. Add packaged-module integration tests for sender and query configuration and real-SDK guards for malformed responses and OAuth error text. Verify that removing either module metadata component fails the module smoke and that both diagnostic guards fail against the original source. Validate targeted Maven verify and Javadoc builds on native arm64 JDK 8 and JDK 25. Execute the JDK 8 jars on JDK 11 and JDK 25 and pass 57 additional configuration, dynamic-auth, health, and shutdown tests.
QwpWebSocketSender now reports connection loss when it retires the\noutgoing I/O loop. A failed slot cleanup or engine rebuild therefore\nleaves health in RECONNECTING with an outage timestamp instead of\nreporting CONNECTED after the transport has closed.\n\nExtend the existing rebuild and filesystem-cleanup tests to check\nconnection health during failure and after recovery. Repeated rebuild\nfailures retain the same outage start and last-connected timestamp.\n\nBoth regression tests fail without the production change. All 107\ndictionary-recycle and connection-health tests pass on native arm64\nJDK 25 and on Java 8 with -Xint, which avoids the local C1 crash\nreproduced on both the PR head and base during review.
Make the test server accept upgrades before restoring the token provider.\nThis prevents a reconnect between the recovery writes from replacing\nthe retained credential failure with an authentication rejection.\n\nAll 11 ConnectionHealthTest tests pass on native arm64 JDK 25.
This was referenced Oct 15, 2026
Reuse the token-provider specification returned by shared config validation instead of resolving it again in QwpQueryClient.fromConfig. Keep the public standalone validator and validation order unchanged. Count factory validations in the test fixture and cover one resolution per client, normalized parameters, and setup without token acquisition. Validation: - Confirmed the regression test fails before the production change. - Passed 165 focused tests on JDK 25 and Java 8 (-Xint).
Classify MSAL HTTP 400 and 401 responses as permanent credential failures even when their AADSTS code is outside the allowlist. Guard the nullable status and preserve known-code diagnostics and retry hints without retaining SDK response bodies or causes. Cover direct and wrapped rejections through the pinned real Azure SDK. Both new regression tests fail against the previous classifier. Wait for the replay frame counter before asserting its exact value, rather than relying on the earlier payload publication. A forced pause reproduces the old assertion failure and passes with the new wait. Validation: - Pass 136 focused tests on native arm64 JDK 25. - Pass 136 focused tests and four packaging checks on JDK 8 with C1 disabled; packaged jars run on JDK 11 and JDK 25. - Reproduce the normal JDK 8 C1 crash in Unsafe.recordMemAlloc on untouched dc9cf22, without either fix.
Contributor
PR #102 — level 3 reviewVerdict: approve. Reviewed FindingsNo Critical, Moderate, or Minor findings. No admitted in-diff or out-of-diff-breakage issues.
ValidationRan sequentially on native arm64 JDK 25:
Exact-head CI is also green, including JDK 8 build/test/Javadoc, modern-JDK compilation, MRJAR smoke tests, and Linux/macOS/Windows jobs. Coverage and limitationsThe tests exercise bounded same-endpoint retries after refreshable 401s, startup failure classification, established-sender retry/recovery, failover and orphan delivery, cancellation during credential callbacks, provider sharing, and deferred query-client cleanup. OSS #7799 and Enterprise #1267 exist, use the matching branch, and cross-link this PR. Neither adds a combined real-server token-provider/rotation test. That remains a validation limitation, not a blocking gap: credential encoding and the established protocol remain unchanged, the server’s rejection forms match the loopback cases, and existing server tests cover bearer authentication separately. Live Entra tenant behavior remains unvalidated. Tradeoffs
The review made no source changes. |
Treat nested NoSuchFileException from Azure Identity as a permanent credential configuration failure. Keep the diagnostic free of SDK messages, file paths and retained SDK causes. Add a regression through the pinned WorkloadIdentityCredential for a configured federated token file that does not exist. Confirm that the test fails before the classifier change and passes with it. Validate 94 selected tests and four packaging checks on JDK 25 and JDK 8. Run the JDK 8 tests with -Xint to avoid the local C1 crash reproduced on both the branch and its merge base.
The released client jar is built on JDK 8 and used to carry no module descriptor, so on Java 11+ it loaded as an automatic module that required nothing. This branch compiles module-info.java into META-INF/versions/11 of that jar, which makes every mandatory requires in it binding for applications that load the client from the module path. No client code uses two of them, java.sql and jdk.management. A jlink image built from what jdeps reports for the client holds java.base, jdk.unsupported and java.desktop, so an application that runs the released client from the module path on such an image failed at startup with "FindException: Module java.sql not found, required by io.questdb.client", where the previous jar started. The descriptor drops both requires. DesktopFreeModulePathTest gains a test that resolves the client with the universe limited to java.base and jdk.unsupported and the client added as a root, whose requires --limit-modules does not close over. It failed with that FindException before the change. The CI MRJAR smoke job now also pins the mandatory requires of the released JDK 8 jar to java.base, jdk.unsupported and org.slf4j.
Members now follow the committed IDE arrangement. In RefreshingTokenProvider the ScheduledTask interface precedes Scheduler and the FetchFailure class follows DefaultScheduler. The test methods and helpers of QueryWorkerBusyShutdownTest and WebSocketDynamicCredentialTest are in name order. These are pure moves. The Entra ID design note still said the azure module has no module-info and relies on an automatic module. It ships a module-info that provides the factory and requires the Azure SDK modules, compiled into META-INF/versions/11 by JDK 8 release builds, and the note now says so.
Two branches of the one retry after a 401 (spec section 8.2) had no test. With either one broken, the full core suite still passed: - The endpoint is retried only with a token that differs from the rejected one. QwpWebSocketSender.connectWalk and the query client's connectWalk and reconnectViaTracker check this. Without the check, a provider that hands the same token back, as an Azure managed identity usually does, has the server reject it a second time in every connect round. - When the pull after the 401 fails, the round ends with the 401 and carries the provider's failure as a suppressed diagnostic. If that failure escaped instead, an OFF startup would throw the provider's own exception, a SYNC startup would retry it for its whole budget, and the query client would report credential-unavailable. WebSocketDynamicCredentialTest gains testUnchangedTokenAfterA401IsNotRetried and testFailedRefreshAfterA401EndsStartupWithTheRejection, each run in OFF and in SYNC mode. QwpQueryClientDynamicCredentialTest gains the connect and failover versions of the first and the connect version of the second. TokenTestKit gains ForgetfulTokenProvider, an application provider that drops its cached token once a server rejects it and then cannot reach its identity platform. Removing each check on its own fails exactly the test that pins it: the unchanged-token tests see "Bearer T1" presented twice, and the failed-refresh tests get TokenUnavailableException (sender) or QwpCredentialUnavailableException (query client) instead of QwpAuthFailedException. The new tests pass on JDK 25 and on JDK 8 (-Xint), and the ten affected test classes (109 tests) pass on JDK 25.
Restrict Azure client ID GUID validation to ASCII hexadecimal digits. Unicode digit and letter look-alikes must fail configuration validation instead of reaching the Azure credential builders. Add regression coverage for fullwidth digits, Arabic-Indic digits, and fullwidth uppercase and lowercase hex letters on both client entry points. The new test fails without the fix, and all 11 configuration tests pass with it on JDK 17. JDK 8 validation was unavailable locally.
Classify Azure Identity's certificate-loading failures as permanent. Recognize the SDK's specific RuntimeException prefix without retaining its message or cause. Keep more-specific missing-file diagnostics ahead of the parsing fallback and leave unrelated I/O failures retryable. Add real-SDK regression tests for malformed and missing certificates, plus synchronous startup through RefreshingTokenProvider. Reuse the core memory-leak checker through a test-only dependency. Add unit coverage for wrapped parsing failures, safe diagnostics and unrelated EOF errors. Validation: - The regression tests fail before the fix and pass after it. - JDK 25 passes 100 focused tests and two module-path integration tests. - JDK 8 passes all 39 Azure tests. The wider local Zulu 8 core run also hits SIGSEGV with unchanged core code at 471f060. All 61 focused core tests pass with -Xint.
Give query workers a sticky shutdown request that stops I/O and\nreconnects without freeing buffers while a handler still uses them.\nWake event consumers even when the I/O loop exits while idle, and\nleave resource cleanup to the dispatch thread after execution ends. Preserve typed endpoint failures as causes when connection walks\nexhaust their endpoints, so health reports HTTP upgrade rejections\nwith their original class and status instead of TRANSPORT and zero. Add regression coverage for consumed interrupts, native batch\nlifetimes, shutdown during reconnect, and upgrade-status reporting.\nReplace the null-client interrupt-test fixture with a real client. Validation: 378 query, pool, health, failover, and buffer-safety tests\npassed on each of JDK 11 and JDK 25. Mutation checks confirm the new\nregressions fail without the fixes. JDK 8 was unavailable locally.
Complete and consume a query before checking idle I/O shutdown. Require its shutdown event before pool cleanup, and retain the original startup state test under a more accurate name. Run both forced-refresh callbacks through FutureTask so bounded result checks propagate callback exceptions to JUnit instead of treating an exceptionally terminated thread as a successful return. Redirect keytool output to a temporary file before waiting for exit. Destroy and reap timed-out or interrupted children, preserve the caller's interrupt flag, and cover success, diagnostics, timeout, and interruption with real child-JVM tests. Validate 274 unit tests and four packaged-jar checks on native JDK 25. Run 52 affected tests on Java 8 in interpreted mode; the default local Java 8 VM also crashes in Unsafe.recordMemAlloc on unmodified HEAD. Mutation checks confirm shutdown, both callback, and child-cleanup regressions now fail their tests.
Contributor
PR #102: level 3 review at
|
The review of this branch at cfee59a flagged new boolean fields that lack the is/has prefix the coding standard asks for. This commit renames them: - RefreshingTokenProvider: backoffActive, closed, failureLogged, fetchRunning, forcedRefreshed and shortWindowWarned become isBackoffActive, isClosed, isFailureLogged, isFetchRunning, isForcedRefreshed and isShortWindowWarned. - AzureTokenSource: chain becomes isChain, and so does the matching classify() parameter. - QwpQueryClient: reconnectOnNextExecute becomes isReconnectOnNextExecute. The renamed fields move to their sorted positions. No behaviour changes. The core and azure modules compile on JDK 25; the nine affected core test classes (118 tests) and the azure module (39 tests) pass.
The azure module declared a test-scoped dependency on the core test jar, which a build with -Dmaven.test.skip=true never produces. Every such build of the client reactor failed to resolve questdb-client:jar:tests. That includes the questdb server's GitHub workflows that run "mvn clean install -Dmaven.test.skip=true -Plocal-client": PGWire stable and latest clients, the R test, Parquet export compatibility and the glibc smoke test. A local repository that still holds an older tests jar hides the failure. The dependency now lives in a core-test-jar profile that is active unless maven.test.skip is true. Such a build compiles no tests, so nothing needs the jar; every other build declares it as before. QwpQueryClient: health() and requestShutdown() move to their sorted places among the public methods, connectWalk() joins the private methods, and refreshAfterRejection() now precedes resolveAuthorizationHeader(). BearerChallenge.allowsTokenRefresh() now precedes bearerError(). These are pure moves. Verification, with the poms set to a version absent from ~/.m2: - mvn -o package -Dmaven.test.skip=true fails at 66009e1 on the azure module and passes with this change. - mvn -o clean package -DskipTests still builds the core test jar and compiles the azure tests against it. - The 39 azure tests and AzureModulePathIT pass, and so do 51 tests in six affected core test classes.
Track result batches that shutdown releases without invoking the handler. If the I/O thread reaches RESULT_END before it observes its own shutdown flag, report a shutdown error instead of success with missing rows. Keep buffer release and credit return intact, and allow complete results to finish successfully. Add four leak-checked regression tests that cover both outcomes and credit return. Use the Windows runner's existing MinGW toolchain instead of installing it through Chocolatey. Download pinned NASM 2.16.03 from its upstream release with bounded retries and SHA-256 verification. Check native command exit codes explicitly so PowerShell stops on download, tool, configuration, and build failures rather than continuing with stale artifacts. The failed Azure job received HTTP 503 from Chocolatey and continued until it could not find NASM. Rename the remaining reviewed boolean locals to use the is prefix. All 152 focused tests pass on JDK 17 and JDK 25, and on JDK 8 with tiered compilation disabled. The default JDK 8 C1 crash in Unsafe.recordMemAlloc also reproduces on the unmodified head. Both skipped-batch regression tests fail without the shutdown fix. YAML parsing, PowerShell parsing, and eight mocked-command Windows build scenarios pass; the actual Windows DLL build still needs validation on Azure.
glasstiger
changed the title
Contributor
[PR Coverage check]😍 pass : 1379 / 1507 (91.51%) file detail
|
Contributor
PR #102: level 3 review at
|
glasstiger
changed the title
Contributor
PR #102: level 3 review at
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tandem: questdb/questdb#7799, questdb/questdb-enterprise#1267
Refreshing credentials
Static tokens expire while clients reconnect. This PR adds a shared, expiry-aware token cache, classified fetch failures, connect-string provider selection, and the optional
questdb-client-azuremodule for Entra ID managed identities and service principals. A refreshable 401 triggers one same-endpoint retry only when the provider supplies a different token. Tokens already withinhandout_flooron arrival count as retryable fetch failures rather than unusable successes.Query recovery and shutdown
A failed failover reconnect left pooled query clients unusable. The next
execute()now reconnects instead of repeatedly reporting that the client is disconnected.A busy dispatch thread could outlive the shutdown join and then access a query client that the pool had already freed. The dispatch thread now owns the eventual close when it remains inside
execute(). Shutdown can also release result batches before the I/O thread observes its stop flag;executeOnce()now reports an error if it skipped any batch, rather than completing successfully with missing rows. Complete results can still finish successfully.Connection health
Query clients could continue reporting
CONNECTEDafter failover exhausted its budget, and terminal sender failures could lack an outage timestamp. The tracker now reports the appropriate failed or reconnecting state and retains the outage start. An established store-and-forward sender retries authentication-class failures indefinitely and reports them through its error handler and health snapshot; applications that require a terminal outcome must close it themselves.Azure deployment and builds
Automatic-module discovery could leave Azure SDK dependencies unresolved. The released core and adapter jars now include Java 11 module descriptors, with service use and provider declarations, while Java 8 retains its root classes. The Azure module also honours test include/exclude filters and avoids requiring the core test jar when
maven.test.skip=true.Azure SDK exceptions can contain token-endpoint response bodies. The adapter now reports failure types, existing HTTP status, recognised Entra configuration codes and retry hints without retaining arbitrary SDK messages or causes. It also rejects malformed certificate configuration before starting credential acquisition.
Windows native build
The Windows job depended on Chocolatey's package feed even though the runner already supplies MinGW. It now uses that toolchain and downloads pinned NASM directly with bounded retries and SHA-256 verification. Explicit native-command exit checks stop PowerShell after download, tool, configuration or build failures instead of allowing later commands to use stale output.
Compatibility and tradeoffs
getToken()call after a refreshable 401.forced_waitpluscold_wait(5 s and 30 s by default);close()interrupts the wait.execute()that never returns retains its query client and provider lease instead of allowing shutdown to free memory it still uses.JAVA11_HOME.auth-rejected:, and query-client provider failures useQwpCredentialUnavailableExceptionwithcredential-unavailable:.local-clientreactor resolves Azure SDK dependencies and builds the adapter; its tests follow the shard's include/exclude filters.Merge order
main.Design: cross-language token-provider specification and Java Entra ID integration.