Skip to content

Commit de12e2b

Browse files
tstrombergtstromberg
authored
Merge pull request #40 from tstromberg/main
don't cache commit->PR misses, cleanup lint
2 parents 7125a03 + 88c723b commit de12e2b

File tree

9 files changed

+179
-43
lines changed

9 files changed

+179
-43
lines changed

‎pkg/client/client.go‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -735,11 +735,15 @@ func (c *Client) readEvents(ctx context.Context, ws *websocket.Conn) error {
735735

736736
// Check cache first
737737
c.cacheMu.RLock()
738-
cached, ok := c.commitPRCache[key]
738+
cached, cacheExists := c.commitPRCache[key]
739+
expiry, hasExpiry := c.commitPRCacheExpiry[key]
739740
c.cacheMu.RUnlock()
740741

742+
// Check if cached empty result has expired (need to re-query GitHub)
743+
cacheExpired := cacheExists && len(cached) == 0 && hasExpiry && time.Now().After(expiry)
744+
741745
var prs []int
742-
if ok {
746+
if cacheExists && !cacheExpired {
743747
// Cache hit - return copy to prevent external modifications
744748
prs = make([]int, len(cached))
745749
copy(prs, cached)
@@ -750,7 +754,13 @@ func (c *Client) readEvents(ctx context.Context, ws *websocket.Conn) error {
750754
"pr_count", len(prs),
751755
"cache_hit", true)
752756
} else {
753-
// Cache miss - look up via GitHub API
757+
// Cache miss or expired empty result - look up via GitHub API
758+
if cacheExpired {
759+
c.logger.Info("Cached empty result expired - retrying GitHub API",
760+
"commit_sha", event.CommitSHA,
761+
"repo_url", event.URL,
762+
"type", event.Type)
763+
}
754764
c.logger.Info("Check event with repo URL - looking up PRs via GitHub API",
755765
"commit_sha", event.CommitSHA,
756766
"repo_url", event.URL,
@@ -776,14 +786,27 @@ func (c *Client) readEvents(ctx context.Context, ws *websocket.Conn) error {
776786
c.commitCacheKeys = append(c.commitCacheKeys, key)
777787
}
778788
// Store copy to prevent external modifications
779-
cached := make([]int, len(prs))
780-
copy(cached, prs)
781-
c.commitPRCache[key] = cached
789+
cachedPRs := make([]int, len(prs))
790+
copy(cachedPRs, prs)
791+
c.commitPRCache[key] = cachedPRs
792+
// Set TTL for empty results so we retry after indexing delay
793+
if len(prs) == 0 {
794+
c.commitPRCacheExpiry[key] = time.Now().Add(c.emptyResultTTL)
795+
} else {
796+
// Remove any expiry for non-empty results (cache permanently until evicted)
797+
delete(c.commitPRCacheExpiry, key)
798+
}
782799
c.cacheMu.Unlock()
783800

784-
c.logger.Info("Cached PR lookup result",
785-
"commit_sha", event.CommitSHA,
786-
"pr_count", len(prs))
801+
if len(prs) == 0 {
802+
c.logger.Info("Cached empty PR lookup result with TTL",
803+
"commit_sha", event.CommitSHA,
804+
"ttl_seconds", int(c.emptyResultTTL.Seconds()))
805+
} else {
806+
c.logger.Info("Cached PR lookup result",
807+
"commit_sha", event.CommitSHA,
808+
"pr_count", len(prs))
809+
}
787810
}
788811
}
789812

‎pkg/client/client_test.go‎

Lines changed: 114 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http/httptest"
99
"strings"
1010
"sync"
11+
"sync/atomic"
1112
"testing"
1213
"time"
1314

@@ -2817,7 +2818,7 @@ func TestWriteChannelBackpressure(t *testing.T) {
28172818
func TestPingChannelFullScenario(t *testing.T) {
28182819
t.Parallel()
28192820

2820-
pongReceived := false
2821+
var pongReceived atomic.Bool
28212822
srv := httptest.NewServer(websocket.Handler(func(ws *websocket.Conn) {
28222823
var sub map[string]any
28232824
_ = websocket.JSON.Receive(ws, &sub)
@@ -2836,7 +2837,7 @@ func TestPingChannelFullScenario(t *testing.T) {
28362837
"type": "pong",
28372838
"seq": fmt.Sprintf("%v", msg["seq"]),
28382839
})
2839-
pongReceived = true
2840+
pongReceived.Store(true)
28402841
}
28412842
}
28422843
}))
@@ -2863,7 +2864,7 @@ func TestPingChannelFullScenario(t *testing.T) {
28632864
time.Sleep(300 * time.Millisecond)
28642865
client.Stop()
28652866

2866-
if !pongReceived {
2867+
if !pongReceived.Load() {
28672868
t.Error("Expected at least one pong to be received")
28682869
}
28692870
}
@@ -3000,7 +3001,7 @@ func TestConcurrentWebSocketClose(t *testing.T) {
30003001
func TestNetworkErrorDuringWrite(t *testing.T) {
30013002
t.Parallel()
30023003

3003-
connectionBroken := false
3004+
var connectionBroken atomic.Bool
30043005
srv := httptest.NewServer(websocket.Handler(func(ws *websocket.Conn) {
30053006
var sub map[string]any
30063007
_ = websocket.JSON.Receive(ws, &sub)
@@ -3011,19 +3012,19 @@ func TestNetworkErrorDuringWrite(t *testing.T) {
30113012

30123013
time.Sleep(50 * time.Millisecond)
30133014
ws.Close()
3014-
connectionBroken = true
3015+
connectionBroken.Store(true)
30153016
}))
30163017
defer srv.Close()
30173018

3018-
errorReceived := false
3019+
var errorReceived atomic.Bool
30193020
client, err := New(Config{
30203021
ServerURL: "ws" + strings.TrimPrefix(srv.URL, "http"),
30213022
Token: "test-token",
30223023
Organization: "test-org",
30233024
PingInterval: 20 * time.Millisecond,
30243025
NoReconnect: true,
30253026
OnDisconnect: func(err error) {
3026-
errorReceived = true
3027+
errorReceived.Store(true)
30273028
},
30283029
})
30293030
if err != nil {
@@ -3035,7 +3036,7 @@ func TestNetworkErrorDuringWrite(t *testing.T) {
30353036

30363037
_ = client.Start(ctx)
30373038

3038-
if connectionBroken && !errorReceived {
3039+
if connectionBroken.Load() && !errorReceived.Load() {
30393040
t.Log("Warning: Network error may not have been properly detected")
30403041
}
30413042
}
@@ -3084,3 +3085,108 @@ func TestIOTimeoutRecovery(t *testing.T) {
30843085
t.Log("Client successfully received event and handled I/O timeouts")
30853086
}
30863087
}
3088+
3089+
// TestEmptyResultCacheTTL tests that empty cache results expire after the TTL.
3090+
func TestEmptyResultCacheTTL(t *testing.T) {
3091+
client, err := New(Config{
3092+
ServerURL: "ws://localhost:8080",
3093+
Token: "test-token",
3094+
Organization: "test-org",
3095+
NoReconnect: true,
3096+
})
3097+
if err != nil {
3098+
t.Fatalf("Failed to create client: %v", err)
3099+
}
3100+
3101+
// Override TTL to a very short duration for testing
3102+
client.emptyResultTTL = 50 * time.Millisecond
3103+
3104+
key := "owner/repo:commit_empty"
3105+
3106+
// Simulate caching an empty result (as production code does)
3107+
client.cacheMu.Lock()
3108+
client.commitCacheKeys = append(client.commitCacheKeys, key)
3109+
client.commitPRCache[key] = []int{} // Empty result
3110+
client.commitPRCacheExpiry[key] = time.Now().Add(client.emptyResultTTL)
3111+
client.cacheMu.Unlock()
3112+
3113+
// Verify cache entry exists and is not expired
3114+
client.cacheMu.RLock()
3115+
cached, cacheExists := client.commitPRCache[key]
3116+
expiry, hasExpiry := client.commitPRCacheExpiry[key]
3117+
client.cacheMu.RUnlock()
3118+
3119+
if !cacheExists {
3120+
t.Fatal("Expected cache entry to exist")
3121+
}
3122+
if len(cached) != 0 {
3123+
t.Errorf("Expected empty cached result, got %v", cached)
3124+
}
3125+
if !hasExpiry {
3126+
t.Fatal("Expected expiry to be set for empty result")
3127+
}
3128+
3129+
// Check that it's not expired yet
3130+
cacheExpired := cacheExists && len(cached) == 0 && hasExpiry && time.Now().After(expiry)
3131+
if cacheExpired {
3132+
t.Error("Cache should not be expired immediately after creation")
3133+
}
3134+
3135+
// Wait for TTL to expire
3136+
time.Sleep(60 * time.Millisecond)
3137+
3138+
// Check that it's now expired
3139+
client.cacheMu.RLock()
3140+
cached, cacheExists = client.commitPRCache[key]
3141+
expiry, hasExpiry = client.commitPRCacheExpiry[key]
3142+
client.cacheMu.RUnlock()
3143+
3144+
cacheExpired = cacheExists && len(cached) == 0 && hasExpiry && time.Now().After(expiry)
3145+
if !cacheExpired {
3146+
t.Errorf("Cache should be expired after TTL, expiry=%v, now=%v", expiry, time.Now())
3147+
}
3148+
}
3149+
3150+
// TestNonEmptyResultNoCacheTTL tests that non-empty cache results don't have a TTL.
3151+
func TestNonEmptyResultNoCacheTTL(t *testing.T) {
3152+
client, err := New(Config{
3153+
ServerURL: "ws://localhost:8080",
3154+
Token: "test-token",
3155+
Organization: "test-org",
3156+
NoReconnect: true,
3157+
})
3158+
if err != nil {
3159+
t.Fatalf("Failed to create client: %v", err)
3160+
}
3161+
3162+
key := "owner/repo:commit_with_pr"
3163+
3164+
// Simulate caching a non-empty result
3165+
client.cacheMu.Lock()
3166+
client.commitCacheKeys = append(client.commitCacheKeys, key)
3167+
client.commitPRCache[key] = []int{123, 456} // Non-empty result
3168+
// Production code doesn't set expiry for non-empty results
3169+
client.cacheMu.Unlock()
3170+
3171+
// Verify no expiry is set for non-empty results
3172+
client.cacheMu.RLock()
3173+
cached, cacheExists := client.commitPRCache[key]
3174+
_, hasExpiry := client.commitPRCacheExpiry[key]
3175+
client.cacheMu.RUnlock()
3176+
3177+
if !cacheExists {
3178+
t.Fatal("Expected cache entry to exist")
3179+
}
3180+
if len(cached) != 2 {
3181+
t.Errorf("Expected 2 cached PRs, got %v", cached)
3182+
}
3183+
if hasExpiry {
3184+
t.Error("Non-empty results should not have expiry set")
3185+
}
3186+
3187+
// The cache entry should never be considered expired
3188+
cacheExpired := cacheExists && len(cached) == 0 && hasExpiry && time.Now().After(time.Time{})
3189+
if cacheExpired {
3190+
t.Error("Non-empty cache should never be expired")
3191+
}
3192+
}

‎pkg/github/client.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,8 @@ func (c *Client) AuthenticatedUser(ctx context.Context) (*User, error) {
113113

114114
case http.StatusForbidden:
115115
// Check if rate limited
116-
if resp.Header.Get("X-RateLimit-Remaining") == "0" { //nolint:canonicalheader // GitHub API header
117-
resetTime := resp.Header.Get("X-RateLimit-Reset") //nolint:canonicalheader // GitHub API header
116+
if resp.Header.Get("X-Ratelimit-Remaining") == "0" {
117+
resetTime := resp.Header.Get("X-Ratelimit-Reset")
118118
c.logger.Warn("GitHub API: 403 Forbidden - rate limit exceeded for /user endpoint", "reset_at", resetTime)
119119
lastErr = errors.New("GitHub API rate limit exceeded")
120120
return lastErr // Retry after backoff

‎pkg/github/client_test.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,8 +106,8 @@ func TestAuthenticatedUser_RateLimit(t *testing.T) {
106106
attempts := 0
107107
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
108108
attempts++
109-
w.Header().Set("X-RateLimit-Remaining", "0") //nolint:canonicalheader // GitHub API header
110-
w.Header().Set("X-RateLimit-Reset", "1234567890") //nolint:canonicalheader // GitHub API header
109+
w.Header().Set("X-Ratelimit-Remaining", "0")
110+
w.Header().Set("X-Ratelimit-Reset", "1234567890")
111111
w.WriteHeader(http.StatusForbidden)
112112
_, _ = w.Write([]byte(`{"message":"API rate limit exceeded"}`))
113113
}))
@@ -901,7 +901,6 @@ func TestMockClient(t *testing.T) {
901901

902902
ctx := context.Background()
903903
username, orgs, err := mock.UserAndOrgs(ctx)
904-
905904
if err != nil {
906905
t.Fatalf("unexpected error: %v", err)
907906
}
@@ -943,7 +942,6 @@ func TestMockClient(t *testing.T) {
943942

944943
ctx := context.Background()
945944
username, orgs, err := mock.ValidateOrgMembership(ctx, "org1")
946-
947945
if err != nil {
948946
t.Fatalf("unexpected error: %v", err)
949947
}

‎pkg/github/mock.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,25 @@ package github
33
import (
44
"context"
55
"errors"
6+
"sync"
67
)
78

89
// MockClient is a mock GitHub API client for testing.
10+
// Thread-safe for concurrent access.
911
type MockClient struct {
1012
Err error
1113
Username string
1214
LastValidatedOrg string
1315
Orgs []string
1416
UserAndOrgsCalls int
1517
ValidateOrgMembershipCalls int
18+
mu sync.Mutex
1619
}
1720

1821
// UserAndOrgs returns the mock user info.
19-
func (m *MockClient) UserAndOrgs(ctx context.Context) (username string, orgs []string, err error) {
22+
func (m *MockClient) UserAndOrgs(_ context.Context) (username string, orgs []string, err error) {
23+
m.mu.Lock()
24+
defer m.mu.Unlock()
2025
m.UserAndOrgsCalls++
2126
if m.Err != nil {
2227
return "", nil, m.Err
@@ -25,7 +30,9 @@ func (m *MockClient) UserAndOrgs(ctx context.Context) (username string, orgs []s
2530
}
2631

2732
// ValidateOrgMembership validates organization membership using the mock data.
28-
func (m *MockClient) ValidateOrgMembership(ctx context.Context, org string) (username string, orgs []string, err error) {
33+
func (m *MockClient) ValidateOrgMembership(_ context.Context, org string) (username string, orgs []string, err error) {
34+
m.mu.Lock()
35+
defer m.mu.Unlock()
2936
m.ValidateOrgMembershipCalls++
3037
m.LastValidatedOrg = org
3138

‎pkg/srv/commit_cache.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
// Package srv provides a WebSocket hub for managing client connections and broadcasting
2-
// GitHub webhook events to subscribed clients based on their subscription criteria.
31
package srv
42

53
import (
@@ -21,11 +19,13 @@ const (
2119
)
2220

2321
// PRInfo contains cached information about a pull request.
22+
//
23+
//nolint:govet // Field order optimized for readability over memory padding
2424
type PRInfo struct {
2525
URL string // Full PR URL (e.g., https://github.com/owner/repo/pull/123)
26-
Number int // PR number
2726
RepoURL string // Repository URL (e.g., https://github.com/owner/repo)
2827
CachedAt time.Time
28+
Number int // PR number
2929
}
3030

3131
// CommitCache maps commit SHAs to their associated pull requests.

‎pkg/srv/hub.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ type Event struct {
4242
// - Non-blocking send to client.send channel prevents deadlocks
4343
// - If client disconnects during iteration, send fails gracefully (channel full or closed)
4444
// - Client.Close() is safe to call multiple times during this window
45+
//
46+
//nolint:govet // Field order optimized for readability over memory padding
4547
type Hub struct {
4648
clients map[string]*Client
4749
register chan *Client

0 commit comments

Comments
 (0)