diff --git a/.changeset/plugin-teardown-reaches-destroy.md b/.changeset/plugin-teardown-reaches-destroy.md new file mode 100644 index 0000000000..8958c81545 --- /dev/null +++ b/.changeset/plugin-teardown-reaches-destroy.md @@ -0,0 +1,27 @@ +--- +"@objectstack/plugin-reports": patch +"@objectstack/connector-openapi": patch +"@objectstack/connector-rest": patch +"@objectstack/connector-slack": patch +"@objectstack/plugin-approvals": patch +"@objectstack/service-knowledge": patch +--- + +Release these plugins' resources from `destroy()`, the teardown hook the kernel +actually calls (#10371). `Plugin` declares `init()`, `start?(ctx)` and +`destroy?()` — and no `stop()` — so `ObjectKernel.performShutdown()` and +`LiteKernel.destroy()`, which walk the plugins in reverse calling +`plugin.destroy()`, walked straight past every plugin whose teardown was spelled +`stop()`. `await kernel.shutdown()` resolved with the reports dispatcher still +armed, the REST/OpenAPI/Slack connectors still registered on the automation +engine, the approvals SLA escalation job still scheduled, and the knowledge +event-sync subscription still open. + +Each teardown body now lives in `destroy()`. `stop()` is retained as a +delegating alias with its parameter made optional, so an embedder that learned +to call it directly — precisely because the kernel never did — keeps working +unchanged. No export is removed and the `Plugin` interface is untouched. + +Same defect as #9371 in `@objectstack/service-messaging`, which surfaced as +fully green test runs exiting 1 on `EnvironmentTeardownError` and being evicted +from the merge queue. diff --git a/packages/connectors/connector-openapi/src/connector-openapi-plugin.ts b/packages/connectors/connector-openapi/src/connector-openapi-plugin.ts index eab8d3326d..65fa80cdad 100644 --- a/packages/connectors/connector-openapi/src/connector-openapi-plugin.ts +++ b/packages/connectors/connector-openapi/src/connector-openapi-plugin.ts @@ -101,10 +101,38 @@ export class ConnectorOpenApiPlugin implements Plugin { ctx.logger.info(`ConnectorOpenApiPlugin: OpenAPI connector '${this.connectorName}' registered`); } - async stop(_ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. `Plugin` declares + * `init()`, `start?()` and `destroy?()` and no `stop()`, so the kernel + * walked past this plugin at shutdown and the OpenAPI connector stayed registered in the automation + * engine for the lifetime of the process. `start()` IS on the + * interface, so the pair read as symmetric in review — that asymmetry is + * what let the same shape survive in six packages at once. + * + * No timers here, so this instance never cost a merge-queue eviction the + * way the `plugin-reports` / `service-messaging` members did (#9371). The + * class is the same one either way: a teardown the kernel does not reach. + */ + async destroy(): Promise { if (this.automation && this.connectorName) { try { this.automation.unregisterConnector(this.connectorName); } catch { /* ignore */ } } + this.automation = undefined; + this.connectorName = undefined; + } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to + * call it directly precisely BECAUSE the kernel never did. Prefer kernel + * shutdown; direct callers keep working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); } private tryGetAutomation(ctx: PluginContext): ConnectorRegistrySurface | undefined { diff --git a/packages/connectors/connector-openapi/src/plugin-shutdown-unregisters-connector.test.ts b/packages/connectors/connector-openapi/src/plugin-shutdown-unregisters-connector.test.ts new file mode 100644 index 0000000000..83dd8a2e1f --- /dev/null +++ b/packages/connectors/connector-openapi/src/plugin-shutdown-unregisters-connector.test.ts @@ -0,0 +1,69 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must actually reach this plugin's teardown. + * + * THE DEFECT THIS PINS. The teardown that unregisters the hand-wired OpenAPI + * connector was spelled `stop()`. `Plugin` (`@objectstack/core`'s `types.ts`) + * declares `init()`, `start?(ctx)` and `destroy?()` — and NO `stop()` — so + * `ObjectKernel.performShutdown()` and `LiteKernel.destroy()`, which walk the + * plugins in reverse calling `plugin.destroy()`, walked straight past it. + * Nothing in the repo ever called `stop()`. + * + * WHY THE ASSERTION IS BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches — the hook merely + * EXISTING is not the property that was missing, being CALLED BY THE KERNEL is. + * + * THE PRE-SHUTDOWN LEG IS A POSITIVE CONTROL and load-bearing: without it, a + * plugin that never registered anything would satisfy the post-shutdown + * assertion vacuously. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { ConnectorOpenApiPlugin } from './connector-openapi-plugin.js'; +import type { OpenApiDocument } from './openapi-connector.js'; + +/** Smallest document that yields one named connector ('mini') with one action. */ +const document: OpenApiDocument = { + info: { title: 'Mini' }, + servers: [{ url: 'https://api.mini.example.com' }], + paths: { + '/ping': { get: { operationId: 'ping', responses: { '200': { description: 'ok' } } } }, + }, +}; + +describe('#10371 ConnectorOpenApiPlugin releases its connector on kernel shutdown', () => { + it('unregisters the OpenAPI connector once shutdown() has resolved', async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + kernel.use(new ConnectorOpenApiPlugin({ document })); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + + // POSITIVE CONTROL — the connector really is registered. + expect(engine.getRegisteredConnectors()).toContain('mini'); + + await kernel.shutdown(); + + // THE PIN. Before the fix this still contained 'mini'. + expect(engine.getRegisteredConnectors()).not.toContain('mini'); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + const plugin = new ConnectorOpenApiPlugin({ document }); + kernel.use(plugin); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + expect(engine.getRegisteredConnectors()).toContain('mini'); + + await plugin.stop(); + + expect(engine.getRegisteredConnectors()).not.toContain('mini'); + }); +}); diff --git a/packages/connectors/connector-openapi/vitest.config.ts b/packages/connectors/connector-openapi/vitest.config.ts new file mode 100644 index 0000000000..cd2bb2114f --- /dev/null +++ b/packages/connectors/connector-openapi/vitest.config.ts @@ -0,0 +1,59 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * This package had NO vitest config until #10371, and that is the fact this + * header exists to keep visible: adding one changes how every test file in + * `packages/connectors/connector-openapi` is configured, not just the file that + * needed it. So it is deliberately minimal — anchored `resolve.alias` entries + * and **no `test` block at all**, because the package's existing test files run + * on vitest's defaults (`globals: false`, `environment: 'node'`) and import + * `describe`/`it`/`expect` explicitly. Sibling configs in this repo do carry + * `test: { globals: true, … }`; copying that shape here would silently + * re-specify the defaults for every existing file. + * + * ## Why the two entries + * + * `plugin-shutdown-unregisters-connector.test.ts` (#10371) boots a REAL + * `LiteKernel` with the REAL `AutomationServicePlugin` to prove that + * `kernel.shutdown()` reaches `ConnectorOpenApiPlugin.destroy()` — the whole + * point of that card is that the kernel calls `destroy()` and never called + * `stop()`, so a stand-in kernel would assert nothing. Without these entries + * both imports resolve through their packages' `exports` to **dist**, which + * makes the test a verdict about build state rather than about the source in + * the checkout — and the dangerous half of that is not a loud error but a test + * that passes GREEN against a stale artifact with nothing in the output saying + * so. `scripts/check-test-source-alias.mjs` carries the measured history + * (#7668, #7778, #7849); it named these two imports and is the gate that fails + * without the entries below. + * + * ⚠️ The registry in that script is SHRINK-ONLY, so widening + * `KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/connector-openapi']` was never an + * option, and it is deliberately left untouched: its one remaining member, + * `@objectstack/spec`, is still reached unaliased by this package's other test + * files and stays that registry's problem to retire. Aliasing bare + * `@objectstack/spec` here would additionally hit rule 5's ENOTDIR trap (its + * subpaths would resolve through `…/spec/src/index.ts/`), which is why + * only the two specifiers the gate actually named are added. + */ + +import { defineConfig } from 'vitest/config'; +import path from 'path'; + +export default defineConfig({ + resolve: { + // Array form with ANCHORED patterns, per the trap the gate documents: the + // object form matches by PREFIX, so a bare key whose replacement is a FILE + // also swallows every subpath and resolves it to `…/index.ts/` + // (`ENOTDIR`, at run time, in a config that looks right). + alias: [ + { + find: /^@objectstack\/core$/, + replacement: path.resolve(__dirname, '../../core/src/index.ts'), + }, + { + find: /^@objectstack\/service-automation$/, + replacement: path.resolve(__dirname, '../../services/service-automation/src/index.ts'), + }, + ], + }, +}); diff --git a/packages/connectors/connector-rest/src/connector-rest-plugin.ts b/packages/connectors/connector-rest/src/connector-rest-plugin.ts index 4dd450ea88..1e94486338 100644 --- a/packages/connectors/connector-rest/src/connector-rest-plugin.ts +++ b/packages/connectors/connector-rest/src/connector-rest-plugin.ts @@ -92,10 +92,38 @@ export class ConnectorRestPlugin implements Plugin { ctx.logger.info(`ConnectorRestPlugin: REST connector '${def.name}' registered`); } - async stop(_ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. `Plugin` declares + * `init()`, `start?()` and `destroy?()` and no `stop()`, so the kernel + * walked past this plugin at shutdown and the REST connector stayed registered in the automation + * engine for the lifetime of the process. `start()` IS on the + * interface, so the pair read as symmetric in review — that asymmetry is + * what let the same shape survive in six packages at once. + * + * No timers here, so this instance never cost a merge-queue eviction the + * way the `plugin-reports` / `service-messaging` members did (#9371). The + * class is the same one either way: a teardown the kernel does not reach. + */ + async destroy(): Promise { if (this.automation && this.connectorName) { try { this.automation.unregisterConnector(this.connectorName); } catch { /* ignore */ } } + this.automation = undefined; + this.connectorName = undefined; + } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to + * call it directly precisely BECAUSE the kernel never did. Prefer kernel + * shutdown; direct callers keep working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); } private tryGetAutomation(ctx: PluginContext): ConnectorRegistrySurface | undefined { diff --git a/packages/connectors/connector-rest/src/plugin-shutdown-unregisters-connector.test.ts b/packages/connectors/connector-rest/src/plugin-shutdown-unregisters-connector.test.ts new file mode 100644 index 0000000000..0352eba9d6 --- /dev/null +++ b/packages/connectors/connector-rest/src/plugin-shutdown-unregisters-connector.test.ts @@ -0,0 +1,68 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must actually reach this plugin's teardown. + * + * THE DEFECT THIS PINS. The teardown that unregisters the REST connector was + * spelled `stop()`. `Plugin` (`@objectstack/core`'s `types.ts`) declares + * `init()`, `start?(ctx)` and `destroy?()` — and NO `stop()` — so + * `ObjectKernel.performShutdown()` and `LiteKernel.destroy()`, which walk the + * plugins in reverse calling `plugin.destroy()`, walked straight past it. + * Nothing in the repo ever called `stop()`. + * + * WHY THE ASSERTION IS BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches — the hook merely + * EXISTING is not the property that was missing, being CALLED BY THE KERNEL is. + * So this drives a real kernel through a real shutdown and reads the automation + * engine's own connector registry. + * + * THE PRE-SHUTDOWN LEG IS A POSITIVE CONTROL and load-bearing: without it, a + * plugin that never registered anything would satisfy the post-shutdown + * assertion vacuously. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { ConnectorRestPlugin } from './connector-rest-plugin.js'; + +const options = { baseUrl: 'https://api.example.com' }; + +describe('#10371 ConnectorRestPlugin releases its connector on kernel shutdown', () => { + it('unregisters the REST connector once shutdown() has resolved', async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + kernel.use(new ConnectorRestPlugin(options)); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + + // POSITIVE CONTROL — the connector really is registered, so the + // assertion below measures removal and not absence. + expect(engine.getRegisteredConnectors()).toContain('rest'); + + await kernel.shutdown(); + + // THE PIN. Before the fix this still contained 'rest': the kernel had + // no `destroy()` to call and `stop()` was never anybody's business. + expect(engine.getRegisteredConnectors()).not.toContain('rest'); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + // The alias exists precisely because an embedder may have learned to + // call it BECAUSE the kernel never did. Removing it would break them, + // so its behaviour is pinned rather than left to the fix's discretion. + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + const plugin = new ConnectorRestPlugin(options); + kernel.use(plugin); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + expect(engine.getRegisteredConnectors()).toContain('rest'); + + await plugin.stop(); + + expect(engine.getRegisteredConnectors()).not.toContain('rest'); + }); +}); diff --git a/packages/connectors/connector-slack/src/connector-slack-plugin.ts b/packages/connectors/connector-slack/src/connector-slack-plugin.ts index 4728d378f8..1dbc4b6766 100644 --- a/packages/connectors/connector-slack/src/connector-slack-plugin.ts +++ b/packages/connectors/connector-slack/src/connector-slack-plugin.ts @@ -71,9 +71,37 @@ export class ConnectorSlackPlugin implements Plugin { ctx.logger.info(`ConnectorSlackPlugin: Slack connector '${def.name}' registered`); } - async stop(_ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. `Plugin` declares + * `init()`, `start?()` and `destroy?()` and no `stop()`, so the kernel + * walked past this plugin at shutdown and the Slack connector stayed registered in the automation + * engine for the lifetime of the process. `start()` IS on the + * interface, so the pair read as symmetric in review — that asymmetry is + * what let the same shape survive in six packages at once. + * + * No timers here, so this instance never cost a merge-queue eviction the + * way the `plugin-reports` / `service-messaging` members did (#9371). The + * class is the same one either way: a teardown the kernel does not reach. + */ + async destroy(): Promise { if (this.automation && this.connectorName) { try { this.automation.unregisterConnector(this.connectorName); } catch { /* ignore */ } } + this.automation = undefined; + this.connectorName = undefined; + } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to + * call it directly precisely BECAUSE the kernel never did. Prefer kernel + * shutdown; direct callers keep working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); } } diff --git a/packages/connectors/connector-slack/src/plugin-shutdown-unregisters-connector.test.ts b/packages/connectors/connector-slack/src/plugin-shutdown-unregisters-connector.test.ts new file mode 100644 index 0000000000..9938a61210 --- /dev/null +++ b/packages/connectors/connector-slack/src/plugin-shutdown-unregisters-connector.test.ts @@ -0,0 +1,61 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must actually reach this plugin's teardown. + * + * THE DEFECT THIS PINS. The teardown that unregisters the Slack connector was + * spelled `stop()`. `Plugin` (`@objectstack/core`'s `types.ts`) declares + * `init()`, `start?(ctx)` and `destroy?()` — and NO `stop()` — so + * `ObjectKernel.performShutdown()` and `LiteKernel.destroy()`, which walk the + * plugins in reverse calling `plugin.destroy()`, walked straight past it. + * Nothing in the repo ever called `stop()`. + * + * WHY THE ASSERTION IS BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches — the hook merely + * EXISTING is not the property that was missing, being CALLED BY THE KERNEL is. + * + * THE PRE-SHUTDOWN LEG IS A POSITIVE CONTROL and load-bearing: without it, a + * plugin that never registered anything would satisfy the post-shutdown + * assertion vacuously. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; +import { ConnectorSlackPlugin } from './connector-slack-plugin.js'; + +const options = { token: 'xoxb-secret-token' }; + +describe('#10371 ConnectorSlackPlugin releases its connector on kernel shutdown', () => { + it('unregisters the Slack connector once shutdown() has resolved', async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + kernel.use(new ConnectorSlackPlugin(options)); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + + // POSITIVE CONTROL — the connector really is registered. + expect(engine.getRegisteredConnectors()).toContain('slack'); + + await kernel.shutdown(); + + // THE PIN. Before the fix this still contained 'slack'. + expect(engine.getRegisteredConnectors()).not.toContain('slack'); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + const kernel = new LiteKernel(); + kernel.use(new AutomationServicePlugin()); + const plugin = new ConnectorSlackPlugin(options); + kernel.use(plugin); + await kernel.bootstrap(); + + const engine = kernel.getService('automation'); + expect(engine.getRegisteredConnectors()).toContain('slack'); + + await plugin.stop(); + + expect(engine.getRegisteredConnectors()).not.toContain('slack'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index c05b5bd70b..033d81be6e 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -92,6 +92,14 @@ export class ApprovalsServicePlugin implements Plugin { private service?: ApprovalService; private engine?: IObjectQLEngine; private escalationJobScheduled = false; + /** + * Captured where the job is scheduled, not resolved at teardown: `destroy()` + * — the hook the kernel actually calls — takes NO `PluginContext` + * (`Plugin.destroy?(): Promise | void`, core `types.ts`). Holding the + * very service the schedule was placed with also means the cancel cannot miss + * it because the registry has already been torn down around us. + */ + private jobService?: IJobService; constructor(options: ApprovalsPluginOptions = {}) { this.options = options; @@ -259,6 +267,7 @@ export class ApprovalsServicePlugin implements Plugin { } }; await jobs.schedule(ESCALATION_JOB_NAME, { type: 'interval', intervalMs }, sweep); + this.jobService = jobs; this.escalationJobScheduled = true; void sweep().catch((err: any) => { ctx.logger.warn?.('[approvals] boot sweep failed', { error: err?.message }); @@ -372,16 +381,46 @@ export class ApprovalsServicePlugin implements Plugin { } } - async stop(ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. `Plugin` declares + * `init()`, `start?()` and `destroy?()` and no `stop()`, so the kernel walked + * past this plugin at shutdown: the SLA escalation job stayed scheduled and + * this plugin's ObjectQL hooks stayed bound to an engine the kernel had + * finished with. `start()` IS on the interface, so the pair read as symmetric + * in review — that asymmetry is what let the same shape survive in six + * packages at once. + * + * This member owns no timer of its own (the escalation clock belongs to + * `service-job`), so it never cost a merge-queue eviction the way the + * `plugin-reports` / `service-messaging` members did (#9371). The class is + * the same one either way: a teardown the kernel does not reach. + */ + async destroy(): Promise { if (this.escalationJobScheduled) { try { - const jobs = ctx.getService('job'); - await jobs?.cancel?.(ESCALATION_JOB_NAME); + await this.jobService?.cancel?.(ESCALATION_JOB_NAME); } catch { /* ignore */ } this.escalationJobScheduled = false; + this.jobService = undefined; } if (this.engine) { try { unbindAllHooks(this.engine); } catch { /* ignore */ } } } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to call + * it directly precisely BECAUSE the kernel never did. The parameter is now + * optional and ignored: `destroy()` takes no context, so teardown uses the + * job service captured when the escalation clock was wired. Prefer kernel + * shutdown; direct callers keep working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); + } } diff --git a/packages/plugins/plugin-approvals/src/plugin-shutdown-cancels-escalation-job.test.ts b/packages/plugins/plugin-approvals/src/plugin-shutdown-cancels-escalation-job.test.ts new file mode 100644 index 0000000000..247fbb660d --- /dev/null +++ b/packages/plugins/plugin-approvals/src/plugin-shutdown-cancels-escalation-job.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must actually reach this plugin's teardown. + * + * THE DEFECT THIS PINS. The teardown that cancels the SLA escalation job and + * unbinds this plugin's ObjectQL lifecycle hooks was spelled `stop()`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` — and NO `stop()` — so `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()`, which walk the plugins in reverse calling + * `plugin.destroy()`, walked straight past it. Nothing in the repo ever called + * `stop()`. + * + * THE ASYMMETRY THAT HID IT. `start?(ctx)` IS on the interface and does fire, + * so a `start`/`stop` pair reads as symmetric in review — which is how the same + * shape survived in six packages at once (#9371 found the first instance only + * after it had evicted two fully green PRs from the merge queue). + * + * WHY THE ASSERTION IS BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches — the hook merely + * EXISTING is not the property that was missing, being CALLED BY THE KERNEL is. + * So this drives a real `ObjectKernel` through a real shutdown and reads what + * the plugin asked the job service to do. + * + * This member owns no timer of its own — the escalation clock belongs to + * `service-job` — so it never cost an eviction the way `plugin-reports` and + * `service-messaging` could. The class is the same one either way. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import type { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import { ESCALATION_JOB_NAME } from './approval-service.js'; + +const openKernels: ObjectKernel[] = []; +const openDrivers: Array<{ disconnect?: () => Promise }> = []; + +afterEach(async () => { + while (openKernels.length) { + try { await openKernels.pop()?.shutdown(); } catch { /* already stopped */ } + } + while (openDrivers.length) { + try { await openDrivers.pop()?.disconnect?.(); } catch { /* noop */ } + } +}); + +interface JobLog { + scheduled: string[]; + cancelled: string[]; +} + +/** + * Publishes a minimal `job` service. A plugin rather than a bare + * `registerService` call because the escalation clock is only wired if the + * service is resolvable at `kernel:ready`, which is the kernel's business. + */ +class FakeJobServicePlugin implements Plugin { + name = 'test.fake.job'; + version = '1.0.0'; + type = 'standard'; + + constructor(private readonly log: JobLog) {} + + async init(ctx: PluginContext): Promise { + ctx.registerService('job', { + schedule: async (name: string) => { this.log.scheduled.push(name); }, + cancel: async (name: string) => { this.log.cancelled.push(name); }, + }); + } +} + +async function bootApprovalsKernel(log: JobLog) { + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); + openKernels.push(kernel); + + await kernel.use(new ObjectQLPlugin()); + await kernel.use(new FakeJobServicePlugin(log)); + const plugin = new ApprovalsServicePlugin({ escalationScanIntervalMs: 60_000 }); + await kernel.use(plugin); + await kernel.bootstrap(); + + // A real table behind the boot catch-up sweep, so its reads resolve instead + // of erroring their way through the plugin's own logger during teardown. + // Typed at the lookup (`check:slot-lookup` / #4251): the slot's contract type, + // not `any`. The two schema-provisioning calls below are engine internals the + // published contract does not carry, so the cast is at the call and not at the + // slot. + const objectql = kernel.getService('objectql'); + const driver: any = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.connect(); + (objectql as any).registerDriver(driver, true); + openDrivers.push(driver); + await (objectql as any).syncSchemas(); + + return { kernel, plugin }; +} + +describe('#10371 ApprovalsServicePlugin releases its escalation job on kernel shutdown', () => { + it('cancels the SLA escalation job once shutdown() has resolved', async () => { + const log: JobLog = { scheduled: [], cancelled: [] }; + const { kernel } = await bootApprovalsKernel(log); + + // POSITIVE CONTROL — the clock really was wired, so the assertion below + // measures a cancel and not an absence. + expect(log.scheduled).toContain(ESCALATION_JOB_NAME); + expect(log.cancelled).toEqual([]); + + await kernel.shutdown(); + + // THE PIN. Before the fix the cancel lived in `stop()`, which the kernel + // never called, so the job outlived the kernel that scheduled it. + expect(log.cancelled).toContain(ESCALATION_JOB_NAME); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + // The alias exists precisely because an embedder may have learned to call + // it BECAUSE the kernel never did. Pinning only the shutdown direction + // would go green on an implementation that simply deletes `stop()`. + const log: JobLog = { scheduled: [], cancelled: [] }; + const { plugin } = await bootApprovalsKernel(log); + + expect(log.scheduled).toContain(ESCALATION_JOB_NAME); + expect(log.cancelled).toEqual([]); + + await plugin.stop(); + + expect(log.cancelled).toContain(ESCALATION_JOB_NAME); + }); + + it('a teardown on a plugin that never started is a no-op rather than a throw', async () => { + const plugin = new ApprovalsServicePlugin(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.stop()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts b/packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts new file mode 100644 index 0000000000..800473ea17 --- /dev/null +++ b/packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must leave NOTHING of this plugin still + * running. + * + * THE DEFECT. `ReportsServicePlugin` arms its schedule dispatcher at + * `kernel:ready` — either a `setInterval` over `sys_report_schedule`, or, when + * `service-job` is installed, a scheduled `reports.dispatch` job. The teardown + * that released both was spelled `stop()`. The kernel's plugin teardown hook is + * `destroy()` (`Plugin.destroy?()` in `@objectstack/core`'s `types.ts` — the + * only teardown `ObjectKernel.performShutdown()` and `LiteKernel.destroy()` + * invoke), and `stop()` is not part of that interface, so nothing in the repo + * ever called it and the dispatcher went on ticking after shutdown resolved. + * + * THE ASYMMETRY THAT HID IT. `start?(ctx)` IS on the interface and does fire. + * A `start`/`stop` pair where only one half is wired reads as symmetric in + * review — which is why the identical shape survived in six packages at once, + * and why #9371 found it in `service-messaging` only after it had already cost + * something. + * + * WHY IT WENT UNNOTICED, AND WHERE THE BILL LANDED. `start()` `unref()`s the + * interval, so a long-lived host process still exits and the leak is silent in + * production. Under vitest the worker is alive throughout teardown, so a tick + * fires AFTER the test file is over, reads through a driver the suite has + * already disconnected, and the driver's console fallback warns. `console.*` + * inside a worker is an RPC to the main process (`onUserConsoleLog`); one + * issued after `rpcDone()` has snapshotted the pending set is rejected by + * `$rejectPendingCalls` as `EnvironmentTeardownError: [vitest-worker]: Closing + * rpc while "onUserConsoleLog" was pending`. Nothing awaits that promise, so it + * surfaces as an unhandled rejection and fails a run in which every test + * passed — twice measured on `examples/app-showcase` (334/334 and 337/337 + * green, exit 1, a merge-queue eviction each time). + * + * WHAT THIS PINS, AND WHY IN THIS SHAPE. The assertions are behavioural — + * "after shutdown the plugin issues no further schedule reads", "after shutdown + * the scheduled job has been cancelled" — and not + * `expect(plugin.destroy).toBeDefined()`, because the hook merely EXISTING is + * not the property that was missing; being REACHED BY THE KERNEL is. + * + * Every pre-shutdown leg is a POSITIVE CONTROL and load-bearing: without it a + * dispatcher that never started would satisfy the post-shutdown assertion + * vacuously, and this file would pass on a plugin that does nothing at all. + * + * The `stop()` legs are the other direction, and they are not decoration: the + * repair keeps `stop()` as a delegating alias because it is public API of an + * exported class and an embedder may have learned to call it directly PRECISELY + * BECAUSE the kernel never did. Pinning only the shutdown direction would go + * green on an implementation that simply deletes `stop()`, which breaks them. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { ObjectKernel, LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import type { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { IDataEngine } from '@objectstack/spec/contracts'; +import { ReportsServicePlugin } from './reports-plugin.js'; + +/** + * The plugin floors its own interval at 5s (`Math.max(5_000, …)`), so this is + * the fastest REAL clock the dispatcher can be driven at. Windows below are + * sized off it rather than off a wish. + */ +const DISPATCH_INTERVAL_MS = 5_000; +/** Comfortably past one tick boundary, so a window that sees zero is silence. */ +const OBSERVE_MS = 5_600; + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +const openKernels: Array<{ shutdown(): Promise }> = []; +const openDrivers: Array<{ disconnect?: () => Promise }> = []; + +afterEach(async () => { + // Kernels first, drivers second: the kernel's own teardown still wants a live + // driver to drain against. + while (openKernels.length) { + try { await openKernels.pop()?.shutdown(); } catch { /* already stopped */ } + } + while (openDrivers.length) { + try { await openDrivers.pop()?.disconnect?.(); } catch { /* noop */ } + } +}); + +/** Records what the plugin asked the platform job service to run and cancel. */ +interface JobLog { + scheduled: string[]; + cancelled: string[]; +} + +/** + * Publishes a minimal `job` service so the plugin takes its job-service branch + * instead of the `setInterval` fallback. A plugin rather than a bare + * `registerService` call because the branch is only taken if the service is + * resolvable at `kernel:ready`, which is the kernel's business, not ours. + */ +class FakeJobServicePlugin implements Plugin { + name = 'test.fake.job'; + version = '1.0.0'; + type = 'standard'; + + constructor(private readonly log: JobLog) {} + + async init(ctx: PluginContext): Promise { + ctx.registerService('job', { + schedule: async (name: string) => { this.log.scheduled.push(name); }, + cancel: async (name: string) => { this.log.cancelled.push(name); }, + }); + } +} + +interface Booted { + kernel: { shutdown(): Promise }; + plugin: ReportsServicePlugin; + /** Reads the dispatcher has made against `sys_report_schedule`. */ + scheduleReads: () => number; +} + +/** + * Installs the read counter on the engine instance the dispatcher captured at + * `kernel:ready` (`ctx.getService('objectql')` resolves to this same object), so + * the tally is of real `ReportService.dispatchDue()` traffic and not of a + * stand-in. + */ +function countScheduleReads(engineHolder: { getService: (n: string) => T }): () => number { + type EngineCall = (name: string, ...rest: unknown[]) => unknown; + const engine = engineHolder.getService('objectql') as unknown as + Record<'find', EngineCall>; + let reads = 0; + const orig = engine.find.bind(engine); + engine.find = (name: string, ...rest: unknown[]) => { + if (String(name) === 'sys_report_schedule') reads++; + return orig(name, ...rest); + }; + return () => reads; +} + +/** A real in-memory SQL driver, connected and registered on `engine`. */ +async function attachSqlite(objectql: any): Promise { + const driver: any = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.connect(); + objectql.registerDriver(driver, true); + openDrivers.push(driver); + await objectql.syncSchemas(); +} + +/** + * The `setInterval` branch, and why it needs `LiteKernel` — MEASURED, not + * assumed. `ReportsServicePlugin.start()` prefers the platform job service and + * only falls through to `setInterval` when `ctx.getService('job')` yields + * nothing with a `schedule` method. `ObjectKernel.preInjectCoreFallbacks()` + * registers `createMemoryJob()` for every unprovided `core` service before + * Phase 2, so on an `ObjectKernel` a `job` service ALWAYS resolves and this + * branch is unreachable. `LiteKernel` injects no fallbacks, which is exactly + * the "single-kernel deployment without `service-job`" the plugin's own + * docblock names as the reason the `setInterval` path exists. + */ +async function bootReportsLiteKernel(): Promise { + const kernel = new LiteKernel({ logger: { level: 'silent' } }); + openKernels.push(kernel); + + kernel.use(new ObjectQLPlugin()); + const plugin = new ReportsServicePlugin({ dispatchIntervalMs: DISPATCH_INTERVAL_MS }); + kernel.use(plugin); + await kernel.bootstrap(); + + await attachSqlite(kernel.getService('objectql')); + return { kernel, plugin, scheduleReads: countScheduleReads(kernel as any) }; +} + +async function bootReportsKernel(extra?: Plugin): Promise { + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); + openKernels.push(kernel); + + await kernel.use(new ObjectQLPlugin()); + if (extra) await kernel.use(extra); + const plugin = new ReportsServicePlugin({ dispatchIntervalMs: DISPATCH_INTERVAL_MS }); + await kernel.use(plugin); + await kernel.bootstrap(); + + await attachSqlite(kernel.getService('objectql')); + return { kernel, plugin, scheduleReads: countScheduleReads(kernel as any) }; +} + +describe('#10371 ReportsServicePlugin releases its dispatcher on kernel shutdown', () => { + it( + 'stops reading sys_report_schedule once shutdown() has resolved', + { timeout: 60_000 }, + async () => { + const { kernel, scheduleReads } = await bootReportsLiteKernel(); + + // POSITIVE CONTROL — the setInterval dispatcher really is ticking, so the + // post-shutdown assertion below measures silence and not absence. + await sleep(OBSERVE_MS); + expect(scheduleReads()).toBeGreaterThan(0); + + await kernel.shutdown(); + + const atShutdown = scheduleReads(); + await sleep(OBSERVE_MS); + + // THE PIN. shutdown() resolving means the plugin is done with the + // database. Before the fix this count kept climbing. + expect(scheduleReads()).toBe(atShutdown); + }, + ); + + it('cancels its scheduled job once shutdown() has resolved', async () => { + const log: JobLog = { scheduled: [], cancelled: [] }; + const { kernel } = await bootReportsKernel(new FakeJobServicePlugin(log)); + + // POSITIVE CONTROL — the job branch was taken and nothing has cancelled it. + expect(log.scheduled).toContain('reports.dispatch'); + expect(log.cancelled).toEqual([]); + + await kernel.shutdown(); + + // THE PIN. Before the fix the cancel lived in `stop()`, which the kernel + // never called, so the job outlived the kernel that scheduled it. + expect(log.cancelled).toContain('reports.dispatch'); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + const log: JobLog = { scheduled: [], cancelled: [] }; + const { plugin } = await bootReportsKernel(new FakeJobServicePlugin(log)); + + expect(log.scheduled).toContain('reports.dispatch'); + expect(log.cancelled).toEqual([]); + + // No argument — the shape an embedder writes today against a method whose + // parameter the repair made optional. + await plugin.stop(); + + expect(log.cancelled).toContain('reports.dispatch'); + }); + + it('the stop() alias still accepts the PluginContext argument it used to require', async () => { + const log: JobLog = { scheduled: [], cancelled: [] }; + const { plugin } = await bootReportsKernel(new FakeJobServicePlugin(log)); + + expect(log.scheduled).toContain('reports.dispatch'); + + // The pre-repair signature was `stop(ctx: PluginContext)`. An embedder + // holding that call shape must keep compiling AND keep working, which is + // the entire reason the alias was retained rather than deleted. + const ctx = { + logger: { info() {}, warn() {}, error() {}, debug() {} }, + } as unknown as PluginContext; + await plugin.stop(ctx); + + expect(log.cancelled).toContain('reports.dispatch'); + }); + + it('a teardown on a plugin that never started is a no-op rather than a throw', async () => { + // Idempotence matters because `destroy()` clears the handles it released; a + // teardown that only works once is a teardown that fails inside a suite, + // and the kernel calls it on every plugin it walks. + const plugin = new ReportsServicePlugin(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.stop()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-reports/src/reports-plugin.ts b/packages/plugins/plugin-reports/src/reports-plugin.ts index c38127db0c..581074d2d8 100644 --- a/packages/plugins/plugin-reports/src/reports-plugin.ts +++ b/packages/plugins/plugin-reports/src/reports-plugin.ts @@ -53,12 +53,20 @@ export class ReportsServicePlugin implements Plugin { private intervalHandle?: ReturnType; private jobName?: string; private jobService?: IJobService; + /** + * Captured in `init()` because `destroy()` — the hook the kernel actually + * calls — takes NO `PluginContext` (`Plugin.destroy?(): Promise | void` + * in core's `types.ts`). Teardown still needs somewhere to report a failed + * job cancellation, so the logger is held rather than resolved late. + */ + private logger?: PluginContext['logger']; constructor(options: ReportsPluginOptions = {}) { this.options = options; } async init(ctx: PluginContext): Promise { + this.logger = ctx.logger; ctx.getService<{ register(m: any): void }>('manifest').register({ id: 'com.objectstack.service.reports', name: 'Reports Service', @@ -166,12 +174,58 @@ export class ReportsServicePlugin implements Plugin { }); } - async stop(ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. The body below was + * correct in every respect except its NAME: `Plugin` declares `init()`, + * `start?()` and `destroy?()` and no `stop()`, so the kernel walked straight + * past this plugin at shutdown and the dispatcher went on ticking after + * `await kernel.shutdown()` had resolved. The asymmetry is what hid it: + * `start()` IS on the interface and does fire, so a `start`/`stop` pair reads + * as symmetric in review. + * + * WHY IT STAYED INVISIBLE, AND WHERE THE BILL LANDS. `start()` `unref()`s the + * interval, so a long-lived host process still exits and nothing complains in + * production. Under vitest the worker is alive throughout teardown, so a tick + * fires after the test file is over, reads through a driver the suite already + * disconnected, and a console fallback warns; `console.*` inside a worker is + * an RPC (`onUserConsoleLog`) and one issued after `rpcDone()` snapshotted the + * pending set is rejected as `EnvironmentTeardownError`. Nothing awaits that + * promise, so it lands as an unhandled rejection and fails a run in which + * every test passed. That is the identical defect #9371 fixed in + * `MessagingServicePlugin` — measured there as two green PRs (334/334 and + * 337/337) evicted from the merge queue. + * + * The job-service branch matters as much as the timer: when `service-job` is + * installed the dispatcher is a scheduled job rather than a `setInterval`, so + * a teardown the kernel never reaches leaks whichever of the two this + * deployment happens to use. + */ + async destroy(): Promise { if (this.intervalHandle) clearInterval(this.intervalHandle); this.intervalHandle = undefined; if (this.jobService && this.jobName && typeof this.jobService.cancel === 'function') { try { await this.jobService.cancel(this.jobName); } - catch (err) { ctx.logger.warn('ReportsServicePlugin: failed to cancel job', err as any); } + catch (err) { this.logger?.warn('ReportsServicePlugin: failed to cancel job', err as any); } } + // Cleared so a second teardown is a no-op rather than a second cancel — a + // teardown that only works once is a teardown that fails inside a suite. + this.jobService = undefined; + this.jobName = undefined; + } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to call + * it directly precisely BECAUSE the kernel never did. The parameter is now + * optional and ignored: `destroy()` takes no context, so teardown reads the + * logger captured in `init()`. Prefer kernel shutdown; direct callers keep + * working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); } } diff --git a/packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts b/packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts new file mode 100644 index 0000000000..3b147f2f36 --- /dev/null +++ b/packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts @@ -0,0 +1,107 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10371] `await kernel.shutdown()` must actually reach this plugin's teardown. + * + * THE DEFECT THIS PINS. The teardown that releases the `knowledge-event-sync` + * realtime subscription was spelled `stop()`. `Plugin` (`@objectstack/core`'s + * `types.ts`) declares `init()`, `start?(ctx)` and `destroy?()` — and NO + * `stop()` — so `ObjectKernel.performShutdown()` and `LiteKernel.destroy()`, + * which walk the plugins in reverse calling `plugin.destroy()`, walked straight + * past it. Nothing in the repo ever called `stop()`, so the subscription + * outlived the kernel that created it. + * + * THE ASYMMETRY THAT HID IT. `start?(ctx)` IS on the interface and does fire, + * so a `start`/`stop` pair reads as symmetric in review — which is how the same + * shape survived in six packages at once (#9371 found the first instance only + * after it had evicted two fully green PRs from the merge queue). + * + * WHY THE ASSERTION IS BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches — the hook merely + * EXISTING is not the property that was missing, being CALLED BY THE KERNEL is. + * So this drives a real `LiteKernel` through a real shutdown and reads what the + * realtime service was actually asked to do. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import type { RealtimeEventHandler } from '@objectstack/spec/contracts'; +import { KnowledgeServicePlugin } from '../knowledge-service-plugin.js'; + +interface RealtimeLog { + subscribed: string[]; + unsubscribed: string[]; +} + +/** + * Publishes a minimal `realtime` service. A plugin rather than a bare + * `registerService` call because the subscription is only taken out if the + * service is resolvable at `kernel:ready`, which is the kernel's business. + */ +class FakeRealtimePlugin implements Plugin { + name = 'test.fake.realtime'; + version = '1.0.0'; + type = 'standard'; + + constructor(private readonly log: RealtimeLog) {} + + async init(ctx: PluginContext): Promise { + ctx.registerService('realtime', { + publish: async () => undefined, + subscribe: async (channel: string, _handler: RealtimeEventHandler) => { + this.log.subscribed.push(channel); + return 'sub-1'; + }, + unsubscribe: async (id: string) => { this.log.unsubscribed.push(id); }, + }); + } +} + +async function bootKnowledgeKernel(log: RealtimeLog) { + const kernel = new LiteKernel(); + kernel.use(new FakeRealtimePlugin(log)); + const plugin = new KnowledgeServicePlugin(); + kernel.use(plugin); + await kernel.bootstrap(); + return { kernel, plugin }; +} + +describe('#10371 KnowledgeServicePlugin releases its realtime subscription on kernel shutdown', () => { + it('unsubscribes from knowledge-event-sync once shutdown() has resolved', async () => { + const log: RealtimeLog = { subscribed: [], unsubscribed: [] }; + const { kernel } = await bootKnowledgeKernel(log); + + // POSITIVE CONTROL — the subscription really was taken out, so the + // assertion below measures a release and not an absence. + expect(log.subscribed).toContain('knowledge-event-sync'); + expect(log.unsubscribed).toEqual([]); + + await kernel.shutdown(); + + // THE PIN. Before the fix the unsubscribe lived in `stop()`, which the + // kernel never called. + expect(log.unsubscribed).toEqual(['sub-1']); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + // The alias exists precisely because an embedder may have learned to call + // it BECAUSE the kernel never did. Pinning only the shutdown direction + // would go green on an implementation that simply deletes `stop()`. + const log: RealtimeLog = { subscribed: [], unsubscribed: [] }; + const { plugin } = await bootKnowledgeKernel(log); + + expect(log.subscribed).toContain('knowledge-event-sync'); + expect(log.unsubscribed).toEqual([]); + + await plugin.stop(); + + expect(log.unsubscribed).toEqual(['sub-1']); + }); + + it('a teardown on a plugin that never started is a no-op rather than a throw', async () => { + const plugin = new KnowledgeServicePlugin(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.stop()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/services/service-knowledge/src/knowledge-service-plugin.ts b/packages/services/service-knowledge/src/knowledge-service-plugin.ts index 1de849e158..2588e1132b 100644 --- a/packages/services/service-knowledge/src/knowledge-service-plugin.ts +++ b/packages/services/service-knowledge/src/knowledge-service-plugin.ts @@ -70,6 +70,15 @@ export class KnowledgeServicePlugin implements Plugin { private service: KnowledgeService | null = null; private subscriptionId: string | undefined; + /** + * Captured where the subscription is taken out, not resolved at teardown: + * `destroy()` — the hook the kernel actually calls — takes NO + * `PluginContext` (`Plugin.destroy?(): Promise | void`, core + * `types.ts`). Holding the very service the subscription was placed with also + * means the unsubscribe cannot miss it because the registry has already been + * torn down around us. + */ + private realtime: IRealtimeService | undefined; private logger: KnowledgeLogger | undefined; constructor(private readonly options: KnowledgeServicePluginOptions = {}) {} @@ -141,6 +150,7 @@ export class KnowledgeServicePlugin implements Plugin { return; } + this.realtime = realtime; this.subscriptionId = await realtime.subscribe('knowledge-event-sync', async (event) => { const object = event.object; if (!object) return; @@ -293,14 +303,42 @@ export class KnowledgeServicePlugin implements Plugin { ); } - async stop(ctx: PluginContext): Promise { + /** + * The kernel's teardown hook (`Plugin.destroy?()`, core `types.ts`) — the + * ONLY teardown entry point `ObjectKernel.performShutdown()` and + * `LiteKernel.destroy()` invoke. + * + * [#10371] IT USED TO BE `stop()`, WHICH NOTHING CALLED. `Plugin` declares + * `init()`, `start?()` and `destroy?()` and no `stop()`, so the kernel walked + * past this plugin at shutdown and the `knowledge-event-sync` realtime + * subscription outlived the kernel that created it. `start()` IS on the + * interface, so the pair read as symmetric in review — that asymmetry is what + * let the same shape survive in six packages at once. + * + * No timer here, so this member never cost a merge-queue eviction the way the + * `plugin-reports` / `service-messaging` members did (#9371). The class is + * the same one either way: a teardown the kernel does not reach. + */ + async destroy(): Promise { if (!this.subscriptionId) return; try { - const realtime = ctx.getService('realtime'); - await realtime.unsubscribe(this.subscriptionId); + await this.realtime?.unsubscribe(this.subscriptionId); } catch { // best-effort } this.subscriptionId = undefined; + this.realtime = undefined; + } + + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class, and removing it would break an embedder who learned to call + * it directly precisely BECAUSE the kernel never did. The parameter is now + * optional and ignored: `destroy()` takes no context, so teardown uses the + * realtime service captured when the subscription was taken out. Prefer + * kernel shutdown; direct callers keep working unchanged. + */ + async stop(_ctx?: PluginContext): Promise { + await this.destroy(); } } diff --git a/scripts/check-plugin-teardown-shape.mjs b/scripts/check-plugin-teardown-shape.mjs index 6858b6f977..3179730fbb 100644 --- a/scripts/check-plugin-teardown-shape.mjs +++ b/scripts/check-plugin-teardown-shape.mjs @@ -233,17 +233,11 @@ const POSITIVE_CONTROL = { * `EmailServicePlugin` / `WebhookOutboxPlugin` spell it `dispose`. */ const KNOWN_TEARDOWN_UNREACHED = [ - { file: 'packages/connectors/connector-openapi/src/connector-openapi-plugin.ts', cls: 'ConnectorOpenApiPlugin', alias: 'stop', repair: '#10371' }, - { file: 'packages/connectors/connector-rest/src/connector-rest-plugin.ts', cls: 'ConnectorRestPlugin', alias: 'stop', repair: '#10371' }, - { file: 'packages/connectors/connector-slack/src/connector-slack-plugin.ts', cls: 'ConnectorSlackPlugin', alias: 'stop', repair: '#10371' }, { file: 'packages/metadata/src/plugin.ts', cls: 'MetadataPlugin', alias: 'stop', repair: '#10371' }, - { file: 'packages/plugins/plugin-approvals/src/approvals-plugin.ts', cls: 'ApprovalsServicePlugin', alias: 'stop', repair: '#10371' }, { file: 'packages/plugins/plugin-email/src/email-plugin.ts', cls: 'EmailServicePlugin', alias: 'dispose', repair: '#10371' }, - { file: 'packages/plugins/plugin-reports/src/reports-plugin.ts', cls: 'ReportsServicePlugin', alias: 'stop', repair: '#10371' }, { file: 'packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts', cls: 'WebhookOutboxPlugin', alias: 'dispose', repair: '#10371' }, { file: 'packages/runtime/src/app-plugin.ts', cls: 'AppPlugin', alias: 'stop', repair: '#10371' }, { file: 'packages/runtime/src/external-validation-plugin.ts', cls: 'ExternalValidationPlugin', alias: 'stop', repair: '#10371' }, - { file: 'packages/services/service-knowledge/src/knowledge-service-plugin.ts', cls: 'KnowledgeServicePlugin', alias: 'stop', repair: '#10371' }, ]; // ---------------------------------------------------------------------------