diff --git a/packages/client/src/effect/service.ts b/packages/client/src/effect/service.ts index db2d15e46e6..24f01f16730 100644 --- a/packages/client/src/effect/service.ts +++ b/packages/client/src/effect/service.ts @@ -150,7 +150,7 @@ export const stop = Effect.fn("service.stop")(function* (options: StopOptions = PtyHandoff.prepare(options.file ?? fallback(), info, defaultEnsureTiming.requestTimeout), ) else yield* Effect.tryPromise(() => PtyHandoff.clear(options.file ?? fallback())) - if (info !== undefined) yield* terminate(info, options, defaultEnsureTiming) + if (info !== undefined) yield* terminate(info, options, ensureTiming(options)) }) function fallback() { @@ -291,9 +291,9 @@ const terminate = Effect.fnUntraced(function* (info: Info, options: { readonly f if (current === undefined || !same(current, info)) return yield* signal(info.pid, "SIGTERM") const done = yield* stopped(info.pid).pipe(Effect.retry(poll(timing)), Effect.option) + // A gracefully stopping server may remove its registration before it releases its port, so only + // the exact process we signalled can tell us when it is safe for a restart to continue. if (Option.isNone(done)) { - const latest = yield* read(options.file) - if (latest === undefined || !same(latest, info)) return yield* signal(info.pid, "SIGKILL") yield* stopped(info.pid).pipe(Effect.retry(poll(timing))) } diff --git a/packages/client/src/promise/service.ts b/packages/client/src/promise/service.ts index c7bbe36d15f..4d0716a737f 100644 --- a/packages/client/src/promise/service.ts +++ b/packages/client/src/promise/service.ts @@ -121,7 +121,7 @@ export async function stop(options: StopOptions = {}) { if (options.pty === "handoff" && info !== undefined) await PtyHandoff.prepare(options.file ?? fallback(), info, defaultEnsureTiming.requestTimeout) else await PtyHandoff.clear(options.file ?? fallback()) - if (info !== undefined) await terminate(info, options, defaultEnsureTiming) + if (info !== undefined) await terminate(info, options, ensureTiming(options)) } function fallback() { @@ -250,9 +250,9 @@ async function terminate(info: Info, options: { readonly file?: string }, timing const current = await read(options.file) if (current === undefined || !same(current, info)) return signal(info.pid, "SIGTERM") + // A gracefully stopping server may remove its registration before it releases its port, so only + // the exact process we signalled can tell us when it is safe for a restart to continue. if (!(await waitUntilStopped(info.pid, timing))) { - const latest = await read(options.file) - if (latest === undefined || !same(latest, info)) return signal(info.pid, "SIGKILL") if (!(await waitUntilStopped(info.pid, timing))) throw new Error(`Server process ${info.pid} is still running`) } diff --git a/packages/client/test/fixture/service.ts b/packages/client/test/fixture/service.ts index bd65e91741e..94a9d9ea8f8 100644 --- a/packages/client/test/fixture/service.ts +++ b/packages/client/test/fixture/service.ts @@ -1,4 +1,4 @@ -import { appendFile, rename, writeFile } from "node:fs/promises" +import { appendFile, rename, rm, writeFile } from "node:fs/promises" const [registration, mode, delay] = process.argv.slice(2) if (registration === undefined || mode === undefined) throw new Error("Missing service fixture arguments") @@ -98,6 +98,11 @@ await rename(registration + ".tmp", registration) async function shutdown(signal?: NodeJS.Signals) { if (signal !== undefined) await writeFile(registration + ".signal", signal) + // A lingering server removes its registration first and keeps its port while it winds down. + if (mode === "lingering") { + await rm(registration, { force: true }) + await Bun.sleep(Number(delay)) + } server.stop(true) process.exit() } diff --git a/packages/client/test/promise-service.test.ts b/packages/client/test/promise-service.test.ts index dca0aac6d27..7806a1f5fc4 100644 --- a/packages/client/test/promise-service.test.ts +++ b/packages/client/test/promise-service.test.ts @@ -4,6 +4,7 @@ import { serviceFixture } from "./fixture/service-fixture" import { accelerate } from "./fixture/service-timing" const ensure = accelerate(Service.ensure) +const stop = accelerate(Service.stop) test("discovers a registered service", async () => { await using fixture = await serviceFixture() @@ -158,3 +159,20 @@ test("signals the registered service process", async () => { expect(await Bun.file(registration + ".signal").text()).toBe("SIGTERM") expect(await Bun.file(registration).exists()).toBe(false) }) + +test("stop outlives a server that unregisters before releasing its port", async () => { + await using fixture = await serviceFixture() + const registration = fixture.registration + const existing = fixture.spawn("lingering", "5000") + await fixture.waitForFile() + const original = await Bun.file(registration).json() + + await stop({ file: registration }) + + expect(await Bun.file(registration + ".signal").text()).toBe("SIGTERM") + expect(() => process.kill(original.pid, 0)).toThrow() + const listener = Bun.serve({ port: Number(new URL(original.url).port), fetch: () => new Response() }) + await listener.stop(true) + expect(await Bun.file(registration).exists()).toBe(false) + await existing.exited +}, 15_000) diff --git a/packages/client/test/service.test.ts b/packages/client/test/service.test.ts index 83c754a50dd..b099d395b3c 100644 --- a/packages/client/test/service.test.ts +++ b/packages/client/test/service.test.ts @@ -7,6 +7,7 @@ import { serviceFixture } from "./fixture/service-fixture" import { accelerate } from "./fixture/service-timing" const ensure = accelerate(Service.ensure) +const stop = accelerate(Service.stop) test("a concurrent same-version start cannot invalidate a resolved endpoint", async () => { await using fixture = await serviceFixture() @@ -161,6 +162,22 @@ test("signals an unresponsive registered service process", async () => { expect(await Bun.file(registration).exists()).toBe(false) }) +test("stop outlives a server that unregisters before releasing its port", async () => { + await using fixture = await serviceFixture() + const registration = fixture.registration + const existing = fixture.spawn("lingering", "5000") + await fixture.waitForFile() + const original = await Bun.file(registration).json() + + await run(stop({ file: registration })) + + expect(await Bun.file(registration + ".signal").text()).toBe("SIGTERM") + expect(() => process.kill(original.pid, 0)).toThrow() + await expectPortAvailable(Number(new URL(original.url).port)) + expect(await Bun.file(registration).exists()).toBe(false) + await existing.exited +}, 15_000) + test("signals an incompatible service before starting its replacement", async () => { await using fixture = await serviceFixture() const registration = fixture.registration @@ -302,3 +319,8 @@ function run(effect: Effect.Effect) { async function status(url: string) { return fetch(new URL("/api/info", url), { signal: AbortSignal.timeout(1_000) }).then((response) => response.json()) } + +async function expectPortAvailable(port: number) { + const server = Bun.serve({ port, fetch: () => new Response() }) + await server.stop(true) +}