diff --git a/packages/kolibri-zip/README.md b/packages/kolibri-zip/README.md index 396444973cb..6c1d09b93c3 100644 --- a/packages/kolibri-zip/README.md +++ b/packages/kolibri-zip/README.md @@ -54,6 +54,10 @@ Chunks: [=====chunk1=====] [===chunk2===] This reduces the number of HTTP requests when extracting multiple files. Concurrent reads from the same chunk are deduplicated into a single fetch. +Range requests for one URL never overlap: chunks end at the following entry's offset, a read spilling past a chunk is served from both chunks, and one reaching the prefetched tail grows the tail downwards. A request that would still overlap one in flight waits for it rather than issuing. + +That does not make a short response impossible: Chromium's in-memory cache serves a partial hit as fewer bytes than the `Content-Length` it declares, sized to a 4096-byte sparse-entry boundary (#15103). The body is always a correct prefix, so a short one is refetched with `Cache-Control: no-cache`, and only becomes an error — a described one, rather than an opaque inflater `TypeError` — if that is short too. + ### Large Media File Handling For large video/audio files, provide a `largeFileUrlGenerator` to serve them directly: diff --git a/packages/kolibri-zip/src/AdaptiveHttpReader.js b/packages/kolibri-zip/src/AdaptiveHttpReader.js index d2cfeea3487..c9c957d1bd4 100644 --- a/packages/kolibri-zip/src/AdaptiveHttpReader.js +++ b/packages/kolibri-zip/src/AdaptiveHttpReader.js @@ -12,7 +12,8 @@ const DEFAULT_CHUNK_SIZE = 500 * 1024; // 500KB const ZIP_LOCAL_HEADER_SIZE = 30; // Conservative estimate for extra field length (varies, typically 0-100 bytes). // ZIP64 extensions alone add 28 bytes, and tools may write timestamps, Unicode paths, etc. -// Overestimating wastes negligible bandwidth; underestimating risks missing chunk coverage. +// Overestimating wastes negligible bandwidth; underestimating risks missing chunk +// coverage. Only the last entry relies on it; the rest are clamped to the next offset. const ZIP_EXTRA_FIELD_ESTIMATE = 100; // Tail prefetch heuristics for Central Directory estimation @@ -25,6 +26,11 @@ const CD_SIZE_RATIO = 0.03; // Estimate CD is ~3% of total file size // zip.js reads near the end of file looking for EOCD signature. const EOCD_PROXIMITY_THRESHOLD = 100; +// Range requests in flight, keyed by URL, so an overlapping pair is never issued together +// (#15103). Shared across readers: one built on navigation can outlive the requests of the +// one it replaced. +const inFlightRanges = new Map(); + /** * AdaptiveHttpReader extends zip.js's Reader to provide adaptive fast/lazy loading for zip files. * @@ -70,6 +76,7 @@ export default class AdaptiveHttpReader extends Reader { this._fullData = null; this._chunks = null; this._tailChunk = null; + this._tailPromise = null; this.size = 0; } @@ -199,39 +206,56 @@ export default class AdaptiveHttpReader extends Reader { return this._readFromChunk(chunk, index, length); } - // Prefetch tail on first read near end of file (EOCD triggers this) - if (!this._tailChunk && index + length >= this.size - EOCD_PROXIMITY_THRESHOLD) { - // Use promise deduplication to prevent concurrent tail fetches - if (!this._tailFetching) { - const tailSize = this._estimateTailSize(); - const tailStart = Math.max(0, this.size - tailSize); - this._tailFetching = this._rangeRequest(tailStart, this.size - tailStart).then(tailData => { - this._tailChunk = { startOffset: tailStart, endOffset: this.size, data: tailData }; - this._tailFetching = null; - return this._tailChunk; - }); - } - const tailChunk = await this._tailFetching; - // The tail chunk always extends to EOF (endOffset === this.size), so reads - // can only miss if they start before the cached region. This happens when - // our CD size estimate was too small and zip.js needs earlier bytes. - if (index < tailChunk.startOffset) { - return this._rangeRequest(index, length); - } - const relativeStart = index - tailChunk.startOffset; - return tailChunk.data.slice(relativeStart, relativeStart + length); - } - // Before configureChunks() is called, allow range requests for zip parsing if (!this.chunksConfigured) { - return this._rangeRequest(index, length); + // Before a tail exists, EOF proximity seeds one; after, any read reaching it grows it down. + const tailReach = this._tailChunk?.startOffset ?? this.size - EOCD_PROXIMITY_THRESHOLD; + if (index + length >= tailReach) { + await this._ensureTail(index); + } + return this._readRegion(index, index + length); } // After chunks are configured, all reads should be covered by chunks - throw new Error( - `No chunk covers range ${index}-${index + length}. ` + - `This may indicate a large file being extracted without a largeFileUrlGenerator.`, + return this._readAcrossChunks(index, length); + } + + /** + * Read a span no single chunk covers from the chunks it crosses. Chunk ends are clamped + * to the next entry's offset, so zip.js's fixed 16-byte data-descriptor read can cross a + * boundary; requesting the span itself would overlap those chunks. + * @param {number} index - Start offset + * @param {number} length - Number of bytes to read + * @returns {Promise} The requested data + */ + async _readAcrossChunks(index, length) { + const end = index + length; + // Walk the span first, so an uncovered span throws before anything is fetched. + const spanning = []; + let offset = index; + while (offset < end) { + const chunk = this._findChunk(offset); + if (!chunk) { + throw new Error( + `No chunk covers range ${index}-${end}. ` + + `This may indicate a large file being extracted without a largeFileUrlGenerator.`, + ); + } + spanning.push(chunk); + offset = chunk.endOffset; + } + + const result = new Uint8Array(length); + await Promise.all( + spanning.map(chunk => { + const partStart = Math.max(chunk.startOffset, index); + const partLength = Math.min(chunk.endOffset, end) - partStart; + return this._readFromChunk(chunk, partStart, partLength).then(part => + result.set(part, partStart - index), + ); + }), ); + return result; } /** @@ -261,17 +285,74 @@ export default class AdaptiveHttpReader extends Reader { } /** - * Make a range request for specific bytes. + * Make a range request for specific bytes, held back until no request for this URL has + * an overlapping range in flight. * @param {number} start - Start offset * @param {number} length - Number of bytes to read * @returns {Promise} The requested data */ - _rangeRequest(start, length) { + async _rangeRequest(start, length) { + const end = start + length; + const inFlight = inFlightRanges.get(this.url) ?? new Set(); + inFlightRanges.set(this.url, inFlight); + + const overlapping = () => [...inFlight].filter(r => r.start < end && start < r.end); + // Re-checked after each wait: another deferred request may have started meanwhile. + for (let waiting = overlapping(); waiting.length; waiting = overlapping()) { + await Promise.allSettled(waiting.map(r => r.response)); + } + + const request = { start, end, response: this._fetchCompleteRange(start, length) }; + inFlight.add(request); + try { + return await request.response; + } finally { + inFlight.delete(request); + if (!inFlight.size) { + inFlightRanges.delete(this.url); + } + } + } + + /** + * Fetch a range, re-fetching past the browser cache if the response is short. + * @param {number} start - Start offset + * @param {number} length - Number of bytes to read + * @returns {Promise} The requested data + */ + async _fetchCompleteRange(start, length) { + const body = await this._sendRangeXhr(start, length); + if (body.byteLength >= length) { + return body; + } + // No caller asks past EOF, so a short body is a partial cache hit (#15103). + const refetched = await this._sendRangeXhr(start, length, true); + if (refetched.byteLength < length) { + throw new Error( + `Truncated range response for bytes=${start}-${start + length - 1}: ` + + `expected ${length} bytes, received ${body.byteLength}, ` + + `then ${refetched.byteLength} with the cache bypassed`, + ); + } + return refetched; + } + + /** + * Issue the range request itself, resolving whatever body arrives. + * @param {number} start - Start offset + * @param {number} length - Number of bytes to read + * @param {boolean} [bypassCache] - Force a network fetch rather than a cached response + * @returns {Promise} The response body, of any length + */ + _sendRangeXhr(start, length, bypassCache = false) { return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); xhr.open('GET', this.url); xhr.responseType = 'arraybuffer'; xhr.setRequestHeader('Range', `bytes=${start}-${start + length - 1}`); + if (bypassCache) { + xhr.setRequestHeader('Cache-Control', 'no-cache'); + } xhr.addEventListener('load', () => { if (xhr.status === 206 || xhr.status === 200) { @@ -286,6 +367,54 @@ export default class AdaptiveHttpReader extends Reader { }); } + /** + * Read [start, end), requesting only the bytes the prefetched tail does not already + * hold so the request cannot overlap the tail's own. + * @param {number} start - Start offset + * @param {number} end - End offset, exclusive + * @returns {Promise} The requested bytes + */ + async _readRegion(start, end) { + // Clamp to EOF: the last chunk's end is an estimate, and a short body must stay a fault. + const stop = Math.min(end, this.size); + const tail = this._tailChunk; + if (!tail || stop <= tail.startOffset) { + return this._rangeRequest(start, stop - start); + } + if (start >= tail.startOffset) { + return tail.data.slice(start - tail.startOffset, stop - tail.startOffset); + } + const head = await this._rangeRequest(start, tail.startOffset - start); + const cached = tail.data.subarray(0, stop - tail.startOffset); + const region = new Uint8Array(head.length + cached.length); + region.set(head); + region.set(cached, head.length); + return region; + } + + /** + * Seed or grow the tail chunk downwards to cover minStart, so a read below an + * underestimated central directory never re-requests the tail's bytes. + * @param {number} minStart - Lowest offset that must be covered + * @returns {Promise} Resolves once _tailChunk covers [<= minStart, size) + */ + _ensureTail(minStart) { + const startOffset = Math.min(minStart, Math.max(0, this.size - this._estimateTailSize())); + // Chained, so concurrent callers each fetch only the bytes the tail still lacks. + this._tailPromise = Promise.resolve(this._tailPromise).then(async () => { + if (this._tailChunk && startOffset >= this._tailChunk.startOffset) { + return; + } + // _readRegion splices in whatever the current tail already holds. + this._tailChunk = { + startOffset, + endOffset: this.size, + data: await this._readRegion(startOffset, this.size), + }; + }); + return this._tailPromise; + } + /** * Build chunk boundaries from entry metadata. * Groups adjacent small files into chunks of approximately chunkSize. @@ -294,17 +423,19 @@ export default class AdaptiveHttpReader extends Reader { * @returns {Array} Array of chunk objects with startOffset, endOffset, data, fetching */ _buildChunks(entries) { - // Filter out directories and entries without offset, sort by offset - const fileEntries = entries - .filter(e => !e.directory && e.offset !== undefined) - .sort((a, b) => a.offset - b.offset); - - if (fileEntries.length === 0) return []; + const sorted = entries.filter(e => e.offset !== undefined).sort((a, b) => a.offset - b.offset); const chunks = []; let currentChunk = null; - for (const entry of fileEntries) { + for (let i = 0; i < sorted.length; i++) { + const entry = sorted[i]; + // Skipped rather than filtered out: a directory holds no data, but its local header + // still bounds the entry before it. + if (entry.directory) { + continue; + } + // Only exclude files that will be served via URL generation (large audio/video). // Large non-media files (e.g. JS libraries, images) must remain in chunks // so they can be extracted in the frontend. @@ -319,12 +450,15 @@ export default class AdaptiveHttpReader extends Reader { } const entryStart = entry.offset; - const entryEnd = + // The next entry's offset is a hard upper bound on where this entry's data can end. + const entryEnd = Math.min( entry.offset + - ZIP_LOCAL_HEADER_SIZE + - entry.filename.length + - ZIP_EXTRA_FIELD_ESTIMATE + - entry.compressedSize; + ZIP_LOCAL_HEADER_SIZE + + entry.filename.length + + ZIP_EXTRA_FIELD_ESTIMATE + + entry.compressedSize, + sorted[i + 1]?.offset ?? Infinity, + ); if (!currentChunk) { // Start a new chunk with this entry @@ -403,8 +537,7 @@ export default class AdaptiveHttpReader extends Reader { // Ensure chunk data is loaded (with promise deduplication for concurrent reads) if (!chunk.data) { if (!chunk.fetching) { - const chunkLength = chunk.endOffset - chunk.startOffset; - chunk.fetching = this._rangeRequest(chunk.startOffset, chunkLength).then(data => { + chunk.fetching = this._readRegion(chunk.startOffset, chunk.endOffset).then(data => { chunk.data = data; chunk.fetching = null; }); diff --git a/packages/kolibri-zip/test/AdaptiveHttpReader.spec.js b/packages/kolibri-zip/test/AdaptiveHttpReader.spec.js index ef724788b6f..fef32de9532 100644 --- a/packages/kolibri-zip/test/AdaptiveHttpReader.spec.js +++ b/packages/kolibri-zip/test/AdaptiveHttpReader.spec.js @@ -4,6 +4,7 @@ import 'web-streams-polyfill/polyfill'; import xhrMock from 'xhr-mock'; import AdaptiveHttpReader from '../src/AdaptiveHttpReader'; +import { findRangeOverlaps } from './rangeOverlaps'; // Ensure URL APIs exist for jest.spyOn (jsdom doesn't provide them) if (!global.URL.createObjectURL) { @@ -34,7 +35,11 @@ function createTestData(sizeBytes) { // Mock server that tracks requests function createMockServer(data, url, options = {}) { - const { supportsRanges = true } = options; + const { + supportsRanges = true, + // Given the honest slice, returns the body to send - short, delayed, or both. + rangeResponse = null, + } = options; const stats = { requestCount: 0, @@ -44,13 +49,15 @@ function createMockServer(data, url, options = {}) { }; xhrMock.reset(); - xhrMock.get(url, (req, res) => { + xhrMock.get(url, async (req, res) => { stats.requestCount++; const rangeHeader = req.header('Range'); + const bypassedCache = req.header('Cache-Control') === 'no-cache'; stats.requests.push({ type: rangeHeader ? 'range' : 'full', rangeHeader, + bypassedCache, }); if (!rangeHeader) { @@ -79,14 +86,19 @@ function createMockServer(data, url, options = {}) { } const slicedData = data.slice(start, actualEnd + 1); - stats.totalBytesSent += slicedData.length; + // Content-Length below still declares the whole slice, so a shorter body is served + // exactly as the browser cache served it in #15103. + const body = rangeResponse + ? await rangeResponse({ body: slicedData, bypassedCache }) + : slicedData; + stats.totalBytesSent += body.length; return res .status(206) .header('Content-Range', `bytes ${start}-${actualEnd}/${data.length}`) .header('Content-Length', slicedData.length.toString()) .header('Accept-Ranges', 'bytes') - .body(slicedData.buffer); + .body(body.buffer); }); return stats; @@ -189,6 +201,24 @@ function createEntries(...specs) { ); } +/** + * Range headers recorded after the given baseline. + * @param {object} stats - Mock server stats + * @param {number} baseline - Number of range requests to skip + * @returns {Array} Range headers issued since the baseline + */ +function rangesSince(stats, baseline) { + return stats.requests + .filter(r => r.type === 'range') + .slice(baseline) + .map(r => r.rangeHeader); +} + +// Range requests recorded so far. +function rangeCount(stats) { + return rangesSince(stats, 0).length; +} + // ============================================================================= // Assertion Helpers // ============================================================================= @@ -270,6 +300,14 @@ const expectations = { expect(stats.requestCount).toBe(baseline); }, + /** + * Assert no two range requests asked for overlapping bytes (#15103). + * @param {object} stats - Mock server stats + */ + noOverlappingRanges: stats => { + expect(findRangeOverlaps(rangesSince(stats, 0))).toEqual([]); + }, + /** * Verify data correctness using createTestData's i % 256 pattern. * @param {Uint8Array} chunk - Chunk to inspect @@ -699,6 +737,67 @@ describe('AdaptiveHttpReader', () => { await expect(reader.readUint8Array(1000, 500)).rejects.toThrow('Network error'); console.error.mockRestore(); // eslint-disable-line no-console }); + + it('refetches a short range response with the cache bypassed', async () => { + const { reader, stats } = await setupReader( + { size: 5 * 1024 * 1024, url: 'cache-truncated-range.zip' }, + { + serverOptions: { + rangeResponse: ({ body, bypassedCache }) => (bypassedCache ? body : body.slice(0, 120)), + }, + }, + ); + + const result = await reader.readUint8Array(1000, 500); + + expect(result.length).toBe(500); + expectations.dataMatches(result, 1000); + const ranges = stats.requests.filter(r => r.type === 'range'); + expect(ranges.map(r => [r.rangeHeader, r.bypassedCache])).toEqual([ + ['bytes=1000-1499', false], + ['bytes=1000-1499', true], + ]); + }); + + it('rejects a range response still short once the cache is bypassed', async () => { + const { reader, stats } = await setupReader( + { size: 5 * 1024 * 1024, url: 'truncated-range.zip' }, + { serverOptions: { rangeResponse: ({ body }) => body.slice(0, 120) } }, + ); + + await expect(reader.readUint8Array(1000, 500)).rejects.toThrow( + 'Truncated range response for bytes=1000-1499: expected 500 bytes, received 120, ' + + 'then 120 with the cache bypassed', + ); + // One refetch, not a retry loop. + expectations.rangeRequestCount(stats, 2); + }); + + it('accepts a chunk range that extends past the end of the file', async () => { + const { reader, stats } = await setupReader( + { size: 100000, url: 'chunk-past-eof.zip' }, + { maxFullLoadSize: 1000 }, + ); + // Chunk ends are padded estimates, so the last chunk routinely runs past EOF. + reader._chunks = [{ startOffset: 99500, endOffset: 100500, data: null, fetching: null }]; + + const result = await reader.readUint8Array(99600, 100); + + expect(result.length).toBe(100); + expectations.dataMatches(result, 99600); + expect(rangesSince(stats, 0)).toEqual(['bytes=99500-99999']); + }); + + it('accepts a whole-file 200 from a server that ignores Range', async () => { + const { reader } = await setupReader( + { size: 5 * 1024 * 1024, url: 'no-range-support.zip' }, + { maxFullLoadSize: 1000, serverOptions: { supportsRanges: false } }, + ); + + // A longer-than-range body is not truncation. The mis-slice of a whole-file response + // is pre-existing, so only non-rejection is asserted. + await expect(reader.readUint8Array(1000, 500)).resolves.toBeInstanceOf(Uint8Array); + }); }); describe('Range request data correctness', () => { @@ -1220,6 +1319,67 @@ describe('AdaptiveHttpReader', () => { expect(chunks[1].startOffset).toBe(6630); expect(chunks[2].startOffset).toBe(57080); }); + + it('bounds an entry by the following entry offset when the local extra field is shorter than the estimate', () => { + // chunkSize: 1 forces each entry into its own chunk, so the boundary is observable + const reader = createReader('test.zip', { chunkSize: 1 }); + const entries = createEntries(['a.txt', 0, 100], ['b.txt', 145, 100]); + + const chunks = reader._buildChunks(entries); + + // Unclamped, a.txt would end at 0 + 30 + 5 + 100 + 100 = 235, past b.txt's offset + expect(chunks[0].endOffset).toBe(145); + expect(chunks[1].startOffset).toBe(145); + }); + + it('produces non-overlapping chunk ranges for tightly packed entries', () => { + const reader = createReader('test.zip', { chunkSize: 200 }); + const entries = createEntries( + ...Array.from({ length: 10 }, (_, i) => [`f${i}.txt`, i * 150, 100]), + ); + + const chunks = reader._buildChunks(entries); + + expect(chunks.length).toBeGreaterThan(1); + for (let i = 1; i < chunks.length; i++) { + expect(chunks[i].startOffset).toBeGreaterThanOrEqual(chunks[i - 1].endOffset); + } + }); + + it('bounds an entry by a following directory or excluded media entry', () => { + const withDirectory = createReader('test.zip', { chunkSize: 1 }); + const directoryChunks = withDirectory._buildChunks( + createEntries( + ['a.txt', 0, 100], + ['sub/', 145, 0, { directory: true }], + ['b.txt', 400, 100], + ), + ); + + // The directory is dropped from chunks but still occupies a local header at 145 + expect(directoryChunks[0].endOffset).toBe(145); + + const withMedia = createReader('test.zip', { chunkSize: 1, largeMediaThreshold: 500 }); + const mediaChunks = withMedia._buildChunks( + createEntries( + ['a.txt', 0, 100], + ['v.mp4', 145, 600 * 1024, { uncompressedSize: 600 * 1024 }], + ), + ); + + expect(mediaChunks[0].endOffset).toBe(145); + }); + + it('keeps the extra-field estimate for the last entry in the archive', () => { + const reader = createReader('test.zip', { chunkSize: 1 }); + + const chunks = reader._buildChunks(createEntries(['a.txt', 0, 100])); + + // Nothing follows a.txt, so the estimate is the only available bound + expect(chunks[0].endOffset).toBe( + ZIP_LOCAL_HEADER_SIZE + 'a.txt'.length + ZIP_EXTRA_FIELD_ESTIMATE + 100, + ); + }); }); describe('Chunked fetching - _findChunk()', () => { @@ -1584,6 +1744,24 @@ describe('AdaptiveHttpReader', () => { await expect(reader.readUint8Array(200, 200)).rejects.toThrow('No chunk covers range'); }); + it('serves a read that spans a chunk boundary from both chunks', async () => { + const { reader, stats } = await setupReader( + { size: 100000, url: 'read-across-chunks.zip' }, + { maxFullLoadSize: 1000, chunkSize: 1, largeMediaThreshold: 50000 }, + ); + + // Clamping a.txt's end to b.txt's offset makes the chunks adjacent at 150, so a + // 16-byte data-descriptor read at 140 crosses the boundary + reader.configureChunks(createEntries(['a.txt', 0, 100], ['b.txt', 150, 100])); + + const result = await reader.readUint8Array(140, 16); + + expect(result.length).toBe(16); + expectations.dataMatches(result, 140); + expect(rangesSince(stats, 0)).toEqual(['bytes=0-149', 'bytes=150-384']); + expectations.noOverlappingRanges(stats); + }); + it('in lazy mode reading large file data throws error after chunks configured', async () => { const { reader } = await setupReader( { size: 100000, url: 'read-large-file.zip' }, @@ -1665,6 +1843,117 @@ describe('AdaptiveHttpReader', () => { }); }); + describe('Non-overlapping range requests', () => { + // For a 100000 byte file the tail estimate is floor(100000 * 0.03) = 3000, + // so the prefetched tail covers [97000, 100000). + const TAIL_START = 97000; + + // A lazy-mode reader whose prefetched tail is already populated. + async function setupReaderWithTail(url) { + const { reader, stats } = await setupReader({ size: 100000, url }, { maxFullLoadSize: 1000 }); + await reader.readUint8Array(99950, 50); + expect(reader._tailChunk.startOffset).toBe(TAIL_START); + return { reader, stats }; + } + + it('a chunk that extends past the tail start only requests the bytes below it', async () => { + const { reader, stats } = await setupReaderWithTail('chunk-past-tail.zip'); + const before = rangeCount(stats); + + reader._chunks = [{ startOffset: 96000, endOffset: 98000, data: null, fetching: null }]; + const result = await reader.readUint8Array(96500, 1000); + + expect(rangesSince(stats, before)).toEqual(['bytes=96000-96999']); + expect(result.length).toBe(1000); + expectations.dataMatches(result, 96500); + expectations.noOverlappingRanges(stats); + }); + + it('grows the tail downwards, fetching only the bytes it does not already hold', async () => { + const { reader, stats } = await setupReaderWithTail('extend-tail.zip'); + const before = rangeCount(stats); + + // Ends exactly at the tail start: the boundary of "reaches the tail", and far outside + // the EOCD proximity window that used to be the trigger + const first = await reader.readUint8Array(96000, 1000); + // Wholly inside the grown tail + const cached = await reader.readUint8Array(96500, 3500); + // Below it again, so the tail grows a second time + const lower = await reader.readUint8Array(95000, 3000); + + expect(rangesSince(stats, before)).toEqual(['bytes=96000-96999', 'bytes=95000-95999']); + expect(reader._tailChunk.startOffset).toBe(95000); + expect(first.length).toBe(1000); + expectations.dataMatches(first, 96000); + expect(cached.length).toBe(3500); + expectations.dataMatches(cached, 96500); + expect(lower.length).toBe(3000); + expectations.dataMatches(lower, 95000); + expectations.noOverlappingRanges(stats); + }); + + it('concurrent near-EOF reads below the tail issue one request per byte range', async () => { + const { reader, stats } = await setupReader( + { size: 100000, url: 'concurrent-tail.zip' }, + { maxFullLoadSize: 1000 }, + ); + + const [first, second] = await Promise.all([ + reader.readUint8Array(96000, 4000), + reader.readUint8Array(95000, 5000), + ]); + + // The tail seeds low enough for the first read, then grows down by only the bytes the + // second one lacks. + expect(rangesSince(stats, 0)).toEqual(['bytes=96000-99999', 'bytes=95000-95999']); + expect(first.length).toBe(4000); + expectations.dataMatches(first, 96000); + expect(second.length).toBe(5000); + expectations.dataMatches(second, 95000); + expectations.noOverlappingRanges(stats); + }); + + // The mechanism behind the invariant: no read path above produces an overlapping pair, + // and if one ever did, the second request waits rather than issuing. + it('defers a range request overlapping one in flight until that one completes', async () => { + let releaseFirst; + const firstHeld = new Promise(resolve => (releaseFirst = resolve)); + let heldOne = false; + const { reader, stats } = await setupReader( + { size: 100000, url: 'serialised.zip' }, + { + maxFullLoadSize: 1000, + serverOptions: { + rangeResponse: async ({ body }) => { + if (!heldOne) { + heldOne = true; + await firstHeld; + } + return body; + }, + }, + }, + ); + + const first = reader._rangeRequest(1000, 500); + const second = reader._rangeRequest(1400, 500); + while (!heldOne) { + await new Promise(resolve => setTimeout(resolve, 0)); + } + await new Promise(resolve => setTimeout(resolve, 5)); + + // The two ranges share bytes 1400-1499, so the second is still held back. + expect(rangesSince(stats, 0)).toEqual(['bytes=1000-1499']); + + releaseFirst(); + const [firstBytes, secondBytes] = await Promise.all([first, second]); + + expect(rangesSince(stats, 0)).toEqual(['bytes=1000-1499', 'bytes=1400-1899']); + expectations.dataMatches(firstBytes, 1000); + expectations.dataMatches(secondBytes, 1400); + }); + }); + describe('shouldLoadFromUrl() edge cases', () => { // Parameterized edge case tests const edgeCaseTests = [ diff --git a/packages/kolibri-zip/test/index.spec.js b/packages/kolibri-zip/test/index.spec.js index e10585a495c..f53b456c262 100644 --- a/packages/kolibri-zip/test/index.spec.js +++ b/packages/kolibri-zip/test/index.spec.js @@ -8,6 +8,7 @@ import xhrMock from 'xhr-mock'; import { zipSync, strToU8 } from 'fflate'; import ZipFile from '../src/index'; import { Mapper } from '../src/fileUtils'; +import { findRangeOverlaps } from './rangeOverlaps'; // Override global with Node's implementation (jsdom's version doesn't work correctly) global.TextEncoder = TextEncoder; @@ -963,6 +964,29 @@ describe('ZipFile lazy loading', () => { }); describe('Chunked fetching integration', () => { + it('issues no overlapping range requests while extracting every file', async () => { + // zip.js forces its own 64KB chunkSize, so >64KB of entry data is what gets a second + // chunk - and chunk boundaries are where the ranges used to overlap. + const zipContents = {}; + for (let i = 0; i < 40; i++) { + zipContents[`content/file${i}.json`] = strToU8( + `{"index": ${i}, "pad": "${'x'.repeat(5000)}"}`, + ); + } + const zipData = zipSync(zipContents, { level: 0 }); + setupTrackingMockServer(zipData, 'overlap.zip'); + + const zip = new ZipFile('overlap.zip', { maxFullLoadSize: 1000 }); + for (let i = 0; i < 40; i++) { + await zip.file(`content/file${i}.json`); + } + + const ranges = requestLog.filter(r => r.range).map(r => r.range); + // More than tail + one chunk, so a chunk boundary was crossed and this is not vacuous. + expect(ranges.length).toBeGreaterThan(2); + expect(findRangeOverlaps(ranges)).toEqual([]); + }); + it('extracting many small files sequentially uses chunked fetching - reduces request count', async () => { // Create a zip with many small files (simulating H5P) const zipContents = {}; diff --git a/packages/kolibri-zip/test/rangeOverlaps.js b/packages/kolibri-zip/test/rangeOverlaps.js new file mode 100644 index 00000000000..186cf686cf0 --- /dev/null +++ b/packages/kolibri-zip/test/rangeOverlaps.js @@ -0,0 +1,15 @@ +/** + * Find every pair of range headers that asked for a shared byte (#15103). + * @param {Array} headers - `bytes=-` headers, in request order + * @returns {Array} One description per overlapping pair + */ +export function findRangeOverlaps(headers) { + // Range headers are inclusive at both ends, so a shared byte means start <= end. + const ranges = headers.map(header => header.replace('bytes=', '').split('-').map(Number)); + return ranges.flatMap(([s1, e1], i) => + ranges + .slice(i + 1) + .filter(([s2, e2]) => s1 <= e2 && s2 <= e1) + .map(([s2, e2]) => `bytes=${s1}-${e1} overlaps bytes=${s2}-${e2}`), + ); +}