From fedd6ed161cb203ebee5d4aedb55ba9c5d1297d4 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 27 Aug 2026 22:45:16 -0700 Subject: [PATCH 1/2] Notice a new build without being told Checking only on a 401 was not automatic, just deferred. It needs the tab to make a request, so one left open and idle went on running the old build until somebody touched it -- which is exactly the thing that cannot be relied on. The obvious signal turned out to be the wrong one, and testing is what showed it. A deploy kills the EventSource behind /api/events, which looks like the perfect cue, except it arrives while the container is still being replaced: the check that follows cannot reach the server, fails, and is never retried. Waiting for the stream to come back instead does not work either, because the session died with the old container, so the reconnect is answered with a 401 and never reaches "connected" at all. The drop is still watched, since it costs nothing and sometimes lands late enough to be useful, but nothing depends on it. What the guarantee rests on is a slow poll while the tab is visible, plus a check when it becomes visible again. Neither cares what the stream is doing or whether anyone is at the keyboard. /api/health touches nothing upstream, so a minute between checks costs one small request per open tab. Reloading is now something that happens to people rather than something they ask for, which makes it able to destroy work. A compose window holds text that has not reached the server, and after a deploy it cannot be saved at all -- the session went with the container. Reloading would be the difference between signing in again and pressing send, and losing what was written. So anything holding such state can say so, and compose does; the tab stays on the old build until the draft is dealt with, and catches up on the next check afterwards. --- web/src/lib/__tests__/staleBuild.test.ts | 82 +++++++++++++++++++- web/src/lib/staleBuild.ts | 96 +++++++++++++++++++++++- web/src/main.tsx | 3 + web/src/store/compose.ts | 6 ++ 4 files changed, 185 insertions(+), 2 deletions(-) diff --git a/web/src/lib/__tests__/staleBuild.test.ts b/web/src/lib/__tests__/staleBuild.test.ts index a265d36..8a62635 100644 --- a/web/src/lib/__tests__/staleBuild.test.ts +++ b/web/src/lib/__tests__/staleBuild.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { reloadIfServerRebuilt } from "@/lib/staleBuild"; +import { reloadIfServerRebuilt, holdReloadWhile, makeConnectionWatcher, startBuildWatch } from "@/lib/staleBuild"; import { APP_VERSION } from "@/lib/version"; function healthReplies(body: unknown, ok = true) { @@ -65,3 +65,83 @@ describe("reloadIfServerRebuilt", () => { expect(reload).not.toHaveBeenCalled(); }); }); + +describe("unsaved work holds the page", () => { + it("does not reload while something says it has unsaved work", async () => { + const release = holdReloadWhile(() => true); + vi.stubGlobal("fetch", healthReplies({ ok: true, version: "9.9.9" })); + expect(await reloadIfServerRebuilt()).toBe(false); + expect(reload).not.toHaveBeenCalled(); + release(); + expect(await reloadIfServerRebuilt()).toBe(true); + expect(reload).toHaveBeenCalledOnce(); + }); + + it("treats a predicate that throws as a reason to wait", async () => { + const release = holdReloadWhile(() => { + throw new Error("broken"); + }); + vi.stubGlobal("fetch", healthReplies({ ok: true, version: "9.9.9" })); + expect(await reloadIfServerRebuilt()).toBe(false); + release(); + }); +}); + +describe("noticing without being asked", () => { + it("checks when the push stream drops, but not before it has connected", async () => { + const fetchMock = healthReplies({ ok: true, version: APP_VERSION }); + vi.stubGlobal("fetch", fetchMock); + const onState = makeConnectionWatcher(); + + // never connected: a disconnect is not news + onState("connecting"); + await new Promise((r) => setTimeout(r, 0)); + expect(fetchMock).not.toHaveBeenCalled(); + + onState("connected"); + onState("connecting"); + await new Promise((r) => setTimeout(r, 0)); + expect(fetchMock).toHaveBeenCalled(); + }); + + it("asks the server once when several things notice at the same moment", async () => { + const fetchMock = healthReplies({ ok: true, version: APP_VERSION }); + vi.stubGlobal("fetch", fetchMock); + await Promise.all([reloadIfServerRebuilt(), reloadIfServerRebuilt(), reloadIfServerRebuilt()]); + expect(fetchMock).toHaveBeenCalledOnce(); + }); +}); + +describe("the poll is what the guarantee rests on", () => { + it("checks on its own while the tab is visible, with nobody touching it", async () => { + vi.useFakeTimers(); + const fetchMock = healthReplies({ ok: true, version: "9.9.9" }); + vi.stubGlobal("fetch", fetchMock); + Object.defineProperty(document, "visibilityState", { configurable: true, get: () => "visible" }); + + startBuildWatch(); + expect(fetchMock).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(60_000); + expect(fetchMock).toHaveBeenCalled(); + vi.useRealTimers(); + }); + + it("leaves a hidden tab alone until it is looked at", async () => { + vi.useFakeTimers(); + const fetchMock = healthReplies({ ok: true, version: APP_VERSION }); + vi.stubGlobal("fetch", fetchMock); + let visibility = "hidden"; + Object.defineProperty(document, "visibilityState", { configurable: true, get: () => visibility }); + + startBuildWatch(); + await vi.advanceTimersByTimeAsync(180_000); + expect(fetchMock).not.toHaveBeenCalled(); + + visibility = "visible"; + document.dispatchEvent(new Event("visibilitychange")); + await vi.advanceTimersByTimeAsync(0); + expect(fetchMock).toHaveBeenCalled(); + vi.useRealTimers(); + }); +}); diff --git a/web/src/lib/staleBuild.ts b/web/src/lib/staleBuild.ts index 0c208b3..9c69ae3 100644 --- a/web/src/lib/staleBuild.ts +++ b/web/src/lib/staleBuild.ts @@ -1,4 +1,5 @@ import { APP_VERSION } from "./version"; +import { push, type PushState } from "@/jmap/push"; /** * Reload the page when the server is serving a build this one did not come @@ -45,12 +46,52 @@ function forget(): void { } } +/** + * Reasons to leave a stale page alone for now. + * + * A reload throws away everything the tab has not sent anywhere, and on an + * immutable instance the session is gone by the time we get here, so a compose + * window holding text that never reached the server cannot save it either. + * Reloading would be the difference between the author signing in again and + * pressing send, and losing what they wrote. Whoever owns such state says so + * here; see the registration at the bottom of `store/compose.ts`. + */ +const holds = new Set<() => boolean>(); + +export function holdReloadWhile(fn: () => boolean): () => void { + holds.add(fn); + return () => holds.delete(fn); +} + +function held(): boolean { + for (const fn of holds) { + try { + if (fn()) return true; + } catch { + /* a broken predicate is not a reason to reload over someone's work */ + return true; + } + } + return false; +} + +let inFlight: Promise | null = null; + /** * True when a reload has been asked for and the caller should leave the page * alone. False for every other outcome, including not being able to tell -- * failing to reach the server is not a reason to throw away what is on screen. */ -export async function reloadIfServerRebuilt(): Promise { +export function reloadIfServerRebuilt(): Promise { + // Several things can notice a deploy at once -- the stream dropping and the + // request that follows it -- and they should not each ask the server. + inFlight ??= check().finally(() => { + inFlight = null; + }); + return inFlight; +} + +async function check(): Promise { let serverVersion: string; try { const res = await fetch("/api/health", { credentials: "same-origin", cache: "no-store" }); @@ -73,7 +114,60 @@ export async function reloadIfServerRebuilt(): Promise { // still reports the old version -- a stale proxy cache, a half-finished // deploy -- this stops the two of them reloading each other in a loop. if (tried() === serverVersion) return false; + if (held()) return false; remember(serverVersion); window.location.reload(); return true; } + +/** + * Watch for a deploy without waiting to be asked. + * + * Checking on a 401 alone was not automatic, only deferred: it needs the tab to + * make a request, so one sitting idle keeps running the old build until someone + * touches it. + * + * The obvious signal turned out to be the wrong one. A deploy kills the + * EventSource behind `/api/events`, which looks like the perfect cue -- except + * it arrives while the container is still being replaced, so the check that + * follows cannot reach the server. Waiting for the stream to come back instead + * does not work either: the session died with the old container, so the + * reconnect is answered with a 401 and never reaches "connected" at all. The + * drop is kept below because it is free and sometimes lands early enough to be + * useful, but nothing depends on it. + * + * What the guarantee rests on is a slow poll while the tab is visible, plus a + * check when it becomes visible again. Neither cares what the stream is doing + * or whether anyone is at the keyboard: a tab left open through a deploy + * notices within a minute, and a backgrounded one notices the moment it is + * looked at. `/api/health` touches nothing upstream, so the cost is one small + * request a minute per open tab. + */ +const POLL_MS = 60_000; + +export function makeConnectionWatcher(): (state: PushState) => void { + let wasConnected = false; + return (state) => { + if (state === "connected") { + wasConnected = true; + return; + } + // Only a drop is news. Never having connected is not evidence of anything. + if (!wasConnected) return; + wasConnected = false; + void reloadIfServerRebuilt(); + }; +} + +export function startBuildWatch(): void { + push.onConnection(makeConnectionWatcher()); + + window.setInterval(() => { + // A hidden tab is not being read, and will be checked when it surfaces. + if (document.visibilityState === "visible") void reloadIfServerRebuilt(); + }, POLL_MS); + + document.addEventListener("visibilitychange", () => { + if (document.visibilityState === "visible") void reloadIfServerRebuilt(); + }); +} diff --git a/web/src/main.tsx b/web/src/main.tsx index 5983146..5c3f5ee 100644 --- a/web/src/main.tsx +++ b/web/src/main.tsx @@ -2,6 +2,9 @@ import { StrictMode } from "react"; import { createRoot } from "react-dom/client"; import "./styles/app.css"; import { App } from "./App"; +import { startBuildWatch } from "@/lib/staleBuild"; + +startBuildWatch(); createRoot(document.getElementById("root")!).render( diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index 8330874..b73ae2d 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -10,6 +10,7 @@ import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; import { ensureScheduledMailbox, useScheduled } from "./scheduled"; import { formatScheduleTime, holdUntil } from "@/lib/schedule"; import { settings } from "./settings"; +import { holdReloadWhile } from "@/lib/staleBuild"; export interface ComposeAttachment { id: string; @@ -807,3 +808,8 @@ export function draftFromMailto(url: string): Partial { ...(body ? { html: body, text: m.body } : {}), }; } + +// A deploy can reload this tab out from under whoever is writing. Text that has +// not been autosaved lives only here, and once the session is gone it cannot be +// saved at all -- so say so, and let them sign in and send it instead. +holdReloadWhile(() => useCompose.getState().drafts.some((d) => d.dirty)); From 8f9d253939089d6830a6541deb3732d54f27b2f6 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Thu, 27 Aug 2026 22:51:33 -0700 Subject: [PATCH 2/2] Reload even when there is an unsent draft Holding the reload back while a compose window had unsaved text protected the text, but it meant a tab could sit on a build the server no longer runs for as long as someone left a draft open -- which is not automatic, and automatic is the point. So the reload is unconditional once the versions differ, and this will sometimes take an unsent draft with it. The trade is deliberate: a tab talking to a server it does not match is the worse failure, and it fails quietly. --- web/src/lib/__tests__/staleBuild.test.ts | 23 +------------ web/src/lib/staleBuild.ts | 44 ++++++------------------ web/src/store/compose.ts | 6 ---- 3 files changed, 11 insertions(+), 62 deletions(-) diff --git a/web/src/lib/__tests__/staleBuild.test.ts b/web/src/lib/__tests__/staleBuild.test.ts index 8a62635..2064f82 100644 --- a/web/src/lib/__tests__/staleBuild.test.ts +++ b/web/src/lib/__tests__/staleBuild.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { reloadIfServerRebuilt, holdReloadWhile, makeConnectionWatcher, startBuildWatch } from "@/lib/staleBuild"; +import { reloadIfServerRebuilt, makeConnectionWatcher, startBuildWatch } from "@/lib/staleBuild"; import { APP_VERSION } from "@/lib/version"; function healthReplies(body: unknown, ok = true) { @@ -66,27 +66,6 @@ describe("reloadIfServerRebuilt", () => { }); }); -describe("unsaved work holds the page", () => { - it("does not reload while something says it has unsaved work", async () => { - const release = holdReloadWhile(() => true); - vi.stubGlobal("fetch", healthReplies({ ok: true, version: "9.9.9" })); - expect(await reloadIfServerRebuilt()).toBe(false); - expect(reload).not.toHaveBeenCalled(); - release(); - expect(await reloadIfServerRebuilt()).toBe(true); - expect(reload).toHaveBeenCalledOnce(); - }); - - it("treats a predicate that throws as a reason to wait", async () => { - const release = holdReloadWhile(() => { - throw new Error("broken"); - }); - vi.stubGlobal("fetch", healthReplies({ ok: true, version: "9.9.9" })); - expect(await reloadIfServerRebuilt()).toBe(false); - release(); - }); -}); - describe("noticing without being asked", () => { it("checks when the push stream drops, but not before it has connected", async () => { const fetchMock = healthReplies({ ok: true, version: APP_VERSION }); diff --git a/web/src/lib/staleBuild.ts b/web/src/lib/staleBuild.ts index 9c69ae3..568ed8e 100644 --- a/web/src/lib/staleBuild.ts +++ b/web/src/lib/staleBuild.ts @@ -14,10 +14,16 @@ import { push, type PushState } from "@/jmap/push"; * * `index.html` is served `no-cache` and the assets under it are content-hashed * and immutable, so a reload is all it takes; the only missing part was - * something to ask for one. Checking on a 401 rather than on a timer keeps it - * to the moment it matters and costs one small request, and comparing versions - * rather than reloading on every 401 means an ordinary session expiry still - * lands on the sign-in form with the page intact. + * something to ask for one. Comparing versions rather than reloading on every + * 401 means an ordinary session expiry still lands on the sign-in form with the + * page intact -- only a build that actually moved costs the page. + * + * The reload is unconditional once the versions differ. A compose window can + * be holding text that never reached the server, and after a deploy it cannot + * be saved either, since the session went with the container -- so this will + * sometimes take an unsent draft with it. That is a deliberate trade: a tab + * running code the server no longer speaks is the worse failure, and one that + * stays behind because someone left a draft open is not automatic at all. */ const TRIED_KEY = "ihasmail:reloaded-for"; @@ -46,35 +52,6 @@ function forget(): void { } } -/** - * Reasons to leave a stale page alone for now. - * - * A reload throws away everything the tab has not sent anywhere, and on an - * immutable instance the session is gone by the time we get here, so a compose - * window holding text that never reached the server cannot save it either. - * Reloading would be the difference between the author signing in again and - * pressing send, and losing what they wrote. Whoever owns such state says so - * here; see the registration at the bottom of `store/compose.ts`. - */ -const holds = new Set<() => boolean>(); - -export function holdReloadWhile(fn: () => boolean): () => void { - holds.add(fn); - return () => holds.delete(fn); -} - -function held(): boolean { - for (const fn of holds) { - try { - if (fn()) return true; - } catch { - /* a broken predicate is not a reason to reload over someone's work */ - return true; - } - } - return false; -} - let inFlight: Promise | null = null; /** @@ -114,7 +91,6 @@ async function check(): Promise { // still reports the old version -- a stale proxy cache, a half-finished // deploy -- this stops the two of them reloading each other in a loop. if (tried() === serverVersion) return false; - if (held()) return false; remember(serverVersion); window.location.reload(); return true; diff --git a/web/src/store/compose.ts b/web/src/store/compose.ts index b73ae2d..8330874 100644 --- a/web/src/store/compose.ts +++ b/web/src/store/compose.ts @@ -10,7 +10,6 @@ import { useMail, FULL_PROPS, BODY_PROPS } from "./mail"; import { ensureScheduledMailbox, useScheduled } from "./scheduled"; import { formatScheduleTime, holdUntil } from "@/lib/schedule"; import { settings } from "./settings"; -import { holdReloadWhile } from "@/lib/staleBuild"; export interface ComposeAttachment { id: string; @@ -808,8 +807,3 @@ export function draftFromMailto(url: string): Partial { ...(body ? { html: body, text: m.body } : {}), }; } - -// A deploy can reload this tab out from under whoever is writing. Text that has -// not been autosaved lives only here, and once the session is gone it cannot be -// saved at all -- so say so, and let them sign in and send it instead. -holdReloadWhile(() => useCompose.getState().drafts.some((d) => d.dirty));