Repository navigation
Conversation
omarchy-dev-add-migration used HEAD's committer date, so every branch cut from the same commit minted the same id and later migrations silently never ran. Use date +%s and bump on collision.
Automated AI review
Outcome: No defects found in the change itself; the one reproduced gap, concurrent runs in a single checkout, predates this PR and is covered in the optional notes. Related PR: #13656 makes the same change to the same lines and adds a test, so merging either one makes the other conflict. Verified
Optional notes
Reproducer: regression test (fails at base, passes at 3ffd965)Proposed regression test (verification, not a defect). Save as #!/bin/bash
set -euo pipefail
source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh"
test_tmp=$(mktemp -d)
trap 'rm -rf "$test_tmp"' EXIT
# A checkout whose HEAD was committed at 1789095456, the id #13516 saw reused.
checkout="$test_tmp/omarchy"
mkdir -p "$checkout/migrations"
git -C "$checkout" init -q
GIT_AUTHOR_NAME=test GIT_AUTHOR_EMAIL=test@example.invalid \
GIT_COMMITTER_NAME=test GIT_COMMITTER_EMAIL=test@example.invalid \
GIT_AUTHOR_DATE=@1789095456 GIT_COMMITTER_DATE=@1789095456 \
git -C "$checkout" commit --allow-empty -q -m base
# Pin the clock so the expected ids are exact.
stub_bin="$test_tmp/bin"
mkdir -p "$stub_bin"
cat >"$stub_bin/date" <<'SH'
#!/bin/bash
[[ ${1:-} == "+%s" ]] || exit 1
echo 1790700000
SH
chmod +x "$stub_bin/date"
add_migration() {
PATH="$stub_bin:$PATH" OMARCHY_PATH="$checkout" "$ROOT/bin/omarchy-dev-add-migration" --no-edit
}
created=$(add_migration)
[[ $created == "$checkout/migrations/1790700000.sh" && -f $created ]] ||
fail "a new migration is named after the current time, not HEAD's commit time" "created: $created"
pass "a new migration is named after the current time, not HEAD's commit time"
created=$(add_migration)
[[ $created == "$checkout/migrations/1790700001.sh" && -f $created ]] ||
fail "a second migration in the same second takes the next free id" "created: $created"
pass "a second migration in the same second takes the next free id"Output at the base e332dc9 (exit status 1): Output at 3ffd965 (exit status 0): Reproducer: concurrent runs (fails at 3ffd965)Save as #!/bin/bash
set -euo pipefail
source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/base-test.sh"
test_tmp=$(mktemp -d)
trap 'rm -rf "$test_tmp"' EXIT
# Pin the clock so both runs start from the same second.
stub_bin="$test_tmp/bin"
mkdir -p "$stub_bin"
cat >"$stub_bin/date" <<'SH'
#!/bin/bash
[[ ${1:-} == "+%s" ]] || exit 1
echo 1790700000
SH
chmod +x "$stub_bin/date"
for trial in $(seq 1 20); do
checkout="$test_tmp/omarchy-$trial"
mkdir -p "$checkout/migrations"
PATH="$stub_bin:$PATH" OMARCHY_PATH="$checkout" "$ROOT/bin/omarchy-dev-add-migration" --no-edit >"$test_tmp/$trial.a" &
first=$!
PATH="$stub_bin:$PATH" OMARCHY_PATH="$checkout" "$ROOT/bin/omarchy-dev-add-migration" --no-edit >"$test_tmp/$trial.b" &
second=$!
wait "$first" "$second"
[[ $(<"$test_tmp/$trial.a") != "$(<"$test_tmp/$trial.b")" ]] ||
fail "two concurrent runs in one checkout create distinct migrations" "trial $trial: both printed $(<"$test_tmp/$trial.a")"
done
pass "two concurrent runs in one checkout create distinct migrations"Output at 3ffd965 (exit status 1): Without the stubbed clock, 50 of 50 trials of two concurrent runs in one checkout printed the same path. With the loop and id=$(date +%s)
migration_file="migrations/${id}.sh"
# noclobber creates the file with O_EXCL, so concurrent runs cannot claim one id.
set -o noclobber
until { : >"$migration_file"; } 2>/dev/null; do
[[ -e $migration_file ]] || { echo "Cannot create $PWD/$migration_file" >&2; exit 1; }
id=$((id + 1))
migration_file="migrations/${id}.sh"
doneReview informationTest scope: Source review and sandboxed command runs with AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
Use noclobber O_EXCL create so concurrent helper runs in one tree cannot mint the same path. Cover wall-clock naming and the race.
|
Thanks for the thorough community review — agreed on the findings. Pushed a follow-up that:
Also noted the overlap with #13656 — fine to take either; this branch now includes the race-safe claim + tests if that’s useful. |
3 participants
Why
omarchy-dev-add-migrationnamed files aftergit log -1 --format=%cd --date=unix. Every branch cut from the same commit therefore received the same migration id. The runner records that id as done, so a later migration with the colliding name never runs.Reported in #13516 (observed collision on
1789095456from the quattro merge of #11263).What
Use wall-clock
date +%sinstead of HEAD's committer date. Claim the path withnoclobber/ O_EXCL create and bump the id until the create succeeds, so concurrent runs in one checkout cannot mint the same file.Does not rewrite existing migrations. Merge-time uniqueness against history is still a useful follow-up; this only stops the helper from minting the same id on sibling branches (and tightens same-tree races).
Test
Note: #13656 targets the same helper lines (with a test). Happy to coordinate / close if maintainers prefer that PR.
Refs #13516