fix(cli): bound Telemetry.shutdown so unreachable PostHog endpoint cannot block CLI exit - #9807
Conversation
…nnot block CLI exit When the PostHog endpoint is unreachable (offline, firewall, DNS adblock resolving us.i.posthog.com to 0.0.0.0), `kilo --help` and other short- lived commands hang in the finally-block telemetry shutdown for tens of seconds before the process exits. Two issues: 1. `Client.shutdown()` called `await client.flush()` before `await client.shutdown()`. The explicit flush() is unbounded: posthog-node 4.4.0 retries each request up to 3x with 3s delays plus 10s per attempt before throwing PostHogFetchNetworkError, blocking process exit before shutdown's outer cap kicks in. 2. `Telemetry.shutdown()` took no timeout, so the CLI bootstrap had no way to bound the wait at the call site. PostHog's `shutdown(shutdownTimeoutMs = 30000)` defaults to 30 seconds. Fix: - Drop the explicit `flush()` call. PostHog's `shutdown(timeoutMs)` drains the queue internally and is bounded by `shutdownTimeoutMs`. - Add an optional `timeoutMs` parameter to both `Client.shutdown` and `Telemetry.shutdown`, threaded through to PostHog. - Pass `2000` from the CLI bootstrap so short-lived commands exit quickly when telemetry is unreachable, while keeping a generous budget for the working-endpoint case. Test: `Telemetry.shutdown timeout (Kilo-Org#9788)` mocks `posthog-node` so the fake `flush()` would hang for 60s if called. The test asserts (a) `flush()` is never invoked and (b) the supplied `timeoutMs` (50ms) is threaded through to PostHog's `shutdown(timeoutMs)`. Total elapsed time stays well under 1s. Closes Kilo-Org#9788
| import { describe, test, expect, beforeEach, mock } from "bun:test" | ||
| import { Identity } from "../identity.js" | ||
| import { TelemetryEvent } from "../events.js" | ||
| import { Telemetry } from "../telemetry.js" |
There was a problem hiding this comment.
WARNING: Static import prevents the PostHog mock from replacing the client under test
This file imports Telemetry before mock.module("posthog-node", ...) runs, which also loads ../client.js and binds it to the real PostHog import. The later dynamic imports return the cached modules, so Client.init() will not use this fake class and the test can either hit the real client or fail to assert the intended shutdown call. Move the telemetry/client imports into the test after the mock is registered (and remove this top-level import) so the mocked posthog-node is the dependency used by Client.init().
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Incremental diff ( Previously-reported WARNING on Carried-Forward Observation (unchanged, not in diff)
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 9394100)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9394100)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit 2723c08)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by deepseek-v4-pro-20260423 · 220,913 tokens Review guidance: REVIEW.md from base branch |
The new shutdown timeout test in telemetry.test.ts shares a file with
tests that statically import `Telemetry` (and thus `client.ts` and
`posthog-node`). Bun's `mock.module()` invalidates the dependency
cache for subsequent dynamic imports so the test passes today, but the
behavior depends on test ordering and mock-cache-invalidation timing
rather than test isolation. Extract the shutdown test into its own file
where `mock.module("posthog-node", ...)` runs before any client.ts
import, so the mock is the source of truth from module load.
|
Addressed the test-isolation flag — moved the shutdown timeout test into its own file ( |
…ing logic(Kilo-Org#9804) (Kilo-Org#9807)" This reverts commit cf6ad4c.
…ing logic(Kilo-Org#9804) (Kilo-Org#9807)" This reverts commit 863dbf3.
|
Friendly check-in. Bot review cleared on May 4; happy to rebase or split if anything would help it land. |
…ing logic(Kilo-Org#9804) (Kilo-Org#9807)" This reverts commit 4b1091d.
…-telemetry-shutdown-on-exit-9788 fix(cli): bound Telemetry.shutdown so unreachable PostHog endpoint cannot block CLI exit
Closes #9788.
kilo --help(and other short-lived commands) hangs in the finally-block telemetry shutdown for tens of seconds whenus.i.posthog.comis unreachable: offline, behind a firewall, or DNS-blackholed. Reporter'stimeout 6s kilo --helpexits non-zero on the broken path; togglingKILO_TELEMETRY_LEVEL=nonemakes it exit fast. Reporter's hypothesis (telemetry flush blocks process exit) checks out against the source.Root cause
Two issues in
packages/kilo-telemetry/src/client.ts:71-78:Unbounded explicit
flush()beforeshutdown(). posthog-node 4.4.0 retries each request up to 3x with 3s delays plus 10s per attempt before throwingPostHogFetchNetworkError(seenode_modules/posthog-node/lib/index.esm.js:1338-1366and the_retryOptionsdefaults atretryCount: 3, retryDelay: 3000plusrequestTimeout: 10000). When the endpoint is unreachable,await client.flush()blocks for up to ~36s before throwing, and that hang happens beforeshutdown()'s outer cap is ever reached.No timeout parameter at the Telemetry layer.
Telemetry.shutdown()andClient.shutdown()accepted no arguments, so the CLI bootstrap had no way to bound the wait at the call site. PostHog's underlyingshutdown(shutdownTimeoutMs = 30000)defaults to 30 seconds.Fix
packages/kilo-telemetry/src/client.ts:flush()call. PostHog'sshutdown(timeoutMs)drains the queue internally (see thedoShutdownloop in posthog-core), so the priorawait client.flush()is redundant on the happy path and harmful on the unreachable path.timeoutMsparameter, threaded through toclient.shutdown(timeoutMs).try/finallysoclient = nullruns even if shutdown rejects.packages/kilo-telemetry/src/telemetry.ts:timeoutMsparameter, passed through toClient.shutdown.packages/opencode/src/index.ts:2000from the CLI bootstrap's finally-block. Two seconds is generous for a working endpoint and keeps short-lived commands likekilo --help/kilo --versionsnappy when the endpoint is unreachable.Sibling check
Telemetry.shutdown(has exactly one caller in the repo (packages/opencode/src/index.ts:319).Client.shutdown(has one caller (packages/kilo-telemetry/src/telemetry.ts:245). The new optional parameter is a backward-compatible API extension; existing callers that omit it preserve PostHog's 30s default.Test
packages/kilo-telemetry/src/__tests__/telemetry.test.ts: new testTelemetry.shutdown timeout (#9788)mocksposthog-nodeso the fakeflush()would hang for 60s if called. Asserts:flush()is never invoked (the explicit unbounded call is gone)timeoutMs(50ms) is threaded through to PostHog'sshutdown(timeoutMs)Typecheck passes (
bun run typecheckclean for bothpackages/kilo-telemetryandpackages/opencode).