Repository navigation
Stop the RocksDB WriteBufferManager from stalling every writer by default - #2492
Merged
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
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.
Contributor
Release cherry-pick
|
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
storage.rocks.writeBufferManagerAllowStallnow defaults tofalse, so the process-wide RocksDBWriteBufferManager— 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 oldtruedefault, reaching the budget parks every writer across every database in the process insideDBImpl::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 : 0in 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_maintainto0only 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 generatedOPTIONSfile:max_write_buffer_number=16,write_buffer_size=16777216, andmax_write_buffer_size_to_maintain=268435456with the stall off versus0with it on — 256 MiB of retained history per column family, so memtable memory can grow toward the sum over families ofmin(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
truedefault is the one that eliminates the window, andfalserestores 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, zeroERR_TRY_AGAINin either arm) and at most ~1.4x attempts per commit when flushes are forced at a cadence comparable to transaction lifetime. Harper already retries bothERR_BUSYandERR_TRY_AGAINinresources/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 istrue, becomesfalse; line 237, thefalse(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 presenttrueas an opt-in hard cap that can wedge every writer in the process (#2490).For the human reviewer
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
maxWriteBufferSizeToMaintainat open. Overruled on a concrete disqualifier — a0target does not surviveTransactionDB::PrepareWrap, which rewrites it to-1and 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: truerestores today's behavior;writeBufferManagerSize: 0disables 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 withcostToCache: truethat 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.The default changes for every deployment on upgrade, silently. Operators who tuned
writeBufferManagerSizeon the assumption of a hard cap get a different memory profile with no config change of their own. Chosen: flip the default rather than leavetrueand have affected deployments opt out, because the deployments that most need the fix are the ones that never touchedstorage.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.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.tsandcrosstable-index-scan-blast-radius.test.ts, whose comments saystorage.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 forcesflush()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).
resolveRocksMemoryConfig's output straight intoRocksDatabase.config()(rocksdb-js 2.8.0 / RocksDB 11.8.1) and thenputSyncs 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).configuredAllowStall: undefined->writeBufferManagerAllowStall: false): 3040putSyncin 30s, worst single call 497ms,RESULT: no stall.true(today's default, everything else identical): blocks inside the first 200 calls and never returns; killed at 90s.OPTIONSfile —max_write_buffer_size_to_maintainis268435456with the stall off and0with it on, atmax_write_buffer_number=16andwrite_buffer_size=16777216.npx mocha unitTests/utility/rocksMemoryConfig.test.js— 15 passing, covering the new default, an explicittrue, an explicitfalse, and a truthy non-boolean that must not coerce into a stall.utility/rocksMemoryConfig.tsrestored toorigin/mainand a cleanrm -rf dist && npm run build, the updated suite goes red — 2 failing, bothAssertionError [ERR_ASSERTION] ... true !== false, at the default case and the non-boolean-fallback case. Restoring the change and rebuilding returns 15 passing.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 definedandupdateAttributesLock > warns exactly once when a successful acquisition waited past the slow-wait thresholdboth pass when re-run alone (they collide with a concurrent mocha run over this box's shared system database), andconfigValidator > does not warn when a relative rootPath resolves within the limitfails in any deep checkout — it resolvesrelative/rootagainstprocess.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