Skip to content

test: close every database pool a test file opens - #849

Open
rahul05ranjan wants to merge 1 commit into
CopilotKit:mainfrom
rahul05ranjan:fix/test-pool-closing
Open

rahul05ranjan wants to merge 1 commit into
CopilotKit:mainfrom
rahul05ranjan:fix/test-pool-closing

Conversation

@rahul05ranjan

Copy link
Copy Markdown

Fixes #785.

What this changes

Three things, exactly as the issue proposes:

  1. The remaining unclosed files close their pool. Every const <name> = createDatabase( variable now gets await <name>.$client.close():
    • 3 files already had a top-level afterAll cleanup — the close is its last statement, the idiom identity-store.integration.test.ts uses;
    • 43 files had no top-level afterAll (only describe-scoped ones) — they gain a module-level afterAll at the end of the file. Registered last, it runs last: Bun runs afterAll hooks first-registered-first, so the close follows every other hook's cleanup. A describe-scoped afterAll would have been wrong here — closing at a describe's end takes the pool from later suites in the same file.
    • 14 files already close via $client.end(...) — left untouched; the pairing accepts either.
  2. A guard test keeps the pairing (database-pool-closing.test.ts): every createDatabase variable in server/tests/*.test.ts must have a $client.close() or $client.end( somewhere in the same file. server/tests is outside tsc, so this is the thing that notices file 118 reintroducing the leak.
  3. The two stale sentences in scripts/test-server-sharded.ts are replaced with what the issue measured: construction opens nothing, a pool costs its max from its first query and is never reaped, exhaustion is a function of files run rather than imported, and an unclosed pool is a defect in the source rather than a harness ceiling. The sharded runner itself is unchanged and still worth having for wall-clock.

Where it runs

Test infrastructure only; no server code is touched.

  • New state that outlives a request? None.
  • Second replica? No server change.
  • Anything serialised? No.
  • Anything fanned out? No.
  • New listener, port, schedule? No.

Boundary and audit

  • No acting call changed: tests and scripts only.
  • No new refusals.
  • Nothing new trusted from the client.

Changelog

  • No deployment behaves differently: the change is entirely inside server/tests and scripts/.

Proof

No Bun or PostgreSQL on this machine (arm64 Linux CI box), so the local verification is tooling around the text, and the suite itself runs in CI:

  • The guard's exact rule, run locally as a Python mirror over all 127 pool-opening files in server/tests: after the edit, zero offenders. Before it, the rule reported the 46 files this PR fixes (the issue counted 55 against an earlier tree; the same rule also finds the 14 $client.end files already satisfied, and correctly ignores scoped pools that are closed in-scope).
  • Every edited or new file parses: node --experimental-strip-types --check on all 47 touched .ts files under server/tests (the sharded runner script is not node-parseable even on main — its edit is comment-only).
  • No double-close: files using $client.end( got nothing added on top of it; a scan for any variable carrying both a close() and an end( finds none.
  • Placement audit: the diff contains exactly 46 inserted close statements — 3 inside an existing top-level afterAll (checked by hand: after the last delete, before the block's close), 43 as the body of a new trailing module-level afterAll. No close was placed inside a describe-scoped hook; the first attempt did that to one file and was reverted and redone with a top-level-only rule.
  • What I could not run: bun test itself (needs PostgreSQL); CI is the authoritative run, and the failure modes here are loud — a pool closed too early fails the deletes around it, a syntax error fails the file's import.

bun test runs server/tests in one process and a module-scope pool is
never reaped, so PostgreSQL's 100-connection ceiling is a function of
the files that have run, not imported, and the failure lands on
whoever adds the next file (CopilotKit#785). Every createDatabase variable now
gets a $client.close(): in the file's last top-level afterAll where
one exists, or a new module-level afterAll registered after every
other hook. A guard test keeps the pairing, and the sharded runner's
'import phase / never a reason to change source' sentences are
corrected to what was measured.

Fixes CopilotKit#785

Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server/tests leaks database connections: 55 files open a pool and never close it

1 participant