Skip to content

fix(cdp): give external-request timeouts a real stack and upstream context#67268

Draft
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-code/fetch-timeout-error-context
Draft

fix(cdp): give external-request timeouts a real stack and upstream context#67268
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-code/fetch-timeout-error-context

Conversation

@posthog

@posthog posthog Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Problem

Every outbound request in nodejs/src/common/utils/request.ts is given signal: AbortSignal.timeout(...). When an upstream endpoint is slow and that signal fires, it rejects with a TimeoutError DOMException that is constructed inside Node's timer callback — so its stack contains only node-internal frames (process.processTimerslistOnTimeoutTimeout._onTimeoutnew DOMException) and no caller context. Because the stack is identical regardless of who made the call, every external-request timeout across the service collapses into a single error-tracking fingerprint, with no URL to say which dependency is actually timing out.

This is an internal observability problem: the one giant issue both drowns out real errors and hides which upstream is degraded, and (because each capture uses a fresh anonymous distinct ID) inflates the "users" count into noise.

Changes

  • Added RequestTimeoutError, which carries the target host, full url, and timeoutMs, and preserves the original DOMException as cause.
  • Wrapped the AbortSignal.timeout rejection in both _fetch and legacyFetch so a timeout is rethrown from the awaiting call site. The rethrown error's stack follows the async caller chain (real in-app frames) instead of the node-internal timer frames, and the message names the host. Together these make timeouts group by real caller + upstream rather than into one bucket.
  • Detection uses signal.aborted on the timeout signal — robust regardless of whether undici surfaces the abort as a TimeoutError or AbortError.

Retry behavior is unchanged: isFetchResponseRetriable still treats the timeout as a general retriable error, and isConnectionLevelError still returns false for it (no code), exactly as with the previous raw DOMException.

Separately worth a follow-up: the default EXTERNAL_REQUEST_TIMEOUT_MS is 3000ms — this change makes it observable which upstreams are hitting that ceiling, so the team can decide whether it's too tight for the callers now timing out (likely CDP fetch destinations).

How did you test this code?

I'm an agent; I did not do manual testing. Automated tests I ran locally in nodejs/:

  • Added one case to request.test.ts (request timeout handling): a request against a non-responding local route with a short timeoutMs must surface as a RequestTimeoutError naming the host, with host/timeoutMs/cause populated. This guards the fix's core contract — a regression that reverted to the raw node-internal DOMException (name TimeoutError, no host) would fail it. It couldn't extend an existing test because none exercised the timeout path.
  • Full src/common/utils/request.test.ts suite: 80 passed, 2 pre-existing skips.
  • tsc --noEmit and eslint on the changed files: clean.

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

🤖 Agent context

Autonomy: Fully autonomous

Authored by Claude Code (Opus 4.8) acting on an inbox report about a spiking, un-debuggable error-tracking issue. Invoked the /writing-tests skill before adding the test to apply the value gate (targeted regression guard, single parameter-free case on the cheapest rung).

Key decisions:

  • Detect via signal.aborted, not error name. In these two functions the timeout signal is the only abort source, so aborted unambiguously means "timed out" and is robust to whether undici rethrows the reason directly or wraps it.
  • Host in the message, not the full URL. Full URLs (query params, path ids) would over-split the fingerprint; the host is the right granularity for "which dependency is degraded." Full url is retained as a property for when a handler logs extra.
  • No per-request stack capture. The error (and its stack) is only constructed on the timeout path, so the hot success path is untouched; Node's async stack traces supply the caller chain from the awaiting frame.
  • Left EXTERNAL_REQUEST_TIMEOUT_MS at 3000ms — changing a global timeout is a separate risk decision; this PR makes that decision informable.

Created with PostHog Code from an inbox report.

…ntext

AbortSignal.timeout rejects with a DOMException built inside Node's timer
callback, so its stack is entirely node-internal and identical for every
caller — every fetch timeout across the service collapsed into one
un-debuggable error fingerprint with no URL or caller context.

Wrap the timeout rejection in _fetch and legacyFetch and rethrow a
RequestTimeoutError from the awaiting call site: the stack now follows the
async caller chain and the target host is embedded in the message, so
timeouts group by which upstream is actually degraded. The original
DOMException is preserved as `cause`.

Generated-By: PostHog Code
Task-Id: 6337f532-5050-407d-9f2f-17a38adf7cb6
@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants