Skip to content

fix: await pool teardown in init().end() - #1151

Open
h-zahar wants to merge 1 commit into
supabase:masterfrom
h-zahar:fix/await-pool-teardown-in-db-end
Open

h-zahar wants to merge 1 commit into
supabase:masterfrom
h-zahar:fix/await-pool-teardown-in-db-end

Conversation

@h-zahar

@h-zahar h-zahar commented Sep 9, 2026

Copy link
Copy Markdown

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

Fixes #1150

init().end() is declared end: () => Promise<void> (src/lib/db.ts:73) and is awaited at 65 call sites — 64 in src/server/routes/*, 1 in src/lib/generators.ts. It resolves before the pool is closed, because the Sentry.startSpan(...) call at src/lib/db.ts:250 is not returned and the promise wrapping await _pool.end() is discarded.

The other three Sentry.startSpan call sites in the file — :31, :75, :117 — all return their result. The behaviour changed in bf2fad4 (14 Apr 2025), which moved the body of end() inside a span callback; before that commit the method awaited _pool.end() directly.

Where it is observable:

  • The CRUD routes (the 64 sites above) are sequential — every pgMeta.* call in src/server/routes/ is awaited, and await pgMeta.end() follows it — so nothing is in flight when end() returns early.
  • GET /generators/{typescript,go,swift,python} reach end() through getGeneratorMetadata's finally (src/lib/generators.ts), which wraps introspect() from @supabase/postgrest-typegen. introspect() issues its introspection work as ten concurrent operations on the same PostgresMeta instance under an un-caught Promise.all. Promise.all rejects on the first rejection, so if one of them fails while the others are still outstanding, the finally calls end() on a pool that is not idle — and end() returns before that outstanding work is done.

What is the new behavior?

end() resolves after the pool has closed.

       async end() {
-        Sentry.startSpan({ op: 'db', name: 'init.end' }, async () => {
+        return Sentry.startSpan({ op: 'db', name: 'init.end' }, async () => {

test/lib/db.ts adds two tests, registered from test/index.test.ts:

  • end() resolves only after in-flight queries have finished — starts select pg_sleep(1) without awaiting it, then races that promise against db.end(). If end() waits for the pool to drain, the query must settle first. It then confirms via pg_stat_activity that the connection is gone. No sleeps and no wall-clock assertions.
  • query() after end() runs on a fresh poolend() sets pool = null, so a later query() takes the if (!pool) branch at src/lib/db.ts:138 and builds a temporary pool. This passes before and after the change; it is coverage for the branch nearest the fix, not a second regression test.
    Before the one-line change:
❯ test/index.test.ts (149 tests | 1 failed | 148 skipped) 14ms
  × end() resolves only after in-flight queries have finished 13ms
    → expected 'end' to be 'query' // Object.is equality
 
AssertionError: expected 'end' to be 'query' // Object.is equality
Expected: "query"
Received: "end"
 ❯ test/lib/db.ts:17:52

After:

✓ test/index.test.ts (149 tests | 148 skipped) 1138ms
  ✓ end() resolves only after in-flight queries have finished 1136ms
Tests  1 passed | 148 skipped (149)

Additional context

Full suite, macOS on arm64, Node v22.23.2, the test/db Docker fixture. One run each.

Unmodified master:

× query with ssl w/o root cert
× test query timeout > pool timeout after 7s with statementTimeoutSecs and connection cleanup
Test Files  1 failed | 12 passed (13)
     Tests  2 failed | 195 passed (197)

With this change:

× query with ssl w/o root cert
× test js parser error max result > should not kill the server on underlying parser error
Test Files  1 failed | 12 passed (13)
     Tests  2 failed | 197 passed (199)

197 → 199 is the two tests added here; both pass. query with ssl w/o root cert fails identically in both runs — it asserts an anchored regex on Node's TLS error string, and Node on macOS appends a --use-system-ca hint that Linux builds do not. The other two failures swapped: test query timeout ... failed on unmodified master and passed with this change, while test js parser error max result ... did the reverse. Both of those assert against the 5-second PG_QUERY_TIMEOUT_SECS the suite sets, and both are throughput-sensitive on a laptop.

Also run:

  • npm run check — clean. Note it only covers src (tsconfig.json has "include": ["src"]), so it does not type-check test/lib/db.ts; that is true of every file in test/lib/.
  • npx prettier --check src/lib/db.ts test/lib/db.ts test/index.test.ts — clean.

@h-zahar
h-zahar requested review from a team, avallete and soedirgo as code owners September 9, 2026 20:37
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.

db.ts: init().end() resolves before the connection pool is closed

1 participant