Skip to content

Commit 958a36d

Browse files
committed
fix(qwp): clear transient sync latch, unpin startup recovery, capture win32 errno at failure point
Review-finding fixes: - C1: SegmentManager.servicePeriodicSync now clears SegmentRing.durabilityFailure after a subsequent fully-successful sync pass, so a transient fsync/msync failure no longer permanently bricks the producer; recovery is logged like the trim path. Regression test: testTransientSyncFailureClearsOnNextSuccess. - C2: startup SF recovery parks slots whose build persistently fails (immediately on SlotLockContentionException, after 3 consecutive failures otherwise), keeps scanning the remaining slots, rewinds the cursors while parked slots remain, and dedups the per-slot WARN. A wedged slot no longer livelocks the driver, floods the log, or starves higher slots' orphan data; the server-wide transient retry contract is preserved. 3 new regression tests in SenderPoolSfTest. - M1: windows open_file saves GetLastError() at the CreateFileW failure point (before free() can clobber it) and fsyncDir0 classifies the ERROR_ACCESS_DENIED best-effort degrade from the saved value via the new GetSavedLastError() (defined in os.c beside the TLS index it reads), mirroring the FlushFileBuffers branch's capture pattern. - M2: replaced leftover test reflection with the existing @testonly seams in CursorSendEngineSlotReacquisitionTest, QueryClientPoolErrorSafetyTest and SlotLockReleasedContractTest. - Stale comments updated: the durability-latch throw is transient, and the recovery streak constant parks on the 3rd consecutive failure.
1 parent 80ad4ca commit 958a36d

12 files changed

Lines changed: 536 additions & 83 deletions

File tree

core/src/main/c/windows/errno.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,10 @@ static DWORD dwTlsIndexLastError = 0;
66

77
void SaveLastError();
88

9+
/* Returns the per-thread error code most recently stored by SaveLastError().
10+
* Both functions are defined in os.c, the only translation unit whose static
11+
* dwTlsIndexLastError copy is initialised by TlsAlloc() in DllMain; every
12+
* other includer's copy of the variable is an unused zero. */
13+
DWORD GetSavedLastError();
14+
915
#endif //ZLIB_ERRNO_H

core/src/main/c/windows/files.c

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,16 @@ static jint open_file(const char *utf8Path,
7777
}
7878
HANDLE h = CreateFileW(wide, desiredAccess, shareMode, NULL,
7979
creationDisposition, flagsAndAttributes, NULL);
80-
free(wide);
8180
if (h == INVALID_HANDLE_VALUE) {
81+
// Save the CreateFileW failure code BEFORE free(): the thread's
82+
// last-error value may be overwritten by any subsequent API/CRT call
83+
// (HeapFree can SetLastError), and callers such as fsyncDir0 rely on
84+
// the saved value being the CreateFileW error.
8285
SaveLastError();
86+
free(wide);
8387
return -1;
8488
}
89+
free(wide);
8590
return HANDLE_TO_FD(h);
8691
}
8792

@@ -237,10 +242,12 @@ JNIEXPORT jint JNICALL Java_io_questdb_client_std_Files_fsyncDir0
237242
// directory handle with ERROR_ACCESS_DENIED. Directory-entry durability
238243
// on NTFS is provided by metadata journaling ($LogFile), so degrade to
239244
// best-effort success here rather than hard-failing the SF durability
240-
// path. open_file() already recorded the error via SaveLastError(); a
241-
// genuine failure such as a missing directory (ERROR_PATH_NOT_FOUND)
242-
// still propagates as fatal.
243-
if (GetLastError() == ERROR_ACCESS_DENIED) {
245+
// path. Consult the error open_file() saved at the CreateFileW failure
246+
// point rather than the live GetLastError(): the intervening free()
247+
// and return path inside open_file() are not guaranteed to preserve
248+
// the thread's last-error value. A genuine failure such as a missing
249+
// directory (ERROR_PATH_NOT_FOUND) still propagates as fatal.
250+
if (GetSavedLastError() == ERROR_ACCESS_DENIED) {
244251
return 0;
245252
}
246253
return -1;

core/src/main/c/windows/os.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,3 +237,7 @@ JNIEXPORT jlong JNICALL Java_io_questdb_client_std_Os_currentTimeMicros
237237
void SaveLastError() {
238238
TlsSetValue(dwTlsIndexLastError, (LPVOID) (DWORD_PTR) GetLastError());
239239
};
240+
241+
DWORD GetSavedLastError() {
242+
return (DWORD) (DWORD_PTR) TlsGetValue(dwTlsIndexLastError);
243+
}

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -869,9 +869,11 @@ public boolean awaitAckedFsn(long targetFsn, long timeoutMillis) {
869869
return targetFsn < 0L;
870870
}
871871
cursorEngine.checkDurability();
872-
// Surface latched I/O errors before any early-return path, so a
873-
// caller polling with timeoutMillis <= 0 to drive their own loop
874-
// sees the terminal throw instead of an indefinite "not yet".
872+
// Surface latched errors before any early-return path, so a caller
873+
// polling with timeoutMillis <= 0 to drive their own loop sees the
874+
// throw instead of an indefinite "not yet". The durability latch
875+
// above is transient: it throws while latched, and clears once a
876+
// later periodic sync pass fully succeeds so producers can resume.
875877
if (cursorSendLoop != null) {
876878
cursorSendLoop.checkError();
877879
}

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1224,8 +1224,15 @@ private void servicePeriodicSync(RingEntry e, long now) {
12241224
syncScratch.getQuick(i).syncPublished();
12251225
}
12261226
e.ring.clearSyncRequestIfActiveDurable();
1227+
// The pass above covered every live segment's published range, so
1228+
// any earlier barrier failure has been remedied — unlatch it so a
1229+
// transient disk fault doesn't permanently brick the producer.
1230+
e.ring.clearDurabilityFailure();
12271231
e.nextDataSyncNanos = now + e.syncIntervalNanos;
1228-
e.syncFailureLogged = false;
1232+
if (e.syncFailureLogged) {
1233+
e.syncFailureLogged = false;
1234+
LOG.info("Periodic SF data sync recovered for {}", e.dir);
1235+
}
12291236
} catch (Throwable failure) {
12301237
e.ring.recordDurabilityFailure(failure);
12311238
if (!e.syncFailureLogged) {

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,10 @@ public final class SegmentRing implements QuietCloseable {
118118
// call installHotSpare on a closed ring (whose hotSpare was just
119119
// zeroed by close()) -- the spare's mmap + fd would never be reclaimed.
120120
private boolean closed;
121-
// First periodic data-barrier failure. The manager latches it and the
122-
// producer observes it before its next append.
121+
// Latest periodic data-barrier failure. The manager latches it and the
122+
// producer observes it before its next append; the manager clears it
123+
// again once a subsequent periodic sync pass succeeds, so a transient
124+
// disk fault does not permanently brick the producer.
123125
private volatile MmapSegmentException durabilityFailure;
124126
// hotSpare: written by segment manager (installHotSpare), read+cleared by
125127
// producer thread on rotation. Volatile so the producer sees fresh installs.
@@ -961,6 +963,22 @@ public void checkDurability() {
961963
}
962964
}
963965

966+
/**
967+
* Clears a latched periodic data-barrier failure. Called by the segment
968+
* manager after a subsequent periodic sync pass over every live segment
969+
* succeeds: the published range the failed barrier was meant to cover is
970+
* durable now, so the producer may resume. The monitor serializes the
971+
* clear with {@link #recordDurabilityFailure(Throwable)}; the volatile
972+
* write publishes it to producer threads calling {@link #checkDurability()}.
973+
*/
974+
void clearDurabilityFailure() {
975+
if (durabilityFailure != null) {
976+
synchronized (this) {
977+
durabilityFailure = null;
978+
}
979+
}
980+
}
981+
964982
synchronized void clearSyncRequestIfActiveDurable() {
965983
if (active != null && active.isPublishedDurable()) {
966984
syncRequested = false;

0 commit comments

Comments
 (0)