Skip to content

Reclaim stale cache locks and reject unknown fallbacks - #11

Open
prenc wants to merge 2 commits into
mainfrom
issues-2-sources-lock-fallback
Open

Reclaim stale cache locks and reject unknown fallbacks#11
prenc wants to merge 2 commits into
mainfrom
issues-2-sources-lock-fallback

Conversation

@prenc

@prenc prenc commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

_directory_lock (sources.py:381) took a sibling .lock directory via mkdir and removed it only in the holder's finally, so a killed holder (job cancel, OOM, kill -9) left a holder-less lock that made every later caller time out after a fixed 30 s with a DownloadError implying a live holder, until the directory was manually deleted. The lock is now liveness-aware: the holder records its PID atomically inside the lock directory; a lock whose marker is missing, invalid, or whose recorded PID is dead, a zombie, or reused for a newer process is reclaimed immediately; a lock held by a live process is waited on for an explicit, sized timeout (default 600 s) and the timeout names the live holder PID (or notes that no live holder could be removed). Separately, CMSProvider silently accepted arbitrary fallback strings; unsupported values now raise ValueError at construction, covering from_cms/for_date through the shared constructor.

Issue coverage

#2: Stale cache lock from a killed process blocks CMS cache access until manual cleanup

  • A holder-less lock is detected and reclaimed so a killed process cannot permanently block a cache destination — staleness detection covers missing marker, invalid marker, dead holder PID, zombie state, and Linux /proc starttime after lock creation (PID reuse); a reclaim re-verifies the moved directory and restores the lock if the holder's atomic marker write landed while the rename was in flight, so a live holder can never lose its lock; test_stale_lock_without_holder_is_reclaimed (plain mkdir), test_stale_lock_with_dead_holder_pid_is_reclaimed (skipped if the dead PID is reused before the probe), test_reclaim_restores_lock_whose_marker_landed_before_rename (deterministic)
  • Waiters do not fail spuriously while a healthy holder still works — test_live_holder_lock_is_waited_not_reclaimed (0.3 s waiter raises DownloadError matching "waiting for cache lock", the live lock is not reclaimed, and acquisition succeeds after release); release renames the whole directory aside atomically, so a releasing holder never leaves a marker-less window a waiter could mistake for stale
  • Existing concurrency and corrupt-cache tests pass unmodified — tests/test_sources.py 31/31 including test_concurrent_requests_share_one_download (stress loop after the reclaim hardening: 9 fast passes, 1 environmental NFS stall, 0 test failures); full offline suite (doctests included) 85 passed

#8: Unknown fallback value is silently ignored in CMS release selection

  • CMSProvider.__init__ raises ValueError for any fallback other than None/"latest_for_fy", covering from_cms/for_date through the shared constructor — test_unknown_fallback_value_is_rejected, test_from_cms_and_for_date_reject_unknown_fallback (the issue's typo example included)
  • Focused pytest case asserting rejection and unchanged behavior for None/"latest_for_fy"test_latest_for_fy_fallback_is_explicit passes unmodified

Changes

  • src/cms_icd/sources.py_directory_lock rewritten around an atomic PID marker (_write_lock_holder), staleness detection (_lock_is_stale), reclaim-aside (_reclaim_stale_lock) that re-verifies the moved directory and restores the lock if the holder's atomic marker write landed while the rename was in flight, an explicit 600 s default timeout with holder-naming timeout errors, and an atomic rename-aside release; _FALLBACK_LATEST_FOR_FY module constant used in __init__ validation and the single existing consumption site.
  • tests/test_sources.py — six focused tests (five from the original implementation plus the deterministic reclaim-restore regression test).

Deviations / Non-goals

  • A marker read failure other than FileNotFoundError (e.g. a transient NFS error) classifies the lock as not stale, so a live holder is never reclaimed on transient I/O.
  • Non-POSIX platforms conservatively treat a recorded PID as alive (no signal-based liveness probing).
  • No knowledge_base.py edits — from_cms/for_date route through the CMSProvider constructor, so validation is inherited.
  • Round 2 review: a full-suite re-run reproduced a one-in-five failure of the existing concurrency test; the root cause was the reclaim rename overtaking the holder's in-flight atomic marker write, letting two threads enter the same critical section. Fixed by the re-verify-and-restore in _reclaim_stale_lock (with a deterministic regression test); stress re-runs showed no recurrence.

Risks or follow-up

The marker-less creation window between mkdir and the atomic marker write can still be reclaimed by a concurrent waiter; the reclaimer re-verifies the moved directory and restores the lock if the holder's marker write completed in flight, otherwise the would-be holder's marker write fails and it retries acquisition (bounded by the deadline). Cache layout, manifest schema, and parsing of valid materials are unchanged, consistent with the reproducibility rule.

Closes #2
Closes #8

- Make the cache directory lock liveness-aware: record the holder PID,
reclaim holder-less locks, wait on live holders, and name the holder on
timeout so a killed process cannot block a cache destination
- Reject unsupported fallback values with ValueError in CMSProvider.__init__
@prenc prenc added in-progress Work is actively underway and removed in-progress Work is actively underway labels Aug 29, 2026
- Re-check the moved lock directory for a live holder before deleting,
  restoring the lock when the holder's atomic marker write completed
  while the reclaim rename was in flight
- Remove a cache lock on marker-write failure only when its marker
  identifies this process
- Add a deterministic regression test for the restore path
@prenc prenc removed the in-progress Work is actively underway label Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant