diff --git a/pkg/cli/mcp_server_cache.go b/pkg/cli/mcp_server_cache.go index f7c6410ca36..649cbfeb9fe 100644 --- a/pkg/cli/mcp_server_cache.go +++ b/pkg/cli/mcp_server_cache.go @@ -37,24 +37,33 @@ func newMCPCacheStore() *mcpCacheStore { } } +func (c *mcpCacheStore) getPermissionEntry(cacheKey string) (*permissionEntry, bool) { + c.mu.RLock() + defer c.mu.RUnlock() + entry, ok := c.permissions[cacheKey] + return entry, ok +} + +func (c *mcpCacheStore) deletePermissionEntryIfUnchanged(cacheKey string, entry *permissionEntry) { + c.mu.Lock() + defer c.mu.Unlock() + if currentEntry, ok := c.permissions[cacheKey]; ok && currentEntry == entry { + delete(c.permissions, cacheKey) + } +} + // GetPermission returns the cached permission for the given actor and repo, or ("", false) on cache miss. func (c *mcpCacheStore) GetPermission(actor, repo string) (string, bool) { cacheKey := actor + ":" + repo - c.mu.RLock() - entry, ok := c.permissions[cacheKey] + entry, ok := c.getPermissionEntry(cacheKey) if ok && time.Since(entry.timestamp) < c.permissionTTL { - perm := entry.permission - c.mu.RUnlock() - mcpServerCacheLog.Printf("Permission cache hit: actor=%s, repo=%s, permission=%s", actor, repo, perm) - return perm, true + mcpServerCacheLog.Printf("Permission cache hit: actor=%s, repo=%s, permission=%s", actor, repo, entry.permission) + return entry.permission, true } - c.mu.RUnlock() if ok { // Expired — remove it mcpServerCacheLog.Printf("Permission cache entry expired for actor=%s, repo=%s", actor, repo) - c.mu.Lock() - delete(c.permissions, cacheKey) - c.mu.Unlock() + c.deletePermissionEntryIfUnchanged(cacheKey, entry) } mcpServerCacheLog.Printf("Permission cache miss: actor=%s, repo=%s", actor, repo) return "", false @@ -65,11 +74,11 @@ func (c *mcpCacheStore) SetPermission(actor, repo, permission string) { cacheKey := actor + ":" + repo mcpServerCacheLog.Printf("Caching permission: actor=%s, repo=%s, permission=%s", actor, repo, permission) c.mu.Lock() + defer c.mu.Unlock() c.permissions[cacheKey] = &permissionEntry{ permission: permission, timestamp: time.Now(), } - c.mu.Unlock() } // GetRepo returns the cached repository name, or ("", false) on cache miss. @@ -88,11 +97,11 @@ func (c *mcpCacheStore) GetRepo() (string, bool) { func (c *mcpCacheStore) SetRepo(repository string) { mcpServerCacheLog.Printf("Caching repository: %s", repository) c.mu.Lock() + defer c.mu.Unlock() c.repo = &repoEntry{ repository: repository, timestamp: time.Now(), } - c.mu.Unlock() } var mcpCache = newMCPCacheStore() diff --git a/pkg/cli/mcp_server_cache_test.go b/pkg/cli/mcp_server_cache_test.go index 601549d914f..89bfa1849d9 100644 --- a/pkg/cli/mcp_server_cache_test.go +++ b/pkg/cli/mcp_server_cache_test.go @@ -81,6 +81,27 @@ func TestMCPCacheStore_PermissionExpiry(t *testing.T) { } } +func TestMCPCacheStore_DeletePermissionEntryIfUnchanged_PreservesRefreshedEntry(t *testing.T) { + cache := newMCPCacheStore() + cacheKey := "actor:owner/repo" + expiredEntry := &permissionEntry{ + permission: "read", + timestamp: time.Now().Add(-2 * cache.permissionTTL), + } + cache.permissions[cacheKey] = expiredEntry + + cache.SetPermission("actor", "owner/repo", "write") + cache.deletePermissionEntryIfUnchanged(cacheKey, expiredEntry) + + entry, ok := cache.getPermissionEntry(cacheKey) + if !ok { + t.Fatal("expected refreshed permission entry to remain cached") + } + if entry.permission != "write" { + t.Fatalf("expected refreshed permission to be preserved, got %q", entry.permission) + } +} + func TestMCPCacheStore_RepoExpiry(t *testing.T) { cache := newMCPCacheStore() cache.repoTTL = 10 * time.Millisecond diff --git a/pkg/cli/repo.go b/pkg/cli/repo.go index b5fbcb3e343..5a9b97fba85 100644 --- a/pkg/cli/repo.go +++ b/pkg/cli/repo.go @@ -91,14 +91,15 @@ func getCurrentRepoSlugUncached() (string, error) { // GetCurrentRepoSlug gets the current repository slug with caching. // This is the recommended function to use for repository access across the codebase. func GetCurrentRepoSlug() (string, error) { - currentRepoSlugCache.mu.Lock() - if !currentRepoSlugCache.done { - currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached() - currentRepoSlugCache.done = true - } - result := currentRepoSlugCache.result - err := currentRepoSlugCache.err - currentRepoSlugCache.mu.Unlock() + result, err := func() (string, error) { + currentRepoSlugCache.mu.Lock() + defer currentRepoSlugCache.mu.Unlock() + if !currentRepoSlugCache.done { + currentRepoSlugCache.result, currentRepoSlugCache.err = getCurrentRepoSlugUncached() + currentRepoSlugCache.done = true + } + return currentRepoSlugCache.result, currentRepoSlugCache.err + }() if err != nil { return "", err diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index d34cd584bb0..ffa0d0195bb 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -112,11 +112,7 @@ func (l *Logger) Printf(format string, args ...any) { if !l.enabled { return } - l.mu.Lock() - now := time.Now() - diff := now.Sub(l.lastLog) - l.lastLog = now - l.mu.Unlock() + diff := l.tickTime() message := fmt.Sprintf(format, args...) lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) @@ -129,14 +125,19 @@ func (l *Logger) Print(args ...any) { if !l.enabled { return } + diff := l.tickTime() + + message := fmt.Sprint(args...) + lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) +} + +func (l *Logger) tickTime() time.Duration { l.mu.Lock() + defer l.mu.Unlock() now := time.Now() diff := now.Sub(l.lastLog) l.lastLog = now - l.mu.Unlock() - - message := fmt.Sprint(args...) - lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) + return diff } // computeEnabled computes whether a namespace matches the DEBUG patterns diff --git a/pkg/workflow/repository_features_validation.go b/pkg/workflow/repository_features_validation.go index 19c2cffea2a..373097cb979 100644 --- a/pkg/workflow/repository_features_validation.go +++ b/pkg/workflow/repository_features_validation.go @@ -157,14 +157,15 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error // getCurrentRepository gets the current repository from git context (with caching) func getCurrentRepository() (string, error) { - currentRepositoryCache.mu.Lock() - if !currentRepositoryCache.done { - currentRepositoryCache.result, currentRepositoryCache.err = getCurrentRepositoryUncached() - currentRepositoryCache.done = true - } - result := currentRepositoryCache.result - err := currentRepositoryCache.err - currentRepositoryCache.mu.Unlock() + result, err := func() (string, error) { + currentRepositoryCache.mu.Lock() + defer currentRepositoryCache.mu.Unlock() + if !currentRepositoryCache.done { + currentRepositoryCache.result, currentRepositoryCache.err = getCurrentRepositoryUncached() + currentRepositoryCache.done = true + } + return currentRepositoryCache.result, currentRepositoryCache.err + }() if err != nil { return "", err