From b50529dcac8cbd3a157a4131d13513602b919b7c Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 12:17:55 +0100 Subject: [PATCH 01/12] fix(qwp): make engine close a worker-quiescence barrier before releasing slot resources SegmentManager.close() gives up after a bounded join, but CursorSendEngine.close() could not observe the incomplete shutdown: it closed the ring and watermark, unlinked segment files and released the slot flock while the worker could still be mid service pass - able to unlink a spare/trim path inside a slot directory that a replacement engine had already re-acquired (SF data loss after restart). - serviceRing now claims the entry as in-service under the manager lock and skips entries deregistered before the pass starts - new SegmentManager.awaitRingQuiescence(ring): bounded, interrupt-preserving barrier that confirms the worker can never touch the ring/slot again - CursorSendEngine.close() releases ring/watermark/segment files/slot lock only after confirmed quiescence (or a reaped owned worker); otherwise it leaks them deliberately, logs, and allows close() to be retried - a timed-out SegmentManager.close() hands pathScratch ownership to the worker, which frees it on exit - no permanent native leak when the worker outlives the join - regression: CursorSendEngineSlotReacquisitionTest (slot retained while worker mid-pass + retry completes cleanup; same-slot reacquisition after normal close) and an awaitRingQuiescence contract test --- .../client/sf/cursor/CursorSendEngine.java | 69 +++++- .../qwp/client/sf/cursor/SegmentManager.java | 200 +++++++++++++--- ...CursorSendEngineSlotReacquisitionTest.java | 218 ++++++++++++++++++ .../cursor/SegmentManagerCloseRaceTest.java | 75 ++++++ 4 files changed, 527 insertions(+), 35 deletions(-) create mode 100644 core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java index 64ca75d0..379b5dbe 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java @@ -114,6 +114,12 @@ public final class CursorSendEngine implements QuietCloseable { // thread, JVM shutdown hooks, test cleanup). volatile + synchronized // close() makes the check-and-set atomic and gives readers a fence. private volatile boolean closed; + // True once close() has run its full cleanup sequence. Stays false when + // a close attempt could not confirm manager-worker quiescence and had to + // leak the ring/watermark/slot lock — in that case a later close() call + // retries the cleanup (the worker may have exited by then). Guarded by + // the synchronized close() method; never read elsewhere. + private boolean closeCompleted; // Producer-thread-only: timestamp of the last "we're backpressured" log // line, used to throttle. Plain long is fine. private long lastBackpressureLogNs; @@ -480,7 +486,7 @@ public long appendOrFsn(long payloadAddr, int payloadLen, long spinDeadlineNanos @Override public synchronized void close() { - if (closed) return; + if (closed && closeCompleted) return; closed = true; // Capture drain state BEFORE closing the ring — once the ring is // closed, its accessors aren't safe to read. The active segment is @@ -492,10 +498,14 @@ public synchronized void close() { // the server has no dedup state for those messageSequences. // Memory mode has no files to unlink. // The whole close sequence runs under try/finally so the slot lock - // is ALWAYS released, even if manager/ring close or unlink throws — - // otherwise a kernel-held flock outlives the engine and the next - // sender for the same slot collides on a lock the dead engine - // never released. + // is released whenever it is safe to do so, even if manager/ring + // close or unlink throws — otherwise a kernel-held flock outlives + // the engine and the next sender for the same slot collides on a + // lock the dead engine never released. The one deliberate exception + // is a manager worker that failed to quiesce: releasing the lock + // then would let a replacement engine acquire the slot while the + // stale worker can still create/unlink segment paths inside it. + boolean workerQuiescent = false; try { // "Fully drained" includes BOTH the obvious case (every published // FSN has been acked) AND the never-published case (publishedFsn @@ -504,9 +514,15 @@ public synchronized void close() { // recreates a fresh sf-initial.sfa — would otherwise leave that // fresh empty file behind, the next scanner finds it, adopts the // slot again, and the cycle repeats forever (M6). - boolean fullyDrained = sfDir != null - && (ring.publishedFsn() < 0 - || ring.ackedFsn() >= ring.publishedFsn()); + // Own try/catch so sabotaged/broken ring state cannot skip the + // quiescence barrier below or the slot-lock release in finally. + boolean fullyDrained = false; + try { + fullyDrained = sfDir != null + && (ring.publishedFsn() < 0 + || ring.ackedFsn() >= ring.publishedFsn()); + } catch (Throwable ignored) { + } // Each cleanup step in its own try/catch so a single failure // doesn't strand later cleanups — mirrors the constructor's // catch block. Without this, a throw from manager.deregister @@ -517,11 +533,41 @@ public synchronized void close() { manager.deregister(ring); } catch (Throwable ignored) { } + // Quiescence barrier. deregister alone only removes the entry + // from the registry — the worker may still be mid service pass + // for this ring (creating a spare file, trimming, unlinking). + // Releasing the ring, watermark, segment files, or the slot lock + // while that pass is in flight lets the stale worker unlink a + // path that a replacement engine — which can acquire the slot + // the moment the lock is released — is actively writing through: + // store-and-forward data loss after restart. Wait for confirmed + // quiescence before touching anything the worker can reach. + try { + workerQuiescent = manager.awaitRingQuiescence(ring); + } catch (Throwable ignored) { + } if (ownsManager) { try { manager.close(); } catch (Throwable ignored) { } + if (!workerQuiescent) { + // manager.close() joins the worker; a reaped (or + // never-started) worker is an even stronger barrier + // than per-ring quiescence. + try { + workerQuiescent = manager.isWorkerReaped(); + } catch (Throwable ignored) { + } + } + } + if (!workerQuiescent) { + LOG.error("SF manager worker did not quiesce during engine close; leaking the " + + "ring, watermark and slot lock so a stale worker cannot corrupt a " + + "future engine on slot {}. The kernel releases the slot flock on " + + "process exit; close() may be invoked again to retry cleanup once " + + "the worker has exited.", sfDir == null ? "" : sfDir); + return; } try { ring.close(); @@ -551,8 +597,13 @@ public synchronized void close() { } catch (Throwable ignored) { } } + closeCompleted = true; } finally { - if (slotLock != null) { + // Gate on quiescence: releasing the flock while a stale worker + // can still touch this slot directory hands the slot to a new + // engine that the worker may then corrupt. Leaking the fd is + // the safe failure mode — the kernel drops it on process exit. + if (workerQuiescent && slotLock != null) { try { slotLock.close(); } catch (Throwable ignored) { diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java index 3b7cff60..6548fdef 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java @@ -100,15 +100,34 @@ public final class SegmentManager implements QuietCloseable { // registered flag check inside the trim block closes for watermark writes // and totalBytes accounting. private volatile Runnable beforeTrimSyncHook; + // Entry currently being serviced by the worker thread, or null when the + // worker is between service passes (or not running). Guarded by + // {@link #lock}; cleared with lock.notifyAll() so awaitRingQuiescence can + // block until an in-flight pass for a just-deregistered ring finishes. + private RingEntry inService; private long lastDiskFullLogNs; private volatile boolean running; + // pathScratch free-exactly-once coordination between a timed-out close() + // and the worker's exit path. All three are guarded by {@link #lock}. + // When close() gives up on the join while the worker loop has not yet + // exited, it hands scratch ownership to the worker + // (scratchHandedToWorker=true) and the worker frees the buffer in its + // exit block; in every other case close() frees it. Without the handoff, + // a worker that outlives the bounded join leaks the native scratch + // buffer forever, because nobody retries manager cleanup after close() + // returns. + private boolean scratchFreed; + private boolean scratchHandedToWorker; + private boolean workerLoopExited; // Total bytes currently allocated across every segment owned by every // registered ring (active + sealed + hot-spare). Mutated by the manager // thread on provision/trim and by register/deregister callers under // {@link #lock}; the lock covers both paths so the counter stays // consistent across registration boundaries. private long totalBytes; - private long workerJoinTimeoutMillis = WORKER_JOIN_TIMEOUT_MILLIS; + // volatile: read by awaitRingQuiescence() from arbitrary caller threads + // while the @TestOnly setter may run on another. + private volatile long workerJoinTimeoutMillis = WORKER_JOIN_TIMEOUT_MILLIS; // volatile because wakeWorker() reads workerThread without holding the // monitor; the synchronized start()/close() pair handles the // start-vs-close ordering. @@ -194,18 +213,100 @@ public synchronized void close() { } } if (t.isAlive()) { - LOG.warn("SegmentManager worker did not stop before close wait completed; " - + "leaving worker-owned resources allocated"); - return; + synchronized (lock) { + if (!workerLoopExited) { + // Hand pathScratch ownership to the worker: its exit + // block frees the buffer under the same lock, so the + // native allocation is reclaimed even though this + // close() could not confirm termination. workerThread + // stays set so isWorkerReaped() reports the incomplete + // shutdown and a later close() can retry the join. + scratchHandedToWorker = true; + LOG.warn("SegmentManager worker did not stop before close wait completed; " + + "worker frees its native scratch buffer on exit"); + return; + } + } + // The worker loop has already run its exit block; the thread + // is at most a few instructions from terminating and can no + // longer touch manager state. Fall through and reap it. } workerThread = null; } // Free the rotation-path native scratch buffer only after worker - // termination has been observed. The worker is the only thread that - // touches the buffer, but close() uses a bounded join; if the worker is - // still alive, leaking this one scratch allocation is safer than freeing - // native memory it may still read or write. - pathScratch.close(); + // termination (or worker-loop exit) has been observed. The worker is + // the only thread that touches the buffer; the scratchFreed flag + // (shared with the worker's exit block) makes the free exactly-once + // no matter which side runs last. + synchronized (lock) { + if (!scratchFreed) { + scratchFreed = true; + pathScratch.close(); + } + } + } + + /** + * Quiescence barrier for {@link #deregister(SegmentRing)}. Blocks until + * the worker thread is provably no longer executing a service pass for + * {@code ring}, or the worker-join timeout elapses. After this returns + * {@code true}, the worker will never again touch the ring, its + * watermark, or path names under its slot directory: deregister has + * removed the entry from the registry, a stale snapshot entry that has + * not started its pass is skipped by the registration check at the top + * of {@link #serviceRing(RingEntry)}, and this method has observed the + * end of any in-flight pass. Only then may the caller release dependent + * resources (ring, watermark, segment files, slot lock). + *

+ * Returns {@code true} immediately when no worker is running or when + * called from the worker thread itself (test hooks inject deregister + * calls there; waiting would self-deadlock). Returns {@code false} when + * the in-flight pass did not finish within the timeout — the caller must + * treat the worker as still live and leak rather than release. + *

+ * A pending caller interrupt is preserved but does not abort the wait, + * mirroring {@link #close()}. + */ + public boolean awaitRingQuiescence(SegmentRing ring) { + Thread t = workerThread; + if (t == null || t == Thread.currentThread()) { + return true; + } + long deadlineNanos = System.nanoTime() + workerJoinTimeoutMillis * 1_000_000L; + boolean interrupted = Thread.interrupted(); + try { + synchronized (lock) { + while (inService != null && inService.ring == ring) { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) { + return false; + } + try { + // Round up so a sub-millisecond remainder still waits + // instead of spinning through wait(0) == wait-forever. + lock.wait(Math.max(1L, remainingNanos / 1_000_000L)); + } catch (InterruptedException ignored) { + interrupted = true; + } + } + } + return true; + } finally { + if (interrupted) { + Thread.currentThread().interrupt(); + } + } + } + + /** + * True when no manager worker thread can be running: either + * {@link #start()} was never called, or a {@link #close()} confirmed + * worker termination and reaped the thread. Owners use this as a + * stronger fallback barrier when {@link #awaitRingQuiescence(SegmentRing)} + * times out but a subsequent {@code close()} join succeeded. + */ + public synchronized boolean isWorkerReaped() { + return workerThread == null; } /** @@ -213,6 +314,13 @@ public synchronized void close() { * created after this returns, but already-installed spares stay with * the ring (the ring closes them on its own {@link SegmentRing#close}). * Idempotent; safe to call from any thread. + *

+ * Non-blocking: a worker service pass already in flight for this ring + * may still be running when this returns. Callers about to release + * resources the worker can reach (the ring itself, its watermark, its + * segment files, or the slot lock guarding its directory) MUST follow + * up with {@link #awaitRingQuiescence(SegmentRing)} and only release on + * a {@code true} result. */ public void deregister(SegmentRing ring) { synchronized (lock) { @@ -412,6 +520,32 @@ private String nextSparePath(String dir) { } private void serviceRing(RingEntry e) { + // Claim the entry as in-service so deregister-side quiescence + // barriers (awaitRingQuiescence) can wait for this pass to finish. + // A stale snapshot entry deregistered before the pass starts is + // skipped entirely: the deregistering thread may already be + // releasing the ring / watermark / slot resources, so the worker + // must not touch them at all. The registered check and the + // in-service claim are atomic under `lock` — deregister flips + // `registered` under the same lock, so it either prevents this + // pass or the barrier observes it via `inService`. + synchronized (lock) { + if (!e.registered) { + return; + } + inService = e; + } + try { + serviceRing0(e); + } finally { + synchronized (lock) { + inService = null; + lock.notifyAll(); + } + } + } + + private void serviceRing0(RingEntry e) { // 1. Provision a hot spare if the ring needs one AND we have headroom // under the disk-total cap. Cap check is per-tick; if we're capped // here, the ring stays in BACKPRESSURE_NO_SPARE until trim (step 2) @@ -571,26 +705,40 @@ private void serviceRing(RingEntry e) { } private void workerLoop() { - while (running) { - // Snapshot the rings under the lock so we don't hold it through the - // (potentially slow) syscalls during creation/unlink. ringSnapshot - // is a thread-confined field — no per-tick allocation. - ringSnapshot.clear(); - synchronized (lock) { - for (int i = 0, n = rings.size(); i < n; i++) { - ringSnapshot.add(rings.getQuick(i)); + try { + while (running) { + // Snapshot the rings under the lock so we don't hold it through the + // (potentially slow) syscalls during creation/unlink. ringSnapshot + // is a thread-confined field — no per-tick allocation. + ringSnapshot.clear(); + synchronized (lock) { + for (int i = 0, n = rings.size(); i < n; i++) { + ringSnapshot.add(rings.getQuick(i)); + } } - } - for (int i = 0, n = ringSnapshot.size(); i < n; i++) { + for (int i = 0, n = ringSnapshot.size(); i < n; i++) { + if (!running) break; + serviceRing(ringSnapshot.getQuick(i)); + } + // Drop strong refs so a deregistered ring becomes collectable + // before the next tick (otherwise the snapshot pins it for up + // to pollNanos after deregister). + ringSnapshot.clear(); if (!running) break; - serviceRing(ringSnapshot.getQuick(i)); + LockSupport.parkNanos(pollNanos); + } + } finally { + // If a timed-out close() abandoned the reap, it handed + // pathScratch ownership to this thread (see close()). Freeing it + // here reclaims the native buffer even when the worker outlives + // every close() attempt — nobody else retries manager cleanup. + synchronized (lock) { + workerLoopExited = true; + if (scratchHandedToWorker && !scratchFreed) { + scratchFreed = true; + pathScratch.close(); + } } - // Drop strong refs so a deregistered ring becomes collectable - // before the next tick (otherwise the snapshot pins it for up - // to pollNanos after deregister). - ringSnapshot.clear(); - if (!running) break; - LockSupport.parkNanos(pollNanos); } } diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java new file mode 100644 index 00000000..2b2ce724 --- /dev/null +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java @@ -0,0 +1,218 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2026 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.client.test.cutlass.qwp.client.sf.cursor; + +import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentManager; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLock; +import io.questdb.client.std.Files; +import io.questdb.client.test.tools.TestUtils; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.file.Paths; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +/** + * Engine-level regression for the shutdown hazard where + * {@link CursorSendEngine#close()} released the slot lock, closed the ring + * and watermark, and unlinked segment files while the shared + * {@link SegmentManager} worker was still mid service pass for the engine's + * ring. A replacement engine could acquire the same slot the moment the + * lock was released, after which the stale worker's abandon/trim path could + * unlink a segment path the replacement was actively writing through — + * store-and-forward data loss after restart. + *

+ * The fix makes {@code close()} run a quiescence barrier + * ({@link SegmentManager#awaitRingQuiescence}) after {@code deregister} and + * refuse to release any worker-reachable resource (ring, watermark, segment + * files, slot lock) until the barrier confirms the worker cannot touch the + * slot again. On barrier timeout the engine deliberately leaks and a later + * {@code close()} retries the cleanup. + */ +public class CursorSendEngineSlotReacquisitionTest { + + private String tmpDir; + + @Before + public void setUp() { + tmpDir = Paths.get(System.getProperty("java.io.tmpdir"), + "qdb-engine-slot-reacq-" + System.nanoTime()).toString(); + Assert.assertEquals(0, Files.mkdir(tmpDir, Files.DIR_MODE_DEFAULT)); + } + + @After + public void tearDown() { + if (tmpDir == null) return; + rmDirRecursive(tmpDir); + Files.remove(tmpDir); + } + + /** + * The structural guarantee: while the manager worker is provably still + * inside a service pass for the engine's ring, {@code close()} must NOT + * hand the slot to anyone else. With the quiescence barrier reverted, + * close() releases the slot lock immediately and the mid-test + * {@code SlotLock.acquire} probe succeeds — failing the test. + */ + @Test(timeout = 30_000L) + public void testCloseRetainsSlotWhileWorkerIsMidServicePass() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/slot"; + // 60 s poll: the worker only acts when explicitly woken, so the + // single pass we park below is the only pass in flight. + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + AtomicReference hookErr = new AtomicReference<>(); + boolean managerClosed = false; + CursorSendEngine engine = null; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + if (!releaseWorker.await(20, TimeUnit.SECONDS)) { + hookErr.compareAndSet(null, + new AssertionError("timed out waiting for test to release worker")); + } + } catch (Throwable t) { + hookErr.compareAndSet(null, t); + } + }); + manager.start(); + + // Shared manager: ownsManager=false, so engine close() cannot + // fall back on manager.close()'s join — the per-ring barrier + // is the only protection, which is exactly what we pin here. + engine = new CursorSendEngine(slot, segSize, manager); + Assert.assertTrue("worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + // Barrier must time out fast: the worker is parked inside the + // service pass for this engine's ring. + manager.setWorkerJoinTimeoutMillis(50L); + engine.close(); + + // The slot must still be locked: a replacement engine (or raw + // SlotLock) acquiring it now would race the stale worker. + try { + SlotLock probe = SlotLock.acquire(slot); + probe.close(); + Assert.fail("engine.close() released the slot lock while the manager " + + "worker was still mid service pass for its ring — a " + + "replacement engine could acquire the slot and have its " + + "segment files unlinked by the stale worker"); + } catch (Exception expected) { + // good — slot retained. + } + + // Let the worker finish its pass (it abandons the spare: the + // ring was deregistered by the close attempt above). + releaseWorker.countDown(); + manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); + + // Retry close(): the barrier now succeeds and the full cleanup + // (ring, watermark, unlink, slot lock) must complete. + engine.close(); + engine = null; + + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after a completed close", probe); + } catch (Exception e) { + throw new AssertionError("retried close() did not release the slot lock", e); + } + + manager.close(); + managerClosed = true; + if (hookErr.get() != null) { + throw new AssertionError("install hook failed", hookErr.get()); + } + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (engine != null) { + try { + engine.close(); + } catch (Throwable ignored) { + } + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + + /** + * Plain-positive path: after a normal close (worker quiesces promptly), + * a second engine must be able to acquire and use the same slot. + */ + @Test(timeout = 30_000L) + public void testSameSlotReacquirableAfterNormalClose() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String slot = tmpDir + "/slot"; + CursorSendEngine first = new CursorSendEngine(slot, 4L * 1024 * 1024); + first.close(); + CursorSendEngine second = new CursorSendEngine(slot, 4L * 1024 * 1024); + try { + Assert.assertFalse("fully-drained close must leave no segments to recover", + second.wasRecoveredFromDisk()); + } finally { + second.close(); + } + }); + } + + private static void rmDirRecursive(String dir) { + if (!Files.exists(dir)) return; + long find = Files.findFirst(dir); + if (find <= 0) return; + try { + int rc = 1; + while (rc > 0) { + String name = Files.utf8ToString(Files.findName(find)); + if (name != null && !".".equals(name) && !"..".equals(name)) { + String child = dir + "/" + name; + if (!Files.remove(child)) { + rmDirRecursive(child); + Files.remove(child); + } + } + rc = Files.findNext(find); + } + } finally { + Files.findClose(find); + } + } +} diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java index 3d7c6a7c..4e62f8ca 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java @@ -204,6 +204,81 @@ public void testCloseDoesNotFreePathScratchWhenWorkerStillAlive() throws Excepti }); } + /** + * Pins the {@link SegmentManager#awaitRingQuiescence} contract that + * {@code CursorSendEngine.close()} depends on: + *

+ */ + @Test(timeout = 15_000L) + public void testAwaitRingQuiescenceBlocksWhileServicePassInFlight() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/quiesce-slot"; + Assert.assertEquals(0, Files.mkdir(slot, Files.DIR_MODE_DEFAULT)); + MmapSegment initial = MmapSegment.create(slot + "/sf-initial.sfa", 0L, segSize); + SegmentRing ring = new SegmentRing(initial, segSize); + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + AtomicReference hookErr = new AtomicReference<>(); + boolean managerClosed = false; + try { + manager.register(ring, slot); + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + if (!releaseWorker.await(10, TimeUnit.SECONDS)) { + hookErr.compareAndSet(null, + new AssertionError("timed out waiting for test to release worker")); + } + } catch (Throwable t) { + hookErr.compareAndSet(null, t); + } + }); + manager.start(); + Assert.assertTrue("worker did not reach install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + manager.deregister(ring); + manager.setWorkerJoinTimeoutMillis(50L); + Thread.currentThread().interrupt(); + Assert.assertFalse( + "awaitRingQuiescence returned true while the worker was parked " + + "inside the service pass for this ring", + manager.awaitRingQuiescence(ring)); + Assert.assertTrue("awaitRingQuiescence must preserve the caller's interrupt", + Thread.interrupted()); + + releaseWorker.countDown(); + manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); + Assert.assertTrue( + "awaitRingQuiescence must return true once the in-flight pass finished", + manager.awaitRingQuiescence(ring)); + + manager.close(); + managerClosed = true; + if (hookErr.get() != null) { + throw new AssertionError("install hook failed", hookErr.get()); + } + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (!managerClosed) { + Thread.interrupted(); + manager.close(); + } + ring.close(); + } + }); + } + @Test(timeout = 15_000L) public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception { TestUtils.assertMemoryLeak(() -> { From 358005c34f3bd2e82bb37ea65b4f7fa6b655c5b1 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 12:21:22 +0100 Subject: [PATCH 02/12] test(qwp): make the interrupted-close regression revert-proof and cover the mid-join interrupt testInterruptedCallerDoesNotAbandonReapableWorker relied on a negative wall-clock assertion (closeReturned must NOT count down within 300 ms) that never proved the closer reached the join path: a descheduled closer plus the old immediate-InterruptedException behavior could satisfy every assertion. The 'interrupt arrives during join' branch of the retry loop had no test. - SegmentManager gains a @TestOnly beforeJoinAttemptHook that runs on the closer thread after interrupt clearing, immediately before each join attempt - reworked test positively awaits the hook and asserts the interrupt is clear and the worker still live at the join point (fails on both revert shapes) - new testInterruptDuringJoinRetriesUntilWorkerReaped drives a mid-join interrupt and uses the second hook invocation as deterministic proof the loop retried instead of abandoning the reap Verified: both tests fail against a simulated revert (single join, no interrupt clearing, abandon on InterruptedException) and pass 3/3 runs with the fix. --- .../qwp/client/sf/cursor/SegmentManager.java | 18 +++ .../cursor/SegmentManagerCloseRaceTest.java | 142 +++++++++++++++++- 2 files changed, 158 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java index 6548fdef..19da3a67 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java @@ -86,6 +86,15 @@ public final class SegmentManager implements QuietCloseable { private final ObjList ringSnapshot = new ObjList<>(); private final ObjList rings = new ObjList<>(); private final long segmentSizeBytes; + // Test seam: runs on the closer thread inside close(), after the pending + // caller interrupt has been cleared and immediately before EACH join + // attempt on the worker thread. Null in production. + // SegmentManagerCloseRaceTest uses it to positively observe (a) that the + // closer reached the join path with a cleared interrupt while the worker + // was still live, and (b) that an interrupt delivered mid-join loops back + // into another join attempt instead of abandoning the reap — without any + // wall-clock coordination. + private volatile Runnable beforeJoinAttemptHook; // Test seam: runs on the worker thread just before the install path's // synchronized(lock) entry (the one that performs installHotSpare + the // totalBytes += segmentSize commit). Null in production; tests use it to @@ -201,6 +210,10 @@ public synchronized void close() { if (remainingMillis <= 0) { break; } + Runnable joinHook = beforeJoinAttemptHook; + if (joinHook != null) { + joinHook.run(); + } try { t.join(remainingMillis); } catch (InterruptedException ignored) { @@ -405,6 +418,11 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) { wakeWorker(); } + @TestOnly + public void setBeforeJoinAttemptHook(Runnable hook) { + this.beforeJoinAttemptHook = hook; + } + @TestOnly public void setBeforeInstallSyncHook(Runnable hook) { this.beforeInstallSyncHook = hook; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java index 4e62f8ca..7ed0ae23 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java @@ -279,6 +279,17 @@ public void testAwaitRingQuiescenceBlocksWhileServicePassInFlight() throws Excep }); } + /** + * A closer arriving with a pending interrupt must clear it, reach the + * join path, and reap the worker — not throw out of join() immediately + * and abandon a live worker that still owns segment files. + *

+ * Deterministic: the before-join-attempt hook is the positive signal + * that the closer reached the join with (a) the interrupt cleared and + * (b) the worker still live. No wall-clock "close didn't return within + * N ms" inversion — that formulation passes with the fix reverted + * whenever the closer is descheduled before the join. + */ @Test(timeout = 15_000L) public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception { TestUtils.assertMemoryLeak(() -> { @@ -291,7 +302,10 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception CountDownLatch workerBlocked = new CountDownLatch(1); CountDownLatch releaseWorker = new CountDownLatch(1); CountDownLatch closeReturned = new CountDownLatch(1); + CountDownLatch closerAtJoin = new CountDownLatch(1); AtomicBoolean fired = new AtomicBoolean(); + AtomicBoolean interruptClearAtJoin = new AtomicBoolean(); + AtomicBoolean workerAliveAtJoin = new AtomicBoolean(); AtomicBoolean interruptPreserved = new AtomicBoolean(); AtomicReference hookErr = new AtomicReference<>(); AtomicReference closeErr = new AtomicReference<>(); @@ -310,6 +324,17 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception hookErr.compareAndSet(null, t); } }); + manager.setBeforeJoinAttemptHook(() -> { + if (closerAtJoin.getCount() == 0) return; + interruptClearAtJoin.set(!Thread.currentThread().isInterrupted()); + try { + Thread w = readWorkerThread(manager); + workerAliveAtJoin.set(w != null && w.isAlive()); + } catch (Throwable t) { + hookErr.compareAndSet(null, t); + } + closerAtJoin.countDown(); + }); manager.start(); Assert.assertTrue("worker did not reach install hook", workerBlocked.await(5, TimeUnit.SECONDS)); @@ -327,8 +352,17 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception }, "interrupted-closer"); closer.start(); - Assert.assertFalse("interrupted close() abandoned a live worker instead of waiting", - closeReturned.await(300, TimeUnit.MILLISECONDS)); + // Positive proof the closer reached the join path: the hook + // fired. With the fix reverted (immediate InterruptedException + // abandon, or no interrupt-clearing), either the hook records + // a still-set interrupt or the final reap assertions fail. + Assert.assertTrue("closer never reached the join path", + closerAtJoin.await(5, TimeUnit.SECONDS)); + Assert.assertTrue("the pending caller interrupt must be cleared before the join " + + "(otherwise join() throws immediately and the worker is abandoned)", + interruptClearAtJoin.get()); + Assert.assertTrue("worker must still be live when the closer starts joining", + workerAliveAtJoin.get()); releaseWorker.countDown(); Assert.assertTrue("close() never returned after the worker was released", @@ -348,6 +382,110 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception } } finally { manager.setBeforeInstallSyncHook(null); + manager.setBeforeJoinAttemptHook(null); + releaseWorker.countDown(); + if (!managerClosed) { + Thread.interrupted(); + manager.close(); + } + ring.close(); + } + }); + } + + /** + * The "interrupt arrives DURING the join" branch of the close() loop: an + * InterruptedException thrown out of {@code t.join()} must mark the + * interrupt for restoration and loop back into another join attempt — + * not abandon the reap. + *

+ * Deterministic: the second before-join-attempt hook invocation is the + * positive proof that the loop retried after the mid-join interrupt. + * With the retry loop reverted (single join, abandon on interrupt) the + * second invocation never happens and the await fails. + */ + @Test(timeout = 15_000L) + public void testInterruptDuringJoinRetriesUntilWorkerReaped() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/mid-join-interrupt-slot"; + Assert.assertEquals(0, Files.mkdir(slot, Files.DIR_MODE_DEFAULT)); + MmapSegment initial = MmapSegment.create(slot + "/sf-initial.sfa", 0L, segSize); + SegmentRing ring = new SegmentRing(initial, segSize); + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + CountDownLatch closeReturned = new CountDownLatch(1); + CountDownLatch firstJoinAttempt = new CountDownLatch(1); + CountDownLatch secondJoinAttempt = new CountDownLatch(2); + AtomicBoolean fired = new AtomicBoolean(); + AtomicBoolean interruptPreserved = new AtomicBoolean(); + AtomicReference hookErr = new AtomicReference<>(); + AtomicReference closeErr = new AtomicReference<>(); + boolean managerClosed = false; + try { + manager.register(ring, slot); + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + if (!releaseWorker.await(10, TimeUnit.SECONDS)) { + hookErr.compareAndSet(null, + new AssertionError("timed out waiting for test to release worker")); + } + } catch (Throwable t) { + hookErr.compareAndSet(null, t); + } + }); + manager.setBeforeJoinAttemptHook(() -> { + firstJoinAttempt.countDown(); + secondJoinAttempt.countDown(); + }); + manager.start(); + Assert.assertTrue("worker did not reach install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + Thread closer = new Thread(() -> { + try { + manager.close(); + } catch (Throwable t) { + closeErr.compareAndSet(null, t); + } finally { + interruptPreserved.set(Thread.currentThread().isInterrupted()); + closeReturned.countDown(); + } + }, "mid-join-interrupted-closer"); + closer.start(); + + Assert.assertTrue("closer never reached the first join attempt", + firstJoinAttempt.await(5, TimeUnit.SECONDS)); + // The worker is still parked in the install hook, so the + // join cannot return normally: this interrupt must surface + // as an InterruptedException inside (or on entry to) join. + closer.interrupt(); + Assert.assertTrue("close() abandoned the reap after a mid-join interrupt " + + "instead of looping back into another join attempt", + secondJoinAttempt.await(5, TimeUnit.SECONDS)); + + releaseWorker.countDown(); + Assert.assertTrue("close() never returned after the worker was released", + closeReturned.await(10, TimeUnit.SECONDS)); + closer.join(TimeUnit.SECONDS.toMillis(5)); + managerClosed = readWorkerThread(manager) == null; + + if (closeErr.get() != null) { + throw new AssertionError("close() threw", closeErr.get()); + } + Assert.assertNull("close() must reap the worker despite the mid-join interrupt", + readWorkerThread(manager)); + Assert.assertTrue("close() must restore the interrupt that arrived during the join", + interruptPreserved.get()); + if (hookErr.get() != null) { + throw new AssertionError("install hook failed", hookErr.get()); + } + } finally { + manager.setBeforeInstallSyncHook(null); + manager.setBeforeJoinAttemptHook(null); releaseWorker.countDown(); if (!managerClosed) { Thread.interrupted(); From 80e69c971bd164bb38a53c21ab80f2684fd9cfff Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 18:52:28 +0100 Subject: [PATCH 03/12] Harden QWP recovery and shutdown Cursor engine closure now retains slot ownership until manager quiescence and transfers stalled cleanup to a dedicated daemon. Recovery validates persisted segments through bounded positional reads before mmap, preserves facade ownership, and retains corrupt data for diagnosis. Deterministic tests cover lifecycle races, compatibility, recovery faults, and slot reuse. --- .../qwp/client/QwpWebSocketSender.java | 72 ++- .../client/sf/cursor/BackgroundDrainer.java | 7 +- .../client/sf/cursor/CursorSendEngine.java | 57 +- .../sf/cursor/CursorWebSocketSendLoop.java | 8 +- .../qwp/client/sf/cursor/MmapSegment.java | 489 +++++++++--------- .../qwp/client/sf/cursor/SegmentManager.java | 68 ++- .../qwp/client/sf/cursor/SegmentRing.java | 11 +- .../io/questdb/client/std/FilesFacade.java | 14 +- ...CursorSendEngineSlotReacquisitionTest.java | 216 +++++++- .../MmapSegmentRecoveryCrcBenchmark.java | 143 +++++ .../cursor/MmapSegmentRecoveryFaultTest.java | 308 ++++++----- .../qwp/client/sf/cursor/MmapSegmentTest.java | 53 +- .../client/sf/cursor/PrReviewRedTests.java | 18 +- .../cursor/SegmentManagerCloseRaceTest.java | 13 +- ...entManagerWatermarkDeregisterRaceTest.java | 73 +++ .../cursor/SegmentRingRecoveryUnlinkTest.java | 42 ++ .../client/test/impl/SenderPoolSfTest.java | 11 +- 17 files changed, 1160 insertions(+), 443 deletions(-) create mode 100644 core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryCrcBenchmark.java diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java index d1744065..79b16f6e 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java @@ -219,6 +219,9 @@ public class QwpWebSocketSender implements Sender { // into the engine's mmap'd ring; the cursorSendLoop is the I/O thread // that walks the ring and sends frames. private CursorSendEngine cursorEngine; + // True after a failed I/O-thread stop handed engine ownership to that + // thread's exit path. Repeated sender close calls must not race that owner. + private boolean cursorEngineCloseDelegated; private CursorWebSocketSendLoop cursorSendLoop; private boolean deferCommit; // User-supplied observer for background orphan-slot drainer events. @@ -1043,8 +1046,20 @@ public QwpWebSocketSender charColumn(CharSequence columnName, char value) { */ @Override public void close() { - if (!closed) { - closed = true; + if (closed) { + // A prior close may have timed out waiting for manager quiescence. + // Retry only when this thread still owns cleanup; delegated engine + // closes belong exclusively to the I/O thread's exit path. + if (!cursorEngineCloseDelegated) { + try { + closeOwnedCursorEngine(); + } catch (Throwable t) { + LOG.error("Error retrying owned CursorSendEngine close: {}", String.valueOf(t)); + } + } + return; + } + closed = true; boolean ioThreadStopped = true; // Captures the first error from the flush/drain path AND any // secondary errors from cleanup steps (added via addSuppressed). @@ -1187,17 +1202,17 @@ public void close() { // close actually runs, so the pool must not reuse the slot // meanwhile. A false return means the thread exited between // the failed close() and now — then closing here is safe. - if (ownsCursorEngine && cursorEngine != null && cursorSendLoop != null - && !cursorSendLoop.delegateEngineClose()) { - try { - cursorEngine.close(); - } catch (Throwable t) { - LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t)); - terminalError = captureCloseError(terminalError, t); + if (ownsCursorEngine && cursorEngine != null && cursorSendLoop != null) { + if (cursorSendLoop.delegateEngineClose()) { + cursorEngineCloseDelegated = true; + } else { + try { + closeOwnedCursorEngine(); + } catch (Throwable t) { + LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t)); + terminalError = captureCloseError(terminalError, t); + } } - cursorEngine = null; - ownsCursorEngine = false; - slotLockReleased = true; } rethrowTerminal(terminalError); return; @@ -1232,18 +1247,15 @@ public void close() { if (ownsCursorEngine && cursorEngine != null) { try { - cursorEngine.close(); + closeOwnedCursorEngine(); } catch (Throwable t) { LOG.error("Error closing owned CursorSendEngine: {}", String.valueOf(t)); terminalError = captureCloseError(terminalError, t); } - cursorEngine = null; - ownsCursorEngine = false; + } else { + // This sender never owned an engine holding the pool's slot. + slotLockReleased = true; } - // Past the ioThreadStopped guard => cursorEngine.close() ran and - // released the SF flock in its finally (or this sender owned no - // engine holding one). Signal the pool it may reuse the slot. - slotLockReleased = true; // Shutdown order: dispatcher last, after the I/O loop has stopped // producing into it. close() drains pending entries with a short @@ -1283,7 +1295,6 @@ public void close() { terminalError = null; } rethrowTerminal(terminalError); - } } /** @@ -1293,7 +1304,8 @@ public void close() { * slot index reserved instead of reusing the still-locked slot dir. */ public boolean isSlotLockReleased() { - return slotLockReleased; + CursorSendEngine engine = cursorEngine; + return slotLockReleased || (engine != null && engine.isCloseCompleted()); } @Override @@ -2527,6 +2539,24 @@ public boolean wasEverConnected() { return l != null && l.hasEverConnected(); } + private void closeOwnedCursorEngine() { + if (!ownsCursorEngine || cursorEngine == null || cursorEngineCloseDelegated) { + return; + } + cursorEngine.close(); + if (cursorEngine.isCloseCompleted()) { + cursorEngine = null; + ownsCursorEngine = false; + slotLockReleased = true; + } else { + // Preserve the sole owner reference for a later retry. The pool + // observes false and retires this index rather than reusing a + // directory whose flock is still held. + slotLockReleased = false; + cursorEngine.closeEventually(); + } + } + private static Throwable captureCloseError(Throwable terminalError, Throwable t) { if (terminalError == null) { return t; diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java index d79080d4..566da28b 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java @@ -704,10 +704,15 @@ public void run() { // between the failed close() and now: then it is safe (and // necessary) to close the engine here. if (ioThreadStopped || !loop.delegateEngineClose()) { + // Make one bounded attempt. If manager quiescence times + // out, transfer ownership so this pool task can terminate. try { - // engine.close() releases the slot lock too. engine.close(); } catch (Throwable ignored) { + // The deferred owner retries below when needed. + } + if (!engine.isCloseCompleted()) { + engine.closeEventually(); } } else { LOG.warn("drainer slot {}: engine close delegated to the I/O thread; " diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java index 379b5dbe..9ebac3a1 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java @@ -61,6 +61,12 @@ public final class CursorSendEngine implements QuietCloseable { * Default deadline for {@link #appendBlocking}: 30 seconds. */ public static final long DEFAULT_APPEND_DEADLINE_NANOS = 30_000_000_000L; + private static final java.util.concurrent.ExecutorService DEFERRED_CLOSE_EXECUTOR = + java.util.concurrent.Executors.newCachedThreadPool(runnable -> { + Thread thread = new Thread(runnable, "qdb-cursor-engine-close"); + thread.setDaemon(true); + return thread; + }); private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(CursorSendEngine.class); private final long appendDeadlineNanos; @@ -118,8 +124,11 @@ public final class CursorSendEngine implements QuietCloseable { // a close attempt could not confirm manager-worker quiescence and had to // leak the ring/watermark/slot lock — in that case a later close() call // retries the cleanup (the worker may have exited by then). Guarded by - // the synchronized close() method; never read elsewhere. + // the synchronized close() method and isCloseCompleted() accessor. private boolean closeCompleted; + // True once a dedicated daemon has accepted ownership of incomplete close + // retries. Guarded by synchronized closeEventually(). + private boolean deferredCloseScheduled; // Producer-thread-only: timestamp of the last "we're backpressured" log // line, used to throttle. Plain long is fine. private long lastBackpressureLogNs; @@ -613,6 +622,25 @@ public synchronized void close() { } } + /** + * Transfers an incomplete close to a dedicated daemon owner. This lets + * I/O and drainer executors terminate even when a manager service pass is + * permanently stalled. The deferred owner retains this engine, and thus + * its flock and native mappings, until a retry completes. + */ + public synchronized void closeEventually() { + if (closeCompleted || deferredCloseScheduled) { + return; + } + deferredCloseScheduled = true; + try { + DEFERRED_CLOSE_EXECUTOR.execute(this::runDeferredClose); + } catch (Throwable t) { + deferredCloseScheduled = false; + throw t; + } + } + /** * Pass-through to {@link SegmentRing#findSegmentContaining(long)}. */ @@ -637,6 +665,16 @@ public long getTotalBackpressureStalls() { return backpressureStallCount.get(); } + /** + * True only after close released every engine-owned resource, including + * the SF slot flock. A false result means close deliberately retained the + * ring/watermark/lock because the manager worker did not quiesce; owners + * must preserve this engine and retry later. + */ + public synchronized boolean isCloseCompleted() { + return closeCompleted; + } + /** * Pass-through to {@link SegmentRing#nextSealedAfter(MmapSegment)}. */ @@ -718,6 +756,23 @@ public long recoveredOrphanTipFsn() { * Best-effort: logs and continues on failures, since we're already on * the close path. */ + private void runDeferredClose() { + while (!isCloseCompleted()) { + try { + close(); + } catch (Throwable ignored) { + // Retain ownership and retry after a bounded pause. + } + if (!isCloseCompleted()) { + LockSupport.parkNanos(10_000_000L); + // The internal daemon owns cleanup independently of caller + // cancellation. Clear interrupts so they cannot create a hot + // retry loop if an executor implementation interrupts it. + Thread.interrupted(); + } + } + } + private static void unlinkAllSegmentFiles(String dir) { if (!io.questdb.client.std.Files.exists(dir)) return; long find = io.questdb.client.std.Files.findFirst(dir); diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java index 13a69f77..185082f6 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java @@ -1679,10 +1679,16 @@ private void ioLoop() { // either this branch or the owner's fallback runs (or both — // engine.close() is idempotent). if (engineCloseDelegated) { + // Make one bounded attempt on this exit path. If manager + // quiescence times out, transfer ownership to the dedicated + // deferred closer so this I/O daemon can terminate. try { engine.close(); } catch (Throwable ignored) { - // best-effort + // The deferred owner retries below when needed. + } + if (!engine.isCloseCompleted()) { + engine.closeEventually(); } } } diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index 78e5db9f..1f0a3707 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -64,9 +64,10 @@ public final class MmapSegment implements QuietCloseable { public static final int FRAME_HEADER_SIZE = 8; // u32 crc + u32 payloadLen public static final int HEADER_SIZE = 24; public static final byte VERSION = 1; - private static final int[] CRC32C_TABLE = buildCrc32cTable(); private static final Logger LOG = LoggerFactory.getLogger(MmapSegment.class); + private static final int RECOVERY_BUFFER_SIZE = 1024 * 1024; + private final FilesFacade filesFacade; private final String path; private final long sizeBytes; // memoryBacked: true when the segment buffer lives in malloc'd native @@ -104,8 +105,9 @@ public final class MmapSegment implements QuietCloseable { private MmapSegment(String path, int fd, long mmapAddress, long sizeBytes, long baseSeq, long initialCursor, long frameCount, - boolean memoryBacked, long tornTailBytes) { + boolean memoryBacked, long tornTailBytes, FilesFacade filesFacade) { this.path = path; + this.filesFacade = filesFacade; this.fd = fd; this.mmapAddress = mmapAddress; this.sizeBytes = sizeBytes; @@ -197,7 +199,8 @@ public static MmapSegment create(FilesFacade ff, long pathPtr, String displayPat Unsafe.getUnsafe().putShort(addr + 6, (short) 0); // reserved Unsafe.getUnsafe().putLong(addr + 8, baseSeq); Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros()); - return new MmapSegment(displayPath, fd, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L); + return new MmapSegment( + displayPath, fd, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L, ff); } catch (Throwable t) { if (addr != Files.FAILED_MMAP_ADDRESS) { Files.munmap(addr, sizeBytes, MemoryTag.MMAP_DEFAULT); @@ -234,7 +237,8 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) { Unsafe.getUnsafe().putShort(addr + 6, (short) 0); Unsafe.getUnsafe().putLong(addr + 8, baseSeq); Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros()); - return new MmapSegment(null, -1, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L); + return new MmapSegment( + null, -1, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L, null); } catch (Throwable t) { Unsafe.free(addr, sizeBytes, MemoryTag.NATIVE_DEFAULT); throw t; @@ -262,92 +266,89 @@ public static MmapSegment openExisting(String path) { } /** - * Facade-aware variant of {@link #openExisting(String)} that takes the file - * length (and open/close) through a {@link FilesFacade} instead of straight - * off {@link Files}. Production uses {@link FilesFacade#INSTANCE}; the seam - * exists so recovery's mmap-fault guard can be regression-tested on any - * filesystem. - *

- * The mapping is sized to {@code ff.length(path)} and every recovery read - * runs straight out of it, so a facade that reports a length larger - * than the real file makes the mapping extend past end-of-file. A read of a - * page beyond real EOF raises SIGBUS on every filesystem — the same - * fault an unbacked/sparse page raises on ZFS, but reproduced - * deterministically on ext4/xfs, where a within-EOF hole would instead - * zero-fill and never exercise the guard. See - * {@link FilesFacade#length(String)}. + * Facade-aware recovery variant. Recovery validates the file with + * positional reads before mapping it. HotSpot can deliver an mmap + * {@link InternalError} after the lexical catch around an Unsafe load on + * older JDKs; pread reports EOF and I/O errors synchronously instead. */ public static MmapSegment openExisting(FilesFacade ff, String path) { - long fileSize = ff.length(path); - if (fileSize < HEADER_SIZE) { - throw new MmapSegmentException("file shorter than header: " + path + " size=" + fileSize); - } int fd = ff.openRW(path); if (fd < 0) { throw new MmapSegmentException("openRW failed for " + path); } long addr = Files.FAILED_MMAP_ADDRESS; + long mapSize = 0L; + long scanBuffer = 0L; try { - addr = Files.mmap(fd, fileSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); - if (addr == Files.FAILED_MMAP_ADDRESS) { - throw new MmapSegmentException("mmap failed for " + path); + scanBuffer = Unsafe.malloc(RECOVERY_BUFFER_SIZE, MemoryTag.NATIVE_DEFAULT); + long fileSize = ff.length(fd); + if (fileSize < HEADER_SIZE) { + throw new MmapSegmentException("file shorter than header: " + path + " size=" + fileSize); } - int magic = Unsafe.getUnsafe().getInt(addr); - if (magic != FILE_MAGIC) { + RecoveryResult recovered = scanFile(ff, fd, path, fileSize, scanBuffer); + + // A concurrent shrink between scan and mmap must not expose a page + // beyond EOF. Slot locking excludes the normal case, but the + // second fd-size read also makes fault-injection and hostile file + // changes deterministic. + long finalFileSize = ff.length(fd); + if (finalFileSize < recovered.lastGoodOffset) { throw new MmapSegmentException( - "bad magic in " + path + ": 0x" + Integer.toHexString(magic)); + "file shrank below recovered data: " + path + + " size=" + finalFileSize + + " recovered=" + recovered.lastGoodOffset); } - byte version = Unsafe.getUnsafe().getByte(addr + 4); - if (version != VERSION) { - throw new MmapSegmentException("unsupported version in " + path + ": " + version); - } - long baseSeq = Unsafe.getUnsafe().getLong(addr + 8); - // FSNs are non-negative by construction (see SegmentRing). - // A negative baseSeq on disk means bit-rot or a malicious file — - // refuse the segment so SegmentRing.openExisting's narrow catch - // skips it like any other unreadable .sfa rather than feeding - // the bad value into Long.compareUnsigned-based contiguity - // checks (which would place the segment last in baseSeq order - // and trip the FSN-gap throw, taking the whole recovery down). - if (baseSeq < 0L) { + if (recovered.hasUnexpectedEof && finalFileSize >= fileSize) { throw new MmapSegmentException( - "bad baseSeq in " + path + ": " + baseSeq); + "short read before stable file EOF during recovery: " + path + + " size=" + fileSize); + } + mapSize = Math.min(fileSize, finalFileSize); + if (mapSize < HEADER_SIZE) { + throw new MmapSegmentException("file shorter than header after recovery scan: " + + path + " size=" + mapSize); + } + addr = Files.mmap(fd, mapSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); + if (addr == Files.FAILED_MMAP_ADDRESS) { + throw new MmapSegmentException("mmap failed for " + path); } - long lastGood = scanFrames(addr, fileSize); - long count = countFrames(addr, lastGood); - long tornTail = detectTornTail(addr, lastGood, fileSize); - if (tornTail > 0) { + MmapSegment segment = new MmapSegment( + path, + fd, + addr, + mapSize, + recovered.baseSeq, + recovered.lastGoodOffset, + recovered.frameCount, + false, + Math.min(recovered.tornTailBytes, mapSize - recovered.lastGoodOffset), + ff + ); + if (segment.tornTailBytes() > 0) { LOG.warn("SF segment {}: torn tail of {} bytes at offset {} " + "(file size {}, frames recovered {}). " + "Recovery will overwrite this region on next append; " + "frames past the tear (if any) are discarded. " + "Investigate disk health or unexpected writer crash.", - path, tornTail, lastGood, fileSize, count); + path, + segment.tornTailBytes(), + segment.publishedOffset(), + mapSize, + segment.frameCount()); } - return new MmapSegment(path, fd, addr, fileSize, baseSeq, lastGood, count, false, tornTail); + // Ownership of fd and addr transfers to the returned segment. + addr = Files.FAILED_MMAP_ADDRESS; + return segment; } catch (Throwable t) { if (addr != Files.FAILED_MMAP_ADDRESS) { - Files.munmap(addr, fileSize, MemoryTag.MMAP_DEFAULT); + Files.munmap(addr, mapSize, MemoryTag.MMAP_DEFAULT); } ff.close(fd); - // The header reads above (magic/version/baseSeq) run before - // scanFrames and are otherwise unguarded. An unbacked page 0 -- - // an unflushed header on a truncate-based-allocate filesystem - // after a crash (create() does not msync), or a file truncated - // under the mapping -- faults at the Unsafe intrinsic site as a - // catchable InternalError. Convert it to a MmapSegmentException so - // SegmentRing's per-file catch skips just this .sfa, instead of - // letting the raw InternalError escape to SegmentRing's outer catch - // and abort recovery of every sibling segment. scanFrames and - // detectTornTail already handle their own in-mapping faults; this - // covers the header block and any future reader placed ahead of the - // scan. - if (isMmapAccessFault(t)) { - throw new MmapSegmentException( - "unreadable mapped header page in " + path - + " (unbacked/sparse page 0): " + t.getMessage(), t); - } throw t; + } finally { + if (scanBuffer != 0L) { + Unsafe.free(scanBuffer, RECOVERY_BUFFER_SIZE, MemoryTag.NATIVE_DEFAULT); + } } } @@ -380,7 +381,11 @@ public void close() { mmapAddress = 0; } if (fd >= 0) { - Files.close(fd); + if (filesFacade != null) { + filesFacade.close(fd); + } else { + Files.close(fd); + } fd = -1; } } @@ -539,203 +544,211 @@ public long frameCount() { * fresh segments, memory-backed segments, and cleanly partially-filled * recovered segments. Operators / tests can read this to tell silent * truncation (corruption) from a normal partial fill (no incident). - *

- * One case this does NOT count: when the scan stops because the bail-out - * region is itself an unbacked mapped page (an in-mapping fault, not a - * backed torn header), that region cannot be probed, so this returns - * {@code 0} even though frames may have been discarded. That outcome is - * surfaced by the {@code WARN} in {@link #scanFrames} instead -- see it for - * the benign-tail-vs-media-error caveat. + * A positional read that reaches EOF before the initially reported file + * size yields {@code 0}: no bytes exist at the bail-out point to identify + * an attempted write. Hard read errors reject the segment instead of + * guessing whether its tail is clean. */ public long tornTailBytes() { return tornTailBytes; } - /** - * True when {@code t} is the JVM's recoverable signal for a fault while - * accessing a memory-mapped region -- a SIGBUS/SIGSEGV that HotSpot - * translates into an {@code InternalError} at an {@code Unsafe} intrinsic - * site instead of aborting the process. It surfaces when a mapped page is - * not backed by real file blocks: a sparse {@code .sfa} tail on a - * filesystem whose pre-allocation leaves holes (e.g. ZFS, where a - * truncate-based {@code allocate} does not materialize blocks), or a file - * truncated under the mapping. Recovery treats this as an I/O boundary -- - * the same way MappedByteBuffer readers do -- not a fatal VM error. - *

- * The message is matched on the fragment {@code "unsafe memory access - * operation"}, which is common to both HotSpot wordings and NOT - * version-stable as a whole: pre-21 JDKs (including the shipping/CI JDK 8) - * emit {@code "a fault occurred in a recent unsafe memory access operation - * in compiled Java code"}, while JDK 21+ shortened it to {@code "a fault - * occurred in an unsafe memory access operation"}. Matching the shared - * fragment keeps the guard effective on JDK 8/11/17 as well as 21+, while - * still being specific enough that a genuine VirtualMachineError (real OOM, - * StackOverflow) is never swallowed. - */ - private static boolean isMmapAccessFault(Throwable t) { - if (!(t instanceof InternalError)) { - return false; + private static RecoveryResult scanFile( + FilesFacade ff, + int fd, + String path, + long fileSize, + long scanBuffer + ) { + RecoveryReader reader = new RecoveryReader(ff, fd, path, fileSize, scanBuffer); + long headerAddress = reader.addressAt(0L, HEADER_SIZE); + if (headerAddress == 0L) { + throw new MmapSegmentException("short read of segment header: " + path); + } + int magic = Unsafe.getUnsafe().getInt(headerAddress); + if (magic != FILE_MAGIC) { + throw new MmapSegmentException( + "bad magic in " + path + ": 0x" + Integer.toHexString(magic)); + } + byte version = Unsafe.getUnsafe().getByte(headerAddress + 4); + if (version != VERSION) { + throw new MmapSegmentException("unsupported version in " + path + ": " + version); + } + long baseSeq = Unsafe.getUnsafe().getLong(headerAddress + 8); + // FSNs are non-negative by construction (see SegmentRing). Reject a + // corrupt value before unsigned contiguity checks can poison recovery. + if (baseSeq < 0L) { + throw new MmapSegmentException("bad baseSeq in " + path + ": " + baseSeq); } - String msg = t.getMessage(); - return msg != null && msg.contains("unsafe memory access operation"); - } - /** - * Forward scan that returns the offset just past the last frame whose - * CRC verifies. A torn-tail frame (declared length runs past EOF, or - * CRC mismatch) leaves both cursors at the start of that frame; the - * next {@link #tryAppend} will overwrite it. The scan only reads from - * the mapping — no syscalls. - */ - private static long scanFrames(long addr, long fileSize) { + long count = 0L; long pos = HEADER_SIZE; - try { - while (pos + FRAME_HEADER_SIZE <= fileSize) { - int crcRead = Unsafe.getUnsafe().getInt(addr + pos); - int payloadLen = Unsafe.getUnsafe().getInt(addr + pos + 4); - // Defensive: a corrupt length field could be enormous or negative, - // both of which would otherwise overrun the mapping. - if (payloadLen < 0 || pos + FRAME_HEADER_SIZE + payloadLen > fileSize) { - return pos; - } - // CRC over the contiguous (payloadLen, payload) pair, folded - // via Unsafe reads rather than the native Crc32c.update. - // Recovery maps to the file's stat length, but a page inside - // that range can be unbacked: a sparse pre-allocation tail (a - // truncate-based allocate that never materialized blocks, as on - // ZFS), or -- via a torn write, since tryAppend writes the - // length field before copying the payload -- a real positive - // payloadLen whose payload spans into an unwritten hole. A raw - // read of an unbacked page raises SIGBUS; HotSpot translates - // that into a catchable InternalError ONLY at an Unsafe - // intrinsic site, NEVER inside JNI native code, so a native - // Crc32c.update over such a page aborts the whole JVM (and, - // empirically on pre-21 JDKs, a preceding Unsafe pre-touch does - // not reliably fault first once an earlier native CRC ran in - // the same scan). Folding over Unsafe keeps every fault - // catchable -- handled below as the boundary of recoverable - // data; a page that instead reads back as zeroes just fails the - // CRC check and ends the scan. Recovery is cold, so the slower - // table CRC here is immaterial. - int crcCalc = crc32cRecovery(addr + pos + 4, 4L + payloadLen); - if (crcCalc != crcRead) { - return pos; + while (pos + FRAME_HEADER_SIZE <= fileSize) { + long frameHeader = reader.addressAt(pos, FRAME_HEADER_SIZE); + if (frameHeader == 0L) { + break; + } + int crcRead = Unsafe.getUnsafe().getInt(frameHeader); + long payloadLen = Unsafe.getUnsafe().getInt(frameHeader + 4) & 0xFFFF_FFFFL; + if (payloadLen > fileSize - pos - FRAME_HEADER_SIZE) { + break; + } + + int crc = Crc32c.update(Crc32c.INIT, frameHeader + 4, 4L); + long payloadOffset = pos + FRAME_HEADER_SIZE; + long remaining = payloadLen; + boolean hasUnexpectedEof = false; + while (remaining > 0L) { + int available = reader.availableAt(payloadOffset); + if (available == 0) { + hasUnexpectedEof = true; + break; } - pos += FRAME_HEADER_SIZE + payloadLen; + long chunk = Math.min(remaining, available); + crc = Crc32c.update(crc, reader.addressAt(payloadOffset, 1), chunk); + payloadOffset += chunk; + remaining -= chunk; } - } catch (InternalError e) { - // The read at `pos` hit a mapped page that is not backed by real - // file blocks: the JVM translates the underlying SIGBUS into a - // recoverable InternalError instead of aborting the process. This - // happens when a prior session left a sparse segment tail (a - // truncate-based pre-allocation that does not materialize blocks, - // as on ZFS) or the file was truncated under the mapping. Every - // frame below `pos` already verified; treat the unreadable region - // exactly like unwritten space or a torn tail -- the boundary of - // recoverable data -- rather than letting the error abort recovery - // of the whole slot. Anything that is not the documented mmap - // access fault is a genuine VM error, so rethrow it. - if (!isMmapAccessFault(e)) { - throw e; + if (hasUnexpectedEof || crc != crcRead) { + break; } - LOG.warn("SF segment recovery: unreadable mapped page at offset {} (file size {}); " - + "treating it as the end of recoverable data -- any frames beyond this " - + "offset are discarded. The usual cause is a benign sparse pre-allocation " - + "tail (e.g. a truncate-based allocate on ZFS) left by a prior session, " - + "but a mid-file media error (bad sector) is indistinguishable here; " - + "check disk health if this segment was expected to be fully written or " - + "if this recurs.", - pos, fileSize); + pos += FRAME_HEADER_SIZE + payloadLen; + count++; } - return pos; + // Empty hot spares are zero-filled all the way to EOF. Inspect the + // entire suffix, not just the failed frame header: a valid zero-length + // frame whose CRC corrupts to zero has an all-zero 8-byte header, while + // later valid frames still contain data that must be quarantined. + long tornTailBytes = reader.hasNonZeroAt(pos, fileSize - pos) + ? fileSize - pos + : 0L; + return new RecoveryResult(baseSeq, count, pos, tornTailBytes, reader.hasUnexpectedEof()); } - /** - * CRC-32C (Castagnoli) of {@code [addr, addr + len)} read through - * {@link Unsafe}, seeded like {@code Crc32c.update(Crc32c.INIT, addr, len)} - * and bit-identical to it (verified) -- but every byte load is an Unsafe - * intrinsic, so a fault on an unbacked mapped page is a catchable - * {@link InternalError} instead of the uncatchable JNI SIGBUS the native - * {@link Crc32c} would raise. Byte-at-a-time via a precomputed table - * ({@code ~0.5 GiB/s}); used only on the cold recovery scan, never on the - * append hot path (which stays on the native, hardware-friendly path). - */ - private static int crc32cRecovery(long addr, long len) { - int crc = ~Crc32c.INIT; - for (long i = 0; i < len; i++) { - crc = (crc >>> 8) ^ CRC32C_TABLE[(crc ^ Unsafe.getUnsafe().getByte(addr + i)) & 0xFF]; + private static final class RecoveryReader { + private final long address; + private final int fd; + private final long fileSize; + private final FilesFacade filesFacade; + private final String path; + private boolean hasUnexpectedEof; + private int windowLength; + private long windowOffset = -1L; + + private RecoveryReader( + FilesFacade filesFacade, + int fd, + String path, + long fileSize, + long address + ) { + this.filesFacade = filesFacade; + this.fd = fd; + this.path = path; + this.fileSize = fileSize; + this.address = address; } - return ~crc; - } - /** - * Standard reflected CRC-32C byte table (polynomial {@code 0x82F63B78}), - * matching {@code crc32c_table[0]} in the native {@code crc32c.c}. Computed - * at class init to avoid 256 hand-transcribed literals; drives - * {@link #crc32cRecovery}. - */ - private static int[] buildCrc32cTable() { - int[] table = new int[256]; - for (int n = 0; n < 256; n++) { - int c = n; - for (int k = 0; k < 8; k++) { - c = (c & 1) != 0 ? (0x82F63B78 ^ (c >>> 1)) : (c >>> 1); + private long addressAt(long offset, int minBytes) { + if (minBytes < 1 || minBytes > RECOVERY_BUFFER_SIZE || offset < 0L || offset >= fileSize) { + return 0L; } - table[n] = c; + if (windowOffset <= offset + && offset - windowOffset <= windowLength + && minBytes <= windowLength - (offset - windowOffset)) { + return address + offset - windowOffset; + } + refill(offset, minBytes); + return windowLength >= minBytes ? address : 0L; } - return table; - } - /** - * Distinguishes "torn tail" (writer attempted a write past the last valid - * frame and failed — partial write, mid-stream corruption, bit rot) from - * clean unwritten space (manager-allocated segment with zero-filled tail). - * Returns the byte count from {@code lastGood} to {@code fileSize} when - * the bytes at the bail-out frame header are non-zero, else {@code 0}. - *

- * Heuristic but robust for the common cases: {@link #create} truncates the - * file to size, leaving the tail zero-filled; the writer only writes - * non-zero bytes via {@link #tryAppend}, which writes the CRC and length - * fields together. So a non-zero byte at the failed-frame position - * implies an attempted write — exactly the case operators want flagged. - */ - private static long detectTornTail(long addr, long lastGood, long fileSize) { - if (lastGood >= fileSize) { - return 0L; + private int availableAt(long offset) { + long currentAddress = addressAt(offset, 1); + if (currentAddress == 0L) { + return 0; + } + return (int) (windowLength - (offset - windowOffset)); } - long probe = Math.min(FRAME_HEADER_SIZE, fileSize - lastGood); - try { - for (long i = 0; i < probe; i++) { - if (Unsafe.getUnsafe().getByte(addr + lastGood + i) != 0) { - return fileSize - lastGood; + + private boolean hasNonZeroAt(long offset, long length) { + long inspected = 0L; + while (inspected < length) { + int available = availableAt(offset + inspected); + if (available == 0) { + return false; + } + int chunk = (int) Math.min(length - inspected, available); + long currentAddress = addressAt(offset + inspected, 1); + int i = 0; + while (i < chunk && ((currentAddress + i) & 7L) != 0L) { + if (Unsafe.getUnsafe().getByte(currentAddress + i++) != 0) { + return true; + } + } + while (i + 8 <= chunk) { + if (Unsafe.getUnsafe().getLong(currentAddress + i) != 0L) { + return true; + } + i += 8; } + while (i < chunk) { + if (Unsafe.getUnsafe().getByte(currentAddress + i++) != 0) { + return true; + } + } + inspected += chunk; } - } catch (InternalError e) { - // The bail-out region is an unbacked (sparse) mapped page -- see - // scanFrames for the mechanism. An unbacked hole was never written, - // so it is clean unwritten space, not a torn write. Rethrow any - // error that is not the recoverable mmap access fault. - if (!isMmapAccessFault(e)) { - throw e; + return false; + } + + private boolean hasUnexpectedEof() { + return hasUnexpectedEof; + } + + private void refill(long offset, int minBytes) { + long maxLength = Math.min(RECOVERY_BUFFER_SIZE, fileSize - offset); + long total = 0L; + while (total < minBytes && total < maxLength) { + long read = filesFacade.read(fd, address + total, maxLength - total, offset + total); + if (read < 0L) { + throw new MmapSegmentException( + "read failed during segment recovery: " + path + " offset=" + (offset + total)); + } + if (read == 0L) { + hasUnexpectedEof = offset + total < fileSize; + break; + } + if (read > maxLength - total) { + throw new MmapSegmentException( + "invalid read length during segment recovery: " + path + " read=" + read); + } + total += read; } - return 0L; + windowOffset = offset; + windowLength = (int) total; } - return 0L; } - /** - * Counts frames in {@code [HEADER_SIZE, lastGood)}. Walks the framing in - * lockstep with {@link #scanFrames} (which already validated CRCs); so - * this is just length-driven traversal, no CRC re-check. - */ - private static long countFrames(long addr, long lastGood) { - long pos = HEADER_SIZE; - long count = 0; - while (pos < lastGood) { - int payloadLen = Unsafe.getUnsafe().getInt(addr + pos + 4); - pos += FRAME_HEADER_SIZE + payloadLen; - count++; + private static final class RecoveryResult { + private final long baseSeq; + private final long frameCount; + private final boolean hasUnexpectedEof; + private final long lastGoodOffset; + private final long tornTailBytes; + + private RecoveryResult( + long baseSeq, + long frameCount, + long lastGoodOffset, + long tornTailBytes, + boolean hasUnexpectedEof + ) { + this.baseSeq = baseSeq; + this.frameCount = frameCount; + this.lastGoodOffset = lastGoodOffset; + this.tornTailBytes = tornTailBytes; + this.hasUnexpectedEof = hasUnexpectedEof; } - return count; } + } diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java index 19da3a67..7a63096c 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java @@ -86,6 +86,10 @@ public final class SegmentManager implements QuietCloseable { private final ObjList ringSnapshot = new ObjList<>(); private final ObjList rings = new ObjList<>(); private final long segmentSizeBytes; + // Test seam: runs after each snapshotted entry's service attempt, including + // a stale entry skipped before claim. Null in production; tests use it to + // observe completion of the exact snapshot entry without timing. + private volatile Runnable afterServiceHook; // Test seam: runs on the closer thread inside close(), after the pending // caller interrupt has been cleared and immediately before EACH join // attempt on the worker thread. Null in production. @@ -95,6 +99,10 @@ public final class SegmentManager implements QuietCloseable { // into another join attempt instead of abandoning the reap — without any // wall-clock coordination. private volatile Runnable beforeJoinAttemptHook; + // Test seam: runs after workerLoop selects a snapshotted entry but before + // serviceRing atomically checks registered and claims inService. Null in + // production; tests pause precisely in the stale-snapshot-before-claim gap. + private volatile Runnable beforeServiceClaimHook; // Test seam: runs on the worker thread just before the install path's // synchronized(lock) entry (the one that performs installHotSpare + the // totalBytes += segmentSize commit). Null in production; tests use it to @@ -115,6 +123,10 @@ public final class SegmentManager implements QuietCloseable { // block until an in-flight pass for a just-deregistered ring finishes. private RingEntry inService; private long lastDiskFullLogNs; + // Number of callers currently waiting for inService to clear. Guarded by + // lock; serviceRing uses it to avoid a monitor-wide notifyAll on every + // polling pass when no quiescence barrier exists. + private int quiescenceWaiterCount; private volatile boolean running; // pathScratch free-exactly-once coordination between a timed-out close() // and the worker's exit path. All three are guarded by {@link #lock}. @@ -289,17 +301,24 @@ public boolean awaitRingQuiescence(SegmentRing ring) { boolean interrupted = Thread.interrupted(); try { synchronized (lock) { - while (inService != null && inService.ring == ring) { - long remainingNanos = deadlineNanos - System.nanoTime(); - if (remainingNanos <= 0) { - return false; - } + if (inService != null && inService.ring == ring) { + quiescenceWaiterCount++; try { - // Round up so a sub-millisecond remainder still waits - // instead of spinning through wait(0) == wait-forever. - lock.wait(Math.max(1L, remainingNanos / 1_000_000L)); - } catch (InterruptedException ignored) { - interrupted = true; + while (inService != null && inService.ring == ring) { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) { + return false; + } + try { + // Round up so a sub-millisecond remainder still waits + // instead of spinning through wait(0) == wait-forever. + lock.wait(Math.max(1L, remainingNanos / 1_000_000L)); + } catch (InterruptedException ignored) { + interrupted = true; + } + } + } finally { + quiescenceWaiterCount--; } } } @@ -311,6 +330,13 @@ public boolean awaitRingQuiescence(SegmentRing ring) { } } + @TestOnly + public boolean hasWorkerLoopExited() { + synchronized (lock) { + return workerLoopExited; + } + } + /** * True when no manager worker thread can be running: either * {@link #start()} was never called, or a {@link #close()} confirmed @@ -418,6 +444,11 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) { wakeWorker(); } + @TestOnly + public void setAfterServiceHook(Runnable hook) { + this.afterServiceHook = hook; + } + @TestOnly public void setBeforeJoinAttemptHook(Runnable hook) { this.beforeJoinAttemptHook = hook; @@ -428,6 +459,11 @@ public void setBeforeInstallSyncHook(Runnable hook) { this.beforeInstallSyncHook = hook; } + @TestOnly + public void setBeforeServiceClaimHook(Runnable hook) { + this.beforeServiceClaimHook = hook; + } + @TestOnly public void setBeforeTrimSyncHook(Runnable hook) { this.beforeTrimSyncHook = hook; @@ -538,6 +574,10 @@ private String nextSparePath(String dir) { } private void serviceRing(RingEntry e) { + Runnable serviceClaimHook = beforeServiceClaimHook; + if (serviceClaimHook != null) { + serviceClaimHook.run(); + } // Claim the entry as in-service so deregister-side quiescence // barriers (awaitRingQuiescence) can wait for this pass to finish. // A stale snapshot entry deregistered before the pass starts is @@ -558,7 +598,9 @@ private void serviceRing(RingEntry e) { } finally { synchronized (lock) { inService = null; - lock.notifyAll(); + if (quiescenceWaiterCount > 0) { + lock.notifyAll(); + } } } } @@ -737,6 +779,10 @@ private void workerLoop() { for (int i = 0, n = ringSnapshot.size(); i < n; i++) { if (!running) break; serviceRing(ringSnapshot.getQuick(i)); + Runnable serviceHook = afterServiceHook; + if (serviceHook != null) { + serviceHook.run(); + } } // Drop strong refs so a deregistered ring becomes collectable // before the next tick (otherwise the snapshot pins it for up diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index c8516c4c..71363c4f 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -197,7 +197,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { // CAUTION: only unlink when the file is genuinely // empty past the header. If frame[0] failed CRC // (bit-rot, partial-page-write at crash, etc.) but - // valid frames followed, scanFrames returns + // valid frames followed, the recovery scan returns // lastGood=HEADER_SIZE and frameCount=0 -- yet // tornTailBytes is non-zero. Treating that as // "empty hot-spare" would silently destroy every @@ -208,7 +208,12 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { seg.close(); seg = null; if (torn > 0) { - Files.rename(path, path + ".corrupt"); + int renameResult = Files.rename(path, path + ".corrupt"); + if (renameResult != 0) { + LOG.warn("openExisting: could not quarantine corrupt segment {}; " + + "the original file remains in place [rc={}]", + path, renameResult); + } } else { Files.remove(path); } @@ -269,7 +274,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { // FSNs after recovery. A gap means a segment went missing (a // manual deletion) or a sealed segment under-recovered -- its tail // was cut short by a sparse/unbacked page or a mid-file media error - // (bad sector), the same class of fault scanFrames tolerates on the + // (bad sector), the same class of fault the recovery scan tolerates on the // active segment but which corrupts the range on a sealed one. for (int i = 1, n = opened.size(); i < n; i++) { MmapSegment prev = opened.get(i - 1); diff --git a/core/src/main/java/io/questdb/client/std/FilesFacade.java b/core/src/main/java/io/questdb/client/std/FilesFacade.java index 1b408cf4..e26039cb 100644 --- a/core/src/main/java/io/questdb/client/std/FilesFacade.java +++ b/core/src/main/java/io/questdb/client/std/FilesFacade.java @@ -90,18 +90,10 @@ public interface FilesFacade { /** * Stat length of the file at {@code path}, in bytes. Default delegates to * {@link Files#length(String)}. - * - *

Test injection point: {@code MmapSegment.openExisting} maps the file to - * this length and scans straight out of the mapping, so a wrapping facade - * that returns a value larger than the real file makes the mapping - * extend past end-of-file. A read of a page beyond real EOF raises SIGBUS on - * every filesystem (which HotSpot translates to a catchable - * {@code InternalError} at an {@code Unsafe} intrinsic site) — the same fault - * a genuinely unbacked/sparse page raises on ZFS, but reproduced - * deterministically on ext4/xfs too. That is what lets recovery's mmap-fault - * guard be regression-tested on any CI runner rather than only on ZFS. */ - long length(String path); + default long length(String path) { + return Files.length(path); + } int lock(int fd); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java index 2b2ce724..28f72f96 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java @@ -24,6 +24,7 @@ package io.questdb.client.test.cutlass.qwp.client.sf.cursor; +import io.questdb.client.cutlass.qwp.client.QwpWebSocketSender; import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentManager; @@ -35,6 +36,7 @@ import org.junit.Before; import org.junit.Test; +import java.lang.reflect.Constructor; import java.nio.file.Paths; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -133,8 +135,11 @@ public void testCloseRetainsSlotWhileWorkerIsMidServicePass() throws Exception { + "worker was still mid service pass for its ring — a " + "replacement engine could acquire the slot and have its " + "segment files unlinked by the stale worker"); - } catch (Exception expected) { - // good — slot retained. + } catch (IllegalStateException expected) { + Assert.assertTrue( + expected.getMessage(), + expected.getMessage().contains("sf slot already in use") + ); } // Let the worker finish its pass (it abandons the spare: the @@ -174,6 +179,213 @@ public void testCloseRetainsSlotWhileWorkerIsMidServicePass() throws Exception { }); } + @Test(timeout = 30_000L) + public void testDeferredCloseRetainsOwnershipWithoutBlockingCaller() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/deferred-slot"; + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + CursorSendEngine engine = null; + boolean managerClosed = false; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + releaseWorker.await(20, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engine = new CursorSendEngine(slot, segSize, manager); + Assert.assertTrue("worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + manager.setWorkerJoinTimeoutMillis(50L); + engine.close(); + Assert.assertFalse("first close must remain incomplete", engine.isCloseCompleted()); + long started = System.nanoTime(); + engine.closeEventually(); + Assert.assertTrue( + "ownership transfer must not block the lifecycle thread", + System.nanoTime() - started < TimeUnit.SECONDS.toNanos(1) + ); + Thread.sleep(100L); + Assert.assertFalse( + "the deferred owner must retain resources while the manager stays stalled", + engine.isCloseCompleted() + ); + try (SlotLock ignored = SlotLock.acquire(slot)) { + Assert.fail("deferred cleanup must retain the slot lock while quiescence is stalled"); + } catch (IllegalStateException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("sf slot already in use")); + } + + releaseWorker.countDown(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (!engine.isCloseCompleted() && System.nanoTime() < deadline) { + Thread.sleep(1L); + } + Assert.assertTrue("deferred cleanup did not complete after quiescence", engine.isCloseCompleted()); + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after deferred cleanup", probe); + } + + manager.close(); + managerClosed = true; + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (engine != null) { + try { + engine.close(); + } catch (Throwable ignored) { + } + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + + @Test(timeout = 30_000L) + public void testOwnedManagerReapCompletesAfterQuiescenceTimeout() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + CountDownLatch closeJoinEntered = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + CursorSendEngine engine = null; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + releaseWorker.await(20, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.setBeforeJoinAttemptHook(() -> { + closeJoinEntered.countDown(); + releaseWorker.countDown(); + }); + manager.setWorkerJoinTimeoutMillis(50L); + + Constructor constructor = CursorSendEngine.class.getDeclaredConstructor( + String.class, + long.class, + SegmentManager.class, + boolean.class, + long.class + ); + constructor.setAccessible(true); + engine = constructor.newInstance( + null, + segSize, + manager, + true, + CursorSendEngine.DEFAULT_APPEND_DEADLINE_NANOS + ); + Assert.assertTrue("owned manager worker never entered its service pass", + workerBlocked.await(5, TimeUnit.SECONDS)); + + engine.close(); + Assert.assertEquals("owned manager close must reach its join fallback", 0L, + closeJoinEntered.getCount()); + Assert.assertTrue("owned manager close must reap the worker", manager.isWorkerReaped()); + Assert.assertTrue( + "a reaped owned manager is a stronger barrier than the timed-out ring wait", + engine.isCloseCompleted() + ); + } finally { + manager.setBeforeInstallSyncHook(null); + manager.setBeforeJoinAttemptHook(null); + releaseWorker.countDown(); + if (engine != null) { + engine.close(); + } else { + manager.close(); + } + } + }); + } + + @Test(timeout = 30_000L) + public void testOwnerCloseRetainsEngineUntilCleanupCompletes() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/owner-slot"; + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + QwpWebSocketSender sender = QwpWebSocketSender.createForTesting("localhost", 9000); + CursorSendEngine engine = null; + boolean managerClosed = false; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + releaseWorker.await(20, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engine = new CursorSendEngine(slot, segSize, manager); + sender.setCursorEngine(engine, true); + Assert.assertTrue("worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + manager.setWorkerJoinTimeoutMillis(50L); + sender.close(); + Assert.assertFalse( + "owner must not report the slot lock released while engine cleanup is incomplete", + sender.isSlotLockReleased() + ); + + releaseWorker.countDown(); + manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); + sender.close(); + Assert.assertTrue( + "a repeated owner close must retry the retained engine and report completion", + sender.isSlotLockReleased() + ); + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after owner retry", probe); + } + + manager.close(); + managerClosed = true; + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + try { + sender.close(); + } catch (Throwable ignored) { + } + if (engine != null) { + try { + engine.close(); + } catch (Throwable ignored) { + } + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + /** * Plain-positive path: after a normal close (worker quiesces promptly), * a second engine must be able to acquire and use the same slot. diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryCrcBenchmark.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryCrcBenchmark.java new file mode 100644 index 00000000..b9422b72 --- /dev/null +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryCrcBenchmark.java @@ -0,0 +1,143 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2026 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.client.test.cutlass.qwp.client.sf.cursor; + +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; +import io.questdb.client.std.MemoryTag; +import io.questdb.client.std.Unsafe; + +import java.nio.file.Path; + +/** + * Standalone large-corpus benchmark for the mmap-safe recovery CRC path. + * The generated segment contains one large frame, so the measured + * {@link MmapSegment#openExisting(String)} time is dominated by recovery CRC + * rather than frame-header traversal or syscalls. + *

+ * Run via Maven exec: + * 

+ *   mvn -pl core test-compile
+ *   mvn -pl core exec:java \
+ *     -Dexec.classpathScope=test \
+ *     -Dexec.mainClass=io.questdb.client.test.cutlass.qwp.client.sf.cursor.MmapSegmentRecoveryCrcBenchmark \
+ *     -Dexec.args="--corpus-bytes=512M --warmup=2 --iterations=5"
+ *
+ */ +public final class MmapSegmentRecoveryCrcBenchmark { + + private static final int DEFAULT_CORPUS_BYTES = 256 * 1024 * 1024; + private static final int DEFAULT_ITERATIONS = 5; + private static final int DEFAULT_WARMUP = 2; + + public static void main(String[] args) throws Exception { + int corpusBytes = DEFAULT_CORPUS_BYTES; + int frameBytes = -1; + int iterations = DEFAULT_ITERATIONS; + int warmup = DEFAULT_WARMUP; + for (String arg : args) { + if (arg.startsWith("--corpus-bytes=")) { + long parsed = parseSize(arg.substring("--corpus-bytes=".length())); + if (parsed <= 0 || parsed > Integer.MAX_VALUE - MmapSegment.FRAME_HEADER_SIZE) { + throw new IllegalArgumentException("corpus size out of range: " + parsed); + } + corpusBytes = (int) parsed; + } else if (arg.startsWith("--frame-bytes=")) { + frameBytes = Integer.parseInt(arg.substring("--frame-bytes=".length())); + } else if (arg.startsWith("--iterations=")) { + iterations = Integer.parseInt(arg.substring("--iterations=".length())); + } else if (arg.startsWith("--warmup=")) { + warmup = Integer.parseInt(arg.substring("--warmup=".length())); + } else { + throw new IllegalArgumentException("unknown option: " + arg); + } + } + if (iterations <= 0 || warmup < 0 || frameBytes == 0 || frameBytes < -1) { + throw new IllegalArgumentException("iterations/frame-bytes/warmup out of range"); + } + + int payloadBytes = frameBytes > 0 ? frameBytes : corpusBytes; + long frameCount = frameBytes > 0 + ? Math.max(1L, corpusBytes / (MmapSegment.FRAME_HEADER_SIZE + (long) payloadBytes)) + : 1L; + long scannedBytes = frameCount * (MmapSegment.FRAME_HEADER_SIZE + (long) payloadBytes); + Path dir = java.nio.file.Files.createTempDirectory("qdb-mmap-recovery-crc-"); + Path segmentPath = dir.resolve("0.sfa"); + long payload = Unsafe.malloc(payloadBytes, MemoryTag.NATIVE_DEFAULT); + try { + Unsafe.getUnsafe().setMemory(payload, payloadBytes, (byte) 0xA5); + long segmentBytes = MmapSegment.HEADER_SIZE + scannedBytes; + try (MmapSegment segment = MmapSegment.create(segmentPath.toString(), 0, segmentBytes)) { + for (long i = 0; i < frameCount; i++) { + if (segment.tryAppend(payload, payloadBytes) < 0) { + throw new AssertionError("failed to create benchmark frame " + i); + } + } + } + + for (int i = 0; i < warmup; i++) { + verifyRecovery(segmentPath, segmentBytes, frameCount); + } + + long start = System.nanoTime(); + for (int i = 0; i < iterations; i++) { + verifyRecovery(segmentPath, segmentBytes, frameCount); + } + long elapsed = System.nanoTime() - start; + double gib = (double) scannedBytes * iterations / (1024.0 * 1024.0 * 1024.0); + double seconds = elapsed / 1_000_000_000.0; + System.out.printf("corpus=%,d bytes, frames=%,d, payload=%,d bytes, iterations=%d, " + + "elapsed=%.3f s, throughput=%.3f GiB/s%n", + scannedBytes, frameCount, payloadBytes, iterations, seconds, gib / seconds); + } finally { + Unsafe.free(payload, payloadBytes, MemoryTag.NATIVE_DEFAULT); + java.nio.file.Files.deleteIfExists(segmentPath); + java.nio.file.Files.deleteIfExists(dir); + } + } + + private static long parseSize(String value) { + String s = value.trim().toUpperCase(); + long multiplier = 1; + if (s.endsWith("K") || s.endsWith("KB")) { + multiplier = 1024L; + s = s.substring(0, s.length() - (s.endsWith("KB") ? 2 : 1)); + } else if (s.endsWith("M") || s.endsWith("MB")) { + multiplier = 1024L * 1024L; + s = s.substring(0, s.length() - (s.endsWith("MB") ? 2 : 1)); + } else if (s.endsWith("G") || s.endsWith("GB")) { + multiplier = 1024L * 1024L * 1024L; + s = s.substring(0, s.length() - (s.endsWith("GB") ? 2 : 1)); + } + return Long.parseLong(s.trim()) * multiplier; + } + + private static void verifyRecovery(Path segmentPath, long expectedOffset, long expectedFrameCount) { + try (MmapSegment segment = MmapSegment.openExisting(segmentPath.toString())) { + if (segment.publishedOffset() != expectedOffset || segment.frameCount() != expectedFrameCount) { + throw new AssertionError("recovery did not preserve the benchmark frames"); + } + } + } +} diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java index 861f761c..58eb4cf2 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java @@ -40,57 +40,11 @@ import static org.junit.Assert.fail; /** - * Regression guard for the recovery-time SIGBUS hazard in {@link MmapSegment}. - *

- * On recovery, {@link MmapSegment#openExisting} maps a persisted {@code .sfa} - * to its stat length and scans frames straight out of the mapping. When a prior - * session left a sparse segment tail -- a truncate-based pre-allocation that - * never materialized the tail blocks, as happens on ZFS -- a read of an - * unbacked page raises the JVM's recoverable - * {@code InternalError("...unsafe memory access operation...")} (a translated - * SIGBUS). Recovery must treat that page as the boundary of recoverable data, - * keep every frame below it, and hand back a usable segment -- not let the - * error abort recovery of the whole slot (the reported ZFS-CI flake). - *

- * These tests drive the production entry point ({@code openExisting}), - * not the private scan methods via reflection. That matters for two reasons: - *

    - *
  • It exercises the real recovery path end to end.
    • - *
  • On pre-21 JDKs the mmap-fault {@code InternalError} is delivered - * imprecisely ("a fault occurred in a recent unsafe memory access - * operation in compiled Java code") and escapes a reflective - * {@code Method.invoke} frame instead of being caught inside the scan -- - * so a reflection-based test spuriously fails on the shipping JDK 8/11/17 - * even though the direct-call production path catches it fine.
    • - *
- * The fault-delivery mechanism the fix rests on was verified directly on the - * shipping/CI Java floor -- JDK 8 (Temurin 1.8.0_492) -- not merely inferred - * from the adjacent pre-21 LTS releases: the whole class passes there in both - * interpreter ({@code -Xint}) and JIT modes, HotSpot emits the exact pre-21 - * message above, and a direct {@code try/catch} catches the fault in - * interpreter, C1, and C2 modes. {@code isMmapAccessFault}'s shared - * {@code "unsafe memory access operation"} fragment matches that message while - * the JDK 21+-only needle it replaced does not -- the guard is live on JDK 8. - * The unbacked tail is produced portably by truncating the file down (dropping - * the tail blocks) and back up to the mapping size (leaving a sparse hole). A - * hole-faulting filesystem (ZFS) then faults on the read exactly as in - * production -- the case the fix must survive rather than fold the CRC through - * the native, JNI-side {@code Crc32c} where a SIGBUS is uncatchable and aborts - * the JVM. A hole-zero-filling filesystem (ext4) instead reads the hole back as - * zeroes, which fails the frame CRC; either way recovery must stop at the same - * boundary and recover the same frames. - *

- * Fail-on-revert on any filesystem. The sparse-hole tests above only - * fault on ZFS: on ext4/xfs the within-EOF hole zero-fills, so the scan stops - * via the CRC-mismatch / bad-magic branch and they stay green even with the - * mmap-fault guard reverted -- no regression protection on the ext4/xfs CI - * runners. The two {@code MapPastEof} tests below close that gap portably. - * They truncate the file down (freeing the tail blocks) and hand - * {@code openExisting} a {@link FilesFacade} that reports the original, larger - * length, so the mapping extends past real end-of-file. A read of a page beyond - * real EOF raises SIGBUS on every filesystem -- the same catchable - * {@code InternalError} an unbacked ZFS page raises -- so they exercise the - * real fault path (and fail on revert) on ext4/xfs too, not only on ZFS. + * Recovery regressions for sparse, short, corrupt, and unreadable segment + * files. {@link MmapSegment#openExisting} validates bytes through positional + * reads before mmap, so EOF and I/O failures remain synchronous on every + * supported HotSpot release and filesystem. Tests exercise the production + * entry point and assert partial recovery, per-file rejection, and cleanup. */ public class MmapSegmentRecoveryFaultTest { @@ -141,12 +95,9 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception { * The harder case: a frame whose 8-byte header sits on a backed page but * whose payload reaches into the unbacked hole (a torn write leaves a real * positive {@code payloadLen} with the payload spanning the boundary). The - * CRC fold therefore reads across the backed-to-unbacked edge. Recovery - * must reject that frame and keep the one below it -- and, crucially, must - * do so via {@code Unsafe} reads: the native, JNI-side {@code Crc32c} over - * an unbacked page raises a SIGBUS that HotSpot cannot translate, aborting - * the whole JVM (verified: an {@code hs_err} in - * {@code Java_io_questdb_client_std_Crc32c_update}). + * CRC fold therefore reads across the backed-to-unbacked edge. Positional + * reads must reject that frame and keep the one below it without exposing + * the mmap to Java or native CRC code during validation. */ @Test public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { @@ -178,18 +129,9 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { } /** - * M1 regression: the header block (magic/version/baseSeq) is read before - * {@code scanFrames}, so an unbacked page 0 faults ahead of the guarded - * scan. {@link MmapSegment#openExisting} must surface that as a - * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} - * catches to skip just this {@code .sfa} -- and never let the raw - * {@code InternalError} escape and abort recovery of every sibling segment. - *

- * Portable across filesystems: on a hole-faulting FS (ZFS) the fault is - * converted to a {@code MmapSegmentException} in {@code openExisting}'s - * catch; on a hole-zero-filling FS (ext4) page 0 reads back as zeroes, so - * the magic check fails and throws {@code MmapSegmentException} directly. - * Either way the file is skippable, not fatal. + * An unbacked page-zero header must produce the per-file + * {@link MmapSegmentException} that SegmentRing skips, not poison recovery + * of valid sibling files. */ @Test public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { @@ -209,36 +151,27 @@ public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { } /** - * Portable fail-on-revert guard for the recovery mmap-fault handling on the - * scan path. Unlike {@link #testRecoveryKeepsFramesBeforeUnbackedTail} - * (which only faults on ZFS), this maps the file past real EOF via the - * length-injecting facade, so the scan's read of the beyond-EOF page faults - * on ext4/xfs too. The fix must recognize that fault and keep - * recovery safe -- never a JVM abort, never a raw {@code InternalError} - * escaping into {@code SegmentRing}'s recovery loop. Revert the - * {@code scanFrames}/{@code openExisting} mmap-fault guard (or fold the CRC - * back through native {@code Crc32c}) and this errors or aborts the fork. - *

- * Two handled outcomes are accepted, because which one occurs depends on - * whether the recovery methods are JIT-compiled at fault time: - *

    - *
  • Interpreter / C1: {@code scanFrames}'s own - * {@code catch (InternalError)} fires, so the frame below the tear is - * recovered and a usable segment is returned.
    • - *
  • C2: once {@code scanFrames} is inlined into - * {@code openExisting}, HotSpot delivers the async unsafe-access - * {@code InternalError} to {@code openExisting}'s outer - * {@code catch (Throwable)} instead of the inlined inner one, which - * converts the file to a skippable {@link MmapSegmentException}. - * Still fully handled -- {@code SegmentRing} skips just this - * {@code .sfa} rather than aborting the slot.
    • - *
- * (The C2 delivery imprecision is a property of HotSpot's async - * unsafe-access fault handling, not of this seam; the seam only makes it - * reproducible off ZFS.) + * A hard positional-read error rejects only this segment and releases the + * fd and fixed native recovery buffer on the failure path. */ @Test - public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception { + public void testReadErrorRejectsSegmentAndClosesFile() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-read-error.sfa"; + writeSegment(path, 4L, new int[]{64}); + RecoveryFilesFacade ff = new RecoveryFilesFacade(path, Files.length(path), 0L); + try { + MmapSegment.openExisting(ff, path).close(); + fail("expected MmapSegmentException for recovery read error"); + } catch (MmapSegmentException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains("read failed")); + } + assertEquals("failed recovery must close the segment fd", 1, ff.targetCloseCount()); + }); + } + + @Test + public void testScanStopsAtShortReadBeforeReportedEof() throws Exception { TestUtils.assertMemoryLeak(() -> { final String path = tmpDir + "/seg-mappasteof-scan.sfa"; final long page = Files.PAGE_SIZE; @@ -249,51 +182,97 @@ public void testScanFaultOnMapPastEofIsHandledAnyFilesystem() throws Exception { // Free every block past the first page: the file is now exactly one // (fully backed) page, with nothing beyond it on disk. truncateTo(path, page); - // Report twice the real length so openExisting maps a second, - // beyond-EOF page; the scan faults reading it on any filesystem. - FilesFacade ff = new MapPastEofFacade(path, 2 * page); + // The first fd-size read reports two pages while pread reaches EOF + // after one. Recovery must retain the valid frame, re-read the fd + // size, and map only the real page. + FilesFacade ff = new RecoveryFilesFacade(path, 2 * page); try (MmapSegment seg = MmapSegment.openExisting(ff, path)) { - // Interpreter / C1: graceful partial recovery. - assertEquals("the frame below the beyond-EOF page must be recovered", 1L, seg.frameCount()); - assertEquals("scan must stop at the beyond-EOF boundary", page, seg.publishedOffset()); - assertEquals("a beyond-EOF page is not a torn write", 0L, seg.tornTailBytes()); - } catch (MmapSegmentException skippedUnderC2) { - // C2: the inlined fault escaped to openExisting's outer catch and - // was converted to a per-file skip. Assert it is the recognized - // mmap fault (not some other data error) so a revert -- which - // lets a raw InternalError through instead -- still fails here. - assertTrue(skippedUnderC2.getMessage(), - skippedUnderC2.getMessage().contains("unsafe memory access operation")); + assertEquals("the frame below EOF must be recovered", 1L, seg.frameCount()); + assertEquals("scan must stop at EOF", page, seg.publishedOffset()); + assertEquals("a short EOF is not a torn write", 0L, seg.tornTailBytes()); } }); } + @Test + public void testShrinkBelowValidatedPrefixRejectsSegment() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-shrink-after-scan.sfa"; + writeSegment(path, 6L, new int[]{64}); + RecoveryFilesFacade ff = new RecoveryFilesFacade( + path, + Files.length(path), + Long.MAX_VALUE, + Long.MAX_VALUE, + MmapSegment.HEADER_SIZE + ); + try { + MmapSegment.openExisting(ff, path).close(); + fail("expected MmapSegmentException after file shrink"); + } catch (MmapSegmentException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains("shrank below recovered data")); + } + assertEquals("shrink rejection must close the segment fd", 1, ff.targetCloseCount()); + }); + } + + @Test + public void testShortReadsAcrossFrameBoundariesRecoverAllFrames() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-short-reads.sfa"; + long expectedOffset = writeSegment(path, 7L, new int[]{1, 17, 64}); + RecoveryFilesFacade ff = new RecoveryFilesFacade( + path, + Files.length(path), + Long.MAX_VALUE, + 3L + ); + try (MmapSegment segment = MmapSegment.openExisting(ff, path)) { + assertEquals(3L, segment.frameCount()); + assertEquals(expectedOffset, segment.publishedOffset()); + assertEquals(0L, segment.tornTailBytes()); + } + }); + } + + @Test + public void testSuccessfulRecoveryClosesThroughFacadeExactlyOnce() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-successful-facade-close.sfa"; + writeSegment(path, 12L, new int[]{64}); + RecoveryFilesFacade ff = new RecoveryFilesFacade(path, Files.length(path)); + MmapSegment segment = MmapSegment.openExisting(ff, path); + assertEquals("successful recovery must not close before segment ownership ends", + 0, ff.targetCloseCount()); + segment.close(); + segment.close(); + assertEquals("successful recovery must close through its facade exactly once", + 1, ff.targetCloseCount()); + }); + } + /** - * Portable fail-on-revert guard for the {@code openExisting} header-block - * guard. The file is truncated to empty and the facade reports a full page, - * so the very first header read (magic) lands on a beyond-EOF page and - * faults on any filesystem. {@code openExisting} must convert that to a - * {@link MmapSegmentException} -- the per-file signal {@code SegmentRing} - * skips on -- not let the raw {@code InternalError} escape and abort recovery - * of every sibling. Revert the header-block conversion and this throws - * {@code InternalError} instead of {@code MmapSegmentException}. + * A header short-read produces the per-file exception that SegmentRing + * skips; recovery never maps the stale reported length. */ @Test - public void testHeaderFaultOnMapPastEofIsSkippableAnyFilesystem() throws Exception { + public void testHeaderShortReadIsSkippable() throws Exception { TestUtils.assertMemoryLeak(() -> { final String path = tmpDir + "/seg-mappasteof-header.sfa"; final long page = Files.PAGE_SIZE; writeSegment(path, 9L, new int[]{64}); - // Free every block: the file is now empty, so even page 0 (the - // header) is beyond EOF under the reported one-page mapping. + // The facade reports a page on the first fd-size read, while the + // positional header read sees the real empty file. truncateTo(path, 0L); - FilesFacade ff = new MapPastEofFacade(path, page); + FilesFacade ff = new RecoveryFilesFacade(path, page); try { MmapSegment.openExisting(ff, path).close(); - fail("expected MmapSegmentException for a beyond-EOF header page"); + fail("expected MmapSegmentException for a short header read"); } catch (MmapSegmentException expected) { - // ok -- SegmentRing's per-file catch skips just this file - // instead of aborting recovery of the whole slot. + assertTrue( + "the fd-length seam must reach the positional header read: " + expected.getMessage(), + expected.getMessage().contains("short read of segment header") + ); } }); } @@ -360,19 +339,50 @@ private static void truncateTo(String path, long keepBytes) { } /** - * A {@link FilesFacade} that reports an inflated stat length for one target - * path so {@code openExisting} maps that file past end-of-file (see - * {@link FilesFacade#length(String)}); every other call, including - * {@code length} for any other path, delegates to the production - * {@link FilesFacade#INSTANCE}. + * Recovery fault seam. The first fd-size read can report a stale larger + * value, subsequent reads return the real size, and positional reads may + * inject a hard error at a selected offset. All unrelated operations + * delegate to {@link FilesFacade#INSTANCE}. */ - private static final class MapPastEofFacade implements FilesFacade { + private static final class RecoveryFilesFacade implements FilesFacade { + private final long failReadAtOffset; + private final long maxReadSize; private final long reportedLength; + private final long shrinkOnSecondLengthTo; private final String targetPath; + private int lengthCallCount; + private int targetCloseCount; + private int targetFd = -1; + + RecoveryFilesFacade(String targetPath, long reportedLength) { + this(targetPath, reportedLength, Long.MAX_VALUE, Long.MAX_VALUE, -1L); + } + + RecoveryFilesFacade(String targetPath, long reportedLength, long failReadAtOffset) { + this(targetPath, reportedLength, failReadAtOffset, Long.MAX_VALUE, -1L); + } + + RecoveryFilesFacade( + String targetPath, + long reportedLength, + long failReadAtOffset, + long maxReadSize + ) { + this(targetPath, reportedLength, failReadAtOffset, maxReadSize, -1L); + } - MapPastEofFacade(String targetPath, long reportedLength) { + RecoveryFilesFacade( + String targetPath, + long reportedLength, + long failReadAtOffset, + long maxReadSize, + long shrinkOnSecondLengthTo + ) { this.targetPath = targetPath; this.reportedLength = reportedLength; + this.failReadAtOffset = failReadAtOffset; + this.maxReadSize = maxReadSize; + this.shrinkOnSecondLengthTo = shrinkOnSecondLengthTo; } @Override @@ -387,7 +397,12 @@ public long allocNativePath(String path) { @Override public int close(int fd) { - return INSTANCE.close(fd); + int result = INSTANCE.close(fd); + if (fd == targetFd) { + targetCloseCount++; + targetFd = -1; + } + return result; } @Override @@ -432,6 +447,14 @@ public int fsync(int fd) { @Override public long length(int fd) { + if (fd == targetFd) { + if (lengthCallCount++ == 0) { + return reportedLength; + } + if (shrinkOnSecondLengthTo >= 0L) { + assertTrue("injected truncate failed", INSTANCE.truncate(fd, shrinkOnSecondLengthTo)); + } + } return INSTANCE.length(fd); } @@ -442,7 +465,7 @@ public long length(long pathPtr) { @Override public long length(String path) { - return targetPath.equals(path) ? reportedLength : INSTANCE.length(path); + return INSTANCE.length(path); } @Override @@ -467,7 +490,11 @@ public int openCleanRW(long pathPtr) { @Override public int openRW(String path) { - return INSTANCE.openRW(path); + int fd = INSTANCE.openRW(path); + if (targetPath.equals(path)) { + targetFd = fd; + } + return fd; } @Override @@ -477,7 +504,10 @@ public int openRW(long pathPtr) { @Override public long read(int fd, long addr, long len, long offset) { - return INSTANCE.read(fd, addr, len, offset); + if (fd == targetFd && offset >= failReadAtOffset) { + return -1L; + } + return INSTANCE.read(fd, addr, Math.min(len, maxReadSize), offset); } @Override @@ -500,6 +530,10 @@ public boolean truncate(int fd, long size) { return INSTANCE.truncate(fd, size); } + int targetCloseCount() { + return targetCloseCount; + } + @Override public long write(int fd, long addr, long len, long offset) { return INSTANCE.write(fd, addr, len, offset); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java index 177ee5f6..718ccefb 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentTest.java @@ -205,6 +205,27 @@ public void testCreateRepeatedAllocateFailuresDoNotAccumulateOrphans() throws Ex }); } + @Test + public void testCreateSuccessClosesThroughFacadeExactlyOnce() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String path = tmpDir + "/seg-facade-close.sfa"; + FaultyFilesFacade ff = new FaultyFilesFacade(); + MmapSegment segment = MmapSegment.create(ff, path, 0L, 4096L); + assertEquals("successful create must not close before segment ownership ends", 0, ff.closeCalls); + segment.close(); + segment.close(); + assertEquals("successful create must close through its facade exactly once", 1, ff.closeCalls); + }); + } + + @Test + public void testFilesFacadePathLengthMethodIsDefault() throws Exception { + assertTrue( + "FilesFacade.length(String) must remain binary-compatible for existing implementations", + FilesFacade.class.getMethod("length", String.class).isDefault() + ); + } + @Test public void testFirstFrameCrcCorruptionFlagsTornTailAndPreservesFile() throws Exception { TestUtils.assertMemoryLeak(() -> { @@ -212,7 +233,7 @@ public void testFirstFrameCrcCorruptionFlagsTornTailAndPreservesFile() throws Ex // frames are followed by garbage. None cover frame[0] itself // being corrupt — yet a single bit-flip on the CRC of frame[0] // at rest (bit-rot, partial-page-write at crash) is the - // worst-case data-loss trigger: scanFrames bails at HEADER_SIZE + // worst-case data-loss trigger: recovery stops at HEADER_SIZE // and frameCount drops to 0, even though valid frames still // sit on disk past the corrupt header. // @@ -255,7 +276,7 @@ public void testFirstFrameCrcCorruptionFlagsTornTailAndPreservesFile() throws Ex Files.exists(path)); try (MmapSegment seg = MmapSegment.openExisting(path)) { - assertEquals("scanFrames must bail at the corrupt frame[0]", + assertEquals("recovery must stop at the corrupt frame[0]", 0L, seg.frameCount()); assertEquals("publishedOffset must rewind to the header end", MmapSegment.HEADER_SIZE, seg.publishedOffset()); @@ -328,6 +349,34 @@ public void testOpenExistingRejectsCorruptHeader() throws Exception { }); } + @Test + public void testRecoveryCrcMatchesNativeAcrossAlignmentsAndTails() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String path = tmpDir + "/seg-crc-alignments.sfa"; + int[] payloadLengths = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 15, 16, 17, 31, 32, 33, + 63, 64, 65, 127, 128, 129, 255, 256, 257, 1023}; + long buf = Unsafe.malloc(1023, MemoryTag.NATIVE_DEFAULT); + long expectedEnd = MmapSegment.HEADER_SIZE; + try { + try (MmapSegment seg = MmapSegment.create(path, 7L, 16 * 1024)) { + for (int i = 0; i < payloadLengths.length; i++) { + int payloadLength = payloadLengths[i]; + fillPattern(buf, payloadLength, i * 31 + 17); + assertEquals(expectedEnd, seg.tryAppend(buf, payloadLength)); + expectedEnd += MmapSegment.FRAME_HEADER_SIZE + payloadLength; + } + } + + try (MmapSegment recovered = MmapSegment.openExisting(path)) { + assertEquals(expectedEnd, recovered.publishedOffset()); + assertEquals(payloadLengths.length, recovered.frameCount()); + } + } finally { + Unsafe.free(buf, 1023, MemoryTag.NATIVE_DEFAULT); + } + }); + } + @Test public void testRecoveryDoesNotFlagCleanPartialFill() throws Exception { TestUtils.assertMemoryLeak(() -> { diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java index e551c6f8..576fc5ac 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/PrReviewRedTests.java @@ -79,8 +79,8 @@ public void tearDown() { /** * Finding C1 / C10 — first-frame CRC corruption silently deletes the segment. *

- * If frame[0] of a recovered .sfa fails CRC validation, scanFrames returns - * lastGood=HEADER_SIZE, countFrames returns 0, and SegmentRing.openExisting + * If frame[0] of a recovered .sfa fails CRC validation, recovery returns + * lastGood=HEADER_SIZE and frameCount=0, and SegmentRing.openExisting * unlinks the file as an "empty hot-spare leftover" — destroying every frame * that physically followed the corrupt header. The torn-tail WARN inside * MmapSegment.openExisting is dropped on the floor. @@ -99,7 +99,11 @@ public void testC1_recoveryMustNotUnlinkSegmentWithCorruptFirstFrame() throws Ex for (int i = 0; i < 32; i++) { Unsafe.getUnsafe().putByte(buf + i, (byte) i); } - Assert.assertTrue("setup: first append must succeed", seg.tryAppend(buf, 32) >= 0); + // A valid zero-length first frame has a non-zero CRC and a + // zero payload length. Corrupting that CRC to zero makes its + // entire 8-byte header zero, identical to unwritten space if + // recovery inspects only the failed header. + Assert.assertTrue("setup: zero-length first append must succeed", seg.tryAppend(buf, 0) >= 0); Assert.assertTrue("setup: second append must succeed", seg.tryAppend(buf, 32) >= 0); Assert.assertTrue("setup: third append must succeed", seg.tryAppend(buf, 32) >= 0); Assert.assertEquals("setup: three frames written", 3L, seg.frameCount()); @@ -110,13 +114,13 @@ public void testC1_recoveryMustNotUnlinkSegmentWithCorruptFirstFrame() throws Ex Assert.assertTrue("setup: file must exist on disk", Files.exists(segPath)); // Corrupt the CRC field of frame[0] (offset HEADER_SIZE..HEADER_SIZE+4). - // A single bit flip is enough; we overwrite the whole 4-byte field with - // a value statistically guaranteed to mismatch any real CRC. + // Zero mismatches the valid zero-length frame's CRC and leaves the + // failed frame's complete 8-byte header all-zero. int fd = Files.openRW(segPath); Assert.assertTrue("setup: openRW failed", fd >= 0); long badCrcBuf = Unsafe.malloc(4, MemoryTag.NATIVE_DEFAULT); try { - Unsafe.getUnsafe().putInt(badCrcBuf, 0xDEADBEEF); + Unsafe.getUnsafe().putInt(badCrcBuf, 0); Files.write(fd, badCrcBuf, 4, MmapSegment.HEADER_SIZE); } finally { Unsafe.free(badCrcBuf, 4, MemoryTag.NATIVE_DEFAULT); @@ -128,7 +132,7 @@ public void testC1_recoveryMustNotUnlinkSegmentWithCorruptFirstFrame() throws Ex // Run recovery. SegmentRing recovered = SegmentRing.openExisting(tmpDir, 64 * 1024); try { - // The bug: openExisting sees frameCount=0 (because scanFrames + // The bug: openExisting sees frameCount=0 (because recovery // bailed at the corrupt frame[0]) and treats the segment as // an "empty hot-spare leftover" — closing AND UNLINKING the // file. The user's frames 1, 2, 3 are gone forever; the only diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java index 7ed0ae23..b0e5a3f9 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java @@ -181,13 +181,18 @@ public void testCloseDoesNotFreePathScratchWhenWorkerStillAlive() throws Excepti Assert.assertTrue("path scratch was freed while worker was still alive", readPathScratchImpl(manager) != 0L); + // Do not call close() again: the worker-exit branch is the sole + // owner of scratch after the timed-out close handed it off. releaseWorker.countDown(); - manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); - manager.close(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (!manager.hasWorkerLoopExited() && System.nanoTime() < deadline) { + Thread.sleep(1L); + } + Assert.assertTrue("worker did not exit after release", manager.hasWorkerLoopExited()); managerClosed = true; - Assert.assertNull("successful close should clear workerThread", + Assert.assertNotNull("no retry close should be needed to clear native scratch", readWorkerThread(manager)); - Assert.assertEquals("successful close should free path scratch", + Assert.assertEquals("worker exit should free path scratch without another close", 0L, readPathScratchImpl(manager)); if (hookErr.get() != null) { throw new AssertionError("install hook failed", hookErr.get()); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerWatermarkDeregisterRaceTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerWatermarkDeregisterRaceTest.java index 29794b68..5005604a 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerWatermarkDeregisterRaceTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerWatermarkDeregisterRaceTest.java @@ -41,6 +41,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import static org.junit.Assert.assertEquals; @@ -77,6 +78,78 @@ public void tearDown() { rmDirRecursive(tmpDir); } + @Test(timeout = 15_000L) + public void testStaleSnapshotBeforeClaimSkipsDeregisteredSecondRing() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + + 4 * (MmapSegment.FRAME_HEADER_SIZE + 32); + SegmentManager mgr = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + SegmentRing firstRing = new SegmentRing(MmapSegment.createInMemory(0L, segSize), segSize); + SegmentRing secondRing = new SegmentRing(MmapSegment.createInMemory(0L, segSize), segSize); + CountDownLatch beforeSecondClaim = new CountDownLatch(1); + CountDownLatch releaseSecondClaim = new CountDownLatch(1); + CountDownLatch secondAttemptFinished = new CountDownLatch(1); + AtomicInteger claimAttempts = new AtomicInteger(); + AtomicInteger installAttempts = new AtomicInteger(); + AtomicInteger serviceAttempts = new AtomicInteger(); + AtomicReference hookError = new AtomicReference<>(); + boolean managerClosed = false; + try { + mgr.register(firstRing, null, null); + mgr.register(secondRing, null, null); + mgr.setBeforeInstallSyncHook(installAttempts::incrementAndGet); + mgr.setBeforeServiceClaimHook(() -> { + if (claimAttempts.incrementAndGet() != 2) { + return; + } + beforeSecondClaim.countDown(); + try { + if (!releaseSecondClaim.await(10, TimeUnit.SECONDS)) { + hookError.compareAndSet(null, + new AssertionError("timed out waiting to release second stale claim")); + } + } catch (Throwable t) { + hookError.compareAndSet(null, t); + } + }); + mgr.setAfterServiceHook(() -> { + if (serviceAttempts.incrementAndGet() == 2) { + secondAttemptFinished.countDown(); + } + }); + mgr.start(); + + assertTrue("worker never reached the second snapshotted ring before claim", + beforeSecondClaim.await(5, TimeUnit.SECONDS)); + assertEquals("first snapshotted ring must enter provisioning", 1, installAttempts.get()); + mgr.deregister(secondRing); + releaseSecondClaim.countDown(); + assertTrue("worker never completed the stale second-ring service attempt", + secondAttemptFinished.await(5, TimeUnit.SECONDS)); + if (hookError.get() != null) { + throw new AssertionError("service hook failed", hookError.get()); + } + assertEquals( + "a ring deregistered after snapshot but before claim must not enter serviceRing0", + 1, installAttempts.get() + ); + + mgr.close(); + managerClosed = true; + } finally { + mgr.setAfterServiceHook(null); + mgr.setBeforeInstallSyncHook(null); + mgr.setBeforeServiceClaimHook(null); + releaseSecondClaim.countDown(); + if (!managerClosed) { + mgr.close(); + } + firstRing.close(); + secondRing.close(); + } + }); + } + @Test(timeout = 15_000L) public void testStaleWorkerDoesNotWriteThroughUnmappedWatermarkAfterDeregister() throws Exception { TestUtils.assertMemoryLeak(() -> { diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryUnlinkTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryUnlinkTest.java index 9f533fc7..b52019db 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryUnlinkTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryUnlinkTest.java @@ -25,6 +25,7 @@ package io.questdb.client.test.cutlass.qwp.client.sf.cursor; import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException; import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing; import io.questdb.client.std.Files; import io.questdb.client.test.tools.TestUtils; @@ -75,6 +76,47 @@ public void tearDown() { Files.remove(tmpDir); } + @Test + public void testRecoveryGapDiagnosticExplainsDeletionAndStorageFailures() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String firstPath = tmpDir + "/sf-first.sfa"; + String secondPath = tmpDir + "/sf-after-gap.sfa"; + long buf = io.questdb.client.std.Unsafe.malloc( + 32, + io.questdb.client.std.MemoryTag.NATIVE_DEFAULT + ); + try { + io.questdb.client.std.Unsafe.getUnsafe().setMemory(buf, 32, (byte) 1); + try (MmapSegment first = MmapSegment.create(firstPath, 0L, SEGMENT_SIZE)) { + Assert.assertTrue(first.tryAppend(buf, 32) >= 0); + } + // One frame at base 0 means the next segment must start at 1. + // Starting at 2 creates a deterministic one-FSN gap. + try (MmapSegment second = MmapSegment.create(secondPath, 2L, SEGMENT_SIZE)) { + Assert.assertTrue(second.tryAppend(buf, 32) >= 0); + } + } finally { + io.questdb.client.std.Unsafe.free( + buf, + 32, + io.questdb.client.std.MemoryTag.NATIVE_DEFAULT + ); + } + + try { + SegmentRing.openExisting(tmpDir, SEGMENT_SIZE).close(); + Assert.fail("expected recovered FSN gap"); + } catch (MmapSegmentException expected) { + String message = expected.getMessage(); + Assert.assertTrue(message, message.contains("FSN gap in recovered segments")); + Assert.assertTrue(message, message.contains("a segment was deleted")); + Assert.assertTrue(message, message.contains("sealed segment's tail was truncated")); + Assert.assertTrue(message, message.contains("sparse/unbacked page or disk media error")); + Assert.assertTrue(message, message.contains("check disk health")); + } + }); + } + @Test public void testRecoveryUnlinksEmptyOrphanSegments() throws Exception { TestUtils.assertMemoryLeak(() -> { diff --git a/core/src/test/java/io/questdb/client/test/impl/SenderPoolSfTest.java b/core/src/test/java/io/questdb/client/test/impl/SenderPoolSfTest.java index cfef2feb..f3e3c543 100644 --- a/core/src/test/java/io/questdb/client/test/impl/SenderPoolSfTest.java +++ b/core/src/test/java/io/questdb/client/test/impl/SenderPoolSfTest.java @@ -792,10 +792,11 @@ public void testStartupRecoveryRetiresSlotWhenRecovererCloseLeavesFlockHeld() th // Phase 2: ack-ing server + a new pool whose injected factory forges // the exact leak symptom for the recovery build of slot 0. The // factory returns a real, flock-holding QwpWebSocketSender but - // pre-sets closed=true, so the recovery close() is a complete no-op - // (checkNotClosed short-circuits drain too): the flock stays held - // and slotLockReleased never flips -- precisely a refused I/O-thread - // stop. flockReleased(recoverer) must therefore report false. + // pre-sets closed=true and cursorEngineCloseDelegated=true, modeling + // a close handed to an I/O thread that is still live. Repeated owner + // close calls must not race that delegated owner: the flock stays + // held and slotLockReleased remains false. flockReleased(recoverer) + // must therefore report false. CountingAckHandler handler = new CountingAckHandler(); try (TestWebSocketServer ack = new TestWebSocketServer(handler)) { int ackPort = ack.getPort(); @@ -809,6 +810,7 @@ public void testStartupRecoveryRetiresSlotWhenRecovererCloseLeavesFlockHeld() th if (idx == 0) { try { setBooleanField(real, "closed", true); + setBooleanField(real, "cursorEngineCloseDelegated", true); } catch (Exception e) { throw new RuntimeException(e); } @@ -858,6 +860,7 @@ public void testStartupRecoveryRetiresSlotWhenRecovererCloseLeavesFlockHeld() th // close it for real, otherwise assertMemoryLeak trips. Sender leaked = forged.get(); if (leaked != null) { + setBooleanField(leaked, "cursorEngineCloseDelegated", false); setBooleanField(leaked, "closed", false); leaked.close(); } From 9f3cb329ca13514b8a5d16dc5293f7685f081b79 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 21:28:29 +0100 Subject: [PATCH 04/12] Add ROLE_CHANGE (4001) close code for the role-change handoff The server's role-change CLOSE now carries the private-use code 4001 (RFC 6455 s7.4.2) instead of NORMAL_CLOSURE, so the client's verbatim CLOSE echo proves it actually read the server's CLOSE frame. Classify ROLE_CHANGE as orderly in the send loop (strike-exempt, paced recycle, reconnect-eligible), matching the treatment the handoff got as 1000. Pin the classification with a poison-frame test: repeated demote closes at an unadvanced head FSN must never latch a PROTOCOL_VIOLATION terminal. --- .../sf/cursor/CursorWebSocketSendLoop.java | 13 ++--- .../qwp/websocket/WebSocketCloseCode.java | 12 +++++ ...ursorWebSocketSendLoopPoisonFrameTest.java | 48 +++++++++++++++++++ design/qwp-nack-policy-v2.md | 12 +++-- 4 files changed, 75 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java index 185082f6..afb7189a 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java @@ -321,7 +321,7 @@ public final class CursorWebSocketSendLoop implements QuietCloseable { private long highestOkFsn = -1L; // Zero-progress recycle pacer (I/O thread only; survives reconnects). // Counts consecutive strike-EXEMPT recycles -- orderly closes - // (NORMAL_CLOSURE/GOING_AWAY), non-orderly closes before any send on the + // (NORMAL_CLOSURE/GOING_AWAY/ROLE_CHANGE), non-orderly closes before any send on the // connection, and pre-send RETRIABLE_OTHER rejections -- with no // acceptance progress in between. These paths deliberately carry no // poison strike (they are not a verdict on the bytes), which also exempts @@ -1527,7 +1527,7 @@ private void failPaced(Throwable initial) { /** * Recycle path for strike-exempt wire events: orderly closes - * (NORMAL_CLOSURE / GOING_AWAY), non-orderly closes before any send on + * (NORMAL_CLOSURE / GOING_AWAY / ROLE_CHANGE), non-orderly closes before any send on * the connection, and RETRIABLE_OTHER rejections (pre- and post-send: * NOT_WRITABLE is a node-state verdict, not a frame verdict). None of * these implicate the head frame, so they carry no poison strike -- but that @@ -2172,11 +2172,12 @@ public void onClose(int code, String reason) { // after this connection already sent the head frame counts a poison // strike; maxHeadFrameRejections consecutive strikes at the same // head FSN with no ack progress escalate to a typed terminal. Orderly - // closes (NORMAL_CLOSURE role-change handoff, GOING_AWAY restart - // drain) never count strikes — they are the server asking us to go - // elsewhere, not a verdict on the bytes. + // closes (ROLE_CHANGE role-change handoff, NORMAL_CLOSURE, GOING_AWAY + // restart drain) never count strikes — they are the server asking us + // to go elsewhere, not a verdict on the bytes. boolean orderly = code == WebSocketCloseCode.NORMAL_CLOSURE - || code == WebSocketCloseCode.GOING_AWAY; + || code == WebSocketCloseCode.GOING_AWAY + || code == WebSocketCloseCode.ROLE_CHANGE; LineSenderException cause = new LineSenderException( "WebSocket closed by server: code=" + code + " reason=" + reason); if (!orderly && nextWireSeq > 0) { diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java b/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java index 3a86bf6e..01d58101 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java @@ -84,6 +84,16 @@ public final class WebSocketCloseCode { * Reserved for future use. */ public static final int RESERVED = 1004; + /** + * Role-change close (4001). QWP application-defined code in the RFC 6455 + * Section 7.4.2 private-use range: the server closed because its role + * changed (primary demoted); reconnect-eligible, not a verdict on the + * bytes. Deliberately distinct from {@link #NORMAL_CLOSURE} so the + * client's verbatim CLOSE echo proves to the server that the client + * received the server's CLOSE (and, by TCP ordering, everything before + * it -- the final durable ack included). + */ + public static final int ROLE_CHANGE = 4001; /** * TLS handshake (1015). * Reserved value. MUST NOT be sent in a Close frame. @@ -134,6 +144,8 @@ public static String describe(int code) { return "Internal Error"; case TLS_HANDSHAKE: return "TLS Handshake"; + case ROLE_CHANGE: + return "Role Change (QWP)"; default: if (code >= 3000 && code < 4000) { return "Library/Framework Code (" + code + ")"; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java index 34ae5518..cc395861 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java @@ -32,6 +32,7 @@ import io.questdb.client.cutlass.qwp.client.WebSocketResponse; import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop; +import io.questdb.client.cutlass.qwp.websocket.WebSocketCloseCode; import io.questdb.client.network.PlainSocketFactory; import io.questdb.client.std.Files; import io.questdb.client.std.MemoryTag; @@ -244,6 +245,43 @@ public void testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine() throws Exception }); } + @Test + public void testRoleChangeCloseIsOrderlyNeverCountsStrikes() throws Exception { + // The server's role-change close (ROLE_CHANGE, 4001) is the demote + // handoff: "go elsewhere", not a verdict on the bytes. Like + // NORMAL_CLOSURE and GOING_AWAY it must never count a poison strike. + // Dropping ROLE_CHANGE from the orderly set would pass the rest of + // this suite while latching a PROTOCOL_VIOLATION terminal after + // maxHeadFrameRejections demote closes at an unadvanced head FSN -- + // turning a routine failover into a false data-poison verdict. + // Mirrors testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine with the + // opposite expectation. + TestUtils.assertMemoryLeak(() -> { + List clients = new ArrayList<>(); + try (CursorSendEngine engine = newEngine()) { + appendFrames(engine, 2); + CursorWebSocketSendLoop loop = newDurableLoop(engine, clients); + setSentCount(loop, 2); + + deliverOk(loop, 0, names("trades"), txns(7L)); + for (int i = 0; i < MAX_REJECTIONS + 1; i++) { + // Role-change close after at least one send on this + // connection: strike-exempt, so no number of repeats may + // escalate. Restore the sent count after each recycle, + // exactly like the non-orderly variant. + setSentCount(loop, 2); + deliverRoleChangeClose(loop); + } + + // Must NOT throw: orderly closes never escalate to a typed + // terminal, however many accumulate at the same head FSN. + loop.checkError(); + } finally { + closeAll(clients); + } + }); + } + @Test public void testNackRecycleIsPacedAgainstHealthyServer() throws Exception { // A reachable, healthy server that NACKs the head frame (RETRIABLE) @@ -907,6 +945,16 @@ private static void deliverOrderlyClose(CursorWebSocketSendLoop loop) throws Exc m.invoke(handler, 1001, "server draining"); // GOING_AWAY: orderly, strike-exempt } + private static void deliverRoleChangeClose(CursorWebSocketSendLoop loop) throws Exception { + Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler"); + f.setAccessible(true); + Object handler = f.get(loop); + Method m = handler.getClass().getDeclaredMethod("onClose", int.class, String.class); + m.setAccessible(true); + // ROLE_CHANGE (4001): the demote handoff, orderly, strike-exempt + m.invoke(handler, WebSocketCloseCode.ROLE_CHANGE, "role change: primary demoted"); + } + private static void deliverNonOrderlyClose(CursorWebSocketSendLoop loop) throws Exception { Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler"); f.setAccessible(true); diff --git a/design/qwp-nack-policy-v2.md b/design/qwp-nack-policy-v2.md index fabe4507..85ea7c30 100644 --- a/design/qwp-nack-policy-v2.md +++ b/design/qwp-nack-policy-v2.md @@ -66,8 +66,9 @@ is caught *behaviorally*: > in durable-ack mode the trim watermark advances only on durable coverage, > so every post-NACK recycle replays from the durable watermark and re-OKs > frames *behind* the suspect — those re-OKs say nothing about the poisoned -> bytes and must not launder the count. Orderly closes (`NORMAL_CLOSURE` -> role-change handoff, `GOING_AWAY` restart drain) never count strikes. +> bytes and must not launder the count. Orderly closes (`ROLE_CHANGE` (4001) +> role-change handoff, `NORMAL_CLOSURE`, `GOING_AWAY` restart drain) never +> count strikes. Below the escalation threshold, a RETRIABLE NACK's recycle is **paced**: the server is reachable (it just answered), so the reconnect succeeds immediately @@ -99,8 +100,11 @@ diagnostics only. The server already handles it at the right layer (Invariant B work): the read-only gate and the commit-path authorization refusal both set -`roleChangeClosePending` and close with a reconnect-eligible `NORMAL_CLOSURE` -instead of NACKing `SECURITY_ERROR` (`QwpIngressProcessorState`). The client +`roleChangeClosePending` and close with a reconnect-eligible `ROLE_CHANGE` +(4001, private-use range; distinct from `NORMAL_CLOSURE` so the client's +verbatim CLOSE echo is distinguishable from a voluntary client CLOSE that +crossed the server's CLOSE on the wire) instead of NACKing `SECURITY_ERROR` +(`QwpIngressProcessorState`). The client reconnects, hits the 421 role reject on the now-replica, and retries from SF until a primary is reachable. Consequently: From a375a78557e428767ed5e524a2c63aebe72a4701 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 21:51:31 +0100 Subject: [PATCH 05/12] Fix SIGBUS risk when appending into recovered sparse SF segments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recovery mmap'd the segment file's complete logical length RW and left appendCursor at lastGoodOffset, exposing the unvalidated tail as writable capacity. If that tail was sparse/unbacked (fallocate reservations do not survive external sparsification, and FilesFacade.allocate cannot retroactively fill holes below EOF), the next producer append stored straight into the hole — which under ENOSPC raises SIGBUS and aborts the JVM. MmapSegment.openExisting now maps only the validated prefix [0, lastGoodOffset), making recovered segments born full: the first append backpressures until the segment manager installs a hot spare (created with genuine block reservation and clean ENOSPC failure), then rotates onto it. Consumers are unaffected — they only read below publishedOffset(), which equals lastGoodOffset. Because a recovered segment's mapped size no longer reflects its on-disk footprint, sf_max_total_bytes accounting (register seed and trim decrement) now uses the new MmapSegment.fileBytes() — the file's logical length — instead of sizeBytes(). Regression tests: recovered sparse-tail segment must be born full and refuse tryAppend; a ring recovered around a sparse-tail active must backpressure the first append and resume with a contiguous FSN after spare rotation. --- .../qwp/client/sf/cursor/MmapSegment.java | 69 ++++++++++++---- .../qwp/client/sf/cursor/SegmentManager.java | 6 +- .../qwp/client/sf/cursor/SegmentRing.java | 33 +++++--- .../cursor/MmapSegmentRecoveryFaultTest.java | 79 +++++++++++++++++++ .../qwp/client/sf/cursor/SegmentRingTest.java | 14 +++- 5 files changed, 172 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java index 1f0a3707..fd4d55d4 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/MmapSegment.java @@ -70,6 +70,11 @@ public final class MmapSegment implements QuietCloseable { private final FilesFacade filesFacade; private final String path; private final long sizeBytes; + // fileBytes: logical size of the backing file — the segment's on-disk + // footprint for sf_max_total_bytes accounting. Equals sizeBytes for + // created and memory-backed segments; for recovered segments it can + // exceed sizeBytes because recovery maps only the validated prefix. + private final long fileBytes; // memoryBacked: true when the segment buffer lives in malloc'd native // memory rather than an mmap'd file. The "non-SF async" path uses // memory-backed segments — same cursor architecture, no disk involvement. @@ -104,13 +109,14 @@ public final class MmapSegment implements QuietCloseable { private final long tornTailBytes; private MmapSegment(String path, int fd, long mmapAddress, long sizeBytes, - long baseSeq, long initialCursor, long frameCount, + long fileBytes, long baseSeq, long initialCursor, long frameCount, boolean memoryBacked, long tornTailBytes, FilesFacade filesFacade) { this.path = path; this.filesFacade = filesFacade; this.fd = fd; this.mmapAddress = mmapAddress; this.sizeBytes = sizeBytes; + this.fileBytes = fileBytes; this.baseSeq = baseSeq; this.appendCursor = initialCursor; this.publishedCursor = initialCursor; @@ -200,7 +206,7 @@ public static MmapSegment create(FilesFacade ff, long pathPtr, String displayPat Unsafe.getUnsafe().putLong(addr + 8, baseSeq); Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros()); return new MmapSegment( - displayPath, fd, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L, ff); + displayPath, fd, addr, sizeBytes, sizeBytes, baseSeq, HEADER_SIZE, 0, false, 0L, ff); } catch (Throwable t) { if (addr != Files.FAILED_MMAP_ADDRESS) { Files.munmap(addr, sizeBytes, MemoryTag.MMAP_DEFAULT); @@ -238,7 +244,7 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) { Unsafe.getUnsafe().putLong(addr + 8, baseSeq); Unsafe.getUnsafe().putLong(addr + 16, Os.currentTimeMicros()); return new MmapSegment( - null, -1, addr, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L, null); + null, -1, addr, sizeBytes, sizeBytes, baseSeq, HEADER_SIZE, 0, true, 0L, null); } catch (Throwable t) { Unsafe.free(addr, sizeBytes, MemoryTag.NATIVE_DEFAULT); throw t; @@ -246,13 +252,22 @@ public static MmapSegment createInMemory(long baseSeq, long sizeBytes) { } /** - * Opens an existing segment file for recovery. mmaps it RW, validates the - * header magic / version, then scans frames forward verifying each CRC. - * The first bad CRC (or a frame whose declared length runs past the file - * end) is treated as a torn tail; both cursors are positioned at the - * start of that frame. Returns the segment ready for further appends. + * Opens an existing segment file for recovery. Validates the header magic + * / version, then scans frames forward verifying each CRC. The first bad + * CRC (or a frame whose declared length runs past the file end) is treated + * as a torn tail; both cursors are positioned at the start of that frame. * Throws {@link MmapSegmentException} on header validation failure. *

+ * The returned segment maps ONLY the validated prefix + * {@code [0, lastGoodOffset)} and is therefore born full: + * {@link #tryAppend} returns -1 and the caller rotates onto a freshly + * created segment before the next append. The tail past the last good + * frame is never mapped — it may be sparse/unbacked ({@code create()}'s + * block reservation does not survive external sparsification, and + * {@link FilesFacade#allocate} cannot retroactively fill holes below EOF), + * and a store into an unbacked page under ENOSPC raises SIGBUS, aborting + * the JVM. + *

* If recovery observes a torn tail (the bytes at the bail-out position * are non-zero, indicating an attempted-but-failed frame write rather * than clean unwritten space), a {@code WARN} is emitted with the byte @@ -303,11 +318,23 @@ public static MmapSegment openExisting(FilesFacade ff, String path) { "short read before stable file EOF during recovery: " + path + " size=" + fileSize); } - mapSize = Math.min(fileSize, finalFileSize); - if (mapSize < HEADER_SIZE) { + long logicalSize = Math.min(fileSize, finalFileSize); + if (logicalSize < HEADER_SIZE) { throw new MmapSegmentException("file shorter than header after recovery scan: " - + path + " size=" + mapSize); + + path + " size=" + logicalSize); } + // Map ONLY the validated prefix [0, lastGoodOffset). The tail past + // the last good frame may be sparse/unbacked, and a store into an + // unbacked page under ENOSPC raises SIGBUS and aborts the JVM. + // FilesFacade.allocate cannot make that tail safe at recovery time + // (it reserves [currentSize, target) only — pre-existing holes + // below EOF are not retroactively filled). Mapping through + // lastGoodOffset makes the recovered segment born full + // (capacityRemaining() == 0), so the producer rotates onto a + // freshly created — and genuinely block-reserved — segment before + // the next append. Consumers are unaffected: they only read below + // publishedOffset(), which equals lastGoodOffset. + mapSize = recovered.lastGoodOffset; addr = Files.mmap(fd, mapSize, 0, Files.MAP_RW, MemoryTag.MMAP_DEFAULT); if (addr == Files.FAILED_MMAP_ADDRESS) { throw new MmapSegmentException("mmap failed for " + path); @@ -317,23 +344,24 @@ public static MmapSegment openExisting(FilesFacade ff, String path) { fd, addr, mapSize, + logicalSize, recovered.baseSeq, recovered.lastGoodOffset, recovered.frameCount, false, - Math.min(recovered.tornTailBytes, mapSize - recovered.lastGoodOffset), + Math.min(recovered.tornTailBytes, logicalSize - recovered.lastGoodOffset), ff ); if (segment.tornTailBytes() > 0) { LOG.warn("SF segment {}: torn tail of {} bytes at offset {} " + "(file size {}, frames recovered {}). " - + "Recovery will overwrite this region on next append; " + + "The torn region is excluded from the recovered mapping and " + "frames past the tear (if any) are discarded. " + "Investigate disk health or unexpected writer crash.", path, segment.tornTailBytes(), segment.publishedOffset(), - mapSize, + logicalSize, segment.frameCount()); } // Ownership of fd and addr transfers to the returned segment. @@ -443,6 +471,19 @@ public long sizeBytes() { return sizeBytes; } + /** + * Logical size of the backing file in bytes — the segment's on-disk + * footprint for {@code sf_max_total_bytes} cap accounting. Equals + * {@link #sizeBytes()} for created and memory-backed segments; for + * recovered segments it can exceed {@link #sizeBytes()} because recovery + * maps only the validated prefix (SIGBUS hardening in + * {@link #openExisting}) while the file keeps its full logical length + * until trim unlinks it. + */ + public long fileBytes() { + return fileBytes; + } + /** * Appends one frame: writes {@code [crc32c | u32 payloadLen | payload]} * starting at the current append cursor, then advances both cursors diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java index 7a63096c..eaf5d407 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java @@ -744,7 +744,11 @@ private void serviceRing0(RingEntry e) { trim = e.ring.drainTrimmable(); if (registered && trim != null) { for (int i = 0, n = trim.size(); i < n; i++) { - totalBytes -= trim.get(i).sizeBytes(); + // fileBytes(), not sizeBytes(): must mirror the register-time + // seed (SegmentRing.totalSegmentBytes), which accounts the + // on-disk footprint — recovered segments map only their + // validated prefix but occupy the full file length on disk. + totalBytes -= trim.get(i).fileBytes(); } } } diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index 71363c4f..0be1994c 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -139,8 +139,11 @@ public SegmentRing(MmapSegment initialActive, long maxBytesPerSegment) { * directory, opens each via {@link MmapSegment#openExisting}, and * arranges them by baseSeq: *

    - *
  • Highest-baseSeq segment becomes the active (further appends land - * there until it fills, at which point normal rotation kicks in).
    • + *
  • Highest-baseSeq segment becomes the active. Recovered segments + * are mapped only through their validated prefix (SIGBUS hardening + * in {@link MmapSegment#openExisting}) and are therefore born full: + * the first append backpressures until the manager installs a hot + * spare, then rotates onto it.
    • *
  • All others become sealed segments awaiting ACK and trim.
    • *
* Returns {@code null} if the directory is empty or contains no @@ -291,10 +294,12 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { + " check disk health"); } } - // The newest segment becomes the active. Even if it's full, that's OK: - // the next appendOrFsn returns BACKPRESSURE_NO_SPARE, the manager - // installs a hot spare, the producer rotates. Same fast path as a - // mid-life ring. + // The newest segment becomes the active. Recovered segments are + // born full (mapped only through the validated prefix -- see the + // SIGBUS hardening in MmapSegment.openExisting), so the next + // appendOrFsn returns BACKPRESSURE_NO_SPARE, the manager installs + // a hot spare, the producer rotates. Same fast path as a mid-life + // ring whose active just filled. int last = opened.size() - 1; MmapSegment active = opened.get(last); opened.remove(last); @@ -682,21 +687,25 @@ public synchronized int snapshotSealedSegments(MmapSegment[] target) { } /** - * Total mmap'd bytes the ring currently owns: active + hot spare (if + * Total on-disk bytes the ring currently owns: active + hot spare (if * installed) + every sealed segment. Used by {@code SegmentManager} * to seed its {@code totalBytes} accounting at register time and to - * reverse the contribution at deregister time. Synchronized against - * rotation so we never read a half-resized sealed list. + * reverse the contribution at deregister time. Uses + * {@link MmapSegment#fileBytes()} — the file's logical length — rather + * than the mapped size, because recovered segments map only their + * validated prefix while the file keeps its full length until trim + * unlinks it. Synchronized against rotation so we never read a + * half-resized sealed list. */ public synchronized long totalSegmentBytes() { long total = 0L; MmapSegment a = active; - if (a != null) total += a.sizeBytes(); + if (a != null) total += a.fileBytes(); MmapSegment hs = hotSpare; - if (hs != null) total += hs.sizeBytes(); + if (hs != null) total += hs.fileBytes(); for (int i = 0, n = sealedSegments.size(); i < n; i++) { MmapSegment s = sealedSegments.get(i); - if (s != null) total += s.sizeBytes(); + if (s != null) total += s.fileBytes(); } return total; } diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java index 58eb4cf2..32f9053b 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java @@ -26,6 +26,7 @@ import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing; import io.questdb.client.std.Files; import io.questdb.client.std.FilesFacade; import io.questdb.client.std.MemoryTag; @@ -91,6 +92,84 @@ public void testRecoveryKeepsFramesBeforeUnbackedTail() throws Exception { }); } + /** + * Regression for the recovered-sparse-tail SIGBUS: recovery used to mmap + * the file's complete logical size RW and leave the sparse tail as + * writable capacity, so the next append stored straight into the unbacked + * hole — which under ENOSPC raises SIGBUS and aborts the JVM. + * {@link FilesFacade#allocate} cannot retroactively fill holes below EOF, + * so no recovery-time reservation can make that tail safe; instead the + * fix maps only the validated prefix. The recovered segment must be born + * full and refuse appends, forcing rotation onto a freshly created, + * genuinely block-reserved segment. + */ + @Test + public void testRecoveredSparseSegmentIsBornFullAndRefusesAppends() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/seg-sparse-no-append.sfa"; + final long page = Files.PAGE_SIZE; + final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE); + long boundary = writeSegment(path, 5L, new int[]{payloadLen}); + assertEquals("frame must fill exactly one page", page, boundary); + punchSparseTail(path, page); + + long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT); + try (MmapSegment seg = MmapSegment.openExisting(path)) { + assertEquals("recovered data must remain readable", 1L, seg.frameCount()); + assertEquals("scan must stop at the unbacked-page boundary", page, seg.publishedOffset()); + assertEquals("mapping must cover only the validated prefix", page, seg.sizeBytes()); + assertTrue("recovered segment must be born full", seg.isFull()); + assertEquals("no payload capacity may remain over the sparse tail", + 0L, seg.capacityRemaining()); + assertEquals("append into the unbacked tail must be refused", + -1L, seg.tryAppend(buf, 16)); + } finally { + Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT); + } + }); + } + + /** + * Recovery-to-producer path for the same regression: a ring recovered + * around a sparse-tail active must backpressure the first append (never + * touching the hole) and resume cleanly — with a contiguous FSN sequence + * — once a genuinely allocated hot spare is installed and rotated in. + */ + @Test + public void testRecoveredRingRotatesBeforeAppendingPastSparseTail() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final String path = tmpDir + "/sf-0.sfa"; + final long page = Files.PAGE_SIZE; + final int payloadLen = (int) (page - MmapSegment.HEADER_SIZE - MmapSegment.FRAME_HEADER_SIZE); + long boundary = writeSegment(path, 0L, new int[]{payloadLen}); + assertEquals("frame must fill exactly one page", page, boundary); + punchSparseTail(path, page); + + long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT); + try { + try (SegmentRing recovered = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES)) { + assertTrue("ring must recover from the on-disk segment", recovered != null); + assertEquals(1L, recovered.getActive().frameCount()); + assertEquals(1L, recovered.nextSeqHint()); + // First append must NOT write into the sparse tail — the + // born-full active backpressures until a spare arrives. + assertEquals(SegmentRing.BACKPRESSURE_NO_SPARE, + recovered.appendOrFsn(buf, 16)); + // create() pre-allocates real blocks, so the rotated-in + // spare is safe to store into. + recovered.installHotSpare( + MmapSegment.create(tmpDir + "/sf-1.sfa", 1L, SEGMENT_BYTES)); + assertEquals("append must resume with a contiguous FSN", + 1L, recovered.appendOrFsn(buf, 16)); + assertEquals("append must land in the rotated-in spare", + 1L, recovered.getActive().baseSeq()); + } + } finally { + Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT); + } + }); + } + /** * The harder case: a frame whose 8-byte header sits on a backed page but * whose payload reaches into the unbacked hole (a torn write leaves a real diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java index f1a0fcde..5c30386d 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java @@ -294,7 +294,8 @@ public void testOpenExistingRecoversActivePlusSealed() throws Exception { long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT); try { // Write three segments with FSN ranges 0..3, 4..7, 8..9 (last - // partially full so the recovered ring has appendable room). + // partially full; recovery still maps it born-full — see the + // SIGBUS hardening in MmapSegment.openExisting). MmapSegment s0 = MmapSegment.create(tmpDir + "/r0.sfa", 0, segSize); for (int i = 0; i < 4; i++) s0.tryAppend(buf, 16); s0.close(); @@ -319,8 +320,17 @@ public void testOpenExistingRecoversActivePlusSealed() throws Exception { assertEquals(4, recovered.getSealedSegments().get(1).baseSeq()); // nextSeq must continue past the recovered frames. assertEquals(10, recovered.nextSeqHint()); - // Further appends land into the active and assign FSN 10. + // Recovered segments are mapped only through their validated + // prefix and are born full: the first append backpressures + // (never writing into the potentially unbacked tail) until a + // spare is installed, then rotates and assigns FSN 10. + assertEquals(SegmentRing.BACKPRESSURE_NO_SPARE, + recovered.appendOrFsn(buf, 16)); + recovered.installHotSpare( + MmapSegment.create(tmpDir + "/r3.sfa", 10, segSize)); assertEquals(10, recovered.appendOrFsn(buf, 16)); + assertEquals("append must land in the rotated-in spare", + 10, recovered.getActive().baseSeq()); } } finally { Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT); From c1ad81a93437f484b08f9416fe6785bb33aaab99 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Fri, 10 Jul 2026 23:43:57 +0100 Subject: [PATCH 06/12] Fail closed on unreadable SF segments and enumeration errors in recovery SegmentRing.openExisting() previously skipped every per-file MmapSegmentException and treated directory-enumeration failure (findFirst < 0) as an empty store. The interior FSN-contiguity check cannot expose a skipped segment at the lowest, highest, or sole position -- the survivors stay mutually contiguous -- so recovery silently retired the skipped segment's unacked frames (lowest/sole: the engine seeds ackedFsn past them), minted duplicate FSNs (highest: nextSeq resumes inside the skipped range), or -- for a sole unreadable segment -- returned null, routing CursorSendEngine onto the fresh-start path whose truncating openCleanRW (O_CREAT|O_TRUNC / CREATE_ALWAYS) destroyed the only surviving sf-initial.sfa. Recovery now fails closed: - an unreadable .sfa aborts recovery with MmapSegmentException naming the file and remediation options (restore, or move the file out to explicitly accept the loss); - enumeration failure throws instead of returning null; - quarantine-rename and empty-leftover-unlink failures throw instead of warn-and-continue; - CursorSendEngine's fresh-start path refuses to create sf-initial.sfa over an existing file (defense in depth), checked before the stale-watermark unlink to preserve forensic state. openExisting() returning null now strictly means "no recoverable data existed". BackgroundDrainer already quarantines a slot (markFailed + FAILED) when engine construction throws, so corrupt orphan slots are preserved for postmortem rather than drained over. New SegmentRingEdgeRecoveryTest pins the contract with sole / lowest / highest unreadable-segment tests, an engine-level no-truncation test, and a POSIX enumeration-failure test; all five reproduced the data loss before the fix. --- .../client/sf/cursor/CursorSendEngine.java | 33 +- .../qwp/client/sf/cursor/SegmentRing.java | 107 +++-- .../cursor/MmapSegmentRecoveryFaultTest.java | 23 +- .../cursor/SegmentRingEdgeRecoveryTest.java | 390 ++++++++++++++++++ .../qwp/client/sf/cursor/SegmentRingTest.java | 25 +- 5 files changed, 524 insertions(+), 54 deletions(-) create mode 100644 core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingEdgeRecoveryTest.java diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java index 9ebac3a1..a596e3ff 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java @@ -316,21 +316,36 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man recoveredCommitBoundaryFsn, publishedFsn); } } else { - // Fresh start with no recovered segments. Any stale - // watermark from a prior fully-drained session refers - // to a lifecycle now gone -- unlink it before opening - // so the new session's first read() correctly reports - // INVALID (magic=0 on a freshly zero-filled file). - if (!memoryMode) { - AckWatermark.removeOrphan(sfDir); - watermarkInProgress = AckWatermark.open(sfDir); - } + // Fresh start with no recovered segments. MmapSegment initial; String initialPath = null; if (memoryMode) { initial = MmapSegment.createInMemory(0L, segmentSizeBytes); } else { initialPath = sfDir + "/sf-initial.sfa"; + // Defense in depth: recovery returning null now guarantees + // no recognizable segment data remained (unreadable files + // and enumeration failures fail closed; empty leftovers + // were unlinked; corrupt-tailed ones quarantined). If a + // file still exists at this path, some invariant broke -- + // and MmapSegment.create's openCleanRW uses truncating + // semantics (O_CREAT|O_TRUNC / CREATE_ALWAYS), so creating + // over it would silently destroy whatever it holds. + // Checked BEFORE the watermark unlink below so a guard + // trip preserves maximal forensic state for postmortem. + if (Files.exists(initialPath)) { + throw new MmapSegmentException( + "refusing to overwrite existing " + initialPath + + " on fresh start -- recovery reported no data, yet the" + + " file exists; creating over it would truncate its" + + " contents"); + } + // Any stale watermark from a prior fully-drained session + // refers to a lifecycle now gone -- unlink it before + // opening so the new session's first read() correctly + // reports INVALID (magic=0 on a freshly zero-filled file). + AckWatermark.removeOrphan(sfDir); + watermarkInProgress = AckWatermark.open(sfDir); initial = MmapSegment.create(initialPath, 0L, segmentSizeBytes); } try { diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index 0be1994c..b3ba57f5 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -146,16 +146,24 @@ public SegmentRing(MmapSegment initialActive, long maxBytesPerSegment) { * spare, then rotates onto it. *
  • All others become sealed segments awaiting ACK and trim.
    • * - * Returns {@code null} if the directory is empty or contains no - * recognizable {@code .sfa} files -- the caller should then construct a - * fresh ring with {@link #SegmentRing(MmapSegment, long)} and a freshly - * created initial segment. + * Returns {@code null} only when there is genuinely no data to recover: + * the directory does not exist, is empty, or contains only empty + * hot-spare leftovers (which are unlinked). The caller may then safely + * construct a fresh ring with {@link #SegmentRing(MmapSegment, long)} and + * a freshly created initial segment. *

    - * Recovery is best-effort: a single bad-magic file is silently skipped - * (logged-then-ignored is the right call here; a stray unrelated file in - * the SF dir shouldn't take the whole sender down). A failure to open - * an otherwise-valid segment IS fatal -- the caller's data integrity - * depends on every segment being readable. + * Recovery fails closed: any {@code .sfa} file that cannot be opened -- + * bad magic, bad header, unsupported version, short file, mmap rejection + * -- aborts recovery with {@link MmapSegmentException}, as does a failure + * to enumerate the directory. Skipping an unreadable file is never safe + * here: the interior FSN-contiguity check below cannot expose a skipped + * segment at the lowest, highest, or sole position (the survivors stay + * mutually contiguous), so recovery would silently retire the skipped + * segment's unacked frames, mint duplicate FSNs past a skipped tail, or + * -- for a sole skipped file -- route the caller onto the truncating + * fresh-start path that destroys the only surviving segment. An operator + * who wants to accept the loss moves the file out of the SF directory + * explicitly. */ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { if (!Files.exists(sfDir)) { @@ -164,9 +172,16 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { ObjList opened = new ObjList<>(); long find = Files.findFirst(sfDir); if (find < 0) { - LOG.warn("openExisting could not enumerate {} - treating as empty, " - + "but this may indicate a permission or transient error", sfDir); - return null; + // Fail closed: an enumeration failure (permissions, transient + // I/O error) is NOT an empty store. Returning null here would + // route the caller onto the fresh-start path, whose truncating + // openCleanRW of sf-initial.sfa destroys any surviving data the + // JVM merely could not list. + throw new MmapSegmentException( + "could not enumerate SF directory " + sfDir + + " -- refusing to treat an unreadable directory as an empty" + + " store (a fresh start would truncate sf-initial.sfa over" + + " any surviving data); check directory permissions"); } if (find == 0) { return null; @@ -185,7 +200,30 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { String path = sfDir + "/" + name; MmapSegment seg = null; try { - seg = MmapSegment.openExisting(path); + try { + seg = MmapSegment.openExisting(path); + } catch (MmapSegmentException t) { + // Fail closed: an unreadable .sfa in the slot is a + // recovery failure, not a skippable curiosity. The + // interior contiguity check below cannot expose a + // skipped segment at the lowest, highest, or sole + // position -- the survivors stay mutually + // contiguous -- so skipping would silently retire + // the segment's unacked frames (lowest/sole: the + // engine seeds ackedFsn past them), mint duplicate + // FSNs (highest: nextSeq resumes inside the skipped + // range), or -- sole -- route the caller onto the + // truncating fresh-start path that destroys the + // only surviving file. The outer catch closes every + // already-recovered segment before this propagates. + throw new MmapSegmentException( + "recovery failed: unreadable segment file " + path + + " -- refusing to start with partial data (a skipped" + + " segment silently loses or duplicates frames). Restore" + + " the file from a copy, or move it out of the SF" + + " directory to explicitly accept the data loss", + t); + } // Filter out empty leftovers -- typically hot-spare // segments the manager pre-allocated for a prior // session that never got rotated into active. They @@ -213,30 +251,39 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { if (torn > 0) { int renameResult = Files.rename(path, path + ".corrupt"); if (renameResult != 0) { - LOG.warn("openExisting: could not quarantine corrupt segment {}; " - + "the original file remains in place [rc={}]", - path, renameResult); + // Fail closed: with the quarantine rename + // failed, the corrupt file is still at its + // original path -- if it is the sole + // segment, the caller's fresh start would + // truncate it (sf-initial.sfa) or restart + // FSNs alongside it. Surface the error + // instead of proceeding over live data. + throw new MmapSegmentException( + "could not quarantine corrupt segment " + path + + " to " + path + ".corrupt [rc=" + renameResult + + "] -- refusing to continue recovery while the" + + " corrupt file occupies its original path"); } } else { - Files.remove(path); + if (!Files.remove(path)) { + // Same fail-closed reasoning: an empty + // leftover that cannot be unlinked would + // be truncated by the caller's fresh + // start (harmless for an empty file) -- + // but unlink failing in a directory we + // just created files in means something + // is wrong (permissions changed, dir + // vanished); don't build on that ground. + throw new MmapSegmentException( + "could not unlink empty leftover segment " + path + + " -- refusing to continue recovery; check" + + " directory permissions"); + } } } else { opened.add(seg); seg = null; } - } catch (MmapSegmentException t) { - // Per-file data error (bad magic, bad header, - // unsupported version, mmap rejection on this one - // file). Don't take down recovery for one corrupt - // .sfa -- log and skip so siblings still recover. - // Resource exhaustion (OOM) and programmer errors - // (IOOBE) deliberately propagate to the outer - // catch, which closes every already-recovered - // segment and rethrows: continuing the loop after - // an OOM would just fail again on the next file - // while silently leaking the segments we managed - // to recover before it. - LOG.warn("openExisting: skipping {} -- {}", path, t.toString()); } finally { // Close any seg whose ownership wasn't transferred // (either to opened, or via the empty-branch close diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java index 32f9053b..c0665ec0 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/MmapSegmentRecoveryFaultTest.java @@ -208,12 +208,15 @@ public void testRecoverySurvivesPayloadReachingUnbackedPage() throws Exception { } /** - * An unbacked page-zero header must produce the per-file - * {@link MmapSegmentException} that SegmentRing skips, not poison recovery - * of valid sibling files. + * An unbacked page-zero header must produce a synchronous + * {@link MmapSegmentException}, not a SIGBUS/undefined mapping. + * SegmentRing.openExisting fails recovery closed on this exception -- + * a segment with an unreadable header may hold recoverable frames, and + * silently skipping it at the lowest/highest/sole position loses or + * duplicates FSNs (see SegmentRingEdgeRecoveryTest). */ @Test - public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { + public void testUnbackedHeaderPageFailsSynchronously() throws Exception { TestUtils.assertMemoryLeak(() -> { final String path = tmpDir + "/seg-unbacked-header.sfa"; writeSegment(path, 3L, new int[]{64}); @@ -223,8 +226,9 @@ public void testUnbackedHeaderPageIsSkippableNotFatal() throws Exception { MmapSegment.openExisting(path).close(); fail("expected MmapSegmentException for an unbacked header page"); } catch (MmapSegmentException expected) { - // ok -- SegmentRing's per-file catch skips just this file - // instead of aborting recovery of the whole slot. + // ok -- surfaces synchronously; SegmentRing.openExisting + // fails recovery closed on it rather than silently skipping + // a file that may hold recoverable frames. } }); } @@ -331,11 +335,12 @@ public void testSuccessfulRecoveryClosesThroughFacadeExactlyOnce() throws Except } /** - * A header short-read produces the per-file exception that SegmentRing - * skips; recovery never maps the stale reported length. + * A header short-read produces a synchronous per-file exception on which + * SegmentRing.openExisting fails recovery closed; recovery never maps + * the stale reported length. */ @Test - public void testHeaderShortReadIsSkippable() throws Exception { + public void testHeaderShortReadFailsSynchronously() throws Exception { TestUtils.assertMemoryLeak(() -> { final String path = tmpDir + "/seg-mappasteof-header.sfa"; final long page = Files.PAGE_SIZE; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingEdgeRecoveryTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingEdgeRecoveryTest.java new file mode 100644 index 00000000..f9f06eb5 --- /dev/null +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingEdgeRecoveryTest.java @@ -0,0 +1,390 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2026 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.client.test.cutlass.qwp.client.sf.cursor; + +import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegmentException; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing; +import io.questdb.client.std.Files; +import io.questdb.client.std.MemoryTag; +import io.questdb.client.std.Os; +import io.questdb.client.std.Unsafe; +import io.questdb.client.test.tools.TestUtils; +import org.junit.After; +import org.junit.Assert; +import org.junit.Assume; +import org.junit.Before; +import org.junit.Test; + +import java.nio.file.Paths; +import java.nio.file.attribute.PosixFilePermissions; + +/** + * Regression pins for the fail-closed edge-position recovery contract. + * Historically {@link SegmentRing#openExisting} silently skipped every + * {@link MmapSegmentException} and treated directory-enumeration failure as + * an empty store. The interior FSN-contiguity check only exposes a skipped + * segment when it sits BETWEEN two survivors — a sole, lowest, or highest + * unreadable segment escaped it: + *

      + *
    • Sole — recovery returns {@code null}; {@link CursorSendEngine} + * then creates a fresh {@code sf-initial.sfa} via truncating + * {@code openCleanRW} ({@code O_CREAT|O_TRUNC} / {@code CREATE_ALWAYS}), + * destroying the only surviving SF file.
      • + *
    • Lowest — survivors are still mutually contiguous; the engine + * seeds {@code ackedFsn = newLowestBase - 1}, permanently classifying + * the lost segment's unacked frames as acked. Silent data loss.
      • + *
    • Highest — the second-highest segment becomes active and + * {@code nextSeqHint()} continues from it, so newly appended frames + * duplicate FSNs still durable in the skipped file.
      • + *
    • Enumeration failure — {@code findFirst < 0} returns + * {@code null} exactly like an empty directory, so a single transient + * readdir failure sends the engine down the truncating fresh-start + * path against fully VALID data.
      • + *
    + * Recovery now distinguishes "no segment files" from "segment files existed + * but could not be recovered" and fails closed on unreadable {@code .sfa} + * files and enumeration failures; these tests pin that contract. + *

    + * Corruption method: the tests clobber the header VERSION byte (offset 4) + * while leaving the magic intact. This is unambiguously a recognizable SF + * segment that fails recovery — not the stray-foreign-file case the per-file + * skip was designed for. + */ +public class SegmentRingEdgeRecoveryTest { + + private static final int FRAME_PAYLOAD_LEN = 32; + private static final long SEGMENT_BYTES = 64 * 1024; + private String tmpDir; + + @Before + public void setUp() { + tmpDir = TestUtils.createTmpDir("qdb-ring-edge-red-"); + } + + @After + public void tearDown() { + if (tmpDir == null) { + return; + } + // The enumeration-failure test drops read permission on the dir; + // restore it so removeTmpDir can enumerate. No-op on Windows. + try { + java.nio.file.Files.setPosixFilePermissions( + Paths.get(tmpDir), PosixFilePermissions.fromString("rwx------")); + } catch (Exception ignored) { + } + TestUtils.removeTmpDir(tmpDir); + } + + /** + * Enumeration failure: {@code Files.findFirst} fails (here: EACCES via a + * write+exec-only directory), and openExisting returns {@code null} — + * indistinguishable from an empty store. The engine's fresh-start path + * would then truncate a fully VALID {@code sf-initial.sfa} and delete the + * prior ack watermark. A transient readdir failure must be an error, not + * "no data". + */ + @Test + public void testEnumerationFailureMustNotBeTreatedAsEmptyStore() throws Exception { + Assume.assumeTrue("POSIX-only: relies on chmod to make readdir fail", + Os.type != Os.WINDOWS); + Assume.assumeFalse("root bypasses permission checks", + "root".equals(System.getProperty("user.name"))); + TestUtils.assertMemoryLeak(() -> { + // A fully VALID segment with real frames -- nothing about this + // file is corrupt. + writeFrames(tmpDir + "/sf-initial.sfa", 0L, 3); + // Write+exec, no read: opendir/readdir fails with EACCES while + // file creation and open-by-path inside the dir still work -- + // exactly the state in which the engine's fresh-start path + // would truncate the valid segment. + java.nio.file.Files.setPosixFilePermissions( + Paths.get(tmpDir), PosixFilePermissions.fromString("-wx------")); + try { + SegmentRing ring; + try { + ring = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES); + } catch (MmapSegmentException expected) { + // Fixed behavior: enumeration failure surfaces as an + // error the caller must handle -- never as "empty". + Assert.assertTrue(expected.getMessage(), + expected.getMessage().contains("could not enumerate")); + return; + } + if (ring != null) { + ring.close(); + Assert.fail("openExisting returned a ring despite readdir failing -- " + + "unexpected; revisit this test's permission setup"); + } + Assert.fail("FINDING: openExisting treated a directory-enumeration failure " + + "as an empty store (returned null). CursorSendEngine's fresh-start " + + "path would now truncate the fully VALID sf-initial.sfa via " + + "openCleanRW and delete the ack watermark -- destroying readable " + + "data after one transient readdir failure."); + } finally { + java.nio.file.Files.setPosixFilePermissions( + Paths.get(tmpDir), PosixFilePermissions.fromString("rwx------")); + } + }); + } + + /** + * Engine level, the destructive case: the slot's only segment is + * {@code sf-initial.sfa} (a low-volume sender that never rotated) whose + * header was torn by a crash. Recovery skips it, returns {@code null}, + * and the engine's fresh-start path re-creates {@code sf-initial.sfa} + * with truncating {@code openCleanRW} -- zeroing the only surviving SF + * file. The frames that physically followed the torn header are + * destroyed with no quarantine and no error. + */ + @Test + public void testFreshStartMustNotTruncateSoleUnreadableInitialSegment() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String initialPath = tmpDir + "/sf-initial.sfa"; + writeFrames(initialPath, 0L, 3); + clobberVersionByte(initialPath); + + // First payload byte of frame[0]; writeFrames fills payloads with + // non-zero bytes, so a zero here means the file was re-created. + long probeOffset = MmapSegment.HEADER_SIZE + MmapSegment.FRAME_HEADER_SIZE; + Assert.assertTrue("setup: probe byte must be non-zero", + readByteAt(initialPath, probeOffset) != 0); + + CursorSendEngine engine = null; + boolean failedClosed = false; + try { + engine = new CursorSendEngine(tmpDir, SEGMENT_BYTES); + } catch (Throwable expected) { + // Fixed behavior: recognizable-but-unreadable sole segment + // must fail engine construction (or be quarantined below). + failedClosed = true; + } + try { + if (!failedClosed) { + // Probe BEFORE close(): a fully-drained engine unlinks + // residual .sfa files on close, which would mask the + // truncation as a deletion. + boolean quarantined = Files.exists(initialPath + ".corrupt"); + boolean preserved = Files.exists(initialPath) + && readByteAt(initialPath, probeOffset) != 0; + Assert.assertTrue( + "FINDING: CursorSendEngine silently TRUNCATED the sole surviving " + + "sf-initial.sfa. Recovery skipped it (torn header -> " + + "MmapSegmentException -> logged-and-ignored), returned null, " + + "and the fresh-start path re-created the same path via " + + "openCleanRW (O_CREAT|O_TRUNC). Every frame that survived " + + "the crash is destroyed; postmortem recovery is impossible. " + + "Expected: fail construction, or quarantine to " + + "sf-initial.sfa.corrupt before creating a fresh file.", + quarantined || preserved); + } + } finally { + if (engine != null) { + engine.close(); + } + } + }); + } + + /** + * Highest segment unreadable: the second-highest survivor becomes the + * active segment and {@code nextSeqHint()} resumes inside the FSN range + * still durable in the skipped file -- future appends mint duplicate + * FSNs, and the skipped file poisons the next recovery's contiguity + * check. Must fail closed instead. + */ + @Test + public void testHighestUnreadableSegmentMustFailRecoveryClosed() throws Exception { + TestUtils.assertMemoryLeak(() -> { + writeFrames(tmpDir + "/sf-0000000000000000.sfa", 0L, 2); // FSN 0..1 + String highPath = tmpDir + "/sf-0000000000000001.sfa"; + writeFrames(highPath, 2L, 2); // FSN 2..3 + clobberVersionByte(highPath); + + SegmentRing ring; + try { + ring = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES); + } catch (MmapSegmentException expected) { + // Fixed behavior: fail closed on the unreadable file itself + // (not e.g. an incidental FSN-gap throw). + Assert.assertTrue(expected.getMessage(), + expected.getMessage().contains("unreadable segment file")); + return; + } + try { + long nextFsn = ring != null ? ring.nextSeqHint() : -1L; + Assert.fail("FINDING: openExisting silently skipped the HIGHEST segment " + + "(unsupported version -> MmapSegmentException -> logged-and-ignored). " + + "The survivors are mutually contiguous so the gap check cannot fire. " + + "The ring now resumes at FSN " + nextFsn + ", duplicating FSNs 2..3 " + + "that are still durable in the skipped file -- duplicate frames now, " + + "poisoned contiguity check on the next recovery. Expected: " + + "MmapSegmentException."); + } finally { + if (ring != null) { + ring.close(); + } + } + }); + } + + /** + * Lowest segment unreadable: the survivors are mutually contiguous, so + * the interior gap check cannot fire and recovery succeeds without the + * lowest segment's frames. Worse, {@link CursorSendEngine} then seeds + * {@code ackedFsn = newLowestBase - 1}, permanently classifying the lost + * unacked frames as acked -- they are never replayed. Must fail closed + * instead. + */ + @Test + public void testLowestUnreadableSegmentMustFailRecoveryClosed() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String lowPath = tmpDir + "/sf-0000000000000000.sfa"; + writeFrames(lowPath, 0L, 2); // FSN 0..1 + writeFrames(tmpDir + "/sf-0000000000000001.sfa", 2L, 2); // FSN 2..3 + clobberVersionByte(lowPath); + + SegmentRing ring; + try { + ring = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES); + } catch (MmapSegmentException expected) { + // Fixed behavior: fail closed on the unreadable file itself + // (not e.g. an incidental FSN-gap throw). + Assert.assertTrue(expected.getMessage(), + expected.getMessage().contains("unreadable segment file")); + return; + } + try { + boolean lowFramesRecovered = ring != null + && ring.findSegmentContaining(0L) != null; + Assert.assertTrue( + "FINDING: openExisting silently skipped the LOWEST segment " + + "(unsupported version -> MmapSegmentException -> " + + "logged-and-ignored). The survivors are mutually contiguous so " + + "the gap check cannot fire; FSNs 0..1 are gone and " + + "CursorSendEngine would seed ackedFsn = 1, marking the lost " + + "unacked frames as acked forever. Silent data loss. " + + "Expected: MmapSegmentException.", + lowFramesRecovered); + } finally { + if (ring != null) { + ring.close(); + } + } + }); + } + + /** + * Sole segment unreadable, ring level: recovery must not report "no + * data" ({@code null}) when a recognizable SF segment with frames exists + * but cannot be opened -- {@code null} is precisely what routes the + * caller onto the truncating fresh-start path. + */ + @Test + public void testSoleUnreadableSegmentMustFailRecoveryClosed() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String segPath = tmpDir + "/sf-0000000000000000.sfa"; + writeFrames(segPath, 0L, 3); + clobberVersionByte(segPath); + + SegmentRing ring; + try { + ring = SegmentRing.openExisting(tmpDir, SEGMENT_BYTES); + } catch (MmapSegmentException expected) { + // Fixed behavior: fail closed on the unreadable file itself + // (not e.g. an incidental FSN-gap throw). + Assert.assertTrue(expected.getMessage(), + expected.getMessage().contains("unreadable segment file")); + return; + } + if (ring != null) { + ring.close(); + Assert.fail("openExisting returned a ring from a sole unreadable segment -- " + + "unexpected; revisit this test's corruption setup"); + } + Assert.fail("FINDING: openExisting returned null for a slot whose ONLY segment " + + "is a recognizable SF file with 3 frames that failed recovery " + + "(unsupported version). The caller cannot distinguish this from an " + + "empty store and will restart at FSN 0, truncating sf-initial.sfa " + + "if that is the surviving file's name. Expected: MmapSegmentException."); + }); + } + + /** + * Clobbers the header VERSION byte (offset 4) while leaving the magic + * intact: unambiguously a recognizable SF segment that fails recovery + * with {@link MmapSegmentException} ("unsupported version"), not a stray + * foreign file. + */ + private static void clobberVersionByte(String path) { + int fd = Files.openRW(path); + Assert.assertTrue("openRW failed for " + path, fd >= 0); + long buf = Unsafe.malloc(1, MemoryTag.NATIVE_DEFAULT); + try { + Unsafe.getUnsafe().putByte(buf, (byte) 0x63); + Assert.assertEquals("version-byte clobber failed", 1L, Files.write(fd, buf, 1, 4)); + } finally { + Unsafe.free(buf, 1, MemoryTag.NATIVE_DEFAULT); + Files.close(fd); + } + } + + private static byte readByteAt(String path, long offset) { + int fd = Files.openRW(path); + Assert.assertTrue("openRW failed for " + path, fd >= 0); + long buf = Unsafe.malloc(1, MemoryTag.NATIVE_DEFAULT); + try { + Assert.assertEquals("probe read failed", 1L, Files.read(fd, buf, 1, offset)); + return Unsafe.getUnsafe().getByte(buf); + } finally { + Unsafe.free(buf, 1, MemoryTag.NATIVE_DEFAULT); + Files.close(fd); + } + } + + /** + * Creates a segment at {@code path} and appends {@code frameCount} + * frames whose payload bytes are all non-zero, so tests can tell + * surviving data from a zero-filled re-created file. + */ + private static void writeFrames(String path, long baseSeq, int frameCount) { + long buf = Unsafe.malloc(FRAME_PAYLOAD_LEN, MemoryTag.NATIVE_DEFAULT); + try { + for (int i = 0; i < FRAME_PAYLOAD_LEN; i++) { + Unsafe.getUnsafe().putByte(buf + i, (byte) (i | 1)); + } + try (MmapSegment seg = MmapSegment.create(path, baseSeq, SEGMENT_BYTES)) { + for (int i = 0; i < frameCount; i++) { + Assert.assertTrue("setup: append must fit", + seg.tryAppend(buf, FRAME_PAYLOAD_LEN) >= 0); + } + } + } finally { + Unsafe.free(buf, FRAME_PAYLOAD_LEN, MemoryTag.NATIVE_DEFAULT); + } + } +} diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java index 5c30386d..d2b66b67 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingTest.java @@ -369,7 +369,7 @@ public void testOpenExistingDetectsFsnGap() throws Exception { } @Test - public void testOpenExistingSkipsBadMagicFile() throws Exception { + public void testOpenExistingFailsClosedOnBadMagicFile() throws Exception { TestUtils.assertMemoryLeak(() -> { long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 16); @@ -379,7 +379,12 @@ public void testOpenExistingSkipsBadMagicFile() throws Exception { MmapSegment s0 = MmapSegment.create(tmpDir + "/good.sfa", 0, segSize); s0.tryAppend(buf, 16); s0.close(); - // One stray .sfa with no proper header — must be ignored. + // One .sfa with no proper header. A torn header write at + // crash produces exactly this on a REAL segment, so recovery + // must fail closed rather than skip: skipped at the lowest/ + // highest/sole position it silently loses or duplicates FSNs + // (see SegmentRingEdgeRecoveryTest), and "stray foreign + // file" is indistinguishable from "torn real segment". int fd = Files.openCleanRW(tmpDir + "/stray.sfa"); long hdr = Unsafe.malloc(8, MemoryTag.NATIVE_DEFAULT); try { @@ -391,11 +396,19 @@ public void testOpenExistingSkipsBadMagicFile() throws Exception { Unsafe.free(hdr, 8, MemoryTag.NATIVE_DEFAULT); } - try (SegmentRing recovered = SegmentRing.openExisting(tmpDir, segSize)) { - assertNotNull(recovered); - assertEquals(0, recovered.getActive().baseSeq()); - assertEquals(0, recovered.getSealedSegments().size()); + try { + Misc.free(SegmentRing.openExisting(tmpDir, segSize)); + throw new AssertionError("expected fail-closed recovery on unreadable .sfa"); + } catch (MmapSegmentException expected) { + assertTrue(expected.getMessage(), + expected.getMessage().contains("unreadable segment file")); + assertTrue("message must name the offending file: " + expected.getMessage(), + expected.getMessage().contains("stray.sfa")); } + // Fail closed must also preserve the good segment untouched + // for the next (post-remediation) recovery attempt. + assertTrue("good segment must survive a failed recovery", + Files.exists(tmpDir + "/good.sfa")); } finally { Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT); } From e2fc980acce936aa5a989208b5e3dc9f888f0c59 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Sat, 11 Jul 2026 00:25:16 +0100 Subject: [PATCH 07/12] Bound the deferred-close daemon and abandon permanently stuck engines CursorSendEngine.closeEventually() used to submit a forever-retrying task to an unbounded Executors.newCachedThreadPool(): each stuck engine pinned one daemon thread in a while-loop whose every close() attempt can block ~10s (5s awaitRingQuiescence + 5s owned-manager join), so E stuck engines retained E threads plus E rings, mmaps, watermarks and flocks with no global bound and no give-up. Replace it with a single shared single-thread scheduled daemon. Each task performs exactly ONE close() attempt and reschedules itself with exponential backoff (10ms doubling to a 10s cap), so E stuck engines cost E queued tasks, not E threads. After 40 attempts the engine is abandoned: terminal for the daemon, one ERROR logged, resources deliberately stay leaked until process exit (the kernel releases the flock; releasing them early would hand the slot to a replacement engine the stale worker could still corrupt), and the process-wide count is observable via getAbandonedDeferredCloseCount(). close() may still be invoked directly after abandonment. The per-engine deferredCloseScheduled flag keeps scheduling deduplicated. Add a many-stuck-engines test pinning the single-thread bound plus eventual completion, and a permanently-stuck-manager test pinning the retry budget, the retained slot lock after abandonment, and the direct close() recovery path. Also reattach the unlinkAllSegmentFiles javadoc that had drifted onto the old retry-loop method. --- .../client/sf/cursor/CursorSendEngine.java | 143 ++++++++++--- ...CursorSendEngineSlotReacquisitionTest.java | 195 ++++++++++++++++++ 2 files changed, 312 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java index a596e3ff..ca45c01f 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorSendEngine.java @@ -28,6 +28,7 @@ import io.questdb.client.std.Files; import io.questdb.client.std.ObjList; import io.questdb.client.std.QuietCloseable; +import org.jetbrains.annotations.TestOnly; import java.util.concurrent.locks.LockSupport; @@ -61,14 +62,41 @@ public final class CursorSendEngine implements QuietCloseable { * Default deadline for {@link #appendBlocking}: 30 seconds. */ public static final long DEFAULT_APPEND_DEADLINE_NANOS = 30_000_000_000L; - private static final java.util.concurrent.ExecutorService DEFERRED_CLOSE_EXECUTOR = - java.util.concurrent.Executors.newCachedThreadPool(runnable -> { + /** + * Default retry budget for {@link #closeEventually()}: initial backoff, + * attempt cap and backoff cap. With these defaults a stuck engine is + * retried for roughly five minutes of scheduled delay (plus whatever + * time each {@link #close()} attempt spends blocked on the quiescence + * barrier) before it is abandoned. + */ + public static final long DEFERRED_CLOSE_DEFAULT_INITIAL_BACKOFF_NANOS = 10_000_000L; // 10 ms + public static final int DEFERRED_CLOSE_DEFAULT_MAX_ATTEMPTS = 40; + public static final long DEFERRED_CLOSE_DEFAULT_MAX_BACKOFF_NANOS = 10_000_000_000L; // 10 s + // Number of engines (process-wide) whose deferred close exhausted its + // retry budget and was abandoned: their ring, watermark and slot lock + // leak until process exit. Observability + test hook; never reset. + private static final java.util.concurrent.atomic.AtomicLong DEFERRED_CLOSE_ABANDONED_COUNT = + new java.util.concurrent.atomic.AtomicLong(); + // Single shared daemon that owns every deferred close retry. One thread + // bounds the process footprint no matter how many engines are stuck: + // each task performs ONE close() attempt and reschedules itself with + // exponential backoff — it never loops or parks in-thread — so E stuck + // engines cost E queued tasks, not E threads. Attempts serialize, which + // is acceptable for a degraded-mode path whose per-attempt blocking is + // bounded by the manager's worker-join timeout. + private static final java.util.concurrent.ScheduledExecutorService DEFERRED_CLOSE_EXECUTOR = + java.util.concurrent.Executors.newSingleThreadScheduledExecutor(runnable -> { Thread thread = new Thread(runnable, "qdb-cursor-engine-close"); thread.setDaemon(true); return thread; }); private static final org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger(CursorSendEngine.class); + // Deferred-close retry tuning. volatile: the @TestOnly setter may run on + // a different thread than the scheduler that reads them. + private static volatile long deferredCloseInitialBackoffNanos = DEFERRED_CLOSE_DEFAULT_INITIAL_BACKOFF_NANOS; + private static volatile int deferredCloseMaxAttempts = DEFERRED_CLOSE_DEFAULT_MAX_ATTEMPTS; + private static volatile long deferredCloseMaxBackoffNanos = DEFERRED_CLOSE_DEFAULT_MAX_BACKOFF_NANOS; private final long appendDeadlineNanos; // Number of times appendBlocking observed BACKPRESSURE_NO_SPARE on its first // ring.appendOrFsn attempt. One increment per blocking-call that had to wait @@ -126,8 +154,16 @@ public final class CursorSendEngine implements QuietCloseable { // retries the cleanup (the worker may have exited by then). Guarded by // the synchronized close() method and isCloseCompleted() accessor. private boolean closeCompleted; - // True once a dedicated daemon has accepted ownership of incomplete close - // retries. Guarded by synchronized closeEventually(). + // Deferred-close retry state. Initialized under closeEventually()'s + // monitor before the first task is submitted (task submission + // happens-before publishes them to the scheduler thread); thereafter + // touched only by the single deferred-close scheduler thread. + private int deferredCloseAttempts; + private long deferredCloseBackoffNanos; + // True once the shared daemon has accepted ownership of incomplete close + // retries. Guarded by synchronized closeEventually(). Stays true after + // the retry budget is exhausted — abandonment is terminal for the daemon, + // though close() may still be invoked directly. private boolean deferredCloseScheduled; // Producer-thread-only: timestamp of the last "we're backpressured" log // line, used to throttle. Plain long is fine. @@ -638,18 +674,27 @@ public synchronized void close() { } /** - * Transfers an incomplete close to a dedicated daemon owner. This lets - * I/O and drainer executors terminate even when a manager service pass is - * permanently stalled. The deferred owner retains this engine, and thus - * its flock and native mappings, until a retry completes. + * Transfers an incomplete close to a shared single-thread daemon owner. + * This lets I/O and drainer executors terminate even when a manager + * service pass is permanently stalled. The deferred owner retains this + * engine, and thus its flock and native mappings, until a retry + * completes — or until the retry budget + * ({@link #DEFERRED_CLOSE_DEFAULT_MAX_ATTEMPTS} attempts with + * exponential backoff) is exhausted, at which point the engine is + * abandoned: its resources stay leaked until process exit (the kernel + * releases the flock), {@link #getAbandonedDeferredCloseCount()} is + * incremented, and one ERROR is logged. {@link #close()} may still be + * invoked directly after abandonment. */ public synchronized void closeEventually() { if (closeCompleted || deferredCloseScheduled) { return; } deferredCloseScheduled = true; + deferredCloseAttempts = 0; + deferredCloseBackoffNanos = deferredCloseInitialBackoffNanos; try { - DEFERRED_CLOSE_EXECUTOR.execute(this::runDeferredClose); + DEFERRED_CLOSE_EXECUTOR.execute(this::deferredCloseAttempt); } catch (Throwable t) { deferredCloseScheduled = false; throw t; @@ -670,6 +715,27 @@ public MmapSegment firstSealed() { return ring.firstSealed(); } + /** + * Number of engines (process-wide) whose deferred close exhausted its + * retry budget and was abandoned, leaking their ring, watermark and + * slot lock until process exit. Cumulative; never reset. + */ + public static long getAbandonedDeferredCloseCount() { + return DEFERRED_CLOSE_ABANDONED_COUNT.get(); + } + + /** + * Tunes the deferred-close retry budget. Affects engines armed by + * {@link #closeEventually()} after this call. Tests must restore the + * {@code DEFERRED_CLOSE_DEFAULT_*} values when done. + */ + @TestOnly + public static void setDeferredCloseTuning(long initialBackoffNanos, long maxBackoffNanos, int maxAttempts) { + deferredCloseInitialBackoffNanos = initialBackoffNanos; + deferredCloseMaxBackoffNanos = maxBackoffNanos; + deferredCloseMaxAttempts = maxAttempts; + } + /** * Number of times {@link #appendBlocking} hit * {@link SegmentRing#BACKPRESSURE_NO_SPARE} on its first attempt and @@ -763,6 +829,48 @@ public long recoveredOrphanTipFsn() { return recoveredOrphanTipFsn; } + /** + * One deferred-close retry. Performs a single {@link #close()} attempt; + * on failure reschedules itself with exponential backoff until the + * retry budget is exhausted, then abandons the engine (terminal for the + * daemon: resources leak until process exit). Runs on the shared + * single-thread deferred-close daemon and never loops or parks, so one + * stuck engine cannot pin a thread and E stuck engines cannot pin E + * threads. + */ + private void deferredCloseAttempt() { + try { + close(); + } catch (Throwable ignored) { + // Retain ownership; the retry budget below decides what's next. + } + // The daemon owns cleanup independently of caller cancellation. Clear + // a stray interrupt so it cannot poison the joins/waits of the next + // attempt on this shared thread. + Thread.interrupted(); + if (isCloseCompleted()) { + return; + } + if (++deferredCloseAttempts >= deferredCloseMaxAttempts) { + DEFERRED_CLOSE_ABANDONED_COUNT.incrementAndGet(); + LOG.error("abandoning deferred close after {} attempts: the SF manager worker never " + + "quiesced; ring, watermark and slot lock for {} stay leaked until process " + + "exit (the kernel releases the flock). close() may still be invoked " + + "directly to retry cleanup.", deferredCloseAttempts, sfDir == null ? "" : sfDir); + return; + } + long delayNanos = deferredCloseBackoffNanos; + deferredCloseBackoffNanos = Math.min(delayNanos * 2, deferredCloseMaxBackoffNanos); + try { + DEFERRED_CLOSE_EXECUTOR.schedule( + this::deferredCloseAttempt, delayNanos, java.util.concurrent.TimeUnit.NANOSECONDS); + } catch (Throwable t) { + DEFERRED_CLOSE_ABANDONED_COUNT.incrementAndGet(); + LOG.error("could not reschedule deferred close for {}; ring, watermark and slot lock " + + "stay leaked until process exit", sfDir == null ? "" : sfDir, t); + } + } + /** * Unlinks every {@code .sfa} file under {@code dir}. Called only on * clean shutdown when the ring confirms every published FSN has been @@ -771,23 +879,6 @@ public long recoveredOrphanTipFsn() { * Best-effort: logs and continues on failures, since we're already on * the close path. */ - private void runDeferredClose() { - while (!isCloseCompleted()) { - try { - close(); - } catch (Throwable ignored) { - // Retain ownership and retry after a bounded pause. - } - if (!isCloseCompleted()) { - LockSupport.parkNanos(10_000_000L); - // The internal daemon owns cleanup independently of caller - // cancellation. Clear interrupts so they cannot create a hot - // retry loop if an executor implementation interrupts it. - Thread.interrupted(); - } - } - } - private static void unlinkAllSegmentFiles(String dir) { if (!io.questdb.client.std.Files.exists(dir)) return; long find = io.questdb.client.std.Files.findFirst(dir); diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java index 28f72f96..35c71c0c 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorSendEngineSlotReacquisitionTest.java @@ -386,6 +386,191 @@ public void testOwnerCloseRetainsEngineUntilCleanupCompletes() throws Exception }); } + /** + * Regression for the unbounded deferred-close thread pool: E engines + * stuck behind stalled manager workers must share ONE deferred-close + * daemon thread (the old newCachedThreadPool + in-thread retry loop + * pinned one thread per engine), and every engine must still complete + * once its worker quiesces. + */ + @Test(timeout = 60_000L) + public void testManyStuckEnginesShareOneDeferredCloseThread() throws Exception { + TestUtils.assertMemoryLeak(() -> { + final int engineCount = 4; + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + SegmentManager[] managers = new SegmentManager[engineCount]; + CursorSendEngine[] engines = new CursorSendEngine[engineCount]; + CountDownLatch releaseWorkers = new CountDownLatch(1); + // Fast retries so completion after release is prompt; attempt + // budget large enough that nothing is abandoned mid-test. + CursorSendEngine.setDeferredCloseTuning(1_000_000L, 50_000_000L, 1_000_000); + try { + for (int i = 0; i < engineCount; i++) { + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + managers[i] = manager; + CountDownLatch workerBlocked = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + releaseWorkers.await(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engines[i] = new CursorSendEngine(tmpDir + "/many-slot-" + i, segSize, manager); + Assert.assertTrue("worker " + i + " never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + manager.setWorkerJoinTimeoutMillis(20L); + engines[i].close(); + Assert.assertFalse("close " + i + " must remain incomplete", + engines[i].isCloseCompleted()); + engines[i].closeEventually(); + } + + // Let the daemon cycle through several retries for every engine. + Thread.sleep(300L); + for (int i = 0; i < engineCount; i++) { + Assert.assertFalse("engine " + i + " cannot complete while its worker is stalled", + engines[i].isCloseCompleted()); + } + Assert.assertTrue( + "deferred close must be bounded to a single shared daemon thread", + countDeferredCloseThreads() <= 1 + ); + + releaseWorkers.countDown(); + for (SegmentManager manager : managers) { + manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); + } + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(20); + for (int i = 0; i < engineCount; i++) { + while (!engines[i].isCloseCompleted() && System.nanoTime() < deadline) { + Thread.sleep(1L); + } + Assert.assertTrue("deferred close " + i + " did not complete after quiescence", + engines[i].isCloseCompleted()); + } + } finally { + CursorSendEngine.setDeferredCloseTuning( + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_INITIAL_BACKOFF_NANOS, + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_MAX_BACKOFF_NANOS, + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_MAX_ATTEMPTS + ); + releaseWorkers.countDown(); + for (int i = 0; i < engineCount; i++) { + if (managers[i] != null) { + managers[i].setBeforeInstallSyncHook(null); + } + if (engines[i] != null) { + try { + engines[i].close(); + } catch (Throwable ignored) { + } + } + if (managers[i] != null) { + try { + managers[i].close(); + } catch (Throwable ignored) { + } + } + } + } + }); + } + + /** + * Permanently-stuck-manager path: the deferred-close daemon must not + * retry forever. After the retry budget is exhausted the engine is + * abandoned (observable via getAbandonedDeferredCloseCount), its slot + * stays locked (leaked, not released to a replacement the stale worker + * could corrupt), and a later direct close() — once the worker finally + * quiesces — still completes the cleanup. + */ + @Test(timeout = 60_000L) + public void testPermanentlyStuckManagerAbandonsDeferredCloseAfterRetryBudget() throws Exception { + TestUtils.assertMemoryLeak(() -> { + long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32); + String slot = tmpDir + "/abandon-slot"; + SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch workerBlocked = new CountDownLatch(1); + CountDownLatch releaseWorker = new CountDownLatch(1); + AtomicBoolean fired = new AtomicBoolean(); + CursorSendEngine engine = null; + boolean managerClosed = false; + CursorSendEngine.setDeferredCloseTuning(1_000_000L, 2_000_000L, 3); + try { + manager.setBeforeInstallSyncHook(() -> { + if (!fired.compareAndSet(false, true)) return; + workerBlocked.countDown(); + try { + releaseWorker.await(30, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engine = new CursorSendEngine(slot, segSize, manager); + Assert.assertTrue("worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + + manager.setWorkerJoinTimeoutMillis(20L); + engine.close(); + Assert.assertFalse("first close must remain incomplete", engine.isCloseCompleted()); + + long abandonedBefore = CursorSendEngine.getAbandonedDeferredCloseCount(); + engine.closeEventually(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (CursorSendEngine.getAbandonedDeferredCloseCount() == abandonedBefore + && System.nanoTime() < deadline) { + Thread.sleep(1L); + } + Assert.assertEquals("deferred close must give up after its retry budget", + abandonedBefore + 1, CursorSendEngine.getAbandonedDeferredCloseCount()); + Assert.assertFalse("an abandoned engine stays incomplete", engine.isCloseCompleted()); + // Abandonment leaks — it must NOT hand the slot to a + // replacement while the stale worker can still touch it. + try (SlotLock ignored = SlotLock.acquire(slot)) { + Assert.fail("abandoned deferred close must retain the slot lock"); + } catch (IllegalStateException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("sf slot already in use")); + } + + // Recovery path promised by the abandonment log: a direct + // close() once the worker quiesces. + releaseWorker.countDown(); + manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60)); + engine.close(); + Assert.assertTrue("direct close after abandonment must complete", + engine.isCloseCompleted()); + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after recovery close", probe); + } + manager.close(); + managerClosed = true; + } finally { + CursorSendEngine.setDeferredCloseTuning( + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_INITIAL_BACKOFF_NANOS, + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_MAX_BACKOFF_NANOS, + CursorSendEngine.DEFERRED_CLOSE_DEFAULT_MAX_ATTEMPTS + ); + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (engine != null) { + try { + engine.close(); + } catch (Throwable ignored) { + } + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + /** * Plain-positive path: after a normal close (worker quiesces promptly), * a second engine must be able to acquire and use the same slot. @@ -406,6 +591,16 @@ public void testSameSlotReacquirableAfterNormalClose() throws Exception { }); } + private static int countDeferredCloseThreads() { + int count = 0; + for (Thread t : Thread.getAllStackTraces().keySet()) { + if (t.isAlive() && t.getName().startsWith("qdb-cursor-engine-close")) { + count++; + } + } + return count; + } + private static void rmDirRecursive(String dir) { if (!Files.exists(dir)) return; long find = Files.findFirst(dir); From 76db2e0769045ff1bdf9d0b6fb8cc918d739197b Mon Sep 17 00:00:00 2001 From: bluestreak Date: Sat, 11 Jul 2026 02:23:43 +0100 Subject: [PATCH 08/12] Avoid double directory scan when registering a recovered SF ring SegmentRing.openExisting walked every slot-directory entry to recover segments, then SegmentManager.register immediately re-enumerated the same directory (scanMaxGeneration) to seed the spare file-generation counter, re-allocating a name String plus substring per entry. The recovery scan now tracks the maximum sf-.sfa generation it observes and carries it on the returned ring (maxRecoveredFileGeneration); register() uses that value and only falls back to the legacy directory rescan for rings that were not built by recovery (FILE_GENERATION_UNKNOWN: fresh start, memory mode, tests), so behavior for non-recovered rings is unchanged. Safety properties, pinned by SegmentRingRecoveryGenerationTest: - the maximum covers every scanned entry, including empty leftovers recovery unlinks and corrupt files it quarantines, so a generation is never reused even for files gone by registration time; - name->generation parsing is a single shared helper (SegmentRing.parseFileGeneration) used by both the recovery scan and the fallback rescan, so the two can never disagree; - freshly minted spares always land strictly above everything on disk -- never at a name whose truncating openCleanRW would destroy a recovered segment. The carried value cannot be stale for its intended (first) registration: recovery and register run on one thread under the engine slot lock, and no spare can be minted into the slot before register wires the ring up. Same-manager re-registration stays safe because advanceFileGeneration only ratchets upward. --- .../qwp/client/sf/cursor/SegmentManager.java | 39 ++- .../qwp/client/sf/cursor/SegmentRing.java | 76 +++++ .../SegmentRingRecoveryGenerationTest.java | 276 ++++++++++++++++++ 3 files changed, 378 insertions(+), 13 deletions(-) create mode 100644 core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryGenerationTest.java diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java index eaf5d407..364b9a20 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentManager.java @@ -418,7 +418,25 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) { // spare at sf-0000000000000000.sfa — and openCleanRW would truncate the // user's existing active file out from under the I/O loop, scrambling // the in-flight mmap. Memory-mode rings have no dir; nothing to scan. - long minNextGeneration = dir == null ? -1L : scanMaxGeneration(dir) + 1L; + // + // A ring produced by SegmentRing.openExisting already carries the + // generation maximum from the full directory scan recovery just + // performed -- prefer it over re-enumerating the directory here. + // The recovered value cannot be stale: the engine holds the slot + // lock, and no spare can be minted into this slot before this call + // wires the ring up. It also cannot be too low after a deregister/ + // re-register cycle: advanceFileGeneration only ever ratchets the + // counter upwards. Rings not built by recovery (fresh start, tests) + // report FILE_GENERATION_UNKNOWN and keep the legacy rescan. + long minNextGeneration; + if (dir == null) { + minNextGeneration = -1L; + } else { + long recoveredMax = ring.maxRecoveredFileGeneration(); + minNextGeneration = (recoveredMax != SegmentRing.FILE_GENERATION_UNKNOWN + ? recoveredMax + : scanMaxGeneration(dir)) + 1L; + } Runnable managerWakeup = this::wakeWorker; RingEntry e = new RingEntry(ring, dir, watermark); // ObjList.add either throws before storing e or makes the entry visible. @@ -501,7 +519,11 @@ public void wakeWorker() { /** * Returns the highest hex-encoded generation across {@code sf-.sfa} * files in {@code dir}, or {@code -1} if none exist. Skips files that - * don't match the pattern (e.g. the legacy {@code sf-initial.sfa}). + * don't match the pattern (e.g. the legacy {@code sf-initial.sfa}); + * name matching is delegated to {@link SegmentRing#parseFileGeneration} + * so this fallback scan and the recovery scan can never disagree. + * Fallback only: {@link #register(SegmentRing, String, AckWatermark)} + * calls this just for rings that don't carry a recovery-scanned maximum. */ private static long scanMaxGeneration(String dir) { long max = -1L; @@ -518,17 +540,8 @@ private static long scanMaxGeneration(String dir) { while (rc > 0) { String name = Files.utf8ToString(Files.findName(find)); rc = Files.findNext(find); - if (name == null || !name.startsWith("sf-") || !name.endsWith(".sfa")) { - continue; - } - String hex = name.substring(3, name.length() - 4); - if (hex.length() != 16) continue; - try { - long gen = Long.parseUnsignedLong(hex, 16); - if (gen > max) max = gen; - } catch (NumberFormatException ignored) { - // sf-initial.sfa or non-hex — skip - } + long gen = SegmentRing.parseFileGeneration(name); + if (gen > max) max = gen; } } finally { Files.findClose(find); diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index b3ba57f5..ee3877aa 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -60,6 +60,14 @@ public final class SegmentRing implements QuietCloseable { /** Sentinel: append failed because no hot spare was available to rotate into. */ public static final long BACKPRESSURE_NO_SPARE = -1L; + /** + * {@link #maxRecoveredFileGeneration()} value for rings that were not + * produced by a directory-scanning recovery (constructor-built rings: + * fresh start, memory mode, tests). {@code Long.MIN_VALUE} cannot collide + * with a real scan result, whose minimum is {@code -1} (scanned, but no + * generation-named files found). + */ + public static final long FILE_GENERATION_UNKNOWN = Long.MIN_VALUE; /** Sentinel: append failed because the payload doesn't fit in a fresh segment. */ public static final long PAYLOAD_TOO_LARGE = -2L; private static final Logger LOG = LoggerFactory.getLogger(SegmentRing.class); @@ -106,6 +114,11 @@ public final class SegmentRing implements QuietCloseable { // manager only notices on its next polling tick -- fine on average, // but the worst-case wait is the full poll interval. Producer-thread-only. private Runnable managerWakeup; + // Highest sf-.sfa file generation observed by the openExisting + // directory scan, or FILE_GENERATION_UNKNOWN for constructor-built rings. + // Written once by openExisting before the ring is published to the + // caller; read-only afterwards (see maxRecoveredFileGeneration()). + private long maxRecoveredFileGeneration = FILE_GENERATION_UNKNOWN; private long nextSeq; private volatile long publishedFsn; // Plain (producer-thread-only) flag; set to true the first time we ask @@ -170,6 +183,11 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { return null; } ObjList opened = new ObjList<>(); + // Highest sf-.sfa generation seen during the scan; -1 when + // none matched. Carried on the returned ring so SegmentManager's + // register() can seed its spare file-generation counter without + // re-enumerating the directory this scan just walked. + long maxFileGeneration = -1L; long find = Files.findFirst(sfDir); if (find < 0) { // Fail closed: an enumeration failure (permissions, transient @@ -196,6 +214,16 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { int rc = 1; while (rc > 0) { String name = Files.utf8ToString(Files.findName(find)); + // Track the highest file generation over EVERY entry the + // scan visits, before the unlink/quarantine below can + // remove it. Including removed files keeps the bound + // conservative -- a generation is never reused, so a new + // spare can never collide with a quarantined twin's + // .sfa.corrupt on a future quarantine rename. + long gen = parseFileGeneration(name); + if (gen > maxFileGeneration) { + maxFileGeneration = gen; + } if (name != null && name.endsWith(".sfa")) { String path = sfDir + "/" + name; MmapSegment seg = null; @@ -355,6 +383,7 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { for (int i = 0, n = opened.size(); i < n; i++) { ring.sealedSegments.add(opened.get(i)); } + ring.maxRecoveredFileGeneration = maxFileGeneration; return ring; } catch (Throwable t) { // Close every recovered MmapSegment that's still in `opened`. @@ -374,6 +403,31 @@ public static SegmentRing openExisting(String sfDir, long maxBytesPerSegment) { } } + /** + * Parses the spare-file generation out of a {@code sf-.sfa} + * file name; returns {@code -1} for anything else ({@code sf-initial.sfa}, + * non-hex, wrong hex length, {@code null}). Single source of truth shared + * by the recovery scan above and the segment manager's fallback directory + * scan (scanMaxGeneration) so the two can never disagree on which file + * names carry a generation. Note: startsWith + endsWith together imply a + * minimum length of 7 (the prefix and suffix cannot overlap), so the + * substring below cannot go out of bounds. + */ + static long parseFileGeneration(String name) { + if (name == null || !name.startsWith("sf-") || !name.endsWith(".sfa")) { + return -1L; + } + String hex = name.substring(3, name.length() - 4); + if (hex.length() != 16) { + return -1L; + } + try { + return Long.parseUnsignedLong(hex, 16); + } catch (NumberFormatException e) { + return -1L; + } + } + /** * Highest FSN that the server has ACK'd. Read by the segment manager to * decide which sealed segments are safe to munmap + unlink. @@ -643,6 +697,28 @@ public long maxBytesPerSegment() { return maxBytesPerSegment; } + /** + * Highest {@code sf-.sfa} file generation observed during the + * {@link #openExisting} directory scan: {@code -1} when the scan matched + * no generation-named files, or {@link #FILE_GENERATION_UNKNOWN} when + * this ring was not produced by recovery at all (constructor-built: + * fresh start, memory mode, tests). The maximum is taken over every + * entry the scan visited -- including empty leftovers it unlinked and + * corrupt files it quarantined -- so it is a safe upper bound for the + * segment manager's spare file-generation counter even though those + * files are gone by the time the ring is registered. + *

    + * Valid only for seeding the ring's FIRST registration: it is a + * construction-time snapshot, so once any manager has minted spares + * into the slot it is stale. Re-registering with the SAME manager stays + * safe regardless -- its generation counter only ratchets upward -- but + * a hypothetical hand-off to a different manager instance must rescan + * the directory instead of trusting this value. + */ + public long maxRecoveredFileGeneration() { + return maxRecoveredFileGeneration; + } + /** True when the segment manager should prepare and install a fresh spare. */ public boolean needsHotSpare() { return hotSpare == null; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryGenerationTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryGenerationTest.java new file mode 100644 index 00000000..94ec0b89 --- /dev/null +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRingRecoveryGenerationTest.java @@ -0,0 +1,276 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2026 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.client.test.cutlass.qwp.client.sf.cursor; + +import io.questdb.client.cutlass.qwp.client.sf.cursor.MmapSegment; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentManager; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentRing; +import io.questdb.client.std.Files; +import io.questdb.client.std.MemoryTag; +import io.questdb.client.std.Unsafe; +import io.questdb.client.test.tools.TestUtils; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.List; + +/** + * {@link SegmentRing#openExisting} computes the maximum on-disk spare file + * generation during its recovery scan and carries it on the returned ring + * ({@link SegmentRing#maxRecoveredFileGeneration()}), so that + * {@link SegmentManager#register} can seed its spare file-generation counter + * without re-enumerating the directory recovery just walked. These tests pin + * the two safety properties that make the single-scan optimization sound: + *

      + *
    • the carried maximum covers every scanned entry, including files + * recovery itself unlinks (empty leftovers) — so generations are never + * reused even for files that are gone by registration time;
      • + *
    • after recovery + registration, freshly minted spares never collide + * with (and therefore never truncate) a recovered on-disk segment.
      • + *
    + */ +public class SegmentRingRecoveryGenerationTest { + + private static final long SEGMENT_SIZE = 64 * 1024; + private String slotDir; + + @Before + public void setUp() { + slotDir = Paths.get(System.getProperty("java.io.tmpdir"), + "qdb-ring-recover-gen-" + System.nanoTime()).toString(); + Assert.assertEquals(0, Files.mkdir(slotDir, Files.DIR_MODE_DEFAULT)); + } + + @After + public void tearDown() { + if (slotDir == null) return; + rmDirRec(slotDir); + } + + @Test + public void testRecoveredRingCarriesMaxFileGeneration() throws Exception { + TestUtils.assertMemoryLeak(() -> { + // Mixed naming, mirroring a real slot: the legacy initial file + // (no generation) plus two generation-named spares that were + // rotated into service. Non-generation names must not contribute. + createSegmentWithOneFrame(slotDir + "/sf-initial.sfa", 0L); + createSegmentWithOneFrame(slotDir + "/sf-0000000000000005.sfa", 1L); + createSegmentWithOneFrame(slotDir + "/sf-0000000000000007.sfa", 2L); + + SegmentRing ring = SegmentRing.openExisting(slotDir, SEGMENT_SIZE); + Assert.assertNotNull("recovery should produce a ring", ring); + try { + Assert.assertEquals( + "recovered ring must report the highest generation seen by the scan", + 7L, ring.maxRecoveredFileGeneration()); + } finally { + ring.close(); + } + }); + } + + @Test + public void testConstructorRingReportsUnknownGeneration() throws Exception { + TestUtils.assertMemoryLeak(() -> { + MmapSegment initial = MmapSegment.createInMemory(0L, SEGMENT_SIZE); + SegmentRing ring = new SegmentRing(initial, SEGMENT_SIZE); + try { + Assert.assertEquals( + "constructor-built rings carry no recovery scan result; register()" + + " must fall back to its own directory scan for them", + SegmentRing.FILE_GENERATION_UNKNOWN, ring.maxRecoveredFileGeneration()); + } finally { + ring.close(); + } + }); + } + + @Test + public void testGenerationIncludesFilesRecoveryUnlinks() throws Exception { + TestUtils.assertMemoryLeak(() -> { + // One valid segment at generation 2 and one EMPTY hot-spare + // leftover at generation 0xa. Recovery unlinks the empty leftover, + // but its generation must still count towards the maximum: a + // generation is never reused, so the bound must cover every entry + // the scan visited, not just the survivors. + createSegmentWithOneFrame(slotDir + "/sf-0000000000000002.sfa", 0L); + MmapSegment empty = MmapSegment.create( + slotDir + "/sf-000000000000000a.sfa", 0L, SEGMENT_SIZE); + empty.close(); + + SegmentRing ring = SegmentRing.openExisting(slotDir, SEGMENT_SIZE); + Assert.assertNotNull("recovery should produce a ring", ring); + try { + Assert.assertFalse("empty leftover should have been unlinked", + Files.exists(slotDir + "/sf-000000000000000a.sfa")); + Assert.assertEquals( + "the unlinked leftover's generation (0xa) must dominate the" + + " survivor's (2) in the carried maximum", + 10L, ring.maxRecoveredFileGeneration()); + } finally { + ring.close(); + } + }); + } + + @Test + public void testManagerMintsNonCollidingSpareAfterRecovery() throws Exception { + TestUtils.assertMemoryLeak(() -> { + // End-to-end guard for the property the whole generation dance + // exists for: after recovery + registration, a freshly minted + // spare must land at a generation strictly above everything on + // disk — never at a name whose truncating create would destroy a + // recovered segment. + createSegmentWithOneFrame(slotDir + "/sf-0000000000000005.sfa", 0L); + createSegmentWithOneFrame(slotDir + "/sf-0000000000000007.sfa", 1L); + + SegmentRing ring = SegmentRing.openExisting(slotDir, SEGMENT_SIZE); + Assert.assertNotNull("recovery should produce a ring", ring); + try { + Assert.assertEquals(7L, ring.maxRecoveredFileGeneration()); + + // Generous cap so the manager provisions a spare for the + // born-full recovered active on its first ticks. + SegmentManager manager = new SegmentManager( + SEGMENT_SIZE, 1_000_000L /* 1ms */, 100 * SEGMENT_SIZE); + try (SegmentManager ignored = manager) { + manager.start(); + manager.register(ring, slotDir); + // Poll (bounded) for the spare to appear instead of a fixed + // sleep — keeps the test fast locally and non-flaky in CI. + long deadline = System.currentTimeMillis() + 10_000; + while (countSfaFiles(slotDir) < 3 && System.currentTimeMillis() < deadline) { + Thread.sleep(1); + } + } + Assert.assertTrue("manager should have provisioned at least one spare", + countSfaFiles(slotDir) >= 3); + + // Both recovered segments must still exist, and every + // generation-named file must be one of the originals (5, 7) or a + // newly minted spare strictly above the recovered maximum. + Assert.assertTrue(Files.exists(slotDir + "/sf-0000000000000005.sfa")); + Assert.assertTrue(Files.exists(slotDir + "/sf-0000000000000007.sfa")); + for (long gen : listGenerations(slotDir)) { + Assert.assertTrue( + "unexpected spare generation " + gen + " — a spare minted at or" + + " below the recovered maximum would have truncated a" + + " recovered segment via openCleanRW", + gen == 5L || gen == 7L || gen > 7L); + } + } finally { + ring.close(); + } + }); + } + + /** + * Creates a valid {@code .sfa} segment at {@code path} holding exactly one + * frame so {@link SegmentRing#openExisting} doesn't filter it as an empty + * orphan. Callers keep baseSeqs contiguous (one frame per segment). + */ + private static void createSegmentWithOneFrame(String path, long baseSeq) { + long buf = Unsafe.malloc(64, MemoryTag.NATIVE_DEFAULT); + try { + for (int i = 0; i < 64; i++) { + Unsafe.getUnsafe().putByte(buf + i, (byte) i); + } + try (MmapSegment seg = MmapSegment.create(path, baseSeq, SEGMENT_SIZE)) { + Assert.assertTrue("setup append should succeed", seg.tryAppend(buf, 64) >= 0); + } + } finally { + Unsafe.free(buf, 64, MemoryTag.NATIVE_DEFAULT); + } + } + + /** Test-side oracle for {@code sf-.sfa} names. */ + private static List listGenerations(String dir) { + List generations = new ArrayList<>(); + long find = Files.findFirst(dir); + if (find <= 0) return generations; + try { + int rc = 1; + while (rc > 0) { + String name = Files.utf8ToString(Files.findName(find)); + rc = Files.findNext(find); + if (name == null || !name.startsWith("sf-") || !name.endsWith(".sfa")) { + continue; + } + String hex = name.substring(3, name.length() - 4); + if (hex.length() != 16) continue; + try { + generations.add(Long.parseUnsignedLong(hex, 16)); + } catch (NumberFormatException ignored) { + } + } + } finally { + Files.findClose(find); + } + return generations; + } + + private static int countSfaFiles(String dir) { + if (!Files.exists(dir)) return 0; + long find = Files.findFirst(dir); + if (find <= 0) return 0; + int n = 0; + try { + int rc = 1; + while (rc > 0) { + String name = Files.utf8ToString(Files.findName(find)); + if (name != null && name.endsWith(".sfa")) n++; + rc = Files.findNext(find); + } + } finally { + Files.findClose(find); + } + return n; + } + + private static void rmDirRec(String dir) { + if (!Files.exists(dir)) return; + long find = Files.findFirst(dir); + if (find > 0) { + try { + int rc = 1; + while (rc > 0) { + String name = Files.utf8ToString(Files.findName(find)); + if (name != null && !".".equals(name) && !"..".equals(name)) { + String child = dir + "/" + name; + if (!Files.remove(child)) rmDirRec(child); + } + rc = Files.findNext(find); + } + } finally { + Files.findClose(find); + } + } + Files.remove(dir); + } +} From c6f8a330ff57d8ccd59311a7459545702d1cd82e Mon Sep 17 00:00:00 2001 From: bluestreak Date: Sat, 11 Jul 2026 02:26:17 +0100 Subject: [PATCH 09/12] Make bulk SF trim O(S) with a single range removal in drainTrimmable drainTrimmable retired fully-acked sealed segments with per-element remove(0), shifting the remaining suffix on every iteration -- a bulk trim of S segments cost O(S^2) element moves. Collect the eligible prefix first, then remove it with one ObjList.remove(0, eligible - 1) range removal (inclusive bounds), all under the same monitor as before, so the I/O thread's snapshotSealedSegments() still cannot observe a partially shifted list. Covered by the existing multi-segment bulk-drain assertion in SegmentRingTest. --- .../cutlass/qwp/client/sf/cursor/SegmentRing.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java index ee3877aa..dbc14858 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SegmentRing.java @@ -573,9 +573,13 @@ public synchronized ObjList drainTrimmable() { // Sealed segments are in baseSeq order, oldest first; once we hit one // that isn't fully acked, none of the later ones can be either. // Synchronized so the I/O thread's snapshotSealedSegments() can't - // race against the remove(0) shuffling slots underneath it. - while (sealedSegments.size() > 0) { - MmapSegment s = sealedSegments.get(0); + // race against the range removal shuffling slots underneath it. + // Collect the eligible prefix first, then remove it with a single + // range removal -- per-element remove(0) would shift the suffix on + // every iteration, making a bulk trim of S segments O(S^2). + int eligible = 0; + for (int i = 0, n = sealedSegments.size(); i < n; i++) { + MmapSegment s = sealedSegments.get(i); long lastSeq = s.baseSeq() + s.frameCount() - 1; if (lastSeq > acked) { break; @@ -584,7 +588,10 @@ public synchronized ObjList drainTrimmable() { out = new ObjList<>(); } out.add(s); - sealedSegments.remove(0); + eligible++; + } + if (eligible > 0) { + sealedSegments.remove(0, eligible - 1); } return out; } From db103845b690d4dd56ada7e215c7761278fec683 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Sat, 11 Jul 2026 03:02:31 +0100 Subject: [PATCH 10/12] Test deferred close caller fallbacks Add deterministic gray-box coverage for BackgroundDrainer and the cursor WebSocket send loop. The tests stall SegmentManager quiescence, verify callers return without releasing the slot lock, and confirm deferred cleanup releases the slot after quiescence resumes. Add narrow test-only seams for engine injection and I/O thread joining so the tests avoid a live server and wait for the complete exit path. --- .../client/sf/cursor/BackgroundDrainer.java | 16 +- .../sf/cursor/CursorWebSocketSendLoop.java | 9 + .../cursor/CursorDeferredCloseCallerTest.java | 349 ++++++++++++++++++ 3 files changed, 372 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorDeferredCloseCallerTest.java diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java index 566da28b..6452d395 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/BackgroundDrainer.java @@ -108,6 +108,7 @@ public final class BackgroundDrainer implements Runnable { private final String slotPath; /** Latest known {@code engine.ackedFsn()}; published for visibility. */ private volatile long ackedFsn = -1L; + private CursorSendEngine engineForTesting; private volatile String lastErrorMessage; /** * Optional observer for durable-ack-unavailable transients and the @@ -530,8 +531,10 @@ public void run() { // holds it, the engine constructor throws and we exit silently // (no .failed sentinel — contention is expected, not an error). try { - engine = new CursorSendEngine(slotPath, segmentSizeBytes, - sfMaxTotalBytes, CursorSendEngine.DEFAULT_APPEND_DEADLINE_NANOS); + engine = engineForTesting != null + ? engineForTesting + : new CursorSendEngine(slotPath, segmentSizeBytes, + sfMaxTotalBytes, CursorSendEngine.DEFAULT_APPEND_DEADLINE_NANOS); } catch (IllegalStateException t) { String msg = t.getMessage(); if (msg != null && msg.contains("already in use")) { @@ -725,6 +728,15 @@ public void run() { } } + /** + * Replaces the engine created by {@link #run()} so tests can drive the + * production teardown path with a controlled manager. + */ + @TestOnly + public void setEngineForTesting(CursorSendEngine engineForTesting) { + this.engineForTesting = engineForTesting; + } + /** * Plug an observer for durable-ack-related events. {@code null} clears * any previously installed listener. See {@link BackgroundDrainerListener} diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java index afb7189a..bde01ae7 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java @@ -842,6 +842,15 @@ public boolean delegateEngineClose() { return shutdownLatch.getCount() != 0L; } + /** + * Returns the I/O thread so lifecycle tests can wait for its exit path, + * which continues beyond the shutdown-latch countdown. + */ + @TestOnly + public Thread getIoThreadForTesting() { + return ioThread; + } + /** * Typed server-rejection payload of the latched terminal error, or * {@code null} when the loop latched a wire-level failure (or nothing). diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorDeferredCloseCallerTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorDeferredCloseCallerTest.java new file mode 100644 index 00000000..82a871aa --- /dev/null +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorDeferredCloseCallerTest.java @@ -0,0 +1,349 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2026 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.client.test.cutlass.qwp.client.sf.cursor; + +import io.questdb.client.DefaultHttpClientConfiguration; +import io.questdb.client.cutlass.http.client.WebSocketClient; +import io.questdb.client.cutlass.http.client.WebSocketFrameHandler; +import io.questdb.client.cutlass.qwp.client.sf.cursor.BackgroundDrainer; +import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; +import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SegmentManager; +import io.questdb.client.cutlass.qwp.client.sf.cursor.SlotLock; +import io.questdb.client.network.PlainSocketFactory; +import io.questdb.client.std.Files; +import io.questdb.client.std.MemoryTag; +import io.questdb.client.std.Unsafe; +import io.questdb.client.test.tools.TestUtils; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.nio.file.Paths; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Caller-level regressions for lifecycle paths that hand an incomplete + * {@link CursorSendEngine#close()} to the deferred close owner. The tests hold + * a real manager worker inside a service pass but replace the wire with an + * in-process stub, so they exercise production teardown without a server. + */ +public class CursorDeferredCloseCallerTest { + + private static final long SEGMENT_BYTES = 64L * 1024L; + private String tmpDir; + + @Before + public void setUp() { + tmpDir = Paths.get(System.getProperty("java.io.tmpdir"), + "qdb-cursor-deferred-caller-" + System.nanoTime()).toString(); + Assert.assertEquals(0, Files.mkdir(tmpDir, Files.DIR_MODE_DEFAULT)); + } + + @After + public void tearDown() { + if (tmpDir == null) { + return; + } + rmDirRecursive(tmpDir); + Files.remove(tmpDir); + } + + @Test(timeout = 30_000L) + public void testBackgroundDrainerDefersCloseWhenManagerQuiescenceStalls() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String slot = tmpDir + "/drainer-slot"; + SegmentManager manager = new SegmentManager(SEGMENT_BYTES, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch releaseWorker = new CountDownLatch(1); + CountDownLatch workerBlocked = new CountDownLatch(1); + AtomicBoolean hookFired = new AtomicBoolean(); + CursorSendEngine engine = null; + BackgroundDrainer drainer = null; + Thread drainerThread = null; + IdleWebSocketClient client = null; + boolean managerClosed = false; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!hookFired.compareAndSet(false, true)) { + return; + } + workerBlocked.countDown(); + try { + releaseWorker.await(20, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engine = new CursorSendEngine(slot, SEGMENT_BYTES, manager); + appendFrame(engine); + Assert.assertTrue("manager worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + manager.setWorkerJoinTimeoutMillis(50L); + + client = new IdleWebSocketClient(); + IdleWebSocketClient connectedClient = client; + drainer = new BackgroundDrainer( + slot, + SEGMENT_BYTES, + Long.MAX_VALUE, + () -> connectedClient, + 5_000L, + 10L, + 50L, + false, + 0L + ); + drainer.setEngineForTesting(engine); + drainerThread = new Thread(drainer, "qdb-drainer-deferred-close-test"); + drainerThread.setDaemon(true); + drainerThread.start(); + Assert.assertTrue("drainer send loop never started", + client.sendAttempted.await(5, TimeUnit.SECONDS)); + + long started = System.nanoTime(); + drainer.requestStop(); + drainerThread.join(5_000L); + Assert.assertFalse("drainer blocked on incomplete engine close", drainerThread.isAlive()); + Assert.assertTrue( + "drainer close fallback exceeded its bounded lifecycle window", + System.nanoTime() - started < TimeUnit.SECONDS.toNanos(1) + ); + Assert.assertEquals(BackgroundDrainer.DrainOutcome.STOPPED, drainer.outcome()); + Assert.assertFalse("stalled manager must keep engine close incomplete", + engine.isCloseCompleted()); + assertSlotLocked(slot); + + releaseWorker.countDown(); + awaitCloseCompleted(engine); + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after deferred drainer cleanup", probe); + } + + manager.close(); + managerClosed = true; + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (drainer != null) { + drainer.requestStop(); + } + if (drainerThread != null) { + drainerThread.join(5_000L); + } + if (client != null) { + client.close(); + } + if (engine != null) { + engine.close(); + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + + @Test(timeout = 30_000L) + public void testSendLoopDefersDelegatedCloseWhenManagerQuiescenceStalls() throws Exception { + TestUtils.assertMemoryLeak(() -> { + String slot = tmpDir + "/send-loop-slot"; + SegmentManager manager = new SegmentManager(SEGMENT_BYTES, TimeUnit.SECONDS.toNanos(60)); + CountDownLatch releaseWorker = new CountDownLatch(1); + CountDownLatch workerBlocked = new CountDownLatch(1); + AtomicBoolean hookFired = new AtomicBoolean(); + CursorSendEngine engine = null; + CursorWebSocketSendLoop loop = null; + IdleWebSocketClient client = null; + boolean managerClosed = false; + try { + manager.setBeforeInstallSyncHook(() -> { + if (!hookFired.compareAndSet(false, true)) { + return; + } + workerBlocked.countDown(); + try { + releaseWorker.await(20, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + manager.start(); + engine = new CursorSendEngine(slot, SEGMENT_BYTES, manager); + Assert.assertTrue("manager worker never reached the install hook", + workerBlocked.await(5, TimeUnit.SECONDS)); + manager.setWorkerJoinTimeoutMillis(50L); + + client = new IdleWebSocketClient(); + loop = new CursorWebSocketSendLoop( + client, + engine, + 0L, + CursorWebSocketSendLoop.DEFAULT_PARK_NANOS, + null, + 0L, + 0L, + 0L + ); + loop.start(); + Assert.assertTrue("send loop never entered its receive pass", + client.receiveAttempted.await(5, TimeUnit.SECONDS)); + Thread ioThread = loop.getIoThreadForTesting(); + Assert.assertNotNull("send loop did not publish its I/O thread", ioThread); + Assert.assertTrue("live send loop must adopt the delegated engine close", + loop.delegateEngineClose()); + + long started = System.nanoTime(); + loop.close(); + ioThread.join(5_000L); + Assert.assertFalse("send-loop exit path did not finish", ioThread.isAlive()); + Assert.assertTrue( + "send-loop close fallback exceeded its bounded lifecycle window", + System.nanoTime() - started < TimeUnit.SECONDS.toNanos(1) + ); + Assert.assertFalse("stalled manager must keep engine close incomplete", + engine.isCloseCompleted()); + assertSlotLocked(slot); + + releaseWorker.countDown(); + awaitCloseCompleted(engine); + try (SlotLock probe = SlotLock.acquire(slot)) { + Assert.assertNotNull("slot must be acquirable after deferred send-loop cleanup", probe); + } + + manager.close(); + managerClosed = true; + } finally { + manager.setBeforeInstallSyncHook(null); + releaseWorker.countDown(); + if (loop != null) { + loop.close(); + } + if (client != null) { + client.close(); + } + if (engine != null) { + engine.close(); + } + if (!managerClosed) { + manager.close(); + } + } + }); + } + + private static void appendFrame(CursorSendEngine engine) { + long buf = Unsafe.malloc(16, MemoryTag.NATIVE_DEFAULT); + try { + for (int i = 0; i < 16; i++) { + Unsafe.getUnsafe().putByte(buf + i, (byte) i); + } + Assert.assertEquals(0L, engine.appendBlocking(buf, 16)); + } finally { + Unsafe.free(buf, 16, MemoryTag.NATIVE_DEFAULT); + } + } + + private static void assertSlotLocked(String slot) { + try (SlotLock ignored = SlotLock.acquire(slot)) { + Assert.fail("incomplete close must retain the slot lock"); + } catch (IllegalStateException e) { + Assert.assertTrue(e.getMessage(), e.getMessage().contains("sf slot already in use")); + } + } + + private static void awaitCloseCompleted(CursorSendEngine engine) throws InterruptedException { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (!engine.isCloseCompleted() && System.nanoTime() < deadline) { + Thread.sleep(1L); + } + Assert.assertTrue("deferred close did not complete after manager quiescence", + engine.isCloseCompleted()); + } + + private static void rmDirRecursive(String dir) { + if (!Files.exists(dir)) { + return; + } + long find = Files.findFirst(dir); + if (find <= 0) { + return; + } + try { + int rc = 1; + while (rc > 0) { + String name = Files.utf8ToString(Files.findName(find)); + if (name != null && !".".equals(name) && !"..".equals(name)) { + String child = dir + "/" + name; + if (!Files.remove(child)) { + rmDirRecursive(child); + Files.remove(child); + } + } + rc = Files.findNext(find); + } + } finally { + Files.findClose(find); + } + } + + private static final class IdleWebSocketClient extends WebSocketClient { + private final CountDownLatch receiveAttempted = new CountDownLatch(1); + private final CountDownLatch sendAttempted = new CountDownLatch(1); + + private IdleWebSocketClient() { + super(DefaultHttpClientConfiguration.INSTANCE, PlainSocketFactory.INSTANCE); + } + + @Override + public void sendBinary(long dataPtr, int length) { + sendAttempted.countDown(); + } + + @Override + public void sendBinary(long dataPtr, int length, int timeout) { + sendAttempted.countDown(); + } + + @Override + public boolean tryReceiveFrame(WebSocketFrameHandler handler) { + receiveAttempted.countDown(); + return false; + } + + @Override + protected void ioWait(int timeout, int op) { + throw new UnsupportedOperationException("stub: no socket"); + } + + @Override + protected void setupIoWait() { + // no-op + } + } +} From dde3c15d8f468156026a33376d93893f8022c9e2 Mon Sep 17 00:00:00 2001 From: Vlad Ilyushchenko Date: Fri, 24 Jul 2026 15:06:34 +0100 Subject: [PATCH 11/12] Deflake close interrupt lifecycle tests The lifecycle tests inferred that close had left its creation wait whenever ReentrantLock.hasWaiters() briefly returned false. Interrupt delivery moves a condition waiter to the lock queue while awaitNanos() reacquires the lock, so the observer could mistake that transition for deadline expiry. Add test-only retry hooks that fire only after each pool catches an interrupt and confirms the original deadline still permits another wait. The tests now send each next interrupt after that acknowledgement, preventing coalescing and removing the transient queue-membership check while retaining deadline-restart coverage. --- .../questdb/client/impl/QueryClientPool.java | 18 +++ .../io/questdb/client/impl/SenderPool.java | 18 +++ .../impl/QuestDBImplCloseLifecycleTest.java | 150 ++++++++---------- 3 files changed, 106 insertions(+), 80 deletions(-) diff --git a/core/src/main/java/io/questdb/client/impl/QueryClientPool.java b/core/src/main/java/io/questdb/client/impl/QueryClientPool.java index 75323a12..ca3aec15 100644 --- a/core/src/main/java/io/questdb/client/impl/QueryClientPool.java +++ b/core/src/main/java/io/questdb/client/impl/QueryClientPool.java @@ -97,6 +97,11 @@ public final class QueryClientPool implements AutoCloseable { // DEFAULT_CLOSE_QUERY_TIMEOUT_MILLIS. Volatile because QuestDBImpl sets it // once at build time on a different thread than the borrowers that read it. private volatile long closeQueryTimeoutMillis = DEFAULT_CLOSE_QUERY_TIMEOUT_MILLIS; + // Test seam invoked after close() handles an interrupt and confirms the + // original creation-wait deadline still permits another wait. Null in + // production; lifecycle tests use it to acknowledge distinct retries + // without inspecting transient Condition queue membership. + private volatile Runnable creationWaitRetryHook; private int inFlightCreations; public QueryClientPool( @@ -340,12 +345,20 @@ public void close() { long creationRemainingNanos = creationWaitNanos; boolean creationWaitInterrupted = false; while (inFlightCreations > 0 && creationRemainingNanos > 0) { + boolean isRetryingAfterInterrupt = false; try { creationFinished.awaitNanos(creationRemainingNanos); } catch (InterruptedException e) { creationWaitInterrupted = true; + isRetryingAfterInterrupt = true; } creationRemainingNanos = creationWaitDeadlineNanos - System.nanoTime(); + if (isRetryingAfterInterrupt && inFlightCreations > 0 && creationRemainingNanos > 0) { + Runnable hook = creationWaitRetryHook; + if (hook != null) { + hook.run(); + } + } } if (creationWaitInterrupted) { Thread.currentThread().interrupt(); @@ -557,6 +570,11 @@ public boolean isClosedForTesting() { return closed; } + @TestOnly + public void setCreationWaitRetryHookForTesting(Runnable hook) { + this.creationWaitRetryHook = hook; + } + private QueryWorker createUnlocked() { QwpQueryClient client = QwpQueryClient.fromConfig(configurationString); try { diff --git a/core/src/main/java/io/questdb/client/impl/SenderPool.java b/core/src/main/java/io/questdb/client/impl/SenderPool.java index 2d01059e..91ed7019 100644 --- a/core/src/main/java/io/questdb/client/impl/SenderPool.java +++ b/core/src/main/java/io/questdb/client/impl/SenderPool.java @@ -222,6 +222,11 @@ public final class SenderPool implements AutoCloseable { // production; regression tests release a retired slot here to prove that // the terminal pass re-probes returned capacity before throwing. private volatile Runnable borrowWaitExpiredHook; + // Test seam invoked after close() handles an interrupt and confirms the + // original creation-wait deadline still permits another wait. Null in + // production; lifecycle tests use it to acknowledge distinct retries + // without inspecting transient Condition queue membership. + private volatile Runnable creationWaitRetryHook; // Slots removed from `all` whose delegate is still releasing its flock. // They keep reserving capacity (and their slotInUse mark) until the // flock drops, so the cap check and the slot allocator stay consistent @@ -1410,6 +1415,11 @@ public void setBorrowWaitExpiredHook(Runnable hook) { this.borrowWaitExpiredHook = hook; } + @TestOnly + public void setCreationWaitRetryHookForTesting(Runnable hook) { + this.creationWaitRetryHook = hook; + } + /** * Raises the shutdown signal early -- without tearing down live delegates -- * so an in-flight startup-recovery step stops promptly between slots. Direct @@ -1484,12 +1494,20 @@ public void close() { long creationRemainingNanos = creationWaitNanos; boolean creationWaitInterrupted = false; while (inFlightCreations > 0 && creationRemainingNanos > 0) { + boolean isRetryingAfterInterrupt = false; try { creationFinished.awaitNanos(creationRemainingNanos); } catch (InterruptedException e) { creationWaitInterrupted = true; + isRetryingAfterInterrupt = true; } creationRemainingNanos = creationWaitDeadlineNanos - System.nanoTime(); + if (isRetryingAfterInterrupt && inFlightCreations > 0 && creationRemainingNanos > 0) { + Runnable hook = creationWaitRetryHook; + if (hook != null) { + hook.run(); + } + } } if (creationWaitInterrupted) { Thread.currentThread().interrupt(); diff --git a/core/src/test/java/io/questdb/client/test/impl/QuestDBImplCloseLifecycleTest.java b/core/src/test/java/io/questdb/client/test/impl/QuestDBImplCloseLifecycleTest.java index 6344336b..3a92e25e 100644 --- a/core/src/test/java/io/questdb/client/test/impl/QuestDBImplCloseLifecycleTest.java +++ b/core/src/test/java/io/questdb/client/test/impl/QuestDBImplCloseLifecycleTest.java @@ -39,11 +39,11 @@ import java.lang.reflect.Proxy; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.BooleanSupplier; import java.util.function.Consumer; import java.util.function.IntFunction; @@ -186,18 +186,21 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr inCreation.countDown(); awaitOrFail(releaseCreation, "test never released query creation"); }; - // A 1s creation-wait budget, not 100ms: the interrupt storm below must land at least - // twice inside this window for the deadline-restart property to be exercised at all, - // and a freshly started, yielding interrupter thread is not guaranteed two scheduler - // quanta within 100ms on a saturated CI agent (observed on hosted 3-core mac agents, - // where the post-join count assert failed with the product deadline honored exactly). + // A 1s creation-wait budget gives the closer scheduler margin. Each next interrupt + // is sent only after the pool acknowledges that it caught the previous one and will + // retry against the original deadline, so interrupts cannot coalesce. QuestDBImpl db = newQuestDB( SENDER_CFG, 0, 0, 1000, slotIndex -> fakeSender(null, null, null), connectHook); QueryClientPool pool = db.getQueryPoolForTesting(); AtomicReference borrowOutcome = new AtomicReference<>(); - AtomicBoolean closeReturnedInterrupted = new AtomicBoolean(); - AtomicBoolean keepInterrupting = new AtomicBoolean(true); - AtomicInteger interruptCount = new AtomicInteger(); + AtomicBoolean isCloseComplete = new AtomicBoolean(); + AtomicBoolean isCloseReturnedInterrupted = new AtomicBoolean(); + AtomicInteger interruptRetryCount = new AtomicInteger(); + Semaphore closeProgress = new Semaphore(0); + pool.setCreationWaitRetryHookForTesting(() -> { + interruptRetryCount.incrementAndGet(); + closeProgress.release(); + }); Thread borrower = new Thread(() -> { try { db.borrowQuery(); @@ -206,16 +209,14 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr } }, "interrupted-query-borrower"); Thread closer = new Thread(() -> { - db.close(); - closeReturnedInterrupted.set(Thread.currentThread().isInterrupted()); - }, "interrupted-query-closer"); - Thread interrupter = new Thread(() -> { - while (keepInterrupting.get()) { - interruptCount.incrementAndGet(); - closer.interrupt(); - Thread.yield(); + try { + db.close(); + isCloseReturnedInterrupted.set(Thread.currentThread().isInterrupted()); + } finally { + isCloseComplete.set(true); + closeProgress.release(); } - }, "query-close-interrupter"); + }, "interrupted-query-closer"); long nativeBaseline = Unsafe.getMemUsedByTag(MemoryTag.NATIVE_DEFAULT); try { borrower.start(); @@ -226,16 +227,15 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr closer.start(); awaitCreationWaiter(pool, "facade close did not wait while query construction was internally owned"); - interrupter.start(); - awaitRepeatedInterrupts(interruptCount, pool::hasCreationWaiterForTesting, - "query close left its creation wait before the interrupt storm landed twice"); - closer.join(TimeUnit.SECONDS.toMillis(5)); - Assert.assertFalse( + Assert.assertTrue( "repeated interrupts restarted the query creation-wait deadline", - closer.isAlive()); - Assert.assertTrue("test did not repeatedly interrupt query close", interruptCount.get() > 1); + interruptUntilClose(closer, closeProgress, isCloseComplete)); + closer.join(TimeUnit.SECONDS.toMillis(5)); + Assert.assertFalse("facade query closer did not terminate", closer.isAlive()); + Assert.assertTrue("test did not observe repeated query close interrupt retries", + interruptRetryCount.get() > 1); Assert.assertTrue("facade close must restore query closer interruption", - closeReturnedInterrupted.get()); + isCloseReturnedInterrupted.get()); Assert.assertEquals( "close must retain late-completion cleanup ownership", 1, pool.inFlightCreations()); @@ -252,9 +252,7 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringQueryCreation() thr borrowOutcome.get() instanceof QueryException && String.valueOf(borrowOutcome.get().getMessage()).contains("closed")); } finally { - keepInterrupting.set(false); releaseCreation.countDown(); - interrupter.join(TimeUnit.SECONDS.toMillis(10)); db.close(); borrower.join(TimeUnit.SECONDS.toMillis(10)); closer.join(TimeUnit.SECONDS.toMillis(10)); @@ -275,15 +273,20 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th }; String senderConfig = "ws::addr=localhost:1;sf_dir=" + System.getProperty("java.io.tmpdir") + "/qdb-interrupted-pool-" + System.nanoTime() + ";"; - // 1s creation-wait budget for the same reason as the query-interrupt test above: the - // interrupt storm must land at least twice inside the window even on a saturated agent. + // 1s creation-wait budget for the same acknowledged-interrupt retry protocol as the + // query test above. QuestDBImpl db = newQuestDB(senderConfig, 0, 0, 1000, senderFactory, client -> { }); SenderPool pool = db.getSenderPoolForTesting(); AtomicReference borrowOutcome = new AtomicReference<>(); - AtomicBoolean closeReturnedInterrupted = new AtomicBoolean(); - AtomicBoolean keepInterrupting = new AtomicBoolean(true); - AtomicInteger interruptCount = new AtomicInteger(); + AtomicBoolean isCloseComplete = new AtomicBoolean(); + AtomicBoolean isCloseReturnedInterrupted = new AtomicBoolean(); + AtomicInteger interruptRetryCount = new AtomicInteger(); + Semaphore closeProgress = new Semaphore(0); + pool.setCreationWaitRetryHookForTesting(() -> { + interruptRetryCount.incrementAndGet(); + closeProgress.release(); + }); Thread borrower = new Thread(() -> { try { db.borrowSender(); @@ -292,16 +295,14 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th } }, "interrupted-sender-borrower"); Thread closer = new Thread(() -> { - db.close(); - closeReturnedInterrupted.set(Thread.currentThread().isInterrupted()); - }, "interrupted-sender-closer"); - Thread interrupter = new Thread(() -> { - while (keepInterrupting.get()) { - interruptCount.incrementAndGet(); - closer.interrupt(); - Thread.yield(); + try { + db.close(); + isCloseReturnedInterrupted.set(Thread.currentThread().isInterrupted()); + } finally { + isCloseComplete.set(true); + closeProgress.release(); } - }, "sender-close-interrupter"); + }, "interrupted-sender-closer"); try { borrower.start(); Assert.assertTrue("sender borrow never reached construction", @@ -313,16 +314,15 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th closer.start(); awaitCreationWaiter(pool, "facade close did not wait while sender construction was internally owned"); - interrupter.start(); - awaitRepeatedInterrupts(interruptCount, pool::hasCreationWaiterForTesting, - "sender close left its creation wait before the interrupt storm landed twice"); - closer.join(TimeUnit.SECONDS.toMillis(5)); - Assert.assertFalse( + Assert.assertTrue( "repeated interrupts restarted the sender creation-wait deadline", - closer.isAlive()); - Assert.assertTrue("test did not repeatedly interrupt sender close", interruptCount.get() > 1); + interruptUntilClose(closer, closeProgress, isCloseComplete)); + closer.join(TimeUnit.SECONDS.toMillis(5)); + Assert.assertFalse("facade sender closer did not terminate", closer.isAlive()); + Assert.assertTrue("test did not observe repeated sender close interrupt retries", + interruptRetryCount.get() > 1); Assert.assertTrue("facade close must restore sender closer interruption", - closeReturnedInterrupted.get()); + isCloseReturnedInterrupted.get()); Assert.assertEquals( "close must retain late-completion cleanup ownership", 1, pool.getInFlightCreationsForTesting()); @@ -341,9 +341,7 @@ public void facadeCloseIsBoundedUnderRepeatedInterruptsDuringSenderCreation() th borrowOutcome.get() instanceof LineSenderException && String.valueOf(borrowOutcome.get().getMessage()).contains("closed")); } finally { - keepInterrupting.set(false); releaseCreation.countDown(); - interrupter.join(TimeUnit.SECONDS.toMillis(10)); db.close(); borrower.join(TimeUnit.SECONDS.toMillis(10)); closer.join(TimeUnit.SECONDS.toMillis(10)); @@ -529,34 +527,6 @@ private static void awaitCreationWaiter(SenderPool pool, String message) { Assert.fail(message); } - /** - * Holds the test until the interrupt storm has landed at least twice while the facade close is - * still inside its bounded creation wait. The deadline-restart property is only exercised by - * interrupts that arrive during that wait, and the scheduler owes the interrupter thread - * nothing: with a post-join count assert alone, the run races the close budget against thread - * scheduling and can fail with the product invariant intact. Failing here instead separates - * "interrupter starved before the budget expired" from a genuine deadline bug. - */ - private static void awaitRepeatedInterrupts( - AtomicInteger interruptCount, - BooleanSupplier closerStillWaiting, - String message - ) { - long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); - while (System.nanoTime() < deadline) { - // Count first: two interrupts observed while polling means the storm landed no matter - // how quickly the wait ends afterwards, so a budget expiry seen next is not a failure. - if (interruptCount.get() > 1) { - return; - } - if (!closerStillWaiting.getAsBoolean()) { - Assert.fail(message + "; interrupts landed: " + interruptCount.get()); - } - Thread.yield(); - } - Assert.fail(message + "; interrupts landed: " + interruptCount.get()); - } - private static void awaitOrFail(CountDownLatch latch, String message) { try { if (!latch.await(10, TimeUnit.SECONDS)) { @@ -610,6 +580,26 @@ private static Sender fakeSender( }); } + private static boolean interruptUntilClose( + Thread closer, + Semaphore closeProgress, + AtomicBoolean isCloseComplete + ) throws InterruptedException { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + closer.interrupt(); + while (!isCloseComplete.get()) { + long remainingNanos = deadline - System.nanoTime(); + if (remainingNanos <= 0 + || !closeProgress.tryAcquire(remainingNanos, TimeUnit.NANOSECONDS)) { + return false; + } + if (!isCloseComplete.get()) { + closer.interrupt(); + } + } + return true; + } + private static QuestDBImpl newQuestDB( int senderMin, int queryMin, From 7ce915a28b869a888983f128d66f311d49545923 Mon Sep 17 00:00:00 2001 From: bluestreak Date: Mon, 27 Jul 2026 09:57:30 +0100 Subject: [PATCH 12/12] Drop the ROLE_CHANGE (4001) close code Reverts 9f3cb329. The server keeps NORMAL_CLOSURE on the role-change CLOSE, so there is no 4001 on the wire for this client to classify. 4001 existed so the client's verbatim CLOSE echo would prove to the server that it had read the server's CLOSE rather than having sent a voluntary CLOSE that crossed it. The server takes identical action on both branches -- the code only selected a log line -- while every client released before 9f3cb329 treats a code outside NORMAL_CLOSURE/GOING_AWAY as a head-of-line poison strike, escalating a routine demote to a PROTOCOL_VIOLATION terminal that quarantines the store-and-forward slot. Putting the code on the wire therefore costs data on mixed-version fleets and buys a diagnostic; it needs a negotiated capability first. Carrying the classification here anyway would leave the client claiming to understand a code nothing sends, and the poison-frame test would pin a contract with no counterparty. Strip both, and record in qwp-nack-policy-v2.md that a distinguishable role-change code is gated behind capability negotiation rather than simply absent. --- .../sf/cursor/CursorWebSocketSendLoop.java | 13 +++-- .../qwp/websocket/WebSocketCloseCode.java | 12 ----- ...ursorWebSocketSendLoopPoisonFrameTest.java | 48 ------------------- design/qwp-nack-policy-v2.md | 17 +++---- 4 files changed, 15 insertions(+), 75 deletions(-) diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java index 4c98b0a3..58c3d31e 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoop.java @@ -357,7 +357,7 @@ public final class CursorWebSocketSendLoop implements QuietCloseable { private long highestOkFsn = -1L; // Zero-progress recycle pacer (I/O thread only; survives reconnects). // Counts consecutive strike-EXEMPT recycles -- orderly closes - // (NORMAL_CLOSURE/GOING_AWAY/ROLE_CHANGE), non-orderly closes before any send on the + // (NORMAL_CLOSURE/GOING_AWAY), non-orderly closes before any send on the // connection, and pre-send RETRIABLE_OTHER rejections -- with no // acceptance progress in between. These paths deliberately carry no // poison strike (they are not a verdict on the bytes), which also exempts @@ -1629,7 +1629,7 @@ private void failPaced(Throwable initial) { /** * Recycle path for strike-exempt wire events: orderly closes - * (NORMAL_CLOSURE / GOING_AWAY / ROLE_CHANGE), non-orderly closes before any send on + * (NORMAL_CLOSURE / GOING_AWAY), non-orderly closes before any send on * the connection, and RETRIABLE_OTHER rejections (pre- and post-send: * NOT_WRITABLE is a node-state verdict, not a frame verdict). None of * these implicate the head frame, so they carry no poison strike -- but that @@ -2394,12 +2394,11 @@ public void onClose(int code, String reason) { // after this connection already sent the head frame counts a poison // strike; maxHeadFrameRejections consecutive strikes at the same // head FSN with no ack progress escalate to a typed terminal. Orderly - // closes (ROLE_CHANGE role-change handoff, NORMAL_CLOSURE, GOING_AWAY - // restart drain) never count strikes — they are the server asking us - // to go elsewhere, not a verdict on the bytes. + // closes (NORMAL_CLOSURE role-change handoff, GOING_AWAY restart + // drain) never count strikes — they are the server asking us to go + // elsewhere, not a verdict on the bytes. boolean orderly = code == WebSocketCloseCode.NORMAL_CLOSURE - || code == WebSocketCloseCode.GOING_AWAY - || code == WebSocketCloseCode.ROLE_CHANGE; + || code == WebSocketCloseCode.GOING_AWAY; LineSenderException cause = new LineSenderException( "WebSocket closed by server: code=" + code + " reason=" + reason); if (!orderly && nextWireSeq > 0) { diff --git a/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java b/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java index 01d58101..3a86bf6e 100644 --- a/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java +++ b/core/src/main/java/io/questdb/client/cutlass/qwp/websocket/WebSocketCloseCode.java @@ -84,16 +84,6 @@ public final class WebSocketCloseCode { * Reserved for future use. */ public static final int RESERVED = 1004; - /** - * Role-change close (4001). QWP application-defined code in the RFC 6455 - * Section 7.4.2 private-use range: the server closed because its role - * changed (primary demoted); reconnect-eligible, not a verdict on the - * bytes. Deliberately distinct from {@link #NORMAL_CLOSURE} so the - * client's verbatim CLOSE echo proves to the server that the client - * received the server's CLOSE (and, by TCP ordering, everything before - * it -- the final durable ack included). - */ - public static final int ROLE_CHANGE = 4001; /** * TLS handshake (1015). * Reserved value. MUST NOT be sent in a Close frame. @@ -144,8 +134,6 @@ public static String describe(int code) { return "Internal Error"; case TLS_HANDSHAKE: return "TLS Handshake"; - case ROLE_CHANGE: - return "Role Change (QWP)"; default: if (code >= 3000 && code < 4000) { return "Library/Framework Code (" + code + ")"; diff --git a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java index cc395861..34ae5518 100644 --- a/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java +++ b/core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/CursorWebSocketSendLoopPoisonFrameTest.java @@ -32,7 +32,6 @@ import io.questdb.client.cutlass.qwp.client.WebSocketResponse; import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine; import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorWebSocketSendLoop; -import io.questdb.client.cutlass.qwp.websocket.WebSocketCloseCode; import io.questdb.client.network.PlainSocketFactory; import io.questdb.client.std.Files; import io.questdb.client.std.MemoryTag; @@ -245,43 +244,6 @@ public void testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine() throws Exception }); } - @Test - public void testRoleChangeCloseIsOrderlyNeverCountsStrikes() throws Exception { - // The server's role-change close (ROLE_CHANGE, 4001) is the demote - // handoff: "go elsewhere", not a verdict on the bytes. Like - // NORMAL_CLOSURE and GOING_AWAY it must never count a poison strike. - // Dropping ROLE_CHANGE from the orderly set would pass the rest of - // this suite while latching a PROTOCOL_VIOLATION terminal after - // maxHeadFrameRejections demote closes at an unadvanced head FSN -- - // turning a routine failover into a false data-poison verdict. - // Mirrors testNonOrderlyClosePoisonKeysOnOkLevelHeadOfLine with the - // opposite expectation. - TestUtils.assertMemoryLeak(() -> { - List clients = new ArrayList<>(); - try (CursorSendEngine engine = newEngine()) { - appendFrames(engine, 2); - CursorWebSocketSendLoop loop = newDurableLoop(engine, clients); - setSentCount(loop, 2); - - deliverOk(loop, 0, names("trades"), txns(7L)); - for (int i = 0; i < MAX_REJECTIONS + 1; i++) { - // Role-change close after at least one send on this - // connection: strike-exempt, so no number of repeats may - // escalate. Restore the sent count after each recycle, - // exactly like the non-orderly variant. - setSentCount(loop, 2); - deliverRoleChangeClose(loop); - } - - // Must NOT throw: orderly closes never escalate to a typed - // terminal, however many accumulate at the same head FSN. - loop.checkError(); - } finally { - closeAll(clients); - } - }); - } - @Test public void testNackRecycleIsPacedAgainstHealthyServer() throws Exception { // A reachable, healthy server that NACKs the head frame (RETRIABLE) @@ -945,16 +907,6 @@ private static void deliverOrderlyClose(CursorWebSocketSendLoop loop) throws Exc m.invoke(handler, 1001, "server draining"); // GOING_AWAY: orderly, strike-exempt } - private static void deliverRoleChangeClose(CursorWebSocketSendLoop loop) throws Exception { - Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler"); - f.setAccessible(true); - Object handler = f.get(loop); - Method m = handler.getClass().getDeclaredMethod("onClose", int.class, String.class); - m.setAccessible(true); - // ROLE_CHANGE (4001): the demote handoff, orderly, strike-exempt - m.invoke(handler, WebSocketCloseCode.ROLE_CHANGE, "role change: primary demoted"); - } - private static void deliverNonOrderlyClose(CursorWebSocketSendLoop loop) throws Exception { Field f = CursorWebSocketSendLoop.class.getDeclaredField("responseHandler"); f.setAccessible(true); diff --git a/design/qwp-nack-policy-v2.md b/design/qwp-nack-policy-v2.md index 85ea7c30..2a9ff891 100644 --- a/design/qwp-nack-policy-v2.md +++ b/design/qwp-nack-policy-v2.md @@ -66,9 +66,8 @@ is caught *behaviorally*: > in durable-ack mode the trim watermark advances only on durable coverage, > so every post-NACK recycle replays from the durable watermark and re-OKs > frames *behind* the suspect — those re-OKs say nothing about the poisoned -> bytes and must not launder the count. Orderly closes (`ROLE_CHANGE` (4001) -> role-change handoff, `NORMAL_CLOSURE`, `GOING_AWAY` restart drain) never -> count strikes. +> bytes and must not launder the count. Orderly closes (`NORMAL_CLOSURE` +> role-change handoff, `GOING_AWAY` restart drain) never count strikes. Below the escalation threshold, a RETRIABLE NACK's recycle is **paced**: the server is reachable (it just answered), so the reconnect succeeds immediately @@ -100,11 +99,13 @@ diagnostics only. The server already handles it at the right layer (Invariant B work): the read-only gate and the commit-path authorization refusal both set -`roleChangeClosePending` and close with a reconnect-eligible `ROLE_CHANGE` -(4001, private-use range; distinct from `NORMAL_CLOSURE` so the client's -verbatim CLOSE echo is distinguishable from a voluntary client CLOSE that -crossed the server's CLOSE on the wire) instead of NACKing `SECURITY_ERROR` -(`QwpIngressProcessorState`). The client +`roleChangeClosePending` and close with a reconnect-eligible +`NORMAL_CLOSURE` instead of NACKing `SECURITY_ERROR` +(`QwpIngressProcessorState`). A private-use code would let the server tell +the client's verbatim CLOSE echo apart from a voluntary client CLOSE that +crossed it on the wire, but deployed fleets classify anything outside +`NORMAL_CLOSURE`/`GOING_AWAY` as a poison strike, so that needs a +negotiated capability first. The client reconnects, hits the 421 role reject on the now-replica, and retries from SF until a primary is reachable. Consequently: