From 6776f25717eadc1d1f00a6505356d70551b694fb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 02:34:34 +0000 Subject: [PATCH] =?UTF-8?q?fix(metadata-protocol):=20listCommits=20?= =?UTF-8?q?=E6=8A=8A=20commit=20store=20outage=20=E4=B8=8A=E6=8A=9B=20503,?= =?UTF-8?q?=E4=B8=8D=E5=86=8D=E7=AD=94=E6=88=90=E3=80=8C=E6=97=A0=E5=8E=86?= =?UTF-8?q?=E5=8F=B2=E3=80=8D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `listCommits` 读 `sys_metadata_commit` 的 catch 对任何失败都返回 `[]`,零日志、 不按错误类型区分 —— 它的 JSDoc 还把这写成了设计("Returns [] if the commit store is unavailable"),于是后来者照着读成契约。 这是 ADR-0110 D3 在 ADR-0067 提交时间线上的违反:miss 与 outage 是意义相反的 两个事实。后果面朝向 revert,因为这条时间线正是 revertCommit 的选择面: GET /packages/:id/commits → { commits: [] },故障期间 UI 显示「无可回滚项」 rollbackToPackageCommit → 过滤同一个 [],一次都没回滚却返回 success: true 第二条是本 seam 比同族其它读更尖的地方:别的读答错的是一个问题,这一处把一次 没做的写报成了完成(实测复现:{"success":true,"revertedCommits":[],"failed":[]})。 改为按错误类型区分,走本文件既有的 rethrowUnlessMetadataStoreUnprovisioned (#5532 为 getMetaItems 引入,#5707 / #5840 沿用):表未 provision 的首启仍返回 `[]`;其余失败包成 503 SERVICE_UNAVAILABLE 上抛,驱动原始错误挂 cause。无新 依赖、无新 import。 JSDoc 改为如实措辞;scripts/durability-read-invention.baseline.json 摘除 `protocol.ts::listCommits` 条目(shrink-only)。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --- .changeset/listcommits-outage-503.md | 11 ++ .../protocol.metadata-store-outage.test.ts | 151 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 34 +++- .../durability-read-invention.baseline.json | 9 -- 4 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 .changeset/listcommits-outage-503.md diff --git a/.changeset/listcommits-outage-503.md b/.changeset/listcommits-outage-503.md new file mode 100644 index 0000000000..54993a99d1 --- /dev/null +++ b/.changeset/listcommits-outage-503.md @@ -0,0 +1,11 @@ +--- +'@objectstack/metadata-protocol': patch +--- + +`MetadataProtocol.listCommits` 不再把 commit store 读不到答成「这个 package 没有提交历史」 + +`listCommits` 读 `sys_metadata_commit` 的 `catch` 此前对任何失败都返回 `[]`,零日志、不按错误类型区分 —— 它的 JSDoc 甚至把这写成了设计(“Returns [] if the commit store is unavailable”)。于是 ADR-0067 的提交时间线上,「确实没有历史」与「有历史但库读不到」返回值完全一致,而这条时间线正是 `revertCommit` 的选择面:故障期间 UI 显示「无可回滚项」,`rollbackToPackageCommit` 更会在一次都没回滚的情况下返回 `success: true`。 + +现在按错误类型区分,与本文件既有的 `sys_metadata` 覆盖层读法(#5532 / #5707 / #5840)同一处方:表未 provision(首启)仍返回 `[]`;其余失败一律包成 503 `SERVICE_UNAVAILABLE` 上抛,驱动原始错误挂在 `cause` 上。调用方由此能把 outage 与 miss 分开。 + +行为变化:`GET /packages/:id/commits` 在 commit store 故障时返回 503 而不再是 `{ commits: [] }`。 diff --git a/packages/metadata-protocol/src/protocol.metadata-store-outage.test.ts b/packages/metadata-protocol/src/protocol.metadata-store-outage.test.ts index bb89ca52b1..31c2a3520f 100644 --- a/packages/metadata-protocol/src/protocol.metadata-store-outage.test.ts +++ b/packages/metadata-protocol/src/protocol.metadata-store-outage.test.ts @@ -600,3 +600,154 @@ describe('[#5840] the narrowness is the design — three things it deliberately expect([caught.status, caught.code]).toEqual([404, 'RESOURCE_NOT_FOUND']); }); }); + +// --------------------------------------------------------------------------- +// [#5980] The SAME rule on the ADR-0067 commit timeline — `listCommits` +// --------------------------------------------------------------------------- +// A fourth read in this same file, and the one whose invented emptiness points +// at a WRITE. `listCommits` reads `sys_metadata_commit` and its `catch` answered +// `[]` for every failure, with no log and no error-type discrimination — its own +// JSDoc said so ("Returns [] if the commit store is unavailable"), which is how +// a defect gets read as a design decision by everyone who arrives after it. +// +// Why it lives in this file rather than beside the commit tests: it is the same +// DEFECT and the same prescription (`rethrowUnlessMetadataStoreUnprovisioned`) +// as the #5532 / #5707 / #5840 reads above, so `expectStoreUnavailable` holds +// the envelope identical across all four seams and a future edit that re-widens +// one catch and not the others is a diff in one file. +// +// What the emptiness costs, and why this seam is worse than a 404. The timeline +// is `revertCommit`'s SELECTION surface: +// +// GET /packages/:id/commits → `{ commits: [] }` — "nothing to roll back", +// rendered as an empty history in the Studio at +// the exact moment an operator is trying to roll +// something back; +// rollbackToPackageCommit → filters that same `[]`, reverts nothing, and +// returns `success: true` — an operation that +// reports having done the job it never started. +// +// The second is the sharp one and it is asserted below: every other read in this +// file mis-answers a QUESTION, this one mis-reports a WRITE as complete. +// +// Reverse verification, direction predicted BEFORE running: ordinary red, on +// the outage half only. Restoring `} catch { return []; }` turns the 4 outage +// cases below red (they resolve instead of throwing) and leaves the 3 +// benign/healthy cases green — that separation is what shows the change is the +// outage split and not a blanket "listCommits now throws". The baseline entry +// `packages/metadata-protocol/src/protocol.ts::listCommits` is removed in the +// same PR, and its gate is shrink-only, so a lap with the limb restored and the +// entry already removed must ALSO fail `check-durability-degradation-log-level` +// — the ledger and the code are pinned to each other in both directions. + +/** One `sys_metadata_commit` row, in the driver's snake_case wire shape. */ +function commitRow(id: string, createdAt: string, operation = 'apply') { + return { + id, + package_id: 'pkg_crm', + operation, + message: `commit ${id}`, + actor: 'alice', + item_count: 1, + items: JSON.stringify([{ type: 'object', name: 'acct', existedBefore: true, prevVersion: 3 }]), + created_at: createdAt, + }; +} + +describe('[#5980] an unreadable commit store is a 503, not "this package has no history"', () => { + it('the timeline read stops inventing an empty history it never verified', async () => { + const err = connectionRefused(); + const p = new ObjectStackProtocolImplementation(engineThatCannotBeRead(() => err)); + + const caught = await rejection(() => p.listCommits({ packageId: 'pkg_crm' })); + expectStoreUnavailable(caught, err); + }); + + it('a timeout is an outage too — the discrimination is by TYPE, not by phrasing', async () => { + // The guard's conservative direction: an error it does not recognise is + // NOT benign. A driver phrasing nobody enumerated must cost one + // retryable 503, never a silent "no history". + const err = Object.assign(new Error('Query read timeout after 30000ms'), { code: 'ETIMEDOUT' }); + const p = new ObjectStackProtocolImplementation(engineThatCannotBeRead(() => err)); + + const caught = await rejection(() => p.listCommits({ packageId: 'pkg_crm' })); + expectStoreUnavailable(caught, err); + }); + + it('a permission failure is an outage, not an empty package', async () => { + const err = Object.assign(new Error('permission denied for table sys_metadata_commit'), { + code: '42501', + }); + const p = new ObjectStackProtocolImplementation(engineThatCannotBeRead(() => err)); + + const caught = await rejection(() => p.listCommits({ packageId: 'pkg_crm' })); + expectStoreUnavailable(caught, err); + }); + + it('rollbackToPackageCommit stops reporting `success: true` for a rollback it never performed', async () => { + // The consequence that points at a write. The target commit is found + // (that read is a separate `findOne` and it succeeds), then the TIMELINE + // read fails — and the filter over `[]` selected nothing to revert, so + // the method reported a clean success for having done nothing. An + // operator reads that as "the rollback went through". + const err = connectionRefused(); + const engine = engineWithRows([]); + engine.findOne = vi.fn(async () => commitRow('cmt_target', '2026-08-01T00:00:00.000Z')); + engine.find = vi.fn(async () => { throw err; }); + + const p = new ObjectStackProtocolImplementation(engine); + const caught = await rejection( + () => p.rollbackToPackageCommit({ commitId: 'cmt_target' }), + ); + expectStoreUnavailable(caught, err); + // The regression this replaces, verbatim: never a success envelope. + expect(caught.success).toBeUndefined(); + expect(caught.revertedCommits).toBeUndefined(); + }); +}); + +describe('[#5980] the benign case and the healthy timeline are untouched', () => { + it('an unprovisioned sys_metadata_commit still reads as an empty history', async () => { + // First boot, before migrations: there genuinely are no commits, so `[]` + // IS the truth and the packages screen must render rather than 503. + const p = new ObjectStackProtocolImplementation(engineThatCannotBeRead(missingTable)); + + await expect(p.listCommits({ packageId: 'pkg_crm' })).resolves.toEqual([]); + }); + + it('a healthy store with no commits for this package is still `[]`', async () => { + const p = new ObjectStackProtocolImplementation(engineWithRows([])); + + await expect(p.listCommits({ packageId: 'pkg_crm' })).resolves.toEqual([]); + }); + + it('a healthy store still maps the rows and still orders them newest-first', async () => { + // The regression half: the mapping and the sort are the behaviour every + // consumer depends on, and neither is touched by the catch change. Rows + // are handed over oldest-first on purpose — the sort, not the driver, is + // what makes the timeline newest-first. + const p = new ObjectStackProtocolImplementation( + engineWithRows([ + commitRow('cmt_old', '2026-08-01T00:00:00.000Z'), + commitRow('cmt_new', '2026-08-03T00:00:00.000Z'), + commitRow('cmt_mid', '2026-08-02T00:00:00.000Z', 'revert'), + ]), + ); + + const commits = await p.listCommits({ packageId: 'pkg_crm' }); + expect(commits.map((c) => c.id)).toEqual(['cmt_new', 'cmt_mid', 'cmt_old']); + expect(commits[1]!.operation).toBe('revert'); + expect(commits[0]).toMatchObject({ + id: 'cmt_new', + operation: 'apply', + message: 'commit cmt_new', + actor: 'alice', + itemCount: 1, + createdAt: '2026-08-03T00:00:00.000Z', + }); + // `items` arrives as a JSON string from the driver and is parsed here. + expect(commits[0]!.items).toEqual([ + { type: 'object', name: 'acct', existedBefore: true, prevVersion: 3 }, + ]); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index d4387d41c3..a79a79c2d8 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -9851,8 +9851,33 @@ export class ObjectStackProtocolImplementation implements } /** - * List the commit timeline for a package, newest-first (ADR-0067). Returns - * [] if the commit store is unavailable. + * List the commit timeline for a package, newest-first (ADR-0067). + * + * `[]` means ONE thing: this package genuinely has no commits — a first + * boot before `sys_metadata_commit` is provisioned, or a package nobody has + * applied yet. It does NOT mean "the commit store could not be read". + * + * [#5980] It used to mean both. The `catch` answered `[]` for every failure + * and this JSDoc said so outright ("Returns [] if the commit store is + * unavailable"), which is ADR-0110 D3 broken on the ADR-0067 timeline: a + * miss and an outage are different facts with opposite meanings, and the + * timeline is `revertCommit`'s selection surface. An unreachable store + * rendered as "this package has no history", so the Studio offers nothing + * to roll back at the exact moment an operator is trying to roll something + * back — and {@link rollbackToPackageCommit}, which filters this list, + * reported `success: true` for having reverted nothing. Not one line was + * logged anywhere on the path. + * + * Classification is by error TYPE through + * {@link rethrowUnlessMetadataStoreUnprovisioned} — the same guard the + * `sys_metadata` overlay reads in this file already ask (#5532 / #5707) and + * the same `isMissingTableError` predicate `DatabaseLoader` (#5108) and + * `SysMetadataRepository` (#4867) ask, so a driver quirk is taught to the + * platform once rather than re-spelled per seam. + * + * @throws {@link metadataStoreUnavailableError} — a 503 carrying the driver + * error as `cause`, for every failure that is not an unprovisioned + * table. */ async listCommits(request: { packageId: string; @@ -9891,7 +9916,10 @@ export class ObjectStackProtocolImplementation implements // insertion order, then sort by the ISO timestamp. mapped.sort((a, b) => String(b.createdAt ?? '').localeCompare(String(a.createdAt ?? ''))); return mapped; - } catch { + } catch (error) { + // [#5980] Benign (the table has not been provisioned) falls through; + // everything else is a read that did not happen and leaves as a 503. + this.rethrowUnlessMetadataStoreUnprovisioned(error); return []; } } diff --git a/scripts/durability-read-invention.baseline.json b/scripts/durability-read-invention.baseline.json index b5f81ed4e0..81ac2b09ce 100644 --- a/scripts/durability-read-invention.baseline.json +++ b/scripts/durability-read-invention.baseline.json @@ -35,15 +35,6 @@ "checker's message points here only after it has printed the fix it would rather you made." ], "entries": [ - { - "file": "packages/metadata-protocol/src/protocol.ts", - "fn": "listCommits", - "verdict": "unfixed-degradation", - "invents": "return [] (catch has no log and no error-type discrimination)", - "why": "`listCommits` reads `sys_metadata_commit` through `engine.find` and answers `[]` for every failure — its own JSDoc says so ('Returns [] if the commit store is unavailable'). An unreachable commit store is then rendered as 'this package has no history', which is the ADR-0067 timeline reading as empty rather than as unavailable: the exact ADR-0110 D3 miss/outage confusion #5108 fixed one layer down in `DatabaseLoader` and #5532 fixed for `getMetaItems` in this same file.", - "closes": "Ask `isMissingTableError` (already imported in this file) and rethrow everything else, exactly as `rethrowUnlessMetadataStoreUnprovisioned` a few thousand lines up already does for the overlay reads. Tracked separately — a gate PR does not edit the packages it scans.", - "tracked_by": "#5980" - }, { "file": "packages/objectql/src/engine.ts", "fn": "seedAutonumber",