Release: merge development into beta

[github-actions]

Automated PR to sync development changes to beta for beta release.

Merging this PR will trigger the beta release workflow.

Reminder: Add a major, minor, or patch label to this PR to control the version bump. Default is patch.

[rjzondervan]

Verdict: REQUEST_CHANGES (Strict), re-review of the 9 commits since 0dbb6af

Both earlier points are fixed. A numeric iss now goes through the weak-secret guard with the same (string) cast OpenRegister uses, a non-scalar is refused, and the tests would fail if either were reverted. HS384 and HS512 now have the same refused/accepted boundary tests as HS256. Threads resolved.

One new 🟡: the subscriptions paging fix leaves limit/offset unclamped. The rest of the delta is clean: the contracts and forward fixes are correct and their tests fail on the old code, and the E2E changes add assertions rather than remove them. CI is green on dbf95d5.

[rjzondervan]

🟡 Concern — A collector outage ties up cron: about 40 s per failed trace, with no backoff or circuit breaker

Each send may block for TIMEOUT_SECONDS = 10 (OtlpTraceExporter), and a failure re-queues immediately, up to MAX_RETRIES = 3. failed traces always pass sampling (OtelSettings::isSampled). A partner outage produces many failed traces, and a collector outage at the same time turns that into N × 40 s of serial cron work, starving other apps' background jobs.

Fix: a circuit breaker (after K consecutive failures, skip sends until a cooldown passes), delayed retries instead of an immediate re-add, a lower timeout, and a cap on the queue depth.

Verification: this file at d9c62df lines 54 and 110-111 → MAX_RETRIES = 3, re-added at once with jobList->add(…, 'attempt' => $attempt + 1). Not load-tested.

[WilcoLouwerse]

Fixed in #2635 (fa09f56): after five failed sends in a row, sends and queuing pause for five minutes; retries wait 5, 10 and 20 minutes; the collector timeout is 3 s instead of 10 s.

[rjzondervan]

⚠️ Partly addressed in fa09f56 (#2635): the breaker and the shorter timeout are in, but the retry doesn't really wait (#2154 (comment)), and a pause drops traces without counting them and uses up retries (#2154 (comment)). Leaving this thread open until both are fixed.

[rjzondervan]

🔧 The remaining half of this (the retry that doesn't wait and the pause that burns attempts) is addressed in #2638; see the f7 and f10 threads. The thread stays open until it is merged.

[rjzondervan]

🟢 Minor — A contact moment can be recorded for any callId

findEnded() filters on callId, kind and sourceId only, so any kiss.push holder can attach a contact moment to a colleague's call. The first one recorded wins, and later pushes get the existing kissId back. REQ-007 does not require binding the call to the agent, and the caller's number is not exposed, so this is a note. Consider checking agentId against the user's agent mapping once one exists.

[WilcoLouwerse]

Left as is in #2635, as a note until an agent mapping exists.

[rjzondervan]

Verdict: REQUEST_CHANGES (Strict), re-review of dbf95d5 → d9c62df

The limit/offset clamp is deferred to #2634 as agreed. This round covers the merge of #2631 (approvals in the shared task inbox, OpenTelemetry export, CTI call log), which landed on development without a review. This is its first one.

Two blockers:

• 🔴 any logged-in user can expire or dead-letter a pending approval through a forged or mis-completed OpenRegister task
• 🔴 an inbound traceparent sets the trace record's uuid, so an anonymous caller can overwrite another execution's trace

Concerns: 🟡 onReject: dead_letter and other rejecting outcomes, 🟡 a cancelled mirror leaves the approval pending forever, 🟡 a BSN and URL credentials in url.full, 🟡 no backoff on collector outages; plus a 🟢 note on contact moments.

Fine as written: the approve/reject path keeps every check the old controller had, the OpenRegister test stubs match OpenRegister beta, the mirror carries no approval data, and retention uses OpenRegister's archival task. CI is green on d9c62df. Until the two 🔴 are fixed on development (or #2631 is reverted there), this should not go to beta.

[rjzondervan]

🟡 Concern: a retry that is not yet due is put back as an immediately runnable job, so the worker keeps picking it up

QueuedJob::start() removes the row by id before run(). run() then calls jobList->add(self::class, $argument), which inserts a new row with last_checked = time() (JobList::add, $firstCheck defaults to now). JobList::getNext() selects any row with last_checked <= now, so the job can run again at once. Under occ background-job:worker (only usleep(50000) between jobs, no executed-ids guard) a waiting retry is picked up about 20 times a second, for 5, 10 or 20 minutes. Each pass deletes a row, inserts one and records a job run, for every trace waiting on a retry during a collector outage. That is the cron load the original finding was about. Nextcloud ≥ 28 has IJobList::scheduleAfter($job, $runAfter, $argument), which sets last_checked to the run time, and Integriq requires NC ≥ 32.

Fix: in retryOrDrop(), schedule the retry with $this->jobList->scheduleAfter(self::class, $notBefore, $argument), and remove the not-due branch in run(), or keep it only as a guard that also uses scheduleAfter. Change the test to assert scheduleAfter with the delay instead of add.

Done when: no code path re-adds an OtelExportJob with last_checked = now before its notBefore, and a test asserts the delayed retry goes through scheduleAfter with the expected run time.

[rjzondervan]

🔧 Proposed fix in #2638, commit 2e9c221: retries, the not-due guard and the paused path now go through IJobList::scheduleAfter() at their run time, and a test asserts add() is never used for a retry. The thread stays open until it is merged.

[rjzondervan]

🟡 Concern: the identifier mask lets a formatted BSN, an e-mail address and letter-only tokens through to the collector

looksLikeIdentifier() masks a segment only if it has a run of 4+ digits, or is 20+ characters of a token alphabet that includes a digit. I ran the extracted method on the host:

• /ingeschrevenpersonen/999.993.653 and /999-993-653 are exported unchanged. Both are common ways to write a BSN, and the spec scenario "a BSN in the URL path never reaches the collector" (specs/execution-trace/spec.md:73) says the payload must not contain it.
• /klanten/j.devries@gemeente.nl is exported unchanged (personal data).
• /services/abcdefghijklmnopqrstuvwxyz (a path token without digits, e.g. a webhook secret) is exported unchanged.
• A host label such as 999993653.example.org is kept (less likely, noted for completeness).

Fix: count digits after removing ., - and spaces (e.g. preg_match('/\d{4,}/', preg_replace('/[\s.\-]/', '', $segment))), mask any segment containing @, and mask long tokens whether or not they contain a digit. Better still, export a route template or only server.address + method. Add these cases to testABsnInThePathAndUrlCredentialsNeverReachTheCollector.

Done when: a dotted BSN, a dashed BSN, an e-mail segment and a long letter-only token are all masked, and the test asserts each one.

[rjzondervan]

🔧 Proposed fix in #2638, commits 4212edf + 992f266: the segment is URL-decoded and stripped of every non-alphanumeric before the 4+-digit test, and @ segments and opaque tokens are masked. The test covers dotted, dashed, _, ,, +, NBSP-encoded, %2F and double-encoded BSNs, plus an e-mail address, and checks that ingeschrevenpersonen stays. The thread stays open until it is merged.

[rjzondervan]

🟡 Concern: while the breaker is open, new traces are discarded without a log line or counter, and waiting traces use up their retries without a send

1. TraceExportQueue::queue() returns false while breaker->isOpen(). Nothing is logged or counted, and this includes failed traces, which the sampler otherwise always keeps. Each 5-minute pause therefore leaves a gap in the collector that an operator cannot see or measure.
2. OtelExportJob::send() throws "Sends are paused" while the breaker is open (line 159-161), and run() sends that to retryOrDrop(), which uses up one of the three attempts. A trace waiting during the pause loses attempts without being sent. After about 35 minutes of outage every trace is dropped, and the drop log says "after 4 failed sends" even when fewer sends were made. Concurrent cron workers also read and write otel_breaker_failures in IAppConfig without locking, so counts can be lost (low impact).

The spec now permits "no new trace is queued meanwhile", so (1) is by design, but the original concern asked for a bounded queue, not unaccounted loss.

Fix: while paused, reschedule a waiting job to open_until without using up an attempt. Count traces skipped at queue time (e.g. an otel_dropped_total app-config counter shown in the admin settings, or one warning per pause window). Make the drop log report the real number of sends.

Done when: a paused send does not consume a retry attempt, every trace skipped or dropped during a pause is counted or logged, and a test covers both.

[rjzondervan]

🔧 Proposed fix in #2638, commits ec06475 + 58ae385 + 1374f45: a paused send keeps its attempt and is rescheduled to the end of the pause. Skipped traces are counted with a distributed-cache inc(), with no app-config write per trace, and one warning per pause carries skippedTotal. Showing the counter to operators is left to the author (see the PR body). The thread stays open until it is merged.

[rjzondervan]

🟢 Minor: an outcome like Rejected or Approved closes the mirror but decides nothing

OpenRegister classifies rejections with in_array(strtolower(trim($outcome)), REJECTING_OUTCOMES, true) (TaskState.php:174), but stores the outcome exactly as sent (completeInternal sets $recordedOutcome = $outcome). The listener compares exactly (=== 'approved', strict in_array). An API client that completes with Rejected or denied therefore closes the mirror as a rejection (comment enforced) while the record stays pending until the local sweep expires it. The same happens for Approved. The UI probably sends lowercase values, so impact is low.

Fix: normalise with strtolower(trim(...)) before decision(), as OpenRegister does, and add a mixed-case case to testEveryRejectingOutcomeAndARoutedRejectionReject.

Done when: the listener classifies outcomes with the same normalisation OpenRegister uses, and a test covers a mixed-case outcome.

[rjzondervan]

🔧 Proposed fix in #2638, commit dc1e7d8: the outcome is normalised with strtolower(trim()) before it is classified, and Rejected, Denied and Approved are tested. The thread stays open until it is merged.

[rjzondervan]

Verdict: REQUEST_CHANGES (Strict), re-review of d9c62df → 73e44a7 (fix PR #2635)

#2635 fixes both 🔴s and three of the four 🟡s from round 5, but its own fixes add 3 new 🟡s and 1 🟢, and the OTel backoff thread is only partly addressed. 6 threads are open, and the beta ruleset needs every thread resolved and 1 approval before merge; #2634 (the clamp) is still a release-blocker for beta → main.

🔧 Fix PR: #2638 — 4 of 4 findings, one commit each (plus 3 pre-push: commits)

Merge it into development, then re-request the review.

Round	Found	Status
1 (b0f9d27)	🔴 3: l10n strings, retour contract tests, gate-23	fixed
2 (539970e)	🔴 1 credential-validation regression, 🟡 1 release ordering	fixed (#2618, OR setup check)
3 (0dbb6af)	🟡 numeric iss skips guard, 🟢 test boundaries	fixed
4 (dbf95d5)	🟡 unclamped subscriptions limit/offset	deferred to #2634 (release-blocker before main)
5 (d9c62df)	🔴 2, 🟡 4, 🟢 1 in #2631 (approvals via task inbox, OTel export)	5 fixed in #2635, 1 partial, 🟢 left as-is
6 (73e44a7) — this review	🟡 3, 🟢 1 in #2635's fixes	open

Finding	Was	Now	Where
🔴 Forged task expires an approval	🔴 round 5	resolved in 8132104	thread
🔴 Inbound traceparent sets the record uuid	🔴 round 5	resolved in efb0558	thread
🟡 dead_letter and other rejecting outcomes	🟡 round 5	resolved in 8132104	thread
🟡 Cancelled mirror leaves approval pending	🟡 round 5	resolved in c19fd31	thread
🟡 BSN and credentials in url.full	🟡 round 5	resolved in ab8672e (gaps: f8)	thread
🟡 OTel export has no backoff	🟡 round 5	still open, partly fixed in fa09f56 (see f7, f10)	thread
🟢 Contact moment for any callId	🟢 round 5	still open, left as-is by agreement	thread
🟡 f7: a retry that isn't due yet runs again straight away	new	new — 🟡	comment
🟡 f8: url.full masking misses a formatted BSN and e-mail	new	new — 🟡	comment
🟡 f10: a paused breaker loses traces and burns retries	new	new — 🟡	comment
🟢 f9: outcome match is case-sensitive	new	new — 🟢	comment

Details of this round

Scope. The delta since round 5 is exactly #2635 (6 commits plus merge 73e44a7, 25 files). #2635 was merged with no review, so this round is a full Strict analysis of it, not the fix-PR fast path.

Verified clean. The forged-task test uses forged-1 against taskUuid task-1, and fails on the old code. OpenRegister sets a flow-timer: completer only on the timer's skip, so a user cannot produce one. or_tasks_uuid is a unique index, so no second task can share the mirror's uuid. TaskService::get() is a plain findByUuid, so "missing" and "unreadable" are told apart correctly. The inbound traceparent is still validated (version 00, exact hex, all-zero ids refused). Records without the new fields fall back to the local sweep. The new en/nl strings match their use.

How the new findings were checked. f7: core JobList::add() sets last_checked to now, getNext() takes last_checked <= now, and QueuedJob::start() removes the row before run(); IJobList::scheduleAfter() is @since 28.0.0, and integriq needs NC 32 or later. f8: I extracted SpanMapper::safeUrl() and ran it on the host; 999.993.653, 999-993-653, j.devries@gemeente.nl and a 26-letter token come out unchanged. f10: TraceExportQueue::queue() returns false while the breaker is open, with no log, and send() throws so that retryOrDrop() uses up an attempt.

Connected issues. #2631 (merged): the source of round 5's findings, unaffected. #2635 (merged): the fix PR reviewed here. #2634 (open): the clamp, labelled release-blocker before beta → main, unaffected by this delta.

CI: all checks green on 73e44a7, including quality / Hydra Gates, PHPUnit (8 runs) and Newman. Gates: taken from that CI run, so no local run.

Before the next round. This round's findings were mostly fix-induced (fix-induced 3 + missed-sibling 1 ≥ new 0), so next round's fixes go through the class sweep and the pre-push check first. Checks to run before asking for a re-review:

• f7: a test asserts scheduleAfter(OtelExportJob::class, <notBefore>, …), and grep -n 'jobList->add' lib/BackgroundJob/OtelExportJob.php finds no not-due re-add.
• f8: safeUrl() masks 999.993.653, 999-993-653, a.b@c.nl and a 20+ letter token, each asserted in testABsnInThePathAndUrlCredentialsNeverReachTheCollector.
• f10: a send while the breaker is open does not increase attempt, and dropped traces are counted or logged.
• f9: completing with Rejected rejects the record.

🤖 Reviewed with Claude Code