Skip to content

Commit fc8b4ba

Browse files
glasstigerclaude
andcommitted
Adopt recovered dictionary into the send mirror
On recovery the send loop copied the persisted dictionary's loaded-entries buffer into a fresh mirror allocation and left PersistedSymbolDict holding a second copy for the engine's lifetime -- roughly twice the dictionary size in native memory on a high-cardinality recovered slot, retained long after the one-time seed. The loop now adopts that buffer as its mirror backing via takeLoadedEntries(), which transfers ownership so the dictionary no longer retains or frees it. The producer's readLoadedSymbols() is the only other consumer and runs first (setCursorEngine seeds the producer before the loop is built; the drainer has no producer consumer), guarded by an assert. Add a recover-then-continue-ingest test. A file-mode sender writes symbols and crashes; a fresh sender recovers the slot and ingests a NEW symbol. It asserts the producer continues the dictionary from the recovered size instead of colliding at id 0, exercising seedGlobalDictionaryFromPersisted, which no prior test drove past recovery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8fcd835 commit fc8b4ba

3 files changed

Lines changed: 97 additions & 8 deletions

File tree

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,16 @@ public CursorWebSocketSendLoop(WebSocketClient client, CursorSendEngine engine,
543543
if (pd != null && pd.size() > 0) {
544544
int len = pd.loadedEntriesLen();
545545
if (len > 0) {
546-
ensureSentDictCapacity(len);
547-
Unsafe.getUnsafe().copyMemory(pd.loadedEntriesAddr(), sentDictBytesAddr, len);
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();
555+
sentDictBytesCapacity = len;
548556
sentDictBytesLen = len;
549557
// Set the count only alongside the bytes so sentDictCount can
550558
// never claim symbols the mirror does not hold. A recovered slot

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

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,14 +94,19 @@ public final class PersistedSymbolDict implements QuietCloseable {
9494
private static final int MAX_ENTRY_LEN = 1 << 20;
9595
private static final Logger LOG = LoggerFactory.getLogger(PersistedSymbolDict.class);
9696
private final int fd;
97-
// In-memory copy of the entry region [len][utf8]... exactly as on disk,
98-
// populated only when open() recovered existing entries (recovery /
99-
// orphan-drain). Zero/empty for a freshly created file. Consumed once to
100-
// seed the send loop's catch-up mirror and the producer's id map.
101-
private final long loadedEntriesAddr;
102-
private final int loadedEntriesLen;
10397
private long appendOffset;
10498
private boolean closed;
99+
// In-memory copy of the entry region [len][utf8]... exactly as on disk,
100+
// populated only when open() recovered existing entries (recovery /
101+
// orphan-drain). Zero/empty for a freshly created file. Consumed once to seed
102+
// the producer's id map (readLoadedSymbols) and then ADOPTED by the send loop's
103+
// catch-up mirror (takeLoadedEntries), which takes ownership so this file no
104+
// longer retains a second copy of the dictionary for the engine's lifetime.
105+
private long loadedEntriesAddr;
106+
private int loadedEntriesLen;
107+
// True once takeLoadedEntries() handed loadedEntriesAddr to the send-loop
108+
// mirror; guards a stale readLoadedSymbols() call (which must run first).
109+
private boolean loadedEntriesTaken;
105110
private long scratchAddr;
106111
private int scratchCap;
107112
private int size;
@@ -256,6 +261,7 @@ public int loadedEntriesLen() {
256261
* recovered.
257262
*/
258263
public ObjList<String> readLoadedSymbols() {
264+
assert !loadedEntriesTaken : "readLoadedSymbols() must run before takeLoadedEntries()";
259265
ObjList<String> out = new ObjList<>(Math.max(size, 1));
260266
long p = loadedEntriesAddr;
261267
long limit = p + loadedEntriesLen;
@@ -286,6 +292,25 @@ public int size() {
286292
return size;
287293
}
288294

295+
/**
296+
* Transfers ownership of the loaded-entries buffer to the caller and returns
297+
* its base address (0 when nothing was recovered). The send loop adopts it as
298+
* the initial backing of its catch-up mirror rather than copying it, so this
299+
* file stops retaining a second copy of the dictionary for the engine's
300+
* lifetime. After this call {@link #close()} no longer frees the buffer -- the
301+
* caller owns it. The producer's {@link #readLoadedSymbols()} is the only other
302+
* consumer and MUST run first: {@code setCursorEngine} seeds the producer
303+
* before the send loop is constructed, and the drainer path has no producer
304+
* consumer. The buffer was allocated with {@link MemoryTag#NATIVE_DEFAULT}.
305+
*/
306+
public long takeLoadedEntries() {
307+
long addr = loadedEntriesAddr;
308+
loadedEntriesAddr = 0L;
309+
loadedEntriesLen = 0;
310+
loadedEntriesTaken = true;
311+
return addr;
312+
}
313+
289314
private static PersistedSymbolDict openExisting(String filePath, long fileLen) {
290315
int fd = Files.openRW(filePath);
291316
if (fd < 0) {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/DeltaDictRecoveryTest.java

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,62 @@ public void testTornDictionaryFailsCleanlyInsteadOfCorrupting() throws Exception
227227
});
228228
}
229229

230+
@Test
231+
public void testRecoveredSenderContinuesIngestingNewSymbols() throws Exception {
232+
// M2 regression: seedGlobalDictionaryFromPersisted resumes the producer's
233+
// dictionary and delta baseline from the persisted .symbol-dict, so a
234+
// recovered sender that continues ingesting assigns the NEXT id, not a
235+
// colliding low one. Without it the new symbol reuses a recovered id and the
236+
// fresh server sees a redefinition -> silent misattribution. No prior test
237+
// ingests on the recovered sender. Replay is from FSN 0 (no acks), so the
238+
// recovered frames legitimately overlap the seeded dictionary -- this also
239+
// pins that the redefinition guard does not false-positive on normal
240+
// recovery.
241+
assertMemoryLeak(() -> {
242+
// Phase 1: ingest DISTINCT_SYMBOLS symbols, silent server, close-fast ->
243+
// unacked frames + a full persisted dictionary.
244+
try (TestWebSocketServer silent = new TestWebSocketServer(new SilentHandler())) {
245+
int port = silent.getPort();
246+
silent.start();
247+
Assert.assertTrue(silent.awaitStart(5, TimeUnit.SECONDS));
248+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir
249+
+ ";sf_max_bytes=4096;close_flush_timeout_millis=0;";
250+
try (Sender s1 = Sender.fromConfig(cfg)) {
251+
for (int i = 0; i < DISTINCT_SYMBOLS; i++) {
252+
s1.table("m").symbol("s", "sym-" + i).longColumn("v", i).atNow();
253+
s1.flush();
254+
}
255+
}
256+
}
257+
258+
// Phase 2: recover against a fresh server, then ingest a genuinely NEW
259+
// symbol. The producer must continue at id DISTINCT_SYMBOLS.
260+
DictReconstructingHandler handler = new DictReconstructingHandler();
261+
try (TestWebSocketServer good = new TestWebSocketServer(handler)) {
262+
int port = good.getPort();
263+
good.start();
264+
Assert.assertTrue(good.awaitStart(5, TimeUnit.SECONDS));
265+
String cfg = "ws::addr=localhost:" + port + ";sf_dir=" + sfDir + ";";
266+
try (Sender s2 = Sender.fromConfig(cfg)) {
267+
s2.table("m").symbol("s", "brand-new").longColumn("v", 99L).atNow();
268+
s2.flush();
269+
long deadline = System.currentTimeMillis() + 10_000;
270+
while (System.currentTimeMillis() < deadline
271+
&& handler.maxDictSize() < DISTINCT_SYMBOLS + 1) {
272+
Thread.sleep(20);
273+
}
274+
}
275+
List<String> dict = handler.dictSnapshot();
276+
Assert.assertEquals("recovered sender must continue the dictionary, not collide: " + dict,
277+
DISTINCT_SYMBOLS + 1, dict.size());
278+
for (int i = 0; i < DISTINCT_SYMBOLS; i++) {
279+
Assert.assertEquals("sym-" + i, dict.get(i));
280+
}
281+
Assert.assertEquals("brand-new", dict.get(DISTINCT_SYMBOLS));
282+
}
283+
});
284+
}
285+
230286
@Test
231287
public void testUnopenablePersistedDictStillGuardsAgainstReplayingDeltaFrames() throws Exception {
232288
// C1 regression: when a recovered disk slot's persisted dictionary cannot be

0 commit comments

Comments
 (0)