test(infra): cover external SDK/HTTP failure modes for Resend, FCM, and OpenAI

[thomasluizon]

What

Adds intelligent failure-mode tests for the three outbound integrations, exercising the retry/timeout behavior added by #327 / #341 / Batch 4b. Every test asserts an observable outcome (no throw, exact attempt count, subscription-pruning decision, usage side-effects) rather than mock interactions, so each can fail if the behavior regresses.

Resend — transactional (ResendEmailServiceTests)

• AuthError401_HandledWithoutRetry — a wrong-API-key 401 is surfaced (logged, no throw) and not retried (single attempt).
• InvalidEmailAddress422_HandledWithoutRetry — a Resend validation 422 is handled without a retry storm.
• TransientServerError_RetriesUpToPolicyLimit — a persistent 503 retries up to HttpRetryPolicy.MaxRetries (1 + MaxRetries attempts), proving the permanent-vs-transient split.
• Adds a ResponseFactory seam to the fake handler so retry counts survive the per-attempt response disposal HttpRetryPolicy performs.

Resend — marketing (ResendEmailServiceMarketingRetryTests)

• DoesNotRetry_On403Forbidden — 403 is treated as permanent (single attempt), complementing the existing retried 429/5xx cases.

FCM (PushNotificationServiceTests)

• Extracts the duplicated stale-token classification into internal static IsStaleFcmError(MessagingErrorCode?) (one source of truth, wired into both the batch-response loop and the per-subscription catch filter).
• IsStaleFcmError_RateLimitAuthAndTransientErrors_KeepsSubscription — QuotaExceeded (429 rate-limit), ThirdPartyAuthError (401/403 auth), Internal/Unavailable, and unknown (null) errors keep the subscription: per-recipient isolation, no batch crash.
• IsStaleFcmError_PermanentlyInvalidTokens_PrunesSubscription — only Unregistered/InvalidArgument/SenderIdMismatch prune.

The full FCM send path can't be exercised in a unit test: FirebaseMessaging.DefaultInstance is a static singleton and FirebaseMessagingException has an internal constructor, so there is no injection seam. The extracted classifier is the data-destructive decision under test (a wrong classification would delete a valid subscription on a transient 429). The "Firebase not initialized → subscription kept, no throw" isolation path is already covered.

OpenAI (AiCompletionErrorTests, new)

• CompleteTextAsync_QuotaExhausted429_RetriesThenThrowsWithoutRecordingUsage — a persistent 429 insufficient_quota retries (each attempt logged by AiRetryLoggingPolicy) then propagates ClientResultException per the documented contract, and records no phantom token usage.
• CompleteJsonAsync_ContextLengthExceeded400_ThrowsImmediatelyWithoutRetryOrUsage — a 400 context_length_exceeded throws immediately with no retry and no usage recorded.

Testing

dotnet build + full Orbit.Infrastructure.Tests suite green (1786 passed, 0 failed); 14 new tests all pass.

Scope

orbit-api only. The PushNotificationService change is an internal refactor (no DTO/endpoint change) — no client-side mirror required.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #368 (thomasluizon/orbit-api)

Scope: PR #368 — test(infra): cover external SDK/HTTP failure modes for Resend, FCM, and OpenAI
Recommendation: APPROVE

Summary

This PR is almost entirely new/expanded unit tests covering the retry/timeout failure modes of three outbound integrations (Resend email, FCM push, OpenAI), plus one small, behavior-preserving production refactor in PushNotificationService.cs that extracts a duplicated FCM stale-token classification check into a single internal static bool IsStaleFcmError(MessagingErrorCode? code). Every new test asserts an observable outcome (attempt count via a real HttpMessageHandler/ILogger fake, thrown-or-not, usage-recorder side effects) rather than mock interactions, and each traces correctly to the underlying retry policies (HttpRetryPolicy.IsTransient, AiRetryLoggingPolicy/ClientRetryPolicy defaults). No DTO, Controller route, or endpoint changed, so there is no cross-repo contract surface to check.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

• [Info] The IsStaleFcmError extraction (src/Orbit.Infrastructure/Services/PushNotificationService.cs:238-241) is a clean, well-documented DRY fix (dimension 3) — same three MessagingErrorCode values as both prior inline checks, now covered directly by [Theory] tests instead of only indirectly through the two call sites. No behavior change.
• [Info] The new AiCompletionErrorTests correctly exercises the real AiRetryLoggingPolicy/ClientRetryPolicy retry/backoff logic via a ZeroDelayRetryPolicy override of GetNextDelay, and the real AiCompletionClient internal test constructor — not a reimplementation, so the assertions (3 attempts for a persistent 429, 1 attempt for a 400, no phantom usage recorded either way) are meaningful regression coverage, not tautological.

Subagents

Agent	Verdict
security-reviewer	PASS — reviewed the PushNotificationService.cs refactor (confirmed behavior-preserving, no change to prune/keep semantics) and all four test files; no real secrets (only obvious fixtures like re_test_key), no Controller/DTO/auth/webhook/CORS surface touched.
contract-aligner	N/A — no DTO, Controller route, or packages/shared/endpoints.ts surface changed.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptation; Build runs as a separate required check on this PR.
Tests (dotnet)	N/A — skipped per CI adaptation; Unit Tests run as a separate required check on this PR. PR body reports dotnet build + full Orbit.Infrastructure.Tests suite green (1786 passed, 0 failed), 14 new tests all pass.

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop): N/A — no apps/* UI files in this diff (backend-only repo).
• Dimension 9 (Parity web↔mobile) and 10 (i18n): N/A — frontend-only dimensions, not verifiable from orbit-api; diff has no user-facing surface.
• Dimension 11 (Contract drift + backward-compat): N/A — no DTO/schema/endpoint field added, removed, or renamed; sibling orbit-ui-mobile repo not checked out in this CI job, but nothing in this diff requires it (test-only + internal refactor, per the PR's own "Scope" note: "no DTO/endpoint change — no client-side mirror required").
• Dimension 14 (FEATURES.md parity): N/A — no user-facing feature added, changed, or removed.
• Every changed file (PushNotificationService.cs, AiCompletionErrorTests.cs, PushNotificationServiceTests.cs, ResendEmailServiceMarketingRetryTests.cs, ResendEmailServiceTests.cs) was read in full diff context and given a verdict above — nothing deferred within the changed-file inventory.
• Phase 6 cross-model /second-opinion step: not applicable — no Critical finding survived to need it.

What's good

• Tests assert real, observable outcomes (attempt counts, thrown-exception type/status, usage-recorder non-invocation) instead of verifying mock call patterns — they will actually fail if the retry/permanent-vs-transient split regresses.
• The PR body is precise about what is and isn't testable (calls out the FCM FirebaseMessagingException internal-constructor limitation honestly rather than working around it with an unjustified seam).
• The refactor is minimal and exactly matches its stated purpose: one source of truth for the stale-token decision, now directly unit-tested via [Theory], with a same-shape doc-comment explaining the transient/permanent split.
• No scope creep — touches only the three integrations named in the PR title.

Recommendation

No changes requested. Safe to merge once the separate required Build / Unit Tests / SonarCloud checks pass.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #368 (thomasluizon/orbit-api)

Scope: test(infra): cover external SDK/HTTP failure modes for Resend, FCM, and OpenAI
Recommendation: APPROVE

Summary

• src/Orbit.Infrastructure/Services/PushNotificationService.cs extracts the duplicated FCM permanent-vs-transient error check into internal static bool IsStaleFcmError(MessagingErrorCode?), used in both SendFcm (batch loop) and SendFcmToSubscription (per-recipient catch). Verified behavior-preserving: the same three prune codes (Unregistered, InvalidArgument, SenderIdMismatch) prune, everything else is kept, in both call sites. No duplicated copy left behind.
• Four test files (AiCompletionErrorTests.cs new, plus additions to PushNotificationServiceTests.cs, ResendEmailServiceMarketingRetryTests.cs, ResendEmailServiceTests.cs) add failure-mode coverage for Resend (401/422/503), the FCM classifier (Theory tests for both prune/keep branches), and OpenAI (429 quota retry-then-throw, 400 context-length no-retry) — each asserting an observable outcome (attempt count, no-throw, usage non-recording) rather than mock interactions.

Findings

Severity	Count
Critical	0
High	0
Medium	0
Low / Info	2

Info — the IsStaleFcmError extraction is a clean root-cause DRY fix, immediately covered by its own tests (tests/Orbit.Infrastructure.Tests/Services/PushNotificationServiceTests.cs).
Info — AiCompletionErrorTests.cs correctly reuses the existing internal test-seam constructor on AiCompletionClient and a ZeroDelayRetryPolicy override of AiRetryLoggingPolicy.GetNextDelay to keep the 429-retry test fast.

An independent security pass (Controllers/Infrastructure focus) found no injection, authn/authz, data-exposure, credential-handling, or webhook-signature issues. Test fakes use obviously-fake keys/hosts (re_test_key, test-key, https://orbit.test/v1) exercised only against in-process fake HttpMessageHandlers.

No Critical/High findings on correctness, dead code, comment policy (only /// XML-doc added, no narration //), type safety, no-workaround, or the backend hard rules (no timezone/authz/validation/transaction/logging-format surface touched).

Not applicable / deferred

• Cross-repo contract parity (orbit-ui-mobile, packages/shared) — no DTO, endpoint, or client-facing surface changed; not verifiable in this CI job regardless (sibling repo not checked out).
• Build/test validation — deferred to this PR's own required Build / Unit Tests / SonarCloud checks.

Clean, focused test-coverage PR with one small behavior-preserving refactor. No changes requested.