From 0a21d2d300a761069d83e6930e86652c4264f4d4 Mon Sep 17 00:00:00 2001 From: Eric Boehs Date: Sat, 29 Aug 2026 20:57:11 -0500 Subject: [PATCH 1/2] perf: drive the worker off HN's updates feed instead of re-walking every thread The worker had never completed a single cycle. In 40 minutes of uptime it logged 1 cycle start, 0 completions, and 33,123 API requests, and 96% of that work was re-fetching items it already had (5,518 rows updated versus 210 created in a 10 minute window). The cause was the traversal, not the volume. prefetch_children called Item.prefetch on every kid, and prefetch re-descended even on a cache hit, so each cycle walked all ~47,000 cached nodes to discover changes. The 10 minute freshness window then expired mid-walk, so everything was stale again before the walk came back around -- a treadmill that could not converge. HN publishes what changed. updates.json returned 40 item ids when sampled, against the 5,518 rows the worker was re-fetching to find the same information. The cycle is now: - refresh the ids in the updates feed that we actually cache - backfill descendants we are missing - prune to the current top-story list Discovery of missing comments moved from Ruby recursion into one recursive CTE. Batching all roots into a single query measured 1.7s across all 504 stories, versus ~135ms per story (67s) one at a time. prefetch no longer recurses; it refreshes the top-story rows only, which bounds it at ~500 requests per cycle. Tests cover the property that regressed: a fully cached 60-comment tree now costs zero per-comment requests in a steady-state cycle, and a cycle with one new reply fetches exactly that one reply. --- lib/hacker_news_api/client.rb | 10 ++++ models/item.rb | 101 +++++++++++++++++++++++++++----- tests/item_test.rb | 107 +++++++++++++++++++++++++++++----- tests/test_helper.rb | 5 ++ tests/worker_test.rb | 37 ++++++++++++ worker.rb | 10 +++- 6 files changed, 239 insertions(+), 31 deletions(-) diff --git a/lib/hacker_news_api/client.rb b/lib/hacker_news_api/client.rb index 8763a6d..210a6b8 100644 --- a/lib/hacker_news_api/client.rb +++ b/lib/hacker_news_api/client.rb @@ -38,6 +38,16 @@ def top_story_ids get "#{base_url}/topstories.json" end + # Item ids HN reports as changed since its last publish. Polling this is what + # lets the worker refresh only what moved instead of re-walking every + # cached thread looking for changes. + def updated_item_ids + payload = get "#{base_url}/updates.json" + return nil unless payload + + payload['items'] + end + def best_story_ids get "#{base_url}/beststories.json" end diff --git a/models/item.rb b/models/item.rb index 39f6bc1..7b57a5e 100644 --- a/models/item.rb +++ b/models/item.rb @@ -107,19 +107,99 @@ def self.title_matches(search) where "(data->>'title') ilike ?", "%#{search}%" end + # Every uncached child referenced anywhere in the cached trees, found in one + # query for all roots at once. This replaces walking the tree in Ruby: the old + # worker called prefetch on all ~47,000 cached nodes every cycle just to + # discover the handful that were new. Measured on production, one batched + # query covers every story in ~1.7s where per-story queries cost ~135ms each. + MISSING_DESCENDANTS_SQL = <<~SQL + WITH RECURSIVE tree AS ( + SELECT i.id, i.data FROM items i WHERE i.id IN (?) + UNION ALL + SELECT c.id, c.data + FROM tree t + CROSS JOIN LATERAL jsonb_array_elements_text(t.data->'kids') AS k(kid) + JOIN items c ON c.id = k.kid::bigint + ) CYCLE id SET is_cycle USING path + SELECT DISTINCT k.kid::bigint AS id + FROM tree t + CROSS JOIN LATERAL jsonb_array_elements_text(t.data->'kids') AS k(kid) + WHERE NOT EXISTS (SELECT 1 FROM items i WHERE i.id = k.kid::bigint) + SQL + + def self.missing_descendant_ids(root_ids) + root_ids = Array(root_ids).compact + return [] if root_ids.empty? + + connection.select_values(sanitize_sql_array([MISSING_DESCENDANTS_SQL, root_ids])).map(&:to_i) + end + + # Fetches only the descendants we do not already have. Each round can reveal + # another level (a newly fetched comment brings its own kids), so it repeats + # until nothing new arrives. Rounds are capped so a thread that keeps failing + # to fetch cannot spin. + def self.backfill(root_ids, max_rounds: 25) + fetched = 0 + + max_rounds.times do + missing = missing_descendant_ids root_ids + break if missing.empty? + + gained = missing.count { |id| store_missing id } + fetched += gained + # Every id in this round failed or returned null (deleted items stay + # uncacheable), so another round would repeat the same requests. + break if gained.zero? + end + + fetched + end + + def self.store_missing(id) + payload = hn_client.item id + return false unless payload + + create! id: id, data: payload + true + rescue ActiveRecord::RecordNotUnique + false + end + + # Refreshes the items HN says changed, limited to ones we already cache. + # The updates feed covers all of HN, so most ids are irrelevant to us. + def self.sync_updates + ids = hn_client.updated_item_ids + if ids.nil? + App.logger.warn 'Updates feed unavailable; skipping refresh' + return nil + end + + known = where(id: ids).pluck(:id) + refreshed = known.count { |id| refresh id } + App.logger.info "Updates feed: #{ids.size} changed, #{known.size} cached, #{refreshed} refreshed" + refreshed + end + + def self.refresh(id) + item = find_by id: id + return false unless item + + payload = hn_client.item id + return false unless payload + + item.update! data: payload, updated_at: Time.now.utc + true + end + def self.prefetch(id) item = Item.find_by id: id - if item && item.updated_at > 10.minutes.ago - item.prefetch_children - return [id, :fresh] - end + return [id, :fresh] if item && item.updated_at > 10.minutes.ago payload = hn_client.item id unless payload if item App.logger.warn "Using stale cache for item #{id}; fetch failed" - item.prefetch_children return [id, :stale] end return [nil, :missing] @@ -130,7 +210,6 @@ def self.prefetch(id) else item = Item.create!(id: id, data: payload) end - item.prefetch_children [item.id, :refreshed] end @@ -170,16 +249,6 @@ def self.prune(keep_ids) deleted end - def prefetch_children - if data.nil? - App.logger.error "Item #{id} has nil data; skipping children" - return - end - return unless data['kids'] - - data['kids'].each { |kid| Item.prefetch kid } - end - def truncated_url if data.nil? App.logger.error "Item #{id} has nil data" diff --git a/tests/item_test.rb b/tests/item_test.rb index b9d992e..7921444 100644 --- a/tests/item_test.rb +++ b/tests/item_test.rb @@ -43,21 +43,31 @@ def test_failed_payload_without_cache_returns_nil assert_equal 0, Item.count end - def test_one_failed_kid_does_not_abort_siblings - create_item id: 1, kids: [2, 3], updated_at: 1.hour.ago + def test_backfill_continues_after_a_failed_kid + create_item id: 1, kids: [2, 3] Item.hn_client = FakeHnClient.new( - 1 => { 'id' => 1, 'type' => 'story', 'kids' => [2, 3] }, 2 => nil, 3 => { 'id' => 3, 'type' => 'comment', 'text' => 'ok' } ) - Item.prefetch 1 + capture_log { Item.backfill [1] } refute Item.exists?(2) assert Item.exists?(3) assert_equal 'ok', Item.find(3).data['text'] end + def test_backfill_stops_when_a_round_gains_nothing + create_item id: 1, kids: [2] + client = FakeHnClient.new(2 => nil) + Item.hn_client = client + + capture_log { Item.backfill [1] } + + # One attempt, not one per round: a permanently dead id must not spin. + assert_equal [2], client.calls + end + def test_stories_scope create_item id: 1, type: 'story', title: 'A story' create_item id: 2, type: 'comment', title: 'A comment' @@ -79,31 +89,100 @@ def test_min_score assert_equal [2], Item.min_score(200).pluck(:id) end - def test_nil_data_does_not_look_like_missing_kids - item = create_item id: 1 - item.define_singleton_method(:data) { nil } + def test_backfill_handles_a_story_with_no_kids + create_item id: 1 client = FakeHnClient.new Item.hn_client = client - item.prefetch_children - + assert_equal 0, Item.backfill([1]) assert_empty client.calls end - def test_prefetch_walks_children_of_fresh_nodes + # Freshness must not gate discovery: a brand new reply under a long-cached + # comment still has to be found. + def test_backfill_fetches_descendants_under_fresh_parents create_item id: 1, kids: [2], updated_at: 1.minute.ago create_item id: 2, kids: [3], type: 'comment', updated_at: 1.minute.ago - client = FakeHnClient.new( - 3 => { 'id' => 3, 'type' => 'comment', 'text' => 'grand' } - ) + client = FakeHnClient.new(3 => { 'id' => 3, 'type' => 'comment', 'text' => 'grand' }) Item.hn_client = client - Item.prefetch 1 + Item.backfill [1] assert Item.exists?(3) assert_equal [3], client.calls end + def test_backfill_walks_newly_revealed_levels + create_item id: 1, kids: [2] + client = FakeHnClient.new( + 2 => { 'id' => 2, 'type' => 'comment', 'kids' => [3] }, + 3 => { 'id' => 3, 'type' => 'comment', 'kids' => [4] }, + 4 => { 'id' => 4, 'type' => 'comment', 'text' => 'deep' } + ) + Item.hn_client = client + + Item.backfill [1] + + assert_equal [1, 2, 3, 4], Item.order(:id).pluck(:id) + assert_equal [2, 3, 4], client.calls + end + + def test_backfill_covers_all_roots_in_one_pass + create_item id: 1, kids: [3] + create_item id: 2, kids: [4] + client = FakeHnClient.new( + 3 => { 'id' => 3, 'type' => 'comment' }, + 4 => { 'id' => 4, 'type' => 'comment' } + ) + Item.hn_client = client + + assert_equal 2, Item.backfill([1, 2]) + assert_equal [3, 4], client.calls.sort + end + + def test_backfill_with_no_roots_makes_no_requests + client = FakeHnClient.new + Item.hn_client = client + + assert_equal 0, Item.backfill([]) + assert_empty client.calls + end + + def test_backfill_makes_no_requests_when_nothing_is_missing + create_item id: 2, type: 'comment' + create_item id: 1, kids: [2] + client = FakeHnClient.new + Item.hn_client = client + + assert_equal 0, Item.backfill([1]) + assert_empty client.calls + end + + def test_sync_updates_refreshes_only_cached_items + create_item id: 1, title: 'Old' + client = FakeHnClient.new( + updated_item_ids: [1, 999], + 1 => { 'id' => 1, 'type' => 'story', 'title' => 'New' }, + 999 => { 'id' => 999, 'type' => 'comment' } + ) + Item.hn_client = client + + refreshed = capture_log { Item.sync_updates } + assert_equal 1, Item.count + + assert_equal 'New', Item.find(1).data['title'] + refute_includes client.calls, 999 + end + + def test_sync_updates_returns_nil_when_feed_unavailable + client = FakeHnClient.new(updated_item_ids: nil) + Item.hn_client = client + + log = capture_log { assert_nil Item.sync_updates } + + assert_match(/Updates feed unavailable/, log) + end + def test_top_stories_keeps_successes_and_drops_failures Item.hn_client = FakeHnClient.new( top_story_ids: [1, 2, 3], diff --git a/tests/test_helper.rb b/tests/test_helper.rb index 2098341..3d61290 100644 --- a/tests/test_helper.rb +++ b/tests/test_helper.rb @@ -66,6 +66,11 @@ def top_story_ids @calls << :top_story_ids @payloads[:top_story_ids] end + + def updated_item_ids + @calls << :updated_item_ids + @payloads[:updated_item_ids] + end end module LogCapture diff --git a/tests/worker_test.rb b/tests/worker_test.rb index 935a36a..c5674d5 100644 --- a/tests/worker_test.rb +++ b/tests/worker_test.rb @@ -96,4 +96,41 @@ def test_interval_rejects_negative ensure ENV.delete 'WORKER_INTERVAL' end + + # The old worker called prefetch on every cached node each cycle, so a fully + # cached, unchanged tree still cost one API request per comment. A steady + # state cycle must now cost a bounded number of requests regardless of how + # many comments are cached. + def test_steady_state_cycle_does_not_touch_every_cached_node + story = { 'id' => 1, 'type' => 'story', 'title' => 'S', 'kids' => (2..61).to_a } + create_item id: 1, kids: (2..61).to_a, updated_at: 1.minute.ago + (2..61).each { |i| create_item id: i, type: 'comment', updated_at: 1.minute.ago } + + client = FakeHnClient.new(top_story_ids: [1], updated_item_ids: [], 1 => story) + Item.hn_client = client + + capture_log { Worker.run } + + item_calls = client.calls.grep(Integer) + + assert_equal 60, Item.where.not(id: 1).count + assert_empty item_calls, "expected no per-comment requests, got #{item_calls.size}" + end + + def test_cycle_fetches_only_genuinely_new_comments + create_item id: 1, kids: [2, 3], updated_at: 1.minute.ago + create_item id: 2, type: 'comment', updated_at: 1.minute.ago + + client = FakeHnClient.new( + top_story_ids: [1], + updated_item_ids: [], + 3 => { 'id' => 3, 'type' => 'comment', 'text' => 'new' } + ) + Item.hn_client = client + + capture_log { Worker.run } + + assert_equal [3], client.calls.grep(Integer) + assert Item.exists?(3) + end end diff --git a/worker.rb b/worker.rb index 1dbb590..f08b59f 100644 --- a/worker.rb +++ b/worker.rb @@ -30,7 +30,15 @@ def self.run App.logger.info "Done fetching top stories (#{fetched.size})" end - Item.prune fetched + # Refresh what HN says changed, then pull in comments we do not have yet. + # Together these replace the old full re-walk of every cached thread. + Item.sync_updates + + story_ids = fetched.pluck :id + added = Item.backfill story_ids + App.logger.info "Backfilled #{added} new comments" + + Item.prune story_ids end def self.start From 93ca402a03328b55c3b1d504e4edf49a999467c8 Mon Sep 17 00:00:00 2001 From: Eric Boehs Date: Sat, 29 Aug 2026 21:10:58 -0500 Subject: [PATCH 2/2] fix: add a reconcile sweep so a missed update cannot strand a reply Review caught a real correctness gap in the updates-feed design. updates.json is a snapshot of what changed at poll time, not a cursor that can be replayed. If a reply lands between polls, the parent's entry leaves the feed before the worker sees it, so the cached kids list stays stale -- and because discovery reads our own cached kids, that reply could never be found. Item.reconcile re-fetches the least recently updated rows each cycle, so every cached row is eventually re-checked regardless of what the feed dropped. Cost is fixed per cycle rather than proportional to cache size, and RECONCILE_BATCH tunes convergence time against API load. The steady-state worker test asserted zero requests, which the sweep deliberately breaks. Replaced with the invariant that actually matters: cost tracks the reconcile budget, not the number of cached comments. Also from review: an unavailable feed logs at error rather than warn and explains that the sweep is the fallback, failed refresh ids are logged instead of collapsed into a count, and exhausting the backfill round cap warns instead of silently returning a partial result. Comment fixes: updates.json is not a "since last publish" delta, and tombstones are cached (a non-nil payload with deleted: true), so the claim that deleted items stay uncacheable was wrong. Tests: the updated-parent-reveals-new-reply integration path, refresh failure preserving cached data, reconcile ordering, backfill cycle termination, an explicit round cap, and the real updates.json shape. --- .env.example | 4 ++ lib/hacker_news_api/client.rb | 6 +-- models/item.rb | 57 +++++++++++++++++----- tests/hacker_news_api/client_test.rb | 24 ++++++++++ tests/item_test.rb | 71 +++++++++++++++++++++++++++- tests/worker_test.rb | 45 ++++++++++++++---- worker.rb | 7 ++- 7 files changed, 185 insertions(+), 29 deletions(-) diff --git a/.env.example b/.env.example index 4bf102e..802213d 100644 --- a/.env.example +++ b/.env.example @@ -4,4 +4,8 @@ DATABASE_URL=postgres:///hackinews_development # Optional # PORT=3000 # WORKER_INTERVAL=300 +# Least-recently-updated items re-checked each cycle. HN's updates feed can +# drop changes between polls, so this sweep is what guarantees the cache +# eventually converges. Raise it to converge faster, lower it to cut API load. +# RECONCILE_BATCH=500 # GA_TRACKING_ID= diff --git a/lib/hacker_news_api/client.rb b/lib/hacker_news_api/client.rb index 210a6b8..7016c16 100644 --- a/lib/hacker_news_api/client.rb +++ b/lib/hacker_news_api/client.rb @@ -38,9 +38,9 @@ def top_story_ids get "#{base_url}/topstories.json" end - # Item ids HN reports as changed since its last publish. Polling this is what - # lets the worker refresh only what moved instead of re-walking every - # cached thread looking for changes. + # Item ids in HN's current updates response. This is a snapshot of what + # changed recently, not a replayable cursor: ids drop off as new ones + # arrive, so a slow poller can miss changes entirely. def updated_item_ids payload = get "#{base_url}/updates.json" return nil unless payload diff --git a/models/item.rb b/models/item.rb index 7b57a5e..8786d36 100644 --- a/models/item.rb +++ b/models/item.rb @@ -109,9 +109,10 @@ def self.title_matches(search) # Every uncached child referenced anywhere in the cached trees, found in one # query for all roots at once. This replaces walking the tree in Ruby: the old - # worker called prefetch on all ~47,000 cached nodes every cycle just to - # discover the handful that were new. Measured on production, one batched - # query covers every story in ~1.7s where per-story queries cost ~135ms each. + # worker called prefetch on every cached node each cycle just to discover the + # handful that were new. Batching all roots into a single query rather than + # one query per story is a large measured saving. Traversal starts only from + # roots that are already cached; ids not in the table discover nothing. MISSING_DESCENDANTS_SQL = <<~SQL WITH RECURSIVE tree AS ( SELECT i.id, i.data FROM items i WHERE i.id IN (?) @@ -136,22 +137,29 @@ def self.missing_descendant_ids(root_ids) # Fetches only the descendants we do not already have. Each round can reveal # another level (a newly fetched comment brings its own kids), so it repeats - # until nothing new arrives. Rounds are capped so a thread that keeps failing - # to fetch cannot spin. + # until nothing new arrives. A round that inserts nothing stops immediate + # retries of unavailable ids; max_rounds bounds how deep one pass will chase + # newly revealed levels when inserts keep succeeding. def self.backfill(root_ids, max_rounds: 25) fetched = 0 + settled = false max_rounds.times do missing = missing_descendant_ids root_ids - break if missing.empty? + if missing.empty? + settled = true + break + end gained = missing.count { |id| store_missing id } fetched += gained - # Every id in this round failed or returned null (deleted items stay - # uncacheable), so another round would repeat the same requests. - break if gained.zero? + if gained.zero? + settled = true + break + end end + App.logger.warn "Backfill hit its #{max_rounds}-round cap; descendants remain uncached" unless settled fetched end @@ -170,13 +178,38 @@ def self.store_missing(id) def self.sync_updates ids = hn_client.updated_item_ids if ids.nil? - App.logger.warn 'Updates feed unavailable; skipping refresh' + # Not fatal: the reconcile sweep below is the safety net, so a failed poll + # delays convergence rather than breaking it. + App.logger.error 'Updates feed unavailable; relying on reconcile sweep' return nil end known = where(id: ids).pluck(:id) - refreshed = known.count { |id| refresh id } - App.logger.info "Updates feed: #{ids.size} changed, #{known.size} cached, #{refreshed} refreshed" + failed = known.reject { |id| refresh id } + App.logger.warn "Updates refresh failed for: #{failed.join(', ')}" if failed.any? + App.logger.info "Updates feed: #{ids.size} changed, #{known.size} cached, #{known.size - failed.size} refreshed" + known.size - failed.size + end + + # How many least-recently-updated items each cycle re-checks. Tunable so the + # sweep can be traded against convergence time without a deploy. + def self.reconcile_batch + Integer ENV.fetch('RECONCILE_BATCH', '500') + end + + # The updates feed is a snapshot of what changed at poll time, not a cursor we + # can replay. An update published between polls is gone before we see it, and + # because discovery reads our own cached kids list, a reply we never learned + # about can never be found. This sweep re-fetches the least recently updated + # items so every cached row is eventually re-checked no matter what the feed + # missed, at a cost per cycle that is fixed rather than proportional to the + # size of the cache. + def self.reconcile(limit: reconcile_batch) + ids = order(:updated_at).limit(limit).pluck(:id) + return 0 if ids.empty? + + refreshed = ids.count { |id| refresh id } + App.logger.info "Reconciled #{refreshed}/#{ids.size} least recently updated items" refreshed end diff --git a/tests/hacker_news_api/client_test.rb b/tests/hacker_news_api/client_test.rb index 4e44c36..df64131 100644 --- a/tests/hacker_news_api/client_test.rb +++ b/tests/hacker_news_api/client_test.rb @@ -30,6 +30,30 @@ def test_top_story_ids_parses_json end end + # Guards the response shape: updates.json returns an object, not an array, + # so returning the whole payload would break `where(id: ids)`. + def test_updated_item_ids_extracts_the_items_key + body = '{"items":[1,2,3],"profiles":["pg"]}' + stub_http(response(code: '200', body: body)) do + assert_equal [1, 2, 3], client.updated_item_ids + end + end + + def test_updated_item_ids_returns_nil_on_failure + stub_http(response(code: '500', body: 'nope')) do + assert_nil client.updated_item_ids + end + end + + def test_updated_item_ids_requests_the_updates_endpoint + requested = nil + client.stub :get, ->(url) { requested = url } do + client.updated_item_ids + end + + assert_match(%r{/v0/updates\.json\z}, requested) + end + def test_item_parses_json stub_http(response(code: '200', body: '{"id":42,"score":10}')) do assert_equal({ 'id' => 42, 'score' => 10 }, client.item(42)) diff --git a/tests/item_test.rb b/tests/item_test.rb index 7921444..2a9a75d 100644 --- a/tests/item_test.rb +++ b/tests/item_test.rb @@ -167,13 +167,80 @@ def test_sync_updates_refreshes_only_cached_items ) Item.hn_client = client - refreshed = capture_log { Item.sync_updates } - assert_equal 1, Item.count + refreshed = nil + capture_log { refreshed = Item.sync_updates } + assert_equal 1, refreshed + assert_equal 1, Item.count assert_equal 'New', Item.find(1).data['title'] refute_includes client.calls, 999 end + def test_sync_updates_keeps_cached_data_when_an_item_fetch_fails + create_item id: 1, title: 'Old' + Item.hn_client = FakeHnClient.new(updated_item_ids: [1], 1 => nil) + + refreshed = nil + log = capture_log { refreshed = Item.sync_updates } + + assert_equal 0, refreshed + assert_equal 'Old', Item.find(1).data['title'] + assert_match(/Updates refresh failed for: 1/, log) + end + + # The updates feed can miss changes, so every cached row must eventually be + # re-checked on its own. + def test_reconcile_refreshes_least_recently_updated_first + create_item id: 1, title: 'Stalest', updated_at: 3.hours.ago + create_item id: 2, title: 'Stale', updated_at: 2.hours.ago + create_item id: 3, title: 'Recent', updated_at: 1.minute.ago + client = FakeHnClient.new( + 1 => { 'id' => 1, 'type' => 'story', 'title' => 'Fresh 1' }, + 2 => { 'id' => 2, 'type' => 'story', 'title' => 'Fresh 2' } + ) + Item.hn_client = client + + refreshed = nil + capture_log { refreshed = Item.reconcile(limit: 2) } + + assert_equal 2, refreshed + assert_equal [1, 2], client.calls.sort + assert_equal 'Recent', Item.find(3).data['title'] + end + + def test_reconcile_with_an_empty_cache_makes_no_requests + client = FakeHnClient.new + Item.hn_client = client + + assert_equal 0, Item.reconcile + assert_empty client.calls + end + + def test_backfill_terminates_on_a_cycle + create_item id: 1, kids: [2] + create_item id: 2, type: 'comment', kids: [1, 3] + client = FakeHnClient.new(3 => { 'id' => 3, 'type' => 'comment' }) + Item.hn_client = client + + assert_equal 1, Item.backfill([1]) + assert_equal [3], client.calls + end + + def test_backfill_respects_an_explicit_round_cap + create_item id: 1, kids: [2] + client = FakeHnClient.new( + 2 => { 'id' => 2, 'type' => 'comment', 'kids' => [3] }, + 3 => { 'id' => 3, 'type' => 'comment', 'kids' => [4] }, + 4 => { 'id' => 4, 'type' => 'comment' } + ) + Item.hn_client = client + + log = capture_log { Item.backfill([1], max_rounds: 2) } + + assert_equal [2, 3], client.calls + assert_match(/hit its 2-round cap/, log) + end + def test_sync_updates_returns_nil_when_feed_unavailable client = FakeHnClient.new(updated_item_ids: nil) Item.hn_client = client diff --git a/tests/worker_test.rb b/tests/worker_test.rb index c5674d5..3a5c881 100644 --- a/tests/worker_test.rb +++ b/tests/worker_test.rb @@ -98,23 +98,23 @@ def test_interval_rejects_negative end # The old worker called prefetch on every cached node each cycle, so a fully - # cached, unchanged tree still cost one API request per comment. A steady - # state cycle must now cost a bounded number of requests regardless of how - # many comments are cached. - def test_steady_state_cycle_does_not_touch_every_cached_node + # cached, unchanged tree cost one API request per comment. Cost is now set by + # the reconcile budget, not by how much is cached. + def test_steady_state_cycle_cost_is_bounded_by_the_reconcile_budget story = { 'id' => 1, 'type' => 'story', 'title' => 'S', 'kids' => (2..61).to_a } create_item id: 1, kids: (2..61).to_a, updated_at: 1.minute.ago (2..61).each { |i| create_item id: i, type: 'comment', updated_at: 1.minute.ago } - client = FakeHnClient.new(top_story_ids: [1], updated_item_ids: [], 1 => story) + client = FakeHnClient.new(top_story_ids: [1], updated_item_ids: [], 1 => story, default: nil) Item.hn_client = client - capture_log { Worker.run } - - item_calls = client.calls.grep(Integer) + with_env 'RECONCILE_BATCH' => '5' do + capture_log { Worker.run } + end assert_equal 60, Item.where.not(id: 1).count - assert_empty item_calls, "expected no per-comment requests, got #{item_calls.size}" + # 5 from the sweep, and nothing proportional to the 60 cached comments. + assert_equal 5, client.calls.grep(Integer).size end def test_cycle_fetches_only_genuinely_new_comments @@ -128,9 +128,34 @@ def test_cycle_fetches_only_genuinely_new_comments ) Item.hn_client = client - capture_log { Worker.run } + with_env 'RECONCILE_BATCH' => '0' do + capture_log { Worker.run } + end assert_equal [3], client.calls.grep(Integer) assert Item.exists?(3) end + + # The gap the reconcile sweep exists to close, end to end: HN reports a + # cached comment as changed, its refreshed kids list reveals a new reply, + # and backfill must pull that reply in during the same cycle. + def test_updated_comment_reveals_and_backfills_a_new_reply + create_item id: 1, kids: [2], updated_at: 1.minute.ago + create_item id: 2, type: 'comment', updated_at: 1.minute.ago + + client = FakeHnClient.new( + top_story_ids: [1], + updated_item_ids: [2], + 2 => { 'id' => 2, 'type' => 'comment', 'kids' => [3] }, + 3 => { 'id' => 3, 'type' => 'comment', 'text' => 'the new reply' } + ) + Item.hn_client = client + + with_env 'RECONCILE_BATCH' => '0' do + capture_log { Worker.run } + end + + assert Item.exists?(3), 'new reply should be discovered via the updated parent' + assert_equal 'the new reply', Item.find(3).data['text'] + end end diff --git a/worker.rb b/worker.rb index f08b59f..588b6e7 100644 --- a/worker.rb +++ b/worker.rb @@ -30,9 +30,12 @@ def self.run App.logger.info "Done fetching top stories (#{fetched.size})" end - # Refresh what HN says changed, then pull in comments we do not have yet. - # Together these replace the old full re-walk of every cached thread. + # Refresh what HN says changed, then re-check a slice of the cache the feed + # may have missed, then pull in comments we still do not have. Discovering a + # new reply depends on its parent's kids list being current, which is why + # both refresh passes run before backfill. Item.sync_updates + Item.reconcile story_ids = fetched.pluck :id added = Item.backfill story_ids