fix(core): fix a potential database crash when under a load of a prolonged period of time#6082
fix(core): fix a potential database crash when under a load of a prolonged period of time#6082bluestreak01 merged 29 commits intomasterfrom
Conversation
…onged period of time
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors FD/mmap caching to use generated mmap cache keys instead of FD-derived keys, updates Files/MmapCache/FdCache accordingly, adds mmap/remap errno-aware error handling in transaction log cursor, relocates a WalTxnDetails helper, and adds tests for FD cache counter wraparound. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Files
participant FdCache
participant MmapCache
Caller->>Files: mmap(fd, len, offset, flags, tag)
Files->>FdCache: toMmapCacheKey(fd)
FdCache-->>Files: mmapCacheKey
Files->>MmapCache: cacheMmap(fd, mmapCacheKey, len, offset, flags, tag)
alt cache miss
MmapCache->>MmapCache: mmap0(fd, len, offset, flags)
MmapCache->>MmapCache: create record and cache under mmapCacheKey
else cache hit
MmapCache->>MmapCache: inc refcount, return existing address
end
MmapCache-->>Files: address
Files-->>Caller: address
sequenceDiagram
autonumber
participant Caller
participant TransactionLogCursorImpl as MmapUser
participant OS
note over MmapUser: mmap/remap with errno-aware error handling
Caller->>MmapUser: of(...) / remap(...)
MmapUser->>OS: mmap0/remap0(...)
OS-->>MmapUser: address or -1
alt address == -1
MmapUser-->>Caller: throw CairoException.critical(Os.errno())
else success
MmapUser->>MmapUser: set address
MmapUser-->>Caller: return success
end
sequenceDiagram
autonumber
participant Caller
participant Files
participant MmapCache
Caller->>Files: mremap(fd, addr, prevSize, newSize, offset, flags, tag)
Files->>FdCache: toMmapCacheKey(fd)
FdCache-->>Files: mmapCacheKey
Files->>MmapCache: mremap(fd, mmapCacheKey, addr, prevSize, newSize, offset, flags, tag)
alt prevSize == 0
MmapCache->>MmapCache: cacheMmap(fd, mmapCacheKey, newSize, offset, flags, tag)
else extend existing
alt refcount == 1
MmapCache->>MmapCache: mremap0(addr, prevSize→newSize) and update record
else refcount > 1
MmapCache->>MmapCache: mmap0 new region, dec old refcount, cache new under mmapCacheKey
end
end
MmapCache-->>Files: newAddress
Files-->>Caller: newAddress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@jerrinot we could expose test-only |
|
an unrelated failure due to #1841 |
…. 64bits counters are effectively guaranteed not to overflow.
This is now done, four tests added that directly wrap-around the FD counter and test file opening, closing and reading. All confirmed to fail on master. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/main/java/io/questdb/std/FdCache.java (1)
126-136: Apply the same collision-handling safety to stdout FD creation.The same infinite loop concern applies here.
🧹 Nitpick comments (3)
core/src/main/java/io/questdb/std/FdCache.java (1)
106-115: Ensure collision handling doesn't lead to infinite loops.While the loop handles wraparound collisions correctly, there's a theoretical risk of infinite looping if all possible index values are occupied. Consider adding a safety counter to prevent infinite loops in pathological cases.
public synchronized long createUniqueFdNonCached(int fd) { if (fd > -1) { long markedFd; int keyIndex; + int attempts = 0; + final int maxAttempts = 1000; // reasonable upper bound // loop to avoid collisions when index wraps over do { markedFd = Numbers.encodeLowHighInts(nextIndex() | NON_CACHED_MASK, fd); keyIndex = openFdMapByFd.keyIndex(markedFd); - } while (keyIndex < 0); + } while (keyIndex < 0 && ++attempts < maxAttempts); + + if (attempts >= maxAttempts) { + throw new IllegalStateException("FD cache exhausted after " + maxAttempts + " attempts"); + } openFdMapByFd.putAt(keyIndex, markedFd, FdCacheRecord.EMPTY);core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java (2)
126-129: Use the openRW helper consistently or remove itMinor consistency nit: you define openRW(Path) but call Files.openRW(...) directly in two places.
Apply:
- rwB = Files.openRW(testFileB.$()); + rwB = openRW(testFileB);- rwA = Files.openRW(testFileA.$()); + rwA = openRW(testFileA); @@ - rwB = Files.openRW(testFileB.$()); + rwB = openRW(testFileB);Alternatively, delete the unused helper if you prefer direct calls everywhere.
Also applies to: 184-196, 229-233
103-107: Method name readabilityConsider renaming to testOpenATwiceCloseOnceThenOpenBAsRwThenReadA for clarity.
- public void testOpenATwiceThenCloseOnceThenOpenBrwThenReadA() throws Exception { + public void testOpenATwiceCloseOnceThenOpenBAsRwThenReadA() throws Exception {
📜 Review details
Configuration used: Path: .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.
📒 Files selected for processing (6)
core/src/main/java/io/questdb/cairo/wal/WalTxnDetails.java(1 hunks)core/src/main/java/io/questdb/cairo/wal/seq/TableTransactionLogV1.java(3 hunks)core/src/main/java/io/questdb/std/FdCache.java(10 hunks)core/src/main/java/io/questdb/std/Files.java(4 hunks)core/src/main/java/io/questdb/std/MmapCache.java(6 hunks)core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (13)
core/src/main/java/io/questdb/cairo/wal/seq/TableTransactionLogV1.java (2)
373-378: LGTM! Robust error handling for mmap failure.The explicit error handling with
Os.errno()reporting provides precise diagnostics for mmap failures. This aligns well with the PR's goal of hardening the FD cache against failures.
391-396: LGTM! Consistent error handling for remap operation.The remap error handling follows the same pattern as mmap, ensuring consistency and providing clear diagnostics via
Os.errno().core/src/main/java/io/questdb/cairo/wal/WalTxnDetails.java (1)
109-131: LGTM! Well-structured helper method with proper bounds checking.The relocated
loadTxnsmethod maintains the original logic while adding an assertion to ensure at least one transaction is loaded. The radix sort with computed min/max bounds is efficient.core/src/main/java/io/questdb/std/FdCache.java (3)
196-204: LGTM! Proper handling of FD counter wraparound for cached files.The collision detection loop ensures unique FD generation even when the counter wraps around, which is crucial for the PR's goal of handling long-running systems.
306-310: LGTM! Clean separation of concerns with the NON_CACHED_MASK.The
nextIndex()method properly masks out the NON_CACHED bit, ensuring clean indices for wraparound handling.
47-48: LGTM! Clean transition from FD-based to key-based mmap caching.The introduction of
mmapKeyGeneratorand the transition tommapCacheKeyeffectively decouples the mmap cache from FD values, preventing the misassociation issues mentioned in the PR description. The atomic long ensures thread-safe unique key generation.Also applies to: 272-279, 295-295, 327-330
core/src/main/java/io/questdb/std/Files.java (3)
347-350: LGTM! Consistent usage of the new mmap cache key API.The transition from FD-based to key-based mmap caching is properly implemented, obtaining the key via
fdCache.toMmapCacheKey(fd).
362-366: LGTM! Consistent key usage in mremap.The mremap operation correctly uses the new mmap cache key approach.
537-540: Test-only API is appropriately marked.The
setFDCacheCountermethod is properly annotated with@TestOnlyand provides necessary test control for simulating wraparound scenarios.core/src/main/java/io/questdb/std/MmapCache.java (3)
42-44: LGTM! Clean transition to mmapCacheKey throughout.The parameter rename from
fileCacheKeytommapCacheKeyand its consistent usage throughout the caching logic properly implements the new key-based approach. The documentation updates clearly explain the purpose of the key.Also applies to: 48-49, 74-76
175-185: LGTM! Good defensive comment explaining reference count handling.The comment clearly explains why the reference count is only decremented on success, maintaining the semantic that callers are responsible for closing the old mapping even on failure.
162-195: Robust remapping logic with proper reference counting.The remap logic correctly handles both single-use (count==1) and shared (count>1) mappings, with appropriate assertions and error handling. The separation of concerns between updating cache state and performing the actual unmap operation is well-designed.
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java (1)
45-205: Good targeted regression coverage for FD-cache wraparoundScenarios are well-chosen and reproduce the failure modes described in the PR. Nice use of assertMemoryLeak to guard native resources.
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@puzpuzpuz I'll review the changes in this PR that addresses the FD cache counter wrap-around issue. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java (4)
118-127: Guard against zero-length files before mmap in readersA defensive check avoids platform-specific EINVAL when mapping length 0 (future refactors could change initial file size).
- long size = Files.length(readFd); + long size = Files.length(readFd); + if (size <= 0) { + // File might be empty; try again in the next iteration. + Os.sleep(1); + continue; + }
96-101: Validate full append or loop until all bytes are writtenIf
Files.appendcan return a short write on some platforms, ensure the full buffer is appended to keep the last-byte read stable under load.- final long res = Files.append(appendFd, buf, appendData.length); - Assert.assertTrue("Files.append failed: " + Os.errno(), res > -1); + int remaining = appendData.length; + long p = buf; + while (remaining > 0) { + final long res = Files.append(appendFd, p, remaining); + Assert.assertTrue("Files.append failed: " + Os.errno(), res > -1); + remaining -= (int) res; + p += res; + }
48-51: Consider aligning FILE_SIZE with OS page size for mmap realismUsing ≥ one page improves fidelity for mmap-heavy paths; you can still cap verification loops to keep tests fast.
- private static final int FILE_SIZE = 1024; + // Use at least one page to better exercise page-aligned mmap behavior. + private static final int FILE_SIZE = Math.max(1024, Os.PAGE_SIZE);
149-160: Report all thread exceptions, not only the last oneFailing with the first encountered exception simplifies triage and avoids overwriting earlier errors.
- Throwable lastException = null; - // Check for exceptions - for (Throwable e : exceptions) { - if (e != null) { - e.printStackTrace(System.out); - lastException = e; - } - } - if (lastException != null) { - throw new AssertionError(lastException); - } + for (int i = 0; i < exceptions.length; i++) { + if (exceptions[i] != null) { + exceptions[i].printStackTrace(System.out); + throw new AssertionError("Unhandled exception in " + + (i == exceptions.length - 1 ? "writer" : "reader[" + i + "]"), + exceptions[i]); + } + }
📜 Review details
Configuration used: Path: .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.
📒 Files selected for processing (1)
core/src/test/java/io/questdb/test/std/FdCacheCounterOverflowTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
[PR Coverage check]😍 pass : 63 / 66 (95.45%) file detail
|
|
a failure due to #6062 |
5 participants
In long-running nodes under sustained write load, an internal file-descriptor cache could wrap its monotonic counter and misassociate cached FDs. In unlucky timing, this could cascade into a crash. This PR hardens the FD cache against wrap-around and tightens memory-mapping error handling.