fix(txnlog): erase a failed append's partial bytes instead of baking a framing break into the log

[kriszyp]

Fixes #748.

A transaction-log append that fails part-way through — a full disk, an exhausted quota, a short
write on a failing volume — left the bytes it had already written on disk, unaccounted for. The log
fd is opened O_APPEND, so the next successful append landed after those bytes: a partial entry
permanently embedded mid-file with valid entries on both sides. That is the one shape open-time
recovery deliberately refuses to repair (truncating there would discard committed, replicated
transactions), so every reader stops at the break and everything written after it becomes
unreachable — the reader-side consequence seen in HarperFast/harper#2016 and HarperFast/harper#2063.

writeBatchToFile() now reports how many bytes reached the file through a bytesLanded out-param
instead of collapsing a hard error to -1, and writeEntriesV1() erases that range before
throwing. Erasing is safe: writeEntriesV1 holds fileMutex across the whole append, size never
advanced over the bytes, and the commit throws — nothing acknowledged them. The erase reuses
eraseTail() from #723. Both platforms truncate back to size (the zero-fill this description
originally claimed for Windows was replaced by a physical truncate in #723) — with the caveat that
the Windows truncate refuses while another owner holds the memory map, so a failed append there
under a live JS reader retires the file instead of erasing it.

Three failure edges around that, each from a review round:

• The erase itself fails (ENOSPC then EIO). The file is retired rather than warned about: an
  appendBoundaryLost flag makes writeEntriesV1 return without writing, which the store already
  reads as "no progress" and answers by rotating to the next sequence. The orphaned bytes stay the
  trailing partial that recoverTail() can truncate, instead of becoming the mid-file break it
  must leave intact. The double fault degrades to a repairable torn tail. Retirement is set before
  the erase and lifted only on success, so no way out of the erase short of success — a refusal, a
  failed truncate, a throw — can leave the file appendable.
• The landed extent is unreportable. Windows takes it from the file pointer rather than
  lpNumberOfBytesWritten, which WriteFile does not promise to set on failure; if that query also
  fails, the extent is reported as TRANSACTION_LOG_BYTES_LANDED_UNKNOWN and the file is retired
  rather than erased against a guess. A file pointer behind the write origin is nonsense for a
  write and is reported as unknown too, not as "nothing landed" — reporting 0 there would skip
  both the erase and the retirement and leave the file appendable over the orphan. A figure larger than the bytes handed to the OS is treated
  the same way — an over-large erase would cut into committed entries.
• A short header write. open() initialized a new file's header with three unchecked writes; a
  header that only partly landed still set size to the full header length, framing the first
  append from the wrong offset for the life of the file. It is now one checked write, and a short
  one removes the file — a size in (0, HEADER_SIZE) fails open()'s "too small" check on every
  future open, and freeing disk space would not heal it. (removeFile() splits into a
  fileMutex-acquiring wrapper and a per-platform removeFileLocked() body, since open() already
  holds the lock.)
• A throw during cleanup. batch.currentEntryIndex is rolled back before the erase and the
  warning emit, both of which allocate; a bad_alloc there must not leave the batch claiming
  entries that never reached disk.

Stacked on #723

This branches from kris/txnlog-committed-position-recovery (#723), which introduces eraseTail().
Review/merge that one first; the diff here is only this change.

Where to look

• writeEntriesV1()'s error branch — the erase range (committedSize .. committedSize + bytesLanded) has to match where the bytes actually landed on both platforms: POSIX appends at
  physical EOF (which equals size), Windows seeks to size before writing and its file is
  pre-extended to maxFileSize, so size there is the logical end of entries, not the physical
  one. Both truncate back to size; a Windows orphan is overwritten by the next append, but a
  shorter next batch would leave its stale bytes reading as an entry, so the rule is the same.
• The retirement path and its contract with TransactionLogStore::writeBatch — an unchanged size
  is the existing "no progress" signal that triggers rotation, the same mechanism a
  max-size file uses. The flag is deliberately sticky for the object's life: it can only
  over-retire, never under-retire, and a fresh process rebuilds the object with it clear after
  recovery has had its chance at the trailing partial.
• bytesLanded accounting in both writeBatchToFile() implementations.

Verification

Native GoogleTest, POSIX (pnpm test:native). Failure injection uses the existing macro-seam
pattern (ROCKSDB_JS_WRITEV, plus new ROCKSDB_JS_WRITE and ROCKSDB_JS_FTRUNCATE), defined only
in the rocksdb-js-native-tests gyp target — production builds resolve to the bare syscalls.

• FailedAppendLeavesTheLogOnAnEntryBoundary — fails on base with exactly the reported shape:
  file size 43 vs committed 37 (6 orphaned bytes), and after the next append the recovery scan
  returns MidFileCorruption with the entry count truncated at the break. Passes with the fix
  (scan Clean, both entries readable).
• UnerasableOrphanRetiresTheFile — ENOSPC + failing ftruncate: the retired file takes no further
  entries and the scan still classifies it TruncateTail.
• ShortHeaderWriteDiscardsTheFileInsteadOfBrickingIt — fails on base: the 5-byte file survives
  and the reopen throws "File is too small to be a valid transaction log file". Passes with the fix.
• UnerasableExtent/Unknown and UnerasableExtent/OverReported — both fail on base with the
  guard removed: the file is either appended past or erased against an untrustworthy range.
• AppendThatWritesNothingLeavesTheFileUntouched, plus two WriteBatchToFile tests for
  bytesLanded on a hard error and on a nothing-written error.

Full gates on macOS: pnpm test:native 120 passed (3 madvise tests skipped — Linux-only),
pnpm test 728 passed / 1 skipped, pnpm check clean.

Open concerns from the pre-push review

Carried here rather than resolved, so a reviewer does not have to rediscover them:

• Coverage stops at TransactionLogFile. Nothing exercises the retire → rotate → retry contract
  through TransactionLogStore::writeBatch; the native-test target does not link the store (it
  needs RocksDB), and inducing a real ENOSPC from the JS suite would need a filled volume. The
  store side was verified by reading it (transaction_log_store.cpp:831 rotates on unchanged size),
  not by a test.
• Windows has no fault injection. Its bytesLanded accounting and truncating erase are
  exercised only by fix(txnlog): recover the committed watermark from the log tail on load #723's own eraseTail coverage (the arithmetic that turns a file pointer into a
  landed extent is now factored into landedBytesFromFilePointer() and unit-tested on every
  platform, but the WriteFile/SetFilePointerEx calls around it are not); there is no WriteFile seam, and I cannot build Windows
  locally — CI is the only verification for that backend. The decision logic those values feed
  (retire-vs-erase) is shared, and a test-only forcedBytesLandedForTests override drives it from
  the POSIX build.
• Pre-existing, not introduced here: writeBatch's open-retry loop is unbounded, so a
  persistently failing volume spins through sequence numbers. A short header write is a new way to
  reach that loop, but openFile() already reached it under the same ENOSPC condition. Worth its
  own issue rather than widening this one.

Verified false and dismissed along the way: removeFileLocked() closes the file handle before
unlinking (no Windows sharing violation), and bytesWritten is zero-initialized before every
WriteFile, so it could never have contained garbage — the repeated "uninitialized stack memory"
framing of the Windows finding was wrong, even though the underlying under-reporting risk was real
and is now fixed.

One unrelated commit

test(block-cache): await the second read so teardown cannot abort it fixes a pre-existing flake
that was blocking this PR's CI, not anything introduced here. test/block-cache.test.ts asserted
db.get() returns a Promise without awaiting it; when dbRunner closed the database first, the
in-flight read was aborted and the unhandled rejection failed the run with every test passing. It
took out the Deno and Node 24 macOS jobs on this branch while the same commit passed everywhere
else. Identical code is on main, and it is the only unawaited get-promise assertion in the suite.

Reviewed by codex + gemini + harper-domain across four rounds. Generated by Claude Opus 5.

---

Review-feedback pass (2026-08-19). Addressing the open threads on this PR:

• Windows writeBatchToFile reports a negative file-pointer delta as unknown, not 0 — the case
  that previously skipped both the erase and the retirement. The derivation is extracted into
  landedBytesFromFilePointer() with GoogleTest coverage of every branch on all platforms.
• snapshotForBackup() seeds the extent of a registered-but-never-opened log file before reading
  it. size is only meaningful after open(), so on the directory-iteration orders that left an
  older segment lazy, a backup silently omitted it. Done under dataSetsMutex (which purge()
  also holds) and the borrowed handle is closed again, so no fd or mapping is pinned.
• POSIX truncateFile() logs a truncation below the MAP_SHARED overlay boundary — the documented
  SIGBUS case — and lowers the overlay bookkeeping afterwards.
• Comment and AGENTS.md corrections: the Windows erase is a truncate, not an allocating zero-fill,
  and invariant 5's "size is the physical extent" clause is POSIX-only.

_{Review-Coverage: authored=claude; ran=gemini; blocked=codex(quota); declined=cursor-grok,cursor-composer,domain; rounds=4 @ ea72583}

_{Human-Review-Need: 4 @ ea72583}

[gemini-code-assist]

Code Review

This pull request addresses issues with failed appends leaving orphaned bytes on disk, which can cause framing breaks that prevent recovery. It ensures that partial writes are erased on failure, and if the erase itself fails, the file is retired from further appends. Additionally, short header writes during initialization now result in the file being discarded. Feedback was provided regarding the Windows implementation of writeBatchToFile, pointing out that lpNumberOfBytesWritten is unreliable on synchronous WriteFile failures and suggesting the use of SetFilePointer to accurately calculate the number of bytes that landed on disk.

[cb1kenobi]

Medium: this catch cannot tell "the header is corrupt" from "I could not open it right now", and silently drops a healthy segment from the backup

The comment states the premise — a segment whose header will not open has no readable entries — but ensureExtent() -> open() throws a bare DBException for two very different classes of failure, and DBException is final with no code or type to separate them (core/exception.h:23):

Genuinely unreadable (the premise holds): bad token, unsupported version, "File is too small" — transaction_log_file.cpp:137,142,163.

Transient / environmental (the premise does not hold — the segment may be intact and full of committed transactions):

• "Failed to open sequence file for read/write" — ::open failed: EMFILE/ENFILE, EACCES, EIO (transaction_log_file_posix.cpp:92)
• "Failed to get file size" — fstat failed (:99)
• "Failed to read version from file" / "Failed to read file timestamp from file" — readFromFile() returned -1, i.e. a pread EIO on a bad sector (transaction_log_file.cpp:147,157,168)
• "Failed to create parent directory" (transaction_log_file_posix.cpp:81)

fd exhaustion is the one to worry about: RocksDB holds a lot of fds, and this loop opens every lazily-registered segment in one pass. One EMFILE and the backup drops that segment, returns success, and the gap is invisible until a restore needs it — which is the exact failure mode argued against on the earlier thread ("an unopenable segment hard-fails the backup rather than silently omitting itself"). log.warn is out-of-band; nothing in the return value tells the caller the backup is incomplete.

Note doPurge() gets this right for free — an unresolvable extent there means "refuse to purge", so both classes fail safe. It is only the backup where the two need separating.

Suggested fix: make the distinction at the throw site rather than at the catch, since that is the only place that still knows which it was — e.g. a bool malformed on DBException (or a small TransactionLogFormatException) set only by the token/version/too-small throws. Then snapshotForBackup() keeps the continue for a malformed header and rethrows everything else, preserving the previous hard-fail for the case where the segment might be fine.

Secondary: this catch is catch (const std::exception&) only, while the matching one in doPurge() (:697) also has a catch (...). Worth matching them.

—
Generated by Barber AI (Claude Opus 5)

[cb1kenobi]

Medium: the same unguarded open() throw escapes two neighbouring N-API callbacks in this file

The reasoning in this comment is right, and it applies verbatim two functions up. Both of these call store methods that open lazily-registered segments with no try/catch anywhere on the path:

• GetLogFileSize (:174) -> TransactionLogStore::getLogFileSize() (transaction_log_store.cpp:251 and :260) — opens the named file, or every file when no sequence number is given.
• GetMemoryMapOfFile (:221) -> TransactionLogStore::getMemoryMap() (transaction_log_store.cpp:228).

Same open(), same DBException on a bad token/version/too-small/short-read, same escape out of a napi_callback. GetLogFileSize is the more exposed of the two: transaction-log-reader.ts calls it from six sites including the tailing loop (:250, :272, :283, :409, :471), and :471 asks for nextLogId — a segment that may well still be lazy.

Pre-existing rather than introduced here, but 1229e855 is specifically "keep a bad log segment from aborting FindPosition", and one corrupt .txnlog still aborts the process through either sibling. Wrapping both the same way would finish the commit's own goal.

—
Generated by Barber AI (Claude Opus 5)