Skip to content

Commit 0304df8

Browse files
bluestreak01claude
andcommitted
fix(ilp): close schema-reset race window with connection-generation tag
Pre-fix: flushPendingRows checked schemaResetNeeded once, at the top of the encode pass. If the I/O thread completed a reconnect AFTER that check but BEFORE encoder.finishMessage, the encoded bytes carried stale schema-id refs into the previous connection's id space. Those bytes then went through segmentLog.append (persisted to SF) and out to the new server, which rejected them. Pre-C4: silent unbounded reconnect-replay loop. Post-C4: terminal failure with no self-heal — the user has to manually clear the SF dir to recover. Fix (option C from the previous review): connection-generation tag on each encoded batch. - New volatile long QwpWebSocketSender.connectionGeneration, bumped by performReconnect AFTER schemaResetNeeded is flipped. Order is load-bearing: a reader that observes the new generation also sees the new schemaResetNeeded (volatile happens-before within the writer thread). - flushPendingRows now wraps the encode in a retry loop: long genBefore = connectionGeneration; // read FIRST if (schemaResetNeeded) reset; encode... if (connectionGeneration != genBefore) discard + retry; Re-encoding is cheap because the source rows in QwpTableBuffer are not reset until AFTER sealAndSwapBuffer (line 1830 in this commit). encoder.beginMessage internally calls buffer.reset(), so the discard step is implicit. - Bounded at MAX_SCHEMA_RACE_RETRIES = 10. Reconnects firing faster than a single encode is pathological and surfaces as LineSenderException to the user rather than a silent infinite loop. countNonEmptyTables extracted as a small helper so the retry loop reads cleanly. Together with C4 (server-error responses are now terminal) the schema-reset race goes from "silent data corruption + infinite loop" to "no poisoned batch ever reaches SF in the first place". Tests: - testGenerationBumpBetweenBatchesTriggersSchemaReset (SfIntegrationTest): reflectively bumps connectionGeneration + sets schemaResetNeeded between batches; asserts the next batch carries a fresh schema definition (frame size >= the first batch). - testSchemaResetRaceUnderConcurrentBumps (SfIntegrationTest, 30s timeout): spawns a bumper thread that flips schemaResetNeeded + bumps generation on a 50us cadence while the main thread flushes 200 batches in a tight loop. Asserts every batch either ships successfully OR the bounded MAX_SCHEMA_RACE_RETRIES fail-fast trips — never a silent escape, never an unexpected exception. Mid-encode injection without instrumentation is timing-sensitive; the stress test is a smoke check that the retry loop does not crash under load. End-to-end "poisoned bytes never reach the server" verification would need a strict QWP-wire-format-validating server test handler — left as a future test. Full suite: 1983 tests pass, zero regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 03afa61 commit 0304df8

2 files changed

Lines changed: 270 additions & 52 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpWebSocketSender.java

Lines changed: 113 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,18 @@ public class QwpWebSocketSender implements Sender {
181181
// thread on the next flushPendingRows so the next batch re-publishes schemas
182182
// the new server doesn't yet know about.
183183
private volatile boolean schemaResetNeeded;
184+
// Monotonic counter bumped by performReconnect AFTER schemaResetNeeded is
185+
// flipped. The user thread reads this at the very top of flushPendingRows
186+
// (before checking schemaResetNeeded), then again after finishMessage; if
187+
// the value changed, a reconnect happened mid-encode and the encoded bytes
188+
// may carry stale schema refs the new server doesn't know — discard and
189+
// re-encode. This closes the schema-reset race window where the previous
190+
// code could persist a poisoned batch into SF that would replay-loop
191+
// forever (or — post C4 fix — surface as a hard terminal failure with no
192+
// self-heal). Single writer (the I/O thread) so plain volatile ++ is
193+
// safe; readers establish happens-before via the volatile read pair.
194+
private volatile long connectionGeneration;
195+
private static final int MAX_SCHEMA_RACE_RETRIES = 10;
184196

185197
private QwpWebSocketSender(
186198
String host,
@@ -1574,9 +1586,15 @@ private WebSocketClient performReconnect() throws Exception {
15741586
client.upgrade(WRITE_PATH, authorizationHeader);
15751587
encoder.setVersion((byte) client.getServerQwpVersion());
15761588
// Tell the user thread to reset schema-id state on its next encode pass.
1577-
// Safe to set from here because the user thread reads this flag only at
1578-
// batch boundaries (top of flushPendingRows), not mid-encode.
1589+
// Order is load-bearing: schemaResetNeeded BEFORE connectionGeneration
1590+
// bump. The user thread reads connectionGeneration first, then
1591+
// schemaResetNeeded; volatile happens-before guarantees that a user
1592+
// thread observing the new generation also observes the new
1593+
// schemaResetNeeded. The post-encode generation re-read catches the
1594+
// race where the user thread already passed the schemaResetNeeded
1595+
// check before this write landed.
15791596
schemaResetNeeded = true;
1597+
connectionGeneration++;
15801598
return client;
15811599
}
15821600

@@ -1668,33 +1686,13 @@ private void flushPendingRows() {
16681686
return;
16691687
}
16701688

1671-
// SF reconnect happened on the I/O thread; the new server has no memory
1672-
// of our previous schema-id assignments. Reset before encoding so the
1673-
// next batch carries full schema definitions, not just refs.
1674-
if (schemaResetNeeded) {
1675-
schemaResetNeeded = false;
1676-
resetSchemaStateForNewConnection();
1677-
}
1678-
1679-
// Invalidate cached column references -- table buffers will be reset below
1689+
// Invalidate cached column references -- table buffers will be reset
1690+
// below (or on retry).
16801691
cachedTimestampColumn = null;
16811692
cachedTimestampNanosColumn = null;
16821693

16831694
ObjList<CharSequence> keys = tableBuffers.keys();
1684-
1685-
// Count non-empty tables for the message header
1686-
int tableCount = 0;
1687-
for (int i = 0, n = keys.size(); i < n; i++) {
1688-
CharSequence tableName = keys.getQuick(i);
1689-
if (tableName == null) {
1690-
continue;
1691-
}
1692-
QwpTableBuffer tableBuffer = tableBuffers.get(tableName);
1693-
if (tableBuffer != null && tableBuffer.getRowCount() > 0) {
1694-
tableCount++;
1695-
}
1696-
}
1697-
1695+
int tableCount = countNonEmptyTables(keys);
16981696
if (tableCount == 0) {
16991697
pendingBytes = 0;
17001698
pendingRowCount = 0;
@@ -1706,42 +1704,90 @@ private void flushPendingRows() {
17061704
LOG.debug("Flushing pending rows [count={}, tables={}]", pendingRowCount, tableCount);
17071705
}
17081706

1709-
// Ensure activeBuffer is ready for writing
1710-
// It might be in RECYCLED state if previous batch was sent but we didn't swap yet
1707+
// Schema-reset race retry loop. The encoded message carries either
1708+
// full schema definitions or schema-id refs. If the I/O thread
1709+
// performs a reconnect mid-encode (schemaResetNeeded flips while
1710+
// we're emitting refs), the resulting bytes would be poisoned —
1711+
// the new server has no memory of those ids and would reject the
1712+
// batch. Connection-generation tagging closes the window: we read
1713+
// connectionGeneration BEFORE checking schemaResetNeeded, encode,
1714+
// re-read after finishMessage. If the value changed, a reconnect
1715+
// happened mid-encode; discard the encoded bytes (still in the
1716+
// encoder, not yet copied into activeBuffer; table buffers haven't
1717+
// been reset() yet) and retry from the top. Bounded at
1718+
// MAX_SCHEMA_RACE_RETRIES to surface a hard failure if reconnects
1719+
// are pathologically frequent.
17111720
ensureActiveBufferReady();
1712-
1713-
// Encode all non-empty tables into a single QWP v1 message
17141721
int batchMaxSchemaId = maxSentSchemaId;
1715-
encoder.beginMessage(tableCount, globalSymbolDictionary, maxSentSymbolId, currentBatchMaxSymbolId);
1716-
for (int i = 0, n = keys.size(); i < n; i++) {
1717-
CharSequence tableName = keys.getQuick(i);
1718-
if (tableName == null) {
1719-
continue;
1720-
}
1721-
QwpTableBuffer tableBuffer = tableBuffers.get(tableName);
1722-
if (tableBuffer == null || tableBuffer.getRowCount() == 0) {
1723-
continue;
1724-
}
1722+
int messageSize;
1723+
QwpBufferWriter buffer;
1724+
int retries = 0;
1725+
while (true) {
1726+
long genBefore = connectionGeneration;
1727+
if (schemaResetNeeded) {
1728+
schemaResetNeeded = false;
1729+
resetSchemaStateForNewConnection();
1730+
// resetSchemaStateForNewConnection wipes nextSchemaId and
1731+
// every table's cached schema id; recompute batchMaxSchemaId
1732+
// against the fresh maxSentSchemaId.
1733+
batchMaxSchemaId = maxSentSchemaId;
1734+
}
1735+
1736+
// Encode all non-empty tables into a single QWP v1 message.
1737+
// beginMessage calls buffer.reset(), so this is safe to invoke
1738+
// on every retry without any explicit cleanup.
1739+
int currBatchMaxSchemaId = batchMaxSchemaId;
1740+
encoder.beginMessage(tableCount, globalSymbolDictionary, maxSentSymbolId, currentBatchMaxSymbolId);
1741+
for (int i = 0, n = keys.size(); i < n; i++) {
1742+
CharSequence tableName = keys.getQuick(i);
1743+
if (tableName == null) {
1744+
continue;
1745+
}
1746+
QwpTableBuffer tableBuffer = tableBuffers.get(tableName);
1747+
if (tableBuffer == null || tableBuffer.getRowCount() == 0) {
1748+
continue;
1749+
}
17251750

1726-
if (tableBuffer.getSchemaId() < 0) {
1727-
if (nextSchemaId >= maxSchemasPerConnection) {
1728-
throw new LineSenderException("maximum schemas per connection exceeded")
1729-
.put("[maxSchemasPerConnection=").put(maxSchemasPerConnection).put(']');
1751+
if (tableBuffer.getSchemaId() < 0) {
1752+
if (nextSchemaId >= maxSchemasPerConnection) {
1753+
throw new LineSenderException("maximum schemas per connection exceeded")
1754+
.put("[maxSchemasPerConnection=").put(maxSchemasPerConnection).put(']');
1755+
}
1756+
tableBuffer.setSchemaId(nextSchemaId++);
17301757
}
1731-
tableBuffer.setSchemaId(nextSchemaId++);
1758+
currBatchMaxSchemaId = Math.max(currBatchMaxSchemaId, tableBuffer.getSchemaId());
1759+
boolean useSchemaRef = tableBuffer.getSchemaId() <= maxSentSchemaId;
1760+
1761+
if (LOG.isDebugEnabled()) {
1762+
LOG.debug("Encoding table [name={}, rows={}, maxSentSymbolId={}, batchMaxId={}, useSchemaRef={}]", tableName, tableBuffer.getRowCount(), maxSentSymbolId, currentBatchMaxSymbolId, useSchemaRef);
1763+
}
1764+
1765+
encoder.addTable(tableBuffer, useSchemaRef);
17321766
}
1733-
batchMaxSchemaId = Math.max(batchMaxSchemaId, tableBuffer.getSchemaId());
1734-
boolean useSchemaRef = tableBuffer.getSchemaId() <= maxSentSchemaId;
1767+
messageSize = encoder.finishMessage();
1768+
buffer = encoder.getBuffer();
17351769

1770+
// Race detection: did connectionGeneration advance during the
1771+
// encode? If yes, the bytes we just produced may carry stale
1772+
// schema refs.
1773+
if (connectionGeneration == genBefore) {
1774+
batchMaxSchemaId = currBatchMaxSchemaId;
1775+
break;
1776+
}
1777+
retries++;
1778+
if (retries >= MAX_SCHEMA_RACE_RETRIES) {
1779+
throw new LineSenderException(
1780+
"schema-reset race exceeded retry limit [" + MAX_SCHEMA_RACE_RETRIES
1781+
+ "] — reconnects are firing faster than the user thread "
1782+
+ "can encode a single batch");
1783+
}
1784+
// Discard and loop. Table buffers were not reset (that happens
1785+
// only after sealAndSwapBuffer below); the source rows are
1786+
// intact for the next attempt.
17361787
if (LOG.isDebugEnabled()) {
1737-
LOG.debug("Encoding table [name={}, rows={}, maxSentSymbolId={}, batchMaxId={}, useSchemaRef={}]", tableName, tableBuffer.getRowCount(), maxSentSymbolId, currentBatchMaxSymbolId, useSchemaRef);
1788+
LOG.debug("Schema-reset race detected mid-encode; retrying [attempt={}]", retries);
17381789
}
1739-
1740-
encoder.addTable(tableBuffer, useSchemaRef);
17411790
}
1742-
int messageSize = encoder.finishMessage();
1743-
1744-
QwpBufferWriter buffer = encoder.getBuffer();
17451791

17461792
// Copy the single multi-table message to the microbatch buffer and seal
17471793
activeBuffer.ensureCapacity(messageSize);
@@ -1904,6 +1950,21 @@ private long getPendingBytes() {
19041950
return pendingBytes;
19051951
}
19061952

1953+
private int countNonEmptyTables(ObjList<CharSequence> keys) {
1954+
int tableCount = 0;
1955+
for (int i = 0, n = keys.size(); i < n; i++) {
1956+
CharSequence tableName = keys.getQuick(i);
1957+
if (tableName == null) {
1958+
continue;
1959+
}
1960+
QwpTableBuffer tableBuffer = tableBuffers.get(tableName);
1961+
if (tableBuffer != null && tableBuffer.getRowCount() > 0) {
1962+
tableCount++;
1963+
}
1964+
}
1965+
return tableCount;
1966+
}
1967+
19071968
private void resetSchemaStateForNewConnection() {
19081969
maxSentSchemaId = -1;
19091970
nextSchemaId = 0;

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

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.junit.Test;
4141

4242
import java.io.IOException;
43+
import java.lang.reflect.Field;
4344
import java.nio.ByteBuffer;
4445
import java.nio.ByteOrder;
4546
import java.nio.charset.StandardCharsets;
@@ -527,6 +528,162 @@ public void testMultiTableSurvivesReconnect() throws Exception {
527528
handler.frameCount() >= 5);
528529
}
529530

531+
/**
532+
* Schema-reset race protection — between-batches case.
533+
* <p>
534+
* After a (real or simulated) reconnect, {@code connectionGeneration} is
535+
* bumped and {@code schemaResetNeeded} flips. The next user-thread
536+
* {@code flushPendingRows} must observe the bump, reset schema state,
537+
* and emit a fresh batch — server receives a frame carrying full
538+
* schema definitions, not stale refs into the previous connection's
539+
* id space. This covers the simple "reconnect happened, then user
540+
* flushes" path.
541+
*/
542+
@Test
543+
public void testGenerationBumpBetweenBatchesTriggersSchemaReset() throws Exception {
544+
int port = TEST_PORT + 90;
545+
CapturingAckHandler handler = new CapturingAckHandler();
546+
try (TestWebSocketServer server = new TestWebSocketServer(port, handler)) {
547+
server.start();
548+
Assert.assertTrue("server start", server.awaitStart(5, TimeUnit.SECONDS));
549+
550+
try (QwpWebSocketSender sender = QwpWebSocketSender.createForTesting(
551+
"localhost", port,
552+
1, // autoFlushRows = 1 → each atNow ships one batch
553+
QwpWebSocketSender.DEFAULT_AUTO_FLUSH_BYTES,
554+
QwpWebSocketSender.DEFAULT_AUTO_FLUSH_INTERVAL_NANOS,
555+
8)) {
556+
// First batch: server sees a fresh schema definition.
557+
sender.table("foo").longColumn("v", 1L).atNow();
558+
sender.flush();
559+
long deadline = System.currentTimeMillis() + 5_000;
560+
while (System.currentTimeMillis() < deadline && handler.frames.size() < 1) {
561+
Thread.sleep(20);
562+
}
563+
Assert.assertEquals(1, handler.frames.size());
564+
int firstBatchSize = handler.frames.get(0).length;
565+
566+
// Simulate a reconnect: flip schemaResetNeeded and bump
567+
// connectionGeneration via reflection. Closes the loop
568+
// without going through the network — we're testing the
569+
// user-thread side of the contract here.
570+
forceSchemaResetAndBumpGeneration(sender);
571+
572+
// Second batch: must carry a full schema definition again,
573+
// not a ref. Frame should be at least as large as the
574+
// first (definition is strictly heavier than a ref).
575+
sender.table("foo").longColumn("v", 2L).atNow();
576+
sender.flush();
577+
deadline = System.currentTimeMillis() + 5_000;
578+
while (System.currentTimeMillis() < deadline && handler.frames.size() < 2) {
579+
Thread.sleep(20);
580+
}
581+
Assert.assertEquals(2, handler.frames.size());
582+
int secondBatchSize = handler.frames.get(1).length;
583+
Assert.assertTrue(
584+
"post-reset batch must carry a fresh schema definition; "
585+
+ "first=" + firstBatchSize + " bytes, second=" + secondBatchSize
586+
+ " bytes (a ref-only batch would be strictly smaller)",
587+
secondBatchSize >= firstBatchSize);
588+
}
589+
}
590+
}
591+
592+
/**
593+
* Schema-reset race protection — concurrent stress.
594+
* <p>
595+
* Spawn a thread that bumps {@code connectionGeneration} as fast as
596+
* it can while the main thread flushes batches in a tight loop. Any
597+
* landing of a bump during {@code flushPendingRows}' encode window
598+
* must be caught by the post-encode generation re-read and re-driven
599+
* through the retry loop. The test passes as long as no exception
600+
* escapes flush() (other than the bounded MAX_SCHEMA_RACE_RETRIES
601+
* fail-fast, which we tolerate at the very upper end of bumper rates).
602+
*/
603+
@Test(timeout = 30_000)
604+
public void testSchemaResetRaceUnderConcurrentBumps() throws Exception {
605+
int port = TEST_PORT + 91;
606+
CapturingAckHandler handler = new CapturingAckHandler();
607+
try (TestWebSocketServer server = new TestWebSocketServer(port, handler)) {
608+
server.start();
609+
Assert.assertTrue("server start", server.awaitStart(5, TimeUnit.SECONDS));
610+
611+
try (QwpWebSocketSender sender = QwpWebSocketSender.createForTesting(
612+
"localhost", port,
613+
1,
614+
QwpWebSocketSender.DEFAULT_AUTO_FLUSH_BYTES,
615+
QwpWebSocketSender.DEFAULT_AUTO_FLUSH_INTERVAL_NANOS,
616+
8)) {
617+
Field genField = QwpWebSocketSender.class.getDeclaredField("connectionGeneration");
618+
genField.setAccessible(true);
619+
Field resetField = QwpWebSocketSender.class.getDeclaredField("schemaResetNeeded");
620+
resetField.setAccessible(true);
621+
622+
final int batches = 200;
623+
final java.util.concurrent.atomic.AtomicBoolean stopBumper = new java.util.concurrent.atomic.AtomicBoolean(false);
624+
final java.util.concurrent.atomic.AtomicLong bumpCount = new java.util.concurrent.atomic.AtomicLong(0);
625+
Thread bumper = new Thread(() -> {
626+
try {
627+
while (!stopBumper.get()) {
628+
// Throttled: pause so most bumps land between
629+
// batches; a few will land mid-encode and
630+
// exercise the retry path.
631+
Thread.sleep(0, 50_000); // 50 microseconds
632+
resetField.setBoolean(sender, true);
633+
genField.setLong(sender, genField.getLong(sender) + 1);
634+
bumpCount.incrementAndGet();
635+
}
636+
} catch (Exception ignored) {
637+
}
638+
}, "schema-race-bumper");
639+
bumper.setDaemon(true);
640+
bumper.start();
641+
642+
try {
643+
int sent = 0;
644+
LineSenderException maxRetryError = null;
645+
for (int i = 0; i < batches; i++) {
646+
try {
647+
sender.table("foo").longColumn("v", (long) i).atNow();
648+
sender.flush();
649+
sent++;
650+
} catch (LineSenderException e) {
651+
// The only acceptable exception is the
652+
// bounded retry-limit fail-fast — bumper is
653+
// running flat-out so it can occasionally
654+
// win 10 races back-to-back.
655+
if (e.getMessage() != null
656+
&& e.getMessage().contains("schema-reset race exceeded retry limit")) {
657+
maxRetryError = e;
658+
break;
659+
}
660+
throw e;
661+
}
662+
}
663+
Assert.assertTrue(
664+
"bumper must have fired at least once; bumps=" + bumpCount.get(),
665+
bumpCount.get() > 0);
666+
Assert.assertTrue(
667+
"either every batch shipped or the retry-limit fail-fast tripped; "
668+
+ "sent=" + sent + ", maxRetryError=" + maxRetryError,
669+
sent == batches || maxRetryError != null);
670+
} finally {
671+
stopBumper.set(true);
672+
bumper.join(5_000);
673+
}
674+
}
675+
}
676+
}
677+
678+
private static void forceSchemaResetAndBumpGeneration(QwpWebSocketSender sender) throws Exception {
679+
Field genField = QwpWebSocketSender.class.getDeclaredField("connectionGeneration");
680+
genField.setAccessible(true);
681+
Field resetField = QwpWebSocketSender.class.getDeclaredField("schemaResetNeeded");
682+
resetField.setAccessible(true);
683+
resetField.setBoolean(sender, true);
684+
genField.setLong(sender, genField.getLong(sender) + 1);
685+
}
686+
530687
/**
531688
* sf_fsync_on_flush=off (default): the user's flush() must NOT call
532689
* segmentLog.fsync(). Pre-fix the docs claimed an fsync happened on

0 commit comments

Comments
 (0)