perf(api): compression, DB health check, txn timeout, slow-query logging

[thomasluizon]

Four independent ops/perf hardening gaps in Orbit.Api, all behavior-preserving (perf/observability only, no contract change).

Changes

1. HTTP response compression (Brotli/Gzip) for JSON

• AddResponseCompression with Brotli + Gzip providers at CompressionLevel.Fastest, MIME types include application/json + application/problem+json.
• EnableForHttps = true — Render terminates TLS upstream (X-Forwarded-Proto=https), so without it compression would never apply. BREACH is not a concern: responses are parameterized JSON with no attacker-reflected secrets.
• UseResponseCompression() placed after UseForwardedHeaders so Request.IsHttps reflects the forwarded proto before the middleware gates on it.

2. Database connectivity health check on /health

• New DatabaseHealthCheck (OrbitDbContext.Database.CanConnectAsync) registered alongside the existing background-services check.
• A DB outage now surfaces as 503 on /health instead of a superficially-live process (the background check only ever returns Healthy/Degraded).

3. Bounded transaction duration in UnitOfWork.ExecuteInTransactionAsync

• A linked CancellationTokenSource.CancelAfter(TransactionTimeoutSeconds) (default 120s, above the 60s per-command timeout) wraps the transaction-owning path.
• A wedged transaction (app-side stall / lock wait that the per-command timeout can't bound) now rolls back and releases its backend instead of leaking it. Surfaces as a TimeoutException; a caller-initiated cancellation still surfaces as OperationCanceledException (distinguished by the exception filter). Ambient/nested and non-relational paths are unchanged.

4. Slow-query logging on prod PostgreSQL

• New SlowQueryCommandInterceptor (EF DbCommandInterceptor) logs a Warning for any command whose measured duration exceeds SlowQueryThresholdMilliseconds (default 500ms) — observable in Render logs without EF's per-command Information logging. XML doc documents the complementary Supabase-side log_min_duration_statement for server-side-only timing.

Both new knobs live in DatabaseConnectionSettings + appsettings.json.

Tests

• UnitOfWorkTests: timeout → TimeoutException + rolled back; caller-cancel → OperationCanceledException (not timeout); existing throw/ambient paths retained.
• DatabaseHealthCheckTests: reachable → Healthy; unreachable Npgsql → Unhealthy.
• SlowQueryCommandInterceptorTests: above/at/below threshold logging (boundary covered).
• All 4,904 tests pass (dotnet build && dotnet test).

The added DatabaseConnectionSettings dependency on UnitOfWork propagated to the existing direct/DI test constructors (updated in lockstep).

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Code Review: PR #357 (perf/ops hardening)

Recommendation: APPROVE

Summary

Four independent, behavior-preserving ops/perf changes: HTTP response compression (Brotli/Gzip), a DB-connectivity /health check, a wall-clock timeout on UnitOfWork.ExecuteInTransactionAsync, and a slow-query warning-log interceptor. No contract surface (DTO, endpoint, Zod schema) is touched, so the backward-compat guard and contract-aligner are N/A by gate, not by omission.

Severity	Count
Critical (incl. old-client breaks)	0
High	0
Medium	1
Low / Info	2 (noted, not blocking)

Findings

Medium

Unauthenticated /health now performs a live DB round trip with no rate limit or cache

• Location: src/Orbit.Infrastructure/Services/DatabaseHealthCheck.cs:19 (wired via src/Orbit.Api/Extensions/WebApplicationExtensions.cs:64-80; registered in src/Orbit.Api/Extensions/ServiceCollectionExtensions.BackgroundJobs.cs:38)
• Before this PR, GET /health (AllowAnonymous, no [DistributedRateLimit]) only checked an in-memory BackgroundServiceHealthCheck. It now also resolves a scoped OrbitDbContext and calls Database.CanConnectAsync() on every hit — a real round trip through the same request-path Npgsql pool real traffic uses (EfMaxPoolSize = 15, sized tightly against Supabase's connection ceiling per DatabaseConnectionSettings.cs's own doc comment). ASP.NET Core health checks have no built-in caching.
• Risk: an anonymous burst against /health (no auth, no rate limit) competes 1:1 with legitimate request-path connections for a small pool, which could degrade or 503 real traffic during a flood — a DoS lever that did not exist before this change.
• Suggested fix: wrap DatabaseHealthCheck's CanConnectAsync result in a short TTL cache (a few seconds is enough for a liveness probe), or add [DistributedRateLimit] / a lightweight per-IP limiter on /health.

Low / Info (non-blocking)

• The BREACH-safety WHY comment in ServiceCollectionExtensions.Infrastructure.cs:260 ("no attacker-reflected secrets") is imprecise: AuthController responses (verify-code, google) do return tokens alongside reflected/attacker-influenced fields in the same JSON body, which is the shape BREACH targets. The actual reason compression is safe here is that auth is Bearer-header-only (no cookie-based session), so a cross-origin page can't force a victim's browser to auto-replay authenticated requests. The conclusion holds; the stated reasoning would go stale if cookie-based auth is ever added. Worth a reword, not a blocker.
• DatabaseConnectionSettings.TransactionTimeoutSeconds / SlowQueryThresholdMilliseconds have no startup bounds validation, unlike the guarded settings in ValidateOrbitSecuritySettings. Config-only risk, not exploitable.
• application/json was already present in ResponseCompressionDefaults.MimeTypes, so half of the explicit .Concat([...]) addition in AddResponseCompression is a harmless no-op duplicate; application/problem+json is the genuinely new, needed addition.

What's good

• The transaction-timeout/caller-cancel distinction in UnitOfWork.ExecuteInTransactionAsync (timeoutCts.IsCancellationRequested && !cancellationToken.IsCancellationRequested) is correctly reasoned and has dedicated tests for both branches, including the ambient-transaction fast path staying untouched.
• ORBIT0002 compliance preserved: the transaction catch block still only does ChangeTracker.Clear(); throw;, relying on await using scope disposal for rollback — no redundant explicit RollbackAsync().
• SlowQueryCommandInterceptor correctly logs via the [LoggerMessage] source-generator pattern with PascalCase structured properties, at Warning level, and is safe against sensitive-data exposure since EnableSensitiveDataLogging() is confirmed absent repo-wide.
• Response compression correctly excludes text/event-stream (ChatController's SSE endpoint) since that content type was never added to the MIME allowlist — no risk of breaking streaming responses.

Deferred — N/A dimensions

• Dimension 9 (Parity web/mobile), 11 (contract drift), 14 (FEATURES.md parity) — N/A: no DTO, Controller route, Zod schema, or user-facing surface touched anywhere in this diff.
• Cross-repo checks (contract-aligner, packages/shared backward-compat) — not applicable rather than unverifiable: determined from the api-side diff alone that no contract surface changed.
• Build/Unit Tests — not run here; covered by separate required CI checks on this PR.

Test coverage spot-checked: UnitOfWorkTests covers timeout→TimeoutException-and-rolled-back and caller-cancel→OperationCanceledException cases; DatabaseHealthCheckTests covers reachable/unreachable; SlowQueryCommandInterceptorTests covers above/at/below threshold. All ten mechanical test-fixture updates for the new DatabaseConnectionSettings constructor argument were confirmed complete via a repo-wide grep — no stale call sites remain.

Recommendation

Approve as-is; the single Medium finding (unauthenticated /health now doing a live DB round trip with no cache/rate-limit) is a defense-in-depth gap, not a blocker — safe to land with a tracked follow-up to add a short TTL cache or [DistributedRateLimit] on /health before it sees adversarial traffic volume.

[thomasluizon]

SonarCloud Code Analysis: the only failing quality-gate condition is new_coverage (45.5% < 80%). All quality conditions pass — new_reliability_rating, new_security_rating, new_maintainability_rating = A, new_duplicated_lines_density = 0%, new_security_hotspots_reviewed = 100%. No new bug, vulnerability, or code smell was introduced.

The uncovered new lines are composition-root DI wiring (AddResponseCompression, AddOrbitDatabase interceptor registration, health-check registration) and the SlowQueryCommandInterceptor's six thin *Executed overrides that delegate to the unit-tested LogIfSlow seam. The load-bearing logic — slow-query threshold decision, DB health check, and transaction-timeout guard — is covered by new unit tests. Known non-required coverage gate; leaving as-is.

[sonarqubecloud]

Quality Gate failed

Failed conditions
45.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #357 — perf(api): compression, DB health check, txn timeout, slow-query logging

Recommendation: NEEDS WORK

Summary

Three of four changes (compression, DB health check, slow-query logging) are clean and well-tested. The transaction-timeout change (UnitOfWork.ExecuteInTransactionAsync, 120s ceiling) has an unconsidered side effect: it wraps pre-existing external HTTP I/O in RunCalendarAutoSyncCommand (sequential, paginated Google Calendar API fetches) inside the same wall-clock budget. Users with several/large calendars can plausibly exceed 120s, causing a silent, indefinitely-recurring background-sync failure that bypasses the handler's existing graceful MarkCalendarSyncTransientError degradation path.

Findings

High

New transaction wall-clock timeout can silently and permanently break Google Calendar auto-sync for users with several/large calendars

• location: src/Orbit.Infrastructure/Persistence/UnitOfWork.cs:40-42 (new ceiling), consumed by src/Orbit.Application/Calendar/Commands/RunCalendarAutoSyncCommand.cs:102-113
• RunCalendarAutoSyncCommand is not IIdempotentCommand, so this call creates a brand-new transaction + fresh 120s timeout (not an ambient join). FetchAndProcessLocked calls deps.EventFetcher.FetchAsync(...) — an external Google Calendar HTTP fetch — inside that same token, before any DB write. GoogleCalendarEventFetcher.FetchAsync lists calendars then loops sequentially per calendar; GoogleCalendarApi.ListEventsAsync itself paginates with a sequential do...while HTTP loop per calendar (30s timeout per call, HttpClients:DefaultTimeoutSeconds). Nothing bounds the aggregate across calendars/pages.
• When the aggregate exceeds 120s, the resulting OperationCanceledException is explicitly excluded from FetchAndProcessLocked's own Google-API-error catch (ex is not OperationCanceledException), so it propagates past the graceful MarkCalendarSyncTransientError path, becomes TimeoutException at the UnitOfWork boundary, and rolls back. Since GoogleCalendarLastSyncedAt never advances, the same user is retried every 15-minute tick indefinitely (not blocked by the 4-hour dedupe window, which only engages once a sync reaches any terminal state) — a silent, permanent-until-fixed degradation with no user-visible status change.
• DatabaseConnectionSettings.TransactionTimeoutSeconds's own doc comment says AI/batch network I/O is deliberately excluded from this ceiling via a separate AI:BatchNetworkTimeoutSeconds — confirming the author's intent was to keep external I/O out of this boundary, but the Calendar-sync path (pre-existing, unchanged by this diff) violates that intent.
• Fix: move deps.EventFetcher.FetchAsync(...) outside ExecuteInTransactionAsync in FetchAndProcess (it's read-only and doesn't need the transaction/advisory lock); only the reconcile+write phase needs the 120s-bounded transaction. As a safety net, also catch TimeoutException in FetchAndProcessLocked alongside the existing Google-API-error catch so failures still call MarkCalendarSyncTransientError. Add a regression test with a fake slow ICalendarEventFetcher.

Medium

New timeout tests only exercise Task.Delay cancellation, not a real in-flight DB-command cancellation — tests/Orbit.Infrastructure.Tests/Persistence/UnitOfWorkTests.cs:526-568. The feature targets "a transaction that wedges between commands," but neither new test has a real DB command outstanding when the timeout fires. Recommend adding one test where the operation delegate awaits a genuinely slow DB call (e.g. via an interceptor that stalls execution) to confirm the timeout still fires and rolls back cleanly against a real command, not just an in-memory delay.

Subagents

• security-reviewer: dispatched async, did not return within session; compensating manual pass found no Critical/High issue (BREACH reasoning holds — JWT Bearer is non-ambient, CORS uses an explicit origin allowlist; /health leaks no exception detail; SlowQueryCommandInterceptor logs CommandText only, not parameter values, and the only raw-SQL call site nearby is parameterized; compression middleware ordering relative to auth is correct).
• contract-aligner: N/A, no DTO/Controller route/packages/shared change in this diff.

Validation

Build/Tests: N/A in this review session (sandbox could not run dotnet build/dotnet test); PR description states all 4,904 tests pass locally, not independently re-verified here.

Deferred

• DESIGN.md/AI-slop, Parity, i18n, FEATURES.md parity: N/A, backend ops/perf-only diff, no UI/contract/feature-surface change.
• /health's new database check entry is additive to the existing checks array; not cross-checked against orbit-ui-mobile (not available in this session) in case any client parses /health specifically.
• The formal Phase-6 adversarial-skeptic subagent for the High finding above did not return within session; the equivalent adversarial checks (ambient-transaction-join check, retry/circuit-breaker check, pagination-depth check, catch-clause-reachability check) were performed directly against source with file:line evidence in place of the subagent pass.

What's good

Compression, DB health check, and slow-query logging are clean, minimal, correctly ordered in the DI/middleware pipeline, fully comment-policy-compliant (every comment carries a WHY + URL), and ORBIT0002-compliant (no explicit rollback inside using-scoped transactions). The OperationCanceledException vs TimeoutException disambiguation logic is correct by manual trace. All existing UnitOfWork test-constructor call sites were updated in lockstep.

Recommendation

Fix the High finding (keep external Calendar HTTP I/O outside the new transaction/timeout boundary, and catch TimeoutException gracefully) before merge. The Medium test-coverage note can land as a fast follow. Everything else in the PR can ship as-is once the timeout/Calendar interaction is fixed.

🤖 Generated with orbit-api /pr-review