fix: lazy load settings service; fix env init order (#10142)#10307
Conversation
* Ensure settings services are lazy loaded; fixes settings init order * Remove dupe tests * remove useless test * [autofix.ci] apply automated fixes * Ruff * [autofix.ci] apply automated fixes * mypy * Fix test reference * ruff fixes * [autofix.ci] apply automated fixes * [autofix.ci] apply automated fixes (attempt 2/3) * ruff + starter projects --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
HimavarshaVS
requested review from
Cristhianzl,
edwinjosechittilappilly and
erichare
and removed request for
edwinjosechittilappilly and
erichare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces lazy-loading patterns for settings initialization and knowledge bases configuration across backend and lfx packages. Adds a guard in the main entry point to prevent env file loading after settings service initialization. Refactors eager path resolution to lazy evaluation and includes comprehensive test validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as __main__.py
participant Deps as deps.py
participant Settings as SettingsService
participant Dotenv as dotenv
rect rgb(200, 220, 255)
Note over Main,Deps: Initialization Guard Pattern
Main->>Deps: is_settings_service_initialized()?
Deps->>Deps: Check service_manager.services
Deps-->>Main: bool (True/False)
end
alt Settings Already Initialized
Main->>Main: Raise ValueError
else Settings Not Initialized
Main->>Dotenv: load_dotenv(env_file, override=True)
Dotenv-->>Main: Environment loaded
end
Main->>Settings: Initialize SettingsService
Settings-->>Main: Service ready
sequenceDiagram
participant Code as Component Code
participant Lazy as _get_knowledge_bases_root_path()
participant Cache as Internal Cache
participant Settings as Settings
rect rgb(220, 240, 220)
Note over Code,Settings: Lazy-Loading Pattern
end
Code->>Lazy: First call to _get_kb_path()
Lazy->>Cache: Check if cached?
Cache-->>Lazy: Not cached
Lazy->>Settings: Fetch knowledge_bases_directory
Settings-->>Lazy: Path config
Lazy->>Cache: Store in cache
Lazy-->>Code: Return Path
Code->>Lazy: Subsequent call
Lazy->>Cache: Check if cached?
Cache-->>Lazy: Found (cached)
Lazy-->>Code: Return cached Path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent lazy-loading pattern across multiple files, reducing review friction through homogeneous refactoring. However, the scope spans backend and lfx packages with mixed concerns (initialization guards, caching logic, test mocking), and a comprehensive new test module requires careful validation of initialization order guarantees and error handling paths. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (3 passed)
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 |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (17.14%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10307 +/- ##
==========================================
+ Coverage 29.98% 30.15% +0.17%
==========================================
Files 1315 1315
Lines 59309 59327 +18
Branches 8882 8883 +1
==========================================
+ Hits 17783 17890 +107
+ Misses 40708 40620 -88
+ Partials 818 817 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
added
bug
![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
🧹 Nitpick comments (3)
src/lfx/src/lfx/base/mcp/util.py (2)
38-62: Consider thread-safe initialization for MCP settings cache.Similar to the knowledge bases lazy loader, this implementation uses a check-and-set pattern that is not thread-safe. Since MCP sessions can be managed concurrently (as evidenced by the session manager's design), consider the same thread safety improvements suggested for the knowledge bases lazy loader.
Apply this diff to add thread safety:
+import threading + # MCP Session Manager constants - lazy loaded _mcp_settings_cache: dict[str, Any] = {} +_mcp_settings_cache_lock = threading.Lock() def _get_mcp_setting(key: str, default: Any = None) -> Any: """Lazy load MCP settings from settings service.""" - if key not in _mcp_settings_cache: - settings = get_settings_service().settings - _mcp_settings_cache[key] = getattr(settings, key, default) + with _mcp_settings_cache_lock: + if key not in _mcp_settings_cache: + settings = get_settings_service().settings + _mcp_settings_cache[key] = getattr(settings, key, default) return _mcp_settings_cache[key]
451-462: Exception handling may be too broad in cleanup loop.The enhanced exception handling in
_periodic_cleanup(line 459) catches a broad set of exceptions to prevent the cleanup loop from stopping. While this improves resilience, the broad exception list might mask unexpected errors that should be investigated.Consider logging the exception type and context for debugging purposes:
except asyncio.CancelledError: break except (RuntimeError, KeyError, ClosedResourceError, ValueError, asyncio.TimeoutError) as e: # Handle common recoverable errors without stopping the cleanup loop - await logger.awarning(f"Error in periodic cleanup: {e}") + await logger.awarning(f"Recoverable error in periodic cleanup ({type(e).__name__}): {e}", exc_info=True)src/lfx/src/lfx/components/knowledge_bases/retrieval.py (1)
19-32: Consider consolidating duplicated lazy loader implementations.The
_get_knowledge_bases_root_path()implementation is duplicated betweenretrieval.pyandingestion.py(as shown in relevant code snippets). This creates maintenance burden and increases the risk of inconsistencies.Consider moving the lazy loader to a shared utility module (e.g.,
lfx/components/knowledge_bases/utils.py):# lfx/components/knowledge_bases/utils.py import threading from pathlib import Path _KNOWLEDGE_BASES_ROOT_PATH: Path | None = None _KNOWLEDGE_BASES_ROOT_PATH_LOCK = threading.Lock() def get_knowledge_bases_root_path() -> Path: """Lazy load the knowledge bases root path from settings.""" global _KNOWLEDGE_BASES_ROOT_PATH # noqa: PLW0603 with _KNOWLEDGE_BASES_ROOT_PATH_LOCK: if _KNOWLEDGE_BASES_ROOT_PATH is None: from lfx.services.deps import get_settings_service settings = get_settings_service().settings knowledge_directory = settings.knowledge_bases_dir if not knowledge_directory: msg = "Knowledge bases directory is not set in the settings." raise ValueError(msg) _KNOWLEDGE_BASES_ROOT_PATH = Path(knowledge_directory).expanduser() return _KNOWLEDGE_BASES_ROOT_PATHThen import and use it in both files:
-_KNOWLEDGE_BASES_ROOT_PATH: Path | None = None - - -def _get_knowledge_bases_root_path() -> Path: - """Lazy load the knowledge bases root path from settings.""" - global _KNOWLEDGE_BASES_ROOT_PATH # noqa: PLW0603 - if _KNOWLEDGE_BASES_ROOT_PATH is None: - settings = get_settings_service().settings - knowledge_directory = settings.knowledge_bases_dir - if not knowledge_directory: - msg = "Knowledge bases directory is not set in the settings." - raise ValueError(msg) - _KNOWLEDGE_BASES_ROOT_PATH = Path(knowledge_directory).expanduser() - return _KNOWLEDGE_BASES_ROOT_PATH +from lfx.components.knowledge_bases.utils import get_knowledge_bases_root_path as _get_knowledge_bases_root_path
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/backend/base/langflow/__main__.py(1 hunks)src/backend/base/langflow/api/v1/knowledge_bases.py(2 hunks)src/backend/base/langflow/services/deps.py(1 hunks)src/backend/tests/unit/components/knowledge_bases/test_ingestion.py(1 hunks)src/backend/tests/unit/components/knowledge_bases/test_retrieval.py(1 hunks)src/backend/tests/unit/test_settings_initialization_order.py(1 hunks)src/lfx/src/lfx/base/mcp/util.py(3 hunks)src/lfx/src/lfx/components/knowledge_bases/ingestion.py(4 hunks)src/lfx/src/lfx/components/knowledge_bases/retrieval.py(3 hunks)src/lfx/src/lfx/graph/edge/base.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
{src/backend/**/*.py,tests/**/*.py,Makefile}
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
{src/backend/**/*.py,tests/**/*.py,Makefile}: Run make format_backend to format Python code before linting or committing changes
Run make lint to perform linting checks on backend Python code
Files:
src/backend/base/langflow/services/deps.pysrc/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/base/langflow/__main__.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.pysrc/backend/base/langflow/api/v1/knowledge_bases.py
src/backend/tests/unit/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
Test component integration within flows using create_flow, build_flow, and get_build_events utilities
Files:
src/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
src/backend/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
src/backend/tests/**/*.py: Unit tests for backend code must be located in the 'src/backend/tests/' directory, with component tests organized by component subdirectory under 'src/backend/tests/unit/components/'.
Test files should use the same filename as the component under test, with an appropriate test prefix or suffix (e.g., 'my_component.py' → 'test_my_component.py').
Use the 'client' fixture (an async httpx.AsyncClient) for API tests in backend Python tests, as defined in 'src/backend/tests/conftest.py'.
When writing component tests, inherit from the appropriate base class in 'src/backend/tests/base.py' (ComponentTestBase, ComponentTestBaseWithClient, or ComponentTestBaseWithoutClient) and provide the required fixtures: 'component_class', 'default_kwargs', and 'file_names_mapping'.
Each test in backend Python test files should have a clear docstring explaining its purpose, and complex setups or mocks should be well-commented.
Test both sync and async code paths in backend Python tests, using '@pytest.mark.asyncio' for async tests.
Mock external dependencies appropriately in backend Python tests to isolate unit tests from external services.
Test error handling and edge cases in backend Python tests, including using 'pytest.raises' and asserting error messages.
Validate input/output behavior and test component initialization and configuration in backend Python tests.
Use the 'no_blockbuster' pytest marker to skip the blockbuster plugin in tests when necessary.
Be aware of ContextVar propagation in async tests; test both direct event loop execution and 'asyncio.to_thread' scenarios to ensure proper context isolation.
Test error handling by mocking internal functions using monkeypatch in backend Python tests.
Test resource cleanup in backend Python tests by using fixtures that ensure proper initialization and cleanup of resources.
Test timeout and performance constraints in backend Python tests using 'asyncio.wait_for' and timing assertions.
Test Langflow's Messag...
Files:
src/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
**/{test_*.py,*.test.ts,*.test.tsx}
📄 CodeRabbit inference engine (coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt)
**/{test_*.py,*.test.ts,*.test.tsx}: Review test files for excessive use of mocks that obscure what's actually being tested
Warn when mocks replace testing of real behavior and interactions
Suggest using real objects or simpler test doubles when mocks become excessive
Ensure mocks are reserved for external dependencies, not core logic
Recommend integration tests when unit tests are overly mocked
Test files should have descriptive test function names that explain what is being tested
Organize tests logically with proper setup and teardown
Include edge cases and error conditions for comprehensive coverage
Verify tests cover both positive and negative scenarios where appropriate
Tests should cover the main functionality being implemented
Tests should not be mere smoke tests; they must assert and validate behavior
Files:
src/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
**/{test_*.py,*.test.ts}
📄 CodeRabbit inference engine (coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt)
Check that test files follow naming conventions: backend test_*.py, frontend *.test.ts
Files:
src/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
**/test_*.py
📄 CodeRabbit inference engine (coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt)
**/test_*.py: Backend tests must be named test_*.py and use proper pytest structure
For async Python code, ensure proper async testing patterns with pytest (e.g., async fixtures, event loop)
Backend tests should follow project patterns: use pytest for test execution and structure
For API endpoints, verify tests cover both success and error responses
Files:
src/backend/tests/unit/test_settings_initialization_order.pysrc/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
src/backend/tests/unit/components/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/backend_development.mdc)
src/backend/tests/unit/components/**/*.py: Mirror the component directory structure for unit tests in src/backend/tests/unit/components/
Use ComponentTestBaseWithClient or ComponentTestBaseWithoutClient as base classes for component unit tests
Provide file_names_mapping for backward compatibility in component tests
Create comprehensive unit tests for all new components
Files:
src/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
src/backend/**/components/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/icons.mdc)
In your Python component class, set the
iconattribute to a string matching the frontend icon mapping exactly (case-sensitive).
Files:
src/backend/tests/unit/components/knowledge_bases/test_retrieval.pysrc/backend/tests/unit/components/knowledge_bases/test_ingestion.py
🧬 Code graph analysis (7)
src/lfx/src/lfx/components/knowledge_bases/retrieval.py (2)
src/lfx/src/lfx/components/knowledge_bases/ingestion.py (1)
_get_knowledge_bases_root_path(54-64)src/backend/base/langflow/services/deps.py (1)
get_settings_service(122-135)
src/backend/base/langflow/services/deps.py (1)
src/backend/base/langflow/services/schema.py (1)
ServiceType(4-22)
src/lfx/src/lfx/components/knowledge_bases/ingestion.py (2)
src/lfx/src/lfx/components/knowledge_bases/retrieval.py (1)
_get_knowledge_bases_root_path(22-32)src/backend/base/langflow/services/deps.py (1)
get_settings_service(122-135)
src/backend/tests/unit/test_settings_initialization_order.py (3)
src/backend/base/langflow/services/deps.py (2)
is_settings_service_initialized(111-119)get_settings_service(122-135)src/backend/base/langflow/services/schema.py (1)
ServiceType(4-22)src/backend/base/langflow/__main__.py (1)
run(184-416)
src/lfx/src/lfx/base/mcp/util.py (1)
src/backend/base/langflow/services/deps.py (1)
get_settings_service(122-135)
src/backend/base/langflow/__main__.py (1)
src/backend/base/langflow/services/deps.py (1)
is_settings_service_initialized(111-119)
src/backend/base/langflow/api/v1/knowledge_bases.py (1)
src/backend/base/langflow/services/deps.py (1)
get_settings_service(122-135)
🔇 Additional comments (10)
src/lfx/src/lfx/graph/edge/base.py (1)
66-67: LGTM! Appropriate type suppressions for legacy code path.The
type: ignore[assignment]comments correctly suppress type errors when assigningNoneto handle attributes in the legacy string-based handle path. This maintains backward compatibility while being explicit about the intentional typing violation.src/backend/base/langflow/api/v1/knowledge_bases.py (2)
50-52: LGTM! Clean delegation to lazy loader.The refactor of
get_kb_root_path()to delegate to_get_knowledge_bases_dir()successfully defers initialization until first use while maintaining the same public interface.
18-31: Thread-safety lock unnecessary for this lazy loader._get_knowledge_bases_dirruns wholly before yielding control (noawait), so calls are atomic under the single-threaded event loop—no additional locking needed.Likely an incorrect or invalid review comment.
src/lfx/src/lfx/base/mcp/util.py (1)
50-62: LGTM! Clean lazy loading accessors for MCP settings.The getter functions (
get_max_sessions_per_server(),get_session_idle_timeout(),get_session_cleanup_interval()) provide a clean interface to the cached MCP settings, successfully deferring settings service access until first use.src/lfx/src/lfx/components/knowledge_bases/retrieval.py (2)
97-103: LGTM! Proper lazy loading in build config updates.The update to
update_build_configcorrectly uses the lazy loader, ensuring the knowledge bases directory is only resolved when actually needed during configuration updates.
181-197: LGTM! Proper lazy loading in data retrieval.The update to
retrieve_datacorrectly uses the lazy loader to construct the knowledge base path, deferring directory resolution until the actual retrieval operation.src/backend/tests/unit/components/knowledge_bases/test_ingestion.py (1)
19-24: LGTM! Test correctly updated for internal symbol.The mock target correctly updated from the public
KNOWLEDGE_BASES_ROOT_PATHto the internal_KNOWLEDGE_BASES_ROOT_PATHsymbol, maintaining test coverage for the lazy loading implementation.src/backend/tests/unit/components/knowledge_bases/test_retrieval.py (1)
20-25: LGTM! Test correctly updated for internal symbol.The mock target correctly updated to match the internal
_KNOWLEDGE_BASES_ROOT_PATHsymbol used in the retrieval component's lazy loading implementation.src/backend/base/langflow/__main__.py (1)
271-279: LGTM! Effective guard against env loading race conditions.The guard successfully prevents loading the env file after settings service initialization, which would cause race conditions. The error message clearly explains the issue and helps developers understand the initialization order requirement.
src/backend/base/langflow/services/deps.py (1)
111-119: LGTM! Clean non-blocking initialization check.The
is_settings_service_initialized()function provides a clean way to check initialization status without triggering initialization. The implementation correctly inspects the service registry directly and includes a clear docstring.
…/langflow into cherry-pick-pr-10142
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
…/langflow into cherry-pick-pr-10142
github-actions
Bot
added
bug
github-actions
Bot
added
bug
HimavarshaVS
added
lgtm
github-actions
Bot
added
bug
|
✅ Component index has been automatically updated due to changes in New Index: |
|
HimavarshaVS
added
lgtm
github-actions
Bot
added
bug
github-actions
Bot
added
bug
github-actions
Bot
added
bug
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
3 participants
This PR brings in the changes from #10142 to:
Ensure settings services are lazy loaded; fixes settings init order
Remove dupe tests
remove useless test
[autofix.ci] apply automated fixes
Ruff
[autofix.ci] apply automated fixes
mypy
Fix test reference
ruff fixes
[autofix.ci] apply automated fixes
[autofix.ci] apply automated fixes (attempt 2/3)
ruff + starter projects
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests