Switch to client-side GRPC retry interceptor #917
Merged
+79
−22
Conversation
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
jribbink
requested review from
chasefleming,
janezpodhostnik,
mfbz and
peterargue
as code owners
jribbink
force-pushed
the
jribbink/revert-bad-changes
branch
2 times, most recently
from
3b18b74 to
b92241f
Compare
jribbink
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the native gRPC retry mechanism with a custom client-side retry interceptor to address issues where Flow public access nodes instruct clients not to retry despite having resources available. The implementation includes comprehensive logging and error handling improvements.
Key Changes:
- Implemented custom gRPC retry interceptor with exponential backoff in
utils/grpc.go - Updated all gRPC client instantiations to use the new retry interceptor
- Enhanced error handling by changing
NewEmulatorServerto return errors instead of nil
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/grpc.go | Replaced service config constant with client-side retry interceptor implementation |
| storage/remote/store.go | Updated to use new retry interceptor instead of service config |
| storage/checkpoint/checkpoint_test.go | Changed test logger to Nop logger |
| server/server_test.go | Updated test error handling for new NewEmulatorServer signature |
| server/server.go | Changed NewEmulatorServer to return errors; enhanced logging in DetectRemoteChainID |
| server/fork_integration_test.go | Updated tests for new error handling and added buffer to fork height |
| cmd/emulator/start/start.go | Updated error handling for NewEmulatorServer |
| .github/workflows/ci.yml | Removed GRPC_GO_RETRY environment variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| const ( | ||
| defaultMaxAttempts = 10 |
Member
There was a problem hiding this comment.
Should these be configurable for any reason?
Contributor
Author
There was a problem hiding this comment.
It could be fine tuned, but imo this is a good enough general number for now.
jribbink
force-pushed
the
jribbink/revert-bad-changes
branch
from
c7e0d81 to
c099e94
Compare
chasefleming
approved these changes
Nov 18, 2025
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.
The previous PR #916 unintentionally committed changes via auto-merge. This PR corrects these.
It was discovered that the built-in GRPC retry mechanism does not always work with Flow public access nodes, as it instructs clients not to retry despite eventually having sufficient resources to reschedule requests. For this reason, I have just switched this to a simple client middleware.
For contributor use:
masterbranchFiles changedin the GitHub PR explorer