Skip to content

fix(desktop): mark transcription connected on websocket open#9071

Merged
eulicesl merged 3 commits into
BasedHardware:mainfrom
eulicesl:codex/desktop-transcription-connect-state
Jul 5, 2026
Merged

fix(desktop): mark transcription connected on websocket open#9071
eulicesl merged 3 commits into
BasedHardware:mainfrom
eulicesl:codex/desktop-transcription-connect-state

Conversation

@eulicesl

@eulicesl eulicesl commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes a desktop transcription connection-state bug where TranscriptionService marked the WebSocket as connected from a fixed 0.5s timer after resume(), rather than from the WebSocket handshake/open event.

Bug

When cloud transcription starts, the app could briefly treat the stream as connected before the WebSocket had actually opened. That could allow audio send/connected callbacks to run against a task that was only in the pre-handshake phase.

Root Cause

TranscriptionService.connectToBackend created a plain URLSession, called webSocketTask.resume(), then set isConnected = true from DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) if the task state looked running. URLSessionWebSocketTask.State.running is not the same as the server-side WebSocket open event.

Fix

  • Install a small URLSessionWebSocketDelegate for the transcription WebSocket session.
  • Move the isConnected = true, reconnect-attempt reset, watchdog start, and onConnected callback into the delegate didOpenWithProtocol path.
  • Route delegate close events through the existing connected/pre-handshake reconnect paths.
  • Ignore stale delegate callbacks by checking that the callback still belongs to the current webSocketTask.
  • Scope the 10s pre-handshake timeout to the same webSocketTask, so a stale timeout from an older attempt cannot interrupt a newer reconnect attempt.

Testing

Before fix:

  • Added focused regression test and confirmed it failed on current upstream behavior:
    • TranscriptionService must not mark the WebSocket connected from a fixed timer
    • TranscriptionService should install a URLSessionWebSocketDelegate
    • TranscriptionService should mark connected from URLSessionWebSocketDelegate.didOpenWithProtocol

After fix:

  • xcrun swift test --filter TranscriptionConnectionStateTests --package-path Desktop passed: 2 tests, 0 failures
  • python3 .github/scripts/desktop-changelog.py validate passed
  • git diff --check passed
  • xcrun swift build -c debug --package-path Desktop passed
  • ./scripts/agent-logic-harness.sh passed all sections:
    • harness failure propagation self-check
    • swift focused lifecycle/state tests
    • agent runtime focused tests
    • pi-mono-extension exact package tests
  • Push pre-push hooks passed, including desktop changelog validation and desktop Swift build

Review follow-up after Cubic comments:

  • Scoped the 10s connect timeout to the current WebSocket task.
  • Updated the focused regression test to assert stale connect timeouts are ignored.
  • Updated the changelog fragment to start with Fixed.
  • Re-ran xcrun swift test --filter TranscriptionConnectionStateTests --package-path Desktop: passed, 2 tests, 0 failures.
  • Re-ran python3 .github/scripts/desktop-changelog.py validate: passed.
  • Re-ran git diff --check: passed.
  • Re-ran xcrun swift build -c debug --package-path Desktop: passed.
  • Re-ran ./scripts/agent-logic-harness.sh: passed.
  • Re-push pre-push hooks passed, including desktop Swift build.

Second review follow-up after Copilot comments:

  • Kept the WebSocket open transition on the main queue before connected state and onConnected run, preserving the previous callback queue behavior.
  • Replaced raw source-substring assertions with behavior-level tests for delegate forwarding and WebSocket task identity matching.
  • Re-ran xcrun swift test --filter TranscriptionConnectionStateTests --package-path Desktop: passed, 2 tests, 0 failures.
  • Re-ran python3 .github/scripts/desktop-changelog.py validate: passed.
  • Re-ran git diff --check: passed.
  • Re-ran xcrun swift build -c debug --package-path Desktop: passed.
  • Re-ran ./scripts/agent-logic-harness.sh: passed.
  • Re-push pre-push hooks passed, including desktop Swift build.

Visual Proof

No useful screenshot for this one: the bug is an internal transport-state race before/around the WebSocket handshake. The strongest evidence is the failing-before/passing-after focused regression plus debug build and desktop harness output.

Risk

Low. The change keeps existing reconnect methods and only changes the source of truth for the initial connected transition from a timer to the system WebSocket open callback.

Rollback

Revert this commit to restore the previous timer-based connection behavior.

Review in cubic

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@cubic-dev-ai cubic-dev-ai Bot 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.

2 issues found across 3 files

Confidence score: 3/5

  • In desktop/macos/Desktop/Sources/TranscriptionService.swift, reconnect logic can schedule overlapping retries when onClose fires before handshake while a prior 10s timeout is still pending, which can over-count backoff and cause unstable or delayed recovery for transcription sessions — cancel/dedupe stale reconnect timers and ensure only one reconnect path can be active before merging.
  • In desktop/macos/changelog/unreleased/20260705-transcription-connect-state.json, the entry format does not follow the required “Fixed/Added/Improved” convention, which risks release-note inconsistency — rename it to start with “Fixed …” before merge.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread desktop/macos/Desktop/Sources/TranscriptionService.swift
Comment thread desktop/macos/changelog/unreleased/20260705-transcription-connect-state.json Outdated

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 desktop transcription connection-state race by making TranscriptionService mark the WebSocket as connected based on the actual WebSocket open/handshake event (via URLSessionWebSocketDelegate) rather than a fixed timer after resume().

Changes:

  • Introduces a URLSessionWebSocketDelegate (WebSocketConnectionDelegate) and wires isConnected + watchdog startup to didOpenWithProtocol.
  • Routes WebSocket close events through the existing connected vs pre-handshake reconnect paths, while ignoring stale delegate callbacks.
  • Adds a focused regression test file plus a desktop changelog fragment for the behavior change.

Reviewed changes

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

File Description
desktop/macos/Desktop/Tests/TranscriptionConnectionStateTests.swift Adds regression coverage for the “connected is driven by WebSocket open” behavior.
desktop/macos/Desktop/Sources/TranscriptionService.swift Moves connected-state transition to the WebSocket delegate open callback; adds delegate and cleanup.
desktop/macos/changelog/unreleased/20260705-transcription-connect-state.json Documents the user-facing behavior change in connection status reporting.

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

Comment thread desktop/macos/Desktop/Sources/TranscriptionService.swift Outdated
Comment thread desktop/macos/Desktop/Sources/TranscriptionService.swift
Comment thread desktop/macos/Desktop/Tests/TranscriptionConnectionStateTests.swift Outdated
@eulicesl eulicesl merged commit bc50c8b into BasedHardware:main Jul 5, 2026
2 checks passed
@eulicesl eulicesl deleted the codex/desktop-transcription-connect-state branch July 5, 2026 20:38
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.

2 participants