Skip to content

fix(daemon): auto-recover transcript on ring_evicted resync#4702

Merged
doudouOUC merged 1 commit into
daemon_mode_b_mainfrom
feat/improve_state_resync_required
Jun 2, 2026
Merged

fix(daemon): auto-recover transcript on ring_evicted resync#4702
doudouOUC merged 1 commit into
daemon_mode_b_mainfrom
feat/improve_state_resync_required

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • When a WebUI client reconnects with a stale SSE cursor past the EventBus ring buffer, the daemon emits state_resync_required(ring_evicted). Previously the provider did nothing for this reason — the reducer's awaitingResync latch silently dropped all subsequent ring replay events, leaving the transcript with a permanent history gap.
  • Hoist store.reset() before the reason-specific branches so it runs for all resync reasons including ring_evicted. This clears the latch and empties the stale transcript, allowing ring replay events to rebuild it from the surviving ring contents (up to 8000 events).
  • Remove the misleading "Reload the session to recover." from the resync error message — reloading re-triggers ring eviction for long sessions, and the WebUI now auto-recovers.

Test plan

  • Updated "resets store on ring-evicted resync so replay events rebuild transcript" — verifies replay events are processed (not dropped) and error block is cleared by reset
  • Updated "clears ring-evicted awaitingResync on same-session reattach" — verifies error block is cleared by reset before stream errors
  • npx vitest run test/unit/daemonUi.test.ts — 200 tests passed (no regression on error message assertions)
  • npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx — 37 tests passed

🤖 Generated with Qwen Code

@doudouOUC
fix(daemon): auto-recover transcript on ring_evicted resync
d98293d 
When a WebUI client reconnects with a stale SSE cursor past the
EventBus ring buffer, the daemon emits state_resync_required with
reason ring_evicted. Previously the provider did nothing for this
reason, leaving the transcript frozen because the reducer's
awaitingResync latch silently dropped all subsequent replay events.

Hoist store.reset() before the reason-specific branches so it runs
for all resync reasons including ring_evicted. This clears the
awaitingResync latch and empties the stale transcript, allowing the
ring replay events (already queued by EventBus.subscribe) to flow
through the reducer and rebuild the transcript from the surviving
ring contents.

Also remove the misleading "Reload the session to recover." suffix
from the resync error message — reloading re-triggers ring eviction
for long sessions, and the WebUI now auto-recovers.
Copilot AI review requested due to automatic review settings June 2, 2026 11:43
Copilot AI reviewed Jun 2, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a WebUI transcript recovery bug when reconnecting with a stale SSE cursor that has fallen behind the daemon’s EventBus ring buffer (state_resync_required with reason ring_evicted). By resetting the UI store for all resync reasons (including ring_evicted), it clears the awaitingResync latch and stale transcript state so subsequent replay events can rebuild the transcript from the remaining ring contents. It also updates the transcript resync error text to remove the misleading “Reload the session to recover.” guidance now that the WebUI auto-recovers.

Changes:

  • Reset the session UI store on all state_resync_required reasons (including ring_evicted) to allow replay events to repopulate transcript state.
  • Update WebUI provider tests to assert replay rebuild behavior and ensure the resync error block is cleared after reset.
  • Simplify the transcript resync error message by removing the “Reload the session to recover.” suffix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/webui/src/daemon/session/DaemonSessionProvider.tsx Hoists store.reset() so ring_evicted resync also clears awaitingResync and stale transcript state before replay continues.
packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx Updates/strengthens tests to validate replay events rebuild transcript after ring-evicted resync and that the resync error block is cleared.
packages/sdk-typescript/src/daemon/ui/transcript.ts Removes misleading reload instruction from the State resync required error text.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR fixes a bug where the daemon UI transcript becomes permanently corrupted when a WebUI client reconnects with a stale SSE cursor past the EventBus ring buffer. The fix ensures store.reset() is called for all resync reasons including ring_evicted, allowing replay events to rebuild the transcript. The implementation is minimal, well-tested, and addresses the root cause effectively.

🔍 General Feedback

  • Clean, minimal fix: The change follows the "minimum code that solves the problem" principle—hoisting store.reset() before the reason-specific branches is exactly the right level of abstraction for this fix.
  • Test coverage is excellent: The updated tests verify both that replay events are processed correctly and that the error block is cleared, covering the two key behaviors.
  • Good error message improvement: Removing the misleading "Reload the session to recover." message is appropriate since the WebUI now auto-recovers and reloading would re-trigger ring eviction for long sessions.
  • Consistent with existing patterns: The fix maintains the existing code structure and just reorders when store.reset() is called.

🎯 Specific Feedback

🔵 Low

  • File: packages/webui/src/daemon/session/DaemonSessionProvider.tsx:508-518 - Consider adding a brief comment explaining why store.reset() must be called before the reason-specific branches. This would help future maintainers understand that the reset is required to clear the awaitingResync latch for all resync reasons, not just epoch_reset. Example:

    // Reset store first to clear awaitingResync latch so replay events
// are processed for all resync reasons including ring_evicted.
store.reset();

  • File: packages/sdk-typescript/src/daemon/ui/transcript.ts:319 - The error message change is correct, but consider whether any additional guidance should be provided to users when this error occurs. Since the WebUI auto-recovers, the message could say "Reconnecting automatically..." or similar to set user expectations. This is optional since the current change is already an improvement.

✅ Highlights

  • Excellent test updates: The renamed test "resets store on ring-evicted resync so replay events rebuild transcript" clearly describes the expected behavior, and the test now verifies both positive (replayed history appears) and negative (no error block) outcomes.
  • Correct root cause analysis: The PR description accurately identifies that the reducer's awaitingResync latch was silently dropping subsequent ring replay events—this is a subtle bug that would be easy to miss.
  • No regression risk: The test plan confirms 200 tests pass in daemonUi.test.ts and 37 tests pass in DaemonSessionProvider.test.tsx, demonstrating thorough validation.
  • Good user experience improvement: Removing the misleading reload instruction prevents users from triggering re-eviction on long sessions.

@doudouOUC doudouOUC requested review from chiga0, wenshao and ytahdn June 2, 2026 12:01
chiga0
chiga0 approved these changes Jun 2, 2026
View reviewed changes

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Overview (AI Generated)

PR: #4702 fix(daemon): auto-recover transcript on ring_evicted resync
Type: Bug Fix
Change size: +29/-7 across 3 files

Multi-Round Review (Rounds 0-6): Clean — 0 findings

Round 0 (Design): Correct minimal fix — hoists store.reset() before reason-specific branches so it runs for ALL resync reasons including ring_evicted. This clears the awaitingResync latch and stale transcript, allowing ring replay events to rebuild.

Round 1 (Architecture):

  • Single code reordering in DaemonSessionProvider.tsx — store.reset() moved from inside the epoch_reset and else branches to before the if/else
  • Error message updated to remove misleading "Reload the session to recover." — correct since reloading re-triggers ring eviction for long sessions

Round 2 (Robustness):

  • ring_evicted previously did nothing → transcript stuck. Now resets and allows replay events to rebuild.
  • epoch_reset behavior unchanged — still resets + sets lastEventId=0
  • Unknown reasons still trigger full reload (resyncRequested + store.reset + session=undefined)

Round 3 (CI/Security): N/A — purely UI state management.

Round 4 (Performance): No performance impact — just reorders existing operations.

Round 5 (Bug Fix): Root cause correctly identified: ring_evicted was silently ignored, leaving awaitingResync latch stuck. Test updated to verify replay events rebuild transcript after reset.

Round 6 (Undirected): The change is semantically correct — resetting for ring_evicted is the right behavior since the transcript was stale anyway. Edge case (no ring replay events) results in empty transcript, which is still better than being stuck.

LGTM!

This review was generated by QoderWork AI

@github-actions github-actions Bot mentioned this pull request Jun 2, 2026
Open
wenshao
wenshao approved these changes Jun 2, 2026
View reviewed changes

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC merged commit 47e217f into daemon_mode_b_main Jun 2, 2026
4 checks passed
@doudouOUC doudouOUC deleted the feat/improve_state_resync_required branch June 2, 2026 14:52
@doudouOUC doudouOUC mentioned this pull request Jun 4, 2026
Merged
xaelistic pushed a commit to xaelistic/qwen-code that referenced this pull request Jun 7, 2026
@abhipatel12
feat(commands): Implement argument handling for custom commands via a…
b53162f 
… prompt pipeline (QwenLM#4702)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@wenshao wenshao wenshao approved these changes

@chiga0 chiga0 chiga0 approved these changes

@ytahdn ytahdn Awaiting requested review from ytahdn

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@doudouOUC @wenshao @chiga0