Skip to content

fix(conversation): avoid creating a conversation on chat reconnect#43

Merged
jasonchen922 merged 1 commit into
microsoft:mainfrom
xinkuleee:fix/conversation-reconnect-ensure-conversation
Jul 8, 2026
Merged

fix(conversation): avoid creating a conversation on chat reconnect#43
jasonchen922 merged 1 commit into
microsoft:mainfrom
xinkuleee:fix/conversation-reconnect-ensure-conversation

Conversation

@xinkuleee

@xinkuleee xinkuleee commented Jul 7, 2026

Copy link
Copy Markdown

Summary

Reconnect previously called ensureConversation (get-or-create). A reconnect
only resumes an in-progress turn, so when a client reconnects to an agent
instance that has no existing conversation, creating one is unnecessary and a
side effect — it leaves a stray empty conversation and then immediately ends the
stream because there is no ongoing turn.

This changes Reconnect to look the conversation up read-only
(conversationRepo.Get). If none exists, it sends the terminal done event and
returns without creating a conversation or touching the ongoing-turn cache.
Chat still uses ensureConversation, where first-turn creation is expected.

Validation

  • make precommit-run
  • cd backend && go test ./...
  • cd core && uv run pytest
  • Frontend build/lint, if working from a frontend source checkout
  • Not run; explain below

Backend-only change. make precommit-run, go test ./..., and
golangci-lint run all pass locally. Core and frontend suites were not run
because neither is touched by this change.

Checklist

  • Linked relevant issues or explained why there is no issue.
  • Updated docs and examples where behavior changed.
  • Updated CHANGELOG.md under ## [Unreleased] for user-facing changes.
  • Regenerated and committed generated files after proto, Wire, or OpenAPI changes.
  • Confirmed no secrets, tokens, credentials, or sensitive logs are included.

Notes for reviewers

  • New test TestReconnect covers the "no conversation -> don't create, send
    done" path. The cache-backed resume paths still require Redis and aren't
    unit-tested here (s.cache is a concrete *redis.Client); covering them would
    need miniredis or a cache interface — possible follow-up.
  • Files changed: chat.go (fix) and chat_test.go (test).
  • Branch is rebased onto latest main.

@jasonchen922 jasonchen922 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.

LGTM

Copilot AI left a comment

Copy link
Copy Markdown

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 adjusts the backend conversation chat reconnection flow so that Reconnect no longer performs a get-or-create on conversations. Instead, it performs a read-only lookup and, when no conversation exists, it terminates the SSE stream cleanly by emitting a done event—preventing stray empty conversation records from being created as a side effect of reconnecting.

Changes:

  • Updated Service.Reconnect to use conversationRepo.Get (read-only) instead of ensureConversation (get-or-create).
  • Added an explicit “no conversation” fast-path that emits a terminal done SSE event and returns without touching the ongoing-turn cache.
  • Added a unit test covering the “no conversation -> don’t create one, send done” reconnect behavior.

Reviewed changes

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

File Description
backend/internal/biz/conversation/impl/chat.go Switches reconnect to read-only conversation lookup and cleanly ends the stream when no conversation exists.
backend/internal/biz/conversation/impl/chat_test.go Adds coverage ensuring reconnect doesn’t create a conversation and only emits a terminal done event when none exists.

@jasonchen922 jasonchen922 merged commit 5cb5db9 into microsoft:main Jul 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants