Make a refused device say so, and stop containers eating clicks

Three faults that all present as "touch does nothing", found while a
console tab and the device page were open on the same simulator.

A refused input socket was announced only in the document title. The
video keeps streaming and the mirror keeps updating, so the page looks
healthy while every touch is discarded — the second tab you open is
simply dead, with no way to tell. The page now says why on screen, and
adds the remedy itself: the server's reason travels in a close frame
capped at 123 bytes, and the remedy is precisely the half that gets
truncated.

That notice is the only wording of ours on a page whose entire purpose is
to contain nothing but the app, so it is painted by CSS from an attribute
rather than written into the DOM. aria-hidden alone was measured to be
insufficient — it hides the element from getByRole and from an ARIA
snapshot, but getByText still matched it, which would let our sentence
satisfy an assertion meant for the app.

Mirroring the native tree faithfully stacks a container's full-size
transparent div over the leaves it contains, and a later sibling wins the
hit test, so Playwright refused to click visible controls with "another
element intercepts pointer events". Containers are now click-through and
only leaves clickable; clicking a container still works, because the hit
lands on a descendant. hittable cannot serve here — iOS reports it false
for whole visible subtrees.

Finally, input connections are pinged. A driver that dies without closing
leaves the socket open from this side, so Read blocks for ever and the
claim outlives the client that made it; every later client is then
refused on behalf of a driver that no longer exists. The close is
CloseNow rather than Close: waiting for a peer already measured as unable
to answer was observed to cost ~3s, all of it with the device still held.

Author: omnarayan

README.md

@@ -109,8 +109,8 @@ on durable selectors.
   in the Android driver, and they are discarded rather than guessed at.
 - **A device serves one driver at a time**, and a second claim is refused rather than shared.
   That includes the console: a browser tab left open on a device will refuse your test run,
-  and only the refused side is told why. There is no keepalive yet, so a client that dies
-  without closing its socket holds the device until the server restarts.
+  and the refused page says so on screen. A driver that dies without closing its socket is
+  detected by ping within about half a minute, and the device is released.
 - Relaunching an app does not reset its state.
 
 ## Licence

internal/server/ws.go

@@ -71,6 +71,48 @@ func (s *Server) handleVideoWS(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+// Input connections are pinged on this cadence. A driver that dies
+// without closing its socket — a killed browser, a slept laptop, a CI
+// job that was cancelled — leaves the connection open from this side, so
+// conn.Read blocks for ever and the device's claim is never released.
+// Every later client is then refused on behalf of a driver that no
+// longer exists, and on the device page the refusal is nearly invisible.
+// A ping is the only thing that distinguishes a quiet driver from a dead
+// one. The interval is a compromise: long enough to be nothing on the
+// wire, short enough that a device is reclaimable within half a minute.
+const (
+	inputPingInterval = 15 * time.Second
+	inputPingTimeout  = 10 * time.Second
+)
+
+// watchLiveness pings conn until a ping goes unanswered, then closes it
+// so the handler's blocked Read returns and its deferred release runs.
+// Closing is what frees the device; this only decides when.
+// The cadence is a parameter so tests need not wait real seconds for a
+// verdict; production callers pass the constants above.
+func watchLiveness(ctx context.Context, conn *websocket.Conn, interval, timeout time.Duration) {
+	ticker := time.NewTicker(interval)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ctx.Done():
+			return
+		case <-ticker.C:
+			pingCtx, cancel := context.WithTimeout(ctx, timeout)
+			err := conn.Ping(pingCtx)
+			cancel()
+			if err != nil {
+				// CloseNow, not Close: a graceful close waits for the
+				// peer to acknowledge, and this peer has already been
+				// measured as unable to answer — waiting was observed to
+				// cost ~3s, all of it with the device still claimed.
+				conn.CloseNow()
+				return
+			}
+		}
+	}
+}
+
 // handleInputWS receives raw sidecar protocol frames from the browser —
 // the real-time path for streamed touches during manual driving. Each
 // binary message must be exactly one valid frame; malformed frames are
@@ -94,6 +136,12 @@ func (s *Server) handleInputWS(w http.ResponseWriter, r *http.Request) {
 	}
 	defer release()
 
+	// Only after the claim: a refused connection has nothing to release,
+	// and pinging it would keep a doomed socket alive for no reason.
+	liveCtx, stopWatching := context.WithCancel(ctx)
+	defer stopWatching()
+	go watchLiveness(liveCtx, conn, inputPingInterval, inputPingTimeout)
+
 	var held heldTouch
 	// Closure, not a direct defer: deferred arguments evaluate at defer
 	// time, when nothing is held yet.

internal/server/ws_test.go

@@ -366,3 +366,67 @@ func TestInputWSReleasesOnDisconnect(t *testing.T) {
 		}
 	}
 }
+
+// wsAcceptOnce serves exactly one upgrade and hands the connection to fn,
+// signalling when fn returns.
+func wsAcceptOnce(t *testing.T, fn func(*websocket.Conn)) (addr string, done <-chan struct{}) {
+	t.Helper()
+	finished := make(chan struct{})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		conn, err := websocket.Accept(w, r, nil)
+		if err != nil {
+			return
+		}
+		defer conn.CloseNow()
+		fn(conn)
+		close(finished)
+	}))
+	t.Cleanup(srv.Close)
+	return "ws" + strings.TrimPrefix(srv.URL, "http"), finished
+}
+
+// A peer that stops answering is the case that stranded a real device for
+// 22 hours: the socket stayed open, Read blocked for ever, and the claim
+// outlived the client that made it. Only an unanswered ping reveals it.
+func TestWatchLivenessGivesUpOnASilentPeer(t *testing.T) {
+	addr, done := wsAcceptOnce(t, func(conn *websocket.Conn) {
+		watchLiveness(context.Background(), conn, 5*time.Millisecond, 40*time.Millisecond)
+	})
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	conn, _, err := websocket.Dial(ctx, addr, nil)
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer conn.CloseNow()
+	// Deliberately never read: coder/websocket answers pings from the read
+	// path, so a client that is not reading is indistinguishable from one
+	// that is gone — which is exactly the condition being tested.
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("watchLiveness never gave up on a peer that stopped answering")
+	}
+}
+
+// Cancellation is the ordinary exit: the handler returned, the claim is
+// already being released, and the watcher must not outlive it.
+func TestWatchLivenessStopsWhenTheHandlerReturns(t *testing.T) {
+	addr, done := wsAcceptOnce(t, func(conn *websocket.Conn) {
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel()
+		watchLiveness(ctx, conn, time.Hour, time.Hour)
+	})
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	conn, _, err := websocket.Dial(ctx, addr, nil)
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer conn.CloseNow()
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("watchLiveness ignored a cancelled context")
+	}
+}

internal/web/static/device.html

@@ -26,13 +26,45 @@
       user-select: none;
       cursor: default;
       font: 10px/1 sans-serif;
+      /* A native container's frame spans its children, so mirroring the
+         tree faithfully stacks big transparent divs over the leaves they
+         contain — and a later sibling wins the hit test. Playwright then
+         refuses to click a perfectly visible control because "another
+         element intercepts pointer events". Containers must therefore be
+         click-through and only leaves clickable. Clicking a container
+         still works: the hit lands on a descendant, which the actionability
+         check accepts. `hittable` cannot serve here — iOS reports it false
+         for whole visible subtrees (see device.js). */
+      pointer-events: none;
     }
+    #mirror [data-dd-node]:not(:has([data-dd-node])) { pointer-events: auto; }
+    /* A refused page looks identical to a working one: the video still
+       streams, the mirror still updates, and only the touches go nowhere.
+       Someone driving by hand reads that as "DeviceDeck is broken".
+
+       The text is drawn from an attribute via ::after rather than written
+       into the DOM. aria-hidden alone was measured to be insufficient: it
+       hides the notice from getByRole and from an ARIA snapshot, but
+       getByText still matched it, which would let our sentence satisfy an
+       assertion meant for the app. Generated content has no text node, so
+       there is nothing for a text query to find. */
+    #notice {
+      position: absolute; top: 0; left: 0; right: 0;
+      padding: 10px 14px;
+      background: #b3261e; color: #fff;
+      font: 13px/1.4 -apple-system, system-ui, sans-serif;
+      pointer-events: none;
+      display: none;
+    }
+    #notice[data-shown] { display: block; }
+    #notice::after { content: attr(data-reason); }
   </style>
 </head>
 <body>
   <div id="stage">
     <canvas id="video"></canvas>
     <div id="mirror" role="application" aria-label="device"></div>
+    <div id="notice" aria-hidden="true"></div>
   </div>
   <script src="/video-common.js"></script>
   <script src="/input-common.js"></script>

internal/web/static/device.js

@@ -36,13 +36,30 @@ function connectVideo() {
   ws.onclose = () => setTimeout(connectVideo, 1500);
 }
 
+// showNotice puts a human-readable reason on screen. The text goes into
+// an attribute and is painted by CSS generated content, so the page gains
+// no text node a locator could match — see device.html for why aria-hidden
+// alone was not enough.
+function showNotice(text) {
+  const notice = document.getElementById("notice");
+  notice.setAttribute("data-reason", text);
+  notice.setAttribute("data-shown", "");
+}
+
+// The server's reason travels in a close frame, which holds 123 bytes —
+// so the remedy is exactly the part that gets truncated away. The page
+// knows the remedy without being told, so it says it itself.
+const REFUSAL_REMEDY =
+  "Close the other DeviceDeck tab or test run for this device, then reload.";
+
 function connectInput() {
   input = createInputSocket(wsURL(`/api/devices/${udid}/input`), {
     // Surfaced in the title so a driver that cannot drive says so
     // somewhere a test's failure message will show it.
     onRefused: (reason) => {
       document.title = `DeviceDeck — ${reason || "device busy"}`;
       mirror.setAttribute("data-dd-input-refused", reason || "device busy");
+      showNotice(`${reason || "Another client is driving this device."} — ${REFUSAL_REMEDY}`);
     },
   });
 }

internal/web/web_test.go

@@ -101,3 +101,38 @@ func TestDevicePageCarriesNoControlsOfOurOwn(t *testing.T) {
 		}
 	}
 }
+
+// The refusal notice is the one piece of our own wording on the automation
+// page, so it has to be provably invisible to a locator. Two things make
+// that true, and both were needed: aria-hidden keeps it out of getByRole
+// and the ARIA snapshot, and keeping the element empty keeps it out of
+// getByText — which was measured still matching an aria-hidden node. A
+// refused page must never offer an agent a sentence the app never had.
+func TestDeviceNoticeIsHiddenFromAutomation(t *testing.T) {
+	body := get(t, "/device/EB69B42A-4763-4A33-AF0F-CD233F721951").Body.String()
+	start := strings.Index(body, "<body>")
+	end := strings.Index(body, "<script")
+	if start < 0 || end < 0 {
+		t.Fatal("device page shape changed; this guard needs updating")
+	}
+	markup := body[start:end]
+	notice := strings.Index(markup, `id="notice"`)
+	if notice < 0 {
+		t.Fatal("device page has no refusal notice; a refused page would look merely broken")
+	}
+	// The attribute must sit on the notice element itself.
+	tagStart := strings.LastIndex(markup[:notice], "<")
+	tagEnd := strings.Index(markup[notice:], ">") + notice
+	if tag := markup[tagStart : tagEnd+1]; !strings.Contains(tag, `aria-hidden="true"`) {
+		t.Errorf("notice is not aria-hidden, so automation would read it as app content: %s", tag)
+	}
+	// It must also live outside the mirror, which is the app's subtree.
+	if mirror := strings.Index(markup, `id="mirror"`); mirror > notice {
+		t.Error("notice precedes the mirror; it must not sit inside the app subtree")
+	}
+	// Empty element: the wording is painted by CSS from an attribute, so
+	// there is no text node for getByText to match.
+	if body := markup[tagEnd+1:]; !strings.HasPrefix(strings.TrimSpace(body), "</div>") {
+		t.Errorf("notice carries text in the DOM, which getByText would match: %.60s", body)
+	}
+}