Skip to content

chore(qwp): drain the old connection's buffered durable ack on failover reconnect#63

Closed
RaphDal wants to merge 1 commit into
mainfrom
rd/qwp-sf-reconnect-durable-ack
Closed

chore(qwp): drain the old connection's buffered durable ack on failover reconnect#63
RaphDal wants to merge 1 commit into
mainfrom
rd/qwp-sf-reconnect-durable-ack

Conversation

@RaphDal

@RaphDal RaphDal commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

The QWP store-and-forward Sender (request_durable_ack=on + multi-host failover=on) could replay frames it had already received a durable ACK for. On a failover reconnect, CursorWebSocketSendLoop.swapClient(...) closed the old connection and repositioned the replay cursor at ackedFsn + 1 without first draining the old connection's receive buffer. A final STATUS_DURABLE_ACK that had arrived on the old wire but was not yet processed was discarded, so ackedFsn stayed stale and the loop replayed frames the server had already confirmed durable.

Where the failover target already holds those rows (an HA / replicated deployment) and the table has no DEDUP keys, this would surface as duplicate rows. The client invariant "a durably-acked frame is never re-sent" is violated.

Root cause

At a role change, a demoting / handing-off primary defers its close and flushes a final covering STATUS_DURABLE_ACK before it closes — precisely so a store-and-forward client won't replay already-durable work. But the client fails over on the earlier read-only (STATUS_NOT_WRITABLE) rejection, which fires before that ACK exists. By the time the server flushes the covering ACK, the I/O thread is parked in connectLoop, so the ACK lands unread in the old socket's recv buffer. swapClient then closes that socket and repositions at ackedFsn + 1 without reading it — the ACK is discarded and the loop rewinds and replays.

This gap has existed since the store-and-forward client was introduced (edbdc3a8, #17): swapClient has never drained the old connection before repositioning. It became reachable as a duplicate once the server began flushing a covering durable ACK on a role-change close (QwpIngressUpgradeProcessor.roleChangeCloseWithUploadGrace, questdb core #7341).

The fix

Before closing the old connection and computing replayStart, swapClient now drains the old socket's recv buffer and applies any buffered durable ACKs, advancing ackedFsn first. The drain uses a dedicated SwapDrainHandler that applies durable ACKs only and no-ops everything else:

  • OK / NACK frames are ignored — the new connection re-OKs every replayed batch and the server re-emits cumulative durable-ack watermarks from scratch.
  • A trailing CLOSE (the demoting primary closes right after its final ACK) is a no-op, so the drain cannot re-enter fail() / connectLoop while a failover is already in progress.

The drain is best-effort (any exception is swallowed — the connection is going away regardless), and it is a no-op in non-durable mode. Genuinely unacked frames are untouched and still replay, exactly as before (covered by ReconnectTest).

Test

CursorWebSocketSendLoopReconnectDurableAckDrainTest — a deterministic, thread-free regression at the send-loop level (same style as CursorWebSocketSendLoopDurableAckTest): the loop is built but never started, the old WebSocketClient carries one covering durable ACK followed by a CLOSE (the exact shape a demoting primary leaves buffered), three OK'd-but-not-yet-durable frames sit in the pending queue, then swapClient runs. It asserts the ACK is drained and applied (ackedFsn advances past those frames; the cursor does not rewind) and that the trailing CLOSE does not re-enter failover.

  • Before the fix: fails with ackedFsn == -1 — the ACK is dropped and the loop rewinds to FSN 0.
  • After the fix: passes.

Full run: CursorWebSocketSendLoop*Test, ReconnectTest, CursorSendEngineTest, AckWatermarkTest
100 tests, 0 failures.

Scope / a deeper follow-up (not in this PR)

This is the surgical fix: it closes the discard where it happens and is a safety net you want regardless. A more robust alternative — call it "wait out the deferral" — would keep the client on the demoting connection on a NOT_WRITABLE rejection (stop sending, keep draining ACKs + PING) until the final ACK + CLOSE arrive, then fail over. That honors the server's deferral contract end-to-end and is robust to the ack landing after the swap or to a reset, but it reworks the NOT_WRITABLE handling, which is a carefully-tuned state machine (all-replica churn protection, pacing, poison-strike bounds). That belongs in a separate, maintainer-reviewed change; this PR does not touch it. The drain here composes cleanly with that follow-up if it lands later.

References

@RaphDal RaphDal added the bug Something isn't working label Jul 7, 2026
@mtopolnik

Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 13 / 13 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java 13 13 100.00%

@RaphDal

RaphDal commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Superseeded by questdb/questdb#7366

@RaphDal RaphDal closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants