-
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 1 commit
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,36 @@ 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 } { | ||||||||||||||||
| const args = parseCommandLineArgs(); | ||||||||||||||||
|
|
||||||||||||||||
| // 1. Command line argument has highest priority. | ||||||||||||||||
| // parseCommandLineArgs turns bare --host (no value) into "true"; reject it. | ||||||||||||||||
| if (args.host) { | ||||||||||||||||
| if (args.host === "true") { | ||||||||||||||||
| console.error("ERROR: --host requires a value (e.g., --host=127.0.0.1)."); | ||||||||||||||||
| process.exit(1); | ||||||||||||||||
| } | ||||||||||||||||
| 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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { fileURLToPath } from "url"; | |
|
|
||
| import { ConnectorManager } from "./connectors/manager.js"; | ||
| import { ConnectorRegistry } from "./connectors/interface.js"; | ||
| import { resolveTransport, resolvePort, resolveSourceConfigs, isDemoMode } from "./config/env.js"; | ||
| import { resolveTransport, resolvePort, resolveHost, resolveSourceConfigs, isDemoMode } from "./config/env.js"; | ||
| import { registerTools } from "./tools/index.js"; | ||
| import { listSources, getSource } from "./api/sources.js"; | ||
| import { listRequests } from "./api/requests.js"; | ||
|
|
@@ -129,8 +129,9 @@ See documentation for more details on configuring database connections. | |
| // Resolve transport type (for MCP server) | ||
| const transportData = resolveTransport(); | ||
|
|
||
| // Resolve port for HTTP server (only needed for http transport) | ||
| // Resolve port and host for HTTP server (only needed for http transport) | ||
| const port = transportData.type === "http" ? resolvePort().port : null; | ||
| const host = transportData.type === "http" ? resolveHost().host : null; | ||
|
|
||
| // Print ASCII art banner with version and slogan | ||
| // Collect active modes | ||
|
|
@@ -258,17 +259,32 @@ See documentation for more details on configuring database connections. | |
| } | ||
|
|
||
| // Start the HTTP server | ||
| app.listen(port, '0.0.0.0', () => { | ||
| const httpServer = app.listen(port!, host!, () => { | ||
| const address = httpServer.address(); | ||
| const boundHost = typeof address === 'object' && address ? address.address : host!; | ||
| const boundPort = typeof address === 'object' && address ? address.port : port!; | ||
| const displayHost = boundHost.includes(':') ? `[${boundHost}]` : boundHost; | ||
|
||
| // Wildcard binds (0.0.0.0 / ::) are not routable; use localhost for user URLs. | ||
| const userHost = (boundHost === '0.0.0.0' || boundHost === '::') ? 'localhost' : displayHost; | ||
|
|
||
|
Comment on lines
+274
to
+277
|
||
| console.error(`HTTP server listening on ${displayHost}:${boundPort}`); | ||
|
|
||
| // In development mode, suggest using the Vite dev server for hot reloading | ||
| if (process.env.NODE_ENV === 'development') { | ||
| console.error('Development mode detected!'); | ||
| console.error(' Workbench dev server (with HMR): http://localhost:5173'); | ||
| console.error(' Backend API: http://localhost:8080'); | ||
| console.error(` Backend API: http://${userHost}:${boundPort}`); | ||
|
||
| console.error(''); | ||
| } else { | ||
| console.error(`Workbench at http://localhost:${port}/`); | ||
| console.error(`Workbench at http://${userHost}:${boundPort}/`); | ||
| } | ||
| console.error(`MCP server endpoint at http://localhost:${port}/mcp`); | ||
| console.error(`MCP server endpoint at http://${userHost}:${boundPort}/mcp`); | ||
| }); | ||
|
|
||
| httpServer.on('error', (err) => { | ||
| const displayHost = host!.includes(':') ? `[${host!}]` : host!; | ||
| console.error(`Failed to bind HTTP server to ${displayHost}:${port}: ${err.message}`); | ||
| process.exit(1); | ||
| }); | ||
| } else { | ||
| // STDIO transport: Pure MCP-over-stdio, no HTTP server | ||
|
|
||
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.