Skip to content

refactor(agent): arbitrate receiver access with leases#302

Open
AprilNEA wants to merge 2 commits into
masterfrom
refactor/hid-receiver-lease
Open

refactor(agent): arbitrate receiver access with leases#302
AprilNEA wants to merge 2 commits into
masterfrom
refactor/hid-receiver-lease

Conversation

@AprilNEA

Copy link
Copy Markdown
Owner

Summary

  • replace pairing/capture coordination atomics with an explicit ReceiverAccess lease arbiter
  • make capture opportunistically hold receiver ownership and make pairing await exclusive ownership before starting
  • release pairing ownership only on terminal watcher events or watcher shutdown, and surface startup failures instead of opening the receiver concurrently

Validation

  • cargo fmt --check
  • cargo test -p openlogi-agent-core receiver_access::tests
  • cargo test -p openlogi-agent pairing::tests
  • cargo test -p openlogi-agent-core -p openlogi-agent
  • cargo clippy -p openlogi-agent-core -p openlogi-agent --all-targets -- -D warnings

@AprilNEA AprilNEA force-pushed the refactor/hid-receiver-lease branch from 51d2b04 to 1d2455c Compare June 21, 2026 08:30
@greptile-apps

greptile-apps Bot commented Jun 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces two ad-hoc Arc<AtomicBool> fields (pairing_active / capture_idle) in SharedRuntime with an explicit ReceiverAccess lease arbiter that enforces exclusive HID node access between capture and pairing. PairingUpdate::Failed is also upgraded from an opaque String to a typed PairingFailure enum, enabling the GUI to render localized copy and choose recovery UI per failure kind, with the protocol version bumped to 5.

  • ReceiverAccess (receiver_access.rs): introduces CaptureReceiverLease and PairingReceiverLease RAII guards backed by a tokio::Mutex; try_acquire_for_capture uses a double-check pattern around pairing_requested to correctly yield to pairing under all orderings; acquire_for_pairing uses a drop-guarded PairingRequest so timeout cancellation automatically withdraws the request.
  • pairing.rs: start() now acquires the exclusive pairing lease before sending Control::Start, surfaces startup failures (ReceiverBusy, WatcherUnavailable, ReceiverAccessUnavailable) as typed events, and uses a SessionAdmission RAII guard to reset the session counter on any early-exit path.
  • gesture.rs: the want == current short-circuit is preserved; the new current.is_some() branch defers re-acquisition for one tick after stopping the old session, giving the spawned task time to release its _receiver_lease.

Confidence Score: 4/5

Safe to merge; the coordination logic is sound for all normal and cancellation paths, and the only gap requires a mutex to be poisoned during a trivially short critical section.

The refactoring correctly handles all race paths: the double-check in try_acquire_for_capture, cancellation-safe PairingRequest drop, SessionAdmission rollback, and typed failure propagation all look correct. The one gap is that receiver_lease.lock() in the terminal-event handler silently skips lease release if the mutex is poisoned, keeping pairing_requested stuck at true and blocking all future capture. Recovering from a StdMutex poison with PoisonError::into_inner() would make both paths fully robust.

crates/openlogi-agent/src/pairing.rs — specifically the terminal-event handler in translate() and the watcher-shutdown cleanup just below it.

Important Files Changed

Filename Overview
crates/openlogi-agent-core/src/receiver_access.rs New arbiter module — double-check pattern in try_acquire_for_capture, RAII lease types, and cancellation-safe acquire_for_pairing are all correct; tests cover the key races.
crates/openlogi-agent/src/pairing.rs Session coordination reworked cleanly; SessionAdmission correctly resets the counter on failure; lease release via let-chain skips silently on a poisoned mutex in the terminal-event handler.
crates/openlogi-agent-core/src/watchers/gesture.rs Replaced two atomics with ReceiverAccess; want==current short-circuit preserved; deferred restart on current.is_some() correctly waits a tick for the spawned task to release its lease.
crates/openlogi-agent-core/src/ipc.rs PairingFailure enum is append-only and correctly maps from PairingError; protocol version bumped to 5; wire format tests updated with new goldens.
crates/openlogi-gui/src/windows/add_device.rs pairing_failure_text covers all PairingFailure variants exhaustively; user-facing strings are localized via tr!.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant GUI
    participant PairingManager
    participant ReceiverAccess
    participant GestureWatcher
    participant PairingWatcher

    GUI->>PairingManager: start_pairing()
    PairingManager->>ReceiverAccess: "acquire_for_pairing() [sets pairing_requested=true]"
    GestureWatcher->>ReceiverAccess: pairing_requested()? → true
    GestureWatcher->>GestureWatcher: stop capture session (drops CaptureReceiverLease)
    ReceiverAccess-->>PairingManager: PairingReceiverLease granted
    PairingManager->>PairingManager: store lease in receiver_lease slot
    PairingManager->>PairingWatcher: Control::Start(selector)
    PairingWatcher-->>PairingManager: PairingEvent::Paired / Failed
    PairingManager->>PairingManager: translate() drops PairingReceiverLease
    Note over ReceiverAccess: pairing_requested=false (via Drop)
    GestureWatcher->>ReceiverAccess: try_acquire_for_capture() → CaptureReceiverLease
    GestureWatcher->>GestureWatcher: resume capture session
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant GUI
    participant PairingManager
    participant ReceiverAccess
    participant GestureWatcher
    participant PairingWatcher

    GUI->>PairingManager: start_pairing()
    PairingManager->>ReceiverAccess: "acquire_for_pairing() [sets pairing_requested=true]"
    GestureWatcher->>ReceiverAccess: pairing_requested()? → true
    GestureWatcher->>GestureWatcher: stop capture session (drops CaptureReceiverLease)
    ReceiverAccess-->>PairingManager: PairingReceiverLease granted
    PairingManager->>PairingManager: store lease in receiver_lease slot
    PairingManager->>PairingWatcher: Control::Start(selector)
    PairingWatcher-->>PairingManager: PairingEvent::Paired / Failed
    PairingManager->>PairingManager: translate() drops PairingReceiverLease
    Note over ReceiverAccess: pairing_requested=false (via Drop)
    GestureWatcher->>ReceiverAccess: try_acquire_for_capture() → CaptureReceiverLease
    GestureWatcher->>GestureWatcher: resume capture session
Loading

Fix All in Codex Fix All in Claude Code

Reviews (1): Last reviewed commit: "refactor(ipc): type pairing failure reas..." | Re-trigger Greptile

Comment on lines +224 to 228
if sessions.fetch_sub(1, Ordering::Relaxed) == 1
&& let Ok(mut lease) = receiver_lease.lock()
{
*lease = None;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Lease not released on poisoned mutex in terminal-event handler

The let-chain short-circuits on Err from receiver_lease.lock(), so if the StdMutex is poisoned the PairingReceiverLease is never dropped, pairing_requested stays true permanently, and all future capture sessions are blocked. While a mutex poison requires a panic in another thread during the critical section (effectively impossible for a bare *slot = None assignment), the consequence — capture deadlocked until the agent restarts — is severe enough to warrant defensive recovery. The watcher-shutdown path just below (if let Ok(mut lease) = receiver_lease.lock()) has the same gap: if the lock is poisoned there, the lease also stays held.

Fix in Codex Fix in Claude Code

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.

1 participant