Repository navigation
Wegent db session lifetime fixes - #1330
Merged
feifei325 merged 6 commits intoJun 10, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors many services to use optional/owned DB sessions and short-lived session factories so sessions are created and closed around preparation/record work and closed before downstream async/gateway calls; matching tests and e2e fixture provisioning were added/updated. ChangesSession Lifecycle Decoupling and Short-Session Pattern Rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/app/services/background_chat_executor.py (1)
387-391: 💤 Low valueConsider logging swallowed exceptions for observability.
The
try-except-passpattern is intentional since errors are handled via the emitter, but a debug log would aid troubleshooting without changing behavior.🔧 Suggested improvement
try: await dispatch_task - except Exception: - pass + except Exception: + logger.debug( + "[BackgroundChatExecutor] dispatch_task exception (handled via emitter)", + exc_info=True, + ) return accumulated_content🤖 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 `@backend/app/services/background_chat_executor.py` around lines 387 - 391, The except block that currently swallows exceptions when awaiting dispatch_task should log the caught exception for observability without changing behavior; update the except Exception: clause in background_chat_executor.py (the block awaiting dispatch_task) to call the existing logger (or add one) and emit a debug/exception-level log including the exception details and context (e.g., mention dispatch_task and accumulated_content or the related emitter) before returning accumulated_content so the runtime behavior is unchanged but errors are visible in logs.Source: Linters/SAST tools
backend/app/services/repository_job.py (2)
211-277: 💤 Low valueOutdated docstrings reference
Userinstead ofRepositoryUserSnapshot.The docstrings in helper methods still say "User object" but now accept
RepositoryUserSnapshot.Suggested fix for all four methods
async def _update_github_repositories( self, user: RepositoryUserSnapshot, git_token: str, git_domain: str ) -> None: """ Update GitHub repositories cache for a user Args: - user: User object + user: User snapshot git_token: GitHub token git_domain: GitHub domain """Apply similarly to
_update_gitlab_repositories,_update_gitee_repositories, and_update_gitea_repositories.🤖 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 `@backend/app/services/repository_job.py` around lines 211 - 277, Docstrings for _update_github_repositories, _update_gitlab_repositories, _update_gitee_repositories, and _update_gitea_repositories incorrectly refer to the parameter as "User object"; update each method's docstring to reference RepositoryUserSnapshot (e.g., change "User object" to "RepositoryUserSnapshot") and optionally add a brief description matching the type, ensuring the Args section lists user: RepositoryUserSnapshot for _update_github_repositories, _update_gitlab_repositories, _update_gitee_repositories, and _update_gitea_repositories.
75-75: ⚡ Quick winRemove extraneous f-string prefix.
The f-string has no placeholders.
Suggested fix
- logger.info(f"[repository_job] Starting get all users task") + logger.info("[repository_job] Starting get all users task")🤖 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 `@backend/app/services/repository_job.py` at line 75, The log call uses an unnecessary f-string with no placeholders; replace the f-string in the logger.info call (the line logging "[repository_job] Starting get all users task" inside repository_job / the get-all-users task) with a plain string literal (remove the leading f) so it’s a normal string instead of an f-string.Source: Linters/SAST tools
backend/tests/tasks/test_subscription_tasks_legacy_trigger.py (1)
46-68: 💤 Low valueRemove unused
_RecordingSubscriptionQueryclass.This class is defined but never used in the test file. Only
_RecordingKeysetSubscriptionQueryis used in the pagination tests. Consider removing it to avoid dead code.🤖 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 `@backend/tests/tasks/test_subscription_tasks_legacy_trigger.py` around lines 46 - 68, The file defines an unused test helper class _RecordingSubscriptionQuery which is dead code; remove the entire _RecordingSubscriptionQuery class definition (its __init__, filter, order_by, limit, offset, all methods) from the test file so only the used helper _RecordingKeysetSubscriptionQuery remains, and run tests to ensure no references to _RecordingSubscriptionQuery exist before committing.
🤖 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 `@backend/app/services/knowledge/indexing.py`:
- Around line 222-237: The helper function _prepare_indexing_runtime is missing
the required OpenTelemetry sync tracing decorator; add from
shared.telemetry.decorators import trace_sync and annotate
_prepare_indexing_runtime with `@trace_sync`(), and likewise apply `@trace_sync`()
to the other new sync helper(s) declared later in this module (the second helper
around the later helper block), ensuring both functions are decorated to create
span-level observability.
---
Nitpick comments:
In `@backend/app/services/background_chat_executor.py`:
- Around line 387-391: The except block that currently swallows exceptions when
awaiting dispatch_task should log the caught exception for observability without
changing behavior; update the except Exception: clause in
background_chat_executor.py (the block awaiting dispatch_task) to call the
existing logger (or add one) and emit a debug/exception-level log including the
exception details and context (e.g., mention dispatch_task and
accumulated_content or the related emitter) before returning accumulated_content
so the runtime behavior is unchanged but errors are visible in logs.
In `@backend/app/services/repository_job.py`:
- Around line 211-277: Docstrings for _update_github_repositories,
_update_gitlab_repositories, _update_gitee_repositories, and
_update_gitea_repositories incorrectly refer to the parameter as "User object";
update each method's docstring to reference RepositoryUserSnapshot (e.g., change
"User object" to "RepositoryUserSnapshot") and optionally add a brief
description matching the type, ensuring the Args section lists user:
RepositoryUserSnapshot for _update_github_repositories,
_update_gitlab_repositories, _update_gitee_repositories, and
_update_gitea_repositories.
- Line 75: The log call uses an unnecessary f-string with no placeholders;
replace the f-string in the logger.info call (the line logging "[repository_job]
Starting get all users task" inside repository_job / the get-all-users task)
with a plain string literal (remove the leading f) so it’s a normal string
instead of an f-string.
In `@backend/tests/tasks/test_subscription_tasks_legacy_trigger.py`:
- Around line 46-68: The file defines an unused test helper class
_RecordingSubscriptionQuery which is dead code; remove the entire
_RecordingSubscriptionQuery class definition (its __init__, filter, order_by,
limit, offset, all methods) from the test file so only the used helper
_RecordingKeysetSubscriptionQuery remains, and run tests to ensure no references
to _RecordingSubscriptionQuery exist before committing.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5faefbc-4404-48b2-a9e7-9d04055de1d9
📒 Files selected for processing (19)
backend/app/services/background_chat_executor.pybackend/app/services/jobs.pybackend/app/services/knowledge/indexing.pybackend/app/services/knowledge/summary_service.pybackend/app/services/rag/gateway.pybackend/app/services/rag/local_data_plane/indexing.pybackend/app/services/rag/local_gateway.pybackend/app/services/rag/remote_gateway.pybackend/app/services/repository_job.pybackend/app/tasks/subscription_tasks.pybackend/tests/services/knowledge/test_indexing.pybackend/tests/services/rag/test_local_data_plane_indexing.pybackend/tests/services/rag/test_local_gateway.pybackend/tests/services/rag/test_remote_gateway.pybackend/tests/services/test_background_chat_executor.pybackend/tests/services/test_repository_job.pybackend/tests/services/test_summary_service.pybackend/tests/tasks/test_knowledge_tasks.pybackend/tests/tasks/test_subscription_tasks_legacy_trigger.py
Comment on lines
+222
to
+237
| def _prepare_indexing_runtime( | ||
| *, | ||
| db: Session, | ||
| knowledge_base_id: str, | ||
| attachment_id: int, | ||
| retriever_name: str, | ||
| retriever_namespace: str, | ||
| embedding_model_name: str, | ||
| embedding_model_namespace: str, | ||
| user_id: int, | ||
| user_name: str, | ||
| splitter_config: Optional[SplitterConfig], | ||
| splitter_config_dict: Optional[dict], | ||
| document_id: Optional[int], | ||
| kb_index_info: Optional[KnowledgeBaseIndexInfo], | ||
| ) -> _IndexingPreparation: |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add tracing decorators to the new sync helper boundaries.
Both new helpers are execution boundaries but currently miss @trace_sync(), which violates the repository tracing requirement and reduces span-level observability consistency.
Suggested patch
from shared.telemetry import add_span_event
+from shared.telemetry.decorators import trace_sync
@@
+@trace_sync()
def _prepare_indexing_runtime(
@@
+@trace_sync()
def _run_indexing_gateway_calls(As per coding guidelines: “Use OpenTelemetry tracing decorators from shared/telemetry/decorators.py: @trace_async() or @trace_sync() for functions, add_span_event() for events, set_span_attribute() for span attributes”.
Also applies to: 374-383
🤖 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 `@backend/app/services/knowledge/indexing.py` around lines 222 - 237, The
helper function _prepare_indexing_runtime is missing the required OpenTelemetry
sync tracing decorator; add from shared.telemetry.decorators import trace_sync
and annotate _prepare_indexing_runtime with `@trace_sync`(), and likewise apply
`@trace_sync`() to the other new sync helper(s) declared later in this module (the
second helper around the later helper block), ensuring both functions are
decorated to create span-level observability.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/services/background_chat_executor.py (2)
79-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required docstring and tracing on the managed-session entrypoint.
with_managed_sessions()is a new public API with no docstring, and this new managed path still lacks the shared tracing decorators/span metadata the repo requires for Python services. As per coding guidelines, "Python code MUST include descriptive names and docstrings for public functions/classes" and "Use OpenTelemetry tracing decorators from shared/telemetry/decorators.py:@trace_async() or@trace_sync() for functions, add_span_event() for events, set_span_attribute() for span attributes."Also applies to: 113-119
🤖 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 `@backend/app/services/background_chat_executor.py` around lines 79 - 86, Add a docstring to the public classmethod with_managed_sessions describing its purpose and parameters, and add OpenTelemetry tracing: import trace_sync and set_span_attribute from shared.telemetry.decorators and annotate the method with `@trace_sync`(); inside the method set span attributes for "managed_sessions"=True and "user_id" (use set_span_attribute) so the managed-session entrypoint emits tracing metadata. Apply the same pattern (docstring + `@trace_sync`() + span attributes) to the other managed-session factory method referenced around lines 113-119.Source: Coding guidelines
67-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject mixed session ownership.
If an instance is created with both
dbandsession_factory,execute()silently takes the managed-session branch and ignoresself.db. That can move task writes outside the caller's transaction/session boundary with no signal to the caller.Suggested guard
def __init__( self, db: Session | None, user_id: int, *, session_factory: Callable[[], Session] | None = None, ): + if (db is None) == (session_factory is None): + raise ValueError("Provide exactly one of `db` or `session_factory`.") self.db = db self.user_id = user_id self.session_factory = session_factoryAlso applies to: 113-119
🤖 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 `@backend/app/services/background_chat_executor.py` around lines 67 - 76, The constructor for BackgroundChatExecutor allows both db and session_factory to be provided, but execute() silently prefers session_factory and ignores self.db, which can leak writes outside the caller's transaction; change the __init__ (and any other ctor/factory like the one referenced at lines 113-119) to validate ownership: if both db and session_factory are supplied, raise a ValueError (or similar) with a clear message refusing mixed session ownership; ensure execute() continues to branch on session_factory vs db but now can assume only one is set.
🤖 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.
Outside diff comments:
In `@backend/app/services/background_chat_executor.py`:
- Around line 79-86: Add a docstring to the public classmethod
with_managed_sessions describing its purpose and parameters, and add
OpenTelemetry tracing: import trace_sync and set_span_attribute from
shared.telemetry.decorators and annotate the method with `@trace_sync`(); inside
the method set span attributes for "managed_sessions"=True and "user_id" (use
set_span_attribute) so the managed-session entrypoint emits tracing metadata.
Apply the same pattern (docstring + `@trace_sync`() + span attributes) to the
other managed-session factory method referenced around lines 113-119.
- Around line 67-76: The constructor for BackgroundChatExecutor allows both db
and session_factory to be provided, but execute() silently prefers
session_factory and ignores self.db, which can leak writes outside the caller's
transaction; change the __init__ (and any other ctor/factory like the one
referenced at lines 113-119) to validate ownership: if both db and
session_factory are supplied, raise a ValueError (or similar) with a clear
message refusing mixed session ownership; ensure execute() continues to branch
on session_factory vs db but now can assume only one is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef18ccae-9457-4e1e-b7d9-f9feef03e5df
📒 Files selected for processing (4)
backend/app/services/background_chat_executor.pybackend/app/services/knowledge/summary_service.pybackend/tests/services/test_background_chat_executor.pybackend/tests/services/test_summary_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/services/test_background_chat_executor.py
- backend/tests/services/test_summary_service.py
- backend/app/services/knowledge/summary_service.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/e2e/tests/code-task.spec.ts (2)
58-65: 💤 Low valueConsider logging cleanup failures for debugging.
The afterAll hook silently swallows deletion errors (lines 60, 63), which hides resource cleanup issues. While best-effort cleanup is reasonable for e2e tests, logging failures would help identify leaked resources or permission issues during test debugging.
♻️ Optional improvement
test.afterAll(async () => { if (codeTeam) { - await apiClient.deleteTeam(codeTeam.name).catch(() => {}) + await apiClient.deleteTeam(codeTeam.name).catch((err) => { + console.warn(`Failed to delete team ${codeTeam.name}:`, err.message) + }) } if (codeBot) { - await apiClient.deleteBot(codeBot.name).catch(() => {}) + await apiClient.deleteBot(codeBot.name).catch((err) => { + console.warn(`Failed to delete bot ${codeBot.name}:`, err.message) + }) } })🤖 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 `@frontend/e2e/tests/code-task.spec.ts` around lines 58 - 65, The afterAll cleanup silently swallows errors when deleting resources; modify the test.afterAll block to catch and log failures instead of ignoring them by capturing the error from apiClient.deleteTeam and apiClient.deleteBot (referencing codeTeam, codeBot, and apiClient.deleteTeam/deleteBot) and calling a logger or console.error with a descriptive message that includes the resource name and the error so cleanup failures are visible during e2e debugging.
26-35: ⚡ Quick winStrengthen API response typing for bot and team creation.
The inline type assertions on lines 34 and 54 (
botResponse.data as { id?: number } | null) cast without validating the response structure. If the API contract changes or returns an unexpected shape, the cast succeeds butidextraction fails silently, producing unclear assertion failures. Consider defining a proper response interface or adding validation before extraction.♻️ Suggested improvement
Define typed response interfaces:
interface BotCreateResponse { id: number name: string // ... other fields } interface TeamCreateResponse { id: number name: string // ... other fields }Then update the extraction logic:
const botResponse = await apiClient.createBot({...}) expect([200, 201]).toContain(botResponse.status) - codeBot = { name: botName, id: (botResponse.data as { id?: number } | null)?.id } + const botData = botResponse.data as BotCreateResponse + codeBot = { name: botName, id: botData.id } expect(codeBot.id).toBeTruthy()Apply the same pattern for team creation on lines 53-55.
Also applies to: 37-55
🤖 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 `@frontend/e2e/tests/code-task.spec.ts` around lines 26 - 35, The code casts API results directly (botResponse.data, teamResponse.data) which can hide shape mismatches; define proper response interfaces (e.g., BotCreateResponse with id: number, name: string and TeamCreateResponse with id: number, name: string), update the creation calls to use those types, and add a small runtime validation/type-guard that checks the presence and type of id on botResponse.data and teamResponse.data before assigning to codeBot.id or team.id so extraction fails with a clear error if the shape is wrong; locate and update the references to botResponse, codeBot, teamResponse, and team in the test to use the new interfaces and guard.
🤖 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 `@frontend/e2e/tests/code-task.spec.ts`:
- Line 16: codePagePath currently interpolates codeTeam?.id which can produce
the literal "teamId=undefined"; update codePagePath to defensively handle
missing id by checking codeTeam?.id and either (a) return '/code' with no teamId
query when id is falsy, or (b) throw a clear Error/Assertion if an id is
required—choose one policy and implement consistently. Locate and change the
codePagePath function (and any callers in the test) to use this explicit check
so the URL cannot contain "teamId=undefined".
---
Nitpick comments:
In `@frontend/e2e/tests/code-task.spec.ts`:
- Around line 58-65: The afterAll cleanup silently swallows errors when deleting
resources; modify the test.afterAll block to catch and log failures instead of
ignoring them by capturing the error from apiClient.deleteTeam and
apiClient.deleteBot (referencing codeTeam, codeBot, and
apiClient.deleteTeam/deleteBot) and calling a logger or console.error with a
descriptive message that includes the resource name and the error so cleanup
failures are visible during e2e debugging.
- Around line 26-35: The code casts API results directly (botResponse.data,
teamResponse.data) which can hide shape mismatches; define proper response
interfaces (e.g., BotCreateResponse with id: number, name: string and
TeamCreateResponse with id: number, name: string), update the creation calls to
use those types, and add a small runtime validation/type-guard that checks the
presence and type of id on botResponse.data and teamResponse.data before
assigning to codeBot.id or team.id so extraction fails with a clear error if the
shape is wrong; locate and update the references to botResponse, codeBot,
teamResponse, and team in the test to use the new interfaces and guard.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c49edbae-09fa-410c-b7f2-73c178bc8b5c
📒 Files selected for processing (2)
backend/tests/integration/test_rag_remote_mode.pyfrontend/e2e/tests/code-task.spec.ts
| let codeBot: CreatedResource | null = null | ||
| let codeTeam: CreatedResource | null = null | ||
|
|
||
| const codePagePath = () => `/code?teamId=${codeTeam?.id}` |
Contributor
There was a problem hiding this comment.
Add defensive check in codePagePath() to prevent malformed URLs.
The function uses optional chaining on codeTeam?.id, which produces /code?teamId=undefined (a string literal, not absent parameter) if codeTeam is null or the id is undefined. While the beforeAll assertions on lines 35 and 55 should prevent this at runtime, the type system doesn't enforce it, and the URL would break silently if assertions are bypassed or removed during refactoring.
🛡️ Proposed fix
- const codePagePath = () => `/code?teamId=${codeTeam?.id}`
+ const codePagePath = () => {
+ if (!codeTeam?.id) {
+ throw new Error('codeTeam not initialized; ensure beforeAll completed')
+ }
+ return `/code?teamId=${codeTeam.id}`
+ }📝 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.
Suggested change
| const codePagePath = () => `/code?teamId=${codeTeam?.id}` | |
| const codePagePath = () => { | |
| if (!codeTeam?.id) { | |
| throw new Error('codeTeam not initialized; ensure beforeAll completed') | |
| } | |
| return `/code?teamId=${codeTeam.id}` | |
| } |
🤖 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 `@frontend/e2e/tests/code-task.spec.ts` at line 16, codePagePath currently
interpolates codeTeam?.id which can produce the literal "teamId=undefined";
update codePagePath to defensively handle missing id by checking codeTeam?.id
and either (a) return '/code' with no teamId query when id is falsy, or (b)
throw a clear Error/Assertion if an id is required—choose one policy and
implement consistently. Locate and change the codePagePath function (and any
callers in the test) to use this explicit check so the URL cannot contain
"teamId=undefined".
Collaborator
|
LGTM |
sunnights
approved these changes
Jun 10, 2026
This was referenced Jun 25, 2026
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 by CodeRabbit
Bug Fixes
Refactor
Tests