Skip to content

Rewrite SessionController's concurrency model onto coroutines/StateFlow #8

Description

@NITISH-R-G

Objective

Replace SessionController's five hand-coordinated Handler threads (bg, capture, speech, plus main, plus the accessibility service's own thread) and manual @Volatile/turn-id state with structured concurrency (Kotlin coroutines + StateFlow/Mutex), without changing the frozen OverlayCommand/planner contracts or any of the five core pieces.

Rationale

Flagged by the repo audit as the highest-maintainability-risk code in the project. The turn-invalidation logic (newTurn(), isCurrent(), recordingTurn) is correct today because it was carefully hand-verified this session, but every new feature added to this class (app-choice state, chosenPackage, currentChoices, the newer open_ended/AppLauncher path) grows the same unstructured state space. This is not a bug report - nothing is currently broken - it's a bet against future regressions being introduced silently.

Acceptance criteria

  • No Handler/HandlerThread remains in SessionController - replaced by coroutines on appropriately-scoped dispatchers
  • Turn invalidation (a mic-tap during "Thinking..." must not let a stale plan repaint the pill) is expressed via Job cancellation, not manual turn-id comparison
  • Every existing behavior verified in this session's manual device testing still holds: stop-mid-recording, stop-and-resume preserving step position, cursor/speech running in parallel with bounds resolution, debug panel not clobbering itself
  • All existing unit tests pass unmodified or are updated to test the same behavior through the new concurrency model
  • No change to contracts/*.schema.json, OverlayCommand, or PlannerResult

Implementation steps

  1. Introduce a viewModelScope-equivalent CoroutineScope owned by SessionController, cancelled in dispose()
  2. Model a "turn" as a cancellable coroutine Job rather than an AtomicInteger comparison
  3. Move WavRecorder start/stop behind a Mutex instead of a dedicated Handler thread
  4. Move TTS synthesis + playback onto its own dispatcher via withContext(Dispatchers.IO), preserving the "speech never blocks the visual path" property
  5. Re-run every manual verification scenario from this session's transcript (mic-tap-during-thinking, Stop mid-utterance, taxi app-choice flow) on a physical device before merging

Estimated effort

High (3-5 days for one engineer familiar with the codebase, including manual device re-verification of every documented scenario)

Risks

  • This is the single riskiest change achievable in this codebase - it touches the class every other feature depends on
  • Coroutine cancellation semantics differ subtly from manual turn-id checks; a rushed port could reintroduce the exact stale-turn bug this session fixed (PR Make a voice turn stale-safe and actually observable #2)
  • No instrumented tests exist today to catch a regression automatically - this PR should add some as part of the work, not just port the logic

Testing checklist

  • Unit tests for turn cancellation semantics
  • Manual device test: mic-tap, then mic-tap again during "Thinking..." - pill must stay on the second turn, first turn's plan must never render
  • Manual device test: Stop mid-recording, then mic-tap again - must resume from the preserved step, not restart
  • Manual device test: full taxi flow (choose app -> launch -> guide inside it) end to end
  • Regression: keyless deterministic fallback path still works with no Sarvam key configured

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    audit-followupFlagged by the repo audit; too large/risky for a small PRenhancementNew feature or request

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions