Skip to content

Commit 4d5ea97

Browse files
Crash0v3rrid3claude
Crash0v3rrid3
and
claude
committed
fix(spm): redesign concurrency lock to fully close the scan race (DEVA11Y-483)
Addresses all code-review findings on the first cut: - #1 Decide manifest ownership UNDER the lock from the live filesystem, not a startup PACKAGE_EXISTS snapshot. The lock is now taken unconditionally, so a peer that starts after the synthetic Package.swift already exists still serializes instead of running unprotected and getting its file deleted. - #2 Reclaim a crashed peer's lock by PID liveness (kill -0), not a 5-min mtime that would steal a slow-but-alive long scan's lock. - #3 Claim a stale lock atomically via rename so two waiters can't both reclaim. - #4 Wait-timeout is non-fatal: it skips the scan (exit 0) with a visible 'waiting...'/'skipping' notice instead of hanging a git commit then aborting it. - #5 A non-EEXIST mkdir failure (unwritable/read-only/full TMPDIR) fails fast with an actionable message instead of waiting out the full timeout. - #6 The lock lives under TMPDIR keyed by the package path, never inside the working tree, so a crash can't leave it to be git-added. - #7 Consistent 'A11y scan:' message prefix. - Also fixes a latent bug from the first cut: cleanup state (lock_dir/have_lock/ created_package) is now global, since the EXIT trap fires after a11y_scan returns when its locals are out of scope (verified) -- previously the lock was never released on a normal run. Verified with an integration test (staggered concurrent runs serialize; both scans see Package.swift throughout; no tree/TMPDIR residue) and unit tests for stale reclaim, live-owner detection, and fail-fast. Applied to bash/zsh/fish. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a3a8794 commit 4d5ea97

3 files changed

Lines changed: 234 additions & 126 deletions

File tree

‎scripts/bash/spm.sh‎

Lines changed: 78 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
#!/usr/bin/env bash -il
22

3-
[ -f "${PWD}/Package.swift" ]
4-
PACKAGE_EXISTS="$?"
53
GIT_ROOT=$(git rev-parse --show-toplevel 2>/dev/null)
64
SCRIPT_PATH=$(realpath --relative-to="$GIT_ROOT" "$0" 2>/dev/null || realpath "$0")
75
SUBCOMMAND="$1"
@@ -38,49 +36,76 @@ EOF
3836
fi
3937
}
4038

41-
a11y_scan() {
42-
# Serialize concurrent scans in the same directory (DEVA11Y-483). Without this the
43-
# first instance to exit fires cleanup() and deletes the shared synthetic
44-
# Package.swift out from under a still-running peer, so both scans fail
45-
# non-deterministically. mkdir is atomic on POSIX filesystems and portable to
46-
# macOS, which has no flock(1). Only lock when we own the synthetic file.
47-
local lock_dir="${PWD}/.browserstack-a11y-spm.lock"
48-
local have_lock=0
49-
if [ $PACKAGE_EXISTS -ne 0 ]; then
50-
local waited=0
51-
while ! mkdir "$lock_dir" 2>/dev/null; do
52-
# Reclaim a stale lock left by a peer killed before its trap could run.
53-
if [ -n "$(find "$lock_dir" -maxdepth 0 -mmin +5 2>/dev/null)" ]; then
54-
rm -rf -- "$lock_dir"
55-
continue
56-
fi
57-
if [ "$waited" -ge 300 ]; then
58-
echo "browserstack-a11y: timed out waiting for another scan in ${PWD}; remove ${lock_dir} if it is stale." >&2
59-
exit 1
60-
fi
61-
sleep 1
62-
waited=$((waited + 1))
63-
done
64-
have_lock=1
65-
fi
39+
# Resolve a lock directory OUTSIDE the scanned package, keyed by the package path,
40+
# so a crash can never leave a lock inside the user's working tree (nor let a
41+
# pre-commit `git add -A` stage it). cksum is POSIX and always present.
42+
_spm_lock_dir() {
43+
local key
44+
key=$(printf '%s' "$PWD" | cksum | cut -d' ' -f1)
45+
printf '%s/browserstack-a11y-spm-%s.lock' "${TMPDIR:-/tmp}" "$key"
46+
}
6647

67-
# Ensure Package.swift and the lock are removed on exit (acts like a finally block)
68-
cleanup() {
69-
if [ $PACKAGE_EXISTS -ne 0 ]; then
70-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
71-
fi
72-
if [ "$have_lock" -eq 1 ]; then
73-
rmdir "$lock_dir" 2>/dev/null
48+
# Acquire an exclusive per-directory lock via atomic mkdir. Serializes ALL
49+
# concurrent scans of the same directory regardless of when each started, so no
50+
# instance ever deletes a synthetic Package.swift another is still using. A lock
51+
# left by a crashed peer is reclaimed by PID liveness (kill -0) -- never by
52+
# wall-clock age, which cannot tell a slow-but-alive scan from a dead one -- and
53+
# the reclaim is claimed atomically via rename so two waiters can't both take it.
54+
# Returns: 0 acquired, 2 timed out (a peer is scanning), 1 lock dir unusable.
55+
_spm_acquire_lock() {
56+
local dir="$1" waited=0 announced=0 owner_pid stale
57+
while ! mkdir "$dir" 2>/dev/null; do
58+
# A non-EEXIST failure (unwritable/read-only/full TMPDIR) is not contention --
59+
# fail fast rather than waiting out the full timeout on a misleading message.
60+
if [ ! -d "$dir" ]; then
61+
echo "A11y scan: cannot create lock at ${dir} (check TMPDIR permissions/space)." >&2
62+
return 1
63+
fi
64+
owner_pid=$(cat "${dir}/pid" 2>/dev/null)
65+
if [ -n "$owner_pid" ] && ! kill -0 "$owner_pid" 2>/dev/null; then
66+
# Owner is dead: claim the stale lock. Only one racer's mv can succeed; the
67+
# losers fall through and retry mkdir. Never rm a path a peer may recreate.
68+
stale="${dir}.stale.$$"
69+
if mv "$dir" "$stale" 2>/dev/null; then
70+
rm -rf -- "$stale"
7471
fi
75-
}
76-
trap cleanup EXIT
72+
continue
73+
fi
74+
if [ "$announced" -eq 0 ]; then
75+
echo "A11y scan: waiting for another scan in ${PWD} to finish..." >&2
76+
announced=1
77+
fi
78+
if [ "$waited" -ge 300 ]; then
79+
return 2
80+
fi
81+
sleep 1
82+
waited=$((waited + 1))
83+
done
84+
echo "$$" > "${dir}/pid"
85+
return 0
86+
}
7787

78-
setup() {
79-
if [ $PACKAGE_EXISTS -eq 0 ]; then
80-
return
81-
fi
88+
# lock_dir/have_lock/created_package are GLOBAL on purpose: the EXIT trap fires
89+
# after a11y_scan has returned, when its `local`s are already out of scope, so
90+
# cleanup state must be global to survive.
91+
a11y_scan() {
92+
lock_dir=$(_spm_lock_dir)
93+
have_lock=0
94+
created_package=0
95+
96+
_spm_acquire_lock "$lock_dir"
97+
case "$?" in
98+
0) have_lock=1 ;;
99+
2) echo "A11y scan: another scan is already running in ${PWD}; skipping." >&2
100+
return 0 ;;
101+
*) echo "A11y scan: proceeding without a lock; concurrent scans in ${PWD} may conflict." >&2 ;;
102+
esac
82103

83-
cat > Package.swift <<EOF
104+
# Decide ownership UNDER the lock, from the live filesystem -- not a snapshot
105+
# taken at script start. Only create the synthetic manifest if none exists now,
106+
# and on exit delete only what this instance created.
107+
if [ ! -f "${PWD}/Package.swift" ]; then
108+
cat > Package.swift <<EOF
84109
// swift-tools-version: 5.9
85110
import PackageDescription
86111
@@ -92,9 +117,20 @@ let package = Package(
92117
targets: []
93118
)
94119
EOF
120+
created_package=1
121+
fi
122+
123+
# finally-block: remove only our own synthetic manifest, then release the lock.
124+
cleanup() {
125+
if [ "${created_package:-0}" -eq 1 ]; then
126+
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
127+
fi
128+
if [ "${have_lock:-0}" -eq 1 ]; then
129+
rm -rf -- "$lock_dir"
130+
fi
95131
}
132+
trap cleanup EXIT
96133

97-
setup
98134
if [[ -z "$EXTRA_ARGS" ]]; then
99135
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
100136
fi

‎scripts/fish/spm.sh‎

Lines changed: 78 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@ export BROWSERSTACK_USERNAME=$($fish_bin -lic 'echo $BROWSERSTACK_USERNAME' | ta
1313
export BROWSERSTACK_ACCESS_KEY=$($fish_bin -lic 'echo $BROWSERSTACK_ACCESS_KEY' | tail -n 1)
1414

1515
# Don't change anything after this, same as the bash equivalent
16-
[ -f "${PWD}/Package.swift" ]
17-
PACKAGE_EXISTS="$?"
1816
GIT_ROOT=$(git rev-parse --show-toplevel 2>/dev/null)
1917
SCRIPT_PATH=$(realpath --relative-to="$GIT_ROOT" "$0" 2>/dev/null || realpath "$0")
2018
SUBCOMMAND="$1"
@@ -51,49 +49,76 @@ EOF
5149
fi
5250
}
5351

54-
a11y_scan() {
55-
# Serialize concurrent scans in the same directory (DEVA11Y-483). Without this the
56-
# first instance to exit fires cleanup() and deletes the shared synthetic
57-
# Package.swift out from under a still-running peer, so both scans fail
58-
# non-deterministically. mkdir is atomic on POSIX filesystems and portable to
59-
# macOS, which has no flock(1). Only lock when we own the synthetic file.
60-
local lock_dir="${PWD}/.browserstack-a11y-spm.lock"
61-
local have_lock=0
62-
if [ $PACKAGE_EXISTS -ne 0 ]; then
63-
local waited=0
64-
while ! mkdir "$lock_dir" 2>/dev/null; do
65-
# Reclaim a stale lock left by a peer killed before its trap could run.
66-
if [ -n "$(find "$lock_dir" -maxdepth 0 -mmin +5 2>/dev/null)" ]; then
67-
rm -rf -- "$lock_dir"
68-
continue
69-
fi
70-
if [ "$waited" -ge 300 ]; then
71-
echo "browserstack-a11y: timed out waiting for another scan in ${PWD}; remove ${lock_dir} if it is stale." >&2
72-
exit 1
73-
fi
74-
sleep 1
75-
waited=$((waited + 1))
76-
done
77-
have_lock=1
78-
fi
52+
# Resolve a lock directory OUTSIDE the scanned package, keyed by the package path,
53+
# so a crash can never leave a lock inside the user's working tree (nor let a
54+
# pre-commit `git add -A` stage it). cksum is POSIX and always present.
55+
_spm_lock_dir() {
56+
local key
57+
key=$(printf '%s' "$PWD" | cksum | cut -d' ' -f1)
58+
printf '%s/browserstack-a11y-spm-%s.lock' "${TMPDIR:-/tmp}" "$key"
59+
}
7960

80-
# Ensure Package.swift and the lock are removed on exit (acts like a finally block)
81-
cleanup() {
82-
if [ $PACKAGE_EXISTS -ne 0 ]; then
83-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
84-
fi
85-
if [ "$have_lock" -eq 1 ]; then
86-
rmdir "$lock_dir" 2>/dev/null
61+
# Acquire an exclusive per-directory lock via atomic mkdir. Serializes ALL
62+
# concurrent scans of the same directory regardless of when each started, so no
63+
# instance ever deletes a synthetic Package.swift another is still using. A lock
64+
# left by a crashed peer is reclaimed by PID liveness (kill -0) -- never by
65+
# wall-clock age, which cannot tell a slow-but-alive scan from a dead one -- and
66+
# the reclaim is claimed atomically via rename so two waiters can't both take it.
67+
# Returns: 0 acquired, 2 timed out (a peer is scanning), 1 lock dir unusable.
68+
_spm_acquire_lock() {
69+
local dir="$1" waited=0 announced=0 owner_pid stale
70+
while ! mkdir "$dir" 2>/dev/null; do
71+
# A non-EEXIST failure (unwritable/read-only/full TMPDIR) is not contention --
72+
# fail fast rather than waiting out the full timeout on a misleading message.
73+
if [ ! -d "$dir" ]; then
74+
echo "A11y scan: cannot create lock at ${dir} (check TMPDIR permissions/space)." >&2
75+
return 1
76+
fi
77+
owner_pid=$(cat "${dir}/pid" 2>/dev/null)
78+
if [ -n "$owner_pid" ] && ! kill -0 "$owner_pid" 2>/dev/null; then
79+
# Owner is dead: claim the stale lock. Only one racer's mv can succeed; the
80+
# losers fall through and retry mkdir. Never rm a path a peer may recreate.
81+
stale="${dir}.stale.$$"
82+
if mv "$dir" "$stale" 2>/dev/null; then
83+
rm -rf -- "$stale"
8784
fi
88-
}
89-
trap cleanup EXIT
85+
continue
86+
fi
87+
if [ "$announced" -eq 0 ]; then
88+
echo "A11y scan: waiting for another scan in ${PWD} to finish..." >&2
89+
announced=1
90+
fi
91+
if [ "$waited" -ge 300 ]; then
92+
return 2
93+
fi
94+
sleep 1
95+
waited=$((waited + 1))
96+
done
97+
echo "$$" > "${dir}/pid"
98+
return 0
99+
}
90100

91-
setup() {
92-
if [ $PACKAGE_EXISTS -eq 0 ]; then
93-
return
94-
fi
101+
# lock_dir/have_lock/created_package are GLOBAL on purpose: the EXIT trap fires
102+
# after a11y_scan has returned, when its `local`s are already out of scope, so
103+
# cleanup state must be global to survive.
104+
a11y_scan() {
105+
lock_dir=$(_spm_lock_dir)
106+
have_lock=0
107+
created_package=0
108+
109+
_spm_acquire_lock "$lock_dir"
110+
case "$?" in
111+
0) have_lock=1 ;;
112+
2) echo "A11y scan: another scan is already running in ${PWD}; skipping." >&2
113+
return 0 ;;
114+
*) echo "A11y scan: proceeding without a lock; concurrent scans in ${PWD} may conflict." >&2 ;;
115+
esac
95116

96-
cat > Package.swift <<EOF
117+
# Decide ownership UNDER the lock, from the live filesystem -- not a snapshot
118+
# taken at script start. Only create the synthetic manifest if none exists now,
119+
# and on exit delete only what this instance created.
120+
if [ ! -f "${PWD}/Package.swift" ]; then
121+
cat > Package.swift <<EOF
97122
// swift-tools-version: 5.9
98123
import PackageDescription
99124
@@ -105,9 +130,20 @@ let package = Package(
105130
targets: []
106131
)
107132
EOF
133+
created_package=1
134+
fi
135+
136+
# finally-block: remove only our own synthetic manifest, then release the lock.
137+
cleanup() {
138+
if [ "${created_package:-0}" -eq 1 ]; then
139+
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
140+
fi
141+
if [ "${have_lock:-0}" -eq 1 ]; then
142+
rm -rf -- "$lock_dir"
143+
fi
108144
}
145+
trap cleanup EXIT
109146

110-
setup
111147
if [[ -z "$EXTRA_ARGS" ]]; then
112148
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
113149
fi

0 commit comments

Comments
 (0)