perf(metadata-protocol): drop diffMetaItem's discarded historyMetaItem round trip (#8841)

* perf(metadata-protocol): drop diffMetaItem's discarded historyMetaItem round trip

`diffMetaItem` awaited a full `historyMetaItem` read, mapped it into a
`versions` array, and threw it away via `const _used = versions; void _used;`
while the real read happened a few lines below through the engine. Every
request to the routed, live `GET /api/v1/meta/:type/:name/diff` therefore
paid for two reads of `sys_metadata_history` where one is used.

The `void _used` marker is the reason this was worth a card rather than a
tidy-up: a deliberate-looking marker on a value that is not load-bearing is
what makes the next reader reason confidently from dead code.

Measured before deleting, rather than inherited from the report: the one
behaviour the call could still have provided is `historyMetaItem`'s early
return for a type that is neither `isOverlayAllowed` nor
`isRuntimeCreateAllowed`. It answers `{ events: [] }` without throwing and
without touching the engine, and `diffMetaItem` never consulted that gate for
its own output -- the five gated-shut types (`field`, `job`, `api`,
`capability`, `agent`) had their diffs served by the direct read regardless.

One measured behaviour change, toward the function's only stated intent: the
discarded call was unguarded, so a `sys_metadata_history` outage was fatal for
gated-open types while gated-shut ones fell into the `try`/`catch` below and
answered an empty diff. One outage, two answers, decided by an authorization
gate unrelated to reading history. Every type now takes the `catch`.

Refs #8798

* docs(metadata-protocol): reference #8833 for the swallowed history-outage question

---------

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

Author: hotlong

.changeset/diff-dead-history-read.md

@@ -0,0 +1,27 @@
+---
+"@objectstack/metadata-protocol": patch
+---
+
+perf(metadata-protocol): `diffMetaItem` stops awaiting a `historyMetaItem` read it discarded, halving the history round trips on the live diff endpoint (#8798)
+
+`diffMetaItem` opened by awaiting a full `historyMetaItem` read, mapped it into a
+`versions` array, and threw it away (`const _used = versions; void _used;`) while
+the read it actually uses ran a few lines below through the engine. Every request
+to the routed `GET /api/v1/meta/:type/:name/diff` paid for two reads of
+`sys_metadata_history` where one is used.
+
+Diff bodies are unchanged. The authorization gate the discarded call passed
+through never reached this function's output: `historyMetaItem`'s early return
+answers `{ events: [] }` for a type that is neither `isOverlayAllowed` nor
+`isRuntimeCreateAllowed`, without throwing and without touching the engine, and
+`diffMetaItem` reads the history rows directly — so the five gated-shut types
+(`field`, `job`, `api`, `capability`, `agent`) were already served a full diff
+regardless.
+
+One behaviour change, on the outage path only. The discarded call was unguarded,
+so an unavailable `sys_metadata_history` was fatal for gated-open types while
+gated-shut types fell into the `try`/`catch` below it and answered an empty diff
+— one outage, two answers, decided by an authorization gate unrelated to reading
+history. Every type now takes the `catch`, which is the function's only stated
+intent for that failure. Whether swallowing that outage is the right answer at
+all is tracked in #8833.

packages/metadata-protocol/src/protocol.diff-dead-history-read.test.ts

@@ -0,0 +1,274 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #8798 — `diffMetaItem` awaited a full `historyMetaItem` read and discarded it.
+ *
+ * The discarded call was marked `const _used = versions; void _used;`, which is
+ * why this got a card rather than a tidy-up: a deliberate-looking marker on a
+ * value that is not load-bearing is exactly the input that makes the next reader
+ * (human or agent) reason confidently from dead code.
+ *
+ * ## What the deletion had to prove, and what these pins are
+ *
+ * The one behaviour the dead call could still have been providing is
+ * `historyMetaItem`'s EARLY RETURN — it answers `{ events: [] }` for a type that
+ * is neither `isOverlayAllowed` nor `isRuntimeCreateAllowed`. A test that only
+ * exercised an ordinary type would prove nothing about the deletion, so the
+ * fixture below is pinned to a type that genuinely takes that early return.
+ *
+ * Measured, not inherited (`DEFAULT_METADATA_TYPE_REGISTRY`, both flags false):
+ * `field`, `job`, `api`, `capability`, `agent`. `field` is the fixture; `view`
+ * is the ordinary-type control.
+ *
+ * ⛔ `earlyReturnFixtureIsStillEarlyReturn` below is the anti-vacuity arm and is
+ * not decoration. If `field` ever gains `allowOrgOverride` or
+ * `allowRuntimeCreate`, every other assertion here silently stops covering the
+ * early-return case while staying green. That test going red is the signal to
+ * re-pick the fixture from the registry, not to delete the assertion.
+ */
+import { describe, expect, it } from 'vitest';
+import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, hashSpec } from '@objectstack/metadata-core';
+import { ObjectStackProtocolImplementation } from './index.js';
+
+/** Takes `historyMetaItem`'s early return — neither flag set in the registry. */
+const EARLY_RETURN_TYPE = 'field';
+/** Passes the same gate — the control that keeps the pins discriminating. */
+const ORDINARY_TYPE = 'view';
+
+/**
+ * Scalar equality only, and it REFUSES anything else rather than guessing.
+ *
+ * Both readers here issue flat filters: `diffMetaItem` queries
+ * `sys_metadata_history` by `{ organization_id, type, name }`, and
+ * `SysMetadataRepository.history` by the same three. No combinator ever arrives.
+ *
+ * The `throw` is the point (`check:where-matcher`). Treating a `$or` / `$and`
+ * key as an ordinary column name is that gate's shape (b): `r.$or` is
+ * `undefined`, the comparison fails, the row is silently excluded, and the suite
+ * goes green while asserting on a query nobody wrote.
+ */
+function matches(r: Record<string, unknown>, where: Record<string, unknown>): boolean {
+    for (const [k, v] of Object.entries(where)) {
+        if (k.startsWith('$')) {
+            throw new Error(
+                `stub engine: WHERE combinator '${k}' is not implemented by this double — `
+                + 'it matches scalar equality only. Implement it here rather than letting it '
+                + 'be read as a field name.',
+            );
+        }
+        if (v === undefined) continue;
+        if (r[k] !== v) return false;
+    }
+    return true;
+}
+
+/**
+ * Table-aware and READ-COUNTING. The count is the subject of this card: the
+ * defect was a second, unused read of `sys_metadata_history` per request, and
+ * a value-only assertion cannot see it — both bodies were always correct.
+ */
+function makeStubEngine(opts: { throwOnHistory?: boolean } = {}) {
+    const tables: Record<string, Array<Record<string, unknown>>> = {
+        sys_metadata: [],
+        sys_metadata_history: [],
+    };
+    const findCalls: string[] = [];
+    const engine: any = {
+        async find(table: string, o: { where: Record<string, unknown> }) {
+            findCalls.push(table);
+            if (opts.throwOnHistory && table === 'sys_metadata_history') {
+                throw new Error('history table unavailable (simulated outage)');
+            }
+            return (tables[table] ?? []).filter((r) => matches(r, o.where));
+        },
+        async findOne(table: string, o: { where: Record<string, unknown> }) {
+            return (tables[table] ?? []).find((r) => matches(r, o.where)) ?? null;
+        },
+        async insert() { return { id: 'stub' }; },
+        async update(_t: string, data: Record<string, unknown>, o: { where: Record<string, unknown> }) {
+            assertEngineUpdateDispatch(data, o);
+            return { id: null };
+        },
+        async delete(_t: string, o?: Record<string, unknown>) {
+            assertEngineDeleteDispatch(o);
+            return { deleted: 0 };
+        },
+        async transaction<T>(cb: (ctx: any, info: { owned: boolean }) => Promise<T>): Promise<T> {
+            return cb(undefined, { owned: true });
+        },
+        async syncObjectSchema() { /* no DDL in this stub */ },
+        registry: {
+            listItems: () => [],
+            isPackageDisabled: () => false,
+            getItem: () => undefined,
+            registerItem: () => {},
+            registerObject: () => {},
+            getPackage: () => undefined,
+        },
+    };
+    /** Reads of the history table only — the quantity the card is about. */
+    const historyReads = () => findCalls.filter((t) => t === 'sys_metadata_history').length;
+    return { engine, tables, findCalls, historyReads };
+}
+
+/** Two versions differing in exactly one top-level key, so the diff is unambiguous. */
+function seedTwoVersions(
+    tables: Record<string, Array<Record<string, unknown>>>,
+    type: string,
+    name: string,
+) {
+    const base = { organization_id: null, type, name };
+    [{ name, label: 'A' }, { name, label: 'B' }].forEach((body, i) => {
+        tables.sys_metadata_history!.push({
+            ...base,
+            id: `h_${i + 1}`,
+            version: i + 1,
+            event_seq: i + 1,
+            operation_type: i === 0 ? 'create' : 'update',
+            metadata: JSON.stringify(body),
+            checksum: hashSpec(body),
+            recorded_at: new Date(i + 1).toISOString(),
+        });
+    });
+}
+
+/** The diff both types must answer, byte for byte. */
+const EXPECTED_DIFF_BODY = {
+    added: [],
+    removed: [],
+    changed: [{ path: 'label', from: 'A', to: 'B' }],
+};
+
+describe('#8798 — the early-return gate never reached diffMetaItem`s output', () => {
+    it('earlyReturnFixtureIsStillEarlyReturn: `field` short-circuits BEFORE any engine read', async () => {
+        // Anti-vacuity. Rows ARE seeded, so an empty answer here can only come
+        // from the gate — and zero engine reads proves it returns before I/O
+        // rather than reading and finding nothing.
+        const { engine, tables, findCalls } = makeStubEngine();
+        seedTwoVersions(tables, EARLY_RETURN_TYPE, 'my_field');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        const res = await protocol.historyMetaItem({ type: EARLY_RETURN_TYPE, name: 'my_field' });
+
+        expect(res.events).toEqual([]);
+        expect(findCalls).toEqual([]);
+    });
+
+    it('serves a FULL diff for that same gated-shut type — the gate never gated this path', async () => {
+        // The case the dead call notionally covered. `diffMetaItem` reads the
+        // history rows through the engine directly and never consults
+        // `isOverlayAllowed`, so the type whose history endpoint refuses to
+        // answer still gets a complete, correct diff. Identical to what the
+        // pre-#8798 code returned.
+        const { engine, tables, historyReads } = makeStubEngine();
+        seedTwoVersions(tables, EARLY_RETURN_TYPE, 'my_field');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        const res: any = await protocol.diffMetaItem({
+            type: EARLY_RETURN_TYPE,
+            name: 'my_field',
+            fromVersion: 1,
+            toVersion: 2,
+        });
+
+        expect(res).toEqual({
+            type: EARLY_RETURN_TYPE,
+            name: 'my_field',
+            fromVersion: 1,
+            toVersion: 2,
+            ...EXPECTED_DIFF_BODY,
+        });
+        expect(historyReads()).toBe(1);
+    });
+
+    it('an ordinary type answers the SAME body — so the fixture choice is not doing the work', async () => {
+        const { engine, tables } = makeStubEngine();
+        seedTwoVersions(tables, ORDINARY_TYPE, 'grid');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        const res: any = await protocol.diffMetaItem({
+            type: ORDINARY_TYPE,
+            name: 'grid',
+            fromVersion: 1,
+            toVersion: 2,
+        });
+
+        expect(res).toEqual({
+            type: ORDINARY_TYPE,
+            name: 'grid',
+            fromVersion: 1,
+            toVersion: 2,
+            ...EXPECTED_DIFF_BODY,
+        });
+    });
+});
+
+describe('#8798 — one request, one read of sys_metadata_history', () => {
+    it('reads the history table exactly ONCE for a gated-open type', async () => {
+        // THE REGRESSION PIN. Red before the deletion (2 reads), green after —
+        // and the only assertion in this file that was. A reinstated
+        // `historyMetaItem` call makes this 2 again while every value
+        // assertion above stays green, which is precisely how the dead read
+        // survived unnoticed in the first place.
+        const { engine, tables, historyReads } = makeStubEngine();
+        seedTwoVersions(tables, ORDINARY_TYPE, 'grid');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        await protocol.diffMetaItem({
+            type: ORDINARY_TYPE,
+            name: 'grid',
+            fromVersion: 1,
+            toVersion: 2,
+        });
+
+        expect(historyReads()).toBe(1);
+    });
+});
+
+describe('#8798 — a history-table outage now answers the same way for every type', () => {
+    /**
+     * Pre-#8798 this pair DISAGREED, and only by accident: the discarded
+     * `historyMetaItem` call was unguarded, so an outage threw for a gated-open
+     * type, while a gated-shut type never reached the engine through that call
+     * and fell into the `try`/`catch` below it. One outage, two answers, decided
+     * by whether the type happened to pass an authorization gate that has
+     * nothing to do with reading history.
+     *
+     * These pins do not endorse swallowing the outage — that `catch` predates
+     * this card and is filed as #8833. They pin that the answer no longer
+     * depends on the type.
+     */
+    it('gated-open type falls through to an empty diff instead of throwing', async () => {
+        const { engine, tables } = makeStubEngine({ throwOnHistory: true });
+        seedTwoVersions(tables, ORDINARY_TYPE, 'grid');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        const res: any = await protocol.diffMetaItem({
+            type: ORDINARY_TYPE,
+            name: 'grid',
+            fromVersion: 1,
+            toVersion: 2,
+        });
+
+        expect(res.added).toEqual([]);
+        expect(res.removed).toEqual([]);
+        expect(res.changed).toEqual([]);
+    });
+
+    it('gated-shut type answers identically — unchanged by #8798', async () => {
+        const { engine, tables } = makeStubEngine({ throwOnHistory: true });
+        seedTwoVersions(tables, EARLY_RETURN_TYPE, 'my_field');
+        const protocol = new ObjectStackProtocolImplementation(engine);
+
+        const res: any = await protocol.diffMetaItem({
+            type: EARLY_RETURN_TYPE,
+            name: 'my_field',
+            fromVersion: 1,
+            toVersion: 2,
+        });
+
+        expect(res.added).toEqual([]);
+        expect(res.removed).toEqual([]);
+        expect(res.changed).toEqual([]);
+    });
+});

packages/metadata-protocol/src/protocol.ts

@@ -15563,17 +15563,28 @@ export class ObjectStackProtocolImplementation implements
     }> {
         const singularType = PLURAL_TO_SINGULAR[request.type] ?? request.type;
         const orgId = request.organizationId ?? null;
-        const events = (await this.historyMetaItem({
-            type: singularType,
-            name: request.name,
-            ...(orgId ? { organizationId: orgId } : {}),
-        })).events;
-        const versions = events
-            .map((ev: any) => (ev as any).version as number | undefined)
-            .filter((v): v is number => typeof v === 'number');
-        // The `historyMetaItem` MetadataEvent shape doesn't carry the
-        // per-(type,name) `version` directly — re-fetch via the repo
-        // to read the underlying history rows with their version.
+        // [#8798] Read the history rows DIRECTLY, once. `historyMetaItem`
+        // cannot serve this function: its `MetadataEvent` shape doesn't carry
+        // the per-(type,name) `version` a diff selects versions by, so its
+        // result was computed into a `versions` array and then discarded
+        // (`const _used = versions; void _used;`) while the real read happened
+        // below — a second, unused round trip over `sys_metadata_history` on
+        // every request to a routed, live endpoint.
+        //
+        // ⛔ Do not reinstate a `historyMetaItem` call here "for the
+        // authorization check". It never performed one for this path, measured
+        // both ways: its early return (`isOverlayAllowed` / `isRuntimeCreateAllowed`)
+        // answers `{ events: [] }` WITHOUT throwing and without touching the
+        // engine, so the five types that take it (`field`, `job`, `api`,
+        // `capability`, `agent`) had their diff served by the read below
+        // regardless — the gate never reached this function's output. What the
+        // discarded call did change was failure behaviour, and only by accident:
+        // being unguarded, it made a `sys_metadata_history` outage FATAL for
+        // gated-open types while the `try` below answered an empty diff for the
+        // five gated-shut ones. One outage, two answers, decided by type. The
+        // `catch` below is this function's only stated intent for that failure,
+        // so removing the call makes every type take it. Pinned in
+        // `protocol.diff-dead-history-read.test.ts`.
         const repo = this.getOverlayRepo(orgId);
         const fullRef = {
             type: singularType,
@@ -15665,7 +15676,6 @@ export class ObjectStackProtocolImplementation implements
                 changed: diff.changed.map((e) => ({ path: e.path, from: servedFrom[e.path], to: servedTo[e.path] })),
             };
         }
-        const _used = versions; void _used;
         return {
             type: request.type,
             name: request.name,