mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
🎛️ refactor: Scope App-Config Override Cache by Isolation Context (#13455)
getAppConfig caches per-principal merged config overrides under a key built by overrideCacheKey(role, userId, tenantId). The key used the tenantId *argument* only — but callers that go through the tenant middleware (the common path) pass no explicit tenantId and rely on the AsyncLocalStorage tenant context. Those calls were keyed under the shared '__default__' bucket, so the DB query (correctly scoped to the ALS tenant by the Mongoose plugin) produced a merged config that was then cached and served to the next tenant resolving the same role/user — leaking model specs, endpoints, and interface flags across tenants. Fall back to getTenantId() before '__default__' so the cache key reflects the actual tenant scope (param or ALS). Tighten the strict-mode warning to fire only when there is genuinely no tenant anywhere (param nor ALS), since the ALS case is now scoped rather than defaulted. No-op for single-tenant deployments, where getTenantId() is undefined and the key stays '__default__'. Adds tests (real Map-backed cache) proving the ALS tenant scopes the key and that two tenants resolving the same role each get their own config with no cache collision.
This commit is contained in:
parent
b483feae8b
commit
983a33fbad
2 changed files with 55 additions and 7 deletions
|
|
@ -327,6 +327,51 @@ describe('createAppConfigService', () => {
|
|||
|
||||
expect(deps.getApplicableConfigs).toHaveBeenCalledWith([]);
|
||||
});
|
||||
|
||||
it('scopes the override cache key to the ALS tenant when no tenantId param is given', async () => {
|
||||
const { tenantStorage } = jest.requireActual('@librechat/data-schemas');
|
||||
const deps = createDeps({
|
||||
getApplicableConfigs: jest
|
||||
.fn()
|
||||
.mockResolvedValue([{ priority: 10, overrides: { x: 1 }, isActive: true }]),
|
||||
});
|
||||
const { getAppConfig } = createAppConfigService(deps);
|
||||
|
||||
await tenantStorage.run({ tenantId: 'tenant-a' }, async () =>
|
||||
getAppConfig({ role: 'USER' }),
|
||||
);
|
||||
|
||||
const overrideKey = [...deps._cache._store.keys()].find((k: string) =>
|
||||
k.includes('_OVERRIDE_:'),
|
||||
);
|
||||
expect(overrideKey).toBe('app_config:_OVERRIDE_:tenant-a:USER');
|
||||
expect(overrideKey).not.toContain('__default__');
|
||||
});
|
||||
|
||||
it('does not serve one tenant a cached config built for another tenant', async () => {
|
||||
const { tenantStorage, getTenantId } = jest.requireActual('@librechat/data-schemas');
|
||||
// Each tenant's DB overrides carry a marker derived from the active ALS tenant.
|
||||
const deps = createDeps({
|
||||
getApplicableConfigs: jest
|
||||
.fn()
|
||||
.mockImplementation(async () => [
|
||||
{ priority: 10, overrides: { whoami: getTenantId() }, isActive: true },
|
||||
]),
|
||||
});
|
||||
const { getAppConfig } = createAppConfigService(deps);
|
||||
|
||||
const configA = (await tenantStorage.run({ tenantId: 'tenant-a' }, async () =>
|
||||
getAppConfig({ role: 'USER' }),
|
||||
)) as TestConfig & { whoami?: string };
|
||||
const configB = (await tenantStorage.run({ tenantId: 'tenant-b' }, async () =>
|
||||
getAppConfig({ role: 'USER' }),
|
||||
)) as TestConfig & { whoami?: string };
|
||||
|
||||
expect(configA.whoami).toBe('tenant-a');
|
||||
expect(configB.whoami).toBe('tenant-b');
|
||||
// A cache collision would short-circuit the second tenant's DB read.
|
||||
expect(deps.getApplicableConfigs).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
||||
it('does not cache on buildPrincipals error — retries on next request', async () => {
|
||||
|
|
|
|||
|
|
@ -73,7 +73,10 @@ export function _resetOverrideStrictCache(): void {
|
|||
}
|
||||
|
||||
function overrideCacheKey(role?: string, userId?: string, tenantId?: string): string {
|
||||
const tenant = tenantId || '__default__';
|
||||
// Fall back to the ALS tenant context before `__default__`: callers that rely on the
|
||||
// tenant middleware (the common path) pass no explicit tenantId, so without this the
|
||||
// entry is keyed under the shared `__default__` bucket and leaks across tenants.
|
||||
const tenant = tenantId || getTenantId() || '__default__';
|
||||
if (userId && role) {
|
||||
return `_OVERRIDE_:${tenant}:${role}:${userId}`;
|
||||
}
|
||||
|
|
@ -174,16 +177,16 @@ export function createAppConfigService(deps: AppConfigServiceDeps) {
|
|||
return baseConfig;
|
||||
}
|
||||
|
||||
// Strict-isolation + no tenant (param or ALS) = pathological path (middleware bypass or
|
||||
// unauthenticated startup). Pre-tenant calls use baseOnly:true; admin calls carry tenantId.
|
||||
// If ALS has a tenant, Mongoose scopes queries to that tenant's overrides — must fall through.
|
||||
// Not cached: the cache key doesn't include ALS context, so a cached __default__ entry would
|
||||
// be served to later ALS-scoped calls that share the same param-derived key.
|
||||
// Strict isolation + no tenant anywhere (neither param nor ALS) is pathological: a
|
||||
// middleware bypass or an unauthenticated startup call. Pre-tenant calls should use
|
||||
// baseOnly:true and admin calls carry an explicit tenantId. Return the base config
|
||||
// without caching it under the shared `__default__` bucket. When ALS has a tenant,
|
||||
// overrideCacheKey scopes the key to it, so we fall through and cache per-tenant.
|
||||
if (principals.length === 0 && !tenantId && !getTenantId() && isStrictOverrideMode()) {
|
||||
return baseConfig;
|
||||
}
|
||||
|
||||
if (!tenantId && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) {
|
||||
if (!tenantId && !getTenantId() && isStrictOverrideMode() && !_warnedNoTenantInStrictMode) {
|
||||
_warnedNoTenantInStrictMode = true;
|
||||
logger.warn(
|
||||
'[getAppConfig] No tenantId in strict mode — falling back to __default__. ' +
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue