[r3.6] cl/beacon: report a syncing node as 503, not 500 - #23608
Open
lystopad wants to merge 1 commit into
Open
Conversation
Fixes #23462. A node with no head state — starting up, restarting, catching up — answered **500 Internal Server Error** on endpoints where `beacon-APIs` declares **503 CurrentlySyncing**. It is transiently unavailable, not faulty, and 500 reads as a node fault to operators and alerting. Same node, same cause, two different codes today: `POST /eth/v1/beacon/pool/attestations` already returns 503 (`pool.go` sets it explicitly), while the committee subscription endpoints return 500. That inconsistency is what makes this an oversight rather than a decision. ## Change Two paths fed the same wrong code. **The sentinel was discarded.** `AddAttestationSubscription` built a fresh error, so nothing downstream could classify it: ```go - return errors.New("head state not available") + return synced_data.ErrNotSynced ``` **The central mapper had no case for it.** `WrapEndpointError` already special-cases one sentinel, so this sits next to it — one change covering every endpoint that surfaces `ErrNotSynced` raw (`getAttesterDuties`, `getCommittees`, `PostEthV1BeaconPoolSyncCommittees`, `blockRootFromBlockId`, `PostEthV1ValidatorSyncCommitteeSubscriptions`): ```go if errors.Is(err, fork_graph.ErrStateNotFound) { return NewEndpointError(http.StatusNotFound, ErrorCantFindBeaconState) } +if errors.Is(err, synced_data.ErrNotSynced) { + return NewEndpointError(http.StatusServiceUnavailable, err) +} ``` Both subscription handlers now route through `WrapEndpointError` instead of hardcoding 500, and the syncing case no longer logs at `Error` — a node that is merely catching up should not fill the log with failures. Also in this PR: - **`WrapEndpointError` matched `*EndpointError` by value.** `errors.As` with a value target never matches the pointer `NewEndpointError` returns, so an explicit code reaching this helper collapsed to 500. `HandleEndpoint` checks the pointer form first, so **no endpoint hit this in practice** — it is latent, not live, and it is a trap for the direct callers this PR adds. Both forms now match. - **`ApiHandler.committeeSub` held the concrete `*CommitteeSubscribeMgmt`**, so the already-generated `MockCommitteeSubscribe` could not be injected. It now takes the existing `CommitteeSubscribe` interface. No caller changes — the concrete type satisfies it. - Removed a commented-out block in `subscription.go` that proposed this exact 503 check. ## Spec [`types/http.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/types/http.yaml) — `CurrentlySyncing`: *"Beacon node is currently syncing, try again later."* Declared on two affected endpoints: [`beacon_committee_subscriptions.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/beacon_committee_subscriptions.yaml) and [`duties/attester.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/duties/attester.yaml). **Worth a reviewer's opinion:** the spec does *not* declare 503 for `states/{state_id}/committees`, `pool/sync_committees` or `sync_committee_subscriptions`, which the central mapping also changes. I went with consistency — the condition is equally reachable there, the error body shape is unchanged, and only strict response-code validation would notice. Happy to narrow it to the two spec-declared endpoints if you would rather stay literal. ## Scope This is a conformance and observability fix, not a high-availability one. Lighthouse does not branch on 503 anywhere in its fallback or publish path — it derives health from `/eth/v1/node/health` and `/eth/v1/node/syncing`, and both codes arrive as the same `RequestFailed(ServerMessage(..))`. Client routing behaviour will not change. ## Test TDD, and each guard was verified by mutation rather than by reading: | Mutation | Test that caught it | | --- | --- | | 503 mapping → 500 | `TestWrapEndpointErrorStatusCodes`, both subscription 503 tests | | sentinel → bare `errors.New` | `TestAddAttestationSubscriptionReportsNotSyncedWhileSyncing` | | pointer match removed | `TestWrapEndpointErrorStatusCodes/explicit_code_is_preserved` | The handler tests mock the subscription call, so they cannot catch a regression in `committee_subscription.go` — hence the separate package-level test for the sentinel. Coverage also pins the cases that must *not* change: an unrelated failure still returns 500, and a synced node still returns 200. `./cl/beacon/... ./cl/validator/...` green (18 packages). `make lintci` reports `0 issues`. (cherry picked from commit ef0a6ce)
AskAlexSharov
approved these changes
Aug 27, 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
3 participants
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.
Cherry-pick of #23464 to release/3.6. Refs #23462.
Applies cleanly — no release-branch adaptations needed.
A node with no head state (starting up, restarting, catching up) answered 500 Internal Server Error where
beacon-APIsdeclares 503 CurrentlySyncing. It is transiently unavailable, not faulty, and 500 reads as a node fault to operators and alerting.The same node already returned 503 on
POST /eth/v1/beacon/pool/attestationswhile returning 500 on the committee subscription endpoints for the identical cause, which is what marks this as an oversight rather than a decision.Scope
This is a conformance and observability fix, not a behavioural one for validator clients. Lighthouse does not branch on 503 anywhere in its fallback or publish path — health comes from
/eth/v1/node/healthand/eth/v1/node/syncing, and both codes arrive as the sameRequestFailed(ServerMessage(..)). Client routing will not change.What it does change: a restarting node stops producing spurious 500s in error dashboards during rolling upgrades, and Caplin no longer logs the syncing case at
Error.Worth a reviewer's note, carried over from the main PR: the spec declares 503 on two of the affected endpoints (
beacon_committee_subscriptions,duties/attester), while the central mapping also coversstates/{state_id}/committees,pool/sync_committeesandsync_committee_subscriptions, where beacon-APIs lists no 503. The main PR went with consistency — same condition, same code, unchanged error body.Test
Verified on this branch rather than inherited from main.
./cl/beacon/... ./cl/validator/...green (18 packages), and each guard re-checked by mutation here:TestWrapEndpointErrorStatusCodes, both subscription 503 testserrors.NewTestAddAttestationSubscriptionReportsNotSyncedWhileSyncingTestWrapEndpointErrorStatusCodes/explicit_code_is_preservedmake lintcirun against a cleared lint cache.