Skip to content

feat(qwp): connect timeout, ingest callbacks, and write-only mode on the QuestDB facade#60

Open
bluestreak01 wants to merge 26 commits into
mainfrom
feat/connect-timeout
Open

feat(qwp): connect timeout, ingest callbacks, and write-only mode on the QuestDB facade#60
bluestreak01 wants to merge 26 commits into
mainfrom
feat/connect-timeout

Conversation

@bluestreak01

@bluestreak01 bluestreak01 commented Jun 28, 2026

Copy link
Copy Markdown
Member

tandem OSS PR: questdb/questdb#7341

Summary

Four related ergonomics/resilience improvements for the QWP (WebSocket) client:

  1. Application-level TCP connect timeout (transport-wide) — bound the connect itself instead of riding the OS-level timeout.
  2. Expose the ingest callbacks on the QuestDB facadeerrorHandler / connectionListener, previously unreachable from the pooled facade.
  3. Tolerant startup via lazy_connect=true — start the handle even when the server is down, buffer writes meanwhile, and read once it's up. Reads stay fully enabled.
  4. Single cluster config on the facade — one ws/wss string (a single addr server list) configures the whole cluster, driving both the ingest and query pools.

Items 1–3 are independently usable and off by default.


1. Configurable TCP connect timeout

A connect to a black-holed/firewalled host blocks on the OS-level TCP connect timeout (60–120s): the socket is created blocking, connect() runs, then it's switched to non-blocking. The code calls this out:

// SenderPool.java
// connect to a black-holed/firewalled host blocks on the OS connect timeout
// (the transport exposes no application-level connect timeout to clamp it).

Approach (native, cross-platform): non-blocking connect() (EINPROGRESS) → poll/select for writability bounded by the caller's budget → confirm via getsockopt(SO_ERROR). A sentinel (CONNECT_TIMEOUT = -3) lets Java raise a timeout-flagged exception. Generalises the existing handleEintrInConnect helper.

Sender.builder("https::addr=host:9000;connect_timeout=5000;")...   // or .connectTimeoutMillis(5000)
QwpQueryClient.fromConfig("ws::addr=host:9000;connect_timeout=5000;");

Touches: native share/net.c + windows/net.c + net.h; Net / NetworkFacade(Impl); HttpClientConfiguration.getConnectTimeout(); HttpClient.connect() / WebSocketClient.doConnect(); ConfigSchema COMMON key connect_timeout; Sender builder + both parsers; QwpWebSocketSender / QwpQueryClient (withConnectTimeout). Bounds only the TCP connect; TLS/upgrade stay under auth_timeout_ms.


2. Ingest callbacks on the QuestDB facade

The facade built ingest senders from config strings only (SenderPool → Sender.fromConfig), so the programmatic SenderErrorHandler / SenderConnectionListener were unreachable — a facade user got the default loud-not-silent handlers with no way to observe async ingest errors or connection transitions.

QuestDB.builder()
    .fromConfig("ws::addr=host:9000;")
    .errorHandler(myErrorHandler)
    .connectionListener(myConnectionListener)
    .build();

QuestDBImpl / SenderPool each gain a full constructor carrying the callbacks; the white-box test-seam constructors are preserved as delegating shims. SenderPool.applyUserCallbacks() applies them to every pooled sender (non-SF and SF paths); internal recovery delegates are excluded. Defaults null.


3. Tolerant startup: lazy_connect=true

The facade prewarms a reader (QueryClientPool) synchronously and fail-fast (default query_pool_min=1; queries have no async connect), so a down server failed the whole build. The fix is a single connect-string flag that makes the handle tolerate a down server without giving up reads — "starts when the server is down" and "never reads" are different things, and you almost always want the first.

lazy_connect=true:

  • a) starts even when the server is down — the ingest side goes async and the read pool defaults to query_pool_min=0, so neither side fail-fasts and build() returns promptly;
  • b) buffers writes while down — the async sender accepts rows that flush once the wire is up;
  • c) reads once the server is up — the read pool stays enabled and connects lazily on the first query (query()/newQuery() never throw).
// starts with no server present; reads work once it's up
try (QuestDB db = QuestDB.connect("ws::addr=host:9000;lazy_connect=true;")) {
    db.borrowSender().table("t").longColumn("v", 1).atNow(); // buffers while down
    db.executeSql("select 1", handler);                      // connects once up
}

Because both sides must start non-blocking, a knob that forces a blocking / fail-fast startup is a configuration conflict, rejected up front with a clear remedy rather than silently overridden:

  • initial_connect_retry other than async (i.e. off/false/on/true/sync), and
  • an explicit query_pool_min > 0 (connect string or builder call).

lazy_connect is a Side.POOL registry key — the two ws clients ignore it; the facade reads it, defaults query_pool_min to 0, and injects initial_connect_retry=async when the user set none.


4. Single cluster config on the facade

A QuestDB cluster is one logical target reached over QWP for both ingest and query, so the facade takes one cluster config: a single ws/wss string that lists every node in one addr server list and drives both the sender and query pools.

QuestDB.builder()
    .fromConfig("ws::addr=node1:9000,node2:9000,node3:9000;")  // whole cluster
    .errorHandler(myErrorHandler)
    .connectionListener(myConnectionListener)
    .build();

build() validates the one string with both the ingest (validateWsConfigString) and egress (QwpQueryClient.validateConfig) validators; each side applies the keys it owns and ignores the rest. Pool keys are read from a single ConfigView, and QuestDBImpl passes the one config to both pool slots (preserving the white-box reflection seam).


Testing

  • NetConnectTimeoutTest — loopback success, refused-vs-timeout disambiguation, black-hole timeout within budget.
  • QuestDBFacadeCallbacksTest — facade-wired errorHandler receives the async budget-exhaustion SenderError; connectionListener observes connection events (no server needed).
  • QuestDBLazyConnectTestlazy_connect=true starts + buffers a write with the server down, keeps query()/newQuery() enabled, and rejects both conflicts (blocking initial_connect_retry, explicit query_pool_min > 0) from the connect string and from builder calls; QuestDBServerRecoveryTest dogfoods lazy_connect=true for the full down → write → up → read lifecycle.
  • QuestDBServerRecoveryTest — full lifecycle: server down → facade starts → client writes (buffered) → server starts → write side reconnects and the reader connects on the first query.
  • QuestDBBuilderTest — covers the single cluster-config surface; the shared-vocabulary and sender-pool-unwind tests run against one server with one config.
  • TestWebSocketServer now serves both pools from one config like a real node: SERVER_INFO is emitted only on the egress /read path (the ingest /write ACK stream would choke on it), plus a setRejectReadUpgrade() toggle to fail only the query upgrade.

Full impl + network + facade suites pass locally on JDK 8 (source of truth) and the surface compiles on JDK 25 (java11+ front).

CI / native

  • ci(native): the rebuild_native_libs.yml linux-x86-64 job moved from manylinux2014 (glibc 2.17) to manylinux_2_28 (glibc 2.28), mirroring linux-aarch64 — GitHub now forces actions onto Node 24 (glibc ≥ 2.27), which couldn't run in the 2.17 container (pre-existing breakage, unrelated to the C change).
  • Native libraries are built on the test/release runners (no longer committed).

Compatibility

The connect_timeout knob and the ingest callbacks are additive and off by default. The QuestDB facade is new in this PR; the legacy direct Sender and QwpQueryClient APIs are unaffected.

Note: the PR bundles several independent features. Happy to split any of them into standalone PRs off main if that's easier to review.

bluestreak01 and others added 5 commits June 28, 2026 21:41
Establish a real, cross-platform connect timeout for the HTTP and
WebSocket (QWP) transports. Previously a connect to a black-holed or
firewalled host blocked on the OS-level TCP connect timeout (often
60-120s) because the socket was created blocking and only switched to
non-blocking *after* connect; the transport exposed no knob to clamp it.

Approach: a new native primitive switches the socket to non-blocking
*before* connect, so connect() returns EINPROGRESS immediately, then
polls for writability bounded by the caller's budget and confirms the
outcome via SO_ERROR. A distinct return code (CONNECT_TIMEOUT, -3) lets
the Java layer raise a timeout-flagged exception rather than decode errno.

Native:
- share/net.c: connectAddrInfoTimeout + awaitConnectComplete (poll +
  getsockopt(SO_ERROR), monotonic-clock EINTR handling)
- windows/net.c: Winsock equivalent (select write/except sets)
- share/net.h: ECONNTIMEOUT (-3) sentinel

Java:
- Net / NetworkFacade(Impl): connectAddrInfoTimeout + CONNECT_TIMEOUT
- HttpClientConfiguration.getConnectTimeout() (default 0 = OS fallback)
- HttpClient.connect() / WebSocketClient.doConnect() honor it and throw a
  timeout-flagged HttpClientException on CONNECT_TIMEOUT
- Sender builder: connectTimeoutMillis() + connect_timeout connect-string
  key (legacy http and ws/wss parsers) + ConfigSchema COMMON key
- QwpWebSocketSender / QwpQueryClient: thread the value through to their
  WebSocketClient (adds QwpQueryClient.withConnectTimeout)

Default is unset (0): behaviour is unchanged unless connect_timeout is
configured.

Tests: NetConnectTimeoutTest covers loopback success, refused-vs-timeout
disambiguation, and a black-hole timeout that fires within budget;
config-honored drift guards updated for the new COMMON key.
On a runner with no route to TEST-NET-1 (192.0.2.0/24) connect() fails
fast with ENETUNREACH instead of dropping the SYN, so the timeout path
can't be exercised. Skip (Assume) in that case rather than asserting a
timeout, while still proving the call never blocked on the OS connect
timeout.
GitHub now forces actions onto Node 24 (glibc >= 2.27), which cannot run
inside the manylinux2014 (glibc 2.17) container the linux-x86-64 native
build used; actions/checkout failed before compilation. The old
Node-20-glibc-217 override only patched /__e/node20, not /__e/node24.

Switch the job to quay.io/pypa/manylinux_2_28_x86_64 (glibc 2.28, runs
stock Node 24) and drop the Node hack, nasm src.rpm rebuild, and manual
CMake download, mirroring the linux-aarch64 job that already builds on
manylinux_2_28.
The pooled QuestDB facade built its ingest Senders from config strings
only (SenderPool -> Sender.fromConfig), so the programmatic ingest
callbacks -- SenderErrorHandler and SenderConnectionListener -- were
unreachable: a facade user got the default loud-not-silent handlers with
no way to observe async ingest errors or connection transitions.

Expose both as QuestDBBuilder setters and thread them to every pooled
Sender:
- QuestDBBuilder.errorHandler(...) / .connectionListener(...)
- QuestDBImpl gains a full constructor carrying the callbacks; the public
  constructor forwards them and the 12-arg white-box test-seam constructor
  is preserved as a delegating shim (null callbacks).
- SenderPool gains a full constructor + applyUserCallbacks() that applies
  the callbacks to every sender it builds (both the non-SF and SF paths);
  the 8-arg test-seam constructor is preserved as a shim.

Recovery delegates (internal, short-lived, OFF-mode drain senders) are
deliberately excluded so the user's callbacks never see events from
internal machinery.

Defaults are null -> behaviour is unchanged unless a callback is set.

Tests: QuestDBFacadeCallbacksTest prewarms one ingest sender at a dead
port in async mode with a tight reconnect budget and asserts the
facade-wired errorHandler receives the budget-exhaustion SenderError and
the facade-wired connectionListener observes connection events -- no
server required.
@bluestreak01 bluestreak01 changed the title feat(net): add application-level TCP connect timeout feat: TCP connect timeout + expose ingest callbacks on the QuestDB facade Jun 28, 2026
The QuestDB facade always built a reader (QueryClientPool), which prewarms
synchronously and fail-fast (default query_pool_min=1, QwpQueryClient has no
async connect). So a down server / read primary sank the whole facade build,
taking the write side with it.

Add QuestDBBuilder.writeOnly(): build an ingest-only handle that never
constructs the query pool, so the read side cannot fail startup. A query
config is no longer required in this mode (any query config set is ignored),
and query()/newQuery() throw a clear "write-only" IllegalStateException.

- QuestDBImpl gains a write-only public constructor + a writeOnly flag on the
  full constructor; the 12-arg white-box test-seam constructor stays unchanged
  (delegates with writeOnly=false). queryPool/queryThreadLocal are null in
  write-only mode.
- PoolHousekeeper tolerates a null query pool.
- QuestDBBuilder.buildWriteOnly() validates + resolves only the sender/shared
  pool knobs from the ingest config.

Pair with initial_connect_retry=async (or sender_pool_min=0) on the ingest
config so the write side does not fail-fast either -> the facade starts with
no server present.

Tests: QuestDBWriteOnlyTest proves the facade builds with no server, that
query()/newQuery() are disabled, that no query config is required, and that an
async warm sender can buffer a write while serverless.
@bluestreak01 bluestreak01 changed the title feat: TCP connect timeout + expose ingest callbacks on the QuestDB facade feat: connect timeout, ingest callbacks, and write-only mode on the QuestDB facade Jun 28, 2026
…nnects

End-to-end resilience test for the QuestDB facade: build with the server
down (ingest initial_connect_retry=async + query_pool_min=0), buffer a
write, then bring the server up and assert the write side reconnects and
the previously-deferred reader connects on the first query.

Uses two TestWebSocketServers bound-but-not-accepting to model a reachable
-but-down server (handshakeCount stays 0 until start()). The mock cannot
serve real SELECT rows, so the read step asserts the query client connects
once the server is up, not the row contents. Stable across repeated runs.
@bluestreak01 bluestreak01 changed the title feat: connect timeout, ingest callbacks, and write-only mode on the QuestDB facade feat(qwp): connect timeout, ingest callbacks, and write-only mode on the QuestDB facade Jun 28, 2026
bluestreak01 and others added 17 commits June 29, 2026 00:20
Remove the committed Linux/Windows native binaries (libquestdb.so,
libquestdb.dll) and compile them locally during the Azure test CI.

- New ci/build_native.yaml template compiles libquestdb on the runner:
  Linux (cmake+nasm+build-essential) and Windows (MinGW-w64+NASM via choco).
  macOS keeps using the committed .dylib. Inits the zstd submodule first.
- Output is copied into src/main/resources/.../bin/<platform>/ so mvn install
  packages it into the client jar for both client and OSS server tests; the
  loader also picks up the CMake bin-local output directly.
- Wired the template into run_tests_pipeline.yaml before client install.

Committed binaries are still produced by the release GitHub Action.
Remove the committed darwin-aarch64/darwin-x86-64 libquestdb.dylib and build
them on the macOS runners, matching the Linux/Windows approach. No native
binaries remain committed; all are compiled during the test CI.

- build_native.yaml: add a macOS build step (brew cmake/nasm,
  MACOSX_DEPLOYMENT_TARGET=13.0), detect darwin-aarch64 vs darwin-x86-64 via
  uname -m, and copy the dylib into src/main/resources/.../bin/<platform>/.
- Init the zstd submodule on all platforms (it was skipped on Darwin).

Release artifacts are still produced by the release GitHub Action.
The macos-15 (x64) agent hardware no longer exists, so remove the mac-x64
matrix entry. macOS is now tested on mac-aarch64 only. The darwin-x86-64
.dylib is still produced by the release GitHub Action, and build_native.yaml
keeps its uname-based arch detection so an x64 macOS runner would still build
correctly if ever reintroduced.
The GitHub Actions build-jdk8 job ran the full test suite against the
committed native libraries, which are now removed. Without the .so the
io.questdb.client.std.{Os,Files,Unsafe,...} static initializers fail with
NoClassDefFound (1289 errors).

Compile the native .so from source first (zstd submodule + cmake/nasm/
build-essential), against the JDK 8 JNI headers, and copy it into
src/main/resources/.../bin/linux-x86-64 so it survives 'mvn clean' and loads
via the production bin/<platform> path. Update the now-stale comment.
glibc 2.17 moved clock_gettime() into libc under a new GLIBC_2.17 version
node. Building the release .so in a modern container (manylinux_2_28) binds
clock_gettime@GLIBC_2.17, which raises the whole library's glibc floor to 2.17
and breaks loading on glibc 2.14-2.16 hosts.

Add src/main/c/share/glibc_compat.h with a .symver directive forcing the
reference back to clock_gettime@GLIBC_2.2.5 (x86-64 glibc only; no-op on
aarch64/macOS/Windows), include it from net.c and os.c, list it in the
CMake sources, and document the glibc floor in rebuild_native_libs.yml.
The Coverage Report job runs 'mvn -P jacoco test' on core but had no native
build step, so after dropping the committed binaries it failed to load
libquestdb.so (NoClassDefFound in io.questdb.client.std.*). Add the
build_native.yaml template before the coverage test run, matching the
BuildAndTest job. The job runs on Linux, so it compiles libquestdb.so.
Collapse the dual ingest/query config surface on the QuestDB facade into a
single configuration string for the whole cluster. A QuestDB cluster is one
logical target reached over QWP for both ingest and query, so one ws/wss
string -- listing every node in a single `addr` server list -- now drives
both the sender and query pools.

- QuestDBBuilder: drop ingestConfig()/queryConfig(); fromConfig() sets the
  one cluster config. Remove the cross-side pool-key conflict resolution
  (no two strings to reconcile) -- resolvePoolInt/Long read one ConfigView.
  build() validates the single string with both the ingest and egress
  validators; each side applies the keys it owns and ignores the rest.
- QuestDB: remove the connect(ingest, query) overload; connect(config) and
  builder() now document the one-config/server-list model.
- QuestDBImpl is unchanged: the builder passes the one config to both pool
  slots, preserving the white-box reflection seam.

Tests: TestWebSocketServer now serves both pools from one config like a real
node -- SERVER_INFO is emitted only on the egress /read path (the ingest
/write ACK stream would choke on it), plus a setRejectReadUpgrade() toggle to
fail just the query upgrade. Rewrote QuestDBBuilderTest and updated the
facade callback/recovery/write-only tests and the examples accordingly.
…n-string key

Make writeOnly() deliver its own promise and reach it from the connect string.

Previously "start even when the server is down" needed two knobs that look
unrelated: writeOnly() (skip the fail-fast read pool) plus
initial_connect_retry=async (keep the write prewarm from fail-fast-ing). The
former governs the read side, the latter the write side, so writeOnly() alone
still hard-failed build() when sender_pool_min >= 1 and the server was down.

- writeOnly() now defaults the ingest side to a non-blocking async initial
  connect (injected right after the schema so an explicit initial_connect_retry
  in the user's string still wins, last-write-wins). build() returns promptly
  with the server down and the sender pool warm; writes buffer until the wire
  comes up.
- New POOL-side connect-string key write_only=on, equivalent to .writeOnly(),
  so the mode is reachable from any config string (and QuestDB.connect). The
  two ws clients ignore it; the facade routes on it.

Tests: writeOnly() with sender_pool_min defaulting to 1 and no
initial_connect_retry now builds without fail-fast; write_only=on routes to
the ingest-only path via builder and via connect(). PoolConfigHonoredTest's
drift guard skips the routing flag (not a numeric sizing knob).
…nabled

Strengthen the server-recovery test to assert what the write-only mode is NOT:
on a normal facade built while the server is down (lazy read pool via
query_pool_min=0, async ingest), query() must still hand back a usable builder
*before* the server is up -- reads are enabled, just deferred -- and the
deferred reader connects on the first submit once the server comes up. This is
the read-capable counterpart to write-only, where query() throws for the life
of the handle.
…ant startup)

Drop write-only mode (it permanently disabled reads -- query()/newQuery()
threw for the life of the handle) in favour of a read-capable tolerant-startup
flag, lazy_connect, reachable from the connect string.

lazy_connect=true:
- a) starts even when the server is down -- the ingest side connects async and
     the read pool defaults to query_pool_min=0, so neither side fail-fasts;
- b) buffers writes while the server is down (async sender);
- c) reads once the server is up -- the read pool stays ENABLED and connects
     lazily on the first query.

Because both sides must start non-blocking, a knob that forces a blocking /
fail-fast startup is a configuration conflict, rejected up front with a clear
remedy:
- initial_connect_retry other than async (off/false/on/true/sync), and
- an explicit query_pool_min > 0 (connect string or builder call).

Changes:
- ConfigSchema: write_only -> lazy_connect (Side.POOL; both clients ignore it).
- ConfigView.getBool: accept true/false (and on/off).
- QuestDBBuilder: remove writeOnly()/buildWriteOnly(); build() resolves
  lazy_connect, validates the two conflicts, defaults query_pool_min to 0 and
  injects initial_connect_retry=async when unset.
- QuestDBImpl: remove the write-only constructor/flag and requireQueryEnabled;
  the query pool is always built (the white-box reflection seam is unchanged).
- Tests: QuestDBWriteOnlyTest -> QuestDBLazyConnectTest (start+write while down,
  reads stay enabled, both conflicts via string and builder, true/on parsing);
  QuestDBServerRecoveryTest now dogfoods lazy_connect=true for the full
  down->write->up->read lifecycle; PoolConfigHonoredTest drift guard skips the
  flag.
…e timeout

QwpQueryClient.runUpgradeWithTimeout wrapped connect() and upgrade() in one
try block, so a connect_timeout overage -- the timeout-flagged
HttpClientException from doConnect()'s CONNECT_TIMEOUT path -- was caught by
the isTimeout() branch meant for upgrade() and rewritten as
"WebSocket upgrade to host:port exceeded auth_timeout=<authTimeoutMs>ms".
A user with connect_timeout=500 and auth_timeout_ms=15000 saw, after ~500ms,
an error blaming a 15000ms auth timeout (wrong phase and wrong value).

Move connect() outside the upgrade try so the auth_timeout rewrite only
applies to genuine upgrade-phase timeouts; connect-phase failures propagate
with their own "connect timed out ..." message. The failover walk is
unchanged (the exception is still a transport error and the next endpoint is
tried). The ingest side (QwpWebSocketSender) was already correct -- it routes
through QwpUpgradeFailures.classify, which leaves the connect-timeout
exception unmodified.

Add QwpQueryClientConnectTimeoutTest: a TEST-NET-1 blackhole connect with
connect_timeout < auth_timeout must report connect_timeout, not auth_timeout.
It skips gracefully when the runner has no route to the blackhole, mirroring
NetConnectTimeoutTest. Verified it fails on the pre-fix code with the exact
misreported message.

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

The PoolHousekeeper reap loop wrapped queryPool.reapIdle() in an
`if (queryPool != null)` guard whose comment ("null for a write-only
handle") described write-only mode. That mode was removed in the
lazy_connect change (7491d95): QuestDBImpl now builds the query pool
unconditionally and is the sole PoolHousekeeper caller, so the field is
never null in a live handle. The null branch is unreachable and the
comment is stale -- drop both. The outer best-effort Throwable catch
stays; it has nothing to do with write-only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ConfigView.getBool accepts true/false and on/off, but its invalid-value
error read "(expected true, false)", under-reporting the accepted forms
(e.g. lazy_connect=on is valid yet the message implies otherwise). List
all four, matching getBoolOnOff's convention of naming exactly what it
accepts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reshape the QuestDB facade so reads and writes share one pooled-lease
model, and remove the thread-affine footguns on both sides.

Ingest:
- Remove QuestDB.sender() and releaseSender(), along with the entire
  thread-pin subsystem behind them (SenderPool.pinToCurrentThread,
  releaseCurrentThread, clearPinIfCurrent, the threadAffine ThreadLocal,
  and the PooledSender invalidated flag that existed only to make pinning
  safe). borrowSender() is now the only way to lease a Sender.

Egress:
- Add QuestDB.borrowQuery(), a closeable, non-allocating Query lease that
  mirrors borrowSender(). Each pooled QueryWorker owns one pre-allocated
  QueryImpl, handed out reset on borrow; submit() dispatches on the held
  worker (single-flight) and close() returns it to the pool. The worker
  no longer auto-releases per query.
- Remove query(), newQuery(), and executeSql(). Reads now connect at
  borrow time rather than submit time; under lazy_connect the read pool
  still defaults to min=0, so build() does not fail-fast while the server
  is down.

Test seams:
- Make the white-box seam constructors public and annotate @testonly
  where production never calls them (QuestDBImpl, SenderPool). The
  QueryClientPool connectHook ctor stays public without @testonly because
  QuestDBImpl constructs the query pool through it. Tests now call these
  constructors directly instead of via reflection.

Update the client tests, the usage example, and the startup/failover
design doc to the new API.
try (server) on an effectively-final existing variable is Java 9+ syntax
(JEP 213) and fails the JDK 8 test-compile (the source-of-truth target)
with -source 1.8, breaking the build-jdk8 CI job and the release build
before any test runs. Inline the resource construction into the
try-with-resources declaration, which is valid on Java 8 and keeps the
server variable name used throughout the body.
…se-after-close

A pooled Query/Sender handle was the reused per-slot object itself, guarded
only by a non-volatile in-use/borrowed flag. Once a worker/slot was released
and re-borrowed, that flag flips back to "live", so a stale handle's
close()/cancel()/write would leak into a *different* borrow: a duplicate close
double-released the worker/slot (enqueued twice -> two concurrent borrowers on
one non-thread-safe client/delegate), and a cached Completion.cancel() or stale
write hit whatever borrow now owns it. Idempotent close() and no-op cancel()
are documented contracts, so this was reachable from contract-legal code, not
just misuse, with pool-wide blast radius and no -ea guard.

Fix: give every borrow its own immutable generation, stamped under the pool
lock when the worker/slot is handed out and bumped again when it is returned.
The reused state stays on the slot; callers get a thin per-borrow handle that
carries the generation and validates it on every operation:
  - close()/cancel() are no-ops on a stale generation (idempotency preserved),
  - submit()/data writes throw,
  - release/giveBack/discardBroken re-check the generation under the pool lock
    so a worker/slot can never be enqueued twice, plus an -ea assert that it is
    not already in the available deque.

Egress: QueryImpl stops being the user-facing Query; new QueryLease wraps it.
Ingest: new SenderSlot is the reused slot; PooledSender becomes the per-borrow
wrapper (keeps the public name, so borrow() still returns it). The per-submit
path stays allocation-free; only the small lease handle is created per borrow
(routinely scalar-replaced under try-with-resources).

Adds QueryLeaseGenerationTest and SenderLeaseGenerationTest covering the
double-release and cross-borrow cancel/write paths; updates the white-box
tests to the new shapes. Full core suite green under -ea (the lone failure is
the unrelated pre-existing FilesTest M2, which fails identically on master).
QueryWorker.runLoop() consumed the dispatch hand-off (q = current) under
signalLock but cleared the slot (current = null) only after runOn()
returned, outside the lock. A Query lease is single-flight but reused:
the user thread loops submit() -> await() on the same handle. The
terminal callback inside runOn() wakes the user thread, which can call
submit() -> dispatch() -- setting current = q and signalling -- before
the worker thread reaches its post-run finally block. That stale
current = null then clobbered the freshly dispatched job and discarded
its already-consumed signal, so the worker parked forever on the
condition while the user thread blocked on a Completion that never
fired. The borrowed worker never returned to the pool and the caller
hung indefinitely.

Clear current under signalLock at the moment of consumption and drop the
post-run finally clear. dispatch() now cannot be clobbered: by the time
the next dispatch runs, the worker is either already awaiting (so the
signal wakes it) or will observe current != null on the while check and
skip awaiting. The exception path leaves current already null, and the
shutdown branch still clears under the lock.

Surfaced as a 60s hang in QuestDBFacadeE2ETest.testSustainedMixedConcurrency
(more threads than pool slots, repeated submit/await per lease). Was
intermittent and timing-sensitive, so it showed up mainly on aarch64 CI;
reproduced locally on x86 about one run in four, and 15/15 clean with
this fix.
borrowQuery() returns a thin Query lease that is freshly allocated on
every borrow, while the heavy state it delegates to -- the per-worker
QueryImpl -- is pre-allocated once and reused across borrows. Nothing
pinned that the fresh wrapper actually points back at the one pooled
QueryImpl, so a regression that allocated a new QueryImpl per borrow (or
dropped the worker's reuse) would have gone unnoticed here.

Add testLeaseWrapsSamePooledQueryImpl: two lease() calls on the same
worker must return distinct wrappers (assertNotSame) that delegate to the
same pooled QueryImpl (assertSame on the reflected impl field). lease()
never dereferences the client or pool, so the worker is built with nulls,
mirroring the null-worker shortcut the reset test already uses.
@mtopolnik

Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 206 / 318 (64.78%)

file detail

path covered line new line coverage
🔵 io/questdb/client/impl/PooledSender.java 14 54 25.93%
🔵 io/questdb/client/impl/QueryLease.java 6 22 27.27%
🔵 io/questdb/client/impl/QueryImpl.java 19 59 32.20%
🔵 io/questdb/client/impl/QuestDBImpl.java 2 4 50.00%
🔵 io/questdb/client/impl/ConfigView.java 5 8 62.50%
🔵 io/questdb/client/Sender.java 11 16 68.75%
🔵 io/questdb/client/cutlass/http/client/HttpClient.java 5 7 71.43%
🔵 io/questdb/client/impl/QueryWorker.java 8 9 88.89%
🔵 io/questdb/client/cutlass/qwp/client/QwpQueryClient.java 12 13 92.31%
🔵 io/questdb/client/impl/SenderSlot.java 19 20 95.00%
🔵 io/questdb/client/impl/SenderPool.java 37 38 97.37%
🔵 io/questdb/client/network/NetworkFacadeImpl.java 1 1 100.00%
🔵 io/questdb/client/cutlass/http/client/WebSocketClient.java 11 11 100.00%
🔵 io/questdb/client/HttpClientConfiguration.java 1 1 100.00%
🔵 io/questdb/client/QuestDBBuilder.java 41 41 100.00%
🔵 io/questdb/client/impl/ConfigSchema.java 2 2 100.00%
🔵 io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java 3 3 100.00%
🔵 io/questdb/client/impl/QueryClientPool.java 9 9 100.00%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants