fix(conversation_tab): add resource cleanup when closing conversations#1057
Merged
clementb49 merged 2 commits intomasterfrom Feb 22, 2026
Merged
fix(conversation_tab): add resource cleanup when closing conversations#1057clementb49 merged 2 commits intomasterfrom
clementb49 merged 2 commits intomasterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-tab resource cleanup and widget-liveness guards, makes recording/OCR flows abort-safe, and adds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant MainFrame
participant ConversationTab
participant RecordingThread
participant CompletionHandler
participant OCRHandler
participant BackgroundProcess
MainFrame->>ConversationTab: cleanup_resources()
activate ConversationTab
ConversationTab->>ConversationTab: set _is_destroying = true
ConversationTab->>RecordingThread: stop_recording(abort=True)
RecordingThread-->>ConversationTab: return (callbacks suppressed)
ConversationTab->>CompletionHandler: stop_completion(skip_callbacks=True)
CompletionHandler-->>ConversationTab: stopped (no end-callback)
ConversationTab->>BackgroundProcess: _terminate_process(process, timeout=1.0)
BackgroundProcess-->>ConversationTab: terminated/killed
ConversationTab->>OCRHandler: _terminate_ocr_process_if_needed()
OCRHandler-->>ConversationTab: dialog destroyed / button not re-enabled
ConversationTab->>MainFrame: cleanup complete
deactivate ConversationTab
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
basilisk/recording_thread.py (1)
174-176: 🧹 Nitpick | 🔵 TrivialMissing
_want_abortguard in transcription error path.The success path (Line 169) checks
_want_abortbefore schedulingon_transcription_received, but the error path unconditionally scheduleson_transcription_error. Ifabort()is called whileget_transcription()is in-flight and it raises, this will fire a callback on a potentially destroyed widget. The receiver does have a_is_widget_valid()guard, so this won't crash, but it should be consistent with the other paths.Proposed fix
except BaseException as err: log.error("Error getting transcription: %s", err) + if self._want_abort: + return wx.CallAfter(self.conversation_tab.on_transcription_error, str(err))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@basilisk/recording_thread.py` around lines 174 - 176, The except block handling errors from get_transcription() should mirror the success path and honor the abort flag: check self._want_abort after catching the exception and return without scheduling the UI callback if abort() was requested; only call wx.CallAfter(self.conversation_tab.on_transcription_error, str(err)) when not self._want_abort. Update the error handler in the RecordingThread around get_transcription() to perform this guard (same pattern used for the success path) to avoid scheduling callbacks for aborted/destroyed widgets.basilisk/gui/conversation_tab.py (1)
881-907: 🧹 Nitpick | 🔵 TrivialCompletion callbacks check validity of one widget but access siblings unchecked.
_on_completion_startvalidatessubmit_btnbut then callsself.stop_completion_btn.Show()without validating it. Similarly,_on_completion_endvalidatesstop_completion_btnbut callsself.submit_btn.Enable(). Since both widgets share the same parent lifecycle and are destroyed together, this is unlikely to cause issues in practice, but for consistency with the defensive approach, consider checking validity for all accessed widgets.Example for _on_completion_start
def _on_completion_start(self): """Called when completion starts.""" - if not self._is_widget_valid('submit_btn'): + if not self._is_widget_valid('submit_btn') or not self._is_widget_valid('stop_completion_btn'): return self.submit_btn.Disable() self.stop_completion_btn.Show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@basilisk/gui/conversation_tab.py` around lines 881 - 907, Both completion callbacks currently validate only one widget but then access siblings without checks; update _on_completion_start to validate both submit_btn and stop_completion_btn (use _is_widget_valid('submit_btn') and _is_widget_valid('stop_completion_btn')) before calling submit_btn.Disable() and stop_completion_btn.Show(), and update _on_completion_end to validate both stop_completion_btn and submit_btn before calling stop_completion_btn.Hide() and submit_btn.Enable(); also ensure you validate messages with _is_widget_valid('messages') before calling messages.SetFocus() and keep the existing call to _clear_stored_content() when success is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/gui/conversation_tab.py`:
- Around line 1029-1069: In cleanup_resources, after aborting and join-ing the
recording thread in the recording_block (the block referencing
self.recording_thread.abort() and self.recording_thread.join(...)), clear the
reference by setting self.recording_thread = None (preferably in a finally or
after the try/except) so the RecordingThread and its held references (e.g.,
provider_engine, conversation_tab) can be GC'd promptly; keep the existing log
and exception handling but ensure the reference is nulled even if abort/join
raises.
In `@basilisk/gui/main_frame.py`:
- Around line 406-413: The docstring for _cleanup_all_tabs is inaccurate about
its callers; update it to reflect that this helper cleans up resources for all
conversation tabs by iterating self.tabs_panels and calling cleanup_resources()
on each tab, and that it is used by on_quit (not on_close_conversation, which
only cleans up a single tab); change the wording to remove the incorrect
reference to on_close_conversation and clearly describe the method's purpose and
behavior.
In `@basilisk/gui/ocr_handler.py`:
- Around line 141-161: The completion handler _handle_ocr_completion_message
should mark that cleanup has been performed (e.g. set self._ocr_cleanup_done =
True) immediately after safely destroying dialog and re-enabling the ocr_button
so _cleanup_ocr_process can check this flag and return early; update
_cleanup_ocr_process to first test for the flag (and clear it if appropriate)
before attempting to destroy dialog or enable the button, ensuring dialog
destruction and button enabling happen exactly once and avoiding duplicate
actions.
---
Outside diff comments:
In `@basilisk/gui/conversation_tab.py`:
- Around line 881-907: Both completion callbacks currently validate only one
widget but then access siblings without checks; update _on_completion_start to
validate both submit_btn and stop_completion_btn (use
_is_widget_valid('submit_btn') and _is_widget_valid('stop_completion_btn'))
before calling submit_btn.Disable() and stop_completion_btn.Show(), and update
_on_completion_end to validate both stop_completion_btn and submit_btn before
calling stop_completion_btn.Hide() and submit_btn.Enable(); also ensure you
validate messages with _is_widget_valid('messages') before calling
messages.SetFocus() and keep the existing call to _clear_stored_content() when
success is true.
In `@basilisk/recording_thread.py`:
- Around line 174-176: The except block handling errors from get_transcription()
should mirror the success path and honor the abort flag: check self._want_abort
after catching the exception and return without scheduling the UI callback if
abort() was requested; only call
wx.CallAfter(self.conversation_tab.on_transcription_error, str(err)) when not
self._want_abort. Update the error handler in the RecordingThread around
get_transcription() to perform this guard (same pattern used for the success
path) to avoid scheduling callbacks for aborted/destroyed widgets.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/gui/conversation_tab.py`:
- Around line 596-601: The debug calls that pass exc_info=True when not inside
an exception handler (seen where _is_widget_valid() is checked in
conversation_tab.py and similar code in ocr_handler.py) produce "NoneType: None"
noise; remove exc_info=True from these log.debug/log.error calls (or only pass
exc_info=sys.exc_info() inside an except block) so that stack/exception info is
logged only when an actual exception is being handled.
- Around line 1006-1031: Add a short join after forcing a kill to ensure the
killed process is reaped: in _terminate_process, after the call to
process.kill() (and the log warning "%s did not terminate, killing it"), call
process.join(timeout=0.5) (optionally guarded by hasattr/process.is_alive
checks) so the child is given a brief chance to be reaped before returning; keep
the existing exception handling unchanged.
- Around line 530-534: The fallback branch that reads widget.__class__ is a
no-op and doesn't detect destroyed wx widgets; remove that branch and instead
call the wx-specific access inside a try/except so RuntimeError from a destroyed
C++ peer is caught. Concretely, replace the current hasattr+__class__ fallback
with a single attempt to call widget.GetParent() (or getattr(widget,
"GetParent") and call it) inside a try/except catching RuntimeError (and
AttributeError if GetParent may be missing) so destroyed widgets are properly
detected; reference the widget.GetParent access in conversation_tab.py and
update the surrounding logic to treat a RuntimeError as “invalid/destroyed”
widget.
In `@basilisk/gui/main_frame.py`:
- Around line 406-414: The current _cleanup_all_tabs iterates self.tabs_panels
and calls tab.cleanup_resources(), but if one tab raises an exception it aborts
cleanup for remaining tabs; update _cleanup_all_tabs to call cleanup_resources()
for each tab inside a try/except that catches exceptions per-tab (e.g.,
Exception), logs the error (including which tab instance or tab.id/name) and
continues to the next tab so all tabs get cleaned; ensure any logging uses the
existing logger used by on_quit/main_frame and do not re-raise so quit proceeds
even if some tab cleanup fails.
In `@basilisk/gui/ocr_handler.py`:
- Around line 249-273: The debug log call that reports "Skipping OCR button
enable: parent tab is being destroyed" should not pass exc_info=True because
it's outside an exception handler; edit the log.debug invocation in
ocr_handler.py (the block guarded by hasattr(self.parent, '_is_destroying') and
self.parent._is_destroying) to remove exc_info=True so it logs only the message;
keep the surrounding checks for dialog, dialog.Destroy(), and
ocr_button.Enable() unchanged.
- Around line 149-159: The guard in the OCR completion handler erroneously
treats missing attributes as "destroying" — change the checks on self.parent so
missing attributes default to safe/valid values (e.g., use getattr(self.parent,
'_is_destroying', False) and call getattr(self.parent, '_is_widget_valid',
lambda: True)() so an absent _is_widget_valid yields True), and remove
exc_info=True from the log.debug call; keep the log message but log only
contextual text (no traceback) when skipping handling.
---
Duplicate comments:
In `@basilisk/gui/conversation_tab.py`:
- Around line 1033-1073: After aborting and joining the recording thread in
cleanup_resources, set self.recording_thread = None to match the pattern used
for self.ocr_handler.process and self.process; this ensures the attribute is
cleared after cleanup and prevents future accidental use of a dead thread
reference when methods like _terminate_process or other teardown logic run
(locate the abort block that calls self.recording_thread.abort() / join and add
the assignment immediately after the try/except).
- Around line 636-654: In stop_recording, after calling
self.recording_thread.abort() or .stop(), ensure the RecordingThread reference
is released so it can be GC'd: if the thread object exposes join(), call it
(with a short timeout) to let it finish, then set self.recording_thread = None;
keep the existing UI update logic (toggle_record_btn and submit_btn) but perform
it after the join/clear so no lingering references to provider_engine or
conversation_tab remain.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/gui/conversation_tab.py`:
- Line 1067: The comment misstates that processes are terminated "in parallel"
while the code in conversation_tab.py actually calls _terminate_process
sequentially (each call waits via join); update the comment near the
_terminate_process invocations (in the ConversationTab code) to accurately state
that processes are terminated sequentially or "terminated one-by-one" and note
that they are independent, or alternatively implement true parallel termination
(e.g., dispatch threads/tasks) if parallel behavior is desired—prefer the
simpler fix: change the comment text to reflect sequential termination.
- Around line 1020-1025: The hasattr(process, 'join') guard is redundant because
process.join() was already invoked earlier, so replace the conditional "if
hasattr(process, 'join') and process.is_alive()" with a simple "if
process.is_alive()" to simplify the logic around process.kill() and the
subsequent join; update the block that calls process.kill() and the follow-up
join (referencing the local variable process and the methods process.kill() and
process.join()) accordingly.
- Line 574: Replace all single-quoted string literals with double-quoted
literals in this change-set: update calls like
self._is_widget_valid('prompt_panel') and other occurrences of
'toggle_record_btn', 'submit_btn', 'messages', 'stop_completion_btn',
'is_alive', and 'join' to use double quotes (e.g., "prompt_panel") so they
conform to the project's string literal guideline; search for these symbols and
update them in the methods and uses around conversation_tab.py (including
anywhere _is_widget_valid and widget attribute checks are used).
---
Duplicate comments:
In `@basilisk/gui/conversation_tab.py`:
- Around line 986-999: The call to show_enhanced_error_dialog in the completion
error path should not pass exc_info=True (we want to avoid printing raw
exception stacks in the dialog); locate the completion handler that checks
_is_widget_valid('prompt_panel'), calls _restore_prompt_content() and
_clear_stored_content(), and ensure the show_enhanced_error_dialog invocation
only passes parent=self, message=(... % error_message), title=(...), and
is_completion_error=True—remove any exc_info=True or similar exception-stack
parameters if present in that call or other nearby calls in conversation_tab.py.
In `@basilisk/gui/ocr_handler.py`:
- Around line 157-166: The hasattr guards are inverted: currently missing
attributes cause early return; change the condition to only skip when the parent
explicitly signals invalidation—i.e., check if getattr(self.parent,
"_is_destroying", False) is True OR (hasattr(self.parent, "_is_widget_valid")
and not self.parent._is_widget_valid()). Update the if in the OCR completion
handler to use getattr(self.parent, "_is_destroying", False) and the hasattr +
negation for _is_widget_valid so absent attributes are treated as “valid” and
valid completions are not skipped; keep the log.debug call and return path the
same when the combined condition is true.
| # Stop any progress sounds | ||
| stop_sound() | ||
|
|
||
| # Terminate all processes in parallel (they're independent) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading comment: processes are terminated sequentially, not in parallel.
The comment says "in parallel" but _terminate_process calls are sequential and blocking (each waits via join). The comment likely means "they're independent of each other," which is true — but "in parallel" is inaccurate.
Suggested comment fix
- # Terminate all processes in parallel (they're independent)
+ # Terminate background processes (they're independent of each other)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Terminate all processes in parallel (they're independent) | |
| # Terminate background processes (they're independent of each other) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/gui/conversation_tab.py` at line 1067, The comment misstates that
processes are terminated "in parallel" while the code in conversation_tab.py
actually calls _terminate_process sequentially (each call waits via join);
update the comment near the _terminate_process invocations (in the
ConversationTab code) to accurately state that processes are terminated
sequentially or "terminated one-by-one" and note that they are independent, or
alternatively implement true parallel termination (e.g., dispatch threads/tasks)
if parallel behavior is desired—prefer the simpler fix: change the comment text
to reflect sequential termination.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/gui/main_frame.py`:
- Around line 650-658: Wrap the call to current_tab.cleanup_resources() in a
try/except (or try/except/finally) so any exceptions raised during cleanup are
caught and logged, and always proceed to remove the page and pop from
self.tabs_panels; specifically, call current_tab.cleanup_resources() inside a
try block, catch Exception as e and log it, and ensure
notebook.DeletePage(current_tab_index) and
self.tabs_panels.pop(current_tab_index) run regardless (use finally or place
removal after the except) to avoid leaving a zombie tab; reference
cleanup_resources, current_tab, current_tab_index, notebook.DeletePage,
self.tabs_panels.pop and mirror the behavior used by _cleanup_all_tabs().
---
Duplicate comments:
In `@basilisk/gui/main_frame.py`:
- Around line 406-426: The review indicates no code changes are required for
_cleanup_all_tabs: the per-tab try/except with index-based logging and the
docstring are correct; no edits needed—leave function _cleanup_all_tabs and its
error logging (log.error with exc_info=True) as-is and proceed to merge.
AAClause
force-pushed
the
cleanup_conversation_tab
branch
from
4ecfcd8 to
2de1f9c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@basilisk/gui/conversation_tab.py`:
- Line 499: The function _is_widget_valid currently has a default None for
widget_name but is annotated as str; update its type annotation to Optional[str]
(or use str | None) to match the default and docstring, e.g., change the
signature of _is_widget_valid to use Optional[str] for widget_name so static
typing and runtime behavior align (Optional is already imported).
---
Duplicate comments:
In `@basilisk/gui/conversation_tab.py`:
- Line 1067: The inline comment "Terminate all processes in parallel (they're
independent)" is inaccurate because the calls to _terminate_process are blocking
(they join sequentially); update the comment near the _terminate_process calls
in conversation_tab.py to reflect sequential termination (e.g., "Terminate
background processes (they're independent of each other)" or "Terminate
background processes sequentially") so the comment matches the actual behavior.
| else: | ||
| dlg.Destroy() | ||
|
|
||
| def _is_widget_valid(self, widget_name: str = None) -> bool: |
Contributor
There was a problem hiding this comment.
Type hint mismatch: widget_name accepts None but is annotated as str.
The default value is None, but the type hint declares str. This should be Optional[str] (already imported) or str | None to match the actual usage and the docstring.
Proposed fix
- def _is_widget_valid(self, widget_name: str = None) -> bool:
+ def _is_widget_valid(self, widget_name: Optional[str] = None) -> bool:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _is_widget_valid(self, widget_name: str = None) -> bool: | |
| def _is_widget_valid(self, widget_name: Optional[str] = None) -> bool: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@basilisk/gui/conversation_tab.py` at line 499, The function _is_widget_valid
currently has a default None for widget_name but is annotated as str; update its
type annotation to Optional[str] (or use str | None) to match the default and
docstring, e.g., change the signature of _is_widget_valid to use Optional[str]
for widget_name so static typing and runtime behavior align (Optional is already
imported).
Add proper cleanup of all running resources when closing conversation tabs or the application to prevent resource leaks, orphaned processes, and RuntimeErrors from accessing destroyed widgets. Resource cleanup improvements: - Add cleanup_resources() method to ConversationTab for comprehensive cleanup - Clean up completion handlers with skip_callbacks to prevent widget access - Abort recording threads (instead of just stopping) to prevent transcription callbacks - Stop progress sounds when closing tabs - Terminate OCR processes gracefully with timeout and kill fallback - Terminate background processes with proper error handling - Add _is_destroying flag to prevent callbacks from accessing destroyed widgets - Factorize cleanup logic with _cleanup_all_tabs() helper in MainFrame - Call cleanup_resources() in on_close_conversation() and on_quit() - Create _terminate_process() helper to factorize process termination logic Defensive callback handling: - Make all completion callbacks defensive (_on_completion_start, _on_completion_end, _on_completion_error, _on_stream_chunk, _on_stream_start, _on_stream_finish, _on_non_stream_finish) - check _is_destroying flag and widget existence - Make all recording callbacks defensive (on_recording_started, on_recording_stopped, on_transcription_started, on_transcription_received, on_transcription_error) - Make OCR callbacks defensive (_handle_ocr_completion_message, _cleanup_ocr_process) - Update RecordingThread to check _want_abort before scheduling callbacks - Add skip_callbacks parameter to CompletionHandler.stop_completion() - Improve stop_recording() with abort parameter and defensive UI updates
AAClause
force-pushed
the
cleanup_conversation_tab
branch
2 times, most recently
from
7c57a3e to
2eca29b
Compare
add stack traces to exception-related logs
Clear the recording_thread reference after aborting and joining the thread
in cleanup_resources() to allow proper garbage collection of the RecordingThread
and its held references (provider_engine, conversation_tab).
Fix _is_widget_valid() method to properly detect destroyed wx widgets by
removing the ineffective __class__ fallback and directly calling GetParent()
to trigger RuntimeError when the C++ peer has been destroyed.
Remove exc_info=True from log calls outside exception handlers
- Remove exc_info=True from log.debug/log.error calls that are not inside
exception handlers to prevent "NoneType: None" noise in logs
- Fixed in on_transcription_error(), _on_completion_error(),
_handle_ocr_completion_message(), and _enable_ocr_button()
- All remaining exc_info=True usages are correctly inside exception handlers
Add join() after kill() in process termination
- Add process.join(timeout=0.5) after process.kill() in _terminate_process()
to ensure killed processes are reaped before returning
- Prevents zombie processes from accumulating
- Guarded with hasattr and is_alive checks for safety
- Uses short timeout to avoid blocking
Add error handling to _cleanup_all_tabs
AAClause
force-pushed
the
cleanup_conversation_tab
branch
from
2eca29b to
d44dabf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Improvements