MemberJunction / MemberJunction/MJ
Open App install cannot bound what it wrote to the shared core schema, so a failed install cannot fully roll back
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
A failed Open App install cannot say what it wrote to the shared `__mj` core schema, so it cannot fully undo it. Today the gap is covered by asking each app publisher to hand-author inverse `DELETE` scripts (`migrations.teardownDirectory`). That is the wrong layer: it makes the platform's atomicity guarantee depend on every publisher writing and maintaining correct inverse SQL forever, and it is currently adopted by exactly one app.
This issue collects five findings from the #3451 / #3469 investigation that are all the same underlying problem, and proposes a direction that removes the publisher burden entirely.
## Why now
#3469 moved app install to `TransactionMode: 'per-migration'` (matching `mj migrate`, per Amith's ruling) because `'per-run'` cannot host an app that creates a SQL Server table type — see #3451. That was the right call and it unblocked installs that were previously impossible. But it also means a migration set that fails partway leaves earlier migrations committed, so the install's all-or-nothing guarantee now lives in application-level compensation rather than in the database.
#3469 made that compensation real (remove entity metadata → run teardown → drop schema). This issue is about the part it could not close.
## The five faces
**1. `per-migration` does not make table types safe — publisher file layout does.**
`per-migration` wraps *each file* in a transaction. A `CREATE TYPE … AS TABLE` and its first `DECLARE @x t` still self-deadlock (error 1205) when they land in the **same** file. Verified directly during the #3451 investigation.
So the current state is not "solved", it is "works if the publisher happens to split the statements across files, and deadlocks with no actionable diagnostic if they don't." That is an implicit convention holding up a correctness property.
**2. Core-schema writes are not rolled back on failure.**
Dropping the app's own schema cannot reach rows its migrations wrote into `__mj`. `RemoveAppEntityMetadata` recovers what is FK-reachable from the app's entities, which in practice is most of it — but not all (see 4).
**3. Nothing validates that an app's migrations write to the core schema at all.**
The engine has no view of an app's intended footprint. An app can write anywhere in `__mj` and the installer will neither know nor object.
**4. Free-floating core rows carry no ownership marker.**
Checked against a live schema: `OpenAppID` exists only on `OpenAppDependency` and `OpenAppInstallHistory`. `Action`, `AIAgent`, `AIPrompt`, `Query`, and `Template` have no column identifying which app created a row. Those are exactly the rows the FK-graph walk in (2) cannot attribute, so they survive a rollback.
**5. Teardown puts the burden on publishers.**
`migrations.teardownDirectory` is the current answer to (2) and (4). It requires each publisher to author inverse `DELETE`s and keep them in sync with their seeds indefinitely. It also has two properties worth recording:
- **Rollback depends on a network round-trip.** `HandleTeardown` downloads the teardown directory from GitHub *at rollback time* — a different repo folder than the one fetched during install, so there is no local copy to reuse. An install that failed because the network failed will fail teardown for the same reason.
- **There is no safe fixed ordering.** Measured experimentally: teardown-before-drop fails with 547 (an FK from the app's schema pins a core row); drop-before-teardown fails with 208 (a teardown script reads an app-schema table). The trade is symmetric, so retry — not reordering — is the only general fix.
## Proposed direction: hoist the transaction-hostile DDL, restore one transaction
The deadlock is narrow and specific: `CREATE TYPE … AS TABLE` followed by an instantiation of that type in the same transaction. Nothing else in a migration set is transaction-hostile.
So: pre-scan the pending set, execute the `CREATE TYPE … AS TABLE` statements **outside** any transaction, then run the entire remaining set as **one transaction**.
The leftover is harmless because table types are schema-scoped, so a hoisted type always lands in the app's own schema — which compensation drops on failure. The two cleanup mechanisms then partition the write-set exactly:
- everything in the app's schema → covered by `DROP SCHEMA`
- everything else, including every core-schema write → covered by `ROLLBACK`
Nothing falls between them. This removes the need for teardown scripts, publisher-authored inverse SQL, migration restructuring, ownership columns, and CodeGen changes. It also makes (1) go away: correctness stops depending on file layout or transaction mode.
Pairs well with a static guard rejecting a `CREATE TYPE` that targets the core schema, and — separately — rejecting core-schema writes from migrations that have no business making them (3).
### Not yet proven
**The hoist has not been tested.** The reasoning is that the Sch-M lock releases when the DDL commits outside the transaction, so the later `DECLARE` inside the transaction takes its Sch-S cleanly — but that is inference, not a result. The repro harness from #3451 can validate or kill this cheaply and should do so before anyone builds on it.
Two further things to measure before committing:
- **Does Skyway write its history row inside the `per-run` transaction?** If it does not, a rolled-back set could still be recorded as applied, which would be worse than today. This is the load-bearing assumption.
- **Lock duration.** One transaction across a whole install holds locks on shared core tables for its full span, which blocks other users on a busy instance.
### Alternative considered and rejected
Splitting migrations into DDL and seed directories (seeds in one transaction) achieves the same guarantee, but changes the manifest and migration structure for every publisher. Rejected as too invasive for the benefit.
## Relationship to other issues
- **#3451** — the deadlock. Fixed by #3469.
- **#3469** — made compensation real. This issue is what it could not close.
- **#3505** — `spCreate*` idempotency. Note the team has since ruled that **install must not be idempotent** — it should succeed or fail cleanly. The direction above removes the PK-collision scenario by making rollback complete, rather than by making re-inserts tolerant.
- **#3506** — the teardown step has never executed real SQL. Now testable: `bizapps-sonar` v0.5.0 is the first app to ship a `teardownDirectory`.
Teardown stays supported for apps that already have it; the intent is that the engine's guarantee no longer depends on it.
Contributor guide
Research direction
Start with the repro harness from #3451 and test whether hoisting CREATE TYPE statements allows the remaining migrations to run in one transaction. Verify Skyway's history-row behavior, rollback coverage for shared core writes, and lock duration before choosing the design. Done means failed installs restore both app-schema and core-schema state without relying on teardown scripts.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- sql
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100