-
Notifications
You must be signed in to change notification settings - Fork 1
feat(api): auto-wire backend services at startup #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4644319
feat(api): auto-wire backend services at startup
Aureliolo 3afea9e
fix(test): suppress mypy unreachable false positives in auto-wire tests
Aureliolo b81fdd6
refactor(api): extract auto-wire into module, fix review findings
Aureliolo a71b39f
fix(test): fix mypy attr-defined error in channel assertion
Aureliolo e564b5f
fix: address 21 PR review findings from 14 agents + external reviewers
Aureliolo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| from synthorg.api.auth.middleware import create_auth_middleware_class | ||
| from synthorg.api.auth.secret import resolve_jwt_secret | ||
| from synthorg.api.auth.service import AuthService | ||
| from synthorg.api.auto_wire import auto_wire_phase1, auto_wire_settings | ||
| from synthorg.api.bus_bridge import MessageBusBridge | ||
| from synthorg.api.channels import ( | ||
| CHANNEL_APPROVALS, | ||
|
|
@@ -64,6 +65,7 @@ | |
| from synthorg.persistence.config import PersistenceConfig, SQLiteConfig | ||
| from synthorg.persistence.factory import create_backend | ||
| from synthorg.persistence.protocol import PersistenceBackend # noqa: TC001 | ||
| from synthorg.providers.registry import ProviderRegistry # noqa: TC001 | ||
| from synthorg.settings.dispatcher import SettingsChangeDispatcher | ||
| from synthorg.settings.subscribers import ( | ||
| BackupSettingsSubscriber, | ||
|
|
@@ -176,7 +178,7 @@ async def _ticket_cleanup_loop(app_state: AppState) -> None: | |
| ) | ||
|
|
||
|
|
||
| def _build_lifecycle( # noqa: PLR0913 | ||
| def _build_lifecycle( # noqa: PLR0913, C901, D417 | ||
| persistence: PersistenceBackend | None, | ||
| message_bus: MessageBus | None, | ||
| bridge: MessageBusBridge | None, | ||
|
|
@@ -185,16 +187,26 @@ def _build_lifecycle( # noqa: PLR0913 | |
| meeting_scheduler: MeetingScheduler | None, | ||
| backup_service: BackupService | None, | ||
| app_state: AppState, | ||
| *, | ||
| should_auto_wire_settings: bool = False, | ||
| effective_config: RootConfig | None = None, | ||
| ) -> tuple[ | ||
| Sequence[Callable[[], Awaitable[None]]], | ||
| Sequence[Callable[[], Awaitable[None]]], | ||
| ]: | ||
| """Build startup and shutdown hooks. | ||
|
|
||
| Args: | ||
| should_auto_wire_settings: When ``True``, Phase 2 auto-wiring | ||
| creates ``SettingsService`` + dispatcher after persistence | ||
| connects. | ||
| effective_config: Root config needed for Phase 2 auto-wiring. | ||
|
|
||
| Returns: | ||
| A tuple of (on_startup, on_shutdown) callback lists. | ||
| """ | ||
| _ticket_cleanup_task: asyncio.Task[None] | None = None | ||
| _auto_wired_dispatcher: SettingsChangeDispatcher | None = None | ||
|
|
||
| def _on_cleanup_task_done(task: asyncio.Task[None]) -> None: | ||
| """Log unexpected cleanup-task death.""" | ||
|
|
@@ -209,7 +221,7 @@ def _on_cleanup_task_done(task: asyncio.Task[None]) -> None: | |
| ) | ||
|
|
||
| async def on_startup() -> None: | ||
| nonlocal _ticket_cleanup_task | ||
| nonlocal _ticket_cleanup_task, _auto_wired_dispatcher | ||
| logger.info(API_APP_STARTUP, version=__version__) | ||
| await _safe_startup( | ||
| persistence, | ||
|
|
@@ -221,20 +233,60 @@ async def on_startup() -> None: | |
| backup_service, | ||
| app_state, | ||
| ) | ||
| # Phase 2 auto-wire: SettingsService (needs connected persistence) | ||
| if ( | ||
| should_auto_wire_settings | ||
| and persistence is not None | ||
| and effective_config is not None | ||
| and not app_state.has_settings_service | ||
| ): | ||
| try: | ||
| _auto_wired_dispatcher = await auto_wire_settings( | ||
| persistence, | ||
| message_bus, | ||
| effective_config, | ||
| app_state, | ||
| backup_service, | ||
| _build_settings_dispatcher, | ||
| ) | ||
| except MemoryError, RecursionError: | ||
| raise | ||
|
Comment on lines
+259
to
+260
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| except Exception: | ||
| logger.exception( | ||
| API_APP_STARTUP, | ||
| error="Phase 2 auto-wire failed", | ||
| ) | ||
| await _safe_shutdown( | ||
| task_engine, | ||
| meeting_scheduler, | ||
| backup_service, | ||
| settings_dispatcher, | ||
| bridge, | ||
| message_bus, | ||
| persistence, | ||
| ) | ||
| raise | ||
| _ticket_cleanup_task = asyncio.create_task( | ||
| _ticket_cleanup_loop(app_state), | ||
| name="ws-ticket-cleanup", | ||
| ) | ||
| _ticket_cleanup_task.add_done_callback(_on_cleanup_task_done) | ||
|
|
||
| async def on_shutdown() -> None: | ||
| nonlocal _ticket_cleanup_task | ||
| nonlocal _ticket_cleanup_task, _auto_wired_dispatcher | ||
| if _ticket_cleanup_task is not None: | ||
| _ticket_cleanup_task.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await _ticket_cleanup_task | ||
| _ticket_cleanup_task = None | ||
| logger.info(API_APP_SHUTDOWN, version=__version__) | ||
| if _auto_wired_dispatcher is not None: | ||
| await _try_stop( | ||
| _auto_wired_dispatcher.stop(), | ||
| API_APP_SHUTDOWN, | ||
| "Failed to stop auto-wired settings dispatcher", | ||
| ) | ||
| _auto_wired_dispatcher = None | ||
| await _safe_shutdown( | ||
| task_engine, | ||
| meeting_scheduler, | ||
|
|
@@ -614,16 +666,13 @@ def create_app( # noqa: PLR0913 | |
| meeting_scheduler: MeetingScheduler | None = None, | ||
| performance_tracker: PerformanceTracker | None = None, | ||
| settings_service: SettingsService | None = None, | ||
| provider_registry: ProviderRegistry | None = None, | ||
| ) -> Litestar: | ||
| """Create and configure the Litestar application. | ||
|
|
||
| All parameters are optional for testing — provide fakes via | ||
| keyword arguments. | ||
|
|
||
| When ``persistence`` is not provided, the factory checks | ||
| ``SYNTHORG_DB_PATH`` in the environment and auto-creates a | ||
| SQLite backend if set (used by the Docker compose template). | ||
| Explicit ``persistence`` always takes precedence. | ||
| All parameters are optional for testing -- provide fakes via | ||
| keyword arguments. Services not explicitly provided are | ||
| auto-wired from config and environment variables. | ||
|
|
||
| Args: | ||
| config: Root company configuration. | ||
|
|
@@ -639,6 +688,7 @@ def create_app( # noqa: PLR0913 | |
| meeting_scheduler: Meeting scheduler. | ||
| performance_tracker: Performance tracking service. | ||
| settings_service: Settings service for runtime config. | ||
| provider_registry: Provider registry. | ||
|
|
||
| Returns: | ||
| Configured Litestar application. | ||
|
|
@@ -677,20 +727,19 @@ def create_app( # noqa: PLR0913 | |
| db_path=db_path, | ||
| ) | ||
|
|
||
| if ( | ||
| persistence is None | ||
| or message_bus is None | ||
| or cost_tracker is None | ||
| or task_engine is None | ||
| or settings_service is None | ||
| ): | ||
| msg = ( | ||
| "create_app called without persistence, message_bus, " | ||
| "cost_tracker, task_engine, and/or settings_service — " | ||
| "controllers accessing missing services will return 503. " | ||
| "Use test fakes for testing." | ||
| ) | ||
| logger.warning(API_APP_STARTUP, note=msg) | ||
| # ── Phase 1 auto-wire: services that don't need connected persistence ── | ||
| phase1 = auto_wire_phase1( | ||
| effective_config=effective_config, | ||
| persistence=persistence, | ||
| message_bus=message_bus, | ||
| cost_tracker=cost_tracker, | ||
| task_engine=task_engine, | ||
| provider_registry=provider_registry, | ||
| ) | ||
| message_bus = phase1.message_bus | ||
| cost_tracker = phase1.cost_tracker | ||
| task_engine = phase1.task_engine | ||
| provider_registry = phase1.provider_registry | ||
|
|
||
| channels_plugin = create_channels_plugin() | ||
| expire_callback = _make_expire_callback(channels_plugin) | ||
|
|
@@ -718,6 +767,7 @@ def create_app( # noqa: PLR0913 | |
| meeting_scheduler=meeting_scheduler, | ||
| performance_tracker=performance_tracker, | ||
| settings_service=settings_service, | ||
| provider_registry=provider_registry, | ||
| startup_time=time.monotonic(), | ||
| ) | ||
|
|
||
|
|
@@ -743,6 +793,10 @@ def create_app( # noqa: PLR0913 | |
| guards=[require_password_changed], | ||
| ) | ||
|
|
||
| # Phase 2 auto-wiring flag: SettingsService needs connected persistence | ||
| # and is created in on_startup after _init_persistence(). | ||
| _should_auto_wire = settings_service is None and persistence is not None | ||
|
|
||
| startup, shutdown = _build_lifecycle( | ||
| persistence, | ||
| message_bus, | ||
|
|
@@ -752,6 +806,8 @@ def create_app( # noqa: PLR0913 | |
| meeting_scheduler, | ||
| backup_service, | ||
| app_state, | ||
| should_auto_wire_settings=_should_auto_wire, | ||
| effective_config=effective_config, | ||
| ) | ||
|
|
||
| return Litestar( | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
D417noqaindicates that the docstring for_build_lifecycleis missing descriptions for some arguments. It's important to keep docstrings up-to-date for maintainability and clarity, especially for complex functions. Please add descriptions forshould_auto_wire_settingsandeffective_configto the docstring.