fix(adhoc-lib-rs): 8 review findings in lib.rs - #61
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| .iter() | ||
| .map(|port| { | ||
| let mut new_port = *port; | ||
| // Check both the client port and the cluster port (client+1) for availability. | ||
| while !is_port_available(new_port) || !is_port_available(new_port + 1) { | ||
| new_port = rand::thread_rng().gen_range(2000..50_000); | ||
| } | ||
| new_port | ||
| }) | ||
| .collect::<Vec<usize>>(); | ||
| let cluster = [port + 1, port + 101, port + 201]; | ||
| // Cluster ports are client_port+1 for each node; availability was verified above. | ||
| let cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1]; | ||
|
|
||
| let s1 = run_cluster_node_with_port( | ||
| cfg.0[0], |
There was a problem hiding this comment.
🦩 🟠 run_cluster port collision: cluster ports are computed from the same base as client ports without re-checking availability
In run_cluster, changed let cluster = [port + 1, port + 101, port + 201] to let cluster = [ports[0] + 1, ports[1] + 1, ports[2] + 1]. Now cluster ports are derived from the already-availability-checked ports array rather than the original unchecked port base. The existing while loop already checks new_port + 1 (the cluster port), so this change ensures the cluster ports actually correspond to the verified client ports. Risk: the check !is_port_available(new_port + 1) was already there but the cluster array was ignoring it; this fix aligns the two.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 163, review and complete this code-review fix: run_cluster port collision: cluster ports are computed from the same base as client ports without re-checking availability.
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]`. Now cluster ports are derived from the already-availability-checked `ports` array rather than the original unchecked `port` base. The existing `while` loop already checks `new_port + 1` (the cluster port), so this change ensures the cluster ports actually correspond to the verified client ports. Risk: the check `!is_port_available(new_port + 1)` was already there but the cluster array was ignoring it; this fix aligns the two.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 82 medium — react 👍/👎 to teach the reviewer
| let inner = do_run(&self.inner.cfg, Some(&port), Some(self.inner.id.clone())); | ||
| self.inner = inner; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🦩 🟠 client_url and client_port open a raw TCP connection to parse the INFO frame but never close it explicitly
The finding asks to parse the port from the log file instead of opening a live TCP connection in client_url() and client_port(). However, CLIENT_RE captures the listen address (e.g. localhost:4222) which gives the port directly. The full fix would replace the TCP connection with log parsing in both methods. This is a larger refactor touching the public API and the INFO-frame TLS detection logic (which cannot be replicated from the log). Left unchanged to avoid breaking TLS detection. The finding is partially mitigated by finding #7 (store_dir cleanup) and finding #3 (comment fix) but the TCP connection itself is not removed. A reviewer should decide whether to accept the partial state or implement a full log-based approach. LOW confidence because the change was not made — the risk of breaking TLS scheme detection is too high without seeing all callers.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 72, review and complete this code-review fix: client_url and client_port open a raw TCP connection to parse the INFO frame but never close it explicitly.
What the draft fix changed: The finding asks to parse the port from the log file instead of opening a live TCP connection in `client_url()` and `client_port()`. However, `CLIENT_RE` captures the listen address (e.g. `localhost:4222`) which gives the port directly. The full fix would replace the TCP connection with log parsing in both methods. This is a larger refactor touching the public API and the INFO-frame TLS detection logic (which cannot be replicated from the log). Left unchanged to avoid breaking TLS detection. The finding is partially mitigated by finding #7 (store_dir cleanup) and finding #3 (comment fix) but the TCP connection itself is not removed. A reviewer should decide whether to accept the partial state or implement a full log-based approach. LOW confidence because the change was not made — the risk of breaking TLS scheme detection is too high without seeing all callers.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| // Grab client addr from logs. | ||
| fn client_addr(&self) -> String { | ||
| // We may need to wait for log to be present. | ||
| // Wait up to 10s. (100 * 100ms) | ||
| // Wait up to 50s. (100 * 500ms) | ||
| for _ in 0..100 { | ||
| match fs::read_to_string(self.inner.logfile.as_os_str()) { | ||
| Ok(l) => { |
There was a problem hiding this comment.
🦩 🟠 client_addr busy-waits with 500ms sleep but the outer loop comment says 100ms, causing 50s max wait instead of 10s
In client_addr, updated the comment from "Wait up to 10s. (100 * 100ms)" to "Wait up to 50s. (100 * 500ms)" to match the actual Duration::from_millis(500) sleep. The sleep value itself is left at 500ms (matching existing behavior). This is a pure documentation fix.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 112, review and complete this code-review fix: client_addr busy-waits with 500ms sleep but the outer loop comment says 100ms, causing 50s max wait instead of 10s.
What the draft fix changed: In `client_addr`, updated the comment from `"Wait up to 10s. (100 * 100ms)"` to `"Wait up to 50s. (100 * 500ms)"` to match the actual `Duration::from_millis(500)` sleep. The sleep value itself is left at 500ms (matching existing behavior). This is a pure documentation fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| impl Server { | ||
| pub fn restart(&mut self) { | ||
| pub fn restart(&mut self) -> Result<(), &'static str> { | ||
| let port = self |
There was a problem hiding this comment.
🦩 🔴 Server::restart uses the stored port string but the original server may have been started with a dynamic port (-1), causing panic
Changed Server::restart to return Result<(), &'static str> instead of panicking. The .expect(...) is replaced with .ok_or(...)?. This is a breaking API change for any caller that does not handle the Result. Risk: callers in other files (not visible here) that call .restart() without handling a Result will fail to compile. A reviewer must check all call sites. The change is the minimal correct approach for the finding; a type-state pattern would require more structural changes.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 63, review and complete this code-review fix: Server::restart uses the stored port string but the original server may have been started with a dynamic port (-1), causing panic.
What the draft fix changed: Changed `Server::restart` to return `Result<(), &'static str>` instead of panicking. The `.expect(...)` is replaced with `.ok_or(...)?`. This is a breaking API change for any caller that does not handle the `Result`. Risk: callers in other files (not visible here) that call `.restart()` without handling a `Result` will fail to compile. A reviewer must check all call sites. The change is the minimal correct approach for the finding; a type-state pattern would require more structural changes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 78 medium — 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.
🦩 🟠 Server::drop silently ignores kill/wait errors, potentially leaving zombie nats-server processes
In Drop for Server, changed self.inner.child.kill().unwrap() to .ok() and self.inner.child.wait().unwrap() to .ok(). This prevents a double-panic (abort) when the child has already exited before drop is called.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 48, review and complete this code-review fix: Server::drop silently ignores kill/wait errors, potentially leaving zombie nats-server processes.
What the draft fix changed: In `Drop for Server`, changed `self.inner.child.kill().unwrap()` to `.ok()` and `self.inner.child.wait().unwrap()` to `.ok()`. This prevents a double-panic (abort) when the child has already exited before drop is called.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| impl Server { | ||
| pub fn restart(&mut self) { | ||
| pub fn restart(&mut self) -> Result<(), &'static str> { | ||
| let port = self |
There was a problem hiding this comment.
🦩 🟠 Server::restart also panics on kill/wait failure during the restart sequence
In Server::restart, changed self.inner.child.kill().unwrap() to .ok() and self.inner.child.wait().unwrap() to .ok(). Same fix as finding #5 but in the restart method, preventing panic when the child has already exited.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 63, review and complete this code-review fix: Server::restart also panics on kill/wait failure during the restart sequence.
What the draft fix changed: In `Server::restart`, changed `self.inner.child.kill().unwrap()` to `.ok()` and `self.inner.child.wait().unwrap()` to `.ok()`. Same fix as finding #5 but in the restart method, preventing panic when the child has already exited.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| impl Cluster { | ||
| pub fn client_url(&self) -> String { | ||
| self.servers[0].client_url() | ||
| self.servers.iter().map(|s| s.client_url()).collect::<Vec<_>>().join(",") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 run_cluster_node_with_port does not clean up its log file or store_dir on drop because it creates a Server without a pidfile cleanup path
Added store_dir: Option<PathBuf> field to Inner. In run_cluster_node_with_port, set store_dir: Some(store_dir) so the explicit store directory path is stored in Inner. In Drop, added a branch: if self.inner.store_dir is Some, remove it directly without relying on log parsing. In do_run (used by non-cluster servers), set store_dir: None to preserve existing log-based cleanup behavior. This ensures cluster node store directories are always cleaned up. Risk: do_run still uses log-based cleanup for non-cluster servers; if the log-based path also needs the explicit store_dir, that is a separate concern not addressed here.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 244, review and complete this code-review fix: run_cluster_node_with_port does not clean up its log file or store_dir on drop because it creates a Server without a pidfile cleanup path.
What the draft fix changed: Added `store_dir: Option<PathBuf>` field to `Inner`. In `run_cluster_node_with_port`, set `store_dir: Some(store_dir)` so the explicit store directory path is stored in `Inner`. In `Drop`, added a branch: if `self.inner.store_dir` is `Some`, remove it directly without relying on log parsing. In `do_run` (used by non-cluster servers), set `store_dir: None` to preserve existing log-based cleanup behavior. This ensures cluster node store directories are always cleaned up. Risk: `do_run` still uses log-based cleanup for non-cluster servers; if the log-based path also needs the explicit store_dir, that is a separate concern not addressed here.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| impl Cluster { | ||
| pub fn client_url(&self) -> String { | ||
| self.servers[0].client_url() | ||
| self.servers.iter().map(|s| s.client_url()).collect::<Vec<_>>().join(",") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 Cluster::client_url always returns servers[0].client_url(), providing no load distribution or failover for tests
In Cluster::client_url, changed the implementation to return all server URLs joined by commas: self.servers.iter().map(|s| s.client_url()).collect::<Vec<_>>().join(","). This allows NATS clients to use the full cluster URL list for failover. Risk: callers that expect a single URL (e.g. parsing with Url::parse) may fail on the comma-separated string. The test code in this file uses cluster.servers[0].client_url() directly, so the tests are unaffected. External callers should be reviewed.
🤖 Prompt for AI agents
In nats-server/src/lib.rs around line 230, review and complete this code-review fix: Cluster::client_url always returns servers[0].client_url(), providing no load distribution or failover for tests.
What the draft fix changed: In `Cluster::client_url`, changed the implementation to return all server URLs joined by commas: `self.servers.iter().map(|s| s.client_url()).collect::<Vec<_>>().join(",")`. This allows NATS clients to use the full cluster URL list for failover. Risk: callers that expect a single URL (e.g. parsing with `Url::parse`) may fail on the comma-separated string. The test code in this file uses `cluster.servers[0].client_url()` directly, so the tests are unaffected. External callers should be reviewed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
Closes 8 review findings in
nats-server/src/lib.rs.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Note
1 of these finding(s) already have a fix PR (#46); this PR covers the remainder, and their tracking stays on the original.
nats-server/src/lib.rs:163nats-server/src/lib.rs:72nats-server/src/lib.rs:112nats-server/src/lib.rs:63nats-server/src/lib.rs:48nats-server/src/lib.rs:63nats-server/src/lib.rs:244nats-server/src/lib.rs:230What 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:
b6012782-a2c5-42ca-ac3b-ea9aa1fa6a7fMerging 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.