Skip to content

enforce version bump in preview#547

Open
aivong-openhands wants to merge 3 commits into
mainfrom
openhands/issue-545-document-race-condition
Open

enforce version bump in preview#547
aivong-openhands wants to merge 3 commits into
mainfrom
openhands/issue-545-document-race-condition

Conversation

@aivong-openhands
Copy link
Copy Markdown
Contributor

@aivong-openhands aivong-openhands commented Apr 13, 2026 •
edited
Loading

Description

This PR addresses the question raised in issue #545 about how enforcement would have prevented the race condition. After investigation, I've documented the true root cause and the complete solution.

Root Cause Discovery

The comment on issue #545 correctly pointed out that PR #531 did bump the openhands chart version and passed its validation check. The investigation revealed:

  1. PR Add automation env vars to automation and openhands charts #531 bumped openhands from 0.4.0 → 0.4.1 (validation passed against base 0.4.0)
  2. PR Experimental laminar support #473 (Laminar support) also bumped openhands from 0.4.0 → 0.4.1
  3. PR Experimental laminar support #473 merged first, making main at 0.4.1
  4. PR Add automation env vars to automation and openhands charts #531 then merged, with both HEAD and HEAD~1 at 0.4.1
  5. Post-merge validation failed because no version bump was detected

Why Enforcement Alone Is Insufficient

Simply changing enforce_version_bump: false → true would NOT have prevented this issue because:

Complete Solution

To fully prevent this race condition, both of the following are needed:

  1. ✅ This PR: Enable enforce_version_bump: true in preview-helm-charts.yml
  2. 🔧 Admin action required: Enable "Require branches to be up to date before merging" in branch protection settings

When both are enabled:

  1. PR Add automation env vars to automation and openhands charts #531 would be blocked from merging after PR Experimental laminar support #473 merged (branch out of date)
  2. After rebasing, PR validation would compare against the updated main (0.4.1)
  3. Validation would fail, requiring author to bump to 0.4.2

Helm Chart Checklist

  • I have updated the version field in Chart.yaml for each modified chart
  • I have tested the chart upgrade path from the previous version (N/A - only workflow and docs changes)
  • I have verified backwards compatibility with existing values.yaml configurations (N/A)
  • I have updated the chart's README.md if there are any breaking changes or new required values (N/A - added separate documentation)

Additional Notes

Changes in This PR

  • Change enforce_version_bump from false to true in preview-helm-charts.yml

Admin Action Required

A repository administrator needs to enable:

  • Settings → Branches → main → Require status checks to pass before merging
  • Settings → Branches → main → Require branches to be up to date before merging

Fixes #545

This PR was created by an AI assistant (OpenHands) on behalf of @aivong-openhands.

@openhands-agent
Document race condition in Helm chart publishing and add preventive m…
c3d5186 
…easures

## 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 <[email protected]>
@aivong-openhands aivong-openhands mentioned this pull request Apr 13, 2026
Closed
@aivong-openhands aivong-openhands marked this pull request as ready for review April 13, 2026 23:09
@aivong-openhands
Copy link
Copy Markdown
Contributor Author

aivong-openhands commented Apr 13, 2026

@OpenHands remove the doc update. I simply wanted documentation on the github issue which is already done

@openhands-ai
Copy link
Copy Markdown

openhands-ai Bot commented Apr 13, 2026

I'm on it! aivong-openhands can track my progress at all-hands.dev

@openhands-agent
Remove documentation file per reviewer feedback
a24e3bc 
Documentation on the race condition is already present in GitHub issue #545.

Co-authored-by: openhands <[email protected]>
@aivong-openhands aivong-openhands changed the title Document race condition in Helm chart publishing and add preventive measures enforce version bump in preview Apr 13, 2026
all-hands-bot
all-hands-bot approved these changes Apr 13, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Good taste - Elegant, pragmatic solution to a real problem.

Analysis:

This solves the documented race condition from PR #531 using the simplest possible approach: a boolean flag + existing GitHub branch protection. No new code complexity, no over-engineering, just leveraging infrastructure that already exists.

The documentation is thorough without being bloated. It explains the root cause with verifiable commit SHAs, shows why enforcement alone is insufficient, and provides clear recovery procedures. The workflow comment correctly points out the limitation and references the docs.

Key Insight:
Requires admin coordination to enable "require up-to-date branches" setting, but this is clearly documented and inherent to the problem space. The PR correctly identifies that validation timing is the root issue, not the validation logic itself.

✅ Worth merging: Solves real problem with minimal change and clear documentation.

aivong-openhands
aivong-openhands commented Apr 13, 2026
View reviewed changes
Comment thread .github/workflows/preview-helm-charts.yml
@openhands-ai
Copy link
Copy Markdown

openhands-ai Bot commented Apr 13, 2026

Summary

I've addressed the reviewer's comment requesting the removal of the documentation file from PR #547.

Changes Made

  • Removed docs/helm-chart-publishing-race-condition.md from the PR
  • Committed and pushed the change to update the PR

Request Fulfillment Checklist

Current PR State

The PR now contains only:

  1. .github/workflows/preview-helm-charts.yml - enables enforce_version_bump: true
  2. Chart version updates (from previous commits in main)

The documentation about the race condition is already present in GitHub issue #545, so a separate docs file was unnecessary.

PR link: #547

@aivong-openhands
Copy link
Copy Markdown
Contributor Author

aivong-openhands commented Apr 15, 2026

is a duplicate of #547

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@openhands-ai openhands-ai[bot] openhands-ai[bot] left review comments

@all-hands-bot all-hands-bot all-hands-bot approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Document race condition in Helm chart publishing workflow and implement preventive measures

3 participants

@aivong-openhands @all-hands-bot @openhands-agent