test(locking): e2e coverage for run-scoped object locking, each leg proven red first - #3460
Merged
Merged
Conversation
…roven red first
The capability shipped validated only by a manual rig walkthrough. The
walkthrough never parked a run, so nobody noticed that its first version
released every lock a run held the moment the run suspended — the one moment a
lock is for. Three earlier tests asserted that behaviour as correct.
WHERE THE COVERAGE LANDS, AND WHY IT IS NOT SPLIT ACROSS REPOS
All of it is here. dossiq has the larger suite and owns the case a demo locks,
but its shipped case flow uses no lock step — so a "case-flow journey" there
would either restate the engine assertions this file makes, or require adding a
lock step to a shipped flow, which is a product change wearing a test's name.
openregister owns the lock, the nodes, the write guard and the palette; that is
where the behaviour is, so that is where it is asserted.
Within this repo it is split by what the assertion needs:
- tests/e2e/api-direct/flow-object-locking.spec.ts — the engine and API legs.
Needs `occ` for the worker, so it cannot join the CI floor, and sits beside
flow-user-task / flow-schedule / delegation-parking, which are the same shape.
- tests/e2e/ci/flow-lock-nodes.spec.ts — the palette. Hermetic, non-mutating,
no occ, no skips, so `ci/*.spec.ts` admits it and CI actually runs it.
THE ASSERTIONS ARE MADE THROUGH THINGS THAT CAN ONLY ANSWER ONE WAY
`@self.locked` is read for `kind` and `runUuid`, not for non-null: an earlier
round of tests asserted `locked !== null` and passed against a user lock a run
had silently stolen. A refusal is asserted as 423 with `lockedByRun` equal to
the run uuid, not as "some 4xx" — a 400 from validation and a 500 from an
untyped service exception are both "not 200" and neither is a lock. A run that
should bounce is asserted to reach `failed` with the HOLDER's uuid in its error.
EACH LEG WAS PROVEN RED, by neutralising the thing it tests and restoring it:
- parked run keeps its lock — dropped `produced:` from `FlowEngine`'s
`enabledAfter`, restoring the false mid-pass terminal. Red: "the parked run
holds NO lock".
- a second run bounces — made the spent wait budget proceed without the lock
instead of failing. Red: run `completed`, expected `failed`.
- a person is refused, the runAs included — keyed run-lock ownership on the
user again. Red: the runAs user's PUT answered 200 with the run's lock intact.
- a person's lock survives a run — removed the branch that makes a run and the
person it runs as different holders. Red: run `completed`, lock destroyed.
- both nodes in the palette — restored `palette()` resolving the icon inline,
with the icons pointed back at `core/img/actions/lock.svg`. Catalogue went
27 → 25 and both tests failed, the editor one naming the palette.
That last proof is why the palette file is TWO tests. As one, the run stopped at
the catalogue assertion and the editor half never executed: a test whose second
half cannot be shown to fire has not been proven, only its first half has.
RERUNNABLE ON ONE RIG, WHICH TOOK TWO CORRECTIONS THE RIG FOUND
Fixtures are namespaced and torn down: runs stopped, then every lock BROKEN as
the administrator, then `occ openregister:objects:purge --force --apply`, then
the register and schema. Releasing the locks first is not tidiness — a left
lock means the object cannot be written, so the next run meets a state the
first never saw, and fails somewhere far from the fixture that caused it.
- a flow terminated by `openregister.end` commits as `stopped`, not
`completed`, so the flows end on a step with no outgoing edge instead;
- `waitSeconds: 1` is racy: the deadline is persisted with `format('c')`, which
truncates to whole seconds, so a budget stamped at .900 is already spent
200 ms later and the run fails on its FIRST attempt instead of parking.
The worker is driven with `occ background-job:execute <id> --force-execute`, and
each wait sleeps to the run's own `resumeAt` first — a pass fired before a run
is due does nothing, and a loop of those fabricates a convincing "wedged
forever" that belongs to the test, not the run.
The nine covered scenarios in the change spec now carry `@e2e` references, and
the two whose exclusions claimed "engine-internal, covered by the rig walk" no
longer need them.
Verified locally on a throwaway rig (own compose project, port 8749, NC 32,
torn down with `down -v`): both suites run TWICE with identical results —
4 passed / 0 failed and 2 passed / 0 failed each time, all 6 per-test outcomes
matching — and the instance holds no leftover flow, register, schema or user
afterwards. prettier --check and eslint both exit 0.
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 19:23 UTC
Download the full PDF report from the workflow artifacts.
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
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.
Why
Run-scoped object locking shipped validated only by a manual rig walkthrough. The walkthrough never parked a run, so nobody noticed that its first version released every lock a run held the moment the run suspended — the one moment a lock is for. Three earlier tests asserted that behaviour as correct, because they mocked the seam that was wrong.
This adds real Playwright coverage for the five legs, and each one was proven red against the neutralised code before being kept.
Where each leg lands, and why it is not split across repos
All of it is in openregister. dossiq has the larger suite and owns the case a demo locks, but its shipped case flow uses no lock step — so a "case-flow journey" there would either restate the engine assertions this PR makes, or require adding a lock step to a shipped flow, which is a product change wearing a test's name. openregister owns the lock, the nodes, the write guard and the palette.
Within this repo the split is by what the assertion needs:
tests/e2e/api-direct/flow-object-locking.spec.tsoccto drive the worker, so it cannot join the CI floor. Sits besideflow-user-task/flow-schedule/delegation-parking, which are the same shape.tests/e2e/ci/flow-lock-nodes.spec.tsocc, no skips — soci/*.spec.tsadmits it and CI actually runs it.No behaviour is asserted in both.
The assertions are made through things that can only answer one way
@self.lockedis read forkindandrunUuid, not for non-null. An earlier round of tests assertedlocked !== nulland passed against a user lock a run had silently stolen.lockedByRunequal to the run uuid, not as "some 4xx". A 400 from validation and a 500 from an untyped service exception are both "not 200", and neither is a lock.failedwith the holder's uuid in its error. A run that fails for another reason would otherwise wear this test's green.Red-proof, per leg
Each was neutralised on a live rig, the spec run, and the source restored.
produced:fromFlowEngine'senabledAfter, restoring the false mid-pass terminalcompleted, expectedfailedrunAsincludedcompleted, lock destroyedpalette()resolving the icon inline, icons back oncore/img/actions/lock.svgThat last proof is why the palette file is two tests rather than one. As a single test the run stopped at the catalogue assertion and the editor half never executed — a test whose second half cannot be shown to fire has not been proven, only its first half has.
Rerunnable on one rig, which took two corrections the rig found
Teardown follows dossiq#1824/#1829: runs stopped, then every lock broken as the administrator, then
occ openregister:objects:purge --force --apply, then the register and schema. Releasing the locks first is not tidiness — a left lock means the object cannot be written, so the next run meets a state the first never saw and fails far from the fixture that caused it.Two assumptions the rig refused:
openregister.endcommits asstopped, notcompleted(stoppedoutrankscompletedin the commit path's severity table), so the fixtures end on a step with no outgoing edge instead;waitSeconds: 1is racy. The deadline is persisted withformat('c'), which truncates to whole seconds, so a budget stamped at.900is already spent 200 ms later and the run fails on its first attempt instead of parking. Identical fixtures gavefailedandsuspendedon consecutive runs. The budgets are 3 s and 8 s, with the reason written beside them.The worker is driven with
occ background-job:execute <id> --force-execute, nevercron.php, and each wait sleeps to the run's ownresumeAtfirst: a pass fired before a run is due does nothing, and a loop of those fabricates a convincing "wedged forever" that belongs to the test rather than the run.Traceability
Nine scenarios in
openspec/changes/run-scoped-object-locking/now carry@e2ereferences to these files, and the two whose exclusions read "engine-internal, covered by … the rig walk" no longer need one.Verified locally
Throwaway rig, own compose project on port 8749 (never
:8080), NC 32 + PostgreSQL 16, torn down withdown -v.4 passed / 0 failedand2 passed / 0 failedeach time, and all 6 per-test outcomes match line for line between the passes.prettier --checkandeslintboth exit 0.conduction/hydra-gatesv1.16.0, diff-scoped toorigin/development) exit 0 — 77 of 77 applicable gates ran, gate-16 spec-coverage PASS, gate-19 advisory-only for the repo-wide legacy backlog.CI was not waited on; the verification above is local.
🤖 Generated with Claude Code