fix(wake-briefing): scan the shared debug.log once at network scope, not per site - #3524
Merged
Merged
Conversation
…not per site WakeBriefingTask's gatherNetworkSignals() ran gatherSiteSignals() once per blog under switch_to_blog(), and getPhpFatals() unconditionally scanned wp-content/debug.log inside it. On an 11-site multisite, debug.log is a single file shared by every site (WP_CONTENT_DIR and resolveDebugLogPath() are both network-wide, never per-blog), so an identical fatal signature was read, parsed, and reported 11 times — once per site line — while re-reading the same up-to-5MB tail 11 times per run. Root fix over post-hoc dedup: scan debug.log exactly once in gatherNetworkSignals(), before the per-site loop, and hoist the result into a single '**network-wide**' line rendered ahead of the per-site lines. gatherSiteSignals() gains a $scan_fatals flag (default true, preserving single-site-scope behavior) so the per-blog loop can opt out instead of scanning-then-discarding. No per-site fact is lost — the fatal was never a site-specific fact — and per-site lines keep their genuinely per-site signals (job failures, stuck jobs, grouped errors) uncollapsed. Adds a dedicated smoke test exercising gatherNetworkSignals() against a 3-site fixture: one hoisted network-wide line, no per-site repetition, and a site with real facts (mirroring the events.extrachill.com signal-loss example from the issue) still rendering on its own line. Fixes #3522
…grouping PHPStan level 7 reported 13 errors on this file, nearly all of the form "always true" / "always false" / "unreachable statement" / "method unused". Root cause was the `$flush = function () use ( &$current, ... )` closure. PHPStan cannot soundly narrow a variable that is both reset inside a closure literal and mutated by the surrounding loop after that literal, so it inferred $current as permanently stuck at its closure-definition value. Every branch downstream then looked reachable only through an always-null/always-zero state, and normalizeFatal() looked unused because its only call site sat in what the analyser had concluded was dead code. Split into two ordinary passes: collect discrete (possibly multi-line) log entries, then filter to the rolling window and group them. Every state transition is now straight-line control flow that is both correct and statically checkable. `return` inside the closure becomes `continue` in the loop, and the redundant `0 === $total ||` guard is dropped since an empty $groups already covers it. Behavior is unchanged: identical signatures across sites still collapse to one hoisted network-wide line, and a signature unique to one site still renders on that site's line. Verified: php -l clean; phpstan level 7 reports zero occurrences of the 13 original error classes on this file.
…er-narrowing WordPress stubs declare WP_DEBUG_LOG as bool, so a direct constant reference let PHPStan prove is_string() always false and mark the explicit-path branch dead (function.impossibleType, booleanAnd.alwaysFalse x2, notIdentical.alwaysTrue). At runtime the constant is genuinely either a bool or an explicit log path string — handling both is the entire purpose of resolveDebugLogPath(). Reading through constant() yields mixed and keeps the real contract statically checkable without weakening it. Behavior unchanged: a non-empty string path is returned; true/false/'' fall through to the ini and WP_CONTENT_DIR fallbacks as before.
…ndant guard constant() with a literal name is still constant-folded to the stub type, so the previous attempt did not widen anything. Resolve the constant name through a variable instead, which keeps the documented bool-or-path contract checkable without suppressing the rule or editing shared stubs. Also drops the is_string( WP_CONTENT_DIR ) guard: the stubs already type it as string, making that check provably redundant rather than defensive. Runtime behavior unchanged — a non-empty string path is returned, bool/empty falls through to the ini and WP_CONTENT_DIR fallbacks.
…_LOG WordPress copies WP_DEBUG_LOG (bool true or an explicit path) into the error_log ini at boot (wp-includes/load.php:630-632). Reading that ini covers both cases without referencing the constant, whose stub type is bool and cannot express the documented string-path contract. Drops the WP_DEBUG_LOG branch that PHPStan kept proving dead even through constant() and a variable name. Behavior unchanged after WordPress has loaded: a custom path still comes from ini, bool-true still lands on WP_CONTENT_DIR/debug.log via the same ini or the fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3522
The bug
WordPress multisite shares one
wp-content/debug.logfile across every site —WP_CONTENT_DIRis network-wide andWakeBriefingTask::resolveDebugLogPath()never reads a per-site option. ButgatherNetworkSignals()rangatherSiteSignals()— which unconditionally calledgetPhpFatals()— once per blog underswitch_to_blog(). On the live 11-site extrachill.com network this meant the identical fatal signature was scanned, parsed, and rendered 11 times, once per site line:~1KB of a 2,389-byte always-on WAKE.md file was one repeated string — roughly 40%. The volume alone is bad; the signal loss is worse:
events.extrachill.comgenuinely has 342 logged errors, 15 stuck jobs, and 2 task types failing repeatedly (128 + 6 occurrences) — and that line read with the exact same visual weight as the ten duplicate-noise lines around it. The briefing's whole job is a 3-second glance at "anything red that happened recently"; repeating one signature 11× worked directly against that.Note: the underlying fatal (
ec_link_page_owner_compatibilityredeclaration) is a separate, already-shipped fix (artist-platform v1.21.1). This PR is only about the briefing reporting one fatal eleven times.The fix: network-scope scan, not post-hoc dedup
Two approaches were on the table:
debug.logis structurally shared, not coincidentally identical, and scan it exactly once at network scope instead of once per site.Chose (2). A same-vs-different threshold in approach (1) has no meaningful "different" case to guard against — the file is quite literally the same file regardless of which blog is switched-to, so it would always compare identical, on every run, forever. Approach (1) would also still pay the O(sites) cost of reading and parsing an up-to-5MB tail N times, just to discover N identical strings and throw N−1 of them away. Approach (2) fixes both the noise and the wasted I/O:
getPhpFatals()now runs once per network briefing, not once per site.Implementation:
gatherSiteSignals()gains a$scan_fatalsflag (defaulttrue, so single-site-scope behavior — the far more common non-multisite case — is untouched).gatherNetworkSignals()callsgetPhpFatals()once before the per-site loop, hoists any result into a**network-wide**line, and passes$scan_fatals = falseinto each per-bloggatherSiteSignals()call so the identical scan never runs again inside the loop.Threshold: none needed, by construction — see above. This isn't "hoist when N/N sites agree," it's "this fact was never a per-site fact to begin with."
Placement: the network-wide line renders first, ahead of the per-site lines. Rationale: it's the most network-wide-relevant fact and a reader scanning top-to-bottom should see it before wading into 11 site lines, several of which are otherwise quiet and omitted entirely.
Before / after
Before (11 identical clauses, shown truncated above) —
events.extrachill.com's real signal buried under 10 copies of the same noise.After:
One shared fact stated once;
events.extrachill.com's real signal now stands on its own with nothing competing for attention on the same line.Verification
php -lon both touched files — clean.*Test.php) coverage exists forWakeBriefingTask; it's covered by dependency-free "smoke" scripts undertests/*-smoke.php(existing pattern — seewake-briefing-infra-signals-smoke.php,wake-briefing-credential-integrity-smoke.php). Addedtests/wake-briefing-network-fatals-dedupe-smoke.php, run viaphp tests/wake-briefing-network-fatals-dedupe-smoke.php:events.extrachill.com's shape) and a shared temp debug.log.gatherNetworkSignals()produces exactly one**network-wide**line, that it renders first, that no per-site line repeats the fatal text, and that the genuinely site-specific facts still render correctly on their own site line.gatherSiteSignals($since), the default) still includes fatals directly — regression guard for the non-multisite path.wake-briefing-*-smoke.phpscripts (infra signals: 13/13, credential integrity: 25/25) — both pass unchanged, confirming the$scan_fatalsdefault preserves existing single-site behavior exactly.Constraints followed
fix/3522-wake-md-network-dedupe.CHANGELOG.mdedit, no version bump.