fix(schemas): prune-retired missed a duplicate row and miscounted what it destroys - #3440
Merged
rubenvdlinde merged 1 commit intoSep 5, 2026
Conversation
…t it destroys
Two defects in a DESTRUCTIVE command, both found by running it on a live
instance rather than by reading it.
IT HANDLED ONE ROW PER (application, slug). `findByApplicationAndSlug()` caps at
one, which is correct where the pair is unique. It is not unique in practice,
and this command exists because it is not: the import unions schema ids and
never removes one, so a descriptor edit leaves the old row behind and a later
import can add a second under the same pair. Measured: opencatalogi owned
`document` at ids 39 AND 40. The command took 40, printed
`Pruned=1, skipped=0`, and left 39 answering every lookup. An operator would
reasonably read that as done.
`SchemaMapper::findAllByApplicationAndSlug()` returns every row, oldest first,
and the command loops over all of them.
ITS SAFETY GUARD FAILED OPEN. The dry run said `0 object(s)`; the apply reported
`objects removed=2`. The guard that refuses to prune a schema still holding data
was counting a different set from the one the delete destroys, in two
independent ways:
1. The count ran through `MagicSearchHandler`, which applies RBAC and
multi-tenancy by default. `occ` has no session, so the CLI reads as
Anonymous, and rows nobody anonymous may see counted as ZERO.
2. It iterated only the registers that REFERENCE the schema. A schema
referenced by none counted zero BY CONSTRUCTION — and that is exactly the
state a half-pruned schema is left in.
Either path let an operator read "0 objects, safe to delete" and then destroy
rows.
The guard now asks the deletion service what a cascade WOULD remove, over the
same table set the delete enumerates, with `_rbac` and `_multitenancy` off and
`_includeDeleted` on — a soft-deleted row is still a row the table drop
destroys, and counting only live rows tells the operator nothing about the
recovery they are giving up. A table that outlives its register counts as at
least one, because the delete drops it without reading it.
And when the two numbers still disagree, the command says so instead of leaving
the operator to infer it: the guard was wrong about what it was protecting, and
that is worth a line of output.
The command's own `MagicMapper` dependency is gone with its count.
19,116 tests green (2 new, both for the cases the existing suite structurally
could not see: two rows under one slug, and a guard/delete count mismatch).
phpcs, psalm and phpstan clean, and all 75 applicable hydra gates pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ❌ | ||||
| test-l10n-parity | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ❌ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 543/543 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-05 09:49 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Sep 5, 2026
…3444) * docs(openspec): specify run-scoped object locking * feat(locking): a lock records whether a run or a person holds it, and refuses writes The write guard never fired: SaveObject read the holder from `userId` and ObjectEntity::lock() has always written `user`, so $lockOwner was invariably null and the !== null test short-circuited every time. The unit test agreed with the bug, hand-writing a payload shape lock() has never produced. Ownership was also keyed on the user alone, so two flow runs under one runAs could not conflict: the second took the extend branch and was handed the object. Adds `kind` and `runUuid` to the lock payload, additive inside the existing _locked JSON column so no migration and no back-fill are needed and a record with no `kind` reads as the user lock it is. One predicate, ObjectEntity::isLockedBySomeoneElse(), now owns the comparison and every guard calls it. * feat(flow): lock and unlock nodes, and three layers that always release Two nodes. `openregister.lock-object` takes a run-scoped lock and, when another run holds the object, parks the run with a non-null resumeAt and retries on the heartbeat until its wait budget expires, then fails naming the holder. The budget is stamped once in the node's own resume slot, so a retry does not restart it. `openregister.unlock-object` releases early. Release does not depend on a node running. A FlowRunTerminalEvent listener releases every lock a run holds on all four terminal statuses, a sweep in FlowRunWorker collects locks whose run is terminal or gone, and the lock TTL remains the backstop. Splits RunLockRegistry and AdvisoryLockStore out of LockHandler: an advisory pre-creation lock and a run-held object lock are different things, and the handler had grown past the complexity threshold. * fix(locking): an engine break needs no session, and name the mapper's arguments The terminal-event listener and the cron sweep both run as nobody, so requiring a session user on the break path would mean the release layers built for crashed runs could never fire. * fix(locking): the sweep read nothing, two ways Found on the rig, not by a mock. The orphan query composed `run_uuid NOT IN (sub-select)` and an expiry comparison as one orX() over a createFunction(). PostgreSQL rejects that with "argument of OR must be type boolean, not type record", and both the registry's catch and the worker's swallowed it: the sweep logged a warning and released nothing, every tick. Now two plain queries merged in PHP. The release read also passed includeDeleted: false. A soft-deleted object can still hold a live lock and is exactly the one nobody watches, so its lock would have been stranded. Every scoping filter on that read is now off and a test pins each one: _rbac and _multitenancy were already off because both release layers run sessionless, which is the same shape as the prune-retired miscount in #3440. * fix(locking): declare the throw the contention path actually takes ObjectEntity::lock() throws the global Exception, not LockedException, so lock()'s broad catch is the arm that fires on contention. Declaring only the narrow one on the extracted helper made PHPStan read the live catch as dead code. Also drops a psalm baseline entry the extraction made stale. * chore(locking): tag the node and registry methods gate-16 asked for Also picks up hydra-gates v1.15.0, which is the version that ran these gates locally. * chore(flow): say why the worker carries ten collaborators phpmd's ExcessiveParameterList fired on the sweep's dependency, the tenth. Splitting the worker would buy a second cron job and a second ordering to reason about.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in a destructive command
Both found by running it on a live instance, not by reading it.
1. It handled one row per
(application, slug)findByApplicationAndSlug()caps at one, which is correct where the pair is unique. It is not unique in practice, and this command exists because it is not: the import unions schema ids and never removes one, so a descriptor edit leaves the old row behind and a later import can add a second under the same pair.Measured: opencatalogi owned
documentat ids 39 and 40. The command took 40, printedPruned=1, skipped=0, and left 39 answering every lookup. An operator would reasonably read that as done.SchemaMapper::findAllByApplicationAndSlug()returns every row, oldest first, and the command loops over all of them.2. Its safety guard failed open
The dry run said
0 object(s). The apply reportedobjects removed=2.The guard that refuses to prune a schema still holding data was counting a different set from the one the delete destroys, in two independent ways:
MagicSearchHandler, which applies RBAC and multi-tenancy by default.occhas no session, so the CLI reads as Anonymous, and rows nobody anonymous may see counted as zeroEither path let an operator read "0 objects, safe to delete" and then destroy rows.
The guard now asks the deletion service what a cascade would remove, over the same table set the delete enumerates, with
_rbacand_multitenancyoff and_includeDeletedon — a soft-deleted row is still a row the table drop destroys, and counting only live rows tells the operator nothing about the recovery they are giving up. A table that outlives its register counts as at least one, because the delete drops it without reading it.And when the two numbers still disagree, the command says so rather than leaving the operator to infer it. The guard was wrong about what it was protecting; that is worth a line of output.
The command's own
MagicMapperdependency goes with its count.Testing
🤖 Generated with Claude Code