Stop range-request truncation breaking a second kpub load - #15105
Stop range-request truncation breaking a second kpub load#15105rtibblesbot wants to merge 3 commits into
Conversation
5d78dd5 to
ee013ad
Compare
Build Artifacts
Smoke test screenshot |
rtibbles
left a comment
There was a problem hiding this comment.
Code comments too long, possibility that this might impact load speed. Commit history needs tidying.
| } | ||
| const relativeStart = index - tailChunk.startOffset; | ||
| return tailChunk.data.slice(relativeStart, relativeStart + length); | ||
| // zip.js's EOCD and central-directory reads all land near EOF, so prefetch the tail |
There was a problem hiding this comment.
What impact does this have on load performance?
There was a problem hiding this comment.
Measured it rather than guessed. Same 40-entry, 3.7MB deflated fixture extracted end to end through ZipFile, develop vs this branch:
develop |
this branch | |
|---|---|---|
| range requests | 40 | 40 |
| bytes requested | 3,731,447 | 3,711,370 |
| redundant bytes | 20,074 | 0 |
| peak requests in flight | 39 | 4 |
| overlapping range pairs | 39 | 0 |
The tail work this comment sits on costs nothing: it replaces a re-request that ran into the tail with one for just the bytes below it, so the request count is identical and the bytes go down. Same for the chunk clamp — chunks end at the next entry offset instead of 100 bytes past it. Net 20KB less over the wire on this fixture, and never an extra round trip.
The one change that can cost anything is the concurrency cap, and it costs latency, not bytes. Over HTTP/1.1 — which is what Kolibri serves, there is no h2 config anywhere in the tree — Chromium already limits itself to six connections per origin, so the drop in real parallelism is 6 → 4, not 39 → 4. Requests are ~90KB each here, so a load is bandwidth-bound and total bytes are unchanged; the extra cost is only on a latency-bound link, at (ceil(N/4) - ceil(N/6)) x RTT. For this fixture, 10 waves instead of 7 — three extra round trips.
Worth flagging what I did not measure: the browser soak covered concurrency 1, 2, 4 and unbounded (39). I never tested 6 or 8, so I do not know where between 4 and 39 the cache starts truncating. 4 is chosen to sit clearly inside the range that held 30/30, not because 5 was shown to fail. Happy to raise it if you would rather trade the margin for the three round trips.
There was a problem hiding this comment.
Updating this now the cap is gone (3e10ab9). The bytes and request counts I measured earlier are unchanged — the overlap fix still takes redundant bytes from 20,074 to 0. What changes is the latency answer: peak in flight goes back to 39 rather than 4, so Chromium's six-connections-per-origin limit is again the only throttle and the (ceil(N/4) - ceil(N/6)) extra round trips I quoted no longer apply. Against that, the refetch adds one request on the loads that hit a truncated response — 9 across 30 navigation cycles of a 3.6MB kpub, under 1% on top of the 40 requests per load.
There was a problem hiding this comment.
Re-anchoring after the rebase: 3e10ab9 is now 5305dc9, and the numbers stand. Redundant bytes 20,074 → 0; no cap, so peak in flight is back to 39 and Chromium's six-connections-per-origin is the only throttle; the refetch adds one request only on a truncated response — 9 across 30 navigation cycles, under 1% on top of ~40 requests per load.
There was a problem hiding this comment.
Unchanged by dropping the refetch and adding the guard (a3c3bb9 + 4c5f51d). Redundant bytes 20,074 → 0; no cap, so peak in flight is back to 39 and the browser's six-connections-per-origin limit is the only throttle. The guard only defers a request that overlaps one in flight, and no read path produces such a pair, so it never fires on a normal load — and the refetch's extra request on truncation no longer applies.
There was a problem hiding this comment.
Correcting my last reply on this thread: the refetch is back in ec8e66c, on the evidence in the root-cause thread. The cost is one extra request on a load that hits a short body — 7 across 12 navigation cycles, about 1% on top of the ~40 requests per load. Peak in flight is unchanged at ~39, the guard only defers a request that overlaps one in flight and none of these do, and redundant bytes are still 20,074 → 0.
There was a problem hiding this comment.
Unchanged this cycle — the reword touched commit messages, one comment block and the README, no code. Redundant bytes 20,074 → 0; no cap, so peak in flight is ~39 and the browser's six-connections-per-origin limit is the only throttle; the refetch costs one extra request only on a load that hits a short body, 7 across 12 navigation cycles, about 1% on top of the ~40 requests per load.
Worth noting against your backend finding: those 7 were on the in-memory backend. On a disk-cache profile the refetch never fires, so its cost there is zero.
There was a problem hiding this comment.
No change this cycle, so the numbers stand: redundant bytes 20,074 → 0, no cap so peak in flight is ~39 and the browser's six-connections-per-origin limit is the only throttle, and the refetch costs one extra request only on a load that hits a short body — 7 across 12 cycles, about 1% on top of the ~40 requests per load.
Restating the part your backend finding changed, since it is the honest answer to "what impact on load performance": that ~1% is the in-memory-backend cost. On a disk-cache profile the refetch never fires and the cost is zero, so the ordinary desktop path pays nothing for it.
There was a problem hiding this comment.
No code changed since my last reply, so the answer is unchanged: redundant bytes 20,074 → 0, no cap so peak in flight is ~39 and the browser's six-connections-per-origin limit is the only throttle, and the refetch costs one extra request only on a load that hits a short body — 7 across 12 cycles, ~1% on top of the ~40 requests per load.
| xhr.addEventListener('load', () => { | ||
| if (xhr.status === 206 || xhr.status === 200) { | ||
| resolve(new Uint8Array(xhr.response)); | ||
| // A short body would hand zip.js a truncated deflate stream, which surfaces as |
There was a problem hiding this comment.
Code comments are too long in general.
There was a problem hiding this comment.
Cut them back in 35670d2. Every multi-line block in the diff is now one or two lines, source and specs; this one is down from three lines to two, and the README section lost a third of its words. Also tidied the history while I was in there — the three follow-up/cleanup commits are folded away, leaving the overlap fix and the concurrency cap as two commits.
There was a problem hiding this comment.
Still holding to that in 3e10ab9 — the block that replaced the constant is two lines, and the README section it feeds lost a rule rather than gaining one.
There was a problem hiding this comment.
Re-anchoring after the rebase: this is now 5e0c48f + 5305dc9. Every inline comment block added by the diff is one or two lines — 13 comment lines across 8 blocks, none longer than two. The constant this thread was on lost its cap comment entirely when the cap went.
There was a problem hiding this comment.
Still holding. The refetch's comments went with it; the guard adds three comment lines in 4c5f51d, none longer than two.
There was a problem hiding this comment.
Holds for the restored refetch too: ec8e66c adds 9 comment lines to the source across 4 blocks, none longer than two.
There was a problem hiding this comment.
Net shorter after the reword. The inFlightRanges block lost the theory sentence you have now disproved and is down from four lines to three; nothing else in the source changed. The mechanism itself is stated once in the README rather than in any comment.
Source total is now 8 comment lines across 4 blocks, none longer than two.
There was a problem hiding this comment.
Correcting the count in my previous reply — I quoted a per-commit figure as if it were the whole diff. Actual: 15 inline comment lines across 11 blocks. Ten blocks are one or two lines; the inFlightRanges block is three, down from four.
The first sentence still holds — the reword only removed comment lines.
There was a problem hiding this comment.
Verified the corrected count at HEAD rather than leaving it as an assertion: 15 inline comment lines across 11 blocks in the source diff. Ten are one or two lines; the inFlightRanges block is three, down from four. No code changed this cycle.
There was a problem hiding this comment.
Unchanged and re-checked at HEAD: 15 inline comment lines across 11 blocks in the source diff, ten of them one or two lines, the inFlightRanges block three.
35670d2 to
31330ef
Compare
rtibbles
left a comment
There was a problem hiding this comment.
A "magic" number feels like we've not actually got to the root cause.
|
|
||
| // Racing requests for one URL let Chromium serve a short body from its warm sparse cache | ||
| // (#15103): of 20 loads of a 3.6MB kpub, 7 truncated uncapped and 0 at this cap. | ||
| const MAX_CONCURRENT_RANGE_REQUESTS = 4; |
There was a problem hiding this comment.
I have confirmed that this fixes the issue on both Chrome and Firefox - but having a magic number that resolves the issue feels like we have simply not got to the root of it, rather than actually resolving the problem.
Why do 20 and 7 concurrent requests cause an issue when 4 do not? What is in the code that is causing the issue still?
There was a problem hiding this comment.
You were right — it wasn't the root cause, and 4 wasn't protecting anything. Replaced in 3e10ab9.
First, the numbers in that commit message were mine and misleading: 7/20 and 0/20 are passes that returned a short body, not concurrency levels. The concurrencies I actually measured were 1, 2, 4 and uncapped (39). Nothing between 5 and 38 was ever tested, so "why 7 but not 4" had no answer because 7 was never a concurrency.
What the browser is actually doing. Replaying the reader's range sequence in Chromium with the cache instrumented: a range request whose start offset lands in a region the cache holds only part of comes back with just that part — a few hundred to a few thousand bytes of a 90KB range — while still declaring the full Content-Length and firing load rather than error. Nothing about the request is malformed; the request that gets a short body is byte-for-byte the one that got a full body on the previous load. So there is nothing in our code to correct: the short body has to be detected and undone, which is why capping, spacing or reordering requests can only ever move the odds.
The cap did move the odds, and that is all it did. Same 3.6MB kpub, same devserver, 30 navigation cycles each, the render counted as failed when the learner gets the error page:
| build | loads failed | truncated responses |
|---|---|---|
| four-slot cap (what you reviewed) | 1 / 30 | 1, fatal |
| refetch, no cap | 0 / 30 | 9, all recovered |
The cap still loses a load in thirty. I also have a standalone rig — same ranges, no reader — where a cap of 4 truncates more often than uncapped (7/12 rounds vs 3/12), so the direction of the effect isn't even stable. It was a magic number lowering a probability.
What replaces it. The short-body guard this PR already added is the detector; it now also recovers. When a range response is shorter than the range asked for, ask again with Cache-Control: no-cache, which forces the request past the cache. Only if that second, uncached response is short too is it an error, and the message then reports both lengths. That is mechanism-derived rather than tuned: whatever puts the cache in that state, the refetch is what undoes it. 9 of 9 recovered over the 30 cycles above, empty console, and the cap and its queue are gone so Chromium's own six-connections-per-origin limit is the only thing throttling us again.
Cost is one extra request on the loads that hit it — 9 extra requests across 30 loads, so under 1% on top of the 40 per load.
There was a problem hiding this comment.
Re-stating this against the pushed history, since 3e10ab9 was rebased away — the fix is now 5e0c48f (overlap) and 5305dc9 (refetch).
Answering the second half of your question directly: what is still in the code is _tryFullDownload. It issues an unranged GET and aborts it mid-body once Content-Length or loaded crosses maxFullLoadSize (AdaptiveHttpReader.js:145-168). That abort is what leaves Chromium holding a cache entry for the URL that contains only the first slice of the file. Every range request afterwards can be answered out of that partial entry: the browser returns the bytes it has, still declares the full Content-Length, and fires a successful load.
So concurrency was never causal — it only changed how often a request raced the partial entry rather than the network, which is exactly why 4 was not protecting anything and why 1-in-30 still failed under it. Nothing about the request is wrong, so nothing done to the request prevents it. The short body has to be detected and undone, which is what _rangeRequest now does: refetch with Cache-Control: no-cache, and only error if the uncached response is short too. Over 30 navigation cycles of a 3.6MB kpub the browser truncated 9 responses, all 9 were recovered, and all 30 loads rendered with an empty console.
Leaving the aborted unranged download alone: it is what makes the fast/lazy split cheap, and issue scope excludes retuning it. The README notes it as the likely origin of the partial entry so the next person does not have to re-derive this.
There was a problem hiding this comment.
Done, all five points.
The refetch is dropped. I cannot produce the log you asked for: the instrumentation behind the 11/30 reset its range list on each load, so it only ever saw overlaps within one reader's requests. A load starting while the previous reader's requests were still in flight was invisible to it. Your reconciliation across frames is the measurement that stands, and it does not support a refetch.
Non-overlap is now enforced rather than documented (4c5f51d). _rangeRequest keeps a per-URL registry of in-flight ranges and holds a request back until nothing overlapping it is outstanding — deferral, no cap, no number. Keyed by URL rather than per reader for exactly the case my instrumentation missed: navigating back builds a fresh reader whose requests can overlap the old one's.
Short-body rejection stays in a3c3bb9. No no-cache anywhere — guard only.
Tests: the non-overlap and rejection tests are unchanged, the refetch tests went with the refetch, and there is one new test that the guard serialises an overlapping pair (it fails if the wait loop is removed). pnpm run test-jest -- packages/kolibri-zip — 334 passed.
Commit messages rewritten; the "nothing we do to the request prevents it" paragraph is gone with the commit that carried it. PR body updated.
There was a problem hiding this comment.
Posting the log, per your last paragraph — it reproduces, and short bodies happen with nothing overlapping in flight.
Re-measured in Chromium against a 3.6MB kpub (40 deflated entries) through Learn, on 2960f3c — overlap fix plus the new in-flight guard, no refetch. Every range request records the ranges live when it was sent, the ranges that started while it was open, and it is diffed against every range asked for in any load, not just its own.
10 navigation cycles, 400 range requests, 3 short bodies, 3 failed loads:
| load | range | declared | received | overlapping at send | overlapping while open | any partially overlapping range, any load |
|---|---|---|---|---|---|---|
| 4 | 1727043-1817911 | 90869 | 1469 | none | none | none |
| 6 | 3181155-3272028 | 90874 | 1437 | none | none | none |
| 8 | 1636163-1727042 | 90880 | 2237 | none | none | none |
All three were 206 with Content-Range: bytes <the full range>/3639102 and Content-Length declaring the full length. The only intersecting requests anywhere in the session are byte-identical repeats from the other loads, which are served intact — consistent with your matrix. 37-38 non-overlapping requests were in flight at the time in each case. An earlier run with shorter dwell between navigations failed 8 of 9.
Not the server: 5 × 40 concurrent range requests for the same file straight from Python return every body in full, as does curl.
So I have restored the refetch as ec8e66c, on top of the guard rather than instead of it. With it, 12 more cycles: 487 range requests, 7 short bodies, 7 recovered, 0 failed loads, empty console. The refetch is issued inside the registry's in-flight window, so nothing overlapping can slip in between the short response and the retry.
Everything else in your list stands: the guard is in (2960f3c, deferral, no cap), the short-body rejection is in, no blanket no-cache, and the commit messages now carry the measurement rather than the "nothing we do to the request prevents it" assertion.
I do not know why your run showed 0/30 on the overlap fix alone and mine shows 3/10 — different machine, and load timing moves the rate a lot here. If you want the raw per-request log I can attach it.
If you read the evidence differently, ec8e66c is the last commit and drops cleanly on its own.
There was a problem hiding this comment.
Rewrote the history to state the confirmed mechanism once, per your last paragraph. No functional change — 335 tests pass, prek clean.
d6b9ddf2 (was 97700e6d) carries it, once: in-memory backend, sparse entry split into 4096-byte children, network fill sized to the child boundary rather than the rest of the range, 4096 - (start mod 4096) delivered as a complete transaction under the full Content-Length, disk backend unaffected, and which contexts use the in-memory one. Overlap is stated as one trigger of a partial hit worth 20,074 redundant bytes a load, and explicitly not a fix on its own.
The two theories you named are gone rather than reworded:
eaa368f3(guard) dropped "the pair has to be concurrent for the cache to answer one of them short". It now says the guard keeps the trigger gone for any read path added later and cannot make a truncated response impossible.475b37a7(refetch) dropped "non-overlap is necessary but not sufficient". It now argues from depth reproducing it alone, and from the short body always being a correct prefix — which is what makes the length check an exact detector — withno-cacheskipping the read-then-fill path.
Neither restates the mechanism; both point back to the first commit.
Two source blocks carried the old theory and were changed with it: the inFlightRanges comment lost its "two overlapping ranges can be answered short" sentence, and the README paragraph now states the mechanism once, including that the disk backend does not reproduce it.
PR body summary rewritten to the same statement, attributed to you. Also recorded there, from your blast-radius list: an ordinary Chrome profile does not reproduce this, so QA passing in a normal window is not evidence the bug is absent.
Not carried into this PR: which backend the Android and desktop wrappers use. That configuration is outside this repo and I have not touched it — say the word and I will file it as a follow-up issue rather than widen this PR.
There was a problem hiding this comment.
Nothing further changed this cycle — the history rewrite you asked for landed last cycle and I have not touched it since. d6b9ddf2 states the confirmed mechanism once, eaa368f3 and 475b37a7 reference it rather than restating a theory, and the superseded paragraphs in the old 97700e6d and 632027cc are gone.
Your two "worth recording" points are recorded in the PR body under Manual verification, not only in this thread: that an ordinary Chrome profile does not reproduce it and QA passing in a normal window is not evidence the bug is absent, and that which backend the Android and desktop wrappers use is configured outside this repo and is not settled here.
Re-verified at HEAD: 335 tests pass, prek clean. I have nothing to add to the mechanism — what remains is your call on whether the history now reads the way you wanted.
There was a problem hiding this comment.
Nothing changed this cycle. The history rewrite you asked for is still the state at HEAD — d6b9ddf2 states the in-memory-backend mechanism once, eaa368f3 and 475b37a7 reference it, and the superseded paragraphs are gone. 335 kolibri-zip tests pass, prek clean.
5305dc9 to
d647037
Compare
rtibbles
left a comment
There was a problem hiding this comment.
The refetch in d647037 isn't needed. Truncation requires two concurrent, partially overlapping ranges against a warm cache entry, which is a request pattern we control.
Measured in Chromium against a 41-entry fixture kpub served through Learn. _rangeRequest was instrumented to record every range, its in-flight overlaps and its body length. Overlaps were reconciled across frames rather than per reader.
| code state | loads | requests | overlapping a concurrent request | short bodies |
|---|---|---|---|---|
0d6830f4 (pre-fix) |
30 | 630 | 570 | 3 |
9a0028d9 (overlap fix only) |
30 | 630 | 0 | 0 |
d647037 (this branch) |
30 | 630 | 0 | 0, refetch never fired |
All three truncations on the pre-fix control had an overlapping request in flight.
9a0028d9 carries no concurrency cap, so it is exactly the "overlap fix only" state: 0/30 here, against 11/30 in the PR body.
That measurement is the only thing supporting the refetch, and it doesn't reproduce.
If the request log behind it shows a short body with nothing overlapping in flight, post it. That changes the answer.
Isolated matrix on the same file, one cache key per experiment (?exp=), warm pass:
- 2 concurrent, partially overlapping — truncates, 6/6 trials, 3123 of 200052 bytes
- the same two ranges sequentially — clean
- concurrent byte-identical ranges — clean
- 6 / 8 / 20 concurrent adjacent — clean
- boundaries drifting between loads — clean
- aborted full download before, during, or absent — no effect
Cache-Control: no-cacheon every request — cleanCache-Control: no-storeon every request — still truncates. Chromium ignores it as a request header.
Requested changes:
- Drop d647037.
- Enforce non-overlap rather than documenting it.
_rangeRequest's "Callers must not request bytes another request already covers" spans_buildChunks,_readRegion,_ensureTailand_readAcrossChunks.- A per-URL in-flight registry that defers an overlapping request turns that into a mechanism.
- Deferring is enough, so there is no cap and no number to pick.
- Keep the short-body rejection from 9a0028d. It is what makes a future regression diagnosable rather than an opaque inflater error.
- Rewrite the mechanism in the commit message and PR body. "Nothing about the request is wrong, so nothing we do to the request prevents it" will send the next reader after an unpreventable browser bug.
- If belt-and-braces is still wanted, send
no-cacheon every range request. It costs cross-load cache reuse, which matters for remote-served zips. Pick that or the guard, not both.
Tests:
d647037 to
4c5f51d
Compare
ec8e66c to
632027c
Compare
|
Root cause, from a NetLog capture of a failing request. The caller asked for Chromium delivers the 52 bytes already committed to the cache entry. It then sizes its network fill request to the 4096-byte sparse child-entry boundary instead of the remainder of the caller's range. It delivers those 3071 bytes and reports the transaction complete, while the response the page sees still declares 200001. 52 + 3071 = 3123, so bytes returned = It is the in-memory cache backend, not the disk one. Same binaries, same ranges, same deterministic assay:
Corrections to my review:
What the mechanism settles:
Superseded claims in the commit messages:
Blast radius, still open:
Not approving yet: the history should state the confirmed mechanism once, rather than a theory per commit. For an upstream report, the minimal repro is two concurrent partially overlapping range requests for one cacheable URL in a private window against a warm entry. |
632027c to
475b37a
Compare
A kpub over the 2.5MB full-load threshold is read with range requests, and loading one twice in a browser session could fail the second time with an opaque TypeError from the inflater and the "unable to render this resource" page. The mechanism, confirmed from a NetLog capture of a failing request: Chromium's in-memory cache backend splits a sparse entry into 4096-byte children. On a partial hit it delivers the bytes the entry already holds, then sizes its network fill to the child boundary rather than to the rest of the caller's range, and reports the transaction complete. The response the page sees still declares the full Content-Length, so the caller is handed 4096 - (start mod 4096) bytes as a successful load. Slicing that short body gives zip.js a truncated deflate stream. The disk cache backend does not mis-size the fill, so an ordinary desktop profile does not reproduce it; private windows, headless contexts and embedded webviews use the in-memory backend, as does Chromium when the disk cache cannot initialise. Overlapping ranges for one URL are one trigger of that partial hit, and three read paths produced them, at 20,074 redundant bytes per load. Removing them is worth doing on its own but is not by itself a fix - concurrency depth alone reproduces the truncation. The two commits that follow add the guard that keeps overlap gone and the recovery for what remains. Bound each chunk by the following entry's offset. The central directory does not record local extra-field lengths, so the end of an entry's data could only be estimated with a fixed 100-byte guess that overshot into the next entry; the next offset is a hard upper bound and is already to hand in _buildChunks. Directories and excluded media count too - they are dropped from chunks but still occupy local headers. Route every fetch through _readRegion, which requests only the bytes the prefetched tail does not already hold, and grow the tail downwards via _ensureTail rather than re-requesting a range that runs into it. The trigger for growing it is "this read reaches the tail", not EOF proximity: the tail spans ~3% of the file, so a central-directory read can run into it while ending well short of the proximity window. Clamping to the next offset makes chunk ranges exactly adjacent, which also removes the slack the 100-byte estimate used to provide: zip.js reads a fixed 16 bytes for a data descriptor that may only be 12, so a read of a chunk's last entry can end a few bytes into the next chunk. _readAcrossChunks serves such a span from both chunks rather than throwing "No chunk covers range" or issuing an overlapping request. Reject a range response whose body is shorter than the bytes requested instead of slicing it. No caller asks for bytes past EOF, so a short body is always a fault, and the truncation is always a short body rather than a full-length one carrying wrong bytes - naming the range makes it diagnosable from a learner's console rather than an opaque inflater error. Refs learningequality#15103 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Non-overlap was a rule the read paths happened to keep rather than something the reader enforced: _buildChunks, _readRegion, _ensureTail and _readAcrossChunks each have to avoid asking for bytes another request already covers, and nothing would catch a fifth path that did not. Track the ranges in flight per URL and hold a request back until nothing overlapping it is outstanding. Deferring is all it takes to turn the rule into a mechanism, so there is no concurrency cap and no number to pick, and non-overlapping requests still run as concurrently as the browser allows. The registry is keyed by URL rather than held per reader because two readers can share a URL: navigating back to a resource builds a fresh one whose requests can overlap the previous reader's still in flight. This keeps the overlap trigger and the redundant bytes gone for any read path added later. It cannot make a truncated response impossible, for the reason in the previous commit, which is what the next one handles. Refs learningequality#15103 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard removes overlapping ranges but not truncation, since concurrency depth alone reproduces it. Rejecting a short body would turn those loads into a described error page rather than a rendered resource: measured in Chromium against a 3.6MB kpub through Learn with the guard in place, 3 of 10 navigation cycles failed that way, none of them with a partially overlapping request anywhere in the session. There is something to recover, because the failure is always a short body rather than a full-length one carrying wrong bytes, and every truncated body on record is a correct prefix of the range asked for. That makes the length check an exact detector rather than a heuristic. So ask again with Cache-Control: no-cache, which skips the cache-read-then-fill path that mis-sizes the fill. Only if that second, uncached response is also short is it an error, and the message then reports both lengths. Over 12 further cycles the browser truncated 7 responses, every one was recovered, all 12 loads rendered and the console stayed empty. The refetch sits inside the in-flight registry's window, so nothing overlapping can be issued between the short response and its retry. Fixes learningequality#15103 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
475b37a to
24663be
Compare
Summary
A kpub over the 2.5MB full-load threshold is read with HTTP range requests, and loading one twice in a browser session failed the second time with
TypeError: Compressed input was truncated.and the "unable to render this resource" page.The mechanism, confirmed by @rtibbles from a NetLog capture (comment): Chromium's in-memory cache backend splits a sparse entry into 4096-byte children. On a partial hit it serves the bytes the entry already holds, then sizes its network fill to the child boundary rather than to the rest of the caller's range, and reports the transaction complete. The caller is handed
4096 - (start % 4096)bytes as a successful 206 still declaring the fullContent-Length. Slicing that hands the inflater a truncated deflate stream.Two properties of that shape the fix. The short body is always a correct prefix — never full length with wrong bytes — so a length check is an exact detector rather than a heuristic. And concurrency depth alone reproduces it, so no request pattern the reader can choose makes it impossible.
Three changes:
Cache-Control: no-cache, which skips the cache-read-then-fill path that mis-sizes the fill. This is what recovers the load.References
Fixes #15103.
Reviewer guidance
packages/kolibri-zip/src/AdaptiveHttpReader.js:294— the in-flight registry is module level and keyed by URL, so two readers for one URL share it. Check the wait loop cannot livelock: it re-reads the set after each wait and registers synchronously once clear.packages/kolibri-zip/src/AdaptiveHttpReader.js:323—_fetchCompleteRangetreats any short body as a cache fault. That rests on no caller ever legitimately getting less than it asked for:_readRegionclamps to EOF, and a server ignoringRangereturns the whole file (both pinned by tests). Check there is no third case.packages/kolibri-zip/src/AdaptiveHttpReader.js:401—_ensureTailgrows the tail downwards with no upper bound;MAX_TAIL_SIZEcaps only the first fetch, and each growth reallocates the whole buffer. Check a badly underestimated central directory cannot walk it down far enough to matter.packages/kolibri-zip/src/AdaptiveHttpReader.js:231—_readAcrossChunksfetches the chunks a spanning read crosses rather than requesting the span. It exists for zip.js's fixed 16-byte data-descriptor read now that chunks are exactly adjacent; check it cannot silently paper over a genuine coverage gap.packages/kolibri-zip/src/AdaptiveHttpReader.js:433—_buildChunksbounds each entry bysorted[i + 1].offset, which assumes entry offsets are distinct and ascending. Two entries sharing an offset would bound the first to zero length; confirm that is acceptable to leave to zip.js's central-directory validation.Manual verification
A 3.6MB kpub (40 deflated entries) loaded repeatedly in Chromium through Learn, navigating to the library and back without reloading the page. Every range request was logged with the ranges in flight when it was sent, the ranges that started while it was open, and every range asked for in any load.
developTypeError: Compressed input was truncated.The three short bodies on the middle row had nothing overlapping in flight, nothing overlapping starting while they were open, and no partially overlapping range anywhere in the session — only byte-identical repeats from other loads, which are served intact. All were 206 declaring the full range and carrying 1469, 1437 and 2237 bytes of ~90880. On the bottom row all 7 short bodies were recovered by the refetch and every load rendered with an empty console.
Not the server: 5 × 40 concurrent range requests for the same file straight from Python return every body in full, as does
curl.An ordinary Chrome profile does not reproduce this. The disk cache backend does not mis-size the fill; the truncation needs the in-memory backend, which private windows, headless contexts and embedded webviews use, and which Chromium also falls back to when the disk cache cannot initialise. QA passing in a normal window is not evidence the bug is absent. Which backend the Android and desktop wrappers use is configured outside this repo and decides whether learners are exposed at all — not settled here.
Acceptance criteria
Every criterion below is met — none are out of scope or deferred.
The boxes in the #15103 body are still empty because ticking them is a permission block, not outstanding work: editing an issue body fails with
GraphQL: rtibblesbot does not have the correct permissions to execute UpdateIssue. They are ticked here and on the plan comment instead. A maintainer needs to tick the issue body._buildChunksclamps chunk-vs-chunk;_readRegion/_ensureTailcover chunk-vs-tail and tail-vs-tail;_readAcrossChunksserves a spanning read from two adjacent chunks instead of a fresh overlapping request; and_rangeRequestdefers anything that would still overlap a request in flight. Asserted byfindRangeOverlaps(test/rangeOverlaps.js), end-to-end over a real fflate zip, and by a test that the guard serialises an overlapping pair.offsetrather than by a guess at the local extra-field length. — every entry with an offset is sorted into the bounding pass, directories and excluded media included, and each chunk end clamped to the next offset.ZIP_EXTRA_FIELD_ESTIMATEnow bounds only the final entry.Truncated range response for bytes=…: expected N bytes, received M, then P with the cache bypassedif that is short too. For 200 and 206 alike. See Deviations.packages/kolibri-zip/test/assert that_buildChunksproduces non-overlapping ranges for entry fixtures whose local extra fields are shorter thanZIP_EXTRA_FIELD_ESTIMATE. — four tests indescribe('Chunked fetching - _buildChunks()').xhr-mockis already a devDependency). — indescribe('Error handling - range requests'): a short body refetches withCache-Control: no-cache, a still-short refetch rejects, plus two tests pinning what must not trip the guard — a chunk range clamped at EOF, and a whole-file 200 from a server that ignoresRange.kpubresource over 2.5 MB renders on two consecutive loads within one browser session, verified manually. — the table above.Deviations from the issue spec
_rangeRequestkeeps a per-URL registry of in-flight ranges and defers a request that overlaps one. Deferral, not a cap — non-overlapping requests still run as concurrently as the browser allows.AI usage
Used Claude Code to implement the change from a pre-agreed plan, and to run the browser QA that the plan's premise, the refetch and the reviewer's request for its removal were each tested against. The root-cause mechanism above is the reviewer's, from a NetLog capture and a backend matrix, not this agent's. Verified with the kolibri-zip test suite, prek, and manual QA in Chromium against a local devserver.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
🟡 Waiting for feedback
Last updated: 2026-07-31 03:16 UTC