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

[codedsultan]

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.

[codecov]

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.

[hwbrzzl]

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

[codedsultan]

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

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

[hwbrzzl]

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

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

Here

[codedsultan]

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

[hwbrzzl]

The setting icon? Could you paste a screenshot?

[goravel-coder]

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]

\

The setting icon? Could you paste a screenshot?

[codedsultan]

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]

The setting icon? Could you paste a screenshot?

Okay, sorry, I had a mistake.

[hwbrzzl]

@codedsultan #1529 has been merged

[hwbrzzl]

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]

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]

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)

[hwbrzzl]

Please simplify the annotations.

[hwbrzzl]

Did you test the feature locally? Could you paste some screenshots in the PR description?

[goravel-coder]

Code Review: Notification Tries/Backoff for Queued Dispatch (revised)

Re-examined against the full worker flow (queue/worker.go:102-171) and BroadcastJob.Handle+ShouldRetry reference implementation. One severity downgrade, one new finding.

---

Must Fix

1. j.item not cleared on non-retry error paths → bounded retry waste — notification/job.go:369-374

j.mu.Lock()
j.item = &item
j.mu.Unlock()

ch := j.manager.Channel(item.Channel)
if ch == nil {
    return errors.NotificationChannelNotFound.Args(item.Channel)  // ← j.item NOT cleared
}

resolvable, ok := ch.(notification.ResolvableChannel)
if !ok {
    return errors.NotificationChannelNotQueueable.Args(item.Channel)  // ← j.item NOT cleared
}

Tracing the full execution flow:

worker.call() → Handle returns error with j.item still set (Tries=3)
             → reservedJob.Attempts() returns attempt count from DB
             → ShouldRetry(callErr, attempt) reads stale j.item
             → attempt < Tries → retry! Release(delay) back to queue
             → repeats until attempt == Tries (3 wasted retries)

Both errors are unrecoverable (channel doesn't exist or can't queue) — retrying accomplishes nothing. The attempt counter from reservedJob.Attempts() bounds this at Tries (not infinite), but it's still wasted work.

Compare to BroadcastJob.Handle:72-78 which correctly clears j.item before returning on its analogous config error:

cfg, err := NewConfig(j.config)
if err != nil {
    j.mu.Lock()
    j.item = nil   // ← clears item
    j.mu.Unlock()
    return err
}

Fix: add j.item = nil under mutex before both returns. The resolvable.Deliver() error path intentionally does NOT clear — that's the transient failure that should be retried.

---

Should Fix

2. PR description claims deliveryError wrapper that doesn't exist — notification/job.go

PR body says:

DispatchJob wraps a Deliver() failure in a small deliveryError type so ShouldRetry...can recover them via errors.As

The code does no such thing — Handle returns the raw Deliver error, and ShouldRetry ignores err entirely (error-agnostic, matching BroadcastJob exactly). The code is correct; the description is stale.

3. Orphaned mocks/notification/NotificationWithRetryUntil.go — file not deleted

The interface was removed from contracts but the mock file remains. It compiles (no interface compliance check, imports only still-existing types) but is dead code. Delete the file or regenerate mocks.

4. %w → %v change correct but undocumented — errors/list.go:207-214

fmt.Sprintf (used by the framework's Error()) doesn't support %w. Old code was silently producing garbled %!w(...) output. This is a bug fix, not incidental cleanup.

5. Implementation details in contract comments — contracts/notification/notification.go:32-43

The NotificationWithBackoff doc describes ShouldRetry-level internals (indexing logic, Laravel references). Trim to match the terser ShouldBroadcastWithBackoff style.

6. Tries(name) called twice — notification/notification.go:578-579

if withTries, ok := n.(...NotificationWithTries); ok && withTries.Tries(name) > 0 {
    item.Tries = withTries.Tries(name)
}

Cache the result to avoid double invocation.

7. json.Unmarshal error silently discarded — notification/job.go:358-362

The underlying unmarshal error is lost; operator sees only generic "missing or malformed". Matches BroadcastJob pattern but worth wrapping.

---

Verified Correct (system-level trace)

What	Verdict
Worker attempt starts at 1, overridden by reservedJob.Attempts() (line 103/123)	✓ Persists across restarts
task.Job is the shared singleton — Handle and ShouldRetry called on same instance (line 129)	✓ Mutex is necessary
ShouldRetry returning false → failed_jobs table + Delete() (line 159-374)	✓ Exhausted retries recorded
All 10 table-driven ShouldRetry test cases	✓ Correct for every edge
dispatchQueued eager capture while notification is live	✓ Mirrors broadcasting
Backoff gated on Tries > 0	✓ Test confirms omit-when-zero
Go 1.25.0 — min() built-in available	✓
omitempty JSON tags on new fields	✓ Backward-compatible with existing queue payloads