fix(flights): a deferred tile is not a failed one - #292
Merged
Conversation
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: Pro Plus 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 | 13 |
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.
ChuckBuilds
force-pushed
the
fix/flights-tile-deferral
branch
from
August 18, 2026 19:42
b6e24a0 to
2bc8301
Compare
#284 moved tile fetching off the render thread, so the render path now asks for tiles that are not in the cache yet and gets None back. The compositing loop counted those the same as a tile the server refused to serve, and the failure branch disables the map background for the rest of the session above 50%. On a cold cache every tile is a deferral. Reproduced against the merged code with a 110-tile grid: frame one returns nothing (protected by the tiles_fetched == 0 early return), frame two sees 10 of 110 tiles and switches the map off permanently, and frame three with all 110 cached still returns None. Below the 50% bar it goes wrong the other way -- the partial composite is cached under a key of centre, zoom and size, all of which come from static config, so the invalidation never fires: a map measured 46.8% black came back as the identical object after every tile was available, re-reading no tiles. Either way the map is gone for the session and only a config reload clears it. Deferrals are now tracked separately and only genuine fetch failures count toward the disable threshold, so the safety valve still fires for a server that is actually refusing. A composite with any tile missing is not cached, which costs a recomposition per frame from already-cached tiles until the prefetch catches up, and nothing after that. Two supporting fixes. _MAX_TILES_WANTED was 64 against a grid that reaches 16x16 and already needs 110 by default, so the render path could never queue a whole map in one pass -- that is what kept frames partial long enough to trip the two bugs above; it is now 256. And the prefetch drained its whole queue with a 3s timeout per tile against PluginExecutor's 30s budget for the entire update, so a slow-but-succeeding server could overrun it and be recorded as a failed update; it now stops at 10s and puts the remainder back on the queue. The existing cooldown only rescued the case where the server fails outright, not where it is merely slow. The new test drives the real _get_map_background through a startup sequence. The existing test checks by AST that display() passes allow_network=False, which is true and was never the problem -- it never executes the function where the bug lives. It follows the LEDMATRIX_CORE skip convention the sibling tests use, so it exercises the real class rather than compiling methods out of the file. Mutation-checked: 6 of its 8 checks fail against main, and the two that pass are the behaviours deliberately preserved (the failure valve, and that a complete map is still cached). Full flights suite passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
ChuckBuilds
force-pushed
the
fix/flights-tile-deferral
branch
from
August 18, 2026 19:45
2bc8301 to
1c970ac
Compare
ChuckBuilds
pushed a commit
that referenced
this pull request
Aug 19, 2026
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 #284, which is now merged. Rebuilt on top of
mainso this is only the fix.The bug
#284 moved tile fetching off the render thread — the right idea — so the render path now asks for tiles that aren't cached yet and gets
Noneback. But the compositing loop counts those the same as a tile the server refused to serve, and the failure branch disables the map background for the rest of the session above 50%.On a cold cache every tile is a deferral. Reproduced against the merged code, 110-tile grid:
Below the 50% bar it fails the other way — the partial composite is cached under a key of
(centre, zoom, size), all static config, so the invalidation can never fire:Either way the map is gone for the session and only a config reload clears it. Before #284 this was unreachable: the render thread fetched inline, so a healthy server meant a 0% failure rate. The queue is only populated by a render, so every cold start goes through this — not an edge case.
The fix
allow_network=True) counts toward the disable threshold, so the safety valve still fires for a server that really is refusing._MAX_TILES_WANTED64 → 256. The grid reaches 16×16 (max_tiles = 16is applied per axis, despite the# 4x4 maximumcomment beside it) and a default config already needs 110, so the render path could never queue a whole map in one pass — which kept frames partial long enough to trip the two bugs above._TILE_TIMEOUT_SECONDS = 3each againstPluginExecutor's 30s budget for the entire update, so a slow-but-succeeding server could overrun it and be recorded as a failed update. The existing cooldown only rescues a server that fails outright, not one that's merely slow.Testing
New
test_map_cold_start.pydrives the real_get_map_backgroundthrough a startup sequence. The existing test asserts by AST thatdisplay()passesallow_network=False— true, and never the problem; it never executes the function where the bug lives.It follows the
LEDMATRIX_COREskip convention the sibling tests use, so it imports and exercises the realFlightTrackerPlugin(including the read-onlydisplay_width/display_heightproperties) rather than compiling methods out of the file.Mutation-checked: 6 of its 8 checks fail against
main. The 2 that pass are the behaviours deliberately preserved — the failure valve, and that a complete map is still cached.Full flights suite passes:
test_config_reload,test_map_cold_start,test_no_disk_write_on_poll,test_overhead_radius,test_tile_network_bound,test_tiles_off_render_thread,test_vegas_map_parity.Manifest 1.12.10 → 1.12.11,
plugins.jsonregenerated by the pre-commit hook.Scope note
A total fetch failure returns earlier, at
tiles_fetched == 0, without disabling anything. That predates #284 and is covered by the fetch cooldown, so I left it rather than widen the change — my first draft of the test assumed otherwise and caught me.🤖 Generated with Claude Code
https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW