Repository navigation
feat: write library metadata into source recordings - #53
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
- Use timestamp_formatted/timestamp_seconds instead of nonexistent timestamp key - Build GENRE string before testing truthiness to avoid empty string emission - Add fallback to format_timestamp when pre-formatted value unavailable - Update tests to use real schema and add edge case coverage Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
… failure - Close SQLite connection only when opened by register_and_promote, not when injected by caller (who may reuse it across many files, e.g., backfill) - Clean up temp file if os.rename fails; mark_done row deliberately persists as it is keyed on a hash no existing file has and is therefore inert - Add tests: injected db not closed, rename failure cleans temp, mark_done persists on rename failure Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…ng-source/no-space in dry runs
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
This adds the window property test, which asserts the timing guarantee: the file is never visible in the watch folder while its hash is unregistered. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Wire source_tagger.tag_sources_for_mix into _mark_complete, immediately after the rename call so tags land on the final (post-rename) path. It shares the renamer's try/except, so a raising tagger cannot un-complete a run whose real work (SoundCloud/YouTube publish) already succeeded. Tests replace the brief's weak substring-position checks with real behavioural coverage: one structural smoke test (explicitly documented as proving textual order only), a behavioural test that monkeypatches both real functions and asserts runtime call order via a shared list, and a behavioural test that a raising tagger does not stop the run from completing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…e logging Finding 1 (critical): tag_sources_for_mix hard-gates on mix.pipeline_status == "completed", but the tag call fired before _mark_complete ever wrote that status to the DB -- the gate tripped on every real invocation and the tagger silently no-op'd forever, with no exception and no distinguishing log line. Fix: hoist the completion write (pipeline_status/pipeline_step/pipeline_error/pipeline_completed_at) to happen first, then rename, then tag -- ordering between rename and tag is unchanged. Both calls are already best-effort/non-fatal, so if the process dies between the completion write and rename/tag, the DB is telling the truth: the publish genuinely succeeded. Finding 2: the shared try/except misattributed tagging failures as rename failures. Split into two try/except blocks, each with its own logger.exception message, so the two components are never conflated. Adds a regression test for Finding 1 (test_mark_complete_lets_the_real_tagger_pass_its_status_gate) that does NOT monkeypatch tag_sources_for_mix -- it spies on the real function (delegates to it, records the result) so the actual status gate is exercised against the real DB state _mark_complete produces. Verified by mutation: temporarily moving the completion write back to its original (buggy) position made this test fail with the exact skipped/pipeline_status reason described in Finding 1; the other three tests in this file still passed under that mutation, as expected since they monkeypatch the tagger itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
POST /api/catalog/retag renames then tags source files for already- published (pipeline_status=="completed") mixes, retroactively applying the same treatment the pipeline now runs automatically at completion. dry_run defaults to True so a bare POST is safe before the real run rewrites ~498GB of irreplaceable FLACs; a run already in progress returns 409 instead of starting a second concurrent pass. Follows the router's existing _spawn()/asyncio.create_task background- task pattern (same as /sync, /regen-thumbnails, /organize-playlists) rather than FastAPI's BackgroundTasks. Replaces the brief's weak inspect.getsource/inspect.signature-only tests with behavioural coverage: HTTP-layer proof that a bare POST reports dry_run:true, that a dry run calls both the renamer and tagger with dry_run=True and performs no writes, that a concurrent run is rejected with 409, and that only completed mixes are selected as candidates. Keeps one cheap signature test for the declared dry_run default, clearly scoped in its docstring to structural-only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
The candidate-count query's await sat between the "is a run already in progress" check and the _retag_task assignment, letting two concurrent POST /api/catalog/retag requests both pass the guard and spawn overlapping runs over the same source files. Move the count query above the check so the check and assignment are adjacent with no await between them (single-threaded asyncio makes that block atomic). Rename the sequential single-flight test to test_second_run_while_one_is_running_returns_409 (it only covers the steady-state branch) and add test_truly_concurrent_requests_only_one_wins, which fires two POSTs via asyncio.gather so they actually interleave and exercises the check-then-set window itself. Verified by mutation: reverting the fix makes the new test fail with [202, 202] as expected; restoring it passes. Also name-scope the test file's shazamio import-stub exception handler so it no longer swallows a missing transitive dependency of a real shazamio install. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
backend/tests/test_seen_files_api.py imported os and backend/tests/test_tagging_does_not_reingest.py imported pytest, neither used. ruff check backend/ was failing CI on exactly these two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…Blocker 1)
FLAC's STREAMINFO duration is a header read, not a measurement: shutil.copy2
carries it over from the original verbatim, so a short write (ENOSPC
part-way, an I/O error on the NAS, a disk that fills after _has_free_space
passed) produces a header-valid, truncated file that passed both existing
verify checks and would have been promoted over an irreplaceable original.
Add a size comparison to write_tagged_copy: the tagged copy is the same
audio plus 64 KB padding plus any cover art minus a small old tag block, so
it must always come out larger than the source. Anything smaller is
definitionally a truncated write and raises RuntimeError, cleaned up by the
existing except-block.
Also fsync the staged file before returning it, so a crash right after a
later atomic rename can't leave the directory entry pointing at data that
was never durably written (this belongs with the same verification step).
New test test_truncated_staged_copy_is_rejected builds a real FLAC with
substantial audio data (a sine wave -- silence compresses too well for a
tiny fixture's padding-dominated size to expose truncation), truncates deep
into the audio frame region only (never touching metadata, so the parse and
duration checks both still pass), and asserts write_tagged_copy raises and
leaves no file in staging. Verified by hand that this test fails
("DID NOT RAISE") with the size check removed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…ilure (Finding 4) Previously only the os.rename failure path removed the staged temp file. A raising compute_file_hash (I/O error reading a multi-gigabyte temp file off a flaky NAS mount) or a raising mark_done (sqlite3.OperationalError: database is locked is live -- the running watcher holds its own connection to the same data/seen_files.db) orphaned it instead, and nothing sweeps .fadeout-tagging/. Wrap the whole compute-hash/mark-done/rename sequence in one try/except so any failure removes the temp file before re-raising. Ordering is unchanged (mark_done strictly before os.rename) and the mark_done row is still deliberately not rolled back on failure -- that's a locked-in decision with its own regression test. Also fsync the containing directory after a successful rename (Finding 5): POSIX gives no ordering guarantee between a rename and the directory entry reaching disk, so without this a crash right after promotion can lose the rename even though the file's own data was already durable. Wrapped so a platform that refuses directory fsync degrades to a logged warning instead of failing an otherwise-successful promote. New tests: a raising compute_file_hash and a raising mark_done each remove the temp file, leave the staging dir empty, never touch the original, and re-raise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
is_within_allowed_roots was imported after the module's other top-level definitions instead of with the rest of the import block -- move it up where it belongs (still patchable at the module level for tests, since Python name lookup doesn't care where an import statement lives). Also drops an unused src_size local left over from an earlier version of the truncation test in test_source_tagger_write.py, caught by ruff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…cker 3)
tag_sources_for_mix only checked _tagging_enabled() when dry_run was False
(Python short-circuits "not dry_run and ..."), so a dry run never looked at
the flag at all. Since both tag_source_files and rename_source_files
default to OFF, an operator could get a full "would tag all 80 mixes"
pre-flight report, flip to a real run, and have every mix come back
"disabled" -- a run that did nothing, indistinguishable from success at the
status endpoint (processed == total either way).
tag_sources_for_mix now evaluates _tagging_enabled() unconditionally, adds
"enabled": <bool> to each reported action, and a disabled dry run says
plainly that the setting is off and a real run would do nothing (instead of
proceeding through the exists/space checks as if it would tag).
_run_retag now aggregates a "summary": {ok, skipped, disabled, failed,
dry_run} count across every rename AND tag outcome of the run, so
GET /api/catalog/retag/status can no longer show a run that did nothing as
if it succeeded.
Also fixes catalog_retag's "candidates" pre-flight count, which ignored the
`limit` query param (a plain COUNT(*) over all completed mixes) -- it now
mirrors _run_retag's own id-selection, limit included, so the number an
operator sees before starting matches what the run will actually touch.
New tests: a dry run with the setting off reports enabled: False and a
plain "off ... would do nothing" reason; _run_retag's summary matches the
per-mix results it was built alongside.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Several tests on this branch asserted something other than what their name,
docstring, or comment claimed -- exactly the failure mode that has already
bitten this branch twice, so fixing all of them together:
- test_source_tagger_write.py: pytest.importorskip("mutagen") was dead --
the module-level `from mutagen.flac import FLAC` already fails collection
if mutagen is missing. Removed.
- test_expected_duration_none_skips_verification renamed to
test_expected_duration_none_skips_only_the_duration_check: the re-read is
unconditional and only the duration comparison is gated by
expected_duration. Docstring/comments now say so, and note that FLAC(out)
succeeding is itself proof the unconditional parse check ran and passed.
- test_zero_duration_read_fails_with_expected_duration's comment claimed it
patches only the verify read; it patches both FLAC() calls
write_tagged_copy makes. Comment corrected, and the dead `_patched` guard
removed (set per-instance on a fresh FLAC() every time, so it was always
False and never actually gated anything).
- test_padding_uses_constant deleted: verbatim duplicate of
test_padding_is_added_so_later_edits_are_in_place.
- test_sequential_calls_produce_different_temp_paths's docstring claimed
"concurrent calls must not collide"; the two calls are sequential.
Docstring now says what the test actually proves: uniqueness comes from
the uuid4 component, not timing.
- test_catalog_retag_endpoint.py: TestDryRunPerformsNoWrites renamed to
TestDryRunCallsHelpersWithDryRunTrue, matching what its one test (whose
docstring was already honest) actually checks.
- test_bare_post_reports_dry_run_true_over_http spawned a real _run_retag
against the real DB with nothing patched and never awaited or cancelled
it, leaking a live background task into the next test. Now patches the
renamer/tagger helpers and awaits the task, like every other test in the
file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…eanup Fills gaps left in the source tagging docs after this fix wave: - Both tag_source_files and rename_source_files must be on for a real backfill run to do anything; since both default off, spell out that a green dry run can precede a real run that silently does nothing, and point at the new GET /retag/status summary as the way to check. - Re-tagging is not idempotent: there's no already-tagged check, so running the backfill twice over the same mixes redoes the full multi-gigabyte rewrite twice. - Document the `limit` query param for running the backfill in supervised batches, and that the returned `candidates` count reflects it. - .fadeout-tagging/ is operator-visible, not OS-reclaimed, and can accumulate orphaned files if a run is killed between writing a staged copy and its own cleanup running. - Step 3 of the promote sequence now mentions the size check (a truncated write still parses and still reports the original duration; only a size comparison catches it) and step 5 mentions the post-rename directory fsync. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
… blockers
1. Truncation guard in write_tagged_copy() compared total file size, which
is a false positive on any healthy re-tag: once a file carries the 64 KB
padding block this module adds, mutagen writes new metadata INTO that
padding and total size doesn't change. Since pipeline.py tags on every
pipeline_complete, this rejected any mix re-run after already being
tagged as "truncated during the write". Fixed by walking the FLAC
metadata block chain (new _audio_payload_offset helper) and comparing
audio-payload bytes instead of total file size -- the real invariant.
2. The retag summary in catalog.py read outcome.get("status") on BOTH the
renamer and tagger results, but rename_sources_for_mix has no top-level
"status" key at all (its shape is {mix_id, reason, dry_run, renamed,
skipped, errors}), so every rename outcome silently vanished from the
summary -- the operator's go/no-go signal for a 498 GB rewrite. Added
_rename_outcome_status() to derive a status from the renamer's actual
fields, and an "unknown" bucket so an unrecognised shape is counted
instead of dropped (sum(summary.values()) == 2 * processed now holds).
Rewrote every renamer stub in test_catalog_retag_endpoint.py to the
renamer's real return shape -- the fixtures previously invented a
"status" key the real function never emits, so the summary test was
green against fiction.
3. rename_sources_for_mix now evaluates rename_source_files unconditionally
(matching how source_tagger already reads its own flag) and surfaces it
via a new "enabled" key, so a dry run reports whether a real run would
actually do anything -- the same signal the tagger's dry run already
gave. Existing skipped-shape assertions in test_source_renamer.py are
untouched.
4. test_dry_run_touches_nothing patched isfile but not _has_free_space, so
it actually exercised the insufficient-free-space branch instead of the
"no changes made" branch its name claims. Patched _has_free_space like
its sibling test already does, and asserted the actual reason string.
Also corrects five README claims that were only true once the above landed:
re-tagging now succeeds rather than merely "redoing the rewrite", the
renamer's real disabled-shape (no top-level status), the tagger vs renamer
dry-run asymmetry, and that limit=<n> caps a single run rather than
paginating (_run_retag has no ORDER BY/offset).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
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.
Mixes on the NAS keep whatever OBS called them and carry no tags, so a recording opened in Plex or any local player has no artist, no title, no date and no artwork. This closes that gap for the file itself. Renaming already existed and is untouched — it is simply switched on alongside.
Spec:
docs/superpowers/specs/2026-09-22-source-file-metadata-design.md· Plan:docs/superpowers/plans/2026-09-22-source-file-metadata.mdThe constraint that shapes the design
The watcher dedupes on
md5(first 10 MB). FLAC metadata lives at the head of the file — measured on this library, audio frames begin at byte 86 with zero padding. So tagging changes the dedupe hash, and a tagged file looks like a new recording: the watcher would ingest it and re-upload a mix already live on SoundCloud and YouTube. And with no padding, the first write is unavoidably a full rewrite.Since the rewrite is unavoidable, tagging is done out-of-place, which costs the same I/O and buys two guarantees:
Registering before promoting means a file is never visible while unregistered. Writing out-of-place means the original survives a crash, a full disk or a corrupt write. In-place tagging gives neither. 64 KB of padding is added so this is the only full rewrite a file ever needs.
Safety rails
Opt-in (
tag_source_files, off by default) · allowlisted to the watch roots · onlypipeline_status == "completed"· never raises (the caller is a run whose mix is already published) · refuses if free space < 2× file size.Backfill
POST /api/catalog/retag—dry_rundefaults toTrue; a bare POST is the safe form. Reports per mix whether the source exists, whether there is space, and whether each setting is actually enabled.GET /api/catalog/retag/statusreturns a status summary that reconciles (sum(counts) == 2 × processed). A second concurrent run returns 409.Review found three things worth naming
Each shipped green and was caught by review, not by the tests:
STREAMINFO, whichcopy2carries over verbatim — a header read, not a measurement. A FLAC with half its bytes deleted still reported full duration and would have been promoted over the original. Now compares audio payload size."skipped"every time — passing every test, because the tests mocked the tagger away.awaitbetween check and set let two concurrent POSTs both start a 498 GB pass. Reproduced withasyncio.gather, fixed, and now guarded by a genuinely concurrent test.Tests
99 passing across 9 files;
ruff check backend/clean. The load-bearing ones are mutation-verified — truncation rejection, the register-before-promote window, runtime call order, and the concurrency guard each demonstrably fail when the behaviour they guard is reverted.Known gaps, deliberately not closed
The spec promised the backfill would be batched, resumable, and logged to activity events. It is none of those. Re-tagging also redoes work rather than skipping already-tagged files.
Run the backfill with
limitunder supervision, not as one unattended 80-file pass.limitcaps a run; it does not paginate.🤖 Generated with Claude Code
https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M