feat(minichat): Persist web_search/code_interpreter completed counts in chat_turns#1418
Open
jalankulkija wants to merge 1 commit intocyberfabric:mainfrom
Open
feat(minichat): Persist web_search/code_interpreter completed counts in chat_turns#1418jalankulkija wants to merge 1 commit intocyberfabric:mainfrom
jalankulkija wants to merge 1 commit intocyberfabric:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPersist per-turn completed tool-call counters (web_search, code_interpreter): DB migration + SeaORM fields, repo enum/method and implementation to atomically increment counters, provider task persists on ToolPhase::Done, counts flow into orphan finalization/usage events, and tests updated. Changes
Sequence Diagram(s)sequenceDiagram
participant ProviderTask as ProviderTask
participant TurnRepo as TurnRepository
participant DB as Database
participant OrphanWD as OrphanWatchdog
participant Finalization as FinalizationService
ProviderTask->>DB: acquire connection (on ToolPhase::Done)
ProviderTask->>TurnRepo: increment_tool_calls(turn_id, ToolCallType::X)
TurnRepo->>DB: UPDATE chat_turns SET <counter> = <counter> + 1 WHERE id = turn_id AND state = 'Running' AND deleted_at IS NULL
DB-->>TurnRepo: success/failure
TurnRepo-->>ProviderTask: result
Note right of ProviderTask: warn! on failure
OrphanWD->>DB: SELECT chat_turns WHERE id = turn_id
DB-->>OrphanWD: row with web_search_completed_count, code_interpreter_completed_count
OrphanWD->>OrphanWD: convert i32 -> u32 (u32::try_from, clamp negatives to 0)
OrphanWD->>Finalization: finalize_orphan_turn(OrphanFinalizationInput{web_search_completed_count, code_interpreter_completed_count})
Finalization->>Finalization: build SettlementInput / UsageEvent with propagated counters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Review Summary by QodoPersist web_search/code_interpreter completed counts in chat_turns for crash-safe billing
WalkthroughsDescription• Add DB migration to persist tool call counts in chat_turns table • Atomically increment web_search_completed_count/code_interpreter_completed_count on ToolPhase::Done • Orphan watchdog reads persisted counts for correct billing on pod crash recovery • Fix zero tool-call billing in orphan finalization path Diagramflowchart LR
migration["Migration m20260402_000003\nadd tool count columns"]
entity["chat_turn Entity\nweb_search_completed_count\ncode_interpreter_completed_count"]
repo["TurnRepository\nincrement_tool_calls()"]
provider["provider_task\nToolPhase::Done"]
orphan["orphan_watchdog\norphan_input_from_turn()"]
finalization["finalization_service\nweb_search_calls / code_interpreter_calls"]
migration -- "adds columns" --> entity
entity -- "model fields" --> repo
provider -- "calls on Done" --> repo
orphan -- "reads persisted counts" --> finalization
repo -- "atomic increment" --> entity
File Changes1. modules/mini-chat/mini-chat/src/domain/model/finalization.rs
|
Code Review by QodoLooking for bugs?Check back in a few minutes. An AI review agent is analyzing this pull request.
|
![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: 2
🧹 Nitpick comments (2)
modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs (1)
1761-1762: Use non-zero orphan tool counts in at least one assertion.Lines 1761, 1857, and 1908 only inject
0, so these tests still pass iffinalize_orphan_turn()regresses to hardcoded zeroes. Setting non-zero counts in a CAS-winner case and asserting the queuedUsageEventfields would pin the new behavior.🧪 Suggested test tightening
let input = crate::domain::model::finalization::OrphanFinalizationInput { turn_id, tenant_id, chat_id, request_id, user_id: Some(user_id), requester_type: mini_chat_sdk::RequesterType::User, effective_model: Some("gpt-5.2".to_owned()), reserve_tokens: Some(100), max_output_tokens_applied: Some(4096), reserved_credits_micro: Some(1000), policy_version_applied: Some(1), minimal_generation_floor_applied: Some(10), started_at: time::OffsetDateTime::now_utc(), - web_search_completed_count: 0, - code_interpreter_completed_count: 0, + web_search_completed_count: 2, + code_interpreter_completed_count: 1, }; @@ assert_eq!(usage_events.len(), 1, "should have one usage event"); assert_eq!(usage_events[0].terminal_state, "failed"); assert_eq!(usage_events[0].billing_outcome, "aborted"); + assert_eq!(usage_events[0].web_search_calls, 2); + assert_eq!(usage_events[0].code_interpreter_calls, 1);Also applies to: 1857-1858, 1908-1909
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs` around lines 1761 - 1762, Tests currently inject zero for web_search_completed_count and code_interpreter_completed_count which allows regressions that hardcode zeros to pass; update the test that covers the CAS-winner path (the one that calls finalize_orphan_turn()) to inject non-zero values for web_search_completed_count and/or code_interpreter_completed_count and add assertions that the queued UsageEvent (the usage event object enqueued by finalize_orphan_turn()) contains those same non-zero counts so the test fails if finalize_orphan_turn() returns hardcoded zeros.modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs (1)
9-18: Consider collapsing to oneALTER TABLEper direction to reduce lock churn and partial-apply surface.Two separate
ALTER TABLE ... ADD COLUMNstatements can hold/reacquire table-level DDL locks twice. A single statement is cleaner operationally.Proposed migration SQL consolidation
- conn.execute_unprepared( - "ALTER TABLE chat_turns ADD COLUMN web_search_completed_count INT NOT NULL DEFAULT 0", - ) - .await?; - conn.execute_unprepared( - "ALTER TABLE chat_turns ADD COLUMN code_interpreter_completed_count INT NOT NULL DEFAULT 0", - ) - .await?; + conn.execute_unprepared( + "ALTER TABLE chat_turns \ + ADD COLUMN web_search_completed_count INT NOT NULL DEFAULT 0, \ + ADD COLUMN code_interpreter_completed_count INT NOT NULL DEFAULT 0", + ) + .await?;- conn.execute_unprepared("ALTER TABLE chat_turns DROP COLUMN web_search_completed_count") - .await?; - conn.execute_unprepared( - "ALTER TABLE chat_turns DROP COLUMN code_interpreter_completed_count", - ) - .await?; + conn.execute_unprepared( + "ALTER TABLE chat_turns \ + DROP COLUMN web_search_completed_count, \ + DROP COLUMN code_interpreter_completed_count", + ) + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs` around lines 9 - 18, The migration currently issues two separate conn.execute_unprepared calls inside async fn up(&self, manager: &SchemaManager) — collapse them into a single ALTER TABLE statement that adds both web_search_completed_count and code_interpreter_completed_count in one command to avoid double DDL locking; update the call that uses manager.get_connection()/execute_unprepared to execute the combined SQL string, and mirror the same consolidation in the corresponding down migration (if present) so both add/remove column operations are applied in one ALTER TABLE per direction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs`:
- Around line 397-403: The DB connection error path is ignored when persisting
tool call counts in the ToolPhase::Done branches; update both places that use
fctx.db.conn() (the WebSearch branch using ToolCallType::WebSearch and the
CodeInterpreter branch using ToolCallType::CodeInterpreter) to log a warn! when
conn() returns Err so failures to acquire a connection are visible;
specifically, in the blocks that call fctx.turn_repo.increment_tool_calls(&conn,
&fctx.scope, fctx.turn_id, ...).await, add an else branch on the conn() attempt
that emits warn!(turn_id = %fctx.turn_id, error = %e, "failed to acquire DB
connection for persisting tool call counts") (or similar) so both
missing-connection and increment errors are logged.
In `@modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs`:
- Around line 385-403: The increment_tool_calls update currently filters only by
Column::Id (in TurnEntity::update_many()), allowing late ToolPhase::Done events
to increment counters on terminal turns; change the update to include the
live-turn predicate used elsewhere by adding filters for
Column::State.eq("running") (or the enum/variant used for running) and
Column::DeletedAt.is_null() (or equivalent) before
.secure().scope_with(scope).exec(runner) so the UPDATE becomes a no-op for
terminal/deleted turns.
---
Nitpick comments:
In `@modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs`:
- Around line 1761-1762: Tests currently inject zero for
web_search_completed_count and code_interpreter_completed_count which allows
regressions that hardcode zeros to pass; update the test that covers the
CAS-winner path (the one that calls finalize_orphan_turn()) to inject non-zero
values for web_search_completed_count and/or code_interpreter_completed_count
and add assertions that the queued UsageEvent (the usage event object enqueued
by finalize_orphan_turn()) contains those same non-zero counts so the test fails
if finalize_orphan_turn() returns hardcoded zeros.
In
`@modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs`:
- Around line 9-18: The migration currently issues two separate
conn.execute_unprepared calls inside async fn up(&self, manager: &SchemaManager)
— collapse them into a single ALTER TABLE statement that adds both
web_search_completed_count and code_interpreter_completed_count in one command
to avoid double DDL locking; update the call that uses
manager.get_connection()/execute_unprepared to execute the combined SQL string,
and mirror the same consolidation in the corresponding down migration (if
present) so both add/remove column operations are applied in one ALTER TABLE per
direction.
🪄 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: d5cc1d60-6922-4064-90a1-cb2268d410f5
📒 Files selected for processing (9)
modules/mini-chat/mini-chat/src/domain/model/finalization.rsmodules/mini-chat/mini-chat/src/domain/repos/turn_repo.rsmodules/mini-chat/mini-chat/src/domain/service/finalization_service.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rsmodules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/mod.rsmodules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rsmodules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs
Show resolved
Hide resolved
MikeFalcon77
reviewed
Apr 3, 2026
| ToolPhase::Done => { | ||
| web_search_completed_count += 1; | ||
| if let Some(ref fctx) = fin_ctx | ||
| && let Ok(conn) = fctx.db.conn() |
Collaborator
There was a problem hiding this comment.
MEDIUM
DB connection error silently swallowed. The let-chain && let Ok(conn) = fctx.db.conn() drops the error without logging — unlike the increment_tool_calls error on the next line which gets a warn!.
If the connection pool is exhausted or the DB is unreachable, this path produces zero signal, making pool-exhaustion incidents harder to diagnose. Separate the conn() call from the let-chain and log the error on failure. Same issue at line 490.
| policy_version_applied: Some(1), | ||
| minimal_generation_floor_applied: Some(10), | ||
| started_at: time::OffsetDateTime::now_utc(), | ||
| web_search_completed_count: 0, |
Collaborator
There was a problem hiding this comment.
HIGH
Core behavior untested. All three orphan-finalization test fixtures set web_search_completed_count: 0 and code_interpreter_completed_count: 0, so they cannot distinguish between the old hardcoded-zero behavior and the new pass-through.
Add a test where these counts are non-zero and assert that the settlement params / outbox event carry the same values. Without it, a regression that reverts to web_search_calls: 0 would pass the suite.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs (1)
397-402:⚠️ Potential issue | 🟠 MajorLog DB connection acquisition failures in ToolPhase::Done persistence branches.
Line 398 and Line 490 currently swallow
fctx.db.conn()errors via let-chains, so persistence misses can be silent. Keep best-effort behavior, but emit a warning when connection acquisition fails.🔧 Proposed fix
- if let Some(ref fctx) = fin_ctx - && let Ok(conn) = fctx.db.conn() - && let Err(e) = fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::WebSearch).await - { - warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist web_search_completed_count"); - } + if let Some(ref fctx) = fin_ctx { + match fctx.db.conn() { + Ok(conn) => { + if let Err(e) = fctx.turn_repo + .increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::WebSearch) + .await + { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist web_search_completed_count"); + } + } + Err(e) => { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to get DB connection for web_search_completed_count"); + } + } + } - if let Some(ref fctx) = fin_ctx - && let Ok(conn) = fctx.db.conn() - && let Err(e) = fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::CodeInterpreter).await - { - warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist code_interpreter_completed_count"); - } + if let Some(ref fctx) = fin_ctx { + match fctx.db.conn() { + Ok(conn) => { + if let Err(e) = fctx.turn_repo + .increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::CodeInterpreter) + .await + { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist code_interpreter_completed_count"); + } + } + Err(e) => { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to get DB connection for code_interpreter_completed_count"); + } + } + }Also applies to: 489-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs` around lines 397 - 402, The code currently uses a let-chain that swallows fctx.db.conn() errors when persisting in ToolPhase::Done; change the let-chain so you explicitly match on fin_ctx / fctx and then call fctx.db.conn() with a match (or if let Err(e) = ...) to log a warning on connection acquisition failure before attempting turn_repo.increment_tool_calls, and keep the existing behavior of warning when increment_tool_calls returns Err; apply the same change to the other occurrence that calls turn_repo.increment_tool_calls with ToolCallType::WebSearch so both connection-acquisition failures are logged (reference: fin_ctx / fctx, fctx.db.conn(), turn_repo.increment_tool_calls, ToolCallType::WebSearch, ToolPhase::Done).
🧹 Nitpick comments (1)
modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs (1)
711-810: Tests are comprehensive; consider adding terminal turn edge case.The new tests correctly verify initial zero values, per-type isolation, and accumulation. Once the
state = Runningfilter is added per the prior review comment, consider adding a test similar toupdate_progress_at_noop_on_terminal(lines 542-572) that verifiesincrement_tool_callsbecomes a no-op on finalized turns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs` around lines 711 - 810, Add a test to ensure increment_tool_calls is a no-op for terminal turns: create a terminal/finalized turn (reuse or mirror the approach from update_progress_at_noop_on_terminal — e.g., use a setup_finished_turn helper or create a turn and set its state to Terminal/Finalized instead of using setup_running_turn), call TurnRepository::increment_tool_calls with both ToolCallType::WebSearch and ToolCallType::CodeInterpreter, and assert that web_search_completed_count and code_interpreter_completed_count remain unchanged; name the test like increment_tool_calls_noop_on_terminal and place it with the other increment_tool_calls tests to cover the edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs`:
- Around line 397-402: The code currently uses a let-chain that swallows
fctx.db.conn() errors when persisting in ToolPhase::Done; change the let-chain
so you explicitly match on fin_ctx / fctx and then call fctx.db.conn() with a
match (or if let Err(e) = ...) to log a warning on connection acquisition
failure before attempting turn_repo.increment_tool_calls, and keep the existing
behavior of warning when increment_tool_calls returns Err; apply the same change
to the other occurrence that calls turn_repo.increment_tool_calls with
ToolCallType::WebSearch so both connection-acquisition failures are logged
(reference: fin_ctx / fctx, fctx.db.conn(), turn_repo.increment_tool_calls,
ToolCallType::WebSearch, ToolPhase::Done).
---
Nitpick comments:
In `@modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs`:
- Around line 711-810: Add a test to ensure increment_tool_calls is a no-op for
terminal turns: create a terminal/finalized turn (reuse or mirror the approach
from update_progress_at_noop_on_terminal — e.g., use a setup_finished_turn
helper or create a turn and set its state to Terminal/Finalized instead of using
setup_running_turn), call TurnRepository::increment_tool_calls with both
ToolCallType::WebSearch and ToolCallType::CodeInterpreter, and assert that
web_search_completed_count and code_interpreter_completed_count remain
unchanged; name the test like increment_tool_calls_noop_on_terminal and place it
with the other increment_tool_calls tests to cover the edge case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4bacb9a-aec9-440e-8bef-68cf0dc778a0
📒 Files selected for processing (12)
modules/mini-chat/mini-chat/src/domain/model/finalization.rsmodules/mini-chat/mini-chat/src/domain/repos/mod.rsmodules/mini-chat/mini-chat/src/domain/repos/turn_repo.rsmodules/mini-chat/mini-chat/src/domain/service/finalization_service.rsmodules/mini-chat/mini-chat/src/domain/service/replay.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/mod.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rsmodules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/mod.rsmodules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rsmodules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
✅ Files skipped from review due to trivial changes (4)
- modules/mini-chat/mini-chat/src/infra/db/migrations/mod.rs
- modules/mini-chat/mini-chat/src/domain/repos/mod.rs
- modules/mini-chat/mini-chat/src/domain/service/replay.rs
- modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/mini-chat/mini-chat/src/domain/model/finalization.rs
- modules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rs
- modules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
- modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs
- modules/mini-chat/mini-chat/src/domain/repos/turn_repo.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs (1)
397-402:⚠️ Potential issue | 🟠 MajorLog DB connection acquisition failures for persisted tool counts.
Line 398 and Line 490 still drop
db.conn()errors silently. Please emit awarn!when connection acquisition fails in both branches, not only whenincrement_tool_callsfails.🔧 Suggested patch
- if let Some(ref fctx) = fin_ctx - && let Ok(conn) = fctx.db.conn() - && let Err(e) = fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::WebSearch).await - { - warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist web_search_completed_count"); - } + if let Some(ref fctx) = fin_ctx { + match fctx.db.conn() { + Ok(conn) => { + if let Err(e) = fctx.turn_repo + .increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::WebSearch) + .await + { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist web_search_completed_count"); + } + } + Err(e) => { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to get DB connection for web_search_completed_count"); + } + } + } @@ - if let Some(ref fctx) = fin_ctx - && let Ok(conn) = fctx.db.conn() - && let Err(e) = fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::CodeInterpreter).await - { - warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist code_interpreter_completed_count"); - } + if let Some(ref fctx) = fin_ctx { + match fctx.db.conn() { + Ok(conn) => { + if let Err(e) = fctx.turn_repo + .increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::CodeInterpreter) + .await + { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to persist code_interpreter_completed_count"); + } + } + Err(e) => { + warn!(turn_id = %fctx.turn_id, error = %e, "failed to get DB connection for code_interpreter_completed_count"); + } + } + }Also applies to: 489-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs` around lines 397 - 402, The current conditional around fin_ctx (fctx) silently ignores errors returned by fctx.db.conn(); update the matching so you explicitly handle the Result from fctx.db.conn() and emit a warn! when connection acquisition fails (include turn_id and the connection error), then only call fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id, ToolCallType::WebSearch).await on Ok(conn); keep the existing warn! when increment_tool_calls itself returns Err to preserve that error log. Apply the same change to the other similar branch that calls increment_tool_calls (the second occurrence handling ToolCallType::WebSearch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs`:
- Around line 397-402: The current conditional around fin_ctx (fctx) silently
ignores errors returned by fctx.db.conn(); update the matching so you explicitly
handle the Result from fctx.db.conn() and emit a warn! when connection
acquisition fails (include turn_id and the connection error), then only call
fctx.turn_repo.increment_tool_calls(&conn, &fctx.scope, fctx.turn_id,
ToolCallType::WebSearch).await on Ok(conn); keep the existing warn! when
increment_tool_calls itself returns Err to preserve that error log. Apply the
same change to the other similar branch that calls increment_tool_calls (the
second occurrence handling ToolCallType::WebSearch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 725edf8e-5c0e-43c1-aa5f-3f8bb852e6ff
📒 Files selected for processing (12)
modules/mini-chat/mini-chat/src/domain/model/finalization.rsmodules/mini-chat/mini-chat/src/domain/repos/mod.rsmodules/mini-chat/mini-chat/src/domain/repos/turn_repo.rsmodules/mini-chat/mini-chat/src/domain/service/finalization_service.rsmodules/mini-chat/mini-chat/src/domain/service/replay.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/mod.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rsmodules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/mod.rsmodules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rsmodules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
✅ Files skipped from review due to trivial changes (6)
- modules/mini-chat/mini-chat/src/infra/db/migrations/mod.rs
- modules/mini-chat/mini-chat/src/domain/service/replay.rs
- modules/mini-chat/mini-chat/src/domain/service/stream_service/mod.rs
- modules/mini-chat/mini-chat/src/domain/repos/mod.rs
- modules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rs
- modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs
- modules/mini-chat/mini-chat/src/domain/repos/turn_repo.rs
- modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs (1)
716-815: Tests are solid; consider adding a guard-behavior test.The tests comprehensively cover:
- Initial counter state (zeros)
- Individual tool type increments
- Counter accumulation
- Counter isolation (incrementing one doesn't affect the other)
Optional: Consider adding a test verifying that
increment_tool_callsis a no-op when the turn is terminal (similar toupdate_progress_at_noop_on_terminal). This would explicitly validate the CAS guard added at lines 401-402.🧪 Optional test for guard behavior
#[tokio::test] async fn increment_tool_calls_noop_on_terminal() { let db = mock_db_provider(inmem_db().await); let (_, chat_id, turn_id, request_id) = setup_running_turn(&db).await; let conn = db.conn().unwrap(); let scope = AccessScope::allow_all(); let repo = TurnRepository; // Finalize the turn to Failed repo.cas_update_state( &conn, &scope, crate::domain::repos::CasTerminalParams { turn_id, state: TurnState::Failed, error_code: Some("test".to_owned()), error_detail: None, assistant_message_id: None, provider_response_id: None, }, ) .await .unwrap(); // Attempt to increment — should be no-op repo.increment_tool_calls(&conn, &scope, turn_id, ToolCallType::WebSearch) .await .expect("increment should not error"); let turn = repo .find_by_chat_and_request_id(&conn, &scope, chat_id, request_id) .await .unwrap() .expect("turn should exist"); assert_eq!( turn.web_search_completed_count, 0, "counter should remain 0 for terminal turn" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs` around lines 716 - 815, Add a guard-behavior test to ensure increment_tool_calls is a no-op for terminal turns: create a new #[tokio::test] (e.g., increment_tool_calls_noop_on_terminal) that sets up a running turn via setup_running_turn, marks it terminal using TurnRepository::cas_update_state with TurnState::Failed, then calls TurnRepository::increment_tool_calls with ToolCallType::WebSearch and asserts the persisted turn (via find_by_chat_and_request_id) still has web_search_completed_count == 0; this mirrors the existing update_progress_at_noop_on_terminal check and verifies the CAS guard in increment_tool_calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@modules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rs`:
- Around line 716-815: Add a guard-behavior test to ensure increment_tool_calls
is a no-op for terminal turns: create a new #[tokio::test] (e.g.,
increment_tool_calls_noop_on_terminal) that sets up a running turn via
setup_running_turn, marks it terminal using TurnRepository::cas_update_state
with TurnState::Failed, then calls TurnRepository::increment_tool_calls with
ToolCallType::WebSearch and asserts the persisted turn (via
find_by_chat_and_request_id) still has web_search_completed_count == 0; this
mirrors the existing update_progress_at_noop_on_terminal check and verifies the
CAS guard in increment_tool_calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a41ad81-2642-48d4-9092-f2abe28ab1aa
📒 Files selected for processing (12)
modules/mini-chat/mini-chat/src/domain/model/finalization.rsmodules/mini-chat/mini-chat/src/domain/repos/mod.rsmodules/mini-chat/mini-chat/src/domain/repos/turn_repo.rsmodules/mini-chat/mini-chat/src/domain/service/finalization_service.rsmodules/mini-chat/mini-chat/src/domain/service/replay.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/mod.rsmodules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rsmodules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rsmodules/mini-chat/mini-chat/src/infra/db/migrations/mod.rsmodules/mini-chat/mini-chat/src/infra/db/repo/turn_repo.rsmodules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
✅ Files skipped from review due to trivial changes (7)
- modules/mini-chat/mini-chat/src/infra/db/migrations/mod.rs
- modules/mini-chat/mini-chat/src/domain/service/replay.rs
- modules/mini-chat/mini-chat/src/infra/db/entity/chat_turn.rs
- modules/mini-chat/mini-chat/src/infra/workers/orphan_watchdog.rs
- modules/mini-chat/mini-chat/src/domain/repos/mod.rs
- modules/mini-chat/mini-chat/src/infra/db/migrations/m20260402_000003_add_tool_counts_to_turns.rs
- modules/mini-chat/mini-chat/src/domain/service/stream_service/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/mini-chat/mini-chat/src/domain/model/finalization.rs
- modules/mini-chat/mini-chat/src/domain/service/stream_service/provider_task.rs
- modules/mini-chat/mini-chat/src/domain/service/finalization_service.rs
…in chat_turns Signed-off-by: Jalan Kulkija <ankoo67@proton.me>
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
2 participants
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.
Tool call counts (
web_search_completed_count,code_interpreter_completed_count) were tracked only in-memory inprovider_task.rsand lost on pod crash. The orphan watchdog therefore always billed zero tool calls when recovering abandoned turns.Changes:
m20260402_000003adds both columns tochat_turns(INT NOT NULL DEFAULT 0)TurnRepository::increment_tool_callsatomically increments the relevant counter on eachToolPhase::Done(best-effort; errors are logged and swallowed)orphan_input_from_turnreads the persisted counts so orphan finalization bills correctlyCloses #1362.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests