refactor(appkit): make CacheManager per-app and delete the process-wide singleton - #566
Open
IamGalymzhan wants to merge 36 commits into
Open
refactor(appkit): make CacheManager per-app and delete the process-wide singleton#566IamGalymzhan wants to merge 36 commits into
IamGalymzhan wants to merge 36 commits into
Conversation
`_createApp` builds one manager per app and passes it through the app's `PluginContext`, so a second app in one process no longer inherits the first's cache and silently loses its own `cache` config. Construction moves behind two statics — the existing async `create()` for the app, and a new synchronous `@internal forStorage()` for the testing kit, which cannot await. The constructor stays `private`, so a consumer has no way to build a manager at all; that is what makes "exactly one manager per app" a compiler-checked property rather than a convention. `PluginContext.cache` is `readonly` for the same reason. The manager reaches the context through a dedicated `AppKit` constructor parameter, never the config bag and never `extraData`: both are spread into every plugin's `baseConfig`, and `_buildExecutionConfig` deep-merges a plugin's config into its execute options — so a manager arriving that way would be merged into what `PluginExecuteConfig.cache` declares as a `CacheConfig` and silently break the cache interceptor's gate. A test pins that, rather than leaving it to reasoning. Passing the already-built `PluginContext` also retires the one-key `mergedConfig` wrapper. `getInstanceSync()` is untouched and still answers: the boot publishes its manager into the deprecated ambient slot, first-wins, exactly as before. Plugins keep reading it until they are rebound from the context, which keeps this change releasable on its own. Two suites that mock the cache module gain `create`/`_publishAmbient` so their fakes match the module's shape; both are migrated off the mock entirely later in this stack. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`attachContext` now takes the cache from the plugin's context, so every plugin in an app shares the one manager that app built. `Plugin.cache` becomes a read-only accessor over a private field: reads are unchanged, and a subclass assignment is rejected by the compiler *and* throws at runtime, so not even a JavaScript consumer can swap an app's cache. A plugin wanting different behaviour sets a per-plugin `cache` config. Telemetry is no longer gated behind the cache. The constructor used to bind the cache first and return early if none existed, leaving `this.telemetry` unset — so a plugin built before any app failed inside the telemetry interceptor, far from the cause. `getProvider` never throws, so it now binds unconditionally. A plugin that never got `attachContext` has no cache, and `cache`'s declared type is non-optional, so the compiler cannot catch it. Every cached execution passes through `_buildInterceptors`, which now reports an `InitializationError` naming the plugin instead of letting a `TypeError` on `undefined.getOrExecute` surface inside a request handler. Chain construction moved inside `execute`'s try block so that failure is returned as a result: the method's contract is that it never throws. The constructor keeps binding the deprecated ambient slot when one exists, unchanged, so an app-less plugin behaves exactly as before and this phase stays releasable on its own. That bind goes away with the slot itself. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`_createApp` builds a manager and, until now, dropped it if any later boot step threw — `ServiceContext.initialize`, resource validation, plugin `setup()`, `onPluginsReady`, or the server's `listen`. Nothing else held a reference afterwards, so the manager was unreachable, and one that resolved to Lakebase owned a `pg.Pool` that was never ended: a leaked pool per failed boot, holding the event loop open. The singleton masked this, because the next boot reused the published manager. Everything past the manager's construction now runs guarded, and the boot error is never masked by a teardown failure. `LifecycleManager` takes the manager as a constructor dependency instead of looking it up during shutdown. A lookup resolves whatever occupies the process-wide slot at that moment, which is not necessarily the app being shut down once more than one app can exist. Its test suite drops the cache-module mock entirely and passes a double, which is what the injection makes possible. One test went with it: "a never-initialized cache is skipped without error" has no subject anymore, since the manager is a required dependency rather than a lookup that can fail. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`close()` closed its storage unconditionally, so an app closing destroyed
storage its caller owned. Nothing exercised that path before: `getInstance`
was first-wins and silently discarded a second caller's `cache: { storage }`,
so the option only starts working now that each app builds its own manager.
Ownership is recorded per construction site rather than derived from
"was `config.storage` supplied?" — of the seven sites, two hand back the
caller's storage and five build their own, and two of those five build a
fresh `InMemoryStorage` *inside* the supplied-storage branch after the
caller's storage fails its health check. A supplied-vs-not flag would get
those two wrong and skip closing storage the manager owns.
The consequence differs by backend, which is why this stayed invisible:
`InMemoryStorage.close()` clears a `Map` and stays usable, while
`PersistentStorage.close()` is `pool.end()` and permanent.
Verified by mutation: removing the gate fails three of the six new tests,
and the three that still pass are the owned-storage cases that should
close regardless.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`TelemetryExamples` assigned `this.cache = new CacheManager({enabled, ttl},
this.telemetry)` — two arguments against a private `(storage, config)`
constructor, so the config landed where storage belongs. At runtime
`config.enabled` was `undefined`, `getOrExecute` short-circuited, and the
route returned correct answers while caching nothing. The assignment was
redundant as well as broken: the call site already passes `{ ttl: 60 }`,
so deleting it lets the route use the app's own cache, which is enabled by
default.
The reason this survived is that nothing type-checked the file. The app had
no `typecheck` script, and its `check: tsc` script has never run clean —
the root tsconfig had no `include`, so it pulled the React client in with
the server's config and produced a wall of JSX and DOM errors that would
have buried any real one.
So scope the config to the server, where the client has its own, and add
the `typecheck` script. That surfaced four genuine pre-existing
possibly-undefined errors in `server/index.ts`, fixed here since this
commit is what turns the gate on. Two further errors were artifacts of the
exports map sending `tsc` to a stale `dist`; `customConditions:
["development"]` resolves the same source the dev runtime uses.
Verified by mutation: reintroducing the original line now fails the gate
twice over — read-only `cache` and the private constructor.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The kit's context now carries a real in-memory `CacheManager` and exposes it on the handle as `cache`. A plugin attached through `attach()` resolves that same object, so a test can spy or read it — `vi.spyOn(mock.cache, "getOrExecute")`, `generateKey`, `get` — and assert real caching against production's own keying instead of a re-implemented fake. `attach()` no longer seeds the process-wide slot to make a cache appear. Each context builds its own, so two contexts in one file cannot see each other's entries and attaching leaves nothing behind for a later test to observe. Built through the synchronous `forStorage`, because `createTestPluginContext` is called at describe-body time and cannot await; in-memory storage has no health check to wait for. The identity between `mock.cache` and the plugin's `this.cache` is the load-bearing part: were they different objects, every spy would record nothing while the suite stayed green. Verified by mutation — detaching the two fails exactly the two tests that assert it. `analytics.test.ts` gains `forStorage` on its cache-module fake so the fake still matches the module's shape; it is migrated off that mock later in this stack. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`resetTestCache()` read `CacheManager.getInstanceSync()`, so it cleared whatever occupied the process-wide slot — which is no longer the cache a test is using. It now clears the caches this kit built for the current file, tracked in a module-level set inside the kit rather than on `CacheManager`: this is test-only bookkeeping, and Vitest isolates test files in separate workers, so the set is per-file by construction. A set rather than a single most-recent slot, because one file can hold several test contexts and the zero-argument form is documented to work mid-test, where "the newest one" would clear the wrong cache. Pass a context (`resetTestCache(mock)`) or a manager (`resetTestCache(mock.cache)`) to clear just one. The published zero-argument call keeps compiling, and clearing nothing is still not an error. Its own tests move with it: the old pair forced the uninitialized branch by making `getInstanceSync` throw, which no longer describes anything. Verified by mutation — stubbing the clear out fails four of the five. The testing guide's "the cache is a process-wide singleton" passage is replaced by what is now true: each context owns its cache, `mock.cache` is the object the plugin resolves, and it is the seam for asserting real caching against production's own keying. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`docs:build` derives this from JSDoc. `Plugin.cache` moved from a mutable protected property to a read-only accessor, so the reference documented a property that no longer exists. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Thirteen suites replaced the internal `cache` module with a fake whose `getOrExecute` just called `fn()`. The fake existed for one reason: the `Plugin` constructor used to throw when no cache had been initialized, so constructing a plugin at all required one. That is no longer true — the constructor binds a cache when one exists and carries on when it does not — and none of these suites assert anything about caching, so the mock has no remaining job. Two of them never had one: the lakebase `pool-manager` and `routing-pool` tests mock a module the code under test does not touch. Eight also left a `mockCacheInstance` fake behind in a separate `vi.hoisted` block. Removed with the mocks: a dead fake reads like the cache is still faked, and the suites now exercise the real one. Suites that genuinely assert caching — files invalidation, jobs read paths, and the store-backed analytics and ai-search fakes — are not in this commit; they need a real cache wired in rather than a mock removed. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The files invalidation tests and the jobs read paths genuinely exercise caching, so removing their mock is not enough — they need a cache. Each file now builds one kit context and binds plugins to it through `attachContext`, the same call `createApp` makes. `files/_test-helpers` grows `filesPlugin()` and `testCache` so its four suites share one seam; jobs keeps a local helper. The keys these tests assert are now production's own `generateKey` output rather than a re-implemented fake's, which is the point: three separate hand-rolled key functions had drifted from each other and from production. Two consequences the fake was hiding, both fixed here rather than worked around: Entries outlive a test once the cache is real, so a cached run answered the next test's read — `resetTestCache()` in the jobs `beforeEach` clears between tests, exactly as the testing guide now documents. Two jobs error tests threw a plain `Error` carrying a `statusCode` property. The passthrough fake ran the work with no error handling, so the look-alike's status leaked through; the real `getOrExecute` preserves status only from `ApiError`/`AppKitError` and wraps anything else to 500. The SDK throws genuine `ApiError`s, so the tests now do too. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
`metric.test.ts` carried a store-backed fake that re-implemented `generateKey` — sha256 over `JSON.stringify([userKey, ...parts])`. It matched production, but as a private copy it could drift silently while the tests kept passing, and two sibling suites hold two more copies that already disagree with each other. The suite now binds its plugins to a kit context and asserts against that cache. The invariant it exists to protect — injecting metric-views metadata must not change the composed cache key — is checked with production's own keying, so a change to `generateKey`'s shape would surface here instead of sailing past. `mockCacheStore.clear()` becomes `resetTestCache()`, the published call for the same job. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Forty-nine of the suite's fifty-two `getInstance` calls only wanted a manager over a given storage; they now call the `create` factory. That also retires the wrinkle they were written around — `getInstance` returns any existing instance and silently ignores the storage argument, so each test had to reset the private statics by reflection first to get the storage it asked for. The three tests in the `singleton pattern` block are left alone: their subject *is* the statics, so they are removed with them rather than migrated. The reflection reset stays for the same reason, now with a comment saying so. The suite still bites: stubbing the cache read path to never serve a hit fails its hit and per-user-key tests. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Seven agents suites reached for the process-wide CacheManager: five monkeypatched `getInstanceSync`/`instance` with a hand-rolled fake, two initialized the real singleton so `attachContext` had something to bind. A probe that throws on every cached execution proves no test in this directory reaches the cache path, so the five fakes were dead weight and are simply gone. The two that attach a context now take the cache from `createTestPluginContext` instead: discovery gets the kit's real context (its provider registry is empty, so tool collection finds nothing, as it did with no context at all), and the plugin suite's own fake context carries `kit.cache`. Nothing in the directory touches CacheManager's statics now. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
plugin.test.ts and asUser-proxy.test.ts both mocked the cache module so the constructor's ambient bind would hand the plugin a fake. Neither needs the process-wide slot. plugin.test.ts genuinely exercises cached executions, so TestPlugin now attaches a context carrying the double — the same path an app uses. The assertion that the constructor called getInstanceSync is replaced by one that the plugin binds the cache its context carried, which is the behaviour worth pinning; mutating attachContext to ignore the context fails it along with every execute test. asUser-proxy.test.ts never reaches a cached execution (verified by a probe that throws on the cache path), so its fake is deleted outright. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
analytics.test.ts hand-rolled a store-backed cache double whose generateKey was a copy of production's, free to drift from it, and mocked the cache module so the ambient bind would hand it over. It now attaches the kit's context, like metric.test.ts already does. The real getOrExecute passes the callback a composed signal, which exposed that the abort test only ever worked because the fake called fn() with no argument — that dropped the shared signal and left the route's own signal in its place, which is the one the fallback checks. The test now fires the close listener the route registered, aborting that signal the way a client disconnect does. Without the abort the statement really does run twice, so the assertion still bites. That route no longer reaches deliverArrowBytes' abort guard, whose only coverage it was, so result-delivery.test.ts pins it directly instead — at the level it lives at, and it fails when the guard is removed. The sibling guards on the EXTERNAL_LINKS and JSON paths were already uncovered before this change. appkit-as-user-exports.test.ts drops its cache mock outright and boots on a real per-app cache. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The fake claimed to key "like the real CacheManager.generateKey, so tests exercise real key composition", but keyed on JSON.stringify([userKey, ...parts]) where production hashes — so the seven caching tests were pinned to a double that could never agree with the real thing. They now run on the kit's cache. The real CacheManager instruments getOrExecute through the same telemetry provider, which this file mocks, so the span stub had to become a whole span rather than the three members the plugin alone touched. Dropping executorKey from _cacheKeyFor fails the per-user isolation test, so the real keying is load-bearing. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The last suite reaching for the cache module. Its double passed getOrExecute straight through and returned undefined from generateKey, so no test here ever saw a cache hit or a real key. The suite now attaches the kit's context, matching the seam the nine sibling files suites already share, and its five cache assertions spy on the real manager. That the fake never cached was hiding coverage, not just fidelity: with the read cache wrongly enabled on OBO volumes, the pass-through double failed two tests, while the real cache fails three — the cross-user freshness test could not have caught it before, because nothing was ever cached to go stale. afterEach unpatches only the cache's own methods. vi.restoreAllMocks() is too broad here: it also strips the implementations from the module-scope SDK doubles, which fails three policy tests. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Nothing in plugin.test.ts reads the statics any more, so the module mock only existed to keep the constructor's ambient bind from throwing — which it already swallows. No test outside cache/ now mocks the cache module. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Deletes `instance`, `initPromise`, `getInstance`, `getInstanceSync`, and the `_publishAmbient` writer the last of those needed. The class stays exported — it is still the type of `Plugin.cache` — and with the constructor private and only `create`/`forStorage` reachable, an app's manager has no public construction path. A plugin's cache now comes only from its context. `attachContext` throws a named InitializationError when a context is supplied that carries no cache, and a plugin that never got one throws at its first cached execution rather than on `undefined` inside a handler. Docs carry the 0.70.0 upgrade section: the removed statics and their replacement, the unattached-plugin error, the read-only `this.cache`, and the supported paths in production and in tests. The note lives in docs/docs/plugins/caching.md — CHANGELOG.md sections are generated into place by tools/finalize-release.ts, so a hand-added note there would pin itself above every later release. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
resetTestCache's docstring still described clearing "AppKit's process-wide cache singleton" and "the cache attach() seeds" — both gone. Its inline comments were already accurate; only the doc comment lagged, and it ships as part of the testing entry point. Same for one comment in the kit that referred to the deleted slot. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Contributor
📦 Bundle size reportCompared against
|
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 1.1 MB (+3.3 KB) | 378 KB (+1.5 KB) |
| Type declarations | 388 KB (+2.7 KB) | 137 KB (+1.1 KB) |
| Source maps | 2.1 MB (+7.6 KB) | 708 KB (+3.0 KB) |
| Other | 11 KB | 3.7 KB |
| Total | 3.5 MB (+14 KB) | 1.2 MB (+5.6 KB) |
Per-entry composition (own code — deps external (as shipped))
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
. |
95 KB (+171 B) | 2.5 KB | 98 KB (+171 B) | external | 311 KB (+390 B) |
./beta |
72 KB (-3.2 KB) | 457 B | 72 KB (-3.2 KB) | external | 215 KB (-11 KB) |
./testing |
17 KB (+45 B) | 0 B | 17 KB (+45 B) | external | 50 KB (-35 B) |
./tsdown |
520 B | 0 B | 520 B | external | 813 B |
./type-generator |
22 KB | 0 B | 22 KB | external | 65 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
. |
index.js |
initial | 91 KB |
. |
utils.js |
initial | 4.0 KB |
. |
remote-tunnel-manager.js |
lazy | 2.5 KB |
./beta |
beta.js |
initial | 56 KB |
./beta |
stream-manager.js |
initial | 5.8 KB |
./beta |
wide-event-emitter.js |
initial | 3.2 KB |
./beta |
databricks.js |
initial | 3.2 KB |
./beta |
configuration.js |
initial | 2.1 KB |
./beta |
service-context.js |
initial | 1.3 KB |
./beta |
client.js |
initial | 434 B |
./beta |
client-options.js |
initial | 219 B |
./beta |
supervisor-api.js |
lazy | 193 B |
./beta |
databricks.js |
lazy | 142 B |
./beta |
index.js |
lazy | 122 B |
./testing |
index.js |
initial | 17 KB |
./tsdown |
index.js |
initial | 520 B |
./type-generator |
index.js |
initial | 22 KB |
@databricks/appkit-ui
npm tarball (packed): 350 KB — gzipped download (dist + bin; excludes release-only docs/NOTICE).
| dist | raw | gzip |
|---|---|---|
| JS (runtime) | 395 KB | 132 KB |
| Type declarations | 229 KB | 84 KB |
| Source maps | 766 KB | 253 KB |
| CSS | 16 KB | 3.2 KB |
| Total | 1.4 MB | 473 KB |
Per-entry composition (consumer bundle — deps bundled, peerDeps external)
| Entry | Initial (gz) | Lazy (gz) | Total (gz) | node_modules (min) | Own code (min) |
|---|---|---|---|---|---|
./js |
5.3 KB | 49 KB | 55 KB | 208 KB | 14 KB |
./js/beta |
20 B | 0 B | 20 B | 0 B | 0 B |
./react |
432 KB | 49 KB | 481 KB | 1.3 MB | 177 KB |
./react/beta |
1.0 KB | 0 B | 1.0 KB | 0 B | 1.9 KB |
Chunks:
| Entry | Chunk | Load | Size (gz) |
|---|---|---|---|
./js |
index.js |
initial | 5.2 KB |
./js |
chunk |
initial | 120 B |
./js |
apache-arrow |
lazy | 49 KB |
./js/beta |
beta.js |
initial | 20 B |
./react |
index.js |
initial | 430 KB |
./react |
tslib |
initial | 2.1 KB |
./react |
apache-arrow |
lazy | 49 KB |
./react/beta |
beta.js |
initial | 1.0 KB |
Contributor
🤖 AppKit PR bot🔬 Run evalsStart an eval for this PR from the evals-monitor app: Go to Evals Monitor → 📦 Try this PR's app templateScaffolds a new app from this PR's SDK build. Run it in any folder (requires the GitHub CLI — gh run download 33642625418 -R databricks/appkit -n appkit-template-0.70.0-pr.b639b12-refactor-cache-manager-per-app-566 -D appkit-pr-566 \
&& unzip -o "appkit-pr-566/appkit-template-0.70.0-pr.b639b12-refactor-cache-manager-per-app-566.zip" -d "appkit-pr-566" \
&& databricks apps init --template "appkit-pr-566"The template pins |
CacheManager.create's Lakebase branch only called pool.end() on the !isHealthy path. If healthCheck() passed but initialize() then threw, the bare catch swallowed the error and fell through to in-memory — leaking the pg.Pool for the life of the process while boot still 'succeeded'. The PR's failed-boot close could not cover it: that branch returns a usable manager, so nothing signals failure. Now the healthy block ends the pool on any throw before falling back. Also drops the 18 dead (CacheManager as any).instance/initPromise reset lines this suite carried — the statics they reset were deleted earlier in this branch, and the file's own comment predicted 'This reset goes when those statics do.' Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
databricks.test.ts mocked '@databricks-apps/cache' — a specifier that resolves nowhere in this repo (the real module is ../../cache) — for getInstance/getInstanceSync, statics that no longer exist. The mock has been inert for a while, so it is pre-existing rather than caused here, but it is the mock that made a147625's 'the last cache-module mock' claim untrue. Its createApp() calls already hit the real cache; removing it changes nothing. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The cache getter returned this._cache as CacheManager, casting away undefined, so the only runtime guard was in _buildInterceptors — which covers executions routed through execute()/executeStream() but not the two production sites that read this.cache directly: analytics.ts's arrow caching executor and files/plugin.ts's list-cache invalidation. On an unattached plugin (the app-less runAgent path) both raised a bare TypeError deep in a handler; the files site's debug-level catch made it silent. The getter now throws a named InitializationError at the read. Removes the lying cast, keeps every call site unchanged, and narrows the _buildInterceptors comment that overstated its reach. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
run-agent.ts calls attachContext({}) on every standalone plugin, but every
double in this suite is duck-typed (implements ToolProvider without extending
Plugin), so Plugin.attachContext never ran under test. The two-case
attach contract — a context-less attach binds telemetry and leaves the cache
unbound without throwing, while a supplied cache-less context throws — had no
end-to-end coverage on the production call site.
Adds a real Plugin subclass routed through runAgent, asserting attachContext
and setup both ran (ready), and the cache is left unbound (a direct read
fails closed). Mutating attachContext to throw whenever no cache is reachable
fails this test.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…in factories
attach() was async only because it once awaited CacheManager.getInstance();
that await is gone, so its body has zero awaits. Because it returned a
promise, seven suites bypassed it and hand-rolled
plugin.attachContext({ context: kit.ctx }) to get a synchronous bind —
which also skipped the registerPlugin/registerToolProvider that attach()
does, so they bound less like production than the kit offers.
attach() is now synchronous and returns P. The seven factories route through
kit.attach(new X(...)), gaining registry parity as a side effect. Two suites
that duplicated the kit+factory block verbatim (analytics.test.ts,
metric.test.ts) now share a new analytics/tests/_test-helpers.ts, and
files/plugin.test.ts imports the three symbols its own _test-helpers.ts
already exported instead of re-declaring them.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
InMemoryStorage's constructor demanded a CacheConfig but reads only maxSize,
so this branch had accumulated 'new InMemoryStorage({} as never)' casts at
ten call sites. Narrowing the parameter to Pick<CacheConfig, "maxSize"> with
a default says what it uses and lets callers write new InMemoryStorage() —
every cast is gone. Behaviour-identical: the body already defaulted a missing
maxSize, and the internal create() calls pass whole-config variables that
remain assignable.
Also drops the now-needless '{ cache } as never' cast in plugin.test.ts
(attachContext's context param is typed unknown).
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
… slot
Several comments this branch added or left still told the reader a
process-wide cache slot exists. plugin-context.ts:79 was outright false — it
promised a plugin 'falls back to the deprecated process-wide slot' where
attachContext actually throws. The rest were change-narration ('no longer',
'Previously') or defended a parameter against a lookup that no longer exists
(lifecycle-manager, appkit.ts's context @PARAM, two cache-injection test
comments, the storage-ownership historical note). Reworded to the durable
claim in each case; the legitimate historical contrast at
appkit-cache-injection.test.ts:120 is kept. Also trims kit-cache.ts's
14-line note on its Set to the load-bearing four.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The caching guide claimed an app's cache is unreachable outside a plugin because 'the manager has no public constructor.' The private-constructor half is true, but CacheManager.create()/forStorage() are @internal in JSDoc only — stripInternal is not set, so both ship in dist/cache/index.d.ts on a value-exported class, and a consumer can call CacheManager.create(). Reworded to the guarantee that actually holds: the cache is handed only to registered plugins and there is no process-wide accessor. Also resolves a self-contradiction — the upgrade note listed reading the cache in setup() as a way to hit the unattached error, while the section above says to read it from setup(). Clarified that under createApp, setup() and handlers run after the bind, so only the constructor or an unregistered hand-built plugin is affected. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Follow-up to the pool-leak fix: the outer catch that falls back to in-memory is silent, which is right for an unreachable Lakebase (the common, expected case) but hides the surprising one — a connection that passed its health check and then failed to initialize. Warn in the inner catch, where the case is unambiguous, before ending the pool and re-throwing into the fallback. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Routing the test factories through kit.attach() (the previous commit) re-registered the tool provider on every call. A file that builds many instances of one plugin — the shared-kit factory pattern — tripped the production "registered more than once" warning ~166 times across the suites, training readers to ignore a diagnostic that exists to catch two plugins claiming one tool namespace, and silently churning the plugin registry. attach() now registers a given name once (guarding on ctx.hasPlugin), so re-attaching another instance still binds it via attachContext but leaves the registry and the warning alone. The production warning is untouched. Also drops the now-needless 'enabled' excess-property cast on EndableStorage, which extends InMemoryStorage and inherits its narrowed constructor. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The generated reference for Plugin.cache lagged the getter's new JSDoc (the fail-closed accessor). Regenerated via docs:build; no hand edits. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
…e tests A second simplification pass flagged the branch's added comments as carrying too much change-narration — 'used to', 'previously', 'the old symptom' — which ages badly and restates what the test name or assertion already says. Cut those in cache-binding.test.ts and appkit-cache-injection.test.ts (keeping the two genuinely load-bearing ones: why cache is read in-class, and the boot-ordering dependency), rephrased the one disputed historical line as the invariant it guards, trimmed the ownsStorage/forStorage JSDoc to the non-obvious hazard, and dropped kit-cache.ts's redundant wrapper comments. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
kit-cache.ts existed only to hold a Set shared between createTestPluginContext (which records each per-file cache) and resetTestCache (which clears them). Both the Set and its trackedKitCaches() reader now live in fixtures.ts next to resetTestCache, its only reader; test-plugin-context.ts imports registerKitCache from fixtures.ts, which it already imports from. One @internal cross-file seam remains (registerKitCache) instead of a standalone file with two, and the kit-cache.js that shipped as unreachable dead code in the tarball is gone. No behaviour change: same per-file registry, same resetTestCache semantics. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
The guard added in the previous fix registers a plugin name once, but the comment still claimed registration makes getPlugins()/hasPlugin()/sibling lookups 'behave as in production' — false for every instance after the first. State the actual contract: first attach of a name wins, later instances of the same name are bound but not re-registered, so a sibling lookup resolves the first instance. Names why that is safe here (direct attach sites use a fresh context per test; no factory suite does a dependent sibling lookup). Comment only — no behaviour change. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Every PluginContext that exists carries a cache: the two production
construction sites both supply one (createApp and the testing kit), and
PluginContext is not exported to consumers, so there is no path that builds a
cache-less one. The app-less case is context-less (attachContext({})), not a
cache-less context. So the optionality guarded a state that cannot occur.
Making the field required turns 'one cache per context' into a
compiler-checked invariant and makes attachContext's runtime 'supplied context
with no cache' guard unreachable — deleted. The remaining guards stay and are
distinct: the Plugin.cache getter (direct readers) and _buildInterceptors (the
execute path), both for the genuinely cache-less context-less plugin. Five
test sites that built a bare PluginContext now pass a cache (a double where the
cache is not exercised); the old runtime-throw test becomes a @ts-expect-error
that fails if the field is made optional again.
Reverses the earlier deliberate 'keep it optional' call, on the design
review's argument — the objection (deviation from R4, error-timing) did not
hold: required is a stronger, compile-time form of R4's fail-loud intent, and
no non-test caller could ever hit the timing difference.
Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
Resolves the one content conflict in plugin/tests/plugin.test.ts: keeps both this branch's 'binds the cache its context carries' test and #568's two streamConfig tests. plugin.ts auto-merged cleanly — new StreamManager( config.streamConfig) from #568 sits alongside the per-app cache/telemetry changes. Regenerated API docs pick up #568's streamConfig on the configs that extend BasePluginConfig; caching.md's upgrade heading moves to 0.71.0 since 0.70.0 already shipped. Signed-off-by: Galymzhan <zhangazy2004@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CacheManagerwas a process-wide singleton reached throughgetInstance()/getInstanceSync(). It's now owned by the app:createAppbuilds one manager and injects it throughPluginContext, so a plugin'sthis.cacheis its own app's cache. Two apps in one process hold two independent managers, each honouring its owncacheconfig.The statics are deleted.
CacheManagerstays exported (it's the type ofPlugin.cache), but with a private constructor andPluginContext.cacherequired, there is no process-wide accessor and no way for a plugin to get a cache other than its app's.This unblocks #540 — the singleton was why test suites had to mock the cache module to keep one test's cached value out of the next.
What changes for a consumer
Most apps need nothing — if your plugins read
this.cacheand you build withcreateApp, this is a no-op.CacheManager.getInstance()/getInstanceSync()are gone. Inside a plugin usethis.cache; there's no process-wide handle to fetch.this.cacheis read-only. A plugin that assigned its own manager no longer compiles — set a per-plugincache: { enabled, ttl }config instead.this.cachebefore the app binds it (a plugin built by hand in a test, or in a constructor) throws a namedInitializationErrorinstead of picking up whatever manager happened to exist.Upgrade notes are in
docs/plugins/caching.md.Design
One manager per app is compiler-enforced:
@internalfactories (createasync for the app,forStoragesync for tests) — no public construction path.PluginContext.cacheis required. Every context carries its app's cache; a cache-less one doesn't typecheck (pinned by a@ts-expect-errortest).Plugin.cacheis a read-only getter that throws when read on an unattached plugin — the guard for the direct readers (analytics,files) that bypassexecute().runAgentcallsattachContext({}); it binds telemetry and leaves the cache unbound, so only a cached execution fails — at the getter or_buildInterceptors.close()closes only manager-built storage (ownsStorage). A caller who passescache: { storage }keeps ownership, sincePersistentStorage.close()is a permanentpool.end().Fixes surfaced along the way
pg.Pool. Now closed on the failure path.create()only ended the pool when the health check failed — a healthy connection whoseinitialize()threw leaked the pool while boot still "succeeded." Now ended on any throw.getOrExecute, so they could never observe a hit; the files suite's fake couldn't have caught a cross-user staleness regression. They now run on a real cache.Test migration
26 suites moved off the cache-module mock onto a real cache from the testing kit. Each migration was verified by mutation — deleting the behaviour under test and confirming the assertion fails — so a detached cache handle can't leave a suite green but asserting nothing.