From 8aa5a31d720838758c3e0c39708cbce745cfb13c Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 10:27:58 +0000 Subject: [PATCH 1/4] fix(plugins,connectors,services): release plugin resources from destroy(), the hook the kernel calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Plugin` (packages/core/src/types.ts) declares `init()`, `start?(ctx)` and `destroy?()` — and no `stop()`. `ObjectKernel.performShutdown()` and `LiteKernel.destroy()` walk the plugins in reverse calling `plugin.destroy()`, so six plugins whose teardown was spelled `stop()` were never torn down: `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 moves into `destroy()`. `stop()` is retained as a delegating alias with an optional parameter, 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. No export is removed; the `Plugin` interface is untouched. `destroy()` takes no `PluginContext`, so the three members whose teardown used `ctx` now hold what they need from where they took it out: plugin-reports keeps the logger captured in `init()`, plugin-approvals keeps the `IJobService` the schedule was placed with, and service-knowledge keeps the `IRealtimeService` the subscription was taken out with. Same defect as #9371 in @objectstack/service-messaging, which surfaced as fully green runs exiting 1 on `EnvironmentTeardownError` and being evicted from the merge queue. connector-openapi gains a vitest.config.ts: `check:test-source-alias` requires the new test's `@objectstack/core` and `@objectstack/service-automation` imports to resolve to source, and its registry entry is shrink-only. Two anchored alias entries, no `test` block, so the package's existing files keep vitest defaults. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/plugin-teardown-reaches-destroy.md | 27 ++ .../src/connector-openapi-plugin.ts | 30 +- ...gin-shutdown-unregisters-connector.test.ts | 69 +++++ .../connector-openapi/vitest.config.ts | 59 ++++ .../src/connector-rest-plugin.ts | 30 +- ...gin-shutdown-unregisters-connector.test.ts | 68 +++++ .../src/connector-slack-plugin.ts | 30 +- ...gin-shutdown-unregisters-connector.test.ts | 61 ++++ .../plugin-approvals/src/approvals-plugin.ts | 45 ++- ...in-shutdown-cancels-escalation-job.test.ts | 143 ++++++++++ ...lugin-shutdown-releases-dispatcher.test.ts | 268 ++++++++++++++++++ .../plugin-reports/src/reports-plugin.ts | 58 +++- .../plugin-shutdown-unsubscribes.test.ts | 107 +++++++ .../src/knowledge-service-plugin.ts | 44 ++- 14 files changed, 1028 insertions(+), 11 deletions(-) create mode 100644 .changeset/plugin-teardown-reaches-destroy.md create mode 100644 packages/connectors/connector-openapi/src/plugin-shutdown-unregisters-connector.test.ts create mode 100644 packages/connectors/connector-openapi/vitest.config.ts create mode 100644 packages/connectors/connector-rest/src/plugin-shutdown-unregisters-connector.test.ts create mode 100644 packages/connectors/connector-slack/src/plugin-shutdown-unregisters-connector.test.ts create mode 100644 packages/plugins/plugin-approvals/src/plugin-shutdown-cancels-escalation-job.test.ts create mode 100644 packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts create mode 100644 packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts 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..196d0e1aaf --- /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({ logLevel: '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..d7f9536057 --- /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({ logLevel: 'silent' } as any); + 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({ logLevel: '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..74ea20ac49 --- /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'; + +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(); } } From ffa08d97fe5506ee224ebc71c356560779538882 Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 10:40:03 +0000 Subject: [PATCH 2/4] test(reports,approvals): configure the pinned kernels through ObjectKernelConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `logLevel` is not a member of `ObjectKernelConfig` / `LiteKernel`'s config — both take `logger?: Partial`. `tsc --noEmit` reads these test files, so the wrong spelling was a TS2353, and the `as any` on the LiteKernel call was hiding the same mistake rather than typing anything. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/plugin-shutdown-cancels-escalation-job.test.ts | 2 +- .../src/plugin-shutdown-releases-dispatcher.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 index 196d0e1aaf..247fbb660d 100644 --- 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 @@ -74,7 +74,7 @@ class FakeJobServicePlugin implements Plugin { } async function bootApprovalsKernel(log: JobLog) { - const kernel = new ObjectKernel({ logLevel: 'silent' }); + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); openKernels.push(kernel); await kernel.use(new ObjectQLPlugin()); 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 index d7f9536057..800473ea17 100644 --- a/packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts +++ b/packages/plugins/plugin-reports/src/plugin-shutdown-releases-dispatcher.test.ts @@ -161,7 +161,7 @@ async function attachSqlite(objectql: any): Promise { * docblock names as the reason the `setInterval` path exists. */ async function bootReportsLiteKernel(): Promise { - const kernel = new LiteKernel({ logLevel: 'silent' } as any); + const kernel = new LiteKernel({ logger: { level: 'silent' } }); openKernels.push(kernel); kernel.use(new ObjectQLPlugin()); @@ -174,7 +174,7 @@ async function bootReportsLiteKernel(): Promise { } async function bootReportsKernel(extra?: Plugin): Promise { - const kernel = new ObjectKernel({ logLevel: 'silent' }); + const kernel = new ObjectKernel({ logger: { level: 'silent' } }); openKernels.push(kernel); await kernel.use(new ObjectQLPlugin()); From c846181d33d067879ecbb58187f5d210787ba97f Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 11:00:49 +0000 Subject: [PATCH 3/4] test(knowledge): give the pin's relative import its explicit .js extension `tsconfig.json` here is `include: ["src"]` under NodeNext, so this file is in the package's tsc program, and an extension-less relative import is a TS2835. `@objectstack/service-knowledge` carries a frozen 10-error DEBT entry that `check:type-check-debt --re-measure` re-measures, so the new error would have taken it to 11 and failed a shrink-only ratchet. Measured back at exactly 10 after this change. The neighbouring test file spells it the same extension-less way and is one of the two ledgered TS2835s there; copying it was the mistake. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/__tests__/plugin-shutdown-unsubscribes.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 74ea20ac49..3b147f2f36 100644 --- a/packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts +++ b/packages/services/service-knowledge/src/__tests__/plugin-shutdown-unsubscribes.test.ts @@ -27,7 +27,7 @@ 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'; +import { KnowledgeServicePlugin } from '../knowledge-service-plugin.js'; interface RealtimeLog { subscribed: string[]; From d06b457004a1c3ed87e071374becc5ae0c47c6da Mon Sep 17 00:00:00 2001 From: os-warren Date: Fri, 21 Aug 2026 11:41:36 +0000 Subject: [PATCH 4/4] chore(tooling): drop this card's six entries from KNOWN_TEARDOWN_UNREACHED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/check-plugin-teardown-shape.mjs` (#10619) landed on main after this branch was cut. Its baseline is shrink-only and a stale entry is itself a failure, so the six plugins repaired here would have reddened the gate on main the moment this merged — the merge queue would more likely have evicted this PR first, rebuilding the queue for everyone behind it. That is the #9371 blast-radius shape, on a PR whose subject is #9371's defect class. Which six was taken from the gate's own failure output, not from line numbers: run before the edit, it named exactly these six as `no longer unreached`, and each was independently confirmed to declare a real `destroy()` in this tree. The other five entries are untouched. `MetadataPlugin`, `AppPlugin` and `ExternalValidationPlugin` spell the alias as an arrow property and `EmailServicePlugin` / `WebhookOutboxPlugin` spell it `dispose`; none has a `destroy()` yet, and they are #10772's card. Deleting them would be a false declaration that they are fixed. Held count after this change is 5, measured, not predicted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- scripts/check-plugin-teardown-shape.mjs | 6 ------ 1 file changed, 6 deletions(-) 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' }, ]; // ---------------------------------------------------------------------------