Skip to content

Commit c4ab50b

Browse files
os-zhuangclaude
andauthored
fix(metadata): sys_metadata 的 DDL 失败必须响亮,只静默「表已存在」一种 (#4728) (#4823)
`DatabaseLoader.ensureSchema()` 过去用一个空 `catch` 吞掉全部 DDL 失败,并且照样 把 `schemaReady` 置为 `true` —— 注释里的免责理由("e.g. table already exists") 只覆盖了最良性的一种原因,却为**所有**原因开脱。权限不足 / 数据源未连上 / 列类型 冲突之后,表或新列压根不存在,而进程状态与成功路径逐字节相同,日志里一行痕迹也 没有。这正是 #4420 的形态,#4632 已把它定成规则并落地了机械检查。 改为按错误类型判别: - 新增内部工具 `isSchemaAlreadyExistsError()`,按驱动错误码(Postgres SQLSTATE 42P07/42701/42710、MySQL ER_TABLE_EXISTS_ERROR/ER_DUP_FIELDNAME/ER_DUP_KEYNAME 及 errno、SQLite 只能靠消息)判别,并跟随 `cause` 链;凡是没有被正面识别为 「已存在」的,一律当作真实失败。 - 良性「已存在」:表确实已就绪,静默通过,并照常执行后续迁移与 ADR-0005 索引。 - 其余失败:`console.error` 上报后果(表/列未创建,后续元数据写入不持久,而服务器 仍报告健康)与修复动作(修掉驱动/数据源错误后重启),且只说一次。 - 真实失败后 `schemaReady` 不再置 `true`:启动依旧不被阻断(方法不抛),但 loader 不再声称它并不具备的就绪状态,下一次操作会重试,瞬时故障可自愈(恢复补一条 info)。 - `ensureHistorySchema()` 按同一规则对齐,两处不再一边过度静默、一边过度报错。 删除 `scripts/durability-degradation.baseline.json` 中指向本单的条目(shrink-only)。 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny Co-authored-by: Claude <noreply@anthropic.com>
1 parent cb680f2 commit c4ab50b

6 files changed

Lines changed: 555 additions & 27 deletions

File tree

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
"@objectstack/metadata": patch
3+
---
4+
5+
fix(metadata): `sys_metadata` 的 DDL 失败不再被静默吞掉 —— 只有「表已存在」这一种原因可以静音 (#4728)
6+
7+
`DatabaseLoader.ensureSchema()` 过去用一个空 `catch` 吞掉 **全部** DDL 失败,并且照样把
8+
`schemaReady` 置为 `true`:
9+
10+
```ts
11+
} catch {
12+
// If syncSchema fails (e.g. table already exists), mark ready and continue
13+
this.schemaReady = true;
14+
}
15+
```
16+
17+
注释里的免责理由只覆盖了失败原因中最良性的一种,却用它为**所有**原因开脱。真实的失败
18+
(权限不足、数据源根本没连上、列类型冲突)之后,表或新列压根不存在,而进程的状态与成功
19+
路径**逐字节相同**,启动日志里一行痕迹都没有 —— 这正是 #4420 的形态:声称已持久化、实
20+
际没落盘、系统看起来完全健康。#4632 把它定成规则(AGENTS.md → "Degradation log levels"),
21+
机械检查 `pnpm check:durability-log-level` 已经能发现这一处。
22+
23+
现在按**错误类型**判别,而不是按注释里的乐观假设:
24+
25+
- **良性的「已存在」**(SQLite 的 `table … already exists` / `duplicate column name`
26+
Postgres 的 SQLSTATE `42P07`/`42701`/`42710`、MySQL 的 `ER_TABLE_EXISTS_ERROR` 等及其
27+
`errno`,并跟随 `cause` 链)—— 表确实已就绪,当作 no-op 静默通过,并照常执行后续的
28+
`project_id → environment_id` 迁移与 ADR-0005 索引。
29+
- **其余一切失败** —— 以 `console.error` 上报,文案同时说清**后果**(`sys_metadata` 的表/
30+
列未创建,后续每一次元数据写入都会报错、或在宽松驱动上悄悄丢列,而服务器仍报告健康)
31+
**修复动作**(修掉下面那条驱动/数据源错误后重启)。只说**一次**,不是每次写入都刷屏。
32+
- `schemaReady` **不再**在真实失败后置 `true`。启动依旧不被阻断(该方法不抛),但 loader
33+
不再声称一个它并不具备的就绪状态,下一次元数据操作会重试 —— 数据源只是还在连接这类瞬
34+
时故障因此可以自愈,恢复时补一条 `info`
35+
36+
`ensureHistorySchema()` 按同一规则对齐:良性「已存在」不再每次写入都打一条 `error`(过度
37+
使用 `error` 是镜像失败),真实失败则同样只响亮一次并保持重试。
38+
39+
无 API / schema 变更;新增内部工具 `isSchemaAlreadyExistsError()`(未从包入口导出)。
40+
`scripts/durability-degradation.baseline.json` 中指向本单的条目随之删除(该文件 shrink-only)。

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

Lines changed: 209 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

3-
import { describe, it, expect, vi, beforeEach } from 'vitest';
3+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
44
import { DatabaseLoader, type DatabaseLoaderOptions } from './database-loader';
55
import type { IDataDriver } from '@objectstack/spec/contracts';
66
import { MetadataManager } from '../metadata-manager';
@@ -433,6 +433,214 @@ describe('DatabaseLoader', () => {
433433
});
434434
});
435435

436+
// ---------- DDL failure is loud, and only "already exists" is silent ----------
437+
438+
/**
439+
* #4728 (rule: #4632, accident: #4420).
440+
*
441+
* `ensureSchema()` used to `catch {}` every DDL failure and set
442+
* `schemaReady = true` regardless — a total durability failure was byte-for-byte
443+
* indistinguishable from success, with no log line at all. The comment excused
444+
* *all* failure reasons with the most benign one ("e.g. table already exists").
445+
*
446+
* Both directions are pinned here on purpose: proving the real failure is loud
447+
* is not enough, because "always log error" would pass that alone while making
448+
* the benign case unreadable noise. The point is that the two are DISTINGUISHED.
449+
*/
450+
describe('DatabaseLoader schema-sync failure reporting (#4728)', () => {
451+
let errorSpy: ReturnType<typeof vi.spyOn>;
452+
let infoSpy: ReturnType<typeof vi.spyOn>;
453+
454+
/** A real DDL failure: the table does NOT exist afterwards. */
455+
const permissionDenied = () =>
456+
Object.assign(new Error('permission denied for schema public'), { code: '42501' });
457+
458+
/** The one benign reason: the table IS already provisioned. */
459+
const alreadyExists = () =>
460+
Object.assign(new Error('table sys_metadata already exists'), { code: 'SQLITE_ERROR' });
461+
462+
beforeEach(() => {
463+
errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
464+
infoSpy = vi.spyOn(console, 'info').mockImplementation(() => {});
465+
});
466+
467+
afterEach(() => {
468+
errorSpy.mockRestore();
469+
infoSpy.mockRestore();
470+
});
471+
472+
describe('a REAL DDL failure', () => {
473+
it('reports at error, naming the consequence and the fix', async () => {
474+
const driver = createMockDriver();
475+
driver.syncSchema = vi.fn().mockRejectedValue(permissionDenied());
476+
const loader = new DatabaseLoader({ driver });
477+
478+
await loader.list('object');
479+
480+
expect(errorSpy).toHaveBeenCalledTimes(1);
481+
const [message, cause] = errorSpy.mock.calls[0] as [string, unknown];
482+
// consequence
483+
expect(message).toContain('sys_metadata');
484+
expect(message).toContain('FAILED');
485+
expect(message).toContain('NOT created');
486+
// the system keeps looking healthy — that is the whole point of the level
487+
expect(message).toMatch(/reporting healthy/i);
488+
// fix
489+
expect(message).toMatch(/fix it and restart/i);
490+
// and the underlying driver error is carried, not discarded
491+
expect((cause as Error).message).toBe('permission denied for schema public');
492+
});
493+
494+
it('does NOT mark the schema ready — the next operation retries the DDL', async () => {
495+
const driver = createMockDriver();
496+
driver.syncSchema = vi.fn().mockRejectedValue(permissionDenied());
497+
const loader = new DatabaseLoader({ driver });
498+
499+
await loader.list('object');
500+
await loader.list('view');
501+
await loader.exists('object', 'account');
502+
503+
// Before #4728 this was 1: the failure set `schemaReady = true` and every
504+
// later write proceeded against a table that was never created.
505+
expect(driver.syncSchema).toHaveBeenCalledTimes(3);
506+
});
507+
508+
it('says it once, not once per operation', async () => {
509+
const driver = createMockDriver();
510+
driver.syncSchema = vi.fn().mockRejectedValue(permissionDenied());
511+
const loader = new DatabaseLoader({ driver });
512+
513+
await loader.list('object');
514+
await loader.list('view');
515+
await loader.list('flow');
516+
517+
expect(errorSpy).toHaveBeenCalledTimes(1);
518+
});
519+
520+
it('recovers silently-loudly: a transient failure that heals reports the recovery', async () => {
521+
const driver = createMockDriver();
522+
driver.syncSchema = vi
523+
.fn()
524+
.mockRejectedValueOnce(
525+
Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:5432'), {
526+
code: 'ECONNREFUSED',
527+
}),
528+
)
529+
.mockResolvedValue(undefined);
530+
const loader = new DatabaseLoader({ driver });
531+
532+
await loader.list('object'); // datasource still connecting → loud
533+
await loader.list('view'); // retried → succeeds
534+
535+
expect(errorSpy).toHaveBeenCalledTimes(1);
536+
expect(infoSpy).toHaveBeenCalledTimes(1);
537+
expect((infoSpy.mock.calls[0] as [string])[0]).toMatch(/succeeded on retry/i);
538+
539+
// Ready now, so a third operation does not re-run the DDL.
540+
await loader.list('flow');
541+
expect(driver.syncSchema).toHaveBeenCalledTimes(2);
542+
});
543+
});
544+
545+
describe('the benign "already exists" failure', () => {
546+
it('is silent — no error, no info', async () => {
547+
const driver = createMockDriver();
548+
driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
549+
const loader = new DatabaseLoader({ driver });
550+
551+
await loader.list('object');
552+
553+
expect(errorSpy).not.toHaveBeenCalled();
554+
expect(infoSpy).not.toHaveBeenCalled();
555+
});
556+
557+
it('marks the schema ready — the table is provisioned, so no retry', async () => {
558+
const driver = createMockDriver();
559+
driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
560+
const loader = new DatabaseLoader({ driver });
561+
562+
await loader.list('object');
563+
await loader.list('view');
564+
565+
expect(driver.syncSchema).toHaveBeenCalledTimes(1);
566+
});
567+
568+
it('still runs the post-sync migrations (the table exists, so they apply)', async () => {
569+
const driver = createMockDriver();
570+
driver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
571+
const raw = vi.fn().mockResolvedValue(undefined);
572+
(driver as unknown as { raw: unknown }).raw = raw;
573+
const loader = new DatabaseLoader({ driver });
574+
575+
await loader.list('object');
576+
577+
expect(raw).toHaveBeenCalled();
578+
expect(raw.mock.calls.some(([sql]) => String(sql).includes('idx_sys_metadata_overlay_active'))).toBe(
579+
true,
580+
);
581+
});
582+
});
583+
584+
it('DISTINGUISHES the two: same call site, opposite verdicts', async () => {
585+
const benignDriver = createMockDriver();
586+
benignDriver.syncSchema = vi.fn().mockRejectedValue(alreadyExists());
587+
const realDriver = createMockDriver();
588+
realDriver.syncSchema = vi.fn().mockRejectedValue(permissionDenied());
589+
590+
await new DatabaseLoader({ driver: benignDriver }).list('object');
591+
const afterBenign = errorSpy.mock.calls.length;
592+
593+
await new DatabaseLoader({ driver: realDriver }).list('object');
594+
const afterReal = errorSpy.mock.calls.length;
595+
596+
expect(afterBenign).toBe(0);
597+
expect(afterReal).toBe(1);
598+
});
599+
600+
describe('the history table follows the same rule', () => {
601+
/** sys_metadata syncs fine; only the history table's DDL fails. */
602+
function driverWithFailingHistoryDdl(error: unknown): IDataDriver {
603+
const driver = createMockDriver();
604+
driver.syncSchema = vi.fn().mockImplementation((table: string) => {
605+
if (table === 'sys_metadata_history') return Promise.reject(error);
606+
return Promise.resolve(undefined);
607+
});
608+
return driver;
609+
}
610+
611+
it('reports a real failure at error, naming the lost audit trail and the fix', async () => {
612+
const driver = driverWithFailingHistoryDdl(permissionDenied());
613+
const loader = new DatabaseLoader({ driver });
614+
615+
await loader.save('object', 'account', { name: 'account' });
616+
617+
expect(errorSpy).toHaveBeenCalledTimes(1);
618+
const message = (errorSpy.mock.calls[0] as [string])[0];
619+
expect(message).toContain('sys_metadata_history');
620+
expect(message).toMatch(/will NOT be persisted/);
621+
expect(message).toMatch(/restart/i);
622+
});
623+
624+
it('is silent on "already exists" and stops retrying', async () => {
625+
const driver = driverWithFailingHistoryDdl(
626+
Object.assign(new Error("Table 'sys_metadata_history' already exists"), {
627+
code: 'ER_TABLE_EXISTS_ERROR',
628+
}),
629+
);
630+
const loader = new DatabaseLoader({ driver });
631+
632+
await loader.save('object', 'account', { name: 'account' });
633+
await loader.save('object', 'contact', { name: 'contact' });
634+
635+
expect(errorSpy).not.toHaveBeenCalled();
636+
const historySyncs = (driver.syncSchema as ReturnType<typeof vi.fn>).mock.calls.filter(
637+
([table]) => table === 'sys_metadata_history',
638+
);
639+
expect(historySyncs).toHaveLength(1);
640+
});
641+
});
642+
});
643+
436644
// ---------- DatabaseLoader read-through cache ----------
437645

438646
describe('DatabaseLoader read-through cache', () => {

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

Lines changed: 86 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import type { IDataDriver, IDataEngine } from '@objectstack/spec/contracts';
2626
import type { MetadataLoader } from './loader-interface.js';
2727
import { calculateChecksum } from '../utils/metadata-history-utils.js';
2828
import { LRUCache } from '../utils/lru-cache.js';
29+
import { isSchemaAlreadyExistsError } from '../utils/schema-sync-errors.js';
2930
import { addSysMetadataOverlayIndex } from '../migrations/add-sys-metadata-overlay-index.js';
3031
import { migrateProjectIdToEnvironmentId } from '../migrations/migrate-project-id-to-environment-id.js';
3132

@@ -123,6 +124,13 @@ export class DatabaseLoader implements MetadataLoader {
123124
private trackHistory: boolean;
124125
private schemaReady = false;
125126
private historySchemaReady = false;
127+
/**
128+
* Whether the loud "DDL failed" report has already been printed for the
129+
* metadata table / history table respectively. AGENTS.md → "Degradation log
130+
* levels": say it **once**, at the first degradation, not once per retry.
131+
*/
132+
private schemaFailureReported = false;
133+
private historySchemaFailureReported = false;
126134

127135
/** (type, name) → metadata payload — primes `load()` */
128136
private readonly loadCache?: LRUCache<string, Record<string, unknown> | null>;
@@ -321,22 +329,59 @@ export class DatabaseLoader implements MetadataLoader {
321329
...SysMetadataObject,
322330
name: this.tableName,
323331
});
324-
this.schemaReady = true;
325-
// v5.0 forward migration: project_id → environment_id (idempotent).
326-
try {
327-
await migrateProjectIdToEnvironmentId(this.driver!);
328-
} catch {
329-
// ignore — migration is best-effort on bootstrap
330-
}
331-
// Apply ADR-0005 partial UNIQUE INDEX (best-effort, idempotent)
332-
try {
333-
await addSysMetadataOverlayIndex(this.driver!);
334-
} catch {
335-
// ignore — index is optimization
332+
} catch (error) {
333+
// #4728 (rule: #4632, accident: #4420) — discriminate by error TYPE.
334+
// Exactly ONE failure reason is benign here: the table/columns are
335+
// already provisioned and a non-fully-idempotent driver reports that as
336+
// an error. Every other reason (insufficient privileges, datasource never
337+
// connected, incompatible column type) means the table or column does NOT
338+
// exist — and the previous code marked `schemaReady = true` for all of
339+
// them, making a total durability failure indistinguishable from success
340+
// with not one line in the log.
341+
if (!isSchemaAlreadyExistsError(error)) {
342+
if (!this.schemaFailureReported) {
343+
this.schemaFailureReported = true;
344+
console.error(
345+
`[Metadata] DDL for the metadata table \`${this.tableName}\` FAILED — its table/columns were NOT created or altered. ` +
346+
`Every metadata write from here on (Studio saves, app installs, org overlays) targets storage that may not exist: ` +
347+
`writes will error out, or silently drop columns on a lenient driver, while the server keeps reporting healthy. ` +
348+
`This is NOT the benign "already exists" case — check the datasource/driver error below (insufficient privileges, ` +
349+
`datasource not connected, incompatible column type), fix it and restart. Schema sync is retried on the next ` +
350+
`metadata operation, so a transient cause recovers on its own.`,
351+
error,
352+
);
353+
}
354+
// Deliberate, and the opposite of what this code did before: on a REAL
355+
// DDL failure `schemaReady` stays FALSE. Startup is still not blocked
356+
// (this method does not throw — callers proceed and fail loudly at the
357+
// driver if the table is truly missing), but the loader never claims a
358+
// readiness it does not have, and the next operation retries the sync
359+
// so a datasource that was merely still connecting heals itself. Same
360+
// shape as `ensureHistorySchema()` below.
361+
return;
336362
}
363+
// Benign — and ONLY benign: the table is already provisioned, so the DDL
364+
// was a no-op rather than a failure. Fall through to the ready path.
365+
}
366+
367+
if (this.schemaFailureReported) {
368+
this.schemaFailureReported = false;
369+
console.info(
370+
`[Metadata] DDL for the metadata table \`${this.tableName}\` succeeded on retry — metadata writes are durable again.`,
371+
);
372+
}
373+
this.schemaReady = true;
374+
// v5.0 forward migration: project_id → environment_id (idempotent).
375+
try {
376+
await migrateProjectIdToEnvironmentId(this.driver!);
337377
} catch {
338-
// If syncSchema fails (e.g. table already exists), mark ready and continue
339-
this.schemaReady = true;
378+
// ignore — migration is best-effort on bootstrap
379+
}
380+
// Apply ADR-0005 partial UNIQUE INDEX (best-effort, idempotent)
381+
try {
382+
await addSysMetadataOverlayIndex(this.driver!);
383+
} catch {
384+
// ignore — index is optimization
340385
}
341386
}
342387

@@ -358,11 +403,35 @@ export class DatabaseLoader implements MetadataLoader {
358403
...SysMetadataHistoryObject,
359404
name: this.historyTableName,
360405
});
406+
if (this.historySchemaFailureReported) {
407+
this.historySchemaFailureReported = false;
408+
console.info(
409+
`[Metadata] DDL for the metadata history table \`${this.historyTableName}\` succeeded on retry — change history is being recorded again.`,
410+
);
411+
}
361412
this.historySchemaReady = true;
362413
} catch (error) {
363-
// Log the error; historySchemaReady remains false so the next operation retries.
364-
// If the error is a benign "already exists" the next attempt will also succeed.
365-
console.error('Failed to ensure history schema, will retry on next operation:', error);
414+
// Same discrimination as `ensureSchema()` above (#4728). A benign
415+
// "already exists" means the history table IS provisioned — treat it as
416+
// the no-op it is instead of re-reporting it (and re-running the DDL) on
417+
// every single write, which is the mirror-image failure: an `error` line
418+
// for a non-degradation trains everyone to skim `error`.
419+
if (isSchemaAlreadyExistsError(error)) {
420+
this.historySchemaReady = true;
421+
return;
422+
}
423+
// Real failure: loud once, `historySchemaReady` stays false so the next
424+
// operation retries.
425+
if (!this.historySchemaFailureReported) {
426+
this.historySchemaFailureReported = true;
427+
console.error(
428+
`[Metadata] DDL for the metadata history table \`${this.historyTableName}\` FAILED — its table/columns were NOT created. ` +
429+
`Metadata change history (versions, diffs, rollback) will NOT be persisted while every metadata write keeps succeeding, ` +
430+
`so the audit trail silently ends here. Fix the datasource/driver error below and restart; the sync is retried on the ` +
431+
`next metadata operation.`,
432+
error,
433+
);
434+
}
366435
}
367436
}
368437

0 commit comments

Comments
 (0)