Repository navigation
fix(knowledge-runtime): shorten db session boundary - #1569
Merged
qdaxb merged 1 commit intoJun 30, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughFastAPI admin, index, and query endpoints no longer inject database sessions. A new ChangesRuntime config resolution refactor
Sequence Diagram(s)sequenceDiagram
participant QueryExecutor
participant RuntimeConfigLoader
participant ConfigResolver
participant KnowledgeQueryExecutor
QueryExecutor->>RuntimeConfigLoader: resolve_query_configs(knowledge_base_ids, user_id)
RuntimeConfigLoader->>ConfigResolver: resolve_query_configs(session, knowledge_base_ids, user_id)
ConfigResolver-->>RuntimeConfigLoader: configs_by_kb_id
RuntimeConfigLoader-->>QueryExecutor: configs_by_kb_id
QueryExecutor->>KnowledgeQueryExecutor: _query_knowledge_base(config, request)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
knowledge_runtime/knowledge_runtime/services/config_loader.py (1)
39-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tracing to the new config-resolution entrypoints.
These new sync service methods are the boundary this PR introduces, but none of them use
@trace_syncor attach span details. That makes the shortened DB-session behavior invisible in telemetry and misses a repo rule for changed Python code. Please instrument the publicresolve_*methods and record identifiers likeknowledge_base_id/ batch size on the span. Based on coding guidelines: "Use@trace_asyncor@trace_syncdecorators from shared.telemetry.decorators for tracing async and sync functions respectively; use add_span_event() and set_span_attribute() for adding details to spans."🤖 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 `@knowledge_runtime/knowledge_runtime/services/config_loader.py` around lines 39 - 100, The new public config-resolution methods in ConfigLoader are missing required telemetry, so add `@trace_sync` from shared.telemetry.decorators to each sync resolve_* entrypoint and attach span attributes/events inside them. Instrument resolve_index_config, resolve_query_config, resolve_query_configs, and resolve_admin_config so the trace captures identifiers like knowledge_base_id, user_id, document_id, and the batch size for resolve_query_configs, using set_span_attribute() and add_span_event() where appropriate.Source: Coding guidelines
knowledge_runtime/tests/test_admin_executor.py (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
resolve_admin_configis called with the request KB ID.These tests now stub
resolve_admin_config.return_value, so they would still pass if the executor resolved a hard-coded or wrongknowledge_base_id. Addassert_called_once_with(knowledge_base_id=1)after each executor call to lock down the new loader boundary.Also applies to: 101-107, 133-139, 181-187, 213-219, 243-249
🤖 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 `@knowledge_runtime/tests/test_admin_executor.py` around lines 65 - 71, The admin executor tests are only stubbing resolve_admin_config and do not verify it is invoked with the request knowledge base ID. After each delete_document_index call in test_admin_executor, add an assertion on admin_executor._config_loader.resolve_admin_config to check it was called once with knowledge_base_id=1, using the same pattern in the other affected test cases.knowledge_runtime/knowledge_runtime/services/index_executor.py (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale docstring reference.
The class docstring still says configs are resolved "using
ConfigResolver", but resolution now flows throughRuntimeConfigLoader. Minor doc drift.📝 Suggested docstring tweak
- 1. Resolves configs from the database using ConfigResolver + 1. Resolves configs from the database using RuntimeConfigLoader🤖 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 `@knowledge_runtime/knowledge_runtime/services/index_executor.py` around lines 27 - 28, The class docstring for KnowledgeRuntimeExecutor is stale: it still references ConfigResolver even though config resolution now happens through RuntimeConfigLoader. Update the docstring text in KnowledgeRuntimeExecutor to describe the current runtime config loading flow and remove the outdated ConfigResolver mention so the documentation matches the implementation.
🤖 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.
Nitpick comments:
In `@knowledge_runtime/knowledge_runtime/services/config_loader.py`:
- Around line 39-100: The new public config-resolution methods in ConfigLoader
are missing required telemetry, so add `@trace_sync` from
shared.telemetry.decorators to each sync resolve_* entrypoint and attach span
attributes/events inside them. Instrument resolve_index_config,
resolve_query_config, resolve_query_configs, and resolve_admin_config so the
trace captures identifiers like knowledge_base_id, user_id, document_id, and the
batch size for resolve_query_configs, using set_span_attribute() and
add_span_event() where appropriate.
In `@knowledge_runtime/knowledge_runtime/services/index_executor.py`:
- Around line 27-28: The class docstring for KnowledgeRuntimeExecutor is stale:
it still references ConfigResolver even though config resolution now happens
through RuntimeConfigLoader. Update the docstring text in
KnowledgeRuntimeExecutor to describe the current runtime config loading flow and
remove the outdated ConfigResolver mention so the documentation matches the
implementation.
In `@knowledge_runtime/tests/test_admin_executor.py`:
- Around line 65-71: The admin executor tests are only stubbing
resolve_admin_config and do not verify it is invoked with the request knowledge
base ID. After each delete_document_index call in test_admin_executor, add an
assertion on admin_executor._config_loader.resolve_admin_config to check it was
called once with knowledge_base_id=1, using the same pattern in the other
affected test cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13ca1404-4ab6-4401-8f2a-f6f374899b23
📒 Files selected for processing (11)
knowledge_runtime/knowledge_runtime/api/endpoints/admin.pyknowledge_runtime/knowledge_runtime/api/endpoints/index.pyknowledge_runtime/knowledge_runtime/api/endpoints/query.pyknowledge_runtime/knowledge_runtime/services/admin_executor.pyknowledge_runtime/knowledge_runtime/services/config_loader.pyknowledge_runtime/knowledge_runtime/services/index_executor.pyknowledge_runtime/knowledge_runtime/services/query_executor.pyknowledge_runtime/tests/test_admin_executor.pyknowledge_runtime/tests/test_config_loader.pyknowledge_runtime/tests/test_index_executor.pyknowledge_runtime/tests/test_query_executor.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
RuntimeConfigLoaderto end DB transactions immediately after runtime config resolutionVerification
uv run pytestinknowledge_runtime(109 passed, third-party Pydantic deprecation warning only)uv run black --check knowledge_runtime testsuv run isort --check-only knowledge_runtime testsuv run flake8 --max-line-length=88 ...on changed knowledge_runtime source filesCI=true AI_VERIFIED=1 git pushSummary by CodeRabbit