Skip to content

feat: Add skipRequest context helper - #4158

Merged
janbuchar merged 9 commits into
masterfrom
skip-request-helper
Oct 6, 2026
Merged

janbuchar merged 9 commits into
masterfrom
skip-request-helper

Conversation

@janbuchar

Copy link
Copy Markdown
Contributor

@janbuchar janbuchar added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 25, 2026
@janbuchar janbuchar self-assigned this Sep 25, 2026
@apify-service-account apify-service-account added the tested Temporary label used only programatically for some analytics. label Sep 25, 2026

@B4nan B4nan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few things found from staff review, I'd address the first one at least

Comment thread packages/basic-crawler/src/internals/basic-crawler.ts Outdated
Comment thread packages/basic-crawler/src/internals/crawlers/crawler_commons.ts Outdated
Comment thread test/core/crawlers/basic_crawler.test.ts

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @janbuchar !

I agree with Martin's notes above, otherwise lgtm 👍

Comment thread packages/basic-crawler/src/internals/errors.ts
@janbuchar
janbuchar changed the base branch from master to improve-browserpool-exceptions September 30, 2026 07:42
@janbuchar
janbuchar added this pull request to stack #4173 September 30, 2026 07:42
@B4nan
B4nan force-pushed the improve-browserpool-exceptions branch from 7313f85 to 05ec98b Compare October 1, 2026 12:57
Base automatically changed from improve-browserpool-exceptions to master October 1, 2026 14:57
@janbuchar
janbuchar force-pushed the skip-request-helper branch from 274dff9 to 47bb349 Compare October 1, 2026 20:19
@janbuchar
janbuchar requested review from B4nan and barjin October 2, 2026 06:20
Comment thread packages/basic-crawler/src/internals/basic-crawler.ts Outdated

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @janbuchar , few more questions, but otherwise lgtm.

Once these are resolved (or considered wontfix), it's good to go by me 👍

Comment thread packages/basic-crawler/src/internals/basic-crawler.ts Outdated
Comment thread packages/basic-crawler/src/internals/basic-crawler.ts Outdated
@janbuchar
janbuchar requested review from B4nan and barjin October 3, 2026 20:06
@janbuchar

Copy link
Copy Markdown
Contributor Author

@B4nan @barjin thanks guys, I fixed the last batch. Give it a quick look please.

@barjin barjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @janbuchar , just one last edge case ⬇️

Once that's dealt with, let's merge :shipit:

// retry is counted, so that an error handler that always times out does not retry it forever.
this.log.warning(unwrappedSecondaryError.message);
if (!skipped && request.retryCount === retryCountBefore) {
if (!(err instanceof ContextPipelineInterruptedError) && request.retryCount === retryCountBefore) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When onSkippedRequest times out, err here is still the ContextPipelineInterruptedError, right? Does it mean this ⬇️ will keep looping?

import { BasicCrawler, Configuration, serviceLocator } from '@crawlee/basic';
import { setTimeout } from 'node:timers/promises';

serviceLocator.setConfiguration(new Configuration({ internalTimeoutMillis: 100 }));
const crawler = new BasicCrawler({
    requestHandlerTimeoutSecs: 0.05,
    requestHandler: async ({ skipRequest }) => skipRequest(),
    onSkippedRequest: () => setTimeout(300),
});
await crawler.run(['https://example.com']);

Something like:

  • onSkippedRequest timeouts
  • we do not increment the counter because of the change here
  • we process the request again, skip it again
  • back to step one - infinite loop?

Maybe a timed-out onSkippedRequest should go the same way as a throwing one, i.e. become an ordinary failure that uses up the retry budget?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, thanks! fedcf11

@janbuchar
janbuchar removed this pull request from stack #4173 October 6, 2026 09:55
@janbuchar
janbuchar enabled auto-merge (squash) October 6, 2026 09:56
@janbuchar
janbuchar merged commit 69a2c07 into master Oct 6, 2026
10 checks passed
@janbuchar
janbuchar deleted the skip-request-helper branch October 6, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decide the public status of ContextPipelineInterruptedError before 4.0

5 participants