docs(objects): failIfExists is NOT a lock — say so where it is read - #2213
Merged
Conversation
#2211 shipped with a guarantee stronger than it delivers. I verified it sequentially (claim1 -> 201, claim2 -> 409) and merged. The concurrent test fails: 10 simultaneous claims on one identifier, three runs run1: 201=6 409=2 rows=1 run2: 201=4 409=6 rows=1 run3: 201=2 409=8 rows=1 Multiple callers receive 201. Exactly one row survives, so the extra 201s are lost updates reporting success. The guard sits between the existence lookup and the write — two separate operations — so N callers can all pass the lookup before any of them writes. It narrows the window; it does not close it. What holds: the DEFAULT path is untouched, so no existing caller is affected, and a sequential duplicate is still correctly refused. What does not: `_failIfExists` / `onConflict: fail` must not be relied on for mutual exclusion. Closing it means letting the database arbitrate — a real INSERT against the existing _uuid unique constraint, translated into ObjectExistsException. Adds that warning to all three places someone will read before trusting it: the exception's own docblock, the guard in SaveObject, and the node's onConflict constant. Tracked as #2212. The acceptance criterion in hydra task 3.5 said "prove this with two flows started simultaneously, not last". I wrote that criterion and then verified sequentially anyway.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 174/174 | |||
| npm | ✅ | ✅ 555/555 | |||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-07-30 22:24 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
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.
Follow-up to #2211, which I merged with a guarantee stronger than it delivers.
The concurrent test fails
10 simultaneous
POST …?_failIfExists=trueon one identifier, three runs:Multiple callers receive 201. One row survives, so the extra 201s are lost updates that reported success. The guard sits between the existence lookup and the write — two separate operations — so N callers can all pass the lookup before any writes.
My sequential test (
claim1 → 201,claim2 → 409) passes precisely because it serialises the calls. That was the wrong test, and it is the one that convinced me.What still holds
What does not
_failIfExists/onConflict: failmust not be relied on for mutual exclusion. Closing it means letting the database arbitrate: a realINSERTagainst the existing_uuidunique constraint, translated intoObjectExistsException. Tracked in #2212.This PR
Puts that warning in all three places someone reads before trusting it — the exception docblock, the guard in
SaveObject, and the node'sonConflictconstant. Docs only; no behaviour change.The acceptance criterion in hydra task 3.5 said "prove this with two flows started simultaneously, not last". I wrote that criterion, then verified sequentially anyway.