fix(NATSRS-002-2): CU-86akbhh82 2 review findings in message.rs - #134
flamingo[bot] wants to merge 1 commit into
Conversation
| match str::parse(try_parse!(str)) { | ||
| Ok(parsed) => parsed, | ||
| Err(e) => { | ||
| log::error!( | ||
| let msg = format!( | ||
| "failed to parse jetstream reply \ | ||
| subject: {}, error: {:?}. Is your \ | ||
| nats-server up to date?", | ||
| reply, | ||
| e | ||
| ); | ||
| return None; | ||
| log::error!("{}", msg); | ||
| return Err(io::Error::new(io::ErrorKind::InvalidData, msg)); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🦩 🔴 jetstream_message_info swallows parse errors instead of surfacing them via a typed error
In Message::jetstream_message_info (nats/src/message.rs), changed the return type from Option<crate::jetstream::JetStreamMessageInfo<'_>> to Result<Option<crate::jetstream::JetStreamMessageInfo<'_>>, io::Error>. All the try_parse! macro failure branches (both the token-missing branch and the str::parse error branch) and the OffsetDateTime::from_unix_timestamp_nanos failure now build a descriptive message, log it via log::error!, and return Err(io::Error::new(io::ErrorKind::InvalidData, msg)) instead of silently returning None. All success paths (n_tokens >= 9, n_tokens == 7, and the "not a JetStream message" cases) now return Ok(...). This is a public API breaking change (callers must now handle a Result instead of an Option), so any call sites elsewhere in the crate/consumers that call .jetstream_message_info() will need to be updated — I could not see those call sites in this file, so this is unverified for compilation across the crate. Using io::Error rather than a crate-specific Error<Kind> type was necessary because no such typed error alias is visible in this file's imports; introducing one would require inventing an identifier not shown in the provided material, which the hard rules forbid.
🤖 Prompt for AI agents
In nats/src/message.rs around line 293, review and complete this code-review fix: jetstream_message_info swallows parse errors instead of surfacing them via a typed error.
What the draft fix changed: In `Message::jetstream_message_info` (nats/src/message.rs), changed the return type from `Option<crate::jetstream::JetStreamMessageInfo<'_>>` to `Result<Option<crate::jetstream::JetStreamMessageInfo<'_>>, io::Error>`. All the `try_parse!` macro failure branches (both the token-missing branch and the `str::parse` error branch) and the `OffsetDateTime::from_unix_timestamp_nanos` failure now build a descriptive message, log it via `log::error!`, and return `Err(io::Error::new(io::ErrorKind::InvalidData, msg))` instead of silently returning `None`. All success paths (`n_tokens >= 9`, `n_tokens == 7`, and the "not a JetStream message" cases) now return `Ok(...)`. This is a public API breaking change (callers must now handle a `Result` instead of an `Option`), so any call sites elsewhere in the crate/consumers that call `.jetstream_message_info()` will need to be updated — I could not see those call sites in this file, so this is unverified for compilation across the crate. Using `io::Error` rather than a crate-specific `Error<Kind>` type was necessary because no such typed error alias is visible in this file's imports; introducing one would require inventing an identifier not shown in the provided material, which the hard rules forbid.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| if retries == 2 { | ||
| log::warn!("double_ack is retrying until the server connection is reestablished"); | ||
| } | ||
| if retries > DOUBLE_ACK_MAX_RETRIES { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::TimedOut, | ||
| format!( | ||
| "double_ack did not receive server confirmation after {} retries", | ||
| DOUBLE_ACK_MAX_RETRIES | ||
| ), | ||
| )); | ||
| } | ||
| let ack_reply = format!("_INBOX.{}", nuid::next()); | ||
| let sub_ret = client.subscribe(&ack_reply, None); | ||
| if sub_ret.is_err() { |
There was a problem hiding this comment.
🦩 🟠 double_ack retries forever without a bound, risking an infinite loop
In Message::double_ack (nats/src/message.rs), added a DOUBLE_ACK_MAX_RETRIES constant (10) and a bound check at the top of the retry loop: once retries exceeds DOUBLE_ACK_MAX_RETRIES, the function now returns Err(io::Error::new(io::ErrorKind::TimedOut, ...)) instead of looping forever. This preserves the existing retry/backoff behavior (100ms sleep, warn log at retry 2) but bounds it so the calling thread can no longer hang indefinitely. The chosen retry count (10, ~1s worst-case of sleeps plus network round trips) is a reasonable but arbitrary default since no existing constant or config value for this was visible in the file; a more complete fix might expose this as a configurable timeout parameter on the public API, which would be a larger signature change than this minimal fix attempts.
🤖 Prompt for AI agents
In nats/src/message.rs around line 229, review and complete this code-review fix: double_ack retries forever without a bound, risking an infinite loop.
What the draft fix changed: In `Message::double_ack` (nats/src/message.rs), added a `DOUBLE_ACK_MAX_RETRIES` constant (10) and a bound check at the top of the retry `loop`: once `retries` exceeds `DOUBLE_ACK_MAX_RETRIES`, the function now returns `Err(io::Error::new(io::ErrorKind::TimedOut, ...))` instead of looping forever. This preserves the existing retry/backoff behavior (100ms sleep, warn log at retry 2) but bounds it so the calling thread can no longer hang indefinitely. The chosen retry count (10, ~1s worst-case of sleeps plus network round trips) is a reasonable but arbitrary default since no existing constant or config value for this was visible in the file; a more complete fix might expose this as a configurable timeout parameter on the public API, which would be a larger signature change than this minimal fix attempts.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
Closes 2 review findings in
nats/src/message.rs.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
nats/src/message.rs:293nats/src/message.rs:229What 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)