Palette: reject bad indexes, suppress no-op events, make reads coherent - #23
Merged
tomlm merged 1 commit intoAug 24, 2026
Merged
Conversation
…rent Three problems, one found in review and two found probing around it. NO-OP EVENTS. ResetAllColors raised ColorChanged whether or not anything had changed, so a bare OSC 104 on an untouched palette told a renderer to repaint for nothing -- and unlike every other setter here, which all suppress. ApplyTheme had the same flaw. Both are silent now when the values already match. CLAMPED INDEXES. The bigger one. Clamping mapped any out-of-range index onto 0 or 255, so SetColor(999, ...) quietly rewrote entry 255 and the indexer answered for entry 0 when asked about -1. A caller with an off-by-one got no error and a corrupted palette. Clamping is the one response that produces a plausible WRONG answer where there should have been none; it throws now. The OSC path is unaffected, because InputHandler range-checks first -- which is exactly why this went unnoticed, and why the existing test did not catch it. COHERENT READS. ApplyTheme copied into the live array, and Array.Copy is not atomic, so a renderer scanning the palette could paint a frame half in the old theme and half in the new. Bulk changes now build a snapshot and swap one reference, which has no middle. That alone was not enough, and measuring said so: a reader calling the indexer eight times takes eight separate snapshots and can still straddle a swap. Under a tight toggle that produced three and a half MILLION mixed reads in three seconds -- routine, not a rare race. So Take() hands out one immutable view, which is what to use when reading more than one colour, and for a renderer painting a frame that is always. Writes copy on write to keep it immutable: an int store is atomic and would have been safe for the value, but it would move a snapshot somebody is holding, and not moving is the entire point. A palette is 1KB and colour changes are rare. Reads stay lock free. The lock only serialises writers, because the indexer is on a per-cell path where locking would be a worse cure than the problem. 656 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tomlm
approved these changes
Aug 24, 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
2 participants
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 #21, which merged before these landed on the branch. Three problems in the colour palette that is now on
main— one raised by Copilot on that PR, two found while looking into it.Out-of-range indexes were clamped
The one I would fix first.
Clampmapped any index outside 0..255 onto an end of the range, so:A caller with an off-by-one got no error and a corrupted palette. Clamping is the single response that produces a plausible wrong answer where there should have been none — it throws
ArgumentOutOfRangeExceptionnow.The OSC path is unaffected:
InputHandlerrange-checks before it reaches the palette. That is exactly why it went unnoticed, and why the existing test did not catch it — the test drove OSC 4, which is the guarded path, not the public API. There is now a test for each.No-op resets raised
ColorChangedResetAllColorsfired whether or not anything had changed, so a bareOSC 104on an untouched palette told a renderer to repaint for nothing — unlikeResetColorand the fg/bg/cursor resets, which all suppress.ApplyThemehad the same flaw. Both are silent now when the values already match.A theme change could be observed half applied
ApplyThemecopied into the live array, andArray.Copyis not atomic, so a renderer scanning the palette could paint a frame half in the old theme and half in the new. Bulk changes now build a snapshot and swap one reference, which has no middle.That alone was not enough, and measuring said so. A reader calling the indexer eight times takes eight separate snapshots and can still straddle the swap. Under a tight toggle that produced 3.5 million mixed reads in three seconds — routine, not a rare race. The atomicity existed but was unreachable through the public API.
So
ColorPalette.Take()returns one immutableColorSnapshot: what to use when reading more than one colour, which for a renderer painting a frame is always. Writes copy on write to keep it immutable — anintstore is atomic and would have been safe for the value, but it would move a snapshot somebody is holding, and not moving is the entire point. A palette is 1KB and colour changes are rare.Reads stay lock free. The lock only serialises writers, because the indexer sits on a per-cell path where locking would be a worse cure than the problem.
Testing
Nine new tests: no-op suppression for both reset paths, out-of-range rejection through the API and continued tolerance through OSC, a snapshot that does not move after later writes, and a reader/writer pair asserting a theme flip is never observed half applied. 665 passed, 0 failed.
Take()andColorSnapshotare a small public addition, so worth a look.🤖 Generated with Claude Code