{{ message }}
Repository navigation
Keep a failed backport from corrupting the PR state - #81
Draft
aiolibsbot wants to merge 1 commit into
Draft
aiolibsbot wants to merge 1 commit into
aiolibsbot wants to merge 1 commit into
Conversation
Three related gaps in the failure paths of the reporting layer, all of which leave the user's repo or check run in a state that never resolves. * `PullRequestReporter.start_reporting()` unlocks a deliberately locked PR and `finish_reporting()` is what puts the lock back. The lock restoration ran *after* the terminal comment update, so a failing comment update left the PR permanently unlocked. It is now in a `finally`, since restoring the maintainer's lock is the one side effect that has to survive a failing report. * `CommentsAPI.update_comment()` sent `None` as the comment URI when `create_comment()` had never run or had failed, so reporting a terminal failure blew up on its own. It is a no-op now, which makes `finish_reporting()` safe to call unconditionally. * `on_merge_of_labeled_pr()` iterated the target branches with no isolation. A PR labeled `backport-3.0` and `backport-2.9` lost the 2.9 backport entirely -- no attempt, no report -- when 3.0 hit a transient API error. Every branch is attempted now and the first error is re-raised afterwards so it still reaches the logs and error tracking. `Exception` rather than `BaseException` is deliberate: task cancellation must keep aborting the loop at once. The suite stubs `anyio`/`gidgethub`/`octomachinery` so the handler stays testable without pulling third-party packages into CI.
This branch has not been deployed
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
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.
What
Three failure paths in the reporting layer leave the user's repo or check run in a state that never resolves. This fixes all three.
Why
Each one is silent — nothing in the PR or the check run tells anyone it happened:
start_reporting()unlocks the PR;finish_reporting()re-locks it after updating the tracking comment. If that comment update fails, the maintainer's deliberate lock is gone for good.update_comment()patchedNoneas the URI whencreate_comment()never ran or errored.backport-3.0andbackport-2.9lost the 2.9 backport entirely — no attempt, no report — when 3.0 hit a transient API error.How
finally— it's the one side effect that has to survive a failing report.update_comment()is a no-op without a tracking comment, which makesfinish_reporting()safe to call unconditionally.Exceptionrather thanBaseExceptionis deliberate: task cancellation must keep aborting the loop at once.Testing
9 tests, stdlib + pytest only. 6 of them fail on
masterand pass here — every fix is load-bearing.tests/test_target_branch_isolation.pystubsanyio/gidgethub/octomachineryinsys.modulesso the webhook handler stays testable without pulling third-party packages into CI. The decorators become identity functions, which is what these tests want — they exercise the handler body, not the router.pytest.iniand.github/workflows/tests.ymlare byte-identical to the copies on #77/#72/#73, so git merges them add/add cleanly regardless of order.Not in scope
An exception escaping
process_pr_backport_labels()still leaves the check run stuck atin_progressforever. The fix is a catch-all around that function's body, which means reindenting ~130 lines — that would conflict with every other open PR touching it. Worth doing once this stack drains.Quality Report
Changes: 7 files changed, 399 insertions(+), 19 deletions(-)
Code scan: clean
Tests: failed (command not found)
Branch hygiene: clean
Generated by Kōan