Skip to content

Commit cad1bb8

Browse files
committed
fix(qwp): make close-time SF cleanup crash-safe and bound flock-release retries
Close-time segment cleanup (review finding: failed cleanup published as reusable slot): - finishClose now persists the final acked FSN through the still-mapped watermark before closing the ring and before any unlink, so any cleanup failure or crash leaves the residue covered by a current watermark - unlinkAllSegmentFiles enumerates fully first, aborts with no unlinks on a failed directory walk, removes segments in ascending generation order and stops at the first failure, so residue is always a contiguous top slice that recovery seeds as fully acked (no replay, no duplicates) - the ack watermark is removed only after every segment is confirmed gone Flock-release retry driver (review finding: unbounded retry threads): - the shared retry driver now backs off exponentially per fully-failed round (100ms base, 5s cap), resets on progress, and is unparked when a fresh engine enqueues - after an injected driver-start failure, pool probes re-arm the retry via ensureFlockReleaseRetryScheduled() from isSlotLockReleased(), so retained capacity recovers without a second explicit close() New regression tests: CursorSendEngineCloseUnlinkFailureTest (close-time unlink fault injection; successor must not see replayable frames) and three FlockReleaseRetryDriverTest cases (backoff schedule, reset-on-progress, pool-probe recovery after start failure).
1 parent a086575 commit cad1bb8

5 files changed

Lines changed: 661 additions & 32 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1357,19 +1357,29 @@ public void close() {
13571357
* manager-worker quiescence or I/O-thread exit path, this re-probes the
13581358
* retained engine and latches true the moment that cleanup completes — pools re-probe retired
13591359
* slots through this getter to recover their capacity. Monotonic:
1360-
* false→true only, never back. Lock-free (volatile reads) so pools may
1361-
* call it under their capacity lock.
1360+
* false→true only, never back. Cheap (volatile reads on every common
1361+
* path) so pools may call it under their capacity lock; only the rare
1362+
* orphaned-retry state below does more.
1363+
* <p>
1364+
* The probe is also the recovery surface for a retained engine whose
1365+
* flock-release retry fell off the shared driver because the driver
1366+
* thread could not start (e.g. OOM at thread creation): close() is
1367+
* one-shot, so without the re-arm below that slot's capacity would stay
1368+
* lost until process exit.
13621369
*/
13631370
public boolean isSlotLockReleased() {
13641371
if (slotLockReleased) {
13651372
return true;
13661373
}
13671374
CursorSendEngine engine = retainedEngine;
1368-
if (engine != null && engine.isCloseCompleted()) {
1369-
// Benign latch race: concurrent callers may both observe the
1370-
// completed cleanup and both write true.
1371-
slotLockReleased = true;
1372-
return true;
1375+
if (engine != null) {
1376+
if (engine.isCloseCompleted()) {
1377+
// Benign latch race: concurrent callers may both observe the
1378+
// completed cleanup and both write true.
1379+
slotLockReleased = true;
1380+
return true;
1381+
}
1382+
engine.ensureFlockReleaseRetryScheduled();
13731383
}
13741384
return false;
13751385
}

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

Lines changed: 170 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,11 @@
3131
import org.jetbrains.annotations.TestOnly;
3232

3333
import java.util.ArrayDeque;
34+
import java.util.ArrayList;
3435
import java.util.concurrent.ThreadFactory;
3536
import java.util.concurrent.atomic.AtomicBoolean;
3637
import java.util.concurrent.locks.LockSupport;
38+
import java.util.function.LongConsumer;
3739

3840
/**
3941
* Facade that bundles a {@link SegmentRing} with a {@link SegmentManager} and
@@ -69,10 +71,13 @@ public final class CursorSendEngine implements QuietCloseable {
6971
org.slf4j.LoggerFactory.getLogger(CursorSendEngine.class);
7072
private static final ThreadFactory DEFAULT_FLOCK_RELEASE_RETRY_THREAD_FACTORY =
7173
runnable -> new Thread(runnable, "qdb-sf-flock-release-retry");
74+
private static final long FLOCK_RELEASE_RETRY_BASE_PARK_NANOS = 100_000_000L; // 100 ms
7275
private static final Object FLOCK_RELEASE_RETRY_LOCK = new Object();
76+
private static final long FLOCK_RELEASE_RETRY_MAX_PARK_NANOS = 5_000_000_000L; // 5 s
7377
private static final ArrayDeque<CursorSendEngine> FLOCK_RELEASE_RETRY_QUEUE = new ArrayDeque<>();
7478
private static volatile Runnable afterFlockReleaseRetryFailureHook;
7579
private static volatile Runnable beforeDeferredCloseCreationHook;
80+
private static volatile LongConsumer flockReleaseRetryParkOverride;
7681
private static Thread flockReleaseRetryThread;
7782
private static volatile ThreadFactory flockReleaseRetryThreadFactory =
7883
DEFAULT_FLOCK_RELEASE_RETRY_THREAD_FACTORY;
@@ -742,6 +747,27 @@ public synchronized void close() {
742747
*/
743748
private void finishClose(boolean fullyDrained) {
744749
try {
750+
// On a fully-drained close, persist the final acked FSN through
751+
// the still-mapped watermark BEFORE closing the ring/watermark
752+
// and BEFORE unlinking any segment file. The manager persists
753+
// the watermark only on its own tick, so it may lag the final
754+
// ack. If the unlink below then fails (or the process dies
755+
// mid-unlink), residual acknowledged .sfa files without an
756+
// up-to-date watermark would seed the successor's recovery at
757+
// lowestBase - 1 and replay already-acknowledged rows --
758+
// duplicates on a non-DEDUP table. The write is a single mmap
759+
// store, so it succeeds even when the unlink is about to fail
760+
// (e.g. the slot dir turned read-only). Quiescence is already
761+
// established here, so no manager tick can race this write.
762+
if (fullyDrained && watermark != null) {
763+
try {
764+
long finalAckedFsn = ring.ackedFsn();
765+
if (finalAckedFsn >= 0) {
766+
watermark.write(finalAckedFsn);
767+
}
768+
} catch (Throwable ignored) {
769+
}
770+
}
745771
try {
746772
ring.close();
747773
} catch (Throwable ignored) {
@@ -761,13 +787,26 @@ private void finishClose(boolean fullyDrained) {
761787
}
762788
}
763789
if (fullyDrained) {
790+
boolean segmentsRemoved = false;
764791
try {
765-
unlinkAllSegmentFiles(sfDir);
792+
segmentsRemoved = unlinkAllSegmentFiles(sfDir);
766793
} catch (Throwable ignored) {
767794
}
768-
try {
769-
AckWatermark.removeOrphan(sfDir);
770-
} catch (Throwable ignored) {
795+
// Remove the watermark ONLY once every segment file is
796+
// confirmed gone. The watermark is what keeps residual
797+
// acknowledged segments inert to a successor's recovery;
798+
// removing it while any .sfa file survives would republish
799+
// those already-acknowledged rows.
800+
if (segmentsRemoved) {
801+
try {
802+
AckWatermark.removeOrphan(sfDir);
803+
} catch (Throwable ignored) {
804+
}
805+
} else {
806+
LOG.warn("close-time segment cleanup incomplete on slot {}; retaining the ack "
807+
+ "watermark so residual acknowledged segments stay covered -- the next "
808+
+ "engine on this slot recovers them as fully acked and retries the "
809+
+ "unlink on its own close", sfDir);
771810
}
772811
}
773812
} finally {
@@ -813,6 +852,17 @@ public static void setBeforeDeferredCloseCreationHook(Runnable hook) {
813852
beforeDeferredCloseCreationHook = hook;
814853
}
815854

855+
/**
856+
* Replaces the shared retry driver's inter-round park with a callback
857+
* receiving the park duration the driver would have used. Test-only:
858+
* makes the retry cadence inspectable and rounds coordinatable without
859+
* wall-clock waits.
860+
*/
861+
@TestOnly
862+
public static void setFlockReleaseRetryParkOverride(LongConsumer override) {
863+
flockReleaseRetryParkOverride = override;
864+
}
865+
816866
/**
817867
* Overrides creation of the single shared flock-release retry thread.
818868
* Test-only: makes thread creation/start failure and persistent retry
@@ -903,6 +953,11 @@ private boolean retryFlockReleaseIfReady() {
903953
}
904954

905955
private static void runFlockReleaseRetryDriver() {
956+
// Capped exponential backoff: a persistent unlock failure must not
957+
// burn a fixed 10 rounds of native release syscalls per second
958+
// forever, but a transient failure must still recover promptly. The
959+
// ramp doubles per fully-failed round from 100 ms up to 5 s.
960+
long parkNanos = FLOCK_RELEASE_RETRY_BASE_PARK_NANOS;
906961
while (true) {
907962
final int roundSize;
908963
synchronized (FLOCK_RELEASE_RETRY_LOCK) {
@@ -913,13 +968,15 @@ private static void runFlockReleaseRetryDriver() {
913968
}
914969
}
915970
boolean hasFailures = false;
971+
boolean hasSuccesses = false;
916972
for (int i = 0; i < roundSize; i++) {
917973
final CursorSendEngine engine;
918974
synchronized (FLOCK_RELEASE_RETRY_LOCK) {
919975
engine = FLOCK_RELEASE_RETRY_QUEUE.pollFirst();
920976
}
921977
if (engine.retryFlockReleaseIfReady()) {
922978
engine.flockReleaseRetryStarted.set(false);
979+
hasSuccesses = true;
923980
} else {
924981
synchronized (FLOCK_RELEASE_RETRY_LOCK) {
925982
FLOCK_RELEASE_RETRY_QUEUE.addLast(engine);
@@ -931,11 +988,22 @@ private static void runFlockReleaseRetryDriver() {
931988
}
932989
}
933990
}
991+
if (hasSuccesses) {
992+
// Progress: the failure condition is clearing, so retry the
993+
// remaining engines on the base cadence again.
994+
parkNanos = FLOCK_RELEASE_RETRY_BASE_PARK_NANOS;
995+
}
934996
if (hasFailures) {
935997
// Interruption must not abandon a retained flock, but clear
936998
// the flag so subsequent parks still throttle retries.
937999
Thread.interrupted();
938-
LockSupport.parkNanos(100_000_000L);
1000+
LongConsumer parkOverride = flockReleaseRetryParkOverride;
1001+
if (parkOverride != null) {
1002+
parkOverride.accept(parkNanos);
1003+
} else {
1004+
LockSupport.parkNanos(parkNanos);
1005+
}
1006+
parkNanos = Math.min(parkNanos * 2, FLOCK_RELEASE_RETRY_MAX_PARK_NANOS);
9391007
}
9401008
}
9411009
}
@@ -947,7 +1015,12 @@ private void startFlockReleaseRetry() {
9471015
Throwable startFailure = null;
9481016
synchronized (FLOCK_RELEASE_RETRY_LOCK) {
9491017
FLOCK_RELEASE_RETRY_QUEUE.addLast(this);
950-
if (flockReleaseRetryThread == null) {
1018+
if (flockReleaseRetryThread != null) {
1019+
// The driver may be parked on a ramped backoff; wake it so a
1020+
// freshly failed release gets its first driver retry promptly
1021+
// instead of inheriting older engines' full backoff.
1022+
LockSupport.unpark(flockReleaseRetryThread);
1023+
} else {
9511024
try {
9521025
Thread retryThread = flockReleaseRetryThreadFactory.newThread(
9531026
CursorSendEngine::runFlockReleaseRetryDriver);
@@ -973,11 +1046,13 @@ private void startFlockReleaseRetry() {
9731046
+ "retired capacity recovers after the transient failure [slot={}]",
9741047
sfDir == null ? "<memory>" : sfDir);
9751048
} else {
976-
// A later explicit close() can still retry without repeating the
977-
// one-time ring/watermark cleanup. The failed queue is cleared so
978-
// the driver does not retain engines it cannot service.
979-
LOG.error("Could not start SF flock-release retry driver; close() may be "
980-
+ "invoked again to retry [slot={}, error={}]",
1049+
// A later explicit close() or a pool's retired-slot probe
1050+
// (ensureFlockReleaseRetryScheduled) can still retry without
1051+
// repeating the one-time ring/watermark cleanup. The failed queue
1052+
// is cleared so the driver does not retain engines it cannot
1053+
// service.
1054+
LOG.error("Could not start SF flock-release retry driver; a retried close() "
1055+
+ "or pool re-probe re-arms the retry [slot={}, error={}]",
9811056
sfDir == null ? "<memory>" : sfDir, String.valueOf(startFailure));
9821057
}
9831058
}
@@ -1015,6 +1090,25 @@ public void setSlotLockReleaseListener(Runnable listener) {
10151090
}
10161091
}
10171092

1093+
/**
1094+
* Re-arms the shared flock-release retry for an engine whose terminal
1095+
* cleanup finished but whose confirmed flock release is still pending
1096+
* and no longer scheduled — the retry driver thread failed to start when
1097+
* the release first failed (e.g. OOM at thread creation).
1098+
* {@code Sender.close()} is one-shot by contract, so pool probes
1099+
* ({@code QwpWebSocketSender.isSlotLockReleased()}) call this to keep a
1100+
* retired slot's capacity recoverable instead of lost until process
1101+
* exit. Cheap unless the engine is in that orphan state (volatile reads,
1102+
* then one failed CAS when the retry is already scheduled), so probes
1103+
* may call it under their capacity lock.
1104+
*/
1105+
public void ensureFlockReleaseRetryScheduled() {
1106+
if (closeCompleted || !terminalResourcesCleaned) {
1107+
return;
1108+
}
1109+
startFlockReleaseRetry();
1110+
}
1111+
10181112
/**
10191113
* Pass-through to {@link SegmentRing#findSegmentContaining(long)}.
10201114
*/
@@ -1112,37 +1206,91 @@ public long recoveredOrphanTipFsn() {
11121206
return recoveredOrphanTipFsn;
11131207
}
11141208

1209+
/**
1210+
* Ascending removal rank of a segment file name for
1211+
* {@link #unlinkAllSegmentFiles(String)}. {@code sf-initial.sfa} is
1212+
* always the fresh-start segment at baseSeq 0, so it ranks first.
1213+
* Spare files carry a monotonic generation ({@code sf-<gen:016x>.sfa})
1214+
* assigned in creation == rotation == baseSeq order, so the parsed
1215+
* generation ranks them. Anything else with the extension is not a
1216+
* live segment and ranks last.
1217+
*/
1218+
private static long segmentCleanupRank(String name) {
1219+
if ("sf-initial.sfa".equals(name)) {
1220+
return Long.MIN_VALUE;
1221+
}
1222+
if (name.length() == 23 && name.startsWith("sf-")) {
1223+
try {
1224+
return Long.parseUnsignedLong(name.substring(3, 19), 16);
1225+
} catch (NumberFormatException ignored) {
1226+
// fall through to the unrecognized-name rank
1227+
}
1228+
}
1229+
return Long.MAX_VALUE;
1230+
}
1231+
11151232
/**
11161233
* Unlinks every {@code .sfa} file under {@code dir}. Called only on
11171234
* clean shutdown when the ring confirms every published FSN has been
11181235
* acked — at that moment the slot has no recoverable work and the
11191236
* files are pure noise that would mislead the next sender's recovery.
1120-
* Best-effort: logs and continues on failures, since we're already on
1121-
* the close path.
1237+
* <p>
1238+
* Removal runs in ascending segment order and STOPS at the first
1239+
* failure, so whatever survives (a failure here, or a crash mid-loop)
1240+
* is always a contiguous top slice of the ring: recovery's
1241+
* FSN-contiguity check still passes, and the retained ack watermark
1242+
* (== the final acked FSN == the highest frame on disk) still covers
1243+
* every surviving frame, so a successor replays nothing.
1244+
*
1245+
* @return {@code true} only when enumeration succeeded and every
1246+
* {@code .sfa} file was confirmed removed — the caller keeps the ack
1247+
* watermark on {@code false} so residual acknowledged segments stay
1248+
* covered.
11221249
*/
1123-
private static void unlinkAllSegmentFiles(String dir) {
1124-
if (!io.questdb.client.std.Files.exists(dir)) return;
1250+
private static boolean unlinkAllSegmentFiles(String dir) {
1251+
if (!io.questdb.client.std.Files.exists(dir)) return true;
11251252
long find = io.questdb.client.std.Files.findFirst(dir);
11261253
if (find < 0) {
11271254
LOG.warn("close-time unlink could not enumerate {}; "
11281255
+ "any residual sf-*.sfa files will be picked up by the next recovery", dir);
1129-
return;
1256+
return false;
11301257
}
1131-
if (find == 0) return;
1258+
if (find == 0) return true;
1259+
ArrayList<String> names = new ArrayList<>();
1260+
int rc = 1;
11321261
try {
1133-
int rc = 1;
11341262
while (rc > 0) {
11351263
String name = io.questdb.client.std.Files.utf8ToString(
11361264
io.questdb.client.std.Files.findName(find));
11371265
rc = io.questdb.client.std.Files.findNext(find);
1138-
if (name == null || !name.endsWith(".sfa")) continue;
1139-
String path = dir + "/" + name;
1140-
if (!io.questdb.client.std.Files.remove(path)) {
1141-
LOG.warn("Failed to unlink fully-acked segment {} on close", path);
1266+
if (name != null && name.endsWith(".sfa")) {
1267+
names.add(name);
11421268
}
11431269
}
11441270
} finally {
11451271
io.questdb.client.std.Files.findClose(find);
11461272
}
1273+
if (rc < 0) {
1274+
// A partial listing must not drive any unlink: removing only the
1275+
// files we happened to see could delete the segment holding the
1276+
// highest frame while a lower one survives, leaving residual
1277+
// state the retained watermark can no longer vouch for.
1278+
LOG.warn("close-time unlink could not fully enumerate {}; "
1279+
+ "leaving all segment files for the next recovery", dir);
1280+
return false;
1281+
}
1282+
names.sort((a, b) -> {
1283+
int byRank = Long.compare(segmentCleanupRank(a), segmentCleanupRank(b));
1284+
return byRank != 0 ? byRank : a.compareTo(b);
1285+
});
1286+
for (int i = 0, n = names.size(); i < n; i++) {
1287+
String path = dir + "/" + names.get(i);
1288+
if (!io.questdb.client.std.Files.remove(path)) {
1289+
LOG.warn("Failed to unlink fully-acked segment {} on close; stopping so the "
1290+
+ "residual files stay a contiguous, watermark-covered range", path);
1291+
return false;
1292+
}
1293+
}
1294+
return true;
11471295
}
11481296
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1428,9 +1428,10 @@ private void recoverReleasedSlots() {
14281428
* run since the retire. Restores {@code leakedSlots} capacity and signals
14291429
* waiters so a parked borrow can admit a creation immediately.
14301430
* <p>
1431-
* Caller must hold {@code lock}. The probe is lock-free on the delegate
1432-
* side ({@code isSlotLockReleased()} reads volatiles only), so holding
1433-
* the pool lock across it cannot stall behind delegate teardown.
1431+
* Caller must hold {@code lock}. The probe is cheap on the delegate side
1432+
* ({@code isSlotLockReleased()} reads volatiles, and only re-arms the
1433+
* shared flock-release retry in the rare orphaned-retry state), so
1434+
* holding the pool lock across it cannot stall behind delegate teardown.
14341435
*
14351436
* @return {@code true} if at least one slot's capacity was recovered
14361437
*/

0 commit comments

Comments
 (0)