fix(cli): stop telemetry hangs and errors on blocked networks#5880
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@74a0e053158089fae8129f41d30300d2a1e0dbcePreview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a96c6f5826
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e38cb95741
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
avallete
left a comment
There was a problem hiding this comment.
The Go-side timeout and debug-log routing in this PR look good and should stay. I don’t think we need to additionally coordinate one shared shutdown budget across TS parents and Go children: that would broaden the change into cross-runtime telemetry ownership for a transitional sidecar. Let’s keep the blocking scope focused on ensuring the TS client’s 2s shutdown cap also bounds actual Bun process exit.
…ry-network-resilience
…ilience' into pamela/cli-telemetry-network-resilience
|
also if this doesn't work, and you have a preferred method @avallete that isn't how i did it, can i close this PR and I'll create a linear issue for CLI team to figure this out? |
avallete
left a comment
There was a problem hiding this comment.
All good, thank's for all the adjustments you made through the review loops @pamelachia !
On networks that block or blackhole
eu.posthog.com(corporate proxies, CI and Docker egress policies, air-gapped machines), telemetry breaks the CLI itself: stable v2.109.1 pays a 5s trailing hang and exits 1 with an UnknownError payload after every command, long-running commands print raw SDK stack traces to stderr, and the Go binary's shutdown wait is unbounded. The only workaround wassupabase telemetry disable, which permanently removes those users from CLI analytics. This PR makes telemetry strictly fire-and-forget: a failed delivery never changes a command's output, exit code, or runtime beyond a 2s cap.Changed:
Effect.promise(...).pipe(Effect.ignore), butEffect.promiseturns the shutdown-deadline rejection into a defect thatEffect.ignorecannot swallow, so a telemetry timeout failed the whole command. The rejection is now caught on the promise itself.EXIT_DELAY_CAP_MS) bounds each request and the entire_shutdowndrain on the TS side, and the Go client gets the same 2sShutdownTimeout(the SDK default waits indefinitely, including retries). The previous 5s shutdown deadline left room for two sequential 2s request timeouts (~4s exits).AbortControllernow composes into every request's signal and fires when the deadline hits, so no request survives or starts after the scope closes. Covered by a unit regression asserting zero active requests one tick after release (red on the previous code) and an e2e test that runs the compiled binary against a TCP blackhole and asserts wall-clock process exit, empty stderr, and that telemetry actually reached the wire.--debuglogger; TS reports delivery failures as delivered inside the wrapped fetch, because posthog-node has no logger hook, only hardcodedconsole.errorcalls, so an infallible transport is the one supported way to get no retries, no output, and no stall.Note: the exit bounds are caps, not floors. On healthy networks both shells return as soon as the flush completes (
flushAt: 1keeps the send overlapping command runtime), and fully detaching the send instead would race process exit and silently drop events on healthy networks. The accepted tradeoff is that deliveries slower than 2s are dropped; telemetry is best-effort, observable under--debugon the Go side only.Linear