diff --git a/apps/dev-playground/package.json b/apps/dev-playground/package.json index d07afce6d..38eac5719 100644 --- a/apps/dev-playground/package.json +++ b/apps/dev-playground/package.json @@ -13,6 +13,7 @@ "install": "cd client && npm install && cd ..", "preview": "vite preview", "check": "tsc", + "typecheck": "tsc --noEmit", "clean": "rm -rf build && cd client && rm -rf dist", "clean:full": "rm -rf build node_modules && cd client && rm -rf dist node_modules", "test:integration": "playwright test", diff --git a/apps/dev-playground/server/index.ts b/apps/dev-playground/server/index.ts index eb88c2a97..9662ab3de 100644 --- a/apps/dev-playground/server/index.ts +++ b/apps/dev-playground/server/index.ts @@ -375,10 +375,13 @@ createApp({ const pngs = new Map(); const metas = new Map(); for (const e of entries) { - if (e.path.endsWith(".png")) { - pngs.set(e.path.replace(/\.png$/, ""), e); - } else if (e.path.endsWith(".json")) { - metas.set(e.path.replace(/\.json$/, ""), e); + // `path` is optional on the SDK's entry type. + const path = e.path; + if (!path) continue; + if (path.endsWith(".png")) { + pngs.set(path.replace(/\.png$/, ""), e); + } else if (path.endsWith(".json")) { + metas.set(path.replace(/\.json$/, ""), e); } } const views = await Promise.all( diff --git a/apps/dev-playground/server/telemetry-example-plugin.ts b/apps/dev-playground/server/telemetry-example-plugin.ts index 7f74b20ed..63345bc73 100644 --- a/apps/dev-playground/server/telemetry-example-plugin.ts +++ b/apps/dev-playground/server/telemetry-example-plugin.ts @@ -4,7 +4,6 @@ import { type BasePluginConfig, - CacheManager, type Counter, type Histogram, Plugin, @@ -32,7 +31,6 @@ class TelemetryExamples extends Plugin { constructor(config: BasePluginConfig) { super(config); - this.cache = new CacheManager({ enabled: true, ttl: 60 }, this.telemetry); const meter = this.telemetry.getMeter({ name: "custom-telemetry-example" }); this.requestCounter = meter.createCounter("app.requests.total", { diff --git a/apps/dev-playground/tsconfig.json b/apps/dev-playground/tsconfig.json index e0c499b84..3d3524b7b 100644 --- a/apps/dev-playground/tsconfig.json +++ b/apps/dev-playground/tsconfig.json @@ -6,7 +6,9 @@ "declaration": false, "declarationMap": false, "experimentalDecorators": true, - "emitDecoratorMetadata": true + "emitDecoratorMetadata": true, + "customConditions": ["development"] }, - "exclude": ["node_modules", "build"] + "exclude": ["node_modules", "build", "client"], + "include": ["server/**/*.ts", "*.ts"] } diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index a5002a097..4cef001ca 100644 --- a/docs/docs/api/appkit/Class.Plugin.md +++ b/docs/docs/api/appkit/Class.Plugin.md @@ -120,14 +120,6 @@ protected app: AppManager; *** -### cache - -```ts -protected cache: CacheManager; -``` - -*** - ### config ```ts @@ -203,6 +195,30 @@ Plugin initialization phase. - 'normal': Initialized second (most plugins) - 'deferred': Initialized last (e.g., server plugin) +## Accessors + +### cache + +#### Get Signature + +```ts +get protected cache(): CacheManager; +``` + +This app's cache, bound by [attachContext](#attachcontext). Every plugin in an app +shares the one manager the app built; a plugin cannot substitute its own, +so set a per-plugin `cache: { enabled, ttl }` config instead of assigning. + +Throws `InitializationError` when read on an unattached plugin (the app-less +`runAgent` path, or a plugin built by hand in a test). This is the guard for +the direct readers — `analytics.ts`, `files/plugin.ts` — that reach the +cache without going through [execute](#execute): they now fail with a named +error at the read rather than a bare `TypeError` deeper in a handler. + +##### Returns + +`CacheManager` + ## Methods ### abortActiveOperations() @@ -268,10 +284,14 @@ attachContext(deps: { Binds runtime dependencies (telemetry provider, cache, plugin context) to this plugin. Called by `AppKit._createApp` after construction and before -`setup()`. Idempotent: safe to call if the constructor already bound them -eagerly. Kept separate so factories can eagerly construct plugin instances -without running this before `TelemetryManager.initialize()` / -`CacheManager.getInstance()` have run. +`setup()`. Kept separate from the constructor so plugin factories can be +evaluated at module top level, before any app exists. + +A context-less `attachContext({})` is the app-less path (see `runAgent`): +it binds telemetry and leaves the cache unbound, so only a cached execution +fails, at the chokepoint in \_buildInterceptors. A supplied context +always carries a cache — `PluginContext.cache` is required, so a cache-less +one cannot be constructed. #### Parameters diff --git a/docs/docs/api/appkit/Interface.AgentsPluginConfig.md b/docs/docs/api/appkit/Interface.AgentsPluginConfig.md index a2f2c963f..2f888b9d0 100644 --- a/docs/docs/api/appkit/Interface.AgentsPluginConfig.md +++ b/docs/docs/api/appkit/Interface.AgentsPluginConfig.md @@ -260,6 +260,23 @@ are discovered at boot and on `reload()` and read as the service principal. *** +### streamConfig? + +```ts +optional streamConfig: StreamConfig; +``` + +SSE stream configuration for this plugin's `executeStream()` calls (buffer +sizes, `maxEventSize`, TTL, heartbeat). Sets the plugin's StreamManager +defaults; a per-call `stream` config still overrides these. Use it to raise +`maxEventSize` above the 5 MiB default when a stream emits larger events. + +#### Inherited from + +[`BasePluginConfig`](Interface.BasePluginConfig.md).[`streamConfig`](Interface.BasePluginConfig.md#streamconfig) + +*** + ### telemetry? ```ts diff --git a/docs/docs/api/appkit/Interface.BasePluginConfig.md b/docs/docs/api/appkit/Interface.BasePluginConfig.md index a109fd560..9ee98474b 100644 --- a/docs/docs/api/appkit/Interface.BasePluginConfig.md +++ b/docs/docs/api/appkit/Interface.BasePluginConfig.md @@ -32,6 +32,19 @@ optional name: string; *** +### streamConfig? + +```ts +optional streamConfig: StreamConfig; +``` + +SSE stream configuration for this plugin's `executeStream()` calls (buffer +sizes, `maxEventSize`, TTL, heartbeat). Sets the plugin's StreamManager +defaults; a per-call `stream` config still overrides these. Use it to raise +`maxEventSize` above the 5 MiB default when a stream emits larger events. + +*** + ### telemetry? ```ts diff --git a/docs/docs/api/appkit/Interface.IAiSearchConfig.md b/docs/docs/api/appkit/Interface.IAiSearchConfig.md index 316b4aac3..679d8b18f 100644 --- a/docs/docs/api/appkit/Interface.IAiSearchConfig.md +++ b/docs/docs/api/appkit/Interface.IAiSearchConfig.md @@ -46,6 +46,23 @@ optional name: string; *** +### streamConfig? + +```ts +optional streamConfig: StreamConfig; +``` + +SSE stream configuration for this plugin's `executeStream()` calls (buffer +sizes, `maxEventSize`, TTL, heartbeat). Sets the plugin's StreamManager +defaults; a per-call `stream` config still overrides these. Use it to raise +`maxEventSize` above the 5 MiB default when a stream emits larger events. + +#### Inherited from + +[`BasePluginConfig`](Interface.BasePluginConfig.md).[`streamConfig`](Interface.BasePluginConfig.md#streamconfig) + +*** + ### telemetry? ```ts diff --git a/docs/docs/api/appkit/Interface.IJobsConfig.md b/docs/docs/api/appkit/Interface.IJobsConfig.md index aeff8fa92..d86e7280c 100644 --- a/docs/docs/api/appkit/Interface.IJobsConfig.md +++ b/docs/docs/api/appkit/Interface.IJobsConfig.md @@ -58,6 +58,23 @@ Poll interval for waitForRun in milliseconds. Defaults to 5000. *** +### streamConfig? + +```ts +optional streamConfig: StreamConfig; +``` + +SSE stream configuration for this plugin's `executeStream()` calls (buffer +sizes, `maxEventSize`, TTL, heartbeat). Sets the plugin's StreamManager +defaults; a per-call `stream` config still overrides these. Use it to raise +`maxEventSize` above the 5 MiB default when a stream emits larger events. + +#### Inherited from + +[`BasePluginConfig`](Interface.BasePluginConfig.md).[`streamConfig`](Interface.BasePluginConfig.md#streamconfig) + +*** + ### telemetry? ```ts diff --git a/docs/docs/plugins/caching.md b/docs/docs/plugins/caching.md index 3bd7fedd6..ff83db8fe 100644 --- a/docs/docs/plugins/caching.md +++ b/docs/docs/plugins/caching.md @@ -35,3 +35,69 @@ const value = await this.cache.getOrExecute( { ttl: 300 }, ); ``` + +### One cache per app + +Each app owns exactly one cache. `createApp` builds it from your `cache` config +and hands the same manager to every plugin it registers, so `this.cache` is the +app's cache — not a process-wide one. Two apps in the same process hold two +independent managers, and each honours its own `cache` config. + +`this.cache` is read-only: a plugin cannot substitute its own manager. To vary +caching per plugin, set a plugin-level `cache` config instead: + +```ts +analytics({ cache: { enabled: true, ttl: 600 } }); +``` + +A plugin receives its cache when the app registers it. Construction alone does +not bind one, so `this.cache` is not available in a plugin's constructor — +read it from `setup()` or from a request handler, both of which run after +registration. + +## Upgrading to 0.71.0 + +The cache became per-app in 0.71.0. Most apps need no changes: if your plugins +reach the cache through `this.cache` and you build apps with `createApp`, this +release is a no-op for you. + +**`CacheManager.getInstance()` and `CacheManager.getInstanceSync()` are +removed.** There is no process-wide cache to fetch. Inside a plugin, use +`this.cache`, which the app binds for you: + +```ts +// Before +const cache = CacheManager.getInstanceSync(); +await cache.getOrExecute(["k"], work, userKey); + +// After +await this.cache.getOrExecute(["k"], work, userKey); +``` + +An app's cache is handed only to the plugins it registers — there is no +process-wide accessor to fetch it from. Move the cached work into a plugin. + +**A plugin constructed without an app has no cache.** Previously such a plugin +picked up whichever manager happened to exist in the process. Now a cached +execution on an unregistered plugin throws `InitializationError` +(`CacheManager not initialized`) naming the plugin. Register it through +`createApp`, or in tests attach it to a test context: + +```ts +import { createTestPluginContext } from "@databricks/appkit/testing"; + +const mock = createTestPluginContext(); +await mock.attach(new MyPlugin({})); +// mock.cache is the very cache the plugin now resolves — spy or read it. +``` + +The most common way to hit this is reading `this.cache` in a plugin's +constructor — which always runs before any attach — or on a plugin you built +by hand and never registered. Under `createApp`, `setup()` and request +handlers run after the cache is bound, so a registered plugin is safe (this is +why [above](#one-cache-per-app) says to read the cache from `setup()`). + +**`this.cache` is read-only.** A plugin that assigned its own manager +(`this.cache = new CacheManager(...)`) no longer compiles. Use a plugin-level +`cache: { enabled, ttl }` config instead — see [One cache per +app](#one-cache-per-app). diff --git a/docs/docs/plugins/testing.md b/docs/docs/plugins/testing.md index 30fc66642..5db940fb8 100644 --- a/docs/docs/plugins/testing.md +++ b/docs/docs/plugins/testing.md @@ -49,7 +49,7 @@ const mock = createTestPluginContext({ ### Attaching to a plugin -`attach()` wires the context to a plugin the production way: it seeds an in-memory cache (if AppKit hasn't already initialized one), then calls the plugin's `attachContext`, which rebuilds telemetry and flips `isReady` to `true`. Await it before exercising any handler that reads `this.context`, `this.cache`, or gates on `isReady`: +`attach()` wires the context to a plugin the production way: it calls the plugin's `attachContext`, which binds this context's cache, rebuilds telemetry, and flips `isReady` to `true`. Await it before exercising any handler that reads `this.context`, `this.cache`, or gates on `isReady`: ```ts const plugin = new MyAgentPlugin({}); @@ -58,17 +58,30 @@ await mock.attach(plugin); Instantiate the plugin **class** directly (`new MyAgentPlugin(...)`). The `analytics()` / `agents()` factories you pass to `createApp` return a descriptor for the app to construct — for a unit test you want the instance. -The cache `attach()` seeds is a process-wide singleton: `CacheManager` is initialized once per test process and reused. Vitest isolates test *files* in separate workers, so caches never leak across files, but tests **within one file** share it. If a test populates the cache and a later test in the same file must not see it, clear it between tests with `resetTestCache()`: +Each `createTestPluginContext()` carries its **own** real in-memory cache, exposed as `mock.cache` — the very object the attached plugin resolves as `this.cache`. Two contexts in one file cannot see each other's entries, and nothing is shared with other test files. + +That makes it the seam for asserting real caching behaviour, against production's own `getOrExecute` and `generateKey` rather than a re-implemented fake: + +```ts +const mock = createTestPluginContext(); +const plugin = await mock.attach(new MyPlugin({})); + +const getOrExecute = vi.spyOn(mock.cache, "getOrExecute"); +await plugin.handleRequest(req, res); +expect(getOrExecute).toHaveBeenCalledOnce(); +``` + +Entries persist across tests in a file, since the context is built where you build it. Clear between tests — or mid-test, to force a miss before asserting the next call is a hit — with `resetTestCache()`: ```ts import { resetTestCache } from "@databricks/appkit/testing"; beforeEach(async () => { - await resetTestCache(); // no-op if the cache isn't initialized yet + await resetTestCache(); // every cache this file's contexts built }); ``` -It also helps *within* a single test — clear the cache to force a miss, then assert the following call is a hit. +Pass a specific context (`resetTestCache(mock)`) or manager (`resetTestCache(mock.cache)`) to clear just that one. ### Inspecting what happened @@ -159,7 +172,7 @@ The kit re-exports the request/response/context fixtures AppKit uses internally: ``` - `createSuccessfulSQLResponse(rows, columns)` / `createFailedSQLResponse(message)` — build SQL Warehouse statement responses. - `setupDatabricksEnv(overrides?)` — set `DATABRICKS_HOST` / `DATABRICKS_WAREHOUSE_ID` to test values. -- `resetTestCache()` — clear the shared cache singleton between (or within) tests; no-ops if the cache isn't initialized yet. +- `resetTestCache(target?)` — clear the caches this file's test contexts built, between or within tests. Pass a context or a manager to clear only that one; no-ops when there is nothing to clear. ## Full example diff --git a/packages/appkit/src/cache/index.ts b/packages/appkit/src/cache/index.ts index a98b7dff4..a428df930 100644 --- a/packages/appkit/src/cache/index.ts +++ b/packages/appkit/src/cache/index.ts @@ -4,7 +4,7 @@ import type { CacheConfig, CacheEntry, CacheStorage } from "shared"; import { createLakebasePool } from "../connectors/lakebase"; import { getClientOptions } from "../context/client-options"; -import { AppKitError, ExecutionError, InitializationError } from "../errors"; +import { AppKitError, ExecutionError } from "../errors"; import { createLogger } from "../logging/logger"; import type { Counter, TelemetryProvider } from "../telemetry"; import { SpanStatusCode, TelemetryManager } from "../telemetry"; @@ -42,22 +42,25 @@ function createAbortError(signal: AbortSignal): unknown { * Cache manager class to handle cache operations. * Can be used with in-memory storage or persistent storage (Lakebase). * - * The cache is automatically initialized by AppKit. Use `getInstanceSync()` to access - * the singleton instance after initialization. + * One manager belongs to one app: `createApp` builds it and hands it to every + * plugin through the plugin context, so a plugin reads it as `this.cache`. + * There is no process-wide instance and no public construction path. * * @internal * @example * ```typescript - * const cache = CacheManager.getInstanceSync(); - * const result = await cache.getOrExecute(["users", userId], () => fetchUser(userId), userKey); + * // Inside a plugin — the app bound this cache when it registered the plugin. + * const result = await this.cache.getOrExecute( + * ["users", userId], + * () => fetchUser(userId), + * userKey, + * ); * ``` */ export class CacheManager { private static readonly MIN_CLEANUP_INTERVAL_MS = 60_000; private static readonly ABORT_GRACE_PERIOD_MS = 100; private readonly name: string = "cache-manager"; - private static instance: CacheManager | null = null; - private static initPromise: Promise | null = null; private storage: CacheStorage; private config: CacheConfig; @@ -71,7 +74,16 @@ export class CacheManager { cacheMissCount: Counter; }; - private constructor(storage: CacheStorage, config: CacheConfig) { + /** + * @param ownsStorage - Whether this manager built its own storage. {@link close} + * closes only owned storage: closing a caller's `cache: { storage }` is + * destructive — `PersistentStorage.close()` is a permanent `pool.end()`. + */ + private constructor( + storage: CacheStorage, + config: CacheConfig, + private readonly ownsStorage: boolean, + ) { this.storage = storage; this.config = config; this.inFlightRequests = new Map(); @@ -95,50 +107,25 @@ export class CacheManager { } /** - * Get the singleton instance of the cache manager (sync version). + * Build a manager over caller-supplied storage, synchronously — the testing + * kit's path, where storage is always in-memory so there is no health check + * to await. The async {@link create} is the app's path. * - * Throws if not initialized - ensure AppKit.create() has completed first. - * @returns CacheManager instance - */ - static getInstanceSync(): CacheManager { - if (!CacheManager.instance) { - throw InitializationError.notInitialized( - "CacheManager", - "Ensure AppKit.create() has completed before accessing the cache", - ); - } - - return CacheManager.instance; - } - - /** - * Initialize and get the singleton instance of the cache manager. - * Called internally by AppKit - prefer `getInstanceSync()` for plugin access. - * @param userConfig - User configuration for the cache manager - * @returns CacheManager instance * @internal */ - static async getInstance( + static forStorage( + storage: CacheStorage, userConfig?: Partial, - ): Promise { - if (CacheManager.instance) { - return CacheManager.instance; - } - - if (!CacheManager.initPromise) { - CacheManager.initPromise = CacheManager.create(userConfig).then( - (instance) => { - CacheManager.instance = instance; - return instance; - }, - ); - } - - return CacheManager.initPromise; + ): CacheManager { + return new CacheManager( + storage, + deepMerge(cacheDefaults, userConfig), + false, + ); } /** - * Create a new cache manager instance + * Create a new cache manager instance. * * Storage selection logic: * 1. If `storage` provided and healthy → use provided storage @@ -148,8 +135,9 @@ export class CacheManager { * * @param userConfig - User configuration for the cache manager * @returns CacheManager instance + * @internal */ - private static async create( + static async create( userConfig?: Partial, ): Promise { const config = deepMerge(cacheDefaults, userConfig); @@ -157,7 +145,7 @@ export class CacheManager { if (config.storage) { const isHealthy = await config.storage.healthCheck(); if (isHealthy) { - return new CacheManager(config.storage, config); + return new CacheManager(config.storage, config, false); } if (config.strictPersistence) { @@ -165,10 +153,11 @@ export class CacheManager { return new CacheManager( new InMemoryStorage(disabledConfig), disabledConfig, + true, ); } - return new CacheManager(new InMemoryStorage(config), config); + return new CacheManager(new InMemoryStorage(config), config, true); } // try to use lakebase storage @@ -181,8 +170,23 @@ export class CacheManager { const isHealthy = await persistentStorage.healthCheck(); if (isHealthy) { - await persistentStorage.initialize(); - return new CacheManager(persistentStorage, config); + try { + await persistentStorage.initialize(); + } catch (err) { + // Health check passed but `initialize()` failed. Unlike an + // unreachable Lakebase (the expected, silent fallback), a healthy + // connection that fails to initialize is surprising and worth a + // signal — the outer catch is silent. End the pool we opened before + // falling through to in-memory, or it is orphaned for the life of the + // process while boot still "succeeds". + logger.warn( + "Lakebase cache is healthy but failed to initialize; falling back to in-memory: %O", + err, + ); + await pool.end().catch(() => {}); + throw err; + } + return new CacheManager(persistentStorage, config, true); } // Health check failed, close the pool and fallback @@ -196,10 +200,11 @@ export class CacheManager { return new CacheManager( new InMemoryStorage(disabledConfig), disabledConfig, + true, ); } - return new CacheManager(new InMemoryStorage(config), config); + return new CacheManager(new InMemoryStorage(config), config, true); } /** @@ -554,6 +559,8 @@ export class CacheManager { /** Close the cache */ async close(): Promise { + // Borrowed storage outlives the manager: whoever supplied it owns closing it. + if (!this.ownsStorage) return; await this.storage.close(); } diff --git a/packages/appkit/src/cache/storage/memory.ts b/packages/appkit/src/cache/storage/memory.ts index 02c4bb53f..883fe47be 100644 --- a/packages/appkit/src/cache/storage/memory.ts +++ b/packages/appkit/src/cache/storage/memory.ts @@ -12,7 +12,9 @@ export class InMemoryStorage implements CacheStorage { private accessCounter: number; private maxSize: number; - constructor(config: CacheConfig) { + // Only `maxSize` is read; the parameter is narrowed to say so, which also + // lets callers write `new InMemoryStorage()` instead of casting a full config. + constructor(config: Pick = {}) { this.cache = new Map(); this.accessOrder = new Map(); this.maxSize = config.maxSize ?? inMemoryStorageDefaults.maxSize; diff --git a/packages/appkit/src/cache/tests/cache-manager-storage-ownership.test.ts b/packages/appkit/src/cache/tests/cache-manager-storage-ownership.test.ts new file mode 100644 index 000000000..5c7ceb96c --- /dev/null +++ b/packages/appkit/src/cache/tests/cache-manager-storage-ownership.test.ts @@ -0,0 +1,114 @@ +import type { CacheConfig, CacheStorage } from "shared"; +import { describe, expect, test, vi } from "vitest"; + +import { CacheManager } from ".."; +import { InMemoryStorage } from "../storage/memory"; + +/** + * Who owns the storage a manager closes. + * + * A caller who passes `cache: { storage }` keeps ownership, so `close()` must + * leave it alone. The hazard stays invisible because `InMemoryStorage.close()` + * merely clears a `Map` and stays usable, while `PersistentStorage.close()` is + * `pool.end()` and permanent. + */ + +function inMemory(): InMemoryStorage { + return new InMemoryStorage({ maxSize: 100 }); +} + +/** Models a storage whose close is permanent, the way `pool.end()` is. */ +class EndableStorage extends InMemoryStorage { + ended = false; + + override async close(): Promise { + this.ended = true; + } +} + +function endable(): EndableStorage { + return new EndableStorage({ maxSize: 100 }); +} + +/** A storage that reports unhealthy, forcing `create()` to build its own. */ +function unhealthy(): CacheStorage { + const storage = inMemory(); + vi.spyOn(storage, "healthCheck").mockResolvedValue(false); + return storage; +} + +const config = (extra: Partial = {}) => + ({ enabled: true, ...extra }) as Partial; + +describe("CacheManager storage ownership", () => { + test("storage the caller supplied survives close and stays usable", async () => { + const storage = endable(); + const manager = await CacheManager.create(config({ storage })); + + await manager.close(); + + expect(storage.ended).toBe(false); + const key = manager.generateKey(["probe"], "user"); + await manager.set(key, { ok: true }); + await expect(manager.get(key)).resolves.toEqual({ ok: true }); + }); + + test("storage the manager built is closed", async () => { + // No `storage` given and Lakebase unreachable in tests, so `create()` falls + // back to storage it owns. + const manager = await CacheManager.create(config()); + const storage = (manager as unknown as { storage: CacheStorage }).storage; + const close = vi.spyOn(storage, "close"); + + await manager.close(); + + expect(close).toHaveBeenCalledTimes(1); + }); + + test("a supplied storage that fails its health check yields owned storage", async () => { + // The branch a "was storage supplied?" flag would get wrong: `create()` + // builds a fresh in-memory storage *inside* the supplied-storage branch, so + // that replacement is the manager's to close. + const supplied = unhealthy(); + const manager = await CacheManager.create(config({ storage: supplied })); + + const actual = (manager as unknown as { storage: CacheStorage }).storage; + expect(actual).not.toBe(supplied); + + const close = vi.spyOn(actual, "close"); + await manager.close(); + expect(close).toHaveBeenCalledTimes(1); + }); + + test("a supplied storage that fails its health check is not closed", async () => { + const supplied = endable(); + vi.spyOn(supplied, "healthCheck").mockResolvedValue(false); + + const manager = await CacheManager.create(config({ storage: supplied })); + await manager.close(); + + expect(supplied.ended).toBe(false); + }); + + test("forStorage treats the kit's storage as borrowed", async () => { + const storage = endable(); + const manager = CacheManager.forStorage(storage); + + await manager.close(); + + expect(storage.ended).toBe(false); + }); + + test("two managers over one storage: closing the first leaves the second usable", async () => { + // The shape a test harness reaches by passing the same storage to two boots. + const storage = inMemory(); + const first = await CacheManager.create(config({ storage })); + const second = await CacheManager.create(config({ storage })); + + const key = second.generateKey(["shared"], "user"); + await second.set(key, { alive: true }); + await first.close(); + + await expect(second.get(key)).resolves.toEqual({ alive: true }); + }); +}); diff --git a/packages/appkit/src/cache/tests/cache-manager.test.ts b/packages/appkit/src/cache/tests/cache-manager.test.ts index 1565978ce..3c621bef5 100644 --- a/packages/appkit/src/cache/tests/cache-manager.test.ts +++ b/packages/appkit/src/cache/tests/cache-manager.test.ts @@ -6,6 +6,9 @@ import { CacheManager } from "../../index"; // Mock createLakebasePool const mockPoolQuery = vi.fn(); const mockPoolEnd = vi.fn(); +// Controls whether PersistentStorage.initialize() resolves; a test can make it +// reject to exercise the "healthy but init failed" pool-leak path. +const mockInitialize = vi.fn(); vi.mock("@/connectors/lakebase", () => ({ createLakebasePool: vi.fn().mockImplementation(() => ({ query: mockPoolQuery, @@ -18,7 +21,7 @@ vi.mock("../storage/persistent", () => ({ PersistentStorage: vi.fn().mockImplementation((_config: any, pool: any) => { const cache = new Map(); return { - initialize: vi.fn().mockResolvedValue(undefined), + initialize: mockInitialize, get: vi .fn() .mockImplementation(async (key: string) => cache.get(key) || null), @@ -98,47 +101,19 @@ function createUnhealthyMockStorage(): CacheStorage { } describe("CacheManager", () => { - // Reset singleton between tests beforeEach(() => { - // Access private static fields to reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; // Default: Lakebase unavailable (most tests pass explicit storage) mockPoolQuery.mockRejectedValue(new Error("Connection failed")); + mockInitialize.mockResolvedValue(undefined); }); afterEach(() => { vi.clearAllMocks(); }); - describe("singleton pattern", () => { - test("getInstanceSync should throw when not initialized", () => { - expect(() => CacheManager.getInstanceSync()).toThrow( - "CacheManager not initialized", - ); - }); - - test("getInstance should create singleton", async () => { - const instance1 = await CacheManager.getInstance({ - storage: createMockStorage(), - }); - const instance2 = await CacheManager.getInstance(); - - expect(instance1).toBe(instance2); - }); - - test("getInstanceSync should return instance after initialization", async () => { - await CacheManager.getInstance({ storage: createMockStorage() }); - - const instance = CacheManager.getInstanceSync(); - - expect(instance).toBeInstanceOf(CacheManager); - }); - }); - describe("generateKey", () => { test("should generate consistent hash for same inputs", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -149,7 +124,7 @@ describe("CacheManager", () => { }); test("should generate different hash for different inputs", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -163,7 +138,7 @@ describe("CacheManager", () => { }); test("should handle objects in key parts", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -178,7 +153,7 @@ describe("CacheManager", () => { describe("get/set operations", () => { test("should return null for non-existent key", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -188,7 +163,7 @@ describe("CacheManager", () => { }); test("should set and get value", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -199,7 +174,7 @@ describe("CacheManager", () => { }); test("should respect TTL expiry", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -217,7 +192,7 @@ describe("CacheManager", () => { describe("delete operation", () => { test("should delete existing key", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -231,7 +206,7 @@ describe("CacheManager", () => { describe("has operation", () => { test("should return true for existing key", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -242,7 +217,7 @@ describe("CacheManager", () => { }); test("should return false for non-existent key", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -251,7 +226,7 @@ describe("CacheManager", () => { }); test("should return false for expired key", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -265,7 +240,7 @@ describe("CacheManager", () => { describe("clear operation", () => { test("should clear all entries", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -281,7 +256,7 @@ describe("CacheManager", () => { describe("getOrExecute", () => { test("should execute function on cache miss", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const fn = vi.fn().mockResolvedValue("result"); @@ -293,7 +268,7 @@ describe("CacheManager", () => { }); test("should return cached value on cache hit", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const fn = vi.fn().mockResolvedValue("new-result"); @@ -309,7 +284,7 @@ describe("CacheManager", () => { }); test("should deduplicate concurrent requests", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); let callCount = 0; @@ -336,7 +311,7 @@ describe("CacheManager", () => { }); test("should use different cache keys for different users", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -359,7 +334,7 @@ describe("CacheManager", () => { }); test("should re-execute function when cached entry has expired", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); let calls = 0; @@ -383,7 +358,7 @@ describe("CacheManager", () => { }); test("should work when fn ignores signal parameter (non-signal caller regression)", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); // Simulates direct callers like telemetry-example-plugin that pass @@ -403,7 +378,7 @@ describe("CacheManager", () => { describe("abort / ref-counting", () => { test("one caller aborts while another still waits — waiting caller resolves normally", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -443,7 +418,7 @@ describe("CacheManager", () => { }); test("all callers abort — shared controller signal is aborted", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -492,7 +467,7 @@ describe("CacheManager", () => { }); test("pre-aborted callerSignal throws immediately without executing fn", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -512,7 +487,7 @@ describe("CacheManager", () => { }); test("single caller abort mid-flight rejects with abort error and aborts shared controller", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -545,7 +520,7 @@ describe("CacheManager", () => { }); test("deduped caller abort does not poison the first caller's result", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -580,7 +555,7 @@ describe("CacheManager", () => { }); test("fn rejects while multiple callers wait — all receive the error", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -616,7 +591,7 @@ describe("CacheManager", () => { }); test("caller aborts after promise already resolved — gets the resolved value", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -648,7 +623,7 @@ describe("CacheManager", () => { }); test("new caller after previous entry fully aborted gets fresh execution", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -696,7 +671,7 @@ describe("CacheManager", () => { }); test("grace period: new caller joins before timer fires — no abort, single execution", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -742,7 +717,7 @@ describe("CacheManager", () => { }); test("grace period: no new caller arrives — timer fires and aborts shared controller", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -781,7 +756,7 @@ describe("CacheManager", () => { describe("disabled cache", () => { test("should bypass cache when disabled", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ enabled: false, storage: createMockStorage(), }); @@ -796,7 +771,7 @@ describe("CacheManager", () => { }); test("should return null for get when disabled", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ enabled: false, storage: createMockStorage(), }); @@ -808,7 +783,7 @@ describe("CacheManager", () => { }); test("should return false for has when disabled", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ enabled: false, storage: createMockStorage(), }); @@ -822,7 +797,7 @@ describe("CacheManager", () => { describe("storage health", () => { test("should check storage health", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -834,7 +809,7 @@ describe("CacheManager", () => { describe("close", () => { test("should close storage", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); @@ -844,7 +819,7 @@ describe("CacheManager", () => { describe("maybeCleanup", () => { test("should not trigger cleanup for non-persistent storage", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(false), cleanupProbability: 1, // 100% probability }); @@ -860,7 +835,7 @@ describe("CacheManager", () => { }); test("should respect MIN_CLEANUP_INTERVAL_MS", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), cleanupProbability: 1, }); @@ -881,7 +856,7 @@ describe("CacheManager", () => { }); test("should trigger cleanup when probability allows and interval passed", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), cleanupProbability: 1, // 100% probability }); @@ -902,7 +877,7 @@ describe("CacheManager", () => { }); test("should not trigger cleanup when already in progress", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), cleanupProbability: 1, }); @@ -922,7 +897,7 @@ describe("CacheManager", () => { }); test("should handle cleanup errors gracefully", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), cleanupProbability: 1, }); @@ -949,7 +924,7 @@ describe("CacheManager", () => { describe("getOrExecute error handling", () => { test("should propagate errors from executed function", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const error = new Error("Execution failed"); @@ -961,7 +936,7 @@ describe("CacheManager", () => { }); test("should remove in-flight request on error", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const error = new Error("Execution failed"); @@ -980,7 +955,7 @@ describe("CacheManager", () => { test("should re-throw ApiError without wrapping", async () => { const { ApiError } = await import("../../workspace-client"); - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const apiError = new ApiError( @@ -998,7 +973,7 @@ describe("CacheManager", () => { }); test("should allow retry after error", async () => { - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createMockStorage(), }); const fn = vi @@ -1017,12 +992,8 @@ describe("CacheManager", () => { describe("strictPersistence mode", () => { test("should disable cache when strictPersistence is true and storage unhealthy", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Pass an unhealthy storage with strictPersistence: true - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createUnhealthyMockStorage(), strictPersistence: true, }); @@ -1039,12 +1010,8 @@ describe("CacheManager", () => { describe("storage fallback", () => { test("should fallback to in-memory when provided storage is unhealthy", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Pass an unhealthy storage, should fallback to in-memory - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createUnhealthyMockStorage(), strictPersistence: false, }); @@ -1056,11 +1023,7 @@ describe("CacheManager", () => { }); test("should use in-memory storage when provided storage health check fails", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ storage: createUnhealthyMockStorage(), strictPersistence: false, }); @@ -1073,17 +1036,13 @@ describe("CacheManager", () => { describe("lakebase default storage", () => { test("should use Lakebase when no storage provided and Lakebase is available", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Make pool.query succeed for healthCheck ('SELECT 1') mockPoolQuery.mockResolvedValue({ rows: [{ "?column?": 1 }], rowCount: 1, }); - const cache = await CacheManager.getInstance({}); + const cache = await CacheManager.create({}); // Storage should be persistent (Lakebase) const storage = (cache as any).storage; @@ -1091,14 +1050,10 @@ describe("CacheManager", () => { }); test("should fallback to in-memory when Lakebase is unavailable", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Lakebase unhealthy (pool.query fails, default in beforeEach) mockPoolQuery.mockRejectedValue(new Error("Connection failed")); - const cache = await CacheManager.getInstance({}); + const cache = await CacheManager.create({}); // Cache should work (in-memory fallback) await cache.set("test-key", "value"); @@ -1111,14 +1066,10 @@ describe("CacheManager", () => { }); test("should disable cache when Lakebase unavailable and strictPersistence is true", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Lakebase unhealthy (pool.query fails) mockPoolQuery.mockRejectedValue(new Error("Connection failed")); - const cache = await CacheManager.getInstance({ + const cache = await CacheManager.create({ strictPersistence: true, }); @@ -1132,14 +1083,10 @@ describe("CacheManager", () => { }); test("should use in-memory storage when Lakebase health check fails", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Lakebase unhealthy - pool.query('SELECT 1') fails mockPoolQuery.mockRejectedValue(new Error("Health check failed")); - const cache = await CacheManager.getInstance({}); + const cache = await CacheManager.create({}); // Should be using in-memory storage const storage = (cache as any).storage; @@ -1147,18 +1094,28 @@ describe("CacheManager", () => { }); test("should use in-memory storage when Lakebase throws an error", async () => { - // Reset singleton - (CacheManager as any).instance = null; - (CacheManager as any).initPromise = null; - // Lakebase throws mockPoolQuery.mockRejectedValue(new Error("Connection refused")); - const cache = await CacheManager.getInstance({}); + const cache = await CacheManager.create({}); // Should be using in-memory storage const storage = (cache as any).storage; expect(storage.isPersistent()).toBe(false); }); + + test("ends the pool and falls back when a healthy Lakebase fails to initialize", async () => { + // Health check passes, so the pool is opened... + mockPoolQuery.mockResolvedValue({ rows: [{ "?column?": 1 }] }); + // ...but initialize() throws after that. The pool must be ended, not + // orphaned, and boot must fall back to in-memory rather than "succeed" + // holding a leaked connection. + mockInitialize.mockRejectedValue(new Error("schema migration failed")); + + const cache = await CacheManager.create({}); + + expect(mockPoolEnd).toHaveBeenCalledTimes(1); + expect((cache as any).storage.isPersistent()).toBe(false); + }); }); }); diff --git a/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts b/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts index 9def8f43e..8462d83fa 100644 --- a/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/pool-manager.test.ts @@ -1,21 +1,6 @@ import type { Pool } from "pg"; import { afterEach, describe, expect, test, vi } from "vitest"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - const mockPools: Pool[] = []; vi.mock("../index", () => ({ diff --git a/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts b/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts index c19f7c15e..4ccccd86d 100644 --- a/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts +++ b/packages/appkit/src/connectors/lakebase/tests/routing-pool.test.ts @@ -3,21 +3,6 @@ import { describe, expect, test, vi } from "vitest"; import { RoutingPool } from "../routing-pool"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - function makeMockPool(label: string) { return { query: vi.fn(async () => ({ rows: [{ source: label }] })), diff --git a/packages/appkit/src/core/agent/tests/run-agent.test.ts b/packages/appkit/src/core/agent/tests/run-agent.test.ts index efaa0356b..1abf3241d 100644 --- a/packages/appkit/src/core/agent/tests/run-agent.test.ts +++ b/packages/appkit/src/core/agent/tests/run-agent.test.ts @@ -6,11 +6,14 @@ import type { AgentToolDefinition, PluginConstructor, PluginData, + PluginManifest, ToolProvider, } from "shared"; import { describe, expect, test, vi } from "vitest"; import { z } from "zod"; +import { InitializationError } from "../../../errors"; +import { Plugin } from "../../../plugin"; import { createAgent } from "../create-agent"; import { runAgent } from "../run-agent"; import { mcpServer } from "../tools/hosted-tools"; @@ -352,6 +355,65 @@ describe("runAgent", () => { ).rejects.toThrow(/createApp/); }); + test("standalone init runs the app-less lifecycle on a real Plugin subclass", async () => { + // run-agent.ts calls `attachContext({})` on every standalone plugin. Every + // other double in this file is duck-typed and never reaches + // `Plugin.attachContext`, so this is the only coverage that the two-case + // contract holds for a REAL plugin: a context-less attach binds telemetry, + // flips isReady, and leaves the cache unbound — without throwing (a + // *supplied* cache-less context is the case that throws). If the call site + // ever changed to trip that throw, only this test would fail. + const attached: RealPlugin[] = []; + + class RealPlugin extends Plugin implements ToolProvider { + static manifest = { + name: "real", + displayName: "Real", + version: "0.0.0", + description: "A real Plugin subclass for the standalone path", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"real">; + + async setup(): Promise { + attached.push(this); + } + getAgentTools(): AgentToolDefinition[] { + return []; + } + async executeAgentTool(): Promise { + return null; + } + /** `cache` is protected; read it in-class so the test can assert it. */ + readCache(): unknown { + return this.cache; + } + ready(): boolean { + return this.isReady; + } + } + + const def = createAgent({ + instructions: "x", + model: scriptedAdapter([{ type: "message_delta", content: "ok" }]), + }); + const pluginData: PluginData = { + plugin: RealPlugin as unknown as PluginConstructor, + config: {}, + name: "real", + }; + + await expect( + runAgent(def, { messages: "hi", plugins: [pluginData] }), + ).resolves.toBeDefined(); + + // attachContext({}) + setup() both ran, without throwing. + expect(attached).toHaveLength(1); + expect(attached[0].ready()).toBe(true); + // App-less: the cache is unbound, so a direct read fails closed rather than + // handing back undefined. + expect(() => attached[0].readCache()).toThrow(InitializationError); + }); + test("sub-agent recursion shares the same plugin instance with the parent", async () => { // Regression: providerCache used to be per-call inside // buildStandaloneToolIndex, so each nested runAgent constructed fresh diff --git a/packages/appkit/src/core/appkit.ts b/packages/appkit/src/core/appkit.ts index 0ccfd64e9..1cedb2125 100644 --- a/packages/appkit/src/core/appkit.ts +++ b/packages/appkit/src/core/appkit.ts @@ -32,10 +32,17 @@ export class AppKit { #setupPromises: Promise[] = []; #context: PluginContext; - private constructor(config: { plugins: TPlugins }) { + /** + * @param context - The app's plugin context, carrying this app's per-app + * services. A separate parameter, never a `config` key: a `CacheManager` + * reaching plugin config would be deep-merged into what + * `PluginExecuteConfig.cache` declares as a `CacheConfig` and break the + * cache interceptor's gate. `appkit-cache-injection.test.ts` pins this. + */ + private constructor(config: { plugins: TPlugins }, context: PluginContext) { const { plugins, ...globalConfig } = config; - this.#context = new PluginContext(); + this.#context = context; const pluginEntries = Object.entries(plugins); @@ -192,63 +199,80 @@ export class AppKit { disableInternalTelemetry?: boolean; } = {}, ): Promise> { - // Initialize core services + // Initialize core services. Telemetry first: the CacheManager constructor + // pulls a telemetry provider. The cache stays an await here — `create()` + // builds its own workspace client before ServiceContext.initialize() runs, + // so it cannot move into the synchronous AppKit constructor. TelemetryManager.initialize(config?.telemetry); - await CacheManager.getInstance(config?.cache); - - const withDefaults = AppKit.withDefaultPlugins(config.plugins as T); - const rawPlugins = AppKit.filterDevOnlyPlugins(withDefaults); - - // Collect manifest resources via registry - const registry = new ResourceRegistry(); - registry.collectResources(rawPlugins); - - // Derive ServiceContext needs from what manifests declared - const needsWarehouse = registry - .getRequired() - .some((r) => r.type === ResourceType.SQL_WAREHOUSE); - await ServiceContext.initialize( - { warehouseId: needsWarehouse }, - config?.client, - ); - - // Validate env vars - registry.enforceValidation(); - - const preparedPlugins = AppKit.preparePlugins(rawPlugins); - const mergedConfig = { - plugins: preparedPlugins, - }; - - const instance = new AppKit(mergedConfig); - - await Promise.all(instance.#setupPromises); - await instance.#context.emitLifecycle("setup:complete"); - - const handle = instance as unknown as PluginMap; + const cache = await CacheManager.create(config?.cache); + + // Everything past the manager's construction runs guarded: the app owns the + // manager now, so a failed boot has to close it. Nothing else holds a + // reference, and a manager that resolved to Lakebase owns a `pg.Pool` that + // would otherwise never be ended — one leaked pool per failed boot, holding + // the event loop open. + try { + const withDefaults = AppKit.withDefaultPlugins(config.plugins as T); + const rawPlugins = AppKit.filterDevOnlyPlugins(withDefaults); + + // Collect manifest resources via registry + const registry = new ResourceRegistry(); + registry.collectResources(rawPlugins); + + // Derive ServiceContext needs from what manifests declared + const needsWarehouse = registry + .getRequired() + .some((r) => r.type === ResourceType.SQL_WAREHOUSE); + await ServiceContext.initialize( + { warehouseId: needsWarehouse }, + config?.client, + ); + + // Validate env vars + registry.enforceValidation(); + + const instance = new AppKit( + { plugins: AppKit.preparePlugins(rawPlugins) }, + new PluginContext({ cache }), + ); + + await Promise.all(instance.#setupPromises); + await instance.#context.emitLifecycle("setup:complete"); + + const handle = instance as unknown as PluginMap; + + if (config.onPluginsReady) { + logger.debug("Running onPluginsReady hook"); + await config.onPluginsReady(handle); + logger.debug("onPluginsReady hook completed"); + } - if (config.onPluginsReady) { - logger.debug("Running onPluginsReady hook"); - await config.onPluginsReady(handle); - logger.debug("onPluginsReady hook completed"); - } + if (isInternalTelemetryEnabled(config)) { + AppKit.bootstrapInternalTelemetry(); + } - if (isInternalTelemetryEnabled(config)) { - AppKit.bootstrapInternalTelemetry(); - } + const serverPlugin = instance.#pluginInstances.server; + if (serverPlugin && typeof (serverPlugin as any).start === "function") { + await (serverPlugin as any).start(); + } - const serverPlugin = instance.#pluginInstances.server; - if (serverPlugin && typeof (serverPlugin as any).start === "function") { - await (serverPlugin as any).start(); + // Core owns graceful shutdown: install the signal handlers once every + // plugin has started. Applies uniformly whether or not a server plugin + // is present — server-less apps still get their telemetry flushed and + // plugin shutdown() hooks run. + new LifecycleManager(instance.#context, cache).installSignalHandlers(); + + return handle; + } catch (err) { + // Never mask the boot error with a teardown failure. + await cache.close().catch((closeErr) => { + logger.error( + "Error closing the cache after a failed boot: %O", + closeErr, + ); + }); + throw err; } - - // Core owns graceful shutdown: install the signal handlers once every - // plugin has started. Applies uniformly whether or not a server plugin - // is present — server-less apps still get their telemetry flushed and - // plugin shutdown() hooks run. - new LifecycleManager(instance.#context).installSignalHandlers(); - - return handle; } private static bootstrapInternalTelemetry(): void { diff --git a/packages/appkit/src/core/lifecycle-manager.ts b/packages/appkit/src/core/lifecycle-manager.ts index 84dcb4a9e..a990fb42c 100644 --- a/packages/appkit/src/core/lifecycle-manager.ts +++ b/packages/appkit/src/core/lifecycle-manager.ts @@ -58,7 +58,14 @@ export class LifecycleManager { */ private shutdownPhase = "not started"; - constructor(private readonly context: PluginContext) {} + /** + * @param cache - This app's cache, by reference. Each `createApp` builds its + * own, so shutdown must close this app's manager specifically. + */ + constructor( + private readonly context: PluginContext, + private readonly cache: CacheManager, + ) {} /** * Install the SIGTERM/SIGINT handlers that trigger {@link shutdown}. @@ -188,18 +195,11 @@ export class LifecycleManager { process.exit(exitCode); } - /** Close the cache storage, bounded and error-isolated. */ + /** Close this app's cache storage, bounded and error-isolated. */ private async closeCacheStorage(): Promise { - let cache: CacheManager; - try { - cache = CacheManager.getInstanceSync(); - } catch { - // Cache was never initialized — nothing to close. - return; - } try { await this.raceWithTimeout( - cache.close(), + this.cache.close(), LifecycleManager.PHASE_SHUTDOWN_TIMEOUT_MS, "cache storage close", ); diff --git a/packages/appkit/src/core/plugin-context.ts b/packages/appkit/src/core/plugin-context.ts index 4f86a5189..09c6b2ae8 100644 --- a/packages/appkit/src/core/plugin-context.ts +++ b/packages/appkit/src/core/plugin-context.ts @@ -1,6 +1,7 @@ import type express from "express"; import type { BasePlugin, IAppRequest, ToolProvider } from "shared"; +import type { CacheManager } from "../cache"; import { createLogger } from "../logging/logger"; import { type ITelemetry, @@ -69,6 +70,15 @@ export class PluginContext { >(); private telemetry: ITelemetry; + /** + * This app's cache. `readonly` so nothing can swap an app's cache after the + * context is built, and required so a context cannot exist without one — the + * two halves that make "exactly one manager per app" a compiler-checked + * invariant. The app-less case is context-less (`attachContext({})`, the + * standalone `runAgent` path), not a cache-less context. + */ + readonly cache: CacheManager; + /** * @param deps.telemetry - Telemetry provider used for `executeTool` spans. * Defaults to the shared `"plugin-context"` provider — the production @@ -76,10 +86,13 @@ export class PluginContext { * `executeTool` without a live OpenTelemetry pipeline. This is the only * seam the mock context needs; route buffering and the tool registry are * exercised through the existing public API. + * @param deps.cache - The manager `_createApp` built for this app. Every + * plugin in the app binds `this.cache` to this object. */ - constructor(deps: { telemetry?: ITelemetry } = {}) { + constructor(deps: { telemetry?: ITelemetry; cache: CacheManager }) { this.telemetry = deps.telemetry ?? TelemetryManager.getProvider("plugin-context"); + this.cache = deps.cache; } /** diff --git a/packages/appkit/src/core/tests/appkit-as-user-exports.test.ts b/packages/appkit/src/core/tests/appkit-as-user-exports.test.ts index 45467625d..a1c7346c1 100644 --- a/packages/appkit/src/core/tests/appkit-as-user-exports.test.ts +++ b/packages/appkit/src/core/tests/appkit-as-user-exports.test.ts @@ -13,31 +13,6 @@ import type { UserContext } from "../../context/user-context"; // ── Mock heavy dependencies ───────────────────────────────────────── -vi.mock("../../cache", () => ({ - CacheManager: { - getInstance: vi.fn(async () => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - vi.mock("../../telemetry", async () => { const actual = await vi.importActual("../../telemetry"); diff --git a/packages/appkit/src/core/tests/appkit-cache-injection.test.ts b/packages/appkit/src/core/tests/appkit-cache-injection.test.ts new file mode 100644 index 000000000..9a42a3aaa --- /dev/null +++ b/packages/appkit/src/core/tests/appkit-cache-injection.test.ts @@ -0,0 +1,281 @@ +import type { PluginManifest } from "shared"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +import { CacheManager } from "../../cache"; +import { InMemoryStorage } from "../../cache/storage"; +import { Plugin, toPlugin } from "../../plugin"; +import { mockServiceContext, setupDatabricksEnv } from "../../testing"; +import { PluginContext } from "../plugin-context"; + +// Each app owns one real `CacheManager`, reached through its own +// `PluginContext` — the cache module is not mocked (a fake would assert +// nothing here). Every boot passes `cache: { storage }`, without which +// `create()` probes Lakebase. + +/** Instances registered by `setup()`, so tests can read a real plugin's cache. */ +const constructed: CacheProbe[] = []; + +/** Shared probe behaviour. Each concrete plugin declares its own manifest — a + * subclass cannot narrow the static side, so they are siblings, not a chain. */ +abstract class CacheProbe extends Plugin { + override async setup() { + constructed.push(this); + } + + /** `cache` is protected, so the read has to happen in-class. */ + boundCache(): CacheManager { + return this.cache; + } + + /** Proves the manager never reaches plugin config — see the KTD4 gate test. */ + configCache(): unknown { + return (this.config as Record).cache; + } +} + +class ProbePlugin extends CacheProbe { + static manifest = { + name: "probe", + displayName: "Probe", + version: "0.0.0", + description: "Reports the cache it was bound to", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"probe">; +} + +class ProbeTwoPlugin extends CacheProbe { + static manifest = { + name: "probeTwo", + displayName: "Probe Two", + version: "0.0.0", + description: "A second cache-using plugin in the same app", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"probeTwo">; +} + +const probe = toPlugin(ProbePlugin); +const probeTwo = toPlugin(ProbeTwoPlugin); + +/** Managers `create()` built, in boot order — captured by spying on `create` + * so a test can assert each app resolved its own. */ +const built: CacheManager[] = []; +const realCreate = CacheManager.create.bind(CacheManager); + +/** Read a manager's private field without adding a public accessor. */ +function privateField(manager: CacheManager, key: string): T { + return (manager as unknown as Record)[key]; +} + +describe("per-app CacheManager injection", () => { + let serviceContextMock: ReturnType; + + beforeEach(() => { + constructed.length = 0; + built.length = 0; + setupDatabricksEnv(); + serviceContextMock = mockServiceContext(); + vi.spyOn(CacheManager, "create").mockImplementation(async (userConfig) => { + const manager = await realCreate(userConfig); + vi.spyOn(manager, "close"); + built.push(manager); + return manager; + }); + }); + + afterEach(() => { + serviceContextMock.restore(); + vi.restoreAllMocks(); + }); + + /** Boot one app offline and return it with the manager it built. */ + async function bootApp( + plugins: unknown[], + cache: Record = {}, + ) { + const { createApp } = await import("../appkit"); + const handle = await createApp({ + plugins, + cache: { storage: new InMemoryStorage(), ...cache }, + } as never); + return { handle, manager: built[built.length - 1] }; + } + + test("two apps in one process hold different managers", async () => { + const first = await bootApp([probe({})]); + const second = await bootApp([probe({})]); + + expect(second.manager).not.toBe(first.manager); + }); + + test("the second app's cache config is honoured, not discarded", async () => { + const first = await bootApp([probe({})], { ttl: 60 }); + const second = await bootApp([probe({})], { ttl: 3600 }); + + // A shared manager would make B silently inherit A's ttl; each keeps its own. + expect(privateField<{ ttl?: number }>(first.manager, "config").ttl).toBe( + 60, + ); + expect(privateField<{ ttl?: number }>(second.manager, "config").ttl).toBe( + 3600, + ); + }); + + test("the app's manager uses the storage the caller supplied", async () => { + const storage = new InMemoryStorage(); + const { manager } = await bootApp([probe({})], { storage }); + + expect(privateField(manager, "storage")).toBe(storage); + }); + + test("every plugin in one app resolves that app's own manager", async () => { + // Booted second on purpose: a plugin that resolved its cache from anywhere + // but its own context would get the *first* app's cache, and this assertion + // would fail. + await bootApp([probe({})]); + const { manager } = await bootApp([probe({}), probeTwo({})]); + + const second = constructed.slice(-2); + expect(second).toHaveLength(2); + for (const plugin of second) { + expect(plugin.boundCache()).toBe(manager); + } + }); + + test("the manager never reaches plugin config", async () => { + await bootApp([probe({})]); + + // A CacheManager here would be deep-merged into execute options, where + // `PluginExecuteConfig.cache` is declared a `CacheConfig` — silently + // breaking the cache interceptor's gate. + expect(constructed[0].configCache()).not.toBeInstanceOf(CacheManager); + }); + + test("PluginContext exposes the cache it was given, and it cannot be swapped", () => { + const cache = CacheManager.forStorage(new InMemoryStorage()); + const context = new PluginContext({ cache }); + + expect(context.cache).toBe(cache); + // @ts-expect-error `cache` is readonly: an app's cache cannot be replaced. + context.cache = CacheManager.forStorage(new InMemoryStorage()); + }); + + test("a consumer has no way to construct a manager", () => { + // @ts-expect-error the constructor is private — `create` and `forStorage` + // are the only entries, which is what makes one-manager-per-app checkable. + void new CacheManager(new InMemoryStorage(), {} as never); + }); +}); + +describe("a failed boot closes the manager it built", () => { + let serviceContextMock: ReturnType; + + beforeEach(() => { + built.length = 0; + constructed.length = 0; + setupDatabricksEnv(); + serviceContextMock = mockServiceContext(); + vi.spyOn(CacheManager, "create").mockImplementation(async (userConfig) => { + const manager = await realCreate(userConfig); + vi.spyOn(manager, "close"); + built.push(manager); + return manager; + }); + }); + + afterEach(() => { + serviceContextMock.restore(); + vi.restoreAllMocks(); + }); + + async function failedBoot(config: Record) { + const { createApp } = await import("../appkit"); + await expect( + createApp({ + cache: { storage: new InMemoryStorage() }, + ...config, + } as never), + ).rejects.toThrow(); + } + + /** + * Nothing else holds a reference once the boot unwinds, so an unclosed manager + * is unreachable — and one that resolved to Lakebase owns a `pg.Pool` that + * would never be ended. + */ + test("when onPluginsReady throws", async () => { + await failedBoot({ + plugins: [probe({})], + onPluginsReady: () => { + throw new Error("boom"); + }, + }); + + expect(built).toHaveLength(1); + expect(built[0].close).toHaveBeenCalledTimes(1); + }); + + test("when a plugin's setup() rejects", async () => { + class FailingSetupPlugin extends CacheProbe { + static manifest = { + name: "failingSetup", + displayName: "Failing Setup", + version: "0.0.0", + description: "Rejects during setup", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"failingSetup">; + + override async setup() { + throw new Error("setup failed"); + } + } + + await failedBoot({ plugins: [toPlugin(FailingSetupPlugin)({})] }); + + expect(built[0].close).toHaveBeenCalledTimes(1); + }); + + test("when attaching a plugin throws inside the AppKit constructor", async () => { + // The site `attachContext`'s own cache guard creates. + const attach = vi + .spyOn(Plugin.prototype, "attachContext") + .mockImplementation(() => { + throw new Error("attach failed"); + }); + try { + await failedBoot({ plugins: [probe({})] }); + expect(built[0].close).toHaveBeenCalledTimes(1); + } finally { + attach.mockRestore(); + } + }); + + test("a boot that fails before the manager exists attempts no close", async () => { + vi.spyOn(CacheManager, "create").mockRejectedValueOnce( + new Error("cache construction failed"), + ); + + await failedBoot({ plugins: [probe({})] }); + + expect(built).toHaveLength(0); + }); + + test("the boot error is not masked by a failing close", async () => { + vi.spyOn(CacheManager, "create").mockImplementationOnce(async (cfg) => { + const manager = await realCreate(cfg); + vi.spyOn(manager, "close").mockRejectedValue(new Error("close failed")); + built.push(manager); + return manager; + }); + + const { createApp } = await import("../appkit"); + await expect( + createApp({ + plugins: [probe({})], + cache: { storage: new InMemoryStorage() }, + onPluginsReady: () => { + throw new Error("the real cause"); + }, + } as never), + ).rejects.toThrow("the real cause"); + }); +}); diff --git a/packages/appkit/src/core/tests/databricks.test.ts b/packages/appkit/src/core/tests/databricks.test.ts index f5cce386f..0fc4a4c8e 100644 --- a/packages/appkit/src/core/tests/databricks.test.ts +++ b/packages/appkit/src/core/tests/databricks.test.ts @@ -42,24 +42,6 @@ vi.mock("../utils", () => ({ deepMerge: vi.fn((a, b) => ({ ...a, ...b })), })); -// Mock CacheManager -vi.mock("@databricks-apps/cache", () => ({ - CacheManager: { - getInstance: vi.fn().mockResolvedValue({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn(), - }), - getInstanceSync: vi.fn().mockReturnValue({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn(), - }), - }, -})); - // Test plugin classes for different phases class CoreTestPlugin implements BasePlugin { static DEFAULT_CONFIG = { coreDefault: "core-value" }; diff --git a/packages/appkit/src/core/tests/lifecycle-manager.test.ts b/packages/appkit/src/core/tests/lifecycle-manager.test.ts index 121e7eb5c..61d0bd6ae 100644 --- a/packages/appkit/src/core/tests/lifecycle-manager.test.ts +++ b/packages/appkit/src/core/tests/lifecycle-manager.test.ts @@ -9,15 +9,9 @@ import { vi, } from "vitest"; -// Mock core singletons before importing the subject under test. -vi.mock("../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn().mockReturnValue({ - close: vi.fn().mockResolvedValue(undefined), - }), - }, -})); - +// Mock core singletons before importing the subject under test. The cache is +// not among them: `LifecycleManager` takes this app's manager as a dependency, +// so each test passes a double directly. vi.mock("../../telemetry", () => ({ TelemetryManager: { getInstance: vi.fn().mockReturnValue({ @@ -35,6 +29,11 @@ vi.mock("../../internal-telemetry", () => ({ }, })); +/** A stand-in for this app's manager; only `close()` is exercised here. */ +function cacheDouble(close = vi.fn().mockResolvedValue(undefined)) { + return { close } as unknown as import("../../cache").CacheManager; +} + const { mockLoggerError } = vi.hoisted(() => ({ mockLoggerError: vi.fn(), })); @@ -48,14 +47,13 @@ vi.mock("../../logging/logger", () => ({ }), })); -import { CacheManager } from "../../cache"; import { TelemetryReporter } from "../../internal-telemetry"; import { TelemetryManager } from "../../telemetry"; import { LifecycleManager } from "../lifecycle-manager"; import { PluginContext } from "../plugin-context"; function contextWithPlugins(plugins: Record>) { - const ctx = new PluginContext(); + const ctx = new PluginContext({ cache: cacheDouble() }); for (const [name, instance] of Object.entries(plugins)) { ctx.registerPlugin(name, instance as BasePlugin); } @@ -99,7 +97,7 @@ describe("LifecycleManager", () => { "no-hooks": { name: "no-hooks" }, }); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager(ctx, cacheDouble()).shutdown(); expect(shutdownA).toHaveBeenCalledTimes(1); expect(shutdownB).toHaveBeenCalledTimes(1); @@ -122,7 +120,7 @@ describe("LifecycleManager", () => { }, }); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager(ctx, cacheDouble()).shutdown(); expect(stop).toHaveBeenCalledTimes(1); expect(order).toEqual(["reporter-stop", "abort"]); @@ -139,7 +137,7 @@ describe("LifecycleManager", () => { bad: { name: "bad", abortActiveOperations: badAbort }, }); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager(ctx, cacheDouble()).shutdown(); expect(okAbort).toHaveBeenCalledTimes(1); expect(badAbort).toHaveBeenCalledTimes(1); @@ -159,7 +157,7 @@ describe("LifecycleManager", () => { healthy: { name: "healthy", shutdown: healthy }, }); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager(ctx, cacheDouble()).shutdown(); expect(failing).toHaveBeenCalledTimes(1); expect(healthy).toHaveBeenCalledTimes(1); @@ -180,7 +178,7 @@ describe("LifecycleManager", () => { fast: { name: "fast", shutdown: fast }, }); - const done = new LifecycleManager(ctx).shutdown(); + const done = new LifecycleManager(ctx, cacheDouble()).shutdown(); await vi.advanceTimersByTimeAsync(10_000); await done; @@ -202,7 +200,7 @@ describe("LifecycleManager", () => { const hook = vi.fn(); ctx.onLifecycle("shutdown", hook); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager(ctx, cacheDouble()).shutdown(); expect(hook).toHaveBeenCalledTimes(1); expect(exitSpy).toHaveBeenCalledWith(0); @@ -213,7 +211,7 @@ describe("LifecycleManager", () => { const ctx = contextWithPlugins({ a: { name: "a", shutdown: shutdownHook }, }); - const manager = new LifecycleManager(ctx); + const manager = new LifecycleManager(ctx, cacheDouble()); await Promise.all([manager.shutdown(), manager.shutdown()]); await manager.shutdown(); @@ -224,14 +222,14 @@ describe("LifecycleManager", () => { test("closes the cache storage and flushes telemetry", async () => { const close = vi.fn().mockResolvedValue(undefined); const flush = vi.fn().mockResolvedValue(undefined); - vi.mocked(CacheManager.getInstanceSync).mockReturnValueOnce({ - close, - } as any); vi.mocked(TelemetryManager.getInstance).mockReturnValueOnce({ shutdown: flush, } as any); - await new LifecycleManager(contextWithPlugins({})).shutdown(); + await new LifecycleManager( + contextWithPlugins({}), + cacheDouble(close), + ).shutdown(); expect(close).toHaveBeenCalledTimes(1); expect(flush).toHaveBeenCalledTimes(1); @@ -245,7 +243,10 @@ describe("LifecycleManager", () => { shutdown: hangingFlush, } as any); - const done = new LifecycleManager(contextWithPlugins({})).shutdown(); + const done = new LifecycleManager( + contextWithPlugins({}), + cacheDouble(), + ).shutdown(); await vi.advanceTimersByTimeAsync(2_000); await done; @@ -263,11 +264,11 @@ describe("LifecycleManager", () => { test("a hanging cache close cannot hang shutdown — the close timeout still exits 0", async () => { vi.useFakeTimers(); const hangingClose = vi.fn(() => new Promise(() => {})); - vi.mocked(CacheManager.getInstanceSync).mockReturnValueOnce({ - close: hangingClose, - } as any); - const done = new LifecycleManager(contextWithPlugins({})).shutdown(); + const done = new LifecycleManager( + contextWithPlugins({}), + cacheDouble(hangingClose), + ).shutdown(); await vi.advanceTimersByTimeAsync(2_000); await done; @@ -282,21 +283,6 @@ describe("LifecycleManager", () => { expect(exitSpy).toHaveBeenCalledWith(0); }); - test("a never-initialized cache is skipped without error", async () => { - vi.mocked(CacheManager.getInstanceSync).mockImplementationOnce(() => { - throw new Error("cache not initialized"); - }); - - await new LifecycleManager(contextWithPlugins({})).shutdown(); - - expect( - mockLoggerError.mock.calls.some((c) => - String(c[0]).includes("Error closing cache storage"), - ), - ).toBe(false); - expect(exitSpy).toHaveBeenCalledWith(0); - }); - test("runs phases in order: abort → plugin hooks → lifecycle emit → cache close + flush (concurrent) → exit", async () => { const order: string[] = []; const ctx = contextWithPlugins({ @@ -313,11 +299,6 @@ describe("LifecycleManager", () => { ctx.onLifecycle("shutdown", () => { order.push("lifecycle"); }); - vi.mocked(CacheManager.getInstanceSync).mockReturnValueOnce({ - close: vi.fn(async () => { - order.push("cache-close"); - }), - } as any); vi.mocked(TelemetryManager.getInstance).mockReturnValueOnce({ shutdown: vi.fn(async () => { order.push("flush"); @@ -327,7 +308,14 @@ describe("LifecycleManager", () => { order.push("exit"); }) as any); - await new LifecycleManager(ctx).shutdown(); + await new LifecycleManager( + ctx, + cacheDouble( + vi.fn(async () => { + order.push("cache-close"); + }), + ), + ).shutdown(); expect(order.slice(0, 3)).toEqual([ "abort", @@ -354,7 +342,7 @@ describe("LifecycleManager", () => { late: { name: "late", shutdown: lateRejecting }, }); - const done = new LifecycleManager(ctx).shutdown(); + const done = new LifecycleManager(ctx, cacheDouble()).shutdown(); await vi.advanceTimersByTimeAsync(10_000); await done; @@ -371,7 +359,7 @@ describe("LifecycleManager", () => { test("registers SIGTERM/SIGINT once and triggers shutdown", async () => { const onceSpy = vi.spyOn(process, "once"); const ctx = contextWithPlugins({}); - const manager = new LifecycleManager(ctx); + const manager = new LifecycleManager(ctx, cacheDouble()); manager.installSignalHandlers(); diff --git a/packages/appkit/src/core/tests/plugin-context.test.ts b/packages/appkit/src/core/tests/plugin-context.test.ts index 194056635..278d4ddf4 100644 --- a/packages/appkit/src/core/tests/plugin-context.test.ts +++ b/packages/appkit/src/core/tests/plugin-context.test.ts @@ -1,8 +1,12 @@ import type { AgentToolDefinition } from "shared"; import { beforeEach, describe, expect, test, vi } from "vitest"; +import type { CacheManager } from "../../cache"; import { isToolProvider, PluginContext } from "../plugin-context"; +/** A CacheManager these context tests carry but never exercise. */ +const cacheStub = {} as unknown as CacheManager; + /** * Holds the most recent mock span instance so tests can assert against * `setStatus` / `recordException` / `end` calls without relying on global @@ -65,7 +69,7 @@ describe("PluginContext", () => { let ctx: PluginContext; beforeEach(() => { - ctx = new PluginContext(); + ctx = new PluginContext({ cache: cacheStub }); }); describe("route buffering", () => { diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 7dbd7a99c..c5d6c2c00 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -15,10 +15,14 @@ import type { import { camelToKebab } from "shared"; import { AppManager } from "../app"; -import { CacheManager } from "../cache"; +import type { CacheManager } from "../cache"; import { getCurrentUserId, runInUserContext, ServiceContext } from "../context"; import type { PluginContext } from "../core/plugin-context"; -import { AppKitError, AuthenticationError } from "../errors"; +import { + AppKitError, + AuthenticationError, + InitializationError, +} from "../errors"; import { createLogger } from "../logging/logger"; import { StreamManager } from "../stream"; import { @@ -220,7 +224,12 @@ export abstract class Plugin< TConfig extends BasePluginConfig = BasePluginConfig, > implements BasePlugin { protected isReady = false; - protected cache!: CacheManager; + /** + * Backing field for {@link cache}. A plain private property rather than a + * `#private` one: `asUser` hands callers a `Proxy` over the plugin, and a + * `#private` read through a proxy receiver throws. + */ + private _cache?: CacheManager; protected app: AppManager; protected devFileReader: DevFileReader; protected streamManager: StreamManager; @@ -246,6 +255,27 @@ export abstract class Plugin< */ name: string; + /** + * This app's cache, bound by {@link attachContext}. Every plugin in an app + * shares the one manager the app built; a plugin cannot substitute its own, + * so set a per-plugin `cache: { enabled, ttl }` config instead of assigning. + * + * Throws `InitializationError` when read on an unattached plugin (the app-less + * `runAgent` path, or a plugin built by hand in a test). This is the guard for + * the direct readers — `analytics.ts`, `files/plugin.ts` — that reach the + * cache without going through {@link execute}: they now fail with a named + * error at the read rather than a bare `TypeError` deeper in a handler. + */ + protected get cache(): CacheManager { + if (!this._cache) { + throw InitializationError.notInitialized( + "CacheManager", + `Plugin "${this.name}" read this.cache before it was attached to an app. Register the plugin through createApp(), or attach it to a test context first.`, + ); + } + return this._cache; + } + constructor(protected config: TConfig) { this.name = config.name ?? @@ -258,34 +288,26 @@ export abstract class Plugin< | PluginContext | undefined; - // Eagerly bind telemetry + cache if the core services have already been - // initialized (normal createApp path, or tests that mock CacheManager). - // If they haven't, we leave these undefined and rely on `attachContext` - // being called later — this lets factories eagerly construct plugin - // instances at module top-level before `createApp` has run. - this.tryAttachContext(); - } - - private tryAttachContext(): void { - try { - this.cache = CacheManager.getInstanceSync(); - } catch { - return; - } + // `getProvider` routes through a lazily-constructed manager and never + // throws, so a plugin factory evaluated at module top level — before + // `createApp` has run — still gets a usable `this.telemetry`. this.telemetry = TelemetryManager.getProvider( this.name, this.config.telemetry, ); - this.isReady = true; } /** * Binds runtime dependencies (telemetry provider, cache, plugin context) to * this plugin. Called by `AppKit._createApp` after construction and before - * `setup()`. Idempotent: safe to call if the constructor already bound them - * eagerly. Kept separate so factories can eagerly construct plugin instances - * without running this before `TelemetryManager.initialize()` / - * `CacheManager.getInstance()` have run. + * `setup()`. Kept separate from the constructor so plugin factories can be + * evaluated at module top level, before any app exists. + * + * A context-less `attachContext({})` is the app-less path (see `runAgent`): + * it binds telemetry and leaves the cache unbound, so only a cached execution + * fails, at the chokepoint in {@link _buildInterceptors}. A supplied context + * always carries a cache — `PluginContext.cache` is required, so a cache-less + * one cannot be constructed. */ attachContext( deps: { @@ -293,16 +315,16 @@ export abstract class Plugin< telemetryConfig?: BasePluginConfig["telemetry"]; } = {}, ): void { - if (!this.cache) { - this.cache = CacheManager.getInstanceSync(); + if (deps.context !== undefined) { + this.context = deps.context as PluginContext; } + // The app's own cache, and every plugin in the app gets the same one; only + // the context-less app-less path leaves it undefined. + this._cache = this.context?.cache; this.telemetry = TelemetryManager.getProvider( this.name, deps.telemetryConfig ?? this.config.telemetry, ); - if (deps.context !== undefined) { - this.context = deps.context as PluginContext; - } this.isReady = true; } @@ -610,8 +632,6 @@ export abstract class Plugin< ): Promise> { const executeConfig = this._buildExecutionConfig(options); - const interceptors = this._buildInterceptors(executeConfig); - // get user key from context if not provided const effectiveUserKey = userKey ?? getCurrentUserId(); @@ -621,6 +641,11 @@ export abstract class Plugin< }; try { + // Inside the try: building the chain can fail — an unattached plugin has + // no cache to give the cache interceptor — and this method's contract is + // to report failures as a result, never to throw. + const interceptors = this._buildInterceptors(executeConfig); + const data = await this._executeWithInterceptors( fn, interceptors, @@ -722,7 +747,17 @@ export abstract class Plugin< } if (options.cache?.enabled && options.cache.cacheKey?.length) { - interceptors.push(new CacheInterceptor(this.cache, options.cache)); + // Every cached execution routed through execute()/executeStream() passes + // here (direct `this.cache` readers are guarded by the accessor instead). + // The compiler cannot catch a plugin that never got `attachContext`, so + // without this the first symptom is a `TypeError` inside a request handler. + if (!this._cache) { + throw InitializationError.notInitialized( + "CacheManager", + `Plugin "${this.name}" requested a cached execution before attachContext() ran, so it has no cache. Register the plugin through createApp(), or attach it to a test context first.`, + ); + } + interceptors.push(new CacheInterceptor(this._cache, options.cache)); } return interceptors; diff --git a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts index 2566f2869..53d89cbf6 100644 --- a/packages/appkit/src/plugin/tests/asUser-proxy.test.ts +++ b/packages/appkit/src/plugin/tests/asUser-proxy.test.ts @@ -33,7 +33,6 @@ import { } from "vitest"; import { AppManager } from "../../app"; -import { CacheManager } from "../../cache"; import { getUserContext } from "../../context/execution-context"; import { ServiceContext } from "../../context/service-context"; import { AuthenticationError } from "../../errors/authentication"; @@ -54,9 +53,6 @@ vi.mock("../../workspace-client", async (importOriginal) => { }); vi.mock("../../app"); -vi.mock("../../cache", () => ({ - CacheManager: { getInstanceSync: vi.fn() }, -})); vi.mock("../../stream"); vi.mock("../../telemetry", () => ({ TelemetryManager: { getProvider: vi.fn() }, @@ -207,7 +203,6 @@ function createReqWithToken(forwardedToken: string): express.Request { describe("Plugin.asUser proxy", () => { let mockTelemetry: ITelemetry; - let mockCache: CacheManager; let serviceContextMock: Awaited>; let config: BasePluginConfig; let contextManager: ContextManager; @@ -227,13 +222,6 @@ describe("Plugin.asUser proxy", () => { serviceContextMock = await mockServiceContext(); mockTelemetry = createMockTelemetry(); - mockCache = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - } as unknown as CacheManager; - - vi.mocked(CacheManager.getInstanceSync).mockReturnValue(mockCache); vi.mocked(AppManager).mockImplementation( () => ({ getAppQuery: vi.fn() }) as unknown as AppManager, ); diff --git a/packages/appkit/src/plugin/tests/cache-binding.test.ts b/packages/appkit/src/plugin/tests/cache-binding.test.ts new file mode 100644 index 000000000..4849a68eb --- /dev/null +++ b/packages/appkit/src/plugin/tests/cache-binding.test.ts @@ -0,0 +1,158 @@ +import type { PluginManifest } from "shared"; +import { describe, expect, test } from "vitest"; + +import { CacheManager } from "../../cache"; +import { InMemoryStorage } from "../../cache/storage"; +import { PluginContext } from "../../core/plugin-context"; +import { InitializationError } from "../../errors"; +import { mockServiceContext } from "../../testing"; +import { Plugin } from "../plugin"; + +// This file never boots an app, so an unattached plugin here is genuinely +// cache-less — there is nothing to fall back to. + +class ProbePlugin extends Plugin { + static manifest = { + name: "probe", + displayName: "Probe", + version: "0.0.0", + description: "Reports how its cache was bound", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"probe">; + + /** `cache` is protected, so the read has to happen in-class. */ + boundCache(): CacheManager { + return this.cache; + } + + ready(): boolean { + return this.isReady; + } + + telemetryProvider(): unknown { + return this.telemetry; + } + + /** Drives the interceptor chain the way a handler would. */ + runCached(): Promise { + return this.execute(async () => "value", { + default: { cache: { enabled: true, cacheKey: ["probe"], ttl: 60 } }, + }); + } + + runUncached(): Promise { + return this.execute(async () => "value", { + default: { cache: { enabled: false } }, + }); + } +} + +function contextWithCache() { + const cache = CacheManager.forStorage(new InMemoryStorage()); + return { cache, context: new PluginContext({ cache }) }; +} + +describe("Plugin cache binding", () => { + test("an app-less plugin constructs and still has telemetry", () => { + const plugin = new ProbePlugin({}); + + expect(plugin.telemetryProvider()).toBeDefined(); + }); + + test("an app-less plugin is not ready until it is attached", () => { + const plugin = new ProbePlugin({}); + expect(plugin.ready()).toBe(false); + + plugin.attachContext({ context: contextWithCache().context }); + expect(plugin.ready()).toBe(true); + }); + + test("attachContext binds the cache the context carries", () => { + const { cache, context } = contextWithCache(); + const plugin = new ProbePlugin({}); + + plugin.attachContext({ context }); + + expect(plugin.boundCache()).toBe(cache); + }); + + test("two plugins attached to one context share its cache", () => { + const { cache, context } = contextWithCache(); + const first = new ProbePlugin({}); + const second = new ProbePlugin({}); + + first.attachContext({ context }); + second.attachContext({ context }); + + expect(first.boundCache()).toBe(cache); + expect(second.boundCache()).toBe(first.boundCache()); + }); + + test("a context-less attachContext is the app-less path, not an error", () => { + const plugin = new ProbePlugin({}); + + // The standalone `runAgent` path (core/agent/run-agent.ts). It must bind + // telemetry and leave the cache unbound rather than refuse. + expect(() => plugin.attachContext({})).not.toThrow(); + expect(() => plugin.attachContext({ context: undefined })).not.toThrow(); + }); + + test("a cache-less context cannot be constructed", () => { + // The previous runtime guard is now a compile-time one: `PluginContext.cache` + // is required, so a context without a cache does not typecheck. This pins + // that the invariant is enforced by the compiler, not a runtime throw. + // @ts-expect-error cache is required on PluginContext + new PluginContext({}); + }); + + test("an unattached plugin's cached execution fails at the chokepoint", async () => { + const serviceContext = mockServiceContext(); + try { + const plugin = new ProbePlugin({}); + + const result = await plugin.runCached(); + + expect(result).toMatchObject({ ok: false }); + expect(JSON.stringify(result)).toContain("attachContext"); + } finally { + serviceContext.restore(); + } + }); + + test("an unattached plugin's direct this.cache read throws, not returns undefined", () => { + const plugin = new ProbePlugin({}); + + // `analytics.ts` and `files/plugin.ts` read `this.cache` directly, outside + // `execute()`, so the accessor — not the interceptor chain — is their guard. + expect(() => plugin.boundCache()).toThrow(InitializationError); + }); + + test("an unattached plugin still runs an uncached execution", async () => { + const serviceContext = mockServiceContext(); + try { + const plugin = new ProbePlugin({}); + + // The guard fires only when a cached path is actually requested. + await expect(plugin.runUncached()).resolves.toMatchObject({ ok: true }); + } finally { + serviceContext.restore(); + } + }); + + test("a plugin cannot substitute its own cache", () => { + class OwnCachePlugin extends ProbePlugin { + constructor() { + super({}); + // @ts-expect-error `cache` is a read-only accessor: every plugin in an + // app shares the one manager the app built. Use a per-plugin + // `cache: { enabled, ttl }` config instead. + this.cache = CacheManager.forStorage(new InMemoryStorage()); + } + } + + // The `@ts-expect-error` proves the compiler rejects it; this proves the + // getter-only accessor also throws at runtime, so a JS consumer cannot + // swap an app's cache either. + expect(() => new OwnCachePlugin()).toThrow(TypeError); + }); +}); diff --git a/packages/appkit/src/plugin/tests/plugin.test.ts b/packages/appkit/src/plugin/tests/plugin.test.ts index d42da93ad..008e913b6 100644 --- a/packages/appkit/src/plugin/tests/plugin.test.ts +++ b/packages/appkit/src/plugin/tests/plugin.test.ts @@ -23,7 +23,7 @@ import { } from "vitest"; import { AppManager } from "../../app"; -import { CacheManager } from "../../cache"; +import type { CacheManager } from "../../cache"; import { ServiceContext } from "../../context/service-context"; import { AuthenticationError, @@ -61,11 +61,6 @@ vi.mock("../../workspace-client", async (importOriginal) => { // Mock all dependencies vi.mock("../../app"); -vi.mock("../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(), - }, -})); vi.mock("../../stream"); vi.mock("../../utils", () => ({ deepMerge: vi.fn((a, b) => { @@ -125,8 +120,23 @@ vi.mock("../interceptors/telemetry", () => ({ })), })); +/** + * The cache the plugins below are attached to. Module-scoped because + * `TestPlugin`'s constructor closes over it; `beforeEach` reassigns it so each + * test gets a fresh double. + */ +let mockCache: CacheManager; + // Test plugin implementations class TestPlugin extends Plugin { + constructor(config: BasePluginConfig) { + super(config); + // A registered plugin gets its cache from the app through `attachContext`; + // doing the same here lets the direct constructions below behave like + // plugins an app owns. + this.attachContext({ context: { cache: mockCache } }); + } + async customMethod(value: string): Promise { return `processed-${value}`; } @@ -185,7 +195,6 @@ class OboTestPlugin extends Plugin { describe("Plugin", () => { let mockTelemetry: ITelemetry; - let mockCache: CacheManager; let mockApp: AppManager; let mockStreamManager: StreamManager; let config: BasePluginConfig; @@ -222,7 +231,6 @@ describe("Plugin", () => { }; // Setup constructor mocks - vi.mocked(CacheManager.getInstanceSync).mockReturnValue(mockCache); vi.mocked(AppManager).mockImplementation(() => mockApp); vi.mocked(StreamManager).mockImplementation(() => mockStreamManager); vi.mocked(TelemetryManager.getProvider).mockReturnValue( @@ -257,11 +265,17 @@ describe("Plugin", () => { test("should initialize managers", () => { new TestPlugin(config); - expect(CacheManager.getInstanceSync).toHaveBeenCalledTimes(1); expect(AppManager).toHaveBeenCalledTimes(1); expect(StreamManager).toHaveBeenCalledTimes(1); }); + test("binds the cache its context carries", () => { + const plugin = new TestPlugin(config); + + // @ts-expect-error - cache is protected + expect(plugin.cache).toBe(mockCache); + }); + test("should forward streamConfig to the StreamManager", () => { const streamConfig = { maxEventSize: 20 * 1024 * 1024 }; new TestPlugin({ ...config, streamConfig }); diff --git a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts index e57225dd2..45d75287a 100644 --- a/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts +++ b/packages/appkit/src/plugins/agents/tests/agents-plugin.test.ts @@ -12,7 +12,7 @@ import type { import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { z } from "zod"; -import { CacheManager } from "../../../cache"; +import type { CacheManager } from "../../../cache"; import { buildToolkitEntries } from "../../../core/agent/build-toolkit"; import { defineTool, @@ -24,6 +24,7 @@ import type { ToolkitEntry, } from "../../../core/agent/types"; import { isToolkitEntry } from "../../../core/agent/types"; +import { createTestPluginContext } from "../../../testing"; // Import the class directly so we can construct it without a createApp import { AgentsPlugin } from "../agents"; @@ -38,8 +39,13 @@ interface FakeContext { localName: string, args: unknown, ) => Promise; + /** What `attachContext` binds as the plugin's `this.cache`. */ + cache: CacheManager; } +/** Supplies the cache the fake contexts below hand to the plugin. */ +const kit = createTestPluginContext(); + function fakeContext( providers: Array<{ name: string; provider: ToolProvider }>, ): FakeContext { @@ -53,6 +59,7 @@ function fakeContext( tool: n, args, })), + cache: kit.cache, }; } @@ -88,20 +95,11 @@ function makeToolProvider( let tmpDir: string; let priorCwd: string; -beforeEach(async () => { +beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "agents-plugin-")); // Discovery scans /server/agents, so run in tmpDir and write agents there. priorCwd = process.cwd(); process.chdir(tmpDir); - const storage = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - keys: vi.fn(), - healthCheck: vi.fn(async () => true), - close: vi.fn(async () => {}), - }; - await CacheManager.getInstance({ storage: storage as any }); }); afterEach(() => { @@ -109,7 +107,7 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); -function instantiate(config: AgentsPluginConfig, ctx?: FakeContext) { +function instantiate(config: AgentsPluginConfig, ctx = fakeContext([])) { const plugin = new AgentsPlugin({ ...config, name: "agent" }); plugin.attachContext({ context: ctx as unknown as object }); return plugin; diff --git a/packages/appkit/src/plugins/agents/tests/approval-config.test.ts b/packages/appkit/src/plugins/agents/tests/approval-config.test.ts index 3322b83bb..eb2cd332b 100644 --- a/packages/appkit/src/plugins/agents/tests/approval-config.test.ts +++ b/packages/appkit/src/plugins/agents/tests/approval-config.test.ts @@ -1,6 +1,5 @@ -import { beforeEach, describe, expect, test, vi } from "vitest"; +import { describe, expect, test } from "vitest"; -import { CacheManager } from "../../../cache"; import { AgentsPlugin } from "../agents"; /** @@ -23,19 +22,6 @@ function policyOf(plugin: AgentsPlugin) { }; } -beforeEach(() => { - CacheManager.getInstanceSync = vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })) as unknown as typeof CacheManager.getInstanceSync; -}); - describe("AgentsPlugin.resolvedApprovalPolicy.timeoutMs", () => { test("uses the default (60_000) when approval is omitted", () => { const plugin = new AgentsPlugin({}); diff --git a/packages/appkit/src/plugins/agents/tests/approval-route.test.ts b/packages/appkit/src/plugins/agents/tests/approval-route.test.ts index e54359799..7c94cb872 100644 --- a/packages/appkit/src/plugins/agents/tests/approval-route.test.ts +++ b/packages/appkit/src/plugins/agents/tests/approval-route.test.ts @@ -1,7 +1,6 @@ import type express from "express"; import { beforeEach, describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../../cache"; import { AgentsPlugin } from "../agents"; /** @@ -48,16 +47,6 @@ function mockRes() { } beforeEach(() => { - CacheManager.getInstanceSync = vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })) as any; process.env.NODE_ENV = "development"; }); diff --git a/packages/appkit/src/plugins/agents/tests/discovery.test.ts b/packages/appkit/src/plugins/agents/tests/discovery.test.ts index 3ab960a82..7f5e96603 100644 --- a/packages/appkit/src/plugins/agents/tests/discovery.test.ts +++ b/packages/appkit/src/plugins/agents/tests/discovery.test.ts @@ -6,8 +6,8 @@ import { fileURLToPath } from "node:url"; import type { AgentAdapter, AgentInput, AgentRunContext } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../../cache"; import type { AgentsPluginConfig } from "../../../core/agent/types"; +import { createTestPluginContext } from "../../../testing"; import { AgentsPlugin } from "../agents"; /** Absolute path to a committed agent fixture directory. */ @@ -22,15 +22,13 @@ function stubAdapter(): AgentAdapter { }; } -beforeEach(async () => { - // Agent setup reads the cache singleton; initialize it with defaults. - await CacheManager.getInstance(); -}); +// One real context for the file: it carries the cache `setup()` reads, and its +// provider registry is empty, so tool collection finds nothing — as it did when +// these tests attached no context at all. +const kit = createTestPluginContext(); function instantiate(config: AgentsPluginConfig) { - const plugin = new AgentsPlugin({ ...config, name: "agent" }); - plugin.attachContext({ context: undefined as unknown as object }); - return plugin; + return kit.attach(new AgentsPlugin({ ...config, name: "agent" })); } type ExportsApi = { diff --git a/packages/appkit/src/plugins/agents/tests/dispatch-tool-call.test.ts b/packages/appkit/src/plugins/agents/tests/dispatch-tool-call.test.ts index 92c44a90a..4aedfea13 100644 --- a/packages/appkit/src/plugins/agents/tests/dispatch-tool-call.test.ts +++ b/packages/appkit/src/plugins/agents/tests/dispatch-tool-call.test.ts @@ -3,9 +3,8 @@ import os from "node:os"; import path from "node:path"; import type express from "express"; -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { afterEach, describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../../cache"; import { resolveSkillCatalog } from "../../../core/agent/skills/resolve-catalog"; import type { SkillDefinition } from "../../../core/agent/skills/types"; import type { ResolvedToolEntry } from "../../../core/agent/types"; @@ -35,20 +34,6 @@ import { * common `RunState` object. Tests below pin those guarantees. */ -beforeEach(() => { - // dispatchToolCall is exercised without going through setup(), so we - // need the cache singleton to be initialised before the plugin reads it. - (CacheManager as any).instance = { - get: vi.fn(), - set: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - }; -}); - function mockReq(): express.Request { // Carry OBO headers so PluginContext.executeTool's asUser(req) resolves a // user scope (the mock context enforces the real token precondition). diff --git a/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts b/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts index d124a1278..4d32bc9de 100644 --- a/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts +++ b/packages/appkit/src/plugins/agents/tests/dos-limits.test.ts @@ -1,7 +1,6 @@ import type express from "express"; import { beforeEach, describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../../cache"; import { AgentsPlugin } from "../agents"; import { chatRequestSchema, invocationsRequestSchema } from "../schemas"; import { @@ -54,16 +53,6 @@ function mockRes() { } beforeEach(() => { - CacheManager.getInstanceSync = vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })) as any; process.env.NODE_ENV = "development"; }); diff --git a/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts b/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts index 0eed1f884..bbc9469eb 100644 --- a/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts +++ b/packages/appkit/src/plugins/agents/tests/route-handler-errors.test.ts @@ -1,7 +1,6 @@ import type express from "express"; -import { beforeEach, describe, expect, test, vi } from "vitest"; +import { describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../../cache"; import { createTestPluginContext } from "../../../testing"; import { AgentsPlugin } from "../agents"; @@ -20,18 +19,6 @@ import { AgentsPlugin } from "../agents"; * rejected up-front with HTTP 400. */ -beforeEach(() => { - (CacheManager as any).instance = { - get: vi.fn(), - set: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - }; -}); - function mockReq(body: unknown, userId = "alice"): express.Request { const headers: Record = { "x-forwarded-user": userId, diff --git a/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts b/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts index 25ad528c9..4b1d30c91 100644 --- a/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts +++ b/packages/appkit/src/plugins/ai-search/tests/ai-search.test.ts @@ -5,6 +5,7 @@ import { } from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createTestPluginContext, resetTestCache } from "../../../testing"; import { Context } from "../../../workspace-client"; vi.mock("../../../context", () => ({ @@ -50,10 +51,25 @@ vi.mock("../../../telemetry", () => ({ fn: (...args: unknown[]) => unknown, _telemetryOpts?: unknown, ) => + // A whole span, not just the members this plugin happens to touch: + // the real CacheManager instruments getOrExecute on the same + // provider, and the faked cache this suite used to carry did not. fn({ setAttribute: vi.fn(), + setAttributes: vi.fn(), setStatus: vi.fn(), recordException: vi.fn(), + addEvent: vi.fn(), + addLink: vi.fn(), + addLinks: vi.fn(), + updateName: vi.fn(), + isRecording: vi.fn(() => false), + spanContext: vi.fn(() => ({ + traceId: "0".repeat(32), + spanId: "0".repeat(16), + traceFlags: 0, + })), + end: vi.fn(), }), ), }), @@ -63,38 +79,6 @@ vi.mock("../../../telemetry", () => ({ normalizeTelemetryOptions: () => ({ traces: false, metrics: false }), })); -// In-memory cache keyed like the real CacheManager.generateKey, so tests -// exercise real key composition. Never stores rejections. -const { mockCacheStore } = vi.hoisted(() => ({ - mockCacheStore: new Map(), -})); - -vi.mock("../../../cache", () => { - const keyOf = (parts: unknown[], userKey: string) => - JSON.stringify([userKey, ...parts]); - return { - CacheManager: { - getInstanceSync: () => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - generateKey: keyOf, - getOrExecute: async ( - key: unknown[], - fn: (signal?: AbortSignal) => Promise, - userKey: string, - ) => { - const k = keyOf(key, userKey); - if (mockCacheStore.has(k)) return mockCacheStore.get(k); - const result = await fn(); - mockCacheStore.set(k, result); - return result; - }, - }), - }, - }; -}); - vi.mock("../../../app", () => ({ AppManager: vi.fn().mockImplementation(() => ({})), })); @@ -135,11 +119,26 @@ const mockWorkspaceClient = { import { AiSearchPlugin } from "../ai-search"; +/** + * One kit context for this file, supplying the real `CacheManager`. The suite + * previously faked one that claimed to key "like the real + * CacheManager.generateKey" but keyed on `JSON.stringify([userKey, ...parts])` + * where production hashes — so the caching tests below could not actually + * exercise production's key composition. + */ +const kit = createTestPluginContext(); + +function aiSearchPlugin( + config: ConstructorParameters[0], +): AiSearchPlugin { + return kit.attach(new AiSearchPlugin(config)); +} + describe("AiSearchPlugin", () => { - beforeEach(() => { + beforeEach(async () => { mockRequest.mockClear(); mockRequest.mockResolvedValue(validVsResponse); - mockCacheStore.clear(); + await resetTestCache(); }); describe("setup()", () => { @@ -154,7 +153,7 @@ describe("AiSearchPlugin", () => { it("defaults indexName from DATABRICKS_VS_INDEX_NAME when omitted", async () => { process.env.DATABRICKS_VS_INDEX_NAME = "cat.sch.from_env"; - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { columns: ["id"] }, }, @@ -170,7 +169,7 @@ describe("AiSearchPlugin", () => { it("seeds a 'default' index from the env var when no indexes are configured", async () => { process.env.DATABRICKS_VS_INDEX_NAME = "cat.sch.from_env"; // Bare aiSearch() — no indexes config. - const plugin = new AiSearchPlugin({}); + const plugin = aiSearchPlugin({}); await plugin.query("default", { queryText: "q", columns: ["id"] }); expect(mockRequest.mock.calls[0][0].path).toBe( @@ -179,7 +178,7 @@ describe("AiSearchPlugin", () => { }); it("throws if pagination enabled but no endpointName", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -192,7 +191,7 @@ describe("AiSearchPlugin", () => { }); it("succeeds with valid config", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { products: { indexName: "cat.sch.products_idx", @@ -209,7 +208,7 @@ describe("AiSearchPlugin", () => { const originalNodeEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; try { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx" } }, }); await expect(plugin.setup()).rejects.toThrow( @@ -224,7 +223,7 @@ describe("AiSearchPlugin", () => { const originalNodeEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; try { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx", columns: ["id"] } }, }); await expect(plugin.setup()).resolves.not.toThrow(); @@ -264,7 +263,7 @@ describe("AiSearchPlugin", () => { it("fills columns from the source table in development and warns", async () => { process.env.NODE_ENV = "development"; mockRequest.mockImplementation(routeByPath); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx" } }, }); @@ -283,7 +282,7 @@ describe("AiSearchPlugin", () => { mockRequest.mockImplementation(routeByPath); // Columns set so the prod no-columns guard doesn't fire; this test only // asserts discovery doesn't run outside development. - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx", columns: ["id"] } }, }); @@ -299,7 +298,7 @@ describe("AiSearchPlugin", () => { it("skips (does not throw) when an index already has columns", async () => { process.env.NODE_ENV = "development"; mockRequest.mockImplementation(routeByPath); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.idx", columns: ["id"] } }, }); @@ -320,7 +319,7 @@ describe("AiSearchPlugin", () => { describe("exports()", () => { it("returns object with query function", () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id"] }, }, @@ -333,7 +332,7 @@ describe("AiSearchPlugin", () => { describe("query()", () => { it("calls VS API via connector and parses response", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { products: { indexName: "cat.sch.products", @@ -361,7 +360,7 @@ describe("AiSearchPlugin", () => { id: number; title: string; } - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { products: { indexName: "cat.sch.products", columns: ["id", "title"] }, }, @@ -379,7 +378,7 @@ describe("AiSearchPlugin", () => { }); it("constructs correct API request", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -409,7 +408,7 @@ describe("AiSearchPlugin", () => { }); it("throws Error for unknown alias", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id"] }, }, @@ -422,7 +421,7 @@ describe("AiSearchPlugin", () => { }); it("includes filters when provided", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -446,7 +445,7 @@ describe("AiSearchPlugin", () => { }); it("includes reranker config when enabled on index", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -468,7 +467,7 @@ describe("AiSearchPlugin", () => { it("calls embeddingFn and drops query_text for ann (vector-only)", async () => { const mockEmbeddingFn = vi.fn().mockResolvedValue([0.1, 0.2, 0.3]); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -489,7 +488,7 @@ describe("AiSearchPlugin", () => { it("keeps query_text alongside the embedded vector for hybrid", async () => { const mockEmbeddingFn = vi.fn().mockResolvedValue([0.1, 0.2, 0.3]); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -510,7 +509,7 @@ describe("AiSearchPlugin", () => { it("skips embeddingFn for full_text and sends query_text only", async () => { const mockEmbeddingFn = vi.fn().mockResolvedValue([0.1, 0.2, 0.3]); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -533,7 +532,7 @@ describe("AiSearchPlugin", () => { const mockEmbeddingFn = vi .fn() .mockRejectedValue(new Error("embedding service unavailable")); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -552,7 +551,7 @@ describe("AiSearchPlugin", () => { describe("shutdown()", () => { it("does not throw", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id"] }, }, @@ -568,7 +567,7 @@ describe("AiSearchPlugin", () => { result: { row_count: 1, data_array: [[1, "hi"]] }, next_page_token: null, }); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id", "t"] } }, }); await plugin.setup(); @@ -583,7 +582,7 @@ describe("AiSearchPlugin", () => { ...validVsResponse, next_page_token: "tok-123", }); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id"] } }, }); await plugin.setup(); @@ -599,7 +598,7 @@ describe("AiSearchPlugin", () => { next_page_token: null, debug_info: { latency_ms: 42 }, }); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", columns: ["id"] } }, }); await plugin.setup(); @@ -613,7 +612,7 @@ describe("AiSearchPlugin", () => { describe("query() overrides and reranker", () => { it("lets the request override index queryType, numResults, and columns", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -638,7 +637,7 @@ describe("AiSearchPlugin", () => { }); it("passes an object reranker through untouched", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -655,7 +654,7 @@ describe("AiSearchPlugin", () => { }); it("lets request.reranker=false suppress an index-enabled reranker", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", @@ -674,7 +673,7 @@ describe("AiSearchPlugin", () => { it("skips the reranker when enabled but no columns are resolved", async () => { // Query-time behavior only; skip setup() (its prod guard rejects the // deliberately column-less config used to exercise this path). - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { test: { indexName: "cat.sch.idx", reranker: true } }, }); await plugin.query("test", { queryText: "q" }); @@ -688,7 +687,7 @@ describe("AiSearchPlugin", () => { // Persistent reject so the retry interceptor exhausts its attempts and // execute() surfaces a failed result, driving the !result.ok branch. mockRequest.mockRejectedValue(new Error("VS 503")); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { products: { indexName: "cat.sch.p", columns: ["id"] } }, }); await plugin.setup(); @@ -701,7 +700,7 @@ describe("AiSearchPlugin", () => { describe("caching", () => { const makePlugin = () => - new AiSearchPlugin({ + aiSearchPlugin({ indexes: { products: { indexName: "cat.sch.products", @@ -798,7 +797,7 @@ describe("AiSearchPlugin", () => { it("keys managed-embedding queries by queryText, skipping embedding on a route cache hit", async () => { // Keyed by queryText, not the derived vector; the hit skips embeddingFn. const embeddingFn = vi.fn().mockResolvedValue([0.1, 0.2, 0.3]); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.docs", @@ -831,7 +830,7 @@ describe("AiSearchPlugin", () => { it("skips embedding on a programmatic query() cache hit too", async () => { const embeddingFn = vi.fn().mockResolvedValue([0.1, 0.2, 0.3]); - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.docs", @@ -851,7 +850,7 @@ describe("AiSearchPlugin", () => { }); it("does not share cache across OBO users (per-user cache key)", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.docs", @@ -894,7 +893,7 @@ describe("AiSearchPlugin", () => { }); it("re-serves the same OBO user from cache (connector called once)", async () => { - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { docs: { indexName: "cat.sch.docs", @@ -931,7 +930,7 @@ describe("AiSearchPlugin", () => { describe("injectRoutes", () => { const makePlugin = () => - new AiSearchPlugin({ + aiSearchPlugin({ indexes: { demo: { indexName: "cat.sch.idx", @@ -1037,7 +1036,7 @@ describe("AiSearchPlugin", () => { it("500s when query preparation throws", async () => { // Query prep (embeddingFn) runs inside execute() so it shares the OBO // context; a failure surfaces as a non-ok result → 500. - const plugin = new AiSearchPlugin({ + const plugin = aiSearchPlugin({ indexes: { demo: { indexName: "cat.sch.idx", diff --git a/packages/appkit/src/plugins/analytics/tests/_test-helpers.ts b/packages/appkit/src/plugins/analytics/tests/_test-helpers.ts new file mode 100644 index 000000000..56bb70ee0 --- /dev/null +++ b/packages/appkit/src/plugins/analytics/tests/_test-helpers.ts @@ -0,0 +1,23 @@ +import { createTestPluginContext } from "../../../testing"; +import { AnalyticsPlugin } from "../analytics"; +import type { IAnalyticsConfig } from "../types"; + +/** + * One kit context for the analytics suites — Vitest isolates files, so this + * module is re-evaluated per file and the `kit` stays per-file. It supplies the + * real `CacheManager`, whose `generateKey` is production's, so key invariants + * are asserted against the real thing rather than a copy that can drift. + */ +const kit = createTestPluginContext(); + +/** + * The cache every plugin from {@link analyticsPlugin} resolves as `this.cache`. + * Spy it (`vi.spyOn(testCache, "getOrExecute")`) to assert caching against + * production's own keying. + */ +export const testCache = kit.cache; + +/** Build an `AnalyticsPlugin` bound to this file's cache, the way an app does. */ +export function analyticsPlugin(config: IAnalyticsConfig): AnalyticsPlugin { + return kit.attach(new AnalyticsPlugin(config)); +} diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts index 68b6b94d7..2f3f93658 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.readonly.test.ts @@ -1,20 +1,5 @@ import { describe, expect, test, vi } from "vitest"; -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - import { AnalyticsPlugin } from "../analytics"; /** diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 5101f9424..fecbb6af7 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -22,48 +22,10 @@ import { sql } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; -import { AnalyticsPlugin, analytics, writeChunk } from "../analytics"; +import { resetTestCache } from "../../../testing"; +import { analytics, writeChunk } from "../analytics"; import type { IAnalyticsConfig } from "../types"; - -// Mock CacheManager singleton with actual caching behavior -const { mockCacheStore, mockCacheInstance } = vi.hoisted(() => { - const store = new Map(); - - const generateKey = (parts: unknown[], userKey: string): string => { - const { createHash } = require("node:crypto"); - const allParts = [userKey, ...parts]; - const serialized = JSON.stringify(allParts); - return createHash("sha256").update(serialized).digest("hex"); - }; - - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (key: unknown[], fn: () => Promise, userKey: string) => { - const cacheKey = generateKey(key, userKey); - if (store.has(cacheKey)) { - return store.get(cacheKey); - } - const result = await fn(); - store.set(cacheKey, result); - return result; - }, - ), - generateKey: vi.fn((parts: unknown[], userKey: string) => - generateKey(parts, userKey), - ), - }; - - return { mockCacheStore: store, mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +import { analyticsPlugin } from "./_test-helpers"; describe("Analytics Plugin", () => { let config: IAnalyticsConfig; @@ -72,7 +34,7 @@ describe("Analytics Plugin", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); + await resetTestCache(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -87,14 +49,14 @@ describe("Analytics Plugin", () => { }); test("Plugin instance should be created with correct configuration", () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); expect(plugin.name).toBe("analytics"); }); describe("injectRoutes", () => { test("should register the query and metric POST routes", () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router } = createMockRouter(); plugin.injectRoutes(router); @@ -113,7 +75,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should return 400 when query_key is missing", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); plugin.injectRoutes(router); @@ -134,7 +96,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should execute as service principal for .sql files (isAsUser: false)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // Mock getAppQuery to return a regular .sql file (isAsUser: false) @@ -197,7 +159,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should execute as user for .obo.sql files (isAsUser: true)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // Mock getAppQuery to return an .obo.sql file (isAsUser: true) @@ -260,7 +222,7 @@ describe("Analytics Plugin", () => { }); test("should use different cache keys for .sql vs .obo.sql queries", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); const getAppQueryMock = vi.fn(); @@ -309,7 +271,7 @@ describe("Analytics Plugin", () => { }); test("should return cached result on second request for .sql files", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -343,7 +305,7 @@ describe("Analytics Plugin", () => { }); test("should share cache across users for .sql files (global cache)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // Mock returns .sql file (isAsUser: false) - should use global cache @@ -413,7 +375,7 @@ describe("Analytics Plugin", () => { }); test("should cache user-scoped .obo.sql queries separately per user", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // Mock returns .obo.sql file (isAsUser: true) @@ -487,7 +449,7 @@ describe("Analytics Plugin", () => { }); test("OBO cache key must use the end user's ID, not the service principal's", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -546,7 +508,7 @@ describe("Analytics Plugin", () => { }); test("OBO requests differing only by whitespace in x-forwarded-user share one cache key", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -603,7 +565,7 @@ describe("Analytics Plugin", () => { }); test("should handle AbortSignal cancellation", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -645,7 +607,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should pass INLINE + ARROW_STREAM format parameters when format is ARROW_STREAM", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -682,7 +644,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should use INLINE + JSON_ARRAY by default when no format specified", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -717,7 +679,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should pass INLINE + JSON_ARRAY when format is explicitly JSON_ARRAY", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -748,7 +710,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key falls back ARROW_STREAM INLINE→EXTERNAL_LINKS and streams the preserved links in-context", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -827,7 +789,7 @@ describe("Analytics Plugin", () => { }); test("OBO: .obo.sql ARROW_STREAM external-links streams under the user context", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // `.obo.sql` → isAsUser true → the route must run through asUser(req). @@ -896,7 +858,7 @@ describe("Analytics Plugin", () => { }); test("ARROW_STREAM: a stuck warehouse fails fast with a 503 WAREHOUSE_UNAVAILABLE", async () => { - const plugin = new AnalyticsPlugin({ + const plugin = analyticsPlugin({ ...config, arrowFirstByteTimeoutMs: 20, }); @@ -938,7 +900,7 @@ describe("Analytics Plugin", () => { }); test("ARROW_STREAM: a schema too wide for the header advertises a columns-ref instead", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -977,7 +939,7 @@ describe("Analytics Plugin", () => { }); test("GET /columns/:statementId returns the manifest column names", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).SQLClient.getColumnNames = vi .fn() @@ -996,7 +958,7 @@ describe("Analytics Plugin", () => { }); test("GET /columns/:statementId falls back to the service principal when the user identity can't read it (OBO)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // User identity 404s (e.g. the statement was executed by the SP, which @@ -1025,7 +987,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key falls back on a structured ExecutionError.errorCode without scanning the message", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1069,7 +1031,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key falls back when error message carries a structured INVALID_PARAMETER_VALUE error_code", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1110,7 +1072,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key does NOT fall back on a non-capability error (auth/SQL)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1153,7 +1115,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key falls back on a capability-coded rejection regardless of exact wording", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1196,7 +1158,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should not fall back for non-format errors", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1230,7 +1192,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key streams ARROW_STREAM INLINE bytes directly on the response body (no SSE, no stash)", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1301,7 +1263,7 @@ describe("Analytics Plugin", () => { // back into plain row objects: the caller's contract is preserved and // the SSE channel still carries a `result` message, not an `arrow` // message. - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1418,7 +1380,7 @@ describe("Analytics Plugin", () => { "base64", ); - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ query: "SELECT * FROM test", @@ -1471,7 +1433,7 @@ describe("Analytics Plugin", () => { // If the JSON_ARRAY retry path (ARROW_STREAM + INLINE) also fails — e.g. // a downstream warehouse outage that affects both shapes — the route // must surface the failure rather than silently dropping it. - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1544,7 +1506,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key rejects unknown format values with 400", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); const executeMock = vi.fn(); @@ -1569,7 +1531,7 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key does not retry the fallback when the request was aborted", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1577,13 +1539,14 @@ describe("Analytics Plugin", () => { isAsUser: false, }); - const executeMock = vi.fn().mockImplementation((_wc, _opts, signal) => { - // Simulate a signal that becomes aborted before the failure surfaces — - // e.g. the client cancelled the SSE stream mid-query. Use vitest's - // getter spy rather than Object.defineProperty so we don't try to - // override the native non-configurable AbortSignal.aborted getter. - if (signal) { - vi.spyOn(signal, "aborted", "get").mockReturnValue(true); + const executeMock = vi.fn().mockImplementation(() => { + // Simulate the client cancelling the SSE stream mid-query by firing the + // `close` listener the route registered. That aborts the route's own + // controller — the signal its fallback check actually reads — so this + // does not depend on which signal object the cache hands the callback + // (the caching executor passes a composed `sharedSignal`, not this one). + for (const [event, handler] of mockRes.on.mock.calls) { + if (event === "close") handler(); } return Promise.reject( new Error( @@ -1614,7 +1577,7 @@ describe("Analytics Plugin", () => { // disposition could be any unrelated SQL/permission error. The classifier // must NOT interpret it as "warehouse wants ARROW_STREAM" — falling back // would mask the real failure. - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1651,7 +1614,7 @@ describe("Analytics Plugin", () => { }); test("emits warehouse_status events before the result", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ @@ -1717,7 +1680,7 @@ describe("Analytics Plugin", () => { }); test("should return 404 when query file is not found", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); // Mock getAppQuery to return null (query not found) @@ -1743,7 +1706,7 @@ describe("Analytics Plugin", () => { describe("toolkit()", () => { test("produces ToolkitEntry records keyed by the plugin name", () => { - const plugin = new AnalyticsPlugin({ name: "analytics" }); + const plugin = analyticsPlugin({ name: "analytics" }); const entries = plugin.toolkit(); expect(Object.keys(entries)).toContain("analytics.query"); const entry = entries["analytics.query"]; @@ -1753,7 +1716,7 @@ describe("Analytics Plugin", () => { }); test("respects prefix and only options", () => { - const plugin = new AnalyticsPlugin({ name: "analytics" }); + const plugin = analyticsPlugin({ name: "analytics" }); const entries = plugin.toolkit({ prefix: "", only: ["query"] }); expect(Object.keys(entries)).toEqual(["query"]); }); diff --git a/packages/appkit/src/plugins/analytics/tests/metric.test.ts b/packages/appkit/src/plugins/analytics/tests/metric.test.ts index bf721feee..60453e7ef 100644 --- a/packages/appkit/src/plugins/analytics/tests/metric.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/metric.test.ts @@ -15,7 +15,8 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AppManager } from "../../../app"; import { ServiceContext } from "../../../context/service-context"; import { AuthenticationError } from "../../../errors"; -import { AnalyticsPlugin } from "../analytics"; +import { resetTestCache } from "../../../testing"; +import type { AnalyticsPlugin } from "../analytics"; import { buildMetricSql, composeMetricCacheKey, @@ -30,41 +31,7 @@ import type { MetricFilter, MetricRegistration, } from "../types"; - -// Mirror the analytics.test.ts CacheManager mock so the inner `execute`'s -// cache interceptor is a no-op pass-through (each request re-executes). -const { mockCacheStore, mockCacheInstance } = vi.hoisted(() => { - const store = new Map(); - const generateKey = (parts: unknown[], userKey: string): string => { - const { createHash } = require("node:crypto"); - const serialized = JSON.stringify([userKey, ...parts]); - return createHash("sha256").update(serialized).digest("hex"); - }; - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (key: unknown[], fn: () => Promise, userKey: string) => { - const cacheKey = generateKey(key, userKey); - if (store.has(cacheKey)) return store.get(cacheKey); - const result = await fn(); - store.set(cacheKey, result); - return result; - }, - ), - generateKey: vi.fn((parts: unknown[], userKey: string) => - generateKey(parts, userKey), - ), - }; - return { mockCacheStore: store, mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +import { analyticsPlugin, testCache } from "./_test-helpers"; // Temp dirs created by `registryDir` / `writeRegistry`, cleaned up after each // test. Using real files (pointing the plugin's `AppManager` at the dir, see @@ -86,7 +53,7 @@ const tempRegistryDirs: string[] = []; * every route-handler test threads through. */ function pluginForDir(config: IAnalyticsConfig, dir: string): AnalyticsPlugin { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); (plugin as any).app = new AppManager(path.join(dir, "queries"), dir); return plugin; } @@ -146,7 +113,7 @@ describe("analytics metric route", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); + await resetTestCache(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -161,7 +128,7 @@ describe("analytics metric route", () => { describe("injectRoutes", () => { test("registers POST /metric/:key alongside /query", () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router } = createMockRouter(); plugin.injectRoutes(router); @@ -1064,8 +1031,9 @@ describe("analytics metric route", () => { // Capture the composed cache key the inner `execute` hands to the shared // CacheManager mock — the same key whether or not metadata is injected. + const getOrExecuteSpy = vi.spyOn(testCache, "getOrExecute"); const cacheKeyFor = async (mvMeta?: MetricViewsMetadata) => { - mockCacheInstance.getOrExecute.mockClear(); + getOrExecuteSpy.mockClear(); const plugin = pluginForDir( { ...config, metricViewsMetadata: mvMeta }, registryDir(registry), @@ -1079,7 +1047,7 @@ describe("analytics metric route", () => { createMockResponse(), ); // First getOrExecute call is the SQL execution's cache interceptor. - const call = mockCacheInstance.getOrExecute.mock.calls[0]; + const call = getOrExecuteSpy.mock.calls[0]; return { cacheKey: call[0], userKey: call[2] }; }; @@ -1359,7 +1327,7 @@ describe("analytics metric route", () => { }); test("no definitions.json present → registry empty, unknown key 404, nothing executes", async () => { - const plugin = new AnalyticsPlugin(config); + const plugin = analyticsPlugin(config); const { router, getHandler } = createMockRouter(); const executeMock = vi.fn(); @@ -2489,7 +2457,7 @@ describe("metric — filter translator", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); + await resetTestCache(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); @@ -2960,7 +2928,7 @@ describe("metric route — lane dispatch", () => { beforeEach(async () => { config = { timeout: 5000 }; setupDatabricksEnv(); - mockCacheStore.clear(); + await resetTestCache(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); }); diff --git a/packages/appkit/src/plugins/analytics/tests/result-delivery.test.ts b/packages/appkit/src/plugins/analytics/tests/result-delivery.test.ts index 06438b5a5..2ababd037 100644 --- a/packages/appkit/src/plugins/analytics/tests/result-delivery.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/result-delivery.test.ts @@ -212,6 +212,37 @@ describe("deliverArrowBytes — normal warehouse (fallback to EXTERNAL_LINKS)", // Only the INLINE attempt — no EXTERNAL_LINKS fallback. expect(calls).toEqual([{ disposition: "INLINE", format: "ARROW_STREAM" }]); }); + + test("an aborted signal propagates the rejection without a fallback", async () => { + // The rejection is the one that normally *does* trigger EXTERNAL_LINKS, so + // the abort is the only thing that can stop the second statement. Covers + // the client disconnecting mid-query: retrying is pure waste once nobody + // is listening. + const inlineErr = reject( + "INVALID_PARAMETER_VALUE", + "The format field must be JSON_ARRAY when the disposition field is INLINE.", + ); + const { executor, calls } = executorFrom(async () => { + throw inlineErr; + }); + const streamer: ArrowChunkStreamer = { + streamExternalLinks: vi.fn(async function* () {}), + }; + + await expect( + collect( + deliverArrowBytes( + executor, + streamer, + "SELECT 1", + undefined, + {}, + AbortSignal.abort(), + ), + ), + ).rejects.toBe(inlineErr); + expect(calls).toEqual([{ disposition: "INLINE", format: "ARROW_STREAM" }]); + }); }); describe("deliverJsonResult", () => { diff --git a/packages/appkit/src/plugins/files/tests/_test-helpers.ts b/packages/appkit/src/plugins/files/tests/_test-helpers.ts index 1531ce1a4..8d58207af 100644 --- a/packages/appkit/src/plugins/files/tests/_test-helpers.ts +++ b/packages/appkit/src/plugins/files/tests/_test-helpers.ts @@ -4,9 +4,34 @@ import { mockServiceContext, setupDatabricksEnv } from "@tools/test-helpers"; import { vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; -import type { FilesPlugin } from "../plugin"; +import { createTestPluginContext } from "../../../testing"; +import { FilesPlugin } from "../plugin"; import { policy } from "../policy"; +/** + * One kit context per test file — Vitest isolates files, so this module is + * re-evaluated for each — supplying the real `CacheManager` a plugin resolves. + */ +const kit = createTestPluginContext(); + +/** + * The cache every plugin from {@link filesPlugin} resolves as `this.cache`. + * Spy it to assert invalidation (`vi.spyOn(testCache, "delete")`), so the keys + * asserted are production's own rather than a re-implemented fake's. + */ +export const testCache = kit.cache; + +/** + * Build a `FilesPlugin` bound to this file's cache, the way an app binds one. + * `attach` runs the production path — attachContext plus the registry parity a + * booted app gives — and is synchronous. + */ +export function filesPlugin( + config: ConstructorParameters[0] = VOLUMES_CONFIG, +): FilesPlugin { + return kit.attach(new FilesPlugin(config)); +} + export const VOLUMES_CONFIG = { volumes: { uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() }, diff --git a/packages/appkit/src/plugins/files/tests/delete.test.ts b/packages/appkit/src/plugins/files/tests/delete.test.ts index 1cdaa7e5d..f00b0636f 100644 --- a/packages/appkit/src/plugins/files/tests/delete.test.ts +++ b/packages/appkit/src/plugins/files/tests/delete.test.ts @@ -1,16 +1,16 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { FilesPlugin } from "../plugin"; import { + filesPlugin, getRouteHandler, mockReq, mockRes, setupTestEnv, teardownTestEnv, - VOLUMES_CONFIG, + testCache, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -34,17 +34,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -66,12 +56,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin delete", () => { let serviceContextMock: Awaited>; @@ -84,9 +68,12 @@ describe("FilesPlugin delete", () => { }); test("successful delete invalidates list cache", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(); const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); + // Production's own keying and invalidation, not a fake's. + const generateKey = vi.spyOn(testCache, "generateKey"); + const invalidate = vi.spyOn(testCache, "delete"); mockClient.files.delete.mockResolvedValue(undefined); @@ -100,12 +87,12 @@ describe("FilesPlugin delete", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(invalidate).toHaveBeenCalled(); }); test("delete without path returns 400", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(); const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); @@ -118,7 +105,7 @@ describe("FilesPlugin delete", () => { }); test("delete that throws ApiError returns proper status", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(); const handler = getRouteHandler(plugin, "delete", ""); const res = mockRes(); diff --git a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts index e96470e1b..95cb34dd1 100644 --- a/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/download-endpoint.test.ts @@ -11,7 +11,7 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -35,17 +35,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -67,12 +57,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin download endpoint Content-Disposition", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/files/tests/error-handling.test.ts b/packages/appkit/src/plugins/files/tests/error-handling.test.ts index f02fb8683..62a065687 100644 --- a/packages/appkit/src/plugins/files/tests/error-handling.test.ts +++ b/packages/appkit/src/plugins/files/tests/error-handling.test.ts @@ -10,7 +10,7 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -34,17 +34,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -66,12 +56,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin error handling", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/files/tests/mkdir.test.ts b/packages/appkit/src/plugins/files/tests/mkdir.test.ts index 00623bef5..b2b767bfe 100644 --- a/packages/appkit/src/plugins/files/tests/mkdir.test.ts +++ b/packages/appkit/src/plugins/files/tests/mkdir.test.ts @@ -1,16 +1,17 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { FilesPlugin } from "../plugin"; import { + filesPlugin, getRouteHandler, mockReq, mockRes, setupTestEnv, teardownTestEnv, + testCache, VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -34,17 +35,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -66,12 +57,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin mkdir", () => { let serviceContextMock: Awaited>; @@ -84,9 +69,12 @@ describe("FilesPlugin mkdir", () => { }); test("successful mkdir invalidates list cache", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); + // Production's own keying and invalidation, not a fake's. + const generateKey = vi.spyOn(testCache, "generateKey"); + const invalidate = vi.spyOn(testCache, "delete"); mockClient.files.createDirectory.mockResolvedValue(undefined); @@ -100,12 +88,12 @@ describe("FilesPlugin mkdir", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(invalidate).toHaveBeenCalled(); }); test("mkdir without path returns 400", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); @@ -118,7 +106,7 @@ describe("FilesPlugin mkdir", () => { }); test("mkdir that throws ApiError 409 is handled via execute", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); diff --git a/packages/appkit/src/plugins/files/tests/path-validation.test.ts b/packages/appkit/src/plugins/files/tests/path-validation.test.ts index 7705b4778..e7b1a3fe8 100644 --- a/packages/appkit/src/plugins/files/tests/path-validation.test.ts +++ b/packages/appkit/src/plugins/files/tests/path-validation.test.ts @@ -11,7 +11,7 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -35,17 +35,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -67,12 +57,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin path validation", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index a9612d47e..9d74f095d 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -7,6 +7,7 @@ import { ServiceContext } from "../../../context/service-context"; import { createApp } from "../../../core"; import { AuthenticationError } from "../../../errors"; import { ResourceType } from "../../../registry"; +import { resetTestCache } from "../../../testing"; import { FILES_DOWNLOAD_DEFAULTS, FILES_READ_DEFAULTS, @@ -14,8 +15,9 @@ import { } from "../defaults"; import { FilesPlugin, files } from "../plugin"; import { PolicyDeniedError, policy } from "../policy"; +import { filesPlugin, testCache, VOLUMES_CONFIG } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -42,18 +44,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(), - }; - - return { mockFilesApi, mockClient, MockApiError, mockCacheInstance }; + return { mockFilesApi, mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -75,25 +66,14 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - getInstance: vi.fn(async () => mockCacheInstance), - }, -})); - -const VOLUMES_CONFIG = { - volumes: { - uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() }, - exports: { policy: policy.allowAll() }, - }, -}; - describe("FilesPlugin", () => { let serviceContextMock: Awaited>; beforeEach(async () => { vi.clearAllMocks(); + // The cache is real and shared across this file's tests now, so entries + // must not outlive a test. + await resetTestCache(); setupDatabricksEnv(); ServiceContext.reset(); process.env.DATABRICKS_VOLUME_UPLOADS = "/Volumes/catalog/schema/uploads"; @@ -102,6 +82,12 @@ describe("FilesPlugin", () => { }); afterEach(() => { + // Unpatch only the shared cache: a stubbed `generateKey`/`delete` would + // otherwise leak into later tests. `vi.restoreAllMocks()` is too broad — + // it also strips the module-scope SDK doubles' implementations. + for (const method of ["getOrExecute", "generateKey", "delete"] as const) { + (testCache[method] as { mockRestore?: () => void }).mockRestore?.(); + } serviceContextMock?.restore(); delete process.env.DATABRICKS_VOLUME_UPLOADS; delete process.env.DATABRICKS_VOLUME_EXPORTS; @@ -113,7 +99,7 @@ describe("FilesPlugin", () => { }); test("plugin instance has correct name", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); expect(plugin.name).toBe("files"); }); @@ -218,7 +204,7 @@ describe("FilesPlugin", () => { describe("getAgentTools / executeAgentTool", () => { test("produces independent tool entries per volume", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const tools = plugin.getAgentTools(); const names = tools.map((t) => t.name); @@ -237,7 +223,7 @@ describe("FilesPlugin", () => { }); test("dispatches to the correct volume API based on the tool name", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const asyncIterable = (items: { path: string }[]) => ({ [Symbol.asyncIterator]: async function* () { for (const item of items) yield item; @@ -264,7 +250,7 @@ describe("FilesPlugin", () => { }); test("returns LLM-friendly error string for invalid tool args", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const result = await plugin.executeAgentTool("uploads.read", {}); expect(typeof result).toBe("string"); expect(result).toContain("Invalid arguments for uploads.read"); @@ -274,7 +260,7 @@ describe("FilesPlugin", () => { describe("exports()", () => { test("returns a callable function with a .volume alias", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const exported = plugin.exports(); expect(typeof exported).toBe("function"); @@ -282,7 +268,7 @@ describe("FilesPlugin", () => { }); test("returns volume handle with asUser and direct VolumeAPI methods", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const exported = plugin.exports(); for (const key of ["uploads", "exports"]) { @@ -295,7 +281,7 @@ describe("FilesPlugin", () => { }); test(".volume() returns the same shape as the callable", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const exported = plugin.exports(); const direct = exported("uploads"); @@ -305,7 +291,7 @@ describe("FilesPlugin", () => { }); test("throws for unknown volume key", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const exported = plugin.exports(); expect(() => exported("unknown")).toThrow(/Unknown volume "unknown"/); @@ -329,7 +315,7 @@ describe("FilesPlugin", () => { ]; test("volume handle exposes asUser and all VolumeAPI methods", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); expect(typeof handle.asUser).toBe("function"); @@ -342,7 +328,7 @@ describe("FilesPlugin", () => { const originalEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; try { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); const mockReq = { header: () => undefined } as any; @@ -356,7 +342,7 @@ describe("FilesPlugin", () => { const originalEnv = process.env.NODE_ENV; process.env.NODE_ENV = "development"; try { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); const mockReq = { header: () => undefined } as any; @@ -370,7 +356,7 @@ describe("FilesPlugin", () => { }); test("direct methods on handle work as service principal", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); // Direct call executes as service principal (returns a promise, does not throw) @@ -379,7 +365,7 @@ describe("FilesPlugin", () => { }); test("injectRoutes registers volume-scoped routes", () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const mockRouter = { use: vi.fn(), get: vi.fn(), @@ -403,7 +389,7 @@ describe("FilesPlugin", () => { }); test("shutdown() calls streamManager.abortAll()", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const abortAllSpy = vi.spyOn((plugin as any).streamManager, "abortAll"); await plugin.shutdown(); @@ -443,7 +429,7 @@ describe("FilesPlugin", () => { } test("returns 404 for unknown volume key", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -458,7 +444,7 @@ describe("FilesPlugin", () => { }); test("/volumes returns configured volume keys", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "get", "/volumes"); const res = mockRes(); @@ -502,7 +488,7 @@ describe("FilesPlugin", () => { } test("rejects upload with content-length over per-volume limit (413)", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getUploadHandler(plugin); const res = mockRes(); @@ -531,7 +517,7 @@ describe("FilesPlugin", () => { }); test("rejects upload with content-length over default limit (413)", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getUploadHandler(plugin); const res = mockRes(); @@ -560,7 +546,7 @@ describe("FilesPlugin", () => { }); test("allows upload with content-length at exactly the limit", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getUploadHandler(plugin); const res = mockRes(); @@ -587,7 +573,7 @@ describe("FilesPlugin", () => { }); test("allows upload when content-length header is missing", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getUploadHandler(plugin); const res = mockRes(); @@ -615,7 +601,7 @@ describe("FilesPlugin", () => { describe("auto-discovery integration", () => { test("files() with no volumes config discovers from env vars", () => { - const plugin = new FilesPlugin({}); + const plugin = filesPlugin({}); const exported = plugin.exports(); // Discovered volumes are accessible via the callable expect(() => exported("uploads")).not.toThrow(); @@ -625,7 +611,7 @@ describe("FilesPlugin", () => { test("files() with no config and no env vars creates no volumes", () => { delete process.env.DATABRICKS_VOLUME_UPLOADS; delete process.env.DATABRICKS_VOLUME_EXPORTS; - const plugin = new FilesPlugin({}); + const plugin = filesPlugin({}); const exported = plugin.exports(); expect(() => exported("uploads")).toThrow(/Unknown volume/); }); @@ -712,7 +698,7 @@ describe("FilesPlugin", () => { }); test("read-tier: list succeeds when operation completes within timeout", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); @@ -736,7 +722,7 @@ describe("FilesPlugin", () => { }); test("read-tier: list returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); @@ -763,7 +749,7 @@ describe("FilesPlugin", () => { }); test("read-tier: read returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/read"); const res = mockRes(); @@ -785,7 +771,7 @@ describe("FilesPlugin", () => { }); test("read-tier: exists returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/exists"); const res = mockRes(); @@ -809,7 +795,7 @@ describe("FilesPlugin", () => { }); test("read-tier: metadata returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/metadata"); const res = mockRes(); @@ -833,7 +819,7 @@ describe("FilesPlugin", () => { }); test("download-tier: download returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/download"); const res = mockRes(); @@ -855,7 +841,7 @@ describe("FilesPlugin", () => { }); test("write-tier: mkdir returns 500 when SDK call rejects", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "post", "/mkdir"); const res = mockRes(); @@ -878,7 +864,7 @@ describe("FilesPlugin", () => { }); test("write-tier: inflightWrites decrements after error", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "post", "/mkdir"); const res = mockRes(); @@ -900,7 +886,7 @@ describe("FilesPlugin", () => { }); test("error response does not leak internal details", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); @@ -927,7 +913,7 @@ describe("FilesPlugin", () => { // context.signal, but the files plugin callbacks don't consume it. // The timeout only works if the underlying SDK call respects the signal // or rejects on its own. - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandlerForTimeout(plugin, "get", "/list"); const res = mockRes(); @@ -1062,7 +1048,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; try { - const plugin = new FilesPlugin(spyConfig); + const plugin = filesPlugin(spyConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1102,7 +1088,7 @@ describe("FilesPlugin", () => { }); test("header-less HTTP + default publicRead() + write action → 403 with SP user", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1139,7 +1125,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_DENIED = "/Volumes/c/s/denied"; try { - const plugin = new FilesPlugin(spyConfig); + const plugin = filesPlugin(spyConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1177,7 +1163,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_GATED = "/Volumes/c/s/gated"; try { - const plugin = new FilesPlugin(allowConfig); + const plugin = filesPlugin(allowConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1225,7 +1211,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_GATED = "/Volumes/c/s/gated"; try { - const plugin = new FilesPlugin(denyConfig); + const plugin = filesPlugin(denyConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1250,7 +1236,7 @@ describe("FilesPlugin", () => { }); test("policy volume + policy returns false → 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1265,7 +1251,7 @@ describe("FilesPlugin", () => { }); test("policy volume + policy returns true → 200, runs as SP", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1301,7 +1287,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_ASYNC_VOL = "/Volumes/c/s/async"; try { - const plugin = new FilesPlugin(asyncConfig); + const plugin = filesPlugin(asyncConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1322,7 +1308,7 @@ describe("FilesPlugin", () => { }); test("default publicRead() volume → reads succeed", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1341,7 +1327,7 @@ describe("FilesPlugin", () => { }); test("default publicRead() volume → writes denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); @@ -1372,7 +1358,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_SIZED = "/Volumes/c/s/sized"; try { - const plugin = new FilesPlugin(sizeConfig); + const plugin = filesPlugin(sizeConfig); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1399,7 +1385,7 @@ describe("FilesPlugin", () => { }); test("upload with malformed content-length → rejected with 400", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1424,7 +1410,7 @@ describe("FilesPlugin", () => { }); test("upload with negative content-length → rejected with 400", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1444,7 +1430,7 @@ describe("FilesPlugin", () => { }); test("upload with partially numeric content-length → rejected with 400", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1464,7 +1450,7 @@ describe("FilesPlugin", () => { }); test("SDK asUser(req) on policy volume → policy-wrapped API works", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const exported = plugin.exports(); const handle = exported("public"); @@ -1490,7 +1476,7 @@ describe("FilesPlugin", () => { }); test("SDK asUser(req) on policy volume + deny → throws PolicyDeniedError", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const exported = plugin.exports(); const handle = exported("locked"); @@ -1507,7 +1493,7 @@ describe("FilesPlugin", () => { }); test("SDK asUser(req) + denyAll() → delete throws PolicyDeniedError", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handle = plugin.exports()("locked"); const mockReqObj = { @@ -1524,7 +1510,7 @@ describe("FilesPlugin", () => { }); test("SDK asUser(req) + publicRead() → upload throws PolicyDeniedError", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handle = plugin.exports()("public"); const mockReqObj = { @@ -1541,7 +1527,7 @@ describe("FilesPlugin", () => { }); test("direct call on policy volume → enforces policy as SP", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handle = plugin.exports()("open"); // Direct call on allowAll() volume succeeds (policy is checked but allows) @@ -1552,7 +1538,7 @@ describe("FilesPlugin", () => { }); test("direct SP call on denyAll() volume → throws PolicyDeniedError", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handle = plugin.exports()("locked"); await expect(handle.list()).rejects.toThrow(PolicyDeniedError); @@ -1570,7 +1556,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; try { - const plugin = new FilesPlugin(spyConfig); + const plugin = filesPlugin(spyConfig); const handle = plugin.exports()("spied"); await handle.list(); @@ -1596,7 +1582,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; try { - const plugin = new FilesPlugin(spyConfig); + const plugin = filesPlugin(spyConfig); const handle = plugin.exports()("spied"); const mockReqObj = { header: (name: string) => { @@ -1626,7 +1612,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → read denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/read"); const res = mockRes(); @@ -1641,7 +1627,7 @@ describe("FilesPlugin", () => { }); test("publicRead() volume → read allowed", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/read"); const res = mockRes(); @@ -1664,7 +1650,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → download denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/download"); const res = mockRes(); @@ -1679,7 +1665,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → raw denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/raw"); const res = mockRes(); @@ -1694,7 +1680,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → exists denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/exists"); const res = mockRes(); @@ -1709,7 +1695,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → metadata denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/metadata"); const res = mockRes(); @@ -1724,7 +1710,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → preview denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/preview"); const res = mockRes(); @@ -1739,7 +1725,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → delete denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "delete", "/:volumeKey"); const res = mockRes(); @@ -1754,7 +1740,7 @@ describe("FilesPlugin", () => { }); test("denyAll() volume → upload denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); @@ -1779,7 +1765,7 @@ describe("FilesPlugin", () => { }); test("not(publicRead()) volume → read denied with 403", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1794,7 +1780,7 @@ describe("FilesPlugin", () => { }); test("not(publicRead()) volume → write allowed", async () => { - const plugin = new FilesPlugin(POLICY_CONFIG); + const plugin = filesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "post", "/mkdir"); const res = mockRes(); @@ -1824,7 +1810,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; try { - const plugin = new FilesPlugin(brokenConfig); + const plugin = filesPlugin(brokenConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1862,7 +1848,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; try { - const plugin = new FilesPlugin(brokenConfig); + const plugin = filesPlugin(brokenConfig); const handle = plugin.exports()("broken"); await expect(handle.list()).rejects.toThrow("policy crashed"); @@ -1889,7 +1875,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; try { - const plugin = new FilesPlugin(brokenConfig); + const plugin = filesPlugin(brokenConfig); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1914,7 +1900,7 @@ describe("FilesPlugin", () => { describe("_resolveAuth config inheritance", () => { test("volume-level auth overrides plugin default", () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ auth: "service-principal", volumes: { uploads: { auth: "on-behalf-of-user" }, @@ -1925,7 +1911,7 @@ describe("FilesPlugin", () => { }); test("volume without auth inherits plugin default", () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ auth: "on-behalf-of-user", volumes: { uploads: {}, @@ -1936,7 +1922,7 @@ describe("FilesPlugin", () => { }); test("neither volume nor plugin sets auth → defaults to service-principal", () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: {}, exports: {}, @@ -2067,7 +2053,7 @@ describe("FilesPlugin", () => { test("OBO volume + valid token → policy receives { isServicePrincipal: false, id: }", async () => { const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2108,7 +2094,7 @@ describe("FilesPlugin", () => { test("OBO volume + missing token + NODE_ENV != 'development' → 401, no SDK call", async () => { process.env.NODE_ENV = "production"; const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2144,7 +2130,7 @@ describe("FilesPlugin", () => { test("OBO volume + missing token + NODE_ENV === 'development' → exactly one warn, SP fallback proceeds", async () => { process.env.NODE_ENV = "development"; const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2201,7 +2187,7 @@ describe("FilesPlugin", () => { test("OBO volume + valid token + policy denies → 403 PolicyDeniedError", async () => { const policySpy = vi.fn().mockReturnValue(false); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2324,7 +2310,7 @@ describe("FilesPlugin", () => { test("OBO list + valid token wraps SDK call in user context (alice's userId resolves inside the wrapped fn)", async () => { await useRealGetCurrentUserId(); const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2367,7 +2353,7 @@ describe("FilesPlugin", () => { test("OBO read happy path: valid token + policy allows + UC allows → 200", async () => { const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2433,7 +2419,7 @@ describe("FilesPlugin", () => { // would re-enable cache here. await useRealGetCurrentUserId(); const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2441,6 +2427,7 @@ describe("FilesPlugin", () => { }, }); const handler = getRouteHandler(plugin, "get", "/list"); + const getOrExecute = vi.spyOn(testCache, "getOrExecute"); mockClient.files.listDirectoryContents.mockImplementation( async function* () { @@ -2468,13 +2455,13 @@ describe("FilesPlugin", () => { // Cache is disabled on OBO: `getOrExecute` is bypassed. The SDK // must execute on every request — no cross-user staleness possible. - expect(mockCacheInstance.getOrExecute).not.toHaveBeenCalled(); + expect(getOrExecute).not.toHaveBeenCalled(); expect(mockClient.files.listDirectoryContents).toHaveBeenCalledTimes(2); }); test("SP volume reads still use the cache (cache is only disabled for OBO)", async () => { await useRealGetCurrentUserId(); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -2487,6 +2474,7 @@ describe("FilesPlugin", () => { }); const listHandler = getRouteHandler(plugin, "get", "/list"); + const getOrExecute = vi.spyOn(testCache, "getOrExecute"); mockClient.files.listDirectoryContents.mockImplementation( async function* () { @@ -2512,7 +2500,7 @@ describe("FilesPlugin", () => { mockRes(), ); - const calls = mockCacheInstance.getOrExecute.mock.calls; + const calls = getOrExecute.mock.calls; // Exactly one cache consultation — the SP volume's. The OBO request // bypassed the cache entirely. expect(calls).toHaveLength(1); @@ -2538,7 +2526,7 @@ describe("FilesPlugin", () => { */ test("OBO HTTP request builds the UserContext exactly once (no duplicate WorkspaceClient allocation)", async () => { const policySpy = vi.fn().mockReturnValue(true); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2766,7 +2754,7 @@ describe("FilesPlugin", () => { .mockResolvedValue({ ok: true, status: 200, text: async () => "" }); vi.stubGlobal("fetch", fetchSpy); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -2817,7 +2805,7 @@ describe("FilesPlugin", () => { const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -2859,7 +2847,7 @@ describe("FilesPlugin", () => { test("OBO mkdir + policy denies → 403 PolicyDeniedError; SDK not invoked", async () => { const policySpy = vi.fn().mockReturnValue(false); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policySpy }, uploads: {}, @@ -2936,7 +2924,7 @@ describe("FilesPlugin", () => { }), ); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -2983,7 +2971,7 @@ describe("FilesPlugin", () => { // SP volume — uses the cache. process.env.DATABRICKS_VOLUME_SP_VOL = "/Volumes/c/s/sp"; try { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { sp_vol: { policy: policy.allowAll() }, uploads: {}, @@ -2996,16 +2984,8 @@ describe("FilesPlugin", () => { // Track which (parts, userKey) pairs go through generateKey so we // can match the invalidation segment exactly. - const generateKeyCalls: Array<{ - parts: (string | number | object)[]; - userKey: string; - }> = []; - mockCacheInstance.generateKey.mockImplementation( - (parts: (string | number | object)[], userKey: string) => { - generateKeyCalls.push({ parts, userKey }); - return "stub-key"; - }, - ); + // Records against production's own generateKey — the spy calls through. + const generateKey = vi.spyOn(testCache, "generateKey"); await mkdirHandler( mockReq("sp_vol", {}, { body: { path: "/Volumes/c/s/sp/foo/bar" } }), @@ -3013,8 +2993,8 @@ describe("FilesPlugin", () => { ); // Exactly one list-cache invalidation key was constructed. - const listInvalidations = generateKeyCalls.filter( - (c) => Array.isArray(c.parts) && c.parts[0] === "files:sp_vol:list", + const listInvalidations = generateKey.mock.calls.filter( + (c) => Array.isArray(c[0]) && c[0][0] === "files:sp_vol:list", ); expect(listInvalidations).toHaveLength(1); @@ -3022,12 +3002,10 @@ describe("FilesPlugin", () => { // written path. `parentDirectory("/Volumes/c/s/sp/foo/bar")` // returns `"/Volumes/c/s/sp/foo"`, which the connector resolves // unchanged because it's already absolute and starts with /Volumes/. - expect(listInvalidations[0].parts[1]).toBe("/Volumes/c/s/sp/foo"); + expect(listInvalidations[0][0][1]).toBe("/Volumes/c/s/sp/foo"); // Defense-in-depth: the file path itself must NOT appear as the // segment. - expect(listInvalidations[0].parts[1]).not.toBe( - "/Volumes/c/s/sp/foo/bar", - ); + expect(listInvalidations[0][0][1]).not.toBe("/Volumes/c/s/sp/foo/bar"); } finally { delete process.env.DATABRICKS_VOLUME_SP_VOL; } @@ -3057,7 +3035,7 @@ describe("FilesPlugin", () => { ]; for (const { label, writePath } of rootInvalidationCases) { test(`SP write at ${label} invalidates __root__ + volumeRoot variants (matches _handleList's possible cache keys)`, async () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: { policy: policy.allowAll() }, exports: {}, @@ -3067,26 +3045,18 @@ describe("FilesPlugin", () => { mockClient.files.createDirectory.mockResolvedValue(undefined); - const generateKeyCalls: Array<{ - parts: (string | number | object)[]; - userKey: string; - }> = []; - mockCacheInstance.generateKey.mockImplementation( - (parts: (string | number | object)[], userKey: string) => { - generateKeyCalls.push({ parts, userKey }); - return "stub-key"; - }, - ); + // Records against production's own generateKey — the spy calls through. + const generateKey = vi.spyOn(testCache, "generateKey"); await mkdirHandler( mockReq("uploads", {}, { body: { path: writePath } }), mockRes(), ); - const listInvalidations = generateKeyCalls.filter( - (c) => Array.isArray(c.parts) && c.parts[0] === "files:uploads:list", + const listInvalidations = generateKey.mock.calls.filter( + (c) => Array.isArray(c[0]) && c[0][0] === "files:uploads:list", ); - const segments = listInvalidations.map((c) => c.parts[1]); + const segments = listInvalidations.map((c) => c[0][1]); expect(segments).toEqual( expect.arrayContaining([ "__root__", @@ -3134,7 +3104,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_SP_VOL = "/Volumes/c/s/sp"; try { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { sp_vol: { policy: policy.allowAll() }, uploads: {}, @@ -3144,7 +3114,7 @@ describe("FilesPlugin", () => { const mkdirHandler = getRouteHandler(plugin, "post", "/mkdir"); mockClient.files.createDirectory.mockResolvedValue(undefined); - mockCacheInstance.generateKey.mockReturnValue("stub-key"); + vi.spyOn(testCache, "generateKey").mockReturnValue("stub-key"); // Deferred promise that gates the cache delete. The handler must // await this before writing the success response. @@ -3152,9 +3122,9 @@ describe("FilesPlugin", () => { const deletePending = new Promise((resolve) => { releaseDelete = resolve; }); - mockCacheInstance.delete.mockImplementation( - async () => await deletePending, - ); + const cacheDelete = vi + .spyOn(testCache, "delete") + .mockImplementation(async () => await deletePending); const res = mockRes(); @@ -3172,15 +3142,12 @@ describe("FilesPlugin", () => { // Use setImmediate to also drain macrotask queue items (telemetry/ // timeout interceptors may use setTimeout under the hood). const deadline = Date.now() + 1000; - while ( - mockCacheInstance.delete.mock.calls.length === 0 && - Date.now() < deadline - ) { + while (cacheDelete.mock.calls.length === 0 && Date.now() < deadline) { await new Promise((resolve) => setImmediate(resolve)); } expect(mockClient.files.createDirectory).toHaveBeenCalledTimes(1); - expect(mockCacheInstance.delete).toHaveBeenCalledTimes(1); + expect(cacheDelete).toHaveBeenCalledTimes(1); // Critical assertion: drain plenty of microtasks AND macrotasks // while `cache.delete` is still parked on the deferred. If the @@ -3223,7 +3190,7 @@ describe("FilesPlugin", () => { () => "test-service-principal", ); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -3391,7 +3358,7 @@ describe("FilesPlugin", () => { // connector picks it up. process.env.DATABRICKS_VOLUME_SP_VOL = "/Volumes/c/s/sp"; try { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { // SP-configured volume (the auth: "service-principal" default). sp_vol: { auth: "service-principal", policy: policy.allowAll() }, @@ -3447,7 +3414,7 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_OBO_VOL = "/Volumes/c/s/obo"; try { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", @@ -3481,7 +3448,7 @@ describe("FilesPlugin", () => { serviceContextMock.createUserContextSpy.mockClear(); - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); await handle.list("subdir"); // no asUser; pure SP path @@ -3504,7 +3471,7 @@ describe("FilesPlugin", () => { */ test("asUser in production with x-forwarded-user but no x-forwarded-access-token throws AuthenticationError.missingToken (privilege-confusion guard)", () => { process.env.NODE_ENV = "production"; - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); const reqWithUserOnly = { header: (name: string) => @@ -3541,7 +3508,7 @@ describe("FilesPlugin", () => { serviceContextMock.createUserContextSpy.mockClear(); - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: { policy: policySpy }, exports: {}, @@ -3680,7 +3647,7 @@ describe("FilesPlugin", () => { }); test("OBO volume + HTTP route + valid token → span attribute is 'on-behalf-of-user'", async () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policy.allowAll() }, uploads: {}, @@ -3712,7 +3679,7 @@ describe("FilesPlugin", () => { }); test("SP volume + HTTP route → span attribute is 'service-principal'", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); mockClient.files.listDirectoryContents.mockImplementation( @@ -3738,7 +3705,7 @@ describe("FilesPlugin", () => { }); test("appKit.files('sp-vol').asUser(req).list() programmatic → span attribute is 'on-behalf-of-user' (asUser forces it on SP volumes)", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); mockClient.files.listDirectoryContents.mockImplementation( @@ -3775,7 +3742,7 @@ describe("FilesPlugin", () => { * AsyncLocalStorage. This test pins span count == 1. */ test("programmatic asUser().list() produces exactly ONE files.list span (no duplicate parent span)", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); mockClient.files.listDirectoryContents.mockImplementation( @@ -3804,7 +3771,7 @@ describe("FilesPlugin", () => { }); test("programmatic SP-volume .list() produces exactly ONE files.list span tagged service-principal", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const calls = spyOnTelemetry(plugin); mockClient.files.listDirectoryContents.mockImplementation( @@ -3831,7 +3798,7 @@ describe("FilesPlugin", () => { // for (OBO). process.env.NODE_ENV = "development"; - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { obo_vol: { auth: "on-behalf-of-user", policy: policy.allowAll() }, uploads: {}, diff --git a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts index 5614e8b37..33fe2ebad 100644 --- a/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts +++ b/packages/appkit/src/plugins/files/tests/raw-endpoint.test.ts @@ -11,7 +11,7 @@ import { VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -35,17 +35,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -67,12 +57,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin raw endpoint security headers", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/files/tests/shutdown.test.ts b/packages/appkit/src/plugins/files/tests/shutdown.test.ts index 239d92cff..c743d8978 100644 --- a/packages/appkit/src/plugins/files/tests/shutdown.test.ts +++ b/packages/appkit/src/plugins/files/tests/shutdown.test.ts @@ -3,7 +3,7 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { FilesPlugin } from "../plugin"; import { setupTestEnv, teardownTestEnv, VOLUMES_CONFIG } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -27,17 +27,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -59,12 +49,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin shutdown and trackWrite", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/files/tests/upload.test.ts b/packages/appkit/src/plugins/files/tests/upload.test.ts index 0e893cd59..e1abdd839 100644 --- a/packages/appkit/src/plugins/files/tests/upload.test.ts +++ b/packages/appkit/src/plugins/files/tests/upload.test.ts @@ -1,17 +1,18 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { FilesPlugin } from "../plugin"; import { policy } from "../policy"; import { + filesPlugin, getRouteHandler, mockRes, mockUploadReq, setupTestEnv, teardownTestEnv, + testCache, VOLUMES_CONFIG, } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -35,17 +36,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -67,12 +58,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin upload", () => { let serviceContextMock: Awaited>; @@ -86,7 +71,7 @@ describe("FilesPlugin upload", () => { describe("Upload stream mid-transfer size enforcement", () => { test("upload exceeding size mid-stream is caught by execute and returns error", async () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: { maxUploadSize: 50, policy: policy.allowAll() }, }, @@ -131,7 +116,7 @@ describe("FilesPlugin upload", () => { // The outer catch in _handleUpload has a specific check for the // "exceeds maximum allowed size" message. This tests that path by // making execute() re-throw instead of catching. - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: { maxUploadSize: 50, policy: policy.allowAll() }, }, @@ -160,7 +145,7 @@ describe("FilesPlugin upload", () => { }); test("upload within size limit succeeds", async () => { - const plugin = new FilesPlugin({ + const plugin = filesPlugin({ volumes: { uploads: { maxUploadSize: 100, policy: policy.allowAll() }, }, @@ -198,9 +183,12 @@ describe("FilesPlugin upload", () => { describe("Upload cache invalidation", () => { test("successful upload calls cache.delete for parent directory", async () => { - const plugin = new FilesPlugin(VOLUMES_CONFIG); + const plugin = filesPlugin(VOLUMES_CONFIG); const handler = getRouteHandler(plugin, "post", "/upload"); const res = mockRes(); + // Production's own keying and invalidation, not a fake's. + const generateKey = vi.spyOn(testCache, "generateKey"); + const invalidate = vi.spyOn(testCache, "delete"); const req = mockUploadReq("uploads", [Buffer.from("file content")], { query: { path: "/Volumes/catalog/schema/uploads/dir/file.txt" }, @@ -223,8 +211,8 @@ describe("FilesPlugin upload", () => { expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ success: true }), ); - expect(mockCacheInstance.generateKey).toHaveBeenCalled(); - expect(mockCacheInstance.delete).toHaveBeenCalled(); + expect(generateKey).toHaveBeenCalled(); + expect(invalidate).toHaveBeenCalled(); }); }); }); diff --git a/packages/appkit/src/plugins/files/tests/volume-config.test.ts b/packages/appkit/src/plugins/files/tests/volume-config.test.ts index 121bdbe39..9e524fbcb 100644 --- a/packages/appkit/src/plugins/files/tests/volume-config.test.ts +++ b/packages/appkit/src/plugins/files/tests/volume-config.test.ts @@ -3,7 +3,7 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { FilesPlugin } from "../plugin"; import { setupTestEnv, teardownTestEnv, VOLUMES_CONFIG } from "./_test-helpers"; -const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { +const { mockClient, MockApiError } = vi.hoisted(() => { const mockFilesApi = { listDirectoryContents: vi.fn(), download: vi.fn(), @@ -27,17 +27,7 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { this.statusCode = statusCode; } } - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockClient, MockApiError, mockCacheInstance }; + return { mockClient, MockApiError }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -59,12 +49,6 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - describe("FilesPlugin volume config surface", () => { let serviceContextMock: Awaited>; diff --git a/packages/appkit/src/plugins/genie/tests/genie.test.ts b/packages/appkit/src/plugins/genie/tests/genie.test.ts index 2d867d335..f4882fdbc 100644 --- a/packages/appkit/src/plugins/genie/tests/genie.test.ts +++ b/packages/appkit/src/plugins/genie/tests/genie.test.ts @@ -14,34 +14,6 @@ import { Plugin } from "../../../plugin"; import { GeniePlugin, genie } from "../genie"; import type { IGenieConfig } from "../types"; -// Mock CacheManager singleton -const { mockCacheInstance } = vi.hoisted(() => { - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi - .fn() - .mockImplementation( - async ( - _key: unknown[], - fn: (signal?: AbortSignal) => Promise, - ) => { - return await fn(); - }, - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - - return { mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - function createMockGenieService() { const getMessageAttachmentQueryResult = vi.fn(); diff --git a/packages/appkit/src/plugins/jobs/tests/plugin.test.ts b/packages/appkit/src/plugins/jobs/tests/plugin.test.ts index 783debc8a..6259fa27d 100644 --- a/packages/appkit/src/plugins/jobs/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/jobs/tests/plugin.test.ts @@ -4,6 +4,8 @@ import { z } from "zod"; import { ServiceContext } from "../../../context/service-context"; import { ResourceType } from "../../../registry"; +import { createTestPluginContext, resetTestCache } from "../../../testing"; +import { ApiError } from "../../../workspace-client"; import { JOBS_READ_DEFAULTS, JOBS_STREAM_DEFAULTS, @@ -12,7 +14,7 @@ import { import { mapParams } from "../params"; import { JobsPlugin, jobs } from "../plugin"; -const { mockClient, mockCacheInstance } = vi.hoisted(() => { +const { mockClient } = vi.hoisted(() => { const mockJobsApi = { runNow: vi.fn(), submit: vi.fn(), @@ -31,18 +33,7 @@ const { mockClient, mockCacheInstance } = vi.hoisted(() => { }, }; - const mockCacheInstance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_key: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(), - }; - - return { mockJobsApi, mockClient, mockCacheInstance }; + return { mockJobsApi, mockClient }; }); vi.mock("../../../workspace-client", async (importOriginal) => { @@ -64,11 +55,15 @@ vi.mock("../../../context", async (importOriginal) => { }; }); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); +/** + * One kit context for this file — Vitest isolates files — supplying the real + * `CacheManager` the read paths need. `attachContext` is the production binding + * and is synchronous, so tests stay unchanged. + */ +const kit = createTestPluginContext(); +function jobsPlugin(...args: ConstructorParameters) { + return kit.attach(new JobsPlugin(...args)); +} describe("JobsPlugin", () => { let serviceContextMock: Awaited>; @@ -78,6 +73,9 @@ describe("JobsPlugin", () => { setupDatabricksEnv(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); + // The cache is real now, so entries outlive a test unless cleared — one + // test's cached run would otherwise answer the next one's read. + await resetTestCache(); }); afterEach(() => { @@ -97,7 +95,7 @@ describe("JobsPlugin", () => { test("plugin instance has correct name", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); expect(plugin.name).toBe("jobs"); }); @@ -242,7 +240,7 @@ describe("JobsPlugin", () => { test("returns a callable function", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(typeof exported).toBe("function"); @@ -251,7 +249,7 @@ describe("JobsPlugin", () => { test("returns job handle with direct JobAPI methods", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); const handle = exported("etl"); @@ -268,7 +266,7 @@ describe("JobsPlugin", () => { test("throws for unknown job key", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(() => exported("unknown")).toThrow(/Unknown job "unknown"/); @@ -277,7 +275,7 @@ describe("JobsPlugin", () => { test("single-job default key is accessible", () => { process.env.DATABRICKS_JOB_ID = "789"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(() => exported("default")).not.toThrow(); @@ -292,7 +290,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); const handle = exported("etl"); @@ -309,7 +307,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); const handle = exported("etl"); @@ -331,7 +329,7 @@ describe("JobsPlugin", () => { test("runNow validates params against job config schema", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { taskType: "notebook", @@ -351,7 +349,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { taskType: "notebook", @@ -377,7 +375,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); await expect(handle.runNow({ anything: "goes" })).resolves.not.toThrow(); @@ -393,7 +391,7 @@ describe("JobsPlugin", () => { state: { life_cycle_state: "TERMINATED" }, }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const executeSpy = vi.spyOn(plugin as any, "execute"); const handle = plugin.exports()("etl"); @@ -417,7 +415,7 @@ describe("JobsPlugin", () => { mockClient.jobs.get.mockResolvedValue({ job_id: 123 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const executeSpy = vi.spyOn(plugin as any, "execute"); const handle = plugin.exports()("etl"); @@ -441,7 +439,7 @@ describe("JobsPlugin", () => { mockClient.jobs.listRuns.mockReturnValue((async function* () {})()); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); await handle.listRuns({ limit: 10000 }); @@ -460,7 +458,7 @@ describe("JobsPlugin", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 1, job_id: 123 }); mockClient.jobs.cancelRun.mockResolvedValue(undefined); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const executeSpy = vi.spyOn(plugin as any, "execute"); const handle = plugin.exports()("etl"); @@ -489,7 +487,7 @@ describe("JobsPlugin", () => { state: { life_cycle_state: "TERMINATED" }, }); - const plugin = new JobsPlugin({ pollIntervalMs: 10 }); + const plugin = jobsPlugin({ pollIntervalMs: 10 }); const handle = plugin.exports()("etl"); const statuses: any[] = []; @@ -507,7 +505,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({}); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const gen = handle.runAndWait(); @@ -523,7 +521,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockRejectedValue(new Error("API timeout")); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.runNow(); @@ -542,7 +540,7 @@ describe("JobsPlugin", () => { new Error("Permission denied"), ); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.cancelRun(42); @@ -560,7 +558,7 @@ describe("JobsPlugin", () => { new Error("Internal server error"), ); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.getRun(42); @@ -578,7 +576,7 @@ describe("JobsPlugin", () => { throw new Error("Auth failure"); }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.listRuns(); @@ -592,11 +590,16 @@ describe("JobsPlugin", () => { test("error result preserves upstream HTTP status code", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const error = new Error("Detailed internal failure: db connection reset"); - (error as any).statusCode = 403; + const error = new ApiError( + "Detailed internal failure: db connection reset", + "PERMISSION_DENIED", + 403, + undefined, + [], + ); mockClient.jobs.getRun.mockRejectedValue(error); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.getRun(42); @@ -613,7 +616,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.runNow(); @@ -630,7 +633,7 @@ describe("JobsPlugin", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 99, job_id: 456 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.getRun(99); @@ -644,7 +647,7 @@ describe("JobsPlugin", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 99, job_id: 456 }); mockClient.jobs.getRunOutput.mockResolvedValue({ logs: "nope" }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.getRunOutput(99); @@ -660,7 +663,7 @@ describe("JobsPlugin", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 99, job_id: 456 }); mockClient.jobs.cancelRun.mockResolvedValue(undefined); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.cancelRun(99); @@ -678,7 +681,7 @@ describe("JobsPlugin", () => { state: { life_cycle_state: "TERMINATED" }, }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const handle = plugin.exports()("etl"); const result = await handle.getRun(42); @@ -724,7 +727,7 @@ describe("JobsPlugin", () => { state: { life_cycle_state: "RUNNING" }, }); - const plugin = new JobsPlugin({ pollIntervalMs: 10 }); + const plugin = jobsPlugin({ pollIntervalMs: 10 }); const handle = plugin.exports()("etl"); const controller = new AbortController(); @@ -745,7 +748,7 @@ describe("JobsPlugin", () => { process.env.DATABRICKS_JOB_ETL = "123"; process.env.DATABRICKS_JOB_ML = "456"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const config = plugin.clientConfig(); expect(config).toEqual({ @@ -759,7 +762,7 @@ describe("JobsPlugin", () => { test("returns single default key for DATABRICKS_JOB_ID", () => { process.env.DATABRICKS_JOB_ID = "789"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const config = plugin.clientConfig(); expect(config).toEqual({ @@ -770,7 +773,7 @@ describe("JobsPlugin", () => { }); test("returns empty jobs when no jobs configured", () => { - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const config = plugin.clientConfig(); expect(config).toEqual({ jobs: {} }); @@ -779,7 +782,7 @@ describe("JobsPlugin", () => { test("includes JSON schema when params schema is configured", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { params: z.object({ key: z.string() }), @@ -800,13 +803,13 @@ describe("JobsPlugin", () => { test("jobs() with no config discovers from env vars", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(() => exported("etl")).not.toThrow(); }); test("jobs() with no config and no env vars creates no jobs", () => { - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(() => exported("etl")).toThrow(/Unknown job/); }); @@ -817,7 +820,7 @@ describe("JobsPlugin", () => { process.env.DATABRICKS_JOB_ETL = "100"; process.env.DATABRICKS_JOB_ML = "200"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); expect(() => exported("etl")).not.toThrow(); @@ -831,7 +834,7 @@ describe("JobsPlugin", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 1 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const exported = plugin.exports(); await exported("etl").runNow(); @@ -930,6 +933,9 @@ describe("injectRoutes", () => { setupDatabricksEnv(); ServiceContext.reset(); serviceContextMock = await mockServiceContext(); + // The cache is real now, so entries outlive a test unless cleared — one + // test's cached run would otherwise answer the next one's read. + await resetTestCache(); }); afterEach(() => { @@ -941,7 +947,7 @@ describe("injectRoutes", () => { test("registers all 5 routes via this.route()", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { @@ -965,7 +971,7 @@ describe("injectRoutes", () => { test("registers correct HTTP methods and paths", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { @@ -1013,7 +1019,7 @@ describe("injectRoutes", () => { test("returns 404 for unknown job key", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const resolveJob = (plugin as any)._resolveJob.bind(plugin); const mockReq = { params: { jobKey: "unknown" } } as any; @@ -1037,7 +1043,7 @@ describe("injectRoutes", () => { test("sanitizes special characters in unknown job key error", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const resolveJob = (plugin as any)._resolveJob.bind(plugin); const mockReq = { @@ -1060,7 +1066,7 @@ describe("injectRoutes", () => { test("returns jobKey and jobId for known job", () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const resolveJob = (plugin as any)._resolveJob.bind(plugin); const mockReq = { params: { jobKey: "etl" } } as any; @@ -1083,7 +1089,7 @@ describe("injectRoutes", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1118,7 +1124,7 @@ describe("injectRoutes", () => { test("returns 400 when params sent to job without taskType or schema", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1154,7 +1160,7 @@ describe("injectRoutes", () => { test("returns 400 on parameter validation failure", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { taskType: "notebook", @@ -1209,7 +1215,7 @@ describe("injectRoutes", () => { })(), ); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1243,7 +1249,7 @@ describe("injectRoutes", () => { mockClient.jobs.listRuns.mockReturnValue((async function* () {})()); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1286,7 +1292,7 @@ describe("injectRoutes", () => { }; mockClient.jobs.getRun.mockResolvedValue(mockRun); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1316,7 +1322,7 @@ describe("injectRoutes", () => { test("returns 400 for invalid runId", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1353,7 +1359,7 @@ describe("injectRoutes", () => { // Run exists upstream but is owned by job 456, not the configured 123. mockClient.jobs.getRun.mockResolvedValue({ run_id: 99, job_id: 456 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1399,7 +1405,7 @@ describe("injectRoutes", () => { })(), ); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1434,7 +1440,7 @@ describe("injectRoutes", () => { mockClient.jobs.listRuns.mockReturnValue((async function* () {})()); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1472,7 +1478,7 @@ describe("injectRoutes", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 42, job_id: 123 }); mockClient.jobs.cancelRun.mockResolvedValue(undefined); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1504,7 +1510,7 @@ describe("injectRoutes", () => { test("returns 400 for invalid runId", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1543,7 +1549,7 @@ describe("injectRoutes", () => { mockClient.jobs.getRun.mockResolvedValue({ run_id: 99, job_id: 456 }); mockClient.jobs.cancelRun.mockResolvedValue(undefined); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1577,7 +1583,7 @@ describe("injectRoutes", () => { test("returns 404 for unknown job key", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1610,7 +1616,7 @@ describe("injectRoutes", () => { test("returns 400 when params is an array", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1645,7 +1651,7 @@ describe("injectRoutes", () => { test("returns 400 when params is a string", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1681,7 +1687,7 @@ describe("injectRoutes", () => { process.env.DATABRICKS_JOB_ETL = "123"; // Job has a taskType but no Zod schema — the cap should kick in. - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { taskType: "notebook" } }, }); const routeSpy = vi.spyOn(plugin as any, "route"); @@ -1724,7 +1730,7 @@ describe("injectRoutes", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({ + const plugin = jobsPlugin({ jobs: { etl: { taskType: "notebook" } }, }); const routeSpy = vi.spyOn(plugin as any, "route"); @@ -1768,7 +1774,7 @@ describe("injectRoutes", () => { mockClient.jobs.runNow.mockResolvedValue({ run_id: 42 }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1809,7 +1815,7 @@ describe("injectRoutes", () => { (error as any).statusCode = 403; mockClient.jobs.runNow.mockRejectedValue(error); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1847,13 +1853,18 @@ describe("injectRoutes", () => { test("GET /:jobKey/runs returns upstream status on failure", async () => { process.env.DATABRICKS_JOB_ETL = "123"; - const error = new Error("Unauthorized"); - (error as any).statusCode = 401; + const error = new ApiError( + "Unauthorized", + "UNAUTHENTICATED", + 401, + undefined, + [], + ); mockClient.jobs.listRuns.mockImplementation(() => { throw error; }); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; @@ -1889,7 +1900,7 @@ describe("injectRoutes", () => { (error as any).statusCode = 403; mockClient.jobs.cancelRun.mockRejectedValue(error); - const plugin = new JobsPlugin({}); + const plugin = jobsPlugin({}); const routeSpy = vi.spyOn(plugin as any, "route"); const mockRouter = { get: vi.fn(), post: vi.fn(), delete: vi.fn() }; diff --git a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts index 7e035bdea..27862deea 100644 --- a/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts +++ b/packages/appkit/src/plugins/lakebase/tests/lakebase-agent-tool.test.ts @@ -9,21 +9,6 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; * (SP or per-user via RoutingPool). */ -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => ({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi.fn( - async (_k: unknown[], fn: (signal?: AbortSignal) => Promise) => - fn(), - ), - generateKey: vi.fn(() => "test-key"), - })), - }, -})); - // Client calls recorded by the read-only-statement test. The `connect()` // mock returns a fresh client whose `query` pushes to this array so tests // can assert the exact sequence of statements emitted on the dedicated diff --git a/packages/appkit/src/plugins/server/tests/server.test.ts b/packages/appkit/src/plugins/server/tests/server.test.ts index 90a6b339b..b7647917e 100644 --- a/packages/appkit/src/plugins/server/tests/server.test.ts +++ b/packages/appkit/src/plugins/server/tests/server.test.ts @@ -1,8 +1,12 @@ import type { BasePlugin } from "shared"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import type { CacheManager } from "../../../cache"; import { PluginContext } from "../../../core/plugin-context"; +/** A CacheManager these server tests carry but never exercise. */ +const cacheStub = {} as unknown as CacheManager; + // Use vi.hoisted for mocks that need to be available before module loading const { mockHttpServer, @@ -107,17 +111,6 @@ vi.mock("../../../telemetry", () => ({ }, })); -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn().mockReturnValue({ - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - close: vi.fn().mockResolvedValue(undefined), - }), - }, -})); - vi.mock("../../../utils", () => ({ deepMerge: vi.fn((a, b) => ({ ...a, ...b })), })); @@ -203,7 +196,7 @@ import { StaticServer } from "../static-server"; import { ViteDevServer } from "../vite-dev-server"; function createContextWithPlugins(plugins: Record): PluginContext { - const ctx = new PluginContext(); + const ctx = new PluginContext({ cache: cacheStub }); for (const [name, instance] of Object.entries(plugins)) { ctx.registerPlugin(name, instance as BasePlugin); } @@ -821,7 +814,7 @@ describe("ServerPlugin", () => { order.push("closeAll"); }); - const ctx = new PluginContext(); + const ctx = new PluginContext({ cache: cacheStub }); const server = new ServerPlugin({ context: ctx } as any); ctx.registerPlugin("server", server as unknown as BasePlugin); ctx.registerPlugin("peer", { @@ -839,7 +832,10 @@ describe("ServerPlugin", () => { }) as any); await server.start(); - await new LifecycleManager(ctx).shutdown(); + // The manager is injected now; only `close()` is exercised here. + await new LifecycleManager(ctx, { + close: vi.fn().mockResolvedValue(undefined), + } as unknown as import("../../../cache").CacheManager).shutdown(); // closeIdle fires in the abort phase, the peer drains next, closeAll only // fires in the later lifecycle-emit phase, then the process exits 0. diff --git a/packages/appkit/src/plugins/serving/tests/serving.test.ts b/packages/appkit/src/plugins/serving/tests/serving.test.ts index bca2f091a..7dffefa9a 100644 --- a/packages/appkit/src/plugins/serving/tests/serving.test.ts +++ b/packages/appkit/src/plugins/serving/tests/serving.test.ts @@ -13,33 +13,6 @@ import { ServiceContext } from "../../../context/service-context"; import { ServingPlugin, serving } from "../serving"; import type { IServingConfig } from "../types"; -// Mock CacheManager singleton -const { mockCacheInstance } = vi.hoisted(() => { - const instance = { - get: vi.fn(), - set: vi.fn(), - delete: vi.fn(), - getOrExecute: vi - .fn() - .mockImplementation( - async ( - _key: unknown[], - fn: (signal?: AbortSignal) => Promise, - ) => { - return await fn(); - }, - ), - generateKey: vi.fn((...args: unknown[]) => JSON.stringify(args)), - }; - return { mockCacheInstance: instance }; -}); - -vi.mock("../../../cache", () => ({ - CacheManager: { - getInstanceSync: vi.fn(() => mockCacheInstance), - }, -})); - // Mock the serving connector const mockInvoke = vi.fn(); const mockStream = vi.fn(); diff --git a/packages/appkit/src/testing/fixtures.ts b/packages/appkit/src/testing/fixtures.ts index f3663b430..3040caf30 100644 --- a/packages/appkit/src/testing/fixtures.ts +++ b/packages/appkit/src/testing/fixtures.ts @@ -2,7 +2,7 @@ import type { Span, SpanOptions } from "@opentelemetry/api"; import type { IAppRouter } from "shared"; import { afterEach, beforeEach, vi } from "vitest"; -import { CacheManager } from "../cache"; +import type { CacheManager } from "../cache"; import type { ServiceContextState } from "../context/service-context"; import { ServiceContext } from "../context/service-context"; import type { InstrumentConfig, ITelemetry } from "../telemetry/types"; @@ -285,16 +285,38 @@ export function setupDatabricksEnv(overrides: Record = {}) { } /** - * Clears AppKit's process-wide cache singleton so cached values don't leak - * between tests in the same file. + * The caches `createTestPluginContext` built for the current test file. Module- + * level on purpose: Vitest isolates test files in separate workers, so this set + * is per-file and never leaks across them. Lives here, next to + * {@link resetTestCache} — its only reader — rather than on `CacheManager`, + * which belongs to an app, not to test bookkeeping. * - * The cache `attach()` seeds is shared by every test in a file (Vitest isolates - * files, not tests within a file). Call this in `beforeEach` when one test's - * cached value must not be seen by the next, or mid-test to force a cache miss - * before asserting a subsequent hit. + * @internal + */ +const kitCaches = new Set(); + +/** + * Record a cache `createTestPluginContext` built, so a no-argument + * {@link resetTestCache} can find it. The kit's one cross-file seam. + * @internal + */ +export function registerKitCache(cache: CacheManager): void { + kitCaches.add(cache); +} + +/** + * Clears the caches this file's test contexts built, so cached values don't + * leak between tests in the same file. + * + * A `createTestPluginContext()` carries its own cache, and Vitest isolates test + * files rather than the tests within one — so that cache is shared by every + * test in the file. Call this in `beforeEach` when one test's cached value must + * not be seen by the next, or mid-test to force a cache miss before asserting a + * subsequent hit. * - * No-ops when the cache has not been initialized yet, so it is safe to call - * before any `attach()`. + * Pass a context or a manager to clear only that one. With no argument it + * clears every cache this kit built for the file, and no-ops when there are + * none — so it is safe to call before creating any context. * * @example * ```ts @@ -303,15 +325,18 @@ export function setupDatabricksEnv(overrides: Record = {}) { * }); * ``` */ -export async function resetTestCache(): Promise { - let cache: ReturnType; - try { - cache = CacheManager.getInstanceSync(); - } catch { - // Not initialized yet — nothing to clear. - return; - } - await cache.clear(); +export async function resetTestCache( + target?: { cache: CacheManager } | CacheManager, +): Promise { + const caches = target ? [resolveCache(target)] : [...kitCaches]; + + // No cache to clear is not an error: a suite may call this before it has + // created one. + await Promise.all(caches.map((cache) => cache.clear())); +} + +function resolveCache(target: { cache: CacheManager } | CacheManager) { + return "cache" in target ? target.cache : target; } /** diff --git a/packages/appkit/src/testing/test-plugin-context.ts b/packages/appkit/src/testing/test-plugin-context.ts index 721e3cdbe..fe25c22b0 100644 --- a/packages/appkit/src/testing/test-plugin-context.ts +++ b/packages/appkit/src/testing/test-plugin-context.ts @@ -12,7 +12,7 @@ import { isToolProvider, PluginContext } from "../core/plugin-context"; import { AuthenticationError } from "../errors"; import type { Plugin } from "../plugin"; import type { ITelemetry } from "../telemetry"; -import { createMockTelemetry } from "./fixtures"; +import { createMockTelemetry, registerKitCache } from "./fixtures"; /** * A concrete (non-function) fake tool response — returned as-is. Covers the @@ -119,6 +119,17 @@ export interface TestPluginContext { * from the real `TelemetryManager`, so plugin-internal spans do not land here. */ telemetry: ITelemetry; + /** + * The real in-memory {@link CacheManager} this context carries — the very + * object a plugin attached through {@link attach} resolves as `this.cache`. + * Spy or read it (`vi.spyOn(mock.cache, "getOrExecute")`, `generateKey`, + * `get`, `has`) to assert a plugin's real caching behaviour against + * production's own keying rather than a re-implemented fake. + * + * Each `createTestPluginContext()` gets its own, so two contexts in one file + * cannot see each other's entries. + */ + cache: CacheManager; /** * Tool dispatches observed across all fake providers, in call order. Live — * read it after the action under test runs. @@ -137,13 +148,13 @@ export interface TestPluginContext { */ registerProvider(name: string, tools: Record): void; /** - * Attach this context to a plugin the production way: seed an in-memory - * cache (if AppKit hasn't already), then call `plugin.attachContext`, which - * also rebuilds the plugin's telemetry and flips `isReady` to `true`. Await - * it before exercising handlers that read `this.context`, `this.cache`, or - * gate on `isReady`. Returns the same plugin for chaining. + * Attach this context to a plugin the production way: calls + * `plugin.attachContext`, which binds {@link cache}, rebuilds the plugin's + * telemetry, and flips `isReady` to `true`. Synchronous — call it before + * exercising handlers that read `this.context`, `this.cache`, or gate on + * `isReady`. Returns the same plugin for chaining. */ - attach

(plugin: P): Promise

; + attach

(plugin: P): P; } /** @@ -177,7 +188,13 @@ export function createTestPluginContext( fakes: FakeProviders = {}, ): TestPluginContext { const telemetry = createMockTelemetry(); - const ctx = new PluginContext({ telemetry }); + // Synchronous on purpose: `createTestPluginContext` is called at describe-body + // time, so it cannot await. `forStorage` skips the health check that the app's + // async `create()` performs, which in-memory storage does not need. + const cache = CacheManager.forStorage(new InMemoryStorage()); + // So `resetTestCache()` with no argument can find it. + registerKitCache(cache); + const ctx = new PluginContext({ telemetry, cache }); const toolCalls: RecordedToolCall[] = []; const routes: RecordedRoute[] = []; @@ -315,27 +332,35 @@ export function createTestPluginContext( registerProvider(name, tools); } - async function attach

(plugin: P): Promise

{ - // Seed a real in-memory cache if AppKit hasn't initialized one. Idempotent: - // getInstance returns any existing singleton (e.g. one a suite already set - // up) and ignores the storage argument in that case. - if (!cacheReady()) { - await CacheManager.getInstance({ storage: new InMemoryStorage({}) }); - } + function attach

(plugin: P): P { + // The context already carries this test's cache, so `attachContext` binds + // it the same way `createApp` binds an app's. A plugin attached here + // reaches only this context's cache, and a sibling context cannot + // observe it. plugin.attachContext({ context: ctx }); - // Mirror what AppKit core does after attachContext (core/appkit.ts): put - // the plugin in the registry so `getPlugins()`/`getPluginNames()`/ - // `hasPlugin()` and any sibling-plugin lookup behave as in production. Only - // register it as a tool provider when it actually is one AND its name does - // not collide with an injected fake — the fakes are the authored test - // doubles and must not be overwritten by the plugin under test. - ctx.registerPlugin(plugin.name, plugin as unknown as BasePlugin); - if (isToolProvider(plugin) && !providers.has(plugin.name)) { - ctx.registerToolProvider( - plugin.name, - plugin as unknown as Parameters[1], - ); + // Put the plugin in the registry (as AppKit core does after attachContext) + // so `getPlugins()`/`getPluginNames()`/`hasPlugin()` resolve it. A tool + // provider is also registered as one, unless its name collides with an + // injected fake — the fakes are the authored doubles and must win. + // + // First attach of a name wins: a test file that builds many instances of + // one plugin (the shared-kit factory pattern) re-attaches the same name, + // and re-registering would churn the registry and trip the production + // "registered more than once" warning. So later instances are bound (their + // `attachContext` runs) but not re-registered. The limit that follows: a + // sibling-plugin lookup through this context resolves the FIRST instance + // attached under a name, not the most recent — fine because the direct + // `mock.attach(...)` sites use a fresh context per test, and no factory + // suite does a sibling lookup that depends on which instance answers. + if (!ctx.hasPlugin(plugin.name)) { + ctx.registerPlugin(plugin.name, plugin as unknown as BasePlugin); + if (isToolProvider(plugin) && !providers.has(plugin.name)) { + ctx.registerToolProvider( + plugin.name, + plugin as unknown as Parameters[1], + ); + } } return plugin; } @@ -343,6 +368,7 @@ export function createTestPluginContext( return { ctx, telemetry, + cache, toolCalls, routes, providers, @@ -350,12 +376,3 @@ export function createTestPluginContext( attach, }; } - -function cacheReady(): boolean { - try { - CacheManager.getInstanceSync(); - return true; - } catch { - return false; - } -} diff --git a/packages/appkit/src/testing/tests/fixtures.test.ts b/packages/appkit/src/testing/tests/fixtures.test.ts index 39fc2fef1..e522f2076 100644 --- a/packages/appkit/src/testing/tests/fixtures.test.ts +++ b/packages/appkit/src/testing/tests/fixtures.test.ts @@ -1,13 +1,12 @@ import { afterEach, describe, expect, test, vi } from "vitest"; -import { CacheManager } from "../../cache"; -import { InMemoryStorage } from "../../cache/storage"; import { ServiceContext } from "../../context"; import { createMockRequest, resetTestCache, useServiceContextMock, } from "../fixtures"; +import { createTestPluginContext } from "../test-plugin-context"; describe("createMockRequest — obo option", () => { test("no obo leaves the forwarded identity headers unset", () => { @@ -79,26 +78,59 @@ describe("resetTestCache", () => { vi.restoreAllMocks(); }); - test("no-ops when the cache is not initialized", async () => { - // Force the uninitialized branch deterministically (there is no public - // un-initialize), so the try/catch is exercised regardless of test order. - vi.spyOn(CacheManager, "getInstanceSync").mockImplementation(() => { - throw new Error("not initialized"); - }); + test("no-ops when this file has no kit cache to clear", async () => { await expect(resetTestCache()).resolves.toBeUndefined(); }); - test("clears a populated cache", async () => { - // Seed the real singleton the way attach() does, then prove reset empties it. - const cache = await CacheManager.getInstance({ - storage: new InMemoryStorage({}), - }); - await cache.set("k", { hello: "world" }); - expect(await cache.get("k")).toEqual({ hello: "world" }); + test("clears the cache a test context carries", async () => { + const mock = createTestPluginContext(); + const key = mock.cache.generateKey(["k"], "user"); + await mock.cache.set(key, { hello: "world" }); + expect(await mock.cache.get(key)).toEqual({ hello: "world" }); + + await resetTestCache(); + + expect(await mock.cache.get(key)).toBeNull(); + }); + + test("clears every kit cache in the file, not just the newest", async () => { + // A file can hold several contexts, so "the most recent one" would clear + // the wrong cache when called mid-test. + const first = createTestPluginContext(); + const second = createTestPluginContext(); + const firstKey = first.cache.generateKey(["a"], "user"); + const secondKey = second.cache.generateKey(["b"], "user"); + await first.cache.set(firstKey, { n: 1 }); + await second.cache.set(secondKey, { n: 2 }); await resetTestCache(); - expect(await cache.get("k")).toBeNull(); + expect(await first.cache.get(firstKey)).toBeNull(); + expect(await second.cache.get(secondKey)).toBeNull(); + }); + + test("clears only the target it is given", async () => { + const kept = createTestPluginContext(); + const cleared = createTestPluginContext(); + const keptKey = kept.cache.generateKey(["keep"], "user"); + const clearedKey = cleared.cache.generateKey(["drop"], "user"); + await kept.cache.set(keptKey, { n: 1 }); + await cleared.cache.set(clearedKey, { n: 2 }); + + await resetTestCache(cleared); + + expect(await cleared.cache.get(clearedKey)).toBeNull(); + expect(await kept.cache.get(keptKey)).toEqual({ n: 1 }); + }); + + test("accepts a manager directly as well as a handle", async () => { + const mock = createTestPluginContext(); + const key = mock.cache.generateKey(["direct"], "user"); + await mock.cache.set(key, { n: 1 }); + + await resetTestCache(mock.cache); + + expect(await mock.cache.get(key)).toBeNull(); }); }); diff --git a/packages/appkit/src/testing/tests/test-plugin-context-cache.test.ts b/packages/appkit/src/testing/tests/test-plugin-context-cache.test.ts new file mode 100644 index 000000000..806a8a9c6 --- /dev/null +++ b/packages/appkit/src/testing/tests/test-plugin-context-cache.test.ts @@ -0,0 +1,111 @@ +import type { PluginManifest } from "shared"; +import { describe, expect, test, vi } from "vitest"; + +import { CacheManager } from "../../cache"; +import { Plugin } from "../../plugin"; +import { mockServiceContext } from "../fixtures"; +import { createTestPluginContext } from "../test-plugin-context"; + +/** + * The cache a `createTestPluginContext` carries, and the plugin that resolves it. + * + * The identity assertions here are the load-bearing ones: if the handle's cache + * were not the object the plugin uses, every spy in every suite would record + * nothing and the tests would still pass. + */ + +class CachingPlugin extends Plugin { + static manifest = { + name: "caching", + displayName: "Caching", + version: "0.0.0", + description: "Runs a cached execution", + resources: { required: [], optional: [] }, + } as unknown as PluginManifest<"caching">; + + /** `cache` is protected, so the read has to happen in-class. */ + boundCache(): CacheManager { + return this.cache; + } + + /** A cached read, the way a handler would do it. */ + fetch(work: () => Promise): Promise { + return this.execute(work, { + default: { cache: { enabled: true, cacheKey: ["caching", "fetch"] } }, + }); + } +} + +describe("createTestPluginContext's cache", () => { + test("the handle exposes a real CacheManager", () => { + const mock = createTestPluginContext(); + + expect(mock.cache).toBeInstanceOf(CacheManager); + }); + + test("an attached plugin resolves the handle's cache, not another", async () => { + const mock = createTestPluginContext(); + const plugin = await mock.attach(new CachingPlugin({})); + + // If these were different objects, a spy on `mock.cache` would silently + // record nothing while the suite stayed green. + expect(plugin.boundCache()).toBe(mock.cache); + }); + + test("two contexts in one file get independent caches", async () => { + const first = createTestPluginContext(); + const second = createTestPluginContext(); + + expect(second.cache).not.toBe(first.cache); + + const key = first.cache.generateKey(["probe"], "user"); + await first.cache.set(key, { from: "first" }); + + await expect(second.cache.get(key)).resolves.toBeNull(); + }); + + test("an attached plugin's cached path really caches", async () => { + const serviceContext = mockServiceContext(); + try { + const mock = createTestPluginContext(); + const plugin = await mock.attach(new CachingPlugin({})); + const work = vi.fn(async () => "value"); + + await plugin.fetch(work); + await plugin.fetch(work); + + // Production's own `getOrExecute`, so a second identical call is a hit. + expect(work).toHaveBeenCalledTimes(1); + } finally { + serviceContext.restore(); + } + }); + + test("a spy on the handle's cache records what the plugin did", async () => { + const serviceContext = mockServiceContext(); + try { + const mock = createTestPluginContext(); + const plugin = await mock.attach(new CachingPlugin({})); + const getOrExecute = vi.spyOn(mock.cache, "getOrExecute"); + + await plugin.fetch(async () => "value"); + + expect(getOrExecute).toHaveBeenCalledTimes(1); + // The key parts are the plugin's, hashed by production's `generateKey`. + expect(getOrExecute.mock.calls[0][0]).toEqual(["caching", "fetch"]); + } finally { + serviceContext.restore(); + } + }); + + test("the handle's key function is production's", () => { + const mock = createTestPluginContext(); + + const a = mock.cache.generateKey(["query", "SELECT 1"], "svc"); + const b = mock.cache.generateKey(["query", "SELECT 1"], "svc"); + const perUser = mock.cache.generateKey(["query", "SELECT 1"], "other"); + + expect(a).toBe(b); + expect(perUser).not.toBe(a); + }); +});