Preserve session ending reasons and distinguish clean disconnects

static/client.html

@@ -1840,6 +1840,36 @@ <h2 id="disconnected-title">Session Ended</h2>
                 rgGo('/connections.html');
             });
 
+            var sessionEndStatus = null;
+            var sessionHasEnded = false;
+            function showSessionEnd(status) {
+                // Keep the first reported reason: subsequent tunnel closure is
+                // a consequence, not a new error. A clean end does not prove
+                // Windows logoff (guacd also uses SUCCESS for disconnect).
+                if (status && !sessionEndStatus) sessionEndStatus = status;
+                var firstEnd = !sessionHasEnded;
+                sessionHasEnded = true;
+                var code = sessionEndStatus && sessionEndStatus.code;
+                var normalEnd = !sessionEndStatus || code === Guacamole.Status.Code.SUCCESS
+                    || code === Guacamole.Status.Code.SESSION_CLOSED
+                    || code === Guacamole.Status.Code.SESSION_TIMEOUT
+                    || code === Guacamole.Status.Code.SESSION_CONFLICT;
+                var message = (sessionEndStatus && sessionEndStatus.message)
+                    || (code === Guacamole.Status.Code.SESSION_TIMEOUT ? 'The remote session timed out.' : '')
+                    || (code === Guacamole.Status.Code.SESSION_CONFLICT ? 'Disconnected by another connection.' : '')
+                    || (normalEnd ? 'The remote connection has ended.' : 'Connection lost');
+                statusEl.textContent = normalEnd ? message : 'Error: ' + message;
+                statusEl.className = '';
+                rgDisarmFullscreenGesture(); rgHideConnectCover();
+                if (_thumbInterval) { clearInterval(_thumbInterval); _thumbInterval = null; }
+                if (firstEnd) teardownMonitorWindows('disconnect');
+                if (embedded) { if (firstEnd) rgNotifyParent('end'); return; }
+                document.getElementById('disconnected-title').textContent = normalEnd ? 'Session Ended' : 'Connection Error';
+                document.getElementById('disconnected-message').textContent = message;
+                document.getElementById('disconnected-overlay').className = 'visible';
+                if (rgFullscreenEl()) rgExitFullscreen();
+            }
+
             client.onstatechange = function(state) {
                 switch (state) {
                     case Guacamole.Client.State.CONNECTING: statusEl.textContent = 'Connecting...'; statusEl.className = ''; rgGateStatus('Connecting...', false); break;
@@ -1923,7 +1953,9 @@ <h2 id="disconnected-title">Session Ended</h2>
                         }
                         break;
                     case Guacamole.Client.State.DISCONNECTING:
-                        statusEl.textContent = 'Disconnecting...'; statusEl.className = '';
+                        if (!sessionHasEnded) {
+                            statusEl.textContent = 'Disconnecting...'; statusEl.className = '';
+                        }
                         // Final thumbnail capture before disconnect
                         if (_thumbInterval) {
                             clearInterval(_thumbInterval);
@@ -1932,45 +1964,21 @@ <h2 id="disconnected-title">Session Ended</h2>
                         }
                         break;
                     case Guacamole.Client.State.DISCONNECTED:
-                        statusEl.textContent = 'Disconnected'; statusEl.className = '';
-                        rgDisarmFullscreenGesture(); rgHideConnectCover();
-                        if (_thumbInterval) { clearInterval(_thumbInterval); _thumbInterval = null; }
-                        if (embedded) { rgNotifyParent('end'); break; }
-                        document.getElementById('disconnected-overlay').className = 'visible';
-                        if (rgFullscreenEl()) rgExitFullscreen();
-                        // Reflect the disconnect in any open monitor pop-outs
-                        // (freeze the stale blit + show a "Disconnected" overlay).
-                        teardownMonitorWindows('disconnect');
+                        showSessionEnd();
                         break;
                 }
             };
 
             client.onerror = function(status) {
-                statusEl.textContent = 'Error: ' + (status.message || 'Unknown error');
-                statusEl.className = '';
-                rgDisarmFullscreenGesture(); rgHideConnectCover();
-                // Stop the 10s thumbnail uploader; the session is dead and
-                // each XHR would otherwise 404 against a cleaned-up id.
-                if (_thumbInterval) { clearInterval(_thumbInterval); _thumbInterval = null; }
-                document.getElementById('disconnected-title').textContent = 'Connection Error';
-                document.getElementById('disconnected-message').textContent = status.message || 'An error occurred.';
-                document.getElementById('disconnected-overlay').className = 'visible';
-                if (rgFullscreenEl()) rgExitFullscreen();
+                showSessionEnd(status || { code: Guacamole.Status.Code.SERVER_ERROR });
+                client.disconnect();
             };
 
             // Wire tunnel events back into the client. Upstream Guacamole's
-            // Client.js deliberately doesn't listen on tunnel.onerror /
-            // tunnel.onstatechange — the Apache webapp installs those
-            // handlers externally via AngularJS. Our lean client.html
-            // inherits Tunnel.js + Client.js from upstream but used to
-            // miss this glue, so when a mid-path middlebox silently
-            // dropped the WS and socket.onclose fired, the tunnel would
-            // transition to CLOSED, fire onerror into the void, and the
-            // Guacamole client would stay stuck in CONNECTED forever.
-            // Effect: the user saw a frozen tab, no "Session Ended"
-            // overlay, and the browser kept uploading thumbnails into
-            // 404 responses for hours. Forwarding tunnel events into the
-            // existing client.onerror closes that loop.
+            // Tunnel.js reports abnormal closure via onerror before CLOSED.
+            // CLOSED itself is not an error: it also follows a clean remote
+            // ending or client.disconnect(). Always finish client cleanup,
+            // without replacing a specific reason with "Connection lost".
             tunnel.onerror = function(status) {
                 if (typeof console !== 'undefined')
                     console.log('[rustguac] tunnel error:',
@@ -1985,11 +1993,9 @@ <h2 id="disconnected-title">Session Ended</h2>
                 if (state === Guacamole.Tunnel.State.CLOSED) {
                     if (typeof console !== 'undefined')
                         console.log('[rustguac] tunnel closed');
-                    if (client.onerror) client.onerror({
-                        code: 519,
-                        message: 'Connection lost'
-                    });
-                } else if (state === Guacamole.Tunnel.State.UNSTABLE) {
+                    client.disconnect();
+                    showSessionEnd();
+                } else if (state === Guacamole.Tunnel.State.UNSTABLE && !sessionHasEnded) {
                     if (typeof console !== 'undefined')
                         console.log('[rustguac] tunnel unstable');
                     statusEl.textContent = 'Connection unstable...';

tests/client-session-endings.mjs

@@ -0,0 +1,125 @@
+#!/usr/bin/env node
+// Exercise the actual client.html handlers with Guacamole's status codes and
+// the same error -> CLOSED -> client DISCONNECTED ordering as Tunnel.js.
+// Run: node tests/client-session-endings.mjs (also covered by CI's tests/*.mjs).
+import { readFile } from 'node:fs/promises';
+import { dirname, resolve, join } from 'node:path';
+import { fileURLToPath } from 'node:url';
+import vm from 'node:vm';
+import assert from 'node:assert/strict';
+
+const root = resolve(dirname(fileURLToPath(import.meta.url)), '..');
+const html = await readFile(join(root, 'static/client.html'), 'utf8');
+const start = html.indexOf('var sessionEndStatus = null;');
+const end = html.indexOf('// Disconnected overlay buttons', start);
+assert.ok(start !== -1 && end > start, 'session-ending handlers must exist');
+const handlers = html.slice(start, end);
+const statusSource = await readFile(join(root, 'static/guac/Status.js'), 'utf8');
+
+function setup(embedded = false) {
+    const elements = new Map();
+    let state = 3;
+    const ctx = {
+        embedded, statusEl: {}, _thumbInterval: 42, cleared: [], notifications: [], monitors: [],
+        console: { log() {} },
+        document: { getElementById(id) {
+            if (!elements.has(id)) elements.set(id, {});
+            return elements.get(id);
+        } },
+        rgDisarmFullscreenGesture() {}, rgHideConnectCover() {},
+        rgFullscreenEl() { return false; },
+        rgNotifyParent(event) { ctx.notifications.push(event); },
+        teardownMonitorWindows(reason) { ctx.monitors.push(reason); },
+        clearInterval(id) { ctx.cleared.push(id); },
+        captureAndUploadThumbnail() {},
+        client: { disconnect() {
+            if (state === 4 || state === 5) return;
+            state = 4;
+            ctx.client.onstatechange(state);
+            ctx.tunnel.onstatechange(ctx.Guacamole.Tunnel.State.CLOSED);
+            state = 5;
+            ctx.client.onstatechange(state);
+        } },
+        tunnel: {}
+    };
+    vm.createContext(ctx);
+    vm.runInContext(statusSource, ctx);
+    ctx.Guacamole.Client = { State: {
+        CONNECTING: 1, WAITING: 2, CONNECTED: 3, DISCONNECTING: 4, DISCONNECTED: 5
+    } };
+    ctx.Guacamole.Tunnel = { State: { CLOSED: 2, UNSTABLE: 3 } };
+    vm.runInContext(handlers, ctx);
+    return {
+        ctx, elements, codes: ctx.Guacamole.Status.Code,
+        closed() { ctx.tunnel.onstatechange(ctx.Guacamole.Tunnel.State.CLOSED); },
+        title() { return elements.get('disconnected-title')?.textContent; },
+        message() { return elements.get('disconnected-message')?.textContent; },
+        cleanup() {
+            assert.equal(state, 5, 'client must leave CONNECTED');
+            assert.equal(ctx._thumbInterval, null);
+            assert.deepEqual(ctx.cleared, [42]);
+            assert.deepEqual(ctx.monitors, ['disconnect']);
+        }
+    };
+}
+
+// Clean remote ending and explicit client disconnect are not connection errors.
+// Neither proves Windows logoff: don't invent that distinction in the UI.
+for (const localDisconnect of [false, true]) {
+    const h = setup();
+    if (localDisconnect) h.ctx.client.disconnect(); else h.closed();
+    assert.equal(h.title(), 'Session Ended');
+    assert.equal(h.message(), 'The remote connection has ended.');
+    assert.doesNotMatch(h.message(), /log(?:ged)?[ -]?off|sign(?:ed)?[ -]?out/i);
+    h.cleanup();
+}
+
+// Guacd's specific reason survives subsequent client/tunnel closure events.
+for (const [codeName, message, title] of [
+    ['SESSION_CLOSED', 'Logged off.', 'Session Ended'],
+    ['SESSION_CLOSED', 'Forcibly disconnected.', 'Session Ended'],
+    ['SESSION_TIMEOUT', 'Idle session time limit exceeded.', 'Session Ended'],
+    ['SESSION_CONFLICT', 'Disconnected by other connection.', 'Session Ended'],
+    ['CLIENT_UNAUTHORIZED', 'Authentication failure (invalid credentials?)', 'Connection Error'],
+    ['UPSTREAM_NOT_FOUND', 'DNS lookup failed (incorrect hostname?)', 'Connection Error']
+]) {
+    const h = setup();
+    h.ctx.client.onerror({ code: h.codes[codeName], message });
+    h.closed();
+    h.ctx.tunnel.onerror({ code: h.codes.SERVER_ERROR, message: 'Connection lost' });
+    assert.equal(h.title(), title);
+    assert.equal(h.message(), message);
+    assert.ok(h.ctx.statusEl.textContent.endsWith(message));
+    h.cleanup();
+}
+
+// Abnormal transport closure still reports an error and performs cleanup.
+for (const message of [undefined, 'Server timeout.']) {
+    const h = setup();
+    h.ctx.tunnel.onerror({ code: h.codes.SERVER_TIMEOUT, message });
+    h.closed();
+    assert.equal(h.title(), 'Connection Error');
+    assert.equal(h.message(), message || 'Connection lost');
+    h.cleanup();
+}
+
+// Missing statuses are errors; explicit SUCCESS is not.
+for (const success of [false, true]) {
+    const h = setup();
+    h.ctx.client.onerror(success ? { code: h.codes.SUCCESS } : undefined);
+    assert.equal(h.title(), success ? 'Session Ended' : 'Connection Error');
+    h.cleanup();
+}
+
+// Embedded launcher receives one end notification, including for errors.
+for (const error of [false, true]) {
+    const h = setup(true);
+    if (error) h.ctx.tunnel.onerror({ code: h.codes.SERVER_ERROR, message: 'Connection lost' });
+    h.closed();
+    h.closed();
+    assert.deepEqual(h.ctx.notifications, ['end']);
+    assert.equal(h.title(), undefined, 'embedded sessions use their launcher');
+    h.cleanup();
+}
+
+console.log('client-session-endings: ok');

tests/e2e/specs/session-endings.spec.mjs

@@ -0,0 +1,49 @@
+import { test, expect } from '@playwright/test';
+import { installApi } from '../lib/api.mjs';
+
+function instruction(opcode, ...args) {
+    return [opcode, ...args].map(value => `${String(value).length}.${value}`).join(',') + ';';
+}
+
+async function connect(page) {
+    await installApi(page);
+    let socket;
+    await page.routeWebSocket('**/ws/**', ws => {
+        socket = ws;
+        ws.send('4.sync,1.0;');
+    });
+    await page.goto('/client.html?session=11111111-1111-4111-8111-111111111111');
+    await expect.poll(() => !!socket).toBe(true);
+    await expect(page.locator('#status')).toHaveText('Connected');
+    return socket;
+}
+
+test('a clean guacd disconnect is a neutral session ending', async ({ page }) => {
+    const socket = await connect(page);
+    socket.send(instruction('disconnect'));
+    await expect(page.locator('#disconnected-overlay')).toBeVisible();
+    await expect(page.locator('#disconnected-title')).toHaveText('Session Ended');
+    await expect(page.locator('#disconnected-message')).toHaveText('The remote connection has ended.');
+});
+
+for (const [code, message, title] of [
+    [523, 'Logged off.', 'Session Ended'],
+    [522, 'Idle session time limit exceeded.', 'Session Ended'],
+    [769, 'Authentication failure (invalid credentials?)', 'Connection Error']
+]) {
+    test(`guacd reason survives tunnel closure: ${message}`, async ({ page }) => {
+        const socket = await connect(page);
+        socket.send(instruction('error', message, code) + instruction('disconnect'));
+        await expect(page.locator('#disconnected-overlay')).toBeVisible();
+        await expect(page.locator('#disconnected-title')).toHaveText(title);
+        await expect(page.locator('#disconnected-message')).toHaveText(message);
+    });
+}
+
+test('an abnormal WebSocket closure is still a connection error', async ({ page }) => {
+    const socket = await connect(page);
+    socket.close({ code: 1011 });
+    await expect(page.locator('#disconnected-overlay')).toBeVisible();
+    await expect(page.locator('#disconnected-title')).toHaveText('Connection Error');
+    await expect(page.locator('#disconnected-message')).toHaveText('Connection lost');
+});

tests/e2e/specs/zero-click-fullscreen.spec.mjs

@@ -6,6 +6,14 @@
 import { test, expect } from '@playwright/test';
 import { installApi, settle } from '../lib/api.mjs';
 
+// HTTP fixtures do not provide a guacd WebSocket. Keep a simulated session
+// alive so these launcher tests don't race genuine disconnect cleanup.
+test.beforeEach(async ({ page }) => {
+    await page.routeWebSocket('**/ws/**', socket => {
+        socket.send('4.sync,1.0;');
+    });
+});
+
 test('Connect on a fullscreen entry launches the embedded frame with the measured size', async ({ page }) => {
     const api = await installApi(page);
     await page.goto('/connections.html');