Skip to content

fix: goroutine leak in IngestEvents when a connection stalls mid-upload - #474

Open
Scientifik wants to merge 1 commit into
axiomhq:mainfrom
Scientifik:fix/ingest-events-goroutine-leak
Open

Scientifik wants to merge 1 commit into
axiomhq:mainfrom
Scientifik:fix/ingest-events-goroutine-leak

Conversation

@Scientifik

Copy link
Copy Markdown

getBody spawns a goroutine that streams zstd-compressed events into an io.Pipe consumed by the outgoing request body. io.Pipe writes block until read; if the connection stalls before the transport finishes draining the body — as opposed to failing fast with an error — nothing unblocks that goroutine, and it (plus its pooled zstd encoder) leaks for the life of the process.

Found via a live SIGQUIT goroutine dump on a production service: over a several-hour window, thousands of goroutines accumulated on this exact stack, driving sustained GC thrashing and pushing the process to within ~4% of its memory limit before recovery.

Two changes, both needed for this to actually protect real callers:

  1. getBody's encoding goroutine now races ctx.Done() against its own completion, closing the pipe reader with ctx's error if the caller's context ends first. This unblocks the goroutine for any caller that supplies a ctx with a deadline or explicit cancellation.

  2. IngestChannel now bounds each flush's IngestEvents call with its own timeout (30s), derived from the loop's own ctx rather than replacing it. This matters because IngestChannel's own long-lived ctx often has no deadline of its own in practice — both the slog and Logrus adapters' background ingest loops run it on context.Background() — so (1) alone cannot help that call path; nothing was ever going to become Done. TestDatasetsService_IngestChannel_FlushTimeout reproduces exactly this shape (context.Background(), a stalled connection) under synctest, and fails with a caught deadlock — not a timeout — if (2) is reverted: synctest correctly identifies that every goroutine in the bubble is durably blocked with nothing left to advance the clock, at the exact stack from the production dump.

TestDatasetsService_IngestEvents_GoroutineLeak reproduces the first issue directly against IngestEvents, via a minimal RoundTripper that never reads the request body (a real httptest server can't reliably reproduce this — small payloads fit entirely into OS socket buffers and the write completes without anything needing to read it, at any payload size short of impractically large).

getBody spawns a goroutine that streams zstd-compressed events into an
io.Pipe consumed by the outgoing request body. io.Pipe writes block
until read; if the connection stalls before the transport finishes
draining the body — as opposed to failing fast with an error — nothing
unblocks that goroutine, and it (plus its pooled zstd encoder) leaks
for the life of the process.

Found via a live SIGQUIT goroutine dump on a production service: over
a several-hour window, thousands of goroutines accumulated on this
exact stack, driving sustained GC thrashing and pushing the process to
within ~4% of its memory limit before recovery.

Two changes, both needed for this to actually protect real callers:

1. getBody's encoding goroutine now races ctx.Done() against its own
   completion, closing the pipe reader with ctx's error if the caller's
   context ends first. This unblocks the goroutine for any caller that
   supplies a ctx with a deadline or explicit cancellation.

2. IngestChannel now bounds each flush's IngestEvents call with its own
   timeout (30s), derived from the loop's own ctx rather than replacing
   it. This matters because IngestChannel's own long-lived ctx often has
   no deadline of its own in practice — both the slog and Logrus
   adapters' background ingest loops run it on context.Background() —
   so (1) alone cannot help that call path; nothing was ever going to
   become Done. TestDatasetsService_IngestChannel_FlushTimeout
   reproduces exactly this shape (context.Background(), a stalled
   connection) under synctest, and fails with a caught deadlock — not a
   timeout — if (2) is reverted: synctest correctly identifies that
   every goroutine in the bubble is durably blocked with nothing left
   to advance the clock, at the exact stack from the production dump.

TestDatasetsService_IngestEvents_GoroutineLeak reproduces the first
issue directly against IngestEvents, via a minimal RoundTripper that
never reads the request body (a real httptest server can't reliably
reproduce this — small payloads fit entirely into OS socket buffers and
the write completes without anything needing to read it, at any
payload size short of impractically large).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant