fix(odds-ticker): publish the scroll cache array before the image - #471
Merged
Conversation
#469 moved the strip rebuild onto a worker thread, which is right, but left _create_ticker_image writing the two helper fields image-first: self.scroll_helper.cached_image = strip self.scroll_helper.cached_array = strip_array That is the reverse of the order the same PR established as correct in display()'s re-seed, with a comment explaining why: _get_visible_portion_integer reads cached_image.width and cached_array as two separate statements, so a reader between the two assignments sees the new (wider) image against the old (shorter) array. It was safe while _create_ticker_image ran with display() blocked on a queue. It is not now: the rebuild runs on a worker, and the re-seed keeps display() scrolling the previous strip while it runs, so the render thread reads in exactly that window. Reproduced against the real helper: image=6392 array=6312, scrolled near the end ValueError: could not broadcast input array from shape (64,12,3) into shape (64,92,3) image=6392 array=6392, same position ok, (128, 64) Two adjacent statements, which is why 30 minutes of soak on #469 saw none of it. The existing test pins ticker_image relative to cached_array and says nothing about the two cache fields relative to each other, so it passed throughout. test_cached_array_is_written_before_cached_image walks both _create_ticker_image and display() in source order and requires every cached_image write to be preceded by a cached_array write. It fails on the pre-fix code naming the exact line. Manifest 1.4.3 -> 1.4.4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
|---|---|
| Complexity | 10 |
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.
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
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.
Follow-up to #469, which merged while I was reviewing it. One finding, one line.
The problem
#469 moved the strip rebuild onto a worker thread — right call — but left
_create_ticker_imagewriting the two helper fields image-first:That is the reverse of the order the same PR established as correct in
display()'s re-seed at 2982-2983, with a comment explaining exactly why:_get_visible_portion_integerreads the two in separate statements:Why it is reachable now and was not before
It was safe while
_create_ticker_imageran withdisplay()blocked onq.get()— no concurrent reader. #469 removed that block. The rebuild now runs on a worker, and the new re-seed path deliberately keepsdisplay()scrolling the previous strip while it runs, so the render thread reads in exactly that window.Publishing
ticker_imagelast does not protect it: that gate is already passed when the old strip is on screen.Reproduced against the real
ScrollHelper:Two adjacent statements, which is why #469's 30-minute soak saw none of it and reported zero
not enough image data.The test that should have caught it
test_ticker_strip_is_published_only_once_it_is_finishedassertsmin(publishes) > max(caches)—ticker_imageaftercached_array. It says nothing about the two cache fields relative to each other, so it passed throughout.test_cached_array_is_written_before_cached_imagewalks both_create_ticker_imageanddisplay()in source order and requires everycached_imagewrite to be preceded by acached_arraywrite. Against the pre-fix code it fails naming the line:Verification
odds-tickertests 3 passed (7 assertions intest_display_defers_network), safety harness PASS at all 8 sizes, manifest gate OK. Manifest 1.4.3 → 1.4.4.Not addressed here
_pump_background("image-rebuild", ..., min_interval=2.0)logs at INFO on every start, and a persistent no-games state leavesticker_imageNone so it retries every 2s — roughly 30 INFO lines/minute for as long as it lasts. That is a large improvement on the per-frame warning it replaced (~6,000/min), so it is not a regression, but the data-update pump usesmin_interval=30.0and this one would read better atdebugor with a longer backoff. Left alone rather than widening a one-line fix.🤖 Generated with Claude Code
https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9