From f4b94c5ba13e6206b0ec09335f12c9ba717616fe Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:13:56 +0000 Subject: [PATCH 1/3] feat(metadata-protocol/objectql): turn-atomic package publish (ADR-0067 D2, #3066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1: every draft promotion + the sys_metadata_commit record run inside ONE engine.transaction() — a mid-batch failure rolls back everything (publishedCount 0; causal item carries its real error, the rest report batch_aborted). publishMetaItem is split into promoteDraftForPublish (guards + promote, metadata-only) and runPublishSideEffects so the batch can transact promotions while deferring side effects. Phase 2 (post-commit): registry refresh, DDL, seed batch apply, materializers, ADR-0094 projections, events — best-effort, surfaced via materializeApplied.failures instead of faking an unpublish (metadata is live; boot reconciliation heals side-effect drift). objectql: engine.transaction() now JOINS an already-open ambient transaction instead of opening a nested driver transaction — a nested begin deadlocks single-connection pools (SQLite knex) and escapes the outer rollback. Locked by two new engine-ambient-transaction tests. BREAKING (behavioral): consumers relying on partial batch publishes now get all-or-nothing. Engines without transaction() keep the previous sequential behavior (same fallback stance as the repo's withTxn). Tests: publish contract suite rewritten to the two-phase seams (+4 new cases incl. rollback/commit tracking and side-effect-failure surfacing); commit-history suite updated; 29 green across the three suites. --- .changeset/turn-atomic-publish.md | 6 + packages/metadata-protocol/src/protocol.ts | 415 ++++++++++++------ .../src/engine-ambient-transaction.test.ts | 44 +- packages/objectql/src/engine.ts | 12 + .../src/protocol-commit-history.test.ts | 9 +- .../protocol-publish-package-drafts.test.ts | 177 ++++++-- 6 files changed, 497 insertions(+), 166 deletions(-) create mode 100644 .changeset/turn-atomic-publish.md diff --git a/.changeset/turn-atomic-publish.md b/.changeset/turn-atomic-publish.md new file mode 100644 index 0000000000..c75bad4a1a --- /dev/null +++ b/.changeset/turn-atomic-publish.md @@ -0,0 +1,6 @@ +--- +'@objectstack/metadata-protocol': minor +'@objectstack/objectql': minor +--- + +Package-draft publishing is now turn-atomic (ADR-0067 Decision-2, #3066). `publishPackageDrafts` runs every draft promotion AND the `sys_metadata_commit` record inside ONE engine transaction — a mid-batch failure rolls back the whole batch (`publishedCount: 0`; the causal item carries its real error, the rest report `batch_aborted`). Side effects (registry refresh, table DDL, seed apply, materializers, ADR-0094 projections, events) run after the metadata commits and are surfaced-not-swallowed on failure. `@objectstack/objectql`'s `engine.transaction()` now JOINS an already-open ambient transaction instead of opening a nested driver transaction (deadlock on single-connection pools; escaped the outer rollback). BREAKING (behavioral): API consumers that relied on partial batch publishes ("2 of 3 landed") now get all-or-nothing; engines without `transaction()` (memory driver, minimal stubs) keep the previous sequential behavior. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index ff8def2fb0..0b245159bf 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -5,7 +5,7 @@ import { IDataEngine } from '@objectstack/core'; import { readEnvWithDeprecation } from '@objectstack/types'; import type { MetadataHostEngine } from './host-engine.js'; import { SysMetadataRepository, type SysMetadataEngine } from './sys-metadata-repository.js'; -import { ConflictError, assertProtocolCompat } from '@objectstack/metadata-core'; +import { ConflictError, assertProtocolCompat, type MetadataItem } from '@objectstack/metadata-core'; import type { BatchUpdateRequest, BatchUpdateResponse, @@ -4467,6 +4467,55 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { * same contract as `seedApplied` — surfaced, never thrown. */ materializeApplied?: PublishMaterializeResult; + }> { + const { singularType, orgId, result } = await this.promoteDraftForPublish(request); + const response: { + success: boolean; + version: string; + seq: number; + message?: string; + seedApplied?: { success: boolean; inserted: number; updated: number; error?: string; errors?: unknown[] }; + materializeApplied?: PublishMaterializeResult; + projectionApplied?: MutationProjectionOutcome; + } = { + success: true, + version: result.version, + seq: result.seq, + message: `Published draft — type=${request.type}, name=${request.name} [seq=${result.seq}]`, + }; + const effects = await this.runPublishSideEffects({ + singularType, + requestType: request.type, + name: request.name, + orgId, + body: result.item.body, + packageId: result.packageId, + ...(request.actor ? { actor: request.actor } : {}), + skipSeedApply: !!request._skipSeedApply, + }); + if (effects.seedApplied) response.seedApplied = effects.seedApplied; + if (effects.materializeApplied) response.materializeApplied = effects.materializeApplied; + if (effects.projectionApplied) response.projectionApplied = effects.projectionApplied; + return response; + } + + /** + * Phase 1 of a publish (ADR-0067 D2) — guards + draft promotion, + * METADATA WRITES ONLY: the draftable gate, the ADR-0010 lock check, and + * `repo.promoteDraft` (active-row put + draft delete), with + * optimistic-lock conflicts translated to `metadata_conflict`. Contains + * NO side effects, so a batch caller (`publishPackageDrafts`) can run + * many promotions inside ONE `engine.transaction()` and roll them ALL + * back together — the "a commit cannot half-land" invariant. + * `publishMetaItem` composes it with {@link runPublishSideEffects} for + * the single-item path. + */ + private async promoteDraftForPublish(request: { + type: string; name: string; organizationId?: string; actor?: string; message?: string; + }): Promise<{ + singularType: string; + orgId: string | null; + result: { version: string; seq: number; item: MetadataItem; packageId: string | null }; }> { const singularType = PLURAL_TO_SINGULAR[request.type] ?? request.type; if (!ObjectStackProtocolImplementation.isOverlayAllowed(singularType) @@ -4506,80 +4555,7 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { ...(request.message ? { message: request.message } : {}), intent, }); - // Drafts skipped the registry mutation; on publish we now - // refresh the runtime object registry so live behaviour - // catches up immediately (matches saveMetaItem's - // post-persistence registry update path). - this.applyObjectRegistryMutation({ - type: request.type, - name: request.name, - item: result.item.body, - }); - // Create the object's table now so it's CRUD-able without a restart. - await this.ensureObjectStorage(request.type, request.name); - const response: { - success: boolean; - version: string; - seq: number; - message?: string; - seedApplied?: { success: boolean; inserted: number; updated: number; error?: string; errors?: unknown[] }; - materializeApplied?: PublishMaterializeResult; - projectionApplied?: MutationProjectionOutcome; - } = { - success: true, - version: result.version, - seq: result.seq, - message: `Published draft — type=${request.type}, name=${request.name} [seq=${result.seq}]`, - }; - // Publishing a `seed` is what makes its rows live — materialize them - // NOW (best-effort, never fails the publish) so every publish path - // (per-ref REST publish, the home banner, package publish-drafts) - // lands data, not just metadata. The body is already in hand from - // the promote — no read-back, so no org-scope resolution pitfalls. - if (singularType === 'seed' && !request._skipSeedApply) { - response.seedApplied = await this.applySeedBodies([result.item.body], orgId); - } - // Publish-time materializer (ADR-0086 P2): project the published body - // into its data-plane row (e.g. `permission` → `sys_permission_set` - // with `managed_by:'package'`). Unlike seeds this needs no batch - // ordering — permission sets carry no cross-item references — so it - // runs on every publish path, package-draft batch included. The - // owning `package_id` rides on `result.packageId` (the draft's - // binding), so a package-door set materializes under the right owner. - const materializer = this.publishMaterializers.get(singularType); - if (materializer) { - try { - response.materializeApplied = await materializer({ - body: result.item.body, - packageId: result.packageId, - organizationId: orgId, - actor: request.actor ?? 'system', - }); - } catch (e: any) { - response.materializeApplied = { - success: false, inserted: 0, updated: 0, - error: e?.message ?? 'materialize failed', - }; - } - } - // [ADR-0094] Awaited projection: runs AFTER the package-door - // materializer (which stamps package provenance) so the projector - // sees final record state; refuses/no-ops per its own rules. - const publishProjection = await this.runMutationProjector({ - type: singularType, - name: request.name, - state: 'active', - organizationId: orgId, - body: result.item.body, - }); - if (publishProjection) response.projectionApplied = publishProjection; - this.emitMetadataMutation({ - type: singularType, - name: request.name, - state: 'active', - organizationId: orgId, - }); - return response; + return { singularType, orgId, result }; } catch (err: any) { if (err instanceof ConflictError) { const conflict: any = new Error( @@ -4596,6 +4572,97 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { } } + /** + * Phase 2 of a publish (ADR-0067 D2) — the post-promotion side effects: + * runtime object-registry refresh, table DDL, single-item seed apply, + * the ADR-0086 P2 publish materializer, the ADR-0094 awaited projector, + * and the mutation event. Everything here is (a) non-transactional by + * nature (DDL cannot run inside the driver transaction; the registry is + * in-memory; projections are best-effort) and (b) healed by boot + * reconciliation when it fails — which is why a batch publish runs it + * AFTER the metadata transaction commits: side effects can self-heal, a + * half-landed metadata batch cannot (the ADR-0094 lesson). + */ + private async runPublishSideEffects(args: { + singularType: string; + requestType: string; + name: string; + orgId: string | null; + body: unknown; + packageId: string | null; + actor?: string; + skipSeedApply?: boolean; + }): Promise<{ + seedApplied?: { success: boolean; inserted: number; updated: number; error?: string; errors?: unknown[] }; + materializeApplied?: PublishMaterializeResult; + projectionApplied?: MutationProjectionOutcome; + }> { + const out: { + seedApplied?: { success: boolean; inserted: number; updated: number; error?: string; errors?: unknown[] }; + materializeApplied?: PublishMaterializeResult; + projectionApplied?: MutationProjectionOutcome; + } = {}; + // Drafts skipped the registry mutation; on publish we now refresh the + // runtime object registry so live behaviour catches up immediately + // (matches saveMetaItem's post-persistence registry update path). + this.applyObjectRegistryMutation({ + type: args.requestType, + name: args.name, + item: args.body, + }); + // Create the object's table now so it's CRUD-able without a restart. + await this.ensureObjectStorage(args.requestType, args.name); + // Publishing a `seed` is what makes its rows live — materialize them + // NOW (best-effort, never fails the publish) so every publish path + // (per-ref REST publish, the home banner, package publish-drafts) + // lands data, not just metadata. The body is already in hand from + // the promote — no read-back, so no org-scope resolution pitfalls. + if (args.singularType === 'seed' && !args.skipSeedApply) { + out.seedApplied = await this.applySeedBodies([args.body], args.orgId); + } + // Publish-time materializer (ADR-0086 P2): project the published body + // into its data-plane row (e.g. `permission` → `sys_permission_set` + // with `managed_by:'package'`). Unlike seeds this needs no batch + // ordering — permission sets carry no cross-item references — so it + // runs on every publish path, package-draft batch included. The + // owning `package_id` rides on the draft's binding, so a package-door + // set materializes under the right owner. + const materializer = this.publishMaterializers.get(args.singularType); + if (materializer) { + try { + out.materializeApplied = await materializer({ + body: args.body, + packageId: args.packageId, + organizationId: args.orgId, + actor: args.actor ?? 'system', + }); + } catch (e: any) { + out.materializeApplied = { + success: false, inserted: 0, updated: 0, + error: e?.message ?? 'materialize failed', + }; + } + } + // [ADR-0094] Awaited projection: runs AFTER the package-door + // materializer (which stamps package provenance) so the projector + // sees final record state; refuses/no-ops per its own rules. + const publishProjection = await this.runMutationProjector({ + type: args.singularType, + name: args.name, + state: 'active', + organizationId: args.orgId, + body: args.body, + }); + if (publishProjection) out.projectionApplied = publishProjection; + this.emitMetadataMutation({ + type: args.singularType, + name: args.name, + state: 'active', + organizationId: args.orgId, + }); + return out; + } + /** * Materialize published `seed` bodies into data rows via the SeedLoaderService * (externalId-keyed upsert, multi-pass for cross-seed references). Passing ALL @@ -4812,47 +4879,160 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { // (owned by the env door / another package), not just a clean count. const materialize = { any: false, inserted: 0, updated: 0, failures: [] as Array<{ type: string; name: string; error: string }> }; - for (const d of ordered) { - try { - if (d.type === 'seed') { - // Capture the body BEFORE promote (the draft row is deleted by - // the promote, and a post-publish read-back has org-scope - // resolution pitfalls — reading the draft is unambiguous). - const ref = { type: d.type, name: d.name, org: orgId ?? 'env' } as unknown as Parameters[0]; - const draft = await repo.get(ref, { state: 'draft' }); - if (draft?.body) seedBodies.push(draft.body); + // ═══ Phase 1 — ATOMIC metadata writes (ADR-0067 D2) ═══ + // Every draft promotion AND the sys_metadata_commit record run inside + // ONE engine transaction: any failure rolls ALL of them back — "a + // commit cannot half-land". Nested repository writes JOIN this + // transaction via the engine's ambient-tx join (a nested begin would + // deadlock single-connection pools), and side effects are deliberately + // deferred to Phase 2: DDL cannot run inside the driver transaction, + // in-memory registry mutations cannot roll back, and projections / + // probes are healed by boot reconciliation — side effects can + // self-heal, a half-landed metadata batch cannot (the ADR-0094 + // lesson). Engines without `transaction()` (memory driver, minimal + // stubs) fall through to a plain sequential run with the same weaker + // guarantee the repository's `withTxn` documents. + type PromotedDraft = { + d: { type: string; name: string }; + singularType: string; + body: unknown; + packageId: string | null; + version: string; + seq: number; + }; + const promoted: PromotedDraft[] = []; + // (assigned inside the transaction closure — keep the wide type) + let commit = null as { commitId: string } | null; + const inTxn: (cb: () => Promise) => Promise = + typeof (this.engine as { transaction?: unknown })?.transaction === 'function' + ? (cb) => (this.engine as unknown as { transaction: (fn: () => Promise) => Promise }).transaction(() => cb()) + : (cb) => cb(); + try { + await inTxn(async () => { + for (const d of ordered) { + try { + if (d.type === 'seed') { + // Capture the body BEFORE promote (the draft row is + // deleted by the promote, and a post-publish read-back + // has org-scope resolution pitfalls — reading the + // draft is unambiguous). + const ref = { type: d.type, name: d.name, org: orgId ?? 'env' } as unknown as Parameters[0]; + const draft = await repo.get(ref, { state: 'draft' }); + if (draft?.body) seedBodies.push(draft.body); + } + const { singularType, result } = await this.promoteDraftForPublish({ + type: d.type, + name: d.name, + ...(request.organizationId ? { organizationId: request.organizationId } : {}), + ...(request.actor ? { actor: request.actor } : {}), + message: `publish app package '${request.packageId}'`, + }); + promoted.push({ + d, singularType, + body: result.item.body, + packageId: result.packageId, + version: result.version, + seq: result.seq, + }); + if (typeof result.seq === 'number') publishedSeqs.push(result.seq); + } catch (e: unknown) { + // Tag the causal item and abort — the surrounding + // transaction rolls back every promotion made so far. + const err = e instanceof Error ? e : new Error(String(e)); + (err as { __batchItem?: unknown }).__batchItem = d; + throw err; + } } - const r = await this.publishMetaItem({ - type: d.type, - name: d.name, - ...(request.organizationId ? { organizationId: request.organizationId } : {}), + // ADR-0067 — record this turn as ONE commit, INSIDE the same + // transaction as the promotions it describes: the commit row and + // the published state land or roll back together, so a recorded + // commit can never describe a partial publish. + if (promoted.length > 0) { + const promotedKeys = new Set(promoted.map((p) => `${p.d.type}/${p.d.name}`)); + commit = await this.recordPackageCommit({ + orgId, + packageId: request.packageId, + operation: 'apply', + ...(request.message ? { message: request.message } : {}), + ...(request.actor ? { actor: request.actor } : {}), + ...(request.aiModel ? { aiModel: request.aiModel } : {}), + items: commitItems.filter((it) => promotedKeys.has(`${it.type}/${it.name}`)), + ...(publishedSeqs.length + ? { eventSeqStart: Math.min(...publishedSeqs), eventSeqEnd: Math.max(...publishedSeqs) } + : {}), + }); + } + }); + } catch (e: any) { + // The batch rolled back — NOTHING landed (ADR-0067 D2). Report the + // causal item with its real error; every other draft is marked + // batch_aborted so the caller sees the all-or-nothing semantics + // instead of inferring them from publishedCount 0. + const causal = e?.__batchItem as { type: string; name: string } | undefined; + const failedOut = ordered.map((d) => + causal && d.type === causal.type && d.name === causal.name + ? { + type: d.type, name: d.name, + error: e?.message ?? 'publish failed', + ...(e?.code ? { code: e.code } : {}), + // Carry structured spec-validation issues so the publish + // surface can point at the offending field. + ...(Array.isArray(e?.issues) ? { issues: e.issues } : {}), + } + : { + type: d.type, name: d.name, + error: `not published — the batch is all-or-nothing (ADR-0067 D2) and ` + + `${causal ? `${causal.type}/${causal.name}` : 'another item'} failed; the transaction rolled back`, + code: 'batch_aborted', + }); + return { + success: false, + publishedCount: 0, + failedCount: failedOut.length, + published: [], + failed: failedOut, + }; + } + + // ═══ Phase 2 — side effects, after the metadata committed ═══ + // Registry refresh, DDL, materializers, projections, events — per item + // in publish order. Best-effort at the batch level: the metadata IS + // live at this point, so a side-effect failure must be surfaced (via + // materialize.failures / probes), never turned into a fake unpublish. + for (const p of promoted) { + published.push({ type: p.d.type, name: p.d.name, version: p.version }); + try { + const eff = await this.runPublishSideEffects({ + singularType: p.singularType, + requestType: p.d.type, + name: p.d.name, + orgId, + body: p.body, + packageId: p.packageId, ...(request.actor ? { actor: request.actor } : {}), - message: `publish app package '${request.packageId}'`, - _skipSeedApply: true, + skipSeedApply: true, }); - published.push({ type: d.type, name: d.name, version: r.version }); - if (typeof r.seq === 'number') publishedSeqs.push(r.seq); - if (r.materializeApplied) { + if (eff.materializeApplied) { materialize.any = true; - materialize.inserted += r.materializeApplied.inserted; - materialize.updated += r.materializeApplied.updated; - if (!r.materializeApplied.success) { + materialize.inserted += eff.materializeApplied.inserted; + materialize.updated += eff.materializeApplied.updated; + if (!eff.materializeApplied.success) { materialize.failures.push({ - type: d.type, name: d.name, - error: r.materializeApplied.error ?? 'materialize failed', + type: p.d.type, name: p.d.name, + error: eff.materializeApplied.error ?? 'materialize failed', }); } } } catch (e: any) { - failed.push({ - type: d.type, - name: d.name, - error: e?.message ?? 'publish failed', - ...(e?.code ? { code: e.code } : {}), - // Carry structured spec-validation issues so the publish - // surface can point at the offending field, not just report - // "N failed" (this catch used to flatten them to a message). - ...(Array.isArray(e?.issues) ? { issues: e.issues } : {}), + // Boot reconciliation heals registry/DDL/projection drift; the + // published metadata is authoritative. Surface, don't lie. + console.warn( + `[Protocol] publish side effects failed for ${p.d.type}/${p.d.name}: ${e?.message ?? e}`, + ); + materialize.any = true; + materialize.failures.push({ + type: p.d.type, name: p.d.name, + error: `side effects failed (metadata is live; boot reconciliation heals): ${e?.message ?? 'unknown'}`, }); } } @@ -4888,25 +5068,8 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol { } } - // ADR-0067 — record this turn as ONE commit (best-effort; never fails - // the publish). Only artifacts that actually published are in the revert - // plan, so a partial publish reverts exactly what landed. - let commit: { commitId: string } | null = null; - if (published.length > 0) { - const publishedKeys = new Set(published.map((p) => `${p.type}/${p.name}`)); - commit = await this.recordPackageCommit({ - orgId, - packageId: request.packageId, - operation: 'apply', - ...(request.message ? { message: request.message } : {}), - ...(request.actor ? { actor: request.actor } : {}), - ...(request.aiModel ? { aiModel: request.aiModel } : {}), - items: commitItems.filter((it) => publishedKeys.has(`${it.type}/${it.name}`)), - ...(publishedSeqs.length - ? { eventSeqStart: Math.min(...publishedSeqs), eventSeqEnd: Math.max(...publishedSeqs) } - : {}), - }); - } + // ADR-0067 D2 — the commit record was written INSIDE the Phase-1 + // transaction above, together with the promotions it describes. return { success: failed.length === 0 && published.length > 0, diff --git a/packages/objectql/src/engine-ambient-transaction.test.ts b/packages/objectql/src/engine-ambient-transaction.test.ts index c8b19a23c7..0a03fce56b 100644 --- a/packages/objectql/src/engine-ambient-transaction.test.ts +++ b/packages/objectql/src/engine-ambient-transaction.test.ts @@ -11,9 +11,16 @@ import { ObjectQL } from './engine.js'; function makeRecordingDriver() { const stores = new Map>(); - const seen: { create: Array<{ object: string; transaction: unknown }>; find: Array<{ object: string; transaction: unknown }> } = { + const seen: { + create: Array<{ object: string; transaction: unknown }>; + find: Array<{ object: string; transaction: unknown }>; + commit: unknown[]; + rollback: unknown[]; + } = { create: [], find: [], + commit: [], + rollback: [], }; const storeFor = (o: string) => { let s = stores.get(o); @@ -60,8 +67,8 @@ function makeRecordingDriver() { async bulkUpdate() { return []; }, async bulkDelete() {}, async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, - async commit() {}, - async rollback() {}, + async commit(trx: unknown) { seen.commit.push(trx); }, + async rollback(trx: unknown) { seen.rollback.push(trx); }, }; return { driver, seen }; } @@ -95,4 +102,35 @@ describe('engine ambient transaction (ADR-0034)', () => { await engine.insert('thing', { name: 'outside' }); expect(seen.create.at(-1)!.transaction).toBeUndefined(); }); + + // ADR-0067 D2 — a nested transaction() JOINS the ambient one instead of + // opening a second driver transaction (which would deadlock a + // single-connection pool and escape the outer rollback). The outer call + // owns the one-and-only commit/rollback. + it('a nested transaction() joins the ambient transaction (no second begin)', async () => { + let outerTrx: unknown; + let innerTrx: unknown; + await engine.transaction(async (ctx: any) => { + outerTrx = ctx.transaction; + await engine.transaction(async (innerCtx: any) => { + innerTrx = innerCtx.transaction; + await engine.insert('thing', { name: 'nested' }); + }); + }); + expect(innerTrx).toBe(outerTrx); // joined, not a fresh begin + expect(seen.create[0].transaction).toBe(outerTrx); + }); + + it('a throw inside a JOINED nested transaction() rolls back the OUTER one', async () => { + await expect(engine.transaction(async () => { + await engine.insert('thing', { name: 'first' }); + await engine.transaction(async () => { + throw new Error('inner boom'); + }); + })).rejects.toThrow('inner boom'); + // the recording driver saw the write, but the outer tx rolled back — + // rollback tracking lives on the driver; assert it was invoked. + expect(seen.rollback.length).toBe(1); + expect(seen.commit.length).toBe(0); + }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 42e155af62..d04802ca31 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2950,6 +2950,18 @@ export class ObjectQL implements IDataEngine { callback: (trxCtx: any) => Promise, baseContext?: any, ): Promise { + // ADR-0067 D2 — JOIN an already-open ambient transaction instead of + // opening a nested driver transaction. A nested begin would acquire a + // second connection (a deadlock on single-connection pools like the + // SQLite knex pool) and would NOT be covered by the outer rollback — + // exactly the half-landing this join prevents: an outer batch + // transaction (e.g. `publishPackageDrafts`) must own the one-and-only + // commit/rollback for every write made through nested helpers (the + // sys-metadata repository's `withTxn`, hook-driven writes, …). + const ambient = this.txStore.getStore(); + if (ambient?.transaction) { + return callback({ ...(baseContext ?? {}), transaction: ambient.transaction }); + } const driver = this.defaultDriver ? this.drivers.get(this.defaultDriver) : undefined; const drv = driver as any; if (!drv?.beginTransaction) { diff --git a/packages/objectql/src/protocol-commit-history.test.ts b/packages/objectql/src/protocol-commit-history.test.ts index 5eff21c50c..8a8f74c0d8 100644 --- a/packages/objectql/src/protocol-commit-history.test.ts +++ b/packages/objectql/src/protocol-commit-history.test.ts @@ -171,7 +171,14 @@ describe('ADR-0067 — publishPackageDrafts records a commit', () => { listDrafts: async () => [{ type: 'object', name: 'course' }], get: async () => null, }); - vi.spyOn(protocol, 'publishMetaItem' as never).mockResolvedValue({ success: true, version: 'h', seq: 7 } as never); + // ADR-0067 D2 — the batch promotes via the phase-1 seam (inside one + // transaction), not per-item publishMetaItem; side effects are phase 2. + vi.spyOn(protocol as any, 'promoteDraftForPublish').mockImplementation(async (req: any) => ({ + singularType: req.type, + orgId: null, + result: { version: 'h', seq: 7, item: { body: { name: req.name } }, packageId: null }, + })); + vi.spyOn(protocol as any, 'runPublishSideEffects').mockResolvedValue({}); const res: any = await (protocol as any).publishPackageDrafts({ packageId: 'app.edu', diff --git a/packages/objectql/src/protocol-publish-package-drafts.test.ts b/packages/objectql/src/protocol-publish-package-drafts.test.ts index 273dcfbb80..b7c2da6e2a 100644 --- a/packages/objectql/src/protocol-publish-package-drafts.test.ts +++ b/packages/objectql/src/protocol-publish-package-drafts.test.ts @@ -4,41 +4,74 @@ import { describe, it, expect, vi } from 'vitest'; import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; /** - * ADR-0033 — `publishPackageDrafts` promotes every pending draft bound to a - * package in one shot ("publish whole app"), reusing the per-item - * `publishMetaItem` primitive (no metadata-service dependency). These tests - * cover the orchestration: it publishes each listed draft, collects per-item - * failures without aborting, and reports an accurate success flag. + * ADR-0033 / ADR-0067 D2 — `publishPackageDrafts` promotes every pending + * draft bound to a package in one shot ("publish whole app"). Since ADR-0067 + * D2 the orchestration is TWO-PHASE: every promotion + the commit record run + * inside ONE engine transaction (all-or-nothing — a commit cannot half-land), + * and side effects (registry/DDL/materializers/projections) run after the + * metadata committed. These tests cover the orchestration contract; the + * per-item guards live in `publishMetaItem`'s own suites. */ function makeProtocol(drafts: Array<{ type: string; name: string }>) { const protocol = new ObjectStackProtocolImplementation({} as never); // Stub the bits that need a real engine/overlay so we can exercise the loop. (protocol as any).ensureOverlayIndex = async () => {}; (protocol as any).getOverlayRepo = () => ({ listDrafts: async () => drafts }); - const publishMetaItem = vi.spyOn(protocol, 'publishMetaItem' as never); - return { protocol, publishMetaItem }; + // Phase-1 / Phase-2 seams (ADR-0067 D2). + const promote = vi.spyOn(protocol as any, 'promoteDraftForPublish'); + const sideEffects = vi + .spyOn(protocol as any, 'runPublishSideEffects') + .mockResolvedValue({}); + const promoteOk = (req: any) => ({ + singularType: req.type, + orgId: null, + result: { version: 'h', seq: 1, item: { body: { name: req.name } }, packageId: null }, + }); + return { protocol, promote, sideEffects, promoteOk }; +} + +/** A fake engine whose transaction() tracks commit/rollback (ADR-0067 D2). */ +function makeTxnEngine() { + const txn = { began: 0, committed: 0, rolledBack: 0 }; + const engine = { + transaction: async (cb: (ctx: unknown) => Promise): Promise => { + txn.began += 1; + try { + const r = await cb({}); + txn.committed += 1; + return r; + } catch (e) { + txn.rolledBack += 1; + throw e; + } + }, + }; + return { engine, txn }; } -describe('protocol.publishPackageDrafts (ADR-0033)', () => { +describe('protocol.publishPackageDrafts (ADR-0033 / ADR-0067 D2)', () => { it('publishes every draft of the package and reports success', async () => { const drafts = [ { type: 'object', name: 'course' }, { type: 'object', name: 'student' }, { type: 'view', name: 'course_list' }, ]; - const { protocol, publishMetaItem } = makeProtocol(drafts); - publishMetaItem.mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + const { protocol, promote, sideEffects, promoteOk } = makeProtocol(drafts); + promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); - expect(publishMetaItem).toHaveBeenCalledTimes(3); - expect((publishMetaItem.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'course' }); + expect(promote).toHaveBeenCalledTimes(3); + expect((promote.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'course' }); + // Side effects ran once per promoted item, AFTER promotion, in order. + expect(sideEffects).toHaveBeenCalledTimes(3); + expect((sideEffects.mock.calls[0][0] as any)).toMatchObject({ requestType: 'object', name: 'course' }); expect(res).toMatchObject({ success: true, publishedCount: 3, failedCount: 0 }); expect(res.published.map((p) => p.name)).toEqual(['course', 'student', 'course_list']); }); it('rejects an object draft missing the package namespace prefix — atomic, before promoting', async () => { - const { protocol, publishMetaItem } = makeProtocol([ + const { protocol, promote } = makeProtocol([ { type: 'object', name: 'edu_course' }, { type: 'object', name: 'ticket' }, // missing the 'edu_' prefix ]); @@ -47,7 +80,7 @@ describe('protocol.publishPackageDrafts (ADR-0033)', () => { const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); - expect(publishMetaItem).not.toHaveBeenCalled(); // aborted BEFORE any promote + expect(promote).not.toHaveBeenCalled(); // aborted BEFORE any promote expect(res.success).toBe(false); expect(res.publishedCount).toBe(0); expect(res.failedCount).toBe(1); @@ -56,12 +89,12 @@ describe('protocol.publishPackageDrafts (ADR-0033)', () => { }); it('publishes compliant prefixed object drafts under a declared namespace', async () => { - const { protocol, publishMetaItem } = makeProtocol([ + const { protocol, promote, promoteOk } = makeProtocol([ { type: 'object', name: 'edu_course' }, { type: 'object', name: 'edu_student' }, ]); (protocol as any).engine = { registry: { getPackage: () => ({ manifest: { namespace: 'edu' } }) } }; - publishMetaItem.mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); @@ -71,41 +104,107 @@ describe('protocol.publishPackageDrafts (ADR-0033)', () => { it('skips the namespace check when the package declares no namespace (legacy grandfathered)', async () => { // No registry / no declared namespace → bare names still publish, exactly // as before this rule existed (mirrors defineStack's absent-namespace skip). - const { protocol, publishMetaItem } = makeProtocol([ + const { protocol, promote, promoteOk } = makeProtocol([ { type: 'object', name: 'course' }, // bare name, no prefix ]); - publishMetaItem.mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); expect(res).toMatchObject({ success: true, publishedCount: 1 }); }); - it('collects per-item failures without aborting the rest', async () => { - const { protocol, publishMetaItem } = makeProtocol([ + it('all-or-nothing (ADR-0067 D2): a mid-batch failure publishes NOTHING and stops the loop', async () => { + const { protocol, promote, sideEffects, promoteOk } = makeProtocol([ { type: 'object', name: 'course' }, { type: 'object', name: 'student' }, { type: 'view', name: 'course_list' }, ]); - publishMetaItem.mockImplementation((async (req: any) => { + promote.mockImplementation(async (req: any) => { if (req.name === 'student') throw Object.assign(new Error('locked'), { code: 'locked' }); - return { success: true, version: 'h', seq: 1 }; - }) as never); + return promoteOk(req); + }); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + + // The loop stopped AT the failure — the third draft was never attempted… + expect(promote).toHaveBeenCalledTimes(2); + // …no side effect ran (side effects are post-commit only)… + expect(sideEffects).not.toHaveBeenCalled(); + // …and NOTHING reports as published: the causal item carries its real + // error, every other draft is batch_aborted. + expect(res.success).toBe(false); + expect(res.publishedCount).toBe(0); + expect(res.published).toEqual([]); + expect(res.failedCount).toBe(3); + expect(res.failed.find((f) => f.name === 'student')).toMatchObject({ code: 'locked', error: 'locked' }); + expect(res.failed.find((f) => f.name === 'course')).toMatchObject({ code: 'batch_aborted' }); + expect(res.failed.find((f) => f.name === 'course_list')).toMatchObject({ code: 'batch_aborted' }); + }); + + it('wraps the batch in ONE engine transaction and rolls it back on failure', async () => { + const { protocol, promote, promoteOk } = makeProtocol([ + { type: 'object', name: 'course' }, + { type: 'object', name: 'student' }, + ]); + const { engine, txn } = makeTxnEngine(); + (protocol as any).engine = engine; + promote.mockImplementation(async (req: any) => { + if (req.name === 'student') throw new Error('boom'); + return promoteOk(req); + }); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + + expect(txn.began).toBe(1); + expect(txn.rolledBack).toBe(1); + expect(txn.committed).toBe(0); + expect(res).toMatchObject({ success: false, publishedCount: 0 }); + }); + + it('commits the transaction once on a clean batch', async () => { + const { protocol, promote, promoteOk } = makeProtocol([ + { type: 'object', name: 'course' }, + ]); + const { engine, txn } = makeTxnEngine(); + (protocol as any).engine = engine; + promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + expect(txn.began).toBe(1); + expect(txn.committed).toBe(1); + expect(txn.rolledBack).toBe(0); + expect(res).toMatchObject({ success: true, publishedCount: 1 }); + }); + + it('a side-effect failure does NOT unpublish — metadata is live, the failure is surfaced', async () => { + const { protocol, promote, sideEffects, promoteOk } = makeProtocol([ + { type: 'object', name: 'course' }, + { type: 'view', name: 'course_list' }, + ]); + promote.mockImplementation(async (req: any) => promoteOk(req)); + sideEffects.mockImplementation(async (args: any) => { + if (args.name === 'course_list') throw new Error('DDL hiccup'); + return {}; + }); + + const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); + + // Both items published (the metadata transaction already committed)… expect(res.publishedCount).toBe(2); - expect(res.failedCount).toBe(1); - expect(res.failed[0]).toMatchObject({ type: 'object', name: 'student', code: 'locked' }); - expect(res.success).toBe(false); // any failure → not a clean success + expect(res.success).toBe(true); + // …and the side-effect failure is SURFACED, not swallowed into a fake unpublish. + expect(res.materializeApplied?.success).toBe(false); + expect(res.materializeApplied?.failures?.[0]).toMatchObject({ name: 'course_list' }); }); it('returns publishedCount 0 / success false for an empty package', async () => { - const { protocol, publishMetaItem } = makeProtocol([]); + const { protocol, promote } = makeProtocol([]); const res = await protocol.publishPackageDrafts({ packageId: 'app.empty' }); - expect(publishMetaItem).not.toHaveBeenCalled(); + expect(promote).not.toHaveBeenCalled(); expect(res).toMatchObject({ success: false, publishedCount: 0, failedCount: 0 }); }); @@ -130,18 +229,24 @@ describe('protocol.publishPackageDrafts (ADR-0033)', () => { ? { body: seedBodyByName[ref.name], hash: 'h' } : null, }); - const publishMetaItem = vi.spyOn(protocol, 'publishMetaItem' as never); - publishMetaItem.mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + const promote = vi.spyOn(protocol as any, 'promoteDraftForPublish') + .mockImplementation(async (req: any) => ({ + singularType: req.type, + orgId: null, + result: { version: 'h', seq: 1, item: { body: { name: req.name } }, packageId: null }, + })); + const sideEffects = vi.spyOn(protocol as any, 'runPublishSideEffects').mockResolvedValue({}); const applySeedBodies = vi .spyOn(protocol as any, 'applySeedBodies') .mockResolvedValue({ success: true, inserted: 2, updated: 0 }); const res = await protocol.publishPackageDrafts({ packageId: 'app.pm' }); - // Object published BEFORE the seeds, and every publish suppressed per-item apply. - expect((publishMetaItem.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'project' }); - for (const call of publishMetaItem.mock.calls) { - expect((call[0] as any)._skipSeedApply).toBe(true); + // Object published BEFORE the seeds, and every item's side effects + // suppressed the per-item seed apply (batch pass below owns it). + expect((promote.mock.calls[0][0] as any)).toMatchObject({ type: 'object', name: 'project' }); + for (const call of sideEffects.mock.calls) { + expect((call[0] as any).skipSeedApply).toBe(true); } // ONE batch apply with BOTH seed bodies (cross-seed refs need a single pass). expect(applySeedBodies).toHaveBeenCalledTimes(1); @@ -153,8 +258,8 @@ describe('protocol.publishPackageDrafts (ADR-0033)', () => { }); it('omits seedApplied when the package has no seed drafts', async () => { - const { protocol, publishMetaItem } = makeProtocol([{ type: 'object', name: 'course' }]); - publishMetaItem.mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + const { protocol, promote, promoteOk } = makeProtocol([{ type: 'object', name: 'course' }]); + promote.mockImplementation(async (req: any) => promoteOk(req)); const res = await protocol.publishPackageDrafts({ packageId: 'app.edu' }); expect(res.seedApplied).toBeUndefined(); }); From 4d5e05c632e940d0ebb1940867a843a01361f97a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:20:14 +0000 Subject: [PATCH 2/3] fix(tests): migrate build-probes spy to the phase-1 seam; rescue orphaned search-conformance files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - build-probes.test.ts publish suite: spy promoteDraftForPublish + runPublishSideEffects (the batch no longer calls publishMetaItem per item, ADR-0067 D2). objectql suite back to 904/904. - The #3037 packages/dogfood -> packages/qa/dogfood restructure merged concurrently with #3065 and did not carry its three just-added files; they were orphaned at the old path outside every workspace (silently not running in CI). git mv them to packages/qa/dogfood/test — all 5 dogfood suites green at the new home (21 tests). --- packages/objectql/src/build-probes.test.ts | 8 +++++++- .../{ => qa}/dogfood/test/search-conformance.ledger.ts | 0 packages/{ => qa}/dogfood/test/search-conformance.test.ts | 0 .../{ => qa}/dogfood/test/showcase-search.dogfood.test.ts | 0 4 files changed, 7 insertions(+), 1 deletion(-) rename packages/{ => qa}/dogfood/test/search-conformance.ledger.ts (100%) rename packages/{ => qa}/dogfood/test/search-conformance.test.ts (100%) rename packages/{ => qa}/dogfood/test/showcase-search.dogfood.test.ts (100%) diff --git a/packages/objectql/src/build-probes.test.ts b/packages/objectql/src/build-probes.test.ts index 22190d12c8..d7d90bf5b2 100644 --- a/packages/objectql/src/build-probes.test.ts +++ b/packages/objectql/src/build-probes.test.ts @@ -183,7 +183,13 @@ describe('publishPackageDrafts — probes ride the response (ADR-0038 L3)', () = get: async (_ref: any, opts: any) => opts?.state === 'draft' ? { body: ITEMS['seed expense_sample'], hash: 'h' } : null, }); - vi.spyOn(protocol, 'publishMetaItem' as never).mockResolvedValue({ success: true, version: 'h', seq: 1 } as never); + // ADR-0067 D2 — the batch promotes via the phase-1 seam; side effects are phase 2. + vi.spyOn(protocol as any, 'promoteDraftForPublish').mockImplementation(async (req: any) => ({ + singularType: req.type, + orgId: null, + result: { version: 'h', seq: 1, item: { body: ITEMS[`${req.type} ${req.name}`] ?? { name: req.name } }, packageId: null }, + })); + vi.spyOn(protocol as any, 'runPublishSideEffects').mockResolvedValue({}); vi.spyOn(protocol as any, 'applySeedBodies').mockResolvedValue({ success: false, inserted: 0, updated: 0, error: 'boom' }); // Probe reads: active items + an engine whose table stayed empty. (protocol as any).getMetaItem = async ({ type, name }: any) => ({ item: ITEMS[`${type} ${name}`] }); diff --git a/packages/dogfood/test/search-conformance.ledger.ts b/packages/qa/dogfood/test/search-conformance.ledger.ts similarity index 100% rename from packages/dogfood/test/search-conformance.ledger.ts rename to packages/qa/dogfood/test/search-conformance.ledger.ts diff --git a/packages/dogfood/test/search-conformance.test.ts b/packages/qa/dogfood/test/search-conformance.test.ts similarity index 100% rename from packages/dogfood/test/search-conformance.test.ts rename to packages/qa/dogfood/test/search-conformance.test.ts diff --git a/packages/dogfood/test/showcase-search.dogfood.test.ts b/packages/qa/dogfood/test/showcase-search.dogfood.test.ts similarity index 100% rename from packages/dogfood/test/showcase-search.dogfood.test.ts rename to packages/qa/dogfood/test/showcase-search.dogfood.test.ts From 110d4e5212dc59e6b1161d815d75b37eda74f605 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 15:22:27 +0000 Subject: [PATCH 3/3] =?UTF-8?q?docs(adr-0067):=20flip=20to=20Accepted=20?= =?UTF-8?q?=E2=80=94=20Decision-2=20turn-atomicity=20lands=20with=20this?= =?UTF-8?q?=20PR=20(#3066)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/adr/0067-commit-history-and-rollback-for-ai-authoring.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0067-commit-history-and-rollback-for-ai-authoring.md b/docs/adr/0067-commit-history-and-rollback-for-ai-authoring.md index e7188e6f8a..5f9c994970 100644 --- a/docs/adr/0067-commit-history-and-rollback-for-ai-authoring.md +++ b/docs/adr/0067-commit-history-and-rollback-for-ai-authoring.md @@ -1,6 +1,6 @@ # ADR-0067: Commit history and rollback for AI authoring — turns become atomic, revertible commits -**Status**: Proposed (2026-06-24) — mostly implemented (2026-07-16 audit): commit grouping (`sys_metadata_commit`), `revertCommit`/`rollbackToPackageCommit`/`listCommits`, REST routes and tests all shipped; **Decision-2 (turn-atomic single-transaction apply, "a commit cannot half-land") is NOT implemented** — publish remains per-item best-effort and commits record partial publishes (`protocol.ts` records over `publishedKeys` after the fact). +**Status**: Accepted (2026-06-24; completed 2026-07-16) — fully implemented: commit grouping (`sys_metadata_commit`), `revertCommit`/`rollbackToPackageCommit`/`listCommits`, REST routes; **Decision-2 landed via #3066**: `publishPackageDrafts` runs every promotion + the commit record inside ONE `engine.transaction()` (two-phase — side effects post-commit), so a commit cannot half-land; `engine.transaction()` joins ambient transactions to make nested repository writes participate. Locked by `protocol-publish-package-drafts.test.ts` (all-or-nothing + rollback tracking) and `engine-ambient-transaction.test.ts`. **Deciders**: ObjectStack Protocol Architects **Builds on / amends**: [ADR-0045](./0045-additive-materialization-and-visibility-gate.md) (**amended**: ADR-0045 keeps a *draft + human-confirm* gate on mutations as the safety mechanism; this ADR replaces *confirm-before* with *revert-after* for everything except irreversible data loss, and unifies the two authoring regimes under one primitive — the commit), [ADR-0027](./0027-metadata-authoring-lifecycle.md) (draft workspace — retained as a *review affordance*, demoted from *safety mechanism*), [ADR-0033](./0033-ai-assisted-metadata-authoring.md) ("AI never publishes — it drafts" → **AI commits; commits are revertible**), [ADR-0034](./0034-transactional-writes-and-ambient-transaction.md) (per-write transaction — **extended to span a whole turn**), [ADR-0038](./0038-build-verification-loop.md) (machine gate — runs per commit, before it lands) **Consumers**: `@objectstack/objectql` (commit grouping, atomic turn-apply, `revertCommit`, history query — built on the existing `sys_metadata_history` + `restoreVersion`), `@objectstack/runtime` + `@objectstack/rest` (commit/revert routes), `../cloud/service-ai-studio` (turn = commit; auto-commit policy; data-loss confirmation), `../objectui` (commit timeline + "revert to here")