From 981c4c81cb7fba84f9182d9b8e2c10fe9481c0f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 9 Jul 2026 14:25:10 +0000 Subject: [PATCH] starknet_transaction_prover: avoid redundant request-id re-parse in RequestSpanLayer RequestSpanLayer re-parsed and re-validated the x-request-id header on every plaintext request via extract_or_generate_request_id, even though RequestLogLayer (upstream in the same chain) already computed and validated that exact id moments earlier. RequestLogLayer now stashes the validated id in a RequestId request extension; RequestSpanLayer reads it directly, falling back to re-deriving it only when used standalone (e.g. without RequestLogLayer upstream, as in some unit tests). Follow-up to #14222. --- .../src/server/request_log.rs | 8 ++++ .../src/server/request_span.rs | 16 +++++--- .../src/server/request_span_test.rs | 40 ++++++++++++++++++- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/crates/starknet_transaction_prover/src/server/request_log.rs b/crates/starknet_transaction_prover/src/server/request_log.rs index 079dc32cdbb..a9899df1cc6 100644 --- a/crates/starknet_transaction_prover/src/server/request_log.rs +++ b/crates/starknet_transaction_prover/src/server/request_log.rs @@ -41,6 +41,13 @@ mod request_log_test; /// HTTP header carrying the request id. pub const REQUEST_ID_HEADER: &str = "x-request-id"; +/// Request extension carrying the id this layer already validated/generated, +/// so a downstream layer (e.g. `RequestSpanLayer`) can reuse it on the +/// plaintext path instead of re-parsing and re-validating the header it just +/// set. +#[derive(Clone)] +pub(crate) struct RequestId(pub String); + /// tower [`Layer`] producing [`RequestLogService`]. #[derive(Clone, Copy, Default)] pub struct RequestLogLayer; @@ -78,6 +85,7 @@ where let request_id = extract_or_generate_request_id(&request); let id_header_value = request_id_header_value(&request_id); request.headers_mut().insert(REQUEST_ID_HEADER, id_header_value.clone()); + request.extensions_mut().insert(RequestId(request_id.clone())); let is_health_probe = request.method() == Method::GET && request.uri().path() == HEALTH_PATH; let method = request.method().clone(); diff --git a/crates/starknet_transaction_prover/src/server/request_span.rs b/crates/starknet_transaction_prover/src/server/request_span.rs index 413a1d205bf..aa75a6fa5db 100644 --- a/crates/starknet_transaction_prover/src/server/request_span.rs +++ b/crates/starknet_transaction_prover/src/server/request_span.rs @@ -24,6 +24,7 @@ use crate::server::request_log::{ extract_or_generate_request_id, new_request_id, request_id_header_value, + RequestId, REQUEST_ID_HEADER, }; @@ -69,11 +70,16 @@ where request.headers_mut().insert(REQUEST_ID_HEADER, request_id_header_value(&fresh_id)); fresh_id } else { - // Re-derives, via the shared validator, the exact id - // `RequestLogLayer` already assigned — the header is left - // untouched. This also keeps the layer correct standalone, - // e.g. in unit tests without `RequestLogLayer` upstream. - extract_or_generate_request_id(&request) + // Reuses the id `RequestLogLayer` already validated/generated via + // its request extension, avoiding a second header parse and + // validation pass per request. Falls back to re-deriving it (the + // header is left untouched either way) so the layer stays correct + // standalone, e.g. in unit tests without `RequestLogLayer` + // upstream. + request.extensions().get::().map_or_else( + || extract_or_generate_request_id(&request), + |request_id| request_id.0.clone(), + ) }; self.inner.call(request).instrument(info_span!("http_request", request_id = %request_id)) } diff --git a/crates/starknet_transaction_prover/src/server/request_span_test.rs b/crates/starknet_transaction_prover/src/server/request_span_test.rs index f800cde5d5c..d242edbbd35 100644 --- a/crates/starknet_transaction_prover/src/server/request_span_test.rs +++ b/crates/starknet_transaction_prover/src/server/request_span_test.rs @@ -1,12 +1,13 @@ use bytes::Bytes; -use http::{Method, Request}; +use http::{Method, Request, Response, StatusCode}; use http_body_util::Full; use jsonrpsee::server::HttpBody; use tower::{Layer, ServiceExt}; use tower_ohttp::Decapsulated; +use tracing_test::traced_test; use crate::server::middleware_test_utils::{echo_request_id_service, read_body_and_headers}; -use crate::server::request_log::{RequestLogLayer, REQUEST_ID_HEADER}; +use crate::server::request_log::{RequestId, RequestLogLayer, REQUEST_ID_HEADER}; use crate::server::request_span::RequestSpanLayer; #[tokio::test] @@ -43,6 +44,41 @@ async fn decapsulated_gets_fresh_id_discarding_inbound() { assert!(uuid::Uuid::parse_str(&id).is_ok(), "must mint a fresh UUID, got {id:?}"); } +/// Proves the fast path actually reads the `RequestId` extension instead of +/// re-parsing the header: the two are set to different values here, which +/// `RequestLogLayer` never lets happen in production, and the span (checked +/// via a log line emitted inside it) must carry the extension's value, not +/// the header's. +#[tokio::test] +#[traced_test] +async fn plaintext_prefers_request_id_extension_over_header() { + let mut request = Request::builder() + .method(Method::POST) + .uri("/") + .header(REQUEST_ID_HEADER, "header-value") + .body(HttpBody::new(Full::new(Bytes::new()))) + .expect("static body is infallible"); + request.extensions_mut().insert(RequestId("extension-value".to_string())); + + let logging_service = tower::service_fn(|_request: Request| async move { + tracing::info!("handler invoked"); + Ok::<_, std::convert::Infallible>( + Response::builder() + .status(StatusCode::OK) + .body(HttpBody::new(Full::new(Bytes::new()))) + .expect("static body is infallible"), + ) + }); + + RequestSpanLayer.layer(logging_service).oneshot(request).await.unwrap(); + + assert!(logs_contain("extension-value"), "span must carry the RequestId extension's value"); + assert!( + !logs_contain("header-value"), + "the mismatched header value must not leak into the span" + ); +} + /// The cross-layer plaintext contract: with `RequestLogLayer` (outer) stacked /// over `RequestSpanLayer` (inner) and no inbound id, the id the outer layer /// generates and echoes on the response must be the same id the inner layer