crypto-org-chain / crypto-org-chain/cronos-store

memiavl: fix WAL-durability-ack races without reintroducing the Commit() throughput regression

Open
#95 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Go
Stars
0
Forks
3
PR merge metrics
No merged PRs in 30d

Description

## Background

PR #92 fixed a real bug: `memiavl.DB.Commit()` could return a version's app hash to the caller before that version's WAL entry was actually fsynced. On a crash before the OS flushed the page cache, the entry would be lost even though its hash was already acknowledged to consensus.

The fix worked by making `Commit()` block on the WAL write's `done` channel before returning, while holding `db.mtx` for the whole wait. That's correct, but it collapses the async writer's batching to at most one in-flight entry and removes the WAL-write/fsync overlap the old (unsafe) code got "for free" — a real throughput regression, not just latency. Closing #92 to redo this without giving up throughput.

Two additional real bugs were found in #92's implementation during review (independent of the throughput question, these need fixing regardless of which design we land on):

1. **Masked failure on `Close()`.** The writer signaled failure by blocking on `walQuit <- err` (unbuffered, sent once). Whichever of `Commit`'s two `select`s or `checkAsyncCommit`'s non-blocking check happened to read it first "consumed" the value — so `Close()` calling `waitAsyncCommit()` afterward could see nothing and report success even though the last WAL write failed. Reproduced: `TestCommitFailsWhenWALSyncFails/asyncCommit=true` in #92 was flaky (~50% of runs) on this exact assertion.
2. **Leaked writer goroutine / misattributed error.** If nobody read the blocked `walQuit` send in time, the writer parked forever (goroutine leak), and `walChan`/`walQuit` were never reset — so a *later* `Commit()` call could drain the stale send and report an old failure against a new, unrelated version.

## Proposed fix plan

### Step 1 — fix the two races (low risk, do this regardless of Step 2)

Replace the "send-once on an unbuffered error channel" pattern with: writer stores its terminal error in a plain field, then `close()`s a `chan struct{}` (never sends a value). Closing a channel doesn't block and can be observed by any number of readers, and the write-before-close / read-after-close ordering is a happens-before edge per the Go memory model — so every caller (`Commit`'s selects, `checkAsyncCommit`, `waitAsyncCommit`) sees the identical terminal error, deterministically, no matter who checks first. No parked send, no goroutine leak, no flakiness.

### Step 2 — recover the throughput without reintroducing the original bug

The constraint we must keep: `Commit()` must not return a version's hash before that version's WAL entry is fsynced. The constraint we don't need to keep: `db.mtx` doesn't need to stay held for the *entire* fsync wait — it only needs to prevent a second `Commit()` from mutating tree state concurrently, and consensus already calls `Commit()` strictly one-at-a-time (CometBFT won't start height N+1's execution until `Commit(N)` returns), so there's no concurrent writer to protect against during the wait.

What *is* blocked today, unnecessarily, by holding `mtx` through the fsync wait: read paths that also take `db.mtx` — `Copy()` (used for consistent-snapshot queries), `RewriteSnapshotWithContext`, `Reload`. Releasing `mtx` right after handing the entry off (and resetting `pendingLog`/`cachedPendingChangesets`, which are already safely captured by value in the entry) but before waiting on `done` would let those proceed concurrently with the fsync, instead of queuing behind it.

Caveat to resolve before implementing: this would let a concurrent `Copy()`/query observe version `v`'s state before its WAL entry is confirmed durable. If `Commit()` then fails and the process crashes (as designed — the sole caller panics on any `Commit()` error), any RPC response already served from that pre-durable read is now describing state that never became durable. Need to decide if that's acceptable (arguably yes — it's a narrow, crash-only window, and the alternative is the throughput cost) or worth gating (e.g., don't release `mtx` for `Copy()`/snapshot paths specifically, only for others) before landing.

### Step 0 — before writing more code, measure

Add a benchmark (`memiavl/db_bench_test.go`) for `Commit()` with `walSync` doing a real fsync, sync vs async mode, on representative storage. If per-commit fsync latency is a small fraction of the chain's actual block interval, the "regression" may not be worth the added complexity of Step 2 at all — land Step 1 alone and close this out. Only pursue Step 2 if the benchmark shows a material hit.

### Cleanup (found during review, not urgent, bundle in whichever PR lands)

- `checkBackgroundSnapshotRewrite`'s busy-wait loop (`time.Sleep(time.Nanosecond)` waiting for `CommittedVersion() == lastCommitInfo.Version`) is now dead weight once `Commit()` always waits for its own entry — the condition is true on the first check, always. Misleading to leave in.
- `CommittedVersion()` is exported but doesn't take `db.mtx`; currently only called internally already under the lock, so no live bug, but it reads `db.wal.LastIndex()` unguarded and would race against the writer if ever called externally while a commit is in flight.
- `DB.Commit()`'s doc comment should state explicitly that any returned error is fatal and must not be retried/ignored by the caller — the in-memory tree version is already advanced before the WAL write, so continuing after an error would run the tree ahead of durable disk state. Right now that contract is enforced only by the single caller (`store/rootmulti/store.go`) happening to panic; it should be documented as the actual contract, not an implementation detail of the one caller that exists today.

## Non-goal

`WorkingHash()` (`store/rootmulti/store.go`) computes the app hash from the working tree *before* `Commit()` runs at all — that's the hash ABCI 2.0 actually uses for `FinalizeBlock`/consensus. This is standard ABCI 2.0 separation (CometBFT replays `FinalizeBlock` on restart if `Commit` never ran) and out of scope here; noting it so it's not mistaken for something this issue needs to fix.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the async Commit/WAL paths and the tests from PR #92, then add the proposed benchmark in memiavl/db_bench_test.go using real fsync in sync and async modes. Review Copy(), RewriteSnapshotWithContext, Reload, and store/rootmulti/store.go to assess the locking and fatal-error contract. Done means the races and error propagation are deterministic, and any throughput change is supported by benchmark results.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.