core/src/main/java/io/questdb/cairo/TableWriterSegmentFileCache.java (1)

208-209: Good call capturing pre-mapping size for precise cleanup

Saving initialSize before mapping ensures the catch path targets only newly added mappings for cleanup, avoiding both leaks and accidental over-closing.

The earlier suggestion to use walMappedColumns.size() in the catch would have resulted in no-op cleanup for newly added columns. Using initialSize is correct here.

core/src/test/java/io/questdb/test/cairo/BitmapIndexConcurrentTest.java (3)

179-183: Avoid String.format in hot loop to reduce allocations

This is a tight loop; String.format allocates and is slower. Simple concatenation is sufficient and cheaper.

Apply this diff:

-                                String updateSql = String.format(
-                                        "UPDATE trades SET symbol = '%s' WHERE id = %d",
-                                        randomSymbol, randomId
-                                );
+                                String updateSql = "UPDATE trades SET symbol = '" + randomSymbol + "' WHERE id = " + randomId;

---

172-194: Optional: mirror per-thread execution context for readers to eliminate any shared-context risk

Writers and updaters now use thread-local contexts; readers still share sqlExecutionContext (Line 229). It may be safe, but for symmetry and to preempt subtle races, consider a per-thread context for readers too.

Example (outside the selected lines, for the reader runnable body):

try (var readCtx = TestUtils.createSqlExecutionCtx(engine)) {
    while (!stopFlag.get() && errorCount.get() == 0) {
        // ...
        try (RecordCursorFactory factory = select(querySql)) {
            try (RecordCursor cursor = factory.getCursor(readCtx)) {
                // ...
            }
        }
    }
}

---

281-281: Harden executor shutdown with a forced fallback to avoid lingering threads on failure

If termination exceeds 60s, issue shutdownNow and wait briefly again. This prevents leaked threads when an assertion fails.

Apply this diff:

-        Assert.assertTrue("failed to terminate threads within 60s", executor.awaitTermination(60, TimeUnit.SECONDS));
+        if (!executor.awaitTermination(60, TimeUnit.SECONDS)) {
+            executor.shutdownNow();
+            Assert.assertTrue(
+                    "failed to terminate threads within 60s (forced shutdown issued)",
+                    executor.awaitTermination(10, TimeUnit.SECONDS)
+            );
+        }

core/src/main/java/io/questdb/cairo/TableWriter.java (1)

7655-7660: Nit: tighten the comment wording for clarity

Minor grammar/wording improvements make the intent clearer and reference the double-close guard explicitly.

-        // Don't move the line to mmap Wal column inside the following try block,
-        // This call, if failed will close the WAL files correctly on its own
-        // putting it inside the try block will cause the WAL files to be closed twice in the finally block
-        // in case of the exception.
+        // Important: do not move the mmapWalColumns call inside the try block below.
+        // If mmapWalColumns fails, it closes the WAL files itself.
+        // Putting it inside the try would cause the finally block to close the WAL files again,
+        // tripping the FdCache double-close guard in case of an exception.
         segmentFileCache.mmapWalColumns(segmentCopyInfo, metadata, path);

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

core/src/main/java/io/questdb/cairo/TableWriterSegmentFileCache.java (1)

303-314: Critical: stale fdCacheKey removal in finally can double-remove and corrupt walFdCache state

If mmap fails, the catch path calls closeWalFiles(true, ...), which removes the cached FD entry and returns the LongList to the pool. The finally block still executes walFdCache.removeAt(fdCacheKey) using the stale index and decrements walFdCacheSize again. This risks:

• Removing a different entry (stale index) from walFdCache
• Double-decrementing walFdCacheSize
• Double-pushing the same LongList to the pool (if fds is still non-null in other paths)

Re-resolve the key in finally and only remove if the key still exists. Also avoid double-pushing by handling the fds reference accordingly.

Apply this diff:

-            // Now that the FDs are used in the column objects, remove them from the cache.
-            // to avoid double close in case of exceptions.
-            if (fdCacheKey < 0) {
-                walFdCache.removeAt(fdCacheKey);
-                walFdCacheSize--;
-            }
-
-            if (fds != null) {
-                fds.clear();
-                walFdCacheListPool.push(fds);
-            }
+            // Remove cached FDs only if they still exist (they may have been removed in the catch branch).
+            final int currentKey = walFdCache.keyIndex(walSegmentId);
+            if (currentKey < 0) {
+                final LongList cached = walFdCache.valueAt(currentKey);
+                walFdCache.removeAt(currentKey);
+                walFdCacheSize--;
+                cached.clear();
+                walFdCacheListPool.push(cached);
+            } else if (fds != null) {
+                // No cache entry remains; just return the list we took earlier, if any.
+                fds.clear();
+                walFdCacheListPool.push(fds);
+            }

core/src/main/java/io/questdb/cairo/TableWriterSegmentFileCache.java (1)

297-301: Using initialSize for partial rollback is correct

Passing initialSize to closeWalFiles(true, walSegmentId, initialSize) closes only the newly mapped columns within this method, which is the intended partial rollback behavior. The explicit walMappedColumns.setPos(initialSize) is redundant given closeWalFiles already sets the position but harmless.

If you want to reduce redundancy, you can remove the setPos(initialSize) line since closeWalFiles already handles it.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.