Skip to content

feat(notification): wire NotificationWithBackoff/RetryUntil to real queue retry - #1528

Open
codedsultan wants to merge 5 commits into
goravel:masterfrom
codedsultan:feature/notification-backoff-retry
Open

feat(notification): wire NotificationWithBackoff/RetryUntil to real queue retry#1528
codedsultan wants to merge 5 commits into
goravel:masterfrom
codedsultan:feature/notification-backoff-retry

Conversation

@codedsultan

Copy link
Copy Markdown
Contributor

Summary

Wires NotificationWithBackoff/RetryUntil to real queue retry behavior, using Goravel's documented optional queue.Job interface:

ShouldRetry(err error, attempt int) (retryable bool, delay time.Duration)

Confirmed against the documented job retry contract: https://www.goravel.dev/digging-deeper/queues.html#job-retry

Design

  • Backoff()/RetryUntil() are evaluated once, eagerly, in Manager.dispatchQueued — while the live notification still exists — and carried through the queue boundary as two new dispatchItem fields.
  • DispatchJob wraps a Deliver() failure in a small deliveryError type so ShouldRetry (called by the worker with only (err, attempt), no access to decoded job state) can recover them via errors.As.
  • DispatchJob itself stays fully stateless — no per-execution fields, since it's registered once and potentially shared across concurrent worker goroutines.
  • Without RetryUntil set, NotificationWithBackoff alone would retry indefinitely. Adds DefaultMaxRetryAttempts (exported var, default 10), applied only when RetryUntil isn't set — RetryUntil already provides its own bound and takes precedence when both are present.

Known limitation

Supports a single fixed backoff per notification+channel, not a growing per-attempt schedule — there's no live notification left to call a second time for a bigger number by retry time.

@codedsultan
codedsultan requested a review from a team as a code owner July 31, 2026 18:23
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.83%. Comparing base (cf1480b) to head (dd66aed).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1528      +/-   ##
==========================================
+ Coverage   69.80%   69.83%   +0.03%     
==========================================
  Files         409      409              
  Lines       31388    31428      +40     
==========================================
+ Hits        21909    21949      +40     
  Misses       8445     8445              
  Partials     1034     1034              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codedsultan
codedsultan force-pushed the feature/notification-backoff-retry branch from ff25d66 to a942be4 Compare July 31, 2026 18:34
…ueue retry

Uses Goravel's documented optional queue.Job interface —

    ShouldRetry(err error, attempt int) (retryable bool, delay time.Duration)

confirmed at https://www.goravel.dev/digging-deeper/queues.html#job-retry.

Backoff()/RetryUntil() are evaluated once, eagerly, in
Manager.dispatchQueued — while the live notification still exists — and
carried through the queue boundary as two new dispatchItem fields.

DispatchJob wraps a Deliver() failure in a small deliveryError type so
ShouldRetry (called by the worker with only (err, attempt), no access to
decoded job state) can recover them via errors.As. DispatchJob itself
stays fully stateless — no per-execution fields, since it's registered
once and potentially shared across concurrent worker goroutines.

Without RetryUntil set, NotificationWithBackoff alone would retry
indefinitely, since ShouldRetry had no other bound to check. Adds
DefaultMaxRetryAttempts (exported var, default 10), applied only when
RetryUntil isn't set — RetryUntil already provides its own bound and
takes precedence when both are present.

Known limitation: supports a single fixed backoff per
notification+channel, not a growing per-attempt schedule — there's no
live notification left to call a second time for a bigger number by
retry time.
@codedsultan
codedsultan force-pushed the feature/notification-backoff-retry branch from a942be4 to dd9df8a Compare July 31, 2026 18:38
@hwbrzzl

hwbrzzl commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

FYI, please add the Ready Review tag once it's ready.

@codedsultan

Copy link
Copy Markdown
Contributor Author

FYI, please add the Ready Review tag once it's ready.

Ready Review. I cant find the tag is it a slash command ?

@hwbrzzl

hwbrzzl commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

FYI, please add the Ready Review tag once it's ready.

Ready Review. I cant find the tag is it a slash command ?

Here

image

@codedsultan

Copy link
Copy Markdown
Contributor Author

the setting icon doesn't appear for me in the label section

@hwbrzzl

hwbrzzl commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

The setting icon? Could you paste a screenshot?

@goravel-coder

Copy link
Copy Markdown
Contributor

PR #1528 wires NotificationWithBackoff as a fixed scalar (Backoff(channel string) int), but PR #1529 (queued broadcast retry) — which is consistent with Laravel — uses a per-attempt schedule: BroadcastBackoff() []time.Duration, last value repeats (BroadcastJob.ShouldRetry indexes min(attempt-1, len(backoff)-1)).

Laravel resolves backoff the same way (Worker::calculateBackoff: $backoff[$job->attempts() - 1] ?? last($backoff) — seconds, scalar or list).

Suggestion: keep #1528 consistent with #1529 — change Backoff(channel string) to return []time.Duration (per-attempt, last repeats) and index it in DispatchJob.ShouldRetry like BroadcastJob. This also removes the "single fixed backoff" limitation noted in this PR.

@codedsultan

Copy link
Copy Markdown
Contributor Author

\

The setting icon? Could you paste a screenshot?

Screenshot 2026-08-02 at 11 36 18 AM

@codedsultan

Copy link
Copy Markdown
Contributor Author

PR #1528 wires NotificationWithBackoff as a fixed scalar (Backoff(channel string) int), but PR #1529 (queued broadcast retry) — which is consistent with Laravel — uses a per-attempt schedule: BroadcastBackoff() []time.Duration, last value repeats (BroadcastJob.ShouldRetry indexes min(attempt-1, len(backoff)-1)).

Laravel resolves backoff the same way (Worker::calculateBackoff: $backoff[$job->attempts() - 1] ?? last($backoff) — seconds, scalar or list).

Suggestion: keep #1528 consistent with #1529 — change Backoff(channel string) to return []time.Duration (per-attempt, last repeats) and index it in DispatchJob.ShouldRetry like BroadcastJob. This also removes the "single fixed backoff" limitation noted in this PR.

Ok , i think i should wait for PR #1529 to be merged

@hwbrzzl

hwbrzzl commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

The setting icon? Could you paste a screenshot?

Okay, sorry, I had a mistake.

@hwbrzzl

hwbrzzl commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@codedsultan #1529 has been merged

@hwbrzzl

hwbrzzl commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Given the broadcasting feature implements the Tries and Backoff functions, I think we can implement them in Notification as well. The RetryUntil function can be implemented when it's needed. @codedsultan What do you think about this, please?

@codedsultan

Copy link
Copy Markdown
Contributor Author

Given the broadcasting feature implements the Tries and Backoff functions, I think we can implement them in Notification as well. The RetryUntil function can be implemented when it's needed. @codedsultan What do you think about this, please?

I will get back to you on this.

@codedsultan

codedsultan commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Given the broadcasting feature implements the Tries and Backoff functions, I think we can implement them in Notification as well. The RetryUntil function can be implemented when it's needed. @codedsultan What do you think about this, please?

Notification now uses the same Tries/Backoff design as broadcasting: NotificationWithTries.Tries(channel string) int and NotificationWithBackoff.Backoff(channel string) []time.Duration, captured eagerly at dispatch and indexed in DispatchJob.ShouldRetry the same way BroadcastJob does (mutex-guarded item, last backoff value repeats)

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