erigontech / erigontech/erigon

execution/stagedsync: parallel exec/apply-boundary error-handling robustness (follow-ups from #22102)

Open
#22,121 4 comments 0 reactions 1 assignee Claimed by @yperbasis View on GitHub
tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

Follow-up from #22102 (masking-layer fixes for the silent parallel-exec loop, #22101).

This tracks the remaining error-handling robustness items at the parallel exec/apply boundary. The fixes in #22102 (`reconcileExecAndWaitErr`, the apply-boundary `checkBlocksDrained`, the `pe.wait` shutdown contract) close the infinite-loop, misreported-bad-block, and shutdown-error-drop paths; finding 1 below (found reviewing #22102) is what's left, and finding 2 (found reviewing #22092) is the same class on the commitment-result path:

- **Finding 1** — `checkBlocksDrained`'s `ctx.Err()` guard doesn't cover the deliberate wrong-trie-root stop; that case is held back only by an implicit, non-local invariant. *No current bug; latent fragility.*
- **Finding 2** — commitment `out`-channel errors are fatal at the apply boundary with no cancellation exemption, so a shutdown-time `context.Canceled` from the calculator can surface as a returned error (newly activated by #22092). *No current bug; latent fragility + diagnosability.*

---

## Finding 1 — `checkBlocksDrained` deliberate-stop suppression relies on a non-local invariant, not on its `ctx.Err()` guard

### Problem

`checkBlocksDrained` runs at the apply boundary in `execImpl` and turns a clean exit (`execErr == nil`) that left blocks in `pe.blockExecutors` into an `ErrInvalidBlock`:

```go
func (pe *parallelExecutor) checkBlocksDrained(ctx context.Context, execErr error) error {
if execErr != nil || ctx.Err() != nil {
return execErr
}
pe.RLock()
pending := slices.Sorted(maps.Keys(pe.blockExecutors))
pe.RUnlock()
if len(pending) > 0 {
return fmt.Errorf("%w: parallel exec apply loop finished cleanly but %d scheduled block(s) never drained: %v",
rules.ErrInvalidBlock, len(pending), pending)
}
return nil
}
```

The `ctx` here is `execImpl`'s **parent** context. But a deliberate wrong-trie-root stop cancels the **executor** context, not the parent: `deliberateCancel` → `executorCancel(nil)` → `execLoopCtxCancel` (in `pe.run`), which cancels only the `execLoopCtx` child created by `context.WithCancelCause(ctx)`. The parent `ctx` passed to `checkBlocksDrained` is never canceled by it, so **`ctx.Err()` does not catch the deliberate stop.**

(The parent-vs-executor split is intentional: the *normal* end-of-batch `executorCancel(nil)` also cancels the executor context, so keying `checkBlocksDrained` on that would over-suppress every batch — which is exactly why the pre-#22102 inner-ctx `execLoopExitCheck` was replaced with a parent-ctx boundary check.)

On a deliberate stop, blocks are usually still pending in `pe.blockExecutors` (the executor is canceled before later blocks produce results). The only thing that stops `checkBlocksDrained` from mislabeling that as a silent miss is the **first** arm, `execErr != nil`: `deliberateCancel` is called exclusively from `processCommitErr`, which sets `*deferredRootErr` (non-nil) immediately before `cancel()`, and the apply loop returns that `deferredRootErr` ahead of any clean `return nil`. So `execErr` is always non-nil on a deliberate stop today.

### Impact

**No current bug.** There is no reachable state where `execErr == nil` AND the parent `ctx` is live AND a non-genuinely-missed block remains in the map, so `checkBlocksDrained` produces no false positive today (verified across the max-reached, fork-validation, size-limit/`ErrLoopExhausted`, and deliberate-stop exits).

The fragility is that the deliberate-stop safety rests on a **non-local** invariant — "every executor cancel coincides with a non-nil error surfaced ahead of the drain check" — rather than on `checkBlocksDrained`'s own guards. Pre-#22102 code enforced this explicitly via `context.Cause(execLoopCtx) == errDeliberateStop`; #22102 removed that sentinel, so the guarantee is now implicit. If a later change adds a second `deliberateCancel`-style caller — or any clean executor-context cancel that does not also surface an error — the boundary would see `execErr == nil`, a live parent `ctx`, and leftover `blockExecutors`, and manufacture a spurious `ErrInvalidBlock` → `BadBlock` + unwind, discarding valid state. Same "silent → surprising" class as #22101.

### Suggested fix

Cheap; pick one:

- **Document the invariant** at `checkBlocksDrained` / `deliberateCancel`: deliberate stops are suppressed via the `execErr != nil` arm because a deliberate cancel always coincides with a non-nil `deferredRootErr` surfaced ahead of the drain check, so any new executor-cancel site must preserve that.
- **Or enforce it rather than document it**: assert the invariant, or key the completeness check off the executor context's `Cause` (reintroducing a typed deliberate-stop cause) so the suppression is explicit and can't silently decouple.

---

## Finding 2 — commitment `out`-channel errors are fatal at the apply boundary (no cancellation exemption)

### Problem

The apply loop's `handleCommitResult` / `processCommitErr` (`execution/stagedsync/exec3_parallel.go`) treat any commitment result whose `err` is not `ErrWrongTrieRoot` as an immediate fatal return:

```go
handleCommitResult := func(cr commitmentResult) error {
if cr.err != nil {
if !errors.Is(cr.err, ErrWrongTrieRoot) {
return fmt.Errorf("[%s] commitment: %w", pe.logPrefix, cr.err) // fatal
}
...
```

There is no `context.Canceled` / `context.DeadlineExceeded` exemption on this path. On graceful shutdown the commitment calculator's `ComputeCommitment` returns raw `context.Canceled` (the per-key `ctx.Err()` check in the trie fold, `hex_patricia_hashed.go`); `compute()` wraps it and `publish()` still **sends** it to `cc.out` — it only skips *logging* cancellation, not the send. If that best-effort send wins the race against `ctx.Done()`/`cc.done`, the wrapped `context.Canceled` becomes `execErr` and is returned from `pe.exec()`.

This path was newly activated by #22092: before it, the first partial (resumed) block's compute failure was logged-and-swallowed (`hasComputed` set, nothing published); #22092 folds `computeWithoutCheck` into the shared `compute()`, which publishes the error like every other compute path.

### Impact

No current bug. It does **not** become a bad-block/unwind — the coarse `errors.Is(execErr, context.Canceled)` guard at the `execImpl` boundary swallows it before the `ErrInvalidBlock` branch. But shutdown correctness on this path now rests entirely on that one coarse catch, and the behavior changed from "returns nil on shutdown" to "can return `context.Canceled`" — the same latent-fragility class as finding 1, with the trigger race-gated (best-effort send).

### Suggested fix

Give the commitment-error path its own local cancellation guard: have `handleCommitResult` / `processCommitErr` explicitly ignore `context.Canceled` / `context.DeadlineExceeded` (treat like the wrong-root deferral) rather than relying on the boundary-level `errors.Is`. This also restores #22092's shutdown behavior to match the code it replaced.

---

## Context

Both findings share the same "silent → surprising" failure class as #22101 (finding 1 surfaced reviewing #22102, finding 2 reviewing #22092). Neither is a correctness bug on `main` today: Finding 1 is held back by the `deferredRootErr` invariant, and Finding 2 is caught before any unwind by the boundary-level `errors.Is(execErr, context.Canceled)` guard. They are tracked here as tech-debt hardening of the exec/apply-boundary error handling.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.