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 8763a6d..7016c16 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 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 + + 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..8786d36 100644 --- a/models/item.rb +++ b/models/item.rb @@ -107,19 +107,132 @@ 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 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 (?) + 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. 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 + if missing.empty? + settled = true + break + end + + gained = missing.count { |id| store_missing id } + fetched += gained + 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 + + 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? + # 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) + 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 + + 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 +243,6 @@ def self.prefetch(id) else item = Item.create!(id: id, data: payload) end - item.prefetch_children [item.id, :refreshed] end @@ -170,16 +282,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/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 b9d992e..2a9a75d 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,167 @@ 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' }) + Item.hn_client = client + + 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( - 3 => { 'id' => 3, 'type' => 'comment', 'text' => 'grand' } + 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.prefetch 1 + Item.backfill [1] - assert Item.exists?(3) + 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 = 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 + + 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..3a5c881 100644 --- a/tests/worker_test.rb +++ b/tests/worker_test.rb @@ -96,4 +96,66 @@ 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 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, default: nil) + Item.hn_client = client + + with_env 'RECONCILE_BATCH' => '5' do + capture_log { Worker.run } + end + + assert_equal 60, Item.where.not(id: 1).count + # 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 + 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 + + 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 1dbb590..588b6e7 100644 --- a/worker.rb +++ b/worker.rb @@ -30,7 +30,18 @@ def self.run App.logger.info "Done fetching top stories (#{fetched.size})" end - Item.prune fetched + # 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 + App.logger.info "Backfilled #{added} new comments" + + Item.prune story_ids end def self.start