-
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 4 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}`, | ||||||||||||||||||||||||||||
| DBHUB_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,64 @@ 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.
resolveHost() treats any truthy DBHUB_HOST as a value without trimming/validating it. A whitespace-only value (e.g. DBHUB_HOST=" ") will be accepted and later cause a confusing bind failure. Consider trimming DBHUB_HOST and either (a) treating empty-after-trim as unset (fall back to default) or (b) exiting with a clear error similar to the --host validation.
| // 2. Environment variable (empty string is treated as unset) | |
| // Using DBHUB_HOST rather than generic HOST to avoid collisions — HOST is | |
| // set by default in csh/tcsh, some CI systems, and Docker base images | |
| // (often to the machine hostname), which would silently redirect binds. | |
| if (process.env.DBHUB_HOST) { | |
| return { host: process.env.DBHUB_HOST, source: "environment variable" }; | |
| // 2. Environment variable (empty string or whitespace-only value is treated as unset) | |
| // Using DBHUB_HOST rather than generic HOST to avoid collisions — HOST is | |
| // set by default in csh/tcsh, some CI systems, and Docker base images | |
| // (often to the machine hostname), which would silently redirect binds. | |
| const envHost = process.env.DBHUB_HOST?.trim(); | |
| if (envHost) { | |
| return { host: envHost, source: "environment variable" }; |
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. DBHUB_HOST is now trimmed before use, and an empty/whitespace-only value falls back to the default (0.0.0.0), matching the --host flag's validation. Tests added for both DBHUB_HOST=" " (falls back to default) and DBHUB_HOST=" 127.0.0.1 " (surrounding whitespace stripped).
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.
This PR’s metadata/title mentions a HOST env var, but the implementation and docs use DBHUB_HOST (and explicitly ignore HOST). Please align the PR description/title (and any release notes) with the actual env var name to avoid operator confusion.
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.
PR title and body updated to reference DBHUB_HOST consistently (summary, changes, and test plan).
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.