Skip to content

Commit c851317

Browse files
committed
fix(pool): force drain_orphans=off on SF startup-recovery delegates
A startup-recovery delegate is built from the shared config string, so with drain_orphans=on it inherited the flag. Since recovery forces initial_connect_mode=OFF (one connect attempt), against a reachable server the build succeeds and reaches startOrphanDrainers(), standing up a BackgroundDrainerPool for any foreign/out-of-range orphan. Its close() -- called from drainCandidateSlotForRecovery() on the PoolHousekeeper thread, BEFORE cursorEngine.close() releases the slot flock -- then blocks up to GRACEFUL_DRAIN_MILLIS + STOP_GRACE_MILLIS (3s) against a reachable-but-not-acking server. That makes one recovery step ~1s drain + ~3s drainerPool.close ~= 4s, overrunning RECOVERY_DRAIN_BUDGET_MILLIS and PoolHousekeeper.STOP_TIMEOUT_MILLIS (2s); a close() landing mid-step then returns with the flock still held, resurfacing the "sf slot already in use" window this PR exists to eliminate. Recovery delegates now force drain_orphans=off: their sole job is to drain their own slot. Sibling in-range slots are covered by recoverOneSlotStep's own passes; foreign/out-of-range orphans are covered by the live pooled senders' drainers (whose close() senderPool.close() awaits synchronously, so they release their flock before close() returns). Adds testRecoveryStepStaysBoundedWithDrainOrphansAgainstNonAckingServer: seeds an in-range stranded slot + a foreign orphan, restarts with drain_orphans=on against a reachable-but-silent server, drives one recovery step, and asserts it returns within STOP_TIMEOUT_MILLIS with the foreign data preserved (not lost, not .failed). Fails pre-fix (~3.5s), passes post-fix (~1s).
1 parent db44da8 commit c851317

2 files changed

Lines changed: 120 additions & 0 deletions

File tree

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,12 @@ private Sender defaultSender(int slotIndex) {
10241024
* keeping a recovery step bounded below
10251025
* {@code PoolHousekeeper.STOP_TIMEOUT_MILLIS}. See M1 / the residual-window
10261026
* note on {@link #recoverOneSlotStep}.
1027+
* <p>
1028+
* Also forces {@code drain_orphans=off} (see
1029+
* {@link #buildManagedSlotSender}): a recovery delegate must never spin up a
1030+
* BackgroundDrainerPool, whose {@code close()} could block ~3s and overrun
1031+
* the step / {@code STOP_TIMEOUT_MILLIS} budget while still holding the slot
1032+
* flock.
10271033
*/
10281034
private Sender defaultRecoverySender(int slotIndex) {
10291035
return buildManagedSlotSender(slotIndex, true);
@@ -1063,6 +1069,27 @@ private Sender buildManagedSlotSender(int slotIndex, boolean forRecovery) {
10631069
// the SYNC auto-promotion the user's reconnect_* knobs would
10641070
// otherwise trigger.
10651071
builder.initialConnectMode(Sender.InitialConnectMode.OFF);
1072+
// Force drain_orphans OFF on recovery delegates regardless of the
1073+
// shared config string. A recovery delegate's sole job is to drain
1074+
// its OWN slot (the one recoverOneSlotStep is processing); it must
1075+
// never start a BackgroundDrainerPool for sibling/foreign orphans.
1076+
// If it did, the delegate's close() -- called from
1077+
// drainCandidateSlotForRecovery() on the PoolHousekeeper thread,
1078+
// BEFORE its cursorEngine.close() releases the slot flock -- would
1079+
// block in BackgroundDrainerPool.close() for up to
1080+
// GRACEFUL_DRAIN_MILLIS + STOP_GRACE_MILLIS (3s) against a
1081+
// reachable-but-not-acking server. That overruns a recovery step's
1082+
// budget (RECOVERY_DRAIN_BUDGET_MILLIS) and PoolHousekeeper
1083+
// .STOP_TIMEOUT_MILLIS, so a close() landing mid-step times out its
1084+
// join and returns while the recoverer still holds the slot flock
1085+
// -- resurrecting the "sf slot already in use" window this pool's
1086+
// per-slot ids exist to eliminate. Sibling in-range slots are
1087+
// covered by recoverOneSlotStep's own passes; foreign/out-of-range
1088+
// orphans are covered by the LIVE pooled senders' drainers (which
1089+
// keep drain_orphans=on and whose close() senderPool.close() awaits
1090+
// synchronously, so they release their flock before close()
1091+
// returns).
1092+
builder.drainOrphans(false);
10661093
}
10671094
return builder.build();
10681095
}

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1452,6 +1452,99 @@ public void testStartupRecoveryIsBoundedByASharedBudget() throws Exception {
14521452
});
14531453
}
14541454

1455+
@Test
1456+
public void testRecoveryStepStaysBoundedWithDrainOrphansAgainstNonAckingServer() throws Exception {
1457+
// M-A regression: a startup-recovery delegate must NOT inherit
1458+
// drain_orphans=on. If it did, building the recoverer (which connects
1459+
// OK against a reachable server because initial_connect_mode is forced
1460+
// OFF) would run an orphan scan and dispatch a BackgroundDrainerPool at
1461+
// any foreign/out-of-range orphan. Against a reachable-but-not-acking
1462+
// server those background drainers never reach their target, so the
1463+
// recoverer's close() -- called inside the recovery step, on the
1464+
// housekeeper thread, BEFORE cursorEngine.close() releases the slot
1465+
// flock -- blocks in BackgroundDrainerPool.close() for ~3s
1466+
// (GRACEFUL_DRAIN_MILLIS + STOP_GRACE_MILLIS). That makes one step
1467+
// ~1s drain + ~3s drainerPool.close ~= 4s, far past
1468+
// RECOVERY_DRAIN_BUDGET_MILLIS and PoolHousekeeper.STOP_TIMEOUT_MILLIS
1469+
// (2s) -- and a close() landing mid-step would return with the flock
1470+
// still held. After the fix recovery delegates force drain_orphans=off,
1471+
// so no drainer pool is created and the step stays bounded by the drain
1472+
// budget alone.
1473+
final long acquireTimeoutMillis = 1_000L;
1474+
final int maxSize = 2;
1475+
1476+
TestUtils.assertMemoryLeak(() -> {
1477+
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
1478+
int silentPort = silent.getPort();
1479+
silent.start();
1480+
Assert.assertTrue(silent.awaitStart(5, TimeUnit.SECONDS));
1481+
String addr = "localhost:" + silentPort;
1482+
1483+
// Phase 1a: strand unacked data in an in-range managed slot
1484+
// (default-0) so the recovery step has a slot to process.
1485+
String seedCfg = "ws::addr=" + addr + ";sf_dir=" + sfDir
1486+
+ ";close_flush_timeout_millis=0;";
1487+
try (SenderPool seed = new SenderPool(seedCfg, 1, maxSize, 1_000, Long.MAX_VALUE, Long.MAX_VALUE)) {
1488+
PooledSender s = seed.borrow();
1489+
s.table("recover").longColumn("v", 1L).atNow();
1490+
s.flush();
1491+
s.close();
1492+
}
1493+
Assert.assertTrue("default-0 must hold unacked data", hasSegmentFile(slot("default-0")));
1494+
1495+
// Phase 1b: strand a FOREIGN orphan (different base) so a
1496+
// recovery delegate that inherited drain_orphans=on would
1497+
// dispatch a background drainer at it.
1498+
String ghostCfg = "ws::addr=" + addr + ";sf_dir=" + sfDir
1499+
+ ";sender_id=legacy;close_flush_timeout_millis=0;";
1500+
try (Sender ghost = Sender.fromConfig(ghostCfg)) {
1501+
for (int i = 0; i < 3; i++) {
1502+
ghost.table("foreign").longColumn("v", i).atNow();
1503+
ghost.flush();
1504+
}
1505+
} catch (Exception ignored) {
1506+
// best-effort: we only need the unacked .sfa on disk
1507+
}
1508+
Assert.assertTrue("foreign leftover must hold unacked data", hasSegmentFile(slot("legacy")));
1509+
1510+
// Phase 2: a deferred drain_orphans=on pool against the SAME
1511+
// reachable-but-not-acking server. Drive ONE recovery step and
1512+
// time it: the step builds a recovery delegate on default-0,
1513+
// which pre-fix would dispatch a drainer at the foreign orphan
1514+
// and then block ~3s in drainerPool.close().
1515+
String cfg = "ws::addr=" + addr + ";sf_dir=" + sfDir
1516+
+ ";drain_orphans=on;close_flush_timeout_millis=0;";
1517+
SenderPool pool = newDeferredPool(cfg, 0, maxSize, acquireTimeoutMillis);
1518+
try {
1519+
long startNanos = System.nanoTime();
1520+
invokeRunStartupRecoveryStep(pool);
1521+
long stepMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos);
1522+
1523+
// Headline guarantee: a recovery delegate must not stand up a
1524+
// BackgroundDrainerPool, so the step is bounded by the drain
1525+
// budget (~1s) -- comfortably under STOP_TIMEOUT_MILLIS. The
1526+
// 2.5s ceiling clears the ~1s post-fix cost with CI margin
1527+
// while decisively failing the pre-fix ~4s overrun.
1528+
Assert.assertTrue(
1529+
"recovery step must stay bounded with drain_orphans=on against a "
1530+
+ "non-acking server (no BackgroundDrainerPool overrun): took "
1531+
+ stepMillis + "ms",
1532+
stepMillis < 2_500L);
1533+
1534+
// Durability, not loss: the foreign orphan stays on disk for
1535+
// a later live-sender drainer; the recovery delegate must
1536+
// not have abandoned it as .failed either.
1537+
Assert.assertTrue("foreign orphan data must be preserved on disk, not lost",
1538+
hasSegmentFile(slot("legacy")));
1539+
Assert.assertFalse("foreign orphan must not be flagged .failed by a recovery delegate",
1540+
Files.exists(slot("legacy") + "/" + OrphanScanner.FAILED_SENTINEL_NAME));
1541+
} finally {
1542+
pool.close();
1543+
}
1544+
}
1545+
});
1546+
}
1547+
14551548
@Test
14561549
public void testDeferredStartupRecoveryDoesNotBlockConstruction() throws Exception {
14571550
// M2 fix: when recovery is deferred (the pooled QuestDB handle's path),

0 commit comments

Comments
 (0)