bread-theme::gtk per-output theme/palette hot-reload never fires (uses rename source path, not destination) #14

New issue

Closed

opened 2026-09-05 01:57:23 +08:00 by Breadway · 1 comment

Breadway commented

2026-09-05 01:57:23 +08:00

Owner

What's broken

bread-theme/src/gtk.rs's arm_dir_reload_watch (lines 312-325) is the directory watcher behind ensure_themes_watch() / ensure_palettes_watch(), which drive live per-monitor recolouring for every window bound via bind_window / bind_window_auto / bind_window_with_app_css / bind_window_auto_with_app_css. It never actually fires on a real write, because it inspects the wrong path for a rename event:

fn arm_dir_reload_watch(dir: std::path::PathBuf, want_ext: &'static str) -> Option<gio::FileMonitor> {
    ...
    monitor.connect_changed(move |_, file, other, _event| {
        let path = file.path().or_else(|| other.and_then(|f| f.path()));   // <-- bug: line 318
        let Some(stem) = path.as_deref().and_then(|p| reload_stem_for_event(p, want_ext)) else {
            return;
        };
        reload_binds_for_sanitized(&stem);
    });
    Some(monitor)
}

Every write this watch needs to react to goes through atomic_write (bread-theme/src/output.rs:68-83): write to .{name}.tmp.{pid}, then rename() to the final themes/<output>.css / palettes/<output>.json. With GFileMonitorFlags::WATCH_MOVES set (which this code does request), GIO/inotify pairs that rename into a single RENAMED event where file is the source (old/tmp) path and other is the destination (final) path — this is standard GIO semantics for WATCH_MOVES, and watch_theme_file two functions above (line 97-115) already relies on exactly this and correctly checks both file and other.

arm_dir_reload_watch instead takes file.path() unconditionally when it's Some (line 318) — which it always is, since the tmp path is a valid path — so other (the only path with the right extension) is never consulted. reload_stem_for_event then runs against the tmp filename (e.g. .eDP-1.css.tmp.12345), whose extension is 12345, not css/json, fails the extension check, and the event is silently dropped.

Evidence

Empirically verified with a standalone GIO test (PyGObject, same WATCH_MOVES flag) reproducing atomic_write's write-then-rename sequence on a real filesystem:

('created', '/tmp/.../.eDP-1.css.tmp.12345', None)
('changed', '/tmp/.../.eDP-1.css.tmp.12345', None)
('changes-done-hint', '/tmp/.../.eDP-1.css.tmp.12345', None)
('renamed', '/tmp/.../.eDP-1.css.tmp.12345', '/tmp/.../eDP-1.css')

The only event carrying the real destination filename (eDP-1.css) is the renamed event, and it's in the other field, not file. None of the four events give file a path with a .css/.json extension. This exact "tmp-suffixed name → no match" case is already unit-tested in this same file (reload_stem_for_event_requires_the_right_extension, asserting .eDP-1.css.tmp.42 returns None) — but no test covers which of file/other actually gets passed to that function from a live FileMonitor, so the bug slipped through ECO-6's otherwise-thorough per-monitor-theming audit (introduced in commit 9486a8c, "bread-theme/gtk: fix per-monitor bind lifecycle/race leaks + tests").

Why it matters

Every GTK app that binds windows via bind_window/bind_window_auto/*_with_app_css — confirmed in breadclip, breadcast, breadpad, breadman, breadpaper, breadsearch, breadhelp (and presumably breadbar/breadbox) — never picks up a per-output palette change live. Concrete repro: run bread-theme generate-output <output> <image> (or breadpaper set_on on a monitor) while e.g. breadclip's popup is open on that monitor — the window's theme/app CSS providers are never reloaded; the window keeps showing the old palette until the app is restarted or some other trigger (e.g. bind_auto's enter-monitor/realize/map re-bind, which re-reads the palette fresh each time) happens to force a full re-bind. So the practical symptom is intermittent/stale colours rather than a hard crash — it only "works" when something unrelated causes a rebind, which will read the freshly-written file since load_palette_for isn't itself broken; only the push notification is dead.

Note: this is isolated to the gio-based watcher in bread-theme::gtk. The notify-crate-based equivalents in breadlock (theme_watch.rs) and bos-settings (commands/theme.rs) are unaffected — notify's rename-"To" event carries the final path directly, and both already filter on it correctly.

Suggested fix

Prefer other over file when present (mirroring how watch_theme_file already checks both), or explicitly branch on _event == gio::FileMonitorEvent::Renamed and use other in that case, falling back to file for direct CREATE/CHANGED events (e.g. a non-atomic editor write). Something like:

let path = other.and_then(|f| f.path()).or_else(|| file.path());

Add a regression test that exercises the real connect_changed callback (not just reload_stem_for_event in isolation) against an actual rename, the way dir_reload_watches_arm_and_create_their_directories exercises real arming — e.g. write a tmp file, rename it into themes_dir()/palettes_dir(), pump the main context, and assert reload_binds_for_sanitized actually ran (a spy/counter on a bound test window, or at minimum assert the callback observed the right stem).

## What's broken `bread-theme/src/gtk.rs`'s `arm_dir_reload_watch` (lines 312-325) is the directory watcher behind `ensure_themes_watch()` / `ensure_palettes_watch()`, which drive live per-monitor recolouring for every window bound via `bind_window` / `bind_window_auto` / `bind_window_with_app_css` / `bind_window_auto_with_app_css`. It never actually fires on a real write, because it inspects the wrong path for a rename event: ```rust fn arm_dir_reload_watch(dir: std::path::PathBuf, want_ext: &'static str) -> Option<gio::FileMonitor> { ... monitor.connect_changed(move |_, file, other, _event| { let path = file.path().or_else(|| other.and_then(|f| f.path())); // <-- bug: line 318 let Some(stem) = path.as_deref().and_then(|p| reload_stem_for_event(p, want_ext)) else { return; }; reload_binds_for_sanitized(&stem); }); Some(monitor) } ``` Every write this watch needs to react to goes through `atomic_write` (`bread-theme/src/output.rs:68-83`): write to `.{name}.tmp.{pid}`, then `rename()` to the final `themes/<output>.css` / `palettes/<output>.json`. With `GFileMonitorFlags::WATCH_MOVES` set (which this code does request), GIO/inotify pairs that rename into a single `RENAMED` event where **`file` is the source (old/tmp) path and `other` is the destination (final) path** — this is standard GIO semantics for `WATCH_MOVES`, and `watch_theme_file` two functions above (line 97-115) already relies on exactly this and correctly checks both `file` and `other`. `arm_dir_reload_watch` instead takes `file.path()` unconditionally when it's `Some` (line 318) — which it always is, since the tmp path is a valid path — so `other` (the only path with the right extension) is never consulted. `reload_stem_for_event` then runs against the tmp filename (e.g. `.eDP-1.css.tmp.12345`), whose extension is `12345`, not `css`/`json`, fails the extension check, and the event is silently dropped. ## Evidence Empirically verified with a standalone GIO test (PyGObject, same `WATCH_MOVES` flag) reproducing `atomic_write`'s write-then-rename sequence on a real filesystem: ``` ('created', '/tmp/.../.eDP-1.css.tmp.12345', None) ('changed', '/tmp/.../.eDP-1.css.tmp.12345', None) ('changes-done-hint', '/tmp/.../.eDP-1.css.tmp.12345', None) ('renamed', '/tmp/.../.eDP-1.css.tmp.12345', '/tmp/.../eDP-1.css') ``` The only event carrying the real destination filename (`eDP-1.css`) is the `renamed` event, and it's in the `other` field, not `file`. None of the four events give `file` a path with a `.css`/`.json` extension. This exact "tmp-suffixed name → no match" case is already unit-tested in this same file (`reload_stem_for_event_requires_the_right_extension`, asserting `.eDP-1.css.tmp.42` returns `None`) — but no test covers which of `file`/`other` actually gets passed to that function from a live `FileMonitor`, so the bug slipped through `ECO-6`'s otherwise-thorough per-monitor-theming audit (introduced in commit `9486a8c`, "bread-theme/gtk: fix per-monitor bind lifecycle/race leaks + tests"). ## Why it matters Every GTK app that binds windows via `bind_window`/`bind_window_auto`/`*_with_app_css` — confirmed in breadclip, breadcast, breadpad, breadman, breadpaper, breadsearch, breadhelp (and presumably breadbar/breadbox) — never picks up a per-output palette change live. Concrete repro: run `bread-theme generate-output <output> <image>` (or `breadpaper set_on` on a monitor) while e.g. breadclip's popup is open on that monitor — the window's theme/app CSS providers are never reloaded; the window keeps showing the old palette until the app is restarted or some *other* trigger (e.g. `bind_auto`'s `enter-monitor`/`realize`/`map` re-bind, which re-reads the palette fresh each time) happens to force a full re-bind. So the practical symptom is intermittent/stale colours rather than a hard crash — it only "works" when something unrelated causes a rebind, which will read the freshly-written file since `load_palette_for` isn't itself broken; only the *push* notification is dead. Note: this is isolated to the `gio`-based watcher in `bread-theme::gtk`. The `notify`-crate-based equivalents in breadlock (`theme_watch.rs`) and bos-settings (`commands/theme.rs`) are unaffected — `notify`'s rename-"To" event carries the final path directly, and both already filter on it correctly. ## Suggested fix Prefer `other` over `file` when present (mirroring how `watch_theme_file` already checks both), or explicitly branch on `_event == gio::FileMonitorEvent::Renamed` and use `other` in that case, falling back to `file` for direct CREATE/CHANGED events (e.g. a non-atomic editor write). Something like: ```rust let path = other.and_then(|f| f.path()).or_else(|| file.path()); ``` Add a regression test that exercises the real `connect_changed` callback (not just `reload_stem_for_event` in isolation) against an actual rename, the way `dir_reload_watches_arm_and_create_their_directories` exercises real arming — e.g. write a tmp file, rename it into `themes_dir()`/`palettes_dir()`, pump the main context, and assert `reload_binds_for_sanitized` actually ran (a spy/counter on a bound test window, or at minimum assert the callback observed the right stem).

Breadway added the

bug

label

2026-09-05 01:57:26 +08:00

Breadway commented

2026-09-05 02:20:52 +08:00

Author

Owner

Confirmed the root cause exactly as described in the audit. bread-theme/src/gtk.rs:318 (arm_dir_reload_watch's connect_changed callback) read:

let path = file.path().or_else(|| other.and_then(|f| f.path()));

Since atomic_write (output.rs) writes every .css/.json update as write-to-.{name}.tmp.{pid}-then-rename(), and WATCH_MOVES is set, the resulting event has file = the tmp source and other = the real destination. file.path() is always Some for a valid tmp path, so other (the only field with the right extension) was never consulted, and reload_stem_for_event always failed its extension check against the tmp name — silently dropping every reload. watch_theme_file two functions above already handles this correctly by checking both fields.

Fix: extracted the file/other selection into a small pure function reload_event_path that prefers other (rename destination), falling back to file for a direct CREATE/CHANGED with no rename. arm_dir_reload_watch now uses it.

Added a unit test (reload_event_path_prefers_the_rename_destination_over_the_tmp_source). Also tried a full FileMonitor + real-rename integration test — it worked in isolation but collided with shell::hotreload's own glib-MainContext-pumping tests under the default parallel test runner (the same one CI uses), aborting the test binary via glib's thread guard. Dropped that in favor of the pure-function test, consistent with this file's existing precedent for isolating GTK/GIO-event-loop-dependent tests.

cargo build --release and cargo test --release --workspace --locked --features gtk pass (run 3x, no flakiness).

PR: #16

Confirmed the root cause exactly as described in the audit. `bread-theme/src/gtk.rs:318` (`arm_dir_reload_watch`'s `connect_changed` callback) read: let path = file.path().or_else(|| other.and_then(|f| f.path())); Since `atomic_write` (`output.rs`) writes every `.css`/`.json` update as write-to-`.{name}.tmp.{pid}`-then-`rename()`, and `WATCH_MOVES` is set, the resulting event has `file` = the tmp source and `other` = the real destination. `file.path()` is always `Some` for a valid tmp path, so `other` (the only field with the right extension) was never consulted, and `reload_stem_for_event` always failed its extension check against the tmp name — silently dropping every reload. `watch_theme_file` two functions above already handles this correctly by checking both fields. Fix: extracted the file/other selection into a small pure function `reload_event_path` that prefers `other` (rename destination), falling back to `file` for a direct CREATE/CHANGED with no rename. `arm_dir_reload_watch` now uses it. Added a unit test (`reload_event_path_prefers_the_rename_destination_over_the_tmp_source`). Also tried a full FileMonitor + real-rename integration test — it worked in isolation but collided with `shell::hotreload`'s own glib-MainContext-pumping tests under the default parallel test runner (the same one CI uses), aborting the test binary via glib's thread guard. Dropped that in favor of the pure-function test, consistent with this file's existing precedent for isolating GTK/GIO-event-loop-dependent tests. `cargo build --release` and `cargo test --release --workspace --locked --features gtk` pass (run 3x, no flakiness). PR: https://git.breadway.dev/Breadway/bread-ecosystem/pulls/16

Breadway closed this issue

2026-09-05 09:22:41 +08:00

Breadway referenced this issue

2026-09-11 21:09:53 +08:00

Shared CI cache path races when multiple repos release concurrently #28

Breadway referenced this issue

2026-09-30 00:12:46 +08:00

Dev track builds against dependency main; releases stay on locked tag pins #38