-
-
Notifications
You must be signed in to change notification settings - Fork 722
refactoring to avoid deadlocks/crashing #467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
nit: not a refactor, bug fix. not a crash, just a deadlock/hang Does this throw away the previous in flight transcription if I accidentally queue another? That could potentially trade one class of bug for a more annoying one. I am looking into adding unit tests in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review
I had some time to test this. Here are my notes! Thnx so much for your time, and this great starting point!
- I can replicate the original
crashhang, on demand. Make a long recording (switch to a slow model first, if needed), then deliberately interrupt it to record again, while its transcribing the previous one. - On
main, it spins forever -> force quit - On this branch, the issue is solved
- It adds new issues
- It does not have tests
Remaining Issues
Now, when I interrupt, I see a "recording" UI flicker for 0.1s, and then vanish.
At this point, there is no UI indicating that it is recording, but I suspect it still was, so I continue speaking out loud.
I pressed the button to start a recording but it actually was already recording, so it stops the recording that had been going on in the background, and spews text unexpectedly from audio I was unaware it was recording.
Opus 4.5 Explanation:
actions.rs lines 391-396 - When Operation 1's async task detects it's stale, it calls cleanup which overwrites the UI that Operation 2 just set:
// Operation 1's async task (still running)
if !coordinator.is_active(operation_id) { // Line 391 - true, it's stale
cleanup(&ah, &coordinator, operation_id); // Line 396
return;
}
The cleanup function (lines 384-388):
let cleanup = |ah: &AppHandle, ...| {
utils::hide_recording_overlay(ah); // ← Hides Operation 2's overlay
change_tray_icon(ah, TrayIconState::Idle); // ← Resets Operation 2's tray
coordinator.complete(op_id);
};
Timeline:
1. Op1 starts recording → UI shows recording
2. Op1 stops → spawns async transcription task
3. Op2 starts → UI shows recording (correct)
4. Op1's async task runs cleanup → UI reset to Idle (wrong - overwrites Op2)
Result: Op2 is recording but UI shows nothing.
Open Questions:
I am slightly concerned about landing tests during other active contributions, not sure how we plan to prioritize testing vs adding new features.
I wonder if it needs to account for n-2, in addition to the n-1 recording, etc.
Are we okay with extracting pure functions for the state machine (to facilitate testing). I would advocate for this, once there is a single source of truth and well tested "model", the UI is just a projection of that (and becomes very easy). Essentially loosely following MVC or React like model (one source of state, view layer declaratively renders the state if its different, no imperative updates to UI). This PR is essentially "forking" operations that are long running over time and mutating state imperatively, it will be difficult to get this right with imperative code + no tests.
|
Hey this issue is low priority generally for the moment, and the pr is just scaffolding to remind me to get to it at some point. It's effectively placeholder. It's called a refactor because that's the overall goal of the PR and it happens to fix an issue as well. It's putting a bookmark that I think this code generally needs to be worked on more broadly Right now just contribute what you can and I'll do my best to pull things in when I get a chance. Scoped changes that don't involve core parts of the app make it in much faster. This touches too much for me to consider it in the short term Adding tests is going to be best effort and incremental most likely. You can propose whatever changes you would like to see and if they have good logic behind them they will likely be accepted. The app generally runs on community contributions, I don't have a top down or strict plan for anything except stabilizing core features and fixing the biggest outstanding issues. I am much more focused on the inference and distribution side at the moment as well as trying to sort out keybindings across platforms. If you want to help take on any maintenance and opinions in certain parts of the codebase I am happy to delegate and build trust through quality accepted PRs |
mainly for #462
needs review and testing.