Backlog grooming session
This commit is contained in:
Claudio Ortolina
-106
@@ -1,106 +0,0 @@
|
||||
---
|
||||
id: ML-146
|
||||
title: Honour Retry-After and rate-limit reset headers for precise snooze
|
||||
status: Done
|
||||
assignee:
|
||||
- Codex
|
||||
created_date: '2026-04-24 11:12'
|
||||
updated_date: '2026-04-27 21:02'
|
||||
labels: []
|
||||
dependencies:
|
||||
- ML-21
|
||||
priority: low
|
||||
ordinal: 1000
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
Follow-up to ML-21.
|
||||
|
||||
Once workers classify API errors as transient vs permanent, the next refinement is using per-response retry hints instead of fixed snooze durations.
|
||||
|
||||
## What each API provides
|
||||
|
||||
| API | Header | Format |
|
||||
|-----|--------|--------|
|
||||
| MusicBrainz | `Retry-After` | seconds (on 503) |
|
||||
| Wikipedia REST v1 / Action API | `Retry-After` | seconds (on 429 / 503 from `maxlag`) |
|
||||
| Discogs | `X-Discogs-Ratelimit-Remaining` + rolling 60s window | no `Retry-After`; fallback to a fixed backoff |
|
||||
| Brave Search | `X-RateLimit-Reset` | seconds until reset (comma-separated for multi-window policies) |
|
||||
| OpenAI | `x-ratelimit-reset-requests`, `x-ratelimit-reset-tokens` | duration string (e.g. `120ms`, `2s`) |
|
||||
| Last.fm | none | keep existing `ErrorResponse.retry_delay/1` fallback |
|
||||
|
||||
## Why this is a follow-up and not part of ML-21
|
||||
|
||||
ML-21 gets us from "no classification" to `:retry | :cancel`. That alone lets workers do the right thing with Oban's default backoff. This task layers on precision: turning `{:snooze, 60}` into `{:snooze, retry_after}` so we don't needlessly hammer rate-limited APIs or wait longer than necessary after a transient blip.
|
||||
|
||||
## Suggested scope
|
||||
|
||||
- Extract a helper that parses each API's reset/retry header into seconds
|
||||
- When the classifier returns `:retry`, emit `{:snooze, seconds}` using the parsed value when available
|
||||
- Clamp to a sane range (e.g. 5–300s) to guard against malformed headers
|
||||
- Unit test per-API header parsing with fixture responses
|
||||
|
||||
## Dependencies
|
||||
|
||||
Blocked by ML-21 — requires the classification plumbing to exist first.
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [x] #1 Each API module extracts a retry-delay value from its response (Retry-After, X-RateLimit-Reset, x-ratelimit-reset-*) when present
|
||||
- [x] #2 Workers emit {:snooze, seconds} with the parsed value for transient errors, falling back to a fixed default when the header is absent or malformed
|
||||
- [x] #3 Parsed durations are clamped to a safe range to prevent pathological values
|
||||
- [x] #4 Per-API header parsing is covered by unit tests using representative fixture responses
|
||||
<!-- AC:END -->
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
<!-- SECTION:PLAN:BEGIN -->
|
||||
# Implementation Plan
|
||||
|
||||
- Add a shared `MusicLibrary.RetryDelay` helper that parses retry/reset headers from `Req.Response` values, clamps parsed provider hints to 5..300 seconds, uses the maximum valid value for multi-window headers, and returns nil for absent or malformed hints.
|
||||
- Extend HTTP-based API `ErrorResponse` structs with an optional `retry_delay_seconds` field populated at `from_response/1` time.
|
||||
- Parse provider hints for MusicBrainz and Wikipedia `retry-after`, Brave `x-ratelimit-reset`, and OpenAI `x-ratelimit-reset-requests` / `x-ratelimit-reset-tokens`; keep Discogs and Last.fm on their existing fixed fallbacks.
|
||||
- Update each affected `retry_delay_seconds/1` implementation to prefer the parsed field when present and preserve existing defaults otherwise.
|
||||
- Add focused unit coverage for the shared parser, per-API parsed/fallback behavior, and one `ErrorHandler` integration assertion showing parsed values flow through to `{:snooze, seconds}`.
|
||||
<!-- SECTION:PLAN:END -->
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
<!-- SECTION:NOTES:BEGIN -->
|
||||
Implemented precise retry-delay parsing through the existing ErrorResponse callback flow. Added MusicLibrary.RetryDelay with 5..300s clamping and max-window selection for multi-window headers. MusicBrainz/Wikipedia parse Retry-After, Brave parses X-RateLimit-Reset, OpenAI parses request/token reset durations. Discogs and Last.fm remain on fixed fallbacks because they do not expose a reliable retry-delay header in scope. Verification passed: focused API/error-handler tests, full mix test suite, mix format --check-formatted, and git diff --check.
|
||||
|
||||
Addressed review gaps after implementation: Wikipedia Action API error promotion now passes the full Req response to `from_action_api_body/2` so `Retry-After` is preserved; OpenAI retry parsing now includes `Retry-After` and compound reset durations such as `1m30s`; `Retry-After: 0` now clamps to the 5s minimum instead of falling back. Verification passed: focused retry/API tests, full `mix test`, and `mix format --check-formatted`.
|
||||
<!-- SECTION:NOTES:END -->
|
||||
|
||||
## Final Summary
|
||||
|
||||
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||
## Summary
|
||||
|
||||
Added shared retry-delay parsing for provider retry/reset headers and wired it into the existing structured API error flow. HTTP-based ErrorResponse structs for MusicBrainz, Wikipedia, Brave Search, and OpenAI now capture an optional parsed retry delay and prefer it from `retry_delay_seconds/1`; workers automatically emit `{:snooze, parsed_seconds}` through the existing `MusicLibrary.Worker.ErrorHandler` path.
|
||||
|
||||
## Details
|
||||
|
||||
- New `MusicLibrary.RetryDelay` parses `Retry-After`, comma-separated reset-second headers, and OpenAI duration reset headers.
|
||||
- Parsed provider hints are clamped to 5..300 seconds and multi-window headers use the maximum valid parsed value.
|
||||
- Existing fixed fallbacks are preserved when hints are absent or malformed.
|
||||
- Discogs and Last.fm remain on current fallback behavior because they do not provide a reliable retry-delay header in this scope.
|
||||
- Updated architecture docs for the new shared helper.
|
||||
|
||||
## Tests
|
||||
|
||||
- `mix test test/music_library/retry_delay_test.exs test/music_brainz/api_test.exs test/wikipedia/api_test.exs test/brave_search/api_test.exs test/open_ai/api_test.exs test/music_library/worker/error_handler_test.exs`
|
||||
- `mix test`
|
||||
- `mix format --check-formatted`
|
||||
- `git diff --check`
|
||||
|
||||
## Review follow-up
|
||||
|
||||
- Wikipedia Action API body-error responses now preserve response headers for `Retry-After` parsing.
|
||||
- OpenAI retry parsing now considers `Retry-After` alongside request/token reset headers and supports compound durations like `1m30s`.
|
||||
- Zero-second retry hints clamp to the 5s minimum rather than falling back to fixed defaults.
|
||||
- Architecture docs now mention header-driven snooze delays in the external API integration notes.
|
||||
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||
@@ -1,23 +0,0 @@
|
||||
---
|
||||
id: ML-147
|
||||
title: Upgrade to ServerSentEvents 1.0
|
||||
status: Done
|
||||
assignee: []
|
||||
created_date: '2026-04-25 21:26'
|
||||
updated_date: '2026-04-27 21:02'
|
||||
labels: []
|
||||
dependencies: []
|
||||
priority: medium
|
||||
ordinal: 2000
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
Allows potential drastic simplification of `OpenAI.API.chat_stream/6` following [this example](https://github.com/benjreinhart/server_sent_events#real-world-example-using-req).
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 Plain refactor - dependency is updated and functionality keeps working as expected
|
||||
<!-- AC:END -->
|
||||
@@ -0,0 +1,38 @@
|
||||
---
|
||||
id: ML-150
|
||||
title: Extract Records sub-contexts to reduce module size
|
||||
status: To Do
|
||||
assignee: []
|
||||
created_date: '2026-04-30 10:47'
|
||||
labels:
|
||||
- refactor
|
||||
- records
|
||||
dependencies: []
|
||||
references:
|
||||
- lib/music_library/records.ex
|
||||
priority: medium
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
The `Records` context module (450+ lines in `lib/music_library/records.ex`) handles CRUD, FTS5 search, MusicBrainz import, cover management, genre population, color extraction, PubSub notifications, and similarity embedding dispatch — too many responsibilities for a single module.
|
||||
|
||||
Extract focused sub-contexts:
|
||||
- `Records.Search` — FTS5 search, `SearchParser` integration, search result formatting
|
||||
- `Records.Import` — MusicBrainz release/group import, barcode scan integration
|
||||
- `Records.Enrichment` — genre population, color extraction, cover management, embedding dispatch
|
||||
|
||||
Keep the public `Records` module as a facade that re-exports key functions for backward compatibility with all existing callers (LiveViews, workers, controllers).
|
||||
|
||||
The `SearchIndex` schema, `Record` schema, `Similarity` module, `TracklistPdf`, and `Batch` sub-modules stay as-is. Only `records.ex` itself is being split.
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 `Records.Search`, `Records.Import`, and `Records.Enrichment` modules exist with focused responsibilities
|
||||
- [ ] #2 Public `Records` module re-exports all previously-public functions through delegation
|
||||
- [ ] #3 All callers (LiveViews, workers, controllers, tests) continue to work without changes to their import/alias lines
|
||||
- [ ] #4 Full test suite passes with no regressions
|
||||
- [ ] #5 `@moduledoc` for each new module explains its responsibility
|
||||
<!-- AC:END -->
|
||||
@@ -0,0 +1,34 @@
|
||||
---
|
||||
id: ML-151
|
||||
title: Document Assets.Cache TTL and cache invalidation strategy
|
||||
status: To Do
|
||||
assignee: []
|
||||
created_date: '2026-04-30 10:48'
|
||||
labels:
|
||||
- documentation
|
||||
- assets
|
||||
dependencies: []
|
||||
references:
|
||||
- lib/music_library/assets/cache.ex
|
||||
priority: low
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
`Assets.Cache` (`lib/music_library/assets/cache.ex`) uses an ETS table with TTL for caching binary asset data. When assets are updated (e.g., a new cover is uploaded), the cache relies on TTL-based expiry — there is no explicit invalidation.
|
||||
|
||||
The TTL duration and the rationale for TTL-only invalidation (vs. explicit purge) are not documented in the module or in the architecture docs.
|
||||
|
||||
Add a `@moduledoc` to `Assets.Cache` that explains:
|
||||
- The TTL value and where it's configured
|
||||
- The invalidation strategy (TTL-based expiry) and why explicit invalidation is/isn't needed
|
||||
- Update `docs/architecture.md` if the design decision has architectural relevance
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 `Assets.Cache` `@moduledoc` documents the TTL value and where it is configured
|
||||
- [ ] #2 The invalidation strategy (TTL-based expiry) is explained with rationale
|
||||
- [ ] #3 Any architectural implications are captured in `docs/architecture.md`
|
||||
<!-- AC:END -->
|
||||
@@ -0,0 +1,45 @@
|
||||
---
|
||||
id: ML-152
|
||||
title: Add API version prefix to collection endpoints
|
||||
status: To Do
|
||||
assignee: []
|
||||
created_date: '2026-04-30 10:48'
|
||||
labels:
|
||||
- api
|
||||
- versioning
|
||||
dependencies: []
|
||||
references:
|
||||
- lib/music_library_web/router.ex
|
||||
- lib/music_library_web/controllers/collection_controller.ex
|
||||
- test/prod.hurl
|
||||
priority: medium
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
The JSON API endpoints under `/api/collection/*` (`lib/music_library_web/controllers/collection_controller.ex`) have no versioning prefix. If the response shape changes, external consumers would break without warning.
|
||||
|
||||
Add a `/api/v1/` prefix to all collection API routes:
|
||||
- `/api/v1/collection`
|
||||
- `/api/v1/collection/latest`
|
||||
- `/api/v1/collection/random`
|
||||
- `/api/v1/collection/on_this_day`
|
||||
|
||||
Keep existing routes as deprecated redirects (301) for backward compatibility, or remove them if there are no known consumers.
|
||||
|
||||
Update:
|
||||
- `lib/music_library_web/router.ex` (route definitions)
|
||||
- Controller and JSON tests
|
||||
- `test/prod.hurl` (post-deploy verification that uses API endpoints)
|
||||
- `docs/architecture.md` (API route table)
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 All collection API routes live under `/api/v1/collection/*`
|
||||
- [ ] #2 Existing `/api/collection/*` routes are removed or redirect to v1 with 301
|
||||
- [ ] #3 Controller and JSON tests pass against the new `/api/v1/` paths
|
||||
- [ ] #4 `test/prod.hurl` references updated to new paths
|
||||
- [ ] #5 `docs/architecture.md` updated with new route paths
|
||||
<!-- AC:END -->
|
||||
@@ -0,0 +1,44 @@
|
||||
---
|
||||
id: ML-153
|
||||
title: Move MusicBrainz data transformations out of Record schema
|
||||
status: To Do
|
||||
assignee: []
|
||||
created_date: '2026-04-30 10:48'
|
||||
labels:
|
||||
- refactor
|
||||
- records
|
||||
- musicbrainz
|
||||
dependencies: []
|
||||
references:
|
||||
- lib/music_library/records/record.ex
|
||||
- lib/music_brainz/release_group.ex
|
||||
priority: medium
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
The `Record` schema (`lib/music_library/records/record.ex`) contains MusicBrainz-specific data transformation functions that belong in the MusicBrainz API layer, not in the database schema:
|
||||
|
||||
- `parse_artists/1` — extracts artist credits from MusicBrainz API response
|
||||
- `parse_subtype/2` / `parse_secondary_types/1` — maps MusicBrainz type strings to `Record` enum values
|
||||
- `attrs_from_release_group/1` — builds a changeset attrs map from a MusicBrainz release group
|
||||
|
||||
These functions are called by `add_musicbrainz_data/2` (changeset pipeline) and during MusicBrainz import flows.
|
||||
|
||||
Move the parsing functions to appropriate `MusicBrainz` modules (e.g., `MusicBrainz.ReleaseGroup` already has related helpers) and keep only struct introspection/presentation functions on `Record` (`artist_names/1`, `releases/1`, `released?/1`, `format_release_date/1`, etc.).
|
||||
|
||||
Update callers in:
|
||||
- `Record.changeset/2` and `add_musicbrainz_data/2`
|
||||
- `Records` context (import functions)
|
||||
- Any tests that reference the moved functions directly
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 `parse_artists/1`, `parse_subtype/2`, `parse_secondary_types/1`, and `attrs_from_release_group/1` are moved to `MusicBrainz` modules
|
||||
- [ ] #2 `Record` schema retains only struct introspection and presentation helpers
|
||||
- [ ] #3 All callers are updated to use the new locations
|
||||
- [ ] #4 Full test suite passes
|
||||
- [ ] #5 `@moduledoc` on moved functions explains their purpose
|
||||
<!-- AC:END -->
|
||||
+40
@@ -0,0 +1,40 @@
|
||||
---
|
||||
id: ML-154
|
||||
title: >-
|
||||
Replace raise with Logger.warning in Repo.extension_path for unsupported
|
||||
platforms
|
||||
status: To Do
|
||||
assignee: []
|
||||
created_date: '2026-04-30 10:48'
|
||||
labels:
|
||||
- robustness
|
||||
- sqlite
|
||||
dependencies: []
|
||||
references:
|
||||
- lib/music_library/repo.ex
|
||||
- config/runtime.exs
|
||||
priority: low
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||
`MusicLibrary.Repo.extension_path/1` (`lib/music_library/repo.ex`) derives the platform from `:erlang.system_info(:system_architecture)` and raises `"Unsupported OS or platform"` if the combination isn't recognised.
|
||||
|
||||
This is called at startup in `config/runtime.exs` to load SQLite extensions (`unicode`, `vec0`). If deployed on an unexpected architecture, the app crashes before serving any traffic.
|
||||
|
||||
Change the function to:
|
||||
1. Log a warning with the detected OS/arch details
|
||||
2. Return `nil` (or an `{:error, reason}` tuple)
|
||||
3. Update `config/runtime.exs` and any other callers to handle the nil/error case gracefully — skipping extension loading but allowing the app to start
|
||||
|
||||
Features that depend on the extensions (FTS5 unicode support, vector similarity search) will degrade but the app remains available.
|
||||
<!-- SECTION:DESCRIPTION:END -->
|
||||
|
||||
## Acceptance Criteria
|
||||
<!-- AC:BEGIN -->
|
||||
- [ ] #1 `Repo.extension_path/1` logs a warning and returns nil instead of raising for unsupported platforms
|
||||
- [ ] #2 `config/runtime.exs` handles a nil return by skipping extension loading for that extension
|
||||
- [ ] #3 App can start on an unsupported platform without crashing
|
||||
- [ ] #4 Existing supported platforms (darwin-amd64, darwin-arm64, linux-amd64, linux-arm64) continue to load extensions correctly
|
||||
<!-- AC:END -->
|
||||
Reference in New Issue
Block a user