From 6ebb4686e3d507a336a615004d5e2613bd09cd98 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:27:20 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): never invent event_seq/version from a failed history read (#4867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SysMetadataRepository.nextEventSeq()` and `nextItemVersion()` both folded EVERY read failure of `sys_metadata_history` into `return 1` — the shape #4825 just fixed on the legacy `DatabaseLoader` path, sitting unchanged on the canonical transactional one, and here on TWO numbers rather than one. With rows already in the table, one flaky read handed the next row `event_seq = 1` / `version = 1`: a collision with an existing row, written successfully, logged nowhere. `version` is the worse half — `nextItemVersion()` reads MAX from history precisely so a delete + recreate keeps incrementing instead of restarting at 1, so a read failure restored exactly the behaviour the method exists to prevent, while `MetadataManager.rollback(type, name, version)` and the rollback REST route resolve a snapshot BY that number. Being inside a transaction does not help: a transaction serialises concurrent writers, but a successfully committed transaction commits a wrong number just as durably. What it does give is the clean remedy — throw, and the whole write rolls back rather than committing an invented number. Now discriminated by error type, reusing #4825's discriminator rather than starting a second vocabulary: only a genuine missing table returns 1; every other read failure reports the consequence and the remedy once at `error` (AGENTS.md degradation log levels) and rethrows. `isMissingTableError()` was internal to `@objectstack/metadata`, so it is now exported deliberately through a new leaf subpath, `@objectstack/metadata/errors` — not the package root, whose entry would drag the manager, every loader and their deps behind a 40-line predicate, which is what would tempt the next author into copying it instead. Fixes #4867 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- ...sys-metadata-repo-history-counters-loud.md | 59 +++ packages/metadata-protocol/package.json | 1 + ...tadata-repository.history-counters.test.ts | 419 ++++++++++++++++++ .../src/sys-metadata-repository.ts | 122 ++++- packages/metadata/package.json | 5 + packages/metadata/src/errors.ts | 50 +++ packages/metadata/tsup.config.ts | 4 + pnpm-lock.yaml | 3 + 8 files changed, 658 insertions(+), 5 deletions(-) create mode 100644 .changeset/sys-metadata-repo-history-counters-loud.md create mode 100644 packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts create mode 100644 packages/metadata/src/errors.ts diff --git a/.changeset/sys-metadata-repo-history-counters-loud.md b/.changeset/sys-metadata-repo-history-counters-loud.md new file mode 100644 index 0000000000..a603bd8c2e --- /dev/null +++ b/.changeset/sys-metadata-repo-history-counters-loud.md @@ -0,0 +1,59 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/metadata": patch +--- + +fix(metadata-protocol): `SysMetadataRepository` 的 `event_seq` / `version` 不再从一次失败的读里凭空发号 —— 只有「表还没建」可以从 1 开始 (#4867) + +`SysMetadataRepository.nextEventSeq()` 与 `nextItemVersion()` 各有一个同形的 `catch`,把读 +`sys_metadata_history` 的**全部**失败折成同一个答案: + +```ts +} catch { + // Table not provisioned yet (fresh DB) — start at 1. + return 1; +} +``` + +这是 #4825 刚在 `DatabaseLoader`(TSDoc 自称 legacy、非事务的那条路径)上修掉的形状,原样长在 +**canonical 路径**上 —— #4825 正文把 `SysMetadataRepository` 称作「历史写入应当收敛过去的地方」。 +而且这里有两个数字: + +- **`event_seq`** —— 历史排序与 rollback 定位的依据。表里已有 N 行时,一次瞬时读失败(连接抖动、 + 超时、权限)让下一条拿到 `1`,与既有行撞号; +- **`version`** —— `nextItemVersion()` 的 TSDoc 明说它刻意从 history 取 MAX「so delete + recreate + continues incrementing instead of restarting at 1」。一次读失败正好把它**恢复成它明确要避免的那个 + 行为**:lineage 从 1 重启并与既有 lineage 撞号,而 `MetadataManager.rollback(type, name, version)` + 与 `POST /api/v1/meta/:type/:name/rollback` 正是按这个数字定位快照 —— 撞号之后回滚可能落到另一条 + 记录的同号版本上。 + +关键危害与 #4825 相同,是「**落盘的字节是错的**」而不是「字节没落盘」:insert 成功、日志一行没有、 +系统对外完全正常,重试不修、重启也不修。 + +**「在事务里」并不能挡住它。** 事务解决的是*并发*撞号;它对「从一次失败的读推导出来的数字」没有任何 +意见,一个成功提交的事务照样把错号提交得同样持久。事务真正给出的是干净的补救:抛出去,整笔写入回滚, +而不是提交一个编造的号。 + +现在按**错误类型**判别,复用 #4825 落地的那套判别器(不另起一套): + +- **良性的「表还没建」** —— 没有行,就没有可撞的号,`1` 确实是下一个号,静默返回,fresh DB 照常启动; +- **其余一切读失败** —— 按 AGENTS.md「Degradation log levels」以 `error` 上报**后果**(写入已被中止、 + 事务回滚、什么都没提交;若按旧行为发 `1` 会与既有行撞号,使版本顺序不可信、回滚目标可能指向另一条 + 记录的同号版本,且无人能发现、重启也修不回来)与**修复动作**(修数据源/驱动错误后重试写入),然后 + **原样抛出**,让事务回滚。一次故障只说一次,恢复时补一条 `info`。 + +### `@objectstack/metadata` 新增子路径导出 `@objectstack/metadata/errors` + +判别器 `isMissingTableError()`(#4728/#4825 家族)此前是 `@objectstack/metadata` 的内部工具,而本次 +消费者在另一个包。三个选项中选了「从现有归属地**显式导出**」:在 `metadata-protocol` 里复制一份会重建 +#4825 刚消灭的双源问题(同一个问题两套「哪些驱动错误算良性」的词汇表,谁先学会一个驱动怪癖谁就先漂移); +下沉到公共依赖本轮不可行(`packages/spec` 冻结、`packages/types` 有并行改动),且本次导出并不妨碍维护者 +之后再下沉。 + +新增的是一个**叶子子路径**而不是包入口导出:`@objectstack/metadata` 的根入口会拖进 manager、全部 +loader 与其 YAML/文件系统依赖,只为一个 40 行谓词付这个重量,正是把下一个作者推回「复制一份」的原因。 +`@objectstack/metadata/errors` 只 re-export 一个叶子模块,跨包依赖边因此仍是叶子边,也是将来下沉时 +一个可 grep、可删除的单点。仅导出 `isMissingTableError`;同族的 `isSchemaAlreadyExistsError` 在包外 +没有消费者,保持内部(导出一个无人 import 的符号是白许的承诺)。 + +无 API 破坏、无 schema 变更、无 `packages/spec` 改动。 diff --git a/packages/metadata-protocol/package.json b/packages/metadata-protocol/package.json index 9f41d6e84a..65046143b9 100644 --- a/packages/metadata-protocol/package.json +++ b/packages/metadata-protocol/package.json @@ -35,6 +35,7 @@ "@objectstack/core": "workspace:*", "@objectstack/formula": "workspace:*", "@objectstack/lint": "workspace:*", + "@objectstack/metadata": "workspace:*", "@objectstack/metadata-core": "workspace:*", "@objectstack/spec": "workspace:*", "@objectstack/types": "workspace:*", diff --git a/packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts b/packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts new file mode 100644 index 0000000000..6bf44b3700 --- /dev/null +++ b/packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts @@ -0,0 +1,419 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4867 — `SysMetadataRepository` never invents `event_seq` / `version` from a + * read that failed. Same family as #4825 (`DatabaseLoader.nextEventSeq()`) and + * #4728 (`ensureSchema()`); the log-level rule is #4632. + * + * Both counters used to `catch { return 1 }`, folding EVERY read failure of + * `sys_metadata_history` into the one benign answer. The damage is not "a row + * that never landed" but **a row that landed carrying a wrong number**, which + * is why every assertion below is about the VALUE that reaches the table — and + * about the table's contents AFTER the failure, since a committed transaction + * commits a wrong number just as durably as a non-transactional insert does. + * + * `version` is the worse of the two: `nextItemVersion()` reads MAX from history + * precisely "so delete + recreate continues incrementing instead of restarting + * at 1", and a read failure restored exactly the behaviour it exists to + * prevent — while `MetadataManager.rollback(type, name, version)` and + * `POST /api/v1/meta/:type/:name/rollback` resolve a snapshot BY that number. + * + * Both directions are pinned deliberately. A suite proving only the loud half + * would pass on a `() => false` discriminator (fresh DBs would stop booting), + * and one proving only the benign half passes on the original defect. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { SysMetadataRepository } from './sys-metadata-repository.js'; +import { isMissingTableError } from '@objectstack/metadata/errors'; + +/** + * The discriminator is spied on, not reimplemented: every test below runs the + * REAL `@objectstack/metadata/errors` implementation through a spy, so + * "was the shared function consulted?" is observable without changing what it + * answers. See the pin at the bottom of this file. + */ +vi.mock('@objectstack/metadata/errors', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, isMissingTableError: vi.fn(actual.isMissingTableError) }; +}); + +const discriminator = vi.mocked(isMissingTableError); + +/** The unmocked implementation, kept so a flipped verdict can be put back. */ +const { isMissingTableError: realIsMissingTableError } = await vi.importActual< + typeof import('@objectstack/metadata/errors') +>('@objectstack/metadata/errors'); + +interface Row { [k: string]: unknown } + +/** Benign: nothing is provisioned, so there is no row to collide with. */ +const noSuchTable = () => + Object.assign(new Error('no such table: sys_metadata_history'), { code: 'SQLITE_ERROR' }); + +/** NOT benign: the rows are still there, this read just did not see them. */ +const connectionReset = () => + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + +/** + * Engine fake with REAL transaction semantics — a txn body that throws commits + * nothing. That is load-bearing here rather than decorative: the claim under + * test is "the enclosing transaction aborts instead of committing a wrong + * number", and a fake that ignores rollback (like the other suites' fakes, which + * do not need it) could not tell a rollback from a commit. + * + * Reads of the HISTORY table can be broken independently; writes and the + * `sys_metadata` table keep working throughout, which is exactly what makes the + * defect invisible in production. + */ +function makeFakeEngine() { + const rows = new Map(); + const historyRows: Row[] = []; + let historyReadFailure: (() => unknown) | null = null; + + const keyOf = (w: Record) => + `${String(w.type)}|${String(w.name)}|${String(w.organization_id ?? 'null')}|${String(w.state ?? 'active')}`; + + const findRow = (where: Record) => { + if (where.id !== undefined) { + for (const [k, r] of rows) if (r.id === where.id) return { key: k, row: r }; + return null; + } + const k = keyOf(where); + const r = rows.get(k); + return r ? { key: k, row: r } : null; + }; + + const matchesHistory = (h: Row, where: Record): boolean => + Object.entries(where).every(([k, v]) => v === undefined || h[k] === v); + + return { + rows, + historyRows, + /** Every history row that actually COMMITTED, in write order. */ + committed: () => + historyRows.map((h) => ({ + name: h.name, + version: h.version, + event_seq: h.event_seq, + operation_type: h.operation_type, + })), + breakHistoryReads(makeError: () => unknown) { + historyReadFailure = makeError; + }, + healHistoryReads() { + historyReadFailure = null; + }, + async find(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_history') { + if (historyReadFailure) throw historyReadFailure(); + return historyRows.filter((h) => matchesHistory(h, opts.where)); + } + return Array.from(rows.values()).filter((r) => { + if (opts.where.type && r.type !== opts.where.type) return false; + if (opts.where.organization_id !== undefined && r.organization_id !== opts.where.organization_id) return false; + if (opts.where.state && r.state !== opts.where.state) return false; + return true; + }); + }, + async findOne(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_history') { + if (historyReadFailure) throw historyReadFailure(); + return historyRows.find((h) => matchesHistory(h, opts.where)) ?? null; + } + return findRow(opts.where)?.row ?? null; + }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_history') { + const h: Row = { ...data }; + if (!h.id) h.id = `h_${historyRows.length + 1}`; + historyRows.push(h); + return { id: h.id as string }; + } + const k = keyOf(data); + const row: Row = { id: `r_${rows.size + 1}`, ...data }; + rows.set(k, row); + return { id: row.id as string }; + }, + async update(_t: string, data: Record, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) throw new Error('not found'); + rows.set(found.key, { ...found.row, ...data }); + return { id: found.row.id as string }; + }, + async delete(_t: string, opts: { where: Record }) { + const found = findRow(opts.where); + if (!found) return { deleted: 0 }; + rows.delete(found.key); + return { deleted: 1 }; + }, + async transaction(cb: (ctx: any) => Promise): Promise { + const rowsSnapshot = new Map(Array.from(rows, ([k, r]) => [k, { ...r }] as const)); + const historySnapshot = historyRows.map((h) => ({ ...h })); + try { + return await cb({ txn: true }); + } catch (err) { + // ACID: a txn body that throws commits nothing at all. + rows.clear(); + for (const [k, r] of rowsSnapshot) rows.set(k, r); + historyRows.length = 0; + historyRows.push(...historySnapshot); + throw err; + } + }, + }; +} + +const view = (label: string) => ({ name: 'case_grid', label, object: 'case', columns: [{ field: 'name' }] }); +const otherView = (label: string) => ({ name: 'lead_grid', label, object: 'lead', columns: [{ field: 'name' }] }); + +describe('#4867 — history counters are never invented from a failed read', () => { + let engine: ReturnType; + let repo: SysMetadataRepository; + let errorSpy: ReturnType; + let infoSpy: ReturnType; + + const ref = { org: 'org_alpha', type: 'view' as const, name: 'case_grid' }; + const ref2 = { org: 'org_alpha', type: 'view' as const, name: 'lead_grid' }; + + beforeEach(() => { + discriminator.mockClear(); + engine = makeFakeEngine(); + repo = new SysMetadataRepository({ engine, organizationId: 'org_alpha', orgLabel: 'org_alpha' }); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + infoSpy = vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + infoSpy.mockRestore(); + }); + + describe('the benign case — a fresh DB where the history table is not provisioned', () => { + it('numbers BOTH counters from 1 and stays silent', async () => { + engine.breakHistoryReads(noSuchTable); + + await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + + // Two independently derived numbers: `version` from nextItemVersion(), + // `event_seq` from nextEventSeq(). Both must survive a missing table. + expect(engine.committed()).toEqual([ + { name: 'case_grid', version: 1, event_seq: 1, operation_type: 'create' }, + ]); + expect(errorSpy).not.toHaveBeenCalled(); + expect(infoSpy).not.toHaveBeenCalled(); + }); + + it('lets the metadata write itself commit — a fresh DB must still boot', async () => { + engine.breakHistoryReads(noSuchTable); + + const result = await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + expect(result.version).toBeTruthy(); + expect((await repo.get(ref))?.body).toMatchObject({ label: 'A' }); + }); + }); + + describe('a REAL read failure against history that already has rows', () => { + /** Two lineages: case_grid at version 1 and 2, lead_grid at version 1. */ + async function seedHistory(): Promise<{ caseHash: string; leadHash: string }> { + const first = await repo.put(ref, view('A'), { parentVersion: null, actor: 'usr_alice' }); + const second = await repo.put(ref, view('B'), { parentVersion: first.version, actor: 'usr_alice' }); + const other = await repo.put(ref2, otherView('C'), { parentVersion: null, actor: 'usr_alice' }); + expect(engine.committed()).toEqual([ + { name: 'case_grid', version: 1, event_seq: 1, operation_type: 'create' }, + { name: 'case_grid', version: 2, event_seq: 2, operation_type: 'update' }, + { name: 'lead_grid', version: 1, event_seq: 3, operation_type: 'create' }, + ]); + return { caseHash: second.version, leadHash: other.version }; + } + + it('aborts the put — no row with a colliding version/event_seq is committed', async () => { + const { caseHash } = await seedHistory(); + const before = engine.committed(); + + engine.breakHistoryReads(connectionReset); + await expect( + repo.put(ref, view('D'), { parentVersion: caseHash, actor: 'usr_alice' }), + ).rejects.toThrow('read ECONNRESET'); + + // Before #4867 this committed a FOURTH row at version 1 / event_seq 1 — + // duplicating case_grid's own first version and the org's first event, + // successfully and silently. + expect(engine.committed()).toEqual(before); + const versions = engine.historyRows.filter((h) => h.name === 'case_grid').map((h) => h.version); + expect(versions).toEqual([1, 2]); + expect(new Set(engine.historyRows.map((h) => h.event_seq)).size).toBe(engine.historyRows.length); + }); + + it('rolls the whole transaction back — the metadata row is untouched too', async () => { + const { caseHash } = await seedHistory(); + + engine.breakHistoryReads(connectionReset); + await expect( + repo.put(ref, view('D'), { parentVersion: caseHash, actor: 'usr_alice' }), + ).rejects.toThrow('read ECONNRESET'); + + engine.healHistoryReads(); + // 'B' was the last committed body; 'D' never happened. + expect((await repo.get(ref))?.body).toMatchObject({ label: 'B' }); + }); + + it('reports at error level, naming the consequence and the remedy', async () => { + const { caseHash } = await seedHistory(); + + engine.breakHistoryReads(connectionReset); + await expect( + repo.put(ref, view('D'), { parentVersion: caseHash, actor: 'usr_alice' }), + ).rejects.toThrow(); + + expect(errorSpy).toHaveBeenCalledTimes(1); + const [message, cause] = errorSpy.mock.calls[0] as [string, unknown]; + expect(message).toContain('sys_metadata_history'); + // which number was at stake + expect(message).toMatch(/\bversion\b/); + // consequence: restarting at 1 collides, and ordering/rollback go bad + expect(message).toMatch(/COLLIDES/); + expect(message).toMatch(/= 1/); + expect(message).toMatch(/ordering untrustworthy/i); + expect(message).toMatch(/rollback/i); + // what happened instead — the write is not silently half-done + expect(message).toMatch(/ABORTED/); + expect(message).toMatch(/rolled back/i); + // remedy + expect(message).toMatch(/Fix the datasource\/driver error/i); + // and the driver error is carried, not swallowed + expect((cause as Error).message).toBe('read ECONNRESET'); + }); + + it('covers the delete path too — no tombstone with an invented number', async () => { + const { caseHash } = await seedHistory(); + const before = engine.committed(); + + engine.breakHistoryReads(connectionReset); + await expect( + repo.delete(ref, { parentVersion: caseHash, actor: 'usr_alice' }), + ).rejects.toThrow('read ECONNRESET'); + + expect(engine.committed()).toEqual(before); + // The row itself is still there: `delete` rolled back with the counter. + engine.healHistoryReads(); + expect(await repo.get(ref)).not.toBeNull(); + }); + + it('says it once per outage, and reports recovery once', async () => { + const { caseHash, leadHash } = await seedHistory(); + + engine.breakHistoryReads(connectionReset); + await expect(repo.put(ref, view('D'), { parentVersion: caseHash, actor: null })).rejects.toThrow(); + await expect( + repo.put(ref2, otherView('E'), { parentVersion: leadHash, actor: null }), + ).rejects.toThrow(); + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(infoSpy).not.toHaveBeenCalled(); + + engine.healHistoryReads(); + await repo.put(ref, view('F'), { parentVersion: caseHash, actor: null }); + + expect(errorSpy).toHaveBeenCalledTimes(1); + expect(infoSpy).toHaveBeenCalledTimes(1); + expect((infoSpy.mock.calls[0] as [string])[0]).toMatch(/readable again/i); + // Numbering resumes after the surviving max — never from 1 again. + expect(engine.committed().at(-1)).toEqual({ + name: 'case_grid', + version: 3, + event_seq: 4, + operation_type: 'update', + }); + }); + }); + + it('DISTINGUISHES the two: same call site, opposite verdicts', async () => { + engine.breakHistoryReads(noSuchTable); + await repo.put(ref, view('A'), { parentVersion: null, actor: null }); + + const engine2 = makeFakeEngine(); + const repo2 = new SysMetadataRepository({ + engine: engine2, + organizationId: 'org_alpha', + orgLabel: 'org_alpha', + }); + engine2.breakHistoryReads(connectionReset); + await expect(repo2.put(ref, view('A'), { parentVersion: null, actor: null })).rejects.toThrow(); + + // Benign: numbered, committed, silent. Real: nothing committed, loud. + expect(engine.committed()).toHaveLength(1); + expect(engine2.committed()).toHaveLength(0); + expect(errorSpy).toHaveBeenCalledTimes(1); + }); +}); + +/** + * The cross-package pin (#4867). The whole point of routing through + * `@objectstack/metadata/errors` is that ONE implementation decides which + * driver errors are benign — a copy in this package would drift the day a + * driver quirk is taught to only one of them, and the drift's symptom is + * silently invented sequence numbers. + */ +describe('#4867 — the discriminator is the shared exported one, not a local copy', () => { + let errorSpy: ReturnType; + + beforeEach(() => { + discriminator.mockClear(); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + discriminator.mockReset(); + discriminator.mockImplementation(realIsMissingTableError); + errorSpy.mockRestore(); + }); + + it('consults the exported `isMissingTableError` with the driver error itself', async () => { + const engine = makeFakeEngine(); + const repo = new SysMetadataRepository({ engine, organizationId: 'org_alpha', orgLabel: 'org_alpha' }); + + engine.breakHistoryReads(noSuchTable); + await repo.put({ org: 'org_alpha', type: 'view', name: 'case_grid' }, view('A'), { + parentVersion: null, + actor: null, + }); + + expect(discriminator).toHaveBeenCalled(); + expect((discriminator.mock.calls[0]![0] as Error).message).toContain('no such table'); + }); + + it("follows the shared function's verdict — flipping it flips the behaviour", async () => { + const engine = makeFakeEngine(); + const repo = new SysMetadataRepository({ engine, organizationId: 'org_alpha', orgLabel: 'org_alpha' }); + + // A connection reset, declared benign BY THE SHARED MODULE. A local copy + // (or an inlined `catch { return 1 }`) would ignore this and answer 1 for + // its own reasons; a local copy that hard-coded "loud" would throw. Only + // code that actually asks this module can track it. + discriminator.mockReturnValue(true); + engine.breakHistoryReads(connectionReset); + + await repo.put({ org: 'org_alpha', type: 'view', name: 'case_grid' }, view('A'), { + parentVersion: null, + actor: null, + }); + + expect(engine.committed()).toEqual([ + { name: 'case_grid', version: 1, event_seq: 1, operation_type: 'create' }, + ]); + expect(errorSpy).not.toHaveBeenCalled(); + }); + + it('keeps no second driver-error vocabulary in this file', () => { + const source = readFileSync(new URL('./sys-metadata-repository.ts', import.meta.url), 'utf8'); + + expect(source).toMatch( + /import\s*\{\s*isMissingTableError\s*\}\s*from\s*'@objectstack\/metadata\/errors'/, + ); + // The signatures the shared matcher owns must live in exactly one place. + expect(source).not.toMatch(/function\s+isMissingTableError/); + expect(source).not.toMatch(/no such table|42P01|ER_NO_SUCH_TABLE|SQLITE_ERROR|errno/i); + }); +}); diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index 89e2fb0391..ee5ef27f95 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -50,6 +50,12 @@ */ import { hashSpec, ConflictError } from '@objectstack/metadata-core'; +// #4867 — the SAME discriminator `DatabaseLoader` uses (#4825), imported, not +// re-implemented. A second hand-rolled "which driver errors are benign?" here +// would be two vocabularies for one question, which is the dual-source debt +// #4825 deliberately retired. See `@objectstack/metadata/errors` for why that +// leaf subpath exists. +import { isMissingTableError } from '@objectstack/metadata/errors'; import { readEnvWithDeprecation } from '@objectstack/types'; import type { MetadataRepository, @@ -226,6 +232,18 @@ export class SysMetadataRepository implements MetadataRepository { /** Table name for the durable event log. */ private readonly historyTable = 'sys_metadata_history'; + /** + * #4867 — once-only reporting for the history-counter read seam. + * + * The history table is readable or it is not; repeating the same paragraph + * once per aborted write turns a real degradation into noise people learn to + * skim, which is what made the #4420 `warn` unreadable in the first place. + * Reset (with an `info`) on the next successful read, so an outage and its + * recovery each say themselves exactly once. Suppressing the *message* never + * suppresses the *failure*: every occurrence still throws. + */ + private historyCounterFailureReported = false; + constructor(opts: SysMetadataRepositoryOptions) { this.engine = opts.engine; this.organizationId = opts.organizationId ?? null; @@ -1047,6 +1065,28 @@ export class SysMetadataRepository implements MetadataRepository { * `sys_metadata_history` scoped by `organization_id`. MUST be called * inside a transaction (the only caller is the put/delete txn body) — * concurrent writers in the same org race otherwise. + * + * #4867 (same shape as #4825 on the legacy `DatabaseLoader` path; rule from + * #4632) — discriminate by error TYPE. This used to `catch { return 1 }` + * under a comment that named only the benign reason and then answered every + * reason with it. Exactly one reason licenses `1`: the history table has not + * been provisioned, so there is no row to collide with. Every other reason — + * dropped connection, timeout, insufficient privileges — means the rows are + * still there and merely were not seen, and numbering from 1 against a table + * with N rows **collides with existing rows** while the insert SUCCEEDS and + * the log stays empty. + * + * Being inside a transaction does not save this. A transaction serialises + * *concurrent* writers; it has no opinion about a number derived from a read + * that failed, and a successfully committed transaction commits a wrong + * `event_seq` just as durably as a non-transactional insert does. What the + * transaction *does* give us is the clean remedy: throw, and the whole write + * rolls back rather than committing an invented number. + * + * @throws The underlying driver error, unchanged, for every non-benign read + * failure — aborting the enclosing put/delete. Deliberate: a sequence + * number this method cannot derive from data it actually read is not + * a number it may invent. */ private async nextEventSeq(ctx: any): Promise { try { @@ -1059,10 +1099,14 @@ export class SysMetadataRepository implements MetadataRepository { const v = typeof row.event_seq === 'number' ? row.event_seq : 0; if (v > max) max = v; } + this.noteHistoryReadable(); return max + 1; - } catch { - // Table not provisioned yet (fresh DB) — start at 1. - return 1; + } catch (error) { + return this.historyCounterVerdict( + error, + 'event_seq', + 'the per-org history cursor that history ordering and rollback targeting both stand on', + ); } } @@ -1070,6 +1114,16 @@ export class SysMetadataRepository implements MetadataRepository { * Per-(org,type,name) lineage counter. Reads from history (not from * `sys_metadata.version`) so delete + recreate continues incrementing * instead of restarting at 1. + * + * #4867 — which is exactly why the old `catch { return 1 }` was the worse of + * the two: a read failure restored, precisely, the behaviour this method + * exists to prevent. The lineage restarts at 1, collides with the existing + * lineage rows, and `MetadataManager.rollback(type, name, version)` / + * `POST /api/v1/meta/:type/:name/rollback` locate their snapshot BY this + * number — so a rollback can land on a different record's same-numbered + * version. Same discrimination, same rethrow; see {@link nextEventSeq}. + * + * @throws The underlying driver error for every non-benign read failure. */ private async nextItemVersion( ref: Pick, @@ -1089,12 +1143,70 @@ export class SysMetadataRepository implements MetadataRepository { const v = typeof row.version === 'number' ? row.version : 0; if (v > max) max = v; } + this.noteHistoryReadable(); return max + 1; - } catch { - return 1; + } catch (error) { + return this.historyCounterVerdict( + error, + 'version', + `the ${ref.type}/${ref.name} lineage counter that rollback resolves a snapshot by`, + ); } } + /** + * The shared `catch` verdict for both history-derived counters (#4867). + * + * @returns `1` — and ONLY — when the table genuinely does not exist yet: + * no rows, therefore nothing to collide with, therefore 1 really is + * the next number. + * @throws The original error for every other read failure, after reporting + * the consequence once at `error` level (AGENTS.md "Degradation log + * levels": this is a durability/consistency degradation, not a + * functional one — the system keeps looking healthy while the bytes + * it persists are wrong). + */ + private historyCounterVerdict( + error: unknown, + counter: 'event_seq' | 'version', + subject: string, + ): 1 { + // Benign — and only benign: a fresh DB has no row to be inconsistent with. + if (isMissingTableError(error)) return 1; + + if (!this.historyCounterFailureReported) { + this.historyCounterFailureReported = true; + console.error( + `[SysMetadataRepository] Could not read \`${this.historyTable}\` to determine the next ` + + `\`${counter}\` (${subject}) — the metadata write is being ABORTED and the enclosing ` + + `transaction rolled back, so nothing is committed and the caller sees the failure. ` + + `Before #4867 this path answered \`${counter} = 1\` instead: against a table that ` + + `already has rows that number COLLIDES with an existing row, while the insert SUCCEEDS ` + + `and not one line is logged — leaving version ordering untrustworthy and rollback ` + + `targets ambiguous (a rollback can then resolve to a different record's same-numbered ` + + `version). No retry and no restart repairs that; a failed write is the loud, ` + + `recoverable alternative. Fix the datasource/driver error below (connection, timeout, ` + + `privileges) and retry the write.`, + error, + ); + } + throw error; + } + + /** + * Recovery half of the #4867 report: the counters are readable again, so the + * next outage gets to speak. Says so once, and only if something was said. + */ + private noteHistoryReadable(): void { + if (!this.historyCounterFailureReported) return; + this.historyCounterFailureReported = false; + console.info( + `[SysMetadataRepository] \`${this.historyTable}\` is readable again — \`event_seq\` / ` + + `\`version\` numbering recovered and metadata writes are being recorded again. Writes ` + + `rejected during the outage were not applied and must be re-submitted.`, + ); + } + /** Lightweight UUID-ish id for history rows; sufficient for an audit log. */ private uuid(): string { if (typeof globalThis.crypto?.randomUUID === 'function') { diff --git a/packages/metadata/package.json b/packages/metadata/package.json index e287ed1891..5b56b702c2 100644 --- a/packages/metadata/package.json +++ b/packages/metadata/package.json @@ -21,6 +21,11 @@ "types": "./dist/migrations/index.d.ts", "import": "./dist/migrations/index.js", "require": "./dist/migrations/index.cjs" + }, + "./errors": { + "types": "./dist/errors.d.ts", + "import": "./dist/errors.js", + "require": "./dist/errors.cjs" } }, "files": [ diff --git a/packages/metadata/src/errors.ts b/packages/metadata/src/errors.ts new file mode 100644 index 0000000000..19e3b63e7f --- /dev/null +++ b/packages/metadata/src/errors.ts @@ -0,0 +1,50 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `@objectstack/metadata/errors` — the shared driver-error discriminators for + * the metadata storage seams (#4728 / #4825 / #4867 family). + * + * ## Why this subpath exists + * + * The "which driver failures may be silenced?" question is not local to one + * package. It was answered first for DDL in `@objectstack/metadata` + * (`ensureSchema`, #4728), then for reads on the legacy `DatabaseLoader` path + * (`nextEventSeq`, #4825) — and the *canonical* transactional producer of the + * very same numbers, `SysMetadataRepository`, lives in a different package + * (`@objectstack/metadata-protocol`, #4867) and carried the identical defect. + * + * Three ways to serve that second package were considered; the third is the + * one taken, and the first is the one this module exists to prevent: + * + * 1. **Copy the predicate.** Rejected. Two hand-rolled vocabularies of + * "benign driver error" is precisely the dual-source debt #4825 killed: + * a driver quirk taught to one copy and not the other produces two + * packages that disagree about whether data may be silently invented. + * 2. **Sink it into a common dependency** (`@objectstack/types`, + * `@objectstack/spec/shared`). Architecturally attractive and explicitly + * *not* precluded by this module — but out of scope on the round that + * needed it (spec was frozen; types was under concurrent change). + * 3. **Export it deliberately from its current home** — this file. One + * declaration, one implementation, one place a new driver quirk is taught. + * + * ## Why a subpath and not the package entry + * + * `@objectstack/metadata`'s root entry pulls the manager, every loader and the + * YAML/filesystem machinery behind them. A consumer that wants a 40-line + * predicate should not have to load any of that, and the weight is exactly + * what would tempt the next author back to option 1. This entry re-exports + * one leaf module and nothing else, so the cross-package edge stays a leaf + * edge — and stays a single, greppable seam to delete if the maintainer later + * takes option 2. + * + * ## Scope of the promise + * + * Only {@link isMissingTableError} is exported: it has a cross-package + * consumer today. Its sibling `isSchemaAlreadyExistsError` deliberately stays + * internal to this package — it has no consumer outside it, and an exported + * symbol nobody imports is a promise made for nothing (Prime Directive #10, + * pointed at our own API surface). Add it here the day something outside + * `@objectstack/metadata` needs it, not before. + */ + +export { isMissingTableError } from './utils/schema-sync-errors.js'; diff --git a/packages/metadata/tsup.config.ts b/packages/metadata/tsup.config.ts index a6d6f382fb..4084e76695 100644 --- a/packages/metadata/tsup.config.ts +++ b/packages/metadata/tsup.config.ts @@ -7,6 +7,10 @@ export default defineConfig({ 'src/index.ts', 'src/node.ts', 'src/migrations/index.ts', + // `@objectstack/metadata/errors` — the shared driver-error discriminators + // (#4728/#4825/#4867). Its own entry so a consumer that needs only the + // predicate does not load the manager, the loaders and their deps. + 'src/errors.ts', ], splitting: false, sourcemap: true, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dae7bbe9ab..7fd6bc826c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -982,6 +982,9 @@ importers: '@objectstack/lint': specifier: workspace:* version: link:../lint + '@objectstack/metadata': + specifier: workspace:* + version: link:../metadata '@objectstack/metadata-core': specifier: workspace:* version: link:../metadata-core From 99be8e92e089850c6b1d305c6a4a6689b850659a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:48:14 +0000 Subject: [PATCH 2/2] test(metadata-protocol): record the #4867 engine double in the delete-dispatch ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:engine-double-contract` (#4550, landed today) flags the fake engine in `sys-metadata-repository.history-counters.test.ts`: its `delete` does not route through `assertEngineDeleteDispatch` from `@objectstack/objectql`. The gate's preferred remedy — add objectql as a devDependency — is not merely unreviewed here, it is CYCLIC. `@objectstack/objectql` already depends on `@objectstack/metadata-protocol` in `dependencies`, so the edge makes turbo refuse the graph outright; measured by adding it and reverting: Cyclic dependency detected: @objectstack/metadata-protocol#build, @objectstack/objectql#build So this takes the gate's other sanctioned route: a measured baseline entry naming the cycle as the reason, classified DEBT rather than EXEMPT because the ledger's own rule reserves EXEMPT for doubles nothing drives, and this one is driven (the #4867 delete-path test). The entry's `closes` names the only route that actually exists — sink the predicate into a package both sides already depend on — because the four sibling metadata-protocol entries prescribe the devDependency this commit just measured to be impossible. Filed separately rather than edited here: their text is not this PR's to rewrite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- scripts/engine-double-contract.baseline.json | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/engine-double-contract.baseline.json b/scripts/engine-double-contract.baseline.json index d4c446aeaa..c884e01341 100644 --- a/scripts/engine-double-contract.baseline.json +++ b/scripts/engine-double-contract.baseline.json @@ -45,6 +45,13 @@ "why": "@objectstack/metadata-protocol does not depend on @objectstack/objectql. Pinning needs a devDependency + lockfile change, which is a separate reviewable act.", "closes": "add @objectstack/objectql to devDependencies, then open the fake's delete with assertEngineDeleteDispatch(opts)" }, + { + "file": "packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "MEASURED (#4867): the devDependency this ledger's sibling entries prescribe is not available here — it is CYCLIC, not merely unreviewed. @objectstack/objectql depends on @objectstack/metadata-protocol in `dependencies`, so adding objectql to metadata-protocol's devDependencies makes turbo refuse the graph outright: `Cyclic dependency detected: @objectstack/metadata-protocol#build, @objectstack/objectql#build` (turbo 2.10.7, `turbo run test --filter=@objectstack/metadata-protocol --dry`, measured by adding the edge and reverting it). The fake's delete is exercised by one test (the #4867 delete path) and is a by-id delete routed through SysMetadataRepository.delete, but that is an argument about this file, not about the contract, so the entry stays DEBT rather than EXEMPT per this ledger's own rule.", + "closes": "sink assertEngineDeleteDispatch into a package BOTH sides already depend on (@objectstack/metadata-core is the common dep; @objectstack/spec/contracts is the other candidate), then open the fake's delete with it — the devDependency route is closed by the cycle above, for this file and for the four sibling metadata-protocol entries alike" + }, { "file": "packages/metadata-protocol/src/sys-metadata-repository.recorded-by.test.ts", "unguarded": 1,