Repository navigation
Commit 5cdb0db
fix(security,metadata-protocol): batchData's delete arm stops reporting a deletion it did not perform (#19451)
Fixes #19433
Clause-②: no
## What was wrong
`POST /api/v1/data/{object}/batch` with `operation: "delete"` reported a
deletion it did not
perform. This is the **third and last** of the three by-id delete doors
to read the engine's
answer: the single-record `DELETE` (PR #19411) and `deleteMany` (PR
#19432) already do, and
this branch is the one that fell between them with no card.
Each row pushed `success: true` as a **literal**, so every engine result
that was not the
driver contract's `false` was reported as a deletion. The `false` arm —
"no row matched" —
has reported honestly since #4435 and #5088. The other ending had not: a
row that **matched**
and was deliberately **not removed** was counted in `succeeded` and
shipped an envelope
byte-identical to a batch in which every id really went.
`sys_permission_set` is the shipped shape of it. Deleting a
package-declared set is an
ADR-0005 RESET — plugin-security's write-through tombstones the overlay
and the record
re-projects to the declared body instead of vanishing — so the row
matched, the write ran,
and the record is still there. On a security-configuration write the
envelope told an
operator a permission set was gone while it was still being enforced.
## Why this is a defect today — rested on the CONTRACT, not on a shipped
handler
`IDataEngine.delete` declares `Promise[boolean | number]`
(`packages/spec/src/contracts/data-engine.ts`; square brackets here for
the generic, the
spelling this family's cards use) — the driver boolean for a by-id
write, a COUNT of rows
removed otherwise. `isDeleteResultShape` in
`packages/objectql/src/verb-hook-result-shape.ts`
is a pair of `typeof` tests admitting `boolean` and `number`, and the
ADR-0112 hook gate in
`packages/objectql/src/engine.ts` uses it. So an `afterDelete` handler —
or a non-ObjectQL
`IDataEngine` — may legally answer `0` **today**, and a numeric zero is
the one value that
positively means "the row is still there".
This PR does **not** rest on "a shipped handler already returns the 0".
That claim was made
on #19412 and the seat could not verify it; the sibling fix stood
because the contract
argument is independent, and this one stands the same way.
## Located by symbol, because this family's line numbers have drifted
four times
`grep -n "throw recordNotFoundError"
packages/metadata-protocol/src/protocol.ts` on this
branch's base (`23f1de078`):
| door | line on this base | state before this PR |
|:---|:---|:---|
| single-record `deleteData` | `:11438` | reads the engine result
(#19306 / PR #19411) |
| `runBatchDataLoop` `case 'delete'` | `:12568` | **the literal — this
PR** |
| `runDeleteManyLoop` | `:13120` | reads the engine result (#19412 / PR
#19432) |
The file's own comment beside the `batchData` site calls it "the OTHER
by-id bulk delete, ten
lines from it".
## Measured on the unfixed base, before any edit
A fake engine per answer, through `batchData` with one record:
| `engine.delete` answers | batch door, before | after this PR |
|:---|:---|:---|
| `true` | `success: true`, `succeeded: 1` | unchanged |
| `1` | `success: true`, `succeeded: 1` | unchanged |
| **`0`** | **`success: true`, `succeeded: 1`, `failed: 0`** |
**`success: false`, `succeeded: 0`, `failed: 1`, no `errors`** |
| `false` | `success: false` + `errors[0].code RECORD_NOT_FOUND`,
`failed: 1` | unchanged |
| `undefined` | `success: true` | unchanged |
Mixed batch `['a', 'kept', 'b']` where `kept` answers `0`, before: all
three `success: true`,
`succeeded: 3`. Atomic with the same engine, before: **committed**,
`succeeded: 3`, `kept`
silently still present.
## The envelope measurement this card owes — what transfers, and what
does not
`batchData`'s envelope is its own, so each of these was read off this
file rather than
inherited from `deleteMany`. All three shapes turn out to **agree**, and
the reason is
mechanical: the two surfaces share the machinery.
| question | `deleteManyData` | `batchData` | why |
|:---|:---|:---|:---|
| same `succeeded`/`failed` partition (#7539)? | yes | **yes** | both
response builders call the shared `reconcileStoppedBatch`;
`buildBatchDataResponse` then sets `success: failed === 0`, `total:
records.length` |
| an `atomic` arm, aborting on `failed > 0`? | yes | **yes** |
`runAtomicBatchData` delegates to the shared `runAtomicBatch`, whose
abort condition is `outcome.failed > 0` |
| does the `continueOnError` stop apply? | no | **no** | the stop lives
in the loop's `catch`, and a surviving row throws nothing |
Two things about `batchData` that have no counterpart on `deleteMany`,
and are therefore
pinned separately:
1. **`runBatchDataLoop` is shared by `create` / `update` / `upsert` /
`delete`**, and each
`case` owns its own `succeeded++`. Only the `delete` case is touched;
the sibling arms keep
their counters exactly as they were.
2. **`returnRecords: false`** is a `batchData`-only projection
(`buildDeleteManyResponse` has
no such flag). It drops `data` and keeps `success` / `index` / `errors`,
so the one key
this card moves is the key that survives it — pinned.
## The fix
The per-row push in `case 'delete'` reads the engine result instead of
being a literal:
```ts
const removed = deleted !== 0;
results.push({ id: record.id, success: removed, index });
if (removed) succeeded++;
else failed++;
```
**Not `false` for this case.** That arm is spoken for by the 404 above,
about a record this
caller can still `GET`; answering it here would trade one wrong answer
for a louder one. Only
a POSITIVE zero is read as "not removed", and an off-contract
`undefined` from a third-party
driver keeps its #4435 reading.
**And the row gets no `errors[]` entry.** A surviving record is an
OUTCOME, not a fault; the
single-record door answers the same case with a bare `success: false` on
a 200, and this
envelope's two per-row codes (`ROLLED_BACK`, `NOT_ATTEMPTED`) both
describe a row that never
ran. Minting a code for this ending is an `ERROR_CODE_LEDGER` widening
in `packages/spec` and
belongs to the spec lane. That was ruled on #19412 and the ruling is
carried here, not
re-opened.
HTTP status is untouched — the route hands the protocol result straight
to `res.json`, so
this stays a 200 whose body reports per row.
## On the wire, per row
| case | before | after |
|:---|:---|:---|
| row removed | `success: true`, in `succeeded` | unchanged |
| matched, not removed (packaged-set reset) | `success: true`, in
`succeeded` | `success: false`, in `failed`, no `errors[]` |
| unknown id | `success: false`, `errors[0].code RECORD_NOT_FOUND` |
unchanged |
| batch stopped / rolled back | `NOT_ATTEMPTED` / `ROLLED_BACK` |
unchanged |
Because the counters partition `results`, a surviving row also makes the
request-level
`success` false, and an `atomic` batch holding one now rolls back rather
than committing
under a response that called every row deleted. A non-atomic batch is
not stopped by it.
## Evidence
All readings at `7b9c6376c` (final commit, `origin/main` merged in),
worktree clean, base
`23f1de078`.
**The pin, and that it can fail.**
`protocol.bulk-record-not-found.test.ts` gains a
`[#19433]` block of five cases beside the existing `[#5088]` one: the
NEGATIVE case (engine
answers `0`, so `success: false`, `succeeded: 0`, `failed: 1`, **no**
`errors` key, and the
record is still in the store), a CONTROL leg (a positive count still
reports a deletion —
without it the pin can pass vacuously), a mixed batch pinning both
directions plus the
partition and the un-stopped run, the `atomic` arm (the surviving row
aborts and the earlier
delete is really undone), and `returnRecords: false`. Run against the
UNFIXED base first:
**4 failed / 18 passed**. After the fix: **22 passed**.
**Reverse verification**, both legs from the committed state, mutation
proven on disk and
restored by blob hash (`scripts/ablation-replace.mjs`, mutation and
command in one process):
| mutation | anchor | blob | result |
|:---|:---|:---|:---|
| the four fixed lines back to `success: true` + `succeeded++` | x1 to
x0 | `aaa92be21a62` to `d9c2974fa715` | **4 failed / 18 passed** |
| `const removed = deleted !== 0` to `const removed = false` | x1 to x0
| `aaa92be21a62` to `39ef16f7b8ee` | **7 failed / 15 passed** |
Both restored: blob equals `HEAD`'s (`aaa92be21a62`) and `git diff HEAD`
is empty. The first
leg also proves the suite resolves `src/protocol.ts` and not `dist/` —
`dist/` held the FIXED
bytes at that moment (`success: removed` occurs twice in
`dist/index.js`) and the run still
went red.
**Unit + types.** `pnpm --filter @objectstack/metadata-protocol test` —
**183 files
(3 skipped) / 2621 passed (19 skipped)**, exit 0. `typecheck` exit 0,
and
`tsc --noEmit --listFiles` names
`protocol.bulk-record-not-found.test.ts` once (negative
control: a name not in the program is listed 0 times), so the pin is
inside the program the
script advertises. No public export moved, so no consumer package owes a
test here.
**Gates.** `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack`
derived **61 families** from this diff; all 61 ran, all exit 0,
reconciled with `--ran`
carrying each exit code — "61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN".
Re-derived after
`git fetch origin main` and again after merging it: identical list, so
the new
`check-dts-references` family that landed on `main` meanwhile is not one
this diff owes.
Three families (`check:dual-build-cjs-loads`,
`check:lean-entry-closure`,
`check:type-check-debt`) exited 3 `PREREQUISITE NOT MET` on the first
sweep, were given the
repo-wide build CI gives them, and then exited 0; the exit-3 runs are
recorded as not
measured rather than as failures. Repo-wide `pnpm lint` ran
**unnarrowed**
(`eslint . --no-inline-config`): exit 0, at this same head.
**Changeset — measured, not assumed.** `@objectstack/metadata-protocol`
is `private: false`
with `files: ["dist", "README.md", "CHANGELOG.md"]`, and the edited
symbol ships: `batchData`
occurs 25 times in the built `dist`, and the moved expression `success:
removed` occurs in
both `dist/index.js` and `dist/index.cjs`, against a negative control
symbol that occurs 0
times. So a changeset is owed and `skip-changeset` does not apply.
**`patch`**: a released
package's bug fix whose published accept set does not move — no schema
key added or removed,
no closed-set member, no new published export, no registry entry, no
request shape touched.
What changes is the VALUE of an existing key, pulled back to what
`BatchOperationResultSchema.success` already declares ("Whether this
record was processed
successfully"), on a schema whose `errors` was already optional. Hence
`Clause-②: no`,
measured against this diff rather than copied from #19412's verdict.
## Acceptance notes
Noticed while measuring, deliberately **not** carried here:
- **The "causal row" of a stopped or rolled-back batch can now name a
row that did not
fail.** `reconcileStoppedBatch` and `buildRolledBackBatchResponse` both
locate the cause
with `findIndex(r => !r.success)`, which since PR #19432 can land on a
SURVIVING row — a
non-success that carries no error. Measured on this branch: a non-atomic
`['survivor', 'missing', 'other']` delete batch answers row 2 with
`NOT_ATTEMPTED: "record 0 failed — unknown error; the batch stopped
there."` when it was
row 1 that threw; an atomic batch answers a rolled-back row with
`ROLLED_BACK: "record 1 failed — unknown error"` for a row that survived
rather than
failed. This is already live on `origin/main` through `deleteMany` since
PR #19432 landed,
and it reproduces identically there — so it is a shared-builder defect,
not one this PR
introduces, and fixing it means editing builders all three bulk faces
read. Reported for
filing. No pin in this PR asserts either message.
- **`makeStoreEngine`'s fake `delete` answers `{ deleted: 1 }`** — an
object, which is
neither arm of the declared `boolean | number`. It happens to be `!== 0`
so the existing
cases keep their reading, but it is a test double speaking off-contract.
Noted, not filed:
no consumer.
- **`os data delete`'s unconditional print** was already recorded on PR
#19411 and is not
re-reported here.
- #19412 is not addressed here; it landed in PR #19432 and this branch
does not touch the
`runDeleteManyLoop` region.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE
---
_Generated by [Claude
Code](https://claude.ai/code/session_01NcPSwnmJHczmTu6FG7NMjE)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 8e368dc commit 5cdb0db
3 files changed
Lines changed: 265 additions & 2 deletions
File tree
- .changeset
- packages/metadata-protocol/src
| 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 | + | |
Lines changed: 163 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
424 | 424 | | |
425 | 425 | | |
426 | 426 | | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
| 534 | + | |
| 535 | + | |
| 536 | + | |
| 537 | + | |
| 538 | + | |
| 539 | + | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
| 583 | + | |
| 584 | + | |
| 585 | + | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
427 | 590 | | |
428 | 591 | | |
429 | 592 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12566 | 12566 | | |
12567 | 12567 | | |
12568 | 12568 | | |
12569 | | - | |
12570 | | - | |
| 12569 | + | |
| 12570 | + | |
| 12571 | + | |
| 12572 | + | |
| 12573 | + | |
| 12574 | + | |
| 12575 | + | |
| 12576 | + | |
| 12577 | + | |
| 12578 | + | |
| 12579 | + | |
| 12580 | + | |
| 12581 | + | |
| 12582 | + | |
| 12583 | + | |
| 12584 | + | |
| 12585 | + | |
| 12586 | + | |
| 12587 | + | |
| 12588 | + | |
| 12589 | + | |
| 12590 | + | |
| 12591 | + | |
| 12592 | + | |
| 12593 | + | |
| 12594 | + | |
| 12595 | + | |
| 12596 | + | |
| 12597 | + | |
| 12598 | + | |
| 12599 | + | |
| 12600 | + | |
| 12601 | + | |
| 12602 | + | |
| 12603 | + | |
| 12604 | + | |
| 12605 | + | |
| 12606 | + | |
| 12607 | + | |
| 12608 | + | |
| 12609 | + | |
| 12610 | + | |
| 12611 | + | |
| 12612 | + | |
| 12613 | + | |
| 12614 | + | |
| 12615 | + | |
| 12616 | + | |
| 12617 | + | |
| 12618 | + | |
| 12619 | + | |
| 12620 | + | |
| 12621 | + | |
| 12622 | + | |
| 12623 | + | |
| 12624 | + | |
| 12625 | + | |
| 12626 | + | |
| 12627 | + | |
| 12628 | + | |
12571 | 12629 | | |
12572 | 12630 | | |
12573 | 12631 | | |
| |||
0 commit comments