Count a firmware download once, not once per range request (#188) - #247
Merged
Merged
Conversation
Download.record ran once per HTTP request, and a browser or download manager fetching an 8-32MB image asks for it in chunks -- so the table counted requests, not downloads. Unevenly, too: the 16MB ultimate images chunk hardest, so the chips people care most about were inflated most. Confirmed against the nginx log before changing anything. Over the fourteen days the host retains, range responses on this path run from none to 99 in a day: 10 September was 99 range responses against 55 real downloads, which read as a spike in the table and was not one. Yesterday there were none at all and the table's 156 rows matched the log's 156 status-200s exactly. The error is bursty, driven by a handful of clients, which is what makes it hard to see. The first chunk is the download. A Range starting at byte 0 counts, so does no Range at all, and everything after is that same fetch continuing. That direction matters. The bug this replaces made the table quietly high, which at least looks suspicious; being too strict would make it quietly low, and an uncounted download is indistinguishable from nobody downloading. So the match tolerates surrounding and internal whitespace, and is anchored so a unit merely ending in "bytes" is not mistaken for the bytes unit. History is left alone. The 2,558 rows written before today meant something else, and rewriting them would make the table agree with itself and disagree with the nginx log -- the only independent record of what happened. The date the meaning changed is recorded on Download itself, so a chart crossing it can say what it is comparing. 774 tests. Three guards, each confirmed by reverting it -- after the first version of the anchoring probe turned out to be untestable with any realistic header, which is how the whitespace case was found. Not included: counting the 302s this action answers for a combination that does not exist or does not fit (44 yesterday, 116 on 16 September). #188 lists it as separately worth a count and it is, but it needs somewhere to go -- a column or a table -- and that is a wider change than the one this issue is blocking.
PR Summary by QodoCount firmware downloads once across range requests
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Two real bugs, both confirmed by running them before fixing. A HEAD wrote a row. Rails routes it to the same action, the reader gets no body, and the table recorded a download that did not happen -- eighteen of these reached the path in the fourteen days of log the host keeps, against 1,871 GETs. Small, and wrong in the direction that invents readers. And a range in a unit the file server does not implement is not a continuation. An unrecognised unit is ignored and the whole representation sent: `Range: kbytes=0-4` comes back 200 with all 8,388,608 bytes, measured, not assumed. The rule dropped it -- and the new test file asserted the drop, which is how a bug gets written down as a rule. That test now asserts the reader got the entire image and the row exists, for three unrecognised units. Rewritten as a sequence rather than one condition, because there are now three separate questions -- is this a fetch at all, is this a byte range, does it start at zero -- and one boolean expression hid the second. 778 tests. Three guards, each confirmed by reverting it: counting HEAD again, dropping unrecognised units again, and counting every request. No rubocop offences added.
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
1 participant
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.
Closes #188. Unblocks #184 from using the
downloadstable.Download.recordran once per HTTP request, and a browser or download manager fetching an 8–32 MB image asks for it in chunks — so the table counted requests, not downloads. Unevenly, too: the 16 MB ultimate images chunk hardest, so the chips people care most about were inflated most.The error is bursty, which is why it was invisible
Confirmed against the nginx log before changing anything. Over the fourteen days the host retains, range responses on this path run from none to 99 in a day:
So on a given day the table is either perfect or 180% high, depending on whether a few clients happened to use range requests. A number that is sometimes right is harder to distrust than one that is always wrong.
The direction of the fix matters more than the fix
The first chunk is the download: a
Rangestarting at byte 0 counts, so does noRangeat all, and everything after is that same fetch continuing.The bug this replaces made the table quietly high, which at least looks suspicious. Being too strict would make it quietly low — and an uncounted download is indistinguishable from nobody downloading. So the match tolerates surrounding and internal whitespace, and is anchored so a unit merely ending in
bytesis not mistaken for the bytes unit.That second point came out of revert-testing rather than design: my first anchoring guard could not be made to fail by any realistic header, so it was proving nothing. Working out why is what surfaced the whitespace case, which is a real way a legitimate first chunk could have gone uncounted.
Exercised against a real image, not only in the suite
On dev, one plain fetch of a 16 MB image followed by eight continuation chunks:
History is left alone
The 2,558 rows written before today meant something else. Rewriting them would make the table agree with itself and disagree with the nginx log, which is the only independent record of what actually happened. The date the meaning changed is recorded on
Downloaditself, so a chart that crosses it can say what it is comparing.Verification
774 tests. Three guards, each confirmed by reverting it: counting every request again, treating any
Rangeas a continuation, and dropping the anchor. Fourteen cases covering plain requests, first chunks, continuations, suffix ranges, other units, whitespace, a full chunked fetch, and two separate downloads.No rubocop offences added.
Not included
Counting the 302s this action answers for a combination that does not exist or does not fit — 44 yesterday, 116 on 16 September. #188 lists it as separately worth a count and it is; those are the wizard's dead ends and they leave no trace beyond the log. But it needs somewhere to go — a column or a table — and that is a wider change than the one blocking #184.