Repository navigation
Commit 7cb33cf
fix(metadata-protocol)!: an uninstall that would drop a table another stored row still serves is refused 409 before anything is removed (#23504)
Fixes #23473
Clause-②: no
## What changes
`deletePackage` (`packages/metadata-protocol/src/protocol.ts`) now
refuses the WHOLE uninstall `409 RESOURCE_CONFLICT` before its first
durable step (step 4, the `sys_packages` delete) when, without
`keepData: true`, an active object row of the package shares its object
with another stored active row: another package's row, or the
package-less row. Nothing is removed: the `sys_packages` row, the
registry entry, every `sys_metadata` row, the tables and their records
all stay. The refusal names each shared object and the rows that keep
it, and prescribes the ways out.
Before this, the per-item delete met #23415's per-object `409` only
after step 4, collected it in `failed[]` and the door answered `400
PACKAGE_DELETE_PARTIAL` over a package that was no longer installed
while its row of the shared object stayed stored and served.
Shape B, per the lane decision on the card (`6168058499`). Not shape C
(uninstall and quietly keep the shared table).
## Governing text
`deletePackage`'s own docblock at `1baf976cf9`, quoted:
> 3. the registry's uninstall refusal (another package extends an object
this one owns, ADR-0029), asked through
`SchemaRegistry.assertPackageUninstallable` — pure; thrown as is;
> [#21276] The steps, in order. Nothing durable happens before step 4,
so a refusal at any of steps 1–4 leaves everything as it was:
A shared object is the same kind of stake as an extended one, so it
joins step 3's up-front refusal. The docblock's step 3 now names both
refusals; the numbering, and with it "nothing durable happens before
step 4", is unchanged.
## How the condition is read (premise, measured)
- **Same reads as the per-item delete, no new store surface.** For each
active object row whose table the uninstall would drop
(`shouldDropStorage`, the same guard: object type, active, not `sys_`),
the pre-flight calls `packagelessAddress(type, name, undefined,
'active')`, the private address read `deleteMetaItem` itself takes for
that row (its `rowsAtAddress`), with the type folded as `deleteMetaItem`
folds it (`canonicalMetaType`, then `PLURAL_TO_SINGULAR`). It refuses
exactly when that delete would: the package holds a row there and
another package's row or the package-less row stays. A failed read is
the uninstall's step-2 read failure (`metadataReadFailureError`), never
"not shared".
- **`keepData: true` really skips every drop.** The only table-drop call
sites are `deleteMetaItem`'s two paths into `dropObjectStorage`, both
gated by `shouldDropStorage` on the request's `dropStorage`, and
`deletePackage` sends `dropStorage` only when `keepData !== true`. So
with `keepData` sharing decides nothing and the pre-flight reads
nothing.
- **No measured flow needs shape C.** `deletePackage` has two callers:
the `DELETE /api/v1/packages/:id` door, and `rollBackUnmanagedImport`,
whose import refuses a name another row holds before it writes anything
(`unmanagedImportCollisions`). The install-local uninstall door does not
call it.
## Error code
The pre-flight answers `RESOURCE_CONFLICT` / `409`, which is also the
code the per-item refusal (`dropStorageRemainingRowsRefusal`) answers
for the same rows. Step 3's own refusal, an object extended by another
package, used to be a code-less `Error` that the door answered `500
INTERNAL_ERROR`. Since PR #23522 (#23505, `989aa483fd`) it answers `409
RESOURCE_CONFLICT` with the way out, pinned in
`package-uninstall-store-refusal.integration.test.ts` on `main`. So both
up-front uninstall refusals now share one code. No code is minted.
## The refusal, as measured (two shared objects)
```text
Uninstall package 'com.probe.p' with keepData (?keepData=true; keepData: true in process), which keeps all its object tables, or first delete, without dropStorage, its row or the other rows of each object below (DELETE /api/v1/meta/object/NAME?package=ID). Nothing was removed: an uninstall drops an object's table only with the object's last row, and 2 objects of this package are also held by other rows: 'probe_ticket' (its row bound to package 'com.probe.q' remains); 'probe_case' (its row bound to package 'com.probe.q' remains).
```
The second way out answers the card body's concern with shape B:
deleting the package's own row of each shared object first (no
`dropStorage`) keeps that table for the other rows, and the uninstall
then still drops the tables the package alone held. That path is pinned
too.
## Merge round after the queue ejection (run 38682506920)
The PR left the merge queue: `Test Core (6/6)` failed on queue commit
`5e97584201` (`main` `8d05456cbe` plus this PR). `main` was merged in as
a merge commit, `192d2051da` (no conflict; `protocol.ts` auto-merged).
Reproduced on that commit, the three cases of the pin where P and Q both
hold the object failed in setup, before any assertion:
```text
Error: Cannot layer object 'probe_ticket': the overlay is bound to package 'com.probe.q', but the object is owned by package 'com.probe.p'. An object has exactly one registry entry, ...
```
Cause: #23510 (`222293f60a`, ADR-0029 D9.9) landed on `main` after this
PR's previous merge base `f9b421716d` (`merge-base --is-ancestor` exit
1, control leg `f9b421716d~60` exit 0; it is in `8d05456cbe`, exit 0).
The save door now refuses a save into a second package of an object a
runtime-authored row owns (`OBJECT_OVERLAY_PACKAGE_MISMATCH` / `422`),
so the pin's fixture could no longer save Q's row. `protocol.ts` was not
the cause and is unchanged by this round; the step-3 CONTROL in
`package-uninstall-store-refusal.integration.test.ts` (uninstalling the
extending package) passes on the merged tree.
**Does the shared state still arise?** Measured on the merged tree, real
`ObjectQL` over better-sqlite3, the real protocol and
`HttpDispatcher.handlePackages` (one-time readings, probe not kept):
| producer | result | stored active rows |
|:--|:--|:--|
| save door: P, then Q | Q refused `422 OBJECT_OVERLAY_PACKAGE_MISMATCH`
| P |
| save door: P, then a save naming no package | addressed to P's row | P
|
| save door: a save naming no package first, then P, then Q | all three
succeed (the registry owner is the `sys_metadata` sentinel, which D9.9
leaves out) | P, Q, package-less |
| Q saves a DRAFT before P's row exists; P publishes; Q promotes
(`publishMetaItem`, `package` Q) | promotion succeeds; the registry
keeps P | P, Q |
| the same, promoted by `POST
/api/v1/packages/com.probe.q/publish-drafts` at the door handler |
`200`, `outcome: published`, `failed: []`; the registry keeps P | P, Q |
| `duplicatePackage` P to Q (`targetNamespace` given) | the copy is
renamed `qq_probe_ticket` | P |
| boot (`loadMetaFromDb`) over a stored Q row beside P's | counts the
row in `errors`, leaves it stored (code reading) | unchanged |
So the state still arises: through today's doors (the last two save-door
rows), and as rows stored before D9.9, which stay at rest. On each
door-reached state the uninstall of P without `keepData` answered `409
RESOURCE_CONFLICT` and removed nothing. The pre-flight guards a live
path.
**The fixture fix.** `twoPackageObject` now saves P's row through the
door, as before, and seeds Q's row straight into the store as a row
stored before D9.9 (the save's shape, checksum by `hashSpec`), the way
#23510 re-seeded its own two sibling fixtures. It does not use the
draft-promotion path: that path is a D9.9 gap, and a fixture built on it
would break again when the gap closes. Every case assertion is
unchanged; the package-less case and the CONTROL are unchanged.
## Second merge round (queue run 38703520759)
The PR left the queue again: `Test Core (6/6)` failed in
`packages/runtime` on queue commit `75e7900e67` (`main` `23e689dc7d`
plus `addcca0cf5`). `main` was merged in as a merge commit, `ce7a65df2c`
(no conflict; `protocol.ts` auto-merged, and this PR's delta to it is
byte-identical before and after the merge). Reproduced there, four of
the pin's five cases (every case that seeds a stored row) failed in
setup:
```text
Error: Unknown field 'organization_id' on object 'sys_metadata'
at seedStoredRow (src/package-uninstall-shared-object.integration.test.ts:156)
```
Cause: #23469 (`dd29bbe60d`, ADR-0131 D6/D7) dropped the organization
column from the `sys_metadata` family, and with it `organization_id:
null` from the sibling pin's seed
(`meta-delete-drop-storage-other-rows.test.ts`). The fix, `4f639e56b0`:
this pin's seed follows `main`'s row shape and no longer writes the
column. Nothing else moves.
**The pre-flight still asks the per-item delete's address read.** On the
merged tree both call the one `packagelessAddress`: the pre-flight with
`(type, name, undefined, 'active')`, `deleteMetaItem` with
`(singularTypeForRepo, name, member, targetState)`. #23469 changed only
that function's body: its `where` was `{ type, name, state,
organization_id: null, ...member }` read by `engine.find`; it is now `{
type, name, state, ...member }` read through `findEnvironmentRows`, the
seam that drops a legacy organization's rows. So both callers moved
together. `deletePackage`'s step-2 read goes through the same seam, and
its per-item loop no longer has a legacy-organization branch, so the
pre-flight's input rows and the rows the per-item delete walks are the
same set.
## Tests
The pin,
`packages/runtime/src/package-uninstall-shared-object.integration.test.ts`,
at the uninstall door itself: real `ObjectQL` over a real better-sqlite3
`SqlDriver` (real tables), the real `PackageServicePlugin`
(`sys_packages`), the real protocol and the real
`HttpDispatcher.handlePackages`. Five cases:
1. P and Q hold an object, uninstall P without `keepData`: `409
RESOURCE_CONFLICT` (status and code asserted), message names the object,
Q and `keepData`; the `sys_packages` row, `GET /packages/P` 200, both
rows, the table, the record and the registry entry all intact.
2. The same with the package-less row as the other holder.
3. `?keepData=true`: 200, `failed: []`, P gone, Q's row, the table and
the record stay, the registry serves Q's definition.
4. P's own row deleted first (no `dropStorage`): the uninstall answers
200, `failed: []`, keeps the shared table and drops the table P alone
held.
5. CONTROL: an object one package holds — 200, row gone, table dropped.
Readings at `4f639e56b0` (`--maxWorkers=2`, each under the shared verify
lock, verdict from its `VERDICT command-exit` line):
- red before, at the merge commit `ce7a65df2c`: the pin 4 failed / 1
passed (the setup error above; the CONTROL passed).
- the pin: 5/5 passed.
- `@objectstack/runtime` uninstall sweep (every
`src/package-uninstall-*.integration.test.ts`, every
`src/domains/packages-*.test.ts`): 30 files, 542 tests passed.
- `@objectstack/runtime` local project, whole (four vitest shards): 387
files passed; 5843 tests passed, 19 skipped. `typecheck` (`tsc --noEmit`
plus `check:test-typecheck`) exit 0.
- `@objectstack/metadata-protocol` whole suite (three vitest shards):
236 files passed, 3 skipped; 20550 tests passed, 19 skipped. `typecheck`
exit 0.
Earlier round, at `addcca0cf5`: the same set green, and
`@objectstack/cli` both tiers green (unit 290 files; integration 131
files; CI's `Test Core (2/6)` slice, `OS_TEST_SHARD=1/3`, 141 files).
## Ablation (one-shot, not kept)
`@objectstack/metadata-protocol` reaches the pin through its `dist/`, so
each leg rebuilt it and proved the marker's presence or absence there.
At `4f639e56b0`, through `scripts/ablation-replace.mjs` (WRAP mode;
anchor 1 to 0, blob `498517e78a` to `4bee71e85b`):
- mutate: the pre-flight's `if (others.length === 0 && !at.unbound)
continue;` widened with a marker condition, so no row ever refuses;
`ablation-dist-preflight` found the marker in `dist/index.js` and
`dist/index.cjs`. Predicted 2 red / 3 green; observed 2 failed / 3
passed. Cases 1 and 2 answered the card's defect: `400
PACKAGE_DELETE_PARTIAL`, the shared object in `failed[]` with #23415's
per-item `RESOURCE_CONFLICT` text, which is the same rows the pre-flight
refuses.
- restore: blob equal to `HEAD`
(`498517e78a682bcb5a82432490f55bd39c91661c`), `git diff HEAD` empty,
whole tree clean; rebuilt; `--absent` found the marker in none of 24
built files; the pin 5/5 green.
## Gates
`node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` at `4f639e56b0` derives 65 families (the
same 65 as on `addcca0cf5`). All 65 ran locally; `--ran` reconciles 65
derived, 65 run, 0 NOT-MEASURED. 63 exit 0. `check:dual-build-cjs-loads`
first exited 3 (PREREQUISITE NOT MET, packages without `dist/`) and
exited 0 once they were built. `check-empty-changeset.mjs --base
origin/main` exits 1 on the 23415 note, the deliberate correction the
maintainer confirmed (`6184266311`); CI's `Check Changeset` is red for
the same reason, as on every head of this PR. Every other family CI runs
is read from this PR's check-runs, not re-run here: on `4f639e56b0` all
five required contexts are green (`Build Core`, `TypeScript Type Check`,
`Lint & Repo Gates`, `Governed Surface Queue Guard`, `Test Core` with
all six shards).
`Test Core (2/6)` (CLI slice 1/3) failed once on `addcca0cf5` (run
`38686154595`) and did not reproduce: the slice and the whole CLI suite
passed locally, and the shard is green on `4f639e56b0`.
## Acceptance notes
- **#23415's unreleased changeset is corrected in one sentence**, the
uninstall sentence of `.changeset/23415-drop-storage-other-rows.md`; the
maintainer confirmed it on this PR (`6184266311`). `Check Changeset`
stays red for it by design.
- **The per-item `409` stays as a safety net.** With the pre-flight, it
is unreachable on the uninstall path except for a row written
concurrently between the pre-flight and the per-item delete; then it is
still caught and reported in `failed[]` as before.
- **Cost.** Without `keepData`, one `sys_metadata` read per active
object row of the package (not `sys_`), the same read the per-item
delete then repeats. With `keepData`, none.
- **File surface.** `protocol.ts`: the `deletePackage` docblock's step 3
and the pre-flight between step 3 and step 4 only. Not the per-item
loop, `deleteMetaItem` or `classifyObjectContribution`.
- **The package-less row as the "other row".** A delete naming no
package addresses the one package's row of an item while one is stored
(`packagelessAddress`), so the package-less row cannot be deleted first
through the metadata door while the package's row exists. The refusal's
other way out (delete the package's own row first) covers that case;
pinned as case 4's mechanism.
- **D9.9 gaps met while measuring, not fixed here: filed as #23630**,
the D9.9 family close-out. Its two members:
- a draft of a second package, saved before the owner's row and then
promoted, stores an active row the registry cannot hold (measured above,
at `publish-drafts` too);
- the package-less-owner case that #23510 already noted.
- **A comment `main` left stale, not this PR's.** The `[#21276]` comment
above step 3 in `deletePackage` still says the door answers that refusal
"`500`, nothing changed". PR #23522 made it `409`. This PR does not edit
that comment, because it lies outside this PR's change.
---
_Generated by [Claude
Code](https://claude.ai/code/session_0121ooBYRmS8ziFXm9ai5YHm)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent c5dd7fc commit 7cb33cf
4 files changed
Lines changed: 420 additions & 1 deletion
File tree
- .changeset
- packages
- metadata-protocol/src
- runtime/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
14 | 14 | | |
15 | 15 | | |
16 | 16 | | |
17 | | - | |
| 17 | + | |
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
| |||
| 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26092 | 26092 | | |
26093 | 26093 | | |
26094 | 26094 | | |
| 26095 | + | |
| 26096 | + | |
| 26097 | + | |
26095 | 26098 | | |
26096 | 26099 | | |
26097 | 26100 | | |
| |||
26181 | 26184 | | |
26182 | 26185 | | |
26183 | 26186 | | |
| 26187 | + | |
| 26188 | + | |
| 26189 | + | |
| 26190 | + | |
| 26191 | + | |
| 26192 | + | |
| 26193 | + | |
| 26194 | + | |
| 26195 | + | |
| 26196 | + | |
| 26197 | + | |
| 26198 | + | |
| 26199 | + | |
| 26200 | + | |
| 26201 | + | |
| 26202 | + | |
| 26203 | + | |
| 26204 | + | |
| 26205 | + | |
| 26206 | + | |
| 26207 | + | |
| 26208 | + | |
| 26209 | + | |
| 26210 | + | |
| 26211 | + | |
| 26212 | + | |
| 26213 | + | |
| 26214 | + | |
| 26215 | + | |
| 26216 | + | |
| 26217 | + | |
| 26218 | + | |
| 26219 | + | |
| 26220 | + | |
| 26221 | + | |
| 26222 | + | |
| 26223 | + | |
| 26224 | + | |
| 26225 | + | |
| 26226 | + | |
| 26227 | + | |
| 26228 | + | |
| 26229 | + | |
| 26230 | + | |
| 26231 | + | |
| 26232 | + | |
| 26233 | + | |
| 26234 | + | |
| 26235 | + | |
| 26236 | + | |
| 26237 | + | |
| 26238 | + | |
| 26239 | + | |
| 26240 | + | |
| 26241 | + | |
| 26242 | + | |
| 26243 | + | |
| 26244 | + | |
| 26245 | + | |
| 26246 | + | |
| 26247 | + | |
| 26248 | + | |
| 26249 | + | |
| 26250 | + | |
| 26251 | + | |
| 26252 | + | |
| 26253 | + | |
26184 | 26254 | | |
26185 | 26255 | | |
26186 | 26256 | | |
| |||
0 commit comments