fix: [RBAC Migration]Hydrate workspace config at startup#1575
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds ChangesConfiguration Cache Hydration at Startup
Sequence Diagram(s)sequenceDiagram
participant ASGIApp
participant WorkspaceConfigService
participant ConfigManager
ASGIApp->>WorkspaceConfigService: call hydrate_on_startup()
WorkspaceConfigService->>ConfigManager: call load_config()
ConfigManager-->>WorkspaceConfigService: return loaded config
WorkspaceConfigService-->>ASGIApp: complete startup hydration (or raise/log error)
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
backend
github-actions
Bot
added
bug
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds an eager workspace-config hydration step during application startup to prevent DB-mode restarts from leaving the in-memory config unset (which can re-trigger onboarding UI behavior).
Changes:
- Add
WorkspaceConfigService.hydrate_on_startup()to preload configuration viaload_config()at startup. - Invoke hydration from
run_startup()and log failures without crashing startup. - Minor formatting change to shutdown logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/services/workspace_config_service.py | Introduces a startup hydration method that eagerly loads workspace config into memory. |
| src/app/lifespan.py | Calls hydration during startup with error logging; minor log formatting adjustment during shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| wcs = services.get("workspace_config_service") | ||
| if wcs is not None: | ||
| await wcs.hydrate_on_startup() | ||
| except Exception as e: | ||
| logger.error("Workspace config hydration failed at startup", error=str(e)) |
| """Eagerly populate ``config_manager._config`` from the DB at | ||
| lifespan startup. | ||
|
|
||
| Without this, in `db` mode a restart leaves ``_config = None`` | ||
| and the synchronous ``get_openrag_config()`` falls back to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/workspace_config_service.py (1)
171-171:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the type annotation to use PEP 604 union syntax.
The pipeline reports that
Optional[Any]should be replaced withAny | None(rule UP045).🔧 Proposed fix
- async def get_onboarding_step(self) -> Optional[Any]: + async def get_onboarding_step(self) -> Any | None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/workspace_config_service.py` at line 171, The return type annotation of get_onboarding_step should use PEP 604 union syntax; change its signature from "async def get_onboarding_step(self) -> Optional[Any]:" to use "-> Any | None" and update imports accordingly (remove Optional from typing imports if no longer used and ensure Any is imported). Also run a quick search for other Optional[Any] usages in this module to convert them to the new "Any | None" form for consistency with rule UP045.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/lifespan.py`:
- Around line 179-184: The current try/except around
services.get("workspace_config_service") and await wcs.hydrate_on_startup()
swallows errors and only logs them via logger.error, allowing startup to
continue with missing config; change the handler so hydration failures are
treated as fatal: when hydrate_on_startup() raises (or if wcs is None
unexpectedly), log the detailed error with logger.error including the exception,
then abort startup by re-raising the exception or invoking a clean process exit
(e.g., raise the caught exception or call sys.exit(1)) so the app does not start
with an empty/default workspace config.
---
Outside diff comments:
In `@src/services/workspace_config_service.py`:
- Line 171: The return type annotation of get_onboarding_step should use PEP 604
union syntax; change its signature from "async def get_onboarding_step(self) ->
Optional[Any]:" to use "-> Any | None" and update imports accordingly (remove
Optional from typing imports if no longer used and ensure Any is imported). Also
run a quick search for other Optional[Any] usages in this module to convert them
to the new "Any | None" form for consistency with rule UP045.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c61c2c7c-2aa2-4046-98cf-66ff7bad538d
📒 Files selected for processing (2)
src/app/lifespan.pysrc/services/workspace_config_service.py
| try: | ||
| wcs = services.get("workspace_config_service") | ||
| if wcs is not None: | ||
| await wcs.hydrate_on_startup() | ||
| except Exception as e: | ||
| logger.error("Workspace config hydration failed at startup", error=str(e)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Consider whether hydration failure should halt startup.
The error is logged but startup continues if hydrate_on_startup() fails. This means the app will serve requests with an empty/default config, potentially causing the onboarding wizard to reappear unexpectedly.
Given that the PR objective is to fix exactly this scenario (preventing the wizard from reappearing after restart), should a hydration failure be treated as fatal?
💡 Alternative: Fail fast on hydration error
try:
wcs = services.get("workspace_config_service")
if wcs is not None:
await wcs.hydrate_on_startup()
except Exception as e:
logger.error("Workspace config hydration failed at startup", error=str(e))
+ raiseThis would prevent the app from starting with stale/empty config, making the failure obvious rather than allowing degraded behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| wcs = services.get("workspace_config_service") | |
| if wcs is not None: | |
| await wcs.hydrate_on_startup() | |
| except Exception as e: | |
| logger.error("Workspace config hydration failed at startup", error=str(e)) | |
| try: | |
| wcs = services.get("workspace_config_service") | |
| if wcs is not None: | |
| await wcs.hydrate_on_startup() | |
| except Exception as e: | |
| logger.error("Workspace config hydration failed at startup", error=str(e)) | |
| raise |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/lifespan.py` around lines 179 - 184, The current try/except around
services.get("workspace_config_service") and await wcs.hydrate_on_startup()
swallows errors and only logs them via logger.error, allowing startup to
continue with missing config; change the handler so hydration failures are
treated as fatal: when hydrate_on_startup() raises (or if wcs is None
unexpectedly), log the detailed error with logger.error including the exception,
then abort startup by re-raising the exception or invoking a clean process exit
(e.g., raise the caught exception or call sys.exit(1)) so the app does not start
with an empty/default workspace config.
Add WorkspaceConfigService.hydrate_on_startup() which calls load_config() to eagerly populate the config manager from the DB during application startup. Call this method from the app lifespan (run_startup) and log errors if hydration fails. This prevents a DB-mode restart from leaving the in-memory config unset and causing the frontend onboarding wizard to reappear; load_config() is mode-aware, so invoking it unconditionally is safe.
8dc9ce5 to
6b556ee
Compare
github-actions
Bot
added
bug
Add WorkspaceConfigService.hydrate_on_startup() which calls load_config() to eagerly populate the config manager from the DB during application startup. Call this method from the app lifespan (run_startup) and log errors if hydration fails. This prevents a DB-mode restart from leaving the in-memory config unset and causing the frontend onboarding wizard to reappear; load_config() is mode-aware, so invoking it unconditionally is safe.
Summary by CodeRabbit
New Features
Bug Fixes
Chores