diff --git a/CHANGELOG.md b/CHANGELOG.md index 65d99fec..3c658b49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### The server connects to Postgres on Windows, and `localhost` is no longer a coin toss + +Two separate faults, both of which stop a deployment reaching its own database and neither of which +says so. + +The connection address went to Bun as a URL. Bun reads such a URL's path, the database name, as the +path of a unix socket, ignores the host and the port, and fails to open a socket Windows does not +have (oven-sh/bun#27713). The server could not reach Postgres there at all, while `psql` inside the +container and a plain TCP connection from the same machine both worked, which makes it look like a +network fault rather than a parsing one. The address is now passed in parts, and `DATABASE_URL` is +removed from the environment as it is read, because Bun prefers that variable to the parts it was +handed and would otherwise put the address straight back through the same parser. A URL with no host +or no database is now refused by name instead of connecting somewhere nobody chose. + +Separately, Compose published its loopback ports on `127.0.0.1` only. `localhost` resolves to `::1` +and `127.0.0.1` in an order the platform decides, and a client handed `::1` first does not fall back +to the other, so the same configuration worked on one machine and failed on the next for a reason +nothing in the error mentions. Every loopback port is now published on both addresses. Both are +loopback, so nothing became reachable from another host. + ### A bad `COMPUTER_MEMORY_BYTES` refuses to start the supervisor, instead of capping a computer at 512 bytes `COMPUTER_MEMORY_BYTES=512m` used to parse as `512` via `parseInt`, which Docker accepts as a memory diff --git a/docker-compose.yml b/docker-compose.yml index 6fca5a72..b4bb122e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -24,7 +24,15 @@ services: # the name and leaves the address. Proven from inside `agent-computer`, where the gateway on # :5432 answered and began authenticating as `openbot` on `openbot`, with the password in # this file. The server on the host still reaches it, because that is what loopback is. + # Both loopback addresses, because `localhost` is not one address. + # + # It resolves to ::1 and 127.0.0.1 in an order the platform decides, and a client that gets + # ::1 first does not fall back: Bun and Node both fail the connection outright rather than + # trying the other. Publishing on one of them makes `localhost` work on some machines and not + # others, for a reason nothing in the error mentions. Publishing on both is still loopback, + # so nothing here is reachable from another host. - "127.0.0.1:${POSTGRES_PORT:-5432}:5432" + - "[::1]:${POSTGRES_PORT:-5432}:5432" volumes: - postgres-data:/var/lib/postgresql/data healthcheck: @@ -74,6 +82,7 @@ services: # Loopback only. This process drives a browser holding real logins; COMPUTER_TOKEN is the # request control, and loopback keeps the surface off routed networks. - "127.0.0.1:${COMPUTER_PORT:-4100}:4100" + - "[::1]:${COMPUTER_PORT:-4100}:4100" # Per-Bot egress, in a file of its own because the names are not knowable here. # # `EGRESS_PROXY_` is derived from the Bot's id, so there is no fixed list to write out the @@ -217,6 +226,7 @@ services: # COMPUTER_NETWORK and reaches the supervisor as `supervisor:4300`, which is unaffected because # the process still listens on every interface inside its own container. - "127.0.0.1:${SUPERVISOR_PORT:-4500}:4300" + - "[::1]:${SUPERVISOR_PORT:-4500}:4300" volumes: # Read-only because this service only ever needs to ask; it is still root-equivalent, which is # the whole reason nothing else here gets it. @@ -253,6 +263,7 @@ services: # on the machine before they can even try it. Nothing legitimate reaches a Bot from another # host: the server calls it over localhost, and other containers use the compose network. - "127.0.0.1:${BOT_PORT:-4200}:4200" + - "[::1]:${BOT_PORT:-4200}:4200" environment: OPENAI_API_KEY: ${OPENAI_API_KEY} # Server sends this on every call to the managed Bot. It refuses to start without it. @@ -281,6 +292,7 @@ services: ports: # Loopback, for the same reason as agent-bot above. - "127.0.0.1:${LANGGRAPH_PORT:-4201}:4201" + - "[::1]:${LANGGRAPH_PORT:-4201}:4201" environment: # The selected provider reads its own key. Models requiring the Responses API use # BOT_RESPONSES_API instead of changing the streaming loop here. diff --git a/server/src/db/client.ts b/server/src/db/client.ts index 2aa57a45..5aa79640 100644 --- a/server/src/db/client.ts +++ b/server/src/db/client.ts @@ -8,6 +8,60 @@ import * as schema from "./schema"; * transaction; a pool of one turns that from a load-dependent production hang into an immediate, * reproducible failure. */ +/** + * The address, taken apart, because Bun will not take it whole on every platform. + * + * `new SQL("postgres://user:pass@host:5432/openbot")` works on macOS and Linux and cannot work on + * Windows: Bun reads the URL's path, `/openbot`, as the path of a unix socket, ignores the host and + * the port, and fails to open a socket that Windows does not have (oven-sh/bun#27713). The server + * then cannot reach Postgres at all there, while `psql` inside the container and a plain TCP + * connection from the same machine both succeed, which is what makes it look like a network fault + * and not a parsing one. + * + * Passing the parts leaves nothing to parse. The behaviour is identical where the URL already + * worked, since these are the same values Bun would have derived. + */ +function addressOf(databaseUrl: string) { + let url: URL; + try { + url = new URL(databaseUrl); + } catch { + throw new TypeError( + `DATABASE_URL is not a URL: ${JSON.stringify(databaseUrl)}`, + ); + } + if (url.hostname === "") { + throw new TypeError( + "DATABASE_URL names no host. Expected postgres://user:password@host:port/database.", + ); + } + const database = decodeURIComponent(url.pathname.replace(/^\//, "")); + if (database === "") { + throw new TypeError( + "DATABASE_URL names no database. Expected postgres://user:password@host:port/database.", + ); + } + /* + * The query string is carried across as connection parameters, not dropped. + * + * `?application_name=…` is the one that matters here: the profile store's serialization tests + * name a session that way and then look for it in `pg_stat_activity`, so losing it turns a lock + * test into a three second timeout with nothing to say why. Anything else Postgres accepts on a + * URL, `sslmode` and the rest, travels the same way. + */ + const connection = Object.fromEntries(url.searchParams); + + return { + adapter: "postgres" as const, + hostname: url.hostname, + port: url.port === "" ? 5432 : Number(url.port), + username: decodeURIComponent(url.username), + password: decodeURIComponent(url.password), + database, + ...(Object.keys(connection).length > 0 ? { connection } : {}), + }; +} + export function createDatabase( databaseUrl: string, options: { max?: number } = {}, @@ -26,10 +80,25 @@ export function createDatabase( "createDatabase needs a connection string as its first argument. Pool options go second.", ); } - const client = - options.max === undefined - ? new SQL(databaseUrl) - : new SQL(databaseUrl, { max: options.max }); + /* + * `$DATABASE_URL` is taken out of the environment first, and stays out. + * + * Passing the parts is not enough on its own: Bun reads `$DATABASE_URL` when one is set and + * prefers it to what the caller passed, so the address goes back through the parser this exists + * to avoid and Windows fails exactly as before. Observed, not assumed: the options form connects + * from a Bun script with no `$DATABASE_URL` set and fails inside the server, which is started + * with `--env-file`, until the variable is gone. + * + * Nothing else reads it after this point. `loadConfig` has already captured it, and the worker + * reads it into a local before it opens a database. Removing it also means a later + * `new SQL()` cannot silently connect somewhere nobody named. + */ + delete process.env.DATABASE_URL; + + const client = new SQL({ + ...addressOf(databaseUrl), + ...(options.max === undefined ? {} : { max: options.max }), + }); return drizzle({ client, schema }); } diff --git a/server/tests/db-client-address.test.ts b/server/tests/db-client-address.test.ts new file mode 100644 index 00000000..47bdca73 --- /dev/null +++ b/server/tests/db-client-address.test.ts @@ -0,0 +1,69 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { createDatabase } from "../src/db/client"; + +/** + * The address goes to Bun in parts, and `$DATABASE_URL` does not survive the call. + * + * Both halves matter and only together. Bun reads a connection URL's path as the path of a unix + * socket, so `postgres://…/openbot` cannot connect on Windows (oven-sh/bun#27713); and it prefers + * `$DATABASE_URL` to the options it was handed, so passing the parts while the variable is still + * set changes nothing. These assert the observable half: what the environment looks like + * afterwards, and which addresses are refused before a socket is ever opened. + */ +const original = process.env.DATABASE_URL; + +afterEach(() => { + if (original === undefined) delete process.env.DATABASE_URL; + else process.env.DATABASE_URL = original; +}); + +describe("the database address", () => { + test("is taken out of the environment, so Bun cannot prefer it to the parts", () => { + process.env.DATABASE_URL = + "postgres://openbot:openbot@127.0.0.1:5432/openbot"; + + createDatabase("postgres://openbot:openbot@127.0.0.1:5432/openbot"); + + expect(process.env.DATABASE_URL).toBeUndefined(); + }); + + test("refuses a connection string that is not a URL, naming what it got", () => { + expect(() => createDatabase("://openbot@/openbot")).toThrow( + /DATABASE_URL is not a URL/, + ); + }); + + test("refuses a URL with no host, which would otherwise parse and connect nowhere", () => { + // `new URL` accepts this: the scheme is "openbot:" and there is no host at all. + expect(() => createDatabase("openbot:openbot@localhost/openbot")).toThrow( + /names no host/, + ); + }); + + test("refuses a URL that names no database, rather than connecting to a default", () => { + expect(() => + createDatabase("postgres://openbot:openbot@127.0.0.1:5432"), + ).toThrow(/names no database/); + }); + + test("still refuses pool options where the address belongs", () => { + // @ts-expect-error the wrong-way-round call this guard exists for + expect(() => createDatabase({ max: 1 })).toThrow(/connection string/); + }); +}); + +describe("connection parameters on the URL", () => { + test("survive, because a dropped application_name turns a lock test into a timeout", async () => { + const named = createDatabase( + "postgres://openbot:openbot@127.0.0.1:5432/openbot?application_name=db_client_address_probe", + ); + + const rows = await named.execute( + "select application_name from pg_stat_activity where pid = pg_backend_pid()", + ); + + expect( + (rows as Array<{ application_name: string }>)[0]?.application_name, + ).toBe("db_client_address_probe"); + }); +});