Repository navigation
fix(queue): app-context guarantee by construction + regression test + add-artist-style plugin settings modal - #38
Merged
tajanthind merged 1 commit intoSep 3, 2026
Conversation
…ons with the ENTIRE body under one context; regression test + add-artist-style settings modal P0 follow-up to PR #37 (final-cleanup review): the wrapper->impl fix was functionally correct (the whole impl ran inside the wrapper's context, and live downloads proved it) but structurally fragile — the impl's inner head-only `with app.app_context():` made it LOOK like the provider chain ran context-free. Reworked so the guarantee holds BY CONSTRUCTION: - services/queue_service.py: _process_track_job and download_manual_match_track are single functions again; the ENTIRE body (DB setup, dedup, resolve, downloader chain, verification) is indented under one `with app.app_context():`. Nothing in the function can run without the context. - app.py: _sync_artist_discography_background likewise. - tests/architecture/test_background_download_app_context.py (new): (a) structural scan asserting no body line after the context `with` escapes to indent < 8; (b) runtime — submits the real _process_track_job to a ThreadPoolExecutor worker (fresh thread, no inherited context) with a downloader whose download() calls context.settings.get('timeout') FIRST (the exact production crash line); the settings read must complete in the worker (marker file). CI smoke does not exercise this path; this test does. UI (user request): the plugin-settings modal previously rendered on an unstyled .custom-modal (settings floating on the translucent overlay). plugins.html now uses the standard modal-dialog-box card (same as the Add-Artist modal), and opening plugin settings toggles #confirmModal 'settings-mode': a wide (720px) add-artist-style card whose body scrolls, so the modal fills up with the plugin settings form. Generic confirmations keep the small default card. (templates/plugins.html, static/style.css, static/app.js) Live-verified on the running container: The Beatles - Let It Be (2021 Mix) FLAC downloaded, verified, and accepted. Smoke + 21 architecture tests green.
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
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.
P0: app-context guarantee now holds BY CONSTRUCTION
PR #37's wrapper→impl fix was functionally correct — the whole impl ran inside the wrapper's
with app.app_context():(live downloads proved it) — but structurally fragile: the impl kept a head-only innerwith, which reads as "provider chain runs context-free" and makes the bug easy to reintroduce.This PR merges each wrapper+impl pair into a single function whose ENTIRE body is indented under one
with app.app_context():— nothing in the function can execute outside the context:services/queue_service.py:_process_track_job,download_manual_match_trackapp.py:_sync_artist_discography_backgroundA structural test pins this: no body line after the context
withmay sit at indent < 8.Regression test (the CI gap)
tests/architecture/test_background_download_app_context.py(new):with._process_track_jobto a ThreadPoolExecutor worker (fresh thread, no inherited app context) with a downloader whosedownload()callsself.context.settings.get("timeout")first — the exact production crash line. The settings read must complete in the worker (asserted via marker file). CI smoke does not exercise this path; this test does.Suggested review:
git diff main..HEAD -w(the raw diff is mostly mechanical reindent).UI: plugin-settings modal = add-artist-style dialog
The plugin-settings modal previously rendered on an unstyled
.custom-modal(settings floating over the translucent backdrop). Now:templates/plugins.html: uses the standard.modal-dialog-boxcard (same as the Add-Artist modal).#confirmModal.settings-mode(static/app.js): a wide (720px) add-artist-style card whose body scrolls, so the modal fills up with the plugin's settings form. Generic confirmations keep the small default card. (static/style.css)Verification
The Beatles - Let It Be (2021 Mix)FLAC downloaded, verified, and accepted through the restructured_process_track_job.