Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional Submit HTTP API: workspace axum dependency, CLI/config wiring for a submit address, new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Args
participant Config as Config
participant Runtime as Node Runtime
participant API as Submit API Server
participant Mempool as ResourceMempool
participant Client as HTTP Client
CLI->>Config: provide submit_api_address
Config->>Runtime: include address in startup config
Runtime->>API: start_submit_api(address, running, exit)
API->>Mempool: obtain shared ResourceMempool<Transaction>
API->>API: bind socket & spawn server task
API-->>Runtime: return JoinHandle
Client->>API: POST /api/submit/tx (application/cbor)
API->>API: verify Content-Type and decode CBOR -> Transaction
API->>Mempool: validate & insert (TxOrigin::Local)
Mempool-->>API: ack + tx_id
API-->>Client: 202 Accepted + {tx_id}
Runtime->>API: shutdown signal (CancellationToken)
API-->>Runtime: graceful shutdown / task termination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/amaru/src/submit_api.rs (1)
49-50: Don't let the API die silently in the background task.
.await.ok()bins theaxum::serveerror, so a late listener failure just makes the submit API vanish with no breadcrumb in the logs. At least log the error inside the task, or return aJoinHandle<Result<_, _>>so shutdown can surface it.🪵 Proposed fix
-use tracing::info; +use tracing::{error, info}; @@ let handle = tokio::spawn(async move { - axum::serve(listener, app).with_graceful_shutdown(shutdown.cancelled_owned()).await.ok(); + if let Err(err) = axum::serve(listener, app) + .with_graceful_shutdown(shutdown.cancelled_owned()) + .await + { + error!(?err, "Submit API server stopped unexpectedly"); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/submit_api.rs` around lines 49 - 50, The background task currently swallows any axum::serve error by calling .await.ok(), so replace the silent discard with explicit error handling: inside the tokio::spawn that creates handle (the async block invoking axum::serve(listener, app).with_graceful_shutdown(shutdown.cancelled_owned())), await the serve call into a Result and either log the Err via the project's logger (so late listener failures are visible) or change the spawn to return a JoinHandle<Result<(), E>> so callers can propagate/surface the error; ensure you reference the same listener, app and shutdown.cancelled_owned() values when capturing the error handling change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru/src/bin/amaru/cmd/run.rs`:
- Around line 159-167: The current shutdown sequence cancels the parent token
then immediately calls handle.abort() on submit_api_handle (created by
start_submit_api and using amaru::exit::hook_exit_token), which can interrupt
Axum's graceful shutdown initiated by exit.cancelled().await; change the logic
so after exit.cancelled().await and running.abort() you await the
submit_api_handle (e.g., let _ = handle.await) to allow the server to finish
draining, or if a hard cutoff is required wrap that await in a
tokio::time::timeout to force-abort only after a fixed deadline instead of
calling handle.abort() immediately.
In `@crates/amaru/src/submit_api.rs`:
- Around line 72-74: The 202 branch is returning a plain String with manual
quotes which mislabels the response as text/plain; change the success branch
that handles mempool.insert(tx, TxOrigin::Local) to return an axum::Json-wrapped
value (so the tx_id is serialized properly as JSON) and remove manual quoting,
while the Err branch should return a plain unquoted error message (no wrapping
in Json) so it stays text/plain; also update any tests that assert quoted
response bodies to expect JSON for the success case and unquoted plain text for
errors; locate the match on mempool.insert and adjust the success tuple to use
Json(...) and the error tuple to remove format!("\"{...}\"").
---
Nitpick comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 49-50: The background task currently swallows any axum::serve
error by calling .await.ok(), so replace the silent discard with explicit error
handling: inside the tokio::spawn that creates handle (the async block invoking
axum::serve(listener, app).with_graceful_shutdown(shutdown.cancelled_owned())),
await the serve call into a Result and either log the Err via the project's
logger (so late listener failures are visible) or change the spawn to return a
JoinHandle<Result<(), E>> so callers can propagate/surface the error; ensure you
reference the same listener, app and shutdown.cancelled_owned() values when
capturing the error handling change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcfdd4df-faf8-4e6e-9e6f-c7bc628d54b8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/amaru/Cargo.tomlcrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/lib.rscrates/amaru/src/stages/config.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/mod.rsdocs/SUBMIT_API.md
94adca6 to
84f2d96
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/amaru/src/bin/amaru/cmd/run.rs (1)
165-167:⚠️ Potential issue | 🟠 MajorLet graceful shutdown finish instead of hard-aborting immediately.
At Line 165, aborting the submit API handle right after cancellation can chop Axum’s graceful drain mid-scene. Since
crates/amaru/src/submit_api.rsalready useswith_graceful_shutdown(...), await the handle (optionally with timeout) before forcing abort.🔧 Proposed fix
exit.cancelled().await; running.abort(); if let Some(handle) = submit_api_handle { - handle.abort(); + // Let Axum finish graceful shutdown triggered by the child cancellation token. + let _ = handle.await; }#!/bin/bash # Verify shutdown ordering between run.rs and submit_api.rs. rg -n -C3 'exit\.cancelled\(\)\.await|handle\.abort\(\)' crates/amaru/src/bin/amaru/cmd/run.rs rg -n -C3 'with_graceful_shutdown' crates/amaru/src/submit_api.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/bin/amaru/cmd/run.rs` around lines 165 - 167, The submit API JoinHandle (submit_api_handle) is being immediately aborted which can interrupt Axum’s with_graceful_shutdown; instead await the handle to allow graceful shutdown (with an overall timeout), and only call handle.abort() if the timeout elapses or the await errors. Replace the immediate handle.abort() in run.rs with awaiting the JoinHandle (e.g. tokio::time::timeout(Duration::from_secs(N), submit_api_handle).await) and on timeout or join error call submit_api_handle.abort(); keep references to submit_api_handle and the submit_api.rs with_graceful_shutdown usage so the graceful drain can complete.
🧹 Nitpick comments (1)
crates/amaru/src/submit_api.rs (1)
80-86: Consider an atomic validate+insert path in mempool.Lines 80–86 split validation and insertion into two calls. Under concurrent state changes, that opens a TOCTOU window between “valid” and “inserted.” A combined mempool API (e.g., validate-and-insert) would tighten correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/submit_api.rs` around lines 80 - 86, The current code calls mempool.validate_transaction(tx.clone()) and then mempool.insert(tx, TxOrigin::Local), which creates a TOCTOU race; implement and call an atomic validate-and-insert API (e.g., mempool.validate_and_insert(tx, TxOrigin::Local) or insert_with_validation) on the mempool trait/impl so validation and insertion happen under the same lock/transaction, and replace the two-step sequence with a single call that returns Ok((tx_id, seq_no)) or Err(e) to preserve the existing response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 69-70: The Content-Type check using content_type !=
"application/cbor" is case-sensitive and should be made case-insensitive; update
the conditional in submit_api.rs to compare the header using a case-insensitive
comparison (e.g., content_type.eq_ignore_ascii_case("application/cbor")) or
parse the header with a MIME parser and compare type/subtype case-insensitively
so requests like "Application/CBOR" are accepted and still return
StatusCode::UNSUPPORTED_MEDIA_TYPE via text_response when it doesn't match.
- Around line 50-52: The axum server result is being swallowed by `.await.ok()`,
so replace that silent ignore with explicit handling: await the future returned
by `axum::serve(listener,
app).with_graceful_shutdown(shutdown.cancelled_owned())`, capture the `Result`
inside the `tokio::spawn` task (the block where `handle` is assigned), and log
or propagate any `Err` (e.g., via `tracing::error!` or `process_logger`)
including the error details so server errors from `axum::serve` are visible;
ensure you reference the same `listener`, `app`, and
`shutdown.cancelled_owned()` used in the current `tokio::spawn` closure.
---
Duplicate comments:
In `@crates/amaru/src/bin/amaru/cmd/run.rs`:
- Around line 165-167: The submit API JoinHandle (submit_api_handle) is being
immediately aborted which can interrupt Axum’s with_graceful_shutdown; instead
await the handle to allow graceful shutdown (with an overall timeout), and only
call handle.abort() if the timeout elapses or the await errors. Replace the
immediate handle.abort() in run.rs with awaiting the JoinHandle (e.g.
tokio::time::timeout(Duration::from_secs(N), submit_api_handle).await) and on
timeout or join error call submit_api_handle.abort(); keep references to
submit_api_handle and the submit_api.rs with_graceful_shutdown usage so the
graceful drain can complete.
---
Nitpick comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 80-86: The current code calls
mempool.validate_transaction(tx.clone()) and then mempool.insert(tx,
TxOrigin::Local), which creates a TOCTOU race; implement and call an atomic
validate-and-insert API (e.g., mempool.validate_and_insert(tx, TxOrigin::Local)
or insert_with_validation) on the mempool trait/impl so validation and insertion
happen under the same lock/transaction, and replace the two-step sequence with a
single call that returns Ok((tx_id, seq_no)) or Err(e) to preserve the existing
response handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5237299-7f91-478c-9816-a978038fb71b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/amaru/Cargo.tomlcrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/lib.rscrates/amaru/src/stages/config.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/mod.rsdocs/SUBMIT_API.md
✅ Files skipped from review due to trivial changes (1)
- docs/SUBMIT_API.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/amaru/src/stages/config.rs
- Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/amaru/src/submit_api.rs (2)
50-52:⚠️ Potential issue | 🟠 MajorDon’t let the server fail off-screen.
That
.await.ok()bins the accept-loop error, so the submit API can keel over with zero log breadcrumbs. Please log theErrbefore the task exits.🔧 Suggested tweak
-use tracing::info; +use tracing::{error, info}; @@ let handle = tokio::spawn(async move { - axum::serve(listener, app).with_graceful_shutdown(shutdown.cancelled_owned()).await.ok(); + if let Err(err) = axum::serve(listener, app) + .with_graceful_shutdown(shutdown.cancelled_owned()) + .await + { + error!(error = %err, "Submit API server stopped with error"); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/submit_api.rs` around lines 50 - 52, The accept-loop error is currently being discarded by calling .await.ok() inside the spawned task; change the spawned task (the closure passed to tokio::spawn that calls axum::serve(listener, app).with_graceful_shutdown(shutdown.cancelled_owned()).await) to capture the Result from .await and log any Err before the task exits (e.g., replace the .ok() with a match or if let Err(e) branch and emit a tracing::error!/log::error! with context mentioning the submit API server and the error).
69-70:⚠️ Potential issue | 🟡 MinorMake the media-type check case-insensitive.
HTTP media types are case-insensitive, so
Application/CBORgets bounced here for no good reason.eq_ignore_ascii_casesorts it.🔧 Suggested tweak
- if content_type != "application/cbor" { + if !content_type.eq_ignore_ascii_case("application/cbor") { return text_response(StatusCode::UNSUPPORTED_MEDIA_TYPE, "Content-Type must be application/cbor"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/submit_api.rs` around lines 69 - 70, The media-type check in submit_api.rs currently uses a case-sensitive comparison on content_type; change the check to use a case-insensitive comparison (e.g., call content_type.eq_ignore_ascii_case("application/cbor")) where the current if uses != "application/cbor", so requests like "Application/CBOR" are accepted while keeping the existing UNSUPPORTED_MEDIA_TYPE response when it fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru/src/bin/amaru/cmd/run.rs`:
- Around line 156-160: If start_submit_api can fail after
build_and_run_node/metrics have started, ensure you roll them back on error:
replace the direct `?` on `start_submit_api(submit_api_address, &running,
&exit).await?` with an explicit match/if-let that, on Err(e), first stops the
running node (call the node handle's graceful shutdown method such as
`running.shutdown().await` or `running.abort()`/`running.stop()` if present) and
also stop/abort the metrics task created by
`meter_provider.clone().map(track_system_metrics).transpose()` (use whatever
handle `track_system_metrics` returns), then return the error; on Ok, continue
as before. Reference symbols:
`meter_provider.clone().map(track_system_metrics).transpose()`,
`build_and_run_node`, `running`, and `start_submit_api`.
In `@crates/amaru/src/submit_api.rs`:
- Around line 84-86: The match on mempool.insert currently maps all errors to
BAD_REQUEST; update the error handling in the submit path that calls
mempool.insert(tx, TxOrigin::Local) so that MempoolFull is mapped to a 503 (or
ServiceUnavailable) response to signal backpressure,
duplicate/AlreadyExists-style rejects map to 409 Conflict, and only genuine
client-side validation rejects continue to return 400; locate the match around
mempool.insert (the Ok((tx_id, _seq_no)) arm and the Err(e) arm) and branch on
the typed reject (e) — e.g., match on TxReject::MempoolFull =>
text_response(StatusCode::SERVICE_UNAVAILABLE, ...),
TxReject::Duplicate/AlreadyExists => text_response(StatusCode::CONFLICT, ...), _
=> text_response(StatusCode::BAD_REQUEST, e.to_string()) — preserving the
accepted path using json_response(StatusCode::ACCEPTED, tx_id.to_string()).
---
Duplicate comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 50-52: The accept-loop error is currently being discarded by
calling .await.ok() inside the spawned task; change the spawned task (the
closure passed to tokio::spawn that calls axum::serve(listener,
app).with_graceful_shutdown(shutdown.cancelled_owned()).await) to capture the
Result from .await and log any Err before the task exits (e.g., replace the
.ok() with a match or if let Err(e) branch and emit a
tracing::error!/log::error! with context mentioning the submit API server and
the error).
- Around line 69-70: The media-type check in submit_api.rs currently uses a
case-sensitive comparison on content_type; change the check to use a
case-insensitive comparison (e.g., call
content_type.eq_ignore_ascii_case("application/cbor")) where the current if uses
!= "application/cbor", so requests like "Application/CBOR" are accepted while
keeping the existing UNSUPPORTED_MEDIA_TYPE response when it fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ec42770-24a6-482c-a06c-8f827d6655c6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcrates/amaru/Cargo.tomlcrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/lib.rscrates/amaru/src/stages/config.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/mod.rsdocs/SUBMIT_API.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/amaru/src/stages/config.rs
- docs/SUBMIT_API.md
cfd710d to
fb45ef4
Compare
Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
fb45ef4 to
8811af1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 82-96: The current validate-then-insert sequence
(validate_transaction(tx.clone()) followed by insert(tx, TxOrigin::Local)) has a
race if validate_transaction depends on mutable mempool state; fix by either
moving validation into the atomic insert path or performing the validation while
holding the same lock used by insert so both checks happen under one critical
section (i.e., call the validation logic from inside mempool.insert or acquire
the mempool write lock, run validate_transaction, then call insert), and if you
choose not to change behavior, add a clear comment in the submit handler
referencing validate_transaction, insert, and TxRejectReason to state that
validation is pure transaction-only (no mempool-dependent checks) so the race is
acceptable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0bdce1c-49fb-4bd4-b48f-b92e34354f2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlcrates/amaru-kernel/src/cardano/block.rscrates/amaru-kernel/src/cardano/network_block.rscrates/amaru-kernel/src/cardano/raw_block.rscrates/amaru-kernel/src/lib.rscrates/amaru-minicbor-extra/src/decode.rscrates/amaru/Cargo.tomlcrates/amaru/src/bin/amaru/cmd/run.rscrates/amaru/src/lib.rscrates/amaru/src/stages/config.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/mod.rsdocs/SUBMIT_API.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/amaru/Cargo.toml
- docs/SUBMIT_API.md
- crates/amaru/src/stages/config.rs
- crates/amaru/src/tests/mod.rs
- crates/amaru/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/amaru/src/submit_api.rs (1)
61-93: Handler logic is clean and well-structured!The Content-Type stripping (split by
;, trim) handles charset parameters properly—nice attention to detail there. The match onTxRejectReasonvariants giving distinct status codes (503/409/400) is exactly what you want for API consumers to react appropriately. It's like a good RPG dialogue tree—each branch leads somewhere meaningful.One small nit: the doc comment on line 60 says "CBOR-encoded." but feels like half a sentence. Maybe "The request body is expected to be CBOR-encoded." would read better?
📝 Suggested doc fix
/// Handle incoming transaction submission requests. -/// The request body is expected to be a CBOR-encoded. +/// The request body is expected to be CBOR-encoded. async fn submit_tx(State(mempool): State<SubmitApiState>, headers: HeaderMap, body: Bytes) -> Response {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/amaru/src/submit_api.rs` around lines 61 - 93, Update the doc comment above the submit_tx handler: replace the half-sentence "CBOR-encoded." with a full sentence such as "The request body is expected to be CBOR-encoded." so the documentation for the submit_tx function is clear and complete; edit the comment immediately preceding the async fn submit_tx(...) declaration to reflect this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/amaru-protocols/src/tx_submission/responder.rs`:
- Around line 326-334: The match on mempool.insert currently returns Err for
non-Invalid TxRejectReason variants; change it so that when mempool.insert
returns Err(TxRejectReason::MempoolFull) or Err(TxRejectReason::Duplicate) you
log a warning/info (including requested_id and the reject reason) and continue
processing instead of returning Err, while preserving the existing behavior for
Err(TxRejectReason::Invalid(error)) (log invalid and continue) and for any other
unexpected error return Err as before; update the match arm(s) around
mempool.insert(...) in responder.rs to handle TxRejectReason::MempoolFull and
TxRejectReason::Duplicate explicitly and avoid terminating the protocol.
---
Nitpick comments:
In `@crates/amaru/src/submit_api.rs`:
- Around line 61-93: Update the doc comment above the submit_tx handler: replace
the half-sentence "CBOR-encoded." with a full sentence such as "The request body
is expected to be CBOR-encoded." so the documentation for the submit_tx function
is clear and complete; edit the comment immediately preceding the async fn
submit_tx(...) declaration to reflect this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e61cf5cf-e18f-47d6-8882-9b743cf70077
📒 Files selected for processing (10)
crates/amaru-mempool/src/strategies/dummy.rscrates/amaru-mempool/src/strategies/in_memory_mempool.rscrates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rscrates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rscrates/amaru-ouroboros-traits/src/lib.rscrates/amaru-ouroboros-traits/src/mempool.rscrates/amaru-protocols/src/mempool_effects.rscrates/amaru-protocols/src/tx_submission/responder.rscrates/amaru-protocols/src/tx_submission/tests/sized_mempool.rscrates/amaru/src/submit_api.rs
💤 Files with no reviewable changes (3)
- crates/amaru-ouroboros-traits/src/lib.rs
- crates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rs
- crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs
…pool at the same time Signed-off-by: Eric Torreborre <etorreborre@yahoo.com>
88e8069 to
a51a146
Compare
This PR allows to start an
amarunode with a local HTTP server offering an endpoint to post a CBOR-serialized transaction.That transaction is then directly inserted in the mempool (if it is not a duplicate of an existing transaction).
Fixes: #706
Summary by CodeRabbit