Skip to content

chore(settings): roll phlix-shared v0.48.0 — the restart-flag audit now closes - #592

Merged
detain merged 1 commit into
masterfrom
fix/restart-flags-final-eight
Jul 28, 2026
Merged

chore(settings): roll phlix-shared v0.48.0 — the restart-flag audit now closes#592
detain merged 1 commit into
masterfrom
fix/restart-flags-final-eight

Conversation

@detain

@detain detain commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Final part of the audit started in #585 and continued in #590. Companion to detain/phlix-shared#37 (v0.48.0).

All 72 schema keys are now traced to a consumer: 49 restart: true, 23 restart: false, none unverified.

Why these 8 were held back

#590 deliberately left them alone: a literal-key search found no getEffective()/getOverride() consumer for any of them, and port-forward.port_forwarding.upnp_enabled had just demonstrated that "no obvious consumer" can mean inert rather than mis-flagged. Guessing a flag would have been the same mistake twice.

Tracing them explains the empty search — none of them is read through SettingsRepository at all. Every one reaches its consumer by a route that snapshots per worker. None is inert.

Key(s) Consumer Why it cannot apply live
webhooks.enabled WebhookDispatcher::getConfig()EffectiveConfig::file('webhooks') See the trap below
stats.enabled StatsCollector::isEnabled()EffectiveConfig::file('stats') Same
transcoding.tone_mapping_mode, prefer_hdr_output HwAccelConfig::get()FfmpegRunner::setConfig() Identical path to transcoding.preferred_accelerator, already restart: true — sibling keys, same config file, same consumer, contradictory flags
newsletter.enabled, send_hour boot $config['newsletter'] read when the newsletter timer is registered, once per worker start
relay.reconnect_delay, ping_interval RelayConfig::class => factory(…) PHP-DI caches the factory result per container

⚠️ The trap: EffectiveConfig::file() at use time is not live

This is what made two of these look correct, and it is worth stating plainly because it will catch the next person:

  • The call site is per-request, so the read looks live.
  • The data is not: EffectiveConfig::$overrides is a static array populated once by bootstrap(), which runs inside onWorkerStart (six call sites in start.php), and file() is memoised per bootstrap generation.

So an override saved while a worker is running is invisible to that worker however often file() is called. Contrast SettingsRepository::getEffective(), which does an uncached SELECT per call and is genuinely live. That distinction is the entire line between the 23 keys that stay restart: false and everything else.

On "restart" vs "reload"

restart: true here means the admin Restart server control, which sends a graceful SIGUSR2 reload — workers cycle, re-run onWorkerStart, re-bootstrap the overlay, rebuild DI containers. A key needing that cycle is restart: true even though systemctl is not involved. StatsCollector::isEnabled()'s existing phrase "applies on reload without a full restart" is accurate and does not conflict with the flag — but it could easily be re-read as "no restart needed", so this PR adds a clarifying note there and in WebhookDispatcher::getConfig(). That exact class of code/schema contradiction is what left the wrong flag on the lastfm.* keys.

Cross-check

The 23 keys remaining at restart: false are exactly the 23 this audit independently verified as correct (each resolving through SettingsRepository at use time). The two lists matching is a clean confirmation that nothing was left unclassified.

Verification

Gate Result
phpunit --testsuite Unit OK — 7858 tests, 50728 assertions
phpstan --level=9 (changed src) [OK] No errors
phpcs --standard=PSR12 (changed src) 0 errors

No lock-in test broke this time (unlike the v0.46.0 and v0.47.0 rolls, which each pinned a wrong flag in a test). Pin verified rather than assumed: locked ref 08f1b490 is the v0.48.0 tag commit, and the vendored schema at that ref carries the flags.

Production untouched — no data changed, no service restarted.

🤖 Generated with Claude Code

…oses

v0.48.0 corrects the last 8 "restart": false keys. All 72 schema keys are now
traced to a consumer: 49 restart:true, 23 restart:false, none unverified.

These 8 were held back in v0.47.0 because a literal-key search found no
getEffective()/getOverride() consumer, and port-forward.upnp_enabled had just
shown that "no obvious consumer" can mean INERT rather than mis-flagged.
Tracing them explains the empty search: none is read through SettingsRepository
at all.

  * webhooks.enabled / stats.enabled -- EffectiveConfig::file(). The call site
    is per-request so it LOOKS live, but $overrides is a static array populated
    once by bootstrap() (which runs in onWorkerStart) and file() memoises per
    bootstrap generation. The data is a per-worker snapshot.
  * transcoding.tone_mapping_mode / .prefer_hdr_output -- HwAccelConfig::get()
    -> FfmpegRunner::setConfig(): the same path as
    transcoding.preferred_accelerator, already restart:true.
  * newsletter.enabled / .send_hour -- boot $config, read when the timer is
    registered at worker start.
  * relay.reconnect_delay / .ping_interval -- captured into the RelayConfig DI
    factory.

No lock-in test broke this time. Adds the reasoning to
docs/dev/settings-restart-gap.md, including the EffectiveConfig::file() trap
(use-time call, boot-time data) and the restart-vs-reload distinction: the
admin Restart control is a graceful SIGUSR2 reload, and a key needing that
cycle is restart:true even though systemctl is not involved.

Also clarifies the StatsCollector and WebhookDispatcher docblocks so their
existing "applies on reload" wording cannot be re-read as "no restart needed"
-- that class of code/schema contradiction is what put the wrong flag on the
lastfm.* keys in the first place.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codacy-production

codacy-production Bot commented Jul 28, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

🟢 Coverage ∅ diff coverage · 0.00% coverage variation

Metric Results
Coverage variation 0.00% coverage variation (-1.00%)
Diff coverage diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (ecbc77f) 62907 41202 65.50%
Head commit (cb8b5e3) 62907 (+0) 41200 (-2) 65.49% (0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#592) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.32%. Comparing base (ecbc77f) to head (cb8b5e3).

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #592      +/-   ##
============================================
- Coverage     66.33%   66.32%   -0.01%     
  Complexity    21322    21322              
============================================
  Files           663      663              
  Lines         67016    67016              
============================================
- Hits          44453    44451       -2     
- Misses        22563    22565       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@detain
detain merged commit 4496ca9 into master Jul 28, 2026
19 checks passed
@detain
detain deleted the fix/restart-flags-final-eight branch July 28, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant