fix(registry): free a job cancelled in the pop-to-claim window, and boot with a bad registry - #526
Merged
Merged
Conversation
…oot with a bad registry Two ways the registry stranded state. A cancel arriving between the worker's _pop_next() and _set_running() found the job in neither the queue nor the running slot. discard() returned False so cancel_job never finalised it, running_id() was None so nothing was terminated, and the worker then dropped it silently. The job stayed at "queued" forever: absent from the queue view, still counted by pending_count against the capacity limit, its uploaded source (up to 400 MB) never freed, and -- because "queued" is persisted -- re-queued on every restart. The worker is the sole consumer and owns the job once it has popped it, so it now closes out the cancellation itself via _finalise_dropped_job(). Extracted rather than inlined so the drop path can be tested directly, and so an already-terminal job is explicitly left alone rather than having a real result rewritten. Separately, restore() caught (OSError, JSONDecodeError, TypeError, ValueError). A registry.json that was valid JSON but not an object -- a top-level list, null, a bare string -- makes _migrate call data.get() and raise AttributeError, which that tuple does not name. restore() runs at import time, so the backend simply never started, with no self-healing path and no way for a user to recover short of deleting the file by hand. Orphan recovery and the persist that follows it sat outside the guard entirely, so an unreadable jobs_dir was fatal at startup for the same reason. Both are now caught broadly and logged. A registry we cannot use should cost the user their job list, not their app. The cancel test drives the real worker loop rather than calling the helper, so it also fails if the worker stops calling it. Verified: reverting the worker's call fails that test, and reverting restore()'s except fails five others. Refs #520
thcp
marked this pull request as ready for review
August 31, 2026 21:02
thcp
added a commit
that referenced
this pull request
Aug 31, 2026
Fixes #521. **Targets `fix/520-registry-phantom-job`, not `0.16.1` directly** -- it builds on that branch's hardening of `restore()`. Merge #526 first. Reported on macOS: a song deleted by clearing the trash, or via Settings -> "Reset app data", comes back later. ## Root cause `restore()` adopts any job-shaped directory it finds. That is correct for a library whose registry was lost, and wrong for a job the user deleted whose files outlived the delete. Nothing on the server knew the difference, so the only thing standing between a failed delete and a resurrected song was a **client-side** tombstone -- which "Reset app data" wipes on its way out. That is why the reset path was the worst one: it destroys the only defence, then trusts an unconditional `{"ok": true}`. ## Changes **Server keeps its own deletion record.** `mark_deleted()` records an id; orphan recovery skips it; `persist()` writes it alongside the jobs. `_prune_deleted()` forgets a record once its directory is finally gone, so the set stays bounded instead of growing for the life of the install. With this, the client tombstone becomes belt-and-braces rather than load-bearing. **`_rmtree_job` reports its outcome** instead of logging and returning `None`, and retries once. On macOS the usual failure is Finder or Spotlight creating a `.DS_Store` between `rmtree`'s scan and its final `rmdir`, which leaves "Directory not empty" on a directory that is about to be empty again -- a retry clears exactly that. **`delete_job`** records the deletion either way, and returns an error when files remain rather than reporting success. **`reset_all` returns what it could not remove** and records the survivors so they cannot be re-adopted. `/api/reset` now returns `{"ok": true, "undeleted": N}`. **Client awaits both halves.** `markJobsDeleted` is awaited before the purge, and the DELETE calls are awaited with failures surfaced through `notifyFailure`. Both were fire-and-forget with `.catch(() => {})`. The button is disabled while it runs. ## Verification New `tests/test_deleted_jobs_stay_deleted.py`, 7 tests. Confirmed not vacuous -- removing the orphan-recovery skip makes 2 fail: ``` FAILED test_a_deleted_job_is_not_re_adopted FAILED test_reset_records_survivors_so_they_cannot_come_back ``` The first test in the file deliberately pins the behaviour we must **not** break: an orphan directory that was never deleted is still adopted. ``` ruff check All checks passed ruff format 95 files already formatted pytest 911 passed, 2 failed (the pre-existing ogg pair, unchanged) npm test:js 48/48 node --check catalog.js, i18n.js OK ``` ## Two things reviewers should weigh **`delete_job` now returns 500 when files remain**, after having already removed the registry entry and recorded the deletion. The row goes and stays gone -- which is what the user asked for -- but the response is an error so the failure is visible. The alternative was keeping the row and forcing a retry; that felt worse given the `.DS_Store` race usually clears on retry anyway. **`/api/reset`'s response shape changed.** `tests/test_reset.py` was updated for the new `undeleted` field. Any other consumer of that endpoint would need the same. ## Still unconfirmed The macOS `.DS_Store` race is a hypothesis, not a verified root cause. Checking `jobs/` for surviving directories and `backend.log` for `reset: could not remove` after a reset that appeared to work would settle it. The fix does not depend on that being the trigger -- the deletion record closes the resurrection path whatever caused the delete to fail.
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.
Fixes #520. Prerequisite for #521 -- that fix builds on
restore()'s hardening here.1. A cancel in the pop-to-claim window stranded the job forever
The worker pops at
jobqueue.py:213and only claims with_set_running()at:229. In between, the job is in neither the queue nor the running slot.A cancel landing there:
cancel_jobsetscancel_requested = Truejobqueue.discard()returns False -- already popped -- so it never setsstatus="cancelled", never callscleanup_job_dir, never persistsrunning_id()is stillNone, so the terminate branch is skippedcontinues -- "drop it silently"The job stayed at
"queued":pending_countcountsstatus == "queued", soregister_if_capacitycounts it forever_queue, not running, so absent from the queue view"queued"is in_RESUMABLE, hence_PERSISTEDThe worker is the sole consumer and owns the job once popped, so it now finalises the cancellation itself. Extracted as
_finalise_dropped_job()rather than inlined, so the drop path is directly testable and so an already-terminal job is explicitly left alone rather than having a real result rewritten.2. A malformed registry.json stopped the backend booting
restore()caught(OSError, json.JSONDecodeError, TypeError, ValueError). Aregistry.jsonthat is valid JSON but not an object --[1,2,3],null,"a string"-- makes_migratecalldata.get()and raiseAttributeError, which that tuple does not name.restore_registry(JOBS_DIR)runs at module scope inapp/main.py, so the import fails and the backend never starts. No self-healing, no user-visible recovery short of deleting the file by hand.Orphan recovery and the trailing
persist()sat outside the try entirely, so an unreadablejobs_dirwas fatal at startup for the same reason.Both are now caught broadly and logged. A registry we cannot use should cost the user their job list, not their app.
Verification
New
tests/test_registry_resilience.py, 11 tests.Confirmed not vacuous, both halves independently:
_finalise_dropped_jobcall ->test_cancel_between_pop_and_claim_finalises_the_jobfailsrestore()'s except back to the tuple -> 5 tests failThe cancel test deliberately drives the real
_worker_looprather than calling the helper, so it also catches the worker simply not calling it -- an earlier draft called the helper directly and did not catch that.The 2 failures are
test_stems_api.py::test_all_stems_zip_oggand::test_ogg_is_still_streamed, which fail identically on0.16.1without this change (verified on the base branch). Local ffmpeg without libvorbis, most likely -- unrelated, and not filed.