diff --git a/.changeset/plugin-teardown-reached-by-kernel.md b/.changeset/plugin-teardown-reached-by-kernel.md new file mode 100644 index 0000000000..3bc89ff57a --- /dev/null +++ b/.changeset/plugin-teardown-reached-by-kernel.md @@ -0,0 +1,47 @@ +--- +"@objectstack/metadata": patch +"@objectstack/runtime": patch +"@objectstack/plugin-email": patch +"@objectstack/plugin-webhooks": patch +--- + +Five `Plugin` implementations now release their resources from `destroy()`, the +only teardown hook the kernel calls (#10772). + +`Plugin` (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and +`destroy?()`. `ObjectKernel.performShutdown()` and `LiteKernel.destroy()` walk +the plugins in reverse calling `plugin.destroy()` — and nothing anywhere calls +`stop()`, `dispose()`, `close()` or `shutdown()` on a plugin. Each of these five +spelled its teardown with one of those names instead, so what it released was +still held after `await kernel.shutdown()` had **resolved**: + +| package | class | was spelled | what outlived shutdown | +|:--|:--|:--|:--| +| `@objectstack/metadata` | `MetadataPlugin` | `stop` (arrow property) | artifact watcher, `manager.dispose()`, repository handle | +| `@objectstack/runtime` | `AppPlugin` | `stop` (arrow property) | the `app:unregistered` catalog event, never emitted | +| `@objectstack/runtime` | `ExternalValidationPlugin` | `stop` (arrow property) | every armed drift-check `setInterval` | +| `@objectstack/plugin-email` | `EmailServicePlugin` | `dispose` | two metadata subscriptions, the SMTP transport, an engine binding | +| `@objectstack/plugin-webhooks` | `WebhookOutboxPlugin` | `dispose` | the auto-enqueuer (2 realtime subscriptions + a refresh interval) and two engine hooks | + +`ExternalValidationPlugin` is the one with teeth: it is one of only two `Plugin` +implementations in the tree that own `setInterval` directly, it is mounted on +the real `os serve` path, and its `stop()`'s only caller anywhere was the class +itself re-arming. Measured against a real kernel, its drift checker performed +five further reads in the five intervals after a resolved shutdown — the #9371 +mechanism verbatim. `WebhookOutboxPlugin.dispose()` had **zero** callers in the +entire repo, so its teardown had never run in any process at all. + +**Nothing is removed and no signature narrows.** Each old name is retained 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. +`stop` stays an arrow property where it was one (so a detached +`const { stop } = plugin` keeps working) and stays synchronous on +`ExternalValidationPlugin` (so a non-awaiting call site is unaffected). The two +`stop(ctx)` aliases widen their parameter to optional. + +One behavioural note for direct callers, since `destroy()` takes no context: +`MetadataPlugin.stop(ctx)` and `AppPlugin.stop(ctx)` now use the context +captured in `init()` and ignore the argument. In a real composition these are +the same object. The visible difference is confined to a plugin whose `init()` +never ran — for `MetadataPlugin` a dropped `warn` line, for `AppPlugin` a +catalog event that is no longer emitted for an app that was never registered. diff --git a/packages/metadata/src/plugin-shutdown-releases-repository.test.ts b/packages/metadata/src/plugin-shutdown-releases-repository.test.ts new file mode 100644 index 0000000000..9e49783a51 --- /dev/null +++ b/packages/metadata/src/plugin-shutdown-releases-repository.test.ts @@ -0,0 +1,175 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10772] `await kernel.shutdown()` must actually reach `MetadataPlugin`'s + * teardown. + * + * THE DEFECT. `MetadataPlugin.start()` attaches a real `FileSystemRepository` + * (an armed chokidar watcher plus a reconciliation sweep), hands it to the + * `NodeMetadataManager`, and may attach an artifact file watcher on top. The + * teardown that closed all three was spelled `stop = async (ctx) => …`. + * `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()` on a plugin. + * + * WHY THE #10371 CENSUS MISSED IT. The alias is an arrow PROPERTY, not a + * method, so a method-only reading of the class does not see it at all. That + * is the whole reason this member — and `AppPlugin` and + * `ExternalValidationPlugin` — were absent from an enumeration that was + * otherwise careful. + * + * WHY THE ASSERTIONS ARE 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 these drive a real `LiteKernel` through a real bootstrap and a real + * shutdown, and read a real `FileSystemRepository` handle. + * + * EVERY PRE-SHUTDOWN LEG IS A POSITIVE CONTROL and load-bearing: without it a + * plugin that never attached a repository would satisfy the post-shutdown + * assertion vacuously. + * + * THE `stop()` LEG IS THE OTHER DIRECTION, and it is 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()`. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { LiteKernel } from '@objectstack/core'; +import type { PluginContext } from '@objectstack/core'; +import { MetadataPlugin } from './plugin.js'; + +const temps: string[] = []; + +function tempRoot(): string { + const dir = mkdtempSync(join(tmpdir(), 'metadata-plugin-teardown-')); + temps.push(dir); + return dir; +} + +/** Boot a real kernel carrying a real MetadataPlugin over a scratch root. */ +async function boot() { + const rootDir = tempRoot(); + const kernel = new LiteKernel(); + const plugin = new MetadataPlugin({ rootDir, watch: false }); + kernel.use(plugin); + await kernel.bootstrap(); + + const manager = kernel.getService<{ + getRepository(): { close(): Promise } | undefined; + }>('metadata'); + + return { kernel, plugin, manager, rootDir }; +} + +/** + * Count `close()` calls on the REAL repository handle the plugin attached — + * the same object the plugin holds, since `start()` assigns one instance to + * both itself and the manager. The real close still runs. + */ +function countCloses(repo: { close(): Promise }): () => number { + let closes = 0; + const real = repo.close.bind(repo); + repo.close = async () => { closes += 1; await real(); }; + return () => closes; +} + +afterEach(() => { + while (temps.length) rmSync(temps.pop()!, { recursive: true, force: true }); +}); + +describe('#10772 MetadataPlugin releases its repository on kernel shutdown', () => { + it('closes the metadata repository once shutdown() has resolved', async () => { + const { kernel, manager } = await boot(); + + // POSITIVE CONTROL — a repository really was attached, so the + // assertion below measures a release and not an absence. + const repo = manager.getRepository(); + expect(repo).toBeDefined(); + const closes = countCloses(repo!); + expect(closes()).toBe(0); + + await kernel.shutdown(); + + // THE PIN. Before the fix this stayed 0: the kernel had no `destroy()` + // to call, and `stop()` was never anybody's business. + expect(closes()).toBe(1); + }); + + it('the kernel reaches destroy() during shutdown', async () => { + const { kernel, plugin } = await boot(); + + let reached = 0; + const real = plugin.destroy; + plugin.destroy = async () => { reached += 1; await real(); }; + + // POSITIVE CONTROL — bootstrap alone must not tear the plugin down. + expect(reached).toBe(0); + + await kernel.shutdown(); + + expect(reached).toBe(1); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + const { manager, plugin } = await boot(); + + const repo = manager.getRepository(); + expect(repo).toBeDefined(); + const closes = countCloses(repo!); + + // No argument — the shape an embedder writes against a property whose + // parameter the repair made optional. + await plugin.stop(); + + expect(closes()).toBe(1); + }); + + it('the stop() alias still accepts the PluginContext argument it used to require', async () => { + const { manager, plugin } = await boot(); + + const repo = manager.getRepository(); + const closes = countCloses(repo!); + + // The pre-repair signature was `stop(ctx: PluginContext)`, required. + // An embedder holding that call shape must keep compiling AND keep + // working — the entire reason the alias was retained. + const ctx = { + logger: { info() {}, warn() {}, error() {}, debug() {} }, + } as unknown as PluginContext; + await plugin.stop(ctx); + + expect(closes()).toBe(1); + }); + + it('the alias survives being detached from the instance', async () => { + // It is an arrow PROPERTY, not a method — `const { stop } = plugin` + // is a call shape the pre-repair class supported, so the repair must + // not quietly convert it into an unbound method. + const { manager, plugin } = await boot(); + + const repo = manager.getRepository(); + const closes = countCloses(repo!); + + const { stop } = plugin; + await stop(); + + expect(closes()).toBe(1); + }); + + it('a teardown on a plugin the kernel 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 fails inside a suite, and + // the kernel calls it on every plugin it walks. + const plugin = new MetadataPlugin({ rootDir: tempRoot(), watch: false }); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.stop()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/metadata/src/plugin.ts b/packages/metadata/src/plugin.ts index dd7f5062e9..a11bf33a60 100644 --- a/packages/metadata/src/plugin.ts +++ b/packages/metadata/src/plugin.ts @@ -238,6 +238,13 @@ export class MetadataPlugin implements Plugin { private repository?: import('@objectstack/metadata-core').MetadataRepository; /** Chokidar watcher on the artifact file (local-file mode) — ADR-0008 PR-8. */ private artifactWatcher?: { close: () => Promise }; + /** + * The context handed to `init()`, retained so `destroy()` can log. + * [#10772] `Plugin.destroy()` takes NO argument — it is the kernel's only + * teardown hook — so the context the old `stop(ctx)` alias received has to + * be captured at init time instead of arriving at teardown time. + */ + private initCtx?: PluginContext; /** * The most recently parsed artifact metadata (the plural-field record: * `objects`, `views`, `data`, …). Carried on the `metadata:reloaded` @@ -283,6 +290,9 @@ export class MetadataPlugin implements Plugin { } init = async (ctx: PluginContext) => { + // [#10772] Retained for `destroy()`, which the kernel calls with no + // context. Assigned before anything that can throw. + this.initCtx = ctx; ctx.logger.info('Initializing Metadata Manager', { root: this.options.rootDir || process.cwd(), watch: this.options.watch, @@ -546,7 +556,22 @@ export class MetadataPlugin implements Plugin { } } - stop = async (ctx: PluginContext) => { + /** + * Teardown — the kernel's ONLY teardown hook. + * + * [#10772] This body used to be spelled `stop(ctx)`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` and no `stop()`, and `ObjectKernel.performShutdown()` / + * `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()` — so the artifact watcher, the manager and the + * repository were all still held after `await kernel.shutdown()` had + * RESOLVED. The `start`/`stop` pair read symmetric to a reviewer because + * `start()` really is on the interface; only one half was ever called. + * + * Idempotent: every handle is cleared as it is released, so a second + * teardown is a no-op rather than a second close. + */ + destroy = async (): Promise => { if (this.artifactWatcher) { try { await this.artifactWatcher.close(); } catch { /* noop */ } this.artifactWatcher = undefined; @@ -554,7 +579,7 @@ export class MetadataPlugin implements Plugin { try { await this.manager.dispose(); } catch (e: any) { - ctx.logger.warn('[MetadataPlugin] manager.dispose() failed', { error: e?.message }); + this.initCtx?.logger?.warn?.('[MetadataPlugin] manager.dispose() failed', { error: e?.message }); } const repo = this.repository as any; if (repo && typeof repo.close === 'function') { @@ -563,6 +588,19 @@ export class MetadataPlugin implements Plugin { this.repository = undefined; } + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class: an embedder may have learned to call it directly + * precisely BECAUSE the kernel never did, and deleting it would break them. + * Still an arrow property, so a detached `const { stop } = plugin` call + * keeps working too. The parameter is now optional and ignored — + * `destroy()` takes no context, so teardown logs through the context + * captured in `init()`. + */ + stop = async (_ctx?: PluginContext): Promise => { + await this.destroy(); + } + /** * Fetch JSON content from a URL with configurable timeout. */ diff --git a/packages/plugins/plugin-email/src/email-plugin.ts b/packages/plugins/plugin-email/src/email-plugin.ts index 3f8308ba66..5773c297bf 100644 --- a/packages/plugins/plugin-email/src/email-plugin.ts +++ b/packages/plugins/plugin-email/src/email-plugin.ts @@ -1252,7 +1252,23 @@ export class EmailServicePlugin implements Plugin { return stripReadDecorations(item); } - async dispose(): Promise { + /** + * Teardown — the kernel's ONLY teardown hook. + * + * [#10772] This body used to be spelled `dispose()`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` and no `dispose()`, and `ObjectKernel.performShutdown()` / + * `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()` — so after `await kernel.shutdown()` had RESOLVED, the + * two metadata subscriptions were still live, the SMTP transport was still + * open and the provenance hook was still bound to the engine. `dispose()` + * had exactly ONE caller in the whole repo, a test in this package; the + * kernel was never one of them. + * + * Idempotent: every handle is cleared as it is released, so a second + * teardown is a no-op rather than a second close. + */ + async destroy(): Promise { this.templateBridgeArmed = false; try { this.unsubscribeTemplates?.(); } catch { /* best effort */ } this.unsubscribeTemplates = undefined; @@ -1268,6 +1284,16 @@ export class EmailServicePlugin implements Plugin { } } + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class: an embedder may have learned to call it directly precisely + * BECAUSE the kernel never did, and deleting it would break them. Same + * signature, same return type — a direct caller sees no change. + */ + async dispose(): Promise { + await this.destroy(); + } + /** * Translate the `mail` settings namespace snapshot into a transport * and `defaultFrom`, then hot-swap them on the running EmailService. diff --git a/packages/plugins/plugin-email/src/plugin-shutdown-detaches-template-bridge.test.ts b/packages/plugins/plugin-email/src/plugin-shutdown-detaches-template-bridge.test.ts new file mode 100644 index 0000000000..47d7ef5837 --- /dev/null +++ b/packages/plugins/plugin-email/src/plugin-shutdown-detaches-template-bridge.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10772] `await kernel.shutdown()` must actually reach `EmailServicePlugin`'s + * teardown. + * + * THE DEFECT. The plugin arms a live `email_template` bridge at + * `kernel:ready`: a metadata subscription, a protocol mutation listener, a + * provenance hook bound to the data engine, and (when SMTP is configured) an + * open transport. The teardown that released all of them was spelled + * `dispose()`. `Plugin` (`@objectstack/core`'s `types.ts`) declares `init()`, + * `start?(ctx)` and `destroy?()` — and NO `dispose()` — and + * `ObjectKernel.performShutdown()` / `LiteKernel.destroy()` walk the plugins + * in reverse calling `plugin.destroy()`. Measured on the same revision: + * `dispose()` had exactly ONE caller in the whole repo, a test in this + * package. The kernel was never one of them, so after `await + * kernel.shutdown()` had RESOLVED the bridge was still armed and still + * writing. + * + * THE SPELLING. This member and `WebhookOutboxPlugin` are the `dispose()` + * half of the family — the "seventh spelling" the #10619 gate's roster was + * widened for BEFORE any instance of it was known, already present when the + * roster was measured. A census that looked only for `stop()` misses them + * entirely, which is the other half of why #10371's enumeration came out + * short. + * + * WHY THE ASSERTIONS ARE BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches. These drive a real + * `LiteKernel` through a real bootstrap and a real shutdown and then perform a + * real runtime write, reading the rows that did or did not land. + * + * EVERY PRE-SHUTDOWN LEG IS A POSITIVE CONTROL: without it a bridge that never + * armed would satisfy the post-shutdown assertion vacuously. + * + * THE `dispose()` LEG IS THE OTHER DIRECTION. The repair keeps `dispose()` as + * a delegating alias because it is public API of an exported class and an + * embedder — here, a test in this very package — calls it directly. Pinning + * only the shutdown direction would go green on an implementation that simply + * deletes `dispose()`. + */ + +import { describe, it, expect } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { EmailServicePlugin } from './email-plugin.js'; + +const TABLE = 'sys_email_template'; +type AnyRecord = Record; + +/** + * The slice of ObjectQL the template bridge and the provenance stamp touch — + * the same double `email-plugin.template-runtime-write.test.ts` uses, so this + * file cannot accept a dispatch shape the real engine would refuse. + */ +function fakeEngine() { + const rows: AnyRecord[] = []; + const matches = (row: AnyRecord, cond?: AnyRecord) => + !cond || + Object.entries(cond).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return row[k] === v; + }); + return { + rows, + _registry: { listItems: () => [] }, + async find(object: string, q?: AnyRecord) { + if (object !== TABLE) return []; + const out = rows.filter((r) => matches(r, q?.where ?? q?.filter)); + return typeof q?.limit === 'number' ? out.slice(0, q.limit) : out; + }, + async insert(_object: string, row: AnyRecord) { + rows.push({ ...row }); + return { id: row.id }; + }, + async update(_object: string, data: AnyRecord, options?: AnyRecord) { + const dispatch = assertEngineUpdateDispatch(data, options); + if (dispatch.kind !== 'by-id') throw new Error(`unexpected update dispatch: ${dispatch.kind}`); + const target = rows.find((r) => r.id === dispatch.id); + if (target) Object.assign(target, data); + return { affected: target ? 1 : 0 }; + }, + registerHook() { /* provenance stamp */ }, + unregisterHooksByPackage() { return 0; }, + }; +} + +/** Mirrors `ObjectStackProtocolImplementation`'s two registration seams. */ +function fakeProtocol() { + const projectors = new Map Promise>(); + const listeners: Array<(evt: AnyRecord) => void> = []; + const p: AnyRecord = { + projectorFailures: [] as string[], + registerMutationProjector: (type: string, fn: (evt: AnyRecord) => Promise) => { + projectors.set(type, fn); + }, + onMetadataMutation: (fn: (evt: AnyRecord) => void) => { + listeners.push(fn); + return () => { + const i = listeners.indexOf(fn); + if (i >= 0) listeners.splice(i, 1); + }; + }, + async announce(evt: AnyRecord) { + const projector = projectors.get(evt.type); + if (projector) { + // Mirrors `runMutationProjector`: a throw is caught and + // reported on the write's response, never propagated. + try { await projector(evt); } + catch (e: any) { p.projectorFailures.push(String(e?.message ?? e)); } + } + for (const l of [...listeners]) l(evt); + await new Promise((r) => setTimeout(r, 0)); + }, + /** A `PUT /api/v1/meta/email_template/:name` that landed. */ + save: (name: string, body: unknown) => + p.announce({ type: 'email_template', name, state: 'active', body }), + }; + return p; +} + +const template = () => ({ + name: 'auth.password_reset', + label: 'Password Reset', + category: 'auth', + locale: 'en-US', + subject: 'Reset your password, {{user.name}}', + bodyHtml: '

Click here

', +}); + +/** Registers the collaborators the email plugin resolves. Nothing under test. */ +class FixturePlugin implements Plugin { + name = 'com.objectstack.engine.objectql'; + type = 'standard'; + version = '1.0.0'; + providesServices = ['objectql', 'manifest', 'metadata', 'protocol']; + readonly engine = fakeEngine(); + readonly protocol = fakeProtocol(); + init(ctx: PluginContext): void { + ctx.registerService('objectql', this.engine); + ctx.registerService('manifest', { register: () => {}, list: () => [] }); + ctx.registerService('metadata', { + list: () => [], + get: async () => undefined, + subscribe: () => () => {}, + }); + ctx.registerService('protocol', this.protocol); + } +} + +async function boot() { + const kernel = new LiteKernel({ logger: { level: 'error' } }); + const fixture = new FixturePlugin(); + kernel.use(fixture); + const plugin = new EmailServicePlugin({ seedTemplates: false }); + kernel.use(plugin); + await kernel.bootstrap(); + return { kernel, plugin, fixture }; +} + +const rowsOf = (engine: AnyRecord) => + engine.rows.filter((r: AnyRecord) => r.name === 'auth.password_reset'); + +describe('#10772 EmailServicePlugin detaches its template bridge on kernel shutdown', () => { + it('stops materializing runtime writes once shutdown() has resolved', async () => { + const { kernel, fixture } = await boot(); + + // POSITIVE CONTROL — the live bridge really is armed, so the assertion + // below measures a detachment and not a bridge that never worked. + await fixture.protocol.save('auth.password_reset', template()); + expect(fixture.protocol.projectorFailures).toEqual([]); + expect(rowsOf(fixture.engine)).toHaveLength(1); + fixture.engine.rows.length = 0; + + await kernel.shutdown(); + + await fixture.protocol.save('auth.password_reset', template()); + + // THE PIN. Before the fix this wrote another row: the kernel had no + // `destroy()` to call, and `dispose()`'s only caller in the entire + // repo was a test. + expect(rowsOf(fixture.engine)).toHaveLength(0); + }); + + it('the kernel reaches destroy() during shutdown', async () => { + const { kernel, plugin } = await boot(); + + let reached = 0; + const real = plugin.destroy.bind(plugin); + plugin.destroy = async () => { reached += 1; await real(); }; + + expect(reached).toBe(0); + + await kernel.shutdown(); + + expect(reached).toBe(1); + }); + + it('the retained dispose() alias still tears down for an embedder that calls it directly', async () => { + const { plugin, fixture } = await boot(); + + await fixture.protocol.save('auth.password_reset', template()); + expect(rowsOf(fixture.engine)).toHaveLength(1); + fixture.engine.rows.length = 0; + + await plugin.dispose(); + + await fixture.protocol.save('auth.password_reset', template()); + + expect(rowsOf(fixture.engine)).toHaveLength(0); + }); + + it('a teardown on a plugin the kernel never started is a no-op rather than a throw', async () => { + // Idempotence matters because `destroy()` clears the handles it + // released; the kernel calls it on every plugin it walks. + const plugin = new EmailServicePlugin({ seedTemplates: false }); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.dispose()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-webhooks/src/plugin-shutdown-stops-auto-enqueuer.test.ts b/packages/plugins/plugin-webhooks/src/plugin-shutdown-stops-auto-enqueuer.test.ts new file mode 100644 index 0000000000..3de1913e35 --- /dev/null +++ b/packages/plugins/plugin-webhooks/src/plugin-shutdown-stops-auto-enqueuer.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10772] `await kernel.shutdown()` must actually reach `WebhookOutboxPlugin`'s + * teardown. + * + * THE DEFECT, AND HOW FAR IT WENT. At `kernel:ready` the plugin binds two + * hooks onto the data engine and starts an {@link AutoEnqueuer}, which holds + * TWO realtime subscriptions, a crypto-provider listener and a `setInterval` + * refresh timer. The teardown that released all of it was spelled `dispose()`. + * `Plugin` (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` + * and `destroy?()` — and NO `dispose()` — and `ObjectKernel.performShutdown()` + * / `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()`. + * + * Measured repo-wide on this revision: `dispose()` had ZERO callers anywhere + * — not the kernel, not a test, not an example. This teardown had therefore + * never run in any process at all, and everything above outlived every kernel + * that started it. + * + * THE SPELLING. This member and `EmailServicePlugin` are the `dispose()` half + * of the family — the seventh spelling the #10619 gate's roster was widened + * for before any instance of it was known, already present when the roster was + * measured. A census looking only for `stop()` misses both. + * + * WHY THE ASSERTIONS ARE BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches. These drive a real + * `LiteKernel` through a real bootstrap and a real shutdown and read the + * realtime service's own subscription ledger and the live timer count. + * + * EVERY PRE-SHUTDOWN LEG IS A POSITIVE CONTROL: without it a plugin that + * subscribed to nothing would satisfy the post-shutdown assertion vacuously. + * + * THE `dispose()` LEG IS THE OTHER DIRECTION. The repair keeps `dispose()` 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 `dispose()` — which, given the zero-caller + * census above, is exactly the shortcut this file exists to refuse. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { WebhookOutboxPlugin } from './webhook-outbox-plugin.js'; + +type AnyRecord = Record; + +/** Realtime double whose whole job is to record what is still subscribed. */ +function fakeRealtime() { + let next = 0; + const live = new Set(); + return { + live, + async subscribe() { + const id = `sub_${++next}`; + live.add(id); + return id; + }, + async unsubscribe(id: string) { live.delete(id); }, + async publish() { /* nothing under test */ }, + }; +} + +/** The slice of ObjectQL the enqueuer's cache refresh and the hooks touch. */ +function fakeEngine() { + return { + registry: { listItems: () => [], getObject: () => undefined }, + async find() { return []; }, + async insert(_o: string, row: AnyRecord) { return { id: 'row_1', ...row }; }, + // No `update()` / `delete()` on purpose. Nothing on the path under + // test writes through them, and a double that declares a write verb + // it never serves is a double looser than `ObjectQL` for no reason — + // which is the shape `check:engine-double-contract` exists to refuse. + registerHook() { /* provenance stamp + headers gate */ }, + unregisterHooksByPackage() { return 0; }, + }; +} + +/** + * Stands in for the plugin's declared dependency + * (`dependencies = ['com.objectstack.service.messaging']`) and registers the + * collaborators it resolves. Nothing here is under test — the plugin's own + * teardown is. + */ +class FixturePlugin implements Plugin { + name = 'com.objectstack.service.messaging'; + type = 'standard'; + version = '1.0.0'; + providesServices = ['manifest', 'objectql', 'realtime', 'messaging']; + readonly realtime = fakeRealtime(); + readonly engine = fakeEngine(); + init(ctx: PluginContext): void { + ctx.registerService('manifest', { register: () => {}, list: () => [] }); + ctx.registerService('objectql', this.engine); + ctx.registerService('realtime', this.realtime); + ctx.registerService('messaging', { + enqueueHttp: async () => ({ id: 'delivery_1' }), + isHttpDeliveryReady: () => true, + registerRedeliverGuard: () => {}, + }); + } +} + +async function boot() { + const kernel = new LiteKernel({ logger: { level: 'error' } }); + const fixture = new FixturePlugin(); + kernel.use(fixture); + const plugin = new WebhookOutboxPlugin(); + kernel.use(plugin); + await kernel.bootstrap(); + return { kernel, plugin, fixture }; +} + +beforeEach(() => { vi.useFakeTimers(); }); +afterEach(() => { vi.useRealTimers(); }); + +describe('#10772 WebhookOutboxPlugin stops its auto-enqueuer on kernel shutdown', () => { + it('releases every realtime subscription once shutdown() has resolved', async () => { + const { kernel, fixture } = await boot(); + + // POSITIVE CONTROL — the enqueuer really did subscribe, so the + // assertion below measures a release and not an absence. + expect(fixture.realtime.live.size).toBe(2); + + await kernel.shutdown(); + + // THE PIN. Before the fix these stayed live: the kernel had no + // `destroy()` to call, and `dispose()` had no caller in the repo at + // all — this teardown had never run in any process. + expect(fixture.realtime.live.size).toBe(0); + }); + + it('leaves no armed refresh interval once shutdown() has resolved', async () => { + const { kernel } = await boot(); + + // POSITIVE CONTROL — the enqueuer's periodic refresh timer is armed. + // Note for the census: this plugin owns a `setInterval` TRANSITIVELY, + // through the collaborator it constructs, so a scan of the plugin + // class's own text does not see it. + expect(vi.getTimerCount()).toBeGreaterThan(0); + + await kernel.shutdown(); + + expect(vi.getTimerCount()).toBe(0); + }); + + it('the kernel reaches destroy() during shutdown', async () => { + const { kernel, plugin } = await boot(); + + let reached = 0; + const real = plugin.destroy.bind(plugin); + plugin.destroy = async () => { reached += 1; await real(); }; + + expect(reached).toBe(0); + + await kernel.shutdown(); + + expect(reached).toBe(1); + }); + + it('the retained dispose() alias still tears down for an embedder that calls it directly', async () => { + const { plugin, fixture } = await boot(); + + expect(fixture.realtime.live.size).toBe(2); + + await plugin.dispose(); + + expect(fixture.realtime.live.size).toBe(0); + }); + + it('a teardown on a plugin the kernel never started is a no-op rather than a throw', async () => { + // Idempotence matters because `destroy()` clears the handles it + // released; the kernel calls it on every plugin it walks. + const plugin = new WebhookOutboxPlugin(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.dispose()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts index 045683afcb..c52589c393 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts @@ -169,7 +169,22 @@ export class WebhookOutboxPlugin implements Plugin { }); } - async dispose(): Promise { + /** + * Teardown — the kernel's ONLY teardown hook. + * + * [#10772] This body used to be spelled `dispose()`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` and no `dispose()`, and `ObjectKernel.performShutdown()` / + * `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()` — so after `await kernel.shutdown()` had RESOLVED the + * auto-enqueuer was still running and both engine hooks were still bound. + * Measured on the same revision: `dispose()` had ZERO callers anywhere in + * the repo, so this teardown had never run in any process at all. + * + * Idempotent: `boundEngine` is cleared as it is unbound, so a second + * teardown is a no-op rather than a second unbind. + */ + async destroy(): Promise { await this.autoEnqueuer?.stop(); if (this.boundEngine) { try { unbindWebhookProvenanceStamp(this.boundEngine); } catch { /* best effort */ } @@ -178,6 +193,16 @@ export class WebhookOutboxPlugin implements Plugin { } } + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class: an embedder may have learned to call it directly + * precisely BECAUSE the kernel never did, and deleting it would break them. + * Same signature, same return type — a direct caller sees no change. + */ + async dispose(): Promise { + await this.destroy(); + } + private getMessaging(ctx: PluginContext): MessagingHttpSurface | undefined { const svc = this.tryGetService(ctx, ['messaging']); return svc && typeof svc.enqueueHttp === 'function' ? svc : undefined; diff --git a/packages/plugins/plugin-webhooks/vitest.config.ts b/packages/plugins/plugin-webhooks/vitest.config.ts new file mode 100644 index 0000000000..1851931a9f --- /dev/null +++ b/packages/plugins/plugin-webhooks/vitest.config.ts @@ -0,0 +1,37 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineConfig } from 'vitest/config'; +import path from 'node:path'; + +/** + * [#10772] This package had no vitest config until + * `plugin-shutdown-stops-auto-enqueuer.test.ts` needed a REAL kernel to prove + * that `await kernel.shutdown()` reaches `WebhookOutboxPlugin.destroy()`. That + * is the package's first VALUE import of `@objectstack/core` — the plugin's own + * `import type { Plugin, PluginContext }` is erased before resolution and so was + * never a hazard — and an unaliased value import resolves through `exports` to + * `core/dist`, which would make the verdict a function of build state rather + * than of the source in the checkout. + * + * `pnpm check:test-source-alias` reds on exactly that and names this remedy: + * alias it to source rather than widen `KNOWN_UNALIASED_TEST_IMPORTS`, which is + * shrink-only. + * + * ANCHORED regex, array form, deliberately. A bare string `find` matches by + * PREFIX, so with a FILE replacement it would also swallow the published + * `@objectstack/core/logger` subpath and resolve it to + * `…/core/src/index.ts/logger` — `ENOTDIR`, at run time, from a config that + * reads as correct. Anchoring leaves that subpath to `exports`, where it + * belongs. + * + * `test` is deliberately left unset: this package's `test` script is a bare + * `vitest run` and was relying on the defaults, so declaring any of them here + * would silently narrow what the suite collects. + */ +export default defineConfig({ + resolve: { + alias: [ + { find: /^@objectstack\/core$/, replacement: path.resolve(__dirname, '../../core/src/index.ts') }, + ], + }, +}); diff --git a/packages/runtime/src/app-plugin-shutdown-emits-unregistered.test.ts b/packages/runtime/src/app-plugin-shutdown-emits-unregistered.test.ts new file mode 100644 index 0000000000..c8aa540ceb --- /dev/null +++ b/packages/runtime/src/app-plugin-shutdown-emits-unregistered.test.ts @@ -0,0 +1,166 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10772] `await kernel.shutdown()` must actually reach `AppPlugin`'s + * teardown. + * + * THE DEFECT. `AppPlugin` emits `app:registered` on the kernel bus at start so + * the control plane's `AppCatalogService` can upsert the `sys_app` row, and it + * emitted the matching `app:unregistered` from a teardown spelled + * `stop = async (ctx) => …`. `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()` on a plugin, so the catalog row + * outlived every kernel that registered it. + * + * WHY THE #10371 CENSUS MISSED IT. The alias is an arrow PROPERTY, not a + * method, so a method-only reading of the class does not see it at all. + * + * 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 the whole reason this shape survived in eleven classes. + * + * WHY THE ASSERTIONS ARE BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches. These drive a real + * `LiteKernel` through a real bootstrap and a real shutdown and read the + * events that actually landed on the bus. + * + * EVERY PRE-SHUTDOWN LEG IS A POSITIVE CONTROL: without it a plugin that never + * registered anything would satisfy the post-shutdown assertion vacuously. + * + * THE `stop()` LEG IS THE OTHER DIRECTION. 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()`. + */ + +import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { ObjectQLPlugin } from '@objectstack/objectql'; +import { AppPlugin, type AppPluginProjectContext } from './app-plugin.js'; + +const PROJECT: AppPluginProjectContext = { + environmentId: 'env_1', + organizationId: 'org_1', + projectName: 'catalog-teardown', +}; + +const BUNDLE = { manifest: { id: 'demo_app', name: 'demo_app', label: 'Demo' } }; + +/** Captures the catalog events AppPlugin puts on the kernel bus. */ +class CatalogRecorderPlugin implements Plugin { + name = 'test.catalog-recorder'; + type = 'standard'; + version = '1.0.0'; + readonly events: string[] = []; + init(ctx: PluginContext): void { + ctx.hook('app:registered', () => { this.events.push('app:registered'); }); + ctx.hook('app:unregistered', () => { this.events.push('app:unregistered'); }); + } +} + +/** `emitCatalogEvent` does not await `ctx.trigger`, so let the bus settle. */ +const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); + +/** + * The REAL composition an app kernel runs: the engine (which also provides the + * `manifest` service `AppPlugin` declares it cannot degrade without) plus the + * app itself. Booting without the engine makes `start()` return before it + * reaches the `app:registered` emit — which would leave the positive control + * below unsatisfiable and the pin vacuous. + */ +async function boot() { + const kernel = new LiteKernel({ logger: { level: 'error' } }); + const recorder = new CatalogRecorderPlugin(); + kernel.use(new ObjectQLPlugin({})); + kernel.use(recorder); + const plugin = new AppPlugin(BUNDLE, PROJECT, { skipSeedData: true }); + kernel.use(plugin); + await kernel.bootstrap(); + await settle(); + return { kernel, plugin, recorder }; +} + +describe('#10772 AppPlugin emits app:unregistered on kernel shutdown', () => { + it('puts app:unregistered on the bus once shutdown() has resolved', async () => { + const { kernel, recorder } = await boot(); + + // POSITIVE CONTROL — the catalog wiring really is live, so the + // assertion below measures an emit and not a dead hook. + expect(recorder.events).toContain('app:registered'); + expect(recorder.events).not.toContain('app:unregistered'); + + await kernel.shutdown(); + await settle(); + + // THE PIN. Before the fix this never arrived: the kernel had no + // `destroy()` to call, and `stop()` was never anybody's business. + expect(recorder.events).toContain('app:unregistered'); + }); + + it('the kernel reaches destroy() during shutdown', async () => { + const { kernel, plugin } = await boot(); + + let reached = 0; + const real = plugin.destroy; + plugin.destroy = async () => { reached += 1; await real(); }; + + expect(reached).toBe(0); + + await kernel.shutdown(); + + expect(reached).toBe(1); + }); + + it('the retained stop() alias still tears down for an embedder that calls it directly', async () => { + const { plugin, recorder } = await boot(); + + expect(recorder.events).not.toContain('app:unregistered'); + + // No argument — the shape an embedder writes against a property whose + // parameter the repair made optional. + await plugin.stop(); + await settle(); + + expect(recorder.events).toContain('app:unregistered'); + }); + + it('the stop() alias still accepts the PluginContext argument it used to require', async () => { + const { plugin, recorder } = await boot(); + + // The pre-repair signature was `stop(ctx: PluginContext)`, required. + // An embedder holding that call shape must keep compiling AND keep + // working — the entire reason the alias was retained. + const ctx = { + logger: { info() {}, warn() {}, error() {}, debug() {} }, + } as unknown as PluginContext; + await plugin.stop(ctx); + await settle(); + + expect(recorder.events).toContain('app:unregistered'); + }); + + it('the alias survives being detached from the instance', async () => { + // It is an arrow PROPERTY, not a method — `const { stop } = plugin` is + // a call shape the pre-repair class supported, so the repair must not + // quietly convert it into an unbound method. + const { plugin, recorder } = await boot(); + + const { stop } = plugin; + await stop(); + await settle(); + + expect(recorder.events).toContain('app:unregistered'); + }); + + it('a teardown on a plugin the kernel never initialized is a no-op rather than a throw', async () => { + // The kernel calls `destroy()` on every plugin it walks, including one + // whose `init()` never ran because an earlier plugin threw. + const plugin = new AppPlugin(BUNDLE, PROJECT, { skipSeedData: true }); + await expect(plugin.destroy()).resolves.toBeUndefined(); + await expect(plugin.stop()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index 21cda81ea4..62c9cfb99b 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -87,6 +87,14 @@ export class AppPlugin implements Plugin { private bundle: any; private projectContext?: AppPluginProjectContext; + /** + * The context handed to `init()`, retained so `destroy()` can emit the + * `app:unregistered` catalog event. [#10772] `Plugin.destroy()` takes NO + * argument — it is the kernel's only teardown hook — so the context the + * old `stop(ctx)` alias received has to be captured at init time instead + * of arriving at teardown time. + */ + private initCtx?: PluginContext; /** When true, init/start become no-ops — env has no app payload. */ private readonly empty: boolean = false; /** @@ -176,6 +184,11 @@ export class AppPlugin implements Plugin { } init = async (ctx: PluginContext) => { + // [#10772] Retained for `destroy()`, which the kernel calls with no + // context. Assigned before anything that can throw, and before the + // empty-env early return, so teardown is armed on every path init + // takes. + this.initCtx = ctx; // Install the engine-wide default hook body runner FIRST — even for // empty envs (an empty env is exactly where a user will author their // first Studio hook). Runs in init (Phase 1) so it is in place before @@ -1419,11 +1432,41 @@ export class AppPlugin implements Plugin { }); } - stop = async (ctx: PluginContext) => { + /** + * Teardown — the kernel's ONLY teardown hook. + * + * [#10772] This body used to be spelled `stop(ctx)`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` and no `stop()`, and `ObjectKernel.performShutdown()` / + * `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()` — so `app:unregistered` was never emitted on a real + * shutdown and the control plane's `sys_app` row outlived the kernel that + * registered it. The `start`/`stop` pair read symmetric to a reviewer + * because `start()` really is on the interface; only one half was called. + * + * No-ops without a project context or a captured context, exactly as the + * alias did. + */ + destroy = async (): Promise => { + const ctx = this.initCtx; + if (!ctx) return; const sys = this.bundle.manifest || this.bundle; this.emitCatalogEvent(ctx, 'app:unregistered', sys); } + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class: an embedder may have learned to call it directly + * precisely BECAUSE the kernel never did, and deleting it would break them. + * Still an arrow property, so a detached `const { stop } = plugin` call + * keeps working too. The parameter is now optional and ignored — + * `destroy()` takes no context, so teardown uses the context captured in + * `init()`. + */ + stop = async (_ctx?: PluginContext): Promise => { + await this.destroy(); + } + /** * Emit a kernel hook so the control-plane `AppCatalogService` can * upsert / delete the corresponding `sys_app` row. Silently no-ops diff --git a/packages/runtime/src/external-validation-plugin.ts b/packages/runtime/src/external-validation-plugin.ts index 4eb27aaa43..9c370df26d 100644 --- a/packages/runtime/src/external-validation-plugin.ts +++ b/packages/runtime/src/external-validation-plugin.ts @@ -178,12 +178,47 @@ export class ExternalValidationPlugin implements Plugin { }); }; - /** Tear down background drift-check timers (idempotent). */ - stop = (): void => { + /** + * Tear down background drift-check timers (idempotent) — the kernel's ONLY + * teardown hook. + * + * [#10772] This body used to be spelled `stop()`. `Plugin` + * (`@objectstack/core`'s `types.ts`) declares `init()`, `start?(ctx)` and + * `destroy?()` and no `stop()`, and `ObjectKernel.performShutdown()` / + * `LiteKernel.destroy()` walk the plugins in reverse calling + * `plugin.destroy()`. Nothing in the tree ever called `stop()` on a plugin, + * and this class's own only caller was `scheduleDriftChecks()` re-arming + * itself — so every armed `setInterval` below was STILL ARMED after + * `await kernel.shutdown()` had RESOLVED. That is #9371's mechanism + * verbatim, and this plugin is one of only two `Plugin` implementations in + * the tree that own `setInterval` at all (`ReportsServicePlugin` is the + * other, repaired under #10371). + * + * The timers are `unref`'d, so a long-lived host process still exits and + * nothing complains in production — the bill lands in a vitest worker, which + * is alive throughout teardown. + * + * Stays SYNCHRONOUS on purpose: `stop()` was `(): void`, and widening a + * public alias to `Promise` would change what an embedder's + * non-awaiting call site does. + */ + destroy = (): void => { for (const timer of this.driftTimers.values()) clearInterval(timer); this.driftTimers.clear(); }; + /** + * Retained alias for {@link destroy}. Kept because it is public API of an + * exported class: an embedder may have learned to call it directly precisely + * BECAUSE the kernel never did, and deleting it would break them. Still an + * arrow property returning `void`, so both a detached + * `const { stop } = plugin` call and a non-awaiting call site keep working + * unchanged. + */ + stop = (): void => { + this.destroy(); + }; + /** Exposed for testing; invoked from the kernel:ready handler. */ async runValidation(ctx: PluginContext): Promise { const svc = safeGet(ctx, 'external-datasource'); @@ -237,7 +272,9 @@ export class ExternalValidationPlugin implements Plugin { * don't accumulate. */ async scheduleDriftChecks(ctx: PluginContext): Promise { - this.stop(); + // [#10772] The canonical hook, not the retained alias: `destroy()` is now + // where the body lives, and the alias exists only for embedders. + this.destroy(); const metadata = safeGet(ctx, 'metadata'); if (!metadata?.list) return; diff --git a/packages/runtime/src/external-validation-shutdown-clears-timers.test.ts b/packages/runtime/src/external-validation-shutdown-clears-timers.test.ts new file mode 100644 index 0000000000..2d98b8f317 --- /dev/null +++ b/packages/runtime/src/external-validation-shutdown-clears-timers.test.ts @@ -0,0 +1,167 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#10772] `await kernel.shutdown()` must actually clear this plugin's armed + * drift-check intervals. + * + * THE DEFECT, AND WHY THIS MEMBER IS THE LOAD-BEARING ONE. + * `ExternalValidationPlugin` arms one `setInterval` per opted-in datasource + * from `scheduleDriftChecks()` at `kernel:ready` (ADR-0015 §5.2), keyed by + * name in `driftTimers`. The `clearInterval` sweep over them was spelled + * `stop = (): void => …`. `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. The + * only caller `stop()` had anywhere in the tree was this class's own + * `scheduleDriftChecks()` re-arming itself. So on kernel shutdown the + * intervals were NEVER cleared — the #9371 mechanism verbatim, in one of only + * two `Plugin` implementations in this tree that own `setInterval` at all + * (`ReportsServicePlugin` is the other, repaired under #10371). + * + * This plugin is mounted on the real serve path + * (`packages/cli/src/commands/serve.ts` — `kernel.use(createExternalValidationPlugin())`), + * so the leak is not confined to tests. + * + * WHY THE LEAK STAYED SILENT. `scheduleDriftChecks()` `unref()`s each timer, + * 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 file is over and lands in whatever the suite has already + * disconnected — which is exactly how #9371's bill arrived, as merge-queue + * evictions of runs in which every test passed. + * + * WHY THE ASSERTIONS ARE BEHAVIOURAL. `expect(plugin.destroy).toBeDefined()` + * would pass on a plugin the kernel still never reaches. These drive a real + * `LiteKernel` through a real bootstrap (so the timers are armed by the real + * `kernel:ready` path) and a real shutdown, and read the timer count and the + * drift checker's own call count. + * + * EVERY PRE-SHUTDOWN LEG IS A POSITIVE CONTROL: without it a plugin that armed + * nothing would satisfy the post-shutdown assertion vacuously. + * + * THE `stop()` LEG IS THE OTHER DIRECTION. 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()`. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { ExternalValidationPlugin } from './external-validation-plugin.js'; + +const INTERVAL_MS = 1000; + +/** Counts the drift checker's reads, so "still ticking" is measurable. */ +class FakeFederationPlugin implements Plugin { + name = 'test.federation'; + type = 'standard'; + version = '1.0.0'; + providesServices = ['external-datasource', 'metadata']; + validateAllCalls = 0; + init(ctx: PluginContext): void { + ctx.registerService('external-datasource', { + validateAll: async () => { + this.validateAllCalls += 1; + return { ok: true, results: [] }; + }, + }); + ctx.registerService('metadata', { + list: async () => [ + { name: 'warehouse', external: { validation: { checkIntervalMs: INTERVAL_MS } } }, + ], + }); + } +} + +async function boot() { + const kernel = new LiteKernel({ logger: { level: 'error' } }); + const federation = new FakeFederationPlugin(); + kernel.use(federation); + const plugin = new ExternalValidationPlugin(); + kernel.use(plugin); + await kernel.bootstrap(); + return { kernel, plugin, federation }; +} + +beforeEach(() => { vi.useFakeTimers(); }); +afterEach(() => { vi.useRealTimers(); }); + +describe('#10772 ExternalValidationPlugin clears its drift timers on kernel shutdown', () => { + it('leaves no armed interval once shutdown() has resolved', async () => { + const { kernel } = await boot(); + + // POSITIVE CONTROL — a drift timer really is armed by the real + // `kernel:ready` path, so the assertion below measures a release. + expect(vi.getTimerCount()).toBe(1); + + await kernel.shutdown(); + + // THE PIN. Before the fix this stayed 1: the kernel had no `destroy()` + // to call, and `stop()`'s only caller was this class re-arming itself. + expect(vi.getTimerCount()).toBe(0); + }); + + it('issues no further drift reads once shutdown() has resolved', async () => { + const { kernel, federation } = await boot(); + + // POSITIVE CONTROL — the armed timer really does fire and really does + // read, so a count that stops climbing means something. + await vi.advanceTimersByTimeAsync(INTERVAL_MS); + expect(federation.validateAllCalls).toBeGreaterThan(0); + + await kernel.shutdown(); + const atShutdown = federation.validateAllCalls; + + await vi.advanceTimersByTimeAsync(INTERVAL_MS * 5); + + // THE PIN. shutdown() resolving means the plugin is done reading. + // Before the fix this count kept climbing. + expect(federation.validateAllCalls).toBe(atShutdown); + }); + + it('the kernel reaches destroy() during shutdown', async () => { + const { kernel, plugin } = await boot(); + + let reached = 0; + const real = plugin.destroy; + plugin.destroy = () => { reached += 1; real(); }; + + expect(reached).toBe(0); + + await kernel.shutdown(); + + expect(reached).toBe(1); + }); + + it('the retained stop() alias still clears the timers for a direct caller', async () => { + const { plugin } = await boot(); + + expect(vi.getTimerCount()).toBe(1); + + plugin.stop(); + + expect(vi.getTimerCount()).toBe(0); + }); + + it('the alias stays SYNCHRONOUS and survives being detached from the instance', async () => { + // It was `stop = (): void =>`, an arrow property. A non-awaiting call + // site and a detached `const { stop } = plugin` are both call shapes + // the pre-repair class supported; widening the alias to a Promise, or + // converting it to an unbound method, would break them. + const { plugin } = await boot(); + + const { stop } = plugin; + const returned = stop(); + + expect(returned).toBeUndefined(); + expect(vi.getTimerCount()).toBe(0); + }); + + it('a teardown on a plugin that armed nothing is a no-op rather than a throw', async () => { + const plugin = new ExternalValidationPlugin(); + expect(() => plugin.destroy()).not.toThrow(); + expect(() => plugin.destroy()).not.toThrow(); + expect(() => plugin.stop()).not.toThrow(); + }); +}); diff --git a/scripts/check-plugin-teardown-shape.mjs b/scripts/check-plugin-teardown-shape.mjs index 3179730fbb..5e93066ba7 100644 --- a/scripts/check-plugin-teardown-shape.mjs +++ b/scripts/check-plugin-teardown-shape.mjs @@ -130,11 +130,12 @@ * * ## The known list * - * `KNOWN_TEARDOWN_UNREACHED` baselines the instances that existed when this - * gate landed. Their repair is #10371's, in the services lane; repairing them - * here would have made this PR unreviewable against its own card. The list is - * a ratchet that only shrinks -- an entry is deleted when its plugin is - * repaired, and a stale entry is itself a failure. + * `KNOWN_TEARDOWN_UNREACHED` baselined the instances that existed when this + * gate landed. It is a ratchet that only shrinks -- an entry is deleted when + * its plugin is repaired, and a stale entry is itself a failure. As of #10772 + * it is EMPTY: every baselined instance has been repaired (six under #10371, + * the remaining five under #10772), so the gate now judges the whole + * population with no exemptions at all. */ import { spawnSync } from 'node:child_process'; @@ -219,26 +220,30 @@ const POSITIVE_CONTROL = { /** * Every `Plugin` implementation that declared a teardown alias and no - * `destroy()` when this gate landed. Their repair is #10371's card, in the - * services lane. + * `destroy()` when this gate landed -- and, since #10772, NONE OF THEM. * * ⛔ SHRINK-ONLY. The list only ever shrinks: an entry is DELETED when its * plugin grows a real `destroy()`, and a stale entry fails this gate. It is * closed to new entries -- see the failure text. * - * Six of these are the instances #10371 enumerates. Five are not, and were - * derived here rather than adopted from that card: `MetadataPlugin`, - * `AppPlugin` and `ExternalValidationPlugin` spell the alias as an arrow - * PROPERTY (which a method-only reading of the class misses), and - * `EmailServicePlugin` / `WebhookOutboxPlugin` spell it `dispose`. + * The burn-down, so the empty array is a MEASURED state and not an abandoned + * one. Eleven entries landed with the gate. Six were the instances #10371 + * enumerates, and #10371 repaired them. The other five were derived here + * rather than adopted from that card, filed as #10772 and repaired there: + * `MetadataPlugin`, `AppPlugin` and `ExternalValidationPlugin` spelled the + * alias as an arrow PROPERTY (which a method-only reading of the class + * misses -- that is precisely why #10371's own enumeration was short by + * five), and `EmailServicePlugin` / `WebhookOutboxPlugin` spelled it + * `dispose` -- the "seventh spelling" this gate's roster was widened for + * before any instance of it existed, already present when the roster was + * measured. + * + * An empty ratchet is the state this gate exists to reach, not a reason to + * delete it: the roster, the population and the refusals are what keep the + * class closed for the NEXT instance, and the self-test's live-tree case + * ("neither short nor stale") is what keeps this array honest either way. */ -const KNOWN_TEARDOWN_UNREACHED = [ - { file: 'packages/metadata/src/plugin.ts', cls: 'MetadataPlugin', alias: 'stop', repair: '#10371' }, - { file: 'packages/plugins/plugin-email/src/email-plugin.ts', cls: 'EmailServicePlugin', alias: 'dispose', 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' }, -]; +const KNOWN_TEARDOWN_UNREACHED = []; // --------------------------------------------------------------------------- // The scan @@ -458,9 +463,10 @@ function main() { + `\n async ${TEARDOWN_ALIASES[0]}(): Promise { await this.${KERNEL_HOOK}(); }` + '\n' + '\n ⛔ Do not add an entry to KNOWN_TEARDOWN_UNREACHED to get past this. That' - + '\n list is shrink-only and closed to new entries: it baselines the instances' - + '\n that predate this gate, whose repair belongs to #10371, and widening it' - + '\n is not a fix -- it would reopen the class this gate exists to close.', + + '\n list is shrink-only and closed to new entries: it baselined the instances' + + '\n that predate this gate, and since #10772 it is EMPTY -- every one of them' + + '\n has been repaired. Widening it is not a fix; it would reopen the class' + + '\n this gate exists to close, and it would be the first entry back.', ); return 1; } @@ -479,10 +485,12 @@ function main() { return 1; } + const repairCards = [...new Set(KNOWN_TEARDOWN_UNREACHED.map((k) => k.repair))].sort(); console.log( `✓ check:plugin-teardown-shape: ${classes} Plugin implementation(s) across ${files} source(s) under ${POPULATION}; ` + `every teardown-shaped method (${TEARDOWN_ALIASES.join(' / ')}) sits beside a real ${KERNEL_HOOK}() ` - + `(${held} known-unreached, ⛔ SHRINK-ONLY, repair tracked on #10371).`, + + `(${held} known-unreached, ⛔ SHRINK-ONLY` + + `${repairCards.length ? `, repair tracked on ${repairCards.join(' / ')}` : ', baseline fully burned down'}).`, ); return 0; } diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 9d41f23fca..2cce7bbeb9 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1361,6 +1361,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-email/src/plugin-shutdown-detaches-template-bridge.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts", "verb": "delete",