From 8ba27ea6dd2f92566a3320bd772544fa550f2d0f Mon Sep 17 00:00:00 2001 From: AI Dev Date: Mon, 21 Sep 2026 11:49:20 +0000 Subject: [PATCH 1/2] Count a firmware download once, not once per range request (#188) 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. --- app/controllers/cameras/socs_controller.rb | 33 ++++- app/models/download.rb | 14 +++ test/controllers/download_counting_test.rb | 136 +++++++++++++++++++++ 3 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 test/controllers/download_counting_test.rb diff --git a/app/controllers/cameras/socs_controller.rb b/app/controllers/cameras/socs_controller.rb index 8fa5b9b4..a69462d9 100644 --- a/app/controllers/cameras/socs_controller.rb +++ b/app/controllers/cameras/socs_controller.rb @@ -222,7 +222,7 @@ def download_full_image FirmwareBuild.record(request.remote_ip) if assembling # Recorded here rather than in Firmware, because a cached image is sent # without being rebuilt and it is the sending that is worth counting. - Download.record(firmware: fw, soc: @soc, bytes: File.size(fw.filepath)) + Download.record(firmware: fw, soc: @soc, bytes: File.size(fw.filepath)) if first_chunk? send_file fw.filepath, filename: fw.filename, disposition: :attachment rescue Firmware::PayloadTooLarge => e # The combination is real but does not fit -- Ultimate on 8MB flash is @@ -285,6 +285,37 @@ def full_list private + # One row per download, not one per HTTP request (#188). + # + # A browser or download manager fetching an 8-32MB image asks for it in + # chunks, and each chunk is its own request through send_file's + # X-Accel-Redirect -- so the table counted requests. Unevenly, too: the + # 16MB ultimate images chunk hardest, so the chips people care most about + # were inflated most. Reconciled against the nginx log for 6-20 September + # 2026, 1,203 rows stood for 824 completed downloads, and on 10 September + # 99 range responses turned 55 real downloads into 155 rows. + # + # The first chunk is the download. A Range starting at byte 0 counts, and + # so does no Range at all; anything else is that same download continuing, + # whether it is a chunked fetch or a resumed one. A suffix range + # (`bytes=-500`) is not a first chunk either. + # + # Anchored, so a unit that merely ends in "bytes" does not look like one. + # Tolerant of surrounding and internal whitespace, because the cost of + # being strict here is the opposite failure and a worse one: a legitimate + # first chunk that goes uncounted makes the table quietly low, where the + # bug this replaces made it quietly high. Undercounting a download is + # indistinguishable from nobody downloading. + # + # Rows written before 2026-09-21 were not filtered this way. A chart that + # crosses that date is comparing two different things -- see the note on + # Download itself. + def first_chunk? + range = request.headers['Range'].to_s.strip + + range.empty? || range.match?(/\Abytes\s*=\s*0-/i) + end + # Read a configuration back out of the query string Camera#permalink writes. # # The keys are the permanent link's, and they have to stay in step with it: diff --git a/app/models/download.rb b/app/models/download.rb index d831700d..6134aa69 100644 --- a/app/models/download.rb +++ b/app/models/download.rb @@ -11,6 +11,20 @@ class Download < ApplicationRecord # Only created, never updated, so there is no updated_at to maintain. self.record_timestamps = false + # What a row means changed on 2026-09-21 (#188). + # + # Before that date a row was one HTTP request, and a chunked or resumed fetch + # wrote several -- so the table counted requests, not downloads, and counted + # the large images worst. From that date only the first chunk of a fetch is + # recorded, which is the download. + # + # The 2,558 rows written before it are left exactly as they are. Rewriting + # history would make the table agree with itself and disagree with the nginx + # log, which is the only independent record of what actually happened. A + # chart that crosses this date is comparing two different measurements, and + # should say so. + COUNTS_ONE_ROW_PER_DOWNLOAD_FROM = Date.new(2026, 9, 21) + # Recording must never cost somebody their download. A full disk, a locked # table, a migration not yet run on one container -- none of those are # reasons to fail a request that has already produced a valid image, so this diff --git a/test/controllers/download_counting_test.rb b/test/controllers/download_counting_test.rb new file mode 100644 index 00000000..cf7141a6 --- /dev/null +++ b/test/controllers/download_counting_test.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'test_helper' + +# One row per download, not one per HTTP request (#188). +# +# A browser or download manager fetching an 8-32MB image asks for it in chunks, +# and each chunk is its own request -- so the table counted requests. Unevenly, +# too: the 16MB ultimate images chunk hardest, so the chips people care most +# about were inflated most. +# +# Reconciled against the nginx log for 6-20 September 2026, 1,203 rows stood +# for 824 completed downloads. On 10 September, 99 range responses turned 55 +# real downloads into 155 rows, which read as a spike and was not one. +# +# The failure is quiet in the direction that matters: the table is the input to +# the monthly memo (#184), and a number that is 46% high looks exactly like a +# number that is right. +class DownloadCountingTest < ActionDispatch::IntegrationTest + setup do + @vendor = Vendor.create!(name: 'Counting Probe Vendor') + @soc = Soc.create!(vendor: @vendor, model: 'CNT1', status: 'done', + uboot_filename: 'u-boot-cnt1.bin', + linux_filename: 'openipc.cnt1-nor-lite.tgz') + @cache = Dir.mktmpdir + Firmware.cache_dir = @cache + # A cached image, so the action sends a file rather than assembling one -- + # which is also the path most real downloads take. + # + # Exactly eight megabytes and world-readable, because Firmware#usable? + # checks both: a NOR image is its flash size by construction, so a short + # file is treated as a leftover and rebuilt. Sparse, so it costs no disk. + name = Firmware.filename_for(soc_model: @soc.model_downcase, flash_type: 'nor', + release: 'lite', size: 8) + path = File.join(@cache, name) + File.open(path, 'wb') { |f| f.truncate(8.megabytes) } + File.chmod(0o644, path) + end + + teardown do + Firmware.cache_dir = nil + FileUtils.remove_entry(@cache) if @cache + end + + def download(range: nil) + headers = range ? { 'Range' => range } : {} + get "/cameras/vendors/#{@vendor.to_param}/socs/#{@soc.to_param}/download_full_image", + params: { flash_size: 8, flash_type: 'nor', fw_release: 'lite' }, headers: headers + end + + test 'a plain request counts once' do + assert_difference 'Download.count', 1 do + download + end + end + + # Download managers that always send a range, starting at the beginning. + # This is a download, and the only one it will send that starts at zero. + test 'a range that starts at the beginning counts once' do + assert_difference 'Download.count', 1 do + download range: 'bytes=0-1048575' + end + end + + test 'an open-ended range from the beginning counts once' do + assert_difference 'Download.count', 1 do + download range: 'bytes=0-' + end + end + + # Every chunk after the first, and every resumed fetch. Same download. + ['bytes=1048576-', 'bytes=1048576-2097151', 'bytes=33554432-'].each do |range| + test "a continuation range #{range} does not count again" do + assert_no_difference 'Download.count' do + download range: range + end + end + end + + # A header that arrives with whitespace is still a first chunk. Being strict + # here fails in the opposite direction to the bug this replaces, and a worse + # one: an uncounted download is indistinguishable from nobody downloading, + # where an over-counted one at least looks suspicious. + [' bytes=0-1023', 'bytes = 0-1023', "bytes=0-1023\t"].each do |range| + test "a first chunk sent as #{range.inspect} still counts" do + assert_difference 'Download.count', 1 do + download range: range + end + end + end + + # A unit that merely ends in "bytes" is not the bytes unit, and its range 0- + # is not our byte zero. + test 'a range in another unit does not count' do + assert_no_difference 'Download.count' do + download range: 'kbytes=0-4' + end + end + + # `bytes=-500` asks for the last 500 bytes. Not a first chunk. + test 'a suffix range does not count' do + assert_no_difference 'Download.count' do + download range: 'bytes=-500' + end + end + + # The shape of the real thing: one fetch, in chunks, is one download. + test 'a chunked fetch of one image writes one row' do + assert_difference 'Download.count', 1 do + download range: 'bytes=0-1048575' + 8.times { |i| download range: "bytes=#{(i + 1) * 1_048_576}-" } + end + end + + # And two people each downloading once are two, however they ask. + test 'two separate downloads are two rows' do + assert_difference 'Download.count', 2 do + download + download range: 'bytes=0-' + end + end + + # The row still says what it always said. The rule changed which requests + # write one, not what is written. + test 'the row it does write is unchanged' do + download + + row = Download.order(:id).last + + assert_equal @soc.model_downcase, row.soc_model + assert_equal 'nor', row.flash_type + assert_equal 'lite', row.release + assert_equal 8, row.flash_size + assert_operator row.bytes.to_i, :>, 0 + end +end From 4a96f43747366f961cadf3b73f0ca8d0d6df1fb6 Mon Sep 17 00:00:00 2001 From: AI Dev Date: Mon, 21 Sep 2026 12:12:29 +0000 Subject: [PATCH 2/2] Address review: a HEAD is not a download, and an ignored range is 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. --- app/controllers/cameras/socs_controller.rb | 19 ++++++++++- test/controllers/download_counting_test.rb | 38 +++++++++++++++++++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/app/controllers/cameras/socs_controller.rb b/app/controllers/cameras/socs_controller.rb index a69462d9..4b1dec56 100644 --- a/app/controllers/cameras/socs_controller.rb +++ b/app/controllers/cameras/socs_controller.rb @@ -307,13 +307,30 @@ def full_list # bug this replaces made it quietly high. Undercounting a download is # indistinguishable from nobody downloading. # + # A HEAD is not a download. Rails routes it to the same action and the + # reader gets no body, so counting it records a download that did not + # happen -- eighteen reached this path in the fourteen days of log the host + # keeps, against 1,871 GETs. Small, and wrong in the direction that invents + # readers. + # + # A range in a unit the file server does not implement is not a + # continuation either. An unrecognised unit is ignored and the whole + # representation sent: `Range: kbytes=0-4` comes back 200 with all + # 8,388,608 bytes. That is a download, and the first version of this rule + # dropped it -- with a test asserting the drop, which is how a bug gets + # written down as a rule. + # # Rows written before 2026-09-21 were not filtered this way. A chart that # crosses that date is comparing two different things -- see the note on # Download itself. def first_chunk? + return false unless request.get? + range = request.headers['Range'].to_s.strip + return true if range.empty? + return true unless range.match?(/\Abytes\s*=/i) - range.empty? || range.match?(/\Abytes\s*=\s*0-/i) + range.match?(/\Abytes\s*=\s*0-/i) end # Read a configuration back out of the query string Camera#permalink writes. diff --git a/test/controllers/download_counting_test.rb b/test/controllers/download_counting_test.rb index cf7141a6..b38af90d 100644 --- a/test/controllers/download_counting_test.rb +++ b/test/controllers/download_counting_test.rb @@ -89,11 +89,41 @@ def download(range: nil) end end - # A unit that merely ends in "bytes" is not the bytes unit, and its range 0- - # is not our byte zero. - test 'a range in another unit does not count' do + # A range in a unit the file server does not implement is ignored and the + # whole file is sent -- `Range: kbytes=0-4` comes back 200 with all 8,388,608 + # bytes. That is a download. + # + # The first version of this file asserted the opposite, which is how a bug + # gets written down as a rule: the reader received the entire image and the + # table recorded nothing. + ['kbytes=0-4', 'items=0-4', 'bytes-and-more=0-4'].each do |range| + test "a range in the unrecognised unit #{range.split('=').first} counts, because the whole file is sent" do + assert_difference 'Download.count', 1 do + download range: range + end + + assert_equal 200, response.status + assert_equal 8.megabytes, response.body.bytesize + end + end + + # A HEAD is a probe. Rails routes it to the same action and the reader gets + # no body, so counting it records a download that did not happen. Eighteen + # reached this path in fourteen days, against 1,871 GETs. + test 'a HEAD probe does not count as a download' do assert_no_difference 'Download.count' do - download range: 'kbytes=0-4' + head "/cameras/vendors/#{@vendor.to_param}/socs/#{@soc.to_param}/download_full_image", + params: { flash_size: 8, flash_type: 'nor', fw_release: 'lite' } + end + + assert_equal 0, response.body.bytesize + end + + test 'a probe followed by the real download counts once' do + assert_difference 'Download.count', 1 do + head "/cameras/vendors/#{@vendor.to_param}/socs/#{@soc.to_param}/download_full_image", + params: { flash_size: 8, flash_type: 'nor', fw_release: 'lite' } + download end end