Skip to content

Commit ac24044

Browse files
committed
fix(qwp): count a retired slot with stranded data as a recovery deferral
The in-range recovery scan skipped any reserved index as "live" without a deferral tick. A RETIRED index (wedged close() kept the slot flock) aliases into that branch: the scan could finish a cycle with zero deferrals and latch recoveryComplete while the retired slot's dir still held unacked durable data. From there nothing in-process ever drained it -- reprobeRetiredSlots() only restores capacity -- so the data waited for a restart or a lucky borrow of that exact index, which steady low load may never produce. Treat a retired index whose dir is still a candidate orphan as a deferral, the same rule as a CONTENDED park: the end-of-scan rewind keeps the cycle alive, and once the deferred cleanup releases the flock the scan reserves the freed index and drains it itself -- no borrow required. Data-inert (durable on disk; a restart already rescanned); this closes the drain- liveness gap for the life of the pool. The regression test drives the scan by hand: forge the wedged retire, walk the scan (pre-fix it latched complete right here), heal the flock, and assert the scan itself delivers the stranded data and only then completes.
1 parent 8722bf1 commit ac24044

2 files changed

Lines changed: 171 additions & 4 deletions

File tree

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

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,9 @@ public final class SenderPool implements AutoCloseable {
287287
// remaining slots, recoveryDeferredThisCycle records the park, and once
288288
// both passes finish with parked slots outstanding the cursors rewind for
289289
// another cycle on a later tick instead of latching recoveryComplete.
290+
// A RETIRED index whose dir still holds data is deferred the same way on
291+
// every walk (see the reserved-skip branch in recoverOneSlotStep) so the
292+
// latch can never strand a retired slot's data while the pool lives.
290293
// recoveryFailStreak/-Slot track consecutive failures on one candidate;
291294
// recoveryWarnedSlots dedups the per-slot WARNs so an indefinitely
292295
// retried slot logs once per failure episode, not once per retry. All of
@@ -649,7 +652,12 @@ boolean runStartupRecoveryStep() {
649652
* can neither target the dir being recovered nor over-allocate past
650653
* {@code maxSize}. Prewarmed/borrowed slots (already live, holding their
651654
* flock) are skipped, as are empty slots (a cheap directory probe); only a
652-
* slot that actually holds stranded data spends the step's single drain. The
655+
* slot that actually holds stranded data spends the step's single drain. A
656+
* RETIRED index (a wedged close() kept its flock; see {@link #reclaimSlot}
657+
* and the retire branch below) is likewise never drained in place, but
658+
* while its dir still holds data it counts as a deferral, so the scan keeps
659+
* cycling instead of latching {@code recoveryComplete} past stranded data
660+
* that only a restart or a lucky borrow of that index would ever deliver. The
653661
* out-of-range pass needs no reservation: those indices have no
654662
* {@code slotInUse} entry and are never allocated by borrow().
655663
* <p>
@@ -702,26 +710,49 @@ private boolean recoverOneSlotStep(long stepBudgetMillis) {
702710
return false;
703711
}
704712
int i = recoveryInRangeNext;
713+
String slotPath = sfDir + "/" + slotBaseId + "-" + i;
705714
// Reserve this index unless prewarm (or a concurrent borrow after
706715
// publication) already holds it live. Count the reservation in
707716
// recoveringSlots so the borrow() cap check cannot over-allocate
708717
// while this slot is held for recovery.
709718
boolean reserved;
719+
boolean retired = false;
710720
lock.lock();
711721
try {
712722
reserved = slotInUse[i];
713723
if (!reserved) {
714724
slotInUse[i] = true;
715725
recoveringSlots++;
726+
} else {
727+
retired = isRetiredSlotIndex(i);
716728
}
717729
} finally {
718730
lock.unlock();
719731
}
720732
if (reserved) {
733+
// A reserved index is normally LIVE (prewarm or a concurrent
734+
// borrow owns it) and is none of recovery's business. A RETIRED
735+
// index is different: its flock is held by this pool's own
736+
// wedged former delegate, and any unacked data in its dir is
737+
// exactly as stranded as a CONTENDED slot's. Without counting
738+
// it as a deferral the scan would latch recoveryComplete and
739+
// abandon that data until a restart or a lucky borrow of this
740+
// exact index -- which steady low load may never produce. Count
741+
// it -- the same rule as a CONTENDED park -- so the end-of-scan
742+
// rewind keeps the cycle alive; once the deferred engine
743+
// cleanup releases the flock, reprobeRetiredSlots()/the release
744+
// callback frees the index and a later cycle reserves and
745+
// drains it right here. The isCandidateOrphan probe (a few
746+
// syscalls, outside the lock) keeps an already-clean retired
747+
// dir from cycling the scan forever, and racing a concurrent
748+
// recover/borrow can only over-count -- costing one extra
749+
// cheap rewound walk, never a missed candidate.
750+
if (retired && OrphanScanner.isCandidateOrphan(slotPath)) {
751+
recoveryDeferredThisCycle++;
752+
}
721753
recoveryInRangeNext++;
722754
continue;
723755
}
724-
String slotPath = sfDir + "/" + slotBaseId + "-" + i;
725756
if (!OrphanScanner.isCandidateOrphan(slotPath)) {
726757
// No stranded data: release the reservation and keep scanning;
727758
// an empty slot must not cost a whole step.
@@ -868,8 +899,8 @@ private boolean recoverOneSlotStep(long stepBudgetMillis) {
868899
}
869900

870901
if (recoveryDeferredThisCycle > 0) {
871-
// At least one slot was parked this cycle (contended or
872-
// persistently failing). Its data stays durable on disk, so
902+
// At least one slot was parked this cycle (contended, persistently
903+
// failing, or retired with data still on disk). Its data stays durable on disk, so
873904
// instead of latching recoveryComplete -- which would abandon it
874905
// until a restart or a lucky borrow of that index -- rewind the
875906
// scan and let the driver's retry cadence run another cycle.
@@ -2073,6 +2104,24 @@ private void addRetiredSlot(SenderSlot s) {
20732104
retiredSlots.add(s);
20742105
}
20752106

2107+
/**
2108+
* Whether {@code idx} is currently held by a RETIRED slot (see
2109+
* {@link #reclaimSlot} and the in-range recovery retire branch) rather
2110+
* than a live one. Lets the recovery scan tell "reserved because
2111+
* borrowed/prewarmed" apart from "reserved because a wedged close() left
2112+
* the flock held" for its deferral accounting: only the latter's dir can
2113+
* hold stranded data no borrow is coming for. Caller must hold
2114+
* {@code lock}; retiredSlots is bounded by maxSize, so the walk is cheap.
2115+
*/
2116+
private boolean isRetiredSlotIndex(int idx) {
2117+
for (int i = 0, n = retiredSlots.size(); i < n; i++) {
2118+
if (retiredSlots.get(i).slotIndex() == idx) {
2119+
return true;
2120+
}
2121+
}
2122+
return false;
2123+
}
2124+
20762125
private void recoverRetiredSlotAt(int retiredIndex) {
20772126
SenderSlot s = retiredSlots.get(retiredIndex);
20782127
int last = retiredSlots.size() - 1;

core/src/test/java/io/questdb/client/test/impl/SenderPoolSfTest.java

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1753,6 +1753,124 @@ public void testStartupRetiredSlotRecoveredAfterLateFlockRelease() throws Except
17531753
});
17541754
}
17551755

1756+
@Test
1757+
public void testRecoveryScanStaysAliveWhileRetiredSlotHoldsStrandedData() throws Exception {
1758+
// Drain-liveness regression: a slot RETIRED mid-scan (recoverer close()
1759+
// returned with the flock still held) keeps slotInUse[i] set, so the
1760+
// next walk over its index took the "reserved => live" skip WITHOUT
1761+
// counting a deferral and latched recoveryComplete with the slot's
1762+
// unacked data still on disk. From there the data was stranded until a
1763+
// restart or a lucky borrow of that exact index -- which steady low
1764+
// load may never produce (testStartupRetiredSlotRecoveredAfterLate-
1765+
// FlockRelease passes only because its borrow IS that lucky borrow).
1766+
// The scan must instead treat a retired index whose dir is still a
1767+
// candidate orphan as a deferral -- the same rule as a CONTENDED park:
1768+
// the cycle keeps rewinding, and once the late flock release restores
1769+
// the index the SCAN itself drains the data, no borrow required. This
1770+
// test is RED (latches complete) until that deferral is counted.
1771+
TestUtils.assertMemoryLeak(() -> {
1772+
// Phase 1: strand unacked data under default-0.
1773+
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
1774+
int silentPort = silent.getPort();
1775+
silent.start();
1776+
Assert.assertTrue(silent.awaitStart(5, TimeUnit.SECONDS));
1777+
String cfg1 = "ws::addr=localhost:" + silentPort + ";sf_dir=" + sfDir
1778+
+ ";close_flush_timeout_millis=500;";
1779+
try (SenderPool pool = new SenderPool(cfg1, 1, 1, 1_000, Long.MAX_VALUE, Long.MAX_VALUE)) {
1780+
PooledSender s = pool.borrow();
1781+
for (int i = 0; i < 3; i++) {
1782+
s.table("recover").longColumn("v", i).atNow();
1783+
s.flush();
1784+
}
1785+
s.close();
1786+
}
1787+
}
1788+
Assert.assertTrue("unacked data must persist under default-0",
1789+
hasSegmentFile(slot("default-0")));
1790+
1791+
// Phase 2: ack-ing server; the FIRST recovery build of slot 0 is
1792+
// forged into the wedged shape (closed=true makes the recovery
1793+
// drain throw and close() a no-op, so the real flock stays held
1794+
// and slotLockReleased stays false) so the scan's own drain FAILs
1795+
// and the slot is retired. deferStartupRecovery=true keeps the
1796+
// scan entirely under this test's control -- no background driver.
1797+
CountingAckHandler handler = new CountingAckHandler();
1798+
try (TestWebSocketServer ack = new TestWebSocketServer(handler)) {
1799+
int ackPort = ack.getPort();
1800+
ack.start();
1801+
Assert.assertTrue(ack.awaitStart(5, TimeUnit.SECONDS));
1802+
String cfg2 = "ws::addr=localhost:" + ackPort + ";sf_dir=" + sfDir + ";";
1803+
1804+
AtomicReference<Sender> forged = new AtomicReference<>();
1805+
IntFunction<Sender> factory = idx -> {
1806+
Sender real = Sender.builder(cfg2).senderId("default-" + idx).build();
1807+
if (idx == 0 && forged.compareAndSet(null, real)) {
1808+
try {
1809+
((QwpWebSocketSender) real).setClosedForTesting(true);
1810+
} catch (Exception e) {
1811+
throw new RuntimeException(e);
1812+
}
1813+
}
1814+
return real;
1815+
};
1816+
1817+
try (SenderPool pool = new SenderPool(cfg2, 0, 1, 500,
1818+
Long.MAX_VALUE, Long.MAX_VALUE, factory, true)) {
1819+
// Step 1: builds the forged recoverer, fails its drain, and
1820+
// retires the slot (leakedSlots=1, slotInUse[0] stays set).
1821+
// The FAILED outcome stops the drive without advancing.
1822+
long deadline = System.currentTimeMillis() + 10_000;
1823+
while (pool.leakedSlotCount() == 0
1824+
&& System.currentTimeMillis() < deadline) {
1825+
pool.runStartupRecoveryStepForTesting();
1826+
}
1827+
Assert.assertNotNull("recovery must have built slot 0", forged.get());
1828+
Assert.assertEquals("scan must retire the wedged slot",
1829+
1, pool.leakedSlotCount());
1830+
1831+
// Re-drive the scan: the retired index is skipped as
1832+
// reserved. Pre-fix the first full walk latched
1833+
// recoveryComplete; post-fix the still-candidate retired
1834+
// dir counts as a deferral and the cycle keeps rewinding.
1835+
for (int k = 0; k < 8; k++) {
1836+
pool.runStartupRecoveryStepForTesting();
1837+
}
1838+
Assert.assertTrue("retired dir must still hold the stranded data",
1839+
hasSegmentFile(slot("default-0")));
1840+
Assert.assertFalse(
1841+
"scan must NOT latch recoveryComplete while a retired slot still "
1842+
+ "holds stranded durable data -- that abandons the data "
1843+
+ "until a restart or a lucky borrow of that exact index",
1844+
pool.isRecoveryCompleteForTesting());
1845+
1846+
// The late flock release: the "wedged worker" finishes
1847+
// (un-forge + real close). The housekeeper tick re-probes
1848+
// retiredSlots and frees the index; the still-alive scan
1849+
// must then reserve it and drain the stranded data ITSELF
1850+
// -- no borrow anywhere in this phase.
1851+
Sender recoverer = forged.get();
1852+
((QwpWebSocketSender) recoverer).setClosedForTesting(false);
1853+
recoverer.close();
1854+
pool.reapIdle();
1855+
Assert.assertEquals("late release must restore the retired capacity",
1856+
0, pool.leakedSlotCount());
1857+
1858+
deadline = System.currentTimeMillis() + 10_000;
1859+
while (!pool.isRecoveryCompleteForTesting()
1860+
&& System.currentTimeMillis() < deadline) {
1861+
pool.runStartupRecoveryStepForTesting();
1862+
}
1863+
Assert.assertTrue("scan must complete once the stranded data is drained",
1864+
pool.isRecoveryCompleteForTesting());
1865+
Assert.assertTrue("the scan itself must have drained the stranded data",
1866+
awaitNoSegmentFile(slot("default-0"), 15_000));
1867+
Assert.assertTrue("replayed frames must reach the server",
1868+
awaitAtLeast(handler.frames, 1, 15_000));
1869+
}
1870+
}
1871+
});
1872+
}
1873+
17561874
@Test
17571875
public void testZeroTimeoutBorrowProbesRetiredSlotBeforeThrowing() throws Exception {
17581876
// Boundary twin of testStartupRetiredSlotRecoveredAfterLateFlockRelease:

0 commit comments

Comments
 (0)