Skip to content

Commit 9031d15

Browse files
Crash0v3rrid3claude
Crash0v3rrid3
and
claude
authored
fix(spm): serialize concurrent scans to protect synthetic Package.swift (DEVA11Y-483) (#33)
* fix(spm): serialize concurrent scans to protect synthetic Package.swift (DEVA11Y-483) Guard the setup/scan/cleanup cycle with an atomic per-directory mkdir lock so concurrent spm.sh invocations in the same working directory no longer race: the first instance to exit can no longer delete the shared synthetic Package.swift out from under a still-running peer. Stale locks left by killed peers are reclaimed after 5 minutes; mkdir is used instead of flock(1) for macOS portability. Applied to the bash, zsh and fish variants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * 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> * fix(review): regenerate spm.sh checksum sidecars after lock redesign (DEVA11Y-483) The concurrency-lock redesign edited all three spm.sh launchers but left their .sha256 sidecars stale, which would make the self-update integrity check abort on every run (dead on arrival, same class as the DEVA11Y-475 fix). Regenerate all three sidecars to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b274cd0 commit 9031d15

6 files changed

Lines changed: 240 additions & 51 deletions

File tree

‎scripts/bash/spm.sh‎

Lines changed: 79 additions & 16 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,22 +36,76 @@ EOF
3836
fi
3937
}
4038

41-
a11y_scan() {
42-
# Ensure Package.swift is removed on exit (acts like a finally block)
43-
cleanup() {
44-
if [ $PACKAGE_EXISTS -eq 0 ]; then
45-
return
46-
fi
47-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
48-
}
49-
trap cleanup EXIT
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+
}
5047

51-
setup() {
52-
if [ $PACKAGE_EXISTS -eq 0 ]; then
53-
return
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"
5471
fi
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+
}
87+
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
5595

56-
cat > Package.swift <<EOF
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
103+
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
57109
// swift-tools-version: 5.9
58110
import PackageDescription
59111
@@ -65,9 +117,20 @@ let package = Package(
65117
targets: []
66118
)
67119
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
68131
}
132+
trap cleanup EXIT
69133

70-
setup
71134
if [[ -z "$EXTRA_ARGS" ]]; then
72135
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
73136
fi

‎scripts/bash/spm.sh.sha256‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2d627fbe06991da445f7fbcd8bc8c4f5c4dd126c04372f74dd0f4ab39222c57e spm.sh
1+
1987749e047ca43b15f10b0eb6a98d0adcfbf5f7de9f54f49831b486406c8ba1 spm.sh

‎scripts/fish/spm.sh‎

Lines changed: 79 additions & 16 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,22 +49,76 @@ EOF
5149
fi
5250
}
5351

54-
a11y_scan() {
55-
# Ensure Package.swift is removed on exit (acts like a finally block)
56-
cleanup() {
57-
if [ $PACKAGE_EXISTS -eq 0 ]; then
58-
return
59-
fi
60-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
61-
}
62-
trap cleanup EXIT
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+
}
6360

64-
setup() {
65-
if [ $PACKAGE_EXISTS -eq 0 ]; then
66-
return
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"
6784
fi
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+
}
100+
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
68108

69-
cat > Package.swift <<EOF
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
116+
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
70122
// swift-tools-version: 5.9
71123
import PackageDescription
72124
@@ -78,9 +130,20 @@ let package = Package(
78130
targets: []
79131
)
80132
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
81144
}
145+
trap cleanup EXIT
82146

83-
setup
84147
if [[ -z "$EXTRA_ARGS" ]]; then
85148
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
86149
fi

‎scripts/fish/spm.sh.sha256‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
8fe00a7635d3ad66d7197994631c1a58581d4b3a845453f72e5e237338dea24e spm.sh
1+
87bf749b365b86157934671d913b3e4bcdb2b54e43b765d0850218d442ca4e26 spm.sh

‎scripts/zsh/spm.sh‎

Lines changed: 79 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@ export BROWSERSTACK_USERNAME=$($zsh_bin -lic 'echo $BROWSERSTACK_USERNAME' | tai
1212
export BROWSERSTACK_ACCESS_KEY=$($zsh_bin -lic 'echo $BROWSERSTACK_ACCESS_KEY' | tail -n 1)
1313

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

53-
a11y_scan() {
54-
# Ensure Package.swift is removed on exit (acts like a finally block)
55-
cleanup() {
56-
if [ $PACKAGE_EXISTS -eq 0 ]; then
57-
return
58-
fi
59-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
60-
}
61-
trap cleanup EXIT
51+
# Resolve a lock directory OUTSIDE the scanned package, keyed by the package path,
52+
# so a crash can never leave a lock inside the user's working tree (nor let a
53+
# pre-commit `git add -A` stage it). cksum is POSIX and always present.
54+
_spm_lock_dir() {
55+
local key
56+
key=$(printf '%s' "$PWD" | cksum | cut -d' ' -f1)
57+
printf '%s/browserstack-a11y-spm-%s.lock' "${TMPDIR:-/tmp}" "$key"
58+
}
6259

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

68-
cat > Package.swift <<EOF
108+
_spm_acquire_lock "$lock_dir"
109+
case "$?" in
110+
0) have_lock=1 ;;
111+
2) echo "A11y scan: another scan is already running in ${PWD}; skipping." >&2
112+
return 0 ;;
113+
*) echo "A11y scan: proceeding without a lock; concurrent scans in ${PWD} may conflict." >&2 ;;
114+
esac
115+
116+
# Decide ownership UNDER the lock, from the live filesystem -- not a snapshot
117+
# taken at script start. Only create the synthetic manifest if none exists now,
118+
# and on exit delete only what this instance created.
119+
if [ ! -f "${PWD}/Package.swift" ]; then
120+
cat > Package.swift <<EOF
69121
// swift-tools-version: 5.9
70122
import PackageDescription
71123
@@ -77,9 +129,20 @@ let package = Package(
77129
targets: []
78130
)
79131
EOF
132+
created_package=1
133+
fi
134+
135+
# finally-block: remove only our own synthetic manifest, then release the lock.
136+
cleanup() {
137+
if [ "${created_package:-0}" -eq 1 ]; then
138+
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
139+
fi
140+
if [ "${have_lock:-0}" -eq 1 ]; then
141+
rm -rf -- "$lock_dir"
142+
fi
80143
}
144+
trap cleanup EXIT
81145

82-
setup
83146
if [[ -z "$EXTRA_ARGS" ]]; then
84147
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
85148
fi

‎scripts/zsh/spm.sh.sha256‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
507bb44d5a17a4e15f59fcd3cdc04692a39a65abc2e4bf5375340939ce484a98 spm.sh
1+
c798dc6be53c1dfdfa5113697cfe1a23ef48ccf33206e9484feb671d84bad45c spm.sh

0 commit comments

Comments
 (0)