You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
With kitty graphics, avatars sometimes render as a different user's avatar or as an emoji image, and stay wrong for the rest of the session. It happens when several images upload at about the same time, for example avatar preloads finishing while a channel's emoji or attachments are being flushed.
Cause
emitKittyUpload (internal/image/kitty.go:305) splits the base64 payload into 4096-byte chunks. Only the first chunk carries i=<id>. The terminal appends every later m= chunk to whichever transmission is in progress.
KittyOutput is serialized (internal/image/renderer.go:121, and in the app FrameOutput.SideChannel at internal/image/sixelframe.go:145), but the lock covers one Write call, not one upload (internal/image/sixelframe.go:177). Several writers upload concurrently: the avatar cache's worker pool writes straight to KittyOutput (internal/avatar/avatar.go:111, internal/avatar/avatar.go:226), while the messages and thread panes write their buffered flushes (internal/ui/messages/model.go:3293, internal/ui/thread/model.go:1665). When one of those writes lands between another image's first chunk and its continuations, the continuation bytes are stored under the wrong id, and every placeholder cell pointing at that id shows the wrong picture.
A debug log from a session that showed the symptom ruled out an id problem inside slk: 213 distinct image ids from the single Registry, no id bound to two keys, and every avatar upload was flushed. 39 of the 62 avatar uploads were within 200 ms of another upload.
Fix
emitKittyUpload builds every chunk, with the same per-chunk tmux passthrough wrapping as before, into one buffer and writes it once. The bytes on the wire are unchanged; only the number of Write calls changes, so an upload can no longer be split by another writer. writeKittySequence keeps its behavior and now shares the wrapping helper.
This applies to every kitty upload, not just avatars: a large attachment is now one write of the whole payload instead of one write per 4 KB chunk.
Tests
internal/image/kitty_upload_atomic_test.go:
TestEmitKittyUpload_MultiChunkPayloadIsOneWrite checks that a three-chunk upload reaches the writer as one Write with the same bytes as before.
TestEmitKittyUpload_ConcurrentUploadsDoNotInterleave runs 16 multi-chunk uploads concurrently through SerializeOutput and parses the stream: each upload's start chunk must be followed only by its own continuations until m=0.
Both fail on their assertions against the old emitKittyUpload (checked by restoring the old kitty.go with a test-only shim for the new helper): "upload took 3 writes; want 1" and "upload 14 started while upload 1 was still in progress".
Test plan
gofmt -l .
go build -ldflags="-s -w" -trimpath ./...
go vet ./...
go test -count=1 ./...
go test -race ./...
golangci-lint run ./... with v2.13.1
git diff --check main...HEAD
Revert check above
Manual: in the kitty session where avatars had shown other images, avatars rendered correctly after restarting on the fixed build
Verified the bug class and the fix. This is correct.
The kitty graphics protocol's continuation chunks (m=1) carry no image id — the terminal appends them to whichever transmission is in progress. And KittyOutput's lock serializes individual Write calls, not whole uploads. So when avatar preloads and an attachment flush upload at the same time, the chunks interleave and the terminal assembles the wrong image. The "wrong avatar for the rest of the session" symptom is exactly what that produces, and it stays wrong because the corrupted image lands in the payload cache under the intended id.
Buffering the whole upload into one Write is the right fix — it makes the upload atomic with respect to the existing per-Write lock, so no other writer can interleave. The forTerminal split to separate tmux-wrapping from writing is a clean way to do it without duplicating the tmux branch per chunk.
Build, vet, gofmt clean; 55 packages green under -race.
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
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.
Problem
With kitty graphics, avatars sometimes render as a different user's avatar or as an emoji image, and stay wrong for the rest of the session. It happens when several images upload at about the same time, for example avatar preloads finishing while a channel's emoji or attachments are being flushed.
Cause
emitKittyUpload(internal/image/kitty.go:305) splits the base64 payload into 4096-byte chunks. Only the first chunk carriesi=<id>. The terminal appends every laterm=chunk to whichever transmission is in progress.KittyOutputis serialized (internal/image/renderer.go:121, and in the appFrameOutput.SideChannelatinternal/image/sixelframe.go:145), but the lock covers oneWritecall, not one upload (internal/image/sixelframe.go:177). Several writers upload concurrently: the avatar cache's worker pool writes straight toKittyOutput(internal/avatar/avatar.go:111,internal/avatar/avatar.go:226), while the messages and thread panes write their buffered flushes (internal/ui/messages/model.go:3293,internal/ui/thread/model.go:1665). When one of those writes lands between another image's first chunk and its continuations, the continuation bytes are stored under the wrong id, and every placeholder cell pointing at that id shows the wrong picture.A debug log from a session that showed the symptom ruled out an id problem inside slk: 213 distinct image ids from the single
Registry, no id bound to two keys, and every avatar upload was flushed. 39 of the 62 avatar uploads were within 200 ms of another upload.Fix
emitKittyUploadbuilds every chunk, with the same per-chunk tmux passthrough wrapping as before, into one buffer and writes it once. The bytes on the wire are unchanged; only the number ofWritecalls changes, so an upload can no longer be split by another writer.writeKittySequencekeeps its behavior and now shares the wrapping helper.This applies to every kitty upload, not just avatars: a large attachment is now one write of the whole payload instead of one write per 4 KB chunk.
Tests
internal/image/kitty_upload_atomic_test.go:TestEmitKittyUpload_MultiChunkPayloadIsOneWritechecks that a three-chunk upload reaches the writer as oneWritewith the same bytes as before.TestEmitKittyUpload_ConcurrentUploadsDoNotInterleaveruns 16 multi-chunk uploads concurrently throughSerializeOutputand parses the stream: each upload's start chunk must be followed only by its own continuations untilm=0.Both fail on their assertions against the old
emitKittyUpload(checked by restoring the oldkitty.gowith a test-only shim for the new helper): "upload took 3 writes; want 1" and "upload 14 started while upload 1 was still in progress".Test plan
gofmt -l .go build -ldflags="-s -w" -trimpath ./...go vet ./...go test -count=1 ./...go test -race ./...golangci-lint run ./...with v2.13.1git diff --check main...HEADmain