Release a purged transaction-log segment's mapping instead of refusing to read it - #820
Open
kriszyp wants to merge 2 commits into
Open
Release a purged transaction-log segment's mapping instead of refusing to read it#820kriszyp wants to merge 2 commits into
kriszyp wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a robust cache revalidation mechanism for JS memory-map caches against the native store's purge epoch, preventing stale reads of purged transaction-log segments. It also hardens databaseFlushed() to verify the physical existence of txn.state on disk rather than relying solely on the stream's open status. The feedback recommends using performance.now() instead of Date.now() in tests to ensure a monotonic clock, and replacing new Uint32Array with readUInt32LE on read buffers to avoid potential alignment errors.
Contributor
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit a69721f |
kriszyp
force-pushed
the
fix/txnlog-purge-read-coherence
branch
2 times, most recently
from
September 3, 2026 06:08
6855e4b to
9398f6b
Compare
kriszyp
marked this pull request as ready for review
September 4, 2026 05:03
A purge unlinks the segment, which removes one link to an inode whose bytes a retired segment never changes again; a reader's MemoryMap is the other link, so the entries it mapped are still exactly the committed history and stay readable. The bug in HarperFast/harper#2337 was never that those bytes were served — it was that the mapping was never released, so the purge reclaimed no space: 16 MiB of a deleted .txnlog resident until restart. TransactionLog._currentLogBuffer, the fast path over the already-weak _logBuffers cache, held a strong reference and is only refreshed by query(), so a reader that calls query() once and next() forever (harper's audit subscription) froze it on whatever segment was current then. It is now a WeakRef: the mapping goes at the next GC once the iterator holding it moves on, with no purge-time invalidation and no cross-handle signalling. Also here, because they are the same reclaim path: - nextReadableLogBuffer() skips a run retention deleted when an iterator advances. Stopping at the hole stopped the iterator permanently, since every later poll stopped in the same place. _findPosition(0) names the oldest survivor, so a purged prefix costs one native call rather than a probe per segment, and only a run the store no longer has is skipped: a segment it still knows is merely unmappable for now, so iteration stops and retries. - readableExtent() bounds a read of a purged segment by its mapping, since the store reports no size for a segment it has forgotten. The 0 it reports dropped every entry the reader had not reached yet — including entries appended after it last polled, which the writer's overlay extension made visible in that same mapping. - removeFile() uses the non-throwing std::filesystem::remove overloads on both platforms; a Windows sharing violation used to unwind a C++ exception through the N-API purge boundary. - A segment that vanished between the purge's scan and its unlink is forgotten from sequenceFiles the way the scan forgets an already-missing one, and a segment that could not be deleted for a real reason is reported once per purge run via log.warn instead of silently stalling retention. Refs HarperFast/harper#2337 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…is_open()
databaseFlushed() keeps the flushed-state stream open across flushes, and a
stream describes a descriptor, not a pathname: once txn.state (or the whole
store directory) is unlinked, every write lands in the orphaned inode while
getLastFlushedPosition(), which reads by path, returns the {0,0} sentinel and
retention never advances.
The pathname is now checked before the unchanged-position shortcut, since a
flush resolving to the already-recorded position must still restore a missing
file. The directory is recreated the way getLogFile() does, isClosing is
re-checked under flushedStateMutex so a concurrent destroy cannot be
resurrected, and the reopen is in-place (in | out) rather than truncating: the
8-byte record is overwritten whole, and a truncating reopen after a failed write
would erase the last durable position before a retry that can fail again. The
creating open is taken only after the file is verified absent.
This runs on RocksDB's flush thread, where an escaping exception ends the
process, so the whole rewrite sits behind a catch-all and every failure is
reported once via log.warn, leaves lastWrittenFlushedPosition untouched, and is
retried on the next flush.
Hardening rather than a live bug: only purgeLogs({ destroy: true }) removes the
directory today, and Harper does not call it in production.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cb1kenobi
reviewed
Sep 4, 2026
kriszyp
force-pushed
the
fix/txnlog-purge-read-coherence
branch
from
September 4, 2026 05:46
9398f6b to
def1b22
Compare
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
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.
purgeLogs()unlinks a retired segment. That removes one link to an inode whose bytes aretired segment never changes again; a reader's
MemoryMapis the other link, so the entries itmapped are still exactly the committed history and are still perfectly readable. The bug in
HarperFast/harper#2337 was never that those bytes were served — it was that the mapping was
never released, so the purge reclaimed no space: a 16 MiB mapping of a deleted
.txnlogresident until restart.
This is a rewrite of the branch. The earlier revision published a per-store "purge epoch" that
every reader revalidated against, dropped its caches on, and relocated off deleted segments —
several hundred lines of native + JS coherence machinery whose whole premise (a purged segment is
no longer authoritative) was wrong. All of it is gone; the two commits here are what is left once
the goal is stated as release the mapping rather than stop serving it.
The fix
TransactionLog._currentLogBufferis aWeakRef. It is the fast path over the per-segment_logBufferscache — which was already weak — and it is only ever refreshed byquery(), so along-lived reader that calls
query()once andnext()forever (harper's audit subscription)froze it on whatever segment was current then, and retention later deleted exactly that segment.
With both weak, the mapping is released at the next GC once the iterator holding it has moved on:
no purge-time invalidation, no cross-handle signalling — which matters, because the store is
process-global and every handle and
worker_threadsworker has its own JS caches.nextReadableLogBuffer()skips a deleted run when an iterator advances. Those segments aregenuinely gone, and stopping at the hole stopped the iterator permanently — every later poll
stopped in the same place, so a reader that fell behind the retention floor never saw another
entry.
_findPosition(0)names the oldest survivor, so a purged prefix costs one native callrather than a probe per deleted segment. Only a run the store no longer has is skipped: if
_findPosition(0)does not resolve past the hole, the store still knows that segment and it ismerely unmappable right now (mid-rotation, 0 bytes at mmap time, transient resource pressure), so
iteration stops and picks it up on the next poll rather than stepping over durable history. Both
call sites already know a later segment exists, so the healthy path is unchanged.
readableExtent()bounds a read of a purged segment by its own mapping. The store forgets apurged segment, so
getLogFileSize()reports 0 for it — and taking that 0 as the bound droppedevery entry the reader had not reached yet, including entries appended to the segment after the
reader last polled, which the writer's in-place overlay extension made visible in that very
mapping. The fallback walks the frame headers to the zero-timestamp end marker
(
endOfEntries()), cached on the buffer, so iteration reads them all and then advances to thenext surviving segment instead of stopping on the zero fill. Broken framing yields the whole
mapping instead, leaving the break for the read path to report with a resync point — this scan
must not be the thing that decides a corrupt frame ends the log (invariant 11). The same
reasoning gives an uncommitted read's
corruptFrame()a realdataEndfor a purged segmentinstead of 0.
removeFile()uses the non-throwingstd::filesystem::removeoverloads on both platforms (a Windows sharing violation used to unwinda C++ exception through the N-API purge boundary), a segment that vanished between the purge's
scan and its unlink is forgotten from
sequenceFilesthe way the scan forgets an already-missingone, and a segment that could not be deleted for a real reason is reported once per purge run via
log.warninstead of silently stalling the retention floor.On Windows a live mapping makes the unlink fail outright, so the purge skips that segment and
retention stalls until the buffer is collected; the loop is refused unlink → collection → the next
purge succeeds. That is the one place a human eye is worth more than CI (see below).
Second commit (independent, droppable)
databaseFlushed()keeps itstxn.statestream open across flushes, and a stream describes adescriptor, not a pathname: once the file — or the whole store directory — is unlinked, every write
lands in the orphaned inode while
getLastFlushedPosition(), which reads by path, returns the{0,0}sentinel and retention never advances. The pathname is now checked before theunchanged-position shortcut (a flush resolving to the already-recorded position must still restore
a missing file), the directory is recreated the way
getLogFile()does,isClosingis re-checkedunder
flushedStateMutexso a concurrent destroy cannot be resurrected, and the reopen is in place(
in | out) rather than truncating — the 8-byte record is overwritten whole, and a truncatingreopen after a failed write would erase the last durable position before a retry that can fail
again. It runs on RocksDB's flush thread, where an escaping exception ends the process, so the
whole rewrite sits behind a catch-all: every failure warns once via
log.warn, leaveslastWrittenFlushedPositionuntouched, and is retried on the next flush.Hardening, not a live bug: only
purgeLogs({ destroy: true })removes the directory today, andHarper does not call it in production.
For the human reviewer
stated invariant (AGENTS.md 18), not an accident. A reader mid-iteration finishes the segment it
mapped and then moves on; the test
should finish the segment it mapped before the purgeexiststo keep anyone (including a future me) from "fixing" that again.
purgeLogs({ destroy: true })removes the store directory and a freshstore restarts segment numbering at 1, so a buffer cached by segment number can answer for a
different store's file. That is a cache-key identity problem, not a purge-coherence one; it is
pre-existing, and Harper does not call
destroyin production. The previous revision tried tosolve it with store ids and reader rebinding, and every round of review found another race in
that machinery.
(
should converge on a purge refused by a live mapping) runs only on win32 and only with--expose-gc; if the external buffer is not collected promptly there, retention stalls until itis. Everything else in the new suite is POSIX-only, since Windows cannot delete a mapped file at
all.
log.warnfor an undeletable segment is once per purge run, not once per process — adirectory that stays unwritable would otherwise stall retention behind a single log line ever.
txn.stateis overwritten in place, so a torn writecould publish a mixed record. The record is 8 bytes (position, sequence) rewritten at offset
0; a short or torn overwrite could leave one field new and the other old. Both fields only ever
advance, and a purge reads the file under the same
flushedStateMutexthe write holds, soin-process no reader sees a half-written record and the two safe mixes are conservative; the
dangerous one is
(new sequence, old position)where the old position is further into itssegment than the new one, which would let a purge treat an unflushed tail of the current segment
as flushed. It needs a torn 8-byte overwrite inside one sector to happen at all, and
mainhasthe same write (this branch only makes the reopen non-truncating, which strictly reduces the
exposure). An atomic publish — temp file plus rename, or a checksummed slot — is the real fix and
belongs in its own change, since the file has no
fsynctoday either.flagged that a purged segment's
getLogFileSize()of 0 truncated a live mapping's read, that anunmappable-but-registered segment was indistinguishable from a deleted one, and that the
original per-segment walk was unbounded. All three are fixed above; the first is the same
invariant this PR exists to state, so it is a fix rather than a follow-up.
Verification
main, passes here:should resume past a purged run rather than stopping at the holeand
should release the mapping of a purged segment without a restart(Linux,--expose-gc;reads
/proc/self/mapsfor1.txnlog (deleted)after purge + GC). Confirmed by reverting onlysrc/transaction-log-reader.ts+src/load-binding.tstomain: 2 failed, 1 passed — the onethat passes is the "still serves what it mapped" case, which is
main's behavior and must staythat way.
should yield entries appended to the segment it mapped before the purgeisolates the extentfallback: it fails with
readableExtent()reduced to the rawgetLogFileSize()and passes withthe fallback in place.
TransactionLogFlushedStateGoogleTests fail onmainfor both the file-only andthe whole-directory unlink; the
flushSync()restore test intransaction-log.test.tsfails onmaintoo.query({start:0})+ onenext()per op, 5x100k per run, 3 alternating rounds): strong~2.20-2.34M ops/s, weak ~2.10-2.22M ops/s — roughly 4% on a loop that pays the cache miss on
every op. A real reader amortizes it over many
next()calls perquery(), and the WeakRef isonly re-wrapped when the segment changes, so the steady state costs a
deref()rather than anallocation.
pnpm test(Node 26, Linux): 837 passed, 4 skipped, 0 failed.pnpm test:native: 163 passed.pnpm checkclean. Bun/Deno/Windows not run locally; CI covers them.Refs HarperFast/harper#2337
Complexity: moderate
Review-Coverage: authored=codex; ran=gemini,claude; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=1 @ def1b22
Human-Review-Need: 4 (decisions: lagging-reader-gap-policy, purged-mapping-lifetime, txn-state-repair-layer) @ def1b22