fix(install): check ports 8080 and 18789 are free before starting gat…#329
fix(install): check ports 8080 and 18789 are free before starting gat…#329webdevpraveen wants to merge 1 commit into
Conversation
…eway Fixes NVIDIA#51 install.sh starts the OpenShell gateway on port 8080 and the OpenClaw dashboard on port 18789 without checking whether those ports are already in use. Users with existing Docker containers, web servers, or other services on those ports get a cryptic startup failure: Error: listen tcp 0.0.0.0:8080: bind: address already in use This change adds a check_required_ports() preflight function that runs before any gateway or sandbox setup: - Detects conflicts using ss (iproute2) with netstat fallback - Prints a clear, named error identifying the conflicting port - Suggests NEMOCLAW_GATEWAY_PORT / NEMOCLAW_DASHBOARD_PORT env vars so users can override without stopping existing services - Degrades gracefully when neither ss nor netstat is available Signed-off-by: webdevpraveen <pr4veensingh@proton.me>
📝 WalkthroughWalkthroughThe installation script now includes a preflight port-availability check that validates both the gateway and dashboard ports are free before proceeding with Node.js installation. Port detection uses ss or netstat, with defaults overridable via environment variables, and the process exits with errors if ports are already in use. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
install.sh (1)
98-98: Useinfohelper for consistency.Line 98 uses a raw
echo "[INFO]..."format while the rest of the script uses theinfo()helper (line 12) which includes color formatting.♻️ Proposed fix
- echo "[INFO] Ports ${NEMOCLAW_GATEWAY_PORT} (gateway) and ${NEMOCLAW_DASHBOARD_PORT} (dashboard) are free." + info "Ports ${NEMOCLAW_GATEWAY_PORT} (gateway) and ${NEMOCLAW_DASHBOARD_PORT} (dashboard) are free."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` at line 98, Replace the raw echo statement that logs ports being free with the script's info() helper to maintain consistent formatting and color; locate the line printing "[INFO] Ports ${NEMOCLAW_GATEWAY_PORT} (gateway) and ${NEMOCLAW_DASHBOARD_PORT} (dashboard) are free." and call info with the same message string so it uses the existing info() function's output style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 141-143: The call to check_required_ports is incorrectly nested
inside install_nodejs’s sha256sum branch; move the check out of install_nodejs
and invoke check_required_ports at the start of main() before any installation
logic so port checks run regardless of Node.js presence or which checksum tool
(sha256sum/shasum) is available; remove the misplaced check_required_ports
invocation inside the command_exists sha256sum conditional (and ensure no
duplicate calls remain).
- Around line 70-80: The _port_in_use function uses netstat with the invalid -H
flag and a brittle grep pattern; change the netstat branch to call netstat -tln
(no -H), skip header rows if present, and extract the port robustly (e.g., use
awk to split the local-address column on ":" and take the last field or use a
regex to capture the trailing port) so IPv4, IPv6 (::) and formats with brackets
are handled; keep the ss branch as-is (ss -tlnH), replace the grep ":${p}$"
check with a comparison against the extracted port (or grep -E "(:|\\])${p}$" if
you prefer) so _port_in_use reliably returns 0 when the port is in use across ss
and netstat.
---
Nitpick comments:
In `@install.sh`:
- Line 98: Replace the raw echo statement that logs ports being free with the
script's info() helper to maintain consistent formatting and color; locate the
line printing "[INFO] Ports ${NEMOCLAW_GATEWAY_PORT} (gateway) and
${NEMOCLAW_DASHBOARD_PORT} (dashboard) are free." and call info with the same
message string so it uses the existing info() function's output style.
| _port_in_use() { | ||
| local p="$1" | ||
| if command -v ss &>/dev/null; then | ||
| ss -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$" | ||
| return $? | ||
| elif command -v netstat &>/dev/null; then | ||
| netstat -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$" | ||
| return $? | ||
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify netstat flag support and output format differences
echo "=== Checking ss availability and -H flag ==="
if command -v ss &>/dev/null; then
echo "ss is available"
ss -tlnH 2>&1 | head -3
else
echo "ss not found"
fi
echo ""
echo "=== Checking netstat -H flag (expected to fail on most systems) ==="
if command -v netstat &>/dev/null; then
echo "netstat is available"
netstat -tlnH 2>&1 | head -3
echo ""
echo "=== netstat without -H for comparison ==="
netstat -tln 2>&1 | head -5
else
echo "netstat not found"
fiRepository: NVIDIA/NemoClaw
Length of output: 203
🏁 Script executed:
# Find and read the install.sh file
find . -name "install.sh" -type f | head -5Repository: NVIDIA/NemoClaw
Length of output: 92
🏁 Script executed:
# Search for netstat documentation or usage patterns in the codebase
rg "netstat" --type sh -B 2 -A 2Repository: NVIDIA/NemoClaw
Length of output: 332
🌐 Web query:
netstat -H flag compatibility unix systems
💡 Result:
netstat -H is not a portable/standard netstat option across Unix-like systems (Linux net-tools netstat doesn’t document -H, and BSD netstat uses -h for other purposes, not -H). (mankier.com)
What is widely compatible is H as a routing-table output flag (not a command-line option):
- On Solaris, when you run
netstat -rn, the routing table “Flags” column can includeHmeaning “route is to a host and not a network”. (docs.oracle.com) - On FreeBSD, the routing table flags include
H(RTF_HOST) meaning “Host entry (net otherwise)”. (man.freebsd.org)
So if you’re trying to detect “host routes” portably, do something like:
netstat -rnand then interpret the Flags column; don’t rely on a-Hoption. (docs.oracle.com)
Citations:
- 1: https://www.mankier.com/8/netstat
- 2: https://docs.oracle.com/cd/E88353_01/html/E72487/netstat-8.html
- 3: https://man.freebsd.org/netstat
- 4: https://docs.oracle.com/cd/E88353_01/html/E72487/netstat-8.html
🏁 Script executed:
# Read the install.sh file to confirm context and line numbers
head -100 install.sh | tail -40Repository: NVIDIA/NemoClaw
Length of output: 1622
🏁 Script executed:
# Check for any IPv6 related patterns or tests in the codebase
rg -i "ipv6|\[::\]|::1" --type sh -B 2 -A 2Repository: NVIDIA/NemoClaw
Length of output: 305
netstat does not support the -H flag on most Unix-like systems, causing silent failures on BSD, macOS, and traditional Linux installations.
The -H (no header) flag is specific to ss from iproute2. Traditional netstat on BSD, macOS, and systems using net-tools does not recognize -H and will error or ignore it. When netstat -tlnH fails, the fallback returns 1 (port free), which silently misses port conflicts during deployment.
Additionally, the grep pattern :${p}$ may not match all IPv6 listener formats consistently.
🐛 Proposed fix
_port_in_use() {
local p="$1"
if command -v ss &>/dev/null; then
ss -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$"
return $?
elif command -v netstat &>/dev/null; then
- netstat -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$"
+ # netstat output varies by OS; skip header with tail, match port at end
+ netstat -tln 2>/dev/null | tail -n +3 | awk '{print $4}' | grep -qE "[:.]${p}$"
return $?
fi
return 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _port_in_use() { | |
| local p="$1" | |
| if command -v ss &>/dev/null; then | |
| ss -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$" | |
| return $? | |
| elif command -v netstat &>/dev/null; then | |
| netstat -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$" | |
| return $? | |
| fi | |
| return 1 | |
| } | |
| _port_in_use() { | |
| local p="$1" | |
| if command -v ss &>/dev/null; then | |
| ss -tlnH 2>/dev/null | awk '{print $4}' | grep -q ":${p}$" | |
| return $? | |
| elif command -v netstat &>/dev/null; then | |
| # netstat lacks -H flag; skip single header line | |
| netstat -tln 2>/dev/null | tail -n +2 | awk '{print $4}' | grep -qE ":${p}$" | |
| return $? | |
| fi | |
| return 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 70 - 80, The _port_in_use function uses netstat with
the invalid -H flag and a brittle grep pattern; change the netstat branch to
call netstat -tln (no -H), skip header rows if present, and extract the port
robustly (e.g., use awk to split the local-address column on ":" and take the
last field or use a regex to capture the trailing port) so IPv4, IPv6 (::) and
formats with brackets are handled; keep the ss branch as-is (ss -tlnH), replace
the grep ":${p}$" check with a comparison against the extracted port (or grep -E
"(:|\\])${p}$" if you prefer) so _port_in_use reliably returns 0 when the port
is in use across ss and netstat.
| if command_exists sha256sum; then | ||
| check_required_ports | ||
| actual_hash="$(sha256sum "$nvm_tmp" | awk '{print $1}')" |
There was a problem hiding this comment.
Critical: check_required_ports is incorrectly placed inside conditional hash-check logic.
The port check is nested inside the if command_exists sha256sum branch within install_nodejs(). This means:
- Skipped if Node.js exists:
install_nodejs()returns early at line 129 if node is found, so ports are never checked for existing Node.js installations. - Skipped if sha256sum unavailable: The check only runs in the
sha256sumbranch, not theshasumfallback (lines 144-145) or the "no tool" fallback (lines 147-148). - Wrong scope: Port availability is a preflight concern independent of Node.js installation.
Move check_required_ports to the beginning of main() before any installation steps.
🐛 Proposed fix
Remove the misplaced call from line 142:
if command_exists sha256sum; then
-check_required_ports
actual_hash="$(sha256sum "$nvm_tmp" | awk '{print $1}')"Add the port check at the start of main():
main() {
info "=== NemoClaw Installer ==="
+
+ check_required_ports
install_nodejs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 141 - 143, The call to check_required_ports is
incorrectly nested inside install_nodejs’s sha256sum branch; move the check out
of install_nodejs and invoke check_required_ports at the start of main() before
any installation logic so port checks run regardless of Node.js presence or
which checksum tool (sha256sum/shasum) is available; remove the misplaced
check_required_ports invocation inside the command_exists sha256sum conditional
(and ensure no duplicate calls remain).
|
Thanks for the contribution! We're going with a different approach for this one that integrates the port check into the Node.js preflight step rather than install.sh. Closing in favor of #251. |
…IA#329) Docker Desktop 29.x defaults to private cgroupns which prevents k3s kubelet from accessing cgroup v2 controllers (cpu, cpuset, memory, pids, hugetlb). This causes ContainerManager to fail during startup. Explicitly set cgroupns_mode to host, which is backwards compatible with all Docker versions and matches what k3s-in-Docker tooling (k3d) requires.
Fixes #51
install.sh starts the OpenShell gateway on port 8080 and the OpenClaw dashboard on port 18789 without checking whether those ports are already in use. Users with existing Docker containers, web servers, or other services on those ports get a cryptic startup failure:
Error: listen tcp 0.0.0.0:8080: bind: address already in use
This change adds a check_required_ports() preflight function that runs before any gateway or sandbox setup:
Summary by CodeRabbit