fix(web): Treat webdriver sidecar failures as non-fatal - #21
fix(web): Treat webdriver sidecar failures as non-fatal#21MuncleUscles wants to merge 4 commits into
Conversation
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.
GenVM PR actionsTick a box to run it (the box unticks itself when handled). Actions only run while the PR has the
MergeRequires, on the exact head commit:
Full CI runs only when Every repo lands ONE squashed commit, subject Commands
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change extracts WebDriver rendering into a dedicated module, classifies WebDriver failures, centralizes wire-result conversion, and adds coverage for rendering, request statuses, cleanup, and error propagation. ChangesWeb error flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WebRequest
participant LuaRender
participant WebDriver
participant VM
participant WireResult
WebRequest->>LuaRender: invoke Render
LuaRender->>WebDriver: request page rendering
WebDriver-->>LuaRender: rendered result or classified failure
LuaRender-->>VM: return ModuleError
VM->>WireResult: convert handler error
WireResult-->>WebRequest: return UserError or FatalError
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Following Kira's question in Slack — I think she's right, and it changes what this PR should assert rather than whether it should exist. Suggesting modify, not close: the harness is worth keeping and most of the analysis holds. Only the central assertion is inverted. The assertion is backwards
Both produce valid, non-error results that disagree. The broken host has laundered its own infrastructure failure into a legitimate-looking consensus result — and a contract can do it deliberately: Fatal → Same reasoning kills suggestion 4: a third I'd also drop this framing from the body:
A Timeout vote isn't a verdict on the transaction. It's the host reporting it didn't complete — which is accurate. What this PR found that is realThe misclassification, which survives untouched:
That's the actual defect. And note why Fix directions 1 and 2 should come out. Both make the sidecar's own failures contract-visible, which is the thing above. Concretely
Inverting beats deleting. It turns the test into a regression guard that encodes this decision, and stops someone later implementing directions 1 or 2 — a realistic risk precisely because the current body argues for them well. Then: retitle (no longer red by design), drop directions 1/2/4 from the body, promote direction 3 to the fix. One blocker in your "Not covered" list has since cleared:
#23 adds Related change in #23#23 originally had this same bug and I've just fixed it there. It added two failure classes and routed both through
The general rule, which I think is what this PR was reaching for:
Corollary that bites existing code: any contract-visible limit has to be protocol-uniform. Caveat on my side: the |
|
Correction to my comment above, and a finding that strengthens this PR's site 1. I endorsed fix direction 3 wholesale. Only two thirds of it is right.
The mapping half is wrong, by the same reasoning that makes the test assertion wrong. What is right: ensure cleanup, and set an explicit And site 1 is worse than described. It isn't only a misclassification — it's a leak: const context = await browserInstance.createBrowserContext();
const page = await context.newPage(); // throws here...
try {
...
} finally {
await Promise.allSettled([page.close(), context.close()]); // ...so this never runs
}The Fixed in #23 ( Net for this PR: no fix needed in One thing I could not verify: inducing a real |
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.
MuncleUscles
changed the title
Completed — fix added, coverage closed, out of draftThe branch opened as a red-by-design repro. It now carries the fix and the four coverage gaps are closed. Rust 115 passed / 0 failed across 16 binaries; TypeScript 5/5. Where the fix lives, and why not the obvious placeEnforcement is at the Lua
Sidecar side — better classification, not just softer
Correcting the assumption this work started from: puppeteer-core 24.16.0 defaults to 180s and Coverage gaps, closed
Red-checksEvery new test was verified non-vacuous by reverting each fix in turn: Two consequences for a reviewer, not buriedA validator whose own sidecar is broken now computes a result (a catchable Diagnosability: the re-raise keeps Not coveredThe last hop —
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webdriver/src/prj/src/index.ts (1)
41-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait the render before releasing the browser handle.
BrowserHolder.close()can reduce the reference count to zero during browser rotation and close the browser whilerenderPageWithBrowseris still running. Returnawait renderPageWithBrowser(...)so the render keeps its reference until completion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webdriver/src/prj/src/index.ts` around lines 41 - 50, Update the try/finally flow around renderPageWithBrowser to await its completion before BrowserHolder.close() runs, preserving the browser reference for the full render operation while keeping the existing cleanup in finally.
🧹 Nitpick comments (1)
webdriver/src/prj/package.json (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate the ESM test setup away from
--loader.
ts-nodeis already declared indevDependencies. Node 20.6+ deprecates--loader; use a supported--importregistration setup. Becauseengines.nodeallows versions before 18.19.0, either raise the minimum version or retain compatibility for those versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webdriver/src/prj/package.json` around lines 10 - 11, Update the package.json test script to replace the deprecated ts-node --loader registration with a supported --import setup, using the existing ts-node devDependency. Ensure the chosen registration works with the declared engines.node range, or raise that minimum to Node 18.19.0+ if older-version compatibility cannot be retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@implementation/src/web/tests.rs`:
- Around line 367-369: Update the documentation comment immediately above the
companion test to describe its actual setup: it constructs a plain anyhow error
and renders a valid URL against the 404 fixture, while preserving the note that
the fatal path remains reachable and ends in FatalError. Remove the inaccurate
claim about malformed URLs bypassing the sidecar.
In `@webdriver/src/prj/src/render.ts`:
- Around line 269-275: Handle all promise rejections in the render setup around
the targetcreated listener and page.setViewport: add catch handling to the async
listener covering target.page() and ssrf.installSsrfGuard(p), and await
setViewport within the surrounding guarded flow. Ensure failures are caught and
handled without creating floating promises or terminating the sidecar process.
---
Outside diff comments:
In `@webdriver/src/prj/src/index.ts`:
- Around line 41-50: Update the try/finally flow around renderPageWithBrowser to
await its completion before BrowserHolder.close() runs, preserving the browser
reference for the full render operation while keeping the existing cleanup in
finally.
---
Nitpick comments:
In `@webdriver/src/prj/package.json`:
- Around line 10-11: Update the package.json test script to replace the
deprecated ts-node --loader registration with a supported --import setup, using
the existing ts-node devDependency. Ensure the chosen registration works with
the declared engines.node range, or raise that minimum to Node 18.19.0+ if
older-version compatibility cannot be retained.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45ea3ecb-611a-4008-a9a7-eefc8692d6f9
📒 Files selected for processing (11)
implementation/src/common/mod.rsimplementation/src/web/mod.rsimplementation/src/web/tests.rsimplementation/tests/request_status_fatality.rsinstall/config/genvm-web-default.luawebdriver/Dockerfilewebdriver/src/prj/package.jsonwebdriver/src/prj/src/browser/chrome.tswebdriver/src/prj/src/index.tswebdriver/src/prj/src/render.tswebdriver/src/prj/test/render.test.ts
| 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 }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle rejections from the targetcreated listener and setViewport.
Line 269 registers an async event listener. Line 275 calls page.setViewport without await. Both produce floating promises. If target.page(), installSsrfGuard(p), or setViewport rejects, Node 20 raises an unhandled rejection, and the default --unhandled-rejections=throw mode terminates the sidecar process. That converts a single bad render into a full sidecar outage, which the module then reports as a connection failure for every concurrent request.
Await setViewport inside the guarded block, and add a catch to the listener body.
🛡️ Proposed fix
await ssrf.installSsrfGuard(page);
context.on('targetcreated', async (target) => {
- const p = await target.page();
- if (p && p !== page) {
- await ssrf.installSsrfGuard(p);
- }
+ try {
+ const p = await target.page();
+ if (p && p !== page) {
+ await ssrf.installSsrfGuard(p);
+ }
+ } catch (error) {
+ logger.log('error', 'could not guard a new target', {
+ url: targetUrl,
+ error: (error as Error).message,
+ });
+ }
});
- page.setViewport({ width: 1920 / 2, height: 1080 / 2 });
+ await page.setViewport({ width: 1920 / 2, height: 1080 / 2 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 }); | |
| context.on('targetcreated', async (target) => { | |
| try { | |
| const p = await target.page(); | |
| if (p && p !== page) { | |
| await ssrf.installSsrfGuard(p); | |
| } | |
| } catch (error) { | |
| logger.log('error', 'could not guard a new target', { | |
| url: targetUrl, | |
| error: (error as Error).message, | |
| }); | |
| } | |
| }); | |
| await page.setViewport({ width: 1920 / 2, height: 1080 / 2 }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webdriver/src/prj/src/render.ts` around lines 269 - 275, Handle all promise
rejections in the render setup around the targetcreated listener and
page.setViewport: add catch handling to the async listener covering
target.page() and ssrf.installSsrfGuard(p), and await setViewport within the
surrounding guarded flow. Ensure failures are caught and handled without
creating floating promises or terminating the sidecar process.
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.
Reworked — sidecar faults stay fatal, and are now labelled
The The TypeScript Diagnosability — the defect actually worth fixing
Red-checks — the suite bites in both directions
The sharpest pair: Recommendation: no wait-and-retry hereAsked for and argued, not assumed:
Suggested order instead: land the label (prerequisite for any retry policy anywhere) → if retry is wanted, put it in the node, beside the manager-availability retry where the phase budget already lives → cheaper and better, make the sidecar queue Four things still worth attention
Also fixed in passing: 11 |
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.
|
| Revert | Result |
|---|---|
err.fatal = false in lib-web.lua |
7 fatal-side tests FAIL, all 5 non-fatal green |
WEBPAGE_LOAD_FAILED made fatal |
5 non-fatal tests FAIL, all 7 fatal-side green |
Plus, per case: restoring the 503 mapping → Missing expected rejection; reclassifying name-not-resolved → reclassified as ours: DNS resolution failed; reclassifying timeout → reclassified as ours: navigation timeout; folding cert into 503 → actual: 503, expected: 495.
Three things for your attention
ERR_PROXY_CONNECTION_FAILEDandnet::ERR_NETWORK_CHANGEDare as unambiguously ours as disconnected, and still fall through to the 418 teapot default on the observation channel. The ruling named one code, so this change implements one code. Worth a follow-up decision.- Liveness cost worth telling ops before this ships. Chrome derives
ERR_INTERNET_DISCONNECTEDfrom its own network-change detector, which misfires on some container/VM setups with no default route. On such a host every render now aborts the run instead of returning a catchable status, so the node abstains repeatedly. Correct semantics, but it converts a contract-visible failure into a repeated-timeout liveness event. - The operator-facing message survives by accident, not design. The transport puts the sidecar body into
ctxasGenericValue::Bytes, which would render as a numeric array in theFatalErrorJSON; it arrives readable only because thepcallround trip through Lua converts it to aStr. A wire test now pins that, but the underlying dependency is fragile and nothing else documents it.
github-actions
Bot
added
the
not rebased
1 participant
The defect
When our own WebDriver sidecar fails — as opposed to the remote website failing — the failure is classified as fatal, aborting the entire contract run as an internal error. The contract cannot catch it. An environmental blip on infrastructure we operate is treated as a deterministic property of the transaction.
Three sites combine:
webdriver/src/prj/src/index.ts:247—const page = await context.newPage();sits outside thetrythat begins at:249.newPage()issues CDPTarget.createTarget; puppeteer's default 180sprotocolTimeoutmakes it throwProtocolError: Target.createTarget timed out, which escapes to the outer handler and becomes HTTP 500.install/config/genvm-web-default.lua—Rendercallslib.rs.requestwitherror_on_status = trueand nopcall, unlikeRequestwhich pcalls and re-raises withfatal = false.implementation/src/scripting/mod.rs:345-348— non-200 yieldsModuleError { causes: [STATUS_NOT_OK], fatal: true }, which becomes a bareanyhowin the executor and surfaces asResultCode::InternalError.The non-fatal
WEBPAGE_LOAD_FAILEDbranch further down inRenderis unreachable for a 5xx, because the fatal raise happens first insidelib.rs.request. The trace logs make the divergence explicit — the control test reachesgenvm-web-default.lua:36, the 5xx case dies at:19.By contrast, a genuine page-load failure is handled correctly today: the sidecar returns
200+Resulting-Status: 408/418/495/502/503, which is non-fatal and surfaces as a catchableNondetException. Only classes (a) transport failure to the sidecar and (b) 5xx from the sidecar are misclassified.Why this matters
GenVM has a single
fatal: boolaxis, wherefatalmeans only "don't show it to the contract". There is no third state meaning environmental — do not turn this into a consensus result at all. On the node's duty path,consensus/genvm.goconverts any GenVM error into a bare Timeout vote, so a sidecar outage becomes a consensus-visible verdict on a transaction that never actually failed.Context: this was found while investigating the Bradbury-phase1 freeze (genlayer-node #1693 / #1694). It was not the cause of that incident — sync replays nondeterministic blocks from recorded
eqBlocksOutputsand never contacts the webdriver — but it is a real defect on the live execution path, and it is what made the incident look like a webdriver problem.The tests
Two tests in
implementation/src/web/mod.rs, deliberately paired so the red one is not vacuous:render_remote_page_load_failure_is_not_fatal200+Resulting-Status: 404render_sidecar_internal_error_is_not_fatal500, noResulting-StatusThe 500 body is byte-accurate to what the outer
catchinindex.tsactually emits ({"error":"Internal server error","message":"Target.createTarget timed out"}).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 site or the Rust site turns it green without the test encoding which one you pick.
Hermetic: a loopback
TcpListenerimpersonates the sidecar and drives the real productionRenderlua through the realscripting::UserVMwiring. No Chromium, no egress past loopback.Suggested fix directions (not applied)
lib.rs.requestcall inRenderinpcall+ re-raise withfatal = false, exactly asRequestalready does. Config-file change, no Rust rebuild.error_on_statusproducefatal: falsefor 5xx specifically, since a 5xx from our own sidecar is never a contract-attributable observation.newPage()inside thetryin the webdriver and mapProtocolError/TimeoutErrorto aResulting-Statuson an HTTP 200, so it joins the already-correct page-load path. Set an explicitprotocolTimeoutbelow the module's request deadline.environmental) that the node must not convert into a vote.Not covered
catchproduces; the TypeScript is never executed, becausewebdriver/src/prjhas no test runner (deps are puppeteer-core + commander, scripts are onlystart/dev/build). I did not scaffold one.fatal:true→FatalError→ bare anyhow →ResultCode::InternalError) — the test stops at the module boundary and asserts onModuleError.fatal.400on bad params,503from the healthcheck path), which take the same fatal branch.implementation/src/scripting/mod.rs:448-451.Summary by CodeRabbit
Bug Fixes
Improvements