Skip to content

Realtime lifecycle cleanup - #235

Merged
goodroot merged 2 commits into
mainfrom
realtime-lifecycle-cleanup
Aug 3, 2026
Merged

Realtime lifecycle cleanup#235
goodroot merged 2 commits into
mainfrom
realtime-lifecycle-cleanup

Conversation

@goodroot

@goodroot goodroot commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Material improvements to the real time life cycle. This is an extension of #229.

@goodroot
goodroot merged commit 447f328 into main Aug 3, 2026
2 checks passed
@goodroot
goodroot deleted the realtime-lifecycle-cleanup branch August 3, 2026 19:15
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Continue PR Review

Automated review via Continue CLI run through opub.

Summary

The cleanup/reconnect refactor is directionally good and the new regression tests cover the main issue #229 paths, but I found one likely functional regression in reconnect behavior and one important missing test around the new cooldown/user-facing state logic.

Findings

  • Reconnect path may stay in permanent cooldown after a failed reconnect of an existing client. In lib/src/backends/realtime_ws_backend.py, _ensure_client() only applies cooldown tracking when _realtime_client is None and a full rebuild is attempted. If an existing client is present but disconnected, _ensure_client() always calls _reconnect_realtime_client() with no cooldown or backoff. After repeated reconnect failures, last_connect_failure becomes "failed", but every recording attempt will still synchronously retry connect() immediately. This appears to contradict the new intent (“Don't rebuild a torn-down client on every keypress while an endpoint is down”) and can still cause repeated network hammering / UI stalls for the idle-closed-client case.
  • Missing regression test for the new user-visible reconnect failure messaging path. lib/main.py:_realtime_unavailable_message() now varies behavior based on WhisperManager.realtime_connect_failure(), but the new test file only exercises backend/client state, not the app-level message selection. A small unit test should verify the three outcomes (failed, cooldown, default/connecting) so future changes don’t regress into misleading “try again in a moment” messages again.

Suggested follow-ups

  • Add cooldown/backoff handling for failed reconnects when a disconnected client still exists, not just when rebuilding from None.
  • Add unit tests for _realtime_unavailable_message() covering failed, cooldown, and default/connecting outcomes.

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.

1 participant