diff --git a/crates/crw-core/src/types.rs b/crates/crw-core/src/types.rs index 20660ede..7867b505 100644 --- a/crates/crw-core/src/types.rs +++ b/crates/crw-core/src/types.rs @@ -963,6 +963,25 @@ impl ScrapeData { self.summary = None; self.chunks = None; } + + /// Whether any page-content field survived. A document without a body is a + /// placeholder for a URL that produced no page (a cleared wall, or a scrape + /// error), as opposed to an origin error page that was kept readable. + /// `source_hash` and `basis` are excluded on purpose: both derive from the + /// fields below and never stand on their own. `screenshot` is not counted + /// either, so a wall captured with a screenshot still reads as no page + /// while the caller keeps the image. + pub fn has_body(&self) -> bool { + self.markdown.is_some() + || self.html.is_some() + || self.raw_html.is_some() + || self.plain_text.is_some() + || self.links.is_some() + || self.images.is_some() + || self.json.is_some() + || self.summary.is_some() + || self.chunks.is_some() + } } /// Typed anti-bot block verdict. `vendor` is the antibot `class_name` @@ -1452,6 +1471,20 @@ mod tests { assert!(d.http_error().is_some()); } + #[test] + fn has_body_separates_a_placeholder_from_a_readable_error_page() { + let mut d = ScrapeData::default(); + assert!(!d.has_body(), "a failed_page or cleared wall has no body"); + d.markdown = Some("404 Not Found".into()); + assert!( + d.has_body(), + "an origin error page kept readable has a body" + ); + d.markdown = None; + d.links = Some(vec!["https://example.com/a".into()]); + assert!(d.has_body(), "a links-only format still delivered a page"); + } + #[test] fn clear_body_drops_content_keeps_metadata_and_block() { let mut data = ScrapeData { diff --git a/crates/crw-crawl/src/crawl.rs b/crates/crw-crawl/src/crawl.rs index 9d5c6bb6..a40863d5 100644 --- a/crates/crw-crawl/src/crawl.rs +++ b/crates/crw-crawl/src/crawl.rs @@ -109,14 +109,14 @@ fn enqueue_discovered_links( } } -/// Build the placeholder document a crawl returns for a URL it could not read. +/// Build the placeholder document a crawl or batch returns for a URL it could not read. /// /// Carries only the URL, the status (0 when there was no response at all) and /// the reason, stamped through the same `block` field the scrape and batch /// paths already use, so every surface that already understands "this document /// is not a page you asked for" understands this one too, and the caller's /// `completed - blocked` billing keeps it free. -fn failed_page(url: &str, status_code: u16, reason: String) -> ScrapeData { +pub fn failed_page(url: &str, status_code: u16, reason: String) -> ScrapeData { ScrapeData { metadata: crw_core::types::PageMetadata { source_url: url.to_string(), diff --git a/crates/crw-crawl/src/single.rs b/crates/crw-crawl/src/single.rs index 00ae3f4e..131e533d 100644 --- a/crates/crw-crawl/src/single.rs +++ b/crates/crw-crawl/src/single.rs @@ -106,6 +106,27 @@ pub async fn scrape_url( }) } +/// Reject the faults in a scrape template that no fetch can repair, before any +/// network work. Shared by the single scrape and the batch route, so a bad +/// template is one 400 on both surfaces rather than one placeholder document +/// per URL labelled as a block. +pub fn validate_scrape_template(req: &ScrapeRequest) -> CrwResult<()> { + if req.actions.is_some() { + return Err(crw_core::error::CrwError::InvalidRequest( + "The 'actions' parameter is not yet supported. Use cssSelector or xpath for element targeting.".into() + )); + } + // A screenshot is captured via CDP and cannot be produced on the HTTP-only + // path. An explicit `renderJs:false` + `screenshot` is contradictory: reject + // it rather than silently return a null screenshot. + if req.formats.contains(&OutputFormat::Screenshot) && req.render_js == Some(false) { + return Err(crw_core::error::CrwError::InvalidRequest( + "screenshot format requires JS rendering; remove renderJs:false (or omit it)".into(), + )); + } + Ok(()) +} + #[allow(clippy::too_many_arguments)] async fn scrape_url_inner( req: &ScrapeRequest, @@ -117,12 +138,7 @@ async fn scrape_url_inner( render_js_default: Option, deadline: Deadline, ) -> CrwResult { - // Reject unsupported `actions` parameter early with a clear error. - if req.actions.is_some() { - return Err(crw_core::error::CrwError::InvalidRequest( - "The 'actions' parameter is not yet supported. Use cssSelector or xpath for element targeting.".into() - )); - } + validate_scrape_template(req)?; // Determine whether stealth headers should be injected for this request. let inject_stealth = req.stealth.unwrap_or(default_stealth); @@ -142,17 +158,10 @@ async fn scrape_url_inner( // render_js_default=true and a per-request proxy still reaches the JS renderer. let effective_render_js = resolve_render_js(effective_render_js_request, render_js_default); - // A screenshot is captured via CDP and cannot be produced on the HTTP-only - // path. An explicit `renderJs:false` + `screenshot` is contradictory — reject - // it rather than silently return a null screenshot. For the default/auto case - // the renderer forces the CDP path (see FallbackRenderer::fetch), and the - // temp HTTP fetcher below is skipped so the screenshot is never dropped. + // For the default/auto case the renderer forces the CDP path for a + // screenshot (see FallbackRenderer::fetch), and the temp HTTP fetcher below + // is skipped so the screenshot is never dropped. let wants_screenshot = req.formats.contains(&OutputFormat::Screenshot); - if wants_screenshot && req.render_js == Some(false) { - return Err(crw_core::error::CrwError::InvalidRequest( - "screenshot format requires JS rendering; remove renderJs:false (or omit it)".into(), - )); - } // Validate pinned renderer is available — fail fast with a 400 instead of // letting the request reach the dispatcher with a hard-pin to a missing pool. diff --git a/crates/crw-server/src/routes/batch.rs b/crates/crw-server/src/routes/batch.rs index a4a8a2f5..1d358007 100644 --- a/crates/crw-server/src/routes/batch.rs +++ b/crates/crw-server/src/routes/batch.rs @@ -102,6 +102,10 @@ pub async fn start_batch( // Reject an unavailable pinned renderer up front (as /v1/scrape and /v1/crawl // do) instead of failing every URL individually deep in the pipeline. crate::state::validate_renderer_pin(template.renderer, template.render_js, &state)?; + // Same rejections `/v1/scrape` applies at the top of the scrape. Left to + // the per-URL path they would come back as N placeholder documents each + // labelled as an anti-bot block, for a fault in the caller's own request. + crw_crawl::single::validate_scrape_template(&template)?; template.url = String::new(); // Partition URLs into valid / invalid (SSRF-checked, same guard as @@ -219,6 +223,38 @@ mod tests { } } + #[tokio::test] + async fn start_batch_rejects_actions_before_any_url_work() { + let state = default_state(); + let err = call( + &state, + json!({ "urls": ["https://example.com/"], "actions": [] }), + ) + .await + .unwrap_err(); + assert!( + invalid_request_message(&err).contains("'actions'"), + "got: {}", + invalid_request_message(&err) + ); + } + + #[tokio::test] + async fn start_batch_rejects_screenshot_without_js_before_any_url_work() { + let state = default_state(); + let err = call( + &state, + json!({ "urls": ["https://example.com/"], "formats": ["screenshot"], "renderJs": false }), + ) + .await + .unwrap_err(); + assert!( + invalid_request_message(&err).contains("screenshot"), + "got: {}", + invalid_request_message(&err) + ); + } + #[tokio::test] async fn start_batch_requires_urls_field() { let state = default_state(); diff --git a/crates/crw-server/src/routes/v2/adapters.rs b/crates/crw-server/src/routes/v2/adapters.rs index 5717d504..044c6c46 100644 --- a/crates/crw-server/src/routes/v2/adapters.rs +++ b/crates/crw-server/src/routes/v2/adapters.rs @@ -123,7 +123,13 @@ pub fn to_v2_document(data: ScrapeData, proxy_used: &str, scrape_id: String) -> summary: data.summary, change_tracking: data.change_tracking, screenshot: data.screenshot, - warning: data.warning, + // `V2Document` has no `block`, so a URL retained only as a placeholder + // would otherwise reach a /v2 caller as an empty document with nothing + // to explain it. `warning` is part of the frozen shape and is exactly + // where Firecrawl surfaces a per-document problem. + warning: data + .warning + .or_else(|| data.block.as_ref().map(|b| b.reason.clone())), metadata, } } @@ -221,12 +227,12 @@ pub fn build_crawl_status( None }; - // A blocked page is not billed, so it must not be counted here either. - // Note this is a lower bound rather than the exact charge on the batch path: - // `completed` also advances for a URL whose scrape returned `Err` and pushed - // no document, and the SaaS bills `completed - blocked`. Pre-existing; fixing - // it means changing what `completed` counts, which the job-completion gate - // depends on. + // A blocked page is not billed, so it must not be counted here either. A + // batch URL whose scrape returns `Err` now pushes a placeholder carrying + // `block` and bumps `blocked`, so it is excluded here for the same reason a + // wall is, and `completed - blocked` no longer over-counts it. The two are + // still not identical: this sum prices a multi-page PDF per page, while + // `completed - blocked` counts documents. let credits_used: u32 = state .data .iter() diff --git a/crates/crw-server/src/routes/v2/batch.rs b/crates/crw-server/src/routes/v2/batch.rs index 543a0aee..4dcb5226 100644 --- a/crates/crw-server/src/routes/v2/batch.rs +++ b/crates/crw-server/src/routes/v2/batch.rs @@ -90,6 +90,10 @@ pub async fn start_batch( .map_err(|e| CrwError::InvalidRequest(format!("invalid batch scrape options: {e}")))?; let (mut template, _decomposed, _tier) = to_internal(template_v2)?; template.url = String::new(); + // Same upfront rejections as /v1/batch/scrape: a fault in the caller's + // own template is one 400 here, not one placeholder document per URL. + crate::state::validate_renderer_pin(template.renderer, template.render_js, &state)?; + crw_crawl::single::validate_scrape_template(&template)?; // Partition URLs into valid / invalid (SSRF-checked, same as v1 scrape). // Validation resolves DNS per URL — run it with bounded concurrency, @@ -174,3 +178,36 @@ pub async fn cancel_batch(state: State, id: Path) -> Result, id: Path) -> Result, AppError> { super::crawl::get_errors(state, id).await } + +#[cfg(test)] +mod tests { + use super::*; + use crate::state::AppState; + use crw_core::config::AppConfig; + use serde_json::json; + + async fn call(body: Value) -> Result { + let config: AppConfig = toml::from_str("").unwrap(); + let state = AppState::new(config).unwrap(); + start_batch(State(state), HeaderMap::new(), Ok(Json(body))) + .await + .map(|Json(r)| r) + } + + /// A template fault is rejected before any URL work on the Firecrawl + /// surface too, instead of surfacing as one placeholder per URL. + #[tokio::test] + async fn v2_start_batch_rejects_screenshot_without_js_before_any_url_work() { + let err = call(json!({ + "urls": ["https://example.com/"], + "formats": ["screenshot"], + "renderJs": false + })) + .await + .unwrap_err(); + match &err.0 { + CrwError::InvalidRequest(msg) => assert!(msg.contains("screenshot"), "got: {msg}"), + other => panic!("expected InvalidRequest, got {other:?}"), + } + } +} diff --git a/crates/crw-server/src/routes/v2/crawl.rs b/crates/crw-server/src/routes/v2/crawl.rs index 5bbcd1d3..e2264ded 100644 --- a/crates/crw-server/src/routes/v2/crawl.rs +++ b/crates/crw-server/src/routes/v2/crawl.rs @@ -260,11 +260,42 @@ pub async fn get_errors( let job = jobs .get(&id) .ok_or_else(|| CrwError::NotFound(format!("Crawl job {id} not found")))?; - let err = job.rx.borrow().error.clone(); - let errors: Vec = err - .into_iter() + // Job-level failure first (the whole job died), then the per-URL ones. A URL + // the engine could not turn into a document is retained as a placeholder + // carrying `block` and no body, and `V2Document` has no field to show the + // block, so without this it would reach the caller as an empty document + // with nothing to explain it while this route, the one documented to carry + // these, said there were no errors at all. An origin error page that was + // kept readable is a delivered document, not an error: listing it here too + // would make a caller retry a URL it already holds. + // + // Each entry gets its own id, the way Firecrawl keys errors per scrape + // rather than per job, so a caller keying them into a map keeps them all. + // The id is the job id plus the document's position, so it is stable + // across polls and a caller deduplicating by id sees each failure once. + let guard = job.rx.borrow(); + let mut errors: Vec = guard + .error + .iter() .map(|e| serde_json::json!({ "id": id.to_string(), "error": e })) .collect(); + errors.extend( + guard + .data + .iter() + .enumerate() + .filter(|(_, d)| !d.has_body()) + .filter_map(|(index, d)| { + d.block.as_ref().map(|b| { + serde_json::json!({ + "id": format!("{id}-{index}"), + "url": d.metadata.source_url, + "error": b.reason, + }) + }) + }), + ); + drop(guard); Ok(Json( serde_json::json!({ "success": true, "errors": errors, "robotsBlocked": [] }), )) @@ -274,6 +305,50 @@ pub async fn get_errors( mod tests { use super::*; + /// A batch URL the engine could not turn into a document is retained with a + /// `block`, but `V2Document` has no field that can show it, so on this + /// surface it is otherwise an empty document with nothing to explain it. + /// This route is the one documented to carry those failures, so it has to. + #[tokio::test] + async fn get_errors_reports_the_per_url_failures_of_a_batch() { + let config: crw_core::config::AppConfig = toml::from_str("").unwrap(); + let state = AppState::new(config).unwrap(); + // `actions` is rejected per URL inside the scrape, which is the shortest + // deterministic way to make one URL fail without touching the network. + let template = crw_core::types::ScrapeRequest { + actions: Some(serde_json::json!([])), + ..Default::default() + }; + let url = "https://example.com/error"; + let id = state + .start_batch_job(vec![url.to_string()], template, None) + .await; + + let mut settled = false; + for _ in 0..100 { + tokio::task::yield_now().await; + let jobs = state.crawl_jobs.read().await; + if jobs.get(&id).unwrap().rx.borrow().status != CrawlStatus::InProgress { + settled = true; + break; + } + } + assert!(settled, "batch job did not settle"); + + let Ok(Json(body)) = get_errors(State(state.clone()), Path(id)).await else { + panic!("errors route should succeed for an existing job"); + }; + let errors = body["errors"].as_array().expect("errors array"); + assert_eq!(errors.len(), 1, "got: {body}"); + assert_eq!(errors[0]["url"], url, "got: {body}"); + assert!( + errors[0]["error"] + .as_str() + .is_some_and(|e| e.contains("actions")), + "got: {body}" + ); + } + /// Regression for #346. `scrapeOptions` is parsed key-by-key out of a raw /// `Value`, so a key nobody reads is silently dropped; `renderJs` was such a /// key and `CrawlRequest.render_js` was hardcoded `None`. A v2 caller then diff --git a/crates/crw-server/src/state.rs b/crates/crw-server/src/state.rs index e54fb855..64163bef 100644 --- a/crates/crw-server/src/state.rs +++ b/crates/crw-server/src/state.rs @@ -5,7 +5,7 @@ use crw_core::types::{ CrawlRequest, CrawlState, CrawlStatus, RequestedRenderer, ScrapeRequest, resolve_pinned_renderer, resolve_render_js, }; -use crw_crawl::crawl::{CrawlOptions, run_crawl}; +use crw_crawl::crawl::{CrawlOptions, failed_page, run_crawl}; use crw_crawl::single::scrape_url; use crw_renderer::FallbackRenderer; use crw_search::SearxngClient; @@ -668,41 +668,40 @@ impl AppState { render_js_default, deadline, ) - .await - .ok(); + .await; // Mutate the shared status in place — push one document // and bump the counter without cloning the whole // accumulated Vec on every completion (avoids O(n^2) // copying on large batches). A failed scrape still // advances `completed`. tx.send_modify(|st| { - if let Some(mut d) = scraped { - // `scrape_url` stamps the verdict but this - // path used to push it through untouched, - // so a wall shipped as an ordinary batch - // document (and `/v2`'s adapter drops - // `block`, hiding it completely). Clear the - // shell and count it, exactly as the single - // scrape route does. - let is_wall = d.block.is_some(); - if !is_wall && let Some(reason) = d.http_error() { - d.block = Some(crw_core::types::BlockOutcome { - vendor: crw_core::types::HTTP_ERROR_VENDOR - .to_string(), - reason, - }); - } - if d.block.is_some() { - // Same split as `/v1/scrape` and the crawl - // loop: a wall loses its shell, an origin - // error page stays readable. - if is_wall { - d.clear_body(); - } - st.blocked += 1; + let mut d = scraped.unwrap_or_else(|err| { + failed_page(&req.url, 0, err.to_string()) + }); + // `scrape_url` stamps the verdict but this + // path used to push it through untouched, + // so a wall shipped as an ordinary batch + // document (and `/v2`'s adapter drops + // `block`, hiding it completely). Clear the + // shell and count it, exactly as the single + // scrape route does. + let is_wall = d.block.is_some(); + if !is_wall && let Some(reason) = d.http_error() { + d.block = Some(crw_core::types::BlockOutcome { + vendor: crw_core::types::HTTP_ERROR_VENDOR.to_string(), + reason, + }); + } + if d.block.is_some() { + // Same split as `/v1/scrape` and the crawl + // loop: a wall loses its shell, an origin + // error page stays readable. + if is_wall { + d.clear_body(); } - st.data.push(d); + st.blocked += 1; } + st.data.push(d); st.completed += 1; // Only flip to Completed from InProgress — never // overwrite a terminal Cancelled set by DELETE. @@ -1542,6 +1541,49 @@ mod tests { assert!(job.data.is_empty()); } + #[tokio::test] + async fn start_batch_job_records_scrape_errors_as_blocked_documents() { + let config: AppConfig = toml::from_str("").unwrap(); + let state = AppState::new(config).unwrap(); + let template = ScrapeRequest { + actions: Some(serde_json::json!([])), + ..Default::default() + }; + let url = "https://example.com/error"; + let id = state + .start_batch_job(vec![url.to_string()], template, None) + .await; + + let mut settled = None; + for _ in 0..100 { + tokio::task::yield_now().await; + let jobs = state.crawl_jobs.read().await; + let job = jobs.get(&id).unwrap().rx.borrow().clone(); + if job.status != CrawlStatus::InProgress { + settled = Some(job); + break; + } + } + let job = settled.expect("batch job did not settle"); + assert_eq!(job.status, CrawlStatus::Completed); + assert_eq!(job.completed, 1); + assert_eq!(job.blocked, 1); + assert_eq!(job.data.len(), 1); + assert_eq!(job.data[0].metadata.source_url, url); + let block = job.data[0] + .block + .as_ref() + .expect("failed URL carries a block"); + assert_eq!(block.vendor, crw_core::types::HTTP_ERROR_VENDOR); + // Substring, not the whole sentence: the wording lives in `crw-crawl` and + // rewording a customer-facing message must not break a `crw-server` test. + assert!( + block.reason.contains("actions"), + "reason should name the rejected parameter, got: {}", + block.reason + ); + } + #[tokio::test] async fn start_extract_job_with_all_preflight_errors_finalizes_without_any_fetch() { let config: AppConfig = toml::from_str("").unwrap(); diff --git a/docs/docs/recipe-batch.md b/docs/docs/recipe-batch.md index 049e812f..83f67a09 100644 --- a/docs/docs/recipe-batch.md +++ b/docs/docs/recipe-batch.md @@ -333,7 +333,8 @@ invalidURLs — URLs that were skipped ``` status — "scraping" | "completed" | "failed" total — total URLs in the job -completed — URLs finished so far +completed : URLs finished so far, including the ones that failed +blocked : URLs that came back a block or an origin error page; never billed creditsUsed — credits consumed so far expiresAt — RFC3339 UTC expiry of this job in server memory next — pagination cursor URL (null when done) @@ -363,11 +364,21 @@ Returns `{ "success": true, "status": "cancelled", "message": "..." }`. ## Checking Errors -URLs that fail mid-job are recorded but don't fail the entire batch. Retrieve them after the job completes: +URLs that fail mid-job are recorded but don't fail the entire batch. Retrieve them at any point during, or after, the job: ```bash curl -s "https://api.fastcrw.com/firecrawl/v2/batch/scrape/$JOB_ID/errors" \ -H "Authorization: Bearer $CRW_API_KEY" ``` -Returns `{ "success": true, "errors": [...], "robotsBlocked": [] }`. +Returns `{ "success": true, "errors": [...], "robotsBlocked": [] }`, with one +entry per failed URL: + +```json +{ "id": "550e8400-...-3", "url": "https://example.com/slow-page", "error": "Target unreachable: Could not reach https://example.com/slow-page" } +``` + +A per-URL entry's `id` is the job id with the document's position appended, so +it is stable across polls. The failed URL is also kept in `data` as a document with no content and the +same reason in `warning`, so the documents across all pages always number +`completed`. diff --git a/docs/docs/v2-api.md b/docs/docs/v2-api.md index 02c25278..6a78825b 100644 --- a/docs/docs/v2-api.md +++ b/docs/docs/v2-api.md @@ -274,13 +274,18 @@ List the IDs of all currently in-progress crawl jobs on this engine instance. ## `GET /firecrawl/v2/crawl/{id}/errors` -Return per-URL errors accumulated during a crawl. +Return the job-level failure, if any, followed by one entry per URL the job +could not turn into a document. Job-level entries carry the job id; per-URL +entries carry the job id with the document's position appended (stable across +polls) and the `url`. Crawl entries appear once the crawl has finished; batch +entries appear as each URL settles. ```json { "success": true, "errors": [ - { "id": "550e8400-...", "error": "fetch timeout for https://example.com/slow-page" } + { "id": "550e8400-...", "error": "Server is overloaded, try again later" }, + { "id": "550e8400-...-3", "url": "https://example.com/slow-page", "error": "Target unreachable: Could not reach https://example.com/slow-page" } ], "robotsBlocked": [] }