Skip to content

nickfujita: Fix: Message buffering, connection handling, and concurrent connection support #873

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tdrz wants to merge 7 commits into
base: main
Choose a base branch
from
Open

nickfujita: Fix: Message buffering, connection handling, and concurrent connection support #873

tdrz wants to merge 7 commits into from
+497 −481

Conversation

@tdrz
Copy link
Collaborator

@tdrz tdrz commented Jan 12, 2026

See #775

@tdrz tdrz mentioned this pull request Jan 12, 2026
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2026 •
edited
Loading

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2026

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2026 •
edited
Loading

🚀 Deployed on https://696780452ed758c935c43571--pglite.netlify.app

@tdrz tdrz requested a review from samwillis January 12, 2026 10:02
@samwillis
Copy link
Collaborator

samwillis commented Jan 13, 2026

I've not had a chance yet to look in detail (I'm tied up this morning in a mob session) - but I did ask GPT5.2 for a review of the branch. It looks like it's spotted a few things.

PR 873 Review — @electric-sql/pglite-socket

Summary

This PR targets three production issues:

  • Large queries causing crashes: PostgreSQL wire-protocol messages split across TCP packets were not being reassembled.
  • ECONNRESET crashes: abrupt disconnects could trigger unhandled rejections and crash the server process.
  • Single connection limitation: connection-level locking prevented concurrent clients from connecting.

The proposed architecture (per-connection handlers + shared query-level serialization) is the right direction for PGlite’s single-threaded execution model.

What looks good

  • Query-level serialization: a shared queue that serializes access to db.execProtocolRaw(...) aligns with PGlite’s constraints while allowing multiple sockets to stay connected.
  • Connection lifecycle separation: isolating per-socket state (buffers, idle timeout, cleanup) into a handler class is a clean boundary.
  • Operational intent is sound: treating ECONNRESET as normal behavior for pooled clients is correct.
  • Config additions: maxConnections and idleTimeout are reasonable operational controls.

Key risks / correctness concerns to double-check

1) TCP fragmentation / buffering must be concurrency-safe

Because Node can emit multiple 'data' events rapidly, ensure the handler’s buffering/drain logic cannot run concurrently in a way that races on shared state (e.g. messageBuffer). A race here can reintroduce message boundary corruption for large queries.

Recommendation: ensure buffer append and buffer-drain are serialized per connection (one drain loop at a time).

2) Queue draining edge cases

The queue manager should guarantee forward progress even if enqueues arrive around the time the processor finishes draining.

Recommendation: verify there is no timing window where processing flips false while there are still items queued (i.e. no “stuck until next enqueue” behavior).

3) Protocol parsing coverage

The parsing logic should handle:

  • StartupMessage (no type byte; [len:int32][protocol:int32][params...])
  • SSLRequest / CancelRequest / GSSENCRequest variants (also no type byte, but different request codes)
  • Regular frontend messages ([type:byte][len:int32][payload...])

Recommendations:

  • Treat lengths as unsigned and validate bounds (e.g. reject absurd lengths to avoid memory blow-ups).
  • Confirm that SSLRequest/CancelRequest packets are not misclassified as regular messages.

4) maxConnections rejection behavior

If rejecting connections when maxConnections is reached, ensure the server doesn’t write arbitrary plaintext to a socket that expects Postgres protocol frames.

Recommendation: either close the socket cleanly, or send a proper Postgres ErrorResponse (more work, better UX).

5) Public API / types

If the handler/server options types changed, ensure:

  • Existing construction patterns remain valid (backwards compatibility expectations).
  • Publicly exported types don’t reference internal/private-only classes (to avoid .d.ts issues).

Testing suggestions (beyond current unit coverage)

  • Fragmentation regression test: send a single large query (>64KB) that is forced to split across multiple TCP packets and confirm the server processes it correctly.
  • Abrupt disconnect test: disconnect mid-query and confirm no unhandled rejections; server continues serving new connections.
  • Concurrency test: open N connections (e.g. 20–50), run interleaved queries, confirm serialization and no deadlocks/starvation.
  • Idle timeout test: with idleTimeout set, verify only idle sockets are closed; active sockets are unaffected.

Files of interest

  • packages/pglite-socket/src/index.ts (handler, queue, server implementation)
  • packages/pglite-socket/src/scripts/server.ts (CLI runner / operational entrypoint)
  • packages/pglite-socket/tests/*.test.ts (coverage for multiplexing/disconnect/fragmentation)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 14, 2026

@github-actions
Copy link
Contributor

github-actions bot commented Jan 14, 2026

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

Reviewers

@samwillis samwillis Awaiting requested review from samwillis

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tdrz @samwillis @nickfujita