feat(aitools): add FastAPI router and config hot reload#1000
Conversation
Signed-off-by: zijian2 <tangzj0911@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the application startup and lifecycle management, centralizing it into the AIToolsServer class. The addition of hot-reloading for both configuration and routes using watchdog and process locks is a great feature for development and dynamic environments. The code is generally of high quality with good test coverage for the new features. I've found one critical issue regarding an incorrect value in the uvicorn configuration that could prevent the server from starting, and a high-severity maintainability issue related to duplicated code for process locking.
| workers=20, | ||
| reload=False, | ||
| ws_ping_interval=None, | ||
| ws_ping_timeout=NotImplemented, |
There was a problem hiding this comment.
The ws_ping_timeout parameter in uvicorn.Config is set to NotImplemented. According to uvicorn's documentation and type hints, this parameter expects a float or None. Using the NotImplemented singleton will likely cause a TypeError at runtime when uvicorn processes this configuration, which could prevent the server from starting or cause unexpected behavior with WebSocket connections.
| ws_ping_timeout=NotImplemented, | |
| ws_ping_timeout=None, |
| def _acquire_process_lock(self) -> bool: | ||
| """Acquire cross-process lock so only one worker watches polaris.""" | ||
| if not self.env_loader: | ||
| log.error("Env loader is not initialized; skip config watcher lock.") | ||
| return False | ||
|
|
||
| lock_path = ( | ||
| Path(self.env_loader.env_file_path).resolve().parent | ||
| / ".config_watcher.pid.lock" | ||
| ) | ||
| # Keep file descriptor open for lock lifetime. | ||
| # pylint: disable=consider-using-with | ||
| lock_file = open(lock_path, "w", encoding="utf-8") | ||
| try: | ||
| fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | ||
| self._lock_file = lock_file | ||
| self._owns_lock = True | ||
| return True | ||
| except BlockingIOError: | ||
| lock_file.close() | ||
| return False | ||
|
|
||
| def _release_process_lock(self) -> None: | ||
| """Release process lock if held by current worker.""" | ||
| if not self._lock_file: | ||
| return | ||
|
|
||
| if self._owns_lock: | ||
| try: | ||
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_UN) | ||
| except OSError: | ||
| pass | ||
| self._lock_file.close() | ||
| self._lock_file = None | ||
| self._owns_lock = False |
There was a problem hiding this comment.
The process locking logic implemented in _acquire_process_lock and _release_process_lock is duplicated from core/plugin/aitools/utils/route_utils.py. While the pylint: disable=duplicate-code comment shows awareness, this duplication presents a maintainability challenge. Any future bug fixes or enhancements to the locking mechanism will need to be applied in both locations. To improve maintainability, this logic should be extracted into a reusable utility class or context manager that can be shared between ConfigWatcher and RouteReloadWatcher.
1 participant
Summary
Add FastAPI router hot reload and config hot reload support for AITools, and refactor application startup so runtime watchers are managed explicitly by the server lifecycle instead of the old start module.
Type of Change
Related Issue
N/A
Changes
1) Add dynamic route hot reload for service changes
core/plugin/aitools/utils/route_utils.pywatchdogto monitor the AITools service directory.requirements.txtchanges under the watched service directory.2) Add config hot reload lifecycle integration
core/plugin/aitools/utils/config_utils.pycore/plugin/aitools/app/aitools_server.py3) Refactor app startup structure around server-owned lifecycle
core/plugin/aitools/app/aitools_server.pycore/plugin/aitools/app/app_factory.pycore/plugin/aitools/app/start_server.pycore/plugin/aitools/main.py.AIToolsServer.4) Update runtime configuration and dependencies
core/plugin/aitools/pyproject.tomlcore/plugin/aitools/config.envdocker/astronAgent/config/aitools/config.envhelm/astron-agent/files/config/aitools/config.envwatchdogdependency required for route watching.SERVICE_APPto point atplugin.aitools.app.app_factory:aitools_app.HOT_RELOAD_ENABLEandCONFIG_WATCH_INTERVAL.5) Add and update test coverage for reload behavior and startup flow
core/plugin/aitools/tests/api/routes/test_register_reload.pycore/plugin/aitools/tests/utils/test_route_utils.pyTesting
Test scope included in this work:
make testpytest -q tests/utils/test_route_utils.py tests/utils/test_config_utils.pyCompatibility / Risk
Rollback Plan
If regressions are found:
aitools_server.py,app_factory.py, andmain.py.route_utils.pyand related registration changes.SERVICE_APPentry and dependency set.Checklist