HarperFast / HarperFast/rocksdb-js

WriteBufferManager stall: runtime allowStall mutation + late-CF size-0 bypass the open-time history clamp

Open
#781 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
21
Forks
2
Avg merge
2d 9h
Merged PRs (30d)
36

Description

## Summary

A Codex review on [#758](https://github.com/HarperFast/rocksdb-js/pull/758) (raised by @kriszyp) surfaced two concerns in the **write-buffer-manager / retained-history** feature (AGENTS.md invariant 10 — "Retained memtable history must fit the WriteBufferManager budget"). They are unrelated to that PR's background-error change (it only inherits these files via a `main` merge), so filing them here to track separately.

## 1. Runtime `writeBufferManagerAllowStall` mutation bypasses the open-time history clamp

`src/binding/database/db_settings.cpp` (~L209)

`buildColumnFamilyOptions` resolves the derived (`-1`) `max_write_buffer_size_to_maintain` to `0` **only when a stalling manager is configured at open time** (`resolveMaxWriteBufferSizeToMaintain`). A database can attach a WriteBufferManager while `allowStall` is `false`, so its column families keep RocksDB's derived (large) history target. Flipping `writeBufferManagerAllowStall` to `true` at runtime then enables stalling on that same manager **without revisiting those already-resolved CF targets**. With a budget below the retained history, every write to that database can stall permanently — the exact failure invariant 10 exists to prevent.

Related size-`0` mismatch for late-created families: an existing DB retains the manager, while `resolveMaxWriteBufferSizeToMaintain()` consults the *global* size and may re-derive a large history for a CF created after the global size was reset to `0`.

**Suggested direction:** enforce the invariant against the manager actually attached to each descriptor — either make `allowStall` immutable after attachment, or safely update/reject incompatible CF `max_write_buffer_size_to_maintain` targets before enabling stall.

**Coverage gap:** add tests for (a) enabling stall *after* open, and (b) creating a CF after the global size is reset to `0`.

## 2. Stall regression test can hang instead of failing

`test/write-buffer-manager-stall.test.ts` (~L41)

`db.put()` is declared `async` but calls `store.putSync()` before returning its promise. If RocksDB stalls inside that native call, the JS worker thread is blocked and Vitest's 60s timeout can't fire — the regression hangs until the outer CI job timeout rather than failing this test.

**Suggested direction:** run the stall scenario in a worker/child process the parent can terminate on a deadline, or drive it through a genuinely asynchronous native operation so the runner can still enforce its timeout.

---

Reported by @kriszyp's Codex review on #758; filed as a scoped follow-up against the write-buffer-manager work.

Contributor guide

Open the contributing guide

Research direction

Read AGENTS.md invariant 10, then inspect src/binding/database/db_settings.cpp around buildColumnFamilyOptions and resolveMaxWriteBufferSizeToMaintain. Run test/write-buffer-manager-stall.test.ts and trace the db.put() helper around its store.putSync() call. Done means the two reported mutation and late-CF cases are covered without permanent stalls, and the regression test fails within the test runner's deadline.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, nodejs, typescript
Domain
backend, databases, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.