History

Fix multi-connection protocol corruption in the dev-db PGlite socket

With the dev-db now accepting concurrent connections, three e2e specs kept failing individually (one per shard, through all retries): policies-ui, mcp-catalog-sync-ui, and oauth-callback-org-scope. Their failure artifacts all showed the same server-side signature: random 500s on /api/tools, /api/connections, /api/oauth/clients ("Failed to load tools" in the UI), backed by PostgresError: bind message supplies N parameters, but prepared statement "" requires M. Root cause: pglite-socket 0.1.4's QueryQueueManager enqueues each postgres wire FRAME (Parse, Bind, Execute, Sync) as its own queue entry against the single shared PGlite session. Two concurrent connections' extended-protocol pipelines interleave: A's Parse (5 params), B's Parse (1 param), then A's Bind hits B's unnamed statement. Whichever request loses the race 500s. A standalone repro (6 clients alternating 1-param and 5-param unprepared queries, the exact drizzle/postgres-js shape) corrupted 34.7% of queries. Patch (patches/@electric-sql%2Fpglite-socket@0.1.4.patch): - batch all frames parsed from one socket data event into ONE queue entry, so a client's Parse..Sync executes atomically under runExclusive; - add pipeline affinity: while a handler's batch ended mid-pipeline (last frame not Sync/Query/Terminate), only that handler's queue entries are scheduled, mirroring the existing transaction affinity; released on detach with a synthesized Sync so an abandoned pipeline can't wedge the queue. Same repro after the patch: 0% corrupted (106k queries clean). Regression test: apps/cloud/src/db/dev-db-socket-concurrency.node.test.ts (fails on stock 0.1.4, verified). Also fixes the remaining race in cloud/oauth-callback-org-scope.test.ts, which had a second, spec-local bug: it deep-linked to ?addAccount=1 while the session cookie still pointed at org B, so the URL bounced (org A -> org B -> org A via OrgSlugGate canonicalization) while the auto-opened modal fetched /api/oauth/clients under the transient org-B scope, got an empty app list, and left "Connect with OAuth" disabled forever. The spec now lands on the integration page without the deep link, waits for org A's shell to settle, opens the modal explicitly, and waits for the button to be enabled before clicking. The pre-armed waitForRequest is also marked handled so a failed click can no longer leak it as an unhandled rejection. All three specs pass together locally against the patched stack.

Rhys Sullivan committed 4 months ago

Commit 57513ff1 parent df86ec56 files · +151 −6Browse files

Showing the changes to apps/cloud/scripts (1 of 6 files). Show all

Unified · Split

apps/cloud/scripts/dev-db.ts+11−4
@@ -95,10 +95,17 @@ await migrate(drizzle(db), { migrationsFolder: MIGRATIONS_FOLDER }); · 94 unmodified lines
9595 // thousands of half-closed sockets, starved real queries, drove request
9696 // latency into the tens of seconds, and eventually hung the stack: the CI e2e
9797 // "cloud dev stack degrades after minutes of sustained load" cascade flake.
98−// PGlite still runs queries serially (its internal QueryQueueManager executes
99−// each under `runExclusive`), so allowing many connections is safe: they queue
100−// instead of being rejected. Raise the cap so concurrent requests wait their
101−// turn instead of storming.
98+// PGlite runs queries serially (its internal QueryQueueManager executes each
99+// under `runExclusive`), so allowing many connections means they queue instead
100+// of being rejected. One caveat makes that safe: stock pglite-socket 0.1.4
101+// enqueues each wire FRAME separately, so two connections' extended-protocol
102+// pipelines (Parse/Bind/Execute/Sync) would interleave inside the one shared
103+// PGlite session and corrupt each other ("bind message supplies N parameters,
104+// but prepared statement requires M" -> random 500s on whichever request lost
105+// the race). The patch in patches/@electric-sql%2Fpglite-socket@0.1.4.patch
106+// batches each socket data event into one queue entry and holds handler
107+// affinity while a pipeline is open;
108+// src/db/dev-db-socket-concurrency.node.test.ts is the regression test.
102109 const server = new PGLiteSocketServer({
103110 db,
104111 port: PORT,