Index the refresh-token lookup, split health checks, add mobile timeouts and validation bounds - #139
Merged
Merged
Conversation
The table indexed only user_id, but token_hash is what the hot path filters on:
AuthService.RefreshAsync and LogoutAsync both look a token up by it. Access
tokens live 15 minutes and refresh tokens rotate on every use, so an active
client ran a sequential scan of this table roughly every 15 minutes - over the
fastest-growing table in the schema, on a 0.1 CPU instance.
Unique rather than a plain index. The value is a SHA-256 of 32 cryptographically
random bytes, so uniqueness is the invariant AuthService already relies on:
SingleOrDefaultAsync throws rather than picking one if two rows ever match. This
makes the database enforce what the code assumes.
The migration creates the index on an existing table, which fails if duplicate
hashes are already present. That needs a SHA-256 collision or a duplicated
insert, neither of which any code path produces - and if it did happen the
migration workflow now fails before the deploy fires, so the schema and the
running release stay consistent either way.
The baseline update is the generated [Migration("2026...")] id tripping
detect-secrets' base64 entropy rule, exactly as the six earlier migration
Designer files already in the baseline do.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…robing Postgres
render.yaml pointed healthCheckPath at /health, which runs an Npgsql probe, and
Render polls that path continuously for the life of the service. DEPLOYMENT.md
§5 already explains why that is expensive - Neon's free tier bills compute and
scales to zero when idle, so a database-touching check held awake around the
clock burns the allowance - and routes the keep-warm cron ping to / for exactly
that reason. The platform was doing it anyway, more often.
It is also the wrong semantics: a failing health check restarts the instance,
and a database blip should not restart an app that is running perfectly well.
Three endpoints now:
- /health/live - no dependency checks, what healthCheckPath points at
- /health/ready - includes the database probe, for deploy verification
- /health - unchanged alias for /health/ready, so the curl lines in the
docs and anything already pointing there keep working
Tests cover all three, and pin the distinction that matters: against an
unreachable database, liveness still answers Healthy while readiness returns
503. That test runs the app in Production, because Program.cs migrates on
startup under Development and would fail to boot against the very state the
test needs to create.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… default No Refit client set HttpClient.Timeout, so all six plus the refresh client used .NET's 100-second default. This compounded the offline crash fixed in #131. A host that refuses a connection fails instantly and the offline message appears at once - but one that swallows it, like a dead adb tunnel after unplugging USB or a sleeping free-tier instance, hangs for the full 100 seconds first. A minute and a half of spinner is long past the point where people force-quit. 30s for normal requests: comfortably above a genuine Render cold start (40-60s happens only on wake, and a request that slow is better retried by the user than waited on), far below where it was. 15s for the refresh client, because a refresh runs inside another request's 401 retry and its wait stacks on top of the original's. Both live on ApiConfig with the reasoning attached, and one ConfigureApiClient method now applies base address and timeout to every Refit client - a client added later cannot quietly ship without the timeout, which is how all six ended up on the default to begin with. ApiErrorMapper already maps TaskCanceledException to "You appear to be offline.", so nothing new is needed to handle it - it just arrives sooner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CostAmount was validated with GreaterThan(0) and nothing else, but the column is numeric(12,2). .NET's decimal holds far more, so anything at or above 10^10 passed validation and failed in Postgres with SQLSTATE 22003 - a DbUpdateException no controller catches, which GlobalExceptionHandler turned into a 500. A client sending a bad number should get a 400 naming the field. AlertDaysAdvance had the same open-ended shape. Nothing overflows server-side since the column is a plain int, but RenewalNotificationPlanner computes NextBillingDate.AddDays(-AlertDaysAdvance), which throws near int.MaxValue. LocalRenewalNotificationScheduler catches and logs it, so the app silently schedules no reminders at all - the feature disappears rather than failing visibly. Capped at 365 days, past any real lead time. UpdateUserProfileRequestValidator gets the same ceiling, since that value becomes a subscription's AlertDaysAdvance when one is created without an explicit lead time - bounding only the subscription would move the problem one step back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third batch from the review. Branched from current
main(c7ad8bc), so #129, #131 and #132 are all included — no conflicts.Closes #133
Closes #134
Closes #135
Closes #136
#137 (FX batching) and #138 (optimistic concurrency) are deliberately not here — they're design calls, left open.
#133 — unique index on
refresh_tokens.token_hashThe table indexed only
user_id, buttoken_hashis whatRefreshAsyncandLogoutAsyncfilter on. With 15-minute access tokens and rotation on every use, an active client ran a sequential scan of the fastest-growing table in the schema roughly every 15 minutes.Unique, not just indexed — the value is a SHA-256 of 32 random bytes, and
SingleOrDefaultAsyncalready depends on uniqueness (two matching rows would throw, not pick one). New migrationAddRefreshTokenHashUniqueIndex.#134 — health checks: liveness split from readiness
render.yamlpointedhealthCheckPathat/health, which probes Postgres, and Render polls that path continuously for the service's whole life.DEPLOYMENT.md§5 already explains why that's expensive — Neon bills compute and scales to zero, so a database-touching check held awake round the clock burns the allowance — and routes the cron ping to/for exactly that reason. The platform was doing it anyway, more often./health/livehealthCheckPath; anything polling on a schedule/health/ready/health/health/ready/healthis preserved as you asked, so thecurllines in both docs and any existing monitor keep working.The test that matters: against an unreachable database,
/health/livestill answersHealthywhile/health/readyreturns 503. It runs the app inProduction, becauseProgram.csmigrates on startup underDevelopmentand can't boot against the state the test needs.#135 — mobile HTTP timeouts
No Refit client set
HttpClient.Timeout, so all six plus the refresh client used the 100-second default. This compounded #130: a refused connection fails instantly, but a swallowed one — dead adb tunnel, sleeping instance — hung the full 100s before the offline message.30s for normal requests (clears a genuine 40–60s cold start), 15s for the refresh client since its wait stacks on top of the request that triggered it. One
ConfigureApiClientmethod now applies base address and timeout to every client, so a client added later can't ship without the timeout — which is how all six ended up on the default.#136 — validation bounds
CostAmounthadGreaterThan(0)and nothing else against anumeric(12,2)column, so anything ≥ 10^10 passed validation and blew up in Postgres as a 500 instead of a 400. Now bounded.AlertDaysAdvancecapped at 365. Nothing overflows server-side, butRenewalNotificationPlannerdoesNextBillingDate.AddDays(-AlertDaysAdvance), which throws nearint.MaxValue; the scheduler catches it, so the app silently scheduled no reminders at all — the feature vanishing rather than failing visibly. Same ceiling onUpdateUserProfileRequestValidator, since that value becomes a subscription's lead time when one is created without an explicit one.Testing
SubVora.Api.TestsSubVora.Infrastructure.TestsSubVora.Application.TestsSubVora.Mobile.Tests416 total, 0 failures — run in full against real Postgres via Testcontainers, and re-run after a commit-boundary rewrite to confirm content was unchanged.
New coverage: three health endpoints plus the liveness/readiness distinction under a dead database; the unique index asserted against the migrated schema via
pg_index; cost and alert-day bounds at and past the limits; timeout invariants.After merge
Nothing required —
render.yamlis a Blueprint, sohealthCheckPathsyncs on the next deploy. Worth confirming in the Render dashboard that the health check moved to/health/live, since a manually-set value there can shadow the blueprint.