Skip to content

Commit ab2b3b6

Browse files
committed
fix(qwp): make the recovery-driver revival handoff transactional
The revive path decided "will a driver observe my un-latch?" by probing Thread.isAlive() -- an out-of-band signal not ordered with the driver's own exit decision. A driver past its final latch check could still report alive, so a release landing in that window skipped the spawn and left the un-latched scan ownerless until the next release event or restart (a microscopic, documented-but-real lost-wakeup corner on direct pools). Ownership is now a lock-guarded token (recoveryDriverRunning). The loop drops it transactionally: releaseRecoveryDriverOwnership re-checks the latch under the pool lock before the drop, so an un-latch that raced the exit keeps this thread driving; the producer (recoverRetiredSlotAt) clears the latch and reads the token under the same lock, so it spawns a successor exactly when no owner remains. Mutual exclusion makes the two decisions serial: no interleaving leaves an un-latched scan ownerless, and cursor ownership hands off cleanly (predecessor's last cursor touch happens-before its token drop happens-before the successor's spawn, all via one lock). Loop semantics are otherwise preserved: waiter only after a workless step on a live un-latched scan, prompt exits on close/interrupt (which also drop the token so a still-live pool can revive later).
1 parent e868354 commit ab2b3b6

1 file changed

Lines changed: 77 additions & 26 deletions

File tree

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

Lines changed: 77 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,16 @@ public final class SenderPool implements AutoCloseable {
198198
// one. Joined by stopStartupRecoveryDriver() on close. Volatile: written
199199
// under `lock`, read by close() without it.
200200
private volatile Thread revivedStartupRecoveryThread;
201+
// Ownership token for the private (direct-pool) recovery driver duty.
202+
// Guarded by `lock`. TRUE while some thread owns runStartupRecoveryLoop;
203+
// the loop drops it transactionally on exit (re-checking the latch under
204+
// the lock first -- see releaseRecoveryDriverOwnership), and
205+
// reviveDirectRecoveryDriverIfNeeded() spawns a successor only when it
206+
// is FALSE. Replaces a Thread.isAlive() heuristic whose read raced the
207+
// driver's final latch check: a driver observed "alive" could already
208+
// have committed to exit, leaving an un-latched scan ownerless until the
209+
// next release event or a restart.
210+
private boolean recoveryDriverRunning;
201211
// Driver policy captured for the re-arm path: deferred pools are driven
202212
// by PoolHousekeeper (never spawn private drivers); direct pools own
203213
// their (revivable) private driver, built via recoveryThreadFactory.
@@ -544,6 +554,9 @@ private SenderPool(
544554
: SenderPool::createStartupRecoveryThread;
545555
recoveryThread = threadFactory.newThread(this::runStartupRecoveryLoop);
546556
recoveryThread.setDaemon(true);
557+
// Ownership precedes start(): the loop's transactional
558+
// exit is the only place the token is dropped.
559+
recoveryDriverRunning = true;
547560
}
548561
}
549562
this.startupRecoveryThread = recoveryThread;
@@ -609,7 +622,25 @@ void runStartupRecoveryToCompletion() {
609622
}
610623

611624
private void runStartupRecoveryLoop() {
612-
while (!closed && !recoveryComplete) {
625+
while (true) {
626+
if (closed || Thread.currentThread().isInterrupted()) {
627+
// Unconditional exits: still drop ownership so a later re-arm
628+
// on a live pool can spawn a successor (the revive gate
629+
// ignores a closing pool anyway).
630+
releaseRecoveryDriverOwnership(false);
631+
return;
632+
}
633+
if (recoveryComplete) {
634+
// Transactional exit: re-check the latch under `lock` so an
635+
// un-latch (recoverRetiredSlotAt's re-arm) cannot slip
636+
// between this thread's decision to exit and its ownership
637+
// drop -- the lost-wakeup race a Thread.isAlive() probe had.
638+
if (releaseRecoveryDriverOwnership(true)) {
639+
return;
640+
}
641+
// An un-latch landed first: this thread stays the driver.
642+
continue;
643+
}
613644
boolean hasImmediateWork;
614645
try {
615646
hasImmediateWork = runStartupRecoveryStep();
@@ -618,18 +649,37 @@ private void runStartupRecoveryLoop() {
618649
// delegate Error must not permanently kill its only driver.
619650
hasImmediateWork = false;
620651
}
621-
if (closed || recoveryComplete) {
622-
return;
623-
}
624-
if (!hasImmediateWork) {
652+
if (!hasImmediateWork && !closed && !recoveryComplete) {
625653
startupRecoveryWaiter.run();
626-
if (Thread.currentThread().isInterrupted()) {
627-
return;
628-
}
629654
}
630655
}
631656
}
632657

658+
/**
659+
* Drops this thread's private-driver ownership token, transactionally
660+
* against the re-arm path. With {@code recheckLatch}, an un-latch that
661+
* raced this driver's exit decision is detected under {@code lock} and
662+
* ownership is KEPT ({@code false} is returned; the caller keeps
663+
* driving). The producer ({@link #recoverRetiredSlotAt}) clears the
664+
* latch and reads {@code recoveryDriverRunning} under the same lock, so
665+
* exactly one of the two parties always owns the follow-up: either this
666+
* driver observes the cleared latch, or the producer observes the
667+
* dropped token and spawns a successor. No interleaving leaves an
668+
* un-latched scan ownerless.
669+
*/
670+
private boolean releaseRecoveryDriverOwnership(boolean recheckLatch) {
671+
lock.lock();
672+
try {
673+
if (recheckLatch && !closed && !recoveryComplete) {
674+
return false;
675+
}
676+
recoveryDriverRunning = false;
677+
return true;
678+
} finally {
679+
lock.unlock();
680+
}
681+
}
682+
633683
/**
634684
* Recovers at most ONE stranded managed slot and reports whether more remain.
635685
* The private direct driver or {@link PoolHousekeeper} drives steps
@@ -2246,28 +2296,28 @@ private void recoverRetiredSlotAt(int retiredIndex) {
22462296
* Deferred pools need nothing: {@code PoolHousekeeper} calls
22472297
* {@link #runStartupRecoveryStep()} on every tick and its gate re-opens
22482298
* 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.
2299+
* scan completes, so when no thread currently owns the driver duty
2300+
* ({@code recoveryDriverRunning} false), spawn a successor running the
2301+
* same loop -- it drains the backlog, re-latches, and exits; close()
2302+
* joins it via {@link #stopStartupRecoveryDriver}. Ownership is a
2303+
* lock-guarded token, not a {@code Thread.isAlive()} probe: the loop
2304+
* drops it transactionally ({@link #releaseRecoveryDriverOwnership}
2305+
* re-checks the latch under this same lock first), so a driver that is
2306+
* mid-exit either observes this caller's un-latch and keeps driving, or
2307+
* has already dropped the token and a successor is spawned here -- no
2308+
* interleaving leaves the un-latched scan ownerless. Caller must hold
2309+
* {@code lock}; {@code Thread.start()} is cheap enough to run under it.
2310+
* Best-effort: a spawn failure is logged and never propagates into the
2311+
* release callback that triggered the re-arm (the data stays durable
2312+
* for a restart).
22612313
*/
22622314
private void reviveDirectRecoveryDriverIfNeeded() {
22632315
if (deferStartupRecovery || closed) {
22642316
return;
22652317
}
2266-
if (startupRecoveryThread != null && startupRecoveryThread.isAlive()) {
2267-
return;
2268-
}
2269-
Thread revived = revivedStartupRecoveryThread;
2270-
if (revived != null && revived.isAlive()) {
2318+
if (recoveryDriverRunning) {
2319+
// A live driver's exit re-checks the latch under this lock, so
2320+
// it is guaranteed to observe the un-latch that brought us here.
22712321
return;
22722322
}
22732323
try {
@@ -2276,10 +2326,11 @@ private void reviveDirectRecoveryDriverIfNeeded() {
22762326
: SenderPool::createStartupRecoveryThread;
22772327
Thread t = factory.newThread(this::runStartupRecoveryLoop);
22782328
t.setDaemon(true);
2329+
recoveryDriverRunning = true;
22792330
revivedStartupRecoveryThread = t;
22802331
t.start();
22812332
} catch (Throwable e) {
2282-
revivedStartupRecoveryThread = null;
2333+
recoveryDriverRunning = false;
22832334
LOG.warn("could not revive the startup recovery driver ({}); the released slot's "
22842335
+ "data stays durable on disk for a later attempt or restart", e.toString());
22852336
}

0 commit comments

Comments
 (0)