Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Commit 75b506c

Browse files
fix(batch-changes): disallow retry on deleted changesets (#63336)
Closes SRCH-524 [Slack Context](https://sourcegraph.slack.com/archives/C05EA9KQUTA/p1718230808589239?thread_ts=1718230808.589239&cid=C05EA9KQUTA) Bitbucket Server (v5.1) added the ability to decline and [delete pull requests](https://confluence.atlassian.com/bitbucketserver/bitbucket-server-5-1-release-notes-902140366.html) - this isn't replicated across other supported codehosts such as: - GITHUB - GITLAB - BITBUCKET CLOUD e.t.c https://github.com/sourcegraph/sourcegraph/assets/25608335/e4e448ee-36a5-4b5a-8265-b47a9ae88628 Due to this, whenever a pull request is deleted, Batch Changes services (like Syncer & Reconciler) keep retrying whatever operation they're running, thereby spamming the codehost in the process. In this PR, I ensured whenever we encounter certain status code from a HTTP request to a codehost, it returns a NonRetryable error to reduce spamming. The logic for handling non-existent pull requests [lived here](https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/extsvc/bitbucketserver/client.go?L464-467), however that condition was never met because `httpError` did not fulfil the `Unwrap` interface therefore `errors.As` couldn't unwrap the underlying error. ## Test plan Create a test batch change targeting a bitbucket server codehost. Once the pull request has been created, delete the pull request. Once the changeset has been synced on Sourcegraph, the status of the changeset will be updated to`CLOSED`. ```yaml name: testbbdecline2 description: Add Hello World to READMEs # Find the first 100 search results for README files. # This could yield less than 100 repos/workspaces if some repos contain multiple READMEs. on: - repositoriesMatchingQuery: repo:^bitbucket\.sgdev\.org/BABU/(delve|compose)$ # In each repository, run this command. Each repository's resulting diff is captured. steps: - run: IFS=$'\n'; echo Hello World | tee -a $(find -name README.md) container: ubuntu:18.04 # Describe the changeset (e.g., GitHub pull request) you want for each repository. changesetTemplate: title: Hello World body: My first batch change! commit: message: Append Hello World to all README.md files # Optional: Push the commit to a branch named after this batch change by default. branch: ${{ batch_change.name }} ``` ## Changelog * Disallow auto-retry when a changeset is deleted.
1 parent cd4d788 commit 75b506c

File tree

3 files changed

+11
-3
lines changed

3 files changed

+11
-3
lines changed

internal/errcode/code.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,8 @@ type nonRetryableError struct{ error }
188188

189189
func (nonRetryableError) NonRetryable() bool { return true }
190190

191+
func (e nonRetryableError) Unwrap() error { return e.error }
192+
191193
func MaybeMakeNonRetryable(statusCode int, err error) error {
192194
if statusCode > 0 && statusCode < 200 ||
193195
statusCode >= 300 && statusCode < 500 ||

internal/extsvc/bitbucketserver/client.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,12 +1011,12 @@ func (c *Client) do(ctx context.Context, req *http.Request, result any) (_ *http
10111011
return resp, err
10121012
}
10131013

1014-
if resp.StatusCode < 200 || resp.StatusCode >= 400 {
1015-
return resp, errors.WithStack(&httpError{
1014+
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusBadRequest {
1015+
return resp, errors.WithStack(errcode.MaybeMakeNonRetryable(resp.StatusCode, &httpError{
10161016
URL: req.URL,
10171017
StatusCode: resp.StatusCode,
10181018
Body: bs,
1019-
})
1019+
}))
10201020
}
10211021

10221022
// handle binary response

internal/extsvc/bitbucketserver/client_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,12 @@ func TestClient_LoadPullRequest(t *testing.T) {
441441
t.Fatalf("error:\nhave: %q\nwant: %q", have, want)
442442
}
443443

444+
if tc.name == "non existing pr" {
445+
if err != ErrPullRequestNotFound {
446+
t.Fatal("expected error, got nil")
447+
}
448+
}
449+
444450
if err != nil || tc.err != "<nil>" {
445451
return
446452
}

0 commit comments

Comments
 (0)