Repository navigation
feat: make the retag backfill safe to run unattended - #55
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Add already_tagged(path, tags, cover_art_path), a header-only mutagen read that compares an existing FLAC's Vorbis comments and embedded- picture presence against the target tag set. Wire it into tag_sources_for_mix so an already-tagged file returns status="skipped", reason="already tagged" instead of paying a full out-of-place rewrite of a multi-gigabyte file. A dry run checks the same condition so its report matches what a real run would actually do -- overstating the work here is exactly the false-green this project has been burned by before a 498 GB operation. This is the foundation the resume cursor in Task 2 builds on: making a re-run nearly free is what makes the backfill safe to interrupt and repeat without a human watching it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…ackfill
_run_retag and the candidate-count query both used .limit(n) with no
ORDER BY and no offset, so a second batch re-selected the same rows the
first already touched -- the documented "run it with limit under
supervision" workflow for the 498 GB retag backfill never actually
paginated.
- Add offset/resume query params to POST /api/catalog/retag.
- Add an identical, deterministic ORDER BY (created_at, id) + OFFSET to
both the pre-flight candidate count and _run_retag's selection, via a
single shared statement builder so the two can never drift apart.
- Persist {last_mix_id, processed, total, dry_run, updated_at} to
AppSettings.settings_json["retag_progress"] after every mix (not just
at the end), committing immediately so a kill -9 mid-run leaves a
usable cursor.
- resume=true resolves that cursor into an offset and continues past
it; with no saved cursor (or one pointing at a since-deleted mix) it
starts from the beginning rather than erroring.
The single-flight 409 guard is untouched: the resume-cursor DB read
happens in an earlier, separate async-with block, so the check-then-set
window between the guard and the _retag_task assignment stays
await-free.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Today the retag backfill's per-mix results live only in the module-level _retag_state dict, wiped on process restart -- after an unattended run (or any run followed by a container restart) there was no way to answer "what did it actually do to my files?" for an operation that rewrites ~498 GB of irreplaceable FLAC masters. _run_retag now emits durable events through app.services.activity_log: one retag_run_started event with dry_run/limit/offset/resume and the candidate total, exactly one retag_mix_processed event per mix carrying its mix_id and both the rename and tag outcome (error level when either leg failed, info otherwise), and one retag_run_completed event with the final summary. Every emit goes through a new _emit_retag_activity helper that wraps the call in its own try/except, following the same pattern as handlers._emit_activity and auth._emit_auth -- an activity-log failure must never abort a run touching irreplaceable masters. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
.fadeout-tagging accumulates multi-gigabyte temps when a retag run dies between staging and promotion. Nothing reclaimed them, the directory is hidden from the watcher, and the resulting disk shortage surfaced only as a routine "skipped" from the free-space check. Add source_tagger.sweep_staging(root, older_than_hours, dry_run), guarded by basename == TEMP_DIRNAME, source_renamer.allowed_roots() (reused, not duplicated), and mtime age -- a fresh temp may belong to a run in flight and is never removed. Symlinks are never followed or removed; a failure on one entry logs and continues rather than aborting the sweep. _run_retag now sweeps each watch root's staging directory before selecting candidates and records what it reclaimed via the existing best-effort activity-log helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
Updates README's retag/backfill section to match what actually shipped on this branch: already-tagged files are skipped instead of fully rewritten, limit+offset genuinely paginate (ordered selection), resume=true continues from a persisted cursor, every run emits durable activity-log events, and orphaned .fadeout-tagging/ staging copies are swept automatically at the start of each run. Removes the "run it with limit under supervision" guidance and the "limit does not paginate" warning -- both existed only because of the four gaps this branch closes, and left in place they would calibrate operators on stale, now-false claims. Adds a v2.6.0 CHANGELOG entry in the existing style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
…y content Blocker 4: shutil.copy2's copystat carries the SOURCE file's mtime onto a freshly staged copy, so a copy of a months-old archival recording reported an age of thousands of hours the instant it was staged -- the sweeper's "never removes a file younger than the threshold" guard did not protect it at all, and a sweep landing while write_tagged_copy was still working could delete it mid-write. write_tagged_copy now stamps the staged copy with the current time immediately after copying, and sweep_staging now ages off the more recent of st_mtime/st_ctime (ctime cannot be backdated by copystat) rather than st_mtime alone -- belt and braces on a delete primitive. test_staging_sweeper.py's existing "old file" tests relied on os.utime backdating alone, which cannot touch ctime (a bare os.utime call always bumps ctime to the moment of the call, regardless of the mtime/atime it sets) -- so they'd have silently started failing under the new ctime-aware guard. They now use a fake_stat fixture that substitutes a controlled st_ctime at the exact os.stat call sweep_staging makes. A new test stages a copy through the REAL write_tagged_copy from a genuinely old-mtime source and simulates a sweep landing mid-write (the actual race the review reproduced) -- the test this bug needed from the start, since a synthetic backdated-mtime fixture can't reproduce copystat's behaviour, and calling sweep_staging only after write_tagged_copy returns can't observe the bug either (audio.save()'s own write naturally self-heals mtime by the time the function returns). Finding 6: already_tagged() compared only embedded-picture PRESENCE against cover_art_path's existence, so a mix whose artwork was regenerated at the same path -- same file, different bytes -- kept its stale embedded art forever. It now compares size and a SHA-256 digest of the embedded picture against the file on disk; the picture bytes are already in memory from the header read this function does anyway, so this adds no extra I/O against the multi-gigabyte source.
…-reporting work
Blocker 1: a dry run's resume cursor could govern a real run's
resume=true. The README's own documented flow -- a bare (dry-run) POST
over the whole backlog, then a real run -- left the real run resuming
past a cursor the dry run wrote, believing every mix up to it was already
done: it touched nothing while still reporting {"started": true}. The
persisted cursor now records the dry_run it was written under, and
resume=true against a cursor from the other mode is treated as no cursor
at all (starts from the beginning) rather than resolved into an offset.
Chose this over namespacing the cursor by mode to keep the persisted
shape unchanged (existing tests read a single flat cursor) -- the POST
response's new resume_cursor_ignored field says when this happened.
Blocker 2: the resume cursor advanced past every mix regardless of
outcome, so a failed write or a skip for a condition that can resolve
itself (missing source, outside the allowed roots, insufficient space)
was skipped forever by a later resume=true. It now only advances past
tag outcomes that are terminal-success (written, disabled with the
setting off, or already carrying the target tags/art) -- and, since the
cursor is an OFFSET into the ordered candidate list rather than a
per-mix ledger, once one mix in a run isn't terminal-success the cursor
stops advancing for the rest of that run too, even past later mixes that
succeed (letting a later success push it forward would still silently
skip the earlier stuck mix on the next resume). The run itself is
unaffected -- it always continues to the next mix in the same pass. Dry
runs always count as terminal (nothing is written, so there's nothing to
protect, and Blocker 1 already stops a dry run's cursor from governing a
real run regardless).
Blocker 3: the durable retag_mix_processed event carried only
rename_status/tag_status, not the reason that actually distinguishes
benign from alarming -- tag_sources_for_mix returns "skipped" for both
"already tagged" (benign) and missing-source/outside-roots/insufficient-
space (alarming), so {skipped: 80} couldn't tell them apart. The event
now carries rename_reason and tag_reason too, and retag_run_completed is
now logged at warn when any tag skip's reason wasn't "already tagged"
(previously warn fired only on failures). _run_retag also gained a real
except: a crash now emits retag_run_failed before re-raising, so a
crashed run is distinguishable in the durable log from one still running
(the existing finally still always clears the running flag).
Blocker 5: the staging sweep's own dry_run was dropped on the floor --
sweep_staging was called with only staging_root, positionally, so its
dry_run parameter silently defaulted to False regardless of the retag
call's own dry_run. A bare, "report-only" POST was therefore performing
real multi-gigabyte deletions. dry_run is now plumbed through properly.
Finding 7: the sweep only ever visited each watch root's own flat
.fadeout-tagging/ directory, but a mix stored in a subdirectory (e.g.
<watch_audio>/2023/foo.flac) stages its own copy alongside itself, which
a flat sweep never visited. Sweep targets are now the union of the flat
watch-root directories and dirname(<candidate's source>)/.fadeout-tagging/
for every mix this run actually selected as a candidate -- candidates are
now selected before the sweep runs (previously after) so their source
paths are available to derive targets from.
LOW: GET /retag/status could report the previous run's numbers while a
new run's staging sweep was still in progress, because the running flag
was set after the sweep rather than before. It's now set (along with the
rest of the reset state) before the sweep starts.
Also fixes the test that enshrined the Blocker 1 bug:
test_resume_true_continues_after_last_mix_id's setup call previously
passed only limit=2, defaulting to a DRY run -- so it actually proved a
dry run advances a real run's cursor, not what its name claims. Both
calls are now explicit real runs (dry_run=false). Adds tests for the
opposite-mode resume guard (Blocker 1) and for both failed and
skip-for-space mixes not advancing the cursor, including across a later
success in the same run (Blocker 2).
…ag backfill Three statements were false against the code as shipped: the bare-POST "without touching anything" claim (the staging sweep ignored dry_run -- Blocker 5), the "never removes a file younger than the threshold" claim in both the sweep_staging docstring and the CHANGELOG (the copy2-mtime bug -- Blocker 4), and the cover-art "matches what the mix now has" claim (it only compared presence -- Finding 6). All three now hold given the fixes in the two preceding commits; this updates the wording to describe the actual (now-accurate) behaviour, including the cursor mode-mismatch guard (Blocker 1), the terminal-success cursor gating (Blocker 2), the reason fields and retag_run_failed event (Blocker 3), and the nested- directory sweep targets (Finding 7). Also drops the "by default" phrasing around older_than_hours, since nothing currently exposes it as a parameter -- chose to correct the wording over adding a query parameter, since the threshold isn't otherwise operator-tunable today and adding one wasn't part of what the review asked for. Adds a CHANGELOG entry for this fix wave.
Gate 1 (Blocker 1 recurrence): a real run whose tag leg comes back "disabled" no longer counts as terminal-success for the resume cursor. Before this, an operator running the real backfill with tag_source_files off would see the cursor race to the end of the backlog under dry_run=False (so the mode guard didn't catch it); enabling the flag and resuming then touched zero files. Gate 2: the cursor decision now inspects BOTH legs. Renamed _tag_outcome_is_terminal_success to _mix_outcome_is_terminal_success and added a rename-status check (via the existing _rename_outcome_status) -- a failed rename no longer lets a successful tag silently advance the cursor past it. Gate 3: retag_run_started's activity-log context now carries resume_cursor_ignored, so a refused resume is distinguishable from a fresh start after the process is gone, not just in the HTTP response. Optional structural fix: tag_sources_for_mix now marks its "already tagged" skip with "terminal": True at the one call site that actually knows the answer; the cursor decision prefers that key and falls back to the old reason-string match only when it's absent. Adds tests for four previously-untested fixes from the last wave (cover-art content-hash comparison, sweep_staging's dry_run plumbing -- the delete-primitive bug, nested sweep targets, and the retag_mix_processed reason fields / alarming-skip warn level), and splits the mtime/ctime staging-sweep test in two so each half of that fix is guarded independently -- the combined test passed with either half reverted. README updated: the disabled-is-terminal claim is gone, the "does not advance the cursor" claim now covers renames too, and two new warnings cover the resume+limit livelock and mixing offset runs with resume runs. Verified: 125/125 targeted tests pass, ruff clean, and all 8 required mutations (Gate 1, Gate 2, cover-art hash, sweep dry_run plumbing, nested sweep targets, event reason fields, the os.utime stamp, ctime-aware aging) were reverted individually and each broke exactly the test meant to catch it, then restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M
The v2.6.1 entry described only the first fix wave. It omitted the three changes that actually earned the unattended verdict: the rename leg now gating the cursor, `disabled` no longer counting as terminal success on a real run, and `resume_cursor_ignored` reaching the durable log. Nothing in the entry was false, but release notes that omit the reason a release exists are their own kind of stale documentation -- and this project has now shipped that twice. Co-Authored-By: Claude Opus 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.
v2.5.0 shipped the retag backfill with gaps stated in its own release notes, and an operator instruction to "run it with
limitunder supervision".limitdidn't even paginate — noORDER BY, no offset, so successive batches re-selected the same rows. This closes the gaps and removes that instruction.Plan:
docs/superpowers/plans/2026-09-23-backfill-safe-unattended.mdWhat changed
offset, and a cursor persisted after each mix so akill -9leaves something usable.retag_run_started/retag_mix_processed/retag_run_completed/retag_run_failed, carrying both legs' status and reason, so the log alone answers "was every file tagged?".fadeout-tagging, refuses anything outside the allowlist, and never touches a file that could belong to a run in flight.Five review rounds, and what they caught
Each round found something the previous round's tests couldn't see. Worth recording, because the pattern is the point:
202 started:true,0/0 processed, at info level.shutil.copy2propagates the source's mtime, so a freshly staged copy of a 200-day-old archival FLAC reported as 4800 hours old instantly. The test named "the most important test in this file" faked its timestamps withos.utime, so it never built a file the production way.disabledcounted as terminal success — so a real run with the setting off advanced the cursor across the whole backlog, and enabling the flag then resuming tagged nothing. Identical shape to (1).permission_denied,read_only_fs, exactly what resume exists for.Tests
125 passing, ruff clean. Every load-bearing fix is mutation-guarded and verified individually — reverting in pairs is how round 4 hid a gap where one test passed with either half of its own fix removed.
Operational preconditions
limitwithresume=true— confirmed livelock past a permanently non-terminal mix. Documented in the README, and the README's own example command was fixed (it previously told you to do exactly this).offsetruns must not be mixed withresumeruns.tag_source_filesis ON before the real run — with it off the pass is wasted and no cursor is persisted.resume_cursor_ignored: trueon the first real resume after a dry run. That's the mode guard working.Known, non-blocking
any_alarming_skipstill matches a reason string, so a future rewording would produce a falsewarnon a healthy run — false alarm only, no data risk. The structural"terminal"key has no test of its own.🤖 Generated with Claude Code
https://claude.ai/code/session_01UoFoJWJX28B7nM6HfriV7M