fix(adhoc-lib-rs): nats-server run_cluster uses unwrap() on TcpStream::connect inside client_url, crashing tests on connection failure - #51
Conversation
…::connect inside client_url, crashing tests on connection failure
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| // Helpful when dynamically allocating ports with -1. | ||
| pub fn client_url(&self) -> String { | ||
| let addr = self.client_addr(); | ||
| let mut r = BufReader::with_capacity(1024, TcpStream::connect(addr).unwrap()); | ||
| // Retry up to 100 times (100 * 100ms = 10s) waiting for the server to accept connections. | ||
| let stream = { | ||
| let mut s = None; | ||
| for _ in 0..100 { | ||
| match TcpStream::connect(&addr) { | ||
| Ok(stream) => { s = Some(stream); break; } | ||
| Err(_) => thread::sleep(Duration::from_millis(100)), | ||
| } | ||
| } | ||
| s.expect("could not connect to server for client_url") | ||
| }; | ||
| let mut r = BufReader::with_capacity(1024, stream); | ||
| let mut line = String::new(); | ||
| r.read_line(&mut line).expect("did not receive INFO"); | ||
| let si: Value = serde_json::from_str(&line["INFO".len()..]).expect("could not parse INFO"); | ||
| let info_prefix = "INFO "; | ||
| assert!(line.starts_with(info_prefix), "expected INFO, got: {line}"); | ||
| let si: Value = serde_json::from_str(&line[info_prefix.len()..]).expect("could not parse INFO"); | ||
| let port = si["port"].as_u64().expect("could not parse port") as u16; | ||
| let mut scheme = "nats://"; | ||
| if si["tls_required"].as_bool().unwrap_or(false) { |
There was a problem hiding this comment.
🦩 🟠 nats-server run_cluster uses unwrap() on TcpStream::connect inside client_url, crashing tests on connection failure
In client_url() and client_port(), replaced TcpStream::connect(addr).unwrap() with a retry loop (up to 100 iterations, 100ms sleep each) that attempts TcpStream::connect(&addr) and only panics with a descriptive message after exhausting all retries. This mirrors the pattern already used in client_addr().
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 74, review and complete this code-review fix: nats-server run_cluster uses unwrap() on TcpStream::connect inside client_url, crashing tests on connection failure.
What the draft fix changed: In `client_url()` and `client_port()`, replaced `TcpStream::connect(addr).unwrap()` with a retry loop (up to 100 iterations, 100ms sleep each) that attempts `TcpStream::connect(&addr)` and only panics with a descriptive message after exhausting all retries. This mirrors the pattern already used in `client_addr()`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| // Helpful when dynamically allocating ports with -1. | ||
| pub fn client_url(&self) -> String { | ||
| let addr = self.client_addr(); | ||
| let mut r = BufReader::with_capacity(1024, TcpStream::connect(addr).unwrap()); |
There was a problem hiding this comment.
🦩 🟠 nats-server/src/lib.rs parses INFO line with a fixed offset ["INFO".len()..] that will panic on non-INFO responses
In both client_url() and client_port(), replaced the bare &line["INFO".len()..] slice with an assert!(line.starts_with(info_prefix), ...) guard followed by &line[info_prefix.len()..] where info_prefix = "INFO " (with trailing space, matching the NATS protocol). This gives a clear panic message on unexpected protocol responses and avoids out-of-bounds slicing.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 78, review and complete this code-review fix: nats-server/src/lib.rs parses INFO line with a fixed offset ["INFO".len()..] that will panic on non-INFO responses.
What the draft fix changed: In both `client_url()` and `client_port()`, replaced the bare `&line["INFO".len()..]` slice with an `assert!(line.starts_with(info_prefix), ...)` guard followed by `&line[info_prefix.len()..]` where `info_prefix = "INFO "` (with trailing space, matching the NATS protocol). This gives a clear panic message on unexpected protocol responses and avoids out-of-bounds slicing.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| pub fn client_pid(&self) -> usize { | ||
| String::from_utf8(fs::read(self.inner.pidfile.clone()).unwrap()) | ||
| .unwrap() | ||
| .parse() | ||
| .unwrap() | ||
| // Retry up to 100 times (100 * 100ms = 10s) waiting for the PID file to appear and be valid. | ||
| for _ in 0..100 { | ||
| match fs::read(self.inner.pidfile.clone()) { | ||
| Ok(bytes) => { | ||
| match String::from_utf8(bytes) { | ||
| Ok(s) => { | ||
| let trimmed = s.trim(); | ||
| if let Ok(pid) = trimmed.parse::<usize>() { | ||
| return pid; | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| panic!("could not read valid PID from pidfile: {:?}", self.inner.pidfile); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 nats-server run_cluster port availability check has a TOCTOU race — ports can be claimed between check and bind
The TOCTOU race between is_port_available and the actual server bind is a fundamental OS-level race that cannot be fully eliminated without changing how ports are allocated (e.g., using port 0 and reading back the assigned port). The suggested fix of using port 0 would require significant restructuring of run_cluster_node_with_port and the cluster route configuration (routes must be known before servers start). As a minimal change, the cluster port array is now computed from the final ports values (finding 5 fix below), which at least ensures client and cluster ports are consistent. The TOCTOU window itself remains; a complete fix would require architectural changes spanning multiple functions.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 163, review and complete this code-review fix: nats-server run_cluster port availability check has a TOCTOU race — ports can be claimed between check and bind.
What the draft fix changed: The TOCTOU race between `is_port_available` and the actual server bind is a fundamental OS-level race that cannot be fully eliminated without changing how ports are allocated (e.g., using port 0 and reading back the assigned port). The suggested fix of using port 0 would require significant restructuring of `run_cluster_node_with_port` and the cluster route configuration (routes must be known before servers start). As a minimal change, the cluster port array is now computed from the final `ports` values (finding 5 fix below), which at least ensures client and cluster ports are consistent. The TOCTOU window itself remains; a complete fix would require architectural changes spanning multiple functions.
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
| } | ||
|
|
||
| pub fn client_pid(&self) -> usize { | ||
| String::from_utf8(fs::read(self.inner.pidfile.clone()).unwrap()) | ||
| .unwrap() | ||
| .parse() | ||
| .unwrap() | ||
| // Retry up to 100 times (100 * 100ms = 10s) waiting for the PID file to appear and be valid. | ||
| for _ in 0..100 { | ||
| match fs::read(self.inner.pidfile.clone()) { | ||
| Ok(bytes) => { | ||
| match String::from_utf8(bytes) { | ||
| Ok(s) => { | ||
| let trimmed = s.trim(); | ||
| if let Ok(pid) = trimmed.parse::<usize>() { | ||
| return pid; | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| panic!("could not read valid PID from pidfile: {:?}", self.inner.pidfile); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 nats-server/src/lib.rs client_pid() uses unwrap() chains that will panic if the PID file is missing or non-UTF8
In client_pid(), replaced the three-unwrap chain with a retry loop (up to 100 iterations, 100ms sleep each) that reads the PID file, parses it as UTF-8, trims whitespace, and parses as usize. Only panics after exhausting retries with a descriptive message including the pidfile path. This mirrors the pattern in client_addr().
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 126, review and complete this code-review fix: nats-server/src/lib.rs client_pid() uses unwrap() chains that will panic if the PID file is missing or non-UTF8.
What the draft fix changed: In `client_pid()`, replaced the three-unwrap chain with a retry loop (up to 100 iterations, 100ms sleep each) that reads the PID file, parses it as UTF-8, trims whitespace, and parses as `usize`. Only panics after exhausting retries with a descriptive message including the pidfile path. This mirrors the pattern in `client_addr()`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| pub fn client_pid(&self) -> usize { | ||
| String::from_utf8(fs::read(self.inner.pidfile.clone()).unwrap()) | ||
| .unwrap() | ||
| .parse() | ||
| .unwrap() | ||
| // Retry up to 100 times (100 * 100ms = 10s) waiting for the PID file to appear and be valid. | ||
| for _ in 0..100 { | ||
| match fs::read(self.inner.pidfile.clone()) { | ||
| Ok(bytes) => { | ||
| match String::from_utf8(bytes) { | ||
| Ok(s) => { | ||
| let trimmed = s.trim(); | ||
| if let Ok(pid) = trimmed.parse::<usize>() { | ||
| return pid; | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| panic!("could not read valid PID from pidfile: {:?}", self.inner.pidfile); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 nats-server run_cluster cluster port array uses base port+1/+101/+201 before availability is checked, creating potential overlap with client ports
In run_cluster, changed let cluster = [port + 1, port + 101, port + 201]; to let cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1]; (after the availability-check loop). This ensures cluster ports are always client_port + 1 for each node, even when the availability loop changes a port from its initial value.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 155, review and complete this code-review fix: nats-server run_cluster cluster port array uses base port+1/+101/+201 before availability is checked, creating potential overlap with client ports.
What the draft fix changed: In `run_cluster`, changed `let cluster = [port + 1, port + 101, port + 201];` to `let cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1];` (after the availability-check loop). This ensures cluster ports are always `client_port + 1` for each node, even when the availability loop changes a port from its initial value.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| impl Drop for Server { | ||
| fn drop(&mut self) { | ||
| self.inner.child.kill().unwrap(); | ||
| self.inner.child.wait().unwrap(); |
There was a problem hiding this comment.
🦩 🟠 nats-server/src/lib.rs Drop impl ignores pidfile cleanup, leaving stale PID files after test runs
In the Drop impl, added fs::remove_file(self.inner.pidfile.as_os_str()).ok(); after the logfile removal. Uses .ok() to silently ignore errors (e.g., file already gone), consistent with the logfile removal style.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 48, review and complete this code-review fix: nats-server/src/lib.rs Drop impl ignores pidfile cleanup, leaving stale PID files after test runs.
What the draft fix changed: In the `Drop` impl, added `fs::remove_file(self.inner.pidfile.as_os_str()).ok();` after the logfile removal. Uses `.ok()` to silently ignore errors (e.g., file already gone), consistent with the logfile removal style.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| self.inner.child.wait().unwrap(); | ||
| let _ = self.inner.child.kill(); | ||
| let _ = self.inner.child.wait(); | ||
| if let Ok(log) = fs::read_to_string(self.inner.logfile.as_os_str()) { |
There was a problem hiding this comment.
🦩 🟠 nats-server/src/lib.rs Drop impl calls unwrap() on child.kill() and child.wait(), panicking during unwind if the process already exited
In the Drop impl, changed self.inner.child.kill().unwrap() and self.inner.child.wait().unwrap() to let _ = self.inner.child.kill(); and let _ = self.inner.child.wait();. This prevents a double-panic/abort if the child has already exited when Drop runs. Also applied the same change to restart() for consistency.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 49, review and complete this code-review fix: nats-server/src/lib.rs Drop impl calls unwrap() on child.kill() and child.wait(), panicking during unwind if the process already exited.
What the draft fix changed: In the `Drop` impl, changed `self.inner.child.kill().unwrap()` and `self.inner.child.wait().unwrap()` to `let _ = self.inner.child.kill();` and `let _ = self.inner.child.wait();`. This prevents a double-panic/abort if the child has already exited when `Drop` runs. Also applied the same change to `restart()` for consistency.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer
Closes findings from rule adhoc-lib-rs — nats-server run_cluster uses unwrap() on TcpStream::connect inside client_url, crashing tests on connection failure.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
nats-server/src/lib.rs:74nats-server/src/lib.rs:78nats-server/src/lib.rs:163nats-server/src/lib.rs:126nats-server/src/lib.rs:155nats-server/src/lib.rs:48nats-server/src/lib.rs:49What 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:
c70be0ee-08a4-4bde-9f5c-d994d0e1d4f2Merging 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.