fix(qwp)!: make sf_max_total_bytes cap the whole sender pool and close sender connections cleanly - #104
Merged
Conversation
…cket first CursorWebSocketSendLoop.close() shut the socket down (closeTraffic) right after unparking the I/O thread, so the thread's exit path wrote its WebSocket CLOSE frame and TLS close_notify into a dead socket. Every sender close, including pool idle reaping and lifetime recycling, looked like a dropped connection to the server, which logs those over TLS. close() now gives a live I/O thread up to 100 ms to stop on its own and close its client in order. Only a thread still running after that, stuck in a native send or receive, has its traffic broken, exactly as before. A connect walk blocked on a published in-flight client or a credential pull is cancelled at once, so outage-time closes are not delayed. The normal close path takes no longer than before.
Every sender the pool built got its own sf_max_total_bytes budget, so a server that stopped acknowledging grew the client's native memory by the cap times the pool size: about 133 MiB per sender with the 128 MiB default, 2.1 GiB for sender_pool_max=16. Users size the pool for concurrency, not memory, and a large pool ran them out of RAM. The pool now hands one SegmentBudget to every sender it builds, live and recovery delegates alike, and each sender's SegmentManager charges its segments to it, so sf_max_total_bytes caps the pool as a whole. A spare is charged in one atomic check-and-charge before it is provisioned, so two managers can never both take the budget's last segment. Each sender still always gets its minimum working set of two segments, so a sibling stuck on a dead connection cannot starve it. A standalone Sender keeps a private budget and behaves as before. With the server down, 16 pooled senders now buffer 206 MiB instead of 2126 MiB. What remains above the cap is per-connection buffers outside the segment ring. This changes an explicit sf_max_total_bytes in a pool config from a per-sender to a per-pool limit, and the default pool of 4 now buffers at most 128 MiB rather than 512 MiB. With sf_dir, the 10 GiB default is shared the same way. Background orphan drainers keep their own cap. Also corrects the storeAndForwardMaxTotalBytes Javadoc, which claimed the 128 MiB default applies with sf_dir too; it is 10 GiB there. SegmentManagerSharedBudgetTest covers the cap across managers, the minimum working set, and a concurrent run that must release every charge. SenderPoolSharedBudgetTest measures native memory for a pool writing to a server that never acknowledges; with the pool's budget wiring removed it fails at 3.6 MiB of growth against a 1 MiB budget.
bluestreak01
added a commit
to questdb/questdb
that referenced
this pull request
Oct 4, 2026
Re-points java-questdb-client from 3fc81136 to 74fb1f71, the head of the client's vi_qwp_clean_close branch (questdb/java-questdb-client#104). On top of the clean sender close, that branch now carries "make sf_max_total_bytes cap the whole sender pool". The sender pool hands one SegmentBudget to every sender it builds, so sf_max_total_bytes caps the store-and-forward memory of the pool as a whole rather than of each sender. A server that stops acknowledging no longer grows the client's native memory by the cap times the pool size. An explicit sf_max_total_bytes in a pool config changes from a per-sender to a per-pool limit.
Contributor
[PR Coverage check]😍 pass : 99 / 109 (90.83%) file detail
|
bluestreak01
changed the title
bluestreak01
changed the title
Member
Author
|
Reviewing PR questdb/questdb-enterprise#1258 at level 3 in tandem mode (ENT questdb/questdb-enterprise#1258 and OSS questdb/questdb#7736). As you asked, I also reviewed the client branch #104. The diff gets no Critical findings and both gates pass. There is one Moderate documentation issue in the client and three Minor items. CriticalNone. Moderate1. [CLIENT] The docs understate how far a pool can go over
Where the claim appears:
What the code does:
When it matters:
Base comparison doesn't apply: the text and the shared budget are both new. An independent check found nothing that limits the charge. Fix: add one sentence to the README and these Javadocs: "Data recovered from Minor2. [CLIENT] A comment points to a method this PR removed
3. [ENT]
4. PR metadata
Coverage mapTest gate: pass. Admitted coverage gaps: 0. Tests I ran locally (macOS arm64, JDK 25):
Summary
Logs and the review context are in |
bluestreak01
added
the
QUEUED FOR MERGE
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
2 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#7736 (submodule bump), questdb/questdb-enterprise#1258
Breaking change:
sf_max_total_bytesnow caps the store-and-forward data of a whole sender pool instead of each sender in it. A pool of N senders reaches the cap after buffering 1× the setting rather than N×: the default pool of 4 buffers at most 128 MiB instead of 512 MiB in memory mode, and 10 GiB instead of 40 GiB withsf_dir. Once the pool reaches the cap, appends block until acknowledgements free space, and throw aftersf_append_deadline_millis(30 s by default). To keep the old headroom, multiplysf_max_total_bytesbysender_pool_max. A standaloneSenderis unaffected.This PR carries two fixes for pooled QWP senders, one commit each:
sf_max_total_bytesnow caps the buffered data of a whole sender pool instead of each sender in it. This is the breaking change described above.1. Close sender connections cleanly
A sender close never reached the server as a clean close.
CursorWebSocketSendLoop.close()shut the socket down (closeTraffic(), i.e.shutdown(SHUT_RDWR)) right after unparking the I/O thread. The thread's exit path then wrote its WebSocket CLOSE frame and TLSclose_notifyinto the dead socket, and both writes failed silently. So the server saw every sender close as a dropped connection. That includes the pool's idle reaping and lifetime recycling. Over TLS that is an unclean shutdown (unexpected end of fileor a connection reset), which TLS servers log as an error, once per close.QwpQueryClientwas not affected: it joins its I/O thread before it closes the socket.Change
close()now gives a live I/O thread up to 100 ms (DEFAULT_CLOSE_GRACEFUL_STOP_MILLIS) to stop on its own. Its exit path closes the client in order, first the CLOSE frame and then theclose_notify, before it counts down the shutdown latch. So in the normal case nothing is left to break.What stays the same:
close()still cannot hang on it.ConnectCancellation.isConnectInFlight()). Reconnect backoff still wakes onclose()'s unpark, so closes during an outage are not delayed.Cost
A normal close costs nothing extra: the wait ends as soon as the I/O thread finishes. Over 200 TLS sender closes against a server, after the ACK arrived, p50 was about 285 µs before and 275 µs after. p90 was about 390 µs in both. The only slower case is a stuck I/O thread, which now gets up to 100 ms before its traffic is broken, where before it got none.
Test plan
CursorWebSocketSendLoopGracefulCloseTest:testCloseLetsIdleWorkerCloseClientBeforeBreakingTrafficfails without the fix (close()callscloseTraffic()on an idle worker) and passes with it.testCloseCancelsInFlightConnectWithoutWaitingOutGracefulStoptimes out if the in-flight-connect skip is removed.CursorWebSocketSendLoopBlockedSendCloseTest, which still sees the closer break a stuck send.2. Make
sf_max_total_bytescap the whole sender poolEvery sender the pool built got its own
sf_max_total_bytesbudget. A server that stopped acknowledging therefore grew the client's native memory by the cap times the pool size: about 133 MiB per sender with the 128 MiB default, 2.1 GiB forsender_pool_max=16. Users size the pool for concurrency, not memory, so a large pool could run them out of RAM.Change
The pool hands one
SegmentBudgetto every sender it builds, live and recovery delegates alike. Each sender'sSegmentManagercharges its segments to that budget, sosf_max_total_bytescaps the pool as a whole.SegmentManagercharges a spare in one atomic check-and-charge before it provisions it, so two managers can never both take the budget's last segment.2 × sf_max_segment_bytes, 8 MiB by default), so a sibling stuck on a dead connection cannot starve it. These working sets are the only way the pool's segments can exceed the cap.Senderkeeps a private budget and behaves as before. Background orphan drainers keep their own cap.storeAndForwardMaxTotalBytesJavadoc claimed the 128 MiB default applies withsf_dirtoo; it is 10 GiB there, and the Javadoc now says so. The README documents the pool-wide cap.Effect and cost
With the server down, 16 pooled senders now buffer 206 MiB instead of 2126 MiB. What remains above the cap is per-connection buffers outside the segment ring.
The pool's senders now share one lock in
SegmentBudget. A sender takes it when it provisions or releases a segment, not per row.Test plan
SegmentManagerSharedBudgetTestcovers the cap across managers, the minimum working set, and a concurrent run that must release every charge.SenderPoolSharedBudgetTestmeasures native memory for a pool writing to a server that never acknowledges. With the pool's budget wiring removed it fails at 3.6 MiB of growth against a 1 MiB budget.Both fixes together
CursorWebSocketSendLoop*,SegmentManager*,CursorSendEngine*andSenderPool*suites: 329 tests, 0 failures (JDK 25).java8profile).