Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
appleboy wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b6004a
fix(router): propagate HTTP request context into push notification tasks
appleboy Mar 21, 2026
6e1ec2c
fix(router): fix wg deadlock and logs race condition in handleNotific…
appleboy Mar 21, 2026
0df6075
test(router): add unit tests for withEitherCancel
appleboy Mar 21, 2026
75f33c7
style(router): fix gofmt double blank line before withEitherCancel
appleboy Mar 21, 2026
adb7be4
style(router): fix golangci-lint issues in withEitherCancel tests
appleboy Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 27 additions & 5 deletions router/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,25 @@
return count
}


Check failure on line 283 in router/server.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (gofmt)
// withEitherCancel returns a context that is cancelled when either ctx1 or ctx2 is done.
// This is useful for merging an HTTP request context with a queue-task context so that
// a push notification is aborted when the caller disconnects OR when the queue shuts down.
func withEitherCancel(ctx1, ctx2 context.Context) (context.Context, context.CancelFunc) {
ctx, cancel := context.WithCancel(ctx1)
go func() {
select {
case <-ctx2.Done():
cancel()
case <-ctx.Done():
}
}()
return ctx, cancel
Comment on lines +288 to +295

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
Comment on lines +283 to +296

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

// HandleNotification add notification to queue list.
func handleNotification(
_ context.Context,
ctx context.Context,
cfg *config.ConfYaml,
req notify.RequestPush,
q *queue.Queue,
Expand All @@ -306,10 +322,16 @@
}

if isLocalSync {
func(msg *notify.PushNotification, cfg *config.ConfYaml) {
if err := q.QueueTask(func(ctx context.Context) error {
func(msg *notify.PushNotification, cfg *config.ConfYaml, reqCtx context.Context) {
if err := q.QueueTask(func(queueCtx context.Context) error {
defer wg.Done()
resp, err := notify.SendNotification(ctx, msg, cfg)
// Merge the HTTP request context with the queue context so that
// the notification is cancelled when either the client disconnects
// or the queue shuts down. See:
// https://github.com/appleboy/gorush/issues/422
mergedCtx, cancel := withEitherCancel(reqCtx, queueCtx)
defer cancel()
resp, err := notify.SendNotification(mergedCtx, msg, cfg)
if err != nil {
return err
}
Expand All @@ -318,7 +340,7 @@
}); err != nil {

Copilot AI Mar 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
logx.LogError.Error(err)
}
}(notification, cfg)
}(notification, cfg, ctx)
} else if err := q.Queue(notification); err != nil {
resp := markFailedNotification(cfg, notification, "max capacity reached")
logs = append(logs, resp...)
Expand Down
Loading