diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fadfc62..45c31f1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -112,6 +112,12 @@ jobs: working-directory: windows run: node --experimental-strip-types scripts/verify-timing.mjs + # A refusal that gets overwritten by the disconnect it caused is only + # reachable with two receivers and one Mac, which is how it reached a user. + - name: Verify sender refusals survive the disconnect + working-directory: windows + run: node --experimental-strip-types scripts/verify-refusals.mjs + # Signing only when the secrets exist. A fork PR has none, and must still # get a green build rather than a confusing failure. - name: Import Developer ID certificate @@ -201,6 +207,7 @@ jobs: node scripts/verify-vectors.mjs node --experimental-strip-types scripts/verify-window-states.mjs node --experimental-strip-types scripts/verify-timing.mjs + node --experimental-strip-types scripts/verify-refusals.mjs # Ahead of the bundle step on purpose. The Rust backend uses Windows-only # APIs (DXGI Desktop Duplication, Task 8.1) that cannot be compiled on a diff --git a/windows/scripts/verify-refusals.mjs b/windows/scripts/verify-refusals.mjs new file mode 100644 index 0000000..380d74c --- /dev/null +++ b/windows/scripts/verify-refusals.mjs @@ -0,0 +1,72 @@ +/** + * Explanations that have to outlive the disconnect that causes them. + * + * Run with `node --experimental-strip-types scripts/verify-refusals.mjs`. + * + * This exists because of a real session lost to it. The Mac refuses a second + * receiver with `busy` and then closes the socket. The receiver displayed the + * refusal correctly — and roughly a hundred milliseconds later the disconnect + * handler wrote "Disconnected. Reconnecting…" straight over it. What was left + * was a spinner that never resolved, which reads as a broken app rather than an + * occupied one, and the information needed to fix it in one click had been on + * screen and was erased. + * + * Nothing about that is reachable without two receivers and one Mac, which is + * why it survived to reach a user. + */ +import assert from "node:assert/strict"; + +const { refusalExplanation, disconnectStatus } = await import("../src/errors.ts"); + +let checks = 0; +function check(name, fn) { + fn(); + checks++; + console.log(` ok ${name}`); +} + +// ------------------------------------------------------------ the regression + +check("a busy sender survives the disconnect it triggers", () => { + const sticky = refusalExplanation("busy"); + assert.ok(sticky, "busy must produce an explanation"); + assert.equal( + disconnectStatus(sticky), + sticky, + "the disconnect that follows a refusal must not overwrite it" + ); +}); + +check("the busy message says what to do, not just what happened", () => { + const text = refusalExplanation("busy").toLowerCase(); + assert.ok( + text.includes("close") || text.includes("wait"), + `no action offered: ${text}` + ); + // The raw sender text names the mechanism; the user needs the remedy. + assert.ok( + !text.includes("another receiver is already connected"), + "this is the raw wire message, which describes plumbing rather than a fix" + ); +}); + +// ------------------------------------------------------- the ordinary case + +check("an ordinary disconnect still says it is reconnecting", () => { + assert.equal(disconnectStatus(null), "Disconnected. Reconnecting…"); +}); + +check("a transient failure is not pinned to the screen", () => { + // Anything without a deliberate explanation must return null, so a stale + // reason cannot outlive the moment it stopped being true. + for (const code of ["", "unknown", "input_unavailable", "internal", "timeout"]) { + assert.equal( + refusalExplanation(code), + null, + `${code} must not become sticky` + ); + assert.equal(disconnectStatus(refusalExplanation(code)), "Disconnected. Reconnecting…"); + } +}); + +console.log(`\n${checks} checks passed`); diff --git a/windows/src/errors.ts b/windows/src/errors.ts index 8d2f047..4b06338 100644 --- a/windows/src/errors.ts +++ b/windows/src/errors.ts @@ -121,3 +121,47 @@ export function backoffFor(attempt: number): number | null { if (attempt < 1) return RETRY_BACKOFF_MS[0]; return attempt <= RETRY_BACKOFF_MS.length ? RETRY_BACKOFF_MS[attempt - 1] : null; } + +/** + * An explanation from the SENDER that has to outlive the disconnect it causes. + * + * The Mac refuses a second receiver with `busy` and then closes the socket. Both + * halves of that are correct — but the close fires `ds://disconnected`, whose + * handler wrote "Disconnected. Reconnecting…" straight over the explanation. So + * the receiver knew exactly what was wrong, said so, and then erased it about a + * hundred milliseconds later. + * + * What the user is left with is a spinner that never resolves, which reads as a + * broken app rather than an occupied one. It cost a real session to diagnose, + * and the information needed was on screen the whole time. + * + * Returns `null` for codes that should not survive a disconnect — a transient + * failure must not leave a stale explanation pinned to the screen once it stops + * being true. + */ +export function refusalExplanation(code: string): string | null { + switch (code) { + case "busy": + // Deliberately not the raw "another receiver is already connected": that + // says what happened, not what to do about it, and retrying continues in + // the background so the wait is a real option. + return ( + "Another screen is already connected to this Mac. " + + "Close it, or wait — this keeps trying." + ); + default: + return null; + } +} + +/** + * What to show when a session ends. + * + * `sticky` is whatever `refusalExplanation` last produced. Passing it here + * rather than checking at the call site is the point: there is now one place + * that decides whether a disconnect may overwrite what is on screen, instead of + * one flag per reason, added each time somebody notices another one. + */ +export function disconnectStatus(sticky: string | null): string { + return sticky ?? "Disconnected. Reconnecting…"; +} diff --git a/windows/src/main.ts b/windows/src/main.ts index 954c7d5..37560d2 100644 --- a/windows/src/main.ts +++ b/windows/src/main.ts @@ -13,7 +13,7 @@ import { type ReceiverPanel, } from "./protocol"; import { InputCapture } from "./input"; -import { backoffFor, humanise } from "./errors"; +import { backoffFor, disconnectStatus, humanise, refusalExplanation } from "./errors"; import { installDisabledGuard, setEnabled, setVariant } from "./components/controls"; import { applyWindowClasses } from "./components/window"; import { @@ -441,13 +441,14 @@ function measureRefreshRate(): Promise { }); } -/// The Mac took its screen away on purpose, so protected video would play. +/// An explanation from the Mac that must outlive the disconnect it causes. /// -/// Held across the disconnect that follows: the socket closes a moment later, -/// and without this the reconnect handler would replace a true explanation with -/// "Reconnecting…" — which is both wrong and worrying. Nothing is broken, and -/// nothing will reconnect until someone at the Mac asks for it. -let releasedBySender = false; +/// Every reason the sender closes a session arrives as a control message and is +/// then followed, immediately, by the close itself — so without somewhere to +/// keep it, the reconnect handler overwrites the one useful thing on screen. +/// Started as a single flag for "the screen was released"; it needed to be +/// general the moment a second reason turned up, and `busy` was that reason. +let stickyStatus: string | null = null; listen("ds://control", (event) => { let message: ControlMessage; @@ -458,15 +459,15 @@ listen("ds://control", (event) => { } switch (message.type) { case "welcome": - releasedBySender = false; + // Connected, so nothing from a previous attempt still applies. + stickyStatus = null; setStatus("", false); break; case "display_released": - releasedBySender = true; - setStatus( + stickyStatus = "The Mac released this screen so protected video can play. " + - "Start sharing there to bring it back." - ); + "Start sharing there to bring it back."; + setStatus(stickyStatus); break; case "pointer_release": // The Mac says the cursor came home. Drop the lock and resume absolute @@ -502,7 +503,11 @@ listen("ds://control", (event) => { } else if (message.code === "pair_rejected") { showPinPrompt(message.message ?? "Incorrect PIN.", true); } else { - setStatus(`${message.code}: ${message.message}`); + // A refusal the sender means to stick: shown now AND kept, because the + // close that follows would otherwise wipe it. + const explained = refusalExplanation(message.code ?? ""); + stickyStatus = explained; + setStatus(explained ?? `${message.code}: ${message.message}`); } break; default: @@ -519,7 +524,7 @@ listen("ds://handoff", (event) => { }); listen("ds://disconnected", () => { - if (!releasedBySender) setStatus("Disconnected. Reconnecting…"); + setStatus(disconnectStatus(stickyStatus)); // A new session starts a new sampler on the Rust side, and its stamps must // not be paired against this session's arrivals. handoffMeter.reset();