Add ViewHandle::try_update and use it to guard shared session retention - #15695
Conversation
Closing the terminal window removes the view mapping, so TerminalDriver::extend_shared_session_retention panicked with "Window does not exist" when it later called ViewHandle::update. Upgrade the weak handle first and no-op if the view is already gone.
|
This PR was generated with Warp. Comment   |
![warp-factories[bot]](https://avatars.githubusercontent.com/in/2278701?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Guards extend_shared_session_retention against the "Window does not exist" panic when the terminal window closed before the call, matching handle_window_closed's actual removal semantics; net position is approve-leaning with two non-blocking suggestions.
Concerns
- Several sibling call sites on
TerminalDriver(execute_command,send_text_to_cli,apply_share_requests,cd/cd_silent,block_snapshot,find_first_match_in_block_output,current_directory) callself.terminal_view.update/read/as_refthe same way this one used to, so they carry the identical "Window does not exist" panic risk if the window closes while the driver is still alive. Worth a follow-up to extract a shared "upgrade-or-return" guard and audit those paths, since this one only reached Sentry because it happened to be exercised.
Verdict
Checks: build n/a (no local toolchain to compile), tests n/a (not independently re-run; PR description reports the new/existing regression tests and cargo clippy/./script/format passing), CI in progress (Linux/macOS/Windows test and clippy jobs pending at review time), visual proof n/a (non-user-visible panic guard)
Found: 0 critical, 0 important, 2 suggestions, 0 nits
| self.terminal_view.update(ctx, |terminal, ctx| { | ||
| let Some(terminal_view) = self.terminal_view.downgrade().upgrade(ctx) else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Suggestion: this early return is silent, unlike the sibling "not an active sharer" branch below it which logs a log::warn!. Since this exact spot reached Sentry as a crash, a log::info!/log::debug! here (e.g. "Skipping shared session retention extension: terminal window already closed") would make it easier to confirm in production logs that this guard — rather than some other path — handled the closed-window case.
| ctx: &mut ModelContext<Self>, | ||
| ) { | ||
| self.terminal_view.update(ctx, |terminal, ctx| { | ||
| let Some(terminal_view) = self.terminal_view.downgrade().upgrade(ctx) else { |
There was a problem hiding this comment.
This .downgrade().upgrade() pattern to check if ViewHandle is alive is very ugly - working on a better approach here
There was a problem hiding this comment.
Understood — I’ll leave the branch unchanged and keep this thread open while you explore the preferred handle-liveness API.
Responding as wilson: Open session · View in factory
A ViewHandle keeps the view's ref count alive but does not keep its window open, so update() panics with "Window does not exist" whenever a spawned task resolves after the window was closed. The only existing way to probe for that was downgrade().upgrade(), which also consults the window's view map. A view is absent from that map while an update against it is in flight, so upgrade() reports a live but reentrantly-borrowed view as gone -- turning what should be a loud "Circular view update" panic into a silent no-op. try_update guards on the window alone, so a torn-down window yields None while a reentrant update still panics as before. Co-Authored-By: Warp <agent@warp.dev>
Replaces the downgrade().upgrade() liveness probe with try_update, so a reentrantly-borrowed terminal view is no longer misreported as a closed window, and logs when the retention update is skipped so the shutdown race is visible. Co-Authored-By: Warp <agent@warp.dev>
advait-m
changed the title
Rust convention is that a try_ method does not panic: RefCell::try_borrow_mut returns Err for exactly the reentrancy that borrow_mut panics on. try_update still panicked with "Circular view update" when the view was already checked out further up the stack, which violates that contract. Report both checkout failures through a typed ViewUpdateError instead, so the two stay distinguishable and per-variant is_actionable() puts the classification where it belongs: WindowClosed is an expected teardown race and only warns, while CircularUpdate is a bug that reaches Sentry. The panicking update_view is unchanged, so callers that never expect reentrancy still fail loudly. Co-Authored-By: Warp <agent@warp.dev>
take_view_for_update is where checkout actually fails, so it now returns Result. try_update_view propagates that error. update_view keeps the legacy panics at the compatibility boundary instead of predicting failure with a separate precondition check. Co-Authored-By: Warp <agent@warp.dev>
Remove the closed-window terminal driver test, the should-panic update tests, the call-site comment, and the over-explaining try_update docs. Keep the behavioral try_update coverage for success, WindowClosed, and CircularUpdate.
Delete the closed-window driver regression test and the two legacy panic assertions. Remove the call-site narration around report_if_error and the try_* docstring that restated a well-known convention. Reflow the remaining try_update why-comment to 100 columns. Co-Authored-By: Warp <agent@warp.dev>
Co-Authored-By: Warp <agent@warp.dev>
2 participants
Description
Fixes the
Window does not existfatal inTerminalDriver::extend_shared_session_retentionafter the terminal window and view mapping are already gone.Sentry: WARP-CLIENT-DEV-YVH (7703530317).
A
ViewHandlekeeps the view's ref count alive but does not keep its window open, so a spawned task that resolves after close used to panic inViewHandle::update. The shared-session retention path onEnvironmentSetupFailed/SetupCommandExitedShellhits that race.ViewHandle::try_updateis the fallible counterpart. Checkout failures come fromtake_view_for_update, which now returnsResult.try_update_viewpropagates that result.update_viewstill panics with"Window does not exist"/"Circular view update"so existing callers keep failing loudly.ViewUpdateErrorkeeps the two failures distinct:WindowClosedis an expected teardown race and is non-actionable, soreport_error!only warns;CircularUpdateis a bug and reaches Sentry.Start at
take_view_for_updateincrates/warpui_core/src/core/app.rs, then the panic mapping inAppContext::update_view.Linked Issue
ready-to-specorready-to-implement.Tracked as Sentry issue WARP-CLIENT-DEV-YVH / 7703530317 rather than a GitHub issue.
Testing
warpui_coretests cover a successfultry_updateplusWindowClosedandCircularUpdateerror propagation.Commands:
cargo nextest run -p warpui_core— 323 passed, 7 skipped.cargo nextest run -p warpui_core -E 'test(try_update)'— 3 passed.cargo clippy -p warp -p warpui_core --all-targets --tests -- -D warnings— clean../script/format— clean.I have manually tested my changes locally with
./script/runScreenshots / Videos
N/A — this is a panic guard on a closed-window agent-driver path.
Agent Mode
CHANGELOG-BUG-FIX: Fix a crash that could occur when extending a shared session after its terminal window had already closed.