S145 — re-parent retagged music tracks, and make the fix reach the rows that need it#573
Merged
Conversation
…ws that need it A track whose ALBUM or ARTIST tag was edited could never be re-filed. Two independent halves, both required; landing them together is not optional (see the test-double note at the bottom). 1. upsertTrack() could neither SEE nor WRITE parentage. Its change predicate compared exactly four fields (title, track_number, disc_number, duration_secs) and its UPDATE named exactly those four columns. Change only the album or artist tag and all four stay identical, so the call returned 'skipped' having written nothing. A repo-wide grep finds three statements that write music_tracks, all in this file, and the other two are INSERTs -- so nothing in the codebase could move a track. Both columns are INT UNSIGNED NOT NULL with enforced FKs (migration 065), so the row was never NULL and never violated a constraint: WRONG BUT VALID, which is why nothing ever surfaced it. The SELECT, the predicate and the UPDATE now all carry album_id/artist_id, and the album a track LEAVES has its total_tracks recomputed -- flushAlbum()'s finally only refreshes the album being flushed, and MusicLibraryService sums that column onto the artist page, so healing without it would trade one wrong number for another. 2. rescan now reads EVERY file for a music library. Today's fast rescan is the thing that was wrong. Fixing upsertTrack() alone is cosmetic: S122(a)'s skip continues BEFORE probeMetadata() and before the file is buffered for flushAlbum(), so an already-stamped row never reaches the repair. Measured on production: 29,134 of 61,111 tracks (47.7%, rising) would be skipped by an ordinary scan. scanDirectory() gains `bool $readEveryFile = false`, which simply leaves the skip index unloaded; LibraryManager::rescanLibrary() passes true. Migration 084 has documented `rescan` as the heavy option since it was written, so this restores a promise the schema already made: NO migration, NO new job type, NO new command. Admin Rescan and `library:scan <id> --rescan` both get it free. The flag is a PARAMETER, never a setter -- the scanner is @autowire'd and can outlive one scan in a Workerman worker. An AUTOMATIC gate was considered and rejected: the only DB-visible fingerprint (a zero-track album) is not self-clearing, so a gate keyed on it would latch the fast path off permanently and re-create the 6.1 h scan S122 exists to prevent. NOT a filesystem-stat backfill (rejected in-code; it is the S122 r1 B1 data-loss shape). Every file this mode stamps is a file it just read. Cost, surfaced in migration 084's header, the CLI --rescan help, the rescan API message and CHANGELOG.md: a music rescan goes from minutes to ~3.5 h, and its first run reports `updated` in the thousands. Interruptible and idempotent. Summary gains `reparented` and `read_every_file`; no per-track logging. Production blast radius, measured 2026-07-27 (SELECT only): 310 albums owning zero tracks, 7 empty artists, 292 tracks provably mis-parented (a FLOOR -- the rest cannot be identified by SQL because the truth is in the tags). Accumulating, not dormant: 100% of albums created 07-26 and 97.8% of 07-27 were empty shells. Tests - tests/Integration/Media/MusicRetagReparentIntegrationTest.php -- acceptance, real MySQL, a GENUINE getid3_writetags ID3v2 write so mtime and size move exactly as a real retag does. Asserts the four old predicate fields are unchanged (so the fixture provably reproduces the defect), then album_id, artist_id, both albums' total_tracks, updated and no duplication. Verified RED against 5c0fdea. - tests/Unit/Media/Music/MusicScanReparentTest.php -- reach. Contains a DELIBERATE negative assertion: a mis-parented row with an intact stamp and no file change is NOT healed by an ordinary rescan. That is the executable form of "the upsertTrack() fix alone is cosmetic"; do not delete it as "asserting the bug". Plus the steady-state guards (a full read of a clean library must update NOTHING, and the flag must default OFF). - tests/Unit/Media/Library/LibraryRescanFullReadTest.php -- scan must not ask for a full read; rescan must. Test doubles, in this SAME commit BY NECESSITY -- read this before reading any red as a regression: - SkipSchemaConnection::runUpdate() read the row id from $p[4]. The instant the production UPDATE gained two parameters, $p[4] stopped being the id, the handler matched nothing, and MusicScanUnchangedSkipTest:: testAChangedFileIsStampedSoItIsSkippedOnTheFollowingScan() went red WITH PRODUCTION CORRECT. Now reads $p[6], and mutates album_id/artist_id from $p[4]/$p[5]. Production column order (two columns appended after duration_secs, id last) is pinned for this reason. - All THREE music doubles dropped $p[2] on INSERT INTO music_tracks, so the stored row had no artist_id key at all and no test could assert parentage. Fixed in SkipSchemaConnection, MusicLibraryScannerTest's MusicSchemaConnection (which also never modelled the 'updated' UPDATE) and its inline statefulDbMock. Out of scope: the album/artist natural keys and the md5(artist|album) grouping; the S122 skip predicate itself; playback (stream_url is minted from media_item_id). Reaping the newly-vacated shell albums is a FOLLOW-UP -- fk_tracks_album is ON DELETE CASCADE, so a careless delete is dangerous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… express `SkipSchemaConnection::runSelect()` returns the stored track row WHOLESALE and ignores the statement's column list, so it answers with columns production never asked for. That makes the unit steady-state test blind to the one mutation that matters here: reverting the widened `SELECT` while keeping the widened predicate and UPDATE. The coercion then reads the absent key as 0, `0 !== $albumId` for every row, and every file in the library becomes an UPDATE on every scan -- 61,111 needless rewrites per healing scan on production. Measured: with the SELECT reverted, the unit steady-state test stays GREEN and this new test goes RED. Only a server distinguishes "selected" from "not selected", which is the same reason the acceptance assertions live here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 |
|---|---|
| UnusedCode | 4 medium |
| BestPractice | 4 medium |
| Documentation | 8 minor |
| ErrorProne | 4 medium |
| CodeStyle | 65 minor |
| Complexity | 6 medium |
| Comprehensibility | 9 minor |
🟢 Metrics 121 complexity · 2 duplication
Metric Results Complexity 121 Duplication 2
🟢 Coverage 100.00% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7130b23) 62173 40431 65.03% Head commit (0b09211) 62199 (+26) 40446 (+15) 65.03% (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 (#573) 41 41 100.00% 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #573 +/- ##
============================================
- Coverage 65.84% 65.84% -0.01%
- Complexity 21058 21069 +11
============================================
Files 657 657
Lines 66237 66262 +25
============================================
+ Hits 43614 43628 +14
- Misses 22623 22634 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…edits The header said re-wording this file would "only take effect on brand-new installs". That is wrong: MigrationRunner::checksum() hashes the whole file, not just its statements, so any comment edit diverges the recorded checksum and the runner re-applies the ALTER once per install. Measured before merge: library_scan_jobs is 153 rows / 48 KB on production and the ENUM definition is unchanged, so the re-run is a sub-second idempotent rebuild. Documented rather than reverted, with a warning not to assume the same for a comment edit to a migration that touches a large table. S145 review finding 1 (non-blocking). 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.
The defect
upsertTrack()has four exit paths. The two a rescan of an existing track can take never writealbum_id/artist_id: the'skipped'branch writes nothing, and the'updated'UPDATE named onlytitle, track_number, disc_number, duration_secs. The change-detection predicate compared those same four fields, so it could not even detect mis-parenting.The album grouping key is the tag, not the directory (
md5($artist . '|' . $album)). Change a file's ALBUM or ARTIST tag while title/track/disc/duration stay identical and the track stays filed under the old album forever. There is no NULL case — both columns areINT UNSIGNED NOT NULLwith enforced FKs — so the failure is wrong but valid, which is exactly why nothing surfaces it.Measured on prod: 310 albums own zero tracks; 100% of albums created 2026-07-26 and 97.8% of 07-27 are empty shells. This is live and accumulating, not historical damage.
Why the obvious fix would have been cosmetic
S122's skip
continues beforeprobeMetadata()and before the file is buffered forflushAlbum(), so an already-stamped unchanged file never reachesupsertTrack()at all. 29,134 of 61,111 tracks (47.7%) would be skipped by the next ordinary scan — and that fraction grows toward 100%. A change confined toupsertTrack()heals only rows whose files change again.So this ships a reach mechanism:
bool $readEveryFile, threaded from the existingrescanjob type. No migration, no new command, no new job type —migrations/084already documentsrescan = purge+rescanwhilerescanLibrary()was running the same skip-enabled scanscandoes. Both operator surfaces (admin Rescan,library:scan <id> --rescan) work for free.The flag is a parameter, never a setter — the scanner is
@autowired and may outlive one scan in the Workerman worker. No filesystem-stat backfill: re-stat-and-stamp is the S122 data-loss shape and the authors rejected it in-code.Also fixed: the vacated album's
total_tracksflushAlbum()'sfinallyrefreshes only the album being flushed, so healing a track without refreshing the album it left trades one wrong number for another —MusicLibraryServicesums that column onto the artist page.Proof it fails against
5c0fdead— verified by checkout, not inferenceThe four old-predicate guard assertions passed at that commit — proof the fixture reproduces this defect rather than something adjacent. The retag is a genuine
getid3_writetagsID3v2.3 write: size 16669 → 20578, mtime moves,playtime_secondsidentical, title/track/disc preserved.Mutation matrix — 11 mutations, all killed
Committed before mutating; each restored by file copy verified with
md5sumagainstgit show HEAD:.⚠ M10 initially SURVIVED. Reverting only the widened
SELECTpassed every unit test, becauseSkipSchemaConnection::runSelect()returns the stored row wholesale and ignores the statement's column list. On a real server the absent key coerces to0and all 61,111 rows get rewritten on every healing scan. Commit2f88446cadds a real-DB steady-state test that kills it. This is a case the in-memory double structurally cannot express.Test doubles: three, not one
All three music doubles dropped
$p[2](artist_id) on insert:SkipSchemaConnection,MusicSchemaConnection, and an inlinestatefulDbMock().MusicSchemaConnectionnever modelled the'updated'UPDATE at all (blanketreturn 1); now modelled.Gates
Out of scope
Reaping the newly-vacated shell albums is a follow-up —
fk_tracks_albumisON DELETE CASCADE, so a careless delete is dangerous.rescangoes from minutes to ~3.5 h for music; the CHANGELOG says so, and says plainly that today's fastrescanis the thing that is wrong.🤖 Generated with Claude Code