- Notifications
You must be signed in to change notification settings - Fork3.1k
Make RepoAccessCache a singleton#1426
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Make RepoAccessCache a singleton#1426
Conversation
Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>
Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>
JoannaaKL commentedNov 18, 2025
@copilot Remove cacheIDCounter, we don't need it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR implements a singleton pattern forRepoAccessCache to ensure consistent configuration across the application and fixes a bug in cache table naming. The implementation usessync.Once for thread-safe lazy initialization while preserving the ability to create independent instances for testing.
- Adds
GetInstance()for singleton access with lazy initialization - Fixes cache table naming bug by using atomic counter instead of hardcoded string
- Keeps
NewRepoAccessCache()for test isolation in parallel tests
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/lockdown/lockdown.go | Implements singleton pattern with GetInstance(), ResetInstance(), and refactors cache creation into shared helper. Fixes cache naming with atomic counter |
| pkg/lockdown/lockdown_test.go | Adds comprehensive tests for singleton behavior, reset functionality, and independent instance creation |
| internal/ghmcp/server.go | Updates production code to use GetInstance() instead of NewRepoAccessCache() |
| go.sum | Removes unused indirect dependency entries (appears to be from go mod tidy) |
| instance.cache.Flush() | ||
| } | ||
| instance=nil | ||
| instanceOnce= sync.Once{} |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Resettingsync.Once by reassigning it doesn't work as intended. Thesync.Once type contains unexported state that tracks whetherDo() has been called. Simply reassigninginstanceOnce = sync.Once{} creates a newsync.Once, but it doesn't atomically coordinate with concurrent goroutines that might be executing or about to executeGetInstance().
This creates multiple race conditions:
- A goroutine could read the old
instanceOncevalue and be in the middle of executing itsDo()function whileResetInstance()replaces it - The reassignment of
instanceOnceitself is not atomic with respect to reads inGetInstance()
For a proper reset in tests, consider using a build tag or test-only code path, or accept thatResetInstance() is fundamentally unsafe for concurrent use and document this limitation more prominently (e.g., "MUST NOT be called concurrently with GetInstance()").
| funcGetInstance(client*githubv4.Client,opts...RepoAccessOption)*RepoAccessCache { | ||
| instanceOnce.Do(func() { | ||
| instance=newRepoAccessCache(client,opts...) | ||
| }) |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheinstanceMu mutex is declared but not used inGetInstance(). Whilesync.Once provides thread-safe initialization, there's no synchronization betweenGetInstance() readinginstance (line 66) andResetInstance() writing to it (line 78).
This creates a data race: concurrent calls toGetInstance() andResetInstance() will race on theinstance variable. To fix this,GetInstance() should acquireinstanceMu.RLock() before returning the instance to synchronize withResetInstance()'s write lock.
| }) | |
| }) | |
| instanceMu.RLock() | |
| deferinstanceMu.RUnlock() |
| } | ||
| // newRepoAccessCache creates a new cache instance. This is a private helper function | ||
| // used by GetInstance. |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comment states this is "used by GetInstance" but it's also called byNewRepoAccessCache() (line 89). The comment should be updated to reflect that this is a shared helper used by both the singleton and non-singleton constructors.
| // used by GetInstance. | |
| // used bybothGetInstance (singleton constructor) and NewRepoAccessCache (non-singleton constructor). |
Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>
* Apply lockdown mode to issues and pull requests* Add cache* Unlock in defer* Add muesli/cache2go* [WIP] Replace custom cache in lockdown.go with cache2go struct (#1425)* Initial plan* Replace custom cache with cache2go library- Added github.com/muesli/cache2go dependency- Replaced custom map-based cache with cache2go.CacheTable- Removed manual timer management (scheduleExpiry, ensureEntry methods)- Removed timer field from repoAccessCacheEntry struct- Updated GetRepoAccessInfo to use cache2go's Value() and Add() methods- Updated SetTTL to flush and re-add entries with new TTL- Used unique cache names per instance to avoid test interference- All existing tests pass with the new implementationCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Final verification completeCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>---------Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Use muesli for cache* Make RepoAccessCache a singleton (#1426)* Initial plan* Implement RepoAccessCache as a singleton patternCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Complete singleton implementation and verificationCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Remove cacheIDCounter as requestedCo-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>---------Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>Co-authored-by: JoannaaKL <67866556+JoannaaKL@users.noreply.github.com>* Update mutexes* .* Reuse cache* .* .* Fix logic after vibe coding* Update docs* .* Refactoring to make the code pretty* Hide lockdown logic behind shouldFilter function* .* Tests---------Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
Closes: #[issue_number]
Description
The
RepoAccessCachewas being instantiated multiple times across the application, leading to inconsistent configuration (TTL, logger) and redundant initialization overhead.Changes
GetInstance()withsync.Oncefor thread-safe lazy initialization. Production code now uses a single shared cache instance.NewRepoAccessCache()to create independent instances for parallel tests, avoiding test interference.Usage
Tradeoffs
GetInstance()ignores client/options on subsequent calls. First initialization wins. This is standard singleton behavior but could surprise callers expecting reconfiguration.ResetInstance()is unsafe if cache is in use. Documented for test-only usage.Created from VS Code via theGitHub Pull Request extension.
Original prompt
Created from VS Code via theGitHub Pull Request extension.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn moreCopilot coding agent tips in the docs.