Skip to content

Commit 345dc4f

Browse files
melvincarvalhomelvincarvalho
authored
feat(cli): shift off a busy port + openable banner URL on jss start — closes #557 (#574)
* feat(cli): shift off a busy port + openable banner URL on jss start (#557) Port-back from jspod's lib/start.js (proven downstream). 1. Busy port → shift, not crash. `jss start` on an in-use port died with a raw EADDRINUSE — a first-run papercut when a stale instance is still running. It now probes upward Vite-style (next free port, up to 10 tries) and binds there. The shift goes to stderr so it surfaces even under --quiet (a port the operator didn't ask for is operationally significant), and runs BEFORE the IdP issuer / baseUrl are derived so they reflect the port actually bound. If the whole window is taken, it exits with a clear message instead of EADDRINUSE. 2. Openable banner URL. The banner rewrote only 0.0.0.0 → localhost; formatUrl now also maps :: / * → localhost and brackets IPv6 literals, so the printed URL is always clickable. findFreePort + formatUrl live in src/utils/port.js (not inline in the executable bin/jss.js) so they're unit-testable without running the CLI. Tests: - test/port.test.js (7): formatUrl wildcard/IPv6/protocol cases; findFreePort returns a free port, shifts off a busy one, and returns null when the window is full. - test/port-shift-cli.test.js (1): spawns `jss start` on an occupied port and asserts it logs the shift notice AND serves on the shifted port — the real end-to-end behaviour. No behaviour change when the requested port is free. Full suite 979/979. Closes #557. * review fix (#574): findFreePort only treats EADDRINUSE as busy, re-throws other errors Copilot: the probe treated ANY listen error as 'port busy', so EACCES (privileged port) or EADDRNOTAVAIL (invalid bind host) would probe the whole window and report a misleading 'no free port found' instead of the real cause. Now only EADDRINUSE resolves false (try next port); every other error rejects, propagating to the CLI's catch (bin/jss.js line 338) which surfaces the actual message. An improvement over jspod's original, which swallowed all errors. New test: findFreePort against a non-local host (192.0.2.1 → EADDRNOTAVAIL) rejects rather than returning null. 8/8 in file; full suite green.
1 parent 3d4f947 commit 345dc4f

4 files changed

Lines changed: 300 additions & 2 deletions

File tree

‎bin/jss.js‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { createInvite, listInvites, revokeInvite } from '../src/idp/invites.js';
1616
import { findByUsername, updatePassword, deleteAccount } from '../src/idp/accounts.js';
1717
import { setQuotaLimit, getQuotaInfo, reconcileQuota, formatBytes } from '../src/storage/quota.js';
1818
import { parseSize } from '../src/config.js';
19+
import { findFreePort, formatUrl } from '../src/utils/port.js';
1920
import crypto from 'crypto';
2021
import fs from 'fs-extra';
2122
import path from 'path';
@@ -176,10 +177,29 @@ program
176177
process.exit(0);
177178
}
178179

180+
// If the requested port is busy, shift up to the next free one
181+
// (Vite-style), rather than dying on a raw EADDRINUSE — a common
182+
// first-run papercut when a stale instance is still running (#557).
183+
// Must run BEFORE the issuer/baseUrl are derived so they reflect
184+
// the port we actually bind. The notice goes to stderr so it
185+
// surfaces even under --quiet (a port change the operator didn't
186+
// ask for is operationally significant).
187+
const requestedPort = config.port;
188+
const boundPort = await findFreePort(requestedPort, config.host);
189+
if (boundPort === null) {
190+
console.error(
191+
`Error: no free port found in ${requestedPort}–${requestedPort + 9} on ${config.host}.`
192+
);
193+
process.exit(1);
194+
}
195+
if (boundPort !== requestedPort) {
196+
console.error(` Port ${requestedPort} is in use — using ${boundPort} instead.`);
197+
config.port = boundPort;
198+
}
199+
179200
// Determine IdP issuer URL
180201
const protocol = config.ssl ? 'https' : 'http';
181-
const serverHost = config.host === '0.0.0.0' ? 'localhost' : config.host;
182-
const baseUrl = `${protocol}://${serverHost}:${config.port}`;
202+
const baseUrl = formatUrl(config.host, config.port, protocol);
183203
// Ensure issuer has trailing slash for CTH compatibility
184204
let idpIssuer = config.idpIssuer || baseUrl;
185205
if (idpIssuer && !idpIssuer.endsWith('/')) {

‎src/utils/port.js‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/**
2+
* Port + URL helpers for `jss start` (#557).
3+
*
4+
* Ported from jspod's lib/start.js, where both have been proven in
5+
* production. Kept in a standalone module (not inline in bin/jss.js) so
6+
* they're unit-testable without executing the CLI.
7+
*/
8+
9+
import { createServer } from 'net';
10+
11+
/**
12+
* Format a host + port into an URL a human can actually open, for the
13+
* startup banner. Wildcard bind addresses (0.0.0.0, ::, *) aren't
14+
* connectable, so show `localhost`; a bare IPv6 literal gets bracketed.
15+
*
16+
* @param {string} host - the bind host
17+
* @param {number} port - the bound port
18+
* @param {string} [protocol] - 'http' (default) or 'https'
19+
* @returns {string}
20+
*/
21+
export function formatUrl(host, port, protocol = 'http') {
22+
if (host === '0.0.0.0' || host === '::' || host === '*') {
23+
return `${protocol}://localhost:${port}`;
24+
}
25+
if (host.includes(':')) {
26+
// IPv6 literal — must be bracketed in a URL authority.
27+
return `${protocol}://[${host}]:${port}`;
28+
}
29+
return `${protocol}://${host}:${port}`;
30+
}
31+
32+
/**
33+
* Find a free port at or above `startPort` on `host`. Mirrors Vite's
34+
* behaviour: probe one port at a time, up to `maxTries`, returning the
35+
* first bindable one — or `null` if every port in the range is taken.
36+
*
37+
* Only EADDRINUSE counts as "busy" (try the next port). Any other
38+
* bind failure — EACCES on a privileged port, EADDRNOTAVAIL for an
39+
* invalid host — is a real error and is re-thrown, so the caller
40+
* surfaces the actual cause instead of a misleading "no free port".
41+
*
42+
* Uses a throwaway net server to test bindability without committing the
43+
* real server. (There is an inherent TOCTOU window between this probe
44+
* and the real listen; the caller falls back to its normal listen-error
45+
* path if the chosen port is grabbed in between.)
46+
*
47+
* @param {number} startPort
48+
* @param {string} host
49+
* @param {number} [maxTries]
50+
* @returns {Promise<number|null>}
51+
*/
52+
export async function findFreePort(startPort, host, maxTries = 10) {
53+
for (let p = startPort; p < startPort + maxTries; p++) {
54+
const free = await new Promise((resolve, reject) => {
55+
const srv = createServer();
56+
srv.once('error', (err) => {
57+
if (err.code === 'EADDRINUSE') resolve(false); // busy — try the next port
58+
else reject(err); // real failure — surface it
59+
});
60+
srv.once('listening', () => srv.close(() => resolve(true)));
61+
srv.listen(p, host);
62+
});
63+
if (free) return p;
64+
}
65+
return null;
66+
}

‎test/port-shift-cli.test.js‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
/**
2+
* CLI wiring for the busy-port shift (#557).
3+
*
4+
* test/port.test.js covers findFreePort/formatUrl directly; this spawns
5+
* `bin/jss.js start` on an occupied port and asserts the real user-facing
6+
* behaviour: instead of dying on EADDRINUSE, jss shifts to the next free
7+
* port (Vite-style), prints a notice to stderr (so it surfaces even under
8+
* --quiet), and actually serves there.
9+
*/
10+
11+
import { describe, it, afterEach } from 'node:test';
12+
import assert from 'node:assert';
13+
import { spawn } from 'node:child_process';
14+
import { createServer as createNetServer } from 'node:net';
15+
import { fileURLToPath } from 'node:url';
16+
import path from 'node:path';
17+
import fs from 'fs-extra';
18+
19+
const __dirname = path.dirname(fileURLToPath(import.meta.url));
20+
const BIN = path.join(__dirname, '..', 'bin', 'jss.js');
21+
const TEST_DATA_DIR = './test-data-port-shift-cli';
22+
const HOST = '127.0.0.1';
23+
24+
let child;
25+
let blocker;
26+
27+
function freePort() {
28+
return new Promise((resolve, reject) => {
29+
const srv = createNetServer();
30+
srv.once('error', reject);
31+
srv.listen(0, HOST, () => {
32+
const { port } = srv.address();
33+
srv.close(() => resolve(port));
34+
});
35+
});
36+
}
37+
38+
function occupy(port) {
39+
return new Promise((resolve, reject) => {
40+
const srv = createNetServer();
41+
srv.once('error', reject);
42+
srv.listen(port, HOST, () => resolve(srv));
43+
});
44+
}
45+
46+
async function stopCli() {
47+
if (!child) return;
48+
const c = child;
49+
child = null;
50+
if (c.exitCode !== null || c.signalCode !== null) return;
51+
const gone = new Promise((r) => c.once('exit', r));
52+
c.kill('SIGTERM');
53+
await Promise.race([gone, new Promise((r) => setTimeout(r, 3000))]);
54+
if (c.exitCode === null && c.signalCode === null) c.kill('SIGKILL');
55+
await Promise.race([gone, new Promise((r) => setTimeout(r, 2000))]);
56+
}
57+
58+
describe('bin/jss.js — busy-port shift (#557)', () => {
59+
afterEach(async () => {
60+
await stopCli();
61+
if (blocker) {
62+
await new Promise((r) => blocker.close(r));
63+
blocker = null;
64+
}
65+
await fs.remove(TEST_DATA_DIR);
66+
});
67+
68+
it('shifts to the next free port and serves there when the requested port is busy', async () => {
69+
await fs.emptyDir(TEST_DATA_DIR);
70+
const busy = await freePort();
71+
blocker = await occupy(busy); // hold `busy` so jss can't bind it
72+
73+
let stderr = '';
74+
child = spawn(process.execPath, [
75+
BIN, 'start',
76+
'--port', String(busy),
77+
'--host', HOST,
78+
'--root', TEST_DATA_DIR,
79+
'--quiet', // banner suppressed; the shift notice still goes to stderr
80+
], { env: { ...process.env }, stdio: ['ignore', 'pipe', 'pipe'] });
81+
child.stderr.on('data', (d) => { stderr += d; });
82+
83+
const deadline = Date.now() + 15_000;
84+
let exited = false;
85+
child.once('exit', () => { exited = true; });
86+
87+
// 1. The shift notice (which carries the chosen port) must appear.
88+
let shifted = null;
89+
while (Date.now() < deadline) {
90+
const m = stderr.match(/using (\d+) instead/);
91+
if (m) { shifted = Number(m[1]); break; }
92+
if (exited) throw new Error(`jss exited before shifting. stderr: ${stderr}`);
93+
await new Promise((r) => setTimeout(r, 100));
94+
}
95+
assert.ok(shifted, `expected a port-shift notice on stderr; got: ${stderr || '(empty)'}`);
96+
assert.ok(stderr.includes(`Port ${busy} is in use`), 'notice should name the busy port');
97+
assert.notStrictEqual(shifted, busy);
98+
99+
// 2. The server must actually serve on the shifted port.
100+
const url = `http://${HOST}:${shifted}`;
101+
let ready = false;
102+
while (Date.now() < deadline) {
103+
if (exited) throw new Error(`jss exited before serving. stderr: ${stderr}`);
104+
try {
105+
await fetch(url, { signal: AbortSignal.timeout(1000) });
106+
ready = true;
107+
break;
108+
} catch {
109+
await new Promise((r) => setTimeout(r, 200));
110+
}
111+
}
112+
assert.ok(ready, `jss should serve on the shifted port ${shifted}`);
113+
});
114+
});

‎test/port.test.js‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* Port + URL helpers for `jss start` (#557): findFreePort shifts off a
3+
* busy port (Vite-style); formatUrl turns a bind host into an openable
4+
* banner URL.
5+
*/
6+
7+
import { describe, it } from 'node:test';
8+
import assert from 'node:assert';
9+
import { createServer } from 'net';
10+
import { findFreePort, formatUrl } from '../src/utils/port.js';
11+
12+
const HOST = '127.0.0.1';
13+
14+
function listen(port, host) {
15+
return new Promise((resolve, reject) => {
16+
const srv = createServer();
17+
srv.once('error', reject);
18+
srv.listen(port, host, () => resolve(srv));
19+
});
20+
}
21+
function close(srv) {
22+
return new Promise((resolve) => srv.close(resolve));
23+
}
24+
function freePort(host) {
25+
return new Promise((resolve, reject) => {
26+
const srv = createServer();
27+
srv.once('error', reject);
28+
srv.listen(0, host, () => {
29+
const { port } = srv.address();
30+
srv.close(() => resolve(port));
31+
});
32+
});
33+
}
34+
35+
describe('formatUrl (#557)', () => {
36+
it('rewrites wildcard bind addresses to localhost', () => {
37+
assert.strictEqual(formatUrl('0.0.0.0', 4443), 'http://localhost:4443');
38+
assert.strictEqual(formatUrl('::', 4443), 'http://localhost:4443');
39+
assert.strictEqual(formatUrl('*', 4443), 'http://localhost:4443');
40+
});
41+
42+
it('passes a normal host through unchanged', () => {
43+
assert.strictEqual(formatUrl('example.com', 8080), 'http://example.com:8080');
44+
assert.strictEqual(formatUrl('127.0.0.1', 3000), 'http://127.0.0.1:3000');
45+
});
46+
47+
it('brackets an IPv6 literal', () => {
48+
assert.strictEqual(formatUrl('::1', 4443), 'http://[::1]:4443');
49+
assert.strictEqual(formatUrl('fe80::1', 4443), 'http://[fe80::1]:4443');
50+
});
51+
52+
it('honours the protocol argument', () => {
53+
assert.strictEqual(formatUrl('example.com', 443, 'https'), 'https://example.com:443');
54+
assert.strictEqual(formatUrl('0.0.0.0', 4443, 'https'), 'https://localhost:4443');
55+
});
56+
});
57+
58+
describe('findFreePort (#557)', () => {
59+
it('returns the requested port when it is free', async () => {
60+
const p = await freePort(HOST);
61+
assert.strictEqual(await findFreePort(p, HOST), p);
62+
});
63+
64+
it('shifts to the next free port when the requested one is busy', async () => {
65+
const p = await freePort(HOST);
66+
const blocker = await listen(p, HOST);
67+
try {
68+
const got = await findFreePort(p, HOST);
69+
assert.ok(got > p, `expected a port above ${p}, got ${got}`);
70+
assert.ok(got < p + 10, 'should stay within the probe window');
71+
} finally {
72+
await close(blocker);
73+
}
74+
});
75+
76+
it('returns null when every port in the window is taken', async () => {
77+
const p = await freePort(HOST);
78+
const a = await listen(p, HOST);
79+
const b = await listen(p + 1, HOST);
80+
try {
81+
// window of 2 — both taken → no free port
82+
assert.strictEqual(await findFreePort(p, HOST, 2), null);
83+
} finally {
84+
await close(a);
85+
await close(b);
86+
}
87+
});
88+
89+
it('re-throws a non-EADDRINUSE bind error instead of reporting "no free port"', async () => {
90+
// 192.0.2.0/24 (TEST-NET-1) isn't a local interface → EADDRNOTAVAIL,
91+
// which is a real failure, not a busy port. Must propagate so the CLI
92+
// surfaces the actual cause rather than "no free port found".
93+
await assert.rejects(
94+
() => findFreePort(40000, '192.0.2.1', 1),
95+
(err) => err && err.code !== undefined && err.code !== 'EADDRINUSE',
96+
);
97+
});
98+
});

0 commit comments

Comments
 (0)