[aivus]

Fixes #386

Reworks the scraper around streams so resource content is no longer stored in memory. Memory usage is now bounded by requestConcurrency instead of total website size.

Benchmark

npm run benchmark:memory (new script) — local server, 100 files × 20 MB (2 GB total), concurrency 8:

	v6.0.0	this PR
peak RSS	2274 MB	160 MB
wall time	2.6 s	1.6 s

How it works

• lib/request.js uses got.stream and resolves at response-headers time with {url, statusCode, mimeType, encoding, metadata, stream, readBody}. A retry-event wrapper keeps got's transient-error retries working for streams. Up to headers time every resource is retried; after that only buffered resources (html/css, read via readBody()) are retried on mid-body errors — a resource which is already being piped to storage can't be handed a replacement stream, so a connection dying mid-body fails that resource.
• lib/scraper.js: resources which need no modification (images, fonts, media, js — the bulk of bytes) are piped straight to storage at response time, while still holding the request-queue slot so requestConcurrency keeps bounding concurrent transfers. HTML/CSS still need full buffering for link rewriting (cheerio re-serializes the whole document), but their text is freed immediately after save.
• This answers the open question in the issue about references to not-yet-downloaded pages: getReference only needs the child's filename, and filenames are derivable from response headers (mime → type → generateFilename) before the body is consumed.
• Redirect dedup resolves to the already-requested resource outside the queue task (awaiting inside would deadlock at requestConcurrency: 1; covered by a regression test).
• Error semantics for streamed resources match v6 request errors: a child failing mid-body with a network error (got RequestError) fires onResourceError and does not abort the scrape (same as a child failing before headers), while a storage/plugin failure or a root resource failing aborts it when ignoreErrors is false. The save also fails when the live stream errors even if the saveResource action never settles (e.g. .pipe() without an error handler), so a dropped connection can't hang the scrape.
• The FS plugin pipes resource.getContentStream() to disk and removes partially written files on mid-stream failure, so ignoreErrors: true never leaves partial garbage. Its error handler removes the output directory only if the plugin itself wrote to it, so files placed there by other plugins survive a failed scrape.
• getContentStream() hands a live stream out only once — a second call throws instead of returning an ended stream.
• For multiple saveResource actions on a streamed resource, content is buffered once (as a Buffer) so every storage can read it (a tee with in-series actions would deadlock on backpressure). Still strictly better than v6, which always buffered everything.

Breaking changes (v7, documented in MIGRATION.md)

• saveResource actions: resource.getText() → resource.getContentStream() (single-use for streamed resources; await buffer(...) from node:stream/consumers for whole-content storages)
• saveResource actions are no longer called one resource at a time: streamed resources are saved as their responses arrive, concurrently (bounded by requestConcurrency). Actions writing to a non-concurrency-safe sink need to serialize themselves or use requestConcurrency: 1
• afterResponse receives {response: {url, statusCode, headers, getBody()}} instead of the full got response, and is called at headers time. getBody() buffers the body and is only callable while the action is running. String returns removed; returning an object without body keeps the resource streaming (zero buffering); {body: null} skips the resource like v6. A string body without encoding is now decoded with the encoding derived from content-type (utf8/binary) instead of always binary
• scrape() result no longer carries content — getText() is null after save (also in onResourceSaved)
• generateFilename's responseData no longer contains body
• Binary files are written byte-for-byte from the network (previously round-tripped through a latin1 string) — bug-fix-class difference
• responseType in the request option is ignored

website-scraper-puppeteer / website-scraper-phantom will need mechanical updates for the new afterResponse contract (response.body → await response.getBody()).

Testing

• 281 unit + functional tests passing (npm test), npm run eslint clean
• New test/functional/streaming suite: byte-fidelity of multi-MB binaries, content freed after save, both afterResponse paths, multiple saveResource storages, unconsumed streams, mid-stream failures (via a real local HTTP server — nock can't simulate a connection dying mid-body) for child/root resources with and without ignoreErrors, a saveResource action which swallows stream errors, mid-body retries (html retried, streamed resource not), stream cleanup when generateFilename throws, redirect-to-already-requested-URL at requestConcurrency: 1
• New test/unit/plugins.test.js for SaveResourceToFileSystemPlugin (partial-file cleanup, cleanup errors not masking the save error, output directory removed only when the plugin wrote to it)
• E2E (npm run test-e2e): 20+ real sites pass; the only failures are antipenko.pp.ua, which is currently stuck in a server-side 301 self-redirect loop (fails on master too)

🤖 Generated with Claude Code

[aivus]

High-effort automated review of the v7 streaming rework (diff master...issue-386-streams). Every finding below was independently re-verified against the code before posting (17/17 candidates confirmed, 0 refuted; deduplicated to 13 distinct issues).

Severity overview, most severe first:

Correctness — streaming lifecycle

1. Save failures of streamed child resources are silently swallowed (scraper.js:228)
2. Live got stream leaks when generateFilename throws (scraper.js:205) — reproduced live
3. Multiple saveResource actions re-buffer the whole body as a string, throwing at ~512 MiB (scraper.js:135)
4. getBody memoization race corrupts content under concurrent calls (request.js:66)

Correctness — undocumented v6→v7 behavior changes
5. afterResponse {body: null} now throws instead of skipping (request.js:109)
6. Default string-body encoding changed from binary to header-derived (request.js:98)
7. saveResource actions no longer run serially (scraper.js:141)

Correctness — FS plugin cleanup
8. Mid-stream failure leaves the output directory behind, breaking retries (save-resource-to-fs-plugin.js:24)
9. Cleanup rm can mask the original save error (save-resource-to-fs-plugin.js:26)

Cleanup
10. readAll duplicates stream/consumers buffer() (stream.js:1)
11. Five copy-pasted originalStream.destroy() branches (request.js:62)
12. Save failures logged as "failed to request resource" (scraper.js:238)
13. outputFile is dead code (fs.js:6)

🤖 Generated with Claude Code

[aivus]

Code review of the streaming rework against master. 7 findings posted inline, ranked by severity: 3 behaviour regressions vs master (fatal-error inconsistency for children, swallowed stream errors, lost mid-body retries for html/css), 3 plugin-facing edge cases, 1 cleanup.

🤖 Generated with Claude Code

[aivus]

Inconsistent fatal-error handling for child resources (regression vs master).

On master the body was buffered in the request phase, so a mid-body network failure on any child was a request error: handleError fired onResourceError, the rejection was discarded by Promise.allSettled in downloadChildrenResources, and the scrape completed. Only save-phase errors (fs/plugin) were fatal.

Moving body consumption for binaries into saveResource turns the same network failure into a save error, and fatalError then aborts the scrape. With default options (ignoreErrors: false), a page with 50 images where one connection is reset mid-body now fails the whole scrape and the error action deletes the output directory. Meanwhile the identical failure on an html/css child (readStreamToBuffer at line 224), or any 404/ECONNREFUSED child, is still swallowed by allSettled and the scrape completes.

The underlying issue is downloadChildrenResources ignoring rejections. Deciding once there whether child errors are fatal with ignoreErrors: false (propagate the first rejection, or consistently ignore) would remove the need for the fatalError side channel.

[aivus]

Fixed in cc1efcd: a got RequestError thrown while streaming a child resource is now treated as a request error (logged, onResourceError fired, scrape continues), matching master's behaviour for children failing before the body. Storage/plugin failures and root resources failing mid-body still abort via fatalError. Added a functional test for a child dying mid-stream with ignoreErrors: false.

[aivus]

Permanent debug-only error listener can turn a failed download into a silent hang.

This listener stays attached for the life of the stream, so any consumer that does not attach its own error handler never sees the failure. A user saveResource plugin written in the v6 style from the README, e.g. await new Promise(res => resource.getContentStream().pipe(ws).on('finish', res)) (.pipe does not forward errors), will never settle when the server resets the connection mid-body: end/finish never fire, persistResource never returns, the p-queue slot is held forever and scrape() never resolves or rejects.

Without this listener the process would at least crash with an unhandled error. Better: surface the error (reject the save), or attach the listener only until a consumer takes over.

[aivus]

Fixed in 704720d: persistResource now races the saveResource actions against the live stream's error event, so a body failure always fails that resource's save (and goes through handleError / onResourceError) even when the action never settles. The debug listener stays, it is still needed for the window between headers arriving and the consumer attaching. Added a functional test with a .pipe()-only plugin which previously hung the scrape.

[aivus]

html/css lose mid-body retries they had on master, with no memory benefit.

These types are still fully buffered, but they now go through got.stream and requestWithRetry removes the retry listener at response time. With request: {retry: {limit: 2}} and a server that sends headers then resets the socket during the html body, master's got() promise API retried the whole request (ReadError/ECONNRESET is retryable) and succeeded on attempt 2. Now readStreamToBuffer rejects, no retry fires, and for the root page scrape() rejects on the first attempt.

Since html/css are buffered anyway, consider the promise API with responseType: 'buffer' for them, or wrap readStreamToBuffer in a retry, so only truly streamed types lose mid-body retry.

[aivus]

Fixed in 51166ee. Rather than switching html/css to the promise API (the type is only known after headers), request.get now also resolves with readBody(): it keeps got's retry listener attached while buffering, so a mid-body ReadError is retried by got under its own limit/errorCodes/backoff and the body is re-read from the retried response. The scraper uses it for html/css and afterResponse's getBody() goes through it as well. Streamed resources still consume stream directly and are not retried mid-body (documented in MIGRATION.md). Unit tests with a local server cover retry, no-retries-left, getBody, and that a streamed consumer does not retry; a functional test covers html retried while a child image is not.

[aivus]

getContentStream() semantics differ by content type, so the same plugin code silently produces empty files for binaries only.

Buffered content yields a fresh stream on every call; streamed content returns the same consumed live stream. A saveResource plugin that hashes then writes (const hash = await sha256(resource.getContentStream()); await pipeline(resource.getContentStream(), ws)) works for index.html but for a.png the second call returns the already-ended got stream, pipeline writes a 0-byte file and nothing throws.

Returning null/throwing on a second call for streamed resources, or buffering on second access, would make this either visible or correct.

[aivus]

Fixed in 3cf7736: a live stream is handed out once; a second getContentStream() call on a streamed resource throws Content stream of <resource> was already taken - ... instead of returning an ended stream. Buffered content (html/css text, or the buffered copy used when several saveResource actions are registered) still yields a fresh stream per call. Documented in README and MIGRATION.md, unit tests added.

[aivus]

error action now removes the directory even when this plugin wrote nothing to it.

Master guarded the rm with loadedResources.length > 0. Now it runs whenever beforeStart passed. A user plugin that writes e.g. ${options.directory}/requests.log from beforeRequest (directory did not exist at beforeStart, so validation passed) loses that file when the first request fails with ignoreErrors: false before any resource was saved.

Tracking "this plugin created something under the directory" (a flag set in saveResource when the stream/file is opened) keeps the intended cleanup of partial saves without deleting files it did not write.

[aivus]

Fixed in eff5146: the error action removes the directory only if this plugin's saveResource ran at least once (the flag is set right before outputFileStream, which is where directories get created, so a partial save is still cleaned up). A failure before any write leaves the directory and whatever other plugins put there alone, as on master. Functional test added.

[aivus]

Duplicate error path: the same failure is handled and logged twice.

saveResource already catches everything and only rethrows via handleError, so this extra catch exists purely to stash fatalError. For a streamed-resource save failure with ignoreErrors: false the log shows failed to save resource, then error occurred: X from handleError, then finishing with error: X from scrape().

If child rejections are propagated in downloadChildrenResources (see the comment on line 237), persistResource's rejection can simply propagate from the queue task and this catch goes away.

[aivus]

Superseded by the fix for the comment on line 237 (cc1efcd): the catch is now the place that classifies a save failure inside a request task - a got RequestError on a child is a non-fatal request error, anything else sets fatalError. So it is no longer a duplicate of saveResource's own handling. The two log lines (error occurred from handleError, finishing with error from scrape) also both appear on master for any save-phase failure, so leaving that as is.

[aivus]

getBody() is still callable after the live stream has been handed to the scraper.

A plugin that stores the response object (afterResponse: ({response}) => { last = response; return {}; }) and later calls last.getBody() while the image is still streaming to disk makes readStreamToBuffer and pipeline consume originalStream concurrently. Chunks are split between them: the saved file is truncated and getBody resolves with the other half, with no error raised.

Invalidating getBody (throw/reject) once transform() has returned would make this a loud failure instead of silent corruption.

(Re-anchored after 516e7a1 moved the getBody definition; the refactor does not change this behaviour - getBody still closes over originalStream and has no guard after applyAfterResponse returns.)

[aivus]

Fixed in 18a1160: getBody() now rejects with getBody() can only be called while the afterResponse action is running (<url>) once the action has returned (or thrown). Calls during the action, including concurrent ones, are unchanged. Unit test added.