Skip to content

fix(batch): retain failed scrape results - #505

Merged
us merged 3 commits into
us:mainfrom
atirna:fix/scrape-completion-accounting
Sep 8, 2026
Merged

fix(batch): retain failed scrape results#505
us merged 3 commits into
us:mainfrom
atirna:fix/scrape-completion-accounting

Conversation

@atirna

@atirna atirna commented Sep 5, 2026

Copy link
Copy Markdown

Change

Batch scrapes now retain a blocked document when an individual scrape fails, keeping the result list aligned with the completed count while preserving the failed URL and reason.

Fixes #487

Measured

The regression failed on current main with blocked = 0 for a completed scrape error. It now reports one blocked document with the failed URL and error reason.

Tests

  • cargo test -p crw-server start_batch_job_records_scrape_errors_as_blocked_documents -- --nocapture

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@atirna

atirna commented Sep 5, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Sep 5, 2026
A URL the engine could not turn into a document is now retained with a `block`,
but `V2Document` has no field that can carry it, so on /v2 it reached the caller
as an empty document with nothing to explain it, while GET
/v2/{batch/scrape,crawl}/{id}/errors, the route documented to carry exactly
those, returned an empty array and reported only a job-level failure.
docs/docs/recipe-batch.md promises the opposite: "URLs that fail mid-job are
recorded ... Retrieve them after the job completes".

Also updates the credits_used note in the v2 adapter, which described the
accounting from before failed URLs were retained and is no longer true: the sum
is now exact on the batch path rather than a lower bound.

The new state test asserted a whole customer-facing sentence that is defined in
crw-crawl; match on the vendor and a substring instead, as the neighbouring test
in that module does, so rewording a message does not break a crw-server test.
@us

us commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks for this, and sorry for the slow start: your CI had never actually run, because a
first-time contributor's workflows sit behind an approval. That is released now.

The bug is real and I reproduced it in both directions against locally built engines rather
than trusting the unit test. Same request, two binaries:

POST /v1/batch/scrape {"urls":["https://example.com/","https://example.com:81/","https://example.org/"]}

main    -> completed=3  blocked=0  data=2  billable=3
this PR -> completed=3  blocked=1  data=3  billable=2
           data[2].block.reason = "Target unreachable: Could not reach https://example.com:81/: ..."

On main the caller receives two documents and is accounted for three pages. Your change makes
the results array and the counters agree, which is the right fix in the right place. There is
now a repro script keyed to it that exits non-zero on main and zero on this branch.

One thing worth recording for the issue: a host that does not resolve never reaches the batch
loop at all, because upfront URL validation rejects it into invalidUrls. The reachable cases
are the ones that pass validation and then fail during the scrape, which is what the port-81
URL above exercises.

I have pushed a commit onto your branch with three follow-ups.

1. The failure reason did not reach the /v2 surface at all. V2Document has no block
field, so the retained placeholder serialized with nothing to explain it, and the route
documented to carry these was empty:

GET /v2/batch/scrape/{id}
  data[1] = {"metadata":{"sourceURL":"https://example.com:81/","statusCode":0,...}}   # no markdown, no warning
GET /v2/batch/scrape/{id}/errors
  {"errors":[],"robotsBlocked":[],"success":true}

docs/docs/recipe-batch.md promises "URLs that fail mid-job are recorded ... Retrieve them
after the job completes" against that route. On /v1 your change delivers exactly that; on
/v2 a caller was getting a silent empty document where the URL used to be absent. The
/errors route now emits the per-URL failures alongside the job-level one, for crawl as well
as batch, which keeps the /v2 document shape untouched and makes the doc true.

2. routes/v2/adapters.rs carried a comment describing the pre-fix accounting. It said
completed "also advances for a URL whose scrape returned Err and pushed no document" and
called it pre-existing because "fixing it means changing what completed counts". After your
change an Err does push a document and the sum is exact, so the note is updated rather than
left to mislead the next reader into re-deriving a lower bound.

3. The new test pinned a whole customer-facing sentence that is defined two crates away in
crw-crawl, and it was the only pin on that string. It now matches on the vendor plus a
substring, the way the neighbouring test in that module does.

Two things I did not change, that are worth deciding on before this merges.

The accounting change should be in the PR description. completed - blocked is what the
hosted side turns into usage, so this PR changes what a batch costs. Measured on the two
binaries: a 60-URL batch where every URL fails goes from 60 billed pages to 0. That is the
intended direction and matches the "no billing on failure" rule, but it is a money path and it
should be visible in the description rather than found later.

err.to_string() goes straight into a customer-visible field. Verified live against a
build with a deliberately named sidecar:

data[0].block.reason =
  "Renderer error: CDP discovery failed: error sending request for url
   (http://internal-secret-chrome-host.crw-internal:9222/json/version)"

On a hosted deployment that renders as the internal renderer host and port. This is not
something you introduced: /v1/scrape returns the identical string in error today, and the
crawl path does the same. The cause is that reqwest::Error's Display appends
" for url (...)" for every internal call, so it reaches the same field from the CDP tier, the
camoufox and cloak sidecars, and the managed LLM endpoint. reqwest::Error::without_url() is
the right primitive and the fix belongs at those call sites, not in the batch loop, so I am
keeping it out of this PR and handling it separately.

Smaller, not blocking: vendor: "http_error" for every error class means an invalid-request
rejection is labelled as an anti-bot block by BlockOutcome::message(), while
CrwError::error_code() already distinguishes invalid_request / timeout /
extraction_error / shutdown.

A failed URL is now kept as a placeholder, but on /v2 that placeholder had
nothing to explain it: V2Document has no block field, and neither SDK reads
the errors route. The block reason now lands in the document's warning, which
the frozen shape already carries. The errors route lists only documents that
have no body, so an origin error page that was delivered readable is not also
reported as a failure, and each entry gets a stable per-entry id derived from
the job id and position.

A fault in the caller's own template (actions, or screenshot with
renderJs:false) used to come back as one anti-bot placeholder per URL. Both
checks now live in crw_crawl::single::validate_scrape_template, run at the top
of the single scrape and of POST /v1/batch/scrape, so a bad template is one
400 on both surfaces.

ScrapeData::has_body names the placeholder shape once. Docs describe the
blocked counter, the errors entry with its url, and the credits caveat for
multi-page PDFs.
@us

us commented Sep 8, 2026

Copy link
Copy Markdown
Owner

one more commit on top. on /v2 the retained placeholder was indistinguishable from a page, and neither sdk reads the errors route, so its block reason now rides in the document's warning, which is already part of the shape. the errors route lists only documents that have no body (an origin error page delivered readable is not also an error), each entry with a stable id. and a fault in the caller's own template (actions, or screenshot with renderJs:false) is now one 400 on both batch routes instead of one placeholder per url; the checks moved into crw_crawl::single::validate_scrape_template so the single scrape and batch share them. docs updated for blocked and the errors entry.

@us
us merged commit 15e9bb9 into us:main Sep 8, 2026
12 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

batch scrape drops a URL from the results array when its scrape errors

2 participants