hope-voice-api/.artifacts/20260831-105824-plan-code-ui-audit/iterations/07-implementation.md

Iteration 07 Implementation Report

• Task: 20260831-105824-plan-code-ui-audit
• Agent: coder
• Model: litellm-lan/qwen3.8-27b
• Status: complete

Summary

All 6 corrections from iterations/07-request.md are implemented. The changes address ordering/race bugs in failure teardown, microphone interruption serialization, cross-source dedupe consumption, server replacement coalescing, and media interruption resume safety. A new .tearingDown state prevents retry during failure cleanup; the mic operation chain now retains both begin and end; dedupe suppressions consume (remove) rather than transfer records; server Apply coalesces rapid values and constructs from captured config; media resume tasks are retained with sequence recheck before play.

Per-Item Detail

1. Failure teardown state (.tearingDown)

Problem: teardownForFailure published .failed before cleanup completed, exposing Start prematurely. A user could tap Start during cleanup, and the old failure task would then tear down the new room.

Fix:

• Added case tearingDown to VoiceSessionState.
• teardownForFailure(message:) now sets .tearingDown before any await (idempotent guard: returns if already .tearingDown or .failed). All cleanup (agent wait cancel, generation bump, mic op cancel, state model stop, idle timer restore, media end, room teardown) runs while in .tearingDown. Only after all cleanup completes does it publish .failed(message).
• connect() rejects .tearingDown (only accepts .idle and .failed).
• Added isFailureTearingDown computed property for UI/test access.
• ConversationView: .tearingDown shows "Cleaning up…" in status text, ProgressView in composer, and maps to .connecting orb state.
• SettingsView: Apply is disabled during .tearingDown with "Waiting for failure cleanup to finish…" help text.

Test seam: The isFailureTearingDown property and the state transition logic are deterministic and inspectable. A full lifecycle test would require a fake Room (not available in current test infrastructure); the state machine is verified by inspection and the build/test pass.

2. Microphone operation chain (retained begin + end)

Problem: .ended created an unretained task. A newer .began could only cancel micMuteTask (the begin task), not the in-flight end/unmute task. An old end could complete its unmute after a newer interruption's mute, leaving the mic enabled during the newer interruption.

Fix:

• Renamed micMuteTask → micOpTask (the single retained operation chain).
• Both .began and .ended are now assigned to micOpTask. Each new operation captures the predecessor (let predecessor = micOpTask) and awaits it before executing.
• Begin: increments sequence, then after predecessor completion, mutes and records ownership only after success (checks !Task.isCancelled, generation match, sequence still current).
• End: assigned to the chain (never unretained), awaits begin, then un-mutes only for matching sequence/generation/ownership/state/textMode/shouldResume. Clears micMutedByInterruption regardless of outcome.
• A newer .began orders after an in-flight old end/unmute (awaits predecessor) and finishes by muting again — the old unmute cannot leave hardware enabled.
• disconnect() and teardownForFailure() cancel micOpTask and nil it out.

3. Consume (remove) on cross-source suppress

Problem: "Consume-on-suppress" transferred ownership (replaced the record with the arriving source) instead of removing it. For P1,D1,P2,D2 with identical text: P1 accepted; D1 suppressed + recorded .data; P2 suppressed (opposite of .data) + recorded .primary; D2 suppressed (opposite of .primary). Only one utterance remained — the second was lost.

Fix:

• In receiveSegment: when suppressing a data-sourced duplicate, set recentFinals[key] = nil (remove) instead of replacing with .primary.
• In receiveDataTranscript: when suppressing a primary-sourced duplicate, set recentFinals[key] = nil (remove) instead of replacing with .data.
• After consumption, the next arrival finds no record and is accepted as a new pair.

Tests added:

• primaryDataPrimaryDataPairs: P1,D1,P2,D2 → exactly 2 canonical + persisted rows.
• dataPrimaryDataPrimaryPairs: D1,P1,D2,P2 → exactly 2 canonical + persisted rows.
• Updated sameSourceRepeatsAfterConsume to reflect remove semantics (P accepted, D suppressed+consumed, P accepted again, D suppressed).

4. Server replacement coalescing + captured config

Problem: While a replacement was active, a second Apply was dropped by guard !isReplacingSession. The first task then read mutable settings.serverURL (which now held the second URL) but set activeServerURL to the first URL — identity mismatch.

Fix:

• Added @State private var pendingServerURL: String? to ConversationView.
• If isReplacingSession is true, retain the latest accepted URL in pendingServerURL (coalesce, latest wins) rather than dropping.
• After the current replacement completes, process any pending URL: disconnect the just-created session, construct a new one from the pending URL's validated config.
• Session construction uses ServerConfig.parse(trimmed) on the captured URL string, not settings.serverURL (which may have changed during the async disconnect).
• SettingsView: canApplyServerURL now permits .idle and .failed (fully torn-down failure — user can correct an unreachable server without relaunching). Disabled during .tearingDown, .connecting, .waitingAgent, .ready.
• SettingsView: session is now @ObservedObject var session: VoiceSession (nonoptional, directly observed) so disabled/help state is reactive.

5. Media interruption retained task + sequence recheck

Problem: The should-resume path started an unretained async task. A subsequent .began set isInterruptionActive = true and paused, but neither cancelled the resume task nor changed the checked generation. The old task could reach player.play() during the newer interruption.

Fix:

• Added mediaInterruptionSequence (incremented on every .began) and mediaInterruptionTask (retained operation chain).
• Begin: increments sequence, cancels prior mediaInterruptionTask, sets isInterruptionActive = true, pauses.
• End (shouldResume): captures sequence + generation + room, assigns task to mediaInterruptionTask. After server sync, rechecks !Task.isCancelled, mediaInterruptionSequence == seq (a newer .began increments this), !isInterruptionActive, generation, and room immediately before player.play().
• End (no-resume): also assigned to mediaInterruptionTask (retained).
• A newer .began makes an old resume task incapable of playing (sequence mismatch + cancellation).
• No-resume suppression semantics preserved (resumeSuppressed flag unchanged).

6. Artifact suite counts + test title rename

Fix:

• Renamed @Test("206 with unknown total returns nil (falls back to expectedContentLength)") → @Test("206 with unknown total (*) returns nil") in PinnedMediaResourceLoaderTests.swift.
• Actual per-suite counts from source (grep -c '@Test('): TranscriptDedupeTests 24, MediaReconcileTests 7, ConversationStateTests 5, MediaAPITests 1, PinnedMediaResourceLoaderTests 12 = 49 total.

Files Changed

• Sources/LiveKit/VoiceSession.swift — .tearingDown state, teardownForFailure publishes .failed after cleanup, connect() rejects .tearingDown, isFailureTearingDown property, micOpTask retained chain for both begin/end
• Sources/LiveKit/TranscriptHandler.swift — consume (remove) on suppress in both receiveSegment and receiveDataTranscript
• Sources/Media/MediaPlayerController.swift — mediaInterruptionSequence + mediaInterruptionTask retained chain, sequence recheck before player.play()
• Sources/Features/Settings/SettingsView.swift — nonoptional @ObservedObject var session: VoiceSession, canApplyServerURL permits .idle + .failed, tearingDown help text, picker views accept nonoptional session, preview updated
• Sources/Features/Conversation/ConversationView.swift — pendingServerURL coalescing, captured-config construction, .tearingDown in status/orb/composer switches
• Tests/TranscriptDedupeTests.swift — 2 new pair tests (P1,D1,P2,D2 and D1,P1,D2,P2), updated consume test for remove semantics
• Tests/PinnedMediaResourceLoaderTests.swift — renamed stale test title

Design Decisions

• .tearingDown vs. keeping .failed: A dedicated state is clearer than a boolean flag because the UI can show distinct "Cleaning up…" copy and the composer shows ProgressView (not Start). The state is non-retry by construction: connect() only accepts .idle/.failed.
• Mic chain as single task: Using one micOpTask for both begin and end (rather than separate begin/end tasks) guarantees total ordering. Each new operation awaits the predecessor, so the chain is strictly sequential. This matches the request's "one retained microphone operation chain" requirement.
• Remove vs. transfer on suppress: Removing the record means the next arrival (regardless of source) starts a fresh pair. This is the correct semantics for "both sources deliver each utterance": P1+D1 form one pair, P2+D2 form another. Transferring ownership would make the second pair's first arrival look like an opposite-source duplicate.
• Coalesce via pendingServerURL: Rather than queuing multiple URLs (which could be surprising), a single latest-wins pending value matches user intent: they applied A then B, and want to end up on B. The intermediate A session is created and immediately replaced — slightly wasteful but correct and simple.
• @ObservedObject nonoptional session: The request explicitly required "directly observe a nonoptional VoiceSession." ConversationView always has a session (created in init), so the optional was unnecessary indirection.

Verification

• XcodeBuildMCP test_sim (scheme Voice_iOS, iPhone 17 Pro simulator): 49 passed, 0 failed, 0 skipped (⏱️ 10.9s). No compiler warnings.
  • Suites: TranscriptDedupeTests (24), MediaReconcileTests (7), ConversationStateTests (5), MediaAPITests (1), PinnedMediaResourceLoaderTests (12)
  • Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T14-12-21-664Z_pid94423_3cc88c6f.log
  • Result bundle: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/result-bundles/test_sim_2026-08-31T14-12-21-664Z_pid94423_d0e637f2.xcresult
• XcodeBuildMCP build_sim: Build succeeded (⏱️ 4.7s). No warnings.
  • Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T14-12-14-582Z_pid94423_62867a1e.log
• git diff --check: no whitespace issues.

Risks And Limitations

• The .tearingDown state transition and mic operation chain are verified by source inspection and the build/test pass, but a full lifecycle test (failure → immediate retry) would require a fake Room seam that does not exist in the current test infrastructure.
• The media interruption sequence recheck is deterministic in source order but requires a device to exercise real AVAudioSession interruptions.
• Server replacement coalescing creates an intermediate session (A) that is immediately replaced by B. This is correct but slightly wasteful; a more optimal design would skip A entirely, but that would require restructuring the onChange handler into a coordinator object.
• The pendingServerURL processing spawns a nested Task within the outer Task. Both are @MainActor so there is no data race, but the nesting makes the control flow harder to follow.

Follow-up For Reviewer

• Verify .tearingDown → .failed transition: confirm that connect() is rejected during .tearingDown and accepted after .failed is published. Check that the old failure task's teardownRoom() cannot race a new start().
• Verify mic chain ordering: trace begin A → end A (in-flight) → begin B. Confirm B awaits A's unmute before muting, and that A's unmute completion sees stale sequence and does not set isMuted = false.
• Verify dedupe consume: P1,D1,P2,D2 with identical text should yield exactly 2 events. D1 consumes the P1 record; P2 finds no record and is accepted; D2 suppresses against P2's new record.
• Verify server coalescing: Apply A while idle → replacement starts. Apply B during replacement → pendingServerURL = B. After A completes, B is processed. Final state: session on B, activeServerURL == B.
• Verify media interruption: begin A (seq=1) → end A/shouldResume (task captures seq=1) → begin B (seq=2, cancels task). The old resume task sees mediaInterruptionSequence != 1 and does not play.