Skip to content

Commit d163fc0

Browse files
Thomas StrombergThomas Stromberg
Thomas Stromberg
authored and
Thomas Stromberg
committed
improve test caching, lint
1 parent 8414d52 commit d163fc0

File tree

3 files changed

+102
-88
lines changed

3 files changed

+102
-88
lines changed

‎cmd/goose/cache.go‎

Lines changed: 54 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,34 @@ type cacheEntry struct {
2525
// checkCache checks the cache for a PR and returns the cached data if valid.
2626
// Returns (cachedData, cacheHit, hasRunningTests).
2727
func (app *App) checkCache(cacheFile, url string, updatedAt time.Time) (cachedData *turn.CheckResponse, cacheHit bool, hasRunningTests bool) {
28-
fileData, readErr := os.ReadFile(cacheFile)
29-
if readErr != nil {
30-
if !os.IsNotExist(readErr) {
31-
slog.Debug("[CACHE] Cache file read error", "url", url, "error", readErr)
28+
fileData, err := os.ReadFile(cacheFile)
29+
if err != nil {
30+
if !os.IsNotExist(err) {
31+
slog.Debug("[CACHE] Cache file read error", "url", url, "error", err)
3232
}
3333
return nil, false, false
3434
}
3535

3636
var entry cacheEntry
37-
if unmarshalErr := json.Unmarshal(fileData, &entry); unmarshalErr != nil {
38-
slog.Warn("Failed to unmarshal cache data", "url", url, "error", unmarshalErr)
37+
if err := json.Unmarshal(fileData, &entry); err != nil {
38+
slog.Warn("Failed to unmarshal cache data", "url", url, "error", err)
3939
// Remove corrupted cache file
40-
if removeErr := os.Remove(cacheFile); removeErr != nil {
41-
slog.Error("Failed to remove corrupted cache file", "error", removeErr)
40+
if err := os.Remove(cacheFile); err != nil {
41+
slog.Error("Failed to remove corrupted cache file", "error", err)
4242
}
4343
return nil, false, false
4444
}
4545

46+
// Determine TTL based on test state - use shorter TTL for incomplete tests
47+
testState := entry.Data.PullRequest.TestState
48+
isTestIncomplete := testState == "running" || testState == "queued" || testState == "pending"
49+
ttl := cacheTTL
50+
if isTestIncomplete {
51+
ttl = runningTestsCacheTTL
52+
}
53+
4654
// Check if cache is expired or PR updated
47-
if time.Since(entry.CachedAt) >= cacheTTL || !entry.UpdatedAt.Equal(updatedAt) {
55+
if time.Since(entry.CachedAt) >= ttl || !entry.UpdatedAt.Equal(updatedAt) {
4856
// Log why cache was invalid
4957
if !entry.UpdatedAt.Equal(updatedAt) {
5058
slog.Debug("[CACHE] Cache miss - PR updated",
@@ -56,15 +64,14 @@ func (app *App) checkCache(cacheFile, url string, updatedAt time.Time) (cachedDa
5664
"url", url,
5765
"cached_at", entry.CachedAt.Format(time.RFC3339),
5866
"cache_age", time.Since(entry.CachedAt).Round(time.Second),
59-
"ttl", cacheTTL)
67+
"ttl", ttl,
68+
"test_state", testState)
6069
}
61-
return nil, false, false
70+
return nil, false, isTestIncomplete
6271
}
6372

64-
// Check for incomplete tests that should invalidate cache
73+
// Check for incomplete tests that should invalidate cache and trigger Turn API cache bypass
6574
cacheAge := time.Since(entry.CachedAt)
66-
testState := entry.Data.PullRequest.TestState
67-
isTestIncomplete := testState == "running" || testState == "queued" || testState == "pending"
6875
if entry.Data != nil && isTestIncomplete && cacheAge < runningTestsCacheBypass {
6976
slog.Debug("[CACHE] Cache invalidated - tests incomplete and cache entry is fresh",
7077
"url", url,
@@ -144,15 +151,15 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
144151
"timestamp_sent", timestampToSend.Format(time.RFC3339))
145152
}
146153

147-
var retryErr error
154+
var err error
148155
slog.Debug("[TURN] Making API call",
149156
"url", url,
150157
"user", app.currentUser.GetLogin(),
151158
"pr_updated_at", timestampToSend.Format(time.RFC3339))
152-
data, retryErr = app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), timestampToSend)
153-
if retryErr != nil {
154-
slog.Warn("Turn API error (will retry)", "error", retryErr)
155-
return retryErr
159+
data, err = app.turnClient.Check(turnCtx, url, app.currentUser.GetLogin(), timestampToSend)
160+
if err != nil {
161+
slog.Warn("Turn API error (will retry)", "error", err)
162+
return err
156163
}
157164
slog.Debug("[TURN] API call successful", "url", url)
158165
return nil
@@ -188,45 +195,36 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
188195
}
189196

190197
// Save to cache (don't fail if caching fails) - skip if --no-cache is set
191-
// Don't cache when tests are incomplete - always re-poll to catch completion
192-
if !app.noCache {
193-
shouldCache := true
194-
195-
// Never cache PRs with incomplete tests - we want fresh data on every poll
196-
testState := ""
197-
if data != nil {
198-
testState = data.PullRequest.TestState
199-
}
198+
// Cache PRs with incomplete tests using short TTL to catch completion quickly
199+
if !app.noCache && data != nil {
200+
testState := data.PullRequest.TestState
200201
isTestIncomplete := testState == "running" || testState == "queued" || testState == "pending"
201-
if data != nil && isTestIncomplete {
202-
shouldCache = false
203-
slog.Debug("[CACHE] Skipping cache for PR with incomplete tests",
204-
"url", url,
205-
"test_state", testState,
206-
"pending_checks", len(data.PullRequest.CheckSummary.Pending))
207-
}
208202

209-
if shouldCache {
210-
entry := cacheEntry{
211-
Data: data,
212-
CachedAt: time.Now(),
213-
UpdatedAt: updatedAt,
214-
}
215-
if cacheData, marshalErr := json.Marshal(entry); marshalErr != nil {
216-
slog.Error("Failed to marshal cache data", "url", url, "error", marshalErr)
203+
entry := cacheEntry{
204+
Data: data,
205+
CachedAt: time.Now(),
206+
UpdatedAt: updatedAt,
207+
}
208+
if cacheData, err := json.Marshal(entry); err != nil {
209+
slog.Error("Failed to marshal cache data", "url", url, "error", err)
210+
} else {
211+
// Ensure cache directory exists with secure permissions
212+
if err := os.MkdirAll(filepath.Dir(cacheFile), 0o700); err != nil {
213+
slog.Error("Failed to create cache directory", "error", err)
214+
} else if err := os.WriteFile(cacheFile, cacheData, 0o600); err != nil {
215+
slog.Error("Failed to write cache file", "error", err)
217216
} else {
218-
// Ensure cache directory exists with secure permissions
219-
if dirErr := os.MkdirAll(filepath.Dir(cacheFile), 0o700); dirErr != nil {
220-
slog.Error("Failed to create cache directory", "error", dirErr)
221-
} else if writeErr := os.WriteFile(cacheFile, cacheData, 0o600); writeErr != nil {
222-
slog.Error("Failed to write cache file", "error", writeErr)
223-
} else {
224-
slog.Debug("[CACHE] Saved to cache",
225-
"url", url,
226-
"cached_at", entry.CachedAt.Format(time.RFC3339),
227-
"pr_updated_at", entry.UpdatedAt.Format(time.RFC3339),
228-
"cache_file", filepath.Base(cacheFile))
217+
ttl := cacheTTL
218+
if isTestIncomplete {
219+
ttl = runningTestsCacheTTL
229220
}
221+
slog.Debug("[CACHE] Saved to cache",
222+
"url", url,
223+
"cached_at", entry.CachedAt.Format(time.RFC3339),
224+
"pr_updated_at", entry.UpdatedAt.Format(time.RFC3339),
225+
"ttl", ttl,
226+
"test_state", testState,
227+
"cache_file", filepath.Base(cacheFile))
230228
}
231229
}
232230
}
@@ -258,8 +256,8 @@ func (app *App) cleanupOldCache() {
258256
// Remove cache files older than cleanup interval (15 days)
259257
if time.Since(info.ModTime()) > cacheCleanupInterval {
260258
filePath := filepath.Join(app.cacheDir, entry.Name())
261-
if removeErr := os.Remove(filePath); removeErr != nil {
262-
slog.Error("Failed to remove old cache file", "file", filePath, "error", removeErr)
259+
if err := os.Remove(filePath); err != nil {
260+
slog.Error("Failed to remove old cache file", "file", filePath, "error", err)
263261
errorCount++
264262
} else {
265263
cleanupCount++

‎cmd/goose/main.go‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ var (
3232

3333
const (
3434
cacheTTL = 10 * 24 * time.Hour // 10 days - rely mostly on PR UpdatedAt
35+
runningTestsCacheTTL = 2 * time.Minute // Short TTL for PRs with incomplete tests to catch completions quickly
3536
cacheCleanupInterval = 15 * 24 * time.Hour // 15 days - cleanup older than cache TTL
3637
stalePRThreshold = 90 * 24 * time.Hour
3738
runningTestsCacheBypass = 90 * time.Minute // Don't cache PRs with running tests if fresher than this
@@ -249,11 +250,11 @@ func main() {
249250
if app.client != nil {
250251
var user *github.User
251252
err := retry.Do(func() error {
252-
var retryErr error
253-
user, _, retryErr = app.client.Users.Get(ctx, "")
254-
if retryErr != nil {
255-
slog.Warn("GitHub Users.Get failed (will retry)", "error", retryErr)
256-
return retryErr
253+
var err error
254+
user, _, err = app.client.Users.Get(ctx, "")
255+
if err != nil {
256+
slog.Warn("GitHub Users.Get failed (will retry)", "error", err)
257+
return err
257258
}
258259
return nil
259260
},
@@ -327,11 +328,11 @@ func (app *App) handleReauthentication(ctx context.Context) {
327328
if app.client != nil {
328329
var user *github.User
329330
err := retry.Do(func() error {
330-
var retryErr error
331-
user, _, retryErr = app.client.Users.Get(ctx, "")
332-
if retryErr != nil {
333-
slog.Warn("GitHub Users.Get failed (will retry)", "error", retryErr)
334-
return retryErr
331+
var err error
332+
user, _, err = app.client.Users.Get(ctx, "")
333+
if err != nil {
334+
slog.Warn("GitHub Users.Get failed (will retry)", "error", err)
335+
return err
335336
}
336337
return nil
337338
},
@@ -536,9 +537,9 @@ func (app *App) updatePRs(ctx context.Context) {
536537

537538
var incoming, outgoing []PR
538539
err := safeExecute("fetchPRs", func() error {
539-
var fetchErr error
540-
incoming, outgoing, fetchErr = app.fetchPRsInternal(ctx)
541-
return fetchErr
540+
var err error
541+
incoming, outgoing, err = app.fetchPRsInternal(ctx)
542+
return err
542543
})
543544
if err != nil {
544545
slog.Error("Error fetching PRs", "error", err)

‎cmd/goose/sprinkler.go‎

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,19 @@ const (
2424
sprinklerMaxDelay = 10 * time.Second // Max delay between retries
2525
)
2626

27+
// prEvent captures the essential details from a sprinkler event.
28+
type prEvent struct {
29+
timestamp time.Time
30+
url string
31+
}
32+
2733
// sprinklerMonitor manages WebSocket event subscriptions for all user orgs.
2834
type sprinklerMonitor struct {
2935
lastConnectedAt time.Time
3036
app *App
3137
client *client.Client
3238
cancel context.CancelFunc
33-
eventChan chan string
39+
eventChan chan prEvent
3440
lastEventMap map[string]time.Time
3541
token string
3642
orgs []string
@@ -45,7 +51,7 @@ func newSprinklerMonitor(app *App, token string) *sprinklerMonitor {
4551
app: app,
4652
token: token,
4753
orgs: make([]string, 0),
48-
eventChan: make(chan string, eventChannelSize),
54+
eventChan: make(chan prEvent, eventChannelSize),
4955
lastEventMap: make(map[string]time.Time),
5056
}
5157
}
@@ -243,12 +249,15 @@ func (sm *sprinklerMonitor) handleEvent(event client.Event) {
243249

244250
slog.Info("[SPRINKLER] PR event received",
245251
"url", event.URL,
246-
"org", org)
252+
"org", org,
253+
"timestamp", event.Timestamp.Format(time.RFC3339))
247254

248255
// Send to event channel for processing (non-blocking)
249256
select {
250-
case sm.eventChan <- event.URL:
251-
slog.Debug("[SPRINKLER] Event queued for processing", "url", event.URL)
257+
case sm.eventChan <- prEvent{timestamp: event.Timestamp, url: event.URL}:
258+
slog.Debug("[SPRINKLER] Event queued for processing",
259+
"url", event.URL,
260+
"timestamp", event.Timestamp.Format(time.RFC3339))
252261
default:
253262
slog.Warn("[SPRINKLER] Event channel full, dropping event",
254263
"url", event.URL,
@@ -268,34 +277,34 @@ func (sm *sprinklerMonitor) processEvents(ctx context.Context) {
268277
select {
269278
case <-ctx.Done():
270279
return
271-
case prURL := <-sm.eventChan:
272-
sm.checkAndNotify(ctx, prURL)
280+
case evt := <-sm.eventChan:
281+
sm.checkAndNotify(ctx, evt)
273282
}
274283
}
275284
}
276285

277286
// checkAndNotify checks if a PR is blocking and sends notification if needed.
278-
func (sm *sprinklerMonitor) checkAndNotify(ctx context.Context, url string) {
287+
func (sm *sprinklerMonitor) checkAndNotify(ctx context.Context, evt prEvent) {
279288
start := time.Now()
280289

281290
user := sm.currentUser()
282291
if user == "" {
283-
slog.Debug("[SPRINKLER] Skipping check - no user configured", "url", url)
292+
slog.Debug("[SPRINKLER] Skipping check - no user configured", "url", evt.url)
284293
return
285294
}
286295

287-
repo, n := parseRepoAndNumberFromURL(url)
296+
repo, n := parseRepoAndNumberFromURL(evt.url)
288297
if repo == "" || n == 0 {
289-
slog.Warn("[SPRINKLER] Failed to parse PR URL", "url", url)
298+
slog.Warn("[SPRINKLER] Failed to parse PR URL", "url", evt.url)
290299
return
291300
}
292301

293-
data, cached := sm.fetchTurnData(ctx, url, repo, n, start)
302+
data, cached := sm.fetchTurnData(ctx, evt, repo, n, start)
294303
if data == nil {
295304
return
296305
}
297306

298-
if sm.handleClosedPR(ctx, data, url, repo, n, cached) {
307+
if sm.handleClosedPR(ctx, data, evt.url, repo, n, cached) {
299308
return
300309
}
301310

@@ -304,11 +313,11 @@ func (sm *sprinklerMonitor) checkAndNotify(ctx context.Context, url string) {
304313
return
305314
}
306315

307-
if sm.handleNewPR(ctx, url, repo, n, act) {
316+
if sm.handleNewPR(ctx, evt.url, repo, n, act) {
308317
return
309318
}
310319

311-
if sm.isAlreadyTrackedAsBlocked(url, repo, n) {
320+
if sm.isAlreadyTrackedAsBlocked(evt.url, repo, n) {
312321
return
313322
}
314323

@@ -317,9 +326,10 @@ func (sm *sprinklerMonitor) checkAndNotify(ctx context.Context, url string) {
317326
"number", n,
318327
"action", act.Kind,
319328
"reason", act.Reason,
329+
"event_timestamp", evt.timestamp.Format(time.RFC3339),
320330
"elapsed", time.Since(start).Round(time.Millisecond))
321331

322-
sm.sendNotifications(ctx, url, repo, n, act)
332+
sm.sendNotifications(ctx, evt.url, repo, n, act)
323333
}
324334

325335
// currentUser returns the configured user for the sprinkler monitor.
@@ -335,16 +345,20 @@ func (sm *sprinklerMonitor) currentUser() string {
335345
}
336346

337347
// fetchTurnData retrieves PR data from Turn API with retry logic.
338-
func (sm *sprinklerMonitor) fetchTurnData(ctx context.Context, url, repo string, n int, start time.Time) (*turn.CheckResponse, bool) {
348+
func (sm *sprinklerMonitor) fetchTurnData(ctx context.Context, evt prEvent, repo string, n int, start time.Time) (*turn.CheckResponse, bool) {
339349
var data *turn.CheckResponse
340350
var cached bool
341351

342352
err := retry.Do(func() error {
343353
var err error
344-
data, cached, err = sm.app.turnData(ctx, url, time.Now())
354+
// Use event timestamp to bypass caching - this ensures we get fresh data for real-time events
355+
data, cached, err = sm.app.turnData(ctx, evt.url, evt.timestamp)
345356
if err != nil {
346357
slog.Debug("[SPRINKLER] Turn API call failed (will retry)",
347-
"repo", repo, "number", n, "error", err)
358+
"repo", repo,
359+
"number", n,
360+
"event_timestamp", evt.timestamp.Format(time.RFC3339),
361+
"error", err)
348362
return err
349363
}
350364
return nil
@@ -365,6 +379,7 @@ func (sm *sprinklerMonitor) fetchTurnData(ctx context.Context, url, repo string,
365379
slog.Warn("[SPRINKLER] Failed to get turn data after retries",
366380
"repo", repo,
367381
"number", n,
382+
"event_timestamp", evt.timestamp.Format(time.RFC3339),
368383
"elapsed", time.Since(start).Round(time.Millisecond),
369384
"error", err)
370385
return nil, false

0 commit comments

Comments
 (0)