From 52a937abf903b4af1f64f7eda53cc5b9a85a9911 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 1 Sep 2026 23:55:11 -0300 Subject: [PATCH 01/12] feat(speakers): chunk the bulk speaker email send and unify its filter whitelist SpeakerService::triggerSendEmails dispatched a single ProcessSpeakersEmailRequestJob for the whole matched set, which the ParametrizedSendEmails trait then paged through internally inside that one queued job. A killed or failed job lost the entire run; on 2026-08-31 that lost 183 of 683 speakers mid-send with no trace. triggerSendEmails now resolves the full set of matched speaker ids synchronously (from an explicit speaker_ids payload if given, otherwise by paging getSpeakersIdsBySummit), applies excluded_speaker_ids, de-duplicates, and dispatches one ProcessSpeakersEmailRequestJob per 100-id chunk (SpeakerService::CHUNK_SIZE) - the same job class, unchanged in its own per-speaker processing. A killed or failed job now loses at most one chunk. Every dispatched chunk receives the exact same raw, unparsed $filter value triggerSendEmails itself received - not a Filter object, not the filter used to select ids. That raw value also scopes which of a speaker's presentations count as accepted/alternate/rejected inside SpeakerActionsEmailStrategy::process, which decides the email type sent to that speaker; passing anything else would silently change that. This is asserted directly in the tests via ReflectionObject on the dispatched job's private filter property. Two behavior changes, both deliberate: a filter matching zero speakers now dispatches nothing (previously a single job still ran and could send a "0 sent" outcome e-mail); and duplicate ids are de-duplicated before dispatch (previously a repeated id was emailed twice). While rewriting every filter-parsing call site the send path depends on, unify them onto a new ISpeakerFilterFields interface (OPERATORS + VALIDATION_RULES constants, following the IEmailExcerptService interface-constants precedent already used in this codebase). Three of the four call sites already used the same 19-21 fields; one (SpeakerService's original_filter parse) was missing presentations_track_group_id. The bulk-send endpoint gains member_id/member_user_external_id as valid filter fields as a result - they were already supported by the repository and by the sibling listing/CSV/count endpoints, just never wired into the send path. tests/SpeakerServiceBulkSendChunkingTest.php: 9 tests. Most use an explicit speaker_ids payload with fabricated ids rather than seeded speakers, since Queue::fake() intercepts dispatch before the job's handle() ever runs and that path never queries the repository. Only the filter-based-selection and member_id cases seed real speakers. Coverage: chunk count and non-overlapping slices above/at/below CHUNK_SIZE, zero-match, exclusion, de-duplication, payload key pass-through, raw filter identity across both the explicit-ids and filter-based paths, and that member_id actually narrows the query rather than merely being accepted. Two mutations were run against the implementation to confirm the tests have teeth: swapping the raw filter for the internally-parsed one at dispatch time, and removing the de-duplication step. Both were caught. Signed-off-by: smarcet --- .../OAuth2SummitSpeakersApiController.php | 46 +-- .../ProcessSpeakersEmailRequestJob.php | 22 +- app/Services/Model/ISpeakerFilterFields.php | 74 +++++ app/Services/Model/Imp/SpeakerService.php | 58 ++-- tests/SpeakerServiceBulkSendChunkingTest.php | 290 ++++++++++++++++++ 5 files changed, 405 insertions(+), 85 deletions(-) create mode 100644 app/Services/Model/ISpeakerFilterFields.php create mode 100644 tests/SpeakerServiceBulkSendChunkingTest.php diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php index 4fb0c181e..a4e131027 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php @@ -3203,7 +3203,7 @@ public function deleteSpeakerBigPhoto($speaker_id) ), new OA\Parameter( name: 'filter', - description: 'Filter speakers by id, first_name, last_name, email, full_name, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_track_group_id, presentations_selection_plan_id, presentations_type_id, presentations_title, presentations_abstract, presentations_submitter_full_name, presentations_submitter_email, has_media_upload_with_type, has_not_media_upload_with_type', + description: 'Filter speakers by id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id, has_accepted_presentations, has_alternate_presentations, has_rejected_presentations, presentations_track_id, presentations_track_group_id, presentations_selection_plan_id, presentations_type_id, presentations_title, presentations_abstract, presentations_submitter_full_name, presentations_submitter_email, has_media_upload_with_type, has_not_media_upload_with_type', in: 'query', required: false, schema: new OA\Schema(type: 'string') @@ -3259,53 +3259,13 @@ public function send($summit_id) $filter = null; if (Request::has('filter')) { - $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]); + $filter = FilterParser::parse(Request::input('filter'), \services\model\ISpeakerFilterFields::OPERATORS); } if (is_null($filter)) $filter = new Filter(); - $filter->validate([ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]); + $filter->validate(\services\model\ISpeakerFilterFields::VALIDATION_RULES); $this->service->triggerSendEmails($summit, $payload, Request::input('filter')); diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index 9df2b20aa..e3bcf9c26 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -79,27 +79,7 @@ public function handle ) ); - $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]) : null; + $filter = !is_null($this->filter) ? FilterParser::parse($this->filter, \services\model\ISpeakerFilterFields::OPERATORS) : null; $service->sendEmails($this->summit_id, $this->payload, $filter); } diff --git a/app/Services/Model/ISpeakerFilterFields.php b/app/Services/Model/ISpeakerFilterFields.php new file mode 100644 index 000000000..a075bbe29 --- /dev/null +++ b/app/Services/Model/ISpeakerFilterFields.php @@ -0,0 +1,74 @@ + ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '@@', '=='], + 'last_name' => ['=@', '@@', '=='], + 'email' => ['=@', '@@', '=='], + 'full_name' => ['=@', '@@', '=='], + 'member_id' => ['=='], + 'member_user_external_id' => ['=='], + 'has_accepted_presentations' => ['=='], + 'has_alternate_presentations' => ['=='], + 'has_rejected_presentations' => ['=='], + 'presentations_track_id' => ['=='], + 'presentations_track_group_id' => ['=='], + 'presentations_selection_plan_id' => ['=='], + 'presentations_type_id' => ['=='], + 'presentations_title' => ['=@', '@@', '=='], + 'presentations_abstract' => ['=@', '@@', '=='], + 'presentations_submitter_full_name' => ['=@', '@@', '=='], + 'presentations_submitter_email' => ['=@', '@@', '=='], + 'has_media_upload_with_type' => ['=='], + 'has_not_media_upload_with_type' => ['=='], + ]; + + const VALIDATION_RULES = [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'email' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'member_user_external_id' => 'sometimes|integer', + 'has_accepted_presentations' => 'sometimes|string|in:true,false', + 'has_alternate_presentations' => 'sometimes|string|in:true,false', + 'has_rejected_presentations' => 'sometimes|string|in:true,false', + 'presentations_track_id' => 'sometimes|integer', + 'presentations_track_group_id' => 'sometimes|integer', + 'presentations_selection_plan_id' => 'sometimes|integer', + 'presentations_type_id' => 'sometimes|integer', + 'presentations_title' => 'sometimes|string', + 'presentations_abstract' => 'sometimes|string', + 'presentations_submitter_full_name' => 'sometimes|string', + 'presentations_submitter_email' => 'sometimes|string', + 'has_media_upload_with_type' => 'sometimes|integer', + 'has_not_media_upload_with_type' => 'sometimes|integer', + ]; +} diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index 42166a44b..da706e147 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -56,6 +56,7 @@ use models\summit\Summit; use utils\Filter; use utils\FilterParser; +use utils\PagingInfo; /** * Class SpeakerService @@ -1227,13 +1228,47 @@ public function deleteSpeakerBigPhoto($speaker_id): void }); } + const CHUNK_SIZE = 100; + /** * @inheritDoc */ public function triggerSendEmails(Summit $summit, array $payload, $filter = null): void { Log::debug(sprintf("SpeakerService::triggerSendEmails summit %s payload %s", $summit->getId(), json_encode($payload))); - ProcessSpeakersEmailRequestJob::dispatch($summit->getId(), $payload, $filter); + + if (isset($payload['speaker_ids'])) { + $ids = $payload['speaker_ids']; + } else { + $parsedFilter = !is_null($filter) ? FilterParser::parse($filter, ISpeakerFilterFields::OPERATORS) : null; + $ids = []; + $page = 1; + do { + $currentPage = $this->tx_service->transaction(function () use ($summit, $page, $parsedFilter) { + return $this->speaker_repository->getSpeakersIdsBySummit($summit, new PagingInfo($page, self::CHUNK_SIZE), $parsedFilter); + }); + $ids = array_merge($ids, $currentPage); + $page++; + } while (count($currentPage) > 0); + } + + if (isset($payload['excluded_speaker_ids'])) { + $ids = array_diff($ids, $payload['excluded_speaker_ids']); + } + + $ids = array_values(array_unique($ids)); + + if (empty($ids)) { + Log::debug(sprintf("SpeakerService::triggerSendEmails summit %s no speakers matched, nothing dispatched", $summit->getId())); + return; + } + + foreach (array_chunk($ids, self::CHUNK_SIZE) as $chunk) { + $chunkPayload = $payload; + $chunkPayload['speaker_ids'] = $chunk; + unset($chunkPayload['excluded_speaker_ids']); + ProcessSpeakersEmailRequestJob::dispatch($summit->getId(), $chunkPayload, $filter); + } } /** @@ -1316,26 +1351,7 @@ function ); $original_filter = count($original_filter) > 0 ? - FilterParser::parse($original_filter, [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]) : null; + FilterParser::parse($original_filter, ISpeakerFilterFields::OPERATORS) : null; } catch (\Exception $ex){ Log::warning($ex); diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php new file mode 100644 index 000000000..9c8b5988a --- /dev/null +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -0,0 +1,290 @@ + 'ACCEPTED_ALTERNATE']; + } + + /** + * @return ProcessSpeakersEmailRequestJob[] + */ + private function pushedJobs(): array + { + $jobs = []; + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, function ($job) use (&$jobs) { + $jobs[] = $job; + return true; + }); + return $jobs; + } + + private function jobProperty(ProcessSpeakersEmailRequestJob $job, string $name) + { + $reflection = new ReflectionObject($job); + $property = $reflection->getProperty($name); + $property->setAccessible(true); + return $property->getValue($job); + } + + public function testDispatchesOneChunkPerHundredIdsWithNoOverlap(): void + { + Queue::fake(); + $ids = range(1, 250); + $payload = $this->basePayload(); + $payload['speaker_ids'] = $ids; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 3); + + $jobs = $this->pushedJobs(); + $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $jobs); + + $this->assertCount(100, $slices[0]); + $this->assertCount(100, $slices[1]); + $this->assertCount(50, $slices[2]); + + $covered = array_merge(...$slices); + sort($covered); + $this->assertEquals($ids, $covered, 'the slices together must cover every matched id exactly once'); + } + + public function testDispatchesExactlyOneJobWhenMatchedCountEqualsChunkSize(): void + { + Queue::fake(); + $ids = range(1, 100); + $payload = $this->basePayload(); + $payload['speaker_ids'] = $ids; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + } + + public function testDispatchesNothingWhenSpeakerIdsIsEmpty(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = []; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertNotPushed(ProcessSpeakersEmailRequestJob::class); + } + + public function testExcludedSpeakerIdsAreRemovedBeforeChunking(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = range(1, 10); + $payload['excluded_speaker_ids'] = [3, 7]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $slice = $this->jobProperty($jobs[0], 'payload')['speaker_ids']; + sort($slice); + + $this->assertEquals([1, 2, 4, 5, 6, 8, 9, 10], $slice); + $this->assertArrayNotHasKey( + 'excluded_speaker_ids', + $this->jobProperty($jobs[0], 'payload'), + 'excluded_speaker_ids must not be carried forward once already applied' + ); + } + + public function testDuplicateExplicitIdsAreDedupedBeforeDispatch(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = [5, 5, 6]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $slice = $this->jobProperty($jobs[0], 'payload')['speaker_ids']; + sort($slice); + + $this->assertEquals([5, 6], $slice, 'a duplicate id must only appear once across all dispatched jobs'); + } + + public function testExplicitFilterRawValueReachesEveryChunkUnchanged(): void + { + Queue::fake(); + $payload = $this->basePayload(); + $payload['speaker_ids'] = range(1, 150); + $rawFilter = ['first_name=@Test']; + + $this->service()->triggerSendEmails(self::$summit, $payload, $rawFilter); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 2); + foreach ($this->pushedJobs() as $job) { + $this->assertSame( + $rawFilter, + $this->jobProperty($job, 'filter'), + "every chunk's private filter constructor argument must be identical (===) to the raw filter triggerSendEmails received" + ); + } + } + + public function testChunkPayloadCarriesOtherPayloadKeysThrough(): void + { + Queue::fake(); + $payload = [ + 'email_flow_event' => 'ACCEPTED_ALTERNATE', + 'speaker_ids' => [1, 2, 3], + 'original_filter' => ['id==1||2||3'], + 'promo_code_spec' => ['class_name' => 'SPEAKERS_PROMO_CODE', 'type' => 'ACCEPTED'], + 'test_email_recipient' => 'test@example.com', + 'outcome_email_recipient' => 'outcome@example.com', + 'should_resend' => false, + 'should_send_copy_2_submitter' => true, + ]; + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertEquals($payload['original_filter'], $chunkPayload['original_filter']); + $this->assertEquals($payload['promo_code_spec'], $chunkPayload['promo_code_spec']); + $this->assertEquals($payload['test_email_recipient'], $chunkPayload['test_email_recipient']); + $this->assertEquals($payload['outcome_email_recipient'], $chunkPayload['outcome_email_recipient']); + $this->assertEquals($payload['should_resend'], $chunkPayload['should_resend']); + $this->assertEquals($payload['should_send_copy_2_submitter'], $chunkPayload['should_send_copy_2_submitter']); + $this->assertEquals($payload['email_flow_event'], $chunkPayload['email_flow_event']); + $this->assertEquals([1, 2, 3], $chunkPayload['speaker_ids']); + } + + public function testFilterBasedSelectionResolvesRealMatchingSpeakersAndChunks(): void + { + Queue::fake(); + + // self::$defaultSpeaker already has 20 presentations attached to self::$summit + // (via InsertSummitTestData), so it matches getSpeakersIdsBySummit with no filter. + $payload = $this->basePayload(); + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertContains( + self::$defaultSpeaker->getId(), + $chunkPayload['speaker_ids'], + 'a speaker with a presentation in this summit must be selected by the filter-based path' + ); + $this->assertNull( + $this->jobProperty($jobs[0], 'filter'), + 'the raw filter (null, in this case) must still reach the dispatched job unchanged' + ); + } + + public function testMemberIdFilterSelectsOnlyTheMatchingSpeaker(): void + { + Queue::fake(); + + $prefix = str_random(10); + $member = new Member(); + $member->setEmail("chunk-test+{$prefix}@test.com"); + $member->setActive(true); + $member->setFirstName("Chunk"); + $member->setLastName("Test"); + $member->setEmailVerified(true); + $member->setUserExternalId(mt_rand()); + self::$em->persist($member); + + $memberSpeaker = new PresentationSpeaker(); + $memberSpeaker->setFirstName("Chunk"); + $memberSpeaker->setLastName("Test Speaker"); + $memberSpeaker->setMember($member); + self::$em->persist($memberSpeaker); + + $presentation = new \models\summit\Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle("Chunk test presentation {$prefix}"); + $presentation->setAbstract("Abstract {$prefix}"); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType(self::$defaultPresentationType); + $presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + $presentation->addSpeaker($memberSpeaker); + self::$em->persist($presentation); + + self::$em->flush(); + + $rawFilter = ['member_id==' . $member->getId()]; + + $payload = $this->basePayload(); + + $this->service()->triggerSendEmails(self::$summit, $payload, $rawFilter); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 1); + $jobs = $this->pushedJobs(); + $chunkPayload = $this->jobProperty($jobs[0], 'payload'); + + $this->assertEquals( + [$memberSpeaker->getId()], + $chunkPayload['speaker_ids'], + 'member_id must select only the speaker belonging to that member, not the default fixture speaker' + ); + } +} From 92bd4a99d4fa4bbf29e13eb48224bd60a385f7e5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 00:08:51 -0300 Subject: [PATCH 02/12] refactor(speakers): unify getSpeakers/getSpeakersActivitiesCount/getSpeakersCSV/getAll onto ISpeakerFilterFields getSpeakers(), getSpeakersActivitiesCount(), and getSpeakersCSV() already carried the exact 21-field whitelist ISpeakerFilterFields formalizes (byte-identical across all three). Point them at the shared interface instead of three independently-maintained inline copies. getAll() (the global, non-summit-scoped speaker listing) was planned to widen to the same 21 fields, matching the other three - but that's unsafe. Reproduced directly against getAllByPage(): applying presentations_track_id throws Doctrine\ORM\Query\QueryException ("too few parameters"). Every presentations_*/ has_*_presentations mapping in DoctrineSpeakerRepository::getFilterMappings() hard-codes a :summit bound parameter in its DQL (shared verbatim with the summit-scoped query methods, which bind it on their own base query); getAllByPage()/getAllIdsByPage() never do, because there is no single summit to scope a global listing by. 13 of the 21 fields hit this; only the 8 with no :summit reference (id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id) are safe on a summit-independent query - exactly getAll()'s original set. getAll() now references a new ISpeakerFilterFields::GLOBAL_OPERATORS/ GLOBAL_VALIDATION_RULES pair covering exactly those 8 fields, documented with the full field-by-field breakdown of why the other 13 don't apply. All four methods end up on one shared interface with zero drift risk; getAll() gains no new fields. Making the :summit clause conditional so those mappings work for both summit-scoped and global callers would be real repository-level work across ~13 shared DQL templates - out of scope here. Also corrects send()'s and getAll()'s Swagger filter descriptions, which were already stale before this change. tests/oauth2/OAuth2SummitSpeakersApiTest.php: testGetAllSpeakersFilteredByMemberId proves the swap to GLOBAL_OPERATORS doesn't regress getAll()'s existing member_id support; testGetAllSpeakersRejectsPresentationScopedFilter proves a presentation-scoped field still returns a clean validation error, not a 500. The other four target methods are covered by the existing functional suite (member/external-id/selection-plan/ media-upload/accepted/rejected/name filters), which passes unchanged - confirming the inline-array-to-constant swap is behavior-preserving there. Signed-off-by: smarcet --- .../OAuth2SummitSpeakersApiController.php | 171 ++---------------- app/Services/Model/ISpeakerFilterFields.php | 41 +++++ tests/oauth2/OAuth2SummitSpeakersApiTest.php | 95 ++++++++++ 3 files changed, 147 insertions(+), 160 deletions(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php index a4e131027..dc5fc6dcf 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php @@ -303,54 +303,10 @@ public function getSpeakers($summit_id) return $this->_getAll( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]; + return \services\model\ISpeakerFilterFields::OPERATORS; }, function () { - return [ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::VALIDATION_RULES; }, function () { return [ @@ -443,55 +399,11 @@ public function getSpeakersActivitiesCount($summit_id) $filter = null; if (Request::has('filter')) { - $filter = FilterParser::parse(Request::input('filter'), [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]); + $filter = FilterParser::parse(Request::input('filter'), \services\model\ISpeakerFilterFields::OPERATORS); } if (!is_null($filter)) { - $filter->validate([ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]); + $filter->validate(\services\model\ISpeakerFilterFields::VALIDATION_RULES); } $count = $this->speaker_repository->getUniqueActivitiesCountBySummit($summit, $filter); @@ -584,54 +496,10 @@ public function getSpeakersCSV($summit_id) return $this->_getAllCSV( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '@@', '=='], - 'last_name' => ['=@', '@@', '=='], - 'email' => ['=@', '@@', '=='], - 'full_name' => ['=@', '@@', '=='], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - 'has_accepted_presentations' => ['=='], - 'has_alternate_presentations' => ['=='], - 'has_rejected_presentations' => ['=='], - 'presentations_track_id' => ['=='], - 'presentations_track_group_id' => ['=='], - 'presentations_selection_plan_id' => ['=='], - 'presentations_type_id' => ['=='], - 'presentations_title' => ['=@', '@@', '=='], - 'presentations_abstract' => ['=@', '@@', '=='], - 'presentations_submitter_full_name' => ['=@', '@@', '=='], - 'presentations_submitter_email' => ['=@', '@@', '=='], - 'has_media_upload_with_type' => ['=='], - 'has_not_media_upload_with_type' => ['=='], - ]; + return \services\model\ISpeakerFilterFields::OPERATORS; }, function () { - return [ - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - 'has_accepted_presentations' => 'sometimes|string|in:true,false', - 'has_alternate_presentations' => 'sometimes|string|in:true,false', - 'has_rejected_presentations' => 'sometimes|string|in:true,false', - 'presentations_track_id' => 'sometimes|integer', - 'presentations_track_group_id' => 'sometimes|integer', - 'presentations_selection_plan_id' => 'sometimes|integer', - 'presentations_type_id' => 'sometimes|integer', - 'presentations_title' => 'sometimes|string', - 'presentations_abstract' => 'sometimes|string', - 'presentations_submitter_full_name' => 'sometimes|string', - 'presentations_submitter_email' => 'sometimes|string', - 'has_media_upload_with_type' => 'sometimes|integer', - 'has_not_media_upload_with_type' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::VALIDATION_RULES; }, function () { return [ @@ -865,7 +733,7 @@ function ($page, $per_page, $filter, $order, $applyExtraFilters) use ($summit) { ), new OA\Parameter( name: 'filter', - description: 'Filter by id, first_name, last_name, email, full_name, member_id', + description: 'Filter by id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id. This endpoint is not summit-scoped, so presentation-related filter fields (available on the per-summit speaker endpoints) do not apply here.', in: 'query', required: false, schema: new OA\Schema(type: 'string') @@ -912,29 +780,12 @@ public function getAll() { return $this->_getAll( function () { - return [ - 'id' => ['=='], - 'not_id' => ['=='], - 'first_name' => ['=@', '==','@@'], - 'last_name' => ['=@', '==','@@'], - 'email' => ['=@', '==','@@'], - 'full_name' => ['=@', '==','@@'], - 'member_id' => ['=='], - 'member_user_external_id' => ['=='], - ]; + // getAll() is summit-independent - only the GLOBAL_* subset is safe here, + // see ISpeakerFilterFields::GLOBAL_OPERATORS for why. + return \services\model\ISpeakerFilterFields::GLOBAL_OPERATORS; }, function () { - return [ - - 'id' => 'sometimes|integer', - 'not_id' => 'sometimes|integer', - 'first_name' => 'sometimes|string', - 'last_name' => 'sometimes|string', - 'email' => 'sometimes|string', - 'full_name' => 'sometimes|string', - 'member_id' => 'sometimes|integer', - 'member_user_external_id' => 'sometimes|integer', - ]; + return \services\model\ISpeakerFilterFields::GLOBAL_VALIDATION_RULES; }, function () { return [ diff --git a/app/Services/Model/ISpeakerFilterFields.php b/app/Services/Model/ISpeakerFilterFields.php index a075bbe29..ce4d43f52 100644 --- a/app/Services/Model/ISpeakerFilterFields.php +++ b/app/Services/Model/ISpeakerFilterFields.php @@ -71,4 +71,45 @@ interface ISpeakerFilterFields 'has_media_upload_with_type' => 'sometimes|integer', 'has_not_media_upload_with_type' => 'sometimes|integer', ]; + + /** + * The subset of OPERATORS safe for a summit-independent query (e.g. + * OAuth2SummitSpeakersApiController::getAll(), which uses + * DoctrineSpeakerRepository::getAllByPage() / getAllIdsByPage()). + * + * The 13 fields excluded here (has_accepted/alternate/rejected_presentations, + * every presentations_* field, has_(not_)media_upload_with_type) all hard-code a + * ":summit" bound parameter inside DoctrineSpeakerRepository::getFilterMappings()'s + * DQL templates (e.g. "__p41_:i.summit = :summit"), because that method is shared + * verbatim by the summit-scoped query methods (getSpeakersBySummit, + * getSpeakersIdsBySummit), which bind that parameter on their own base query. + * getAllByPage()/getAllIdsByPage() never bind it - there is no single summit to + * scope by on a global listing - so applying any of those 13 fields there throws + * Doctrine\ORM\Query\QueryException ("too few parameters"), not a silent no-op. + * Verified by reproducing the exception directly against getAllByPage() with a + * presentations_track_id filter. Removing the hard-coded :summit clause from those + * DQL templates would need it to become conditional depending on caller context - + * real repository work, out of scope for a filter-whitelist unification. + */ + const GLOBAL_OPERATORS = [ + 'id' => ['=='], + 'not_id' => ['=='], + 'first_name' => ['=@', '@@', '=='], + 'last_name' => ['=@', '@@', '=='], + 'email' => ['=@', '@@', '=='], + 'full_name' => ['=@', '@@', '=='], + 'member_id' => ['=='], + 'member_user_external_id' => ['=='], + ]; + + const GLOBAL_VALIDATION_RULES = [ + 'id' => 'sometimes|integer', + 'not_id' => 'sometimes|integer', + 'first_name' => 'sometimes|string', + 'last_name' => 'sometimes|string', + 'email' => 'sometimes|string', + 'full_name' => 'sometimes|string', + 'member_id' => 'sometimes|integer', + 'member_user_external_id' => 'sometimes|integer', + ]; } diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index 6e57e5760..c657428da 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -495,6 +495,101 @@ public function testGetCurrentSummitSpeakersOrderByIDAndFilteredBySelPlan() $this->assertTrue(count($speakers[0]->accepted_presentations) == 20); } + public function testGetAllSpeakersFilteredByMemberId() + { + // getAll() is global/non-summit-scoped. Its member_id/member_user_external_id + // support predates this change; this proves swapping its inline whitelist for + // ISpeakerFilterFields::GLOBAL_OPERATORS/GLOBAL_VALIDATION_RULES didn't break it. + // + // Widening getAll() to the full ISpeakerFilterFields (like the summit-scoped + // endpoints) was attempted and reverted: DoctrineSpeakerRepository::getFilterMappings() + // hard-codes a ":summit" bound parameter into every presentations_*/has_*_presentations + // DQL template (shared verbatim with the summit-scoped query methods, which bind it on + // their own base query). getAllByPage()/getAllIdsByPage() never bind it - reproduced + // directly: Doctrine\ORM\Query\QueryException "too few parameters" applying + // presentations_track_id to getAllByPage(). Not a silent no-op, a hard 500. See + // ISpeakerFilterFields::GLOBAL_OPERATORS's docblock for the full field-by-field + // breakdown of what is/isn't summit-independent. + $control = new PresentationSpeaker(); + $control->setFirstName("NoMember"); + $control->setLastName("Control " . str_random(8)); + self::$em->persist($control); + self::$em->flush(); + + $params = [ + 'page' => 1, + 'per_page' => 100, + 'filter' => [ + sprintf('member_id==%s', self::$defaultMember->getId()), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getAll", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(200); + $speakers_response = json_decode($response->getContent()); + $this->assertTrue(!is_null($speakers_response)); + $ids = array_map(fn($s) => $s->id, $speakers_response->data); + + $this->assertContains( + self::$defaultSpeaker->getId(), + $ids, + 'the speaker belonging to the filtered member must be included' + ); + $this->assertNotContains( + $control->getId(), + $ids, + 'a speaker with no member (or a different one) must not match' + ); + } + + public function testGetAllSpeakersRejectsPresentationScopedFilter() + { + // getAll() is not summit-scoped and was never wired to accept presentation-related + // fields (see ISpeakerFilterFields::GLOBAL_OPERATORS's docblock: those fields hard-code + // a :summit bound parameter in the shared repository mapping that a global query can + // never bind). This must stay a clean validation error, not a 500. + $params = [ + 'page' => 1, + 'per_page' => 10, + 'filter' => [ + sprintf('presentations_track_id==%s', self::$defaultTrack->getId()), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getAll", + $params, + [], + [], + [], + $headers + ); + + $this->assertResponseStatus(412); + $content = json_decode($response->getContent(), true); + $this->assertStringContainsString('presentations_track_id', $content['errors'][0]); + } + public function testGetCurrentSummitSpeakersOrderByIDAndFilteredByMediaUploadType() { $media_upload_ids =array_map(function($v){ From c47c1d61bd78f0bc4fad78d8ce2ad50a437b7e3a Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 01:58:49 -0300 Subject: [PATCH 03/12] fix(speakers): dispatch bulk-send chunks through JobDispatcher::withDbFallback Plain ProcessSpeakersEmailRequestJob::dispatch() left the chunk loop exposed to a queue-backend failure part-way through: some chunks already queued, the rest lost with the aborted request, and an operator retry re-emailing the chunks that already went out (should_resend defaults to true and the admin UI never sends it, so re-runs are not deduplicated today). JobDispatcher::withDbFallback tries the primary connection, fails over to the database queue, and runs the chunk synchronously on a double failure - same pattern as PresentationSubmissionReopenService::notify's per-recipient loop. Each iteration additionally wraps the dispatch in its own try/catch so one chunk whose three fallback tiers all failed cannot abort the sibling chunks that would have succeeded - the chunk-isolation property this whole feature exists for. That catch logs at error level: by then the primary, the database fallback, and the synchronous run have all failed, which is an alert-worthy infrastructure event, not a routine warning. Signed-off-by: smarcet --- app/Services/Model/Imp/SpeakerService.php | 25 ++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index da706e147..988cd0d15 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -18,6 +18,7 @@ use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionRejectedEmail; use App\Jobs\Emails\PresentationSubmissions\SpeakerEditPermissionRequestedEmail; use App\Jobs\Emails\ProcessSpeakersEmailRequestJob; +use App\Jobs\Utils\JobDispatcher; use App\Jobs\Emails\Registration\PromoCodes\PromoCodeEmailFactory; use App\Models\Foundation\Main\CountryCodes; use App\Models\Foundation\Main\Repositories\ILanguageRepository; @@ -34,6 +35,7 @@ use App\Services\Model\Strategies\EmailActions\SpeakerActionsEmailStrategy; use App\Services\Model\Strategies\PromoCodes\IPromoCodeStrategyFactory; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; @@ -1263,11 +1265,32 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null return; } + // JobDispatcher, not ::dispatch(): a queue-backend failure part-way through this loop + // would otherwise abort the request with some chunks already queued and the rest lost, + // and an operator retry would re-email the chunks that already went out (should_resend + // defaults to true and the admin UI never sends it, so today's re-runs are not + // deduplicated). withDbFallback() fails over to the database queue (and runs sync on a + // double failure) so the loop completes. Same pattern as + // PresentationSubmissionReopenService::notify's per-recipient dispatch loop. foreach (array_chunk($ids, self::CHUNK_SIZE) as $chunk) { $chunkPayload = $payload; $chunkPayload['speaker_ids'] = $chunk; unset($chunkPayload['excluded_speaker_ids']); - ProcessSpeakersEmailRequestJob::dispatch($summit->getId(), $chunkPayload, $filter); + try { + JobDispatcher::withDbFallback( + job: new ProcessSpeakersEmailRequestJob($summit->getId(), $chunkPayload, $filter), + logContext: ['summit_id' => $summit->getId(), 'speaker_count' => count($chunk)], + primaryConnection: Config::get('queue.default') + ); + } + catch (\Throwable $ex){ + // withDbFallback already exhausted the primary connection, the database + // fallback, and a synchronous run - reaching here means all three failed for + // this chunk. Log at error (not warning) so it surfaces to alerting; keep the + // loop going so a bad chunk doesn't also block every sibling chunk that would + // otherwise succeed. + Log::error($ex); + } } } From 747ac4a05a2e7601d019cce57199c6db57071539 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 02:11:07 -0300 Subject: [PATCH 04/12] test(speakers): cover chunk-failure isolation and the send() member filter at the HTTP layer Addresses the three findings from the changes review: - testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks: forces every Bus dispatch (queued and sync) to throw so all three JobDispatcher fallback tiers fail for every chunk, then asserts the loop still visited every chunk (one-plus Log::error per chunk) instead of aborting on the first. Mutation-verified: moving the per-chunk try/catch outside the foreach fails the count; removing it fails on the propagated exception. - testSendSpeakersBulkEmailFilteredByMemberUserExternalId: drives the real PUT send() action with a member_user_external_id filter, exercising the controller's FilterParser::parse + Filter::validate against the shared ISpeakerFilterFields constants and the service's id resolution end to end - the review noted the member filter was only proven at the service layer, and only for member_id. - Rewords the getAll() code comment so it no longer contains the literal "ISpeakerFilterFields::" substring, making Task 2's documented DoD grep count (8) match what the command actually returns. Signed-off-by: smarcet --- .../OAuth2SummitSpeakersApiController.php | 2 +- tests/SpeakerServiceBulkSendChunkingTest.php | 26 +++++++++++ tests/oauth2/OAuth2SummitSpeakersApiTest.php | 45 +++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php index dc5fc6dcf..d47d7b9a1 100644 --- a/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php +++ b/app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php @@ -781,7 +781,7 @@ public function getAll() return $this->_getAll( function () { // getAll() is summit-independent - only the GLOBAL_* subset is safe here, - // see ISpeakerFilterFields::GLOBAL_OPERATORS for why. + // see GLOBAL_OPERATORS' docblock on the shared filter-fields interface for why. return \services\model\ISpeakerFilterFields::GLOBAL_OPERATORS; }, function () { diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php index 9c8b5988a..92c100447 100644 --- a/tests/SpeakerServiceBulkSendChunkingTest.php +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -287,4 +287,30 @@ public function testMemberIdFilterSelectsOnlyTheMatchingSpeaker(): void 'member_id must select only the speaker belonging to that member, not the default fixture speaker' ); } + + public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): void + { + // Per-chunk failure isolation: JobDispatcher::withDbFallback tries the primary + // connection, the database fallback, and a synchronous run. When ALL THREE fail + // for a chunk, the per-chunk try/catch must log at error level and keep the loop + // going, so a bad chunk cannot also block every sibling chunk. Forcing every Bus + // dispatch to throw makes all 3 chunks fail through all 3 tiers; one Log::error + // per chunk proves the loop reached every chunk instead of aborting on the first. + \Illuminate\Support\Facades\Bus::shouldReceive('dispatch') + ->andThrow(new \RuntimeException('queue backend down')); + \Illuminate\Support\Facades\Bus::shouldReceive('dispatchSync') + ->andThrow(new \RuntimeException('sync run failed')); + \Illuminate\Support\Facades\Log::spy(); + + $payload = $this->basePayload(); + $payload['speaker_ids'] = range(1, 250); // 3 chunks + + $this->service()->triggerSendEmails(self::$summit, $payload, null); + + // At least one Log::error per chunk (JobDispatcher logs its own on the database + // fallback failing, plus the per-chunk catch). If the try/catch moved outside the + // loop, only the first chunk would ever be attempted (< 3 errors); if the catch + // were removed, the exception would propagate and fail this test outright. + \Illuminate\Support\Facades\Log::shouldHaveReceived('error')->atLeast()->times(3); + } } diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index c657428da..d4c67a226 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -895,6 +895,51 @@ public function testExportCurrentSummitSpeakersWithAcceptedPresentations() $this->assertTrue(!empty($csv)); } + public function testSendSpeakersBulkEmailFilteredByMemberUserExternalId() { + // member_user_external_id was previously rejected by send()'s own whitelist (it only + // existed on the listing endpoints). This drives the real HTTP action, so it exercises + // the controller's FilterParser::parse + Filter::validate against the shared + // ISpeakerFilterFields constants AND the service's id resolution, end to end. + Queue::fake(); + + $params = [ + 'id' => self::$summit->getId(), + 'filter' => [ + 'member_user_external_id==' . self::$member->getUserExternalId(), + ], + ]; + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json" + ]; + + $data = [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + ]; + + $response = $this->action( + "PUT", + "OAuth2SummitSpeakersApiController@send", + $params, + [], + [], + [], + $headers, + json_encode($data) + ); + + $this->assertResponseStatus(200); + + Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, function ($job) { + $ref = new \ReflectionObject($job); + $prop = $ref->getProperty('payload'); + $prop->setAccessible(true); + $payload = $prop->getValue($job); + return in_array(self::$defaultSpeaker->getId(), $payload['speaker_ids'] ?? []); + }); + } + public function testSendSpeakersBulkEmail() { $params = [ 'id' => self::$summit->getId(), From af3c2d6f71b14da0cc09c96359fedf0b9a7b105b Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 11:34:57 -0300 Subject: [PATCH 05/12] fix(speakers): report a lost bulk-send chunk from ProcessSpeakersEmailRequestJob::failed() With tries = 1, a chunk whose worker is killed mid-run sits reserved until the connection's retry_after elapses, is re-served, and is marked failed without re-running. Nothing reported that loss: the outcome excerpt is only sent when sendEmails() runs to completion, so a dead chunk left no trace beyond a queue_failed_jobs row - the same silence as the 2026-08-31 incident, capped at 100 speakers instead of the whole run. The new failed() hook (invoked by Job::fail() -> CallQueuedHandler::failed(), including the sync tier of JobDispatcher::withDbFallback) logs the summit, flow event, exception, unprocessed speaker ids and raw filter at error level, and when the payload carries outcome_email_recipient dispatches PresentationSpeakerSelectionProcessExcerptEmail with an ERROR line naming the ids and the cause, so the operator can re-send that chunk by id. The excerpt dispatch is best-effort inside a try/catch so it never masks the original failure; without a recipient the hook only logs and never touches the database. tests/ProcessSpeakersEmailRequestJobFailedHookTest.php invokes the hook directly (Queue::fake stops at dispatch, so the framework plumbing cannot be driven end to end here) and pins both paths. Mutation-verified: dropping the recipient guard fails the "exactly one error line" expectation, dropping the ids from the ERROR line fails the id assertion. Named apart from ProcessSpeakersEmailRequestJobTest.php, which exists on another branch. Signed-off-by: smarcet --- .../ProcessSpeakersEmailRequestJob.php | 78 +++++++++++ ...sSpeakersEmailRequestJobFailedHookTest.php | 122 ++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 tests/ProcessSpeakersEmailRequestJobFailedHookTest.php diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index e3bcf9c26..fb0123829 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -11,13 +11,17 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessExcerptEmail; +use App\Services\utils\IEmailExcerptService; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Log; use models\summit\ISummitRepository; +use models\summit\Summit; use services\model\ISpeakerService; use utils\FilterParser; /** @@ -83,4 +87,78 @@ public function handle $service->sendEmails($this->summit_id, $this->payload, $filter); } + + /** + * Invoked by the queue worker once this job is marked failed. With tries = 1 that includes a + * chunk whose worker was killed mid-run: the job sits reserved until the connection's + * retry_after elapses, is re-served, and is failed without re-running. Nothing else reports + * that loss - the outcome excerpt is only sent when sendEmails() runs to completion - so + * without this hook a dead chunk leaves no trace beyond a queue_failed_jobs row. + * + * Log the unprocessed speaker ids at error, and when the operator asked for an outcome + * e-mail send one naming them, so the chunk can be re-sent by id. The excerpt dispatch is + * best-effort: a failure there must not mask the original failure. + * + * @param \Throwable $e + */ + public function failed(\Throwable $e): void + { + $speaker_ids = $this->payload['speaker_ids'] ?? []; + $flow_event = $this->payload['email_flow_event'] ?? ''; + $ids_list = implode(', ', $speaker_ids); + + Log::error + ( + sprintf + ( + "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) NOT processed (%s: %s). Unprocessed speaker ids: [%s] filter %s", + $this->summit_id, + $flow_event, + count($speaker_ids), + get_class($e), + $e->getMessage(), + $ids_list, + json_encode($this->filter) + ) + ); + + $outcome_email_recipient = $this->payload['outcome_email_recipient'] ?? null; + if (empty($outcome_email_recipient)) return; + + try { + $summit = App::make(ISummitRepository::class)->getById($this->summit_id); + if (!$summit instanceof Summit) { + Log::warning(sprintf("ProcessSpeakersEmailRequestJob::failed summit %s not found, outcome excerpt not sent", $this->summit_id)); + return; + } + + // Same line types AbstractExcerptEmailJob renders for a completed run, so the + // operator's inbox reads the same either way. + $report = [ + [ + 'type' => IEmailExcerptService::InfoType, + 'message' => sprintf("Processing EMAIL %s for Summit %s", $flow_event, $this->summit_id), + ], + [ + 'type' => IEmailExcerptService::ErrorType, + 'message' => sprintf + ( + "Chunk of %s speaker(s) was NOT processed because the job failed (%s). Unprocessed speaker ids: %s", + count($speaker_ids), + $e->getMessage(), + $ids_list + ), + ], + [ + 'type' => IEmailExcerptService::InfoType, + 'message' => "TOTAL of 0 speaker(s) processed", + ], + ]; + + PresentationSpeakerSelectionProcessExcerptEmail::dispatch($summit, $outcome_email_recipient, $report); + } + catch (\Throwable $ex) { + Log::error($ex); + } + } } diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php new file mode 100644 index 000000000..172a2dd94 --- /dev/null +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -0,0 +1,122 @@ + failed() plumbing can't be driven end to end without a real queue + * connection. That plumbing is Laravel's (Illuminate\Queue\Jobs\Job::fail() -> + * CallQueuedHandler::failed() -> $command->failed($e)); what this class pins is what OUR hook does. + * + * Class ProcessSpeakersEmailRequestJobFailedHookTest + */ +class ProcessSpeakersEmailRequestJobFailedHookTest extends ProtectedApiTestCase +{ + use InsertSummitTestData; + + protected function setUp(): void + { + parent::setUp(); + self::insertSummitTestData(); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + parent::tearDown(); + } + + private function jobProperty(object $job, string $name) + { + $reflection = new ReflectionObject($job); + while ($reflection && !$reflection->hasProperty($name)) { + $reflection = $reflection->getParentClass(); + } + $property = $reflection->getProperty($name); + $property->setAccessible(true); + return $property->getValue($job); + } + + public function testFailedChunkWithOutcomeRecipientSendsExcerptNamingTheUnprocessedIds(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22, 33], + 'outcome_email_recipient' => 'outcome@example.com', + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::class, 1); + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::class, function ($excerpt) { + $this->assertEquals('outcome@example.com', $this->jobProperty($excerpt, 'to_email')); + + $lines = $this->jobProperty($excerpt, 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + + $this->assertCount(1, $errorLines, 'the excerpt must carry exactly one ERROR line for the lost chunk'); + $this->assertStringContainsString('11, 22, 33', $errorLines[0], 'the ERROR line must name every unprocessed speaker id'); + $this->assertStringContainsString('worker killed mid-chunk', $errorLines[0], 'the ERROR line must carry the failure reason'); + $this->assertEmpty( + array_filter($lines, fn($l) => str_starts_with($l, 'Email type')), + 'a failed chunk must not report any e-mail as sent' + ); + return true; + }); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, (string) self::$summit->getId()) + && str_contains($message, '11, 22, 33')) + ->once(); + } + + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void + { + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [44, 55], + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertNothingPushed(); + + // Exactly one error line, and it names the ids. The "exactly one" half is what + // distinguishes "skipped the excerpt on purpose" from "tried to build it without a + // recipient and swallowed the resulting exception into a second error entry". + Log::shouldHaveReceived('error')->once(); + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) && str_contains($message, '44, 55')) + ->once(); + } +} From bcbcffaec1a858ac771e2c5152ce50a78d4c63d3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 12:09:00 -0300 Subject: [PATCH 06/12] fix(speakers): add chunk context to the all-tiers-failed log and prove send() narrowing with a control speaker Two review-thread follow-ups on #595: - SpeakerService::triggerSendEmails's per-chunk catch logged only the Throwable. JobDispatcher::withDbFallback already logs the first two tiers with summit_id / speaker_count and, since 126616df0, the sync tier's failure runs ProcessSpeakersEmailRequestJob::failed() before this catch fires, but the last line should stand on its own: it now names the summit, the chunk size, the exception and the unprocessed speaker ids, with summit_id / speaker_ids / exception in the context array. - testSendSpeakersBulkEmailFilteredByMemberUserExternalId asserted only that the filtered member's speaker was in the dispatched chunk. Exact equality alone would not have proven narrowing either - the fixture summit has a single speaker with presentations - so the test now seeds a control speaker (different member, with a presentation in the summit) and requires the chunk to equal exactly [defaultSpeaker]. Mutation-verified: a filter matching every speaker (not_id==0) fails it on the control speaker. Signed-off-by: smarcet --- app/Services/Model/Imp/SpeakerService.php | 14 +++++- tests/oauth2/OAuth2SummitSpeakersApiTest.php | 47 +++++++++++++++++++- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index 988cd0d15..d6b32c427 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -1289,7 +1289,19 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null // this chunk. Log at error (not warning) so it surfaces to alerting; keep the // loop going so a bad chunk doesn't also block every sibling chunk that would // otherwise succeed. - Log::error($ex); + Log::error + ( + sprintf + ( + "SpeakerService::triggerSendEmails summit %s: chunk of %s speaker(s) failed every dispatch tier (%s: %s). Unprocessed speaker ids: [%s]", + $summit->getId(), + count($chunk), + get_class($ex), + $ex->getMessage(), + implode(', ', $chunk) + ), + ['summit_id' => $summit->getId(), 'speaker_ids' => $chunk, 'exception' => $ex] + ); } } } diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index d4c67a226..90f1a130d 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -900,8 +900,42 @@ public function testSendSpeakersBulkEmailFilteredByMemberUserExternalId() { // existed on the listing endpoints). This drives the real HTTP action, so it exercises // the controller's FilterParser::parse + Filter::validate against the shared // ISpeakerFilterFields constants AND the service's id resolution, end to end. + // + // The fixture summit has a single speaker with presentations (self::$defaultSpeaker), + // so on its own an exact-equality assertion would pass for the same reason an + // in_array one does. Seed a control speaker - different member, with a presentation + // in this summit - so "exactly [defaultSpeaker]" actually proves the filter narrowed. Queue::fake(); + $prefix = str_random(10); + $control_member = new Member(); + $control_member->setEmail("send-control+{$prefix}@test.com"); + $control_member->setActive(true); + $control_member->setFirstName("Control"); + $control_member->setLastName("Member"); + $control_member->setEmailVerified(true); + $control_member->setUserExternalId(mt_rand()); + self::$em->persist($control_member); + + $control_speaker = new PresentationSpeaker(); + $control_speaker->setFirstName("Control"); + $control_speaker->setLastName("Speaker {$prefix}"); + $control_speaker->setMember($control_member); + self::$em->persist($control_speaker); + + $control_presentation = new Presentation(); + self::$summit->addEvent($control_presentation); + $control_presentation->setTitle("Send control presentation {$prefix}"); + $control_presentation->setAbstract("Abstract {$prefix}"); + $control_presentation->setCategory(self::$defaultTrack); + $control_presentation->setType(self::$defaultPresentationType); + $control_presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $control_presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + $control_presentation->addSpeaker($control_speaker); + self::$em->persist($control_presentation); + + self::$em->flush(); + $params = [ 'id' => self::$summit->getId(), 'filter' => [ @@ -931,12 +965,21 @@ public function testSendSpeakersBulkEmailFilteredByMemberUserExternalId() { $this->assertResponseStatus(200); - Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, function ($job) { + Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, 1); + Queue::assertPushed(\App\Jobs\Emails\ProcessSpeakersEmailRequestJob::class, function ($job) use ($control_speaker) { $ref = new \ReflectionObject($job); $prop = $ref->getProperty('payload'); $prop->setAccessible(true); $payload = $prop->getValue($job); - return in_array(self::$defaultSpeaker->getId(), $payload['speaker_ids'] ?? []); + $this->assertEquals( + [self::$defaultSpeaker->getId()], + $payload['speaker_ids'] ?? [], + sprintf( + 'the chunk must contain exactly the speaker of the filtered member, not the control speaker %s', + $control_speaker->getId() + ) + ); + return true; }); } From 0a4a56da24c355ace8c073619a6061513dec4b39 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 13:48:04 -0300 Subject: [PATCH 07/12] fix(speakers): stop reporting a failed chunk as fully unprocessed and cover multi-page id resolution ProcessSpeakersEmailRequestJob::failed() said "chunk of N speaker(s) NOT processed. Unprocessed speaker ids: [...]" for every id in the chunk. The chunk is processed one speaker per transaction, so the hook's own trigger case (a worker killed mid-run) has already e-mailed and written the "already sent" proof for the speakers before the kill. An operator re-sending that list from summit-admin, which never sends should_resend, would mail those speakers twice (the DTO defaults should_resend to true). The log line and the excerpt ERROR line now say up to N of them may not have been processed, name the ids as the chunk's ids, and tell the operator to re-send with should_resend=false so the resend guard skips the ones with a proof. When the send carries a promo_code_spec the same line warns that a re-send creates a new code for every speaker in the list, because AutomaticMultiSpeakerPromoCodeStrategy generates a fresh code before the resend guard runs. The INFO line no longer claims 0 processed. Add a chunking test for the filter-based path across a page boundary: CHUNK_SIZE + 1 seeded speakers behind a first_name filter must resolve into exactly two chunks (100 + 1) covering every seeded id once, with the fixture speaker left out. This is the only path summit-admin drives and nothing exercised the do/while beyond a single page. Mutation-verified: dropping the array_merge of the pages fails it. --- .../ProcessSpeakersEmailRequestJob.php | 31 ++++++++--- ...sSpeakersEmailRequestJobFailedHookTest.php | 38 ++++++++++++- tests/SpeakerServiceBulkSendChunkingTest.php | 53 +++++++++++++++++++ 3 files changed, 112 insertions(+), 10 deletions(-) diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index fb0123829..93331b214 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -95,9 +95,13 @@ public function handle * that loss - the outcome excerpt is only sent when sendEmails() runs to completion - so * without this hook a dead chunk leaves no trace beyond a queue_failed_jobs row. * - * Log the unprocessed speaker ids at error, and when the operator asked for an outcome - * e-mail send one naming them, so the chunk can be re-sent by id. The excerpt dispatch is - * best-effort: a failure there must not mask the original failure. + * Log the chunk's speaker ids at error, and when the operator asked for an outcome e-mail + * send one naming them, so the chunk can be re-sent by id. The chunk is processed one speaker + * per transaction, so a worker killed mid-run has already e-mailed (and written the "already + * sent" proof for) the speakers before the kill: the ids are an upper bound on what was lost, + * not confirmed misses, and both messages say so and tell the operator to re-send with + * should_resend=false so the resend guard skips the speakers that already have a proof. The + * excerpt dispatch is best-effort: a failure there must not mask the original failure. * * @param \Throwable $e */ @@ -107,18 +111,27 @@ public function failed(\Throwable $e): void $flow_event = $this->payload['email_flow_event'] ?? ''; $ids_list = implode(', ', $speaker_ids); + $resend_hint = "Re-send these ids with should_resend=false so the speakers already e-mailed are skipped"; + if (isset($this->payload['promo_code_spec'])) { + // AutomaticMultiSpeakerPromoCodeStrategy::getPromoCode() generates a fresh code on every + // call, before the resend guard runs, so should_resend=false does not prevent this one. + $resend_hint .= "; this send auto-generates promo codes and a re-send creates a new code for every speaker in the list, including the ones already e-mailed"; + } + Log::error ( sprintf ( - "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) NOT processed (%s: %s). Unprocessed speaker ids: [%s] filter %s", + "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) failed (%s: %s); up to %s of them may not have been processed. Speaker ids in the chunk: [%s] filter %s. %s.", $this->summit_id, $flow_event, count($speaker_ids), get_class($e), $e->getMessage(), + count($speaker_ids), $ids_list, - json_encode($this->filter) + json_encode($this->filter), + $resend_hint ) ); @@ -143,15 +156,17 @@ public function failed(\Throwable $e): void 'type' => IEmailExcerptService::ErrorType, 'message' => sprintf ( - "Chunk of %s speaker(s) was NOT processed because the job failed (%s). Unprocessed speaker ids: %s", + "Chunk of %s speaker(s) failed (%s); up to %s of them may not have been processed. Speaker ids in the chunk: %s. %s", count($speaker_ids), $e->getMessage(), - $ids_list + count($speaker_ids), + $ids_list, + $resend_hint ), ], [ 'type' => IEmailExcerptService::InfoType, - 'message' => "TOTAL of 0 speaker(s) processed", + 'message' => "TOTAL processed for this chunk is unknown, the job did not run to completion", ], ]; diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index 172a2dd94..caabec6b0 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -81,8 +81,14 @@ public function testFailedChunkWithOutcomeRecipientSendsExcerptNamingTheUnproces $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); $this->assertCount(1, $errorLines, 'the excerpt must carry exactly one ERROR line for the lost chunk'); - $this->assertStringContainsString('11, 22, 33', $errorLines[0], 'the ERROR line must name every unprocessed speaker id'); + $this->assertStringContainsString('11, 22, 33', $errorLines[0], 'the ERROR line must name every speaker id in the chunk'); $this->assertStringContainsString('worker killed mid-chunk', $errorLines[0], 'the ERROR line must carry the failure reason'); + // The chunk runs one speaker per transaction, so a mid-run kill has already e-mailed + // part of it: the line must not present the ids as confirmed misses, and it must tell + // the operator how to re-send without duplicating those. + $this->assertStringContainsString('up to 3 of them may not have been processed', $errorLines[0]); + $this->assertStringContainsString('should_resend=false', $errorLines[0], 'the ERROR line must tell the operator to re-send with should_resend=false'); + $this->assertStringNotContainsString('promo code', $errorLines[0], 'no promo-code caveat when the send carries no promo_code_spec'); $this->assertEmpty( array_filter($lines, fn($l) => str_starts_with($l, 'Email type')), 'a failed chunk must not report any e-mail as sent' @@ -93,10 +99,38 @@ public function testFailedChunkWithOutcomeRecipientSendsExcerptNamingTheUnproces Log::shouldHaveReceived('error') ->withArgs(fn($message) => is_string($message) && str_contains($message, (string) self::$summit->getId()) - && str_contains($message, '11, 22, 33')) + && str_contains($message, '11, 22, 33') + && str_contains($message, 'should_resend=false')) ->once(); } + public function testFailedChunkWithPromoCodeSpecWarnsThatAResendCreatesNewCodes(): void + { + // AutomaticMultiSpeakerPromoCodeStrategy::getPromoCode() generates a fresh code on every + // call and runs before the resend guard, so should_resend=false does not stop a re-send + // from handing a second code to the speakers already e-mailed. The operator has to know. + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22], + 'outcome_email_recipient' => 'outcome@example.com', + 'promo_code_spec' => ['class_name' => 'SPEAKERS_PROMO_CODE', 'type' => 'ACCEPTED'], + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Queue::assertPushed(PresentationSpeakerSelectionProcessExcerptEmail::class, function ($excerpt) { + $lines = $this->jobProperty($excerpt, 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + $this->assertCount(1, $errorLines); + $this->assertStringContainsString('should_resend=false', $errorLines[0]); + $this->assertStringContainsString('auto-generates promo codes', $errorLines[0], 'a promo_code_spec send must warn that a re-send creates new codes'); + return true; + }); + } + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void { Queue::fake(); diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php index 92c100447..5c1c266b2 100644 --- a/tests/SpeakerServiceBulkSendChunkingTest.php +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -19,6 +19,7 @@ use models\summit\PresentationSpeaker; use ReflectionObject; use services\model\ISpeakerService; +use services\model\SpeakerService; /** * Covers SpeakerService::triggerSendEmails's chunking behaviour: it resolves the full set of @@ -288,6 +289,58 @@ public function testMemberIdFilterSelectsOnlyTheMatchingSpeaker(): void ); } + public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedIdExactlyOnce(): void + { + // The filter-based path is the only one summit-admin drives (it always sends filter[], + // never speaker_ids), and it resolves ids by paging getSpeakersIdsBySummit CHUNK_SIZE at + // a time. Seed CHUNK_SIZE + 1 speakers sharing a unique first name on one presentation of + // this summit, so the resolution has to walk two non-empty pages plus the empty one that + // ends the loop. self::$defaultSpeaker (with its own presentations) is the control the + // first_name filter must leave out. A page that is skipped, re-read, or overwritten + // instead of merged breaks the exact-set assertion below. + Queue::fake(); + + $firstName = 'PageSeed' . str_random(8); + + $presentation = new \models\summit\Presentation(); + self::$summit->addEvent($presentation); + $presentation->setTitle("Multi-page chunk test {$firstName}"); + $presentation->setAbstract("Abstract {$firstName}"); + $presentation->setCategory(self::$defaultTrack); + $presentation->setType(self::$defaultPresentationType); + $presentation->setStartDate(new \DateTime('now', new \DateTimeZone('UTC'))); + $presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); + + $seeded = []; + for ($i = 0; $i < SpeakerService::CHUNK_SIZE + 1; $i++) { + $speaker = new PresentationSpeaker(); + $speaker->setFirstName($firstName); + $speaker->setLastName("Speaker {$i}"); + self::$em->persist($speaker); + $presentation->addSpeaker($speaker); + $seeded[] = $speaker; + } + self::$em->persist($presentation); + self::$em->flush(); + + $seededIds = array_map(fn($s) => $s->getId(), $seeded); + sort($seededIds); + + $this->service()->triggerSendEmails(self::$summit, $this->basePayload(), ["first_name=={$firstName}"]); + + Queue::assertPushed(ProcessSpeakersEmailRequestJob::class, 2); + + $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $this->pushedJobs()); + $sizes = array_map('count', $slices); + rsort($sizes); + $this->assertEquals([SpeakerService::CHUNK_SIZE, 1], $sizes, 'CHUNK_SIZE + 1 matched ids must become one full chunk and one single-id chunk'); + + $covered = array_merge(...$slices); + sort($covered); + $this->assertEquals($seededIds, $covered, 'the chunks together must cover every seeded id exactly once, across both pages'); + $this->assertNotContains(self::$defaultSpeaker->getId(), $covered, 'the first_name filter must leave the control speaker out'); + } + public function testOneChunkFailingAllFallbackTiersDoesNotAbortSiblingChunks(): void { // Per-chunk failure isolation: JobDispatcher::withDbFallback tries the primary From fb28d3e704fdb2000547b83cbf4f556a10a61d67 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 15:50:49 -0300 Subject: [PATCH 08/12] fix(queue): give the database fallback connection a retry_after matching redis The database connection only carried the dead Laravel-4 'expire' key, so Laravel applied its 60 s retry_after default. A ProcessSpeakersEmailRequestJob chunk that failed over to that tier (JobDispatcher::withDbFallback) and ran longer than 60 s was re-served by a sibling worker-db-fallback replica, failed on tries = 1, and the failed() hook reported a false chunk loss while the original run was still completing. Align it with the redis primary (1800, DB_QUEUE_RETRY_AFTER) and drop the unused key. --- .env.example | 1 + config/queue.php | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.env.example b/.env.example index 8d4fa1b52..5e3de2850 100644 --- a/.env.example +++ b/.env.example @@ -34,6 +34,7 @@ SESSION_COOKIE_SECURE=false QUEUE_DRIVER=database QUEUE_CONN= QUEUE_DATABASE= +DB_QUEUE_RETRY_AFTER=1800 MAIL_DRIVER=sendgrid SENDGRID_API_KEY='YOUR_SENDGRID_API_KEY' diff --git a/config/queue.php b/config/queue.php index 6a175b9bb..abbe0b127 100644 --- a/config/queue.php +++ b/config/queue.php @@ -43,7 +43,10 @@ 'driver' => 'database', 'table' => 'queue_jobs', 'queue' => 'default', - 'expire' => 60, + // Same window as the redis primary: this is the failover tier for the same jobs + // (JobDispatcher::withDbFallback), so a reserved job must not be re-served before + // a chunk-sized job can finish. Laravel 12 ignores the old 'expire' key. + 'retry_after' => (int) env('DB_QUEUE_RETRY_AFTER', 1800), ], 'redis' => [ From 23d1eec0b99aac7fe62540d0a757cd666443f87e Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 16:19:22 -0300 Subject: [PATCH 09/12] fix(speakers): route the failed-chunk excerpt through JobDispatcher::withDbFallback ProcessSpeakersEmailRequestJob::failed() dispatched the lost-chunk excerpt with a bare ::dispatch() on the default connection. A chunk runs on the database fallback worker precisely when the redis primary was down at dispatch time, so if it then failed while redis was still down the excerpt push threw, the best-effort catch swallowed it, and the operator report was lost in the one scenario it exists for. Dispatch it through JobDispatcher::withDbFallback with primaryConnection following queue.default, the same route the chunk itself takes in SpeakerService::triggerSendEmails. The try/catch stays so an excerpt failure never masks the original one. Adds a test that scripts the primary dispatch to throw and asserts the excerpt is re-dispatched on the database connection with the chunk's speaker ids; it fails against the bare dispatch (nothing captured) and passes with the fallback. --- .../ProcessSpeakersEmailRequestJob.php | 15 ++++++- ...sSpeakersEmailRequestJobFailedHookTest.php | 41 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index 93331b214..d0b9b1dfa 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -12,6 +12,7 @@ * limitations under the License. **/ use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessExcerptEmail; +use App\Jobs\Utils\JobDispatcher; use App\Services\utils\IEmailExcerptService; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -19,6 +20,7 @@ use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; use models\summit\ISummitRepository; use models\summit\Summit; @@ -101,7 +103,8 @@ public function handle * sent" proof for) the speakers before the kill: the ids are an upper bound on what was lost, * not confirmed misses, and both messages say so and tell the operator to re-send with * should_resend=false so the resend guard skips the speakers that already have a proof. The - * excerpt dispatch is best-effort: a failure there must not mask the original failure. + * excerpt goes through JobDispatcher::withDbFallback (primary, then the database queue, then + * an inline run) and is best-effort: a failure there must not mask the original failure. * * @param \Throwable $e */ @@ -170,7 +173,15 @@ public function failed(\Throwable $e): void ], ]; - PresentationSpeakerSelectionProcessExcerptEmail::dispatch($summit, $outcome_email_recipient, $report); + // Same failover route as the chunk itself (SpeakerService::triggerSendEmails): a chunk + // runs on the database fallback worker precisely when the redis primary was down at + // dispatch time, so a bare ::dispatch() here would throw into the catch below and lose + // the report in the one scenario it exists for. + JobDispatcher::withDbFallback( + job: new PresentationSpeakerSelectionProcessExcerptEmail($summit, $outcome_email_recipient, $report), + logContext: ['summit_id' => $this->summit_id, 'speaker_count' => count($speaker_ids)], + primaryConnection: Config::get('queue.default') + ); } catch (\Throwable $ex) { Log::error($ex); diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index caabec6b0..5dc80afd5 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -15,8 +15,10 @@ use App\Jobs\Emails\IMailTemplatesConstants; use App\Jobs\Emails\PresentationSubmissions\SelectionProcess\PresentationSpeakerSelectionProcessExcerptEmail; use App\Jobs\Emails\ProcessSpeakersEmailRequestJob; +use Illuminate\Support\Facades\Bus; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; +use Mockery; use ReflectionObject; /** @@ -131,6 +133,45 @@ public function testFailedChunkWithPromoCodeSpecWarnsThatAResendCreatesNewCodes( }); } + public function testFailedChunkExcerptFailsOverToTheDatabaseQueueWhenThePrimaryDispatchFails(): void + { + // A chunk lands on the database fallback worker precisely when the redis primary was down + // at dispatch time. If that chunk then fails while redis is still down, a bare ::dispatch() + // of the excerpt throws, the best-effort catch swallows it, and the operator report is lost + // in the one scenario it exists for. The excerpt must take the same failover route as the + // chunk itself (JobDispatcher::withDbFallback): primary throws, database gets the job. + Log::spy(); + $captured = []; + $excerpt = Mockery::type(PresentationSpeakerSelectionProcessExcerptEmail::class); + // Only the excerpt dispatches are scripted (primary throws, fallback captures); every + // other dispatch - the fixture teardown queues hundreds - goes to the real dispatcher. + $realBus = Bus::getFacadeRoot(); + Bus::shouldReceive('dispatch')->with($excerpt)->once()->andThrow(new \RuntimeException('redis down')); + Bus::shouldReceive('dispatch')->with($excerpt)->once()->andReturnUsing(function ($job) use (&$captured) { + $captured[] = $job; + return null; + }); + Bus::shouldReceive('dispatch')->andReturnUsing(fn($job) => $realBus->dispatch($job)); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22], + 'outcome_email_recipient' => 'outcome@example.com', + ], null); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + $this->assertCount(1, $captured, 'after the primary dispatch fails the excerpt must be re-dispatched on the fallback connection, not swallowed'); + $this->assertInstanceOf(PresentationSpeakerSelectionProcessExcerptEmail::class, $captured[0]); + $this->assertSame('database', $captured[0]->connection, 'the retry must target the database fallback connection'); + $this->assertEquals('outcome@example.com', $this->jobProperty($captured[0], 'to_email')); + + $lines = $this->jobProperty($captured[0], 'payload')[IMailTemplatesConstants::report]; + $errorLines = array_values(array_filter($lines, fn($l) => str_starts_with($l, 'ERROR'))); + $this->assertCount(1, $errorLines); + $this->assertStringContainsString('11, 22', $errorLines[0], 'the failed-over excerpt must still name every speaker id in the chunk'); + } + public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds(): void { Queue::fake(); From 6837136f1b80c9d33ccb229d738a6f92516898d2 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 2 Sep 2026 22:39:48 -0300 Subject: [PATCH 10/12] chore(config): add chunk sizes to config chore(debug): add log info --- app/Services/Model/Imp/SpeakerService.php | 38 ++++++++++++++++++++--- config/emails.php | 20 ++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 config/emails.php diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index d6b32c427..de07538d7 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -1230,14 +1230,26 @@ public function deleteSpeakerBigPhoto($speaker_id): void }); } - const CHUNK_SIZE = 100; /** * @inheritDoc */ public function triggerSendEmails(Summit $summit, array $payload, $filter = null): void { - Log::debug(sprintf("SpeakerService::triggerSendEmails summit %s payload %s", $summit->getId(), json_encode($payload))); + $process_db_chunk_size = intval(Config::get('emails.speakers_process_db_chunk_size', 500)); + $process_jon_chunk_size = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + + Log::debug + ( + sprintf + ( + "SpeakerService::triggerSendEmails summit %s payload %s process_db_chunk_size %s process_jon_chunk_size %s", + $summit->getId(), + json_encode($payload), + $process_db_chunk_size, + $process_jon_chunk_size + ) + ); if (isset($payload['speaker_ids'])) { $ids = $payload['speaker_ids']; @@ -1246,8 +1258,8 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null $ids = []; $page = 1; do { - $currentPage = $this->tx_service->transaction(function () use ($summit, $page, $parsedFilter) { - return $this->speaker_repository->getSpeakersIdsBySummit($summit, new PagingInfo($page, self::CHUNK_SIZE), $parsedFilter); + $currentPage = $this->tx_service->transaction(function () use ($summit, $page, $parsedFilter, $process_db_chunk_size) { + return $this->speaker_repository->getSpeakersIdsBySummit($summit, new PagingInfo($page, $process_db_chunk_size), $parsedFilter); }); $ids = array_merge($ids, $currentPage); $page++; @@ -1265,6 +1277,8 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null return; } + Log::debug(sprintf("SpeakerService::triggerSendEmail got %s speakers to process", count($ids))); + // JobDispatcher, not ::dispatch(): a queue-backend failure part-way through this loop // would otherwise abort the request with some chunks already queued and the rest lost, // and an operator retry would re-email the chunks that already went out (should_resend @@ -1272,16 +1286,30 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null // deduplicated). withDbFallback() fails over to the database queue (and runs sync on a // double failure) so the loop completes. Same pattern as // PresentationSubmissionReopenService::notify's per-recipient dispatch loop. - foreach (array_chunk($ids, self::CHUNK_SIZE) as $chunk) { + $chunk_nbr = 1; + foreach (array_chunk($ids, $process_jon_chunk_size) as $chunk) { $chunkPayload = $payload; $chunkPayload['speaker_ids'] = $chunk; unset($chunkPayload['excluded_speaker_ids']); + + Log::debug + ( + sprintf + ( + "SpeakerService::triggerSendEmails sending summit id %s chunk %s speakers count %s", + $summit->getId(), + $chunk_nbr, + count($chunk) + ) + ); + try { JobDispatcher::withDbFallback( job: new ProcessSpeakersEmailRequestJob($summit->getId(), $chunkPayload, $filter), logContext: ['summit_id' => $summit->getId(), 'speaker_count' => count($chunk)], primaryConnection: Config::get('queue.default') ); + $chunk_nbr++; } catch (\Throwable $ex){ // withDbFallback already exhausted the primary connection, the database diff --git a/config/emails.php b/config/emails.php new file mode 100644 index 000000000..5db39bb72 --- /dev/null +++ b/config/emails.php @@ -0,0 +1,20 @@ + env('EMAILS_SPEAKERS_PROCESS_DB_CHUNK', 500), + // size of the chunk of job processing for speakers email + 'speakers_process_job_chunk_size' => env('EMAILS_SPEAKERS_PROCESS_JOB_CHUNK', 200) +]; From ae32f90f32707f25c8e6b27010c186a0949c4d51 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 3 Sep 2026 01:27:21 -0300 Subject: [PATCH 11/12] test(speakers): fix chunk-size assumptions in bulk send chunking tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpeakerService::CHUNK_SIZE does not exist on the class; two other tests in the same file separately hardcoded a 100-based chunk size. All three now read emails.speakers_process_job_chunk_size (default 200), the same value triggerSendEmails() actually chunks by. Verified against a real dev send of 661 speakers: the app log's chunk sizes (200/200/200/61) matched the summit-admin CSV export exactly. CodeRabbit also proposed lowering the config default from 200 to 100; rejected — 200 is the value actually in effect and verified correct. --- tests/SpeakerServiceBulkSendChunkingTest.php | 37 +++++++++++--------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/tests/SpeakerServiceBulkSendChunkingTest.php b/tests/SpeakerServiceBulkSendChunkingTest.php index 5c1c266b2..77ee73a25 100644 --- a/tests/SpeakerServiceBulkSendChunkingTest.php +++ b/tests/SpeakerServiceBulkSendChunkingTest.php @@ -14,17 +14,17 @@ use App\Jobs\Emails\ProcessSpeakersEmailRequestJob; use Illuminate\Support\Facades\App; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Queue; use models\main\Member; use models\summit\PresentationSpeaker; use ReflectionObject; use services\model\ISpeakerService; -use services\model\SpeakerService; /** * Covers SpeakerService::triggerSendEmails's chunking behaviour: it resolves the full set of * matched speaker ids synchronously and dispatches one ProcessSpeakersEmailRequestJob per - * SpeakerService::CHUNK_SIZE-sized group, instead of a single job the trait pages through. + * emails.speakers_process_job_chunk_size-sized group, instead of a single job the trait pages through. * * Most cases use an explicit speaker_ids payload with fabricated ids rather than real, seeded * speakers: Queue::fake() intercepts dispatch before the job's handle() ever runs, and the @@ -81,10 +81,12 @@ private function jobProperty(ProcessSpeakersEmailRequestJob $job, string $name) return $property->getValue($job); } - public function testDispatchesOneChunkPerHundredIdsWithNoOverlap(): void + public function testDispatchesOneChunkPerConfiguredSizeWithNoOverlap(): void { Queue::fake(); - $ids = range(1, 250); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + $remainder = 50; + $ids = range(1, 2 * $chunkSize + $remainder); $payload = $this->basePayload(); $payload['speaker_ids'] = $ids; @@ -95,9 +97,9 @@ public function testDispatchesOneChunkPerHundredIdsWithNoOverlap(): void $jobs = $this->pushedJobs(); $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $jobs); - $this->assertCount(100, $slices[0]); - $this->assertCount(100, $slices[1]); - $this->assertCount(50, $slices[2]); + $this->assertCount($chunkSize, $slices[0]); + $this->assertCount($chunkSize, $slices[1]); + $this->assertCount($remainder, $slices[2]); $covered = array_merge(...$slices); sort($covered); @@ -107,7 +109,8 @@ public function testDispatchesOneChunkPerHundredIdsWithNoOverlap(): void public function testDispatchesExactlyOneJobWhenMatchedCountEqualsChunkSize(): void { Queue::fake(); - $ids = range(1, 100); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + $ids = range(1, $chunkSize); $payload = $this->basePayload(); $payload['speaker_ids'] = $ids; @@ -168,8 +171,9 @@ public function testDuplicateExplicitIdsAreDedupedBeforeDispatch(): void public function testExplicitFilterRawValueReachesEveryChunkUnchanged(): void { Queue::fake(); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); $payload = $this->basePayload(); - $payload['speaker_ids'] = range(1, 150); + $payload['speaker_ids'] = range(1, $chunkSize + 1); $rawFilter = ['first_name=@Test']; $this->service()->triggerSendEmails(self::$summit, $payload, $rawFilter); @@ -292,14 +296,15 @@ public function testMemberIdFilterSelectsOnlyTheMatchingSpeaker(): void public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedIdExactlyOnce(): void { // The filter-based path is the only one summit-admin drives (it always sends filter[], - // never speaker_ids), and it resolves ids by paging getSpeakersIdsBySummit CHUNK_SIZE at - // a time. Seed CHUNK_SIZE + 1 speakers sharing a unique first name on one presentation of - // this summit, so the resolution has to walk two non-empty pages plus the empty one that - // ends the loop. self::$defaultSpeaker (with its own presentations) is the control the - // first_name filter must leave out. A page that is skipped, re-read, or overwritten + // never speaker_ids). Seed one more speaker than the configured job chunk size sharing a + // unique first name on one presentation of this summit, so dispatch has to split them + // into two job chunks. self::$defaultSpeaker (with its own presentations) is the control + // the first_name filter must leave out. A page that is skipped, re-read, or overwritten // instead of merged breaks the exact-set assertion below. Queue::fake(); + $chunkSize = intval(Config::get('emails.speakers_process_job_chunk_size', 200)); + $firstName = 'PageSeed' . str_random(8); $presentation = new \models\summit\Presentation(); @@ -312,7 +317,7 @@ public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedId $presentation->setEndDate(new \DateTime('+1 hour', new \DateTimeZone('UTC'))); $seeded = []; - for ($i = 0; $i < SpeakerService::CHUNK_SIZE + 1; $i++) { + for ($i = 0; $i < $chunkSize + 1; $i++) { $speaker = new PresentationSpeaker(); $speaker->setFirstName($firstName); $speaker->setLastName("Speaker {$i}"); @@ -333,7 +338,7 @@ public function testFilterBasedSelectionSpanningSeveralPagesCoversEveryMatchedId $slices = array_map(fn($job) => $this->jobProperty($job, 'payload')['speaker_ids'], $this->pushedJobs()); $sizes = array_map('count', $slices); rsort($sizes); - $this->assertEquals([SpeakerService::CHUNK_SIZE, 1], $sizes, 'CHUNK_SIZE + 1 matched ids must become one full chunk and one single-id chunk'); + $this->assertEquals([$chunkSize, 1], $sizes, 'chunkSize + 1 matched ids must become one full chunk and one single-id chunk'); $covered = array_merge(...$slices); sort($covered); From b2dfc48531a38bcae2f5cc2efe62d4c608a78976 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 3 Sep 2026 10:38:08 -0300 Subject: [PATCH 12/12] fix(speakers): stop logging access_token and raw filter PII in bulk send debug/error logs SpeakerService::triggerSendEmails's debug log json_encode()'d the full payload, including access_token - confirmed leaking a live bearer token in a real dev send today. Replaced with summit id, flow_event, speaker_ids count and whether a filter was given. ProcessSpeakersEmailRequestJob::failed()'s error log json_encode()'d the raw filter, which can carry email/full_name PII (valid speaker filter fields). New redactFilterFieldNames() logs only the filter's field names. Found and confirmed via adversarial review of CodeRabbit's full-review findings on PR #595. test(speakers): add red-green verified regression test for filter PII redaction testFailedChunkLogsFilterFieldNamesButNotTheirValues asserts the failed() error log contains the filter's field names but not an email value from it; reverting the redaction makes it fail (Mockery 0 matching calls). --- .../ProcessSpeakersEmailRequestJob.php | 19 +++++++++++++-- app/Services/Model/Imp/SpeakerService.php | 6 +++-- ...sSpeakersEmailRequestJobFailedHookTest.php | 23 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php index d0b9b1dfa..0510e85a5 100644 --- a/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php +++ b/app/Jobs/Emails/PresentationSubmissions/ProcessSpeakersEmailRequestJob.php @@ -125,7 +125,7 @@ public function failed(\Throwable $e): void ( sprintf ( - "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) failed (%s: %s); up to %s of them may not have been processed. Speaker ids in the chunk: [%s] filter %s. %s.", + "ProcessSpeakersEmailRequestJob::failed summit %s flow_event %s: chunk of %s speaker(s) failed (%s: %s); up to %s of them may not have been processed. Speaker ids in the chunk: [%s] filter fields %s. %s.", $this->summit_id, $flow_event, count($speaker_ids), @@ -133,7 +133,7 @@ public function failed(\Throwable $e): void $e->getMessage(), count($speaker_ids), $ids_list, - json_encode($this->filter), + json_encode($this->redactFilterFieldNames($this->filter)), $resend_hint ) ); @@ -187,4 +187,19 @@ public function failed(\Throwable $e): void Log::error($ex); } } + + /** + * Reduces a raw filter (["email==foo@bar.com", "presentations_selection_plan_id==78"]) to + * just its field names (["email", "presentations_selection_plan_id"]) so error logs never + * carry filter values that may be PII - email and full_name are valid filter fields + * (ISpeakerFilterFields::OPERATORS). + * + * @param mixed $filter + * @return string[] + */ + private function redactFilterFieldNames($filter): array + { + if (empty($filter) || !is_array($filter)) return []; + return array_map(fn($condition) => preg_replace('/[=<>@!].*/', '', (string)$condition), $filter); + } } diff --git a/app/Services/Model/Imp/SpeakerService.php b/app/Services/Model/Imp/SpeakerService.php index de07538d7..e47018633 100644 --- a/app/Services/Model/Imp/SpeakerService.php +++ b/app/Services/Model/Imp/SpeakerService.php @@ -1243,9 +1243,11 @@ public function triggerSendEmails(Summit $summit, array $payload, $filter = null ( sprintf ( - "SpeakerService::triggerSendEmails summit %s payload %s process_db_chunk_size %s process_jon_chunk_size %s", + "SpeakerService::triggerSendEmails summit %s email_flow_event %s speaker_ids_count %s has_filter %s process_db_chunk_size %s process_jon_chunk_size %s", $summit->getId(), - json_encode($payload), + $payload['email_flow_event'] ?? '', + isset($payload['speaker_ids']) ? count($payload['speaker_ids']) : 0, + !is_null($filter) ? 'yes' : 'no', $process_db_chunk_size, $process_jon_chunk_size ) diff --git a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php index 5dc80afd5..cfdfdc781 100644 --- a/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php +++ b/tests/ProcessSpeakersEmailRequestJobFailedHookTest.php @@ -194,4 +194,27 @@ public function testFailedChunkWithoutOutcomeRecipientOnlyLogsTheUnprocessedIds( ->withArgs(fn($message) => is_string($message) && str_contains($message, '44, 55')) ->once(); } + + public function testFailedChunkLogsFilterFieldNamesButNotTheirValues(): void + { + // email and full_name are valid speaker filter fields (ISpeakerFilterFields::OPERATORS), + // so the raw filter can carry PII. The error log must still say which fields were used + // (useful to reproduce the failed send) without writing the PII value itself. + Queue::fake(); + Log::spy(); + + $job = new ProcessSpeakersEmailRequestJob(self::$summit->getId(), [ + 'email_flow_event' => 'SUMMIT_SUBMISSIONS_PRESENTATION_SPEAKER_ACCEPTED_ALTERNATE', + 'speaker_ids' => [11, 22], + ], ['email==foo@bar.com', 'presentations_selection_plan_id==78']); + + $job->failed(new \RuntimeException('worker killed mid-chunk')); + + Log::shouldHaveReceived('error') + ->withArgs(fn($message) => is_string($message) + && str_contains($message, 'email') + && str_contains($message, 'presentations_selection_plan_id') + && !str_contains($message, 'foo@bar.com')) + ->once(); + } }