diff --git a/.changeset/dev-server-review-followup.md b/.changeset/dev-server-review-followup.md new file mode 100644 index 00000000..274935c1 --- /dev/null +++ b/.changeset/dev-server-review-followup.md @@ -0,0 +1,5 @@ +--- +"@aws-blocks/core": patch +--- + +Tighten dev server singleton and retry handling for port contention follow-ups. diff --git a/packages/core/src/scripts/dev-server-reclaim.test.ts b/packages/core/src/scripts/dev-server-reclaim.test.ts index 7a28312d..8bab8bff 100644 --- a/packages/core/src/scripts/dev-server-reclaim.test.ts +++ b/packages/core/src/scripts/dev-server-reclaim.test.ts @@ -8,6 +8,7 @@ import { reclaimMessage, evaluatePortBindRetry, createBindRetryController, + scheduleBindRetry, DEFAULT_PORT_BIND_RETRY_POLICY, evaluateSingleton, parsePidRecord, @@ -188,8 +189,8 @@ describe('evaluatePortBindRetry — front-door bind retry budget', () => { }); // ── evaluateSingleton — singleton guard decision ──────────────────────────── -// Prevents two fighting supervisors while never blocking a `tsx watch` reload of -// the SAME supervisor (same parent pid). +// Prevents two fighting supervisors while allowing a `tsx watch` reload after +// the previous child has exited. describe('evaluateSingleton — one supervisor per port, hot-reload safe', () => { const rec = (over: Partial = {}): DevServerPidRecord => ({ pid: 1000, @@ -204,12 +205,19 @@ describe('evaluateSingleton — one supervisor per port, hot-reload safe', () => assert.deepStrictEqual(evaluateSingleton(null, { pid: 1, ppid: 2 }, true, alive), { action: 'proceed' }); }); - it('proceeds on a tsx-watch relaunch of our own supervisor (same parent pid)', () => { - // New child, DIFFERENT own pid, but SAME watcher parent → not a competitor. - const d = evaluateSingleton(rec({ pid: 1000, ppid: 500 }), { pid: 1001, ppid: 500 }, true, alive); + it('proceeds on a tsx-watch relaunch after the previous child has exited', () => { + // New child, DIFFERENT own pid, SAME watcher parent, and old child is dead. + const d = evaluateSingleton(rec({ pid: 1000, ppid: 500 }), { pid: 1001, ppid: 500 }, true, dead); assert.deepStrictEqual(d, { action: 'proceed' }); }); + it('exits when a same-parent live owner is actually holding the port', () => { + const d = evaluateSingleton(rec({ pid: 1000, ppid: 500, port: 3000 }), { pid: 1001, ppid: 500 }, true, alive); + if (d.action !== 'exit') assert.fail(`expected exit, got ${d.action}`); + assert.match(d.reason, /already running on :3000/); + assert.match(d.reason, /pid 1000/); + }); + it('exits when a different, live supervisor is actually holding the port', () => { const d = evaluateSingleton(rec({ pid: 1000, ppid: 500, port: 3000 }), { pid: 2000, ppid: 900 }, true, alive); if (d.action !== 'exit') assert.fail(`expected exit, got ${d.action}`); @@ -428,3 +436,29 @@ describe('createBindRetryController — bounded EADDRINUSE reclaim-and-rebind', assert.strictEqual(c.scheduled.length, 1); }); }); + +describe('scheduleBindRetry — front-door retry timer', () => { + it('does not unref the retry timer', () => { + let callback: (() => void) | undefined; + let delay: number | undefined; + let unrefCalls = 0; + const timer = { + unref: () => { unrefCalls += 1; return timer; }, + } as NodeJS.Timeout; + + const returned = scheduleBindRetry( + () => {}, + 250, + (fn, delayMs) => { + callback = fn; + delay = delayMs; + return timer; + }, + ); + + assert.strictEqual(returned, timer); + assert.strictEqual(delay, 250); + assert.ok(callback); + assert.strictEqual(unrefCalls, 0, 'front-door bind retry must keep the event loop alive'); + }); +}); diff --git a/packages/core/src/scripts/dev-server.ts b/packages/core/src/scripts/dev-server.ts index 80bebd7c..6022e22c 100644 --- a/packages/core/src/scripts/dev-server.ts +++ b/packages/core/src/scripts/dev-server.ts @@ -396,6 +396,15 @@ export interface BindRetryDeps { warn: (msg: string) => void; } +/** Schedule a front-door rebind retry; keep it refed so the retry deterministically fires. */ +export function scheduleBindRetry( + fn: () => void, + delayMs: number, + setTimer: (fn: () => void, delayMs: number) => NodeJS.Timeout = setTimeout, +): NodeJS.Timeout { + return setTimer(fn, delayMs); +} + /** * Build the `:3000` front-door EADDRINUSE bind-retry handler. Extracted from the * `server.on('error')` closure so the retry *wiring* — the 1-based attempt @@ -490,10 +499,11 @@ export type SingletonDecision = { action: 'proceed' } | { action: 'exit'; reason * * - **No / corrupt pidfile** → proceed (first start; startup reclaim covers any orphan socket). * - **Same pid** → proceed (defensive; the record is our own). - * - **Same parent (`ppid`)** → proceed. `tsx watch` is the stable parent across - * reloads, so a matching parent means the watcher is relaunching OUR OWN script - * — not a competitor. A second `npm run dev` runs under a *different* watcher, - * so it never matches here. This carve-out is what preserves hot reload. + * - **Same parent (`ppid`) and dead recorded child** → proceed. `tsx watch` is + * the stable parent across reloads, and it relaunches after the old child exits. + * A live recorded child with the same parent can also happen when two + * `npm run dev` jobs share a shell, so that still goes through the live-owner + * check below. * - **Different, still-live owner actually holding the port** → exit cleanly with * a clear message (do not spawn a competing supervisor). * - **Otherwise** (recorded owner is dead → stale pidfile, or the port is free) @@ -507,8 +517,9 @@ export function evaluateSingleton( ): SingletonDecision { if (!existing) return { action: 'proceed' }; if (existing.pid === self.pid) return { action: 'proceed' }; - if (existing.ppid === self.ppid) return { action: 'proceed' }; // tsx-watch relaunch of our own supervisor - const ownerAlive = isAlive(existing.pid) || (existing.ppid > 1 && isAlive(existing.ppid)); + const existingPidAlive = isAlive(existing.pid); + if (existing.ppid === self.ppid && !existingPidAlive) return { action: 'proceed' }; // tsx-watch relaunch after old child exit + const ownerAlive = existingPidAlive || (existing.ppid > 1 && isAlive(existing.ppid)); if (ownerAlive && portInUse) { return { action: 'exit', reason: `dev server already running on :${existing.port} (pid ${existing.pid})` }; } @@ -984,7 +995,7 @@ export async function startDevServer(options: DevServerOptions) { const onEaddrinuse = createBindRetryController(port, { reclaim: (p) => reclaimPort(p), relisten: () => server.listen(port, onListening), - scheduleRetry: (fn, delayMs) => { setTimeout(fn, delayMs).unref?.(); }, + scheduleRetry: (fn, delayMs) => { scheduleBindRetry(fn, delayMs); }, onExhausted: () => process.exit(1), warn: (msg) => console.error(msg), });