Skip to content

Commit efde7bc

Browse files
bluestreak01claude
andcommitted
fix(ilp): harden SF disk-full retry, reconnect, and CRC paths
Multiple correctness fixes to the QWiP store-and-forward client that were silently losing data under realistic outage scenarios. Critical: - markSending() moved after segmentLog.append; nextBatchSequence only advances on append success. Disk-full retry no longer crashes with IllegalStateException + drift exception, recycling the buffer without persistence (C1, C2). - doReconnectCycle no longer drops pendingBuffer on every reconnect attempt. Buffer survives across attempts and is persisted by the post-reconnect ACTIVE state (C3). - createActive closes fd in try/catch on writeHeader/fsync failure; no more fd leak on every failed rotation under disk pressure (C4). - scanActive/replaySegment reject Files.length(fd) == -1 instead of treating it as "empty segment" (C5). Moderate: - scanActive distinguishes torn tail from mid-stream CRC mismatch; bit-rot followed by trailing bytes throws instead of silently truncating (M1). - Files.close accepts any fd >= 0 (was refusing 0/1/2, leaking lock fd in containers where stdin/stdout/stderr were pre-closed) (M2). - Connect-string sf_max_bytes / sf_max_total_bytes parsed as long; was capped at ~2 GB by parseIntValue (M3). - WebSocketSendQueue.client made volatile so close-during-reconnect reads the live ref, not a stale one (M4). - SegmentLog uses ObjList instead of java.util.ArrayList; bytesOnDisk is cached and updated incrementally so append() is O(1) zero-alloc on the I/O hot path (M6, N3). - Each Segment caches a native UTF-8 path pointer; remove(String) is no longer called per-trim, eliminating the byte[] alloc on the I/O thread per ACK (M7). - retryStalled always re-flags interrupt status (M8). Cleanup: - Dead WebSocketSendQueue.safeSendBatch removed (N1). - @FunctionalInterface on Reconnector (N2). - Inline FQNs in QwpWebSocketSender / Sender replaced with imports (N6). - setSegmentLog overload pair co-located with cleaner doc (N8). - Javadoc added to Files.java public surface and Crc32c.update (N9). - Single-arg failConnection overload removed; every call site is now explicit about fatal vs non-fatal (N10). Infrastructure: - New FilesFacade interface + DefaultFilesFacade impl in io.questdb.client.std. SegmentLog refactored to use the facade so tests can inject OS-level failures (short writes, fstat -1, fsync EIO) without filesystem-level tricks. Tests: - 12 new red regression tests for the bug fixes above (now green). - 5 coverage-gap tests for previously-untested error paths (M9): unsupported version header, baseSeq mismatch, multi-active rejection, oldestSeq edge cases, short-write recovery via fault-injection. - Full SF + Files suite: 70 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f5afbf6 commit efde7bc

12 files changed

Lines changed: 1847 additions & 170 deletions

File tree

core/src/main/java/io/questdb/client/Sender.java

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@
3636
import io.questdb.client.cutlass.line.tcp.PlainTcpLineChannel;
3737
import io.questdb.client.cutlass.qwp.client.QwpUdpSender;
3838
import io.questdb.client.cutlass.qwp.client.QwpWebSocketSender;
39+
import io.questdb.client.cutlass.qwp.client.sf.SegmentLog;
40+
import io.questdb.client.cutlass.qwp.client.sf.SfDiskFullException;
3941
import io.questdb.client.impl.ConfStringParser;
4042
import io.questdb.client.network.NetworkFacade;
4143
import io.questdb.client.network.NetworkFacadeImpl;
@@ -931,7 +933,7 @@ public Sender build() {
931933
);
932934
}
933935

934-
io.questdb.client.cutlass.qwp.client.sf.SegmentLog segmentLog = null;
936+
SegmentLog segmentLog = null;
935937
if (storeAndForward) {
936938
if (sfDir == null) {
937939
throw new LineSenderException(
@@ -942,12 +944,12 @@ public Sender build() {
942944
"store_and_forward requires async mode (in_flight_window > 1)");
943945
}
944946
long actualSfMaxBytes = sfMaxBytes == PARAMETER_NOT_SET_EXPLICITLY
945-
? io.questdb.client.cutlass.qwp.client.sf.SegmentLog.DEFAULT_MAX_BYTES_PER_SEGMENT
947+
? SegmentLog.DEFAULT_MAX_BYTES_PER_SEGMENT
946948
: sfMaxBytes;
947949
long actualSfMaxTotalBytes = sfMaxTotalBytes == PARAMETER_NOT_SET_EXPLICITLY
948-
? io.questdb.client.cutlass.qwp.client.sf.SegmentLog.DEFAULT_MAX_TOTAL_BYTES
950+
? SegmentLog.DEFAULT_MAX_TOTAL_BYTES
949951
: sfMaxTotalBytes;
950-
segmentLog = io.questdb.client.cutlass.qwp.client.sf.SegmentLog.open(
952+
segmentLog = SegmentLog.open(
951953
sfDir, actualSfMaxBytes, actualSfMaxTotalBytes, sfFsync);
952954
} else if (sfDir != null) {
953955
throw new LineSenderException(
@@ -1589,7 +1591,7 @@ public LineSenderBuilder storeAndForwardDir(String dir) {
15891591

15901592
/**
15911593
* Maximum bytes per segment file before rotation. Defaults to
1592-
* {@link io.questdb.client.cutlass.qwp.client.sf.SegmentLog#DEFAULT_MAX_BYTES_PER_SEGMENT}
1594+
* {@link SegmentLog#DEFAULT_MAX_BYTES_PER_SEGMENT}
15931595
* (64 MiB). Smaller segments mean faster trim of acked data; larger
15941596
* segments mean fewer rotations.
15951597
*/
@@ -1606,7 +1608,7 @@ public LineSenderBuilder storeAndForwardMaxBytes(long maxBytes) {
16061608

16071609
/**
16081610
* Hard cap on total bytes consumed by SF on disk. When the cap is reached,
1609-
* subsequent appends throw {@link io.questdb.client.cutlass.qwp.client.sf.SfDiskFullException}
1611+
* subsequent appends throw {@link SfDiskFullException}
16101612
* which propagates as back-pressure: {@code flush()} blocks on the user
16111613
* thread until ACKs trim acknowledged segments and free space. Default is
16121614
* unbounded ({@link Long#MAX_VALUE}).
@@ -1694,6 +1696,17 @@ private static int parseIntValue(@NotNull StringSink value, @NotNull String name
16941696
}
16951697
}
16961698

1699+
private static long parseLongValue(@NotNull StringSink value, @NotNull String name) {
1700+
if (Chars.isBlank(value)) {
1701+
throw new LineSenderException(name).put(" cannot be empty");
1702+
}
1703+
try {
1704+
return Numbers.parseLong(value);
1705+
} catch (NumericException e) {
1706+
throw new LineSenderException("invalid ").put(name).put(" [value=").put(value).put("]");
1707+
}
1708+
}
1709+
16971710
private static int resolveIPv4(String host) {
16981711
try {
16991712
byte[] addr = InetAddress.getByName(host).getAddress();
@@ -2071,14 +2084,14 @@ private LineSenderBuilder fromConfig(CharSequence configurationString) {
20712084
throw new LineSenderException("sf_max_bytes is only supported for WebSocket transport");
20722085
}
20732086
pos = getValue(configurationString, pos, sink, "sf_max_bytes");
2074-
long maxBytes = parseIntValue(sink, "sf_max_bytes");
2087+
long maxBytes = parseLongValue(sink, "sf_max_bytes");
20752088
storeAndForwardMaxBytes(maxBytes);
20762089
} else if (Chars.equals("sf_max_total_bytes", sink)) {
20772090
if (protocol != PROTOCOL_WEBSOCKET) {
20782091
throw new LineSenderException("sf_max_total_bytes is only supported for WebSocket transport");
20792092
}
20802093
pos = getValue(configurationString, pos, sink, "sf_max_total_bytes");
2081-
long maxTotal = parseIntValue(sink, "sf_max_total_bytes");
2094+
long maxTotal = parseLongValue(sink, "sf_max_total_bytes");
20822095
storeAndForwardMaxTotalBytes(maxTotal);
20832096
} else if (Chars.equals("sf_fsync", sink)) {
20842097
if (protocol != PROTOCOL_WEBSOCKET) {

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

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import io.questdb.client.cutlass.line.LineSenderException;
3434
import io.questdb.client.cutlass.line.array.DoubleArray;
3535
import io.questdb.client.cutlass.line.array.LongArray;
36+
import io.questdb.client.cutlass.qwp.client.sf.SegmentLog;
3637
import io.questdb.client.cutlass.qwp.protocol.QwpConstants;
3738
import io.questdb.client.cutlass.qwp.protocol.QwpTableBuffer;
3839
import io.questdb.client.std.CharSequenceLongHashMap;
@@ -167,7 +168,7 @@ public class QwpWebSocketSender implements Sender {
167168
private boolean sawBinaryAck;
168169
private boolean sawPong;
169170
private WebSocketSendQueue sendQueue;
170-
private io.questdb.client.cutlass.qwp.client.sf.SegmentLog segmentLog;
171+
private SegmentLog segmentLog;
171172
// True when this sender took ownership of segmentLog (e.g. via the
172173
// connect-string builder); close() will then close the log too.
173174
private boolean ownsSegmentLog;
@@ -348,7 +349,7 @@ public static QwpWebSocketSender connect(
348349
String authorizationHeader,
349350
int maxSchemasPerConnection,
350351
boolean requestDurableAck,
351-
io.questdb.client.cutlass.qwp.client.sf.SegmentLog segmentLog
352+
SegmentLog segmentLog
352353
) {
353354
QwpWebSocketSender sender = new QwpWebSocketSender(
354355
host, port, tlsConfig,
@@ -1192,38 +1193,32 @@ public void setRequestDurableAck(boolean enabled) {
11921193
}
11931194

11941195
/**
1195-
* Attach a store-and-forward log. Every outgoing batch is captured to disk
1196-
* before the wire send and trimmed on cumulative ACK; the log also becomes
1197-
* the batch-sequence authority so sequencing survives sender restarts. The
1198-
* caller retains ownership of the log and is responsible for closing it
1199-
* after this sender has been closed.
1196+
* Attach a store-and-forward log to capture every outgoing batch to disk
1197+
* before the wire send and trim it on cumulative ACK. The log also becomes
1198+
* the batch-sequence authority so sequencing survives sender restarts.
12001199
* <p>
1201-
* Requires async mode ({@code inFlightWindowSize > 1}).
1200+
* The caller retains ownership of {@code log} and is responsible for
1201+
* closing it after this sender has been closed; use the two-arg overload
1202+
* {@link #setSegmentLog(SegmentLog, boolean)} to transfer ownership.
1203+
* <p>
1204+
* Must be called before the first send. Requires async mode
1205+
* ({@code inFlightWindowSize > 1}).
12021206
*
12031207
* @throws LineSenderException if the sender is already connected or closed,
12041208
* or if async mode is not enabled
12051209
*/
1206-
public void setSegmentLog(io.questdb.client.cutlass.qwp.client.sf.SegmentLog log) {
1210+
public void setSegmentLog(SegmentLog log) {
12071211
setSegmentLog(log, false);
12081212
}
12091213

12101214
/**
1211-
* Number of times an outgoing batch was stalled because the SF total disk cap
1212-
* was reached. Each stall blocks the user thread's flush() until ACKs trim
1213-
* sealed segments and free space. Useful for monitoring backpressure under
1214-
* production load.
1215-
*/
1216-
public long getTotalSfDiskFullStalls() {
1217-
return sendQueue == null ? 0 : sendQueue.getTotalDiskFullStalls();
1218-
}
1219-
1220-
/**
1221-
* Like {@link #setSegmentLog(io.questdb.client.cutlass.qwp.client.sf.SegmentLog)} but
1222-
* with explicit ownership transfer: when {@code takeOwnership} is true, this
1223-
* sender will close the log on its own {@link #close()}. Used by the
1224-
* connect-string builder to give the sender a self-contained lifecycle.
1215+
* Like {@link #setSegmentLog(SegmentLog)} but with explicit ownership
1216+
* transfer: when {@code takeOwnership} is true the sender closes
1217+
* {@code log} on its own {@link #close()}. Used by the connect-string
1218+
* builder to give the sender a self-contained lifecycle. Pass
1219+
* {@code false} to keep ownership with the caller.
12251220
*/
1226-
public void setSegmentLog(io.questdb.client.cutlass.qwp.client.sf.SegmentLog log, boolean takeOwnership) {
1221+
public void setSegmentLog(SegmentLog log, boolean takeOwnership) {
12271222
if (closed) {
12281223
throw new LineSenderException("Sender is closed");
12291224
}
@@ -1239,6 +1234,16 @@ public void setSegmentLog(io.questdb.client.cutlass.qwp.client.sf.SegmentLog log
12391234
this.ownsSegmentLog = takeOwnership && log != null;
12401235
}
12411236

1237+
/**
1238+
* Number of times an outgoing batch was stalled because the SF total disk cap
1239+
* was reached. Each stall blocks the user thread's flush() until ACKs trim
1240+
* sealed segments and free space. Useful for monitoring backpressure under
1241+
* production load.
1242+
*/
1243+
public long getTotalSfDiskFullStalls() {
1244+
return sendQueue == null ? 0 : sendQueue.getTotalDiskFullStalls();
1245+
}
1246+
12421247
/**
12431248
* Adds a SHORT column value to the current row.
12441249
*

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
* retry. Connection-fatal errors (auth failure, protocol mismatch) should still
4545
* be thrown; classification of fatal vs recoverable is the caller's job.
4646
*/
47+
@FunctionalInterface
4748
public interface Reconnector {
4849
WebSocketClient reconnect() throws Exception;
4950
}

0 commit comments

Comments
 (0)