Skip to content

fix: prevent handle leak and add configurable statement_timeout for D…#308

Open
slowbackspace wants to merge 2 commits intomasterfrom
chore/statement-timeout
Open

fix: prevent handle leak and add configurable statement_timeout for D…#308
slowbackspace wants to merge 2 commits intomasterfrom
chore/statement-timeout

Conversation

@slowbackspace
Copy link
Contributor

…B Sync

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a streaming resource leak when clients disconnect mid-response and introduces a configurable PostgreSQL statement_timeout for DB Sync queries via config/env var.

Changes:

  • Destroy JSON streaming readable when the client connection closes to prevent stream handle leaks.
  • Add dbSync.statementTimeout / BLOCKFROST_CONFIG_DBSYNC_STATEMENT_TIMEOUT and wire it to Postgres statement_timeout.
  • Bump version to 6.3.1 and document the release in the changelog.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/utils/string-utils.ts Adds client-disconnect cleanup for toJSONStream to prevent handle leaks.
src/config.ts Loads optional DB Sync statement timeout from env/config.
src/app.ts Passes statement timeout into @fastify/postgres connection options.
package.json Version bump to 6.3.1.
CHANGELOG.md Documents 6.3.1 changes (timeout + leak fix).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@slowbackspace slowbackspace force-pushed the chore/statement-timeout branch 2 times, most recently from 58a9927 to f4bec7a Compare March 4, 2026 18:45
@slowbackspace slowbackspace requested a review from Copilot March 4, 2026 18:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +70 to +76
const cleanup = () => {
dataStream.unpipe(transformStream);
dataStream.destroy();
if (typeof (transformStream as stream.Transform).destroy === 'function') {
(transformStream as stream.Transform).destroy();
}
};
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup can be invoked more than once because it is registered separately with once('close') and once('error'). If both events fire (e.g., write error followed by close), this can double-destroy/unpipe streams and lead to noisy errors. Consider making cleanup idempotent with a cleaned flag (or wrap it in a one-time guard) before calling unpipe()/destroy().

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +80
const cleanup = () => {
dataStream.unpipe(transformStream);
dataStream.destroy();
if (typeof (transformStream as stream.Transform).destroy === 'function') {
(transformStream as stream.Transform).destroy();
}
};

serverResponse.once('close', cleanup);
serverResponse.once('error', cleanup);

Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toJSONStream attaches listeners only for close/error on the provided Writable. Many Writable implementations (including custom ones) may never emit close on normal completion, which can leave the cleanup listener (and captured dataStream/transformStream) referenced longer than necessary. Consider also cleaning up on finish (and/or removing the listeners when the piping completes) so cleanup reliably runs and listeners don’t linger.

Suggested change
const cleanup = () => {
dataStream.unpipe(transformStream);
dataStream.destroy();
if (typeof (transformStream as stream.Transform).destroy === 'function') {
(transformStream as stream.Transform).destroy();
}
};
serverResponse.once('close', cleanup);
serverResponse.once('error', cleanup);
function cleanup() {
// Remove listeners so the closure and streams can be garbage-collected.
serverResponse.removeListener('close', onClose);
serverResponse.removeListener('error', onError);
serverResponse.removeListener('finish', onFinish);
dataStream.unpipe(transformStream);
dataStream.destroy();
if (typeof (transformStream as stream.Transform).destroy === 'function') {
(transformStream as stream.Transform).destroy();
}
}
function onClose() {
cleanup();
}
function onError() {
cleanup();
}
function onFinish() {
cleanup();
}
serverResponse.once('close', onClose);
serverResponse.once('error', onError);
serverResponse.once('finish', onFinish);

Copilot uses AI. Check for mistakes.
@slowbackspace slowbackspace force-pushed the chore/statement-timeout branch from f4bec7a to df2ea83 Compare March 4, 2026 19:28
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.

2 participants