FIX: Harden scanner server lifecycle#2239
Open
romanlutz wants to merge 1 commit into
Open
Conversation
Preserve layered configuration values, add configurable startup timing, and clean up failed launches. Persist validated listener PIDs and make scanner and shell shutdown outcomes accurate across platforms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9705ca6a-e3d9-4eb2-863d-93f705d91b30
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the PyRIT scanner’s local backend lifecycle and configuration layering so CLI runs reliably preserve explicit config values, wait appropriately for legitimate startup, correctly identify the real listener PID (including Windows venv child-process cases), and shut down safely (loopback-only) with accurate exit codes.
Changes:
- Reworked
ConfigurationLoader.load_with_overridesmerge semantics to preserve explicit falsy/empty/null values and support extension overlay/reset behavior. - Added validated
server.startup_timeout(default 120s) with--startup-timeoutoverride, and enforced startup deadlines with bounded health probes. - Persisted listener PID state with platform-specific PID discovery and process-tree cleanup/termination; updated CLI stop/start flows, docs, and added focused unit tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/setup/test_configuration_loader.py | Adds tests for explicit-field preservation, key-presence merge behavior, extension overlay/reset semantics, and server timeout validation. |
| tests/unit/cli/test_server_launcher.py | Adds unit tests covering startup timeout defaults, PID persistence, cancellation/timeout cleanup, process-tree termination, and Windows process existence behavior. |
| tests/unit/cli/test_pyrit_shell.py | Updates shell stop-server behavior to report failures and refuse remote shutdown attempts. |
| tests/unit/cli/test_pyrit_scan.py | Adds CLI arg validation for startup timeout and expands stop/start server resolution/exit-code behavior tests. |
| tests/unit/cli/test_config_reader.py | Adds tests for new read_server_settings defaults, overlay precedence, and timeout validation. |
| pyrit/setup/configuration_loader.py | Implements presence-aware layered merging and robust extensions handling; adds server.startup_timeout normalization/validation. |
| pyrit/cli/pyrit_shell.py | Ensures shell startup doesn’t impose an extra timeout and restricts shutdown to loopback servers with clearer outcomes. |
| pyrit/cli/pyrit_scan.py | Adds --startup-timeout, uses read_server_settings, tightens local-only start/stop semantics, and returns meaningful exit codes. |
| pyrit/cli/_server_launcher.py | Adds startup deadline logic, PID persistence and validation, multi-platform listener PID discovery, and process-tree termination helpers. |
| pyrit/cli/_config_reader.py | Expands lightweight CLI config reading to include startup timeout with validation and overlay semantics. |
| doc/getting_started/pyrit_conf.md | Documents the new server block fields and startup timeout behavior/constraints. |
| .pyrit_conf_example | Updates example config to include server.url and server.startup_timeout. |
Comment on lines
+539
to
+543
| _, port = local_address | ||
| if not stop_server_on_port(port=port): | ||
| print(f"Server at {base_url} is running but could not be stopped.") | ||
| print(f"Find and kill it manually: look for a process listening on port {port}.") | ||
| return 1 |
Comment on lines
+459
to
+463
| pid = self._process.pid | ||
| self._pid = pid | ||
| self._port = port | ||
| _write_pid_record(host=host, port=port, pid=pid) | ||
| _logger.info("Backend PID: %d (logs: %s)", pid, self._log_path) |
Comment on lines
+492
to
+495
| listener_pid = _find_pid_on_port(port=port) | ||
| if listener_pid is not None: | ||
| _write_pid_record(host=host, port=port, pid=listener_pid) | ||
| print(f"Server ready (PID {self._pid}). Logs: {self._log_path}") |
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
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.
Description
Scanner CLI runs could lose explicitly configured values, time out during legitimate backend startup, and fail to identify or stop the actual listener process. This made scenario execution unreliable and could leave launcher-owned processes running.
This change:
server.startup_timeoutsetting with a 120-second default and a--startup-timeoutoverride, enforced through a monotonic deadline and bounded health probes.A notable Windows detail is that virtual-environment Python may launch a child interpreter that owns the socket, so successful startup records the listener PID rather than assuming the original process PID owns it.
Tests and Documentation
tytype checking, repository pre-commit hooks, and live Windows startup/shutdown smoke tests.