test(qwp): close TestWebSocketServer connections gracefully so scripted drops deliver their acks

[jovfer]

Failure

BackgroundDrainerMidDrainCapabilityGapTest#testDeliveringBetweenTwoGapWindowsGrantsAFreshSettleBudget failed on the "Build, test & javadoc (JDK 8)" job of #91 (run https://github.com/questdb/java-questdb-client/actions/runs/34368418967):

delivering between the windows ends the episode, so neither window reaches the threshold
and the slot must still drain [attempts=20] expected:<SUCCESS> but was:<FAILED>

The two pushes of #91 earlier that day and main's own JDK 8 run (981bdb0, 2026-09-04) were green; the only delta between the green and red heads was two unrelated test files. The test and the code it exercises are main code, not part of #91.

Evidence

Comparing the drainer logs of the red run, the green run and main's run for this test class:

• Red run, the delivering session between the two gap windows: peer disconnect [104] with the pending range still [1,4]. No STATUS_DURABLE_ACK advanced the watermark, so noteAckProgress never reset the episode and the second window ran the settle counter on to 16.
• Green runs, same session: peer disconnect [11] (FIN) with the range advanced to [3,4], then the counter restarted at 1 for the second window and the slot drained.
• Main's run shows the same [104] / no-advance pairing on the first connection of the same test, where it happens not to matter. The pairing "errno 104, watermark unchanged" appears in every run; the test fails only when it lands on the delivering session.

Mechanism

GapScenarioHandler acks a batch with two back-to-back sendBinary writes (OK frame, then STATUS_DURABLE_ACK) and drops the wire with client.close() at the next sequence number. TestWebSocketServer set no TCP_NODELAY, so the second small write was held by Nagle; the client does set it, so its remaining frames arrive as separate segments. When one of them is still unread at socket.close(), the kernel sends RST instead of FIN and purges the held durable-ack write. Which connection hits that depends on scheduling, hence the flake.

The old ClientHandler.close() also did readThread.join(5000) on the read thread itself when called from a handler callback. That self-join parked the synchronized handler for the full 5 s on every scripted drop, which is where the 5 s cadence between sessions in the CI logs came from, and it kept the client's trailing frames unread in the kernel buffer for the whole window.

Fix

In TestWebSocketServer:

• setTcpNoDelay(true) on every accepted socket, so both ack writes leave immediately.
• ClientHandler.close() calls shutdownOutput() first, so already-written frames are followed by FIN rather than an RST; when the close runs on the read thread (the scripted-drop path) it drains unread input with a 20 ms socket timeout and a 200 ms cap before socket.close(), so no unread data remains to turn the close into an RST on any platform.
• The read-thread join is skipped when close() runs on that thread.

Cross-thread closes (server teardown) keep today's fast path: FIN, then close(), then the join.

Verification

Local, arm64 JDK 25, mvn -pl core surefire:test:

Run	Tests	Failures	Errors	Skipped	Wall time
main 981bdb0 (baseline)	3456	0	0	5	6 min 35 s
this branch	3456	0	0	5	3 min 32 s

BackgroundDrainerMidDrainCapabilityGapTest run five times in a row: 6/6 green each time, 0.99 s to 1.16 s per run, down from 40.15 s on the baseline. The suite speed-up is the removed 5 s self-join stall per scripted drop across the drainer tests.

The flake itself is a Linux kernel RST-vs-FIN race that does not reproduce on macOS; the CI JDK 8 job is the check that exercises it.

🤖 Generated with Claude Code

https://claude.ai/code/session_017S9gyXv4MwoDHEohA9At5d

[bluestreak01]

Reviewing PR #95 at level 3 (full review).

Scope: the change is test-only. It touches one file, TestWebSocketServer.java (+27/−1), adds no production code and moves no submodule pointer.

Critical

None.

Moderate

None.

Minor

1. drainInput() breaks ClientHandler's member ordering

• Problem: the new private drainInput() sits inside ClientHandler's block of public methods.
• Net impact: none at runtime. It breaks the member-ordering standard (group by visibility, then sort alphabetically).
• Evidence: read from the source at 0388d1c: public close() is at :466, private drainInput() at :492, and public sendBinary at :507. The private block starts at computeAcceptKey (:540) and continues with handleRead (:550).
• At base, the public and private blocks were each contiguous and alphabetical, so this PR introduced the break.
• Fix: move drainInput() into the private block, between computeAcceptKey and handleRead.

2. The "never an RST" comment is false for closes from other threads

• Problem: the comment promises "never an RST", but close() called from another thread still sends one.
• Net impact: the fixture's comment misleads readers. Frames already written are still delivered.
• Evidence: a probe against the real fixture at 0388d1c (Linux arm64, JDK 25) counted the kernel's TCPAbortOnClose counter, which records an RST sent because unread input was discarded on close:
  • close() from another thread with unread input: 10 of 10 closes sent an RST.
  • close() from the read thread: 0 of 10.
  • The written frame reached the client in all 10 runs on both paths.
• Why: drainInput() only runs when readThread == Thread.currentThread() (:472). TestWebSocketServer.close() (:208) and SymbolDictRecycleCatchUpSkipTest.dropConnection call close() from the test thread. On that path socket.close() (:479) can run with unread input, so an RST follows the FIN.
• Base: the same cross-thread probe also sent an RST 10 of 10 times. This is a comment problem, not a regression.
• Fix: limit the comment to the path it describes, e.g. "Half-close so already-written frames go out ahead of a FIN; on the read thread also drain unread input so close() is not turned into an RST."

Coverage map

The test gate passes, with 0 admitted coverage gaps. No production behaviour changed, so no tandem PR is required; searches for a matching branch in questdb and questdb-enterprise found none.

Summary

• Verdict: approve. Both findings are Minor and optional.

• Is the fix real? Yes. I built base (21e3268) and head (0388d1c) and ran them in a Linux arm64 container.

  Target test (testDeliveringBetweenTwoGapWindowsGrantsAFreshSettleBudget):

  Revision	JDK	Passed	Avg time	Notes
  base	8	18 of 20	~10.1 s	the 2 failures have the same assertion and log signature as the CI failure
  head	8	300 of 300	288 ms	no ECONNRESET ([104]) disconnects at all
  head	25	300 of 300	287 ms

  The base failures match the CI failure exactly: the delivering session ends in [104] peer disconnect, the pending range does not advance, and the drainer gives up.

  Deterministic probe: the handler sends two acks back-to-back, then closes from the read thread while more than 8 KiB of client input is still unread.

  Revision	JDK	Result
  base	8	15 of 15 runs: the client gets 1 byte, then "Connection reset"; both frames are lost
  head	8	200 of 200 runs: the client gets both frames, then a clean end-of-stream

  Wider check: the 17 classes that close connections in scripted drops (133 tests) passed 3 of 3 runs on JDK 8 at head. All CI checks are green at 0388d1c, including Azure Linux, macOS and Windows.

• Correction to the description: whether the race happens depends on the JDK, not the OS. Since JDK 13, NioSocketImpl.close() already half-closes the socket before closing it (JDK source NioSocketImpl.java:891-899). On JDK 25 the base fixture delivers both frames even on Linux. That is why it never reproduced on macOS with JDK 25, and why only the JDK 8 job catches it. The new shutdownOutput() gives JDK 8 the same half-close.

• Submodules: no pointer moved.

• Callsites: I reviewed every ClientHandler.close() caller: 19 handler sites, the test-thread dropConnection, and server teardown. I also reviewed the handshakeCount and liveConnectionCount users and the sendClose / sendBinaryThenClose users. Nothing broke. Of the 2 findings, both are in the diff and none affect unchanged code.

• Severity: 0 Critical, 0 Moderate, 2 Minor.