fix: handle Windows shutdown cleanup#136
Conversation
Co-Authored-By: Warp <agent@warp.dev>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
call_previous_wndproc, ifprevious_wndprocis 0 you currently return 0; consider falling back toDefWindowProcWinstead so you don’t accidentally swallow messages when no previous proc is installed. - In
install, treating aSetWindowLongPtrWreturn value of 0 as a hard failure may conflate a real error with a valid (but rare) address; if you need to distinguish, you may want to checkGetLastErrorbefore logging the install as failed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `call_previous_wndproc`, if `previous_wndproc` is 0 you currently return 0; consider falling back to `DefWindowProcW` instead so you don’t accidentally swallow messages when no previous proc is installed.
- In `install`, treating a `SetWindowLongPtrW` return value of 0 as a hard failure may conflate a real error with a valid (but rare) address; if you need to distinguish, you may want to check `GetLastError` before logging the install as failed.
## Individual Comments
### Comment 1
<location path="src-tauri/src/windows_shutdown.rs" line_range="147-68" />
<code_context>
+ }
+ }
+
+ unsafe fn call_previous_wndproc(
+ hwnd: HWND,
+ msg: u32,
+ wparam: WPARAM,
+ lparam: LPARAM,
+ ) -> LRESULT {
+ let previous = SHUTDOWN_HOOK
+ .get()
+ .and_then(|hook| hook.lock().ok().map(|guard| guard.previous_wndproc))
+ .unwrap_or_default();
+ if previous == 0 {
+ return 0;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Fall back to DefWindowProcW instead of returning 0 when no previous WndProc is available.
If `previous_wndproc` is ever 0 (e.g., due to initialization issues), this helper returns 0 and skips default window handling. To preserve normal behavior and make this more resilient to partial initialization or future hook changes, it would be better to call `DefWindowProcW` instead.
Suggested implementation:
```rust
if previous == 0 {
// Fall back to the default window procedure when no previous WndProc is available.
return DefWindowProcW(hwnd, msg, wparam, lparam);
}
```
1. Ensure `DefWindowProcW` is imported at the top of `src-tauri/src/windows_shutdown.rs` from the same Windows crate used for `HWND`, `WPARAM`, `LPARAM`, and `LRESULT`. For example, if you are using `windows-sys` elsewhere in this file, add:
```rust
use windows_sys::Win32::UI::WindowsAndMessaging::DefWindowProcW;
```
or, if using the `windows` crate:
```rust
use windows::Win32::UI::WindowsAndMessaging::DefWindowProcW;
```
2. Keep the import grouped consistently with existing Windows API imports in this file (respecting your current style/order).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Windows-specific shutdown handler to gracefully and quickly stop the backend process when a system shutdown is requested. The feedback focuses on improving the implementation by ensuring WM_QUERYENDSESSION is correctly propagated down the window procedure chain instead of returning early, and optimizing performance by replacing the mutex lock in the window procedure hot path with a global atomic variable (AtomicIsize).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| WM_QUERYENDSESSION => { | ||
| handle_query_end_session(); | ||
| 1 | ||
| } |
There was a problem hiding this comment.
In shutdown_wndproc, returning 1 directly for WM_QUERYENDSESSION bypasses the rest of the window procedure subclass chain (including Tauri's default window procedure and any other plugins). This can prevent other components from performing necessary cleanup or correctly handling the shutdown query.
Instead, you should call the previous window procedure and return its result so that the message propagates down the chain.
| WM_QUERYENDSESSION => { | |
| handle_query_end_session(); | |
| 1 | |
| } | |
| WM_QUERYENDSESSION => { | |
| handle_query_end_session(); | |
| call_previous_wndproc(hwnd, msg, wparam, lparam) | |
| } |
| #[derive(Default)] | ||
| struct ShutdownHookState { | ||
| app_handle: Option<AppHandle>, | ||
| previous_wndproc: isize, | ||
| cleanup_started: bool, | ||
| } | ||
|
|
||
| static SHUTDOWN_HOOK: OnceLock<Mutex<ShutdownHookState>> = OnceLock::new(); |
There was a problem hiding this comment.
Locking the SHUTDOWN_HOOK mutex on every single window message in call_previous_wndproc introduces significant performance overhead on the main UI thread, especially during high-frequency events like mouse movement or rendering.
Since previous_wndproc is immutable once set, you can store it in a global AtomicIsize and load it using Ordering::Relaxed to completely avoid mutex locking in the hot path.
| #[derive(Default)] | |
| struct ShutdownHookState { | |
| app_handle: Option<AppHandle>, | |
| previous_wndproc: isize, | |
| cleanup_started: bool, | |
| } | |
| static SHUTDOWN_HOOK: OnceLock<Mutex<ShutdownHookState>> = OnceLock::new(); | |
| #[derive(Default)] | |
| struct ShutdownHookState { | |
| app_handle: Option<AppHandle>, | |
| cleanup_started: bool, | |
| } | |
| static SHUTDOWN_HOOK: OnceLock<Mutex<ShutdownHookState>> = OnceLock::new(); | |
| static PREVIOUS_WNDPROC: std::sync::atomic::AtomicIsize = std::sync::atomic::AtomicIsize::new(0); |
| if guard.previous_wndproc != 0 { | ||
| return; | ||
| } | ||
|
|
||
| let previous = unsafe { SetWindowLongPtrW(hwnd, GWLP_WNDPROC, shutdown_wndproc as isize) }; | ||
| if previous == 0 { | ||
| append_shutdown_log("Windows shutdown handler install returned empty previous WndProc"); | ||
| return; | ||
| } | ||
| guard.previous_wndproc = previous; | ||
| append_shutdown_log("Windows shutdown handler installed"); |
There was a problem hiding this comment.
Update the install function to use the new PREVIOUS_WNDPROC atomic variable instead of the mutex-protected previous_wndproc field.
| if guard.previous_wndproc != 0 { | |
| return; | |
| } | |
| let previous = unsafe { SetWindowLongPtrW(hwnd, GWLP_WNDPROC, shutdown_wndproc as isize) }; | |
| if previous == 0 { | |
| append_shutdown_log("Windows shutdown handler install returned empty previous WndProc"); | |
| return; | |
| } | |
| guard.previous_wndproc = previous; | |
| append_shutdown_log("Windows shutdown handler installed"); | |
| if PREVIOUS_WNDPROC.load(std::sync::atomic::Ordering::Relaxed) != 0 { | |
| return; | |
| } | |
| let previous = unsafe { SetWindowLongPtrW(hwnd, GWLP_WNDPROC, shutdown_wndproc as isize) }; | |
| if previous == 0 { | |
| append_shutdown_log("Windows shutdown handler install returned empty previous WndProc"); | |
| return; | |
| } | |
| PREVIOUS_WNDPROC.store(previous, std::sync::atomic::Ordering::Relaxed); | |
| append_shutdown_log("Windows shutdown handler installed"); |
| let previous = SHUTDOWN_HOOK | ||
| .get() | ||
| .and_then(|hook| hook.lock().ok().map(|guard| guard.previous_wndproc)) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Update call_previous_wndproc to load the previous window procedure from the PREVIOUS_WNDPROC atomic variable, completely removing the mutex lock from the hot path.
let previous = PREVIOUS_WNDPROC.load(std::sync::atomic::Ordering::Relaxed);Co-Authored-By: Warp <agent@warp.dev>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
shutdown_wndproc, theWM_ENDSESSIONbranch withwparam != 0callsstd::process::exit(0)without forwarding the message to the previous window procedure, which may bypass any cleanup or state handling that Tauri or other hooks rely on; consider delegating to the previous WndProc before or after your cleanup. - The shutdown handling in
handle_query_end_sessionsynchronously callsstop_backend_with_timeout, which can block the UI thread for up toSYSTEM_SHUTDOWN_STOP_TIMEOUT_MS; consider performing the backend stop on a separate thread to avoid stalling the message loop during system shutdown. - When the
SHUTDOWN_HOOKmutex is poisoned (both ininstalland cleanup paths), the code only logs and then bails; depending on how critical this hook is, you might want to recover by reinitializing the state or falling back to a safe default rather than silently skipping future shutdown handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `shutdown_wndproc`, the `WM_ENDSESSION` branch with `wparam != 0` calls `std::process::exit(0)` without forwarding the message to the previous window procedure, which may bypass any cleanup or state handling that Tauri or other hooks rely on; consider delegating to the previous WndProc before or after your cleanup.
- The shutdown handling in `handle_query_end_session` synchronously calls `stop_backend_with_timeout`, which can block the UI thread for up to `SYSTEM_SHUTDOWN_STOP_TIMEOUT_MS`; consider performing the backend stop on a separate thread to avoid stalling the message loop during system shutdown.
- When the `SHUTDOWN_HOOK` mutex is poisoned (both in `install` and cleanup paths), the code only logs and then bails; depending on how critical this hook is, you might want to recover by reinitializing the state or falling back to a safe default rather than silently skipping future shutdown handling.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-Authored-By: Warp <agent@warp.dev>
|
@sourcery-ai review |
1 participant
Summary
Fixes #135.
Validation
cargo fmt --checkcargo checkcargo test(136 passed)Notes
cargo check --target x86_64-pc-windows-msvccould not complete on this macOS host becauseringrequires an unavailable Windows C toolchain/header set (fatal error:assert.hfile not found).Co-Authored-By: Warp agent@warp.dev
Summary by Sourcery
Handle Windows OS shutdown events to stop the backend process quickly while preserving existing graceful shutdown behavior for normal exits.
New Features:
Bug Fixes:
Enhancements:
Build: