S151 — use the path_hash index the scanner already had, and make CLI scans visible#575
Merged
Conversation
…scans visible
## 1. The per-file track lookup now uses the index that already existed (S151)
`MusicLibraryScanner::findExistingTrackMediaItemId()` ran
SELECT id FROM media_items WHERE type = 'track' AND path = ? AND library_id <=> ?
`media_items.path` has NO b-tree index and cannot get one: `varchar(1000)` utf8mb4 is
4,000 bytes, over InnoDB's 3,072-byte key limit on its own. So the planner used only
the FIRST column of `idx_media_items_library_path_hash` — `library_id`, cardinality 3
on production — and hand-filtered the rest. Measured on prod (warm, SQL_NO_CACHE, 4
repeats): EXPLAIN `ref`, key_len 144, **48,512 rows examined**, 0.714–0.864 s. Once
per audio file, 61,122 times => ~13 h of pure query time. That is the dominant cost of
a music scan, ahead of the tag reading everyone assumed was the bottleneck.
The fix binds the STORED generated column alongside the raw path:
WHERE type = 'track' AND path_hash = ? AND path = ? AND library_id <=> ?
with `sha1($path)` computed in PHP. Reproduced locally on MySQL 8.0.46: `ref`/144/403
rows -> **`const`/305/rows=1**. No schema change, no new index, no migration.
`path = ?` is KEPT. The row is already fetched by then so it costs nothing, and it
turns "a SHA-1 collision is unlikely" into "a SHA-1 collision cannot return the wrong
row".
MySQL does not do this substitution itself: generated-column index substitution fires
only when the WHERE clause contains the column's exact generating expression, and
`path = '...'` never triggers it. Verified by EXPLAIN, not assumed.
### Sweep — every other `path = ?` lookup on media_items was left alone, deliberately
* `ItemRepository::findByPath()` pass 1 and `findPathsMap()` pass 1 already bind
`path_hash`; nothing to do.
* `findByPath()` pass 2 and `findPathsMap()` pass 2 are the RAW-PATH FALLBACKS whose
entire purpose is the types whose `path_hash` is NULL, and neither statement
constrains `type`. Converting them would make them match nothing — a fast wrong
answer replacing a slow right one. Left raw, with a comment saying why.
* `MusicLibraryScanner`'s artist/album adoption lookups filter `type IN
('artist','album') AND path = ''` — uncovered types, empty path. Not converted.
`path_hash` is NULL for 7 of the 13 `type` ENUM members; migration 087 covers only
`episode, movie, audio, book, track, audiobook`. `type = 'track'` is pinned inside the
statement itself, which is what makes this call site safe.
Stale comments that called `track`/`audiobook` "NON-deduped (NULL path_hash)" — true
under migration 072, FALSE since 087 — are corrected in `ItemRepository`,
`MusicLibraryManager`, `AudiobookScanner` and `AudiobookLibraryManager`.
## 2. A CLI scan is no longer invisible to the admin UI (S150)
`LibraryScanCommand` wrote NOTHING to `library_scan_jobs`. The admin Libraries page
polls that table, so during a live CLI scan it showed whatever the last web-enqueued
job left behind. Measured on prod during the S145 healing rescan: a healthy scan
running ~45 min and demonstrably repairing rows, while the page showed a red `failed`
badge from a job that ended at 06:27 with 'Interrupted by server restart'. A stale
failure reads as "the last thing that happened, broke" — worse than no badge.
The command now opens a `running` row up front (`ScanJobRepository::startRunning()`,
correct `scan`/`rescan` type), streams `items_found`/`items_updated`/`current_path`
through the SAME throttled sink the worker uses, and stamps `markCompleted()` /
`markFailed()` on the success and throw paths, on SIGTERM/SIGINT/SIGHUP, and on a
fatal-error shutdown.
`items_updated` remains the PROCESSED-FILE numerator, not a count of rows written —
the badge percentage divides by `items_found`. Not touched.
The sink implementation moved to a new `ScanProgressSink` so the CLI and the worker
share ONE copy. A STATIC factory taking the repository, not a method on it, because
`LibraryScanWorker`'s tests mock `ScanJobRepository` and assert on the
`updateProgress()` calls the sink makes — a sink built by a method on that same mock
would return null and those assertions would silently observe nothing.
### DECISION: a CLI scan REFUSES to start when a job is already queued/running
Today nothing prevented a CLI scan and a worker scan running concurrently over one
library. Two scanners interleave find-or-create on every file, and they share one job
row's worth of UI, so the badge would report one scan's progress under the other's
counters. The command therefore exits FAILURE with an explanatory message when the
library's newest job is `queued` or `running`.
`--force` overrides it, for the one case the check cannot distinguish: a row stranded
`running` by a `kill -9` / power loss, which NO in-process handler can ever clean up.
(A `phlix-server` restart clears such rows by itself — `LibraryScanWorker::start()`
reaps every `running` row at boot.)
Two residual behaviours, stated rather than glossed:
* `reapStaleJobs()` is unscoped and has no age guard, so a `phlix-server` restart
during a long CLI scan marks that row `failed` while the CLI keeps running. Bounding
it needs per-job worker ownership, i.e. a schema change out of scope here. The
failure mode is a false badge, never a lost scan or a stuck row.
* SIGKILL cannot be trapped by anyone, so it still leaves the row `running` until the
next server boot reaps it.
Job tracking is observability and must never refuse an operator's scan: an absent or
unreachable store degrades to the pre-S150 behaviour with a warning on stderr.
## 3. A false figure I shipped, corrected
Migration 084's header and the S145 notes said a music `rescan` costs "roughly 3.5
HOURS (measured basis: ...)". That was an ESTIMATE presented as a MEASUREMENT and it
was wrong. The last completed music rescan of the production library (`d8e21a1b`,
2026-07-25) took **9 h 55 m**; earlier full walks recorded 5:20, 3:29, 2:22. Corrected
in all 9 places it appears. S151 removes what was then the dominant cost, so the figure
should fall sharply — but no post-S151 rescan has been timed, and no new duration is
stated as if it had been.
Editing that header is safe and this was verified, not assumed: `MigrationRunner::
checksum()` strips full-line `--`/`#` comments before hashing, and the ledger checksum
is byte-identical before and after (c948944dfc1bbb4e82b0675efdb341e2). The executable
SQL is unchanged.
## Verification
* New real-MySQL test `MusicTrackPathHashLookupTest` EXPLAINs the statement the
production method ACTUALLY emits (captured through a recording connection, never
re-typed) and asserts `type=const`, `key=idx_media_items_library_path_hash`,
`key_len=305`, `rows=1`; the pre-S151 form is EXPLAINed alongside and asserted to be
non-const with key_len 144.
* Non-ASCII fixture (`Sigur Rós`, `Björk`, CJK) proving PHP `sha1()` equals the
MySQL-generated `path_hash`, with a guard assertion that the fixture really is
multi-byte. An ASCII-only fixture cannot distinguish the two behaviours.
* Both halves of the NULL-hash landmine pinned: no `track` row may have a NULL
path_hash, and an uncovered type (`album`) must still hash to NULL.
* The three in-memory doubles now recompute `sha1()` from the stored row, so reverting
the production query to its two-parameter form makes every unit lookup miss.
* New real-MySQL test `CliScanJobVisibilityTest`, including a genuinely killed
subprocess (real `php`, real SIGTERM) proving the row lands `failed` with a truthful
reason and never stays `running`.
* Full suite: 7711 tests (7692 on master + 19 new), 57,803 assertions. The single
failure is the documented S137 pool-exhaustion flake in
`PooledConnectionConcurrencyTest`, which passes in isolation.
* phpstan level 9 on src/: no errors. phpcs PSR12 on src/: 0 errors.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 16 minor |
| ErrorProne | 13 medium |
| CodeStyle | 64 minor |
| Complexity | 7 medium |
🟢 Metrics 187 complexity · 2 duplication
Metric Results Complexity 187 Duplication 2
🟢 Coverage 80.89% diff coverage · -0.04% coverage variation
Metric Results Coverage variation ✅ -0.04% coverage variation (-1.00%) Diff coverage ✅ 80.89% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0389da7) 62297 40551 65.09% Head commit (fe3d64e) 62430 (+133) 40615 (+64) 65.06% (-0.04%) 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 (#575) 157 127 80.89% 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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
============================================
- Coverage 65.90% 65.87% -0.03%
- Complexity 21101 21154 +53
============================================
Files 657 658 +1
Lines 66366 66506 +140
============================================
+ Hits 43741 43814 +73
- Misses 22625 22692 +67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ng a migration Nine review findings against bf55454, all addressed. **1 (BLOCKING) — the row became visible before anything could fail it.** LibraryScanCommand INSERTed the `running` row at :332 and only installed its signal/shutdown handlers at :343. A SIGTERM in that window killed the process with the default disposition and stranded the row `running` forever — the exact state S150 exists to abolish. Reproduced by the reviewer 3 times in 10 PHPUnit runs and 12 times in 15 with the MySQL general log on. The fix is ordering, not tolerance: ScanJobRepository::newJobId() mints the id, the handlers are installed, and only then does the INSERT run. That closes the window COMPLETELY rather than narrowly, and the reason is measurable rather than hopeful: pcntl_async_signals(true) does not run PHP from the C signal handler — it sets EG(vm_interrupt) and the callback runs at an opcode boundary. Probed directly (SELECT SLEEP(3), SIGTERM at 500 ms): the handler was entered at 3,003 ms, never mid-statement. So a signal during the INSERT's round-trip is dispatched after it returns, when markFailed() names the row that was just committed; a signal before it stamps an id that does not exist yet (0 rows) and re-raises, so no row is ever created to strand. 19 consecutive full runs of both integration files: green. **2 — hash-ONLY lookup, no raw-path fallback.** findExistingTrackMediaItemId() now has the same two passes ItemRepository::findByPath() has. `path_hash` is a GENERATED column; where migration 087 has not run (it is two statements, a failed migration is not recorded, and docker-entrypoint.sh runs migrations with `|| true`) it is NULL for every track, every lookup misses, and the scanner mints a duplicate per file with no unique index to catch it. The fallback runs only on a miss, so a rescan — the workload S151 was measured on — never pays for it, and a first scan pays exactly the pre-S151 cost plus a const probe. **3 — the perf claim needs an index `migrations/` does not create.** 087 drops idx_media_items_library_path_hash and only the manual cleanup_072.php re-adds it. Now stated plainly in the method docblock and the test: after a clean run-migrations.php the statement plans as ref/144/404, and the test CREATES the index itself, so a green run says nothing about your database. **4 — hasActiveJobForLibrary() could miss a live job.** It read only the newest row by `queued_at`, a TIMESTAMP with 1-second granularity, so a tie or a newer terminal job hid a running one. It now asks whether ANY non-terminal job exists. **5 — TOCTOU between the check and the insert.** Closed: startRunningIfIdle() folds the predicate into one `INSERT … SELECT … WHERE NOT EXISTS`, with a single retry on a deadlock/lock-wait so the loser refuses instead of degrading to "run untracked". The cheap pre-check is kept for the operator message only. **6 — S150 falsified reapStaleJobs()'s stated invariant.** Both docblocks (ScanJobRepository and LibraryScanWorker::start()) claimed that `count:1` made a live `running` row impossible at worker boot. It is not true and the claim is DELETED, not softened, along with the consequence the old text missed: a false-`failed` row also makes hasActiveJobForLibrary() report idle, so a second scan can start. **7 — the SIGTERM handler writes to the DB from an async handler.** Assessed and proven safe here, for two reasons that are now documented: dispatch is at an opcode boundary (above), and PhlixMySQLConnection sets USE_BUFFERED_QUERY, so a statement's result set is client-side before execute() returns and a nested query starts on an idle connection. Pinned end to end by a new test that SIGTERMs the child while a query is genuinely in flight, rather than at a quiescent point. **8** — the S151 EXPLAIN proof is MusicTrackPathHashLookupTest, not the pre-existing ItemRepository PathHashIndexUsageTest. **9 — no more shared-schema mutation mid-suite.** The test no longer adds a UNIQUE index to `media_items`; it builds a uniquely-named private copy via CREATE TABLE … LIKE and EXPLAINs against that, so it can no longer make an unrelated concurrent test fail with error 1062. Verification: phpcs PSR12 0 errors / 6 pre-existing warnings in 5 files; phpstan --level=9 [OK]; full suite 7,739 tests / 59,332 assertions, zero failures (master 7,712 + 19 from S151 + 8 added here). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
S151 — use the
path_hashindex the scanner already had, and make CLI scans visiblePart 1 — the per-file lookup was ignoring a unique index that already existed
The music scanner's existence check ran:
pathis unindexed, so the planner used only the first column ofidx_media_items_library_path_hash—library_id, cardinality 3 — and hand-filtered the rest. Measured on production, warm,SQL_NO_CACHE, four repeats: 0.714 / 0.772 / 0.780 / 0.864 s,rows examined 48,512. Executed 61,122 times per music scan.media_items.path_hashis a STORED GENERATED column (sha(path)for six file-backed types includingtrack) andUNIQUE KEY idx_media_items_library_path_hash (library_id, path_hash)already existed. Adding the hash predicate alongside the existing one is enough:EXPLAIN, real MySQL 8.0.46, before → after:
typerefconstconstis the optimum — the index is UNIQUE, so at most one row can match. The test EXPLAINs the statement the production method actually emits, captured through a recording connection, not a re-typed copy.path = ?is deliberately kept: the row is already fetched, so it costs nothing, and it turns "a SHA-1 collision is unlikely" into "a collision cannot be observed".Exactly one call site qualified. Every other
path-keyed lookup was left alone on purpose, each with an in-code comment saying why:ItemRepository::findByPath()pass 2type— a hash predicate would match nothingItemRepository::findPathsMap()pass 2findAdoptableArtist/AlbumMediaItemId(),hasAdoptableMusicMediaItem()type IN ('artist','album') AND path = ''— excluded by the generating expression twice overA docs inversion fixed along the way: five files described
track/audiobookas "NON-deduped types (NULL path_hash)". True under migration 072, false since 087 — the exact inverse of the real hazard, and it would have sent the next author the wrong way.Part 2 — a CLI scan was invisible to the UI (S150)
LibraryScanCommandwrote nothing tolibrary_scan_jobs. During a live CLI rescan the admin Libraries page therefore showed a stale redfailedbadge from a scan that had ended hours earlier — worse than showing nothing, because it reads as "the last thing that happened, broke".New
ScanProgressSink(shared by the worker and the CLI), plusScanJobRepository::startRunning()/hasActiveJobForLibrary(). The command now opens arunningrow, streamsitems_found/items_updated/current_path, and stamps completed/failed on success, throw, SIGTERM/SIGINT/SIGHUP, and fatal shutdown.Concurrency: refuses to start when the newest job for that library is
queued/running;--forceoverrides.Two residuals stated rather than glossed:
reapStaleJobs()is unscoped and ageless, so a server restart can false-faila live CLI row; and SIGKILL is untrappable.Part 3 — correcting a false figure I shipped
Migration
084's header and the S145 notes claimed a musicrescancosts "roughly 3.5 HOURS". That was an estimate presented as a measurement. The last completed music rescan (d8e21a1b) took 9 h 55 m; earlier walks 5:20 / 3:29 / 2:22. Corrected in 9 places, with the post-S151 figure explicitly marked unmeasured.The migration edit is comment-only and verified not to re-run: recomputing with
MigrationRunner::checksum()'s own comment-stripping algorithm givesc948944dfc1bbb4e82b0675efdb341e2on both master and HEAD — identical to production's ledger value.Verification
8 mutations, all killed: M1 full revert of the statement (12 unit + 1 real-DB red) · M2 drop
path = ?· M3md5()forsha1()(4 unit + 2 real-DB red) · M4 job type always'scan'· M5 no signal handlers · M6 refusal disabled · M7 no progress sink · M8startRunning()insertsqueued.Non-ASCII hash agreement between PHP
sha1()and the MySQL generated column confirmed forSigur Rós,Björk,坂本龍一— an ASCII-only fixture cannot distinguish the two implementations and would have passed either way.🤖 Generated with Claude Code