Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the RPC server's startup behavior by deferring HTTP server listening until after initial bank indices are populated. Previously, the server would start listening immediately and return HTTP 500 errors for requests that arrived before the node finished booting. Now, the server doesn't begin accepting connections until all three bank indices (processed, confirmed, and finalized) have been initialized with valid values.
Changes:
- Deferred HTTP server listening from privileged_init to before_credit function, which waits for all bank indices to be initialized
- Added new error constant
FD_HTTP_SERVER_CONNECTION_CLOSE_STILL_BOOTINGto the connection close reasons (renumbering all subsequent WebSocket error codes) - Replaced runtime error handling with FD_TEST assertions for bank index validation, since these values are guaranteed to be valid once the server starts listening
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/waltz/http/fd_http_server.h | Added FD_HTTP_SERVER_CONNECTION_CLOSE_STILL_BOOTING constant and renumbered subsequent WebSocket error constants |
| src/waltz/http/fd_http_server.c | Added human-readable string representation for the new error constant |
| src/discof/rpc/fd_rpc_tile.c | Added boot state tracking fields, moved server listen to before_credit callback, replaced ULONG_MAX error returns with FD_TEST assertions, added conditional debug logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d37e324 to
2ed7ded
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
02fdf3d to
939e57f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
939e57f to
128b9d1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
128b9d1 to
9540922
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9540922 to
3fb5954
Compare
3fb5954 to
673745a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
673745a to
a60ef55
Compare
b6a6eb0 to
ef41e5f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ef41e5f to
f636fb7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f636fb7 to
a8f6eb3
Compare
a8f6eb3 to
bcc6f7a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bcc6f7a to
aa35069
Compare
aa35069 to
17f8c31
Compare
304dcbd to
e5359d1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
src/discof/rpc/fd_rpc_tile.c:1782
- This still calls
fd_http_server_listen()duringprivileged_init, which means the RPC port will be listening immediately. That conflicts with the nearby comments (and PR description) that listening should be delayed until replay banks are available. If the intent is to match Agave by refusing connections during boot, consider moving thelistencall to the point where banks are initialized (e.g. after receiving the firstREPLAY_SIG_SLOT_COMPLETED/ setting the processed/confirmed/finalized indices) and only then start polling/accepting.
extern char const fdctl_version_string[];
src/discof/rpc/fd_rpc_tile.c:730
- This error response uses
"code":32065(positive). JSON-RPC error codes should be negative here (and the rest of this file uses-32065for Firedancer-specific errors). As written, clients/tests that match on the error code will see a different value than intended.
*bank_idx = _bank_idx;
src/discof/rpc/fd_rpc_tile.c:224
is_bootingis introduced with a comment that the server won't start listening until it becomes false, but there is no logic in this file that reads or updatesctx->is_booting. Either wire this into the actual startup/listen gating (and clear it once banks are initialized) or remove it to avoid misleading state that suggests behavior that isn't implemented.
fd_http_server_t * http;
fd_rpc_cluster_node_dlist_t * cluster_nodes_dlist;
fd_rpc_cluster_node_t cluster_nodes[ FD_CONTACT_INFO_TABLE_SIZE ];
e5359d1 to
2766f15
Compare
closes #7885