Skip to content

handle same port values for API and metrics endpoints#345

Merged
pablodeymo merged 7 commits intolambdaclass:mainfrom
d4m014:refactor/merge-api-and-metrics-servers
May 7, 2026
Merged

handle same port values for API and metrics endpoints#345
pablodeymo merged 7 commits intolambdaclass:mainfrom
d4m014:refactor/merge-api-and-metrics-servers

Conversation

@d4m014
Copy link
Copy Markdown
Contributor

@d4m014 d4m014 commented May 5, 2026

🗒️ Description

merged both start_metrics_server and start_api_server functions into one start_rpc_server receiving RpcConfig struct:

pub struct RpcConfig {
    pub http_address: IpAddr,
    pub api_port: u16,
    pub metrics_port: u16,
}

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 struct RpcConfig, start_metrics_server and start_api_server replaced with a single start_rpc_server.
  • bin/ethlambda/src/main.rs: import RpcConfig and update callers.

Related Issues / PRs

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR merges the separate start_api_server and start_metrics_server functions into a single start_rpc_server that inspects a new RpcConfig struct and either serves both routers on one port (when the ports match) or binds two independent listeners (when they differ). However, the PR was submitted with unresolved git stash pop conflict markers in both changed files, so it does not compile in its current state.

  • Unresolved conflict markers in both crates/net/rpc/src/lib.rs and bin/ethlambda/src/main.rs leave the old and new server implementations interleaved with raw <<<<<<</=======/>>>>>>> delimiters, causing an immediate syntax error.
  • Dangling variable referencesshutdown_token, api_handle, and metrics_handle are referenced in the shutdown block of main.rs but are only defined inside the discarded upstream half of the conflict.
  • Graceful shutdown removedstart_rpc_server no longer accepts a CancellationToken, dropping the .with_graceful_shutdown behavior that the old functions provided.

Confidence Score: 1/5

Not safe to merge — both files contain raw conflict markers and will not compile.

Both changed files were submitted with unresolved git stash pop conflict markers, making the entire changeset uncompilable. On top of that, the new start_rpc_server silently drops graceful shutdown, and main.rs references variables (shutdown_token, api_handle, metrics_handle) that only exist in the discarded half of the conflict. The core idea of the PR is sound, but none of it can be validated until the conflicts are properly resolved and shutdown support is restored.

Both crates/net/rpc/src/lib.rs and bin/ethlambda/src/main.rs need conflict resolution before anything else can be reviewed.

Important Files Changed

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]
Loading
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

Comment thread crates/net/rpc/src/lib.rs Outdated
Comment thread bin/ethlambda/src/main.rs Outdated
Comment thread crates/net/rpc/src/lib.rs Outdated
Comment thread bin/ethlambda/src/main.rs Outdated
Copy link
Copy Markdown
Collaborator

@pablodeymo pablodeymo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to modifiy this code

Copy link
Copy Markdown
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some suggestions

Comment thread bin/ethlambda/src/main.rs Outdated
Comment thread bin/ethlambda/src/main.rs Outdated
@pablodeymo
Copy link
Copy Markdown
Collaborator

Thanks for picking this up! The same-port case looks correct and the graceful-shutdown handling reads cleanly. A few things before merge:

  1. CI lint is red on cargo fmt --check — running cargo fmt --all and pushing should fix it. Two diffs:
    • bin/ethlambda/src/main.rs: use ethlambda_rpc::RpcConfig; should come before the ethlambda_storage import (matches the suggestion already left in this PR).
    • crates/net/rpc/src/lib.rs: the Router::new().merge(api_router).merge(metrics_router).merge(debug_router); chain needs to be split across multiple lines.
  2. crates/net/rpc/src/lib.rs:68 — the docstring still says (see start_api_server); please update to start_rpc_server.
  3. Optional: derive #[derive(Debug, Clone)] on RpcConfig.

The greptile P0 (conflict markers) and P1 (missing graceful shutdown) comments are stale — they applied to earlier commits, not the current head.

@d4m014
Copy link
Copy Markdown
Contributor Author

d4m014 commented May 6, 2026

Thanks for picking this up! The same-port case looks correct and the graceful-shutdown handling reads cleanly. A few things before merge:

  1. CI lint is red on cargo fmt --check — running cargo fmt --all and pushing should fix it. Two diffs:

    • bin/ethlambda/src/main.rs: use ethlambda_rpc::RpcConfig; should come before the ethlambda_storage import (matches the suggestion already left in this PR).
    • crates/net/rpc/src/lib.rs: the Router::new().merge(api_router).merge(metrics_router).merge(debug_router); chain needs to be split across multiple lines.
  2. crates/net/rpc/src/lib.rs:68 — the docstring still says (see start_api_server); please update to start_rpc_server.

  3. Optional: derive #[derive(Debug, Clone)] on RpcConfig.

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!

@pablodeymo pablodeymo merged commit 5e805ee into lambdaclass:main May 7, 2026
2 checks passed
@MegaRedHand
Copy link
Copy Markdown
Collaborator

Thanks for your contribution! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle same port values for API and metrics endpoints

3 participants