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 23b6cf2a79..c738827d34 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 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,