Keep the WriteBufferManager stall safeguard across a reopen

[kriszyp]

The stall safeguard added in #755 has never survived a restart. resolveMaxWriteBufferSizeToMaintain dropped the derived retained-history target to 0, and 0 is the one value RocksDB's transaction wrappers reinterpret: OptimisticTransactionDB::Open and TransactionDB::PrepareWrap each rewrite it to -1, which SanitizeOptions expands to max_write_buffer_number * write_buffer_size — 256MB per column family with this codebase's defaults. Only families created after an open, through DB::CreateColumnFamily, kept the 0. So the existing fresh-database test passed while every production restart reopened all families at 256MB apiece and wedged (#821, HarperFast/harper#2490).

The fix resolves the derived target to 1, the smallest value that survives that rewrite.

What changed

• The resolver returns a named 1 instead of 0, and an explicit 0 now takes the same path rather than being forwarded to the wrappers — passing 0 through hands the caller who asked for the least history the most. An explicit positive value is still honored untouched.
• The clamp keys off a WriteBufferManager being attached, not off it being a stalling one. writeBufferManagerAllowStall is mutable at runtime — DBSettings::Config propagates it through SetAllowStall — while max_write_buffer_size_to_maintain is an immutable ColumnFamilyOptions field fixed when a column family is created. Gating the clamp on it left every family opened before the switch permanently at 256MB, with nothing able to lower it. A non-stalling manager needs the bound anyway: without allowStall the same history is simply never reclaimed, which is the ~3GB of retained history WBM stall safeguard (#755) is undone on reopen: TransactionDB::Open re-derives max_write_buffer_size_to_maintain=0 to 256 MiB per CF, permanent stall on Harper prod #821 measured on the affected deployment's hot families while it was running with the stall disabled as a workaround.
• 1 is not "no history", and the docs now say so. MemTableListVersion::MemtableLimitExceeded compares against MemoryAllocatedBytesExcludingLast(), which subtracts the entry TrimHistory is about to pop, so with one entry the comparison is 0 >= 1 and the newest flushed memtable is retained. It is dropped on that family's next write: CheckMemtableFull schedules a trim whenever mem()->MemoryAllocatedBytes() + imm()->MemoryAllocatedBytesExcludingLast() >= target, which any non-empty mutable memtable satisfies at 1, and DBImpl::PreprocessWrite drains that scheduler before its ShouldStall() check. The bound is one flushed memtable per family instead of 256MB per family, and no positive target does better — the residual is independent of the target's magnitude.
• An over-budget report at open, on the log.warn channel and in the database's own LOG, when the families known at that open already reach the budget. It covers both consequences — memory the manager never reclaims, and a permanent stall under allowStall. It compares by division rather than multiplying, because the caller's target reaches native as an unvalidated int64, and uses >= because RocksDB stalls at memory_usage() >= buffer_size(). Its wording states that only the families present at that open are counted, so it is a report and never a proof of safety.
• The attachment it reads is the database's own, not the global setting. writeBufferManagerSize is mutable too, and dropping it to 0 is documented as "no new attachments" rather than a teardown, so a database keeps whichever manager it was opened with. buildColumnFamilyOptions therefore takes the attachment as a parameter: the open path passes the manager it just attached, and the late-column-family path in DBRegistry passes db->GetDBOptions().write_buffer_manager. Reading the global instead handed a family created after such a reconfiguration the unclamped 256MB while its history was still charged to the manager the database was holding — the same permanent stall by another route. A review round found this; the regression fails without the fix with late: 268435456 against late: 1. The over-budget report takes its budget from the same attached manager's buffer_size(); at open those two sources agree, so this is about reading the authoritative value rather than a bug that was reachable single-threaded.

src/binding/database/db_settings.cpp is untouched in the final diff. An earlier revision warned when allowStall was switched on at runtime; keying the clamp on the manager's presence makes that transition a non-event, so the warning went with it.

For the human reviewer

The framing changed mid-flight, on your ruling, and the planning verdict never cleared. Framing-Verdict: better-alternative-exists. The planning gate accepted the value and the layer but named the allowStall gate as a missing approach, on the mutability fact above. That was put to you as the one open question and you chose to drop the gate, so the clamp now applies to any attached manager. The two blockers the same gate raised are also fixed: explicit 0 was still taking the broken path, and the runtime transition defeated an open-time safeguard.

This changes behavior for the configuration Harper runs today. With writeBufferManagerAllowStall: false as the current workaround, families go from a 256MB conflict-check window to 1 — more ERR_TRY_AGAIN retries in the forced-flush regime, in exchange for the memory bound the manager exists to provide. The measurements below show no write-path throughput cost from the target itself, but the retry regime is the part worth watching after rollout.

1 is a large improvement, not a proof. The residual is one flushed memtable per family, so a budget below roughly familyCount * writeBufferSize can still wedge — and an idle family's retained memtable cannot be trimmed while every writer is stalled, because a write is what schedules the trim. I measured both sides: 7 families with a 1MB write buffer against an 8MB budget still stalls at target 1, while 4 families at the shipped 16MB default against a 128MB budget writes 160MB in 660ms. The production shape (28 families, 16MB write buffer, 661MB budget) sits inside that, but the arithmetic does not close for an unbounded family count. The over-budget report therefore cannot fire for the resolved target, and the test asserting the quiet case says in its name that this is the guard's blind spot rather than codifying it as correct. Every review round raised it; the adjudication kept it at major and ruled it belongs in its own issue rather than here. The mechanism is MemTableList::RemoveMemTable calling TrimHistory(to_delete, 0) while MemoryAllocatedBytesExcludingLast() excludes the sole entry, so the candidates are a pinned-RocksDB patch passing the active memtable's usage instead of 0, or an in-binding post-flush trim. Neither is something this PR can set.

One pre-existing shape the warning leans on. An existing database has every column family re-opened with the first opener's maxWriteBufferSizeToMaintain; unlike compression, per-family explicit values are not restored from the OPTIONS file. That is how this code already behaved and I did not change it, but the over-budget report's arithmetic assumes it. Making per-family targets persist would need the same LoadLatestOptions round-trip compression does.

Review points I declined, with the reason:

• Two rounds called the child-process fixtures a blocker because Node "does not natively parse TypeScript" and process.execArgv is not propagated. Both were adjudicated factually wrong: native type stripping is unflagged at this repo's engines floor, AGENTS.md documents .mts fixtures running with no flags, and all thirteen existing spawn-fixture tests use the identical form. CI confirms it on Node 22, 24 and 26 plus Bun and Deno.

• db->GetDBOptions() returns the struct by value at two call sites. Both are once per open and once per column-family creation, never a read or write path, and the adjudication downgraded it to a nit on that basis. The info_log one cannot use the local DBOptions anyway: RocksDB creates the logger during open and stores it on the database, so the local copy's is null.

• A standing nit across rounds is that the new comments still carry rationale AGENTS.md invariant 10 owns. I made a consolidation pass — the resolver docblock is now a pointer plus the three facts the code cannot state, and the duplicated restatements at the header, the DBRegistry call site and in the stall test are gone. What remains is deliberate.

• A leg called the child-process fixtures a blocker because Node "does not natively parse TypeScript", so spawning a .mts file would fail with ERR_UNKNOWN_FILE_EXTENSION. This repo's engines floor is ^22.18.0 || >=24.0.0 precisely because that is where native type stripping is unflagged, AGENTS.md documents the no-tsx rule, and test/fixtures/fork-park-timeout.mts already does exactly this. CI confirms it on Node 22, 24 and 26 across Linux, macOS and Windows, plus Bun and Deno.

• An earlier leg flagged the wbmAlreadyCreated gate on the runtime-transition warning as a proxy that is only true after the first write. It was true from the first open — the manager is created inside DBDescriptor::open — and the adjudication pass reached the same conclusion. That code is gone from the final diff regardless.

• Three rounds raised comment mass on the new docblocks. I kept them: each states a why or a constraint the code cannot express — the wrapper rewrite, the MemoryAllocatedBytesExcludingLast semantics, the overflow reason for the division, the immutability that motivates the clamp condition. That is what AGENTS.md asks comments to carry, and the surrounding file is written the same way.

Verification

The regression was watched go red on the base commit, most recently with the resolver reverted to origin/main and the tests kept:

 × keeps the safeguard across a reopen and does not stall (optimistic) 60289ms
   → expected 'SIGKILL' to be null
 × keeps the safeguard across a reopen and does not stall (pessimistic) 60234ms
   → expected 'SIGKILL' to be null
 × clamps under a non-stalling manager too 816ms
   → expected [ 268435456, 268435456, …(2) ] to deeply equal [ 1, 1, 1, 1 ]
 × normalizes an explicitly requested 0 rather than passing it to the wrappers 60249ms
   → expected 'SIGKILL' to be null
 × warns when the known families already reach the budget, and says so 415ms
   → expected [] to have a length of 1 but got +0
 × warns when the total exactly equals the budget 274ms
   → expected [] to have a length of 1 but got +0

 Tests  6 failed | 5 passed (11)

SIGKILL rather than a red assertion is the point for the stalling cases: put() runs store.putSync() before returning its promise, so a stalled write blocks the JS thread and no in-process timer or runner timeout can fire (#781). The scenario therefore runs in a child process the parent kills on a deadline, so a regression fails bounded instead of hanging the job. The non-stalling case cannot stall by construction, so it fails as a plain value assertion — which is what pins the new clamp condition.

A fourth case covers the attachment path: it opens a database under a manager, clears the global budget, then creates a column family and reads both families' targets out of the LOG after the child exits, because RocksDB's info logger is buffered and only flushes the newest block on close.

The fixture runs at the shipped 16MB writeBufferSize default with a 128MB budget over four families, so the pre-fix target it faces is the real 16 * 16MB rather than a scaled-down stand-in. Both transaction modes get their own case, because the rewrite is duplicated in each wrapper. The assertion reads the per-family max_write_buffer_size_to_maintain back out of the database LOG, which RocksDB rewrites on every open, and requires every family to be at 1 — the same observation the issue's repro used.

Measured, because the #755 benchmark cited in the issue measured the old effective 0 and not 1. Interleaved arms, fresh database per rep, 4 families, 1MB write buffer, 32MB written, WriteBufferManager off so every target is viable, median of 5:

target	organic flushing	flush every 2MB
0 (temporary local patch)	205ms	582ms
1	170ms	592ms
16MB	174ms	593ms
256MB	161ms	619ms

No measurable write-path cost from the positive target in either regime, including the forced-flush case where the extra history bookkeeping was predicted to show up.

Three existing tests changed meaning, and all were corrected rather than weakened.

The conflicting-commit case asserted ERR_TRY_AGAIN, and with a retained memtable the check can now find the conflict and report ERR_BUSY. It writes twice after the flush so the trim is scheduled and then drained, which puts it back on the minimal-history path the test exists to cover. Both codes refuse the commit, so this was never a safety change, but the specific code is what proves the scenario is the intended one.

write-buffer-manager.test.ts's two memory-reclamation cases observe what a retained window charges to the block cache, and under a configured manager that window is no longer the default. They now ask for an explicit 256MB target, which is what the default used to derive to there, so they keep testing WriteBufferManager charge accounting rather than the new default.

Separately, a latent cross-file hazard surfaced. The WriteBufferManager is a native process-global and Vitest's threads pool runs every file in one process, so the two WriteBufferManager test files were racing to create the singleton and only worked because of the file order. Growing the stall file changed that order and exposed it: costToCache disagreed between them and config() refuses to change it after creation. Both files now agree on it, and the stall file resets the manager when it finishes.

Suites. Rebased onto current origin/main (19 commits) and re-verified there. Full Vitest suite green locally: 856 passed, 9 skipped, 0 failed. Native GoogleTest: 159/159. pnpm check clean. CI was green on the pre-rebase head across Node 22, 24 and 26, Bun and Deno, on Linux, macOS and Windows, plus the native suite, stress tests and benchmarks.

CI on the rebased head is green as well: 24 pass, 0 fail. Two Windows alternate-runtime jobs failed on the first attempt and both passed on re-run, in areas this change cannot reach — Test on Bun (windows-latest) in lock-tracker.test.ts's coordinated-retry park case, and Test on Deno (windows-latest) which passed all 787 tests and then exited non-zero on a single [vitest-pool] worker-teardown crash. The diff touches no transaction, park, orphan-GC or verification-table code, those tests configure no WriteBufferManager, and both alternate runtimes use Vitest's forks pool so they do not share a process with the files that do. Worth knowing that both flaked, since the rebase pulled in 19 upstream commits that rewrote transaction.cpp, transaction_handle.cpp and src/transaction.ts.

No data-format change and no migration: this only changes an in-memory RocksDB option, so rolling back is mechanically safe and simply restores the permanent-stall defect. It does need rebuilt native prebuilds on every supported platform to take effect.

Refs #821

_{Review-Coverage: authored=claude; ran=gemini; adjudicated=domain; declined=codex,cursor-grok,cursor-composer; rounds=6 @ 923401c}

_{Human-Review-Need: 3 (decisions: clamp-on-any-manager-not-only-stalling, explicit-zero-normalized-without-manager, warn-rather-than-reject-over-budget, full-size-child-process-regression-tests, attachment-queried-not-cached) @ 923401c}

[gemini-code-assist]

Code Review

This pull request fixes a critical issue where the memtable history retention safeguard resolved to 0, which was rewritten back to its default (256MB) by RocksDB's transaction wrappers upon database reopen, leading to permanent write stalls. The safeguard has been updated to resolve to 1 instead of 0, and explicit 0 targets are now normalized to 1. Additionally, the PR introduces warnings for configurations exceeding the WriteBufferManager budget or when stalling is enabled after database initialization, and adds comprehensive regression tests. I have no further feedback to provide as there are no review comments.

[github-actions]

📊 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.27K ops/sec	41.20	39.85	625.857	0.113	121,365
🥈 rocksdb	2	10.41K ops/sec	96.05	92.14	31,565.363	1.25	52,059

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	28.46K ops/sec	35.13	34.06	2,742.867	0.146	142,317
🥈 rocksdb	2	10.74K ops/sec	93.09	90.09	2,965.501	0.124	53,713

ranges.bench.ts

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

Implementation	Rank	Operations/sec	Mean (ms)	Min (ms)	Max (ms)	RME (%)	Samples
🥇 lmdb	1	23.80K ops/sec	42.02	37.05	1,775.164	0.287	118,989
🥈 rocksdb	2	16.24K ops/sec	61.59	52.09	1,064.351	0.116	81,180

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	350.58 ops/sec	2,852.431	89.07	74,974.123	19.86	724
🥈 lmdb	2	26.22 ops/sec	38,139.656	429.57	1,206,814.472	136.384	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	37.07K ops/sec	26.98	12.56	20,496.583	0.840	185,330
🥈 lmdb	2	439.91 ops/sec	2,273.174	135.831	22,919.835	1.47	2,200

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	741.45K ops/sec	1.35	1.17	425.285	0.067	3,707,257
🥈 lmdb	2	443.67K ops/sec	2.25	1.20	10,180.479	1.03	2,218,351

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	809.40 ops/sec	1,235.476	1,056.793	2,782.127	0.449	1,619
🥈 lmdb	2	1.15 ops/sec	873,088.851	847,039.654	983,273.749	3.29	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	22.49K ops/sec	44.47	29.65	19,889.894	2.03	44,974
🥈 lmdb	2	827.45 ops/sec	1,208.527	97.63	8,955.727	5.08	1,655

---

Results from commit 2774046