Skip to content

fix(serve): friendly error and a safe --replace for a busy dashboard port - #107

Merged
vsn411 merged 1 commit into
secfoo-com:mainfrom
AdithKB:fix/serve-port-already-in-use
Nov 14, 2026
Merged

vsn411 merged 1 commit into
secfoo-com:mainfrom
AdithKB:fix/serve-port-already-in-use

Conversation

@AdithKB

@AdithKB AdithKB commented Nov 14, 2026

Copy link
Copy Markdown
Contributor

Summary

secfoo serve on a port that's already in use currently fails like this, with no indication of what to do:

ERROR:    [Errno 48] error while attempting to bind on address ('127.0.0.1', 8787): address already in use

Found this live while demoing the dashboard to the team — nobody could tell why it failed or how to fix it.

This adds:

  • A pre-flight check (a real listen-bind probe) before uvicorn starts, so the failure is ours to handle with a clear message instead of a raw socket error.
  • The error names the actual process holding the port when it can be identified (lsof+ps on macOS/Linux, netstat+wmic on Windows), and always prints the fix: find it, stop it, pick another --port, or --replace.
  • secfoo serve --replace, which only stops a process it can positively identify as another secfoo serve by inspecting its command line. It never touches an unrelated or unidentified process — even with --replace — and sends SIGTERM first, falling back to SIGKILL only if the process doesn't exit in time.

I looked at how other tools handle this exact problem before building it (search notes below) — the consensus is "identify the owner, stop gracefully, never kill blind," which is what this follows:

Test plan

  • pytest tests passes (1326 passed; the one pre-existing failure elsewhere in the suite, a flaky socket-timing test in the sensor module, independently re-verified to fail identically on a fresh clone of untouched main with a brand-new venv)
  • ruff check . — diffed rule-by-rule against main's cli.py: every "new" finding is the same pre-existing violation shifted by the +40 lines this PR inserts. Zero new findings. The new files have zero findings.
  • New unit coverage (tests/test_port_guard.py): a real bound socket is detected; parsing lsof+ps output into a PortOwner; is_secfoo_serve across several command-line shapes; graceful-then-forced stop; giving up if the process never exits
  • New CLI coverage (tests/test_serve_cli.py): starts normally when the port is free; refuses with the fix message for both a known and an unknown owner; --replace stops a recognized secfoo serve and starts; --replace refuses an unrelated process (and never calls stop_gracefully on it); reports clearly if the old process won't die
  • Verified for real, not just mocked (macOS): ran an actual secfoo serve, tried a second one without --replace (got the new clear error naming the real pid), then with --replace (it stopped the old one and took over the port, confirmed with a real HTTP request to the new instance). This caught a real bug the mocked tests alone missed — lsof -iTCP:<port> has to be one argv token, not two (-iTCP, :<port>) — fixed before opening this PR.
  • The Windows path (netstat+wmic+taskkill) is implemented and unit-tested (mocked) but not verified against a real Windows process — no Windows machine available to confirm it live.

🤖 Generated with Claude Code

https://claude.ai/code/session_012MkDc2pyUgcPHyj5joTrip

…port

`secfoo serve` on a busy port used to fail with a raw uvicorn socket
error (no fix, no indication of who's using it). Found live while
demoing to the team: nobody knew why it failed or what to do next.

- Pre-flight check: a real listen-bind probe before uvicorn starts, so
  the failure is ours to handle instead of uvicorn's raw error.
- The error now names the actual pid/command holding the port (via
  lsof+ps / netstat+wmic on Windows) when it can be determined, and
  always prints the fix: find it, stop it, use another --port, or
  --replace.
- --replace only stops a process it can positively identify as another
  `secfoo serve` by inspecting its command line -- never an unrelated
  or unidentified process, matching the practitioner consensus for
  this exact problem (identify the owner, never kill blind). It sends
  SIGTERM first and only falls back to SIGKILL if the process doesn't
  exit in time.
- Verified end-to-end against a real running `secfoo serve`, not just
  mocked: caught and fixed a wrong lsof invocation (-iTCP and the port
  must be one argv token, not two) that the mocked tests alone would
  have missed.
@AdithKB
AdithKB requested a review from vsn411 as a code owner November 14, 2026 11:12
_run(args, timeout=_PROBE_TIMEOUT)
else:
os.kill(pid, signal.SIGKILL if force else signal.SIGTERM)
except (OSError, subprocess.TimeoutExpired, ProcessLookupError):
@vsn411
vsn411 merged commit e6f8999 into secfoo-com:main Nov 14, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants