Improve stale entry cleanup for cross-platform compatibility#38
Conversation
Enhanced stale entry cleanup to support both Windows and Bash process lookups.
There was a problem hiding this comment.
Pull request overview
This PR improves stale server-registry entry cleanup in discover-servers.sh by adding Windows/MSYS/Cygwin-specific PID liveness checks so registry entries are removed correctly across mixed Windows/Bash process environments.
Changes:
- Adds an MSYS/Cygwin branch that checks for PID existence via
powershell.exe Get-Processbefore falling back tokill -0. - Keeps existing
kill -0behavior for non-Windows environments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ "$OSTYPE" == msys* || "$OSTYPE" == cygwin* ]]; then | ||
| if ! powershell.exe -NoProfile -Command "Get-Process -Id $pid -ErrorAction SilentlyContinue | Out-Null; if (\$?) { exit 0 } else { exit 1 }" \ | ||
| && ! kill -0 "$pid" 2>/dev/null; then |
There was a problem hiding this comment.
If powershell.exe is unavailable on a given MSYS/Cygwin setup, this will emit a "command not found" error to stderr on every iteration (even though the kill -0 fallback still works). Consider checking command -v powershell.exe first (or redirecting its stderr) to keep output clean and avoid noisy logs.
| if [[ "$OSTYPE" == msys* || "$OSTYPE" == cygwin* ]]; then | ||
| if ! powershell.exe -NoProfile -Command "Get-Process -Id $pid -ErrorAction SilentlyContinue | Out-Null; if (\$?) { exit 0 } else { exit 1 }" \ | ||
| && ! kill -0 "$pid" 2>/dev/null; then |
There was a problem hiding this comment.
$pid is interpolated directly into the PowerShell -Command string. Since the PID is read from a JSON file in the registry directory, a malformed or tampered .pid value could break the PowerShell command or be used for command injection. Please validate that pid is strictly numeric before using it, and pass it to PowerShell as an argument/parameter (instead of string interpolation) so it can’t alter the command text.
|
Oops, I only now noticed #31 . There's a lot of good stuff in that one, and it should be merged, but I think the implementation here might be cleaner. |
|
@copilot apply changes based on the comments in this thread |
|
@copilot resolve the merge conflicts in this pull request |
|
@daanzu it looks like copilot doesn't want to help. i merged my branch, let me know if that helps. happy to merge this if you want to resolve conflicts / comments |
Enhanced stale entry cleanup to support both Windows and Bash process lookups.