Skip to content

perf: refresh exchange rates in one transaction with one prepared statement - #1167

Closed
thorstenhornung1 wants to merge 3 commits into
ellite:mainfrom
thorstenhornung1:perf/atomic-currency-refresh
Closed

thorstenhornung1 wants to merge 3 commits into
ellite:mainfrom
thorstenhornung1:perf/atomic-currency-refresh

Conversation

@thorstenhornung1

Copy link
Copy Markdown
Contributor

Builds on #1165 and #1166. If you would prefer this standalone, I will rebase it onto main.

The problem

Both rate refresh paths — endpoints/cronjobs/updateexchange.php and endpoints/currency/update_exchange.php — do this:

foreach ($apiData['rates'] as $currencyCode => $rate) {
    ...
    $updateStmt = $db->prepare($updateQuery);   // re-prepared every iteration
    ...
    $updateResult = $updateStmt->execute();     // and autocommitted on its own

    if (!$updateResult) {
        echo "Error updating rate for currency: $currencyCode";   // then carries on
    }
}

Two problems follow.

A partial refresh is silently wrong. Rates are only comparable when they share a conversion base. If the loop stops halfway — a truncated provider response, a failing write, a fatal error — some currencies are converted against the new base and some against the old one. Totals still render, they are just incorrect, and nothing indicates it.

The same statement is parsed once per currency. It never changes between iterations.

The change

  • prepare the update once, before the loop, and reuse it with reset()
  • wrap one user's rate writes and their refresh timestamp in a transaction
  • roll back and report when a write fails, instead of continuing with a half-converted set

The cron job keeps its per-user granularity: one user's failure does not affect the users refreshed before or after them.

Verification

New test cases prove the semantics rather than the implementation:

  • an interrupted refresh leaves the previous rates intact
  • a completed refresh commits every rate together
  • five writes need one prepare
  • a structural guard that both refresh paths open, commit and roll back a transaction, and that the prepare sits outside the loop — so a future path that autocommits per currency fails the tests

Also ran the cron job against a seeded database with six users and confirmed it leaves no transaction open when the provider is unreachable.

thorstenhornung1 and others added 3 commits August 16, 2026 11:43
The exchange-rate cron updated currency rows by code alone:

    UPDATE currencies SET rate = :rate WHERE code = :code

Every user has their own currency rows, and each rate is converted against
that user's main currency. Updating by code therefore overwrote every other
user's rates with a conversion base that is not theirs, on every scheduled
refresh. A user whose main currency is USD would silently get rates derived
from another user's EUR base, and all their converted amounts with them.

The manual refresh endpoint already scoped its writes, so the two paths
disagreed about the same table.

Single-user installations are unaffected, which is why this went unnoticed.

Also adds a small test suite, because the fix is one word and the guarantee
is what matters: a rate write that forgets the user filter now fails the
tests instead of reaching production. The harness needs no Composer and runs
in a container (dev/test.sh), matching the way Wallos vendors its libraries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Price conversion looked the exchange rate up per converted value:

    SELECT rate FROM currencies WHERE id = :currency

Eight call sites did this, each inside a loop over subscriptions, statistics
rows or calendar entries. Rendering a list of 200 subscriptions therefore
issued 200 rate queries for data that changes once a day and is already in
memory for display.

Rates are now loaded once per connection and answered from a map. Measured
against a seeded database with 200 subscriptions: 200 queries and 41ms become
1 query and 0.3ms.

The map is keyed by the connection object through a WeakMap rather than an id,
because ids are reused once a connection is closed and a later connection
would inherit stale rates.

Two behaviours are deliberately preserved: a lookup that resolved a currency
by id alone still resolves any currency, and one that filtered by user still
only sees that user's currencies. A missing rate leaves the price untouched,
as before — and so does a rate of zero, which previously raised a division
error everywhere except the budget calculation, which already guarded it.

Verified by diffing the rendered subscriptions, statistics and calendar pages
before and after the change: byte identical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tement

The rate refresh prepared a new statement for every currency and committed
each write on its own. Two consequences:

A refresh that stops halfway — a provider response that breaks off, a failing
write — leaves some currencies converted against the new base and some against
the old one. Rates are only comparable when they share a base, so the result
looks plausible and is wrong, which is worse than not refreshing at all.

Preparing the same statement once per currency also does the parsing work
repeatedly for a statement that never changes.

Both refresh paths now prepare the update once, reuse it across the loop, and
wrap one user's rates together with their refresh timestamp in a transaction.
A failed write rolls the user's refresh back and reports it, instead of
leaving a half-converted set behind.

The cron job keeps its per-user granularity: one user's failure does not
affect the users refreshed before or after them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ellite added a commit that referenced this pull request Aug 23, 2026
…refresh

Both refresh paths wrote one currency rate per statement, each
committed on its own, so a failure partway through left some
currencies converted against the new base and some against the old
one. Wraps one user's writes in a transaction and reuses one prepared
statement for the loop instead of re-preparing every iteration.

Reviewed by hand-tracing every path from BEGIN to COMMIT/ROLLBACK in
both files and running the test suite (4 new cases, all passing).
Clean merge, no conflicts.
@ellite ellite closed this Aug 23, 2026
thorstenhornung1 added a commit to thorstenhornung1/Wallos that referenced this pull request Sep 3, 2026
…anded

The condition set this morning — every PR brings a regression test, because
upstream has carried our harness since ellite#1165-ellite#1168 and has never been offered
one — is met for all five. Each guard was checked by breaking it, and each
names what it found: the deletion test lists the twelve tables by name, the
password reset test the four lines whose results were dropped.

Where an endpoint cannot be run from a test, and most of these are scripts
needing a session, the guard is structural. That is not a compromise: it is the
idiom upstream's own suite already carries, because ellite#1167 put it there. Two of
the five go further and assert their guarantee against the built schema — that
a rollback takes both halves of a 2FA enrolment with it, and that a rolled back
token swap puts the previous reset token back. Those are the claims the fixes
rest on, and they are worth more than the source-level check above them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013Jnmyn4fw3F1fu9wuo6C87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants