Skip to content

Commit 31066e1

Browse files
committed
fix(qwp): free native path-scratch sink when engine/manager construction fails
Two constructor-throw paths leaked SegmentManager's 256-byte native pathScratch sink (NATIVE_DIRECT_UTF8_SINK) per failed construction: - CursorSendEngine: the delegating constructors evaluate new SegmentManager(...) BEFORE the private ctor body runs, so a pre-try throw (slot lock collision -- 'already in use', or empty sfDir) orphaned the owned manager. A submit/close race in the drainer pool leaked 1546 sinks (395,776 bytes) in a single test run; any client retrying fromConfig against a locked slot leaks 256 bytes per attempt. The pre-try section now closes an owned manager before propagating, and the main catch closes it regardless of whether start() was reached (close on an unstarted manager is safe -- no worker to join, just frees the scratch). - SegmentManager: the pathScratch field initializer has already allocated by the time the ctor's validation throws fire (segmentSizeBytes/maxTotalBytes too small). Both throws now close the sink first. Found by tightening the test leak-check (follow-up commits); pinned by the existing suite going green under strict per-tag accounting.
1 parent b827c55 commit 31066e1

2 files changed

Lines changed: 33 additions & 11 deletions

File tree

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

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -145,15 +145,30 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
145145
boolean memoryMode = sfDir == null;
146146
SlotLock acquiredLock = null;
147147
if (!memoryMode) {
148-
if (sfDir.isEmpty()) {
149-
throw new IllegalArgumentException("sfDir must not be empty");
148+
try {
149+
if (sfDir.isEmpty()) {
150+
throw new IllegalArgumentException("sfDir must not be empty");
151+
}
152+
// Acquire the slot lock BEFORE we touch any *.sfa files. Two
153+
// engines pointed at the same slot would otherwise race on
154+
// recovery and create overlapping FSN ranges. SlotLock.acquire
155+
// also creates the slot dir if it doesn't exist yet — no
156+
// separate mkdir step needed here.
157+
acquiredLock = SlotLock.acquire(sfDir);
158+
} catch (Throwable t) {
159+
// The delegating constructors evaluate `new SegmentManager(...)`
160+
// BEFORE this body runs, so on a pre-try throw (e.g. slot lock
161+
// collision) an owned manager is already alive and would leak
162+
// its native path-scratch sink -- 256 bytes per failed
163+
// construction attempt. Close it before propagating.
164+
if (ownsManager) {
165+
try {
166+
manager.close();
167+
} catch (Throwable ignored) {
168+
}
169+
}
170+
throw t;
150171
}
151-
// Acquire the slot lock BEFORE we touch any *.sfa files. Two
152-
// engines pointed at the same slot would otherwise race on
153-
// recovery and create overlapping FSN ranges. SlotLock.acquire
154-
// also creates the slot dir if it doesn't exist yet — no
155-
// separate mkdir step needed here.
156-
acquiredLock = SlotLock.acquire(sfDir);
157172
}
158173
this.slotLock = acquiredLock;
159174
this.sfDir = sfDir;
@@ -168,7 +183,6 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
168183
// reference instead of orphaning the mmap'd segments + fds.
169184
SegmentRing ringInProgress = null;
170185
AckWatermark watermarkInProgress = null;
171-
boolean managerStarted = false;
172186
try {
173187
// Disk mode: try to recover any *.sfa files left behind by a prior
174188
// session before deciding to start fresh. Without this the engine
@@ -277,7 +291,6 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
277291

278292
if (ownsManager) {
279293
manager.start();
280-
managerStarted = true;
281294
}
282295
manager.register(ringInProgress, sfDir, watermarkInProgress);
283296
// All construction succeeded — commit the ring and
@@ -288,7 +301,10 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
288301
// Stop an owned manager before freeing the ring and watermark it may
289302
// touch, then release the slot lock. Each cleanup is in its own
290303
// try/catch so a single failure doesn't strand later cleanups.
291-
if (ownsManager && managerStarted) {
304+
// Closing an owned-but-never-started manager is safe (no worker to
305+
// join) and required: skipping it leaked the manager's native
306+
// path-scratch sink whenever construction failed before start().
307+
if (ownsManager) {
292308
try {
293309
manager.close();
294310
} catch (Throwable ignored) {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,16 @@ public SegmentManager(long segmentSizeBytes, long pollNanos) {
143143
* hold an initial active plus one hot spare.
144144
*/
145145
public SegmentManager(long segmentSizeBytes, long pollNanos, long maxTotalBytes) {
146+
// The pathScratch field initializer has already allocated its native
147+
// buffer by the time this body runs, so a validation throw must free
148+
// it or every failed construction leaks 256 bytes of native memory
149+
// (e.g. a drainer retry loop hitting the same bad config).
146150
if (segmentSizeBytes < MmapSegment.HEADER_SIZE + MmapSegment.FRAME_HEADER_SIZE + 1) {
151+
pathScratch.close();
147152
throw new IllegalArgumentException("segmentSizeBytes too small: " + segmentSizeBytes);
148153
}
149154
if (maxTotalBytes < segmentSizeBytes) {
155+
pathScratch.close();
150156
throw new IllegalArgumentException(
151157
"maxTotalBytes (" + maxTotalBytes + ") must allow at least one segment of "
152158
+ segmentSizeBytes + " bytes");

0 commit comments

Comments
 (0)