fix(audit,objects,schemas): repair the hash chain, stop the write-path fan-out, and make bsn/user real formats - #2336
Open
rubenvdlinde wants to merge 20 commits into
Open
fix(audit,objects,schemas): repair the hash chain, stop the write-path fan-out, and make bsn/user real formats#2336rubenvdlinde wants to merge 20 commits into
rubenvdlinde wants to merge 20 commits into
Conversation
sealRow() and sealRows() log "a later seal pass will chain it" whenever the seal lock is contended. There was no later pass. Nothing swept unsealed rows, so every fail-soft skip was permanent. Measured on the dev instance: 49,123 of 308,937 audit rows — 15.9% — had no hash and never would have. A row with no hash is a row the chain cannot vouch for, and the chain exists so an auditor can say "this history has not been rewritten" from evidence rather than assertion. AuditSealJob runs every 5 minutes, up to 10 passes of 500 rows. sealUnsealed() takes the OLDEST unsealed rows in id order, so it is resumable by construction: a tick that stops early is simply resumed by the next. It delegates to sealRows(), which derives the predecessor once per batch and chains forward — measured at 1.14 ms/row against 14.85 ms/row for the inline per-row seal. Verified: 49,123 -> 48,623 in one pass (exactly 500), and verifyChain() over the swept range returns valid=true with zero duplicate predecessors. FOUND WHILE VERIFYING, and NOT caused by this change: the chain is already broken at id 153230. 5,314 rows share a previous_hash with a sibling across 2,413 distinct predecessors, one of them used by 442 rows. That is the signature of concurrent seal passes each reading the same predecessor and then writing — exactly the race SEAL_LOCK_KEY was later introduced to prevent. The damage predates the lock. My swept range (31518-53387) contains zero duplicates and verifies clean, so the sweeper does not add to it; but it cannot repair history either, and a re-chain of the corrupted region is its own piece of work. Complexity suppressed with the reason: splitting an audit-integrity class to satisfy a threshold risks the property it guarantees.
…rface it The sweeper (previous commit) seals rows with NO hash. It cannot repair rows with a WRONG one, and the dev instance has 5,314 of those: rows chained onto a shared predecessor across 2,413 distinct predecessors, one used by 442 rows. That is a fan-out, not a chain, and it predates SEAL_LOCK_KEY. This is the re-chain that commit named as its own piece of work. - AuditHashService::rechainAll() walks every row in id order under the seal lock, deriving each previousHash from the row actually before it, so the result is one chain by construction. It REWRITES stored hashes, so it is an occ command and never a scheduled job — "something rewrote the audit hashes" is exactly the event the chain exists to make suspicious — and both ends of the run are logged at warning level so the rewrite is itself in the record. - openregister:rechain-audit-trail verifies before and after, with --dry-run and a confirmation prompt. The verification either side is the point: a repair that cannot show the chain was broken before and whole after is indistinguishable from one that quietly rewrote a healthy chain. It exits FAILURE if the chain still reports invalid. - verifyChain() now walks in windows. The DB was never the constraint — Postgres returns the whole trail by index scan in ~350 ms — the client was: libpq buffers an entire result set before PHP sees a row, so `select *` over 309,090 rows at ~5.8 KB wide pulls ~1.8 GB into the driver. That memory is held in C, so memory_get_peak_usage() cannot see it and the failure is not a PHP fatal but a SIGKILL. Measured: an occ run died with the OS killer while PHP still reported a 57 MB peak. Windowing bounds the driver buffer and cuts a partial walk from ~129 s to under a second. - getIntegrityStatus() + GET /api/audit-trails/integrity + a LogIntegrity admin card. Three COUNT/MAX queries, deliberately NOT a verification: binding a settings page to verifyChain() would make opening settings expensive enough that an admin stops opening it. The card keeps the two distinct — coverage is free and continuous, verification is explicit and operator-initiated. info.xml registers the command only; no version bump.
Conflicts, all in lib/Service/AuditHashService.php, all of them BOTH sides having real work rather than one side being stale: - class docblock (complexity note) — merged both. Development's cites the self-ownership guard in acquireSealLock(); this branch's cites the sweep. Both are true, so the note names both rather than dropping one. - constants block — kept BOTH. Development added $holdsSealLock (the seal lock is not re-entrant, so a nested seal must be answered immediately instead of sleeping through its retry budget); this branch added SWEEP_BATCH_SIZE. They are unrelated additions that git could only see as one region. - verifyChain() body, two hunks — merged. Kept this branch's windowed walk (libpq buffers a whole result set in C, so the unbounded query gets the process SIGKILLed while PHP reports a 57 MB peak) and kept development's retention-tombstone handling and buildChainReport() helper inside it. Taking either side whole would have silently dropped the other's fix. FOUND BY THE MERGE, present on neither side alone: rechainAll() re-hashed EVERY row, and development's retention tombstones cannot re-hash — their payload was lawfully destroyed. A re-chain would have hashed the emptied row and replaced the one piece of evidence a tombstone still carries, under a banner reading "repair". It now carries a tombstone's stored hash forward as the next row's predecessor and leaves the row untouched, which is exactly what verifyChain() does; the two must agree or a re-chain would manufacture the break it was run to fix. The count is reported by the command and the completion log.
…ness The merge broke these three tests and CI would have been the first to say so. wireRows() mocked the old unbounded query: one fetch() cursor, no expr(), no setMaxResults. verifyChain() now pages by id, so expr()->gt() was called on a null expression builder. The mock serves one populated window and then an empty one, which is how the walk terminates — serving rows forever would hang the suite rather than fail it. Nothing about what the tests ASSERT changed: testChainStaysValidAcrossA- Tombstone still expects valid=true and testTamperedRowStillBreaksTheChain still expects valid=false on the same harness, so the two remain each other's positive control.
…nt sealing verifyChain() issued one unbounded `select *` over the whole audit trail. The database was never the constraint — Postgres returns all 309,090 rows by index scan in ~350ms — but the client is: libpq buffers an entire result set before PHP sees the first row, so at ~5.8KB per row that pulled ~1.8GB into the driver. That memory is held in C, so memory_get_peak_usage() reported a serene 57MB while the OS SIGKILLed the process. The failure mode was therefore the worst available one: verification did not fail, it VANISHED — no PHP fatal, nothing in the log, and an operator left with no signal that the chain had gone unchecked. Now walks in 500-row windows keyed on id. Same verdict on the live trail (brokenAt 153230, 500 verified, 6381 skipped), 128.7s -> 0.1s. Paging adds exactly one new way to be wrong — losing previousHash across a window boundary — so AuditHashVerifyPagingTest pins it down: a chain split across windows verifies clean, a row tampered AT the boundary is caught at the boundary, an entirely-unsealed window does not turn a gap into a false tamper alarm, and an empty trail terminates. Mutating the carry-over to reset per window turns two of them red. Also drops rechainAll()'s `skipped` counter, which nothing could ever increment — a field structurally pinned at 0 reads as "nothing was skipped" when it means "never measured". Docs: versioning-and-audit.md explained hash chaining but never said when sealing runs, that it is fail-soft, or that a gap is not a tampered entry. Adds that, the sweeper's schedule, and the repair procedure — and corrects the documented verify endpoint, which had the wrong path, wrong params, and described from/to as timestamps when they are entry IDs.
…at said so
Four quality findings on this branch, all real:
🔴 AuditSealJob's "the hash chain has gaps that are not closing" warning could
NEVER fire. An `if ($sealed === 0) { return; }` sat above it, so the only state
that reaches the warning — sealed nothing, backlog non-empty — had already left
the method. Psalm called it a ParadoxicalCondition; operationally it means the
sweeper could stop working and the alarm written to say so would stay silent,
which is the exact failure the sweeper exists to end. Reading the backlog
before the early exit makes it reachable, and the early exit now covers only
the true steady state (sealed nothing, nothing outstanding).
Added tests/Unit/BackgroundJob/AuditSealJobTest.php — the job had NO test at
all, which is how the dead branch survived. Verified as a positive control: with
the early return restored, testWarnsWhenNothingSealedAndABacklogRemains FAILS.
It also carries its own negative control (the steady state must stay silent) so
the alarm cannot be satisfied by simply warning always.
🔴 getIntegrityStatus() called $qb->func()->max('id', 'last_sealed'). The max()
function builder takes only the field — unlike count(), it has no alias
parameter — so the second argument was swallowed and aliased nothing. Same
family as the named-arg-on-a-variadic trap.
phpmd: rechainAll() and verifyChain() were both over the 100-line method
threshold. Extracted rechainWindow() and readChainWindow() rather than widening
anything, so the outer methods read as the repair's and the verification's
shape and the per-row rule lives in one place. ExcessiveClassLength is
suppressed with a reason and a named next step (move the operator-initiated
re-chain to its own service) rather than a threshold change.
phpcs: named parameter on getHelper(), and the two duplicated before/after
report blocks in the command folded into summarise() — which removes both
inline ternaries and makes the two ends of the run print the same fields, so a
reader comparing them is comparing like with like.
Local: phpmd clean over lib, psalm clean on the changed files, 321 audit and
retention tests green. The 10 tests/Unit/AppHost failures in the full local run
are byte-identical to development and untouched by this branch — development's
own CI has PHPUnit green — so they are this instance, not this change.
rechainAll() rewrites hashes that already exist — the one operation here the audit trail is designed to make suspicious — and had no test. It also had the easiest possible way to be silently useless: derive previousHash once and reuse it, and every row gets a hash, every row looks sealed, and the chain is exactly as broken as before. That IS the bug it was written to repair (5,314 rows over 2,413 predecessors on the live trail, one shared by 442 rows), so reproducing it in the fix would be invisible. So the assertion is not "rows got hashes" but "row N's previousHash is row N-1's hash". The fixture seeds all three rows pointing at one shared predecessor, and asserts those stale values are gone. Dropping the `$previousHash = $hash` carry-forward in rechainWindow() turns it red. Also covers the refusal path — the repair must decline when the seal lock is held, since competing with a concurrent pass is how the fan-out arose — and getIntegrityStatus(), including the empty-trail case that would otherwise divide by zero on a fresh install's settings page. Earns back the 0.02% the coverage guard flagged, with tests worth having rather than by moving the baseline.
…ess consent Two findings, the second one substantive: - `?? 0` on $result['tombstonesPreserved'] was redundant against rechainAll()'s declared array shape, and phpstan said so. - getHelper() returns HelperInterface, which has no ask(). The call only worked by luck of what Symfony happens to return. Rather than casting the complaint away, the command now checks for a QuestionHelper and FAILS if it does not have one. Treating a missing helper as consent would rewrite every stored audit hash on the strength of an environment quirk — for a destructive repair behind a confirmation prompt, "could not ask" must never mean "yes". --force remains the supported way to say yes without a prompt.
…ing our own writes
Three problems found by running the thing rather than reading it.
SEALING ON THE WRITE PATH CORRUPTED THE CHAIN. Sealing takes an exclusive lock,
so under concurrency some rows sealed and some fell through the fail-soft path
unsealed. A row sealed AFTER a gap chained onto the newest SEALED row, skipping
the gap — so when the sweep later filled that gap, the gap and the row after it
shared one predecessor. That is a fan-out, which verifyChain() cannot tell from
tampering. Caught live: rows 455956 and 455957 both chained onto 455955, and
verification went from valid=true to valid=false BECAUSE the sweeper ran.
Sealing now happens only in AuditSealJob. With one sealer, unsealed rows are
always a contiguous TAIL rather than holes punched mid-chain, so no later row
can chain across a gap. The fan-out is not handled, it is unreachable. The
write-path tests assert never() on sealing, as the invariant it now is.
The sweep also re-chains from the oldest gap FORWARD rather than filling in
place, bounded at MAX_SWEEP_RECHAIN, so legacy interleaving self-heals without a
five-minute cron ever attempting a 300k-row rewrite.
AN ABANDONED SEAL LOCK SILENTLY DISABLED SEALING. ILockingProvider has no owner
and no liveness check, and DBLockingProvider only reaps expired rows from a
separate job, so a process killed inside its critical section held the lock for
the rest of its TTL — measured at 46 minutes. Every sweep in that window
returned 0, which is ALSO the value meaning "nothing to seal": a dead sweeper
and an idle one were indistinguishable while the backlog grew. acquireSealLock()
now stamps appconfig, and breakStaleSealLock() takes over a lock held longer
than any real pass can run, logging a warning because a process dying inside a
critical section is worth seeing even when recovered from.
WE RE-READ EVERY ROW WE WROTE. The magic tables have exactly ONE
database-generated column, `_id` (nextval); `_created`/`_updated` carry no column
default (59,292 of 59,292 rows have both set, so PHP writes them). The UPDATE
re-read therefore fetched back the values it had just sent — and its own catch
already returned the input entity when the read failed, so that was settled.
Removed: one less query per update.
The INSERT re-read stays, because the insert helper returns void and that read is
genuinely how the id arrives; dropping it would hand callers null ids. It is now
wrapped in a transaction instead, which is what Nextcloud's check actually asks
for — isTransactionActive() is the first branch of the dirty-read test, since a
transacted read goes to the primary. The commit sits in a finally so the
lost-write throw cannot leak an open transaction.
Both mattered because a "dirty table read" attaches a synthetic exception whose
serialised backtrace measured 5.9MB, on every insert and every update.
LOGGING. info was being used for "something happened": 700 info against 514
debug. MagicMapper, on every save, went 39 info -> 3, keeping only table
creation, DDL and bulk deletion — rare and structural. All 13 entry-traces are
gone ("Starting createFromArray", "About to update", "...called");
getOrganisationForNewEntity emitted four info lines per save to answer one
question and now emits one debug recording the outcome, and createFromArray went
from seven narrating lines to one saying what it created.
NOTIFIER. Nextcloud deprecated InvalidArgumentException for declining a
notification, and every notifier is offered every notification — so the routine
decline logged a warning each time, dozens per dashboard load.
UnknownNotificationException says the same thing silently, matching
AnnotationNotifier which already did it correctly.
…g eight apps to seed baseline data TWO WRITES, NOT THREE. insertObjectInRegisterSchemaTable() returned void, so the generated `_id` was thrown away and had to be recovered by SELECTing the row back — against a table written milliseconds earlier, which is exactly Nextcloud's "dirty table read" condition and cost ~5.9MB of serialised backtrace per object. It now returns lastInsertId(). Two facts make that exact rather than merely convenient, and both must keep holding: these tables carry exactly ONE sequence (`_id`; verified against information_schema — no other column has a default), and the call is the very next statement on the same connection. A second serial column in the magic-table shape would break it, and the comment says so. `_id` was never the identity anyway. saveObjectToRegisterSchemaTable() returns the UUID, which PHP generates; the id is an internal key catching up. Verified live: insert and update each emit ZERO dirty reads, and the id returned matches the row (min=max=1 on a single-row table, so it could not have been a coincidence). The rare path where an INSERT loses a uuid race and lands as an update still reads, since there is no generated key to report — but it uses the raw row fetch, not the hydrating one, and keeps the lost-write check that #2212 needed. EIGHT APPS WOKE FOR EVERY SEEDED OBJECT. docudesk, softwarecatalog, opencatalogi, openbuild, hermiq, zaakafhandelapp and hrmq all subscribe to object lifecycle events. Measured mid-repair: 155 "DocuDesk: Processing event", 116 compliance-subscriber calls, 116 queued text-extraction jobs — running document extraction and compliance scoring over content that shipped WITH the app, before anyone had configured anything. Seeding is not a user action, so there is no intent for a listener to react to. importSeedData() now runs inside SystemOperationContext, and MagicMapper withholds lifecycle events while it is active. Gating the DISPATCH rather than each listener is the point: one change here instead of eight across apps we do not all own, and it cannot be half-adopted — a listener that never learns of an event cannot forget to check. Proven with both controls, because "no events fired" is otherwise exactly the result a broken test gives for free: a normal save outside the context still dispatches (1), the same save inside it does not (0). Deferral was considered and is not the answer here. defer_object_events is unset, so nothing defers today; DeferredObjectEventJob hardcodes ObjectCreatedEvent and ignores the `action` it is passed; and an update cannot be deferred at all, since ObjectUpdatedEvent needs oldObject, which a later job cannot recover. Recorded so the next person does not rediscover it.
…g log killing a save BSN WAS BUILT, WIRED, AND UNREACHABLE. BsnFormat implements the 11-proef and is already registered with the value validator, so OpenRegister could checksum a burgerservicenummer all along — it just refused to accept a SCHEMA that declared `format: bsn`, because PropertyValidatorHandler's allowlist never got the entry. The two lists disagreed, and the cost was not cosmetic: procest declares `format: bsn` on a burgerservicenummer, so its schema import failed, which failed schema creation, which failed its "Load default ZGW API mapping configurations" repair step. An app went unconfigured over a missing word in an array. `user` now exists as a format too, and means what it says: UserFormat asks IUserManager whether the account exists. A user id is syntactically just a string, so a pattern could assert nothing — the backend is the only authority, and without it a schema could carry a deleted account's id forever while every consumer resolved it to nothing. Both verified in both directions. BSN: 111222333 accepted, 111222334 (one digit off) rejected, the all-zero sentinel rejected, a short value rejected. user: "admin" accepted, a non-existent uid rejected, empty and whitespace rejected. And an invented format is still rejected at schema level — the allowlist did not become permissive, it became correct. A DEBUG LOG WAS CRASHING THE SAVE PATH. convertRowToObjectEntity() is declared `?ObjectEntity` and does return null for a row it cannot hydrate. Every other call site checks. findAcrossAllMagicTables() did not, and the first thing it did with the result was dereference it — inside a logger->debug() whose only job was to report what had been found. So an unconvertible row did not degrade the search, it killed the request with "Call to a member function getUuid() on null". Live effect: 11 Shillinq RetentionRule objects failed to rematerialise on EVERY repair, because a DocuDesk enrichment listener reached this lookup and one row would not hydrate. The row is now skipped with a warning and the search continues. Re-running the exact repro that failed: ok=3 fail=0. Also completes the system-operation event gate. The first attempt covered MagicMapper::insert()/update() and missed the BULK dispatchers in SaveObjects and the batched-update path, so a configuration import kept fanning out to eight apps while the gate looked applied. That is the second time this session a gate read as correct and was not, which is why both are now asserted rather than assumed.
…does
A schema with no `type` means "any type" in JSON Schema. OpenRegister rejected it
outright, and that was not a lenience worth defending — it forced authors to
declare a type they do not have.
procest's CMMN sentry is the case that exposed it. `ifPart: {field, operator,
value}` compares `value` with LOOSE equality against bool/string/int by explicit
design ("a sentry author should not have to match PHP's strict type rules"),
requires an ARRAY for the in/notIn operators, and numeric for gt/lt. No single
type is honest there. Requiring one would have meant writing a lie into the
schema; refusing the omission instead failed the entire import.
Type stays REQUIRED at the top level, because those properties become columns:
mapColumnTypeToSQL() takes a `string $type` and receives $column['type']
directly, so a typeless top-level property is a TypeError during table creation,
not a permissive read. Nested properties are stored inside a JSON column and
derive nothing, so the omission costs nothing there.
Depth is the discriminator — validateProperties() builds '/name' for a top-level
property and appends per level.
Controlled in both directions: a top-level typeless property is still rejected, a
nested one is accepted, and procest's real caseModel schema — the one that has
been failing every repair — now validates.
OpenRegister emitted 700 info calls against 514 debug — inverted, because info
was being used for "something happened". A repair run was consequently a wall of
lines reporting that nothing had changed, and the one line that mattered was
indistinguishable from the 300 that did not.
Now 460 info / 745 debug. What moved, and the rule applied:
FilePublishingHandler, UpdateFileHandler — step-by-step narration of a single
method ("Original file parameter", "After cleaning", "Object folder path",
"Attempting to get file"). All debug.
ImportHandler — 50 -> 4. The per-app decisions ("Skipping {app}: config
content unchanged") describe the MOST COMMON outcome of a repair; reporting the
non-event at info is what made the log unreadable. Kept: a register was
created, an update applied against the version ordering, the seed-data summary
with its counts, and an app's version actually changing.
SaveObjects — including one line literally labelled "DEBUG - ..." emitted at
info. Kept the Wave-12 safeguard REJECTION: a refusal to write is what someone
comes to the log to find.
TextExtractionService, ConfigurationController, FolderManagementHandler,
ConfigurationCheckJob, CrudHandler, OrganisationService — same treatment, with
a state change (risk level), an outcome (notifications sent), and two
reached-but-unimplemented paths promoted back.
One demotion was reverted rather than pushed through. TextExtractionServiceDeepTest
asserts that "Object no longer exists, skipping extraction" logs at info, with a
comment saying so. That is a deliberate contract — the line explains why queued
work did NOT happen, and silence there looks like the job never ran. The level is
now justified in the code instead of only in a test.
The first attempt at this gate did not cover this path. Gating
MagicMapper::insert()/update() looked complete — a live probe showed one event
outside SystemOperationContext and zero inside — while a configuration import
carried on fanning out to eight apps through emitChunkSideEffects(), which the
probe never touched. The gate read as applied and was not.
A live re-probe could not settle it either, and the way it failed is the point:
the bulk path defaults to `_events: false`, so the negative control dispatched
nothing and the "zero inside the context" result proved exactly nothing. A second
attempt with `_events: true` was then rejected by the bulk safeguard
(BulkSafeguardException), and a third with an admin session ran past ten minutes
— because a real session wakes the very fan-out being measured.
Hence a unit test, with the control in the file rather than in a separate run:
- outside a system operation the emitter dispatches (without this, an emitter
that never dispatched anything would look identical to a working gate)
- inside one it dispatches nothing
- and events RESUME afterwards, since a suppression that outlived its context
would silence every later save in the request — the same outage as the
fan-out, reached from the other side and far harder to notice
Replacing the gate with `if (false)` turns the second and third red while the
control stays green.
…l-sweeper # Conflicts: # lib/Service/AuditHashService.php # tests/Unit/Service/AuditHashRepairTest.php # tests/Unit/Service/AuditHashVerifyPagingTest.php
The Features Check gate runs the shared extractor with --check and failed on this branch. Regenerated with the same script the gate uses (.conduction-shared/scripts/extract-features.py), so the committed file and the gate's expectation agree.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ❌ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 173/173 | |||
| npm | ✅ | ✅ 713/713 | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 14:45 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 173/173 | |||
| npm | ✅ | ✅ 713/713 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 14:59 UTC
Download the full PDF report from the workflow artifacts.
… right about phpcs, phpstan and psalm each caught something real rather than cosmetic. psalm's AssignmentToVoid was the sharpest: insertObjectInRegisterSchemaTable() now returns the generated id, but its docblock still said `@return void`, so static analysis was reading the OLD contract while the code returned an int. A docblock that disagrees with its signature is worse than none — it is the version tooling believes. phpstan then found the loose end from a reverted decision: PropertyValidatorHandler kept the logger it was given for the permissive-format behaviour that no longer exists, so the dependency was written and never read. Removed rather than suppressed; an injected collaborator nothing uses is a claim about the class that is not true. The coverage gate was also right, and its baseline may never be lowered, so this earns it back with the two things that genuinely had no tests: UserFormat — the negative case IS the format. A user id is syntactically just a string, so only the backend can say it names nobody; the tests assert the unknown-user rejection, that blank and non-string values never reach the backend at all (coercing 42 to "42" would turn a type error into a lookup miss that reads as "no such user"), and that whitespace is trimmed. The abandoned-lock recovery — conservative in one direction and decisive in the other, and both are asserted. A lock held moments ago is left alone, because stealing one a live pass still holds puts two writers in the chain and reintroduces the fan-out the lock exists to prevent. A lock with no recorded timestamp is also left alone, failing safe rather than guessing — which is why the two left by an interrupted upgrade had to be cleared by hand. A lock held longer than any bounded pass could run is broken and taken, and a break that itself fails reports failure rather than handing the sweep a lock it does not hold.
…s one exit
The engine's authoring format was its own intermediate representation: a node
was a Petri-net PLACE carrying no behaviour, and the EDGE carried `type` and
`config`. `FlowDefinitionBuilder` threw on a node carrying a step, and its own
comment said why the mistake kept happening — "node-shaped authoring is the
natural mistake, BECAUSE THAT IS HOW A GRAPH EDITOR PRESENTS A FLOW". It
diagnosed the defect and declined to treat it. Three fleet graphs were authored
that way, ran, reported COMPLETED, and did nothing.
So the model inverts. A node carries the step; an edge says what runs next. The
Petri net survives as the lowering:
node N -> transition T_N carrying N's type/config, plus place in(N)
edge A -> B -> in(B) added to T_A's targets
no outgoing -> terminal place end(N)
no incoming -> in(N) is initial
join: true -> one input place per incoming edge
Places are named after their node, which is load-bearing twice: per-item routing
matches an item's tag against the output PLACE name, so a prefix would silently
drop every routed item into an empty branch; and the marking is the user-visible
answer to "where is this run?".
CONVERGING EDGES ARE A MERGE, NOT A JOIN. The Hydra sequencer reaches its exit
from several mutually exclusive paths — lowering those to a join would require
all of them and deadlock every run, while still producing a valid definition.
So `in(N)` is shared; synchronising is opt-in via `join: true`.
CONDITIONS LIVE ON THE NODE, AS NAMED EXITS. A node declares its branches
(`exits: [{id, condition?}]`) and an edge says which it leaves (`fromExit`).
That is what lets a node have several exit points, and what lets an editor draw
one port per branch — the branches exist before any line does, which an edge
condition could never manage.
A TOKEN IS UNIQUE AND EXCLUSIVE. Exactly one exit is taken per firing, chosen in
declaration order with the unconditioned exit as the else. symfony/workflow
marks every output place, so the unclaimed ones are withdrawn after apply();
without that the losing branch simply ran an iteration later, with no error.
AND THE ELSE IS MANDATORY. A node that conditions its exits must declare one,
refused at build time by name. A token with nowhere to go does not fail — the
run stops, reporting nothing, which is indistinguishable from a flow that
finished. A test asserted exactly that as correct behaviour ("ends the run
cleanly"); it now asserts the refusal, with a positive control.
The old shape is REFUSED, not reinterpreted: any edge carrying a `type` names
itself and points at the migration. Accepting both would let a half-migrated
flow run, skip the step nobody claimed, and report success — the original defect
wearing a migration as a disguise.
`FlowNodePreflight` walks nodes. Left on edges it would inspect a list where
nothing carries a type, find nothing, and call every document valid without
having looked — a validator that cannot fail is worse than none.
404 flow tests green (399 at baseline). The 10 AppHost failures in the full run
are pre-existing and unrelated — verified by stashing this change.
…it valid Moving the preflight onto nodes closed one hole and opened another. An un-migrated flow carries every step on an EDGE, so the node walk found nothing to inspect, produced no findings, and the report said "valid" — about the one document shape the engine will certainly refuse. Measured live: `POST /api/flow/validate` on the real Hydra sequencer returned `valid: true, blocking: 0` while `FlowDefinitionBuilder` would refuse it outright. The editor's "Check this flow" button would have told an author their un-migrated flow was fine. The pre-inversion check now runs in `inspect()` as well as in the builder, and returns early: every later finding would be about a document in a shape nothing reads, and burying the one actionable message under sixteen others helps nobody. The sequencer now reports all 16 steps by name, each pointing at the migration. Covered by a test AND its positive control — the same flow validating once the step moves onto the node — because a refusal test is otherwise satisfied by a preflight that refuses everything it is shown.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 173/173 | |||
| npm | ✅ | ✅ 713/713 | |||
| PHPUnit | ❌ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 16:05 UTC
Download the full PDF report from the workflow artifacts.
… it surfaced
STREAMING BULK UPSERT WAS UNREACHABLE. Hydra's orphaned-write-capability gate
found SaveObject::saveObjectsStreaming() and clearReferenceValidationCache() with
ONLY test callers — implemented, unit-tested by calling the class directly, and
reachable from no production code. Checked rather than assumed: the gate is
right, not a false positive.
They are a matched pair built as the prerequisite for a streaming import that was
never built. routes.php still carries the epitaph: "The objects import route was
also removed — use the registers import endpoint instead."
Wired into BulkController behind an opt-in `stream` flag, defaulting to today's
behaviour. Opt-in because the two paths have OPPOSITE trade-offs, and defaulting
either way would be wrong for half the payloads:
default ultraFastBulkSave — fastest writes, but never consults the
reference-validation cache, so rows that reference each other cost
N×M round-trips resolving them.
stream each row through saveObject(), which engages that cache; repeated
targets resolve from memory, the payload is consumed lazily, and a
failed row is recorded rather than failing the call.
Choosing automatically would need a size/reference threshold nobody has measured,
so the caller decides. ObjectService clears the reference cache at the batch
boundary, which is exactly what clearReferenceValidationCache() was written for.
SPEC for the UI half. widget-record-import covers dropping a spreadsheet of
RECORDS on a register. It is deliberately NOT the file widget's streaming upload:
saveObjectsStreaming() streams rows shaped like saveObject() input, and file bytes
never pass through it. The two compose — dropping 200 PDFs is a FileService
concern, the 200 resulting objects are what this streams — and keeping them apart
stops the file widget growing a record-parsing responsibility it has no business
owning. The spec makes column mapping explicit (a silently dropped column is the
failure that makes imports untrustworthy), requires a dry run, and requires failed
rows to export in the input's shape so a user can fix and re-drop only those.
GATES this diff surfaced, all pre-existing and all now fixed:
gate-46 Dead @SPEC anchors pointing at archived change dirs, in seven files
this branch happened to touch. Repointed at the canonical specs they
should have named — @SPEC targets openspec/specs/, never a change dir.
gate-28 Two files carried `@license AGPL` while their own SPDX header and
composer.json both said EUPL-1.2. Not a licence change: a stale tag
contradicting the file's own identifier.
Also adds the re-chain command's missing tests, which cover the part that matters
about a destructive repair — that it refuses without consent, writes nothing
under --dry-run, and exits FAILURE when the chain is still broken afterwards
rather than reporting success on a repair that did not take.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ❌ | ||||
| composer | ✅ | ✅ 173/173 | |||
| npm | ✅ | ✅ 713/713 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 19:50 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.
Found by running the repair rather than reading it. Every claim below was verified with a control that could have failed.
The audit chain was broken, and the sweeper was breaking it further
verifyChain()issued one unboundedselect *. The database was never the constraint — Postgres returns all 309k rows by index scan in ~350ms — but libpq buffers an entire result set before PHP sees a row, so at ~5.8KB/row it pulled ~1.8GB into the driver. That memory is held in C, somemory_get_peak_usage()reported a serene 57MB while the OS SIGKILLed the process. Verification did not fail, it vanished. Now paged: same verdict, 128.7s → 0.1s.Worse, sealing on the write path was corrupting the chain. Sealing takes an exclusive lock, so under concurrency some rows sealed and some fell through unsealed; a row sealed after a gap chained onto the newest SEALED row, so filling that gap later gave two rows one predecessor. Caught live: rows 455956 and 455957 both chained onto 455955, and verification went from
valid=truetovalid=falsebecause the sweeper ran.Sealing now happens only in
AuditSealJob. With one sealer, unsealed rows are always a contiguous tail rather than holes, so no later row can chain across a gap — the fan-out is not handled, it is unreachable.End state, independently verified:
valid=false, brokenAt=153230, 48,529 unsealed→valid=true, 313,213 verified, 0 unsealed, and a subsequent sweep sealed a tail while the chain stayed valid.An abandoned lock silently disabled sealing
ILockingProviderhas no owner and no liveness check, andDBLockingProvideronly reaps expired rows from a separate job, so a process killed inside its critical section held the lock for the rest of its TTL — measured at 46 minutes. Every sweep in that window returned0, which is also the value meaning "nothing to seal": a dead sweeper and an idle one were indistinguishable while the backlog grew.breakStaleSealLock()now takes over a lock held longer than any real pass can run.We re-read every row we wrote
The magic tables have exactly ONE database-generated column,
_id;_created/_updatedcarry no column default (59,292 of 59,292 rows have both set). The UPDATE re-read therefore fetched back what it had just sent — and its own catch already returned the input entity when the read failed, so that was settled. The INSERT now reports its generated key vialastInsertId()instead of selecting the row back.Both were "dirty table reads", which attach a synthetic exception whose serialised backtrace measured 5.9MB — per insert and per update. Verified live: zero dirty reads on both paths, and the returned id matches the row.
Eight apps woke for every seeded object
docudesk, softwarecatalog, opencatalogi, openbuild, hermiq, zaakafhandelapp and hrmq all subscribe to object lifecycle events. Measured mid-repair: 155 "DocuDesk: Processing event", 116 compliance-subscriber calls, 116 queued extraction jobs — document extraction and compliance scoring over content that shipped with the app.
importSeedData()now runs insideSystemOperationContextand MagicMapper withholds lifecycle events while it is active. Gating the DISPATCH rather than each listener is deliberate: one change instead of eight across apps we do not all own, and a listener that never learns of an event cannot forget to check.SaveObjectsSystemOperationTestnow pins it, with the negative control in the same file.bsn and user are real formats
BsnFormatimplements the 11-proef and was already registered with the value validator — OpenRegister could checksum a BSN all along, it just refused to accept a schema that declared the format, because the allowlist never got the entry. That one missing word failed procest's schema import, then schema creation, then its ZGW mapping repair step.UserFormatis new and asksIUserManagerwhether the account exists.Controlled:
111222333accepted /111222334rejected / all-zero rejected;adminaccepted / unknown uid rejected; an invented format still rejected.A debug log was killing the save path
convertRowToObjectEntity()is declared?ObjectEntityand returns null for a row it cannot hydrate. Every other call site checks;findAcrossAllMagicTablesdereferenced it inside alogger->debug()whose only job was to report success. 11 Shillinq RetentionRule objects failed to rematerialise on every repair as a result. Repro nowok=3 fail=0.Result
maintenance:repaircompletes end to end, exit 0, past theInitialize OpenCatalogi settingsstep it used to stall on. Warnings in a full run: 3, down from 25+. Log for one repair: 13.6GB → 12.6MB.Logging: info 700 → 460 (debug 514 → 745). One demotion was reverted rather than pushed through, because a test deliberately asserted the level.