diff --git a/.changeset/preserve-cli-telemetry-exit.md b/.changeset/preserve-cli-telemetry-exit.md new file mode 100644 index 00000000000..2eaad21f271 --- /dev/null +++ b/.changeset/preserve-cli-telemetry-exit.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Prevent unreachable telemetry endpoints from blocking or failing completed CLI commands. diff --git a/packages/kilo-telemetry/src/__tests__/telemetry-shutdown.test.ts b/packages/kilo-telemetry/src/__tests__/telemetry-shutdown.test.ts new file mode 100644 index 00000000000..ed0782aa4ef --- /dev/null +++ b/packages/kilo-telemetry/src/__tests__/telemetry-shutdown.test.ts @@ -0,0 +1,52 @@ +// Isolated test file so `mock.module("posthog-node", ...)` registers before +// any import of `client.ts`. Living alongside `telemetry.test.ts` would let the +// top-level `Telemetry` import there resolve the real PostHog into the module +// cache before the mock is set, making the test rely on bun:test's cache +// invalidation timing rather than testing the shutdown path directly. +import { beforeEach, describe, test, expect, mock } from "bun:test" + +const timeout = "Timeout while shutting down PostHog. Some events may not have been sent." + +mock.module("posthog-node", () => ({ + PostHog: class { + async flush() { + flushCalls += 1 + throw new Error("flush should not be called") + } + async shutdown(timeoutMs?: number) { + shutdownCalls.push(timeoutMs) + throw timeout + } + optIn() {} + optOut() {} + capture() {} + alias() {} + }, +})) + +let flushCalls = 0 +const shutdownCalls: Array = [] + +describe("Telemetry.shutdown timeout (#9788)", () => { + beforeEach(() => { + flushCalls = 0 + shutdownCalls.length = 0 + }) + + test("passes timeoutMs through to PostHog.shutdown and skips unbounded explicit flush()", async () => { + // Reproduces the CLI exit hang reported in #9788: when the PostHog endpoint + // is unreachable (offline, firewall, DNS adblock resolving the host to + // 0.0.0.0), an explicit flush() call before shutdown retries 3x with 3s + // gaps plus 10s per attempt before throwing, blocking process.exit on + // short-lived commands like `kilo --help`. The fix drops the explicit + // flush() (PostHog.shutdown drains the queue itself) and threads a caller- + // supplied timeoutMs through to PostHog.shutdown. + const { Telemetry } = await import("../telemetry.js") + const { Client } = await import("../client.js") + Client.init() + await expect(Telemetry.shutdown(50)).rejects.toBe(timeout) + + expect(flushCalls).toBe(0) + expect(shutdownCalls).toEqual([50]) + }) +}) diff --git a/packages/kilo-telemetry/src/__tests__/telemetry.test.ts b/packages/kilo-telemetry/src/__tests__/telemetry.test.ts index ea5adaafd52..ff87bf69094 100644 --- a/packages/kilo-telemetry/src/__tests__/telemetry.test.ts +++ b/packages/kilo-telemetry/src/__tests__/telemetry.test.ts @@ -96,3 +96,4 @@ describe("Telemetry", () => { expect(typeof Telemetry.trackSuggestionAccepted).toBe("function") }) }) + diff --git a/packages/kilo-telemetry/src/client.ts b/packages/kilo-telemetry/src/client.ts index 0b9f3d885b5..24c902b5435 100644 --- a/packages/kilo-telemetry/src/client.ts +++ b/packages/kilo-telemetry/src/client.ts @@ -68,12 +68,18 @@ export namespace Client { }) } - export async function shutdown(): Promise { + export async function shutdown(timeoutMs?: number): Promise { if (client) { - // Flush any pending events before shutdown - await client.flush() - await client.shutdown() - client = null + try { + // PostHog's shutdown drains the queue internally and is bounded by + // shutdownTimeoutMs. Calling flush() first is redundant and unbounded: + // when the endpoint is unreachable (offline, firewall, DNS adblock), + // flush retries up to 3x with 3s delays plus 10s per attempt before + // throwing, blocking process exit before shutdown's outer cap kicks in. + await client.shutdown(timeoutMs) + } finally { + client = null + } } } } diff --git a/packages/kilo-telemetry/src/telemetry.ts b/packages/kilo-telemetry/src/telemetry.ts index bb1c037d02d..21ea9043c81 100644 --- a/packages/kilo-telemetry/src/telemetry.ts +++ b/packages/kilo-telemetry/src/telemetry.ts @@ -285,7 +285,7 @@ export namespace Telemetry { track(TelemetryEvent.FEEDBACK_SUBMITTED, props) } - export async function shutdown(): Promise { - await Client.shutdown() + export async function shutdown(timeoutMs?: number): Promise { + await Client.shutdown(timeoutMs) } } diff --git a/packages/opencode/src/kilocode/cli/setup.ts b/packages/opencode/src/kilocode/cli/setup.ts index d767453196d..df2adc47b73 100644 --- a/packages/opencode/src/kilocode/cli/setup.ts +++ b/packages/opencode/src/kilocode/cli/setup.ts @@ -1,4 +1,5 @@ import type { Argv } from "yargs" +import * as Log from "@opencode-ai/core/util/log" import { Global } from "@opencode-ai/core/global" import { InstallationBuildKind, InstallationVersion } from "@opencode-ai/core/installation/version" import { Telemetry } from "@kilocode/kilo-telemetry" @@ -17,6 +18,8 @@ import { DevSetupCommand, DevAliasCommand } from "@/kilocode/cli/dev-setup" import { RemoteCommand } from "@/cli/cmd/remote" import { ConfigCommand as ConfigCLICommand } from "@/cli/cmd/config" +const log = Log.create({ service: "kilocode.cli" }) + // All Kilo-specific CLI customization lives here so the shared upstream entrypoint // (src/index.ts) only needs a handful of thin call-sites behind kilocode_change markers. // This keeps index.ts close to upstream and reduces merge conflicts on every sync. @@ -73,8 +76,18 @@ export namespace KiloCli { export async function shutdown(): Promise { const code = typeof process.exitCode === "number" ? process.exitCode : undefined Telemetry.trackCliExit(code) - await SessionExport.shutdown() - await Telemetry.shutdown() - await InstanceRuntime.disposeAllInstances() // safety net (no-op if already disposed) + try { + await SessionExport.shutdown() + // Bound telemetry shutdown so an unreachable endpoint (offline, firewall, + // DNS adblock resolving the host to 0.0.0.0) cannot block process exit on + // short-lived commands like `kilo --help` / `kilo --version` (#9788). + try { + await Telemetry.shutdown(2000) + } catch (err) { + log.warn("telemetry shutdown failed", { err }) + } + } finally { + await InstanceRuntime.disposeAllInstances() // safety net (no-op if already disposed) + } } } diff --git a/packages/opencode/test/kilocode/cli-shutdown.test.ts b/packages/opencode/test/kilocode/cli-shutdown.test.ts new file mode 100644 index 00000000000..274e5c906bd --- /dev/null +++ b/packages/opencode/test/kilocode/cli-shutdown.test.ts @@ -0,0 +1,123 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test" + +const calls: string[] = [] +const timeouts: Array = [] +let err: unknown +let exit: string | number | null | undefined + +mock.module("@opencode-ai/core/global", () => ({ + Global: { Path: { data: "/tmp/kilo-test" } }, +})) + +mock.module("@opencode-ai/core/installation/version", () => ({ + InstallationBuildKind: "release", + InstallationVersion: "test", +})) + +mock.module("@kilocode/kilo-telemetry", () => ({ + Telemetry: { + async init() {}, + async updateIdentity() {}, + trackCliStart() {}, + trackCliExit(code?: number) { + calls.push(`track:${code ?? "undefined"}`) + }, + async shutdown(timeout?: number) { + calls.push("telemetry") + timeouts.push(timeout) + if (err) throw err + }, + }, +})) + +mock.module("@kilocode/kilo-gateway", () => ({ + ENV_FEATURE: "KILO_FEATURE", + ENV_VERSION: "KILO_VERSION", + async migrateLegacyKiloAuth() {}, +})) + +mock.module("@/config/config", () => ({ + Config: { Service: { use: () => ({ experimental: {} }) } }, +})) + +mock.module("@/auth", () => ({ + Auth: { Service: { use: () => undefined } }, +})) + +mock.module("@/project/instance-runtime", () => ({ + InstanceRuntime: { + async disposeAllInstances() { + calls.push("dispose") + }, + }, +})) + +mock.module("@/kilocode/session-export", () => ({ + SessionExport: { + async shutdown() { + calls.push("session") + }, + }, +})) + +mock.module("@/kilocode/help-command", () => ({ + createHelpCommand: () => ({ command: "help", handler() {} }), +})) + +for (const path of [ + "@/kilocode/cli/cmd/console", + "@/kilocode/cli/cmd/roll-call", + "@/kilocode/cli/cmd/profile", + "@/kilocode/cli/cmd/daemon", + "@/kilocode/cli/dev-setup", + "@/cli/cmd/remote", + "@/cli/cmd/config", +]) { + mock.module(path, () => ({ + KiloConsoleCommand: { command: "console", handler() {} }, + RollCallCommand: { command: "roll-call", handler() {} }, + ProfileCommand: { command: "profile", handler() {} }, + DaemonCommand: { command: "daemon", handler() {} }, + DevSetupCommand: { command: "dev-setup", handler() {} }, + DevAliasCommand: { command: "dev-alias", handler() {} }, + RemoteCommand: { command: "remote", handler() {} }, + ConfigCommand: { command: "config", handler() {} }, + })) +} + +describe("KiloCli.shutdown", () => { + beforeEach(() => { + calls.length = 0 + timeouts.length = 0 + err = undefined + exit = process.exitCode + process.exitCode = undefined + }) + + afterEach(() => { + process.exitCode = exit + }) + + test("keeps telemetry shutdown timeout best-effort and still disposes instances", async () => { + err = "Timeout while shutting down PostHog. Some events may not have been sent." + process.exitCode = 0 + const { KiloCli } = await import("../../src/kilocode/cli/setup") + + await expect(KiloCli.shutdown()).resolves.toBeUndefined() + + expect(timeouts).toEqual([2000]) + expect(calls).toEqual(["track:0", "session", "telemetry", "dispose"]) + expect(process.exitCode).toBe(0) + }) + + test("preserves failing command exit status", async () => { + process.exitCode = 1 + const { KiloCli } = await import("../../src/kilocode/cli/setup") + + await KiloCli.shutdown() + + expect(timeouts).toEqual([2000]) + expect(calls).toEqual(["track:1", "session", "telemetry", "dispose"]) + expect(process.exitCode).toBe(1) + }) +})