Bound what a run can consume, and stop doing work it never needed - #22
Merged
Conversation
A run had no enforceable ceiling on time or on memory. All three of these end the same way — a CI job that never finishes, or one that dies taking every finding with it — and none of them needed a hostile input to happen. The per-page timeout could not stop the thing it was written for. It is a Promise.race, and a race cannot interrupt synchronous work: neither jsdom's parse nor axe-core's walk of the tree yields, so the timer that is meant to stop them never runs. Measured on a 120,000-element document with a two-second ceiling, the audit was still going more than ten minutes later, and because the pool waits on its workers the whole run went with it. The supervisor now keeps its own deadline and terminates the thread that overruns it, which is the only thing that stops synchronous work; the page is recorded as unaudited rather than left looking clean, and the rest of the run carries on. Two configurations still have no hard ceiling because both refuse the threads that would carry one — --concurrency 1, and a machine with too few cores to spare one — and that is now written down rather than implied. Nothing capped the size of an input. readFile and response.text() both buffer whatever they are given, so a generated catalogue, a database export with an .html extension, or a server that does not stop sending was an out-of-memory crash rather than a report. Both paths now cap at 32 MB, and an over-size page is reported as unmeasured through the channel that already exists for pages a run could not read. robots.txt and sitemap.xml were fetched with no timeout at all, while every other request had one. They are fetched before any page is, so a server that accepts the connection and never answers left the crawl waiting before it had reported a single page. They now share the request timeout, refuse a redirect that leaves the origin as fetchPage already does, and are capped like any other body. The per-page timeout constant moves to result.ts so the pool can read it without pulling 630 ms of jsdom into a module that deliberately avoids it — the same move ENGINE_BLIND_RULES already made. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xCGmHyxdTMCF2GFwr4CAm
…ng asks about Both of these were work the run did not need to do. --browser took pages strictly one at a time while the browserless engine had a whole measured worker pool, which had it backwards: the browser is the slow engine, and it spends most of a page waiting on the stylesheets and images it fetches rather than on the CPU, so waiting on four at once is close to free. Over 24 pages of a styled site, 17.9 s became 7.7 s. --concurrency now sets this too — threads without --browser, open tabs with it. Four is the default because each open page holds a document tree, its decoded images and its own copy of axe-core, so Chromium's memory is the limit rather than cores. Results are placed by position rather than pushed as they arrive, and a test asserts that runs at 1, 4 and 8 tabs produce the same report page for page. The component index exists to answer one question — which file a failing element was written in — and a run that found nothing to fix never asks it. It was built anyway, before either report rendered, reading up to two thousand source files. Measured on a 1500-file project auditing one clean page: around 450 ms and 20 MB spent on lookups that never happened, scaling with the size of the source tree rather than with anything the run actually did. It is now built only when there is an element to attribute, with tests pinning both sides: attribution still names the file when there is a violation, and a clean run says nothing about source at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xCGmHyxdTMCF2GFwr4CAm
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Most of what a short audit costs is not the audit. Importing jsdom is ~700 ms on a warm page cache and axe-core another ~130 ms to import and compile, and V8 pays to parse and compile both from source on every single run — of a CLI that build scripts invoke over and over against code that has not changed since last time. Node can cache the compiled bytecode instead, which is the shape of this problem exactly. Enabled in the CLI entry and again in the audit worker, because the cache is per-thread and each worker compiles its own copy of both libraries. Measured over nine runs per configuration, median: a one-page audit goes 1397 ms to 1256 ms, and a fifty-page one is unchanged at 4.7 s. That is the expected shape — it is a fixed cost, so it counts for most on the small sites where fixed cost is most of the run, and disappears into the per-page work on large ones. An earlier three-run sample suggested the large case regressed 7%; nine runs put the two within noise of each other. Best-effort by design. A read-only filesystem, a sandbox that forbids the directory, or a Node built without the feature all mean the run is a little slower, which is not a reason to fail an audit. Node's own NODE_DISABLE_COMPILE_CACHE is honoured by the call itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xCGmHyxdTMCF2GFwr4CAm
The browserless engine runs every rule and then throws some of the answers away. Colour contrast is computed against a stylesheet jsdom never fetched, target size against boxes that are all 0x0, and shapeResults discards both as untrustworthy. The work to produce a discarded verdict is not cheap: colour contrast is the most expensive rule axe-core has, and the blind set is 14-19% of a page, 8-10% of a whole run once start-up is counted in. --fast switches those rules off instead of running them. The verdict does not move, which is the part that had to be got right. shapeResults already had a pass for rules axe-core skipped entirely — the preload-dependent ones arrive that way — so a disabled rule falls into it and is reported as unevaluated with the same reason it always had. Three things are asserted rather than assumed: the violations are identical, no skipped rule ever reaches the passes bucket, and the coverage view is criterion-for-criterion the same. What is given up is the element list. A rule that runs can say which elements it could not decide, and those are exactly the ones a person then checks by hand; a rule that never ran cannot name them. That is the whole of the trade, and it is why this is a flag rather than the default — worth it on a run whose job is to fail a build, not on a run somebody will read. A test pins the loss so it is visible in the suite rather than discovered from an empty list in a report. No effect under --browser, which can decide those rules for real. Passing both warns instead of ignoring the flag quietly, since silence would leave somebody believing they had traded detail for speed when they had done neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012xCGmHyxdTMCF2GFwr4CAm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven changes from auditing the tool's own hot spots: three that bound what a run can consume, four that make it faster. Every number here is measured, not estimated.
Four commits, in that order.
The per-page timeout could not stop the thing it was written for
It is a
Promise.race, and a race cannot interrupt synchronous work. Neither jsdom's parse nor axe-core's walk of the tree yields, so the timer meant to stop them never gets to run.createDom()sat outside the timeout entirely.Measured: a 120,000-element document with a two-second ceiling was still running more than ten minutes later. Because the pool waits on its workers, the whole run went with it — a CI job hung until the platform killed it, which is exactly the failure the ceiling exists to prevent.
worker.terminate()is the only thing that stops synchronous work, so the pool supervisor now keeps its own deadline per page and kills the thread that overruns it. The page is recorded as unaudited rather than left looking clean, and the surviving workers carry the rest of the run. Same document, after: 7 s, with the two healthy pages in the run still audited normally.Two configurations still have no hard ceiling, because both refuse the threads that would carry one —
--concurrency 1, and a machine with too few cores to spare one. That is now written down indocs/audit.mdrather than implied.Nothing capped the size of an input
readFileandresponse.text()both buffer whatever they are given. A generated catalogue, a database export carrying an.htmlextension, or a server that does not stop sending was an out-of-memory crash rather than a report — taking every finding on every page that was fine with it.Both paths now cap at 32 MB. An over-size page is reported as unmeasured through the channel that already exists for pages a run could not read, so it reaches the completeness block instead of vanishing. The crawl counts bytes as they arrive and cancels the body, because
response.text()has already read everything by the time its length could be checked, and acontent-lengthheader is optional and unverified.For scale: the largest page in this repo's fixtures is 4 KB.
robots.txtandsitemap.xmlwere fetched with no timeoutEvery other request in the crawler had one. These two are fetched before any page is, so a server that accepts the connection and never answers left the crawl waiting before it had reported a single page. They now share the request timeout, refuse a redirect that leaves the origin as
fetchPagealready does, and are capped like any other body.--browsernow audits four pages at onceThis runner took pages strictly one at a time while the browserless engine had a whole measured worker pool — backwards, since the browser is the slow engine and spends most of a page waiting on the stylesheets and images it fetches rather than on the CPU.
--concurrency 1--concurrencynow sets both engines — threads without--browser, open tabs with it — and its help text says so. Four rather than more because each open page holds a document tree, its decoded images and its own copy of axe-core, so Chromium's memory is the limit rather than cores.Results are placed by position rather than pushed as they arrive. I verified the reports are byte-identical at 1, 4 and 8 tabs (after stripping the generation timestamp), and a test pins it.
A clean run no longer reads the project's source
The component index answers one question — which file a failing element was written in — and a run that found nothing to fix never asks it. It was built anyway, before either report rendered, reading up to two thousand source files.
Measured on a 1500-file project auditing one clean page: 1707 ms → 1235 ms and ~20 MB, scaling with the size of the source tree rather than with anything the run did. It is now built only when there is an element to attribute.
One correction worth flagging: I first reported this as a 3.3 s saving. That was a cold-page-cache artifact from having just written 30 MB of fixture files — repeated runs put the real figure at ~450 ms. Still worth having, and it grows with the source tree, but it is not the headline I first made it.
Compiled bytecode is reused between runs
Importing jsdom is ~700 ms and axe-core another ~130 ms to import and compile, and V8 paid to compile both from source on every invocation of a CLI that build scripts run over and over against unchanged code. Node's compile cache is enabled in the CLI entry and again in the audit worker, since the cache is per-thread and each worker compiles its own copy.
A one-page audit goes 1397 ms → 1256 ms; a fifty-page one is unchanged, which is the expected shape for a fixed cost. Best-effort: a read-only or sandboxed cache directory makes a run slightly slower and nothing else.
An earlier three-run sample suggested the fifty-page case regressed 7%. Nine runs put the two within noise of each other. Worth saying, because I nearly shipped that conclusion.
--fast, for skipping the rules this engine cannot decideThe browserless engine runs every rule and then throws some of the answers away: colour contrast is computed against a stylesheet jsdom never fetched, target size against boxes that are all 0x0, and
shapeResultsdiscards both as untrustworthy. Producing a discarded verdict is not cheap — colour contrast is the most expensive rule axe-core has.--fastThe rules themselves are 14–19% of a page; the fixed start-up cost dilutes that to 8–10% of a whole run, and the honest number is the second one.
The verdict does not move.
shapeResultsalready had a pass for rules axe-core skipped entirely — the preload-dependent ones arrive that way — so a disabled rule falls into it and is reported as unevaluated with the same reason it always had. Three things are asserted rather than assumed: the violations are identical, no skipped rule ever reaches thepassesbucket, and the coverage view is criterion-for-criterion the same.What is given up is the element list. A rule that runs can say which elements it could not decide, and those are exactly the ones a person then checks by hand; a rule that never ran cannot name them. That is the whole of the trade, and why it is a flag rather than the default — worth it on a run whose job is to fail a build, not on a run somebody will read. A test pins the loss so it shows up in the suite rather than as an empty array in a report.
No effect under
--browser, which can decide those rules for real; passing both warns rather than ignoring the flag quietly.Where this leaves the tool
Measured on 4 cores, Node 22.22, median of 3–9 runs:
Fixed cost is ~1 s, almost all of it
import jsdom, paid per thread. That is the floor until jsdom stops being loaded per process, and it is not addressed here.Validation
pnpm test— 49 files, 1085 passing, 2 skipped, browser suite included against real Chromiumpnpm lint,pnpm typecheck,pnpm smoke,pnpm test:packagedall cleanrobots.txtandsitemap.xml, off-origin sitemap redirect, browser lane equivalence and page ordering, source attribution pinned from both sides, and four--fastcases including the coverage equivalence and the detail it costsNo contract versions move: no field changed meaning.
🤖 Generated with Claude Code
https://claude.ai/code/session_012xCGmHyxdTMCF2GFwr4CAm