This is a submission for DEV's Summer Bug Smash: Smash Stories.
TL;DR. I bounded a database read to make my analyzer faster. I derived the bound carefully, wrote the reasoning into the KDoc, and shipped it behind five passing tests. The bound was wrong in a way none of those tests could see. The result: if a lifter deloaded once in the middle of a stall, which is the correct thing for a lifter to do, my app stopped telling them they had plateaued. No crash. No error. No log line. The feature just quietly stopped being true for the people using the app correctly.
The setup
WhyRep analyzes your training rather than just recording it. The core promise is that it tells you when you have stalled and what to change about it, and that every verdict traces back to a methodology document rather than to something a language model made up.
The architecture decision underneath that promise is that nothing is precomputed. Verdicts are derived from raw set logs on read, every time, so there is no cached judgement to go stale when the rules change.
Which means every read walked the lifter's entire history for every exercise in the session. That is fine at ten sessions. It is not fine at three hundred.
The obvious optimization is to bound the read. The obvious bound is "it only needs the last two weeks."
That was my first wrong answer, and it is worth thirty seconds before I get to the interesting one.
The plateau rules are not measured in calendar time. They are consecutive-miss counts, and the count varies by lifter tier and by whether the movement is a big or small joint action. The widest window in the signed methodology is an elite lifter on a small joint action: 14 consecutive sessions without progress. Train a lateral raise once a week and 14 sessions is over three months of data.
A 14-day cutoff could never have fired a plateau for anyone above beginner tier. It would not have thrown. It would have quietly stopped detecting the exact thing the product exists to detect.
The unit was wrong, not the number.
So I threw that out and derived a real bound from the rules table instead. That is where the actual story starts.
The bound I was proud of
Here is the reasoning, and I want you to notice that it is not sloppy. I wrote this out before writing the code:
- The widest plateau window is 14 consecutive misses.
-
ProgressionEnginerebuilds its baseline from a single session. Nothing accumulates across sessions, so only the miss streak needs historical depth. - The oldest row in a truncated window gets consumed as a fresh baseline and therefore cannot itself count as a miss.
- Therefore
maxWindow + 2rows is sufficient. Exact, even.
I put that reasoning in the KDoc so the next person would not have to rederive it. I wrote five tests. All five passed. I shipped it.
/**
* Rows to load for analysis.
*
* The widest plateau window is [PlateauWindows.MAX] consecutive misses. The
* oldest row in a truncated window is consumed as a fresh baseline and cannot
* count as a miss, so one spare row covers it. One more for safety.
*/
private const val ANALYSIS_ROW_BUDGET = PlateauWindows.MAX + 2
Read that comment again. It is confident, it is specific, it cites the right constant, and it is wrong.
Why it was wrong
consecutiveMisses does not skip only the first no-verdict session.
It skips every no-verdict session, and it does not reset the streak when it does. That behaviour is correct and deliberate. A session that produces no verdict is not evidence of progress and it is not evidence of a miss, so it should neither break the streak nor extend it. It should be transparent.
But transparent to the streak is not transparent to the row budget. Every skipped session still consumes a row.
And ProgressionEngine emits NO_VERDICT on four entirely ordinary paths:
- a flagged deload
- a weight change without earned overload
- a rep-range change
- a variant change
Every one of those eats a row from the budget while contributing nothing to the count.
I had budgeted exactly one spare row. There is no bound on how many are needed.
Step 3 of my reasoning was true. Step 4 assumed step 3 was the only case, and I never wrote down that assumption, so I never checked it.
Where the streak went. The deload consumed a row and contributed no miss, and the two rows that would have completed the streak fell off the end of the window.
What it costs a real lifter
I verified it rather than reasoning about it, because I had just learned what my reasoning was worth.
Setup: an ELITE-tier lifter, barbell curl, 40 weekly sessions, every one of them stalled at 30 kg for 8 reps. A textbook plateau, forty weeks long, impossible to miss.
| streak | plateau fired | |
|---|---|---|
| control | 15 | yes |
| one deload at session 34 | 13 | no |
One deload. In the middle of a forty-week stall. And the app stops saying the word "plateau."
Sit with the shape of that for a second, because it is worse than it first looks. Deloading during a stall is the correct thing to do. It is what a good lifter does, it is what my own app's coaching would tell them to do, and it is the single behaviour most likely to appear in the history of exactly the user who needs the plateau verdict most.
The MAJOR severity chip disappears. The documented rep-range fix disappears. The screen renders perfectly. It says nothing is wrong.
No crash. No error. No log line. Nothing to report, nothing to alert on, and nothing a user could file a bug about, because the app has no visible failure. It just quietly becomes a worse app for the people using it best.
This is a P0 coaching-logic regression introduced by a performance change. That is the category of change that was supposed to be safe.
The fix
The bound cannot be a constant.
That is the whole insight and it took me longer than I would like to get to it. The thing being counted (misses) and the thing being limited (rows) are not the same quantity, and no fixed ratio relates them. You cannot pick a number. Any number I pick is a number some sequence of deloads exceeds.
So loadForAnalysis starts at the nominal window and widens until the answer is provably settled:
private suspend fun loadForAnalysis(exerciseId: Long): List<SessionRow> {
var budget = PlateauWindows.MAX + 2
while (true) {
val rows = dao.recentRows(exerciseId, budget)
// Settled if any of these hold:
// 1. we reached the start of history, so there is nothing older
// 2. the window contains a PROGRESS verdict, which resets the streak,
// so nothing older can affect the answer
// 3. the streak already meets the widest window, so it cannot grow
// into a different verdict
if (rows.size < budget) return rows
if (rows.any { it.verdict == PROGRESS }) return rows
if (consecutiveMisses(rows) >= PlateauWindows.MAX) return rows
budget *= 2
}
}
Three termination conditions, each of them a proof that older rows cannot change the answer. Not a heuristic, and not a bigger constant.
The common case still settles on the first query. A lifter who is progressing hits condition 2 immediately, because a PROGRESS verdict is in the recent window by definition. The loop only widens for someone in a long unbroken stall, which is the exact population whose answer is worth paying an extra query for.
The test that guards it is named after the scenario rather than the mechanism:
AnalysisWindowTest > a deload inside a long stall must not hide the plateau
I checked that it fails against the old constant before I trusted it. It does.
Three exits, each one a proof that nothing older can change the verdict. The loop is not a retry, it is a search for sufficiency.
What I take from it
A performance change that preserves most behaviour is not a performance change. It is a behaviour change with a performance benefit, and it deserves the review a behaviour change gets. I had filed this work mentally under "optimization" and optimizations feel safe, so it got the review that optimizations get. The category was the mistake before the code was.
My tests passed because I wrote tests for the reasoning I had. This is the one that bothers me most, because there is no amount of discipline that fixes it directly. Five tests, all genuine, all passing, all derived from the same four-step argument that contained the hole. Tests written from your model of the system cannot find the part of the system your model is missing. They can only confirm the model.
Write down the assumption, not just the conclusion. My KDoc said "the oldest row is consumed as a baseline, so one spare covers it." What it did not say was "and that is the only reason a loaded row might not count." The moment you write that second sentence down you can see it is a claim, and a claim you can see is a claim you can check. I had done the thinking and recorded only the output of it.
Silent correctness regressions have no reporting surface. I have Sentry across three projects in this app now and it would not have caught this. There is no exception, no slow span, no failed request. A verdict that should have fired and did not produces exactly the same telemetry as a verdict that correctly did not fire. If your product's core value is a judgement call, your observability stack cannot see your core value.
The honest note on how this was found, since I would rather say it than imply otherwise: an automated review of my own pull request caught it. Not Sentry, not a user, not a test. Something else read the diff and asked why the spare was one and not two. It was right to ask.
Here is what I have not solved, and I would take suggestions.
I now have a test for this specific scenario. I do not have a general way to catch the class. A test suite can prove my analyzer does the right thing on the histories I thought of, and this bug lived entirely in a history I had not thought of.
Property-based testing over generated training histories is the obvious answer and I have not built it, mostly because writing a generator that produces plausible training histories is its own hard problem. Random ones would pass trivially. Realistic ones need the domain model I am trying to test.
If you have solved that circle for a domain of your own, I would genuinely like to hear how.
WhyRep is in closed testing on Play and launches in September.
Top comments (0)