Release frozen transaction-log memory maps once JS releases them#653
Open
kriszyp wants to merge 3 commits into
Open
Release frozen transaction-log memory maps once JS releases them#653kriszyp wants to merge 3 commits into
kriszyp wants to merge 3 commits into
Conversation
A TransactionLogFile held a strong reference to its memory map for the object's whole life, so a log file's mmap stayed resident until the file was closed, removed, or remapped — even after every JS reference to its external buffer was gone. During replication catch-up this retains the full set of read log maps. Hold the strong reference only for the current (actively-written) file — the writer extends its overlay and the timestamp index reads through it. A frozen (rotated-out) file keeps only a weak handle, so the JS external buffer becomes the sole owner and the mapping is unmapped when JS GCs the buffer. On rotation, the file being rotated away from is downgraded to the weak handle so a reader that mapped it while it was current no longer pins it. store/handle getMemoryMap now return a strong shared_ptr (the buffer adopts ownership). Windows keeps the strong-retain behavior (not the memory-pressure target; its open path would self-deadlock under fileMutex). getMemoryMap now (re)assigns the map under fileMutex, which also closes the memoryMap shared_ptr data race deferred from the MADV_COLD change (#652): adviseCold/close/removeFile already hold fileMutex, and the lock order (dataSetsMutex/indexMutex -> fileMutex) is consistent and deadlock-free. A process-global MemoryMap::liveCount (exposed as transactionLogMapCount()) lets tests assert that releasing a frozen log's buffer actually unmaps it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The "unmaps a frozen log when all JS references are released" test asserts the weak-for-frozen behavior, which is POSIX-only — Windows getMemoryMap retains the map strongly by design, so a directly-mapped frozen log is not released until the file is closed/removed. The sibling rotation test still runs everywhere (downgradeMapToFrozen is platform-agnostic). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
It is written on the write path (under writeMutex) but read on the read path (getMemoryMap/findPositionByTimestamp under dataSetsMutex) — different locks, so the plain uint32_t accesses were a data race (flagged in review). Make it std::atomic<uint32_t> with relaxed ordering: readers only need a coherent value, and the existing mutexes already provide ordering against other state. Pre-existing; surfaced by the rotateToNextSequence refactor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworks transaction-log memory-map ownership so a frozen (rotated-out) log file no longer pins its mmap for the life of the
TransactionLogFile:memoryMap(the writer extends its overlay; the timestamp index reads through it).frozenMapCache; the strong reference is handed to the caller and thus to the JS external buffer, so the mapping ismunmap'd when JS GCs the buffer.rotateToNextSequence(), so a reader that mapped it while it was current no longer pins it.store/handlegetMemoryMapnow return a strongshared_ptr(the buffer adopts ownership);GetMemoryMapOfFileuses it directly.Purpose
Follow-up to #650/#652. PR #652 (
MADV_COLD) makes read txn-log pages reclaimable under pressure; this PR lets them be freed outright when the reader is done — during replication catch-up the joining/source node reads the full log, and previously every read frozen map stayed resident until the file was closed/removed.It also resolves #652's deferred concurrency finding:
getMemoryMapnow (re)assignsmemoryMapunderfileMutex, matchingadviseCold/close/removeFile, so theshared_ptrread/write race is closed.Where to look
transaction_log_file_posix.cppgetMemoryMap): now holdsfileMutexfor its whole body. Order isdataSetsMutex/indexMutex → fileMutex, consistent with the existingstore.getMemoryMap → open()path; nofileMutexholder takes the outer locks. Worth a second pair of eyes.getMemoryMapdeliberately does not takefileMutex— Windowsopen() → openFile() → findPositionByTimestamp() → getMemoryMap()already holds it, so locking there would self-deadlock (POSIXopenFiledoesn't index). Commented inline; the pre-existing Windows reassignment race is left as-is (not the memory-pressure target).transaction_log_store.cpprotateToNextSequence): applied at all 5writeBatchrotation sites; the purge advance is excluded because it removes the file (map reset on removal).Cross-model review
Codex (correctness/concurrency) + a Harper-domain pass. Both findings from Codex are addressed in this PR: the
shared_ptrrace (thefileMutexserialization above) and a test-baseline-stability flake (the finalization test now flushes earlier finalizers before sampling). The rotation-downgrade hole Codex flagged is fixed here (it was the originally-deferred tradeoff). The domain pass found no blockers.Caveat: the Gemini/
agyreview leg hung and produced no output (same as #652) — no Gemini coverage this round; Codex + the domain pass are the cross-model coverage.Tests: native GoogleTest ownership cases (frozen released on drop, current retained then freed on file destroy, downgrade releases on drop, frozen handout dedup) + Vitest finalization tests (a frozen buffer and a mapped-while-current-then-rotated buffer are both unmapped after release, asserted via
transactionLogMapCount()). Full suite green (485 passed; 37 native).Generated by an LLM (Claude Opus 4.7); reviewed/curated by Kris.