-
Notifications
You must be signed in to change notification settings - Fork 254
feat: support configurable HTTP bind host via --host / DBHUB_HOST env #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
152bd8f
1527f6a
185fa52
d32c7ab
0d35b8d
b084cc4
101773f
94be454
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,85 @@ | ||||||||||||||||||||||||||||
| import { describe, it, expect, beforeAll, afterAll } from 'vitest'; | ||||||||||||||||||||||||||||
| import { spawn, ChildProcess } from 'child_process'; | ||||||||||||||||||||||||||||
| import fs from 'fs'; | ||||||||||||||||||||||||||||
| import path from 'path'; | ||||||||||||||||||||||||||||
| import os from 'os'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| describe('HTTP bind host integration', () => { | ||||||||||||||||||||||||||||
| let serverProcess: ChildProcess | null = null; | ||||||||||||||||||||||||||||
| let testDbPath: string; | ||||||||||||||||||||||||||||
| const testPort = 3002; | ||||||||||||||||||||||||||||
| const testHost = '127.0.0.1'; | ||||||||||||||||||||||||||||
| const startupLogs: string[] = []; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| beforeAll(async () => { | ||||||||||||||||||||||||||||
| testDbPath = path.join(os.tmpdir(), `bind_host_test_${Date.now()}_${Math.random().toString(36).substr(2, 9)}.db`); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Invoke tsx directly via node to avoid pnpm.cmd resolution issues on Windows. | ||||||||||||||||||||||||||||
| const tsxCli = path.resolve(process.cwd(), 'node_modules', 'tsx', 'dist', 'cli.mjs'); | ||||||||||||||||||||||||||||
| const entry = path.resolve(process.cwd(), 'src', 'index.ts'); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| serverProcess = spawn(process.execPath, [tsxCli, entry, '--transport=http'], { | ||||||||||||||||||||||||||||
| env: { | ||||||||||||||||||||||||||||
| ...process.env, | ||||||||||||||||||||||||||||
| DSN: `sqlite://${testDbPath}`, | ||||||||||||||||||||||||||||
| HOST: testHost, | ||||||||||||||||||||||||||||
| PORT: testPort.toString(), | ||||||||||||||||||||||||||||
| NODE_ENV: 'test', | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| stdio: 'pipe', | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| serverProcess.stdout?.on('data', (data) => { | ||||||||||||||||||||||||||||
| startupLogs.push(data.toString()); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
| serverProcess.stderr?.on('data', (data) => { | ||||||||||||||||||||||||||||
| startupLogs.push(data.toString()); | ||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Wait for /healthz to respond on the configured host | ||||||||||||||||||||||||||||
| let ready = false; | ||||||||||||||||||||||||||||
| for (let i = 0; i < 30; i++) { | ||||||||||||||||||||||||||||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const res = await fetch(`http://${testHost}:${testPort}/healthz`); | ||||||||||||||||||||||||||||
| if (res.status === 200) { | ||||||||||||||||||||||||||||
| ready = true; | ||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||
| // not ready yet | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (!ready) { | ||||||||||||||||||||||||||||
| throw new Error(`Server did not bind to ${testHost}:${testPort} within timeout. Logs:\n${startupLogs.join('')}`); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| }, 45000); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| afterAll(async () => { | ||||||||||||||||||||||||||||
| if (serverProcess) { | ||||||||||||||||||||||||||||
| serverProcess.kill('SIGTERM'); | ||||||||||||||||||||||||||||
| await new Promise<void>((resolve) => { | ||||||||||||||||||||||||||||
| if (!serverProcess) return resolve(); | ||||||||||||||||||||||||||||
| serverProcess.on('exit', () => resolve()); | ||||||||||||||||||||||||||||
| setTimeout(() => { | ||||||||||||||||||||||||||||
| if (serverProcess && !serverProcess.killed) serverProcess.kill('SIGKILL'); | ||||||||||||||||||||||||||||
| resolve(); | ||||||||||||||||||||||||||||
| }, 5000); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| serverProcess.on('exit', () => resolve()); | |
| setTimeout(() => { | |
| if (serverProcess && !serverProcess.killed) serverProcess.kill('SIGKILL'); | |
| resolve(); | |
| }, 5000); | |
| const killTimeout = setTimeout(() => { | |
| if (serverProcess && !serverProcess.killed) serverProcess.kill('SIGKILL'); | |
| resolve(); | |
| }, 5000); | |
| serverProcess.on('exit', () => { | |
| clearTimeout(killTimeout); | |
| resolve(); | |
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 101773f. The 5s SIGKILL timer is now captured in a killTimeout handle and clearTimeout'd inside the child exit handler, so a clean SIGTERM shutdown lets the test finish immediately instead of waiting for the safety timer to elapse.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -333,6 +333,61 @@ export function resolvePort(): { port: number; source: string } { | |||||||||||||||
| return { port: 8080, source: "default" }; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Resolve HTTP bind host from command line args or environment variables. | ||||||||||||||||
| * Returns the host with "0.0.0.0" as the default (listen on all interfaces). | ||||||||||||||||
| * | ||||||||||||||||
| * Note: Only applicable when using --transport=http. Default "0.0.0.0" keeps | ||||||||||||||||
| * backward compatibility; production deployments should set "127.0.0.1" and | ||||||||||||||||
| * front DBHub with a reverse proxy or firewall. | ||||||||||||||||
| */ | ||||||||||||||||
| export function resolveHost(): { host: string; source: string } { | ||||||||||||||||
| // Detect a missing --host value directly in argv. parseCommandLineArgs() | ||||||||||||||||
| // collapses bare flags and explicit empty values into the same sentinel | ||||||||||||||||
| // string "true", which is indistinguishable from an explicit --host=true. | ||||||||||||||||
| // We inspect argv here so we can reject only the genuinely value-less cases: | ||||||||||||||||
| // --host (followed by nothing or another --flag) | ||||||||||||||||
| // --host= (empty after equals, alone or followed by another --flag) | ||||||||||||||||
| // An explicit --host=true passes through and fails later at listen(). | ||||||||||||||||
| const rawArgs = process.argv.slice(2); | ||||||||||||||||
| for (let i = 0; i < rawArgs.length; i++) { | ||||||||||||||||
| const token = rawArgs[i]; | ||||||||||||||||
|
|
||||||||||||||||
| if (token === "--host") { | ||||||||||||||||
| const next = rawArgs[i + 1]; | ||||||||||||||||
| if (!next || next.startsWith("--")) { | ||||||||||||||||
| console.error("ERROR: --host requires a value (e.g., --host=127.0.0.1)."); | ||||||||||||||||
| process.exit(1); | ||||||||||||||||
| } | ||||||||||||||||
| break; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (token === "--host=") { | ||||||||||||||||
| const next = rawArgs[i + 1]; | ||||||||||||||||
| if (!next || next.startsWith("--")) { | ||||||||||||||||
| console.error("ERROR: --host requires a value (e.g., --host=127.0.0.1)."); | ||||||||||||||||
| process.exit(1); | ||||||||||||||||
| } | ||||||||||||||||
| break; | ||||||||||||||||
|
||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+357
to
+373
|
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| const args = parseCommandLineArgs(); | ||||||||||||||||
|
|
||||||||||||||||
| // 1. Command line argument has highest priority | ||||||||||||||||
| if (args.host) { | ||||||||||||||||
| return { host: args.host, source: "command line argument" }; | ||||||||||||||||
|
||||||||||||||||
| return { host: args.host, source: "command line argument" }; | |
| const cliHost = args.host.trim(); | |
| if (!cliHost) { | |
| console.error("ERROR: --host requires a value (e.g., --host=127.0.0.1)."); | |
| process.exit(1); | |
| } | |
| return { host: cliHost, source: "command line argument" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 94be454. The CLI path now mirrors the env var path: args.host is trimmed, a whitespace-only value exits with the same --host requires a value error as the bare/empty forms, and surrounding whitespace on a valid value is stripped. Tests added for both --host=" " (rejected) and --host=" 127.0.0.1 " (trimmed to 127.0.0.1).
Copilot
AI
Apr 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the generic env var name HOST risks unexpected behavior changes because many environments/shells set HOST by default (often to the machine hostname). That can unintentionally change the bind address or even cause startup failure (e.g., unresolvable hostname), undermining the stated backward compatibility. Consider switching to a more specific variable name (e.g., DBHUB_HOST / HTTP_BIND_HOST) and optionally supporting HOST as a deprecated alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — fixed in d32c7ab. Renamed the env var to DBHUB_HOST so it cannot be shadowed by the generic HOST that csh/tcsh, some CI systems, and Docker base images set automatically (usually to the machine hostname). No alias is kept since this PR hasn't shipped yet. Updated env.ts, tests (incl. a new test asserting the generic HOST is ignored), the integration test, .env.example, and docs/config/command-line.mdx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration test uses a fixed port (3002). This can make the test flaky on CI/dev machines where that port is already in use. Prefer selecting an available ephemeral port at runtime (e.g., bind a temporary server to port 0 to discover a free port, or use a small helper like
get-port) and pass that value viaPORTwhen spawning DBHub.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declining for scope. The existing integration test in this repo (
src/__tests__/json-rpc-integration.test.ts) already uses a fixed port (testPort = 3001); I deliberately picked3002for the newhttp-bind-host.integration.test.tsso it follows the same convention and cannot collide with that one. Switching only this test to an ephemeral port would leave two integration tests with inconsistent patterns; the cleanup you describe is a sensible improvement, but it belongs in its own PR that migrates both tests (and any future ones) together rather than being bundled into a--hostbind feature.