cl/phase1, execution: give the execution module a typed busy signal and the caller's context

[lystopad]

Split out of #23105, which grew too large to review in one piece. Independent of the other parts of that series.

Contention was indistinguishable from rejection

AssembleBlock and GetAssembledBlock reported a busy execution module as errors.New("execution data is still syncing"). That message is inaccurate — it is weight-one semaphore contention with a forkchoice update or another payload request, not syncing — and being an untyped error, callers could not tell it apart from a real rejection.

Both now return chainreader.ErrExecutionBusy, and the assemble retry waits only on that. Previously a permanent rejection — mismatched withdrawals, for instance — was retried thirty times across six seconds before surfacing, which spends the proposal slot rather than reporting the problem.

The retry ignored its caller

The loop was for range 30 { ...; time.Sleep(200 * time.Millisecond) }, with no context check anywhere. A cancelled caller still waited the full six seconds. It is now an extracted helper that checks the context before each attempt and waits on it rather than on a bare sleep.

While there, ChainReaderWriterEth1.AssembleBlock and GetAssembledBlock take the caller's context instead of substituting context.Background(), so the deadline a caller sets actually reaches the execution module.

Tests

The helper is covered directly: first success, stopping on a rejection, exhausting attempts on contention, cancellation mid-flight and before the first attempt, and the zero-attempts guard.

Part of a series splitting #23105.

[Copilot]

Pull request overview

This PR improves Caplin↔Execution-module interaction for local (in-process) execution by (1) introducing a typed “busy” signal for contention and (2) propagating the caller’s context.Context into AssembleBlock / GetAssembledBlock, so retries stop on real rejections and respect cancellation/deadlines.

Changes:

• Introduce chainreader.ErrExecutionBusy and return it from AssembleBlock / GetAssembledBlock when the exec module reports Busy.
• Replace the fixed sleep/retry loop with a context-aware retryAssembleBlock helper that retries only on ErrExecutionBusy.
• Add direct unit tests for the retry helper (success, rejection, exhaustion, cancellation, and zero-attempt guard).

Reviewed changes

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

Show a summary per file

File	Description
execution/execmodule/exec_module_test.go	Updates test call site to pass ctx into GetAssembledBlock.
execution/execmodule/chainreader/chain_reader.go	Adds ErrExecutionBusy; threads caller context into exec-module calls; returns typed busy error instead of an untyped “syncing” message.
cl/phase1/execution_client/execution_client_engine.go	Passes caller ctx through to the local chain reader on GetAssembledBlock.
cl/phase1/execution_client/execution_client_direct.go	Replaces retry loop with retryAssembleBlock that respects context and retries only on ErrExecutionBusy.
cl/phase1/execution_client/execution_client_direct_test.go	Adds unit tests covering the retry helper’s behavior (success, rejection, busy exhaustion, cancellation).

---

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[domiwei]

The implementation looks correct. I found two non-blocking production-path coverage gaps that would make these lifecycle guarantees more regression-resistant.

[domiwei]

Could we add a public-path test that drives an ExecModule Busy result through ChainReaderWriterEth1 into ForkChoiceUpdate, then verifies Busy -> success retries and Busy -> permanent error stops? The current helper tests inject ErrExecutionBusy directly, so they would still pass if the Busy-to-sentinel mapping or this wiring regressed. Non-blocking, but this is the production sequence the change is protecting.

[domiwei]

Could we cover this local-engine path, and the direct-client equivalent, with a canceled-context test that reaches the blocking builder fixture? The existing cancellation test stops at ExecModule.GetAssembledBlock, so replacing either forwarded context with context.Background() would not be caught. It would also be useful to assert that a healthy subsequent retrieval can still progress. Non-blocking coverage suggestion.

[yperbasis]

Reviewed — no blocking correctness bug in the new retry logic (attempt counting, errors.Is identity, and the fail-fast on non-busy errors all check out). A few points:

• Now that a cancellable ctx reaches ExecModule.GetAssembledBlock, a normal client-side cancellation of GET /eth/v3/validator/blocks (VC timeout, multi-BN strategies) gets logged as ERROR "Failed to build PoS block" err=context canceled on a healthy node (execution/execmodule/block_building.go:131) — this path used context.Background() before. Consider treating ctx.Err() from bldr.Stop as cancellation rather than a build failure. Related: BlockBuilder.Stop's select picks randomly when both ctx.Done() and b.done are ready, so it can return ctx.Err() although the payload is finished; checking b.done first avoids discarding a ready payload.
• When the ctx expires during the busy waits, retryAssembleBlock returns bare ctx.Err(), so the busy cause — the diagnostic this PR introduces — is lost from the caller's log. Wrapping both (e.g. fmt.Errorf("%w (last attempt: %w)", ctx.Err(), err)) keeps it.
• Sentinel home: polygon/sync already defines ErrExecutionClientBusy for the same ExecutionStatusBusy signal, and set_head.go returns an identically worded but untyped "execution module is busy" that fails errors.Is against the new sentinel. Defining it once in execution/execmodule (next to AssembledBlockResult.Busy) would give all sites one identity — and pollAssembledPayload could then also use it: demote busy probes from ERROR (they can fire every 10ms during a long FCU commit) and stop re-polling permanent errors.
• Latent, on touched lines (fine as follow-ups): the isLocal branch of ExecutionClientEngine.GetAssembledBlock decodes the payload id as LittleEndian while the in-process EngineServer encodes it BigEndian (ConvertPayloadId) — unreachable today only because --caplin.use-engine-api disables the beacon API. And ChainReaderWriterEth1.GetAssembledBlock returns all nils with a nil error for an unknown/evicted payload id, indistinguishable from "still building", so pollAssembledPayload polls a dead id for the whole slot; a typed ErrUnknownPayload next to ErrExecutionBusy would let it stop early.
• Bigger-picture option: this is the fifth client-side loop absorbing the same TryAcquire busy signal (engine_server's waitForResponse, pollAssembledPayload, the two retryBusy helpers). A server-side Acquire(ctx) wait inside AssembleBlock/GetAssembledBlock would serve every client at once and avoid TryAcquire starvation while insert traffic keeps waiters queued.
• Simplification: the timer + Stop() + drain block is common.Sleep(ctx, delay) (common/sleep.go); the drain branch is dead code (the timer is dropped right after, and since Go 1.23 timer channels are unbuffered). A single-exit loop would also remove the attempts <= 0 guard, the outer id/err vars, the break, and the trailing return. Alternative: cenkalti/backoff/v4 (already a direct dependency), as polygon/sync's retryBusy does for this same signal.
• Tests: the time.Hour waits would turn a regression into a 10-minute package timeout instead of a clear assertion failure — time.Second is enough with the same call-count asserts. Also, the comment in StopsOnRejection repeats the ErrExecutionBusy doc comment, and both use above-B2 phrasing ("burns the slot").