Seed the monotonic timestamp floor from a named log at database open by kriszyp · Pull Request #825 · HarperFast/rocksdb-js

kriszyp · 2026-09-04T03:08:04Z

Adds a caller-named timestampFloorLog open option and seeds the process clock after transaction-log recovery but before descriptor publication, preventing a backward wall-clock step from reissuing retained local batch keys. The unused draft getEntry() surface and second-header-word parser have been removed, so rocksdb-js continues to own only the first metadata word.

Independent review also found a pre-existing writable/read-only log rediscovery race, tracked separately as #838.

For the human reviewer

Floor source: The implementation scans retained entries in the caller-named locally originated log, rather than adding a separately persisted high-water mark. This avoids new write-path durability machinery, but protection depends on retained history and startup work remains proportional to entries; changing this later requires a crash-safe persistence design.
Incomplete scans: An unreadable segment, framing break, or exhausted budget warns and opens with the partial floor instead of refusing database open. This favors availability over a strict uniqueness guarantee; changing it later would alter operational expectations.
Budget granularity: ROCKSDB_JS_TIMESTAMP_FLOOR_SCAN_MS is checked between segments, not within one segment. A large or oversized segment can exceed the stated budget; finer checks are local and can be added without changing the API.
Plausibility cutoff: Keys more than ten years ahead of wall clock are warned about and excluded instead of advancing the process-wide clock. The cutoff limits clock poisoning but can reject an extreme legitimate rollback; it is a single policy constant if a different bound is preferred.
Later opens: A later handle that names a different floor log warns and proceeds because seeding is first-open-only. Rejecting instead would make the request enforceable but could break callers that currently share an already-open descriptor.

Verification

pnpm build: production TypeScript and native binding build passed.
pnpm check: type-check, lint, and formatting passed.
pnpm test: 64 files passed, 1 skipped; 859 tests passed, 9 skipped. After the final scanner narrowing, the focused clock-floor and crash-recovery run passed 21/21, including the recover → append → query regression for copied Windows padding on POSIX.
./build/Release/rocksdb-js-native-tests: 176/176 passed at the final head.

Complexity: complicated

_{Review-Coverage: authored=codex; ran=claude,gemini; declined=cursor-grok,cursor-composer,domain; rounds=4 @ d89e15c}

_{Human-Review-Need: 3 @ d89e15c}

#811) Delivers #811's read surface, clock-floor seed and docs. `setTimestamp` hardening (deliverable 2) stays with #819. `getEntry()` / `getEntrySync()` return `{ value, localTime, version }`: `localTime` is the value's first word — the transaction timestamp it was written under and the key of its transaction-log batch — and `version` is the distinct second word when the producer set `HAS_DISTINCT_VERSION_FLAG` (`0x20000`, now exported alongside `VERSION_HEADER_TAG`), otherwise the same value. `value` decodes exactly as `get()` decodes it and the sentinel and option behavior match, so nothing about the existing read paths changes. The process clock is monotonic within a process only: a new process re-reads the wall clock, so a backward step between runs can reissue a timestamp that is already a durable batch key. The new `timestampFloorLog` open option names the log whose keys this process originates; every segment of that store is walked after open-time recovery and the clock is raised above the largest key found, inside `DBDescriptor::open` and so before any transaction can exist. The log is named rather than inferred because a log written under a timestamp adopted from another node is keyed by that node's clock, and seeding from it would ratchet this node's clock to the fastest peer at every restart. There is also no cheap shortcut to the largest key: a segment header carries the store's `latestTimestamp` as of its creation, which starts at 0 in each new process and so sits below older segments' keys after a rollback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

…ze the scan Pre-push review (Gemini) findings: - the documented `getEntry()` return type omitted the `| number` the `FRESH_VERSION_FLAG` sentinel needs, contradicting the text below it; - the best-effort path had no test. A segment whose token is corrupted now has one: the database still opens and the `log.warn` names it; - the open-time walk is the cost of the guarantee, so it is measured rather than asserted: ~31 ms for a 25,000-entry log, ~273 ms for 250,000 entries, roughly a millisecond per thousand entries, and nothing at all when the option is unset. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

The pre-push review kept one major finding across both rounds: the open-time walk is O(entries in the named log) on the calling thread, and a log's retention window bounds its age, not its entry count — so a busy node's log could stall `open()` for seconds and fail a startup health check. The walk is now bounded by `ROCKSDB_JS_TIMESTAMP_FLOOR_SCAN_MS` (default 2000) and goes newest segment first, because a rollback leaves the highest keys in the run it interrupted. Running out warns and keeps the floor reached so far, which costs coverage rather than correctness — the same best-effort contract an unreadable segment already had. The value is honored literally, `0` included; there is no unbounded setting, since the failure it bounds is an open that does not return. Two smaller review items: the corrupt-segment test now proves the walk carries on past the failure and still seeds from the healthy segment, and a `timestampFloorLog` naming a log the database does not have now warns instead of silently protecting nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

…serve (#811) Two majors from the domain review of the previous commit. **A mid-file framing break silently truncated the scan.** Entries after such a break are durable and `query()` resyncs past them (AGENTS.md invariant 11), but the recovery walk stops at the break — so the floor was seeded from the keys before it while reporting a complete answer. The walk now says it stopped short, which puts the case on the same best-effort footing as an unreadable segment: warned, never silently wrong. Resuming past the break is #817's change; when it lands this becomes a non-event. **A second open of the same path could not apply the option, and said nothing.** The seed runs inside `DBDescriptor::open`, which happens once per path per process, so a later open of another column family or from a worker env carrying `timestampFloorLog` was ignored. It now warns rather than looking applied. It warns rather than throwing, unlike its first-open-only siblings, because this option only ever raises a floor — refusing the open would cost more than it protects. Also from that round: an implausible key is now excluded per entry rather than per segment, so one corrupt key no longer discards the real keys beside it, and the budget's per-segment granularity and the README anchor are corrected. The native floor tests took their targets from the wall clock while mutating a process-global floor, which made them order-coupled; they now take them from the floor itself and pass under `--gtest_shuffle --gtest_repeat=5`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

…811) Round 4 review. The mid-file-break case shared the "could not be read at open" warning, which points an operator at disk health and permissions when the real cause is a framing break at a known offset that a later query surfaces as a corrupt frame; it now has its own text. And the round-3 fix was proven only at the scanner primitive, so a regression anywhere in the two adapters between it and the warning would have gone green: there is now a test that overruns a frame's declared length in a real log, leaves the high keys behind the break, and asserts both the warning and that the floor does not claim them. Also from that round: the `skipDecode` case now pins the bytes rather than just the type, `getEntry()` documents that with a copying decoder `skipDecode` hands back the same reusable buffer `get()` does, and the segment-overshoot figure no longer reads as a ceiling. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

Round 5 review, all minors. The incomplete-scan warning reported only the first reason, so a budget that ran out while a segment also had a framing break sent the operator after the wrong one. It now names each reason it found. Two smaller items: the shared-buffer retention note told the caller to keep a `subarray`, which is a view of the buffer about to be overwritten rather than a copy of it; and `HAS_DISTINCT_VERSION_FLAG` was exported both at the top level and on `constants`, while the two flags beside it in the same metadata word live only on `constants` — it now follows them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

…811) Round 6 review, which caught two regressions from round 5's fixes. Moving implausible-key isolation from per-segment to per-entry made the warning text wrong: that segment's other keys *do* seed the floor now, and the reported key is a store-wide maximum, so "not seeded from that segment" would send an operator to repair something that was never skipped. And the every-reason accumulator had flags for the budget and for framing breaks but not for the read and open failures, so "could not be read at open" only appeared when it was the sole reason — a log with both an unreadable segment and a break reported only the break. The read surface's headline claim — that `localTime` is the key of the batch that wrote the record — was asserted nowhere: every header in the suite was hand-assembled, so a divergence between the timestamp a transaction claims and the key its batch lands under would have passed green. There is now a case that writes a real transaction's timestamp into both the record and the log and seeks the log at the `localTime` it reads back. Also: `getEntry()` no longer reports a `version` for a value whose first word is unusable, which the README already said it would not, and the fixture's payload constant is no longer exported from a file nothing imports. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

Round 7. Saying the floor is "seeded from every other key in the log" is not true when a framing break or an exhausted budget also left keys unread. The warning now says only what this check did: it excludes keys one at a time, not the segments holding them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

Raising `ROCKSDB_JS_TIMESTAMP_FLOOR_SCAN_MS` is the documented way to buy a longer walk, but the deadline is a `steady_clock` time point and the addition happens in that clock's resolution — so a large enough value wrapped into the past and scanned nothing at all, the exact opposite of the request, leaving the floor unseeded. The budget is capped at a day, which is far past any open worth waiting for, with a test that would fail on the wrap. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

The docs claimed the value is honored literally, which stopped being true when the previous commit capped it to keep the deadline from overflowing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

Post-rebase review. The walk reported an incomplete scan only for a mid-file framing break, but a torn tail stops it just as short — and when the break sits inside a prefix RocksDB already flushed, `recoverTail()` leaves the file at full extent, so the entries past it stay durable and unread. The floor was then seeded from the keys before the break while telling the operator the scan was complete, which is the silent under-seed this reporting exists to prevent, one classification over. Any classification but a clean walk now counts, with a native case for the torn tail and wording that covers both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

The previous commit's native case asserted the scanner's own output, which the fix does not change — reverting the predicate left every test green, so the silent under-seed it closed had no regression guard at all. The guard now runs over a real transaction log file and asserts the propagation itself (`MaxEntryScan::stoppedAtBreak`) on a retained torn tail and on a clean file; it fails on the old predicate. Two wording fixes from the same round: the warning no longer asserts flatly that entries after a break stay durable, which is untrue for a plain torn tail and would train an operator to ignore the warning that matters; and the comment explaining the branch no longer says "mid-file", which stopped being the whole story when the predicate broadened. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011WS3SZS5e2DmtMJB6adntj

gemini-code-assist

Code Review

This pull request introduces a process-wide monotonic timestamp floor mechanism (timestampFloorLog) to prevent backward wall-clock steps across restarts from reissuing transaction timestamps, along with new getEntry and getEntrySync APIs to retrieve values with their header clock words. The review feedback highlights a potential loss of precision in getWallClockTimestamp() when casting nanoseconds to double, a strict environment variable parsing check that could silently ignore configurations with trailing whitespace, and a compatibility issue in tests when spawning .mts fixtures without propagating process.execArgv.

gemini-code-assist · 2026-09-04T03:11:07Z

+double getWallClockTimestamp() {
 	int64_t now = std::chrono::duration_cast<std::chrono::nanoseconds>(
 		std::chrono::system_clock::now().time_since_epoch()
 	).count();
+	return static_cast<double>(now) / 1000000.0;
+}


In getWallClockTimestamp(), casting a 64-bit integer nanosecond timestamp (now) directly to a double before dividing by 1000000.0 can lead to a loss of precision. A double has 53 bits of precision (about $9 \times 10^{15}$), whereas the current nanosecond timestamp is around $1.7 \times 10^{18}$ (exceeding $2^{53}$). This results in a loss of precision in the lower bits of the nanosecond value (coarsening the resolution to ~250ns).

To maintain full precision for the millisecond part and avoid any rounding errors, you can perform the division in integer space first to get the exact milliseconds, and then add the fractional sub-millisecond part.

Suggested change

double getWallClockTimestamp() {

int64_t now = std::chrono::duration_cast<std::chrono::nanoseconds>(

std::chrono::system_clock::now().time_since_epoch()

).count();

return static_cast<double>(now) / 1000000.0;

}

double getWallClockTimestamp() {

auto now = std::chrono::system_clock::now().time_since_epoch();

auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(now).count();

auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(now).count() % 1000000;

return static_cast<double>(ms) + static_cast<double>(ns) / 1000000.0;

}

gemini-code-assist · 2026-09-04T03:11:08Z

+				size_t consumed = 0;
+				long long parsed = std::stoll(raw, &consumed);
+				constexpr long long maxBudgetMs = 24LL * 60LL * 60LL * 1000LL;
+				if (consumed == std::strlen(raw) && parsed >= 0) {
+					return std::chrono::milliseconds(std::min(parsed, maxBudgetMs));
+				}


The check consumed == std::strlen(raw) is extremely strict and will cause the environment variable ROCKSDB_JS_TIMESTAMP_FLOOR_SCAN_MS to be silently ignored (falling back to the default of 2000ms) if it contains any trailing whitespace or a trailing newline (which is very common when variables are set via shell scripts or exported in certain environments).

To make this more robust and user-friendly, you should allow and skip any trailing whitespace characters after parsing.

size_t consumed = 0; long long parsed = std::stoll(raw, &consumed); constexpr long long maxBudgetMs = 24LL * 60LL * 60LL * 1000LL; while (consumed < std::strlen(raw) && (raw[consumed] == ' ' || raw[consumed] == '\t' || raw[consumed] == '\r' || raw[consumed] == '\n')) { consumed++; } if (consumed == std::strlen(raw) && parsed >= 0) { return std::chrono::milliseconds(std::min(parsed, maxBudgetMs)); }

gemini-code-assist · 2026-09-04T03:11:08Z

+	env: Record<string, string> = {}
+): Promise<{ code: number | null; stdout: string; stderr: string }> {
+	return new Promise((resolve, reject) => {
+		const child = spawn(process.execPath, [fixture, mode, dbPath, String(key), log], {


When spawning the child process for the test fixture, process.execPath is called with the .mts file directly. In Node.js LTS versions (like 18 or 20) that do not natively support executing TypeScript files without a loader, this will fail with ERR_UNKNOWN_FILE_EXTENSION.

To ensure cross-version compatibility and that the child process inherits the same TypeScript loaders/configuration as the parent test runner, you should propagate process.execArgv to the spawned child process.

Suggested change

const child = spawn(process.execPath, [fixture, mode, dbPath, String(key), log], {

const child = spawn(process.execPath, [...process.execArgv, fixture, mode, dbPath, String(key), log], {

github-actions · 2026-09-04T03:18:16Z

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	24.56K ops/sec	40.72	39.38	588.965	0.113	122,778
🥈 rocksdb	2	10.72K ops/sec	93.27	90.18	31,361.741	1.24	53,610

getSync() > sequential keys - small key size (100 records)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.52K ops/sec	35.06	34.14	705.666	0.065	142,616
🥈 rocksdb	2	10.46K ops/sec	95.58	92.73	634.789	0.059	52,313

ranges.bench.ts

getRange() > small range (100 records, 50 range)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	25.67K ops/sec	38.95	36.10	1,777.42	0.289	128,371
🥈 rocksdb	2	16.24K ops/sec	61.58	52.00	1,086.061	0.126	81,190

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	343.96 ops/sec	2,907.315	59.74	107,301.008	19.91	688
🥈 lmdb	2	26.41 ops/sec	37,861.088	445.704	1,236,189.522	136.906	64.00

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	39.03K ops/sec	25.62	11.19	20,828.901	0.855	195,166
🥈 lmdb	2	442.09 ops/sec	2,261.982	223.552	16,516.644	1.38	2,211

Transaction log > read one entry from random position from log with 1000 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	723.60K ops/sec	1.38	1.18	4,731.499	0.200	3,618,009
🥈 lmdb	2	446.85K ops/sec	2.24	1.09	5,639.714	0.803	2,234,246

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	847.84 ops/sec	1,179.465	998.754	2,412.094	0.391	1,696
🥈 lmdb	2	1.14 ops/sec	880,078.694	829,498.893	954,103.025	3.45	10.00

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 rocksdb	1	18.29K ops/sec	54.67	30.32	21,449.201	2.52	36,589
🥈 lmdb	2	811.31 ops/sec	1,232.571	53.25	15,100.551	5.75	1,623

Results from commit f5fc304

Keep record-header decoding in Harper's RecordEncoder, where the complete value layout is owned. Preserve the independent timestamp-floor-at-open behavior and its tests. Co-Authored-By: Codex GPT-5 <noreply@openai.com>

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

kriszyp and others added 12 commits September 3, 2026 20:06

gemini-code-assist Bot reviewed Sep 4, 2026

View reviewed changes

kriszyp and others added 3 commits September 5, 2026 22:13

Remove the unused dual-clock read surface (#811)

1ed0fb9

Keep record-header decoding in Harper's RecordEncoder, where the complete value layout is owned. Preserve the independent timestamp-floor-at-open behavior and its tests. Co-Authored-By: Codex GPT-5 <noreply@openai.com>

Make clock-floor scans independent of shared log state

554b5af

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

Harden bounded timestamp floor scans

d9d44fb

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

kriszyp mentioned this pull request Sep 6, 2026

Concurrent read-only discovery can recover a live transaction-log segment #838

Open

Keep zero-tail normalization on recovery paths

d89e15c

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

kriszyp changed the title ~~Expose both record clock words and seed the timestamp floor from the named log at open~~ Seed the monotonic timestamp floor from a named log at database open Sep 6, 2026

Navigation Menu

Uh oh!

Seed the monotonic timestamp floor from a named log at database open - #825

Seed the monotonic timestamp floor from a named log at database open#825
kriszyp wants to merge 16 commits into
mainfrom
feat/dual-clock-read-surface-and-clock-floor

kriszyp commented Sep 4, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	const child = spawn(process.execPath, [fixture, mode, dbPath, String(key), log], {
	const child = spawn(process.execPath, [...process.execArgv, fixture, mode, dbPath, String(key), log], {

Uh oh!

Conversation

kriszyp commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For the human reviewer

Verification

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Benchmark Results

get-sync.bench.ts

getSync() > random keys - small key size (100 records)

getSync() > sequential keys - small key size (100 records)

ranges.bench.ts

getRange() > small range (100 records, 50 range)

realistic-load.bench.ts

Realistic write load with workers > write variable records with transaction log

transaction-log.bench.ts

Transaction log > read 100 iterators while write log with 100 byte records

Transaction log > read one entry from random position from log with 1000 100 byte records

worker-put-sync.bench.ts

putSync() > random keys - small key size (100 records, 10 workers)

worker-transaction-log.bench.ts

Transaction log with workers > write log with 100 byte records

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kriszyp commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading