execution/commitment: fix wave-BFS livelock when a step budget fills exactly - #23066
Conversation
…exactly ContractTrunkPreloadParallel.Run could spin forever inside its wave loop. Once fileBudget (stepCap - usedBytes - dbHitsBytes) reached zero, the whole file-miss set was deferred and fileMiss set to nil, so no pin ran for that wave. budgetHit is only raised from inside pin() on a strict overflow, so it stayed false, the loop did not break, and p.frontier = slices.Clone(fileMissDeferred) reassigned the same frontier at the same depth with usedBytes unchanged — every loop variable identical on re-entry. usedBytes lands exactly on stepCap because pin() admits an entry when usedBytes+cost == stepCap. AdaptivePinController.OnBlockComplete makes that reachable: as a contract approaches PerContractMaxBudgetBytes, step collapses onto the remaining bytes and stepCap onto that fixed cap. When a wave then fills the cap exactly and the following frontier misses dbBranches entirely, Run never returns. Observed on a Gnosis validator (release/3.6, 4402a1d): an FCU wedged in SharedDomains.Commit held the execution semaphore, so NewPayload blocked while holding the fork-choice write mutex. The CL slot ticker and ~420 getAttesterDuties readers piled up behind it, and the node stopped attesting and producing for 17 hours while burning a core in sortAndPartitionFrontier. Treat a wave with no room for a single file entry as a budget hit. Each iteration now either breaks, strictly shrinks the frontier, or increments nextDepth, so the loop always terminates; the deferred set stays in p.frontier and resumes on the next Run.
There was a problem hiding this comment.
Pull request overview
Fixes a livelock in execution/commitment where ContractTrunkPreloadParallel.Run can spin forever when a wave exactly fills stepCap and the next wave has only file misses with zero remaining file budget. This prevents FCU/commit paths from wedging the node under certain adaptive pin budgeting conditions.
Changes:
- Treat “no room for even one file entry” as a budget hit, forcing the wave loop to terminate and defer work to the next
Run. - Add regression tests covering the exact-budget-fill livelock scenario and a step-budget sweep to guard termination across edge budgets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| execution/commitment/preload_parallel.go | Adds a noFileBudget guard to convert full deferral at zero file budget into a budgetHit, guaranteeing wave-loop termination. |
| execution/commitment/preload_parallel_test.go | Adds two regression tests to reproduce the hang scenario and validate termination across multiple step budgets. |
Suppressed comments (1)
execution/commitment/preload_parallel_test.go:923
- Same issue as above: on timeout the test fails but the goroutine running the sweep can remain stuck inside Run and continue consuming CPU. Using panic here avoids leaving a runaway goroutine alive after the failure is signaled.
select {
case out = <-results:
case <-time.After(30 * time.Second):
t.Fatal("a Run did not terminate: a wave with no file budget must end the step, not re-enter on an unchanged frontier")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Abort the test binary when Run is stuck instead of only failing the test. Run
has no cancellation, so on a regression the spinning goroutine would keep
burning a core for the rest of the package run and time out unrelated tests.
SetTraceback("all") widens the dump so the spinning stack, not just the waiter,
lands in the failure output.
Derive the guaranteed-progress step budget from the costliest entry in the
synthetic tree instead of scaling the root's cost by a constant, so the
assertion survives changes to the entry-cost overhead.
Correct the sweep test's doc comment to state the invariant actually asserted:
a budget covering the costliest entry must finish the tree, not any budget that
can afford the root.
|
Thanks — all three Copilot comments taken, pushed in 5be5ef7. Runaway goroutine on timeout (both tests). Agreed, and it is specific to this failure mode: I also added
affordable := 0
for path, afterMap := range tree {
if cost := estimatedEntryCost(nibbles.HexToCompact([]byte(path)), branchVal(afterMap, valSz)); cost > affordable {
affordable = cost
}
}That is also the tight threshold rather than a padded one: a step budget at least the costliest entry can always pin at least one entry per step, so progress is guaranteed. The old Doc comment disagreed with the assertions. Correct — the comment claimed any budget that can afford the root must finish the tree, but depth-66+ entries cost more than the depth-64 root (34-byte compact keys vs 33), so a root-sized budget legitimately stalls. Reworded to state the invariant the test actually checks. Re-verified after the changes: full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
execution/commitment/preload_parallel_test.go:799
- panicOnStuck is called from the test goroutine, so the panic will be recovered by the testing framework. That means it will not abort the test process and
debug.SetTraceback("all")will not produce an all-goroutine dump as intended, leaving the runawayRungoroutine spinning and potentially timing out unrelated tests anyway. To actually crash the process (and get the full goroutine dump), trigger the panic from a separate goroutine and block this one.
// panicOnStuck aborts the test binary rather than just failing the test. Run has
// no cancellation, so a livelock regression would leave a goroutine spinning on a
// core for the rest of the package run and time out unrelated tests. The
// traceback setting widens the dump to every goroutine so the spinning one, not
// just this waiter, shows up in the failure output.
func panicOnStuck(why string) {
debug.SetTraceback("all")
panic("ContractTrunkPreloadParallel.Run did not terminate: " + why)
}
…ed wave The guard ended a step only when the whole miss set was deferred. A capped fetch whose keys are all absent from the file layer pins nothing either, so that wave re-entered at the same depth with the same budget, re-sorting and re-cloning the frontier once per remaining entry. Also check cache in the PreloadContractTrunkParallel wrapper (Run returns the error, then the logger block dereferences it), close the BranchCaches the new tests build (NewBranchCache bumps a live-instance counter that adaptiveTrunkDepth reads), and add coverage for a wave that defers misses while pinning db-hits.
|
Reviewed. The fix is right; pushed four follow-ups to the branch (4e47f07). Differential check on the fix. Ran this branch's Pushed:
Separate issue, outside this diff —
|
|
Thanks @awskii — reviewed all four changes in 4e47f07, they all hold up. Re-verified locally: The widened guard is a strict superset of the original (the old Two notes. The new test doesn't cover the clause it's named for.
So it pins the pair, not the widening — either change alone satisfies it. It's a one-byte budget choice. At One more byte enters it: - stepBudget := estimatedEntryCost(rootKey, branchVal(0xffff, valSz)) + minEntryBytes - 1
+ stepBudget := estimatedEntryCost(rootKey, branchVal(0xffff, valSz)) + minEntryBytesMeasured at that budget (16-wide wave,
which is the re-entry-per-chunk behaviour the test's Separately, Minor: Happy either way on the test budget — say the word and I'll push it, or take it yourself if you'd rather keep the follow-ups in one hand. Agreed the |
The no-pin test only reached the full-deferral branch, so reverting either the minEntryBytes floor or the widened guard alone left the package green. Split it into two budgets one byte apart and assert the exact resolver call count. Fold the two queueEmpty expressions into one method: the zero-budget early return omitted the depth-ceiling clause. Hoist the wrapper's resolve check up with the other argument guards.
|
Took this myself in 03a83f4 rather than leave it hanging — split
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
execution/commitment/preload_parallel.go:277
budgetHitis now used both for true budget exhaustion (insidepin) and for the no-progress early-termination guard (budgetHit = truehere). That makes the name misleading and can confuse future changes (e.g., distinguishing “budget exhausted” vs “stopped to avoid livelock”). Consider renaming the flag to something likestopStep/terminateStep, or splitting it intobudgetExhaustedandterminateStepbooleans.
if len(fileMissDeferred) > 0 && (len(fileMiss) == 0 || chunkPinned == wavePinnedBefore) {
budgetHit = true
}
The flag ends a step for two reasons now: pin() overflowing the budget, and a wave making no progress. Name it for the effect — no read site distinguishes the cause.
|
Took Copilot's naming note in 810377d: |
awskii
left a comment
awskii
left a comment
There was a problem hiding this comment.
Inline notes on the fix and test coverage.
| p, err := NewContractTrunkPreloadParallel(contractHash) | ||
| if err != nil { | ||
| return 0, err | ||
| if cache == nil { |
There was a problem hiding this comment.
This guard is what's actually fixing a real bug, worth calling out: pre-fix, calling this wrapper with cache=nil and a non-nil logger reached the unconditional logger.Info(...cache.PinnedCount()) below (line 371) and panicked — p.Run returns an error on nil cache, but nothing here checked that error before logging. No test exercises the wrapper with cache=nil, logger!=nil: TestContractTrunkPreloadParallel_NilCacheError only calls p.Run directly with logger=nil. Worth a regression test at the wrapper level for this specific fix.
| endStep = true | ||
| } | ||
|
|
||
| if endStep { |
There was a problem hiding this comment.
A pathKey that was a dbBranches hit but gets deferred into rest/p.frontier here loses that provenance. If a later Run call's dbBranches overlay rotates past it and the file resolver also misses, sortAndPartitionFrontier reclassifies it as fileMiss, resolve() returns nil, and it's dropped silently — indistinguishable from a genuine BFS-fringe absence. Pre-existing classification behavior, but this PR is what makes multi-call resumption of budget-deferred waves routine instead of hanging, so it's now reachable in practice (adaptive_pin.go's runExtensionLocked holds a persistent preload instance across blocks with a fresh overlay each call). No test covers a dbHit surviving a deferral into a call with a changed overlay.
| // db-hits, so the resumed frontier is a concatenation (unpinned db-hits, then | ||
| // the deferred misses) at the current depth while pendingChildren already holds | ||
| // the pinned hits' children one level down. | ||
| func TestContractTrunkPreloadParallel_DeferWithDbHitsInSameWave(t *testing.T) { |
There was a problem hiding this comment.
Ran this against the pre-PR code directly — it passes unchanged, so it isn't actually covering this PR's fix. Its budget (rootCost+r1Cost+minEntryBytes-1) makes R1 fit and R2 overflow inside the fileMiss loop itself, a shape the old code already handled via fileMissStop's in-loop assignment. Also, unlike its three siblings (ExactBudgetFillTerminates/StepBudgetSweepTerminates/NoPinWaveEndsStep), it calls p.Run synchronously with no panicOnStuck timeout guard — a livelock regression specific to this db-hit+deferred-miss shape would hang the whole test binary instead of failing fast.
| // entry, so the miss set is deferred without being fetched, or it affords a | ||
| // capped fetch whose keys are all absent from the file layer — the normal shape | ||
| // at the BFS fringe, where a set afterMap bit names a leaf with no branch | ||
| // record. Neither raises endStep from pin(), so the wave used to be re-entered |
There was a problem hiding this comment.
Traced both subtests against pre-PR code: neither actually hangs here (frontier strictly shrinks each inner iteration) — they terminate, but with the wrong result (queueEmpty=true and 2/9 resolver calls instead of 1/2) after grinding through the whole wave in one Run call. "the wave used to be re-entered ... once per chunk" reads as hang narration for something that doesn't hang at this test's scale — better to describe the wrong outcome (call count, queueEmpty) that the assertions actually pin down.
| // branch (33 = shortest HexToCompact key at depth >= 64; value may be empty). | ||
| // Bounds a wave's file fetch so the budget is guaranteed exhausted inside it. | ||
| // Caps a wave's file fetch to what the remaining budget could still pin. Keys | ||
| // absent from the file layer cost nothing, so a capped fetch may leave the |
There was a problem hiding this comment.
This rationale (capped fetch may leave budget unspent; the no-progress check is what ends the step) is restated in full again at the check itself (lines 270-277), with nothing tying the two together. Worth keeping one canonical explanation and a short pointer at the other site so a future change to the deferral threshold or no-progress condition doesn't leave one stale.
| } | ||
| if stepBudgetBytes <= 0 { | ||
| return 0, len(p.frontier) == 0, nil | ||
| return 0, p.queueEmpty(), nil |
There was a problem hiding this comment.
Extracting queueEmpty() also changes what this early return reports when stepBudgetBytes<=0 — from len(p.frontier)==0 to also checking pendingChildren and the depth ceiling. The two can only disagree when frontier is non-empty, pendingChildren is empty, and nextDepth > maxStorageTrunkDepth all at once — looks unreachable given how these three are always mutated together, but no test calls Run with stepBudgetBytes<=0 and non-empty pendingChildren to pin that down, so the behavior change rides along inside what otherwise reads as a pure refactor.
| pinned int | ||
| err error | ||
| } | ||
| results := make(chan []sweepResult, 1) |
There was a problem hiding this comment.
All 7 budgets in this sweep run inside one goroutine behind one generic panicOnStuck message. If a regression livelocks only one specific budget (e.g. exact+1), the panic text won't say which — unlike NoPinWaveEndsStep's t.Run(tc.name, ...) plus isolated timeout, which would name the failing case directly.
DeferWithDbHitsInSameWave passed against pre-PR code — deferring a miss unfetched and fetching it only to fail the pin leave an identical frontier, so only the resolver call count separates them. Assert it, and add the hang guard its siblings have. Cover the two untested cases: the wrapper's nil-cache guard, which panicked via the logger before returning Run's error, and the depth ceiling, where the zero-budget early return used to disagree with a spendable one. Sweep budgets become subtests so a regression names the failing budget. Drop the minEntryBytes rationale that was restated in full at the no-progress check.
|
Thanks @awskii — all seven taken or answered, pushed in 42c4aab. Your test doesn't cover the fix — confirmed. Reverted
The budget was already right; the gap was the assertions. Deferring the miss unfetched and fetching it only to fail the pin leave an identical frontier, Wrapper nil-cache. Added
Duplicated rationale — agreed, and it is against the repo comment policy. The Sweep guard — each budget is a Wording — reworded; it now describes the wrong outcome (call count, dbHit provenance — real, and your reasoning holds. Filed as #23143 rather than folded in here: the impact is a
|
…nters (erigontech#23067) ## Problem `commitment_trunk_preload_duration_seconds_total` and `commitment_trunk_preload_bytes_total` are declared in `trunk_pin_metrics.go` but never written anywhere in the tree, so both read `0` for the life of the process. Confirmed on a live Gnosis node with 5.9 days of uptime and 68,732 recorded preload extensions: ``` commitment_adaptive_pin_extended_total 68732 commitment_adaptive_pin_promoted_total 5137 commitment_branchcache_pinned_entries 118337 commitment_trunk_preload_bytes_total 0 <-- never written commitment_trunk_preload_duration_seconds_total 0 <-- never written ``` The neighbouring counters work; only these two are dead. The result is that there is no metric signal for how much work the adaptive pin controller does or how long it spends doing it. That gap was noticed while diagnosing erigontech#23066 — a livelock inside `ContractTrunkPreloadParallel.Run` — where these counters would have been the natural place to see preload time climbing. ## Change Record both at the two places a preload actually runs, covering the parallel and serial paths in each: - `promoteLocked` — the initial view for a newly promoted contract. - `runExtensionLocked` — the per-block extension step, which is the dominant path in a running node. A promote whose `Run` fails has its pins rolled back via `Invalidate`, so it contributes its duration but no bytes. That is why `recordPreload` takes the byte count as a parameter instead of reading it back off the preloader. No behaviour change beyond the counters; no new metric names, cardinality, or hot-path work (both call sites already run once per contract per block under `c.mu`). ## Testing TDD, red → green. Both tests fail on `main` with the counters flat at `0`: - `TestAdaptivePin_PromoteRecordsPreloadMetrics` — both counters advance across a promote. - `TestAdaptivePin_ExtendRecordsPreloadMetrics` — both counters advance across an extension, with the initial view budgeted so the queue survives promotion and there is real work left to measure. Each test guards against a vacuous pass by first asserting the preload actually pinned bytes / left a non-empty queue. Full `execution/commitment/...` suite passes. `make lint` clean for the touched files. ## Relationship to erigontech#23066 Split out of erigontech#23066 review discussion to keep that fix minimal. The two branches touch disjoint files (`adaptive_pin.go` / `trunk_pin_metrics.go` here, `preload_parallel.go` there). Verified by trial-merging the two branches: no conflicts, merged result builds and the full package suite passes. They can land in either order. --------- Co-authored-by: Alex Sharov <AskAlexSharov@gmail.com>
Problem
ContractTrunkPreloadParallel.Runcan spin forever in its wave loop, wedging the node.The loop only breaks on
budgetHit, whichpin()raises on a strict overflow. A wave that pins nothing never raises it, and the frontier is reassigned at the same depth — identical state on re-entry. Two ways a wave pins nothing:fileBudgeton re-entry isstepCap - (usedBytes + dbHitsBytes), bit-identical to the value that caused the deferral, because the db-hits consume exactly the bytes the deferral already accounted for. Infinite.afterMapbit names a leaf with no branch record. Re-enters once per chunk: bounded, but quadratic.Impact
Hit in production on a Gnosis validator (
release/3.6, 4402a1d):The stuck goroutine holds the execution semaphore inside an FCU, so
NewPayloadblocks while holding the fork-choice write mutex. The CL slot ticker stops and ~420getAttesterDutieshandlers pile up on the read lock. The node stopped attesting and producing for 17h while burning a core;eth_syncingstill reportedfalse.Two dumps minutes apart showed the same goroutine id, receiver and
dbBranchespointers — livelock, not a slow walk. 68,732 extension runs over 5.9d uptime before it hit.Fix
End the step when a wave defers part of its miss set and pinned nothing. Each iteration now either breaks, strictly shrinks the frontier, or increments
nextDepth. Deferred work resumes on the nextRun, so there is no throughput cost.Also:
minEntryBytes— below one entry's cost the batch is pure waste.cache == nilcheck toPreloadContractTrunkParallel:Runreturns the error, then the wrapper's logger block dereferencescache.PinnedCount(), so callers got a panic instead.queueEmptyexpressions into one method.Testing
Differential run of the fixed
Runagainst the pre-guard one over 40k configs / 2.3M steps: identical pins,queueEmptyandusedBytesat every step. The pre-guard code livelocked in 18,306 of those configs. Thanks @awskii.ExactBudgetFillTerminates— the production state; hangs before the fix.NoPinWaveEndsStep— both no-pin paths as two budgets one byte apart, asserting the exact resolver call count. Each case fails against exactly one of the two production changes reverted, so neither is left unpinned by the other.DeferWithDbHitsInSameWave— deferral boundary with db-hits pinned in the same wave.StepBudgetSweepTerminates— budgets straddling one entry's cost.Hang guards abort the binary rather than
t.Fatal, sinceRunhas no cancellation and a spinning goroutine would outlive the failure.execution/commitmentgreen under-race -count=2;make lintclean.Scope
preload_parallel.gois the only site with this shape: the serialContractTrunkPreload.Runpops its queue head unconditionally,OnBlockCompleteranges a bounded set, andexecStatusList.drainDeferredalready has a progress net.release/3.6needs a backport;release/3.5does not have the feature.Notes (not fixed here)
adaptive_pin.go:c.missesentries are never removed, so onesync.Mapentry accumulates per contract hash ever touched andOnBlockCompleteranges the whole set every block. Found by @awskii.commitment_trunk_preload_{bytes,duration_seconds}_totalare declared but never written — no metric signal for preload work while this was wedged. Fixed in execution/commitment: record the trunk-preload duration and bytes counters #23067.PerContractMaxBudgetBytes - usedBytesdrops below one entry's cost,runExtensionLockedruns every block and pins nothing. Bounded per-block work, not a hang.