feat(locking): run-scoped object locks that actually refuse a write by rubenvdlinde · Pull Request #3444 · ConductionNL/openregister

rubenvdlinde · 2026-09-05T12:10:54Z

What this does

Makes an object lock mean something, and lets a flow run hold one.

Two new nodes, openregister.lock-object and openregister.unlock-object, take and release a lock in the name of the run rather than the user the run happens to execute as. A second run that wants the same object parks and retries on the heartbeat. A person who writes to a locked object is refused with a message naming the run. The engine releases every lock a run holds on any terminal outcome, a sweep collects what a killed worker left behind, and the existing TTL stays as the backstop.

Spec first, in openspec/changes/run-scoped-object-locking/.

Why

No lock had ever blocked a write. ObjectEntity::lock() writes the holder under the key user. SaveObject::findAndValidateExistingObject() read it under userId, so $lockOwner was invariably null and the guard's !== null test short-circuited every time. Every lock taken through the lock endpoint since it shipped was decorative on the service write path.

The unit test agreed with the bug. Three test files hand-wrote setLocked(['userId' => ...]), a payload shape lock() has never produced, and passed. That agreement is why the defect survived. Every one of those fixtures now goes through the production writer, so writer and reader cannot diverge again without this suite failing.

Two runs under one identity could not conflict. Ownership was compared on the user alone, so the second run took the extend branch, pushed the first's expiry out, and was handed the object. On an instance where flows run as one service account that is every pair of runs.

The process tag was dropped on the floor. LockHandler::lock() accepted $process and never forwarded it; MagicMapper hardcoded 'MagicMapper lock'.

The lock record

kind: 'run'|'user' and runUuid, added beside the existing keys. _locked is a JSON column, so this is additive with no migration and no back-fill: a record written before this change has no kind and reads as the user lock it is. user keeps its meaning in both cases (the person, or the run's runAs), so getLockedBy(), callerMayUnlock(), the 423 body and two frontend stores all keep working. What changes is that user stops being the authority.

One predicate owns ownership, ObjectEntity::isLockedBySomeoneElse(), and every guard calls it. A run lock refuses everyone, the run's own runAs user included; a malformed run lock refuses everybody rather than failing open. Reasoning in design.md D-1.

Three release layers

A FlowRunTerminalEvent listener. The dispatch predicate in FlowRunMapper::update() is isTerminal(), not a whitelist, so all four terminal statuses and every reaper path are covered.
A sweep in FlowRunWorker, reading a small run-lock registry rather than scanning magic tables (an instance-wide scan of 2,728 of those costs ~3.4s just to plan).
The lock TTL, unchanged.

Note on the brief: there is no cancelled run status. FlowRun::TERMINAL is completed, stopped, failed, dead_letter; a cancellation and a queue-TTL expiry both land on failed. The tests cover the four that exist, read from the constant so a fifth arrives as a failure.

What the rig caught that mocks did not

Two defects, both found only on a real Postgres instance:

The orphan query composed its two predicates 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 would have logged a warning and released nothing, every tick, forever. Now two plain queries.
The release read passed includeDeleted: false. A soft-deleted object can still hold a live lock and is exactly the one nobody is watching, so its lock would have been stranded.

_rbac and _multitenancy were already off on that read, because both release layers run sessionless; a test now pins all three flags. This is the same shape as the prune-retired miscount in #3440.

Caller sweep

Every lockObject() caller in the fleet was checked for the shape that breaks when locks become real, lock under one identity and write under another. None has it, so nothing starts refusing. Two callers start being protected: buildiq's ApplicationVersionsController::update() and AbstractToolHandler::saveVersionManifest() both take locks explicitly to stop two concurrent MCP agents losing each other's writes, and two agents under one account have never been able to conflict. Both already handle the 409 they will now sometimes get. dossiq's DrcController::lock() locks then writes as the same user, which is admitted. Nothing in lib/Repair/, lib/Command/, lib/Migration/ or any seed takes an object lock. Full table in design.md D-6.

Verified locally

CI is bottlenecked, so this was verified locally and judged by exit code.

Check	Result
`phpunit` Unit Tests	19177 tests, 0 failures; exit 1 on `No code coverage driver available`, identical to the untouched base (19116 tests, same 1 warning / 2 deprecations / 43 skipped)
`phpstan`	exit 0
`psalm`	exit 0, no errors
`phpcs`	exit 0 on all changed files
`phpmd`	exit 0 on all 13 changed files (`lib/Service` whole-dir is OOM-killed on this host). Three findings fixed rather than baselined, one documented
hydra gates	full run (not diff-scoped), 80 of 80 applicable gates. Two failures found and fixed: gate-16 wanted 8 `@spec` tags, and gate-67 refused the contract change until the shipped copy moved with it, which is ConductionNL/.github#689
rig	own compose project on a free port, 18/18 checks passed against real Postgres, torn down with `down -v`

The rig proved, end to end: the payload shape, a second run under the same user refused without rewriting the first's lock, a human write refused naming the run, the registry row, release layer 1, and the sweep freeing a lock whose run never existed.

Not run locally: the Integration, Database and Service suites, which need a booted Nextcloud checkout, and the E2E suite, which only runs on the development push.

Quality debt paid on the way

LockHandler had grown past the complexity threshold, so RunLockRegistry and AdvisoryLockStore are split out: an advisory pre-creation lock and a run-held object lock are different things. A psalm baseline entry the extraction made stale was removed rather than left. testLockNewLock asserted only a return value behind a comment claiming the payload "may not actually be set"; it is not true, and that test now checks the payload.

Companion PR

ConductionNL/.github#689 mirrors ObjectServiceInterface into the shipped contract copy. Merge that one first, or gate-67 stays red here for the right reason.

🤖 Generated with Claude Code

… 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.

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.

… 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.

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.

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.

Also picks up hydra-gates v1.15.0, which is the version that ran these gates locally.

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.

github-actions · 2026-09-05T12:15:33Z

Quality Report — ConductionNL/openregister @ `b5982cc`

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 12:15 UTC

Download the full PDF report from the workflow artifacts.

github-actions · 2026-09-05T12:25:14Z

Quality Report — ConductionNL/openregister @ `092d809`

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 12:25 UTC

Download the full PDF report from the workflow artifacts.

…d-object-locking

github-actions · 2026-09-05T13:03:46Z

Quality Report — ConductionNL/openregister @ `000c0ec`

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 13:03 UTC

Download the full PDF report from the workflow artifacts.

…s nobody (#3451) Nextcloud reads an app's migration directory only when appinfo/info.xml's <version> is greater than the installed_version it recorded. Equal versions mean `occ upgrade` answers "No upgrade required.", exits 0, and opens no migration file at all. Nothing is logged, nothing fails, and the feature that needed the table is absent with no error anywhere. Measured on a throwaway NC 34.0.3 rig running openregister 2.0.15-unstable.20260905134511 from its release tarball: a migration added with <version> left alone did not run and was not recorded; changing nothing but <version> and re-running `occ upgrade` ran it. The code was byte-identical across the two runs. This is not hypothetical. development carried four migrations added since <version> last moved on 2026-09-03, one of them the run-lock table #3444 depends on, and an instance updated to that code got none of them. scripts/check-migration-version-bump.php fails when a branch adds a file under lib/Migration/ without moving <version> past its value at the merge base. It runs in Merge Hygiene on every push and PR, in composer check:strict, and as a warning from .githooks/pre-commit. Run against the real history it reds on exactly those four files. It exits 2, not 0, when it cannot resolve the base ref: a check that cannot see the base has no verdict, and a silent pass is the failure this removes. occ migrations:status cannot be used for this and is documented as such rather than worked around. It is Nextcloud's command and three of its five counting fields are wrong: Pending Migrations reads None for this app always, because core filters the list on \$migration->name() and SimpleMigrationStep::name() returns '' for all 204 of ours; New Migrations and Executed Unavailable both call array_keys() on a list and so diff version strings against 0..n. The only honest pair is Executed against Available, which is what read 204 of 205 on the rig while the line below it said nothing was pending. Refs #3444

…in the editor (#3454) * fix(locking): a parked run keeps its locks, and the lock nodes exist in the editor Three defects in the run-scoped locking that #3444 shipped, all three found by walking a live instance and none by the suite. A RUN LOST EVERY LOCK THE MOMENT IT PARKED. The dispatch predicate really is `isTerminal()` on the persisted row, and the row really did say `completed` — transiently, in the middle of a pass that went on to store `suspended`. `FlowEngine::fireOnStream()` computes `enabledAfter` by asking `FlowStreamWalk::workRemains()` for the transitions enabled on the marking it has just advanced, but that method answers from the walk's in-memory stream picture, which `commitFiring()` only re-reads AFTER the commit. So it compared the NEW enabled transitions against the OLD places and answered "no work remains" at every ordinary mid-flow firing. `applyDerivedStatus()` then took its "nothing enabled, nothing parked, nothing terminal" arm and wrote `completed`, `FlowRunMapper::update()` announced terminality, and the lock listener released the run's locks — every firing, of every run, not just ones that lock. The park path had the mirror bug: `workRemains()` was evaluated before `park()` marked the stream parked, so a parking run was derived `queued` with no wake time until `finalize()` corrected it. `workRemains()` now takes what the caller's commit is about to change: `produced`, the places the firing takes, and `settling`, a stream whose place stops counting because it is parking. Over-counting is the safe direction here — it yields `queued`, which the next pass corrects — and under-counting is what produced a false terminal. A RUN-KIND LOCK REFUSED THE RUN THAT HELD IT. `SaveObject` asked the guard "which user", never "which run", so a flow that locked a case was turned away by its own lock at its next write. The run identity now reaches the guard through the ambient `FlowRunContext`, for the same reason attribution does: the write is routinely several calls deep inside code that has never heard of flows. Absent, it reads as a person, which is the fail-closed answer. The D-6 sweep is re-done over the GUARD's callers, not just `lockObject()`'s: `SaveObject`, `RevertHandler` and `ObjectsController::update` now pass the caller's run; the three post-save auto-unlock tests in the controller deliberately do not, and say so — they decide a RELEASE, and a run's lock must outlive every write the run makes. `LockObjectNode` and `UnlockObjectNode` already passed theirs. The same predicate had the opposite hole: a user lock did not refuse a run executing as its holder, so a run passing over a person's locked object took the extend branch, rewrote the payload as its own run lock, and destroyed the person's lock when it ended. A run and the person it runs as are different holders in both directions. BOTH NODES WERE INVISIBLE IN THE EDITOR. `core/img/actions/lock.svg` and `unlock.svg` do not exist in NC 33 or 34, `imagePath()` throws for an image the server does not ship, and `palette()` caught that with everything else — so the catalogue held 25 nodes rather than 27 and neither node could be added to a flow at all. The icons are now app-owned, and the silent skip is loud: an icon is resolved on its own, an unresolvable one is an ERROR naming the node, and the node is served with the app icon instead of being deleted from the catalogue. A node that survives with the wrong picture beats a node that does not exist. Tests, each proven red first, driving the real engine rather than a fake — the existing coverage mocked `FlowRunMapper::update()` and restated `workRemains()` in a fake, so both agreed with the bug: - a suspended run announces nothing and keeps its locks, and each of `FlowRun::TERMINAL` releases them (iterated from the constant) - the holding run writes to its own locked object; another run and a person are refused - a person's lock survives a run passing over the object, payload byte-identical - every registered node's icon resolves, as a sweep over all 27 rather than a check of these two, plus the palette behaviour that stops the next one vanishing * fix(locking): trace the two icon methods to the palette requirement gate-16 counts a changed method with no `@spec` as an untraceable change, and these two are exactly the methods the palette requirement is about. * test(locking): say why each terminal leg rebuilds the harness

rubenvdlinde added 8 commits September 5, 2026 13:06

docs(openspec): specify run-scoped object locking

6827900

fix(locking): an engine break needs no session, and name the mapper's…

c7ac6c8

… 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.

chore(locking): tag the node and registry methods gate-16 asked for

8f70d8c

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

986db95

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.

Merge remote-tracking branch 'origin/development' into feat/run-scope…

70d156b

…d-object-locking

rubenvdlinde merged commit d5b57af into development Sep 5, 2026
36 checks passed

rubenvdlinde mentioned this pull request Sep 5, 2026

fix(migrations): a migration that ships without a version bump reaches nobody #3451

Merged

rubenvdlinde mentioned this pull request Sep 5, 2026

fix(locking): a parked run keeps its locks, and the lock nodes exist in the editor #3454

Merged

rubenvdlinde mentioned this pull request Sep 5, 2026

test(locking): e2e coverage for run-scoped object locking, each leg proven red first #3460

Merged

Navigation Menu

Uh oh!

feat(locking): run-scoped object locks that actually refuse a write - #3444

feat(locking): run-scoped object locks that actually refuse a write#3444
rubenvdlinde merged 9 commits into
developmentfrom
feat/run-scoped-object-locking

rubenvdlinde commented Sep 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Why

The lock record

Three release layers

What the rig caught that mocks did not

Caller sweep

Verified locally

Quality debt paid on the way

Companion PR

Uh oh!

github-actions Bot commented Sep 5, 2026

Quality Report — ConductionNL/openregister @ b5982cc

Uh oh!

github-actions Bot commented Sep 5, 2026

Quality Report — ConductionNL/openregister @ 092d809

Uh oh!

Uh oh!

github-actions Bot commented Sep 5, 2026

Quality Report — ConductionNL/openregister @ 000c0ec

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

rubenvdlinde commented Sep 5, 2026 •

edited

Loading

Quality Report — ConductionNL/openregister @ `b5982cc`

Quality Report — ConductionNL/openregister @ `092d809`

Quality Report — ConductionNL/openregister @ `000c0ec`