Reclaim stale cache locks and reject unknown fallbacks - #11
Open
prenc wants to merge 2 commits into
Open
Conversation
- 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__
- 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
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.
Summary
_directory_lock(sources.py:381) took a sibling.lockdirectory viamkdirand removed it only in the holder'sfinally, 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 aDownloadErrorimplying 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,CMSProvidersilently accepted arbitraryfallbackstrings; unsupported values now raiseValueErrorat construction, coveringfrom_cms/for_datethrough the shared constructor.Issue coverage
#2: Stale cache lock from a killed process blocks CMS cache access until manual cleanup
/procstarttime 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(plainmkdir),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)test_live_holder_lock_is_waited_not_reclaimed(0.3 s waiter raisesDownloadErrormatching "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 staletests/test_sources.py31/31 includingtest_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__raisesValueErrorfor anyfallbackother thanNone/"latest_for_fy", coveringfrom_cms/for_datethrough the shared constructor —test_unknown_fallback_value_is_rejected,test_from_cms_and_for_date_reject_unknown_fallback(the issue's typo example included)None/"latest_for_fy"—test_latest_for_fy_fallback_is_explicitpasses unmodifiedChanges
src/cms_icd/sources.py—_directory_lockrewritten 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_FYmodule 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
FileNotFoundError(e.g. a transient NFS error) classifies the lock as not stale, so a live holder is never reclaimed on transient I/O.knowledge_base.pyedits —from_cms/for_dateroute through theCMSProviderconstructor, so validation is inherited._reclaim_stale_lock(with a deterministic regression test); stress re-runs showed no recurrence.Risks or follow-up
The marker-less creation window between
mkdirand 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