fix(test): cap integration/e2e MySQL test pools to avoid max_connections exhaustion (bookshelf-or2yq) #1328

Merged

zombor merged 1 commit from bd-bookshelf-or2yq into main 2026-08-04 19:22:43 +00:00

Conversation 2 Commits 1 Files changed 4 +54 -2

zombor commented 2026-08-04 12:18:30 +00:00

Owner

Copy link

Summary

• Root cause: dbtest.NewSuiteDB opened each isolated test DB with MaxOpenConns:10; bookdrop_integration_test.go and scan_integration_test.go call StartSharedMySQL+NewSuiteDB per-BeforeEach (not once per suite), so under make integration -p 8 the 8 parallel packages × uncapped pools could exhaust MySQL's max_connections, blocking connections with a 60s context.DeadlineExceeded (bookshelf-rgjd / bookshelf-or2yq).
• internal/dbtest/dbtest.go: Reduce NewSuiteDB pool from MaxOpenConns:10, MaxIdleConns:5 to 5/2 (exported as SuiteDBMaxOpenConns/SuiteDBMaxIdleConns constants) to match the discipline already used by openRoot and the wfengine suites.
• internal/wfengine/engine_integration_test.go: Cap the previously-uncapped rawDB (used only for seed queries in one BeforeEach) to 5/2.
• e2e/testutil/server.go provisionWFDatabase: Cap the two short-lived sql.Open connections (one-shot DDL CREATE/DROP) to 1/1 — they need at most 1 connection each.
• internal/dbtest/dbtest_test.go: Add a pool-budget guard test (mirrors e2e/testutil/server_test.go) so future cap bumps fail the integration suite if they blow the budget.

Test plan

• make build — compiles clean
• make test — all unit tests pass
• go build -tags integration ./internal/db/... ./internal/dbtest/... ./internal/wfengine/... — compiles clean
• CI integration job passes without connection-exhaustion timeouts

Closes bead bookshelf-or2yq on merge.

## Summary - **Root cause:** `dbtest.NewSuiteDB` opened each isolated test DB with `MaxOpenConns:10`; `bookdrop_integration_test.go` and `scan_integration_test.go` call `StartSharedMySQL+NewSuiteDB` per-BeforeEach (not once per suite), so under `make integration -p 8` the 8 parallel packages × uncapped pools could exhaust MySQL's `max_connections`, blocking connections with a 60s `context.DeadlineExceeded` (bookshelf-rgjd / bookshelf-or2yq). - **`internal/dbtest/dbtest.go`:** Reduce `NewSuiteDB` pool from `MaxOpenConns:10, MaxIdleConns:5` to 5/2 (exported as `SuiteDBMaxOpenConns`/`SuiteDBMaxIdleConns` constants) to match the discipline already used by `openRoot` and the wfengine suites. - **`internal/wfengine/engine_integration_test.go`:** Cap the previously-uncapped `rawDB` (used only for seed queries in one BeforeEach) to 5/2. - **`e2e/testutil/server.go` `provisionWFDatabase`:** Cap the two short-lived `sql.Open` connections (one-shot DDL CREATE/DROP) to 1/1 — they need at most 1 connection each. - **`internal/dbtest/dbtest_test.go`:** Add a pool-budget guard test (mirrors `e2e/testutil/server_test.go`) so future cap bumps fail the integration suite if they blow the budget. ## Test plan - [ ] `make build` — compiles clean - [ ] `make test` — all unit tests pass - [ ] `go build -tags integration ./internal/db/... ./internal/dbtest/... ./internal/wfengine/...` — compiles clean - [ ] CI integration job passes without connection-exhaustion timeouts Closes bead bookshelf-or2yq on merge.

zombor added 1 commit 2026-08-04 12:18:31 +00:00

fix(test): cap integration/e2e MySQL test pools to avoid max_connections exhaustion (bookshelf-or2yq) ... 93ea6f0d93

- dbtest.NewSuiteDB: reduce pool from MaxOpenConns:10/MaxIdleConns:5 to
  5/2 (exported as SuiteDBMaxOpenConns/SuiteDBMaxIdleConns constants) so
  parallel integration packages don't exhaust the server's max_connections
- wfengine engine_integration_test: cap the ad-hoc rawDB (previously
  uncapped) to 5/2, matching the discipline used everywhere else
- e2e/testutil/server.go provisionWFDatabase: cap the short-lived rootDB
  DDL connection to 1/1 (single-use CREATE/DROP DATABASE calls)
- internal/dbtest/dbtest_test.go: add a pool-budget guard test
  (mirrors e2e/testutil/server_test.go) to catch future cap regressions

Root cause: bookdrop_integration_test.go and scan_integration_test.go call
StartSharedMySQL+NewSuiteDB per BeforeEach; with the old MaxOpenConns:10
per suiteDB, the shared MySQL could exhaust under parallel -p 8 runs
(bookshelf-rgjd / bookshelf-or2yq).

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

zombor referenced this pull request 2026-08-04 12:39:16 +00:00

fix(e2e): widen move_toast BeforeAll Eventually timeouts (residual flake) (bookshelf-49zk3) #1330

zombor commented 2026-08-04 18:30:25 +00:00

Author

Owner

Copy link

Security Review — PR #1328 (bookshelf-or2yq)

Scope verified: all 9 changed files are test-only (*_test.go, e2e/testutil/server.go, internal/dbtest/dbtest.go). No production internal/db connection handling, DSN construction, credential handling, or auth surfaces changed. Zero diff against internal/app/, internal/config/, internal/httpserver/, or internal/users/.

---

Findings

No security findings.

Detailed reasoning:

Test-harness scope: The diff touches exclusively test infrastructure — internal/dbtest/dbtest.go (test helper package), internal/dbtest/dbtest_test.go (guard test), e2e/testutil/server.go (e2e suite helper), internal/wfengine/engine_integration_test.go, internal/wfengine/export_test.go, internal/wfengine/retention_sweep_test.go, internal/governor/inmemory_test.go, internal/library/scan/walk_test.go, and e2e/browser/journey_move_toast_test.go. None of these files execute in the production binary.

No hard-coded secrets or DSNs: The new exported constants SuiteDBMaxOpenConns = 5 and SuiteDBMaxIdleConns = 2 are plain integers with no credential or connection-string content. DSN handling in e2e/testutil/server.go and internal/dbtest is unchanged in structure — the existing PERGAMUM_DSN env-var path still strips the database name via stripDBName before use; no DSN is newly logged or echoed.

No cross-user / multi-user regression: The changes are all in test harness plumbing (pool caps, timeout tightening, rendezvous simplification). No handler, service, or query is touched. User-scoping in production paths is unaffected.

No injection surface added: The provisionWFDatabase function that appends backtick-quoted DB names to raw SQL strings is pre-existing (not introduced by this PR) and that surface is unchanged. The two new SetMaxOpenConns(1) / SetMaxIdleConns(1) lines sit immediately after the existing sql.Open("mysql", rootDSN) call in the same short-lived DDL helper — no new SQL construction.

Pool caps reduce, not expand, resource exposure: All changes reduce or cap connection counts (from 10→5 for suite pools, from uncapped to 1 for single-use DDL connections in provisionWFDatabase, from 5→5 explicit for rawDB in the wfengine integration test). Lower caps narrow the attack surface against a shared MySQL instance rather than widening it.

Guard test is black-box and correct: internal/dbtest/dbtest_test.go declares package dbtest_test and references only the two newly exported constants — no unexported symbols accessed. The arithmetic guard (parallelPkgs × (rootDBMaxOpenConns + SuiteDBMaxOpenConns) < maxConnections − safeHeadroom) is a deterministic compile-time-ish check that will fail the test suite if a future PR bumps either cap beyond the safe ceiling.

Flake-regression assessment: The walk_test.go and retention_sweep_test.go changes replace channel-based rendezvous with runtime.Gosched() + time.Sleep(1ms) + cancel() and Eventually(..., 2s, 1ms). The time.Sleep in walk_test.go is in a test that pre-cancels a context before launching the walk goroutine — the sleep is not asserting on wall-clock elapsed time, it is yielding to let the goroutine fill the error channel before the context cancel is observed. This is a minor smell (a bounded Eventually loop would be more deterministic) but is not a security concern and the test's purpose — exercising the ctx.Done() drop path — is preserved. No flake-standard wall-clock assertion in the review-standard.md sense (asserting time.Since in the assertion itself) is introduced.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1328 (bookshelf-or2yq) **Scope verified:** all 9 changed files are test-only (`*_test.go`, `e2e/testutil/server.go`, `internal/dbtest/dbtest.go`). No production `internal/db` connection handling, DSN construction, credential handling, or auth surfaces changed. Zero diff against `internal/app/`, `internal/config/`, `internal/httpserver/`, or `internal/users/`. --- ### Findings **No security findings.** Detailed reasoning: **Test-harness scope:** The diff touches exclusively test infrastructure — `internal/dbtest/dbtest.go` (test helper package), `internal/dbtest/dbtest_test.go` (guard test), `e2e/testutil/server.go` (e2e suite helper), `internal/wfengine/engine_integration_test.go`, `internal/wfengine/export_test.go`, `internal/wfengine/retention_sweep_test.go`, `internal/governor/inmemory_test.go`, `internal/library/scan/walk_test.go`, and `e2e/browser/journey_move_toast_test.go`. None of these files execute in the production binary. **No hard-coded secrets or DSNs:** The new exported constants `SuiteDBMaxOpenConns = 5` and `SuiteDBMaxIdleConns = 2` are plain integers with no credential or connection-string content. DSN handling in `e2e/testutil/server.go` and `internal/dbtest` is unchanged in structure — the existing `PERGAMUM_DSN` env-var path still strips the database name via `stripDBName` before use; no DSN is newly logged or echoed. **No cross-user / multi-user regression:** The changes are all in test harness plumbing (pool caps, timeout tightening, rendezvous simplification). No handler, service, or query is touched. User-scoping in production paths is unaffected. **No injection surface added:** The `provisionWFDatabase` function that appends backtick-quoted DB names to raw SQL strings is pre-existing (not introduced by this PR) and that surface is unchanged. The two new `SetMaxOpenConns(1)` / `SetMaxIdleConns(1)` lines sit immediately after the existing `sql.Open("mysql", rootDSN)` call in the same short-lived DDL helper — no new SQL construction. **Pool caps reduce, not expand, resource exposure:** All changes reduce or cap connection counts (from 10→5 for suite pools, from uncapped to 1 for single-use DDL connections in `provisionWFDatabase`, from 5→5 explicit for `rawDB` in the wfengine integration test). Lower caps narrow the attack surface against a shared MySQL instance rather than widening it. **Guard test is black-box and correct:** `internal/dbtest/dbtest_test.go` declares `package dbtest_test` and references only the two newly exported constants — no unexported symbols accessed. The arithmetic guard (`parallelPkgs × (rootDBMaxOpenConns + SuiteDBMaxOpenConns) < maxConnections − safeHeadroom`) is a deterministic compile-time-ish check that will fail the test suite if a future PR bumps either cap beyond the safe ceiling. **Flake-regression assessment:** The `walk_test.go` and `retention_sweep_test.go` changes replace channel-based rendezvous with `runtime.Gosched() + time.Sleep(1ms) + cancel()` and `Eventually(..., 2s, 1ms)`. The `time.Sleep` in `walk_test.go` is in a test that pre-cancels a context before launching the walk goroutine — the `sleep` is not asserting on wall-clock elapsed time, it is yielding to let the goroutine fill the error channel before the context cancel is observed. This is a minor smell (a bounded `Eventually` loop would be more deterministic) but is not a security concern and the test's purpose — exercising the `ctx.Done()` drop path — is preserved. No flake-standard wall-clock assertion in the `review-standard.md` sense (asserting `time.Since` in the assertion itself) is introduced. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-04 18:31:00 +00:00

Author

Owner

Copy link

Code Review — bookshelf-or2yq / PR #1328

Phase 1: Spec Compliance

The bead goal is to cap integration/e2e test-harness MySQL connection pools to stop max_connections exhaustion under parallel procs. All four touch-points specified are addressed: internal/dbtest/dbtest.go (suite pool 10/5→5/2 via exported constants), internal/dbtest/dbtest_test.go (budget-guard test), internal/wfengine/engine_integration_test.go (rawDB 5/2), e2e/testutil/server.go (DDL pools 1/1). Production internal/db/db.go is confirmed untouched — pool sizing remains config-driven.

Phase 2: Findings

---

[MINOR] internal/dbtest/dbtest_test.go:78 — parallelPkgs = 8 comment says "4 packages" but lists the wrong count, and the constant conflates -p N with package count

The comment reads: "Current packages: internal/db, internal/dbtest, internal/wfengine, cmd/pergamum." But cmd/pergamum has no //go:build integration test files (worker_test.go and healthcheck_test.go have no integration tag), so there are only 3 packages that open DB pools. The constant 8 equals the -p 8 flag (process-parallelism ceiling), not the number of packages. In the worst case go test -p 8 can run at most 3 pool-owning binaries concurrently, not 8. The math is safe (3 × 10 = 30 << 950) but the constant and its comment are misleading — a future maintainer adding a new package may trust the comment and miscalculate. Suggested fix: rename to maxParallelProcs = 8 (matching -p 8) and update the comment to list the 3 real packages: internal/db, internal/dbtest, internal/wfengine.

---

[MINOR] internal/dbtest/dbtest_test.go:86 — rootDBMaxOpenConns is a hardcoded shadow; drift will silently produce a false-passing budget check

The constant rootDBMaxOpenConns = 5 mirrors the literal in dbtest.openRoot (line 478 of dbtest.go), but openRoot is unexported so the test cannot reference it directly. If openRoot's pool is ever changed, this shadow constant will silently desync and the budget calculation will be wrong while still passing. The fix is to export a RootDBMaxOpenConns constant from internal/dbtest (alongside the already-exported SuiteDB* constants) and reference it in the test, the same pattern used for the suite constants.

---

No Coverage / golangci Changes

Confirmed: scripts/check-coverage.sh and .golangci.yml are unchanged. No new exclusions. Both test files declare package dbtest_test (black-box). The rawDB cap in engine_integration_test.go is inside an existing black-box test (package wfengine_test) — convention is preserved.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — bookshelf-or2yq / PR #1328 ### Phase 1: Spec Compliance The bead goal is to cap integration/e2e test-harness MySQL connection pools to stop `max_connections` exhaustion under parallel procs. All four touch-points specified are addressed: `internal/dbtest/dbtest.go` (suite pool 10/5→5/2 via exported constants), `internal/dbtest/dbtest_test.go` (budget-guard test), `internal/wfengine/engine_integration_test.go` (rawDB 5/2), `e2e/testutil/server.go` (DDL pools 1/1). Production `internal/db/db.go` is confirmed untouched — pool sizing remains config-driven. ### Phase 2: Findings --- [MINOR] internal/dbtest/dbtest_test.go:78 — `parallelPkgs = 8` comment says "4 packages" but lists the wrong count, and the constant conflates `-p N` with package count The comment reads: "Current packages: internal/db, internal/dbtest, internal/wfengine, cmd/pergamum." But `cmd/pergamum` has no `//go:build integration` test files (`worker_test.go` and `healthcheck_test.go` have no integration tag), so there are only 3 packages that open DB pools. The constant `8` equals the `-p 8` flag (process-parallelism ceiling), not the number of packages. In the worst case `go test -p 8` can run at most 3 pool-owning binaries concurrently, not 8. The math is safe (3 × 10 = 30 << 950) but the constant and its comment are misleading — a future maintainer adding a new package may trust the comment and miscalculate. Suggested fix: rename to `maxParallelProcs = 8` (matching `-p 8`) and update the comment to list the 3 real packages: `internal/db`, `internal/dbtest`, `internal/wfengine`. --- [MINOR] internal/dbtest/dbtest_test.go:86 — `rootDBMaxOpenConns` is a hardcoded shadow; drift will silently produce a false-passing budget check The constant `rootDBMaxOpenConns = 5` mirrors the literal in `dbtest.openRoot` (line 478 of `dbtest.go`), but `openRoot` is unexported so the test cannot reference it directly. If `openRoot`'s pool is ever changed, this shadow constant will silently desync and the budget calculation will be wrong while still passing. The fix is to export a `RootDBMaxOpenConns` constant from `internal/dbtest` (alongside the already-exported `SuiteDB*` constants) and reference it in the test, the same pattern used for the suite constants. --- ### No Coverage / golangci Changes Confirmed: `scripts/check-coverage.sh` and `.golangci.yml` are unchanged. No new exclusions. Both test files declare `package dbtest_test` (black-box). The rawDB cap in `engine_integration_test.go` is inside an existing black-box test (`package wfengine_test`) — convention is preserved. --- REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-or2yq from 93ea6f0d93 to 421b84834e 2026-08-04 19:11:20 +00:00 Compare

zombor merged commit 8fe9e5535d into main 2026-08-04 19:22:43 +00:00

zombor referenced this pull request from a commit 2026-08-04 19:22:45 +00:00

Merge pull request 'fix(test): cap integration/e2e MySQL test pools to avoid max_connections exhaustion (bookshelf-or2yq)' (#1328) from bd-bookshelf-or2yq 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!1328

No description provided.