execution, cl/phase1: stop discarding a payload that is already built

[lystopad]

Follow-up to #23273, addressing @yperbasis's post-merge review. Part of the series splitting #23105.

A finished payload could be thrown away

BlockBuilder.Stop selected on the caller's context and on the finished payload at once. When both are ready Go picks between the cases at random, so roughly half the time it returned context canceled while holding a complete block.

This was unreachable before #23273 because the caller passed context.Background(). Now that a real context reaches it, a validator client that times out — or any request cancellation during the proposal window — can lose a payload that was ready to publish. The finished payload now wins.

TestBlockBuilderStopPrefersAFinishedPayloadOverAnExpiredCaller drives 50 iterations with both ready; it fails on every run against the previous ordering.

Cancellation is not a build failure

GetAssembledBlock logged ERROR "Failed to build PoS block" err=context canceled when the caller simply gave up — routine on a healthy node once a cancellable context reaches it. It now returns the error without an error-level record. Classification comes from the returned error rather than the ambient context, which cannot drift between Stop returning and the check.

One identity for the busy signal

The sentinel moves to execution/execmodule, next to the Busy field it reports, so set_head.go's identically worded error shares its identity rather than only its wording. errors.Is now works across both.

Smaller points from the same review

• the hand-rolled cancellable sleep is common.Sleep(ctx, delay), and the drain branch it had was dead;
• when the context expires during a busy wait, the contention that caused the wait is wrapped into the error instead of being replaced by a bare context.Canceled — that cause is the diagnostic cl/phase1, execution: give the execution module a typed busy signal and the caller's context #23273 introduced;
• the time.Hour waits in the retry tests are time.Second, so a regression fails as an assertion rather than as a package timeout.

Left for later, as suggested: the server-side Acquire(ctx) question, ErrUnknownPayload, and the LittleEndian/BigEndian payload-id mismatch on the isLocal engine path.

[Copilot]

Pull request overview

This PR is a follow-up in the Caplin ↔ execution-module integration that prevents a fully-built payload from being discarded when BlockBuilder.Stop is called with a canceled/expired caller context, and it unifies the “busy” sentinel error identity across call sites.

Changes:

• Make BlockBuilder.Stop prefer a finished payload over a canceled/expired caller context to avoid nondeterministically dropping ready blocks.
• Move the execution-module “busy” sentinel to execution/execmodule and update callers/tests to use the shared identity (errors.Is works across packages).
• Improve retry behavior and cancellation reporting: use common.Sleep(ctx, d), preserve the last contention error when timing out/canceling, and tighten test delays to avoid hour-long hangs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
execution/execmodule/set_head.go	Wraps semaphore acquisition failures with the shared execmodule.ErrBusy sentinel.
execution/execmodule/interface.go	Introduces execmodule.ErrBusy sentinel next to the Busy field it represents.
execution/execmodule/chainreader/chain_reader.go	Switches busy returns from the old chainreader sentinel to execmodule.ErrBusy.
execution/execmodule/block_building.go	Avoids error-level logging for caller cancellation/deadline in GetAssembledBlock.
execution/builder/block_builder.go	Updates Stop to deterministically prefer an already-finished payload over ctx.Done().
execution/builder/block_builder_test.go	Adds a regression test to ensure Stop returns the finished payload even with a canceled caller context.
cl/phase1/execution_client/execution_client_direct.go	Updates assemble retry logic to wait via common.Sleep and key off execmodule.ErrBusy.
cl/phase1/execution_client/execution_client_direct_test.go	Updates retry tests to use execmodule.ErrBusy and shorter delays.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lystopad]

Good catch. Acquire fails for two different reasons and I flattened them: the local 5s timeout, which does mean the module is occupied, and the caller's own context going away, which says nothing about it. Marking the second as busy invites a retry with nothing to wait for.

Only the timeout reports ErrBusy now. The conflation predates this PR, but it was harmless while the error was untyped — making it a sentinel is what turned it into something callers can branch on, so it belongs here. Covered by TestSetHeadReportsBusyOnlyWhenTheModuleIsOccupied, which fails against the previous wrapping.

[lystopad]

You are right — Stop sets interrupt before it waits, so the builder is stopped either way and the comment described something that does not happen. Reworded to the actual point: the caller gave up waiting, and nothing about the build itself went wrong.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[yperbasis]

Requesting changes for one remaining race in BlockBuilder.Stop. The current priority probe does not cover completion between the outer and inner selects, so the payload-loss behavior can still occur. Please address the inline finding.

[yperbasis]

b.done is only prioritized by the probe above. If it closes after that probe but before this inner select chooses, both ctx.Done() and b.done are ready and Go may still select this branch, returning cancellation while the completed payload is available. This leaves the proposal-loss race in place, only with a smaller window. Please non-blockingly recheck b.done here before returning; a regression test can force completion between the probe and selection.

[lystopad]

You are right — the probe narrowed the window rather than closing it. If the payload lands between the probe and the select's choice, both channels are ready again and it is back to a coin flip.

The cancellation branch now re-checks before returning, so a payload that has landed is never traded for it. The remaining window is only where the payload genuinely had not landed when the decision was made, which is a real race rather than a scheduling bias.

On forcing it: my first attempt at a regression test did not reproduce the bug at all — the payload landed before Stop even probed, so the outer check caught it and the test passed against the broken version too. What works is closing the completion channel from the context's own Done method, which the select reads while setting itself up. That puts the completion exactly between the probe and the choice on every iteration. Removing the re-check makes it fail immediately; with the re-check, 300 iterations pass every run.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

execution/execmodule/set_head.go:59

• This still classifies the failure by reading the parent context after Acquire returns. If the five-second timeout releases Acquire and the caller is canceled before this check, the local timeout is incorrectly returned without ErrBusy. Latch the timeout source with WithTimeoutCause and inspect the derived context's cause instead.

		if ctx.Err() != nil {

[lystopad]

Also took the suppressed comment about SetHead in a6d145b487 — you were right that reading the parent context after Acquire returns is itself a guess. If the five-second wait runs out and the caller is cancelled a moment later, the check sees cancellation and drops ErrBusy from a failure that genuinely was contention.

The wait now carries its own cause via context.WithTimeoutCause, recorded when it ran out, so the classification does not depend on what the caller's context looks like afterwards. TestSetHeadReportsBusyWhenItsOwnWaitRunsOut pins that a cancellation arriving after the timeout does not change the cause.

[yperbasis]

Nits only, none blocking. The red mainnet-rpc-integ-tests check is an infra flake (HTTP 429 while downloading actions/checkout), unrelated to this PR.

• execution/execmodule/block_building.go:134: the gate classifies by error shape, but Build runs on the node lifecycle ctx, not the caller's. A genuine build failure that wraps a context error — e.g. Shutter's parent-block wait timeout, or node shutdown surfacing through BeginTemporalRo — skips the ERROR log while the caller is still alive, and the comment above is then wrong. A precise alternative: let Stop mark its own ctx.Done() branch with a typed sentinel and gate on that.
• execution/execmodule/set_head.go:64: this is now the only unwrapped error return in SetHead; fmt.Errorf("set head: %w", err) would keep the operation visible in the error. On line 62, %w: %w also makes the busy error satisfy errors.Is(err, context.DeadlineExceeded); if that matchability is unwanted, %v for the inner error avoids it with the same message.
• execution/execmodule/set_head_internal_test.go:42: the new test pins the stdlib cause-latch but never calls SetHead, so deleting the ErrBusy wrap on set_head.go line 62 still leaves the package green. An unexported acquire-timeout field on ExecModule (the tests already build the struct literally) would let the busy arm run through SetHead in milliseconds.
• execution/execmodule/interface.go:101: "Busy is true when the builder has not finished yet" describes the wrong condition — it is true when the module semaphore was contended; otherwise GetAssembledBlock blocks in Stop until the builder finishes. It now also contradicts the ErrBusy doc a few lines above.
• cl/phase1/execution_client/execution_client_direct_test.go:71: with a 1s delay the test also passes if common.Sleep is replaced by a plain time.Sleep — the loop-top check then returns an error of the same shape after the sleep, so the "abort during the backoff wait" property is no longer pinned. Asserting the test finishes well under the 1s delay restores it.
• cl/phase1/execution_client/execution_client_direct.go:167: the err != nil arm duplicates the Sleep-site wrap and looks unreachable — assemble never blocks on ctx, and common.Sleep already returns the wrap on cancellation; only a cancellation landing in the gap after Sleep returned nil hits it. A plain ctx check hoisted above the loop keeps one wrap site.
• execution/builder/block_builder.go:96: "The second check matters as much as the first" overstates the outer probe — done never reopens, so the inner re-check already covers it, and both tests stay green with the probe deleted. Worth rewording so nobody later keeps the probe and drops the re-check, which is the one that prevents the bug. Line 94: "however close together the two arrive" → "even when both are ready at the same time" is easier to parse.
• Optional, on-theme: cl/beacon/handler/block_production.go:259 logs ERROR for every poll failure, including execmodule.ErrBusy — the routine contention the poll loop exists to wait out. The now-exported sentinel makes a one-line demotion to Debug possible.

[domiwei]

I found a few remaining issues on a6d145b:

• Cancellation is lost on the final busy attempt (cl/phase1/execution_client/execution_client_direct.go:172-186). If assemble cancels the context and returns execmodule.ErrBusy on the last attempt, the attempt+1 == attempts branch breaks and returns only ErrBusy. This makes cancellation classification depend on the retry position. A deterministic reproducer is attempts=1, with the callback calling cancel() and returning ErrBusy; the returned error does not match context.Canceled. Please recheck ctx.Err() before the final busy return and preserve both causes, as the earlier-attempt paths do.

• Removing chainreader.ErrExecutionBusy breaks source compatibility. It was exported on the base branch, so downstream code referring to it no longer compiles. The canonical identity can still move to execmodule.ErrBusy while retaining a compatibility alias such as var ErrExecutionBusy = execmodule.ErrBusy in chainreader.

• SetHead can mistake a caller-supplied cancellation cause for local contention (execution/execmodule/set_head.go:54-63). ErrBusy is used both as the exported classification and as the derived context's private timeout marker. With ctx, cancel := context.WithCancelCause(parent); cancel(ErrBusy), context.Cause(acquireCtx) is also ErrBusy, so SetHead returns an error matching ErrBusy even though only the caller went away. I reproduced this against the PR head. Please use a private sentinel for the local timeout and map only that sentinel to the public ErrBusy.

• The positive SetHead test does not exercise SetHead (execution/execmodule/set_head_internal_test.go:41-55). It independently constructs a WithTimeoutCause, calls the semaphore, and checks context.Cause; it would still pass if SetHead stopped returning ErrBusy. Please cover the actual acquisition-failure path and assert the public returned error identity.

The updated BlockBuilder.Stop handoff looks correct now: the post-cancellation completion probe closes the remaining select window. Focused existing tests and CI are green; the two deterministic reproducers above still fail on the current head.

Kewei found issues