# 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.