From dfa15391d75dd49343a22f172419406eef8f6f50 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 20 Oct 2026 05:13:14 +0000 Subject: [PATCH 1/2] fix: open one MongoDB client per Worker request workerd ties a socket to the request that opened it. The cached KeyvMongo client in the Worker served later requests and kept driver monitoring alive after its request ended, so bin requests failed with an uncaught error that Cloudflare reports as 1101. The Keyv store now opens a client inside the request scope, carried by AsyncLocalStorage through handleAsNodeRequest, and closes it after the response in ctx.waitUntil. Nothing is cached at isolate scope. An operation outside a request opens and closes its own client. An error that escapes the Node bridge becomes a redacted JSON 500 instead of a Worker exception. An opt-in workerd test runs the create/capture/get/list loop against a real MongoDB when MOCKHTTP_TEST_MONGODB_URI is set. Co-authored-by: Jared Wray --- test/bin-store-scope.test.ts | 395 +++++++++++++++++++++++++++++++++++ test/worker-runtime.test.ts | 91 ++++++++ test/worker.test.ts | 231 ++++++++++++++------ worker/app.ts | 57 ++++- worker/bin-store.ts | 378 ++++++++++++++++++++++++++------- worker/index.ts | 13 +- 6 files changed, 1017 insertions(+), 148 deletions(-) create mode 100644 test/bin-store-scope.test.ts diff --git a/test/bin-store-scope.test.ts b/test/bin-store-scope.test.ts new file mode 100644 index 0000000..91d00e4 --- /dev/null +++ b/test/bin-store-scope.test.ts @@ -0,0 +1,395 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +const { FakeKeyvMongo } = vi.hoisted(() => { + type Listener = (error: unknown) => void; + + /** Stands in for `@keyv/mongo`. Instances share one map, like one database. */ + class FakeKeyvMongo { + static instances: FakeKeyvMongo[] = []; + static readonly shared = new Map(); + static failConnect: Error | undefined; + static failDisconnect: Error | undefined; + static failGet: Error | undefined; + static disconnects = 0; + + readonly url: string; + readonly options: Record | undefined; + readonly connect: Promise; + private readonly listeners: Listener[] = []; + + constructor(url: string, options?: Record) { + this.url = url; + this.options = options; + FakeKeyvMongo.instances.push(this); + if (FakeKeyvMongo.failConnect) { + this.connect = Promise.reject(FakeKeyvMongo.failConnect); + void this.connect.catch(() => undefined); + } else { + this.connect = Promise.resolve("ok"); + } + } + + on(event: string, listener: Listener): this { + if (event === "error") { + this.listeners.push(listener); + } + return this; + } + + emitError(error: unknown): void { + for (const listener of this.listeners) { + listener(error); + } + } + + async disconnect(): Promise { + FakeKeyvMongo.disconnects += 1; + if (FakeKeyvMongo.failDisconnect) { + throw FakeKeyvMongo.failDisconnect; + } + } + + async get(key: string): Promise { + if (FakeKeyvMongo.failGet) { + throw FakeKeyvMongo.failGet; + } + return FakeKeyvMongo.shared.get(key); + } + + async getMany(keys: string[]): Promise { + return keys.map((key) => FakeKeyvMongo.shared.get(key)); + } + + async has(key: string): Promise { + return FakeKeyvMongo.shared.has(key); + } + + async hasMany(keys: string[]): Promise { + return keys.map((key) => FakeKeyvMongo.shared.has(key)); + } + + async set(key: string, value: unknown): Promise { + FakeKeyvMongo.shared.set(key, value); + return true; + } + + async setMany( + entries: Array<{ key: string; value: unknown }>, + ): Promise { + for (const entry of entries) { + FakeKeyvMongo.shared.set(entry.key, entry.value); + } + return entries.map(() => true); + } + + async delete(key: string): Promise { + return FakeKeyvMongo.shared.delete(key); + } + + async deleteMany(keys: string[]): Promise { + return keys.map((key) => FakeKeyvMongo.shared.delete(key)); + } + + async clear(): Promise { + FakeKeyvMongo.shared.clear(); + } + + async *iterator(): AsyncGenerator<[string, unknown]> { + for (const entry of FakeKeyvMongo.shared) { + yield entry; + } + } + + static reset(): void { + FakeKeyvMongo.instances.length = 0; + FakeKeyvMongo.shared.clear(); + FakeKeyvMongo.failConnect = undefined; + FakeKeyvMongo.failDisconnect = undefined; + FakeKeyvMongo.failGet = undefined; + FakeKeyvMongo.disconnects = 0; + } + } + + return { FakeKeyvMongo }; +}); + +vi.mock("@keyv/mongo", () => ({ default: FakeKeyvMongo })); + +const { + RequestScopedMongoStore, + closeBinStoreScope, + createAdaptiveBinKeyv, + createBinStoreScope, + currentBinStoreScope, + runInBinStoreScope, +} = await import("../worker/bin-store.js"); + +const URI = "mongodb+srv://user:pass@cluster0.example.invalid/mockhttp"; + +describe("request scoped mongo store", () => { + afterEach(() => { + FakeKeyvMongo.reset(); + vi.restoreAllMocks(); + }); + + it("shares one client across the operations of a scope and closes it once", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const scope = createBinStoreScope(); + expect(currentBinStoreScope()).toBeUndefined(); + + await runInBinStoreScope(scope, async () => { + expect(currentBinStoreScope()).toBe(scope); + expect(await store.set("a", 1)).toBe(true); + expect(await store.setMany([{ key: "b", value: 2 }])).toEqual([true]); + expect(await store.get("a")).toBe(1); + expect(await store.getMany(["a", "b"])).toEqual([1, 2]); + expect(await store.has("a")).toBe(true); + expect(await store.hasMany(["a", "zzz"])).toEqual([true, false]); + const seen: unknown[] = []; + for await (const entry of store.iterator()) { + seen.push(entry); + } + expect(seen).toEqual([ + ["a", 1], + ["b", 2], + ]); + expect(await store.delete("b")).toBe(true); + expect(await store.deleteMany(["a"])).toEqual([true]); + await store.clear(); + }); + + expect(FakeKeyvMongo.instances).toHaveLength(1); + expect(FakeKeyvMongo.instances[0]?.url).toBe(URI); + expect(FakeKeyvMongo.disconnects).toBe(0); + expect(scope.pending.size).toBeGreaterThan(0); + + await closeBinStoreScope(scope); + expect(scope.closed).toBe(true); + expect(scope.client).toBeUndefined(); + expect(scope.pending.size).toBe(0); + expect(FakeKeyvMongo.disconnects).toBe(1); + expect(console.info).toHaveBeenCalledTimes(1); + expect(vi.mocked(console.info).mock.calls.join("\n")).toContain( + "keyv.connect", + ); + + // Closing again is a no-op. + await closeBinStoreScope(scope); + expect(FakeKeyvMongo.disconnects).toBe(1); + }); + + it("waits for operations still running when the scope closes", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const scope = createBinStoreScope(); + let release: () => void = () => undefined; + const gate = new Promise((resolve) => { + release = resolve; + }); + const client = await runInBinStoreScope(scope, async () => { + await store.set("k", "v"); + return FakeKeyvMongo.instances[0]; + }); + const slowGet = vi + .spyOn(client as FakeKeyvMongo, "get") + .mockImplementation(async () => { + await gate; + return "late"; + }); + const read = runInBinStoreScope(scope, () => store.get("k")); + + let closed = false; + const closing = closeBinStoreScope(scope).then(() => { + closed = true; + }); + await new Promise((resolve) => setTimeout(resolve, 5)); + expect(closed).toBe(false); + expect(FakeKeyvMongo.disconnects).toBe(0); + + release(); + expect(await read).toBe("late"); + await closing; + expect(closed).toBe(true); + expect(slowGet).toHaveBeenCalledOnce(); + expect(FakeKeyvMongo.disconnects).toBe(1); + }); + + it("opens and closes a client per operation outside a scope or after close", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + + expect(await store.set("x", 1)).toBe(true); + expect(FakeKeyvMongo.instances).toHaveLength(1); + expect(FakeKeyvMongo.disconnects).toBe(1); + + const seen: unknown[] = []; + for await (const entry of store.iterator()) { + seen.push(entry); + } + expect(seen).toEqual([["x", 1]]); + expect(FakeKeyvMongo.instances).toHaveLength(2); + expect(FakeKeyvMongo.disconnects).toBe(2); + + const scope = createBinStoreScope(); + await closeBinStoreScope(scope); + await runInBinStoreScope(scope, async () => { + expect(await store.get("x")).toBe(1); + const again: unknown[] = []; + for await (const entry of store.iterator()) { + again.push(entry); + } + expect(again).toEqual([["x", 1]]); + }); + expect(scope.client).toBeUndefined(); + expect(FakeKeyvMongo.instances).toHaveLength(4); + expect(FakeKeyvMongo.disconnects).toBe(4); + + // Nothing is open outside a request, so disconnect has nothing to do. + await store.disconnect(); + expect(FakeKeyvMongo.disconnects).toBe(4); + }); + + it("disconnect closes the current scope's client", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const scope = createBinStoreScope(); + await runInBinStoreScope(scope, async () => { + await store.set("k", "v"); + await store.disconnect(); + }); + expect(scope.closed).toBe(true); + expect(FakeKeyvMongo.disconnects).toBe(1); + }); + + it("fails cleanly when the URI disappears before the first operation", async () => { + const store = new RequestScopedMongoStore(() => undefined); + await expect(store.get("k")).rejects.toMatchObject({ + name: "BinStoreFailure", + message: "MONGODB_URI is not set", + }); + expect(FakeKeyvMongo.instances).toHaveLength(0); + }); + + it("drops a client after a connection failure and skips closing it", async () => { + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const scope = createBinStoreScope(); + FakeKeyvMongo.failConnect = Object.assign( + new Error(`querySrv ENOTFOUND ${URI}`), + { name: "MongoNetworkError", code: "ENOTFOUND" }, + ); + FakeKeyvMongo.failGet = FakeKeyvMongo.failConnect; + + await runInBinStoreScope(scope, async () => { + await expect(store.get("k")).rejects.toMatchObject({ + name: "BinStoreFailure", + }); + // The failed client was dropped, so the next operation opens a new one. + expect(scope.client).toBeUndefined(); + FakeKeyvMongo.failConnect = undefined; + FakeKeyvMongo.failGet = undefined; + expect(await store.get("k")).toBeUndefined(); + }); + expect(FakeKeyvMongo.instances).toHaveLength(2); + await closeBinStoreScope(scope); + // Only the client that connected is closed. + expect(FakeKeyvMongo.disconnects).toBe(1); + const logged = errorLog.mock.calls.join("\n"); + expect(logged).toContain("keyv.connect"); + expect(logged).toContain("keyv.get"); + expect(logged).not.toContain("user:pass"); + expect(logged).not.toContain("cluster0"); + }); + + it("ignores a reset for a client the scope no longer holds", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + vi.spyOn(console, "error").mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const scope = createBinStoreScope(); + await runInBinStoreScope(scope, () => store.set("k", "v")); + const first = FakeKeyvMongo.instances[0] as FakeKeyvMongo; + await closeBinStoreScope(scope); + expect(FakeKeyvMongo.disconnects).toBe(1); + + const failure = Object.assign(new Error("connection reset"), { + name: "MongoNetworkError", + }); + first.emitError(failure); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(FakeKeyvMongo.disconnects).toBe(1); + }); + + it("forwards store errors to Keyv and logs a failed close", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + const store = new RequestScopedMongoStore(() => URI); + const forwarded: unknown[] = []; + expect( + store.on("error", (error) => { + forwarded.push(error); + }), + ).toBe(store); + store.on("other", () => undefined); + expect(store.capabilities.expires).toBe(true); + expect(store.capabilities.compatible).toBe(true); + + FakeKeyvMongo.failDisconnect = new Error(`close failed ${URI}`); + const scope = createBinStoreScope(); + await runInBinStoreScope(scope, () => store.set("k", "v")); + const client = FakeKeyvMongo.instances[0] as FakeKeyvMongo; + const failure = new Error("write concern"); + client.emitError(failure); + expect(forwarded).toEqual([failure]); + + await closeBinStoreScope(scope); + const logged = errorLog.mock.calls.join("\n"); + expect(logged).toContain("keyv.error"); + expect(logged).toContain("keyv.close"); + expect(logged).not.toContain("user:pass"); + expect(logged).not.toContain("cluster0"); + }); + + it("logs a URI whose shape is dirty when the client opens", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + const dirty = '"mongodb://user:p@ss@cluster0.example.invalid/db "'; + const store = new RequestScopedMongoStore(() => dirty); + await store.set("k", "v"); + const logged = errorLog.mock.calls.join("\n"); + expect(logged).toContain("keyv.open"); + expect(logged).toContain('"extraAt":true'); + expect(logged).toContain('"whitespace":true'); + expect(logged).toContain('"wrappedQuotes":true'); + expect(logged).not.toContain("p@ss"); + }); + + it("adaptive Keyv switches between memory and MongoDB by URI", async () => { + vi.spyOn(console, "info").mockImplementation(() => undefined); + let uri: string | undefined; + const keyv = createAdaptiveBinKeyv(() => uri); + + await keyv.set("memory", { bin: "m" } as never); + expect(await keyv.get("memory")).toEqual({ bin: "m" }); + expect(FakeKeyvMongo.instances).toHaveLength(0); + expect(typeof keyv.store.iterator).toBe("function"); + + uri = URI; + expect(await keyv.get("memory")).toBeUndefined(); + await keyv.set("mongo", { bin: "x" } as never); + expect(await keyv.get("mongo")).toEqual({ bin: "x" }); + expect(typeof keyv.store.iterator).toBe("function"); + expect(keyv.store).toBeInstanceOf(RequestScopedMongoStore); + expect(FakeKeyvMongo.instances.length).toBeGreaterThan(0); + expect(FakeKeyvMongo.disconnects).toBe(FakeKeyvMongo.instances.length); + + uri = undefined; + expect(await keyv.get("memory")).toEqual({ bin: "m" }); + }); +}); diff --git a/test/worker-runtime.test.ts b/test/worker-runtime.test.ts index e95a371..b8d1bd5 100644 --- a/test/worker-runtime.test.ts +++ b/test/worker-runtime.test.ts @@ -222,6 +222,97 @@ describe("worker srv and unencoded password under workerd", () => { }, 60_000); }); +/** + * Needs a reachable MongoDB, for example `mongodb://127.0.0.1:27017/mockhttp`. + * Set MOCKHTTP_TEST_MONGODB_URI to run it. CI has no MongoDB, so CI skips it. + * Before the per-request client, the second bin request on an isolate failed + * with an uncaught "code had hung" error and the third one never answered. + */ +describe.skipIf(!process.env.MOCKHTTP_TEST_MONGODB_URI)( + "worker bins on MongoDB under workerd", + () => { + let worker: Unstable_DevWorker; + + beforeAll(async () => { + worker = await unstable_dev("worker/index.ts", { + ...devOptions, + port: 18794, + inspectorPort: 19236, + vars: { + MONGODB_URI: process.env.MOCKHTTP_TEST_MONGODB_URI as string, + }, + }); + }, 120_000); + + afterAll(async () => { + await worker?.stop(); + }); + + it("serves repeated create, capture, get, and list requests", async () => { + const health = await worker.fetch("https://mockhttp.org/healthz"); + expect(health.status, await textOf(health)).toBe(200); + + for (let round = 0; round < 5; round++) { + const created = await worker.fetch("https://mockhttp.org/bins", { + method: "POST", + }); + expect(created.status, await textOf(created)).toBe(200); + const bin = (await created.json()) as { id: string }; + + for (let i = 0; i < 3; i++) { + const captured = await worker.fetch( + `https://mockhttp.org/b/${bin.id}/hook`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ round, i }), + }, + ); + expect(captured.status, await textOf(captured)).toBe(200); + } + + for (let i = 0; i < 3; i++) { + const fetched = await worker.fetch( + `https://mockhttp.org/bins/${bin.id}`, + ); + expect(fetched.status, await textOf(fetched)).toBe(200); + expect(await fetched.json()).toMatchObject({ + id: bin.id, + requestCount: 3, + }); + } + + const requests = await worker.fetch( + `https://mockhttp.org/bins/${bin.id}/requests`, + ); + expect(requests.status, await textOf(requests)).toBe(200); + expect( + ((await requests.json()) as { requests: unknown[] }).requests, + ).toHaveLength(3); + + const missing = await worker.fetch( + "https://mockhttp.org/bins/does-not-exist", + ); + expect(missing.status, await textOf(missing)).toBe(404); + + const listed = await worker.fetch("https://mockhttp.org/bins"); + expect(listed.status, await textOf(listed)).toBe(200); + expect( + ((await listed.json()) as { bins: Array<{ id: string }> }).bins, + ).toEqual( + expect.arrayContaining([expect.objectContaining({ id: bin.id })]), + ); + + const removed = await worker.fetch( + `https://mockhttp.org/bins/${bin.id}`, + { method: "DELETE" }, + ); + expect(removed.status, await textOf(removed)).toBe(204); + } + }, 120_000); + }, +); + async function textOf(response: Response): Promise { return response.clone().text(); } diff --git a/test/worker.test.ts b/test/worker.test.ts index 3f63d3e..8c5da36 100644 --- a/test/worker.test.ts +++ b/test/worker.test.ts @@ -2,11 +2,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { readPublicFile, setPublicFileReader } from "../src/public-files.js"; const { handleAsNodeRequestMock, KeyvMongoMock } = vi.hoisted(() => { + /** + * Stands in for `@keyv/mongo`. Every instance reads and writes one shared + * map, like clients of one database. The Worker opens one instance per + * request, or one per operation outside a request. + */ class KeyvMongoMock { static instances: KeyvMongoMock[] = []; + static readonly shared = new Map(); readonly url: string; readonly options: Record | undefined; - readonly data = new Map(); + readonly connect: Promise; + private readonly listeners: Array<(error: unknown) => void> = []; constructor( url?: string | Record, @@ -26,29 +33,32 @@ const { handleAsNodeRequestMock, KeyvMongoMock } = vi.hoisted(() => { if (KeyvMongoMock.failConnect) { this.connect = Promise.reject(KeyvMongoMock.failConnect); void this.connect.catch(() => undefined); - } else if (KeyvMongoMock.resolveConnect) { + } else { this.connect = Promise.resolve("ok"); } - if (KeyvMongoMock.useDisconnect) { - this.disconnect = async () => { - KeyvMongoMock.disconnects += 1; - if (KeyvMongoMock.failDisconnect) { - throw new Error("disconnect failed"); - } - }; + } + + get data(): Map { + return KeyvMongoMock.shared; + } + + on(event: string, handler: (error: unknown) => void): this { + if (event === "error") { + this.listeners.push(handler); } - if (KeyvMongoMock.useOn) { - const listeners: Array<(error: unknown) => void> = []; - this.on = (event, handler) => { - if (event === "error") { - listeners.push(handler); - } - }; - this.emitError = (error) => { - for (const listener of listeners) { - listener(error); - } - }; + return this; + } + + emitError(error: unknown): void { + for (const listener of this.listeners) { + listener(error); + } + } + + async disconnect(): Promise { + KeyvMongoMock.disconnects += 1; + if (KeyvMongoMock.failDisconnect) { + throw new Error("disconnect failed"); } } @@ -93,16 +103,21 @@ const { handleAsNodeRequestMock, KeyvMongoMock } = vi.hoisted(() => { static failSetFalse = false; static failIterator: Error | undefined; static failConnect: Error | undefined; - static resolveConnect = false; - static useOn = false; - static useDisconnect = true; static failDisconnect = false; static disconnects = 0; - connect?: Promise; - on?: (event: string, handler: (error: unknown) => void) => void; - emitError?: (error: unknown) => void; - disconnect?: () => Promise; + static reset(): void { + KeyvMongoMock.instances.length = 0; + KeyvMongoMock.shared.clear(); + KeyvMongoMock.failConstruct = undefined; + KeyvMongoMock.failGet = undefined; + KeyvMongoMock.failSet = undefined; + KeyvMongoMock.failSetFalse = false; + KeyvMongoMock.failIterator = undefined; + KeyvMongoMock.failConnect = undefined; + KeyvMongoMock.failDisconnect = false; + KeyvMongoMock.disconnects = 0; + } } return { @@ -142,17 +157,7 @@ describe("cloudflare worker", () => { const dispatch = vi.fn(); afterEach(() => { - KeyvMongoMock.failConstruct = undefined; - KeyvMongoMock.failGet = undefined; - KeyvMongoMock.failSet = undefined; - KeyvMongoMock.failSetFalse = false; - KeyvMongoMock.failIterator = undefined; - KeyvMongoMock.failConnect = undefined; - KeyvMongoMock.resolveConnect = false; - KeyvMongoMock.useOn = false; - KeyvMongoMock.useDisconnect = true; - KeyvMongoMock.failDisconnect = false; - KeyvMongoMock.disconnects = 0; + KeyvMongoMock.reset(); workerMongoState.uri = undefined; dispatch.mockReset(); dispatch.mockResolvedValue(new Response("ok", { status: 200 })); @@ -214,6 +219,7 @@ describe("cloudflare worker", () => { headers: { "cf-connecting-ip": "203.0.113.10" }, }), { RATE_LIMITER: { limit } }, + undefined, dispatch, ); @@ -237,6 +243,7 @@ describe("cloudflare worker", () => { const response = await handleWorkerFetch( request, { RATE_LIMITER: { limit } }, + undefined, dispatch, ); @@ -259,6 +266,7 @@ describe("cloudflare worker", () => { await handleWorkerFetch( new Request("https://mockhttp.org/get"), { ASSETS: { fetch: fetchAsset } }, + undefined, dispatch, ); @@ -346,17 +354,7 @@ describe("cloudflare worker", () => { describe("worker Fastify app", () => { afterEach(() => { - KeyvMongoMock.failConstruct = undefined; - KeyvMongoMock.failGet = undefined; - KeyvMongoMock.failSet = undefined; - KeyvMongoMock.failSetFalse = false; - KeyvMongoMock.failIterator = undefined; - KeyvMongoMock.failConnect = undefined; - KeyvMongoMock.resolveConnect = false; - KeyvMongoMock.useOn = false; - KeyvMongoMock.useDisconnect = true; - KeyvMongoMock.failDisconnect = false; - KeyvMongoMock.disconnects = 0; + KeyvMongoMock.reset(); workerMongoState.uri = undefined; delete process.env.MONGODB_URI; }); @@ -451,13 +449,17 @@ describe("worker Fastify app", () => { }); expect(created.statusCode).toBe(200); const store = KeyvMongoMock.instances[0]; + // No request scope is active under `inject`, so the write opened its own + // client and closed it when it finished. expect(KeyvMongoMock.instances).toHaveLength(1); + expect(KeyvMongoMock.disconnects).toBe(1); expect(store?.url).toBe(uri); expect(store?.options).toEqual({ collection: "bins", - maxPoolSize: 5, + maxPoolSize: 1, minPoolSize: 0, serverSelectionTimeoutMS: 5_000, + connectTimeoutMS: 5_000, }); const id = created.json().id as string; expect(store?.data.size).toBeGreaterThan(0); @@ -484,10 +486,111 @@ describe("worker Fastify app", () => { }); expect(removed.statusCode).toBe(204); await mockHttp.bins.keyv.clear(); + // Every operation outside a request opened one client and closed it. + expect(KeyvMongoMock.instances.length).toBeGreaterThan(1); + expect(KeyvMongoMock.disconnects).toBe(KeyvMongoMock.instances.length); await mockHttp.close(); delete process.env.MONGODB_URI; }); + it("opens one client per Worker request and closes it after the response", async () => { + const uri = "mongodb+srv://user:pass@cluster.mongodb.net/mockhttp"; + KeyvMongoMock.instances.length = 0; + const mockHttp = await createWorkerApp({ MONGODB_URI: uri }); + const background: Promise[] = []; + const ctx = { + waitUntil: (promise: Promise) => { + background.push(promise); + }, + }; + + // One request that creates a bin, captures into it, and reads it back: + // three bin operations, one client. + let id = ""; + const response = await handleWorkerFetch( + new Request("https://mockhttp.org/bins", { method: "POST" }), + { MONGODB_URI: uri }, + ctx, + async () => { + const created = await mockHttp.server.inject({ + method: "POST", + url: "/bins", + }); + id = created.json().id as string; + const captured = await mockHttp.server.inject({ + method: "POST", + url: `/b/${id}`, + payload: { ok: true }, + }); + expect(captured.statusCode).toBe(200); + const fetched = await mockHttp.server.inject({ + method: "GET", + url: `/bins/${id}`, + }); + return new Response(fetched.body, { status: fetched.statusCode }); + }, + ); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ id, requestCount: 1 }); + expect(KeyvMongoMock.instances).toHaveLength(1); + // The close is handed to `ctx.waitUntil`, not awaited before the response. + expect(background).toHaveLength(1); + await Promise.all(background); + expect(KeyvMongoMock.disconnects).toBe(1); + + // The next request must not reuse that client. + const again = await handleWorkerFetch( + new Request(`https://mockhttp.org/bins/${id}`), + { MONGODB_URI: uri }, + ctx, + async () => { + const fetched = await mockHttp.server.inject({ + method: "GET", + url: `/bins/${id}`, + }); + return new Response(fetched.body, { status: fetched.statusCode }); + }, + ); + expect(again.status).toBe(200); + expect(KeyvMongoMock.instances).toHaveLength(2); + await Promise.all(background); + expect(KeyvMongoMock.disconnects).toBe(2); + await mockHttp.close(); + }); + + it("answers with JSON when the Node bridge itself throws", async () => { + const uri = "mongodb+srv://user:pass@cluster.mongodb.net/mockhttp"; + const errorLog = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + workerMongoState.uri = uri; + const response = await handleWorkerFetch( + new Request("https://mockhttp.org/bins"), + { MONGODB_URI: uri }, + undefined, + vi + .fn() + .mockRejectedValue( + new Error( + `Cannot perform I/O on behalf of a different request ${uri}`, + ), + ), + ); + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + const body = (await response.json()) as { message: string }; + expect(body).toMatchObject({ + statusCode: 500, + error: "Internal Server Error", + }); + expect(body.message).toContain("different request"); + expect(body.message).not.toContain("user:pass"); + const logged = errorLog.mock.calls.join("\n"); + expect(logged).toContain('"phase":"fetch"'); + expect(logged).not.toContain("user:pass"); + errorLog.mockRestore(); + }); + it("passes MONGODB_URI through without trimming or joining text", async () => { const uri = "mongodb://user:pass@cluster.example.invalid/mockhttp\n"; delete process.env.MONGODB_URI; @@ -618,6 +721,7 @@ describe("worker Fastify app", () => { await handleWorkerFetch( new Request("https://mockhttp.org/bins"), { MONGODB_URI: requestUri }, + undefined, vi.fn().mockResolvedValue(new Response("ok")), ); const created = await mockHttp.server.inject({ @@ -684,7 +788,8 @@ describe("worker Fastify app", () => { expect(logged).toContain("extraAt"); expect(logged).not.toContain(uri); expect(logged).not.toContain("p@ssw"); - expect(KeyvMongoMock.disconnects).toBe(1); + // A client whose connect failed has no topology to close. + expect(KeyvMongoMock.disconnects).toBe(0); await mockHttp.close(); KeyvMongoMock.failConnect = undefined; @@ -694,17 +799,24 @@ describe("worker Fastify app", () => { name: "MongoParseError", }, ); - KeyvMongoMock.useDisconnect = false; + KeyvMongoMock.failDisconnect = true; mockHttp = await createWorkerApp({ MONGODB_URI: "mongodb://user:pass@cluster.mongodb.net/mockhttp", }); - const noDisconnect = await mockHttp.server.inject({ + const connected = await mockHttp.server.inject({ method: "POST", url: "/bins", }); - expect(noDisconnect.statusCode).toBe(500); - expect(noDisconnect.body).toContain("Invalid connection string"); - expect(noDisconnect.body).not.toContain("user:pass"); + expect(connected.statusCode).toBe(500); + expect(connected.body).toContain("Invalid connection string"); + expect(connected.body).not.toContain("user:pass"); + // The failed write dropped the client once. The close failure is logged, not thrown. + expect(KeyvMongoMock.disconnects).toBe(1); + const closeLog = errorLog.mock.calls + .map((call) => String(call[0])) + .join("\n"); + expect(closeLog).toContain("keyv.close"); + expect(closeLog).not.toContain("user:pass"); errorLog.mockRestore(); await mockHttp.close(); }); @@ -717,7 +829,6 @@ describe("worker Fastify app", () => { const errorLog = vi .spyOn(console, "error") .mockImplementation(() => undefined); - KeyvMongoMock.resolveConnect = true; KeyvMongoMock.failSetFalse = true; const mockHttp = await createWorkerApp({ MONGODB_URI: uri }); const saved = await mockHttp.server.inject({ @@ -758,7 +869,6 @@ describe("worker Fastify app", () => { await mockHttp.close(); KeyvMongoMock.failIterator = undefined; - KeyvMongoMock.useOn = true; KeyvMongoMock.instances.length = 0; const again = await createWorkerApp({ MONGODB_URI: uri }); await again.server.inject({ method: "POST", url: "/bins" }); @@ -767,7 +877,8 @@ describe("worker Fastify app", () => { name: "MongoNetworkError", code: "ENOTFOUND", }); - expect(() => store?.emitError?.(failure)).toThrow(failure); + // Keyv has no error listener, so a forwarded store error is thrown. + expect(() => store?.emitError(failure)).toThrow(failure); expect(errorLog.mock.calls.join("\n")).toContain("keyv.error"); expect(errorLog.mock.calls.join("\n")).not.toContain("user:pass"); errorLog.mockRestore(); diff --git a/worker/app.ts b/worker/app.ts index b483b52..b5a20b0 100644 --- a/worker/app.ts +++ b/worker/app.ts @@ -1,11 +1,17 @@ import { handleAsNodeRequest } from "cloudflare:node"; -import { logBinStore } from "../src/bin-store-diagnostics.js"; +import { logBinStore, redactStoreError } from "../src/bin-store-diagnostics.js"; import { MockHttp, type MockHttpOptions } from "../src/mock-http.js"; import { MockHttpBins } from "../src/mock-http-bins.js"; import { setPublicFileReader } from "../src/public-files.js"; import { PUBLIC_PROTOCOL_HEADER } from "../src/routes/bins/management.js"; import { checkMemoryBinStore } from "../src/routes/health.js"; -import { binStoreRuntime, createAdaptiveBinKeyv } from "./bin-store.js"; +import { + binStoreRuntime, + closeBinStoreScope, + createAdaptiveBinKeyv, + createBinStoreScope, + runInBinStoreScope, +} from "./bin-store.js"; export const WORKER_PORT = 3000; @@ -35,6 +41,11 @@ export type WorkerEnv = { MONGODB_URI?: string; }; +/** The part of the Workers `ExecutionContext` this app uses. */ +export type WorkerExecutionContext = { + waitUntil(promise: Promise): void; +}; + /** * Atlas URI for this request. The Worker binding wins. An empty string is * treated as unset so a blank secret does not open a client. @@ -177,9 +188,37 @@ export async function listenWorkerApp(mockHttp: MockHttp): Promise { }); } +/** + * Last resort for an error that escapes the Node bridge. Fastify answers its + * own route errors. This keeps the Worker from throwing, which Cloudflare + * reports as error 1101 with no body. + */ +export function internalErrorResponse(error: unknown): Response { + logBinStore({ phase: "fetch", store: "worker", error }); + const info = redactStoreError(error, workerMongoState.uri); + return new Response( + JSON.stringify({ + statusCode: 500, + error: "Internal Server Error", + message: info.message, + }), + { + status: 500, + headers: { "content-type": "application/json; charset=utf-8" }, + }, + ); +} + +/** + * Handle one Worker request. + * The bin store client for this request opens on its first bin operation and + * closes after the response, in `ctx.waitUntil`. No client is shared between + * requests: workerd rejects I/O on a socket that a different request opened. + */ export async function handleWorkerFetch( request: Request, env: WorkerEnv, + ctx?: WorkerExecutionContext, dispatch: WorkerDispatch = workerRuntime.dispatch, ): Promise { applyWorkerMongoEnv(env); @@ -189,5 +228,17 @@ export async function handleWorkerFetch( } bindPublicAssets(env.ASSETS); - return dispatch(WORKER_PORT, requestWithPublicProtocol(request)); + const scope = createBinStoreScope(); + try { + return await runInBinStoreScope(scope, () => + dispatch(WORKER_PORT, requestWithPublicProtocol(request)), + ); + } catch (error) { + return internalErrorResponse(error); + } finally { + const closing = closeBinStoreScope(scope); + if (ctx) { + ctx.waitUntil(closing); + } + } } diff --git a/worker/bin-store.ts b/worker/bin-store.ts index cb87233..86d791a 100644 --- a/worker/bin-store.ts +++ b/worker/bin-store.ts @@ -1,9 +1,16 @@ +import { AsyncLocalStorage } from "node:async_hooks"; import net from "node:net"; import KeyvMongo from "@keyv/mongo"; -import Keyv from "keyv"; +import Keyv, { + type KeyvStorageCapability, + type KeyvStorageEntry, + type KeyvStorageGetResult, + keyvStorageCapability, +} from "keyv"; import { MongoClient, type MongoClientOptions } from "mongodb"; import { type BinHealth, + BinStoreFailure, isBinStoreConnectionFailure, logBinStore, mongoUriFacts, @@ -54,11 +61,16 @@ export function installSocketProxyGuard(): void { installSocketProxyGuard(); +/** + * One client serves one request, so one pooled connection is enough. + * The short timeouts keep a bad Atlas host from holding a request open. + */ const workerBinMongoOptions = { collection: "bins", - maxPoolSize: 5, + maxPoolSize: 1, minPoolSize: 0, serverSelectionTimeoutMS: 5_000, + connectTimeoutMS: 5_000, }; type WorkerBinKeyv = NonNullable; @@ -145,106 +157,310 @@ export function guardMongoStore( }); } -function watchMongoStore(store: object, uri: string, reset: () => void): void { - const source = store as { - on?: (event: "error", handler: (error: unknown) => void) => void; - connect?: Promise; - }; - if (typeof source.on === "function") { - source.on("error", (error) => { - noteFailure("keyv.error", uri, error, reset); +type ErrorHandler = (error: unknown) => void; + +function watchMongoStore( + store: KeyvMongo, + uri: string, + reset: () => void, + forward: readonly ErrorHandler[], +): void { + store.on("error", (error: unknown) => { + noteFailure("keyv.error", uri, error, reset); + for (const handler of forward) { + handler(error); + } + }); + const started = Date.now(); + void store.connect.then( + () => { + logBinStore({ + phase: "keyv.connect", + store: "keyv-mongo", + uri, + level: "info", + connected: true, + latencyMs: Date.now() - started, + }); + }, + (error: unknown) => { + noteFailure("keyv.connect", uri, error, reset); + }, + ); +} + +function logDirtyUri(uri: string): void { + const shape = uriShape(uri); + if (shape.extraAt || shape.whitespace || shape.wrappedQuotes) { + logBinStore({ + phase: "keyv.open", + store: "keyv-mongo", + uri, + connected: false, }); } - if (source.connect && typeof source.connect.then === "function") { - const started = Date.now(); - void source.connect.then( - () => { - logBinStore({ - phase: "keyv.connect", - store: "keyv-mongo", - uri, - level: "info", - connected: true, - latencyMs: Date.now() - started, - }); - }, - (error: unknown) => { - noteFailure("keyv.connect", uri, error, reset); - }, - ); +} + +/** One MongoDB client. It serves one request, or one operation outside a request. */ +type BinStoreClient = { + readonly uri: string; + readonly store: KeyvMongo; + readonly guarded: KeyvMongo; +}; + +/** + * The bin store work of one Worker request. + * workerd ties a socket to the request that opened it. A client that one request + * opened cannot serve a later request, and the driver cannot keep monitoring it + * after that request ends. So the client lives in this scope and closes with it. + */ +export type BinStoreScope = { + client: BinStoreClient | undefined; + closed: boolean; + readonly pending: Set>; +}; + +const scopeStorage = new AsyncLocalStorage(); + +export function createBinStoreScope(): BinStoreScope { + return { client: undefined, closed: false, pending: new Set() }; +} + +/** Run `fn` so that every bin operation inside it shares the scope's client. */ +export function runInBinStoreScope( + scope: BinStoreScope, + fn: () => Promise, +): Promise { + return scopeStorage.run(scope, fn); +} + +/** The scope of the current request, if one is active. */ +export function currentBinStoreScope(): BinStoreScope | undefined { + return scopeStorage.getStore(); +} + +async function disconnectClient(client: BinStoreClient): Promise { + // A client whose connect failed has no open topology. The driver closed it. + const connected = await client.store.connect.then( + () => true, + () => false, + ); + if (!connected) { + return; + } + try { + await client.store.disconnect(); + } catch (error) { + logBinStore({ + phase: "keyv.close", + store: "keyv-mongo", + uri: client.uri, + error, + }); } } -function logOpen(uri: string): void { - const shape = uriShape(uri); - const dirty = shape.extraAt || shape.whitespace || shape.wrappedQuotes; - logBinStore({ - phase: "keyv.open", - store: "keyv-mongo", +/** + * Close the scope's client. Operations still running in the scope finish first. + * An operation that starts after this call opens and closes its own client. + * This never rejects, so it is safe to hand to `ctx.waitUntil`. + */ +export async function closeBinStoreScope(scope: BinStoreScope): Promise { + scope.closed = true; + while (scope.pending.size > 0) { + const batch = [...scope.pending]; + await Promise.allSettled(batch); + for (const task of batch) { + scope.pending.delete(task); + } + } + const client = scope.client; + scope.client = undefined; + if (client) { + await disconnectClient(client); + } +} + +function trackPending(scope: BinStoreScope, task: Promise): Promise { + const settled = task.then( + () => undefined, + () => undefined, + ); + scope.pending.add(settled); + return task; +} + +function openClient( + uri: string, + scope: BinStoreScope, + forward: readonly ErrorHandler[], +): BinStoreClient { + logDirtyUri(uri); + let store: KeyvMongo; + try { + store = new KeyvMongo(uri, workerBinMongoOptions); + } catch (error) { + logBinStore({ phase: "keyv.construct", store: "keyv-mongo", uri, error }); + throw toSafeStoreError(error, uri); + } + // A connection failure drops the client. The next operation in this scope + // opens a new one. The close runs inside the scope so the request waits for it. + const reset = (): void => { + if (scope.client !== client) { + return; + } + scope.client = undefined; + trackPending(scope, disconnectClient(client)); + }; + const client: BinStoreClient = { uri, - level: dirty ? "error" : "info", - connected: !dirty, - }); + store, + guarded: guardMongoStore(store, uri, reset) as KeyvMongo, + }; + watchMongoStore(store, uri, reset, forward); + return client; } /** - * Keyv uses MongoDB when a URI is set, and memory when it is not. - * `@keyv/mongo` connects in its constructor. workerd rejects that network call - * during isolate startup, so the client opens on the first bin operation. + * Keyv storage adapter that opens a MongoDB client per request. + * Inside {@link runInBinStoreScope}, operations share the request's client. + * Outside a scope, an operation opens a client and closes it when done. + * Nothing is kept at module scope, so no socket outlives its request. */ -export function createAdaptiveBinKeyv( - getUri: () => string | undefined, -): WorkerBinKeyv { - let keyv: WorkerBinKeyv | undefined; - let openedFor: string | undefined; - const closing = new WeakSet(); - - const reset = (store: object) => { - keyv = undefined; - openedFor = undefined; - if (closing.has(store)) { - return; +export class RequestScopedMongoStore { + namespace?: string | undefined; + private readonly _listeners: ErrorHandler[] = []; + private readonly _getUri: () => string | undefined; + + constructor(getUri: () => string | undefined) { + this._getUri = getUri; + } + + get capabilities(): KeyvStorageCapability { + return keyvStorageCapability(this); + } + + /** Keyv forwards store errors to its own `error` event through this. */ + on(event: string, handler: ErrorHandler): this { + if (event === "error") { + this._listeners.push(handler); } - closing.add(store); - const disconnect = (store as { disconnect?: () => Promise }) - .disconnect; - if (typeof disconnect === "function") { - void disconnect.call(store).catch(() => undefined); + return this; + } + + async get(key: string): Promise> { + return this.run((store) => store.get(key)); + } + + async getMany( + keys: string[], + ): Promise>> { + return this.run((store) => store.getMany(keys)); + } + + async has(key: string): Promise { + return this.run((store) => store.has(key)); + } + + async hasMany(keys: string[]): Promise { + return this.run((store) => store.hasMany(keys)); + } + + async set(key: string, value: unknown, expires?: number): Promise { + return this.run((store) => store.set(key, value, expires)); + } + + async setMany( + entries: KeyvStorageEntry[], + ): Promise { + return this.run((store) => store.setMany(entries)); + } + + async delete(key: string): Promise { + return this.run((store) => store.delete(key)); + } + + async deleteMany(keys: string[]): Promise { + return this.run((store) => store.deleteMany(keys)); + } + + async clear(): Promise { + return this.run((store) => store.clear()); + } + + /** Close the current request's client. There is nothing to close outside a request. */ + async disconnect(): Promise { + const scope = scopeStorage.getStore(); + if (scope) { + await closeBinStoreScope(scope); } - }; + } - const open = (): WorkerBinKeyv => { - const uri = getUri(); - if (!uri) { - if (keyv && openedFor === undefined) { - return keyv; + async *iterator(): AsyncGenerator<[string, unknown], void> { + const scope = scopeStorage.getStore(); + if (scope && !scope.closed) { + const running = Promise.withResolvers(); + scope.pending.add(running.promise); + try { + yield* this.clientFor(scope).guarded.iterator() as AsyncIterable< + [string, unknown] + >; + } finally { + running.resolve(); } - keyv = new Keyv() as WorkerBinKeyv; - openedFor = undefined; - return keyv; + return; } - if (keyv && openedFor === uri) { - return keyv; + const own = createBinStoreScope(); + try { + yield* this.clientFor(own).guarded.iterator() as AsyncIterable< + [string, unknown] + >; + } finally { + await closeBinStoreScope(own); + } + } + + private async run(op: (store: KeyvMongo) => Promise): Promise { + const scope = scopeStorage.getStore(); + if (scope && !scope.closed) { + return trackPending(scope, op(this.clientFor(scope).guarded)); } - logOpen(uri); - let store: KeyvMongo; + const own = createBinStoreScope(); try { - store = new KeyvMongo(uri, workerBinMongoOptions); - } catch (error) { - logBinStore({ phase: "keyv.construct", store: "keyv-mongo", uri, error }); - throw toSafeStoreError(error, uri); + return await op(this.clientFor(own).guarded); + } finally { + await closeBinStoreScope(own); } - watchMongoStore(store, uri, () => reset(store)); - keyv = new Keyv({ - store: guardMongoStore(store, uri, () => reset(store)), - }) as WorkerBinKeyv; - openedFor = uri; - return keyv; - }; + } + + private clientFor(scope: BinStoreScope): BinStoreClient { + if (!scope.client) { + const uri = this._getUri(); + if (!uri) { + throw new BinStoreFailure("MONGODB_URI is not set"); + } + scope.client = openClient(uri, scope, this._listeners); + } + return scope.client; + } +} + +/** + * Keyv uses MongoDB when a URI is set, and memory when it is not. + * The MongoDB client opens on the first bin operation of a request and closes + * when that request ends. Nothing connects while the isolate starts. + */ +export function createAdaptiveBinKeyv( + getUri: () => string | undefined, +): WorkerBinKeyv { + const memory = new Keyv() as WorkerBinKeyv; + const mongo = new Keyv({ + store: new RequestScopedMongoStore(getUri), + }) as WorkerBinKeyv; return new Proxy({} as WorkerBinKeyv, { get(_target, property) { - const target = open(); + const target = getUri() ? mongo : memory; const value = Reflect.get(target, property, target); if (typeof value === "function") { return value.bind(target); diff --git a/worker/index.ts b/worker/index.ts index 494b691..f3cef51 100644 --- a/worker/index.ts +++ b/worker/index.ts @@ -4,17 +4,22 @@ import { handleWorkerFetch, listenWorkerApp, type WorkerEnv, + type WorkerExecutionContext, } from "./app.js"; // Fastify compiles routes with `new Function()`. workerd allows that only // while the isolate is starting, so the server has to listen at module load. // `cloudflare:workers` exposes MONGODB_URI here. The Mongo client connects -// later, on the first bin operation, because network I/O is rejected during -// startup. +// later, on the first bin operation of a request, because network I/O is +// rejected during startup. It closes when that request ends. await listenWorkerApp(await createWorkerApp(workerEnv)); export default { - async fetch(request: Request, env: WorkerEnv): Promise { - return handleWorkerFetch(request, env); + async fetch( + request: Request, + env: WorkerEnv, + ctx: WorkerExecutionContext, + ): Promise { + return handleWorkerFetch(request, env, ctx); }, }; From 9258478bf512f4516a9d3841268c64b3d321cdec Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 20 Oct 2026 05:13:14 +0000 Subject: [PATCH 2/2] docs: describe the per-request MongoDB client on Cloudflare Co-authored-by: Jared Wray --- site/docs/how-to-deploy/cloudflare.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/docs/how-to-deploy/cloudflare.md b/site/docs/how-to-deploy/cloudflare.md index 621488d..5e40f3f 100644 --- a/site/docs/how-to-deploy/cloudflare.md +++ b/site/docs/how-to-deploy/cloudflare.md @@ -25,7 +25,7 @@ The Worker starts the same MockHTTP app as Node.js, with a few options set for t The rate limiter key is the `cf-connecting-ip` header, then the first address in `x-forwarded-for`, then `unknown`. A limited request returns `429` with `retry-after: 60`. That limiter is shared across isolates. -Bins stay in memory in the isolate when `MONGODB_URI` is not set. A bin from one request can be absent when a different isolate handles the next request. When `MONGODB_URI` is set, bins are stored in MongoDB. The client connects on the first bin operation. It does not connect while the isolate starts. +Bins stay in memory in the isolate when `MONGODB_URI` is not set. A bin from one request can be absent when a different isolate handles the next request. When `MONGODB_URI` is set, bins are stored in MongoDB. Each request opens its own client on its first bin operation and closes it after the response, through `ctx.waitUntil`. No client is shared between requests: workerd rejects I/O on a socket that a different request opened, and a shared client made bin requests fail with error 1101. It does not connect while the isolate starts. Each bin request therefore pays one connection setup, which is a few round trips to Atlas. Percent-encode reserved characters in the username and the password. `@` is `%40`. An unencoded `@` makes the driver throw `Invalid connection string` before it contacts Atlas.