Merge remote-tracking branch 'origin/main' into feat/execution-limit-gate

Author: RhysSullivan

.github/workflows/ci.yml

@@ -7,8 +7,8 @@ on:
       - main
 
 concurrency:
-  group: ci-${{ github.ref }}
-  cancel-in-progress: true
+  group: ci-${{ github.event_name == 'push' && format('{0}-{1}', github.ref, github.sha) || github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 jobs:
   format:
@@ -21,6 +21,14 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       - run: bun install --frozen-lockfile
 
       - run: bun run format:check
@@ -35,6 +43,14 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       - run: bun install --frozen-lockfile
 
       - run: bun run lint
@@ -49,20 +65,38 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       - run: bun install --frozen-lockfile
 
       - run: bun run typecheck
 
   test:
     name: Test
     runs-on: ubuntu-latest
+    env:
+      TURBO_TEST_CONCURRENCY: 3
     steps:
       - uses: actions/checkout@v4
 
       - uses: oven-sh/setup-bun@v2
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       # apps/cloud's test script invokes `node` directly; undici 8.x (pulled
       # in by @cloudflare/vitest-pool-workers) calls webidl.markAsUncloneable
       # which only exists in Node 22.10+. Pin a known-good runtime.
@@ -105,6 +139,14 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       # The dev stacks spawn Node sidecars (vite/workerd tooling); pin the
       # same known-good runtime the unit-test job uses.
       - uses: actions/setup-node@v4
@@ -113,6 +155,12 @@ jobs:
 
       - run: bun install --frozen-lockfile
 
+      - name: Cache Playwright browsers
+        uses: actions/cache@v4
+        with:
+          path: ~/.cache/ms-playwright
+          key: ${{ runner.os }}-playwright-1.60.0
+
       # Install from e2e so bunx resolves ITS pinned playwright (the version
       # the tests run against) rather than floating to the latest.
       - name: Install Playwright Chromium
@@ -152,6 +200,14 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       # The local scenarios boot a real `executor web` (which spawns a Node
       # sidecar) and some drive a browser, so pin Node 22 and install Chromium.
       - uses: actions/setup-node@v4
@@ -160,6 +216,12 @@ jobs:
 
       - run: bun install --frozen-lockfile
 
+      - name: Cache Playwright browsers
+        uses: actions/cache@v4
+        with:
+          path: ~/.cache/ms-playwright
+          key: ${{ runner.os }}-playwright-1.60.0
+
       # `chromium` and the new `chromium-headless-shell` ship as separate
       # downloads; the browser-driven scenarios launch the headless shell.
       # Install from e2e so bunx resolves ITS pinned playwright (the version the
@@ -190,6 +252,14 @@ jobs:
         with:
           bun-version: 1.3.11
 
+      - name: Cache Bun package cache
+        uses: actions/cache@v4
+        with:
+          path: ~/.bun/install/cache
+          key: ${{ runner.os }}-bun-1.3.11-${{ hashFiles('bun.lock') }}
+          restore-keys: |
+            ${{ runner.os }}-bun-1.3.11-
+
       - run: bun install --frozen-lockfile
 
       - name: Build web app
@@ -221,3 +291,5 @@ jobs:
           file: apps/host-selfhost/Dockerfile
           push: false
           tags: executor-selfhost:ci
+          cache-from: type=gha
+          cache-to: type=gha,mode=max

apps/cloud/scripts/dev-db.ts

@@ -82,10 +82,41 @@ const db = await PGlite.create(DB_PATH);
 console.log(`[dev-db] Running migrations from ${MIGRATIONS_FOLDER}`);
 await migrate(drizzle(db), { migrationsFolder: MIGRATIONS_FOLDER });
 
+// `PGLiteSocketServer` defaults to `maxConnections: 1` and answers every extra
+// concurrent connection with "Too many connections" + an immediate socket
+// close. (pglite-socket 0.1.4's published index.d.ts documents "default: 100",
+// but the shipped runtime JS is `maxConnections ?? 1`, verified in the shipped
+// chunk, so the runtime default really is 1.) The cloud worker opens a fresh
+// postgres pool per request (the MCP auth seam rebuilds one on EVERY `/mcp`
+// request, see apps/cloud/src/mcp/auth.ts), so under concurrent load, exactly
+// what the e2e suite generates against one shared dev stack, the
+// second-and-later connects were rejected, and postgres.js reconnected in a
+// tight loop against the closed socket. That reconnect storm piled up
+// thousands of half-closed sockets, starved real queries, drove request
+// latency into the tens of seconds, and eventually hung the stack: the CI e2e
+// "cloud dev stack degrades after minutes of sustained load" cascade flake.
+// PGlite runs queries serially (its internal QueryQueueManager executes each
+// under `runExclusive`), so allowing many connections means they queue instead
+// of being rejected. One caveat makes that safe: stock pglite-socket 0.1.4
+// enqueues each wire FRAME separately, so two connections' extended-protocol
+// pipelines (Parse/Bind/Execute/Sync) would interleave inside the one shared
+// PGlite session and corrupt each other ("bind message supplies N parameters,
+// but prepared statement requires M" -> random 500s on whichever request lost
+// the race). The patch in patches/@electric-sql%2Fpglite-socket@0.1.4.patch
+// batches each socket data event into one queue entry and holds handler
+// affinity while a pipeline is open;
+// src/db/dev-db-socket-concurrency.node.test.ts is the regression test.
 const server = new PGLiteSocketServer({
   db,
   port: PORT,
   host: "127.0.0.1",
+  maxConnections: Number(process.env.DEV_DB_MAX_CONNECTIONS ?? 1000),
+  // Backstop for pipeline affinity: a client that stalls mid-pipeline (Parse
+  // sent, no Sync) with its socket still OPEN would hold the queue's handler
+  // affinity forever and starve every other connection, since affinity only
+  // releases on detach and detach needs close/error/idle-timeout. In ms; the
+  // timer resets on every data event, so only a genuinely dead client trips it.
+  idleTimeout: Number(process.env.DEV_DB_IDLE_TIMEOUT_MS ?? 30_000),
 });
 
 await server.start();

apps/cloud/src/db/db.close.test.ts

@@ -0,0 +1,109 @@
+// Contract: the postgres pool finalizer must AWAIT the connection teardown.
+//
+// The cloud MCP auth seam (`makeMcpOrganizationAuthServices`) builds a fresh
+// postgres pool on EVERY `/mcp` request and closes it in its `acquireRelease`
+// finalizer. That finalizer used to be fire-and-forget (`Effect.runFork(
+// sql.end({ timeout: 0 }))`): it returned before the socket was torn down, so
+// under sustained MCP load closed-but-unreaped sockets piled up against the
+// dev PGlite server (effectively single-connection) faster than it reaped
+// them. New connects queued behind the backlog, request latency climbed into
+// the tens of seconds, and the e2e cloud dev stack hung after a few minutes:
+// the CI cascade flake.
+//
+// These tests characterize the contract the old fire-and-forget close violated
+// (`closePostgres` itself is new alongside them): it must (a) call `sql.end`
+// with a NON-zero drain window (a clean Terminate, not an abandon) and (b)
+// return an Effect that does not complete until `sql.end` has resolved, even
+// when the teardown takes real wall-clock time. All asserted with a fake `sql`
+// whose `end()` completion is observable, so the tests are fast and need no
+// live database.
+
+import { describe, expect, it } from "@effect/vitest";
+import { Effect, Exit } from "effect";
+
+import { POSTGRES_END_TIMEOUT_SECONDS, closePostgres } from "./db";
+
+describe("closePostgres", () => {
+  it.effect("passes a non-zero drain window to sql.end (clean Terminate, not abandon)", () =>
+    Effect.gen(function* () {
+      let received: { timeout?: number } | undefined;
+      const fakeSql = {
+        end: (options?: { timeout?: number }) => {
+          received = options;
+          return Promise.resolve();
+        },
+      };
+
+      yield* closePostgres(fakeSql);
+
+      expect(received?.timeout).toBe(POSTGRES_END_TIMEOUT_SECONDS);
+      expect(POSTGRES_END_TIMEOUT_SECONDS).toBeGreaterThan(0);
+    }),
+  );
+
+  it.effect("does not complete until sql.end resolves (awaits the teardown)", () =>
+    Effect.gen(function* () {
+      // `end` records an ordering marker only after an async tick. If
+      // `closePostgres` awaits it, the "close completed" marker lands AFTER the
+      // "end resolved" marker. The old fire-and-forget close returned before
+      // `end` ran, so "close completed" would land FIRST.
+      const order: string[] = [];
+      const fakeSql = {
+        end: () =>
+          // Defer resolution across a microtask so a non-awaiting close would
+          // observably finish before this runs.
+          Promise.resolve()
+            .then(() => Promise.resolve())
+            .then(() => {
+              order.push("end-resolved");
+            }),
+      };
+
+      yield* closePostgres(fakeSql);
+      order.push("close-completed");
+
+      // Awaiting the teardown means end resolved strictly before close returned.
+      expect(order).toEqual(["end-resolved", "close-completed"]);
+    }),
+  );
+
+  it.effect("awaits a teardown that takes real wall-clock time (bounded by the ceiling)", () =>
+    Effect.gen(function* () {
+      // `end` resolves only after a real timer delay, not just a microtask.
+      // This pins the "we await to completion, the timeout is only a ceiling"
+      // contract: closePostgres must stay suspended across the delay rather
+      // than resolving early.
+      const order: string[] = [];
+      const fakeSql = {
+        end: () =>
+          new Promise<void>((resolve) => {
+            setTimeout(() => {
+              order.push("end-resolved");
+              resolve();
+            }, 75);
+          }),
+      };
+
+      const startedAt = Date.now();
+      yield* closePostgres(fakeSql);
+      order.push("close-completed");
+
+      expect(order).toEqual(["end-resolved", "close-completed"]);
+      // Slack of a few ms: platform timers may fire marginally early.
+      expect(Date.now() - startedAt).toBeGreaterThanOrEqual(70);
+    }),
+  );
+
+  it.effect("swallows sql.end failures (a teardown error must not fail the request scope)", () =>
+    Effect.gen(function* () {
+      const fakeSql = {
+        // A rejected teardown (connection already gone) must not surface as a
+        // scope failure.
+        // oxlint-disable-next-line executor/no-promise-reject -- test fake: model `sql.end` (a raw postgres.js promise) rejecting
+        end: () => new Promise<void>((_resolve, reject) => reject("connection already gone")),
+      };
+      const exit = yield* Effect.exit(closePostgres(fakeSql));
+      expect(Exit.isSuccess(exit)).toBe(true);
+    }),
+  );
+});

apps/cloud/src/db/db.ts

@@ -67,22 +67,53 @@ const makeSql = (): Sql =>
     onnotice: () => undefined,
   });
 
+/**
+ * Graceful drain window (seconds) handed to `postgres`'s `sql.end`. Small but
+ * non-zero: `timeout: 0` closes immediately without waiting for a clean
+ * Terminate, which leaves the socket half-closed for the server to reap on its
+ * own schedule. A short drain lets postgres.js finish the wire teardown.
+ *
+ * This is a `Promise.race` CEILING, not a fixed wait: postgres.js races
+ * `end({ timeout })` against the actual teardown, and an idle connection's
+ * `end()` calls `terminate()` immediately and resolves as soon as the socket
+ * closes (sub-millisecond). Awaiting it in the request scope therefore adds no
+ * meaningful latency on the common path; the 5s only bounds how long a
+ * connection still mid-query can hold the scope open.
+ */
+export const POSTGRES_END_TIMEOUT_SECONDS = 5;
+
+/**
+ * Close a postgres pool and AWAIT its teardown.
+ *
+ * The `DbService` layers ({@link DbService.Live}, {@link makeDbLayer}) run this
+ * as their `acquireRelease` finalizer. The MCP auth seam
+ * (`makeMcpOrganizationAuthServices`) builds a FRESH pool on EVERY `/mcp`
+ * request, so this finalizer runs per request under sustained load.
+ *
+ * It used to be fire-and-forget (`Effect.runFork(sql.end({ timeout: 0 }))`),
+ * which returned before the connection was actually torn down. The abandoned
+ * sockets piled up against the dev PGlite server (effectively single-connection)
+ * faster than it reaped them; new connects then queued behind the backlog, so
+ * request latency climbed into the tens of seconds and the stack eventually
+ * hung — the CI e2e "cloud dev stack degrades after minutes of sustained load"
+ * cascade. Awaiting the close bounds the number of live-plus-closing sockets to
+ * what is actually in flight. It runs inside the request's own Effect scope, so
+ * it respects workerd's per-request I/O rule.
+ */
+export const closePostgres = (sql: Pick<Sql, "end">): Effect.Effect<void> =>
+  Effect.ignore(
+    Effect.tryPromise({
+      try: () => sql.end({ timeout: POSTGRES_END_TIMEOUT_SECONDS }),
+      catch: (cause) => cause,
+    }),
+  );
+
 const makePostgresResource = (): DbResource => {
   const sql = makeSql();
   return {
     sql,
     db: drizzle(sql, { schema: combinedSchema }) as DrizzleDb,
-    close: () =>
-      Effect.sync(() => {
-        void Effect.runFork(
-          Effect.ignore(
-            Effect.tryPromise({
-              try: () => sql.end({ timeout: 0 }),
-              catch: (cause) => cause,
-            }),
-          ),
-        );
-      }),
+    close: () => closePostgres(sql),
   };
 };