Skip to content

feat(notification): add interval for new question emails#1545

Open
AsyncAssassin wants to merge 1 commit into
apache:devfrom
AsyncAssassin:feat/625-email-notification-interval
Open

feat(notification): add interval for new question emails#1545
AsyncAssassin wants to merge 1 commit into
apache:devfrom
AsyncAssassin:feat/625-email-notification-interval

Conversation

@AsyncAssassin

@AsyncAssassin AsyncAssassin commented Jun 17, 2026

Copy link
Copy Markdown

Summary

Adds a configurable interval between email send attempts for new-question / followed-tag notification fan-out.

The interval is controlled by NEW_QUESTION_NOTIFICATION_EMAIL_SEND_INTERVAL_SECONDS.

The default is 0, so the current behavior is preserved unless the environment variable is explicitly set. Empty, invalid, negative, or overflowing values also fall back to 0.

Scope

This is based on the scope discussed in #625 and confirmed by @LinkinStars.

This PR only applies the delay to new-question notification email fan-out. It does not change:

  • daily digest behavior
  • logged-in user handling
  • verification emails
  • password reset emails
  • SMTP test emails
  • the shared EmailService.Send
  • UI/admin settings
  • migrations

When the interval is enabled, plugin notifications are sent before the throttled email fan-out, so they are not delayed by the configured sleep.

Tests

Added focused tests for:

  • environment variable parsing
  • delay ordering between email attempts
  • disabled/non-email channel handling
  • unsubscribe token generation per email attempt
  • service-level new-question fan-out behavior with fake service dependencies

Validation:

go test -count=1 ./internal/service/notification/...
go test -race -count=1 ./internal/service/notification/...
go test -count=1 ./internal/...
go test -count=1 ./...
go vet ./internal/service/notification/...
git diff --check

@LinkinStars LinkinStars self-requested a review June 18, 2026 02:34
@LinkinStars LinkinStars self-assigned this Jun 18, 2026
@LinkinStars LinkinStars added this to the v2.0.2 milestone Jun 18, 2026
@LinkinStars

Copy link
Copy Markdown
Member

Thanks for working on this. The scope matches the discussion in #625, and the default 0 interval preserves the current behavior.

One concern: the current implementation sleeps inside handleNewQuestionNotification. Since external_notification is processed by a single queue worker, this does not only throttle new-question emails. A large fan-out with a configured interval can block the whole external notification queue, delaying unrelated notifications such as new answer, new comment, and invite-answer emails.

There is also a shutdown concern because the wait uses time.Sleep, so it cannot be cancelled. If the interval is large or misconfigured, queue shutdown may have to wait for the sleep to finish.

I think a better approach would be to move the throttled new-question email fan-out into a dedicated queue/worker:

  • handleNewQuestionNotification collects subscribers, syncs plugin notifications, enqueues new-question email tasks, and returns immediately.
  • A dedicated new_question_email worker consumes those tasks serially.
  • The worker waits between email attempts using a context-aware timer/select instead of bare time.Sleep.
  • The interval should probably have a reasonable upper bound to avoid accidental long blocking from misconfiguration.

This would keep the intended email throttling behavior while avoiding delays for unrelated external notifications and making shutdown behavior safer.

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.

2 participants