Skip to content

fix(registry): free a job cancelled in the pop-to-claim window, and boot with a bad registry - #526

Merged
thcp merged 1 commit into
0.16.1from
fix/520-registry-phantom-job
Aug 31, 2026
Merged

fix(registry): free a job cancelled in the pop-to-claim window, and boot with a bad registry#526
thcp merged 1 commit into
0.16.1from
fix/520-registry-phantom-job

Conversation

@thcp

@thcp thcp commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

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:213 and only claims with _set_running() at :229. In between, the job is in neither the queue nor the running slot.

A cancel landing there:

  1. cancel_job sets cancel_requested = True
  2. jobqueue.discard() returns False -- already popped -- so it never sets status="cancelled", never calls cleanup_job_dir, never persists
  3. running_id() is still None, so the terminate branch is skipped
  4. The worker sees the flag and continues -- "drop it silently"

The job stayed at "queued":

  • Permanent capacity loss -- pending_count counts status == "queued", so register_if_capacity counts it forever
  • Invisible -- not in _queue, not running, so absent from the queue view
  • Disk never freed -- a queued upload holds its source, up to 400 MB
  • Resurrected every restart -- "queued" is in _RESUMABLE, hence _PERSISTED

The 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). A registry.json that is valid JSON but not an object -- [1,2,3], null, "a string" -- makes _migrate call data.get() and raise AttributeError, which that tuple does not name.

restore_registry(JOBS_DIR) runs at module scope in app/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 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.

Verification

New tests/test_registry_resilience.py, 11 tests.

Confirmed not vacuous, both halves independently:

  • Reverting the worker's _finalise_dropped_job call -> test_cancel_between_pop_and_claim_finalises_the_job fails
  • Reverting restore()'s except back to the tuple -> 5 tests fail

The cancel test deliberately drives the real _worker_loop rather 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.

ruff check       All checks passed
ruff format      94 files already formatted
pytest tests/    904 passed, 2 failed

The 2 failures are test_stems_api.py::test_all_stems_zip_ogg and ::test_ogg_is_still_streamed, which fail identically on 0.16.1 without this change (verified on the base branch). Local ffmpeg without libvorbis, most likely -- unrelated, and not filed.

…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
thcp marked this pull request as ready for review August 31, 2026 21:02
@thcp
thcp merged commit cdb68fc into 0.16.1 Aug 31, 2026
8 checks passed
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant