Stop the live screen only for the socket that is showing it (#191)

* Stop the live screen only for the socket that is showing it

A second /stream for a Bot replaces the first: open stops whatever was casting
and puts the new socket in the session. It does not close the socket it
replaced, so that one closes whenever its client gets round to it, which on an
ordinary reconnect is after the replacement is already running. close then
stopped the session's viewer without asking whether the closing socket was the
one casting, so it stopped the new one.

Both halves of the failure are silent. The screen stops updating, and the
person's input is dropped without a word, because the input path checks for a
viewer before it checks anything it can report.

The decision goes in its own file for the reason bot-id.ts and authorisation.ts
are separate: index.ts imports Playwright at module scope, so anything left in
it needs Chrome merely to be imported by a test.

* Say in the changelog that a live-screen reconnect stops killing itself

Author: beardthelion

CHANGELOG.md

@@ -107,6 +107,19 @@ A thread the platform does not have now reads as having no messages. A 404 and o
 a 500, because an outage answered with an empty history would tell the browser the conversation is gone
 and invite somebody to start it over.
 
+### Reconnecting a live screen no longer stops the screen you just reconnected
+
+A Bot's screen allows one viewer, and opening a second `/stream` replaces the first. The replaced
+socket is left open, because it belongs to a client that may still be using it, so on an ordinary
+reconnect, where the browser opens the new connection before dropping the old one, the old socket
+closed after the new one was already casting. Closing it stopped the session's viewer without asking
+whether the closing socket was the one casting, so it stopped the replacement.
+
+Both halves were silent. The screen stopped updating, and anything typed afterwards was dropped
+without a word, because the input path looks for a viewer before it looks for anything it can report.
+
+A close now stops casting only when the socket closing is the one that was casting.
+
 ## 0.0.4
 
 ### A click citing a ref this deployment cannot resolve is refused

agent-computer/src/index.ts

@@ -19,6 +19,7 @@ import {
   startScreencast,
 } from "./screencast";
 import { createShell } from "./shell";
+import { isCurrentViewer } from "./viewer";
 import {
   createWorkspace,
   WorkspaceFileError,
@@ -441,7 +442,11 @@ serve<StreamData>({
     },
 
     async close(ws) {
-      await stopViewer(sessionFor(ws.data.botId));
+      const session = sessionFor(ws.data.botId);
+      // Only the socket that is casting. A superseded one closing after its replacement has started
+      // would otherwise stop the new viewer; see viewer.ts.
+      if (!isCurrentViewer(session.viewer, ws)) return;
+      await stopViewer(session);
     },
   },
   async fetch(request, server) {

agent-computer/src/viewer.ts

@@ -0,0 +1,33 @@
+/**
+ * Which socket is allowed to stop the live screen.
+ *
+ * A Bot's screen has one viewer, and a second `/stream` replaces the first rather than being
+ * refused: `open` stops whatever was casting and puts the new socket in the session. What it does
+ * not do is close the socket it replaced, because that socket belongs to a client that may still be
+ * using it. So the superseded socket closes on its own schedule, and on an ordinary reconnect, where
+ * a client opens the new connection before dropping the old one, that is after the replacement is
+ * already casting.
+ *
+ * A `close` handler that stops the session's viewer without asking whether the closing socket is the
+ * one casting therefore stops the wrong viewer. The screen the person just reconnected to goes quiet,
+ * and their input is dropped without a word, because the input path checks for a viewer before it
+ * checks anything it could report. Both failures are silent; the browser is fine, the Bot is fine,
+ * and the person is looking at a still image.
+ *
+ * It lives in its own file for the reason `bot-id.ts` and `authorisation.ts` do: `index.ts` imports
+ * Playwright at module scope, so anything left in it needs Chrome merely to be imported by a test.
+ * The decision is here and the stopping stays there, the same split `browser-eviction.ts` makes.
+ */
+
+/**
+ * Is this socket the one currently casting?
+ *
+ * By identity, never by value. Two sockets are distinct objects however alike they look, and an
+ * equality that compared their contents would put the bug back for any pair that happened to match.
+ */
+export function isCurrentViewer(
+  current: { socket: unknown } | undefined,
+  socket: unknown,
+): boolean {
+  return current?.socket === socket;
+}

agent-computer/tests/viewer.test.ts

@@ -0,0 +1,41 @@
+import { describe, expect, test } from "bun:test";
+import { isCurrentViewer } from "../src/viewer";
+
+/**
+ * Which socket is allowed to stop the live screen.
+ *
+ * One viewer per Bot, and a second `/stream` replaces the first: `open` stops whatever was casting
+ * and puts the new socket in the session. The old socket is not closed by that, so its `close`
+ * arrives whenever the client gets round to it, which on an ordinary make-before-break reconnect is
+ * after the replacement is already running. A close that stops the current viewer without asking
+ * whether it owns it stops the wrong one, and the person who just reconnected gets a screen that
+ * never updates and input that goes nowhere.
+ *
+ * The decision rather than the stopping. Stopping a cast is Playwright's job and is not where the
+ * wrong answer was; `browser-eviction.ts` splits the same way and for the same reason.
+ */
+describe("deciding whether a closing socket stops the live screen", () => {
+  const socket = { id: "a" };
+  const other = { id: "b" };
+
+  test("the socket that is casting stops it", () => {
+    expect(isCurrentViewer({ socket }, socket)).toBe(true);
+  });
+
+  test("a socket that was replaced stops nothing", () => {
+    // The bug this exists for. The old socket closes after the new one has taken over, and without
+    // this the new viewer is the one that gets stopped.
+    expect(isCurrentViewer({ socket: other }, socket)).toBe(false);
+  });
+
+  test("a close with no viewer at all stops nothing", () => {
+    // Both sockets gone, or the cast never started. There is nothing to stop and nothing to get wrong.
+    expect(isCurrentViewer(undefined, socket)).toBe(false);
+  });
+
+  test("identity, not shape", () => {
+    // Two sockets are never equal by value, and comparing them that way would put the bug back for
+    // any pair that happened to look alike.
+    expect(isCurrentViewer({ socket: { id: "a" } }, { id: "a" })).toBe(false);
+  });
+});