Bound dynamic-provider cache TTL by default + add invalidation API (#105)#106
Bound dynamic-provider cache TTL by default + add invalidation API (#105)#106heskew wants to merge 1 commit into
Conversation
) The dynamic-provider cache (for onResolveProvider-resolved providers) was never evicted under the default config: cacheDynamicProviders defaulted to true (Infinity), and nothing — backing-store writes, config reloads, login/logout — cleared it. Disabling, deleting, or rotating credentials on a dynamically-resolved provider therefore had no effect until process restart, and a revoked SSO config kept authenticating through the session-validation middleware. Two changes: - Default cacheDynamicProviders to a bounded 300s TTL instead of forever, so every worker thread re-resolves within the window. `true` still opts into forever. This matches the cache's documented "bounded window" intent. - Add a public DynamicProviderCache.delete() and export invalidateDynamicProvider()/clearDynamicProviderCache() so a consumer can evict immediately when its backing config changes. The cache is per-worker-thread, so explicit invalidation reaches only the calling thread; the bounded TTL is the cross-thread convergence mechanism (documented). Adds delete() unit tests and corrects the default-TTL test (was asserting "forever"; now asserts the bounded default expires past the window). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces module-level functions (invalidateDynamicProvider and clearDynamicProviderCache) to allow consumers to invalidate or clear dynamically-resolved providers from the in-memory cache. It also changes the default cache TTL from infinite (true) to a bounded 300 seconds to prevent serving stale data indefinitely. The feedback highlights a potential race condition when multiple plugin instances are active in the same thread, as a single activeDynamicProviderCache reference can be prematurely set to null upon closing one instance. The reviewer suggests using a Set of active caches to safely track and propagate invalidation requests across all active instances.
| // Active dynamic-provider cache for the loaded plugin instance, so consumers | ||
| // can invalidate entries when their backing config changes. Per-worker-thread: | ||
| // this references the cache in the thread that ran handleApplication. | ||
| let activeDynamicProviderCache: DynamicProviderCache | null = null; | ||
|
|
||
| /** | ||
| * Invalidate a single dynamically-resolved provider in the in-memory cache. | ||
| * Call this from application code after the backing config for `providerConfigId` | ||
| * changes (disabled, deleted, or credentials rotated) so the next request | ||
| * re-resolves it via the `onResolveProvider` hook instead of serving stale data. | ||
| * | ||
| * NOTE: the cache is per-worker-thread, so this evicts only in the thread that | ||
| * runs the call. Other threads converge within the configured TTL | ||
| * (`cacheDynamicProviders`, default {@link DEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS}s). | ||
| * For immediate cluster-wide effect, pair invalidation with a short TTL. | ||
| * | ||
| * @returns true if an entry was present and removed in this thread. | ||
| */ | ||
| export function invalidateDynamicProvider(providerConfigId: string): boolean { | ||
| return activeDynamicProviderCache?.delete(providerConfigId) ?? false; | ||
| } | ||
|
|
||
| /** | ||
| * Clear every dynamically-resolved provider from the in-memory cache (this | ||
| * worker thread only — see {@link invalidateDynamicProvider} for cross-thread notes). | ||
| */ | ||
| export function clearDynamicProviderCache(): void { | ||
| activeDynamicProviderCache?.clear(); | ||
| } |
There was a problem hiding this comment.
Using a single activeDynamicProviderCache reference can lead to issues when multiple plugin instances are active in the same thread. Specifically, if one instance is closed, it sets activeDynamicProviderCache to null, which completely disables invalidation for any other active instances.
Using a Set of active caches solves this race condition and ensures that invalidation requests are correctly propagated to all active instances.
const activeDynamicProviderCaches = new Set<DynamicProviderCache>();
/**
* Invalidate a single dynamically-resolved provider in the in-memory cache.
* Call this from application code after the backing config for `providerConfigId`
* changes (disabled, deleted, or credentials rotated) so the next request
* re-resolves it via the `onResolveProvider` hook instead of serving stale data.
*
* NOTE: the cache is per-worker-thread, so this evicts only in the thread that
* runs the call. Other threads converge within the configured TTL
* (`cacheDynamicProviders`, default {@link DEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS}s).
* For immediate cluster-wide effect, pair invalidation with a short TTL.
*
* @returns true if an entry was present and removed in this thread.
*/
export function invalidateDynamicProvider(providerConfigId: string): boolean {
let deleted = false;
for (const cache of activeDynamicProviderCaches) {
if (cache.delete(providerConfigId)) {
deleted = true;
}
}
return deleted;
}
/**
* Clear every dynamically-resolved provider from the in-memory cache (this
* worker thread only — see {@link invalidateDynamicProvider} for cross-thread notes).
*/
export function clearDynamicProviderCache(): void {
for (const cache of activeDynamicProviderCaches) {
cache.clear();
}
}| // Expose this thread's cache for consumer-driven invalidation | ||
| activeDynamicProviderCache = dynamicProviderCache; |
There was a problem hiding this comment.
Add the newly created cache to the set of active caches instead of overwriting a single reference.
| // Expose this thread's cache for consumer-driven invalidation | |
| activeDynamicProviderCache = dynamicProviderCache; | |
| // Expose this thread's cache for consumer-driven invalidation | |
| activeDynamicProviderCaches.add(dynamicProviderCache); |
| if (activeDynamicProviderCache === dynamicProviderCache) { | ||
| activeDynamicProviderCache = null; | ||
| } |
There was a problem hiding this comment.
Remove the cache from the set of active caches when the scope is closed.
activeDynamicProviderCaches.delete(dynamicProviderCache);
|
Reviewed; no blockers found. |
|
3 blockers found. 1. Documentation drift: Configuration defaultFile: docs/configuration.md:28 2. Documentation drift: Lifecycle hooks stale wordingFile: docs/lifecycle-hooks.md:147 3. Missing documentation for new public APIsFile: docs/api-reference.md:192 |
1 participant
Fixes #105.
Problem
The dynamic-provider cache (for
onResolveProvider-resolved providers) was never evicted under the default config.cacheDynamicProvidersdefaulted totrue→Infinity, and nothing — backing-store writes, plugin config reloads, login/logout — cleared it. Once anoac-*/dynamic provider was resolved, disabling it, deleting it, or rotating itsclientId/clientSecrethad no effect until process restart, and a revoked SSO config kept authenticating through the per-request session-validation middleware.Change
cacheDynamicProvidersnow defaults to 300s (new exportedDEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS) instead of forever, so every worker thread re-resolves within the window. This matches the cache's documented "bounded window" intent.DynamicProviderCache.delete(name)and exportedinvalidateDynamicProvider(providerConfigId)/clearDynamicProviderCache(), backed by a module-level reference to the active cache (mirrors the existingactiveHookManagerpattern). A consumer can evict immediately when its backing config changes.The cache is in-memory per worker thread, so explicit invalidation reaches only the calling thread; the bounded TTL is the cross-thread convergence mechanism. Both behaviors are documented in the code.
The default goes from cache-forever → 300s. Any deployment relying on indefinite in-process caching of a dynamically-resolved provider will now re-resolve every 5 minutes.
cacheDynamicProviders: truestill opts back into forever. This is intentional — forever-with-no-invalidation is the bug.Tests
test/lib/dynamicProviderCache.test.js:delete()coverage; corrected the default-TTL test (was asserting "forever"; now asserts the bounded default expires past the window).test/dynamic-provider-invalidation.test.js(new): exercises theindex.tswiring — invalidate-before-load is a no-op, resolve-on-miss → cache-hit → invalidate evicts → re-resolve,clear(), and the scope-close ownership guard.Cross-model review (thorough: codex + gemini + Harper adjudication)
Per the engineering guidelines. No blocker introduced. Gemini's "module-level singleton race" was adjudicated as NOISE under Harper's runtime (per-thread module context, single-threaded, no shared
Map); it mirrors the pre-existingactiveHookManagersingleton, and the scope-close guard (=== dynamicProviderCache) was confirmed correct. Codex: no findings. The one real item — the default-TTL behavior change — is the fix itself, flagged above.Follow-ups
updateOrganizationSettingsshould callinvalidateDynamicProvider(configId)after writing/disabling/rotating an org OAuth config for immediate effect. Separate CM change.v1.x(the 1.x line CM currently runs) — to follow.🤖 Generated with Claude Code