Repository navigation
fix(review): #2154 review findings, round 6 - #2638
Merged
Merged
Conversation
… immediately runnable job #2154 (comment) OtelExportJob now schedules a retry with IJobList::scheduleAfter(), so the job row's last_checked is the retry's run time (five, ten, twenty minutes out) and the worker does not pick it up before then. The not-due guard in run() also uses scheduleAfter() with the argument's notBefore instead of add(), so no path re-adds the job with last_checked = now. The tests assert scheduleAfter() with the expected run times (1300/1600/2200, and 1001 for the not-due guard) and that add() is never used for a retry. Sweep: the other IJobList::add() calls in lib/ (TraceExportQueue, EventService, DsoIngestService, JobService) queue new work to run now; none re-adds a delayed job, so no other site changed. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e-mail address and letter-only tokens through #2154 (comment) SpanMapper::looksLikeIdentifier() now masks a URL path segment when: - it contains `@` (an e-mail address); - it has a run of 4+ digits after dots, dashes and whitespace are removed (999.993.653, 999-993-653, "999 993 653"); - it is a token-alphabet string of 32+ characters, or of 20+ characters that holds a digit (the old rule) or mixes upper and lower case. A lowercase route word under 32 characters stays, so the spec scenario's `/ingeschrevenpersonen/{id}` holds. Trade-off: a lowercase route word of 32+ characters, or a camel-case one of 20+, is masked as well. New test testFormattedIdentifiersAndOpaqueTokensAreMaskedRouteWordsStay asserts url.full for a dotted, dashed and spaced BSN, an e-mail segment, a 32-letter token, a 20-letter mixed-case token, and that `ingeschrevenpersonen` and `zaakinformatieobjecten` survive. Each rule was mutated out in turn and the test failed each time. Sweep: SpanMapper::safeUrl() is the only URL/attribute redaction in lib/Observability; nothing else changed. The host-label case in the finding (999993653.example.org) is not addressed: the host is kept. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ded silently and retries are used up #2154 (comment) (a) OtelExportJob::run() now checks the breaker before sending: while sends are paused it reschedules the same attempt with IJobList::scheduleAfter() at the breaker's open-until time, so a paused send no longer reaches retryOrDrop() and no attempt is used up without a send. send() no longer throws "Sends are paused". (b) TraceExportQueue::queue() checks sampling before the breaker, so only a trace that would have been exported is counted as skipped. OtelExportBreaker::recordSkipped() increments the otel_skipped_total app setting and returns true for the first skip in each pause window (tracked by otel_skipped_warned_until = open-until); the queue logs one warning for that one, not one per trace. New breaker method openUntil() backs isOpen() and both callers. (c) The drop log line reports ($attempt + 1) failed sends instead of the constant MAX_RETRIES + 1; with (a) every attempt is a real send try. Tests: testACollectorOutageOpensTheBreaker asserts a paused send is scheduled at the pause end with its attempt unchanged (0 and 2); testTracesSkippedDuringAPauseAreCountedAndLoggedOncePerPause asserts the counter (2, then 3), one warning per pause, and a new warning for a new pause. Reverting OtelExportJob.php or TraceExportQueue.php to the previous commit fails the respective test. The drop-log assertion ("after 4 failed sends") also passes on the old code, since the normal path drops at attempt 3. Not changed: the unlocked read-modify-write of otel_breaker_failures across concurrent cron workers (the finding rates it low impact). Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…but decides nothing #2154 (comment) SharedApprovalTaskListener::resolve() now normalises the task outcome with strtolower(trim()) before any classification, as OpenRegister's TaskState does for REJECTING_OUTCOMES. This covers decision() (approve / reject) and the timer-outcome check, so `Rejected`, ` Denied ` and ` Approved ` now decide the record instead of leaving it pending. Tests: testEveryRejectingOutcomeAndARoutedRejectionReject adds `Rejected` and ` Denied ` (7 rejections); testAnApprovalInTheShared InboxResumesTheRun adds ` Approved ` (2 approvals). Both fail with the previous listener. Sweep: the listener is the only place in lib/ that reads an OpenRegister task outcome; ApprovalService/ApprovalTaskMirror only write outcomes, and the other `outcome` comparisons (ContractCommitNode etc.) are Integriq's own values. Nothing else changed. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…coding #2154 (comment) looksLikeIdentifier() now URL-decodes the segment three more times (nested rawurldecode; a no-op once nothing is left to decode, and no added class complexity, which the helper-method version pushed over the phpmd limit of 50), then tests for `@` and, after removing every character that is not an ASCII letter or digit (byte-wise, so a UTF-8 NBSP goes too), for a run of 4+ digits. Newly masked: 999_993_653, 999,993,653, 999+993+653, 999%C2%A0993%C2%A0653 (NBSP), 999%2F993%2F653, 999%252E993%252E653 and j.devries%2540gemeente.nl (double-encoded e-mail). A reflection script calling safeUrl() on the previous SpanMapper returned the first six unmasked; with the decode removed, the double-encoded e-mail case fails the test. `ingeschrevenpersonen` and `zaakinformatieobjecten` stay. The test docblock now says lowercase route words under 32 characters stay (32+ are masked as opaque tokens). Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…buted cache, not app config #2154 (comment) OtelExportBreaker::recordSkipped() wrote otel_skipped_total to IAppConfig (an unlocked read-modify-write and a database UPDATE) for every sampled trace skipped while paused, on the traced request path (REQ-OTEL-002). It now counts with IMemcache::inc('otel_skipped_total') on ICacheFactory::createDistributed('integriq'). If there is no distributed IMemcache, or creating or incrementing it throws, the count is 0 and nothing reaches the traced work. The only app-config write left is otel_skipped_warned_until, once per pause window. recordSkipped() returns the count so far for the first skip of a pause and null after that. TraceExportQueue logs that one warning with `skippedSoFar` in the context. Tests: testTracesSkippedDuringAPauseAreCountedAndLoggedOncePerPause asserts 3 then 4 cache increments, that the only app-config write is otel_skipped_warned_until, and warnings carrying counts [1, 4]; testASkippedTraceWithoutADistributedCacheStillLogsOnce covers a plain ICache and a throwing factory. On the previous breaker and queue both tests fail; replacing the inc() call fails the first, removing the catch fails the second. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#2154 (comment) The pause warning's log field `skippedSoFar` was the cache counter's lifetime total, which never resets per pause. The field is renamed to `skippedTotal`. The docblocks of OtelExportBreaker::recordSkipped() and countSkipped() and of TraceExportQueue::skipWhilePaused() now say it is the running total since the cache was last cleared, per node when the distributed cache falls back to APCu. The tests read the renamed field. Behaviour is otherwise unchanged. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WilcoLouwerse
approved these changes
Oct 9, 2026
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
Contributor
There was a problem hiding this comment.
Verdict: APPROVE (Quick)
No definite blockers. All four round-6 findings from #2154 are fixed, and each done-when check in the round-6 review holds on
1374f45.
✅ Done-when checks
- f7 (thread): the not-due guard, the paused path and the retry all go through
IJobList::scheduleAfter()(OtelExportJob.php:115,:124,:199).jobList->addno longer occurs inOtelExportJob.php, andOtelExportTestassertsrunAfter=notBefore. - f8 (thread):
999.993.653,999-993-653, an e-mail segment and opaque tokens are masked, and route words such asingeschrevenpersonen,apiandv1stay. Asserted inSpanMapperTest. - f10 (thread): a paused send is rescheduled to the end of the pause and keeps its attempt. Skipped traces are counted, and one warning per pause carries
skippedTotal. Without a memcache the count is 0, with no crash (NullCache::increturnstrue, which is guarded byis_int). - f9 (thread): the outcome is normalised once with
strtolower(trim())at the top ofresolve()(SharedApprovalTaskListener.php:158), so the timer, approve and reject branches all compare against lowercase constants.
Scope: Quick mode, the whole diff (8 files, +333/−50), one analysis pass with a separate grep of the load-bearing claims. The DI change is safe: OtelExportBreaker is autowired, and its only new is in the updated test.
CI: 45 of 46 checks completed with no failure on 1374f45. One was still running when this review was posted. Gates: taken from CI, so no local run.
Connected: #2154 (the beta promotion) gets these fixes once this is merged into development. #2634 (the clamp, release-blocker before beta → main) is unaffected.
🤖 Reviewed with Claude Code
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
2 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.
Fixes for the round-6 review of #2154 (#2154 (review)). Merge into
development, then re-request the review; nothing here is merged by the reviewer.Findings → commits
2e9c221IJobList::scheduleAfter()at their run time;add()is no longer used for a delayed job.4212edf,992f266@is masked, and so is an opaque token (32+ characters, or 20+ with a digit or mixed case). Route words such asingeschrevenpersonenstay.ec06475,58ae385,1374f45inc(), so the request path makes no app-config write per trace, and one warning per pause carriesskippedTotal. The drop log reports the real number of sends.Rejectedcloses the mirror but decides nothingdc1e7d8strtolower(trim())before it is classified, as OpenRegister'sTaskStatedoes.The
pre-push:commits answer findings from the adversarial check run before the push.Left to the author (threads stay open)
otel_skipped_totalis not shown to operators. It lives in the distributed cache, so it counts per node when that cache falls back to APCu and is 0 without a memory cache. My recommendation: expose it throughIntegriqMetricsProvider.otel_breaker_failuresin app config has no lock, so concurrent cron workers can lose a count. Low impact.999993653.example.org) is still exported as is.callId: left as is by agreement in round 5.Checks run on this branch
vendor/bin/phpunit -c phpunit-unit.xml tests/Unit/Observability tests/Unit/EventListener tests/Unit/Service/ApprovalServiceTaskFirstTest.php(host) → OK, 178 tests, 556 assertions at58ae385;tests/Unit/Observability→ OK, 32 tests at1374f45vendor/bin/phpcs --standard=phpcs.xml,vendor/bin/phpmd … text phpmd.xml,vendor/bin/psalm --no-cacheon every changed lib file → clean, after each commitpre-push:commits, except the operator-visibility 🟢, which is listed above.TestSuite::empty()undefined), an environment clash outside this branch. CI runs the full suite.🤖 Generated with Claude Code