Skip to content

fix(coordinator): clear focus_target on cancel regardless of phase - #387

Merged
H-Chris233 merged 1 commit into
betafrom
fix/audit-focus-target-leak
May 9, 2026
Merged

fix(coordinator): clear focus_target on cancel regardless of phase#387
H-Chris233 merged 1 commit into
betafrom
fix/audit-focus-target-leak

Conversation

@appergb

@appergbappergb commented May 9, 2026

Copy link
Copy Markdown
Collaborator

User description

Summary

`finish_cancel_session_state` only cleared `state.focus_target` when the cancelled phase was not `Processing`. A Processing-phase cancel therefore left the stale window-resource handle in place until the next `begin_session_state` overwrote it. Between cancel and the next `begin_session`, any reader of `focus_target` sees a value belonging to a session that is about to be torn down.

Why this matters

Today the leak is bounded — `begin_session_state` always overwrites `focus_target` (`coordinator_state.rs:80`), and there is no documented reader that races with that interval. So this isn't user-visible today. But the asymmetry is a footgun: anyone wiring a new code path that consults `focus_target` between cancel and next session would silently inherit the stale handle.

Change

Move `state.focus_target = None` ahead of the phase check so it always fires when the cancel was accepted (i.e. not rejected for `Idle` / `Inserting` phases). Phase transition stays conditional on the original non-Processing branch — `end_session` must still own the `Processing → Idle` collapse to avoid racing with the polish/insert tail of `Processing`.

Audit linkage

Audit ID 3.3.5 (CONFIRMED). See `docs/audit-2026-05-10-validated.md` (local) for full validation context.

Test plan

  • `cargo test --manifest-path openless-all/app/src-tauri/Cargo.toml --lib coordinator_state` — 9/9 passing.
  • Tightened `cancel_session_state_machine_is_table_driven` to assert `focus_target` clears for every accepted phase (including Processing) and stays untouched on rejected cancels (Idle / Inserting). Test fails on the old code (regression-guard).
  • CI build — to be verified

Pure state-machine bookkeeping fix; no runtime behavior change in any documented path. Awaiting manual verification of cancel-during-polish on a real build.


PR Type

Bug fix, Tests


Description

  • Move focus_target clearing before phase check

  • Retain conditional phase transition for Processing

  • Assert focus_target cleared after accepted cancels

  • Assert focus_target unchanged after rejected cancels


Diagram Walkthrough

flowchart LR
A["begin_cancel_session_state"] --> B{"Phase accepted?"}
B -- "No (Idle/Inserting)" --> C["Return None"]
B -- "Yes" --> D["finish_cancel_session_state"]
D --> E["Clear focus_target"]
E --> F{"Phase != Processing?"}
F -- "Yes" --> G["Set phase to Idle"]
F -- "No" --> H["Leave phase for end_session"]
Loading

File Walkthrough

Relevant files
Bug fix
coordinator_state.rs
Clear focus_target on cancel in all phases

openless-all/app/src-tauri/src/coordinator_state.rs

  • Move state.focus_target = None before the phase check so it clears on
    every accepted cancel (including Processing)
  • Update doc comment to explain why focus_target must always be
    released, referencing audit 3.3.5
  • Enhance cancel_session_state_machine_is_table_driven test to assert
    focus_target is None after any accepted cancel and remains untouched
    on rejected cancels
+18/-4

finish_cancel_session_state previously only cleared state.focus_target
when the cancelled phase was not Processing — Processing-phase cancel
left the stale window-resource handle in place until the next
begin_session_state overwrote it. Between cancel and next begin, any
reader of focus_target sees a value belonging to a session that is
about to be torn down.
Move state.focus_target = None ahead of the phase check so it always
fires when the cancel was accepted (i.e. not rejected for Idle /
Inserting phases). Phase transition stays conditional on the original
non-Processing branch, since end_session must still own the
Processing → Idle collapse to avoid racing with polish/insert.
Tighten cancel_session_state_machine_is_table_driven so it asserts
focus_target gets cleared for every accepted phase (including
Processing) and stays untouched on rejected cancels.
Audit ID 3.3.5.
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

@H-Chris233
H-Chris233 merged commit 13bdf52 into betaMay 9, 2026
4 checks passed
pullBot pushed a commit to yimmy23/openless that referenced this pull request May 10, 2026
Two real bugs surfaced by the 2026-05-10 end-to-end logic review of
beta after the 8-PR audit-fix wave landed.
1. coordinator.rs:2313 — `acquire_recording_mute(inner, "qa");` was
missing `.await`. PR Open-Less#391 (`6171df61`) made the function `async fn`
and updated the dictation call site at coordinator/dictation.rs:451,
but the QA call site was missed. Effect: when a user has
`mute_during_recording = true` and triggers QA via Right Option, the
returned Future is dropped on the next line, `spawn_blocking` is
never scheduled, holders never increments, and system audio is NOT
muted (e.g. YouTube playback continues into the QA recording). Both
the matching `release_recording_mute(inner, "qa")` calls become
no-ops (early return at holders == 0). The compiler was emitting
`unused_must_use` for this site (verified before this commit).
Fix: add `.await`.
2. coordinator/dictation.rs:843-849 — PR Open-Less#387 (`ce82fcd9`) was framed
as "clear focus_target on cancel regardless of phase", but the only
code path that gained the unconditional clear was
`finish_cancel_session_state`. cancel_session deliberately skips
that helper for Processing (so end_session can drive its own
teardown), and end_session's "ASR-finished, cancelled" exit at
dictation.rs:843-849 set phase=Idle but never touched focus_target.
Result: cancel-during-Processing leaves a stale `usize` focus slot
in state.focus_target until the next begin_session_state overwrites
it. Today the leak is bounded (no documented reader between cancel
and next begin), but it violates PR Open-Less#387's stated contract and is a
silent footgun for any future reader of focus_target on that
interval. Fix: in the cancelled-after-ASR exit branch, take the
state lock once and clear both phase and focus_target together.
Source: docs/logic-review-2026-05-10.md (subagent end-to-end review).
185/185 lib tests pass; cargo check clean (unused_must_use gone).
Manual verification checklist for both fixes in the review doc.
pullBot pushed a commit to yimmy23/openless that referenced this pull request May 10, 2026
10 PRs landed on beta this cycle:
- Open-Less#377 paste shortcut configurable (issue Open-Less#360)
- Open-Less#386 TS UserPreferences updateChannel alignment
- Open-Less#387 focus_target leak on Processing-phase cancel
- Open-Less#388 [严重] MacHotkeyAdapter::shutdown stops CFRunLoop + tap
- Open-Less#389 emit_capsule window.show/hide off audio thread
- Open-Less#390 QA / dictation hotkey routing race
- Open-Less#391 audio-mute spawn_blocking (async hygiene)
- Open-Less#392 hotkey supervisor + global dispatcher exit signal
- Open-Less#393 post-audit logic-review hotfixes (QA mute .await + focus_target Processing branch)
- Open-Less#394 in-process credentials cache (kills repeated Keychain prompts)
Bump 4 files: package.json, tauri.conf.json, Cargo.toml, Cargo.lock.
@appergb
appergb deleted the fix/audit-focus-target-leak branch May 10, 2026 10:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@appergb@H-Chris233