fix(NATSRS-008-3): CU-86akbhh82 2 review findings across 2 files - #140
flamingo[bot] wants to merge 2 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| version: info.version.clone(), | ||
| metadata: info.metadata.clone(), | ||
| })?; | ||
| client.publish(ping.reply.unwrap(), pong.into()).await?; |
There was a problem hiding this comment.
🦩 🟠 unwrap() on reply subject in service verb dispatch loop can panic the whole service task
In Service::add's spawned task (the tokio::select! loop handling PING/INFO/STATS verb subscriptions), replaced the three .reply.unwrap() calls with let Some(reply) = <msg>.reply else { debug!(...); continue; }; guards for the PING, INFO, and STATS branches respectively. Each branch now skips processing (via continue, logging at debug level) instead of panicking when a request lacks a reply subject, and uses the extracted reply variable for the subsequent client.publish(...) call. This prevents a malformed request from aborting the whole background service task while leaving all other behavior (response payloads, stats updates) unchanged.
🤖 Prompt for AI agents
In async-nats/src/service/mod.rs around line 390, review and complete this code-review fix: unwrap() on reply subject in service verb dispatch loop can panic the whole service task.
What the draft fix changed: In `Service::add`'s spawned task (the `tokio::select!` loop handling PING/INFO/STATS verb subscriptions), replaced the three `.reply.unwrap()` calls with `let Some(reply) = <msg>.reply else { debug!(...); continue; };` guards for the PING, INFO, and STATS branches respectively. Each branch now skips processing (via `continue`, logging at debug level) instead of panicking when a request lacks a reply subject, and uses the extracted `reply` variable for the subsequent `client.publish(...)` call. This prevents a malformed request from aborting the whole background service task while leaving all other behavior (response payloads, stats updates) unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| // TODO store pending_publish as a future and return errors from it | ||
| let client = self.context.client.clone(); | ||
| tokio::task::spawn(async move { | ||
| client | ||
| .publish(subject, Bytes::from_static(b"")) | ||
| .await | ||
| .unwrap(); | ||
| if let Err(err) = | ||
| client.publish(subject, Bytes::from_static(b"")).await | ||
| { | ||
| error!( | ||
| "failed to respond to idle heartbeat: {}", | ||
| err | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 unwrap() on publish inside spawned heartbeat responder task can panic the task silently
In Messages::poll_next (impl of futures_util::Stream for Messages), replaced the .publish(subject, Bytes::from_static(b"")).await.unwrap() call inside the spawned heartbeat-responder tokio::task::spawn with an if let Err(err) = ... { error!(...) } pattern, matching the existing non-panicking style used for the analogous flow-control publish in Ordered::poll_next (which uses .ok()). This removes the panic-on-publish-failure path while still surfacing the failure via a log line instead of silently dropping it. Added error to the tracing import list at the top of the file to support the new log call. No other behavior changed.
🤖 Prompt for AI agents
In async-nats/src/jetstream/consumer/push.rs around line 152, review and complete this code-review fix: unwrap() on publish inside spawned heartbeat responder task can panic the task silently.
What the draft fix changed: In `Messages::poll_next` (impl of `futures_util::Stream for Messages`), replaced the `.publish(subject, Bytes::from_static(b"")).await.unwrap()` call inside the spawned heartbeat-responder `tokio::task::spawn` with an `if let Err(err) = ... { error!(...) }` pattern, matching the existing non-panicking style used for the analogous flow-control publish in `Ordered::poll_next` (which uses `.ok()`). This removes the panic-on-publish-failure path while still surfacing the failure via a log line instead of silently dropping it. Added `error` to the `tracing` import list at the top of the file to support the new log call. No other behavior changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 2 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
async-nats/src/service/mod.rs:390async-nats/src/jetstream/consumer/push.rs:152What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
4fa2224e-8528-443d-a51a-823c78308977Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akbhh82 NATS async-nats and CI review findings (9 PRs)