Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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_<BOT>` is derived from the Bot's id, so there is no fixed list to write out the
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
77 changes: 73 additions & 4 deletions server/src/db/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } = {},
Expand All @@ -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 });
}
Expand Down
69 changes: 69 additions & 0 deletions server/tests/db-client-address.test.ts
Original file line number Diff line number Diff line change
@@ -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");
});
});