Skip to content

fix(storage): video pieces really go to disk; saves keep their file; fewer download stalls - #304

Merged
Nawid3333 merged 15 commits into
mainfrom
claude/issues-storage
Oct 3, 2026
Merged

fix(storage): video pieces really go to disk; saves keep their file; fewer download stalls#304
Nawid3333 merged 15 commits into
mainfrom
claude/issues-storage

Conversation

@Nawid3333

@Nawid3333 Nawid3333 commented Oct 3, 2026

Copy link
Copy Markdown
Owner

Storage and downloads: video pieces really go to disk, saves keep their file, fewer stalls

What was wrong, as you'd notice it:

  • With "download entire video" on, Firefox kept every downloaded piece of the video in memory. The on-disk store was never used for playback: each piece was named after its web address, and Firefox refuses file names containing a "/". A long video could use gigabytes of RAM.
  • When the disk was slow, a queue of writes longer than 30 seconds was taken for a crash, and the on-disk store was switched off for the rest of the video.
  • When the disk was full, a write could be cut short without any error, so a saved video could end up with a hole in it.
  • A save that took Firefox more than two minutes to copy (a big video to a slow disk) could lose its file halfway, with no message.
  • "Save the buffer" (.fsa) said "saved" before the file had actually been handed to Firefox, and a failure at that point showed nothing.
  • A damaged .fsa file could make the player try to reserve up to 4 GB of memory, or fail with an unclear error.
  • After a few failed downloads in a long video, the player kept downloading with fewer connections until the next video. Right after a failure, it fetched only one piece at a time.
  • When a server ignored a request for one part of a file and sent the whole file, the file's first bytes were played as that part.
  • A custom header Firefox can't send was retried for about a minute before the error showed.
  • On high-bitrate MP4s, the player could show a false "not enough storage" warning, and then kept every downloaded piece for the whole session.
  • The download speed could show "Infinity" for the rest of the session.
  • Smaller fixes:
    • the temporary database cleanup could block another tab from deleting its own data;
    • a failed database write could look successful;
    • a seek during a DASH download could make the next download fail.
  • Opening a video waits less before it starts: the two checks behind the remembered position now run at the same time.

How it's proven:

  • Every fix has a unit test that fails without it.
  • The storage tests run the real storage worker against a stand-in disk that follows Firefox's file-name rules and can fill up. Giving the worker web addresses as names again turns that test red.
  • The browser storage test (run by CI) now stores a piece named by its web address.
  • Lint, typecheck and 1007 unit tests pass.

For the final try-list: this switches on, for the first time, the on-disk store for playback pieces. Things to try:

  • play a long HLS or DASH video with "download entire video" on: memory should stay flat;
  • seek back into the downloaded part;
  • switch to another video afterwards: the switch shouldn't be slower;
  • save a DASH or HLS video, and a .fsa archive;
  • play in a private window.

Fixes #132
Fixes #133
Fixes #135
Fixes #136
Fixes #137
Fixes #138
Fixes #139
Fixes #140
Fixes #141
Fixes #148
Fixes #149
Fixes #150
Fixes #210
Fixes #251

Not fixed here:

🤖 Generated with Claude Code

Nawid3333 and others added 15 commits October 3, 2026 15:46
@Nawid3333 @claude
fix(storage): store fragments in OPFS at all, survive a write backlog…
0848099 
…, catch short writes

- Every downloaded fragment's identifier is its URL and range
  ("https://host/seg.ts::0-100::arraybuffer"), and Firefox refuses an OPFS name
  with a "/" (IsValidName, dom/fs/shared/FileSystemHelpers.cpp). So no fragment
  was ever stored: each write failed, was logged, and the fragment stayed in RAM.
  OPFSManager now gives each identifier a short file name of its own (fileName)
  and the worker only sees those (#132).
- The 30 s worker timeout ran from when a call was posted. The worker answers
  one call at a time, so a backlog of writes longer than that (a whole video
  downloading to a slow disk) counted as a crash and OPFS was dropped for the
  session. One watchdog now measures how long the worker has gone without
  answering any call, and no timer is left per call (#135).
- FileSystemSyncAccessHandle.write() reports a write that failed part-way (disk
  or quota full) only through the byte count it returns; Gecko never throws
  for it. Nothing checked it, so a fragment could be stored cut short and a save
  go on with a hole. Every write now checks the count; a fragment that did not
  fit is removed and kept in RAM, and a save fails (#138).

Proven in tests/unit/opfsStorage.test.mjs, which now runs the real worker on a
stand-in OPFS that applies Firefox's name rule and can run out of space: with
the old OPFSManager the URL-named fragment and the backlog cases fail, and with
the count check removed the three short-write cases fail. storage.e2e.mjs
round-trips a URL-named fragment in real Firefox.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(save): keep a streamed save's file until its download is over
0e16175 
A streamed save (accelerated MP4, a direct video, an archive) writes its file
into its own OPFS session and hands Firefox a URL to it. The session was closed
two minutes after the save ended, whatever the download was doing, and a closed
session is deleted by the next player or save that starts (10 s after its last
heartbeat). So a download longer than that - a big video to a slow disk - could
lose its file midway, with no message.

Utils.revokeWhenDownloaded now says (a promise) when the download is over, and
StreamSaver closes the store then, never sooner than the old two minutes, which
is still all a download without an id to follow gets. Both sinks (OPFS and
memory) do the same.

tests/unit/StreamSaver.test.mjs: with the old timer the store closes at two
minutes of a ten-minute download (fails); without the floor it closes right
after a quick download (fails). revokeWhenDownloaded.test.mjs checks the promise.
Part of #134: merged saves (DASH, HLS) still close their store on the timer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(archive): check a length from an archive before allocating it, re…
c03554e 
…ad sizes unsigned

An .fsa archive's header and entry sizes come from the file. LargeBuffer.read
allocated the buffer before checking the length against what was left, so a
damaged or crafted archive could ask for up to 4 GB before "out of range". The
32-bit size was assembled with a signed `<< 24`, so a size from 2 GB up came out
negative and failed with an unexplained RangeError, and getParts took a negative
length as "read nothing", which put every later read in the wrong place.

The length is now checked first (and must be a whole, non-negative number), and
uint32() is unsigned. tests/unit/LargeBuffer.test.mjs and
FastStreamArchiveUtils.test.mjs ("says a header size past the end of the file is
out of range"): all four new cases fail on the old LargeBuffer.

Fixes #141.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(archive): finish or abort the save stream when writing an archive
1eb6ca3 
writeFSAToStream did not await writer.close(), so "Saved" showed before the
save's stream had handed its file to the download, and a failure there (quota,
download refused) was an unhandled rejection with no message. An entry that was
not complete, or a failed read, threw with the stream left open: the save's OPFS
file handle, worker and session stayed until the tab closed.

It now awaits close() and, on any failure, aborts the stream (which ends the
save's session) before passing the error on.

tests/unit/FastStreamArchiveUtils.test.mjs: the three writer cases fail on the
old code (and the failing close is an unhandled rejection there).

Fixes #136.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(storage): IndexedDB prune lets go of live databases; transact alw…
f6505cb 
…ays answers

FSBlob's last backend (used when OPFS and the Cache API both refuse):
- prune() opened every other player's temporary database to read its heartbeat
  and closed only the stale ones. A live player's connection stayed open, so
  when that player closed, its deleteDatabase was blocked (5 s timeout, then
  the database stayed on disk) until the pruning tab closed too. The connection
  is now closed either way (#137).
- transact() had no abort handler, so a commit that failed (quota exceeded
  aborts without an error event) never answered. And a callback that threw
  before making its request (a value that cannot be stored) left a transaction
  with nothing in it, which completed: the call answered null as if it had
  worked, and FSBlob then kept `undefined` as the fragment. Both now reject; the
  executor is no longer async (its throw was an unhandled rejection). keepAlive
  catches a failed beat, which is retried a second later (#149).

tests/unit/IndexedDBManager.test.mjs: on the old code the first transact case
answers "resolved: null", the second stays pending, and the prune case leaves the
live connection open.

Fixes #137, fixes #149.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(network): a request that fails after its download was aborted fai…
576cb50 
…ls nothing else

StandardDownloader.run makes the request (a DASH segment's preprocessor) before
loading it. When the entry was aborted meanwhile (a seek) and the request then
failed, the catch used `this.entry`: null after the abort (a TypeError, an
unhandled rejection), or the next entry the downloader had started by then,
which it failed and cleaned up. It now returns when its own run was aborted.

tests/unit/StandardDownloader.test.mjs: both new cases fail on the old code (the
next download is failed; an unhandled rejection with nothing else running).

Fixes #148.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(network): downloaders dropped after failures come back; every fre…
edc2a6c 
…e one gets work

- A failed download (after XHRLoader's own retries) takes one downloader away,
  to ease off a struggling server, and nothing gave it back: three failures in
  a long video left three downloaders of six until the next video. Now, once no
  download has failed for 10 s, each successful download brings one back, up to
  as many as were taken. A downloader the user adds or removes resets that.
- queueNext started at most one download per call. After a failure it is
  called once, when the 1 s wait is over, so with five free downloaders and
  forty queued fragments one ran at a time until other calls came in. It now
  starts a download on every free downloader.

tests/unit/DownloadManager.test.mjs: each case fails with its fix taken out
(one download started of three possible; the pool stays at one).

Fixes #133.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(network): speed over a zero-length window is 0, and prune's test …
a2c3bf4 
…can fail

- SpeedTracker.getSpeed divided by the time since its oldest entry started. A
  response that arrived within one performance.now() tick of being asked for
  (a cache hit; Firefox's clock moves in 1 ms steps) gave Infinity. The speed
  shown smooths each value into the last, so it then said Infinity for the rest
  of the session, and the download speed limit stopped predownloading while it
  lasted. A zero window now counts as 0, like no data (#140).
- The prune test added its stale entries through update(), which already
  prunes, so it passed with prune() doing nothing (`while (false)`: 6 of 6). The
  prune cases now fill the buffer directly and check exactly what is left: four
  stale entries leave the last two, two stale entries are both kept, and only
  stale entries go (#251). SpeedTracker.mjs is left out of Stryker's list here;
  #253 adds it with the other tested modules.

Mutation-checked: `while (false)` fails three cases, a floor of 1 instead of 2
fails three, a wider cutoff fails one, and without the zero check the new
getSpeed case fails.

Fixes #140, fixes #251.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(network): ignored Range answers are cut to the range; unsendable …
3c26d2c 
…headers fail at once

- A server that ignores the Range header answers 200 with the whole file. That
  was taken for the range asked for: the file's first bytes, handed to the
  player as a range further in (a byte-range HLS segment, a DASH index or
  segment, an MP4 range). XHRLoader now cuts the asked-for range out of such an
  answer and stops reading once it is in; a range that starts past the end of
  the file fails as 416 (#139).
- A header fetch() cannot send (a custom header whose name has a space) made
  fetch() reject with a TypeError, which was retried like a network error, six
  times over about a minute. The headers are now checked with `new Headers()`
  first, and such a load fails at once (#150).

tests/unit/XHRLoader.test.mjs: the header case and three of the four ignored-
Range cases fail on the old loader; without the reader.cancel() the "stops
reading" case fails.

Fixes #139, fixes #150.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(player): no false "not enough storage" on high-bitrate MP4s
3afbbd8 
updateHasDownloadSpace estimates the video's bitrate from the downloaded
fragments once five are in: their bytes over their duration. MP4Player starts
its 1 MB ranges on whole seconds, so on a video of about 40 Mbit/s or more the
first five lie in the first second and last 0 s. The bitrate came out as
Infinity: the player said storage was short, set the buffer back, and marked
every downloaded fragment to be kept for the rest of the session.

The estimate is now Utils.measuredBitrate, which answers null (use the level's
own bitrate, as before five fragments) while the fragments span no time.
tests/unit/measuredBitrate.test.mjs; without the zero-time check its third case
gets Infinity.

Fixes #210.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
perf(progress): derive the two remembered-position hashes side by side
fa17116 
Opening a video looks up its remembered position under two PBKDF2 hashes of its
address (600,000 rounds each), and the video's start - autoplay, the seek to the
time in its URL - waits for that lookup. The two were derived one after the
other; they are now derived at once. The hashes, and so the stored records, are
unchanged. In Node the pair takes about 100 ms instead of 180 ms; Firefox runs
WebCrypto work off the main thread as well, so the wait should drop the same way
(not measured there).

tests/unit/SecureMemory.test.mjs: the new case fails on the old code (the second
hash starts only after the first ends).

Part of #146 (the proposal to replace PBKDF2 is left for a decision).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
style(save): await the download in closeWhenDownloaded rather than ch…
8af51b0 
…ain it

eslint.modules.config.js's promise/catch-or-return flagged the bare .then()
chain added in the previous StreamSaver commit. Same behaviour; the
StreamSaver tests still pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
docs: OPFS file names and the worker watchdog in CLAUDE.md
85551bf 
What a later reader of the OPFS code needs: fragments were never stored there
until #132's fix, files are found by OPFSManager.fileName(), write() reports a
full disk only by its count, and the 30 s limit is a watchdog over the worker's
answers, not a per-call timer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
fix(network): a 200 that says it is the range is still taken as the r…
c6e759b 
…ange

Narrows the previous commit's ignored-Range handling to the clear case. A 200
with a Content-Range header, or exactly as long as the range asked for (from a
start past 0, which no whole file can be), is a server sending the range with
the wrong status; that worked before and is left alone. Only a 200 without
either is the whole file and has the range cut out of it.

tests/unit/XHRLoader.test.mjs: "takes a 200 that says it is the range" fails
with either check taken out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nawid3333 @claude
test(save): the StreamSaver test's stand-in store has whenStored
9b6bddc 
The save PR's memory sink waits on FSBlob.whenStored. With both PRs merged, this test's
stand-in lacked it and two memory-sink cases failed; one no-op method fixes that, so the
two PRs merge in either order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 3, 2026
Merged
Merged
@Nawid3333
Nawid3333 merged commit 5520d8a into main Oct 3, 2026
9 of 10 checks passed
@Nawid3333
Nawid3333 deleted the claude/issues-storage branch October 3, 2026 20:28
This was referenced Oct 3, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment