fix(middleware): connection_drop_prevention_handler loses tracing context and amplifies panics - #5611
fix(middleware): connection_drop_prevention_handler loses tracing context and amplifies panics#5611dimazzq92 wants to merge 2 commits into
Conversation
…text and amplifies panics
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughMutating request handlers now execute in instrumented Tokio tasks. Successful task results return their HTTP responses. Task panics and cancellations are logged and converted into empty HTTP 500 responses. Other request methods retain direct execution. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/macro_middleware/src/lib.rs (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a structured
JoinErrorfield.Lines 30 and 32 render
errinside the message. Keep the distinct messages, but recorderras theerrorfield so tracing backends can index it consistently.Proposed change
- tracing::error!("Request handler panicked: {:?}", err); + tracing::error!(error = ?err, "Request handler panicked"); ... - tracing::error!("Request handler task was cancelled: {:?}", err); + tracing::error!(error = ?err, "Request handler task was cancelled");As per coding guidelines, log errors using
tracing::error!(error=?e, "error msg")rather than interpolating the error into the message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macro_middleware/src/lib.rs` around lines 28 - 33, Update the Err handling in the request-handler task join flow to keep the distinct panic and cancellation messages while passing the JoinError as a structured `error` field to `tracing::error!`. Remove the interpolated `err` formatting from both log messages and use debug-field recording consistent with the project’s logging convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/macro_middleware/src/lib.rs`:
- Around line 28-33: Update the Err handling in the request-handler task join
flow to keep the distinct panic and cancellation messages while passing the
JoinError as a structured `error` field to `tracing::error!`. Remove the
interpolated `err` formatting from both log messages and use debug-field
recording consistent with the project’s logging convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56d728fe-c524-4100-9a9e-1a70360016d0
📒 Files selected for processing (1)
crates/macro_middleware/src/lib.rs
| Ok(response) => response, | ||
| Err(err) => { | ||
| if err.is_panic() { | ||
| tracing::error!("Request handler panicked: {:?}", err); |
There was a problem hiding this comment.
prefer
| tracing::error!("Request handler panicked: {:?}", err); | |
| tracing::error!(error=?err, "request handler panicked"); |
| if err.is_panic() { | ||
| tracing::error!("Request handler panicked: {:?}", err); | ||
| } else { | ||
| tracing::error!("Request handler task was cancelled: {:?}", err); |
There was a problem hiding this comment.
prefer
| tracing::error!("Request handler task was cancelled: {:?}", err); | |
| tracing::error!(error=?err, "request handler task was cancelled"); |
|
Applied the suggested structured tracing format (error=?err). Thanks for the review! |
2 participants
Summary
While exploring the architecture in
crates/macro_middleware/src/lib.rs, I noticed an observability bug in theconnection_drop_prevention_handler. This middleware wraps mutating requests intokio::task::spawnto prevent database operations from being aborted if the client drops the connection.By moving the request execution into a new tokio task without explicit instrumentation, this inadvertently introduces two problems:
tokio::task::spawndoes not inherit the current thread-localtracing::Span. Any logs (tracing::info!,error!, etc.) emitted downstream inside the request handler are completely orphaned. They lose therequest_id, user context, and path set by outer middleware layers, making production debugging very difficult for POST/PUT requests..unwrap()on theJoinHandlemeans that if the inner handler panics, theJoinError::Panicis unwrapped, causing the middleware itself to panic. This bypasses normal Axum panic-handling layers and abruptly kills the connection, rather than returning a clean HTTP 500.Proposed Fix
.instrument(tracing::Span::current())from thetracingcrate to explicitly pass the span into the spawned task.JoinErrorgracefully, mapping it to a 500 response.This PR implements these fixes, ensuring that network drops won't cancel the query, while keeping production observability intact and safely converting task panics to standard 500s.