From 9b9d4b39eb1db8e8320a5a2cbbc1a4f5e87965ce Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 26 Jul 2026 19:19:33 -0700 Subject: [PATCH] Never leave updatedb.conf with two definitions of the same setting updatedb refuses to run at all on a config that defines a variable twice ("variable `PRUNEPATHS' was already defined"), so any rewrite that misses an existing line and appends a second one takes the locate index down rather than fixing it. Two shapes updatedb accepts got missed: a trailing comment after the value, and a setting indented by whitespace. Read the existing paths out of the quoted value and write the whole setting back canonically instead of splicing into a line of unknown shape. Quotes are not optional to updatedb ("value in quotes expected after `='"), so a bare value is already a broken config: rewriting it quoted repairs the file as a side effect. The tests now hand every rewritten file to the real parser through `updatedb --config-file`, which is what caught this. Read the Snapper config as root when the running user cannot read it. snapper create-config leaves the config root-only, and a config the user cannot read was passing for one that wants its timeline snapshots kept. Report the snapshots the drain could not delete. omarchy-migrate writes the completion marker whether or not the batches succeeded, so there is no later run to pick up the remainder, whatever the comment claimed. Co-Authored-By: Claude Opus 5 (1M context) --- install/config/locate.sh | 25 +++++++------ migrations/1784809452.sh | 23 +++++++++--- test/shell.d/locate-test.sh | 41 ++++++++++++++++++++-- test/shell.d/snapper-timeline-leak-test.sh | 30 +++++++++++++--- 4 files changed, 97 insertions(+), 22 deletions(-) diff --git a/install/config/locate.sh b/install/config/locate.sh index 529fa1ab..cc95e9b8 100644 --- a/install/config/locate.sh +++ b/install/config/locate.sh @@ -4,24 +4,29 @@ echo "Configuring locate to skip Btrfs snapshots and index Btrfs subvolumes" [[ -f $UPDATEDB_CONF_PATH ]] || exit 0 +# updatedb refuses to run at all on a config that defines a variable twice, so +# every setting here is rewritten where it already stands and only appended +# when the file has no line for it. + # Btrfs subvolume mounts (like /home) look like bind mounts, so pruning # bind mounts leaves them out of the index entirely. -if grep -qE '^PRUNE_BIND_MOUNTS[[:space:]]*=' "$UPDATEDB_CONF_PATH"; then - sed -i -E 's|^PRUNE_BIND_MOUNTS[[:space:]]*=.*|PRUNE_BIND_MOUNTS = "no"|' "$UPDATEDB_CONF_PATH" +if grep -qE '^[[:space:]]*PRUNE_BIND_MOUNTS[[:space:]]*=' "$UPDATEDB_CONF_PATH"; then + sed -i -E 's|^[[:space:]]*PRUNE_BIND_MOUNTS[[:space:]]*=.*|PRUNE_BIND_MOUNTS = "no"|' "$UPDATEDB_CONF_PATH" else printf '%s\n' 'PRUNE_BIND_MOUNTS = "no"' >>"$UPDATEDB_CONF_PATH" fi # Snapper snapshots are nested subvolumes reached by plain directory # traversal, so without this updatedb indexes the system once per snapshot. -# The quotes are optional in updatedb.conf, so read the paths back out of -# whatever quoting the file uses and write the setting in one canonical form. -pruned=$(sed -nE 's|^PRUNEPATHS[[:space:]]*=[[:space:]]*"?([^"]*)"?[[:space:]]*$|\1|p' "$UPDATEDB_CONF_PATH" | tail -n 1) +if grep -qE '^[[:space:]]*PRUNEPATHS[[:space:]]*=' "$UPDATEDB_CONF_PATH"; then + # updatedb only accepts quoted values and allows a comment after them. Read + # back what the machine already prunes and write the whole setting out again + # rather than splicing into a line of unknown shape. + pruned=$(sed -nE 's|^[[:space:]]*PRUNEPATHS[[:space:]]*=[[:space:]]*"([^"]*)".*|\1|p' "$UPDATEDB_CONF_PATH" | tail -n 1) -if [[ " $pruned " != *" /.snapshots "* ]]; then - if [[ -n $pruned ]]; then - sed -i -E "s|^PRUNEPATHS[[:space:]]*=.*|PRUNEPATHS = \"/.snapshots $pruned\"|" "$UPDATEDB_CONF_PATH" - else - printf '%s\n' 'PRUNEPATHS = "/.snapshots"' >>"$UPDATEDB_CONF_PATH" + if [[ " $pruned " != *" /.snapshots "* ]]; then + sed -i -E "s|^[[:space:]]*PRUNEPATHS[[:space:]]*=.*|PRUNEPATHS = \"/.snapshots${pruned:+ $pruned}\"|" "$UPDATEDB_CONF_PATH" fi +else + printf '%s\n' 'PRUNEPATHS = "/.snapshots"' >>"$UPDATEDB_CONF_PATH" fi diff --git a/migrations/1784809452.sh b/migrations/1784809452.sh index e32da36b..d70d7659 100644 --- a/migrations/1784809452.sh +++ b/migrations/1784809452.sh @@ -14,8 +14,14 @@ command -v snapper >/dev/null || exit 0 [[ -f $SNAPPER_CONFIG_PATH ]] || exit 0 # Only clean up when timeline snapshotting is off, as Omarchy configures it. -# Anyone who deliberately turned it back on keeps their snapshots. -grep -qFx 'TIMELINE_CREATE="no"' "$SNAPPER_CONFIG_PATH" || exit 0 +# Anyone who deliberately turned it back on keeps their snapshots. Snapper's +# own create-config leaves the file readable by root alone, and a config this +# user cannot read must not pass for one that wants its snapshots kept. +if [[ -r $SNAPPER_CONFIG_PATH ]]; then + grep -qFx 'TIMELINE_CREATE="no"' "$SNAPPER_CONFIG_PATH" || exit 0 +else + as_root grep -qFx 'TIMELINE_CREATE="no"' "$SNAPPER_CONFIG_PATH" || exit 0 +fi # Earlier installs ran hourly timeline snapshots. Later configs stopped # creating them but never deleted the existing ones, and number cleanup @@ -28,16 +34,23 @@ echo "Deleting $(wc -w <<<"$leaked") leaked timeline snapshots (disk space is re # Delete in small batches; one big delete can die on a DBus timeout partway. # A failed batch must not take the rest of the migration run down with it, so -# the drain is best effort: whatever survives is picked up by the next run. +# the drain is best effort. omarchy-migrate records the migration either way, +# so say what is left rather than counting on a rerun that will not come. +failed=0 batch=() + for number in $leaked; do batch+=("$number") if (( ${#batch[@]} == 20 )); then - as_root snapper -c root delete "${batch[@]}" || true + as_root snapper -c root delete "${batch[@]}" || failed=$((failed + ${#batch[@]})) batch=() fi done if (( ${#batch[@]} > 0 )); then - as_root snapper -c root delete "${batch[@]}" || true + as_root snapper -c root delete "${batch[@]}" || failed=$((failed + ${#batch[@]})) +fi + +if (( failed > 0 )); then + echo "$failed snapshots could not be deleted. Finish with: sudo snapper -c root delete " fi diff --git a/test/shell.d/locate-test.sh b/test/shell.d/locate-test.sh index 77446103..cab54c24 100644 --- a/test/shell.d/locate-test.sh +++ b/test/shell.d/locate-test.sh @@ -18,6 +18,19 @@ PRUNEPATHS = "/afs /media /mnt /net /sfs /tmp /udev /var/cache /var/lib/pacman/l CONF } +# updatedb dies on a config that defines a variable twice, so hand every +# rewritten file to the real parser rather than trusting the greps below. +empty_tree="$test_tmp/empty-tree" +mkdir -p "$empty_tree" + +assert_conf_parses() { + command -v updatedb >/dev/null || return 0 + + local errors + errors=$(updatedb --config-file "$1" -U "$empty_tree" -o "$test_tmp/plocate.db" 2>&1 >/dev/null | grep -F "$1:" || true) + [[ -z $errors ]] || fail "updatedb accepts the rewritten config" "$errors" +} + conf="$test_tmp/updatedb.conf" stock_conf "$conf" @@ -25,11 +38,13 @@ OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/nul grep -qFx 'PRUNE_BIND_MOUNTS = "no"' "$conf" || fail "locate config indexes Btrfs subvolume mounts like /home" grep -qF 'PRUNEPATHS = "/.snapshots /afs' "$conf" || fail "locate config prunes /.snapshots" +assert_conf_parses "$conf" pass "locate config skips Btrfs snapshots and indexes Btrfs subvolumes" OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/null [[ $(grep -o '/\.snapshots' "$conf" | wc -l) -eq 1 ]] || fail "locate config is idempotent" +assert_conf_parses "$conf" pass "locate config leaves an already-configured file alone" OMARCHY_UPDATEDB_CONF_PATH="$test_tmp/missing.conf" bash -euo pipefail "$config_script" >/dev/null @@ -44,6 +59,7 @@ OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/nul grep -qFx 'PRUNE_BIND_MOUNTS = "no"' "$conf" || fail "locate config adds a missing PRUNE_BIND_MOUNTS" grep -qFx 'PRUNEPATHS = "/.snapshots"' "$conf" || fail "locate config adds a missing PRUNEPATHS" +assert_conf_parses "$conf" pass "locate config adds settings a hand-edited updatedb.conf is missing" conf="$test_tmp/unspaced-updatedb.conf" @@ -53,16 +69,34 @@ OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/nul grep -qFx 'PRUNE_BIND_MOUNTS = "no"' "$conf" || fail "locate config rewrites an unspaced PRUNE_BIND_MOUNTS" grep -qFx 'PRUNEPATHS = "/.snapshots /tmp /var/tmp"' "$conf" || fail "locate config prunes /.snapshots in an unspaced PRUNEPATHS" -[[ $(grep -c '^PRUNEPATHS' "$conf") -eq 1 ]] || fail "locate config keeps a single PRUNEPATHS setting" +[[ $(grep -c 'PRUNEPATHS' "$conf") -eq 1 ]] || fail "locate config keeps a single PRUNEPATHS setting" +assert_conf_parses "$conf" pass "locate config handles updatedb.conf written without spaces around =" +# updatedb allows a comment after a value and indented settings, and defining +# either setting twice makes it refuse to run at all. +conf="$test_tmp/commented-updatedb.conf" +printf '%s\n' ' PRUNE_BIND_MOUNTS = "yes" # subvolumes look like bind mounts' \ + 'PRUNEPATHS = "/tmp" # scratch' >"$conf" + +OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/null + +grep -qFx 'PRUNE_BIND_MOUNTS = "no"' "$conf" || fail "locate config rewrites an indented PRUNE_BIND_MOUNTS" +grep -qFx 'PRUNEPATHS = "/.snapshots /tmp"' "$conf" || fail "locate config keeps the paths a commented PRUNEPATHS already prunes" +[[ $(grep -c 'PRUNEPATHS' "$conf") -eq 1 ]] || fail "locate config replaces a commented PRUNEPATHS instead of adding a second one" +assert_conf_parses "$conf" +pass "locate config handles indented settings and trailing comments" + +# A hand-edited file may have dropped the quotes updatedb requires, which +# leaves it unparseable until something writes the setting out properly. conf="$test_tmp/unquoted-updatedb.conf" printf '%s\n' 'PRUNEPATHS = /tmp' >"$conf" OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/null -grep -qFx 'PRUNEPATHS = "/.snapshots /tmp"' "$conf" || fail "locate config keeps the paths an unquoted PRUNEPATHS already prunes" -[[ $(grep -c '^PRUNEPATHS' "$conf") -eq 1 ]] || fail "locate config replaces an unquoted PRUNEPATHS instead of adding a second one" +grep -qFx 'PRUNEPATHS = "/.snapshots"' "$conf" || fail "locate config repairs an unquoted PRUNEPATHS" +[[ $(grep -c 'PRUNEPATHS' "$conf") -eq 1 ]] || fail "locate config replaces an unquoted PRUNEPATHS instead of adding a second one" +assert_conf_parses "$conf" pass "locate config handles updatedb.conf written without quotes" # A path that merely ends in /.snapshots is not the root snapshot directory. @@ -72,6 +106,7 @@ printf '%s\n' 'PRUNEPATHS = "/var/lib/machines/.snapshots"' >"$conf" OMARCHY_UPDATEDB_CONF_PATH="$conf" bash -euo pipefail "$config_script" >/dev/null grep -qFx 'PRUNEPATHS = "/.snapshots /var/lib/machines/.snapshots"' "$conf" || fail "locate config prunes /.snapshots alongside a path that ends in it" +assert_conf_parses "$conf" pass "locate config tells /.snapshots apart from a path that ends in it" locate_migration=$(grep -rl 'Configure locate to skip Btrfs snapshots' "$ROOT/migrations" | head -n 1 || true) diff --git a/test/shell.d/snapper-timeline-leak-test.sh b/test/shell.d/snapper-timeline-leak-test.sh index a5adb84f..21b15ac7 100644 --- a/test/shell.d/snapper-timeline-leak-test.sh +++ b/test/shell.d/snapper-timeline-leak-test.sh @@ -15,6 +15,7 @@ mkdir -p "$fake_bin" cat >"$fake_bin/sudo" <<'STUB' #!/bin/bash +printf 'sudo %s\n' "$*" >>"$TEST_LOG" exec "$@" STUB chmod +x "$fake_bin/sudo" @@ -72,14 +73,18 @@ echo "failure: dbus timeout" >&2 exit 1 STUB -TEST_LOG="$test_tmp/calls.log" \ -PATH="$fake_bin:$PATH" \ -OMARCHY_SNAPPER_CONFIG_PATH="$snapper_config" \ - bash -euo pipefail "$leak_migration" >/dev/null 2>&1 || +output=$(TEST_LOG="$test_tmp/calls.log" \ + PATH="$fake_bin:$PATH" \ + OMARCHY_SNAPPER_CONFIG_PATH="$snapper_config" \ + bash -euo pipefail "$leak_migration" 2>/dev/null) || fail "leak migration survives a failed delete batch" deletes=$(grep -c '^snapper -c root delete ' "$test_tmp/calls.log" || true) [[ $deletes -eq 3 ]] || fail "leak migration keeps draining after a failed batch" "expected 3 delete calls, got $deletes" + +# omarchy-migrate writes the completion marker even when the drain gave up, so +# what is left has to be said out loud rather than left for a rerun. +grep -qF '45 snapshots could not be deleted' <<<"$output" || fail "leak migration reports the snapshots it could not delete" "$output" pass "leak migration tolerates a batch that fails partway" : >"$test_tmp/calls.log" @@ -102,3 +107,20 @@ OMARCHY_SNAPPER_CONFIG_PATH="$test_tmp/missing" \ [[ ! -s $test_tmp/calls.log ]] || fail "leak migration skips systems without a Snapper root config" pass "leak migration is a no-op without Snapper configured" + +# Snapper's create-config writes a root-only config, and a config this user +# cannot read says nothing about whether timeline snapshots are wanted. +: >"$test_tmp/calls.log" +printf '%s\n' 'TIMELINE_CREATE="no"' >"$snapper_config" +chmod 000 "$snapper_config" + +TEST_LOG="$test_tmp/calls.log" \ +PATH="$fake_bin:$PATH" \ +OMARCHY_SNAPPER_CONFIG_PATH="$snapper_config" \ + bash -euo pipefail "$leak_migration" >/dev/null 2>&1 + +chmod 600 "$snapper_config" + +grep -qF "sudo grep -qFx TIMELINE_CREATE=\"no\" $snapper_config" "$test_tmp/calls.log" || + fail "leak migration reads a root-only Snapper config as root" "$(cat "$test_tmp/calls.log")" +pass "leak migration does not mistake an unreadable Snapper config for an intentional one"