Repository navigation
Hibernate laptops on critical battery once hibernation is set up - #13540
stevederico wants to merge 10 commits into
Conversation
Automated AI review
Outcome: Laptops that already have hibernation keep the old critical battery action after updating. Existing hibernating laptops keep the 2% defaultOnly setup installs the drop-in, and nothing in Suggested change: add a migration that installs it when Reproducer (source search at 28c4d46)Reproducer: laptops that already have hibernation keep the stock critical action after updatingReviewed head: 28c4d46. This is a source-level reproduction: it shows that nothing in the update path installs the new drop-in, so an existing install only gets it if the user reruns The installer sets hibernation up as a fixed install phase: From a checkout of the PR head: rg -n "omarchy-hibernation-setup" bin install migrations
rg -n "critical-hibernate|UPower.conf.d" bin install migrationsActual output: The only caller of DetailsVerified (UPower 1.91.4 and systemd 262 source, sandbox runs):
Optional notes:
Reproducer for the first-setup test (fails at 28c4d46 with line 171 removed)Reproducer: the first-setup path is not covered by the PR's testReviewed head: 28c4d46 (base 97a9fce). New installs reach Step 1: remove the first-setup call and run the PR's testFrom a checkout of the PR head, on a machine where sed -i '/^setup_critical_battery_hibernate$/d' bin/omarchy-hibernation-setup
bash test/shell.d/hibernation-critical-battery-test.shThe edit removes exactly one line: @@ -168,7 +168,6 @@
fi
fi
-setup_critical_battery_hibernate
if ! $NO_REBUILD; thenActual output, exit status 0 (run in a sandbox with a private Step 2: a test case for the first-setup pathSave as #!/bin/bash
#
# The first hibernation setup on a laptop (the installer runs it with --force
# --no-rebuild) also installs the critical battery action. /etc, /swap and the
# system-sleep hook directory are redirected into a scratch root; sudo runs the
# file operations there and records service restarts.
set -euo pipefail
source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh"
[[ -f /sys/power/image_size ]] || { skip "no hibernation support to exercise"; exit 0; }
scratch=$(mktemp -d)
trap 'rm -rf "$scratch"' EXIT
mkdir -p "$scratch/bin" "$scratch/etc/limine-entry-tool.d" "$scratch/usr/lib/systemd/system-sleep" "$scratch/swap"
export CALL_LOG="$scratch/calls"
export OMARCHY_PATH="$ROOT"
export PATH="$scratch/bin:$ROOT/bin:$PATH"
cat > "$scratch/bin/sudo" <<'STUB'
#!/bin/bash
case "$1" in
systemctl) echo "systemctl ${*:2}" >> "$CALL_LOG" ;;
/usr/bin/install)
args=()
while (($# > 1)); do
shift
case "$1" in -o | -g) shift ;; *) args+=("$1") ;; esac
done
exec /usr/bin/install "${args[@]}"
;;
*) exec "$@" ;;
esac
STUB
printf '#!/bin/bash\nexit 0\n' > "$scratch/bin/omarchy-battery-present"
printf '#!/bin/bash\necho "btrfs $*" >> "$CALL_LOG"\n' > "$scratch/bin/btrfs"
printf '#!/bin/bash\nexit 0\n' > "$scratch/bin/swaplabel"
printf '#!/bin/bash\necho "%s"\n' "$scratch/swap/swapfile" > "$scratch/bin/swapon"
chmod +x "$scratch/bin/"*
sed -e "s|/etc/|$scratch/etc/|g" -e "s|\"/swap|\"$scratch/swap|g" \
-e "s|/usr/lib/systemd/system-sleep/|$scratch/usr/lib/systemd/system-sleep/|g" \
"$ROOT/bin/omarchy-hibernation-setup" > "$scratch/omarchy-hibernation-setup"
# Swap subvolume, swapfile, fstab entry and resume parameters already exist, so
# the run only writes the resume hook, the sleep hook and the battery action.
printf '\n# Btrfs swapfile for system hibernation\n%s none swap defaults,pri=0 0 0\n' "$scratch/swap/swapfile" > "$scratch/etc/fstab"
echo 'KERNEL_CMDLINE[default]+=" resume=/dev/null resume_offset=1"' > "$scratch/etc/limine-entry-tool.d/resume.conf"
: > "$CALL_LOG"
bash "$scratch/omarchy-hibernation-setup" --force --no-rebuild > "$scratch/out" 2>&1 ||
fail "first setup on a laptop succeeds" "$(<"$scratch/out")"
grep -qx 'HOOKS+=(resume)' "$scratch/etc/mkinitcpio.conf.d/omarchy_resume.conf" ||
fail "first setup writes the resume hook" "$(<"$scratch/out")"
cmp -s "$ROOT/default/upower/omarchy-critical-hibernate.conf" \
"$scratch/etc/UPower/UPower.conf.d/70-omarchy-critical-hibernate.conf" ||
fail "first setup on a laptop installs the critical battery action" "$(<"$scratch/out")"
grep -qx 'systemctl try-restart upower.service' "$CALL_LOG" || fail "UPower is restarted to read the new action"
pass "first setup on a laptop installs the critical battery action"Actual results (same sandbox): At the PR head, unmodified (exit 0): At the PR head with the Step 1 edit (exit 1): At the base commit 97a9fce, with the PR's
Review informationTest scope: Source review and sandbox tests at AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
|
Reproduced the critical-action issue on an LG Gram running Omarchy 4.0.4-1. Hibernation is available ( This is another case where changing the percentage alone is insufficient. The explicit |
|
+1. Same setup here on an ASUS ROG Zephyrus G16 (GU605CP), Omarchy 4.0.4-1, upower 1.91.3: hibernation set up by Omarchy, logind One suggestion: make the level easy to change. 5 % is a sensible default, but people will weigh the margin differently (e.g. 3 % on a new battery). UPower already allows this without a new Omarchy setting: a later drop-in such as [UPower]
PercentageAction=3.0overrides the |
 [Medium impact] The PR appears safe to merge; the latest test changes address all three numbered findings without introducing a new actionable issue. SummaryMakes configured laptops hibernate directly at 5% battery through a UPower drop-in.
Reviews (6) · Last reviewed commit: "Fold the critical battery checks into th..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
I read this pull request at 6ad6e4c, together with #13256, to decide whether it is a bug fix or a new feature before reviewing it further. The two models that classify items separately did not agree, so it has no class label for now and was not run on a worker.
Both readings hold up, so which it is comes down to whether the maintainer counts the stock critical-battery action as broken on a machine Omarchy set up for hibernation. Nothing here waits on you; this waits on the maintainer to decide that. |
6ad6e4c to
d28ef2f
Compare
d28ef2f to
d269fea
Compare
|
I read this pull request again at d269fea, since the head moved after my last note, together with #13256, to decide whether it is a bug fix or a new feature. The two models classifying it separately still do not agree, so it carries no class label and was not run on a worker.
The two readings swapped since the last pass, which says the question really is a judgement call: whether the stock critical-battery action counts as broken on a machine Omarchy set up to hibernate. That is the maintainer's to decide, and this waits on them. Nothing here waits on you. |
UPower's Auto critical action is logind's Sleep(), which is suspend-then-hibernate, and it fires at 2%. A laptop can drain in the suspend before the hibernate runs and power off hard, even with hibernation set up. When hibernation setup runs on a machine with a battery, install a UPower drop-in that hibernates directly at 5% (critical at 8%). Rerunning setup adds it to machines set up before, and removing hibernation takes it out. Fixes omacom#13256
Only omarchy-hibernation-setup installs the UPower drop-in, so a laptop that set up hibernation before this change kept UPower's Auto action and still suspends into a dying battery. Rerun setup from a migration on laptops that already have the resume hook. On those, setup only adds what is missing, and it leaves an installed drop-in alone.
The drop-in's comment and the sleep manual now show how to set a different PercentageAction in a later UPower drop-in, which setup leaves alone, and that it has to stay below PercentageCritical.
Setup now installs the UPower drop-in only when the resume parameters carry a swapfile offset, and on a fresh setup only after the UKI rebuild succeeds, so a dying battery never powers off a session the kernel cannot restore. On a laptop that already has hibernation, a failed install now fails setup, so the migration that runs it is retried instead of being marked done. The test no longer skips on hosts without /sys/power/image_size: the scratch copy of setup reads a scratch file instead. New cases cover a missing or empty resume offset and a failed install.
d269fea to
39a39b2
Compare
The setup test now redirects /etc/UPower and stubs battery detection, so on a laptop host it no longer reaches the real UPower config. The separate critical battery test and its grep of the config value are gone.
Fixes #13256
UPower's
Autocritical action resolves to logind'sSleep(), which is suspend-then-hibernate, and fires at 2%. A laptop can drain in the suspend before the hibernate runs and power off hard, even with hibernation set up.When
omarchy-hibernation-setupruns on a machine with a battery, it now installs a UPower drop-in (/etc/UPower/UPower.conf.d/70-omarchy-critical-hibernate.conf) that hibernates directly at 5%, with critical at 8%. Rerunning setup adds it to machines set up before, andomarchy-hibernation-removetakes it out. Machines without a battery get nothing. If hibernate isn't available, UPower falls back to a clean power off.Test plan
bash test/shell.d/hibernation-setup-test.sh: new cases on its scratch root (/etc/UPowerredirected, battery detection andsystemctlstubbed) check the drop-in is installed on a laptop, left alone when unchanged, not installed without a battery or without a resume offset, that a failed install fails setup (so the migration retries), and that it is removed with hibernation. They fail with quattro's setup and remove/etc/UPower/UPower.conf.d/README.md(upower 1.91.3)manual/36-system-sleep.mdshow how to pick another level with a later drop-in (99-local.conf), as suggested in review./test/shell: same 11 failures with and without this change (unrelated)