Release: merge development into beta - #1711
Open
github-actions[bot] wants to merge 3058 commits into
Open
Conversation
* wip: preserve uncommitted working tree (AppHost settings plane, flow wiring, schema dedup)
Snapshot of the local working tree taken before reconciling with
origin/development, which had moved 69 commits ahead.
Much of this tree is already upstream (27 of 36 new PHP files and 13 of 49
modified files are byte-identical to origin/development). The genuinely new
work preserved here is:
- lib/AppHost/{Controller,Service}: generic settings plane
(GenericSettingsControllerBase, GenericSettingsService, RegisterConfigResolver)
- lib/Command/DedupCollidedSchemasCommand.php
- unit tests for the above plus HandlesExceptionsTrait
- four openspec changes (apphost-settings-plane, apphost-schedule-flow-action,
app-declared-credential-providers, or-flow-object-write-node)
- lib/Settings/flow_register.json and flow e2e coverage
Committed on a stale base on purpose so the subsequent merge of
origin/development is a real three-way merge rather than a tree overwrite.
* docs(openspec): audit-seal-backlog-repair — drain the unsealed audit backlog
Measured on the shared dev instance: 108,151 of 227,063 audit rows (48%) carry
no hash, interleaved across the whole id range (31,518..290,493 = min..max id).
Three defects, all evidenced:
1. insertHashChained() seals per row under a global exclusive advisory lock
(3 attempts x 50ms, fail-soft). Under any concurrency each insert pays up to
150ms and then abandons the seal anyway. Measured ~152 rows/min during the
maintenance:repair that had to be killed after 74 minutes with ~12h left.
The batched sealRows() path already exists but single inserts never reach it.
2. The backfill that specs/audit-hash-chain/spec.md:105 requires does not exist.
harden-audit-seal-concurrency (12/12 complete) made the lock fail-soft on the
explicit promise that a "later seal pass chains them" -- that pass was never
built, so every contended write permanently degrades the chain.
3. verifyChain() skips ANY null hash and still returns valid: true. The comment
claims "pre-migration entries" but no cutover marker exists in lib/, so the
tamper-evidence check currently passes over a table that is 48% unverified.
The change specifies a windowed driver around sealRows() (one lock per window,
not per row), a two-phase read so sealed rows do not have their ~5.3KB payloads
fetched just to contribute a chain link, a partial index for the backlog cursor
(today it pkey-scans with a filter at 784ms/2000 ids), a hard-capped background
job plus an occ command, and a cutover marker so unsealed rows stop hiding
behind a passing verification.
Design records why a naive bulk call is impossible: sealRowsLocked() SELECTs *
over [min,max], which for this backlog is ~227k rows x 5,270B ~= 1.14GB in PHP.
* docs(perf): plan to bring object writes under 500ms + fix features.json drift
A single-object create currently takes 13-99s on the dev instance (six runs on
larpingapp/character, two-field payload each time: 13.6/17.8/20.4/41.0/62.8/99.1s).
This is NOT the CloudEvent storm — that was openconnector's inert recursion
guard, fixed separately, and a create now emits 1 event rather than 255. The
remainder is our own write path.
Counter deltas across one HTTP 201 create (the 41.0s run):
sequential scans of oc_openregister_schemas 5,135
sequential scans of oc_openregister_registers 6
transactions committed 12,541
Four measured costs:
1. 5,135 schema resolutions against a 1,917-row table. SchemaMapper::find()
HAS a request cache and is a shared service, so this is either a cache
key too specific (rbac/multitenancy flags multiply it 4x) or an uncached
sibling on the hot path. The query is a seq scan by construction —
SELECT * hydrates a ~2KB properties blob and LOWER(slug) defeats any
index ('Rows Removed by Filter: 1916'). ~4ms x 5,135 = ~20s, half the run.
2. Resolving an object reference whose table is unknown emits a UNION ALL
with one branch per magic table. At 2,728 tables that is 690KB of SQL:
planning 3,404.9ms, execution 546.1ms. 86% of the cost is PARSING a
statement that returns zero rows, so no index can help — and it is
usually avoidable, since character's six relation properties each
already declare their target schema.
3. 12,541 commits for one create: essentially every statement autocommits,
and the request waits on each fsync.
4. CloudEvent fan-out, audit-trail sealing (228,932 rows), notification
history and oc_activity all run before the response is returned.
Target p95 <500ms with the 2,728-table shape unchanged — fixing the write
path, not shrinking the dataset. Task 1 is deliberately 'attribute the 5,135
calls before changing anything': guessing at a hot path is how the CloudEvent
guard stayed inert for so long.
Also fixes an unrelated red gate: openspec/specs/saved-search-views/spec.md was
missing the blank line before '## Requirements', so the features-manifest
generator swallowed the heading into the feature summary and docs/features.json
drifted. 'quality / Features Check' fails on development because of it.
* style(dedup): satisfy phpcs on the schema-dedup command
Conduction's standard requires named arguments on internal calls, forbids
inline IFs, and has its own spacing///end conventions — the command shipped
with 21 violations and turned 'quality / PHP Quality (phpcs)' red. 14 fixed by
phpcbf; the rest by hand:
setName/setDescription/addOption -> named arguments (four calls)
splitOne/splitOneLocked/findCollisions/pickOwner call sites -> named arguments
the two ternaries in execute() -> explicit if blocks
All 11 PHP files changed on this branch now pass phpcs.
* ci: regenerate docs/features.json from openspec/specs/ [skip ci]
* style(phpcs): clear the 15 pre-existing violations that kept phpcs red
lib/ was 15 errors over 9 files before this branch, so 'quality / PHP Quality
(phpcs)' failed on development and on every PR opened against it.
6x curl_close($ch) deprecated since PHP 8.0 and a no-op — CurlHandle is
an object freed when it leaves scope, not a resource
needing an explicit close. Removed, with a comment so
nobody re-adds them.
3x missing @PARAM FlowRunController::__construct($userSession),
FederatedConfigService::publish($private),
FlowScheduleService::fire($owner)
6x file header ReconcileDeclaredBackgroundJobs.php put its docblock
AFTER declare(strict_types=1), so phpcs read it as a
stray inline block rather than the file header; tag
order was @author/@license/@copyright. Moved above the
declare and reordered to @author/@copyright/@license.
phpcs now reports 0 errors across all 72 files in lib/. phpstan is unchanged —
the same 7 findings exist with and without this commit (verified by stashing),
so nothing here introduced or masked one.
* perf(objects): scope the cross-schema fallback to the caller's register
ObjectService::find() takes register+schema, does a scoped lookup, and on a
miss retries. The retry dropped BOTH the register and the schema, so a
legitimate 'not in this register' answer was produced by scanning every magic
table on the instance.
That scan is a UNION with one branch per magic table. At 2,728 tables it is
690 KB of SQL, and its cost is almost entirely PLANNING:
Planning Time: 3404.926 ms
Execution Time: 546.145 ms <- returns zero rows
No index can reduce parse time, so the only fix is to not emit it.
The fallback exists for a stale or sibling SCHEMA inside a register the caller
named correctly (openbuild#75 / openregister#1520) — it was never meant to
search other registers. It now keeps the register and drops only the schema,
and MagicMapper::find()/findAcrossAllSources() accept a registerIdScope that
filters candidate tables by register id.
Why this mattered on the write path: the flow-resolver registry asks every
resolver in turn whether a flow is theirs. Each non-owning resolver called
find(register: <its own>, schema: 'flow'), missed correctly, and paid a full
instance-wide scan to say 'not mine'. Measured 2026-07-29, that single widened
fallback was ~1.9s of a ~3.0s create.
event dispatch inside the create 1,900ms -> 80-187ms
wall (median of 5) 3.2s -> 1.55s
Also adds the measurement half of the change:
- tests/perf/object-create.sh reports min/median/p95 plus per-write schema
seq scans and commit count, and separately measures the INSTANCE FLOOR (an
authenticated request doing no object work). Nextcloud boots every enabled
app per request; on this instance, with 92 apps, that floor is ~864ms —
larger than the whole 500ms budget — so wall time alone cannot tell you
whether the write path regressed.
- SchemaMapper::traceRead() attributes every uncached schema read to its
caller (gated on the perf_trace_schema_reads app-config flag); this is
what identified the
1,471-call DocuDesk path.
- WritePhaseProbe times the write path's phases; this is what showed the cost
was event dispatch rather than the INSERT (76ms).
Current state: wall p95 1,421ms = 864ms instance floor + 557ms write path.
Refs openspec/changes/object-write-sub-500ms tasks 0, 1, 5.
* perf(flow): memoise flow resolution for the request
FlowResolverRegistry::resolveFlow() asks every registered resolver in turn.
A resolver that does not own the flow answers by looking the id up in its own
register — so an unowned flow pays the entire chain, and every answer costs a
database round trip.
FlowTriggerService::fire() calls it once per queued run, and it calls it from
runInline() BEFORE checking whether the flow is even synchronous, so an async
flow pays a full resolution for nothing. A save that cascades fires the same
triggers again for each child, so one object write resolved the same ids
several times over.
Memoised per request, misses included: 'no app owns this flow' is the
expensive answer, since producing it requires every resolver to look and fail.
Refs openspec/changes/object-write-sub-500ms.
* perf(objects): optionally defer the created-event dispatch out of the write
ObjectCreatedEvent has ~22 listeners across the fleet. Dispatched inline the
caller waits for all of them: measured 2026-07-29 it was 234-501ms of a ~530ms
create, against a 76ms insert. None of it is needed to tell the caller its
object was saved.
Opt-in via 'occ config:app:set openregister defer_object_events --value=1'.
Off by default, because deferring changes an observable contract: a 2xx stops
meaning 'and every side effect has been applied'. A flow declaring
executionMode: sync exists precisely so its effects land before the save
returns, so this cannot be flipped on for everyone by fiat.
event dispatch inside the create 234-501ms -> 6-8ms
write path p95 (wall minus floor) 557ms -> 300ms
CARRIES THE ACTING USER, and that is not incidental. The first version did
not, and it was a perfectly green no-op: the job ran, logged nothing, threw
nothing — and produced ZERO CloudEvents where the inline path produced one.
A background job has no session, and OpenRegister reads are organisation-
filtered against the session user, so every listener that consults the
register (the CloudEvent firehose gate most visibly) saw an empty instance and
skipped. Deferring side effects without carrying identity does not move the
work, it deletes it. Verified after the fix: deferred dispatch produces
exactly 1 CloudEvent, matching inline.
The impersonation is released in a finally so it cannot leak into whatever job
the worker runs next from the same process, and a missing acting user is
logged at WARNING (not INFO — the default loglevel is 2, and a side effect
that silently vanished is exactly what must not be filtered out).
* docs(perf): record measured results and correct the budget's definition
Two corrections the measurements forced, both of which change what the
requirement can mean:
1. The budget must be wall time MINUS THE INSTANCE FLOOR. An authenticated
request doing no object work costs 864-1,099ms on this instance, because
Nextcloud boots all 92 enabled apps per request. That is larger than the
whole 500ms budget, it is PHP-side (boot issues almost no queries, and
opcache is healthy: 0 OOM restarts, 91% hit rate), and no work in the write
path can remove it. A measurement that does not subtract it reports how
many apps are installed.
2. Deferred dispatch must carry the acting user. Added as an explicit
requirement and scenario after the first implementation shipped a flawless
no-op — zero CloudEvents, no exception, no log.
Results recorded: write path 12.8s -> 183ms median / 300ms p95, inside budget.
Wall 13.7s -> 1.28s.
Task 5 is ticked but was solved by a DIFFERENT mechanism than specified — the
fan-out was reached through the cross-schema fallback, not through untyped
relation properties — so the originally-specified work is explicitly folded
into task 6 rather than quietly counted as done.
* perf(probe): attribute the request timeline, not just the write
Adds WritePhaseProbe::stamp() — absolute offsets from REQUEST_TIME_FLOAT,
distinct from mark()'s durations, because the question is 'how much of the
request had already elapsed before our code ran at all'.
Stamped at OpenRegister's register(), its boot() entry and exit, the create
controller, and the end of the write. One create, wall 1,464ms:
or.register.in 122ms NC core + apps registering before us
or.boot.in 903ms +781ms of OTHER APPS registering
or.boot.out 928ms our own boot: 25ms
ctrl.create.in 964ms +36ms routing and middleware
flush 1,357ms +393ms the actual write
So 964ms of a 1,464ms request elapses before the controller is entered, and
781ms of that is apps registering.
Bracketed directly on the same instance with the same auth:
status.php (no app boot) 47ms
capabilities (full app boot) 970ms
~920ms is booting 92 enabled apps, ~10ms each. 24 of them are Conduction
fleet apps; a deployment running OpenRegister plus a handful of leaves boots a
fraction of that.
This is why the 500ms budget is specified against the write path with the
instance floor subtracted: no change to the write path can move the 920ms, and
a wall-clock measurement of it is a measurement of the app count.
Off unless /tmp/or-trace-write-phases exists, checked once per process.
* perf(probe): stamp register() exit — proves the boot cost is not ours
Adds or.register.out, closing the one gap in the request timeline that still
allowed 'maybe OpenRegister's own registration is the pig'. It is not:
or.register.in 235ms
or.register.out 235ms <- our register() is 0ms
or.boot.in 1,355ms <- +1,120ms of other apps + NC phases
or.boot.out 1,376ms <- our boot() is 21ms
ctrl.create.in 1,451ms
OpenRegister has 268 registerService/registerEventListener calls across a
4,248-line Application.php and they cost nothing measurable, because they are
lazy closures — which is the point of the API and worth having proof of.
So the per-request app-boot cost (~16ms/app across 92 apps, measured by
disabling 8 and restoring them) is Nextcloud's own registration and boot
machinery plus the other 91 apps, and no change inside this app can reduce it.
* chore: retrigger CI (previous run was cancelled by a duplicate dispatch)
* perf: creates are under 500ms end-to-end — p95 469ms
BUDGET MET. Measured on larpingapp/character with tests/perf/object-create.sh:
wall p95 13,700ms -> 469ms
wall median 13,688ms -> 385ms
write path 12,800ms -> 93ms median / 177ms p95
Requires an app token, defer_object_events=1, and PHP JIT disabled
(Conduction/.github#75). With deferral off the p95 is 650ms.
CORRECTS MY OWN EARLIER ANALYSIS. This change previously documented an
'instance floor' of 864-1,099ms attributed to Nextcloud booting 92 apps, and
concluded wall-clock under 500ms was unreachable without disabling apps. That
was an artefact of the benchmark, not a property of Nextcloud.
Every sample authenticated with HTTP Basic auth carrying the ACCOUNT PASSWORD,
which Nextcloud bcrypt-verifies on every request. Same endpoint, same instance,
back to back:
account password median 1,058ms
app token median 456ms
~600ms per request was password hashing. No real client authenticates that way
- browsers carry a session cookie, integrations use app tokens - so the
benchmark was measuring bcrypt and charging it to the application. The true
floor is ~240-290ms.
The harness now warns when NC_AUTH looks like a password, and the spec makes
token auth a precondition of the measurement with its own scenario.
Worth stating plainly: I disabled apps to derive a per-app cost, checked
opcache, checked APCu, and measured a slope - all real work, all answering the
wrong question, because I never questioned the instrument. A floor assumed to
be structural deserves the same attribution discipline as the code under test.
* fix(deps): pull ddn/sapp from GitHub, not Codeberg
The code moved back to GitHub but this composer VCS repository did not, so
every 'composer install' in CI still cloned from codeberg.org. When Codeberg
returned 504s today, openregister's Newman suite failed in 'Install composer
deps' on a dependency fetch — a full CI outage caused by a host we no longer
publish to.
ConductionNL/sapp on GitHub carries the same history: the pinned commit
5c406e91254d6936f44372db35f1cc15e5a06c56 and its branch
feat/chained-filter-text-replace both resolve there. The lock now references
the identical commit via GitHub, and no other package moved.
Verified with a cold, unauthenticated 'composer install' against an empty
cache: ddn/sapp downloads and extracts from GitHub with no Codeberg contact.
Note for whoever picks this up: the .github submodule's origin is still
https://codeberg.org/Conduction/.github.git even though ConductionNL/.github
exists on GitHub. That is why the JIT change had to be raised as
Conduction/.github#75 on Codeberg rather than a GitHub PR.
* docs(perf): plan to bring object writes to the instance floor
object-write-sub-500ms took a create from 13,688ms to 322ms median / 476ms p95.
The 500ms budget is met and is no longer the binding constraint: the instance
floor — an authenticated request doing no object work — is 172-213ms, so an
absolute wall budget mostly measures how many apps are installed.
This change targets the write path costing <=50ms above that floor, and the
budget in the spec becomes floor-relative for the same reason.
What the remaining cost is, from full PostgreSQL statement logging of one
create scoped to its backend and time window (326 statements, 176.8ms):
57 x 44.7ms SELECT * FROM oc_openregister_schemas
WHERE uuid=? OR LOWER(slug)=? OR id=?
24 x 3.2ms register lookups, same shape
18 x 0.8ms SELECT lastval()
9 x 4.7ms SELECT 1 FROM information_schema.tables
-- 2 audit rows + hash-chain UPDATE, 2 notification rows
~135 committed transactions where there should be 1
The schema query seq-scans by construction: SELECT * hydrates a ~2KB properties
blob and LOWER(slug) cannot use an index (Rows Removed by Filter: 1916 of 1929).
19 tasks in 5 phases, ordered by measured payoff: identity map for schemas and
registers, cheap miss path, stop probing information_schema, one transaction,
finish the deferral set, then delete the 2,728-branch fan-out via a
uuid->(register,schema) index rather than optimise it.
Phase 4 covers per-request work outside the write, in scope because the write is
measured against the floor (ADR-076):
pipelinq iterates the 3.4MB appstore catalogue on EVERY request
(resolveDependencyStatuses -> buildAppStoreLookup -> AppFetcher::get). It is
free here ONLY because has_internet_connection=false returns an empty set. It
also computes provideInitialState() on API requests that render no UI.
openconnector invokes a repair step from boot(). Correctly gated AND persists
its key, so free today — and one cleared config key from a repair step per
request. ADR-076 rule 4 puts that fallback in a TimedJob.
31 cron jobs, 8 at 60s. An idle instance does 18 schema seq scans and 356
commits per 4 seconds: the noise floor every measurement here fights.
Two measurement hazards written into the tasks because both cost me real time
this session: benchmarking with the account password adds ~600ms of bcrypt per
request (app token: 456ms median vs 1,058ms), and pg_stat_* counters are
database-global so cron pollutes them — the statement-log method is
authoritative.
Task 8 is flagged as a product decision, not a performance one: whether
deferral becomes default depends on what executionMode:sync promises.
* docs(perf): re-measure before executing — the target is already met at median
I re-measured before starting on this plan, and the numbers it was written
against are stale. Acting on them would have meant a large refactor for a small
gain.
wall min 220ms median 249ms p95 394ms
instance floor 207ms
WRITE PATH min 13ms median 42ms p95 187ms
The <=50ms target is met at median (42ms). p95 is over, but the host was at load
4.6-6.0 from unrelated work — noise, not code.
What moved, from statement logs of one create before and after the DocuDesk
fixes landed:
before now
statements 326 251
schema lookups 57 (44.7ms) 15 (11.3ms)
register lookups 24 (3.2ms) 24 (3.5ms)
information_schema 9 (4.7ms) 9 (3.5ms)
SELECT lastval() 18 9
Phase 1 is therefore worth ~11ms, not ~45ms. Tasks 1-4 are DEFERRED, not
cancelled — they earn their keep again if the schema count grows or a caller
reintroduces a hot loop, and the reasoning stays in the file so nobody has to
rediscover it.
Also recording a hypothesis of mine that did NOT hold, because it looked
compelling: the re-measurement showed the non-lazy appconfig load at 80.6ms,
42% of all DB time, and app_versions stores 9.1MB across 86 non-lazy keys
(appstore.payload.*, up to 3.2MB for mail). Marking them lazy made creates
MARGINALLY SLOWER (275 -> 299ms median) because memcache.local is APCu and the
config is cached across requests — the 80ms was a cold-cache event, once per PHP
worker, not per request. Reverted. Still worth doing as hygiene; not a
performance task.
Revised priority: p95 stability first (establish whether it moves at all on an
unloaded host before calling it a code problem), then the phase-4 latent items
(pipelinq's appstore walk is free ONLY because has_internet_connection=false;
openconnector is one config key from a repair step per request; the 60s cron
fleet is the noise floor), then task 6 (one transaction) on correctness grounds
rather than latency.
* test(pdf): commit the PDF fixture instead of reading a dependency's examples/
PdfExtractorTest read vendor/ddn/sapp/examples/testdoc.pdf. That path only ever
resolved because composer happened to install ddn/sapp from SOURCE: the package
declares
/examples export-ignore
in its .gitattributes, so every distribution archive omits the directory. A git
clone keeps it; a zipball does not.
Switching the package to its GitHub dist (7ac9c92, to get Codeberg off the CI
critical path after an outage took the suite down) made that latent dependency
visible as exactly one failing assertion out of 15,504 tests:
Failed asserting that file ".../vendor/ddn/sapp/examples/testdoc.pdf" exists.
So the regression was mine, and the underlying fault is older: a test must not
depend on a dependency's examples directory, because the dependency has
explicitly declared it not part of what it ships. The fixture is now committed
at tests/fixtures/pdf/testdoc.pdf (51,269 bytes, byte-identical) and the test is
independent of how composer chooses to install anything.
Verified: 3 tests, 7 assertions, green.
* perf(crud): measure read/search/update/delete — this REVERSES the Phase 1 deferral
I had only ever measured creates, and generalised from that to defer the schema
identity map as "worth ~11ms". Measuring the rest of the object API shows that
was the create-only figure and the wrong call for everything else.
tests/perf/object-crud.sh covers create/read/search/update/delete, reports every
figure both as wall time and as wall minus the instance floor measured in the
same run, and prints host load so a bad sample is visible.
Wall (5 runs each, host load 61.7 from unrelated work — the ABSOLUTE numbers are
badly inflated, the RATIOS are the finding):
create 1,477ms 525ms above floor
read 1,447ms 495ms above floor
search 659ms 0ms above floor <- never leaves the floor
update 9,080ms 8,128ms above floor <- 15x a create
delete 4,243ms 3,291ms above floor <- 6x a create
Statement counts, which do NOT inflate with load (statement log scoped to the
request's backend and time window):
statements DB time
create 251 194ms
update 716 2,672ms
delete 595 1,781ms
Same shape in both slow paths — it is repeated resolution, not the write:
update delete create
register lookups 66 64 24
schema lookups (uuid OR slug OR id) 51 51 15
schema lookups (slug + id IN) 42 42 9
information_schema.tables probes 27 27 9
getLiveMagicTables() (all 2,728) 12 - ~3
So tasks 1-5 move back to the top. Task 3 (the REGISTER map) now matters more
than task 1, because registers are re-resolved more often than schemas.
Search being free is worth noting too: whatever the list path does, it is
already right.
The lesson is mine to own — I measured one operation and generalised. The
budget in the spec is per-operation for a reason.
* perf(magic): memoise the magic-table enumeration per request
getLiveMagicTables() lists EVERY magic table from information_schema and then
fetches the full register and schema id lists to discard orphans. On this
instance that is 2,728 tables, ~60ms a call.
Measured 2026-07-30 (statement log, scoped to the request): an object UPDATE
called it 12 TIMES. The answer cannot change mid-request unless this request
creates a table, which is handled below.
getLiveMagicTables enumerations per update: 12 -> 3
Not 1, because several MagicMapper instances participate in one update; the memo
is per-instance. Getting to 1 needs the instances shared, which is a DI change
and out of scope here.
Also memoises checkTableExistsInDatabase(), but ONLY POSITIVE answers. A
negative can legitimately become positive within the request —
ensureTableExists() creates a table and then writes to it — and caching "no"
would break that write against a table that now exists. Creating a table
invalidates both memos via invalidateTableMemos().
HONEST LIMITATION: this did NOT reduce the 27 information_schema existence
probes an update issues; that count is unchanged, so those come from a third
path (most likely Doctrine's IDBConnection::tableExists() through another
caller, not through this method). RegisterService::magicTableExists() already
documents the same class of bug costing 76 SECONDS on a stats endpoint, so the
pattern is known and solved in one place and not others. Tracked as task 5 of
object-write-at-instance-floor; finding the third caller is the next step.
Verified: 15,509 tests / 34,667 assertions green. phpcs clean.
* docs(perf): the statement counts were measured on a POOLED connection — caveat them
The per-operation statement counts (create 251, update 716, delete 595) came
from taking every statement on the request's PostgreSQL backend within a time
window. A backend is a pooled connection serving consecutive requests, so the
window sweeps in unrelated traffic. Tight enough for a ~500ms create; useless
for an update that took 30s under host load 21-62 — 24 distinct backends issued
probes during that capture.
This also retracts the inference I drew from it: '27 information_schema probes
per update, each table probed 3x, therefore 3 MagicMapper instances'. The 3x is
far more likely 3 REQUESTS reusing one pooled connection.
What stands: the wall-clock ratios (update 15x a create, delete 6x, search free)
time individual HTTP requests and are unaffected by pooling, and the direction of
the finding — update/delete do far more repeated resolution than a create — is
visible regardless of the multiplier.
What does not: the counts themselves, and the breakdowns derived from them.
Correct method for next time: bracket on a marker the request emits in its own
SQL, add a per-request id to log_line_prefix, or count from inside PHP where
'this request' is unambiguous.
* perf(probe): count per-request from inside PHP — supersedes the pooled-log counts
Adds WritePhaseProbe::count() and instruments the three lookups the CRUD work
implicated. Counting inside the request makes "this request" unambiguous, which
the PostgreSQL statement log cannot: a backend is a pooled connection serving
consecutive requests, so bracketing by wall-clock sweeps in unrelated traffic.
The real numbers, and they are much lower than the log suggested:
schema reads tableExists full enumerations
create 6 3 0
update 13 7 1
delete 12 7 1
Against the log-derived figures I published (create 15 / update 51 schema
lookups, 27 probes), these are 2-4x smaller. Update and delete do roughly TWICE
a create's schema reads and each performs one full 2,728-table enumeration a
create does not — a real gap, and a far more modest one than I reported.
Note this also means the wall-clock ratios (update 15x a create, delete 6x) are
NOT explained by round-trip counts alone. Those ratios came from timing HTTP
requests and stand; the gap between them and these counts points at PHP-side
work, which is the next thing to measure rather than assume.
The magic-table memoisation holds enumerations at 1 per request — the floor
without sharing mapper instances, and better than the "12 -> 3" I claimed from
the log.
Limitation: read and search produce no counts, because flush() is only reached
from the write path. Instrumenting the read path is open.
* style(probe): restore stamp()'s docblock, which my count() insertion orphaned
Inserting count() above stamp() left stamp()'s docblock attached to count() and
stamp() with none — the third time this session that anchoring an insertion on a
function SIGNATURE rather than on the docblock above it produced exactly this.
Worth remembering: anchor on the docblock opener, not the signature.
* feat(flow): live-runs read for the shared "running flows" widget
Gives every app one honest answer to "which flows are running right now",
scoped to the caller's organisation, so a dashboard widget can show it
without each app building the same surface again.
GET /api/flow-runs/active returns the NON-TERMINAL runs — queued, running
and suspended, defined once as FlowRun::ACTIVE. Filtering to literally
`running` would be empty almost always: a run holds that status only for
the duration of a worker pass, while queued and suspended are where a live
run actually waits.
Three things had to change for that read to exist:
- `organisation` is now STAMPED on queue(). The column has existed since
the table was created and nothing ever wrote to it, so no tenant filter
was possible at all. Resolution is lazy through the container: the cron
worker builds this service on every pass and must not drag the RBAC graph
in to fill a column it usually cannot fill. A run queued with no session
is recorded unattributed rather than guessed at.
- Scoping is STRICT. A run with no organisation goes to nobody. This feeds a
widget every app renders to every user; attributing an unattributed run to
the reader's tenant would put one tenant's activity on another's dashboard.
- The rows are SUMMARISED — uuid, flow id AND resolved flow NAME, status,
trigger, who started it, subject, current step, timestamps. Not the
marking, not the items (which can hold the subject's own record data), not
the step log: kilobytes per run a list never renders. The single-run
endpoint stays the place to ask for a run's contents.
`GET /api/flow-runs` is deliberately unchanged. It is the history surface
with existing e2e coverage; a separate endpoint is what lets the tenant
boundary be strict here without changing what existing callers see.
Also indexes (organisation, status, id). Measured before the index on a dev
instance with 48,058 runs: the planner walked the primary key backwards and
filtered, reading 48,048 rows to return 1 (294ms in postgres, ~21s over
HTTP) — on a surface a widget polls every 15 seconds.
Tests: 32 green (5 new — no organisation reads nothing and never queries the
store, scoping passes the caller's org through, rows carry the resolved name
and step, an unresolvable flow falls back to its id, the row limit is
capped). phpcs / phpmd / phpstan / psalm clean on the changed files; the two
pre-existing StaticAccess findings and TooManyFields on the entity are now
carried as reasoned suppressions rather than left failing.
* feat(icons): adopt the ADR-077 semantic icon vocabulary
Menu icons across the fleet had drifted into meaninglessness: a scan of 21
manifest-shipping apps found 120 distinct icons for 262 distinct labels, with
one glyph standing for as many as 18 unrelated concepts
(`icon-category-monitoring`) and the same concept drawn differently per app —
Store was `icon-category-integration` in one app and
`icon-category-organization` in another.
Moves this app's menu onto the shared vocabulary: MDI PascalCase names, one
concept to one icon. Tier A entries (Dashboard, Documentation, Settings, Store,
Features & roadmap) now match every other Conduction app, which is the whole
point — a glyph should mean the same thing wherever a user meets it.
Two defect classes are fixed along the way:
* Icon names that do not exist in vue-material-design-icons at all. They could
never resolve — rendering a help-circle at best, nothing at all in the
navigation.
* Menu entries that rendered with NO icon, because CnAppNav resolves an MDI name
only through the registry `registerIcons()` populates, with no fallback for a
name the app never registered. Apps that relied on legacy `icon-*` classes
registered nothing at all and were fine until the first MDI name appeared.
src/icons.js is generated from the app's own manifests and register files, so
every name the app references is registered and the migration stands on its own
against the CURRENTLY RELEASED @conduction/nextcloud-vue — it does not wait on
the library-side vocabulary (ConductionNL/nextcloud-vue#563).
Verified: 0 menu entries render without an icon (was 51 fleet-wide), every icon
import resolves against the app's own node_modules, and hydra's gate-60
icon-vocabulary check passes with no failures or warnings.
Spec: ADR-077 (ConductionNL/hydra#408).
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… ~458ms (#2207) * wip: preserve uncommitted working tree (AppHost settings plane, flow wiring, schema dedup) Snapshot of the local working tree taken before reconciling with origin/development, which had moved 69 commits ahead. Much of this tree is already upstream (27 of 36 new PHP files and 13 of 49 modified files are byte-identical to origin/development). The genuinely new work preserved here is: - lib/AppHost/{Controller,Service}: generic settings plane (GenericSettingsControllerBase, GenericSettingsService, RegisterConfigResolver) - lib/Command/DedupCollidedSchemasCommand.php - unit tests for the above plus HandlesExceptionsTrait - four openspec changes (apphost-settings-plane, apphost-schedule-flow-action, app-declared-credential-providers, or-flow-object-write-node) - lib/Settings/flow_register.json and flow e2e coverage Committed on a stale base on purpose so the subsequent merge of origin/development is a real three-way merge rather than a tree overwrite. * docs(openspec): audit-seal-backlog-repair — drain the unsealed audit backlog Measured on the shared dev instance: 108,151 of 227,063 audit rows (48%) carry no hash, interleaved across the whole id range (31,518..290,493 = min..max id). Three defects, all evidenced: 1. insertHashChained() seals per row under a global exclusive advisory lock (3 attempts x 50ms, fail-soft). Under any concurrency each insert pays up to 150ms and then abandons the seal anyway. Measured ~152 rows/min during the maintenance:repair that had to be killed after 74 minutes with ~12h left. The batched sealRows() path already exists but single inserts never reach it. 2. The backfill that specs/audit-hash-chain/spec.md:105 requires does not exist. harden-audit-seal-concurrency (12/12 complete) made the lock fail-soft on the explicit promise that a "later seal pass chains them" -- that pass was never built, so every contended write permanently degrades the chain. 3. verifyChain() skips ANY null hash and still returns valid: true. The comment claims "pre-migration entries" but no cutover marker exists in lib/, so the tamper-evidence check currently passes over a table that is 48% unverified. The change specifies a windowed driver around sealRows() (one lock per window, not per row), a two-phase read so sealed rows do not have their ~5.3KB payloads fetched just to contribute a chain link, a partial index for the backlog cursor (today it pkey-scans with a filter at 784ms/2000 ids), a hard-capped background job plus an occ command, and a cutover marker so unsealed rows stop hiding behind a passing verification. Design records why a naive bulk call is impossible: sealRowsLocked() SELECTs * over [min,max], which for this backlog is ~227k rows x 5,270B ~= 1.14GB in PHP. * docs(perf): plan to bring object writes under 500ms + fix features.json drift A single-object create currently takes 13-99s on the dev instance (six runs on larpingapp/character, two-field payload each time: 13.6/17.8/20.4/41.0/62.8/99.1s). This is NOT the CloudEvent storm — that was openconnector's inert recursion guard, fixed separately, and a create now emits 1 event rather than 255. The remainder is our own write path. Counter deltas across one HTTP 201 create (the 41.0s run): sequential scans of oc_openregister_schemas 5,135 sequential scans of oc_openregister_registers 6 transactions committed 12,541 Four measured costs: 1. 5,135 schema resolutions against a 1,917-row table. SchemaMapper::find() HAS a request cache and is a shared service, so this is either a cache key too specific (rbac/multitenancy flags multiply it 4x) or an uncached sibling on the hot path. The query is a seq scan by construction — SELECT * hydrates a ~2KB properties blob and LOWER(slug) defeats any index ('Rows Removed by Filter: 1916'). ~4ms x 5,135 = ~20s, half the run. 2. Resolving an object reference whose table is unknown emits a UNION ALL with one branch per magic table. At 2,728 tables that is 690KB of SQL: planning 3,404.9ms, execution 546.1ms. 86% of the cost is PARSING a statement that returns zero rows, so no index can help — and it is usually avoidable, since character's six relation properties each already declare their target schema. 3. 12,541 commits for one create: essentially every statement autocommits, and the request waits on each fsync. 4. CloudEvent fan-out, audit-trail sealing (228,932 rows), notification history and oc_activity all run before the response is returned. Target p95 <500ms with the 2,728-table shape unchanged — fixing the write path, not shrinking the dataset. Task 1 is deliberately 'attribute the 5,135 calls before changing anything': guessing at a hot path is how the CloudEvent guard stayed inert for so long. Also fixes an unrelated red gate: openspec/specs/saved-search-views/spec.md was missing the blank line before '## Requirements', so the features-manifest generator swallowed the heading into the feature summary and docs/features.json drifted. 'quality / Features Check' fails on development because of it. * style(dedup): satisfy phpcs on the schema-dedup command Conduction's standard requires named arguments on internal calls, forbids inline IFs, and has its own spacing///end conventions — the command shipped with 21 violations and turned 'quality / PHP Quality (phpcs)' red. 14 fixed by phpcbf; the rest by hand: setName/setDescription/addOption -> named arguments (four calls) splitOne/splitOneLocked/findCollisions/pickOwner call sites -> named arguments the two ternaries in execute() -> explicit if blocks All 11 PHP files changed on this branch now pass phpcs. * ci: regenerate docs/features.json from openspec/specs/ [skip ci] * style(phpcs): clear the 15 pre-existing violations that kept phpcs red lib/ was 15 errors over 9 files before this branch, so 'quality / PHP Quality (phpcs)' failed on development and on every PR opened against it. 6x curl_close($ch) deprecated since PHP 8.0 and a no-op — CurlHandle is an object freed when it leaves scope, not a resource needing an explicit close. Removed, with a comment so nobody re-adds them. 3x missing @PARAM FlowRunController::__construct($userSession), FederatedConfigService::publish($private), FlowScheduleService::fire($owner) 6x file header ReconcileDeclaredBackgroundJobs.php put its docblock AFTER declare(strict_types=1), so phpcs read it as a stray inline block rather than the file header; tag order was @author/@license/@copyright. Moved above the declare and reordered to @author/@copyright/@license. phpcs now reports 0 errors across all 72 files in lib/. phpstan is unchanged — the same 7 findings exist with and without this commit (verified by stashing), so nothing here introduced or masked one. * perf(objects): scope the cross-schema fallback to the caller's register ObjectService::find() takes register+schema, does a scoped lookup, and on a miss retries. The retry dropped BOTH the register and the schema, so a legitimate 'not in this register' answer was produced by scanning every magic table on the instance. That scan is a UNION with one branch per magic table. At 2,728 tables it is 690 KB of SQL, and its cost is almost entirely PLANNING: Planning Time: 3404.926 ms Execution Time: 546.145 ms <- returns zero rows No index can reduce parse time, so the only fix is to not emit it. The fallback exists for a stale or sibling SCHEMA inside a register the caller named correctly (openbuild#75 / openregister#1520) — it was never meant to search other registers. It now keeps the register and drops only the schema, and MagicMapper::find()/findAcrossAllSources() accept a registerIdScope that filters candidate tables by register id. Why this mattered on the write path: the flow-resolver registry asks every resolver in turn whether a flow is theirs. Each non-owning resolver called find(register: <its own>, schema: 'flow'), missed correctly, and paid a full instance-wide scan to say 'not mine'. Measured 2026-07-29, that single widened fallback was ~1.9s of a ~3.0s create. event dispatch inside the create 1,900ms -> 80-187ms wall (median of 5) 3.2s -> 1.55s Also adds the measurement half of the change: - tests/perf/object-create.sh reports min/median/p95 plus per-write schema seq scans and commit count, and separately measures the INSTANCE FLOOR (an authenticated request doing no object work). Nextcloud boots every enabled app per request; on this instance, with 92 apps, that floor is ~864ms — larger than the whole 500ms budget — so wall time alone cannot tell you whether the write path regressed. - SchemaMapper::traceRead() attributes every uncached schema read to its caller (gated on the perf_trace_schema_reads app-config flag); this is what identified the 1,471-call DocuDesk path. - WritePhaseProbe times the write path's phases; this is what showed the cost was event dispatch rather than the INSERT (76ms). Current state: wall p95 1,421ms = 864ms instance floor + 557ms write path. Refs openspec/changes/object-write-sub-500ms tasks 0, 1, 5. * perf(flow): memoise flow resolution for the request FlowResolverRegistry::resolveFlow() asks every registered resolver in turn. A resolver that does not own the flow answers by looking the id up in its own register — so an unowned flow pays the entire chain, and every answer costs a database round trip. FlowTriggerService::fire() calls it once per queued run, and it calls it from runInline() BEFORE checking whether the flow is even synchronous, so an async flow pays a full resolution for nothing. A save that cascades fires the same triggers again for each child, so one object write resolved the same ids several times over. Memoised per request, misses included: 'no app owns this flow' is the expensive answer, since producing it requires every resolver to look and fail. Refs openspec/changes/object-write-sub-500ms. * perf(objects): optionally defer the created-event dispatch out of the write ObjectCreatedEvent has ~22 listeners across the fleet. Dispatched inline the caller waits for all of them: measured 2026-07-29 it was 234-501ms of a ~530ms create, against a 76ms insert. None of it is needed to tell the caller its object was saved. Opt-in via 'occ config:app:set openregister defer_object_events --value=1'. Off by default, because deferring changes an observable contract: a 2xx stops meaning 'and every side effect has been applied'. A flow declaring executionMode: sync exists precisely so its effects land before the save returns, so this cannot be flipped on for everyone by fiat. event dispatch inside the create 234-501ms -> 6-8ms write path p95 (wall minus floor) 557ms -> 300ms CARRIES THE ACTING USER, and that is not incidental. The first version did not, and it was a perfectly green no-op: the job ran, logged nothing, threw nothing — and produced ZERO CloudEvents where the inline path produced one. A background job has no session, and OpenRegister reads are organisation- filtered against the session user, so every listener that consults the register (the CloudEvent firehose gate most visibly) saw an empty instance and skipped. Deferring side effects without carrying identity does not move the work, it deletes it. Verified after the fix: deferred dispatch produces exactly 1 CloudEvent, matching inline. The impersonation is released in a finally so it cannot leak into whatever job the worker runs next from the same process, and a missing acting user is logged at WARNING (not INFO — the default loglevel is 2, and a side effect that silently vanished is exactly what must not be filtered out). * docs(perf): record measured results and correct the budget's definition Two corrections the measurements forced, both of which change what the requirement can mean: 1. The budget must be wall time MINUS THE INSTANCE FLOOR. An authenticated request doing no object work costs 864-1,099ms on this instance, because Nextcloud boots all 92 enabled apps per request. That is larger than the whole 500ms budget, it is PHP-side (boot issues almost no queries, and opcache is healthy: 0 OOM restarts, 91% hit rate), and no work in the write path can remove it. A measurement that does not subtract it reports how many apps are installed. 2. Deferred dispatch must carry the acting user. Added as an explicit requirement and scenario after the first implementation shipped a flawless no-op — zero CloudEvents, no exception, no log. Results recorded: write path 12.8s -> 183ms median / 300ms p95, inside budget. Wall 13.7s -> 1.28s. Task 5 is ticked but was solved by a DIFFERENT mechanism than specified — the fan-out was reached through the cross-schema fallback, not through untyped relation properties — so the originally-specified work is explicitly folded into task 6 rather than quietly counted as done. * perf(probe): attribute the request timeline, not just the write Adds WritePhaseProbe::stamp() — absolute offsets from REQUEST_TIME_FLOAT, distinct from mark()'s durations, because the question is 'how much of the request had already elapsed before our code ran at all'. Stamped at OpenRegister's register(), its boot() entry and exit, the create controller, and the end of the write. One create, wall 1,464ms: or.register.in 122ms NC core + apps registering before us or.boot.in 903ms +781ms of OTHER APPS registering or.boot.out 928ms our own boot: 25ms ctrl.create.in 964ms +36ms routing and middleware flush 1,357ms +393ms the actual write So 964ms of a 1,464ms request elapses before the controller is entered, and 781ms of that is apps registering. Bracketed directly on the same instance with the same auth: status.php (no app boot) 47ms capabilities (full app boot) 970ms ~920ms is booting 92 enabled apps, ~10ms each. 24 of them are Conduction fleet apps; a deployment running OpenRegister plus a handful of leaves boots a fraction of that. This is why the 500ms budget is specified against the write path with the instance floor subtracted: no change to the write path can move the 920ms, and a wall-clock measurement of it is a measurement of the app count. Off unless /tmp/or-trace-write-phases exists, checked once per process. * perf(probe): stamp register() exit — proves the boot cost is not ours Adds or.register.out, closing the one gap in the request timeline that still allowed 'maybe OpenRegister's own registration is the pig'. It is not: or.register.in 235ms or.register.out 235ms <- our register() is 0ms or.boot.in 1,355ms <- +1,120ms of other apps + NC phases or.boot.out 1,376ms <- our boot() is 21ms ctrl.create.in 1,451ms OpenRegister has 268 registerService/registerEventListener calls across a 4,248-line Application.php and they cost nothing measurable, because they are lazy closures — which is the point of the API and worth having proof of. So the per-request app-boot cost (~16ms/app across 92 apps, measured by disabling 8 and restoring them) is Nextcloud's own registration and boot machinery plus the other 91 apps, and no change inside this app can reduce it. * chore: retrigger CI (previous run was cancelled by a duplicate dispatch) * perf: creates are under 500ms end-to-end — p95 469ms BUDGET MET. Measured on larpingapp/character with tests/perf/object-create.sh: wall p95 13,700ms -> 469ms wall median 13,688ms -> 385ms write path 12,800ms -> 93ms median / 177ms p95 Requires an app token, defer_object_events=1, and PHP JIT disabled (Conduction/.github#75). With deferral off the p95 is 650ms. CORRECTS MY OWN EARLIER ANALYSIS. This change previously documented an 'instance floor' of 864-1,099ms attributed to Nextcloud booting 92 apps, and concluded wall-clock under 500ms was unreachable without disabling apps. That was an artefact of the benchmark, not a property of Nextcloud. Every sample authenticated with HTTP Basic auth carrying the ACCOUNT PASSWORD, which Nextcloud bcrypt-verifies on every request. Same endpoint, same instance, back to back: account password median 1,058ms app token median 456ms ~600ms per request was password hashing. No real client authenticates that way - browsers carry a session cookie, integrations use app tokens - so the benchmark was measuring bcrypt and charging it to the application. The true floor is ~240-290ms. The harness now warns when NC_AUTH looks like a password, and the spec makes token auth a precondition of the measurement with its own scenario. Worth stating plainly: I disabled apps to derive a per-app cost, checked opcache, checked APCu, and measured a slope - all real work, all answering the wrong question, because I never questioned the instrument. A floor assumed to be structural deserves the same attribution discipline as the code under test. * fix(deps): pull ddn/sapp from GitHub, not Codeberg The code moved back to GitHub but this composer VCS repository did not, so every 'composer install' in CI still cloned from codeberg.org. When Codeberg returned 504s today, openregister's Newman suite failed in 'Install composer deps' on a dependency fetch — a full CI outage caused by a host we no longer publish to. ConductionNL/sapp on GitHub carries the same history: the pinned commit 5c406e91254d6936f44372db35f1cc15e5a06c56 and its branch feat/chained-filter-text-replace both resolve there. The lock now references the identical commit via GitHub, and no other package moved. Verified with a cold, unauthenticated 'composer install' against an empty cache: ddn/sapp downloads and extracts from GitHub with no Codeberg contact. Note for whoever picks this up: the .github submodule's origin is still https://codeberg.org/Conduction/.github.git even though ConductionNL/.github exists on GitHub. That is why the JIT change had to be raised as Conduction/.github#75 on Codeberg rather than a GitHub PR. * docs(perf): plan to bring object writes to the instance floor object-write-sub-500ms took a create from 13,688ms to 322ms median / 476ms p95. The 500ms budget is met and is no longer the binding constraint: the instance floor — an authenticated request doing no object work — is 172-213ms, so an absolute wall budget mostly measures how many apps are installed. This change targets the write path costing <=50ms above that floor, and the budget in the spec becomes floor-relative for the same reason. What the remaining cost is, from full PostgreSQL statement logging of one create scoped to its backend and time window (326 statements, 176.8ms): 57 x 44.7ms SELECT * FROM oc_openregister_schemas WHERE uuid=? OR LOWER(slug)=? OR id=? 24 x 3.2ms register lookups, same shape 18 x 0.8ms SELECT lastval() 9 x 4.7ms SELECT 1 FROM information_schema.tables -- 2 audit rows + hash-chain UPDATE, 2 notification rows ~135 committed transactions where there should be 1 The schema query seq-scans by construction: SELECT * hydrates a ~2KB properties blob and LOWER(slug) cannot use an index (Rows Removed by Filter: 1916 of 1929). 19 tasks in 5 phases, ordered by measured payoff: identity map for schemas and registers, cheap miss path, stop probing information_schema, one transaction, finish the deferral set, then delete the 2,728-branch fan-out via a uuid->(register,schema) index rather than optimise it. Phase 4 covers per-request work outside the write, in scope because the write is measured against the floor (ADR-076): pipelinq iterates the 3.4MB appstore catalogue on EVERY request (resolveDependencyStatuses -> buildAppStoreLookup -> AppFetcher::get). It is free here ONLY because has_internet_connection=false returns an empty set. It also computes provideInitialState() on API requests that render no UI. openconnector invokes a repair step from boot(). Correctly gated AND persists its key, so free today — and one cleared config key from a repair step per request. ADR-076 rule 4 puts that fallback in a TimedJob. 31 cron jobs, 8 at 60s. An idle instance does 18 schema seq scans and 356 commits per 4 seconds: the noise floor every measurement here fights. Two measurement hazards written into the tasks because both cost me real time this session: benchmarking with the account password adds ~600ms of bcrypt per request (app token: 456ms median vs 1,058ms), and pg_stat_* counters are database-global so cron pollutes them — the statement-log method is authoritative. Task 8 is flagged as a product decision, not a performance one: whether deferral becomes default depends on what executionMode:sync promises. * docs(perf): re-measure before executing — the target is already met at median I re-measured before starting on this plan, and the numbers it was written against are stale. Acting on them would have meant a large refactor for a small gain. wall min 220ms median 249ms p95 394ms instance floor 207ms WRITE PATH min 13ms median 42ms p95 187ms The <=50ms target is met at median (42ms). p95 is over, but the host was at load 4.6-6.0 from unrelated work — noise, not code. What moved, from statement logs of one create before and after the DocuDesk fixes landed: before now statements 326 251 schema lookups 57 (44.7ms) 15 (11.3ms) register lookups 24 (3.2ms) 24 (3.5ms) information_schema 9 (4.7ms) 9 (3.5ms) SELECT lastval() 18 9 Phase 1 is therefore worth ~11ms, not ~45ms. Tasks 1-4 are DEFERRED, not cancelled — they earn their keep again if the schema count grows or a caller reintroduces a hot loop, and the reasoning stays in the file so nobody has to rediscover it. Also recording a hypothesis of mine that did NOT hold, because it looked compelling: the re-measurement showed the non-lazy appconfig load at 80.6ms, 42% of all DB time, and app_versions stores 9.1MB across 86 non-lazy keys (appstore.payload.*, up to 3.2MB for mail). Marking them lazy made creates MARGINALLY SLOWER (275 -> 299ms median) because memcache.local is APCu and the config is cached across requests — the 80ms was a cold-cache event, once per PHP worker, not per request. Reverted. Still worth doing as hygiene; not a performance task. Revised priority: p95 stability first (establish whether it moves at all on an unloaded host before calling it a code problem), then the phase-4 latent items (pipelinq's appstore walk is free ONLY because has_internet_connection=false; openconnector is one config key from a repair step per request; the 60s cron fleet is the noise floor), then task 6 (one transaction) on correctness grounds rather than latency. * test(pdf): commit the PDF fixture instead of reading a dependency's examples/ PdfExtractorTest read vendor/ddn/sapp/examples/testdoc.pdf. That path only ever resolved because composer happened to install ddn/sapp from SOURCE: the package declares /examples export-ignore in its .gitattributes, so every distribution archive omits the directory. A git clone keeps it; a zipball does not. Switching the package to its GitHub dist (7ac9c92, to get Codeberg off the CI critical path after an outage took the suite down) made that latent dependency visible as exactly one failing assertion out of 15,504 tests: Failed asserting that file ".../vendor/ddn/sapp/examples/testdoc.pdf" exists. So the regression was mine, and the underlying fault is older: a test must not depend on a dependency's examples directory, because the dependency has explicitly declared it not part of what it ships. The fixture is now committed at tests/fixtures/pdf/testdoc.pdf (51,269 bytes, byte-identical) and the test is independent of how composer chooses to install anything. Verified: 3 tests, 7 assertions, green. * perf(crud): measure read/search/update/delete — this REVERSES the Phase 1 deferral I had only ever measured creates, and generalised from that to defer the schema identity map as "worth ~11ms". Measuring the rest of the object API shows that was the create-only figure and the wrong call for everything else. tests/perf/object-crud.sh covers create/read/search/update/delete, reports every figure both as wall time and as wall minus the instance floor measured in the same run, and prints host load so a bad sample is visible. Wall (5 runs each, host load 61.7 from unrelated work — the ABSOLUTE numbers are badly inflated, the RATIOS are the finding): create 1,477ms 525ms above floor read 1,447ms 495ms above floor search 659ms 0ms above floor <- never leaves the floor update 9,080ms 8,128ms above floor <- 15x a create delete 4,243ms 3,291ms above floor <- 6x a create Statement counts, which do NOT inflate with load (statement log scoped to the request's backend and time window): statements DB time create 251 194ms update 716 2,672ms delete 595 1,781ms Same shape in both slow paths — it is repeated resolution, not the write: update delete create register lookups 66 64 24 schema lookups (uuid OR slug OR id) 51 51 15 schema lookups (slug + id IN) 42 42 9 information_schema.tables probes 27 27 9 getLiveMagicTables() (all 2,728) 12 - ~3 So tasks 1-5 move back to the top. Task 3 (the REGISTER map) now matters more than task 1, because registers are re-resolved more often than schemas. Search being free is worth noting too: whatever the list path does, it is already right. The lesson is mine to own — I measured one operation and generalised. The budget in the spec is per-operation for a reason. * perf(magic): memoise the magic-table enumeration per request getLiveMagicTables() lists EVERY magic table from information_schema and then fetches the full register and schema id lists to discard orphans. On this instance that is 2,728 tables, ~60ms a call. Measured 2026-07-30 (statement log, scoped to the request): an object UPDATE called it 12 TIMES. The answer cannot change mid-request unless this request creates a table, which is handled below. getLiveMagicTables enumerations per update: 12 -> 3 Not 1, because several MagicMapper instances participate in one update; the memo is per-instance. Getting to 1 needs the instances shared, which is a DI change and out of scope here. Also memoises checkTableExistsInDatabase(), but ONLY POSITIVE answers. A negative can legitimately become positive within the request — ensureTableExists() creates a table and then writes to it — and caching "no" would break that write against a table that now exists. Creating a table invalidates both memos via invalidateTableMemos(). HONEST LIMITATION: this did NOT reduce the 27 information_schema existence probes an update issues; that count is unchanged, so those come from a third path (most likely Doctrine's IDBConnection::tableExists() through another caller, not through this method). RegisterService::magicTableExists() already documents the same class of bug costing 76 SECONDS on a stats endpoint, so the pattern is known and solved in one place and not others. Tracked as task 5 of object-write-at-instance-floor; finding the third caller is the next step. Verified: 15,509 tests / 34,667 assertions green. phpcs clean. * docs(perf): the statement counts were measured on a POOLED connection — caveat them The per-operation statement counts (create 251, update 716, delete 595) came from taking every statement on the request's PostgreSQL backend within a time window. A backend is a pooled connection serving consecutive requests, so the window sweeps in unrelated traffic. Tight enough for a ~500ms create; useless for an update that took 30s under host load 21-62 — 24 distinct backends issued probes during that capture. This also retracts the inference I drew from it: '27 information_schema probes per update, each table probed 3x, therefore 3 MagicMapper instances'. The 3x is far more likely 3 REQUESTS reusing one pooled connection. What stands: the wall-clock ratios (update 15x a create, delete 6x, search free) time individual HTTP requests and are unaffected by pooling, and the direction of the finding — update/delete do far more repeated resolution than a create — is visible regardless of the multiplier. What does not: the counts themselves, and the breakdowns derived from them. Correct method for next time: bracket on a marker the request emits in its own SQL, add a per-request id to log_line_prefix, or count from inside PHP where 'this request' is unambiguous. * perf(probe): count per-request from inside PHP — supersedes the pooled-log counts Adds WritePhaseProbe::count() and instruments the three lookups the CRUD work implicated. Counting inside the request makes "this request" unambiguous, which the PostgreSQL statement log cannot: a backend is a pooled connection serving consecutive requests, so bracketing by wall-clock sweeps in unrelated traffic. The real numbers, and they are much lower than the log suggested: schema reads tableExists full enumerations create 6 3 0 update 13 7 1 delete 12 7 1 Against the log-derived figures I published (create 15 / update 51 schema lookups, 27 probes), these are 2-4x smaller. Update and delete do roughly TWICE a create's schema reads and each performs one full 2,728-table enumeration a create does not — a real gap, and a far more modest one than I reported. Note this also means the wall-clock ratios (update 15x a create, delete 6x) are NOT explained by round-trip counts alone. Those ratios came from timing HTTP requests and stand; the gap between them and these counts points at PHP-side work, which is the next thing to measure rather than assume. The magic-table memoisation holds enumerations at 1 per request — the floor without sharing mapper instances, and better than the "12 -> 3" I claimed from the log. Limitation: read and search produce no counts, because flush() is only reached from the write path. Instrumenting the read path is open. * style(probe): restore stamp()'s docblock, which my count() insertion orphaned Inserting count() above stamp() left stamp()'s docblock attached to count() and stamp() with none — the third time this session that anchoring an insertion on a function SIGNATURE rather than on the docblock above it produced exactly this. Worth remembering: anchor on the docblock opener, not the signature. * feat(flow): live-runs read for the shared "running flows" widget Gives every app one honest answer to "which flows are running right now", scoped to the caller's organisation, so a dashboard widget can show it without each app building the same surface again. GET /api/flow-runs/active returns the NON-TERMINAL runs — queued, running and suspended, defined once as FlowRun::ACTIVE. Filtering to literally `running` would be empty almost always: a run holds that status only for the duration of a worker pass, while queued and suspended are where a live run actually waits. Three things had to change for that read to exist: - `organisation` is now STAMPED on queue(). The column has existed since the table was created and nothing ever wrote to it, so no tenant filter was possible at all. Resolution is lazy through the container: the cron worker builds this service on every pass and must not drag the RBAC graph in to fill a column it usually cannot fill. A run queued with no session is recorded unattributed rather than guessed at. - Scoping is STRICT. A run with no organisation goes to nobody. This feeds a widget every app renders to every user; attributing an unattributed run to the reader's tenant would put one tenant's activity on another's dashboard. - The rows are SUMMARISED — uuid, flow id AND resolved flow NAME, status, trigger, who started it, subject, current step, timestamps. Not the marking, not the items (which can hold the subject's own record data), not the step log: kilobytes per run a list never renders. The single-run endpoint stays the place to ask for a run's contents. `GET /api/flow-runs` is deliberately unchanged. It is the history surface with existing e2e coverage; a separate endpoint is what lets the tenant boundary be strict here without changing what existing callers see. Also indexes (organisation, status, id). Measured before the index on a dev instance with 48,058 runs: the planner walked the primary key backwards and filtered, reading 48,048 rows to return 1 (294ms in postgres, ~21s over HTTP) — on a surface a widget polls every 15 seconds. Tests: 32 green (5 new — no organisation reads nothing and never queries the store, scoping passes the caller's org through, rows carry the resolved name and step, an unresolvable flow falls back to its id, the row limit is capped). phpcs / phpmd / phpstan / psalm clean on the changed files; the two pre-existing StaticAccess findings and TooManyFields on the entity are now carried as reasoned suppressions rather than left failing. * feat(icons): adopt the ADR-077 semantic icon vocabulary Menu icons across the fleet had drifted into meaninglessness: a scan of 21 manifest-shipping apps found 120 distinct icons for 262 distinct labels, with one glyph standing for as many as 18 unrelated concepts (`icon-category-monitoring`) and the same concept drawn differently per app — Store was `icon-category-integration` in one app and `icon-category-organization` in another. Moves this app's menu onto the shared vocabulary: MDI PascalCase names, one concept to one icon. Tier A entries (Dashboard, Documentation, Settings, Store, Features & roadmap) now match every other Conduction app, which is the whole point — a glyph should mean the same thing wherever a user meets it. Two defect classes are fixed along the way: * Icon names that do not exist in vue-material-design-icons at all. They could never resolve — rendering a help-circle at best, nothing at all in the navigation. * Menu entries that rendered with NO icon, because CnAppNav resolves an MDI name only through the registry `registerIcons()` populates, with no fallback for a name the app never registered. Apps that relied on legacy `icon-*` classes registered nothing at all and were fine until the first MDI name appeared. src/icons.js is generated from the app's own manifests and register files, so every name the app references is registered and the migration stands on its own against the CURRENTLY RELEASED @conduction/nextcloud-vue — it does not wait on the library-side vocabulary (ConductionNL/nextcloud-vue#563). Verified: 0 menu entries render without an icon (was 51 fleet-wide), every icon import resolves against the app's own node_modules, and hydra's gate-60 icon-vocabulary check passes with no failures or warnings. Spec: ADR-077 (ConductionNL/hydra#408). * perf(probe): instrument the read paths + completion plan Read paths were unmeasurable. WritePhaseProbe::flush() was only called from the write path AND returned early unless $phases was non-empty — so search and single-read, which produce stamps and counts but no phase marks, emitted nothing at all. Half the object API had never been measured. Three changes: flush() now emits when there is anything to say (phases OR counts OR stamps), not only when phases exist. stamp()/count() arm a one-shot register_shutdown_function on first use, so every path flushes without threading a flush() call through each controller — and a controller added later gets it for free. ObjectsController::index() and ::show() stamp their entry. First results, per request, from inside PHP: SEARCH 240ms wall — or.boot.out=198ms, ctrl.index.in=217ms, flush=228ms => the search itself is ~11ms; the rest is app boot. READ 450ms wall — or.boot.out=185ms, flush=643ms => ~458ms in the read path, ~40x search. HONEST GAP: `ctrl.show.in` never fired, so the single-read route is NOT ObjectsController::show(). The 458ms is bounded but not yet attributed — finding the real route is the next step, and I did not want to report an attribution I had not verified. Also adds COMPLETION-PLAN.md: what remains, ordered, with the blockers named. Phase A (re-baseline on a quiet host) blocks the rest — load ran 4.6-61.7 all session and three conclusions had to be retracted or reversed because of it. Phase E is a licence decision that is not mine: vue3-apexcharts <=1.8.0 is MIT, >=1.9.0 is dual and revenue-gated, CI resolved 1.11.1, and openconnector's License gate is CORRECTLY failing. I did not write an override. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The whole e2e suite was unrunnable locally. global-setup gates every spec, and
it died at:
TimeoutError: page.waitForSelector: Timeout 20000ms exceeded.
- waiting for locator('#header, header.header') to be visible
- locator resolved to visible <header id="header">…</header>
The call log contradicts itself: the locator RESOLVED TO VISIBLE and the wait
still expired. That is a navigation race — the post-login redirect invalidates
the handle mid-check, so waitForSelector keeps retrying a stale one until the
budget runs out. It widens under load, which is why it bit here (host load
14-62) and presumably passes on a quiet CI runner.
Three changes:
The URL check moves FIRST. It is cheaper, needs nothing rendered, and when
login genuinely failed it gives "still on /login" instead of a timeout on an
element that was never going to appear.
expect().toBeVisible() replaces waitForSelector. It re-resolves the locator
every poll, so a redirect costs one poll rather than the whole timeout.
waitForLoadState('domcontentloaded') settles the navigation first. NOT
networkidle, which never fires on Nextcloud because of its notification poll
(ADR-074 rule 4).
Verified: tests/e2e/core-crud.spec.ts, chromium, 7 passed (3.8m) on the same
loaded host that could not get past global setup before. Covers auth,
app-bootstrap and four deep-link routes.
Closes #2208.
# Conflicts: # lib/Service/WritePhaseProbe.php
… suite was unrunnable (#2209) * wip: preserve uncommitted working tree (AppHost settings plane, flow wiring, schema dedup) Snapshot of the local working tree taken before reconciling with origin/development, which had moved 69 commits ahead. Much of this tree is already upstream (27 of 36 new PHP files and 13 of 49 modified files are byte-identical to origin/development). The genuinely new work preserved here is: - lib/AppHost/{Controller,Service}: generic settings plane (GenericSettingsControllerBase, GenericSettingsService, RegisterConfigResolver) - lib/Command/DedupCollidedSchemasCommand.php - unit tests for the above plus HandlesExceptionsTrait - four openspec changes (apphost-settings-plane, apphost-schedule-flow-action, app-declared-credential-providers, or-flow-object-write-node) - lib/Settings/flow_register.json and flow e2e coverage Committed on a stale base on purpose so the subsequent merge of origin/development is a real three-way merge rather than a tree overwrite. * docs(openspec): audit-seal-backlog-repair — drain the unsealed audit backlog Measured on the shared dev instance: 108,151 of 227,063 audit rows (48%) carry no hash, interleaved across the whole id range (31,518..290,493 = min..max id). Three defects, all evidenced: 1. insertHashChained() seals per row under a global exclusive advisory lock (3 attempts x 50ms, fail-soft). Under any concurrency each insert pays up to 150ms and then abandons the seal anyway. Measured ~152 rows/min during the maintenance:repair that had to be killed after 74 minutes with ~12h left. The batched sealRows() path already exists but single inserts never reach it. 2. The backfill that specs/audit-hash-chain/spec.md:105 requires does not exist. harden-audit-seal-concurrency (12/12 complete) made the lock fail-soft on the explicit promise that a "later seal pass chains them" -- that pass was never built, so every contended write permanently degrades the chain. 3. verifyChain() skips ANY null hash and still returns valid: true. The comment claims "pre-migration entries" but no cutover marker exists in lib/, so the tamper-evidence check currently passes over a table that is 48% unverified. The change specifies a windowed driver around sealRows() (one lock per window, not per row), a two-phase read so sealed rows do not have their ~5.3KB payloads fetched just to contribute a chain link, a partial index for the backlog cursor (today it pkey-scans with a filter at 784ms/2000 ids), a hard-capped background job plus an occ command, and a cutover marker so unsealed rows stop hiding behind a passing verification. Design records why a naive bulk call is impossible: sealRowsLocked() SELECTs * over [min,max], which for this backlog is ~227k rows x 5,270B ~= 1.14GB in PHP. * docs(perf): plan to bring object writes under 500ms + fix features.json drift A single-object create currently takes 13-99s on the dev instance (six runs on larpingapp/character, two-field payload each time: 13.6/17.8/20.4/41.0/62.8/99.1s). This is NOT the CloudEvent storm — that was openconnector's inert recursion guard, fixed separately, and a create now emits 1 event rather than 255. The remainder is our own write path. Counter deltas across one HTTP 201 create (the 41.0s run): sequential scans of oc_openregister_schemas 5,135 sequential scans of oc_openregister_registers 6 transactions committed 12,541 Four measured costs: 1. 5,135 schema resolutions against a 1,917-row table. SchemaMapper::find() HAS a request cache and is a shared service, so this is either a cache key too specific (rbac/multitenancy flags multiply it 4x) or an uncached sibling on the hot path. The query is a seq scan by construction — SELECT * hydrates a ~2KB properties blob and LOWER(slug) defeats any index ('Rows Removed by Filter: 1916'). ~4ms x 5,135 = ~20s, half the run. 2. Resolving an object reference whose table is unknown emits a UNION ALL with one branch per magic table. At 2,728 tables that is 690KB of SQL: planning 3,404.9ms, execution 546.1ms. 86% of the cost is PARSING a statement that returns zero rows, so no index can help — and it is usually avoidable, since character's six relation properties each already declare their target schema. 3. 12,541 commits for one create: essentially every statement autocommits, and the request waits on each fsync. 4. CloudEvent fan-out, audit-trail sealing (228,932 rows), notification history and oc_activity all run before the response is returned. Target p95 <500ms with the 2,728-table shape unchanged — fixing the write path, not shrinking the dataset. Task 1 is deliberately 'attribute the 5,135 calls before changing anything': guessing at a hot path is how the CloudEvent guard stayed inert for so long. Also fixes an unrelated red gate: openspec/specs/saved-search-views/spec.md was missing the blank line before '## Requirements', so the features-manifest generator swallowed the heading into the feature summary and docs/features.json drifted. 'quality / Features Check' fails on development because of it. * style(dedup): satisfy phpcs on the schema-dedup command Conduction's standard requires named arguments on internal calls, forbids inline IFs, and has its own spacing///end conventions — the command shipped with 21 violations and turned 'quality / PHP Quality (phpcs)' red. 14 fixed by phpcbf; the rest by hand: setName/setDescription/addOption -> named arguments (four calls) splitOne/splitOneLocked/findCollisions/pickOwner call sites -> named arguments the two ternaries in execute() -> explicit if blocks All 11 PHP files changed on this branch now pass phpcs. * ci: regenerate docs/features.json from openspec/specs/ [skip ci] * style(phpcs): clear the 15 pre-existing violations that kept phpcs red lib/ was 15 errors over 9 files before this branch, so 'quality / PHP Quality (phpcs)' failed on development and on every PR opened against it. 6x curl_close($ch) deprecated since PHP 8.0 and a no-op — CurlHandle is an object freed when it leaves scope, not a resource needing an explicit close. Removed, with a comment so nobody re-adds them. 3x missing @PARAM FlowRunController::__construct($userSession), FederatedConfigService::publish($private), FlowScheduleService::fire($owner) 6x file header ReconcileDeclaredBackgroundJobs.php put its docblock AFTER declare(strict_types=1), so phpcs read it as a stray inline block rather than the file header; tag order was @author/@license/@copyright. Moved above the declare and reordered to @author/@copyright/@license. phpcs now reports 0 errors across all 72 files in lib/. phpstan is unchanged — the same 7 findings exist with and without this commit (verified by stashing), so nothing here introduced or masked one. * perf(objects): scope the cross-schema fallback to the caller's register ObjectService::find() takes register+schema, does a scoped lookup, and on a miss retries. The retry dropped BOTH the register and the schema, so a legitimate 'not in this register' answer was produced by scanning every magic table on the instance. That scan is a UNION with one branch per magic table. At 2,728 tables it is 690 KB of SQL, and its cost is almost entirely PLANNING: Planning Time: 3404.926 ms Execution Time: 546.145 ms <- returns zero rows No index can reduce parse time, so the only fix is to not emit it. The fallback exists for a stale or sibling SCHEMA inside a register the caller named correctly (openbuild#75 / openregister#1520) — it was never meant to search other registers. It now keeps the register and drops only the schema, and MagicMapper::find()/findAcrossAllSources() accept a registerIdScope that filters candidate tables by register id. Why this mattered on the write path: the flow-resolver registry asks every resolver in turn whether a flow is theirs. Each non-owning resolver called find(register: <its own>, schema: 'flow'), missed correctly, and paid a full instance-wide scan to say 'not mine'. Measured 2026-07-29, that single widened fallback was ~1.9s of a ~3.0s create. event dispatch inside the create 1,900ms -> 80-187ms wall (median of 5) 3.2s -> 1.55s Also adds the measurement half of the change: - tests/perf/object-create.sh reports min/median/p95 plus per-write schema seq scans and commit count, and separately measures the INSTANCE FLOOR (an authenticated request doing no object work). Nextcloud boots every enabled app per request; on this instance, with 92 apps, that floor is ~864ms — larger than the whole 500ms budget — so wall time alone cannot tell you whether the write path regressed. - SchemaMapper::traceRead() attributes every uncached schema read to its caller (gated on the perf_trace_schema_reads app-config flag); this is what identified the 1,471-call DocuDesk path. - WritePhaseProbe times the write path's phases; this is what showed the cost was event dispatch rather than the INSERT (76ms). Current state: wall p95 1,421ms = 864ms instance floor + 557ms write path. Refs openspec/changes/object-write-sub-500ms tasks 0, 1, 5. * perf(flow): memoise flow resolution for the request FlowResolverRegistry::resolveFlow() asks every registered resolver in turn. A resolver that does not own the flow answers by looking the id up in its own register — so an unowned flow pays the entire chain, and every answer costs a database round trip. FlowTriggerService::fire() calls it once per queued run, and it calls it from runInline() BEFORE checking whether the flow is even synchronous, so an async flow pays a full resolution for nothing. A save that cascades fires the same triggers again for each child, so one object write resolved the same ids several times over. Memoised per request, misses included: 'no app owns this flow' is the expensive answer, since producing it requires every resolver to look and fail. Refs openspec/changes/object-write-sub-500ms. * perf(objects): optionally defer the created-event dispatch out of the write ObjectCreatedEvent has ~22 listeners across the fleet. Dispatched inline the caller waits for all of them: measured 2026-07-29 it was 234-501ms of a ~530ms create, against a 76ms insert. None of it is needed to tell the caller its object was saved. Opt-in via 'occ config:app:set openregister defer_object_events --value=1'. Off by default, because deferring changes an observable contract: a 2xx stops meaning 'and every side effect has been applied'. A flow declaring executionMode: sync exists precisely so its effects land before the save returns, so this cannot be flipped on for everyone by fiat. event dispatch inside the create 234-501ms -> 6-8ms write path p95 (wall minus floor) 557ms -> 300ms CARRIES THE ACTING USER, and that is not incidental. The first version did not, and it was a perfectly green no-op: the job ran, logged nothing, threw nothing — and produced ZERO CloudEvents where the inline path produced one. A background job has no session, and OpenRegister reads are organisation- filtered against the session user, so every listener that consults the register (the CloudEvent firehose gate most visibly) saw an empty instance and skipped. Deferring side effects without carrying identity does not move the work, it deletes it. Verified after the fix: deferred dispatch produces exactly 1 CloudEvent, matching inline. The impersonation is released in a finally so it cannot leak into whatever job the worker runs next from the same process, and a missing acting user is logged at WARNING (not INFO — the default loglevel is 2, and a side effect that silently vanished is exactly what must not be filtered out). * docs(perf): record measured results and correct the budget's definition Two corrections the measurements forced, both of which change what the requirement can mean: 1. The budget must be wall time MINUS THE INSTANCE FLOOR. An authenticated request doing no object work costs 864-1,099ms on this instance, because Nextcloud boots all 92 enabled apps per request. That is larger than the whole 500ms budget, it is PHP-side (boot issues almost no queries, and opcache is healthy: 0 OOM restarts, 91% hit rate), and no work in the write path can remove it. A measurement that does not subtract it reports how many apps are installed. 2. Deferred dispatch must carry the acting user. Added as an explicit requirement and scenario after the first implementation shipped a flawless no-op — zero CloudEvents, no exception, no log. Results recorded: write path 12.8s -> 183ms median / 300ms p95, inside budget. Wall 13.7s -> 1.28s. Task 5 is ticked but was solved by a DIFFERENT mechanism than specified — the fan-out was reached through the cross-schema fallback, not through untyped relation properties — so the originally-specified work is explicitly folded into task 6 rather than quietly counted as done. * perf(probe): attribute the request timeline, not just the write Adds WritePhaseProbe::stamp() — absolute offsets from REQUEST_TIME_FLOAT, distinct from mark()'s durations, because the question is 'how much of the request had already elapsed before our code ran at all'. Stamped at OpenRegister's register(), its boot() entry and exit, the create controller, and the end of the write. One create, wall 1,464ms: or.register.in 122ms NC core + apps registering before us or.boot.in 903ms +781ms of OTHER APPS registering or.boot.out 928ms our own boot: 25ms ctrl.create.in 964ms +36ms routing and middleware flush 1,357ms +393ms the actual write So 964ms of a 1,464ms request elapses before the controller is entered, and 781ms of that is apps registering. Bracketed directly on the same instance with the same auth: status.php (no app boot) 47ms capabilities (full app boot) 970ms ~920ms is booting 92 enabled apps, ~10ms each. 24 of them are Conduction fleet apps; a deployment running OpenRegister plus a handful of leaves boots a fraction of that. This is why the 500ms budget is specified against the write path with the instance floor subtracted: no change to the write path can move the 920ms, and a wall-clock measurement of it is a measurement of the app count. Off unless /tmp/or-trace-write-phases exists, checked once per process. * perf(probe): stamp register() exit — proves the boot cost is not ours Adds or.register.out, closing the one gap in the request timeline that still allowed 'maybe OpenRegister's own registration is the pig'. It is not: or.register.in 235ms or.register.out 235ms <- our register() is 0ms or.boot.in 1,355ms <- +1,120ms of other apps + NC phases or.boot.out 1,376ms <- our boot() is 21ms ctrl.create.in 1,451ms OpenRegister has 268 registerService/registerEventListener calls across a 4,248-line Application.php and they cost nothing measurable, because they are lazy closures — which is the point of the API and worth having proof of. So the per-request app-boot cost (~16ms/app across 92 apps, measured by disabling 8 and restoring them) is Nextcloud's own registration and boot machinery plus the other 91 apps, and no change inside this app can reduce it. * chore: retrigger CI (previous run was cancelled by a duplicate dispatch) * perf: creates are under 500ms end-to-end — p95 469ms BUDGET MET. Measured on larpingapp/character with tests/perf/object-create.sh: wall p95 13,700ms -> 469ms wall median 13,688ms -> 385ms write path 12,800ms -> 93ms median / 177ms p95 Requires an app token, defer_object_events=1, and PHP JIT disabled (Conduction/.github#75). With deferral off the p95 is 650ms. CORRECTS MY OWN EARLIER ANALYSIS. This change previously documented an 'instance floor' of 864-1,099ms attributed to Nextcloud booting 92 apps, and concluded wall-clock under 500ms was unreachable without disabling apps. That was an artefact of the benchmark, not a property of Nextcloud. Every sample authenticated with HTTP Basic auth carrying the ACCOUNT PASSWORD, which Nextcloud bcrypt-verifies on every request. Same endpoint, same instance, back to back: account password median 1,058ms app token median 456ms ~600ms per request was password hashing. No real client authenticates that way - browsers carry a session cookie, integrations use app tokens - so the benchmark was measuring bcrypt and charging it to the application. The true floor is ~240-290ms. The harness now warns when NC_AUTH looks like a password, and the spec makes token auth a precondition of the measurement with its own scenario. Worth stating plainly: I disabled apps to derive a per-app cost, checked opcache, checked APCu, and measured a slope - all real work, all answering the wrong question, because I never questioned the instrument. A floor assumed to be structural deserves the same attribution discipline as the code under test. * fix(deps): pull ddn/sapp from GitHub, not Codeberg The code moved back to GitHub but this composer VCS repository did not, so every 'composer install' in CI still cloned from codeberg.org. When Codeberg returned 504s today, openregister's Newman suite failed in 'Install composer deps' on a dependency fetch — a full CI outage caused by a host we no longer publish to. ConductionNL/sapp on GitHub carries the same history: the pinned commit 5c406e91254d6936f44372db35f1cc15e5a06c56 and its branch feat/chained-filter-text-replace both resolve there. The lock now references the identical commit via GitHub, and no other package moved. Verified with a cold, unauthenticated 'composer install' against an empty cache: ddn/sapp downloads and extracts from GitHub with no Codeberg contact. Note for whoever picks this up: the .github submodule's origin is still https://codeberg.org/Conduction/.github.git even though ConductionNL/.github exists on GitHub. That is why the JIT change had to be raised as Conduction/.github#75 on Codeberg rather than a GitHub PR. * docs(perf): plan to bring object writes to the instance floor object-write-sub-500ms took a create from 13,688ms to 322ms median / 476ms p95. The 500ms budget is met and is no longer the binding constraint: the instance floor — an authenticated request doing no object work — is 172-213ms, so an absolute wall budget mostly measures how many apps are installed. This change targets the write path costing <=50ms above that floor, and the budget in the spec becomes floor-relative for the same reason. What the remaining cost is, from full PostgreSQL statement logging of one create scoped to its backend and time window (326 statements, 176.8ms): 57 x 44.7ms SELECT * FROM oc_openregister_schemas WHERE uuid=? OR LOWER(slug)=? OR id=? 24 x 3.2ms register lookups, same shape 18 x 0.8ms SELECT lastval() 9 x 4.7ms SELECT 1 FROM information_schema.tables -- 2 audit rows + hash-chain UPDATE, 2 notification rows ~135 committed transactions where there should be 1 The schema query seq-scans by construction: SELECT * hydrates a ~2KB properties blob and LOWER(slug) cannot use an index (Rows Removed by Filter: 1916 of 1929). 19 tasks in 5 phases, ordered by measured payoff: identity map for schemas and registers, cheap miss path, stop probing information_schema, one transaction, finish the deferral set, then delete the 2,728-branch fan-out via a uuid->(register,schema) index rather than optimise it. Phase 4 covers per-request work outside the write, in scope because the write is measured against the floor (ADR-076): pipelinq iterates the 3.4MB appstore catalogue on EVERY request (resolveDependencyStatuses -> buildAppStoreLookup -> AppFetcher::get). It is free here ONLY because has_internet_connection=false returns an empty set. It also computes provideInitialState() on API requests that render no UI. openconnector invokes a repair step from boot(). Correctly gated AND persists its key, so free today — and one cleared config key from a repair step per request. ADR-076 rule 4 puts that fallback in a TimedJob. 31 cron jobs, 8 at 60s. An idle instance does 18 schema seq scans and 356 commits per 4 seconds: the noise floor every measurement here fights. Two measurement hazards written into the tasks because both cost me real time this session: benchmarking with the account password adds ~600ms of bcrypt per request (app token: 456ms median vs 1,058ms), and pg_stat_* counters are database-global so cron pollutes them — the statement-log method is authoritative. Task 8 is flagged as a product decision, not a performance one: whether deferral becomes default depends on what executionMode:sync promises. * docs(perf): re-measure before executing — the target is already met at median I re-measured before starting on this plan, and the numbers it was written against are stale. Acting on them would have meant a large refactor for a small gain. wall min 220ms median 249ms p95 394ms instance floor 207ms WRITE PATH min 13ms median 42ms p95 187ms The <=50ms target is met at median (42ms). p95 is over, but the host was at load 4.6-6.0 from unrelated work — noise, not code. What moved, from statement logs of one create before and after the DocuDesk fixes landed: before now statements 326 251 schema lookups 57 (44.7ms) 15 (11.3ms) register lookups 24 (3.2ms) 24 (3.5ms) information_schema 9 (4.7ms) 9 (3.5ms) SELECT lastval() 18 9 Phase 1 is therefore worth ~11ms, not ~45ms. Tasks 1-4 are DEFERRED, not cancelled — they earn their keep again if the schema count grows or a caller reintroduces a hot loop, and the reasoning stays in the file so nobody has to rediscover it. Also recording a hypothesis of mine that did NOT hold, because it looked compelling: the re-measurement showed the non-lazy appconfig load at 80.6ms, 42% of all DB time, and app_versions stores 9.1MB across 86 non-lazy keys (appstore.payload.*, up to 3.2MB for mail). Marking them lazy made creates MARGINALLY SLOWER (275 -> 299ms median) because memcache.local is APCu and the config is cached across requests — the 80ms was a cold-cache event, once per PHP worker, not per request. Reverted. Still worth doing as hygiene; not a performance task. Revised priority: p95 stability first (establish whether it moves at all on an unloaded host before calling it a code problem), then the phase-4 latent items (pipelinq's appstore walk is free ONLY because has_internet_connection=false; openconnector is one config key from a repair step per request; the 60s cron fleet is the noise floor), then task 6 (one transaction) on correctness grounds rather than latency. * test(pdf): commit the PDF fixture instead of reading a dependency's examples/ PdfExtractorTest read vendor/ddn/sapp/examples/testdoc.pdf. That path only ever resolved because composer happened to install ddn/sapp from SOURCE: the package declares /examples export-ignore in its .gitattributes, so every distribution archive omits the directory. A git clone keeps it; a zipball does not. Switching the package to its GitHub dist (7ac9c92, to get Codeberg off the CI critical path after an outage took the suite down) made that latent dependency visible as exactly one failing assertion out of 15,504 tests: Failed asserting that file ".../vendor/ddn/sapp/examples/testdoc.pdf" exists. So the regression was mine, and the underlying fault is older: a test must not depend on a dependency's examples directory, because the dependency has explicitly declared it not part of what it ships. The fixture is now committed at tests/fixtures/pdf/testdoc.pdf (51,269 bytes, byte-identical) and the test is independent of how composer chooses to install anything. Verified: 3 tests, 7 assertions, green. * perf(crud): measure read/search/update/delete — this REVERSES the Phase 1 deferral I had only ever measured creates, and generalised from that to defer the schema identity map as "worth ~11ms". Measuring the rest of the object API shows that was the create-only figure and the wrong call for everything else. tests/perf/object-crud.sh covers create/read/search/update/delete, reports every figure both as wall time and as wall minus the instance floor measured in the same run, and prints host load so a bad sample is visible. Wall (5 runs each, host load 61.7 from unrelated work — the ABSOLUTE numbers are badly inflated, the RATIOS are the finding): create 1,477ms 525ms above floor read 1,447ms 495ms above floor search 659ms 0ms above floor <- never leaves the floor update 9,080ms 8,128ms above floor <- 15x a create delete 4,243ms 3,291ms above floor <- 6x a create Statement counts, which do NOT inflate with load (statement log scoped to the request's backend and time window): statements DB time create 251 194ms update 716 2,672ms delete 595 1,781ms Same shape in both slow paths — it is repeated resolution, not the write: update delete create register lookups 66 64 24 schema lookups (uuid OR slug OR id) 51 51 15 schema lookups (slug + id IN) 42 42 9 information_schema.tables probes 27 27 9 getLiveMagicTables() (all 2,728) 12 - ~3 So tasks 1-5 move back to the top. Task 3 (the REGISTER map) now matters more than task 1, because registers are re-resolved more often than schemas. Search being free is worth noting too: whatever the list path does, it is already right. The lesson is mine to own — I measured one operation and generalised. The budget in the spec is per-operation for a reason. * perf(magic): memoise the magic-table enumeration per request getLiveMagicTables() lists EVERY magic table from information_schema and then fetches the full register and schema id lists to discard orphans. On this instance that is 2,728 tables, ~60ms a call. Measured 2026-07-30 (statement log, scoped to the request): an object UPDATE called it 12 TIMES. The answer cannot change mid-request unless this request creates a table, which is handled below. getLiveMagicTables enumerations per update: 12 -> 3 Not 1, because several MagicMapper instances participate in one update; the memo is per-instance. Getting to 1 needs the instances shared, which is a DI change and out of scope here. Also memoises checkTableExistsInDatabase(), but ONLY POSITIVE answers. A negative can legitimately become positive within the request — ensureTableExists() creates a table and then writes to it — and caching "no" would break that write against a table that now exists. Creating a table invalidates both memos via invalidateTableMemos(). HONEST LIMITATION: this did NOT reduce the 27 information_schema existence probes an update issues; that count is unchanged, so those come from a third path (most likely Doctrine's IDBConnection::tableExists() through another caller, not through this method). RegisterService::magicTableExists() already documents the same class of bug costing 76 SECONDS on a stats endpoint, so the pattern is known and solved in one place and not others. Tracked as task 5 of object-write-at-instance-floor; finding the third caller is the next step. Verified: 15,509 tests / 34,667 assertions green. phpcs clean. * docs(perf): the statement counts were measured on a POOLED connection — caveat them The per-operation statement counts (create 251, update 716, delete 595) came from taking every statement on the request's PostgreSQL backend within a time window. A backend is a pooled connection serving consecutive requests, so the window sweeps in unrelated traffic. Tight enough for a ~500ms create; useless for an update that took 30s under host load 21-62 — 24 distinct backends issued probes during that capture. This also retracts the inference I drew from it: '27 information_schema probes per update, each table probed 3x, therefore 3 MagicMapper instances'. The 3x is far more likely 3 REQUESTS reusing one pooled connection. What stands: the wall-clock ratios (update 15x a create, delete 6x, search free) time individual HTTP requests and are unaffected by pooling, and the direction of the finding — update/delete do far more repeated resolution than a create — is visible regardless of the multiplier. What does not: the counts themselves, and the breakdowns derived from them. Correct method for next time: bracket on a marker the request emits in its own SQL, add a per-request id to log_line_prefix, or count from inside PHP where 'this request' is unambiguous. * perf(probe): count per-request from inside PHP — supersedes the pooled-log counts Adds WritePhaseProbe::count() and instruments the three lookups the CRUD work implicated. Counting inside the request makes "this request" unambiguous, which the PostgreSQL statement log cannot: a backend is a pooled connection serving consecutive requests, so bracketing by wall-clock sweeps in unrelated traffic. The real numbers, and they are much lower than the log suggested: schema reads tableExists full enumerations create 6 3 0 update 13 7 1 delete 12 7 1 Against the log-derived figures I published (create 15 / update 51 schema lookups, 27 probes), these are 2-4x smaller. Update and delete do roughly TWICE a create's schema reads and each performs one full 2,728-table enumeration a create does not — a real gap, and a far more modest one than I reported. Note this also means the wall-clock ratios (update 15x a create, delete 6x) are NOT explained by round-trip counts alone. Those ratios came from timing HTTP requests and stand; the gap between them and these counts points at PHP-side work, which is the next thing to measure rather than assume. The magic-table memoisation holds enumerations at 1 per request — the floor without sharing mapper instances, and better than the "12 -> 3" I claimed from the log. Limitation: read and search produce no counts, because flush() is only reached from the write path. Instrumenting the read path is open. * style(probe): restore stamp()'s docblock, which my count() insertion orphaned Inserting count() above stamp() left stamp()'s docblock attached to count() and stamp() with none — the third time this session that anchoring an insertion on a function SIGNATURE rather than on the docblock above it produced exactly this. Worth remembering: anchor on the docblock opener, not the signature. * feat(flow): live-runs read for the shared "running flows" widget Gives every app one honest answer to "which flows are running right now", scoped to the caller's organisation, so a dashboard widget can show it without each app building the same surface again. GET /api/flow-runs/active returns the NON-TERMINAL runs — queued, running and suspended, defined once as FlowRun::ACTIVE. Filtering to literally `running` would be empty almost always: a run holds that status only for the duration of a worker pass, while queued and suspended are where a live run actually waits. Three things had to change for that read to exist: - `organisation` is now STAMPED on queue(). The column has existed since the table was created and nothing ever wrote to it, so no tenant filter was possible at all. Resolution is lazy through the container: the cron worker builds this service on every pass and must not drag the RBAC graph in to fill a column it usually cannot fill. A run queued with no session is recorded unattributed rather than guessed at. - Scoping is STRICT. A run with no organisation goes to nobody. This feeds a widget every app renders to every user; attributing an unattributed run to the reader's tenant would put one tenant's activity on another's dashboard. - The rows are SUMMARISED — uuid, flow id AND resolved flow NAME, status, trigger, who started it, subject, current step, timestamps. Not the marking, not the items (which can hold the subject's own record data), not the step log: kilobytes per run a list never renders. The single-run endpoint stays the place to ask for a run's contents. `GET /api/flow-runs` is deliberately unchanged. It is the history surface with existing e2e coverage; a separate endpoint is what lets the tenant boundary be strict here without changing what existing callers see. Also indexes (organisation, status, id). Measured before the index on a dev instance with 48,058 runs: the planner walked the primary key backwards and filtered, reading 48,048 rows to return 1 (294ms in postgres, ~21s over HTTP) — on a surface a widget polls every 15 seconds. Tests: 32 green (5 new — no organisation reads nothing and never queries the store, scoping passes the caller's org through, rows carry the resolved name and step, an unresolvable flow falls back to its id, the row limit is capped). phpcs / phpmd / phpstan / psalm clean on the changed files; the two pre-existing StaticAccess findings and TooManyFields on the entity are now carried as reasoned suppressions rather than left failing. * fix(e2e): global setup timed out on a header it reported as VISIBLE The whole e2e suite was unrunnable locally. global-setup gates every spec, and it died at: TimeoutError: page.waitForSelector: Timeout 20000ms exceeded. - waiting for locator('#header, header.header') to be visible - locator resolved to visible <header id="header">…</header> The call log contradicts itself: the locator RESOLVED TO VISIBLE and the wait still expired. That is a navigation race — the post-login redirect invalidates the handle mid-check, so waitForSelector keeps retrying a stale one until the budget runs out. It widens under load, which is why it bit here (host load 14-62) and presumably passes on a quiet CI runner. Three changes: The URL check moves FIRST. It is cheaper, needs nothing rendered, and when login genuinely failed it gives "still on /login" instead of a timeout on an element that was never going to appear. expect().toBeVisible() replaces waitForSelector. It re-resolves the locator every poll, so a redirect costs one poll rather than the whole timeout. waitForLoadState('domcontentloaded') settles the navigation first. NOT networkidle, which never fires on Nextcloud because of its notification poll (ADR-074 rule 4). Verified: tests/e2e/core-crud.spec.ts, chromium, 7 passed (3.8m) on the same loaded host that could not get past global setup before. Covers auth, app-bootstrap and four deep-link routes. Closes #2208. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…path An update resolved the same UUID FOUR times over every magic table on the instance before doing any work, and a delete twice. Each resolution falls through to findAcrossAllMagicTables(), which UNIONs the register's tables after enumerating information_schema. Measured with WritePhaseProbe on the development instance (2,728 magic tables, 92 enabled apps), same host, minutes apart: pc:permissions 1,435 ms -> 2 ms two unscoped find() calls folder 1,030 ms -> 2 ms ensureObjectFolder's unscoped find() post-save unlock 780 ms -> 0 ms LockHandler's all-tables scan update wall 1,260 ms -> ~550 ms The five sites: - ObjectsController::update() passed register/schema NULL to findSilent(), three lines after resolving them, and then asserted the object was in that exact register/schema anyway. patch() already did this correctly. - ObjectService::checkSavePermissions() resolved the UUID unscoped to decide create-vs-update. Also the wrong question: an object of the same UUID in another register must not supply the owner the permission check is made against. - ObjectService::rejectIfTransferred() did the same lookup again, on the same UUID, in the same phase. - ObjectService::ensureObjectFolder() did it a third time, directly below enforceReadOnlyOnUpdate(), which was already scoped and carries a comment warning about this exact fallback (openregister#1520). - ObjectsController::destroy() set register/schema on the service but called deleteObject() without them; deleteObject gates scoping on its ARGUMENTS ($hasScope), not on service context, so the scope was silently dropped. This also meant the URL's scope was never enforced: an object could be deleted through a register/schema it does not belong to. The post-save unlock is now guarded on isLocked(). unlock() must resolve the identifier before it can act, then returns immediately when nothing is locked (the openregister#195 idempotence branch) - which is the normal case for a defensive post-save unlock. The controller already holds the saved entity, so that question is free. Adds phase marks (pc:permissions.check, folder.readonly, folder.ensure) and stamps (ctrl.update.in, ctrl.destroy.in, ctrl.update.out) so update and delete are attributable the way create already was. All gated on the existing /tmp/or-trace-write-phases flag. Note for anyone using the probe: /tmp/or-write-phases.log must be writable by www-data. Created root-owned it produces NO output at all, because the write is @file_put_contents - a silently dead instrument.
…ributable Adds del.scope / del.transferred / del.found / del.permitted stamps to deleteObject(), bounding the work between entering the controller and handing off to the delete handler. With the scoping fixes in place that preamble is now 14 ms (ctrl.destroy.in=218ms -> del.permitted=232ms), which rules it out: the remaining ~650 ms of a delete is inside the delete handler itself. Of that, 219 ms is traced (mm:EVENT-DISPATCH 147 ms, mm:DB-INSERT 40 ms) and ~430 ms is still unattributed - referential-integrity checks, cascade, file cleanup and audit sealing are the candidates. Recording the bound rather than a guess: a delete runs the full save path (soft delete writes the object back), so it inherits the create-path event dispatch that defer_object_events does not yet cover for deletes.
…f overwriting (#2211) saveObject() is an upsert: given an identifier that already exists it updates, silently and successfully. That is right for almost every caller and is unchanged here. It is wrong for a caller that is CLAIMING something — a lock, a slot, a lease, a queue position. There "it already existed" is the entire answer, and swallowing it means two callers both believe they won while the loser is never told. Measured before writing any of this: POST /api/objects/13/213 {"id":"a1b2…0001","name":"probe-1"} -> created POST /api/objects/13/213 {"id":"a1b2…0001","name":"probe-2"} -> 200, same id SELECT count(*), string_agg(_name,'|') -> 1 | probe-2 The _uuid unique constraint exists on the table but nothing surfaces it — the collision is resolved into an update before it can fail. WHAT THIS ADDS, all opt-in: ObjectExistsException 409 Conflict, carries the uuid so a caller can tell "my claim lost" from other errors SaveObject::saveObject() $failIfExists (default false) + a guard on the existing-object branch ObjectService::saveObject() pass-through ObjectsController::create() `_failIfExists=true` -> 409. Caught BEFORE the generic \Exception handler, which flattens everything to 403 — a losing claim reported as "forbidden" is indistinguishable from a permissions problem ObjectWriteNode `onConflict: fail` for flows DEFAULTING TO false IS THE LOAD-BEARING CHOICE. Making create strictly-insert by default would change behaviour for every caller relying on today's upsert — and because that upsert is silent, there is no way to enumerate from the code who depends on it. VERIFIED ON A LIVE INSTANCE, both directions, after the final reformat: default (no flag): create=201, create=201 -> 1 row, last write wins _failIfExists=true: create=201, create=409 -> 1 row, FIRST claimant kept it 409 body: {"error":"An object with identifier \"…\" already exists.","uuid":"…"} Gates: phpcs clean on all changed files, phpstan OK, 15,516 unit tests green. Closes #2210. The consuming case is hydra's flows-first port (hydra#425 D7): a concurrency cap expressed as slot objects, where the node's findMatch/saveObject pair could not claim a slot without a lost update.
…2213) #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.
… index Event dispatch is the largest remaining cost in an object write (mm:EVENT-DISPATCH median 133ms, measured n=32 at host load 3.2), so this splits openregister's object listeners along the line the event names already imply: `*ing` pre-events stay synchronous because they veto or mutate; `*ed` post-events defer. Measured first, so the work went where the cost actually is: - DI construction of all 21 listener classes totals <1ms, so the cost is handler bodies, not wiring. Deferral is the right lever. - ObjectCleanupListener does the heaviest post-event work (a full CalDAV calendar walk, vCard/VEVENT rewrites across six services). Every cleanup is keyed by the object UUID alone, so unlike the other delete handlers it needs no re-fetch of the hard-deleted row. - FlowTriggerListener's trigger resolution listed and rendered EVERY flow object on every write to answer "is any flow wired to this event?". Changes: - Extract ObjectRelationCleanupService from ObjectCleanupListener verbatim, so the inline (kill-switch) path and the deferred job share one implementation and cannot drift. - Add ObjectCleanupJob on the existing actor-forwarded deferral contract (openregister#408). The six cleanup services resolve the ACTING user, which ActorForwardedJob re-establishes. Includes a re-create guard: an entry whose UUID resolves to a live object again is skipped, because at-least-once delivery must not wipe a re-created object's relations. - ObjectCleanupListener becomes gate-and-enqueue (0.25ms), retaining the full inline path for `listenerDeferral=inline`. - Cache the flow trigger index in OpenRegisterFlowResolver, keyed off a compact (id, trigger, register, schema) projection of enabled flows, with a request-level memo and a 60s TTL matching AggregationCache. Set `openregister/flow_trigger_index_ttl=0` to restore read-through. Verified: - Controlled A/B at identical host load via the TTL kill switch, 3 rounds: EVENT-DISPATCH median 124/124/127ms off vs 117/116/116ms on; p95 146/141/145 vs 134/136/125. - Trigger resolution returns an identical flow set (11 enabled flows, same ids) with the new `enabled` query filter as with the old PHP-side filter. - The deferred path was proven end-to-end, not merely "the job ran": the listener enqueues a correctly shaped ObjectCleanupJob argument. - phpcs + phpstan clean on all four files. Psalm crashes on OpenRegisterFlowResolver both before and after this change (pre-existing). Also logs an unreadable flow store, which previously failed silently and was indistinguishable from "no flows configured".
… silently updating (#2215) #2211's guard sat in SaveObject, one layer too high, and changed nothing under concurrency. Measured then: 12 simultaneous claims on one identifier produced up to 8 responses of 201, up to 8 audit `create` entries, and ONE surviving row. WHY. There are two stacked check-then-writes, not one: 1. SaveObject::saveObject() findAndValidateExistingObject -> create 2. MagicMapper::saveObjectToRegisterSchemaTable() findObject -> insert | update A losing writer passes (1), so the SERVICE labels the operation a create and the audit trail records `create`. By the time it reaches (2) the winner's row exists, so it quietly took the UPDATE branch — overwriting the winner's data, returning 201, and logging a create. That is why guarding only (1) had no effect: the common case never reaches an INSERT at all, so there is no constraint violation to catch. Pinned by a single observation: after a race the table held ONE row with ONE _id and three audit creates. A second INSERT would have produced a second _id or a violation. Neither happened, so the extra "creates" were updates. THE FIX, in MagicMapper where the branch actually is: - update branch + failIfExists -> throw ObjectExistsException (the common case) - INSERT collision + failIfExists -> catch DbException REASON_UNIQUE_CONSTRAINT_VIOLATION -> throw ObjectExistsException (the genuinely simultaneous case; portable across MySQL/PostgreSQL, matching NotificationDedupeStateMapper's existing pattern rather than emitting dialect-specific ON CONFLICT SQL) - INSERT collision without the flag -> fall through to update, so upsert intent still lands the caller's data VERIFIED, 12 racers per run: insert-only 6/6 runs 1x201 11x409 1 row, 1 _id (was up to 8x201) default 10 racers 10x201 1 row, no errors (unchanged) sequential upsert still last-write-wins Re-run after the final phpcbf reformat, not just before it. Gates: phpcs clean, phpstan OK, 15,516 unit tests green. Closes #2212. Unblocks hydra#425 decision D7 (task 3.5): a slot claim can now be expressed with object-write without a lost update.
fireDueFlows() had no overlap guard. A scheduled flow can be slower than its own interval — a pipeline poll on a five-minute cron easily is — so tick N+1 started while tick N was still going, and two runs of one flow raced on whatever that flow was bookkeeping. That is the same failure openregister#2212 documented one layer down at the object store, arriving from the scheduler instead. Adds FlowRunMapper::hasActiveRun(), using the same NON-terminal definition findActive() already uses: queued, running and suspended all mean "still going". A guard that only looked at `running` would let a suspended run be overlapped, which is precisely the long-lived state a slow flow spends its time in. It is a count with a limit rather than a fetch, since the scheduler asks once per due flow per tick. The last-fire marker is deliberately NOT advanced when a tick is skipped. If it were, a flow skipped at 10:00 would not be due again until 10:05 even though its run finished at 10:01 — idling a whole interval for nothing. Left alone, the flow starts on the first tick after its previous run completes. Why this matters beyond one guard: it is the property that makes the shell orchestrator being replaced safe today. hydra-supervisor.sh holds an exclusive flock, so exactly one supervisor exists and its check-then-write slot bookkeeping never races — not because the claim is atomic, but because nothing runs beside it. A scheduled flow now gets the same guarantee, and with it most flow state needs no locking at all. Tests assert both halves and were verified RED against origin/development (2 failures) before this change: the skip itself, and that skipping leaves the flow due. Gates: phpcs clean, phpstan OK, 15,518 unit tests green. Part of #2216.
A scheduled flow starts blank on every tick. The flow object carries only its definition (nodes, edges, cron), and openregister_flow_runs carries marking/items/context, which are PER-RUN resumption state discarded when the run ends. So a flow polling every five minutes had nowhere to keep a counter, a cursor, a capacity table or "what did I already process". Every such value had to become a separate register object — which is how hydra's concurrency cap became object writes and walked into the lost update documented in #2212. WHY A TABLE, NOT A COLUMN ON THE FLOW. FlowScheduleService already keeps one piece of per-flow state — the last-fire timestamp — beside the flow rather than on it, and says why: "so the flow object itself is never rewritten". Writing state onto the flow object each tick would churn the definition's version and audit history with machine noise, race with an operator editing that flow in the UI, and merge "what this flow does" with "what it is currently holding". This generalises the scheduler's existing choice rather than contradicting it. flow_id is UNIQUE — one state row per flow. put() therefore attempts the INSERT FIRST and catches the unique-constraint violation by REASON, rather than checking for a row and then writing. Check-then-write is a lost update waiting for two concurrent writers, which is the mistake this whole line of work exists to stop repeating. Caught by reason rather than by dialect-specific upsert SQL, so MySQL and PostgreSQL behave identically — the pattern NotificationDedupeStateMapper already uses here. VERIFIED ON THE LIVE INSTANCE, not only in unit tests: create -> update -> readback -> delete round-trips correctly 10 CONCURRENT put() on a fresh flow id 10 OK, 1 row, 1 id, no errors Migration executed for real (occ upgrade, instance left out of maintenance mode), and the table verified to carry or_flowstate_flow_uq UNIQUE on flow_id. Live checks re-run AFTER the final phpcbf reformat. Gates: phpcs clean, phpstan OK, 15,521 unit tests green. SCOPE, stated plainly: this is the storage layer. Nodes cannot read or write it yet — exposing it in the run context and persisting changes at run end is the next step, deliberately separate so this lands verifiable rather than half-wired. Part of #2216.
…migration runs (#2220) Completes the storage layer from #2219. FlowStateHandle is FlowToken's long-lived sibling: the token belongs to one RUN and dies with it, this belongs to the FLOW and outlives every run of it. Nodes reach it at $context['flowState']. It is a class rather than an array for exactly the reason FlowToken is: IFlowNode::execute() takes $context by value, so a plain array could only ever be read. An object is a handle and the handle survives the copy, which buys write access for every node without changing the signature any node implements. Loaded from openregister_flow_state at run start and persisted at run end, then REMOVED from the run's context before that context is stored. Writing it into the context JSON would (a) leave a per-run copy of flow-level state, so a resumed run would restore a stale snapshot over whatever later runs wrote, and (b) duplicate a slot table into every run row the flow ever produces. Only persisted when a node actually changed something. A flow that merely reads its state should not touch the row, and on a five-minute schedule that is thousands of writes a week avoided. ALSO IN HERE, and the reason this is not purely additive: - VERSION BUMP. #2219 shipped a migration WITHOUT bumping appinfo/info.xml, so it would never have executed on any deployment. 3 of the 4 most recent migration commits bump it; mine did not. Found because a host reboot wiped the dev container and the table had to be recreated. - Two stale comments corrected. SaveObject and ObjectExistsException still carried "THIS IS NOT A LOCK — openregister#2212" warnings I added before #2215 fixed it. They were true when written and are now false, and a false warning on a hot write path is worse than none: it invites someone to add a second guard, or to avoid a mechanism that works. VERIFIED LIVE, four separate handles against one flow: run1 sees [] dirty=false run2 sees {"slots":{"1":"job-A","2":null},...} <- run1's writes survived run3 read-only dirty=false, row untouched run4 forget("slots") dirty=true, has()=false Gates: phpcs clean, phpstan OK, 15,528 unit tests green. Part of #2216.
…claim (#2221) #2219 gave a flow somewhere to remember things and #2220 let nodes reach it, but nothing could WRITE it from a graph — a flow author still had to drop into PHP. This is the node that closes that. get / set / forget are the obvious three. `claim` is the one worth having. A capacity cap — hydra's "at most ten pipelines at once", a booking, a lease — is a map of named slots where a free one must be taken by exactly one holder. Expressed with get and set that is a read-modify-write across two steps, and that is precisely the shape that lost writes in #2212. `claim` does the whole thing in one step and emits `claimed: false` rather than pretending it succeeded, so a router can branch on it and the flow author decides whether no capacity means wait, stop or escalate. `release` takes a slot number when the caller has one, and falls back to the holder when it does not — a stage that crashed knows who it was, not which slot it got. A run with NO flow state throws rather than treating every key as empty. Absent state read as "nothing claimed" would make a capacity cap wave everything through, which is the worst possible failure for this node.⚠️ Documented in the class, because it will be the first question: this is not a cross-flow lock. What makes it safe is that a scheduled flow never overlaps itself (#2218) — the same property the shell orchestrator being replaced relies on, where one supervisor holds a flock and its own slot bookkeeping is a plain check-then-write, safe only because nothing runs beside it. For coordination ACROSS flows, write an object with `onConflict: fail` and let the database arbitrate. VERIFIED LIVE, node resolved from the registry rather than constructed by hand: flow-state registered: true claim job-1 -> slot 1 claim job-2 -> slot 2 claim job-3 -> slot 3 claim job-4 -> claimed=false, slot=NULL <- the cap holds release slot 2, then job-5 -> slot 2 <- freed slot is reused Gates: phpcs clean, phpstan OK, 15,539 unit tests green (11 new). Part of #2216. This is the primitive hydra#425 task 3.5 is built on.
#2222) Flow state has existed since #2219 and nodes could write it from #2221, but nothing outside the engine could READ it — a slot table was live data nobody could show. GET /api/flow/{flowId}/state closes that. A flow with no state yet returns an empty map rather than 404. "Nothing claimed" is a perfectly good answer, and a widget should not have to special-case a flow's first tick to avoid rendering an error. This is what makes the slot dashboard possible: with the claim node writing {"slots": {"1": "issue-101", "2": null, "3": "issue-207"}}, a widget can now show which slot is running what without shelling onto the host — which is how the shell orchestrator's SLOT_DIR files could only ever be inspected. VERIFIED LIVE against the running instance: GET /api/flow/api-demo-flow/state {"id":16,"flowId":"api-demo-flow", "state":{"slots":{"1":"issue-101","2":null,"3":"issue-207"},"cursor":42}, "updated":"2026-07-31T07:34:36+00:00"} GET /api/flow/never-run-flow/state {"flowId":"never-run-flow","state":[],"updated":null} Gates: phpcs clean on the controller, phpstan OK, 15,539 unit tests green. Note on the test file: FlowControllerTest carries 15 phpcs errors on origin/development and still carries exactly 15 here — the one my change added is fixed. The rest are pre-existing and unrelated to this endpoint. Part of #2216.
…31 (#2224) Closes #2203. This does not widen what the credential can do; it makes an existing capability honest. PATCH /repos/*/git/refs/* has been granted since v1.6.0, and it can fast-forward a base branch to a PR head — which GitHub records as a merged pull request. So merge was always reachable through the broker. The catalogue simply did not say so, and #2203 was filed on exactly that gap after I found it while landing commits through the brokered path. A rule the allow-list forbids but a one-line call achieves is worse than an explicit grant, because it makes the list unreliable as a statement of what a credential can do. Someone reading it would conclude hydra cannot merge, and be wrong. Naming the capability keeps merge authority reviewable, greppable and revocable in one place instead of being an emergent property of the refs rule.⚠️ The consequence is real and should be scoped accordingly: whoever holds a github credential brokered through this catalogue can now merge pull requests, explicitly. That is the decision, not a side effect of it. The guard test in DoffinProviderTest caught this immediately (14 -> 15), which is what it is for. Updated with the reasoning rather than just the number, so the next reader sees why the count moved. Unblocks hydra#425 task 2.3: the applier's GO verdict can now become a merge. Gates: 15,539 unit tests green.
…it as a no-op (#2227) * fix(flow): refuse a node that carries step config instead of running it as a no-op The step is the EDGE. `FlowEngine::stepFor()` resolves a transition to the matching entry in `edges[]`, and `RegistryStepDispatcher::dispatch()` reads `type` and `config` off that edge. A node is a Petri-net place and carries no behaviour. A `type` on a node is therefore not merely redundant — it is the whole behaviour of the flow, put where nothing looks. The engine accepted it: every transition became a pass-through (dispatch() returns items untouched when `type` is empty) and the run reported COMPLETED. No error, no warning, nothing in the trace, and an output key simply absent — indistinguishable from a flow whose steps genuinely had nothing to do. Three graphs in the fleet were authored this way and none of them failed anywhere: hydra's dispatch flow (shipped as a completed task), hydra's applier flow, and hermiq's seeded Hydra Triage flow — whose unit tests asserted on `nodes[].type` and therefore all passed. Node-shaped authoring is the natural mistake, because that is how a graph editor presents a flow, and because this class's own docblock calls itself the translation layer between what users author and what executes. `extractPlaces()` already refuses a duplicate id for the same reason ("the graph would run but not be the graph the user drew"). This is the same kind of authoring error with a larger blast radius, so it is refused in the same place, with a message naming the node and where the step belongs. Presentational keys are untouched: position, label and styling are what a canvas legitimately stores on a node, and a test pins that they still build. Fixes #2226. The three affected graphs are corrected in ConductionNL/hydra#435 and ConductionNL/hermiq#91; all three are `enabled: false`, so nothing in flight is broken by this becoming an error. 15,542 unit tests green (3 new); phpcs, phpstan and psalm clean. * style: group the `failIfExists` param tag with the rest Pre-existing phpcs failure on development, not introduced here: `SaveObject::handleObjectCreation()`'s docblock had `@param bool $failIfExists` separated from the other param tags by a blank line and misaligned, which trips "Parameter tags must be grouped together" and the type-padding sniff. Caught because CI runs phpcs over the whole tree while a local run scoped to the changed file does not.
…slots exist (#2228) * feat(flow-state): a slot records WHAT is running and WHEN, and empty slots exist `GET /api/flow/{flowId}/state` is meant to answer "what is running right now" — the whole reason slots became flow state instead of files on the supervisor host (or#2216). It could not. A claimed slot held a bare holder string, and an unclaimed one was simply ABSENT from the map, so a reader could not tell an empty slot from one outside the cap, could not say when a stage started, and had to be told the capacity separately — which puts the cap in two places again. Now: - an occupied slot holds `{holder, since, …}`; - `record` names item fields to copy onto it, so a caller decides what "what is running" means for its own flow (hydra: stage, repo, issue). A named field the item does not carry is recorded as null rather than skipped, so every slot has the same shape and a table renders evenly; - every slot from 1 to capacity is present, a free one as `null`. Compatibility is handled rather than assumed. A scalar written by the previous revision is carried forward into the record shape with a null `since`, so a flow mid-run does not lose track of what it was holding. A slot occupied ABOVE a capacity the operator has just lowered is KEPT until released — dropping it would free a slot somebody still holds and let the flow exceed the new cap immediately. `doRelease`'s by-holder path is fixed with it: it used `array_search($holder, $slots, true)`, correct only while a slot held the holder string itself. Against a record that never matches, so a crashed stage's slot would leak forever. It now reads into the record. This is what hydra-flows-first-port task 3.6 needs — its acceptance is "the widget shows each slot's stage, repo, issue and claim age, and an empty slot reads as empty rather than absent", and none of that was expressible before. 15,544 unit tests green (3 new); phpcs, phpstan and psalm clean. * feat(flow-state): serve one state key as a list so a table can render it `?list=<key>` on `GET /api/flow/{flowId}/state` additionally returns that key as `results` — one entry per slot, each carrying its own `slot` number. The state's natural shape and a table's required shape genuinely differ. A slot table is keyed by slot number so a claim is one lookup; a manifest `object-table` requires an ARRAY at its `responsePath`. Without this projection a dashboard has to reshape the payload in code, which is the thing manifest-driven widgets exist to avoid. Opt-in, and it names its key. Nothing is inferred from the shape of the data — a flow's state is arbitrary and "looks like a slot table" is not a contract. A FREE slot is emitted as a row, not skipped. "Slot 3 of 10 is empty" is what an operator needs to see, and a table that silently omits free slots reads as a smaller pool rather than an idle one — which is also why the node materialises the whole table rather than storing only what is taken. * chore: re-trigger Code Quality on the merged head The Code Quality run on this branch is pinned to the pre-merge commit, where the SaveObject phpcs error that or#2227 fixed was still present.
… and the audit must say what happened (#2229) Two of the four questions in #2217, answered by fixing them. ## A re-fetch that finds nothing is a lost write, not a fallback `insertObjectEntity()` re-reads the row it just wrote and, on DoesNotExistException, logged a warning and returned the IN-MEMORY entity as though persisted. Two very different causes shared that outcome: - the row IS there and only the filtered read could not reach it (scoping, an org/owner context still being established) — degraded but honest; - the row is NOT there. The write did not land, and the caller gets an entity with a null id and no way to tell. The second now throws. That is the shape of a lost write reported as a success, which is what #2212 turned out to be. ## The audit action now comes from the layer that knows `SaveObject` labels the operation from its OWN existence lookup. The mapper then looks again, and between the two lookups a concurrent writer can land the row — so the mapper takes its UPDATE branch on what the service already called a create, and the audit records `create`. Measured while debugging #2212: three audit `create` entries against a single `_id` that was inserted once. MagicMapper now reports what it actually did (`getLastWriteAction()`), set at all three write branches, and SaveObject records that instead of its own prediction. ## The fix that would have looked applied and changed nothing `buildAuditTrail()` inferred the action from `$action === 'update'` — which is both the DEFAULT and a legitimate explicit value. So `old: null, action: 'update'` was rewritten to `create` before it reached the row, and a caller who KNEW the write was an update could not say so. Passing an explicit action would have been a silent no-op. The default is now null = "infer"; a named action is used verbatim. Behaviour is unchanged for every caller that omits it, which is most of them — pinned by two tests alongside the guard, because a change that simply stopped inferring would pass the guard and break everyone else. Refs #2217. 15,545 unit tests green (3 new); phpcs, phpstan clean.
…ens (#2230) `pull_request.types` was [opened, reopened]. So the quality suite ran ONCE, when the PR was opened, and every commit after that merged unchecked while the PR still showed the first run's green. Observed here today: - #2227 was opened, failed phpcs on pre-existing debt, was fixed by a follow-up push — and the fix was never verified by CI. - #2228's only Code Quality run stayed pinned to its first commit across a merge from development and two further commits, so its checks were describing code that no longer existed. It took a manual workflow_dispatch to find out whether the branch was actually green. The branch list on `push:` does not cover the gap either: it names `feature/**` while the convention in practice is `feat/**` and `fix/**`. Cost is bounded by the existing concurrency block — a new push cancels the in-flight run for the same head ref rather than queueing beside it. hermiq's copy of this workflow omits `types:` entirely, which defaults to including synchronize; this repo is the outlier.
…nywhere (#2231) `normalisePath()` denied any path containing the substring `..`. That rejected legitimate paths whose segments merely contain dots — most importantly GitHub's diff endpoint, `/repos/{o}/{r}/compare/{base}...{head}`, so EVERY commit comparison was refused as traversal. That is not a cosmetic refusal. `hydra-flows-first-port` task 2.5 makes "diff the produced tree against the base before moving the ref" a MANDATORY rail on the commit-by-API path — precisely because `base_tree` overwrites rather than merges, so a tree built against a base that has moved silently reverts every file the caller did not send while producing a clean-looking commit. The rail could not be built at all over the brokered path while this guard stood. Found by running the chain: blob 201, tree 201, commit 201, then compare 403. The check now looks for a segment that IS `..`, which is what traversal actually is. The security property is unchanged and pinned both ways: - `/repos/Conduction/../../etc/passwd` still denied - `/repos/Conduction/%2e%2e/%2e%2e/etc/passwd` still denied (the guard decodes once BEFORE checking, so an encoded traversal segment is caught — pinned separately because a segment check running before the decode would be trivially bypassable) - `/repos/Conduction/openregister/compare/A...B` now allowed - `/repos/Conduction/some..name` now allowed Double-encoding is unaffected: `%252e%252e` single-decodes to `%2e%2e`, which is not `..`, exactly as before. 15,553 unit tests green (4 new); phpcs, phpstan clean.
…aken (#2232) `FlowEngine::advanceItems()` marks EVERY place on a firing transition's `to` list and then distributes items to them by output tag. So after a route, the branch the router did NOT choose is marked and holds zero items — and its steps still fire. Item-driven nodes shrug that off (zero items, zero work; SourceCallNode short-circuits explicitly). StopNode did not: it threw regardless, so a graph with a refusal stop on each guard branch ended its run on a guard that had not tripped. Measured on hydra's commit-by-API flow, which is what surfaced it. Every step through `move-ref` completed and the branch ref genuinely moved on GitHub — and the run reported: failed — "The branch tip moved while the commit was being built. Refusing to base a tree on a stale parent." The opposite of what happened, and precisely the failure the safety rail exists to report. A caller reading the run status would roll back a commit that was correct, or trust a rail that had in fact never fired. An empty branch also cannot be what an author meant: they wrote the stop to describe a condition, and no items reaching it means the condition selected nothing. The success path is pinned alongside the guards, because a node that stopped stopping would pass the guard tests and be useless. 15,552 unit tests green (3 new); phpcs clean.
…ise is stale (#2233) `ObjectWriteNode`'s docblock still carried:⚠️ NOT YET SAFE FOR MUTUAL EXCLUSION — openregister#2212. The underlying guard is a check followed by a write, so concurrent flow runs can all pass the check and several succeed. That was true of #2212 and stopped being true at #2215, where the database arbitrates through the `_uuid` unique constraint and the losing writer is told. Re-measured through THIS node rather than inherited from the fix's own PR: twelve simultaneous flow runs claiming one identifier produced 1 completed / 11 stopped each loser: An object with identifier "…" already exists exactly ONE row in the table The stale warning mattered more than a stale comment usually does. It sat on the one primitive hydra's per-issue lock needs — `hydra-flows-first-port` task 3.1 records per-issue locking as having "no node equivalent" — and it said precisely the thing that would stop somebody building it.
… of over it (#2234) `ObjectWriteNode` replaced the item's json with the written object. Every other node preserves the record and writes its result under an `output` key — `hermiq.agent-step`, `openconnector.source-call`, `openregister.flow-state` all do. This one was the outlier, and the difference is not cosmetic: it discards everything the run was carrying. Replacing is fine for a write that ENDS a branch and wrong for one in the middle of a chain. A per-issue lock is exactly the second shape — hydra's sequencer claims a lock and then still needs the repo, the issue and its slot number to do the work the lock protects. Measured while building it: after the lock write, `{{repo}}` rendered empty and the next call went to `/repos//issues`.⚠️ The step already ACCEPTED an `output` key and silently ignored it, so a flow author could write one, see no error, and get the replace behaviour anyway. With `output` set the record survives and the write lands beside it. Without it, the historical replace behaviour is unchanged — pinned by its own test, because changing that silently would rewrite what every existing flow sees downstream of a write. 15,558 unit tests green (2 new); phpcs, phpstan clean.
…bout objects (#2236) The live catalogue carried fourteen node types and exactly ONE object-shaped step: the write. A flow could create, patch and delete register objects and had no way to READ them. The motivating case is a reaper. hydra's sequencer takes a per-issue lock as an object write with `onConflict: fail` — a real mutual-exclusion primitive since #2215 — but a crashed run leaves its lock behind, and the scheduled flow that would clear stale ones has to begin by LISTING them. Without a read step that flow cannot be written at all. The workarounds were worse than the gap. Calling OpenRegister's own REST API back through `openconnector.source-call` leaves the process, re-authenticates and crosses an HTTP boundary to read a table it is already sitting on top of — and puts OR's API surface into a Source object, where an unrelated URL or permission change breaks the flow silently. Same guards as the write, for the same reason. A flow is authored data: if a flow could READ past RBAC then authoring one would be a disclosure escalation, exactly as writing past it would be a privilege escalation. So the read runs as `context.triggeredBy`, and a run with no resolvable owner reads nothing. Three properties worth naming: - a FAILED read throws rather than returning an empty list. "No objects matched" and "the read did not happen" are different answers, and a reaper that reads the second as the first quietly stops reaping. - every result carries its `uuid`, because that is what a follow-up write or delete names it by and it is not in the record's own fields. - the result set is bounded (default 100, ceiling 1000). A reaper over a runaway lock table should take a batch and come back next tick, not build a million-item walk and time the run out. Filters are templated per item, so two items ask their own questions. The renderer is deliberately local rather than shared: `FlowTemplate` belongs to OpenConnector, and OpenRegister's own node must not depend on an app that may not be installed — `ObjectWriteNode` resolves its `fields` privately for the same reason. Closes #2235. 15,568 unit tests green (10 new); phpcs, phpstan clean.
I shipped the read node returning a LIST under `output`, tried to build the
reaper it exists for, and it did not compose.
Every other node in the engine acts per item, and nothing expands a list inside
an item's json back into items — `openregister.loop` batches the items on the
walk, not the entries of a field. So a read that returns `{objects: [...]}` can
be counted and branched on, and cannot be acted on one row at a time.
Which is the motivating case. A reaper reads stale locks and then DELETES each
one; with the list shape its delete step sees a single item whose `uuid` is not
a field, and fails with "the match value for uuid could not be resolved from
the item". Measured, not predicted.
`fanOut: true` emits one item per object, carrying the incoming record onto each
so the run keeps what it was holding. No matches means no items, which ends the
branch — the same contract `openregister.loop` has for an empty input, and not
an error, because "nothing to reap" is the ordinary case.
The list stays the DEFAULT: it is the right shape for the other half of the uses
— "how many are there", "is there any" — where fanning out turns one decision
into N. Both shapes are pinned.
Refs #2235. 15,571 unit tests green (3 new); phpcs, phpstan clean.
There were no flow rights to find on the rights list because there were none at all. The endpoints are `@NoAdminRequired` and scoped only by organisation, so any member of an organisation could create, edit, delete and run flows, and no admin could narrow that. WHY IT HAD NEVER BEEN ADDED. OpenRegister ships `GenericActionAuthService` for the leaf apps it hosts and never bound one for itself — but the deeper reason is that the matrix could not express this case. It was fail-closed with no "open" value: an unknown action, an empty list and `['admin']` all denied non-admins, so the only thing an action could BE was admin-only. Naming these four would have locked out every non-admin flow author on every instance, which means the matrix was unusable for exactly the operations that are already open — and those stayed invisible and unconfigurable as a result. So `GenericActionAuthService` gains `@authenticated`: an explicit, revocable grant meaning any signed-in user. It is NOT a default — an action with no entry still denies, and that property has its own test, because an "everyone" value that crept into the default would turn a fail-closed matrix into a fail-open one. `flow.create`, `flow.update`, `flow.delete` and `flow.run` are seeded `@authenticated`, which is exactly the access that exists today. Nothing changes on upgrade; what changes is that an admin can now SEE these rights and tighten them. The seed is idempotent — a matrix with entries is preserved, so an admin's tightening survives the next upgrade. Refusal is a 403 RESPONSE, not an escaping OCS exception: that surfaces as a 500 from a plain Controller, and a right that reads as a server fault is one nobody can act on. Verified live: after upgrade the matrix reads all four as `@authenticated`, and creating a flow still returns 201. Narrowing `flow.create` to a group and restoring it was exercised on the instance and the config put back. 16182 tests green, phpcs 0 errors. Positive controls, all three verified red: removing the everyone-grant fails "seeding flow.create locked out a non-admin"; making ABSENCE mean open fails the empty/admin-only test; and making `denyUnless` always allow fails the 403 test.
The @SPEC anchors in FlowController, the seed, the repair step and the tests all pointed at a requirement that did not exist yet. It records the property that makes this safe to ship: seeded '@authenticated' preserves today's access exactly, and absence must keep meaning deny or the matrix goes fail-open.
The agent form's tool picker showed 98 raw ids like `cms_create_page` followed by a truncated sentence, because that is all a descriptor carries: `name`, `description`, `parameters`. Which APP contributed a tool, and what the tool DOES to the data, were not in it. Neither could be recovered downstream. The app is the first segment of the REGISTRY ID, which only this class sees — a consumer parsing it off the name prefix would be inventing a mapping the registry already owns, free to drift from it. A SEPARATE METHOD, and that is the point. `listTools()`' descriptors go to the model as function definitions — `ToolLoop` passes them straight through — so adding `app`/`right` there would put them in a tool-calling payload that strict provider APIs reject. `listTools()` returns exactly what it always returned; a test asserts its keys are still only name/description/parameters. `right` is DERIVED and says so: the verb is read out of the function name, a convention this registry owns. The split handles camelCase, because `decidesk_listOpenActionItems` hides its verb inside one token and a naive split mislabelled 36 of 98 tools. Measured over the real registry: 87 classify, 11 stay `special` — `delegateAgent`, `pipelineForecast`, `upsertSchema` — which genuinely are neither create nor update alone. Unrecognised is never guessed into a CRUD bucket: a tool filed under the wrong right tells an administrator granting access something confident and false. `tool` is the FUNCTION, not its group. Grouping collapsed `cms_create_page` and `cms_create_menu` into one label — "opencatalogi | cms | create", twice — and a picker with two identical rows is one nobody can choose from. Caught in the browser after the first version shipped exactly that. This widens the governed facade surface from two public methods to three (gate-27 / ADR-022), so the contract test, the class docblock and the ai-mcp spec move together rather than the assertion being edited to fit. 16185 tests green, phpcs 0 errors. Positive controls, both verified red: leaking a key into `listTools()` fails "a key leaked into the descriptors the model is sent"; reverting `tool` to the group fails "two tools produced the same label".
The MCP surface exposed "Run a flow" and "Get a flow run status" and nothing else, so an agent could start a flow somebody else had drawn but could never create one. That is the gap that makes "have an agent build and run a synchronisation" impossible today — found by trying it, not by reading the provider. `openregister.saveFlow` closes it: nodes, edges, name, enabled, create or update. It goes through `FlowService`, never `FlowMapper`, because the service is where the organisation scoping, the owner stamping and the per-flow guard live — and an MCP caller is precisely the one that must not bypass them. It returns the PREFLIGHT's `blocking` and `warnings` with the saved flow. An agent that wrote a half-wired document otherwise learns nothing until the run completes having done no work: the engine reports COMPLETED when a path simply runs out of places to go, which is the failure the dead-end check exists to make visible. Handing the findings back at save time is the difference between an agent that can correct itself and one that reports success. The tool's description states the two rules an author cannot guess and the engine refuses outright — a flow needs a trigger and an end node, and an edge carrying a `type` rejects the whole document. `reach` is `instance`, matching runFlow: authoring a flow is authoring something that can later write objects and drive an outbound integration, so its reach is the reach of what it may become rather than of one save. 16185 tests green, phpcs clean.
…e, four anchors pointing at nothing (#2414) * fix(a11y,spec): gate-13 and gate-46 — three dialogs in the wrong place, four anchors pointing at nothing Measured full-scope (workflow_dispatch, run 31459649780) on development, the only honest scope: a push-scope Hydra Gates run marks untouched findings non-blocking and passes over them. gate-13 modal-isolation, 3 findings: Two were already standalone dialog components — RemoveConnectionDialog and CreateConnectedObjectDialog — sitting in src/mail-sidebar/dialogs/ rather than src/dialogs/. ADR-004 names one location, not "a directory called dialogs somewhere". A git mv and two re-pointed importers; the components themselves are byte-identical. The third is real: CacheManagement.vue carried 70 lines of NcDialog markup inline. Extracted to src/dialogs/settings/ClearCacheDialog.vue following the props-in / events-out shape its four siblings in that directory already use (ClearAuditTrailsDialog et al). The radio selection still lives in the settings store, reached through a cacheType prop and an update:cacheType event, so performClearCache reads the same value it always did. NcDialog and NcCheckboxRadioSwitch are dropped from the parent's imports — nothing else there used them — and the five dialog-only CSS rules move with the markup. gate-46 spec-anchor-existence, 4 findings from 4 distinct targets: All four are test docblocks whose @SPEC resolved to no file. Each is repointed to the canonical target the IMPLEMENTATION already carries, so test and code now agree on which requirement they answer to: ObjectExistsExceptionTest openspec/specs/object-crud → objects-crud (typo; the 409-Conflict requirement is in that file) ImportServiceErrorsCsvTest changes/…/tasks.md#task-error-csv → specs/data-import-export/spec.md#import-must- provide-detailed-error-reporting-… , the anchor ImportService.php:2360 uses TenantKeyServiceTest changes/scholiq-deps/… → specs/saas-multi-tenant, where TenantKeyService.php:31 already points ObjectsControllerCountsTest changes/or-batched-object-counts/… → specs/aggregation-api, what counts() itself carries No spec was authored for this change. Every target above already existed; three of the four anchors pointed into openspec/changes/, which is never a valid @SPEC home. Verified with the same gate package CI runs (ConductionNL/.github@main, 78d882a) against the full tree: gate-13 3 → 0, gate-46 4 → 0. The pre-fix run named these exact seven findings and the post-fix run names none. Left red and reported, not silenced: gate-7 (6), gate-57 (3), gate-32 (1). * fix(lint): a scratch-file ignore pattern was swallowing a real Vue component `quality / Vue Quality (eslint)` failed on the previous commit with src/views/settings/sections/CacheManagement.vue 303:30 error "../../../dialogs/settings/ClearCacheDialog.vue" is not published n/no-unpublished-import and the cause is not the new component. `.gitignore` carries a block of patterns for stray scratch files whose names come from shell output — `**/clearCache*`, `**/rebase*`, `**/setup*` — introduced with the explicit warning that they "must NOT match legitimate PHP source filenames". They do not, and git agrees: `git check-ignore -v src/dialogs/settings/ClearCacheDialog.vue` exits 1, because matching is case-sensitive here and the pattern begins with a lowercase `c`. eslint-plugin-n reads the same file CASE-INSENSITIVELY: minimatch('**/clearCache*', {nocase:true}).match('…/ClearCacheDialog.vue') => true minimatch('**/clearCache*').match('…/ClearCacheDialog.vue') => false So the file is committed, is not ignored, lints clean on its own — and every import of it is an error. Proven by substitution rather than by reading: with the component copied to `ZzProbeDialog.vue` and the import repointed, contents byte-identical, the error disappears; repointed back, it returns. The fix is a `!**/*.vue` negation, in the idiom this file already uses for the same problem (`!**/*Analysis*.php`, `!**/*references*.php`). It cannot re-admit what those patterns exist to catch — that output has no extension at all — and it cannot reach into `node_modules/`, since git will not re-include a file whose parent directory is excluded. `git status` is unchanged apart from this file, which is the measurement: no .vue was being hidden. Verified: eslint on CacheManagement.vue reports the error without the negation and reports nothing with it. * docs(mail): the two dialogs moved to src/dialogs/mail — point the component table at where they are The table in docs/features/mail-integration.md is a path index, so a moved file turns it into a map to a directory that no longer exists. Both rows now name src/dialogs/mail/. Nothing else in the tree still says src/mail-sidebar/dialogs. * fix(spec): the extracted dialog dropped two @SPEC exclusions that travelled with the code `quality / Hydra Gates` on this PR: gate-13 PASS, gate-46 PASS — and gate-16 spec-coverage FAIL, 2 changed methods missing @SPEC: src/dialogs/settings/ClearCacheDialog.vue::get — missing @SPEC src/dialogs/settings/ClearCacheDialog.vue::set — missing @SPEC Caused by this branch, so it is fixed rather than merged over. The cause is an extraction artefact, not a new untraced method. On development the identical accessors sat in CacheManagement.vue and each carried its own annotation: get() … @SPEC exclude UI plumbing — computed getter proxies the store set() … @SPEC exclude UI plumbing — computed setter proxies the store Moving the pair into the child collapsed the two per-accessor docblocks into one block on the computed, and the annotations went with the blocks. Restored per accessor, reworded for what they now do: the getter proxies the prop and the setter emits to the parent, rather than both touching the store directly. The exclusion reason is unchanged in substance because the code's job is unchanged — this pair moves a value across the prop/event boundary and implements no requirement of its own. Verified with the gate package CI uses, at the same diff scope (--base origin/development): gates 13, 16 and 46 all PASS.
`openregister.runFlow` answered "No such flow" for every correctly-stored flow. Its guard resolved the id as an OpenRegister OBJECT — `ObjectService->find(register: 'flows', schema: 'flow')` — but flows are not objects. They live in the native `oc_openregister_flows` table, and the flow-storage spec forbids storing a definition as an object outright. So the check consulted a store that only a spec-violating flow could be in, and refused everything else. Found by using it, not by reading it: an agent authored a flow through `saveFlow` and then `runFlow` refused to run the flow it had just created. The tool had presumably never been exercised against a flow this engine actually produces. It now resolves through `FlowService::find()` — the same store `saveFlow` writes and `FlowService::run()` reads, so the guard can no longer disagree with the thing it guards. Organisation scoping is unchanged: `find()` throws for a flow the caller may not see, and both "absent" and "not yours" still answer with one message so the tool cannot be used to enumerate ids. END-TO-END, verified on the live instance: `saveFlow` authors a 5-node sync (trigger-manual → openconnector.source-call /users → explode → object-write → end), `runFlow` queues it (a real run uuid where it previously returned "No such flow"), the worker executes it, and the step log reads trigger1:completed → call1:completed → explode1:completed → write1:completed → end1:stopped. Objects in the target schema went 10 → 20 — real rows from jsonplaceholder, counted in psql rather than inferred from a green status. 16185 tests green, phpcs 0 errors.
…, and the two red CI quality jobs (#2416) Measured full scope with the gate package CI actually runs. `quality / Hydra Gates` is green on development only because a push diffs `event.before...HEAD`, so none of this was ever in scope there. gate-7 no-admin-idor 6 -> 0. Two were real: FederatedConfigController::discover and ::fetch hand a caller-supplied target to the GitHub broker, which signs with the caller's store credential — held BY REFERENCE, secret never exposed — so an ungated fetch(repo, path) was an arbitrary CREDENTIALED GitHub read for exactly the callers install() refuses. Now gated on canInstall, symmetric with the reasoning already shipped for bundle()/publish(). Four were not gaps: shares/revokeShare are enforced at the mapper (findAll() and find() both apply the organisation filter, which fails CLOSED at `1 = 0`; revokeShare reaches it via updateFromArray() -> find(), so another org's id is a 404 rather than a 403 that would confirm it exists), and validate/logActions dereference nothing stored. Exempted with those reasons, and the mapper reason is pinned by a test that goes red if either filter call is removed. gate-57 orphaned-write-capability 3 -> 0. clearInheritFromPublicCache() and clearPermissionCache() were a STATED AUTHORIZATION INVARIANT WITH ZERO ENFORCEMENT, inherited by every app consuming this repo: both memos key on identity with no fingerprint of the policy, so editing a schema's or register's authorization mid-request kept serving the pre-edit verdict. AuthorizationCacheInvalidationListener now evicts on schema/register update and delete. PermissionHandler itself is byte-identical to development — the class was always correct, nothing routed to it. The test pins the INVARIANT, not the call: a planted no-op evictor turns three tests red while the listener test, which only asserts the call is made, stays green. recordUnchanged was a missing branch, not dead code. The streaming loop guessed from the shape of the input and called the unchanged bucket a future enhancement needing "a deep diff against the previous state" — no diff was needed, handleObjectUpdate() already clones the pre-update state for the audit trail. Also fixes a live misreport: a row supplying a uuid for an object that does not exist yet was counted as an update. gate-32 semantic-controls 1 -> 0. BulkTranslateDialog had a hand-rolled backdrop with @click and no keyboard path, and no importer anywhere — rebuilt on NcDialog and moved to src/dialogs/i18n/ per ADR-004. Still unmounted; wiring it is a product decision. CI, both jobs red on development and neither caused here. PHP Quality (phpmd): FlowController::__construct had 10 parameters, fixed by extracting FlowAccess, not by a suppression. PHP Quality (phpstan): FlowMcpToolProvider injected ObjectService and IAppConfig and never read them, left behind when the runFlow guard stopped resolving flows as objects. gate-16 regressed on this branch's own change — the @SPEC exclude sat on the computed property, but a getter/setter pair is two functions to the checker. Fixed rather than merged over. Coverage guard: this change adds 41 statements, so it adds the tests for them rather than a baseline bump. 58.93% -> 58.94% (84795/143876), measured. Every gate move proven in both directions with a planted true positive. 42 new tests. Follow-up filed as #2417: MappingService::invalidateMappingCache() is the same unenforced-invariant defect, still live on a DISTRIBUTED cache, and gate-57 structurally cannot see it because `invalidate` is not in WRITE_VERB_PREFIXES.
…and test the invalidation that is real (#2418) Follow-up to #2416, and a correction to a finding filed during it. Issue #2417 is closed as not-a-defect. #2417 CLAIMED MappingService::invalidateMappingCache() was an unenforced invariant on a distributed cache — stale mappings served across requests. That premise was WRONG. MappingMapper::invalidateCache() already busts the cache on createFromArray, updateFromArray and delete, removing id + uuid + slug against a prefix byte-identical to MappingService::CACHE_PREFIX. The orphan was found by grepping for callers of the public method, without checking whether the mapper enforced it UNDER A DIFFERENT NAME — the exact check gate-57's documentation prescribes, quoted in the issue and not performed. What is true is smaller. getMapping() caches under whatever identifier the caller passed and MappingMapper::find() accepts an id, a uuid OR a slug, so one mapping can hold three cache keys. The public method removes exactly the one it is given: calling it with an id leaves the uuid- and slug-keyed copies live, which looks like a flush and is not one. A footgun rather than a bug — latent until its first caller. Its docblock claimed to be the write-path invalidation; it now says what it is and points at the mapper. What was genuinely missing is the test. The live invalidation had none, and two properties held it up as comments only: - ALL THREE KEYS. Narrow it to the id and two stale copies keep being served. - THE SAME PREFIX. The mapper's constant is annotated "Cache key prefix matching MappingService" — a claim about another file that nothing checked. Drift them and there is no error at either end: the mapper reports a successful invalidation into a namespace nothing reads, and the service serves stale mappings from a DISTRIBUTED cache until TTL. Also pins the write-path coverage and the insert-then-invalidate ordering — getSlug() falls back to `mapping-{random hex}` when the entity has no id, so evicting before persist would remove a key nothing ever wrote. Proven in both directions with planted true positives: id-only invalidation reddens 2 tests, a drifted prefix reddens the prefix test, and removing the call from delete() reddens the write-path test. MappingMapper itself is unchanged, verified byte-identical to development after reverting every plant. Separately, Application.php imported 13 classes from OCA\OpenRegister\Service\Objects\ — A NAMESPACE THAT DOES NOT EXIST. They live under Service\Object\ (singular); the plural survived a rename. All 13 were unused, so PHP never resolved them and nothing ever failed — the first reference would have fataled at boot, in the app's own bootstrap. Removed. The `@package OCA\OpenRegister\Service\Objects\*` docblock tags in 9 other files name the same phantom namespace; cosmetic, left alone. Measurement note: `phpstan analyse <single file>` reported "MappingMapper::$userSession is never read" — a false positive from subsetting. MultiTenancyTrait reads it and was not in the file list. A per-file phpstan run is not the same check as the full run.
491 PHPUnit coverage-cache files were tracked despite .gitignore listing .phpunit.cache three times — they were committed before the ignore rules existed, and adding a rule never untracks what is already in the index. A local test run regenerates the directory, so the files showed up as 491 deletions in every `git status` on a checkout that had run the suite. They are build artefacts: PHPUnit rebuilds them on demand, and the ignore rules keep them out from here on.
… delete
Two changes to the engine, and the coverage that proves it actually executes.
## Synchronous runs
`POST /api/flows/{id}/run` accepts `sync: true` and answers with the FINISHED
run — status, step log and all — instead of a queued row a worker picks up
later. Async remains the default for triggers and schedules.
This is what a person pressing Run wants (an outcome, not a receipt), and it
is what makes the engine testable anywhere. A queued run needs a scheduler,
and every dev stack sets `backgroundjobs_mode=cron` with nothing calling
cron.php — so a test that queues and polls fails against a perfectly healthy
engine, which is the most misleading result a test can produce.
`FlowRunAdvancer` is the worker's `advance()` lifted out unchanged so BOTH
paths run the same code. A separate inline implementation would resolve the
flow, subject and payload seed slightly differently and drift, and a flow
would behave one way under Run and another under cron. The two differ only in
error handling, deliberately: the worker swallows so one poisoned run cannot
stop the queue; sync rethrows, because it is answering a request about THIS
run. Sync still queues first and then advances the queued row — never executes
instead of queueing — so the run record every observability surface reads
still exists.
## Deleting a flow now takes its runs, steps and state
There is no endpoint that lists or deletes a run except by its flow, so the
moment the flow row is gone its history is unreachable through every read path
the app has. Measured before this landed: 493 orphaned runs across 80 already
deleted flows, plus 4 orphaned state rows — and nothing noticed, because
nothing could see them.
Steps are collected before the runs are dropped: `flow_run_steps` keys on the
RUN, not the flow, so deleting runs first strands the steps with nothing left
to name them. A sweep that throws does not fail the delete — the flow is
already gone, and a retry would 404 before ever reaching the cascade.
## Coverage — eight flows that actually run
`tests/newman/openregister-flow-engine.postman_collection.json` creates, runs
and asserts eight real flows: an API sync, an object change plus a real
Nextcloud notification, an AI mailbox summary through an Ollama agent, a
scheduled quality sweep, an enrichment, approval routing, a retention sweep,
and a batched export. Generated from `flow-engine-definitions.mjs`; the
generated file is committed so CI needs no build step.
Every case asserts what the run CHANGED, never just its status. A run reports
`completed` when every step processed zero items — during development one of
these was green with `explode: in=1 out=0` because the path was `body` rather
than `response.body`, and a status-only assertion would have shipped it.
Teardown deletes objects first (register and schema refuse to go while they
hold rows, and there is no bulk delete), and the runner verifies the leak
rather than trusting it: flows/runs/steps counted before and after, plus an
orphan check.
Verified: 76/76 assertions, 0 failures, no rows left behind, no orphaned runs.
489 unit tests green, psalm clean, phpcs clean.
Three engine contracts this pinned down, each of which fails silently:
- a `filter` condition is JsonLogic; the template string form is accepted and
then keeps EVERY item
- `object-write` delete refuses a match resolving to more than one row
- a Source's auth must go in `configuration.headers`; the top-level `headers`
and `auth`/`username`/`password` fields persist and are ignored, so the
source reads as configured while every call returns 401
…kes the shorthand
Two config shapes that were accepted and then misbehaved. Both produced a
GREEN run, which is why each now has a test rather than a reviewer's eye.
FILTER — a condition that is not an expression evaluates to the same answer
for every item, so the step keeps everything or drops everything while reading
like a rule. `FlowExpression::isValid()` accepts any scalar because a bare
literal is legal JSONLogic — true where a value is wanted, useless where a
predicate is. Measured: `'{{ status == "synced" }}'` passed validation and
kept all eleven items; the sweep downstream flagged every object instead of
the nine it meant to, and nothing errored anywhere.
Now refused, with the shape spelled out when the condition looks like a
template. The accepting test is the load-bearing one: without it, a guard that
refused EVERY condition would satisfy both refusal tests while breaking every
filter in the fleet.
MATCH — `{"status": "flagged"}` is what every author writes first, because it
is how `fields` and `filters` are written on neighbouring nodes, and it was
rejected outright. Now normalised to the canonical pair list. The two shapes
are unambiguous: a pair list is a LIST, a map is not, so `array_is_list()`
tells them apart without inspecting values. `[]` stays a pair list meaning "no
match". The refusal message now shows both accepted forms.
Verified live, not just in unit tests: a delete step with the map shorthand
took its register from 1 object to 0.
RETRACTION — I also reported a fourth contract, that `"id-{{ item.id }}"`
returns the raw integer rather than interpolating. That is FALSE and no code
changed for it. `FlowValueTemplate::WHOLE` is anchored, so only a value that
is ONLY a placeholder returns the raw typed value; anything else goes through
preg_replace_callback and comes out a string. Re-measured: it writes `id-1`.
The false claim came from re-reading `flow-runs?flowId=…`, which returns runs
OLDEST FIRST — so I read the original failing run three times and concluded my
fix had not landed. It reached the previous commit message and a summary
before I checked it.
Verified: 492 unit tests, Newman 76/76 with no rows left behind, e2e green.
…#2422) gate-19 full tree, canonical checker 3c8da4c: uncovered 850 -> 847, covered 10 -> 13. Zero new excludes. Every requirement assertion proven able to fail and restored byte-identical. CI E2E: 44 passed, all three new tests named individually in the job log.
…overview read the rows findAll actually returns (#2427) * fix: the reverse-FK recompute leaves the request, and the map overview reads the rows findAll actually returns Two independent defects, both found by measuring rather than reading. 1. openregister#2420 — a synchronous saveObject() inside every object write. `SourceRecordChangeListener::recomputeMaster()` called `ObjectService::saveObject()` inline while handling ObjectCreated/Updated/ Deleted for a reverse-FK source object: a full object write nested inside the write that triggered it, and TWICE on an update that reassigns a source between masters. That is the shape ADR-078 forbids and the shape that serialised every object write on a live instance on 2026-08-11. The recompute now goes through the deferral machinery this repo already has (`ListenerDeferralService` + `ActorForwardedJob`), as `SourceRecordRecomputeJob`. Entries are deduped on the MASTER uuid, so N source objects pointing at one master — the ordinary shape of a reverse-FK relationship, and the whole point of one — enqueue ONE recompute instead of N. The body itself moved to `MasterRecomputeService` unchanged, so the inline branch kept for the `openregister.listener_deferral=inline` kill switch runs exactly the code it ran before. Proven able to fail, expectation computed in advance both times: - dedupeKey -> null EXPECT 1 failure GOT 1 ("null" vs "master-1") - always-inline (the old code) EXPECT 3 failures GOT 3, and the two tests that should survive did. 2. `MapsOverviewService::queryPoints()` returned {"points":[],"count":0} for every register/schema. The loop tested `is_array($row)` on the strength of a comment claiming `findAll()` "returns a list of rendered object arrays". It does not: `ObjectService::findAll()` delegates to `RenderObject::renderEntities()`, declared `@return ObjectEntity[]`, and that method calls `$renderedEntity->setSource(null)` on each row. So every row failed the is_array test, every row was skipped, and GET /api/integrations/maps/overviews/{register}/{schema}/points answered HTTP 200 with zero points — indistinguishable from "nothing has geometry". The existing unit test passed throughout because its double returned plain arrays, i.e. it asserted a contract the collaborator does not have. The added test asserts the real one. With the fix removed it fails (size 0 vs 1) while the original array-based test stays green — which is the demonstration that the old suite could not have caught this. composer check:strict clean on all four files (PHPCS, PHPStan, PHPMD, Psalm); queryPoints' cyclomatic complexity was kept at its threshold by extracting `rowToArray()` rather than by touching a baseline. 16/16 unit tests pass. * fix: FlowRunWorker drops two dependencies nothing reads, and its 12 dead tests run again Pre-existing on `development`, both surfaced by this PR's CI rather than by anything in it. Fixed here per the standing rule to clear pre-existing quality issues encountered during a task. 1. `quality / PHP Quality (phpstan)` was RED on `development`: Property OCA\OpenRegister\Cron\FlowRunWorker::$runner is never read, only written. Property OCA\OpenRegister\Cron\FlowRunWorker::$resolvers is never read, only written. `FlowRunAdvancer` replaced both in ee3448f ("run a flow synchronously"), and the worker's only execution path is now `$this->advancer->advance()` at :330. `FlowRunService` and `FlowLocator` are still injected elsewhere (`FlowController`, `FlowRunController`, `FlowDeadEnd`), so this removes an unused injection, not a class. `FlowItems` was likewise imported and unused. 2. Twelve tests in `FlowRunWorkerExpiryTest` / `FlowRunWorkerStaleTest` had not executed since that same commit: TypeError: FlowRunWorker::__construct(): Argument #5 ($advancer) must be of type FlowRunAdvancer, MockObject_IAppConfig given Both classes still passed the pre-ee3448fea six-argument shape, so `$appConfig` landed on `$advancer` and every test in both classes errored before its body ran. They are rewired to the current constructor, and the five `FlowRunService`/`FlowLocator` "never called" expectations — which named collaborators the worker no longer holds — become one expectation on `FlowRunAdvancer::advance()`, the worker's only execution path. That is a stronger claim, not a weaker one.⚠️ Twelve tests that error are not twelve tests that pass, but the run tally counts them as neither: `ERRORS! Tests: 16241 ... Errors: 12` reads as a red suite, not as a whole test class that has been dead for two commits. Proven able to fail, expectation computed in advance: with `DEFAULT_QUEUED_TTL_HOURS` mutated 24 -> 25, exactly one test fails (`testTheDefaultTtlIsTwentyFourHours`), naming the real cut-off it observed. Restored. 12/12 pass. PHPCS and PHPStan clean on all three files.
…ld fail a test
Asked whether any flow paged an API, the answer was no, and building one found
that no flow COULD: every looping primitive was broken. Four separate defects,
each of which left the node validating, drawing on the canvas and appearing in
the palette — so nothing short of running a flow that contained one could see
them, and no flow on the instance did.
1. THE LOOP COULD NOT RUN AT ALL. `IterateNode` resolves `FlowStepDispatcher`
— the interface, correctly, since it should not know which dispatcher runs
its body — and nothing ever bound it. Every repeat step died on
"Could not resolve …\FlowStepDispatcher! Class can not be instantiated".
Bound to `RegistryStepDispatcher`, its only implementation.
2. THE LOOP RAN ONCE. Later passes seeded the source with ZERO items. A
per-item source does its work once per item, so it did nothing, returned
nothing, and the loop read that as "the source ran out". A paging
source-call fetched page one and stopped — a loop that never looped, while
reporting success.
3. THE PAGE NEVER ADVANCED. The class documents `context['iteration']` as how
a source asks for the right page, but a step's templates render against the
ITEM, not the context, so `{{ iteration.index }}` resolved to nothing.
Measured against a real paging API: identical ids every pass, the source
never ran out, and the loop hit its ceiling and failed with "the source
never runs out" — blaming the API for what the engine had not told it.
The seed now carries the iteration, so the template resolves.
4. A SUB-FLOW COULD NOT SUCCEED. `SubFlowNode` accepted only `completed`, but
`stopped` is the success terminal state — it is what an End node does. The
engine REQUIRES an end node on every flow, so the two rules contradicted
each other: any child that satisfied the connectivity check failed here
with "did not complete (status: stopped)".
## Case 9: a real paginated sync
The suite gains the flow the fleet actually needs: page an external API until
it runs out, handing each page to a sync sub-flow that UPSERTS on the source's
own id.
Its source is a sub-flow, and has to be. A loop ends when its source returns no
items, and a source-call returns exactly one response item whether the page has
records or not — so a bare source-call can never signal "exhausted". The
sub-flow fetches AND explodes, so a page past the end yields zero items.
Proven, not assumed: 100 records over 11 pages, then a SECOND run that leaves
100 — idempotent rather than duplicated. A loop that fetched page one eleven
times would also report success and would also leave objects behind; only the
count separates them, which is why the count is the assertion.
Verified: Newman 186/186 with no rows left behind and no orphaned runs; 476
unit tests; psalm and phpcs clean.
Case 10 exercises `openregister.flow-state` — the node a real incremental sync needs, and one of the ten that had no coverage at all: set a cursor, read it back, record what came out. The value is the assertion, not the run's status. A state node that stored nothing and read back its `default` would report `completed` just the same and write 0 instead of 100 — so the case checks the number that landed on the object. The generator grew a `field` expectation for exactly that: a case can now say which value must survive the round trip, not merely that a row appeared. Verified: the whole collection green, no rows left behind, no orphaned runs. Still uncovered, and worth naming rather than leaving to look done: `route`, `merge`, `map`, `wait`, `trigger-object`, `synchronization-run` and `workload-step`. Three of those need fixtures this collection does not build (a stored Mapping, a configured Synchronization, a checked-out workload ref), and `route`/`merge` need multi-branch graphs — real work, not a line each.
…other people Case 11 splits records down two branches by a rule, writes a different outcome on each, and merges the branches back into one list — `openregister.route` and `openregister.merge`, two more of the ten that had no coverage. A router's `output` is the ID OF THE TARGET NODE: placement asks `itemsForOutput(items, output: $to)`. Naming a branch anything else sends its items nowhere, silently — so the ids are the contract, and the case pins them. BOTH branches are asserted, because either alone proves nothing: a router that sent everything down one path would satisfy a single-branch assertion while doing no routing at all. The split is the claim, so the test states it. Two harness defects this surfaced, both of the same family — a check that stops seeing rather than starts failing: _limit=200 on the assert query. The paginated case's own 100 records plus the routed branches pushed later cases' rows past the page boundary, and the assertion reported "the run was green but wrote nothing" about a flow that had written correctly. Raised to 1000. The leak check counted GLOBAL flow/run/step totals before and after. On a shared dev instance that fails for someone else's work: another agent ran a flow during the window, three step rows appeared, and the check announced that this collection had left rows behind — while it had cleaned up perfectly, measured 零 orphans by both keys. It now counts fixtures BY NAME plus referential integrity, so it can only fail for something this collection actually did. Verified: 301 assertions, 0 failures, 0 fixtures left, 0 orphaned rows.
…ards (#2437) Nextcloud's Entity serves get*() through __call() and declares the accessors only as `@method`, so method_exists() is FALSE for every one of them — including getId(), which Entity itself declares that way. Four guards in this app were built on that probe and were therefore permanently false. MultiTenancyTrait::verifyOrganisationAccess() — the serious one. The guard returned early for Schema, Register, Configuration, Action, Mapping, Webhook, Agent and Endpoint, so cross-tenant write enforcement was silently off for eight of the twelve mappers using the trait, across 24 update()/delete() call sites: no organisation comparison, no cross_tenant_access_denied audit line, no 403. It was live only for Source, View and Application, whose getOrganisation() is concrete — which is why nothing looked broken. The two sibling helpers in the same file already use property_exists() and carry a written post-mortem of this exact bug fifty lines above. ViewPresentationService::objectIdentity() — the de-duplication key merging the calendar view's "starting" and "spanning" queries fell through to spl_object_hash(), which is unique per PHP instance. The same database row hydrated by both queries produced two keys, so any object that both starts in range and spans it rendered twice. LogDanglingLinkedTypes::safeStringAccessor() — every candidate accessor was rejected, so 100% of reported rows read `Schema "unknown" (id=)`. Fixed at the shared seam both call sites funnel into. SearchController::search() — is_array() is false for an ObjectEntity too, so every search hit came back `id: null, name: 'Unknown'`. Unlike the other getUuid probes in this app, nothing on this path routes through getObject(), so there was no fallback. property_exists() is the instrument, not is_callable(): is_callable() is unconditionally true on a __call class and converts a silent skip into a BadFunctionCallException. It is also the test Entity::getter() itself performs. Each fix ships with tests stating in their own docblocks which of them turn red on a revert. Observed: 10 / 2 / 2 / 2 failures, exactly the predicted sets.
POST /api/mappings answered 201 and the very next GET answered 404, for the same session, every time. 186 of 578 mappings on the dev instance were in that state and unreachable by anyone. Two independent faults, both in MappingMapper: 1. Write and read disagreed about organisation. createFromArray() leaves organisation NULL when the session has no ACTIVE organisation, while every read welded `1 = 0` onto the query in exactly that case. RegisterMapper, SchemaMapper and AgentMapper all pass allowNullOrg: true at every read site for this reason — a configuration resource with no organisation belongs to the instance, not to a tenant. MappingMapper was the only one of the four that did not, and ImportHandler had already worked around it by passing includeNullOrg: true at two of its own call sites. 2. find()'s uuid/slug branch could never succeed on Postgres. It built `uuid = ? OR slug = ? OR id = ?` with a non-numeric string, and `id = '<uuid>'` raises "invalid input syntax for type bigint", taking the whole disjunction down. The id disjunct was unreachable-as-useful anyway: the branch runs only when the value is non-numeric. Callers that wrapped the lookup in a catch read the exception as "no such mapping". Tenancy is not widened for tenants that have one: a session with an active organisation still sees only its own rows plus instance-level ones, the same visibility registers and schemas already have. Measured on the dev instance — the session went from 0 visible mappings to 187, all of them organisation NULL, with the 392 org-stamped rows still correctly hidden. openregister.map resolves by id and by uuid as a result. Resolution by SLUG still cannot work, for a third defect left unfixed here: slugs are never persisted (579 of 579 rows are NULL) because getSlug() generates one on read and jsonSerialize() reports it, so the API hands back a slug that no lookup can match. Fixing that needs a uniqueness strategy and a migration for every existing row.
…leak check passing on a query it never ran
Three more node types get a case, taking the collection to 14 cases and 325
assertions:
- map-transform names its mapping by UUID, the resolve path that used to throw,
and asserts the row's name equals 'Ada Lovelace'. `fullName` exists only as
the mapping's output, so a map node that quietly handed items back unchanged
fails this — status alone would not have caught it.
- trigger-object proves a flow whose entry point is an object event still runs
when triggered by hand.
- wait-suspends asserts `suspended`, which is this case's SUCCESS. The generator
grew an `expect.terminal` override for it: asserting the usual terminal set
would have demanded the broken behaviour of a wait that finishes immediately.
Also: the wait case was written with `for: 'PT1H'`. The node takes seconds as a
bare number or a strtotime() phrase, and strtotime('+PT1H') is false, whereupon
it passes the items straight through — a wait that silently does not wait, with
a perfectly healthy-looking run afterwards. The case now uses '1 hour' and says
why.
The leak check had two faults, and together they made it announce a verdict it
had not collected the evidence for:
- The orphan count was global while the fixture count beside it was name-scoped
— the identical trap, one function apart. It duly went red on 12 step rows an
unrelated workstream had orphaned the previous evening. It is now windowed to
rows created since the run started, stamped from the DATABASE clock.
- `tr -d ' '` on that stamp deleted the space INSIDE the timestamp, Postgres
rejected it, 2>/dev/null ate the error, and the empty result printed as `?`
and was read as "no orphans". An unreadable count now fails; it can no longer
pass by default.
Verified the orphan query can still report non-zero before trusting its zero:
0 within its window, 12 against an epoch window.
…ulary (#2421) Adds an x-wire-standard marker to the bag, brp, dso, kvk and ori mock registers, recording that their Dutch schema and property names are the published standards' own and must not be internationalised. Renames nothing: 5 files, 25 insertions, 0 deletions. The ORI marker additionally records that procest declares the same six slugs in its own operational register and that slug resolution is instance-global, so those must be resolved at fleet level rather than renamed on one side. Landed on named cells with the branch first updated onto fe81b38, which turned four stale-base red cells green. The four cells still red are development's own debt, reproduced on its own push run: two phpcs findings and six phpmd findings in files this diff does not touch, a structural gate-24 skip whose reason is byte-identical on development, and the aggregator over them.
… as (#2438) The federated-share visibility filter withholds non-public objects from non-object-scope shares. It read exactly ONE property name: $confidentiality = strtolower((string) ($object['confidentiality'] ?? '')); if (in_array($confidentiality, self::PUBLIC_CONFIDENTIALITY, true) === false) while the same concept is written under two others: - `confidentialityLevel` — the target SeedZgwZakenMigrationPack maps `/vertrouwelijkheidaanduiding` onto; - `vertrouwelijkheidaanduiding` — the ZGW/GGM schema property itself. THIS FAILS OPEN, NOT CLOSED. `?? ''` yields the empty string for an object storing its level under either other name, and `PUBLIC_CONFIDENTIALITY` is `['', 'openbaar', 'public', 'open']` — the empty string is IN it, because an object with no level set is public. So an object marked `zeer_geheim` under a name this guard did not read was served as public over federation. One concept, three spellings, and the mismatch is silent in both directions: nothing errors, nothing logs, and a response-shape assertion cannot see it because the field is absent rather than wrong. THE FIX reads the first present, non-empty key from an explicit alias list. "Present AND non-empty", not merely present: a schema sync can add an empty column before anything writes to it, and an empty `confidentiality` sitting in front of a populated `confidentialityLevel` would reinstate the same fail-open. The same alias-reading pattern already guards organisation two lines above. DIRECTION OF CHANGE. Measured across seven cases, three moved SHARED to blocked and NONE moved blocked to SHARED. The change can only ever withhold more, never expose more. Objects with no level set stay public, which is the intended meaning of the empty string. VERIFIED - phpunit: 14 tests, 20 assertions green across the new suite and the existing FederationControllerScopeTest, on PHP 8.3 (this app's floor; the host runs 8.2 and composer's platform check refuses it). - Positive control: reverting the guard to the single-key read turns the new suite RED at 4 of 8 — the four leaking rows — and restoring it returns 8/8. A first attempt at this control silently did not apply the revert and re-tested the fixed code twice; the run above is the one that actually flipped the source. - phpcs: 0 errors on both files. FederationController carries 7 warnings both before and after this change. The related finding is NOT addressed here: ZaaktypeAuthorizationService — the ZGW authorization mapper that owns confidentiality ordinal ordering and buildConfidentialityMatch() — has zero call sites in lib/. All seven of its public methods are exercised only by their own unit test, while the archived change 2026-06-14-rbac-zaaktype marks those tasks complete and describes the service as the enforcement path. That needs its own investigation. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ating it (#2439) * fix(tests): pull in the helper trait four Flow tests use but never require `composer test:unit` FATALS outside a Nextcloud checkout: Fatal error: Trait "…\FiltersFlowLevelFindings" not found in tests/Unit/Service/Flow/FlowNodeConfigDialectTest.php on line 61 Not a new break — it reproduces on an untouched `development`. The suite matches `*Test.php`, so a helper that is not itself a test case is never included, and composer declares no `autoload-dev`, so nothing autoloads the test namespace either. In CI the trait resolves only because `tests/bootstrap.php` requires the SERVER's `tests/autoload.php` when it can find a Nextcloud root. The cost was not the four files: PHPUnit aborts the whole run at the first fatal, so the ENTIRE 16,303-test unit suite was unrunnable locally, and the 51 tests in these four files never ran anywhere the server autoloader was absent. Fixed the way the AppHost and Mcp fixtures in this repo already do it — an explicit `require_once` in each consumer. An `autoload-dev` PSR-4 rule was tried first and rejected twice over: 572 test classes use namespaces that do not correspond to their paths (composer prints a skip warning for each), and a `classmap` over `tests/` would make `tests/stubs/`'s OCP stubs autoloadable, which has bricked an instance here before. Verified: 16,303 tests, 0 failures, 0 errors. * style: clear the two pre-existing phpcs errors on development `lib/AppInfo/Application.php` indented a quoted error message three spaces inside a `//` comment run; `ObjectWriteNode.php` had a 166-character translatable string on one line. The long line is wrapped as concatenated literals inside `t()` — the form already used elsewhere in the same file (line ~1264), so the extractor still sees a literal. Assigning the message to a variable first would have hidden it from translation extraction entirely. * feat(flow): give a flow's rationale a column, so the database stops eating it A flow authored as a definition file carries its reasoning in a top-level `$comment`. On hydra's lock reaper that is 90 lines recording four defects and what prevents each recurring. `openregister_flows` had no column for it, so importing such a file and regenerating it FROM the database returned a flow without that text — silently, because a flow with no rationale looks exactly like one whose author wrote none. The standing workaround was to regenerate by MERGING file and database rather than exporting. That kept the text alive but made the FILE its only home: a flow edited through the UI could not carry a rationale at all, and two authors working on the same flow by different routes disagreed about why it was shaped that way with nothing to reconcile them. - `comment` TEXT on `openregister_flows`, nullable. TEXT because the existing bodies already exceed 6,000 characters and a length-capped column would truncate on write rather than refuse it. - `Flow::$comment`, in `addType()` and in `jsonSerialize()`. - `FlowService::applyEditableFields()` accepts `comment`, and normalises the `$comment` a definition file actually uses — that key cannot be a column name, so it is translated once here rather than at every call site. An explicit `comment` wins over the alias: a UI edit is a deliberate write, `$comment` is whatever the file happened to carry. Distinct from the two fields it sits next to: `description` is the one-line label the UI lists a flow by, `notes` is a working scratchpad. Six tests, positive-controlled: removing the field mapping turns four of them red, and the fifth (a partial update must not blank a stored comment) is deliberately driven through the UPDATE path — on a create the field starts null, so the same assertion there would hold no matter what the code did. Its counterpart pins that an explicit null still clears the field. Closes the decision recorded on #529.
…eaves, not a 500 (#2441) `GET /api/objects/{register}/{schema}/{id}/integrations/{integrationId}` with an id matching nothing answered 500, `DoesNotExistException: Object not found in magic table`, instead of the 404 `guardObjectAccess()`'s own docblock promises. `setObject()` is not a setter. It calls `objectMapper->find()`, which throws when nothing matches — and it sat one line ABOVE the `try` whose catch already returns 404. The `getObject()` call the catch did cover is only reachable once `setObject()` has succeeded, so the guard was wrapped around the wrong call. Moving the three resolution calls inside the try fixes all five verbs, which share the guard. The existing test named "denies inaccessible object" could not catch this: it stubs `getObject()` alone, and on a mock `setObject()` is a silent no-op, so the throwing path was unreachable from the suite. A green test with the right name was standing over a 500. Found from the other end. openconnector's `synced-from-leaf` e2e spec asks this endpoint about a deliberately absent uuid and expects an empty result; it had never run, because it skips unless `openconnector.storage_migrated` is true, and no fresh install ever set that flag (ocon#1180). Fixing the flag ran the spec, and the spec found this. Reproduced live on the dev instance, which has the flag set: HTTP 500, same exception. Six tests, positive-controlled: reverting the guard turns all five new ones red. `missingObjectService()` throws from `setObject()` the way the real service does, rather than stubbing the call that comes after it.
The Code Quality run on development is red in six jobs. These are the two I put there — phpcs and phpmd — and `lib/` now passes both with zero findings. phpcs: a 166-character message string in ObjectWriteNode, and a three-space indent inside a comment block in Application.php. phpmd: five findings, all suppressed with the reason rather than refactored, because in each case the rule is describing a deliberate choice. - FlowService coupling/parameter count: it is the single seam between the flow API and the five stores a flow touches. A facade to satisfy the count would depend on all of them itself. - FlowService::run and FlowRunAdvancer::advance boolean flags: both select WHO handles the work, not what it is. Queued either way; sync just advances the run itself rather than leaving it for cron, and lets the failure out so the HTTP response can carry it. - IterateNode::seedFor static access: FlowItems::item is the item constructor every node uses. A factory would give the engine two ways to build an item. - ObjectWriteNode::matchPairs complexity: one branch per REJECTED shape, each naming what the author wrote and what to write instead. The remaining red jobs (PHPUnit on 8.3 and 8.4, Hydra Gates, Quality Report) are not addressed here and I have not established whether they are mine.
…y retire (#2442) `FlowNodeRegistry` resolves two old ids through an alias — `openregister.loop` → `openregister.batch`, `openregister.stop` → `openregister.end` — and its own docblock says the alias is removed one release after the rename. Nothing ever rewrote the stored definitions. Measured on the development instance: 13 nodes across 19 flows still named `openregister.stop`. They work, silently, on the alias. That is a deadline nobody can see — the definitions look fine, the runs succeed, and the breakage arrives with a release that touches none of them. The map is READ FROM the registry via a new `renamedTypes()` rather than copied into the repair step, which makes the retirement order correct by construction: drop a pair from `RENAMED` and this step stops rewriting it in the same commit. A second copy would keep rewriting a name nothing answers to. - Writes only flows that actually carried an old name. A blanket save would move every flow's `updated` timestamp for a no-op, which is the churn that makes "what changed and when" unanswerable — and would make a second run indistinguishable from a first. - Registered ahead of `BackfillFlowTriggerIndex` in both `<install>` and `<post-migration>`: the index is derived FROM node types, so the types should already be current when it runs. - Never throws. An upgrade that aborted over a node-type rewrite is a worse outcome than a flow staying on an alias that still resolves today. Seven tests, positive-controlled: removing the rewrite turns six red. They cover both pairs, a no-op flow, a malformed node entry, and idempotency — and one drives every pair the registry declares, so a future rename is covered without editing the test.
…ained skip (#2444) Every guarded case already set `<case>_skipped` when its prerequisite was absent. Nothing ever read it. The only trace of a skip was a `console.log`, so a run in which a case never executed reported exactly the same all-green summary as one in which it did. That is how `hermiq.agent-step` came to be described as "executed through the engine". It is — on an instance that has hermiq and Ollama. openregister's CI installs neither: the fixture request POSTs to `/objects/hermiq/agent`, fails, leaves `agent` empty, and closes with `pm.expect(true).to.be.true`. So the one case covering hermiq's contribution to the engine has been skipped in every CI run, invisibly, while the suite reported green. The new `98 — coverage` folder does two things: * names the count in a TEST title, so "N of M flow-engine cases executed" lands in the run summary rather than only in stdout * FAILS when a case skipped without declaring a prerequisite The second is the part that matters. A case may only be absent when it said what it needs; anything else skipping is a hole, and a hole must not pass as green. Today that check is satisfied — the single skip declares `requires: 'agent'` — which is exactly the state it exists to keep honest. Refs ConductionNL/hermiq#185, whose coverage table records `agent-step` as engine-executed and `workload-step` as not. The truer statement is that on CI NEITHER has been.
Two independent reds on `development`, neither of them a defect in shipped behaviour, both of them a test/spec artefact that CI is right to refuse. 1. PHPUnit (PHP 8.3 and 8.4, NC stable32) — ONE failing test out of 16306. a5b8dc2 ("a mapping you just created was invisible to every read") deliberately made MapNode::resolve() ask find() FIRST for a non-numeric reference, because find() is the only lookup that consults the uuid and slug columns. MapNodeTest::testANonNumericReferenceResolvesByRef still pinned the OLD order with `expects($this->never())->method('find')`, so the fix and its own suite disagreed: Expectation failed for method name is "find" when invoked 0 times. Method was expected to be called 0 times, actually called 1 time. The test's intent — a name living in the `reference` column alone still resolves — is unchanged and worth keeping. It now reaches findByRef() the way production does: through find() MISSING. That fall-through is the part that actually carries an exported flow, so making find() throw is a stronger test than making it unreachable. testAUuidReferenceResolvesThroughFind is added because nothing pinned the new order at all. Its `never()` on findByRef() is the assertion: a resolve() that went back to consulting the `reference` column first would satisfy every other test in the file. POSITIVE CONTROL. At the tree before this commit the suite is 8 tests, 1 failure — the exact CI message. With this commit, 9/9 green. Deleting the find()-first block from MapNode::resolve() again turns it into 1 failure + 1 error, on exactly these two tests, with the predicted text ("expected 1 time, actually 0" and RuntimeException "No mapping matches ..."). MapNode.php is restored byte-identical; it is NOT touched here. 2. Hydra Gates — [gate-46] spec-anchor-existence, 3 findings, 1 target. #2438 tagged three methods `@spec openspec/specs/federation/spec.md`. That file did not exist: federation's only written specs live in openspec/changes/federation-scope-enforcement and openspec/changes/federated-config-sharing, and a change directory is not a canonical target. The gate says "fix the TARGET, not each tag", so the canonical spec is written rather than the tags retargeted. Its content is read off the code it describes — CONFIDENTIALITY_KEYS, PUBLIC_CONFIDENTIALITY and applyShareVisibility() — and its scenarios are the seven data-provider cases that already exist in FederationControllerConfidentialityTest plus the object-scope bypass. Nothing is invented. The two requirements carry a reason-bearing `@e2e exclude` naming that test file. Without it this commit would have traded gate-46 for gate-19: measured, the spec's 7 new scenarios fail gate-19 as "missing @e2e" and pass with the excludes. POSITIVE CONTROL. check_spec_anchors.py against the two tagged files reports exactly the 3 CI findings when the spec is moved aside and 0 when it is present. check_e2e_coverage.py reports FAIL 7 without the exclusions and PASS with them. check_spec_coverage.py (gate-16) is 0 either way — nothing here is in its scope. NOT FIXED HERE, and deliberately so: phpmd's 6 findings are all in lib/Service/Flow (FlowRunAdvancer:83, FlowService:55/78/480, IterateNode:238, ObjectWriteNode:1445), which is another session's live work under #2429. phpcs's two errors were fixed by that session in 61c9f38/9c05a3f44 while this was in progress — verified here, full-tree phpcs is 0 errors on 1428 files at that base. Quality Report is a pure aggregator and carries no finding of its own.
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
4 participants
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.
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.