Skip to content

Commit c6eadf0

Browse files
committed
fix(qwp): stop close() waiting on acks of uncommitted deferred frames
The server (as of the companion core fix) withholds cumulative OK acks for FLAG_DEFER_COMMIT frames until their group-closing commit lands: acking them earlier let the store-and-forward client trim slots whose rows the server rolls back on any error, demote, or disconnect -- silent data loss (#7144's stated replay contract, previously unenforced). Client-side consequences fixed here: - QwpWebSocketSender tracks lastCommitBoundaryFsn: the FSN of the last commit-bearing (non-deferred) frame published. Updated by flushPendingRows, flushPendingRowsSplit, sendCommitMessage, and the close()-time residual seal. - drainOnClose() now drains to min(publishedFsn, commit boundary) instead of publishedFsn. Waiting for acks of an uncommitted deferred tail could only ever time out (300s hangs in testDeferredCommitConnectionDropRollsBack). Abandoning frames is WARN-logged with an actionable message (flush() to commit, or the abort is intentional). - CursorSendEngine computes recoveredCommitBoundaryFsn at disk recovery: the last commit-bearing frame below a potentially orphaned deferred tail left by a producer that died mid-transaction. Close drains combine it with the session boundary. Recovery WARNs when an orphaned deferred tail exists, because replaying it into a NEW session's commit would resurrect a partial transaction -- the skip-and-self-ack replay policy for that tail is a documented follow-up. - MmapSegment/SegmentRing grow a read-only recovery-time frame walk (findLastFsnWithoutPayloadFlag) that locates the boundary by peeking the QWP flags byte of each published frame. Full client suite passes (2505 tests).
1 parent 51bc8af commit c6eadf0

4 files changed

Lines changed: 130 additions & 2 deletions

File tree

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

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,13 @@ public class QwpWebSocketSender implements Sender {
253253
private int errorInboxCapacity = SenderErrorDispatcher.DEFAULT_CAPACITY;
254254
private long firstPendingRowTimeNanos;
255255
private boolean hasDeferredMessages;
256+
// FSN of the last commit-bearing (non-FLAG_DEFER_COMMIT) frame this session
257+
// published, or -1 when none. Frames above it are deferred and uncommitted:
258+
// the server withholds their acks by design (their rows are rolled back on
259+
// any error, demote, or disconnect), so close-time drains must never wait
260+
// for them. Updated on every non-deferred publish; combined with the
261+
// engine's recovered boundary in drainOnClose.
262+
private long lastCommitBoundaryFsn = -1L;
256263
// Stickys true once any successful FOREGROUND connect has happened
257264
// (background drainer connects never set it). Drives the
258265
// CONNECTED-vs-RECONNECTED-vs-FAILED_OVER classification at the success
@@ -1076,6 +1083,9 @@ public void close() {
10761083
}
10771084
if (activeBuffer != null && activeBuffer.hasData()) {
10781085
sealAndSwapBuffer();
1086+
if (!deferCommit) {
1087+
lastCommitBoundaryFsn = cursorEngine.publishedFsn();
1088+
}
10791089
}
10801090
// 2) Safety-net rethrow: surface the latched terminal
10811091
// error only when no other channel has already
@@ -3149,7 +3159,22 @@ private void drainOnClose(boolean errorOwnedByCustomHandler) {
31493159
if (closeFlushTimeoutMillis <= 0L) {
31503160
return;
31513161
}
3152-
long target = cursorEngine.publishedFsn();
3162+
long published = cursorEngine.publishedFsn();
3163+
// Never wait for uncommitted deferred frames: the server withholds
3164+
// their acks by design (FLAG_DEFER_COMMIT rows are rolled back on
3165+
// error/demote/disconnect and must stay replayable client-side), so a
3166+
// drain targeting them can only time out. The drain target is the last
3167+
// commit-bearing frame this session published, or -- for a ring
3168+
// recovered from disk with no commit published this session -- the
3169+
// recovered commit boundary.
3170+
long boundary = Math.max(lastCommitBoundaryFsn, cursorEngine.recoveredCommitBoundaryFsn());
3171+
long target = Math.min(published, boundary);
3172+
if (target < published) {
3173+
LOG.warn("close() abandoning {} uncommitted deferred frame(s) [commitBoundaryFsn={}, publishedFsn={}] "
3174+
+ "-- their transaction was never committed; the server rolls their rows back. "
3175+
+ "Call flush() before close() to commit, or ignore if the abort is intentional.",
3176+
published - target, target, published);
3177+
}
31533178
if (cursorEngine.ackedFsn() >= target) {
31543179
return;
31553180
}
@@ -3169,7 +3194,7 @@ private void drainOnClose(boolean errorOwnedByCustomHandler) {
31693194
LOG.warn("close() drain timed out after {}ms [target={} acked={}], pending data may be lost",
31703195
closeFlushTimeoutMillis, target, acked);
31713196
throw new LineSenderException("close() drain timed out after ")
3172-
.put(closeFlushTimeoutMillis).put(" ms [publishedFsn=")
3197+
.put(closeFlushTimeoutMillis).put(" ms [targetFsn=")
31733198
.put(target).put(", ackedFsn=").put(acked)
31743199
.put("] - server did not acknowledge ")
31753200
.put(target - acked)
@@ -3430,6 +3455,9 @@ private void flushPendingRows(boolean deferCommit) {
34303455
sealAndSwapBuffer();
34313456

34323457
hasDeferredMessages = deferCommit;
3458+
if (!deferCommit) {
3459+
lastCommitBoundaryFsn = cursorEngine.publishedFsn();
3460+
}
34333461

34343462
resetTableBuffersAfterFlush(keys);
34353463
}
@@ -3502,6 +3530,11 @@ private void flushPendingRowsSplit(ObjList<CharSequence> keys, boolean deferComm
35023530

35033531
encoder.setDeferCommit(false);
35043532
hasDeferredMessages = deferCommit;
3533+
if (!deferCommit) {
3534+
// The last message of the split carried no defer flag -- it
3535+
// committed the whole group.
3536+
lastCommitBoundaryFsn = cursorEngine.publishedFsn();
3537+
}
35053538
resetTableBuffersAfterFlush(keys);
35063539
}
35073540

@@ -3543,6 +3576,7 @@ private void sendCommitMessage() {
35433576
activeBuffer.incrementRowCount();
35443577
sealAndSwapBuffer();
35453578
hasDeferredMessages = false;
3579+
lastCommitBoundaryFsn = cursorEngine.publishedFsn();
35463580
}
35473581

35483582
private void resetSymbolDictStateForNewConnection() {

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,14 @@ public final class CursorSendEngine implements QuietCloseable {
8888
// full symbol-dict delta), so producer-side schema reset on recovery
8989
// is not required.
9090
private final boolean wasRecoveredFromDisk;
91+
// FSN of the last commit-bearing (non-FLAG_DEFER_COMMIT) frame found in a
92+
// ring recovered from disk, or -1 for fresh/memory rings and recovered
93+
// rings whose every frame is deferred. Frames above this FSN in the
94+
// recovered ring belong to a transaction whose commit frame was never
95+
// published; the server will never ack them until some later commit
96+
// covers them. Read by the sender's close-time drain to avoid waiting on
97+
// acks that cannot arrive.
98+
private long recoveredCommitBoundaryFsn = -1L;
9199
// Engine-owned mmap'd watermark file. {@code null} in memory mode and
92100
// in disk mode if open() failed (we proceed without it; recovery just
93101
// falls back to lowestBase - 1). Lifetime tied to the engine: opened
@@ -260,6 +268,29 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
260268
if (seed >= 0) {
261269
recovered.acknowledge(seed);
262270
}
271+
// Locate the last commit-bearing frame below a potentially
272+
// orphaned FLAG_DEFER_COMMIT tail. A producer that crashed (or
273+
// closed) mid-transaction leaves deferred frames with no
274+
// covering commit frame at the top of the ring. The server
275+
// never acks uncommitted deferred frames, so (a) close-time
276+
// drains must not wait for them (see the sender's
277+
// drainOnClose), and (b) replaying them into a NEW session's
278+
// commit would resurrect half a transaction -- see the WARN
279+
// below. Computed before the I/O loop or producer append.
280+
this.recoveredCommitBoundaryFsn = recovered.findLastFsnWithoutPayloadFlag(
281+
io.questdb.client.cutlass.qwp.protocol.QwpConstants.HEADER_OFFSET_FLAGS,
282+
io.questdb.client.cutlass.qwp.protocol.QwpConstants.FLAG_DEFER_COMMIT
283+
);
284+
if (publishedFsn >= 0 && recoveredCommitBoundaryFsn < publishedFsn) {
285+
LOG.warn("recovered SF log ends with {} deferred frame(s) whose transaction was never "
286+
+ "committed [commitBoundaryFsn={}, publishedFsn={}]. On replay these frames "
287+
+ "re-append rows the server will only commit when THIS session sends a "
288+
+ "commit-bearing frame -- which would resurrect a partial transaction. "
289+
+ "If the prior transaction must stay aborted, clear the sf_dir before "
290+
+ "restarting the producer.",
291+
publishedFsn - Math.max(recoveredCommitBoundaryFsn, -1L),
292+
recoveredCommitBoundaryFsn, publishedFsn);
293+
}
263294
} else {
264295
// Fresh start with no recovered segments. Any stale
265296
// watermark from a prior fully-drained session refers
@@ -600,6 +631,16 @@ public boolean wasRecoveredFromDisk() {
600631
return wasRecoveredFromDisk;
601632
}
602633

634+
/**
635+
* FSN of the last commit-bearing frame in a disk-recovered ring, or
636+
* {@code -1} for fresh/memory rings. Frames above it are an orphaned
637+
* deferred tail (transaction never committed) that the server will not
638+
* ack until a later commit-bearing frame covers them.
639+
*/
640+
public long recoveredCommitBoundaryFsn() {
641+
return recoveredCommitBoundaryFsn;
642+
}
643+
603644
/**
604645
* Unlinks every {@code .sfa} file under {@code dir}. Called only on
605646
* clean shutdown when the ring confirms every published FSN has been

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,34 @@ public long tryAppend(long payloadAddr, int payloadLen) {
443443
return offset;
444444
}
445445

446+
/**
447+
* Walks every published frame in this segment and returns the FSN of the
448+
* LAST frame whose payload does NOT carry the given flag bit, or {@code -1}
449+
* when every frame carries it (or the segment is empty). Frames shorter
450+
* than {@code flagsOffset + 1} bytes are treated as not carrying the flag.
451+
* <p>
452+
* Producer-thread only, and only meaningful before new appends race the
453+
* walk (recovery time). Used to locate the last commit-bearing QWP frame
454+
* below a potentially orphaned FLAG_DEFER_COMMIT tail: frames above the
455+
* returned FSN all carry the flag, i.e. they belong to a transaction whose
456+
* commit frame was never published.
457+
*/
458+
public long findLastFrameFsnWithoutPayloadFlag(int flagsOffset, int flagMask) {
459+
long best = -1L;
460+
long off = HEADER_SIZE;
461+
long frames = frameCount;
462+
for (long i = 0; i < frames; i++) {
463+
int payloadLen = Unsafe.getUnsafe().getInt(mmapAddress + off + 4);
464+
boolean flagSet = payloadLen > flagsOffset
465+
&& (Unsafe.getUnsafe().getByte(mmapAddress + off + FRAME_HEADER_SIZE + flagsOffset) & flagMask) != 0;
466+
if (!flagSet) {
467+
best = baseSeq + i;
468+
}
469+
off += FRAME_HEADER_SIZE + payloadLen;
470+
}
471+
return best;
472+
}
473+
446474
/**
447475
* Number of frames written since {@link #create} (or recovered by
448476
* {@link #openExisting}). Used by {@code SegmentRing} to compute

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,31 @@ public synchronized MmapSegment firstSealed() {
510510
}
511511

512512
/** Active segment -- exposed for the I/O thread's "send next batch" path. */
513+
/**
514+
* Walks every published frame in the ring (sealed segments plus the active
515+
* segment) and returns the FSN of the LAST frame whose payload does NOT
516+
* carry the given flag bit, or {@code -1} when every published frame
517+
* carries it (or the ring is empty). All frames above the returned FSN
518+
* carry the flag.
519+
* <p>
520+
* Recovery-time helper: locates the last commit-bearing QWP frame below a
521+
* potentially orphaned FLAG_DEFER_COMMIT tail left behind by a producer
522+
* that crashed (or closed) mid-transaction. Call before the I/O loop and
523+
* producer start appending; the walk is not synchronized against appends
524+
* into the active segment.
525+
*/
526+
public synchronized long findLastFsnWithoutPayloadFlag(int flagsOffset, int flagMask) {
527+
long best = -1L;
528+
for (int i = 0, n = sealedSegments.size(); i < n; i++) {
529+
long fsn = sealedSegments.get(i).findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask);
530+
if (fsn > best) {
531+
best = fsn;
532+
}
533+
}
534+
long fsn = active.findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask);
535+
return Math.max(best, fsn);
536+
}
537+
513538
public MmapSegment getActive() {
514539
return active;
515540
}

0 commit comments

Comments
 (0)