From 3f2bb22586d3e1aef765149b555ccc8e174ce7eb Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 8 Sep 2026 13:20:29 +0200 Subject: fix(hub): make the streamed download path reliable, and clean up after an abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Firefox and Safari the service worker is the only unbounded way to write a download to disk: neither has the File System Access API, and OPFS is not a substitute — measured on Firefox 154, its quota is exactly 10% of the volume's size (389,233,459 bytes of a 3,892,334,592-byte volume, refused to the byte), which a film exceeds. So when this path declines, a large download has nowhere left to go, which makes its reliability a correctness property. Four ways it declined, all of them avoidable: - it was registered inside the first click on Download, so that click paid install, activate and claim while somebody watched a button do nothing; - `_swReady` cached a null for the life of the page. One slow first click left the tab unable to stream anything again, curable only by a reload nobody knew to do. Only a successful controller is remembered now; - control was waited for with a 3 s cap. It is 15 s, and a page that is active but not controlled asks the worker to claim again (`mbdl-claim`) instead of declaring the path unavailable; - a missed navigation gave up at once. It gets a second attempt with a fresh id and iframe, the failed one torn down completely first. Also closes a MessagePort leaked per download, and gives the reason a name (`lastStreamFailure`) so a refusal can say what happened. The timeouts became parameters: the defaults are the production values, no caller passes any, and the tests do not spend a minute waiting. `openTarget` gets an unrelated but adjacent fix, in the same file: it creates the destination with `getFileHandle({create: true})`, so an empty file exists before the first byte, and `abort()` leaves the target untouched — every cancelled download left a 0-byte file behind, and since `freeName` avoids collisions, three cancels left film.mkv, film (2).mkv and film (3).mkv, all empty. Its `abort()` now removes the entry. Safe here and only here, because `freeName` guarantees the name was not taken: the `showSaveFilePicker` path must not do the same, where the person may have picked an existing file whose contents `abort()` correctly preserves. Verified by hand in Chrome. test_streamed_download_reliability.py runs the real module under Node against a stubbed browser — it fails if the null is cached again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HCGdheDLxGReuKHga3BtST --- packages/meshbay-hub/src/meshbay_hub/static/sw.js | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'packages/meshbay-hub/src/meshbay_hub/static/sw.js') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/sw.js b/packages/meshbay-hub/src/meshbay_hub/static/sw.js index 119687d..0dd87d8 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/sw.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/sw.js @@ -24,6 +24,14 @@ self.addEventListener('activate', (event) => event.waitUntil(self.clients.claim( self.addEventListener('message', (event) => { const data = event.data || {}; + // A page that loaded before any worker existed can miss the claim on + // activate. Rather than declare the streamed path unavailable — which on + // Firefox and Safari means the download cannot happen at all — the page asks + // for another claim and waits a moment longer. + if (data.type === 'mbdl-claim') { + event.waitUntil(self.clients.claim()); + return; + } if (data.type !== 'mbdl' || !data.id || !data.readable) return; pending.set(data.id, { readable: data.readable, -- cgit v1.2.3 From 051100ca32f72dd8f28489e1b6cb084f321b3b54 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 8 Sep 2026 22:53:53 +0200 Subject: fix(hub): keep the download worker alive, and never hang on a dead sink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A download froze part-way through, on Firefox, with an empty console and a node that stayed perfectly healthy. Three separate measurements cleared the node (615 MB pulled whole over MNP), the transport (three files interleaved on one connection, 1.5 GB, all whole) and the service worker (three concurrent 150 MB streams in real Firefox 154) — because none of them was wrong. The empty console was the evidence. `_sendAndWait` logs every timeout, so no chunk request had expired: the client was not waiting on the node. Of the three awaits left on that path only one was unbounded. **A service worker with no event for about thirty seconds is terminated**, and `respondWith(new Response(stream))` does not extend its life while the response is still being written. The reader vanished mid-file and `writable.write()` then never resolved and never rejected — no error, no log, no failed transfer, just a progress bar that stops. The first stress probe wrote 450 MB in two seconds and passed: fast enough to hide it entirely. Measured in Firefox 154, writing 1 MB every 2 s: without the ping it stalled at 17 MB after 59 s; with it, 40 MB in 80 s, complete. - the page pings the worker every 10 s while it writes, and the worker answers. Receiving a message is an event, and an event resets the timer; - that interval stops itself after two minutes with no write. A target can be opened and never written to — a transfer cancelled while it waits for a slot never runs, so nothing calls close() or abort() — and a timer nobody clears pings for the life of the page. It also kept the Node test process alive for ever, which is the same defect wearing a louder symptom; - `writable.write()` is bounded at 60 s and fails with a message naming the chunk. That does not fix whatever stopped a sink; it turns an unexplainable freeze into a failed transfer that says so, which is the difference between a mystery and a bug report. Also: `Content-Disposition` lost a filename to a single apostrophe. `encodeURIComponent` leaves `'` alone and `'` is the delimiter in RFC 5987's `filename*=''`, so the header became unparseable and Firefox named the file after the URL — 449 MB of film arrived complete as "mtsshk9w-ohqty535". `(`, `)` and `*` get the same treatment, and a plain ASCII `filename=` rides alongside. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HCGdheDLxGReuKHga3BtST --- .../src/meshbay_hub/static/downloads.js | 33 ++++- packages/meshbay-hub/src/meshbay_hub/static/sw.js | 49 ++++++- packages/meshbay-hub/tests/test_downloads.py | 146 +++++++++++++++++++++ .../tests/test_streamed_download_reliability.py | 17 ++- 4 files changed, 238 insertions(+), 7 deletions(-) (limited to 'packages/meshbay-hub/src/meshbay_hub/static/sw.js') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/downloads.js b/packages/meshbay-hub/src/meshbay_hub/static/downloads.js index f620e15..cfb0051 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/downloads.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/downloads.js @@ -216,6 +216,17 @@ const SW_CONTROL_BUDGET_MS = 15000; const SW_SERVED_BUDGET_MS = 15000; // A transient miss gets a second go with a fresh id and a fresh iframe. const SW_ATTEMPTS = 2; +// How often the page pokes the worker while a download is being written. +// Firefox terminates a service worker that has had no event for roughly thirty +// seconds, and a streaming response does not count as activity — so a download +// that takes longer than that lost its reader half way through. Ten seconds +// leaves a wide margin and costs one empty message. +const SW_KEEPALIVE_MS = 10000; +// And the ping stops on its own once nothing has been written for this long. +// Well past any real gap between chunks, and short enough that an abandoned +// target does not ping for ever. Bounded because the alternative is a timer +// whose lifetime depends on every caller remembering to close its sink. +const SW_KEEPALIVE_IDLE_MS = 120000; // Holds a *successful* controller, or an in-flight attempt. Never a failure — // see serviceWorker(). The previous version cached the rejected/null result @@ -399,6 +410,24 @@ async function _attemptStreamedDownload(filename, size, attempt, } _lastFailure = ''; + // Every few seconds for as long as this download is being written. Well + // inside the ~30 s Firefox allows an idle worker, and cheap: one postMessage + // with no payload. Cleared by close() and abort() below, so a finished + // download leaves no timer behind. + // Self-limiting, and that is not belt-and-braces: a target can be opened and + // then never written to — a transfer cancelled while it waits for a slot + // never runs, so nothing calls close() or abort() — and an interval nobody + // clears pings for the life of the page. It also kept the Node test process + // alive for ever, which is the same defect wearing a louder symptom (the + // MessagePort above did exactly this a few hours earlier). + let lastWrite = Date.now(); + const keepAlive = setInterval(() => { + if (Date.now() - lastWrite > SW_KEEPALIVE_IDLE_MS) { + clearInterval(keepAlive); + return; + } + try { worker.postMessage({ type: 'mbdl-ping' }); } catch { /* gone */ } + }, SW_KEEPALIVE_MS); // The port has delivered the one message it exists for. Closing it matters: // an open MessagePort is a live handle, and one was leaked per download for // the life of the page. (It is also what hung the Node harness in @@ -410,12 +439,14 @@ async function _attemptStreamedDownload(filename, size, attempt, return { name: filename, writable: { - write: (bytes) => writer.write(bytes), + write: (bytes) => { lastWrite = Date.now(); return writer.write(bytes); }, close: async () => { + clearInterval(keepAlive); await writer.close(); setTimeout(() => frame.remove(), 2000); }, abort: async (reason) => { + clearInterval(keepAlive); try { await writer.abort(reason); } catch { /* already gone */ } frame.remove(); }, diff --git a/packages/meshbay-hub/src/meshbay_hub/static/sw.js b/packages/meshbay-hub/src/meshbay_hub/static/sw.js index 0dd87d8..309ecc1 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/sw.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/sw.js @@ -17,6 +17,35 @@ */ const PREFIX = '/_mbdl/'; + +/** + * A filename, safe to put in Content-Disposition. + * + * `encodeURIComponent` alone is not enough, and the way it fails is invisible + * until somebody downloads the wrong film: it leaves `'` untouched, and `'` is + * the *delimiter* in RFC 5987's `filename*=''`. A single + * apostrophe in a name therefore makes the header unparseable, and a browser + * that cannot parse it falls back to the last segment of the URL — which here + * is the made-up id this worker answers on. The file arrives complete, 449 MB + * of it, called "mtsshk9w-ohqty535". + * + * Found by downloading three files where exactly one had an apostrophe in its + * name. `(`, `)` and `*` are excluded from RFC 5987's attr-char for the same + * reason and get the same treatment. + * + * The plain `filename=` beside it is the ASCII fallback every parser + * understands: it loses the accents, and it is what stops a name being lost + * entirely the next time one of these encodings surprises us. + */ +function contentDisposition(name) { + const encoded = encodeURIComponent(name) + .replace(/['()*]/g, (c) => '%' + c.charCodeAt(0).toString(16).toUpperCase()); + // Quotes and backslashes would end the quoted-string early; anything not + // plain ASCII is dropped rather than mangled, since the starred form above + // carries the real name. + const ascii = name.replace(/["\\]/g, '_').replace(/[^\x20-\x7e]/g, '_'); + return `attachment; filename="${ascii}"; filename*=UTF-8''${encoded}`; +} const pending = new Map(); self.addEventListener('install', () => self.skipWaiting()); @@ -24,6 +53,23 @@ self.addEventListener('activate', (event) => event.waitUntil(self.clients.claim( self.addEventListener('message', (event) => { const data = event.data || {}; + // A worker with nothing to do is terminated — Firefox after about thirty + // seconds, and `respondWith(new Response(stream))` does not extend its life + // for the duration of the response. So a download longer than that lost its + // reader mid-file: the page's next `write()` never resolved and never + // rejected, the progress bar stopped, the console stayed empty and the node + // went on looking perfectly healthy. Handling a message is an event, and an + // event resets that timer, so the page pings while it is writing. + // + // It also has to be answered: a ping that only arrives keeps *this* worker + // alive, and the reply is how the page learns the worker it is talking to is + // still the one holding its stream. + if (data.type === 'mbdl-ping') { + if (event.ports && event.ports[0]) { + try { event.ports[0].postMessage({ type: 'mbdl-pong' }); } catch { /* gone */ } + } + return; + } // A page that loaded before any worker existed can miss the claim on // activate. Rather than declare the streamed path unavailable — which on // Firefox and Safari means the download cannot happen at all — the page asks @@ -64,8 +110,7 @@ self.addEventListener('fetch', (event) => { const headers = { 'Content-Type': 'application/octet-stream', // filename* so a name with accents or spaces survives the trip. - 'Content-Disposition': - `attachment; filename*=UTF-8''${encodeURIComponent(entry.filename)}`, + 'Content-Disposition': contentDisposition(entry.filename), 'Cache-Control': 'no-store', }; // Only when it is known. A zip is assembled as it goes and announcing a diff --git a/packages/meshbay-hub/tests/test_downloads.py b/packages/meshbay-hub/tests/test_downloads.py index fc69fac..e68396f 100644 --- a/packages/meshbay-hub/tests/test_downloads.py +++ b/packages/meshbay-hub/tests/test_downloads.py @@ -217,3 +217,149 @@ def test_an_uncontrolled_page_is_not_treated_as_ready(): "control can arrive a tick after registration; waiting beats refusing") assert "mbdl-claim" in section, ( "an active-but-uncontrolled page must ask for a claim, not give up") + + +def test_an_apostrophe_in_a_name_does_not_lose_the_name(tmp_path): + """ + `encodeURIComponent` leaves `'` alone, and `'` is the delimiter in RFC + 5987's `filename*=''`. One apostrophe made the header + unparseable, and a browser that cannot parse it names the file after the + last segment of the URL — which for this worker is a made-up id. The file + arrived complete and 449 MB of it was called "mtsshk9w-ohqty535". + + Found by downloading three files where exactly one had an apostrophe. + Nothing in the suite could have: the header was built correctly for every + name anybody had tested with. + + The real function is lifted out of sw.js and run — a second copy here would + have the same blind spot as the first. + """ + src = SW.read_text() + fn = src[src.index("function contentDisposition"):] + fn = fn[:fn.index("\n}") + 2] + + script = tmp_path / "case.mjs" + script.write_text(fn + """ +const out = {}; +for (const name of ["S03E02. Queen's Landing.mp4", 'Caf\\u00e9 (2019).mkv', + 'plain.mp4', 'quote".mp4', 'star*.mp4']) { + out[name] = contentDisposition(name); +} +console.log(JSON.stringify(out)); +""") + proc = subprocess.run(["node", str(script)], capture_output=True, text=True) + assert proc.returncode == 0, proc.stderr + out = json.loads(proc.stdout) + + for name, header in out.items(): + starred = header.split("filename*=UTF-8''", 1)[1] + assert "'" not in starred, ( + f"{name!r}: an apostrophe survived into the starred value, which " + f"is where RFC 5987 puts its delimiter — the name is lost") + for forbidden in "()*": + assert forbidden not in starred, ( + f"{name!r}: {forbidden!r} is not an attr-char and must be " + f"percent-encoded") + # The starred value has to decode back to the real name, or the escaping + # fixed the parse and broke the result. + from urllib.parse import unquote + assert unquote(starred) == name + + # The ASCII fallback must not end its own quoted string. + for name, header in out.items(): + ascii_part = header.split('filename="', 1)[1].split('";', 1)[0] + assert '"' not in ascii_part and "\\" not in ascii_part + + +def test_a_sink_that_stops_consuming_fails_instead_of_hanging(tmp_path): + """ + `writable.write()` was the one await on the download path with no bound. + + Every other one reports itself: `_sendAndWait` logs a Response timeout, + `_fetchChunkResilient` retries and throws. A sink that stops consuming — a + service-worker stream the browser has stopped reading — leaves `write()` + pending for ever. It never rejects, so there is no error, no log and no + failed transfer: the progress bar stops, the console stays empty, and the + node is healthy throughout. + + That combination is what made it unfindable: three separate measurements + cleared the node, the transport and the worker, because none of them was + wrong. Bounding it does not fix whatever stopped the sink — it turns an + unexplainable freeze into a failed transfer that names itself. + """ + src = (STATIC / "file-utils.js").read_text() + fn = src[src.index("async function _writeOrStall"):] + fn = fn[:fn.index("\n}\n") + 2] + + script = tmp_path / "case.mjs" + script.write_text(""" +const t = (key, vars) => key + ' ' + JSON.stringify(vars); +const WRITE_STALL_MS = 300; // the real value is 60s; the shape is the test +""" + fn + """ +const out = {}; +// A sink that never resolves — the frozen download, exactly. +const dead = { write: () => new Promise(() => {}) }; +const t0 = Date.now(); +try { + await _writeOrStall(dead, new Uint8Array(4), 41); + out.threw = null; +} catch (e) { out.threw = e.message; } +out.ms = Date.now() - t0; + +// And a working sink is not slowed down or wrapped in anything. +const live = { write: async () => {} }; +await _writeOrStall(live, new Uint8Array(4), 0); +out.liveOk = true; +console.log(JSON.stringify(out)); +""") + proc = subprocess.run(["node", str(script)], capture_output=True, text=True) + assert proc.returncode == 0, proc.stderr + out = json.loads(proc.stdout) + assert out["threw"], "a dead sink hung for ever instead of failing" + assert "group.download_write_stalled" in out["threw"], ( + "the failure must name itself in the transfers panel") + assert "41" in out["threw"], "and say which chunk it stopped at" + assert out["ms"] < 3000 + assert out["liveOk"] is True + + +def test_the_worker_is_kept_alive_while_it_streams(): + """ + A service worker with no event for ~30 s is terminated — Firefox does it, + and `respondWith(new Response(stream))` does not extend its life while the + response is still being written. The reader vanishes mid-file, the page's + next `write()` never resolves and never rejects: the progress bar stops, + the console stays empty, and the node looks healthy throughout. + + Measured in real Firefox 154 on 2026-09-08, writing 1 MB every 2 s: + without the ping it stalled at 17 MB after 59 s; with it, 40 MB in 80 s, + complete. The first version of that probe wrote 450 MB in two seconds and + passed — fast enough to hide the bug entirely, which is why the pacing + matters and is written down here. + + Source-reading, because the behaviour needs a browser and a minute of wall + clock. What it protects is that the ping exists at all, is cleared on both + exits, and is answered by the worker. + """ + dl = DOWNLOADS.read_text() + sw = SW.read_text() + + assert "SW_KEEPALIVE_MS" in dl and "mbdl-ping" in dl, ( + "nothing keeps the worker alive; downloads longer than ~30 s will " + "stall on Firefox with no error anywhere") + fn = dl[dl.index("async function _attemptStreamedDownload"):] + interval = fn[fn.index("setInterval"):] + assert "mbdl-ping" in interval[:200] + + # Cleared on both ways out, or a finished download leaves a timer pinging a + # worker for the life of the page. + for exit_path in ("close:", "abort:"): + block = fn[fn.index(exit_path):] + assert "clearInterval(keepAlive)" in block[:220], ( + f"the keep-alive is not cleared in {exit_path} — it outlives the " + f"download") + + # And the worker has to answer it: a message it ignores still counts as an + # event, but the reply is what tells the page it is talking to the worker + # that holds its stream. + assert "mbdl-ping" in sw and "mbdl-pong" in sw diff --git a/packages/meshbay-hub/tests/test_streamed_download_reliability.py b/packages/meshbay-hub/tests/test_streamed_download_reliability.py index fa346cb..45138b4 100644 --- a/packages/meshbay-hub/tests/test_streamed_download_reliability.py +++ b/packages/meshbay-hub/tests/test_streamed_download_reliability.py @@ -168,8 +168,13 @@ def test_a_missed_claim_does_not_poison_the_page(tmp_path): def test_a_success_is_reused_rather_than_re_registered(tmp_path): """The other half: once controlled, it must not re-register per download.""" r = _run(tmp_path, """ - out.a = await M.openStreamedDownload('a.bin', 10, FAST) !== null; - out.b = await M.openStreamedDownload('b.bin', 10, FAST) !== null; + // Closed, like a real caller: an open target holds a keep-alive interval + // for the worker, and a test that leaks one never lets Node exit. + for (const name of ['a.bin', 'b.bin']) { + const t = await M.openStreamedDownload(name, 10, FAST); + out[name[0]] = t !== null; + if (t) await t.writable.close(); + } """) assert r["a"] and r["b"] assert r["log"]["registers"] <= 1, "re-registered on a page already controlled" @@ -187,8 +192,10 @@ def test_control_arriving_late_is_still_used(tmp_path): """ r = _run(tmp_path, """ const t0 = Date.now(); - out.ok = await M.openStreamedDownload('film.mkv', 20e9, FAST) !== null; + const target = await M.openStreamedDownload('film.mkv', 20e9, FAST); + out.ok = target !== null; out.waitedMs = Date.now() - t0; + if (target) await target.writable.close(); """, control_after_ms=1200, control_budget_ms=6000) assert r["ok"] is True, "gave up on a claim that arrived late" assert r["waitedMs"] >= 1100, "did not actually wait for the claim" @@ -214,7 +221,9 @@ def test_a_missed_navigation_is_retried(tmp_path): floor. It gets a second go, with a fresh id and a fresh iframe. """ r = _run(tmp_path, """ - out.ok = await M.openStreamedDownload('film.mkv', 20e9, FAST) !== null; + const t = await M.openStreamedDownload('film.mkv', 20e9, FAST); + out.ok = t !== null; + if (t) await t.writable.close(); """, serve="second") assert r["ok"] is True, "one missed navigation ended the download" assert r["log"]["navigations"] == 2 -- cgit v1.2.3 From dee57df42a525cead93fa30b4e7fa38a489d5b11 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 9 Sep 2026 12:43:08 +0200 Subject: fix(spa): wake the download worker before handing it a stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from Chrome: a download started while an upload was running took thirty seconds to begin, every time. The console named it exactly — /_mbdl/mtty5btz-sbmgdegx 404 () [MeshBay] the worker did not answer the download within 15s (attempt 1) A 404 from the hub means the request reached the *network*: the worker looked, found no entry for that id and let it through. So the worker was alive and controlling the page, and the message handing it the stream had simply never been processed. `pending` lives in the worker's memory, and a worker with nothing to do is terminated within tens of seconds. A WebRTC upload gives it no events at all, so minutes of uploading leave it dead; the stream posted to it is lost, silently, and the iframe then wakes it with nothing to find. `mbdl-ping` already existed for this exact reason -- sent every ten seconds *while* writing, because a streaming response does not count as activity. Nothing sent one before *starting*. So a download now wakes the worker and waits for the pong, and `sw.js` answers `mbdl-ready` once it has actually stored the entry, which the page waits for before navigating: confirmed rather than assumed. A worker that predates the ack sends nothing and the page navigates anyway, which is what it did before. This cause was measured and wrongly dismissed hours earlier, with an idle probe that made the worker work between its own attempts -- it never actually slept. A measurement that does not reproduce the conditions refutes nothing. The harness now models a worker that is asleep: a ping wakes it, and anything else posted while it sleeps is lost, which is what made the failure silent. `test_backpressure_is_real` read the first `worker.postMessage` in the function to check that the readable half is transferred rather than copied. The wake-up put a ping in front of it, so it began inspecting a call that carries only a port -- and kept passing. It now checks every post, each bounded by its own call, since the keep-alive ping transfers nothing at all. Same shape as the upload-seal contract this morning: a guard that reads "the first" stops guarding the moment something is inserted before it. Hub suite 851 passed. Both new cases checked against the unfixed source. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HCGdheDLxGReuKHga3BtST --- .../src/meshbay_hub/static/downloads.js | 58 +++++++++++++++++++- packages/meshbay-hub/src/meshbay_hub/static/sw.js | 15 ++++++ packages/meshbay-hub/tests/test_downloads.py | 20 +++++-- .../tests/test_streamed_download_reliability.py | 61 +++++++++++++++++++++- 4 files changed, 148 insertions(+), 6 deletions(-) (limited to 'packages/meshbay-hub/src/meshbay_hub/static/sw.js') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/downloads.js b/packages/meshbay-hub/src/meshbay_hub/static/downloads.js index ce1901c..c790394 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/downloads.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/downloads.js @@ -232,6 +232,16 @@ const SW_KEEPALIVE_MS = 10000; // target does not ping for ever. Bounded because the alternative is a timer // whose lifetime depends on every caller remembering to close its sink. const SW_KEEPALIVE_IDLE_MS = 120000; +// How long to spend waking the worker, and then confirming it holds the stream, +// before starting the navigation that has to find it. +// +// Both are answered in milliseconds when the worker is alive. They exist for +// when it is not: `pending` lives in the worker's memory, and one with nothing +// to do is terminated within tens of seconds — which a long upload spends +// without giving it a single event. A stream handed to a worker in that state +// is lost, and the iframe then wakes it with nothing to find, which is a 404 +// from the hub and fifteen seconds of silence per attempt. +const SW_WAKE_BUDGET_MS = 3000; // Holds a *successful* controller, or an in-flight attempt. Never a failure — // see serviceWorker(). The previous version cached the rejected/null result @@ -504,6 +514,35 @@ export async function openStreamedDownload(filename, size = 0, { return null; } +/** + * Get the worker running, and know that it is. + * + * `mbdl-ping` exists already — the page sends it every ten seconds *while* + * writing, because a streaming response does not count as activity and Firefox + * kills an idle worker mid-download. Nothing sent one before *starting* a + * download, which is the case that fails after a long upload has left the + * worker with nothing to do for minutes. + * + * Never fatal: a worker that does not answer may still be perfectly able to + * serve, and the caller finds that out the honest way. + */ +async function _wake(worker) { + const chan = new MessageChannel(); + const pong = new Promise((resolve) => { + chan.port1.onmessage = () => resolve(true); + }); + try { + worker.postMessage({ type: 'mbdl-ping' }, [chan.port2]); + } catch { + return false; + } + const awake = await Promise.race([ + pong, new Promise((r) => setTimeout(() => r(false), SW_WAKE_BUDGET_MS)), + ]); + try { chan.port1.close(); } catch { /* already gone */ } + return awake; +} + async function _attemptStreamedDownload(filename, size, attempt, controlMs, servedMs) { const worker = await serviceWorker(controlMs); @@ -517,12 +556,23 @@ async function _attemptStreamedDownload(filename, size, attempt, // backpressure that will never be relieved, which reads as a download frozen // after one chunk rather than as an error. const chan = new MessageChannel(); + let markReady = null; + const held = new Promise((resolve) => { markReady = resolve; }); const serving = new Promise((resolve) => { chan.port1.onmessage = (e) => { - if (e.data && e.data.type === 'mbdl-serving') resolve(true); + if (!e.data) return; + // The worker says it has the stream. Waiting for this is what stops the + // navigation racing a worker that was asleep when we posted. + if (e.data.type === 'mbdl-ready') markReady(true); + if (e.data.type === 'mbdl-serving') resolve(true); }; }); + // Wake it first, and wait for the answer. A worker that has been idle through + // a long upload is terminated, and a message posted to it in that state is + // lost — silently, which is the whole difficulty. + await _wake(worker); + try { worker.postMessage({ type: 'mbdl', id, filename, size, readable, port: chan.port2 }, [readable, chan.port2]); @@ -536,6 +586,12 @@ async function _attemptStreamedDownload(filename, size, attempt, return null; } + // Confirmed, not assumed. A worker that predates this sends no answer, and + // then navigating anyway is exactly what this code did before. + await Promise.race([ + held, new Promise((r) => setTimeout(r, SW_WAKE_BUDGET_MS)), + ]); + const frame = document.createElement('iframe'); frame.hidden = true; frame.src = `${PREFIX_PATH}${id}`; diff --git a/packages/meshbay-hub/src/meshbay_hub/static/sw.js b/packages/meshbay-hub/src/meshbay_hub/static/sw.js index 309ecc1..5f663f9 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/sw.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/sw.js @@ -89,6 +89,21 @@ self.addEventListener('message', (event) => { // stream and the page's first write blocks for good. port: data.port || null, }); + // Say so, on the port the page is already listening to. + // + // `pending` is in memory, and a worker with nothing to do is terminated: + // Chrome after tens of seconds, which a long upload spends without giving + // this worker a single event. A stream posted to a worker in that state is + // lost, the iframe then wakes it with no entry to find, and the request falls + // through to the network — measured as a 404 from the hub and thirty seconds + // of nothing, twice, before the download started at all. + // + // The page waits for this before navigating, so the entry is known to be here + // rather than hoped to be. A page talking to an older worker gets no answer + // and navigates anyway, which is what it did before. + if (data.port) { + try { data.port.postMessage({ type: 'mbdl-ready', id: data.id }); } catch { /* gone */ } + } // A tab that is closed before it navigates would leave a stream here for the // life of the worker. setTimeout(() => pending.delete(data.id), 60000); diff --git a/packages/meshbay-hub/tests/test_downloads.py b/packages/meshbay-hub/tests/test_downloads.py index 41ae5d3..afb85d6 100644 --- a/packages/meshbay-hub/tests/test_downloads.py +++ b/packages/meshbay-hub/tests/test_downloads.py @@ -175,9 +175,23 @@ def test_backpressure_is_real(tmp_path): # The transfer list may carry more than the stream — a reply port rides # along now — so this asserts that `readable` is transferred, not the exact # shape of the list. - transfer = fn[fn.index("worker.postMessage("):] - transfer = transfer[transfer.index("["):transfer.index("]") + 1] - assert "readable" in transfer, "the readable half must be transferred, not copied" + # + # And every `postMessage` in here, not the first: a ping is sent to wake the + # worker before it is handed anything, and it carries only a port. Reading + # the first one would have moved this check onto the ping the day it was + # added, leaving the stream unguarded while still passing. + posts = [] + rest = fn + while "worker.postMessage(" in rest: + rest = rest[rest.index("worker.postMessage("):] + # Bounded by the call's own end: the keep-alive ping transfers nothing + # at all, and reaching past it for a `[` would read the next call's. + posts.append(rest[:rest.index(");") + 2]) + rest = rest[len("worker.postMessage("):] + lists = [c[c.index("["):c.index("]") + 1] for c in posts if "[" in c] + assert len(posts) >= 2, "the wake-up and the stream are both posted from here" + assert any("readable" in t for t in lists), ( + "the readable half must be transferred, not copied") assert "writer.write(bytes)" in fn assert "return null" in fn, "a browser that cannot transfer streams must say so" diff --git a/packages/meshbay-hub/tests/test_streamed_download_reliability.py b/packages/meshbay-hub/tests/test_streamed_download_reliability.py index bfd4a89..e1b3800 100644 --- a/packages/meshbay-hub/tests/test_streamed_download_reliability.py +++ b/packages/meshbay-hub/tests/test_streamed_download_reliability.py @@ -50,15 +50,37 @@ globalThis.localStorage = { }; const PLAN = %(plan)s; const log = { registers: 0, claims: 0, navigations: 0, served: 0, - unregisters: 0 }; + unregisters: 0, wakes: 0 }; // The worker as the page sees it: something with postMessage. It answers a // navigation by posting mbdl-serving back on the port it was handed, which is // exactly the confirmation the real sw.js sends from its fetch handler. let controller = null; const pendingByFrame = new Map(); +// Set before the controller exists, because the declaration below is what +// the temporal dead zone protects. +let asleep = PLAN.workerAsleep; const makeController = () => ({ postMessage: (msg, transfer) => { + // A worker with nothing to do is terminated, and `pending` goes with it. + // A ping wakes it; anything else posted while it sleeps is simply lost, + // which is what makes this failure silent. + if (asleep) { + if (msg.type === 'mbdl-ping') { + asleep = false; + log.wakes += 1; + if (msg.ports || (transfer && transfer[0])) { + const port = (transfer && transfer[0]) || null; + if (port) setTimeout(() => port.postMessage({type: 'mbdl-pong'}), 0); + } + } + return; + } + if (msg.type === 'mbdl-ping') { + const port = (transfer && transfer[0]) || null; + if (port) setTimeout(() => port.postMessage({type: 'mbdl-pong'}), 0); + return; + } if (msg.type === 'mbdl-claim') { log.claims += 1; // A worker that actually claims when asked, which is what sw.js does. @@ -70,6 +92,9 @@ const makeController = () => ({ } if (msg.type !== 'mbdl') return; pendingByFrame.set('/_mbdl/' + msg.id, msg.port); + // The worker says it has it, which is what the page waits for. + if (msg.port) setTimeout(() => msg.port.postMessage({type: 'mbdl-ready', + id: msg.id}), 0); }, }); @@ -167,7 +192,8 @@ def _run(tmp_path, body, *, control_after_ms=0, active=True, serve="always", register_throws=False, control_budget_ms=800, ready_settles=True, register_hangs=False, active_after_unregister=False, control_on_claim=False, - controlled_at_load=False, registered_at_load=False): + controlled_at_load=False, registered_at_load=False, + worker_asleep=False): module = tmp_path / "downloads.mjs" module.write_text(DOWNLOADS.read_text()) (tmp_path / "package.json").write_text('{"type":"module"}') @@ -181,6 +207,7 @@ def _run(tmp_path, body, *, control_after_ms=0, active=True, "activeAfterUnregister": active_after_unregister, "controlOnClaim": control_on_claim, "registeredAtLoad": registered_at_load, + "workerAsleep": worker_asleep, } script = tmp_path / "case.mjs" script.write_text( @@ -530,3 +557,33 @@ def test_the_streamed_target_says_it_cannot_be_paused(tmp_path): if (target) await target.writable.close(); """) assert out["pausable"] is False + + +# ── a worker that was asleep when we posted ───────────────────────────────── + +def test_a_sleeping_worker_is_woken_before_it_is_handed_a_stream(tmp_path): + """Reported from Chrome: a download started while an upload was running took + thirty seconds to begin, every time. + + `pending` lives in the worker's memory and a worker with nothing to do is + terminated — which is what a long upload leaves it, for minutes, since a + WebRTC transfer gives it no events at all. The stream posted to it was lost; + the iframe then woke it with nothing to find and the request fell through to + the network, measured in the console as a 404 from the hub and fifteen + seconds of silence, twice. + + `mbdl-ping` already existed — sent every ten seconds *while* writing, for + the same reason. Nothing sent one before *starting*. + """ + out = _run(tmp_path, """ + const target = await M.openStreamedDownload('film.mkv', 20e9, FAST); + out.target = target !== null; + out.wakes = log.wakes; + out.navigations = log.navigations; + if (target) await target.writable.close(); + """, worker_asleep=True) + assert out["target"] is True, "the download never started" + assert out["wakes"] == 1, "the worker was handed a stream while asleep" + assert out["navigations"] == 1, ( + f"took {out['navigations']} attempts — the first one was wasted on a " + "worker that had not been woken") -- cgit v1.2.3