Skip to content

Commit 358005c

Browse files
committed
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.
1 parent b50529d commit 358005c

2 files changed

Lines changed: 158 additions & 2 deletions

File tree

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,15 @@ public final class SegmentManager implements QuietCloseable {
8686
private final ObjList<RingEntry> ringSnapshot = new ObjList<>();
8787
private final ObjList<RingEntry> rings = new ObjList<>();
8888
private final long segmentSizeBytes;
89+
// Test seam: runs on the closer thread inside close(), after the pending
90+
// caller interrupt has been cleared and immediately before EACH join
91+
// attempt on the worker thread. Null in production.
92+
// SegmentManagerCloseRaceTest uses it to positively observe (a) that the
93+
// closer reached the join path with a cleared interrupt while the worker
94+
// was still live, and (b) that an interrupt delivered mid-join loops back
95+
// into another join attempt instead of abandoning the reap — without any
96+
// wall-clock coordination.
97+
private volatile Runnable beforeJoinAttemptHook;
8998
// Test seam: runs on the worker thread just before the install path's
9099
// synchronized(lock) entry (the one that performs installHotSpare + the
91100
// totalBytes += segmentSize commit). Null in production; tests use it to
@@ -201,6 +210,10 @@ public synchronized void close() {
201210
if (remainingMillis <= 0) {
202211
break;
203212
}
213+
Runnable joinHook = beforeJoinAttemptHook;
214+
if (joinHook != null) {
215+
joinHook.run();
216+
}
204217
try {
205218
t.join(remainingMillis);
206219
} catch (InterruptedException ignored) {
@@ -405,6 +418,11 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) {
405418
wakeWorker();
406419
}
407420

421+
@TestOnly
422+
public void setBeforeJoinAttemptHook(Runnable hook) {
423+
this.beforeJoinAttemptHook = hook;
424+
}
425+
408426
@TestOnly
409427
public void setBeforeInstallSyncHook(Runnable hook) {
410428
this.beforeInstallSyncHook = hook;

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentManagerCloseRaceTest.java

Lines changed: 140 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,17 @@ public void testAwaitRingQuiescenceBlocksWhileServicePassInFlight() throws Excep
279279
});
280280
}
281281

282+
/**
283+
* A closer arriving with a pending interrupt must clear it, reach the
284+
* join path, and reap the worker — not throw out of join() immediately
285+
* and abandon a live worker that still owns segment files.
286+
* <p>
287+
* Deterministic: the before-join-attempt hook is the positive signal
288+
* that the closer reached the join with (a) the interrupt cleared and
289+
* (b) the worker still live. No wall-clock "close didn't return within
290+
* N ms" inversion — that formulation passes with the fix reverted
291+
* whenever the closer is descheduled before the join.
292+
*/
282293
@Test(timeout = 15_000L)
283294
public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception {
284295
TestUtils.assertMemoryLeak(() -> {
@@ -291,7 +302,10 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception
291302
CountDownLatch workerBlocked = new CountDownLatch(1);
292303
CountDownLatch releaseWorker = new CountDownLatch(1);
293304
CountDownLatch closeReturned = new CountDownLatch(1);
305+
CountDownLatch closerAtJoin = new CountDownLatch(1);
294306
AtomicBoolean fired = new AtomicBoolean();
307+
AtomicBoolean interruptClearAtJoin = new AtomicBoolean();
308+
AtomicBoolean workerAliveAtJoin = new AtomicBoolean();
295309
AtomicBoolean interruptPreserved = new AtomicBoolean();
296310
AtomicReference<Throwable> hookErr = new AtomicReference<>();
297311
AtomicReference<Throwable> closeErr = new AtomicReference<>();
@@ -310,6 +324,17 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception
310324
hookErr.compareAndSet(null, t);
311325
}
312326
});
327+
manager.setBeforeJoinAttemptHook(() -> {
328+
if (closerAtJoin.getCount() == 0) return;
329+
interruptClearAtJoin.set(!Thread.currentThread().isInterrupted());
330+
try {
331+
Thread w = readWorkerThread(manager);
332+
workerAliveAtJoin.set(w != null && w.isAlive());
333+
} catch (Throwable t) {
334+
hookErr.compareAndSet(null, t);
335+
}
336+
closerAtJoin.countDown();
337+
});
313338
manager.start();
314339
Assert.assertTrue("worker did not reach install hook",
315340
workerBlocked.await(5, TimeUnit.SECONDS));
@@ -327,8 +352,17 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception
327352
}, "interrupted-closer");
328353
closer.start();
329354

330-
Assert.assertFalse("interrupted close() abandoned a live worker instead of waiting",
331-
closeReturned.await(300, TimeUnit.MILLISECONDS));
355+
// Positive proof the closer reached the join path: the hook
356+
// fired. With the fix reverted (immediate InterruptedException
357+
// abandon, or no interrupt-clearing), either the hook records
358+
// a still-set interrupt or the final reap assertions fail.
359+
Assert.assertTrue("closer never reached the join path",
360+
closerAtJoin.await(5, TimeUnit.SECONDS));
361+
Assert.assertTrue("the pending caller interrupt must be cleared before the join "
362+
+ "(otherwise join() throws immediately and the worker is abandoned)",
363+
interruptClearAtJoin.get());
364+
Assert.assertTrue("worker must still be live when the closer starts joining",
365+
workerAliveAtJoin.get());
332366

333367
releaseWorker.countDown();
334368
Assert.assertTrue("close() never returned after the worker was released",
@@ -348,6 +382,110 @@ public void testInterruptedCallerDoesNotAbandonReapableWorker() throws Exception
348382
}
349383
} finally {
350384
manager.setBeforeInstallSyncHook(null);
385+
manager.setBeforeJoinAttemptHook(null);
386+
releaseWorker.countDown();
387+
if (!managerClosed) {
388+
Thread.interrupted();
389+
manager.close();
390+
}
391+
ring.close();
392+
}
393+
});
394+
}
395+
396+
/**
397+
* The "interrupt arrives DURING the join" branch of the close() loop: an
398+
* InterruptedException thrown out of {@code t.join()} must mark the
399+
* interrupt for restoration and loop back into another join attempt —
400+
* not abandon the reap.
401+
* <p>
402+
* Deterministic: the second before-join-attempt hook invocation is the
403+
* positive proof that the loop retried after the mid-join interrupt.
404+
* With the retry loop reverted (single join, abandon on interrupt) the
405+
* second invocation never happens and the await fails.
406+
*/
407+
@Test(timeout = 15_000L)
408+
public void testInterruptDuringJoinRetriesUntilWorkerReaped() throws Exception {
409+
TestUtils.assertMemoryLeak(() -> {
410+
long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + 32);
411+
String slot = tmpDir + "/mid-join-interrupt-slot";
412+
Assert.assertEquals(0, Files.mkdir(slot, Files.DIR_MODE_DEFAULT));
413+
MmapSegment initial = MmapSegment.create(slot + "/sf-initial.sfa", 0L, segSize);
414+
SegmentRing ring = new SegmentRing(initial, segSize);
415+
SegmentManager manager = new SegmentManager(segSize, TimeUnit.SECONDS.toNanos(60));
416+
CountDownLatch workerBlocked = new CountDownLatch(1);
417+
CountDownLatch releaseWorker = new CountDownLatch(1);
418+
CountDownLatch closeReturned = new CountDownLatch(1);
419+
CountDownLatch firstJoinAttempt = new CountDownLatch(1);
420+
CountDownLatch secondJoinAttempt = new CountDownLatch(2);
421+
AtomicBoolean fired = new AtomicBoolean();
422+
AtomicBoolean interruptPreserved = new AtomicBoolean();
423+
AtomicReference<Throwable> hookErr = new AtomicReference<>();
424+
AtomicReference<Throwable> closeErr = new AtomicReference<>();
425+
boolean managerClosed = false;
426+
try {
427+
manager.register(ring, slot);
428+
manager.setBeforeInstallSyncHook(() -> {
429+
if (!fired.compareAndSet(false, true)) return;
430+
workerBlocked.countDown();
431+
try {
432+
if (!releaseWorker.await(10, TimeUnit.SECONDS)) {
433+
hookErr.compareAndSet(null,
434+
new AssertionError("timed out waiting for test to release worker"));
435+
}
436+
} catch (Throwable t) {
437+
hookErr.compareAndSet(null, t);
438+
}
439+
});
440+
manager.setBeforeJoinAttemptHook(() -> {
441+
firstJoinAttempt.countDown();
442+
secondJoinAttempt.countDown();
443+
});
444+
manager.start();
445+
Assert.assertTrue("worker did not reach install hook",
446+
workerBlocked.await(5, TimeUnit.SECONDS));
447+
448+
Thread closer = new Thread(() -> {
449+
try {
450+
manager.close();
451+
} catch (Throwable t) {
452+
closeErr.compareAndSet(null, t);
453+
} finally {
454+
interruptPreserved.set(Thread.currentThread().isInterrupted());
455+
closeReturned.countDown();
456+
}
457+
}, "mid-join-interrupted-closer");
458+
closer.start();
459+
460+
Assert.assertTrue("closer never reached the first join attempt",
461+
firstJoinAttempt.await(5, TimeUnit.SECONDS));
462+
// The worker is still parked in the install hook, so the
463+
// join cannot return normally: this interrupt must surface
464+
// as an InterruptedException inside (or on entry to) join.
465+
closer.interrupt();
466+
Assert.assertTrue("close() abandoned the reap after a mid-join interrupt "
467+
+ "instead of looping back into another join attempt",
468+
secondJoinAttempt.await(5, TimeUnit.SECONDS));
469+
470+
releaseWorker.countDown();
471+
Assert.assertTrue("close() never returned after the worker was released",
472+
closeReturned.await(10, TimeUnit.SECONDS));
473+
closer.join(TimeUnit.SECONDS.toMillis(5));
474+
managerClosed = readWorkerThread(manager) == null;
475+
476+
if (closeErr.get() != null) {
477+
throw new AssertionError("close() threw", closeErr.get());
478+
}
479+
Assert.assertNull("close() must reap the worker despite the mid-join interrupt",
480+
readWorkerThread(manager));
481+
Assert.assertTrue("close() must restore the interrupt that arrived during the join",
482+
interruptPreserved.get());
483+
if (hookErr.get() != null) {
484+
throw new AssertionError("install hook failed", hookErr.get());
485+
}
486+
} finally {
487+
manager.setBeforeInstallSyncHook(null);
488+
manager.setBeforeJoinAttemptHook(null);
351489
releaseWorker.countDown();
352490
if (!managerClosed) {
353491
Thread.interrupted();

0 commit comments

Comments
 (0)