Skip to content

Stop the RocksDB WriteBufferManager from stalling every writer by default - #2492

Merged
kriszyp merged 3 commits into
mainfrom
fix/wbm-allow-stall-default-false
Sep 3, 2026
Merged

kriszyp merged 3 commits into
mainfrom
fix/wbm-allow-stall-default-false

Conversation

@kriszyp

@kriszyp kriszyp commented Sep 3, 2026 •

Copy link
Copy Markdown
Member

storage.rocks.writeBufferManagerAllowStall now defaults to false, so the process-wide RocksDB WriteBufferManager — enabled by default at 1/3 of the block cache — is a soft cap that flushes harder rather than a hard cap that blocks writers. Under the old true default, reaching the budget parks every writer across every database in the process inside DBImpl::WriteBufferManagerStallWrites(), and nothing guarantees the flush that would release them: WriteBufferManager::ShouldFlush() only triggers once mutable memtable memory alone reaches half the budget, so when the budget is spread across many column families each far below its own flush trigger, RocksDB schedules no WBM-reason flush at all and the only exit is a process restart. That is the production wedge in #2490 — two nodes daily on 5.2.6–5.2.8, rocksdb.flush.reason.write_buffer_manager COUNT : 0 in the RocksDB LOG, every writer thread parked on the stall condition variable. An explicitly configured value still wins in both directions, and a non-boolean configured value still falls through to the default.

The flip is not free, and it is only half the fix. rocksdb-js derives max_write_buffer_size_to_maintain to 0 only when a stalling manager is configured (HarperFast/rocksdb-js#755), so with the stall off each column family keeps its full derived conflict-check history, and retained history is a floor RocksDB trims toward and never below — aggressive flushing does not release it. Measured against the pinned rocksdb-js 2.8.0 by opening a database each way and reading the generated OPTIONS file: max_write_buffer_number=16, write_buffer_size=16777216, and max_write_buffer_size_to_maintain = 268435456 with the stall off versus 0 with it on — 256 MiB of retained history per column family, so memtable memory can grow toward the sum over families of min(256 MiB, bytes written), above the manager's budget. HarperFast/rocksdb-js#821 is the other half: it lets that target be lowered and survive a reopen. Both are needed — this change alone leaves the memory growth, #821 alone leaves the stall.

The conflict-check window is what this flag was silently choosing, in the opposite direction from what the old comment claimed: today's true default is the one that eliminates the window, and false restores it. rocksdb-js#755 measured the near-zero-window arm on durable storage — no cost under organic flushing (1467 vs 1413 commits/s at a 10 ms transaction window, zero ERR_TRY_AGAIN in either arm) and at most ~1.4x attempts per commit when flushes are forced at a cadence comparable to transaction lifetime. Harper already retries both ERR_BUSY and ERR_TRY_AGAIN in resources/DatabaseTransaction.ts, so that retry path is what the current default already pays for; this change moves toward more conflict-check window, not less.

Docs follow-up, in the separate HarperFast/documentation repo (not editable from this PR) — reference/database/storage-tuning.md: line 233, which states the default is true, becomes false; line 237, the false (soft cap) bullet describes the overshoot as merely "brief"/"temporary" and should say memtable memory can sit at the per-column-family retained-history floor until rocksdb-js#821 lands; line 240, the paragraph beginning "The default (true) strictly bounds total memtable memory, applying write backpressure … which also keeps bulk ingest from outrunning the memtable flush/conflict-check window" inverts the trade in both halves and must be rewritten to present true as an opt-in hard cap that can wedge every writer in the process (#2490).

For the human reviewer

  1. Shipping half the fix now, and paying the memory cost until rocksdb-js#821. Chosen: flip the default and accept the 256 MiB-per-family retained-history floor measured above, with the rationale and its cost recorded at the default itself, tracked to closure by 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 rocksdb-js#821. Alternative both outside review legs raised: bound the history from Harper by passing maxWriteBufferSizeToMaintain at open. Overruled on a concrete disqualifier — a 0 target does not survive TransactionDB::PrepareWrap, which rewrites it to -1 and re-derives the 256 MiB, and a non-zero value would mean choosing a conflict-check window for every table in every deployment from this repo without the per-family budget arithmetic Fall back to polling-based file watching on inotify/FD exhaustion #821 adds. Reversibility: total, per node and without a code change (storage.rocks.writeBufferManagerAllowStall: true restores today's behavior; writeBufferManagerSize: 0 disables the manager entirely). What a "no" costs: the status quo is a deterministic all-writer wedge whose only exit is a restart, hit daily in production. Where to look hardest: the worked small-container case — a 1 GiB cgroup resolves an ~85 MiB manager budget while four write-active families can retain ~1 GiB of history, and with costToCache: true that is charged against the block cache, evicting reads rather than slowing writes. If that profile is unacceptable for the 5.2 line, the alternative is to hold this PR until Fall back to polling-based file watching on inotify/FD exhaustion #821, not to bound the history from here.

  2. The default changes for every deployment on upgrade, silently. Operators who tuned writeBufferManagerSize on the assumption of a hard cap get a different memory profile with no config change of their own. Chosen: flip the default rather than leave true and have affected deployments opt out, because the deployments that most need the fix are the ones that never touched storage.rocks.* (the production nodes in RocksDB WriteBufferManager stall (writeBufferManagerAllowStall=true default) wedges every writer until restart — root cause of #2450 #2490 have no overrides at all). The mitigation is a release note plus the docs follow-up above — worth confirming this lands in the 5.2 release notes, since the code change cannot trigger one.

  3. No permanent end-to-end guard in this repo, and the in-tree "reproducer" is not one. The adjudicating review's major finding was that the central claim rested on a pure-function assertion, and it pointed at integrationTests/database/crosstable-index-scan-completeness.test.ts and crosstable-index-scan-blast-radius.test.ts, whose comments say storage.rocks.writeBufferManagerSize: 8388608 "now HANGS ... indefinitely". I re-armed that cap locally and ran the fixture both ways: it passes on both settings (four runs; wall time varied 15-58s in both arms, dominated by harness boot). That fixture forces flush() between seeding waves — precisely the release the production wedge lacks — so it cannot reproduce the stall, its "now hangs" comments are stale, and re-arming it would be a guard that never goes red. Chosen instead: verify at the storage boundary with the two-arm live smoke below and commit no test, because the failure mode is a hang (a regression would wedge CI rather than fail it) and Harper cannot read back the effective setting (RocksDatabase.config() is write-only). A permanent guard belongs in rocksdb-js next to Fall back to polling-based file watching on inotify/FD exhaustion #821.

Verification

Route: live smoke with recorded evidence at the storage boundary (see decision 3 for why not an integration test).

  • Two-arm live smoke, same build. A script feeds resolveRocksMemoryConfig's output straight into RocksDatabase.config() (rocksdb-js 2.8.0 / RocksDB 11.8.1) and then putSyncs 8 KiB values round-robin across 40 column families in 3 databases under a 2 MiB manager budget — the shape of the production incident (one budget spread over many families, none near its own flush trigger, no forced flushes).
    • default (configuredAllowStall: undefined -> writeBufferManagerAllowStall: false): 3040 putSync in 30s, worst single call 497ms, RESULT: no stall.
    • explicit true (today's default, everything else identical): blocks inside the first 200 calls and never returns; killed at 90s.
    • The arms differ only in this flag, so the wedge is the WriteBufferManager stall and not per-column-family backpressure — the pinned build allows 16 x 16 MiB per family, nowhere near binding at this volume.
  • Retained-history measurement behind the memory trade above: opened a database each way and read RocksDB's generated OPTIONS file — max_write_buffer_size_to_maintain is 268435456 with the stall off and 0 with it on, at max_write_buffer_number=16 and write_buffer_size=16777216.
  • Unit: npx mocha unitTests/utility/rocksMemoryConfig.test.js — 15 passing, covering the new default, an explicit true, an explicit false, and a truthy non-boolean that must not coerce into a stall.
  • Fails-on-base: with utility/rocksMemoryConfig.ts restored to origin/main and a clean rm -rf dist && npm run build, the updated suite goes red — 2 failing, both AssertionError [ERR_ASSERTION] ... true !== false, at the default case and the non-boolean-fallback case. Restoring the change and rebuilding returns 15 passing.
  • Suites: npm run test:unit:main — 5290 passing, 2 failing. npm run test:unit:resources — 2019 passing, 1 failing. All three failures are environmental and unrelated to this diff: tokenAuthentication > rsa_keys is defined and updateAttributesLock > warns exactly once when a successful acquisition waited past the slow-wait threshold both pass when re-run alone (they collide with a concurrent mocha run over this box's shared system database), and configValidator > does not warn when a relative rootPath resolves within the limit fails in any deep checkout — it resolves relative/root against process.cwd(), 77 bytes here, so the socket path exceeds the 107-byte limit the test expects it to stay under.

Refs #2490

Complexity: easy

Review-Coverage: authored=claude; ran=gemini,codex; declined=cursor-grok,cursor-composer,domain; rounds=3 @ 1c6ab06

Human-Review-Need: 4 @ 1c6ab06

kriszyp and others added 3 commits September 3, 2026 13:47
resolveRocksMemoryConfig defaulted writeBufferManagerAllowStall to true, so
the process-wide WriteBufferManager (on by default at 1/3 of the block cache)
was a hard cap. When the budget is reached, RocksDB parks every writer across
every database in DBImpl::WriteBufferManagerStallWrites() and nothing
guarantees the flush that would release them: WriteBufferManager::ShouldFlush()
only fires when mutable memory alone reaches half the budget, so a budget held
by memory that is not mutable — the shape you get when it is spread over many
column families — never drops and the only exit is a process restart (#2490).

Default it to false instead: the budget becomes a soft cap and RocksDB
schedules flushes more aggressively rather than blocking writers. Explicitly
configured values still win in both directions, and a non-boolean value still
falls through to the default.

The trade is memory. rocksdb-js derives max_write_buffer_size_to_maintain to 0
only under a stalling manager, so with the stall off each column family keeps
its full derived conflict-check history (maxWriteBufferNumber *
writeBufferSize), a floor RocksDB trims toward but never below.
HarperFast/rocksdb-js#821 is the other half of the fix.

Refs #2490

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NM7k8TuWFtb36JajMCjKS9
Both outside review legs flagged it: the comment restated the assertion on
the next line.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NM7k8TuWFtb36JajMCjKS9
Two review rounds flagged the eight-line block as an internals tour. Keep the
non-obvious why (a stall with no flush guaranteed to release it) and the
memory cost; drop the ShouldFlush() mechanics.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NM7k8TuWFtb36JajMCjKS9
@kriszyp
kriszyp requested a review from dawsontoth September 3, 2026 20:14
@kriszyp kriszyp added this to the v5.2 milestone Sep 3, 2026
@kriszyp
kriszyp requested a review from heskew September 3, 2026 20:14

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request changes the default value of writeBufferManagerAllowStall from true to false in utility/rocksMemoryConfig.ts to prevent writer stalls that can lead to process hangs requiring a restart. The associated unit tests in unitTests/utility/rocksMemoryConfig.test.js have been updated to reflect this new default and to verify fallback behavior for non-boolean values. There are no review comments, so no additional feedback is provided.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.2: merged

Cherry-picked onto v5.2.

@kriszyp
kriszyp marked this pull request as ready for review September 3, 2026 22:06
@kriszyp
kriszyp merged commit 70387f7 into main Sep 3, 2026
73 of 74 checks passed
@kriszyp
kriszyp deleted the fix/wbm-allow-stall-default-false branch September 3, 2026 22:06
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant