refactor(api): serve espresso-node API via axum (rm tide 1/5)#4703
refactor(api): serve espresso-node API via axum (rm tide 1/5)#4703sveitser wants to merge 14 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
Switch espresso-node's serve modes (full, fs, status-only, bare) to the in-repo axum server. The tide-disco App is still constructed so the module definitions keep compiling; it no longer binds the API port. Restores tide-disco wire parity on the axum server: tide-compatible paths and /v0 URI rewrites (without the unconsumed v0 legacy availability formats), reward-state and namespace proof endpoints, tide 404/400 statuses on reward endpoints, accept-negotiated JSON/vbs healthcheck bodies (AppHealth shape at top level, bare HealthStatus per module), and ws Close frames on stream completion.
41fb01c to
041b34a
Compare
The separate axum_port option was removed when axum took over the main ESPRESSO_NODE_API_PORT; these .env entries have no consumer.
|
Claude finished @sveitser's task in 5m 56s —— View job Code ReviewI reviewed this against
What I verifiedReward-status classification (
Error envelope ( Healthcheck wire shapes ( Legacy URI rewrite ( WS close handshake ( Minor observations (non-blocking)
Nice work on the parity comments throughout — they make this migration reviewable, and I couldn't find a wire-shape regression. |
|
Problems found so far
|
Convert the 15 v1 module routers to aide's ApiRouter and annotate all 171 routes with summaries and descriptions lifted from the legacy tide-disco toml specs, restoring the docs that died with the tide App. Serve the spec at /v1/docs/openapi.json with Swagger UI at /v1 and Scalar at /v1/scalar. Point / at /v1 (temporary redirect) while the v2 API is a work in progress. Wire-neutral: ApiJson delegates to axum::Json, so response bytes, statuses, and headers are unchanged; the OpenAPI spec reports untyped 200 responses instead of adding JsonSchema derives to domain types.
The docs routes and OpenAPI finish lived only in create_router_v1, which no serve mode calls: the serve modes compose module routers directly, so the running node 404'd on /v1 while the unit tests passed. Extract finish_v1_docs() and route all four serve modes (full, fs, status, bare) through it; the generated spec now reflects exactly the modules each mode mounts. Regression test assembles the router the way serve_axum_status does, wrapped in the production top-level-routes and legacy-URI-rewrite layers.
aide only derives path parameters from named-struct Path<T>
extractors; the v1 handlers all use primitives and tuples, so every
operation showed zero parameters and Swagger's try-it-out could not
fill the URL templates. Declare a required string parameter for every
{name} template segment after finish_api (string because primitive
extractors carry no type metadata; handlers parse).
Also: Swagger UI opens operations with try-it-out enabled; the
test_api example moves to port 5001 (macOS AirPlay holds 5000) and
its startup log lists the v1 doc URLs.
Numeric template segments (height, from/until, epoch, view, index, limit, offset, namespace, finalized, ...) are declared as unsigned integers, matching each handler's Path<T> extractor; hash, key, and address segments stay strings. Unknown future names default to string. Documentation-only: handlers parse the raw segment regardless.
The first path segment after /v1/ becomes the operation's OpenAPI tag, so Swagger renders one collapsible section per module (availability, status, catchup, ...), sorted.
Palette from the staking UI: cream background, coffee headings, cookie-brown GET and orange POST accents; topbar hidden.
| /// `namespaces` is the raw `TaggedBase64`-encoded path segment produced by the light-client | ||
| /// client (tag `NS`, wrapping a JSON `Vec<u64>`); decoding it is left to the implementation | ||
| /// so this crate does not need a `tagged-base64` dependency. | ||
| async fn get_lc_namespaces_proof_range( |
There was a problem hiding this comment.
why is this added in this PR?
| >; | ||
|
|
||
| async fn get_reward_state_height(&self) -> anyhow::Result<u64> { | ||
| use hotshot_query_service::merklized_state::MerklizedStateHeightPersistence; |
There was a problem hiding this comment.
nit: can we remove this in function import
| } | ||
|
|
||
| async fn get_reward_state_v2_height(&self) -> anyhow::Result<u64> { | ||
| // Both the V1 and V2 reward merklized-state modules share the same underlying |
There was a problem hiding this comment.
actually this comment can be made better by mentioning that the last_merklized_state_height is the same for all the state modules
| snapshot: espresso_api::v1::Snapshot, | ||
| key: String, | ||
| ) -> anyhow::Result<Self::RewardStatePathV1> { | ||
| use hotshot_query_service::merklized_state::{ |
| HsSnapshot::Commit(commit) | ||
| }, | ||
| }; | ||
| let key: espresso_types::v0_3::RewardAccountV1 = key |
There was a problem hiding this comment.
also it would make reading code easier if we can remove the full import from some places
| let mut router = axum::with_top_level_routes(router); | ||
| if let Some(limit) = max_connections { | ||
| router = router.layer( | ||
| tower::ServiceBuilder::new() |
There was a problem hiding this comment.
does this limit also apply to streaming endpoints?
| let mut router = axum::with_top_level_routes(router); | ||
| if let Some(limit) = max_connections { | ||
| router = router.layer( | ||
| tower::ServiceBuilder::new() |
There was a problem hiding this comment.
does this limit also apply to streaming endpoints?
| .with_state(state) | ||
| } | ||
|
|
||
| pub(crate) fn router_availability<S>(state: S) -> ApiRouter |
There was a problem hiding this comment.
hmmm this file is too big. I am wondering if these routes registration should be moved to where we define the traits or maybe into their own modules. So like the structure would be
axum/mod.rs
axum/availability.rs, axum/node.rs ...
axum/tests.rs
| "/v1/reward-state-v2/reward-amounts/{height}/{offset}/{limit}"; | ||
| pub const REWARD_MERKLE_TREE_V2_ROUTE: &str = | ||
| "/v1/reward-state-v2/reward-merkle-tree-v2/{height}"; | ||
| pub const REWARD_STATE_HEIGHT_ROUTE: &str = "/v1/reward-state/block-height"; |
There was a problem hiding this comment.
The version is hardcoded into each route path. Previously we could simply bump the version, and all the routes would automatically use the new version. How would this work with axum now?
There was a problem hiding this comment.
We could use this? https://docs.rs/axum/latest/axum/struct.Router.html#method.nest
But I don't really want to rewrite even more in this PR.
Review feedback: 44 in-function use statements move to the file header; long v0_3/v0_4 turbofish paths shortened via imports; the shared last_merklized_state_height comment now names all state modules.
Review feedback: the macro now stringifies the parameter ident instead
of taking a redundant string. Also fixes namespace_proof_by_payload_hash,
whose "payload-hash" literal never matched the {payload_hash}
placeholder, leaving it unsubstituted in built paths.
Review findings. tower's concurrency permit is released at the 101 upgrade, so websockets escaped ESPRESSO_NODE_API_MAX_CONNECTIONS entirely; a shared semaphore now gives plain requests a slot while in flight and streaming sockets one for their lifetime, with a real-TCP regression test. The API port also binds before router composition: OpenAPI generation takes ~0.5s in debug builds, and clients connecting during it now queue in the accept backlog instead of being refused. This is what broke the test-postgres shards: first-request test helpers panic on a refused connection.
drive_ws_stream only noticed a dead client on the next send, so a socket whose stream was quiet held its max_connections slot and its stream task forever; reconnect churn exhausted the demo's budget of 25 and every plain request got 429, killing the native-demo e2e tests (claim-rewards-loop exits on non-404 and tears the demo down). Poll the client side with select so disconnects release the slot and end the task immediately. Regression test now uses a never-yielding stream.
The demo's nasty-client holds up to 100 open streams per resource type by design, against a request budget of 25; counting sockets for their lifetime starved every plain request on node-1 with 429 and killed the native-demo e2e tests. max_connections goes back to bounding in-flight requests only (as in every green run); a socket's slot is released at the 101 upgrade. Stream tasks still end promptly on client disconnect, which was the actual leak.
2 participants
Re-slice of #4592 (reference branch
ss/remove-tide).all serve modes; the tide App is still built but no longer binds
reward-state and ns-proof endpoints, tide 404/400 reward statuses,
accept-negotiated JSON/vbs healthchecks, ws Close frames
Review focus: axum.rs error envelope and healthcheck wire shapes;
reward-status classification in api/state.rs; local single-variant
HealthStatus (moves to http-client in 4/5).