fix(metadata): a writable datasource: loader must implement delete() (#5276) (#5652)

`MetadataLoader` declared `save?` and no `delete`, so `capabilities.write` meant
two different things at the two ends of an item's life: "persist into me" to
`register()`, and nothing at all to `unregister()`, which duck-typed `delete` at
the call site and silently skipped a loader that had none — then dropped the
registry entry, invalidated the list cache and announced a `deleted` event
anyway. The caller was told the delete succeeded while the row stayed in the
loader and was read straight back out by the next `list()`/`get()`, across
restarts, with nothing to retry it. declared != enforced (Prime Directive #10).

- `MetadataLoader` now declares `delete?(type, name): Promise<void>`, next to
  `save?`. The temporary `DeletableMetadataLoader` shape #5259 introduced to
  name the duck-typed hole is retired with both of its casts.
- `registerLoader()` rejects, loudly, a loader declaring `protocol:
  'datasource:'` with `capabilities.write: true` and no `delete()` method,
  naming the loader, the consequence, and both repairs (implement it, or
  declare `capabilities.write: false`). It is the sole writer of the loader
  map — the constructor's `config.loaders` funnel through it — so the
  combination cannot reach the runtime.
- `unregister()`'s `typeof ... === 'function'` guard stays as defensive code
  whose unreachability is now guaranteed by the registration gate; the comment
  says so.

Scope is exactly the combination `unregister()` acts on: `file:`/`memory:`/
`http:`/`s3:` loaders are never written to by the manager at runtime and are
not gated, and a read-only `datasource:` loader is untouched. `DatabaseLoader`,
the only `datasource:` loader in the repo, has always had `delete()` and is
unchanged.


Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/metadata-loader-delete-contract.md

@@ -0,0 +1,48 @@
+---
+"@objectstack/metadata": patch
+---
+
+fix(metadata): `capabilities.write` now means BOTH directions — a writable datasource loader must implement `delete()` (#5276)
+
+`MetadataLoader` declared `save?` and no `delete`, so `capabilities.write` meant
+two different things at the two ends of an item's life: to `register()` it meant
+"persist into me", and to `unregister()` it guaranteed nothing at all.
+`unregister()` duck-typed `delete` at the call site and, when a loader had none,
+**silently skipped it** — then dropped the registry entry, invalidated the list
+cache and announced a `deleted` event anyway. The caller (Studio/Setup, REST
+DELETE, the CLI, a package teardown) was told the delete succeeded while the row
+stayed in the loader's store, was read straight back out by the next
+`list()`/`get()`, and survived every restart with nothing to retry it.
+
+Two changes, both making the declaration binding instead of decorative:
+
+- **`MetadataLoader` now declares `delete?(type: string, name: string): Promise<void>`.**
+  The capability is stated on the contract, next to `save?`, instead of being
+  guessed at by each caller. A loader implemented against the interface can now
+  see that the method exists.
+- **`MetadataManager.registerLoader()` rejects the combination that cannot
+  honour it.** A loader declaring `protocol: 'datasource:'` **and**
+  `capabilities.write: true` **without** a `delete()` method is refused at
+  registration with an error naming the loader, the consequence, and both
+  repairs. `registerLoader()` is the sole writer of the loader map — the
+  constructor's `config.loaders` funnel through it — so the combination can no
+  longer reach the runtime and lose a deletion there.
+
+**Does this affect you?** Only if you register a custom metadata loader that
+declares `protocol: 'datasource:'` with `capabilities.write: true`. If it does
+and has no `delete()`, registration now throws where it previously succeeded and
+quietly discarded your deletions. Two ways to fix it, both stated in the error:
+
+1. implement `async delete(type: string, name: string): Promise<void>` on the
+   loader, removing the item from its store (`DatabaseLoader` in this package is
+   the reference implementation); or
+2. if the loader is genuinely read-only, declare `capabilities.write: false` — a
+   read-only `datasource:` loader registers without complaint and is never
+   written to in the first place.
+
+Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are
+unaffected in either direction: `MetadataManager` never persists to them at
+runtime, so it has no deletion of its own to take back, and they may declare
+`capabilities.write` without a `delete()` exactly as before. The one
+`datasource:` loader shipped in this package, `DatabaseLoader`, has always
+implemented `delete()` and is unchanged.

packages/metadata/src/loaders/loader-interface.ts

@@ -85,5 +85,33 @@ export interface MetadataLoader {
     data: any,
     options?: MetadataSaveOptions
   ): Promise<MetadataSaveResult>;
+
+  /**
+   * Delete a metadata item from this loader's store.
+   *
+   * [#5276] Optional on the interface, **mandatory for a `datasource:` loader
+   * that declares `capabilities.write`** — `MetadataManager.registerLoader()`
+   * refuses to register such a loader when this method is missing, so the
+   * combination "declared writable, cannot delete" never reaches the runtime.
+   *
+   * The reason it is enforced at registration rather than tolerated at the
+   * delete site: `MetadataManager.register()` persists into every writable
+   * `datasource:` loader, and `unregister()` has to take those rows back out
+   * again. A loader that can be written to but not deleted from makes every
+   * deletion a silent lie — `unregister()` would skip it, then drop the
+   * registry entry, invalidate the list cache and announce a `deleted` event,
+   * so the caller is told the delete succeeded while the row is read straight
+   * back out of this loader by the next `list()`/`get()`. `capabilities.write`
+   * therefore means *both* directions of the write, on both ends of the item's
+   * life — declared = enforced.
+   *
+   * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not
+   * gated: `MetadataManager` never writes to them at runtime, so it never has a
+   * deletion of its own to take back.
+   *
+   * @param type The metadata type
+   * @param name The item name
+   */
+  delete?(type: string, name: string): Promise<void>;
 }
 

packages/metadata/src/metadata-manager-loader-delete-contract.test.ts

@@ -0,0 +1,276 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #5276 — `capabilities.write` means BOTH directions, and registration enforces it.
+ *
+ * `MetadataLoader` declared `save?` and no `delete`, so `capabilities.write`
+ * meant two different things at the two ends of an item's life: to
+ * `register()` it meant "persist into me", and to `unregister()` it guaranteed
+ * nothing at all. `unregister()` duck-typed `delete` at the call site and, when
+ * the loader had none, **silently skipped it** — then dropped the registry
+ * entry, invalidated the list cache and announced a `deleted` event anyway. The
+ * caller was told the delete succeeded; the row stayed in the loader and was
+ * read straight back out by the next `list()`/`get()`, across restarts, with
+ * nothing to retry it. Standard declared ≠ enforced (Prime Directive #10).
+ *
+ * The fix enforces the declaration where the author is standing:
+ *   1. `MetadataLoader` now declares `delete?(type, name): Promise<void>` — the
+ *      contract states the capability instead of leaving each caller to guess;
+ *   2. `registerLoader()` REJECTS a `datasource:` loader that declares
+ *      `capabilities.write` without a `delete()` method, loudly, naming the
+ *      consequence and both ways out. `registerLoader()` is the sole writer of
+ *      the loader map (the constructor's `config.loaders` funnel through it),
+ *      so the rejected combination cannot reach the runtime at all;
+ *   3. `unregister()`'s `typeof … === 'function'` guard stays as defensive code
+ *      whose unreachability is now guaranteed by construction.
+ *
+ * What these tests pin:
+ *   1. the rejection, on both entry points (constructor config and the direct
+ *      `registerLoader()` call), including that nothing is half-registered;
+ *   2. the message is actionable — it names the loader and BOTH repairs;
+ *   3. the positive case is untouched: a writable datasource loader WITH
+ *      `delete` registers and `unregister()` really calls it;
+ *   4. the gate's scope is exactly the combination `unregister()` acts on —
+ *      a read-only `datasource:` loader and every non-`datasource:` protocol
+ *      register without a `delete`, because the manager never writes to them;
+ *   5. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the
+ *      gate unchanged.
+ */
+
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import type {
+    MetadataLoadResult,
+    MetadataLoaderContract,
+    MetadataSaveResult,
+    MetadataStats,
+} from '@objectstack/spec/system';
+import type { IDataDriver } from '@objectstack/spec/contracts';
+import { MetadataManager } from './metadata-manager.js';
+import { DatabaseLoader } from './loaders/database-loader.js';
+import type { MetadataLoader } from './loaders/loader-interface.js';
+
+const logger = vi.hoisted(() => ({
+    info: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+    debug: vi.fn(),
+}));
+
+vi.mock('@objectstack/core', () => ({
+    createLogger: () => logger,
+}));
+
+type Protocol = MetadataLoaderContract['protocol'];
+
+/**
+ * A loader whose contract is dictated per test and whose `delete` is present or
+ * absent on demand — the two axes the gate reads, and nothing else.
+ */
+function makeLoader(opts: {
+    name: string;
+    protocol: Protocol;
+    write: boolean;
+    withDelete: boolean;
+}): MetadataLoader & { deleteCalls: Array<[string, string]>; saveCalls: Array<[string, string]> } {
+    const deleteCalls: Array<[string, string]> = [];
+    const saveCalls: Array<[string, string]> = [];
+    const store = new Map<string, unknown>();
+    const key = (type: string, name: string) => `${type}/${name}`;
+
+    const loader: MetadataLoader & {
+        deleteCalls: Array<[string, string]>;
+        saveCalls: Array<[string, string]>;
+    } = {
+        contract: {
+            name: opts.name,
+            protocol: opts.protocol,
+            capabilities: { read: true, write: opts.write, watch: false, list: true },
+        },
+        deleteCalls,
+        saveCalls,
+        async load(type: string, name: string): Promise<MetadataLoadResult> {
+            const data = store.get(key(type, name));
+            return data === undefined ? { data: null } : { data };
+        },
+        async loadMany<T = unknown>(): Promise<T[]> {
+            return Array.from(store.values()) as T[];
+        },
+        async exists(type: string, name: string): Promise<boolean> {
+            return store.has(key(type, name));
+        },
+        async stat(): Promise<MetadataStats | null> {
+            return null;
+        },
+        async list(): Promise<string[]> {
+            return [];
+        },
+        async save(type: string, name: string, data: unknown): Promise<MetadataSaveResult> {
+            saveCalls.push([type, name]);
+            store.set(key(type, name), data);
+            return { success: true };
+        },
+    };
+
+    if (opts.withDelete) {
+        loader.delete = async (type: string, name: string): Promise<void> => {
+            deleteCalls.push([type, name]);
+            store.delete(key(type, name));
+        };
+    }
+
+    return loader;
+}
+
+/** Read the manager's private loader map — the thing registration writes. */
+const registeredLoaderNames = (mgr: MetadataManager): string[] =>
+    Array.from((mgr as unknown as { loaders: Map<string, unknown> }).loaders.keys());
+
+beforeEach(() => {
+    logger.info.mockClear();
+    logger.warn.mockClear();
+    logger.error.mockClear();
+    logger.debug.mockClear();
+});
+
+describe("a `datasource:` loader that declares `capabilities.write` MUST implement `delete()`", () => {
+    it('registerLoader() throws rather than accepting a loader it can never delete from', () => {
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [] });
+        const undeletable = makeLoader({
+            name: 'half_writable_store',
+            protocol: 'datasource:',
+            write: true,
+            withDelete: false,
+        });
+
+        expect(() => mgr.registerLoader(undeletable)).toThrow(/half_writable_store/);
+    });
+
+    it('…and nothing is half-registered — the rejected loader is not in the map', () => {
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [] });
+        const undeletable = makeLoader({
+            name: 'half_writable_store',
+            protocol: 'datasource:',
+            write: true,
+            withDelete: false,
+        });
+
+        expect(() => mgr.registerLoader(undeletable)).toThrow();
+        expect(registeredLoaderNames(mgr)).not.toContain('half_writable_store');
+    });
+
+    it('the constructor rejects it too — `config.loaders` is not a back door', () => {
+        const undeletable = makeLoader({
+            name: 'half_writable_store',
+            protocol: 'datasource:',
+            write: true,
+            withDelete: false,
+        });
+
+        expect(
+            () => new MetadataManager({ formats: ['json'], loaders: [undeletable] }),
+        ).toThrow(/half_writable_store/);
+    });
+
+    it('the message names the loader, the consequence, and BOTH repairs', () => {
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [] });
+        const undeletable = makeLoader({
+            name: 'half_writable_store',
+            protocol: 'datasource:',
+            write: true,
+            withDelete: false,
+        });
+
+        let message = '';
+        try {
+            mgr.registerLoader(undeletable);
+        } catch (error) {
+            message = error instanceof Error ? error.message : String(error);
+        }
+
+        // Which loader, and what it declared.
+        expect(message).toContain('half_writable_store');
+        expect(message).toContain("protocol: 'datasource:'");
+        expect(message).toContain('capabilities.write: true');
+        // The consequence: the delete is announced but never lands.
+        expect(message).toContain('`unregister()`');
+        expect(message).toContain('`deleted`');
+        // Repair A — implement it. Repair B — stop declaring the capability.
+        expect(message).toContain('delete(type: string, name: string)');
+        expect(message).toContain('capabilities.write: false');
+    });
+
+    it('the same loader WITH `delete` registers, and `unregister()` really calls it', async () => {
+        const deletable = makeLoader({
+            name: 'writable_store',
+            protocol: 'datasource:',
+            write: true,
+            withDelete: true,
+        });
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [deletable] });
+
+        expect(registeredLoaderNames(mgr)).toContain('writable_store');
+
+        await mgr.register('object', 'account', { name: 'account' });
+        expect(deletable.saveCalls).toEqual([['object', 'account']]);
+
+        await mgr.unregister('object', 'account');
+        expect(deletable.deleteCalls).toEqual([['object', 'account']]);
+        // The announced deletion is now the truth in every store.
+        expect(await mgr.get('object', 'account')).toBeUndefined();
+        expect(await deletable.exists('object', 'account')).toBe(false);
+    });
+});
+
+describe('the gate covers exactly the combination `unregister()` acts on', () => {
+    it('a read-only `datasource:` loader needs no `delete` — nothing ever writes to it', async () => {
+        const readOnly = makeLoader({
+            name: 'reporting_replica',
+            protocol: 'datasource:',
+            write: false,
+            withDelete: false,
+        });
+
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [readOnly] });
+        expect(registeredLoaderNames(mgr)).toContain('reporting_replica');
+
+        await mgr.register('object', 'account', { name: 'account' });
+        expect(readOnly.saveCalls).toEqual([]);
+        await expect(mgr.unregister('object', 'account')).resolves.toBeUndefined();
+    });
+
+    it.each<Protocol>(['file:', 'memory:', 'http:', 's3:'])(
+        'a `%s` loader may declare write without a `delete` — the manager never persists there',
+        (protocol) => {
+            const loader = makeLoader({
+                name: `loader_${protocol.replace(':', '')}`,
+                protocol,
+                write: true,
+                withDelete: false,
+            });
+
+            const mgr = new MetadataManager({ formats: ['json'], loaders: [] });
+            expect(() => mgr.registerLoader(loader)).not.toThrow();
+            expect(registeredLoaderNames(mgr)).toContain(loader.contract.name);
+        },
+    );
+});
+
+describe('regression — the real `datasource:` loader is unaffected', () => {
+    /**
+     * `DatabaseLoader` declares `datasource:` + `capabilities.write` and has
+     * implemented `delete()` all along; the gate must be a no-op for it. The
+     * driver is a stub because registration touches no storage — construction
+     * and the contract are the whole surface under test here.
+     */
+    it('DatabaseLoader registers under the gate', () => {
+        const loader = new DatabaseLoader({ driver: {} as IDataDriver });
+
+        expect(loader.contract.protocol).toBe('datasource:');
+        expect(loader.contract.capabilities.write).toBe(true);
+        expect(typeof loader.delete).toBe('function');
+
+        const mgr = new MetadataManager({ formats: ['json'], loaders: [] });
+        expect(() => mgr.registerLoader(loader)).not.toThrow();
+        expect(registeredLoaderNames(mgr)).toContain('database');
+    });
+});