Skip to content

fix(router): resolve memory leak from uncancelled contexts in async mode#866

Merged
appleboy merged 2 commits intoappleboy:masterfrom
savinnorse:claude/blissful-shockley
Mar 21, 2026
Merged

fix(router): resolve memory leak from uncancelled contexts in async mode#866
appleboy merged 2 commits intoappleboy:masterfrom
savinnorse:claude/blissful-shockley

Conversation

@savinnorse
Copy link
Copy Markdown
Contributor

Summary

  • Fix memory leak in pushHandler caused by context.WithCancel(context.Background()) where cancel() is never called in async mode (sync: false)
  • Each request leaked a context + a monitoring goroutine, growing memory indefinitely
  • Replace with c.Request.Context() whose lifecycle is managed by net/http automatically

Root Cause

In pushHandler, every incoming request creates a new context via context.WithCancel(context.Background()) and spawns a goroutine that only calls cancel() when cfg.Core.Sync is true. In async mode (the default and most common configuration), cancel() is never invoked, so:

  1. The child context remains registered in Go's internal context tree under context.Background() forever
  2. The monitoring goroutine stays blocked on <-c.Request.Context().Done() until the HTTP connection closes, then exits without cancelling

Over time this causes unbounded memory growth (~1 KB per request). At moderate traffic this reaches GiBs within days.

Fix

Replace the context.WithCancel + goroutine pattern with c.Request.Context() directly. This is safe because handleNotification already ignores the context parameter (_ context.Context), so the behavioral change is zero — we're only removing the leak.

Evidence

Production memory graph showing continuous growth over 7 days with gorush v1.18.4:

Memory climbs from ~100 MiB to ~1.9 GiB without ever being reclaimed, a classic leak pattern.

Test plan

  • go build -tags sqlite ./... compiles successfully
  • Existing tests pass (router tests require FCM credentials, unrelated)
  • Deploy and monitor memory usage over 24-48h to confirm flat memory profile

🤖 Generated with Claude Code

a020792s and others added 2 commits March 18, 2026 10:33
In pushHandler, context.WithCancel(context.Background()) was called on
every request, but cancel() was only invoked in sync mode. In async mode
(the common case), the context was never cancelled, causing it to persist
in Go's internal context tree for the lifetime of the process. Each
request also leaked a monitoring goroutine blocked on <-c.Request.Context().Done().

Replace with c.Request.Context() which is automatically managed by
net/http and cleaned up when the request completes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@appleboy
Copy link
Copy Markdown
Owner

I will take it.

@appleboy appleboy merged commit e189f99 into appleboy:master Mar 21, 2026
11 checks passed
appleboy added a commit that referenced this pull request Mar 21, 2026
Previously, handleNotification accepted a context.Context parameter but ignored it (_ context.Context). The context passed from pushHandler (c.Request.Context()) was therefore never used when dispatching notifications, so client disconnection could not cancel in-flight push tasks.

This commit:
1. Renames the ignored parameter to `ctx` so it is actually used.
2. Adds a withEitherCancel helper that merges the HTTP request context with the queue-task context, cancelling when either is done (client disconnects OR queue shuts down).
3. Threads the merged context through to notify.SendNotification, which already propagates it to PushToIOS / PushToAndroid / PushToHuawei and DispatchFeedback.

This completes the fix originally intended by the goroutine pattern in issue #422, which was removed in #866 because the cancel() call was never reached in async mode.
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.

3 participants