miner: recover veblop producer stall when a build wedges with no pending task

[lucca30]

Problem

While stress-testing the veblop block-production path, a producer can enter a permanent stall: it stops producing/importing at a fixed height and recovers only after a process restart. Symptoms are Possible producer stall: veblop fallback skipping while chain is stale logged repeatedly with a static currentBlock and pendingTasksCount=0 — the producer's own stall detector fires, but the fallback never actually recovers.

Root cause

newWorkLoop's veblop stall-fallback skipped recovery whenever pendingWorkBlock == currentBlock+1, treating that value as proof the next block was being built. But commitWork only clears pendingWorkBlock via a defer ...Store(0) that runs on return. If a build blocks inside commitWork and never returns, pendingWorkBlock stays pinned at currentBlock+1 with no sealing task (pendingTasks empty). In that state the fallback short-circuited forever — the producer never resubmitted work, and the only escape was a restart.

(A build blocking inside commitWork is a separate concern being investigated independently. This PR addresses the recovery path so that such a hang is no longer permanent.)

Fix

• The fallback now treats pendingWorkBlock == next as "work in flight" only when a sealing task actually backs it. When pendingWorkBlock == next but no task exists, it distinguishes a wedged build from a slow-but-live one by how long the build has been outstanding (time since submit), not by block-timestamp age (chainAge is meaningless when the head carries an old timestamp, e.g. genesis). Recovery fires only past a 3×-blocktime threshold, so a live build is never interrupted.
• The fallback re-commit uses a non-blocking send, so newWorkLoop cannot itself wedge on the unbuffered newWorkCh when mainLoop is the blocked goroutine. It keeps emitting the stall warning and retrying every tick, and auto-recovers the moment mainLoop is unblocked.
• The decision is extracted into a pure decideVeblopFallback function so it can be unit-tested without driving the whole loop.

Testing

• New table-driven unit test TestDecideVeblopFallback covering: the wedged-build case (the bug), the genesis in-progress-build case (regression guard against interrupting a live build), task-in-flight, the no-work-claimed paths, and the wedge threshold boundaries.
• Verified as a true regression test: the suite goes red against the pre-fix logic (the wedged-build cases return "skip" instead of "recommit") and green against this fix.
• Full miner/ package suite passes (this caught an over-aggressive first attempt that used chainAge and interrupted live builds at genesis).

Scope / follow-ups

This restores producer-loop liveness and auto-recovery once the underlying build unblocks. It does not by itself resolve a hard mainLoop deadlock; that underlying build-blocking concern is tracked separately.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[codecov]

Codecov Report

❌ Patch coverage is 86.95652% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.09%. Comparing base (5df879f) to head (f52c135).
⚠️ Report is 49 commits behind head on v2.9.1-candidate.

Files with missing lines	Patch %	Lines
miner/worker.go	86.95%	8 Missing and 4 partials ⚠️

❌ Your patch check has failed because the patch coverage (86.95%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           v2.9.1-candidate    #2287      +/-   ##
====================================================
+ Coverage             53.44%   54.09%   +0.64%     
====================================================
  Files                   896      907      +11     
  Lines                159770   162060    +2290     
====================================================
+ Hits                  85397    87668    +2271     
+ Misses                69054    68981      -73     
- Partials               5319     5411      +92     

Files with missing lines	Coverage Δ
miner/worker.go	78.02% <86.95%> (+0.01%)	⬆️

... and 74 files with indirect coverage changes

Files with missing lines	Coverage Δ
miner/worker.go	78.02% <86.95%> (+0.01%)	⬆️

... and 74 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lucca30]

@claude review

[claude]

This is a block-producer liveness fix in miner/worker.go — well-tested at the unit level, but it touches production consensus timing, changes the semantics of the commit send (blocking → non-blocking on the fallback path), and defers pendingWorkBlock.Store until after the send succeeds. The PR itself notes it's a recovery mitigation while the underlying commitWork wedge remains under investigation. Worth a human eye on the design choices (3× block-time threshold, non-blocking send, pendingWorkSubmittedAt tracking).

What was reviewed

• The new decideVeblopFallback pure function and its truth table in TestDecideVeblopFallback (wedged-build, genesis regression guard, threshold boundaries).
• commit() signature change and the two call sites that pass nonblocking=false vs true; verified pendingWorkBlock is only claimed after a successful non-blocking send.
• Candidate race between pendingWorkBlock.Store(next) in the fallback and mainLoop's Store(0) in commitWork — examined and refuted (mainLoop's Store(0) only runs after it accepted the request, so the ordering is safe).
• Checked that no consensus/signer/snapshot/Heimdall behavior is touched — this is purely producer-loop liveness.

Extended reasoning...

Overview

The PR modifies newWorkLoop in miner/worker.go to recover from a producer stall where a build wedged inside commitWork leaves pendingWorkBlock pinned at currentBlock+1 with no sealing task. It extracts the fallback decision into a pure decideVeblopFallback function, adds a stallThresholdSec (3× block time) heuristic based on time-since-submit rather than block-timestamp age, changes commit() to support a non-blocking send on the fallback path (so newWorkLoop doesn't wedge on the unbuffered newWorkCh when mainLoop is itself blocked), and only stores pendingWorkBlock after the send succeeds. New table-driven tests cover the wedged-build, genesis regression, threshold-boundary, and task-in-flight cases.

Security risks

No direct consensus-rule, signer, snapshot, Heimdall, or block-validation changes. The concerns are producer liveness rather than consensus safety: a false-positive wedge detection would abort an in-flight build, but the code guards against this by requiring the build to have been outstanding for 3× the block period. The non-blocking send cannot cause a lost work request in the steady case because the normal new-head/timer paths still use blocking sends; the fallback path is only exercised when the producer is already stuck.

Level of scrutiny

Higher than a typical fix. miner/worker.go is production block-producer code; the .claude/rules/consensus-security.md scope explicitly covers miner/ and treats changes here as CRITICAL by default. The fix relies on threshold heuristics (3× blocktime) and channel timing (non-blocking send when mainLoop is wedged), and the PR itself acknowledges the deeper commitWork hang is still unresolved. A human familiar with veblop's block-production model should confirm the threshold value and the ordering of pendingWorkBlock.Store vs the send.

Other factors

• Codecov reports 86% patch coverage (below the 90% target) — the untested branches are the non-blocking-send-failed and the veblopTimer.Reset paths.
• SonarQube gate passed with zero new issues.
• No prior human review yet; the author (lucca30) explicitly requested @claude review.
• The candidate race between the fallback's pendingWorkBlock.Store(next) and mainLoop's deferred Store(0) was examined by the bug-hunting verifier and refuted — mainLoop only runs its defer after accepting the request, so the ordering is safe.
• I have not previously commented on this PR.

[lucca30]

On the two red checks — deliberately leaving the code as-is:

Quality metrics (diffguard complexity) — advisory, not a merge gate. The flagged function is newWorkLoop, which is already a known //nolint:gocognit giant (base cognitive complexity 41, well over the 15 threshold before this PR). This change adds +13, but the right structural move was already made: the fallback decision is extracted into the pure, unit-tested decideVeblopFallback. The remaining complexity lives in the veblopTimer.C case body, which is tightly coupled to the commit closure (captured interrupt/timestamp/timer locals). Extracting it further would mean reshaping timing-sensitive block-producer recovery code purely to satisfy an advisory metric — a poor risk/reward trade on the liveness path. For reference, the check is non-blocking: #2296 merged with Quality metrics failing.

codecov/patch (86.36% vs 90%) — also advisory. The uncovered lines are the non-blocking-send-failed branch and the veblopTimer.Reset path, which can't be exercised without driving the full newWorkLoop goroutine; the decision logic itself is covered by the table-driven decideVeblopFallback tests.

The meaningful CI (build / unit / integration / e2e / lint) is green. This is ready for a human review of the design choices the bot flagged (3× block-time threshold, non-blocking send, pendingWorkSubmittedAt tracking).

[cffls]

What's the reason for 3x veblopTimeout? Will there be any potential issue if we use 1x?

[lucca30]

The 3x is anchored to two things — the span-rotation window (the reason for the value) and the existing staleness definition (the reason it's this multiple, not a new one).

Rotation window (load-bearing). Mainnet producers run miner.blocktime = 1.5s, so 3x = 4.5s. That sits deliberately below veblop's rotation clock — veblopBlockTimeout = 8s (the new-span check) and waitForNewSpan = 16s. The producer gets ~4.5s to self-recover locally before the network rotates production to the next span's producer. Recovery is single-producer-local (no wiggle/backup in veblop), so it never competes with rotation; and if the node truly can't recover, the recommits fail seal-authorization once the span moves — no fork, rotation just takes over. Going higher would be the risky direction: at 1.5s a 5x = 7.5s starts crowding the 8s rotation wall.

Same staleness definition. warnIfStalled flags "stale" at chainAge > 3x block period, and recovery reuses that same 3x rather than inventing a second threshold, so we act on the same notion of "stuck" we already warn on. (They apply the 3x to different clocks: the warn measures chain-age — time since the head advanced; the wedge-recovery measures build-outstanding age — time since we submitted the build. Same multiple, the appropriate signal for each.)

Why not 1x. Builds legitimately overrun a single period under transient load (heavy block, GC pause, disk I/O, txpool-lock contention). At 1x those get interrupted and re-run from scratch — churn that can cause the very misses it's meant to prevent. 3x is the smallest multiple that clears normal jitter while staying under the rotation window.

One follow-up your question surfaced: this was silently computing the threshold in whole seconds, so at 1.5s it truncated 3x to 3s (and would degrade to 6x a hypothetical 0.5s block). I pushed a follow-up commit (f52c135) moving both this recovery path and warnIfStalled to real time.Duration, so 3x honors the 1.5s block time exactly. Thanks for the prompt.

[Copilot]

Pull request overview

Improves Bor miner liveness by hardening the “veblop” producer stall-fallback so a wedged commitWork no longer results in a permanent block-production stall, and adds unit tests around the decision logic.

Changes:

• Introduces a pure decideVeblopFallback decision function with an explicit action enum to make fallback behavior testable.
• Tracks pendingWorkSubmittedAt and uses “time since submit” (not head timestamp age) to distinguish a slow build from a wedged build.
• Makes the veblop fallback recommit path use a non-blocking send to avoid wedging newWorkLoop when mainLoop is stuck, plus adds a table-driven unit test suite.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
miner/worker.go	Adds veblop fallback decision logic, outstanding-build tracking, and non-blocking recommit submission to improve stall recovery behavior.
miner/veblop_fallback_test.go	Adds unit coverage for the fallback decision matrix, including the wedged-build regression case and boundary conditions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lucca30]

Good observation on the ordering — interrupt.Store(s) does run before the send and isn't rolled back if the non-blocking send hits default. But it doesn't cause the harm described, because of the state this path runs in.

The non-blocking send is only reached from the veblopRecommit branch. For a claimed-but-taskless build (pendingWorkBlock == next && !hasPendingTasks) that requires the build to have been outstanding >= 3x the block period. So the interrupt being signaled is only ever one of:

1. a genuinely wedged build — and signaling it to abort is the intended behavior: when it finally unblocks inside commitWork we don't want it sealing a block built against stale state, we want it to bail so the fresh recommit wins. Setting the interrupt even when the channel isn't ready is deliberate for exactly this case; or
2. in the "nothing claimed" recommit branch (pendingWorkBlock != next), a build that already returned and cleared pendingWorkBlock — so interrupt references a completed build and the store is a no-op.

A healthy, live build can't reach this path: mid-execution it either has a pending task (→ veblopSkip) or is under the 3x threshold (→ veblopWait). So we never interrupt a build that would otherwise have succeeded.

On churn: the dropped-send path doesn't lose or leak a request — newInterrupt is local and only promoted to interrupt on a successful send; the old interrupt is re-stored idempotently each tick. Recovery retries on the next veblop tick and self-corrects the moment mainLoop is ready again, so the extra delay is bounded to one tick and only occurs while mainLoop genuinely isn't receiving.

Net: keeping the store before the send is intentional for the wedged case; moving it after the send would let a wedged build seal a stale block when it unblocks. Leaving as-is.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud