fix(docker): add pypdf and honor stream scraping strategy - #2130
fix(docker): add pypdf and honor stream scraping strategy#2130nightcityblade wants to merge 2 commits into
Conversation
SohamKukreti
left a comment
There was a problem hiding this comment.
Thanks @nightcityblade for the quick turnaround on this — both code changes are correct, and I've verified them against an image built from this branch. There's one scope issue that blocks merging as-is, detailed below; the smaller items are in inline comments.
What I verified (image built from this branch, INSTALL_TYPE=default)
- ✅ pypdf is present in the default image; the ImportError from #2127 is gone on both /crawl and /crawl/stream.
- ✅ /crawl/stream now honors the client's scraping_strategy. Differential test (PDF strategy pointed at an HTML page): both endpoints now behave identically, where before the stream path silently swapped in LXMLWebScrapingStrategy and returned wrong content with no error.
- ✅ Removing the override causes no regression for clients that don't send a strategy: the library default has been LXMLWebScrapingStrategy since June 2025 (async_configs.py), so they get identical behavior — verified with plain crawls on both endpoints. The deleted line dates to Feb 2025 (392c923), when the library default was still the slower BS4 scraper; it's been redundant since the default changed, and its only remaining effect was this bug.
- ✅ Both new tests fail with the fix reverted; the branch merges cleanly into current develop.
Blocking: this PR should not close #2127
The issue's repro still fails on the image built from this branch — the error just changes from the ImportError to:
Failed on navigating ACS-GOTO: Page.goto: Download is starting
The container's headless Chromium can't render PDFs inline, so page.goto() turns every PDF URL into a download before the scraping strategy ever runs, and async_crawler_strategy.py only tolerates net::ERR_ABORTED. I tested 5 PDF hosts (arxiv, w3.org, pdfobject.com, irs.gov, raw.githubusercontent.com): 0/5 succeed. The accept_downloads escape hatch doesn't work through the API either (downloads_path is stripped by the untrusted-field allowlist, and the default /app/downloads isn't writable by appuser), and file:// URLs are blocked by SSRF protection by design — so there is currently no path to a working PDF scrape through the Docker server, with or without this PR.
Both changes here are necessary — they're just not sufficient for the issue's title claim. Two ways forward, either is fine with me:
- Rescope: retitle to what the PR verifiably does — e.g. fix(docker): add pypdf and honor client scraping strategy on /crawl/stream — and reference #2127 without a closing keyword, leaving the navigation failure tracked in the issue (also related: #1367).
- Extend: additionally make PDF navigation survivable — e.g. tolerate the download-triggered navigation failure when the configured scraping strategy is PDFContentScrapingStrategy, since that strategy fetches the PDF itself via its own download path and doesn't need the rendered page at all.
Also requested (see inline comments)
- Bound pypdf to match the library's existing spec in the root requirements.txt (pypdf>=6.0.0).
- Test robustness: parse requirement names instead of exact-line matching, and consider a behavioral test for the stream handler instead of the AST shape check — details and suggested code inline.
| mcp>=1.18.0 | ||
| websockets>=15.0.1 | ||
| httpx[http2]>=0.27.2 | ||
| pypdf |
There was a problem hiding this comment.
| pypdf | |
| pypdf>=6.0.0 |
There was a problem hiding this comment.
Updated to pypdf>=6.0.0 in 10130de, matching the library's root requirement.
| (ROOT / "deploy" / "docker" / "requirements.txt").read_text().splitlines() | ||
| ) | ||
| assert "pypdf" in requirements |
There was a problem hiding this comment.
This exact-line match breaks as soon as the requirement gets a version spec (e.g. pypdf>=6.0.0, requested above) — the test would fail on a correct change. Parsing the requirement names makes it robust:
frompackaging.requirementsimportRequirement, InvalidRequirementdeftest_default_docker_dependencies_include_pypdf():
lines= (ROOT/"deploy"/"docker"/"requirements.txt").read_text().splitlines()
names=set()
forlineinlines:
line=line.strip()
ifnotlineorline.startswith(("#", "-")):
continuetry:
names.add(Requirement(line).name)
exceptInvalidRequirement:
continue# pip-specific syntax (inline comments, paths, etc.)assert"pypdf"innamesThere was a problem hiding this comment.
Updated in 10130de. The regression now parses each valid requirement with packaging.requirements.Requirement, so version constraints do not make a correct dependency entry fail the test.
| if isinstance(target, ast.Attribute) | ||
| } | ||
| assert "scraping_strategy" not in assigned_attributes |
There was a problem hiding this comment.
This asserts on the shape of the source rather than the behavior, which makes it both bypassable and over-strict — I verified both directions locally:
- It passes with the bug reintroduced via
setattr(crawler_config, "scraping_strategy", LXMLWebScrapingStrategy())or via an annotated assignment (crawler_config.scraping_strategy: object = ...is anast.AnnAssign, which this walk doesn't catch). - It fails on legitimate code: a future default-only-if-unset pattern like
crawler_config.scraping_strategy = crawler_config.scraping_strategy or LXMLWebScrapingStrategy()preserves client strategies but trips this assertion — so the test would actively obstruct the correct implementation if we ever want an explicit server-side default here.
A behavioral test would guard the actual contract. For example: load the issue's payload through CrawlerRunConfig.load(..., provenance=Provenance.UNTRUSTED), run it through the same config-processing the stream handler does, and assert the config still holds a PDFContentScrapingStrategy. That stays green for any implementation that preserves the client's strategy and red for any that clobbers it, regardless of syntax.
There was a problem hiding this comment.
Replaced the AST assertion in 10130de with an async behavioral test. It invokes the real handle_stream_crawl_request, deserializes the issue's PDFContentScrapingStrategy config through the handler, and verifies that the resulting strategy is passed to crawler.arun_many.
Signed-off-by: nightcityblade <nightcityblade@gmail.com>
nightcityblade
commented
Aug 12, 2026
Thanks for the thorough validation and for identifying the remaining Chromium navigation failure. I took the rescope option: the title and description no longer claim to close #2127, and the unresolved download-triggered navigation behavior is explicitly left tracked there (with #1367 noted as related). The inline requests are addressed in 10130de: |
Summary
Addresses part of #2127 without closing it.
The default Docker image now installs the PDF processor dependency, and the streaming crawl handler preserves the scraping strategy supplied by the client instead of replacing it with
LXMLWebScrapingStrategy.This PR does not resolve the remaining Chromium navigation failure when a PDF response triggers a download. That behavior remains tracked in #2127 (and is related to #1367).
List of files changed and why
deploy/docker/requirements.txt- Installpypdf>=6.0.0in the default Docker image, matching the root requirement.deploy/docker/api.py- Preserve the deserialized scraping strategy for streaming crawls.tests/test_issue_2127_docker_pdf.py- Parse Docker requirement names robustly and behaviorally verify that the real stream handler passes a requestedPDFContentScrapingStrategyto the crawler.How Has This Been Tested?
.venv/bin/pytest -q tests/test_issue_2127_docker_pdf.py tests/test_pr_1795_1798_1734.py::TestDeepCrawlStreamBranching(7 passed).venv/bin/black --check --target-version py312 --fast tests/test_issue_2127_docker_pdf.py.venv/bin/python -m py_compile tests/test_issue_2127_docker_pdf.py deploy/docker/api.pyuv pip install --dry-run --python .venv/bin/python -r deploy/docker/requirements.txtgit diff --checkChecklist: