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

[lystopad]

@domiwei — all four fixed in d228f186f6, and both of your reproducers now fail on the previous code.

Cancellation lost on the final busy attempt. Right: with no attempts left there is no wait to notice it, so how the caller got classified depended on which retry it died on. The last attempt is checked directly now and keeps both causes. TestRetryAssembleBlockKeepsCancellationOnTheFinalAttempt is your attempts=1 case.

SetHead mistaking a caller-supplied cause. This was the sharpest one — I had used the exported sentinel as the private marker, so cancel(ErrBusy) from a caller was indistinguishable from the module being occupied. The marker is private now and only it maps to the public ErrBusy. Your reproducer is TestSetHeadDoesNotReportBusyForACallerCancelledWithThatCause, and it fails against the previous head.

The positive test not exercising SetHead. Fair, and it is the third test of mine in this series that did not run what it claimed. The wait's length is a field now, so all three cases go through SetHead itself in milliseconds and assert the returned error rather than a reconstruction beside it.

Source compatibility. Taken — chainreader.ErrExecutionBusy stays as an alias for execmodule.ErrBusy, marked deprecated, so callers that referred to it keep compiling and errors.Is matches across both.

Also took @yperbasis's non-blocking notes on this branch: the Busy field doc described the builder still working rather than the module being occupied; the cancellation test could not tell a cancellable wait from a plain sleep, so it now asserts it returns well inside the backoff; the SetHead returns are wrapped; and the comment on the payload priority check credited the shortcut rather than the check that actually prevents the bug.

[yperbasis]

lint is red

[Copilot]

Pull request overview

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

[yperbasis]

Requesting changes on the two blocking items; the rest are non-blocking.

Blocking

• 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 (Shutter's parent-block wait timeout, shutdown during BeginTemporalRo) is treated as "the caller gave up": the ERROR log is skipped and the comment above it is wrong. Let Stop tag its own ctx.Done() give-up branch with a typed sentinel and gate on that instead. Flagged in the previous review as well as by a second reviewer.
• execution/execmodule/interface.go:160: the Busy field doc fix did not reach the GetAssembledBlock method doc, which still says "The result is Busy when the builder has not finished" — the file now contradicts itself. Same one-line fix as the field doc.

Test coverage (non-blocking)

• execution/builder/block_builder.go:103: the cancellation branch (return nil, ctx.Err()) has no test — deleting it leaves the suite green, and if it is ever lost, a caller cancellation turns into UnknownPayloadErr at the engine API instead of a context error. A cancelled ctx plus an unfinished build pins it.
• execution/execmodule/set_head.go:71: the three SetHead tests pin outcomes but not the mechanism — classifying on errors.Is(err, context.DeadlineExceeded) instead of the cause passes all three while misreporting a caller's shorter deadline as busy. A fourth case (acquireTimeout of an hour, caller timeout of 1ms → not busy) kills that mutant.
• cl/phase1/execution_client/execution_client_direct_test.go:80: 500ms against a 1s delay leaves little margin on a loaded CI runner, and a plain time.Sleep(200ms) still passes. delay = time.Minute with a ~10s bound keeps the property with margin in both directions.

Minor

• cl/phase1/execution_client/execution_client_direct.go:192: the retry path keeps ErrBusy matchable in cancelled-caller errors while SetHead strips it — both defensible, but one doc line on ErrBusy stating the convention would keep a future errors.Is(err, ErrBusy) retry trigger from firing for a dead caller.
• cl/phase1/execution_client/execution_client_direct.go:163: ranOut duplicates errors.Join (precedent: db/integrity/torrent_verify.go:54); worth keeping only if the single-line message shape is load-bearing.

[lystopad]

All of it in 517b7bff83, blocking and not.

Classifying by error shape. You and @domiwei both raised this and I answered it twice without fixing it — I argued the returned error was enough, then narrowed it to context.Canceled/DeadlineExceeded, which is still reading the shape. It is not enough, for the reason you give: Build runs on the node lifecycle context, so a real build failure can carry a context error of its own and be read as the caller giving up, skipping the record for it.

Stop now tags its own give-up branch with ErrStopAbandoned and that is what the caller checks. The context error is still wrapped inside it, so errors.Is(err, context.Canceled) keeps working for anyone who wants it.

The GetAssembledBlock method doc. Fixed — I had corrected the field doc and left the method doc saying the opposite two screens away.

The three coverage gaps. All three were fair, and all three describe tests of mine that pinned an outcome without pinning the mechanism. I checked each against the substitution you named rather than assuming:

• deleting Stop's cancellation branch: now fails;
• classifying SetHead on errors.Is(err, context.DeadlineExceeded) instead of the cause: now fails, via the hour-long wait against a millisecond caller;
• replacing common.Sleep with a plain sleep: now fails, and takes the full minute doing so, which is the point of moving the delay to a minute and the bound to ten seconds.

Minor. ErrBusy documents the convention: matchable inside a cancelled caller's error so the contention is not lost, but not on its own a reason to retry for a caller that has gone. On ranOut versus errors.Join — I kept it, and said why in the comment: this string ends up in a log record, and errors.Join puts a newline between the two.

make lintci clean apart from the darwin-only db/seg artefact, go test -race green across execution/... and cl/phase1/execution_client.