fix(web): an async message listener does not serialize, and a watchdog must not overwrite a real error

Three findings from the review-only PR #13, which put 28 already-merged commits in front of a second reader, plus two comments that had gone false. worker.js — `self.onmessage` was `async` and awaited `handle()`, which awaits `server.flush()`. An async listener does NOT serialize: the event loop may start the next `message` while an earlier one is suspended at an await, so two `on_message`/`flush` pairs interleave and a later frame's replies can be posted before an earlier frame's. That breaks the one rule the file names first — a `GridUpdate::Cells` that overtakes its labels is DROPPED by the client's fold, not replayed (gotcha 33) — and the pre-start `pending` drain had the same race twice over, since `server` was assigned before the loop that drained it, so a message arriving mid-drain overtook the frames it was queued behind. Every frame is now chained onto one tail promise, which also makes "arrived before start() finished" an ordinary case of the same rule instead of a second path. app.js — the handshake watchdog fires 8s after the handshake and writes "the peer did not answer". A peer that failed LOUDLY has already said something more precise (worker `error`, `messageerror`, socket `error`), and the watchdog then overwrote it with the vaguer guess. Those three paths now disarm it: `setShellFailure` clears the timer rather than merely pre-empting it. The watchdog is for silence only, which is what it was always for. README — the "In the browser" section still said the deployed page needs a server and that the in-page worker "is not built yet". Both landed in the same unreviewed batch (improvise-9x6, improvise-djm), so the README described the published site as a client with nowhere to connect while index.html described it correctly. Rewritten, and it now states the caveat the commits state: no line of that JavaScript has ever run. Two comments that had gone false with it, and for the same reason — a follow-up commit updated one copy and not the others. `web/app.js`'s keyboard note said key resolution was "text entry, and nothing else" and that upstream keys would work "until improvise-bpq2 lands"; bpq2 landed, moved the grid, panel and tile cursors to the client, and settled that the host must not know the keymap. `Host::handle`'s `ClientFrame::Key` arm said the same thing; 43422d4 corrected the worker's copy of that argument and missed this one. NOT VERIFIED, and it cannot be here: there is no browser and no headless DOM in this environment, so neither JavaScript change has been executed. Both are argued rather than tested, which is why each carries its argument in the file. `node --check` passes on both, the wasm bundles build, and the Rust gates are unchanged: 1172 passing / 0 failed, clippy clean, fmt clean, actionlint clean, wasm32 clean for all seven bundle crates. Declined from the same review, with the reason on the thread: `Host::commands` can apply a batch's earlier commands before rejecting a later one. The finding is correct, the hole is real, and it is already stated on `ClientFrame::Commands` and in `Host::commands` — closing it means the basis check moving from the command to the batch, which is a design change with ADR 001 consequences and not a patch to land beside three small ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
2026-09-19 20:30:18 -07:00
parent d0cecff1be
commit bf02472d89
4 changed files with 98 additions and 56 deletions
@@ -183,27 +183,38 @@ and published from `main` by the `pages-build` / `pages-deploy` jobs in
 `.github/workflows/ci.yml`. A deploy only happens after `cargo test` and the
 wasm32 check have passed.

-**It needs a server, and a static host is not one.** The page is a thin client:
-it holds no model, and every question about a cell — its text, whether it may
-be typed into — is answered by a server and arrives already decided. So the
-published page paints an empty grid until it is pointed at one:
+**It needs no server.** The page is still a thin client — it holds no model,
+and every question about a cell, its text and whether it may be typed into, is
+answered by a server and arrives already decided — but the server it talks to
+is an in-page Web Worker hosting the whole `App` over the browser's private
+storage (OPFS). It is seeded on first load from `web/demo.improv`, so a visitor
+lands on a document, and edits persist across a reload without leaving the
+machine. `?doc=` and `?seed=` name another document and the file it is first
+seeded from.
+
+To run the same page locally:
+
+```sh
+./web/build.sh                              # wasm-pack -> web/{pkg,worker-pkg}
+python3 -m http.server 8080 --directory web
+```
+
+To open a document hosted by a separate process instead — which is how a
+document open in the TUI is shared — start the websocket server and pass
+`?ws=`:

 ```sh
 cargo run -p improvise-ws-server -- examples/demo.improv --port 8787
 ```

-then open the page with `?ws=ws://127.0.0.1:8787`. Over https a `ws://`
-address is blocked as mixed content, so for now a local server is most easily
-paired with a local page:
+then reload with `?ws=ws://127.0.0.1:8787`. From an https origin a `ws://`
+address is blocked as mixed content, so that pairing is most easily done with
+the local page above.

-```sh
-./web/build.sh                              # wasm-pack -> web/pkg (gitignored)
-python3 -m http.server 8080 --directory web
-```
-
-Running with no separate process is the in-page worker, improvise-9x6, which
-is not built yet. Until it lands, what is deployed is the delivery pipeline,
-not a working demo.
+Caveat worth stating plainly: **no line of the browser JavaScript has ever
+run.** There is no browser and no headless DOM in the development environment,
+so `web/app.js`, `web/worker.js` and the OPFS backend are compile-checked,
+syntax-checked and read, never executed.

 ## What's interesting about the architecture

@@ -143,15 +143,19 @@ impl Host {
        match msg {
            ClientFrame::Viewport { rows, cols } => self.resize(id, rows, cols),
            ClientFrame::Commands(cmds) => self.commands(id, &cmds),
-            // Key forwarding over this socket is improvise-bpq2 and is not
-            // built: resolving a keypress needs `reduce_key` against the
-            // authoritative `App`, and the `Dirty`/broadcast plumbing that
-            // `command` gets from `Sessions::apply` has no counterpart there
-            // yet. This arm answers nothing, which is exactly what the
-            // socket did before the envelopes were shared — the frame used
-            // to fail to parse and be skipped by `next_client_msg`. The
-            // difference is that the gap is now a line of code with a ticket
-            // number on it rather than a variant that does not exist.
+            // This arm answers nothing, and since improvise-bpq2 that is the
+            // settled answer rather than a gap. Resolving a keypress here
+            // needs `reduce_key` against the authoritative `App`, which emits
+            // no `Command` a client could fold — so the two would diverge,
+            // which `improvise-worker-server`'s
+            // `a_key_resolved_on_the_host_moves_state_no_client_can_hear`
+            // demonstrates. bpq2 closed that by moving resolution to the
+            // client, so a key a client can answer never arrives as a `Key`
+            // frame at all, and one that does is a binding nothing on this
+            // side could answer without diverging. Growing the set is a
+            // matter of enriching the projection until the client can
+            // compose the command itself — see `improvise-wasm-client::keys`,
+            // which carries the list of what is still upstream.
            ClientFrame::Key { .. } => Vec::new(),
        }
    }
@@ -110,6 +110,16 @@ function armWatchdog(message) {
  watchdog = setTimeout(() => setShellStatus(message), HANDSHAKE_TIMEOUT_MS);
 }

+// A peer that failed *loudly* has already said something more precise than
+// the watchdog's guess, so the watchdog must be disarmed and not merely
+// pre-empted: it fires later and would overwrite the better message with
+// "did not answer" (PR #13 review). The watchdog is for silence only.
+function setShellFailure(text) {
+  clearTimeout(watchdog);
+  watchdog = null;
+  setShellStatus(text);
+}
+
 /** One frame, one fold, in arrival order. Never batch or reorder these. */
 function onFrame(text) {
  clearTimeout(watchdog);
@@ -136,10 +146,10 @@ function workerTransport() {
  const worker = new Worker(`./worker.js?${query}`, { type: "module" });
  worker.addEventListener("message", (event) => onFrame(event.data));
  worker.addEventListener("messageerror", () =>
-    setShellStatus("the worker sent a frame this page could not read"),
+    setShellFailure("the worker sent a frame this page could not read"),
  );
  worker.addEventListener("error", (event) => {
-    setShellStatus(`worker failed to start: ${event.message ?? "see console"}`);
+    setShellFailure(`worker failed to start: ${event.message ?? "see console"}`);
    console.error("improvise: worker error", event);
  });
  setShellStatus(`in-page worker · ${DOC}`);
@@ -161,7 +171,7 @@ function socketTransport() {
  // Without this, a connection that never opens left #status blank and
  // "server not running" read exactly like "still connecting" (improvise-hwpi).
  socket.addEventListener("error", () =>
-    setShellStatus(`cannot reach ${WS_URL} — is improvise-ws-server running?`),
+    setShellFailure(`cannot reach ${WS_URL} — is improvise-ws-server running?`),
  );
  socket.addEventListener("close", () => setShellStatus("disconnected"));
  setShellStatus(`connecting to ${WS_URL}…`);
@@ -293,11 +303,13 @@ function categoryList(model) {
 // ── the keyboard ─────────────────────────────────────────────────────────
 //
 // `Client::on_key_event` classifies the event and folds it locally where this
-// client may answer it (text entry, and nothing else — see keys.rs); anything
-// else becomes a `ClientFrame::Key` for the peer to resolve against the real
-// App. Until improvise-bpq2 lands, `Host::handle` answers that frame with an
-// empty vec, so an upstream key is sent and correctly does nothing yet. That
-// is a gap in the peer, not in this listener.
+// client may answer it — text entry, and since improvise-bpq2 the grid, panel
+// and tile cursors too, which is why `sendViewport` must call `set_viewport`
+// and not `Client.viewport` (keys.rs carries the list, and what is still
+// upstream). Anything it cannot answer becomes a `ClientFrame::Key`, which
+// `Host::handle` answers with an empty vec: those keys are sent and do
+// nothing. That is settled rather than unfinished — the host must not know
+// the keymap — and closing one means teaching this client, not that arm.

 window.addEventListener("keydown", (e) => {
  // The inline editor owns the keyboard while it is open. It stops
@@ -16,6 +16,17 @@
 //   2. Await `flush()` after each message. `Storage::delete` is visible to
 //      `read`/`list` immediately but only reaches OPFS here (gotcha 28).
 //
+// Rule 1 is about frames within one call's replies AND about the calls
+// themselves, which is why every message is chained onto one tail promise
+// below rather than handled by an `async` listener: an `async` listener does
+// not serialize. The event loop is free to start the next `message` while an
+// earlier one is suspended at an `await`, so two `on_message`/`flush` pairs
+// interleave and a later frame's replies can be posted before an earlier
+// frame's. The chain is also what makes "frames that arrived before start()
+// finished" an ordinary case of the same rule instead of a second code path
+// with its own drain loop — and that loop had the race in it twice over,
+// since `server` was set before it ran.
+//
 // The document is named in this worker's own URL — `new
 // Worker('worker.js?doc=demo.improv')` — the analogue of the socket server
 // naming it in the ws:// path. It is deliberately not a `ClientFrame`: the
@@ -44,10 +55,6 @@ async function seedBytes() {
 }

 let server = null;
-// Frames that arrived before `start()` finished. The client sends its
-// `Viewport` as soon as it loads, and losing it would leave the page showing
-// an empty grid forever.
-let pending = [];

 function post(replies) {
  for (const reply of replies) self.postMessage(reply);
@@ -59,27 +66,35 @@ async function handle(data) {
  await server.flush();
 }

-self.onmessage = async (event) => {
-  if (!server) {
-    pending.push(event.data);
-    return;
-  }
-  try {
-    await handle(event.data);
-  } catch (err) {
-    // An unhandled rejection inside a `message` listener reaches the devtools
-    // console and nothing else. Say which frame it was; the main thread cannot
-    // see this and has no way to ask.
-    console.error("improvise worker: frame rejected", event.data, err);
-  }
-};
-
-(async () => {
+// Startup, and the head of the chain. The client sends its `Viewport` as soon
+// as it loads, so frames arrive before this settles; chaining onto it is what
+// holds them, in arrival order, without a queue to drain.
+const started = (async () => {
  await init();
  server = await start(DOC, await seedBytes());
-  const queued = pending;
-  pending = [];
-  for (const data of queued) await handle(data);
-})().catch((err) => {
+})();
+started.catch((err) => {
  console.error("improvise worker: failed to start", err);
 });
+
+// One frame at a time, in arrival order: the listener is deliberately NOT
+// `async`. It appends to the chain and returns, so the ordering does not
+// depend on when the event loop chooses to resume a suspended listener.
+let tail = started.catch(() => {});
+
+self.onmessage = (event) => {
+  tail = tail
+    .then(async () => {
+      // `start()` failed. There is nothing to answer with, and the main
+      // thread's handshake watchdog is what tells the user.
+      if (!server) return;
+      await handle(event.data);
+    })
+    .catch((err) => {
+      // An unhandled rejection inside a `message` listener reaches the
+      // devtools console and nothing else. Say which frame it was; the main
+      // thread cannot see this and has no way to ask. Caught here rather than
+      // left on the chain so one bad frame does not poison every later one.
+      console.error("improvise worker: frame rejected", event.data, err);
+    });
+};