Repository navigation
Commit 1fd5664
fix(metadata-protocol): a refused sys_packages delete fails the uninstall before anything is removed (#21438)
Fixes #21276
Clause-②: no
When `DELETE /api/v1/packages/:id` is refused — by the store, or by the
registry because another package extends an object this one owns — it
now answers that refusal and changes nothing. The running process keeps
serving the package, a disabled package stays disabled across a restart,
and its metadata, tables and grants stay in place. Three changes, in
three files:
- **`metadata-protocol`.** `deletePackage` asks the registry's uninstall
refusal first, then deletes the stored row as its first durable step. A
refusal from either throws before anything else is removed.
- **`objectql`.** The registry's refusal check becomes one public method
that changes nothing, so it can be asked before that first durable step.
- **Door (`runtime`).** The dispatcher asks `deletePackage` before it
withdraws the package from the running registry or clears its disable
record.
**Cross-lane surfaces, named before the change list.**
- `packages/metadata-protocol/src/protocol.ts` (`domain:engine`):
`deletePackage`. Beside it, the two #21243 helpers it reuses
(`packagePersistFailureError` and `packagePersistFailureMessage`) take
one more verb value, `'delete'`, with one new sentence.
- `packages/objectql/src/registry.ts` (`domain:engine`, added by claim
revision 2 on #21276): the refusal pass of `unregisterObjectsByPackage`,
extracted into `assertPackageUninstallable`.
- `packages/runtime/src/domains/packages.ts` (`domain:cli`, claim
revision 1): the `DELETE` arm only, plus the header note of its
organization-scope refusal.
- Pins and fixtures:
- `packages/objectql/src/registry-assert-package-uninstallable.test.ts`
(new);
-
`packages/metadata-protocol/src/protocol.package-delete-refusal.test.ts`
(new);
-
`packages/runtime/src/package-uninstall-store-refusal.integration.test.ts`
(new);
- the registry double of
`packages/runtime/src/domains/packages-uninstall-envelope.test.ts`.
- `service-package` is unchanged: its `delete` already states a refusal
on both channels (`PackageDeleteResult`). No `packages/spec` edit.
`assertPackageUninstallable` is not added to the spec's engine contract
interface; `deletePackage` reaches it through a capability probe.
## The door, measured live
`pnpm dev:crm` was booted twice over one SQLite file (a private
`OS_HOME`, no `--fresh`) and driven as the seeded platform admin. A
trigger refuses `DELETE` on `sys_packages` for the forced package only:
`CREATE TRIGGER refuse_forced_delete BEFORE DELETE ON sys_packages WHEN
OLD.id = 'com.example.forced' BEGIN SELECT RAISE(ABORT, '…'); END`. The
control package has no trigger.
| step | `main` `22c2d6f4d`, forced | protocol half only (`6d823ae7a`),
forced | with the door half (`608838a73`), forced | control, every tree
|
|:--|:--|:--|:--|:--|
| `DELETE /api/v1/packages/:id` | `200`, `success: true` | `500
DATABASE_ERROR` | **`500 DATABASE_ERROR`** | `200` |
| `GET`, same process | `404` | `404` | **`200`** | `404` |
| `GET` after a restart | `200`: comes back | `200` | `200` | `404` |
| the package disabled first, then the forced delete; `GET` after a
restart | not measured | `enabled: true`, `status: installed` |
**`enabled: false`, `status: disabled`** | — |
The disable leg has a control leg on `6d823ae7a`: the package was
disabled and the server restarted with no `DELETE`, and it came back
`disabled`. This round's change (the registry question) is pinned at the
door by the real-composition pins below rather than re-measured live,
because a live extender needs an installed app whose `objectExtensions`
target a writable package's object.
The store-refusal `500` message reads: "Package 'com.example.forced' was
not uninstalled: the package registry could not delete its stored
record, and a package whose record is kept comes back on the next
restart, so its metadata, data and grants were left in place. The reason
is in the server log." The driver's `SQLITE_CONSTRAINT_TRIGGER` line
stays in the server log and on `cause`.
## The steps of `deletePackage`, in their new order
| # | step | durable? | can it refuse? | on refusal |
|:--|:--|:--|:--|:--|
| 1 | tenant-scope check | no | yes, `400 TENANT_SCOPE_REQUIRED` |
thrown; nothing changed |
| 2 | `sys_metadata` read | no | yes, `503` or a classified refusal |
thrown; nothing changed |
| 3 | **the registry's uninstall refusal**, asked through
`SchemaRegistry.assertPackageUninstallable` | no | yes: another package
extends an object this one owns (ADR-0029) | **thrown as is; nothing
changed** |
| 4 | **`sys_packages` delete** (on `main`: after step 5) | yes, the
first | yes: returned `{ success: false }`, or thrown | **thrown through
`packagePersistFailureError`; nothing changed** (on `main`:
`console.warn`, then success) |
| 5 | per-item `sys_metadata` deletes and table teardown | yes | yes:
authorization, lock, store fault | collected in `failed[]`; the door
answers `400 PACKAGE_DELETE_PARTIAL` |
| 6 | registry withdrawal (`SchemaRegistry.uninstallPackage`, which also
releases the namespace) | process state | its refusal was asked at step
3; anything else it throws is a safety-net case | `console.warn`; the
package leaves at the next restart |
| 7 | uninstall cleanups | yes | yes | reported in `cleanups[]` |
The store error shape is #21243's, unchanged. A declared 4xx leaves as
the producer answered it. Anything else is a `500` that quotes nothing,
with the original on `cause` and a catalogued code carried
(`DATABASE_ERROR` from a live SQL driver; `INTERNAL_ERROR` derived for a
returned `{ success: false }`). No code is minted. There is no undo
machinery, because nothing durable precedes step 4.
Step 3 reaches the registry the way step 6 already did, through
`this.engine.registry`. A registry without `assertPackageUninstallable`
(an engine double, or a host on a registry without it) is not asked.
`deletePackage` then behaves as it did before the method existed: the
refusal surfaces at step 6.
**Steps that can still refuse after the store delete** (triage's ruling
asks for them to be moved ahead or reported here):
- **Step 5.** An authorization or lock refusal could in principle be
decided first, but only by a dry run of `deleteMetaItem`'s own checks,
which is another `protocol.ts` region. A store fault cannot be decided
ahead. This is unchanged from `main`, where the store delete ran after
these steps whatever they answered. The door answers `400
PACKAGE_DELETE_PARTIAL` and lists what was left.
- **Step 7.** A cleanup is a data-plane delete, so deciding it means
doing it. This is unchanged: each refusal is reported in `cleanups[]`.
## `SchemaRegistry.assertPackageUninstallable`
This is the refusal pass of `unregisterObjectsByPackage` (#7970:
"refuses before it mutates"), moved into one public method. It has the
same predicate, the same iteration order and the same message, and it
mutates nothing. `unregisterObjectsByPackage` calls it (`if (!force)
this.assertPackageUninstallable(packageId)`), so there is still one copy
of the check. `force` stays the caller's decision: forcing means not
asking.
**Why the name:** this class already names its non-mutating
throw-or-pass check `assertSingleOwnerPerObject`, and the repo's other
`assert…` verbs (`assertProtocolCompat`, `assertLockAllowsDelete`) also
check and throw without changing state. "Uninstallable" names the
question at the level both of its callers ask it.
## Door half
`DELETE /api/v1/packages/:id` used to run
`registry.uninstallPackage(id)` and `setPackageDisabled(environmentId,
id, false)` before it called `protocol.deletePackage`. It now runs in
this order:
1. the unchanged refusals: `requireManageMetadata`, the ADR-0070
read-only gate, and the organization-scope mirror;
2. an existence **read**: `registry.getPackage(id)`;
3. `deletePackage`. Its throw — the store's refusal or the registry's
ADR-0029 refusal — is answered through the existing `errorFromThrown(e,
500)`, and nothing has been touched at that point. For the extender
refusal this is the envelope the door gave before this PR, when its own
up-front `uninstallPackage` raised it: `500 INTERNAL_ERROR`, nothing
changed;
4. only then the withdrawal from the running registry (skipped when
`deletePackage` already withdrew it), and the clear of the disable
record for a package this request found.
The late withdrawal keeps its `try`/`catch` as a safety net, for a
registry without the new method or a throw nothing asked ahead of time.
Since the stored rows are gone by then, that case is reported as
`registryRemoved: false`, not as a failure. The extender refusal no
longer arrives there.
The refusal envelopes are unchanged: `403`, `422
WRITABLE_PACKAGE_REQUIRED`, `400 TENANT_SCOPE_REQUIRED`, the thrown
`deletePackage` failure, `400 PACKAGE_DELETE_PARTIAL` and the `404`. The
`404` asks whether the package existed rather than whether it was
withdrawn. A host with no persisted half keeps its old behaviour: the
withdrawal is the uninstall, and its refusal is the request's refusal.
## Tests (code at `01dae724d`; the head `fc6b6515a` differs from it only
in the changeset)
**Registry pin** (`registry-assert-package-uninstallable.test.ts`, 3
tests):
- the method refuses with the uninstall's exact sentence, and every
contributor of every object is unchanged;
- it answers normally for a package nothing extends, and for an unknown
id, and changes nothing;
- `unregisterObjectsByPackage` refuses with the same sentence by calling
the method (a spy sees the call), and `force: true` does not ask.
**Protocol pins** (`protocol.package-delete-refusal.test.ts`, 7 tests):
- a store refusal on either channel gives `500` with nothing removed;
- a declared `409` passes through unchanged;
- **new:** the registry's ADR-0029 refusal is thrown as is, with not
even the store delete run, and the stored row survives a restart;
- after a store refusal, a restart still has the package, its metadata
and its grants;
- CONTROL: an ordinary uninstall runs in the order store delete,
metadata, registry, cleanup.
**Door pins** (`package-uninstall-store-refusal.integration.test.ts`, 4
tests). They run on a real composition booted twice over one SQLite
file: real `ObjectQL` and `SqlDriver`; the real `PackageServicePlugin`
hydration; the real protocol and `HttpDispatcher`; and `AppPlugin`'s
disable seed.
- a store refusal answers `500 DATABASE_ERROR`, and the same process
still serves the package, disabled, with its view row;
- after a restart the package is still installed, still disabled, and
still has its view row;
- CONTROL: an ordinary delete answers `200`, then `404` in the same
process and `404` after a restart. Its rows are gone and its disable
record is cleared;
- **flipped:** with an ADR-0029 extender, the delete answers `500
INTERNAL_ERROR`. The stored row, the view row, the registry entry
(disabled) and the disable record are all intact, in the same process
and after a restart.
**Fixture change.** In `packages-uninstall-envelope.test.ts`, the
registry double reads one `registered` flag from both `getPackage` and
`uninstallPackage`, because the door now reads existence with
`getPackage`. The case still asserts the same `404`.
Full suites at `fc6b6515a`:
| package | command | files | tests |
|:--|:--|:--|:--|
| `objectql` | `vitest run --project local`, 2 shards | 183 + 183 passed
| 3602 + 3780 passed |
| `metadata-protocol` | `vitest run`, 2 shards | 103 passed; 100 passed,
3 skipped | 1372 passed; 1669 passed, 19 skipped |
| `runtime` | `vitest run --project local`, 2 shards | 155 + 154 passed
| 2048 passed, 4 skipped; 2314 passed, 15 skipped |
| `rest` | `vitest run --project local`, 2 shards | 128 + 128 passed |
2581 passed, 87 skipped; 2267 passed, 235 skipped |
The skipped files are the opt-in live-database files. `typecheck` passes
for `objectql`, `metadata-protocol` and `runtime` (each with its
`check:test-typecheck` layer where it has one).
## Ablations (every mutation through `scripts/ablation-replace.mjs`;
each anchor hit once; each restore proven with the blob equal to `HEAD`
and `git diff HEAD` empty; each prediction written before the run)
| ablation | tree | predicted | observed |
|:--|:--|:--|:--|
| A1, protocol: the returned-result check removed | `6d823ae7a` |
returned-channel pins | 2 of 6 red |
| A2, protocol: the `catch` that logs a warning and goes on restored |
`6d823ae7a` | thrown-channel pins | 3 of 6 red |
| A3, protocol: the store delete moved back after the metadata deletes |
`6d823ae7a` | every refusal and restart pin | 6 of 6 red |
| D1, door: withdraw before `deletePackage` | `608838a73` | same-process
pin | 2 of 4 red |
| D2, door: clear the disable record before `deletePackage` |
`608838a73` | restart-disabled pin | 1 of 4 red |
| **P1**, protocol: `deletePackage` no longer asks
`assertPackageUninstallable` | `01dae724d` | protocol and door extender
pins | protocol 1 of 7 red; door 1 of 4 red |
| **P2**, registry: `unregisterObjectsByPackage` no longer calls the
method | `01dae724d` | my registry pin, plus at least 4 #7970 cases | 6
of 104 red: my pin and 5 refusal cases in `registry.test.ts` |
| **P2b**, registry: the call replaced by an inline copy of the
predicate, so behaviour is identical | `01dae724d` | the spy assertion
only | 1 of 104 red: my pin only |
The protocol and registry pins import their subjects from `src`. The
door pins read `metadata-protocol` through `dist`, so P1's door leg
rebuilt it:
- `scripts/ablation-dist-preflight.mjs` found the marker in 2 built
files;
- the door pins went 1 of 4 red;
- after the restore and a rebuild, the marker was absent from all 24
built files, and the door pins were 4 of 4 green again.
Void attempts, declared:
- The first P1 run used a comment as its marker. The build strips
comments, so the dist preflight reported the marker absent, and that
door reading is void. It was re-run with a marker that survives the
build: a comparison against a string constant.
- The first A1 attempt, and the first two attempts of A3's call leg,
were refused by the tool before any test ran, because each replacement
contained its own anchor. They were re-run.
P2b is what makes "`unregisterObjectsByPackage` not calling the method"
distinguishable: an inline copy behaves exactly like the call, so only
the spy can tell them apart.
## Gates (head `fc6b6515a`)
`node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
--commands` derived 69 commands. Compared with the previous round's 65,
this round's new paths add `check:engine-split-ratio` and
`check:rest-log-declared`, each with its self-test. All 69 ran on that
head and exited 0. Reconciling with `--ran` gives "69 derived, 69 run, 0
NOT-MEASURED, 0 UNRUN", with every exit code recorded.
`origin/main` was merged at `7d29e5c5d` and at `2db45821b`. The second
merge brought `535d1d25a`, which edits other `protocol.ts` regions. The
3 commits on `main` after that merge touch none of this PR's files.
ESLint was narrowed to the seven touched TypeScript files, which is a
measured narrowing:
- ESLint's own `--print-config` resolves every one of them;
- `--format json` counts 7 files, with 0 errors and 0 warnings;
- no resolved config carries `parserOptions.project` or
`projectService`, so type-aware linting is off and no untouched file's
verdict can move.
The repo-wide `pnpm lint` is CI's.
## Changeset
This PR's own file covers `@objectstack/metadata-protocol`,
`@objectstack/runtime` and `@objectstack/objectql`, all at `patch`. All
three are released packages in one `fixed` version group.
The bump for the new `SchemaRegistry` method follows the repo's rules:
- AGENTS.md Post-Task Checklist step 3: a bug fix in a released package
takes a `patch`, and this method is the mechanism of this fix;
- `check-changeset-no-major.mjs`'s level axis demands `minor` only for a
`Clause-②: yes` (a new key on a published payload), and this PR declares
`no`.
## Docs
I grepped `content/docs/**` (outside `releases/` and `references/`) and
`skills/**` for uninstall and package-delete semantics:
`api/metadata-api.mdx`, `kernel/contracts/metadata-service.mdx`,
`permissions/permission-sets.mdx`, `protocol/kernel/plugin-spec.mdx` and
`deployment/publish-and-preview.mdx`. Each describes a successful
uninstall, and that path is unchanged, so none of their sentences is now
false. No doc edits.
## Acceptance notes
- The ADR-0029 extender refusal is decided before the store delete
again, through `SchemaRegistry.assertPackageUninstallable`. At the door
it answers `500` with nothing changed, the envelope it had before this
PR.
- Observed and not filed: an ordinary uninstall of a package with no
`sys_metadata` rows carries `persisted.success: false` inside the door's
`200`, because `deletePackage` computes `success` as `failed.length ===
0 && deleted.length > 0`. The door does not read that field.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 94a8761 commit 1fd5664
8 files changed
Lines changed: 944 additions & 83 deletions
File tree
- .changeset
- packages
- metadata-protocol/src
- objectql/src
- runtime/src
- domains
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
Lines changed: 305 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
0 commit comments