refactor(server): move the metadata-enrichment subsystem into lib/enrichment.py (R3) - #861
Merged
Merged
Conversation
…ichment.py (R3)
MusicBrainz / Cover-Art-Archive / AcoustID transport, the match-scorer glue, and
the background enrichment worker (~930 lines, 61 defs) leave server.py as one
cohesive unit. Bodies are verbatim; the only changes are seam reads:
meta_db / config_dir / sloppak_cache_dir / art_cache_dir -> appstate.<slot>
_song_pack_art_exists / _art_override_paths (stay in server.py for the art +
delete routes) -> appstate.<callable> (new seam slots, injected by reference)
_env_flag -> env_compat.env_flag_compat (the existing identical helper)
_artist_title_from_filename -> imported from metadata_db (its home)
the User-Agent VERSION lookup: Path(__file__).parent ->
Path(__file__).resolve().parents[1] (lib/enrichment.py -> app root)
server.py drives the worker through the module (import enrichment; the routes +
scan lifecycle call enrichment.X). Tests that faked the network on `server`
(_mb_http_get, _enrich_network_enabled, _caa_http_get, ...) now patch the same
names on `enrichment` — the module attribute is resolved at call time, so one
setattr reaches both the routes and the worker's internal callers. Acyclic:
enrichment imports appstate/appconfig/dlc_paths/metadata_db/mb_match/
acoustid_match/sloppak/loosefolder, never server.
server.py: 6,917 -> 5,988 (-929).
Verified: pyflakes clean (bar the pre-existing File/safe_join/tuning_name/ET);
route table IDENTICAL (143); full pytest 2400 passed (140 enrichment/art cases
incl the offline-safety + transport-error-pauses-pass contracts that fake the
network); test_packaging 44 passed (enrichment.py resolves under lib/); eslint 0.
Boot smoke: /api/enrichment/status + POST /kick serve from the new module.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe metadata-enrichment subsystem moves from ChangesMetadata enrichment extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant Enrichment
participant Appstate
participant ExternalAPIs
Server->>Enrichment: kick enrichment pass
Enrichment->>Appstate: read database and art seams
Enrichment->>ExternalAPIs: fetch MusicBrainz, AcoustID, or CAA data
ExternalAPIs-->>Enrichment: return enrichment data
Enrichment->>Appstate: persist matches and art state
Server->>Enrichment: read status or cancel worker
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.py (1)
2517-2574: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftEnforce the upload cap before
request.form()
max_part_sizeonly applies to non-file fields; file parts still stream into Starlette’s temp file once parsing starts. The currentContent-Lengthguard only covers honest clients, so a chunked multipart upload without that header can still consume temp storage before the later 413 in the read loop. Add an ASGI/proxy body limit or a true streaming size check ahead of form parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` around lines 2517 - 2574, Update api_enrichment_identify so the request body is size-limited before request.form() begins parsing, including chunked requests without Content-Length. Add or reuse an ASGI/proxy-level body limit or equivalent streaming guard that rejects bodies exceeding the allowed upload size before Starlette can spool file parts; retain the existing per-part validation as a secondary safeguard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/enrichment.py`:
- Around line 722-758: Update _enrich_art_one to write downloaded cover data to
a temporary file in the same cache directory, then atomically replace the target
.jpg only after the write completes successfully. Ensure failed or partial
writes do not leave the final cache path behind, while preserving the existing
cache-hit and enrichment-state behavior.
- Around line 345-362: Update _acoustid_gate so the unavailable response
identifies the actual failed prerequisite instead of always claiming fpcalc is
missing. Distinguish missing/invalid AcoustID configuration or network-disabled
states from a missing fpcalc binary, while preserving the existing 412 response
for opt-in/key absence and the appropriate 503 status for
configured-but-unavailable dependencies.
In `@tests/test_context_menu_api.py`:
- Around line 89-91: Reset the enrichment worker state before this direct
_background_enrich() invocation so leaked _enrich_cancel and _enrich_status
values cannot short-circuit the test; add the reset to an appropriate per-test
fixture, or replace the direct call with _kick_enrich() if that is the
established state-safe entry point.
---
Outside diff comments:
In `@server.py`:
- Around line 2517-2574: Update api_enrichment_identify so the request body is
size-limited before request.form() begins parsing, including chunked requests
without Content-Length. Add or reuse an ASGI/proxy-level body limit or
equivalent streaming guard that rejects bodies exceeding the allowed upload size
before Starlette can spool file parts; retain the existing per-part validation
as a secondary safeguard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b562362-171e-4359-a4dd-fbf03d83871b
📒 Files selected for processing (13)
CHANGELOG.mddocs/size-exemptions.mdlib/appstate.pylib/enrichment.pyserver.pytests/test_art_candidates.pytests/test_art_layer.pytests/test_artist_page.pytests/test_context_menu_api.pytests/test_enrichment_plumbing.pytests/test_field_overrides.pytests/test_mb_enrichment.pytests/test_scraper_options.py
lib/enrichment.py now owns the worker, and it stays imported for the whole session while the `server` fixtures pop-and-reimport `server` — so the cancel Event / status dict / caches would leak across tests, and a stale `_enrich_cancel` could short-circuit a later direct `_background_enrich()`. An autouse conftest fixture clears that process-global state before each test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Around line 23-27: In the enrichment import setup around the try block,
replace the broad Exception handler with the specific import-related exception
raised when enrichment is unavailable, such as ModuleNotFoundError or
ImportError. Preserve yielding and returning for a genuinely unavailable module,
while allowing unrelated errors from enrichment to propagate and fail the test.
- Line 35: Update the test fixture cleanup around _caa_index_locks.clear() to
acquire _caa_index_locks_guard before mutating the shared dictionary, then
release it reliably after clearing. Preserve the existing fixture behavior while
ensuring cleanup is serialized with _caa_index_lock().
- Around line 12-37: Update the _reset_enrichment_state fixture to clear
enrichment._enrich_ua_cache alongside the existing process-global cache resets,
ensuring each test starts without a stale VERSION-derived User-Agent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Narrow the import guard to ImportError (not blind Exception, BLE001), and stop clearing _caa_index_locks — it's guarded by _caa_index_locks_guard, so an unlocked clear() would race a still-alive worker, and its per-release mutexes carry no test state. Co-Authored-By: Claude Opus 4.8 (1M context) <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.
MusicBrainz / Cover-Art-Archive / AcoustID transport, the match-scorer glue, and the background enrichment worker (~930 lines, 61 defs) leave
server.pyas one cohesive unit →lib/enrichment.py.Bodies verbatim; the only changes are seam reads:
meta_db/config_dir/sloppak_cache_dir/art_cache_dir→appstate.<slot>_song_pack_art_exists/_art_override_paths(stay inserver.pyfor the art + delete routes) →appstate.<callable>— new slots, injected by reference_env_flag→ the existing identicalenv_compat.env_flag_compat;_artist_title_from_filename→ imported frommetadata_db(its home)Path(__file__).parent→.resolve().parents[1](lib/enrichment.py→ app root)server.pydrives the worker through the module (import enrichment; the routes + scan lifecycle callenrichment.X). Tests that faked the network onserver(_mb_http_get,_enrich_network_enabled,_caa_http_get, …) now patch the same names onenrichment— the module attribute is resolved at call time, so onesetattrreaches both the routes and the worker's internal callers. Acyclic:enrichmentimportsappstate/appconfig/dlc_paths/metadata_db/mb_match/acoustid_match/sloppak/loosefolder, neverserver.server.py: 6,917 → 5,988 (−929).Verification
pyflakesclean bar the pre-existingFile/safe_join/tuning_name/ET.pytest2400 passed — incl the 140 enrichment/art cases (the offline-safety "pytest never hits the network" contract + transport-error-pauses-pass-without-burning-attempts, all faking the network onenrichment).test_packaging44 passed (enrichment.pyresolves underlib/);npm run lint0; Codex 0 findings./api/enrichment/status+POST /api/enrichment/kickserve from the new module.🤖 Generated with Claude Code
Summary by CodeRabbit