Skip to content

fix(adhoc-lib-rs): Server::client_url and client_port panic on TcpStream connection failure #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(adhoc-lib-rs): Server::client_url and client_port panic on TcpStream connection failure #46

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 61 additions & 9 deletions nats-server/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ struct Inner {
child: Child,
logfile: PathBuf,
pidfile: PathBuf,
store_dir: PathBuf,
}

lazy_static! {
Expand All @@ -44,8 +45,8 @@ lazy_static! {

impl Drop for Server {
fn drop(&mut self) {
self.inner.child.kill().unwrap();
self.inner.child.wait().unwrap();
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.

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 ✅ 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.

if let Some(caps) = SD_RE.captures(&log) {
Expand All @@ -55,6 +56,10 @@ impl Drop for Server {
// 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();
}
}

Comment on lines 56 to 65

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 ✅ 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.

Expand All @@ -67,6 +72,9 @@ impl Server {
.expect("can't restart server with dynamic port");
self.inner.child.kill().unwrap();
self.inner.child.wait().unwrap();
// Clean up old logfile and pidfile before replacing inner.
fs::remove_file(&self.inner.logfile).ok();
fs::remove_file(&self.inner.pidfile).ok();
let inner = do_run(&self.inner.cfg, Some(&port), Some(self.inner.id.clone()));
self.inner = inner;
}

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 ✅ 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.

Expand All @@ -75,7 +83,22 @@ impl Server {
// 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 connecting up to 100 times (up to ~10s) to avoid races between
// server startup and the first call.
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");
Expand All @@ -89,7 +112,22 @@ impl Server {

pub fn client_port(&self) -> u16 {
let addr = self.client_addr();
let mut r = BufReader::with_capacity(1024, TcpStream::connect(addr).unwrap());
// Retry connecting up to 100 times (up to ~10s) to avoid races between
// server startup and the first call.
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_port")
};
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");
Expand Down Expand Up @@ -133,10 +171,22 @@ impl Server {
}

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");
}
}

Comment on lines 171 to 192

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 ✅ 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.

Expand Down Expand Up @@ -194,7 +244,7 @@ pub fn run_cluster<'a, C: IntoConfig<'a>>(cfg: C) -> Cluster {
new_port
})
.collect::<Vec<usize>>();
let cluster = [port + 1, port + 101, port + 201];
let cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1];

let s1 = run_cluster_node_with_port(
cfg.0[0],
Expand Down Expand Up @@ -275,6 +325,7 @@ fn do_run(cfg: &str, port: Option<&str>, id: Option<String>) -> Inner {
child,
logfile,
pidfile,
store_dir,
}
}

Comment on lines 325 to 331

@flamingo flamingo Bot Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 ✅ 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.

Expand Down Expand Up @@ -334,6 +385,7 @@ fn run_cluster_node_with_port(
child,
logfile,
pidfile,
store_dir,
},
}
}
Expand Down
Loading