From a8b7010f310f9aef81e19a0db70eb9ae692128b4 Mon Sep 17 00:00:00 2001 From: ditadi Date: Tue, 12 May 2026 16:15:37 +0100 Subject: [PATCH] refactor(appkit): introduce ServiceManager for core service lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppKit's core previously ran a hardcoded boot sequence for cache + telemetry. This refactor introduces a generic service container so the core no longer names concrete services in its bootstrap path: - `ServiceManager` — container that holds booted core services, resolves them by name via `get(name)`, and stops them in reverse start order. `add(name, null)` is a no-op so callers can pass opt-out results directly. - `startCoreServices(config)` — builds the ServiceManager AppKit ships with; lives next to the concrete service modules so `core/appkit.ts` stays free of concrete-class imports. Cache and Telemetry expose a `static boot(config)` returning `{ instance, stop } | null`. Telemetry returns `null` when no OTLP endpoint is configured (preserves the existing opt-out semantics). Plugins receive a typed `services` locator through `attachContext`; the shape is inlined in `BasePlugin` so the shared package does not gain a new exported interface. New plugins resolve services via `services.get("my-service")` when the base class doesn't surface them as properties. Groundwork only — no behaviour change, no public-API removal. The goal is to make room for a third core service (TaskFlow) without re-threading the cross-package `attachContext` contract: adding a new service touches only the service module and `startCoreServices`. TaskFlow itself is intentionally NOT in this PR. Verified: pnpm -r typecheck, pnpm build, full vitest run (123 files, 2261 tests) all green. Signed-off-by: ditadi --- packages/appkit/src/cache/index.ts | 16 ++++ packages/appkit/src/core/appkit.ts | 96 ++++++++++--------- packages/appkit/src/core/service-manager.ts | 68 +++++++++++++ .../src/core/tests/service-manager.test.ts | 74 ++++++++++++++ packages/appkit/src/plugin/plugin.ts | 30 +++--- .../appkit/src/telemetry/telemetry-manager.ts | 31 ++++-- .../telemetry/tests/telemetry-manager.test.ts | 2 - packages/shared/src/plugin.ts | 6 +- 8 files changed, 241 insertions(+), 82 deletions(-) create mode 100644 packages/appkit/src/core/service-manager.ts create mode 100644 packages/appkit/src/core/tests/service-manager.test.ts diff --git a/packages/appkit/src/cache/index.ts b/packages/appkit/src/cache/index.ts index 37bd1659e..c92dc7b52 100644 --- a/packages/appkit/src/cache/index.ts +++ b/packages/appkit/src/cache/index.ts @@ -110,6 +110,22 @@ export class CacheManager { return CacheManager.initPromise; } + /** @internal */ + static async boot( + config?: CacheConfig, + ): Promise<{ instance: CacheManager; stop(): Promise }> { + const mgr = await CacheManager.getInstance(config); + return { instance: mgr, stop: () => mgr.shutdown() }; + } + + /** @internal */ + async shutdown(): Promise { + await this.close(); + this.inFlightRequests.clear(); + CacheManager.instance = null; + CacheManager.initPromise = null; + } + /** * Create a new cache manager instance * diff --git a/packages/appkit/src/core/appkit.ts b/packages/appkit/src/core/appkit.ts index 3421e03bb..28fa56059 100644 --- a/packages/appkit/src/core/appkit.ts +++ b/packages/appkit/src/core/appkit.ts @@ -9,29 +9,27 @@ import type { PluginMap, } from "shared"; import { version as productVersion } from "../../package.json"; -import { CacheManager } from "../cache"; import { ServiceContext } from "../context"; import { isInternalTelemetryEnabled, TelemetryReporter, } from "../internal-telemetry"; -import { createLogger } from "../logging/logger"; import { ResourceRegistry, ResourceType } from "../registry"; import type { TelemetryConfig } from "../telemetry"; -import { TelemetryManager } from "../telemetry"; import { isToolProvider, PluginContext } from "./plugin-context"; - -const logger = createLogger("appkit"); +import { type ServiceManager, startCoreServices } from "./service-manager"; export class AppKit { #pluginInstances: Record = {}; #setupPromises: Promise[] = []; #context: PluginContext; + #services: ServiceManager; - private constructor(config: { plugins: TPlugins }) { + private constructor(config: { plugins: TPlugins }, services: ServiceManager) { const { plugins, ...globalConfig } = config; this.#context = new PluginContext(); + this.#services = services; const pluginEntries = Object.entries(plugins); @@ -89,6 +87,7 @@ export class AppKit { if (typeof pluginInstance.attachContext === "function") { pluginInstance.attachContext({ context: this.#context, + services: this.#services, telemetryConfig: baseConfig.telemetry, }); } @@ -199,56 +198,59 @@ export class AppKit { disableInternalTelemetry?: boolean; } = {}, ): Promise> { - // Initialize core services - TelemetryManager.initialize(config?.telemetry); - await CacheManager.getInstance(config?.cache); - - const rawPlugins = config.plugins as T; - - // 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, - ); + const services = await startCoreServices({ + telemetry: config?.telemetry, + cache: config?.cache, + }); - // Validate env vars - registry.enforceValidation(); + try { + const rawPlugins = config.plugins as T; - const preparedPlugins = AppKit.preparePlugins(rawPlugins); - const mergedConfig = { - plugins: preparedPlugins, - }; + const resourceRegistry = new ResourceRegistry(); + resourceRegistry.collectResources(rawPlugins); - const instance = new AppKit(mergedConfig); + const needsWarehouse = resourceRegistry + .getRequired() + .some((r) => r.type === ResourceType.SQL_WAREHOUSE); + await ServiceContext.initialize( + { warehouseId: needsWarehouse }, + config?.client, + ); - await Promise.all(instance.#setupPromises); - await instance.#context.emitLifecycle("setup:complete"); + resourceRegistry.enforceValidation(); - const handle = instance as unknown as PluginMap; + const preparedPlugins = AppKit.preparePlugins(rawPlugins); + const mergedConfig = { + plugins: preparedPlugins, + }; - if (config.onPluginsReady) { - logger.debug("Running onPluginsReady hook"); - await config.onPluginsReady(handle); - logger.debug("onPluginsReady hook completed"); - } + const instance = new AppKit(mergedConfig, services); - if (isInternalTelemetryEnabled(config)) { - AppKit.bootstrapInternalTelemetry(); - } + await Promise.all(instance.#setupPromises); + await instance.#context.emitLifecycle("setup:complete"); - const serverPlugin = instance.#pluginInstances.server; - if (serverPlugin && typeof (serverPlugin as any).start === "function") { - await (serverPlugin as any).start(); - } + const handle = instance as unknown as PluginMap; - return handle; + if (config.onPluginsReady) { + await config.onPluginsReady(handle); + } + + if (isInternalTelemetryEnabled(config)) { + AppKit.bootstrapInternalTelemetry(); + } + + const serverPlugin = instance.#pluginInstances.server; + if (serverPlugin && typeof (serverPlugin as any).start === "function") { + await (serverPlugin as any).start({ + shutdownCoreServices: () => services.stop(), + }); + } + + return handle; + } catch (error) { + await services.stop(); + throw error; + } } private static bootstrapInternalTelemetry(): void { diff --git a/packages/appkit/src/core/service-manager.ts b/packages/appkit/src/core/service-manager.ts new file mode 100644 index 000000000..05b6a0786 --- /dev/null +++ b/packages/appkit/src/core/service-manager.ts @@ -0,0 +1,68 @@ +import type { CacheConfig } from "shared"; +import { CacheManager } from "../cache"; +import { createLogger } from "../logging"; +import { type TelemetryConfig, TelemetryManager } from "../telemetry"; + +const logger = createLogger("services"); + +interface ServiceEntry { + readonly name: string; + readonly instance: unknown; + stop(): Promise; +} + +/** Holds booted core services and resolves them by name. */ +export class ServiceManager { + #services: ServiceEntry[] = []; + + /** Adds a service. `null` is ignored so callers can pass an opt-out result. */ + add( + name: string, + service: { instance: unknown; stop(): Promise } | null, + ): void { + if (!service) return; + this.#services.push({ name, ...service }); + logger.debug("Started: %s", name); + } + + get(name: string): T | null { + for (const s of this.#services) { + if (s.name === name) return s.instance as T; + } + return null; + } + + /** Stops services in reverse start order. Per-service failures are logged. */ + async stop(): Promise { + while (this.#services.length > 0) { + const s = this.#services.pop(); + if (!s) continue; + try { + await s.stop(); + logger.debug("Stopped: %s", s.name); + } catch (error) { + logger.error("Stop failed for %s: %O", s.name, error); + } + } + } +} + +/** + * Boots the core services AppKit ships with. Adding a new service touches + * only this function and the service module itself — the rest of the core + * stays free of concrete service imports. + */ +export async function startCoreServices(config: { + telemetry?: TelemetryConfig; + cache?: CacheConfig; +}): Promise { + const services = new ServiceManager(); + try { + services.add("telemetry", await TelemetryManager.boot(config.telemetry)); + services.add("cache", await CacheManager.boot(config.cache)); + return services; + } catch (error) { + await services.stop(); + throw error; + } +} diff --git a/packages/appkit/src/core/tests/service-manager.test.ts b/packages/appkit/src/core/tests/service-manager.test.ts new file mode 100644 index 000000000..926d6b1f6 --- /dev/null +++ b/packages/appkit/src/core/tests/service-manager.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, test, vi } from "vitest"; +import { ServiceManager } from "../service-manager"; + +function mockService(name: string, calls: string[]) { + return { + instance: { name }, + stop: vi.fn(async () => { + calls.push(`stop:${name}`); + }), + }; +} + +describe("ServiceManager", () => { + test("get returns the registered instance", () => { + const sm = new ServiceManager(); + const svc = { instance: { hello: "world" }, stop: vi.fn() }; + sm.add("greeter", svc); + + expect(sm.get<{ hello: string }>("greeter")).toEqual({ hello: "world" }); + }); + + test("get returns null for unknown service", () => { + const sm = new ServiceManager(); + expect(sm.get("missing")).toBeNull(); + }); + + test("add(null) is a no-op (opt-out pattern)", async () => { + const sm = new ServiceManager(); + sm.add("absent", null); + + expect(sm.get("absent")).toBeNull(); + await expect(sm.stop()).resolves.toBeUndefined(); + }); + + test("stop() invokes services in reverse add order", async () => { + const calls: string[] = []; + const sm = new ServiceManager(); + sm.add("a", mockService("a", calls)); + sm.add("b", mockService("b", calls)); + sm.add("c", mockService("c", calls)); + + await sm.stop(); + + expect(calls).toEqual(["stop:c", "stop:b", "stop:a"]); + }); + + test("stop() continues when one service throws", async () => { + const calls: string[] = []; + const sm = new ServiceManager(); + sm.add("a", mockService("a", calls)); + sm.add("b", { + instance: {}, + stop: vi.fn(async () => { + throw new Error("boom"); + }), + }); + sm.add("c", mockService("c", calls)); + + await expect(sm.stop()).resolves.toBeUndefined(); + expect(calls).toEqual(["stop:c", "stop:a"]); + }); + + test("stop() drains the registry (idempotent on second call)", async () => { + const calls: string[] = []; + const sm = new ServiceManager(); + sm.add("a", mockService("a", calls)); + + await sm.stop(); + await sm.stop(); + + expect(calls).toEqual(["stop:a"]); + expect(sm.get("a")).toBeNull(); + }); +}); diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 49d211913..d8801aba5 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -218,14 +218,10 @@ 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(); } + // Fallback for plugins instantiated outside `_createApp` (test path). private tryAttachContext(): void { try { this.cache = CacheManager.getInstanceSync(); @@ -239,23 +235,17 @@ export abstract class Plugin< 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. - */ attachContext( deps: { context?: unknown; + services?: { get(name: string): T | null }; telemetryConfig?: BasePluginConfig["telemetry"]; } = {}, ): void { - if (!this.cache) { - this.cache = CacheManager.getInstanceSync(); - } + this.cache = + deps.services?.get("cache") ?? + this.cache ?? + CacheManager.getInstanceSync(); this.telemetry = TelemetryManager.getProvider( this.name, deps.telemetryConfig ?? this.config.telemetry, @@ -698,10 +688,12 @@ export abstract class Plugin< } private _checkIfGenerator( - result: any, - ): result is AsyncGenerator { + result: unknown, + ): result is AsyncGenerator { return ( - result && typeof result === "object" && Symbol.asyncIterator in result + typeof result === "object" && + result !== null && + Symbol.asyncIterator in result ); } } diff --git a/packages/appkit/src/telemetry/telemetry-manager.ts b/packages/appkit/src/telemetry/telemetry-manager.ts index 6660b6b2c..a062c8f32 100644 --- a/packages/appkit/src/telemetry/telemetry-manager.ts +++ b/packages/appkit/src/telemetry/telemetry-manager.ts @@ -95,13 +95,27 @@ export class TelemetryManager { }); this.sdk.start(); - this.registerShutdown(); logger.debug("Initialized successfully"); } catch (error) { logger.error("Failed to initialize: %O", error); } } + /** True once the underlying NodeSDK has started. */ + isActive(): boolean { + return this.sdk !== undefined; + } + + /** Returns `null` when no OTLP endpoint is configured (telemetry opt-out). */ + static async boot( + config?: TelemetryConfig, + ): Promise<{ instance: TelemetryManager; stop(): Promise } | null> { + TelemetryManager.initialize(config); + const instance = TelemetryManager.getInstance(); + if (!instance.isActive()) return null; + return { instance, stop: () => instance.shutdown() }; + } + /** * Register OpenTelemetry instrumentations. * Can be called at any time, but recommended to call in plugin constructor. @@ -158,15 +172,12 @@ export class TelemetryManager { ]; } - private registerShutdown() { - const shutdownFn = async () => { - await TelemetryManager.getInstance().shutdown(); - }; - process.once("SIGTERM", shutdownFn); - process.once("SIGINT", shutdownFn); - } - - private async shutdown(): Promise { + /** + * Drains pending spans/metrics/logs and shuts down the NodeSDK. + * Idempotent. + * @internal + */ + async shutdown(): Promise { if (!this.sdk) { return; } diff --git a/packages/appkit/src/telemetry/tests/telemetry-manager.test.ts b/packages/appkit/src/telemetry/tests/telemetry-manager.test.ts index 11b85d9bf..92e453a01 100644 --- a/packages/appkit/src/telemetry/tests/telemetry-manager.test.ts +++ b/packages/appkit/src/telemetry/tests/telemetry-manager.test.ts @@ -54,8 +54,6 @@ describe("TelemetryManager", () => { vi.clearAllMocks(); // @ts-expect-error - accessing private static property for testing TelemetryManager.instance = undefined; - // @ts-expect-error - accessing private static property for testing - TelemetryManager.shutdownRegistered = false; }); afterEach(() => { diff --git a/packages/shared/src/plugin.ts b/packages/shared/src/plugin.ts index 651840c7b..aae5c7a4d 100644 --- a/packages/shared/src/plugin.ts +++ b/packages/shared/src/plugin.ts @@ -27,12 +27,10 @@ export interface BasePlugin { clientConfig?(): Record; - /** - * Binds runtime dependencies (telemetry, cache, plugin context) after the - * plugin has been constructed. Called by the AppKit core before `setup()`. - */ + /** Binds runtime deps (context, core services). Called before `setup()`. */ attachContext?(deps: { context?: unknown; + services?: { get(name: string): T | null }; telemetryConfig?: TelemetryOptions; }): void; }