FlatKV: background read/fold for account data - #4155
Conversation
cody-littley
changed the title
PR SummaryHigh Risk Overview FlatKV stops batch-reading account rows on the block-apply thread. Partial nonce/codehash/balance changes are parsed up front, then Coverage includes dedicated batch-update tests, differential fuzz with Reviewed by Cursor Bugbot for commit 4ec4270. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4155 +/- ##
==========================================
- Coverage 66.69% 65.91% -0.78%
==========================================
Files 2202 2105 -97
Lines 169563 160874 -8689
==========================================
- Hits 113087 106044 -7043
+ Misses 56335 54689 -1646
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d38fe4d. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving the account read/fold off the execution thread is well structured — staging under the shard lock, pending-value handles checked on every read path, and per-version latches that gate the diff consumers — and the new tests (including the differential opUpdate op) cover the staging/chaining/delete/failure semantics well. The blocking gap is lifecycle: the folds are fire-and-forget work that nothing drains, so Close can tear down the read pool and the database underneath an in-flight fold.
Findings: 1 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
ApplyChangeSetsno longer detects a corrupt stored account row —newAccountUpdateronly parses the changeset, and deserialization of the row being folded onto now happens on a fold worker. The block is therefore accepted and the failure resurfaces later as a bricked manager (and a panic out ofCommitStore.Get, asTestCrashRecoveryCorruptedAccountValueInDBnow records) rather than as an error attributable to the apply. The node still halts before publishing a hash, so this is a change in failure surface rather than a correctness hole, but it is worth calling out explicitly in theCommitStore.ApplyChangeSetscontract so operators reading a fold-failure brick know which block produced it. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
|
@seidroid review |
There was a problem hiding this comment.
Moving the account fold off the execution thread is carefully built: staging under the shard lock, pending handles checked on every read surface, per-version latches gating the diff consumers, and a Close drain that closes the lifecycle gap from the previous review. Remaining notes are non-blocking: a residual shutdown window after a brick, an unenforced no-duplicate-keys contract that turns into a hang, and the fold's latency no longer being measured anywhere.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
CommitStore.ApplyChangeSets's contract still describes the old failure surface. A corrupt stored account row used to fail the apply withunsupported serialization versionattributable to the block that read it; now the apply succeeds and the failure resurfaces as a bricked manager atCommit(or a panic out ofCommitStore.Get, asTestCrashRecoveryCorruptedAccountValueInDBnow records). The node still halts before publishing a hash, so this is a change in failure surface rather than a correctness hole, but theApplyChangeSetsgodoc is where an operator would look to map a fold-failure brick back to a block. This was raised on the previous review and the code still does not state it anywhere. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Moving the account fold off the execution thread remains well built — staging under the shard lock, pending handles on every read surface, per-version latches gating the diff consumers, and a Close drain — and the head commit is unchanged since the last review, so the three previously unresolved suggestions still stand. No blockers; the notes are a duplicate-key hang, a drain whose godoc over-claims on the brick path, a metrics blind spot, and some comments left stale by the removed account reads.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Two comments in
store_apply.gowere left describing account reads this PR removed.applyChangeSets's lock rationale (line 36) still says the body "reads old values out of the stores and writes this block's values into them", andwriteToStores's godoc (line 234) still says "The writes must come after the account reads in prepareWrites, because writing here is what makes this block's values visible to a read through the same store."prepareWritesno longer reads any account, so that ordering constraint no longer exists and the next reader will look for a read that isn't there. Rewrite both rather than patching them. - [suggestion]
CommitStore.ApplyChangeSets's contract still describes the old failure surface. A corrupt stored account row used to fail the apply withunsupported serialization versionattributable to the block that read it; now the apply succeeds and the failure resurfaces as a bricked manager atCommit(or a panic out ofCommitStore.Get, asTestCrashRecoveryCorruptedAccountValueInDBnow records). The node still halts before publishing a hash, so this is a change in failure surface rather than a correctness hole, but theApplyChangeSetsgodoc is where an operator would look to map a fold-failure brick back to a block. Raised on both previous reviews; the code still does not state it anywhere. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
readCache.ResolveBatchUnlocked(sei-db/db_engine/view/read_cache.go:368-375) abandons its drain on context cancellation without waiting for the reads it already submitted toreadPool. This predates the PR — an ordinaryBatchGetracing a brick hits the same window — and the inline note onawaitOutstandingFoldsis about the new fold path inheriting it.
| if len(work[shardIndex]) == 0 { | ||
| continue | ||
| } | ||
| folds, err := c.shards[shardIndex].StageUpdates(keys, work[shardIndex], version) |
There was a problem hiding this comment.
[suggestion] BatchUpdate's contract says "keys must not repeat", but nothing enforces it and a violation is unrecoverable rather than an error.
With keys = [k, k], both indices land in the same shard batch. StageUpdates stages P0 for the first; capturePriorValueWLocked then sees P0 as the newest entry and gives the second priorValueInEarlierFold{P0}. FoldStagedValues awaits P0 inside resolvePriorValuesUnlocked — the very call that would resolve it — so the fold never completes. The latch never opens, Commit/flush block in awaitVersionFoldsUnlocked, and Close blocks forever in AwaitOutstandingFolds, whose wait is deliberately not interruptible (and which runs before cancel(), so nothing wakes the parked await either).
The only caller today derives keys from a map, so it cannot produce a duplicate — but that is a convention the next caller can forget, where a check at the single function every path passes through is an invariant they cannot. A dedup check in StageUpdates (or in partitionIndicesByShard, which already walks the slice) returning an error would turn an unrecoverable hang into a failed call.
Still present at head; no commits since the previous review.
| // closes once Close returns. Close calls this before cancelling, so that a fold in flight resolves | ||
| // against an open database instead of abandoning a read that would then race db.Close, and outside | ||
| // versionLock, which a failing fold takes to brick the manager. | ||
| func (c *viewManager) awaitOutstandingFolds() { |
There was a problem hiding this comment.
[suggestion] The drain closes the clean-shutdown case, but the godoc's guarantee — "a fold in flight resolves against an open database instead of abandoning a read that would then race db.Close" — does not hold once the manager has bricked, because bricking cancels the context the fold's read is parked on.
With two folds in flight: fold A fails, FailStagedFolds releases A's latch and reportFoldFailure → brick → cancel(). Fold B is inside readCache.ResolveBatchUnlocked, parked in InterruptiblePull; the cancel makes it bail without draining the reads it already submitted (read_cache.go:368-375). B then calls FailStagedFolds, releasing its latch, so awaitOutstandingFolds sees nothing outstanding and returns while B's readPool task is still queued or inside readFromDB.
Scoping this down from the previous review: in the production teardown this is contained, because CommitStore.Close closes readPool (a fixedPool, whose Close drains the queue and waits) before closeDBsOnly, and the abandoned task's valueChan is buffered so it cannot wedge a worker. So the read-after-close does not actually occur through that path. What is left is that this function's stated invariant is stronger than what it delivers, and it only holds because of an ordering the manager does not own. Either narrow the comment to the clean-shutdown case and name the readPool-before-DB dependency, or drain the fold-owned read tasks themselves rather than the latches, which a failure releases early.
| if alreadyHave[accountDBDir] < version && prepared.accounts != nil { | ||
| start := time.Now() | ||
| err := s.accountStore.BatchUpdate(prepared.accounts.keys, prepared.accounts) | ||
| otelMetrics.AccountUpdateLatency.Record(s.ctx, secondsSince(start), |
There was a problem hiding this comment.
[suggestion] AccountUpdateLatency now times accountStore.BatchUpdate, which by design does no I/O and no folding — it is one lock hold per shard. The work this PR moved (the prior-value reads plus the merge) is no longer measured by anything: flatkv_batch_read_old_values_latency covered the account reads and is removed here.
That leaves a blind spot exactly where the new risk is. A fold that stalls on a slow database read now delays Commit (via awaitVersionFoldsUnlocked) and any read of a touched account, but the only visible symptom would be CommitLatency rising with no metric explaining why. Consider recording the fold duration where FoldStagedValues completes, or keeping a read-latency histogram on the prior-value path, so the moved cost stays attributable.
2 participants
Describe your changes and provide context
Move account read/fold workflow off of the execution thread.