fix(sync): bounded retry on force-with-lease rejection in commit_and_push_translations_branch (#70)

timon0305 · web-flow · commit 01fa10de6581 · 2026-07-31T02:05:46.000-04:00
* fix(sync): bounded retry on force-with-lease rejection (#63) commit_and_push_translations_branch pushed translation branches with a single git push --force-with-lease and failed fast when the lease was rejected by a concurrent advance, failing the whole daily run on a transient race. Retry the push up to 3 times, running git fetch for the branch between attempts so the local ref reflects the advanced remote before re-attempting, and surface the existing rejection error unchanged once the attempts are exhausted (fail closed). The bats fixture gains an optional injection cap so a test can simulate a transient rejection; the fail-fast test is replaced by two covering retry-then- succeed and fail-after-exhaustion. * fix(sync): keep the force-with-lease retry safe with --force-if-includes The bounded retry fetched then re-pushed with --force-with-lease, which after the fetch could overwrite a concurrent commit that was never integrated. Add --force-if-includes (Git 2.30+) so the retry still rejects an unintegrated remote advance: a spurious rejection retries to success, a real concurrent commit fails closed instead of being silently overwritten. Guard force-if-includes support with a clear "requires Git 2.30+" error, and cover it plus the not-overwritten behavior with bats tests. * fix(sync): fail fast when the inter-attempt fetch fails The retry loop discarded git fetch failures, so an auth/network fetch failure left the tracking ref stale and was misreported as a concurrent-advance rejection. Surface the real cause and return the fetch exit code instead. Cover it with a bats test via an opt-in fetch-failure flag on the push hook. * fix(sync): pipefail-safe capability probes + route sync-local through the helper Address review feedback on the force-with-lease retry: - The force-with-lease/force-if-includes probes piped `git push -h` (exit 129) into grep, which under `set -o pipefail` (the workflows enable it) reported "not supported" even when the flag was present. Compute the probe from a captured `git push -h 2>&1 || true` variable via a shared git_push_help_mentions helper (no pipe). Test shims exit 129 like real git, the unsupported probes run under pipefail, and a positive test asserts both probes pass under pipefail. - The sync-local job now calls commit_and_push_translations_branch instead of a bare `git push --force-with-lease`, so it gets the same retry + force-if-includes. - Reword the injection-arg doc, capture stderr in the not-overwritten test, and dedup the two capability shims into install_git_without_flag.
diff --git a/.github/workflows/assets/lib.sh b/.github/workflows/assets/lib.sh
@@ -203,13 +203,28 @@ update_translations_submodule() {
   fi
 }
 
+# `git push -h` exits 129, so capture the help text first (|| true) and grep it
+# separately. Piping straight into grep would fail the probe under `set -o
+# pipefail` (which the workflows enable) even when the flag is present.
+git_push_help_mentions() {
+  local help
+  help="$(git push -h 2>&1 || true)"
+  grep -qF "$1" <<<"$help"
+}
+
 git_push_supports_force_with_lease() {
-  git push -h 2>&1 | grep -qF 'force-with-lease'
+  git_push_help_mentions 'force-with-lease'
+}
+
+# --force-if-includes (Git 2.30+) makes the safe retry below reject an unmerged
+# remote advance instead of overwriting it.
+git_push_supports_force_if_includes() {
+  git_push_help_mentions 'force-if-includes'
 }
 
 commit_and_push_translations_branch() {
   local dir="$1" branch="$2" libs_ref="$3" force="${4:-false}"
-  local push_rc remote_sha
+  local push_rc remote_sha fetch_rc attempt max_attempts=3
   git -C "$dir" status --short
   if git -C "$dir" diff --cached --quiet; then
     echo "  No staged submodule changes on $branch; skipping commit." >&2
@@ -221,14 +236,35 @@ commit_and_push_translations_branch() {
       phase_err "git push --force-with-lease is not supported by this Git installation"
       return 1
     fi
-    if git -C "$dir" push --force-with-lease origin "$branch"; then
-      :
-    else
-      push_rc=$?
-      remote_sha=$(git -C "$dir" ls-remote --heads origin "$branch" | awk '{print $1}')
-      phase_err "force-with-lease push rejected for branch $branch (remote HEAD=${remote_sha:-unknown}); remote may have advanced concurrently — re-run after fetch or resolve manually."
-      return "$push_rc"
+    if ! git_push_supports_force_if_includes; then
+      phase_err "git push --force-if-includes is not supported; requires Git 2.30+"
+      return 1
     fi
+    # A concurrent advance rejects the lease. Fetch so the lease reflects the
+    # advanced remote and retry, but keep --force-if-includes so the retry still
+    # rejects an advance we never integrated. A spurious rejection (remote
+    # unchanged from our base) retries to success; a real concurrent commit
+    # fails closed rather than being silently overwritten.
+    for (( attempt=1; attempt<=max_attempts; attempt++ )); do
+      if git -C "$dir" push --force-with-lease --force-if-includes origin "$branch"; then
+        return 0
+      else
+        push_rc=$?
+      fi
+      if (( attempt < max_attempts )); then
+        # A failed fetch (auth/network) leaves the tracking ref stale, so a
+        # retry can't satisfy the lease. Fail with the real cause instead of
+        # letting the loop misreport it as a concurrent-advance rejection.
+        git -C "$dir" fetch origin "$branch" || {
+          fetch_rc=$?
+          phase_err "failed to fetch origin/$branch before retrying the push"
+          return "$fetch_rc"
+        }
+      fi
+    done
+    remote_sha=$(git -C "$dir" ls-remote --heads origin "$branch" | awk '{print $1}')
+    phase_err "force-with-lease push rejected for branch $branch (remote HEAD=${remote_sha:-unknown}); remote may have advanced concurrently — re-run after fetch or resolve manually."
+    return "$push_rc"
   else
     git -C "$dir" push origin "$branch"
   fi
diff --git a/.github/workflows/sync-translation.yml b/.github/workflows/sync-translation.yml
@@ -138,8 +138,9 @@ jobs:
                      2>/dev/null | sed 's/^[^ ]* //')
 
           if [[ $updated -eq 1 ]] && ! git diff --staged --quiet; then
-            git commit -m "Update libs submodules ($branch)"
-            git push --force-with-lease origin "HEAD:$branch"
+            # Commit + push through the shared helper so this path gets the same
+            # bounded retry and --force-if-includes safety as the finalize flow.
+            commit_and_push_translations_branch "$GITHUB_WORKSPACE" "$branch" "$branch" true
             echo "  Pushed to origin/$branch" >&2
           else
             echo "  No pointer changes for $branch." >&2
diff --git a/tests/helpers/git_fixtures.bash b/tests/helpers/git_fixtures.bash
@@ -86,27 +86,44 @@ push_commit_to_remote_branch() {
   rm -rf "$tmp"
 }
 
-# Prepend a git wrapper to PATH that advances remote branch before push --force-with-lease.
+# A git wrapper that injects a concurrent advance before each force-with-lease
+# push, so the lease is rejected. The optional 4th arg caps how many pushes get a
+# fresh advance (0 = every push, the default); pass 1 to inject a single real
+# concurrent advance that the retry still refuses to overwrite. Pass 1 as the
+# 5th arg to also fail every branch-specific `git fetch origin <branch>` (the
+# inter-attempt retry fetch).
 install_git_push_pre_hook() {
-  local bare="$1" branch="$2" message="${3:-concurrent advance}"
+  local bare="$1" branch="$2" message="${3:-concurrent advance}" max_injections="${4:-0}" fail_branch_fetch="${5:-0}"
   local wrapper_dir="$GIT_FIXTURE_ROOT/bin"
   REAL_GIT="$(command -v git)"
   export REAL_GIT
   export GIT_HOOK_BARE_REMOTE="$bare" GIT_HOOK_BRANCH="$branch" GIT_HOOK_MESSAGE="$message"
+  export GIT_HOOK_MAX_INJECTIONS="$max_injections"
+  export GIT_HOOK_INJECT_COUNTER="$GIT_FIXTURE_ROOT/inject-count"
+  export GIT_HOOK_FAIL_BRANCH_FETCH="$fail_branch_fetch"
+  : >"$GIT_HOOK_INJECT_COUNTER"
   mkdir -p "$wrapper_dir"
   cat >"$wrapper_dir/git" <<'EOF'
 #!/usr/bin/env bash
+if [[ "$GIT_HOOK_FAIL_BRANCH_FETCH" == "1" && "${1:-}" == "-C" && "${3:-}" == "fetch" && -n "${5:-}" ]]; then
+  echo "simulated fetch failure" >&2
+  exit 1
+fi
 if [[ "${1:-}" == "-C" && "${3:-}" == "push" && "$*" == *"--force-with-lease"* ]]; then
-  tmp="$(mktemp -d)"
-  "$REAL_GIT" clone "$GIT_HOOK_BARE_REMOTE" "$tmp"
-  "$REAL_GIT" -C "$tmp" config user.email "test@test.local"
-  "$REAL_GIT" -C "$tmp" config user.name "Test"
-  "$REAL_GIT" -C "$tmp" checkout "$GIT_HOOK_BRANCH"
-  echo "$GIT_HOOK_MESSAGE" >>"$tmp/README"
-  "$REAL_GIT" -C "$tmp" add README
-  "$REAL_GIT" -C "$tmp" commit -m "$GIT_HOOK_MESSAGE"
-  "$REAL_GIT" -C "$tmp" push origin "$GIT_HOOK_BRANCH"
-  rm -rf "$tmp"
+  injected="$(wc -l <"$GIT_HOOK_INJECT_COUNTER" 2>/dev/null || echo 0)"
+  if [[ "$GIT_HOOK_MAX_INJECTIONS" -eq 0 || "$injected" -lt "$GIT_HOOK_MAX_INJECTIONS" ]]; then
+    tmp="$(mktemp -d)"
+    "$REAL_GIT" clone "$GIT_HOOK_BARE_REMOTE" "$tmp"
+    "$REAL_GIT" -C "$tmp" config user.email "test@test.local"
+    "$REAL_GIT" -C "$tmp" config user.name "Test"
+    "$REAL_GIT" -C "$tmp" checkout "$GIT_HOOK_BRANCH"
+    echo "$GIT_HOOK_MESSAGE" >>"$tmp/README"
+    "$REAL_GIT" -C "$tmp" add README
+    "$REAL_GIT" -C "$tmp" commit -m "$GIT_HOOK_MESSAGE"
+    "$REAL_GIT" -C "$tmp" push origin "$GIT_HOOK_BRANCH"
+    rm -rf "$tmp"
+    echo 1 >>"$GIT_HOOK_INJECT_COUNTER"
+  fi
 fi
 exec "$REAL_GIT" "$@"
 EOF
@@ -122,20 +139,22 @@ restore_git_push_pre_hook() {
     rm -rf "$GIT_WRAPPER_DIR"
     unset GIT_WRAPPER_DIR REAL_GIT
     unset GIT_HOOK_BARE_REMOTE GIT_HOOK_BRANCH GIT_HOOK_MESSAGE GIT_FETCH_COUNTER_FILE
+    unset GIT_HOOK_MAX_INJECTIONS GIT_HOOK_INJECT_COUNTER GIT_HOOK_FAIL_BRANCH_FETCH GIT_HIDE_PUSH_FLAG
   fi
 }
 
-# Prepend a git wrapper that hides --force-with-lease from push -h (simulates pre-2.8 git).
-install_git_without_force_with_lease() {
+# Prepend a git wrapper that hides one flag from `git push -h` output (which
+# exits 129) to simulate a git that predates that flag.
+install_git_without_flag() {
   local wrapper_dir="$GIT_FIXTURE_ROOT/bin"
   REAL_GIT="$(command -v git)"
-  export REAL_GIT
+  export REAL_GIT GIT_HIDE_PUSH_FLAG="$1"
   mkdir -p "$wrapper_dir"
   cat >"$wrapper_dir/git" <<'EOF'
 #!/usr/bin/env bash
 if [[ "${1:-}" == "push" && "${2:-}" == "-h" ]]; then
-  "$REAL_GIT" push -h 2>&1 | grep -v 'force-with-lease'
-  exit 0
+  "$REAL_GIT" push -h 2>&1 | grep -vF "$GIT_HIDE_PUSH_FLAG"
+  exit 129  # match real `git push -h`
 fi
 exec "$REAL_GIT" "$@"
 EOF
@@ -144,6 +163,11 @@ EOF
   export PATH="$wrapper_dir:$PATH"
 }
 
+# simulates pre-2.8 git (no force-with-lease)
+install_git_without_force_with_lease() { install_git_without_flag 'force-with-lease'; }
+# simulates a git from 2.8 to 2.29 (force-with-lease present, force-if-includes not)
+install_git_without_force_if_includes() { install_git_without_flag 'force-if-includes'; }
+
 # Prepend a git wrapper that counts fetch invocations and optionally injects concurrent push.
 install_git_fetch_counter() {
   local counter_file="$1"
diff --git a/tests/test_lib.bats b/tests/test_lib.bats
@@ -345,7 +345,7 @@ teardown() {
   git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance"
 }
 
-@test "finalize_translations_local: fail-fast on lease rejection (no retry)" {
+@test "finalize_translations_local: does not overwrite a concurrent advance (force-if-includes)" {
   # shellcheck source=tests/helpers/git_fixtures.bash
   source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash"
   init_git_fixture_root
@@ -358,22 +358,84 @@ teardown() {
   UPDATES=("algorithm")
   update_translations_submodule() { :; }
 
+  # One concurrent advance: the fetch updates the lease baseline, but
+  # --force-if-includes still rejects the retry because that advance was never
+  # integrated, so the run fails closed instead of overwriting it.
+  install_git_push_pre_hook "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance" 1
+
+  local stderr_file="$BATS_TMPDIR/finalize-no-overwrite-stderr"
+  set +e
+  finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file"
+  rc=$?
+  set -e
+
+  [ "$rc" -ne 0 ]
+  grep -q "force-with-lease push rejected" "$stderr_file"
+  # the concurrent advance is still the remote tip, not overwritten by our push
+  remote_sha=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}')
+  git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance"
+}
+
+@test "finalize_translations_local: fails after force-with-lease retries are exhausted" {
+  # shellcheck source=tests/helpers/git_fixtures.bash
+  source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash"
+  init_git_fixture_root
+  create_bare_remote_with_clone "translations"
+  create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH"
+  trans_dir="$GIT_FIXTURE_ROOT/translations-work"
+  git clone "$BARE_REMOTE" "$trans_dir"
+  set_git_bot_config "$trans_dir"
+
+  UPDATES=("algorithm")
+  update_translations_submodule() { :; }
+
+  # Advance the remote before every push, so all three attempts are rejected.
+  # The counter also records fetches: 1 finalize pre-fetch + 2 between retries.
   local fetch_counter="$BATS_TMPDIR/fetch-count"
   : >"$fetch_counter"
   install_git_fetch_counter "$fetch_counter" "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance"
 
-  remote_sha_before=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}')
+  local stderr_file="$BATS_TMPDIR/finalize-exhausted-stderr"
+  set +e
+  finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file"
+  rc=$?
+  set -e
+
+  [ "$rc" -ne 0 ]
+  [ "$(wc -l <"$fetch_counter")" -eq 3 ]
+  grep -q "force-with-lease push rejected" "$stderr_file"
+  remote_sha=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}')
+  grep -q "$remote_sha" "$stderr_file"
+  git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance"
+}
 
+@test "finalize_translations_local: fails with the real cause when an inter-attempt fetch fails" {
+  # shellcheck source=tests/helpers/git_fixtures.bash
+  source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash"
+  init_git_fixture_root
+  create_bare_remote_with_clone "translations"
+  create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH"
+  trans_dir="$GIT_FIXTURE_ROOT/translations-work"
+  git clone "$BARE_REMOTE" "$trans_dir"
+  set_git_bot_config "$trans_dir"
+
+  UPDATES=("algorithm")
+  update_translations_submodule() { :; }
+
+  # First push is rejected (one concurrent advance), then the inter-attempt
+  # `git fetch origin <branch>` fails.
+  install_git_push_pre_hook "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance" 1 1
+
+  local stderr_file="$BATS_TMPDIR/finalize-fetch-fail-stderr"
   set +e
-  finalize_translations_local "$trans_dir" "develop" "en" 2>/dev/null
+  finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file"
   rc=$?
   set -e
 
   [ "$rc" -ne 0 ]
-  [ "$(wc -l <"$fetch_counter")" -eq 1 ]
-  remote_sha_after=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}')
-  [ "$remote_sha_before" != "$remote_sha_after" ]
-  git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha_after" | grep -q "concurrent advance"
+  grep -q "failed to fetch origin/${LOCAL_BRANCH_PREFIX}en" "$stderr_file"
+  # not misreported as a concurrent-advance rejection
+  ! grep -q "force-with-lease push rejected" "$stderr_file"
 }
 
 @test "commit_and_push_translations_branch: clear error when force-with-lease unsupported" {
@@ -390,17 +452,55 @@ teardown() {
   install_git_without_force_with_lease
 
   local stderr_file="$BATS_TMPDIR/push-unsupported-stderr"
-  set +e
+  # pipefail is on in the workflows; the probe must still work when `git push -h`
+  # exits 129 (the shim mirrors that).
+  set +e -o pipefail
   commit_and_push_translations_branch "$trans_dir" "${LOCAL_BRANCH_PREFIX}en" "develop" true \
     2>"$stderr_file"
   rc=$?
-  set -e
+  set -e +o pipefail
 
   [ "$rc" -ne 0 ]
   grep -q "force-with-lease is not supported" "$stderr_file"
   grep -q "not supported by this Git installation" "$stderr_file"
 }
 
+@test "commit_and_push_translations_branch: clear error when force-if-includes unsupported" {
+  # shellcheck source=tests/helpers/git_fixtures.bash
+  source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash"
+  init_git_fixture_root
+  create_bare_remote_with_clone "translations"
+  create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH"
+  trans_dir="$GIT_FIXTURE_ROOT/translations-work"
+  git clone "$BARE_REMOTE" "$trans_dir"
+  set_git_bot_config "$trans_dir"
+  git -C "$trans_dir" checkout "${LOCAL_BRANCH_PREFIX}en"
+
+  # force-with-lease present, force-if-includes hidden (pre-2.30 Git).
+  install_git_without_force_if_includes
+
+  local stderr_file="$BATS_TMPDIR/push-no-includes-stderr"
+  set +e -o pipefail
+  commit_and_push_translations_branch "$trans_dir" "${LOCAL_BRANCH_PREFIX}en" "develop" true \
+    2>"$stderr_file"
+  rc=$?
+  set -e +o pipefail
+
+  [ "$rc" -ne 0 ]
+  grep -q "force-if-includes is not supported" "$stderr_file"
+  grep -q "requires Git 2.30+" "$stderr_file"
+}
+
+@test "git_push_supports_* probes are pipefail-safe with real git" {
+  # Modern git supports both flags, but `git push -h` exits 129; piped into grep
+  # that fails under pipefail even when the flag is present. The probes must
+  # still report supported.
+  set -o pipefail
+  git_push_supports_force_with_lease
+  git_push_supports_force_if_includes
+  set +o pipefail
+}
+
 @test "begin_phase and end_phase: emit group markers and track CURRENT_PHASE" {
   local out_file="$BATS_TMPDIR/phase.out"
 
