Skip to content

Fix discovery crawls saving zero pages on Scrapy ≥ 2.13 - #33

Open
sahilsunny wants to merge 4 commits into
sahil/scr-585-update-scrapingbee-cli-package-to-support-header-basedfrom
fix/crawl-engine-dispatch-scrapy213
Open

Fix discovery crawls saving zero pages on Scrapy ≥ 2.13#33
sahilsunny wants to merge 4 commits into
sahil/scr-585-update-scrapingbee-cli-package-to-support-header-basedfrom
fix/crawl-engine-dispatch-scrapy213

Conversation

@sahilsunny

Copy link
Copy Markdown
Collaborator

Scrapy 2.13 removed the spider argument of ExecutionEngine.crawl(), and both of our save-dispatch sites still passed it. On the locked Scrapy 2.16 every queued save raised TypeError, the error handler logged and moved on, so any crawl that needs the discovery phase (--return-page-text, --extract-rules, --ai-query, screenshot without --json-response) exited successfully with zero pages saved. Pre-existing on main — would have shipped in 1.6.0.

The fix is a small _engine_crawl() helper that checks the engine's signature and calls it the right way on both old and new Scrapy. Regression tests cover both shapes, plus a canary that fails loudly if a future Scrapy changes the signature again.

Two small extras: the discovery double-credit prompt now says --confirm yes (it used to suggest --yes, which doesn't exist), and the skill docs' custom-google price goes back to 15 credits — measured Spb-cost is 15; it's the API's error message claiming 20 that's wrong.

Verified with a live discovery crawl on a pipx install (2/2 pages saved with real content). 866 unit tests green, ruff + ty clean.

Stacked on #32 — merge order: #31#32 → this, all with merge commits.

Scrapy 2.13 removed the 'spider' argument of ExecutionEngine.crawl()
(deprecated since 2.10). Both save-dispatch sites still passed it, so on
the locked Scrapy 2.16 every queued save raised TypeError, the
log-and-continue handler swallowed it, and any crawl needing the
discovery phase (--return-page-text, --extract-rules, --ai-query,
screenshot without --json-response) reported success with zero pages
saved. New _engine_crawl() helper probes the signature and dispatches
correctly on both old and new Scrapy; regression tests cover both
shapes plus a canary that fails loudly if a future Scrapy changes the
signature again.

Also: the discovery double-credit prompt told users to pass --yes; the
real flag is --confirm yes. And the skill docs' custom-google price is
restored to 15 credits — measured Spb-cost is 15; the API's own error
message claiming 20 is what's wrong.

Repro red->green: bug reproduced on main (same TypeError mechanically
confirmed against installed Scrapy 2.16 signature); after the fix a
live discovery crawl on the pipx build saved 2/2 pages with real
content. 866 unit tests green; ruff + ty clean.
Comment thread src/scrapingbee_cli/crawl.py Outdated
def _engine_crawl(engine: Any, request: Any, spider: Spider) -> None:
"""Dispatch a request on a running engine across Scrapy versions.

Scrapy 2.10 deprecated (and 2.13 removed) the ``spider`` argument of

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So version pinned is 2.16, but I guess the idea here is that user may still have old scrapy installed and not have their env upgraded, right? Just to understand

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point — old Scrapy versions can't run our spider anyway, so the fallback was useless. Removed it; now it's just engine.crawl(request) plus a test that catches any future signature change.

Stacked PRs (based on another PR's branch) silently got zero checks:
on.pull_request was filtered to branches [main], so PRs #32 and #33
never ran CI. Drop the filter so every PR runs the suite; the push
trigger stays main-only.
…t-only

Review feedback on #33: the legacy engine.crawl(request, spider) branch is
unreachable in any working configuration — the spider's async start() entry
point requires Scrapy >= 2.13, and 2.13 is also where the spider argument
was removed. Call engine.crawl(request) directly at both dispatch sites and
tighten the contract test to require the exact (self, request) signature,
so any future Scrapy API change fails loudly in CI at upgrade time instead
of silently adapting. Live re-verified the idle-flush path (example.com,
--max-pages 10 -> 1.txt saved) on the pipx build.
The entry overstated the blast radius (only crawls whose save queue never
reached --max-pages were affected) and still described the removed
version-adaptive dispatch; the fix is now a direct request-only call plus
a CI contract test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants