-
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 6 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.
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.