Skip to content

fix(ingest): keep ProcessLock exclusive when a contender fails - #1659

Open
SrkianFardin25 wants to merge 1 commit into
crocodilestick:mainfrom
SrkianFardin25:fix-1657-process-lock
Open

SrkianFardin25 wants to merge 1 commit into
crocodilestick:mainfrom
SrkianFardin25:fix-1657-process-lock

Conversation

@SrkianFardin25

Copy link
Copy Markdown

Closes #1657

Problem

ProcessLock.acquire() in scripts/ingest_processor.py opened the lock file with open(path, 'w+'), which truncates it before flock is tried. So a second caller:

  1. erased the owner's PID,
  2. failed flock,
  3. read an empty (non-digit) PID, decided the lock was stale and called _cleanup_stale_lock(), which unlinked the path while the owner still held its kernel lock.

A third caller then created a new inode at the same path and got the lock, so two ingest processes could run at the same time. release() also unlinked the path, which can delete a lock file that another process has just created.

The same "non-PID content means stale" check also fired while cps/admin.py holds this lock during a library restore, because it writes restore_calibre_db into the file instead of a PID.

Fix

flock is now the only source of truth, which matches how the kernel behaves: a flock is dropped when the holder's descriptors close, including when the process dies. A lock left by a dead process is never held, so no stale-lock cleanup is needed.

  • Open the file with os.open(..., O_RDWR | O_CREAT) (no truncation) and write the PID only after flock succeeds.
  • A contender that cannot get flock just waits and retries until the timeout. It never truncates or unlinks the file.
  • release() clears the PID while it still holds the lock, unlocks, and leaves the file in place.
  • Removed _check_stale_lock() and _cleanup_stale_lock(). Nothing else referenced them. _get_holding_pid() is unchanged and now reads a PID that a contender can no longer erase.

The lock path, name, timeout and return values are unchanged, so initialize_runtime() and the restore code in cps/admin.py work as before. The cwa-init service still deletes leftover lock files at container start.

Tests

Added tests/unit/test_ingest_process_lock.py (marked unit):

  • test_failed_contender_keeps_owner_lock_file: owner acquires; a contender times out; the path still exists with the owner's inode and PID; a third caller also fails; after the owner releases, the third caller gets the same inode.
  • test_contender_does_not_treat_non_pid_content_as_stale: a holder that writes restore_calibre_db (like cps/admin.py) keeps its file.
  • test_unlocked_file_left_by_dead_process_is_reused: an unlocked file with a dead PID is taken over, and the PID is cleared on release.

All three fail on main (the path is unlinked) and pass with this change.

What I ran locally (Linux, Python 3.13, requirements.txt + requirements-dev.txt; python-ldap, Flask-SimpleLDAP, py7zr and faust-cchardet were left out because they need a C compiler my environment does not have):

PYTHONPATH=.:scripts pytest tests/unit/test_ingest_process_lock.py
  main:     3 failed
  this PR:  3 passed

PYTHONPATH=.:scripts pytest -m "smoke or unit" -n 2
  273 passed, 27 skipped, 1 failed
  The one failure is tests/smoke/test_smoke.py::TestBasicFunctionality::test_required_directories_exist,
  which asserts that /config exists. My environment has no /config directory, and the
  failure is unrelated to this change.

ruff check scripts/ingest_processor.py tests/unit/test_ingest_process_lock.py
  The new test file is clean. ingest_processor.py goes from 67 to 65 existing findings,
  and this change adds none.

I did not run the Docker integration tests (tests/docker, tests/integration). Those are left to CI.

ProcessLock.acquire opened the lock file with "w+", which truncated the
owner's PID before flock was even tried. The contender then read an
empty PID, treated the lock as stale and unlinked the path while the
owner still held its flock. A third caller created a new inode and
acquired it, so two ingest processes could run at once. release() also
unlinked the path, which could remove a lock file another process had
just created.

The lock now relies on flock alone: the kernel drops a flock when the
holder's descriptors close (including when the process dies), so a lock
left by a dead process is never held and needs no cleanup. The file is
opened without truncation, the PID is written only after flock
succeeds, contenders only wait and retry, and release() clears the PID
but leaves the file in place.

This also covers the restore in cps/admin.py, which holds this lock
with "restore_calibre_db" written in it instead of a PID; a contender
no longer treats that as stale and unlinks it.

Fixes crocodilestick#1657
@SrkianFardin25
SrkianFardin25 marked this pull request as ready for review November 6, 2026 10:44
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.

bug: ProcessLock contender unlinks a live lock and allows another owner

1 participant