fix(displays): revert a pending change even if the app is killed #84

Merged

Breadway merged 2 commits from fix/bset-67-revert-after-crash into main 2026-10-14 19:05:33 +08:00

Conversation 3 Commits 2 Files changed 4 +617 -9

Breadway commented 2026-10-14 18:21:14 +08:00

Owner

Copy link

Closes BSET-67

What

The keep-or-revert deadline from #67 lives in a thread of the app. A SIGKILL, crash or OOM kill during the countdown left an unconfirmed layout live until the next login. (SIGTERM, SIGINT and SIGHUP are already handled.)

New display_rescue.rs:

• Before applying, the guard writes {id, pid, deadline_ms, before} to $XDG_RUNTIME_DIR/bos-settings/display-pending.json (atomic tmp then rename). It then starts a detached bos-settings --display-watchdog <id> in its own session (setsid in pre_exec), so killing the app or its process group doesn't kill the watchdog. If the record can't be written or the watchdog can't start, nothing is changed and the user gets a clear error.
• Every end of a change clears the record: Keep, Revert, timeout, unplug, exit/signal (all through finish_revert), a failed apply, and the thread-spawn failure path. clear_record only removes a record whose id matches, so an older change can never delete a newer change's record.
• The watchdog sleeps until the deadline. If the record is still there and the owner pid is gone (or is no longer a bos-settings exe, which covers pid reuse), it restores before through the same plan_statements and hyprctl eval path, then removes the record. If the owner is alive, it gets a 10s grace period to revert itself before the watchdog steps in.
• App start runs the same check for a record left by a dead run whose deadline has passed.
• The runtime dir is per user, mode 0700 and cleared at logout, so a stale record can't apply to a later session. Tests use DisplayGuard::new without a rescue, so they never spawn processes.

Tests

• display_rescue: decide() covering not due, owner dead, a different change, and owner alive with grace then stuck. Also a record round trip with own-id clearing, check_once restoring exactly once and clearing, and pid liveness.
• display_guard:
  • The record lives exactly while a change is pending: it is cleared after Keep, Revert, timeout and exit, and it holds the pre-change layout.
  • A failed apply leaves no record.
  • A simulated kill: apply, then drop the guard with mem::forget and expire the deadline. check_once puts the mock compositor back to the original layout.
• cargo fmt, clippy --all-targets -D warnings, cargo test (190), svelte-check 0/0, npm test.
• By hand: I ran the release binary as --display-watchdog 5 against an expired record, with a fake hyprctl on PATH and a temp XDG_RUNTIME_DIR. It issued exactly one hl.monitor(...) restoring the recorded layout and removed the record.

Not verified live

I didn't SIGKILL a real session's app mid-countdown, because this would reverse a display change on a real compositor. It is worth one check in the BOS VM, the way the SIGTERM fix was checked.

Capability changes

None user-facing. Each display change now also starts one short-lived background bos-settings --display-watchdog process, which exits when the change ends.

Closes BSET-67 ## What The keep-or-revert deadline from #67 lives in a thread of the app. A SIGKILL, crash or OOM kill during the countdown left an unconfirmed layout live until the next login. (SIGTERM, SIGINT and SIGHUP are already handled.) New `display_rescue.rs`: - **Before applying**, the guard writes `{id, pid, deadline_ms, before}` to `$XDG_RUNTIME_DIR/bos-settings/display-pending.json` (atomic tmp then rename). It then starts a detached `bos-settings --display-watchdog <id>` in its own session (`setsid` in `pre_exec`), so killing the app or its process group doesn't kill the watchdog. If the record can't be written or the watchdog can't start, **nothing is changed** and the user gets a clear error. - **Every end of a change clears the record**: Keep, Revert, timeout, unplug, exit/signal (all through `finish_revert`), a failed apply, and the thread-spawn failure path. `clear_record` only removes a record whose id matches, so an older change can never delete a newer change's record. - **The watchdog** sleeps until the deadline. If the record is still there and the owner pid is gone (or is no longer a bos-settings exe, which covers pid reuse), it restores `before` through the same `plan_statements` and `hyprctl eval` path, then removes the record. If the owner is alive, it gets a 10s grace period to revert itself before the watchdog steps in. - **App start** runs the same check for a record left by a dead run whose deadline has passed. - The runtime dir is per user, mode 0700 and cleared at logout, so a stale record can't apply to a later session. Tests use `DisplayGuard::new` without a rescue, so they never spawn processes. ## Tests - `display_rescue`: decide() covering not due, owner dead, a different change, and owner alive with grace then stuck. Also a record round trip with own-id clearing, `check_once` restoring exactly once and clearing, and pid liveness. - `display_guard`: - The record lives exactly while a change is pending: it is cleared after Keep, Revert, timeout and exit, and it holds the pre-change layout. - A failed apply leaves no record. - **A simulated kill**: apply, then drop the guard with `mem::forget` and expire the deadline. `check_once` puts the mock compositor back to the original layout. - cargo fmt, clippy --all-targets -D warnings, cargo test (190), svelte-check 0/0, npm test. - By hand: I ran the release binary as `--display-watchdog 5` against an expired record, with a fake `hyprctl` on PATH and a temp `XDG_RUNTIME_DIR`. It issued exactly one `hl.monitor(...)` restoring the recorded layout and removed the record. ## Not verified live I didn't SIGKILL a real session's app mid-countdown, because this would reverse a display change on a real compositor. It is worth one check in the BOS VM, the way the SIGTERM fix was checked. ## Capability changes None user-facing. Each display change now also starts one short-lived background `bos-settings --display-watchdog` process, which exits when the change ends.

Breadway added 1 commit 2026-10-14 18:21:14 +08:00

fix(displays): revert a pending change even if the app is killed ... 44d5438ecb

The keep-or-revert deadline lived only in a thread of the app, so SIGKILL or
a crash during the countdown left an unconfirmed layout live until the next
login (BSET-67).

Before applying, the guard now writes the previous layout, a wall-clock
deadline and its pid to $XDG_RUNTIME_DIR/bos-settings/display-pending.json
and starts a detached watchdog (bos-settings --display-watchdog <id>, own
session). Keep, Revert, timeout, exit and a failed apply all clear the
record; clear_record only removes a record for its own change id. If the
record is still there at the deadline and the owner pid is no longer a
bos-settings process (or is stuck 10s past it), the watchdog restores the
previous layout. App start does the same for a record left by a dead run.
If the record or watchdog cannot be set up, nothing is changed.

Tests: record round trip and own-id clearing, decide() timing and grace,
record lifetime across keep/revert/timeout/exit and failed apply, and a
simulated kill where check_once puts the mock compositor back. Also run
by hand: the release binary in watchdog mode with a fake hyprctl restored
the recorded layout and removed the record.

Breadway referenced this pull request 2026-10-14 18:21:15 +08:00

Displays: a pending change cannot be reverted if the app is hard-killed or crashes #68

Breadway commented 2026-10-14 18:55:27 +08:00

Author

Owner

Copy link

Review of BSET-67 at exact head 44d5438ecb (base main 7c7749d). Verdict: CHANGES REQUESTED (one real defect, plus non-blocking notes).

Verified on this head

• frontend: npm ci, npm run check (svelte-check 0 errors, 0 warnings), npm test, npm run build: all exit 0.
• src: cargo fmt --check, cargo clippy --locked --all-targets -D warnings, cargo test --locked (190 passed, 0 failed, including the 6 new rescue/guard tests): all exit 0.
• Forgejo CI check / check (push): success on this exact SHA (run 178).
• Independent debug-binary runs of --display-watchdog with a fake hyprctl first on PATH and a temp XDG_RUNTIME_DIR (no real session, no real hyprctl):
  • dead owner, deadline in 3s: waited, issued exactly one hl.monitor with the recorded layout, removed the record.
  • record id differs from the watchdog id: no call, record kept.
  • record owner pid belongs to a non-bos-settings process (pid reuse): treated as gone and restored.
  • no record: exits at once, no call.
  • record cleared while waiting (Keep): exits, no call. A newer record replacing it: exits, no call, newer record kept.
  • owner is a live bos-settings process, deadline passed: no restore inside the 10s grace, restored after it.
  • No watchdog processes left behind in any case.
• Ordering: with the spawn stubbed to fail, and separately with an unwritable record path, apply_for returns the "so nothing was changed" error, the mock compositor is untouched and no pending change exists.
• setsid runs in pre_exec and only calls setsid (async-signal-safe); stdio is nulled; the record lives in $XDG_RUNTIME_DIR/bos-settings and holds only id, pid, deadline and monitor geometry. No secrets.
• Watchdog lifetime is bounded: it exits on no record, a different id, or a restore, and the window is clamped to 5..60s.

Required change

1. Change ids are not unique across processes, so the "old watchdog cannot touch a newer change" guarantee only holds inside one process.
   State::default() starts next_id at 0 (display_guard.rs ~106-108, id taken at ~481-485), so every run's first change is id 1.
   Sequence: run A applies change id 1 and is SIGKILLed; its watchdog W1 (id 1) waits for the deadline. The user restarts the app inside the window. recover_at_start correctly does nothing (deadline not passed), then the user applies a new change, which is again id 1.
   arm_rescue (display_guard.rs ~426-447) overwrites the stale record without looking at it, so:
   a) the record's before becomes the still-unconfirmed layout from run A, and the original good layout is lost, so Revert, timeout or a second kill restores the unconfirmed layout;
   b) W1 now matches the new record by id and treats it as its own.
   I reproduced b) in a scratch test (not committed): with the new change at id 1 and an expired deadline, check_once(.., Some(1), ..) from the old watchdog acted on the new record and removed it.
   Fix: make ids unique across processes (for example start with now_ms() or combine pid and counter) AND make apply deal with an existing record from a different owner before overwriting it (restore it if due, or refuse with a clear message if its owner is alive). Add a test for "stale record from a dead run, new change in the same window".

Non-blocking notes

• owner_alive (display_rescue.rs 68-77): after a package update replaces the binary, a live owner's exe reads .../bos-settings (deleted), so a freshly started watchdog treats it as dead and restores inside the grace period (I reproduced this with a replaced binary). Practical risk is small because spawning from a deleted exe fails and apply then changes nothing, but stripping the " (deleted)" suffix before comparing would close it.
• spawn_watchdog drops the Child without waiting, so each finished watchdog stays a zombie until the app exits. Cheap to fix by moving the Child into a short reaper thread.
• DisplayGuard::live silently skips rescue when XDG_RUNTIME_DIR is unset, so there is no protection and no error. Consider refusing the change, in line with "failure to set it up changes nothing".
• disarm_rescue runs after a failed restore too (failed apply, RevertFailed), so the watchdog cannot act as a last-resort retry. This matches the stated criterion that a failed apply is never undone later, so I am not asking for a change.
• Not done: a live SIGKILL in the BOS VM, as the PR already says. It still needs doing before the card is closed.

CI: check green on 44d5438. Not merging because of item 1.

Review of BSET-67 at exact head 44d5438ecba0dee78de71bfde3623f2b041929dd (base main 7c7749d). Verdict: CHANGES REQUESTED (one real defect, plus non-blocking notes). ## Verified on this head - frontend: npm ci, npm run check (svelte-check 0 errors, 0 warnings), npm test, npm run build: all exit 0. - src: cargo fmt --check, cargo clippy --locked --all-targets -D warnings, cargo test --locked (190 passed, 0 failed, including the 6 new rescue/guard tests): all exit 0. - Forgejo CI `check / check (push)`: success on this exact SHA (run 178). - Independent debug-binary runs of `--display-watchdog` with a fake hyprctl first on PATH and a temp XDG_RUNTIME_DIR (no real session, no real hyprctl): - dead owner, deadline in 3s: waited, issued exactly one hl.monitor with the recorded layout, removed the record. - record id differs from the watchdog id: no call, record kept. - record owner pid belongs to a non-bos-settings process (pid reuse): treated as gone and restored. - no record: exits at once, no call. - record cleared while waiting (Keep): exits, no call. A newer record replacing it: exits, no call, newer record kept. - owner is a live bos-settings process, deadline passed: no restore inside the 10s grace, restored after it. - No watchdog processes left behind in any case. - Ordering: with the spawn stubbed to fail, and separately with an unwritable record path, apply_for returns the "so nothing was changed" error, the mock compositor is untouched and no pending change exists. - setsid runs in pre_exec and only calls setsid (async-signal-safe); stdio is nulled; the record lives in $XDG_RUNTIME_DIR/bos-settings and holds only id, pid, deadline and monitor geometry. No secrets. - Watchdog lifetime is bounded: it exits on no record, a different id, or a restore, and the window is clamped to 5..60s. ## Required change 1. Change ids are not unique across processes, so the "old watchdog cannot touch a newer change" guarantee only holds inside one process. `State::default()` starts `next_id` at 0 (display_guard.rs ~106-108, id taken at ~481-485), so every run's first change is id 1. Sequence: run A applies change id 1 and is SIGKILLed; its watchdog W1 (id 1) waits for the deadline. The user restarts the app inside the window. `recover_at_start` correctly does nothing (deadline not passed), then the user applies a new change, which is again id 1. `arm_rescue` (display_guard.rs ~426-447) overwrites the stale record without looking at it, so: a) the record's `before` becomes the still-unconfirmed layout from run A, and the original good layout is lost, so Revert, timeout or a second kill restores the unconfirmed layout; b) W1 now matches the new record by id and treats it as its own. I reproduced b) in a scratch test (not committed): with the new change at id 1 and an expired deadline, `check_once(.., Some(1), ..)` from the old watchdog acted on the new record and removed it. Fix: make ids unique across processes (for example start with `now_ms()` or combine pid and counter) AND make `apply` deal with an existing record from a different owner before overwriting it (restore it if due, or refuse with a clear message if its owner is alive). Add a test for "stale record from a dead run, new change in the same window". ## Non-blocking notes - `owner_alive` (display_rescue.rs 68-77): after a package update replaces the binary, a live owner's exe reads `.../bos-settings (deleted)`, so a freshly started watchdog treats it as dead and restores inside the grace period (I reproduced this with a replaced binary). Practical risk is small because spawning from a deleted exe fails and apply then changes nothing, but stripping the " (deleted)" suffix before comparing would close it. - `spawn_watchdog` drops the `Child` without waiting, so each finished watchdog stays a zombie until the app exits. Cheap to fix by moving the Child into a short reaper thread. - `DisplayGuard::live` silently skips rescue when XDG_RUNTIME_DIR is unset, so there is no protection and no error. Consider refusing the change, in line with "failure to set it up changes nothing". - `disarm_rescue` runs after a failed restore too (failed apply, `RevertFailed`), so the watchdog cannot act as a last-resort retry. This matches the stated criterion that a failed apply is never undone later, so I am not asking for a change. - Not done: a live SIGKILL in the BOS VM, as the PR already says. It still needs doing before the card is closed. CI: check green on 44d5438. Not merging because of item 1.

Breadway referenced this pull request 2026-10-14 18:55:33 +08:00

Displays: a pending change cannot be reverted if the app is hard-killed or crashes #68

Breadway referenced this pull request from a commit 2026-10-14 19:00:35 +08:00

fix(displays): unique change ids across runs and never overwrite another run's record

Breadway added 1 commit 2026-10-14 19:00:35 +08:00

fix(displays): unique change ids across runs and never overwrite another run's record ... 726ddb9eac

Review of #84 found that change ids restarted at 1 in every run. After a
kill and a restart inside the window, the new change got id 1 again: it
overwrote the dead run's record (losing the only copy of the good layout)
and the old watchdog treated the new record as its own.

- next_id is seeded from the clock, so ids never repeat across runs.
- Before arming, apply settles a record written by another process: a dead
  owner's layout is restored first (its watchdog then finds nothing and
  exits); a live owner makes apply refuse with a clear message.
- owner_alive treats an owner whose binary was replaced ('(deleted)') as
  the same program, so an update mid-countdown does not trigger a restore.
- The watchdog child is reaped by a thread, so no zombies are left.
- A missing XDG_RUNTIME_DIR is reported instead of silently skipped.

Tests: restart inside a dead run's window (fails on the previous head),
a live other window blocks a new change without touching the compositor,
and the '(deleted)' exe case.

Breadway commented 2026-10-14 19:00:52 +08:00

Author

Owner

Copy link

Addressed review 4918 in 726ddb9:

• Blocking: ids collide across runs. next_id is now seeded from the clock (ms x 1000), so a restarted app never reuses an id.
• Blocking: a foreign record was overwritten. apply calls settle_foreign_record() before reading the layout:
  • If the record's owner is gone, its before layout is restored first and the record cleared. Its watchdog then finds nothing and exits.
  • If the owner is still running, apply refuses ("Another BOS Settings window is waiting...") and touches nothing.
• Regression test: restart_inside_a_dead_runs_window_keeps_both_safe fails on 44d5438 and passes now. a_live_other_window_blocks_a_new_change is also new.
• Non-blocking notes:
  • An exe showing (deleted) now counts as the same program (same_program, with a test).
  • The watchdog child is reaped by a thread, so it no longer lingers as a zombie.
  • A missing XDG_RUNTIME_DIR is now logged.
• Checks: cargo fmt, clippy --all-targets -D warnings, cargo test 193, svelte-check 0/0, npm test, build. main is merged in.

The live SIGKILL check in the BOS VM is still outstanding.

Addressed review 4918 in 726ddb9: - **Blocking: ids collide across runs.** `next_id` is now seeded from the clock (ms x 1000), so a restarted app never reuses an id. - **Blocking: a foreign record was overwritten.** `apply` calls `settle_foreign_record()` before reading the layout: - If the record's owner is gone, its `before` layout is restored first and the record cleared. Its watchdog then finds nothing and exits. - If the owner is still running, `apply` refuses ("Another BOS Settings window is waiting...") and touches nothing. - **Regression test:** `restart_inside_a_dead_runs_window_keeps_both_safe` fails on 44d5438 and passes now. `a_live_other_window_blocks_a_new_change` is also new. - **Non-blocking notes:** - An exe showing `(deleted)` now counts as the same program (`same_program`, with a test). - The watchdog child is reaped by a thread, so it no longer lingers as a zombie. - A missing `XDG_RUNTIME_DIR` is now logged. - **Checks:** cargo fmt, clippy --all-targets -D warnings, cargo test 193, svelte-check 0/0, npm test, build. main is merged in. The live SIGKILL check in the BOS VM is still outstanding.

Breadway commented 2026-10-14 19:05:25 +08:00

Author

Owner

Copy link

Re-review at exact head 726ddb9eac (base main 7c7749d, unchanged). Verdict: PASS.

Blocking defect from comment 4918: fixed.

• next_id is seeded from the clock (ms x 1000), display_guard.rs ~325. apply calls settle_foreign_record before reading the layout (~503): a record from another pid whose owner is gone has its before restored and cleared; a live owner makes apply refuse without touching the compositor.
• I reverted both changes (no settle call, next_id back to 0) in my scratch worktree: restart_inside_a_dead_runs_window_keeps_both_safe FAILS ("ids are unique across runs", left 1 right 1) and a_live_other_window_blocks_a_new_change FAILS. With the fix both pass. Scratch edit discarded.
• Cross-process run with the real binary, fake hyprctl and temp XDG_RUNTIME_DIR: an old watchdog (id 1) sleeping while a newer-run record replaced the file made no hyprctl call and left the newer record intact.

Non-blocking items: fixed.

• same_program strips " (deleted)": a live owner whose binary was replaced (exe read "... (deleted)") got no restore inside the grace period (watchdog still waiting at the 5s timeout, 0 calls, record kept). Unit test replaced_binary_is_the_same_program passes.
• The watchdog child is reaped by a thread (display_rescue.rs spawn_watchdog).
• A missing XDG_RUNTIME_DIR is logged.

New-risk review: settle_foreign_record runs under the op lock, but apply already runs eval plus confirm under that lock, so restoring a stale layout there adds at most one 5s hyprctl call. It runs before any state change or id allocation, so a refusal leaves no pending change, no record write and no counter change. If the foreign restore fails, apply stops and leaves that record in place, so its watchdog can still retry. Minor, not blocking: a stale record whose pid equals the new process's own pid is skipped as "ours" (pid reuse by the same binary), which is very unlikely.

Checks on this exact head:

• frontend: npm ci, npm run check (0 errors, 0 warnings), npm test, npm run build: all exit 0.
• src: cargo fmt --check, cargo clippy --locked --all-targets -D warnings, cargo test --locked (193 passed, 0 failed), cargo build --locked: all exit 0.
• Forgejo check / check (push): success on 726ddb9.

Still outstanding after merge, for the coordinator: a live SIGKILL in the BOS VM.

Re-review at exact head 726ddb9eace66eb13ce9882196451bcd66fa1cc3 (base main 7c7749d, unchanged). Verdict: PASS. Blocking defect from comment 4918: fixed. - next_id is seeded from the clock (ms x 1000), display_guard.rs ~325. apply calls settle_foreign_record before reading the layout (~503): a record from another pid whose owner is gone has its `before` restored and cleared; a live owner makes apply refuse without touching the compositor. - I reverted both changes (no settle call, next_id back to 0) in my scratch worktree: restart_inside_a_dead_runs_window_keeps_both_safe FAILS ("ids are unique across runs", left 1 right 1) and a_live_other_window_blocks_a_new_change FAILS. With the fix both pass. Scratch edit discarded. - Cross-process run with the real binary, fake hyprctl and temp XDG_RUNTIME_DIR: an old watchdog (id 1) sleeping while a newer-run record replaced the file made no hyprctl call and left the newer record intact. Non-blocking items: fixed. - same_program strips " (deleted)": a live owner whose binary was replaced (exe read "... (deleted)") got no restore inside the grace period (watchdog still waiting at the 5s timeout, 0 calls, record kept). Unit test replaced_binary_is_the_same_program passes. - The watchdog child is reaped by a thread (display_rescue.rs spawn_watchdog). - A missing XDG_RUNTIME_DIR is logged. New-risk review: settle_foreign_record runs under the op lock, but apply already runs eval plus confirm under that lock, so restoring a stale layout there adds at most one 5s hyprctl call. It runs before any state change or id allocation, so a refusal leaves no pending change, no record write and no counter change. If the foreign restore fails, apply stops and leaves that record in place, so its watchdog can still retry. Minor, not blocking: a stale record whose pid equals the new process's own pid is skipped as "ours" (pid reuse by the same binary), which is very unlikely. Checks on this exact head: - frontend: npm ci, npm run check (0 errors, 0 warnings), npm test, npm run build: all exit 0. - src: cargo fmt --check, cargo clippy --locked --all-targets -D warnings, cargo test --locked (193 passed, 0 failed), cargo build --locked: all exit 0. - Forgejo `check / check (push)`: success on 726ddb9. Still outstanding after merge, for the coordinator: a live SIGKILL in the BOS VM.

Breadway merged commit c8decceb1f into main 2026-10-14 19:05:33 +08:00

Breadway referenced this pull request from a commit 2026-10-14 19:05:34 +08:00

Merge pull request 'fix(displays): revert a pending change even if the app is killed' (#84) from fix/bset-67-revert-after-crash into main

Breadway referenced this pull request 2026-10-14 19:05:40 +08:00

Displays: a pending change cannot be reverted if the app is hard-killed or crashes #68

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

breadbug

  

bug

  

displays

  

enhancement

  

keybinds

  

network

  

phase-1

  

power

  

privacy

  

recovery

  

search

  

snapshots

  

sound

  

ui

  

ux

No labels

breadbug

bug

displays

enhancement

keybinds

network

phase-1

power

privacy

recovery

search

snapshots

sound

ui

ux

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Breadway/bos-settings!84

No description provided.