Skip to content

Commit eea7f89

Browse files
committed
test(qwp): pin slot retention when worker-exit handoff registration fails
A throw from deferUntilWorkerExit (allocation failure while building the handoff) carries no worker-liveness information, so close() must retain every worker-reachable resource instead of running terminal cleanup inline under a possibly-live worker. Add a test seam that throws from the registration path while the worker is provably mid service pass and assert the slot flock, ring and watermark are retained, close stays incomplete, and a retried close() after worker exit converges and releases the slot.
1 parent 930175f commit eea7f89

2 files changed

Lines changed: 136 additions & 0 deletions

File tree

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,13 @@ public final class SegmentManager implements QuietCloseable {
9797
// production; owned-engine close tests use it to prove they take only the
9898
// stronger whole-manager join path, not two sequential timeout budgets.
9999
private volatile Runnable beforeRingQuiescenceAwaitHook;
100+
// Test seam: runs at the top of deferUntilWorkerExit, before the
101+
// worker-liveness check. Null in production; registration-failure tests
102+
// throw from it to simulate an allocation failure (OOM building the
103+
// cleanup lambda or growing exitCleanups) while the worker is still
104+
// live. Callers must treat such a throw as "worker state unknown",
105+
// never as the exact false return meaning the worker loop has exited.
106+
private volatile Runnable beforeExitCleanupRegistrationHook;
100107
// Test seam: runs on the worker thread just before the trim block's
101108
// synchronized(lock) entry. Null in production; only
102109
// SegmentManagerTrimDeregisterRaceTest installs it, to deterministically
@@ -283,8 +290,18 @@ public synchronized void close() {
283290
* flip share {@link #lock}, so the cleanup runs exactly once: either it
284291
* is registered before the flip and the worker runs it, or the flip won
285292
* and this method rejects the handoff.
293+
* <p>
294+
* May throw on allocation failure (the lambda at the call site, the
295+
* {@code ObjList}, or its growth). A throw carries NO liveness
296+
* information: the worker was never observed, so the caller must treat
297+
* it as "worker possibly still live" and retain resources — never as
298+
* the exact {@code false} return above.
286299
*/
287300
public boolean deferUntilWorkerExit(Runnable cleanup) {
301+
Runnable hook = beforeExitCleanupRegistrationHook;
302+
if (hook != null) {
303+
hook.run();
304+
}
288305
synchronized (lock) {
289306
if (workerLoopExited || workerThread == null) {
290307
return false;
@@ -465,6 +482,11 @@ public void register(SegmentRing ring, String dir, AckWatermark watermark) {
465482
wakeWorker();
466483
}
467484

485+
@TestOnly
486+
public void setBeforeExitCleanupRegistrationHook(Runnable hook) {
487+
this.beforeExitCleanupRegistrationHook = hook;
488+
}
489+
468490
@TestOnly
469491
public void setBeforeInstallSyncHook(Runnable hook) {
470492
this.beforeInstallSyncHook = hook;

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

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,120 @@ public void testOwnedEngineCloseHandsCleanupToWorkerExit() throws Exception {
409409
});
410410
}
411411

412+
/**
413+
* Registration-failure twin of
414+
* {@link #testOwnedEngineCloseHandsCleanupToWorkerExit}: when
415+
* {@code deferUntilWorkerExit} itself throws (allocation failure while
416+
* building the handoff), close() must NOT mistake the swallowed throw
417+
* for "worker already exited" and run the terminal cleanup inline — the
418+
* worker is provably still mid service pass, so releasing the ring,
419+
* watermark or slot flock here is the original stale-worker UAF/data-loss
420+
* hazard. Every worker-reachable resource must be retained and the close
421+
* must stay incomplete; a retried close() after the worker exits
422+
* converges and releases the slot.
423+
*/
424+
@Test(timeout = 30_000L)
425+
public void testOwnedEngineCloseRetainsSlotWhenHandoffRegistrationFails() throws Exception {
426+
TestUtils.assertMemoryLeak(() -> {
427+
final int payloadLen = 32;
428+
long segSize = MmapSegment.HEADER_SIZE + (MmapSegment.FRAME_HEADER_SIZE + payloadLen);
429+
String slot = tmpDir + "/owned-regfail-slot";
430+
CountDownLatch workerBlocked = new CountDownLatch(1);
431+
CountDownLatch releaseWorker = new CountDownLatch(1);
432+
AtomicBoolean fired = new AtomicBoolean();
433+
AtomicReference<Throwable> hookErr = new AtomicReference<>();
434+
// Production shape: private, owned manager (ownsManager=true).
435+
CursorSendEngine engine = new CursorSendEngine(slot, segSize);
436+
SegmentManager manager = readManager(engine);
437+
long buf = Unsafe.malloc(payloadLen, MemoryTag.NATIVE_DEFAULT);
438+
try {
439+
// Phase 1: wait out the initial spare install so the park hook
440+
// can only fire on the rotation-triggered pass (see
441+
// testOwnedEngineCloseRetainsSlotWhileWorkerIsMidServicePass).
442+
SegmentRing ring = readRing(engine);
443+
long deadlineNs = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
444+
while (ring.needsHotSpare()) {
445+
if (System.nanoTime() > deadlineNs) {
446+
throw new AssertionError("manager worker never installed the initial hot spare");
447+
}
448+
Thread.sleep(1);
449+
}
450+
manager.setBeforeInstallSyncHook(() -> {
451+
if (!fired.compareAndSet(false, true)) return;
452+
workerBlocked.countDown();
453+
try {
454+
if (!releaseWorker.await(20, TimeUnit.SECONDS)) {
455+
hookErr.compareAndSet(null,
456+
new AssertionError("timed out waiting for test to release worker"));
457+
}
458+
} catch (Throwable t) {
459+
hookErr.compareAndSet(null, t);
460+
}
461+
});
462+
463+
// Phase 2: fill the active segment and rotate onto the spare;
464+
// the worker's next tick re-enters the install pass and parks.
465+
Unsafe.getUnsafe().putLong(buf, 0L);
466+
Assert.assertEquals(0L, engine.appendBlocking(buf, payloadLen));
467+
Assert.assertEquals(1L, engine.appendBlocking(buf, payloadLen));
468+
Assert.assertTrue("worker never re-entered a spare-install pass",
469+
workerBlocked.await(5, TimeUnit.SECONDS));
470+
471+
// Phase 3: make the handoff registration throw — simulating
472+
// an OutOfMemoryError allocating the cleanup lambda/list —
473+
// with the worker provably still mid service pass. close()
474+
// must retain everything: no inline finishClose, no flock
475+
// release, closeCompleted stays false.
476+
manager.setBeforeExitCleanupRegistrationHook(() -> {
477+
throw new OutOfMemoryError("simulated allocation failure registering exit cleanup");
478+
});
479+
manager.setWorkerJoinTimeoutMillis(50L);
480+
engine.close();
481+
Assert.assertFalse("close must stay incomplete when handoff registration fails",
482+
engine.isCloseCompleted());
483+
try {
484+
SlotLock probe = SlotLock.acquire(slot);
485+
probe.close();
486+
Assert.fail("engine.close() released the slot lock after a failed handoff "
487+
+ "registration while the manager worker was still mid service "
488+
+ "pass — the swallowed throw was mistaken for proof the worker "
489+
+ "exited (stale-worker UAF/data-loss hazard)");
490+
} catch (Exception expected) {
491+
// good — slot retained while the worker can still touch it.
492+
}
493+
494+
// Phase 4: clear the fault, release the worker (its loop was
495+
// already stopped by the close attempt, so it exits), and
496+
// retry close(). The retry must converge via isWorkerReaped()
497+
// and release the slot.
498+
manager.setBeforeExitCleanupRegistrationHook(null);
499+
releaseWorker.countDown();
500+
manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60));
501+
engine.close();
502+
Assert.assertTrue("retried close must report complete cleanup",
503+
engine.isCloseCompleted());
504+
try (SlotLock probe = SlotLock.acquire(slot)) {
505+
Assert.assertNotNull("slot must be acquirable after the retried close", probe);
506+
} catch (Exception e) {
507+
throw new AssertionError("retried close() did not release the slot lock", e);
508+
}
509+
if (hookErr.get() != null) {
510+
throw new AssertionError("install hook failed", hookErr.get());
511+
}
512+
} finally {
513+
Unsafe.free(buf, payloadLen, MemoryTag.NATIVE_DEFAULT);
514+
manager.setBeforeInstallSyncHook(null);
515+
manager.setBeforeExitCleanupRegistrationHook(null);
516+
releaseWorker.countDown();
517+
manager.setWorkerJoinTimeoutMillis(TimeUnit.SECONDS.toMillis(60));
518+
try {
519+
engine.close();
520+
} catch (Throwable ignored) {
521+
}
522+
}
523+
});
524+
}
525+
412526
/**
413527
* An engine that owns its manager must use the whole-manager stop/join as
414528
* its only quiescence barrier. Calling the per-ring barrier first would

0 commit comments

Comments
 (0)