Add retry logic with jitter for sub_issue_write to handle parallel calls

pkg/github/issues.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"math/rand/v2"
 	"net/http"
 	"strings"
 	"time"
@@ -800,37 +801,95 @@ 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 {
+			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) {

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)