diff --git a/.changeset/org-gated-registry-heal.md b/.changeset/org-gated-registry-heal.md new file mode 100644 index 0000000000..519a00d410 --- /dev/null +++ b/.changeset/org-gated-registry-heal.md @@ -0,0 +1,59 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): an org-scoped metadata DELETE no longer evicts the env-wide registry entry (#6780) + +`restoreArtifactRegistryView` — the three-tier heal that repairs the in-memory +`SchemaRegistry` after an overlay-row delete — was `(type, name)`-addressed and +org-blind. Every tier writes the PLAIN key (`removeRuntimeShadow` drops it, the +layer-2 re-register rewrites it, `removeOverlayEntry` retires it), and per +ADR-0005 that one plain-key entry belongs to the **env-wide** row: an org-scoped +overlay never enters the process-wide registry at all (#6602). `deleteMetaItem` +called the heal on all three of its paths without passing the delete's own +scope, so org A resetting **its own** customization reached in and evicted the +entry every other org and the control plane read. + +Measured before the fix on an unscoped (control-plane) kernel — the shape #5086 +found the flagship showcase booting with: + +``` +after env-wide save : "Env grid" +after org A save : "Env grid" # #6602 holding — the org row stays out +delete receipt (org A) : { success: true, reset: true, … } +after org A DELETE : undefined # the eviction +rows left : [{ name: "shared_grid", org: null, state: "active" }, …] +``` + +The env-wide row is still in `sys_metadata`; only its registry entry is gone. +While it is gone, direct registry readers answer as if the item does not exist +— ADR-0110 D3's declaration gate, `resolveRouteActionDeclaration`, and +fail-closed `assertObjectRegistered` (404). One tenant's "reset my +customization" therefore degraded every other tenant's runtime until restart. +The no-row **self-heal** branch was the cheaper door still: an org that had +never customized anything could evict the entry with a single no-op DELETE +(`reset: false`, "nothing to delete") — so a gate on the delete-ful branch +alone would have left it open. + +The scope verdict now lives INSIDE the helper as a **required** +`organizationId` parameter — the `hydrateOverlayIntoRegistry` shape #6602 used +on the register side — rather than as a test repeated at each call site. There +are four call sites, not the two the report named: `deleteMetaItem` has three +(self-heal, post-`repo.delete`, legacy raw-engine path) and `revertCommit` one. +A required parameter makes the next caller answer at compile time; an optional +one would default an omission back to "env-wide" and reinstate the hole. PR +#6807's call-site gate on the revert limb is now redundant-not-contradictory +and was folded into the argument it passes — its pin still covers the batch +path, and it goes red if the gate is ever removed. + +**Register wide, retire narrow.** The write-through's `object` carve-out stays +un-org-gated and deliberately does not transfer to removal: it rests on +`assertObjectRegistered` failing CLOSED, so a surplus entry degrades to +"listable but rowless" and the next reload heals it, whereas a wrongly retired +entry 404s data CRUD for every tenant. + +Unchanged: row-level delete behaviour (an org-scoped delete still removes the +org row, and the org's next read falls through to the env-wide body); the +env-wide delete's full three-tier walk (#6687 tier 1 un-shadowing, #5079 tier 3 +retirement); and the kernel-scope gate, which still guards re-registration +only because that is a fact about the kernel, not about the row. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index de71ce9ccb..64d3b2111a 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -7925,10 +7925,60 @@ export class ObjectStackProtocolImplementation implements * Only the READ is now unconditional, because a project kernel needs the * same evidence before retiring an entry. * + * ## Why `organizationId` is a REQUIRED parameter (#6780) + * + * Every tier above is `(type, name)`-addressed: `removeRuntimeShadow` + * drops the PLAIN key, the layer-2 re-register writes the PLAIN key, and + * `removeOverlayEntry` retires the PLAIN key. There is exactly one + * plain-key entry per (type, name) in a process, and per ADR-0005 it + * belongs to the ENV-WIDE row — an org-scoped overlay never enters the + * registry at all (the rule {@link hydrateOverlayIntoRegistry} owns since + * #6602). So a heal run on behalf of an ORG-scoped delete cannot address + * anything of its own: it can only un-shadow or retire the entry every + * other org and the control plane read. + * + * Measured on `origin/main` before this gate existed: env-wide + * `view/shared_grid` in the registry → org A saves its own overlay (the + * entry correctly stays `Env grid`, #6602 holding) → org A DELETEs ITS + * OWN overlay → `registry.getItem('view','shared_grid')` is `undefined` + * while the env-wide row still sits in `sys_metadata`. While the entry is + * gone, direct registry readers answer as if the item does not exist + * (ADR-0110 D3's declaration gate, `resolveRouteActionDeclaration`, + * fail-closed `assertObjectRegistered` → 404) — one tenant's "reset my + * customization" degrading every other tenant's runtime on the unscoped + * kernels #5086 measured the flagship showcase booting with. + * + * The verdict lives HERE rather than at the call sites for the reason + * {@link hydrateOverlayIntoRegistry} states on the register side (#6602 / + * PR #6779): this is the ONE choke point all four heal callers already + * route through, and a REQUIRED (never optional) `organizationId` makes a + * fifth caller answer the question at compile time. An optional parameter + * would default an omission to "env-wide" and reinstate the exact hole. + * + * REGISTER WIDE, RETIRE NARROW — the asymmetry is deliberate. The + * write-through's `object` branch is NOT org-gated ({@link + * applyRegistryWriteThrough}), and that carve-out does not transfer to + * removal: it is argued from `assertObjectRegistered` failing CLOSED, so + * a surplus entry degrades to "listable but rowless" and the next reload + * heals it, while a wrongly retired entry 404s data CRUD for every tenant. + * The two costs are not symmetric, so the two gates are not either. + * + * The KERNEL-scope gate stays where it was: `environmentId === undefined` + * still guards re-registration only, because that is a fact about the + * kernel this protocol instance serves, not about the row in hand. + * * Best-effort: a failure must never block the delete that already * succeeded; the next full reload fixes the registry anyway. */ - private async restoreArtifactRegistryView(type: string, name: string): Promise { + private async restoreArtifactRegistryView( + type: string, + name: string, + /** The DELETE's own scope — `null` for an env-wide row. [#6780] */ + organizationId: string | null, + ): Promise { + // [#6780] ADR-0005 — the plain-key entry belongs to the env-wide row, + // so only an env-wide removal may heal (or retire) it. + if (organizationId !== null && organizationId !== undefined) return; try { const registry: any = this.engine.registry; const singular = PLURAL_TO_SINGULAR[type] ?? type; @@ -10584,9 +10634,17 @@ export class ObjectStackProtocolImplementation implements // is argued from `assertObjectRegistered` failing CLOSED, // which licenses registering broadly and never retiring // broadly. Register wide, retire narrow. - if (orgId === null) { - await this.restoreArtifactRegistryView(it.type, it.name); - } + // + // [#6780] The verdict this comment argues now lives INSIDE + // {@link restoreArtifactRegistryView} as a REQUIRED + // parameter, so `orgId` is handed over rather than tested + // here: PR #6807's call-site `if (orgId === null)` guarded + // this ONE caller while the sibling `deleteMetaItem` — the + // caller this limb was modelled on — had the same hole on + // all three of its own call sites. The gate moved to the + // choke point every caller shares; the pin below this + // comment is unchanged and still covers the batch path. + await this.restoreArtifactRegistryView(it.type, it.name, orgId); reverted.push({ type: it.type, name: it.name, action: 'removed' }); } else if (it.prevVersion !== null && it.prevVersion !== undefined) { // Edited an existing artifact → restore the pre-commit body. @@ -11100,8 +11158,18 @@ export class ObjectStackProtocolImplementation implements // shadow may linger in the registry (e.g. pollution from // before this fix shipped) — drop it so the artifact // view really IS the default we claim below. + // + // [#6780] `orgId` is passed, and this branch is where it + // matters MOST: with no row to delete, an org that never + // customized anything at all could evict the env-wide + // plain-key entry with a single no-op DELETE. Measured on + // `origin/main`: receipt `{reset: false, "nothing to + // delete"}` and `registry.getItem('view','shared_grid')` + // → `undefined`, the env-wide row untouched in + // `sys_metadata`. A gate applied only to the delete-ful + // branch below would have left this door standing open. if (targetState === 'active') { - await this.restoreArtifactRegistryView(request.type, request.name); + await this.restoreArtifactRegistryView(request.type, request.name, orgId); } return { success: true, @@ -11148,8 +11216,16 @@ export class ObjectStackProtocolImplementation implements // see {@link restoreArtifactRegistryView}. Draft discards // skip this: drafts never hydrate into the registry, and the // still-active overlay (if any) must keep its shadow. + // + // [#6780] SCOPED by the same `orgId` the row was deleted with, + // so the registry's view cannot disagree with the row's scope + // — the sibling rule the write side states in + // {@link applyRegistryWriteThrough}. Org A resetting ITS OWN + // overlay used to retire the plain-key entry belonging to the + // ENV-WIDE row, i.e. one tenant's "reset to default" blanked + // the item for every other tenant and the control plane. if (targetState === 'active') { - await this.restoreArtifactRegistryView(request.type, request.name); + await this.restoreArtifactRegistryView(request.type, request.name, orgId); } // Storage teardown (opt-in): drop the now-orphaned physical table @@ -11287,8 +11363,17 @@ export class ObjectStackProtocolImplementation implements } } + // [#6780] The legacy path deletes under the SAME org predicate it + // built into `scopedWhere` above, so the heal reads its scope from + // the same place. Reachable only on a control-plane kernel for a + // code-only type, which is exactly the kernel whose registry every + // org shares — the narrowest path and the widest blast radius. if (request.state !== 'draft') { - await this.restoreArtifactRegistryView(request.type, request.name); + await this.restoreArtifactRegistryView( + request.type, + request.name, + request.organizationId ?? null, + ); } return { diff --git a/packages/objectql/src/protocol-org-overlay-registry-gate.test.ts b/packages/objectql/src/protocol-org-overlay-registry-gate.test.ts index 07fbd98402..5b1633c54c 100644 --- a/packages/objectql/src/protocol-org-overlay-registry-gate.test.ts +++ b/packages/objectql/src/protocol-org-overlay-registry-gate.test.ts @@ -447,10 +447,19 @@ describe('#6602 — the delete chain needs no re-keying under this fix', () => { }); it('an org-scoped delete has no plain-key entry of its own to retire', async () => { - // The whole argument for leaving `restoreArtifactRegistryView` alone: - // the delete chain is `(type, name)`-addressed and org-blind, but with - // both entry seams refusing org rows there is nothing org-scoped in the + // The argument for leaving `restoreArtifactRegistryView` alone: the + // delete chain is `(type, name)`-addressed and org-blind, but with both + // entry seams refusing org rows there is nothing org-scoped in the // registry for it to mis-address. + // + // [#6780] TRUE OF THIS CASE, AND ONLY THIS CASE — measured later, and + // the correction is the next describe block. The name here is org A's + // alone, so the plain key really is empty and the org-blind heal has + // nothing to hit. Give the name an ENV-WIDE row as well and the same + // heal addresses that row's entry instead: `(type, name)` cannot tell + // the two apart, so "no entry of its own" was never "no entry". The + // fix keeps this file's conclusion (no org-scoped registry keys) and + // adds the missing half (an org-scoped delete may not heal at all). await protocol.saveMetaItem({ type: 'view', name: 'org_grid', @@ -464,3 +473,215 @@ describe('#6602 — the delete chain needs no re-keying under this fix', () => { expect(registry.getItem('view', 'org_grid')).toBeUndefined(); }); }); + +/** + * #6780 — an ORG-scoped metadata DELETE must not evict the ENV-WIDE registry + * entry that every org and the control plane read. + * + * The block above closed the two ENTRY seams (write-through + read-side + * hydration) and concluded the delete chain needed no re-keying. That + * conclusion holds for a name only one org has touched — and fails for the + * name that matters, because `restoreArtifactRegistryView` is + * `(type, name)`-addressed and its every tier writes the PLAIN key: + * `removeRuntimeShadow` drops it, the layer-2 re-register rewrites it, + * `removeOverlayEntry` retires it. Per ADR-0005 that one plain key belongs to + * the ENV-WIDE row. So org A's delete could only ever hit somebody else's + * entry. + * + * Measured on `origin/main` (5e247fd6b) BEFORE the fix, the card's sequence: + * + * after env save : "Env grid" + * after org save : "Env grid" ← #6602 holding + * delete receipt : {"success":true,"reset":true,…} + * after org DELETE : undefined ← the eviction + * rows left : [{"n":"shared_grid","org":null,"state":"active"}, …] + * + * — the env-wide ROW still in `sys_metadata`, its registry entry gone. While + * it is gone, direct registry readers answer as if the item does not exist + * (ADR-0110 D3's declaration gate, `resolveRouteActionDeclaration`, + * fail-closed `assertObjectRegistered` → 404), so one tenant's "reset my + * customization" degrades every other tenant's runtime. Pre-existing; #6602 + * neither introduced nor covered it. + * + * ── The shape, and why (b) rather than (a) ───────────────────────────── + * + * The verdict lives INSIDE the helper as a REQUIRED `organizationId` + * parameter — the {@link hydrateOverlayIntoRegistry} shape #6602/PR #6779 + * used on the register side — rather than as a gate repeated at each call + * site. Measured reason: there are FOUR call sites, not the two the card + * names — `deleteMetaItem` has three (self-heal, post-`repo.delete`, legacy + * raw-engine path) and `revertCommit` one. PR #6807 had already gated the + * revert one; a call-site fix would have had to find the other three, and + * the legacy raw-engine path is exactly the kind a sweep misses. A required + * parameter makes a fifth caller answer at compile time. #6807's call-site + * `if (orgId === null)` is now redundant-not-contradictory and was folded + * into the argument it passes; its pin ("an ORG-scoped soft-remove leaves the + * env-wide registry entry alone", protocol-commit-history.test.ts) is + * untouched and still covers the batch path. + * + * REGISTER WIDE, RETIRE NARROW: the write-through's `object` carve-out is + * deliberately NOT org-gated, and that does not transfer here — it rests on + * `assertObjectRegistered` failing CLOSED, so a surplus entry degrades to + * "listable but rowless" and the next reload heals it, while a wrongly + * retired entry 404s data CRUD for every tenant. + * + * ── Reverse verification, direction predicted BEFORE running ─────────── + * + * Ordinary red, with deliberately green controls. Deleting the + * `organizationId` refusal from `restoreArtifactRegistryView` must turn the + * three org-scoped REGISTRY cases red — reproducing the card's `undefined` + * and, for the artifact case, the un-shadowing that precedes it — while the + * three env-wide cases stay green (an env-wide delete is exactly what the + * heal is FOR: #6687's three-tier walk must not regress) and so does the + * org-scoped ROW control, which asserts the delete itself and never reads the + * registry. Predicted 3 red / 4 green in this block; measured 3 red / 4 green: + * + * × …the card's sequence → expected undefined to be defined + * × …the SELF-HEAL branch → expected undefined to be 'Env grid' + * × …the runtime-SHADOW tier → expected 'Artifact grid' to be 'Env grid' + * ✓ the ORG ROW is still removed · ✓ tier 1 · ✓ tier 3 · ✓ env-wide + org overlay + * + * A FOURTH red lands in a file this change did not edit, and it is the point + * of shape (b) rather than a surprise: PR #6807's own pin + * (`protocol-commit-history.test.ts` → "an ORG-scoped soft-remove leaves the + * env-wide registry entry alone") goes red too — `expected null to be + * 'EnvWide'` — because its call-site `if (orgId === null)` was folded into + * the argument it now passes. The gate moved; the coverage did not. + */ +describe('#6780 — the registry heal is org-gated: an org DELETE never evicts the env-wide entry', () => { + let registry: SchemaRegistry; + let engine: any; + let protocol: ObjectStackProtocolImplementation; + + /** A code-shipped `view` under a composite key — the tier-1 layer. */ + const PKG = 'com.objectstack.test-pkg'; + const artifactView = () => ({ + ...viewBody('shared_grid', 'Artifact grid'), + _packageId: PKG, + _packageVersion: '1.0.0', + _provenance: 'package', + }); + + beforeEach(() => { + registry = new SchemaRegistry({ multiTenant: false }); + registry.logLevel = 'silent'; + engine = makeEngine(registry); + // No environmentId — the unscoped control-plane kernel #5086 measured + // the flagship showcase booting with, and the one whose registry every + // org in the process shares. + protocol = new ObjectStackProtocolImplementation(engine); + }); + + it("org A deleting its OWN overlay leaves the env-wide entry standing (the card's sequence)", async () => { + await protocol.saveMetaItem({ type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Env grid') }); + await protocol.saveMetaItem({ + type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Org A grid'), organizationId: ORG_A, + }); + // #6602 holding: the org write never reached the shared registry. + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + + await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid', organizationId: ORG_A }); + + // Pre-fix this read was `undefined` — the whole defect, in one line. + expect(registry.getItem('view', 'shared_grid')).toBeDefined(); + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + }); + + it('the same delete still removes the ORG ROW — row-level behaviour is untouched', async () => { + // The control that keeps the case above from passing for the wrong + // reason: a "fix" that skipped the delete entirely would also leave the + // env-wide entry standing. The reset must still reset. + await protocol.saveMetaItem({ type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Env grid') }); + await protocol.saveMetaItem({ + type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Org A grid'), organizationId: ORG_A, + }); + const beforeDelete: any = await protocol.getMetaItem({ + type: 'view', name: 'shared_grid', organizationId: ORG_A, + }); + expect(beforeDelete.item.label).toBe('Org A grid'); + + const deleted = await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid', organizationId: ORG_A }); + + expect(deleted.success).toBe(true); + expect(deleted.reset).toBe(true); + // Org A now falls through to the env-wide body — ADR-0005's "reset to + // default", which is what the org author actually asked for. + const afterDelete: any = await protocol.getMetaItem({ + type: 'view', name: 'shared_grid', organizationId: ORG_A, + }); + expect(afterDelete.item.label).toBe('Env grid'); + }); + + it('the SELF-HEAL branch respects the same scope — a no-op org DELETE is inert', async () => { + // The cheapest eviction door of the three, and the one a gate on the + // delete-ful branch alone would have left open: org A has no overlay + // row at all, so the delete answers `reset: false` … and pre-fix still + // ran the heal. Measured on `origin/main`: + // delete receipt : {"success":true,"reset":false,"…nothing to delete."} + // after org DELETE : undefined + await protocol.saveMetaItem({ type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Env grid') }); + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + + const deleted = await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid', organizationId: ORG_A }); + + expect(deleted.reset).toBe(false); + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + }); + + it('the runtime-SHADOW tier is org-gated too: the env-wide overlay keeps shadowing its artifact', async () => { + // Tier 1 of the walk, which retires nothing and is still wrong for an + // org-scoped delete: un-shadowing hands every reader the PACKAGED body + // while the env-wide overlay row is still in force. + registry.registerItem('view', artifactView(), 'name', PKG); + await protocol.saveMetaItem({ + type: 'view', name: 'shared_grid', packageId: PKG, item: viewBody('shared_grid', 'Env grid'), + }); + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + + await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid', organizationId: ORG_A }); + + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + // The artifact is still there under its composite key, unharmed. + expect((registry.getArtifactItem('view', 'shared_grid') as any)?.label).toBe('Artifact grid'); + }); + + it('an ENV-WIDE delete still un-shadows the artifact — #6687 tier 1 not regressed', async () => { + // The green half. This is what the heal is FOR: the env-wide overlay + // goes away and the packaged default becomes visible again. + registry.registerItem('view', artifactView(), 'name', PKG); + await protocol.saveMetaItem({ + type: 'view', name: 'shared_grid', packageId: PKG, item: viewBody('shared_grid', 'Env grid'), + }); + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Env grid'); + + await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid' }); + + expect((registry.getItem('view', 'shared_grid') as any)?.label).toBe('Artifact grid'); + }); + + it('an ENV-WIDE delete of a runtime-only item still RETIRES the entry — #5079 tier 3 not regressed', async () => { + await protocol.saveMetaItem({ type: 'view', name: 'env_grid', item: viewBody('env_grid', 'Env grid') }); + expect(registry.getItem('view', 'env_grid')).toBeDefined(); + + await protocol.deleteMetaItem({ type: 'view', name: 'env_grid' }); + + expect(registry.getItem('view', 'env_grid')).toBeUndefined(); + }); + + it('an env-wide delete heals even while an org overlay of the same name exists', async () => { + // The direction the gate must NOT over-reach in: the org row is not a + // reason to leave the env-wide entry stale. Scope is read from the + // DELETE, never from what else happens to be stored. + await protocol.saveMetaItem({ type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Env grid') }); + await protocol.saveMetaItem({ + type: 'view', name: 'shared_grid', item: viewBody('shared_grid', 'Org A grid'), organizationId: ORG_A, + }); + + await protocol.deleteMetaItem({ type: 'view', name: 'shared_grid' }); + + expect(registry.getItem('view', 'shared_grid')).toBeUndefined(); + // Org A's own overlay row is untouched by an env-wide delete. + const forOrgA: any = await protocol.getMetaItem({ type: 'view', name: 'shared_grid', organizationId: ORG_A }); + expect(forOrgA.item.label).toBe('Org A grid'); + }); +});