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

[yperbasis]

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 execution/stagedsync: checkpoint commitment at step boundaries in parallel exec #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:

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:

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.

[VBulikov]

yperbasis added Imp2 as Importance

[VBulikov]

VBulikov added Imp2 as Importance

[VBulikov]

yperbasis changed Importance from Imp2 to Imp3

[VBulikov]

yperbasis changed Importance from Imp3 to Imp2