commit 17c35931744210ec4d428681f1a0a6791a5d6bae Spenser Truex <truex@equwal.com> 2026-09-21 14:05:43 -0700 bm, bm-sync: a write lock, so that no bookmark is lost bm and bm-sync wrote the bookmark file without a lock. A line that bm added between the check and the write of bm-sync was lost. Now both take the write lock, the directory $BOOKMARKS.lock: bm for each add, and bm-sync to read the file that it sends, and from its check to its write. bm-sync holds no lock while the request runs. A lock whose process is gone is taken over. After $SBM_LOCK_WAIT seconds (default 10) bm stops with a message, and bm-sync stops without a write. The 5 new tests fail with the old bm and bm-sync.
bm | 32 ++++++++++++++++++++++++++++++++ bm-sync | 35 +++++++++++++++++++++++++++++++++++ test.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/bm b/bm index a8c9d8f..d45a726 100755 --- a/bm +++ b/bm @@ -16,6 +16,36 @@ help () { printf 'Flags:\n-h, --help\n-c, --copy\n-a, --add\n-e, --edit\n' } +# lock: take the write lock of $BOOKMARKS, a directory beside it. bm-sync +# takes it too, so that neither writes over a change of the other. The lock +# of a process that is gone is taken over. bm waits $SBM_LOCK_WAIT seconds +# (default 10) for it. +lock () { + waited=0 + until mkdir "$BOOKMARKS.lock" 2>/dev/null; do + pid=$(cat "$BOOKMARKS.lock/pid" 2>/dev/null) + if [ -n "$pid" ] && ! kill -0 "$pid" 2>/dev/null; then + [ "$(cat "$BOOKMARKS.lock/pid" 2>/dev/null)" != "$pid" ] || rm -rf "$BOOKMARKS.lock" + continue + fi + if [ "$waited" -ge "${SBM_LOCK_WAIT:-10}" ]; then + printf 'bm: %s is in use. If no bm-sync runs, remove %s.lock\n' \ + "$BOOKMARKS" "$BOOKMARKS" >&2 + exit 1 + fi + sleep 1 + waited=$((waited + 1)) + done + echo $$ > "$BOOKMARKS.lock/pid" + trap 'rm -rf "$BOOKMARKS.lock"' EXIT + trap 'exit 1' HUP INT TERM +} + +unlock () { + rm -rf "$BOOKMARKS.lock" + trap - EXIT +} + copy () { sort -t2 "$BOOKMARKS" | dmenu -i -l $DMENULINES -p "bookmark:" | awk '{ print $1 }' | xclip -i -selection clipboard } @@ -34,7 +64,9 @@ add () { tags="$(printf "$tags" | tr ' ' '\n' | sort | uniq | tr '\n' ' ' \ | sed 's/ $//')" + lock printf "%s %s | %s\\n" "$url" "$desc" "$tags" >> "$BOOKMARKS" + unlock # bm-sync, if installed, sends the bookmark to your other devices. if command -v bm-sync >/dev/null 2>&1; then bm-sync -q >/dev/null 2>&1 & fi } diff --git a/bm-sync b/bm-sync index 8839d35..710e7d9 100755 --- a/bm-sync +++ b/bm-sync @@ -10,6 +10,7 @@ CONFIG=${SBM_SYNC_CONFIG:-${XDG_CONFIG_HOME:-$HOME/.config}/sbm/sync} SERVER=https://sbm.subread.space STATE=$BOOKMARKS.sync # the version of the last sync LOCK=$BOOKMARKS.sync.lock +WLOCK=$BOOKMARKS.lock # the write lock of bm quiet= usage () { @@ -40,6 +41,32 @@ setting () { cleanup () { rm -rf "$work" [ -z "$locked" ] || rm -rf "$LOCK" + wunlock +} + +# wlock: take the write lock of bm, as bm does, so that neither writes over +# a change of the other. After $SBM_LOCK_WAIT seconds (default 10) bm-sync +# stops, and the next add in bm syncs again. +wlock () { + waited=0 + until mkdir "$WLOCK" 2>/dev/null; do + pid=$(cat "$WLOCK/pid" 2>/dev/null) + if [ -n "$pid" ] && ! kill -0 "$pid" 2>/dev/null; then + [ "$(cat "$WLOCK/pid" 2>/dev/null)" != "$pid" ] || rm -rf "$WLOCK" + continue + fi + [ "$waited" -lt "${SBM_LOCK_WAIT:-10}" ] \ + || die "$BOOKMARKS is in use: sync again later" + sleep 1 + waited=$((waited + 1)) + done + echo $$ > "$WLOCK/pid" + wlocked=1 +} + +wunlock () { + [ -z "$wlocked" ] || rm -rf "$WLOCK" + wlocked= } login () { @@ -119,7 +146,9 @@ sync () { tries=0 while :; do rm -f "$LOCK/again" + wlock cat "$BOOKMARKS" > "$work/sent" || die "cannot read $BOOKMARKS" + wunlock # An empty file with a version tells the server to delete all # bookmarks. Without a version, the server deletes nothing. [ -s "$work/sent" ] || rm -f "$STATE" @@ -136,8 +165,12 @@ sync () { version=$(tr -d '\r' < "$work/head" | sed -n 's/^[Ss][Bb][Mm]-[Vv][Ee][Rr][Ss][Ii][Oo][Nn]: *//p') [ -n "$version" ] || die 'the server gave no version' + # Hold the lock from the check to the write, so that no add of bm + # comes between them. + wlock # bm changed the file during the request: send the new file. if ! cmp -s "$work/sent" "$BOOKMARKS"; then + wunlock tries=$((tries + 1)) [ $tries -lt 5 ] || die 'the file changes all the time: try again later' continue @@ -150,6 +183,7 @@ sync () { cat "$work/out" > "$BOOKMARKS" || die "cannot write $BOOKMARKS" fi printf '%s\n' "$version" > "$STATE" + wunlock [ -e "$LOCK/again" ] || break done say "in sync: $(grep -c . "$BOOKMARKS") bookmarks" @@ -172,6 +206,7 @@ command -v curl >/dev/null 2>&1 || die 'curl is not installed' work=${TMPDIR:-/tmp}/bm-sync.$$ mkdir -m 700 "$work" || die "cannot create $work" locked= +wlocked= trap cleanup EXIT trap 'exit 1' HUP INT TERM diff --git a/test.sh b/test.sh index 9c255c4..37201f4 100755 --- a/test.sh +++ b/test.sh @@ -53,7 +53,9 @@ EOF # curl: plays an sbm-sync server that keeps its file in $T/srv/file. Lines # in $T/srv/other come from another device, once. With $T/srv/code, the # server refuses with that status. With $T/srv/touch, bm adds a line during -# the request. With $T/srv/broken, bm-sync cannot read the answer. +# the request. With $T/srv/broken, bm-sync cannot read the answer. With +# $T/srv/hold, a process takes the write lock of bm during the request and +# keeps it; its pid goes to $T/srv/holder. cat > "$t/bin/curl" <<'EOF' #!/bin/sh srv=$T/srv @@ -95,6 +97,11 @@ if [ -e "$srv/touch" ]; then rm -f "$srv/touch" echo 'https://c.example C | ' >> "$BOOKMARKS" fi +if [ -e "$srv/hold" ]; then + sleep 30 >/dev/null 2>&1 & + echo $! > "$srv/holder" + mkdir "$BOOKMARKS.lock" && echo $! > "$BOOKMARKS.lock/pid" +fi printf 200 EOF chmod +x "$t/bin/dmenu" "$t/bin/xclip" "$t/bin/bm-sync" "$t/bin/curl" @@ -123,6 +130,38 @@ while [ ! -s "$t/bm-sync.log" ] && [ $i -lt 10 ]; do done eq 'add runs bm-sync -q' "$(cat "$t/bm-sync.log" 2>/dev/null)" -q +# The write lock. The other process adds a line after two seconds, and then +# frees the lock. +: > "$BOOKMARKS" +mkdir "$BOOKMARKS.lock" +( sleep 2; echo 'https://first.example First | ' >> "$BOOKMARKS" + rm -rf "$BOOKMARKS.lock" ) & +first=$! +echo "$first" > "$BOOKMARKS.lock/pid" +printf '%s\n' https://second.example Second done > "$t/answers" +SBM_LOCK_WAIT=10 $sh "$here/bm" -a +wait "$first" +eq 'add waits for the write lock, and adds after the other process' \ + "$(cut -d ' ' -f 1 "$BOOKMARKS" | tr '\n' ' ')" 'https://first.example https://second.example ' + +sleep 30 >/dev/null 2>&1 & +holder=$! +mkdir "$BOOKMARKS.lock" && echo "$holder" > "$BOOKMARKS.lock/pid" +printf '%s\n' https://new.example New done > "$t/answers" +SBM_LOCK_WAIT=1 $sh "$here/bm" -a 2>"$t/err" +eq 'add stops when the lock stays, says why, and adds nothing' \ + "$?:$(grep -c 'in use' "$t/err"):$(grep -c new.example "$BOOKMARKS")" '1:1:0' +kill "$holder" +rm -rf "$BOOKMARKS.lock" + +true & +wait $! +mkdir "$BOOKMARKS.lock" && echo $! > "$BOOKMARKS.lock/pid" +printf '%s\n' https://after.example After done > "$t/answers" +SBM_LOCK_WAIT=1 $sh "$here/bm" -a +eq 'add takes over the lock of a process that is gone, and frees it' \ + "$(grep -c after.example "$BOOKMARKS"):$(ls -d "$BOOKMARKS.lock" 2>/dev/null)" '1:' + # ---- bm-sync ---- bs () { @@ -172,6 +211,29 @@ eq 'on 402, bm-sync fails, says why, and keeps the file and the version' \ '1:bm-sync: sync is paused:same' rm -f "$srv/code" +# While bm holds the write lock, bm-sync neither reads nor writes the file. +export SBM_LOCK_WAIT=1 +echo 'https://other.example Other | ' > "$srv/other" +sleep 30 >/dev/null 2>&1 & +holder=$! +mkdir "$BOOKMARKS.lock" && echo "$holder" > "$BOOKMARKS.lock/pid" +bs -q 2>"$t/err" +rc=$? +kill "$holder" +rm -rf "$BOOKMARKS.lock" +eq 'bm-sync does not read the file while bm holds the lock, and says why' \ + "$rc:$(grep -c 'in use' "$t/err"):$(cmp -s "$t/file.before" "$BOOKMARKS" && echo same)" \ + '1:1:same' +: > "$srv/hold" +bs -q 2>"$t/err" +rc=$? +kill "$(cat "$srv/holder")" +rm -rf "$BOOKMARKS.lock" "$srv/hold" +eq 'bm-sync does not write while bm holds the lock, and keeps the version' \ + "$rc:$(cmp -s "$t/file.before" "$BOOKMARKS" && + cmp -s "$t/version.before" "$BOOKMARKS.sync" && echo same)" '1:same' +unset SBM_LOCK_WAIT + rm -f "$BOOKMARKS" bs -q eq 'bm-sync makes a missing file and sends it without a version' \