Skip to content

Commit 7a95653

Browse files
glasstigerclaude
andcommitted
Discard a surviving symbol dict on fresh start
A fresh store-and-forward slot inherited a stale .symbol-dict when the best-effort removeOrphan failed to delete it (e.g. a Windows share lock) or a crash landed in the close window: open() parses and TRUSTS any survivor. Because the fresh-start producer is not seeded from the dictionary (that is gated on wasRecoveredFromDisk, while the send-loop mirror and the persist resume point key off pd.size()), the producer's ids then diverged from the dictionary the send loop replays, silently misattributing symbol values after a reconnect. The dictionary is load-bearing, unlike the max()-clamped ack watermark whose removeOrphan+open pattern it had copied. Enforce "fresh start -> empty dictionary" at the source: openClean() truncates any survivor via openCleanRW instead of trusting it, and if the clean open itself fails the sender falls back to full self-sufficient frames, which is also safe. The recovery path keeps open(); removeOrphan stays for the fully-drained close. Both regression tests fail without the fix (dict size 2, not 0): - PersistedSymbolDictTest.testOpenCleanDiscardsSurvivingDictionary - EmptyOrphanSlotChurnTest.testFreshStartDiscardsSurvivingStaleDictionary Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 444199e commit 7a95653

4 files changed

Lines changed: 131 additions & 8 deletions

File tree

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -321,10 +321,19 @@ private CursorSendEngine(String sfDir, long segmentSizeBytes, SegmentManager man
321321
if (!memoryMode) {
322322
AckWatermark.removeOrphan(sfDir);
323323
watermarkInProgress = AckWatermark.open(sfDir);
324-
// Same stale-side-file hygiene for the symbol dictionary: a
325-
// fresh slot starts with an empty dictionary.
326-
PersistedSymbolDict.removeOrphan(sfDir);
327-
persistedDictInProgress = PersistedSymbolDict.open(sfDir);
324+
// A fresh slot MUST start with an EMPTY symbol dictionary.
325+
// Unlike the ack watermark above -- a discardable optimization a
326+
// max() clamp protects -- the dictionary is load-bearing: a
327+
// delta frame referencing an id missing from it is unrecoverable,
328+
// and a STALE dictionary inherited here (the segments are gone, so
329+
// the producer is NOT seeded from it) shifts the dense id->symbol
330+
// mapping and silently misattributes symbols on the next
331+
// reconnect. openClean() truncates any survivor to empty rather
332+
// than trusting a best-effort delete that may have failed (e.g. a
333+
// Windows share lock); if the clean open itself fails,
334+
// persistedSymbolDict stays null and the sender falls back to full
335+
// self-sufficient frames, which is also safe.
336+
persistedDictInProgress = PersistedSymbolDict.openClean(sfDir);
328337
}
329338
MmapSegment initial;
330339
String initialPath = null;

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,31 @@ public static PersistedSymbolDict open(String slotDir) {
147147
}
148148

149149
/**
150-
* Best-effort removal of a stale dictionary file. Used at fresh-start (a
151-
* stale dict with no segments behind it is meaningless) and at fully-drained
152-
* close (the slot is empty, nothing references the dictionary any more),
153-
* mirroring {@link AckWatermark#removeOrphan}.
150+
* Opens the dictionary in {@code slotDir} as a FRESH, EMPTY file, discarding
151+
* any surviving content. This is the fresh-start counterpart to {@link #open}:
152+
* a slot with no recovered segments must start with an empty dictionary, so a
153+
* dictionary left by a prior lifecycle -- a fully-drained slot whose
154+
* best-effort delete failed, or a crash in the close window -- must NOT be
155+
* inherited. Unlike {@link #open}, which parses and TRUSTS an existing file for
156+
* recovery/orphan-drain replay, this truncates it: the fresh-start producer is
157+
* not seeded from the dictionary, so trusting a survivor would leave the
158+
* producer's ids diverged from the dictionary the send loop replays and
159+
* silently misattribute symbols on the next reconnect. Truncating (rather than
160+
* relying on an unlink succeeding first) closes the gap even when the delete is
161+
* refused -- e.g. a Windows share lock. Returns {@code null} on I/O failure, so
162+
* the caller falls back to full self-sufficient frames exactly as {@link #open}
163+
* does.
164+
*/
165+
public static PersistedSymbolDict openClean(String slotDir) {
166+
return openFresh(slotDir + "/" + FILE_NAME);
167+
}
168+
169+
/**
170+
* Best-effort removal of a stale dictionary file. Used at fully-drained close
171+
* (the slot is empty, nothing references the dictionary any more), mirroring
172+
* {@link AckWatermark#removeOrphan}. The fresh-start path deliberately does NOT
173+
* use this -- it opens a clean dictionary via {@link #openClean} instead, so a
174+
* failed delete cannot leave a stale dictionary a new session would trust.
154175
*/
155176
public static void removeOrphan(String slotDir) {
156177
Files.remove(slotDir + "/" + FILE_NAME);

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
package io.questdb.client.test.cutlass.qwp.client.sf.cursor;
2626

2727
import io.questdb.client.cutlass.qwp.client.sf.cursor.CursorSendEngine;
28+
import io.questdb.client.cutlass.qwp.client.sf.cursor.PersistedSymbolDict;
2829
import io.questdb.client.std.Files;
2930
import io.questdb.client.test.tools.TestUtils;
3031
import org.junit.After;
@@ -35,6 +36,7 @@
3536

3637
import static org.junit.Assert.assertEquals;
3738
import static org.junit.Assert.assertFalse;
39+
import static org.junit.Assert.assertNotNull;
3840

3941
/**
4042
* Regression test for M6 — drainer adopting an empty orphan slot would
@@ -87,6 +89,48 @@ public void tearDown() {
8789
Files.remove(sfDir);
8890
}
8991

92+
@Test
93+
public void testFreshStartDiscardsSurvivingStaleDictionary() throws Exception {
94+
// Regression: a prior fully-drained lifecycle can leave a stale
95+
// .symbol-dict behind (a best-effort delete that failed, or a crash in the
96+
// close window) with NO segments. A fresh start must DISCARD it -- the
97+
// dictionary is load-bearing and the fresh-start producer is not seeded
98+
// from it, so trusting a survivor would diverge the producer ids from the
99+
// dictionary the send loop replays and misattribute symbols on reconnect.
100+
TestUtils.assertMemoryLeak(() -> {
101+
// Pre-seed a stale dictionary in the slot, with no segments behind it.
102+
PersistedSymbolDict stale = PersistedSymbolDict.open(sfDir);
103+
assertNotNull(stale);
104+
try {
105+
stale.appendSymbol("staleX");
106+
stale.appendSymbol("staleY");
107+
assertEquals(2, stale.size());
108+
} finally {
109+
stale.close();
110+
}
111+
112+
// A fresh start (no recovered segments) must open a CLEAN, empty
113+
// dictionary -- not inherit the survivor.
114+
try (CursorSendEngine engine = new CursorSendEngine(sfDir, 4L * 1024 * 1024)) {
115+
assertFalse("fresh start must not report a disk recovery",
116+
engine.wasRecoveredFromDisk());
117+
PersistedSymbolDict pd = engine.getPersistedSymbolDict();
118+
assertNotNull(pd);
119+
assertEquals("fresh start must discard the surviving stale dictionary",
120+
0, pd.size());
121+
}
122+
123+
// The survivor's bytes are physically gone, not just hidden.
124+
PersistedSymbolDict reopened = PersistedSymbolDict.open(sfDir);
125+
assertNotNull(reopened);
126+
try {
127+
assertEquals(0, reopened.size());
128+
} finally {
129+
reopened.close();
130+
}
131+
});
132+
}
133+
90134
@Test
91135
public void testNeverPublishedCloseLeavesNoSfaFiles() throws Exception {
92136
TestUtils.assertMemoryLeak(() -> {

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,55 @@ public void testLargeSymbolRoundTripsAcrossReopen() throws Exception {
299299
});
300300
}
301301

302+
@Test
303+
public void testOpenCleanDiscardsSurvivingDictionary() throws Exception {
304+
// A fresh start must NOT inherit a dictionary left by a prior lifecycle:
305+
// openClean() truncates any survivor to empty, where open() would recover
306+
// (and TRUST) it. Trusting a survivor whose segments are gone -- the
307+
// fresh-start producer is not seeded from it -- shifts the dense id->symbol
308+
// mapping and misattributes symbols on the next reconnect.
309+
assertMemoryLeak(() -> {
310+
Path dir = Files.createTempDirectory("qwp-symdict-clean");
311+
try {
312+
PersistedSymbolDict stale = PersistedSymbolDict.open(dir.toString());
313+
Assert.assertNotNull(stale);
314+
try {
315+
stale.appendSymbol("staleX");
316+
stale.appendSymbol("staleY");
317+
Assert.assertEquals(2, stale.size());
318+
} finally {
319+
stale.close();
320+
}
321+
322+
// Fresh start: openClean yields an EMPTY dictionary regardless of
323+
// the survivor, and appends from id 0 again.
324+
PersistedSymbolDict fresh = PersistedSymbolDict.openClean(dir.toString());
325+
Assert.assertNotNull(fresh);
326+
try {
327+
Assert.assertEquals(0, fresh.size());
328+
Assert.assertEquals(0, fresh.readLoadedSymbols().size());
329+
fresh.appendSymbol("freshA");
330+
Assert.assertEquals(1, fresh.size());
331+
} finally {
332+
fresh.close();
333+
}
334+
335+
// The survivor's bytes are physically gone, not just hidden: a
336+
// subsequent recovery open() sees only the post-clean content.
337+
PersistedSymbolDict reopened = PersistedSymbolDict.open(dir.toString());
338+
Assert.assertNotNull(reopened);
339+
try {
340+
Assert.assertEquals(1, reopened.size());
341+
Assert.assertEquals("freshA", reopened.readLoadedSymbols().getQuick(0));
342+
} finally {
343+
reopened.close();
344+
}
345+
} finally {
346+
rmDir(dir);
347+
}
348+
});
349+
}
350+
302351
@Test
303352
public void testRemoveOrphanDeletesFile() throws Exception {
304353
assertMemoryLeak(() -> {

0 commit comments

Comments
 (0)