[v0.1/P1] Make image transfers canonical and stateful #51
Labels
No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
natsukium/felis#51
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
The image wire contract has two representations of the root frame and no enforced transfer state machine.
At review snapshot
5077d74b:docs/reference/ipc.mdsaysframe: Noneis root andSome(index)is an animation frame withindex >= 1.ImageMsgdocumentsSome(i)as 0-based, the protobuf conversion acceptsSome(0), andImageShadowappliesSome(0)toframes[0]. Root therefore has two valid-looking encodings with different header semantics.width,height, andformat, but the client ignores them for animation frames because frames inherit root metadata.Completemarks the buffer complete even when bytes are missing (zero fill remains).ImageShadow::applycannot return an error, despite the IPC contract saying malformed sequencing is connection-fatal and local to that connection.The reliable ordered carrier and the current daemon producer already emit a contiguous
Header -> Chunk* -> Completesequence, so the extra ambiguity buys no current behavior.Required decision
Redesign the protocol-2.0 image transfer into one canonical target and one enforceable state machine. Prefer shapes that make invalid combinations unrepresentable; at minimum, ingress must reject them before touching the mirror.
Questions to settle explicitly:
oneof/domain enum), with animation indices non-zero.total_bytesand/oroffsetif they are derivable from dimensions and ordered delivery; otherwise define their exact consistency rules.Acceptance criteria
Complete, and terminal events for unknown targets.ImageShadow::apply(or a validator in front of it) returns a typed error that the connector turns into connection-local teardown; malformed input is not silently normalized.docs/reference/ipc.md, and Kitty graphics docs state the same indexing and sequencing rules.Triage plan (2026-09-03)
Source-grounded triage against
mainat69076d42, reviewed through seven rounds of an independent reviewer (pisol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12.Claim check
Mostly accurate, with one factual correction that changes the redesign.
docs/reference/ipc.md:1389-1391saysframe: Some(index)addresses "animation frameindex ≥ 1" and "frame 0 is the root image, whichframe: Nonedelivers";crates/felis-protocol/src/messages/image.rs:67-69saysSome(i)is "the 0-based animation framei";felis.proto:993-995says "0-based".convert/image.rs:126-133acceptsSome(0);ImageShadow::apply_frame_header(crates/felis-client-core/src/image_shadow.rs:219-242) appliesSome(0)toframes[0]in place, andapply_header(:299-332) appliesNoneby replacing the whole entry (dimensions, format, frame list reset to one). So root has two encodings with different semantics: confirmed.a=f r=1,a=c c=1) goes throughimages.replace_frame(id, idx)withidx == 0and emitsImageEvent::TransmitFrame { index: 0 }(crates/felis-daemon/src/graphics.rs:1440-1452,:1636-1645), whichframe_sync_messages(:186-218) turns intoHeader { frame: Some(0) },Chunk { frame: Some(0) },Complete { frame: Some(0) }. The client testediting_the_displayed_frame_redirties(image_shadow.rs:723-735) pins exactly this. The two encodings therefore carry two behaviors today:None= fresh transmission (replace image, drop animation frames),Some(0)= edit the root's pixels in place (keep frames). "Reject frame 0" as written would breaka=f r=1/a=c c=1; the redesign must keep the distinction under a shape that has one encoding per meaning.width/height/formatand the client ignores them (apply_frame_headertakes onlytotal_bytes): confirmed. Every frame is a full coalesced canvas (docs/reference/protocols/kitty-graphics.md:191-198), sototal_bytes == width * height * bppfor root and frames alike (graphics.rs:156,:203derive it from the stored buffer).image.rs:71-72documents "any order";apply_chunk(:334-350) andapply_frame_chunk(:244-265) track no coverage and silently drop chunk-before-header, out-of-range, and (for frames) unknown index;apply_complete(:352-362) marks complete with whatever arrived ("A short transmission leaves trailing zeros; marked complete anyway").applyreturns()(:136), while the IPC contract says malformed sequencing is connection-fatal (docs/reference/ipc.md:446-455). Confirmed.image_sync_messages,graphics.rs:142-181: Header → Chunk* → Complete for the root, then each frame in order), the carrier is ordered, and the client applies inevent_handler.rs:563in arrival order. No interleaving exists.Verdict
accept-with-changes: the decision is requested. Recommend:
ImageHeader { id, oneof target { ImageNew new = 2; ImageFrame frame = 3; } }whereImageNew { width, height, format }starts a fresh image (replaces any prior entry and its frames) andImageFrame { number: uint32 }addresses framenumber ≥ 1of an existing image:1is the root in place (today'sSome(0)),N > 1edits or appends (append iffN == frames.len() + 1). This matches the reference's own frame model (kitty-graphics.md:193-194: "frames[0]is the root ... Kitty frame number 1") andShowFrameshould switch to the same 1-basednumberso one numbering appears on the wire (ShowFrame.indexis 0-based today,image.rs:136). Frame0becomes unrepresentable-as-valid (unset oneof / zero number is malformed), satisfying "animation frame 0 is rejected" without losing the root-edit behavior.ImageFramecarries onlynumber; dimensions and format are inherited and cannot be restated.total_bytesandoffset. Expected bytes arewidth * height * bpp(ImageFormat::bytes_per_pixel,image.rs:19-27) for every target; chunks are ordered and append. The receiver counts received bytes against expected. This also removes theu64 total_bytes → vec![0; n]allocation from a tiny frame that #49 objects to (image_shadow.rs:310-311,:226-227): the allocation is bounded by the shared dimension cap #49 introduces.Option<ActiveTransfer { id, target, expected, received }>per connection on the receiver;Chunk { id, bytes }andComplete { id }must name the active transfer's id (keepidas a cross-check; a mismatch is malformed). A secondHeaderwhile a transfer is live is malformed. Zero-length chunks: allow as a no-op (they cost nothing to accept; the producer never emits one,chunkedatgraphics.rs:127-134).Why this rather than keeping
optional framewith≥ 1: the oneof makes "new image" and "frame" different shapes with different required fields, so the invalid combinations (a frame with dimensions, a new image with a number) cannot be written; and it turns theImageShadowmatch frame { Some, None }split (:145-161) into amatch targetwith no zero case.Approach
Schema/domain (
felis-protocol):felis.proto:977-1018(newImageHeader,ImageChunk { id, bytes },ImageComplete { id },ImageShowFrame { id, number }),messages/image.rs:56-146(ImageMsg::Header { id, target: ImageTarget },enum ImageTarget { New { width, height, format }, Frame { number: NonZeroU32 } }),convert/image.rs(reject unset target, zero number,UNSPECIFIEDformat),image.rsdoc comments:48-54,:67-85. Round-trip cases atimage.rs:185-289updated; add rejection tests.Receiver (
felis-client-core/src/image_shadow.rs):apply(&mut self, msg) -> Result<(), ImageShadowError>with variantsChunkWithoutHeader,HeaderWhileActive,FrameForUnknownImage,FrameNumberSkipsAhead { number, have },ChunkOverrun { expected, got },IncompleteComplete { expected, received },TerminalForUnknownTarget,DimensionsOverCap(from #49's shared limit). Replace the threeapply_*pairs with oneActiveTransferstate;apply_completeassertsreceived == expected. Placements naming an unknown image stay tolerated (the comment at:131-135documents why). Callers:crates/felis-client/src/event_handler.rs:563mapsErrto the same close pathforward_frameuses for a driver error (main.rs:1784-1786, "daemon frame refused; closing the connection"); the bridge does not mirror images (confirm with grepImageShadowinfelis-cli: none).Producer (
felis-daemon/src/graphics.rs:142-218): emitNewforimage_sync_messages,Frame { number: idx + 1 }forframe_sync_messages;ShowFrame { number: current + 1 }at:176-179,:292;TransmitFrame's dedup key (:97) already usesindex + 1. The producer is already sequential; add a debug assertion inmaterialize_image_eventsthat noHeaderis queued while a prior transfer'sCompleteis pending (cheap, catches a future interleaving producer).Tests: shadow unit tests for root replacement (
Newtwice), frame append (number == len + 1), frame edit (number ≤ len), root edit (number == 1), skip-ahead (number == len + 2→ error), chunk-before-header, second header while active, overrun, shortComplete, terminal for an unknown id, id mismatch mid-transfer, zero-length chunk no-op, and a daemon-side test that a fullimage_sync_messagesfor a two-frame image applies cleanly on the shadow (round trip through the codec).serve/streaming.rs:967andsession_task.rs:2352match onImageMsg::Complete { .. }and keep compiling.Docs:
docs/reference/ipc.md:1367-1401(new shapes, ordered chunks, one transfer at a time, the malformed list, expected-byte rule),docs/reference/protocols/kitty-graphics.md:191-200,:253-259(1-based numbers on the wire,ShowFrame),docs/explanation/protocols/kitty-graphics.md"Frame storage (pre-coalesced)" if it mentions the wire triple,docs/explanation/architecture/ipc.mda short "Image transfers are one at a time" paragraph with the rejected alternatives (offset-addressed chunks: unneeded on an ordered carrier and unverifiable without coverage tracking; 0-based optional frame: two encodings of the root).felis.protocomments. CHANGELOG under the 2.0 wire entry.skills/felis: none (no CLI surface).Dependencies
#49 first (or together): the expected-byte computation needs the shared dimension/byte caps #49 places below daemon/client policy (
felis-protocol), and #49's "frame index growth" item is subsumed by the skip-ahead rule here. Land before #30. Independent of #47/#48 (Imageis uncorrelated and one-way). #52's order (item 3) holds.Risk/effort
M (2 days). Main risk: the 1-based
ShowFramerenumbering touches the daemon animation timer and the client'scurrentindex (image_shadow.rs:283-297); an off-by-one there shows as the wrong frame on reattach, so add the reattach round-trip test above. Secondary: rehydrate ships every frame of every image through the same sequential state, so a rehydrate that is interrupted by aDeletefor the active image must be defined (recommend:Deleteof the active transfer's image aborts the transfer cleanly, tested).Labels
Keep
priority/P1,release/v0.1.0.