FIX: PyRITShell startup deadlock and improve shell startup time#1489
FIX: PyRITShell startup deadlock and improve shell startup time#1489rlundeen2 merged 14 commits intomicrosoft:mainfrom
Conversation
cmdloop() was waiting for background initialization to complete before playing the banner animation, defeating the purpose of the background thread and causing ~14s startup instead of ~4s. Changes: - Play banner animation immediately while init runs in background - Suppress root logger during animation to prevent ANSI cursor corruption - Surface init errors after animation if init already finished - Add test verifying cmdloop does not block on slow init Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…startup The module-level 'from pyrit.cli import frontend_core' took ~11s because it transitively imports pyrit.scenario. This blocked the shell from starting before all modules were loaded. Changes: - Move frontend_core import to TYPE_CHECKING (type hints only) - Background thread now handles: import frontend_core -> create context -> initialize_async (all ~14s of work off the main thread) - main() plays banner animation immediately, then creates shell with context_kwargs for deferred construction - Shell methods use lazy 'from pyrit.cli import frontend_core' which is free (sys.modules lookup) after the background thread imports it - PyRITShell.__init__ accepts either context= (tests) or context_kwargs= (CLI startup) for backward compatibility Result: module import drops from ~14s to ~3s. The prompt appears after the banner animation (~5s total) while init continues in background. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Runs pyrit_shell module import in a subprocess and asserts it completes within 5 seconds. Guards against regressions from heavy top-level imports that would delay the banner animation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Raise ValueError if both context and context_kwargs are provided - Add test for context_kwargs background initialization path - Add test for the mutual exclusivity validation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The _resolve_env_files() method returns Optional[Sequence[Path]], not Optional[list[Path]]. Widen the type annotation and move the import into TYPE_CHECKING to satisfy both mypy and ruff TC003. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace untyped **context_kwargs with explicit keyword-only parameters mirroring FrontendCore.__init__ (config_file, database, env_files, etc.). This provides strong typing at the call site without importing the heavy frontend_core module. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a 'shell' fixture that creates an already-initialized PyRITShell without spawning a thread or running asyncio.run. Only the 4 init/thread-specific tests still use the real background thread via mock_fc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add back context as an optional deprecated parameter to PyRITShell. Emits a DeprecationWarning via print_deprecation_message (removed in 0.14.0). Raises ValueError if context is provided together with any FrontendCore keyword arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove test_cmdloop_does_not_hang_when_background_init_fails and test_cmdloop_does_not_block_on_slow_init from unit tests. These thread-timing tests are flaky on CI; startup performance is already covered by the integration test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move constants, ARG_HELP, validators, argparse wrappers, and parsers from frontend_core.py into _cli_args.py (stdlib-only imports). Both pyrit_shell and pyrit_scan can now reference help text and constants without the heavy frontend_core import. frontend_core re-exports everything for backward compatibility. Also replaces per-method lazy frontend_core imports in PyRITShell with a single self._fc reference set during background init. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a PyRITShell initialization hang on background-thread failure and restores fast perceived startup by deferring heavy CLI imports/initialization to a background thread. It also factors shared CLI argument parsing/validation into a new lightweight module and updates unit/integration tests accordingly.
Changes:
- Make background initialization failure-safe (always releases the init barrier and re-raises the original error).
- Defer
frontend_coreimport /FrontendCore.initialize_async()to a background thread, allowing the banner/prompt to appear immediately. - Extract shared CLI constants/validators/parsers into
pyrit.cli._cli_argsand adjust shell/tests to use the new structure.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pyrit/cli/pyrit_shell.py |
Moves heavy initialization off the main thread, adds failure propagation, and updates CLI entrypoint to keep startup responsive. |
pyrit/cli/frontend_core.py |
Re-exports shared CLI argument helpers/constants from the new lightweight module. |
pyrit/cli/_cli_args.py |
New stdlib-only shared CLI args/validators/parsers to enable fast imports for CLI frontends. |
tests/unit/cli/test_pyrit_shell.py |
Refactors unit tests to work with deferred background initialization and new shell init contract. |
tests/integration/cli/test_pyrit_shell_startup.py |
Adds an import-time budget regression test for pyrit_shell startup performance. |
tests/integration/cli/__init__.py |
Adds package init for integration CLI tests. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR targets PyRIT CLI usability and reliability by preventing pyrit_shell from hanging when background initialization fails, and by improving perceived startup time via deferring heavy imports/initialization to a background thread. It also factors shared CLI argument definitions into a lightweight module and adds tests to guard against startup regressions.
Changes:
- Make
PyRITShellinitializefrontend_coreand runFrontendCore.initialize_async()in a background thread, unblocking waiters even on failure. - Add a lightweight
pyrit.cli._cli_argsmodule and re-export its symbols fromfrontend_core. - Add/adjust unit tests and introduce an integration test enforcing a module import-time budget for
pyrit_shell.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
pyrit/cli/pyrit_shell.py |
Background initialization, new init kwargs, banner/logging behavior changes, and updated CLI parsing path. |
pyrit/cli/frontend_core.py |
Re-exports CLI arg parsing/validation utilities from _cli_args instead of defining them locally. |
pyrit/cli/_cli_args.py |
New stdlib-only shared CLI constants/validators/parsers/help text to support fast startup. |
tests/unit/cli/test_pyrit_shell.py |
Refactors tests around deferred init behavior, adds failure/deprecation coverage, updates main() expectations. |
tests/integration/cli/test_pyrit_shell_startup.py |
New subprocess-based import-time budget test to prevent heavy top-level import regressions. |
tests/integration/cli/__init__.py |
Adds package marker + licensing header. |
You can also share your feedback on Copilot code review. Take the survey.
…correctness - Use logging.disable() instead of setLevel() to avoid race with background thread - do_help() no longer blocks on full init; imports ARG_HELP from lightweight _cli_args - Convert string log_level to int via validate_log_level() in main() - Validate env file existence with resolve_env_files() in main() - Move print_deprecation_message to top-level import - Fix parse_run_arguments docstring: initializers type is str | dict - Include stderr in startup integration test failure message Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ca7bc09 to
f7ced9a
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…osoft#1489) Co-authored-by: Richard Lundeen <rlundeen@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…osoft#1489) Co-authored-by: Richard Lundeen <rlundeen@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
instead of hanging indefinitely.
As @rlundeen2 was testing this, noticed some things to fix:
immediately while initialization completes in the background. This was broken when we introduced the banner. Added a integration test for < 6s startup time
FrontendCorewith explicit keyword args (database, log_level, etc.) socallers get strong typing without the heavy import. Original context param preserved as deprecated.
_cli_args module: Extract constants, ARG_HELP, validators, and parsers from frontend_core into alightweight stdlib-only module. Eliminates duplicated help text across CLI entry points and allows for fast loading. frontend_core re-exports everything.