From c3d51868ecae1a6e5ae41b41035bc3e03a13f062 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 13 Apr 2026 22:47:51 +0000 Subject: [PATCH 1/3] Document race condition in Helm chart publishing and add preventive measures ## Problem A race condition exists when two PRs modify the same chart concurrently: 1. Both PRs pass version validation against their original base 2. First PR merges, bumping chart to version X 3. Second PR merges, also targeting version X 4. Post-merge validation fails because HEAD and HEAD~1 both have version X This was observed in PR #531, where both #531 and #473 bumped openhands to 0.4.1, causing the post-merge publish workflow to fail. ## Solution 1. Enable 'enforce_version_bump: true' on PR checks (this PR) 2. Enable 'Require branches to be up to date before merging' in GitHub branch protection (requires admin action) The combination ensures PRs always validate against the latest main, preventing the race condition. ## Changes - Add comprehensive documentation in docs/helm-chart-publishing-race-condition.md - Change enforce_version_bump from false to true in preview-helm-charts.yml - Add explanatory comments about the race condition and recommended settings Closes #545 Co-authored-by: openhands --- .github/workflows/preview-helm-charts.yml | 8 +- docs/helm-chart-publishing-race-condition.md | 127 +++++++++++++++++++ 2 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 docs/helm-chart-publishing-race-condition.md diff --git a/.github/workflows/preview-helm-charts.yml b/.github/workflows/preview-helm-charts.yml index e28e9ee0..0100bc36 100644 --- a/.github/workflows/preview-helm-charts.yml +++ b/.github/workflows/preview-helm-charts.yml @@ -9,7 +9,13 @@ jobs: uses: ./.github/workflows/validate-chart-versions.yml with: base_ref: origin/${{ github.event.pull_request.base.ref }} - enforce_version_bump: false + # IMPORTANT: Set to true to enforce version bumps on PRs. + # However, enforcement alone does NOT prevent race conditions when two PRs + # modify the same chart concurrently. To fully prevent race conditions: + # 1. Enable "Require branches to be up to date before merging" in branch protection + # 2. This ensures PRs always validate against the latest main before merge + # See: docs/helm-chart-publishing-race-condition.md + enforce_version_bump: true detect-changes: runs-on: ubuntu-latest diff --git a/docs/helm-chart-publishing-race-condition.md b/docs/helm-chart-publishing-race-condition.md new file mode 100644 index 00000000..08c52b10 --- /dev/null +++ b/docs/helm-chart-publishing-race-condition.md @@ -0,0 +1,127 @@ +# Helm Chart Publishing Race Condition + +## Summary + +A race condition exists in the Helm chart publishing workflow that can leave the `main` branch in a broken state when multiple PRs modify the same chart concurrently. + +## Root Cause Analysis + +The race condition occurs when two PRs that modify the same chart are merged in quick succession: + +### Timeline Example (PR #531 Incident) + +1. **Initial State**: `main` branch has `openhands` chart at version `0.4.0` + +2. **PR #531 Opens**: + - Based on commit `1833cd3dc8474d7aa203ba410ce29bafbad1a237` (openhands: `0.4.0`) + - Bumps `openhands` from `0.4.0` → `0.4.1` + - PR validation passes ✅ (compares against `origin/main` which has `0.4.0`) + +3. **PR #473 Merges First**: + - Also bumped `openhands` from `0.4.0` → `0.4.1` + - Now `main` has `openhands` at `0.4.1` + +4. **PR #531 Merges**: + - Still contains `openhands: 0.4.1` + - GitHub merge succeeds (no conflict on version field since both target `0.4.1`) + - Merge commit `0de14f6dc4807f9310626a9beeba0d7e0610a0d0` + +5. **Post-merge Validation Fails**: + - `publish-helm-charts.yml` compares `HEAD` vs `HEAD~1` + - `HEAD` (merge commit): `openhands: 0.4.1` + - `HEAD~1` (previous main): `openhands: 0.4.1` + - **No version bump detected** → validation fails + - `automation:0.1.2` is not published (sequential publishing stops) + +### The Gap in Validation + +| Stage | Comparison | PR #531 Result | +|-------|------------|----------------| +| PR Check | `HEAD` vs `origin/main` (at PR open time) | `0.4.1` vs `0.4.0` ✅ | +| Post-merge | `HEAD` vs `HEAD~1` (commit before merge) | `0.4.1` vs `0.4.1` ❌ | + +The PR validation passes because it compares against the base branch **at the time the PR was created**, not the **current** state of main at merge time. + +## Solutions + +### ✅ Recommended: Require Branch to Be Up-to-Date Before Merging + +Enable GitHub Branch Protection setting: **"Require branches to be up to date before merging"** + +**How it works:** +1. PR #531 would be blocked from merging after PR #473 merged +2. Author must update/rebase PR #531 against latest `main` +3. After rebase, PR validation runs again against updated `origin/main` (now `0.4.1`) +4. Validation would fail because `0.4.1` → `0.4.1` is not a version bump +5. Author must bump to `0.4.2` to pass validation + +**Configuration (Repository Settings → Branches → main):** +- ✅ Require status checks to pass before merging +- ✅ Require branches to be up to date before merging +- ✅ Require `validate-chart-versions / validate-chart-versions` as status check + +**Pros:** +- Prevents the race condition entirely +- Uses existing GitHub infrastructure +- Forces PRs to always validate against latest main + +**Cons:** +- Adds friction (must update branch before merge) +- May require more frequent rebasing + +### Alternative: Use Merge Queue + +GitHub Merge Queue serializes PRs, automatically rebasing each before merge. + +**Pros:** +- Fully automated +- Handles rebasing automatically + +**Cons:** +- Requires GitHub Enterprise or Teams plan +- More complex setup + +### Complementary: Enforce Version Bump on PRs + +Change `preview-helm-charts.yml` to set `enforce_version_bump: true`: + +```yaml +validate-chart-versions: + uses: ./.github/workflows/validate-chart-versions.yml + with: + base_ref: origin/${{ github.event.pull_request.base.ref }} + enforce_version_bump: true # Changed from false +``` + +**Important:** This alone does NOT prevent the race condition. It only converts warnings to errors. Combined with "require up-to-date branches", it ensures: +1. PR must be rebased before merge +2. After rebase, version validation enforces a bump against current main + +## Recovery from Broken State + +If a race condition has already occurred and publishing failed: + +### Option A: Bump Chart Version in New PR + +```bash +# Bump the version of any chart that failed to publish +yq -i '.version = "X.Y.Z"' charts//Chart.yaml +``` + +### Option B: Re-run Publish Workflow + +If the chart version is already higher than what's in GHCR: +1. Go to Actions → Publish Helm Charts +2. Click "Run workflow" → Select `main` branch → Run + +## Related Issues and PRs + +- Issue #545: This documentation +- PR #531: Original PR that triggered the race condition +- PR #473: The PR that merged first, also bumping to `0.4.1` +- PR #543: Downstream PR affected by unpublished `automation:0.1.2` + +## References + +- [GitHub: Require branches to be up to date](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging) +- [GitHub: Merge Queue](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue) From a24e3bcc3a2996d5325460a31ff1b4334971e5fd Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 13 Apr 2026 23:11:24 +0000 Subject: [PATCH 2/3] Remove documentation file per reviewer feedback Documentation on the race condition is already present in GitHub issue #545. Co-authored-by: openhands --- docs/helm-chart-publishing-race-condition.md | 127 ------------------- 1 file changed, 127 deletions(-) delete mode 100644 docs/helm-chart-publishing-race-condition.md diff --git a/docs/helm-chart-publishing-race-condition.md b/docs/helm-chart-publishing-race-condition.md deleted file mode 100644 index 08c52b10..00000000 --- a/docs/helm-chart-publishing-race-condition.md +++ /dev/null @@ -1,127 +0,0 @@ -# Helm Chart Publishing Race Condition - -## Summary - -A race condition exists in the Helm chart publishing workflow that can leave the `main` branch in a broken state when multiple PRs modify the same chart concurrently. - -## Root Cause Analysis - -The race condition occurs when two PRs that modify the same chart are merged in quick succession: - -### Timeline Example (PR #531 Incident) - -1. **Initial State**: `main` branch has `openhands` chart at version `0.4.0` - -2. **PR #531 Opens**: - - Based on commit `1833cd3dc8474d7aa203ba410ce29bafbad1a237` (openhands: `0.4.0`) - - Bumps `openhands` from `0.4.0` → `0.4.1` - - PR validation passes ✅ (compares against `origin/main` which has `0.4.0`) - -3. **PR #473 Merges First**: - - Also bumped `openhands` from `0.4.0` → `0.4.1` - - Now `main` has `openhands` at `0.4.1` - -4. **PR #531 Merges**: - - Still contains `openhands: 0.4.1` - - GitHub merge succeeds (no conflict on version field since both target `0.4.1`) - - Merge commit `0de14f6dc4807f9310626a9beeba0d7e0610a0d0` - -5. **Post-merge Validation Fails**: - - `publish-helm-charts.yml` compares `HEAD` vs `HEAD~1` - - `HEAD` (merge commit): `openhands: 0.4.1` - - `HEAD~1` (previous main): `openhands: 0.4.1` - - **No version bump detected** → validation fails - - `automation:0.1.2` is not published (sequential publishing stops) - -### The Gap in Validation - -| Stage | Comparison | PR #531 Result | -|-------|------------|----------------| -| PR Check | `HEAD` vs `origin/main` (at PR open time) | `0.4.1` vs `0.4.0` ✅ | -| Post-merge | `HEAD` vs `HEAD~1` (commit before merge) | `0.4.1` vs `0.4.1` ❌ | - -The PR validation passes because it compares against the base branch **at the time the PR was created**, not the **current** state of main at merge time. - -## Solutions - -### ✅ Recommended: Require Branch to Be Up-to-Date Before Merging - -Enable GitHub Branch Protection setting: **"Require branches to be up to date before merging"** - -**How it works:** -1. PR #531 would be blocked from merging after PR #473 merged -2. Author must update/rebase PR #531 against latest `main` -3. After rebase, PR validation runs again against updated `origin/main` (now `0.4.1`) -4. Validation would fail because `0.4.1` → `0.4.1` is not a version bump -5. Author must bump to `0.4.2` to pass validation - -**Configuration (Repository Settings → Branches → main):** -- ✅ Require status checks to pass before merging -- ✅ Require branches to be up to date before merging -- ✅ Require `validate-chart-versions / validate-chart-versions` as status check - -**Pros:** -- Prevents the race condition entirely -- Uses existing GitHub infrastructure -- Forces PRs to always validate against latest main - -**Cons:** -- Adds friction (must update branch before merge) -- May require more frequent rebasing - -### Alternative: Use Merge Queue - -GitHub Merge Queue serializes PRs, automatically rebasing each before merge. - -**Pros:** -- Fully automated -- Handles rebasing automatically - -**Cons:** -- Requires GitHub Enterprise or Teams plan -- More complex setup - -### Complementary: Enforce Version Bump on PRs - -Change `preview-helm-charts.yml` to set `enforce_version_bump: true`: - -```yaml -validate-chart-versions: - uses: ./.github/workflows/validate-chart-versions.yml - with: - base_ref: origin/${{ github.event.pull_request.base.ref }} - enforce_version_bump: true # Changed from false -``` - -**Important:** This alone does NOT prevent the race condition. It only converts warnings to errors. Combined with "require up-to-date branches", it ensures: -1. PR must be rebased before merge -2. After rebase, version validation enforces a bump against current main - -## Recovery from Broken State - -If a race condition has already occurred and publishing failed: - -### Option A: Bump Chart Version in New PR - -```bash -# Bump the version of any chart that failed to publish -yq -i '.version = "X.Y.Z"' charts//Chart.yaml -``` - -### Option B: Re-run Publish Workflow - -If the chart version is already higher than what's in GHCR: -1. Go to Actions → Publish Helm Charts -2. Click "Run workflow" → Select `main` branch → Run - -## Related Issues and PRs - -- Issue #545: This documentation -- PR #531: Original PR that triggered the race condition -- PR #473: The PR that merged first, also bumping to `0.4.1` -- PR #543: Downstream PR affected by unpublished `automation:0.1.2` - -## References - -- [GitHub: Require branches to be up to date](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging) -- [GitHub: Merge Queue](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue) From 324ccc74f5aacc092e41f47783ddd6e7f2e51cc1 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 13 Apr 2026 23:12:50 +0000 Subject: [PATCH 3/3] Remove explanatory comments, keep only enforce_version_bump setting Co-authored-by: openhands --- .github/workflows/preview-helm-charts.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/preview-helm-charts.yml b/.github/workflows/preview-helm-charts.yml index 0100bc36..9f273f14 100644 --- a/.github/workflows/preview-helm-charts.yml +++ b/.github/workflows/preview-helm-charts.yml @@ -9,12 +9,6 @@ jobs: uses: ./.github/workflows/validate-chart-versions.yml with: base_ref: origin/${{ github.event.pull_request.base.ref }} - # IMPORTANT: Set to true to enforce version bumps on PRs. - # However, enforcement alone does NOT prevent race conditions when two PRs - # modify the same chart concurrently. To fully prevent race conditions: - # 1. Enable "Require branches to be up to date before merging" in branch protection - # 2. This ensures PRs always validate against the latest main before merge - # See: docs/helm-chart-publishing-race-condition.md enforce_version_bump: true detect-changes: