Skip to content

Backport #106 to v1.x: bound dynamic-provider cache TTL + invalidation API (#105)#107

Open
heskew wants to merge 1 commit into
v1.xfrom
backport/dynamic-provider-cache-invalidation-v1x
Open

Backport #106 to v1.x: bound dynamic-provider cache TTL + invalidation API (#105)#107
heskew wants to merge 1 commit into
v1.xfrom
backport/dynamic-provider-cache-invalidation-v1x

Conversation

@heskew
Copy link
Copy Markdown
Member

@heskew heskew commented May 28, 2026

Backport of #106 to the v1.x line (the 1.x release central-manager currently runs). Addresses #105.

What

Cherry-pick of the #106 fix:

  • Default cacheDynamicProviders to a bounded 300s TTL instead of forever, so disabled/deleted/credential-rotated dynamic providers stop being served without a process restart.
  • Add DynamicProviderCache.delete() + exported invalidateDynamicProvider() / clearDynamicProviderCache() for immediate consumer-driven eviction (per-worker-thread; TTL is the cross-thread convergence mechanism).

Backport notes

The only conflict was the index.ts import block — main bundles a registerWellKnownHandlers (MCP) import that does not exist on v1.x; dropped it. Verified:

  • The dynamicProviderCache.ts diff is byte-identical to the main change.
  • All reviewed elements present (bounded default, delete(), invalidateDynamicProvider/clearDynamicProviderCache, scope-close ownership guard); no MCP references.

⚠️ Behavior change

Same as #106: default goes from cache-forever → 300s. cacheDynamicProviders: true still opts into forever.

Tests

  • v1.x full unit suite: 346 pass / 2 skip / 0 fail.
  • Includes the ported delete() tests and the new index.ts wiring test (invalidate/clear/close-guard), both green on v1.x.

Review

The code is the cross-model-reviewed change from #106 (thorough: codex + gemini + Harper adjudication — no blocker; the module-level singleton flag was adjudicated NOISE under Harper's per-thread model). The backport delta is solely the dropped MCP import, verified equivalent above.

Follow-up

central-manager's updateOrganizationSettings should call invalidateDynamicProvider(configId) after writing/disabling/rotating an org OAuth config for immediate effect. Separate CM change.


🤖 Generated with Claude Code

)

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>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces module-level cache invalidation functions (invalidateDynamicProvider and clearDynamicProviderCache) and changes the default dynamic provider cache TTL from infinite to a bounded 300 seconds to prevent serving stale data. The review feedback suggests replacing the single global activeDynamicProviderCache variable with a Set of active caches. This improvement ensures that invalidation and clearing are correctly propagated across multiple active plugin instances running in the same thread, which is particularly useful in multi-tenant environments or during hot-reloads.

Comment thread src/index.ts
Comment on lines +42 to +70
// 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();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a single global variable activeDynamicProviderCache can lead to issues if multiple instances of the plugin are loaded in the same thread (e.g., in multi-tenant environments, multiple applications, or during hot-reloads where a new scope is initialized before the old one is fully closed). In such cases, the global variable only tracks the most recently initialized instance, and closing any instance could prematurely clear the reference or leave other active caches unreachable for invalidation.

Using a Set of active caches resolves this cleanly, ensuring that invalidation and clearing are correctly propagated to all active caches in the thread.

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 anyDeleted = false;
	for (const cache of activeDynamicProviderCaches) {
		if (cache.delete(providerConfigId)) {
			anyDeleted = true;
		}
	}
	return anyDeleted;
}

/**
 * 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();
	}
}

Comment thread src/index.ts
Comment on lines +117 to +118
// Expose this thread's cache for consumer-driven invalidation
activeDynamicProviderCache = dynamicProviderCache;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add the initialized cache to the set of active caches to support multiple active plugin instances in the same thread.

Suggested change
// Expose this thread's cache for consumer-driven invalidation
activeDynamicProviderCache = dynamicProviderCache;
// Expose this thread's cache for consumer-driven invalidation
activeDynamicProviderCaches.add(dynamicProviderCache);

Comment thread src/index.ts
Comment on lines +318 to +320
if (activeDynamicProviderCache === dynamicProviderCache) {
activeDynamicProviderCache = null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Remove the cache from the set of active caches when the scope closes.

		activeDynamicProviderCaches.delete(dynamicProviderCache);

@github-actions
Copy link
Copy Markdown
Contributor

1 blocker found.

1. Module-level cache reference loses track of active apps on close

File: src/index.ts:318
What: The activeDynamicProviderCache module-level variable uses a "last-one-wins" pattern to track the active cache. The scope.on('close') handler nulls this reference if the closing app was the last one loaded. In environments where multiple Harper applications use the OAuth plugin in the same worker thread, if the most recently loaded app closes, the global reference is lost (null) even if other applications remain active.
Why it matters: Once the "active" reference is nulled, subsequent calls to invalidateDynamicProvider or clearDynamicProviderCache become no-ops for all remaining applications in that worker thread, preventing them from ever invalidating their (now orphaned) in-memory caches until a process restart.
Suggested fix: Replace the single activeDynamicProviderCache variable with a Set<DynamicProviderCache> to track all active caches in the worker.

In src/index.ts:

const activeCaches = new Set<DynamicProviderCache>();

export function invalidateDynamicProvider(providerConfigId: string): boolean {
    let deleted = false;
    for (const cache of activeCaches) {
        if (cache.delete(providerConfigId)) deleted = true;
    }
    return deleted;
}

export function clearDynamicProviderCache(): void {
    for (const cache of activeCaches) {
        cache.clear();
    }
}

// Inside handleApplication:
activeCaches.add(dynamicProviderCache);

// Inside scope.on('close'):
activeCaches.delete(dynamicProviderCache);

This ensures that invalidation requests reach all active plugin instances in the worker and that closing one app doesn't break invalidation for others. (Note: the existing activeHookManager shares a similar "last-one-wins" flaw, but since this PR introduces the cache invalidation API, it should avoid propagating that pattern to the new functionality).

@claude
Copy link
Copy Markdown

claude Bot commented May 28, 2026

Reviewed; no blockers found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant