Conversation
…cache - Add GITLAB_INSTANCE_CACHE_MAX (default: 100) and GITLAB_INSTANCE_TTL_MS (default: 1h) env vars to cap per-URL InstanceState memory - Implement evictExpired(): removes entries idle beyond TTL on each insert - Implement evictLRUIfOverCapacity(): removes oldest-accessed entry when over the capacity limit, also triggered on each insert - Both eviction paths skip currentInstanceUrl and entries with in-flight initialize/introspection promises to prevent orphaned callers - touchInstance() updates LRU timestamp on every resolveState() read - dropInstance() removes entries from both instances and instanceAccessTimes - Static MAX_INSTANCES/INSTANCE_TTL_MS use Number.isFinite guard for safety against partial test mocks that omit the new config constants - 13 new unit tests covering: TTL expiry, LRU ordering, multi-evict, currentInstanceUrl protection, in-flight promise protection, touch path, and reset cleanup - Update README.md.in with new env var documentation Closes #376
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-URL InstanceState access tracking and bounded in-memory cache with LRU and idle-TTL eviction in ConnectionManager; introduces two new env-config constants to control capacity and TTL; updates read/initialize/reset paths to touch/protect entries; tests expanded to cover eviction behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CM as ConnectionManager
participant Instances as InstancesMap
participant Introspect as IntrospectionCache
Client->>CM: initialize(url)
CM->>Instances: lookup or create InstanceState
CM->>CM: touchInstance(url)
CM->>CM: evictExpired()
CM->>CM: evictLRUIfOverCapacity()
alt state exists
CM->>Client: return client
Client->>CM: calls getClient(url)
CM->>Instances: resolveState(url)
CM->>CM: touchInstance(url)
else initialization in-flight
CM->>Client: await initializePromises[url]
end
CM->>Introspect: drop related introspection keys on dropInstance(url)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Test Coverage ReportOverall Coverage: 96.5%
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/services/ConnectionManager.test.ts (1)
67-69:⚠️ Potential issue | 🟡 Minor
injectWithTime()undoes the "third-party current URL" setup.
injectInstanceState()resetscurrentInstanceUrlto the injected URL at Lines 67-69, so every call frominjectWithTime()overwrites the value established inbeforeEach(). That means the last injected entry is still protected by the eviction guards, and several tests are not exercising the unprotected TTL/LRU path their comments describe.Proposed fix
function injectWithTime( url: string, accessedAt: number, overrides: Partial<InstanceState> = {}, ): void { + const preservedCurrent = internals(manager).currentInstanceUrl; injectInstanceState(manager, url, { isInitialized: true, ...overrides }); internals(manager).instanceAccessTimes.set(url, accessedAt); + internals(manager).currentInstanceUrl = preservedCurrent; }Also applies to: 434-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/services/ConnectionManager.test.ts` around lines 67 - 69, injectInstanceState() is overwriting the test's "third-party current URL" by unconditionally setting internals(manager).currentInstanceUrl, which causes injectWithTime() calls to undo the beforeEach() setup; change injectInstanceState() so it does not reset internals(manager).currentInstanceUrl unless explicitly required (e.g., add an optional parameter setCurrent:boolean defaulting to false, or only set currentInstanceUrl when it is currently undefined), and apply the same change to the other occurrence of injectInstanceState() used around the 434-448 region so that injectWithTime() no longer overwrites the test-established currentInstanceUrl.src/services/ConnectionManager.ts (1)
165-170:⚠️ Potential issue | 🟠 MajorRefresh access timestamps on every successful cache hit.
touchInstance()currently only runs on insert and throughresolveState(). Theinitialize()cache-hit path, the cached early-return inensureIntrospected(),getInstanceClient(), and the direct getters at Lines 666-716 all return live state without updatinginstanceAccessTimes. In multi-instance OAuth traffic, those are hot paths, so a recently used instance can still look idle and be evicted on the next insert.Also applies to: 429-430, 574-575, 612-619, 666-716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/ConnectionManager.ts` around lines 165 - 170, Cache HIT paths are not updating instanceAccessTimes, so call touchInstance(...) on every successful cache hit to refresh access timestamps: in ConnectionManager.initialize() where you early-return after finding existing?.isInitialized, invoke this.touchInstance(url) before setting this.currentInstanceUrl and returning; likewise add this.touchInstance(url) at the cached early-return in ensureIntrospected(), inside getInstanceClient() on the fast-return of a live instance, and at every direct getter that returns live instance state (the getters that currently return instances without mutation). Use the existing touchInstance method/signature so cache eviction sees recent activity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/ConnectionManager.ts`:
- Around line 95-98: dropInstance currently deletes live maps but leaves
ConnectionManager.introspectionCache entries for that URL, allowing unbounded
memory growth; update dropInstance (and the other instance-teardown sites
referenced) to also remove the introspectionCache entries for both the raw URL
key and the introspection endpoint key (`url` and `${url}/api/graphql`), or
alternatively implement a TTL/LRU eviction for
ConnectionManager.introspectionCache and ensure teardown paths call that
eviction; locate and modify the dropInstance method and the other
instance-removal spots (the blocks that call this.instances.delete /
this.instanceAccessTimes.delete) to call introspectionCache.delete(key) for both
keys (or invoke the new TTL/LRU cleanup) so schema payloads are removed when an
instance is dropped.
---
Outside diff comments:
In `@src/services/ConnectionManager.ts`:
- Around line 165-170: Cache HIT paths are not updating instanceAccessTimes, so
call touchInstance(...) on every successful cache hit to refresh access
timestamps: in ConnectionManager.initialize() where you early-return after
finding existing?.isInitialized, invoke this.touchInstance(url) before setting
this.currentInstanceUrl and returning; likewise add this.touchInstance(url) at
the cached early-return in ensureIntrospected(), inside getInstanceClient() on
the fast-return of a live instance, and at every direct getter that returns live
instance state (the getters that currently return instances without mutation).
Use the existing touchInstance method/signature so cache eviction sees recent
activity.
In `@tests/unit/services/ConnectionManager.test.ts`:
- Around line 67-69: injectInstanceState() is overwriting the test's
"third-party current URL" by unconditionally setting
internals(manager).currentInstanceUrl, which causes injectWithTime() calls to
undo the beforeEach() setup; change injectInstanceState() so it does not reset
internals(manager).currentInstanceUrl unless explicitly required (e.g., add an
optional parameter setCurrent:boolean defaulting to false, or only set
currentInstanceUrl when it is currently undefined), and apply the same change to
the other occurrence of injectInstanceState() used around the 434-448 region so
that injectWithTime() no longer overwrites the test-established
currentInstanceUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa46e6ae-1518-4d9c-a89d-2281e48402b8
📒 Files selected for processing (5)
README.md.insrc/config.tssrc/services/ConnectionManager.tstests/unit/services/ConnectionManager.test.tstests/unit/services/ConnectionManagerEnhanced.test.ts
…cache-hit paths - dropInstance now removes both introspectionCache keys (bare URL and /api/graphql suffix) preventing schema payload accumulation for evicted instances - touchInstance called on every path that bypasses resolveState: initialize() early-return, ensureIntrospected() cache hit, getInstanceClient() fallback, and all six direct-getter methods (isConnected, isFeatureAvailable, getTier, getVersion, isWidgetAvailable, getTokenScopeInfo) - injectWithTime() preserves currentInstanceUrl from beforeEach so eviction tests exercise the unprotected path
Member
Author
|
@coderabbitai Re outside-diff items in latest review. (1) tests/unit/services/ConnectionManager.test.ts:67-69 injectWithTime undoes currentInstanceUrl: Fixed - injectWithTime() now saves and restores currentInstanceUrl around the injectInstanceState() call. (2) src/services/ConnectionManager.ts:165-170 cache-hit paths not touching instanceAccessTimes: Fixed - added touchInstance(url) to initialize() early-return, ensureIntrospected() cache-hit, getInstanceClient() fallback, and all six direct-getter methods. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me quickly verify both fixes in the PR branch. 🏁 Script executed: Length of output: 6091
Both look correct — thanks for the thorough follow-up! ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/ConnectionManager.ts`:
- Around line 67-83: Mark the instanceAccessTimes field as readonly because it's
never reassigned (only mutated via .set/.delete/.clear) to satisfy SonarCloud;
keep the static fields MAX_INSTANCES and INSTANCE_TTL_MS unchanged (they must
remain mutable for tests that override them via statics()). Locate the
declarations instanceAccessTimes, MAX_INSTANCES, and INSTANCE_TTL_MS in
ConnectionManager and change only instanceAccessTimes to readonly while leaving
the two static properties as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08e4b8ea-8466-41b9-8bc4-a0646b8b781a
📒 Files selected for processing (2)
src/services/ConnectionManager.tstests/unit/services/ConnectionManager.test.ts
There was a problem hiding this comment.
Pull request overview
Adds bounded-memory behavior to ConnectionManager.instances to prevent unbounded growth in high-cardinality OAuth multi-tenant scenarios by evicting idle entries (TTL) and least-recently-used entries (LRU), with configuration exposed via new environment variables.
Changes:
- Implement TTL expiry + LRU eviction for cached per-instance connection state in
ConnectionManager. - Add config/env support for
GITLAB_INSTANCE_CACHE_MAXandGITLAB_INSTANCE_TTL_MS. - Add/extend unit tests covering eviction behavior and access-time tracking; document the new env vars.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/services/ConnectionManager.ts |
Adds access-time tracking plus TTL/LRU eviction and integrates “touch” updates on multiple read/insert paths. |
src/config.ts |
Introduces parsed configuration for cache capacity and TTL with safe defaults. |
tests/unit/services/ConnectionManager.test.ts |
Adds comprehensive unit tests for TTL expiry, LRU eviction, protection rules, and reset behavior. |
tests/unit/services/ConnectionManagerEnhanced.test.ts |
Updates config mock to include new exported config constants used by the service. |
README.md.in |
Documents the two new environment variables and defaults. |
…reIntrospected paths - instanceAccessTimes is never reassigned (only .set/.delete/.clear), so readonly satisfies SonarCloud without affecting test mutability - Move touchInstance() before cache-hit check in ensureIntrospected() so LRU timestamp is refreshed on every call path (cache-hit, await existing introspection, and new introspection start)
|
polaz
deleted the
feat/#376-feat-add-ttllru-eviction-to-connectionmanager-per
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ConnectionManager.instancesMap in high-cardinality multi-instance (OAuth SaaS) scenariosGITLAB_INSTANCE_CACHE_MAX(default: 100) andGITLAB_INSTANCE_TTL_MS(default: 1h)Technical Details
Problem
In OAuth multi-tenant mode, every unique GitLab URL creates a new
InstanceStateentry inConnectionManager.instances. Without a bound, the map grows without limit — in a long-running SaaS server serving many tenants this is a memory leak.Implementation
evictExpired()— called on every insert; removes entries whoselastAccessedAtexceedsINSTANCE_TTL_MS. Skip rules:currentInstanceUrland any URL with an in-flightinitializePromisesorintrospectionPromisesentry (evicting those would orphan awaiting callers).evictLRUIfOverCapacity()— called afterevictExpired(); removes the least-recently-used (oldestlastAccessedAt) entry untilinstances.size <= MAX_INSTANCES. Same skip rules apply.touchInstance(url)— recordsDate.now()intoinstanceAccessTimeson everyresolveState()call (all read paths) and on initial insertion.dropInstance(url)— atomically removes from bothinstancesandinstanceAccessTimes.Static guards —
MAX_INSTANCESandINSTANCE_TTL_MSuseNumber.isFinite()fallbacks to stay safe against partial test mocks.Test Plan
currentInstanceUrlprotection, in-flight promise protection,touchInstanceon read,reset()cleanup