commit 46fb4edfb1267cd0ab816329fe87575c55a4a373 Spenser Truex <truex@equwal.com> 2026-09-21 14:02:47 -0700 bm, bm-sync: a write lock, so that no change is lost bm and bm-sync changed the bookmark file without a lock. A line that bm added between the check and the write of bm-sync was lost. A delete or an edit that read the file before the write of bm-sync and wrote it after, removed the merged lines, and the next sync then deleted them on the server and on all devices. bm-sync could also read a delete of bm half written, and send a file that lacks lines. Now both take the write lock, the directory $BOOKMARKS.lock: - bm for each add, merge and delete, and for the whole editor session of bm -e; - bm-sync to read the file that it sends, and from its check to its write. It 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 next change in bm syncs again. The 9 new tests fail with the old bm and bm-sync.
README | 6 ++++ bm | 43 +++++++++++++++++++++++++++ bm-sync | 51 ++++++++++++++++++++++++++----- test/run.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 191 insertions(+), 8 deletions(-)
diff --git a/README b/README index 86b5931..d39c3f9 100644 --- a/README +++ b/README @@ -205,6 +205,11 @@ network. The sign-in token is kept in ~/.config/sbm/sync. An empty or missing bookmark file removes nothing on the other devices: bm-sync gets all the bookmarks back from the server. +bm and bm-sync lock the file for each change, with the directory +$BOOKMARKS.lock, so that neither writes over a change of the other. bm -e +holds the lock until the editor ends; bm-sync waits for it, and bm syncs +the edit after. A lock left by a process that is gone is taken over. + The server is sbm-sync (https://github.com/equwal/sbm-sync). bm-sync uses https://sbm.subread.space unless you give another server: create an account there. The server is free software, so you can run your own: @@ -238,6 +243,7 @@ All through the environment, all optional. SBM_COPY command that reads the new clipboard contents from stdin SBM_PASTE command that writes the clipboard contents to stdout SBM_OPEN command that opens the URL given as its argument + SBM_LOCK_WAIT seconds that bm and bm-sync wait for the lock (default 10) With a display bm uses dmenu; fzf is only used when dmenu is missing or there is no X11 or Wayland display. A custom SBM_MENU is called with the diff --git a/bm b/bm index 8baff33..af6345d 100755 --- a/bm +++ b/bm @@ -14,6 +14,8 @@ USERTAGS=${USERTAGS:-$DATADIR/usertags} ENGINES=${SBM_ENGINES:-$DATADIR/engines} SEARCH=${SBM_SEARCH:-https://duckduckgo.com/?q=%s} USAGE=$BOOKMARKS.usage +WLOCK=$BOOKMARKS.lock +locked= TAB=$(printf '\t') # Two URLs are the same bookmark when they differ only in scheme, a leading @@ -301,6 +303,36 @@ rewrite () { return $rc } +# lock: take the write lock of the bookmark file, a directory beside it. bm +# holds it while it changes the file, and bm-sync while it reads or writes +# the file, so that neither writes over a change of the other. A lock whose +# process is gone is taken over. After $SBM_LOCK_WAIT seconds (default 10) +# bm stops. +lock () { + 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 (bm -e or bm-sync). If neither runs, remove $WLOCK" + sleep 1 + waited=$((waited + 1)) + done + echo $$ > "$WLOCK/pid" + locked=1 + # Also in the subshell of add_inside, which does not keep these traps. + trap unlock EXIT + trap 'exit 1' HUP INT TERM +} + +unlock () { + [ -z "$locked" ] || rm -rf "$WLOCK" + locked= +} + # bump <url>: count a use, for the "used" order. The counts live beside the # bookmark file, which stays a plain list: url, count, time of last use. # srand() returns the previous seed, and the first seed is the time of day: @@ -345,19 +377,26 @@ open_url () { opener "$1" } +# The editor reads the file when it starts and writes it when you save, so +# bm holds the lock until the editor ends. bm-sync then waits, and syncs the +# edit after it. edit_url () { + lock line=$(SBM_URL=$1 awk -F'\t' \ '$1 == ENVIRON["SBM_URL"] { print NR; exit }' "$BOOKMARKS") ${VISUAL:-${EDITOR:-vi}} "+${line:-1}" "$BOOKMARKS" + unlock changed 'bm: edit' } delete_url () { + lock for file in "$BOOKMARKS" "$USAGE"; do [ -e "$file" ] || continue SBM_URL=$1 awk -F'\t' '$1 != ENVIRON["SBM_URL"]' "$file" \ | rewrite "$file" || die "could not rewrite $file" done + unlock changed "bm: delete $1" } @@ -431,7 +470,9 @@ add () { printf '%s\n' "$names" | learn_tags tags=$(printf '%s\n' "$names" | paste -sd ' ' -) + lock printf '%s\t%s\t%s\n' "$url" "$desc" "$tags" >> "$BOOKMARKS" + unlock changed "bm: add $url" } @@ -439,6 +480,7 @@ add () { # known yet are appended. This is how the output of bm-import gets in. merge () { new=$(tmpfile "$BOOKMARKS") || die 'cannot create a temporary file' + lock SBM_FILE=$BOOKMARKS SBM_NEW=$new awk -F'\t' "$AWK_NORM"' BEGIN { while ((getline line < ENVIRON["SBM_FILE"]) > 0) { @@ -452,6 +494,7 @@ merge () { END { printf "added %d, skipped %d duplicates\n", added, skipped }' cut -f3 "$new" | tr ' ' '\n' | sort -u | learn_tags cat "$new" >> "$BOOKMARKS" + unlock rm -f "$new" changed 'bm: merge' } diff --git a/bm-sync b/bm-sync index 8fdb56e..2e441b2 100755 --- a/bm-sync +++ b/bm-sync @@ -13,6 +13,7 @@ CONFIG=${SBM_SYNC_CONFIG:-${XDG_CONFIG_HOME:-$HOME/.config}/sbm/sync} SERVER=https://sbm.subread.space STATE=$BOOKMARKS.sync # name of the version of the last sync LOCK=$BOOKMARKS.sync.lock +WLOCK=$BOOKMARKS.lock # the write lock of bm quiet= usage () { @@ -51,6 +52,33 @@ private () { cleanup () { rm -rf "$work" [ -z "$locked" ] || rm -rf "$LOCK" + wunlock +} + +# wlock: take the write lock of bm, as bm does. bm holds it while it changes +# the file: while bm -e runs, that is until the editor ends. After +# $SBM_LOCK_WAIT seconds (default 10) bm-sync stops, and the next change 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 (bm -e?): sync again later" + sleep 1 + waited=$((waited + 1)) + done + echo $$ > "$WLOCK/pid" + wlocked=1 +} + +wunlock () { + [ -z "$wlocked" ] || rm -rf "$WLOCK" + wlocked= } login () { @@ -137,7 +165,12 @@ sync () { tries=0 while :; do rm -f "$LOCK/again" + # bm writes a delete as an empty file and then the new text. Without + # the lock, bm-sync could send the half-written file, and the server + # would delete the lines that it lacks. + wlock cat "$BOOKMARKS" > "$work/sent" + wunlock # An empty file with a version would delete all bookmarks on the # server, and then on all devices. Without a version, the server # deletes nothing, and sends all bookmarks back. @@ -155,19 +188,18 @@ 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 change of bm + # comes between them. + wlock # bm changed the file while it was away: send the new file. The # answer to the old one is of no use. 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 fi - # A known race: bm and bm-sync do not lock the file against each - # other. If bm adds a line after the cmp above and before the write - # below, the write removes that line. If bm reads the file for a - # delete or an edit before the write and writes it after the write, - # bm removes the merged lines. The next sync then deletes them on the - # server. A fix needs a lock that bm and bm-sync take for each write. + wrote= if ! cmp -s "$work/out" "$BOOKMARKS"; then # Forget the version first, and keep the new one only after the # write. If the write stops part way (a full disk), the next sync @@ -175,9 +207,13 @@ sync () { rm -f "$STATE" # Through cat, not mv, as in bm: a symlinked file stays a symlink. cat "$work/out" > "$BOOKMARKS" || die "cannot write $BOOKMARKS" - if command -v bm-commit >/dev/null 2>&1; then bm-commit 'bm-sync: merge'; fi + wrote=1 fi printf '%s\n' "$version" > "$STATE" + wunlock + if [ -n "$wrote" ] && command -v bm-commit >/dev/null 2>&1; then + bm-commit 'bm-sync: merge' + fi [ -e "$LOCK/again" ] || break done say "in sync: $(grep -c -v -e '^#' -e '^[[:space:]]*$' "$BOOKMARKS") bookmarks" @@ -199,6 +235,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' INT TERM diff --git a/test/run.sh b/test/run.sh index fbf9dc7..083a1f1 100755 --- a/test/run.sh +++ b/test/run.sh @@ -950,7 +950,9 @@ fi # $srv/code, the server refuses with that status. With $srv/touch, bm adds a # bookmark while the request runs. Without a version, the server deletes # nothing, as the real one: it keeps its file and adds the new lines. With -# $srv/broken, the answer is a directory, so bm-sync cannot write it. +# $srv/broken, the answer is a directory, so bm-sync cannot write it. With +# $srv/hold, a process takes the write lock of bm while the request runs, +# and keeps it; its pid goes to $srv/holder. srv="$work/srv" mkdir "$srv" "$work/syncnet" cat > "$work/syncnet/curl" <<'FAKE' @@ -998,6 +1000,11 @@ if [ -e "$SBM_TEST_SRV/touch" ]; then rm -f "$SBM_TEST_SRV/touch" printf 'https://c.example\tC\t\n' >> "$BOOKMARKS" fi +if [ -e "$SBM_TEST_SRV/hold" ]; then + sleep 30 >/dev/null 2>&1 & + echo $! > "$SBM_TEST_SRV/holder" + mkdir "$BOOKMARKS.lock" && echo $! > "$BOOKMARKS.lock/pid" +fi printf 200 FAKE chmod +x "$work/syncnet/curl" @@ -1071,6 +1078,36 @@ bmsync -q eq 'after a failed write, the next sync has no version, so the server deletes nothing' \ "$(kept)" ':kept:back' +# While bm holds its write lock, bm-sync neither reads nor writes the file. +export SBM_LOCK_WAIT=1 +cp "$BOOKMARKS" "$work/before" +cp "$BOOKMARKS.sync" "$work/state" +printf 'https://other.example\tOther\t\n' > "$srv/other" +sleep 30 >/dev/null 2>&1 & +holder=$! +mkdir "$BOOKMARKS.lock" && echo "$holder" > "$BOOKMARKS.lock/pid" +bmsync -q 2>"$work/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' "$work/err"):$(cmp -s "$work/before" "$BOOKMARKS" && echo same)" \ + '1:1:same' +# bm takes the lock while the request runs: bm-sync must not write. +: > "$srv/hold" +bmsync -q 2>"$work/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 old version' \ + "$rc:$(cmp -s "$work/before" "$BOOKMARKS" && echo same):$(cmp -s "$work/state" "$BOOKMARKS.sync" && echo same)" \ + '1:same:same' +printf 'https://other.example\tOther\t\n' > "$srv/other" +bmsync -q +eq 'after the lock is free, the next sync brings the change of the other device' \ + "$(grep -c other.example "$BOOKMARKS"):$(ls -d "$BOOKMARKS.lock" 2>/dev/null)" '1:' +unset SBM_LOCK_WAIT + bmsync -q logout eq 'bm-sync logout forgets the token and signs out on the server' \ "$([ -e "$SBM_SYNC_CONFIG" ] || echo gone) $(cat "$srv/log")" 'gone logout' @@ -1095,6 +1132,66 @@ done eq 'bm starts bm-sync with the config of the tests, never the real one' \ "$(sort -u "$work/stubsync.log")" "$work/sync.conf" +# ---- the write lock of the bookmark file ---- + +# hold: a live process takes the write lock; its pid goes to $holder. +hold () { + sleep 30 >/dev/null 2>&1 & + holder=$! + mkdir "$BOOKMARKS.lock" && echo "$holder" > "$BOOKMARKS.lock/pid" +} +free () { + kill "$holder" 2>/dev/null + rm -rf "$BOOKMARKS.lock" +} + +reset +# The other process adds a line after two seconds, and then frees the lock. +mkdir "$BOOKMARKS.lock" +( sleep 2; printf 'https://first.example\tFirst\t\n' >> "$BOOKMARKS" + rm -rf "$BOOKMARKS.lock" ) & +first=$! +echo "$first" > "$BOOKMARKS.lock/pid" +answers 'Second' '' +SBM_LOCK_WAIT=10 $BM -a https://second.example 2>/dev/null +wait "$first" +eq 'add waits for the lock, and adds after the change of the other process' \ + "$(cut -f1 "$BOOKMARKS" | paste -sd ' ' -)" 'https://first.example https://second.example' + +reset +printf 'https://kept.example\tKept\t\n' > "$BOOKMARKS" +hold +answers 'New' '' +SBM_LOCK_WAIT=1 $BM -a https://new.example 2>"$work/err" +eq 'add stops when the lock stays, says why, and changes nothing' \ + "$?:$(grep -c 'in use' "$work/err"):$(cut -f1 "$BOOKMARKS")" '1:1:https://kept.example' +answers 'Kept' +SBM_LOCK_WAIT=1 $BM -d >/dev/null 2>&1 +eq 'delete waits for the lock too' "$(cut -f1 "$BOOKMARKS")" 'https://kept.example' +printf 'https://merged.example\n' | SBM_LOCK_WAIT=1 $BM -m >/dev/null 2>&1 +eq 'merge waits for the lock too' "$(cut -f1 "$BOOKMARKS")" 'https://kept.example' +free + +reset +sh -c 'exit 0' & +gone=$! +wait "$gone" +mkdir "$BOOKMARKS.lock" && echo "$gone" > "$BOOKMARKS.lock/pid" +answers 'After' '' +SBM_LOCK_WAIT=1 $BM -a https://after.example 2>/dev/null +eq 'add takes over the lock of a process that is gone, and frees it' \ + "$(cut -f1 "$BOOKMARKS"):$(ls -d "$BOOKMARKS.lock" 2>/dev/null)" 'https://after.example:' + +reset +printf 'https://ed.example\tEd\t\n' > "$BOOKMARKS" +printf '#!/bin/sh\n[ -d "%s" ] && echo locked > "%s"\n' \ + "$BOOKMARKS.lock" "$work/edit.log" > "$work/editor" +chmod +x "$work/editor" +answers 'Ed' +VISUAL="$work/editor" $BM -e 2>/dev/null +eq 'edit holds the lock while the editor runs, and frees it after' \ + "$(cat "$work/edit.log" 2>/dev/null):$(ls -d "$BOOKMARKS.lock" 2>/dev/null)" 'locked:' + # ---- make install ---- if command -v make >/dev/null 2>&1; then