From f0a1a0b8f2bffd3f95c55a2738e1521ff0a254de Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Wed, 29 Jul 2026 12:07:59 +0100 Subject: [PATCH] fix: release closed session resources --- frontend/package.json | 2 +- frontend/package.json.md5 | 2 +- frontend/scripts/test-regressions.js | 40 ++++ frontend/scripts/test-session-lifecycle.js | 234 +++++++++++++++++++++ frontend/src/js/app.js | 98 ++++++++- frontend/src/js/irc.js | 7 + frontend/src/js/leviathan.js | 1 + frontend/src/js/recordings.js | 10 + frontend/src/js/sftp.js | 9 +- frontend/src/js/splitpane.js | 180 ++++++++++++++-- frontend/src/js/state.js | 3 + frontend/src/js/tabs.js | 23 +- frontend/src/js/vnc.js | 52 ++++- internal/sessions/manager.go | 215 ++++++++++++++----- internal/sessions/manager_test.go | 47 +++++ 15 files changed, 809 insertions(+), 114 deletions(-) create mode 100644 frontend/scripts/test-session-lifecycle.js diff --git a/frontend/package.json b/frontend/package.json index 6f10de4..478c986 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -8,7 +8,7 @@ "bundle:novnc": "node scripts/bundle-novnc.js", "build": "node scripts/build.js", "dev": "node scripts/build.js --watch", - "test": "node scripts/test-regressions.js", + "test": "node scripts/test-session-lifecycle.js && node scripts/test-regressions.js", "postinstall": "npm run install:assets && npm run bundle:novnc" }, "dependencies": { diff --git a/frontend/package.json.md5 b/frontend/package.json.md5 index e76773a..f57e1e5 100755 --- a/frontend/package.json.md5 +++ b/frontend/package.json.md5 @@ -1 +1 @@ -2f3e27ceb2ed8494c242e0b06d9f4da3 \ No newline at end of file +42461cd004e5d9113e61dcb6d13e144e \ No newline at end of file diff --git a/frontend/scripts/test-regressions.js b/frontend/scripts/test-regressions.js index 7de226d..f4f1dc9 100644 --- a/frontend/scripts/test-regressions.js +++ b/frontend/scripts/test-regressions.js @@ -220,6 +220,46 @@ assertContains( "async closeTab(sessionId) {\n this._hideTabTooltip();", "closing a tab must clear any visible or pending tab tooltip", ); +assertContains( + "frontend/src/js/tabs.js", + "await this.app.closeSessionImmediate(sessionId);", + "all normal tab closes must use the complete session teardown path", +); +assertContains( + "frontend/src/js/app.js", + "this.splitPanes.cleanup(sessionId)", + "closing an owner tab must release all split-pane state and child sessions", +); +assertContains( + "frontend/src/js/app.js", + "window.NexusTerminal.VNCViewer?.destroyViewer(sessionId)", + "session teardown must disconnect and remove VNC viewers", +); +assertContains( + "frontend/src/js/app.js", + "window.NexusTerminal.IRCViewer?.destroyViewer(sessionId)", + "session teardown must dispose IRC terminals and resize observers", +); +assertContains( + "frontend/src/js/app.js", + "this.recordings.cleanup(sessionId)", + "session teardown must clear frontend recording state", +); +assertContains( + "frontend/src/js/splitpane.js", + "this.app.closeSessionImmediate(leaf.sessionId", + "closing a split child must release its backend session as well as its terminal", +); +assertContains( + "frontend/src/js/splitpane.js", + "await window.NexusTerminal.wailsService.DisconnectSession(sessionInfo.id)", + "late split-pane connection results must be disconnected after their pane closes", +); +assertContains( + "internal/sessions/manager.go", + "delete(m.sessions, sessionID)", + "explicit backend disconnect must remove the session from the manager map", +); assertContains( "frontend/src/js/tabs.js", "!tab?.isConnected", diff --git a/frontend/scripts/test-session-lifecycle.js b/frontend/scripts/test-session-lifecycle.js new file mode 100644 index 0000000..b4a8ad9 --- /dev/null +++ b/frontend/scripts/test-session-lifecycle.js @@ -0,0 +1,234 @@ +#!/usr/bin/env node +const assert = require("assert"); +const fs = require("fs"); +const path = require("path"); +const vm = require("vm"); + +const frontendRoot = path.resolve(__dirname, ".."); + +function loadScript(relativePath, additions = {}) { + const context = { + console, + Promise, + Map, + Set, + setTimeout, + clearTimeout, + window: { NexusTerminal: {} }, + document: { + querySelector: () => null, + addEventListener: () => {}, + }, + ...additions, + }; + vm.createContext(context); + vm.runInContext( + fs.readFileSync(path.join(frontendRoot, relativePath), "utf8"), + context, + { filename: relativePath }, + ); + return context; +} + +async function testRepeatedSplitCleanup() { + const context = loadScript("src/js/splitpane.js"); + const SplitPaneManager = context.window.NexusTerminal.SplitPaneManager; + const closed = []; + const app = { + closeSessionImmediate: async (sessionId, options) => { + closed.push({ sessionId, options }); + }, + }; + const manager = new SplitPaneManager(app); + let cancelledDrags = 0; + + for (let i = 0; i < 500; i++) { + const ownerId = `owner-${i}`; + const childId = `child-${i}`; + const ownerEl = { _splitNode: {} }; + const childEl = { _splitNode: {} }; + const ownerLeaf = { + type: "leaf", + paneId: `owner-pane-${i}`, + el: ownerEl, + sessionId: ownerId, + parent: null, + }; + const childLeaf = { + type: "leaf", + paneId: `child-pane-${i}`, + el: childEl, + sessionId: childId, + parent: null, + }; + const branchEl = { _splitNode: {} }; + const branch = { + type: "branch", + first: ownerLeaf, + second: childLeaf, + el: branchEl, + parent: null, + cancelDrag: () => { + cancelledDrags++; + }, + }; + ownerLeaf.parent = branch; + childLeaf.parent = branch; + manager.trees.set(ownerId, branch); + manager.pendingConnections.set(`pending-${i}`, { + ownerSessionId: ownerId, + cancelled: false, + completed: false, + }); + + await manager.cleanup(ownerId); + + assert.strictEqual(ownerEl._splitNode, null); + assert.strictEqual(childEl._splitNode, null); + assert.strictEqual(branchEl._splitNode, null); + } + + assert.strictEqual(manager.trees.size, 0); + assert.strictEqual(manager.pendingConnections.size, 0); + assert.strictEqual(closed.length, 500); + assert.strictEqual(cancelledDrags, 500); + for (const call of closed) { + assert.strictEqual(call.options.removeTab, false); + assert.strictEqual(call.options.cleanupSplits, false); + } +} + +function testClosedPaneRejectsLateConnection() { + const context = loadScript("src/js/splitpane.js"); + const SplitPaneManager = context.window.NexusTerminal.SplitPaneManager; + const manager = new SplitPaneManager({ closeSessionImmediate: async () => {} }); + const pane = { + dataset: { paneId: "pane-1" }, + isConnected: true, + }; + const leaf = { + type: "leaf", + paneId: "pane-1", + el: pane, + sessionId: null, + parent: null, + }; + pane._splitNode = leaf; + manager.trees.set("owner-1", leaf); + + const token = manager._beginPendingConnection(pane, "owner-1"); + manager._cancelPendingConnection("pane-1"); + + assert.strictEqual( + manager._completePendingConnection(pane, "owner-1", token), + false, + ); + assert.strictEqual(manager.pendingConnections.size, 0); +} + +function testPerSessionStoresReleaseIDs() { + const stateContext = loadScript("src/js/state.js"); + const State = stateContext.window.NexusTerminal.State; + const state = new State(); + state.addSession({ id: "session-1" }); + state.broadcastTargets = new Set(["session-1"]); + state.removeSession("session-1"); + assert.strictEqual(state.sessions.size, 0); + assert.strictEqual(state.broadcastTargets.size, 0); + + const recordingContext = loadScript("src/js/recordings.js"); + const RecordingManager = + recordingContext.window.NexusTerminal.RecordingManager; + const recordings = new RecordingManager({}); + recordings.activeSessions.add("session-1"); + recordings.cleanup("session-1"); + assert.strictEqual(recordings.activeSessions.size, 0); +} + +async function testCompleteAppTeardownIsImmediateAndIdempotent() { + let finishBackendClose; + let backendCloses = 0; + const context = loadScript("src/js/app.js"); + const App = context.window.NexusTerminal.App; + const app = Object.create(App.prototype); + const resources = { + sftp: new Set(["session-1"]), + triggers: new Set(["session-1"]), + recordings: new Set(["session-1"]), + terminals: new Set(["session-1"]), + raw: new Set(["session-1"]), + github: new Set(["session-1"]), + vnc: new Set(["session-1"]), + irc: new Set(["session-1"]), + leviathan: new Set(["session-1"]), + }; + const drop = (name) => ({ + cleanup: (id) => resources[name].delete(id), + close: (id) => resources[name].delete(id), + destroyTerminal: (id) => resources[name].delete(id), + destroyViewer: (id) => resources[name].delete(id), + destroyPanel: (id) => resources[name].delete(id), + }); + + app._closingSessions = new Map(); + app.splitPanes = { cleanup: async () => {} }; + app.sftp = drop("sftp"); + app.triggers = drop("triggers"); + app.recordings = drop("recordings"); + app.terminals = drop("terminals"); + app.rawViewer = drop("raw"); + app.leviathan = drop("leviathan"); + app.tabs = { + order: ["session-1"], + removeTab: (id) => { + app.tabs.order = app.tabs.order.filter((value) => value !== id); + }, + }; + app.state = { + sessions: new Map([["session-1", { id: "session-1" }]]), + getSession(id) { + return this.sessions.get(id); + }, + removeSession(id) { + this.sessions.delete(id); + }, + }; + app.updateStatusBar = () => {}; + + context.window.NexusTerminal.GitHubViewer = drop("github"); + context.window.NexusTerminal.VNCViewer = drop("vnc"); + context.window.NexusTerminal.IRCViewer = drop("irc"); + context.window.NexusTerminal.wailsService = { + DisconnectSession: () => { + backendCloses++; + return new Promise((resolve) => { + finishBackendClose = resolve; + }); + }, + }; + + const first = app.closeSessionImmediate("session-1"); + const second = app.closeSessionImmediate("session-1"); + assert.strictEqual(first, second); + assert.strictEqual(backendCloses, 1); + assert.strictEqual(app.tabs.order.length, 0); + assert.strictEqual(app.state.sessions.size, 0); + for (const retained of Object.values(resources)) { + assert.strictEqual(retained.size, 0); + } + + finishBackendClose(); + await first; + assert.strictEqual(app._closingSessions.size, 0); +} + +(async () => { + await testRepeatedSplitCleanup(); + testClosedPaneRejectsLateConnection(); + testPerSessionStoresReleaseIDs(); + await testCompleteAppTeardownIsImmediateAndIdempotent(); + console.log("Session lifecycle regression tests passed."); +})().catch((err) => { + console.error(err); + process.exitCode = 1; +}); diff --git a/frontend/src/js/app.js b/frontend/src/js/app.js index 5da303f..3fdf308 100644 --- a/frontend/src/js/app.js +++ b/frontend/src/js/app.js @@ -47,6 +47,8 @@ this.sshKeys = new SSHKeyManager(this); this.triggers = new TriggerEngine(this); this.leviathan = new LeviathanWorkbench(this); + /** @type {Map>} Idempotent in-flight session closes. */ + this._closingSessions = new Map(); this.updateInfo = null; this.mode = "nexus"; } @@ -399,20 +401,94 @@ * Disconnects and removes a session without confirmation prompts. * @param {string} sessionId */ - async closeSessionImmediate(sessionId) { + closeSessionImmediate(sessionId, options = {}) { + const existing = this._closingSessions.get(sessionId); + if (existing) return existing; + + let resolveOperation; + let rejectOperation; + const operation = new Promise((resolve, reject) => { + resolveOperation = resolve; + rejectOperation = reject; + }); + this._closingSessions.set(sessionId, operation); + try { - await window.NexusTerminal.wailsService.DisconnectSession(sessionId); - } catch (_) { - // Ignore failures; we still want to clean up locally. + this._closeSessionResources(sessionId, options).then( + resolveOperation, + rejectOperation, + ); + } catch (err) { + rejectOperation(err); } - this.terminals.destroyTerminal(sessionId); - this.rawViewer.destroyViewer(sessionId); - window.NexusTerminal.GitHubViewer.destroyViewer(sessionId); - window.NexusTerminal.VNCViewer.destroyViewer(sessionId); - this.tabs.removeTab(sessionId); - this.state.removeSession(sessionId); + const clearClosing = () => { + if (this._closingSessions.get(sessionId) === operation) { + this._closingSessions.delete(sessionId); + } + }; + operation.then(clearClosing, clearClosing); + return operation; + } + + /** + * Releases every frontend and backend resource owned by a session. + * Local objects are dropped before awaiting the backend so a slow close + * cannot leave renderers, observers or viewers running invisibly. + * @param {string} sessionId + * @param {{removeTab?: boolean, cleanupSplits?: boolean}} options + * @private + */ + async _closeSessionResources(sessionId, options = {}) { + const removeTab = options.removeTab !== false; + const cleanupSplits = options.cleanupSplits !== false; + + let splitCleanup = Promise.resolve(); + if (cleanupSplits) { + try { + splitCleanup = Promise.resolve(this.splitPanes.cleanup(sessionId)); + } catch (_) { + // Continue with the rest of the idempotent local cleanup. + } + } + + const localCleanups = [ + () => this.sftp.close(sessionId), + () => this.triggers.cleanup(sessionId), + () => this.recordings.cleanup(sessionId), + () => this.terminals.destroyTerminal(sessionId), + () => this.rawViewer.destroyViewer(sessionId), + () => window.NexusTerminal.GitHubViewer?.destroyViewer(sessionId), + () => window.NexusTerminal.VNCViewer?.destroyViewer(sessionId), + () => window.NexusTerminal.IRCViewer?.destroyViewer(sessionId), + () => this.leviathan?.destroyPanel(sessionId), + ]; + for (const cleanup of localCleanups) { + try { + cleanup(); + } catch (_) { + // One faulty viewer must not prevent the remaining resources closing. + } + } + + if (removeTab && this.tabs.order.includes(sessionId)) { + this.tabs.removeTab(sessionId); + } + if (this.state.getSession(sessionId)) { + this.state.removeSession(sessionId); + } this.updateStatusBar(); + + let backendCleanup = Promise.resolve(); + try { + backendCleanup = Promise.resolve( + window.NexusTerminal.wailsService.DisconnectSession(sessionId), + ); + } catch (_) { + // Ignore failures; all local resources have already been released. + } + + await Promise.allSettled([splitCleanup, backendCleanup]); } // ── Keyboard shortcuts ─────────────────────────────────────────── @@ -1829,6 +1905,8 @@ // ── Bootstrap ──────────────────────────────────────────────────────── + window.NexusTerminal.App = App; + document.addEventListener("DOMContentLoaded", () => { try { const app = new App(); diff --git a/frontend/src/js/irc.js b/frontend/src/js/irc.js index 3144acc..3c21a71 100644 --- a/frontend/src/js/irc.js +++ b/frontend/src/js/irc.js @@ -195,6 +195,13 @@ this.term.dispose(); this.term = null; } + this.fitAddon = null; + this.searchAddon = null; + this.targets.clear(); + this.users.clear(); + this.unreadTargets.clear(); + this.joinedRooms = []; + this.roomList = []; this.container.classList.remove("irc-viewer-panel"); this.container.innerHTML = ""; } diff --git a/frontend/src/js/leviathan.js b/frontend/src/js/leviathan.js index 8746a48..81882bb 100644 --- a/frontend/src/js/leviathan.js +++ b/frontend/src/js/leviathan.js @@ -542,6 +542,7 @@ destroyPanel(sessionId) { this.lastResults.delete(sessionId); this.autoRunners.delete(sessionId); + this.hexSearches.delete(sessionId); } handleTabFocus(sessionId) { diff --git a/frontend/src/js/recordings.js b/frontend/src/js/recordings.js index 3d3789f..f9946b7 100644 --- a/frontend/src/js/recordings.js +++ b/frontend/src/js/recordings.js @@ -65,6 +65,16 @@ class RecordingManager { } } + /** + * Drops frontend recording state when a session is closed. The backend + * finalises the recorder as part of DisconnectSession. + * @param {string} sessionId + */ + cleanup(sessionId) { + this.activeSessions.delete(sessionId); + this._updateRecordingIndicator(sessionId, false); + } + /** * Opens the recordings browser modal. */ diff --git a/frontend/src/js/sftp.js b/frontend/src/js/sftp.js index 4be4f3e..afb9546 100644 --- a/frontend/src/js/sftp.js +++ b/frontend/src/js/sftp.js @@ -192,6 +192,7 @@ var state = this.panels.get(sessionId); if (!state) return; + if (state.cleanupResize) state.cleanupResize(); state.el.remove(); this.panels.delete(sessionId); @@ -1196,14 +1197,20 @@ } function onMouseUp() { + cleanupResize(); + self._refitTerminal(sessionId); + } + + function cleanupResize() { handle.classList.remove("is-dragging"); document.body.style.userSelect = ""; document.body.style.cursor = ""; document.removeEventListener("mousemove", onMouseMove); document.removeEventListener("mouseup", onMouseUp); - self._refitTerminal(sessionId); } + state.cleanupResize = cleanupResize; + handle.addEventListener("mousedown", function (e) { e.preventDefault(); startX = e.clientX; diff --git a/frontend/src/js/splitpane.js b/frontend/src/js/splitpane.js index a2a21d8..bdc77cf 100644 --- a/frontend/src/js/splitpane.js +++ b/frontend/src/js/splitpane.js @@ -33,6 +33,14 @@ class SplitPaneManager { * @type {Map} */ this.trees = new Map(); + + /** + * Connections started from placeholders but not yet attached to a pane. + * Closing a pane/tab marks its token cancelled so a late backend result + * is disconnected instead of becoming an invisible session. + * @type {Map} + */ + this.pendingConnections = new Map(); } // ── Public API ─────────────────────────────────────────────────── @@ -55,8 +63,47 @@ class SplitPaneManager { * Removes all split state for a session (used on tab close). * @param {string} sessionId */ - cleanup(sessionId) { + async cleanup(sessionId) { + var root = this.trees.get(sessionId); this.trees.delete(sessionId); + + var childSessionIds = new Set(); + var self = this; + function releaseNode(node) { + if (!node) return; + if (node.type === 'leaf') { + self._cancelPendingConnection(node.paneId); + if (node.sessionId && node.sessionId !== sessionId) { + childSessionIds.add(node.sessionId); + } + if (node.el) node.el._splitNode = null; + node.parent = null; + return; + } + if (node.cancelDrag) node.cancelDrag(); + releaseNode(node.first); + releaseNode(node.second); + if (node.el) node.el._splitNode = null; + node.parent = null; + } + releaseNode(root); + + // A connection may still be resolving before its leaf has a session ID. + // Its token remains captured by the promise and will disconnect any late + // result after _cancelPendingConnection marks it cancelled. + for (var [paneId, token] of this.pendingConnections) { + if (token.ownerSessionId === sessionId) { + token.cancelled = true; + this.pendingConnections.delete(paneId); + } + } + + await Promise.allSettled(Array.from(childSessionIds, function(childId) { + return self.app.closeSessionImmediate(childId, { + removeTab: false, + cleanupSplits: false + }); + })); } /** @@ -265,15 +312,27 @@ class SplitPaneManager { var branch = leaf.parent; var sibling = (branch.first === leaf) ? branch.second : branch.first; - // If the closing pane has a terminal session, destroy it (but not the owner session) + this._cancelPendingConnection(leaf.paneId); + + // If the closing pane has a child session, release both its frontend + // renderer and backend connection. The owner pane cannot be closed + // independently because it provides the tab's identity. if (leaf.sessionId && leaf.sessionId !== ownerSessionId) { - this.app.terminals.destroyTerminal(leaf.sessionId); + this.app.closeSessionImmediate(leaf.sessionId, { + removeTab: false, + cleanupSplits: false + }); + leaf.sessionId = null; } + if (branch.cancelDrag) branch.cancelDrag(); + // Replace the branch element with the sibling element in the DOM sibling.el.style.flex = branch.el.style.flex || '1'; branch.el.parentNode.insertBefore(sibling.el, branch.el); branch.el.remove(); + leaf.el._splitNode = null; + branch.el._splitNode = null; // Remove close button from sibling if it's now the only pane sibling.parent = branch.parent; @@ -312,18 +371,37 @@ class SplitPaneManager { */ _setupDividerDrag(divider, branch) { var self = this; + var onMouseMove = null; + var onMouseUp = null; + var bodyClass = null; + + function stopDrag(refit) { + if (bodyClass) document.body.classList.remove(bodyClass); + divider.classList.remove('is-dragging'); + if (onMouseMove) document.removeEventListener('mousemove', onMouseMove); + if (onMouseUp) document.removeEventListener('mouseup', onMouseUp); + onMouseMove = null; + onMouseUp = null; + bodyClass = null; + if (refit) self._refitTree(branch); + } + + branch.cancelDrag = function() { + stopDrag(false); + }; divider.addEventListener('mousedown', function(e) { e.preventDefault(); + stopDrag(false); var isHorizontal = branch.direction === 'horizontal'; - var bodyClass = isHorizontal ? 'split-resizing-row' : 'split-resizing'; + bodyClass = isHorizontal ? 'split-resizing-row' : 'split-resizing'; document.body.classList.add(bodyClass); divider.classList.add('is-dragging'); var containerRect = branch.el.getBoundingClientRect(); var dividerSize = isHorizontal ? 4 : 4; // divider thickness - function onMouseMove(e) { + onMouseMove = function(e) { var totalSize, mousePos; if (isHorizontal) { totalSize = containerRect.height - dividerSize; @@ -343,17 +421,11 @@ class SplitPaneManager { // Refit terminals in this branch self._refitTree(branch); - } + }; - function onMouseUp() { - document.body.classList.remove(bodyClass); - divider.classList.remove('is-dragging'); - document.removeEventListener('mousemove', onMouseMove); - document.removeEventListener('mouseup', onMouseUp); - - // Final refit - self._refitTree(branch); - } + onMouseUp = function() { + stopDrag(true); + }; document.addEventListener('mousemove', onMouseMove); document.addEventListener('mouseup', onMouseUp); @@ -443,17 +515,29 @@ class SplitPaneManager { */ _connectInPane(paneEl, profile, ownerSessionId) { var self = this; + var token = this._beginPendingConnection(paneEl, ownerSessionId); // Remove placeholder var placeholder = paneEl.querySelector('.split-pane-placeholder'); if (placeholder) placeholder.remove(); // Connect via backend - window.NexusTerminal.wailsService.ConnectSession(profile.id).then(function(sessionInfo) { + window.NexusTerminal.wailsService.ConnectSession(profile.id).then(async function(sessionInfo) { + if (!self._completePendingConnection(paneEl, ownerSessionId, token)) { + try { + await window.NexusTerminal.wailsService.DisconnectSession(sessionInfo.id); + } catch (_) { + // The backend may already have completed the close. + } + return; + } self._attachTerminalToPane(paneEl, sessionInfo, profile); self.app.notifications.info('Connecting to ' + (profile.displayName || profile.host) + '…'); }).catch(function(err) { + if (!self._completePendingConnection(paneEl, ownerSessionId, token)) { + return; + } self.app.notifications.error('Connection failed: ' + err); // Re-render placeholder on failure self._renderPlaceholder(paneEl, ownerSessionId); @@ -468,10 +552,19 @@ class SplitPaneManager { */ _connectLocalInPane(paneEl, ownerSessionId) { var self = this; + var token = this._beginPendingConnection(paneEl, ownerSessionId); var placeholder = paneEl.querySelector('.split-pane-placeholder'); if (placeholder) placeholder.remove(); - window.NexusTerminal.wailsService.ConnectLocal().then(function(sessionInfo) { + window.NexusTerminal.wailsService.ConnectLocal().then(async function(sessionInfo) { + if (!self._completePendingConnection(paneEl, ownerSessionId, token)) { + try { + await window.NexusTerminal.wailsService.DisconnectSession(sessionInfo.id); + } catch (_) { + // The backend may already have completed the close. + } + return; + } var prefs = self.app.state.preferences || {}; self._attachTerminalToPane(paneEl, sessionInfo, { terminalFontFamily: prefs.defaultFontFamily, @@ -481,6 +574,9 @@ class SplitPaneManager { }); self.app.notifications.info('Opening local terminal…'); }).catch(function(err) { + if (!self._completePendingConnection(paneEl, ownerSessionId, token)) { + return; + } self.app.notifications.error('Local terminal failed: ' + err); self._renderPlaceholder(paneEl, ownerSessionId); }); @@ -513,6 +609,53 @@ class SplitPaneManager { this.app.terminals.createTerminal(sessionInfo.id, termContainer.id, terminalProfile); } + /** + * Tracks an asynchronous connection attempt owned by a pane. + * @private + */ + _beginPendingConnection(paneEl, ownerSessionId) { + var paneId = paneEl.dataset.paneId; + this._cancelPendingConnection(paneId); + var token = { + ownerSessionId: ownerSessionId, + cancelled: false, + completed: false + }; + this.pendingConnections.set(paneId, token); + return token; + } + + /** + * Returns true only if a connection result still belongs to a live pane. + * @private + */ + _completePendingConnection(paneEl, ownerSessionId, token) { + if (token.completed) return false; + token.completed = true; + var paneId = paneEl.dataset.paneId; + if (this.pendingConnections.get(paneId) === token) { + this.pendingConnections.delete(paneId); + } + var root = this.trees.get(ownerSessionId); + var leaf = this._findLeafById(root, paneId); + return !token.cancelled && + paneEl.isConnected !== false && + leaf && + leaf.el === paneEl; + } + + /** + * Cancels a pending pane connection without waiting for the backend call. + * A late successful result will disconnect itself. + * @private + */ + _cancelPendingConnection(paneId) { + var token = this.pendingConnections.get(paneId); + if (!token) return; + token.cancelled = true; + this.pendingConnections.delete(paneId); + } + // ── Tree traversal helpers ─────────────────────────────────────── /** @@ -594,6 +737,9 @@ class SplitPaneManager { _addCloseButton(paneEl, ownerSessionId) { // Don't add if already present if (paneEl.querySelector(':scope > .split-pane-close')) return; + // The owner session supplies the tab/panel identity. Allowing that leaf + // to close would leave an invisible backend session behind. + if (paneEl._splitNode && paneEl._splitNode.sessionId === ownerSessionId) return; var btn = document.createElement('button'); btn.className = 'split-pane-close'; diff --git a/frontend/src/js/state.js b/frontend/src/js/state.js index 026c72a..0956d1d 100644 --- a/frontend/src/js/state.js +++ b/frontend/src/js/state.js @@ -164,6 +164,9 @@ */ removeSession(id) { this.sessions.delete(id); + if (this.broadcastTargets) { + this.broadcastTargets.delete(id); + } if (this.activeSessionId === id) { // Activate the most recent remaining session, or null const remaining = [...this.sessions.keys()]; diff --git a/frontend/src/js/tabs.js b/frontend/src/js/tabs.js index 496933a..8ff8624 100644 --- a/frontend/src/js/tabs.js +++ b/frontend/src/js/tabs.js @@ -182,19 +182,7 @@ if (!ok) return; } - try { - await window.NexusTerminal.wailsService.DisconnectSession(sessionId); - } catch (_) { - // Session may already be closed — carry on - } - - this.app.terminals.destroyTerminal(sessionId); - this.app.rawViewer.destroyViewer(sessionId); - window.NexusTerminal.GitHubViewer.destroyViewer(sessionId); - window.NexusTerminal.IRCViewer.destroyViewer(sessionId); - this.app.leviathan?.destroyPanel(sessionId); - this.removeTab(sessionId); - this.app.state.removeSession(sessionId); + await this.app.closeSessionImmediate(sessionId); } /** @@ -852,13 +840,8 @@ const session = this.app.state.getSession(sessionId); if (!session || !session.profileId) return; - // Close the current tab, then open a fresh connection - this.app.terminals.destroyTerminal(sessionId); - this.app.rawViewer.destroyViewer(sessionId); - window.NexusTerminal.GitHubViewer.destroyViewer(sessionId); - this.app.state.removeSession(sessionId); - this.removeTab(sessionId); - + // Fully close the old session before opening a fresh connection. + await this.app.closeSessionImmediate(sessionId); await this.app.connectProfile(session.profileId); } diff --git a/frontend/src/js/vnc.js b/frontend/src/js/vnc.js index 1f8495d..fc35e41 100644 --- a/frontend/src/js/vnc.js +++ b/frontend/src/js/vnc.js @@ -74,6 +74,9 @@ this.container = container; this.profile = profile; this.rfb = null; + this._rfbListeners = []; + this._credentialDialog = null; + this._destroyed = false; this._buildUI(); } @@ -132,6 +135,7 @@ * @param {string} wsURL - WebSocket URL (ws://127.0.0.1:) */ connect(wsURL) { + if (this._destroyed) return; if (!window.noVNC || !window.noVNC.RFB) { this.statusEl.textContent = "Error: noVNC not loaded"; this.statusEl.classList.add("vnc-status-error"); @@ -154,13 +158,13 @@ this.rfb.compressionLevel = 2; // Event handlers - this.rfb.addEventListener("connect", () => { + this._addRFBListener("connect", () => { this.statusEl.textContent = "Connected"; this.statusEl.classList.remove("vnc-status-error"); this.statusEl.classList.add("vnc-status-connected"); }); - this.rfb.addEventListener("disconnect", (ev) => { + this._addRFBListener("disconnect", (ev) => { const clean = ev.detail.clean; this.statusEl.textContent = clean ? "Disconnected" @@ -171,13 +175,13 @@ } }); - this.rfb.addEventListener("credentialsrequired", () => { + this._addRFBListener("credentialsrequired", () => { this.statusEl.textContent = "Credentials required"; // TODO: prompt for VNC password via modal this._promptCredentials(); }); - this.rfb.addEventListener("desktopname", (ev) => { + this._addRFBListener("desktopname", (ev) => { this.statusEl.textContent = "Connected — " + ev.detail.name; }); } catch (err) { @@ -192,6 +196,11 @@ } } + _addRFBListener(name, handler) { + this.rfb.addEventListener(name, handler); + this._rfbListeners.push({ name, handler }); + } + async _syncClipboard() { if (!this.rfb) return; try { @@ -213,6 +222,9 @@ } _promptCredentials() { + if (this._destroyed) return; + this._closeCredentialDialog(); + // Build a simple modal for VNC password const backdrop = createElement("div", "modal-backdrop"); const modal = createElement("div", "modal modal-sm"); @@ -242,13 +254,21 @@ const input = modal.querySelector("#vnc-password-input"); const submit = modal.querySelector("#vnc-password-submit"); const cancel = modal.querySelector("#vnc-password-cancel"); + const dialog = { + backdrop, + modal, + removeTimer: null, + focusTimer: null, + }; + this._credentialDialog = dialog; const close = () => { backdrop.classList.remove("is-visible"); modal.classList.remove("is-visible"); - setTimeout(() => { - backdrop.remove(); - modal.remove(); + dialog.removeTimer = setTimeout(() => { + if (this._credentialDialog === dialog) { + this._closeCredentialDialog(); + } }, 200); }; @@ -269,7 +289,17 @@ if (e.key === "Escape") cancel.click(); }); - setTimeout(() => input.focus(), 50); + dialog.focusTimer = setTimeout(() => input.focus(), 50); + } + + _closeCredentialDialog() { + const dialog = this._credentialDialog; + if (!dialog) return; + if (dialog.removeTimer) clearTimeout(dialog.removeTimer); + if (dialog.focusTimer) clearTimeout(dialog.focusTimer); + dialog.backdrop.remove(); + dialog.modal.remove(); + this._credentialDialog = null; } resize() { @@ -281,7 +311,13 @@ } destroy() { + this._destroyed = true; + this._closeCredentialDialog(); if (this.rfb) { + for (const { name, handler } of this._rfbListeners) { + this.rfb.removeEventListener(name, handler); + } + this._rfbListeners = []; this.rfb.disconnect(); this.rfb = null; } diff --git a/internal/sessions/manager.go b/internal/sessions/manager.go index c3bbb3f..dbbbed9 100644 --- a/internal/sessions/manager.go +++ b/internal/sessions/manager.go @@ -38,6 +38,7 @@ const maxTunnelPrivateKeyBytes = 1 << 20 // Session wraps a protocol client with metadata and state. type Session struct { Info models.SessionInfo + closed bool // explicitly removed from the manager sshClient *sshclient.Client telnetClient *telnetclient.Client rawClient *rawclient.Client @@ -169,6 +170,11 @@ func (m *Manager) Connect(profile *models.Profile, getSecret func(prompt string) if err != nil { m.mu.Lock() + if current, ok := m.sessions[sessionID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + m.closeSession(sess) + return + } sess.Info.State = models.SessionError sess.Info.Error = err.Error() m.mu.Unlock() @@ -182,6 +188,11 @@ func (m *Manager) Connect(profile *models.Profile, getSecret func(prompt string) } m.mu.Lock() + if current, ok := m.sessions[sessionID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + m.closeSession(sess) + return + } sess.Info.State = models.SessionConnected sess.Info.ConnectedAt = time.Now() m.mu.Unlock() @@ -191,7 +202,9 @@ func (m *Manager) Connect(profile *models.Profile, getSecret func(prompt string) "state": string(models.SessionConnected), }) - m.auditLog.LogConnect(profile.ID, sessionID, string(profile.Protocol), profile.Host) + if m.auditLog != nil { + m.auditLog.LogConnect(profile.ID, sessionID, string(profile.Protocol), profile.Host) + } }() return sessionID, nil @@ -317,7 +330,7 @@ func (m *Manager) startLocalSession(sess *Session, cols, rows int) { // Publish the client before Start so a resize arriving during process // creation is retained by Client.Resize and used for the initial PTY size. m.mu.Lock() - if sess.Info.State != models.SessionConnecting { + if sess.closed || sess.Info.State != models.SessionConnecting { m.mu.Unlock() return } @@ -348,8 +361,9 @@ func (m *Manager) startLocalSession(sess *Session, cols, rows int) { } m.mu.Lock() - if sess.localClient != client || sess.Info.State != models.SessionConnecting { + if sess.closed || sess.localClient != client || sess.Info.State != models.SessionConnecting { m.mu.Unlock() + client.Close() return } sess.Info.State = models.SessionConnected @@ -360,7 +374,9 @@ func (m *Manager) startLocalSession(sess *Session, cols, rows int) { "sessionID": sessionID, "state": string(models.SessionConnected), }) - m.auditLog.LogConnect("", sessionID, "local", "localhost") + if m.auditLog != nil { + m.auditLog.LogConnect("", sessionID, "local", "localhost") + } } func (m *Manager) handleLocalSessionClosed(sessionID string, client *localshell.Client) { @@ -397,9 +413,17 @@ func (m *Manager) Disconnect(sessionID string) error { m.mu.Unlock() return fmt.Errorf("sessions: session %q not found", sessionID) } + // Explicit disconnect means the tab/session is gone, not merely offline. + // Detach it before closing clients so their onClose callbacks cannot start + // auto-reconnect work or retain the session in the manager indefinitely. + delete(m.sessions, sessionID) + sess.closed = true m.mu.Unlock() m.closeSession(sess) + if m.auditLog != nil { + m.auditLog.LogDisconnect(sessionID) + } return nil } @@ -622,20 +646,20 @@ func (m *Manager) PinSession(sessionID string, pinned bool) error { // CloseAll closes every active session. func (m *Manager) CloseAll() { - m.mu.RLock() + m.mu.Lock() sessions := make([]*Session, 0, len(m.sessions)) for _, sess := range m.sessions { + sess.closed = true sessions = append(sessions, sess) } - m.mu.RUnlock() + // Detach all sessions before closing protocol clients. Close callbacks can + // otherwise schedule reconnects while the application is shutting down. + m.sessions = make(map[string]*Session) + m.mu.Unlock() for _, sess := range sessions { m.closeSession(sess) } - - m.mu.Lock() - m.sessions = make(map[string]*Session) - m.mu.Unlock() } // GetSessionHistory returns the RAW session history (RAW protocol only). @@ -894,6 +918,11 @@ func (m *Manager) connectSSH(sess *Session, getSecret func(prompt string) (strin } m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + _ = client.Close() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.sshClient = client cols, rows := sess.consumePendingResizeLocked() m.mu.Unlock() @@ -997,6 +1026,11 @@ func (m *Manager) connectTelnet(sess *Session) error { } m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + _ = client.Close() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.telnetClient = client cols, rows := sess.consumePendingResizeLocked() m.mu.Unlock() @@ -1049,7 +1083,14 @@ func (m *Manager) connectRAW(sess *Session) error { return err } + m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + _ = client.Close() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.rawClient = client + m.mu.Unlock() return nil } @@ -1095,7 +1136,13 @@ func (m *Manager) connectGitHub(sess *Session, getSecret func(prompt string) (st } } + m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.githubClient = client + m.mu.Unlock() // Perform the initial fetch. m.emitEvent("session:github:status", map[string]interface{}{ @@ -1129,7 +1176,14 @@ func (m *Manager) connectGitHub(sess *Session, getSecret func(prompt string) (st } ctx, cancel := context.WithCancel(context.Background()) + m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + cancel() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.githubCancel = cancel + m.mu.Unlock() go func() { ticker := time.NewTicker(interval) @@ -1316,7 +1370,14 @@ func (m *Manager) connectVNC(sess *Session, getSecret func(prompt string) (strin return err } + m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + client.Close() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.vncClient = client + m.mu.Unlock() // Emit the WebSocket URL so the frontend can connect noVNC. m.emitEvent("session:vnc:ready", map[string]interface{}{ @@ -1433,7 +1494,14 @@ func (m *Manager) connectIRC(sess *Session, getSecret func(prompt string) (strin return err } + m.mu.Lock() + if current, ok := m.sessions[sess.Info.ID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + client.Close() + return fmt.Errorf("session %q was closed while connecting", sess.Info.ID) + } sess.ircClient = client + m.mu.Unlock() return nil } @@ -2077,55 +2145,72 @@ func (m *Manager) isGitHubAuthError(err error) bool { // closeSession shuts down the protocol client for a session. func (m *Manager) closeSession(sess *Session) { - // Cancel any in-progress reconnect backoff. - if sess.cancelReconn != nil { - sess.cancelReconn() - sess.cancelReconn = nil - } - - // Stop any active recording. - if sess.recorder != nil { - sess.recorder.Close() - sess.recorder = nil - } - - // Close SFTP subsystem if open. - if sess.sftpClient != nil { - sess.sftpClient.Close() - sess.sftpClient = nil - } - - if sess.sshClient != nil { - sess.sshClient.Close() - } - if sess.telnetClient != nil { - sess.telnetClient.Close() - } - if sess.rawClient != nil { - sess.rawClient.Close() - } - if sess.githubCancel != nil { - sess.githubCancel() - } - if sess.vncClient != nil { - sess.vncClient.Close() - sess.vncClient = nil - } - if sess.ircClient != nil { - sess.ircClient.Close() - sess.ircClient = nil - } m.mu.Lock() + sess.closed = true + cancelReconn := sess.cancelReconn + recorder := sess.recorder + sftpClient := sess.sftpClient + sshClient := sess.sshClient + telnetClient := sess.telnetClient + rawClient := sess.rawClient + githubCancel := sess.githubCancel + vncClient := sess.vncClient + ircClient := sess.ircClient localClient := sess.localClient + + // Break all retained client/callback references while holding the lifecycle + // lock. This makes repeated close calls harmless and lets closed sessions be + // collected even if a short-lived callback still holds the Session object. + sess.cancelReconn = nil + sess.recorder = nil + sess.sftpClient = nil + sess.sshClient = nil + sess.telnetClient = nil + sess.rawClient = nil + sess.githubClient = nil + sess.githubCancel = nil + sess.vncClient = nil + sess.ircClient = nil sess.localClient = nil + sess.profile = nil + sess.localShell = "" + sess.localCwd = "" + sess.reconnectCount = 0 + sess.pendingResizeCols = 0 + sess.pendingResizeRows = 0 + sess.Info.State = models.SessionDisconnected m.mu.Unlock() + + if cancelReconn != nil { + cancelReconn() + } + if recorder != nil { + _ = recorder.Close() + } + if sftpClient != nil { + _ = sftpClient.Close() + } + if sshClient != nil { + _ = sshClient.Close() + } + if telnetClient != nil { + _ = telnetClient.Close() + } + if rawClient != nil { + _ = rawClient.Close() + } + if githubCancel != nil { + githubCancel() + } + if vncClient != nil { + vncClient.Close() + } + if ircClient != nil { + ircClient.Close() + } if localClient != nil { localClient.Close() } - - m.mu.Lock() - sess.Info.State = models.SessionDisconnected - m.mu.Unlock() } // --------------------------------------------------------------------------- @@ -2229,15 +2314,19 @@ func (m *Manager) waitForOutput(sess *Session, pattern string, timeout time.Dura func (m *Manager) handleSessionClosed(sessionID string) { m.mu.Lock() sess, ok := m.sessions[sessionID] - if ok { - sess.Info.State = models.SessionDisconnected + if !ok || sess.closed { + m.mu.Unlock() + return } + sess.Info.State = models.SessionDisconnected m.mu.Unlock() - m.auditLog.LogDisconnect(sessionID) + if m.auditLog != nil { + m.auditLog.LogDisconnect(sessionID) + } // Check whether auto-reconnect is enabled for this session. - if ok && m.shouldAutoReconnect(sess) { + if m.shouldAutoReconnect(sess) { m.scheduleReconnect(sess, sessionID) return } @@ -2358,6 +2447,10 @@ func (m *Manager) scheduleReconnect(sess *Session, sessionID string) { // Clear old clients before reconnecting. m.mu.Lock() + if current, ok := m.sessions[sessionID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + return + } sess.sshClient = nil sess.telnetClient = nil sess.rawClient = nil @@ -2387,6 +2480,11 @@ func (m *Manager) scheduleReconnect(sess *Session, sessionID string) { if err != nil { m.mu.Lock() + if current, ok := m.sessions[sessionID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + m.closeSession(sess) + return + } sess.Info.State = models.SessionError sess.Info.Error = err.Error() m.mu.Unlock() @@ -2404,6 +2502,11 @@ func (m *Manager) scheduleReconnect(sess *Session, sessionID string) { // Success — reset the counter. m.mu.Lock() + if current, ok := m.sessions[sessionID]; !ok || current != sess || sess.closed { + m.mu.Unlock() + m.closeSession(sess) + return + } sess.reconnectCount = 0 sess.Info.State = models.SessionConnected sess.Info.ConnectedAt = time.Now() diff --git a/internal/sessions/manager_test.go b/internal/sessions/manager_test.go index 5ab12a2..ca76215 100644 --- a/internal/sessions/manager_test.go +++ b/internal/sessions/manager_test.go @@ -4,6 +4,7 @@ import ( "crypto/ed25519" "crypto/rand" "encoding/base64" + "fmt" "os" "path/filepath" "runtime" @@ -148,6 +149,52 @@ func TestDisconnectNotFound(t *testing.T) { } } +func TestDisconnectRemovesSessionAndReleasesReferences(t *testing.T) { + mgr := NewManager(nil, nil, nil, nil, nil) + sess := &Session{ + Info: models.SessionInfo{ + ID: "session-1", + State: models.SessionConnected, + }, + profile: &models.Profile{ID: "profile-1"}, + } + mgr.sessions[sess.Info.ID] = sess + + if err := mgr.Disconnect(sess.Info.ID); err != nil { + t.Fatalf("Disconnect() error = %v", err) + } + if got := len(mgr.ListSessions()); got != 0 { + t.Fatalf("ListSessions() returned %d sessions after disconnect, want 0", got) + } + if !sess.closed { + t.Fatal("disconnected session was not marked closed") + } + if sess.profile != nil { + t.Fatal("disconnected session retained its profile") + } +} + +func TestRepeatedDisconnectDoesNotRetainSessions(t *testing.T) { + mgr := NewManager(nil, nil, nil, nil, nil) + + for i := 0; i < 1000; i++ { + id := fmt.Sprintf("session-%d", i) + mgr.sessions[id] = &Session{ + Info: models.SessionInfo{ + ID: id, + State: models.SessionConnected, + }, + } + if err := mgr.Disconnect(id); err != nil { + t.Fatalf("Disconnect(%q) error = %v", id, err) + } + } + + if got := len(mgr.ListSessions()); got != 0 { + t.Fatalf("ListSessions() retained %d explicitly closed sessions, want 0", got) + } +} + // --------------------------------------------------------------------------- // CloseAll on empty manager does not panic // ---------------------------------------------------------------------------