test: fix storage pollution in browser crawler tests#3861
Open
B4nan wants to merge 132 commits into
Open
Conversation
BREAKING CHANGE: The project is now native ESM without a CJS alternative. This is fine since all supported node versions allow `require(esm)`. Also all the dependencies are updated to the latest versions, including cheerio v1.
BREAKING CHANGE: The crawler following options are removed: - `handleRequestFunction` -> `requestHandler` - `handlePageFunction` -> `requestHandler` - `handleRequestTimeoutSecs` -> `requestHandlerTimeoutSecs` - `handleFailedRequestFunction` -> `failedRequestHandler`
BREAKING CHANGE: The crawling context no longer includes the `Error` object for failed requests. Use the second parameter of the `errorHandler` or `failedRequestHandler` callbacks to access the error. Previously, the crawling context extended a `Record` type, allowing to access any property. This was changed to a strict type, which means that you can only access properties that are defined in the context.
….retireOnBlockedStatusCodes` BREAKING CHANGE: `additionalBlockedStatusCodes` parameter of `Session.retireOnBlockedStatusCodes` method is removed. Use the `blockedStatusCodes` crawler option instead.
….retireOnBlockedStatusCodes` BREAKING CHANGE: `additionalBlockedStatusCodes` parameter of `Session.retireOnBlockedStatusCodes` method is removed. Use the `blockedStatusCodes` crawler option instead.
also tries to bump better-sqlite3 to latest version to have prebuilds for node 22
- closes #2479 - closes #3106 - closes #3107 - closes #3078 In my opinion, it makes a lot of sense to do the remaining changes in a separate PR. - [x] Introduce a `ContextPipeline` abstraction - [x] Update crawlers to use it - [x] Make sure that existing tests pass - [ ] Refine the `ContextPipeline.compose` signature and the semantics of `BasicCrawlerOptions.contextPipelineEnhancer` to maximize DX - [x] Write tests for the `contextPipelineEnhancer` - [x] Resolve added TODO comments (fix immediately or make issues) - [ ] Update documentation The `context-pipeline` branch introduces a fundamental architectural change to how Crawlee crawlers build and enhance the crawling context passed to request handlers. The core motivation is to fix the composition and extensibility nightmare in the current crawler hierarchy. 1. **Rigid inheritance hierarchy**: Crawlers were stuck in a brittle inheritance chain where each layer manipulated the context object while assuming that it already satisfied its final type. Multiple overrides of `BasicCrawler` lifecycle methods made the execution flow even harder to follow. 2. **Context enhancement via monkey-patching**: Manual property assignment (`crawlingContext.page = page`, `crawlingContext.$ = $`) scattered everywhere. It was a mess to follow and impossible to reason about. 3. **Cleanup coordination**: Resource cleanup was handled by separate `_cleanupContext` methods that were not co-located with the initialization. 4. **Extension mechanism was broken**: The `CrawlerExtension.use()` API tried to let you extend crawlers (the ones based on `HttpCrawler`) by overwriting properties - completely type-unsafe and fragile as hell. Introduces `ContextPipeline` - a **middleware-based composition pattern** where: - Each crawler layer defines how it enhances the context through explicit `action` functions - Cleanup logic is co-located with initialization via optional `cleanup` functions - Type safety is maintained through TypeScript generics that track context transformations - The pipeline executes middleware sequentially with proper error handling and guaranteed cleanup Declarative middleware composition with co-located cleanup: ```typescript contextPipeline.compose({ action: async (context) => ({ page, $ }), cleanup: async (context) => { await page.close(); } }) ``` The `ContextPipeline<TBase, TFinal>` tracks type transformations through the chain: ```typescript ContextPipeline<CrawlingContext, CrawlingContext> .compose<{ page: Page }>(...) // ContextPipeline<CrawlingContext, CrawlingContext & { page: Page }> .compose<{ $: CheerioAPI }>(...) // ContextPipeline<CrawlingContext, CrawlingContext & { page: Page, $: CheerioAPI }> ``` The `CrawlerExtension.use()` is gone. New approach via `contextPipelineEnhancer`: ```typescript new BasicCrawler({ contextPipelineEnhancer: (pipeline) => pipeline.compose({ action: async (context) => ({ myCustomProp: ... }) }) }) ``` The current way to express a context pipeline middleware has some shortcomings (`ContextPipeline.compose`, `BasicCrawlerOptions.contextPipelineEnhancer`). I suggest resolving this in another PR. For most legitimate use cases, this should be non-breaking. Those who extend the Crawler classes in non-trivial ways may need to adjust their code though - the non-public interface of `BasicCrawler` and `HttpCrawler` changed quite a bit. The pipeline uses `Object.defineProperties` for each middleware. Is this a serious performance consideration? --------- Co-authored-by: Martin Adámek <banan23@gmail.com>
Extracts `ProxyConfiguration` to `BasicCrawler` (related to discussion under #2917). Pass the `ProxyConfiguration` instance to the `SessionPool` for new `Session` object creation. Store and read the `ProxyInfo` from the `Session` instance instead of calling the `ProxyConfiguration` methods in the crawlers. closes #3198
Phasing out `got-scraping`-specific interfaces in favour of native `fetch` API. Related to #3071
Fixes build toolchain errors caused by the recent rebase onto the current `master` ([more details here](https://apify.slack.com/archives/C02JQSN79V4/p1764373034961859)). The largest thing is probably updating the dependency versions in `package.json` - if `turborepo` doesn't find the matching version in the local workspace, it will build against the package pulled from `npm` (which doesn't match the v4 API at this point).
Related to #3275 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Makes `@crawlee/impit-client` the default HTTP client for `@crawlee/basic`'s `BasicCrawler`.
Adds recent changes to the `SessionPool` and related classes to the existing guides. Closes #796
Utilizes the byte-stream prescan (https://html.spec.whatwg.org/multipage/parsing.html#prescan-a-byte-stream-to-determine-its-encoding) to determine the HTML document encoding, if not specified in HTTP headers (or explicitly forced by user). Closes #2317
Due to the nature of the concurrency model in JS, we cannot reliably abort the `requestHandler` execution on timeouts. This can lead to "race conditions" with, e.g., the `requestHandler` modifying the storages while the `errorHandler` / `failedRequestHandler` is already running. This PR adds a `tryCancel()` check throwing on elapsed timeouts before each storage method call. Closes #2889
Avoid double-counting requests in `RequestManagerTandem`. Closes #3787
…)` (#3831) Serializes the passed data in `pushData` calls before storing it.
Closes #3396 --------- Co-authored-by: Jindřich Bär <jindrichbar@gmail.com>
…ite (#3844) `scripts/copy.ts` rewrites the version of every `@crawlee/*` dependency (and `crawlee`) to the monorepo version during canary publish and `pin-versions`. But `@crawlee/fs-storage-native` is an externally published package (pinned to `0.1.5-beta.18` in `@crawlee/fs-storage`) that is **not** versioned in lockstep with the monorepo, so rewriting it produced a non-existent version and broke CI publishing. This excludes `@crawlee/fs-storage-native` from the version-rewrite loop in both the `canary` and `pin-versions` branches.
…e` (#3840) Changes the extended unique key separator from `METHOD(payloadHash):url` to `METHOD|payloadHash|url`, matching crawlee-python. Adds an `alwaysEnqueue` request option that prepends a random value to the unique key so the request always gets enqueued; throws if combined with a custom `uniqueKey`. Closes #3221
Merging #3545 has caused some CI issues in other v4 PRs. This PR fixes these.
Makes `cheerio` and `proxy-chain` imports in `crawlee` packages lazy to optimize import times. Related to #3549
`format:check` ran bare `oxfmt`, which defaults to writing: ``` "format": "oxfmt packages test docs --write", "format:check": "oxfmt packages test docs", ``` So the CI step (`.github/workflows/test-ci.yml:163`) rewrote the files inside the runner and exited 0. The formatting gate could never fail, and three files drifted past it. Adds the missing `--check` and reformats the drift, which turned out to be exactly three files — all cosmetic (one wrapped signature, two stray blank lines). With the fix in place `pnpm format:check` correctly exits 1 on unformatted input. Found while working on the timeout changes, where a stray `pnpm format:check` spontaneously reformatted files in a clean tree.
`browser_crawler.test.ts` is the only test file in the repo using `test.concurrent`, and the only one that gives different results run to run. Locally it failed anywhere between 3 and 10 tests with the set changing every time, which makes it useless for telling whether a change actually broke something — I hit this while trying to work out whether a timeout change had regressed anything, and had to run a control on a clean branch to be sure it hadn't. Running the file serially costs ~28s and makes the outcome deterministic. Worth knowing: locally this does not make the file green — 9 tests fail consistently, identically on a clean v4, because they pass in isolation but not in file order. That pollution is pre-existing and out of scope here; this only removes the variance that was hiding it. If CI is green, those 9 are local-environment-specific and can be looked at separately.
`browser_crawler.test.ts` failed 9 tests locally on every run after the first, while CI stayed green. The tests each passed in isolation. The file scopes a per-test `ServiceLocator` with a `MemoryStorageBackend` via `aroundEach`, but the global setup in `test/vitest.setup.ts` calls `serviceLocator.reset()` from a `beforeEach` — and `beforeEach` runs *inside* an `aroundEach` scope. That reset resolves through the `serviceLocator` proxy to the active, scoped locator and wipes the backend the hook just installed, so each test implicitly built a `FileSystemStorageBackend` and wrote a real `./storage` instead. Since `BasicCrawler` assigns each instance a deterministic `__default_<n>__` queue alias, the next run reopened the previous run's persisted queue, saw the requests already handled, and crawled nothing — hence hooks never firing and ~80ms "failures". CI never saw it because it always starts without a `storage/` directory. Reset the global locator explicitly in the setup file so it no longer clobbers a deliberately scoped one. The suite now keeps its in-memory backend and writes nothing to disk. This also removes the shared state behind the run-to-run variance that #3859 worked around, so the `test.concurrent` usage that PR removed is restored (that revert is exact — the four tests that were deliberately serial for env-var, log-level and queue-conflict reasons stay serial).
B4nan
added
adhoc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
browser_crawler.test.tsfails ~9 tests locally on a second and every subsequent run, while passing on the first run after a fresh clone. CI never sees it because CI always starts without astorage/directory.The cause is cross-run pollution via disk, in three parts:
ServiceLocatorwith aMemoryStorageBackendviaaroundEach.test/vitest.setup.tscallsserviceLocator.reset()from a globalbeforeEach— andbeforeEachruns inside anaroundEachscope.serviceLocatoris an ALS-aware proxy that resolves to the active locator, so that reset wiped the backend the hook had just installed. Each test then implicitly constructed aFileSystemStorageBackendand wrote a real./storage.BasicCrawler's staticinstanceCountgives each crawler a deterministic__default_<n>__queue alias, so run N+1 reopened run N's persisted queue, sawhandledAtalready set, and crawled nothing — which is why the failures look like "the pre-navigation hook was never called".The fix resets the global locator explicitly instead of going through the scope-aware proxy, so it cannot clobber a deliberately scoped one. That needs
globalServiceLocatorexported frompackages/core/src/service_locator.ts, marked@internal. Nothing changes for the ~40 other test files: with no scope active, the proxy already resolved to the global locator.This is the root cause rather than a workaround — the suite keeps its in-memory backend and now writes nothing to disk at all.
test.concurrentis restored as a result. The conversion to serial in #3859 was treating a symptom; with the pollution actually fixed, the file is green and stable with concurrency (4/4 consecutive runs) and faster with it (~23s vs ~37s). The four tests that were deliberately serial before #3859 — with comments explaining why — stay serial.Verified against a polluted
storage/still on disk, and confirmed nostorage/is recreated.