Fix cross-window tab drag: undo-close resurrecting content-transferred windows (APP-5285) - #14938
Conversation
…d windows (APP-5285) When a cross-window tab drag closes a window because its content was handed off elsewhere (TerminationMode::ContentTransferred), the closing Workspace's own `tabs` list still references the PaneGroup that was just adopted by the target window. The generic on_window_will_close app callback doesn't know a close was transfer-driven, so it unconditionally pushed the stale Workspace onto UndoCloseStack. Cmd+Shift+T could then resurrect that window, giving two windows permanent ownership of the same pane group -- producing a duplicate tab, a sustained `terminal_panes.uuid` UNIQUE-constraint storm on every subsequent save_app, and eventual data loss if the user closed either copy. Fix: - Workspace::on_window_closed marks the close via CrossWindowTabDrag::mark_content_transferred_window_close whenever suppress_detach_panes_on_window_close is true. - The top-level on_window_will_close callback in lib.rs consumes that marker and skips the UndoCloseStack registration for content-transferred closes. - Defense in depth: report_db_error now classifies a terminal_panes.uuid UNIQUE violation and reports it with ReportErrorLogMode::OncePerRun, so a residual/future instance of this class of bug produces one Sentry event instead of a storm. Adds unit tests for the new CrossWindowTabDrag marking API and a save_app_state rollback/classification test, plus a GUI integration test (test_undo_close_does_not_resurrect_content_transferred_window) that drives a real cross-window attach and confirms undo-close is a no-op afterward. Updates specs/pei/cross-window-tab-drag/TECH.md to document the mitigation. Co-Authored-By: Warp Agent <agent@warp.dev>
There was a problem hiding this comment.
Overview
Fixes APP-5285: a cross-window tab drag could leave two windows owning the same pane group, producing a vanished tab, a duplicate on Cmd+Shift+T, and a terminal_panes.uuid UNIQUE error storm. The mechanism holds up under review, but one conclusion in the description needs the reporter's judgment before this is called complete.
Concerns
- The description concludes that the reporter's emptied command and prompt history was harmless fallout that stops being possible once the root cause is fixed. The transactional rollback argument proves the failing snapshot could not wipe already-saved SQLite state, but it does not establish that the lost tab's session-associated history survived, nor rule out an orphaned session or an earlier successful write of a bad snapshot — APP-5285 records session/prompt association loss as an open risk, and this branch adds no restart or history validation. Only the reporter can settle it: if the history came back after restart, this closes as no-data-loss; if it did not, this needs a tear-off-plus-restart coverage case and a look at the reporter's database before merge.
Verdict
Checks: build pass, tests pass, CI green (core test jobs skipped while draft), visual proof missing (recording in progress)
Found: 0 critical, 1 important, 0 suggestions, 0 nits
Remaining review findings — the marker's trigger condition, the over-broad UNIQUE matcher, the missing pre-fix test failure, and the missing recording — are being addressed in a revision and are not repeated here.
|
Answer from the reporter on the open history question: the general command and prompt history came back after restart, but the conversation belonging to the dragged tab is missing from it. That settles it as partial data loss rather than a wipe or a pure display artifact. The snapshot-rehydration explanation holds for the bulk of the history, but the lost tab's conversation is the session-association loss APP-5285 flagged as an open risk, so the description's "harmless fallout, no separate defect" conclusion needs correcting. Being established now: whether that conversation is orphaned in the database or discarded, whether this branch prevents a recurrence or only suppresses the duplicate-tab and save-storm symptoms, and whether the conversation is recoverable. |
|
Oz couldn't complete this task because required inputs, permissions, or resources were invalid or unavailable. Error Details: |
…ndo-close skip Give the `terminal_panes.uuid` UNIQUE violation its own error context so the Sentry report states the known cause instead of the generic "SQLite <kind> error", and document why a content-transferred window close is never recorded on the undo-close stack rather than filtered when the stack is popped. Co-Authored-By: Warp <agent@warp.dev>
…ant assert Fixes four review findings on the original fix: 1. The content-transferred marker was keyed off `suppress_detach_panes_on_window_close`, which is also set (and not reliably cleared) on windows that stay open after a handoff or reverse-handoff. That made a later, ordinary close of such a window get mis-marked as content-transferred, silently skipping both its undo-close registration and its pane detach. Moved the marking to the four exact call sites that issue `close_window(_, ContentTransferred)` (`Workspace::close_window_for_content_transfer`, the `RemoveSourceTabAndClosePreview` / `ClosePreviewOnly` arms of `handle_drop_result`, and `CrossWindowTabDrag::finalize_handoff`'s put-back preview close), so the marker reflects the actual close reason instead of a proxy signal. 2. `is_terminal_panes_unique_violation` matched any `terminal_panes` UNIQUE violation, not just `terminal_panes.uuid` specifically, which would have thrown a distinct corruption signal (e.g. a primary-key collision) into the same once-per-run throttle. Narrowed the match to the exact column and added a negative test. 3. Improved the throttled error's context message to name the specific violation and root cause, since `report_db_error` already knows exactly what triggered it. 4. Added a debug-time invariant check (`assert_no_duplicate_pane_group_ownership` in `get_app_state`) that catches dual pane-group ownership at the moment of formation: `debug_assert!` in dev/dogfood builds, throttled `report_error!` in release so a regression can't crash a session or storm Sentry the way the DB-level symptom did. Also confirmed (via temporary instrumentation, since removed) that `workspace:save_app` never fires during the GUI integration test binary's lifecycle in this environment -- 0 invocations across a full test run, independent of the fix -- so the new assert and the underlying `get_app_state`/SQLite path are not reachable from that harness. This is a structural gap in what the integration harness exercises, not a failure to trigger the right sequence; the persistence-layer regression coverage (`test_sqlite_save_app_state_rolls_back_on_duplicate_terminal_uuid`) is what actually exercises the real `terminal_panes.uuid` violation path. Co-Authored-By: Warp Agent <agent@warp.dev>
|
Ready for a merge decision. Posting this here because the Slack thread this came from is currently unreachable. Since the review, this branch also:
On the missing conversation: orphaned, not deleted. Nothing in the paths this incident exercised writes to Two caveats to weigh before merging:
|
Description
Fixes APP-5285: dragging a tab across windows could leave the dragged tab's pane group with two owners, which showed up to the reporter as a tab disappearing during the drag, a duplicate tab after
Cmd+Shift+T, and a "crash" that was actually a repeatingterminal_panes.uuidUNIQUE-constraint error storm fromsave_app_state.Root cause. The existing guards in
CrossWindowTabDrag(pending_source_window_closes,source_placeholder_consumed,ClosePreviewOnly) all protect the transient async gap between a transfer-driven window close being requested andWorkspace::on_window_closedactually running. They do not cover a separate, generic path: when a window closes withTerminationMode::ContentTransferred(its content was handed off, not destroyed), the closingWorkspace's owntabslist is never cleared, so it still references thePaneGroupnow owned by the target window. The top-levelon_window_will_closeapp callback (app/src/lib.rs) has no notion of why a window closed, so it unconditionally pushed every closed window ontoUndoCloseStack. PressingCmd+Shift+Tshortly after a cross-window drag could pop that staleWorkspaceback onto the screen, permanently duplicating ownership of the same pane group across two windows (not just for a few frames) — hence the duplicate tab, the confirmation-dialog-on-close warning about killing processes, and aterminal_panes.uuidUNIQUE violation on every subsequentsave_app(window move/resize/focus) until the app restarted.Fix.
close_window(_, TerminationMode::ContentTransferred):Workspace::close_window_for_content_transfer, theRemoveSourceTabAndClosePreview/ClosePreviewOnlyarms ofhandle_drop_result, andCrossWindowTabDrag::finalize_handoff's put-back preview close. Each now callsCrossWindowTabDrag::mark_content_transferred_window_closeat the exact point the close is issued.on_window_will_closecallback (app/src/lib.rs) consumes that marker viatake_content_transferred_window_closeand skips theUndoCloseStackregistration when set, so a content-transferred close can never be resurrected via undo-close. (An earlier revision of this fix keyed the marker offWorkspace::suppress_detach_panes_on_window_closeinstead — review caught that this flag is also set, and not reliably cleared, on windows that stay open after a handoff/reverse-handoff, which would have mis-marked a later ordinary close of such a window. Moving the marking to the actual close call sites fixed that.)report_db_errornow classifies aterminal_panes.uuidUNIQUE violation specifically (not just anyterminal_panesUNIQUE violation, which could mask a distinct corruption signal like a primary-key collision) and reports it withReportErrorLogMode::OncePerRunand a message naming the known cause, instead of the generic "SQLite error" every time.assert_no_duplicate_pane_group_ownershipinget_app_state) catches dual pane-group ownership at the moment of formation:debug_assert!panics loudly in dev/dogfood builds with a backtrace into the offending mutation; release builds report once per run instead, so a regression can't crash a user's session or storm Sentry the way the DB-level symptom did.Updated
specs/pei/cross-window-tab-drag/TECH.mdwith the new mitigation.On the reporter's history question
The reporter followed up: general command/prompt history came back after restart, but the one AI conversation that was on the dragged tab is missing from it. I traced this and it does not come from a genuine deletion:
agent_conversations(the table backing conversation content) is a table independent ofterminal_panes/windows/tabs, keyed byconversation_id, and is written via its ownModelEvents on transactions independent ofsave_app_state. Startup loads all rows from it unconditionally (read_agent_conversation_metadata), regardless of whether any tab currently references them — that's what backs the conversation list view.TerminalPane::detach'sDetachType::Closedbranch (the only path that clears anything AI-related on close) only clears in-memory tracking (clear_conversations_for_terminal_surface) and deletes terminal blocks (delete_blocks, a different table); it never callsdelete_conversation, which is the one path that actually issuesModelEvent::DeleteAIConversation. The reporter also said he never touched (closed) either duplicate, so this path didn't run anyway.save_app_stateis a single full-snapshot rebuild (delete-then-reinsert of every session table in one transaction), and it started failing on theterminal_panes.uuidUNIQUE violation the moment the duplicate ownership existed. That snapshot is also what capturesterminal_panes.conversation_ids/active_conversation_id— the linkage of which conversation belongs to which pane. Once saves started failing, that linkage froze at whatever was last captured, which predates the newest conversation on the dragged tab. On restart, the app rehydrated that stale linkage, so the terminal pane it restored doesn't point at the newest conversation anymore.agent_conversationstable and reachable through the global conversation list/history surface, just no longer auto-surfaced in that specific tab.save_appfailures are now bounded to the brief async close gap instead of persisting indefinitely, so the linkage-freeze window that caused this is gone.I was not able to independently confirm from inside this environment that the reporter's specific conversation is still present and reachable (that requires his account/database), so treat the above as a well-evidenced code-path analysis, not a confirmed recovery. If he still can't locate it via conversation history/search, that would point at something this analysis didn't account for and is worth a fast follow-up.
Linked Issue
Testing
cargo check -p warp --lib,cargo clippy -p warp --all-targets --tests -- -D warnings,./script/format— all clean.Unit tests (
cargo nextest run -p warp persistence::sqlite cross_window_tab_drag undo_close app_state): 36 passed, including:workspace::cross_window_tab_drag::tests::content_transferred_close_is_marked_and_consumed_once/..._is_tracked_independently_per_window— the marking API.persistence::sqlite::tests::test_sqlite_save_app_state_rolls_back_on_duplicate_terminal_uuid— reproduces theterminal_panes.uuidUNIQUE violation directly and confirmssave_app_staterolls back atomically.persistence::sqlite::tests::test_terminal_panes_unique_violation_classifier_matches_uuid_column_only— negative test proving a UNIQUE violation on a differentterminal_panescolumn (theidprimary key) is not folded into the same throttle.GUI integration test
test_undo_close_does_not_resurrect_content_transferred_window(crates/integration/src/test/workspace.rs): drags a tab into an existing window (closing the temporary preview viaContentTransferred), triggers undo-close, and asserts the window/tab counts are unchanged. Re-ran the four existing cross-window-drag integration tests plustest_restore_single_closed_pane— all pass.Regression-evidence gap, still open: I could not get this class of regression to fail visibly in the GUI integration harness. I confirmed why, concretely: I temporarily disabled the fix and added instrumentation to
workspace:save_app, then ran the full integration test suite —save_app/get_app_state(and therefore the new invariant assert, and the realterminal_panes.uuidSQLite path) were invoked zero times during the entire test lifecycle, independent of whether the fix was present. This is a structural property of the GUI integration harness (persistence/global-action dispatch isn't exercised by it), not a failure to drive the right sequence of drag/undo-close actions. The only place I can demonstrate the actualterminal_panes.uuidviolation and its throttling is the SQLite unit test above, which reproduces it directly by constructing the duplicate rows rather than driving the full UI flow.Visual verification, inconclusive. I attempted a
computer_usesession against a real, from-sourcecargo run --bin warpbuild in this sandbox (a display andWARP_API_KEYare both available here). Two blockers prevented a clean repro: (1) this environment's identity is a service account, which staging's auth rejects with "Expected a user account," so the app stays on the onboarding screen rather than reaching a normal authenticated session (I proceeded through onboarding to reach a local, non-cloud-authenticated window instead); (2) synthetic mouse input could not complete a real native cross-window drag-and-drop — every attempt reverted the tab back to the source window instead of dropping it on the target. Because the drag itself never completed, the captured video/screenshots do not demonstrate the actual bug or fix — they show an unrelated side effect of a reverted drag (Ctrl+Shift+Tadded an extra "New agent conversation" tab to the source window after the failed drag, with no dialog and no new window). I'm including this recording/screenshots for transparency about the attempt, but the automated GUI integration test above (which drives the drag through the test framework's direct APIs rather than synthetic OS-level mouse events) is the actual verification of this fix's behavior.I have manually tested my changes locally with
./script/run(blocked by the same service-account auth limitation above; see the automated integration test coverage instead).Agent Mode
Computer-use video recordings
Warp tab drag between windows + undo-close (inconclusive — see Testing section): In Warp, open a second tab and a second window, drag a tab from the first window onto the second window's tab bar, then press Cmd+Shift+T (undo-close) and observe the result (APP-5285).
Computer-use screenshots (3) — see Testing section for why this attempt is inconclusive
Warp launched from source but is stuck on the "Welcome to Warp" onboarding/login screen instead of an authenticated terminal, because the environment's identity resolves to a service account, which staging rejects with "Expected a user account".
State immediately after the cross-window tab-drag attempt: the dragged tab reverted to the source (left) window, which still has 2 tabs, while the target (right) window still has only 1 tab — the tab did not transfer.
State immediately after pressing Ctrl+Shift+T (undo-close): the focused left window gained a new "New agent conversation" tab (now 3 tabs: ~, ~, New agent conversation), duplicating the type/content of the right window's existing "New agent conversation" tab; no new window and no confirmation dialog appeared.
Conversation: https://staging.warp.dev/conversation/7c07f701-2d29-404e-bcb7-c07dae0307c0
Run: https://oz.staging.warp.dev/runs/019ff0ef-ab9c-75a6-b8fc-07b43d8b831a
This PR was generated with Oz.