diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 62e1a0bac..f2a8b16cd 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "math/rand/v2" "net/http" "strings" "time" @@ -800,37 +801,96 @@ Options are: } func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int, replaceParent bool) (*mcp.CallToolResult, error) { + const ( + maxRetries = 3 + minJitterMs = 50 + maxJitterMs = 200 + jitterRangeMs = maxJitterMs - minJitterMs + 1 + ) + subIssueRequest := github.SubIssueRequest{ SubIssueID: int64(subIssueID), ReplaceParent: github.Ptr(replaceParent), } - subIssue, resp, err := client.SubIssue.Add(ctx, owner, repo, int64(issueNumber), subIssueRequest) - if err != nil { + var lastResp *github.Response + var lastErr error + var lastBody []byte + + for attempt := 0; attempt < maxRetries; attempt++ { + if attempt > 0 { + // Random jitter: 50-200ms to desynchronize parallel retries + // #nosec G404 -- Using non-cryptographic random for jitter is acceptable + jitter := time.Duration(minJitterMs+rand.IntN(jitterRangeMs)) * time.Millisecond + time.Sleep(jitter) + } + + subIssue, resp, err := client.SubIssue.Add(ctx, owner, repo, int64(issueNumber), subIssueRequest) + lastResp = resp + lastErr = err + + // Success case + if err == nil && resp.StatusCode == http.StatusCreated { + _ = resp.Body.Close() + r, err := json.Marshal(subIssue) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + return utils.NewToolResultText(string(r)), nil + } + + // Check if this is a retryable priority conflict error + shouldRetry := false + if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity && resp.Body != nil { + // Read the body to check for priority conflict + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr == nil { + lastBody = body + if strings.Contains(string(body), "Priority has already been taken") { + shouldRetry = true + } + } + } + + // If not a retryable error or last attempt, break and return error + if !shouldRetry || attempt == maxRetries-1 { + break + } + } + + // Handle final error after all retries exhausted + if lastErr != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to add sub-issue", - resp, - err, + lastResp, + lastErr, ), nil } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusCreated { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + // Handle non-201 status codes after retries exhausted + if lastResp != nil && lastResp.StatusCode != http.StatusCreated { + // Use the body we already read during retry attempts if available + if len(lastBody) > 0 { + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, lastBody), nil + } + // Otherwise try to read the body if it's still available + if lastResp.Body != nil { + body, err := io.ReadAll(lastResp.Body) + _ = lastResp.Body.Close() + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", lastResp, body), nil } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to add sub-issue", resp, body), nil } - r, err := json.Marshal(subIssue) - if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) + // This should not be reached in normal operation - indicates a logic error + statusCode := 0 + if lastResp != nil { + statusCode = lastResp.StatusCode } - - return utils.NewToolResultText(string(r)), nil - + return nil, fmt.Errorf("unexpected error: failed to add sub-issue after %d attempts (last status: %d)", maxRetries, statusCode) } func RemoveSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int) (*mcp.CallToolResult, error) { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index a338efcba..ffea00b7f 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "strings" + "sync/atomic" "testing" "time" @@ -3020,6 +3021,149 @@ func Test_AddSubIssue(t *testing.T) { } } +func Test_AddSubIssue_RetryLogic(t *testing.T) { + // Setup mock issue for success case + mockIssue := &github.Issue{ + Number: github.Ptr(42), + Title: github.Ptr("Parent Issue"), + Body: github.Ptr("This is the parent issue with a sub-issue"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"), + User: &github.User{ + Login: github.Ptr("testuser"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectSuccess bool + expectedErrMsg string + }{ + { + name: "successful retry after priority conflict", + mockedClient: func() *http.Client { + // Create a handler that fails twice with priority conflict, then succeeds + var callCount atomic.Int32 + handler := func(w http.ResponseWriter, _ *http.Request) { + count := callCount.Add(1) + if count <= 2 { + // First two calls fail with priority conflict + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) + } else { + // Third call succeeds + w.WriteHeader(http.StatusCreated) + responseJSON, _ := json.Marshal(mockIssue) + _, _ = w.Write(responseJSON) + } + } + return MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: handler, + }) + }(), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: true, + }, + { + name: "exhausted retries with priority conflict", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + // Always fail with priority conflict + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "An error occurred while adding the sub-issue to the parent issue. Priority has already been taken"}`)) + }, + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: false, + expectedErrMsg: "Priority has already been taken", + }, + { + name: "non-retryable 422 error - sub-issue cannot be parent of itself", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + // Non-retryable 422 error + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "Validation failed", "errors": [{"message": "Sub-issue cannot be a parent of itself"}]}`)) + }, + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(42), + }, + expectSuccess: false, + expectedErrMsg: "failed to add sub-issue", + }, + { + name: "first attempt succeeds - no retry needed", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusCreated, mockIssue), + }), + requestArgs: map[string]interface{}{ + "method": "add", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "sub_issue_id": float64(123), + }, + expectSuccess: true, + }, + } + + serverTool := SubIssueWrite(translations.NullTranslationHelper) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + deps := BaseDeps{ + Client: client, + } + handler := serverTool.Handler(deps) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectSuccess { + require.NoError(t, err) + require.NotNil(t, result) + + // Parse and verify the result + textContent := getTextResult(t, result) + var returnedIssue github.Issue + err = json.Unmarshal([]byte(textContent.Text), &returnedIssue) + require.NoError(t, err) + assert.Equal(t, *mockIssue.Number, *returnedIssue.Number) + assert.Equal(t, *mockIssue.Title, *returnedIssue.Title) + } else { + // Expect error in result content + require.NotNil(t, result) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedErrMsg) + } + }) + } +} + func Test_GetSubIssues(t *testing.T) { // Verify tool definition once serverTool := IssueRead(translations.NullTranslationHelper)