Skip to content

Commit 94ddc60

Browse files
committed
fix(qwp): re-arm the recovery scan when a released retired slot still holds data
Two remaining drain-liveness windows around retired-slot flock releases: 1. Post-latch runtime retire: discardBroken/reapIdle can retire a wedged slot AFTER the startup scan legitimately latched recoveryComplete. The late release then restored capacity only -- nothing re-drained the dir (the scan was done, close() never owns an unowned dir), so the data waited for a restart or a lucky borrow of that exact index. 2. Mid-cycle release (found by the drain-liveness fuzz): a retire AND its release can both land while the scan is alive but after the cursor already passed that index -- it was LIVE at walk time, so no retired-candidate deferral was counted, and recoveryComplete was still false at release, so no post-latch path applied. A cycle ending with zero deferrals then latched past the freed dir's stranded data. recoverRetiredSlotAt now probes the freed dir: if it is still a candidate orphan it either un-latches and rewinds the scan (post-latch case; the volatile latch publishes the rewound cursors to the unlocked driver gates) or raises recoveryRearmRequested (alive case; cursors stay driver-owned). The end-of-cycle latch decision consumes the flag under the pool lock -- the producer runs under the same lock, so no release can slip between the check and the latch write. Deferred pools re-arm through the housekeeper's unconditional per-tick step; a direct pool whose private driver already exited gets the driver revived (daemon, same loop, joined by close()). Three deterministic pins, each red before this change: the post-latch re-arm, the revived direct driver draining with no external help, and the mid-cycle release that must not latch past data it never revisited.
1 parent ccb9229 commit 94ddc60

2 files changed

Lines changed: 469 additions & 15 deletions

File tree

core/src/main/java/io/questdb/client/impl/SenderPool.java

Lines changed: 161 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,18 @@ public final class SenderPool implements AutoCloseable {
191191
// leave this null.
192192
private final Object startupRecoverySignal = new Object();
193193
private final Thread startupRecoveryThread;
194+
// Revived direct-pool recovery driver: spawned by recoverRetiredSlotAt()
195+
// when a retired slot's late flock release re-arms an already-latched
196+
// scan on a pool whose original private driver has exited. Deferred
197+
// pools re-arm through the PoolHousekeeper tick instead and never spawn
198+
// one. Joined by stopStartupRecoveryDriver() on close. Volatile: written
199+
// under `lock`, read by close() without it.
200+
private volatile Thread revivedStartupRecoveryThread;
201+
// Driver policy captured for the re-arm path: deferred pools are driven
202+
// by PoolHousekeeper (never spawn private drivers); direct pools own
203+
// their (revivable) private driver, built via recoveryThreadFactory.
204+
private final boolean deferStartupRecovery;
205+
private final ThreadFactory recoveryThreadFactory;
194206
// Test-only constructor seam for the failed-retry operation. Production
195207
// uses waitForStartupRecoveryRetry(), which performs the one-second signal
196208
// wait; lifecycle tests inject an event barrier without wall-clock checks.
@@ -290,14 +302,32 @@ public final class SenderPool implements AutoCloseable {
290302
// A RETIRED index whose dir still holds data is deferred the same way on
291303
// every walk (see the reserved-skip branch in recoverOneSlotStep) so the
292304
// latch can never strand a retired slot's data while the pool lives.
305+
// If the retire lands only AFTER the scan latched (a runtime
306+
// discardBroken/reapIdle reclaim), the late flock release re-arms the
307+
// scan instead: recoverRetiredSlotAt() rewinds the cursors and clears
308+
// recoveryComplete -- volatile, so the un-latch (written under `lock` by
309+
// release callbacks and reprobes) is visible to the unlocked driver
310+
// gates -- and, for direct pools whose private driver already exited,
311+
// revives the driver. Deferred pools re-arm through the housekeeper tick.
293312
// recoveryFailStreak/-Slot track consecutive failures on one candidate;
294313
// recoveryWarnedSlots dedups the per-slot WARNs so an indefinitely
295314
// retried slot logs once per failure episode, not once per retry. All of
296315
// this state is owned by the single recovery driver like the cursors.
297316
private int recoveryInRangeNext;
298317
private IntList recoveryOutOfRange;
299318
private int recoveryOutOfRangeNext;
300-
private boolean recoveryComplete;
319+
private volatile boolean recoveryComplete;
320+
// Set by recoverRetiredSlotAt() whenever a released retired slot's dir is
321+
// still a candidate orphan, and consumed (under `lock`) by the scan's
322+
// end-of-cycle latch decision. Closes the mid-cycle window: a retire AND
323+
// its release can both land while the scan is alive but after the cursor
324+
// already passed that index (it was LIVE when walked -- no retired-
325+
// candidate deferral -- and recoveryComplete was still false at release,
326+
// so the post-latch re-arm does not apply). Without this flag such a
327+
// cycle could end with zero deferrals and latch past the freed dir's
328+
// stranded data. Producer and consumer both run under `lock`, so no
329+
// set can slip between the driver's check and its latch write.
330+
private volatile boolean recoveryRearmRequested;
301331
private int recoveryDeferredThisCycle;
302332
private int recoveryFailStreak;
303333
private int recoveryFailStreakSlot = -1;
@@ -455,6 +485,8 @@ private SenderPool(
455485
this.maxLifetimeMillis = maxLifetimeMillis;
456486
this.postFactoryHook = postFactoryHook;
457487
this.beforeFailedStartupRecoveryJoinHook = beforeFailedRecoveryJoinHook;
488+
this.deferStartupRecovery = deferStartupRecovery;
489+
this.recoveryThreadFactory = recoveryThreadFactory;
458490
this.startupRecoveryWaiter = recoveryWaiter != null
459491
? recoveryWaiter
460492
: this::waitForStartupRecoveryRetry;
@@ -912,7 +944,26 @@ private boolean recoverOneSlotStep(long stepBudgetMillis) {
912944
recoveryOutOfRangeNext = 0;
913945
return false;
914946
}
915-
recoveryComplete = true;
947+
// Latch decision under `lock`: a mid-cycle flock release can free a
948+
// still-candidate dir AFTER this cycle's cursor passed its index (it
949+
// was live/retired at walk time -- no deferral) and BEFORE the latch
950+
// (so the post-latch re-arm in recoverRetiredSlotAt does not apply
951+
// either). recoverRetiredSlotAt raises recoveryRearmRequested under
952+
// the same lock, so consuming it here mutually excludes the race: no
953+
// release can slip between this check and the latch write.
954+
lock.lock();
955+
try {
956+
if (recoveryRearmRequested) {
957+
recoveryRearmRequested = false;
958+
recoveryDeferredThisCycle = 0;
959+
recoveryInRangeNext = 0;
960+
recoveryOutOfRangeNext = 0;
961+
return false;
962+
}
963+
recoveryComplete = true;
964+
} finally {
965+
lock.unlock();
966+
}
916967
return false;
917968
}
918969

@@ -1583,19 +1634,30 @@ private void stopFailedStartupRecoveryDriver(Thread recoveryThread) {
15831634
}
15841635

15851636
private void stopStartupRecoveryDriver() {
1586-
if (startupRecoveryThread == null || startupRecoveryThread == Thread.currentThread()) {
1587-
return;
1588-
}
1589-
if (beforeStartupRecoveryJoinHook != null) {
1590-
beforeStartupRecoveryJoinHook.run();
1591-
}
1592-
try {
1593-
startupRecoveryThread.join(PoolHousekeeper.STOP_TIMEOUT_MILLIS);
1594-
} catch (InterruptedException e) {
1595-
Thread.currentThread().interrupt();
1637+
if (startupRecoveryThread != null && startupRecoveryThread != Thread.currentThread()) {
1638+
if (beforeStartupRecoveryJoinHook != null) {
1639+
beforeStartupRecoveryJoinHook.run();
1640+
}
1641+
try {
1642+
startupRecoveryThread.join(PoolHousekeeper.STOP_TIMEOUT_MILLIS);
1643+
} catch (InterruptedException e) {
1644+
Thread.currentThread().interrupt();
1645+
}
1646+
if (afterStartupRecoveryJoinHook != null) {
1647+
afterStartupRecoveryJoinHook.run();
1648+
}
15961649
}
1597-
if (afterStartupRecoveryJoinHook != null) {
1598-
afterStartupRecoveryJoinHook.run();
1650+
// A revived driver (late-release re-arm on a direct pool; see
1651+
// reviveDirectRecoveryDriverIfNeeded) is joined with the same bound:
1652+
// `closed` is already raised, so its loop exits on the next waiter
1653+
// wake-up, within RECOVERY_RETRY_INTERVAL_MILLIS < STOP_TIMEOUT.
1654+
Thread revived = revivedStartupRecoveryThread;
1655+
if (revived != null && revived != Thread.currentThread()) {
1656+
try {
1657+
revived.join(PoolHousekeeper.STOP_TIMEOUT_MILLIS);
1658+
} catch (InterruptedException e) {
1659+
Thread.currentThread().interrupt();
1660+
}
15991661
}
16001662
}
16011663

@@ -2019,7 +2081,10 @@ private static boolean flockReleased(SenderSlot s) {
20192081
* capacity and no borrow reuses the still-locked dir unless
20202082
* {@link #reprobeRetiredSlots} later observes the deferred cleanup's
20212083
* release and recovers the index. Either way {@code closingSlots} is
2022-
* decremented.
2084+
* decremented. A later release that frees a dir still holding unacked
2085+
* data also re-arms the startup recovery scan (see
2086+
* {@link #recoverRetiredSlotAt}) so the data is drained in-process
2087+
* instead of waiting for a restart or a lucky borrow of that index.
20232088
* <p>
20242089
* Caller must hold {@code lock} and is responsible for signalling waiters
20252090
* (only the free path admits a new creation). Shared by
@@ -2137,6 +2202,87 @@ private void recoverRetiredSlotAt(int retiredIndex) {
21372202
LOG.info("SF slot {} recovered: deferred cleanup released the flock after retirement; " +
21382203
"pool capacity restored, now {} of {} usable [leakedSlots={}]",
21392204
s.slotIndex(), maxSize - leakedSlots, maxSize, leakedSlots);
2205+
// The freed dir may still hold unacked durable data: a runtime retire
2206+
// (discardBroken/reapIdle) can land AFTER the startup scan latched
2207+
// recoveryComplete, and close() never drains an unowned dir, so
2208+
// without re-arming the scan that data would wait for a restart or a
2209+
// lucky borrow of exactly this index. When the latch is already set,
2210+
// rewind the cursors FIRST, then clear the volatile latch (the write
2211+
// order publishes the rewound cursors to the next driver's unlocked
2212+
// gate read), then revive the private driver when this is a direct
2213+
// pool whose original driver already exited; recoveryComplete==true
2214+
// here also guarantees no driver is mid-step, so touching the
2215+
// driver-owned cursors is safe. When the scan is still ALIVE, only
2216+
// raise recoveryRearmRequested: the cursor may already have passed
2217+
// this index while it was live/retired, and the flag makes the
2218+
// end-of-cycle latch decision rewind instead -- the cursors stay
2219+
// strictly driver-owned. The isCandidateOrphan probe is a bounded
2220+
// directory scan; holding the pool lock across it mirrors
2221+
// reprobeRetiredSlots()' delegate probes. The pass-2 work list is
2222+
// kept and only its cursor rewound, exactly like the end-of-scan
2223+
// cycle rewind.
2224+
if (sfDir != null
2225+
&& OrphanScanner.isCandidateOrphan(sfDir + "/" + slotBaseId + "-" + s.slotIndex())) {
2226+
if (recoveryComplete) {
2227+
recoveryInRangeNext = 0;
2228+
recoveryOutOfRangeNext = 0;
2229+
recoveryDeferredThisCycle = 0;
2230+
recoveryRearmRequested = false;
2231+
recoveryComplete = false;
2232+
LOG.info("SF slot {} released with stranded data still on disk; re-arming the "
2233+
+ "startup recovery scan to drain it", s.slotIndex());
2234+
reviveDirectRecoveryDriverIfNeeded();
2235+
} else {
2236+
recoveryRearmRequested = true;
2237+
LOG.info("SF slot {} released with stranded data while the recovery scan is "
2238+
+ "mid-cycle; flagging a rewind so this cycle cannot latch past it",
2239+
s.slotIndex());
2240+
}
2241+
}
2242+
}
2243+
2244+
/**
2245+
* Re-arms a driver for an un-latched scan (see {@link #recoverRetiredSlotAt}).
2246+
* Deferred pools need nothing: {@code PoolHousekeeper} calls
2247+
* {@link #runStartupRecoveryStep()} on every tick and its gate re-opens
2248+
* with the cleared latch. A direct pool's private driver exits once the
2249+
* scan completes, so if neither the original nor a previously revived
2250+
* driver is alive, spawn a fresh one running the same loop -- it drains
2251+
* the backlog, re-latches, and exits; {@code close()} joins it via
2252+
* {@link #stopStartupRecoveryDriver}. Caller must hold {@code lock};
2253+
* {@code Thread.start()} is cheap enough to run under it. Best-effort: a
2254+
* spawn failure is logged and never propagates into the release callback
2255+
* that triggered the re-arm (the data stays durable for a restart). A
2256+
* driver observed alive here can, in principle, be exiting past its final
2257+
* latch check; that microscopic window degrades to the pre-fix behavior
2258+
* (the data waits for the next release event or a restart) and is
2259+
* accepted rather than risking two concurrent drivers on the
2260+
* driver-owned cursors.
2261+
*/
2262+
private void reviveDirectRecoveryDriverIfNeeded() {
2263+
if (deferStartupRecovery || closed) {
2264+
return;
2265+
}
2266+
if (startupRecoveryThread != null && startupRecoveryThread.isAlive()) {
2267+
return;
2268+
}
2269+
Thread revived = revivedStartupRecoveryThread;
2270+
if (revived != null && revived.isAlive()) {
2271+
return;
2272+
}
2273+
try {
2274+
ThreadFactory factory = recoveryThreadFactory != null
2275+
? recoveryThreadFactory
2276+
: SenderPool::createStartupRecoveryThread;
2277+
Thread t = factory.newThread(this::runStartupRecoveryLoop);
2278+
t.setDaemon(true);
2279+
revivedStartupRecoveryThread = t;
2280+
t.start();
2281+
} catch (Throwable e) {
2282+
revivedStartupRecoveryThread = null;
2283+
LOG.warn("could not revive the startup recovery driver ({}); the released slot's "
2284+
+ "data stays durable on disk for a later attempt or restart", e.toString());
2285+
}
21402286
}
21412287

21422288
// Outcome of one drainCandidateSlotForRecovery attempt, letting the scan

0 commit comments

Comments
 (0)