From 4efbc01fa1d0f696b173a443801932e610e7611d Mon Sep 17 00:00:00 2001 From: Edgars Date: Wed, 5 Aug 2026 17:15:48 +0100 Subject: [PATCH 1/4] test(web): Add failing repro for webdriver 5xx fatality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RED BY DESIGN — this commit adds no fix. The new test fails on purpose to pin a real defect, so CI on this PR is expected to be red. When our own WebDriver sidecar fails (as opposed to the remote website failing), a 5xx is classified as a fatal error that aborts the whole contract run instead of surfacing as a catchable nondeterministic exception. Three sites combine: 1. webdriver/src/prj/src/index.ts:247 — context.newPage() sits outside the try that begins at :249. newPage() issues CDP Target.createTarget; puppeteer's default 180s protocolTimeout makes it throw ProtocolError, which escapes to the outer handler and becomes HTTP 500. 2. install/config/genvm-web-default.lua — Render calls lib.rs.request with error_on_status = true and no pcall, unlike Request which pcalls and re-raises with fatal = false. 3. implementation/src/scripting/mod.rs:345-348 — non-200 yields ModuleError{ fatal: true }, which becomes a bare anyhow in the executor and surfaces as ResultCode::InternalError. The non-fatal WEBPAGE_LOAD_FAILED branch further down in Render is unreachable for a 5xx, because the fatal raise happens first inside lib.rs.request. Trace logs confirm it: the control test reaches genvm-web-default.lua:36, the 5xx case dies at :19. Two tests, deliberately paired so the red one is not vacuous: - render_remote_page_load_failure_is_not_fatal — sidecar answers 200 + Resulting-Status: 404. PASSES today; proves the non-fatal channel exists and is reachable. - render_sidecar_internal_error_is_not_fatal — sidecar answers a bare 500 with no Resulting-Status, byte-accurate to the outer catch in index.ts. FAILS today. The test asserts only the externally meaningful property (a 5xx from our own sidecar must not be fatal), so a fix at either the lua or the Rust site turns it green without the test encoding which one is chosen. Hermetic: a loopback TcpListener impersonates the sidecar and drives the real production Render lua through the real module wiring. No Chromium, no egress. --- implementation/src/web/mod.rs | 229 ++++++++++++++++++++++++++++++++++ 1 file changed, 229 insertions(+) diff --git a/implementation/src/web/mod.rs b/implementation/src/web/mod.rs index 872e3d0b..38f27253 100644 --- a/implementation/src/web/mod.rs +++ b/implementation/src/web/mod.rs @@ -148,3 +148,232 @@ pub fn entrypoint(args: CliArgs) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + use crate::common::ModuleError; + use genvm_modules_interfaces::web as web_iface; + use mlua::LuaSerdeExt as _; + use tokio::io::AsyncWriteExt as _; + + type TestVM = scripting::UserVM, WebSubContext>; + + /// Absolute path of `/`. Tests run with the crate + /// directory (`implementation`) as the working directory. + fn modules_path(rel: &str) -> String { + let mut path = std::env::current_dir().unwrap(); + path.pop(); + path.push(rel); + path.canonicalize() + .with_context(|| format!("canonicalizing {path:?}")) + .unwrap() + .to_str() + .unwrap() + .to_owned() + } + + /// Stands in for the webdriver sidecar: answers every connection with + /// `head` (status line + headers, `Content-Length` is appended) and `body`. + /// No Chromium and no egress past loopback. + async fn serve_sidecar(head: &'static str, body: &'static [u8]) -> std::net::SocketAddr { + let server = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + + tokio::spawn(async move { + while let Ok((mut client, _)) = server.accept().await { + let response = format!("{head}Content-Length: {}\r\n\r\n", body.len()); + client.write_all(response.as_bytes()).await.unwrap(); + client.write_all(body).await.unwrap(); + client.shutdown().await.unwrap(); + } + }); + + addr + } + + /// The sidecar's success path: `200` with the *page's* status reported in + /// `Resulting-Status`. A page that 404s looks like this. + async fn serve_sidecar_page_not_found() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 404\r\n", + b"Not Found", + ) + .await + } + + /// The sidecar's outer `catch` (`modules/webdriver/src/prj/src/index.ts`): a + /// bare `500` with no `Resulting-Status`, which is what an exception thrown + /// *outside* the per-page `try` becomes — e.g. `newPage()` exceeding + /// puppeteer's `protocolTimeout` on `Target.createTarget`. + async fn serve_sidecar_internal_error() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n", + br#"{"error":"Internal server error","message":"Target.createTarget timed out"}"#, + ) + .await + } + + fn test_config(webdriver_host: String) -> config::Config { + config::Config { + webdriver_host, + extra_tld: Vec::new(), + always_allow_hosts: Vec::new(), + meta: serde_json::Value::Null, + max_wait_after_loaded: common::Timeout::from_secs(60), + base: BaseConfig { + threads: 0, + blocking_threads: 0, + log_level: logger::Level::Trace, + log_disable: String::new(), + }, + mod_base: common::ModuleBaseConfig { + bind_address: None, + vm_count: 1, + lua_script_path: modules_path("install/config/genvm-web-default.lua"), + lua_path: format!("{}/?.lua", modules_path("install/lib/genvm-lua")), + signer_headers: Arc::new(std::collections::BTreeMap::new()), + signer_url: Arc::from(""), + data_dir: String::new(), + }, + } + } + + /// Same VM wiring as [`create_web_module`], without the pool. + async fn create_test_vm(config: &sync::DArc) -> TestVM { + let config_for_data = config.clone(); + + scripting::UserVM::create( + &config.mod_base, + move |vm: mlua::Lua| async move { + vm.globals() + .set("__web", ctx::create_global(&vm, &config_for_data)?)?; + + scripting::load_script(&vm, &config_for_data.mod_base.lua_script_path).await?; + + let render: mlua::Function = vm.globals().get("Render")?; + let request: mlua::Function = vm.globals().get("Request")?; + + Ok(ctx::VMData { render, request }) + }, + Box::new( + move |vm: &mlua::Lua, table: &mlua::Table, sub_ctx: &sync::DArc| { + let scripting = sub_ctx.gep(|x| &x.scripting); + scripting::setup_lua_default_ctx(scripting, vm, table)?; + + let ctx = Arc::new(ctx::CtxPart {}); + table.set("__ctx_web", vm.create_userdata(ctx.clone())?)?; + + Ok(ctx) + }, + ), + ) + .await + .unwrap() + } + + /// Same context as `HandlerProvider::create_execution_context`, including + /// `filter_dns = true` — the webdriver host is an IP literal, which reqwest + /// never sends through the resolver, so the SSRF guard stays out of the way. + fn create_test_ctx(config: &sync::DArc) -> sync::DArc { + let hello = common::tests::get_hello(); + let metrics = sync::DArc::new(Metrics::default()); + + let scripting = scripting::create_ctx_part( + &hello, + &config.gep(|x| &x.mod_base), + metrics.gep(|x| &x.scripting), + true, + ) + .unwrap(); + + sync::DArc::new(WebSubContext { scripting }) + } + + /// Drives the production `Render` of + /// `modules/install/config/genvm-web-default.lua`, the way `Handler::handle` + /// does for `Message::Render`. + async fn render( + config: &sync::DArc, + url: &str, + ) -> anyhow::Result { + let user_vm = create_test_vm(config).await; + let sub_ctx = create_test_ctx(config); + let (_ctx, ctx_val) = user_vm.create_ctx(&sub_ctx)?; + + let payload = user_vm.vm.create_table()?; + // what `RenderMode::Text` serializes to + payload.set("mode", "text")?; + payload.set("url", url)?; + payload.set("wait_after_loaded", 0.0)?; + payload.set("size_limit", 1024 * 1024)?; + + let res: mlua::Value = user_vm + .call_fn(&user_vm.data.render, (ctx_val, payload)) + .await?; + + Ok(user_vm.vm.from_value(res)?) + } + + /// The module error `Render` failed with. Panics if it succeeded, or if the + /// failure was not a [`ModuleError`]. + async fn render_err(config: &sync::DArc, url: &str) -> ModuleError { + let err = render(config, url) + .await + .err() + .expect("expected Render to fail"); + + log_info!(error:ah = &err; "render failed"); + + scripting::try_unwrap_any_err(err).expect("expected a module error") + } + + // ── a failing sidecar is an environment fault, not a verdict ────────── + + /// Control. When the *page* fails, the sidecar still answers `200` and puts + /// the page status in `Resulting-Status`; `Render` turns that into a + /// non-fatal `WEBPAGE_LOAD_FAILED`, which the executor hands the contract as + /// a catchable nondeterministic exception. + #[tokio::test] + async fn render_remote_page_load_failure_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_page_not_found().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!(!err.fatal, "page load failure must be non-fatal: {err:?}"); + } + + /// A 5xx from our *own* webdriver sidecar says nothing about the page — it + /// is this node's environment failing. It must travel the same non-fatal + /// channel as the control above instead of aborting the whole contract run + /// with an internal error. + /// + /// It does not today: `Render` calls `lib.rs.request` with + /// `error_on_status = true` and, unlike `Request`, without a `pcall`, so + /// `send_request_get_lua_compatible_response_bytes` raises a **fatal** + /// `STATUS_NOT_OK` and the `WEBPAGE_LOAD_FAILED` branch below it is never + /// reached. + #[tokio::test] + async fn render_sidecar_internal_error_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_internal_error().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + !err.fatal, + "a 5xx from our own webdriver sidecar must be non-fatal, got {err:?}" + ); + } +} From 563f2b484e99107fc25e8d82b9577b9e7969c6fd Mon Sep 17 00:00:00 2001 From: Edgars Date: Mon, 10 Aug 2026 13:44:46 +0100 Subject: [PATCH 2/4] =?UTF-8?q?fix(web):=20Treat=20sidecar=20failures=20as?= =?UTF-8?q?=20non-fatal,=20and=20test=20the=20path=20=F0=9F=90=9B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the repro this branch opened with. A failure of our OWN webdriver sidecar was classified fatal, aborting the whole contract run as an internal error the contract could not catch. Downstream the node turns any GenVM error into a Timeout vote, so an infrastructure blip became a consensus verdict. Enforcement point: the Lua Render call site, not the Rust STATUS_NOT_OK sites. Reasoning, since the alternative looks tempting: - error_on_status is a request-SHAPE flag ("raise instead of returning the response"), not a trust flag. The generic helpers are handed a URL and cannot tell our sidecar from a site the contract named, so fatality — a trust-domain question — is the wrong thing to answer there. - The other callers are not the sidecar. Production callers passing error_on_status = true are Render and the nine LLM providers. The contract-facing Request does NOT set the flag, so an arbitrary contract URL returning 500 already comes back as an ordinary Response with a status and never reaches STATUS_NOT_OK. Flipping the Rust default would have silently changed LLM-provider semantics, which feed retry and backend selection. - The pcall's lexical extent IS the trust boundary: no contract-controlled URL sits inside it, so the distinction is structural rather than heuristic. web.check_url and the WEBPAGE_LOAD_FAILED branch stay outside. - It also covers more than STATUS_NOT_OK. A sidecar that is gone fails earlier as a fatal SENDING_REQUEST, and a truncated body as fatal READING_BODY — the same fault class. The pcall covers the whole hop. Sidecar side, as the better classification rather than only a softer one: createBrowserContext() and newPage() move inside an error scope, and any failure there returns 200 + Resulting-Status: 503 instead of escaping to the outer catch as HTTP 500. That puts it on the already-correct WEBPAGE_LOAD_FAILED path with the status visible in ctx. All setup failures are mapped, not only ProtocolError/TimeoutError: the target URL is never passed to those calls, so by construction nothing there is an observation about the page. protocolTimeout is now explicit at 90s (env GVM_WEBDRIVER_PROTOCOL_TIMEOUT). Correcting the assumption this work started from: puppeteer-core 24.16.0 defaults to 180s and base_client_builder is 300s, so that pair was never inverted. What is inverted is 180s against the render budget — 30s navigation plus at most 60s waitAfterLoaded is about 90s — so a wedged CDP command was noticed roughly 90s after the render should have ended. The 90s value is a reasoned choice, not a measured one. Coverage the repro left open, now closed: - The webdriver TypeScript had never executed. It has no test runner, and importing index.ts starts an HTTP server and launches Chromium at module scope. The render logic moves to src/render.ts importing puppeteer types only; index.ts keeps the server and handlers. Tests use node:test with zero new dependencies, live outside src/ so tsc does not emit them, and run in the Dockerfile, which is the only CI path touching this code. - The downstream half had no seam. The fatal-to-wire branch is extracted from the private, stream-generic loop_one_inner into module_error_to_wire, and pinned through a calldata round-trip. - The sidecar's other non-200 exits: 400 bad params, 503 healthcheck, plus connection-refused, which is the same fault class and was also fatal. - The JSON twin at scripting/mod.rs is now covered at error_on_status true and false. It is COVERED, not changed: Render never sets json, so the twin has no sidecar caller and its only such callers are the LLM providers. Every new test was red-checked by reverting each fix in turn. Reverting the Lua pcall: 5 failures, controls green. Reverting the TypeScript mapping: 3 failures, 2 controls green. Rust: 115 passed, 0 failed across 16 binaries. TypeScript: 5/5. tsc --noEmit and cargo fmt --check clean. Two consequences worth a reviewer's attention rather than burying: A validator whose own sidecar is broken now computes a result (a catchable NondetException) and votes disagree, rather than voting Timeout. That may be worse for that validator than abstaining. The repo already made this choice for WEBPAGE_LOAD_FAILED and for Request's blanket reraise(false); this follows it rather than inventing a third classification. The re-raise keeps causes = ["STATUS_NOT_OK"], so downstream cannot tell a sidecar status error from any other. A distinct cause such as WEBDRIVER_UNAVAILABLE would help ops, but reraise_with_fatality does not support one and contract-visible vocabulary was not invented here. Not covered in this repo: the last hop, FatalError to bare anyhow to ResultCode::InternalError, lives in the executors/v0.3.x submodule and testing it there needs a gitlink bump and an executor build, which the repo's macos guidance rules out natively on Darwin. Documented with file references on module_error_to_wire instead. --- implementation/src/common/mod.rs | 156 ++++++- implementation/src/web/mod.rs | 228 +--------- implementation/src/web/tests.rs | 398 ++++++++++++++++++ .../tests/request_status_fatality.rs | 222 ++++++++++ install/config/genvm-web-default.lua | 16 +- webdriver/Dockerfile | 1 + webdriver/src/prj/package.json | 3 +- webdriver/src/prj/src/browser/chrome.ts | 15 + webdriver/src/prj/src/index.ts | 281 +------------ webdriver/src/prj/src/render.ts | 336 +++++++++++++++ webdriver/src/prj/test/render.test.ts | 150 +++++++ 11 files changed, 1281 insertions(+), 525 deletions(-) create mode 100644 implementation/src/web/tests.rs create mode 100644 implementation/tests/request_status_fatality.rs create mode 100644 webdriver/src/prj/src/render.ts create mode 100644 webdriver/src/prj/test/render.test.ts diff --git a/implementation/src/common/mod.rs b/implementation/src/common/mod.rs index c9709634..416ad628 100644 --- a/implementation/src/common/mod.rs +++ b/implementation/src/common/mod.rs @@ -243,6 +243,49 @@ where .with_context(|| "handling with handler") } +/// Classify a handler failure into the wire result the executor reads. +/// +/// This is the only place the `fatal` flag is spent, and the two outcomes are +/// not interchangeable: +/// +/// 1. `FatalError` is turned into a bare `anyhow` by the executor +/// (`executors/v0.3.x/executor/src/modules.rs`, the `Result::FatalError` +/// arm), which lands as `ErrorKind::Internal` in `rt/errors.rs` and finally +/// as `ResultCode::InternalError` in `host/mod.rs`. It has no `RunOk` +/// representation, so the whole contract run is aborted and the contract +/// cannot catch it. Downstream, a node turns any GenVM error into a Timeout +/// vote +/// 2. `UserError` is handed to the runner as `{"error": …}`, which +/// `genlayer-py-std` raises as a catchable `NondetException` +/// +/// An error that is not a [`ModuleError`] is fatal, so "not a `ModuleError`" +/// and "`fatal: true`" are indistinguishable past this point +pub(crate) fn module_error_to_wire( + err: anyhow::Error, + genvm_id: genvm_modules_interfaces::GenVMId, +) -> genvm_modules_interfaces::Result { + match scripting::try_unwrap_any_err(err) { + Ok(err) => { + if err.fatal { + genvm_modules_interfaces::Result::FatalError(format!("{err:#}")) + } else { + let res = GenericValue::Map(BTreeMap::from([ + ( + "causes".to_owned(), + GenericValue::Array(err.causes.into_iter().map(Into::into).collect()), + ), + ("ctx".to_owned(), GenericValue::Map(err.ctx)), + ])); + genvm_modules_interfaces::Result::UserError(res) + } + } + Err(err) => { + log_error_into!(&LoggerWithId, error:ah = &err, genvm_id:id = genvm_id.0; "handler fatal error"); + genvm_modules_interfaces::Result::FatalError(format!("{err:#}")) + } + } +} + async fn loop_one_inner( handler: &mut impl MessageHandler, stream: &mut S, @@ -265,28 +308,7 @@ where let res = loop_one_inner_handle(handler, &data).await; let res = match res { Ok(res) => genvm_modules_interfaces::Result::Ok(res), - Err(err) => match scripting::try_unwrap_any_err(err) { - Ok(err) => { - if err.fatal { - genvm_modules_interfaces::Result::FatalError(format!("{err:#}")) - } else { - let res = GenericValue::Map(BTreeMap::from([ - ( - "causes".to_owned(), - GenericValue::Array( - err.causes.into_iter().map(Into::into).collect(), - ), - ), - ("ctx".to_owned(), GenericValue::Map(err.ctx)), - ])); - genvm_modules_interfaces::Result::UserError(res) - } - } - Err(err) => { - log_error_into!(&LoggerWithId, error:ah = &err, genvm_id:id = genvm_id.0; "handler fatal error"); - genvm_modules_interfaces::Result::FatalError(format!("{err:#}")) - } - }, + Err(err) => module_error_to_wire(err, genvm_id), }; let message = calldata::encode_obj(&res); @@ -1159,3 +1181,93 @@ mod ip_filter_tests { } } } + +#[cfg(test)] +mod wire_result_tests { + use super::*; + + use genvm_modules_interfaces::Result as WireResult; + + const ID: genvm_modules_interfaces::GenVMId = genvm_modules_interfaces::GenVMId(7); + + fn module_error(fatal: bool) -> anyhow::Error { + ModuleError { + causes: vec![ErrorKind::STATUS_NOT_OK.into()], + fatal, + ctx: BTreeMap::from([("status".to_owned(), GenericValue::Number(500.0))]), + } + .into() + } + + /// The classification survives calldata encoding, which is what the executor + /// actually reads off the socket + fn roundtrip(res: WireResult) -> WireResult { + let bytes = calldata::encode_obj(&res); + calldata::decode_obj(&bytes).unwrap() + } + + // -- fatal: the run is aborted, the contract cannot catch it -------- + + #[test] + fn fatal_module_error_becomes_fatal_error() { + let res: WireResult = module_error_to_wire(module_error(true), ID); + + match roundtrip(res) { + WireResult::FatalError(msg) => { + assert!(msg.contains("STATUS_NOT_OK"), "unexpected message: {msg}") + } + WireResult::Ok(_) | WireResult::UserError(_) => { + panic!("a fatal module error must become FatalError") + } + } + } + + /// An error that never was a [`ModuleError`] carries no fatality flag at + /// all, and is treated as fatal + #[test] + fn plain_error_becomes_fatal_error() { + let res: WireResult = + module_error_to_wire(anyhow::anyhow!("something unclassified"), ID); + + match roundtrip(res) { + WireResult::FatalError(msg) => assert!( + msg.contains("something unclassified"), + "unexpected message: {msg}" + ), + WireResult::Ok(_) | WireResult::UserError(_) => { + panic!("an unclassified error must become FatalError") + } + } + } + + // -- non-fatal: a catchable NondetException for the contract --------- + + #[test] + fn non_fatal_module_error_becomes_user_error() { + let res: WireResult = module_error_to_wire(module_error(false), ID); + + let WireResult::UserError(value) = roundtrip(res) else { + panic!("a non-fatal module error must become UserError") + }; + + let GenericValue::Map(map) = value else { + panic!("expected a map, got {value:?}") + }; + + let Some(GenericValue::Array(causes)) = map.get("causes") else { + panic!("expected an array of causes in {map:?}") + }; + let [GenericValue::Str(cause)] = &causes[..] else { + panic!("expected exactly one cause, got {causes:?}") + }; + assert_eq!(cause, "STATUS_NOT_OK"); + + let Some(GenericValue::Map(ctx)) = map.get("ctx") else { + panic!("expected a ctx map in {map:?}") + }; + let Some(GenericValue::Number(status)) = ctx.get("status") else { + panic!("expected a numeric status in {ctx:?}") + }; + assert_eq!(*status, 500.0); + } +} diff --git a/implementation/src/web/mod.rs b/implementation/src/web/mod.rs index 38f27253..301fee08 100644 --- a/implementation/src/web/mod.rs +++ b/implementation/src/web/mod.rs @@ -150,230 +150,4 @@ pub fn entrypoint(args: CliArgs) -> Result<()> { } #[cfg(test)] -mod tests { - use super::*; - - use crate::common::ModuleError; - use genvm_modules_interfaces::web as web_iface; - use mlua::LuaSerdeExt as _; - use tokio::io::AsyncWriteExt as _; - - type TestVM = scripting::UserVM, WebSubContext>; - - /// Absolute path of `/`. Tests run with the crate - /// directory (`implementation`) as the working directory. - fn modules_path(rel: &str) -> String { - let mut path = std::env::current_dir().unwrap(); - path.pop(); - path.push(rel); - path.canonicalize() - .with_context(|| format!("canonicalizing {path:?}")) - .unwrap() - .to_str() - .unwrap() - .to_owned() - } - - /// Stands in for the webdriver sidecar: answers every connection with - /// `head` (status line + headers, `Content-Length` is appended) and `body`. - /// No Chromium and no egress past loopback. - async fn serve_sidecar(head: &'static str, body: &'static [u8]) -> std::net::SocketAddr { - let server = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let addr = server.local_addr().unwrap(); - - tokio::spawn(async move { - while let Ok((mut client, _)) = server.accept().await { - let response = format!("{head}Content-Length: {}\r\n\r\n", body.len()); - client.write_all(response.as_bytes()).await.unwrap(); - client.write_all(body).await.unwrap(); - client.shutdown().await.unwrap(); - } - }); - - addr - } - - /// The sidecar's success path: `200` with the *page's* status reported in - /// `Resulting-Status`. A page that 404s looks like this. - async fn serve_sidecar_page_not_found() -> std::net::SocketAddr { - serve_sidecar( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 404\r\n", - b"Not Found", - ) - .await - } - - /// The sidecar's outer `catch` (`modules/webdriver/src/prj/src/index.ts`): a - /// bare `500` with no `Resulting-Status`, which is what an exception thrown - /// *outside* the per-page `try` becomes — e.g. `newPage()` exceeding - /// puppeteer's `protocolTimeout` on `Target.createTarget`. - async fn serve_sidecar_internal_error() -> std::net::SocketAddr { - serve_sidecar( - "HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n", - br#"{"error":"Internal server error","message":"Target.createTarget timed out"}"#, - ) - .await - } - - fn test_config(webdriver_host: String) -> config::Config { - config::Config { - webdriver_host, - extra_tld: Vec::new(), - always_allow_hosts: Vec::new(), - meta: serde_json::Value::Null, - max_wait_after_loaded: common::Timeout::from_secs(60), - base: BaseConfig { - threads: 0, - blocking_threads: 0, - log_level: logger::Level::Trace, - log_disable: String::new(), - }, - mod_base: common::ModuleBaseConfig { - bind_address: None, - vm_count: 1, - lua_script_path: modules_path("install/config/genvm-web-default.lua"), - lua_path: format!("{}/?.lua", modules_path("install/lib/genvm-lua")), - signer_headers: Arc::new(std::collections::BTreeMap::new()), - signer_url: Arc::from(""), - data_dir: String::new(), - }, - } - } - - /// Same VM wiring as [`create_web_module`], without the pool. - async fn create_test_vm(config: &sync::DArc) -> TestVM { - let config_for_data = config.clone(); - - scripting::UserVM::create( - &config.mod_base, - move |vm: mlua::Lua| async move { - vm.globals() - .set("__web", ctx::create_global(&vm, &config_for_data)?)?; - - scripting::load_script(&vm, &config_for_data.mod_base.lua_script_path).await?; - - let render: mlua::Function = vm.globals().get("Render")?; - let request: mlua::Function = vm.globals().get("Request")?; - - Ok(ctx::VMData { render, request }) - }, - Box::new( - move |vm: &mlua::Lua, table: &mlua::Table, sub_ctx: &sync::DArc| { - let scripting = sub_ctx.gep(|x| &x.scripting); - scripting::setup_lua_default_ctx(scripting, vm, table)?; - - let ctx = Arc::new(ctx::CtxPart {}); - table.set("__ctx_web", vm.create_userdata(ctx.clone())?)?; - - Ok(ctx) - }, - ), - ) - .await - .unwrap() - } - - /// Same context as `HandlerProvider::create_execution_context`, including - /// `filter_dns = true` — the webdriver host is an IP literal, which reqwest - /// never sends through the resolver, so the SSRF guard stays out of the way. - fn create_test_ctx(config: &sync::DArc) -> sync::DArc { - let hello = common::tests::get_hello(); - let metrics = sync::DArc::new(Metrics::default()); - - let scripting = scripting::create_ctx_part( - &hello, - &config.gep(|x| &x.mod_base), - metrics.gep(|x| &x.scripting), - true, - ) - .unwrap(); - - sync::DArc::new(WebSubContext { scripting }) - } - - /// Drives the production `Render` of - /// `modules/install/config/genvm-web-default.lua`, the way `Handler::handle` - /// does for `Message::Render`. - async fn render( - config: &sync::DArc, - url: &str, - ) -> anyhow::Result { - let user_vm = create_test_vm(config).await; - let sub_ctx = create_test_ctx(config); - let (_ctx, ctx_val) = user_vm.create_ctx(&sub_ctx)?; - - let payload = user_vm.vm.create_table()?; - // what `RenderMode::Text` serializes to - payload.set("mode", "text")?; - payload.set("url", url)?; - payload.set("wait_after_loaded", 0.0)?; - payload.set("size_limit", 1024 * 1024)?; - - let res: mlua::Value = user_vm - .call_fn(&user_vm.data.render, (ctx_val, payload)) - .await?; - - Ok(user_vm.vm.from_value(res)?) - } - - /// The module error `Render` failed with. Panics if it succeeded, or if the - /// failure was not a [`ModuleError`]. - async fn render_err(config: &sync::DArc, url: &str) -> ModuleError { - let err = render(config, url) - .await - .err() - .expect("expected Render to fail"); - - log_info!(error:ah = &err; "render failed"); - - scripting::try_unwrap_any_err(err).expect("expected a module error") - } - - // ── a failing sidecar is an environment fault, not a verdict ────────── - - /// Control. When the *page* fails, the sidecar still answers `200` and puts - /// the page status in `Resulting-Status`; `Render` turns that into a - /// non-fatal `WEBPAGE_LOAD_FAILED`, which the executor hands the contract as - /// a catchable nondeterministic exception. - #[tokio::test] - async fn render_remote_page_load_failure_is_not_fatal() { - common::tests::setup(); - - let addr = serve_sidecar_page_not_found().await; - let config = sync::DArc::new(test_config(format!("http://{addr}"))); - - let err = render_err(&config, "https://example.com/").await; - - assert!( - err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), - "unexpected causes: {:?}", - err.causes - ); - assert!(!err.fatal, "page load failure must be non-fatal: {err:?}"); - } - - /// A 5xx from our *own* webdriver sidecar says nothing about the page — it - /// is this node's environment failing. It must travel the same non-fatal - /// channel as the control above instead of aborting the whole contract run - /// with an internal error. - /// - /// It does not today: `Render` calls `lib.rs.request` with - /// `error_on_status = true` and, unlike `Request`, without a `pcall`, so - /// `send_request_get_lua_compatible_response_bytes` raises a **fatal** - /// `STATUS_NOT_OK` and the `WEBPAGE_LOAD_FAILED` branch below it is never - /// reached. - #[tokio::test] - async fn render_sidecar_internal_error_is_not_fatal() { - common::tests::setup(); - - let addr = serve_sidecar_internal_error().await; - let config = sync::DArc::new(test_config(format!("http://{addr}"))); - - let err = render_err(&config, "https://example.com/").await; - - assert!( - !err.fatal, - "a 5xx from our own webdriver sidecar must be non-fatal, got {err:?}" - ); - } -} +mod tests; diff --git a/implementation/src/web/tests.rs b/implementation/src/web/tests.rs new file mode 100644 index 00000000..2fa38bb0 --- /dev/null +++ b/implementation/src/web/tests.rs @@ -0,0 +1,398 @@ +use super::*; + +use crate::common::ModuleError; +use genvm_modules_interfaces::web as web_iface; +use mlua::LuaSerdeExt as _; +use tokio::io::AsyncWriteExt as _; + +type TestVM = scripting::UserVM, WebSubContext>; + +/// Absolute path of `/`. Tests run with the crate +/// directory (`implementation`) as the working directory. +fn modules_path(rel: &str) -> String { + let mut path = std::env::current_dir().unwrap(); + path.pop(); + path.push(rel); + path.canonicalize() + .with_context(|| format!("canonicalizing {path:?}")) + .unwrap() + .to_str() + .unwrap() + .to_owned() +} + +/// Stands in for the webdriver sidecar: answers every connection with +/// `head` (status line + headers, `Content-Length` is appended) and `body`. +/// No Chromium and no egress past loopback. +async fn serve_sidecar(head: &'static str, body: &'static [u8]) -> std::net::SocketAddr { + let server = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + + tokio::spawn(async move { + while let Ok((mut client, _)) = server.accept().await { + let response = format!("{head}Content-Length: {}\r\n\r\n", body.len()); + client.write_all(response.as_bytes()).await.unwrap(); + client.write_all(body).await.unwrap(); + client.shutdown().await.unwrap(); + } + }); + + addr +} + +/// The sidecar's success path: `200` with the *page's* status reported in +/// `Resulting-Status`. A page that 404s looks like this. +async fn serve_sidecar_page_not_found() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 404\r\n", + b"Not Found", + ) + .await +} + +/// The sidecar's outer `catch` (`webdriver/src/prj/src/index.ts`, +/// `handleRenderRequest`): a bare `500` with no `Resulting-Status`, which is +/// what an exception thrown *outside* the per-page `try` becomes — e.g. +/// `newPage()` exceeding puppeteer's `protocolTimeout` on +/// `Target.createTarget`. +async fn serve_sidecar_internal_error() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n", + br#"{"error":"Internal server error","message":"Target.createTarget timed out"}"#, + ) + .await +} + +/// The sidecar's parameter validation (`handleRenderRequest`): a `400` when +/// `url` is missing or `mode` is not one of text/html/screenshot. Reachable +/// whenever the module and the sidecar disagree about the query format, e.g. +/// across a version skew +async fn serve_sidecar_bad_request() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\n", + br#"{"error":"Missing url parameter"}"#, + ) + .await +} + +/// The sidecar's healthcheck path (`handleHealthcheck`): a plain-text `503` +/// when its own probe render fails. A proxy or orchestrator in front of the +/// sidecar answers the same way while the sidecar is down +async fn serve_sidecar_unhealthy() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 503 Service Unavailable\r\nContent-Type: text/plain\r\n", + b"unhealthy", + ) + .await +} + +/// An address nothing listens on: the sidecar process is gone, so the +/// connection is refused before any status exists. Binds and drops, so the +/// port is known to have been free +async fn sidecar_down() -> std::net::SocketAddr { + let server = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + std::mem::drop(server); + addr +} + +fn test_config(webdriver_host: String) -> config::Config { + config::Config { + webdriver_host, + extra_tld: Vec::new(), + always_allow_hosts: Vec::new(), + meta: serde_json::Value::Null, + max_wait_after_loaded: common::Timeout::from_secs(60), + base: BaseConfig { + threads: 0, + blocking_threads: 0, + log_level: logger::Level::Trace, + log_disable: String::new(), + }, + mod_base: common::ModuleBaseConfig { + bind_address: None, + vm_count: 1, + lua_script_path: modules_path("install/config/genvm-web-default.lua"), + lua_path: format!("{}/?.lua", modules_path("install/lib/genvm-lua")), + signer_headers: Arc::new(std::collections::BTreeMap::new()), + signer_url: Arc::from(""), + data_dir: String::new(), + }, + } +} + +/// Same VM wiring as [`create_web_module`], without the pool. +async fn create_test_vm(config: &sync::DArc) -> TestVM { + let config_for_data = config.clone(); + + scripting::UserVM::create( + &config.mod_base, + move |vm: mlua::Lua| async move { + vm.globals() + .set("__web", ctx::create_global(&vm, &config_for_data)?)?; + + scripting::load_script(&vm, &config_for_data.mod_base.lua_script_path).await?; + + let render: mlua::Function = vm.globals().get("Render")?; + let request: mlua::Function = vm.globals().get("Request")?; + + Ok(ctx::VMData { render, request }) + }, + Box::new( + move |vm: &mlua::Lua, table: &mlua::Table, sub_ctx: &sync::DArc| { + let scripting = sub_ctx.gep(|x| &x.scripting); + scripting::setup_lua_default_ctx(scripting, vm, table)?; + + let ctx = Arc::new(ctx::CtxPart {}); + table.set("__ctx_web", vm.create_userdata(ctx.clone())?)?; + + Ok(ctx) + }, + ), + ) + .await + .unwrap() +} + +/// Same context as `HandlerProvider::create_execution_context`, including +/// `filter_dns = true` — the webdriver host is an IP literal, which reqwest +/// never sends through the resolver, so the SSRF guard stays out of the way. +fn create_test_ctx(config: &sync::DArc) -> sync::DArc { + let hello = common::tests::get_hello(); + let metrics = sync::DArc::new(Metrics::default()); + + let scripting = scripting::create_ctx_part( + &hello, + &config.gep(|x| &x.mod_base), + metrics.gep(|x| &x.scripting), + true, + ) + .unwrap(); + + sync::DArc::new(WebSubContext { scripting }) +} + +/// Drives the production `Render` of +/// `modules/install/config/genvm-web-default.lua`, the way `Handler::handle` +/// does for `Message::Render`. +async fn render( + config: &sync::DArc, + url: &str, +) -> anyhow::Result { + let user_vm = create_test_vm(config).await; + let sub_ctx = create_test_ctx(config); + let (_ctx, ctx_val) = user_vm.create_ctx(&sub_ctx)?; + + let payload = user_vm.vm.create_table()?; + // what `RenderMode::Text` serializes to + payload.set("mode", "text")?; + payload.set("url", url)?; + payload.set("wait_after_loaded", 0.0)?; + payload.set("size_limit", 1024 * 1024)?; + + let res: mlua::Value = user_vm + .call_fn(&user_vm.data.render, (ctx_val, payload)) + .await?; + + Ok(user_vm.vm.from_value(res)?) +} + +/// The error `Render` failed with, still wrapped, the way `Handler::handle` +/// hands it to the message loop. Panics if `Render` succeeded. +async fn render_raw_err(config: &sync::DArc, url: &str) -> anyhow::Error { + let err = render(config, url) + .await + .err() + .expect("expected Render to fail"); + + log_info!(error:ah = &err; "render failed"); + + err +} + +/// The module error `Render` failed with. Panics if it succeeded, or if the +/// failure was not a [`ModuleError`]. +async fn render_err(config: &sync::DArc, url: &str) -> ModuleError { + let err = render_raw_err(config, url).await; + + scripting::try_unwrap_any_err(err).expect("expected a module error") +} + +/// Asserts that talking to the sidecar described by `addr` fails non-fatally, +/// i.e. as something the contract can catch rather than as an internal error +async fn assert_sidecar_failure_is_not_fatal( + addr: std::net::SocketAddr, + what: &str, +) -> ModuleError { + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + !err.fatal, + "{what} from our own webdriver sidecar must be non-fatal, got {err:?}" + ); + + err +} + +// ── a failing sidecar is an environment fault, not a verdict ────────── + +/// Control. When the *page* fails, the sidecar still answers `200` and puts +/// the page status in `Resulting-Status`; `Render` turns that into a +/// non-fatal `WEBPAGE_LOAD_FAILED`, which the executor hands the contract as +/// a catchable nondeterministic exception. +#[tokio::test] +async fn render_remote_page_load_failure_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_page_not_found().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!(!err.fatal, "page load failure must be non-fatal: {err:?}"); +} + +/// A 5xx from our *own* webdriver sidecar says nothing about the page — it +/// is this node's environment failing. It must travel the same non-fatal +/// channel as the control above instead of aborting the whole contract run +/// with an internal error. +/// +/// `send_request_get_lua_compatible_response_bytes` raises `STATUS_NOT_OK` +/// as fatal, because it sees only a URL and cannot tell our sidecar from a +/// contract-controlled site. `Render` is the caller that does know, so it +/// wraps the sidecar hop in a `pcall` and re-raises non-fatally, the way +/// `Request` already did +#[tokio::test] +async fn render_sidecar_internal_error_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_internal_error().await; + let err = assert_sidecar_failure_is_not_fatal(addr, "a 5xx").await; + + // Not vacuous: the failure really is the status error raised on the + // sidecar hop, not the `WEBPAGE_LOAD_FAILED` branch further down, which + // this response never reaches + assert!( + err.causes.contains(&"STATUS_NOT_OK".to_owned()), + "unexpected causes: {:?}", + err.causes + ); +} + +/// The sidecar's other non-200 exits take the same branch, and are the same +/// kind of fault: a `400` means the module and the sidecar disagree about the +/// query format, which is a deployment problem, not a statement about the page +#[tokio::test] +async fn render_sidecar_bad_request_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_bad_request().await; + let err = assert_sidecar_failure_is_not_fatal(addr, "a 400").await; + + assert!( + err.causes.contains(&"STATUS_NOT_OK".to_owned()), + "unexpected causes: {:?}", + err.causes + ); +} + +/// A `503`, as answered by the sidecar's healthcheck path or by whatever +/// proxies it while it is down +#[tokio::test] +async fn render_sidecar_unhealthy_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_unhealthy().await; + let err = assert_sidecar_failure_is_not_fatal(addr, "a 503").await; + + assert!( + err.causes.contains(&"STATUS_NOT_OK".to_owned()), + "unexpected causes: {:?}", + err.causes + ); +} + +/// The sidecar being *gone* is the same class of fault as it answering badly, +/// and it arrives on a different path: `map_send_error` raises a fatal +/// `SENDING_REQUEST` before any status exists. The `pcall` covers the whole +/// hop, so this is non-fatal too +#[tokio::test] +async fn render_sidecar_unreachable_is_not_fatal() { + common::tests::setup(); + + let addr = sidecar_down().await; + let err = assert_sidecar_failure_is_not_fatal(addr, "a refused connection").await; + + assert!( + err.causes.contains(&"SENDING_REQUEST".to_owned()), + "unexpected causes: {:?}", + err.causes + ); +} + +// ── the classification as the executor reads it ────────────────────── + +/// The end the property is really about. `ModuleError.fatal` is only a flag +/// until [`crate::common::module_error_to_wire`] spends it: `FatalError` is +/// what the executor turns into a bare `anyhow` and finally +/// `ResultCode::InternalError`, which the contract cannot catch and which a +/// node reports as a Timeout vote. A sidecar 5xx must not take that path +#[tokio::test] +async fn render_sidecar_internal_error_reaches_the_wire_as_user_error() { + common::tests::setup(); + + let addr = serve_sidecar_internal_error().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_raw_err(&config, "https://example.com/").await; + let wire: genvm_modules_interfaces::Result = + crate::common::module_error_to_wire(err, common::tests::get_hello().genvm_id); + + match wire { + genvm_modules_interfaces::Result::UserError(_) => {} + genvm_modules_interfaces::Result::FatalError(msg) => { + panic!("a sidecar 5xx must not abort the run as an internal error: {msg}") + } + genvm_modules_interfaces::Result::Ok(_) => panic!("expected Render to fail"), + } +} + +/// Companion to the above, so it is not vacuous: the fatal path is still +/// reachable and still ends in `FatalError`. A malformed URL never reaches the +/// sidecar at all +#[tokio::test] +async fn a_genuinely_fatal_failure_still_reaches_the_wire_as_fatal_error() { + common::tests::setup(); + + let addr = serve_sidecar_page_not_found().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = anyhow::anyhow!("not a module error at all"); + let wire: genvm_modules_interfaces::Result = + crate::common::module_error_to_wire(err, common::tests::get_hello().genvm_id); + + match wire { + genvm_modules_interfaces::Result::FatalError(msg) => assert!( + msg.contains("not a module error at all"), + "unexpected message: {msg}" + ), + genvm_modules_interfaces::Result::Ok(_) + | genvm_modules_interfaces::Result::UserError(_) => { + panic!("an unclassified error must stay fatal") + } + } + + // and the config above is a working one, so the fatality is not an + // artifact of a broken fixture + assert!( + render(&config, "https://example.com/").await.is_err(), + "a 404 page is still an error, just a catchable one" + ); +} diff --git a/implementation/tests/request_status_fatality.rs b/implementation/tests/request_status_fatality.rs new file mode 100644 index 00000000..97c68583 --- /dev/null +++ b/implementation/tests/request_status_fatality.rs @@ -0,0 +1,222 @@ +//! How the shared request helpers classify a non-200, for both response +//! shapes. +//! +//! `send_request_get_lua_compatible_response_bytes` and its JSON twin are +//! generic: they are given a URL and a flag, and cannot tell this node's own +//! webdriver sidecar from a site the contract named. A non-200 from the latter +//! is a legitimate observation, a non-200 from the former never is -- so the +//! helpers do not decide fatality on their own, they keep raising `STATUS_NOT_OK` +//! as fatal and leave the exception to the caller that knows which endpoint it +//! is talking to. +//! +//! Today the only caller that lowers it is `Render` in +//! `install/config/genvm-web-default.lua`, which wraps the sidecar hop in a +//! `pcall`; see `web::tests` in the library. The LLM providers +//! (`src/llm/providers.rs`) all pass `error_on_status = true` and rely on the +//! fatal classification pinned here, so a change to the default would move them +//! too. + +use genvm_common::*; + +use genvm_modules::common; +use genvm_modules::scripting::{self, Metrics}; +use tokio::io::AsyncWriteExt as _; + +/// A one-shot server answering `head` (status line plus headers, +/// `Content-Length` is appended) followed by `body` +async fn serve(head: &'static str, body: &'static [u8]) -> std::net::SocketAddr { + let server = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = server.local_addr().unwrap(); + + tokio::spawn(async move { + while let Ok((mut client, _)) = server.accept().await { + let response = format!("{head}Content-Length: {}\r\n\r\n", body.len()); + client.write_all(response.as_bytes()).await.unwrap(); + client.write_all(body).await.unwrap(); + client.shutdown().await.unwrap(); + } + }); + + addr +} + +async fn serve_server_error() -> std::net::SocketAddr { + serve( + "HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n", + br#"{"error":"Internal server error"}"#, + ) + .await +} + +fn metrics() -> sync::DArc { + sync::DArc::new(Metrics::default()) +} + +fn get(url: &str) -> reqwest::RequestBuilder { + common::tests::create_client().unwrap().get(url) +} + +/// The module error a call failed with. Panics if it succeeded, or if the +/// failure was not a `ModuleError` +fn module_err(err: anyhow::Error) -> common::ModuleError { + scripting::try_unwrap_any_err(err).expect("expected a module error") +} + +fn assert_status_not_ok(err: common::ModuleError) { + assert!( + err.causes.contains(&"STATUS_NOT_OK".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!( + err.fatal, + "the generic helper keeps a bad status fatal; lowering it is the \ + caller's decision, see the module docs: {err:?}" + ); + assert!( + matches!( + err.ctx.get("status"), + Some(genvm_modules_interfaces::GenericValue::Number(s)) if *s == 500.0 + ), + "the status must survive into ctx for the caller to act on: {:?}", + err.ctx + ); +} + +// -- error_on_status = true: raise, fatally --------------------------- + +#[tokio::test] +async fn bytes_bad_status_raises_a_fatal_status_not_ok() { + common::tests::setup(); + + let addr = serve_server_error().await; + let url = format!("http://{addr}/"); + + let err = scripting::send_request_get_lua_compatible_response_bytes( + &metrics(), + &url, + get(&url), + true, + usize::MAX, + ) + .await + .err() + .expect("a 500 with error_on_status must fail"); + + assert_status_not_ok(module_err(err)); +} + +/// The twin of the above. It is reached whenever the request sets `json`, which +/// every LLM provider does +#[tokio::test] +async fn json_bad_status_raises_a_fatal_status_not_ok() { + common::tests::setup(); + + let addr = serve_server_error().await; + let url = format!("http://{addr}/"); + + let err = scripting::send_request_get_lua_compatible_response_json( + &metrics(), + &url, + get(&url), + true, + usize::MAX, + ) + .await + .err() + .expect("a 500 with error_on_status must fail"); + + assert_status_not_ok(module_err(err)); +} + +// -- error_on_status = false: the status is just data ----------------- + +#[tokio::test] +async fn bytes_bad_status_is_returned_when_not_erroring_on_status() { + common::tests::setup(); + + let addr = serve_server_error().await; + let url = format!("http://{addr}/"); + + let res = scripting::send_request_get_lua_compatible_response_bytes( + &metrics(), + &url, + get(&url), + false, + usize::MAX, + ) + .await + .expect("without error_on_status a 500 is an ordinary response"); + + assert_eq!(res.status, 500); + assert_eq!(res.body, br#"{"error":"Internal server error"}"#.to_vec()); +} + +#[tokio::test] +async fn json_bad_status_is_returned_when_not_erroring_on_status() { + common::tests::setup(); + + let addr = serve_server_error().await; + let url = format!("http://{addr}/"); + + let res = scripting::send_request_get_lua_compatible_response_json( + &metrics(), + &url, + get(&url), + false, + usize::MAX, + ) + .await + .expect("without error_on_status a 500 is an ordinary response"); + + assert_eq!(res.status, 500); + assert_eq!( + res.body, + serde_json::json!({ "error": "Internal server error" }) + ); +} + +// -- the JSON twin's own failure mode --------------------------------- + +/// Only the JSON path can fail this way: a `200` whose body is not JSON. It is +/// fatal for the same reason, and pinned here because the twin had no coverage +/// at all +#[tokio::test] +async fn json_undecodable_body_is_a_fatal_deserializing_error() { + common::tests::setup(); + + let addr = serve( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\n", + b"not json at all", + ) + .await; + let url = format!("http://{addr}/"); + + let err = scripting::send_request_get_lua_compatible_response_json( + &metrics(), + &url, + get(&url), + true, + usize::MAX, + ) + .await + .err() + .expect("an undecodable body must fail"); + + let err = module_err(err); + assert!( + err.causes.contains(&"DESERIALIZING".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!(err.fatal, "expected a fatal error: {err:?}"); + // the undecodable bytes are kept, so the failure is diagnosable + assert!( + matches!( + err.ctx.get("body"), + Some(genvm_modules_interfaces::GenericValue::Bytes(b)) if b == b"not json at all" + ), + "expected the raw body in ctx: {:?}", + err.ctx + ); +} diff --git a/install/config/genvm-web-default.lua b/install/config/genvm-web-default.lua index 47055ff0..c9e7871a 100644 --- a/install/config/genvm-web-default.lua +++ b/install/config/genvm-web-default.lua @@ -16,7 +16,17 @@ function Render(ctx, payload) .. "&waitAfterLoaded=" .. tostring(payload.wait_after_loaded or 0) - local result = lib.rs.request(ctx, { + -- This hop goes to *our own* webdriver sidecar, never to the contract's URL, + -- so the extent of this `pcall` is exactly the trust boundary: everything it + -- can catch is this node's environment failing, and nothing it catches is an + -- observation about the page. `lib.rs.request` cannot make that distinction + -- itself -- it sees only a URL -- so the fatality decision belongs here. + -- + -- Re-raised non-fatally so the contract gets a catchable nondeterministic + -- exception instead of the run being aborted as an internal error. Mirrors + -- `Request` below. The page's own outcome is reported by the sidecar as a + -- `200` plus a `resulting-status` header, and is handled further down. + local success, result = pcall(lib.rs.request, ctx, { method = "GET", url = web.rs.config.webdriver_host .. "/render" .. url_params, headers = {}, @@ -25,6 +35,10 @@ function Render(ctx, payload) unfiltered = true, }) + if not success then + lib.reraise_with_fatality(result, false) + end + lib.log { level = "debug", message = "web render result", diff --git a/webdriver/Dockerfile b/webdriver/Dockerfile index 1ad8d177..c1561b2b 100644 --- a/webdriver/Dockerfile +++ b/webdriver/Dockerfile @@ -74,6 +74,7 @@ RUN \ which chromium ; \ cd /src/prj && npm ci ; \ npm run build ; \ + npm test ; \ true WORKDIR /wd diff --git a/webdriver/src/prj/package.json b/webdriver/src/prj/package.json index 52706881..c2ec1883 100644 --- a/webdriver/src/prj/package.json +++ b/webdriver/src/prj/package.json @@ -7,7 +7,8 @@ "scripts": { "start": "node --loader ts-node/esm src/index.ts", "dev": "ts-node src/index.ts", - "build": "tsc" + "build": "tsc", + "test": "node --loader ts-node/esm --test test/*.test.ts" }, "dependencies": { "puppeteer-core": "24.16.0", diff --git a/webdriver/src/prj/src/browser/chrome.ts b/webdriver/src/prj/src/browser/chrome.ts index 6d618df9..11ff2e3b 100644 --- a/webdriver/src/prj/src/browser/chrome.ts +++ b/webdriver/src/prj/src/browser/chrome.ts @@ -74,6 +74,20 @@ const CHROME_HEADLESS = envBool('GVM_WEBDRIVER_CHROME_HEADLESS', true); // Extra flags appended to the defaults below. See `envStrList` for the format. const CHROME_EXTRA_ARGS = envStrList('GVM_WEBDRIVER_CHROME_ARGS', []); +/** + * Ceiling on a single CDP round-trip. Puppeteer's own default is 180s, which + * sits above the whole render budget (a 30s navigation plus at most 60s of + * `waitAfterLoaded`), so a wedged command used to be noticed long after the + * render should have finished. It must also stay below the module's HTTP + * deadline (`base_client_builder` in `implementation/src/common/mod.rs`, 300s) + * so that a stuck browser produces an answer from us rather than a client-side + * timeout with nothing to report. + */ +const PROTOCOL_TIMEOUT_MS = envDurationMs( + 'GVM_WEBDRIVER_PROTOCOL_TIMEOUT', + '90s', +); + async function newBrowser(): Promise { const args = [ '--no-sandbox', @@ -91,6 +105,7 @@ async function newBrowser(): Promise { headless: CHROME_HEADLESS, args, executablePath: CHROME_EXECUTABLE, + protocolTimeout: PROTOCOL_TIMEOUT_MS, }); logger.log('info', 'created new raw browser', { diff --git a/webdriver/src/prj/src/index.ts b/webdriver/src/prj/src/index.ts index d948c6ac..691c569f 100644 --- a/webdriver/src/prj/src/index.ts +++ b/webdriver/src/prj/src/index.ts @@ -1,23 +1,15 @@ -import puppeteer, * as pup from 'puppeteer-core'; +import type * as pup from 'puppeteer-core'; import http from 'http'; import { Command } from 'commander'; import * as logger from './logging.js'; import * as chromeBrowser from './browser/chrome.js'; -import * as ssrf from './ssrf.js'; -import { envDurationMs, envInt, formatDurationMs } from './duration.js'; - -interface NavigationOptions { - waitUntil?: pup.PuppeteerLifeCycleEvent; - timeout?: number; -} - -interface RenderOptions { - loadTimeout?: number; - waitAfterLoaded?: number; - waitUntil?: pup.PuppeteerLifeCycleEvent; - maxPageHeapMB?: number; -} +import { + renderPageWithBrowser, + statusIsGood, + type RenderOptions, +} from './render.js'; +import { envDurationMs, formatDurationMs } from './duration.js'; const program = new Command(); program @@ -29,15 +21,6 @@ program const options = program.opts(); -const STATUS_I_AM_A_TEAPOT = 418; - -const DEFAULT_MAX_PAGE_HEAP_MB = envInt('GVM_WEBDRIVER_MAX_PAGE_HEAP_MB', 1024); - -const MAX_WAIT_AFTER_LOADED_MS = envDurationMs( - 'GVM_WEBDRIVER_MAX_WAIT_AFTER_LOADED', - '60s', -); - const HEALTHCHECK_CACHE_DURATION_MS = envDurationMs( 'GVM_WEBDRIVER_HEALTHCHECK_CACHE_DURATION', '5m', @@ -48,166 +31,6 @@ function updateLastSuccessfulRenderTime() { lastSuccessfulRenderTime = Date.now(); } -function normalizeWhitespace(contents: string): string { - return contents - .split('\n') - .map((line) => line.trim().replace(/\s+/g, ' ')) - .join('\n') - .replace(/\n{2,}/g, '\n\n'); -} - -function getNavigationErrorStatus(error: any): number { - if (error.name === 'TimeoutError') { - return 408; // Request Timeout - } else if (error.message?.includes('net::ERR_NAME_NOT_RESOLVED')) { - return 502; // Bad Gateway - } else if (error.message?.includes('net::ERR_CONNECTION_REFUSED')) { - return 503; // Service Unavailable - } else if (error.message?.includes('net::ERR_CERT_')) { - return 495; // SSL Certificate Error - } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { - return 503; // Service Unavailable - } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { - return 403; // Forbidden (SSRF guard) - } - return STATUS_I_AM_A_TEAPOT; // Unknown error -} - -function getNavigationErrorMessage(error: any): string { - if (error.name === 'TimeoutError') { - return 'Navigation timeout'; - } else if (error.message?.includes('net::ERR_NAME_NOT_RESOLVED')) { - return 'DNS resolution failed'; - } else if (error.message?.includes('net::ERR_CONNECTION_REFUSED')) { - return 'Connection refused'; - } else if (error.message?.includes('net::ERR_CERT_')) { - return 'SSL certificate error'; - } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { - return 'No internet connection'; - } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { - return 'Blocked by SSRF guard: address not allowed'; - } - return `Navigation error: ${error.message || 'Unknown error'}`; -} - -async function navigateToPage( - page: pup.Page, - targetUrl: string, - options: NavigationOptions = {}, -): Promise<{ status: number; error?: string; response?: pup.HTTPResponse }> { - const { waitUntil = 'domcontentloaded', timeout = 30000 } = options; - - try { - const response = await page.goto(targetUrl, { - waitUntil, - timeout, - }); - - if (!response) { - return { - status: STATUS_I_AM_A_TEAPOT, - error: 'Navigation did not result in a valid HTTP response', - }; - } - - return { status: response.status(), response }; - } catch (navigationError: any) { - logger.log('error', 'navigation Error', navigationError); - const statusCode = getNavigationErrorStatus(navigationError); - const errorMessage = getNavigationErrorMessage(navigationError); - return { status: statusCode, error: errorMessage }; - } -} - -async function asText(page: pup.Page) { - const bodyText = await page.evaluate(() => { - return document.body.innerText; - }); - - return normalizeWhitespace(bodyText); -} - -async function asHTML(page: pup.Page) { - return await page.evaluate(() => { - return document.body.innerHTML; - }); -} - -async function asScreenshot(page: pup.Page) { - return await page.screenshot(); -} - -class HeapLimitExceeded extends Error { - constructor(heapMB: number, maxHeapMB: number) { - super(`Page JS heap ${heapMB.toFixed(1)}MB exceeds limit ${maxHeapMB}MB`); - } -} - -const HEAP_CHECK_INTERVAL_MS = envInt( - 'GVM_WEBDRIVER_HEAP_CHECK_INTERVAL_MS', - 200, -); - -async function withHeapMonitor( - page: pup.Page, - maxHeapMB: number, - fn: () => Promise, -): Promise { - let stopped = false; - let stopResolve: () => void; - const stopPromise = new Promise((r) => { - stopResolve = r; - }); - const monitor = (async () => { - while (!stopped) { - await new Promise((r) => setTimeout(r, HEAP_CHECK_INTERVAL_MS)); - if (stopped) break; - let totalMB: number; - try { - totalMB = await page.evaluate( - () => (performance as any).memory?.totalJSHeapSize / 1024 / 1024, - ); - } catch { - await stopPromise; - return; - } - logger.log('debug', 'heap monitor check', { - heapMB: totalMB.toFixed(1), - }); - if (totalMB > maxHeapMB) { - throw new HeapLimitExceeded(totalMB, maxHeapMB); - } - } - })(); - - try { - const result = await Promise.race([ - fn(), - monitor.then(() => undefined as never), - ]); - let totalMB: number; - try { - totalMB = await page.evaluate( - () => (performance as any).memory?.totalJSHeapSize / 1024 / 1024, - ); - } catch { - return result; - } - if (totalMB > maxHeapMB) { - throw new HeapLimitExceeded(totalMB, maxHeapMB); - } - return result; - } finally { - stopped = true; - stopResolve!(); - await monitor.catch(() => {}); - } -} - -function statusIsGood(status: number): boolean { - return (status >= 200 && status < 300) || status === 304; -} - async function renderPage( targetUrl: string, mode: 'text' | 'html' | 'screenshot', @@ -227,96 +50,6 @@ async function renderPage( } } -async function renderPageWithBrowser( - browserInstance: pup.Browser, - targetUrl: string, - mode: 'text' | 'html' | 'screenshot', - options: RenderOptions = {}, -): Promise<{ status: number; body: any }> { - const { - loadTimeout = 30000, - waitAfterLoaded = 0, - waitUntil = 'domcontentloaded', - maxPageHeapMB = DEFAULT_MAX_PAGE_HEAP_MB, - } = options; - - // Each render runs in its own browser context so cookies, localStorage, - // IndexedDB, service workers and HSTS state never leak between tenants - // sharing this long-lived browser. - const context = await browserInstance.createBrowserContext(); - const page = await context.newPage(); - - try { - await ssrf.installSsrfGuard(page); - context.on('targetcreated', async (target) => { - const p = await target.page(); - if (p && p !== page) { - await ssrf.installSsrfGuard(p); - } - }); - page.setViewport({ width: 1920 / 2, height: 1080 / 2 }); - - return await withHeapMonitor(page, maxPageHeapMB, async () => { - const navigationResult = await navigateToPage(page, targetUrl, { - waitUntil, - timeout: loadTimeout, - }); - - if (navigationResult.error) { - return { - status: navigationResult.status, - body: navigationResult.error, - }; - } - - const statusCode = navigationResult.status; - - if (statusIsGood(statusCode) && waitAfterLoaded > 0) { - let waitMs = Math.floor(waitAfterLoaded * 1000); - if (waitMs > MAX_WAIT_AFTER_LOADED_MS) { - logger.log('warn', 'waitAfterLoaded clamped to maximum', { - url: targetUrl, - requested: formatDurationMs(waitMs), - max: formatDurationMs(MAX_WAIT_AFTER_LOADED_MS), - }); - waitMs = MAX_WAIT_AFTER_LOADED_MS; - } - await new Promise((resolve) => setTimeout(resolve, waitMs)); - } - - let data; - switch (mode) { - case 'text': - data = await asText(page); - break; - case 'html': - data = await asHTML(page); - break; - case 'screenshot': - data = await asScreenshot(page); - break; - default: - data = 'Invalid mode'; - } - - return { status: statusCode, body: data }; - }); - } catch (e) { - if (e instanceof HeapLimitExceeded) { - logger.log('warn', 'page heap limit exceeded', { - url: targetUrl, - error: e.message, - }); - return { status: 507, body: e.message }; - } - throw e; - } finally { - // Close the page and its context together; the context teardown is what - // actually discards the per-request browsing state. - await Promise.allSettled([page.close(), context.close()]); - } -} - async function handleRenderRequest( parsedUrl: URL, req: http.IncomingMessage, diff --git a/webdriver/src/prj/src/render.ts b/webdriver/src/prj/src/render.ts new file mode 100644 index 00000000..68c0df13 --- /dev/null +++ b/webdriver/src/prj/src/render.ts @@ -0,0 +1,336 @@ +/** + * Rendering a page with an already-running browser. + * + * Split out of `index.ts` so it can be exercised without launching Chromium or + * binding a port: importing `index.ts` starts the HTTP server, and importing + * `browser/chrome.js` launches a browser at module scope. + * + * The contract this file upholds: everything it returns is an observation + * about the *page*, reported as a status the caller puts in `Resulting-Status` + * alongside a `200`. A failure of the sidecar itself is reported the same way + * rather than raised, because the caller turns a raised error into a `500`, + * and the module classifies a `500` from us as fatal. + */ + +import type * as pup from 'puppeteer-core'; + +import * as logger from './logging.js'; +import * as ssrf from './ssrf.js'; +import { envDurationMs, envInt, formatDurationMs } from './duration.js'; + +interface NavigationOptions { + waitUntil?: pup.PuppeteerLifeCycleEvent; + timeout?: number; +} + +export interface RenderOptions { + loadTimeout?: number; + waitAfterLoaded?: number; + waitUntil?: pup.PuppeteerLifeCycleEvent; + maxPageHeapMB?: number; +} + +const STATUS_I_AM_A_TEAPOT = 418; + +/** + * Reported when this sidecar cannot render at all, as opposed to the page + * failing to load. It joins the codes `getNavigationErrorStatus` already + * returns for an unreachable target, so the module treats it as a non-fatal + * `WEBPAGE_LOAD_FAILED` and the contract can catch it. + */ +export const STATUS_SERVICE_UNAVAILABLE = 503; + +const DEFAULT_MAX_PAGE_HEAP_MB = envInt('GVM_WEBDRIVER_MAX_PAGE_HEAP_MB', 1024); + +const MAX_WAIT_AFTER_LOADED_MS = envDurationMs( + 'GVM_WEBDRIVER_MAX_WAIT_AFTER_LOADED', + '60s', +); + +function normalizeWhitespace(contents: string): string { + return contents + .split('\n') + .map((line) => line.trim().replace(/\s+/g, ' ')) + .join('\n') + .replace(/\n{2,}/g, '\n\n'); +} + +function getNavigationErrorStatus(error: any): number { + if (error.name === 'TimeoutError') { + return 408; // Request Timeout + } else if (error.message?.includes('net::ERR_NAME_NOT_RESOLVED')) { + return 502; // Bad Gateway + } else if (error.message?.includes('net::ERR_CONNECTION_REFUSED')) { + return 503; // Service Unavailable + } else if (error.message?.includes('net::ERR_CERT_')) { + return 495; // SSL Certificate Error + } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { + return 503; // Service Unavailable + } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { + return 403; // Forbidden (SSRF guard) + } + return STATUS_I_AM_A_TEAPOT; // Unknown error +} + +function getNavigationErrorMessage(error: any): string { + if (error.name === 'TimeoutError') { + return 'Navigation timeout'; + } else if (error.message?.includes('net::ERR_NAME_NOT_RESOLVED')) { + return 'DNS resolution failed'; + } else if (error.message?.includes('net::ERR_CONNECTION_REFUSED')) { + return 'Connection refused'; + } else if (error.message?.includes('net::ERR_CERT_')) { + return 'SSL certificate error'; + } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { + return 'No internet connection'; + } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { + return 'Blocked by SSRF guard: address not allowed'; + } + return `Navigation error: ${error.message || 'Unknown error'}`; +} + +async function navigateToPage( + page: pup.Page, + targetUrl: string, + options: NavigationOptions = {}, +): Promise<{ status: number; error?: string; response?: pup.HTTPResponse }> { + const { waitUntil = 'domcontentloaded', timeout = 30000 } = options; + + try { + const response = await page.goto(targetUrl, { + waitUntil, + timeout, + }); + + if (!response) { + return { + status: STATUS_I_AM_A_TEAPOT, + error: 'Navigation did not result in a valid HTTP response', + }; + } + + return { status: response.status(), response }; + } catch (navigationError: any) { + logger.log('error', 'navigation Error', navigationError); + const statusCode = getNavigationErrorStatus(navigationError); + const errorMessage = getNavigationErrorMessage(navigationError); + return { status: statusCode, error: errorMessage }; + } +} + +async function asText(page: pup.Page) { + const bodyText = await page.evaluate(() => { + return document.body.innerText; + }); + + return normalizeWhitespace(bodyText); +} + +async function asHTML(page: pup.Page) { + return await page.evaluate(() => { + return document.body.innerHTML; + }); +} + +async function asScreenshot(page: pup.Page) { + return await page.screenshot(); +} + +export class HeapLimitExceeded extends Error { + constructor(heapMB: number, maxHeapMB: number) { + super(`Page JS heap ${heapMB.toFixed(1)}MB exceeds limit ${maxHeapMB}MB`); + } +} + +const HEAP_CHECK_INTERVAL_MS = envInt( + 'GVM_WEBDRIVER_HEAP_CHECK_INTERVAL_MS', + 200, +); + +async function withHeapMonitor( + page: pup.Page, + maxHeapMB: number, + fn: () => Promise, +): Promise { + let stopped = false; + let stopResolve: () => void; + const stopPromise = new Promise((r) => { + stopResolve = r; + }); + const monitor = (async () => { + while (!stopped) { + await new Promise((r) => setTimeout(r, HEAP_CHECK_INTERVAL_MS)); + if (stopped) break; + let totalMB: number; + try { + totalMB = await page.evaluate( + () => (performance as any).memory?.totalJSHeapSize / 1024 / 1024, + ); + } catch { + await stopPromise; + return; + } + logger.log('debug', 'heap monitor check', { + heapMB: totalMB.toFixed(1), + }); + if (totalMB > maxHeapMB) { + throw new HeapLimitExceeded(totalMB, maxHeapMB); + } + } + })(); + + try { + const result = await Promise.race([ + fn(), + monitor.then(() => undefined as never), + ]); + let totalMB: number; + try { + totalMB = await page.evaluate( + () => (performance as any).memory?.totalJSHeapSize / 1024 / 1024, + ); + } catch { + return result; + } + if (totalMB > maxHeapMB) { + throw new HeapLimitExceeded(totalMB, maxHeapMB); + } + return result; + } finally { + stopped = true; + stopResolve!(); + await monitor.catch(() => {}); + } +} + +export function statusIsGood(status: number): boolean { + return (status >= 200 && status < 300) || status === 304; +} + +/** + * Acquire the per-render browsing context and its page. + * + * Both calls are CDP round-trips (`Target.createBrowserContext`, + * `Target.createTarget`) that can hang or fail on their own, so they are kept + * in one place with the context closed if the page never materializes. + */ +async function newRenderTarget( + browserInstance: pup.Browser, +): Promise<{ context: pup.BrowserContext; page: pup.Page }> { + // Each render runs in its own browser context so cookies, localStorage, + // IndexedDB, service workers and HSTS state never leak between tenants + // sharing this long-lived browser. + const context = await browserInstance.createBrowserContext(); + try { + return { context, page: await context.newPage() }; + } catch (error) { + await context.close().catch(() => {}); + throw error; + } +} + +export async function renderPageWithBrowser( + browserInstance: pup.Browser, + targetUrl: string, + mode: 'text' | 'html' | 'screenshot', + options: RenderOptions = {}, +): Promise<{ status: number; body: any }> { + const { + loadTimeout = 30000, + waitAfterLoaded = 0, + waitUntil = 'domcontentloaded', + maxPageHeapMB = DEFAULT_MAX_PAGE_HEAP_MB, + } = options; + + // No contract input reaches `newRenderTarget` -- the target URL is not + // passed to it -- so anything it throws is this sidecar failing, never an + // observation about the page. Reported on the same channel a failed page + // load uses (a `200` carrying `Resulting-Status`) rather than being left to + // escape into the caller's `500` handler, because a `500` from us is + // classified as a fatal internal error that aborts the whole contract run. + let renderTarget: { context: pup.BrowserContext; page: pup.Page }; + try { + renderTarget = await newRenderTarget(browserInstance); + } catch (error) { + logger.log('error', 'could not open a page for rendering', { + url: targetUrl, + name: (error as Error).name, + error: (error as Error).message, + }); + return { + status: STATUS_SERVICE_UNAVAILABLE, + body: `Webdriver unavailable: ${(error as Error).message}`, + }; + } + const { context, page } = renderTarget; + + try { + await ssrf.installSsrfGuard(page); + context.on('targetcreated', async (target) => { + const p = await target.page(); + if (p && p !== page) { + await ssrf.installSsrfGuard(p); + } + }); + page.setViewport({ width: 1920 / 2, height: 1080 / 2 }); + + return await withHeapMonitor(page, maxPageHeapMB, async () => { + const navigationResult = await navigateToPage(page, targetUrl, { + waitUntil, + timeout: loadTimeout, + }); + + if (navigationResult.error) { + return { + status: navigationResult.status, + body: navigationResult.error, + }; + } + + const statusCode = navigationResult.status; + + if (statusIsGood(statusCode) && waitAfterLoaded > 0) { + let waitMs = Math.floor(waitAfterLoaded * 1000); + if (waitMs > MAX_WAIT_AFTER_LOADED_MS) { + logger.log('warn', 'waitAfterLoaded clamped to maximum', { + url: targetUrl, + requested: formatDurationMs(waitMs), + max: formatDurationMs(MAX_WAIT_AFTER_LOADED_MS), + }); + waitMs = MAX_WAIT_AFTER_LOADED_MS; + } + await new Promise((resolve) => setTimeout(resolve, waitMs)); + } + + let data; + switch (mode) { + case 'text': + data = await asText(page); + break; + case 'html': + data = await asHTML(page); + break; + case 'screenshot': + data = await asScreenshot(page); + break; + default: + data = 'Invalid mode'; + } + + return { status: statusCode, body: data }; + }); + } catch (e) { + if (e instanceof HeapLimitExceeded) { + logger.log('warn', 'page heap limit exceeded', { + url: targetUrl, + error: e.message, + }); + return { status: 507, body: e.message }; + } + throw e; + } finally { + // Close the page and its context together; the context teardown is what + // actually discards the per-request browsing state. + await Promise.allSettled([page.close(), context.close()]); + } +} diff --git a/webdriver/src/prj/test/render.test.ts b/webdriver/src/prj/test/render.test.ts new file mode 100644 index 00000000..f8c4d32e --- /dev/null +++ b/webdriver/src/prj/test/render.test.ts @@ -0,0 +1,150 @@ +/** + * Failures of this sidecar must not leave `renderPageWithBrowser` by being + * thrown. + * + * `handleRenderRequest` turns anything thrown out of here into a bare `500`, + * and the module classifies a `500` from its own webdriver as a fatal + * `STATUS_NOT_OK`, which aborts the whole contract run as an internal error. + * A page that merely fails to load takes a different route: it comes back as a + * status the caller puts in `Resulting-Status` next to a `200`, which the + * module reports as a catchable, non-fatal `WEBPAGE_LOAD_FAILED`. + * + * The browser is faked outright: no Chromium is launched and nothing leaves + * the process. `render.ts` is imported directly rather than through + * `index.ts`, which would start the HTTP server and launch a browser at module + * scope. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import type * as pup from 'puppeteer-core'; +import { ProtocolError, TimeoutError } from 'puppeteer-core'; + +import { + renderPageWithBrowser, + statusIsGood, + STATUS_SERVICE_UNAVAILABLE, +} from '../src/render.js'; + +interface FakeContext { + closes: number; +} + +/** + * A browser whose page creation fails the way a wedged or dying Chromium does. + * Records context closes so a leak shows up as a failing assertion. + */ +function browserFailingOn( + where: 'createBrowserContext' | 'newPage', + error: Error, +): { browser: pup.Browser; context: FakeContext } { + const context: FakeContext = { closes: 0 }; + + const fakeContext = { + newPage: async () => { + if (where === 'newPage') { + throw error; + } + throw new Error('unreachable'); + }, + close: async () => { + context.closes++; + }, + on: () => {}, + }; + + const browser = { + createBrowserContext: async () => { + if (where === 'createBrowserContext') { + throw error; + } + return fakeContext; + }, + }; + + return { browser: browser as unknown as pup.Browser, context }; +} + +async function render(browser: pup.Browser) { + return renderPageWithBrowser(browser, 'https://example.com/', 'text'); +} + +// -- a broken sidecar reports, it does not raise ---------------------- + +test('a protocol timeout opening a page is reported, not thrown', async () => { + // what puppeteer raises once `Target.createTarget` exceeds protocolTimeout + const { browser, context } = browserFailingOn( + 'newPage', + new ProtocolError('Target.createTarget timed out'), + ); + + const result = await render(browser); + + assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); + assert.match(String(result.body), /Webdriver unavailable/); + assert.equal(context.closes, 1, 'the browser context must not be leaked'); +}); + +test('a TimeoutError opening a page is reported, not thrown', async () => { + const { browser, context } = browserFailingOn( + 'newPage', + new TimeoutError('waiting for target failed'), + ); + + const result = await render(browser); + + assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); + assert.equal(context.closes, 1, 'the browser context must not be leaked'); +}); + +test('a failure creating the context is reported, not thrown', async () => { + const { browser } = browserFailingOn( + 'createBrowserContext', + new ProtocolError('Target.createBrowserContext timed out'), + ); + + const result = await render(browser); + + assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); +}); + +test('the reported status is one the module treats as a failed load', () => { + // `statusIsGood` is what both this sidecar and `Render` in + // `genvm-web-default.lua` use to decide whether a render succeeded + assert.equal(statusIsGood(STATUS_SERVICE_UNAVAILABLE), false); +}); + +// -- and the mapping stays scoped to setup ---------------------------- + +test('a failure after the page exists still propagates', async () => { + // Not vacuous: the change must not swallow every error. `installSsrfGuard` + // is the first thing done with a live page, and a page that cannot be + // guarded is a bug, not an environment blip + const closes = { page: 0, context: 0 }; + const page = { + setRequestInterception: async () => { + throw new Error('interception unavailable'); + }, + close: async () => { + closes.page++; + }, + on: () => {}, + setViewport: () => {}, + }; + const browser = { + createBrowserContext: async () => ({ + newPage: async () => page, + close: async () => { + closes.context++; + }, + on: () => {}, + }), + } as unknown as pup.Browser; + + await assert.rejects(render(browser), /interception unavailable/); + assert.deepEqual( + closes, + { page: 1, context: 1 }, + 'the page and its context must still be torn down', + ); +}); From 0a541a1c1d7bb2e69b76c443c309dc256cd0a30d Mon Sep 17 00:00:00 2001 From: Edgars Date: Mon, 10 Aug 2026 14:07:34 +0100 Subject: [PATCH 3/4] =?UTF-8?q?fix(web):=20Label=20sidecar=20faults,=20kee?= =?UTF-8?q?p=20them=20fatal=20so=20validators=20abstain=20=F0=9F=90=9B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reworks the previous commit, whose classification was wrong. A broken webdriver sidecar means this validator has no valid observation of the world. Making the failure contract-visible let the contract compute a result and the validator vote on a claim it never observed. The truthful answer is to abstain, and the node already turns a fatal GenVM error into a Timeout vote. The node states this principle for the sibling case, GenVM-manager availability: an infra outage is not an execution outcome, and a timeout is honest only once real phase time has elapsed. A sidecar fault is the same class; the previous commit routed it into the execution-outcome path. So the ORIGINAL behaviour produced the correct vote, and the defects worth addressing were the other two: a sidecar outage was indistinguishable from a genuine contract internal error, and it left no local trace. Reverted: - reraise_with_fatality(result, false) in Render. The pcall stays, but its job changes from softening to labelling: it calls web.reraise_as_webdriver_unavailable, which sets fatal = true explicitly. Fatality is now pinned at the call site rather than inherited from whatever the generic transport decided, so a later flip of a Rust default cannot silently make the validator vote. - the TypeScript mapping of setup failures onto 200 + Resulting-Status: 503. That status sits in the non-fatal page-load set, so it had the same wrong effect by a different route. Setup failures propagate to the outer catch and stay HTTP 500. render.ts now states the invariant as two channels: RETURNED means an observation about the page, THROWN means this sidecar failed, and they must not be mixed. Diagnosability, the improvement actually missing: a WEBDRIVER_UNAVAILABLE cause is prepended so STATUS_NOT_OK / SENDING_REQUEST / READING_BODY survive underneath. ErrorKind is deliberately NOT extended — this is Lua-raised vocabulary like WEBPAGE_LOAD_FAILED, MALFORMED_URL and TLD_FORBIDDEN, none of which are in the enum. It is not contract-visible: on the fatal path the contract receives no causes array, while ModuleError's Display is its JSON, so the label survives into the operator-facing abort message. Two sidecar-side logs added; a 500 previously left no local trace. Kept: the node:test runner and the src/render.ts split, which are the only reason the TypeScript is testable; module_error_to_wire and its tests, the seam where fatal is spent; the explicit 90s protocolTimeout, now strictly better since a wedged CDP call becomes a fast honest abstention rather than a slow one; newRenderTarget, which fixes a real context leak and re-throws. All test cases retained with assertions inverted. Four red-checks, each reverted then restored: - reinstating the rejected regression: 5 sidecar tests fail, 3 remote controls green - stripping the pcall entirely: same 5 fail, but at a DIFFERENT assertion, proving fatality and the label are independently load-bearing - making WEBPAGE_LOAD_FAILED fatal: the 3 remote controls fail and the sidecar tests pass, so the suite bites in both directions - re-applying the TS 503 mapping: 3 TypeScript tests fail The sharpest pair is render_sidecar_unhealthy_is_fatal against render_remote_page_unreachable_is_not_fatal: identical 503, opposite verdict, differing only in which channel carries it. That is exactly the confusion that produced the regression. Rust 116 passed, 0 failed. TypeScript 5/5. tsc --noEmit and cargo fmt --check clean. Also fixed 11 check-source-text violations this branch introduced plus 2 that chrome.ts carried from before it; the hook would have blocked a commit. --- implementation/src/common/mod.rs | 13 +- implementation/src/web/tests.rs | 168 ++++++++++++------ .../tests/request_status_fatality.rs | 22 +-- install/config/genvm-web-default.lua | 16 +- install/lib/genvm-lua/lib-web.lua | 29 +++ webdriver/src/prj/src/browser/chrome.ts | 4 +- webdriver/src/prj/src/index.ts | 8 + webdriver/src/prj/src/render.ts | 59 +++--- webdriver/src/prj/test/render.test.ts | 105 +++++++---- 9 files changed, 278 insertions(+), 146 deletions(-) diff --git a/implementation/src/common/mod.rs b/implementation/src/common/mod.rs index 416ad628..1a5cb697 100644 --- a/implementation/src/common/mod.rs +++ b/implementation/src/common/mod.rs @@ -255,9 +255,20 @@ where /// representation, so the whole contract run is aborted and the contract /// cannot catch it. Downstream, a node turns any GenVM error into a Timeout /// vote -/// 2. `UserError` is handed to the runner as `{"error": …}`, which +/// 2. `UserError` is handed to the runner as `{"error": ...}`, which /// `genlayer-py-std` raises as a catchable `NondetException` /// +/// Which one an infrastructure fault deserves is settled: (1). If this node's +/// own environment failed, it has no valid observation of the world, and a +/// `UserError` would let it assert to the contract -- and then vote on -- a claim +/// it never observed. The Timeout vote is the truthful "I could not do the +/// work", and consensus has that vote for exactly this case. (2) is for things +/// the run genuinely observed, such as a page that failed to load. +/// +/// The cost of (1) is diagnostic, not semantic: `FatalError` carries only a +/// string, so a raiser that wants to stay distinguishable must put a cause in +/// it. [`ModuleError`]'s `Display` is its JSON, so `causes` survives. +/// /// An error that is not a [`ModuleError`] is fatal, so "not a `ModuleError`" /// and "`fatal: true`" are indistinguishable past this point pub(crate) fn module_error_to_wire( diff --git a/implementation/src/web/tests.rs b/implementation/src/web/tests.rs index 2fa38bb0..78fa941b 100644 --- a/implementation/src/web/tests.rs +++ b/implementation/src/web/tests.rs @@ -52,7 +52,7 @@ async fn serve_sidecar_page_not_found() -> std::net::SocketAddr { /// The sidecar's outer `catch` (`webdriver/src/prj/src/index.ts`, /// `handleRenderRequest`): a bare `500` with no `Resulting-Status`, which is -/// what an exception thrown *outside* the per-page `try` becomes — e.g. +/// what an exception thrown *outside* the per-page `try` becomes -- e.g. /// `newPage()` exceeding puppeteer's `protocolTimeout` on /// `Target.createTarget`. async fn serve_sidecar_internal_error() -> std::net::SocketAddr { @@ -86,6 +86,19 @@ async fn serve_sidecar_unhealthy() -> std::net::SocketAddr { .await } +/// The same number on the other channel: the sidecar itself answered fine +/// (`200`) and is telling us the *page's* host refused the connection, which +/// `getNavigationErrorStatus` reports as `503`. Paired with +/// [`serve_sidecar_unhealthy`] on purpose -- the two differ only in which +/// channel carries the 503, and they must be classified oppositely +async fn serve_sidecar_page_unreachable() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 503\r\n", + b"Connection refused", + ) + .await +} + /// An address nothing listens on: the sidecar process is gone, so the /// connection is refused before any status exists. Binds and drops, so the /// port is known to have been free @@ -155,7 +168,7 @@ async fn create_test_vm(config: &sync::DArc) -> TestVM { } /// Same context as `HandlerProvider::create_execution_context`, including -/// `filter_dns = true` — the webdriver host is an IP literal, which reqwest +/// `filter_dns = true` -- the webdriver host is an IP literal, which reqwest /// never sends through the resolver, so the SSRF guard stays out of the way. fn create_test_ctx(config: &sync::DArc) -> sync::DArc { let hello = common::tests::get_hello(); @@ -218,25 +231,35 @@ async fn render_err(config: &sync::DArc, url: &str) -> ModuleErr scripting::try_unwrap_any_err(err).expect("expected a module error") } -/// Asserts that talking to the sidecar described by `addr` fails non-fatally, -/// i.e. as something the contract can catch rather than as an internal error -async fn assert_sidecar_failure_is_not_fatal( - addr: std::net::SocketAddr, - what: &str, -) -> ModuleError { +/// Asserts that talking to the sidecar described by `addr` fails fatally and is +/// labelled as a webdriver fault. +/// +/// Fatal is the point, not an accident. A broken sidecar means this validator +/// never observed the page, so it must not hand the contract a result: the run +/// is aborted as an internal error and the node votes Timeout, which is the +/// truthful "I could not do the work". The label is what lets an operator tell +/// this apart from a genuine internal error, and it survives into the +/// `FatalError` string because [`ModuleError`]'s `Display` is its JSON. +async fn assert_sidecar_failure_is_fatal(addr: std::net::SocketAddr, what: &str) -> ModuleError { let config = sync::DArc::new(test_config(format!("http://{addr}"))); let err = render_err(&config, "https://example.com/").await; assert!( - !err.fatal, - "{what} from our own webdriver sidecar must be non-fatal, got {err:?}" + err.fatal, + "{what} from our own webdriver sidecar must stay fatal so the validator \ + abstains rather than voting on a page it never observed, got {err:?}" + ); + assert!( + err.causes.contains(&"WEBDRIVER_UNAVAILABLE".to_owned()), + "{what} must be labelled as a webdriver fault, got causes {:?}", + err.causes ); err } -// ── a failing sidecar is an environment fault, not a verdict ────────── +// -- a failing sidecar is an environment fault, so we abstain ---------- /// Control. When the *page* fails, the sidecar still answers `200` and puts /// the page status in `Resulting-Status`; `Render` turns that into a @@ -259,26 +282,26 @@ async fn render_remote_page_load_failure_is_not_fatal() { assert!(!err.fatal, "page load failure must be non-fatal: {err:?}"); } -/// A 5xx from our *own* webdriver sidecar says nothing about the page — it -/// is this node's environment failing. It must travel the same non-fatal -/// channel as the control above instead of aborting the whole contract run -/// with an internal error. +/// A 5xx from our *own* webdriver sidecar says nothing about the page -- it is +/// this node's environment failing, so there is no observation to report and +/// nothing legitimate to vote on. It must stay fatal, which the executor turns +/// into `ResultCode::InternalError` and the node into a Timeout vote. /// -/// `send_request_get_lua_compatible_response_bytes` raises `STATUS_NOT_OK` -/// as fatal, because it sees only a URL and cannot tell our sidecar from a -/// contract-controlled site. `Render` is the caller that does know, so it -/// wraps the sidecar hop in a `pcall` and re-raises non-fatally, the way -/// `Request` already did +/// `Render` is the caller that knows which endpoint it is talking to, so it +/// wraps the sidecar hop in a `pcall` -- not to soften the failure, but to +/// label it and to pin fatality here rather than inherit whatever the generic +/// transport decided. #[tokio::test] -async fn render_sidecar_internal_error_is_not_fatal() { +async fn render_sidecar_internal_error_is_fatal() { common::tests::setup(); let addr = serve_sidecar_internal_error().await; - let err = assert_sidecar_failure_is_not_fatal(addr, "a 5xx").await; + let err = assert_sidecar_failure_is_fatal(addr, "a 5xx").await; // Not vacuous: the failure really is the status error raised on the // sidecar hop, not the `WEBPAGE_LOAD_FAILED` branch further down, which - // this response never reaches + // this response never reaches. The transport's own cause is kept as the + // underlying detail rather than replaced assert!( err.causes.contains(&"STATUS_NOT_OK".to_owned()), "unexpected causes: {:?}", @@ -290,11 +313,11 @@ async fn render_sidecar_internal_error_is_not_fatal() { /// kind of fault: a `400` means the module and the sidecar disagree about the /// query format, which is a deployment problem, not a statement about the page #[tokio::test] -async fn render_sidecar_bad_request_is_not_fatal() { +async fn render_sidecar_bad_request_is_fatal() { common::tests::setup(); let addr = serve_sidecar_bad_request().await; - let err = assert_sidecar_failure_is_not_fatal(addr, "a 400").await; + let err = assert_sidecar_failure_is_fatal(addr, "a 400").await; assert!( err.causes.contains(&"STATUS_NOT_OK".to_owned()), @@ -304,13 +327,15 @@ async fn render_sidecar_bad_request_is_not_fatal() { } /// A `503`, as answered by the sidecar's healthcheck path or by whatever -/// proxies it while it is down +/// proxies it while it is down. Half of a pair with +/// [`render_remote_page_unreachable_is_not_fatal`]: same number, opposite +/// verdict, because here it is the sidecar's own HTTP status #[tokio::test] -async fn render_sidecar_unhealthy_is_not_fatal() { +async fn render_sidecar_unhealthy_is_fatal() { common::tests::setup(); let addr = serve_sidecar_unhealthy().await; - let err = assert_sidecar_failure_is_not_fatal(addr, "a 503").await; + let err = assert_sidecar_failure_is_fatal(addr, "a 503").await; assert!( err.causes.contains(&"STATUS_NOT_OK".to_owned()), @@ -319,16 +344,47 @@ async fn render_sidecar_unhealthy_is_not_fatal() { ); } +/// The other half of that pair. A `503` in `Resulting-Status` on an otherwise +/// good `200` is the sidecar telling us the *page's* host refused the +/// connection -- something we did observe -- so it stays catchable. The two +/// tests differ only in which channel carries the 503, which is exactly the +/// distinction a fix is most likely to erase +#[tokio::test] +async fn render_remote_page_unreachable_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_page_unreachable().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!( + !err.fatal, + "a 503 the sidecar *reports* is an observation about the page, and must \ + stay catchable: {err:?}" + ); + assert!( + !err.causes.contains(&"WEBDRIVER_UNAVAILABLE".to_owned()), + "a working sidecar must not be blamed: {:?}", + err.causes + ); +} + /// The sidecar being *gone* is the same class of fault as it answering badly, /// and it arrives on a different path: `map_send_error` raises a fatal /// `SENDING_REQUEST` before any status exists. The `pcall` covers the whole -/// hop, so this is non-fatal too +/// hop, so this is labelled too #[tokio::test] -async fn render_sidecar_unreachable_is_not_fatal() { +async fn render_sidecar_unreachable_is_fatal() { common::tests::setup(); let addr = sidecar_down().await; - let err = assert_sidecar_failure_is_not_fatal(addr, "a refused connection").await; + let err = assert_sidecar_failure_is_fatal(addr, "a refused connection").await; assert!( err.causes.contains(&"SENDING_REQUEST".to_owned()), @@ -337,15 +393,20 @@ async fn render_sidecar_unreachable_is_not_fatal() { ); } -// ── the classification as the executor reads it ────────────────────── +// -- the classification as the executor reads it ---------------------- /// The end the property is really about. `ModuleError.fatal` is only a flag /// until [`crate::common::module_error_to_wire`] spends it: `FatalError` is /// what the executor turns into a bare `anyhow` and finally /// `ResultCode::InternalError`, which the contract cannot catch and which a -/// node reports as a Timeout vote. A sidecar 5xx must not take that path +/// node reports as a Timeout vote. That is where a sidecar 5xx has to land -- +/// abstaining is the honest answer when we never ran the render. +/// +/// The label has to survive the trip, because `FatalError` carries only a +/// string: without it an operator cannot tell an outage of our own webdriver +/// from a genuine internal error in the contract's execution #[tokio::test] -async fn render_sidecar_internal_error_reaches_the_wire_as_user_error() { +async fn render_sidecar_internal_error_reaches_the_wire_as_fatal_error() { common::tests::setup(); let addr = serve_sidecar_internal_error().await; @@ -356,43 +417,38 @@ async fn render_sidecar_internal_error_reaches_the_wire_as_user_error() { crate::common::module_error_to_wire(err, common::tests::get_hello().genvm_id); match wire { - genvm_modules_interfaces::Result::UserError(_) => {} - genvm_modules_interfaces::Result::FatalError(msg) => { - panic!("a sidecar 5xx must not abort the run as an internal error: {msg}") + genvm_modules_interfaces::Result::FatalError(msg) => assert!( + msg.contains("WEBDRIVER_UNAVAILABLE"), + "the abort must name the webdriver so it is not mistaken for a \ + contract-level internal error: {msg}" + ), + genvm_modules_interfaces::Result::UserError(v) => { + panic!("a sidecar 5xx must not become a result the contract can act on: {v:?}") } genvm_modules_interfaces::Result::Ok(_) => panic!("expected Render to fail"), } } -/// Companion to the above, so it is not vacuous: the fatal path is still -/// reachable and still ends in `FatalError`. A malformed URL never reaches the -/// sidecar at all +/// Companion to the above, so it is not vacuous: the non-fatal path is still +/// reachable and still ends in `UserError`. The sidecar here is working, and +/// what failed is the page -- a real observation the contract may catch and act +/// on #[tokio::test] -async fn a_genuinely_fatal_failure_still_reaches_the_wire_as_fatal_error() { +async fn render_remote_page_load_failure_reaches_the_wire_as_user_error() { common::tests::setup(); let addr = serve_sidecar_page_not_found().await; let config = sync::DArc::new(test_config(format!("http://{addr}"))); - let err = anyhow::anyhow!("not a module error at all"); + let err = render_raw_err(&config, "https://example.com/").await; let wire: genvm_modules_interfaces::Result = crate::common::module_error_to_wire(err, common::tests::get_hello().genvm_id); match wire { - genvm_modules_interfaces::Result::FatalError(msg) => assert!( - msg.contains("not a module error at all"), - "unexpected message: {msg}" - ), - genvm_modules_interfaces::Result::Ok(_) - | genvm_modules_interfaces::Result::UserError(_) => { - panic!("an unclassified error must stay fatal") + genvm_modules_interfaces::Result::UserError(_) => {} + genvm_modules_interfaces::Result::FatalError(msg) => { + panic!("a page that 404s must stay catchable, not abort the run: {msg}") } + genvm_modules_interfaces::Result::Ok(_) => panic!("expected Render to fail"), } - - // and the config above is a working one, so the fatality is not an - // artifact of a broken fixture - assert!( - render(&config, "https://example.com/").await.is_err(), - "a 404 page is still an error, just a catchable one" - ); } diff --git a/implementation/tests/request_status_fatality.rs b/implementation/tests/request_status_fatality.rs index 97c68583..8122c6a6 100644 --- a/implementation/tests/request_status_fatality.rs +++ b/implementation/tests/request_status_fatality.rs @@ -3,18 +3,18 @@ //! //! `send_request_get_lua_compatible_response_bytes` and its JSON twin are //! generic: they are given a URL and a flag, and cannot tell this node's own -//! webdriver sidecar from a site the contract named. A non-200 from the latter -//! is a legitimate observation, a non-200 from the former never is -- so the -//! helpers do not decide fatality on their own, they keep raising `STATUS_NOT_OK` -//! as fatal and leave the exception to the caller that knows which endpoint it -//! is talking to. +//! webdriver sidecar from a site the contract named. Fatal is the safe default +//! for that ambiguity, and it is what both keep raising for a bad status. //! -//! Today the only caller that lowers it is `Render` in -//! `install/config/genvm-web-default.lua`, which wraps the sidecar hop in a -//! `pcall`; see `web::tests` in the library. The LLM providers -//! (`src/llm/providers.rs`) all pass `error_on_status = true` and rely on the -//! fatal classification pinned here, so a change to the default would move them -//! too. +//! Callers that know which endpoint they are talking to adjust it, in one +//! direction only. `Request` in `install/config/genvm-web-default.lua` lowers +//! it, because its URL is contract-controlled and a failure there is a real +//! observation. `Render` keeps it fatal for the sidecar hop and only adds a +//! `WEBDRIVER_UNAVAILABLE` label, because a broken sidecar means the validator +//! observed nothing and must abstain rather than vote; see `web::tests` in the +//! library. The LLM providers (`src/llm/providers.rs`) all pass +//! `error_on_status = true` and rely on the fatal classification pinned here, +//! so a change to the default would move them too. use genvm_common::*; diff --git a/install/config/genvm-web-default.lua b/install/config/genvm-web-default.lua index c9e7871a..9385c810 100644 --- a/install/config/genvm-web-default.lua +++ b/install/config/genvm-web-default.lua @@ -19,13 +19,15 @@ function Render(ctx, payload) -- This hop goes to *our own* webdriver sidecar, never to the contract's URL, -- so the extent of this `pcall` is exactly the trust boundary: everything it -- can catch is this node's environment failing, and nothing it catches is an - -- observation about the page. `lib.rs.request` cannot make that distinction - -- itself -- it sees only a URL -- so the fatality decision belongs here. + -- observation about the page. -- - -- Re-raised non-fatally so the contract gets a catchable nondeterministic - -- exception instead of the run being aborted as an internal error. Mirrors - -- `Request` below. The page's own outcome is reported by the sidecar as a - -- `200` plus a `resulting-status` header, and is handled further down. + -- The `pcall` does NOT soften the failure -- it labels it. A sidecar we + -- cannot reach or that answers badly means we have no observation to report, + -- so the error stays fatal and this validator abstains (Timeout) rather than + -- voting on a result it never computed. Contrast `Request` below, whose URL + -- is contract-controlled: a failure there IS an observation, so it is + -- lowered to non-fatal. The page's own outcome likewise arrives as a `200` + -- plus a `resulting-status` header and is handled further down, non-fatally. local success, result = pcall(lib.rs.request, ctx, { method = "GET", url = web.rs.config.webdriver_host .. "/render" .. url_params, @@ -36,7 +38,7 @@ function Render(ctx, payload) }) if not success then - lib.reraise_with_fatality(result, false) + web.reraise_as_webdriver_unavailable(result) end lib.log { diff --git a/install/lib/genvm-lua/lib-web.lua b/install/lib/genvm-lua/lib-web.lua index eeb554ee..63a707c4 100644 --- a/install/lib/genvm-lua/lib-web.lua +++ b/install/lib/genvm-lua/lib-web.lua @@ -121,4 +121,33 @@ M.check_url = function(url) return false end +--- Re-raise a failure of *our own* webdriver sidecar. +--- +--- Deliberately fatal. A broken sidecar means this validator has no valid +--- observation of the page at all, so it must not produce a contract-visible +--- result: doing that would let it assert a claim it never observed, and then +--- vote on it. Fatal becomes `ResultCode::InternalError`, which a node turns +--- into a Timeout vote -- the truthful answer, "I could not do the work". +--- A failure of the *remote site* is a real observation and stays non-fatal; +--- only this node's own infrastructure takes this path. +--- +--- The cause is prepended rather than replacing what the transport reported, +--- so `STATUS_NOT_OK` / `SENDING_REQUEST` / `READING_BODY` survive as the +--- underlying detail. It is not contract-visible: a fatal `ModuleError` is +--- stringified into `FatalError` and never reaches the runner as `causes`, so +--- this word is for operators and for the node's logs. +---@param e any the caught error value +M.reraise_as_webdriver_unavailable = function(e) + local err = lib.rs.as_user_error(e) + if err == nil then + -- Not a module error, so it carries no fatality flag; unclassified is + -- already treated as fatal downstream. Re-raised untouched. + error(e) + end + + table.insert(err.causes, 1, "WEBDRIVER_UNAVAILABLE") + err.fatal = true + lib.rs.user_error(err) +end + return M diff --git a/webdriver/src/prj/src/browser/chrome.ts b/webdriver/src/prj/src/browser/chrome.ts index 11ff2e3b..0947f014 100644 --- a/webdriver/src/prj/src/browser/chrome.ts +++ b/webdriver/src/prj/src/browser/chrome.ts @@ -168,9 +168,9 @@ class ChromeBrowserManager implements browser.Manager { * Arm the next rotation check. Uses a self-rescheduling timeout instead of * setInterval so the next check is only scheduled once the current one has * fully settled (in `finally`). This structurally prevents overlapping - * rotations from interleaving holder swaps / closes — which could otherwise + * rotations from interleaving holder swaps / closes -- which could otherwise * orphan a freshly launched browser (process leak) or close one still - * serving a render — and avoids interval backlog under sustained load. + * serving a render -- and avoids interval backlog under sustained load. */ private scheduleRotationCheck(): void { setTimeout(() => { diff --git a/webdriver/src/prj/src/index.ts b/webdriver/src/prj/src/index.ts index 691c569f..1bdd611e 100644 --- a/webdriver/src/prj/src/index.ts +++ b/webdriver/src/prj/src/index.ts @@ -103,6 +103,14 @@ async function handleRenderRequest( } res.end(result.body); } catch (error) { + // Everything reaching here is this sidecar failing, not the page: page + // outcomes are *returned* as a status. The module reads this `500` as a + // fatal `WEBDRIVER_UNAVAILABLE` and the validator abstains, so leave a + // local trace -- otherwise the only record is the message below. + logger.log('error', 'render request failed', { + url: query.get('url') ?? '', + error: (error as Error).message, + }); res.writeHead(500, { 'Content-Type': 'application/json' }); res.end( JSON.stringify({ diff --git a/webdriver/src/prj/src/render.ts b/webdriver/src/prj/src/render.ts index 68c0df13..5e9a262b 100644 --- a/webdriver/src/prj/src/render.ts +++ b/webdriver/src/prj/src/render.ts @@ -5,11 +5,17 @@ * binding a port: importing `index.ts` starts the HTTP server, and importing * `browser/chrome.js` launches a browser at module scope. * - * The contract this file upholds: everything it returns is an observation - * about the *page*, reported as a status the caller puts in `Resulting-Status` - * alongside a `200`. A failure of the sidecar itself is reported the same way - * rather than raised, because the caller turns a raised error into a `500`, - * and the module classifies a `500` from us as fatal. + * The contract this file upholds: everything it *returns* is an observation + * about the page, reported as a status the caller puts in `Resulting-Status` + * alongside a `200`, which the module surfaces to the contract as a catchable + * `WEBPAGE_LOAD_FAILED`. Everything it *throws* is this sidecar failing. The + * caller turns a throw into a `500`, which the module classifies as fatal, so + * the validator abstains rather than voting on a page it never observed. + * + * The two channels must not be mixed. Folding a sidecar failure into a + * `Resulting-Status` -- 503 is the tempting one, since a page whose host + * refused the connection already reports 503 -- would make a broken sidecar + * indistinguishable from a real observation, and let the validator vote on it. */ import type * as pup from 'puppeteer-core'; @@ -32,14 +38,6 @@ export interface RenderOptions { const STATUS_I_AM_A_TEAPOT = 418; -/** - * Reported when this sidecar cannot render at all, as opposed to the page - * failing to load. It joins the codes `getNavigationErrorStatus` already - * returns for an unreachable target, so the module treats it as a non-fatal - * `WEBPAGE_LOAD_FAILED` and the contract can catch it. - */ -export const STATUS_SERVICE_UNAVAILABLE = 503; - const DEFAULT_MAX_PAGE_HEAP_MB = envInt('GVM_WEBDRIVER_MAX_PAGE_HEAP_MB', 1024); const MAX_WAIT_AFTER_LOADED_MS = envDurationMs( @@ -212,7 +210,8 @@ export function statusIsGood(status: number): boolean { * * Both calls are CDP round-trips (`Target.createBrowserContext`, * `Target.createTarget`) that can hang or fail on their own, so they are kept - * in one place with the context closed if the page never materializes. + * in one place with the context closed if the page never materializes. The + * failure itself is re-raised, not translated -- see the file header. */ async function newRenderTarget( browserInstance: pup.Browser, @@ -225,6 +224,13 @@ async function newRenderTarget( return { context, page: await context.newPage() }; } catch (error) { await context.close().catch(() => {}); + // Logged here as well as re-raised: this is the last point that still + // knows the failure was ours rather than the page's, and the `500` the + // caller sends carries only a message. + logger.log('error', 'could not open a page for rendering', { + name: (error as Error).name, + error: (error as Error).message, + }); throw error; } } @@ -244,25 +250,12 @@ export async function renderPageWithBrowser( // No contract input reaches `newRenderTarget` -- the target URL is not // passed to it -- so anything it throws is this sidecar failing, never an - // observation about the page. Reported on the same channel a failed page - // load uses (a `200` carrying `Resulting-Status`) rather than being left to - // escape into the caller's `500` handler, because a `500` from us is - // classified as a fatal internal error that aborts the whole contract run. - let renderTarget: { context: pup.BrowserContext; page: pup.Page }; - try { - renderTarget = await newRenderTarget(browserInstance); - } catch (error) { - logger.log('error', 'could not open a page for rendering', { - url: targetUrl, - name: (error as Error).name, - error: (error as Error).message, - }); - return { - status: STATUS_SERVICE_UNAVAILABLE, - body: `Webdriver unavailable: ${(error as Error).message}`, - }; - } - const { context, page } = renderTarget; + // observation about the page. It is therefore allowed to propagate: the + // caller turns it into a `500`, which the module classifies as fatal, and + // the validator abstains instead of reporting a page outcome it never + // observed. It must NOT be folded into a `Resulting-Status`, which is the + // channel reserved for what actually happened to the page. + const { context, page } = await newRenderTarget(browserInstance); try { await ssrf.installSsrfGuard(page); diff --git a/webdriver/src/prj/test/render.test.ts b/webdriver/src/prj/test/render.test.ts index f8c4d32e..2e982bf3 100644 --- a/webdriver/src/prj/test/render.test.ts +++ b/webdriver/src/prj/test/render.test.ts @@ -1,13 +1,18 @@ /** - * Failures of this sidecar must not leave `renderPageWithBrowser` by being - * thrown. + * Failures of this sidecar must leave `renderPageWithBrowser` by being thrown, + * and must not be dressed up as a page status. * * `handleRenderRequest` turns anything thrown out of here into a bare `500`, * and the module classifies a `500` from its own webdriver as a fatal - * `STATUS_NOT_OK`, which aborts the whole contract run as an internal error. - * A page that merely fails to load takes a different route: it comes back as a - * status the caller puts in `Resulting-Status` next to a `200`, which the - * module reports as a catchable, non-fatal `WEBPAGE_LOAD_FAILED`. + * `WEBDRIVER_UNAVAILABLE`. That is the intended outcome: a validator whose own + * sidecar is broken has no observation of the page, so it must abstain (the + * node votes Timeout) rather than assert a result it never computed. + * + * A page that merely fails to load is the opposite case -- a real observation. + * It comes back as a *returned* status the caller puts in `Resulting-Status` + * next to a `200`, which the module reports as a catchable, non-fatal + * `WEBPAGE_LOAD_FAILED`. The pairs below pin both directions, so reclassifying + * either one fails the suite. * * The browser is faked outright: no Chromium is launched and nothing leaves * the process. `render.ts` is imported directly rather than through @@ -20,11 +25,7 @@ import assert from 'node:assert/strict'; import type * as pup from 'puppeteer-core'; import { ProtocolError, TimeoutError } from 'puppeteer-core'; -import { - renderPageWithBrowser, - statusIsGood, - STATUS_SERVICE_UNAVAILABLE, -} from '../src/render.js'; +import { renderPageWithBrowser, statusIsGood } from '../src/render.js'; interface FakeContext { closes: number; @@ -69,57 +70,44 @@ async function render(browser: pup.Browser) { return renderPageWithBrowser(browser, 'https://example.com/', 'text'); } -// -- a broken sidecar reports, it does not raise ---------------------- +// -- a broken sidecar raises, it does not report ---------------------- -test('a protocol timeout opening a page is reported, not thrown', async () => { +test('a protocol timeout opening a page is thrown, not reported', async () => { // what puppeteer raises once `Target.createTarget` exceeds protocolTimeout const { browser, context } = browserFailingOn( 'newPage', new ProtocolError('Target.createTarget timed out'), ); - const result = await render(browser); - - assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); - assert.match(String(result.body), /Webdriver unavailable/); + await assert.rejects(render(browser), /Target.createTarget timed out/); assert.equal(context.closes, 1, 'the browser context must not be leaked'); }); -test('a TimeoutError opening a page is reported, not thrown', async () => { +test('a TimeoutError opening a page is thrown, not reported', async () => { const { browser, context } = browserFailingOn( 'newPage', new TimeoutError('waiting for target failed'), ); - const result = await render(browser); - - assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); + await assert.rejects(render(browser), /waiting for target failed/); assert.equal(context.closes, 1, 'the browser context must not be leaked'); }); -test('a failure creating the context is reported, not thrown', async () => { +test('a failure creating the context is thrown, not reported', async () => { const { browser } = browserFailingOn( 'createBrowserContext', new ProtocolError('Target.createBrowserContext timed out'), ); - const result = await render(browser); - - assert.equal(result.status, STATUS_SERVICE_UNAVAILABLE); -}); - -test('the reported status is one the module treats as a failed load', () => { - // `statusIsGood` is what both this sidecar and `Render` in - // `genvm-web-default.lua` use to decide whether a render succeeded - assert.equal(statusIsGood(STATUS_SERVICE_UNAVAILABLE), false); + await assert.rejects( + render(browser), + /Target.createBrowserContext timed out/, + ); }); -// -- and the mapping stays scoped to setup ---------------------------- - test('a failure after the page exists still propagates', async () => { - // Not vacuous: the change must not swallow every error. `installSsrfGuard` - // is the first thing done with a live page, and a page that cannot be - // guarded is a bug, not an environment blip + // `installSsrfGuard` is the first thing done with a live page, and a page + // that cannot be guarded is this sidecar failing too const closes = { page: 0, context: 0 }; const page = { setRequestInterception: async () => { @@ -148,3 +136,48 @@ test('a failure after the page exists still propagates', async () => { 'the page and its context must still be torn down', ); }); + +// -- and a failing remote page still reports, it does not raise ------- + +/** + * The other half of the pair, so the tests above are not satisfied by "throw + * everything". A site that refuses the connection is something we *did* + * observe, so it comes back as a status rather than an exception -- and it is + * a 503, the very code the sidecar-failure path must not borrow. + */ +test('an unreachable remote site is reported, not thrown', async () => { + const closes = { page: 0, context: 0 }; + const page = { + setRequestInterception: async () => {}, + goto: async () => { + throw new Error('net::ERR_CONNECTION_REFUSED at https://example.com/'); + }, + evaluate: async () => 0, + close: async () => { + closes.page++; + }, + on: () => {}, + off: () => {}, + setViewport: () => {}, + }; + const browser = { + createBrowserContext: async () => ({ + newPage: async () => page, + close: async () => { + closes.context++; + }, + on: () => {}, + }), + } as unknown as pup.Browser; + + const result = await render(browser); + + assert.equal(result.status, 503); + assert.match(String(result.body), /Connection refused/); + assert.equal( + statusIsGood(result.status), + false, + 'the module must see this as a failed load', + ); + assert.deepEqual(closes, { page: 1, context: 1 }); +}); From a86ca277430a55af14072990ac0509b75d4a9bab Mon Sep 17 00:00:00 2001 From: Edgars Date: Mon, 10 Aug 2026 14:52:25 +0100 Subject: [PATCH 4/4] =?UTF-8?q?fix(web):=20Route=20a=20disconnected=20host?= =?UTF-8?q?=20as=20our=20fault,=20not=20the=20site's=20=F0=9F=90=9B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit net::ERR_INTERNET_DISCONNECTED means this host has no network route. The validator observed nothing about the page, so it must abstain rather than return a catchable status the contract can act on and vote from. It now leaves through the thrown channel: navigateToPage raises LocalNetworkUnavailable before any status mapping runs, handleRenderRequest turns it into a bare 500, and the module keeps it fatal. net::ERR_NAME_NOT_RESOLVED and TimeoutError are deliberately NOT treated the same way, and there are now tests that fail if anyone changes that. Both are genuinely ambiguous: a bad domain or our broken resolver, a slow site or our wedged browser. Resolving that ambiguity is what having several validators and an equivalence principle is for, and pre-judging it here would suppress the disagreement consensus exists to reconcile. ERR_INTERNET_DISCONNECTED differs only in being unambiguous: there is no observation to reconcile. ERR_CONNECTION_REFUSED, ERR_CERT_* and ERR_BLOCKED_BY_CLIENT are untouched; they are genuine observations about the site. The certificate case had no coverage and now has some. The label is knowingly imprecise and is NOT widened here. reraise_as_webdriver_unavailable names the hop, not the diagnosis: it fires for anything the pcall around the sidecar request catches. On a host with no network both the browser and the sidecar answered normally, so the word points an operator at a working subsystem. Renaming it to something hop-shaped is safe, since a fatal ModuleError never reaches the contract as causes, but it is a separate decision. Until then the distinction is carried by the sidecar message, which the wire test pins. Tests: 4 TypeScript, 4 Rust. Eight red-checks, each reverted and restored. Four revert the sidecar behaviour the fixtures model; two more break the classification itself in each direction, so the suite bites both ways -- making the raise non-fatal fails 7 fatal-side tests with all 5 non-fatal green, and making WEBPAGE_LOAD_FAILED fatal fails 5 non-fatal tests with all 7 fatal-side green. Rust 120 passed, 0 failed (was 116). TypeScript 9/9 (was 5/5). cargo fmt --check, tsc --noEmit and check-source-text all clean. Left open, deliberately: ERR_PROXY_CONNECTION_FAILED and net::ERR_NETWORK_CHANGED are as unambiguously ours as disconnected but still fall through to the 418 default on the observation channel. The ruling named one code and this change implements one code. --- implementation/src/web/tests.rs | 184 ++++++++++++++++++++++++++ webdriver/src/prj/src/render.ts | 56 +++++++- webdriver/src/prj/test/render.test.ts | 149 ++++++++++++++++++++- 3 files changed, 378 insertions(+), 11 deletions(-) diff --git a/implementation/src/web/tests.rs b/implementation/src/web/tests.rs index 78fa941b..a13d8819 100644 --- a/implementation/src/web/tests.rs +++ b/implementation/src/web/tests.rs @@ -63,6 +63,23 @@ async fn serve_sidecar_internal_error() -> std::net::SocketAddr { .await } +/// The one navigation failure the sidecar refuses to report: +/// `net::ERR_INTERNET_DISCONNECTED` means the request never left the host, so +/// there is no observation to report and `render.ts` throws instead of +/// returning a status. The outer catch turns that into the same bare `500`, +/// byte-accurate down to the message, which is the only thing that crosses. +/// +/// Its wording is the point. The browser and the sidecar answered normally, so +/// an operator told "webdriver unavailable" would go and stare at a working +/// browser; the text has to say the local network is what broke. +async fn serve_sidecar_local_network_unavailable() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n", + br#"{"error":"Internal server error","message":"Local network fault: this host has no network route (net::ERR_INTERNET_DISCONNECTED). The browser answered, so it is the local network that failed rather than the webdriver, and nothing about the page was observed."}"#, + ) + .await +} + /// The sidecar's parameter validation (`handleRenderRequest`): a `400` when /// `url` is missing or `mode` is not one of text/html/screenshot. Reachable /// whenever the module and the sidecar disagree about the query format, e.g. @@ -99,6 +116,29 @@ async fn serve_sidecar_page_unreachable() -> std::net::SocketAddr { .await } +/// A name that did not resolve, which `getNavigationErrorStatus` reports as a +/// `502` on the observation channel. Paired with +/// [`serve_sidecar_local_network_unavailable`]: both are network failures, and +/// they are classified oppositely on purpose. See +/// [`render_remote_page_name_not_resolved_is_not_fatal`] +async fn serve_sidecar_page_name_not_resolved() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 502\r\n", + b"DNS resolution failed", + ) + .await +} + +/// A navigation that ran out of time, reported as a `408` on the observation +/// channel. See [`render_remote_page_navigation_timeout_is_not_fatal`] +async fn serve_sidecar_page_navigation_timeout() -> std::net::SocketAddr { + serve_sidecar( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nResulting-Status: 408\r\n", + b"Navigation timeout", + ) + .await +} + /// An address nothing listens on: the sidecar process is gone, so the /// connection is refused before any status exists. Binds and drops, so the /// port is known to have been free @@ -344,6 +384,29 @@ async fn render_sidecar_unhealthy_is_fatal() { ); } +/// The host this validator runs on has no network at all. The sidecar throws +/// rather than reporting a status, because a request that never left the +/// machine is not an observation about the page, and the outer catch turns +/// that into the same bare `500`. +/// +/// It travels the webdriver hop, so it collects the same `WEBDRIVER_UNAVAILABLE` +/// label; that label names the hop, not the diagnosis. Which box actually broke +/// is carried by the sidecar's own message, pinned by +/// [`render_sidecar_local_network_fault_names_the_local_network_on_the_wire`] +#[tokio::test] +async fn render_sidecar_local_network_fault_is_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_local_network_unavailable().await; + let err = assert_sidecar_failure_is_fatal(addr, "a local network fault").await; + + assert!( + err.causes.contains(&"STATUS_NOT_OK".to_owned()), + "unexpected causes: {:?}", + err.causes + ); +} + /// The other half of that pair. A `503` in `Resulting-Status` on an otherwise /// good `200` is the sidecar telling us the *page's* host refused the /// connection -- something we did observe -- so it stays catchable. The two @@ -375,6 +438,88 @@ async fn render_remote_page_unreachable_is_not_fatal() { ); } +/// A name that does not resolve stays on the observation channel, and this +/// test exists to keep it there. +/// +/// It is genuinely ambiguous -- the domain may not exist, or *our* resolver may +/// be broken -- and one validator cannot tell those apart. That is precisely +/// what several validators and an equivalence principle are for: deciding it +/// here, the way `net::ERR_INTERNET_DISCONNECTED` is decided, would suppress +/// the disagreement consensus exists to reconcile. So it stays catchable, the +/// contract sees it, and the vote records what this node saw +#[tokio::test] +async fn render_remote_page_name_not_resolved_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_page_name_not_resolved().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!( + !err.fatal, + "a name that does not resolve is ambiguous between the site and us, and \ + that ambiguity is for the validators to settle, not for this node to \ + pre-judge by abstaining: {err:?}" + ); + assert!( + !err.causes.contains(&"WEBDRIVER_UNAVAILABLE".to_owned()), + "a working sidecar must not be blamed: {:?}", + err.causes + ); + // not vacuous: this really is the 502 branch, not some other failure + assert!( + matches!( + err.ctx.get("status"), + Some(genvm_modules_interfaces::GenericValue::Number(s)) if *s == 502.0 + ), + "expected the reported page status in ctx: {:?}", + err.ctx + ); +} + +/// The same guard for the other ambiguous case. A navigation timeout is either +/// a slow site or a browser of ours that wedged, and a validator cannot tell +/// which; it stays a contract-visible observation for the same reason +#[tokio::test] +async fn render_remote_page_navigation_timeout_is_not_fatal() { + common::tests::setup(); + + let addr = serve_sidecar_page_navigation_timeout().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_err(&config, "https://example.com/").await; + + assert!( + err.causes.contains(&"WEBPAGE_LOAD_FAILED".to_owned()), + "unexpected causes: {:?}", + err.causes + ); + assert!( + !err.fatal, + "a navigation timeout is ambiguous between the site and us, and stays \ + catchable so the validators can disagree about it: {err:?}" + ); + assert!( + !err.causes.contains(&"WEBDRIVER_UNAVAILABLE".to_owned()), + "a working sidecar must not be blamed: {:?}", + err.causes + ); + assert!( + matches!( + err.ctx.get("status"), + Some(genvm_modules_interfaces::GenericValue::Number(s)) if *s == 408.0 + ), + "expected the reported page status in ctx: {:?}", + err.ctx + ); +} + /// The sidecar being *gone* is the same class of fault as it answering badly, /// and it arrives on a different path: `map_send_error` raises a fatal /// `SENDING_REQUEST` before any status exists. The `pcall` covers the whole @@ -452,3 +597,42 @@ async fn render_remote_page_load_failure_reaches_the_wire_as_user_error() { genvm_modules_interfaces::Result::Ok(_) => panic!("expected Render to fail"), } } + +/// The other thing the abort string has to carry. `WEBDRIVER_UNAVAILABLE` is +/// the name of the hop that failed, and for a machine with no network it is the +/// wrong diagnosis: the browser and the sidecar answered normally, so an +/// operator following that word alone goes and stares at a working browser. +/// +/// The sidecar's own message is what distinguishes them, and this pins that it +/// survives all the way to the string an operator reads. The body arrives from +/// the transport as bytes, but the `pcall` round trip through Lua turns it into +/// a string, so it lands in [`ModuleError`]'s JSON as readable text rather than +/// a byte array +#[tokio::test] +async fn render_sidecar_local_network_fault_names_the_local_network_on_the_wire() { + common::tests::setup(); + + let addr = serve_sidecar_local_network_unavailable().await; + let config = sync::DArc::new(test_config(format!("http://{addr}"))); + + let err = render_raw_err(&config, "https://example.com/").await; + let wire: genvm_modules_interfaces::Result = + crate::common::module_error_to_wire(err, common::tests::get_hello().genvm_id); + + match wire { + genvm_modules_interfaces::Result::FatalError(msg) => { + assert!( + msg.contains("local network"), + "the abort must say which part of this node broke: {msg}" + ); + assert!( + msg.contains("WEBDRIVER_UNAVAILABLE"), + "the hop is still named, so the two live side by side: {msg}" + ); + } + genvm_modules_interfaces::Result::UserError(v) => { + panic!("a host with no network observed nothing, so the contract must not act on it: {v:?}") + } + genvm_modules_interfaces::Result::Ok(_) => panic!("expected Render to fail"), + } +} diff --git a/webdriver/src/prj/src/render.ts b/webdriver/src/prj/src/render.ts index 5e9a262b..1da01c59 100644 --- a/webdriver/src/prj/src/render.ts +++ b/webdriver/src/prj/src/render.ts @@ -10,7 +10,9 @@ * alongside a `200`, which the module surfaces to the contract as a catchable * `WEBPAGE_LOAD_FAILED`. Everything it *throws* is this sidecar failing. The * caller turns a throw into a `500`, which the module classifies as fatal, so - * the validator abstains rather than voting on a page it never observed. + * the validator abstains rather than voting on a page it never observed. "This + * sidecar failing" includes the host it runs on: a machine with no network is + * ours too, and `LocalNetworkUnavailable` below is that case. * * The two channels must not be mixed. Folding a sidecar failure into a * `Resulting-Status` -- 503 is the tempting one, since a page whose host @@ -53,17 +55,57 @@ function normalizeWhitespace(contents: string): string { .replace(/\n{2,}/g, '\n\n'); } +/** + * This machine has no network at all, which is not something we observed about + * the page: `net::ERR_INTERNET_DISCONNECTED` is Chrome saying the request never + * left the host. + * + * It is the one navigation error that leaves through the thrown channel. The + * others are ambiguous between the site and us, and stay returned: a name that + * does not resolve is either a domain that does not exist or a resolver of ours + * that is broken, and a timeout is either a slow site or a browser of ours that + * is wedged. Settling that is what several validators and an equivalence + * principle are for, so deciding it here would suppress the very disagreement + * consensus exists to reconcile. This case is different only because it is + * unambiguous -- there is no observation to reconcile. + * + * The wording says *local network* rather than *webdriver*: the browser and + * this sidecar are answering normally, and the `500` the caller sends carries + * only this text. + */ +export class LocalNetworkUnavailable extends Error { + constructor() { + super( + 'Local network fault: this host has no network route ' + + '(net::ERR_INTERNET_DISCONNECTED). The browser answered, so it is ' + + 'the local network that failed rather than the webdriver, and ' + + 'nothing about the page was observed.', + ); + this.name = 'LocalNetworkUnavailable'; + } +} + +/** + * What we observed happening to the page, as a status for `Resulting-Status`. + * + * `net::ERR_INTERNET_DISCONNECTED` is deliberately absent: it is ours rather + * than the page's, so `navigateToPage` throws it before reaching here. See + * [`LocalNetworkUnavailable`] for why the neighbouring cases are NOT treated + * the same way. + */ function getNavigationErrorStatus(error: any): number { if (error.name === 'TimeoutError') { + // Ambiguous on purpose: a slow site and a browser of ours that wedged + // are indistinguishable here, so the validators settle it, not us. return 408; // Request Timeout } else if (error.message?.includes('net::ERR_NAME_NOT_RESOLVED')) { + // Ambiguous in the same way: a domain that does not exist and a broken + // resolver of ours look identical from this side. return 502; // Bad Gateway } else if (error.message?.includes('net::ERR_CONNECTION_REFUSED')) { return 503; // Service Unavailable } else if (error.message?.includes('net::ERR_CERT_')) { return 495; // SSL Certificate Error - } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { - return 503; // Service Unavailable } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { return 403; // Forbidden (SSRF guard) } @@ -79,8 +121,6 @@ function getNavigationErrorMessage(error: any): string { return 'Connection refused'; } else if (error.message?.includes('net::ERR_CERT_')) { return 'SSL certificate error'; - } else if (error.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { - return 'No internet connection'; } else if (error.message?.includes('net::ERR_BLOCKED_BY_CLIENT')) { return 'Blocked by SSRF guard: address not allowed'; } @@ -110,6 +150,12 @@ async function navigateToPage( return { status: response.status(), response }; } catch (navigationError: any) { logger.log('error', 'navigation Error', navigationError); + if (navigationError.message?.includes('net::ERR_INTERNET_DISCONNECTED')) { + // Thrown, not returned: this one is our fault, and the caller's `500` + // is what makes the module abort, so the validator abstains rather + // than voting on a page the request never reached. + throw new LocalNetworkUnavailable(); + } const statusCode = getNavigationErrorStatus(navigationError); const errorMessage = getNavigationErrorMessage(navigationError); return { status: statusCode, error: errorMessage }; diff --git a/webdriver/src/prj/test/render.test.ts b/webdriver/src/prj/test/render.test.ts index 2e982bf3..3872780f 100644 --- a/webdriver/src/prj/test/render.test.ts +++ b/webdriver/src/prj/test/render.test.ts @@ -14,6 +14,13 @@ * `WEBPAGE_LOAD_FAILED`. The pairs below pin both directions, so reclassifying * either one fails the suite. * + * One navigation error crosses that line, and only one: + * `net::ERR_INTERNET_DISCONNECTED` says the request never left this host, so + * there is nothing observed to report and it is thrown. The neighbouring + * errors are deliberately NOT treated that way, and the tests that pin them + * are guards rather than descriptions -- see each one for why the ambiguity is + * left for the validators to settle. + * * The browser is faked outright: no Chromium is launched and nothing leaves * the process. `render.ts` is imported directly rather than through * `index.ts`, which would start the HTTP server and launch a browser at module @@ -140,18 +147,21 @@ test('a failure after the page exists still propagates', async () => { // -- and a failing remote page still reports, it does not raise ------- /** - * The other half of the pair, so the tests above are not satisfied by "throw - * everything". A site that refuses the connection is something we *did* - * observe, so it comes back as a status rather than an exception -- and it is - * a 503, the very code the sidecar-failure path must not borrow. + * A browser that opens a page normally and then fails the navigation with + * `error`, which is how a healthy sidecar meets a page it cannot load. + * Records page and context closes so a leak shows up as a failing assertion. */ -test('an unreachable remote site is reported, not thrown', async () => { +function browserWhoseNavigationFails(error: Error): { + browser: pup.Browser; + closes: { page: number; context: number }; +} { const closes = { page: 0, context: 0 }; const page = { setRequestInterception: async () => {}, goto: async () => { - throw new Error('net::ERR_CONNECTION_REFUSED at https://example.com/'); + throw error; }, + // the heap monitor polls this evaluate: async () => 0, close: async () => { closes.page++; @@ -170,6 +180,25 @@ test('an unreachable remote site is reported, not thrown', async () => { }), } as unknown as pup.Browser; + return { browser, closes }; +} + +/** A Chrome navigation failure, which arrives as a plain `Error`. */ +function netError(code: string): Error { + return new Error(`${code} at https://example.com/`); +} + +/** + * The other half of the pair, so the tests above are not satisfied by "throw + * everything". A site that refuses the connection is something we *did* + * observe, so it comes back as a status rather than an exception -- and it is + * a 503, the very code the sidecar-failure path must not borrow. + */ +test('an unreachable remote site is reported, not thrown', async () => { + const { browser, closes } = browserWhoseNavigationFails( + netError('net::ERR_CONNECTION_REFUSED'), + ); + const result = await render(browser); assert.equal(result.status, 503); @@ -181,3 +210,111 @@ test('an unreachable remote site is reported, not thrown', async () => { ); assert.deepEqual(closes, { page: 1, context: 1 }); }); + +/** + * A certificate the site presents badly is likewise the site's doing, and the + * contract is entitled to see it: a contract may legitimately branch on "that + * host is not serving valid TLS". + */ +test('a certificate error is reported, not thrown', async () => { + const { browser, closes } = browserWhoseNavigationFails( + netError('net::ERR_CERT_AUTHORITY_INVALID'), + ); + + const result = await render(browser); + + assert.equal(result.status, 495); + assert.match(String(result.body), /SSL certificate error/); + assert.deepEqual(closes, { page: 1, context: 1 }); +}); + +// -- the two ambiguous cases stay on the observation channel ---------- + +/** + * Regression guard, not a description of a certainty. + * + * A name that does not resolve is either a domain that does not exist -- the + * site -- or a resolver of *ours* that is broken. The two are indistinguishable + * from inside one validator, and resolving that ambiguity is exactly what + * several validators and an equivalence principle are for: pre-judging it here + * would suppress the disagreement consensus exists to reconcile. So it stays + * returned and contract-visible, and this test fails if someone later + * "helpfully" reclassifies it as ours the way `ERR_INTERNET_DISCONNECTED` is. + */ +test('a name that does not resolve is still reported, not thrown', async () => { + const { browser, closes } = browserWhoseNavigationFails( + netError('net::ERR_NAME_NOT_RESOLVED'), + ); + + const result = await render(browser); + + assert.equal(result.status, 502); + assert.match(String(result.body), /DNS resolution failed/); + assert.equal( + statusIsGood(result.status), + false, + 'the module must see this as a failed load', + ); + assert.deepEqual(closes, { page: 1, context: 1 }); +}); + +/** + * The same guard for the other ambiguous case: a navigation timeout is either + * a slow site or a browser of ours that wedged, and it is not this process's + * place to decide which. Returned, not thrown. + */ +test('a navigation timeout is still reported, not thrown', async () => { + const { browser, closes } = browserWhoseNavigationFails( + new TimeoutError('Navigation timeout of 30000 ms exceeded'), + ); + + const result = await render(browser); + + assert.equal(result.status, 408); + assert.match(String(result.body), /Navigation timeout/); + assert.equal( + statusIsGood(result.status), + false, + 'the module must see this as a failed load', + ); + assert.deepEqual(closes, { page: 1, context: 1 }); +}); + +// -- and the one navigation error that is ours ------------------------ + +/** + * `net::ERR_INTERNET_DISCONNECTED` is not ambiguous: the request never left + * this host, so there is no observation to reconcile with anyone. It must + * leave by the thrown channel, which the caller turns into a `500` and the + * module into a fatal error, so the validator abstains. + * + * The message must say so in its own words. It is all that reaches the `500` + * body, and from there the module error's `ctx` and the operator-facing abort + * string; "webdriver unavailable" would send an operator to look at a browser + * that is working fine. + */ +test('a local network outage is thrown, not reported', async () => { + const { browser, closes } = browserWhoseNavigationFails( + netError('net::ERR_INTERNET_DISCONNECTED'), + ); + + await assert.rejects(render(browser), (e: Error) => { + assert.match( + e.message, + /local network/i, + 'the message must name the fault an operator has to go and fix', + ); + assert.match(e.message, /net::ERR_INTERNET_DISCONNECTED/); + assert.doesNotMatch( + e.message, + /webdriver unavailable/i, + 'the browser answered, so it must not be reported as the broken part', + ); + return true; + }); + assert.deepEqual( + closes, + { page: 1, context: 1 }, + 'the page and its context must still be torn down', + ); +});