This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch fork-ci-6-reconciler-followups in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 01cb999b2e315d4a503caa3482b10cdce689732b Author: Jarek Potiuk <[email protected]> AuthorDate: Sat Oct 17 02:57:45 2026 +0200 Retry the fork CI push-again nudge when posting it fails The state comment is what the next reconciler run compares with. Recording awaiting_push there before the nudge was posted meant a failed post was never retried, and the contributor got no prompt before the inactivity closure. Generated-by: Claude Opus 5 --- dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md | 3 +++ dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py | 9 +++++---- dev/breeze/tests/test_fork_ci_reconcile.py | 12 ++++++++++++ 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md index 3b14a552d1a..540000aab23 100644 --- a/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md +++ b/dev/breeze/doc/ci/08_fork_ci_for_external_contributors.md @@ -444,6 +444,9 @@ Rules that sit under the table: closing comment or close failed, and the next run finishes the closure. The same holds for a `gate_bypassed` marker, where only a reopen by someone other than the author counts, so the closure is finished even after the offending run has left the tampering check's window. +- **New comments go first.** A nudge, a closing explanation or a tampering notice is posted + before the state comment records the new state, so when posting fails the next run sees the + old state and posts it again; a repeat is preferred to a lost notice. - **`fork ci required` goes last.** When a run removes the label (on release, or for an ungated base branch), it does so after every other change, so a failure on the way leaves the label in place and the next run retries the rest. diff --git a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py index db21caaef39..22ec0e15b2b 100644 --- a/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py +++ b/dev/breeze/src/airflow_breeze/utils/fork_ci_reconcile.py @@ -471,8 +471,6 @@ def classify( plan.state = state since = marker.since if marker and marker.state == state.value else now - # The marker records awaiting_push before the nudge is posted, so a failed post is not retried; - # tracked at https://github.com/apache/airflow/issues/74908 if marker and marker.state == ForkState.SETUP_REQUIRED.value and state == ForkState.AWAITING_PUSH: plan.new_comments.append(nudge_comment(fork=pr.head_repo)) @@ -1012,6 +1010,11 @@ def apply_plan(client: GitHubClient, pr: PullRequestSnapshot, plan: Plan) -> Non for label in plan.remove_labels: if label != LABEL_FORK_CI_REQUIRED: client.remove_label(pr.number, label) + # New comments go before the state comment records the new state: the state recorded there is + # what the next run compares with, so a failure on the way posts them again rather than never. + # They carry no marker, so the state comment stays the earliest bot comment with one. + for comment in plan.new_comments: + client.create_comment(pr.number, comment) if plan.marker_comment is not None: if pr.marker_comment_id is not None: client.update_comment(pr.number, pr.marker_comment_id, plan.marker_comment) @@ -1019,8 +1022,6 @@ def apply_plan(client: GitHubClient, pr: PullRequestSnapshot, plan: Plan) -> Non client.create_comment(pr.number, plan.marker_comment) if plan.status is not None: client.set_status(pr.number, pr.head_sha, *plan.status) - for comment in plan.new_comments: - client.create_comment(pr.number, comment) if plan.draft is False: client.mark_ready(pr.number, pr.node_id) if plan.close: diff --git a/dev/breeze/tests/test_fork_ci_reconcile.py b/dev/breeze/tests/test_fork_ci_reconcile.py index 06f77699a2f..e7e929917a9 100644 --- a/dev/breeze/tests/test_fork_ci_reconcile.py +++ b/dev/breeze/tests/test_fork_ci_reconcile.py @@ -1189,3 +1189,15 @@ def test_slack_payload_stays_within_slack_limits(): assert "…" in texts[1] assert texts[-1].startswith("_") assert "more sections are in the <https://run|run log>" in texts[-1] + + +def test_nudge_is_posted_before_the_state_comment_records_awaiting_push(client): + pr = _pr(marker=_marker("setup_required"), marker_comment_id=5) + plan, _, _ = _classify(pr, ForkObservation(workflow_state="active")) + client.create_comment.side_effect = GitHubError("gh api failed: HTTP 500") + + with pytest.raises(GitHubError): + apply_plan(client, pr, plan) + + assert "Push this branch again" in client.create_comment.call_args.args[1] + client.update_comment.assert_not_called()
