handle same port values for API and metrics endpoints#345
handle same port values for API and metrics endpoints#345pablodeymo merged 7 commits intolambdaclass:mainfrom
Conversation
Greptile SummaryThis PR merges the separate
Confidence Score: 1/5Not safe to merge — both files contain raw conflict markers and will not compile. Both changed files were submitted with unresolved Both
|
| Filename | Overview |
|---|---|
| crates/net/rpc/src/lib.rs | Introduces RpcConfig and start_rpc_server that merges API+metrics onto one port when they match, but the file contains unresolved git conflict markers (both old and new server functions coexist) and the new function drops graceful shutdown support. |
| bin/ethlambda/src/main.rs | Updated to import RpcConfig and call start_rpc_server, but contains unresolved conflict markers; post-conflict code still references shutdown_token, api_handle, and metrics_handle that only exist in the discarded upstream half, causing a second independent compilation failure. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[start_rpc_server called] --> B{api_port == metrics_port?}
B -- Yes --> C[Merge api_router + metrics_router + debug_router]
C --> D[Bind single TcpListener on api_port]
D --> E[axum::serve — single combined server]
B -- No --> F[Bind api_listener on api_port]
F --> G[Bind metrics_listener on metrics_port]
G --> H[tokio::try_join! both axum::serve calls]
E & H --> I[Return Ok / propagate Err]
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
crates/net/rpc/src/lib.rs:20-82
**Unresolved git conflict markers — file will not compile**
Both `crates/net/rpc/src/lib.rs` and `bin/ethlambda/src/main.rs` still contain raw `<<<<<<< Updated upstream`, `=======`, and `>>>>>>> Stashed changes` conflict markers from a `git stash pop` (or rebase) that was never resolved. Rust will reject these as syntax errors on the very first `cargo build` or `cargo check`, so the PR is not mergeable in its current state. The conflict needs to be resolved so only one set of definitions remains (the new `RpcConfig` + `start_rpc_server` path, or the old two-server path).
### Issue 2 of 4
bin/ethlambda/src/main.rs:214-244
**Conflict markers also present in `main.rs`**
Same unresolved `<<<<<<< Updated upstream` / `>>>>>>> Stashed changes` conflict exists here. Additionally, the code _after_ the conflict block (lines 273–278) still references `shutdown_token`, `api_handle`, and `metrics_handle`, which are only defined inside the "Updated upstream" half of the conflict. If the intended resolution is the "Stashed changes" side (using `start_rpc_server`), those variables will be undefined and compilation will fail for a second independent reason.
### Issue 3 of 4
crates/net/rpc/src/lib.rs:61-81
**Graceful shutdown removed from `start_rpc_server`**
The old `start_api_server` and `start_metrics_server` both accepted a `CancellationToken` and called `.with_graceful_shutdown(…)` on `axum::serve`. The new `start_rpc_server` accepts no token and drives `axum::serve` without a shutdown hook. The call site in `main.rs` (`shutdown_token.cancel()` + awaiting the spawned handles) was designed to drain in-flight requests before the process exits; omitting it means a graceful Ctrl-C will now force-kill live connections instead of waiting for them to finish.
### Issue 4 of 4
bin/ethlambda/src/main.rs:115-119
**Inconsistent indentation in `RpcConfig` literal**
The struct fields are indented with 2 spaces while the surrounding codebase uses 4-space indentation. The closing brace `}` is also misaligned (it aligns with 2 spaces instead of the enclosing `let` statement). This won't affect behaviour but will cause `rustfmt` to reformat the block and may create noise in future diffs.
Reviews (1): Last reviewed commit: "handle same port values for API and metr..." | Re-trigger Greptile
pablodeymo
left a comment
There was a problem hiding this comment.
we need to modifiy this code
MegaRedHand
left a comment
There was a problem hiding this comment.
Left some suggestions
|
Thanks for picking this up! The same-port case looks correct and the graceful-shutdown handling reads cleanly. A few things before merge:
The greptile P0 (conflict markers) and P1 (missing graceful shutdown) comments are stale — they applied to earlier commits, not the current head. |
thanks for the feedback! i just drop a commit that fixes all issues, everything should run fine now! |
|
Thanks for your contribution! 😄 |
🗒️ Description
merged both
start_metrics_serverandstart_api_serverfunctions into onestart_rpc_serverreceivingRpcConfigstruct:while in that single function, handling whether to merge both API and metrics routers into a single endpoint if specified ports (API and metrics) are the same, or just start two separate servers as before if different ports were chosen.
What Changed
crates/net/rpc/src/lib.rs: new structRpcConfig,start_metrics_serverandstart_api_serverreplaced with a singlestart_rpc_server.bin/ethlambda/src/main.rs: importRpcConfigand update callers.Related Issues / PRs