fix(e2e): give axe.run() a dedicated 120s eval timeout (bookshelf-t4ftz) #1336

Merged

zombor merged 1 commit from bd-bookshelf-t4ftz into main 2026-08-05 02:12:26 +00:00

Conversation 2 Commits 1 Files changed 1 +14 -1

zombor commented 2026-08-05 02:01:29 +00:00

Owner

Copy link

Root cause

axe.run() is a CPU-intensive JS Promise that serialises the full WCAG rule-set result. Under CI CPU contention it routinely exceeds the 60s pageTimeout (which covers navigation + DOM stability), causing page.Eval(runJS) to return context deadline exceeded — surfacing as [FAIL] at axe_helpers_test.go:229.

The inherited page context deadline (set by refreshPageTimeout for navigation) was also being used for the axe.run() Promise eval. Navigation + DOM-stability typically takes <5s, leaving only the remainder of 60s for the actual axe scan — not enough under contention.

Fix

Inside runAxe, call page.CancelTimeout().Timeout(axeEvalTimeout) (120s) before page.Eval(runJS) to give the scan a fresh, independent budget. This matches the CancelTimeout+Timeout pattern used throughout the browser e2e suite (refreshPageTimeout, per the go-rod Ordered-journey gotcha documented in CLAUDE.md) and does not affect navigation/stability timeouts in callers.

Confirmed not a real a11y violation

The [FAIL] at axe_helpers_test.go:229 is Expect(err).NotTo(HaveOccurred(), "runAxe: axe.run() evaluation failed") — the error is a Go context error from the deadline expiring, not an axe violation report. A real violation would fail at the Fail(msg.String()) call later in the function with a descriptive message listing rule IDs and nodes.

Test plan

• CI E2E Browser job passes (axe.run() gets 120s instead of inheriting a near-exhausted 60s navigation deadline)
• go vet -tags e2e ./e2e/browser/ clean
• go build ./... clean

Closes bead bookshelf-t4ftz on merge.

## Root cause `axe.run()` is a CPU-intensive JS Promise that serialises the full WCAG rule-set result. Under CI CPU contention it routinely exceeds the 60s `pageTimeout` (which covers navigation + DOM stability), causing `page.Eval(runJS)` to return `context deadline exceeded` — surfacing as `[FAIL]` at `axe_helpers_test.go:229`. The inherited page context deadline (set by `refreshPageTimeout` for navigation) was also being used for the axe.run() Promise eval. Navigation + DOM-stability typically takes <5s, leaving only the remainder of 60s for the actual axe scan — not enough under contention. ## Fix Inside `runAxe`, call `page.CancelTimeout().Timeout(axeEvalTimeout)` (120s) before `page.Eval(runJS)` to give the scan a fresh, independent budget. This matches the `CancelTimeout+Timeout` pattern used throughout the browser e2e suite (`refreshPageTimeout`, per the go-rod Ordered-journey gotcha documented in CLAUDE.md) and does not affect navigation/stability timeouts in callers. ## Confirmed not a real a11y violation The `[FAIL]` at `axe_helpers_test.go:229` is `Expect(err).NotTo(HaveOccurred(), "runAxe: axe.run() evaluation failed")` — the error is a Go context error from the deadline expiring, not an axe violation report. A real violation would fail at the `Fail(msg.String())` call later in the function with a descriptive message listing rule IDs and nodes. ## Test plan - [x] CI E2E Browser job passes (axe.run() gets 120s instead of inheriting a near-exhausted 60s navigation deadline) - [x] `go vet -tags e2e ./e2e/browser/` clean - [x] `go build ./...` clean Closes bead bookshelf-t4ftz on merge.

zombor added 1 commit 2026-08-05 02:01:29 +00:00

fix(e2e): give axe.run() a dedicated 120s eval timeout to stop context deadline exceeded (bookshelf-t4ftz) ... 9b7db0b1a7

axe.run() is a CPU-intensive JS Promise that serialises the full WCAG
rule-set result. Under CI CPU contention it routinely exceeds the 60s
pageTimeout (which covers navigation + DOM stability), causing
page.Eval(runJS) to return "context deadline exceeded" and the A11Y
guardrails It to fail with [FAIL] at axe_helpers_test.go:229.

Root cause: runAxe used the page's inherited context deadline (set by
refreshPageTimeout for navigation) for the axe.run() Promise eval too.
Navigation + DOM-stability typically takes <5s, leaving only the
remainder of 60s for the actual axe scan — not enough under contention.

Fix: inside runAxe, call page.CancelTimeout().Timeout(axeEvalTimeout)
(120s) before page.Eval(runJS) to give the scan a fresh, independent
budget. This matches the CancelTimeout+Timeout pattern used throughout
the browser e2e suite (refreshPageTimeout, journey comments on go-rod
Ordered gotcha) and does not affect navigation/stability timeouts in
callers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-08-05 02:11:00 +00:00

Author

Owner

Copy link

Code Review — bookshelf-t4ftz / PR #1336

CI status: success (all required checks green).
Mergeable: True.

---

Phase 1: Spec Compliance

The bead description says: determine whether the axe-core scan is genuinely slow (needs a bigger deadline / go-rod page.Timeout reset per It per CLAUDE.md go-rod gotcha) or a real violation. Fix root cause; do not just bump the timeout without understanding.

Root cause determination is evidenced in the bead completion comment: "context deadline exceeded during axe.run() Promise eval under CI CPU contention — the page's inherited navigation deadline was expiring before the CPU-intensive axe scan completed." The fix correctly addresses this by giving the eval its own dedicated deadline rather than bumping the shared navigation timeout. Scope is appropriate.

---

Phase 2: Code Quality

Correctness of go-rod timeout handling

page.CancelTimeout() returns a new *rod.Page clone whose context has been cancelled (the deadline removed), and .Timeout(axeEvalTimeout) then attaches a fresh 120s absolute deadline to that clone. The original page variable (the outer closure variable shared across It steps) is NOT reassigned — axePage is a local. This is the correct pattern: runAxe receives page by value, creates axePage locally, evaluates, and returns. The outer page variable retains whatever deadline refreshPageTimeout set at the top of the It (via the BeforeEach + inline reassignments like page = refreshPageTimeout(page)). So runAxe does NOT clobber the journey-level page deadline — subsequent It steps start with refreshPageTimeout anyway per the BeforeEach. No state leak.

Does 120s paper over a real a11y violation?

No. The timeout only gates the Eval call — i.e., whether axe.run() completes before Go's context deadline fires. If axe.run() finishes and reports violations, the result flows into runAxe's violation parsing logic exactly as before. The stale-baseline check and the unexpected-violation assertion are both downstream of the eval; a genuine new violation still fails the test at the Fail(msg.String()) call in runAxe. The 120s budget cannot mask a real a11y regression.

Could this hide a genuine hang?

Under a genuine hang (e.g., axe.run() never resolving because the page is broken), the test would now wait 120s before failing instead of 60s. The cost is 60 additional seconds of CI time per hung spec, not a masked failure — it still fails. Acceptable.

Page reuse across Ordered journey (CLAUDE.md go-rod gotcha)

The BeforeEach in the calling spec (journey_a11y_guardrails_test.go:101) always calls page = refreshPageTimeout(page) at the start of every It, which resets the page's absolute deadline to a fresh pageTimeout (60s). The axePage local in runAxe is derived from the per-call page argument and is never stored back. After runAxe returns, subsequent calls inside the same It (e.g., next MustNavigate, next runAxe) work against the same page that had refreshPageTimeout called at the top of the step — not against the expired axePage context. The fix is correctly scoped.

Convention checks

• package browser_test: correct black-box test package.
• "time" import: newly added, correctly placed.
• const axeEvalTimeout declared at top-level with a clear doc comment: follows var-at-top spirit (constants are hoisted by nature). Naming is clear.
• No .golangci.yml exclusions added.
• No new white-box test patterns.
• No coverage exclusion changes.

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — bookshelf-t4ftz / PR #1336 **CI status:** `success` (all required checks green). **Mergeable:** `True`. --- ### Phase 1: Spec Compliance The bead description says: determine whether the axe-core scan is genuinely slow (needs a bigger deadline / go-rod page.Timeout reset per It per CLAUDE.md go-rod gotcha) or a real violation. Fix root cause; do not just bump the timeout without understanding. Root cause determination is evidenced in the bead completion comment: "context deadline exceeded during axe.run() Promise eval under CI CPU contention — the page's inherited navigation deadline was expiring before the CPU-intensive axe scan completed." The fix correctly addresses this by giving the eval its own dedicated deadline rather than bumping the shared navigation timeout. Scope is appropriate. --- ### Phase 2: Code Quality **Correctness of go-rod timeout handling** `page.CancelTimeout()` returns a new `*rod.Page` clone whose context has been cancelled (the deadline removed), and `.Timeout(axeEvalTimeout)` then attaches a fresh 120s absolute deadline to that clone. The original `page` variable (the outer closure variable shared across `It` steps) is NOT reassigned — `axePage` is a local. This is the correct pattern: `runAxe` receives `page` by value, creates `axePage` locally, evaluates, and returns. The outer `page` variable retains whatever deadline `refreshPageTimeout` set at the top of the `It` (via the `BeforeEach` + inline reassignments like `page = refreshPageTimeout(page)`). So `runAxe` does NOT clobber the journey-level page deadline — subsequent `It` steps start with `refreshPageTimeout` anyway per the `BeforeEach`. No state leak. **Does 120s paper over a real a11y violation?** No. The timeout only gates the `Eval` call — i.e., whether `axe.run()` completes before Go's context deadline fires. If axe.run() finishes and reports violations, the result flows into `runAxe`'s violation parsing logic exactly as before. The stale-baseline check and the unexpected-violation assertion are both downstream of the eval; a genuine new violation still fails the test at the `Fail(msg.String())` call in `runAxe`. The 120s budget cannot mask a real a11y regression. **Could this hide a genuine hang?** Under a genuine hang (e.g., `axe.run()` never resolving because the page is broken), the test would now wait 120s before failing instead of 60s. The cost is 60 additional seconds of CI time per hung spec, not a masked failure — it still fails. Acceptable. **Page reuse across Ordered journey (CLAUDE.md go-rod gotcha)** The `BeforeEach` in the calling spec (`journey_a11y_guardrails_test.go:101`) always calls `page = refreshPageTimeout(page)` at the start of every `It`, which resets the page's absolute deadline to a fresh `pageTimeout` (60s). The `axePage` local in `runAxe` is derived from the per-call `page` argument and is never stored back. After `runAxe` returns, subsequent calls inside the same `It` (e.g., next `MustNavigate`, next `runAxe`) work against the same `page` that had `refreshPageTimeout` called at the top of the step — not against the expired `axePage` context. The fix is correctly scoped. **Convention checks** - `package browser_test`: correct black-box test package. - `"time"` import: newly added, correctly placed. - `const axeEvalTimeout` declared at top-level with a clear doc comment: follows var-at-top spirit (constants are hoisted by nature). Naming is clear. - No `.golangci.yml` exclusions added. - No new white-box test patterns. - No coverage exclusion changes. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-05 02:11:12 +00:00

Author

Owner

Copy link

Security Review — PR #1336 (bookshelf-t4ftz)

Diff scope: e2e/browser/axe_helpers_test.go only — adds axeEvalTimeout = 120 * time.Second constant and changes runAxe to call page.CancelTimeout().Timeout(axeEvalTimeout) before axePage.Eval(runJS).

---

Surface analysis

Is runJS attacker-influenced? No. runJS is a package-level const string literal hardcoded in the source. It is never interpolated from test inputs, environment variables, or page content. No injection surface exists.

Does injectAxe execute attacker-controlled code? No. loadAxeScript() reads testdata/axe.min.js from a repo-committed path via runtime.Caller(0) — a fixed relative path to a file checked into source control. The content is never user-supplied.

Does CancelTimeout().Timeout(120s) affect scope beyond the axe Promise? The CancelTimeout() call resets the deadline on a clone of the page (rod's Timeout/CancelTimeout methods return a new *rod.Page value with a modified context — they do not mutate the original page pointer). The result is assigned to the local axePage variable; subsequent operations in the same It step that use the outer page variable are unaffected. This matches the existing refreshPageTimeout pattern in browser_suite_test.go.

Does the A11Y journey gate any security-relevant assertion? The journey (journey_a11y_guardrails_test.go) calls runAxe exactly once in Step 1 (the WCAG axe scan). CSP checking is fully separate and lives in browser_csp_helpers_test.go via the cspViolationCollector / proto.AuditsEnable path — it is not wired through runAxe and is unaffected by this timeout change. There is no scenario where a longer axe timeout could cause a security-relevant assertion (CSP, header check, etc.) to silently pass.

Production code path? Zero. The file carries //go:build e2e and package browser_test. It is never compiled into the application binary.

Secrets / PII in test? None. The diff adds only a time.Duration constant and three lines rewiring the eval call. No tokens, credentials, or PII appear.

---

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1336 (bookshelf-t4ftz) **Diff scope:** `e2e/browser/axe_helpers_test.go` only — adds `axeEvalTimeout = 120 * time.Second` constant and changes `runAxe` to call `page.CancelTimeout().Timeout(axeEvalTimeout)` before `axePage.Eval(runJS)`. --- ### Surface analysis **Is `runJS` attacker-influenced?** No. `runJS` is a package-level `const` string literal hardcoded in the source. It is never interpolated from test inputs, environment variables, or page content. No injection surface exists. **Does `injectAxe` execute attacker-controlled code?** No. `loadAxeScript()` reads `testdata/axe.min.js` from a repo-committed path via `runtime.Caller(0)` — a fixed relative path to a file checked into source control. The content is never user-supplied. **Does `CancelTimeout().Timeout(120s)` affect scope beyond the axe Promise?** The `CancelTimeout()` call resets the deadline on a **clone** of the page (rod's `Timeout`/`CancelTimeout` methods return a new `*rod.Page` value with a modified context — they do not mutate the original page pointer). The result is assigned to the local `axePage` variable; subsequent operations in the same `It` step that use the outer `page` variable are unaffected. This matches the existing `refreshPageTimeout` pattern in `browser_suite_test.go`. **Does the A11Y journey gate any security-relevant assertion?** The journey (`journey_a11y_guardrails_test.go`) calls `runAxe` exactly once in Step 1 (the WCAG axe scan). CSP checking is fully separate and lives in `browser_csp_helpers_test.go` via the `cspViolationCollector` / `proto.AuditsEnable` path — it is not wired through `runAxe` and is unaffected by this timeout change. There is no scenario where a longer axe timeout could cause a security-relevant assertion (CSP, header check, etc.) to silently pass. **Production code path?** Zero. The file carries `//go:build e2e` and `package browser_test`. It is never compiled into the application binary. **Secrets / PII in test?** None. The diff adds only a `time.Duration` constant and three lines rewiring the eval call. No tokens, credentials, or PII appear. --- No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor merged commit 024ef2d817 into main 2026-08-05 02:12:26 +00:00

zombor referenced this pull request from a commit 2026-08-05 02:12:32 +00:00

Merge pull request 'fix(e2e): give axe.run() a dedicated 120s eval timeout (bookshelf-t4ftz)' (#1336) from bd-bookshelf-t4ftz into main

zombor referenced this pull request 2026-08-05 02:14:00 +00:00

fix(js-test): harden timing-fragile metron detail-fetch tests with vi.waitFor (bookshelf-wro8e) #1337

zombor referenced this pull request 2026-08-05 02:53:52 +00:00

CI test-MySQL tuning: redo capacity + buffer pool + io_capacity + table caches (bookshelf-7w1mn) #1335

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1336

No description provided.