fix(chat): surface trigger_task dispatch failure + test env isolation - #17
Conversation
- /chat/confirm trigger_task: wrap dispatch_collection in try/except; dispatch failure after task commit now returns 502 with task_id instead of swallowing the error and reporting applied=true (F841 result unused) - success response now includes executor dispatch info - tests/conftest.py: clear API_AUTH_TOKEN/AGENT_API_TOKEN before backend import — deployment .env with fleet token was failing 19 unit tests (401) - add dispatch-failure regression test Triage ledger: docs/BUG-TRIAGE-20260718.md
|
✅ Health: 8.2 📋 At a glance 🚨 Change risk: 7.9/10 (high)
📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-18 11:12 UTC |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe chat confirmation endpoint now surfaces task dispatch failures as HTTP 502 errors, successful responses include dispatch results, integration tests cover the failure path, test authentication variables are cleared, and a dated triage report documents related findings. ChangesChat dispatch and test reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses several backend issues identified during bug triage. Specifically, it wraps the task dispatch call in backend/api/v1/chat.py with error handling to surface dispatch failures as 502 errors, adds a triage documentation file, isolates test environments from local .env configurations in tests/conftest.py, and introduces an integration test for dispatch failures. The review feedback recommends updating the task status to 'failed' in the database upon dispatch failure to prevent tasks from being permanently stuck in a 'pending' state, and suggests adding a corresponding assertion in the new integration test.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| except Exception as exc: | ||
| # Task row is already committed; surface the dispatch failure instead | ||
| # of reporting applied=True with a silently dead task. | ||
| logger.exception("chat confirm | trigger_task dispatch failed source=%s task=%s", source.id, task.id) | ||
| raise HTTPException( | ||
| status_code=502, detail=f"任务已创建但派发失败 (task_id={task.id}), 请到工作项里重试" | ||
| ) from exc |
There was a problem hiding this comment.
当任务派发失败时，虽然 API 抛出了 502 异常，但由于 task 记录在前面已经通过 db.commit() 提交到了数据库，此时该任务在数据库中的状态仍然会保持为 pending。这会导致该任务在数据库和前端 UI 中永久处于“等待中”状态，给用户带来困惑。
建议在捕获到异常时，将 task.status 更新为 "failed" 并再次提交，以便准确反映任务的实际状态。
except Exception as exc:
# Task row is already committed; surface the dispatch failure instead
# of reporting applied=True with a silently dead task.
logger.exception("chat confirm | trigger_task dispatch failed source=%s task=%s", source.id, task.id)
task.status = "failed"
await db.commit()
raise HTTPException(
status_code=502, detail=f"任务已创建但派发失败 (task_id={task.id}), 请到工作项里重试"
) from exc| assert response.status_code == 502 | ||
| assert "派发失败" in response.json()["detail"] |
There was a problem hiding this comment.
为了确保在派发失败时，数据库中的任务状态确实被正确更新为了 "failed"，建议在测试中增加对任务状态的断言。
| assert response.status_code == 502 | |
| assert "派发失败" in response.json()["detail"] | |
| assert response.status_code == 502 | |
| assert "派发失败" in response.json()["detail"] | |
| from backend.models.task import CollectionTask | |
| from sqlalchemy import select | |
| stmt = select(CollectionTask).where(CollectionTask.source_id == source.id) | |
| db_task = (await db_session.execute(stmt)).scalar_one() | |
| assert db_task.status == "failed" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/v1/chat.py`:
- Around line 437-438: The executor factory currently loses LocalExecutor state
and dispatch_collection does not retain spawned tasks. Cache get_executor so
repeated calls return the same LocalExecutor instance, and update
LocalExecutor.dispatch_collection to store each created asyncio task in an
instance-level tracking dictionary, removing it when it completes while
preserving existing dispatch and cancellation behavior.
In `@docs/BUG-TRIAGE-20260718.md`:
- Around line 26-28: Update the triage entry for chat confirm trigger_task to
mark the dispatch issue resolved and document the shipped behavior:
dispatch_collection failures are caught, the API returns HTTP 502 with the
committed task_id, and the prior unchecked-result/applied:true behavior no
longer applies.
- Around line 32-35: Update the “单测无 .env 隔离” finding and remediation text to
document both authentication variables, API_AUTH_TOKEN and AGENT_API_TOKEN.
State that the conftest autouse fixture clears both variables before backend
imports, while preserving the existing test-isolation context.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1c21d8e-b00f-4129-b0f6-8238e1970800
📒 Files selected for processing (4)
backend/api/v1/chat.pydocs/BUG-TRIAGE-20260718.mdtests/conftest.pytests/integration/test_chat_api.py
| try: | ||
| dispatch = await get_executor().dispatch_collection(task.id, {}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
State loss and GC hazards in LocalExecutor.
While reviewing the get_executor().dispatch_collection() invocation, I noticed two critical concurrency and state issues in the LocalExecutor implementation provided in the context snippets:
- State loss in
get_executor:get_executor()returns a new instance ofLocalExecutoron every call. BecauseLocalExecutorrelies on an instance-level_acquisition_tasksdictionary to track tasks, returning a new instance each time means previously dispatched tasks cannot be found or canceled later viacancel_acquisition. - Task garbage collection hazard:
LocalExecutor.dispatch_collectionfires and forgets the collection task (t = asyncio.create_task(...)) without keeping a strong reference. The Python event loop only keeps weak references to tasks; this means active collection pipelines can be silently garbage-collected mid-execution when they yield to I/O.
Please consider applying @functools.lru_cache to get_executor in backend/executor/__init__.py to preserve the executor state, and updating LocalExecutor.dispatch_collection to track its spawned tasks in an instance dictionary (similar to _acquisition_tasks) to prevent them from vanishing.
🤖 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/api/v1/chat.py` around lines 437 - 438, The executor factory
currently loses LocalExecutor state and dispatch_collection does not retain
spawned tasks. Cache get_executor so repeated calls return the same
LocalExecutor instance, and update LocalExecutor.dispatch_collection to store
each created asyncio task in an instance-level tracking dictionary, removing it
when it completes while preserving existing dispatch and cancellation behavior.
| ### 3. chat confirm `trigger_task` 吞掉派发失败 | ||
| - `backend/api/v1/chat.py:437`: `result = await get_executor().dispatch_collection(task.id, {})` 结果未检查, 派发炸了 API 仍回 `applied: true` | ||
| - 修法: 检查 result / try-except 回 `applied: false` + 原因 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark the dispatch issue as resolved.
This entry still documents the pre-fix behavior (result unchecked and applied: true). The current implementation catches dispatch_collection failures and returns HTTP 502 with the committed task_id; update the triage record to reflect the shipped contract.
🤖 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 `@docs/BUG-TRIAGE-20260718.md` around lines 26 - 28, Update the triage entry
for chat confirm trigger_task to mark the dispatch issue resolved and document
the shipped behavior: dispatch_collection failures are caught, the API returns
HTTP 502 with the committed task_id, and the prior unchecked-result/applied:true
behavior no longer applies.
| ### 4. 单测无 .env 隔离 | ||
| - 根 `.env` 配了 `API_AUTH_TOKEN` (部署必需) 时, `tests/unit` 挂 19 个 (workers/nodes_install/geo 全是 401 或 token 注入断言) | ||
| - pydantic Settings 直读仓库根 `.env`; conftest 未清空鉴权相关 env | ||
| - 修法: conftest autouse fixture 强制 `API_AUTH_TOKEN=''` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document both authentication variables in test isolation.
The remediation currently names only API_AUTH_TOKEN, but this PR also clears AGENT_API_TOKEN before backend import. Update the finding and fix description to cover both variables.
🤖 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 `@docs/BUG-TRIAGE-20260718.md` around lines 32 - 35, Update the “单测无 .env 隔离”
finding and remediation text to document both authentication variables,
API_AUTH_TOKEN and AGENT_API_TOKEN. State that the conftest autouse fixture
clears both variables before backend imports, while preserving the existing
test-isolation context.
1 participant
What
Test