diff --git a/.changeset/next-event-seq-read-failure-loud.md b/.changeset/next-event-seq-read-failure-loud.md new file mode 100644 index 0000000000..ac4472f49f --- /dev/null +++ b/.changeset/next-event-seq-read-failure-loud.md @@ -0,0 +1,43 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): 历史序号 `event_seq` 不再从一次失败的读里凭空发号 —— 只有「表还没建」可以从 1 开始 (#4825) + +`DatabaseLoader.nextEventSeq()` 过去把读 `sys_metadata_history` 的**全部**失败折成同一个答案: + +```ts +} catch { + // Table not provisioned yet or driver error — start at 1. + return 1; +} +``` + +注释同时点名了两种原因,然后用同一个 `return 1` 对待。这是 #4728 刚修掉的同一种形状,但危害是 +**更贵的那一半**:#4728 是「字节没落盘」,本条是「**落盘的字节是错的**」。历史表里已经有 N 行时, +一次瞬时读失败(连接抖动、超时、权限)会让下一条历史拿到 `event_seq = 1`,与既有行**直接撞号**, +而 insert **成功**、日志**一行没有**。`event_seq` 正是历史列表排序与 rollback 定位的依据,撞号之后 +版本顺序就永久不可信 —— 重试不修、重启也不修。 + +现在按**错误类型**判别,复用 #4728 落地的那套判别机制(`packages/metadata/src/utils/schema-sync-errors.ts` +里新增的 `isMissingTableError()` 与既有 `isSchemaAlreadyExistsError()` 共用同一个 code / errno / +message + `cause` 链匹配器,而不是在同一个包里另起一套错误判别): + +- **良性的「表还没建」**(SQLite `no such table: …`、Postgres SQLSTATE `42P01` / + `relation "…" does not exist`、MySQL `ER_NO_SUCH_TABLE` / errno `1146`,并跟随 `cause` 链)—— + 没有行,就没有可撞的号,`1` 确实是下一个号,静默返回。 +- **其余一切读失败** —— `nextEventSeq()` 原样抛出。调用方 `createHistoryRecord()` 以 + `console.error` 上报**后果**(该条历史记录未写入;元数据写入本身已成功,所以服务器仍报告健康, + 而变更历史正在悄悄出现空洞,版本时间线与 rollback 目标将不完整)、**为什么是空洞而不是错号** + (从 1 发号会与既有行撞号,把「不完整」变成「顺序错误」,后者无人能发现)与**修复动作**, + 然后**跳过这条历史记录**。 +- 判别的方向刻意保守:凡是没有被正面识别为「表不存在」的,一律当作真实失败。`does not exist` + 本身不够 —— `role "…" does not exist`、`database "…" does not exist`、`column "…" does not exist` + 都是真实失败,对着一张可能满是行的表返回 1 正是要避免的事,所以消息匹配要求 table/relation 与 + 该短语同现。 + +两条边界保持不变:元数据写入本身**不**因此失败(记录已经落盘,把它报成失败是比原缺陷更糟的谎), +以及本路径已知的并发撞号限制(非事务,canonical producer 仍是 `SysMetadataRepository`)——那是被 +记录过的限制,与「读失败静默重置到 1」是两回事。报告只说**一次**,恢复时补一条 `info`。 + +无 API / schema 变更;新增内部工具 `isMissingTableError()`(未从包入口导出)。 diff --git a/packages/metadata/src/loaders/database-loader.test.ts b/packages/metadata/src/loaders/database-loader.test.ts index fde893870e..b64a6c3847 100644 --- a/packages/metadata/src/loaders/database-loader.test.ts +++ b/packages/metadata/src/loaders/database-loader.test.ts @@ -641,6 +641,201 @@ describe('DatabaseLoader schema-sync failure reporting (#4728)', () => { }); }); +// ---------- event_seq is never invented from a read that failed ---------- + +/** + * #4825 (same family as #4728, rule: #4632). + * + * `nextEventSeq()` used to `catch { return 1 }`, with a comment naming BOTH + * "table not provisioned yet" (benign) and "driver error" (not benign). With N + * rows already in `sys_metadata_history`, one flaky read therefore handed the + * next row `event_seq = 1` — colliding with an existing row while the insert + * SUCCEEDED and nothing was logged. + * + * That is why these tests assert on the VALUE that lands, not merely on whether + * a write happened: the damage here is not a missing row, it is a written row + * carrying a wrong number, which no retry and no restart repairs. + * + * Both directions are pinned, plus a same-call-site/opposite-verdict case — a + * suite proving only the loud half would pass on a `() => true` classifier, + * which is the bug, and one proving only the benign half would pass on the + * `catch { return 1 }` being replaced. + */ +describe('DatabaseLoader event_seq on a failed history read (#4825)', () => { + let errorSpy: ReturnType; + let infoSpy: ReturnType; + + /** Benign: nothing has been 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' }); + + /** Every `event_seq` this driver was asked to persist, in order. */ + function historySeqsWritten(driver: IDataDriver): unknown[] { + const calls = (driver.create as ReturnType).mock.calls as unknown[][]; + return calls + .filter((call: unknown[]) => call[0] === 'sys_metadata_history') + .map((call: unknown[]) => (call[1] as Record).event_seq); + } + + /** The mock driver's own `find`, still callable after we wrap it. */ + type DriverFind = (table: string, query: unknown) => Promise[]>; + + /** + * A driver whose reads of the HISTORY table fail while `broken` — writes and + * the `sys_metadata` table keep working throughout, which is exactly what + * makes the defect invisible in production. + */ + function driverWithBreakableHistoryReads(makeError: () => unknown, startBroken = false) { + const driver = createMockDriver(); + const realFind = driver.find as DriverFind; + let broken = startBroken; + driver.find = vi.fn().mockImplementation((table: string, query: unknown) => { + if (broken && table === 'sys_metadata_history') return Promise.reject(makeError()); + return realFind(table, query); + }); + return { + driver, + breakReads: () => { + broken = true; + }, + healReads: () => { + broken = false; + }, + }; + } + + beforeEach(() => { + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + infoSpy = vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + infoSpy.mockRestore(); + }); + + describe('the benign case — the history table is not provisioned yet', () => { + it('numbers from 1 and stays silent', async () => { + const { driver } = driverWithBreakableHistoryReads(noSuchTable, true); + const loader = new DatabaseLoader({ driver }); + + await loader.save('object', 'account', { name: 'account' }); + + expect(historySeqsWritten(driver)).toEqual([1]); + expect(errorSpy).not.toHaveBeenCalled(); + expect(infoSpy).not.toHaveBeenCalled(); + }); + }); + + describe('a REAL read failure against a table that already has rows', () => { + it('does NOT restart at 1 — no colliding row is written at all', async () => { + const { driver, breakReads } = driverWithBreakableHistoryReads(connectionReset); + const loader = new DatabaseLoader({ driver }); + + // Build real history first: two rows, event_seq 1 and 2. + await loader.save('object', 'account', { name: 'account' }); + await loader.save('object', 'contact', { name: 'contact' }); + expect(historySeqsWritten(driver)).toEqual([1, 2]); + + breakReads(); + await loader.save('object', 'lead', { name: 'lead' }); + + // Before #4825 this was [1, 2, 1] — a duplicate `event_seq` written + // successfully, silently, over the top of an existing row's number. + expect(historySeqsWritten(driver)).toEqual([1, 2]); + }); + + it('reports at error, naming the consequence, the deliberate skip, and the fix', async () => { + const { driver, breakReads } = driverWithBreakableHistoryReads(connectionReset); + const loader = new DatabaseLoader({ driver }); + + await loader.save('object', 'account', { name: 'account' }); + breakReads(); + await loader.save('object', 'lead', { name: 'lead' }); + + expect(errorSpy).toHaveBeenCalledTimes(1); + const [message, cause] = errorSpy.mock.calls[0] as [string, unknown]; + expect(message).toContain('sys_metadata_history'); + expect(message).toContain('event_seq'); + // consequence: the row is gone AND the system keeps looking fine + expect(message).toMatch(/NOT written/); + expect(message).toMatch(/SUCCEEDED/); + expect(message).toMatch(/looking healthy/i); + // why a hole is preferable to a wrong number + expect(message).toMatch(/collide/i); + // fix + expect(message).toMatch(/fix the datasource\/driver error/i); + // and the driver error is carried, not discarded + expect((cause as Error).message).toBe('read ECONNRESET'); + }); + + it('does not fail the metadata write it accompanies', async () => { + const { driver, breakReads } = driverWithBreakableHistoryReads(connectionReset); + const loader = new DatabaseLoader({ driver }); + + breakReads(); + const result = await loader.save('object', 'account', { name: 'account' }); + + // The record write already happened; reporting it as failed would be a + // worse lie than the one being fixed. The history hole is what is loud. + expect(result.success).toBe(true); + expect((await loader.load('object', 'account')).data).toEqual({ name: 'account' }); + }); + + it('says it once, not once per skipped entry, and reports recovery', async () => { + const { driver, breakReads, healReads } = driverWithBreakableHistoryReads(connectionReset); + const loader = new DatabaseLoader({ driver }); + + await loader.save('object', 'account', { name: 'account' }); + breakReads(); + await loader.save('object', 'contact', { name: 'contact' }); + await loader.save('object', 'lead', { name: 'lead' }); + expect(errorSpy).toHaveBeenCalledTimes(1); + + healReads(); + await loader.save('object', 'deal', { name: 'deal' }); + + 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 (1), never from 1 again. + expect(historySeqsWritten(driver)).toEqual([1, 2]); + }); + }); + + it('DISTINGUISHES the two: same call site, opposite verdicts', async () => { + const { driver: benignDriver } = driverWithBreakableHistoryReads(noSuchTable, true); + const { driver: realDriver } = driverWithBreakableHistoryReads(connectionReset, true); + + await new DatabaseLoader({ driver: benignDriver }).save('object', 'account', { name: 'a' }); + await new DatabaseLoader({ driver: realDriver }).save('object', 'account', { name: 'a' }); + + // Benign: numbered, written, silent. Real: not numbered, not written, loud. + expect(historySeqsWritten(benignDriver)).toEqual([1]); + expect(historySeqsWritten(realDriver)).toEqual([]); + expect(errorSpy).toHaveBeenCalledTimes(1); + }); + + it('applies to the rollback history path too, not just save()', async () => { + const { driver, breakReads } = driverWithBreakableHistoryReads(connectionReset); + const loader = new DatabaseLoader({ driver }); + + await loader.save('object', 'account', { name: 'account' }); + await loader.save('object', 'account', { name: 'account', label: 'Account' }); + expect(historySeqsWritten(driver)).toEqual([1, 2]); + + breakReads(); + await loader.registerRollback('object', 'account', { name: 'account' }, 1); + + expect(historySeqsWritten(driver)).toEqual([1, 2]); + expect(errorSpy).toHaveBeenCalledTimes(1); + }); +}); + // ---------- DatabaseLoader read-through cache ---------- describe('DatabaseLoader read-through cache', () => { diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index 0b98fdd835..dbb22b338c 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -26,7 +26,7 @@ import type { IDataDriver, IDataEngine } from '@objectstack/spec/contracts'; import type { MetadataLoader } from './loader-interface.js'; import { calculateChecksum } from '../utils/metadata-history-utils.js'; import { LRUCache } from '../utils/lru-cache.js'; -import { isSchemaAlreadyExistsError } from '../utils/schema-sync-errors.js'; +import { isMissingTableError, isSchemaAlreadyExistsError } from '../utils/schema-sync-errors.js'; import { addSysMetadataOverlayIndex } from '../migrations/add-sys-metadata-overlay-index.js'; import { migrateProjectIdToEnvironmentId } from '../migrations/migrate-project-id-to-environment-id.js'; @@ -131,6 +131,12 @@ export class DatabaseLoader implements MetadataLoader { */ private schemaFailureReported = false; private historySchemaFailureReported = false; + /** + * Same once-only discipline for the #4825 seam: the history table is readable + * or it is not, and repeating the report per skipped write turns a real + * degradation into noise people learn to skim. + */ + private historySeqFailureReported = false; /** (type, name) → metadata payload — primes `load()` */ private readonly loadCache?: LRUCache | null>; @@ -267,6 +273,24 @@ export class DatabaseLoader implements MetadataLoader { * Reads `MAX(event_seq) + 1` for the configured `organization_id`. * Legacy path — not transactional, so concurrent writes can collide. * The canonical (transactional) producer is `SysMetadataRepository`. + * + * #4825 (same shape as #4728, rule from #4632) — discriminate by error TYPE. + * This used to `catch { return 1 }`, with a comment that named BOTH reasons a + * read can fail and then answered both the same way. Exactly one of them is + * benign: the history table has not been provisioned, so there is no row to + * be inconsistent with and 1 genuinely IS the next number. Every other reason + * — connection drop, timeout, insufficient privileges — means the rows are + * still there and simply were not seen, and answering 1 against a table with + * N rows **collides with existing rows**: the insert succeeds, the log stays + * empty, and `event_seq` (the ordering key that history listing and rollback + * targeting both stand on) is silently wrong from then on. Note this is the + * costlier half of the #4728 family — not bytes that never landed, but bytes + * that landed *wrong*, which no retry and no restart repairs. + * + * @throws The underlying driver error, unchanged, for every non-benign read + * failure. Deliberate: a sequence number this method cannot derive + * from data it actually read is not a number it may invent. The + * caller ({@link createHistoryRecord}) owns the consequence. */ private async nextEventSeq(): Promise { const where: Record = this.organizationId @@ -280,9 +304,11 @@ export class DatabaseLoader implements MetadataLoader { if (v > max) max = v; } return max + 1; - } catch { - // Table not provisioned yet or driver error — start at 1. - return 1; + } catch (error) { + // Benign — and ONLY benign: there is no table, therefore no row, so + // numbering from 1 cannot collide with anything. + if (isMissingTableError(error)) return 1; + throw error; } } @@ -493,7 +519,41 @@ export class DatabaseLoader implements MetadataLoader { // transaction, so concurrent writers can collide. The SysMetadataRepository // path serializes this under engine.transaction(); DatabaseLoader is // deprecated for new writes and tolerates the race. - const eventSeq = await this.nextEventSeq(); + // + // #4825: the concurrency race above is a KNOWN, recorded limitation of this + // path. A read failure is not the same thing and is not tolerated — if the + // sequence cannot be derived from rows we actually read, we write NO history + // row rather than one carrying a number we made up. A missing row is loud + // here and visibly absent later; a colliding row is silent now and corrupts + // the ordering that `queryHistory` and `rollback` both depend on, forever. + let eventSeq: number; + try { + eventSeq = await this.nextEventSeq(); + } catch (error) { + if (!this.historySeqFailureReported) { + this.historySeqFailureReported = true; + console.error( + `[Metadata] Could not read \`${this.historyTableName}\` to determine the next \`event_seq\` — the history ` + + `entry for ${type}/${name} was NOT written, and further entries are being skipped while this persists. ` + + `The metadata write itself SUCCEEDED, so the server keeps looking healthy while its change history ` + + `silently develops holes: version timelines and rollback targets will be incomplete. The entry is skipped ` + + `deliberately — numbering it from 1 (what this code did before #4825) would collide with existing rows and ` + + `make \`event_seq\` ordering wrong rather than merely incomplete, which nothing detects and no restart ` + + `repairs. Fix the datasource/driver error below (connection, timeout, privileges); the next metadata write ` + + `retries and reports recovery.`, + error, + ); + } + return; + } + + if (this.historySeqFailureReported) { + this.historySeqFailureReported = false; + console.info( + `[Metadata] \`${this.historyTableName}\` is readable again — \`event_seq\` numbering recovered and change ` + + `history is being recorded again. Entries skipped during the outage are not backfilled.`, + ); + } const historyRecord: Partial = { id: historyId, diff --git a/packages/metadata/src/utils/schema-sync-errors.test.ts b/packages/metadata/src/utils/schema-sync-errors.test.ts index 04ea9fbde1..fd187090da 100644 --- a/packages/metadata/src/utils/schema-sync-errors.test.ts +++ b/packages/metadata/src/utils/schema-sync-errors.test.ts @@ -1,15 +1,17 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * #4728 — the classification that decides whether a DDL failure may be silenced. + * #4728 / #4825 — the classifications that decide whether a driver failure may + * be silenced. * - * Both directions are pinned deliberately. A test suite that only proves the - * benign case is recognised would pass just as happily on `() => true`, which is - * exactly the bug being fixed (one benign reason excusing every reason). + * Both directions are pinned deliberately, for both predicates. A test suite + * that only proves the benign case is recognised would pass just as happily on + * `() => true`, which is exactly the bug being fixed (one benign reason excusing + * every reason). */ import { describe, it, expect } from 'vitest'; -import { isSchemaAlreadyExistsError } from './schema-sync-errors.js'; +import { isMissingTableError, isSchemaAlreadyExistsError } from './schema-sync-errors.js'; describe('isSchemaAlreadyExistsError', () => { describe('benign — the table/column is already provisioned', () => { @@ -115,3 +117,154 @@ describe('isSchemaAlreadyExistsError', () => { }); }); }); + +/** + * #4825 — the READ-side counterpart. `nextEventSeq()` may only answer "1" when + * the table genuinely has no rows to collide with; every other read failure has + * to stay loud, because the rows are still there and were merely not seen. + */ +describe('isMissingTableError', () => { + describe('benign — the table has not been provisioned yet', () => { + it('recognises the SQLite message (code is the undifferentiated SQLITE_ERROR)', () => { + const err = Object.assign(new Error('no such table: sys_metadata_history'), { + code: 'SQLITE_ERROR', + }); + expect(isMissingTableError(err)).toBe(true); + }); + + it('recognises PostgreSQL undefined_table by SQLSTATE, even with an opaque message', () => { + expect(isMissingTableError(Object.assign(new Error('db error'), { code: '42P01' }))).toBe( + true, + ); + }); + + it('recognises the PostgreSQL undefined_table message', () => { + const err = new Error('relation "sys_metadata_history" does not exist'); + expect(isMissingTableError(err)).toBe(true); + }); + + it('recognises MySQL ER_NO_SUCH_TABLE by code, by errno and by message', () => { + expect( + isMissingTableError( + Object.assign(new Error('opaque'), { code: 'ER_NO_SUCH_TABLE' }), + ), + ).toBe(true); + expect(isMissingTableError(Object.assign(new Error('opaque'), { errno: 1146 }))).toBe( + true, + ); + expect( + isMissingTableError(new Error("Table 'app.sys_metadata_history' doesn't exist")), + ).toBe(true); + }); + + it('follows an error wrapped as `cause`', () => { + const inner = Object.assign(new Error('no such table: sys_metadata_history'), { + code: 'SQLITE_ERROR', + }); + const outer = Object.assign(new Error('find failed'), { cause: inner }); + expect(isMissingTableError(outer)).toBe(true); + }); + + it('accepts a bare thrown string', () => { + expect(isMissingTableError('no such table: sys_metadata_history')).toBe(true); + }); + }); + + describe('NOT benign — the rows may exist and simply were not read', () => { + it('rejects a dropped connection', () => { + const err = Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + expect(isMissingTableError(err)).toBe(false); + }); + + it('rejects a statement timeout', () => { + const err = Object.assign(new Error('canceling statement due to statement timeout'), { + code: '57014', + }); + expect(isMissingTableError(err)).toBe(false); + }); + + it('rejects insufficient privileges on an EXISTING table', () => { + const err = Object.assign( + new Error('permission denied for table sys_metadata_history'), + { code: '42501' }, + ); + expect(isMissingTableError(err)).toBe(false); + }); + + it('rejects a locked/busy database', () => { + const err = Object.assign(new Error('database is locked'), { code: 'SQLITE_BUSY' }); + expect(isMissingTableError(err)).toBe(false); + }); + + /** + * The reason the message test demands the word table/relation instead of + * a bare "does not exist". Each of these is a REAL failure against a + * table that may be full of rows — classifying any of them benign would + * restart numbering at 1 and collide. + */ + it('rejects other "does not exist" objects — role, database, column', () => { + expect( + isMissingTableError( + Object.assign(new Error('role "app_rw" does not exist'), { code: '42704' }), + ), + ).toBe(false); + expect( + isMissingTableError( + Object.assign(new Error('database "objectstack" does not exist'), { + code: '3D000', + }), + ), + ).toBe(false); + expect( + isMissingTableError( + Object.assign(new Error('column "event_seq" does not exist'), { code: '42703' }), + ), + ).toBe(false); + }); + + it('rejects values that carry no signal at all', () => { + expect(isMissingTableError(undefined)).toBe(false); + expect(isMissingTableError(null)).toBe(false); + expect(isMissingTableError(new Error(''))).toBe(false); + expect(isMissingTableError({})).toBe(false); + expect(isMissingTableError(42)).toBe(false); + }); + + it('does not follow a cause chain forever', () => { + let err: Error = new Error('no such table: sys_metadata_history'); + for (let i = 0; i < 8; i++) { + err = Object.assign(new Error(`wrap ${i}`), { cause: err }); + } + expect(isMissingTableError(err)).toBe(false); + }); + }); +}); + +/** + * The two predicates share one matcher but must never collapse into each + * other's negation: both ask "is this THE one benign reason?", and an error + * neither recognises has to be loud under both. + */ +describe('the two classifications are independent, not complementary', () => { + it('an "already exists" DDL error is not a missing-table read error', () => { + const err = Object.assign(new Error('table sys_metadata_history already exists'), { + code: '42P07', + }); + expect(isSchemaAlreadyExistsError(err)).toBe(true); + expect(isMissingTableError(err)).toBe(false); + }); + + it('a missing-table read error is not a benign DDL error', () => { + const err = Object.assign(new Error('no such table: sys_metadata_history'), { + code: '42P01', + }); + expect(isMissingTableError(err)).toBe(true); + expect(isSchemaAlreadyExistsError(err)).toBe(false); + }); + + it('an unrecognised failure is benign under NEITHER — the default is loud', () => { + const err = Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + expect(isSchemaAlreadyExistsError(err)).toBe(false); + expect(isMissingTableError(err)).toBe(false); + }); +}); diff --git a/packages/metadata/src/utils/schema-sync-errors.ts b/packages/metadata/src/utils/schema-sync-errors.ts index 1df1c22305..f23bd8cb6e 100644 --- a/packages/metadata/src/utils/schema-sync-errors.ts +++ b/packages/metadata/src/utils/schema-sync-errors.ts @@ -1,7 +1,27 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * DDL failure classification for metadata schema sync (#4728, rule from #4632). + * Driver-error classification for the metadata storage seams (#4728, #4825; + * rule from #4632). + * + * Two questions live here, and they share one mechanism on purpose. A second + * hand-rolled `catch`-and-guess elsewhere in this package would be a second + * de-facto vocabulary of "which driver errors are benign" — the exact debt this + * module exists to retire. Both predicates below are thin wrappers over one + * signature matcher, so a driver quirk is taught to the package once. + * + * 1. {@link isSchemaAlreadyExistsError} — "was this DDL failure just the table + * already being there?" (#4728, `ensureSchema` / `ensureHistorySchema`). + * 2. {@link isMissingTableError} — "did this READ fail because the table has + * not been provisioned yet?" (#4825, `nextEventSeq`). + * + * They are deliberately **not** each other's negation. Each answers "is this + * the one benign reason?" and defaults to *not benign*, so an error neither + * recognises is loud under both. + * + * --- + * + * ## 1. DDL failure classification (#4728) * * `IDataDriver.syncSchema()` is contractually **idempotent** ("creates tables if * missing, adds columns, updates indexes"), so in principle a re-sync of an @@ -36,54 +56,117 @@ * recognised as "already exists" is treated as a real failure, because the cost * of a false "benign" (silent data loss) is far higher than the cost of a false * "real" (one extra error line). + * + * --- + * + * ## 2. Missing-table classification for reads (#4825) + * + * `DatabaseLoader.nextEventSeq()` reads `sys_metadata_history` to decide what + * `event_seq` the NEXT history row gets. Its `catch` named both reasons a read + * can fail — "table not provisioned yet" (benign: 1 really is the next number) + * and "driver error" (**not** benign) — and answered both with `return 1`. + * + * That is the #4728 shape one layer down, but the damage is the opposite kind + * and worse. #4728 was *bytes that never landed*; this is **bytes that land + * wrong**: with N rows already in the table, one flaky read hands the next row + * `event_seq = 1`, colliding with an existing row. The insert **succeeds**, no + * line is logged, and `event_seq` — the ordering key that history listing and + * rollback targeting both stand on — is now silently untrustworthy. + * + * So the read seam gets the same treatment, with the same conservative default: + * + * ```ts + * catch (error) { + * if (isMissingTableError(error)) return 1; // benign: nothing to collide with + * throw error; // caller reports the consequence + * } + * ``` */ +/** One "which errors mean X?" vocabulary, in the three forms drivers use. */ +interface DriverErrorSignature { + /** `error.code` — Postgres SQLSTATE, or mysql2's symbolic name. */ + readonly codes: ReadonlySet; + /** `error.errno` — MySQL/MariaDB numeric equivalents. */ + readonly errnos: ReadonlySet; + /** `error.message` — the only signal SQLite-family drivers give. */ + readonly message: RegExp; +} + /** * Driver/SQLSTATE codes that mean "the thing you asked me to create is already * there". Postgres reports SQLSTATE on `code`; mysql2 reports its symbolic name. */ -const ALREADY_EXISTS_CODES: ReadonlySet = new Set([ - // PostgreSQL SQLSTATE (class 42 — syntax error or access rule violation) - '42P07', // duplicate_table - '42701', // duplicate_column - '42710', // duplicate_object — index / constraint already exists - // MySQL / MariaDB (mysql2 puts the symbolic name on `code`) - 'ER_TABLE_EXISTS_ERROR', // 1050 - 'ER_DUP_FIELDNAME', // 1060 - 'ER_DUP_KEYNAME', // 1061 -]); - -/** MySQL/MariaDB numeric equivalents of the codes above (`errno`). */ -const ALREADY_EXISTS_ERRNOS: ReadonlySet = new Set([1050, 1060, 1061]); +const ALREADY_EXISTS: DriverErrorSignature = { + codes: new Set([ + // PostgreSQL SQLSTATE (class 42 — syntax error or access rule violation) + '42P07', // duplicate_table + '42701', // duplicate_column + '42710', // duplicate_object — index / constraint already exists + // MySQL / MariaDB (mysql2 puts the symbolic name on `code`) + 'ER_TABLE_EXISTS_ERROR', // 1050 + 'ER_DUP_FIELDNAME', // 1060 + 'ER_DUP_KEYNAME', // 1061 + ]), + errnos: new Set([1050, 1060, 1061]), + /** + * Message fallback for drivers that carry no machine-readable code — + * notably SQLite, whose `code` is the undifferentiated `SQLITE_ERROR` for + * every DDL failure, so the message is the only signal available: + * - `table sys_metadata already exists` + * - `duplicate column name: environment_id` + * - `index idx_x already exists` + * Postgres phrases its own as `relation "x" already exists` / + * `column "x" of relation "y" already exists`, which matches the same test. + */ + message: /already exists|duplicate column name|duplicate key name/i, +}; /** - * Message fallback for drivers that carry no machine-readable code — notably - * SQLite, whose `code` is the undifferentiated `SQLITE_ERROR` for every DDL - * failure, so the message is the only signal available: - * - `table sys_metadata already exists` - * - `duplicate column name: environment_id` - * - `index idx_x already exists` - * Postgres phrases its own as `relation "x" already exists` / - * `column "x" of relation "y" already exists`, which matches the same test. + * Codes/messages that mean "the table you tried to READ has not been created". + * + * Narrower than it looks, on purpose. `does not exist` on its own also covers + * `role "x" does not exist` (42704), `database "x" does not exist` (3D000) and + * `column "x" does not exist` (42703) — every one of them a **real** failure + * that must stay loud, and every one of them a case where "start numbering at + * 1" would be the wrong answer against a table that may be full of rows. So the + * message test demands the word table/relation next to the phrase rather than + * the phrase alone, and the code set carries only the table-scoped SQLSTATEs. */ -const ALREADY_EXISTS_MESSAGE = /already exists|duplicate column name|duplicate key name/i; +const MISSING_TABLE: DriverErrorSignature = { + codes: new Set([ + '42P01', // PostgreSQL undefined_table + 'ER_NO_SUCH_TABLE', // MySQL / MariaDB 1146 + ]), + errnos: new Set([1146]), + /** + * - SQLite / libsql: `no such table: sys_metadata_history` + * - PostgreSQL: `relation "sys_metadata_history" does not exist` + * - MySQL/MariaDB: `Table 'app.sys_metadata_history' doesn't exist` + */ + message: + /no such table|relation ["'`][^"'`]+["'`] does not exist|table ["'`][^"'`]+["'`] doesn'?t exist|unknown table/i, +}; /** How far to follow an `error.cause` chain — drivers wrap, but not deeply. */ const MAX_CAUSE_DEPTH = 4; /** - * Is this DDL error the benign "already provisioned" case? + * The single matcher both predicates run on: code, then errno, then message, + * then one step down the `cause` chain. * - * @param error - The value thrown by `syncSchema()` (or any DDL call). - * @returns `true` only when the error positively identifies as - * table/column/index-already-exists. Anything else — including an - * unrecognised error, `undefined`, or a permission/connection failure — - * returns `false` and MUST be reported loudly by the caller. + * Unrecognised is always `false` — a benign verdict must be *earned*, never + * defaulted to, because a false "benign" corrupts data while a false "real" + * costs one error line. */ -export function isSchemaAlreadyExistsError(error: unknown, depth = 0): boolean { +function matchesDriverError( + error: unknown, + signature: DriverErrorSignature, + depth: number, +): boolean { if (error === null || error === undefined || depth > MAX_CAUSE_DEPTH) return false; - if (typeof error === 'string') return ALREADY_EXISTS_MESSAGE.test(error); + if (typeof error === 'string') return signature.message.test(error); if (typeof error !== 'object') return false; const err = error as { @@ -93,10 +176,43 @@ export function isSchemaAlreadyExistsError(error: unknown, depth = 0): boolean { cause?: unknown; }; - if (typeof err.code === 'string' && ALREADY_EXISTS_CODES.has(err.code)) return true; - if (typeof err.errno === 'number' && ALREADY_EXISTS_ERRNOS.has(err.errno)) return true; - if (typeof err.message === 'string' && ALREADY_EXISTS_MESSAGE.test(err.message)) return true; + if (typeof err.code === 'string' && signature.codes.has(err.code)) return true; + if (typeof err.errno === 'number' && signature.errnos.has(err.errno)) return true; + if (typeof err.message === 'string' && signature.message.test(err.message)) return true; // Drivers commonly re-throw with the original attached as `cause`. - return isSchemaAlreadyExistsError(err.cause, depth + 1); + return matchesDriverError(err.cause, signature, depth + 1); +} + +/** + * Is this DDL error the benign "already provisioned" case? + * + * @param error - The value thrown by `syncSchema()` (or any DDL call). + * @param depth - Internal `cause`-chain recursion counter; callers pass nothing. + * @returns `true` only when the error positively identifies as + * table/column/index-already-exists. Anything else — including an + * unrecognised error, `undefined`, or a permission/connection failure — + * returns `false` and MUST be reported loudly by the caller. + */ +export function isSchemaAlreadyExistsError(error: unknown, depth = 0): boolean { + return matchesDriverError(error, ALREADY_EXISTS, depth); +} + +/** + * Is this READ error the benign "table has not been provisioned yet" case? + * + * The only failure that licenses a caller to treat an empty table as the truth + * — there are no rows, so there is nothing to be inconsistent with. A + * connection drop, a timeout, a permission denial or a query error all mean the + * rows may well exist and simply were not seen; those return `false` and the + * caller must report the consequence and give up rather than compute an answer + * from data it never read (#4825). + * + * @param error - The value thrown by a driver/engine read (`find`, `findOne`, …). + * @param depth - Internal `cause`-chain recursion counter; callers pass nothing. + * @returns `true` only when the error positively identifies as + * table/relation-does-not-exist. + */ +export function isMissingTableError(error: unknown, depth = 0): boolean { + return matchesDriverError(error, MISSING_TABLE, depth); }