Skip to content

Commit 57f2ea3

Browse files
tstrombergtstromberg
authored
Merge pull request #37 from tstromberg/main
Add TestCheckEventRaceCondition
2 parents e5a4f9a + 03baab4 commit 57f2ea3

File tree

1 file changed

+324
-0
lines changed

1 file changed

+324
-0
lines changed

‎pkg/webhook/handler_test.go‎

Lines changed: 324 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,3 +594,327 @@ func TestExtractPRURLVariations(t *testing.T) {
594594
})
595595
}
596596
}
597+
598+
// TestCheckEventRaceCondition tests the GitHub webhook timing issue where check events
599+
// can arrive before the pull_requests array is populated.
600+
//
601+
// Background:
602+
// GitHub's webhook system can send check_run/check_suite events immediately when a check
603+
// completes, but their internal indexing may not have updated the pull_requests array yet.
604+
// This creates a race condition where the event arrives without PR information.
605+
//
606+
// Expected behavior:
607+
// - If pull_requests array is empty but we have repository info, use repo URL as fallback
608+
// - Include commit SHA so clients can look up the PR later
609+
// - Org-based subscriptions still work with repo URL
610+
// - Only drop event if we can't extract ANY repository information
611+
func TestCheckEventRaceCondition(t *testing.T) {
612+
ctx, cancel := context.WithCancel(context.Background())
613+
defer cancel()
614+
615+
h := srv.NewHub()
616+
go h.Run(ctx)
617+
defer h.Stop()
618+
619+
secret := "testsecret"
620+
handler := NewHandler(h, secret, nil)
621+
622+
tests := []struct {
623+
name string
624+
eventType string
625+
payload map[string]any
626+
expectStatusCode int
627+
expectBroadcast bool
628+
expectRepoURL bool // true if we expect repo URL fallback
629+
expectCommitSHA bool
630+
}{
631+
{
632+
name: "check_run with empty pull_requests array - GitHub race condition",
633+
eventType: "check_run",
634+
payload: map[string]any{
635+
"action": "completed",
636+
"check_run": map[string]any{
637+
"head_sha": "abc123def456789",
638+
"pull_requests": []any{}, // Empty - race condition!
639+
"status": "completed",
640+
"conclusion": "success",
641+
},
642+
"repository": map[string]any{
643+
"html_url": "https://github.com/testorg/testrepo",
644+
"owner": map[string]any{
645+
"login": "testorg",
646+
},
647+
"name": "testrepo",
648+
},
649+
},
650+
expectStatusCode: http.StatusOK,
651+
expectBroadcast: true, // Should still broadcast with repo URL
652+
expectRepoURL: true,
653+
expectCommitSHA: true,
654+
},
655+
{
656+
name: "check_suite with missing pull_requests field - GitHub race condition",
657+
eventType: "check_suite",
658+
payload: map[string]any{
659+
"action": "completed",
660+
"check_suite": map[string]any{
661+
"head_sha": "def456abc123789",
662+
// No pull_requests field at all
663+
"status": "completed",
664+
"conclusion": "success",
665+
},
666+
"repository": map[string]any{
667+
"html_url": "https://github.com/myorg/myrepo",
668+
"owner": map[string]any{
669+
"login": "myorg",
670+
},
671+
"name": "myrepo",
672+
},
673+
},
674+
expectStatusCode: http.StatusOK,
675+
expectBroadcast: true,
676+
expectRepoURL: true,
677+
expectCommitSHA: true,
678+
},
679+
{
680+
name: "check_run with null pull_requests - another variant",
681+
eventType: "check_run",
682+
payload: map[string]any{
683+
"action": "completed",
684+
"check_run": map[string]any{
685+
"head_sha": "nullcase123456",
686+
"pull_requests": nil, // Explicitly null
687+
},
688+
"repository": map[string]any{
689+
"html_url": "https://github.com/nullorg/nullrepo",
690+
"owner": map[string]any{
691+
"login": "nullorg",
692+
},
693+
},
694+
},
695+
expectStatusCode: http.StatusOK,
696+
expectBroadcast: true,
697+
expectRepoURL: true,
698+
expectCommitSHA: true,
699+
},
700+
{
701+
name: "check_run with no repository - must drop event",
702+
eventType: "check_run",
703+
payload: map[string]any{
704+
"action": "completed",
705+
"check_run": map[string]any{
706+
"head_sha": "norepository123",
707+
"pull_requests": []any{},
708+
},
709+
// Missing repository field entirely
710+
},
711+
expectStatusCode: http.StatusOK, // Still 200 to GitHub
712+
expectBroadcast: false, // Event dropped
713+
expectRepoURL: false,
714+
expectCommitSHA: false,
715+
},
716+
{
717+
name: "check_suite with repository but no html_url - must drop",
718+
eventType: "check_suite",
719+
payload: map[string]any{
720+
"action": "completed",
721+
"check_suite": map[string]any{
722+
"head_sha": "nohtmlurl456",
723+
"pull_requests": []any{},
724+
},
725+
"repository": map[string]any{
726+
"name": "repo",
727+
// Missing html_url
728+
},
729+
},
730+
expectStatusCode: http.StatusOK,
731+
expectBroadcast: false,
732+
expectRepoURL: false,
733+
expectCommitSHA: false,
734+
},
735+
{
736+
name: "check_run with populated pull_requests - normal case (no race)",
737+
eventType: "check_run",
738+
payload: map[string]any{
739+
"action": "completed",
740+
"check_run": map[string]any{
741+
"head_sha": "normalcase123",
742+
"pull_requests": []any{
743+
map[string]any{
744+
"number": float64(42),
745+
},
746+
},
747+
},
748+
"repository": map[string]any{
749+
"html_url": "https://github.com/normalorg/normalrepo",
750+
},
751+
},
752+
expectStatusCode: http.StatusOK,
753+
expectBroadcast: true,
754+
expectRepoURL: false, // Should have PR URL, not repo URL
755+
expectCommitSHA: true,
756+
},
757+
}
758+
759+
for _, tt := range tests {
760+
t.Run(tt.name, func(t *testing.T) {
761+
body, err := json.Marshal(tt.payload)
762+
if err != nil {
763+
t.Fatalf("Failed to marshal payload: %v", err)
764+
}
765+
766+
req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(body))
767+
req.Header.Set("X-GitHub-Event", tt.eventType) //nolint:canonicalheader // GitHub webhook header
768+
req.Header.Set("X-GitHub-Delivery", "test-delivery-"+tt.name)
769+
770+
// Add valid signature
771+
mac := hmac.New(sha256.New, []byte(secret))
772+
mac.Write(body)
773+
signature := "sha256=" + hex.EncodeToString(mac.Sum(nil))
774+
req.Header.Set("X-Hub-Signature-256", signature)
775+
776+
w := httptest.NewRecorder()
777+
handler.ServeHTTP(w, req)
778+
779+
// Check response status
780+
if w.Code != tt.expectStatusCode {
781+
t.Errorf("expected status %d, got %d", tt.expectStatusCode, w.Code)
782+
}
783+
784+
// Verify URL extraction behavior
785+
// Note: ExtractPRURL returns "" when there's no PR URL in check events.
786+
// The handler in ServeHTTP then falls back to repo URL (lines 163-222 in handler.go)
787+
ctx := context.Background()
788+
extractedURL := ExtractPRURL(ctx, tt.eventType, tt.payload)
789+
790+
if tt.expectRepoURL {
791+
// ExtractPRURL should return empty (no PR URL)
792+
if extractedURL != "" {
793+
t.Errorf("Expected ExtractPRURL to return empty (triggering repo fallback), got %q", extractedURL)
794+
}
795+
// Verify we have repo URL available for fallback
796+
repoURL, ok := tt.payload["repository"].(map[string]any)["html_url"].(string)
797+
if !ok {
798+
t.Fatal("Test setup error: repository.html_url missing")
799+
}
800+
// Repo URL should NOT contain /pull/
801+
if contains := bytes.Contains([]byte(repoURL), []byte("/pull/")); contains {
802+
t.Errorf("Repo URL should not contain /pull/, got %q", repoURL)
803+
}
804+
} else if tt.expectBroadcast {
805+
// Should have PR URL (normal case)
806+
if extractedURL == "" {
807+
t.Error("Expected PR URL but got empty string")
808+
}
809+
if contains := bytes.Contains([]byte(extractedURL), []byte("/pull/")); !contains {
810+
t.Errorf("Expected PR URL with /pull/, got %q", extractedURL)
811+
}
812+
} else {
813+
// Event should be dropped, no URL
814+
if extractedURL != "" {
815+
t.Errorf("Expected empty URL for dropped event, got %q", extractedURL)
816+
}
817+
}
818+
819+
// Verify commit SHA extraction
820+
extractedSHA := extractCommitSHA(tt.eventType, tt.payload)
821+
if tt.expectCommitSHA {
822+
if extractedSHA == "" {
823+
t.Error("Expected commit SHA but got empty string")
824+
}
825+
// Verify it matches the SHA in payload
826+
var expectedSHA string
827+
if tt.eventType == "check_run" {
828+
if checkRun, ok := tt.payload["check_run"].(map[string]any); ok {
829+
expectedSHA, _ = checkRun["head_sha"].(string)
830+
}
831+
} else if tt.eventType == "check_suite" {
832+
if checkSuite, ok := tt.payload["check_suite"].(map[string]any); ok {
833+
expectedSHA, _ = checkSuite["head_sha"].(string)
834+
}
835+
}
836+
if expectedSHA != "" && extractedSHA != expectedSHA {
837+
t.Errorf("Expected SHA %q, got %q", expectedSHA, extractedSHA)
838+
}
839+
}
840+
})
841+
}
842+
}
843+
844+
// TestCheckEventRaceConditionEndToEnd tests the complete flow including hub broadcast.
845+
// This verifies that events with repo URL fallback can still be received by org-based subscribers.
846+
func TestCheckEventRaceConditionEndToEnd(t *testing.T) {
847+
ctx, cancel := context.WithCancel(context.Background())
848+
defer cancel()
849+
850+
h := srv.NewHub()
851+
go h.Run(ctx)
852+
defer h.Stop()
853+
854+
secret := "testsecret"
855+
handler := NewHandler(h, secret, nil)
856+
857+
// Simulate check_run event with empty pull_requests (GitHub race condition)
858+
payload := map[string]any{
859+
"action": "completed",
860+
"check_run": map[string]any{
861+
"head_sha": "racecommit123",
862+
"pull_requests": []any{}, // Empty due to timing
863+
"status": "completed",
864+
},
865+
"repository": map[string]any{
866+
"html_url": "https://github.com/raceorg/racerepo",
867+
"owner": map[string]any{
868+
"login": "raceorg", // This is what org subscribers will match on
869+
},
870+
"name": "racerepo",
871+
},
872+
}
873+
874+
body, err := json.Marshal(payload)
875+
if err != nil {
876+
t.Fatalf("Failed to marshal payload: %v", err)
877+
}
878+
879+
req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(body))
880+
req.Header.Set("X-GitHub-Event", "check_run")
881+
req.Header.Set("X-GitHub-Delivery", "race-test-delivery-123")
882+
883+
mac := hmac.New(sha256.New, []byte(secret))
884+
mac.Write(body)
885+
signature := "sha256=" + hex.EncodeToString(mac.Sum(nil))
886+
req.Header.Set("X-Hub-Signature-256", signature)
887+
888+
w := httptest.NewRecorder()
889+
handler.ServeHTTP(w, req)
890+
891+
if w.Code != http.StatusOK {
892+
t.Errorf("Expected status %d, got %d", http.StatusOK, w.Code)
893+
}
894+
895+
// Verify that ExtractPRURL returns empty (no PR URL in check event with empty pull_requests)
896+
// The handler then falls back to repo URL when broadcasting
897+
extractedURL := ExtractPRURL(ctx, "check_run", payload)
898+
if extractedURL != "" {
899+
t.Errorf("Expected ExtractPRURL to return empty string (no PR URL), got %q", extractedURL)
900+
}
901+
902+
// Verify we have repo URL available for the handler's fallback logic
903+
expectedRepoURL := "https://github.com/raceorg/racerepo"
904+
repoURL, ok := payload["repository"].(map[string]any)["html_url"].(string)
905+
if !ok || repoURL != expectedRepoURL {
906+
t.Errorf("Repository URL should be %q, got %q", expectedRepoURL, repoURL)
907+
}
908+
909+
// Verify commit SHA is extracted for client-side lookup
910+
extractedSHA := extractCommitSHA("check_run", payload)
911+
if extractedSHA != "racecommit123" {
912+
t.Errorf("Expected SHA 'racecommit123', got %q", extractedSHA)
913+
}
914+
915+
// Verify the repo URL is NOT a PR URL (no /pull/ in path)
916+
// This confirms the handler will use repo URL as fallback, not a PR URL
917+
if contains := bytes.Contains([]byte(repoURL), []byte("/pull/")); contains {
918+
t.Errorf("Repo URL should not contain /pull/, got %q", repoURL)
919+
}
920+
}

0 commit comments

Comments
 (0)