Fix/terminal tab close persistent (#14359)

* fix(terminal): ensure tab close is resilient to session cleanup failures

- Wrap _closeTerminalSessionIfNeeded in isolated try/catch so that
  tabController.closeBy always executes even if FFI calls throw
- Add clarifying comment in handleWindowCloseButton for single-tab
  audit dialog flow

* fix(terminal): fix session reconnect ID mismatch and tab close race condition

Remap surviving persistent sessions to client-requested terminal IDs on
reconnect, preventing new shell creation when IDs are non-contiguous.
Snapshot peerTabCount before async operations in _closeTab to avoid race
with concurrent _closeAllTabs clearing the tab controller.
Remove debug log statements.

Signed-off-by: fufesou <linlong1266@gmail.com>

---------

Signed-off-by: fufesou <linlong1266@gmail.com>

flutter/lib/desktop/pages/terminal_tab_page.dart

@@ -34,6 +34,8 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
   static const IconData selectedIcon = Icons.terminal;
   static const IconData unselectedIcon = Icons.terminal_outlined;
   int _nextTerminalId = 1;
+  // Lightweight idempotency guard for async close operations
+  final Set<String> _closingTabs = {};
 
   _TerminalTabPageState(Map<String, dynamic> params) {
     Get.put(DesktopTabController(tabType: DesktopTabType.terminal));
@@ -70,24 +72,7 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
       label: tabLabel,
       selectedIcon: selectedIcon,
       unselectedIcon: unselectedIcon,
-      onTabCloseButton: () async {
+      onTabCloseButton: () => _closeTab(tabKey),
-        if (await desktopTryShowTabAuditDialogCloseCancelled(
-          id: tabKey,
-          tabController: tabController,
-        )) {
-          return;
-        }
-        // Close the terminal session first
-        final ffi = TerminalConnectionManager.getExistingConnection(peerId);
-        if (ffi != null) {
-          final terminalModel = ffi.terminalModels[terminalId];
-          if (terminalModel != null) {
-            await terminalModel.closeTerminal();
-          }
-        }
-        // Then close the tab
-        tabController.closeBy(tabKey);
-      },
       page: TerminalPage(
         key: ValueKey(tabKey),
         id: peerId,
@@ -102,6 +87,149 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
     );
   }
 
+  /// Unified tab close handler for all close paths (button, shortcut, programmatic).
+  /// Shows audit dialog, cleans up session if not persistent, then removes the UI tab.
+  Future<void> _closeTab(String tabKey) async {
+    // Idempotency guard: skip if already closing this tab
+    if (_closingTabs.contains(tabKey)) return;
+    _closingTabs.add(tabKey);
+
+    try {
+      // Snapshot peerTabCount BEFORE any await to avoid race with concurrent
+      // _closeAllTabs clearing tabController (which would make the live count
+      // drop to 0 and incorrectly trigger session persistence).
+      // Note: the snapshot may become stale if other individual tabs are closed
+      // during the audit dialog, but this is an acceptable trade-off.
+      int? snapshotPeerTabCount;
+      final parsed = _parseTabKey(tabKey);
+      if (parsed != null) {
+        final (peerId, _) = parsed;
+        snapshotPeerTabCount = tabController.state.value.tabs.where((t) {
+          final p = _parseTabKey(t.key);
+          return p != null && p.$1 == peerId;
+        }).length;
+      }
+
+      if (await desktopTryShowTabAuditDialogCloseCancelled(
+        id: tabKey,
+        tabController: tabController,
+      )) {
+        return;
+      }
+
+      // Close terminal session if not in persistent mode.
+      // Wrapped separately so session cleanup failure never blocks UI tab removal.
+      try {
+        await _closeTerminalSessionIfNeeded(tabKey,
+            peerTabCount: snapshotPeerTabCount);
+      } catch (e) {
+        debugPrint('[TerminalTabPage] Session cleanup failed for $tabKey: $e');
+      }
+      // Always close the tab from UI, regardless of session cleanup result
+      tabController.closeBy(tabKey);
+    } catch (e) {
+      debugPrint('[TerminalTabPage] Error closing tab $tabKey: $e');
+    } finally {
+      _closingTabs.remove(tabKey);
+    }
+  }
+
+  /// Close all tabs with session cleanup.
+  /// Used for window-level close operations (onDestroy, handleWindowCloseButton).
+  /// UI tabs are removed immediately; session cleanup runs in parallel with a
+  /// bounded timeout so window close is not blocked indefinitely.
+  Future<void> _closeAllTabs() async {
+    final tabKeys = tabController.state.value.tabs.map((t) => t.key).toList();
+    // Remove all UI tabs immediately (same instant behavior as the old tabController.clear())
+    tabController.clear();
+    // Run session cleanup in parallel with bounded timeout (closeTerminal() has internal 3s timeout).
+    // Skip tabs already being closed by a concurrent _closeTab() to avoid duplicate FFI calls.
+    final futures = tabKeys
+        .where((tabKey) => !_closingTabs.contains(tabKey))
+        .map((tabKey) async {
+      try {
+        await _closeTerminalSessionIfNeeded(tabKey, persistAll: true);
+      } catch (e) {
+        debugPrint('[TerminalTabPage] Session cleanup failed for $tabKey: $e');
+      }
+    }).toList();
+    if (futures.isNotEmpty) {
+      await Future.wait(futures).timeout(
+        const Duration(seconds: 4),
+        onTimeout: () {
+          debugPrint(
+              '[TerminalTabPage] Session cleanup timed out for batch close');
+          return [];
+        },
+      );
+    }
+  }
+
+  /// Close the terminal session on server side based on persistent mode.
+  ///
+  /// [persistAll] controls behavior when persistent mode is enabled:
+  /// - `true` (window close): persist all sessions, don't close any.
+  /// - `false` (tab close): only persist the last session for the peer,
+  ///   close others so only the most recent disconnected session survives.
+  Future<void> _closeTerminalSessionIfNeeded(String tabKey,
+      {bool persistAll = false, int? peerTabCount}) async {
+    final parsed = _parseTabKey(tabKey);
+    if (parsed == null) return;
+    final (peerId, terminalId) = parsed;
+
+    final ffi = TerminalConnectionManager.getExistingConnection(peerId);
+    if (ffi == null) return;
+
+    final isPersistent = bind.sessionGetToggleOptionSync(
+      sessionId: ffi.sessionId,
+      arg: kOptionTerminalPersistent,
+    );
+
+    if (isPersistent) {
+      if (persistAll) {
+        // Window close: persist all sessions
+        return;
+      }
+      // Tab close: only persist if this is the last tab for this peer.
+      // Use the snapshot value if provided (avoids race with concurrent tab removal).
+      final effectivePeerTabCount = peerTabCount ??
+          tabController.state.value.tabs.where((t) {
+            final p = _parseTabKey(t.key);
+            return p != null && p.$1 == peerId;
+          }).length;
+      if (effectivePeerTabCount <= 1) {
+        // Last tab for this peer — persist the session
+        return;
+      }
+      // Not the last tab — fall through to close the session
+    }
+
+    final terminalModel = ffi.terminalModels[terminalId];
+    if (terminalModel != null) {
+      // closeTerminal() has internal 3s timeout, no need for external timeout
+      await terminalModel.closeTerminal();
+    }
+  }
+
+  /// Parse tabKey (format: "peerId_terminalId") into its components.
+  /// Note: peerId may contain underscores, so we use lastIndexOf('_').
+  /// Returns null if tabKey format is invalid.
+  (String peerId, int terminalId)? _parseTabKey(String tabKey) {
+    final lastUnderscore = tabKey.lastIndexOf('_');
+    if (lastUnderscore <= 0) {
+      debugPrint('[TerminalTabPage] Invalid tabKey format: $tabKey');
+      return null;
+    }
+    final terminalIdStr = tabKey.substring(lastUnderscore + 1);
+    final terminalId = int.tryParse(terminalIdStr);
+    if (terminalId == null) {
+      debugPrint('[TerminalTabPage] Invalid terminalId in tabKey: $tabKey');
+      return null;
+    }
+    final peerId = tabKey.substring(0, lastUnderscore);
+    return (peerId, terminalId);
+  }
+
   Widget _tabMenuBuilder(String peerId, CancelFunc cancelFunc) {
     final List<MenuEntryBase<String>> menu = [];
     const EdgeInsets padding = EdgeInsets.only(left: 8.0, right: 5.0);
@@ -185,7 +313,8 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
       } else if (call.method == kWindowEventRestoreTerminalSessions) {
         _restoreSessions(call.arguments);
       } else if (call.method == "onDestroy") {
-        tabController.clear();
+        // Clean up sessions before window destruction (bounded wait)
+        await _closeAllTabs();
       } else if (call.method == kWindowActionRebuild) {
         reloadCurrentWindow();
       } else if (call.method == kWindowEventActiveSession) {
@@ -269,7 +398,7 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
           // macOS: Cmd+W (standard for close tab)
           final currentTab = tabController.state.value.selectedTabInfo;
           if (tabController.state.value.tabs.length > 1) {
-            tabController.closeBy(currentTab.key);
+            _closeTab(currentTab.key);
             return true;
           }
         } else if (!isMacOS &&
@@ -278,7 +407,7 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
           // Other platforms: Ctrl+Shift+W (to avoid conflict with Ctrl+W word delete)
           final currentTab = tabController.state.value.selectedTabInfo;
           if (tabController.state.value.tabs.length > 1) {
-            tabController.closeBy(currentTab.key);
+            _closeTab(currentTab.key);
             return true;
           }
         }
@@ -357,12 +486,10 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
 
   void _addNewTerminalForCurrentPeer({int? terminalId}) {
     final currentTab = tabController.state.value.selectedTabInfo;
-    final tabKey = currentTab.key;
+    final parsed = _parseTabKey(currentTab.key);
-    final lastUnderscore = tabKey.lastIndexOf('_');
+    if (parsed == null) return;
-    if (lastUnderscore > 0) {
+    final (peerId, _) = parsed;
-      final peerId = tabKey.substring(0, lastUnderscore);
+    _addNewTerminal(peerId, terminalId: terminalId);
-      _addNewTerminal(peerId, terminalId: terminalId);
-    }
   }
 
   @override
@@ -376,11 +503,9 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
           selectedBorderColor: MyTheme.accent,
           labelGetter: DesktopTab.tablabelGetter,
           tabMenuBuilder: (key) {
-            // Extract peerId from tab key (format: "peerId_terminalId")
+            final parsed = _parseTabKey(key);
-            // Use lastIndexOf to handle peerIds containing underscores
+            if (parsed == null) return Container();
-            final lastUnderscore = key.lastIndexOf('_');
+            final (peerId, _) = parsed;
-            if (lastUnderscore <= 0) return Container();
-            final peerId = key.substring(0, lastUnderscore);
             return _tabMenuBuilder(peerId, () {});
           },
         ));
@@ -435,7 +560,7 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
       }
     }
     if (connLength <= 1) {
-      tabController.clear();
+      await _closeAllTabs();
       return true;
     } else {
       final bool res;
@@ -446,7 +571,7 @@ class _TerminalTabPageState extends State<TerminalTabPage> {
         res = await closeConfirmDialog();
       }
       if (res) {
-        tabController.clear();
+        await _closeAllTabs();
       }
       return res;
     }

src/server/terminal_service.rs

@@ -777,6 +777,32 @@ impl TerminalServiceProxy {
     ) -> Result<Option<TerminalResponse>> {
         let mut response = TerminalResponse::new();
 
+        // When the client requests a terminal_id that doesn't exist but there are
+        // surviving persistent sessions, remap the lowest-ID session to the requested
+        // terminal_id. This handles the case where _nextTerminalId resets to 1 on
+        // reconnect but the server-side sessions have non-contiguous IDs (e.g. {2: htop}).
+        //
+        // The client's requested terminal_id may not match any surviving session ID
+        // (e.g. _nextTerminalId incremented beyond the surviving IDs). This remap is a
+        // one-time handle reassignment — only the first reconnect triggers it because
+        // needs_session_sync is cleared afterward. Remaining sessions are communicated
+        // back via `persistent_sessions` with their original server-side IDs.
+        if !service.sessions.contains_key(&open.terminal_id)
+            && service.needs_session_sync
+            && !service.sessions.is_empty()
+        {
+            if let Some(&lowest_id) = service.sessions.keys().min() {
+                log::info!(
+                    "Remapping persistent session {} -> {} for reconnection",
+                    lowest_id,
+                    open.terminal_id
+                );
+                if let Some(session_arc) = service.sessions.remove(&lowest_id) {
+                    service.sessions.insert(open.terminal_id, session_arc);
+                }
+            }
+        }
+
         // Check if terminal already exists
         if let Some(session_arc) = service.sessions.get(&open.terminal_id) {
             // Reconnect to existing terminal
@@ -824,7 +850,7 @@ impl TerminalServiceProxy {
 
         // Create new terminal session
         log::info!(
-            "Creating new terminal {} for service: {}",
+            "Creating new terminal {} for service {}",
             open.terminal_id,
             service.service_id
         );