fix(background): release the payload after every callback, not after each one - #509
Conversation
…each one Callers that join an in-flight fetch share one FetchResult. #499 released the payload inside the delivery loop, so the first callback got the data and every joiner got `result.data is None`. That is not a quiet degradation. Consumers read `result.data.get('events')`, so they raise AttributeError -- which the delivery loop catches and logs. The entire failure surfaced as one line: ERROR - src.background_data_service - Error in callback for request nhl_2026_...: 'NoneType' object has no attribute 'get' and a manager that silently never received its schedule. Seen on hardware: NHLRecentManager logs "Background fetch completed for 2026: 1000 events" and the very next line is the error, from NHLUpcomingManager's callback on the same request -- which had already logged "No events found in shared data." Deduplication is the normal case, not a corner. A sport's recent, upcoming and live managers all want the same season schedule, so the second and third are joiners on almost every cycle. _release_payload's own docstring said "once A callback has been handed the data", singular, which is the assumption that broke: the loop above it was written for many, and says so. Moved after the loop, and guarded on `callbacks` being non-empty. The guard matters: a request submitted without a callback must keep its payload, because polling get_result() is then the only way to collect it. The per-delivery release got that right by accident -- an empty list never entered the loop body -- and the existing test for it caught the omission. test_background_payload_release.py gains TestJoinersAllGetTheData: two submitters on one in-flight cache_key, asserting both are handed a populated payload, plus that the memory fix still happens once they have all had it. test_background_fetch_dedupe.py already proved the joiner's callback FIRES; it never checked what the callback received, which is the gap that let this through. Verified the new test bites: restoring the release inside the loop fails it with "'second' was handed a released payload". Full suite 3716 passed, 6 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe fetch worker now releases shared payloads after all callbacks receive the data. Callback-less requests retain payloads for polling. New tests cover joined requests and final payload release. ChangesShared fetch payload lifecycle
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This localized change ensures all joined callbacks receive the shared payload before it is released while preserving polling behavior for callback-less requests. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Title checkExplanation The title addresses payload release timing, but it states “after every callback,” which conflicts with the implemented change. The payload is released after all callbacks complete, not after each callback.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
1 participant
Callers that join an in-flight fetch share a single
FetchResult. #499 released the payload inside the delivery loop, so the first callback got the data and every joiner gotresult.data is None.That is not a quiet degradation. Consumers read
result.data.get('events'), so they raiseAttributeError— which this very loop catches and logs. The whole failure surfaces as one line and a manager that silently never receives its schedule.Seen on hardware
While soaking an unrelated change on a live rig:
NHLRecentManager's callback ran first and logged its 1000 events. The error is a second callback on the same request — andNHLUpcomingManagerhad already loggedNo events found in shared data.a moment earlier.Deduplication is the normal case, not a corner. A sport's recent, upcoming and live managers all want the same season schedule, so the second and third are joiners on almost every cycle.
_release_payload's own docstring said "Only called once a callback has been handed the data" — singular. That is the assumption that broke; the loop above it was written for many and says so explicitly ("Call every callback: the original submitter's and any that joined this fetch").The fix
Move the release after the loop, and guard it on
callbacksbeing non-empty.The guard is load-bearing. A request submitted without a callback must keep its payload, because polling
get_result()is then the only way to collect it. The per-delivery release got that right by accident — an empty list never entered the loop body — andtest_without_a_callback_the_payload_is_keptcaught me omitting it on the first attempt.Tests
TestJoinersAllGetTheData— two submitters on one in-flightcache_key:test_every_joiner_is_handed_the_payloadasserts both callbacks receive a populated payload, reading it the way the sport managers do (result.data.get('events'), since that is the access that actually raises). It asserts the coalescing happened first, or two independent fetches would each own their result and the test would prove nothing.test_the_payload_is_still_released_once_they_have_all_had_it— the memory fix from fix(memory): release fetched payloads once they have been delivered #499 must survive this ordering fix.test_background_fetch_dedupe.pyalready hadtest_the_joiner_still_gets_its_callback, which proves the callback fires. It never checked what the callback received. That is the gap that let this through, and the new test closes it.Verified the new test bites: restoring the release inside the loop fails it with
'second' was handed a released payload: the result was emptied before every callback had been delivered.Full suite
3716 passed, 6 skipped.🤖 Generated with Claude Code
https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
Summary by CodeRabbit