graphics: deletes and re-transmits leave image refcounts and placements out of step #79

New issue

Closed

opened 2026-10-09 21:51:16 +09:00 by natsukium · 0 comments

natsukium commented

2026-10-09 21:51:16 +09:00

Owner

The daemon's image refcounts and placement table drift from each other in three ways, all in crates/felis-daemon/src/graphics.rs:

An uppercase delete frees an image that other placements still use. delete_by_id with p= (d=I,i=1,p=1) and delete_filtered (d=A/C/P/Q/X/Y/Z/R) call free_image for every touched image regardless of remaining placements. free_image removes the image and its virtual placements but not its regular placements, so the untouched placements stay in the daemon's table pointing at a deleted image. The client drops them on Delete, and a reattach re-sends them.
kitty: filter_refs frees an image only once it has no refs left.
Re-transmitting an existing i= keeps its placements. insert_into_store replaces the entry, and ImageStore::insert resets the refcount to 0 on the assumption that the dispatcher handles the old placements, but nothing does: they stay in both tables and now draw the new pixels, with a refcount of 0.
kitty: "the existing image and all its placements must be deleted" (free_refs_data on re-transmit).
Re-placing the same (i, p) leaks a refcount. upsert_placement calls images.retain on every a=p, including one that Placements::upsert turns into a replacement, so an image placed N times under one placement id holds N refs after a single live placement. No visible effect has been found yet: evict_oldest_to_fit ignores refcounts, so the only readers of a stale count are the animation tickers (advance_animations, next_animation_due_ms in crates/felis-grid/src/images.rs).

Repro (1): a=T,i=1,p=1 and a=p,i=1,p=2 at another position, then a=d,d=I,i=1,p=1. kitty keeps placement 2 on screen and answers OK to a later a=p,i=1,p=3; felis removes the image and answers that place with ENOENT.
Repro (2): place a=T,i=1,p=1, then re-transmit a=t,i=1 with different pixels and no placement. kitty clears the placement; felis redraws it with the new pixels.
Confirmed (3) by reading only: a=p,i=1,p=1 twice, then a=d,d=i,i=1,p=1 leaves the refcount at 1.

Fix: make the refcount follow the placement table (release on replace, drop the placements on re-transmit with PlacementRemoved events), and free on an uppercase delete only images left with no placements.

The daemon's image refcounts and placement table drift from each other in three ways, all in `crates/felis-daemon/src/graphics.rs`: 1. **An uppercase delete frees an image that other placements still use.** `delete_by_id` with `p=` (`d=I,i=1,p=1`) and `delete_filtered` (`d=A/C/P/Q/X/Y/Z/R`) call `free_image` for every touched image regardless of remaining placements. `free_image` removes the image and its virtual placements but not its regular placements, so the untouched placements stay in the daemon's table pointing at a deleted image. The client drops them on `Delete`, and a reattach re-sends them. **kitty:** `filter_refs` frees an image only once it has no refs left. 2. **Re-transmitting an existing `i=` keeps its placements.** `insert_into_store` replaces the entry, and `ImageStore::insert` resets the refcount to 0 on the assumption that the dispatcher handles the old placements, but nothing does: they stay in both tables and now draw the new pixels, with a refcount of 0. **kitty:** "the existing image and all its placements must be deleted" (`free_refs_data` on re-transmit). 3. **Re-placing the same `(i, p)` leaks a refcount.** `upsert_placement` calls `images.retain` on every `a=p`, including one that `Placements::upsert` turns into a replacement, so an image placed N times under one placement id holds N refs after a single live placement. No visible effect has been found yet: `evict_oldest_to_fit` ignores refcounts, so the only readers of a stale count are the animation tickers (`advance_animations`, `next_animation_due_ms` in `crates/felis-grid/src/images.rs`). **Repro (1):** `a=T,i=1,p=1` and `a=p,i=1,p=2` at another position, then `a=d,d=I,i=1,p=1`. kitty keeps placement 2 on screen and answers OK to a later `a=p,i=1,p=3`; felis removes the image and answers that place with `ENOENT`. **Repro (2):** place `a=T,i=1,p=1`, then re-transmit `a=t,i=1` with different pixels and no placement. kitty clears the placement; felis redraws it with the new pixels. **Confirmed (3)** by reading only: `a=p,i=1,p=1` twice, then `a=d,d=i,i=1,p=1` leaves the refcount at 1. **Fix:** make the refcount follow the placement table (release on replace, drop the placements on re-transmit with `PlacementRemoved` events), and free on an uppercase delete only images left with no placements.

~~natsukium referenced this issue 2026-10-09 23:23:27 +09:00~~

Follow kitty's placement identity and lifetime rules #87

natsukium referenced this issue from a pull request that will close it,

2026-10-09 23:56:07 +09:00

Follow kitty's placement identity and lifetime rules #87

natsukium closed this issue

2026-10-10 11:47:16 +09:00