fix(engine): classify EL HTTP 4xx as fatal via typed HTTPStatusError#3109
Merged
Conversation
fridrik01
force-pushed
the
fix-engine-4xx-retry-loop
branch
from
May 20, 2026 14:20
940581c to
b8b4c63
Compare
fridrik01
marked this pull request as ready for review
May 20, 2026 14:24
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3109 +/- ##
==========================================
+ Coverage 61.21% 61.34% +0.13%
==========================================
Files 369 370 +1
Lines 18925 18936 +11
==========================================
+ Hits 11584 11617 +33
+ Misses 6382 6363 -19
+ Partials 959 956 -3
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves execution-layer (EL) RPC error propagation by introducing a typed rpc.HTTPStatusError so handleRPCError can distinguish HTTP status failures and classify EL HTTP 4xx responses as fatal (non-retryable), while keeping 5xx/transport failures retryable—preventing infinite retries on request rejections.
Changes:
- Add
rpc.HTTPStatusErrorto preserve EL HTTP status codes (and body) fromcallRaw. - Update
EngineClient.handleRPCErrorto classify HTTP 4xx as fatal viaErrHTTPClientError, while preserving unauthorized handling. - Add unit tests covering the new classification behavior (including HTTP timeout detection).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| execution/client/ethclient/rpc/errors.go | Introduces HTTPStatusError to carry HTTP status/body for upstream classification. |
| execution/client/ethclient/rpc/client.go | Returns HTTPStatusError on non-200 responses instead of collapsing into a generic error / unauthorized sentinel. |
| execution/client/errors.go | Adds ErrHTTPClientError and updates handleRPCError + IsFatalError to treat HTTP 4xx as fatal. |
| execution/client/errors_test.go | Adds tests validating classification across HTTP status, JSON-RPC codes, and timeouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fridrik01
force-pushed
the
fix-engine-4xx-retry-loop
branch
3 times, most recently
from
May 20, 2026 17:16
daa79f1 to
abbd8d4
Compare
calbera
reviewed
May 20, 2026
calbera
reviewed
May 20, 2026
fridrik01
force-pushed
the
fix-engine-4xx-retry-loop
branch
from
May 20, 2026 18:18
abbd8d4 to
fd10658
Compare
calbera
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR Introduces a typed
rpc.HTTPStatusErrorso the EL's HTTP status code propagates through tohandleRPCError. Previously,callRawcollapsed every non-200 response intofmt.Errorf("unexpected status code %d: ..."), which didn't satisfy thejsonrpc.Errorinterface and so fell into the catch-allErrBadConnection(retryable) bucket.With the typed error in place,
handleRPCErrornow classifies HTTP 4xx as fatal (ErrHTTPClientError) and leaves 5xx and other transport failures retryable (ErrBadConnection), so the engine no longer retries forever on rejections the EL won't accept on a second try.Will likely made a follow-up PR which will split the engine retry policy between
ProcessProposal(bounded retry where a failing proposal should be rejected so consensus can move on) andFinalizeBlock(unbounded retry where the block is already agreed, must be applied).