Repository navigation
perf: drive the worker off HN's updates feed instead of re-walking every thread - #29
Merged
Merged
Conversation
…ery 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.
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.
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.
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.
Why
The cause was the traversal, not the volume.
prefetch_childrencalledItem.prefetchon every kid, andprefetchre-descended even on a cache hit, so each cycle walked all ~47,000 cached nodes just to discover changes. The 10-minute freshness window then expired mid-walk, so everything was stale again before the walk came back around.Measured over one 10-minute window:
96% of the work was rediscovering things it already knew. HN publishes exactly this information:
updates.jsonreturned 40 ids when sampled.The new cycle
Discovery of missing comments moved from Ruby recursion into one recursive CTE. Batching all roots into a single query measured 1.7 s across all 504 stories, versus ~135 ms per story (67 s) one at a time.
prefetchno longer recurses — it refreshes top-story rows only, bounding it at ~500 requests per cycle.The gap review caught
updates.jsonis a snapshot, not a replayable cursor. If a reply lands between polls, the parent's entry leaves the feed before we poll, the cachedkidslist stays stale, and since discovery reads our own cached kids, that reply could never be found.Item.reconcilecloses it by re-fetching the least-recently-updated rows each cycle. Cost is fixed per cycle rather than proportional to cache size, andRECONCILE_BATCHtrades convergence time against API load.This deliberately broke my "steady state costs zero requests" test. Replaced with the invariant that actually matters: cost tracks the reconcile budget, not the number of cached comments — 60 cached comments with a budget of 5 costs 5 requests.
Net effect per cycle
Tests: 103 runs, 261 assertions, green.