Skip to content

Commit 930175f

Browse files
committed
fix(qwp): transfer slot cleanup to the worker's exit path and recover retired pool slots
When an owned manager's bounded join timed out during engine close, the engine leaked ring + watermark mmaps and the slot flock until process exit, the sender latched slotLockReleased=false forever, and SenderPool retired the slot permanently (leakedSlots++) - a transiently-slow SF filesystem op at close time (> workerJoinTimeoutMillis, default 5 s) permanently ratcheted pool capacity down, even though the worker often exits moments later. - new SegmentManager.deferUntilWorkerExit(cleanup): hands an action to the worker-loop exit block, which runs strictly after the final service pass - the last point the worker can touch any slot path. Registration and the exit block's workerLoopExited flip share the manager lock, so the handoff is exactly-once: accepted while the worker is live, rejected (caller cleans up inline) once it exited - CursorSendEngine.close(): on an owned-manager join timeout, terminal cleanup (ring, watermark, drained-file unlink, flock release) now transfers to the worker's exit path instead of leaking; the quiescence gate is unchanged - the slot stays locked until the pass provably ends - exactly-once via a terminalCleanupClaimed CAS, deliberately not the engine monitor: a retried close() holds the monitor while joining the worker, and the worker cannot die until its cleanup returns - monitor-based exclusion would stall that close() for its full join budget. With the CAS the worker never blocks and the join returns as soon as the pass ends - QwpWebSocketSender.isSlotLockReleased() is now monotonic, not frozen: it re-probes the retained engine (volatile reads only, safe under the pool lock) and flips true once the deferred cleanup - worker exit path or delegated I/O-thread close - releases the flock - SenderPool re-probes retired slots (housekeeper reapIdle tick and capacity-starved borrows just before parking) and returns a recovered index to the free set: leakedSlots goes back down and a would-be borrow timeout becomes an immediate creation. A persistent non-zero leakedSlotCount() now means a genuinely wedged worker - shared-manager engines (test-only construction) keep the old leak-and-retry-close contract: their worker serves other rings and has no exit to defer to; startup-recovery retirements also stay permanent - regression: deferUntilWorkerExit contract test, owned-close handoff test (slot reacquirable after worker exit with NO close() retry), pool recovery via reapIdle and via capacity-starved borrow; the SlotLockReleasedContractTest leak-path pin updated to the new monotonic-getter contract
1 parent 7cf9612 commit 930175f

8 files changed

Lines changed: 697 additions & 111 deletions

File tree

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

Lines changed: 64 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -281,8 +281,17 @@ public class QwpWebSocketSender implements Sender {
281281
// teardown path). Stays false if an I/O or manager worker did not stop and
282282
// cursorEngine retained the flock, so the owning pool MUST keep the slot
283283
// reserved rather than hand the still-locked dir to the next borrow
284-
// ("sf slot already in use").
285-
private boolean slotLockReleased;
284+
// ("sf slot already in use"). May flip to true LATER via the getter's
285+
// re-probe of retainedEngine, once the deferred cleanup (manager-worker
286+
// exit path or delegated I/O-thread close) releases the flock — pools
287+
// re-probe retired slots to recover their capacity. volatile: written on
288+
// the closing thread, read by pool threads.
289+
private volatile boolean slotLockReleased;
290+
// Engine whose close() could not complete during sender close() — its
291+
// cleanup is pending on a worker/I/O-thread exit path. isSlotLockReleased()
292+
// re-probes it so a late flock release becomes visible to the owning pool.
293+
// Only ever set inside close(); null for a sender that closed cleanly.
294+
private volatile CursorSendEngine retainedEngine;
286295
private int pendingRowCount;
287296
private SenderProgressDispatcher progressDispatcher;
288297
// Async-delivery sink for ack-watermark advances. Default no-op; a
@@ -1190,24 +1199,33 @@ public void close() {
11901199
// close actually runs, so the pool must not reuse the slot
11911200
// meanwhile. A false return means the thread exited between
11921201
// the failed close() and now — then closing here is safe.
1193-
if (ownsCursorEngine && cursorEngine != null && cursorSendLoop != null
1194-
&& !cursorSendLoop.delegateEngineClose()) {
1195-
CursorSendEngine engine = cursorEngine;
1196-
try {
1197-
engine.close();
1198-
} catch (Throwable t) {
1199-
LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t));
1200-
terminalError = captureCloseError(terminalError, t);
1201-
}
1202-
if (engine.isCloseCompleted()) {
1203-
cursorEngine = null;
1204-
ownsCursorEngine = false;
1205-
slotLockReleased = true;
1202+
if (ownsCursorEngine && cursorEngine != null && cursorSendLoop != null) {
1203+
if (cursorSendLoop.delegateEngineClose()) {
1204+
// The I/O thread adopted the engine close and runs it on
1205+
// its exit path. Keep the engine visible for re-probing:
1206+
// isSlotLockReleased() flips true once that close
1207+
// completes, letting a pool recover the retired slot.
1208+
retainedEngine = cursorEngine;
12061209
} else {
1207-
// Preserve the engine and report the retained flock.
1208-
// Sender.close() is idempotent, so this is a deliberate
1209-
// safe leak until process exit rather than a retry path.
1210-
slotLockReleased = false;
1210+
CursorSendEngine engine = cursorEngine;
1211+
try {
1212+
engine.close();
1213+
} catch (Throwable t) {
1214+
LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t));
1215+
terminalError = captureCloseError(terminalError, t);
1216+
}
1217+
if (engine.isCloseCompleted()) {
1218+
cursorEngine = null;
1219+
ownsCursorEngine = false;
1220+
slotLockReleased = true;
1221+
} else {
1222+
// Engine cleanup is pending on its manager worker's
1223+
// exit path (or leaked, for a shared manager). Report
1224+
// the retained flock now; the getter re-probes the
1225+
// retained engine so a late release is not lost.
1226+
slotLockReleased = false;
1227+
retainedEngine = engine;
1228+
}
12111229
}
12121230
}
12131231
rethrowTerminal(terminalError);
@@ -1257,7 +1275,12 @@ public void close() {
12571275
// The manager worker did not quiesce. Preserve ownership
12581276
// and report the retained flock so pools retire this slot.
12591277
// Repeated Sender.close() calls remain no-ops by contract.
1278+
// Engine cleanup was handed to the worker's exit path
1279+
// (owned manager); the getter re-probes the retained
1280+
// engine so the pool can reclaim the slot once cleanup
1281+
// actually completes.
12601282
slotLockReleased = false;
1283+
retainedEngine = engine;
12611284
}
12621285
} else {
12631286
// This sender owns no cursor engine holding an SF flock.
@@ -1306,13 +1329,30 @@ public void close() {
13061329
}
13071330

13081331
/**
1309-
* True once {@link #close()} has released the store-and-forward slot
1310-
* flock. False means an I/O or manager worker did not stop and close()
1311-
* retained the lock and worker-reachable resources; the owning pool must
1312-
* keep the slot index reserved instead of reusing the still-locked dir.
1332+
* True once the store-and-forward slot flock has been released. False
1333+
* means an I/O or manager worker did not stop and close() retained the
1334+
* lock and worker-reachable resources; the owning pool must keep the slot
1335+
* index reserved instead of reusing the still-locked dir.
1336+
* <p>
1337+
* Not a one-shot snapshot: when close() left engine cleanup pending on a
1338+
* worker/I/O-thread exit path, this re-probes the retained engine and
1339+
* latches true the moment that cleanup completes — pools re-probe retired
1340+
* slots through this getter to recover their capacity. Monotonic:
1341+
* false→true only, never back. Lock-free (volatile reads) so pools may
1342+
* call it under their capacity lock.
13131343
*/
13141344
public boolean isSlotLockReleased() {
1315-
return slotLockReleased;
1345+
if (slotLockReleased) {
1346+
return true;
1347+
}
1348+
CursorSendEngine engine = retainedEngine;
1349+
if (engine != null && engine.isCloseCompleted()) {
1350+
// Benign latch race: concurrent callers may both observe the
1351+
// completed cleanup and both write true.
1352+
slotLockReleased = true;
1353+
return true;
1354+
}
1355+
return false;
13161356
}
13171357

13181358
@Override

0 commit comments

Comments
 (0)