🧵 feat: ALS Context Middleware, Tenant Threading, and Config Cache Invalidation#12407
Merged
Conversation
danny-avila
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Activates ALS-based tenant context propagation for request-scoped Mongoose tenant isolation, threads tenantId through config resolution call sites, and adds cache invalidation hooks after admin config mutations to keep per-tenant configuration consistent.
Changes:
- Introduces
tenantContextMiddlewareto propagatereq.user.tenantIdintoAsyncLocalStorage(with strict-mode 403 behavior). - Threads
tenantIdintogetAppConfigcallers across API services/controllers and wraps pre-auth/startup config reads inrunAsSystem()for strict-mode compatibility. - Adds per-tenant override-cache clearing and a higher-level
invalidateConfigCaches()invoked after admin config mutations.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-schemas/src/methods/userGroup.ts | Updates commentary to rely on ALS + tenant isolation plugin for group membership scoping. |
| packages/api/src/middleware/tenant.ts | Adds Express middleware to set ALS tenant context; supports strict-mode enforcement. |
| packages/api/src/middleware/index.ts | Re-exports the new tenant middleware. |
| packages/api/src/middleware/balance.ts | Passes tenantId into getAppConfig to keep per-tenant config caching correct. |
| packages/api/src/middleware/tests/tenant.spec.ts | Adds unit tests for ALS propagation and strict-mode behavior. |
| packages/api/src/app/service.ts | Adds clearOverrideCache(tenantId?) and warning on missing tenantId in strict mode. |
| packages/api/src/app/service.spec.ts | Adds tests for clearOverrideCache behavior (all tenants / scoped / base preserved). |
| packages/api/src/admin/config.ts | Scopes /base config retrieval by tenant and triggers cache invalidation after mutations. |
| api/strategies/socialLogin.js | Wraps getAppConfig() in runAsSystem() for strict-mode compatibility. |
| api/strategies/samlStrategy.js | Wraps getAppConfig() in runAsSystem() for strict-mode compatibility. |
| api/strategies/openidStrategy.js | Wraps getAppConfig() in runAsSystem() for strict-mode compatibility. |
| api/strategies/ldapStrategy.js | Wraps getAppConfig() in runAsSystem() for strict-mode compatibility. |
| api/server/services/MCP.js | Threads tenantId into getAppConfig for per-tenant MCP allowlist decisions. |
| api/server/services/Files/Audio/getVoices.js | Threads tenantId into getAppConfig. |
| api/server/services/Files/Audio/getCustomConfigSpeech.js | Threads tenantId into getAppConfig. |
| api/server/services/Files/Audio/TTSService.js | Threads tenantId into getAppConfig. |
| api/server/services/Files/Audio/STTService.js | Threads tenantId into getAppConfig. |
| api/server/services/Config/loadDefaultModels.js | Threads tenantId into getAppConfig. |
| api/server/services/Config/loadConfigModels.js | Threads tenantId into getAppConfig. |
| api/server/services/Config/getEndpointsConfig.js | Threads tenantId into getAppConfig for endpoint config construction. |
| api/server/services/Config/app.js | Exposes invalidateConfigCaches() and wires it to clear multiple related caches. |
| api/server/services/AuthService.js | Wraps pre-auth getAppConfig() calls in runAsSystem(). |
| api/server/routes/config.js | Threads tenantId into getAppConfig for startup config payload. |
| api/server/routes/admin/config.js | Injects invalidateConfigCaches into admin config handlers. |
| api/server/middleware/checkDomainAllowed.js | Threads tenantId into getAppConfig for domain allow checks. |
| api/server/index.js | Mounts tenant middleware; wraps startup initialization and post-listen init in runAsSystem(). |
| api/server/controllers/UserController.js | Threads tenantId into getAppConfig. |
| api/server/controllers/PluginController.js | Threads tenantId into getAppConfig for tool/plugin resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fd4cf41c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 727f878144
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+202
to
+206
| if (!store || typeof store.keys !== 'function') { | ||
| logger.warn( | ||
| '[clearOverrideCache] Cache store does not support key enumeration (e.g. Redis). ' + | ||
| 'Override caches will not be cleared — they will expire naturally via TTL.', | ||
| ); |
There was a problem hiding this comment.
clearOverrideCache exits when store.keys is unavailable and explicitly treats Redis as that case, so override entries are not actually invalidated in Redis-backed caches. Since admin mutations now depend on this function via invalidateConfigCaches, Redis deployments will continue serving stale _OVERRIDE_ configs until TTL expiry after successful config updates.
Useful? React with 👍 / 👎.
danny-avila
changed the title
danny-avila
changed the title
danny-avila
force-pushed
the
feat/multi-tenancy-followups
branch
2 times, most recently
from
March 26, 2026 20:50
7fdafb2 to
9cd7cce
Compare
Introduces tenantContextMiddleware that propagates req.user.tenantId into AsyncLocalStorage, activating the Mongoose applyTenantIsolation plugin for all downstream DB queries within a request. - Strict mode (TENANT_ISOLATION_STRICT=true) returns 403 if no tenantId - Non-strict mode passes through for backward compatibility - No-op for unauthenticated requests - Includes 6 unit tests covering all paths
- Register tenantContextMiddleware in Express app after capability middleware - Wrap server startup initialization in runAsSystem() for strict mode compat - Wrap auth strategy getAppConfig() calls in runAsSystem() since they run before user context is established (LDAP, SAML, OpenID, social login, AuthService)
Pass tenantId from req.user to getAppConfig() across all callers that have request context, ensuring correct per-tenant cache key resolution. Also fixes getBaseConfig admin endpoint to scope to requesting admin's tenant instead of returning the unscoped base config. Files updated: - Controllers: UserController, PluginController - Middleware: checkDomainAllowed, balance - Routes: config - Services: loadConfigModels, loadDefaultModels, getEndpointsConfig, MCP - Audio services: TTSService, STTService, getVoices, getCustomConfigSpeech - Admin: getBaseConfig endpoint
- Add clearOverrideCache(tenantId?) to flush per-principal override caches by enumerating Keyv store keys matching _OVERRIDE_: prefix - Add invalidateConfigCaches() helper that clears base config, override caches, tool caches, and endpoint config cache in one call - Wire invalidation into all 5 admin config mutation handlers (upsert, patch, delete field, delete overrides, toggle active) - Add strict mode warning when __default__ tenant fallback is used - Add 3 new tests for clearOverrideCache (all/scoped/base-preserving)
…iltering The TODO(#12091) about missing tenantId filtering is resolved by the tenant context middleware + applyTenantIsolation Mongoose plugin. Group queries are now automatically scoped by tenantId via ALS.
App configs are tenant-owned — runAsSystem() would bypass tenant
isolation and return cross-tenant DB overrides. Instead, add
baseOnly option to getAppConfig() that returns YAML-derived config
only, with zero DB queries.
All startup code, auth strategies, and MCP initialization now use
getAppConfig({ baseOnly: true }) to get the YAML config without
touching the Config collection.
…afety - Chain tenantContextMiddleware inside requireJwtAuth after passport auth instead of global app.use() where req.user is always undefined (Finding 1) - Remove global tenantContextMiddleware registration from index.js - Update BalanceMiddlewareOptions to include tenantId, remove redundant cast (Finding 4) - Add warning log when clearOverrideCache cannot enumerate keys on Redis (Finding 3) - Use startsWith instead of includes for cache key filtering (Finding 12) - Use generator loop instead of Array.from for key enumeration (Finding 3) - Selective barrel export — exclude _resetTenantMiddlewareStrictCache (Finding 5) - Move isMainThread check to module level, remove per-request check (Finding 9) - Move mid-file require to top of app.js (Finding 8) - Parallelize invalidateConfigCaches with Promise.all (Finding 10) - Remove clearOverrideCache from public app.js exports (internal only) - Strengthen getUserPrincipals comment re: ALS dependency (Finding 2)
…rify baseOnly
- Restore runAsSystem() around performStartupChecks, updateInterfacePermissions,
initializeMCPs, and initializeOAuthReconnectManager — these make Mongoose
queries that need system context in strict tenant mode (NEW-3)
- Consolidate duplicate require('@librechat/api') in requireJwtAuth.js (NEW-1)
- Document that baseOnly ignores role/userId/tenantId in JSDoc (NEW-2)
- requireJwtAuth: 5 tests verifying ALS tenant context is set after passport auth, isolated between concurrent requests, and not set when user has no tenantId (Finding 6) - invalidateConfigCaches: 4 tests verifying all four caches are cleared, tenantId is threaded through, partial failure is handled gracefully, and operations run in parallel via Promise.all (Finding 11)
… /base scoping - Forward passport errors in requireJwtAuth before entering tenant middleware — prevents silent auth failures from reaching handlers (P1) - Account for Keyv namespace prefix in clearOverrideCache — stored keys are namespaced as "APP_CONFIG:_OVERRIDE_:..." not "_OVERRIDE_:...", so override caches were never actually matched/cleared (P2) - Remove role from getBaseConfig — /base should return tenant-scoped base config, not role-merged config that drifts per admin role (P2) - Return tenantStorage.run() for cleaner async semantics - Update mock cache in service.spec.ts to simulate Keyv namespacing
…lity - Decouple cache invalidation from mutation response: fire-and-forget with logging so DB mutation success is not masked by cache failures - Extract clearEndpointConfigCache helper from inline IIFE - Move isMainThread check to lazy once-per-process guard (no import side effect) - Memoize process.env read in overrideCacheKey to avoid per-request env lookups and log flooding in strict mode - Remove flaky timer-based parallelism assertion, use structural check - Merge orphaned double JSDoc block on getUserPrincipals - Fix stale [getAppConfig] log prefix → [ensureBaseConfig] - Fix import order in tenant.spec.ts (package types before local values) - Replace "Finding 1" reference with self-contained description - Use real tenantStorage primitives in requireJwtAuth spec mock
danny-avila
force-pushed
the
feat/multi-tenancy-followups
branch
from
March 26, 2026 20:55
9cd7cce to
0987607
Compare
Redis SCAN causes 60s+ stalls under concurrent load (see #12410). APP_CONFIG defaults to FORCED_IN_MEMORY_CACHE_NAMESPACES, so the in-memory store.keys() path handles the standard case. When APP_CONFIG is Redis-backed, overrides expire naturally via overrideCacheTtl (60s default) — an acceptable window for admin config mutations.
danny-avila
force-pushed
the
feat/multi-tenancy-followups
branch
from
March 26, 2026 20:58
0987607 to
8059cd1
Compare
danny-avila
force-pushed
the
feat/multi-tenancy-followups
branch
from
March 26, 2026 21:02
79beeb8 to
ffbe8e1
Compare
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbe8e103e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
Re: Codex P1 comments on This is intentional. Auth strategies run outside the tenant ALS context — passport executes before Registration flows: The user doesn't exist yet — there's no tenant identity to scope against. Login flows (follow-up): Once the user is found during login (LDAP bind, OAuth callback, SAML assertion), we have This is tracked as a follow-up: "Tenant-scoped app config during auth login flows." |
…lity
- Switch invalidateConfigCaches from Promise.all to Promise.allSettled
so partial failures are logged individually instead of producing one
undifferentiated error (Finding 3)
- Gate overrideCacheKey strict-mode warning behind a once-per-process
flag to prevent log flooding under load (Finding 4)
- Add test for passport error forwarding in requireJwtAuth — the
if (err) { return next(err) } branch now has coverage (Finding 5)
- Add test for real partial failure in invalidateConfigCaches where
clearAppConfigCache rejects (not just the swallowed endpoint error)
- Moved logger and runAsSystem imports to maintain a consistent import order across files. - Improved code readability by ensuring related imports are grouped together.
jcbartle
pushed a commit
to jcbartle/LibreChat
that referenced
this pull request
May 11, 2026
…validation (danny-avila#12407) * feat: add tenant context middleware for ALS-based isolation Introduces tenantContextMiddleware that propagates req.user.tenantId into AsyncLocalStorage, activating the Mongoose applyTenantIsolation plugin for all downstream DB queries within a request. - Strict mode (TENANT_ISOLATION_STRICT=true) returns 403 if no tenantId - Non-strict mode passes through for backward compatibility - No-op for unauthenticated requests - Includes 6 unit tests covering all paths * feat: register tenant middleware and wrap startup/auth in runAsSystem() - Register tenantContextMiddleware in Express app after capability middleware - Wrap server startup initialization in runAsSystem() for strict mode compat - Wrap auth strategy getAppConfig() calls in runAsSystem() since they run before user context is established (LDAP, SAML, OpenID, social login, AuthService) * feat: thread tenantId through all getAppConfig callers Pass tenantId from req.user to getAppConfig() across all callers that have request context, ensuring correct per-tenant cache key resolution. Also fixes getBaseConfig admin endpoint to scope to requesting admin's tenant instead of returning the unscoped base config. Files updated: - Controllers: UserController, PluginController - Middleware: checkDomainAllowed, balance - Routes: config - Services: loadConfigModels, loadDefaultModels, getEndpointsConfig, MCP - Audio services: TTSService, STTService, getVoices, getCustomConfigSpeech - Admin: getBaseConfig endpoint * feat: add config cache invalidation on admin mutations - Add clearOverrideCache(tenantId?) to flush per-principal override caches by enumerating Keyv store keys matching _OVERRIDE_: prefix - Add invalidateConfigCaches() helper that clears base config, override caches, tool caches, and endpoint config cache in one call - Wire invalidation into all 5 admin config mutation handlers (upsert, patch, delete field, delete overrides, toggle active) - Add strict mode warning when __default__ tenant fallback is used - Add 3 new tests for clearOverrideCache (all/scoped/base-preserving) * chore: update getUserPrincipals comment to reflect ALS-based tenant filtering The TODO(danny-avila#12091) about missing tenantId filtering is resolved by the tenant context middleware + applyTenantIsolation Mongoose plugin. Group queries are now automatically scoped by tenantId via ALS. * fix: replace runAsSystem with baseOnly for pre-tenant code paths App configs are tenant-owned — runAsSystem() would bypass tenant isolation and return cross-tenant DB overrides. Instead, add baseOnly option to getAppConfig() that returns YAML-derived config only, with zero DB queries. All startup code, auth strategies, and MCP initialization now use getAppConfig({ baseOnly: true }) to get the YAML config without touching the Config collection. * fix: address PR review findings — middleware ordering, types, cache safety - Chain tenantContextMiddleware inside requireJwtAuth after passport auth instead of global app.use() where req.user is always undefined (Finding 1) - Remove global tenantContextMiddleware registration from index.js - Update BalanceMiddlewareOptions to include tenantId, remove redundant cast (Finding 4) - Add warning log when clearOverrideCache cannot enumerate keys on Redis (Finding 3) - Use startsWith instead of includes for cache key filtering (Finding 12) - Use generator loop instead of Array.from for key enumeration (Finding 3) - Selective barrel export — exclude _resetTenantMiddlewareStrictCache (Finding 5) - Move isMainThread check to module level, remove per-request check (Finding 9) - Move mid-file require to top of app.js (Finding 8) - Parallelize invalidateConfigCaches with Promise.all (Finding 10) - Remove clearOverrideCache from public app.js exports (internal only) - Strengthen getUserPrincipals comment re: ALS dependency (Finding 2) * fix: restore runAsSystem for startup DB ops, consolidate require, clarify baseOnly - Restore runAsSystem() around performStartupChecks, updateInterfacePermissions, initializeMCPs, and initializeOAuthReconnectManager — these make Mongoose queries that need system context in strict tenant mode (NEW-3) - Consolidate duplicate require('@librechat/api') in requireJwtAuth.js (NEW-1) - Document that baseOnly ignores role/userId/tenantId in JSDoc (NEW-2) * test: add requireJwtAuth tenant chaining + invalidateConfigCaches tests - requireJwtAuth: 5 tests verifying ALS tenant context is set after passport auth, isolated between concurrent requests, and not set when user has no tenantId (Finding 6) - invalidateConfigCaches: 4 tests verifying all four caches are cleared, tenantId is threaded through, partial failure is handled gracefully, and operations run in parallel via Promise.all (Finding 11) * fix: address Copilot review — passport errors, namespaced cache keys, /base scoping - Forward passport errors in requireJwtAuth before entering tenant middleware — prevents silent auth failures from reaching handlers (P1) - Account for Keyv namespace prefix in clearOverrideCache — stored keys are namespaced as "APP_CONFIG:_OVERRIDE_:..." not "_OVERRIDE_:...", so override caches were never actually matched/cleared (P2) - Remove role from getBaseConfig — /base should return tenant-scoped base config, not role-merged config that drifts per admin role (P2) - Return tenantStorage.run() for cleaner async semantics - Update mock cache in service.spec.ts to simulate Keyv namespacing * fix: address second review — cache safety, code quality, test reliability - Decouple cache invalidation from mutation response: fire-and-forget with logging so DB mutation success is not masked by cache failures - Extract clearEndpointConfigCache helper from inline IIFE - Move isMainThread check to lazy once-per-process guard (no import side effect) - Memoize process.env read in overrideCacheKey to avoid per-request env lookups and log flooding in strict mode - Remove flaky timer-based parallelism assertion, use structural check - Merge orphaned double JSDoc block on getUserPrincipals - Fix stale [getAppConfig] log prefix → [ensureBaseConfig] - Fix import order in tenant.spec.ts (package types before local values) - Replace "Finding 1" reference with self-contained description - Use real tenantStorage primitives in requireJwtAuth spec mock * fix: move JSDoc to correct function after clearEndpointConfigCache extraction * refactor: remove Redis SCAN from clearOverrideCache, rely on TTL expiry Redis SCAN causes 60s+ stalls under concurrent load (see danny-avila#12410). APP_CONFIG defaults to FORCED_IN_MEMORY_CACHE_NAMESPACES, so the in-memory store.keys() path handles the standard case. When APP_CONFIG is Redis-backed, overrides expire naturally via overrideCacheTtl (60s default) — an acceptable window for admin config mutations. * fix: remove return from tenantStorage.run to satisfy void middleware signature * fix: address second review — cache safety, code quality, test reliability - Switch invalidateConfigCaches from Promise.all to Promise.allSettled so partial failures are logged individually instead of producing one undifferentiated error (Finding 3) - Gate overrideCacheKey strict-mode warning behind a once-per-process flag to prevent log flooding under load (Finding 4) - Add test for passport error forwarding in requireJwtAuth — the if (err) { return next(err) } branch now has coverage (Finding 5) - Add test for real partial failure in invalidateConfigCaches where clearAppConfigCache rejects (not just the swallowed endpoint error) * chore: reorder imports in index.js and app.js for consistency - Moved logger and runAsSystem imports to maintain a consistent import order across files. - Improved code readability by ensuring related imports are grouped together.
ThomasVuNguyen
pushed a commit
to ThomasVuNguyen/LibreChat
that referenced
this pull request
Jul 15, 2026
…validation (danny-avila#12407) * feat: add tenant context middleware for ALS-based isolation Introduces tenantContextMiddleware that propagates req.user.tenantId into AsyncLocalStorage, activating the Mongoose applyTenantIsolation plugin for all downstream DB queries within a request. - Strict mode (TENANT_ISOLATION_STRICT=true) returns 403 if no tenantId - Non-strict mode passes through for backward compatibility - No-op for unauthenticated requests - Includes 6 unit tests covering all paths * feat: register tenant middleware and wrap startup/auth in runAsSystem() - Register tenantContextMiddleware in Express app after capability middleware - Wrap server startup initialization in runAsSystem() for strict mode compat - Wrap auth strategy getAppConfig() calls in runAsSystem() since they run before user context is established (LDAP, SAML, OpenID, social login, AuthService) * feat: thread tenantId through all getAppConfig callers Pass tenantId from req.user to getAppConfig() across all callers that have request context, ensuring correct per-tenant cache key resolution. Also fixes getBaseConfig admin endpoint to scope to requesting admin's tenant instead of returning the unscoped base config. Files updated: - Controllers: UserController, PluginController - Middleware: checkDomainAllowed, balance - Routes: config - Services: loadConfigModels, loadDefaultModels, getEndpointsConfig, MCP - Audio services: TTSService, STTService, getVoices, getCustomConfigSpeech - Admin: getBaseConfig endpoint * feat: add config cache invalidation on admin mutations - Add clearOverrideCache(tenantId?) to flush per-principal override caches by enumerating Keyv store keys matching _OVERRIDE_: prefix - Add invalidateConfigCaches() helper that clears base config, override caches, tool caches, and endpoint config cache in one call - Wire invalidation into all 5 admin config mutation handlers (upsert, patch, delete field, delete overrides, toggle active) - Add strict mode warning when __default__ tenant fallback is used - Add 3 new tests for clearOverrideCache (all/scoped/base-preserving) * chore: update getUserPrincipals comment to reflect ALS-based tenant filtering The TODO(danny-avila#12091) about missing tenantId filtering is resolved by the tenant context middleware + applyTenantIsolation Mongoose plugin. Group queries are now automatically scoped by tenantId via ALS. * fix: replace runAsSystem with baseOnly for pre-tenant code paths App configs are tenant-owned — runAsSystem() would bypass tenant isolation and return cross-tenant DB overrides. Instead, add baseOnly option to getAppConfig() that returns YAML-derived config only, with zero DB queries. All startup code, auth strategies, and MCP initialization now use getAppConfig({ baseOnly: true }) to get the YAML config without touching the Config collection. * fix: address PR review findings — middleware ordering, types, cache safety - Chain tenantContextMiddleware inside requireJwtAuth after passport auth instead of global app.use() where req.user is always undefined (Finding 1) - Remove global tenantContextMiddleware registration from index.js - Update BalanceMiddlewareOptions to include tenantId, remove redundant cast (Finding 4) - Add warning log when clearOverrideCache cannot enumerate keys on Redis (Finding 3) - Use startsWith instead of includes for cache key filtering (Finding 12) - Use generator loop instead of Array.from for key enumeration (Finding 3) - Selective barrel export — exclude _resetTenantMiddlewareStrictCache (Finding 5) - Move isMainThread check to module level, remove per-request check (Finding 9) - Move mid-file require to top of app.js (Finding 8) - Parallelize invalidateConfigCaches with Promise.all (Finding 10) - Remove clearOverrideCache from public app.js exports (internal only) - Strengthen getUserPrincipals comment re: ALS dependency (Finding 2) * fix: restore runAsSystem for startup DB ops, consolidate require, clarify baseOnly - Restore runAsSystem() around performStartupChecks, updateInterfacePermissions, initializeMCPs, and initializeOAuthReconnectManager — these make Mongoose queries that need system context in strict tenant mode (NEW-3) - Consolidate duplicate require('@librechat/api') in requireJwtAuth.js (NEW-1) - Document that baseOnly ignores role/userId/tenantId in JSDoc (NEW-2) * test: add requireJwtAuth tenant chaining + invalidateConfigCaches tests - requireJwtAuth: 5 tests verifying ALS tenant context is set after passport auth, isolated between concurrent requests, and not set when user has no tenantId (Finding 6) - invalidateConfigCaches: 4 tests verifying all four caches are cleared, tenantId is threaded through, partial failure is handled gracefully, and operations run in parallel via Promise.all (Finding 11) * fix: address Copilot review — passport errors, namespaced cache keys, /base scoping - Forward passport errors in requireJwtAuth before entering tenant middleware — prevents silent auth failures from reaching handlers (P1) - Account for Keyv namespace prefix in clearOverrideCache — stored keys are namespaced as "APP_CONFIG:_OVERRIDE_:..." not "_OVERRIDE_:...", so override caches were never actually matched/cleared (P2) - Remove role from getBaseConfig — /base should return tenant-scoped base config, not role-merged config that drifts per admin role (P2) - Return tenantStorage.run() for cleaner async semantics - Update mock cache in service.spec.ts to simulate Keyv namespacing * fix: address second review — cache safety, code quality, test reliability - Decouple cache invalidation from mutation response: fire-and-forget with logging so DB mutation success is not masked by cache failures - Extract clearEndpointConfigCache helper from inline IIFE - Move isMainThread check to lazy once-per-process guard (no import side effect) - Memoize process.env read in overrideCacheKey to avoid per-request env lookups and log flooding in strict mode - Remove flaky timer-based parallelism assertion, use structural check - Merge orphaned double JSDoc block on getUserPrincipals - Fix stale [getAppConfig] log prefix → [ensureBaseConfig] - Fix import order in tenant.spec.ts (package types before local values) - Replace "Finding 1" reference with self-contained description - Use real tenantStorage primitives in requireJwtAuth spec mock * fix: move JSDoc to correct function after clearEndpointConfigCache extraction * refactor: remove Redis SCAN from clearOverrideCache, rely on TTL expiry Redis SCAN causes 60s+ stalls under concurrent load (see danny-avila#12410). APP_CONFIG defaults to FORCED_IN_MEMORY_CACHE_NAMESPACES, so the in-memory store.keys() path handles the standard case. When APP_CONFIG is Redis-backed, overrides expire naturally via overrideCacheTtl (60s default) — an acceptable window for admin config mutations. * fix: remove return from tenantStorage.run to satisfy void middleware signature * fix: address second review — cache safety, code quality, test reliability - Switch invalidateConfigCaches from Promise.all to Promise.allSettled so partial failures are logged individually instead of producing one undifferentiated error (Finding 3) - Gate overrideCacheKey strict-mode warning behind a once-per-process flag to prevent log flooding under load (Finding 4) - Add test for passport error forwarding in requireJwtAuth — the if (err) { return next(err) } branch now has coverage (Finding 5) - Add test for real partial failure in invalidateConfigCaches where clearAppConfigCache rejects (not just the swallowed endpoint error) * chore: reorder imports in index.js and app.js for consistency - Moved logger and runAsSystem imports to maintain a consistent import order across files. - Improved code readability by ensuring related imports are grouped together.
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
Completes the multi-tenancy story started in #12354 by activating the ALS-based tenant isolation
infrastructure and wiring all outstanding follow-up items.
tenantContextMiddlewareinpackages/apithat propagatesreq.user.tenantIdintoAsyncLocalStorage, activating theapplyTenantIsolationMongoose plugin for all downstream DBqueries scoped to the request.
tenantContextMiddlewaredirectly insiderequireJwtAuthafter passport populatesreq.user, ensuring ALS context is set on every authenticated request without a globalapp.use()registration.baseOnlyoption togetAppConfigthat returns the YAML-derived config with zero DBqueries, used by all pre-tenant code paths (server startup, auth strategies, MCP initialization).
performStartupChecks,updateInterfacePermissions,initializeMCPs, andinitializeOAuthReconnectManagerinrunAsSystem()for strict-mode compatibility.tenantIdfromreq.userintogetAppConfigacross 14 call sites (controllers,middleware, routes, audio services, MCP services) to ensure correct per-tenant cache key
resolution.
GET /api/admin/config/baseendpoint to the requesting admin's tenant instead ofreturning an unscoped config.
clearOverrideCache(tenantId?)tocreateAppConfigService, which enumerates Keyv storekeys and deletes all matching
_OVERRIDE_:${tenantId}:*entries (with a documented TTL-fallbackwarning for Redis deployments that do not expose
keys()).invalidateConfigCaches(tenantId?)inapp.jsthat clears the base config, overridecaches, tool caches, and the
ENDPOINT_CONFIGcache entry in a singlePromise.all, wired asfire-and-forget (with error logging) into all five admin config mutation handlers.
tenantContextMiddlewarereturns 403 whenTENANT_ISOLATION_STRICT=trueand the authenticated user carries no
tenantId.TODO(#12091)ongetUserPrincipals— group membership queries are nowautomatically tenant-scoped via the ALS context set by
tenantContextMiddleware.strict/non-strict mode paths,
clearOverrideCachescoping, andinvalidateConfigCachesparallelism and partial-failure handling.
Change Type
Testing
New automated tests cover the core paths:
packages/api/src/middleware/__tests__/tenant.spec.ts(6 tests): ALS context set forauthenticated requests with
tenantId; no-op for unauthenticated requests; 403 returned instrict mode without
tenantId; concurrent requests receive independent ALS contexts.api/server/middleware/__tests__/requireJwtAuth.spec.js(5 tests): ALS tenant context ispresent inside
next()after passport auth; absent when user has notenantIdor is undefined;concurrent requests are isolated; top-level scope has no ALS context.
packages/api/src/app/service.spec.ts—clearOverrideCachesuite (3 tests): clears alloverride caches when no
tenantIdis provided; clears only the specified tenant's caches; doesnot clear the base config.
api/server/services/Config/__tests__/invalidateConfigCaches.spec.js(4 tests): all fourcaches are cleared;
tenantIdis forwarded toclearOverrideCache; aCONFIG_STOREfailuredoes not prevent other caches from clearing; all operations execute in parallel via
Promise.all.Test Configuration
Run per-workspace:
For manual strict-mode verification:
TENANT_ISOLATION_STRICT=truein.env.tenantId— expect403 { "error": "Tenant context required in strict isolation mode" }.PUT /api/admin/config/:type/:id) and confirm that asubsequent
GET /api/configreflects the updated config immediately (cache invalidated).Checklist