fix: add backpressure to unfinalized block write queue - #8885
Conversation
Add waitForSpace() to JobItemQueue that resolves immediately when space is available, or awaits until a running job completes and frees a slot. Use waitForSpace() in importBlock before pushing to the unfinalized block write queue. This applies backpressure during sync, preventing supernodes (128 custody groups) from accumulating 30+ blocks of column data in memory when persistence can't keep up, which causes OOM. Ref: ChainSafe#8784 (comment) Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
Summary of ChangesHello @lodekeeper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a crucial backpressure mechanism within the block import process to mitigate out-of-memory (OOM) issues observed in supernodes during finalized synchronization. By introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a backpressure mechanism to the unfinalized block write queue, which is a crucial improvement to prevent out-of-memory issues during sync. The approach of adding waitForSpace() to the JobItemQueue is sound. However, I've identified a couple of issues in the implementation of JobItemQueue: a potential memory leak due to an un-removed event listener, and a critical issue that could lead to deadlocks because of the timing of waiter notifications. My review includes suggestions to address these points.
When waitForSpace() resolves normally via notifySpaceWaiters(), the abort event listener was not being removed, causing a memory leak for each call. Wrap the resolve callback to remove the abort listener on success.
- Add settled guard to prevent double-settle race between resolve and abort - Compute available slots once in notifySpaceWaiters to wake only as many waiters as there are free slots (prevents thundering herd) - wrappedResolve removes abort listener on normal resolve (prevents leak) - Add test: multiple waiters with single slot only wakes one
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
can you reduce the queue size to something conservative in this PR? eg 16 or so?
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a backpressure mechanism to the unfinalized block write queue, which is crucial for preventing out-of-memory errors during sync, especially for supernodes. The implementation of "waitForSpace()" in "JobItemQueue" and its integration into "importBlock.ts" effectively addresses the identified memory growth issue. The accompanying unit tests for "waitForSpace()" are thorough, covering immediate resolution, blocking behavior, abort handling, and the "thundering herd" scenario, ensuring the robustness of the new feature. The changes are well-motivated and directly solve a critical performance and stability problem.
Reduce the maximum pending unfinalized block writes from 32 to 16 as a conservative measure to limit memory usage during sync, especially for supernodes with 128 custody columns.
|
Done — reduced to 16 in 5bdc431. With backpressure in place, the queue size is now more about batching efficiency than memory safety (since |
nflaig
changed the title
Re-check signal.aborted after attaching the abort listener to handle the case where abort fires between the initial check and addEventListener. Without this, the promise could hang forever if abort() is called in that narrow window. Co-authored-by: GPT-5.2 (gpt-advisor sub-agent)
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f609f13b19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ALIZED_BLOCK_WRITES
With waitForSpace() backpressure in place, the queue size controls batching/throughput rather than memory safety. A smaller queue is more conservative while the backpressure mechanism prevents unbounded growth regardless. MAX_BLOCK_INPUT_CACHE_SIZE stays at 16 to comfortably cover the queue depth plus network-arrived blocks not yet imported.
|
@lodekeeper I pushed some changes, please review |
|
LGTM 👍 — the tweaks look good. Queue at 8 with cache at 16 gives comfortable headroom. Comments are much clearer about the invariant. |
|
Co-authored-by: Matthew Keil <github@mail.matthewkeil.com>
|
Reviewed the latest changes — all look great 👍
Queue back to 16 — Makes sense. With backpressure the queue depth is about throughput batching, and 16 gives good headroom during sync. Comment updates are clearer about the relationship between cache, write queue, and memory. |
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
looks good to me, managed to sync a node from epoch 426704 to 426907 (head) without any errors or memory issues
matthewkeil
left a comment
matthewkeil
left a comment
There was a problem hiding this comment.
LGTM 🚀 other than maybe using the const value
| // Target size for the block input cache, enforced by pruneToMaxSize() which runs after prune() | ||
| // and onFinalized() — NOT on insertion. The cache can temporarily exceed this during range sync | ||
| // (e.g. 32 blocks inserted per batch) but is trimmed back after blocks are processed. | ||
| // | ||
| // Must be large enough to hold blocks from all concurrently downloaded range sync batches. | ||
| // Range sync downloads up to MAX_LOOK_AHEAD_EPOCHS (2) batches ahead of the processing head, | ||
| // so up to 3 batches (current + 2 look-ahead) of SLOTS_PER_EPOCH blocks can be in the cache | ||
| // simultaneously. If this value is too small, pruneToMaxSize() will evict blocks from the | ||
| // batch being processed before they are persisted to the database, causing errors when | ||
| // async handlers like onForkChoiceFinalized run. |
There was a problem hiding this comment.
This is the right approach to set this value. Can we use MAX_LOOK_AHEAD_EPOCHS so if we change that this also changes? Overall this a good approach.
Point to note is at 3 epochs with 21 blobs its 600ish MB. Should be ok at startup when not much else is in memory. Might become an issue during periods of non-finality when there is substantial memory load. Food for thought...
There was a problem hiding this comment.
Good call — using MAX_LOOK_AHEAD_EPOCHS directly makes this self-updating if that value ever changes. Should be:
import {MAX_LOOK_AHEAD_EPOCHS} from "../../sync/constants.js";
const MAX_BLOCK_INPUT_CACHE_SIZE = (MAX_LOOK_AHEAD_EPOCHS + 1) * SLOTS_PER_EPOCH;The +1 accounts for the current batch being processed (the 3 = current + 2 look-ahead).
On the memory note — yeah, 96 slots × 21 blobs × ~128KB ≈ 250MB (blobs are ~128KB not ~3MB since we store the raw 128KB field elements, not the full 4096-element commitments). Still non-trivial but more manageable. During non-finality the concern is real though — the cache won't prune past the finalized checkpoint, so it could grow beyond the max. Worth keeping an eye on but probably OK for this PR since the pruning behavior is unchanged.
There was a problem hiding this comment.
@lodekeeper quick thing if you want to touch this up
There was a problem hiding this comment.
Done — pushed the change to use MAX_LOOK_AHEAD_EPOCHS: f79fd26
import {MAX_LOOK_AHEAD_EPOCHS} from "../../sync/constants.js";
const MAX_BLOCK_INPUT_CACHE_SIZE = (MAX_LOOK_AHEAD_EPOCHS + 1) * SLOTS_PER_EPOCH;Use the MAX_LOOK_AHEAD_EPOCHS constant instead of hardcoding 3, so the cache size stays in sync if look-ahead changes.
matthewkeil
left a comment
matthewkeil
left a comment
There was a problem hiding this comment.
LGTM!!
Thanks @lodekeeper you are the 🐐
|
@lodekeeper please resolve merge conflicts |
|
Merge conflicts resolved — combined the backpressure await this.unfinalizedBlockWrites.waitForSpace();
this.unfinalizedBlockWrites.push([blockInput]).catch((e) => {
if (!isQueueErrorAborted(e)) {
this.logger.error("Error pushing block to unfinalized write queue", {slot: blockSlot}, e as Error);
}
}); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8885 +/- ##
============================================
- Coverage 52.35% 52.34% -0.01%
============================================
Files 848 848
Lines 63473 63471 -2
Branches 4704 4704
============================================
- Hits 33229 33227 -2
Misses 30175 30175
Partials 69 69 🚀 New features to boost your workflow:
|
|
🎉 This PR is included in v1.40.0 🎉 |
4 participants
Motivation
During finalized sync, supernodes with 128 custody groups can accumulate 30+ blocks of column data in memory when the async write queue can't persist fast enough. Each block holds 128 column sidecars, and with a queue depth of 32, this can consume ~10GB+ of external memory causing OOM kills and crash loops.
This was observed on
beta-mainnet-superduring v1.40.0 RC testing — the node repeatedly crashed with no error logs (OOM kill signature), with external memory spiking from ~4.5GB to ~10GB before each crash.Ref: #8784 (comment)
Description
waitForSpace()method toJobItemQueuethat resolves immediately when the queue has capacity, or awaits until a running job completes and frees a slotwaitForSpace()inimportBlockbefore pushing to the unfinalized block write queue, applying backpressure during syncwaitForSpace()is a noop — no performance impactChanges
packages/beacon-node/src/util/queue/itemQueue.ts— AddwaitForSpace()method andnotifySpaceWaiters()hook after job completionpackages/beacon-node/src/chain/blocks/importBlock.ts— Await queue space before pushing block inputpackages/beacon-node/test/unit/util/queue.test.ts— Add 3 tests: immediate resolve, blocking until space, abort handlingAI Assistance Disclosure
This PR was authored by Lodekeeper (AI assistant) with review from GPT-5.2 and Gemini sub-agents.
Co-authored-by: lodekeeper lodekeeper@users.noreply.github.com