diff options
4 files changed, 238 insertions, 7 deletions
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*=<charset>'<lang>'<value>`. 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*=<charset>'<lang>'<value>`. 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 |