diff --git a/.changeset/overlay-index-probe-first.md b/.changeset/overlay-index-probe-first.md new file mode 100644 index 0000000000..9b2a8e022d --- /dev/null +++ b/.changeset/overlay-index-probe-first.md @@ -0,0 +1,53 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): `ensureOverlayIndex` probes before it drops, and says what it could not enforce (#6418) + +`sys_metadata`'s overlay-uniqueness migration ran **DROP then CREATE**: + +```text +DROP INDEX IF EXISTS idx_sys_metadata_overlay_active ← always succeeds +CREATE UNIQUE INDEX idx_sys_metadata_overlay_active … ← may fail +``` + +with nothing that puts the dropped index back, and both `catch` blocks empty. On +the dialects that *do* support the form (SQLite / PostgreSQL), a `CREATE` that +failed on existing rows therefore left the table with **no** unique index at all +— and no line in the log. ADR-0005 overlay uniqueness is the base of metadata +correctness: with two ACTIVE rows for one +`(type, name, organization_id, package_id)`, which one `getMetaItem` returns is +undefined. + +The degradation branch could not save it either. It fired only when the driver's +message matched `/partial|where clause|syntax/i`, which duplicate-row errors +(`UNIQUE constraint failed` / `duplicate key value`) do not — so the one failure +that is about DATA fell through to a bare `// best-effort` comment. MySQL was +safe only by accident: `DROP INDEX IF EXISTS` is not legal MySQL, so the drop +failed first and the old index survived. + +**The order is now probe-first**, ported from the sibling +`view-definition-active-index.ts` (#5839 / #6417) and extracted into a shared +`partial-index-probe.ts` both migrations use: build the partial UNIQUE under a +throwaway probe name, and only once that has demonstrably succeeded drop the +real name and rebuild it. On any dialect or dataset that cannot take the form, +whatever index was protecting the table is left exactly as it was — degraded to +yesterday's behaviour, never below it. Both sections get this treatment +(`…_overlay_active` and `…_overlay_draft`), and the two are independent so a +failure on one no longer decides the other. + +**The empty catches are replaced by ADR-0120 D4's disposition**: classify the +failure, keep the previous index, name the key that is not enforced and what +that costs, ship the exact query that lists the offending rows, point at +`os migrate plan`, and let the boot continue — reported at `error`, because what +goes missing is an integrity guarantee the platform states it enforces while +everything else keeps looking healthy. + +Two things deliberately do **not** change. The key spelling stays byte-identical +(`(type, name, organization_id, COALESCE(package_id, ''))`) — this is an +ordering and reporting fix, not a re-keying. And the dialect fallback stays a +**non-UNIQUE** composite index: one ACTIVE row and one DRAFT row for the same +key legitimately coexist on this table, so a full UNIQUE would reject legal +data. What changes about the fallback is that it is now issued additively +(`IF NOT EXISTS`, no preceding drop, so it can never replace a stronger index) +and that the report says plainly what is and is not enforced. diff --git a/packages/metadata-protocol/src/migrations/overlay-index.test.ts b/packages/metadata-protocol/src/migrations/overlay-index.test.ts new file mode 100644 index 0000000000..cb06d28b7d --- /dev/null +++ b/packages/metadata-protocol/src/migrations/overlay-index.test.ts @@ -0,0 +1,529 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { DatabaseSync } from 'node:sqlite'; + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +import { + buildOverlayDuplicateProbeSql, + buildOverlayFallbackIndexSql, + buildOverlayIndexSql, + ensureMetadataOverlayIndexes, + ensureOverlayStateIndex, + overlayIndexKeyParts, + OVERLAY_INDEX_COLUMNS, + OVERLAY_INDEX_NAMES, + OVERLAY_NULL_SENTINELS, + OVERLAY_PROBE_INDEX_NAMES, +} from './overlay-index.js'; +import type { IndexExec } from './partial-index-probe.js'; + +/** + * `sys_metadata` — overlay uniqueness survives a failed tightening (#6418). + * + * Every assertion here runs against a REAL SQLite database, because the defect + * was a claim about DDL ordering that no test ever asked a database to confirm: + * `ensureOverlayIndex` dropped the live index and then tried to build the + * partial one, so a rejected `CREATE` left the table with NO unique index at + * all — silently, since both `catch` blocks were empty. + * + * Uses Node's built-in `node:sqlite` rather than `better-sqlite3` (which the + * driver packages use), for the reason the sibling + * `view-definition-active-index.test.ts` gives: this package needs no SQL + * dependency of its own, and the built-in gives the same real SQLite — real + * partial indexes, real UNIQUE enforcement, real NULL-distinctness — for free. + */ +describe('sys_metadata overlay uniqueness (#6418)', () => { + let db: DatabaseSync; + let exec: IndexExec; + + /** + * What `SqlDriver.syncDeclaredIndexes` materializes for + * `sys-metadata.object.ts`'s declaration + * (`['type','name','organization_id','package_id']`, `unique: true`) — knex's + * form, mirrored from the sibling test's measured fixture. It is + * UNRESTRICTED (no `WHERE`) and NULL-distinct, which is exactly why the + * runtime migration exists and exactly what it must not destroy. + */ + const DECLARED_ACTIVE_INDEX_DDL = + 'CREATE UNIQUE INDEX `idx_sys_metadata_overlay_active` on `sys_metadata` ' + + '(`type`, `name`, `organization_id`, `package_id`)'; + + /** + * A draft index a pre-#6418 deployment can be carrying — state-scoped but + * NULL-distinct on `package_id`. Seeded only by the draft-section cases, + * since `sys-metadata.object.ts` declares no draft index at all and a fresh + * boot therefore has none until this migration builds it. + */ + const PRE_EXISTING_DRAFT_INDEX_DDL = + 'CREATE UNIQUE INDEX `idx_sys_metadata_overlay_draft` on `sys_metadata` ' + + "(`type`, `name`, `organization_id`, `package_id`) WHERE state = 'draft'"; + + const indexDdl = (name: string): string | undefined => + (db.prepare("SELECT sql FROM sqlite_master WHERE type='index' AND name=?").get(name) as + | { sql?: string } + | undefined)?.sql ?? undefined; + + const insert = ( + id: string, + type: string, + name: string, + org: string | null, + packageId: string | null, + state: string, + ): { ok: boolean; error?: string } => { + try { + db.prepare( + 'INSERT INTO sys_metadata (id, type, name, organization_id, package_id, state) ' + + 'VALUES (?,?,?,?,?,?)', + ).run(id, type, name, org, packageId, state); + return { ok: true }; + } catch (e) { + return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } + }; + + /** Insert bypassing every index, to seed rows the current index admits. */ + const forceInsert = ( + id: string, + type: string, + name: string, + org: string | null, + packageId: string | null, + state: string, + ): void => { + const outcome = insert(id, type, name, org, packageId, state); + if (!outcome.ok) throw new Error(`fixture insert rejected: ${outcome.error}`); + }; + + beforeEach(() => { + db = new DatabaseSync(':memory:'); + db.exec(`CREATE TABLE sys_metadata ( + id TEXT PRIMARY KEY, type TEXT, name TEXT, + organization_id TEXT, package_id TEXT, state TEXT + );`); + db.exec(DECLARED_ACTIVE_INDEX_DDL); + exec = async (sql: string) => db.exec(sql); + }); + + afterEach(() => { + db.close(); + }); + + // ── What the migration is FOR ───────────────────────────────────────── + + it('BEFORE the migration, an ACTIVE and a DRAFT row for one key cannot coexist', () => { + // The declared index is unrestricted, so it collides rows the platform + // says are legal — that is the whole reason for two state-scoped indexes. + expect(insert('a1', 'view', 'lead.list', 'org1', 'pkg1', 'active').ok).toBe(true); + const draft = insert('d1', 'view', 'lead.list', 'org1', 'pkg1', 'draft'); + expect(draft.ok).toBe(false); + expect(draft.error).toContain('UNIQUE constraint failed'); + }); + + it('AFTER the migration, the ACTIVE+DRAFT pair is legal — and stays legal', async () => { + const result = await ensureMetadataOverlayIndexes(exec); + expect(result.active.status).toBe('created'); + expect(result.draft.status).toBe('created'); + + expect(insert('a1', 'view', 'lead.list', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('d1', 'view', 'lead.list', 'org1', 'pkg1', 'draft').ok).toBe(true); + }); + + it('leaves PARTIAL UNIQUE indexes under both official names, and no probe residue', async () => { + await ensureMetadataOverlayIndexes(exec); + + const active = indexDdl(OVERLAY_INDEX_NAMES.active); + expect(active).toBeDefined(); + expect(active!.toLowerCase()).toContain('unique'); + expect(active!.toLowerCase()).toContain("where state = 'active'"); + expect(active).toContain("COALESCE(package_id, '')"); + // Reusing the DECLARED name is what stops `syncDeclaredIndexes` — which + // skips by name — from re-imposing the unrestricted form next boot. + expect(active).not.toEqual(DECLARED_ACTIVE_INDEX_DDL); + + const draft = indexDdl(OVERLAY_INDEX_NAMES.draft); + expect(draft!.toLowerCase()).toContain('unique'); + expect(draft!.toLowerCase()).toContain("where state = 'draft'"); + + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.active)).toBeUndefined(); + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.draft)).toBeUndefined(); + }); + + it('still rejects two ACTIVE rows, and two DRAFT rows, for one key', async () => { + await ensureMetadataOverlayIndexes(exec); + + expect(insert('a1', 'view', 'lead.hot', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('a2', 'view', 'lead.hot', 'org1', 'pkg1', 'active').ok).toBe(false); + + expect(insert('d1', 'view', 'lead.hot', 'org1', 'pkg1', 'draft').ok).toBe(true); + expect(insert('d2', 'view', 'lead.hot', 'org1', 'pkg1', 'draft').ok).toBe(false); + }); + + it('closes the NULL-distinct hole for package-less rows — which is why a conflict is reachable', async () => { + // BEFORE: the declared index treats the two NULL package_ids as + // distinct, so this pair is legal. This is the tightening. + expect(insert('g1', 'view', 'lead.g', 'org1', null, 'active').ok).toBe(true); + expect(insert('g2', 'view', 'lead.g', 'org1', null, 'active').ok).toBe(true); + db.exec("DELETE FROM sys_metadata WHERE id='g2'"); + + await ensureMetadataOverlayIndexes(exec); + + // AFTER: COALESCE(package_id, '') folds them into one bucket. + expect(insert('g3', 'view', 'lead.g', 'org1', null, 'active').ok).toBe(false); + // …while a real package id still gets its own overlay row. + expect(insert('g4', 'view', 'lead.g', 'org1', 'pkg1', 'active').ok).toBe(true); + }); + + it('keeps distinct types, names and organizations independent', async () => { + await ensureMetadataOverlayIndexes(exec); + + expect(insert('i1', 'view', 'lead.x', 'org1', null, 'active').ok).toBe(true); + expect(insert('i2', 'flow', 'lead.x', 'org1', null, 'active').ok).toBe(true); + expect(insert('i3', 'view', 'lead.y', 'org1', null, 'active').ok).toBe(true); + expect(insert('i4', 'view', 'lead.x', 'org2', null, 'active').ok).toBe(true); + }); + + // ── Idempotence ─────────────────────────────────────────────────────── + + it('is idempotent — a second run leaves the schema byte-identical', async () => { + const first = await ensureMetadataOverlayIndexes(exec); + const afterFirst = [indexDdl(OVERLAY_INDEX_NAMES.active), indexDdl(OVERLAY_INDEX_NAMES.draft)]; + + const second = await ensureMetadataOverlayIndexes(exec); + const afterSecond = [indexDdl(OVERLAY_INDEX_NAMES.active), indexDdl(OVERLAY_INDEX_NAMES.draft)]; + + expect(first.active.status).toBe('created'); + expect(second.active.status).toBe('created'); + expect(second.draft.status).toBe('created'); + expect(afterSecond).toEqual(afterFirst); + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.active)).toBeUndefined(); + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.draft)).toBeUndefined(); + }); + + it('converges from a table that never had either index at all', async () => { + db.exec(`DROP INDEX ${OVERLAY_INDEX_NAMES.active}`); + + const result = await ensureMetadataOverlayIndexes(exec); + + expect(result.active.status).toBe('created'); + expect(result.draft.status).toBe('created'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)!.toLowerCase()).toContain("where state = 'active'"); + }); + + // ── Degradation: the constraint is never destroyed ──────────────────── + + /** + * The nail of #6418, on a REAL database rather than a mocked throw: seed + * the duplicate pair the declared NULL-distinct index admits, run the + * migration, and assert ADR-0120 D4's whole disposition. + * + * Under the DROP-then-CREATE order this left `sys_metadata` with no unique + * index at all and nothing in the log. + */ + it('a pre-existing duplicate ACTIVE pair blocks the tightening — old index kept, rows named, boot survives', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + // Two package-less ACTIVE rows under one (type, name, org) — legal for + // the declared index because SQL UNIQUE treats NULLs as DISTINCT. + forceInsert('g1', 'view', 'lead.dupe', 'org1', null, 'active'); + forceInsert('g2', 'view', 'lead.dupe', 'org1', null, 'active'); + + const result = await ensureMetadataOverlayIndexes(exec, logger); + + // Reported, never thrown: a boot must not fail over an index. + expect(result.active.status).toBe('conflict'); + expect(result.active.detail).toContain('UNIQUE constraint failed'); + // The PREVIOUS index survives byte-for-byte and still enforces what it + // always did — at no point is the table left unconstrained. + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(insert('k1', 'view', 'lead.k', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('k2', 'view', 'lead.k', 'org1', 'pkg1', 'active').ok).toBe(false); + // No probe residue, and no half-built index under the real name. + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.active)).toBeUndefined(); + // No non-UNIQUE fallback is smuggled in on a DATA conflict — the + // fallback is the DIALECT arm's answer only. + expect(result.active.fallback).toBe('not-attempted'); + + // The two indexes are independent: the draft one still gets built. + expect(result.draft.status).toBe('created'); + + // D4's wording contract: what is not enforced, the rows, the command. + expect(logger.error).toHaveBeenCalledTimes(1); + const msg = String(logger.error.mock.calls[0]![0]); + expect(msg).toContain("COALESCE(package_id, '')"); + expect(msg).toContain('The previous index is left in place'); + expect(msg).toContain('os migrate plan'); + expect(msg).toContain(buildOverlayDuplicateProbeSql('active')); + + // …and that shipped query really does name the offenders, on this very + // database. It is not a decorative string. + const offenders = db + .prepare(buildOverlayDuplicateProbeSql('active')) + .all() as Array>; + expect(offenders).toHaveLength(1); + expect(offenders[0]!.name).toBe('lead.dupe'); + expect(offenders[0]!.duplicate_rows).toBe(2); + }); + + /** + * MariaDB shape: `CREATE INDEX IF NOT EXISTS` is understood, `WHERE` is not. + * The pre-existing UNIQUE index must survive, and the fallback statement + * must not be able to downgrade it. + */ + it('a dialect without partial indexes keeps the original index intact and never downgrades it', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const mariadbish: IndexExec = async (sql: string) => { + if (/where/i.test(sql)) { + throw new Error( + "You have an error in your SQL syntax; check the manual … near 'WHERE state = 'active''", + ); + } + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(mariadbish, 'active', logger); + + expect(result.status).toBe('unsupported'); + // The fallback statement ran and succeeded — as a NO-OP, because + // `IF NOT EXISTS` sees the declared index already holding the name. The + // stronger index is therefore still there, byte-for-byte. + expect(result.fallback).toBe('ensured'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(insert('m1', 'view', 'lead.m', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('m2', 'view', 'lead.m', 'org1', 'pkg1', 'active').ok).toBe(false); + + expect(logger.error).toHaveBeenCalledTimes(1); + const note = String(logger.error.mock.calls[0]![0]); + expect(note).toContain('NOT enforced as specified on this dialect'); + expect(note).toContain('never replaced by something weaker'); + expect(note).toContain('deliberately NOT UNIQUE'); + expect(note).toContain('SQLite/PostgreSQL'); + expect(note).toContain(buildOverlayDuplicateProbeSql('active')); + expect(logger.info).not.toHaveBeenCalled(); + }); + + /** + * MySQL proper, where the old code was safe only by ACCIDENT: it has + * neither `DROP INDEX IF EXISTS` nor `CREATE INDEX IF NOT EXISTS`, so every + * statement this migration issues is refused. Nothing may be touched, and + * the report must say the degradation target could not be built either. + */ + it('MySQL refuses every statement — nothing is touched, and the report says so', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const mysqlish: IndexExec = async (sql: string) => { + throw new Error(`You have an error in your SQL syntax near '${sql.slice(0, 24)}'`); + }; + + const result = await ensureOverlayStateIndex(mysqlish, 'active', logger); + + expect(result.status).toBe('unsupported'); + expect(result.fallback).toBe('refused'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(String(logger.error.mock.calls[0]![0])).toContain('no index was added'); + }); + + it('a conflict in MySQL wording takes the same disposition', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const conflicting: IndexExec = async (sql: string) => { + if (/CREATE UNIQUE INDEX/i.test(sql)) { + throw new Error("Duplicate entry 'view-lead.dupe-org1-' for key 'idx_sys_metadata_overlay_active'"); + } + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(conflicting, 'active', logger); + + expect(result.status).toBe('conflict'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(String(logger.error.mock.calls[0]![0])).toContain('os migrate plan'); + }); + + it('an unclassifiable failure is reported at error and leaves the index alone', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const broken: IndexExec = async (sql: string) => { + if (/CREATE UNIQUE INDEX/i.test(sql)) throw new Error('disk I/O error'); + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(broken, 'active', logger); + + expect(result.status).toBe('failed'); + expect(result.fallback).toBe('not-attempted'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)).toEqual(DECLARED_ACTIVE_INDEX_DDL); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(String(logger.error.mock.calls[0]![0])).toContain('can still coexist'); + }); + + /** + * The one branch where the real name CAN end up empty: another process + * changed the data between the probe and the rebuild. It is unreachable by + * design and reported at the loudest level anyway, because it is the only + * state in which the operator has to act now. + */ + it('a race between the probe and the rebuild is named, not swallowed', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const racing: IndexExec = async (sql: string) => { + // ⚠️ The probe name has the real name as a PREFIX, so "contains the + // real name" also matches the probe statement. Match the probe out + // explicitly or this mock fails the probe instead of the rebuild. + const isProbe = sql.includes(OVERLAY_PROBE_INDEX_NAMES.active); + if (!isProbe && sql.includes(OVERLAY_INDEX_NAMES.active) && /CREATE UNIQUE INDEX/i.test(sql)) { + throw new Error('database is locked'); + } + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(racing, 'active', logger); + + expect(result.status).toBe('failed'); + expect(logger.error).toHaveBeenCalledTimes(1); + const msg = String(logger.error.mock.calls[0]![0]); + expect(msg).toContain('after the probe succeeded'); + expect(msg).toContain('may currently have NO unique index'); + }); + + it('a host with no raw-SQL driver is a silent no-op, not a failure', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const result = await ensureMetadataOverlayIndexes(undefined, logger); + expect(result.active.status).toBe('no-driver'); + expect(result.draft.status).toBe('no-driver'); + expect(logger.error).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + // ── The DRAFT section gets the same treatment ───────────────────────── + + it('a duplicate DRAFT pair keeps the pre-existing draft index — and the active one still lands', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + // The draft index a pre-#6418 deployment can be carrying: state-scoped + // but NULL-distinct on `package_id`, so two package-less DRAFT rows + // under one (type, name, org) are legal for it and rejected by the + // `COALESCE(package_id, '')` key. Exactly the tightening that fails. + db.exec(PRE_EXISTING_DRAFT_INDEX_DDL); + forceInsert('x1', 'view', 'lead.e', 'org1', null, 'draft'); + forceInsert('x2', 'view', 'lead.e', 'org1', null, 'draft'); + const draftDdlBefore = indexDdl(OVERLAY_INDEX_NAMES.draft); + + const result = await ensureMetadataOverlayIndexes(exec, logger); + + expect(result.draft.status).toBe('conflict'); + // Byte-for-byte survival of the draft index. + expect(indexDdl(OVERLAY_INDEX_NAMES.draft)).toEqual(draftDdlBefore); + expect(indexDdl(OVERLAY_PROBE_INDEX_NAMES.draft)).toBeUndefined(); + // …and it still enforces what it always did. + expect(insert('y1', 'view', 'lead.f', 'org1', 'pkg1', 'draft').ok).toBe(true); + expect(insert('y2', 'view', 'lead.f', 'org1', 'pkg1', 'draft').ok).toBe(false); + // The ACTIVE index is unaffected by the draft conflict. + expect(result.active.status).toBe('created'); + expect(indexDdl(OVERLAY_INDEX_NAMES.active)!.toLowerCase()).toContain("where state = 'active'"); + + expect(logger.error).toHaveBeenCalledTimes(1); + const msg = String(logger.error.mock.calls[0]![0]); + expect(msg).toContain(OVERLAY_INDEX_NAMES.draft); + expect(msg).toContain("state='draft'"); + expect(msg).toContain(buildOverlayDuplicateProbeSql('draft')); + const offenders = db + .prepare(buildOverlayDuplicateProbeSql('draft')) + .all() as Array>; + expect(offenders.map((o) => o.name)).toEqual(['lead.e']); + }); + + it('a dialect refusal on the DRAFT index reports it under the draft name', async () => { + const logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn() }; + const mariadbish: IndexExec = async (sql: string) => { + if (/where/i.test(sql)) throw new Error("near 'WHERE state': syntax error"); + return db.exec(sql); + }; + + const result = await ensureOverlayStateIndex(mariadbish, 'draft', logger); + + expect(result.status).toBe('unsupported'); + expect(result.fallback).toBe('ensured'); + // Nothing partial survives, and the composite lookup index is what the + // name now holds — non-UNIQUE, so the ACTIVE+DRAFT pair stays legal. + expect(indexDdl(OVERLAY_INDEX_NAMES.draft)!.toLowerCase()).not.toContain('unique'); + expect(String(logger.error.mock.calls[0]![0])).toContain(OVERLAY_INDEX_NAMES.draft); + }); + + it('the ACTIVE+DRAFT pair stays legal even after BOTH indexes degrade', async () => { + const mariadbish: IndexExec = async (sql: string) => { + if (/where/i.test(sql)) throw new Error("near 'WHERE state': syntax error"); + return db.exec(sql); + }; + // Start from a table with no unrestricted UNIQUE, so the degradation + // target is the only index either name holds. + db.exec(`DROP INDEX ${OVERLAY_INDEX_NAMES.active}`); + + const result = await ensureMetadataOverlayIndexes(mariadbish); + + expect(result.active.status).toBe('unsupported'); + expect(result.draft.status).toBe('unsupported'); + // The whole reason the fallback may never become a full UNIQUE. + expect(insert('c1', 'view', 'lead.c', 'org1', 'pkg1', 'active').ok).toBe(true); + expect(insert('c2', 'view', 'lead.c', 'org1', 'pkg1', 'draft').ok).toBe(true); + }); + + // ── Seams ───────────────────────────────────────────────────────────── + + /** + * #6418 is an ORDER and REPORTING fix. The key spelling it inherited is + * pinned as a literal — byte-identical to what `protocol.ts` issued before + * — so a later "while we are in here" re-keying cannot ride in unnoticed: + * re-partitioning a live unique index needs its own ADR-0120 D4 ceremony. + */ + it('pins the key spelling, unchanged by this fix', () => { + expect(OVERLAY_INDEX_COLUMNS).toEqual(['type', 'name', 'organization_id', 'package_id']); + expect(OVERLAY_NULL_SENTINELS).toEqual({ package_id: '' }); + expect(overlayIndexKeyParts()).toEqual([ + 'type', + 'name', + 'organization_id', + "COALESCE(package_id, '')", + ]); + expect(buildOverlayIndexSql(OVERLAY_INDEX_NAMES.active, 'active')).toEqual( + 'CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_active ' + + "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + + "WHERE state = 'active'", + ); + expect(buildOverlayIndexSql(OVERLAY_INDEX_NAMES.draft, 'draft')).toEqual( + 'CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_draft ' + + "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + + "WHERE state = 'draft'", + ); + }); + + /** + * The scope caution, as a test. A full UNIQUE here would reject the ACTIVE + * + DRAFT coexistence this table is built on, which is the key difference + * from `sys_view_definition`. + */ + it('the dialect fallback is NOT unique, and never drops anything first', () => { + const sql = buildOverlayFallbackIndexSql(OVERLAY_INDEX_NAMES.active); + expect(sql).toEqual( + 'CREATE INDEX IF NOT EXISTS idx_sys_metadata_overlay_active ' + + 'ON sys_metadata (type, name, organization_id, package_id)', + ); + expect(sql.toUpperCase()).not.toContain('UNIQUE'); + expect(sql).toContain('IF NOT EXISTS'); + }); + + /** + * The duplicate-listing query projects the nullable column through the SAME + * `COALESCE` it groups by. A bare `package_id` projection would be rejected + * by PostgreSQL ("must appear in the GROUP BY clause"), and this query has + * to run on both dialects that can build the index it explains. + */ + it('the duplicate-listing query is groupable on PostgreSQL, not only SQLite', () => { + const sql = buildOverlayDuplicateProbeSql('active'); + expect(sql).toEqual( + 'SELECT type, name, organization_id, ' + + "COALESCE(package_id, '') AS package_id_key, COUNT(*) AS duplicate_rows " + + "FROM sys_metadata WHERE state = 'active' " + + "GROUP BY type, name, organization_id, COALESCE(package_id, '') HAVING COUNT(*) > 1", + ); + // Every bare projection is a bare GROUP BY term; the folded one is + // projected only through its own expression. + expect(sql).not.toMatch(/SELECT [^,]*, package_id,/); + }); +}); diff --git a/packages/metadata-protocol/src/migrations/overlay-index.ts b/packages/metadata-protocol/src/migrations/overlay-index.ts new file mode 100644 index 0000000000..ede45717d8 --- /dev/null +++ b/packages/metadata-protocol/src/migrations/overlay-index.ts @@ -0,0 +1,381 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `sys_metadata` — overlay uniqueness, delivered at runtime (#6418). + * + * ## What was broken + * + * `metadata-core`'s `sys-metadata.object.ts` declares + * + * ```ts + * { name: 'idx_sys_metadata_overlay_active', + * fields: ['type', 'name', 'organization_id', 'package_id'], unique: true } + * ``` + * + * and `syncDeclaredIndexes` materializes it as an UNRESTRICTED, NULL-distinct + * UNIQUE index — knex's `table.unique()` can express neither a `WHERE` nor a + * `COALESCE`. What ADR-0005 actually wants is uniqueness among ACTIVE rows over + * a NULL-safe key, and `protocol.ts`'s `ensureOverlayIndex` has always + * delivered that in raw SQL at boot. + * + * Its ORDER was the defect. It ran + * + * ```text + * DROP INDEX IF EXISTS idx_sys_metadata_overlay_active ← always succeeds + * CREATE UNIQUE INDEX idx_sys_metadata_overlay_active … ← may fail + * ``` + * + * with nothing that puts the dropped index back, and both `catch` blocks empty. + * On SQLite/PostgreSQL — the dialects that DO support the form — a `CREATE` that + * failed on existing rows therefore left `sys_metadata` with **no** unique index + * at all, and no line in the log. ADR-0005 overlay uniqueness is the base of + * metadata correctness: with two ACTIVE rows for one + * `(type, name, organization_id, package_id)`, which one `getMetaItem` returns + * is undefined. + * + * The degradation branch could not save it either. It fired only when the + * driver's message matched `/partial|where clause|syntax/i`, which duplicate-row + * errors (`UNIQUE constraint failed` / `duplicate key value`) do not — so the + * one failure that is about DATA fell through to a bare `// best-effort` + * comment. MySQL was safe only by accident: `DROP INDEX IF EXISTS` is not legal + * MySQL, so the drop failed first and the old index survived. + * + * ## Why a conflict is a LIVE path here + * + * The runtime index is a **tightening** of the declared one in two independent + * ways, so rows the database admits today can block it: + * + * - the declared index is NULL-distinct, and `package_id` is NULL for every + * package-less (global) overlay — so two ACTIVE global rows under one + * `(type, name, organization_id)` are legal for it and rejected by + * `COALESCE(package_id, '')`; + * - `WHERE state = 'active'` narrows the row set, which by itself is a + * relaxation — but the DDL still has to be built against whatever the table + * already holds. + * + * Hence {@link ensureMetadataOverlayIndexes} takes + * `partial-index-probe.ts`'s probe-first order — build under a throwaway name, + * and only once that has demonstrably succeeded drop the real name and rebuild + * it — and reports every failure the way ADR-0120 D4 requires: keep the + * previous index, name the key that is NOT enforced, ship the exact query that + * lists the offending rows, point at `os migrate plan`, never block the boot. + * `SqlDriver.createNullSafeUniqueIndex` is the in-repo precedent for that + * disposition; the sibling `view-definition-active-index.ts` is the precedent + * for the order. + * + * ## Why the fallback stays NON-unique + * + * On a dialect that cannot build the partial form the degradation target is a + * plain composite index (`CREATE INDEX`, no `UNIQUE`) — exactly as before + * #6418. That is deliberate and must not be "fixed" into a full UNIQUE: this + * table legitimately holds one ACTIVE row and one DRAFT row for the same + * `(type, name, organization_id, package_id)` at the same time — that + * coexistence is the entire reason + * {@link OVERLAY_DRAFT_INDEX_NAME} exists as a separate index — and an + * unrestricted UNIQUE would reject it. This is the key difference from + * `sys_view_definition`, whose declared index IS a full UNIQUE. + * + * What #6418 changes about the fallback is not its shape but its honesty: it is + * created with `IF NOT EXISTS` and **without** a preceding drop, so it can only + * ever add an index and never replace a stronger one, and the report says + * plainly that uniqueness is not enforced on this dialect. + * + * ## The KEY spelling is untouched + * + * `(type, name, organization_id, COALESCE(package_id, ''))`, byte-identical to + * what shipped before. #6418 is an ORDER and REPORTING fix; re-keying this + * index (notably the bare, NULL-distinct `organization_id` part) is a separate + * question with its own ADR-0120 D4 ceremony and is deliberately not decided + * here. + */ + +import { + logProblem, + probeThenReplaceIndex, + type IndexExec, + type IndexMigrationLogger, + type PartialIndexStatus, +} from './partial-index-probe.js'; + +/** The one table this migration touches. */ +export const OVERLAY_TABLE = 'sys_metadata'; + +/** + * The two overlay states that get their own partial UNIQUE index. They are + * separate indexes precisely so an active row and a draft row for one key can + * coexist — the `state` predicate is what keeps them from colliding. + */ +export type OverlayIndexState = 'active' | 'draft'; + +/** + * Index names, per state. `…_active` is deliberately the SAME name + * `sys-metadata.object.ts` declares, so `syncDeclaredIndexes` — which skips by + * name — treats the slot as filled and never re-imposes the unrestricted form. + */ +export const OVERLAY_INDEX_NAMES: Readonly> = { + active: 'idx_sys_metadata_overlay_active', + draft: 'idx_sys_metadata_overlay_draft', +}; + +/** Throwaway names used to prove the partial form is possible before dropping. */ +export const OVERLAY_PROBE_INDEX_NAMES: Readonly> = { + active: 'idx_sys_metadata_overlay_active_probe', + draft: 'idx_sys_metadata_overlay_draft_probe', +}; + +/** The key COLUMNS, unchanged by #6418 — see the module header. */ +export const OVERLAY_INDEX_COLUMNS = ['type', 'name', 'organization_id', 'package_id'] as const; + +/** + * The nullable key column that folds its NULLs into ONE bucket, and the + * sentinel it folds to. + * + * `package_id` is NULL for every package-less (global) overlay, and SQL UNIQUE + * treats NULLs as DISTINCT — so without this the global rows would be + * unconstrained among themselves. `''` is the spelling that shipped; a package + * id is never the empty string, so the bucket cannot collide with real data. + * + * ⚠️ `organization_id` is deliberately NOT listed. It is bare in the index that + * shipped, and #6418 is an ordering fix — folding it too is a re-keying with + * its own ADR-0120 D4 ceremony, not a rider on this one. + */ +export const OVERLAY_NULL_SENTINELS: Readonly> = { + package_id: '', +}; + +/** + * The index's key parts, in key order: a bare column, or its NULL-safe + * `COALESCE` form when {@link OVERLAY_NULL_SENTINELS} names one. + * + * One builder so the CREATE, the duplicate-listing query the conflict report + * ships, and the degradation messages can never describe different keys. + */ +export function overlayIndexKeyParts(): string[] { + return OVERLAY_INDEX_COLUMNS.map((column) => { + const sentinel = OVERLAY_NULL_SENTINELS[column]; + return sentinel === undefined ? column : `COALESCE(${column}, '${sentinel}')`; + }); +} + +/** `CREATE UNIQUE INDEX … WHERE state = ''` under the given name. */ +export function buildOverlayIndexSql(indexName: string, state: OverlayIndexState): string { + return ( + `CREATE UNIQUE INDEX IF NOT EXISTS ${indexName} ` + + `ON ${OVERLAY_TABLE} (${overlayIndexKeyParts().join(', ')}) ` + + `WHERE state = '${state}'` + ); +} + +/** + * The dialect fallback: a plain composite index over the BARE columns. + * + * Non-UNIQUE on purpose (see the module header) and created with + * `IF NOT EXISTS` and no preceding drop, so on a name that already holds a + * stronger index this statement is a no-op rather than a downgrade. + */ +export function buildOverlayFallbackIndexSql(indexName: string): string { + return ( + `CREATE INDEX IF NOT EXISTS ${indexName} ` + + `ON ${OVERLAY_TABLE} (${OVERLAY_INDEX_COLUMNS.join(', ')})` + ); +} + +/** + * The query that lists the rows blocking the tightening — ADR-0120 D4's "name + * the offending rows", shipped inside the report so an operator has it without + * waiting for `os migrate plan`. + * + * It GROUPs by exactly the index's own key parts, so what it reports and what + * the index rejects cannot diverge. The nullable column is projected through + * the SAME `COALESCE` it is grouped by (aliased `package_key`) rather than + * bare: a bare projection of a column that appears in `GROUP BY` only inside an + * expression is rejected by PostgreSQL, and this query has to run on both + * dialects that can build the index it is explaining. + */ +export function buildOverlayDuplicateProbeSql(state: OverlayIndexState): string { + const projected = OVERLAY_INDEX_COLUMNS.map((column) => { + const sentinel = OVERLAY_NULL_SENTINELS[column]; + return sentinel === undefined ? column : `COALESCE(${column}, '${sentinel}') AS ${column}_key`; + }); + return ( + `SELECT ${projected.join(', ')}, COUNT(*) AS duplicate_rows ` + + `FROM ${OVERLAY_TABLE} WHERE state = '${state}' ` + + `GROUP BY ${overlayIndexKeyParts().join(', ')} HAVING COUNT(*) > 1` + ); +} + +/** + * What became of the non-UNIQUE composite fallback, when one was attempted. + * + * `'ensured'` deliberately does not claim the index was CREATED: the statement + * carries `IF NOT EXISTS`, so on a name that already holds an index (the + * declaration's own unrestricted UNIQUE, typically) it is a no-op and that + * stronger index survives. All the outcome states is that the name now holds + * an index and nothing was replaced by something weaker. + */ +export type OverlayFallbackOutcome = 'not-attempted' | 'ensured' | 'refused'; + +export interface EnsureOverlayIndexResult { + status: PartialIndexStatus; + /** Driver error text, when there was one. */ + detail?: string; + /** Only ever leaves `'not-attempted'` on the `unsupported` path. */ + fallback: OverlayFallbackOutcome; +} + +/** + * Bring ONE overlay index (`active` or `draft`) to its partial-UNIQUE form + * without ever leaving `sys_metadata` less protected than it was. + * + * Best-effort by design — a boot must never fail because an index could not be + * tightened, which is why every branch returns a status instead of throwing. + */ +export async function ensureOverlayStateIndex( + exec: IndexExec | undefined, + state: OverlayIndexState, + logger?: IndexMigrationLogger, +): Promise { + if (!exec) return { status: 'no-driver', fallback: 'not-attempted' }; + + const indexName = OVERLAY_INDEX_NAMES[state]; + const outcome = await probeThenReplaceIndex(exec, { + indexName, + probeIndexName: OVERLAY_PROBE_INDEX_NAMES[state], + buildSql: (name) => buildOverlayIndexSql(name, state), + }); + + if (outcome.status === 'created') return { status: 'created', fallback: 'not-attempted' }; + + const detail = outcome.detail ?? ''; + + if (outcome.failedAt === 'replace') { + // Only reachable on a race with another process between the drop and + // the create — the probe already cleared dialect and data. Say so + // rather than leaving a table that now has no unique index at all. + logProblem( + logger, + `[metadata-protocol] could not create '${indexName}' on "${OVERLAY_TABLE}" after the probe ` + + `succeeded — the table may currently have NO unique index over ` + + `(${overlayIndexKeyParts().join(', ')}) among state='${state}' rows. Restart to retry (#6418).`, + detail, + ); + return { status: 'failed', detail, fallback: 'not-attempted' }; + } + + // The probe failed, so nothing was dropped: whatever index held this name + // is exactly as it was. On a dialect that cannot build the partial form, + // still offer the composite LOOKUP index — additively, never replacing. + let fallback: OverlayFallbackOutcome = 'not-attempted'; + if (outcome.status === 'unsupported') { + try { + await exec(buildOverlayFallbackIndexSql(indexName)); + fallback = 'ensured'; + } catch { + // Expected on MySQL proper, which has no `CREATE INDEX IF NOT + // EXISTS` either. Reported below rather than swallowed. + fallback = 'refused'; + } + } + + reportDegradation(state, outcome.status, detail, fallback, logger); + return { status: outcome.status, detail, fallback }; +} + +/** + * Bring BOTH overlay indexes to their partial-UNIQUE form. + * + * The two are independent: a failure on one must not skip the other, because + * they protect different row sets and losing both is strictly worse than + * losing one. + */ +export async function ensureMetadataOverlayIndexes( + exec: IndexExec | undefined, + logger?: IndexMigrationLogger, +): Promise> { + return { + active: await ensureOverlayStateIndex(exec, 'active', logger), + draft: await ensureOverlayStateIndex(exec, 'draft', logger), + }; +} + +/** + * Say what is NOT enforced and what fixes it — ADR-0120 D4's wording contract, + * which `SqlDriver.createNullSafeUniqueIndex` already follows for the same class + * of event. Never fails the boot: from the outside everything else looks normal, + * so silence here is what makes the gap expensive. + * + * Every arm is `error`, and that is a deliberate level choice rather than a + * default. What goes missing in all three is an INTEGRITY guarantee the platform + * states it enforces (ADR-0005): duplicate ACTIVE overlays accumulate, nothing + * looks broken, and `getMetaItem` starts returning whichever row the engine + * happened to reach first — a defect that surfaces releases later to someone who + * cannot connect it to a boot line. That is the durability arm of AGENTS.md's + * logging rule, and the level the empty `catch` blocks this replaces were the + * exact opposite of. + */ +function reportDegradation( + state: OverlayIndexState, + status: PartialIndexStatus, + detail: string, + fallback: OverlayFallbackOutcome, + logger?: IndexMigrationLogger, +): void { + const indexName = OVERLAY_INDEX_NAMES[state]; + const columns = OVERLAY_INDEX_COLUMNS.join(', '); + const keyParts = overlayIndexKeyParts().join(', '); + const duplicateQuery = buildOverlayDuplicateProbeSql(state); + + if (status === 'unsupported') { + const fallbackNote = + fallback === 'ensured' + ? `A plain composite index over (${columns}) is ensured under that name instead, as the ` + + `degradation target — deliberately NOT UNIQUE, because one ACTIVE and one DRAFT row ` + + `for the same key legitimately coexist on this table and an unrestricted UNIQUE would ` + + `reject legal data.` + : `The plain-composite degradation target could not be created either (this dialect has ` + + `no "CREATE INDEX IF NOT EXISTS"), so no index was added.`; + logProblem( + logger, + `[metadata-protocol] this database cannot build the overlay-uniqueness index — ` + + `'${indexName}' on "${OVERLAY_TABLE}" cannot be scoped to state='${state}' rows over ` + + `(${keyParts}), so ADR-0005 overlay uniqueness is NOT enforced as specified on this ` + + `dialect: package-less ${state} rows (package_id NULL) stay NULL-distinct and can duplicate ` + + `— getMetaItem then has no defined answer for which row wins — and any unrestricted UNIQUE ` + + `that remains additionally rejects the legal ACTIVE+DRAFT pair. The system keeps looking ` + + `healthy either way. Whatever index already carried this name is left exactly as it was, ` + + `never replaced by something weaker. ${fallbackNote} MySQL/MariaDB has no partial indexes, ` + + `so there is no in-dialect fix: run this platform on SQLite/PostgreSQL for the guarantee, ` + + `and meanwhile watch for duplicates with: ${duplicateQuery}`, + detail, + ); + return; + } + + if (status === 'conflict') { + logProblem( + logger, + `[metadata-protocol] cannot rebuild '${indexName}' on "${OVERLAY_TABLE}" — existing rows ` + + `violate (${keyParts}) among state='${state}'. The previous index is left in place, so ` + + `(${columns}) is enforced only as far as it was before; ADR-0005 overlay uniqueness is NOT ` + + `enforced until the duplicates are resolved, and getMetaItem has no defined answer for ` + + `which of the colliding rows wins. List them with: ${duplicateQuery} — or run ` + + `"os migrate plan" — then restart (ADR-0120 D4, #6418).`, + detail, + ); + return; + } + + // The catch-all ('failed'), raised alongside the dialect arm above and for + // the same reason. Leaving it quieter would report the case we UNDERSTAND + // (a named dialect limitation) more loudly than the one we do not, while the + // consequence is identical: the DDL did not run, overlay uniqueness is not + // in force, and nothing else looks wrong. + logProblem( + logger, + `[metadata-protocol] could not rebuild '${indexName}' on "${OVERLAY_TABLE}" as the ` + + `state='${state}' partial UNIQUE index; the existing index is unchanged, so two ${state} ` + + `overlay rows for one (${columns}) can still coexist while everything else looks healthy. ` + + `Fix the cause below and restart (#6418).`, + detail, + ); +} diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts new file mode 100644 index 0000000000..304f715e9e --- /dev/null +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts @@ -0,0 +1,174 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { DatabaseSync } from 'node:sqlite'; + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +import { + classifyIndexFailure, + dropIndexQuietly, + logProblem, + probeThenReplaceIndex, + type IndexExec, +} from './partial-index-probe.js'; + +/** + * The probe-first order, tested where it lives (#6418). + * + * Both migrations in this directory delegate their DDL sequence here, so the + * ORDER is asserted once — against a real SQLite database — rather than + * inferred twice from each caller's outcomes. + */ +describe('probe-first partial index replacement (#6418)', () => { + let db: DatabaseSync; + let exec: IndexExec; + + const REAL = 'idx_probe_real'; + const PROBE = 'idx_probe_probe'; + const EXISTING_DDL = 'CREATE UNIQUE INDEX `idx_probe_real` on `t` (`k`)'; + + const buildSql = (indexName: string): string => + `CREATE UNIQUE INDEX IF NOT EXISTS ${indexName} ON t (COALESCE(k, '')) WHERE live = 1`; + + const indexDdl = (name: string): string | undefined => + (db.prepare("SELECT sql FROM sqlite_master WHERE type='index' AND name=?").get(name) as + | { sql?: string } + | undefined)?.sql ?? undefined; + + beforeEach(() => { + db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (id TEXT PRIMARY KEY, k TEXT, live INTEGER);'); + db.exec(EXISTING_DDL); + exec = async (sql: string) => db.exec(sql); + }); + + afterEach(() => { + db.close(); + }); + + it('builds under the probe name FIRST, and only then claims the real one', async () => { + const seen: string[] = []; + const recording: IndexExec = async (sql: string) => { + seen.push(sql); + return db.exec(sql); + }; + + const outcome = await probeThenReplaceIndex(recording, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome).toEqual({ status: 'created' }); + // The real name is never mentioned until the probe has been built AND + // dropped — that ordering IS the fix. + const firstRealMention = seen.findIndex((sql) => sql.includes(REAL)); + const probeCreate = seen.findIndex((sql) => sql.startsWith('CREATE') && sql.includes(PROBE)); + expect(probeCreate).toBeGreaterThanOrEqual(0); + expect(firstRealMention).toBeGreaterThan(probeCreate); + expect(seen[firstRealMention]).toContain('DROP INDEX IF EXISTS'); + // No probe residue survives. + expect(indexDdl(PROBE)).toBeUndefined(); + expect(indexDdl(REAL)!.toLowerCase()).toContain('where live = 1'); + }); + + it('a failed probe touches NOTHING — the previous index is byte-for-byte intact', async () => { + // Two live rows sharing a key: the tighter index cannot be built. + db.prepare('INSERT INTO t (id, k, live) VALUES (?,?,?)').run('a', null, 1); + db.prepare('INSERT INTO t (id, k, live) VALUES (?,?,?)').run('b', null, 1); + + const outcome = await probeThenReplaceIndex(exec, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome.status).toBe('conflict'); + expect(outcome.failedAt).toBe('probe'); + expect(outcome.detail).toContain('UNIQUE constraint failed'); + expect(indexDdl(REAL)).toEqual(EXISTING_DDL); + expect(indexDdl(PROBE)).toBeUndefined(); + }); + + it('clears probe residue left by a process that died mid-probe', async () => { + db.exec(`CREATE INDEX ${PROBE} ON t (id)`); + + const outcome = await probeThenReplaceIndex(exec, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome.status).toBe('created'); + expect(indexDdl(PROBE)).toBeUndefined(); + }); + + it('distinguishes a post-probe rebuild failure, the one branch that CAN leave the name empty', async () => { + const racing: IndexExec = async (sql: string) => { + if (sql.includes(REAL) && sql.startsWith('CREATE')) throw new Error('database is locked'); + return db.exec(sql); + }; + + const outcome = await probeThenReplaceIndex(racing, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + + expect(outcome.status).toBe('failed'); + expect(outcome.failedAt).toBe('replace'); + expect(indexDdl(REAL)).toBeUndefined(); + }); + + it('never throws, whatever the driver does', async () => { + const hostile: IndexExec = async () => { + throw new Error('connection reset'); + }; + + await expect( + probeThenReplaceIndex(hostile, { indexName: REAL, probeIndexName: PROBE, buildSql }), + ).resolves.toEqual({ status: 'failed', detail: 'connection reset', failedAt: 'probe' }); + // …including a driver that rejects with a non-Error. + const weird: IndexExec = async () => Promise.reject('just a string'); + const outcome = await probeThenReplaceIndex(weird, { + indexName: REAL, + probeIndexName: PROBE, + buildSql, + }); + expect(outcome.detail).toBe('just a string'); + }); + + it('dropIndexQuietly swallows the dialects that have no IF EXISTS form', async () => { + const mysqlish: IndexExec = vi.fn(async () => { + throw new Error("You have an error in your SQL syntax near 'IF EXISTS'"); + }); + await expect(dropIndexQuietly(mysqlish, REAL)).resolves.toBeUndefined(); + expect(mysqlish).toHaveBeenCalledWith(`DROP INDEX IF EXISTS ${REAL}`); + }); + + it('classifies data conflicts ahead of dialect refusals', () => { + // MySQL's duplicate message mentions the key, so the data verdict has to + // win or a real conflict reads as "no partial index support here". + expect(classifyIndexFailure("Duplicate entry 'a-b' for key 'idx_probe_real'")).toBe('conflict'); + expect(classifyIndexFailure('UNIQUE constraint failed: t.k')).toBe('conflict'); + expect(classifyIndexFailure('duplicate key value violates unique constraint')).toBe('conflict'); + expect(classifyIndexFailure('near "WHERE": syntax error')).toBe('unsupported'); + expect(classifyIndexFailure('Functional index on a column is not supported')).toBe('unsupported'); + expect(classifyIndexFailure('disk I/O error')).toBe('failed'); + }); + + it('logProblem prefers error(), falls back to warn(), and tolerates neither', () => { + const full = { warn: vi.fn(), error: vi.fn() }; + logProblem(full, 'msg', 'detail'); + expect(full.error).toHaveBeenCalledTimes(1); + expect(full.error.mock.calls[0]![1]).toBeInstanceOf(Error); + expect(full.warn).not.toHaveBeenCalled(); + + const warnOnly = { warn: vi.fn() }; + logProblem(warnOnly, 'msg', 'detail'); + expect(warnOnly.warn).toHaveBeenCalledWith('msg', { detail: 'detail' }); + + expect(() => logProblem(undefined, 'msg', 'detail')).not.toThrow(); + expect(() => logProblem({}, 'msg', 'detail')).not.toThrow(); + }); +}); diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.ts new file mode 100644 index 0000000000..b407c37485 --- /dev/null +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.ts @@ -0,0 +1,188 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Probe-first partial-index replacement — the one description of an order two + * runtime migrations in this directory both need (#6418). + * + * ## The order, and why it is the whole point + * + * Both migrations here replace an index that is ALREADY protecting a table + * with a tighter one (a `WHERE`-scoped UNIQUE, NULL-safe key parts, or both). + * The obvious sequence — `DROP` the old, `CREATE` the new — has a failure mode + * with no recovery: the drop always succeeds, and if the create then fails + * (dialect cannot express the form, or existing rows violate the tighter key) + * the table is left with NO index under that name at all. Nothing restores it, + * and on the dialects that DO support the form the loss is silent. + * + * That was `ensureOverlayIndex`'s shape until #6418, and it is the exact defect + * `view-definition-active-index.ts` was written to avoid (see its "Why it + * PROBES before dropping anything"). The order this module implements: + * + * 1. build the tighter index under a THROWAWAY probe name — nothing that is + * currently enforcing is touched, so a failure here costs nothing; + * 2. drop the probe; + * 3. only now drop the real name and rebuild it with the tighter definition. + * + * The cost is building a small index twice on the boot that migrates. The + * benefit is that no failure mode can destroy a live constraint: on any dialect + * or dataset that cannot take the tighter form, what was there stays there — + * degraded to yesterday's behaviour, never below it. + * + * ## What this module deliberately does NOT do + * + * It does not report. Each caller owns its own wording, because what is lost + * when a tightening fails differs per table (ADR-0120 D4 requires naming the + * key that is not enforced and the consequence of it not being enforced), and + * one generic sentence would be true of neither table. This module hands back + * a classified status plus the driver's own text and stays out of the way. + */ + +/** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */ +export type IndexExec = (sql: string) => Promise; + +/** + * Minimal logger surface, structurally compatible with `@objectstack/spec`'s + * `Logger` (every method optional so a bare console or a test double fits). + * Signatures mirror that contract exactly — notably `error(msg, Error, meta)` + * versus `warn(msg, meta)` — so a host `Logger` is assignable as-is. + */ +export interface IndexMigrationLogger { + info?(message: string, meta?: Record): void; + warn?(message: string, meta?: Record): void; + error?(message: string, error?: Error, meta?: Record): void; +} + +/** + * Report a problem at the loudest level the host offers, bridging the two + * different shapes (`error` takes an Error, `warn` takes metadata) so callers + * never have to care which one exists. + */ +export function logProblem( + logger: IndexMigrationLogger | undefined, + message: string, + detail: string, +): void { + if (logger?.error) { + logger.error(message, new Error(detail)); + return; + } + logger?.warn?.(message, { detail }); +} + +export type PartialIndexStatus = + /** The tighter index is in place under its real name. */ + | 'created' + /** + * The dialect rejects the form — `CREATE INDEX … WHERE` (no dialect of + * MySQL has partial indexes) or `COALESCE` functional key parts + * (MySQL < 8.0.13 / MariaDB). Previous index kept. + */ + | 'unsupported' + /** Existing rows violate the tighter key. Previous index kept, operator told. */ + | 'conflict' + /** No raw-SQL-capable driver reachable (memory/mock hosts). No-op. */ + | 'no-driver' + /** Anything else, best-effort. Previous index kept. */ + | 'failed'; + +/** + * Classify a failed `CREATE UNIQUE INDEX`. + * + * Duplicate-row wording is checked BEFORE dialect wording: MySQL's duplicate + * error mentions the key, and some drivers wrap both facts in one string, so + * the more specific verdict has to win or a real data conflict would be + * misreported as "this dialect cannot build this index". + * + * The dialect arm covers both refusals a single `unsupported` verdict has to + * stand for, because MySQL hits them together and one error string cannot be + * split: no partial indexes at all, and (before 8.0.13 / on MariaDB) no + * functional key parts for `COALESCE` parts. Both leave the same outcome — the + * previous index stays — so one verdict is enough. + */ +export function classifyIndexFailure(message: string): PartialIndexStatus { + if (/unique constraint failed|duplicate entry|duplicate key value|violates unique/i.test(message)) { + return 'conflict'; + } + if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(message)) { + return 'unsupported'; + } + return 'failed'; +} + +/** + * `DROP INDEX IF EXISTS`, swallowing everything. + * + * Best-effort by contract. MySQL has no `DROP INDEX IF EXISTS ` form at + * all, so this throws there on every call — which is harmless precisely because + * the probe has already decided whether anything is allowed to be dropped. + */ +export async function dropIndexQuietly(exec: IndexExec, indexName: string): Promise { + try { + await exec(`DROP INDEX IF EXISTS ${indexName}`); + } catch { + // Best-effort — see above. + } +} + +export interface ProbeThenReplaceOptions { + /** The real index name, which the rebuilt index must claim. */ + indexName: string; + /** Throwaway name used to prove the form is possible before dropping anything. */ + probeIndexName: string; + /** Builds the tighter `CREATE …` statement for a given index name. */ + buildSql: (indexName: string) => string; +} + +export interface ProbeThenReplaceOutcome { + status: PartialIndexStatus; + /** Driver error text, when there was one. */ + detail?: string; + /** + * Which step failed, when one did. + * + * `'probe'` — nothing was touched; whatever index held {@link + * ProbeThenReplaceOptions.indexName} is exactly as it was. + * + * `'replace'` — the probe had already proven the form buildable, so the + * real name was dropped and the rebuild then failed anyway (a race with + * another process is the only way to get here). The caller owes the loudest + * report it has: this is the one branch where the name may now hold nothing. + */ + failedAt?: 'probe' | 'replace'; +} + +/** + * Build `indexName`'s tighter definition without ever leaving the table + * unprotected. See the module header for the order and its rationale. + * + * Never throws: a boot must not fail because an index could not be tightened, + * so every branch returns a status instead. + */ +export async function probeThenReplaceIndex( + exec: IndexExec, + { indexName, probeIndexName, buildSql }: ProbeThenReplaceOptions, +): Promise { + // ── Step 1: prove the form is possible WITHOUT touching the index that is + // currently protecting the table. The leading drop clears residue from a + // process that died mid-probe on an earlier boot. ───────────────────────── + await dropIndexQuietly(exec, probeIndexName); + try { + await exec(buildSql(probeIndexName)); + } catch (err: unknown) { + const detail = err instanceof Error ? err.message : String(err); + await dropIndexQuietly(exec, probeIndexName); + return { status: classifyIndexFailure(detail), detail, failedAt: 'probe' }; + } + await dropIndexQuietly(exec, probeIndexName); + + // ── Step 2: the form is known-buildable on this dialect, over this data. + // Claim the real name. ──────────────────────────────────────────────────── + await dropIndexQuietly(exec, indexName); + try { + await exec(buildSql(indexName)); + } catch (err: unknown) { + const detail = err instanceof Error ? err.message : String(err); + return { status: 'failed', detail, failedAt: 'replace' }; + } + return { status: 'created' }; +} diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts index 3ea5be0720..18a5a31193 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts @@ -117,6 +117,14 @@ * points at `os migrate plan`, and the boot continues. */ +import { + logProblem, + probeThenReplaceIndex, + type IndexExec, + type IndexMigrationLogger, + type PartialIndexStatus, +} from './partial-index-probe.js'; + /** The one table this migration touches. */ export const VIEW_DEFINITION_TABLE = 'sys_view_definition'; @@ -176,53 +184,22 @@ export function viewActiveIndexKeyParts(): string[] { }); } -/** Raw-SQL seam. Mirrors `ensureOverlayIndex`: `raw()` first, `execute()` second. */ -export type IndexExec = (sql: string) => Promise; - /** - * Minimal logger surface, structurally compatible with `@objectstack/spec`'s - * `Logger` (every method optional so a bare console or a test double fits). - * Signatures mirror that contract exactly — notably `error(msg, Error, meta)` - * versus `warn(msg, meta)` — so a host `Logger` is assignable as-is. + * The raw-SQL seam, the logger surface, the status vocabulary and the failure + * classifier all live in `partial-index-probe.ts` since #6418, when + * `ensureOverlayIndex` adopted this module's probe-first order and the two + * migrations stopped being able to afford separate copies of them. Re-exported + * under this module's original names so its callers and `index.ts` see no + * change. */ -export interface EnsureViewIndexLogger { - info?(message: string, meta?: Record): void; - warn?(message: string, meta?: Record): void; - error?(message: string, error?: Error, meta?: Record): void; -} +export type { IndexExec } from './partial-index-probe.js'; +export { classifyIndexFailure } from './partial-index-probe.js'; -/** - * Report a problem at the loudest level the host offers, bridging the two - * different shapes (`error` takes an Error, `warn` takes metadata) so callers - * never have to care which one exists. - */ -function logProblem( - logger: EnsureViewIndexLogger | undefined, - message: string, - detail: string, -): void { - if (logger?.error) { - logger.error(message, new Error(detail)); - return; - } - logger?.warn?.(message, { detail }); -} +/** @see IndexMigrationLogger */ +export type EnsureViewIndexLogger = IndexMigrationLogger; -export type EnsureViewIndexStatus = - /** The partial UNIQUE index is in place under the declared name. */ - | 'created' - /** - * The dialect rejects the form — `CREATE INDEX … WHERE` (no dialect of - * MySQL has partial indexes) or the `COALESCE` functional key parts - * (MySQL < 8.0.13 / MariaDB). Legacy index kept. - */ - | 'unsupported' - /** Existing rows violate the key. Legacy index kept, operator told. */ - | 'conflict' - /** No raw-SQL-capable driver reachable (memory/mock hosts). No-op. */ - | 'no-driver' - /** Anything else, best-effort. Legacy index kept. */ - | 'failed'; +/** @see PartialIndexStatus */ +export type EnsureViewIndexStatus = PartialIndexStatus; export interface EnsureViewIndexResult { status: EnsureViewIndexStatus; @@ -259,32 +236,6 @@ export function buildDuplicateProbeSql(): string { ); } -/** - * Classify a failed `CREATE UNIQUE INDEX … WHERE`. - * - * Duplicate-row wording is checked BEFORE dialect wording: MySQL's duplicate - * error mentions the key, and some drivers wrap both facts in one string, so - * the more specific verdict has to win or a real data conflict would be - * misreported as "this dialect cannot build this index". That ordering matters - * more since #6417 — the tightening makes a data conflict a LIVE path, not the - * near-unreachable corner it was under #5839 alone. - * - * The dialect arm covers both refusals a single `unsupported` verdict has to - * stand for, because MySQL hits them together and one error string cannot be - * split: no partial indexes at all, and (before 8.0.13 / on MariaDB) no - * functional key parts for the `COALESCE` parts. Both leave the same outcome — - * the declared bare composite stays — so one verdict is enough. - */ -export function classifyIndexFailure(message: string): EnsureViewIndexStatus { - if (/unique constraint failed|duplicate entry|duplicate key value|violates unique/i.test(message)) { - return 'conflict'; - } - if (/partial|where clause|near "where"|near 'where'|functional|syntax/i.test(message)) { - return 'unsupported'; - } - return 'failed'; -} - /** * Resolve a raw-SQL seam for `sys_view_definition`. * @@ -350,39 +301,24 @@ export async function ensureViewDefinitionActiveIndex( ): Promise { if (!exec) return { status: 'no-driver' }; - const drop = async (indexName: string): Promise => { - try { - await exec(`DROP INDEX IF EXISTS ${indexName}`); - } catch { - // Best-effort. MySQL has no `DROP INDEX IF EXISTS ` form at - // all; on that path the probe below has already bailed out. - } - }; + // The probe-first order — prove the partial form is possible under a + // throwaway name, and only THEN drop the declared name and rebuild it — + // lives in `partial-index-probe.ts` since #6418, when `ensureOverlayIndex` + // adopted it. Claiming the DECLARED name is what stops `syncDeclaredIndexes` + // (which skips by name) from re-imposing the unrestricted form next boot. + const outcome = await probeThenReplaceIndex(exec, { + indexName: VIEW_ACTIVE_INDEX_NAME, + probeIndexName: VIEW_ACTIVE_PROBE_INDEX_NAME, + buildSql: buildActiveIndexSql, + }); - // ── Step 1: prove the partial form is possible WITHOUT touching the - // constraint that is currently protecting the table. ────────────────── - await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); - try { - await exec(buildActiveIndexSql(VIEW_ACTIVE_PROBE_INDEX_NAME)); - } catch (err: unknown) { - const detail = err instanceof Error ? err.message : String(err); - const status = classifyIndexFailure(detail); - await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); - reportDegradation(status, detail, logger); - return { status, detail }; - } - await drop(VIEW_ACTIVE_PROBE_INDEX_NAME); + if (outcome.status === 'created') return { status: 'created' }; - // ── Step 2: the partial index is known-buildable here. Claim the - // DECLARED name so `syncDeclaredIndexes` never re-imposes the full one. ─ - await drop(VIEW_ACTIVE_INDEX_NAME); - try { - await exec(buildActiveIndexSql(VIEW_ACTIVE_INDEX_NAME)); - } catch (err: unknown) { + const detail = outcome.detail ?? ''; + if (outcome.failedAt === 'replace') { // Only reachable on a race with another process between the drop and // the create — the probe already cleared dialect and data. Say so // rather than leaving a table that now has no unique index at all. - const detail = err instanceof Error ? err.message : String(err); logProblem( logger, `[metadata-protocol] could not create '${VIEW_ACTIVE_INDEX_NAME}' on ` + @@ -392,7 +328,9 @@ export async function ensureViewDefinitionActiveIndex( ); return { status: 'failed', detail }; } - return { status: 'created' }; + + reportDegradation(outcome.status, detail, logger); + return { status: outcome.status, detail }; } /** diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 64e4bff9e1..59785af95e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -12,6 +12,9 @@ import { readEnvWithDeprecation, resolveTenancyPosture } from '@objectstack/type import { postureEnforcesWall } from '@objectstack/spec/security'; import type { MetadataHostEngine } from './host-engine.js'; import { evaluateRuntimeAuthoringGate } from './runtime-authoring-gate.js'; +// [#6418] `sys_metadata`'s overlay-uniqueness indexes: probe-first DDL plus the +// ADR-0120 D4 reporting that replaced this file's empty `catch` blocks. +import { ensureMetadataOverlayIndexes } from './migrations/overlay-index.js'; import { SysMetadataRepository, type SysMetadataEngine } from './sys-metadata-repository.js'; import { ConflictError, assertProtocolCompat, applyAuditFieldGovernance, type MetadataItem } from '@objectstack/metadata-core'; // [#5532] One vocabulary of "which driver read errors are benign", shared with @@ -2623,13 +2626,25 @@ export class ObjectStackProtocolImplementation implements } /** - * One-time guard for ensuring the overlay-uniqueness UNIQUE INDEX exists - * on `sys_metadata`. ADR-0005: scopes overlays by - * `(type, name, organization_id, environment_id, scope)` for active rows only. - * Idempotent SQL — safe to attempt on every protocol instance. - * - * Inlined here (rather than importing from @objectstack/metadata/migrations) - * to avoid a circular dependency: metadata already depends on objectql. + * One-time guard for ensuring the overlay-uniqueness UNIQUE INDEXes exist + * on `sys_metadata`. ADR-0005 (revised 2026-05) + ADR-0048: per-env DBs + * replace the old "per-project" isolation, so `environment_id` is no longer + * a discriminator — overlay uniqueness is + * `(type, name, organization_id, COALESCE(package_id, ''))`, enforced once + * among ACTIVE rows and once among DRAFT rows. Idempotent SQL — safe to + * attempt on every protocol instance. + * + * ⚠️ This method resolves a raw-SQL seam and nothing more. The DDL, its + * ORDER and its reporting live in `./migrations/overlay-index.ts` (#6418), + * which replaced the DROP-then-CREATE sequence that used to sit here: the + * drop always succeeded and a failing create left `sys_metadata` with no + * unique index at all, silently, because both `catch` blocks were empty. + * See that module's header for why the order is now probe-first and why the + * dialect fallback must stay NON-unique. + * + * Kept in this package (rather than imported from + * `@objectstack/metadata/migrations`) to avoid a circular dependency: + * metadata already depends on objectql. */ private overlayIndexEnsured = false; private async ensureOverlayIndex(): Promise { @@ -2660,66 +2675,15 @@ export class ObjectStackProtocolImplementation implements throw new Error('driver has neither raw nor execute'); } }; - // ADR-0005 (revised 2026-05) + ADR-0048: per-env DBs replace the old - // "per-project" isolation, so `environment_id` is no longer a - // discriminator. Overlay uniqueness is `(type, name, - // organization_id, COALESCE(package_id,''))` filtered to active - // rows — `package_id` is in the key so two installed packages - // shipping the same name each get their own overlay, while - // `COALESCE(...,'')` keeps the package-less (global) rows unique - // among themselves (a plain unique index would treat NULLs as - // distinct and allow duplicate globals). Drop the legacy composite - // index first so the new partial UNIQUE can claim the same name — - // DROP INDEX IF EXISTS is idempotent. - try { await exec("DROP INDEX IF EXISTS idx_sys_metadata_overlay_active"); } catch { /* best-effort */ } - const partialSql = - "CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_active " + - "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + - "WHERE state = 'active'"; - const fallbackSql = - "CREATE INDEX IF NOT EXISTS idx_sys_metadata_overlay_active " + - "ON sys_metadata (type, name, organization_id, package_id)"; - try { - await exec(partialSql); - } catch (err: any) { - const msg = err instanceof Error ? err.message : String(err); - if (/partial|where clause|syntax/i.test(msg)) { - try { - await exec(fallbackSql); - } catch { - // ignore — non-essential optimization - } - } - // "already exists" or anything else: best-effort - } - // Mirror the same partial-UNIQUE for draft rows so a second - // simultaneous draft cannot be inserted for the same - // (type,name,org,package). The unique-active index above already - // guards published rows; the two never collide because the - // `state` predicate disambiguates them. DROP first so an existing - // legacy 3-column draft index is replaced in-place (ADR-0048). - try { await exec("DROP INDEX IF EXISTS idx_sys_metadata_overlay_draft"); } catch { /* best-effort */ } - const draftPartialSql = - "CREATE UNIQUE INDEX IF NOT EXISTS idx_sys_metadata_overlay_draft " + - "ON sys_metadata (type, name, organization_id, COALESCE(package_id, '')) " + - "WHERE state = 'draft'"; - try { - await exec(draftPartialSql); - } catch (err: any) { - const msg = err instanceof Error ? err.message : String(err); - if (/partial|where clause|syntax/i.test(msg)) { - try { - await exec( - "CREATE INDEX IF NOT EXISTS idx_sys_metadata_overlay_draft " + - "ON sys_metadata (type, name, organization_id, package_id)", - ); - } catch { - // ignore — best effort - } - } - } + // `console` satisfies the logger surface structurally; this class + // carries no injected logger, and its own diagnostics go to + // `console.warn` throughout (see `emitMetadataMutation`). + await ensureMetadataOverlayIndexes(exec, console); } catch { - // ignore — index is an optimization, not a correctness invariant + // A boot must never fail over an index. Note this arm is now only + // reachable for driver RESOLUTION failures: every DDL failure past + // this point is classified and reported by the migration itself, + // instead of vanishing into an empty catch (#6418). } }