fix(speakers): remove the quadratic hydration and the unordered pagination behind the bulk send ( promo codes ) - #593
Conversation
…a speaker as sent SpeakersPromoCodeTrait::setEmailSent found the AssignedPromoCodeSpeaker to mark through $this->owners->filter(). Collection::filter() initializes the whole collection regardless of the EXTRA_LAZY mapping, and the closure then dereferenced ->getSpeaker()->getEmail() on every element, lazily hydrating one PresentationSpeaker per element as well. Every speaker of a bulk send shares the same promo code, so the cost of marking speaker N grew with the number of speakers already assigned to it, and every speaker of the run stayed pinned in the identity map. Over a 683 recipient send that is quadratic hydration on top of a heap that only grows. The lookup is now a targeted query. The e-mail matching mirrors PresentationSpeaker::getEmail(), which is computed rather than mapped: the member's e-mail wins, and the registration request's is only used when the speaker has no member. Behaviour is unchanged. The tests were written against the previous implementation and pass unmodified against this one. They cover both branches of the e-mail precedence, a speaker carrying both a member and a registration request, the scoping to this promo code when the same speaker is assigned to another one, and an unknown recipient. Signed-off-by: smarcet <smarcet@gmail.com>
…n getParametrizedAllIdsByPage The default-order callback of getParametrizedAllIdsByPage was chained to $filter instead of $order, so it only ran when no filter was given. Any filtered page was therefore emitted with LIMIT/OFFSET and no ORDER BY at all, and MySQL makes no promise about the order of such a result: paging through one can skip rows or return the same row on two different pages. The sibling getParametrizedAllByPage already chains the same callback to $order, which is the intended contract - an explicit order wins, otherwise the caller's default applies. This aligns the two. The only caller is DoctrineSpeakerRepository::getSpeakersIdsBySummit, whose callback is the default speaker order (e.id ASC). It never passes an explicit order, and ParametrizedSendEmails substitutes an empty Filter when none was given, so in practice the bulk speaker send has been paging an unordered query. The regression test asserts the generated DQL rather than paged data on purpose: an unordered query frequently happens to come back in a stable order, so a data-level test would pass by luck against the defect. It covers a filtered query, an unfiltered one, and that an explicit order still suppresses the default. Signed-off-by: smarcet <smarcet@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes update speaker promo code assignment matching and restore default ordering for paginated ID queries. New tests cover email precedence, case-insensitive matching, promo code isolation, unknown recipients, and explicit or default query ordering. ChangesSpeaker promo code email matching
Paginated ID query ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR replaces inefficient speaker sent-state lookup and restores deterministic ordering for filtered pagination without changing intended behavior; targeted and repository tests are green, so no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately identifies both main changes: targeted handling that removes quadratic hydration and corrected pagination ordering for bulk promo-code sends. It is specific and related to the changeset, although the spacing inside the parentheses is unnecessary.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-593/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🟢 Approval recommended
The fixes are well-scoped, align with existing repository contracts, and are backed by targeted regression tests; the only noted concern is a minor warning-path behavior discrepancy.
Pull request overview
This PR addresses two reliability/performance issues impacting bulk speaker email sends: (1) eliminating quadratic Doctrine hydration when marking a speaker promo code assignment as “sent”, and (2) ensuring paginated ID queries remain deterministic by applying the default ORDER BY even when filters are present.
Changes:
- Refactors speaker promo-code “mark as sent” resolution to use a targeted query instead of filtering an EXTRA_LAZY collection.
- Fixes
DoctrineRepository::getParametrizedAllIdsByPageso the default-order callback is applied whenever no explicit order is provided (even with filters). - Adds focused regression tests covering email-precedence matching/scoping and default-order behavior in filtered pagination.
File summaries
| File | Description |
|---|---|
| tests/SpeakersPromoCodeMarkSentTest.php | Adds coverage proving the new lookup matches PresentationSpeaker::getEmail() precedence and promo-code scoping. |
| tests/ParametrizedAllIdsByPageOrderTest.php | Adds regression tests ensuring default ORDER BY is present for filtered/unfiltered ID pagination and suppressed by explicit order. |
| app/Repositories/DoctrineRepository.php | Applies default order fallback for ID pagination when no explicit order is provided, regardless of filter presence. |
| app/Models/Foundation/Summit/Registration/PromoCodes/Traits/SpeakersPromoCodeTrait.php | Replaces collection filtering with a targeted DQL lookup to avoid quadratic hydration during bulk sends. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $recipient = strtolower(trim($recipient ?? '')); | ||
|
|
||
| if (empty($recipient)) | ||
| throw new ValidationException("Can't mark the promo_code as sent without a recipient."); |
smarcet
changed the title
ref:https://app.clickup.com/t/9014802374/86bbreptr
Two independent fixes found while investigating the 2026-08-31 bulk speaker email
incident (summit 73, promo code OCPSPEAKER26, killed at 500 of 683 speakers). Both
are self-contained; neither changes behaviour.
1. Quadratic hydration in the per-speaker mark-as-sent path
SpeakersPromoCodeTrait::setEmailSentfound theAssignedPromoCodeSpeakerto markthrough
$this->owners->filter().Collection::filter()goes throughAbstractLazyCollection::filter(), which callsinitialize()unconditionally, so theEXTRA_LAZYmapping on$ownersdoes not apply. The closure then dereferenced->getSpeaker()->getEmail()on every element, lazily hydrating onePresentationSpeakerper element as well.Every speaker of a bulk send shares the same promo code, so marking speaker N
hydrated N assignments plus N speakers. The cost of each speaker grew with the number
already assigned, and every speaker of the run stayed pinned in the identity map.
Over a 683 recipient send that is quadratic hydration on top of a heap that only
grows — it matches the observed decay from 57 to 10 speakers/min.
The lookup is now a targeted query. The e-mail matching mirrors
PresentationSpeaker::getEmail(), which is computed rather than mapped: the member'se-mail wins, and the registration request's is only used when the speaker has no
member.
2. Missing ORDER BY in filtered ids pagination
The default-order callback of
DoctrineRepository::getParametrizedAllIdsByPagewaschained to
$filterinstead of$order, so it only ran when no filter was given. Anyfiltered page was emitted with LIMIT/OFFSET and no ORDER BY, and MySQL makes no
promise about the order of such a result: paging through one can skip rows or return
the same row on two different pages.
The sibling
getParametrizedAllByPagealready chains the same callback to$order,which is the intended contract. This aligns the two.
The only caller is
DoctrineSpeakerRepository::getSpeakersIdsBySummit, whose callbackis the default speaker order (
e.id ASC). It never passes an explicit order, andParametrizedSendEmailssubstitutes an emptyFilterwhen none was given, so inpractice the bulk speaker send had been paging an unordered query on every run.
Tests
tests/SpeakersPromoCodeMarkSentTest.php— 7 tests. Because this is a refactor andnot a behaviour change, they were written against the previous implementation
first and passed 4/4 there; they then pass unmodified against the new one, which is
the equivalence proof. Coverage: both branches of the
getEmail()precedence, aspeaker carrying both a member and a registration request (where the precedence
actually matters), the scoping to this promo code when the same speaker is assigned
to another one, case-insensitivity, and an unknown recipient.
tests/ParametrizedAllIdsByPageOrderTest.php— 3 tests: a filtered query, anunfiltered one, and that an explicit order still suppresses the default. Red/green
verified — reverting the fix fails the filtered case.
The assertion there is on the generated DQL rather than on paged data on purpose: an
unordered query frequently happens to come back in a stable order, so a data-level
test would pass by luck against the defect and would not protect against a
regression. That reasoning is in the test's docblock so it does not get "improved"
away later.
Two mutations were run against each fix to confirm the tests have teeth. One did not
fail: removing
LOWER()from the column side still passes, because MySQL's collationis already case-insensitive — so that test is backed by the database rather than by
the SQL, and the
LOWER()is defensive.Verification
Run inside the local
summit-apicontainer.SpeakersPromoCodeMarkSentTest+ParametrizedAllIdsByPageOrderTest: 10 tests, green.(
SpeakerRepositoryTest,SubmitterRepositoryTest,SummitRegistrationPromoCodeRepositoryTest,tests/Repositories, plus the twoabove): 84 tests, 251 assertions, exit 0.
PromoCodesServiceTesthas 2 errors on the sponsor promo code path. Verifiedpre-existing: identical on a clean
main.SpeakerServiceTesthas 3 errors from fixture summit ids that do not exist in thelocal DB. Also verified pre-existing.
Not in this PR
The ticket carries the rest of the plan. Worth knowing while reviewing:
de-duplication is currently off in production (
should_resenddefaults totrueand the admin never sends the key), so retries cannot be enabled until that is fixed;
and the worker deployment sets no
terminationGracePeriodSeconds, so it defaults to30s and any SIGTERM kills a long send silently.
Summary by CodeRabbit
Bug Fixes
Tests