fix(router): propagate HTTP request context into push notification tasks

[appleboy]

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.

[Copilot]

Pull request overview

This PR fixes push-notification cancellation behavior by ensuring the HTTP request context is actually used when dispatching notifications, so client disconnects (or queue shutdown) can cancel in-flight sync push work.

Changes:

• Introduces withEitherCancel to merge request and queue-task cancellation.
• Uses the previously-ignored context.Context parameter in handleNotification.
• Passes the merged context into notify.SendNotification for local sync mode.

Comments suppressed due to low confidence (1)

router/server.go:339

• logs = append(logs, resp.Logs...) is executed inside queue worker tasks; with a pool (queue.NewPool) multiple tasks can run concurrently, so this append mutates a shared slice without synchronization and can data race/panic. Use a mutex around logs writes or collect per-task logs into a channel and merge after wg.Wait().

					resp, err := notify.SendNotification(mergedCtx, msg, cfg)
					if err != nil {
						return err
					}
					logs = append(logs, resp.Logs...)
					return nil

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In local sync mode, wg.Add(1) happens before QueueTask, but if q.QueueTask(...) returns an error (e.g., queue closed/full), the task body never runs and wg.Done() is never called. This can deadlock wg.Wait() and hang the HTTP request. Consider calling wg.Done() on the QueueTask error path (and optionally recording failed logs, similar to the q.Queue(...) branch).

Suggested change

	}); err != nil {
	}); err != nil {
	if cfg.Core.Sync {
	wg.Done()
	}

[Copilot]

The new cancellation behavior is central to this change, but there’s no unit test covering withEitherCancel (e.g., verifying it cancels when either parent context is canceled, and that the returned cancel func stops the goroutine). Adding a focused test in router/server_test.go would prevent regressions in the sync-timeout/disconnect behavior.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

router/server.go:353

• wg.Done() is called when q.Queue(notification) returns an error even if cfg.Core.Sync is false. In async mode this will panic with "sync: negative WaitGroup counter" because wg.Add(1) is only done when sync is enabled. Guard the wg.Done() here behind if cfg.Core.Sync (or restructure so the WaitGroup is only used in sync mode).

		} else if err := q.Queue(notification); err != nil {
			resp := markFailedNotification(cfg, notification, "max capacity reached")
			logs = append(logs, resp...)
			wg.Done()
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

withEitherCancel starts a goroutine per call. Since this can run per-notification in sync mode, it adds avoidable overhead. Given this repo targets Go 1.25, consider using context.AfterFunc(ctx2, cancel) (and stopping it via the returned stop func) to avoid spawning a goroutine while preserving the same cancellation semantics.

Suggested change

	go func() {
	select {
	case <-ctx2.Done():
	cancel()
	case <-ctx.Done():
	}
	}()
	return ctx, cancel
	// Use context.AfterFunc to avoid spawning a goroutine per call while preserving
	// the same cancellation semantics: cancel when ctx2 is done.
	stop := context.AfterFunc(ctx2, cancel)
	// Return a CancelFunc that both stops the AfterFunc callback and cancels ctx.
	return ctx, func() {
	// Ensure we do not leave a pending AfterFunc callback around.
	stop()
	cancel()
	}

[Copilot]

These tests rely on a 100ms time.After timeout to observe cancellation. This is prone to flakes on slower/contended CI runners. Prefer require.Eventually/polling with a more generous overall timeout (e.g., 1–2s) to make the tests deterministic.

[Copilot]

TestWithEitherCancel_NoGoroutineLeak is likely flaky: runtime.NumGoroutine() is process-global (other tests/background goroutines can change it), and fixed sleeps make timing-dependent assertions. Also, assert.GreaterOrEqual(goroutinesDuring, goroutinesBefore) doesn’t actually validate that a new goroutine started despite the message saying "at least one". Consider dropping the goroutine-count assertions (or rewriting to avoid global counts) and focusing on functional behavior (merged ctx cancels; returned cancel releases resources).

Suggested change

	// preventing goroutine leaks.
	func TestWithEitherCancel_NoGoroutineLeak(t *testing.T) {
	ctx1, cancel1 := context.WithCancel(context.Background())
	ctx2 := t.Context()
	goroutinesBefore := runtime.NumGoroutine()
	merged, cancelMerged := withEitherCancel(ctx1, ctx2)
	// Give the internal goroutine time to start
	time.Sleep(10 * time.Millisecond)
	goroutinesDuring := runtime.NumGoroutine()
	assert.GreaterOrEqual(t, goroutinesDuring, goroutinesBefore,
	"at least one new goroutine should exist while merged context is live")
	// Cancel via ctx1 - this should trigger the internal goroutine to exit
	cancel1()
	cancelMerged() // also call cancelMerged to release resources
	// Give the goroutine time to clean up
	time.Sleep(50 * time.Millisecond)
	goroutinesAfter := runtime.NumGoroutine()
	// Allow a ±1 goroutine variance due to runtime scheduling
	assert.LessOrEqual(t, goroutinesAfter, goroutinesBefore+1,
	"goroutine count should be near baseline after cancellation")
	// merged context must be done
	select {
	case <-merged.Done():
	// expected
	default:
	t.Fatal("merged context should be done after cancel1() and cancelMerged()")
	// preventing goroutine leaks. Instead of counting goroutines (which is
	// process-global and flaky), we assert the functional behavior of the
	// merged context around cancellation.
	func TestWithEitherCancel_NoGoroutineLeak(t *testing.T) {
	ctx1, cancel1 := context.WithCancel(context.Background())
	defer cancel1()
	ctx2 := t.Context()
	merged, cancelMerged := withEitherCancel(ctx1, ctx2)
	defer cancelMerged()
	// Initially, the merged context should not be done.
	select {
	case <-merged.Done():
	t.Fatal("merged context should not be done before any input context is cancelled")
	default:
	// expected: still active
	}
	// Cancel via ctx1 - this should trigger the merged context to be done
	cancel1()
	select {
	case <-merged.Done():
	// expected: merged context became done after cancellation
	case <-time.After(100 * time.Millisecond):
	t.Fatal("merged context should be done shortly after ctx1 is cancelled")
	}
	// Calling cancelMerged should be safe and keep the merged context done.
	cancelMerged()
	select {
	case <-merged.Done():
	// still done, as expected
	case <-time.After(50 * time.Millisecond):
	t.Fatal("merged context should remain done after cancelMerged is called")