diff --git a/context/code/clients.md b/context/code/clients.md index edd147d8..e88653f3 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1700,6 +1700,31 @@ 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, and `setShellFailure` clears the timer. +**Success is claimed by the first frame, and only if nothing failed first** +(improvise-i0v9). The worker transport used to write `in-page worker · ` +the moment it spawned the worker — 13ms after navigation, before anything had +answered — so a worker that could not start showed a success-looking status +over a blank page. It now writes `starting the worker · …`, and +`createShell().arm(message, { answer })` holds the claim until `sawFrame`. The +rule the claim obeys is **the watchdog's own state**: it is written only if the +watchdog is still armed when the first frame lands. That is not a convenience. +`worker.js` posts a `ReadOnly` (or `SeedMissing`) notice straight after +`start()` and **before** the Snapshot that answers the handshake, so an +unconditional "first frame ⇒ say we are up" would overwrite the precise reason +the notice just wrote. `fail` already disarms the watchdog, so "armed" answers +"has anything been said since arming" without a second flag to keep in step. +`fail` disarms **directly** rather than through `sawFrame`, or it would +briefly flash the success text on its way to the failure. The starting text +deliberately does not contain `in-page worker`, so the older specs asserting +that substring now need the first frame to pass. The socket path passes no +`answer`: `open` is already a real fact, and its status was never premature. + +One consequence changes what the watchdog guards. In the read-only case a +Snapshot always follows the notice, and `sawFrame` disarms the watchdog either +way, so a notice that forgot to disarm would **not** be overwritten eight +seconds later. It is overwritten **at once**, by the claim. The eight-second +overwrite survives only where no frame follows: `BootFailed`. + **Did this socket ever open?** The page has to record that itself, because nothing on the close event carries it: `readyState` is already CLOSED by the time a close listener runs, whichever happened, and a refused connection and a diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 3ce396f8..1a557e9c 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3054,6 +3054,23 @@ The third is the gotcha 48 property, and it needs `timesArmed() > 0` first or it denies an unreachable event: the precise reason must still be on screen after the 8s window, not replaced by the watchdog. `page.clock` walks past it. +**Strengthened by improvise-i0v9's second pass.** Every assertion above read the +*final* `#status`, so two failures could still pass it. A watchdog that fired +first and was then replaced by a late notice passed. So did a success claim +shown before the notice. The test now records every status with `logStatus` and +asserts, **before** the page clock is walked, that the log ends in `read-only` +and never contained `did not answer` or the exact `in-page worker · `. A +reason that arrived after the watchdog would sit in that log behind "did not +answer"; that is the "well before 8s" property, stated without a wall-clock +bound. Measured failing against `gh/main` at `da2e5ab`, whose log was +`["in-page worker · two-tab-….improv", "read-only — …"]`: the premature claim +the ticket measured at 13ms is still there once 1wft's notice exists. The +brief's rival, `onNotice` calling `setShell` instead of `fail` (the watchdog not +disarmed), fails it at the first `toContainText("read-only")`. The Snapshot's +claim replaced the reason at once, which is why the eight-second form of this +rival is now exercised by the invalid-`?doc=` spec instead (see +`context/code/clients.md`, "The watchdog"). + It runs on Chromium and Firefox — the two engines with OPFS — rather than Chromium alone, because the exclusivity semantics are the engine's and the two word the refusal completely differently. @@ -4405,3 +4422,35 @@ assertion. The tail yanks off the `Region` and `+` columns and requires the two registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on `+`, which is why that filter was removed rather than kept. The evidence entries for the host-side tests are in `command.md`. + +### the worker is not called up until its first frame has arrived + +improvise-i0v9. The worker transport wrote `in-page worker · ` when it +spawned the worker, before anything had answered. A worker that then failed +therefore showed a success-looking status over a blank page. The spec records +every `#status` with `logStatus` and requires the whole log to be exactly +`["starting the worker · …", "in-page worker · "]`. It pins the order +as well as the final text, since a final read cannot see a claim made too early. +Measured failing against `gh/main` at `da2e5ab`. The rival, restoring the +immediate `setShell(\`in-page worker · ${DOC}\`)` in `workerTransport`, fails it +on the log's first entry. The final read is `toHaveText`, not `toContainText`. +The older specs' `toContainText("in-page worker")` could not tell the starting +text from the claim if the one contained the other, and the starting text +deliberately does not. + +### only the first frame claims the peer answered, and not before it + +improvise-i0v9, the unit half of the spec above. `arm(message, { answer })` +must write nothing until `sawFrame`, then write `answer` once, however many +frames follow. Fails against `gh/main`'s `createShell`, which has no `answer` +and so never writes the claim at all. + +### a reason given before the first frame is not replaced by the frame's claim + +The order `worker.js` really produces on a locked document: the `ReadOnly` +notice is posted after `start()` and **before** the Snapshot. So `fail` then +`sawFrame` must leave the notice's text as the only thing ever shown. The rival +is the obvious fix, `sawFrame` writing `answer` unconditionally. It fails both +this test and the one above: the first on its exact log, which is then written +twice. This test passes against `gh/main` by construction, since no claim exists +there. It is not evidence of the bug; it guards against the naive fix. diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 9e393226..73feb294 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -187,6 +187,21 @@ test.describe("the standalone deployment", () => { await expect(page.locator("#grid td.editable").first()).toBeVisible(); }); + // improvise-i0v9: "in-page worker · " was written at 13ms, before the + // worker had answered anything, and read as success over a blank page. + test("the worker is not called up until its first frame has arrived", async ({ page }) => { + const doc = `first-frame-${Date.now()}.improv`; + await page.addInitScript(logStatus); + await page.goto(`/app/index.html?doc=${doc}`); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await expect(status(page)).toHaveText(`in-page worker · ${doc}`); + const log = await page.evaluate(() => window.__statusLog); + expect(log, `#status said: ${json(log)}`).toEqual([ + `starting the worker · ${doc}…`, + `in-page worker · ${doc}`, + ]); + }); + // improvise-9cma: a `?doc=` this backend cannot hold used to reach the // generic watchdog 8s later. The REASON now crosses the port (improvise-1wft). test("an invalid ?doc= is named on screen, and not blamed on the browser", async ({ @@ -235,6 +250,7 @@ test.describe("the standalone deployment", () => { const second = await context.newPage(); await second.clock.install(); + await second.addInitScript(logStatus); await second.goto(`/app/index.html?doc=${doc}`); // It PAINTS. That is the whole point: a locked document is read-only, not @@ -242,6 +258,15 @@ test.describe("the standalone deployment", () => { await expect(second.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).toContainText("another tab"); + // The page's clock has not been walked, so a reason that arrived AFTER the + // watchdog would have been logged behind "did not answer". Nor was success + // ever claimed: the Snapshot that follows the notice must not overwrite it. + const early = await second.evaluate(() => window.__statusLog); + expect(early.at(-1), `#status said: ${json(early)}`).toContain("read-only"); + expect( + early.filter((t) => t.includes("did not answer") || t === `in-page worker · ${doc}`), + `#status said: ${json(early)}`, + ).toEqual([]); // And the reason STANDS: it is not replaced 8s later by a watchdog // blaming the browser for something the code knew precisely. @@ -252,6 +277,10 @@ test.describe("the standalone deployment", () => { await second.clock.runFor(HANDSHAKE_TIMEOUT_MS + 2_000); await expect(second.locator("#status")).toContainText("read-only"); await expect(second.locator("#status")).not.toContainText("did not answer"); + const late = await second.evaluate(() => window.__statusLog); + expect(late.some((t) => t.includes("did not answer")), `#status said: ${json(late)}`).toBe( + false, + ); await second.close(); await first.close(); diff --git a/web/app.js b/web/app.js index b3c0149c..3d0cad82 100644 --- a/web/app.js +++ b/web/app.js @@ -181,7 +181,8 @@ function workerTransport() { shell.fail(`worker failed to start: ${event.message ?? "see console"}`); console.error("improvise: worker error", event); }); - shell.setShell(`in-page worker · ${DOC}`); + // Not yet a claim that it works: the first frame makes that (improvise-i0v9). + shell.setShell(`starting the worker · ${DOC}…`); // Frames posted before the worker has finished `start()` are held by // worker.js in arrival order, so the handshake can go out immediately. return { send: (text) => worker.postMessage(text), ready: Promise.resolve() }; @@ -797,6 +798,7 @@ shell.arm( WS_URL ? `${WS_URL} never answered — is improvise-ws-server running, and did it send a document?` : "the in-page worker did not answer (see console)", + { answer: WS_URL ? null : `in-page worker · ${DOC}` }, ); await transport.ready; sendViewport(); // the handshake: the snapshot is built at this extent diff --git a/web/shell.js b/web/shell.js index c2393afc..169373a7 100644 --- a/web/shell.js +++ b/web/shell.js @@ -339,7 +339,12 @@ export function createShell({ clearTimer = clearTimeout, }) { let watchdog = null; + let answered = null; let arms = 0; + const disarm = () => { + clearTimer(watchdog); + watchdog = null; + }; return { setShell(text) { @@ -350,24 +355,25 @@ export function createShell({ }, /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ - arm(message) { + arm(message, { answer = null } = {}) { arms += 1; - clearTimer(watchdog); + answered = answer; + disarm(); watchdog = setTimer(() => { watchdog = null; this.setShell(message); }, timeoutMs); }, - /** A frame arrived, so the peer answered: the watchdog has no question - * left to ask. */ + /** A frame arrived, so the peer answered. Only the FIRST, and only if no + * failure spoke first, may say so (improvise-i0v9, clients.md). */ sawFrame() { - clearTimer(watchdog); - watchdog = null; + if (watchdog !== null && answered !== null) this.setShell(answered); + disarm(); }, /** A peer that failed *loudly* has already said something more precise, so * the watchdog must be DISARMED and not merely pre-empted (PR #13). */ fail(text) { - this.sawFrame(); + disarm(); this.setShell(text); }, armed: () => watchdog !== null, diff --git a/web/tests/shell.test.js b/web/tests/shell.test.js index b2d02999..e8a9570d 100644 --- a/web/tests/shell.test.js +++ b/web/tests/shell.test.js @@ -351,6 +351,43 @@ test("a notice DISARMS the watchdog rather than being replaced by it", () => { ); }); +// improvise-i0v9: "in-page worker · " was written before the worker had +// answered anything, and read as success over a blank page for eight seconds. +test("only the first frame claims the peer answered, and not before it", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + assert.deepEqual(seen, [], "the claim was made before any frame arrived"); + shell.sawFrame(); + shell.sawFrame(); + assert.deepEqual(seen, ["in-page worker · a.improv"]); +}); + +// The order worker.js really produces on a locked document: the ReadOnly +// notice is posted after `start()` and BEFORE the Snapshot answers the handshake. +test("a reason given before the first frame is not replaced by the frame's claim", () => { + const clock = fakeClock(); + const seen = []; + const shell = createShell({ + onShell: (t) => seen.push(t), + timeoutMs: 8000, + setTimer: clock.setTimer, + clearTimer: clock.clearTimer, + }); + shell.arm("did not answer", { answer: "in-page worker · a.improv" }); + shell.fail(noticeText({ kind: "ReadOnly", document: "a.improv", detail: "" })); + shell.sawFrame(); + clock.fire(); + assert.equal(seen.length, 1, `#status said: ${JSON.stringify(seen)}`); + assert.match(seen[0], /read-only/); +}); + // The notice's remembered state. The value crosses `localStorage`, which is // allowed to forget and allowed to throw, so the interesting inputs are the // ones that are NOT either spelling.