perf(api): parallelize/batch background fan-out to cut sequential latency

[thomasluizon]

What & why

Three background/command paths awaited slow per-item work sequentially, so their latency scaled linearly with recipient/user count. This makes the fan-out concurrent or batched while preserving behavior — same records written, same push count, per-item error isolation intact.

1. CreateChallengeCommand — invite pushes

Sequential foreach ... await SendToUserAsync → throttled Task.WhenAll (SemaphoreSlim, cap 5). Because PushNotificationService shares the caller's scoped OrbitDbContext (registered via AddHttpClient → transient over a scoped context) and EF DbContext is not thread-safe, each concurrent send runs on its own DI child scope — the same isolation pattern already used by the Astra read-only tools (#301) and GoogleAuthCommand. Per-recipient try/catch + warning log is preserved.

2. ReminderSchedulerService — two job paths

The DB record-keeping (the in-memory dedup set + per-item SaveChanges ordering) is correctness-critical and stays sequential on the single request-scoped context. The slow part — the FCM/WebPush network sends — is deferred into a bounded worker pool (BackgroundServices:ReminderPushConcurrency, default 8), each worker draining a shared ConcurrentQueue on its own isolated scope. Push failures are now logged per recipient (Warning) instead of aborting the whole tick.

3. StreakFreezeAutoActivationService — per-user saves

Eligible users are staged and persisted in one batched SaveChanges. On a unique/concurrency conflict the batch is discarded and it falls back to the original per-user saves, so the failure-isolation contract locked in by the existing OneUserConflict… / OneUserConcurrencyConflict… tests is fully preserved (one bad user never drops another's freeze).

Tests

• Reminder: many due reminders deliver to each recipient exactly once; one recipient's push throwing still records all reminders and delivers the rest.
• Streak-freeze: multiple eligible users persist in a single SaveChanges (counting interceptor); existing conflict/isolation tests now exercise the fallback path.
• Challenge: invitees beyond the concurrency cap are each pushed exactly once (throttle drops nothing).
• Full suite green: 4901 tests, 0 failures. No ORBIT0001 narration comments.

Scope

orbit-api only; no API contract, endpoint, or DTO change (parity nudge is a false positive here). Behavior-preserving perf.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #355 — perf(api): parallelize/batch background fan-out to cut sequential latency

Scope: PR #355 in thomasluizon/orbit-api
Recommendation: NEEDS WORK

Summary

This is a behavior-preserving performance PR that parallelizes/batches three previously-sequential
background fan-out paths (challenge-invite pushes, reminder pushes, streak-freeze auto-activation).
Two of the three sites (CreateChallengeCommand's throttled Task.WhenAll and
ReminderSchedulerService's bounded worker pool) correctly isolate per-item push failures with a
try/catch, matching the PR's stated "per-item error isolation intact" claim. The third site,
StreakFreezeAutoActivationService's new batched-save fast path, does not: it persists every staged
user's freeze in one SaveChangesAsync and only afterward loops to send pushes with no per-item
try/catch, so one user's transient push failure silently and permanently drops the notification for
every user later in that batch. This is a real regression the PR's own tests don't cover (they only
throw at save time, never at notify time). No security, contract, or authorization issues found.

Findings

Critical

None.

High

NotifyActivatedAsync has no per-item error isolation, unlike the other two fan-out sites in this PR

• dimension: 1. Correctness
• location: src/Orbit.Infrastructure/Services/StreakFreezeAutoActivationService.cs:176-184 (NotifyActivatedAsync), caller at :101-105 (ActivateMissedDayFreezes)
• issue: ActivateMissedDayFreezes now stages every eligible user's freeze and commits them ALL in one batched SaveChangesAsync (TrySaveBatchAsync) before any push is sent. NotifyActivatedAsync then loops await pushService.SendToUserAsync(...) with no try/catch, unlike ReminderSchedulerService.DrainPushQueueAsync (catches Exception ex when (ex is not OperationCanceledException)) and CreateChallengeCommandHandler.NotifyOneAsync (catches Exception ex) — the other two fan-out sites touched by this same PR.
• risk: PushNotificationService.SendToUserAsync has two unguarded EF calls (the subscription ToListAsync and, when stale subscriptions exist, a SaveChangesAsync) that can throw on a transient DB/network error — FCM/WebPush per-subscription failures are already caught internally, but these two calls are not. If SendToUserAsync throws for user K, the exception propagates out of NotifyActivatedAsync/ActivateMissedDayFreezes and is only caught by ScheduledServiceBase's tick-level handler (logs and waits for next hour). Users K+1..N in staged never get a push this tick. Because their StreakFreeze row is already committed (the batch save ran first), StageFreeze's idempotency guard (existingFreezes.Any(f => f.UsedOnDate == missedDate)) will skip them on every future tick too — they will never receive the "streak protected" notification for that date, ever (the streak-preservation logic itself is unaffected — only the notification is silently and permanently lost). This directly contradicts the PR's own description ("failure-isolation contract... fully preserved (one bad user never drops another's freeze)"), which only holds for the save step, not the notify step. Confirmed with an independent skeptic pass: pre-PR, the equivalent bug existed only for the single user whose push failed (per-user save-then-notify meant everyone after them wasn't even saved, so they'd retry next tick); this PR's batching turns that into an unbounded, permanent multi-user regression.
• fix: Wrap the SendToUserAsync call in NotifyActivatedAsync (and, for consistency, in ActivatePerUserFallbackAsync) in a try/catch that logs at Warning and continues, mirroring DrainPushQueueAsync's LogReminderPushFailed pattern — e.g. add a LogFreezePushFailed [LoggerMessage] at Warning and catch Exception ex when (ex is not OperationCanceledException) around each SendToUserAsync call. Add a test that throws from SendToUserAsync mid-NotifyActivatedAsync loop and asserts the remaining staged users still get notified (mirroring CheckAndSendReminders_OneRecipientPushThrows_OthersStillRecordedAndDelivered).
• reference: orbit-api CLAUDE.md hard rule "No workarounds / root-cause every bug" + rubric dimension 1 (Correctness) — internal pattern inconsistency within the same PR; independently corroborated by the security-reviewer subagent as a Low-severity availability/logging-noise finding.

Medium

None.

Low / Info

• (Noted by security-reviewer, folded into the High finding above rather than double-counted) The same missing try/catch also applies to ActivatePerUserFallbackAsync's push call, but that specific gap is not new — it mirrors the pre-existing (pre-PR) ProcessUserAsync code, so it is not flagged as its own finding.

Subagents

Agent	Verdict
security-reviewer	PASS (0 Critical/High/Medium; 1 Low — the same NotifyActivatedAsync/ActivatePerUserFallbackAsync missing-try/catch gap, independently found, folded into the High finding above)
contract-aligner	N/A — no DTO, Controller route, or packages/shared type/endpoints.ts changed (PR body confirms: "no API contract, endpoint, or DTO change")

Validation

Check	Result
Build (dotnet)	N/A — dotnet invocation requires interactive approval unavailable in this automated session; Build is enforced as a separate required CI check
Tests (dotnet)	N/A — same reason; Unit Tests run as a separate required CI check. PR body states "Full suite green: 4901 tests, 0 failures"

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop): N/A — no apps/* UI files in this diff.
• Dimension 9 (Parity web↔mobile): N/A — backend-only diff, not verifiable from this repo.
• Dimension 10 (i18n): N/A — no new user-facing strings (existing PT/EN push copy unchanged).
• Dimension 11 (Contract drift): N/A — no DTO/route/schema surface touched; contract-aligner gate not triggered.
• Dimension 14 (FEATURES.md parity): N/A — pure behavior-preserving perf change, no user-facing feature-surface change per the PR's own "Scope" note.
• Backward-compat guard (Phase 5): N/A — no packages/shared or DTO field add/remove/rename in this diff.
• Build/Test validation (Phase 7): not run locally — dotnet commands required interactive approval unavailable to this session; relying on CI's separate required checks and the PR body's stated test results.
• All 6 changed files (CreateChallengeCommand.cs, ReminderSchedulerService.cs, StreakFreezeAutoActivationService.cs, and their 3 corresponding test files) were read in full and given a verdict — nothing changed was skipped.

What's good

• CreateChallengeCommand's throttled Task.WhenAll (SemaphoreSlim cap 5) and ReminderSchedulerService's bounded worker-pool-over-ConcurrentQueue are both correctly scoped (IServiceScopeFactory.CreateScope() per concurrent unit, avoiding the well-known EF DbContext-is-not-thread-safe trap), correctly bounded (no unbounded task fan-out — capped by MaxChallengeParticipants/MaxInvitePushConcurrency and Math.Min(_pushConcurrency, pending.Count) respectively), and correctly isolate per-item push failures with try/catch + Warning-level logging.
• StreakFreezeAutoActivationService's batch-save-with-fallback design is a genuinely nice pattern: it keeps the happy path to a single SaveChangesAsync while falling back to the proven-safe per-user sequential path on any concurrency/uniqueness conflict, and the existing OneUserConflict…/OneUserConcurrencyConflict… tests correctly continue to exercise that fallback.
• New/updated tests (ManyInvitedFriends_PushesEachExactlyOnce_AcrossThrottledFanOut, CheckAndSendReminders_ManyDueReminders_DeliversEachRecipientExactlyOnce, CheckAndSendReminders_OneRecipientPushThrows_OthersStillRecordedAndDelivered, ActivateMissedDayFreezes_MultipleEligibleUsers_PersistsAllInOneBatchedSave) are well-targeted at the exact new concurrency/batching behavior.
• No narration comments, no dead code, no security issues, no console.log/Debug.WriteLine, structured logging throughout with PascalCase properties.

Recommendation

Fix the High finding — add per-item try/catch around the push send in NotifyActivatedAsync (and, while there, ActivatePerUserFallbackAsync for consistency) — plus a regression test covering a mid-loop push throw. This is a small, isolated fix; once addressed this PR is ready to merge.

---

🤖 Generated with the /pr-review skill (Claude Sonnet 5)

[claude]

Code Review: PR #355 — perf(api): parallelize/batch background fan-out to cut sequential latency

Recommendation: APPROVE

Summary

The PR converts three sequential per-item background/command loops (challenge-invite pushes, reminder pushes, streak-freeze activation) into concurrent or batched fan-out while preserving per-item failure isolation. Each concurrent worker correctly gets its own IServiceScopeFactory.CreateScope() child scope so IPushNotificationService resolves a fresh, isolated OrbitDbContext per task. This mirrors existing precedent already used elsewhere in the codebase (GoogleAuthCommand.cs, SendMarketingBroadcastCommand.cs, ProcessUserChatCommand.cs).

Findings

Critical

None.

High

None.

Medium

None. (One candidate — SemaphoreSlim.WaitAsync(cancellationToken) in CreateChallengeCommand.NotifyOneAsync at src/Orbit.Application/Challenges/Commands/CreateChallengeCommand.cs:102 sits outside the try/catch, so a canceled request could in theory throw past the per-item isolation — investigated and dropped: src/Orbit.Api/Middleware/UnhandledExceptionHandler.cs:17-22 special-cases OperationCanceledException when RequestAborted.IsCancellationRequested, returning a quiet 499 with no Sentry/Error-log noise, so this is not an observable regression.)

Low / Info

• MaxInvitePushConcurrency in CreateChallengeCommand.cs:37 is a hardcoded const 5, while the equivalent ReminderSchedulerService concurrency is configurable via BackgroundServices:ReminderPushConcurrency. Defensible (challenge invites are already bounded by AppConstants.MaxChallengeParticipants), not worth a blocking finding.
• StreakFreezeAutoActivationService's batch-then-fallback path correctly reloads users from the DB after ChangeTracker.Clear() before re-running StageFreeze/ConsumeStreakFreeze(), so a failed batch save cannot double-decrement StreakFreezesAccumulated — this was the main place a bug was expected and it's handled correctly.

Subagent: security-reviewer

Verdict: PASS. Reviewed all three files in full plus confirmed PushNotificationService is itself DbContext-backed, validating why per-worker IServiceScopeFactory.CreateScope() is required. No DbContext instance is ever shared across concurrently-running tasks. [Authorize] N/A (no controllers changed). Logging uses source-generated structured logging, no PII beyond IDs. No empty catches; per-item exceptions caught and logged individually, preserving pre-PR isolation semantics. All SemaphoreSlim/CreateScope() resources properly scoped/disposed. Concurrency bounded in all fan-out paths — no unbounded parallelism. No findings at any severity.

Subagent: contract-aligner

N/A — no DTO, Controller route, or packages/shared surface changed (confirmed via diff + PR body's own "no API contract change" claim). Not independently verifiable in this CI job since orbit-ui-mobile is not checked out here.

What's good

• Correct, precedented DI-scope-per-task isolation pattern for the shared-DbContext-is-not-thread-safe problem, applied consistently across all three call sites.
• The streak-freeze batch/fallback design preserves the exact existing single-user-conflict isolation contract (verified against pre-existing fallback-path tests, unchanged).
• New tests target the actual concurrency-relevant behavior (each recipient pushed exactly once under throttling, one thrower doesn't block the others, exactly one batched SaveChanges call via a counting interceptor) rather than just happy-path coverage.
• No narration comments, no dead code left behind, logging levels follow project hard rules.

Recommendation

Approve. No Critical/High findings. Optional, non-blocking follow-up: consider making MaxInvitePushConcurrency configurable for consistency with ReminderPushConcurrency.

---

Build/test validation skipped per workflow instructions — this PR runs Build / Unit Tests / SonarCloud as separate required checks.

[claude]

PR Review — #355 (see below)

Recommendation: APPROVE

Provenance
This bot has reviewed this PR twice before:

1. Commit af1c083 — REQUEST_CHANGES: High finding — StreakFreezeAutoActivationService.NotifyActivatedAsync sent pushes in a loop with no per-item try/catch, unlike the other two fan-out sites in this PR. A mid-loop throw would silently and permanently drop notifications for every later user in the batch (idempotency guard skips them on retry).
2. Commit ce406b6 — APPROVE: verified the fix landed via a shared NotifyFreezeActivatedAsync helper with try/catch (Exception ex) when (ex is not OperationCanceledException) + LogFreezePushFailed (Warning), used by both the batch-notify path and the per-user fallback path.

git diff ce406b6 0aa9dea (current HEAD) is EMPTY — this commit only merges already-landed main history (PR #357), no new file changes since the last approval.

Findings

• Critical: None
• High: None (previously-flagged High is fixed and unchanged since the prior APPROVE)
• Medium: None
• Low/Info: None new; carried-over Info from prior review (hardcoded MaxInvitePushConcurrency const vs. configurable ReminderPushConcurrency) remains non-blocking

Subagents

• security-reviewer: PASS (reused from prior review — code unchanged)
• contract-aligner: N/A — no DTO/Controller/packages-shared surface touched; not verifiable in CI (sibling repo not checked out)

Validation
Skipped per CI adaptation — Build/Unit Tests/SonarCloud run as separate required checks. SonarCloud shows Quality Gate Passed, 88.2% coverage on new code, 0 duplication.

What's good
Consistent DI-scope-per-task isolation across all three fan-out sites, streak-freeze batch/fallback design correctly avoids double-decrementing on conflict, and targeted concurrency-specific tests (throttled fan-out, one-thrower-doesn't-block-others, single batched SaveChanges via counting interceptor).

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud