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') 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