Skip to content

Add TestCheckEventRaceCondition #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 29, 2025
Merged

Add TestCheckEventRaceCondition #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
324 changes: 324 additions & 0 deletions pkg/webhook/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,3 +594,327 @@ func TestExtractPRURLVariations(t *testing.T) {
})
}
}

// TestCheckEventRaceCondition tests the GitHub webhook timing issue where check events
// can arrive before the pull_requests array is populated.
//
// Background:
// GitHub's webhook system can send check_run/check_suite events immediately when a check
// completes, but their internal indexing may not have updated the pull_requests array yet.
// This creates a race condition where the event arrives without PR information.
//
// Expected behavior:
// - If pull_requests array is empty but we have repository info, use repo URL as fallback
// - Include commit SHA so clients can look up the PR later
// - Org-based subscriptions still work with repo URL
// - Only drop event if we can't extract ANY repository information
func TestCheckEventRaceCondition(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

h := srv.NewHub()
go h.Run(ctx)
defer h.Stop()

secret := "testsecret"
handler := NewHandler(h, secret, nil)

tests := []struct {
name string
eventType string
payload map[string]any
expectStatusCode int
expectBroadcast bool
expectRepoURL bool // true if we expect repo URL fallback
expectCommitSHA bool
}{
{
name: "check_run with empty pull_requests array - GitHub race condition",
eventType: "check_run",
payload: map[string]any{
"action": "completed",
"check_run": map[string]any{
"head_sha": "abc123def456789",
"pull_requests": []any{}, // Empty - race condition!
"status": "completed",
"conclusion": "success",
},
"repository": map[string]any{
"html_url": "https://github.com/testorg/testrepo",
"owner": map[string]any{
"login": "testorg",
},
"name": "testrepo",
},
},
expectStatusCode: http.StatusOK,
expectBroadcast: true, // Should still broadcast with repo URL
expectRepoURL: true,
expectCommitSHA: true,
},
{
name: "check_suite with missing pull_requests field - GitHub race condition",
eventType: "check_suite",
payload: map[string]any{
"action": "completed",
"check_suite": map[string]any{
"head_sha": "def456abc123789",
// No pull_requests field at all
"status": "completed",
"conclusion": "success",
},
"repository": map[string]any{
"html_url": "https://github.com/myorg/myrepo",
"owner": map[string]any{
"login": "myorg",
},
"name": "myrepo",
},
},
expectStatusCode: http.StatusOK,
expectBroadcast: true,
expectRepoURL: true,
expectCommitSHA: true,
},
{
name: "check_run with null pull_requests - another variant",
eventType: "check_run",
payload: map[string]any{
"action": "completed",
"check_run": map[string]any{
"head_sha": "nullcase123456",
"pull_requests": nil, // Explicitly null
},
"repository": map[string]any{
"html_url": "https://github.com/nullorg/nullrepo",
"owner": map[string]any{
"login": "nullorg",
},
},
},
expectStatusCode: http.StatusOK,
expectBroadcast: true,
expectRepoURL: true,
expectCommitSHA: true,
},
{
name: "check_run with no repository - must drop event",
eventType: "check_run",
payload: map[string]any{
"action": "completed",
"check_run": map[string]any{
"head_sha": "norepository123",
"pull_requests": []any{},
},
// Missing repository field entirely
},
expectStatusCode: http.StatusOK, // Still 200 to GitHub
expectBroadcast: false, // Event dropped
expectRepoURL: false,
expectCommitSHA: false,
},
{
name: "check_suite with repository but no html_url - must drop",
eventType: "check_suite",
payload: map[string]any{
"action": "completed",
"check_suite": map[string]any{
"head_sha": "nohtmlurl456",
"pull_requests": []any{},
},
"repository": map[string]any{
"name": "repo",
// Missing html_url
},
},
expectStatusCode: http.StatusOK,
expectBroadcast: false,
expectRepoURL: false,
expectCommitSHA: false,
},
{
name: "check_run with populated pull_requests - normal case (no race)",
eventType: "check_run",
payload: map[string]any{
"action": "completed",
"check_run": map[string]any{
"head_sha": "normalcase123",
"pull_requests": []any{
map[string]any{
"number": float64(42),
},
},
},
"repository": map[string]any{
"html_url": "https://github.com/normalorg/normalrepo",
},
},
expectStatusCode: http.StatusOK,
expectBroadcast: true,
expectRepoURL: false, // Should have PR URL, not repo URL
expectCommitSHA: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
body, err := json.Marshal(tt.payload)
if err != nil {
t.Fatalf("Failed to marshal payload: %v", err)
}

req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(body))
req.Header.Set("X-GitHub-Event", tt.eventType) //nolint:canonicalheader // GitHub webhook header
req.Header.Set("X-GitHub-Delivery", "test-delivery-"+tt.name)

// Add valid signature
mac := hmac.New(sha256.New, []byte(secret))
mac.Write(body)
signature := "sha256=" + hex.EncodeToString(mac.Sum(nil))
req.Header.Set("X-Hub-Signature-256", signature)

w := httptest.NewRecorder()
handler.ServeHTTP(w, req)

// Check response status
if w.Code != tt.expectStatusCode {
t.Errorf("expected status %d, got %d", tt.expectStatusCode, w.Code)
}

// Verify URL extraction behavior
// Note: ExtractPRURL returns "" when there's no PR URL in check events.
// The handler in ServeHTTP then falls back to repo URL (lines 163-222 in handler.go)
ctx := context.Background()
extractedURL := ExtractPRURL(ctx, tt.eventType, tt.payload)

if tt.expectRepoURL {
// ExtractPRURL should return empty (no PR URL)
if extractedURL != "" {
t.Errorf("Expected ExtractPRURL to return empty (triggering repo fallback), got %q", extractedURL)
}
// Verify we have repo URL available for fallback
repoURL, ok := tt.payload["repository"].(map[string]any)["html_url"].(string)
if !ok {
t.Fatal("Test setup error: repository.html_url missing")
}
// Repo URL should NOT contain /pull/
if contains := bytes.Contains([]byte(repoURL), []byte("/pull/")); contains {
t.Errorf("Repo URL should not contain /pull/, got %q", repoURL)
}
} else if tt.expectBroadcast {
// Should have PR URL (normal case)
if extractedURL == "" {
t.Error("Expected PR URL but got empty string")
}
if contains := bytes.Contains([]byte(extractedURL), []byte("/pull/")); !contains {
t.Errorf("Expected PR URL with /pull/, got %q", extractedURL)
}
} else {
// Event should be dropped, no URL
if extractedURL != "" {
t.Errorf("Expected empty URL for dropped event, got %q", extractedURL)
}
}

// Verify commit SHA extraction
extractedSHA := extractCommitSHA(tt.eventType, tt.payload)
if tt.expectCommitSHA {
if extractedSHA == "" {
t.Error("Expected commit SHA but got empty string")
}
// Verify it matches the SHA in payload
var expectedSHA string
if tt.eventType == "check_run" {
if checkRun, ok := tt.payload["check_run"].(map[string]any); ok {
expectedSHA, _ = checkRun["head_sha"].(string)
}
} else if tt.eventType == "check_suite" {
if checkSuite, ok := tt.payload["check_suite"].(map[string]any); ok {
expectedSHA, _ = checkSuite["head_sha"].(string)
}
}
if expectedSHA != "" && extractedSHA != expectedSHA {
t.Errorf("Expected SHA %q, got %q", expectedSHA, extractedSHA)
}
}
})
}
}

// TestCheckEventRaceConditionEndToEnd tests the complete flow including hub broadcast.
// This verifies that events with repo URL fallback can still be received by org-based subscribers.
func TestCheckEventRaceConditionEndToEnd(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

h := srv.NewHub()
go h.Run(ctx)
defer h.Stop()

secret := "testsecret"
handler := NewHandler(h, secret, nil)

// Simulate check_run event with empty pull_requests (GitHub race condition)
payload := map[string]any{
"action": "completed",
"check_run": map[string]any{
"head_sha": "racecommit123",
"pull_requests": []any{}, // Empty due to timing
"status": "completed",
},
"repository": map[string]any{
"html_url": "https://github.com/raceorg/racerepo",
"owner": map[string]any{
"login": "raceorg", // This is what org subscribers will match on
},
"name": "racerepo",
},
}

body, err := json.Marshal(payload)
if err != nil {
t.Fatalf("Failed to marshal payload: %v", err)
}

req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(body))
req.Header.Set("X-GitHub-Event", "check_run")
req.Header.Set("X-GitHub-Delivery", "race-test-delivery-123")

mac := hmac.New(sha256.New, []byte(secret))
mac.Write(body)
signature := "sha256=" + hex.EncodeToString(mac.Sum(nil))
req.Header.Set("X-Hub-Signature-256", signature)

w := httptest.NewRecorder()
handler.ServeHTTP(w, req)

if w.Code != http.StatusOK {
t.Errorf("Expected status %d, got %d", http.StatusOK, w.Code)
}

// Verify that ExtractPRURL returns empty (no PR URL in check event with empty pull_requests)
// The handler then falls back to repo URL when broadcasting
extractedURL := ExtractPRURL(ctx, "check_run", payload)
if extractedURL != "" {
t.Errorf("Expected ExtractPRURL to return empty string (no PR URL), got %q", extractedURL)
}

// Verify we have repo URL available for the handler's fallback logic
expectedRepoURL := "https://github.com/raceorg/racerepo"
repoURL, ok := payload["repository"].(map[string]any)["html_url"].(string)
if !ok || repoURL != expectedRepoURL {
t.Errorf("Repository URL should be %q, got %q", expectedRepoURL, repoURL)
}

// Verify commit SHA is extracted for client-side lookup
extractedSHA := extractCommitSHA("check_run", payload)
if extractedSHA != "racecommit123" {
t.Errorf("Expected SHA 'racecommit123', got %q", extractedSHA)
}

// Verify the repo URL is NOT a PR URL (no /pull/ in path)
// This confirms the handler will use repo URL as fallback, not a PR URL
if contains := bytes.Contains([]byte(repoURL), []byte("/pull/")); contains {
t.Errorf("Repo URL should not contain /pull/, got %q", repoURL)
}
}