fix(meet_call): abort scanner on close to unblock 60-second navigation stall#1380
Conversation
…n stall The meet_scanner join-automation task held CDP WebSocket connections open for up to NAME_INPUT_BUDGET + JOIN_BUTTON_BUDGET (60 s total) after the Meet window closed. CEF waited for all active CDP sessions to detach before firing WindowEvent::Destroyed, delaying the meet-call:closed frontend event by the same margin and blocking navigation. Fixes tinyhumansai#1378. Changes: - meet_scanner::spawn() returns an AbortHandle instead of () so callers can cancel the task. - MeetCallState gains a scanner_aborts map keyed by request_id. - meet_call_open_window stores the AbortHandle and aborts on CloseRequested (covers both programmatic close and OS title-bar close). A defensive abort on Destroyed handles the rare case where CloseRequested didn't fire. - meet_call_close_window aborts the scanner before calling window.close() so CEF receives the close signal with no active CDP sessions. - 3 new tests: budget_constants_are_sane, abort_handle_cancels_spawned_task, meet_call_state_scanner_aborts_insert_and_remove.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes a 60-second navigation stall by making meet_scanner tasks cancellable: ChangesScanner Lifecycle Abort Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src-tauri/src/meet_call/mod.rs (1)
229-291: 💤 Low valueConsider whether synchronous
remove_dir_allinside async spawn is optimal.The
Destroyedhandler spawns an async task but then callsstd::fs::remove_dir_all, which is a blocking filesystem operation. While this moves the blocking call off the UI thread (good), it still blocks a Tokio worker thread.For a directory that CEF may have written significant data to, this could briefly starve other async tasks. Consider using
tokio::fs::remove_dir_allfor fully async I/O:♻️ Suggested improvement
tauri::async_runtime::spawn(async move { - if let Err(err) = std::fs::remove_dir_all(&dir_to_purge) { + if let Err(err) = tokio::fs::remove_dir_all(&dir_to_purge).await { log::debug!( "[meet-call] data-dir cleanup skipped request_id={request_id_for_purge} dir={} err={err}", dir_to_purge.display() ); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src-tauri/src/meet_call/mod.rs` around lines 229 - 291, The cleanup task currently uses blocking std::fs::remove_dir_all inside tauri::async_runtime::spawn (see dir_to_purge / request_id_for_purge and the std::fs::remove_dir_all call), which can block a Tokio worker; change it to perform nonblocking IO by calling tokio::fs::remove_dir_all(&dir_to_purge).await inside the async move spawned closure (or, if tokio::fs is unavailable, wrap the existing call in tokio::task::spawn_blocking), and propagate/log errors the same way so the log message still reports request_id_for_purge and dir_to_purge.display() on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/meet_call/mod.rs`:
- Around line 324-330: The code currently returns Ok(false) when
app.get_webview_window(&label) is None but leaves the stale entry in
state.inner; update the handler so that when app.get_webview_window(&label)
returns None you first remove the stale mapping from state.inner (e.g., call
remove(&request_id) on the same lock used earlier) before returning Ok(false),
referencing the existing variables request_id, label, state.inner and the call
to app.get_webview_window(&label).
---
Nitpick comments:
In `@app/src-tauri/src/meet_call/mod.rs`:
- Around line 229-291: The cleanup task currently uses blocking
std::fs::remove_dir_all inside tauri::async_runtime::spawn (see dir_to_purge /
request_id_for_purge and the std::fs::remove_dir_all call), which can block a
Tokio worker; change it to perform nonblocking IO by calling
tokio::fs::remove_dir_all(&dir_to_purge).await inside the async move spawned
closure (or, if tokio::fs is unavailable, wrap the existing call in
tokio::task::spawn_blocking), and propagate/log errors the same way so the log
message still reports request_id_for_purge and dir_to_purge.display() on
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e40db51-634b-4674-8301-0f3d18b2dc51
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
app/src-tauri/src/meet_call/mod.rsapp/src-tauri/src/meet_scanner/mod.rs
…ri registry When app.get_webview_window returns None but the request_id still exists in state.inner, clean up the orphaned map entry before returning Ok(false). Addresses CodeRabbit suggestion on PR tinyhumansai#1380.
Summary
WindowEvent::Destroyed, so themeet-call:closedfrontend event was delayed by the same margin — blocking navigation.meet_scanner::spawn()now returns anAbortHandle.MeetCallStatestores it per-call and aborts onCloseRequested(and as a fallback onDestroyed).meet_call_close_windowalso aborts the scanner before sending the close signal to CEF.Problem
meet_scanner(phase 2 + phase 3, each up to 30 s) kept active WebSocket connections to CEF's debugging endpoint. CEF defers renderer shutdown until all CDP sessions detach, so the window close stalled for the full scanner timeout.Solution
meet_scanner::spawn()changed to returntokio::task::AbortHandle(switched fromtauri::async_runtime::spawntotokio::spawnto expose the handle).MeetCallStategains ascanner_aborts: Mutex<HashMap<String, AbortHandle>>field.WindowEvent::CloseRequested— primary path, covers both programmaticwindow.close()and OS title-bar close. Aborts the scanner immediately so CEF finds no active CDP sessions when it starts renderer shutdown.WindowEvent::Destroyed— defensive fallback for cases whereCloseRequesteddid not fire.meet_call_close_windowalso callsabort()beforewindow.close()so CEF receives the close signal with no competing CDP work.Submission Checklist
docs/TESTING-STRATEGY.mdbudget_constants_are_sane,abort_handle_cancels_spawned_task,meet_call_state_scanner_aborts_insert_and_remove)Closes #1378in Related sectionImpact
meet_callandmeet_scannerare Tauri-shell modules.abort()is idempotent — calling it after the scanner finishes naturally is a safe no-op.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/meet-cleanup-blocks-navigationValidation Run
pnpm --filter openhuman-app format:check— N/A: no frontend changespnpm typecheck— N/A: no frontend changescargo test --manifest-path app/src-tauri/Cargo.toml meet→ 42 passedcargo fmt --all+cargo check→ cleancargo check --manifest-path app/src-tauri/Cargo.toml→ no errorsValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
meet-call:closedreaches the frontend in milliseconds instead of up to 60 s.Parity Contract
meet-call:closedevent emission still happen viaWindowEvent::Destroyedexactly as before.Destroyedhandler ensures the scanner is always cancelled even ifCloseRequestedis not fired by the OS.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
New Features