Batch FX lookups, guard subscription updates with a concurrency token - #140
Merged
Conversation
…r subscription BurnRateCalculator called GetRateAsync per subscription, and FxRateService reads the cache with a fresh AsNoTracking query each time, so nothing deduplicated them. Twenty USD subscriptions against an INR home currency issued the same query twenty times - on the screen the app opens to, and again on every SubscriptionsChangedMessage, against a 0.1 CPU instance and a scale-to-zero database. IFxRateService gains GetRatesAsync: many base currencies, one target, one round trip. Npgsql turns the Contains into base_currency = ANY(...). Absent from the returned dictionary means "no rate", exactly as a null return does for the single-pair call, so UnresolvedSubscriptionIds behaves unchanged. Chosen over memoizing inside FxRateService, which would have hidden the round trips rather than removed them and left the interface implying a cost it no longer had. Two behaviours the batch deliberately keeps: - The on-demand fetch still runs, for missed pairs only. A user's first subscription in a new currency has to count toward their totals before the next scheduled pass, and that path - cooldown included - is what does it. - The target currency is dropped and the rest deduplicated before querying. Nothing stores an identity rate, so asking for one is a guaranteed miss that would then trigger a pointless provider call. The calculator now walks the subscription list twice - once for currencies, once for amounts - so it materializes rather than trusting the caller's IEnumerable. A test pins that with a sequence that throws on second enumeration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An update read the row and wrote every field back with no check, so overlapping writes were last-write-wins across the whole entity, silently. The case that is not theoretical: MarkPaidAsync sets LastPaidDate and advances NextBillingDate, while UpdateAsync writes NextBillingDate straight from the request. An edit screen opened before a mark-paid - a second device, or a stale detail page - wrote the pre-payment billing date back on save. The payment was reversed and the row left inconsistent: LastPaidDate set, but the charge due again on the old date. That matters more than an ordinary lost update, because a past NextBillingDate is what IsOverdue reads, so the corrupted state reads as "this charge is outstanding" for a charge already settled. Postgres' xmin is mapped as a shadow row version. No column is added - xmin exists on every table - and Npgsql's generator emits no DDL for it, so the migration produces only the history row: no backfill, no lock on an existing table. The migration file reads as though it adds a column; a comment there records that `dotnet ef migrations script` proves otherwise. The token has to make a round trip to be worth anything. Loading a row and saving it in the same call compares it with itself and can never conflict, so SubscriptionDto carries Version out and CreateSubscriptionRequest takes it back in, where it becomes the OriginalValue the UPDATE asserts on. Version is optional, deliberately. APKs already sideloaded do not know the field and are not force-upgraded, so omitting it keeps the previous behaviour rather than failing every older client's saves. New clients get the guarantee; old ones are no worse off than before. UpdateAsync returns SubscriptionUpdateResult rather than a nullable DTO, because null could no longer distinguish "no such subscription" from "someone else got there first" - 404 and 409 are different answers. On the client, a 409 reloads the record and says so, rather than retrying: a retry would re-apply an edit written against a state that no longer exists, which is the very thing being prevented. Reloading also refreshes the version, so the next save is not doomed to conflict again. 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.
The two design calls left open from the review. Branched from current
main(ea7d9a1), so #139 is included.Closes #137
Closes #138
#137 — one FX query instead of one per subscription
BurnRateCalculatorcalledGetRateAsyncper subscription, andFxRateServicereads the cache with a freshAsNoTrackingquery each time, so nothing deduplicated them. Twenty USD subscriptions against an INR home currency issued the same query twenty times — on the screen the app opens to, again on everySubscriptionsChangedMessage, against a 0.1 CPU instance and a scale-to-zero database.IFxRateServicegainsGetRatesAsync: many base currencies, one target, one round trip (base_currency = ANY(...)). Absent from the returned dictionary means "no rate", exactly asnulldoes for the single-pair call, soUnresolvedSubscriptionIdsis unchanged.I went with the batch method on the interface rather than memoizing inside
FxRateService— memoization would have hidden the round trips rather than removed them, and left the interface implying a cost it no longer had.Two behaviours deliberately kept: the on-demand fetch still runs for missed pairs only (a user's first subscription in a new currency must count before the next scheduled pass), and the target currency is dropped before querying since nothing stores an identity rate.
The calculator now walks the list twice, so it materializes rather than trusting the caller's
IEnumerable— pinned by a test using a sequence that throws on second enumeration.#138 — optimistic concurrency
Postgres'
xminmapped as a shadow row version. No column is added —xminexists on every table, and Npgsql's generator emits no DDL for it. I verified withdotnet ef migrations script: the migration produces only the__EFMigrationsHistoryinsert. No backfill, no lock on an existing table. The generated C# reads as though it adds a column, so there's a comment on the file recording what the SQL actually is.The token has to round-trip to be worth anything. Loading a row and saving it in the same call compares it with itself and can never conflict — so
SubscriptionDtocarriesVersionout andCreateSubscriptionRequesttakes it back in, where it becomes theOriginalValuethe UPDATE asserts on. This is the part that makes the feature real rather than decorative.Versionis optional, and that's a judgement call worth reviewing: APKs already sideloaded don't know the field and aren't force-upgraded, so omitting it keeps last-write-wins rather than failing every older client's save. New clients get the guarantee; old ones are no worse off than today. A test pins that path.UpdateAsyncnow returnsSubscriptionUpdateResult—nullcould no longer distinguish "no such subscription" from "someone got there first", and 404 and 409 are different answers.Client behaviour is the one you picked: a 409 reloads the record and explains, without navigating away. Not a retry — a retry would re-apply an edit written against a state that no longer exists, which is the thing being prevented. Reloading also refreshes the version, so the next save isn't doomed to conflict again.
Testing
SubVora.Api.TestsSubVora.Mobile.TestsSubVora.Infrastructure.TestsSubVora.Application.Tests436 total, 0 failures, run in full against real Postgres via Testcontainers.
New coverage worth calling out —
ConcurrentUpdateTestsreproduces the actual bug end to end: create → mark paid → save a stale edit → 409, and the payment survives withNextBillingDateandLastPaidDateintact. Before this change that request succeeded and reversed the payment. Also covered: stale-edit-vs-edit, no-version-still-applies, 404-not-409 for a missing row, and on the client, version round-trip, the 409 reload, no spuriousSubscriptionsChangedMessageon a refused save, a fresh version on retry, and the reload-also-failed case keeping the offline message rather than claiming a reload happened.Note
SubscriptionDtoandCreateSubscriptionRequestchanged on both sides — there's no shared DTO project by design, so API and mobile were edited together, per CLAUDE.md.