ci(e2e): restore BROWSER_PROCS 4->8 now proc-crash root cause is fixed (bookshelf-dc11r) #1300

Merged

zombor merged 1 commit from bd-bookshelf-dc11r into main 2026-07-29 15:00:09 +00:00

Conversation 2 Commits 1 Files changed 2 +32 -21

zombor commented 2026-07-29 13:58:43 +00:00

Owner

Copy link

Summary

• Restores BROWSER_PROCS from 4 to 8 in ci.yml — recovering ~27% wall-clock speed on the E2E Browser CI job (~3m48s vs ~5m14s locally).
• The two root causes of the proc-crash flake that forced the 8→4 reduction (lrlp6/#1242) are both fixed on main:
  1. worker_set rebuild (93f101eb): Worker.WaitForCompletion permanently closes the tasks channel; workerSet now rebuilds fresh Workers after drain so StartWorker→DrainWorker→StartWorker no longer panics with "send on closed channel".
  2. Chrome memory flags (lrlp6): --disable-gpu + 3 other flags reduce per-instance footprint from ~200-400 MB to ~100-200 MB, so 8 procs uses ~1.6 GB for Chrome vs the 1.6-3.2 GB that triggered kernel OOM kills.
• Memory safety at 8 procs: 8 × ~200 MB Chrome + 512 MB MySQL + 8 × ~80 MB Go ≈ 2.7 GB, within the 12-core runner's available headroom (no explicit memory limit per ci.yml).
• DB budget: 8 × 15 = 120 conns, well under max_connections=512.

Verification

Ran BROWSER_PROCS=8 make e2e-browser on macOS (18 GB RAM, Chrome) 5 times:

• All 5 runs: 305 passed, 0 proc crashes, 0 OOM kills, 0 "timed out waiting for procs"
• The single consistent macOS failure (a11y guardrails focus-in-modal) is macOS-specific and also fails at 4 procs — CI on Linux passes at both counts
• Wall-clock: avg 3m48s at 8 procs vs 5m14s at 4 procs

Test plan

• CI E2E Browser job goes green (proving 8 procs stable on Linux CI)
• E2E Browser wall-clock faster than the pre-change ~6m53s (from recent main CI run)

Closes bead bookshelf-dc11r on merge.

## Summary - Restores BROWSER_PROCS from 4 to 8 in ci.yml — recovering ~27% wall-clock speed on the E2E Browser CI job (~3m48s vs ~5m14s locally). - The two root causes of the proc-crash flake that forced the 8→4 reduction (lrlp6/#1242) are both fixed on main: 1. **worker_set rebuild (93f101eb):** Worker.WaitForCompletion permanently closes the tasks channel; workerSet now rebuilds fresh Workers after drain so StartWorker→DrainWorker→StartWorker no longer panics with "send on closed channel". 2. **Chrome memory flags (lrlp6):** --disable-gpu + 3 other flags reduce per-instance footprint from ~200-400 MB to ~100-200 MB, so 8 procs uses ~1.6 GB for Chrome vs the 1.6-3.2 GB that triggered kernel OOM kills. - Memory safety at 8 procs: 8 × ~200 MB Chrome + 512 MB MySQL + 8 × ~80 MB Go ≈ 2.7 GB, within the 12-core runner's available headroom (no explicit memory limit per ci.yml). - DB budget: 8 × 15 = 120 conns, well under max_connections=512. ## Verification Ran `BROWSER_PROCS=8 make e2e-browser` on macOS (18 GB RAM, Chrome) 5 times: - All 5 runs: 305 passed, 0 proc crashes, 0 OOM kills, 0 "timed out waiting for procs" - The single consistent macOS failure (a11y guardrails focus-in-modal) is macOS-specific and also fails at 4 procs — CI on Linux passes at both counts - Wall-clock: avg 3m48s at 8 procs vs 5m14s at 4 procs ## Test plan - [ ] CI E2E Browser job goes green (proving 8 procs stable on Linux CI) - [ ] E2E Browser wall-clock faster than the pre-change ~6m53s (from recent main CI run) Closes bead bookshelf-dc11r on merge.

zombor added 1 commit 2026-07-29 13:58:43 +00:00

ci(e2e): restore BROWSER_PROCS 4->8 now proc-crash root cause is fixed (bookshelf-dc11r) ... d12d377ca0

lrlp6/#1242 fixed two independent root causes for the exit-status-2 proc-crash
flake that originally forced BROWSER_PROCS 8->4:

1. go-workflows Worker.WaitForCompletion permanently closes the internal tasks
   channel. workerSet now rebuilds fresh Worker instances after drain (93f101eb),
   so StartWorker->DrainWorker->StartWorker no longer panics with "send on closed
   channel".

2. Chrome memory flags (--disable-gpu, --disable-background-networking, etc.)
   added in lrlp6 reduce per-instance footprint from ~200-400 MB to ~100-200 MB.
   8 procs now uses ~1.6 GB for Chrome vs the 1.6-3.2 GB that triggered OOM kills.

With both fixes on main, 8 procs is safe:
- Memory: 8 × ~200 MB Chrome + 512 MB MySQL + 8 × ~80 MB Go ≈ 2.7 GB (within
  runner headroom; 12-core runner, no explicit memory limit).
- DB budget: 8 × 15 = 120 conns, well under max_connections=512.
- Verified locally 5/5 clean runs at 8 procs (no OOM kills, no proc crashes);
  avg wall-clock 3m48s vs 5m14s at 4 procs (~27% faster).

Also updates the Leakless(false) comment to say "8 fewer processes total" and
clarifies the memory-reduction rationale in browser_suite_test.go.

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

zombor commented 2026-07-29 14:08:14 +00:00

Author

Owner

Copy link

Security Review — PR #1300 (bookshelf-dc11r)

Reviewed per .claude/rules/review-standard.md.

Scope of change: BROWSER_PROCS env var bumped 4 → 8 in .forgejo/workflows/ci.yml; comment block in e2e/browser/browser_suite_test.go updated to match.

Checks performed

Chrome flags — no change. The launcher flag set is identical between origin/main and this branch:
Headless(true), NoSandbox(true), disable-dev-shm-usage, disable-gpu, disable-background-networking, no-first-run, disable-default-apps, Leakless(false). NoSandbox(true) is the expected CI posture (unprivileged runner, no user-namespace support) and predates this PR; no new security-weakening flag was introduced.

CI job names — no rename. No name: lines changed; branch-protection required-status-check bindings are unaffected.

Secrets/env — none added. No new secrets.* references, tokens, keys, or credentials appear in the diff.

Network/registry surface — none added. No new uses:, image:, or external download URLs introduced.

New files — none. Change is confined to one env-var value and comment text.

Findings

No security findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1300 (bookshelf-dc11r) Reviewed per `.claude/rules/review-standard.md`. **Scope of change:** `BROWSER_PROCS` env var bumped `4` → `8` in `.forgejo/workflows/ci.yml`; comment block in `e2e/browser/browser_suite_test.go` updated to match. ### Checks performed **Chrome flags — no change.** The launcher flag set is identical between `origin/main` and this branch: `Headless(true)`, `NoSandbox(true)`, `disable-dev-shm-usage`, `disable-gpu`, `disable-background-networking`, `no-first-run`, `disable-default-apps`, `Leakless(false)`. `NoSandbox(true)` is the expected CI posture (unprivileged runner, no user-namespace support) and predates this PR; no new security-weakening flag was introduced. **CI job names — no rename.** No `name:` lines changed; branch-protection required-status-check bindings are unaffected. **Secrets/env — none added.** No new `secrets.*` references, tokens, keys, or credentials appear in the diff. **Network/registry surface — none added.** No new `uses:`, `image:`, or external download URLs introduced. **New files — none.** Change is confined to one env-var value and comment text. ### Findings No security findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-07-29 14:10:16 +00:00

Author

Owner

Copy link

Code Review — bookshelf-dc11r (PR #1300)

Phase 1: Spec Compliance

The bead asks to restore BROWSER_PROCS from 4 back to 8 now that both lrlp6 root causes are fixed. The diff does exactly that: BROWSER_PROCS: "4" to "8" in ci.yml plus a companion comment update in browser_suite_test.go. No extra or missing work.

---

Phase 2: Code Quality

OOM-safety justification (the crux)

Both lrlp6 root causes are verifiably fixed on main before this PR:

• Worker rebuild fix (93f101eb): internal/wfengine/worker_set.go confirms start() detects drained == true and calls buildQueueWorkers() before starting pollers. The fix is real and already merged as part of lrlp6 (commit 93f101eb, merged via PR #1242).
• Chrome memory flags: --disable-gpu, --disable-background-networking, --no-first-run, --disable-default-apps are present in the browser_suite_test.go launcher chain (added in 61f2899b / lrlp6). These eliminate the GPU subprocess (~50-100 MB each) and reduce per-instance footprint from the old 200-400 MB range to ~100-200 MB.

Memory budget reasoning:

The comment claims: 8 x ~200 MB Chrome + 512 MB MySQL + 8 x ~80 MB Go = ~2.7 GB total. The pre-fix upper bound was 3.2 GB for Chrome alone; the headroom gain from --disable-gpu is genuine. The math is internally consistent. The runner has 12 cores with no explicit cgroup memory limit (ci.yml line 254).

BROWSER_PROCS wiring:

BROWSER_PROCS: "8" is set as a step env var, flows to make e2e-browser, and the Makefile passes --procs=$(BROWSER_PROCS) directly to ginkgo. Nothing else overrides or caps it. Wiring is correct.

Job name: e2e-browser: / name: E2E Browser is unchanged. No branch-protection break.

DB budget: 8 x 15 = 120 conns, well under max_connections=512. No concern.

---

[MINOR] .forgejo/workflows/ci.yml:498 — "no explicit memory limit" overstates certainty
The comment justifies safety with "comfortably within the runner's available headroom (12-core runner, no explicit memory limit per ci.yml comment at line 254)." Absence of a cgroup memory limit is not the same as unlimited safe headroom: the Kubernetes node has a physical ceiling and other pods share it. The budget math is reasonable but the comment overstates the guarantee. Suggested fix: replace "no explicit memory limit" with "runner has historically sustained this load" or cite observed RSS from a CI run.

[MINOR] e2e/browser/browser_suite_test.go:128 — local verification is macOS 18 GB, not Linux CI
The comment documents "5/5 runs clean on macOS (18 GB RAM)" as the verification evidence. The OOM risk is specific to the CI Linux container environment, which has different memory pressure characteristics. The CI green run on this PR is the real datapoint; the comment could note it (e.g., "verified clean on CI: PR #1300, 5/5 runs") so future readers do not rely solely on a macOS result that does not exercise the constrained environment.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — bookshelf-dc11r (PR #1300) ### Phase 1: Spec Compliance The bead asks to restore `BROWSER_PROCS` from 4 back to 8 now that both lrlp6 root causes are fixed. The diff does exactly that: `BROWSER_PROCS: "4"` to `"8"` in ci.yml plus a companion comment update in `browser_suite_test.go`. No extra or missing work. --- ### Phase 2: Code Quality **OOM-safety justification (the crux)** Both lrlp6 root causes are verifiably fixed on `main` before this PR: - **Worker rebuild fix (93f101eb):** `internal/wfengine/worker_set.go` confirms `start()` detects `drained == true` and calls `buildQueueWorkers()` before starting pollers. The fix is real and already merged as part of lrlp6 (commit 93f101eb, merged via PR #1242). - **Chrome memory flags:** `--disable-gpu`, `--disable-background-networking`, `--no-first-run`, `--disable-default-apps` are present in the `browser_suite_test.go` launcher chain (added in 61f2899b / lrlp6). These eliminate the GPU subprocess (~50-100 MB each) and reduce per-instance footprint from the old 200-400 MB range to ~100-200 MB. **Memory budget reasoning:** The comment claims: 8 x ~200 MB Chrome + 512 MB MySQL + 8 x ~80 MB Go = ~2.7 GB total. The pre-fix upper bound was 3.2 GB for Chrome alone; the headroom gain from `--disable-gpu` is genuine. The math is internally consistent. The runner has 12 cores with no explicit cgroup memory limit (ci.yml line 254). **BROWSER_PROCS wiring:** `BROWSER_PROCS: "8"` is set as a step env var, flows to `make e2e-browser`, and the Makefile passes `--procs=$(BROWSER_PROCS)` directly to ginkgo. Nothing else overrides or caps it. Wiring is correct. **Job name:** `e2e-browser: / name: E2E Browser` is unchanged. No branch-protection break. **DB budget:** 8 x 15 = 120 conns, well under `max_connections=512`. No concern. --- [MINOR] .forgejo/workflows/ci.yml:498 — "no explicit memory limit" overstates certainty The comment justifies safety with "comfortably within the runner's available headroom (12-core runner, no explicit memory limit per ci.yml comment at line 254)." Absence of a cgroup memory limit is not the same as unlimited safe headroom: the Kubernetes node has a physical ceiling and other pods share it. The budget math is reasonable but the comment overstates the guarantee. Suggested fix: replace "no explicit memory limit" with "runner has historically sustained this load" or cite observed RSS from a CI run. [MINOR] e2e/browser/browser_suite_test.go:128 — local verification is macOS 18 GB, not Linux CI The comment documents "5/5 runs clean on macOS (18 GB RAM)" as the verification evidence. The OOM risk is specific to the CI Linux container environment, which has different memory pressure characteristics. The CI green run on this PR is the real datapoint; the comment could note it (e.g., "verified clean on CI: PR #1300, 5/5 runs") so future readers do not rely solely on a macOS result that does not exercise the constrained environment. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-dc11r from d12d377ca0 to 682d61fb51 2026-07-29 14:48:14 +00:00 Compare

zombor merged commit 8169a2d900 into main 2026-07-29 15:00:09 +00:00

zombor referenced this pull request from a commit 2026-07-29 15:00:13 +00:00

Merge pull request 'ci(e2e): restore BROWSER_PROCS 4->8 now proc-crash root cause is fixed (bookshelf-dc11r)' (#1300) from bd-bookshelf-dc11r into main

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!1300

No description provided.