fix(proxy): batch Traefik outdated notifications system per team.#9262
fix(proxy): batch Traefik outdated notifications system per team.#9262Iisyourdad wants to merge 5 commits intocoollabsio:nextfrom
Conversation
|
@coderabbitai do your thing! |
|
Consider it done — I'm spinning up my review engines right now. No serverless nonsense here, just good old-fashioned server-side processing! 🌮 Let's see what we've got with this batch Traefik notification fix. ✅ Actions performedReview triggered.
|
WalkthroughRefactors Traefik version checks to a coordinated batched scan flow. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Jobs/CheckTraefikVersionForServerJob.php`:
- Around line 26-30: The constructor of CheckTraefikVersionForServerJob allows
creating a "batched" job with shouldNotify = false but without checkedAt,
causing storeOutdatedInfo to use now() per server and breaking aggregation; fix
by preventing that: make the constructor private (or add a guard) and add two
named constructors—e.g. public static function forImmediateNotify(Server
$server, array $traefikVersions) which sets shouldNotify = true and checkedAt =
null, and public static function forBatchedRun(Server $server, array
$traefikVersions, string $checkedAt) which sets shouldNotify = false and
requires checkedAt; alternatively, if you prefer not to make the constructor
private, add a guard in __construct that throws InvalidArgumentException when
shouldNotify === false && checkedAt === null so storeOutdatedInfo always
receives a shared checkedAt.
In `@app/Jobs/CheckTraefikVersionJob.php`:
- Around line 44-52: Replace the timer-based coordination with a Bus::batch
completion barrier: instead of dispatching CheckTraefikVersionForServerJob per
server then scheduling NotifyOutdatedTraefikServersJob with
calculateNotificationDelay/isDev, collect the per-server jobs into a
Bus::batch([...]) and attach a then()/finally() callback that dispatches
NotifyOutdatedTraefikServersJob($checkedAt) only after the batch completes;
remove the heuristic delay logic (calculateNotificationDelay and the isDev
override) so notification reliably runs after all
CheckTraefikVersionForServerJob instances finish.
In `@app/Jobs/NotifyOutdatedTraefikServersJob.php`:
- Around line 29-60: Eager-load the Team relationship on the initial Server
query by adding ->with('team') to the query that builds $servers, and then stop
calling Team::find() inside the group loop; instead obtain the team from a
server in the group (e.g. $team = $teamServers->first()->team) and guard for
null before calling $team->notify(...). Update the code that currently calls
Team::find($teamId) to use the hydrated $server->team reference when grouping
and notifying.
In `@tests/Unit/NotifyOutdatedTraefikServersJobTest.php`:
- Around line 8-11: The test uses RefreshDatabase and real factories (see
uses(RefreshDatabase::class)) so it must be moved out of Unit tests; relocate
the file from tests/Unit/NotifyOutdatedTraefikServersJobTest.php into
tests/Feature/ (update the namespace/path if present) so it runs under the
Feature test suite, keeping the existing imports
(Illuminate\Foundation\Testing\RefreshDatabase and
Illuminate\Support\Facades\Notification) and any factory/model usage intact.
- Around line 116-123: The test currently only asserts that the correct
TraefikVersionOutdated notifications were sent to $team1 and $team2 but doesn't
guard against duplicates; add an explicit assertion after the two
Notification::assertSentTo(...) checks that verifies Notification::sent()
contains exactly 2 notifications (one per team) to ensure no duplicate emails
are sent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 196969e3-4a67-40ae-89c3-5a28a6623448
📒 Files selected for processing (6)
app/Jobs/CheckTraefikVersionForServerJob.phpapp/Jobs/CheckTraefikVersionJob.phpapp/Jobs/NotifyOutdatedTraefikServersJob.phptests/Feature/CheckTraefikVersionJobTest.phptests/Unit/CheckTraefikVersionForServerJobTest.phptests/Unit/NotifyOutdatedTraefikServersJobTest.php
There was a problem hiding this comment.
This test files test something that is no longer used.
|
Could we do the same for server patches? :) |
|
@jstet is there a problem aswell for server patches? I addressed your comment in your pr about the jobs in the most recent commit. |
|
@coderabbitai please review again. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/Jobs/CheckTraefikVersionForServerJob.php (1)
27-35:⚠️ Potential issue | 🟡 MinorReject blank
scanIdvalues too.An empty string gets past Line 33, but Line 163 then drops
scan_idbecause the check is truthy. In batched mode that means no immediate notification and no aggregated one either. Skynet likes correlation IDs; make this reject blank / whitespace IDs, not onlynull.🛠️ Suggested hardening
- if (! $this->shouldNotify && is_null($this->scanId)) { + if (! $this->shouldNotify && blank($this->scanId)) { throw new InvalidArgumentException('Batched Traefik version checks require a shared scan identifier.'); } @@ - if ($this->scanId) { + if ($this->scanId !== null) { $outdatedInfo['scan_id'] = $this->scanId; }Also applies to: 163-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/CheckTraefikVersionForServerJob.php` around lines 27 - 35, The constructor in CheckTraefikVersionForServerJob currently only rejects null scanId when shouldNotify is false, allowing empty/whitespace strings which later get treated as falsy and dropped; update the constructor validation to throw InvalidArgumentException when shouldNotify is false and scanId is null or empty/whitespace (use trim($scanId) === ''). Also audit the code path that emits/builds the payload (where scan_id is conditionally included — the code that currently checks truthiness of $this->scanId, around the scan_id inclusion) and change that check to a strict null check (!== null) so intentionally empty/whitespace values are not silently dropped; reference the class CheckTraefikVersionForServerJob, its __construct, the public bool $shouldNotify and public ?string $scanId properties, and the payload/scan_id inclusion logic to implement these fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Jobs/NotifyOutdatedTraefikServersJob.php`:
- Around line 21-29: The job NotifyOutdatedTraefikServersJob currently retries
($tries = 3) while iterating and calling notify() for every team, which causes
already-sent teams to be resent on retry; fix this by splitting delivery into
one queued job per team: change NotifyOutdatedTraefikServersJob to accept a
single team identifier (e.g., teamId) along with scanId/lock keys and move the
per-team notify() logic into its handle method, then update the higher-level
dispatcher (where the job was originally queued) to enumerate teams and dispatch
one NotifyOutdatedTraefikServersJob per team (so each job retries independently
and cannot re-send other teams), or alternatively persist per-team progress
keyed by scanId before retrying if you must keep a batch job; ensure references
to notify(), scanId, $tries, and the constructor signature are updated
accordingly.
---
Duplicate comments:
In `@app/Jobs/CheckTraefikVersionForServerJob.php`:
- Around line 27-35: The constructor in CheckTraefikVersionForServerJob
currently only rejects null scanId when shouldNotify is false, allowing
empty/whitespace strings which later get treated as falsy and dropped; update
the constructor validation to throw InvalidArgumentException when shouldNotify
is false and scanId is null or empty/whitespace (use trim($scanId) === ''). Also
audit the code path that emits/builds the payload (where scan_id is
conditionally included — the code that currently checks truthiness of
$this->scanId, around the scan_id inclusion) and change that check to a strict
null check (!== null) so intentionally empty/whitespace values are not silently
dropped; reference the class CheckTraefikVersionForServerJob, its __construct,
the public bool $shouldNotify and public ?string $scanId properties, and the
payload/scan_id inclusion logic to implement these fixes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b87c773-f8d4-492c-8ed8-7d41ebfae814
📒 Files selected for processing (8)
app/Jobs/CheckTraefikVersionForServerJob.phpapp/Jobs/CheckTraefikVersionJob.phpapp/Jobs/NotifyOutdatedTraefikServersJob.phpapp/Models/Server.phptests/Feature/CheckTraefikVersionJobTest.phptests/Feature/NotifyOutdatedTraefikServersJobTest.phptests/Unit/CheckTraefikVersionForServerJobTest.phptests/Unit/NotifyOutdatedTraefikServersJobTest.php
💤 Files with no reviewable changes (1)
- tests/Unit/NotifyOutdatedTraefikServersJobTest.php
not really a problem, just a feature request to avoid inbox overload. Sry for not being specific |
2 participants
Changes
Issues
Category
AI Assistance
If AI was used:
Testing
tests/Feature/CheckTraefikVersionJobTest.php tests/Unit/CheckTraefikVersionForServerJobTest.php tests/Unit/NotifyOutdatedTraefikServersJobTest.phpContributor Agreement
Important