fix(adhoc-lib-rs): Server::client_url and client_port panic on TcpStream connection failure - #46
fix(adhoc-lib-rs): Server::client_url and client_port panic on TcpStream connection failure#46flamingo[bot] wants to merge 1 commit into
Conversation
…eam connection failure
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦩 What this fix changed, finding by finding
6 finding(s) — 5 fixed (5 explained inline on the diff), 1 not fixed.
⚠️ Not fixed — needs human judgment
🟠 2. run_cluster uses rand::thread_rng().gen_range without seeding, and port collision check has a TOCTOU race — nats-server/src/lib.rs:168
NOT FIXED: The TOCTOU race in run_cluster between is_port_available and actual server binding cannot be safely fixed by a purely mechanical change without restructuring how cluster ports are discovered (i.e., passing -1 and reading back from the INFO line, which requires significant refactoring of run_cluster_node_with_port and the Cluster API). The existing is_port_available check and retry loop are preserved unchanged. A minor related bug was fixed: the original code used cluster = [port + 1, port + 101, port + 201] (based on the original port, not the remapped ports[i]), which was inconsistent; changed to cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1] so cluster ports are always adjacent to the chosen client ports.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 168, address this code-review finding: run_cluster uses rand::thread_rng().gen_range without seeding, and port collision check has a TOCTOU race.
The TOCTOU race in `run_cluster` between `is_port_available` and actual server binding cannot be safely fixed by a purely mechanical change without restructuring how cluster ports are discovered (i.e., passing `-1` and reading back from the INFO line, which requires significant refactoring of `run_cluster_node_with_port` and the `Cluster` API). The existing `is_port_available` check and retry loop are preserved unchanged. A minor related bug was fixed: the original code used `cluster = [port + 1, port + 101, port + 201]` (based on the original `port`, not the remapped `ports[i]`), which was inconsistent; changed to `cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1]` so cluster ports are always adjacent to the chosen client ports.
Make the minimal change that resolves the finding; do not refactor unrelated code.
| fs::remove_file(&self.inner.pidfile).ok(); | ||
| let inner = do_run(&self.inner.cfg, Some(&port), Some(self.inner.id.clone())); | ||
| self.inner = inner; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 ✅ Fixed — Server::client_url and client_port panic on TcpStream connection failure
FIXED: client_url() and client_port() (lines ~72 and ~83): Replaced the bare TcpStream::connect(addr).unwrap() in both methods with a retry loop of up to 100 iterations with 100ms sleep between attempts, matching the pattern used in client_addr(). After the loop, .expect(...) is used instead of .unwrap() to give a clear panic message if all retries are exhausted.
| } | ||
|
|
||
| pub fn client_pid(&self) -> usize { | ||
| String::from_utf8(fs::read(self.inner.pidfile.clone()).unwrap()) | ||
| .unwrap() | ||
| .parse() | ||
| .unwrap() | ||
| // Retry up to 100 times (up to ~10s) waiting for the PID file to be written | ||
| // by the server process after startup. | ||
| for _ in 0..100 { | ||
| match fs::read(self.inner.pidfile.clone()) { | ||
| Ok(bytes) => { | ||
| if let Ok(s) = String::from_utf8(bytes) { | ||
| if let Ok(pid) = s.trim().parse::<usize>() { | ||
| return pid; | ||
| } | ||
| } | ||
| } | ||
| Err(_) => {} | ||
| } | ||
| thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| panic!("could not read PID file after waiting"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 ✅ Fixed — Server::client_pid panics with an unhelpful message if the PID file is not yet written
FIXED: client_pid() (line ~128): Replaced the bare fs::read(...).unwrap() chain with a retry loop of up to 100 iterations with 100ms sleep between attempts. Each iteration attempts to read and parse the PID file; on success it returns immediately. After all retries are exhausted it panics with a descriptive message. Also added .trim() before .parse() to handle trailing newlines in PID files.
| // Remove Logfile. | ||
| fs::remove_file(self.inner.logfile.as_os_str()).ok(); | ||
| } | ||
| // Always attempt to remove the store_dir (covers non-JetStream cluster nodes too). | ||
| fs::remove_dir_all(&self.inner.store_dir).ok(); | ||
| // Remove pidfile. | ||
| fs::remove_file(&self.inner.pidfile).ok(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 ✅ Fixed — Server::restart does not clean up the old server's log and PID files before replacing inner
FIXED: Server::restart() (line ~62): Added fs::remove_file(&self.inner.logfile).ok() and fs::remove_file(&self.inner.pidfile).ok() immediately before self.inner = inner to clean up the old server's files before the old Inner is overwritten.
| self.inner.child.kill().ok(); | ||
| self.inner.child.wait().ok(); | ||
| if let Ok(log) = fs::read_to_string(self.inner.logfile.as_os_str()) { | ||
| // Check if we had JetStream running and if so cleanup the storage directory. |
There was a problem hiding this comment.
🦩 🟠 ✅ Fixed — Server Drop impl calls unwrap() on kill() and wait(), masking double-panic in test teardown
FIXED: Drop for Server (line ~50): Changed self.inner.child.kill().unwrap() to self.inner.child.kill().ok() and self.inner.child.wait().unwrap() to self.inner.child.wait().ok() so that a already-exited child process does not cause a panic during drop/test teardown.
| child, | ||
| logfile, | ||
| pidfile, | ||
| store_dir, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 ✅ Fixed — run_cluster_node_with_port does not clean up the store_dir on drop, unlike single-server do_run
FIXED: run_cluster_node_with_port store_dir leak (line ~248): Added a store_dir: PathBuf field to the Inner struct. Both do_run and run_cluster_node_with_port now store the store_dir path in Inner. The Drop impl calls fs::remove_dir_all(&self.inner.store_dir).ok() unconditionally, ensuring cleanup regardless of whether JetStream was enabled and regardless of whether the log contains the SD_RE pattern. The Drop impl also now explicitly removes the pidfile.
Closes findings from rule adhoc-lib-rs — Server::client_url and client_port panic on TcpStream 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:72nats-server/src/lib.rs:168nats-server/src/lib.rs:128nats-server/src/lib.rs:62nats-server/src/lib.rs:50nats-server/src/lib.rs:248What 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:
1bff4c8b-6382-4a91-9010-0698bc64d01bMerging 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.