Skip to content

Commit 87320e5

Browse files
bluestreak01claude
andcommitted
fix(ilp): three SF recovery correctness fixes from PR-17 review
Three independent recovery-time bugs in SegmentLog that all let a durability layer silently produce or operate on a wrong view of the on-disk log. Each fix has a red regression test that fails on the unfixed code and passes after the fix. 1. Mid-rotate crash recovery resets FSN sequence to 0. rotate() has a window between ff.rename(.sfa → .sfs) and the subsequent createActive(lastSeq + 1) where the process can die or createActive can throw (allocNativePath OOM, openCleanRW failure, etc.) leaving on disk: one or more sealed .sfs files, no .sfa. openInternal saw active==null after scanDirectory and unconditionally called createActive(FIRST_SEQ=0), restarting FSN assignment at 0 even though sealed segments on disk already covered 0..N. The new active produced frames whose FSNs collided with sealed FSNs already on disk, breaking ACK translation, trim, and replay against data the recovery never saw. Fix derives the new active's baseSeq from the highest sealed lastSeqOnDisk + 1 (segments is sorted by baseSeq and sealed ranges are non-overlapping, so the last entry holds the largest lastSeqOnDisk). Tests: - testMidRotateCrashRecoveryPreservesFsnMonotonicity (fault injection: failNextActiveAllocNativePath inside rotate()). - testRestartWithOnlySealedSegmentsRecoversCorrectly (independent coverage via pure on-disk filesystem manipulation — write frames, manually rename .sfa to .sfs — to exercise the open/recovery code in isolation from rotate's failure handling, then verify the full contract: nextSeq, oldestSeq, replay order, and post-restart append). 2. oldestSeq() returned a removePending segment's baseSeq even though replay() skips it. trim() keeps an undeletable sealed segment in the in-memory list as removePending; replay() correctly skips such segments so already- acked frames are not re-shipped on reconnect. oldestSeq() returned segments.getQuick(0).baseSeq unconditionally — including when the first segment was removePending. WebSocketSendQueue pins fsnAtZero = oldestSeq() in both the constructor (line 247-248) and doReconnectCycle (line 925-926), then asserts fsn == fsnAtZero + wireSeq inside the replay visitor (line 974). The mismatch threw "SF replay FSN drift" on the first replayed frame; the catch triggered failConnection(non-fatal); reconnectRequested fired; the I/O loop re-entered doReconnectCycle, called oldestSeq() again with the same stale return, and drift fired identically. Permanent reconnect loop until either the FS issue cleared AND a non-reconnect trim ran (it can't — the I/O thread is stuck reconnecting), or the user closed the sender. Fix skips removePending in oldestSeq() the same way replay() does. Tests: - testOldestSeqMustSkipRemovePendingToMatchReplay (unit-level: cross- check oldestSeq() against the first FSN replay() actually visits). - testReplaySucceedsWithRemovePendingSegmentAtHeadOfList (end-to-end integration: real TestWebSocketServer + sender + RemoveFailingSf Facade; verified pre-fix to reproduce the reconnect loop with "SF replay FSN drift: fsn=2 expected=0", post-fix the 2 unacked frames replay successfully and a fresh send reaches the server). 3. Directory scan errors silently treated as EOF / empty log. Files.findNext()'s contract is 1=success, 0=EOF, -1=read error. scanDirectory's while (rc > 0) loop exited identically on both 0 and -1, conflating a real readdir failure (EIO/ESTALE on NFS, etc.) with normal end-of-directory. Files.findFirst()==0 means either opendir failed (errno set — transient EACCES/EMFILE/ESTALE/ENOMEM) or the directory is empty; scanDirectory unconditionally treated it as "nothing to scan." By the time scanDirectory runs, openInternal has created the directory if missing and successfully opened+locked the lock file inside it, so an empty listing is impossible — find==0 here can only mean opendir failed. The silent fallthrough let openInternal proceed to createActive(...) on top of any unscanned on-disk segments, aliasing or overwriting still-existing data — the exact failure mode a durability layer must guard against. Fix throws SfException in both branches; recovery refuses to proceed from a partial / unknown view of its own log. Tests: - testScanDirectoryFailsWhenFindFirstReturnsZero (FilesFacade forces findFirst to return 0; pre-fix open silently succeeded with empty segments and nextSeq=0 over real on-disk data). - testScanDirectoryFailsWhenFindNextReturnsError (FilesFacade forces findNext to return -1; same shape, mid-scan readdir failure is now fatal). Full module suite: 1994/1994 green (1988 baseline + 6 new tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 83bb368 commit 87320e5

3 files changed

Lines changed: 971 additions & 10 deletions

File tree

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

Lines changed: 59 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -394,17 +394,31 @@ private void trimSealedSegments(long ackedSeq) {
394394
}
395395
}
396396

397-
/** Lowest seq currently on disk, or -1 if log is empty. */
397+
/**
398+
* Lowest seq currently on disk that {@link #replay} will visit, or -1 if
399+
* none. Must skip {@code removePending} segments — replay() does the same
400+
* (line 277), and {@code WebSocketSendQueue.doReconnectCycle} pins
401+
* {@code fsnAtZero} to this value before invoking replay. A mismatch here
402+
* trips the "SF replay FSN drift" guard inside the replay visitor and
403+
* aborts every reconnect attempt, turning a transient remove() failure
404+
* into a permanent reconnect loop.
405+
*/
398406
public long oldestSeq() {
399407
ensureOpen();
400-
if (segments.size() == 0) {
401-
return -1;
402-
}
403-
Segment first = segments.getQuick(0);
404-
if (first.frameCount == 0) {
405-
return -1;
408+
for (int i = 0, n = segments.size(); i < n; i++) {
409+
Segment s = segments.getQuick(i);
410+
if (s.removePending) {
411+
continue;
412+
}
413+
if (s.frameCount == 0) {
414+
// Empty segment can only be the tail active (sealed segments
415+
// always carry frames — rotate drops empty ones). Nothing
416+
// after this is replay-visible.
417+
return -1;
418+
}
419+
return s.baseSeq;
406420
}
407-
return first.baseSeq;
421+
return -1;
408422
}
409423

410424
/** Sequence number that will be assigned to the next {@link #append}. */
@@ -503,15 +517,41 @@ private void openInternal() {
503517

504518
scanDirectory();
505519
if (active == null) {
506-
createActive(FIRST_SEQ);
520+
// Mid-rotate crash recovery: rotate() has a window between
521+
// ff.rename(.sfa → .sfs) and createActive(lastSeq + 1) where the
522+
// process can die (or createActive can throw, leaving the .sfa
523+
// removed by its own catch block) with sealed segments on disk
524+
// and no active. Resuming at FIRST_SEQ here would let the next
525+
// session's appends produce frames whose FSNs collide with FSNs
526+
// already on disk in the sealed segments, breaking ACK
527+
// translation, trim, and replay. Pick up past the highest sealed
528+
// lastSeqOnDisk instead. scanDirectory sorts segments by baseSeq
529+
// and sealed segments cover non-overlapping FSN ranges, so the
530+
// last entry holds the largest lastSeqOnDisk.
531+
long resumeFrom = FIRST_SEQ;
532+
int n = segments.size();
533+
if (n > 0) {
534+
resumeFrom = segments.getQuick(n - 1).lastSeqOnDisk + 1;
535+
}
536+
createActive(resumeFrom);
507537
}
508538
nextSeq = active.baseSeq + active.frameCount;
509539
}
510540

511541
private void scanDirectory() {
512542
long find = ff.findFirst(dir);
513543
if (find == 0) {
514-
return;
544+
// findFirst returns 0 for either "directory could not be opened"
545+
// (errno set — transient EACCES/EMFILE/ESTALE/ENOMEM) or
546+
// "directory is empty." By the time we get here, openInternal has
547+
// created the directory if missing AND opened+locked the lock
548+
// file inside it, so an empty listing is impossible — find==0
549+
// here can only mean opendir failed. Treating it as "nothing to
550+
// scan" would let openInternal proceed to createActive(...) on
551+
// top of any unscanned on-disk segments, silently aliasing or
552+
// overwriting still-existing data. A durability layer must
553+
// refuse to proceed from an unknown view of its own log.
554+
throw new SfException("findFirst failed for SF directory: " + dir);
515555
}
516556
try {
517557
int rc = 1;
@@ -526,6 +566,15 @@ private void scanDirectory() {
526566
}
527567
rc = ff.findNext(find);
528568
}
569+
if (rc < 0) {
570+
// findNext == -1 is a readdir read error (EIO/ESTALE on NFS,
571+
// etc.). The in-memory `segments` list is now a partial view
572+
// of what's on disk. Same hazard as findFirst==0: subsequent
573+
// createActive(...) or appends would alias unscanned on-disk
574+
// segments. Refuse rather than recover from an unknown
575+
// partial state.
576+
throw new SfException("findNext failed mid-scan of SF directory: " + dir);
577+
}
529578
} finally {
530579
ff.findClose(find);
531580
}

0 commit comments

Comments
 (0)