-
-
Notifications
You must be signed in to change notification settings - Fork 475
fix(sync): harden range req/resp retries and envelope completeness #8995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1ba3edb
4e50a11
91f2485
a06b070
624a03d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,12 @@ const multiStreamSelectErrorCodes = { | |||||
| protocolSelectionFailed: "protocol selection failed", | ||||||
| }; | ||||||
|
|
||||||
| const reqRespRateLimitErrorMessages = [ | ||||||
| RequestErrorCode.REQUEST_RATE_LIMITED, | ||||||
| RequestErrorCode.REQUEST_SELF_RATE_LIMITED, | ||||||
| RequestErrorCode.RESP_RATE_LIMITED, | ||||||
| ] as const; | ||||||
|
|
||||||
| export function onOutgoingReqRespError(e: RequestError, method: ReqRespMethod): PeerAction | null { | ||||||
| switch (e.type.code) { | ||||||
| case RequestErrorCode.INVALID_REQUEST: | ||||||
|
|
@@ -27,7 +33,9 @@ export function onOutgoingReqRespError(e: RequestError, method: ReqRespMethod): | |||||
| return PeerAction.LowToleranceError; | ||||||
|
|
||||||
| case RequestErrorCode.SERVER_ERROR: | ||||||
| return PeerAction.MidToleranceError; | ||||||
| return reqRespRateLimitErrorMessages.some((errMessage) => e.message.includes(errMessage)) | ||||||
|
||||||
| return reqRespRateLimitErrorMessages.some((errMessage) => e.message.includes(errMessage)) | |
| return reqRespRateLimitErrorMessages.some((errMessage) => e.type.errorMessage?.includes(errMessage)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable suggestion. The e.message check is pre-existing code from unstable (not introduced in this PR). Using e.type.errorMessage would be slightly more robust, but both paths produce identical results since RequestError.message includes the errorMessage field. Not changing pre-existing patterns in this scoped PR — can be picked up in a general reqresp cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these introduced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These (
reqRespRateLimitErrorMessagesarray + theSERVER_ERRORcheck) are part of the rate-limit backoff handling. Onepbs-devnet-0, peers frequently returnSERVER_ERRORwith rate-limit messages when hammered with by-range requests during sync. Without this, every rate-limited response penalizes the peer withMidToleranceError, which quickly burns through all peers in a small devnet.The change distinguishes rate-limit
SERVER_ERROR(skip penalty, let the backoff/retry handle it) from genuine server errors (still penalized). This was essential for syncingepbs-devnet-0where the peer set is small (~5 nodes).On a stable mainnet-scale network it would still be beneficial — rate-limiting is a cooperative signal, not adversarial behavior, so penalizing peers for it is counterproductive regardless of network size.