From f75adbc4840c441b23bc61a7a3759e412f6aed40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20Hern=C3=A1ndez?= Date: Mon, 31 Aug 2026 22:12:46 -0400 Subject: [PATCH] Close the holes the committee found, and stop testing the file's prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewers, three lenses, and between them the first version promised more than it delivered. Copilot measured the scope in a real browser: five files were still a single request each, and a 503 on any of them showed no panel at all. One of those was new — with autostart="false" set and the retry living in boot.js, a 503 on boot.js itself meant nothing ever started. That was a regression this PR introduced, so it is fixed here rather than filed. The panel and a watchdog now live inline in index.html. Inline because the one failure an external file can never explain is its own. The watchdog fires when nothing has advanced for 45 s, not on a deadline, so a cheap phone on a bad connection is still allowed to be slow; it covers the runtime's ES modules, which must be handed back as a URL and so can never be retried, and a connection that accepts and then hangs. Each fetch now has its own 25 s abort, because a hung request consumed no attempt and reached no panel. The failure state is per download. Kept per module, a file that failed once and then recovered was named as the cause of a later, unrelated startup error — found by executing it, not by reading. DeepSeek read the copy and found the panel saying more than it knows. "Almost always a passing server fault" is not a frequency we failed to measure, it is one we can never measure: the panel exists precisely when the app never started, and this project does not phone home. Gone, along with exonerating a browser that a school VPN has already disproved once, and a privacy assurance answering a question nobody asked — the reader has not typed anything yet. The panel now offers the desktop app by absolute URL, because every route on this site is served by the page that just failed, and it says to copy the technical detail when reporting. It also honours the language the reader chose, which persists in localStorage and outlives the failed startup. The tests are behaviour now. The old ones searched this file's text, and every one of them could be satisfied by a comment or by dead code — `void undefined;` passed the test guarding the ES-module exclusion while breaking every module load. tests/boot/boot.test.mjs runs the real boot.js against a fake network, and each guarantee was checked by reintroducing the bug it guards: dropping the integrity handoff, fetching the ES modules, and the original misattribution are all detected. What stays in C# is the wiring those cannot see, asserted against source with comments stripped. Verified in a browser for all five scenarios: a healthy load, a 503 on boot.js, on the Blazor script, on a runtime module, and on an assembly. All five now explain themselves; four of them previously did not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015PEbbiYSNPw7jE3LrPNhyF --- .github/workflows/ci.yml | 6 + src/SignsOfAI.Web/wwwroot/index.html | 136 +++++++++++++- src/SignsOfAI.Web/wwwroot/js/boot.js | 144 ++++++-------- tests/SignsOfAI.Core.Tests/WebBootTests.cs | 80 +++++--- tests/boot/boot.test.mjs | 206 +++++++++++++++++++++ 5 files changed, 456 insertions(+), 116 deletions(-) create mode 100644 tests/boot/boot.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bc8f8e8..18d61f7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,12 @@ jobs: - name: Test run: dotnet test SignsOfAI.slnx -c Release --no-build --nologo + # The web app's startup path is JavaScript, so no .NET test can execute it. These run the real + # boot.js against a fake network: a 503 that recovers, one that does not, a connection that + # hangs, and the integrity hash it must keep handing to fetch. Node's own runner, no packages. + - name: Test boot script + run: node --test tests/boot/boot.test.mjs + # The desktop app is WPF, so it needs a Windows runner and it is not in SignsOfAI.slnx — see the # comment at the top of that file. Its own job keeps the build above on Linux, where it stays fast # for the translation PRs that are the reason this workflow exists. diff --git a/src/SignsOfAI.Web/wwwroot/index.html b/src/SignsOfAI.Web/wwwroot/index.html index f86eaac..164284a 100644 --- a/src/SignsOfAI.Web/wwwroot/index.html +++ b/src/SignsOfAI.Web/wwwroot/index.html @@ -37,8 +37,140 @@ - + + + + + diff --git a/src/SignsOfAI.Web/wwwroot/js/boot.js b/src/SignsOfAI.Web/wwwroot/js/boot.js index 5f3209c..0dec91e 100644 --- a/src/SignsOfAI.Web/wwwroot/js/boot.js +++ b/src/SignsOfAI.Web/wwwroot/js/boot.js @@ -1,129 +1,97 @@ // Starting the app, out loud when it fails. // -// The runtime arrives as ~50 separate files, each one pinned by a SHA-256 in the boot manifest. +// The runtime arrives as ~54 separate files, each one pinned by a SHA-256 in the boot manifest. // GitHub Pages answers one of them with a 503 now and then. The browser hashes whatever body did // arrive — an error page, not the assembly — the integrity check fails, and Blazor gives up for -// good. What the visitor sees is the loading circle, forever, with no message: the failure happens +// good. What the visitor saw was the loading circle, forever, with no message: the failure happens // before Blazor has an error UI to show. That is how this reached us, as "the app is down", when // every file on the server was intact. // -// So: retry the download before believing it, and if it still will not come, say so. +// So: retry the download before believing it. The panel that speaks when the retry does not help +// lives inline in index.html, because it also has to survive this file never arriving. (function () { const ATTEMPTS = 3; const BACKOFF_MS = [400, 1200]; - // The .NET runtime's own scripts are ES modules — they have to be handed back as a URL for the - // import to work, so a Response would break them. Those keep the default loader. + // A fetch has no timeout of its own. A server that accepts the connection and then never + // answers would otherwise consume no attempt, reach no panel, and hand back the same eternal + // spinner this file exists to kill — the failure mode measured and missed on the first pass. + const ATTEMPT_TIMEOUT_MS = 25000; + + // The .NET runtime's own scripts are ES modules: the loader asserts they come back as a URL, so + // a Response would break the import outright. Those keep the default loader — and therefore get + // no retry, which is one of the holes the inline watchdog covers. const NOT_OURS = 'dotnetjs'; - let lastFailure = null; + const boot = window.signsofaiBoot; async function fetchWithRetry(url, integrity) { + // Local to this download, never shared. A module-level "last failure" would let a file that + // failed once and then recovered be named as the cause of somebody else's error later. + let failure = null; + for (let attempt = 0; attempt < ATTEMPTS; attempt++) { if (attempt > 0) { await new Promise(done => setTimeout(done, BACKOFF_MS[attempt - 1] ?? 1200)); } + + const abort = new AbortController(); + const timer = setTimeout(() => abort.abort(), ATTEMPT_TIMEOUT_MS); try { const response = await fetch(url, { // Handing fetch the manifest's own hash keeps the guarantee we would otherwise - // be dropping: returning a Response from loadBootResource takes the integrity - // check away from Blazor, so the browser has to do it here instead. + // be dropping. Returning a Response takes the check away from Blazor entirely: + // the runtime short-circuits on a returned promise before it sets integrity of + // its own, so this line is not hygiene, it is the whole verification. integrity: integrity || undefined, // A first try may legitimately come from the browser cache. A retry may not — // if what we hold is a truncated or stale body, asking for it again is useless. cache: attempt === 0 ? 'default' : 'reload', + signal: abort.signal, }); - if (response.ok) return response; - lastFailure = { url, reason: `HTTP ${response.status}` }; + if (response.ok) { + if (attempt > 0) { + // The only trace a rescued startup leaves. Without it, a recovered blip is + // indistinguishable from a clean load, and the next person to ask "was the + // site flaky?" has nothing to look at. + console.info(`[signsofai] ${url} recovered after ${attempt + 1} attempts`); + } + boot.progress(); + return response; + } + failure = { url, reason: `HTTP ${response.status}` }; } catch (error) { // An integrity mismatch rejects here too, which is the case we are actually chasing: - // a 503 body hashes to something else, and retrying is exactly the right answer. - lastFailure = { url, reason: String(error && error.message || error) }; + // a 503 body hashes to something else. So does an abort. Both are worth retrying. + failure = { url, reason: String(error && error.message || error) }; + } finally { + clearTimeout(timer); } } + // Blazor does not reject Blazor.start() when a boot file will not come — the failure - // surfaces as an unhandled rejection deep in mono_download_assets, which is why the - // spinner used to spin for good. We are the ones who know the download is out of tries, - // so the explanation is written from here rather than from a .catch that never runs. - reportFailure(); - throw new Error(`${lastFailure.reason} for ${lastFailure.url}`); + // surfaces as an unhandled rejection inside mono_download_assets, which is why the spinner + // used to spin for good. We are the ones who know this download is out of tries, so the + // explanation is asked for from here rather than from a .catch that never runs. + boot.fail(`${failure.reason}\n${failure.url}`); + throw new Error(`${failure.reason} for ${failure.url}`); } - // The app has not started, so Blazor cannot render this and its own error UI is not wired yet. - // It also styles itself: app.css is a separate request, and a page that is explaining a failed - // download should not depend on one more download having worked. The custom properties are used - // where they exist and fall back to their light-theme values where they do not. - let reported = false; - - function reportFailure(error) { - // Several assets can fail in the same run; the first explanation is the one that stands. - if (reported) return; - reported = true; - - const spanish = (navigator.language || '').toLowerCase().startsWith('es'); - const copy = spanish ? { - title: 'No se pudo terminar de cargar', - body: 'Un archivo de la aplicación no llegó. Casi siempre es un fallo pasajero del ' + - 'servidor, no un problema de tu texto ni de tu navegador. Nada de lo que ' + - 'escribiste salió de tu equipo — la aplicación ni siquiera llegó a arrancar.', - retry: 'Reintentar', - details: 'Detalle técnico', - } : { - title: "Couldn't finish loading", - body: 'One of the application files did not arrive. This is almost always a passing ' + - 'server fault, not a problem with your text or your browser. Nothing you typed ' + - 'left your machine — the application never got as far as starting.', - retry: 'Try again', - details: 'Technical detail', - }; - - const app = document.getElementById('app'); - if (!app) return; - - app.innerHTML = ''; - const panel = document.createElement('div'); - panel.setAttribute('role', 'alert'); - panel.style.cssText = - 'max-width:34rem;margin:12vh auto;padding:1.5rem;border-radius:14px;' + - 'border:1px solid var(--border,#e2e6ea);background:var(--surface,#fff);' + - 'color:var(--text,#1a1d21);font:1rem/1.5 system-ui,-apple-system,Segoe UI,sans-serif;'; - - const title = document.createElement('h1'); - title.textContent = copy.title; - title.style.cssText = 'margin:0 0 .6rem;font-size:1.25rem;'; - - const body = document.createElement('p'); - body.textContent = copy.body; - body.style.cssText = 'margin:0 0 1.2rem;color:var(--text-muted,#5b6470);'; - - const retry = document.createElement('button'); - retry.type = 'button'; - retry.textContent = copy.retry; - retry.style.cssText = - 'padding:.55rem 1.1rem;border:0;border-radius:8px;cursor:pointer;font:inherit;' + - 'background:var(--brand,#2563eb);color:var(--brand-ink,#fff);'; - retry.addEventListener('click', () => window.location.reload()); - - const details = document.createElement('details'); - details.style.cssText = 'margin-top:1.2rem;font-size:.82rem;color:var(--text-muted,#5b6470);'; - const summary = document.createElement('summary'); - summary.textContent = copy.details; - summary.style.cssText = 'cursor:pointer;'; - const pre = document.createElement('pre'); - // textContent, not innerHTML: the URL comes back from the network and is never markup here. - pre.textContent = lastFailure - ? `${lastFailure.reason}\n${lastFailure.url}` - : String(error && error.message || error); - pre.style.cssText = 'white-space:pre-wrap;word-break:break-all;margin:.5rem 0 0;'; - details.append(summary, pre); - - panel.append(title, body, retry, details); - app.append(panel); + // A 503 on the Blazor script leaves this file running with nothing to call. Saying so now beats + // waiting for the watchdog: we already know it is never going to start. + if (typeof Blazor === 'undefined') { + boot.fail('Blazor.start is unavailable — _framework/blazor.webassembly.js did not load.'); + return; } Blazor.start({ loadBootResource(type, name, defaultUri, integrity) { return type === NOT_OURS ? undefined : fetchWithRetry(defaultUri, integrity); }, - }).catch(reportFailure); + }).then( + () => boot.ok(), + // Reached by startup failures that are not downloads at all. Those carry their own error, + // and it is the one to show — the retry's own failures have already spoken for themselves. + error => boot.fail(String(error && error.message || error)) + ); })(); diff --git a/tests/SignsOfAI.Core.Tests/WebBootTests.cs b/tests/SignsOfAI.Core.Tests/WebBootTests.cs index ab5f1eb..054ac12 100644 --- a/tests/SignsOfAI.Core.Tests/WebBootTests.cs +++ b/tests/SignsOfAI.Core.Tests/WebBootTests.cs @@ -1,3 +1,4 @@ +using System.Text.RegularExpressions; using Xunit; namespace SignsOfAI.Core.Tests; @@ -11,18 +12,21 @@ namespace SignsOfAI.Core.Tests; /// the visitor gets the loading circle and nothing else. That is a healthy deployment reading as an /// outage, and it is how it was first reported to us. /// -/// boot.js retries, and says so when the retry does not help. Three things in it fail -/// silently if a later edit gets them wrong, which is why they are asserted here rather than left -/// to a reviewer: the script has to run *after* Blazor is defined, it has to keep handing the -/// manifest hash to fetch, and it has to leave the runtime's own ES modules alone. +/// The retry itself is tested where it can actually be executed: tests/boot/boot.test.mjs +/// runs boot.js against a fake network. What is left here is the wiring those tests cannot +/// see — which file loads in which order, and whether the explanation can still be shown when the +/// file holding it is the one that never arrived. +/// +/// Everything is asserted against source with comments stripped. An earlier version of these tests +/// searched the raw text, and a reviewer showed every one of them could be satisfied by a comment +/// while the real markup said the opposite. /// public class WebBootTests { private static readonly string WebRoot = FindWebRoot(); + private static readonly string IndexHtml = - File.ReadAllText(Path.Combine(WebRoot, "index.html")); - private static readonly string BootJs = - File.ReadAllText(Path.Combine(WebRoot, "js", "boot.js")); + StripComments(File.ReadAllText(Path.Combine(WebRoot, "index.html"))); [Fact] public void Blazor_does_not_autostart_so_boot_js_can_supply_the_retry() @@ -44,38 +48,62 @@ public void Boot_script_is_loaded_after_blazor_defines_itself() } [Fact] - public void Boot_script_starts_blazor_through_a_boot_resource_loader() + public void The_failure_panel_stays_inline_so_it_survives_its_own_file_not_arriving() { - Assert.Contains("Blazor.start(", BootJs); - Assert.Contains("loadBootResource", BootJs); + // The point of the whole exercise. Move this into a .js file and the one failure it could + // never explain becomes its own: a 503 on that request leaves the eternal spinner back. + var panel = IndexHtml.IndexOf("window.signsofaiBoot", StringComparison.Ordinal); + var firstScriptFile = IndexHtml.IndexOf("js/boot.js", StringComparison.Ordinal); + + Assert.True(panel >= 0, "The inline boot panel is gone from index.html."); + Assert.True(panel < firstScriptFile, + "The panel must be defined before any script it has to survive the loss of."); } [Fact] - public void Retry_still_hands_the_manifest_hash_to_fetch() + public void A_watchdog_covers_the_failures_the_retry_cannot_reach() { - // Returning a Response from loadBootResource takes the integrity check away from Blazor. - // Dropping `integrity` here would therefore not fail anything — it would quietly stop - // verifying every assembly the app loads, which is the opposite of what this file is for. - Assert.Contains("integrity: integrity", BootJs); + // The runtime's ES modules must be left to the default loader, so they get no retry; and a + // connection that hangs never errors. Only a watchdog catches those. + Assert.Contains("setInterval", IndexHtml); + Assert.Contains("lastProgress", IndexHtml); } [Fact] - public void Runtime_modules_are_left_to_the_default_loader() + public void The_panel_offers_the_desktop_app_by_absolute_url_not_an_in_app_route() { - // The dotnet.js family is imported as ES modules, which needs a URL. Answer those with a - // Response and the import fails — turning a fix for rare 503s into a permanent breakage. - Assert.Contains("dotnetjs", BootJs); - Assert.Contains("undefined", BootJs); + // Every route on this site is served by this same page, so a link to /download would try to + // start the app it just failed to start. The escape hatch has to leave the site. + var match = Regex.Match(IndexHtml, @"var DESKTOP = '([^']+)'"); + + Assert.True(match.Success, "The panel no longer names a desktop download URL."); + Assert.StartsWith("https://github.com/", match.Groups[1].Value); + } + + [Fact] + public void The_panel_prefers_the_language_the_reader_actually_chose() + { + // The app persists the switch here. Reading only navigator.language would show an English + // panel to a teacher using the app in Spanish — issue #36 in the one place Loc cannot reach. + Assert.Contains("signsofai.ui.lang", IndexHtml); } [Theory] - // The app picks its language from the browser, and this panel renders before the app exists, - // so it carries its own copy. English-only here would be the #36 bug in a new place. - [InlineData("No se pudo terminar de cargar")] - [InlineData("Couldn't finish loading")] - public void Failure_panel_speaks_both_languages(string phrase) + [InlineData("No se pudo completar la carga")] + [InlineData("Couldn’t load")] + public void The_panel_speaks_both_languages(string phrase) + { + Assert.Contains(phrase, IndexHtml); + } + + /// + /// Removes HTML comments and JavaScript line comments, so an assertion cannot be satisfied by + /// prose describing what the markup ought to do. // inside a URL is left alone. + /// + private static string StripComments(string source) { - Assert.Contains(phrase, BootJs); + var withoutHtml = Regex.Replace(source, "", string.Empty, RegexOptions.Singleline); + return Regex.Replace(withoutHtml, @"(? calls.push(args.join(' ')) }, + setTimeout: (fn, delay) => setTimeout(fn, delay === ABORT_DELAY ? abortAfterMs : 1), + clearTimeout, + fetch: (url, init) => fetch(url, init), + }; + if (withBlazor) { + sandbox.Blazor = { + start(options) { + loadBootResource = options.loadBootResource; + return onStart ? onStart() : new Promise(() => {}); + }, + }; + } + + vm.createContext(sandbox); + vm.runInContext(SOURCE, sandbox); + return { boot, logged: calls, load: (...args) => loadBootResource(...args) }; +} + +const ok = () => ({ ok: true, status: 200 }); +const unavailable = () => ({ ok: false, status: 503 }); + +test('a transient 503 is retried and the asset still loads', async () => { + let attempts = 0; + const app = load({ fetch: async () => (++attempts < 3 ? unavailable() : ok()) }); + + const response = await app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-x'); + + assert.equal(response.ok, true); + assert.equal(attempts, 3, 'should have used all three attempts'); + assert.equal(app.boot.shown, false, 'a recovered download must not raise the panel'); + assert.equal(app.boot.progressed, 1, 'a completed download must report progress'); +}); + +test('a recovered download says so, so a rescued startup leaves a trace', async () => { + let attempts = 0; + const app = load({ fetch: async () => (++attempts < 2 ? unavailable() : ok()) }); + + await app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-x'); + + assert.match(app.logged.join('\n'), /recovered after 2 attempts/); +}); + +test('a download that never succeeds gives up and names the file it could not get', async () => { + let attempts = 0; + const app = load({ fetch: async () => { attempts++; return unavailable(); } }); + + await assert.rejects(() => app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-x')); + + assert.equal(attempts, 3, 'should stop after three attempts, not retry for ever'); + assert.equal(app.boot.shown, true, 'the panel must be raised from the retry loop'); + assert.match(app.boot.detail, /HTTP 503/); + assert.match(app.boot.detail, /\/A\.wasm/); +}); + +test('the manifest hash is handed to fetch on every attempt', async () => { + // Load-bearing, and silent if it breaks. Returning a Response short-circuits the runtime before + // it sets integrity of its own, so dropping this would fail no test and quietly stop verifying + // every assembly the app loads. + const seen = []; + const app = load({ fetch: async (url, init) => { seen.push(init.integrity); return unavailable(); } }); + + await assert.rejects(() => app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-abc')); + + assert.deepEqual(seen, ['sha256-abc', 'sha256-abc', 'sha256-abc']); +}); + +test('an asset with no hash in the manifest is not sent an empty integrity', async () => { + // The runtime normalises a missing hash to "", and fetch would treat "" as "verify nothing" + // rather than "no opinion". Passing undefined keeps parity with the default loader. + let seen; + const app = load({ fetch: async (url, init) => { seen = init; return ok(); } }); + + await app.load('assembly', 'A.wasm', '/A.wasm', ''); + + assert.equal(seen.integrity, undefined); +}); + +test('the first attempt may use the cache; the retries may not', async () => { + const seen = []; + const app = load({ fetch: async (url, init) => { seen.push(init.cache); return unavailable(); } }); + + await assert.rejects(() => app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-x')); + + assert.deepEqual(seen, ['default', 'reload', 'reload']); +}); + +test('the runtime ES modules are left to the default loader and never fetched here', async () => { + // Answering these with a Response breaks the import outright — a fix for rare 503s turned into + // a permanent failure. The old test for this passed against `void undefined;`. + let fetched = 0; + const app = load({ fetch: async () => { fetched++; return ok(); } }); + + const result = await app.load('dotnetjs', 'dotnet.js', '/dotnet.js', 'sha256-x'); + + assert.equal(result, undefined, 'must hand dotnetjs back to the default loader'); + assert.equal(fetched, 0, 'must not fetch a dotnetjs module itself'); +}); + +test('a connection that hangs is abandoned rather than waited on for ever', async () => { + // The failure the first version missed: fetch has no timeout, so a server that accepts and then + // never answers consumed no attempt, reached no panel, and left the eternal spinner in place. + let attempts = 0; + const app = load({ + abortAfterMs: 5, + fetch: (url, init) => new Promise((resolve, reject) => { + attempts++; + init.signal.addEventListener('abort', () => reject(new Error('aborted'))); + }), + }); + + await assert.rejects(() => app.load('assembly', 'A.wasm', '/A.wasm', 'sha256-x')); + + assert.equal(attempts, 3); + assert.equal(app.boot.shown, true, 'a hung connection must still reach the panel'); +}); + +test('a file that failed once and recovered is not blamed for a later startup error', async () => { + // The misattribution a reviewer found by executing it: failure state kept per module rather + // than per download let a recovered file be named as the cause of somebody else's error. + let attempts = 0; + let capture; + const app = load({ + fetch: async () => (++attempts === 1 ? unavailable() : ok()), + onStart: () => new Promise((resolve, reject) => { capture = reject; }), + }); + + await app.load('assembly', 'recovered.wasm', '/recovered.wasm', 'sha256-x'); + capture(new Error('WASM instantiation failed')); + await new Promise(done => setTimeout(done, 5)); + + assert.equal(app.boot.shown, true); + assert.match(app.boot.detail, /WASM instantiation failed/); + assert.doesNotMatch(app.boot.detail, /recovered\.wasm/, + 'the panel must report the error that raised it, not a download that succeeded'); +}); + +test('a successful startup marks the app as started', async () => { + const app = load({ fetch: async () => ok(), onStart: () => Promise.resolve() }); + + await new Promise(done => setTimeout(done, 5)); + + assert.equal(app.boot.started, true); + assert.equal(app.boot.shown, false); +}); + +test('a missing Blazor script is reported at once rather than waited out', async () => { + // A 503 on blazor.webassembly.js leaves this file running with nothing to call. Before the + // guard it threw a synchronous ReferenceError that no catch saw. + const app = load({ fetch: async () => ok(), withBlazor: false }); + + assert.equal(app.boot.shown, true); + assert.match(app.boot.detail, /blazor\.webassembly\.js/); +});