From ca6bdc32ab98e96745bb5cea566f6a681cbce9f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 00:01:56 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): discriminate publishDraft's draft-drain failures instead of swallowing all of them (#4981) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The post-promotion drain `delete` was guarded by a bare `catch {}` whose comment named only the benign concurrent-publisher race while its behaviour amnestied every cause. A connection blip, timeout or privilege error therefore left a stale `state='draft'` row behind with no log and no retry: Studio/Setup kept showing "unpublished changes" for an artifact that had none, and the next publish re-promoted the already-published body. The drain now discriminates: ConflictError (the only error `delete()` raises from its own pre-driver lookup, covering both "row already gone" and "a newer draft was saved") stays silent; every other failure is reported at `error` level with the consequence, the remedy and the original cause. `promoteDraft` still returns success — the drain runs after the `put` committed, so throwing would misreport a durable publish and invite the retry that re-promotes the stale draft. The failure is surfaced machine-readably instead, via a new optional `draftDrainFailed` field on the existing result object. The write is extracted as a named `dropPromotedDraftRow` callee so `check:durability-log-level` can see a seam otherwise spelled `this.delete(...)`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- .../publish-draft-drain-discriminate.md | 44 ++ packages/metadata-protocol/src/index.ts | 1 + ...ys-metadata-repository.draft-drain.test.ts | 458 ++++++++++++++++++ .../src/sys-metadata-repository.ts | 157 +++++- ...check-durability-degradation-log-level.mjs | 4 + 5 files changed, 650 insertions(+), 14 deletions(-) create mode 100644 .changeset/publish-draft-drain-discriminate.md create mode 100644 packages/metadata-protocol/src/sys-metadata-repository.draft-drain.test.ts diff --git a/.changeset/publish-draft-drain-discriminate.md b/.changeset/publish-draft-drain-discriminate.md new file mode 100644 index 0000000000..ff70bb79d2 --- /dev/null +++ b/.changeset/publish-draft-drain-discriminate.md @@ -0,0 +1,44 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): stop `promoteDraft`'s draft drain from swallowing every failure (#4981) + +Publishing a draft is two writes: a transactional `put` that promotes the body onto +the active row, then a `delete` that drains the now-redundant `state='draft'` row. +The drain was guarded by a bare `catch {}` whose comment named exactly one cause — +"a concurrent publisher may have already drained the draft" — while its behaviour +covered **all** of them: connection drops, statement timeouts, missing privileges, +driver faults, `parentVersion` mismatches. + +The result was a silent, self-perpetuating inconsistency. `publishDraft` returned +success, the active row was correct and durable, and a stale `state='draft'` row +stayed in `sys_metadata` holding the body that had just been published. Nothing +logged it and nothing retried it, so Studio/Setup kept reporting "unpublished +changes" for an artifact that had none, and the next publish of that artifact +promoted the same already-published body again — which overwrites the active row if +anything published or reverted in between. + +**The drain now discriminates by cause.** `ConflictError` — the only error +`delete()` raises from its own pre-driver row lookup — stays silent, because both of +its arms are genuinely benign: `actualHead === null` is the concurrent-publisher +race the old comment described, and a differing head means a *newer* draft was saved +while the publish was in flight, so the surviving row is real pending work that must +not be dropped. Every other failure is reported at `error` level (per the +`warn`-vs-`error` rule: the system keeps looking healthy while something it claims to +have cleaned up is still there), naming the orphaned artifact, the consequence, and +the remedy, with the original cause attached. + +**`promoteDraft` still returns success, deliberately.** The drain runs *after* the +`put` has committed, so throwing would misreport a durably successful publish as a +failure and invite the caller to retry — and a retried publish is precisely the +harmful path, because it re-promotes the stale draft. The failure is surfaced +without lying about the publish instead: alongside the log, the result carries a new +optional `draftDrainFailed` field (`{ ref, draftHash, cause }`, exported as +`DraftDrainFailure`) so callers can react without parsing logs. It is an additive +optional field on an existing result object — absent on every clean publish — so no +existing caller changes. + +No protocol or spec shape changed. The drain seam is registered with +`pnpm check:durability-log-level` (as the named callee `dropPromotedDraftRow`) so +the catch cannot quietly go back to swallowing everything. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 3bc534e7aa..4f4c0d1546 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -13,6 +13,7 @@ export type { SysMetadataRepositoryOptions, OverlayState, ExtendedOperation, + DraftDrainFailure, } from './sys-metadata-repository.js'; export { formatStoredMigrationReport, storedMigrationClean } from './stored-migration.js'; diff --git a/packages/metadata-protocol/src/sys-metadata-repository.draft-drain.test.ts b/packages/metadata-protocol/src/sys-metadata-repository.draft-drain.test.ts new file mode 100644 index 0000000000..3c84be631d --- /dev/null +++ b/packages/metadata-protocol/src/sys-metadata-repository.draft-drain.test.ts @@ -0,0 +1,458 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4981 — `SysMetadataRepository.promoteDraft()` no longer treats EVERY draft + * drain failure as "a concurrent publisher already took it". Same family as + * #4728 / #4825 / #4867 (one benign cause amnestying all causes); the + * log-level rule is #4632. + * + * The old code was a bare `catch {}` whose comment named exactly one cause and + * whose behaviour covered all of them — connection drops, timeouts, privilege + * errors, driver faults included. What it leaves behind is not a wrong number + * (#4867) but a wrong ROW: a `state='draft'` row for an artifact that has just + * been published. So every assertion below is about the overlay's contents + * AFTER the drain, and about who is told. + * + * The throw-vs-report question this seam raises is answered "report", by + * maintainer ruling on the issue: the drain runs AFTER the `put` committed, so + * throwing would report a durably successful publish as a failure AND invite a + * retry — and a retried publish is the harmful path, because it promotes the + * stale draft again. Hence: success + `error` log + a machine-readable + * `draftDrainFailed` on the result. + * + * Both directions are pinned deliberately. A suite proving only the loud half + * would pass on a discriminator that never returns "benign" (every concurrent + * publish would start screaming); 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'; + +interface Row { + [k: string]: unknown; +} + +/** NOT benign: the draft row is still there, this delete just did not land. */ +const connectionReset = () => Object.assign(new Error('write ECONNRESET'), { code: 'ECONNRESET' }); + +/** + * Engine fake with REAL transaction semantics — a txn body that throws commits + * nothing, which is what makes "the draft row survives a failed drain" an + * observation rather than an assumption. + * + * Two seams the tests drive: + * - `beforeDraftLookup(n)` — runs before the n-th `findOne` for a draft row, + * so a test can race the repository the way a second publisher does. Lookup + * #1 is `promoteDraft`'s own read; #2 is the drain's. + * - `breakDraftDeletes()` — makes the drain's `engine.delete` fail while + * every other write keeps working, which is exactly what makes the defect + * invisible in production. + */ +function makeFakeEngine() { + const rows = new Map(); + const historyRows: Row[] = []; + let draftLookups = 0; + let beforeDraftLookup: ((n: number) => void) | null = null; + let draftDeleteFailure: (() => unknown) | null = null; + let pendingRollback: Map | 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, + })), + /** The surviving `state='draft'` row for `name`, if any. */ + draftRow: (name: string) => + Array.from(rows.values()).find((r) => r.name === name && r.state === 'draft') ?? null, + /** + * Installing the hook RESETS the lookup counter, so a test counts draft + * reads from its own starting line rather than from the fixture setup: + * inside `promoteDraft`, #1 is the promotion's read and #2 is the drain's. + */ + onBeforeDraftLookup(fn: (n: number) => void) { + draftLookups = 0; + beforeDraftLookup = fn; + }, + breakDraftDeletes(makeError: () => unknown) { + draftDeleteFailure = makeError; + }, + healDraftDeletes() { + draftDeleteFailure = null; + }, + /** + * Apply a change as if ANOTHER connection had committed it while our + * transaction is open — it lands in the live rows *and* in the open + * transaction's rollback snapshot, so our rollback cannot undo somebody + * else's commit. Without this the fake would resurrect the very row a + * concurrent publisher just drained, and the benign race would be + * unobservable. + */ + raceCommit(fn: (rows: Map) => void) { + fn(rows); + if (pendingRollback) fn(pendingRollback); + }, + async find(table: string, opts: { where: Record }) { + if (table === 'sys_metadata_history') { + 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') { + return historyRows.find((h) => matchesHistory(h, opts.where)) ?? null; + } + if (opts.where.state === 'draft') { + draftLookups += 1; + beforeDraftLookup?.(draftLookups); + } + 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 (draftDeleteFailure && found?.row.state === 'draft') throw draftDeleteFailure(); + 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 })); + const outer = pendingRollback; + pendingRollback = rowsSnapshot; + try { + return await cb({ txn: true }); + } catch (err) { + rows.clear(); + for (const [k, r] of rowsSnapshot) rows.set(k, r); + historyRows.length = 0; + historyRows.push(...historySnapshot); + throw err; + } finally { + pendingRollback = outer; + } + }, + }; +} + +const view = (label: string) => ({ + name: 'case_grid', + label, + object: 'case', + columns: [{ field: 'name' }], +}); + +const ref = { org: 'org_alpha', type: 'view' as const, name: 'case_grid' }; + +describe('#4981 — a failed draft drain is discriminated, not blanket-swallowed', () => { + let engine: ReturnType; + let repo: SysMetadataRepository; + let errorSpy: ReturnType; + + beforeEach(() => { + engine = makeFakeEngine(); + repo = new SysMetadataRepository({ + engine, + organizationId: 'org_alpha', + orgLabel: 'org_alpha', + }); + errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + errorSpy.mockRestore(); + }); + + /** Save a draft and return its hash. */ + async function saveDraft(label: string): Promise { + const existing = await repo.get(ref, { state: 'draft' }); + const res = await repo.put(ref, view(label), { + parentVersion: existing?.hash ?? null, + actor: 'studio', + state: 'draft', + }); + return res.version; + } + + describe('the benign races — silent, and the publish reports plain success', () => { + it('a concurrent publisher already drained the draft', async () => { + await saveDraft('A'); + // Race the repo: the row vanishes between promoteDraft's read and the + // drain's own lookup, exactly as a second publisher would make it. + engine.onBeforeDraftLookup((n) => { + if (n === 2) { + engine.raceCommit((rows) => { + for (const [k, r] of rows) if (r.state === 'draft') rows.delete(k); + }); + } + }); + + const result = await repo.promoteDraft(ref, { actor: 'studio' }); + + expect(result.draftDrainFailed).toBeUndefined(); + expect(errorSpy).not.toHaveBeenCalled(); + expect((await repo.get(ref))?.body).toMatchObject({ label: 'A' }); + expect(await repo.get(ref, { state: 'draft' })).toBeNull(); + }); + + it('a NEWER draft was saved while the publish was in flight — and it survives', async () => { + await saveDraft('A'); + let raced = false; + engine.onBeforeDraftLookup((n) => { + if (n === 2 && !raced) { + raced = true; + // Somebody hit Save again while Publish was running. The drain's + // parentVersion no longer matches, and NOT deleting is the correct + // outcome — this row is genuine pending work, not a stale ghost. + engine.raceCommit((rows) => { + for (const [k, r] of rows) { + if (r.state === 'draft') { + rows.set(k, { ...r, metadata: JSON.stringify(view('B')), checksum: 'newer' }); + } + } + }); + } + }); + + const result = await repo.promoteDraft(ref, { actor: 'studio' }); + + expect(result.draftDrainFailed).toBeUndefined(); + expect(errorSpy).not.toHaveBeenCalled(); + // 'A' went live; the newer draft 'B' is still pending, untouched. + expect((await repo.get(ref))?.body).toMatchObject({ label: 'A' }); + expect((await repo.get(ref, { state: 'draft' }))?.body).toMatchObject({ label: 'B' }); + }); + }); + + describe('a REAL drain failure — success, but loudly and machine-readably', () => { + async function publishWithBrokenDrain(): Promise< + Awaited> + > { + await saveDraft('A'); + engine.breakDraftDeletes(connectionReset); + return repo.promoteDraft(ref, { actor: 'studio' }); + } + + it('still returns success — the publish itself committed and must not be re-run', async () => { + const result = await publishWithBrokenDrain(); + + expect(result.version).toBeTruthy(); + expect(result.item.body).toMatchObject({ label: 'A' }); + expect((await repo.get(ref))?.body).toMatchObject({ label: 'A' }); + // The `publish` history event is durable — the failure is post-commit. + 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: 'publish' }, + ]); + }); + + it('leaves the stale draft row behind — the consequence, observed', async () => { + await publishWithBrokenDrain(); + + // This is the row the old `catch {}` never mentioned: an artifact with + // no pending changes that every "unpublished changes" read still sees. + const stale = await repo.get(ref, { state: 'draft' }); + expect(stale).not.toBeNull(); + expect(stale?.body).toMatchObject({ label: 'A' }); + expect(engine.draftRow('case_grid')).not.toBeNull(); + }); + + it('reports at error level, naming the consequence and the remedy', async () => { + await publishWithBrokenDrain(); + + expect(errorSpy).toHaveBeenCalledTimes(1); + const [message, cause] = errorSpy.mock.calls[0] as [string, unknown]; + expect(message).toContain('view/case_grid'); + // consequence — the row that stayed, and that nothing repairs it + expect(message).toMatch(/state='draft'/); + expect(message).toMatch(/STILL in/); + expect(message).toMatch(/nothing retries or repairs it/i); + expect(message).toMatch(/unpublished changes/i); + expect(message).toMatch(/NEXT publish/i); + // why it is not thrown — a successful publish is not reported as failed + expect(message).toMatch(/COMMITTED/); + expect(message).toMatch(/reported, not thrown/i); + // remedy + expect(message).toMatch(/Remedy:/); + expect(message).toMatch(/re-publish/i); + expect(message).toMatch(/sys_metadata/); + // and the driver error is carried, not swallowed + expect((cause as Error).message).toBe('write ECONNRESET'); + }); + + it('surfaces the failure on the RESULT too, so a caller need not parse logs', async () => { + const result = await publishWithBrokenDrain(); + + expect(result.draftDrainFailed).toBeDefined(); + expect(result.draftDrainFailed?.ref).toEqual({ + org: 'org_alpha', + type: 'view', + name: 'case_grid', + }); + expect(result.draftDrainFailed?.draftHash).toBe(result.version); + expect((result.draftDrainFailed?.cause as Error).message).toBe('write ECONNRESET'); + }); + + it('speaks once per orphaned artifact — each names a different stale row', async () => { + await publishWithBrokenDrain(); + const ref2 = { org: 'org_alpha', type: 'view' as const, name: 'lead_grid' }; + await repo.put( + ref2, + { name: 'lead_grid', label: 'L', object: 'lead', columns: [{ field: 'name' }] }, + { parentVersion: null, actor: 'studio', state: 'draft' }, + ); + await repo.promoteDraft(ref2, { actor: 'studio' }); + + // Deliberately NOT #4867's once-per-outage suppression: the remedy is + // per-row, so collapsing these would hide which drafts are stale. + expect(errorSpy).toHaveBeenCalledTimes(2); + expect((errorSpy.mock.calls[1] as [string])[0]).toContain('view/lead_grid'); + }); + }); + + it('DISTINGUISHES the two: same call site, opposite verdicts', async () => { + // Benign. + await saveDraft('A'); + engine.onBeforeDraftLookup((n) => { + if (n === 2) { + engine.raceCommit((rows) => { + for (const [k, r] of rows) if (r.state === 'draft') rows.delete(k); + }); + } + }); + const benign = await repo.promoteDraft(ref, { actor: 'studio' }); + + // Real. + const engine2 = makeFakeEngine(); + const repo2 = new SysMetadataRepository({ + engine: engine2, + organizationId: 'org_alpha', + orgLabel: 'org_alpha', + }); + await repo2.put(ref, view('A'), { parentVersion: null, actor: 'studio', state: 'draft' }); + engine2.breakDraftDeletes(connectionReset); + const real = await repo2.promoteDraft(ref, { actor: 'studio' }); + + expect(benign.draftDrainFailed).toBeUndefined(); + expect(real.draftDrainFailed).toBeDefined(); + expect(errorSpy).toHaveBeenCalledTimes(1); + // Both published; only one left a row behind. + expect(engine.draftRow('case_grid')).toBeNull(); + expect(engine2.draftRow('case_grid')).not.toBeNull(); + }); + + /** + * The cheap half of "can the NEXT publish detect a stale draft?" — it + * already can, and this pins that it does. `put()`'s identical-hash + * short-circuit means re-publishing a stale draft writes NO second history + * event, and the drain then succeeds and removes the row. The expensive half + * (a stale draft whose body no longer matches the active row, i.e. after + * something else published or reverted in between) is indistinguishable from + * genuine pending work by content alone and is NOT guessed at here. + */ + it('self-heals on the next publish while the active row is unchanged', async () => { + await saveDraft('A'); + engine.breakDraftDeletes(connectionReset); + await repo.promoteDraft(ref, { actor: 'studio' }); + expect(engine.draftRow('case_grid')).not.toBeNull(); + const afterFirst = engine.committed(); + + // The driver recovers; the operator (or the UI's still-lit Publish button) + // publishes again. + engine.healDraftDeletes(); + const second = await repo.promoteDraft(ref, { actor: 'studio' }); + + expect(second.draftDrainFailed).toBeUndefined(); + expect(engine.draftRow('case_grid')).toBeNull(); + // No duplicate `publish` event: the body was already live, so put() no-ops. + expect(engine.committed()).toEqual(afterFirst); + expect((await repo.get(ref))?.body).toMatchObject({ label: 'A' }); + }); +}); + +/** + * Gate registration (#4981). The behavioural tests above go red if the catch + * goes silent, but only for the shapes they simulate; the AST gate is what + * stops the seam regressing at all. `dropPromotedDraftRow` exists as a named + * method precisely so the scanner can see a write that is otherwise spelled + * `this.delete(...)` — a callee name far too common to put in the vocabulary. + * Losing either half silently un-guards the seam, so both are pinned. + */ +describe('#4981 — the drain seam is registered with check:durability-log-level', () => { + it('names `dropPromotedDraftRow` in the checker vocabulary', () => { + const checker = readFileSync( + new URL('../../../scripts/check-durability-degradation-log-level.mjs', import.meta.url), + 'utf8', + ); + expect(checker).toContain('dropPromotedDraftRow'); + expect(checker).toMatch(/DURABILITY_CRITICAL_CALLEES[\s\S]*dropPromotedDraftRow/); + }); + + it('keeps the drain behind that named callee, with a discriminating catch', () => { + const source = readFileSync( + new URL('./sys-metadata-repository.ts', import.meta.url), + 'utf8', + ); + // The guarded call the scanner looks for. + expect(source).toMatch(/try\s*\{\s*await this\.dropPromotedDraftRow\(/); + // The catch delegates to the verdict helper rather than swallowing. + expect(source).toMatch(/catch \(error\) \{\s*draftDrainFailed = this\.draftDrainVerdict\(/); + // And the verdict amnesties exactly one class of cause. + expect(source).toMatch(/if \(error instanceof ConflictError\) return undefined;/); + }); +}); diff --git a/packages/metadata-protocol/src/sys-metadata-repository.ts b/packages/metadata-protocol/src/sys-metadata-repository.ts index ee5ef27f95..af03340364 100644 --- a/packages/metadata-protocol/src/sys-metadata-repository.ts +++ b/packages/metadata-protocol/src/sys-metadata-repository.ts @@ -100,6 +100,34 @@ export type OverlayState = 'active' | 'draft'; */ export type ExtendedOperation = 'create' | 'update' | 'publish' | 'revert' | 'delete'; +/** + * #4981 — the machine-readable half of "the publish landed, its cleanup did + * not". Set on {@link SysMetadataRepository.promoteDraft}'s result ONLY when + * the post-promotion drain failed for a reason that leaves a **stale** draft + * row behind; absent means the overlay is in its intended state. + * + * Absent covers both good outcomes, which is why absence is safe to read as + * "clean": + * - the draft row was dropped, or + * - the drain lost a race it is *supposed* to lose (a concurrent publisher + * already drained it, or a newer draft was saved while this publish was in + * flight — see {@link SysMetadataRepository.draftDrainVerdict}). + * + * Present means: the active row is correct and durable, and a `state='draft'` + * row for the same `(org, type, name)` is still in `sys_metadata` holding the + * body that was just published. Nothing retries it. Callers that surface + * "has unpublished changes" (Studio / Setup) are about to be wrong, and the + * next publish of this artifact promotes that same body again. + */ +export interface DraftDrainFailure { + /** The artifact whose draft row could not be dropped. */ + ref: MetaRef; + /** Checksum of the draft body the drain targeted — the row's `checksum`. */ + draftHash: string; + /** The original failure, unchanged, so callers can classify it themselves. */ + cause: unknown; +} + /** * Sub-set of the ObjectQL engine shape we depend on. Kept narrow so * tests can stub it with a plain mock. Mirrors the real engine's @@ -618,11 +646,23 @@ export class SysMetadataRepository implements MetadataRepository { * also surfaces optimistic-lock conflicts when something else has * published in between (e.g. another admin reverted to an older * version since the draft was authored). + * + * #4981 — the promotion is a `put` (durable, transactional) followed by a + * drain `delete` of the draft row. The drain runs AFTER the put committed, + * so its failure is reported, never thrown: see {@link draftDrainVerdict} + * for why, and {@link DraftDrainFailure} for the signal it returns. */ async promoteDraft( ref: MetaRef, opts: { actor: string | null; source?: string; message?: string; intent?: MetadataWriteIntent }, - ): Promise<{ version: string; seq: number; item: MetadataItem; packageId: string | null }> { + ): Promise<{ + version: string; + seq: number; + item: MetadataItem; + packageId: string | null; + /** #4981 — set only when the draft row survived the promotion. */ + draftDrainFailed?: DraftDrainFailure; + }> { this.assertOpen(); // Read the RAW draft row (not just the body) so the promotion can carry // the draft's package binding onto the active row. ADR-0048 keys overlay @@ -660,24 +700,21 @@ export class SysMetadataRepository implements MetadataRepository { opType: 'publish', packageId: draftPackageId, }); - // Drop the draft row — it has been promoted. Tolerate races where - // a second publisher already drained it. + // Drop the draft row — it has been promoted. + let draftDrainFailed: DraftDrainFailure | undefined; try { - await this.delete(ref, { - parentVersion: draft.hash, - actor: opts.actor, - source: opts.source ?? 'sys-metadata-repo.publish', - intent: opts.intent ?? 'override-artifact', - state: 'draft', - }); - } catch { - // best-effort: a concurrent publisher may have already drained - // the draft; the active row's authoritative content is intact. + await this.dropPromotedDraftRow(ref, draft.hash, opts); + } catch (error) { + draftDrainFailed = this.draftDrainVerdict(error, ref, draft.hash); } // Surface the promoted draft's package binding so publish-time // materializers (ADR-0086 P2 — package-door permission sets) can stamp // the data-plane row with the owning `package_id`. - return { ...result, packageId: draftPackageId }; + return { + ...result, + packageId: draftPackageId, + ...(draftDrainFailed ? { draftDrainFailed } : {}), + }; } /** @@ -1207,6 +1244,98 @@ export class SysMetadataRepository implements MetadataRepository { ); } + /** + * The post-promotion draft drain (#4981) — the `sys_metadata` write whose + * failure leaves a promoted draft row behind. + * + * Extracted as a *named* callee for one reason beyond readability: the write + * itself is `this.delete(...)`, and `delete` is far too common a method name + * to put in `DURABILITY_CRITICAL_CALLEES`. Naming the seam is what lets + * `scripts/check-durability-degradation-log-level.mjs` see it and keep this + * catch from ever going quiet again — the same move #5001 made when the + * guarded write was hidden inside a closure the AST scan could not enter. + */ + private async dropPromotedDraftRow( + ref: MetaRef, + draftHash: string, + opts: { actor: string | null; source?: string; intent?: MetadataWriteIntent }, + ): Promise { + await this.delete(ref, { + parentVersion: draftHash, + actor: opts.actor, + source: opts.source ?? 'sys-metadata-repo.publish', + intent: opts.intent ?? 'override-artifact', + state: 'draft', + }); + } + + /** + * The verdict for a failed draft drain (#4981) — same shape as the + * #4728 / #4825 / #4867 family: **one benign cause may not amnesty every + * cause**. Before this, a bare `catch {}` named the concurrent-publisher + * race in its comment and swallowed connection drops, timeouts, privilege + * errors and driver faults with it. + * + * **Benign — silent, and only these.** Both arms are a `ConflictError` from + * {@link delete}, which does its own row lookup before touching the driver, + * so "the row is gone" is not a driver-dependent signal but a ConflictError + * carrying `actualHead === null`: + * + * - `actualHead === null` — a concurrent publisher already drained the + * draft. Exactly the race the old comment described: no row is left. + * - `actualHead !== draftHash` — a *newer* draft was saved while this + * publish was in flight. The row that survives is not stale, it is + * genuine pending work, and dropping it would have destroyed an admin's + * edit. "Has unpublished changes" is then *correct*, so reporting a + * consequence here would be a false alarm — and AGENTS.md is explicit + * that escalating a non-degradation to `error` is the mirror-image + * failure of hiding one. + * + * **Everything else — reported at `error`, and never thrown.** The drain + * runs after the `put` committed. Throwing would (a) report a durably + * successful publish as a failure and (b) invite the caller to retry — and a + * retried publish is precisely the harmful path, because it promotes the + * stale draft a second time. So the failure is surfaced two ways instead: + * loudly in the log, and machine-readably as {@link DraftDrainFailure} on + * the result. This is a durability/consistency degradation by the AGENTS.md + * test — the system keeps looking healthy while something it claims to have + * cleaned up is still there — so it is `error`, not `warn` (#4632). + * + * Unlike #4867's once-per-outage reporting, this speaks on **every** + * occurrence: each one names a different orphaned artifact, and the remedy + * is per-row. Deduplicating would hide which drafts are stale, which is the + * one fact the reader needs. + * + * @returns `undefined` for the benign races; a {@link DraftDrainFailure} — + * after reporting it — for every other failure. + */ + private draftDrainVerdict( + error: unknown, + ref: MetaRef, + draftHash: string, + ): DraftDrainFailure | undefined { + if (error instanceof ConflictError) return undefined; + + const full = this.fullRef(ref); + console.error( + `[SysMetadataRepository] Published ${full.type}/${full.name} but could NOT drop its ` + + `promoted draft row. The publish itself COMMITTED — the active row holds the published ` + + `body and the history event was recorded — so this is reported, not thrown, and ` + + `promoteDraft() still returns success. Consequence: the \`state='draft'\` row for ` + + `${full.type}/${full.name} (org ${full.org}, checksum ${draftHash}) is STILL in ` + + `\`sys_metadata\`, and nothing retries or repairs it — Studio/Setup will keep showing ` + + `this artifact as having unpublished changes when it has none, and the NEXT publish of ` + + `it promotes that same already-published body again (harmless while the active row is ` + + `unchanged, but it overwrites the active row if anything has published or reverted it ` + + `since). Remedy: fix the datasource/driver error below (connection, timeout, ` + + `privileges), then re-publish ${full.type}/${full.name} — the drain runs again and ` + + `succeeds — or delete the row directly from \`sys_metadata\` ` + + `(type=${full.type}, name=${full.name}, state='draft').`, + error, + ); + return { ref: full, draftHash, cause: error }; + } + /** Lightweight UUID-ish id for history rows; sufficient for an audit log. */ private uuid(): string { if (typeof globalThis.crypto?.randomUUID === 'function') { diff --git a/scripts/check-durability-degradation-log-level.mjs b/scripts/check-durability-degradation-log-level.mjs index 5269107f60..4e653e17ca 100644 --- a/scripts/check-durability-degradation-log-level.mjs +++ b/scripts/check-durability-degradation-log-level.mjs @@ -128,6 +128,10 @@ const DURABILITY_CRITICAL_CALLEES = new Map([ 'writeRecord', 'A seed record was not written — the row is simply absent (or, on the upsert/update path, still holds its pre-seed contents) while the load moves on to the next record (#4729).', ], + [ + 'dropPromotedDraftRow', + "A published draft was never drained — the active row is correct, but the `state='draft'` row is still in `sys_metadata`, so Studio/Setup keeps showing unpublished changes that do not exist and the next publish promotes the same stale body again (#4981).", + ], ]); /** Log levels that are ACCEPTABLE inside a durability-guarding catch. */ From 92e06232420e12e26163cb05bd935d1138ea3457 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 00:46:08 +0000 Subject: [PATCH 2/2] test(metadata-protocol): record the #4981 draft-drain fake in the engine-double-contract ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check-engine-double-contract` flagged the new draft-drain suite's fake engine: its `delete()` does not route through `assertEngineDeleteDispatch`, so the double is structurally looser than the real `ObjectQL.delete` — the #4434 shape. The gate's preferred remedy (add @objectstack/objectql to devDependencies) is not available to this package: @objectstack/objectql depends on @objectstack/metadata-protocol in `dependencies`, so the edge is CYCLIC and turbo refuses the graph. That was measured in #4867 by adding the edge and reverting it; the dependency direction is re-verified statically here rather than re-run. So this takes the gate's other sanctioned route — a MEASURED DEBT entry, modeled on the sibling entry #4980 added for sys-metadata-repository.history-counters.ts, with the same `closes` route: sink assertEngineDeleteDispatch into a package both sides already depend on (@objectstack/metadata-core), tracked as #4987. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- scripts/engine-double-contract.baseline.json | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/engine-double-contract.baseline.json b/scripts/engine-double-contract.baseline.json index c884e01341..4db872aa83 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.draft-drain.test.ts", + "unguarded": 1, + "kind": "DEBT", + "why": "MEASURED (#4981): identical to the sibling `sys-metadata-repository.history-counters.test.ts` entry below, and closed by the same route. The devDependency this ledger's other metadata-protocol entries prescribe is CYCLIC, not merely unreviewed: @objectstack/objectql depends on @objectstack/metadata-protocol in `dependencies` (re-verified statically on this branch), so adding objectql to metadata-protocol's devDependencies makes turbo refuse the graph outright — `Cyclic dependency detected: @objectstack/metadata-protocol#build, @objectstack/objectql#build`, measured in #4867 by adding the edge and reverting it, and deliberately NOT re-run here. The fake's delete is exercised only by the #4981 drain 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) — tracked as #4987 — then open the fake's delete with it; the devDependency route is closed by the cycle above, for this file and for the five sibling metadata-protocol entries alike" + }, { "file": "packages/metadata-protocol/src/sys-metadata-repository.history-counters.test.ts", "unguarded": 1,