Skip to content

Commit 4200697

Browse files
committed
fix(qwp): transient churn must not consume the drainer's durable-ack settle budget (Invariant B)
The drainer's 16-attempt durable-ack settle budget and its wall-clock deadline were shared with the transient paths: every all-replica role reject incremented the same mismatchAttempts counter, and the deadline was anchored at connect entry, so a long failover window burned the budget before a genuine capability gap was ever observed. In a rolling upgrade (role-reject churn, then an old-build node promoted to primary) the first capability-gap attempt could quarantine the slot immediately, collapsing the documented 16-attempt budget to zero. The budget now measures a capability-gap episode: - dedicated capabilityGapAttempts counter, incremented only by QwpDurableAckMismatchException sweeps - wall-clock deadline anchored lazily at the first capability-gap error of the episode, never at connect entry - a role reject resets the episode (topology churn: the offending node is gone), making the javadoc's 'consecutive' literally true - a transport error neither increments nor resets, so a flaky but misconfigured cluster cannot evade the cap - role rejects keep an observability-only counter, fixing the mixed attempt numbers previously reported to the listener Adds interleaved regression tests (role-reject churn before/between capability gaps, wall-clock anchoring, transport no-reset); three of the four fail against the previous shared-counter implementation.
1 parent b40e782 commit 4200697

5 files changed

Lines changed: 241 additions & 34 deletions

File tree

.claude/skills/review-pr/SKILL.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,18 @@ loses data and never hard-fails on a transient outage.
346346
delay between attempts), but the RETRY LOOP ITSELF must be unbounded. Flag a
347347
capped total retry duration or an attempt-count cap on the steady-state
348348
drainer.
349+
- **Sanctioned terminals (orphan-slot drainer only).** The orphan drainer
350+
(`BackgroundDrainer`) MAY quarantine its slot (`.failed` sentinel,
351+
human-in-the-loop) on conditions that are terminal by design: auth failure,
352+
a non-421 upgrade reject, and a genuine cluster-wide durable-ack capability
353+
gap that exhausted its documented settle budget (16 consecutive
354+
capability-gap sweeps, or a wall-clock budget anchored at the FIRST
355+
capability-gap error of the episode — whichever is hit first). These are
356+
NOT violations of the no-budget rule above. The settle budget applies ONLY
357+
to consecutive capability-gap attempts: transient classes (role reject,
358+
transport error) must never increment it or burn its wall clock — a
359+
transient state consuming the terminal budget (shared attempt counter,
360+
entry-anchored deadline) IS a Critical violation of this checklist.
349361

350362
**Pool startup — two modes; the mode decides who sees connectivity errors.**
351363
- `lazy_connect=true`: `build()` MUST succeed with **no server present**. The

.pi/skills/review-pr/SKILL.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,18 @@ loses data and never hard-fails on a transient outage.
357357
delay between attempts), but the RETRY LOOP ITSELF must be unbounded. Flag a
358358
capped total retry duration or an attempt-count cap on the steady-state
359359
drainer.
360+
- **Sanctioned terminals (orphan-slot drainer only).** The orphan drainer
361+
(`BackgroundDrainer`) MAY quarantine its slot (`.failed` sentinel,
362+
human-in-the-loop) on conditions that are terminal by design: auth failure,
363+
a non-421 upgrade reject, and a genuine cluster-wide durable-ack capability
364+
gap that exhausted its documented settle budget (16 consecutive
365+
capability-gap sweeps, or a wall-clock budget anchored at the FIRST
366+
capability-gap error of the episode — whichever is hit first). These are
367+
NOT violations of the no-budget rule above. The settle budget applies ONLY
368+
to consecutive capability-gap attempts: transient classes (role reject,
369+
transport error) must never increment it or burn its wall clock — a
370+
transient state consuming the terminal budget (shared attempt counter,
371+
entry-anchored deadline) IS a Critical violation of this checklist.
360372

361373
**Pool startup — two modes; the mode decides who sees connectivity errors.**
362374
- `lazy_connect=true`: `build()` MUST succeed with **no server present**. The

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

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,12 @@ public final class BackgroundDrainer implements Runnable {
7272
* transient all-replica failover window (role reject) is retried
7373
* indefinitely and is never subject to this cap (Invariant B). The
7474
* wall-clock budget {@code reconnectMaxDurationMillis} also caps this
75-
* capability-gap loop; whichever is hit first triggers escalation. 16
75+
* capability-gap loop; whichever is hit first triggers escalation. Both
76+
* halves of the budget measure a capability-gap <i>episode</i>: the
77+
* deadline is anchored at the first capability-gap error (never at
78+
* connect entry), and an intervening role reject restarts the episode
79+
* -- it proves the topology changed, so the next capability-gap error
80+
* is a fresh episode against a newly promoted node. 16
7681
* attempts gives the cluster room to settle through a rolling upgrade
7782
* (each attempt walks every endpoint internally) without letting a genuine
7883
* cluster-wide misconfig hang the drainer forever.
@@ -171,10 +176,14 @@ public BackgroundDrainer() {
171176
* budget, the drainer drops a {@code .failed} sentinel and exits
172177
* exactly as the original single-shot path did.
173178
* <p>
174-
* Other exceptions (auth failure, version mismatch, transport error,
175-
* etc.) preserve the original behavior: mark failed, exit. They are
176-
* either terminal in their own right or already retried inside
177-
* {@code reconnect()}.
179+
* The budget measures a capability-gap <i>episode</i>: consecutive
180+
* {@link QwpDurableAckMismatchException} sweeps only. Transient
181+
* conditions -- an all-replica failover window (role reject) or a
182+
* transport error -- are retried indefinitely (Invariant B) and never
183+
* consume the budget; a role reject additionally restarts the episode,
184+
* because it proves the topology changed under the rolling upgrade.
185+
* Genuine terminals (auth failure, non-421 upgrade reject) preserve
186+
* the original behavior: mark failed, exit.
178187
*
179188
* @return a fresh durable-ack-capable client, or {@code null} if
180189
* {@link #outcome} has been set to FAILED or STOPPED
@@ -185,10 +194,22 @@ public WebSocketClient connectWithDurableAckRetry() {
185194
// on that path but wires up direct @TestOnly calls so requestStop()
186195
// can unpark them too.
187196
runnerThread = Thread.currentThread();
188-
long startNanos = System.nanoTime();
189-
long deadlineNanos = startNanos + reconnectMaxDurationMillis * 1_000_000L;
190197
long backoffMillis = reconnectInitialBackoffMillis;
191-
int mismatchAttempts = 0;
198+
// Capability-gap settle budget. Counts ONLY consecutive
199+
// QwpDurableAckMismatchException sweeps; the wall-clock deadline is
200+
// anchored lazily at the FIRST capability-gap error of an episode so
201+
// transient churn (role reject, transport) can never burn the budget
202+
// before a genuine gap is even observed. An intervening role reject
203+
// resets the episode (topology churn: the offending node is gone); a
204+
// transport error neither increments nor resets -- a dropped socket
205+
// does not prove promotion churn, and resetting on it would let a
206+
// flaky-but-misconfigured cluster evade the cap forever.
207+
int capabilityGapAttempts = 0;
208+
long capabilityGapStartNanos = 0L; // 0 = no episode active
209+
long capabilityGapDeadlineNanos = Long.MAX_VALUE;
210+
// Observability-only counter for the transient all-replica window;
211+
// never consulted for escalation (Invariant B).
212+
int roleRejectAttempts = 0;
192213
// Throttle the all-replica retry WARN to one per 5s: a real failover
193214
// window can last minutes and (Invariant B) is retried indefinitely, so
194215
// per-attempt logging would flood. Mirrors CursorWebSocketSendLoop.
@@ -222,11 +243,18 @@ public WebSocketClient connectWithDurableAckRetry() {
222243
// stopRequested, or SF exhaustion -- it must NEVER quarantine the
223244
// slot on a wall-clock budget or an attempt cap. Surface the
224245
// per-attempt observability callback, then back off and retry.
225-
mismatchAttempts++;
246+
roleRejectAttempts++;
247+
// Topology is mid-churn: whatever node produced any earlier
248+
// capability-gap errors is no longer the primary the next
249+
// sweep hits, so the gap episode (attempts + deadline)
250+
// restarts and the next gap gets the full settle budget.
251+
capabilityGapAttempts = 0;
252+
capabilityGapStartNanos = 0L;
253+
capabilityGapDeadlineNanos = Long.MAX_VALUE;
226254
BackgroundDrainerListener l = listener;
227255
if (l != null) {
228256
try {
229-
l.onDurableAckUnavailable(slotPath, mismatchAttempts);
257+
l.onDurableAckUnavailable(slotPath, roleRejectAttempts);
230258
} catch (Throwable cb) {
231259
LOG.warn("drainer listener onDurableAckUnavailable threw: {}",
232260
cb.getMessage());
@@ -236,7 +264,7 @@ public WebSocketClient connectWithDurableAckRetry() {
236264
if (nowWarn - lastReplicaWarnNanos >= 5_000_000_000L) {
237265
LOG.warn("drainer slot {} attempt {}: all endpoints are replicas "
238266
+ "(transient failover window), retrying after backoff",
239-
slotPath, mismatchAttempts);
267+
slotPath, roleRejectAttempts);
240268
lastReplicaWarnNanos = nowWarn;
241269
}
242270
} catch (QwpDurableAckMismatchException e) {
@@ -245,18 +273,25 @@ public WebSocketClient connectWithDurableAckRetry() {
245273
// reject this will not clear by waiting for a promotion, so it
246274
// stays terminal for the drainer -- give the cluster a bounded
247275
// settle budget (rolling upgrade), then quarantine the slot.
248-
mismatchAttempts++;
276+
capabilityGapAttempts++;
249277
long now = System.nanoTime();
250-
long elapsedMs = (now - startNanos) / 1_000_000L;
251-
boolean exhausted = mismatchAttempts >= DEFAULT_MAX_DURABLE_ACK_MISMATCH_ATTEMPTS
252-
|| now >= deadlineNanos;
278+
if (capabilityGapStartNanos == 0L) {
279+
// First gap error of this episode: anchor the wall-clock
280+
// half of the budget HERE, not at connect entry, so time
281+
// spent in a preceding transient window is not charged.
282+
capabilityGapStartNanos = now;
283+
capabilityGapDeadlineNanos = now + reconnectMaxDurationMillis * 1_000_000L;
284+
}
285+
long elapsedMs = (now - capabilityGapStartNanos) / 1_000_000L;
286+
boolean exhausted = capabilityGapAttempts >= DEFAULT_MAX_DURABLE_ACK_MISMATCH_ATTEMPTS
287+
|| now >= capabilityGapDeadlineNanos;
253288
BackgroundDrainerListener l = listener;
254289
if (exhausted) {
255290
LOG.error("drainer giving up on slot {} after {} durable-ack-mismatch attempts ({}ms): {}",
256-
slotPath, mismatchAttempts, elapsedMs, e.getMessage());
291+
slotPath, capabilityGapAttempts, elapsedMs, e.getMessage());
257292
if (l != null) {
258293
try {
259-
l.onDurableAckPersistentFailure(slotPath, mismatchAttempts, elapsedMs);
294+
l.onDurableAckPersistentFailure(slotPath, capabilityGapAttempts, elapsedMs);
260295
} catch (Throwable cb) {
261296
LOG.warn("drainer listener onDurableAckPersistentFailure threw: {}",
262297
cb.getMessage());
@@ -265,21 +300,21 @@ public WebSocketClient connectWithDurableAckRetry() {
265300
lastErrorMessage = e.getMessage();
266301
OrphanScanner.markFailed(slotPath,
267302
"durable-ack persistently unavailable after "
268-
+ mismatchAttempts + " attempts: " + e.getMessage());
303+
+ capabilityGapAttempts + " attempts: " + e.getMessage());
269304
outcome = DrainOutcome.FAILED;
270305
return null;
271306
}
272307
boundedByBudget = true;
273308
if (l != null) {
274309
try {
275-
l.onDurableAckUnavailable(slotPath, mismatchAttempts);
310+
l.onDurableAckUnavailable(slotPath, capabilityGapAttempts);
276311
} catch (Throwable cb) {
277312
LOG.warn("drainer listener onDurableAckUnavailable threw: {}",
278313
cb.getMessage());
279314
}
280315
}
281316
LOG.warn("drainer slot {} attempt {}: durable-ack unavailable, retrying after backoff",
282-
slotPath, mismatchAttempts);
317+
slotPath, capabilityGapAttempts);
283318
} catch (Throwable t) {
284319
// INVARIANT B: a transport failure -- the whole cluster is
285320
// unreachable right now (server down, network partition) -- is
@@ -307,7 +342,7 @@ public WebSocketClient connectWithDurableAckRetry() {
307342
long sleepMillis = backoffMillis + jitter;
308343
if (boundedByBudget) {
309344
sleepMillis = Math.min(sleepMillis,
310-
Math.max(0L, (deadlineNanos - System.nanoTime()) / 1_000_000L));
345+
Math.max(0L, (capabilityGapDeadlineNanos - System.nanoTime()) / 1_000_000L));
311346
}
312347
if (sleepMillis > 0L && !stopRequested) {
313348
long parkDeadlineNanos = System.nanoTime() + sleepMillis * 1_000_000L;

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

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,27 +43,34 @@ public interface BackgroundDrainerListener {
4343

4444
/**
4545
* Fired when the drainer has retried past its budget on consecutive
46-
* durable-ack-unavailable failures. The drainer drops a {@code .failed}
47-
* sentinel and exits. Treat as cluster-wide misconfiguration and
48-
* surface to operators.
46+
* durable-ack capability-gap failures. The drainer drops a
47+
* {@code .failed} sentinel and exits. Treat as cluster-wide
48+
* misconfiguration and surface to operators.
4949
*
5050
* @param slotPath slot the drainer was processing
51-
* @param totalAttempts how many connect attempts hit the same failure
52-
* @param elapsedMillis wall time spent on this failure mode
51+
* @param totalAttempts capability-gap attempts in the final episode;
52+
* transient sweeps (role reject, transport) are
53+
* never counted
54+
* @param elapsedMillis wall time of the final capability-gap episode,
55+
* anchored at its first capability-gap error
5356
*/
5457
void onDurableAckPersistentFailure(String slotPath, int totalAttempts, long elapsedMillis);
5558

5659
/**
57-
* Fired when {@code clientFactory.reconnect()} threw
58-
* {@code QwpDurableAckMismatchException} — i.e. every endpoint in the
59-
* current sweep failed to advertise durable ack. The drainer will
60-
* back off and retry; this callback is purely observability. Source
61-
* data stays pinned regardless because the loop runs in
62-
* {@code durableAckMode=true} and only trims on STATUS_DURABLE_ACK.
60+
* Fired when a connect sweep found durable ack unavailable — either a
61+
* genuine capability gap ({@code QwpDurableAckMismatchException}: an
62+
* endpoint upgrades but does not advertise durable ack) or a transient
63+
* all-replica failover window (role reject). The drainer will back off
64+
* and retry; this callback is purely observability. Source data stays
65+
* pinned regardless because the loop runs in {@code durableAckMode=true}
66+
* and only trims on STATUS_DURABLE_ACK.
6367
*
6468
* @param slotPath slot the drainer is processing
65-
* @param attemptNumber 1-based count of consecutive durable-ack-unavailable
66-
* failures for this drainer
69+
* @param attemptNumber 1-based attempt count within the current mode:
70+
* the running role-reject count for a transient
71+
* window, or the attempt number within the current
72+
* capability-gap episode (restarts when a role
73+
* reject resets the episode)
6774
*/
6875
void onDurableAckUnavailable(String slotPath, int attemptNumber);
6976
}

0 commit comments

Comments
 (0)