Skip to content

Prevent possible deadlocks when pressing Escape to cancel recordings#408

Merged
cjpais merged 3 commits intocjpais:mainfrom
dannysmith:fix/toggle-mode-deadlock
Dec 18, 2025
Merged

Prevent possible deadlocks when pressing Escape to cancel recordings#408
cjpais merged 3 commits intocjpais:mainfrom
dannysmith:fix/toggle-mode-deadlock

Conversation

@dannysmith
Copy link
Collaborator

So #224 allows the Escape key to be used to cancel recording. My #401 basically implemented this in a similar (though more generic way) before 224 was merged. Looking at this commit in 401, I think the only thing worth keeping from my implementation is this...

Problem

In toggle mode (non-push-to-talk), the shortcut callback holds the ManagedToggleState
lock while calling action.start() or action.stop(). If the action needs to acquire the
same lock (e.g., cancel_current_operation resets toggle states), this causes a
deadlock.

Solution

Release the lock before calling the action:

let should_start: bool;
{
    let mut states = toggle_state_manager.lock()...;
    should_start = !*is_currently_active;
    *is_currently_active = should_start;
} // Lock released

if should_start { action.start(...) } else { action.stop(...) }

Steps after this PR is merged

@cjpais If you'd prefer the slightly more generic approach to handling dynamic keybindings in 2915aea I can also add that to this PR too.

@cjpais
Copy link
Owner

cjpais commented Dec 5, 2025

I really liked the dynamic keybindings bit when I reviewed from what I recall if you could bring that refactor in too it would be great. I think it starts to set us up a lot better for moving to letting users define all kinds of keybindings if they want

@dannysmith
Copy link
Collaborator Author

#224 includes dynaming keybinding already, but not in an easily reusable way. I'll bring that stuff over into here from #401 because yeah - I agree. Will ping you when I ned a review

@cjpais
Copy link
Owner

cjpais commented Dec 18, 2025

I had claude port over the dynamic bindings, I haven't reviewed honestly. It does work from my brief testing, but would be helpful to have your review as well. I will take a closer look at the code itself when I can. If I just messed things up, feel free to revert the changes

@cjpais
Copy link
Owner

cjpais commented Dec 18, 2025

Im running into some issues while testing the dynamic part. I think maybe it would be a nice thing to bring in but I'd rather bring in the fix now rather than later so I'm going to revert to your merge, re-merge, test and pull in

@cjpais cjpais force-pushed the fix/toggle-mode-deadlock branch from a291676 to 71f837a Compare December 18, 2025 14:07
@cjpais cjpais merged commit 3f401d9 into cjpais:main Dec 18, 2025
2 checks passed
@jacksongoode
Copy link
Contributor

Thanks for the PR! This implementation definitely catches a major flaw.

melnikov-s pushed a commit to melnikov-s/handy-ramble that referenced this pull request Jan 25, 2026
…jpais#408)

* PRevent possible deadlocks when pressing Escape to cancel recordings

* same for sigusr2

---------

Co-authored-by: CJ Pais <[email protected]>
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.

3 participants