Skip to content

Commit a6b45c3

Browse files
committed
fix(ilp): cross-platform drainer/slot-lock and quieter PARSE_ERROR logs
BackgroundDrainerPool.close() called requestStop() before shutdown(), forcing in-flight drainers to exit as STOPPED instead of SUCCESS. Their engine.close() then saw fullyDrained=false and skipped the .sfa cleanup, so drain_orphans=true left orphan slots on disk on macOS where the race went the other way. Now we shutdown() first and let drainers complete naturally within a grace window; requestStop() is the fallback. SlotLock kept the holder's PID inside the .lock file itself. Windows' LockFileEx is a mandatory whole-file lock, so a contender reading the PID got 0 bytes and surfaced "holder=unknown" instead of "pid=<n>". Move the PID to a sibling .lock.pid sidecar that nobody locks — the diagnostic now works uniformly on POSIX and Windows. CursorWebSocketSendLoop.recordFatal() logged the full throwable for server-side rejects like PARSE_ERROR. The structured info is already in the message and the dispatcher's one-line log; suppress the stack trace for serverError != null and keep it for genuine client-side failures.
1 parent 9be35cb commit a6b45c3

3 files changed

Lines changed: 67 additions & 33 deletions

File tree

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

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,13 @@
5252
public final class BackgroundDrainerPool implements QuietCloseable {
5353

5454
private static final Logger LOG = LoggerFactory.getLogger(BackgroundDrainerPool.class);
55-
private static final long CLOSE_GRACE_MILLIS = 3_000L;
55+
// Time we let drainers finish their drain naturally before signaling
56+
// stop. awaitTermination returns as soon as the last drainer exits,
57+
// so this only matters when something is genuinely stuck.
58+
private static final long GRACEFUL_DRAIN_MILLIS = 2_500L;
59+
// After signaling stop, give drainers a brief window to unwind cleanly
60+
// (release slot lock, close engine) before forcing shutdownNow.
61+
private static final long STOP_GRACE_MILLIS = 500L;
5662
// CAS gate. Single AtomicInteger packs the closed flag (sign bit) and
5763
// the in-flight submit count (low 31 bits):
5864
// state >= 0 → open, value is the in-flight submit count
@@ -160,16 +166,25 @@ public void close() {
160166
while (state.get() != CLOSED_BIT) {
161167
Thread.onSpinWait();
162168
}
163-
for (BackgroundDrainer d : active) {
164-
d.requestStop();
165-
}
169+
// Reject new tasks but let in-flight drainers finish their drain
170+
// naturally. Without this grace window a drainer that's seconds
171+
// away from acked >= target gets requestStop()'d and exits as
172+
// STOPPED — its engine.close() then sees fullyDrained=false and
173+
// leaves the slot's .sfa files behind, defeating drain_orphans.
166174
executor.shutdown();
167175
try {
168-
if (!executor.awaitTermination(CLOSE_GRACE_MILLIS, TimeUnit.MILLISECONDS)) {
169-
LOG.warn("drainer pool did not finish in {}ms; "
170-
+ "remaining drainers will exit on their own",
171-
CLOSE_GRACE_MILLIS);
172-
executor.shutdownNow();
176+
if (!executor.awaitTermination(GRACEFUL_DRAIN_MILLIS, TimeUnit.MILLISECONDS)) {
177+
LOG.warn("orphan drainers still running after {}ms — signaling stop",
178+
GRACEFUL_DRAIN_MILLIS);
179+
for (BackgroundDrainer d : active) {
180+
d.requestStop();
181+
}
182+
if (!executor.awaitTermination(STOP_GRACE_MILLIS, TimeUnit.MILLISECONDS)) {
183+
LOG.warn("drainer pool did not exit in {}ms after stop; "
184+
+ "remaining drainers will exit on their own",
185+
STOP_GRACE_MILLIS);
186+
executor.shutdownNow();
187+
}
173188
}
174189
} catch (InterruptedException e) {
175190
Thread.currentThread().interrupt();

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,11 @@ private void recordFatal(Throwable t, SenderError serverError) {
591591
lastTerminalServerError = serverError;
592592
}
593593
running = false;
594-
LOG.error("Cursor I/O loop failure: {}", t.getMessage(), t);
594+
if (serverError != null) {
595+
LOG.error("Cursor I/O loop failure: {}", t.getMessage());
596+
} else {
597+
LOG.error("Cursor I/O loop failure: {}", t.getMessage(), t);
598+
}
595599
}
596600

597601
/**

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

Lines changed: 38 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,18 @@
3434
/**
3535
* Advisory exclusive lock for a single SF slot directory.
3636
* <p>
37-
* One {@code .lock} file per slot, held via {@code flock} for the entire
38-
* lifetime of the engine that owns the slot. The lock is automatically
39-
* released when the fd is closed — including on hard process exit, since
40-
* the kernel cleans up flocks for terminated processes (see flock(2)).
37+
* One {@code .lock} file per slot, held via {@code flock}/{@code LockFileEx}
38+
* for the entire lifetime of the engine that owns the slot. The lock is
39+
* automatically released when the fd is closed — including on hard process
40+
* exit, since the kernel cleans up file locks for terminated processes.
4141
* <p>
42-
* The lock file's payload is the holder's PID, written at acquisition
43-
* time. A failed acquisition reads it back so the error message can name
44-
* the offending process — turning a vague "slot in use" into actionable
45-
* diagnostics.
42+
* The holder's PID is written to a sibling {@code .lock.pid} file at
43+
* acquisition time. A failed acquisition reads it back so the error message
44+
* can name the offending process — turning a vague "slot in use" into
45+
* actionable diagnostics. The PID lives in a separate file because Windows'
46+
* {@code LockFileEx} is a mandatory range lock: while the {@code .lock}
47+
* file is held, a second handle cannot read its bytes, so we couldn't
48+
* recover the holder's PID from the lock file itself.
4649
* <p>
4750
* Two senders pointing at the same slot dir is the multi-writer footgun
4851
* the slot model exists to prevent: their FSN sequences would interleave
@@ -53,6 +56,7 @@
5356
public final class SlotLock implements QuietCloseable {
5457

5558
private static final String LOCK_FILE_NAME = ".lock";
59+
private static final String LOCK_PID_FILE_NAME = ".lock.pid";
5660
private final String slotDir;
5761
private final String lockPath;
5862
private int fd;
@@ -83,6 +87,7 @@ public static SlotLock acquire(String slotDir) {
8387
}
8488
}
8589
String lockPath = slotDir + "/" + LOCK_FILE_NAME;
90+
String pidPath = slotDir + "/" + LOCK_PID_FILE_NAME;
8691
int fd = Files.openRW(lockPath);
8792
if (fd < 0) {
8893
throw new IllegalStateException(
@@ -92,12 +97,12 @@ public static SlotLock acquire(String slotDir) {
9297
try {
9398
int rc = Files.lock(fd);
9499
if (rc != 0) {
95-
String holder = readHolder(lockPath);
100+
String holder = readHolder(pidPath);
96101
throw new IllegalStateException(
97102
"sf slot already in use by another process [slot="
98103
+ slotDir + ", holder=" + holder + "]");
99104
}
100-
writePid(fd);
105+
writePid(pidPath);
101106
ok = true;
102107
return new SlotLock(slotDir, lockPath, fd);
103108
} finally {
@@ -114,17 +119,18 @@ public String slotDir() {
114119

115120
@Override
116121
public void close() {
117-
// Closing the fd releases the flock. We do NOT remove the file —
118-
// a stale .lock with the previous PID is harmless (next acquirer
119-
// can flock it just fine, and overwrites the PID on success).
122+
// Closing the fd releases the lock. We do NOT remove the .lock
123+
// file or the .lock.pid sidecar — a stale PID is harmless (next
124+
// acquirer overwrites .lock.pid on success).
120125
if (fd >= 0) {
121126
Files.close(fd);
122127
fd = -1;
123128
}
124129
}
125130

126-
private static String readHolder(String lockPath) {
127-
int rfd = Files.openRO(lockPath);
131+
private static String readHolder(String pidPath) {
132+
if (!Files.exists(pidPath)) return "unknown";
133+
int rfd = Files.openRO(pidPath);
128134
if (rfd < 0) return "unknown";
129135
try {
130136
long fileLen = Files.length(rfd);
@@ -147,24 +153,33 @@ private static String readHolder(String lockPath) {
147153
}
148154
}
149155

150-
private static void writePid(int fd) {
156+
private static void writePid(String pidPath) {
151157
long pid;
152158
try {
153159
pid = ProcessHandle.current().pid();
154160
} catch (Throwable ignored) {
155161
// Diagnostic-only — never block lock acquisition on it.
156162
pid = -1L;
157163
}
158-
byte[] payload = (pid + "\n").getBytes(StandardCharsets.UTF_8);
159-
Files.truncate(fd, 0L);
160-
long addr = Unsafe.malloc(payload.length, MemoryTag.NATIVE_DEFAULT);
164+
int wfd = Files.openRW(pidPath);
165+
if (wfd < 0) {
166+
// Diagnostic-only — never block lock acquisition on it.
167+
return;
168+
}
161169
try {
162-
for (int i = 0; i < payload.length; i++) {
163-
Unsafe.getUnsafe().putByte(addr + i, payload[i]);
170+
Files.truncate(wfd, 0L);
171+
byte[] payload = (pid + "\n").getBytes(StandardCharsets.UTF_8);
172+
long addr = Unsafe.malloc(payload.length, MemoryTag.NATIVE_DEFAULT);
173+
try {
174+
for (int i = 0; i < payload.length; i++) {
175+
Unsafe.getUnsafe().putByte(addr + i, payload[i]);
176+
}
177+
Files.write(wfd, addr, payload.length, 0L);
178+
} finally {
179+
Unsafe.free(addr, payload.length, MemoryTag.NATIVE_DEFAULT);
164180
}
165-
Files.write(fd, addr, payload.length, 0L);
166181
} finally {
167-
Unsafe.free(addr, payload.length, MemoryTag.NATIVE_DEFAULT);
182+
Files.close(wfd);
168183
}
169184
}
170185
}

0 commit comments

Comments
 (0)