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. 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); }, };