Skip to content

Commit fa50e81

Browse files
glasstigerclaude
andcommitted
Fix orphan-drain losing symbol-dict mirror seed
The send loop seeded its reconnect catch-up mirror by calling PersistedSymbolDict.takeLoadedEntries(), a one-shot ownership transfer that zeroed the dictionary's loaded-entries buffer. The foreground sender builds a single loop, so that was safe. But BackgroundDrainer builds a fresh send loop per wire session against the SAME, persistent engine when a durable-ack capability gap forces a mid-drain recycle. The second loop then found an empty loaded-entries buffer, seeded an empty mirror (sentDictCount = 0), sent no reconnect catch-up, and the first replayed delta frame (deltaStart > 0) tripped the torn-dict guard -- falsely quarantining a healthy slot with a bogus "resend required" terminal and defeating the capability-gap settle-retry for delta slots. The send loop now COPIES the loaded entries into its own mirror and leaves the dictionary owning its buffer for the engine's lifetime, so every recycled loop re-seeds. PersistedSymbolDict.close() frees the dictionary's copy; each loop frees its own copy on exit. Peak footprint is unchanged (the drainer closes loop N before building loop N+1). Removes the now-unused takeLoadedEntries() and loadedEntriesTaken, and adds a regression test that builds two loops against one recovered engine and asserts the second re-seeds (pre-fix its mirror was empty). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 10a125c commit fa50e81

3 files changed

Lines changed: 91 additions & 36 deletions

File tree

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

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -543,15 +543,21 @@ public CursorWebSocketSendLoop(WebSocketClient client, CursorSendEngine engine,
543543
if (pd != null && pd.size() > 0) {
544544
int len = pd.loadedEntriesLen();
545545
if (len > 0) {
546-
// Adopt the persisted dictionary's loaded-entries buffer as the
547-
// mirror's initial backing instead of copying it and leaving the
548-
// dictionary to retain a second copy for the engine's lifetime.
549-
// The producer's readLoadedSymbols() -- the only other consumer --
550-
// runs earlier in setCursorEngine; the drainer path has no
551-
// producer consumer. takeLoadedEntries() transfers ownership, so
552-
// this loop's mirror lifecycle frees it (not pd.close()). The
553-
// buffer's allocated size equals loadedEntriesLen.
554-
sentDictBytesAddr = pd.takeLoadedEntries();
546+
// COPY the persisted dictionary's loaded-entries buffer into this
547+
// loop's own mirror rather than taking ownership of it. The engine
548+
// (and its PersistedSymbolDict) OUTLIVES this loop on the orphan
549+
// drainer path: BackgroundDrainer builds a fresh send loop per wire
550+
// session against the same engine on a durable-ack capability-gap
551+
// recycle. A one-shot ownership transfer would leave every loop
552+
// after the first with an EMPTY mirror -- it would then send no
553+
// reconnect catch-up, and the first replayed delta frame
554+
// (deltaStart > 0) would trip the torn-dict guard, falsely
555+
// quarantining a healthy slot. Copying keeps the dictionary's
556+
// loaded entries intact for the engine's lifetime so every
557+
// recycled loop re-seeds; pd.close() (at engine close) frees the
558+
// dictionary's copy, this loop frees its own copy on exit.
559+
sentDictBytesAddr = Unsafe.malloc(len, MemoryTag.NATIVE_DEFAULT);
560+
Unsafe.getUnsafe().copyMemory(pd.loadedEntriesAddr(), sentDictBytesAddr, len);
555561
sentDictBytesCapacity = len;
556562
sentDictBytesLen = len;
557563
// Set the count only alongside the bytes so sentDictCount can

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

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,14 @@ public final class PersistedSymbolDict implements QuietCloseable {
9595
private boolean closed;
9696
// In-memory copy of the entry region [len][utf8]... exactly as on disk,
9797
// populated only when open() recovered existing entries (recovery /
98-
// orphan-drain). Zero/empty for a freshly created file. Consumed once to seed
99-
// the producer's id map (readLoadedSymbols) and then ADOPTED by the send loop's
100-
// catch-up mirror (takeLoadedEntries), which takes ownership so this file no
101-
// longer retains a second copy of the dictionary for the engine's lifetime.
98+
// orphan-drain). Zero/empty for a freshly created file. READ (not consumed) to
99+
// seed the producer's id map (readLoadedSymbols) and to seed the send loop's
100+
// catch-up mirror, which COPIES it. This file retains ownership for the engine's
101+
// lifetime -- the orphan drainer builds a fresh send loop per wire session
102+
// against the same engine, and each must re-seed its mirror -- and frees this
103+
// buffer in close().
102104
private long loadedEntriesAddr;
103105
private int loadedEntriesLen;
104-
// True once takeLoadedEntries() handed loadedEntriesAddr to the send-loop
105-
// mirror; guards a stale readLoadedSymbols() call (which must run first).
106-
private boolean loadedEntriesTaken;
107106
private long scratchAddr;
108107
private int scratchCap;
109108
private int size;
@@ -282,7 +281,6 @@ public int loadedEntriesLen() {
282281
* recovered.
283282
*/
284283
public ObjList<String> readLoadedSymbols() {
285-
assert !loadedEntriesTaken : "readLoadedSymbols() must run before takeLoadedEntries()";
286284
ObjList<String> out = new ObjList<>(Math.max(size, 1));
287285
long p = loadedEntriesAddr;
288286
long limit = p + loadedEntriesLen;
@@ -313,25 +311,6 @@ public int size() {
313311
return size;
314312
}
315313

316-
/**
317-
* Transfers ownership of the loaded-entries buffer to the caller and returns
318-
* its base address (0 when nothing was recovered). The send loop adopts it as
319-
* the initial backing of its catch-up mirror rather than copying it, so this
320-
* file stops retaining a second copy of the dictionary for the engine's
321-
* lifetime. After this call {@link #close()} no longer frees the buffer -- the
322-
* caller owns it. The producer's {@link #readLoadedSymbols()} is the only other
323-
* consumer and MUST run first: {@code setCursorEngine} seeds the producer
324-
* before the send loop is constructed, and the drainer path has no producer
325-
* consumer. The buffer was allocated with {@link MemoryTag#NATIVE_DEFAULT}.
326-
*/
327-
public long takeLoadedEntries() {
328-
long addr = loadedEntriesAddr;
329-
loadedEntriesAddr = 0L;
330-
loadedEntriesLen = 0;
331-
loadedEntriesTaken = true;
332-
return addr;
333-
}
334-
335314
private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
336315
int fd = Files.openRW(filePath);
337316
if (fd < 0) {

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.junit.Test;
3434

3535
import java.io.IOException;
36+
import java.lang.reflect.Field;
3637
import java.nio.file.Files;
3738
import java.nio.file.Path;
3839
import java.util.Comparator;
@@ -96,6 +97,69 @@ public void testSeededMirrorFreedWhenLoopClosedWithoutStart() throws Exception {
9697
}
9798
}
9899

100+
@Test
101+
public void testRecycledLoopReSeedsMirrorFromPersistedDict() throws Exception {
102+
// C1 regression: the orphan drainer (BackgroundDrainer) builds a NEW
103+
// CursorWebSocketSendLoop per wire session against the SAME, persistent
104+
// engine when a durable-ack capability gap forces a mid-drain recycle. The
105+
// recovery mirror seed must survive that recycle. If the first loop CONSUMED
106+
// the persisted dictionary's loaded entries (a one-shot ownership transfer),
107+
// the second loop seeds an EMPTY mirror (sentDictCount = 0), sends no
108+
// reconnect catch-up, and the first replayed delta frame (deltaStart > 0)
109+
// trips the torn-dict guard -- falsely quarantining a healthy slot with a
110+
// bogus "resend required" terminal. Copying the entries (leaving the
111+
// dictionary intact for the engine's lifetime) lets every recycled loop
112+
// re-seed. Pre-fix, loop2's sentDictCount is 0 and this assertion fails.
113+
Path sfDir = Files.createTempDirectory("qwp-mirror-reseed");
114+
try {
115+
populateRecoverableSlot(sfDir);
116+
Path slot = sfDir.resolve("default");
117+
assertMemoryLeak(() -> {
118+
try (CursorSendEngine engine = new CursorSendEngine(slot.toString(), 4096)) {
119+
PersistedSymbolDict pd = engine.getPersistedSymbolDict();
120+
Assert.assertNotNull(pd);
121+
int dictSize = pd.size();
122+
Assert.assertTrue("recovery must load a non-empty dictionary", dictSize > 0);
123+
124+
// Session 1 seeds its mirror from the persisted dictionary.
125+
CursorWebSocketSendLoop loop1 = newRecoveryLoop(engine);
126+
try {
127+
Assert.assertEquals("session-1 mirror must seed from the persisted dict",
128+
dictSize, readInt(loop1, "sentDictCount"));
129+
} finally {
130+
loop1.close();
131+
}
132+
133+
// Session 2 against the SAME engine (the drainer recycle): the
134+
// seed must NOT have been consumed -- the mirror must re-seed to
135+
// the full dictionary so the reconnect catch-up is complete.
136+
CursorWebSocketSendLoop loop2 = newRecoveryLoop(engine);
137+
try {
138+
Assert.assertEquals("recycled session-2 mirror must re-seed from the "
139+
+ "persisted dict (pre-fix it was 0)",
140+
dictSize, readInt(loop2, "sentDictCount"));
141+
} finally {
142+
loop2.close();
143+
}
144+
}
145+
});
146+
} finally {
147+
rmDir(sfDir);
148+
}
149+
}
150+
151+
// Constructs a recovery send loop but does NOT start it: the ctor seeds the
152+
// catch-up mirror synchronously, which is all these tests observe. The
153+
// reconnect factory is never invoked.
154+
private static CursorWebSocketSendLoop newRecoveryLoop(CursorSendEngine engine) {
155+
return new CursorWebSocketSendLoop(
156+
null, engine, 0, 1_000_000L,
157+
() -> {
158+
throw new IOException("no reconnect in this test");
159+
},
160+
0, 0, 1);
161+
}
162+
99163
private static void populateRecoverableSlot(Path sfDir) throws Exception {
100164
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
101165
int port = silent.getPort();
@@ -117,6 +181,12 @@ private static void populateRecoverableSlot(Path sfDir) throws Exception {
117181
}
118182
}
119183

184+
private static int readInt(CursorWebSocketSendLoop loop, String name) throws Exception {
185+
Field f = CursorWebSocketSendLoop.class.getDeclaredField(name);
186+
f.setAccessible(true);
187+
return f.getInt(loop);
188+
}
189+
120190
private static void rmDir(Path dir) throws IOException {
121191
if (dir == null || !Files.exists(dir)) {
122192
return;

0 commit comments

Comments
 (0)