Release a purged transaction-log segment's mapping instead of refusing to read it - #820
Open
kriszyp wants to merge 12 commits into
Open
Release a purged transaction-log segment's mapping instead of refusing to read it#820kriszyp wants to merge 12 commits into
kriszyp wants to merge 12 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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 89fb394 |
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
changed the title
kriszyp
marked this pull request as ready for review
September 4, 2026 05:03
cb1kenobi
reviewed
Sep 4, 2026
kriszyp
force-pushed
the
fix/txnlog-purge-read-coherence
branch
2 times, most recently
from
September 9, 2026 14:33
7751c8f to
20d25cf
Compare
cb1kenobi
reviewed
Sep 9, 2026
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>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
AGENTS.md said oxfmt formats "TS/JS/JSON only" and does "not touch C++ or Markdown". It does format Markdown, including AGENTS.md — that file is what the failing `Check` job named, and renumbering its ordered invariant list is what oxfmt objected to. Believing the doc is why the failure was first read as a hand-fixable numbering slip. Also record the trap the doc hid: a `pull_request` build formats the merge commit, so a branch whose own `fmt:check` is green fails CI whenever it and main have each appended an invariant and the numbers collide. Verified: oxfmt scans .md and skips .cpp entirely, renumbers `19, 19, 20` to `19, 20, 21`, and leaves lazy `1.` numbering alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GePs22dNhggr8XKDc1DThG
kriszyp
force-pushed
the
fix/txnlog-purge-read-coherence
branch
from
September 9, 2026 21:53
93facff to
112c6cf
Compare
cb1kenobi
reviewed
Sep 9, 2026
…ating one
Two independent defects in the purge-advance path, both found by the pre-push
review of this branch.
nextReadableLogBuffer() asked _findPosition(0) for "what comes after N". That
walks backward from the current sequence and stops at the first gap, so it
names the bottom of the contiguous run ending at the current segment. That is
the oldest survivor only when the deletions form a single prefix. With a
survivor between two holes - a purge({all}) that continued past a segment it
could not unlink, or segments deleted out of band and registered that way at
load - it lands past the survivor, and that segment's committed entries are
never yielded to the reader. Silently: the skip has no signal.
_nextLogId() (TransactionLogStore::nextSequenceAfter, sequenceFiles.upper_bound)
answers the question actually being asked, in one O(log n) lookup. The probe is
a bounded loop rather than one hop, because a registered successor can be absent
too - unlinked out of band, or by another process's retention, before this
process's purge run forgets it - and stopping at the first one that will not map
is the same permanent wedge. It terminates because _nextLogId() strictly
increases and is capped at the latest sequence.
Separately, resolving a registered-but-closed segment opened it, and
TransactionLogFile::open() creates (O_RDWR | O_CREAT, and OPEN_ALWAYS on
Windows). Probing a segment that discovery registered but that is no longer on
disk therefore recreated it as a header-only ghost that the next startup
registers again. openIfPresent() skips a definite absence, using the same
reasoning ensureExtent() already documents - only a definite absence skips,
since a stat that errors leaves the extent unresolved - and is meaningful
because dataSetsMutex is held across the check and the open. All three paths
that resolve a segment go through it: getLogFileSize(), getMemoryMap(), and
findPositionByTimestamp()'s backward walk, which skips to the previous sequence
instead of opening. The walk's outcome is unchanged, since an absent file
yielded position 0 and continued anyway; only the ghost goes away.
An already-open segment is unaffected by any of this: its handle still describes
the file, and on POSIX its unlinked inode is still exactly the committed history
this branch exists to keep serving.
The regression covers the successor lookup against a two-hole registry, a
registered successor that is itself absent, and the absence of resurrection
across all three resolve paths. It deliberately does not drive an iterator end
to end: findPositionByTimestamp() keeps the backward-walk shape, so after a
restart with holes no entry point positions a reader below one - measured
_findPosition(0) = 5 and startFromLastFlushed = 5 on a 1/3/5 layout - and that
shape is only reachable for an iterator already live when the holes appear.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GePs22dNhggr8XKDc1DThG
…ositioning
Two follow-ups approved after review, both regressions this branch introduced or
left behind.
The resync bound at src/transaction-log-reader.ts:133 fell back to the raw
mapping length when the store reports no extent for a purged segment. Invariant
11 forbids exactly that, and prior behaviour was dataEnd = 0 (no scan), so this
branch introduced the failure rather than inheriting it: findResyncPosition
tries every start offset, so a mapped-capacity bound byte-scans the whole
pre-extended map on the JS thread, finds nothing (zeros never satisfy
frameFits), and returns undefined - reporting a recoverable mid-log break as a
torn tail, the harper#2016 amputation the invariant exists to prevent. It now
uses the cached extent or readableExtent(), which walks the frames to the
end-of-entries marker when the store has forgotten the segment.
That does not cover every case, and the gap is worth naming: endOfEntries()
deliberately returns the whole mapping when framing is broken, so that it never
becomes the thing that decides a corrupt frame ends the log. For a purged
segment whose framing is broken - the case that reaches corruptFrame in the
first place - the bound is therefore still the mapped capacity. Every other
purged-segment case is fixed and none is made worse, but choosing a bound when
no authoritative extent exists (the store has forgotten the segment and the
frame walk is defeated by the break) is a design question left open.
findPositionByTimestamp() carried the same defect just fixed in
nextReadableLogBuffer(), and a worse consequence. It stepped with
sequenceFiles.find(--sequenceNumber), so the walk ended at the first missing
sequence: after out-of-band deletion a reader asking for timestamp 0 silently
received only the newest contiguous run while every older survivor sat
registered, on disk, with a valid extent. It now descends by map order, and
tracks the next registered sequence above the entry being examined so the two
"the timestamp belongs further up" exits name a segment that exists rather than
sequenceNumber + 1, which a hole may have removed. Only a segment that actually
opened becomes that tracked sequence: both exits hand it back as a position to
read from, so a registered-but-absent one would send the reader to a file that
is not on disk.
Fixing positioning is what makes the end-to-end case testable at all: before it,
no entry point could put a reader below a hole, which is why the earlier
regression asserted the successor primitive instead. The test now covers a
three-wide gap - 2 and 4 never registered, 3 registered but absent - asserting
query({start: 0}) yields [1, 5] where the old lookup started at 5 and yielded
[5], plus a timestamp past every segment so the other exit is exercised rather
than only the position-zero path. Its read order is load-bearing and says so:
reading a segment's extent opens it, and an open handle keeps reporting the real
size after an unlink (invariant 20), so nothing may touch a segment before it is
meant to be gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GePs22dNhggr8XKDc1DThG
Carry the append-owned readable extent with retained memory maps and invalidate stale flushed-state correlations when destructive purge empties a live store. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Use atomic no-create opens for read probes and verify flushed-state writes through their pathname so purge cannot leave ghost segments or stale retention state. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
The Windows-only "converge on a purge refused by a live mapping" test asserted that a live reader mapping refuses the unlink there. Windows CI showed both outcomes for that test across this branch's heads (refused at 9a8606a, removed at c8e790a) with no change to the mapping's lifetime in between, so the outcome of any single purge run is not something to assert on that platform. Replace it with a cross-platform test of the contract the code actually implements: the reader keeps every entry it mapped, and retention reclaims the segment once nothing maps it — immediately where the unlink lands, on the next run where it did not. It reads the mapping reference after the purge so V8 cannot collect it first, which is what made the old test's premise unverifiable. The three POSIX-only tests that assert a first-run deletion now say why they are skipped on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVAGTbmYzkKxvuH4HQjXKd
The purge's refused-unlink branch (segment stays registered, one warn line per run, next run reclaims it) had no test that reached it: POSIX always unlinks and the Windows outcome is not predictable. An unwritable store directory fails the unlink with EACCES, which is the same branch a sharing violation takes, so the contract — including end-to-end delivery of the `log.warn` line — is now covered deterministically where permissions apply. `lastRemoveError` is a plain std::error_code written under fileMutex; the purge read it unlocked, which can tear its value/category pair against a concurrent retirement of the same file. Read it through a locked accessor. Also trims the `nextReadableLogBuffer` header to the two constraints the code cannot state itself; the rest restated invariant 22 verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVAGTbmYzkKxvuH4HQjXKd
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
2 participants
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()removes a retired segment's pathname, but an iterator that already holds its immutable mapping can safely finish reading that committed history. This PR keeps that behavior while making the mapping collectible and ensuring readers resume at the next real survivor after a purged run.What changed
TransactionLog._currentLogBufferis weak, matching the per-segment cache, so a long-lived iterator no longer pins a deleted mapping after it advances.nextReadableLogBuffer()walks the registered successor set instead of deriving a jump from the newest contiguous run. It crosses multiple holes without skipping an intervening survivor, and stops rather than skipping a registered segment that is only temporarily unmappable.TransactionLogFile::openExisting(), backed byO_RDWRwithoutO_CREATon POSIX andOPEN_EXISTINGon Windows. A segment removed between discovery and the OS open therefore remains absent instead of being recreated as a header-only ghost.readableExtent()requires that exact bound, so purge cannot make a reader infer authority from frame-shaped bytes left by a failed append or from unused mapped capacity.writeFlushedPosition()reopens and verifiestxn.stateby pathname, preventing a pre-purge callback or an unlinked/replaced stream from publishing stale retention state.log.warnline is asserted end to end through the public global-event API, and the next run reclaims it.lastRemoveErroris read through afileMutex-held accessor rather than unlocked.For the human reviewer
log.warn, no marker on the resumed entry — so a follower that lost segments 6–8 believes its history is contiguous. Stopping instead is the wedge this PR fixes, so the choice is between silent skip and a new reader-visible signal Harper would have to handle; it is cheaper to decide now than to retrofit.purgeLogs({ destroy: true })can restart segment numbering while a JavaScript buffer cache from the prior store generation survives. That pre-existing cache-identity problem is outside this PR; Harper does not use destructive log-store purge in production.9a8606a8and failed on all three (both attempts) atc8e790a0, with nothing in between changing how long the mapping lives. Only the convergence is asserted cross-platform now; the first-run deletion is asserted on POSIX only. Worth a word if you know which Windows rule applies here.770b69b8(codex + gemini + harper-domain). Three findings were fixed in that round (refused-unlink coverage, the unlockedlastRemoveErrorread, purge-warning delivery). Two are deliberately left for you: the reader-gap policy in item 1, andwriteFlushedPosition()verifyingtxn.stateby pathname on every flush including the unchanged-position case — that is the invariant-23 hardening as designed, but it costs anopen()/read()/close()per quiescent bound store on RocksDB's flush thread, and moving it behindpositionChangedis a one-line reversal if you would rather not pay it.Verification
pnpm checkpnpm test— 939 passed, 9 skippedpnpm test:native— 203 passednode --expose-gc ./node_modules/vitest/vitest.mjs test/transaction-log.test.ts— 101 passedlingering-txn-shutdownchild aborted (918 passed, 27 skipped before it). Bun is not installed locally.Refs HarperFast/harper#2337
Complexity: moderate
Generated by GPT-5 Codex; maintained by Claude Opus 5
Review-Coverage: authored=claude; ran=codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=25 @ 770b69b
Human-Review-Need: 4 (decisions: reader-gap-policy, txn-state-verify-every-flush, chmod-forced-unlink-test, weakref-current-log-buffer, extent-carried-on-mapping, store-wide-flush-generation, invariant-23-scope) @ 770b69b