Skip to content

Commit 6be20d6

Browse files
committed
fix(qwp): SF drainer must never give up on a wall-clock reconnect budget
Invariant B: once rows are in on-disk store-and-forward, the background drainer must NEVER terminate on reconnect_max_duration_millis. A replica-only / all-endpoints-replica window is transient (a replica gets promoted, a primary reappears), so the client must keep retrying -- with capped backoff -- until a primary is reachable. The ONLY producer-observable terminal condition is SF exhaustion; the client must never fail because of the drainer. CursorWebSocketSendLoop.connectLoop (used by both the async-initial-connect and mid-stream reconnect paths on the I/O thread) enforced the budget as a give-up deadline: on expiry it recordFatal'd a PROTOCOL_VIOLATION "...-budget-exhausted" that the next producer call surfaced -> a long-but- recoverable failover window became a permanent, producer-visible terminal. Fix: the loop now runs `while (running)` with capped exponential backoff and returns quietly only when the sender is closing. Genuine terminals (auth / non-421 upgrade / durable-ack capability gap) still return immediately. reconnect_max_duration_millis is no longer consulted here; it continues to bound ONLY the blocking (non-lazy) initial connect in QwpWebSocketSender.buildAndConnect (connectWithRetry), preserving fail-loud startup semantics. Turns green the red-first guards added in questdb-enterprise: ReplicationTest.testQwpDurableAckDrainerNeverGivesUpPastBudget and the e2e test_durable_ack_drainer_never_gives_up_on_reconnect_budget.
1 parent d920b9c commit 6be20d6

1 file changed

Lines changed: 24 additions & 69 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java

Lines changed: 24 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -824,12 +824,22 @@ private void connectLoop(Throwable initial, String phase) {
824824
LOG.warn("cursor I/O loop entering {} loop: {}",
825825
phase, initial.getMessage());
826826
long outageStartNanos = System.nanoTime();
827-
long deadlineNanos = outageStartNanos + reconnectMaxDurationMillis * 1_000_000L;
827+
// INVARIANT B: a store-and-forward drainer must NEVER terminate on a
828+
// wall-clock reconnect budget. A replica-only / all-endpoints-replica
829+
// window is TRANSIENT -- a replica gets promoted, a primary reappears --
830+
// so this background loop retries for as long as it is running, backing
831+
// off between attempts. The ONLY terminal conditions are a genuinely
832+
// non-retriable upgrade (auth / non-421 upgrade / durable-ack capability
833+
// gap), which return directly below, or the sender being stopped. SF
834+
// exhaustion is surfaced to the PRODUCER as append backpressure, never
835+
// here. reconnect_max_duration_millis is intentionally NOT consulted: it
836+
// bounds only the blocking (non-lazy) initial connect in
837+
// QwpWebSocketSender.buildAndConnect, never this background loop.
828838
long backoffMillis = reconnectInitialBackoffMillis;
829839
int attempts = 0;
830840
long lastLogNanos = 0L;
831841
Throwable lastReconnectError = initial;
832-
while (running && System.nanoTime() < deadlineNanos) {
842+
while (running) {
833843
attempts++;
834844
totalReconnectAttempts.incrementAndGet();
835845
try {
@@ -905,12 +915,10 @@ private void connectLoop(Throwable initial, String phase) {
905915
backoffMillis = reconnectInitialBackoffMillis;
906916
lastReconnectError = e;
907917
if (running) {
908-
long remainingNanos = deadlineNanos - System.nanoTime();
909-
if (remainingNanos <= 0L) {
910-
break;
911-
}
912-
long parkNanos = Math.min(reconnectInitialBackoffMillis * 1_000_000L, remainingNanos);
913-
LockSupport.parkNanos(parkNanos);
918+
// Failover usually clears within seconds -- retry at the
919+
// initial interval. No wall-clock deadline (Invariant B): a
920+
// replica is promotable, so keep retrying until it is.
921+
LockSupport.parkNanos(reconnectInitialBackoffMillis * 1_000_000L);
914922
}
915923
continue;
916924
} catch (Throwable e) {
@@ -924,73 +932,20 @@ private void connectLoop(Throwable initial, String phase) {
924932
if (running) {
925933
long jitter = ThreadLocalRandom.current().nextLong(backoffMillis);
926934
long sleepMillis = backoffMillis + jitter;
927-
long remainingMillis = (deadlineNanos - System.nanoTime()) / 1_000_000L;
928-
if (remainingMillis <= 0) {
929-
break;
930-
}
931-
if (sleepMillis > remainingMillis) {
932-
sleepMillis = remainingMillis;
933-
}
934935
LockSupport.parkNanos(sleepMillis * 1_000_000L);
935936
backoffMillis = Math.min(backoffMillis * 2, reconnectMaxBackoffMillis);
936937
}
937938
}
939+
// The loop exits ONLY because running == false, i.e. the sender is
940+
// closing / stopping. Under Invariant B this is NOT a budget give-up
941+
// (there is no wall-clock terminal): we retried until asked to stop, so
942+
// we return quietly and let close() drive shutdown. Un-acked rows remain
943+
// in on-disk SF for this sender's next run or an orphan drainer to ship.
938944
long elapsedMs = (System.nanoTime() - outageStartNanos) / 1_000_000L;
939-
String lastMsg = lastReconnectError.getMessage();
940-
LOG.error("cursor I/O loop giving up {} after {}ms, {} attempts; last error: {}",
945+
String lastMsg = lastReconnectError == null ? "n/a" : lastReconnectError.getMessage();
946+
LOG.info("cursor I/O loop {} stopped after {}ms, {} attempts (sender closing); "
947+
+ "un-acked rows remain in SF for retry; last error: {}",
941948
phase, elapsedMs, attempts, lastMsg);
942-
long fromFsn = engine.ackedFsn() + 1L;
943-
long toFsn = Math.max(fromFsn, engine.publishedFsn());
944-
// Disambiguate by what the sender saw on the wire: if we never got
945-
// a successful upgrade, the user is most likely looking at a config
946-
// problem (typo in addr, wrong port, firewall, server not deployed
947-
// yet); if we connected at least once and then exhausted the budget,
948-
// it's a transient connectivity issue (server down, network flap).
949-
// Tag and free-text hint encode the same signal so both grep-the-logs
950-
// and read-the-message users get it without parsing.
951-
String connectivityTag;
952-
String connectivityHint;
953-
if (hasEverConnected) {
954-
connectivityTag = "connection-lost-budget-exhausted";
955-
connectivityHint = "server unreachable since last connect (transient)";
956-
} else {
957-
connectivityTag = "never-connected-budget-exhausted";
958-
connectivityHint = "never reached the server (check addr/port/firewall)";
959-
}
960-
SenderError err = new SenderError(
961-
SenderError.Category.PROTOCOL_VIOLATION,
962-
SenderError.Policy.HALT,
963-
SenderError.NO_STATUS_BYTE,
964-
connectivityTag + ": " + elapsedMs + "ms / " + attempts
965-
+ " attempts; " + connectivityHint
966-
+ "; last error: " + lastMsg,
967-
SenderError.NO_MESSAGE_SEQUENCE,
968-
fromFsn,
969-
toFsn,
970-
null,
971-
System.nanoTime()
972-
);
973-
totalServerErrors.incrementAndGet();
974-
// recordFatal MUST run before dispatchError so the producer-observable
975-
// terminal error is latched before the handler is invoked.
976-
recordFatal(new LineSenderServerException(err));
977-
dispatchError(err);
978-
// Surface the terminal classification through the connection-event
979-
// dispatcher too. Listeners learn about budget exhaustion without
980-
// having to also subscribe to SenderError. Fire AFTER recordFatal so
981-
// a listener that immediately checks the producer-side terminal state
982-
// sees a consistent picture.
983-
SenderConnectionDispatcher cd = connectionDispatcher;
984-
if (cd != null) {
985-
cd.offer(new SenderConnectionEvent(
986-
SenderConnectionEvent.Kind.RECONNECT_BUDGET_EXHAUSTED,
987-
null, SenderConnectionEvent.NO_PORT,
988-
null, SenderConnectionEvent.NO_PORT,
989-
attempts,
990-
SenderConnectionEvent.NO_ROUND_NUMBER,
991-
lastReconnectError,
992-
System.currentTimeMillis()));
993-
}
994949
}
995950

996951
/**

0 commit comments

Comments
 (0)