From 0896d82459aae34e80a5a3ef638d565d4b6a911e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 20:53:17 -0600 Subject: [PATCH 01/13] fix(txnlog): preserve live flush state across purge --- AGENTS.md | 11 + README.md | 2 + .../transaction_log/transaction_log_store.cpp | 191 ++++++++---------- .../transaction_log/transaction_log_store.h | 10 +- src/transaction-log-reader.ts | 24 ++- test/transaction-log.test.ts | 158 ++++++++++++++- 6 files changed, 279 insertions(+), 117 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5d76ca0cc..1b2a295f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -629,6 +629,17 @@ sufficient (env teardown does not honor tsfn acquire counts); see never reproduces natively or on glibc, so the repro test is `skipIf(darwin)` (and, like the repo's other teardown repros, gated to Node). +18. **Only a closed transaction-log store may lose its directory or flush watermark**: ordinary + retention purges remove eligible `.txnlog` segments but keep the live store directory and + `txn.state`; startup age-based retention goes through the same persisted-flush gate. Destructive + teardown belongs to `TransactionLogStoreRegistry::PurgeStores()`'s + `destroy` path, which closes and unregisters the store before removing its directory. Flush + callbacks write `txn.state` through its current pathname rather than a cached file handle, so + replacing or removing the path cannot strand later updates in an unlinked inode. When a restart + finds a retained watermark but no segments, `load()` continues at the next sequence; reusing + an earlier sequence would make the old watermark falsely prove new entries durable. Readers + traverse the ordered segment set rather than assuming retained sequence numbers are contiguous. + ## Debugging native heap corruption AddressSanitizer is the first choice (`ROCKSDB_ASAN=1 node-gyp rebuild` toggles `-fsanitize=address` diff --git a/README.md b/README.md index 834f0f536..a39cd368d 100644 --- a/README.md +++ b/README.md @@ -1620,6 +1620,8 @@ const names = db.listLogs(); ### `db.purgeLogs({ includeEntryCounts: true, ...options }): { path: string; entries: number }[]` Deletes transaction log files older than the `transactionLogRetention` (defaults to 3 days). +Startup and runtime retention remove only segments proven flushed, and retain the live log store +and its flush watermark; use `destroy: true` to close and remove a store completely. - `options: object` - `before?: number` Remove all transaction log files older than the specified timestamp. diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 95b50e1a0..c3dc7776b 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -68,22 +68,16 @@ void TransactionLogStore::doClose() { it = this->sequenceFiles.erase(it); } - // Close the state file if it's open. flushedStateMutex must be held for - // all flushedStateFile access; release it before calling doPurge() since - // doPurge() → getLastFlushedPosition() will acquire flushedStateMutex - // itself (and we must not hold it when doPurge re-acquires it). - { - std::lock_guard flushedLock(this->flushedStateMutex); - if (this->flushedStateFile.is_open()) { - this->flushedStateFile.close(); - } - } - for (auto& logFile : logFilesToClose) { logFile->close(); } - this->doPurge(); + // Drain any flush callback that entered before isClosing was set. A callback + // that arrives later will observe isClosing while holding this mutex and exit. + { + std::lock_guard flushedLock(this->flushedStateMutex); + } + } void TransactionLogStore::close() { @@ -101,23 +95,32 @@ void TransactionLogStore::close() { } bool TransactionLogStore::tryClose() { + auto finishClose = [this]() { + std::lock_guard writeLock(this->writeMutex); + std::lock_guard dataLock(this->dataSetsMutex); + this->doClose(); + return true; + }; + // Fast path: already closing. if (this->isClosing.load(std::memory_order_relaxed)) { - return true; + return finishClose(); } // Phase 1 — quick count check under transactionBindMutex. // transactionBindMutex is a lightweight lock used only for pendingTransactionCount // increments and the isClosing assignment; it is never held during I/O. + bool closeInProgress = false; { std::lock_guard bindLock(this->transactionBindMutex); - if (this->isClosing.load(std::memory_order_relaxed)) return true; - if (this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { + closeInProgress = this->isClosing.load(std::memory_order_relaxed); + if (!closeInProgress && this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { DEBUG_LOG("%p TransactionLogStore::tryClose Skipping (phase 1): pendingTransactionCount=%d\n", this, this->pendingTransactionCount.load(std::memory_order_relaxed)); return false; } } + if (closeInProgress || this->isClosing.load(std::memory_order_relaxed)) return finishClose(); // Phase 2 — drain any in-progress writeBatch and check uncommitted positions. // Acquiring writeMutex here blocks until any concurrent writeBatch() finishes @@ -126,7 +129,10 @@ bool TransactionLogStore::tryClose() { { std::lock_guard writeLock(this->writeMutex); std::lock_guard dataLock(this->dataSetsMutex); - if (this->isClosing.load(std::memory_order_relaxed)) return true; + if (this->isClosing.load(std::memory_order_relaxed)) { + this->doClose(); + return true; + } for (const auto& pos : this->uncommittedTransactionPositions) { if (pos.positionInLogFile == this->nextLogPosition.positionInLogFile && pos.logSequenceNumber == this->nextLogPosition.logSequenceNumber) { @@ -142,17 +148,21 @@ bool TransactionLogStore::tryClose() { // A new UseLog/addLogEntry may have bound a transaction between phases 1 and 3, // so we must re-verify. Once isClosing is set here, all future bind attempts // will fail (they also check isClosing under transactionBindMutex). + bool closeAlreadyClaimed = false; { std::lock_guard bindLock(this->transactionBindMutex); - if (this->isClosing.load(std::memory_order_relaxed)) return true; - if (this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { + closeAlreadyClaimed = this->isClosing.load(std::memory_order_relaxed); + if (!closeAlreadyClaimed && this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { DEBUG_LOG("%p TransactionLogStore::tryClose Skipping (phase 3): pendingTransactionCount=%d\n", this, this->pendingTransactionCount.load(std::memory_order_relaxed)); return false; } - bool expected = false; - this->isClosing.compare_exchange_strong(expected, true); + if (!closeAlreadyClaimed) { + bool expected = false; + this->isClosing.compare_exchange_strong(expected, true); + } } + if (closeAlreadyClaimed) return finishClose(); // Phase 4 — perform the actual close. // Any writeBatch() that starts after phase 3 will see isClosing=true and @@ -303,16 +313,16 @@ std::weak_ptr TransactionLogStore::getLastCommittedPosition() { LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { std::lock_guard lock(this->dataSetsMutex); - uint32_t sequenceNumber = this->currentSequenceNumber.load(std::memory_order_relaxed); - bool isCurrent = true; + uint32_t currentSequence = this->currentSequenceNumber.load(std::memory_order_relaxed); uint32_t positionInLogFile = 0; - auto it = this->sequenceFiles.find(sequenceNumber); - if (it == this->sequenceFiles.end()) { - // it is possible that the current log file doesn't exist yet, so we need to look at the previous one - it = this->sequenceFiles.find(--sequenceNumber); - isCurrent = false; - } - while (it != this->sequenceFiles.end()) { + auto it = this->sequenceFiles.upper_bound(currentSequence); + uint32_t nextHigherSequence = currentSequence; + uint32_t oldestSequence = currentSequence; + while (it != this->sequenceFiles.begin()) { + --it; + uint32_t sequenceNumber = it->first; + oldestSequence = sequenceNumber; + bool isCurrent = sequenceNumber == currentSequence; auto logFile = it->second.get(); // Directory iteration order is unspecified, so registerLogFile() may not // have opened an older file before a higher sequence became current. @@ -329,9 +339,8 @@ LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { if (positionInLogFile > 0) { if (positionInLogFile == 0xFFFFFFFF) { // beyond the end of this log file - if (sequenceNumber < this->currentSequenceNumber.load(std::memory_order_relaxed)) { - // revert to next one (because it exists) - break; + if (sequenceNumber < currentSequence) { + return { TRANSACTION_LOG_FILE_HEADER_SIZE, nextHigherSequence }; } else { // otherwise position at the end of the log file (JS code can filter from here) positionInLogFile = logFile->size; } @@ -339,11 +348,9 @@ LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { // found a valid position in the log file return { positionInLogFile, sequenceNumber }; } - isCurrent = false; - it = this->sequenceFiles.find(--sequenceNumber); + nextHigherSequence = sequenceNumber; }; - // we iterated too far, return to the beginning position in the current log file - return { TRANSACTION_LOG_FILE_HEADER_SIZE, sequenceNumber + 1 }; + return { TRANSACTION_LOG_FILE_HEADER_SIZE, oldestSequence }; } LogPosition TransactionLogStore::getLastFlushedPosition() { @@ -398,8 +405,8 @@ std::vector TransactionLogStore::snapshotForBackup() TransactionLogBackupEntry stateEntry; bool hasStateEntry = false; { - // databaseFlushed() (RocksDB's OnFlushComplete callback) rewrites txn.state - // in place under flushedStateMutex, and a flush can fire mid-backup — an + // databaseFlushed() (RocksDB's OnFlushComplete callback) updates txn.state + // under flushedStateMutex, and a flush can fire mid-backup — an // unsynchronized read could tear, decoding a position that is neither the // old nor the new value and may point past the log extents captured below // (same discipline as getLastFlushedPosition()). The lock is scoped to this @@ -649,6 +656,7 @@ void TransactionLogStore::doPurge(std::function sequenceNumbersToRemove; + auto lastFlushedPosition = this->getLastFlushedPosition(); for (const auto& entry : this->sequenceFiles) { auto& sequenceNumber = entry.first; @@ -690,7 +698,6 @@ void TransactionLogStore::doPurge(std::functiongetLastFlushedPosition(); if (sequenceNumber > lastFlushedPosition.logSequenceNumber) { continue; } @@ -756,25 +763,6 @@ void TransactionLogStore::doPurge(std::functionsequenceFiles.erase(sequenceNumber); } - - // if all log files have been removed, clean up the empty directory - // only try to remove if we actually removed at least one file from this store - if (this->sequenceFiles.empty() && !sequenceNumbersToRemove.empty()) { - try { - if (std::filesystem::exists(this->path)) { - DEBUG_LOG("%p TransactionLogStore::purge Removing log store directory: %s\n", this, this->path.string().c_str()); - std::filesystem::remove_all(this->path); - DEBUG_LOG("%p TransactionLogStore::purge Removed log store directory: %s\n", this, this->path.string().c_str()); - } - } catch (const std::filesystem::filesystem_error& e) { - DEBUG_LOG("%p TransactionLogStore::purge Failed to remove log store directory %s: %s\n", this, this->path.string().c_str(), e.what()); - } catch (...) { - auto eptr = std::current_exception(); - std::string errorMsg = getExceptionMessage(eptr); - DEBUG_LOG("%p TransactionLogStore::purge Unknown error removing log store directory %s: %s\n", - this, this->path.string().c_str(), errorMsg.c_str()); - } - } } void TransactionLogStore::registerLogFile(const std::filesystem::path& path, const uint32_t sequenceNumber) { @@ -1106,25 +1094,28 @@ void TransactionLogStore::databaseFlushed(rocksdb::SequenceNumber rocksSequenceN // flushedStateMutex (not dataSetsMutex) so that getLastFlushedPosition() // can safely read txn.state from doPurge() without risk of deadlock. std::lock_guard flushedLock(this->flushedStateMutex); + if (this->isClosing.load(std::memory_order_relaxed)) { + return; + } - // Only write if the position has changed if (latestSequencePosition.fullPosition == lastWrittenFlushedPosition.fullPosition) { return; } - // open the state file if it isn't open yet - if (!this->flushedStateFile.is_open()) { - auto flushedStateFilePath = this->path / "txn.state"; - this->flushedStateFile.open(flushedStateFilePath, std::ios::binary | std::ios::out); + auto flushedStateFilePath = this->path / "txn.state"; + std::fstream flushedStateFile(flushedStateFilePath, std::ios::binary | std::ios::in | std::ios::out); + if (!flushedStateFile.is_open()) { + // Recreate a missing state file, but never its parent store directory. + flushedStateFile.open(flushedStateFilePath, std::ios::binary | std::ios::out); } - - // write the position to the file - if (this->flushedStateFile.is_open()) { - this->flushedStateFile.seekp(0); - this->flushedStateFile.write(reinterpret_cast(&latestSequencePosition), sizeof(latestSequencePosition)); - this->flushedStateFile.flush(); - lastWrittenFlushedPosition = latestSequencePosition; - this->databaseFlushes.fetch_add(1, std::memory_order_relaxed); + if (flushedStateFile.is_open()) { + flushedStateFile.seekp(0); + flushedStateFile.write(reinterpret_cast(&latestSequencePosition), sizeof(latestSequencePosition)); + flushedStateFile.flush(); + if (flushedStateFile.good()) { + lastWrittenFlushedPosition = latestSequencePosition; + this->databaseFlushes.fetch_add(1, std::memory_order_relaxed); + } } } @@ -1156,39 +1147,6 @@ std::shared_ptr TransactionLogStore::load( sequenceNumber = std::stoul(sequenceNumberStr); - // check if the file is too old - if (retentionMs.count() > 0) { - auto mtime = std::filesystem::last_write_time(filePath); - auto mtime_sys = convertFileTimeToSystemTime(mtime); - auto now = std::chrono::system_clock::now(); - auto fileAgeMs = std::chrono::duration_cast(now - mtime_sys); - auto delta = fileAgeMs - retentionMs; - - if (delta.count() > 0) { - // file is too old, remove it - DEBUG_LOG("%p TransactionLogStore::load File \"%s\" age=%lldms, expired %lldms ago, purging\n", - store.get(), filePath.filename().string().c_str(), fileAgeMs.count(), delta.count()); - try { - DEBUG_LOG("%p TransactionLogStore::load Removing expired file: %s\n", store.get(), filePath.string().c_str()); - if (std::filesystem::remove(filePath)) { - std::error_code markerError; - auto markerPath = transactionLogAppendBoundaryMarkerPath(filePath); - std::filesystem::remove(markerPath, markerError); - std::filesystem::remove(markerPath.parent_path(), markerError); - std::filesystem::remove( - markerPath.parent_path().parent_path(), markerError); - } - } catch (const std::filesystem::filesystem_error& e) { - DEBUG_LOG("%p TransactionLogStore::load Failed to remove expired file %s: %s\n", - store.get(), filePath.string().c_str(), e.what()); - } - continue; - } else { - DEBUG_LOG("%p TransactionLogStore::load File \"%s\" age=%lldms, not expired, %lldms left\n", - store.get(), filePath.filename().string().c_str(), fileAgeMs.count(), delta.count() * -1); - } - } - store->registerLogFile(filePath, sequenceNumber); } } catch (const TransactionLogAppendBoundaryException&) { @@ -1214,6 +1172,18 @@ std::shared_ptr TransactionLogStore::load( } LogPosition flushedPosition = store->getLastFlushedPosition(); + uint32_t discoveredCurrentSequence = store->currentSequenceNumber.load(std::memory_order_relaxed); + bool hasCurrentSegment = store->sequenceFiles.find(discoveredCurrentSequence) != store->sequenceFiles.end(); + bool resumedPastWatermark = false; + if (flushedPosition.logSequenceNumber > discoveredCurrentSequence || + (!hasCurrentSegment && flushedPosition.logSequenceNumber == discoveredCurrentSequence && + flushedPosition.logSequenceNumber > 0)) { + uint32_t nextWritableSequence = flushedPosition.logSequenceNumber + 1; + store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); + store->nextSequenceNumber = nextWritableSequence + 1; + store->nextLogPosition = { 0, nextWritableSequence }; + resumedPastWatermark = true; + } // Only the active file can carry a torn append; recover it after discovery and // refresh the write position if recovery shortened it. @@ -1240,7 +1210,9 @@ std::shared_ptr TransactionLogStore::load( // Legacy batches can span any number of rotations, so walk back through their // unflagged files until a boundary is found. Once the flushed file has been // scanned, older files cannot improve the floor and need not be read. - LogPosition recoveredPosition = { 0, 0 }; + LogPosition recoveredPosition = (store->sequenceFiles.empty() || resumedPastWatermark) + ? store->nextLogPosition + : LogPosition { 0, 0 }; for (auto it = store->sequenceFiles.rbegin(); it != store->sequenceFiles.rend(); ++it) { if (it->first > storeCurrentSeq) { continue; @@ -1284,6 +1256,15 @@ std::shared_ptr TransactionLogStore::load( } } *store->lastCommittedPosition = recoveredPosition < flushedPosition ? flushedPosition : recoveredPosition; + if (retentionMs.count() > 0) { + try { + store->purge(nullptr, false, 0, false); + } catch (const std::exception& e) { + DEBUG_LOG("%p TransactionLogStore::load Failed to purge expired files: %s\n", store.get(), e.what()); + } catch (...) { + DEBUG_LOG("%p TransactionLogStore::load Failed to purge expired files\n", store.get()); + } + } return store; } diff --git a/src/binding/transaction_log/transaction_log_store.h b/src/binding/transaction_log/transaction_log_store.h index 0133537fe..112677dbf 100644 --- a/src/binding/transaction_log/transaction_log_store.h +++ b/src/binding/transaction_log/transaction_log_store.h @@ -3,7 +3,6 @@ #include #include -#include #include #include #include @@ -322,8 +321,8 @@ struct TransactionLogStore final { unsigned int nextSequencePositionsCount = 0; /** - * Protects flushedStateFile, lastWrittenFlushedPosition, and all I/O on - * "txn.state". This is a separate, lightweight lock so that + * Protects lastWrittenFlushedPosition and all I/O on "txn.state". + * This is a separate, lightweight lock so that * getLastFlushedPosition() — which is called from doPurge() while * dataSetsMutex is already held — never needs to acquire dataSetsMutex, * eliminating that deadlock path. @@ -333,11 +332,6 @@ struct TransactionLogStore final { */ std::mutex flushedStateMutex; - /** - * This file stream is used to track how much of the transaction log has been flushed to the database. - */ - std::ofstream flushedStateFile; - /** * The last flushed position that was written to the state file. */ diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index ae1d52354..cfa491d3c 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -271,8 +271,11 @@ Object.defineProperty(TransactionLog.prototype, 'query', { logBuffer!.size ?? (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); if (position >= size) { - // we can't read any further in this block, go to the next block - const nextLogBuffer = getLogMemoryMap(transactionLog, logBuffer!.logId + 1)!; + const nextLogBuffer = getNextLogMemoryMap( + transactionLog, + logBuffer!.logId, + latestLogId + ); if (nextLogBuffer) { dataView = nextLogBuffer.dataView; logBuffer = nextLogBuffer; @@ -395,7 +398,11 @@ Object.defineProperty(TransactionLog.prototype, 'query', { ); size = latestSize; if (latestLogId > logBuffer!.logId) { - const nextLogBuffer = getLogMemoryMap(transactionLog, logBuffer!.logId + 1); + const nextLogBuffer = getNextLogMemoryMap( + transactionLog, + logBuffer!.logId, + latestLogId + ); if (!nextLogBuffer) { // the next log file can't be mapped (purged, mid-rotation, // 0-byte at mmap time, FS race); stop cleanly rather than @@ -454,6 +461,17 @@ function getLogMemoryMap(transactionLog: TransactionLog, logId: number): LogBuff return logBuffer; } +function getNextLogMemoryMap( + transactionLog: TransactionLog, + logId: number, + latestLogId: number +): LogBuffer | undefined { + while (++logId <= latestLogId) { + const logBuffer = getLogMemoryMap(transactionLog, logId); + if (logBuffer) return logBuffer; + } +} + function loadLastPosition( transactionLog: TransactionLog, readUncommitted: boolean diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index e8e6b324f..efd5c0ddd 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -11,7 +11,7 @@ import { writeLazyTransactionLogSegments } from './lib/transaction-log-fixtures. import { dbRunner, generateDBPath, terminateWorker } from './lib/util.ts'; import { createWorkerBootstrapScript } from './lib/worker-bootstrap.ts'; import assert from 'node:assert'; -import { existsSync, readFileSync, statSync } from 'node:fs'; +import { existsSync, readFileSync, statSync, unlinkSync } from 'node:fs'; import { mkdir, readdir, stat, utimes, writeFile } from 'node:fs/promises'; import { release } from 'node:os'; import { join } from 'node:path'; @@ -2094,6 +2094,139 @@ describe('Transaction Log', () => { return Buffer.concat(parts); }; + it('preserves the live flush watermark across repeated complete purges', () => + dbRunner(async ({ db, dbPath }) => { + const log = db.useLog('foo'); + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + const stateFile = join(logDirectory, 'txn.state'); + + for (let cycle = 1; cycle <= 2; cycle++) { + await db.transaction(async (txn) => { + const value = Buffer.from(`value-${cycle}`); + log.addEntry(value, txn.id); + db.putSync(`key-${cycle}`, value, { transaction: txn }); + }); + db.flushSync(); + + const state = readFileSync(stateFile); + const flushed = new Uint32Array(state.buffer, state.byteOffset, 2); + expect(flushed[1]).toBe(cycle); + expect(flushed[0]).toBeGreaterThan(TRANSACTION_LOG_FILE_HEADER_SIZE); + + const logFile = join(logDirectory, `${cycle}.txnlog`); + expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([logFile]); + expect(existsSync(stateFile)).toBe(true); + if (cycle === 1) unlinkSync(stateFile); + } + })); + + it('does not recreate a missing state file for an unchanged flush position', () => + dbRunner(async ({ db, dbPath }) => { + const log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('value'), txn.id); + db.putSync('key', 'value', { transaction: txn }); + }); + db.flushSync(); + + const stateFile = join(dbPath, 'transaction_logs', 'foo', 'txn.state'); + unlinkSync(stateFile); + db.flushSync(); + expect(existsSync(stateFile)).toBe(false); + })); + + it('continues after a retained watermark when reopening an empty store', () => + dbRunner(async ({ db, dbPath }) => { + let database = db; + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + try { + let log = database.useLog('foo'); + await database.transaction(async (txn) => { + log.addEntry(Buffer.from('first'), txn.id); + database.putSync('first', 'value', { transaction: txn }); + }); + database.flushSync(); + expect(database.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([ + join(logDirectory, '1.txnlog'), + ]); + database.close(); + + database = RocksDatabase.open(dbPath); + log = database.useLog('foo'); + const lastCommittedBuffer = log._getLastCommittedPosition(); + const lastCommitted = new Uint32Array( + lastCommittedBuffer.buffer, + lastCommittedBuffer.byteOffset, + 2 + ); + expect(Array.from(lastCommitted)).toEqual([0, 2]); + await database.transaction(async (txn) => { + log.addEntry(Buffer.from('second'), txn.id); + database.putSync('second', 'value', { transaction: txn }); + }); + + const logFiles = (await readdir(logDirectory)).filter((name) => name.endsWith('.txnlog')); + expect(logFiles).toEqual(['2.txnlog']); + expect(database.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([]); + expect(existsSync(join(logDirectory, '2.txnlog'))).toBe(true); + } finally { + database.close(); + } + })); + + it('continues past the watermark when only lower sequence files survive', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + db.open(); + let log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('first'), txn.id); + }); + const firstTimestamp = Array.from(log.query({ start: 0 }))[0].timestamp; + db.close(); + + const state = Buffer.alloc(8); + state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); + state.writeUInt32LE(2, 4); + await writeFile(join(logDirectory, 'txn.state'), state); + + db.open(); + log = db.useLog('foo'); + const lastCommittedBuffer = log._getLastCommittedPosition(); + const lastCommitted = new Uint32Array( + lastCommittedBuffer.buffer, + lastCommittedBuffer.byteOffset, + 2 + ); + expect(Array.from(lastCommitted)).toEqual([0, 3]); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('third'), txn.id); + }); + expect(existsSync(join(logDirectory, '3.txnlog'))).toBe(true); + expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ + 'first', + 'third', + ]); + expect( + Array.from(log.query({ start: firstTimestamp + 0.5 }), (entry) => entry.data.toString()) + ).toEqual(['third']); + })); + + it('leaves a destroyed store absent after a concurrent flush request', () => + dbRunner(async ({ db, dbPath }) => { + const log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('value'), txn.id); + db.putSync('key', 'value', { transaction: txn }); + }); + + const flush = db.flush(); + db.purgeLogs({ destroy: true, name: 'foo' }); + await flush; + await delay(10); + expect(existsSync(join(dbPath, 'transaction_logs', 'foo'))).toBe(false); + })); + it('should return entry counts when includeEntryCounts is true', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); @@ -2205,12 +2338,35 @@ describe('Transaction Log', () => { const oneWeekAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000); await utimes(logFile, oneWeekAgo, oneWeekAgo); + const state = Buffer.alloc(8); + state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); + state.writeUInt32LE(1, 4); + await writeFile(join(logDirectory, 'txn.state'), state); db.open(); expect(db.listLogs()).toEqual(['foo']); expect(existsSync(logFile)).toBe(false); })); + it('should retain an aged, unflushed log file on load', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + const logFile = join(logDirectory, '1.txnlog'); + await mkdir(logDirectory, { recursive: true }); + + const header = Buffer.alloc(TRANSACTION_LOG_FILE_HEADER_SIZE); + header.writeUInt32BE(TRANSACTION_LOG_TOKEN, 0); + header.writeUInt8(1, 4); + header.writeDoubleBE(0, 5); + await writeFile(logFile, header); + + const oneWeekAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000); + await utimes(logFile, oneWeekAgo, oneWeekAgo); + + db.open(); + expect(existsSync(logFile)).toBe(true); + })); + // doPurge()'s flushed-position guard, which reads TransactionLogFile::size. // A registered-but-never-opened segment reads 0 there and would be deleted // with an unflushed tail (HarperFast/rocksdb-js#751); these cover the guard From 3d2a2ee37ff193049b63376bf54c7f96ada64d9b Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 23:08:30 -0600 Subject: [PATCH 02/13] Continue transaction-log reads through empty segments --- src/transaction-log-reader.ts | 68 +++++++++++++++++------------------ test/transaction-log.test.ts | 30 ++++++++++++++++ 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index cfa491d3c..2ef8e49ff 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -270,25 +270,24 @@ Object.defineProperty(TransactionLog.prototype, 'query', { size = logBuffer!.size ?? (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); - if (position >= size) { + while (position >= size && latestLogId > logBuffer!.logId) { const nextLogBuffer = getNextLogMemoryMap( transactionLog, logBuffer!.logId, latestLogId ); - if (nextLogBuffer) { - dataView = nextLogBuffer.dataView; - logBuffer = nextLogBuffer; - if (latestLogId > logBuffer!.logId) { - // it is non-current log file, we can safely use or cache the size - size = - logBuffer!.size ?? - (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); - } else { - size = latestSize; // use the latest position from loadLastPosition - } - position = TRANSACTION_LOG_FILE_HEADER_SIZE; + if (!nextLogBuffer) break; + dataView = nextLogBuffer.dataView; + logBuffer = nextLogBuffer; + if (latestLogId > logBuffer!.logId) { + // it is non-current log file, we can safely use or cache the size + size = + logBuffer!.size ?? + (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); + } else { + size = latestSize; // use the latest position from loadLastPosition } + position = TRANSACTION_LOG_FILE_HEADER_SIZE; } } } @@ -390,36 +389,35 @@ Object.defineProperty(TransactionLog.prototype, 'query', { }, }; } - if (position >= size) { + while (position >= size) { // move to the next log file const { logId: latestLogId, size: latestSize } = loadLastPosition( transactionLog, !!readUncommitted ); size = latestSize; - if (latestLogId > logBuffer!.logId) { - const nextLogBuffer = getNextLogMemoryMap( - transactionLog, - logBuffer!.logId, - latestLogId - ); - if (!nextLogBuffer) { - // the next log file can't be mapped (purged, mid-rotation, - // 0-byte at mmap time, FS race); stop cleanly rather than - // dereferencing an undefined buffer - return { done: true, value: undefined }; - } - logBuffer = nextLogBuffer; - dataView = logBuffer.dataView; - size = logBuffer.size; - if (size == undefined) { - size = transactionLog.getLogFileSize(logBuffer.logId); - if (!readUncommitted) { - logBuffer.size = size; - } + if (latestLogId <= logBuffer!.logId) break; + const nextLogBuffer = getNextLogMemoryMap( + transactionLog, + logBuffer!.logId, + latestLogId + ); + if (!nextLogBuffer) { + // the next log file can't be mapped (purged, mid-rotation, + // 0-byte at mmap time, FS race); stop cleanly rather than + // dereferencing an undefined buffer + return { done: true, value: undefined }; + } + logBuffer = nextLogBuffer; + dataView = logBuffer.dataView; + size = logBuffer.size; + if (size == undefined) { + size = transactionLog.getLogFileSize(logBuffer.logId); + if (!readUncommitted) { + logBuffer.size = size; } - position = TRANSACTION_LOG_FILE_HEADER_SIZE; } + position = TRANSACTION_LOG_FILE_HEADER_SIZE; } } return { done: true, value: undefined }; diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index efd5c0ddd..b5d5c0c75 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -713,6 +713,36 @@ describe('Transaction Log', () => { } })); + it('continues through an empty segment to later committed entries', () => + dbRunner({ dbOptions: [{ transactionLogMaxSize: 1000 }] }, async ({ db, dbPath }) => { + let database = db; + try { + let log = database.useLog('foo'); + for (let i = 0; i < 20; i++) { + await database.transaction(async (txn) => { + log.addEntry(Buffer.from(String(i).padStart(100, '0')), txn.id); + }); + } + database.close(); + + const middlePath = join(dbPath, 'transaction_logs', 'foo', '2.txnlog'); + await writeFile( + middlePath, + readFileSync(middlePath).subarray(0, TRANSACTION_LOG_FILE_HEADER_SIZE) + ); + + database = RocksDatabase.open(dbPath, { transactionLogMaxSize: 1000 }); + log = database.useLog('foo'); + const values = Array.from(log.query({ start: 0 }), (entry) => entry.data.toString()); + expect(values).toEqual([ + ...Array.from({ length: 8 }, (_, i) => String(i).padStart(100, '0')), + ...Array.from({ length: 4 }, (_, i) => String(i + 16).padStart(100, '0')), + ]); + } finally { + database.close(); + } + })); + it('should allow unlimited transaction log size', () => dbRunner({ dbOptions: [{ transactionLogMaxSize: 0 }] }, async ({ db, dbPath }) => { const log = db.useLog('foo'); From b1681b2f152c3d57b8635e5f3b244542ce944738 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 23:45:51 -0600 Subject: [PATCH 03/13] Harden transaction-log purge lifecycle --- AGENTS.md | 3 +- .../transaction_log/transaction_log.cpp | 20 +++++ src/binding/transaction_log/transaction_log.h | 4 +- .../transaction_log_handle.cpp | 15 ++++ .../transaction_log/transaction_log_handle.h | 3 + .../transaction_log/transaction_log_store.cpp | 23 ++++++ .../transaction_log/transaction_log_store.h | 8 ++ .../transaction_log_store_registry.cpp | 27 +++---- src/load-binding.ts | 5 +- src/transaction-log-reader.ts | 43 ++++++++-- test/transaction-log.test.ts | 81 +++++++++++++++++++ 11 files changed, 208 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1b2a295f9..64a0c4a96 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -633,7 +633,8 @@ sufficient (env teardown does not honor tsfn acquire counts); see retention purges remove eligible `.txnlog` segments but keep the live store directory and `txn.state`; startup age-based retention goes through the same persisted-flush gate. Destructive teardown belongs to `TransactionLogStoreRegistry::PurgeStores()`'s - `destroy` path, which closes and unregisters the store before removing its directory. Flush + `destroy` path, which closes the store and holds the per-database store-registry lock across + unregistering and directory removal so a same-name replacement cannot be deleted. Flush callbacks write `txn.state` through its current pathname rather than a cached file handle, so replacing or removing the path cannot strand later updates in an unlinked inode. When a restart finds a retained watermark but no segments, `load()` continues at the next sequence; reusing diff --git a/src/binding/transaction_log/transaction_log.cpp b/src/binding/transaction_log/transaction_log.cpp index 99fbd76a7..97c883e31 100644 --- a/src/binding/transaction_log/transaction_log.cpp +++ b/src/binding/transaction_log/transaction_log.cpp @@ -183,6 +183,24 @@ napi_value TransactionLog::GetLogFileSize(napi_env env, napi_callback_info info) return result; } +napi_value TransactionLog::GetNextLogSequenceNumber(napi_env env, napi_callback_info info) { + NAPI_METHOD_ARGV(1); + UNWRAP_TRANSACTION_LOG_HANDLE("GetNextLogSequenceNumber"); + uint32_t sequenceNumber; + NAPI_STATUS_THROWS(::napi_get_value_uint32(env, argv[0], &sequenceNumber)); + napi_value result; + NAPI_STATUS_THROWS(::napi_create_uint32(env, (*txnLogHandle)->getNextLogSequenceNumber(sequenceNumber), &result)); + return result; +} + +napi_value TransactionLog::GetPurgeGeneration(napi_env env, napi_callback_info info) { + NAPI_METHOD(); + UNWRAP_TRANSACTION_LOG_HANDLE("GetPurgeGeneration"); + napi_value result; + NAPI_STATUS_THROWS(::napi_create_double(env, static_cast((*txnLogHandle)->getPurgeGeneration()), &result)); + return result; +} + struct PositionHandle { std::shared_ptr position; }; @@ -440,6 +458,8 @@ void TransactionLog::Init(napi_env env, napi_value exports) { { "_findPosition", nullptr, FindPosition, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getLastCommittedPosition", nullptr, GetLastCommittedPosition, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getMemoryMapOfFile", nullptr, GetMemoryMapOfFile, nullptr, nullptr, nullptr, napi_default, nullptr }, + { "_getNextLogSequenceNumber", nullptr, GetNextLogSequenceNumber, nullptr, nullptr, nullptr, napi_default, nullptr }, + { "_getPurgeGeneration", nullptr, GetPurgeGeneration, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getLastFlushed", nullptr, GetLastFlushed, nullptr, nullptr, nullptr, napi_default, nullptr } }; diff --git a/src/binding/transaction_log/transaction_log.h b/src/binding/transaction_log/transaction_log.h index 6c8f815f2..2aa8b5ff6 100644 --- a/src/binding/transaction_log/transaction_log.h +++ b/src/binding/transaction_log/transaction_log.h @@ -15,6 +15,8 @@ struct TransactionLog final { static napi_value GetLastCommittedPosition(napi_env env, napi_callback_info info); static napi_value GetLastFlushed(napi_env env, napi_callback_info info); static napi_value GetLogFileSize(napi_env env, napi_callback_info info); + static napi_value GetNextLogSequenceNumber(napi_env env, napi_callback_info info); + static napi_value GetPurgeGeneration(napi_env env, napi_callback_info info); static napi_value GetMemoryMapOfFile(napi_env env, napi_callback_info info); static napi_value GetName(napi_env env, napi_callback_info info); static napi_value GetPath(napi_env env, napi_callback_info info); @@ -25,4 +27,4 @@ struct TransactionLog final { } // namespace rocksdb_js -#endif \ No newline at end of file +#endif diff --git a/src/binding/transaction_log/transaction_log_handle.cpp b/src/binding/transaction_log/transaction_log_handle.cpp index d7b5205da..7ae83787f 100644 --- a/src/binding/transaction_log/transaction_log_handle.cpp +++ b/src/binding/transaction_log/transaction_log_handle.cpp @@ -15,6 +15,9 @@ TransactionLogHandle::TransactionLogHandle( ): dbHandle(dbHandle), logName(logName), readOnly(readOnly), transactionId(0) { DEBUG_LOG("%p TransactionLogHandle::TransactionLogHandle Creating TransactionLogHandle \"%s\"\n", this, logName.c_str()); this->store = dbHandle->descriptor->resolveTransactionLogStore(logName); + if (auto store = this->store.lock()) { + this->lastKnownPurgeGeneration = store->getPurgeGeneration(); + } } TransactionLogHandle::~TransactionLogHandle() { @@ -75,6 +78,18 @@ uint64_t TransactionLogHandle::getLogFileSize(uint32_t sequenceNumber) { return 0; } +uint32_t TransactionLogHandle::getNextLogSequenceNumber(uint32_t sequenceNumber) { + auto store = this->store.lock(); + if (store) return store->getNextLogSequenceNumber(sequenceNumber); + return 0; +} + +uint64_t TransactionLogHandle::getPurgeGeneration() { + auto store = this->store.lock(); + if (store) this->lastKnownPurgeGeneration = store->getPurgeGeneration(); + return this->lastKnownPurgeGeneration; +} + std::shared_ptr TransactionLogHandle::getMemoryMap(uint32_t sequenceNumber) { auto store = this->store.lock(); if (store) return store->getMemoryMap(sequenceNumber); diff --git a/src/binding/transaction_log/transaction_log_handle.h b/src/binding/transaction_log/transaction_log_handle.h index 993dff896..73476a634 100644 --- a/src/binding/transaction_log/transaction_log_handle.h +++ b/src/binding/transaction_log/transaction_log_handle.h @@ -36,6 +36,7 @@ struct TransactionLogHandle final : Closable { * The transaction id. */ uint32_t transactionId; + uint64_t lastKnownPurgeGeneration = 0; /** * Creates a new transaction log handle. @@ -64,6 +65,8 @@ struct TransactionLogHandle final : Closable { LogPosition findPosition(double timestamp); LogPosition getLastFlushed(); uint64_t getLogFileSize(uint32_t sequenceNumber); + uint32_t getNextLogSequenceNumber(uint32_t sequenceNumber); + uint64_t getPurgeGeneration(); std::weak_ptr getLastCommittedPosition(); /** diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index c3dc7776b..49507528e 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include "transaction_log_store.h" @@ -11,6 +12,8 @@ namespace rocksdb_js { +static std::atomic nextTransactionLogStoreGeneration = 1; + // Helper function to extract exception message from exception_ptr static std::string getExceptionMessage(std::exception_ptr eptr) { if (!eptr) { @@ -43,6 +46,7 @@ TransactionLogStore::TransactionLogStore( maxAgeThreshold(maxAgeThreshold) { DEBUG_LOG("%p TransactionLogStore::TransactionLogStore Opening transaction log store \"%s\"\n", this, this->name.c_str()); + this->purgeGeneration.store(nextTransactionLogStoreGeneration.fetch_add(1, std::memory_order_relaxed), std::memory_order_relaxed); lastCommittedPosition = std::make_shared(); uncommittedTransactionPositions.reserve(16); for (int i = 0; i < RECENTLY_COMMITTED_POSITIONS_SIZE; i++) { // initialize recent commits to not match until values are entered @@ -276,6 +280,16 @@ uint64_t TransactionLogStore::getLogFileSize(uint32_t logSequenceNumber) { return size; } +uint32_t TransactionLogStore::getNextLogSequenceNumber(uint32_t logSequenceNumber) { + std::lock_guard lock(this->dataSetsMutex); + auto it = this->sequenceFiles.upper_bound(logSequenceNumber); + return it == this->sequenceFiles.end() ? 0 : it->first; +} + +uint64_t TransactionLogStore::getPurgeGeneration() const { + return this->purgeGeneration.load(std::memory_order_relaxed); +} + std::weak_ptr TransactionLogStore::getLastCommittedPosition() { // Initialize lastCommittedPosition if it's still at {0, 0} and invalid if (this->lastCommittedPosition->fullPosition == 0) { @@ -763,6 +777,9 @@ void TransactionLogStore::doPurge(std::functionsequenceFiles.erase(sequenceNumber); } + if (!sequenceNumbersToRemove.empty()) { + this->purgeGeneration.store(nextTransactionLogStoreGeneration.fetch_add(1, std::memory_order_relaxed), std::memory_order_relaxed); + } } void TransactionLogStore::registerLogFile(const std::filesystem::path& path, const uint32_t sequenceNumber) { @@ -1178,7 +1195,13 @@ std::shared_ptr TransactionLogStore::load( if (flushedPosition.logSequenceNumber > discoveredCurrentSequence || (!hasCurrentSegment && flushedPosition.logSequenceNumber == discoveredCurrentSequence && flushedPosition.logSequenceNumber > 0)) { + if (flushedPosition.logSequenceNumber == std::numeric_limits::max()) { + throw std::runtime_error("Transaction log flush watermark sequence is out of range"); + } uint32_t nextWritableSequence = flushedPosition.logSequenceNumber + 1; + if (nextWritableSequence == std::numeric_limits::max()) { + throw std::runtime_error("Transaction log sequence space is exhausted"); + } store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); store->nextSequenceNumber = nextWritableSequence + 1; store->nextLogPosition = { 0, nextWritableSequence }; diff --git a/src/binding/transaction_log/transaction_log_store.h b/src/binding/transaction_log/transaction_log_store.h index 112677dbf..934a1a720 100644 --- a/src/binding/transaction_log/transaction_log_store.h +++ b/src/binding/transaction_log/transaction_log_store.h @@ -362,6 +362,7 @@ struct TransactionLogStore final { std::atomic filesPurged = 0; std::atomic bytesPurged = 0; std::atomic purgeRuns = 0; + std::atomic purgeGeneration = 0; std::atomic databaseFlushes = 0; std::atomic writeFailures = 0; std::atomic lastPurgeMs = 0; @@ -442,6 +443,13 @@ struct TransactionLogStore final { **/ uint64_t getLogFileSize(uint32_t logSequenceNumber); + /** + * Get the first registered log sequence strictly after the supplied sequence, + * or zero when no later segment exists. + */ + uint32_t getNextLogSequenceNumber(uint32_t logSequenceNumber); + uint64_t getPurgeGeneration() const; + /** * Get the shared represention object representing the last committed position. **/ diff --git a/src/binding/transaction_log/transaction_log_store_registry.cpp b/src/binding/transaction_log/transaction_log_store_registry.cpp index 01518ea30..dff5c02a6 100644 --- a/src/binding/transaction_log/transaction_log_store_registry.cpp +++ b/src/binding/transaction_log/transaction_log_store_registry.cpp @@ -360,8 +360,8 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str } } - // Phase 3: Remove closed stores from the registry while holding the lock - std::vector> storesActuallyRemoved; + // Phase 3: Remove closed stores and their directories while holding the lock so + // ResolveStore cannot publish a replacement at the same path before deletion finishes. if (destroy) { std::lock_guard storeLock(entry->storesMutex); for (auto& store : storesToPurge) { @@ -371,24 +371,19 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str auto storeIt = entry->stores.find(store->name); if (storeIt != entry->stores.end() && storeIt->second.get() == store.get()) { entry->stores.erase(storeIt); - storesActuallyRemoved.push_back(store); + try { + std::filesystem::remove_all(store->path); + } catch (const std::filesystem::filesystem_error& e) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove log directory %s: %s\n", + instance.get(), store->path.string().c_str(), e.what()); + } catch (...) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Unknown error removing log directory %s\n", + instance.get(), store->path.string().c_str()); + } } } } - // Phase 4: Delete directories outside the lock - for (auto& store : storesActuallyRemoved) { - try { - std::filesystem::remove_all(store->path); - } catch (const std::filesystem::filesystem_error& e) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove log directory %s: %s\n", - instance.get(), store->path.string().c_str(), e.what()); - } catch (...) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Unknown error removing log directory %s\n", - instance.get(), store->path.string().c_str()); - } - } - return removed; } diff --git a/src/load-binding.ts b/src/load-binding.ts index 04ff17a3e..f4cf86842 100644 --- a/src/load-binding.ts +++ b/src/load-binding.ts @@ -217,11 +217,14 @@ export type TransactionLog = { new (db: NativeDatabase, name: string): TransactionLog; addEntry(data: Buffer | Uint8Array, txnId?: number): void; getLogFileSize(sequenceId?: number): number; + _getNextLogSequenceNumber(sequenceId: number): number; + _getPurgeGeneration(): number; + _purgeGeneration?: number; getStats(): TransactionLogStats; name: string; path: string; query(options?: TransactionLogQueryOptions): IterableIterator; - _currentLogBuffer: LogBuffer; + _currentLogBuffer?: LogBuffer; _findPosition(timestamp: number): number; _getLastCommittedPosition(): Buffer; _getLastFlushed(): number; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index 2ef8e49ff..c24a9ab90 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -214,6 +214,14 @@ Object.defineProperty(TransactionLog.prototype, 'query', { } } + const purgeGeneration = this._getPurgeGeneration(); + if (this._purgeGeneration !== undefined && purgeGeneration !== this._purgeGeneration) { + this._logBuffers?.clear(); + this._currentLogBuffer = undefined; + logBuffer = undefined; + } + this._purgeGeneration = purgeGeneration; + if (logBuffer === undefined || logBuffer.logId !== logId) { // if the current log buffer is not the one we want, load the memory map logBuffer = getLogMemoryMap(this, logId); @@ -228,7 +236,7 @@ Object.defineProperty(TransactionLog.prototype, 'query', { if (logBuffer === undefined) { // create a fake log buffer if we don't have any log buffer yet logBuffer = Buffer.alloc(0) as unknown as LogBuffer; - logBuffer.logId = 0; + logBuffer.logId = logId; logBuffer.size = 0; logBuffer.dataView = new DataView(logBuffer.buffer); // the outer size variable was set from loadLastPosition() above, but if we @@ -265,7 +273,21 @@ Object.defineProperty(TransactionLog.prototype, 'query', { !!readUncommitted ); size = latestSize; - if (latestLogId > logBuffer!.logId) { + if (logBuffer!.length === 0) { + const nextLogBuffer = getNextLogMemoryMap( + transactionLog, + Math.max(0, logBuffer!.logId - 1), + latestLogId + ); + if (!nextLogBuffer) return { done: true, value: undefined }; + logBuffer = nextLogBuffer; + dataView = logBuffer.dataView; + position = TRANSACTION_LOG_FILE_HEADER_SIZE; + size = + latestLogId === logBuffer.logId + ? latestSize + : (logBuffer.size ??= transactionLog.getLogFileSize(logBuffer.logId)); + } else if (latestLogId > logBuffer!.logId) { // if it is not the latest log, get the file size size = logBuffer!.size ?? @@ -464,10 +486,21 @@ function getNextLogMemoryMap( logId: number, latestLogId: number ): LogBuffer | undefined { - while (++logId <= latestLogId) { - const logBuffer = getLogMemoryMap(transactionLog, logId); - if (logBuffer) return logBuffer; + let nextLogId = transactionLog._getNextLogSequenceNumber(logId); + if (nextLogId === 0) { + for (const [cachedLogId, reference] of transactionLog._logBuffers!) { + if ( + cachedLogId > logId && + cachedLogId <= latestLogId && + reference.deref() && + (nextLogId === 0 || cachedLogId < nextLogId) + ) { + nextLogId = cachedLogId; + } + } } + if (nextLogId === 0 || nextLogId > latestLogId) return; + return getLogMemoryMap(transactionLog, nextLogId); } function loadLastPosition( diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index b5d5c0c75..b12926574 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -2165,6 +2165,44 @@ describe('Transaction Log', () => { expect(existsSync(stateFile)).toBe(false); })); + it('does not reuse a mapped log file after a live purge', () => + dbRunner(async ({ db }) => { + const log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('purged'), txn.id); + db.putSync('key', 'value', { transaction: txn }); + }); + db.flushSync(); + expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ + 'purged', + ]); + + expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toHaveLength(1); + expect(Array.from(log.query({ start: 0 }))).toEqual([]); + })); + + it('does not reuse a mapped log file after destroying and recreating its store', () => + dbRunner(async ({ db }) => { + const log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('old'), txn.id); + db.putSync('old', 'value', { transaction: txn }); + }); + db.flushSync(); + expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ + 'old', + ]); + expect(db.purgeLogs({ destroy: true, name: 'foo' })).toHaveLength(1); + + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('replacement'), txn.id); + db.putSync('replacement', 'value', { transaction: txn }); + }); + expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ + 'replacement', + ]); + })); + it('continues after a retained watermark when reopening an empty store', () => dbRunner(async ({ db, dbPath }) => { let database = db; @@ -2240,6 +2278,49 @@ describe('Transaction Log', () => { expect( Array.from(log.query({ start: firstTimestamp + 0.5 }), (entry) => entry.data.toString()) ).toEqual(['third']); + expect( + Array.from(log.query({ startFromLastFlushed: true }), (entry) => entry.data.toString()) + ).toEqual(['third']); + })); + + it('does not enumerate absent sequence numbers after a retained watermark', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + const state = Buffer.alloc(8); + state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); + state.writeUInt32LE(10_000, 4); + await writeFile(join(logDirectory, 'txn.state'), state); + + db.open(); + const log = db.useLog('foo'); + const realGetMemoryMap = log._getMemoryMapOfFile.bind(log); + let probes = 0; + Object.defineProperty(log, '_getMemoryMapOfFile', { + value(sequenceNumber) { + probes++; + return realGetMemoryMap(sequenceNumber); + }, + configurable: true, + }); + try { + expect(Array.from(log.query({ startFromLastFlushed: true }))).toEqual([]); + expect(probes).toBe(1); + } finally { + delete (log as { _getMemoryMapOfFile?: unknown })._getMemoryMapOfFile; + } + })); + + it('rejects a flush watermark that cannot advance to a writable sequence', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + const state = Buffer.alloc(8); + state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); + state.writeUInt32LE(0xffffffff, 4); + await writeFile(join(logDirectory, 'txn.state'), state); + + expect(() => db.open()).toThrow('Transaction log flush watermark sequence is out of range'); })); it('leaves a destroyed store absent after a concurrent flush request', () => From 18dd15a76100e66574ed16e98b5286eb17b72bab Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 00:05:09 -0600 Subject: [PATCH 04/13] Finish transaction-log purge reader recovery --- AGENTS.md | 3 +- .../transaction_log_handle.cpp | 7 ++ .../transaction_log/transaction_log_store.cpp | 4 +- src/load-binding.ts | 2 +- src/transaction-log-reader.ts | 69 ++++++++++--------- test/transaction-log.test.ts | 44 ++++++++---- 6 files changed, 80 insertions(+), 49 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 64a0c4a96..32bb2d25a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -638,7 +638,8 @@ sufficient (env teardown does not honor tsfn acquire counts); see callbacks write `txn.state` through its current pathname rather than a cached file handle, so replacing or removing the path cannot strand later updates in an unlinked inode. When a restart finds a retained watermark but no segments, `load()` continues at the next sequence; reusing - an earlier sequence would make the old watermark falsely prove new entries durable. Readers + an earlier sequence would make the old watermark falsely prove new entries durable, and an + exhausted sequence space fails the database open with the affected store path. Readers traverse the ordered segment set rather than assuming retained sequence numbers are contiguous. ## Debugging native heap corruption diff --git a/src/binding/transaction_log/transaction_log_handle.cpp b/src/binding/transaction_log/transaction_log_handle.cpp index 7ae83787f..e5cc8864e 100644 --- a/src/binding/transaction_log/transaction_log_handle.cpp +++ b/src/binding/transaction_log/transaction_log_handle.cpp @@ -86,6 +86,13 @@ uint32_t TransactionLogHandle::getNextLogSequenceNumber(uint32_t sequenceNumber) uint64_t TransactionLogHandle::getPurgeGeneration() { auto store = this->store.lock(); + if (!store) { + auto dbHandle = this->dbHandle.lock(); + if (dbHandle && dbHandle->opened()) { + store = dbHandle->descriptor->resolveTransactionLogStore(this->logName); + this->store = store; + } + } if (store) this->lastKnownPurgeGeneration = store->getPurgeGeneration(); return this->lastKnownPurgeGeneration; } diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 49507528e..df3f59473 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -1196,11 +1196,11 @@ std::shared_ptr TransactionLogStore::load( (!hasCurrentSegment && flushedPosition.logSequenceNumber == discoveredCurrentSequence && flushedPosition.logSequenceNumber > 0)) { if (flushedPosition.logSequenceNumber == std::numeric_limits::max()) { - throw std::runtime_error("Transaction log flush watermark sequence is out of range"); + throw std::runtime_error("Transaction log flush watermark sequence is out of range for " + path.string()); } uint32_t nextWritableSequence = flushedPosition.logSequenceNumber + 1; if (nextWritableSequence == std::numeric_limits::max()) { - throw std::runtime_error("Transaction log sequence space is exhausted"); + throw std::runtime_error("Transaction log sequence space is exhausted for " + path.string()); } store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); store->nextSequenceNumber = nextWritableSequence + 1; diff --git a/src/load-binding.ts b/src/load-binding.ts index f4cf86842..aeee0ca7b 100644 --- a/src/load-binding.ts +++ b/src/load-binding.ts @@ -229,7 +229,7 @@ export type TransactionLog = { _getLastCommittedPosition(): Buffer; _getLastFlushed(): number; _getMemoryMapOfFile(sequenceId: number): LogBuffer | undefined; - _lastCommittedPosition: Float64Array; + _lastCommittedPosition?: Float64Array; _logBuffers: Map>; }; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index c24a9ab90..19498e02c 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -167,6 +167,13 @@ Object.defineProperty(TransactionLog.prototype, 'query', { exclusiveStart, }: TransactionLogQueryOptions = {} ): IterableIterator { + const purgeGeneration = this._getPurgeGeneration(); + if (this._purgeGeneration !== undefined && purgeGeneration !== this._purgeGeneration) { + this._logBuffers?.clear(); + this._currentLogBuffer = undefined; + this._lastCommittedPosition = undefined; + } + this._purgeGeneration = purgeGeneration; if (!this._lastCommittedPosition) { // if this is the first time we are querying the log, initialize the last committed position and memory map cache const lastCommittedPosition = this._getLastCommittedPosition(); @@ -214,14 +221,6 @@ Object.defineProperty(TransactionLog.prototype, 'query', { } } - const purgeGeneration = this._getPurgeGeneration(); - if (this._purgeGeneration !== undefined && purgeGeneration !== this._purgeGeneration) { - this._logBuffers?.clear(); - this._currentLogBuffer = undefined; - logBuffer = undefined; - } - this._purgeGeneration = purgeGeneration; - if (logBuffer === undefined || logBuffer.logId !== logId) { // if the current log buffer is not the one we want, load the memory map logBuffer = getLogMemoryMap(this, logId); @@ -274,19 +273,19 @@ Object.defineProperty(TransactionLog.prototype, 'query', { ); size = latestSize; if (logBuffer!.length === 0) { - const nextLogBuffer = getNextLogMemoryMap( - transactionLog, - Math.max(0, logBuffer!.logId - 1), - latestLogId - ); - if (!nextLogBuffer) return { done: true, value: undefined }; - logBuffer = nextLogBuffer; - dataView = logBuffer.dataView; - position = TRANSACTION_LOG_FILE_HEADER_SIZE; - size = - latestLogId === logBuffer.logId - ? latestSize - : (logBuffer.size ??= transactionLog.getLogFileSize(logBuffer.logId)); + let searchAfter = Math.max(0, logBuffer!.logId - 1); + do { + const nextLogBuffer = getNextLogMemoryMap(transactionLog, searchAfter, latestLogId); + if (!nextLogBuffer) return { done: true, value: undefined }; + logBuffer = nextLogBuffer; + searchAfter = logBuffer.logId; + dataView = logBuffer.dataView; + position = TRANSACTION_LOG_FILE_HEADER_SIZE; + size = + latestLogId === logBuffer.logId + ? latestSize + : (logBuffer.size ??= transactionLog.getLogFileSize(logBuffer.logId)); + } while (position >= size); } else if (latestLogId > logBuffer!.logId) { // if it is not the latest log, get the file size size = @@ -486,21 +485,25 @@ function getNextLogMemoryMap( logId: number, latestLogId: number ): LogBuffer | undefined { - let nextLogId = transactionLog._getNextLogSequenceNumber(logId); - if (nextLogId === 0) { - for (const [cachedLogId, reference] of transactionLog._logBuffers!) { - if ( - cachedLogId > logId && - cachedLogId <= latestLogId && - reference.deref() && - (nextLogId === 0 || cachedLogId < nextLogId) - ) { - nextLogId = cachedLogId; + while (logId < latestLogId) { + let nextLogId = transactionLog._getNextLogSequenceNumber(logId); + if (nextLogId === 0) { + for (const [cachedLogId, reference] of transactionLog._logBuffers!) { + if ( + cachedLogId > logId && + cachedLogId <= latestLogId && + reference.deref() && + (nextLogId === 0 || cachedLogId < nextLogId) + ) { + nextLogId = cachedLogId; + } } } + if (nextLogId === 0 || nextLogId > latestLogId) return; + const logBuffer = getLogMemoryMap(transactionLog, nextLogId); + if (logBuffer) return logBuffer; + logId = nextLogId; } - if (nextLogId === 0 || nextLogId > latestLogId) return; - return getLogMemoryMap(transactionLog, nextLogId); } function loadLastPosition( diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index b12926574..dd26e499f 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -738,6 +738,19 @@ describe('Transaction Log', () => { ...Array.from({ length: 8 }, (_, i) => String(i).padStart(100, '0')), ...Array.from({ length: 4 }, (_, i) => String(i + 16).padStart(100, '0')), ]); + + database.close(); + unlinkSync(join(dbPath, 'transaction_logs', 'foo', '1.txnlog')); + const state = Buffer.alloc(8); + state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); + state.writeUInt32LE(1, 4); + await writeFile(join(dbPath, 'transaction_logs', 'foo', 'txn.state'), state); + + database = RocksDatabase.open(dbPath, { transactionLogMaxSize: 1000 }); + log = database.useLog('foo'); + expect( + Array.from(log.query({ startFromLastFlushed: true }), (entry) => entry.data.toString()) + ).toEqual(Array.from({ length: 4 }, (_, i) => String(i + 16).padStart(100, '0'))); } finally { database.close(); } @@ -2182,25 +2195,32 @@ describe('Transaction Log', () => { })); it('does not reuse a mapped log file after destroying and recreating its store', () => - dbRunner(async ({ db }) => { + dbRunner(async ({ db, dbPath }) => { const log = db.useLog('foo'); + let readerDatabase; await db.transaction(async (txn) => { log.addEntry(Buffer.from('old'), txn.id); db.putSync('old', 'value', { transaction: txn }); }); db.flushSync(); - expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ - 'old', - ]); - expect(db.purgeLogs({ destroy: true, name: 'foo' })).toHaveLength(1); + try { + readerDatabase = RocksDatabase.open(dbPath); + const readerLog = readerDatabase.useLog('foo') as TransactionLog; + expect( + Array.from(readerLog.query({ start: 0 }), (entry) => entry.data.toString()) + ).toEqual(['old']); + expect(db.purgeLogs({ destroy: true, name: 'foo' })).toHaveLength(1); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('replacement'), txn.id); - db.putSync('replacement', 'value', { transaction: txn }); - }); - expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ - 'replacement', - ]); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('replacement'), txn.id); + db.putSync('replacement', 'value', { transaction: txn }); + }); + expect( + Array.from(readerLog.query({ start: 0 }), (entry) => entry.data.toString()) + ).toEqual(['replacement']); + } finally { + readerDatabase?.close(); + } })); it('continues after a retained watermark when reopening an empty store', () => From 21e9b77ac27c810f595845389a760c3c15dcf990 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 00:12:04 -0600 Subject: [PATCH 05/13] Handle stores disappearing during log queries --- .../transaction_log_handle.cpp | 2 ++ src/load-binding.ts | 2 +- src/transaction-log-reader.ts | 3 +++ test/transaction-log.test.ts | 23 +++++++++++++++---- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/binding/transaction_log/transaction_log_handle.cpp b/src/binding/transaction_log/transaction_log_handle.cpp index e5cc8864e..c11ad23a6 100644 --- a/src/binding/transaction_log/transaction_log_handle.cpp +++ b/src/binding/transaction_log/transaction_log_handle.cpp @@ -89,6 +89,8 @@ uint64_t TransactionLogHandle::getPurgeGeneration() { if (!store) { auto dbHandle = this->dbHandle.lock(); if (dbHandle && dbHandle->opened()) { + // A live query revives a destroyed log like useLog() and collectStats(); the + // new store generation makes JS discard every mapping from the old store. store = dbHandle->descriptor->resolveTransactionLogStore(this->logName); this->store = store; } diff --git a/src/load-binding.ts b/src/load-binding.ts index aeee0ca7b..5016b3565 100644 --- a/src/load-binding.ts +++ b/src/load-binding.ts @@ -226,7 +226,7 @@ export type TransactionLog = { query(options?: TransactionLogQueryOptions): IterableIterator; _currentLogBuffer?: LogBuffer; _findPosition(timestamp: number): number; - _getLastCommittedPosition(): Buffer; + _getLastCommittedPosition(): Buffer | undefined; _getLastFlushed(): number; _getMemoryMapOfFile(sequenceId: number): LogBuffer | undefined; _lastCommittedPosition?: Float64Array; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index 19498e02c..b9d8f4bd2 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -177,6 +177,9 @@ Object.defineProperty(TransactionLog.prototype, 'query', { if (!this._lastCommittedPosition) { // if this is the first time we are querying the log, initialize the last committed position and memory map cache const lastCommittedPosition = this._getLastCommittedPosition(); + if (!lastCommittedPosition) { + return [][Symbol.iterator]() as IterableIterator; + } this._lastCommittedPosition = new Float64Array(lastCommittedPosition.buffer); this._logBuffers = new Map>(); } diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index dd26e499f..33bfa1c36 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -265,7 +265,7 @@ describe('Transaction Log', () => { await db.transaction(async (txn) => { log.addEntry(value, txn.id); }); - const positionBuffer = log._getLastCommittedPosition(); + const positionBuffer = log._getLastCommittedPosition()!; const dataView = new DataView(positionBuffer.buffer); expect(dataView.getUint32(0)).toBeGreaterThan(10); const sequenceNumber = dataView.getUint32(1); @@ -2191,6 +2191,19 @@ describe('Transaction Log', () => { ]); expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toHaveLength(1); + const lastPositionDescriptor = Object.getOwnPropertyDescriptor( + Object.getPrototypeOf(log), + '_getLastCommittedPosition' + )!; + Object.defineProperty(log, '_getLastCommittedPosition', { + value: () => undefined, + configurable: true, + }); + try { + expect(Array.from(log.query({ start: 0 }))).toEqual([]); + } finally { + Object.defineProperty(log, '_getLastCommittedPosition', lastPositionDescriptor); + } expect(Array.from(log.query({ start: 0 }))).toEqual([]); })); @@ -2241,7 +2254,7 @@ describe('Transaction Log', () => { database = RocksDatabase.open(dbPath); log = database.useLog('foo'); - const lastCommittedBuffer = log._getLastCommittedPosition(); + const lastCommittedBuffer = log._getLastCommittedPosition()!; const lastCommitted = new Uint32Array( lastCommittedBuffer.buffer, lastCommittedBuffer.byteOffset, @@ -2280,7 +2293,7 @@ describe('Transaction Log', () => { db.open(); log = db.useLog('foo'); - const lastCommittedBuffer = log._getLastCommittedPosition(); + const lastCommittedBuffer = log._getLastCommittedPosition()!; const lastCommitted = new Uint32Array( lastCommittedBuffer.buffer, lastCommittedBuffer.byteOffset, @@ -2611,7 +2624,7 @@ describe('Transaction Log', () => { const log = db.useLog('foo'); // Get initial lastCommittedPosition - should be valid even though no commits yet - let lastCommittedBuffer = log._getLastCommittedPosition(); + let lastCommittedBuffer = log._getLastCommittedPosition()!; let lastCommittedPosUint32 = new Uint32Array(lastCommittedBuffer.buffer, 0, 2); expect(lastCommittedPosUint32[1]).toBeGreaterThanOrEqual(1); expect(lastCommittedPosUint32[0]).toBeGreaterThanOrEqual(10); @@ -2643,7 +2656,7 @@ describe('Transaction Log', () => { const log2 = db.useLog('foo'); // After reopening, lastCommittedPosition should be valid and point to log file 2 - lastCommittedBuffer = log2._getLastCommittedPosition(); + lastCommittedBuffer = log2._getLastCommittedPosition()!; lastCommittedPosUint32 = new Uint32Array(lastCommittedBuffer.buffer, 0, 2); expect(lastCommittedPosUint32[1]).toBeGreaterThanOrEqual(2); expect(lastCommittedPosUint32[0]).toBeGreaterThanOrEqual(10); From 9f3456b20b7b2daa21d562036b3461b45ba6edab Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 00:34:17 -0600 Subject: [PATCH 06/13] Detach destroyed logs before asynchronous cleanup --- .../transaction_log/transaction_log_store.cpp | 4 +- .../transaction_log_store_registry.cpp | 73 ++++++++++++++----- src/transaction-log-reader.ts | 6 +- test/transaction-log.test.ts | 6 +- 4 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index df3f59473..81396393f 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -1196,11 +1196,11 @@ std::shared_ptr TransactionLogStore::load( (!hasCurrentSegment && flushedPosition.logSequenceNumber == discoveredCurrentSequence && flushedPosition.logSequenceNumber > 0)) { if (flushedPosition.logSequenceNumber == std::numeric_limits::max()) { - throw std::runtime_error("Transaction log flush watermark sequence is out of range for " + path.string()); + throw rocksdb_js::DBException("Transaction log flush watermark sequence is out of range for " + path.string()); } uint32_t nextWritableSequence = flushedPosition.logSequenceNumber + 1; if (nextWritableSequence == std::numeric_limits::max()) { - throw std::runtime_error("Transaction log sequence space is exhausted for " + path.string()); + throw rocksdb_js::DBException("Transaction log sequence space is exhausted for " + path.string()); } store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); store->nextSequenceNumber = nextWritableSequence + 1; diff --git a/src/binding/transaction_log/transaction_log_store_registry.cpp b/src/binding/transaction_log/transaction_log_store_registry.cpp index dff5c02a6..9c1a60c8a 100644 --- a/src/binding/transaction_log/transaction_log_store_registry.cpp +++ b/src/binding/transaction_log/transaction_log_store_registry.cpp @@ -4,6 +4,7 @@ #include "core/platform.h" #include "napi/helpers.h" #include "napi/async.h" +#include #include #include @@ -12,6 +13,10 @@ namespace rocksdb_js { // Initialize the static instance std::unique_ptr TransactionLogStoreRegistry::instance; +namespace { +std::atomic nextDeletionId { 0 }; +} + /** * Initializes the singleton instance. */ @@ -147,7 +152,18 @@ void TransactionLogStoreRegistry::DiscoverStores(const std::string& dbPath) { config = &entry->config; } - if (transactionLogsPath.empty() || !std::filesystem::exists(transactionLogsPath)) { + if (transactionLogsPath.empty()) { + return; + } + + // A process can exit after atomically detaching a destroyed store but before + // its files are removed. These paths are outside the discovery directory. + auto deletionRoot = std::filesystem::path(transactionLogsPath); + deletionRoot += ".deleting"; + std::error_code cleanupError; + std::filesystem::remove_all(deletionRoot, cleanupError); + + if (!std::filesystem::exists(transactionLogsPath)) { DEBUG_LOG("%p TransactionLogStoreRegistry::DiscoverStores No transaction logs path or directory does not exist for \"%s\"\n", instance.get(), dbPath.c_str()); return; @@ -360,28 +376,49 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str } } - // Phase 3: Remove closed stores and their directories while holding the lock so - // ResolveStore cannot publish a replacement at the same path before deletion finishes. + // Phase 3: Atomically detach closed store directories while holding the registry + // lock, then remove them without blocking flush callbacks or store resolution. if (destroy) { - std::lock_guard storeLock(entry->storesMutex); - for (auto& store : storesToPurge) { - if (!store->isClosing.load(std::memory_order_relaxed)) { - continue; - } - auto storeIt = entry->stores.find(store->name); - if (storeIt != entry->stores.end() && storeIt->second.get() == store.get()) { + auto deletionRoot = std::filesystem::path(entry->config.transactionLogsPath); + deletionRoot += ".deleting"; + rocksdb_js::tryCreateDirectory(deletionRoot); + std::vector pathsToRemove; + { + std::lock_guard storeLock(entry->storesMutex); + for (auto& store : storesToPurge) { + if (!store->isClosing.load(std::memory_order_relaxed)) { + continue; + } + auto storeIt = entry->stores.find(store->name); + if (storeIt == entry->stores.end() || storeIt->second.get() != store.get()) { + continue; + } + + auto deletionPath = deletionRoot / + (store->name + "-" + std::to_string(nextDeletionId.fetch_add(1, std::memory_order_relaxed))); + std::error_code renameError; + std::filesystem::rename(store->path, deletionPath, renameError); + if (renameError && renameError != std::errc::no_such_file_or_directory) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to detach log directory %s: %s\n", + instance.get(), store->path.string().c_str(), renameError.message().c_str()); + continue; + } entry->stores.erase(storeIt); - try { - std::filesystem::remove_all(store->path); - } catch (const std::filesystem::filesystem_error& e) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove log directory %s: %s\n", - instance.get(), store->path.string().c_str(), e.what()); - } catch (...) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Unknown error removing log directory %s\n", - instance.get(), store->path.string().c_str()); + if (!renameError) { + pathsToRemove.push_back(std::move(deletionPath)); } } } + for (const auto& deletionPath : pathsToRemove) { + std::error_code removeError; + std::filesystem::remove_all(deletionPath, removeError); + if (removeError) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove detached log directory %s: %s\n", + instance.get(), deletionPath.string().c_str(), removeError.message().c_str()); + } + } + std::error_code removeRootError; + std::filesystem::remove(deletionRoot, removeRootError); } return removed; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index b9d8f4bd2..25fb107eb 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -169,9 +169,13 @@ Object.defineProperty(TransactionLog.prototype, 'query', { ): IterableIterator { const purgeGeneration = this._getPurgeGeneration(); if (this._purgeGeneration !== undefined && purgeGeneration !== this._purgeGeneration) { + const lastCommittedPosition = this._getLastCommittedPosition(); + if (!lastCommittedPosition) { + return [][Symbol.iterator]() as IterableIterator; + } this._logBuffers?.clear(); this._currentLogBuffer = undefined; - this._lastCommittedPosition = undefined; + this._lastCommittedPosition = new Float64Array(lastCommittedPosition.buffer); } this._purgeGeneration = purgeGeneration; if (!this._lastCommittedPosition) { diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index 33bfa1c36..7eb02625b 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -2186,9 +2186,8 @@ describe('Transaction Log', () => { db.putSync('key', 'value', { transaction: txn }); }); db.flushSync(); - expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ - 'purged', - ]); + const existingIterator = log.query({ start: 0 }); + expect(existingIterator.next().value.data.toString()).toBe('purged'); expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toHaveLength(1); const lastPositionDescriptor = Object.getOwnPropertyDescriptor( @@ -2201,6 +2200,7 @@ describe('Transaction Log', () => { }); try { expect(Array.from(log.query({ start: 0 }))).toEqual([]); + expect(existingIterator.next()).toEqual({ done: true, value: undefined }); } finally { Object.defineProperty(log, '_getLastCommittedPosition', lastPositionDescriptor); } From 05ecd7ecedd6dd50227c35fc3d442e54ded51948 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 00:42:41 -0600 Subject: [PATCH 07/13] Fail closed when log deletion cannot detach --- AGENTS.md | 3 +- README.md | 3 +- .../transaction_log_handle.cpp | 3 +- .../transaction_log_store_registry.cpp | 45 +++++++++++++------ src/transaction-log-reader.ts | 2 +- test/transaction-log.test.ts | 19 +++++++- 6 files changed, 56 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 32bb2d25a..96ed13290 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -634,7 +634,8 @@ sufficient (env teardown does not honor tsfn acquire counts); see `txn.state`; startup age-based retention goes through the same persisted-flush gate. Destructive teardown belongs to `TransactionLogStoreRegistry::PurgeStores()`'s `destroy` path, which closes the store and holds the per-database store-registry lock across - unregistering and directory removal so a same-name replacement cannot be deleted. Flush + unregistering and atomically detaching its directory; recursive removal happens after the lock + is released so a same-name replacement cannot be deleted or block flush callbacks. Flush callbacks write `txn.state` through its current pathname rather than a cached file handle, so replacing or removing the path cannot strand later updates in an unlinked inode. When a restart finds a retained watermark but no segments, `load()` continues at the next sequence; reusing diff --git a/README.md b/README.md index a39cd368d..13858a587 100644 --- a/README.md +++ b/README.md @@ -1621,7 +1621,8 @@ const names = db.listLogs(); Deletes transaction log files older than the `transactionLogRetention` (defaults to 3 days). Startup and runtime retention remove only segments proven flushed, and retain the live log store -and its flush watermark; use `destroy: true` to close and remove a store completely. +and its flush watermark; use `destroy: true` to close and remove the current store. A later +`useLog()` call or query through a retained handle creates a new empty store with the same name. - `options: object` - `before?: number` Remove all transaction log files older than the specified timestamp. diff --git a/src/binding/transaction_log/transaction_log_handle.cpp b/src/binding/transaction_log/transaction_log_handle.cpp index c11ad23a6..003498123 100644 --- a/src/binding/transaction_log/transaction_log_handle.cpp +++ b/src/binding/transaction_log/transaction_log_handle.cpp @@ -89,8 +89,7 @@ uint64_t TransactionLogHandle::getPurgeGeneration() { if (!store) { auto dbHandle = this->dbHandle.lock(); if (dbHandle && dbHandle->opened()) { - // A live query revives a destroyed log like useLog() and collectStats(); the - // new store generation makes JS discard every mapping from the old store. + // The new store generation makes JS discard every mapping from the old store. store = dbHandle->descriptor->resolveTransactionLogStore(this->logName); this->store = store; } diff --git a/src/binding/transaction_log/transaction_log_store_registry.cpp b/src/binding/transaction_log/transaction_log_store_registry.cpp index 9c1a60c8a..f36f061e2 100644 --- a/src/binding/transaction_log/transaction_log_store_registry.cpp +++ b/src/binding/transaction_log/transaction_log_store_registry.cpp @@ -156,19 +156,23 @@ void TransactionLogStoreRegistry::DiscoverStores(const std::string& dbPath) { return; } - // A process can exit after atomically detaching a destroyed store but before - // its files are removed. These paths are outside the discovery directory. - auto deletionRoot = std::filesystem::path(transactionLogsPath); - deletionRoot += ".deleting"; - std::error_code cleanupError; - std::filesystem::remove_all(deletionRoot, cleanupError); - if (!std::filesystem::exists(transactionLogsPath)) { DEBUG_LOG("%p TransactionLogStoreRegistry::DiscoverStores No transaction logs path or directory does not exist for \"%s\"\n", instance.get(), dbPath.c_str()); return; } + // A process can exit after atomically detaching a destroyed store but before + // its files are removed. Keep the root itself so concurrent detaches remain valid. + auto deletionRoot = std::filesystem::path(transactionLogsPath); + deletionRoot += ".deleting"; + rocksdb_js::tryCreateDirectory(deletionRoot); + std::error_code cleanupError; + for (std::filesystem::directory_iterator it(deletionRoot, cleanupError), end; !cleanupError && it != end; it.increment(cleanupError)) { + std::error_code removeError; + std::filesystem::remove_all(it->path(), removeError); + } + std::lock_guard storeLock(entry->storesMutex); for (const auto& dirEntry : std::filesystem::directory_iterator(transactionLogsPath)) { @@ -338,6 +342,19 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str entry = it->second; } + std::filesystem::path deletionRoot; + if (destroy) { + deletionRoot = std::filesystem::path(entry->config.transactionLogsPath); + deletionRoot += ".deleting"; + rocksdb_js::tryCreateDirectory(deletionRoot); + std::error_code rootError; + if (!std::filesystem::is_directory(deletionRoot, rootError)) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Cannot create deletion directory %s: %s\n", + instance.get(), deletionRoot.string().c_str(), rootError.message().c_str()); + return removed; + } + } + size_t i = 0; std::vector> storesToPurge; @@ -379,9 +396,6 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str // Phase 3: Atomically detach closed store directories while holding the registry // lock, then remove them without blocking flush callbacks or store resolution. if (destroy) { - auto deletionRoot = std::filesystem::path(entry->config.transactionLogsPath); - deletionRoot += ".deleting"; - rocksdb_js::tryCreateDirectory(deletionRoot); std::vector pathsToRemove; { std::lock_guard storeLock(entry->storesMutex); @@ -398,7 +412,14 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str (store->name + "-" + std::to_string(nextDeletionId.fetch_add(1, std::memory_order_relaxed))); std::error_code renameError; std::filesystem::rename(store->path, deletionPath, renameError); - if (renameError && renameError != std::errc::no_such_file_or_directory) { + if (renameError == std::errc::no_such_file_or_directory) { + std::error_code sourceError; + if (std::filesystem::exists(store->path, sourceError) || sourceError) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to detach log directory %s: %s\n", + instance.get(), store->path.string().c_str(), renameError.message().c_str()); + continue; + } + } else if (renameError) { DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to detach log directory %s: %s\n", instance.get(), store->path.string().c_str(), renameError.message().c_str()); continue; @@ -417,8 +438,6 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str instance.get(), deletionPath.string().c_str(), removeError.message().c_str()); } } - std::error_code removeRootError; - std::filesystem::remove(deletionRoot, removeRootError); } return removed; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index 25fb107eb..4b57aa17a 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -173,7 +173,7 @@ Object.defineProperty(TransactionLog.prototype, 'query', { if (!lastCommittedPosition) { return [][Symbol.iterator]() as IterableIterator; } - this._logBuffers?.clear(); + this._logBuffers = new Map>(); this._currentLogBuffer = undefined; this._lastCommittedPosition = new Float64Array(lastCommittedPosition.buffer); } diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index 7eb02625b..465f4343b 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -12,7 +12,7 @@ import { dbRunner, generateDBPath, terminateWorker } from './lib/util.ts'; import { createWorkerBootstrapScript } from './lib/worker-bootstrap.ts'; import assert from 'node:assert'; import { existsSync, readFileSync, statSync, unlinkSync } from 'node:fs'; -import { mkdir, readdir, stat, utimes, writeFile } from 'node:fs/promises'; +import { mkdir, readdir, rm, stat, utimes, writeFile } from 'node:fs/promises'; import { release } from 'node:os'; import { join } from 'node:path'; import { setTimeout as delay } from 'node:timers/promises'; @@ -2371,6 +2371,23 @@ describe('Transaction Log', () => { expect(existsSync(join(dbPath, 'transaction_logs', 'foo'))).toBe(false); })); + it('keeps a store registered when its deletion staging directory is unavailable', () => + dbRunner(async ({ db, dbPath }) => { + const log = db.useLog('foo'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.from('value'), txn.id); + db.putSync('key', 'value', { transaction: txn }); + }); + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + const deletionRoot = join(dbPath, 'transaction_logs.deleting'); + await rm(deletionRoot, { recursive: true, force: true }); + await writeFile(deletionRoot, 'not a directory'); + + expect(db.purgeLogs({ destroy: true, name: 'foo' })).toEqual([]); + expect(db.listLogs()).toContain('foo'); + expect(existsSync(logDirectory)).toBe(true); + })); + it('should return entry counts when includeEntryCounts is true', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); From 93ee204562a2f060a8c67a4860d29258675414e7 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 00:47:54 -0600 Subject: [PATCH 08/13] Keep read-only log discovery side-effect free --- README.md | 2 ++ src/binding/transaction_log/transaction_log_store_registry.cpp | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 13858a587..a1f9d30f1 100644 --- a/README.md +++ b/README.md @@ -1623,6 +1623,8 @@ Deletes transaction log files older than the `transactionLogRetention` (defaults Startup and runtime retention remove only segments proven flushed, and retain the live log store and its flush watermark; use `destroy: true` to close and remove the current store. A later `useLog()` call or query through a retained handle creates a new empty store with the same name. +If no flush watermark has been recorded yet, retention conservatively keeps eligible segments; +`log.getStats().purge.retainedUnflushedFiles` reports how many are waiting for a durable flush. - `options: object` - `before?: number` Remove all transaction log files older than the specified timestamp. diff --git a/src/binding/transaction_log/transaction_log_store_registry.cpp b/src/binding/transaction_log/transaction_log_store_registry.cpp index f36f061e2..7cab3acb2 100644 --- a/src/binding/transaction_log/transaction_log_store_registry.cpp +++ b/src/binding/transaction_log/transaction_log_store_registry.cpp @@ -166,7 +166,6 @@ void TransactionLogStoreRegistry::DiscoverStores(const std::string& dbPath) { // its files are removed. Keep the root itself so concurrent detaches remain valid. auto deletionRoot = std::filesystem::path(transactionLogsPath); deletionRoot += ".deleting"; - rocksdb_js::tryCreateDirectory(deletionRoot); std::error_code cleanupError; for (std::filesystem::directory_iterator it(deletionRoot, cleanupError), end; !cleanupError && it != end; it.increment(cleanupError)) { std::error_code removeError; From 1adbf04b3c869c1c5e4e4caefe4595023acf0a49 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 06:25:17 -0600 Subject: [PATCH 09/13] fix(transaction-log): retain live sequence during purge --- AGENTS.md | 17 +- README.md | 13 +- docs/stats.md | 3 +- docs/transaction-log.md | 4 +- .../transaction_log/transaction_log.cpp | 20 - src/binding/transaction_log/transaction_log.h | 2 - .../transaction_log_handle.cpp | 23 - .../transaction_log/transaction_log_handle.h | 3 - .../transaction_log/transaction_log_store.cpp | 214 +++++----- .../transaction_log/transaction_log_store.h | 18 +- .../transaction_log_store_registry.cpp | 96 +---- src/load-binding.ts | 9 +- src/transaction-log-reader.ts | 129 ++---- test/transaction-log-stats.test.ts | 18 + test/transaction-log.test.ts | 392 +++--------------- 15 files changed, 273 insertions(+), 688 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 96ed13290..25637dac7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -629,19 +629,10 @@ sufficient (env teardown does not honor tsfn acquire counts); see never reproduces natively or on glibc, so the repro test is `skipIf(darwin)` (and, like the repo's other teardown repros, gated to Node). -18. **Only a closed transaction-log store may lose its directory or flush watermark**: ordinary - retention purges remove eligible `.txnlog` segments but keep the live store directory and - `txn.state`; startup age-based retention goes through the same persisted-flush gate. Destructive - teardown belongs to `TransactionLogStoreRegistry::PurgeStores()`'s - `destroy` path, which closes the store and holds the per-database store-registry lock across - unregistering and atomically detaching its directory; recursive removal happens after the lock - is released so a same-name replacement cannot be deleted or block flush callbacks. Flush - callbacks write `txn.state` through its current pathname rather than a cached file handle, so - replacing or removing the path cannot strand later updates in an unlinked inode. When a restart - finds a retained watermark but no segments, `load()` continues at the next sequence; reusing - an earlier sequence would make the old watermark falsely prove new entries durable, and an - exhausted sequence space fails the database open with the affected store path. Readers - traverse the ordered segment set rather than assuming retained sequence numbers are contiguous. +18. **Ordinary transaction-log retention removes only a contiguous oldest prefix**: it never + removes the highest sequence file or the live store directory. Stop the scan when an older + file is ineligible or cannot be removed; continuing could create a sequence gap. Destructive + store removal is the separate `destroy` path. ## Debugging native heap corruption diff --git a/README.md b/README.md index a1f9d30f1..12ba46455 100644 --- a/README.md +++ b/README.md @@ -1620,11 +1620,11 @@ const names = db.listLogs(); ### `db.purgeLogs({ includeEntryCounts: true, ...options }): { path: string; entries: number }[]` Deletes transaction log files older than the `transactionLogRetention` (defaults to 3 days). -Startup and runtime retention remove only segments proven flushed, and retain the live log store -and its flush watermark; use `destroy: true` to close and remove the current store. A later -`useLog()` call or query through a retained handle creates a new empty store with the same name. -If no flush watermark has been recorded yet, retention conservatively keeps eligible segments; -`log.getStats().purge.retainedUnflushedFiles` reports how many are waiting for a durable flush. +Ordinary retention keeps the highest sequence file as the live store's durable floor and removes +only an eligible contiguous prefix of older files. An idle store can therefore retain one file +past the cutoff until a later write rotates it; disk retained past the cutoff is bounded by that +store's `transactionLogMaxSize` (except when a single transaction exceeds the target). Use +`destroy: true` only to remove the store itself. - `options: object` - `before?: number` Remove all transaction log files older than the specified timestamp. @@ -1801,7 +1801,8 @@ stats.totals.transactionsWritten; // lifetime count of transactions written The `purge.retainedUnflushedFiles` gauge is useful for diagnosing why logs are not being cleaned up: a file can be older than the retention period but still retained because its transactions have -not yet been flushed to RocksDB (purging it would be unsafe for crash recovery). +not yet been flushed to RocksDB (purging it would be unsafe for crash recovery). The highest +sequence file is not counted as purgeable even when it is old and fully flushed. ### Transaction Log Initialization diff --git a/docs/stats.md b/docs/stats.md index e410083d6..1ff199539 100644 --- a/docs/stats.md +++ b/docs/stats.md @@ -477,7 +477,8 @@ const stats: TransactionLogStats = log.getStats(); - `offset: number` The byte offset within that file. - `purge: object` - `oldestFileAgeMs: number` Age in milliseconds of the oldest file on disk. - - `purgeableFiles: number` Number of files eligible for purge under the retention policy. + - `purgeableFiles: number` Number of files below the durable highest-sequence floor that are + eligible for purge under the retention policy. - `retainedUnflushedFiles: number` Number of files past the retention threshold but retained because they are unflushed. - `lastPurgeMs: number` Timestamp of the last purge scan. diff --git a/docs/transaction-log.md b/docs/transaction-log.md index 8c1df8ad2..41e972770 100644 --- a/docs/transaction-log.md +++ b/docs/transaction-log.md @@ -198,7 +198,9 @@ await db.transaction((txn) => { - Log files are automatically rotated when either the index or data file reaches their configured maximum sizes - Rotation happens on the next write after the size limit is exceeded -- Old log files can be automatically purged based on retention policy +- Old log files can be automatically purged based on retention policy. The highest sequence file is + retained as the live store's durable floor, so an idle store can keep one bounded file past the + cutoff until a later write rotates it. ### Error Handling diff --git a/src/binding/transaction_log/transaction_log.cpp b/src/binding/transaction_log/transaction_log.cpp index 97c883e31..99fbd76a7 100644 --- a/src/binding/transaction_log/transaction_log.cpp +++ b/src/binding/transaction_log/transaction_log.cpp @@ -183,24 +183,6 @@ napi_value TransactionLog::GetLogFileSize(napi_env env, napi_callback_info info) return result; } -napi_value TransactionLog::GetNextLogSequenceNumber(napi_env env, napi_callback_info info) { - NAPI_METHOD_ARGV(1); - UNWRAP_TRANSACTION_LOG_HANDLE("GetNextLogSequenceNumber"); - uint32_t sequenceNumber; - NAPI_STATUS_THROWS(::napi_get_value_uint32(env, argv[0], &sequenceNumber)); - napi_value result; - NAPI_STATUS_THROWS(::napi_create_uint32(env, (*txnLogHandle)->getNextLogSequenceNumber(sequenceNumber), &result)); - return result; -} - -napi_value TransactionLog::GetPurgeGeneration(napi_env env, napi_callback_info info) { - NAPI_METHOD(); - UNWRAP_TRANSACTION_LOG_HANDLE("GetPurgeGeneration"); - napi_value result; - NAPI_STATUS_THROWS(::napi_create_double(env, static_cast((*txnLogHandle)->getPurgeGeneration()), &result)); - return result; -} - struct PositionHandle { std::shared_ptr position; }; @@ -458,8 +440,6 @@ void TransactionLog::Init(napi_env env, napi_value exports) { { "_findPosition", nullptr, FindPosition, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getLastCommittedPosition", nullptr, GetLastCommittedPosition, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getMemoryMapOfFile", nullptr, GetMemoryMapOfFile, nullptr, nullptr, nullptr, napi_default, nullptr }, - { "_getNextLogSequenceNumber", nullptr, GetNextLogSequenceNumber, nullptr, nullptr, nullptr, napi_default, nullptr }, - { "_getPurgeGeneration", nullptr, GetPurgeGeneration, nullptr, nullptr, nullptr, napi_default, nullptr }, { "_getLastFlushed", nullptr, GetLastFlushed, nullptr, nullptr, nullptr, napi_default, nullptr } }; diff --git a/src/binding/transaction_log/transaction_log.h b/src/binding/transaction_log/transaction_log.h index 2aa8b5ff6..b16b1ffa6 100644 --- a/src/binding/transaction_log/transaction_log.h +++ b/src/binding/transaction_log/transaction_log.h @@ -15,8 +15,6 @@ struct TransactionLog final { static napi_value GetLastCommittedPosition(napi_env env, napi_callback_info info); static napi_value GetLastFlushed(napi_env env, napi_callback_info info); static napi_value GetLogFileSize(napi_env env, napi_callback_info info); - static napi_value GetNextLogSequenceNumber(napi_env env, napi_callback_info info); - static napi_value GetPurgeGeneration(napi_env env, napi_callback_info info); static napi_value GetMemoryMapOfFile(napi_env env, napi_callback_info info); static napi_value GetName(napi_env env, napi_callback_info info); static napi_value GetPath(napi_env env, napi_callback_info info); diff --git a/src/binding/transaction_log/transaction_log_handle.cpp b/src/binding/transaction_log/transaction_log_handle.cpp index 003498123..d7b5205da 100644 --- a/src/binding/transaction_log/transaction_log_handle.cpp +++ b/src/binding/transaction_log/transaction_log_handle.cpp @@ -15,9 +15,6 @@ TransactionLogHandle::TransactionLogHandle( ): dbHandle(dbHandle), logName(logName), readOnly(readOnly), transactionId(0) { DEBUG_LOG("%p TransactionLogHandle::TransactionLogHandle Creating TransactionLogHandle \"%s\"\n", this, logName.c_str()); this->store = dbHandle->descriptor->resolveTransactionLogStore(logName); - if (auto store = this->store.lock()) { - this->lastKnownPurgeGeneration = store->getPurgeGeneration(); - } } TransactionLogHandle::~TransactionLogHandle() { @@ -78,26 +75,6 @@ uint64_t TransactionLogHandle::getLogFileSize(uint32_t sequenceNumber) { return 0; } -uint32_t TransactionLogHandle::getNextLogSequenceNumber(uint32_t sequenceNumber) { - auto store = this->store.lock(); - if (store) return store->getNextLogSequenceNumber(sequenceNumber); - return 0; -} - -uint64_t TransactionLogHandle::getPurgeGeneration() { - auto store = this->store.lock(); - if (!store) { - auto dbHandle = this->dbHandle.lock(); - if (dbHandle && dbHandle->opened()) { - // The new store generation makes JS discard every mapping from the old store. - store = dbHandle->descriptor->resolveTransactionLogStore(this->logName); - this->store = store; - } - } - if (store) this->lastKnownPurgeGeneration = store->getPurgeGeneration(); - return this->lastKnownPurgeGeneration; -} - std::shared_ptr TransactionLogHandle::getMemoryMap(uint32_t sequenceNumber) { auto store = this->store.lock(); if (store) return store->getMemoryMap(sequenceNumber); diff --git a/src/binding/transaction_log/transaction_log_handle.h b/src/binding/transaction_log/transaction_log_handle.h index 73476a634..993dff896 100644 --- a/src/binding/transaction_log/transaction_log_handle.h +++ b/src/binding/transaction_log/transaction_log_handle.h @@ -36,7 +36,6 @@ struct TransactionLogHandle final : Closable { * The transaction id. */ uint32_t transactionId; - uint64_t lastKnownPurgeGeneration = 0; /** * Creates a new transaction log handle. @@ -65,8 +64,6 @@ struct TransactionLogHandle final : Closable { LogPosition findPosition(double timestamp); LogPosition getLastFlushed(); uint64_t getLogFileSize(uint32_t sequenceNumber); - uint32_t getNextLogSequenceNumber(uint32_t sequenceNumber); - uint64_t getPurgeGeneration(); std::weak_ptr getLastCommittedPosition(); /** diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 81396393f..39d95c891 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include #include "transaction_log_store.h" @@ -12,8 +11,6 @@ namespace rocksdb_js { -static std::atomic nextTransactionLogStoreGeneration = 1; - // Helper function to extract exception message from exception_ptr static std::string getExceptionMessage(std::exception_ptr eptr) { if (!eptr) { @@ -46,7 +43,6 @@ TransactionLogStore::TransactionLogStore( maxAgeThreshold(maxAgeThreshold) { DEBUG_LOG("%p TransactionLogStore::TransactionLogStore Opening transaction log store \"%s\"\n", this, this->name.c_str()); - this->purgeGeneration.store(nextTransactionLogStoreGeneration.fetch_add(1, std::memory_order_relaxed), std::memory_order_relaxed); lastCommittedPosition = std::make_shared(); uncommittedTransactionPositions.reserve(16); for (int i = 0; i < RECENTLY_COMMITTED_POSITIONS_SIZE; i++) { // initialize recent commits to not match until values are entered @@ -72,16 +68,22 @@ void TransactionLogStore::doClose() { it = this->sequenceFiles.erase(it); } - for (auto& logFile : logFilesToClose) { - logFile->close(); - } - - // Drain any flush callback that entered before isClosing was set. A callback - // that arrives later will observe isClosing while holding this mutex and exit. + // Close the state file if it's open. flushedStateMutex must be held for + // all flushedStateFile access; release it before calling doPurge() since + // doPurge() → getLastFlushedPosition() will acquire flushedStateMutex + // itself (and we must not hold it when doPurge re-acquires it). { std::lock_guard flushedLock(this->flushedStateMutex); + if (this->flushedStateFile.is_open()) { + this->flushedStateFile.close(); + } } + for (auto& logFile : logFilesToClose) { + logFile->close(); + } + + this->doPurge(); } void TransactionLogStore::close() { @@ -99,32 +101,23 @@ void TransactionLogStore::close() { } bool TransactionLogStore::tryClose() { - auto finishClose = [this]() { - std::lock_guard writeLock(this->writeMutex); - std::lock_guard dataLock(this->dataSetsMutex); - this->doClose(); - return true; - }; - // Fast path: already closing. if (this->isClosing.load(std::memory_order_relaxed)) { - return finishClose(); + return true; } // Phase 1 — quick count check under transactionBindMutex. // transactionBindMutex is a lightweight lock used only for pendingTransactionCount // increments and the isClosing assignment; it is never held during I/O. - bool closeInProgress = false; { std::lock_guard bindLock(this->transactionBindMutex); - closeInProgress = this->isClosing.load(std::memory_order_relaxed); - if (!closeInProgress && this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { + if (this->isClosing.load(std::memory_order_relaxed)) return true; + if (this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { DEBUG_LOG("%p TransactionLogStore::tryClose Skipping (phase 1): pendingTransactionCount=%d\n", this, this->pendingTransactionCount.load(std::memory_order_relaxed)); return false; } } - if (closeInProgress || this->isClosing.load(std::memory_order_relaxed)) return finishClose(); // Phase 2 — drain any in-progress writeBatch and check uncommitted positions. // Acquiring writeMutex here blocks until any concurrent writeBatch() finishes @@ -133,10 +126,7 @@ bool TransactionLogStore::tryClose() { { std::lock_guard writeLock(this->writeMutex); std::lock_guard dataLock(this->dataSetsMutex); - if (this->isClosing.load(std::memory_order_relaxed)) { - this->doClose(); - return true; - } + if (this->isClosing.load(std::memory_order_relaxed)) return true; for (const auto& pos : this->uncommittedTransactionPositions) { if (pos.positionInLogFile == this->nextLogPosition.positionInLogFile && pos.logSequenceNumber == this->nextLogPosition.logSequenceNumber) { @@ -152,21 +142,17 @@ bool TransactionLogStore::tryClose() { // A new UseLog/addLogEntry may have bound a transaction between phases 1 and 3, // so we must re-verify. Once isClosing is set here, all future bind attempts // will fail (they also check isClosing under transactionBindMutex). - bool closeAlreadyClaimed = false; { std::lock_guard bindLock(this->transactionBindMutex); - closeAlreadyClaimed = this->isClosing.load(std::memory_order_relaxed); - if (!closeAlreadyClaimed && this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { + if (this->isClosing.load(std::memory_order_relaxed)) return true; + if (this->pendingTransactionCount.load(std::memory_order_relaxed) > 0) { DEBUG_LOG("%p TransactionLogStore::tryClose Skipping (phase 3): pendingTransactionCount=%d\n", this, this->pendingTransactionCount.load(std::memory_order_relaxed)); return false; } - if (!closeAlreadyClaimed) { - bool expected = false; - this->isClosing.compare_exchange_strong(expected, true); - } + bool expected = false; + this->isClosing.compare_exchange_strong(expected, true); } - if (closeAlreadyClaimed) return finishClose(); // Phase 4 — perform the actual close. // Any writeBatch() that starts after phase 3 will see isClosing=true and @@ -280,16 +266,6 @@ uint64_t TransactionLogStore::getLogFileSize(uint32_t logSequenceNumber) { return size; } -uint32_t TransactionLogStore::getNextLogSequenceNumber(uint32_t logSequenceNumber) { - std::lock_guard lock(this->dataSetsMutex); - auto it = this->sequenceFiles.upper_bound(logSequenceNumber); - return it == this->sequenceFiles.end() ? 0 : it->first; -} - -uint64_t TransactionLogStore::getPurgeGeneration() const { - return this->purgeGeneration.load(std::memory_order_relaxed); -} - std::weak_ptr TransactionLogStore::getLastCommittedPosition() { // Initialize lastCommittedPosition if it's still at {0, 0} and invalid if (this->lastCommittedPosition->fullPosition == 0) { @@ -327,16 +303,16 @@ std::weak_ptr TransactionLogStore::getLastCommittedPosition() { LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { std::lock_guard lock(this->dataSetsMutex); - uint32_t currentSequence = this->currentSequenceNumber.load(std::memory_order_relaxed); + uint32_t sequenceNumber = this->currentSequenceNumber.load(std::memory_order_relaxed); + bool isCurrent = true; uint32_t positionInLogFile = 0; - auto it = this->sequenceFiles.upper_bound(currentSequence); - uint32_t nextHigherSequence = currentSequence; - uint32_t oldestSequence = currentSequence; - while (it != this->sequenceFiles.begin()) { - --it; - uint32_t sequenceNumber = it->first; - oldestSequence = sequenceNumber; - bool isCurrent = sequenceNumber == currentSequence; + auto it = this->sequenceFiles.find(sequenceNumber); + if (it == this->sequenceFiles.end()) { + // it is possible that the current log file doesn't exist yet, so we need to look at the previous one + it = this->sequenceFiles.find(--sequenceNumber); + isCurrent = false; + } + while (it != this->sequenceFiles.end()) { auto logFile = it->second.get(); // Directory iteration order is unspecified, so registerLogFile() may not // have opened an older file before a higher sequence became current. @@ -353,8 +329,9 @@ LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { if (positionInLogFile > 0) { if (positionInLogFile == 0xFFFFFFFF) { // beyond the end of this log file - if (sequenceNumber < currentSequence) { - return { TRANSACTION_LOG_FILE_HEADER_SIZE, nextHigherSequence }; + if (sequenceNumber < this->currentSequenceNumber.load(std::memory_order_relaxed)) { + // revert to next one (because it exists) + break; } else { // otherwise position at the end of the log file (JS code can filter from here) positionInLogFile = logFile->size; } @@ -362,9 +339,11 @@ LogPosition TransactionLogStore::findPositionByTimestamp(double timestamp) { // found a valid position in the log file return { positionInLogFile, sequenceNumber }; } - nextHigherSequence = sequenceNumber; + isCurrent = false; + it = this->sequenceFiles.find(--sequenceNumber); }; - return { TRANSACTION_LOG_FILE_HEADER_SIZE, oldestSequence }; + // we iterated too far, return to the beginning position in the current log file + return { TRANSACTION_LOG_FILE_HEADER_SIZE, sequenceNumber + 1 }; } LogPosition TransactionLogStore::getLastFlushedPosition() { @@ -419,8 +398,8 @@ std::vector TransactionLogStore::snapshotForBackup() TransactionLogBackupEntry stateEntry; bool hasStateEntry = false; { - // databaseFlushed() (RocksDB's OnFlushComplete callback) updates txn.state - // under flushedStateMutex, and a flush can fire mid-backup — an + // databaseFlushed() (RocksDB's OnFlushComplete callback) rewrites txn.state + // in place under flushedStateMutex, and a flush can fire mid-backup — an // unsynchronized read could tear, decoding a position that is neither the // old nor the new value and may point past the log extents captured below // (same discipline as getLastFlushedPosition()). The lock is scoped to this @@ -577,6 +556,9 @@ void TransactionLogStore::collectStats(TransactionLogStoreStats& out) { } const bool retentionEnabled = this->retentionMs.count() > 0; + const uint32_t durableFloorSequence = this->sequenceFiles.empty() + ? 0 + : this->sequenceFiles.rbegin()->first; for (const auto& [seq, logFile] : this->sequenceFiles) { // A registered-but-never-opened older file still reads 0 here (see @@ -636,9 +618,9 @@ void TransactionLogStore::collectStats(TransactionLogStoreStats& out) { // extent — would retain it. Gauge-only skew; nothing acts on it. bool fullyFlushed = !(seq > flushedPosition.logSequenceNumber || (seq == flushedPosition.logSequenceNumber && fileSize > flushedPosition.positionInLogFile)); - if (fullyFlushed) { + if (fullyFlushed && seq != durableFloorSequence) { out.purgeableFiles++; - } else { + } else if (!fullyFlushed) { out.retainedUnflushedFiles++; } } @@ -670,11 +652,14 @@ void TransactionLogStore::doPurge(std::function sequenceNumbersToRemove; - auto lastFlushedPosition = this->getLastFlushedPosition(); + const uint32_t durableFloorSequence = this->sequenceFiles.rbegin()->first; for (const auto& entry : this->sequenceFiles) { auto& sequenceNumber = entry.first; auto& logFile = entry.second; + if (!all && sequenceNumber == durableFloorSequence) { + break; + } bool shouldPurge = all; if (!shouldPurge && (before > 0 || this->retentionMs.count() > 0)) { @@ -693,27 +678,31 @@ void TransactionLogStore::doPurge(std::functionpath.string().c_str()); - continue; + break; } catch (const std::exception& e) { DEBUG_LOG("%p TransactionLogStore::purge Failed to get last write time for file %s: %s\n", this, logFile->path.string().c_str(), e.what()); - continue; + break; } catch (...) { auto eptr = std::current_exception(); std::string errorMsg = getExceptionMessage(eptr); DEBUG_LOG("%p TransactionLogStore::purge Unknown error getting last write time for file %s: %s\n", this, logFile->path.string().c_str(), errorMsg.c_str()); - continue; + break; } } if (!shouldPurge) { - continue; + break; } // only purge files that are entirely before the last flushed position, // guaranteeing all their transactions have been committed to RocksDB + auto lastFlushedPosition = this->getLastFlushedPosition(); if (sequenceNumber > lastFlushedPosition.logSequenceNumber) { - continue; + if (all) { + continue; + } + break; } if (sequenceNumber == lastFlushedPosition.logSequenceNumber) { // The only comparison here that reads `size`, and the only file whose @@ -726,13 +715,22 @@ void TransactionLogStore::doPurge(std::functionpath.string().c_str(), e.what()); - continue; + if (all) { + continue; + } + break; } catch (...) { DEBUG_LOG("%p TransactionLogStore::purge Failed to resolve extent for file %s\n", this, logFile->path.string().c_str()); - continue; + if (all) { + continue; + } + break; } if (logFile->size > lastFlushedPosition.positionInLogFile) { - continue; + if (all) { + continue; + } + break; } } @@ -744,7 +742,10 @@ void TransactionLogStore::doPurge(std::functionsize.load(std::memory_order_relaxed); auto removed = logFile->removeFile(); if (!removed) { - continue; + if (all) { + continue; + } + break; } this->filesPurged.fetch_add(1, std::memory_order_relaxed); this->bytesPurged.fetch_add(removedSize, std::memory_order_relaxed); @@ -777,8 +778,24 @@ void TransactionLogStore::doPurge(std::functionsequenceFiles.erase(sequenceNumber); } - if (!sequenceNumbersToRemove.empty()) { - this->purgeGeneration.store(nextTransactionLogStoreGeneration.fetch_add(1, std::memory_order_relaxed), std::memory_order_relaxed); + + // if all log files have been removed, clean up the empty directory + // only try to remove if we actually removed at least one file from this store + if (all && this->sequenceFiles.empty() && !sequenceNumbersToRemove.empty()) { + try { + if (std::filesystem::exists(this->path)) { + DEBUG_LOG("%p TransactionLogStore::purge Removing log store directory: %s\n", this, this->path.string().c_str()); + std::filesystem::remove_all(this->path); + DEBUG_LOG("%p TransactionLogStore::purge Removed log store directory: %s\n", this, this->path.string().c_str()); + } + } catch (const std::filesystem::filesystem_error& e) { + DEBUG_LOG("%p TransactionLogStore::purge Failed to remove log store directory %s: %s\n", this, this->path.string().c_str(), e.what()); + } catch (...) { + auto eptr = std::current_exception(); + std::string errorMsg = getExceptionMessage(eptr); + DEBUG_LOG("%p TransactionLogStore::purge Unknown error removing log store directory %s: %s\n", + this, this->path.string().c_str(), errorMsg.c_str()); + } } } @@ -1111,28 +1128,25 @@ void TransactionLogStore::databaseFlushed(rocksdb::SequenceNumber rocksSequenceN // flushedStateMutex (not dataSetsMutex) so that getLastFlushedPosition() // can safely read txn.state from doPurge() without risk of deadlock. std::lock_guard flushedLock(this->flushedStateMutex); - if (this->isClosing.load(std::memory_order_relaxed)) { - return; - } + // Only write if the position has changed if (latestSequencePosition.fullPosition == lastWrittenFlushedPosition.fullPosition) { return; } - auto flushedStateFilePath = this->path / "txn.state"; - std::fstream flushedStateFile(flushedStateFilePath, std::ios::binary | std::ios::in | std::ios::out); - if (!flushedStateFile.is_open()) { - // Recreate a missing state file, but never its parent store directory. - flushedStateFile.open(flushedStateFilePath, std::ios::binary | std::ios::out); + // open the state file if it isn't open yet + if (!this->flushedStateFile.is_open()) { + auto flushedStateFilePath = this->path / "txn.state"; + this->flushedStateFile.open(flushedStateFilePath, std::ios::binary | std::ios::out); } - if (flushedStateFile.is_open()) { - flushedStateFile.seekp(0); - flushedStateFile.write(reinterpret_cast(&latestSequencePosition), sizeof(latestSequencePosition)); - flushedStateFile.flush(); - if (flushedStateFile.good()) { - lastWrittenFlushedPosition = latestSequencePosition; - this->databaseFlushes.fetch_add(1, std::memory_order_relaxed); - } + + // write the position to the file + if (this->flushedStateFile.is_open()) { + this->flushedStateFile.seekp(0); + this->flushedStateFile.write(reinterpret_cast(&latestSequencePosition), sizeof(latestSequencePosition)); + this->flushedStateFile.flush(); + lastWrittenFlushedPosition = latestSequencePosition; + this->databaseFlushes.fetch_add(1, std::memory_order_relaxed); } } @@ -1189,24 +1203,6 @@ std::shared_ptr TransactionLogStore::load( } LogPosition flushedPosition = store->getLastFlushedPosition(); - uint32_t discoveredCurrentSequence = store->currentSequenceNumber.load(std::memory_order_relaxed); - bool hasCurrentSegment = store->sequenceFiles.find(discoveredCurrentSequence) != store->sequenceFiles.end(); - bool resumedPastWatermark = false; - if (flushedPosition.logSequenceNumber > discoveredCurrentSequence || - (!hasCurrentSegment && flushedPosition.logSequenceNumber == discoveredCurrentSequence && - flushedPosition.logSequenceNumber > 0)) { - if (flushedPosition.logSequenceNumber == std::numeric_limits::max()) { - throw rocksdb_js::DBException("Transaction log flush watermark sequence is out of range for " + path.string()); - } - uint32_t nextWritableSequence = flushedPosition.logSequenceNumber + 1; - if (nextWritableSequence == std::numeric_limits::max()) { - throw rocksdb_js::DBException("Transaction log sequence space is exhausted for " + path.string()); - } - store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); - store->nextSequenceNumber = nextWritableSequence + 1; - store->nextLogPosition = { 0, nextWritableSequence }; - resumedPastWatermark = true; - } // Only the active file can carry a torn append; recover it after discovery and // refresh the write position if recovery shortened it. @@ -1233,9 +1229,7 @@ std::shared_ptr TransactionLogStore::load( // Legacy batches can span any number of rotations, so walk back through their // unflagged files until a boundary is found. Once the flushed file has been // scanned, older files cannot improve the floor and need not be read. - LogPosition recoveredPosition = (store->sequenceFiles.empty() || resumedPastWatermark) - ? store->nextLogPosition - : LogPosition { 0, 0 }; + LogPosition recoveredPosition = { 0, 0 }; for (auto it = store->sequenceFiles.rbegin(); it != store->sequenceFiles.rend(); ++it) { if (it->first > storeCurrentSeq) { continue; diff --git a/src/binding/transaction_log/transaction_log_store.h b/src/binding/transaction_log/transaction_log_store.h index 934a1a720..0133537fe 100644 --- a/src/binding/transaction_log/transaction_log_store.h +++ b/src/binding/transaction_log/transaction_log_store.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -321,8 +322,8 @@ struct TransactionLogStore final { unsigned int nextSequencePositionsCount = 0; /** - * Protects lastWrittenFlushedPosition and all I/O on "txn.state". - * This is a separate, lightweight lock so that + * Protects flushedStateFile, lastWrittenFlushedPosition, and all I/O on + * "txn.state". This is a separate, lightweight lock so that * getLastFlushedPosition() — which is called from doPurge() while * dataSetsMutex is already held — never needs to acquire dataSetsMutex, * eliminating that deadlock path. @@ -332,6 +333,11 @@ struct TransactionLogStore final { */ std::mutex flushedStateMutex; + /** + * This file stream is used to track how much of the transaction log has been flushed to the database. + */ + std::ofstream flushedStateFile; + /** * The last flushed position that was written to the state file. */ @@ -362,7 +368,6 @@ struct TransactionLogStore final { std::atomic filesPurged = 0; std::atomic bytesPurged = 0; std::atomic purgeRuns = 0; - std::atomic purgeGeneration = 0; std::atomic databaseFlushes = 0; std::atomic writeFailures = 0; std::atomic lastPurgeMs = 0; @@ -443,13 +448,6 @@ struct TransactionLogStore final { **/ uint64_t getLogFileSize(uint32_t logSequenceNumber); - /** - * Get the first registered log sequence strictly after the supplied sequence, - * or zero when no later segment exists. - */ - uint32_t getNextLogSequenceNumber(uint32_t logSequenceNumber); - uint64_t getPurgeGeneration() const; - /** * Get the shared represention object representing the last committed position. **/ diff --git a/src/binding/transaction_log/transaction_log_store_registry.cpp b/src/binding/transaction_log/transaction_log_store_registry.cpp index 7cab3acb2..01518ea30 100644 --- a/src/binding/transaction_log/transaction_log_store_registry.cpp +++ b/src/binding/transaction_log/transaction_log_store_registry.cpp @@ -4,7 +4,6 @@ #include "core/platform.h" #include "napi/helpers.h" #include "napi/async.h" -#include #include #include @@ -13,10 +12,6 @@ namespace rocksdb_js { // Initialize the static instance std::unique_ptr TransactionLogStoreRegistry::instance; -namespace { -std::atomic nextDeletionId { 0 }; -} - /** * Initializes the singleton instance. */ @@ -152,26 +147,12 @@ void TransactionLogStoreRegistry::DiscoverStores(const std::string& dbPath) { config = &entry->config; } - if (transactionLogsPath.empty()) { - return; - } - - if (!std::filesystem::exists(transactionLogsPath)) { + if (transactionLogsPath.empty() || !std::filesystem::exists(transactionLogsPath)) { DEBUG_LOG("%p TransactionLogStoreRegistry::DiscoverStores No transaction logs path or directory does not exist for \"%s\"\n", instance.get(), dbPath.c_str()); return; } - // A process can exit after atomically detaching a destroyed store but before - // its files are removed. Keep the root itself so concurrent detaches remain valid. - auto deletionRoot = std::filesystem::path(transactionLogsPath); - deletionRoot += ".deleting"; - std::error_code cleanupError; - for (std::filesystem::directory_iterator it(deletionRoot, cleanupError), end; !cleanupError && it != end; it.increment(cleanupError)) { - std::error_code removeError; - std::filesystem::remove_all(it->path(), removeError); - } - std::lock_guard storeLock(entry->storesMutex); for (const auto& dirEntry : std::filesystem::directory_iterator(transactionLogsPath)) { @@ -341,19 +322,6 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str entry = it->second; } - std::filesystem::path deletionRoot; - if (destroy) { - deletionRoot = std::filesystem::path(entry->config.transactionLogsPath); - deletionRoot += ".deleting"; - rocksdb_js::tryCreateDirectory(deletionRoot); - std::error_code rootError; - if (!std::filesystem::is_directory(deletionRoot, rootError)) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Cannot create deletion directory %s: %s\n", - instance.get(), deletionRoot.string().c_str(), rootError.message().c_str()); - return removed; - } - } - size_t i = 0; std::vector> storesToPurge; @@ -392,50 +360,32 @@ napi_value TransactionLogStoreRegistry::PurgeStores(napi_env env, const std::str } } - // Phase 3: Atomically detach closed store directories while holding the registry - // lock, then remove them without blocking flush callbacks or store resolution. + // Phase 3: Remove closed stores from the registry while holding the lock + std::vector> storesActuallyRemoved; if (destroy) { - std::vector pathsToRemove; - { - std::lock_guard storeLock(entry->storesMutex); - for (auto& store : storesToPurge) { - if (!store->isClosing.load(std::memory_order_relaxed)) { - continue; - } - auto storeIt = entry->stores.find(store->name); - if (storeIt == entry->stores.end() || storeIt->second.get() != store.get()) { - continue; - } - - auto deletionPath = deletionRoot / - (store->name + "-" + std::to_string(nextDeletionId.fetch_add(1, std::memory_order_relaxed))); - std::error_code renameError; - std::filesystem::rename(store->path, deletionPath, renameError); - if (renameError == std::errc::no_such_file_or_directory) { - std::error_code sourceError; - if (std::filesystem::exists(store->path, sourceError) || sourceError) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to detach log directory %s: %s\n", - instance.get(), store->path.string().c_str(), renameError.message().c_str()); - continue; - } - } else if (renameError) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to detach log directory %s: %s\n", - instance.get(), store->path.string().c_str(), renameError.message().c_str()); - continue; - } + std::lock_guard storeLock(entry->storesMutex); + for (auto& store : storesToPurge) { + if (!store->isClosing.load(std::memory_order_relaxed)) { + continue; + } + auto storeIt = entry->stores.find(store->name); + if (storeIt != entry->stores.end() && storeIt->second.get() == store.get()) { entry->stores.erase(storeIt); - if (!renameError) { - pathsToRemove.push_back(std::move(deletionPath)); - } + storesActuallyRemoved.push_back(store); } } - for (const auto& deletionPath : pathsToRemove) { - std::error_code removeError; - std::filesystem::remove_all(deletionPath, removeError); - if (removeError) { - DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove detached log directory %s: %s\n", - instance.get(), deletionPath.string().c_str(), removeError.message().c_str()); - } + } + + // Phase 4: Delete directories outside the lock + for (auto& store : storesActuallyRemoved) { + try { + std::filesystem::remove_all(store->path); + } catch (const std::filesystem::filesystem_error& e) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Failed to remove log directory %s: %s\n", + instance.get(), store->path.string().c_str(), e.what()); + } catch (...) { + DEBUG_LOG("%p TransactionLogStoreRegistry::PurgeStores Unknown error removing log directory %s\n", + instance.get(), store->path.string().c_str()); } } diff --git a/src/load-binding.ts b/src/load-binding.ts index 5016b3565..04ff17a3e 100644 --- a/src/load-binding.ts +++ b/src/load-binding.ts @@ -217,19 +217,16 @@ export type TransactionLog = { new (db: NativeDatabase, name: string): TransactionLog; addEntry(data: Buffer | Uint8Array, txnId?: number): void; getLogFileSize(sequenceId?: number): number; - _getNextLogSequenceNumber(sequenceId: number): number; - _getPurgeGeneration(): number; - _purgeGeneration?: number; getStats(): TransactionLogStats; name: string; path: string; query(options?: TransactionLogQueryOptions): IterableIterator; - _currentLogBuffer?: LogBuffer; + _currentLogBuffer: LogBuffer; _findPosition(timestamp: number): number; - _getLastCommittedPosition(): Buffer | undefined; + _getLastCommittedPosition(): Buffer; _getLastFlushed(): number; _getMemoryMapOfFile(sequenceId: number): LogBuffer | undefined; - _lastCommittedPosition?: Float64Array; + _lastCommittedPosition: Float64Array; _logBuffers: Map>; }; diff --git a/src/transaction-log-reader.ts b/src/transaction-log-reader.ts index 4b57aa17a..ae1d52354 100644 --- a/src/transaction-log-reader.ts +++ b/src/transaction-log-reader.ts @@ -167,23 +167,9 @@ Object.defineProperty(TransactionLog.prototype, 'query', { exclusiveStart, }: TransactionLogQueryOptions = {} ): IterableIterator { - const purgeGeneration = this._getPurgeGeneration(); - if (this._purgeGeneration !== undefined && purgeGeneration !== this._purgeGeneration) { - const lastCommittedPosition = this._getLastCommittedPosition(); - if (!lastCommittedPosition) { - return [][Symbol.iterator]() as IterableIterator; - } - this._logBuffers = new Map>(); - this._currentLogBuffer = undefined; - this._lastCommittedPosition = new Float64Array(lastCommittedPosition.buffer); - } - this._purgeGeneration = purgeGeneration; if (!this._lastCommittedPosition) { // if this is the first time we are querying the log, initialize the last committed position and memory map cache const lastCommittedPosition = this._getLastCommittedPosition(); - if (!lastCommittedPosition) { - return [][Symbol.iterator]() as IterableIterator; - } this._lastCommittedPosition = new Float64Array(lastCommittedPosition.buffer); this._logBuffers = new Map>(); } @@ -242,7 +228,7 @@ Object.defineProperty(TransactionLog.prototype, 'query', { if (logBuffer === undefined) { // create a fake log buffer if we don't have any log buffer yet logBuffer = Buffer.alloc(0) as unknown as LogBuffer; - logBuffer.logId = logId; + logBuffer.logId = 0; logBuffer.size = 0; logBuffer.dataView = new DataView(logBuffer.buffer); // the outer size variable was set from loadLastPosition() above, but if we @@ -279,43 +265,27 @@ Object.defineProperty(TransactionLog.prototype, 'query', { !!readUncommitted ); size = latestSize; - if (logBuffer!.length === 0) { - let searchAfter = Math.max(0, logBuffer!.logId - 1); - do { - const nextLogBuffer = getNextLogMemoryMap(transactionLog, searchAfter, latestLogId); - if (!nextLogBuffer) return { done: true, value: undefined }; - logBuffer = nextLogBuffer; - searchAfter = logBuffer.logId; - dataView = logBuffer.dataView; - position = TRANSACTION_LOG_FILE_HEADER_SIZE; - size = - latestLogId === logBuffer.logId - ? latestSize - : (logBuffer.size ??= transactionLog.getLogFileSize(logBuffer.logId)); - } while (position >= size); - } else if (latestLogId > logBuffer!.logId) { + if (latestLogId > logBuffer!.logId) { // if it is not the latest log, get the file size size = logBuffer!.size ?? (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); - while (position >= size && latestLogId > logBuffer!.logId) { - const nextLogBuffer = getNextLogMemoryMap( - transactionLog, - logBuffer!.logId, - latestLogId - ); - if (!nextLogBuffer) break; - dataView = nextLogBuffer.dataView; - logBuffer = nextLogBuffer; - if (latestLogId > logBuffer!.logId) { - // it is non-current log file, we can safely use or cache the size - size = - logBuffer!.size ?? - (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); - } else { - size = latestSize; // use the latest position from loadLastPosition + if (position >= size) { + // we can't read any further in this block, go to the next block + const nextLogBuffer = getLogMemoryMap(transactionLog, logBuffer!.logId + 1)!; + if (nextLogBuffer) { + dataView = nextLogBuffer.dataView; + logBuffer = nextLogBuffer; + if (latestLogId > logBuffer!.logId) { + // it is non-current log file, we can safely use or cache the size + size = + logBuffer!.size ?? + (logBuffer!.size = transactionLog.getLogFileSize(logBuffer!.logId)); + } else { + size = latestSize; // use the latest position from loadLastPosition + } + position = TRANSACTION_LOG_FILE_HEADER_SIZE; } - position = TRANSACTION_LOG_FILE_HEADER_SIZE; } } } @@ -417,35 +387,32 @@ Object.defineProperty(TransactionLog.prototype, 'query', { }, }; } - while (position >= size) { + if (position >= size) { // move to the next log file const { logId: latestLogId, size: latestSize } = loadLastPosition( transactionLog, !!readUncommitted ); size = latestSize; - if (latestLogId <= logBuffer!.logId) break; - const nextLogBuffer = getNextLogMemoryMap( - transactionLog, - logBuffer!.logId, - latestLogId - ); - if (!nextLogBuffer) { - // the next log file can't be mapped (purged, mid-rotation, - // 0-byte at mmap time, FS race); stop cleanly rather than - // dereferencing an undefined buffer - return { done: true, value: undefined }; - } - logBuffer = nextLogBuffer; - dataView = logBuffer.dataView; - size = logBuffer.size; - if (size == undefined) { - size = transactionLog.getLogFileSize(logBuffer.logId); - if (!readUncommitted) { - logBuffer.size = size; + if (latestLogId > logBuffer!.logId) { + const nextLogBuffer = getLogMemoryMap(transactionLog, logBuffer!.logId + 1); + if (!nextLogBuffer) { + // the next log file can't be mapped (purged, mid-rotation, + // 0-byte at mmap time, FS race); stop cleanly rather than + // dereferencing an undefined buffer + return { done: true, value: undefined }; + } + logBuffer = nextLogBuffer; + dataView = logBuffer.dataView; + size = logBuffer.size; + if (size == undefined) { + size = transactionLog.getLogFileSize(logBuffer.logId); + if (!readUncommitted) { + logBuffer.size = size; + } } + position = TRANSACTION_LOG_FILE_HEADER_SIZE; } - position = TRANSACTION_LOG_FILE_HEADER_SIZE; } } return { done: true, value: undefined }; @@ -487,32 +454,6 @@ function getLogMemoryMap(transactionLog: TransactionLog, logId: number): LogBuff return logBuffer; } -function getNextLogMemoryMap( - transactionLog: TransactionLog, - logId: number, - latestLogId: number -): LogBuffer | undefined { - while (logId < latestLogId) { - let nextLogId = transactionLog._getNextLogSequenceNumber(logId); - if (nextLogId === 0) { - for (const [cachedLogId, reference] of transactionLog._logBuffers!) { - if ( - cachedLogId > logId && - cachedLogId <= latestLogId && - reference.deref() && - (nextLogId === 0 || cachedLogId < nextLogId) - ) { - nextLogId = cachedLogId; - } - } - } - if (nextLogId === 0 || nextLogId > latestLogId) return; - const logBuffer = getLogMemoryMap(transactionLog, nextLogId); - if (logBuffer) return logBuffer; - logId = nextLogId; - } -} - function loadLastPosition( transactionLog: TransactionLog, readUncommitted: boolean diff --git a/test/transaction-log-stats.test.ts b/test/transaction-log-stats.test.ts index ecc4e79e0..967654968 100644 --- a/test/transaction-log-stats.test.ts +++ b/test/transaction-log-stats.test.ts @@ -114,6 +114,24 @@ describe('Transaction Log Stats', () => { } )); + it('should not report the flushed current file as purgeable', () => + dbRunner( + { dbOptions: [{ path: generateDBPath(), transactionLogRetention: 500 }] }, + async ({ db }) => { + const log = db.useLog('retain-current'); + await db.transaction(async (txn) => { + log.addEntry(Buffer.alloc(100, 'a'), txn.id); + db.putSync('key', 'value', { transaction: txn }); + }); + db.flushSync(); + await delay(1200); + + const stats = log.getStats(); + expect(stats.purge.purgeableFiles).toBe(0); + expect(stats.purge.retainedUnflushedFiles).toBe(0); + } + )); + it('should increment purgeRuns when logs are purged', () => dbRunner(async ({ db }) => { const log = db.useLog('purge'); diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index 465f4343b..d3721bfcc 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -11,8 +11,8 @@ import { writeLazyTransactionLogSegments } from './lib/transaction-log-fixtures. import { dbRunner, generateDBPath, terminateWorker } from './lib/util.ts'; import { createWorkerBootstrapScript } from './lib/worker-bootstrap.ts'; import assert from 'node:assert'; -import { existsSync, readFileSync, statSync, unlinkSync } from 'node:fs'; -import { mkdir, readdir, rm, stat, utimes, writeFile } from 'node:fs/promises'; +import { existsSync, readFileSync, statSync } from 'node:fs'; +import { mkdir, readdir, stat, utimes, writeFile } from 'node:fs/promises'; import { release } from 'node:os'; import { join } from 'node:path'; import { setTimeout as delay } from 'node:timers/promises'; @@ -265,7 +265,7 @@ describe('Transaction Log', () => { await db.transaction(async (txn) => { log.addEntry(value, txn.id); }); - const positionBuffer = log._getLastCommittedPosition()!; + const positionBuffer = log._getLastCommittedPosition(); const dataView = new DataView(positionBuffer.buffer); expect(dataView.getUint32(0)).toBeGreaterThan(10); const sequenceNumber = dataView.getUint32(1); @@ -713,49 +713,6 @@ describe('Transaction Log', () => { } })); - it('continues through an empty segment to later committed entries', () => - dbRunner({ dbOptions: [{ transactionLogMaxSize: 1000 }] }, async ({ db, dbPath }) => { - let database = db; - try { - let log = database.useLog('foo'); - for (let i = 0; i < 20; i++) { - await database.transaction(async (txn) => { - log.addEntry(Buffer.from(String(i).padStart(100, '0')), txn.id); - }); - } - database.close(); - - const middlePath = join(dbPath, 'transaction_logs', 'foo', '2.txnlog'); - await writeFile( - middlePath, - readFileSync(middlePath).subarray(0, TRANSACTION_LOG_FILE_HEADER_SIZE) - ); - - database = RocksDatabase.open(dbPath, { transactionLogMaxSize: 1000 }); - log = database.useLog('foo'); - const values = Array.from(log.query({ start: 0 }), (entry) => entry.data.toString()); - expect(values).toEqual([ - ...Array.from({ length: 8 }, (_, i) => String(i).padStart(100, '0')), - ...Array.from({ length: 4 }, (_, i) => String(i + 16).padStart(100, '0')), - ]); - - database.close(); - unlinkSync(join(dbPath, 'transaction_logs', 'foo', '1.txnlog')); - const state = Buffer.alloc(8); - state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(1, 4); - await writeFile(join(dbPath, 'transaction_logs', 'foo', 'txn.state'), state); - - database = RocksDatabase.open(dbPath, { transactionLogMaxSize: 1000 }); - log = database.useLog('foo'); - expect( - Array.from(log.query({ startFromLastFlushed: true }), (entry) => entry.data.toString()) - ).toEqual(Array.from({ length: 4 }, (_, i) => String(i + 16).padStart(100, '0'))); - } finally { - database.close(); - } - })); - it('should allow unlimited transaction log size', () => dbRunner({ dbOptions: [{ transactionLogMaxSize: 0 }] }, async ({ db, dbPath }) => { const log = db.useLog('foo'); @@ -2137,257 +2094,6 @@ describe('Transaction Log', () => { return Buffer.concat(parts); }; - it('preserves the live flush watermark across repeated complete purges', () => - dbRunner(async ({ db, dbPath }) => { - const log = db.useLog('foo'); - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - const stateFile = join(logDirectory, 'txn.state'); - - for (let cycle = 1; cycle <= 2; cycle++) { - await db.transaction(async (txn) => { - const value = Buffer.from(`value-${cycle}`); - log.addEntry(value, txn.id); - db.putSync(`key-${cycle}`, value, { transaction: txn }); - }); - db.flushSync(); - - const state = readFileSync(stateFile); - const flushed = new Uint32Array(state.buffer, state.byteOffset, 2); - expect(flushed[1]).toBe(cycle); - expect(flushed[0]).toBeGreaterThan(TRANSACTION_LOG_FILE_HEADER_SIZE); - - const logFile = join(logDirectory, `${cycle}.txnlog`); - expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([logFile]); - expect(existsSync(stateFile)).toBe(true); - if (cycle === 1) unlinkSync(stateFile); - } - })); - - it('does not recreate a missing state file for an unchanged flush position', () => - dbRunner(async ({ db, dbPath }) => { - const log = db.useLog('foo'); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('value'), txn.id); - db.putSync('key', 'value', { transaction: txn }); - }); - db.flushSync(); - - const stateFile = join(dbPath, 'transaction_logs', 'foo', 'txn.state'); - unlinkSync(stateFile); - db.flushSync(); - expect(existsSync(stateFile)).toBe(false); - })); - - it('does not reuse a mapped log file after a live purge', () => - dbRunner(async ({ db }) => { - const log = db.useLog('foo'); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('purged'), txn.id); - db.putSync('key', 'value', { transaction: txn }); - }); - db.flushSync(); - const existingIterator = log.query({ start: 0 }); - expect(existingIterator.next().value.data.toString()).toBe('purged'); - - expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toHaveLength(1); - const lastPositionDescriptor = Object.getOwnPropertyDescriptor( - Object.getPrototypeOf(log), - '_getLastCommittedPosition' - )!; - Object.defineProperty(log, '_getLastCommittedPosition', { - value: () => undefined, - configurable: true, - }); - try { - expect(Array.from(log.query({ start: 0 }))).toEqual([]); - expect(existingIterator.next()).toEqual({ done: true, value: undefined }); - } finally { - Object.defineProperty(log, '_getLastCommittedPosition', lastPositionDescriptor); - } - expect(Array.from(log.query({ start: 0 }))).toEqual([]); - })); - - it('does not reuse a mapped log file after destroying and recreating its store', () => - dbRunner(async ({ db, dbPath }) => { - const log = db.useLog('foo'); - let readerDatabase; - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('old'), txn.id); - db.putSync('old', 'value', { transaction: txn }); - }); - db.flushSync(); - try { - readerDatabase = RocksDatabase.open(dbPath); - const readerLog = readerDatabase.useLog('foo') as TransactionLog; - expect( - Array.from(readerLog.query({ start: 0 }), (entry) => entry.data.toString()) - ).toEqual(['old']); - expect(db.purgeLogs({ destroy: true, name: 'foo' })).toHaveLength(1); - - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('replacement'), txn.id); - db.putSync('replacement', 'value', { transaction: txn }); - }); - expect( - Array.from(readerLog.query({ start: 0 }), (entry) => entry.data.toString()) - ).toEqual(['replacement']); - } finally { - readerDatabase?.close(); - } - })); - - it('continues after a retained watermark when reopening an empty store', () => - dbRunner(async ({ db, dbPath }) => { - let database = db; - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - try { - let log = database.useLog('foo'); - await database.transaction(async (txn) => { - log.addEntry(Buffer.from('first'), txn.id); - database.putSync('first', 'value', { transaction: txn }); - }); - database.flushSync(); - expect(database.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([ - join(logDirectory, '1.txnlog'), - ]); - database.close(); - - database = RocksDatabase.open(dbPath); - log = database.useLog('foo'); - const lastCommittedBuffer = log._getLastCommittedPosition()!; - const lastCommitted = new Uint32Array( - lastCommittedBuffer.buffer, - lastCommittedBuffer.byteOffset, - 2 - ); - expect(Array.from(lastCommitted)).toEqual([0, 2]); - await database.transaction(async (txn) => { - log.addEntry(Buffer.from('second'), txn.id); - database.putSync('second', 'value', { transaction: txn }); - }); - - const logFiles = (await readdir(logDirectory)).filter((name) => name.endsWith('.txnlog')); - expect(logFiles).toEqual(['2.txnlog']); - expect(database.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([]); - expect(existsSync(join(logDirectory, '2.txnlog'))).toBe(true); - } finally { - database.close(); - } - })); - - it('continues past the watermark when only lower sequence files survive', () => - dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - db.open(); - let log = db.useLog('foo'); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('first'), txn.id); - }); - const firstTimestamp = Array.from(log.query({ start: 0 }))[0].timestamp; - db.close(); - - const state = Buffer.alloc(8); - state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(2, 4); - await writeFile(join(logDirectory, 'txn.state'), state); - - db.open(); - log = db.useLog('foo'); - const lastCommittedBuffer = log._getLastCommittedPosition()!; - const lastCommitted = new Uint32Array( - lastCommittedBuffer.buffer, - lastCommittedBuffer.byteOffset, - 2 - ); - expect(Array.from(lastCommitted)).toEqual([0, 3]); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('third'), txn.id); - }); - expect(existsSync(join(logDirectory, '3.txnlog'))).toBe(true); - expect(Array.from(log.query({ start: 0 }), (entry) => entry.data.toString())).toEqual([ - 'first', - 'third', - ]); - expect( - Array.from(log.query({ start: firstTimestamp + 0.5 }), (entry) => entry.data.toString()) - ).toEqual(['third']); - expect( - Array.from(log.query({ startFromLastFlushed: true }), (entry) => entry.data.toString()) - ).toEqual(['third']); - })); - - it('does not enumerate absent sequence numbers after a retained watermark', () => - dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - await mkdir(logDirectory, { recursive: true }); - const state = Buffer.alloc(8); - state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(10_000, 4); - await writeFile(join(logDirectory, 'txn.state'), state); - - db.open(); - const log = db.useLog('foo'); - const realGetMemoryMap = log._getMemoryMapOfFile.bind(log); - let probes = 0; - Object.defineProperty(log, '_getMemoryMapOfFile', { - value(sequenceNumber) { - probes++; - return realGetMemoryMap(sequenceNumber); - }, - configurable: true, - }); - try { - expect(Array.from(log.query({ startFromLastFlushed: true }))).toEqual([]); - expect(probes).toBe(1); - } finally { - delete (log as { _getMemoryMapOfFile?: unknown })._getMemoryMapOfFile; - } - })); - - it('rejects a flush watermark that cannot advance to a writable sequence', () => - dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - await mkdir(logDirectory, { recursive: true }); - const state = Buffer.alloc(8); - state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(0xffffffff, 4); - await writeFile(join(logDirectory, 'txn.state'), state); - - expect(() => db.open()).toThrow('Transaction log flush watermark sequence is out of range'); - })); - - it('leaves a destroyed store absent after a concurrent flush request', () => - dbRunner(async ({ db, dbPath }) => { - const log = db.useLog('foo'); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('value'), txn.id); - db.putSync('key', 'value', { transaction: txn }); - }); - - const flush = db.flush(); - db.purgeLogs({ destroy: true, name: 'foo' }); - await flush; - await delay(10); - expect(existsSync(join(dbPath, 'transaction_logs', 'foo'))).toBe(false); - })); - - it('keeps a store registered when its deletion staging directory is unavailable', () => - dbRunner(async ({ db, dbPath }) => { - const log = db.useLog('foo'); - await db.transaction(async (txn) => { - log.addEntry(Buffer.from('value'), txn.id); - db.putSync('key', 'value', { transaction: txn }); - }); - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - const deletionRoot = join(dbPath, 'transaction_logs.deleting'); - await rm(deletionRoot, { recursive: true, force: true }); - await writeFile(deletionRoot, 'not a directory'); - - expect(db.purgeLogs({ destroy: true, name: 'foo' })).toEqual([]); - expect(db.listLogs()).toContain('foo'); - expect(existsSync(logDirectory)).toBe(true); - })); - it('should return entry counts when includeEntryCounts is true', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); @@ -2485,7 +2191,7 @@ describe('Transaction Log', () => { expect(existsSync(barLogDirectory)).toBe(true); })); - it('should purge old log file on load', () => + it('should retain the current old log file on load', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); const logFile = join(logDirectory, '1.txnlog'); @@ -2499,32 +2205,9 @@ describe('Transaction Log', () => { const oneWeekAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000); await utimes(logFile, oneWeekAgo, oneWeekAgo); - const state = Buffer.alloc(8); - state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(1, 4); - await writeFile(join(logDirectory, 'txn.state'), state); db.open(); expect(db.listLogs()).toEqual(['foo']); - expect(existsSync(logFile)).toBe(false); - })); - - it('should retain an aged, unflushed log file on load', () => - dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { - const logDirectory = join(dbPath, 'transaction_logs', 'foo'); - const logFile = join(logDirectory, '1.txnlog'); - await mkdir(logDirectory, { recursive: true }); - - const header = Buffer.alloc(TRANSACTION_LOG_FILE_HEADER_SIZE); - header.writeUInt32BE(TRANSACTION_LOG_TOKEN, 0); - header.writeUInt8(1, 4); - header.writeDoubleBE(0, 5); - await writeFile(logFile, header); - - const oneWeekAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000); - await utimes(logFile, oneWeekAgo, oneWeekAgo); - - db.open(); expect(existsSync(logFile)).toBe(true); })); @@ -2596,7 +2279,7 @@ describe('Transaction Log', () => { expect(existsSync(logFiles[0])).toBe(false); })); - it('should purge log files before a specific timestamp', () => + it('should retain the current log file past a specific timestamp', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); const logFile = join(logDirectory, '1.txnlog'); @@ -2620,8 +2303,65 @@ describe('Transaction Log', () => { expect(existsSync(logFile)).toBe(true); expect(db.purgeLogs({ before: threeHoursAgo.getTime() })).toEqual([]); expect(existsSync(logFile)).toBe(true); - expect(db.purgeLogs({ before: oneHourAgo.getTime() })).toEqual([logFile]); - expect(existsSync(logFile)).toBe(false); + expect(db.purgeLogs({ before: oneHourAgo.getTime() })).toEqual([]); + expect(existsSync(logFile)).toBe(true); + expect(existsSync(join(logDirectory, 'txn.state'))).toBe(true); + })); + + it('should purge only the eligible contiguous prefix before the current file', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + + const old = new Date(Date.now() - 2 * 60 * 60 * 1000); + const logFiles: string[] = []; + for (const sequence of [1, 2, 3]) { + const logFile = join(logDirectory, `${sequence}.txnlog`); + await writeFile(logFile, buildLogFile(1)); + await utimes(logFile, old, old); + logFiles.push(logFile); + } + + const state = Buffer.alloc(8); + state.writeUInt32LE(statSync(logFiles[2]).size, 0); + state.writeUInt32LE(3, 4); + const stateFile = join(logDirectory, 'txn.state'); + await writeFile(stateFile, state); + + db.open(); + expect(db.purgeLogs({ name: 'foo', before: Date.now() - 60 * 60 * 1000 })).toEqual( + logFiles.slice(0, 2) + ); + expect(existsSync(logFiles[0])).toBe(false); + expect(existsSync(logFiles[1])).toBe(false); + expect(existsSync(logFiles[2])).toBe(true); + expect(existsSync(stateFile)).toBe(true); + })); + + it('should keep repeated flush and retention purges bounded to the current file', () => + dbRunner({ dbOptions: [{ transactionLogMaxSize: 500 }] }, async ({ db, dbPath }) => { + const log = db.useLog('foo'); + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + const stateFile = join(logDirectory, 'txn.state'); + + for (let cycle = 1; cycle <= 6; cycle++) { + await db.transaction(async (txn) => { + const value = Buffer.alloc(300, cycle); + log.addEntry(value, txn.id); + db.putSync(`key-${cycle}`, value, { transaction: txn }); + }); + db.flushSync(); + db.purgeLogs({ name: 'foo', before: Date.now() + 1000 }); + + const logFiles = (await readdir(logDirectory)) + .filter((name) => name.endsWith('.txnlog')) + .sort((a, b) => Number.parseInt(a) - Number.parseInt(b)); + expect(logFiles).toEqual([`${cycle}.txnlog`]); + const state = readFileSync(stateFile); + expect(state.readUInt32LE(4)).toBe(cycle); + } + + expect(Array.from(log.query({ start: 0 }))).toHaveLength(1); })); it('should return valid lastCommittedPosition after purging earlier log files and reopening', () => @@ -2641,7 +2381,7 @@ describe('Transaction Log', () => { const log = db.useLog('foo'); // Get initial lastCommittedPosition - should be valid even though no commits yet - let lastCommittedBuffer = log._getLastCommittedPosition()!; + let lastCommittedBuffer = log._getLastCommittedPosition(); let lastCommittedPosUint32 = new Uint32Array(lastCommittedBuffer.buffer, 0, 2); expect(lastCommittedPosUint32[1]).toBeGreaterThanOrEqual(1); expect(lastCommittedPosUint32[0]).toBeGreaterThanOrEqual(10); @@ -2673,7 +2413,7 @@ describe('Transaction Log', () => { const log2 = db.useLog('foo'); // After reopening, lastCommittedPosition should be valid and point to log file 2 - lastCommittedBuffer = log2._getLastCommittedPosition()!; + lastCommittedBuffer = log2._getLastCommittedPosition(); lastCommittedPosUint32 = new Uint32Array(lastCommittedBuffer.buffer, 0, 2); expect(lastCommittedPosUint32[1]).toBeGreaterThanOrEqual(2); expect(lastCommittedPosUint32[0]).toBeGreaterThanOrEqual(10); From 3e9e83e59bb8b8d05b1628951cdbb2fd327284ee Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 06:44:47 -0600 Subject: [PATCH 10/13] fix(transaction-log): retain flushed sequence floor --- AGENTS.md | 5 -- README.md | 15 ++-- docs/stats.md | 2 +- docs/transaction-log.md | 6 +- .../transaction_log/transaction_log_store.cpp | 25 ++++-- test/transaction-log.test.ts | 83 ++++++++++++++++++- 6 files changed, 109 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 25637dac7..5d76ca0cc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -629,11 +629,6 @@ sufficient (env teardown does not honor tsfn acquire counts); see never reproduces natively or on glibc, so the repro test is `skipIf(darwin)` (and, like the repo's other teardown repros, gated to Node). -18. **Ordinary transaction-log retention removes only a contiguous oldest prefix**: it never - removes the highest sequence file or the live store directory. Stop the scan when an older - file is ineligible or cannot be removed; continuing could create a sequence gap. Destructive - store removal is the separate `destroy` path. - ## Debugging native heap corruption AddressSanitizer is the first choice (`ROCKSDB_ASAN=1 node-gyp rebuild` toggles `-fsanitize=address` diff --git a/README.md b/README.md index 12ba46455..8b87f99ad 100644 --- a/README.md +++ b/README.md @@ -1620,11 +1620,12 @@ const names = db.listLogs(); ### `db.purgeLogs({ includeEntryCounts: true, ...options }): { path: string; entries: number }[]` Deletes transaction log files older than the `transactionLogRetention` (defaults to 3 days). -Ordinary retention keeps the highest sequence file as the live store's durable floor and removes -only an eligible contiguous prefix of older files. An idle store can therefore retain one file -past the cutoff until a later write rotates it; disk retained past the cutoff is bounded by that -store's `transactionLogMaxSize` (except when a single transaction exceeds the target). Use -`destroy: true` only to remove the store itself. +Ordinary retention keeps the sequence file named by `txn.state` and every newer file as the live +store's durable floor, or the highest sequence file when there is no persisted flush position. It +removes only an eligible contiguous prefix below that floor. An idle store can therefore retain one +file past the cutoff until a later write rotates and flushes it; disk retained past the cutoff is +bounded by that store's `transactionLogMaxSize` (except when a single transaction exceeds the +target). Use `destroy: true` only to remove the store itself. - `options: object` - `before?: number` Remove all transaction log files older than the specified timestamp. @@ -1801,8 +1802,8 @@ stats.totals.transactionsWritten; // lifetime count of transactions written The `purge.retainedUnflushedFiles` gauge is useful for diagnosing why logs are not being cleaned up: a file can be older than the retention period but still retained because its transactions have -not yet been flushed to RocksDB (purging it would be unsafe for crash recovery). The highest -sequence file is not counted as purgeable even when it is old and fully flushed. +not yet been flushed to RocksDB (purging it would be unsafe for crash recovery). The durable floor +named by `txn.state` is not counted as purgeable even when it is old and fully flushed. ### Transaction Log Initialization diff --git a/docs/stats.md b/docs/stats.md index 1ff199539..f5b3e0e06 100644 --- a/docs/stats.md +++ b/docs/stats.md @@ -477,7 +477,7 @@ const stats: TransactionLogStats = log.getStats(); - `offset: number` The byte offset within that file. - `purge: object` - `oldestFileAgeMs: number` Age in milliseconds of the oldest file on disk. - - `purgeableFiles: number` Number of files below the durable highest-sequence floor that are + - `purgeableFiles: number` Number of files below the durable `txn.state` sequence floor that are eligible for purge under the retention policy. - `retainedUnflushedFiles: number` Number of files past the retention threshold but retained because they are unflushed. diff --git a/docs/transaction-log.md b/docs/transaction-log.md index 41e972770..dd09e99f7 100644 --- a/docs/transaction-log.md +++ b/docs/transaction-log.md @@ -198,9 +198,9 @@ await db.transaction((txn) => { - Log files are automatically rotated when either the index or data file reaches their configured maximum sizes - Rotation happens on the next write after the size limit is exceeded -- Old log files can be automatically purged based on retention policy. The highest sequence file is - retained as the live store's durable floor, so an idle store can keep one bounded file past the - cutoff until a later write rotates it. +- Old log files can be automatically purged based on retention policy. The sequence file named by + `txn.state` and every newer file form the live store's durable floor, so an idle store can keep + one bounded file past the cutoff until a later write rotates and flushes it. ### Error Handling diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 39d95c891..7f38a6e4a 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -556,9 +556,12 @@ void TransactionLogStore::collectStats(TransactionLogStoreStats& out) { } const bool retentionEnabled = this->retentionMs.count() > 0; - const uint32_t durableFloorSequence = this->sequenceFiles.empty() + uint32_t durableFloorSequence = this->sequenceFiles.empty() ? 0 : this->sequenceFiles.rbegin()->first; + if (this->sequenceFiles.find(flushedPosition.logSequenceNumber) != this->sequenceFiles.end()) { + durableFloorSequence = flushedPosition.logSequenceNumber; + } for (const auto& [seq, logFile] : this->sequenceFiles) { // A registered-but-never-opened older file still reads 0 here (see @@ -652,7 +655,11 @@ void TransactionLogStore::doPurge(std::function sequenceNumbersToRemove; - const uint32_t durableFloorSequence = this->sequenceFiles.rbegin()->first; + auto lastFlushedPosition = this->getLastFlushedPosition(); + uint32_t durableFloorSequence = this->sequenceFiles.rbegin()->first; + if (this->sequenceFiles.find(lastFlushedPosition.logSequenceNumber) != this->sequenceFiles.end()) { + durableFloorSequence = lastFlushedPosition.logSequenceNumber; + } for (const auto& entry : this->sequenceFiles) { auto& sequenceNumber = entry.first; @@ -676,18 +683,21 @@ void TransactionLogStore::doPurge(std::function this->retentionMs; } } catch (const std::filesystem::filesystem_error& e) { - // file was deleted or doesn't exist - DEBUG_LOG("%p TransactionLogStore::purge File no longer exists: %s\n", this, logFile->path.string().c_str()); - break; + if (e.code() == std::errc::no_such_file_or_directory) { + DEBUG_LOG("%p TransactionLogStore::purge Forgetting missing file: %s\n", this, logFile->path.string().c_str()); + sequenceNumbersToRemove.push_back(sequenceNumber); + continue; + } + throw; } catch (const std::exception& e) { DEBUG_LOG("%p TransactionLogStore::purge Failed to get last write time for file %s: %s\n", this, logFile->path.string().c_str(), e.what()); - break; + throw; } catch (...) { auto eptr = std::current_exception(); std::string errorMsg = getExceptionMessage(eptr); DEBUG_LOG("%p TransactionLogStore::purge Unknown error getting last write time for file %s: %s\n", this, logFile->path.string().c_str(), errorMsg.c_str()); - break; + throw; } } @@ -697,7 +707,6 @@ void TransactionLogStore::doPurge(std::functiongetLastFlushedPosition(); if (sequenceNumber > lastFlushedPosition.logSequenceNumber) { if (all) { continue; diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index d3721bfcc..b770dd89a 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -12,7 +12,7 @@ import { dbRunner, generateDBPath, terminateWorker } from './lib/util.ts'; import { createWorkerBootstrapScript } from './lib/worker-bootstrap.ts'; import assert from 'node:assert'; import { existsSync, readFileSync, statSync } from 'node:fs'; -import { mkdir, readdir, stat, utimes, writeFile } from 'node:fs/promises'; +import { mkdir, readdir, stat, unlink, utimes, writeFile } from 'node:fs/promises'; import { release } from 'node:os'; import { join } from 'node:path'; import { setTimeout as delay } from 'node:timers/promises'; @@ -2267,10 +2267,10 @@ describe('Transaction Log', () => { logFiles.push(logFile); } - // flushed position is at segment 1's end of entries — nothing unflushed + // flushed position is at segment 2's end, so segment 1 is below the floor const state = Buffer.alloc(8); state.writeUInt32LE(TRANSACTION_LOG_FILE_HEADER_SIZE, 0); - state.writeUInt32LE(1, 4); + state.writeUInt32LE(2, 4); await writeFile(join(logDirectory, 'txn.state'), state); db.open(); @@ -2338,6 +2338,83 @@ describe('Transaction Log', () => { expect(existsSync(stateFile)).toBe(true); })); + it('should purge an eligible prefix during startup', () => + dbRunner( + { skipOpen: true, dbOptions: [{ transactionLogRetention: 500 }] }, + async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + + const old = new Date(Date.now() - 60 * 1000); + const logFiles: string[] = []; + for (const sequence of [1, 2, 3]) { + const logFile = join(logDirectory, `${sequence}.txnlog`); + await writeFile(logFile, buildLogFile(1)); + await utimes(logFile, old, old); + logFiles.push(logFile); + } + + const state = Buffer.alloc(8); + state.writeUInt32LE(statSync(logFiles[2]).size, 0); + state.writeUInt32LE(3, 4); + const stateFile = join(logDirectory, 'txn.state'); + await writeFile(stateFile, state); + + db.open(); + expect(existsSync(logFiles[0])).toBe(false); + expect(existsSync(logFiles[1])).toBe(false); + expect(existsSync(logFiles[2])).toBe(true); + expect(existsSync(stateFile)).toBe(true); + } + )); + + it('should detach an already-missing prefix entry without wedging retention', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + + const logFiles: string[] = []; + for (const sequence of [1, 2, 3]) { + const logFile = join(logDirectory, `${sequence}.txnlog`); + await writeFile(logFile, buildLogFile(1)); + logFiles.push(logFile); + } + const state = Buffer.alloc(8); + state.writeUInt32LE(statSync(logFiles[2]).size, 0); + state.writeUInt32LE(3, 4); + await writeFile(join(logDirectory, 'txn.state'), state); + + db.open(); + const log = db.useLog('foo'); + await unlink(logFiles[0]); + expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([logFiles[1]]); + expect(log.getStats().fileCount).toBe(1); + expect(existsSync(logFiles[2])).toBe(true); + })); + + it('should retain the flushed-position segment for startFromLastFlushed readers', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + + const logFiles: string[] = []; + for (const sequence of [1, 2, 3]) { + const logFile = join(logDirectory, `${sequence}.txnlog`); + await writeFile(logFile, buildLogFile(1)); + logFiles.push(logFile); + } + const state = Buffer.alloc(8); + state.writeUInt32LE(statSync(logFiles[1]).size, 0); + state.writeUInt32LE(2, 4); + await writeFile(join(logDirectory, 'txn.state'), state); + + db.open(); + const log = db.useLog('foo'); + expect(db.purgeLogs({ name: 'foo', before: Date.now() + 1000 })).toEqual([logFiles[0]]); + expect(existsSync(logFiles[1])).toBe(true); + expect(Array.from(log.query({ startFromLastFlushed: true }))).toHaveLength(1); + })); + it('should keep repeated flush and retention purges bounded to the current file', () => dbRunner({ dbOptions: [{ transactionLogMaxSize: 500 }] }, async ({ db, dbPath }) => { const log = db.useLog('foo'); From c925e9b2f991a62b64c96e36aef752085a9cef24 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 06:50:13 -0600 Subject: [PATCH 11/13] fix(transaction-log): contain purge stat failures --- src/binding/transaction_log/transaction_log_store.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 7f38a6e4a..7c034f2e2 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -688,16 +688,17 @@ void TransactionLogStore::doPurge(std::functionpath.string().c_str(), e.what()); + break; } catch (const std::exception& e) { DEBUG_LOG("%p TransactionLogStore::purge Failed to get last write time for file %s: %s\n", this, logFile->path.string().c_str(), e.what()); - throw; + break; } catch (...) { auto eptr = std::current_exception(); std::string errorMsg = getExceptionMessage(eptr); DEBUG_LOG("%p TransactionLogStore::purge Unknown error getting last write time for file %s: %s\n", this, logFile->path.string().c_str(), errorMsg.c_str()); - throw; + break; } } From 8eb1a45b7fd36f24c847ce054a203b4b6d9d4dac Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 07:47:47 -0600 Subject: [PATCH 12/13] fix(transaction-log): activate only the surviving segment at discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directory iteration order is unspecified, so registerLogFile() promoted any segment that briefly held the highest sequence: it marker-enabled and opened that file, and nothing ever demoted it. Every superseded segment kept an append-boundary marker it will never use and an open handle for the life of the store. On Windows the handle is opened without FILE_SHARE_DELETE, so it also made those segments undeletable from outside the process — which is what failed the new already-missing-prefix purge test on all five Windows runtimes (EBUSY on unlink). Discovery now registers without opening; load() marker-enables and opens the file that is still current once the whole directory has been scanned. A current segment that cannot be opened degrades as before (append starts at a fresh sequence) rather than failing the load, while an append-boundary violation stays fatal. Co-Authored-By: Claude Opus --- AGENTS.md | 7 ++ .../transaction_log/transaction_log_store.cpp | 64 ++++++++++++++----- .../transaction_log/transaction_log_store.h | 8 +-- test/transaction-log.test.ts | 20 ++++++ 4 files changed, 80 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5d76ca0cc..31e85c2c5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -274,6 +274,13 @@ sufficient (env teardown does not honor tsfn acquire counts); see use the marked logical prefix and never expose the orphaned physical tail. The marker is not copied into backups: the copied prefix is already a clean canonical `.txnlog`. A known-zero-byte failure leaves the segment and its zero marker reusable. + Only the **active** segment owns a marker and an open handle, and startup discovery cannot know + which segment that is until the whole directory has been scanned — directory iteration order is + unspecified, so any segment can briefly hold the highest sequence. `registerLogFile()` therefore + registers without opening, and `load()` marker-enables and opens the surviving current file after + discovery. Promoting eagerly wrote a marker for a segment that is never appended to and, on + Windows (where the handle is opened without `FILE_SHARE_DELETE`), left every superseded segment + undeletable by anything outside the process for the life of the store. **The physical extent tracks `size` on POSIX only.** There the fd is `O_APPEND`, so writes go to physical EOF, not to `size`, and leaving orphaned bytes makes every later append land after a partial entry: a mid-file framing break that `recoverTail()` deliberately diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index 7c034f2e2..b686d7da4 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -366,9 +366,10 @@ void TransactionLogStore::ensureExtent(const std::shared_ptr } // Never borrow the active segment's handle. Its lifecycle belongs to the - // write path — registerLogFile()/getLogFile() open it before the first - // append — so the only way it reads size 0 and closed is the window between - // getLogFile() creating it and the first append, where 0 is the truth. A + // write path — load()'s post-discovery activation and getLogFile() open it + // before the first append — so the only way it reads size 0 and closed is the + // window between getLogFile() creating it and the first append, where 0 is + // the truth. A // close() here would drop a handle (and, on Windows, the mapping) the next // append expects to still hold. if (file->sequenceNumber == this->currentSequenceNumber.load(std::memory_order_relaxed)) { @@ -845,12 +846,16 @@ void TransactionLogStore::registerLogFile(const std::filesystem::path& path, con if (retiredBoundary == 0 && sequenceNumber >= this->currentSequenceNumber.load(std::memory_order_relaxed)) { - logFile->appendBoundaryMarkerEnabled = true; - if (!logFile->isOpen()) { - logFile->open(this->latestTimestamp); - } + // Do NOT open (or marker-enable) the file here. Directory iteration order is + // unspecified, so any segment can hold the highest-seen sequence for a moment + // and then be superseded; opening on that transient promotion left every + // superseded segment open for the life of the store and wrote an + // append-boundary marker for a file that is never appended to. On Windows an + // open handle also makes the segment undeletable by anything outside this + // process. load() activates whichever file is still current once discovery + // has finished. this->currentSequenceNumber.store(sequenceNumber, std::memory_order_relaxed); - this->nextLogPosition = { logFile->size, sequenceNumber }; + this->nextLogPosition = { 0, sequenceNumber }; } else if (retiredBoundary > 0 && sequenceNumber >= this->currentSequenceNumber.load(std::memory_order_relaxed)) { uint32_t nextWritableSequence = sequenceNumber + 1; @@ -1221,14 +1226,43 @@ std::shared_ptr TransactionLogStore::load( auto currentIt = store->sequenceFiles.find(storeCurrentSeq); if (currentIt != store->sequenceFiles.end()) { auto& currentFile = currentIt->second; - if (!currentFile->isOpen()) { - currentFile->open(store->latestTimestamp); + // Discovery deliberately leaves every file closed; the surviving current + // file is the only one that owns an append-boundary marker and an open + // handle. + currentFile->appendBoundaryMarkerEnabled = true; + bool activated = true; + try { + if (!currentFile->isOpen()) { + currentFile->open(store->latestTimestamp); + } + } catch (const TransactionLogAppendBoundaryException&) { + // The marker is authoritative; a segment we cannot reconcile with it + // must fail the load rather than expose orphaned bytes. + throw; + } catch (const std::exception& e) { + // A newest segment we cannot open is not fatal: leave it registered + // (readers still see whatever a later ensureExtent() can resolve) and + // start appending at a fresh sequence instead of failing the open. + DEBUG_LOG("%p TransactionLogStore::load Failed to open current log file %s: %s\n", + store.get(), currentFile->path.string().c_str(), e.what()); + currentFile->appendBoundaryMarkerEnabled = false; + activated = false; + } + if (activated) { + uint32_t protectedPosition = flushedPosition.logSequenceNumber == storeCurrentSeq + ? flushedPosition.positionInLogFile + : 0; + currentFile->recoverTail(protectedPosition); + store->nextLogPosition = { currentFile->size, storeCurrentSeq }; + } else { + uint32_t nextWritableSequence = storeCurrentSeq + 1; + store->currentSequenceNumber.store(nextWritableSequence, std::memory_order_relaxed); + store->nextLogPosition = { 0, nextWritableSequence }; + if (store->nextSequenceNumber <= nextWritableSequence) { + store->nextSequenceNumber = nextWritableSequence + 1; + } + storeCurrentSeq = nextWritableSequence; } - uint32_t protectedPosition = flushedPosition.logSequenceNumber == storeCurrentSeq - ? flushedPosition.positionInLogFile - : 0; - currentFile->recoverTail(protectedPosition); - store->nextLogPosition = { currentFile->size, storeCurrentSeq }; } } diff --git a/src/binding/transaction_log/transaction_log_store.h b/src/binding/transaction_log/transaction_log_store.h index 0133537fe..3354c302d 100644 --- a/src/binding/transaction_log/transaction_log_store.h +++ b/src/binding/transaction_log/transaction_log_store.h @@ -546,10 +546,10 @@ struct TransactionLogStore final { * Resolves `file->size` — the written extent — for a file that was * registered but never opened, by borrowing its handle briefly. * - * registerLogFile() opens the current file eagerly but leaves older ones - * lazy, and a lazy file reads `size == 0`, which is indistinguishable from - * "empty" to every consumer of the field. Two of them act on that: the - * backup snapshot would omit the segment, and doPurge()'s flushed-position + * Discovery leaves every registered file closed (load() opens only the + * surviving current one), and a closed file reads `size == 0`, which is + * indistinguishable from "empty" to every consumer of the field. Two of them + * act on that: the backup snapshot would omit the segment, and doPurge()'s flushed-position * guard would delete a segment whose tail never reached RocksDB. Anything * that makes a decision from `size` must call this first. * diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index b770dd89a..7e42db049 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -2392,6 +2392,26 @@ describe('Transaction Log', () => { expect(existsSync(logFiles[2])).toBe(true); })); + // Directory iteration order is unspecified, so startup discovery used to open + // (and write an append-boundary marker for) every segment that briefly held the + // highest sequence. On Windows the leaked handle also made a superseded segment + // undeletable from outside the process — the EBUSY that broke the detach test above. + it('should activate only the surviving current segment during discovery', () => + dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { + const logDirectory = join(dbPath, 'transaction_logs', 'foo'); + await mkdir(logDirectory, { recursive: true }); + + for (const sequence of [1, 2, 3]) { + await writeFile(join(logDirectory, `${sequence}.txnlog`), buildLogFile(1)); + } + + db.open(); + db.useLog('foo'); + + const markerDirectory = join(dbPath, 'transaction_logs', '.append-boundaries', 'foo'); + expect((await readdir(markerDirectory)).sort()).toEqual(['3.txnlog.boundary']); + })); + it('should retain the flushed-position segment for startFromLastFlushed readers', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); From c955d315308b3cf629d29336652243f1a3d7e473 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Wed, 26 Aug 2026 08:09:03 -0600 Subject: [PATCH 13/13] test(transaction-log): assert discovery activation order-independently The marker assertion alone can pass against the old code when directory enumeration yields the highest sequence first. Add an open-descriptor check via procfs (Linux) so the assertion does not depend on iteration order, and widen the fixture to six segments. Also drops two comments that narrated PR history rather than the invariant, and corrects the AGENTS.md wording: only the active segment may have a marker created, but a retired segment keeps the one it already earned. Co-Authored-By: Claude Opus --- AGENTS.md | 15 ++++---- .../transaction_log/transaction_log_store.cpp | 18 ++++------ test/transaction-log.test.ts | 34 +++++++++++++++---- 3 files changed, 42 insertions(+), 25 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 31e85c2c5..9486d33a5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -274,13 +274,14 @@ sufficient (env teardown does not honor tsfn acquire counts); see use the marked logical prefix and never expose the orphaned physical tail. The marker is not copied into backups: the copied prefix is already a clean canonical `.txnlog`. A known-zero-byte failure leaves the segment and its zero marker reusable. - Only the **active** segment owns a marker and an open handle, and startup discovery cannot know - which segment that is until the whole directory has been scanned — directory iteration order is - unspecified, so any segment can briefly hold the highest sequence. `registerLogFile()` therefore - registers without opening, and `load()` marker-enables and opens the surviving current file after - discovery. Promoting eagerly wrote a marker for a segment that is never appended to and, on - Windows (where the handle is opened without `FILE_SHARE_DELETE`), left every superseded segment - undeletable by anything outside the process for the life of the store. + Only the **active** segment may have a marker _created_ for it (a retired segment keeps the one it + already earned), and startup discovery cannot know which segment that is until the whole directory + has been scanned — directory iteration order is unspecified, so any segment can briefly hold the + highest sequence. `registerLogFile()` therefore registers without opening, and `load()` + marker-enables and opens the surviving current file after discovery. Promoting eagerly minted a + marker for a segment that is never appended to and, on Windows (where the handle is opened without + `FILE_SHARE_DELETE`), left every superseded segment undeletable by anything outside the process for + the life of the store. **The physical extent tracks `size` on POSIX only.** There the fd is `O_APPEND`, so writes go to physical EOF, not to `size`, and leaving orphaned bytes makes every later append land after a partial entry: a mid-file framing break that `recoverTail()` deliberately diff --git a/src/binding/transaction_log/transaction_log_store.cpp b/src/binding/transaction_log/transaction_log_store.cpp index b686d7da4..088f31003 100644 --- a/src/binding/transaction_log/transaction_log_store.cpp +++ b/src/binding/transaction_log/transaction_log_store.cpp @@ -846,14 +846,13 @@ void TransactionLogStore::registerLogFile(const std::filesystem::path& path, con if (retiredBoundary == 0 && sequenceNumber >= this->currentSequenceNumber.load(std::memory_order_relaxed)) { - // Do NOT open (or marker-enable) the file here. Directory iteration order is - // unspecified, so any segment can hold the highest-seen sequence for a moment - // and then be superseded; opening on that transient promotion left every - // superseded segment open for the life of the store and wrote an - // append-boundary marker for a file that is never appended to. On Windows an - // open handle also makes the segment undeletable by anything outside this - // process. load() activates whichever file is still current once discovery - // has finished. + // Record the candidate only; load() opens and marker-enables whichever file + // is still current once the whole directory has been scanned. Directory + // iteration order is unspecified, so any segment can hold the highest-seen + // sequence for a moment, and a segment opened on that transient promotion is + // never closed again: it keeps an append-boundary marker it will never use + // and, on Windows (no FILE_SHARE_DELETE), stays undeletable by anything + // outside this process for the life of the store. this->currentSequenceNumber.store(sequenceNumber, std::memory_order_relaxed); this->nextLogPosition = { 0, sequenceNumber }; } else if (retiredBoundary > 0 && @@ -1226,9 +1225,6 @@ std::shared_ptr TransactionLogStore::load( auto currentIt = store->sequenceFiles.find(storeCurrentSeq); if (currentIt != store->sequenceFiles.end()) { auto& currentFile = currentIt->second; - // Discovery deliberately leaves every file closed; the surviving current - // file is the only one that owns an append-boundary marker and an open - // handle. currentFile->appendBoundaryMarkerEnabled = true; bool activated = true; try { diff --git a/test/transaction-log.test.ts b/test/transaction-log.test.ts index 7e42db049..f5e4e4a52 100644 --- a/test/transaction-log.test.ts +++ b/test/transaction-log.test.ts @@ -11,7 +11,7 @@ import { writeLazyTransactionLogSegments } from './lib/transaction-log-fixtures. import { dbRunner, generateDBPath, terminateWorker } from './lib/util.ts'; import { createWorkerBootstrapScript } from './lib/worker-bootstrap.ts'; import assert from 'node:assert'; -import { existsSync, readFileSync, statSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync, readlinkSync, statSync } from 'node:fs'; import { mkdir, readdir, stat, unlink, utimes, writeFile } from 'node:fs/promises'; import { release } from 'node:os'; import { join } from 'node:path'; @@ -2073,6 +2073,20 @@ describe('Transaction Log', () => { }); describe('purgeLogs', () => { + // Which `.txnlog` files under `logDirectory` this process currently holds + // open, via procfs. Linux only; callers guard on the platform. + const openLogDescriptors = (logDirectory: string): string[] => + readdirSync('/proc/self/fd') + .map((fd) => { + try { + return readlinkSync(`/proc/self/fd/${fd}`); + } catch { + return ''; + } + }) + .filter((target) => target.startsWith(logDirectory) && target.endsWith('.txnlog')) + .sort(); + // Build a transaction log file image with a valid header followed by // `entryCount` well-formed v1 entry frames so the native counter has real // framing to walk. @@ -2392,24 +2406,30 @@ describe('Transaction Log', () => { expect(existsSync(logFiles[2])).toBe(true); })); - // Directory iteration order is unspecified, so startup discovery used to open - // (and write an append-boundary marker for) every segment that briefly held the - // highest sequence. On Windows the leaked handle also made a superseded segment - // undeletable from outside the process — the EBUSY that broke the detach test above. + // Only the segment that is still current after the whole directory has been + // scanned may be opened or marker-enabled. A segment that is merely the + // highest seen so far must not be, and directory iteration order decides + // which those are. The open-descriptor assertion is the order-independent + // half — on a filesystem that enumerates in sorted order the marker + // assertion catches it too, but a hash-ordered one can yield the highest + // sequence first and leave nothing behind. it('should activate only the surviving current segment during discovery', () => dbRunner({ skipOpen: true }, async ({ db, dbPath }) => { const logDirectory = join(dbPath, 'transaction_logs', 'foo'); await mkdir(logDirectory, { recursive: true }); - for (const sequence of [1, 2, 3]) { + for (const sequence of [1, 2, 3, 4, 5, 6]) { await writeFile(join(logDirectory, `${sequence}.txnlog`), buildLogFile(1)); } db.open(); db.useLog('foo'); + if (process.platform === 'linux') { + expect(openLogDescriptors(logDirectory)).toEqual([join(logDirectory, '6.txnlog')]); + } const markerDirectory = join(dbPath, 'transaction_logs', '.append-boundaries', 'foo'); - expect((await readdir(markerDirectory)).sort()).toEqual(['3.txnlog.boundary']); + expect(await readdir(markerDirectory)).toEqual(['6.txnlog.boundary']); })); it('should retain the flushed-position segment for startFromLastFlushed readers', () =>