Make wrapClient idempotent to prevent stack overflow on pooled clients - #190
Merged
Conversation
The query-stack-trace wrapper re-wrapped client.query on every checkout. Because pg pools reuse idle clients, wrapClient ran repeatedly on the same physical client, nesting the query wrapper one level deeper each time: wN -> wN-1 -> ... -> w1 -> realQuery. Over hours of traffic (the every-minute notification scheduler plus all HTTP requests) the nesting grew until invoking client.query blew the call stack with "RangeError: Maximum call stack size exceeded". The scheduler surfaced in the trace only because it is the most frequent query path; every query shared the same fault. Make wrapClient idempotent by marking a client once its query is wrapped and skipping re-wrap on subsequent checkouts. Add regression tests covering wrap-once-across-checkouts and stack-trace preservation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKxNmLHzc7ygiEWQYHXwgP
There was a problem hiding this comment.
Pull request overview
This pull request addresses a critical backend reliability issue where pooled pg clients could be re-wrapped on every checkout, eventually causing a stack overflow when calling client.query() in long-running processes.
Changes:
- Makes
wrapClient()idempotent by marking wrapped clients with aSymboland returning early on subsequent calls. - Exports
wrapClient()and adds tests intended to cover idempotency and rejection/error behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/backend/src/utils/db.ts | Adds a Symbol marker and early-return logic to ensure pooled clients are wrapped only once; exports wrapClient() for testability. |
| apps/backend/tests/db.test.ts | Adds tests for idempotent wrapping and query rejection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+136
to
+144
| it('preserves the application stack trace on query rejection', async () => { | ||
| const dbError = new Error('boom'); | ||
| const realQuery = vi.fn().mockRejectedValue(dbError); | ||
| const client = { query: realQuery } as unknown as pg.PoolClient; | ||
|
|
||
| wrapClient(client); | ||
|
|
||
| await expect(client.query('SELECT 1')).rejects.toThrow('boom'); | ||
| }); |
The rejection test only checked the error message, so it would have passed even if enhanceErrorWithStack regressed. Capture the thrown error and assert the "--- Query initiated from ---" marker plus real call-site frames are appended, so the test actually guards stack-trace enhancement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MKxNmLHzc7ygiEWQYHXwgP
| // client once it's wrapped prevents re-wrapping its `query` on every checkout, | ||
| // which would otherwise nest the wrapper deeper each time until invoking | ||
| // `client.query` overflows the call stack ("Maximum call stack size exceeded"). | ||
| const WRAPPED = Symbol('freundebuch.queryWrapped'); |
Comment on lines
6
to
10
| closePool, | ||
| createPool, | ||
| setupGracefulShutdown, | ||
| wrapClient, | ||
| } from '../src/utils/db.js'; |
Symbol() is module-instance local, so if the db module is ever loaded under
two specifiers (db.js and db.ts) each instance gets a distinct marker and the
same pooled client could be wrapped twice, reintroducing the stack-overflow
risk. Symbol.for shares the marker across instances while still avoiding
string-key collisions.
Also drop the dead no-op vi.mock('../src/utils/db.ts') from the test, which
mixed module specifiers (the file imports from db.js) for no effect.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MKxNmLHzc7ygiEWQYHXwgP
|
🎉 This PR is included in version 2.89.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed a critical bug where reusing pooled database clients would cause stack overflow errors by preventing the query wrapper from being applied multiple times to the same client.
Key Changes
wrapClient()idempotent by tracking wrapped clients with a Symbol marker (WRAPPED)wrapClient()now returns it unchanged instead of wrapping the query method againwrapClient()for testing purposesImplementation Details
WRAPPED) to mark clients that have already been wrapped, avoiding property name collisionshttps://claude.ai/code/session_01MKxNmLHzc7ygiEWQYHXwgP