fix(sync): bounded retry on force-with-lease rejection in commit_and_push_translations_branch - #70
Merged
wpak-ai merged 4 commits intoJul 31, 2026
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe translation branch push now requires ChangesTranslation push safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant commit_and_push_translations_branch
participant GitRemote
participant ConcurrentWriter
commit_and_push_translations_branch->>commit_and_push_translations_branch: detect force-if-includes support
commit_and_push_translations_branch->>GitRemote: protected force push
ConcurrentWriter->>GitRemote: advance branch
GitRemote-->>commit_and_push_translations_branch: lease rejection
commit_and_push_translations_branch->>GitRemote: fetch origin/branch
commit_and_push_translations_branch->>GitRemote: retry protected force push
GitRemote-->>commit_and_push_translations_branch: success or final rejection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
whisper67265
requested changes
Jul 30, 2026
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/assets/lib.sh:
- Line 217: Update the retry loop using local variables push_rc, remote_sha,
attempt, and max_attempts so each git fetch failure is detected and handled
immediately instead of discarded. Do not continue attempting pushes without a
refreshed tracking ref; preserve the fetch failure status/details for the final
diagnostic rather than reporting it as a concurrent-advance rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49f5b6a4-1118-4a7f-8c01-dc35607b3f65
📒 Files selected for processing (3)
.github/workflows/assets/lib.shtests/helpers/git_fixtures.bashtests/test_lib.bats
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/helpers/git_fixtures.bash
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.
whisper67265
approved these changes
Jul 30, 2026
wpak-ai
requested changes
Jul 30, 2026
wpak-ai
left a comment
wpak-ai
left a comment
There was a problem hiding this comment.
-
tests/helpers/git_fixtures.bash:171-176,tests/test_lib.bats:476- make theinstall_git_without_force_if_includesshim exit 129 like realgit push -h, and run the capability-probe tests underset -o pipefail -
.github/workflows/sync-translation.yml:142,.github/workflows/assets/lib.sh:215- have thesync-localjob callcommit_and_push_translations_branch, which it already has in scope via thelib.shsource at line 112, instead of open-coding a baregit push --force-with-lease origin "HEAD:$branch"
Contributor
Author
Both fixed. Capability probes: git push -h exits 129, so under pipefail the piped grep reported "not supported" even when the flag was present. Now they capture the help text first and grep it separately; made the shims exit 129 to match real git, ran the probe tests under pipefail, and added a positive probe test. sync-local now goes through commit_and_push_translations_branch instead of a bare git push --force-with-lease, so it gets the same bounded retry and --force-if-includes safety. |
… 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.
wpak-ai
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #63
Problem
commit_and_push_translations_branch(.github/workflows/assets/lib.sh) pushed translation branches with a singlegit push --force-with-lease. When the lease was rejected because the remote advanced concurrently, it failed fast with the existingphase_err, so a transient concurrent advance during the daily scheduled run failed the whole run even though fetching and re-attempting would succeed.What changed
force-with-lease, retry up to 3 attempts, runninggit fetchfor the branch between attempts so the local ref reflects the advanced remote before re-attempting.install_git_push_pre_hookgained an optional injection cap to simulate a transient rejection.Full bats suite passes locally (132).
Note: because each retry fetches before re-pushing, the retry refreshes the lease and overwrites the concurrent advance. That is intended here since these
local-<lang>branches are bot-regenerated each run.Summary by CodeRabbit