diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-09 02:09:52 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-09 02:09:52 +0200 |
| commit | 7e480254a014a3e72815e8b971d5560d42872c5a (patch) | |
| tree | 82fc06461330cf247a9c0c9370d0ccbf5e465e80 | |
| parent | d6c4808d9a3ef6bc740cda7892508f15ee9ea030 (diff) | |
| download | meshbay-7e480254a014a3e72815e8b971d5560d42872c5a.tar.gz | |
fix(spa): the target queue must not be able to freeze a batch
Four downloads on Firefox all sat at "preparing", with the node journal showing
`d=0/8(q0) u=0/8(q0)` — not one transfer opened, so nothing had got past the
client's target opening.
Serialising those openings was new in d6c4808, and on Firefox it regressed what
had always worked: four openings that ran at the same time began waiting on the
slowest. `_targetQueue` is module-level and never reset, so an opening that
never settles leaves the page unable to start any download again until it is
reloaded.
Two bounds, both narrowings of the queue rather than of any capability:
Only an opening that could actually show a dialog joins it. Firefox and Safari
have no `showSaveFilePicker` at all, so nothing there can race anything and the
queue bought nothing while costing everything; they now bypass it entirely,
which restores the previous behaviour by construction rather than by tuning.
And no opening waits behind another for longer than TARGET_QUEUE_BUDGET_MS
(90s) — generous enough never to cut in front of a real dialog, finite because
the alternative is a download panel that only a reload can fix. Releasing early
is safe: whatever is ahead is still the only unbatched opening, so the released
one takes the streamed path and opens no second dialog.
Measured on Firefox 154 against the deployed hub before writing any of this:
`register` and `ready` return instantly, the page is controlled, and four
serialised openings are served in 5-18 ms. The streamed path was never the
delay; the queue was.
Both new cases were checked against the unfixed source: without the bypass the
peak concurrency is 1 instead of 4, and without the budget the stuck-opening
case hangs. Hub suite 826 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCGdheDLxGReuKHga3BtST
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/static/file-utils.js | 37 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_downloads.py | 124 |
2 files changed, 129 insertions, 32 deletions
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js b/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js index 065079b..ef074dd 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/file-utils.js @@ -218,16 +218,42 @@ async function _openDownloadTarget(filename, size = 0, pickerOpts = {}, // // So the queue is explicit now, and it is the *targets* that queue, not the // rows: every download still appears the moment it is asked for. +// +// Two things keep the queue from becoming the problem it was meant to solve. +// It only ever holds openings that could actually put a dialog on screen, and +// no opening waits behind another for longer than a budget. let _targetQueue = Promise.resolve(); let _targetsInFlight = 0; +// How long an opening waits for the one ahead of it before going anyway. +// +// A queue with no bound is a way for one stuck opening to freeze every later +// download for the life of the page, since `_targetQueue` is never reset. That +// is what turned a slow first download into four rows stuck at "preparing" on +// Firefox. Generous, because a dialog legitimately waits for a person and +// cutting in front of one would be worse than waiting; finite, because the +// alternative is a download panel that never recovers. +// +// Going anyway is safe: whatever was ahead is still the only unbatched opening, +// so the one released here takes the streamed path and opens no second dialog. +const TARGET_QUEUE_BUDGET_MS = 90000; + function _openTargetInTurn(filename, size, pickerOpts, swSize) { + // Only an opening that could show a dialog has any reason to wait. Firefox + // and Safari have no `showSaveFilePicker` at all, so nothing there can race + // anything, and queueing them bought nothing while costing everything: four + // downloads that used to open their targets at the same time became four + // that waited on the slowest. + const canPick = typeof window !== 'undefined' + && typeof window.showSaveFilePicker === 'function'; + if (!canPick) return _openDownloadTarget(filename, size, pickerOpts, swSize); + // Anything that has to wait its turn is, by definition, not the first of the // batch — so it will not be the one holding the user's gesture. const batched = _targetsInFlight > 0; _targetsInFlight += 1; - const mine = _targetQueue + const mine = _waitBriefly(_targetQueue, TARGET_QUEUE_BUDGET_MS) .then(() => _openDownloadTarget(filename, size, pickerOpts, swSize, { batched })) .finally(() => { _targetsInFlight -= 1; }); @@ -237,6 +263,15 @@ function _openTargetInTurn(filename, size, pickerOpts, swSize) { return mine; } +/** Settles with `promise`, or after `ms`, whichever comes first. */ +function _waitBriefly(promise, ms) { + return new Promise((resolve) => { + const timer = setTimeout(resolve, ms); + promise.then(() => { clearTimeout(timer); resolve(); }, + () => { clearTimeout(timer); resolve(); }); + }); +} + /** The download of last resort, for browsers with no way to stream to disk. */ function _saveBlob(blob, filename) { const url = URL.createObjectURL(blob); diff --git a/packages/meshbay-hub/tests/test_downloads.py b/packages/meshbay-hub/tests/test_downloads.py index 81ab9d3..dfb1433 100644 --- a/packages/meshbay-hub/tests/test_downloads.py +++ b/packages/meshbay-hub/tests/test_downloads.py @@ -368,6 +368,52 @@ def test_the_worker_is_kept_alive_while_it_streams(): assert "mbdl-ping" in sw and "mbdl-pong" in sw +def _turn_harness(tmp_path, name, body, *, picker=True, budget_ms=90000): + """Run the real `_openTargetInTurn` against a stubbed opener. + + Both it and `_waitBriefly` are lifted out of `file-utils.js` as text; only + the budget is supplied here, so a case about the budget need not wait a + minute and a half for it. + """ + src = (STATIC / "file-utils.js").read_text() + + def lift(decl): + cut = src[src.index(decl):] + return cut[:cut.index("\n}\n") + 2] + + picker_js = ("window.showSaveFilePicker = async () => ({});" + if picker else "") + script = tmp_path / f"{name}.mjs" + script.write_text(f""" +const out = []; +let live = 0, peak = 0; +const asked = []; +// Stands in for _openDownloadTarget: records how many are open at once, and +// whether each was told it is not the first of its batch. +const _openDownloadTarget = async (name, size, opts, swSize, flags) => {{ + live += 1; peak = Math.max(peak, live); + asked.push(!!(flags && flags.batched)); + if (name === 'stuck') return await new Promise(() => {{}}); + await new Promise(r => setTimeout(r, 20)); + live -= 1; + if (name === 'boom') throw new Error('refused'); + return {{ name }}; +}}; +// Only a browser with a Save As dialog has anything to serialise. +globalThis.window = {{}}; +{picker_js} +let _targetQueue = Promise.resolve(); +let _targetsInFlight = 0; +const TARGET_QUEUE_BUDGET_MS = {budget_ms}; +""" + lift("function _openTargetInTurn") + lift("function _waitBriefly") + f""" +{body} +console.log(JSON.stringify(out)); +""") + proc = subprocess.run(["node", str(script)], capture_output=True, text=True) + assert proc.returncode == 0, proc.stderr + return json.loads(proc.stdout) + + def test_targets_are_opened_one_at_a_time(tmp_path): """ A browser shows one file picker at a time and grants one per user gesture, @@ -378,8 +424,7 @@ def test_targets_are_opened_one_at_a_time(tmp_path): Opening the target inside `prepare` — so the row appears at the click rather than tens of seconds later — removed the accident, and four pickers raced. Reported from Chrome: one file downloaded, a prompt for the second, the - other two timed out. Firefox and Electron never noticed, because neither - opens a picker at all, which is why this reached one browser only. + other two timed out. The queue is on the *targets*, never on the rows: every download still appears the moment it is asked for. @@ -387,43 +432,60 @@ def test_targets_are_opened_one_at_a_time(tmp_path): Queueing alone was not enough: a second dialog with no gesture behind it still waits for a human, and the two behind it wait for the dialog. So everything that has to wait its turn is also marked `batched`, which the - opener reads as "do not ask" — see the streamed-path branch below. + opener reads as "do not ask" — see the streamed-path branch in + test_memory_ceiling.py. """ - src = (STATIC / "file-utils.js").read_text() - fn = src[src.index("function _openTargetInTurn"):] - fn = fn[:fn.index("\n}\n") + 2] - - script = tmp_path / "case.mjs" - script.write_text(""" -const out = []; -let live = 0, peak = 0; -// Stands in for _openDownloadTarget: records how many are open at once. -const asked = []; -const _openDownloadTarget = async (name, size, opts, swSize, flags) => { - live += 1; peak = Math.max(peak, live); - asked.push(flags && flags.batched); - await new Promise(r => setTimeout(r, 20)); - live -= 1; - if (name === 'boom') throw new Error('refused'); - return { name }; -}; -let _targetQueue = Promise.resolve(); -let _targetsInFlight = 0; -""" + fn + """ + peak, statuses, batched = _turn_harness(tmp_path, "one_at_a_time", """ const results = await Promise.allSettled( ['a', 'boom', 'c', 'd'].map(n => _openTargetInTurn(n))); out.push(peak); out.push(results.map(r => r.status).join(',')); out.push(asked); -console.log(JSON.stringify(out)); """) - proc = subprocess.run(["node", str(script)], capture_output=True, text=True) - assert proc.returncode == 0, proc.stderr - peak, statuses, batched = json.loads(proc.stdout) assert peak == 1, f"{peak} targets were being opened at once" - assert batched == [False, True, True, True], ( - "only the first of a batch holds the user's gesture; the rest must be " - "opened without asking") # And one refusal must not stop the rest: a chain that breaks on a rejection # leaves every later download unable to open anything at all. assert statuses == "fulfilled,rejected,fulfilled,fulfilled" + assert batched == [False, True, True, True], ( + "only the first of a batch holds the user's gesture; the rest must be " + "opened without asking") + + +def test_a_browser_with_no_dialog_does_not_queue_at_all(tmp_path): + """Firefox and Safari have no `showSaveFilePicker`, so no two openings there + can race a dialog and there is nothing for a queue to protect. + + Queueing them anyway was a regression: four downloads that had always opened + their targets at the same time began waiting on the slowest, and all four + sat at "preparing". A queue that buys nothing must not be paid for. + """ + peak, = _turn_harness(tmp_path, "no_picker", """ +await Promise.all(['a', 'b', 'c', 'd'].map(n => _openTargetInTurn(n))); +out.push(peak); +""", picker=False) + assert peak == 4, ( + f"only {peak} target opening(s) ran at once; without a dialog to " + "serialise, all four must proceed together as they did before") + + +def test_one_stuck_opening_does_not_hold_the_others_for_ever(tmp_path): + """`_targetQueue` is never reset, so an opening that never settles would + otherwise leave the page unable to start any download again — a panel that + only a reload can fix. + + The budget is 60 ms here; in the page it is ninety seconds, long enough that + a real dialog is never cut in front of. + """ + statuses, batched = _turn_harness(tmp_path, "stuck", """ +const first = _openTargetInTurn('stuck'); +first.catch(() => {}); +const rest = await Promise.allSettled( + ['b', 'c'].map(n => _openTargetInTurn(n))); +out.push(rest.map(r => r.status).join(',')); +out.push(asked); +""", budget_ms=60) + assert statuses == "fulfilled,fulfilled", ( + "an opening that never settles must not strand the ones behind it") + assert batched == [False, True, True], ( + "the stuck one is still the only holder of the gesture, so the released " + "openings must not try for a dialog of their own") |