Fix provider retry waiting feedback - #2225
Conversation
Publish a temporary stream update while provider retries so the UI remains responsive. Fixes #2221
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughDeepChatLoopRunner now reports provider retry delays through an ephemeral rate-limit message. It clears the message when retries start, finish, abort, or when the run exits. Waiting blocks omit undefined queue metrics. Tests cover consecutive retries and cancellation. ChangesProvider retry waiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change shows and clears temporary provider retry status across the retry lifecycle; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Provider
participant retryObserver
participant DeepChatLoopRunner
participant ChatStream
Provider->>retryObserver: Schedule retry after provider failure
retryObserver->>DeepChatLoopRunner: retry_scheduled
DeepChatLoopRunner->>ChatStream: Emit ephemeral waiting block
Provider->>retryObserver: Start or finish retry
retryObserver->>DeepChatLoopRunner: retry_started or retry_finished
DeepChatLoopRunner->>ChatStream: Clear waiting block
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address the silent waiting symptom in issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 too large.)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zerob13
left a comment
There was a problem hiding this comment.
Review summary
Request changes — the targeted tests pass, but the retry status lifecycle still emits an unnecessary clear between consecutive retries.
P2 — Avoid clearing the retry indicator between retries
- Location:
src/main/agent/deepchat/runtime/deepChatLoopRunner.ts:2075-2086 - When a retry attempt finishes with
retryDecision: retry_scheduled,contextCoordinatoremitsretry_finishedand then immediately emits the nextretry_scheduledevent. The new observer clears the ephemeral rate-limit snapshot on everyretry_finished, then publishes it again for the next schedule. - This produces an empty snapshot and a visible status flicker during multi-retry runs, even though the run remains in the same waiting state. It also makes the UI briefly lose the estimated wait state between retries.
- Please only clear on a terminal retry outcome, or otherwise keep the indicator continuous when
retry_finishedis followed by anotherretry_scheduled. Add a regression assertion covering two consecutive transient retries and the abort path.
Verification
pnpm exec vitest run --config vitest.config.ts test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts test/main/agent/deepchat/loop/contextCoordinator.test.ts- Result: 482 tests passed
git diff --check- Result: clean
No other merge-blocking issue was identified in this pass.
Avoid clearing the temporary retry status between consecutive provider retries and cover cancellation during the retry wait.
|
Thanks for catching this. Fixed in
The targeted regression tests pass (2/2), and the touched files have no diagnostics; |
zerob13
left a comment
There was a problem hiding this comment.
Review summary
Approve — the previous blocking finding is resolved.
Re-review scope
Reviewed the incremental changes from 992ecc292 to dac59bb539e892dac818926fc5d8d33b56656d00.
retry_finishednow clears the ephemeral retry status only for terminal outcomes; consecutiveretry_scheduledtransitions keep the status continuous.- Added focused regression coverage for consecutive transient retries and cancellation during the retry wait.
- No over-engineering or unexplained breaking change found. The lifecycle handling matches the existing
retry_scheduled→retry_started→ terminalretry_finishedflow. - Test additions are targeted and proportionate.
Verification
- Focused Vitest: 483 tests passed
oxfmt --check: cleangit diff --check: clean- PR checks: all required checks passed
Review status: APPROVED
Show a temporary rate-limit status while a provider request is waiting to retry, then clear it when retry starts, finishes, aborts, or the run exits. Provider retry payloads stay separate from local QPS queue fields, and a harness regression test covers the full publish-and-clear flow.
Fixes #2221
Tests:
pnpm exec vitest run --config vitest.config.ts test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts test/main/agent/deepchat/runtime/deepChatLoopRunner.test.tsgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests