fix: stale backend holding port blocks restart (reap own unhealthy listener; Windows netstat/taskkill fallback)#116
Open
smoochy wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds Windows-compatible fallback logic to backend-service.sh for detecting and terminating processes holding a TCP port when lsof is unavailable, using netstat, tasklist, and taskkill. The start flow now reaps stale reflexio/uvicorn/python holders before giving up on port conflicts. ChangesBackend service Windows reap support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Start as start command
participant PortCheck as port_occupied/port_holder
participant Reap as reap_port_listeners
participant OS as netstat/tasklist/taskkill
Start->>PortCheck: check if port occupied
PortCheck->>OS: query port_pids_netstat / pid_cmdline_win
OS-->>PortCheck: PID and command info
PortCheck-->>Start: holder description
alt holder matches reflexio/uvicorn/python
Start->>Reap: reap_port_listeners
Reap->>OS: port_pids_netstat / pid_cmdline_win
Reap->>OS: taskkill /F matched PIDs
Reap-->>Start: listeners terminated
Start->>PortCheck: re-check port availability
PortCheck-->>Start: port free or still occupied
else holder is foreign process
Start-->>Start: log and skip start
end
Related Issues: Suggested labels: bug, windows, backend-service Suggested reviewers: backend-service maintainers familiar with shell process handling 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/scripts/backend-service.sh`:
- Around line 233-237: The PID command lookup in backend-service.sh should fall
back to pid_cmdline_win when the ps-based cmdline lookup returns empty, not only
when ps is unavailable. Update the stale-listener matching logic around the
pid/cmdline resolution so the command line is populated from pid_cmdline_win
whenever ps -p "$pid" -o command= yields no result, keeping the existing command
detection flow intact.
- Around line 207-214: The Windows PID lookup in pid_cmdline_win currently
returns only the image name, which lets the generic python fallback match
unrelated processes. Update the backend-service.sh reaping flow so the Windows
path uses a real command line source or removes the broad *python* match and
only kills processes with project-specific markers. Keep the fix centered around
pid_cmdline_win and the stale backend detection logic that consumes its output.
- Around line 202-204: The PID extraction in the netstat pipeline still carries
Windows CRLF, which leaves a trailing carriage return on the value returned from
the listener lookup. Update the parsing in the backend-service.sh netstat/awk
flow so the emitted PID is normalized by stripping any trailing \r before it is
used by the stale-listener cleanup logic, ensuring the values match downstream
calls like tasklist, ps, and taskkill.
- Around line 246-248: The Windows-only direct taskkill path in
backend-service.sh should not bypass the existing kill-tree logic. Update the
branch in the shutdown/cleanup flow to reuse claude_smart_kill_tree for each PID
in ours so Git Bash slash escaping and child-process termination are handled
consistently, instead of calling taskkill /F /PID directly in the loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af6d4f10-675d-4021-b4dd-b2bd75ba99de
📒 Files selected for processing (1)
plugin/scripts/backend-service.sh
Comment on lines
+202
to
+204
| netstat -ano 2>/dev/null | awk -v p=":$1\$" ' | ||
| $1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { print $5 } | ||
| ' | sort -u |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' "== plugin/scripts/backend-service.sh (around lines 195-210) =="
sed -n '190,215p' plugin/scripts/backend-service.sh | cat -n
printf '\n%s\n' "== Where port_pids_netstat is used =="
rg -n "port_pids_netstat|taskkill|tasklist|reap_port_listeners|port_holder|pid_cmdline_win" plugin/scripts/backend-service.sh plugin/scripts/_lib.sh
printf '\n%s\n' "== Relevant helper docs =="
sed -n '1,220p' plugin/scripts/_lib.sh | cat -n | sed -n '1,220p'Repository: ReflexioAI/claude-smart
Length of output: 12950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,380p' plugin/scripts/backend-service.sh | cat -n
printf '\n%s\n' "== Simulate netstat CRLF parsing with awk =="
python3 - <<'PY'
import subprocess, textwrap, os, json, sys, tempfile
sample = textwrap.dedent("""\
TCP 127.0.0.1:8080 0.0.0.0:0 LISTENING 1234\r
TCP 127.0.0.1:8080 0.0.0.0:0 LISTENING 5678\r
""")
script = r'''awk -v p=":8080$" '
$1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { print $5 }
' | sort -u'''
proc = subprocess.run(
["bash", "-lc", script],
input=sample.encode(),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
print("stdout repr:", repr(proc.stdout.decode()))
print("stderr repr:", repr(proc.stderr.decode()))
PYRepository: ReflexioAI/claude-smart
Length of output: 7778
Strip \r from Windows netstat PIDs.
netstat -ano on Git Bash leaves CRLF on $5, so the PID becomes 1234\r and won’t match tasklist, ps, or taskkill. That breaks stale-listener cleanup on Windows.
Proposed fix
- $1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { print $5 }
+ $1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { gsub(/\r$/, "", $5); print $5 }📝 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.
Suggested change
| netstat -ano 2>/dev/null | awk -v p=":$1\$" ' | |
| $1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { print $5 } | |
| ' | sort -u | |
| netstat -ano 2>/dev/null | awk -v p=":$1\$" ' | |
| $1 ~ /^TCP/ && $2 ~ p && $4 == "LISTENING" { gsub(/\r$/, "", $5); print $5 } | |
| ' | sort -u |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/scripts/backend-service.sh` around lines 202 - 204, The PID extraction
in the netstat pipeline still carries Windows CRLF, which leaves a trailing
carriage return on the value returned from the listener lookup. Update the
parsing in the backend-service.sh netstat/awk flow so the emitted PID is
normalized by stripping any trailing \r before it is used by the stale-listener
cleanup logic, ensuring the values match downstream calls like tasklist, ps, and
taskkill.
Comment on lines
+246
to
+248
| if command -v taskkill >/dev/null 2>&1 && ! command -v lsof >/dev/null 2>&1; then | ||
| # Windows: no POSIX signals to these PIDs; force-kill directly. | ||
| for pid in $ours; do taskkill /F /PID "$pid" >/dev/null 2>&1 || true; done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the shared Git Bash-safe kill helper.
This direct taskkill /F /PID path bypasses the existing claude_smart_kill_tree handling for Git Bash slash escaping and child-process termination. Reuse the helper instead.
Proposed fix
if command -v taskkill >/dev/null 2>&1 && ! command -v lsof >/dev/null 2>&1; then
- # Windows: no POSIX signals to these PIDs; force-kill directly.
- for pid in $ours; do taskkill /F /PID "$pid" >/dev/null 2>&1 || true; done
+ # Windows: netstat returns native PIDs; shared helper handles taskkill safely.
+ for pid in $ours; do claude_smart_kill_tree "$pid"; done
return 0
fi📝 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.
Suggested change
| if command -v taskkill >/dev/null 2>&1 && ! command -v lsof >/dev/null 2>&1; then | |
| # Windows: no POSIX signals to these PIDs; force-kill directly. | |
| for pid in $ours; do taskkill /F /PID "$pid" >/dev/null 2>&1 || true; done | |
| if command -v taskkill >/dev/null 2>&1 && ! command -v lsof >/dev/null 2>&1; then | |
| # Windows: netstat returns native PIDs; shared helper handles taskkill safely. | |
| for pid in $ours; do claude_smart_kill_tree "$pid"; done | |
| return 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/scripts/backend-service.sh` around lines 246 - 248, The Windows-only
direct taskkill path in backend-service.sh should not bypass the existing
kill-tree logic. Update the branch in the shutdown/cleanup flow to reuse
claude_smart_kill_tree for each PID in ours so Git Bash slash escaping and
child-process termination are handled consistently, instead of calling taskkill
/F /PID directly in the loop.
fix: fall back to tasklist when Git Bash ps yields no cmdline reap_port_listeners only consulted pid_cmdline_win when `ps` was entirely absent. Git Bash ships `ps`, but it may not support `-o command=` or resolve a native PID from netstat, leaving cmdline empty so a stale listener never matched and was never reaped. Try tasklist whenever the ps lookup comes back empty. Addresses CodeRabbit review on ReflexioAI#116. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> @
Author
|
Thanks for the review — addressed in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In start: reap our own unhealthy holder and retry before skipping. Add netstat/tasklist/taskkill fallback when lsof is absent. Closes #108.
Summary by CodeRabbit