fix(analytics): stop filing user-side network drops as wizard errors - #1182
Draft
posthog[bot] wants to merge 4 commits into
Conversation
The Slack connect poll (and other callers) already degrade correctly when the user's own network drops a socket, then still call `analytics.captureException`. Each errno and host string fingerprints as a separate error tracking issue, so every one-off opens a fresh issue that buries real wizard bugs. Filter transport-level errno codes once at `captureException`, reusing the shape of `BENIGN_FS_ERROR_CODES` in `bounded-fs.ts`. Reads the error `code` first, then falls back to scanning the message so API errors that fold the errno into their text (and drop `code`) are caught too. Generated-By: PostHog Desktop Task-Id: 217e822f-fa35-4794-bb03-c40575e49608
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 3 should fix, 1 consider. Published 4 findings (view the review). Resolved comments: 1 fixed, 1 already settled |
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Results will be posted here when complete. |
Contributor
Author
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+280
to
+286
| // Drop transport-level failures on the user's side. They never mean the | ||
| // wizard is broken and each variant opens its own error tracking issue. | ||
| const benign = benignTransportCode(error); | ||
| if (benign) { | ||
| logToFile(`[analytics] skipped benign transport error (${benign})`); | ||
| return; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Global filter hides fatal service failures
Why we think it's a valid issue
-
Checked: every non-test caller of
captureException/captureUnknown, the error shape thathandleApiErrorbuilds, and the fatal-exit path insrc/lib/runners/run-wizard.ts. -
Found:
handleApiErroratsrc/lib/api.ts:298-302returnsnew ApiError(\Failed to ${operation} (${axiosError.code ?? 'network error'})`)for every transport failure. The errno therefore sits in the message of each API error, so the message scan atsrc/utils/analytics.ts:101-104` drops it. -
Found: Three capture sites report and then rethrow, so the failure is fatal and not recovered:
src/utils/setup-utils.ts:672(step: 'wizard_login', thenthrow error),src/utils/setup-utils.ts:552and585(CI project lookup, then rethrow), andsrc/lib/programs/posthog-doctor/fetch.ts:24(throw apiError). This contradicts the new comment atsrc/utils/analytics.ts:73-76, which states that every caller of these codes already degrades on its own. -
Found: No other signal replaces the dropped event on the login path. The fatal catch at
src/lib/runners/run-wizard.ts:249-281logs to file, prints to the console, and callsprocess.exit(1). It never captures, and it never callsanalytics.shutdown, which is the only source of thesetup wizard finishedterminal event (src/utils/analytics.ts:399-421, called fromwizard-abort.ts:80,linear.ts:301,orchestrator-runner.ts:1099,start-tui.ts:90). The singlecaptureUnknownatsetup-utils.ts:672was the only report for a failed login. -
Found: The codebase already uses call-site filtering for the equivalent case.
src/utils/bounded-fs.ts:41-44drops benign filesystem codes inside its own reporter, where the caller is known to continue. This is the precedent the suggestion asks for. -
Impact: If the PostHog API host becomes unreachable, or a build points at a wrong host or port, every run dies at login with
ECONNREFUSED,ECONNRESET, orETIMEDOUT, and Error Tracking records nothing at all. The team keeps no exception event and no terminal event for that run. This is a swallowed error that hides a failure, and the trigger and the consequence are both concrete.
Issue description
captureException reports both recoverable and fatal failures. Login, CI, and health-check paths capture errors before they rethrow them. A reset, refusal, or timeout now returns here without an error event. An errno cannot show whether the user, PostHog, or the wizard caused the failure. A service outage can therefore stop the wizard while Error Tracking stays silent.
Suggested fix
Make suppression opt-in for callers that recover. For example, add an option and call captureException(error, properties, { suppressTransportErrors: true }) from the Slack poll and filesystem reporter. Keep the default capture path for fatal failures. Add tests for both recovered and rethrown transport errors.
Prompt to fix with AI (copy-paste)
## Context
@src/utils/analytics.ts#L280-286
<issue_description>
`captureException` reports both recoverable and fatal failures. Login, CI, and health-check paths capture errors before they rethrow them. A reset, refusal, or timeout now returns here without an error event. An errno cannot show whether the user, PostHog, or the wizard caused the failure. A service outage can therefore stop the wizard while Error Tracking stays silent.
</issue_description>
<issue_validation>
- **Checked:** every non-test caller of `captureException`/`captureUnknown`, the error shape that `handleApiError` builds, and the fatal-exit path in `src/lib/runners/run-wizard.ts`.
- **Found:** `handleApiError` at `src/lib/api.ts:298-302` returns `new ApiError(\`Failed to ${operation} (${axiosError.code ?? 'network error'})\`)` for every transport failure. The errno therefore sits in the message of each API error, so the message scan at `src/utils/analytics.ts:101-104` drops it.
- **Found:** Three capture sites report and then rethrow, so the failure is fatal and not recovered: `src/utils/setup-utils.ts:672` (`step: 'wizard_login'`, then `throw error`), `src/utils/setup-utils.ts:552` and `585` (CI project lookup, then rethrow), and `src/lib/programs/posthog-doctor/fetch.ts:24` (`throw apiError`). This contradicts the new comment at `src/utils/analytics.ts:73-76`, which states that every caller of these codes already degrades on its own.
- **Found:** No other signal replaces the dropped event on the login path. The fatal catch at `src/lib/runners/run-wizard.ts:249-281` logs to file, prints to the console, and calls `process.exit(1)`. It never captures, and it never calls `analytics.shutdown`, which is the only source of the `setup wizard finished` terminal event (`src/utils/analytics.ts:399-421`, called from `wizard-abort.ts:80`, `linear.ts:301`, `orchestrator-runner.ts:1099`, `start-tui.ts:90`). The single `captureUnknown` at `setup-utils.ts:672` was the only report for a failed login.
- **Found:** The codebase already uses call-site filtering for the equivalent case. `src/utils/bounded-fs.ts:41-44` drops benign filesystem codes inside its own reporter, where the caller is known to continue. This is the precedent the suggestion asks for.
- **Impact:** If the PostHog API host becomes unreachable, or a build points at a wrong host or port, every run dies at login with `ECONNREFUSED`, `ECONNRESET`, or `ETIMEDOUT`, and Error Tracking records nothing at all. The team keeps no exception event and no terminal event for that run. This is a swallowed error that hides a failure, and the trigger and the consequence are both concrete.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Make suppression opt-in for callers that recover. For example, add an option and call `captureException(error, properties, { suppressTransportErrors: true })` from the Slack poll and filesystem reporter. Keep the default capture path for fatal failures. Add tests for both recovered and rethrown transport errors.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed — this is a real gap at current head, not a stale flag. I traced every capture site: the global filter in captureException drops fatal transport failures, not just recovered ones.
- The CI project lookup (
fetchProjectDataWithApiKey/fetchProjectDataByIdin setup-utils.ts) and the doctor health-issues fetch (posthog-doctor/fetch.ts) both capture and then rethrow.handleApiErrorfolds the errno into the message (Failed to … (ECONNRESET)) and dropscode, so the message scan matches and the event is dropped before it's ever sent. - The rethrow lands on the fatal exit in run-wizard.ts, which logs to file and
process.exit(1)without capturing and without callinganalytics.shutdown— so neither an exception event nor a terminal event survives.
Net effect: if a build points at a wrong host/port, or the API times out, a run can die at login/CI/doctor with nothing in Error Tracking. The PR comment's premise that 'every caller already degrades on its own' doesn't hold for these rethrowing sites.
I'm not fixing this unattended because the fix is a design decision. The suggested opt-in suppressTransportErrors flag reverses this PR's central choice — 'quiet once at the capture site, not at each call site' — and adds a new option to the analytics API. There are a few defensible resolutions: (a) opt-in suppression at the two recovering callers (Slack poll + bounded-fs) while fatal paths keep capturing; (b) move filtering entirely back to those recovering sites and drop the chokepoint; or (c) accept the tradeoff, on the view that a true outage can't transmit a capture anyway and only the misconfigured-host case genuinely loses signal.
Human decision needed: keep the chokepoint filter and accept that fatal login/CI/doctor transport failures stop capturing, or make suppression opt-in so those fatal paths capture again. I'll implement whichever you choose.
…ing transport noise benignTransportCode's message fallback used message.includes(candidate), a bare substring match. Several install-failure reporters (Codex/Claude MCP add) wrap raw CLI stderr in a plain new Error(...) with no `code`, so the message scan is the only classifier that runs on them. If that stderr merely quoted a benign errno (e.g. a "retrying ECONNRESET" retry line) while the command actually failed for auth/permission/config reasons, the whole install failure was silently dropped from error tracking. Narrow the fallback to the exact "(CODE)" wrapper api.ts emits (`Failed to ... (ECONNRESET)`), which still matches the intended ApiError cases — including the CI re-wrap that appends text after the wrapper — but no longer fires on an errno embedded elsewhere in wrapped tool output. Add a negative test proving an install failure whose stderr mentions ECONNRESET still captures. Generated-By: PostHog Desktop Task-Id: 80a79146-2554-4271-88bc-894c3053ef14
… log line The skip log for a benign transport error wrote only the errno. Since these failures are deliberately withheld from error tracking, that debug line is the only surviving record — and it's a user-facing support artifact (printed as "Full logs: <path>" on failure and rendered in the RunScreen LogViewer). A bare "(ETIMEDOUT)" can't be attributed to the operation that produced it, whether a Slack poll, a project-tree read, or a doctor fetch. Include the operation context (properties.step / properties.source) and error.message alongside the errno, mirroring bounded-fs's skip log that this PR already cites as precedent. Callers redact secrets from these fields before reporting, and only step/source are read from properties, so no sensitive values reach the log. Generated-By: PostHog Desktop Task-Id: 80a79146-2554-4271-88bc-894c3053ef14
ENOTFOUND is Node/Axios's DNS-lookup-failed errno — the code a machine that is offline or behind a captive portal returns most often. It sits in the same user-side transport-failure class as EAI_AGAIN (temporary DNS, already allowlisted) and ECONNREFUSED, but was missing from BENIGN_TRANSPORT_ERROR_CODES, so a wizard user with no working DNS still opened a "Failed to fetch user data (ENOTFOUND)" error-tracking issue per operation — the exact noise this PR removes for ECONNRESET/ETIMEDOUT. handleApiError folds the errno into the ApiError message and drops `code`, so the "(ENOTFOUND)" wrapper is the only trace; the existing message scan in benignTransportCode now catches it. Adds a regression test that routes an ENOTFOUND AxiosError through handleApiError into captureException and asserts it is not reported. Generated-By: PostHog Desktop Task-Id: e271ea17-9e5a-4d29-93f1-74d5be2bd974
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SlackConnectScreen.tsxalready degrades correctly on a failed check — it marks the project not-connected, stops polling, and shows the connect nudge — then still callsanalytics.captureException.captureExceptionfilters nothing, so a user-side socket failure becomes a wizard error.Changes
captureExceptioninsrc/utils/analytics.ts), not at each call site — every reporter (SlackConnectScreen,bounded-fs,api.tscallers) flows through this one chokepoint.BENIGN_FS_ERROR_CODESinbounded-fs.ts: a named set of benign transport codes (ECONNRESET,ETIMEDOUT,EHOSTUNREACH, and similar).codefield first, then fall back to scanning the message —api.tsfolds the errno into theApiErrortext and dropscode, so(ECONNRESET)in the message is the only trace left.Test plan
analytics.test.tscover: a raw socket error with acode, a host-unreachable socket error, a wrapped API error carrying the errno only in its message, a filesystem timeout, and a genuine error that must still capture.pnpm build && pnpm test— 2026 tests pass.pnpm lint— 0 errors.LLM context
api.tsandanalytics.tsin the same error-noise family.Created with PostHog Desktop from this inbox report.