MemberJunction / MemberJunction/MJ
Open App install runs all migrations in one transaction, which SQL Server cannot always host (CREATE TYPE + TVP self-deadlock, 1205)
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
Open App installs run the entire pending migration set inside **one database transaction** (Skyway's `per-run` default), and use that transaction as the install's rollback mechanism. SQL Server cannot host that transaction in the general case: **a transaction that creates a table type and then instantiates a variable of that type deadlocks against itself** (error 1205). On a from-zero install every migration is pending, so the whole app is one transaction and *no arrangement of migration files avoids it*.
The result is that an app which ships a table type and uses it — a standard T-SQL pattern — **cannot be installed from a clean database**, and it fails as an opaque deadlock rather than an actionable error.
This is not a bad migration. The minimal reproduction is two statements with no MemberJunction involved.
## Minimal reproduction
Run against any SQL Server database. No MJ, no Skyway, no migration files.
```sql
BEGIN TRAN;
CREATE TYPE dbo.WidgetIDList AS TABLE (ID UNIQUEIDENTIFIER NOT NULL PRIMARY KEY);
GO
DECLARE @ids dbo.WidgetIDList; -- Msg 1205: deadlocked on lock resources
GO
COMMIT;
```
Verified on **SQL Server 2022 RTM-CU25 (16.0.4255.1)**, deterministic across repeated runs.
**Mechanism.** The `CREATE TYPE` holds a Sch-M lock on the type for the life of the transaction. Instantiating a table-valued variable runs in a **nested system transaction** that does not share the session's lock ownership, so it requests Sch-S on that same type and waits on its own session's Sch-M. Single-session self-deadlock — it reproduces with no other connection to the server. A captured `system_health` deadlock graph from the real failure shows exactly this: owner and waiter are the same process, resource `USER_TYPE`, waiter a system transaction named `@ids`.
## Scope — what does and does not trigger it
| Scenario (all inside one transaction) | Result |
|---|---|
| `CREATE TYPE … AS TABLE` + bare `DECLARE` of that type | **1205** |
| … + a statement trigger that declares the TVP, fired by DML | **1205** |
| … + a stored procedure that declares the TVP, then `EXEC` | **1205** |
| … + a stored procedure with a **TVP parameter**, then `EXEC` passing one | **1205** |
| Ordinary `CREATE TABLE` + `INSERT` + `SELECT` | ✅ commits |
| User-defined **scalar** type created and used | ✅ commits |
| Table type **committed first**, then everything else | ✅ commits |
| Trigger/procedure *created* in the transaction, *used* after commit | ✅ commits |
So it is specific to **table types**, and specifically to *instantiation* — creating the consumer is fine. Note the trigger is incidental; it is not required to reproduce.
## Why this reaches the install path and not `mj migrate`
- **MJCLI** (`packages/MJCLI/src/config.ts`) declares a user-facing `transactionMode` defaulting to `'per-migration'` and forwards it to Skyway, so `mj migrate` commits each file separately.
- **`RunAppMigrations`** (`packages/OpenApp/Engine/src/install/migration-runner.ts`) never sets `TransactionMode`. The option is declared only on the module's *internal* Skyway config interface — the public `MigrationRunOptions` has no such field — so no caller can select a mode, and skyway-core's `TransactionMode: config.TransactionMode ?? 'per-run'` applies.
Two migration paths in the same repo therefore have silently different transaction semantics.
## Why it is invisible until a customer hits it
Incremental development databases never reproduce it: the type was committed by an earlier run, so its lock is long released. It appears only on a **from-zero clean install** — i.e. on the real install path, at the customer, never on the author's machine. App test harnesses that run against an already-migrated database exercise the objects but never the from-zero transaction window.
## `per-migration` is a mitigation, not the fix
Switching the mode narrows the transaction from *the whole run* to *one file*. It does not eliminate the class: a migration file that contains both the `CREATE TYPE` and something that instantiates it still deadlocks (row 2 of the table above, collapsed into one file). It would also require an app to keep the type in a separate migration forever, and it trades away the all-or-nothing guarantee that the current design is presumably there to provide.
## Discussion — where the fix belongs
The underlying conflation is that **the atomicity of the install *operation* is bound to a single *database transaction***, which the engine cannot always provide. Worth deciding:
1. **Should install atomicity come from compensation rather than from one transaction?** The orchestrator already drops the app schema on install failure (`CompensateSchemaOnFailure` → `DropAppSchema`), which is a stronger rollback than a transaction for everything the app owns. The gap is rows migrations seed into shared `__mj`, and the manifest already declares `migrations.teardownDirectory` for exactly those rows (used today on remove). Running teardown on the install-failure path would give the same "a failed install leaves nothing behind" guarantee without requiring an impossible transaction. *(Not verified: whether teardown scripts run cleanly against a partially-seeded state.)*
2. **Should a 1205 during a migration produce a better error?** The runner could check the SQL it just executed for `CREATE TYPE … AS TABLE` and, on a deadlock, say so — turning an opaque 1205 into a named cause. Low false-positive risk since it only fires on an actual deadlock.
3. **Side question — who should own the transaction mode?** If it is expressed at all, is it a property of the *installing host* (CLI config) or of the *app* (manifest), given the app is what knows whether it ships table types? A small PR making the existing `TransactionMode` option reachable on `RunAppMigrations` — with **no default change** — is being prepared separately; it deliberately does not answer this question.
Note the upgrade flow's own failure message already documents forward-only, resume-from-last-successful-migration semantics, which are only reachable under per-file transactions — so parts of the engine already assume a model the runtime does not currently provide.
## Impact today
One shipped app currently carries the pattern (a table type plus rollup triggers that declare it). A survey of 13 open apps found it is the only one so far — but TVPs are the standard way to pass a set to a procedure, so any app doing rollups, batch posting, or bulk operations is likely to reach for one.
## The early-return guard in the affected app does not work (verified)
The one shipped app carrying this pattern currently guards its rollup triggers with an early return, and its in-code comment states that returning first means "the type is never referenced during the migration at all":
```sql
IF NOT EXISTS (SELECT 1 FROM inserted) AND NOT EXISTS (SELECT 1 FROM deleted) RETURN;
DECLARE @ids .OrderHeaderIDList;
```
**That comment is wrong.** Reproduced faithfully — same trigger shape, same guard, a 0-row `UPDATE … WHERE col IS NULL` backfill against an empty table, all in one transaction:
| Case | Result |
|---|---|
| 0 rows affected — the guard *does* return | **1205** |
| rows present — the guard falls through | **1205** |
Variable instantiation happens when the trigger's module is executed, before the guard's control flow can skip it, so the Sch-S on the type is taken either way. An earlier commit message in that app reached the same conclusion ("compilation precedes execution"); the guard was nonetheless kept and documented as a fix.
Two consequences worth noting: the app is **not** protected today, and any app author who reaches for the same intuitive workaround will believe they are safe when they are not — which argues for the platform surfacing a clear error rather than leaving app authors to discover this by deadlock.
Contributor guide
Research direction
Start with packages/OpenApp/Engine/src/install/migration-runner.ts and packages/MJCLI/src/config.ts, then reproduce the SQL Server 1205 case described in the issue. Trace RunAppMigrations, TransactionMode, and the existing CompensateSchemaOnFailure path. Done requires an agreed fix direction, coverage for clean installs involving table types, and a clear failure or rollback outcome.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- sql, typescript
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100