Skip to content

Commit 98b7397

Browse files
committed
fix(qwp): probe the slot flock O(1) before paying a recovery build on parked slots
A managed slot whose flock is held by another live owner is parked as CONTENDED and re-probed on every startup-recovery retry cycle -- every second on the direct pool's private driver, for as long as the owner lives. Each re-probe paid a full recovery build just to reach SlotLock.acquire and throw: two isCandidateOrphan dir enumerations, a complete config re-parse plus builder graph, parent-dir fsync barriers in periodic durability mode, and an owned SegmentManager allocation torn straight back down. Add SlotLock.probeHolder: a side-effect-light non-blocking flock probe that never creates dirs or files, never throws, and routes its momentary hold through the standard close() so an unconfirmed unlock can never leak from a probe. The recovery scan asks the probe first and parks on a held flock for a few syscalls; an indeterminate probe falls through to the full build, which keeps sole ownership of error classification. Races are benign both ways: a free probe can still lose the acquire (parks exactly as before) and a stale held probe is re-observed next cycle. New SenderPoolSfTest proves the arc with a real externally held flock: three parked cycles cost zero builds, and the first cycle after release recovers the slot with exactly one build.
1 parent fd424e3 commit 98b7397

3 files changed

Lines changed: 157 additions & 0 deletions

File tree

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,57 @@ public static SlotLock acquire(String slotDir, boolean syncParentDirectory) {
135135
}
136136
}
137137

138+
/**
139+
* Side-effect-light contention probe: reports the current holder of the
140+
* slot flock without creating the slot dir or lock file and without
141+
* paying a full engine build. Opens the existing {@code .lock} file
142+
* (absent means nothing can hold a flock on it), try-locks it
143+
* non-blocking, and releases immediately on success.
144+
* <p>
145+
* Returns a non-null holder description (the {@code .lock.pid} payload,
146+
* or {@code "unknown"}) when the flock is currently held by a live
147+
* owner; {@code null} when the lock is free or the probe could not
148+
* determine state (missing lock file, open failure). Callers must treat
149+
* {@code null} as "proceed to a full acquire", which owns real error
150+
* classification -- the probe never throws.
151+
* <p>
152+
* Races are benign in both directions: a free probe can still lose the
153+
* subsequent acquire to a concurrent owner (the caller handles that
154+
* contention exactly as before), and a held probe that goes stale the
155+
* moment the owner exits is simply re-observed on the caller's next
156+
* cycle. The probe's momentary hold can make a concurrent acquirer see
157+
* spurious contention -- the same class of race two real contenders
158+
* already have.
159+
*/
160+
public static String probeHolder(String slotDir) {
161+
if (slotDir == null || slotDir.isEmpty()) {
162+
return null;
163+
}
164+
// Same pre-step as acquire(): a lock retained by THIS process after
165+
// an unconfirmed unlock would otherwise read as a live holder for as
166+
// long as the retry list carries it.
167+
retryPendingReleases();
168+
String lockPath = slotDir + "/" + LOCK_FILE_NAME;
169+
if (!Files.exists(lockPath)) {
170+
return null;
171+
}
172+
int fd = Files.openRW(lockPath);
173+
if (fd < 0) {
174+
return null;
175+
}
176+
if (Files.lock(fd) != 0) {
177+
String holder = readHolder(slotDir + "/" + LOCK_PID_FILE_NAME);
178+
Files.close(fd);
179+
return holder;
180+
}
181+
// The flock was free and is momentarily ours. Route the release
182+
// through the standard close() so an unconfirmed unlock is retained
183+
// on the retry list exactly like a normal owner's -- a probe must
184+
// never leak a held flock.
185+
new SlotLock(slotDir, fd).close();
186+
return null;
187+
}
188+
138189
/**
139190
* Replaces the live descriptor with a known-dead value until the returned
140191
* guard closes. Test-only: exercises release retry paths without exposing

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import io.questdb.client.cutlass.qwp.client.QwpWebSocketSender;
3232
import io.questdb.client.cutlass.qwp.client.sf.cursor.BackgroundDrainerListener;
3333
import io.questdb.client.cutlass.qwp.client.sf.cursor.OrphanScanner;
34+
import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLock;
3435
import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLockContentionException;
3536
import io.questdb.client.std.Files;
3637
import io.questdb.client.std.IntList;
@@ -934,6 +935,29 @@ private RecoveryDrainOutcome drainCandidateSlotForRecovery(int slotIndex, String
934935
if (!OrphanScanner.isCandidateOrphan(slotPath)) {
935936
return RecoveryDrainOutcome.DRAINED;
936937
}
938+
// O(1) contention pre-probe (flock only). A slot parked as
939+
// CONTENDED is re-probed on every retry cycle for as long as its
940+
// live owner runs -- potentially that owner's whole lifetime --
941+
// and the full recovery build below (config re-parse, builder
942+
// graph, parent-dir fsync barriers in periodic durability, owned
943+
// SegmentManager allocation) would exist only to reach
944+
// SlotLock.acquire and throw. Ask the flock directly first so
945+
// the steady-state cost of a held slot is a few syscalls per
946+
// cycle, not a build. Races are benign in both directions: a
947+
// free probe can still lose the acquire inside the build (the
948+
// contention catch below parks exactly as before), and a held
949+
// probe that goes stale is re-observed on the next cycle. An
950+
// indeterminate probe (null) falls through to the build, which
951+
// owns real error classification.
952+
String probedHolder = SlotLock.probeHolder(slotPath);
953+
if (probedHolder != null) {
954+
if (warnSlotOnce(slotIndex)) {
955+
LOG.warn("startup SF recovery: slot {} is held by another live owner (holder={}); "
956+
+ "parking it and continuing with the remaining slots",
957+
slotPath, probedHolder);
958+
}
959+
return RecoveryDrainOutcome.CONTENDED;
960+
}
937961
try {
938962
// Recovery delegate: forced OFF-mode initial connect (see
939963
// createRecoverer / defaultRecoverySender), so this build does

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@
3636
import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentManager;
3737
import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLock;
3838
import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLockContentionException;
39+
import io.questdb.client.std.MemoryTag;
40+
import io.questdb.client.std.Unsafe;
3941
import io.questdb.client.impl.PooledSender;
4042
import io.questdb.client.impl.SenderPool;
4143
import io.questdb.client.std.Files;
@@ -3840,6 +3842,86 @@ private static boolean awaitAtLeast(AtomicInteger counter, int target, long time
38403842
return counter.get() >= target;
38413843
}
38423844

3845+
@Test
3846+
public void testContendedSlotReprobeUsesFlockProbeNotFullBuild() throws Exception {
3847+
// A slot whose flock is held by another LIVE owner is parked and
3848+
// re-probed on every retry cycle -- potentially for the owner's whole
3849+
// lifetime. The re-probe must ask the flock directly (O(1) probe,
3850+
// a few syscalls), not pay a full recovery build (config re-parse,
3851+
// builder graph, parent-dir fsync barriers in periodic durability,
3852+
// owned SegmentManager allocation) per cycle just to reach
3853+
// SlotLock.acquire and throw.
3854+
TestUtils.assertMemoryLeak(() -> {
3855+
String config = "ws::addr=localhost:1;sf_dir=" + sfDir + ";";
3856+
String slot0 = slot("default-0");
3857+
3858+
// Seed one unacked frame so slot 0 is a candidate orphan. The
3859+
// group root normally comes from Sender.build(); this test seeds
3860+
// the slot directly, so create the parent first (mkdir in
3861+
// SlotLock.acquire is non-recursive).
3862+
Assert.assertEquals(0, Files.mkdir(sfDir, Files.DIR_MODE_DEFAULT));
3863+
long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT);
3864+
try (CursorSendEngine seed = new CursorSendEngine(slot0, 1 << 20)) {
3865+
Unsafe.getUnsafe().setMemory(buf, 16, (byte) 1);
3866+
Assert.assertEquals(0L, seed.appendBlocking(buf, 16));
3867+
} finally {
3868+
Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT);
3869+
}
3870+
Assert.assertTrue("seeded slot must be a candidate orphan",
3871+
OrphanScanner.isCandidateOrphan(slot0));
3872+
3873+
AtomicInteger builds = new AtomicInteger();
3874+
IntFunction<Sender> factory = idx -> {
3875+
builds.incrementAndGet();
3876+
// Fidelity with the production build: the recovery build's
3877+
// fate is decided by the real slot flock, exactly like
3878+
// defaultRecoverySender's engine construction.
3879+
SlotLock lock = SlotLock.acquire(slot("default-" + idx));
3880+
return (Sender) Proxy.newProxyInstance(
3881+
Sender.class.getClassLoader(),
3882+
new Class[]{Sender.class},
3883+
(proxy, method, args) -> {
3884+
if ("drain".equals(method.getName())) {
3885+
return Boolean.TRUE;
3886+
}
3887+
if ("close".equals(method.getName())) {
3888+
lock.close();
3889+
return null;
3890+
}
3891+
throw new AssertionError(
3892+
"unexpected recovery sender call: " + method.getName());
3893+
});
3894+
};
3895+
3896+
SlotLock held = SlotLock.acquire(slot0);
3897+
try (SenderPool pool = newDeferredPoolWithFactory(config, 0, 1, 5_000, factory)) {
3898+
// Steady-state re-probe of a parked contended slot: three
3899+
// full cycles while the flock is held.
3900+
for (int cycle = 0; cycle < 3; cycle++) {
3901+
Assert.assertFalse("a cycle with only a contended slot must defer",
3902+
pool.runStartupRecoveryStepForTesting());
3903+
}
3904+
Assert.assertEquals("re-probing a contended slot must be a flock probe, "
3905+
+ "not a full recovery build per cycle", 0, builds.get());
3906+
Assert.assertTrue("parked slot must keep its durable data",
3907+
hasSegmentFile(slot0));
3908+
3909+
// The probe must not dampen recovery: once the owner lets
3910+
// go, the very next cycle pays exactly one real build and
3911+
// drains the slot.
3912+
held.close();
3913+
Assert.assertTrue("released slot must be recovered on the next cycle",
3914+
pool.runStartupRecoveryStepForTesting());
3915+
Assert.assertEquals("released slot must be recovered with exactly one build",
3916+
1, builds.get());
3917+
Assert.assertFalse("final scan step must mark recovery complete",
3918+
pool.runStartupRecoveryStepForTesting());
3919+
} finally {
3920+
held.close(); // idempotent when already released
3921+
}
3922+
});
3923+
}
3924+
38433925
private static boolean awaitNoSegmentFile(String slotPath, long timeoutMillis)
38443926
throws InterruptedException {
38453927
long deadline = System.currentTimeMillis() + timeoutMillis;

0 commit comments

Comments
 (0)