Skip to content

Commit 9d5451c

Browse files
committed
fix(qwp): skip-and-self-ack the recovered orphaned deferred tail
Closes the resurrect-partial-transaction hole documented in the deferred-ack series: a producer that crashes (or closes) mid-transaction leaves FLAG_DEFER_COMMIT frames with no covering commit frame at the top of its disk-recovered SF log. Replaying them would let a commit-bearing frame from the NEW session commit them -- half a dead transaction resurrected, violating transaction=on atomicity. Previously detected + WARN'd only. Mechanism (CursorWebSocketSendLoop): - The loop adopts the engine's recovered orphan range [recoveredCommitBoundaryFsn+1 .. recoveredOrphanTipFsn] at construction; the BackgroundDrainer path builds through the same constructor and gets identical containment. - trySendOne stops the cursor at the tail: orphan frames are never transmitted. - tryRetireOrphanTail retires the tail with a cumulative self-acknowledge once every frame below it is server-acked (legal: the tail was never sent, no wire sequence references it). Crash-safe and idempotent -- dying before the self-ack re-detects the same range at next recovery. - Fast path (common crash profile: acks current, only the in-flight transaction unacked; trivially when the whole log is the tail): the tail retires in positionCursorForStart before any connection exists. Zero wire cost, no reconnect. - Slow path (unacked committed frames below the tail): they replay first; on retirement the connection recycles exactly once so the linear wireSeq<->FSN mapping re-anchors past the FSN gap. swapClient also re-attempts retirement so a reconnect never replays into the tail. Positive-identification hardening (found by the drainer test suite): the recovery walk classifies a frame as deferred ONLY when it parses as a QWP message -- payload >= HEADER_SIZE and u32 'QWP1' magic at offset 0 -- before reading the flags byte. Anything unidentifiable is a retirement barrier, never trimmable: misclassifying an unknown frame as deferred would silently discard data that should replay (the drainer tests' filler payloads had bit 0 set at offset 5 and were being trimmed wholesale). Tests: CursorWebSocketSendLoopOrphanTailTest covers recovery detection (orphan tail present/absent), the zero-send fast path, and the slow path end-to-end (replay below tail, single recycle, correct ack attribution across the FSN gap for new-session frames). Full client suite: 2509 green.
1 parent cd48ae3 commit 9d5451c

5 files changed

Lines changed: 491 additions & 15 deletions

File tree

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

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,14 @@ public final class CursorSendEngine implements QuietCloseable {
9696
// covers them. Read by the sender's close-time drain to avoid waiting on
9797
// acks that cannot arrive.
9898
private long recoveredCommitBoundaryFsn = -1L;
99+
// FSN of the last frame of a recovered orphaned deferred tail, or -1 when
100+
// the recovered ring has no such tail. When >= 0, frames
101+
// [recoveredCommitBoundaryFsn + 1 .. recoveredOrphanTipFsn] all carry
102+
// FLAG_DEFER_COMMIT with no covering commit frame -- an aborted
103+
// transaction. The send loop must never transmit them; it retires the
104+
// range with a cumulative self-acknowledge once everything below is
105+
// server-acked (CursorWebSocketSendLoop.tryRetireOrphanTail).
106+
private long recoveredOrphanTipFsn = -1L;
99107
// Engine-owned mmap'd watermark file. {@code null} in memory mode and
100108
// in disk mode if open() failed (we proceed without it; recovery just
101109
// falls back to lowestBase - 1). Lifetime tied to the engine: opened
@@ -279,15 +287,16 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
279287
// below. Computed before the I/O loop or producer append.
280288
this.recoveredCommitBoundaryFsn = recovered.findLastFsnWithoutPayloadFlag(
281289
io.questdb.client.cutlass.qwp.protocol.QwpConstants.HEADER_OFFSET_FLAGS,
282-
io.questdb.client.cutlass.qwp.protocol.QwpConstants.FLAG_DEFER_COMMIT
290+
io.questdb.client.cutlass.qwp.protocol.QwpConstants.FLAG_DEFER_COMMIT,
291+
io.questdb.client.cutlass.qwp.protocol.QwpConstants.MAGIC_MESSAGE,
292+
io.questdb.client.cutlass.qwp.protocol.QwpConstants.HEADER_SIZE
283293
);
284294
if (publishedFsn >= 0 && recoveredCommitBoundaryFsn < publishedFsn) {
295+
this.recoveredOrphanTipFsn = publishedFsn;
285296
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.",
297+
+ "committed [commitBoundaryFsn={}, publishedFsn={}]. The tail belongs to an "
298+
+ "aborted transaction: it will never be transmitted and its slots are "
299+
+ "retired (trimmed) once every frame below it is server-acked.",
291300
publishedFsn - Math.max(recoveredCommitBoundaryFsn, -1L),
292301
recoveredCommitBoundaryFsn, publishedFsn);
293302
}
@@ -641,6 +650,15 @@ public long recoveredCommitBoundaryFsn() {
641650
return recoveredCommitBoundaryFsn;
642651
}
643652

653+
/**
654+
* FSN of the last frame of a recovered orphaned deferred tail, or
655+
* {@code -1} when none. See {@link #recoveredCommitBoundaryFsn()}: the
656+
* orphan range is {@code [recoveredCommitBoundaryFsn() + 1 .. this]}.
657+
*/
658+
public long recoveredOrphanTipFsn() {
659+
return recoveredOrphanTipFsn;
660+
}
661+
644662
/**
645663
* Unlinks every {@code .sfa} file under {@code dir}. Called only on
646664
* clean shutdown when the ring confirms every published FSN has been

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,18 @@ public final class CursorWebSocketSendLoop implements QuietCloseable {
275275
// to -1 once trySendOne has caught up past it. Used to count replay
276276
// frames without a per-frame branch on the steady-state path.
277277
private long replayTargetFsn = -1L;
278+
// Recovered orphaned deferred tail: frames [orphanSkipStartFsn ..
279+
// orphanSkipTipFsn] carry FLAG_DEFER_COMMIT with no covering commit frame
280+
// -- a transaction whose commit was never published (producer crashed or
281+
// closed mid-transaction). They must never reach the wire: a
282+
// commit-bearing frame from THIS session would commit them server-side
283+
// and resurrect a partial transaction. trySendOne stops the cursor at the
284+
// tail; tryRetireOrphanTail retires it with a cumulative self-acknowledge
285+
// once everything below is server-acked. -1 when none/retired. Written by
286+
// the constructor and start() (user thread, before the I/O thread exists)
287+
// and by the I/O thread afterwards -- never concurrently.
288+
private long orphanSkipStartFsn = -1L;
289+
private long orphanSkipTipFsn = -1L;
278290
// Poison-frame detector state (I/O thread only). poisonFsn is the FSN of the
279291
// frame implicated by the most recent server-active rejection: the NACK-named
280292
// frame, or the OK-level head-of-line frame (highestOkFsn+1) for a
@@ -478,6 +490,15 @@ public CursorWebSocketSendLoop(WebSocketClient client, CursorSendEngine engine,
478490
// hands null and lets the I/O thread connect — hasEverConnected
479491
// stays false until swapClient sees its first success.
480492
this.hasEverConnected = client != null;
493+
// Adopt the engine's recovered orphaned-deferred-tail range (if any).
494+
// See the field docs on orphanSkipStartFsn/orphanSkipTipFsn; the
495+
// BackgroundDrainer path builds its loop through this same
496+
// constructor, so drained orphan slots get the identical containment.
497+
long orphanTip = engine.recoveredOrphanTipFsn();
498+
if (orphanTip >= 0) {
499+
this.orphanSkipStartFsn = engine.recoveredCommitBoundaryFsn() + 1L;
500+
this.orphanSkipTipFsn = orphanTip;
501+
}
481502
}
482503

483504
/**
@@ -1759,6 +1780,11 @@ private void swapClient(WebSocketClient newClient) {
17591780
// best-effort
17601781
}
17611782
}
1783+
// A recovered orphaned deferred tail may be retirable now (e.g. the
1784+
// acks covering everything below it arrived just before the drop).
1785+
// Retiring before computing replayStart anchors the new connection
1786+
// past the tail instead of replaying into it.
1787+
tryRetireOrphanTail();
17621788
long replayStart = engine.ackedFsn() + 1L;
17631789
this.fsnAtZero = replayStart;
17641790
this.nextWireSeq = 0L;
@@ -1794,6 +1820,34 @@ private boolean tryReceiveAcks() {
17941820
* scheduling fairness.
17951821
*/
17961822
private boolean trySendOne() {
1823+
if (orphanSkipTipFsn >= 0 && fsnAtZero + nextWireSeq >= orphanSkipStartFsn) {
1824+
// The send cursor reached the orphaned deferred tail. Its frames
1825+
// belong to an aborted transaction and must never be transmitted
1826+
// (a commit-bearing frame from this session would commit them --
1827+
// partial-transaction resurrection).
1828+
if (!tryRetireOrphanTail()) {
1829+
// Frames below the tail still await server acks; keep ticking
1830+
// (tryReceiveAcks runs every loop iteration) until the ack
1831+
// watermark reaches the tail's lower edge.
1832+
return false;
1833+
}
1834+
if (nextWireSeq == 0) {
1835+
// Nothing sent on this connection yet: re-anchor in place past
1836+
// the retired tail. The wireSeq<->FSN mapping is untouched
1837+
// because no wire sequence has been consumed.
1838+
positionCursorForStart();
1839+
return true;
1840+
}
1841+
// Frames were already sent on this connection: the linear
1842+
// fsnAtZero + wireSeq mapping cannot express the gap the retired
1843+
// tail leaves behind. Recycle; swapClient re-anchors the mapping
1844+
// at ackedFsn + 1 = the first frame past the tail. One reconnect,
1845+
// once per recovery -- and only on the slow path (unacked
1846+
// committed frames existed below the tail).
1847+
fail(new LineSenderException(
1848+
"recycling connection after retiring orphaned deferred tail (wire-seq realignment)"));
1849+
return false;
1850+
}
17971851
long pub = sendingSegment.publishedOffset();
17981852
if (sendOffset >= pub) {
17991853
// Nothing more in the current segment. If it's a sealed segment
@@ -1859,12 +1913,53 @@ private boolean trySendOne() {
18591913
* real I/O thread + WebSocket.
18601914
*/
18611915
void positionCursorForStart() {
1916+
// Fast path for a recovered orphaned deferred tail: when everything
1917+
// below the tail is already acked (the common crash profile -- acks
1918+
// were flowing until the crash, only the in-flight transaction is
1919+
// unacked; and trivially when the WHOLE recovered log is the tail),
1920+
// the tail retires before any connection exists and the cursor
1921+
// starts past it. Zero wire cost, no recycle.
1922+
tryRetireOrphanTail();
18621923
long replayStart = engine.ackedFsn() + 1L;
18631924
this.fsnAtZero = replayStart;
18641925
this.nextWireSeq = 0L;
18651926
positionCursorAt(replayStart);
18661927
}
18671928

1929+
/**
1930+
* Retires the recovered orphaned deferred tail once retirable.
1931+
* <p>
1932+
* The tail's frames all carry FLAG_DEFER_COMMIT with no covering commit
1933+
* frame: the producer died (or closed) mid-transaction, so the
1934+
* transaction is aborted by definition. Retirement is a cumulative
1935+
* self-acknowledge of the tail's top FSN -- legal once every frame BELOW
1936+
* the tail is server-acked, because the tail frames were never
1937+
* transmitted and no wire sequence references them. The freed slots are
1938+
* trimmed by the normal ack-driven machinery.
1939+
* <p>
1940+
* Crash-safe: dying before the self-ack leaves the tail in place; the
1941+
* next recovery re-detects the same range and retries. Idempotent once
1942+
* retired.
1943+
*
1944+
* @return true when no orphan tail remains (retired now, previously, or
1945+
* never existed); false while frames below the tail are unacked
1946+
*/
1947+
private boolean tryRetireOrphanTail() {
1948+
if (orphanSkipTipFsn < 0) {
1949+
return true;
1950+
}
1951+
if (engine.ackedFsn() < orphanSkipStartFsn - 1L) {
1952+
return false;
1953+
}
1954+
LOG.warn("retiring orphaned deferred tail: {} frame(s) [fsn {}..{}] belong to a transaction "
1955+
+ "whose commit was never published; aborting them (never transmitted, slots trimmed)",
1956+
orphanSkipTipFsn - orphanSkipStartFsn + 1, orphanSkipStartFsn, orphanSkipTipFsn);
1957+
engine.acknowledge(orphanSkipTipFsn);
1958+
orphanSkipStartFsn = -1L;
1959+
orphanSkipTipFsn = -1L;
1960+
return true;
1961+
}
1962+
18681963
/**
18691964
* Factory used by the I/O loop to build a fresh, connected, upgraded
18701965
* {@link WebSocketClient} after a wire failure. Implementations close

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -446,23 +446,36 @@ public long tryAppend(long payloadAddr, int payloadLen) {
446446
/**
447447
* Walks every published frame in this segment and returns the FSN of the
448448
* 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.
449+
* when every frame carries it (or the segment is empty).
450+
* <p>
451+
* A frame counts as carrying the flag ONLY when it positively parses as a
452+
* message of the expected protocol: payload at least {@code minPayloadLen}
453+
* bytes AND the little-endian u32 at payload offset 0 equals
454+
* {@code headerMagic} AND the byte at {@code flagsOffset} has
455+
* {@code flagMask} set. Anything else -- short frames, foreign payloads,
456+
* magic mismatches -- counts as NOT carrying the flag. This direction is
457+
* deliberate: the caller retires (trims) frames ABOVE the returned FSN,
458+
* so a frame we cannot positively identify must act as a retirement
459+
* barrier, never as trimmable. Misclassifying an unknown frame as
460+
* deferred would silently discard data that should replay.
451461
* <p>
452462
* Producer-thread only, and only meaningful before new appends race the
453463
* walk (recovery time). Used to locate the last commit-bearing QWP frame
454464
* below a potentially orphaned FLAG_DEFER_COMMIT tail: frames above the
455465
* returned FSN all carry the flag, i.e. they belong to a transaction whose
456466
* commit frame was never published.
457467
*/
458-
public long findLastFrameFsnWithoutPayloadFlag(int flagsOffset, int flagMask) {
468+
public long findLastFrameFsnWithoutPayloadFlag(int flagsOffset, int flagMask, int headerMagic, int minPayloadLen) {
459469
long best = -1L;
460470
long off = HEADER_SIZE;
461471
long frames = frameCount;
462472
for (long i = 0; i < frames; i++) {
463473
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;
474+
long payload = mmapAddress + off + FRAME_HEADER_SIZE;
475+
boolean flagSet = payloadLen >= minPayloadLen
476+
&& payloadLen > flagsOffset
477+
&& Unsafe.getUnsafe().getInt(payload) == headerMagic
478+
&& (Unsafe.getUnsafe().getByte(payload + flagsOffset) & flagMask) != 0;
466479
if (!flagSet) {
467480
best = baseSeq + i;
468481
}

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -521,17 +521,21 @@ public synchronized MmapSegment firstSealed() {
521521
* potentially orphaned FLAG_DEFER_COMMIT tail left behind by a producer
522522
* that crashed (or closed) mid-transaction. Call before the I/O loop and
523523
* producer start appending; the walk is not synchronized against appends
524-
* into the active segment.
524+
* into the active segment. See
525+
* {@link MmapSegment#findLastFrameFsnWithoutPayloadFlag} for the
526+
* positive-identification contract: frames that do not parse as protocol
527+
* messages count as commit-bearing (retirement barriers), never as
528+
* trimmable.
525529
*/
526-
public synchronized long findLastFsnWithoutPayloadFlag(int flagsOffset, int flagMask) {
530+
public synchronized long findLastFsnWithoutPayloadFlag(int flagsOffset, int flagMask, int headerMagic, int minPayloadLen) {
527531
long best = -1L;
528532
for (int i = 0, n = sealedSegments.size(); i < n; i++) {
529-
long fsn = sealedSegments.get(i).findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask);
533+
long fsn = sealedSegments.get(i).findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask, headerMagic, minPayloadLen);
530534
if (fsn > best) {
531535
best = fsn;
532536
}
533537
}
534-
long fsn = active.findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask);
538+
long fsn = active.findLastFrameFsnWithoutPayloadFlag(flagsOffset, flagMask, headerMagic, minPayloadLen);
535539
return Math.max(best, fsn);
536540
}
537541

0 commit comments

Comments
 (0)