diff --git a/packages/client/src/effect/service.ts b/packages/client/src/effect/service.ts index db2d15e46e63..05caaea8fdf3 100644 --- a/packages/client/src/effect/service.ts +++ b/packages/client/src/effect/service.ts @@ -84,7 +84,9 @@ export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOpti info, count: timeouts !== undefined && same(timeouts.info, info) ? timeouts.count + 1 : 1, } - if (timeouts.count >= 3) { + // Require sustained unresponsiveness before evicting: transient Windows probe stalls must + // reconnect, never kill a healthy service and abort its sessions. + if (timeouts.count >= 5) { yield* announce("missing") yield* Effect.logWarning("Background service is unresponsive; recovery cannot preserve persistent terminals") yield* Effect.tryPromise(() => PtyHandoff.clear(options.file ?? fallback())) @@ -121,8 +123,10 @@ export const ensure = Effect.fn("service.ensure")(function* (options: EnsureOpti } finished.forEach((item) => contenders.delete(item)) if (failure !== undefined && contenders.size === 0) return yield* Effect.fail(failure) - // Keep one candidate plus one lock probe so a pre-lock stall cannot block recovery. - if (contenders.size < 2 && Date.now() - lastSpawn >= spawnDelay) { + // Keep one candidate plus one lock probe so a pre-lock stall cannot block recovery. While health + // probes against the registered service keep timing out, wait for eviction instead: a contender + // that registers first would take over and leak the hung process terminate() no longer matches. + if (timeouts === undefined && contenders.size < 2 && Date.now() - lastSpawn >= spawnDelay) { yield* announce("missing") contenders.add(yield* spawnContender) lastSpawn = Date.now() diff --git a/packages/client/src/promise/service.ts b/packages/client/src/promise/service.ts index c7bbe36d15f5..bdadf262d240 100644 --- a/packages/client/src/promise/service.ts +++ b/packages/client/src/promise/service.ts @@ -63,7 +63,9 @@ export async function ensure(options: EnsureOptions = {}): Promise { info: registration.info, count: timeouts !== undefined && same(timeouts.info, registration.info) ? timeouts.count + 1 : 1, } - if (timeouts.count >= 3) { + // Require sustained unresponsiveness before evicting: transient Windows probe stalls must + // reconnect, never kill a healthy service and abort its sessions. + if (timeouts.count >= 5) { announce("missing") console.warn("Background service is unresponsive; recovery cannot preserve persistent terminals") await PtyHandoff.clear(options.file ?? fallback()) @@ -101,8 +103,10 @@ export async function ensure(options: EnsureOptions = {}): Promise { } finished.forEach((item) => contenders.delete(item)) if (failure !== undefined && contenders.size === 0) throw failure - // Keep one candidate plus one lock probe so a pre-lock stall cannot block recovery. - if (contenders.size < 2 && Date.now() - lastSpawn >= spawnDelay) { + // Keep one candidate plus one lock probe so a pre-lock stall cannot block recovery. While health + // probes against the registered service keep timing out, wait for eviction instead: a contender + // that registers first would take over and leak the hung process terminate() no longer matches. + if (timeouts === undefined && contenders.size < 2 && Date.now() - lastSpawn >= spawnDelay) { announce("missing") contenders.add(await spawnContender()) lastSpawn = Date.now() diff --git a/packages/client/src/solid/connection.ts b/packages/client/src/solid/connection.ts index 01e6b75b28c1..70fd5023be11 100644 --- a/packages/client/src/solid/connection.ts +++ b/packages/client/src/solid/connection.ts @@ -20,7 +20,8 @@ export type ClientConnectionOptions = { readonly pageLifecycle?: boolean /** * Abort and reconnect a stream that receives no bytes for this long. The server writes a keepalive - * comment every 15 seconds, so a quiet but healthy stream never trips this. + * comment every 15 seconds, so a quiet but healthy stream never trips this. The default allows six + * missed keepalives so transient Windows stalls never look like a dead stream. */ readonly idleTimeout?: number readonly log?: { @@ -32,7 +33,7 @@ export type ClientConnectionOptions = { const connectTimeout = 2_000 const reconnectDelay = 1_000 const connectionHistoryLimit = 50 -export const defaultIdleTimeout = 45_000 +export const defaultIdleTimeout = 90_000 // Longer than one server keepalive interval: a stream that is silent this long when the page // returns to the foreground is probably half-open after the device slept. export const foregroundIdleThreshold = 20_000 diff --git a/packages/client/test/fixture/service.ts b/packages/client/test/fixture/service.ts index bd65e91741ea..09500c8f55c8 100644 --- a/packages/client/test/fixture/service.ts +++ b/packages/client/test/fixture/service.ts @@ -59,6 +59,10 @@ const server = Bun.serve({ await appendFile(registration + ".requests", process.pid + "\n") return new Promise(() => {}) } + if (mode === "flaky") { + await appendFile(registration + ".requests", process.pid + "\n") + if (requests <= Number(delay ?? "3")) return new Promise(() => {}) + } if (mode === "modern" && requests === 1) { await writeFile(registration + ".first-request", "") while (!(await Bun.file(registration + ".release").exists())) await Bun.sleep(5) diff --git a/packages/client/test/promise-service.test.ts b/packages/client/test/promise-service.test.ts index dca0aac6d27b..06886175ce4e 100644 --- a/packages/client/test/promise-service.test.ts +++ b/packages/client/test/promise-service.test.ts @@ -141,12 +141,33 @@ test("evicts an unresponsive registered service before starting its replacement" const replacement = await Bun.file(registration).json() fixture.track(replacement.pid) - expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(3) + expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(5) expect(await existing.exited).toBe(0) expect(replacement.pid).not.toBe(original.pid) expect(endpoint.url).toBe(replacement.url) }) +test("keeps a registered service that recovers after transient probe stalls", async () => { + await using fixture = await serviceFixture() + const registration = fixture.registration + const existing = fixture.spawn("flaky", "3") + await fixture.waitForFile() + const original = await Bun.file(registration).json() + + const endpoint = await ensure({ + file: registration, + version: "test", + command: fixture.command("delayed", "10"), + }) + const current = await Bun.file(registration).json() + fixture.track(current.pid) + + expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(4) + expect(existing.exitCode).toBe(null) + expect(current.pid).toBe(original.pid) + expect(endpoint.url).toBe(original.url) +}) + test("signals the registered service process", async () => { await using fixture = await serviceFixture() const registration = fixture.registration diff --git a/packages/client/test/service.test.ts b/packages/client/test/service.test.ts index 83c754a50ddb..7bf0af6d6e33 100644 --- a/packages/client/test/service.test.ts +++ b/packages/client/test/service.test.ts @@ -142,13 +142,37 @@ test("evicts an unresponsive registered service before starting its replacement" const replacement = await Bun.file(registration).json() fixture.track(replacement.pid) - expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(3) + expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(5) expect(await existing.exited).toBe(0) expect(replacement.pid).not.toBe(original.pid) expect(endpoint.url).toBe(replacement.url) expect(await status(endpoint.url)).toMatchObject({ version: "test", pid: replacement.pid }) }) +test("keeps a registered service that recovers after transient probe stalls", async () => { + await using fixture = await serviceFixture() + const registration = fixture.registration + const existing = fixture.spawn("flaky", "3") + await fixture.waitForFile() + const original = await Bun.file(registration).json() + + const endpoint = await run( + ensure({ + file: registration, + version: "test", + command: fixture.command("delayed", "10"), + }), + ) + const current = await Bun.file(registration).json() + fixture.track(current.pid) + + expect((await Bun.file(registration + ".requests").text()).trim().split("\n")).toHaveLength(4) + expect(existing.exitCode).toBe(null) + expect(current.pid).toBe(original.pid) + expect(endpoint.url).toBe(original.url) + expect(await status(endpoint.url)).toMatchObject({ version: "test", pid: original.pid }) +}) + test("signals an unresponsive registered service process", async () => { await using fixture = await serviceFixture() const registration = fixture.registration diff --git a/packages/client/test/solid-connection.test.ts b/packages/client/test/solid-connection.test.ts index 60219ed73ea3..3ce23f6f4cd3 100644 --- a/packages/client/test/solid-connection.test.ts +++ b/packages/client/test/solid-connection.test.ts @@ -1,6 +1,6 @@ import { expect, test } from "bun:test" import { createRoot } from "solid-js" -import { createClientConnection } from "../src/solid" +import { createClientConnection, defaultIdleTimeout } from "../src/solid" import { OpenCode, type OpenCodeEvent } from "../src/promise" const connected = { id: "evt_connected", created: 1, type: "server.connected", data: {} } @@ -179,3 +179,9 @@ test("a reconnect does not ask the volatile event stream to replay", async () => ctx.dispose() } }) + +test("the default idle timeout spans several server keepalives", () => { + // The server writes a keepalive comment every 15 seconds. The default must cover several missed + // keepalives so a transient Windows stall triggers a reconnect, never a service replacement. + expect(defaultIdleTimeout).toBeGreaterThanOrEqual(6 * 15_000) +})