DEV Community

Hammad Shams Uddin
Hammad Shams Uddin

Posted on

A missing exchange rate is not an exchange rate of 1

#devchallenge #bugsmash #javascript #api

Summer Bug Smash: Smash Stories 🐛🛹

This is a submission for DEV's Summer Bug Smash: Smash Stories powered by Sentry.

var fx = (rates.rates && rates.rates[v.currency]) || 1;
Enter fullscreen mode Exit fullscreen mode

That line converts a crypto price into whatever currency the visitor picked. It has been on my site for months. It is one line, it reads like defensive programming, and it is capable of being wrong by a factor of 280.

What || 1 means here

The rate table comes from a currency API. When that API fails, my server-side cache falls back to the only rate it can be sure of:

return $c['data'] ?? ['USD' => 1.0];
Enter fullscreen mode Exit fullscreen mode

Which is honest. USD => 1.0 is true, and the fallback contains nothing it cannot vouch for.

Then the browser asks that table for PKR, gets undefined, and || 1 turns "I don't have that" into "the rate is 1".

So the page renders:

1 BTC = 95,000.00 PKR
Enter fullscreen mode Exit fullscreen mode

The real figure is about 26,600,000. And there is no asterisk, no greyed-out state, no "approximate" — it looks exactly like the correct answer, because it is produced by exactly the same code path as the correct answer.

The second one, in the same function

Three lines down:

var ago = rates.cached_age_sec != null ? Math.round(rates.cached_age_sec / 60) : 0;
...
note: 'Live spot price, updated about ' + (ago <= 0 ? 'just now' : ago + ' min ago')
Enter fullscreen mode Exit fullscreen mode

cached_age_sec is null when the cache layer cannot say how old the figure is. The ternary is careful — it checks for null — and then maps that null onto 0. One line later, 0 is read as "updated just now".

The one case where the freshness is genuinely unknown is the case that produces the most reassuring sentence on the page.

The shape

Both are the same mistake wearing different clothes:

A value that was absent, replaced by a plausible default, and then rendered with the same confidence as a measured one.

The defaults are not stupid. A rate of 1 is the identity. An age of 0 is the neutral element. In isolation each choice looks like the sensible thing to reach for when something is missing. The damage is done at the point of display, where the substitute and the real thing become indistinguishable.

And it is worse than crashing. A crash is a bad experience that tells the truth. This tells a confident lie in the visitor's own currency.

The part that makes it embarrassing

The correct handling was already in the file. Twenty lines above, the plain currency converter has always done this:

var fr = rates.rates[v.from], to = rates.rates[v.to];
if (!(fr > 0) || !(to > 0)) {
  return { error: 'That currency is not in the live feed.' };
}
Enter fullscreen mode Exit fullscreen mode

Same file. Same rate table. Same author. One engine refuses; the two next to it substitute. Nothing distinguishes them except which one I happened to write while thinking about failure.

That is the real lesson and it is not about currencies: defensive habits do not generalise on their own. Getting it right once in a file is not evidence the file is right. It is one data point about one function.

The fixes

Refuse, and name what is missing:

var fx = (rates.rates && rates.rates[v.currency] > 0) ? rates.rates[v.currency] : null;
if (fx === null) {
  return { error: 'Live rates for ' + (v.currency || 'that currency')
    + ' are unavailable right now — try USD, or refresh in a moment.' };
}
Enter fullscreen mode Exit fullscreen mode

Note > 0 rather than a truthiness check. A rate of 0 is as unusable as a missing one, and undefined > 0 is false, so the two collapse into the same branch without a separate test.

And let unknown stay unknown:

function freshness(age, unit) {
  if (age === null || age === undefined) { return 'age unknown'; }
  if (age <= 0) { return 'updated just now'; }
  return 'updated about ' + age + ' ' + unit + ' ago';
}
Enter fullscreen mode Exit fullscreen mode

Three sentences for three states, instead of two sentences for three states. The bug was entirely in that missing third branch.

Testing a feed that has failed

Neither bug is reachable by using the page. The upstream API has to be partially down — up enough to return a table, broken enough that your currency is not in it. I have never once seen that state in a browser, and I never will on purpose.

So the engines had to become reachable from a test. They live in a browser IIFE, so they got the same export guard the widgets file already used:

if (typeof module !== 'undefined' && module.exports) {
  module.exports = { ENGINES, freshness, trueCost };
}
Enter fullscreen mode Exit fullscreen mode

Then the fixture is the whole point — a feed that is deliberately, plausibly incomplete:

const feed = {
  coins: { bitcoin: 95000 },        // but not dogecoin
  rates: { USD: 1, EUR: 0.92 },     // but not PKR
  cached_age_sec: 120,
};
Enter fullscreen mode Exit fullscreen mode

That is not a made-up shape. It is precisely what the server returns when the currency provider is down and the crypto provider is not: ['USD' => 1.0] and nothing else.

One of the nineteen assertions is about a function I did not change:

const bad = ENGINES.currencyConvert({ amount: '100', from: 'USD', to: 'PKR' }, fx);
check('and a missing one was always refused', typeof bad.error === 'string', true);
Enter fullscreen mode Exit fullscreen mode

It cannot fail today. It is there so the three engines that read the same table cannot quietly drift apart again — which is how they got out of step in the first place.

I build Utilorax, a set of free browser-based tools. This came out of the crypto price converter and the gold & silver price calculator, both of which now tell you when they don't know rather than guessing on your behalf.

Top comments (15)

Collapse
 
alexshev profile image
Alex Shev •

The post makes a useful distinction between a feature working once and a system being dependable. I’d add an explicit failure-mode checklist so the next contributor can see which assumptions are intentional and which ones still need evidence.

Collapse
 
hammad4june1999 profile image
Hammad Shams Uddin •

Fair — a failure-mode checklist would make the intent explicit rather than
implied. The three states here were "have it", "don't have it", "don't know
how old it is", and only the third was missing; writing them out beforehand
would have shown the gap without needing the bug.

Collapse
 
alexshev profile image
Alex Shev •

Right — a missing value needs to preserve its uncertainty instead of acquiring a convenient default. Treating absence as a real state gives the operator a chance to investigate rather than letting a downstream calculation turn it into a plausible fiction.

Thread Thread
 
hammad4june1999 profile image
Hammad Shams Uddin •

That is the sentence I was reaching for and did not find. "Preserve the
uncertainty" is the actual requirement; a default is just the shape it loses.

The follow-on I got wrong the first time is that the instrument has the same
problem. My failure counter could not tell "nothing failed" from "the counter
itself has been throwing since the last deploy" — one absent state, one
convenient reading. It now records attempts alongside failures, so 0 of 8,640
and 0 of 0 stop printing the same line.

Thread Thread
 
alexshev profile image
Alex Shev •

Exactly. The attempt count turns a zero from a claim into an observable: 0 failures / 8,640 attempts and 0 failures / 0 attempts carry completely different operational meaning. I’d also emit an explicit instrumentation-health state, so a broken counter cannot silently masquerade as a clean service.

Thread Thread
 
hammad4june1999 profile image
Hammad Shams Uddin •

Both directions of that bit me in one morning.

A cloaking check I added last week only spoke when it found cloaking, so a
clean day and a check that never ran printed the same thing: nothing. And a
file-integrity check alarmed every single morning on a log the cron writes
nightly — which trains the reader to skim the one line that matters.

Silent-when-healthy and loud-when-fine are the same defect wearing opposite
clothes. Both now report their own state explicitly, which is your point:
the instrument has to say it is alive, not just say when it is unhappy.

Thread Thread
 
alexshev profile image
Alex Shev •

That is a very useful distinction: a health signal needs both a result and a witness that the check actually ran. We ended up treating "not evaluated" as its own outcome, rather than letting it borrow the visual shape of healthy. It makes dashboards noisier in the right way—and makes silence inspectable.

Collapse
 
vinhnguyenthanhdn profile image
Vinh Nguyen •

The > 0 guard moves the problem out of display and into the refusal branch, and that branch has the same property the substituting one had: it only runs when the upstream is partially down, a state you say you have never seen in a browser and never will on purpose. What I would add is a counter at the point of refusal rather than only an error string, so a partially-down feed leaves a trace you can read afterwards instead of a message one visitor saw once. It also gives the branch a heartbeat, because a counter that never moves across months in which your currency provider did have incidents is telling you the refusal path stopped being reachable.

Collapse
 
hammad4june1999 profile image
Hammad Shams Uddin •

This is the right criticism and I'd only move where the counter goes. The
browser is the wrong place to keep it — each visitor's count dies with the
tab, and the one person who saw the refusal is the one person who can't tell
me.

The server already knows: LiveRates::fx() is where the provider fails and
falls back to ['USD' => 1.0], and that's the moment worth counting. Counting
at the display point measures how many people met a degraded state; counting
at the source measures how often it happened, which is the number I'd act on.

Your heartbeat point is the part I hadn't considered. A refusal counter that
stays at zero through a month when the provider did have incidents isn't good
news — it means the branch stopped being reachable and something upstream is
swallowing it.

Collapse
 
vinhnguyenthanhdn profile image
Vinh Nguyen •

Agreed on the source, with one thing worth nailing down at that point. The counter has to be incremented inside the fallback branch itself, not derived later by noticing the table came back as ['USD' => 1.0], because a healthy provider response can legitimately carry 1.0 for USD and the two are the same shape once they leave fx(). Anything reading the value downstream cannot tell them apart, so the count quietly turns into a count of something else. The two numbers are not substitutes either, since the source count says how often the provider failed and the display count says whether anyone was standing there when it did, and it is the gap between them that tells you the refusal path is still wired.

Thread Thread
 
hammad4june1999 profile image
Hammad Shams Uddin •

Implemented, and your refinement is the reason it works rather than a detail on
top of it. The increment sits inside the branch that failed:

if (is_array($c['data'] ?? null)) {
    return $c['data'];
}

self::countFailure('fx');
return ['USD' => 1.0];
Enter fullscreen mode Exit fullscreen mode

Deriving it downstream would have been wrong for exactly the reason you gave.
A healthy response legitimately carries 1.0 for USD, so once that array leaves
fx() a fallback and a good day are the same shape, and the count silently
becomes a count of something else.

On the two numbers not being substitutes — that changed what I store. It is one
integer per provider per month rather than a single running total, because "the
provider failed 40 times" and "it failed 40 times in one afternoon in March"
are different facts and only the second one is actionable.

Kept in a file rather than a table: it has to survive a deploy, and a counter
must never be the reason a price fails to render, so the whole thing sits
inside a try that swallows.

And it is printed in the daily report even when it reads zero. A counter nobody
looks at is the failure it was built to catch — which is your heartbeat point,
and the part I would not have got to on my own.

Thread Thread
 
vinhnguyenthanhdn profile image
Vinh Nguyen •

The try that swallows is the part I would instrument next, because it puts the counter in the same position the price was in. A zero in the daily report now reads either as no fallback happened or as countFailure threw on every call, and a file write is exactly the thing that starts failing quietly after a deploy changes the path or the owner. The cheapest fix that keeps the swallow is to increment a total on the success branch too, so the report prints failures against attempts. 0 of 8640 is a working instrument reporting good news and 0 of 0 is the instrument itself down, and right now those two print the same line.

Thread Thread
 
hammad4june1999 profile image
Hammad Shams Uddin •

You are right, and it is the same defect the post was about. A lone zero could
not tell "nothing failed" from "this has been throwing since the last deploy",
which is a value that cannot distinguish two states — exactly what I wrote 900
words complaining about.

Both outcomes are tallied now, and the report reads failures against attempts:

fx: 0 fallbacks of 8,640 calls this month.

and if the denominator is zero it is raised as a problem rather than passed
over, with the reason spelled out — 0 of 0 means the counter is not being
reached, which is not the same as nothing failing.

Your point about the file write specifically is the part I would not have got
to. It is not a hypothetical: this deploys by unpacking a tar over an existing
tree, so a path or an owner changing under the cache directory is a completely
ordinary Tuesday, and it would have failed in the one way that leaves the
report looking healthy.

Collapse
 
to21as profile image
Tobias •

The > 0 rather than a truthiness check is the part I would underline, because one layer down the same three-valued logic goes the other way. A Postgres CHECK only rejects a row when its expression is false, and NULL passes.

I hit that in a migration meant to stop a record going active without recorded proof of ownership. With the proof column NULL, jsonb_typeof(NULL) is NULL, the AND chain is NULL, and false OR NULL is not a violation, so the absent case was the one thing that slipped through the gate written to catch it. One leading IS NOT NULL fixed it.

Which makes me wonder about yours: does the new freshness() get a test that passes null, or only one that passes a number?

Collapse
 
hammad4june1999 profile image
Hammad Shams Uddin •

Thanks — and yes, both. The first two assertions in the file are
freshness(null) and freshness(undefined), because "age unknown" was the whole
missing branch and a test that only passes a number would agree with the buggy
version too.

Your Postgres example is the same three-valued logic pointed the other way,
and it's worse than mine: my null produced a reassuring sentence, yours
produced a passing CHECK. A constraint that only rejects on false and lets
NULL through is a gate that opens for exactly the row it was written to
stop. The leading IS NOT NULL is the same fix as my > 0 — say what you
require instead of trusting what's truthy.

View full discussion (15 comments)