fix(metadata): one owner for idx_sys_metadata_overlay_active — remove the stale-keyed second producer

.changeset/overlay-index-single-producer.md

@@ -0,0 +1,56 @@
+---
+"@objectstack/metadata": major
+---
+
+fix(metadata): remove the second, stale-keyed producer of `idx_sys_metadata_overlay_active` (#6771)
+
+**Breaking:** `addSysMetadataOverlayIndex` and its `AddSysMetadataOverlayIndexResult`
+type are removed from `@objectstack/metadata/migrations`. Nothing needs to replace
+them — see below.
+
+One index name, `idx_sys_metadata_overlay_active`, had **two** producers with
+**different** keys:
+
+| producer | key |
+|---|---|
+| `metadata-protocol`'s `ensureMetadataOverlayIndexes` (runtime, ADR-0048) | `(type, name, organization_id, COALESCE(package_id, ''))` `WHERE state = 'active'` |
+| this package's `addSysMetadataOverlayIndex` | `(type, name, organization_id, environment_id, scope)` |
+
+The second key is the pre-ADR-0048 one. `environment_id` has been retired since
+ADR-0005 (2026-05 revision) — `saveMetaItem` no longer writes it and overlay reads
+never consult it, so it is NULL on every new row, and SQL UNIQUE treats NULLs as
+DISTINCT. `scope` is not part of the current discriminator at all. Both producers
+used `IF NOT EXISTS`, so whichever ran first claimed the name and the other
+silently became a no-op — decided by boot order, not by any declaration.
+
+Measured against real SQLite before removal:
+
+- On a normal `DatabaseLoader` boot the stored DDL is
+ ``CREATE UNIQUE INDEX `idx_sys_metadata_overlay_active` on `sys_metadata` (`type`, `name`, `organization_id`, `package_id`)`` —
+ the **declared** index from `metadata-core`'s `sys-metadata.object.ts`, materialized
+ by `SqlDriver.syncDeclaredIndexes`, already holds the name with the current key.
+ `addSysMetadataOverlayIndex` therefore changed nothing, while still returning
+ `status: 'created'`.
+- In the one window where it was *not* a no-op — the table present but its declared
+ indexes not yet materialized, which the engine path hits by construction because
+ ObjectQL's startup owns the sync — it installed the **retired** key. Since
+ `syncDeclaredIndexes` skips by name, nothing ever repaired it afterwards, and
+ overlay uniqueness was left unenforced on every new row.
+
+So the function could only ever do nothing or do harm. Overlay uniqueness keeps the
+two producers that are correctly keyed and deliberate: the runtime partial,
+NULL-safe index from `metadata-protocol`, and — for stacks assembled without it —
+the coarser unrestricted UNIQUE that the declaration in `metadata-core` materializes,
+exactly as that file documents.
+
+Both call sites in `DatabaseLoader.ensureSchema()` are gone with it, and the empty
+`catch` that surrounded the engine-path one now reports per the ADR-0120 D4 shape
+(name what did not happen, point at the fix, never block the boot) instead of
+swallowing driver-resolution failures.
+
+**Migration:** if you called `addSysMetadataOverlayIndex(driver)` directly, delete
+the call. Assemble `metadata-protocol` for the partial, active-scoped index, or rely
+on the declared index that `syncSchema` already builds.
+
+<!-- adr-0087: not-required (no-migration-prescription) what is removed is a TypeScript function export, not an authored metadata surface: no metadata key, no key spelling and no stored value moves, so `objectstack migrate meta` has nothing to rewrite and the ledger has no upgrader to reach. The index itself is unchanged in the only spelling that ever reached a database from a correct producer. Measured: the export had zero call sites outside its own package across objectstack, cloud and objectui. -->
+

packages/metadata/src/loaders/database-loader.test.ts

@@ -580,11 +580,36 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => {
 
  await loader.list('object');
 
+ // The `project_id` → `environment_id` forward migration still runs; it
+ // probes the column list before touching anything.
  expect(raw).toHaveBeenCalled();
- expect(raw.mock.calls.some(([sql]) => String(sql).includes('idx_sys_metadata_overlay_active'))).toBe(
+ expect(raw.mock.calls.some(([sql]) => /table_info|information_schema/i.test(String(sql)))).toBe(
  true,
  );
  });
+
+ /**
+ * #6771 — this path is precisely where the removed producer was NOT a
+ * no-op. `syncSchema` threw "already exists" BEFORE materializing the
+ * declared indexes, so `idx_sys_metadata_overlay_active` was unclaimed and
+ * the loader's own `CREATE UNIQUE INDEX IF NOT EXISTS` won it — with the
+ * pre-ADR-0048 key `(type, name, organization_id, environment_id, scope)`.
+ * `syncDeclaredIndexes` skips by name, so nothing ever repaired it.
+ */
+ it('issues NO overlay-index DDL — this package is not a producer of that name', async () => {
+ const driver = createMockDriver();
+ driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
+ const raw = vi.fn().mockResolvedValue(undefined);
+ (driver as unknown as { raw: unknown }).raw = raw;
+ const loader = new DatabaseLoader({ driver });
+
+ await loader.list('object');
+
+ const overlayDdl = raw.mock.calls
+ .map(([sql]) => String(sql))
+ .filter((sql) => /idx_sys_metadata_overlay_active/i.test(sql));
+ expect(overlayDdl).toEqual([]);
+ });
  });
 
  it('DISTINGUISHES the two: same call site, opposite verdicts', async () => {

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

@@ -27,7 +27,6 @@ import type { MetadataLoader } from './loader-interface.js';
 import { calculateChecksum } from '../utils/metadata-history-utils.js';
 import { LRUCache } from '../utils/lru-cache.js';
 import { isMissingTableError, isSchemaAlreadyExistsError } from '../utils/schema-sync-errors.js';
-import { addSysMetadataOverlayIndex } from '../migrations/add-sys-metadata-overlay-index.js';
 import { migrateProjectIdToEnvironmentId } from '../migrations/migrate-project-id-to-environment-id.js';
 
 /**
@@ -323,9 +322,16 @@ export class DatabaseLoader implements MetadataLoader {
  // When using engine, schema sync is handled by ObjectQL startup
  if (this.engine) {
  this.schemaReady = true;
- // Best-effort: also ensure the overlay-uniqueness index.
- // The engine-managed driver may still benefit from a partial UNIQUE
- // INDEX (ADR-0005). Failures are swallowed by the migration itself.
+ // ⚠️ This loader does NOT build `idx_sys_metadata_overlay_active` (#6771).
+ // It used to, with the pre-ADR-0048 key, and because every producer uses
+ // `IF NOT EXISTS` the first one to run claimed the name for good. On this
+ // engine path nothing has synced `sys_metadata` yet, so that producer was
+ // the one most likely to win — and it installed a key the platform
+ // retired. Overlay uniqueness has exactly two owners now, both correctly
+ // keyed: `metadata-protocol`'s `ensureMetadataOverlayIndexes` (the
+ // partial, NULL-safe form) and, for stacks without it, the declaration in
+ // `metadata-core`'s `sys-metadata.object.ts` that ObjectQL's own startup
+ // materializes through `syncDeclaredIndexes`.
  try {
  const engineAny = this.engine as any;
  let driver: IDataDriver | undefined =
@@ -342,10 +348,22 @@ export class DatabaseLoader implements MetadataLoader {
  if (driver) {
  // v5.0 forward migration: project_id → environment_id (idempotent).
  await migrateProjectIdToEnvironmentId(driver).catch(() => undefined);
- await addSysMetadataOverlayIndex(driver);
  }
- } catch {
- // ignore — index is an optimization, not a correctness invariant
+ } catch (error) {
+ // ADR-0120 D4: never block the boot, never swallow it either (#6771 —
+ // this catch was empty). Resolving a raw-SQL driver off the engine is
+ // the only thing left that can throw here, and when it does the
+ // `project_id` → `environment_id` forward migration did NOT run: rows
+ // written before v5.0 keep the old column and read back as if the
+ // field were unset.
+ console.warn(
+ `[Metadata] Could not resolve a raw-SQL driver from the engine for \`${this.tableName}\` — ` +
+ `the project_id→environment_id forward migration was SKIPPED. Legacy rows (if any) keep the ` +
+ `pre-v5.0 column and read back as unset. Metadata reads and writes are otherwise unaffected. ` +
+ `Re-run it explicitly with \`migrateProjectIdToEnvironmentId(driver)\` from ` +
+ `\`@objectstack/metadata/migrations\` once the datasource is reachable.`,
+ error,
+ );
  }
  return;
  }
@@ -403,12 +421,16 @@ export class DatabaseLoader implements MetadataLoader {
  } catch {
  // ignore — migration is best-effort on bootstrap
  }
- // Apply ADR-0005 partial UNIQUE INDEX (best-effort, idempotent)
- try {
- await addSysMetadataOverlayIndex(this.driver!);
- } catch {
- // ignore — index is optimization
- }
+ // ⚠️ No overlay-index DDL is issued from here (#6771). `syncSchema` above
+ // already materialized the DECLARED `idx_sys_metadata_overlay_active` from
+ // `sys-metadata.object.ts` with the CURRENT ADR-0048 discriminator
+ // `(type, name, organization_id, package_id)`; measured on real SQLite, the
+ // producer that used to sit here found the name taken and no-opped —
+ // while still reporting `status: 'created'`. Its one non-no-op window was
+ // the benign "already exists" path above, where `syncSchema` threw before
+ // creating the declared indexes: there it installed the RETIRED key
+ // `(…, environment_id, scope)`, and `syncDeclaredIndexes` skips by name, so
+ // nothing ever repaired it. See the tombstone in `../migrations/index.ts`.
  }
 
  /**

packages/metadata/src/loaders/overlay-index-single-producer.test.ts

@@ -0,0 +1,157 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #6771 — `idx_sys_metadata_overlay_active` has ONE key, and this package is
+ * not a producer of it.
+ *
+ * The name used to have two producers with DIFFERENT keys, both issuing
+ * `CREATE … INDEX IF NOT EXISTS`, so whichever ran first claimed the name and
+ * the other silently no-opped:
+ *
+ * - `metadata-protocol`'s `ensureMetadataOverlayIndexes` — the current
+ * ADR-0048 key `(type, name, organization_id, COALESCE(package_id, ''))`
+ * `WHERE state = 'active'`;
+ * - this package's `addSysMetadataOverlayIndex` — the PRE-ADR-0048 key
+ * `(type, name, organization_id, environment_id, scope)`, where
+ * `environment_id` is retired (always NULL on new rows, and SQL UNIQUE
+ * treats NULLs as DISTINCT, so the index constrained nothing) and `scope`
+ * is not in the discriminator at all.
+ *
+ * The second one is gone. These tests pin the invariant against a REAL SQLite
+ * database — the stored DDL, not the absence of an exception — because the
+ * defect was never a throw: the losing producer returned `status: 'created'`
+ * exactly as the winning one did.
+ *
+ * ⚠️ Reverse verification — directions were predicted BEFORE running, and they
+ * are NOT uniform. Restoring the deleted producer from `origin/main` and
+ * re-running gave 3 failed / 84 passed:
+ * - **GREEN by design (predicted):** the two `syncSchema` cases. On both, the
+ * DECLARED index has already claimed the name with the CURRENT key before
+ * the old producer ever ran, so the producer was a no-op that nonetheless
+ * returned `status: 'created'`. These pin the COVERING producer — the reason
+ * deleting was safe rather than merely tidy — not the deleted one. A test
+ * asserting they go red would be fiction.
+ * - **RED (predicted):** the engine-path case, `@objectstack/metadata/
+ * migrations` exports no producer, and — in `database-loader.test.ts` —
+ * `issues NO overlay-index DDL`.
+ * - **One prediction was WRONG and is recorded rather than reshaped:** the
+ * pre-existing-table case was expected to go red on the theory that it
+ * reaches the benign "already exists" branch with the declared indexes
+ * unbuilt. A real `syncSchema` does not throw there, so it stays green. That
+ * branch is reachable only with a mock driver, and it is covered as such in
+ * `database-loader.test.ts`.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { DatabaseLoader } from './database-loader';
+import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm';
+
+const INDEX = 'idx_sys_metadata_overlay_active';
+const TABLE = 'sys_metadata';
+
+/** Read the DDL SQLite actually stored for an index, or `null` if absent. */
+async function storedDdl(driver: SqliteWasmDriver, name: string): Promise<string | null> {
+ const rows: any = await (driver as any).execute(
+ `SELECT sql FROM sqlite_master WHERE type='index' AND name='${name}'`,
+ );
+ const list = Array.isArray(rows) ? rows : (rows?.rows ?? []);
+ if (!list.length) return null;
+ return String(list[0].sql ?? '');
+}
+
+async function freshDriver(): Promise<SqliteWasmDriver> {
+ const driver = new SqliteWasmDriver({ filename: ':memory:' });
+ await driver.connect();
+ return driver;
+}
+
+describe('#6771 — sys_metadata overlay index: one key, one owner', () => {
+ it('a normal DatabaseLoader boot leaves the CURRENT ADR-0048 key in place', async () => {
+ const driver = await freshDriver();
+ const loader = new DatabaseLoader({ driver, tableName: TABLE });
+
+ await loader.list('object');
+
+ const ddl = await storedDdl(driver, INDEX);
+ expect(ddl).not.toBeNull();
+ // The current discriminator, delivered by the DECLARED index in
+ // metadata-core's `sys-metadata.object.ts` via `syncDeclaredIndexes`.
+ expect(ddl).toMatch(/package_id/);
+ // And NOT the retired one.
+ expect(ddl).not.toMatch(/environment_id/);
+ expect(ddl).not.toMatch(/\bscope\b/);
+ });
+
+ /**
+ * ⚠️ Direction, measured rather than presumed: this case is GREEN with the
+ * producer restored, and that is the point of it. It was written expecting
+ * red — the guess being that a pre-existing table sends the driver path down
+ * the benign "already exists" branch with the declared indexes unbuilt. A
+ * REAL `SqliteWasmDriver.syncSchema` does not throw there: it reconciles the
+ * existing table and materializes the declared indexes anyway. So on a real
+ * SQL driver the driver path is SELF-COVERING, which is precisely why
+ * deleting the producer is safe rather than merely tidy. What this pins is
+ * the covering producer, not the deleted one; the red-on-restore cases are
+ * the engine path below and the mock-driver case in `database-loader.test.ts`
+ * (`issues NO overlay-index DDL`), which does reach the benign branch.
+ */
+ it('syncSchema self-covers a pre-existing table with the CURRENT key', async () => {
+ const driver = await freshDriver();
+ // A table that exists but carries none of the declared indexes.
+ await (driver as any).execute(
+ `CREATE TABLE ${TABLE} (id text, type text, name text, organization_id text, ` +
+ `environment_id text, scope text, package_id text, state text)`,
+ );
+
+ const loader = new DatabaseLoader({ driver, tableName: TABLE });
+ await loader.list('object');
+
+ const ddl = await storedDdl(driver, INDEX);
+ expect(ddl).not.toBeNull();
+ expect(ddl).toMatch(/package_id/);
+ expect(ddl).not.toMatch(/environment_id/);
+ expect(ddl).not.toMatch(/\bscope\b/);
+ });
+
+ /**
+ * The engine path is the cleanest reproduction: the loader syncs no schema
+ * at all there (ObjectQL's startup owns it), so the removed producer ran
+ * against a table whose declared indexes did not exist yet and won the name
+ * outright.
+ */
+ it('never installs the retired key on the engine path either', async () => {
+ const driver = await freshDriver();
+ await (driver as any).execute(
+ `CREATE TABLE ${TABLE} (id text, type text, name text, organization_id text, ` +
+ `environment_id text, scope text, package_id text, state text)`,
+ );
+
+ // Read-only stand-in: `list()` reaches `engine.find` and nothing else, so
+ // this double declares no write verbs at all rather than declaring loose
+ // ones (`check:engine-double-contract` — a fake looser than the real
+ // `ObjectQL.update`/`.delete` is how #4434 shipped a dead route green).
+ // What the test needs from the engine is the `driver` it hands back, which
+ // is what `ensureSchema`'s engine path resolves.
+ const engine = {
+ driver,
+ find: async () => [],
+ };
+ const loader = new DatabaseLoader({ engine: engine as any, tableName: TABLE });
+ await loader.list('object');
+
+ const ddl = await storedDdl(driver, INDEX);
+ if (ddl !== null) {
+ expect(ddl).not.toMatch(/environment_id/);
+ expect(ddl).not.toMatch(/\bscope\b/);
+ }
+ });
+
+ it('`@objectstack/metadata/migrations` exports no overlay-index producer', async () => {
+ const migrations: Record<string, unknown> = await import('../migrations/index.js');
+
+ expect(Object.keys(migrations)).not.toContain('addSysMetadataOverlayIndex');
+ // Nothing else in the barrel may quietly take the job over either.
+ const overlayProducers = Object.keys(migrations).filter((k) => /overlayindex/i.test(k));
+ expect(overlayProducers).toEqual([]);
+ });
+});