diff --git a/bin/omarchy-migrate-notify b/bin/omarchy-migrate-notify index b478e876..43e452aa 100755 --- a/bin/omarchy-migrate-notify +++ b/bin/omarchy-migrate-notify @@ -4,6 +4,19 @@ set -euo pipefail +update_in_progress() { + local lock="${XDG_RUNTIME_DIR:-}/omarchy-update.lock" + + [[ -n ${XDG_RUNTIME_DIR:-} && -f $lock ]] || return 1 + + # flock -n only fails here when the lock is held, since the file is ours. + ! flock -n "$lock" true 2>/dev/null +} + +if update_in_progress; then + exit 0 +fi + pending_migrations=$(omarchy-migrate --pending 2>/dev/null) || exit 0 pending_count=$(printf '%s\n' "$pending_migrations" | sed '/^[[:space:]]*$/d' | wc -l) @@ -21,6 +34,12 @@ notify_command=$(printf 'if [[ -n $(omarchy-notification-send -u critical -g  # void and the user never learns about their pending migrations. omarchy-notification-wait || true +# That wait is long enough for an update to start underneath us, and the count +# above is already stale by then, so re-check before spending the toast. +if update_in_progress; then + exit 0 +fi + unit="omarchy-migrations-notification-$(date +%Y%m%d%H%M%S)" systemd-run --user --scope --unit="$unit" bash -lc "$notify_command" >/dev/null 2>&1 && exit 0 diff --git a/bin/omarchy-update b/bin/omarchy-update index 99a6026f..82064293 100755 --- a/bin/omarchy-update +++ b/bin/omarchy-update @@ -45,7 +45,7 @@ disable_sleep_for_update() { --who=omarchy-update \ --why="Omarchy update in progress" \ --mode=block \ - sleep infinity >/dev/null 2>&1 & + sleep infinity >/dev/null 2>&1 {OMARCHY_UPDATE_LOCK_FD}>&- & sleep_inhibit_pid=$! } diff --git a/docs/migrations.md b/docs/migrations.md index 9eb000fa..f7f00229 100644 --- a/docs/migrations.md +++ b/docs/migrations.md @@ -52,6 +52,9 @@ Every graphical login starts `omarchy-migrate-notify.service` after omarchy-migrate --pending ``` +It stays silent while `omarchy update` holds its lock, since that update applies +the pending migrations itself. + If that user has pending migrations, it shows a notification that opens a terminal for: diff --git a/docs/update-process.md b/docs/update-process.md index fcdda247..5081ffaa 100644 --- a/docs/update-process.md +++ b/docs/update-process.md @@ -163,6 +163,28 @@ transaction inside a normal `omarchy update`, so it fired notifications for migrations that `omarchy-migrate` was about to apply in the visible update terminal. The retired unit was `omarchy-update-user-notify.path`. +Retiring that watcher through a migration cannot come in time for the update +that retires it: pacman writes the migration directory, the watcher fires, and +only then does `omarchy-migrate` reach the migration that stops it. So the +notifier also refuses to run while `omarchy update` holds its +`$XDG_RUNTIME_DIR/omarchy-update.lock`, which covers the stale watcher and any +trigger added later — during an update, every pending migration is by +definition already being applied a step away. It checks again after waiting for +the notification server, since that wait is long enough for an update to start +underneath it. + +The notifier reads only its own user's runtime directory, never the `/tmp` path +`omarchy-update` falls back to when `XDG_RUNTIME_DIR` is unset. A shared lock +file belongs to whoever created it first, so honouring it would let one user +silence another user's notification. Missing an update and showing a redundant +toast is the better failure. + +Suppression is why `omarchy-update` starts its sleep inhibitor with the lock +descriptor closed. That inhibitor outlives the step that starts it, so an update +killed before `restore_update_inhibitors` would otherwise leave it holding the +flock indefinitely — blocking later updates and, now that the notifier reads the +same lock, silencing migration notifications at every login. + Fallbacks: - `omarchy-first-run` enables `omarchy-migrate-notify.service`, which also diff --git a/test/shell.d/migrate-notify-test.sh b/test/shell.d/migrate-notify-test.sh index a07888e9..80515b54 100644 --- a/test/shell.d/migrate-notify-test.sh +++ b/test/shell.d/migrate-notify-test.sh @@ -33,9 +33,29 @@ bash -c "$command" SH chmod +x "$stub_bin/systemd-run" +# Waiting for the notification server is the notifier's one long pause, so it is +# also where an update can start underneath it. Stand one up from inside the +# wait to prove the notifier re-checks afterwards instead of sending a toast it +# decided to send before the update existed. cat >"$stub_bin/omarchy-notification-wait" <<'SH' #!/bin/bash -exit 0 +[[ ${OMARCHY_TEST_LOCK_DURING_WAIT:-0} == 1 ]] || exit 0 + +lock="$XDG_RUNTIME_DIR/omarchy-update.lock" +: >"$lock" +# Hold the lock through a bash-allocated descriptor rather than `flock +# `: bash marks those close-on-exec, so the holder owns the lock alone +# and killing it releases immediately, with no exec'd child to outlive it. +bash -c 'exec {fd}>"$1"; flock -n $fd || exit 1; sleep 60' _ "$lock" & +echo "$!" >"$OMARCHY_TEST_LOCK_HOLDER_PID" + +for _ in {1..200}; do + flock -n "$lock" true 2>/dev/null || exit 0 + sleep 0.05 +done + +echo "stub could not establish the update lock" >&2 +exit 1 SH chmod +x "$stub_bin/omarchy-notification-wait" @@ -45,12 +65,18 @@ printf '%s\n' "$@" >"$OMARCHY_TEST_NOTIFY_ARGS" SH chmod +x "$stub_bin/omarchy-notification-send" +runtime_dir="$test_tmp/runtime" +mkdir -p "$runtime_dir" + run_notify() { HOME="$test_home" \ PATH="$stub_bin:$ROOT/bin:$PATH" \ + XDG_RUNTIME_DIR="$runtime_dir" \ OMARCHY_TEST_PENDING_MIGRATIONS="$1" \ OMARCHY_TEST_NOTIFY_ARGS="$test_tmp/notify-args" \ OMARCHY_TEST_SYSTEMD_RUN="${2:-run}" \ + OMARCHY_TEST_LOCK_DURING_WAIT="${OMARCHY_TEST_LOCK_DURING_WAIT:-0}" \ + OMARCHY_TEST_LOCK_HOLDER_PID="$test_tmp/lock-holder-pid" \ "$ROOT/bin/omarchy-migrate-notify" } @@ -69,3 +95,55 @@ grep -Fx 'Pending Omarchy Migrations' "$test_tmp/notify-args" >/dev/null || fail grep -Fx 'Click to run 1 pending migration.' "$test_tmp/notify-args" >/dev/null || fail "migration notifier describes the pending migration" grep -Fx '' "$test_tmp/notify-args" >/dev/null || fail "migration notifier includes the large-slot glyph" pass "migration notifier uses the actionable notification format" + +# `omarchy update` applies migrations itself, so nothing may notify about them +# while it holds its lock -- a stale trigger firing mid-transaction is exactly +# how the retired omarchy-update-user-notify.path used to interrupt updates. +rm -f "$test_tmp/notify-args" +update_lock="$runtime_dir/omarchy-update.lock" +: >"$update_lock" +exec {update_lock_fd}>"$update_lock" +flock -n "$update_lock_fd" || fail "test could not hold the update lock" + +run_notify 1 >"$test_tmp/during-update.out" 2>"$test_tmp/during-update.err" +[[ ! -s $test_tmp/during-update.out ]] || fail "migration notifier stays quiet on stdout during an update" +[[ ! -s $test_tmp/during-update.err ]] || fail "migration notifier stays quiet on stderr during an update" +[[ ! -e $test_tmp/notify-args ]] || fail "migration notifier sends no notification during an update" +pass "migration notifier stays quiet while omarchy update holds its lock" + +exec {update_lock_fd}>&- + +run_notify 1 >/dev/null 2>&1 +grep -Fx 'Pending Omarchy Migrations' "$test_tmp/notify-args" >/dev/null || + fail "migration notifier resumes notifying once the update lock is released" +pass "migration notifier resumes notifying after the update releases its lock" + +rm -f "$test_tmp/notify-args" +OMARCHY_TEST_LOCK_DURING_WAIT=1 run_notify 1 >"$test_tmp/raced.out" 2>"$test_tmp/raced.err" +if [[ -s $test_tmp/lock-holder-pid ]]; then + kill "$(<"$test_tmp/lock-holder-pid")" 2>/dev/null || true + for _ in {1..200}; do + flock -n "$runtime_dir/omarchy-update.lock" true 2>/dev/null && break + sleep 0.05 + done +fi +[[ ! -e $test_tmp/notify-args ]] || + fail "migration notifier sends no notification when an update starts while it waits for the notification server" +pass "migration notifier re-checks for an update after waiting for the notification server" + +# The guard must never read a lock outside this user's runtime directory: a +# shared /tmp path belongs to whoever created it first, so honouring it would +# let one user silence another user's critical notification. +rm -f "$test_tmp/notify-args" /tmp/omarchy-update.lock +foreign_lock="$test_tmp/foreign/omarchy-update.lock" +mkdir -p "$(dirname "$foreign_lock")" +: >"$foreign_lock" +exec {foreign_lock_fd}>"$foreign_lock" +flock -n "$foreign_lock_fd" || fail "test could not hold the foreign update lock" + +run_notify 1 >/dev/null 2>&1 +grep -Fx 'Pending Omarchy Migrations' "$test_tmp/notify-args" >/dev/null || + fail "migration notifier ignores update locks outside its own runtime directory" +pass "migration notifier ignores update locks outside its own runtime directory" + +exec {foreign_lock_fd}>&- diff --git a/test/shell.d/update-lock-test.sh b/test/shell.d/update-lock-test.sh index 37c03258..4d333ec5 100644 --- a/test/shell.d/update-lock-test.sh +++ b/test/shell.d/update-lock-test.sh @@ -101,6 +101,41 @@ grep -q "already running" "$test_tmp/perform-second.out" || fail "second omarchy [[ ! -f $test_tmp/perform-second-started ]] || fail "second omarchy-update-perform did not snapshot while lock was held" pass "omarchy-update-perform compatibility wrapper respects update lock" +# The sleep inhibitor deliberately outlives the step that starts it, so it must +# not inherit the update lock. An update killed before restore_update_inhibitors +# would otherwise leave the inhibitor holding the flock forever, blocking every +# later update and silencing omarchy-migrate-notify, which reads the same lock. +inhibit_pid_file="$test_tmp/inhibit-pid" +keyring_marker="$test_tmp/keyring-started" +write_stub omarchy-snapshot 'exit 0' +write_stub systemd-inhibit 'echo "$$" >"$INHIBIT_PID_FILE"; exec sleep 30' +write_stub omarchy-update-keyring 'echo started >"$TEST_MARKER"; sleep 3; exit 0' + +OMARCHY_UPDATE_LOGGED=1 TEST_MARKER="$keyring_marker" INHIBIT_PID_FILE="$inhibit_pid_file" \ + run_with_lock_env "$ROOT/bin/omarchy-update" -y >"$test_tmp/update-inhibit.out" 2>&1 & +inhibit_update_pid=$! + +for _ in {1..100}; do + [[ -s $inhibit_pid_file && -f $keyring_marker ]] && break + sleep 0.05 +done +[[ -s $inhibit_pid_file ]] || fail "update starts its sleep inhibitor" + +inhibitor_pid=$(<"$inhibit_pid_file") +kill -0 "$inhibitor_pid" 2>/dev/null || fail "sleep inhibitor is still running when its descriptors are inspected" + +lock_target=$(readlink -f "$runtime_dir/omarchy-update.lock") +inhibitor_holds_lock=0 +for fd in /proc/"$inhibitor_pid"/fd/*; do + [[ -e $fd ]] || continue + [[ $(readlink -f "$fd" 2>/dev/null) == "$lock_target" ]] && inhibitor_holds_lock=1 +done + +wait "$inhibit_update_pid" + +(( inhibitor_holds_lock == 0 )) || fail "update keeps the update lock out of the sleep inhibitor it leaves running" +pass "omarchy-update keeps the update lock out of its sleep inhibitor" + # Update-owned Stay Awake state must be cleared before the restart helper can # reboot the machine, rather than relying on an EXIT trap during shutdown. write_stub omarchy-snapshot 'exit 0'