Fix lost expiration index updates in the filesystem lease storage - #22971
Open
afonsojanu wants to merge 5 commits into
Open
Fix lost expiration index updates in the filesystem lease storage#22971afonsojanu wants to merge 5 commits into
afonsojanu wants to merge 5 commits into
Conversation
Two lease renewals landing close together could interleave inside _update_expiration_index: both read the same version of the shared expirations.json, and whichever one saved last silently threw away the other's write. The reaper only ever looks at that index to decide what's expired, so a single lost update was enough to revoke a lease that was still very much alive. This adds an asyncio.Lock around the read, modify, and write of the index so two updates can no longer race each other. It also has read_expired_lease_ids check the actual lease file before reporting something as expired, since that file is what renew_lease really stamps and the index is only meant to be a shortcut to it. If the index is ever stale for some other reason down the line, this keeps that stale entry from being fatal instead of harmless. Added a test that forces the interleaving deterministically (delaying the first read just enough for the second update to finish first) and one that backdates an index entry while leaving the lease file alone, confirming the reaper no longer trusts it blindly. Both fail against the old code and pass with the fix. Closes PrefectHQ#22935
afonsojanu
requested review from
chrisguidry,
desertaxle and
zzstoatzz
as code owners
August 28, 2026 16:48
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Merging this PR will not alter performance
Comparing 
Footnotes
|
get_concurrency_lease_storage() builds a fresh ConcurrencyLeaseStorage per call, so the lock added to serialize concurrent index renewals was an instance attribute that never actually served that purpose - each caller held its own lock and could still interleave with another request's read-modify-write of expirations.json. Moving the lock to a class attribute means every instance in the process shares it, which is what the guard was meant to do.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The lock guarding the expiration index's read-modify-write cycle was a single asyncio.Lock created once at class definition time. An asyncio.Lock only binds to an event loop once something actually contends on it, and raises RuntimeError if a different loop later contends on it too. That's exactly what happens across an embedded server restart or a second asyncio.run() call in the same process: the first loop's contention binds the lock, and any later loop that also needs to serialize concurrent renewals blows up instead. The lock is now looked up from a WeakKeyDictionary keyed by the running event loop, so each loop gets its own lock and old ones are freed once their loop is gone, while still serializing renewals across the fresh ConcurrencyLeaseStorage instances that get_concurrency_lease_storage() builds per call. Added a regression test that forces genuine lock contention once on the fixture's own event loop and then again on a freshly created one, confirming the second round no longer raises.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…le lock A per-event-loop asyncio.Lock registry fixed the RuntimeError from an embedded server restart binding a stale lock, but it never actually serialized concurrent access: two loops running at the same time (two worker threads, each with its own loop) each get a different lock from the registry, so their read-modify-write cycles on the expiration index still race and one write clobbers the other. Replace the registry with the same FileLock used elsewhere in this codebase (GitRepository.pull_code, the server startup migration lock). It's not bound to any event loop or thread, so a single lock file next to the index serializes renewals across loops, threads, and even separate processes sharing the same storage_path, which this backend already needs to tolerate given it stores everything on disk. Its aacquire() polls instead of blocking the event loop while waiting. Added a regression test that runs two updates concurrently on separate threads, each with its own asyncio.run() loop, and confirms both index entries survive instead of one clobbering the other.
Comment on lines
+692
to
+700
| async def load_with_a_delay_on_the_first_call(): | ||
| nonlocal call_count | ||
| call_count += 1 | ||
| result = await original_load() | ||
| if call_count == 1: | ||
| # Give the second update a chance to read, write, and finish | ||
| # before this one acts on what it already read. | ||
| await asyncio.sleep(0.05) | ||
| return result |
Contributor
There was a problem hiding this comment.
🔍 Concurrency tests rely on scheduler timing
Fixed sleeps do not guarantee the intended interleaving, and timed joins never verify thread termination. Replace them with explicit synchronization to satisfy the deterministic-test rule in the test guidance.
Was this helpful? React with 👍 or 👎 to provide feedback.
join(timeout=...) returning tells you nothing about whether the thread finished or just timed out, so a real deadlock would slip through as a silent pass. Assert not is_alive() after each join instead.
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
1 participant
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.
Closes #22935
Two lease renewals can land close enough together that they both read the same snapshot of the shared
expirations.jsonindex before either one writes back. Whichever one saves last wins, and the other's update just disappears. Since the reaper only ever looks at that index to decide what's expired, losing a single update is enough to have it revoke a lease that's still perfectly alive. The reporter hit this in production on a long-running flow that got killed with a bare 410 about half an hour in, with every renewal up to that point having succeeded.I added an
asyncio.Lockaround the read, modify, and write of the index in_update_expiration_indexand_remove_from_expiration_index, so two updates can't interleave anymore.I also changed
read_expired_lease_idsto check the actual lease file before reporting something as expired. The lease file is whatrenew_leasestamps directly, so it's the source of truth, and the index is only supposed to be a shortcut to it. This means that if the index ever ends up stale for some other reason down the road, a stale entry becomes harmless instead of fatal. If the lease file is missing entirely, that entry really is orphaned, so it still gets reported as expired and cleaned up.For tests, I added one that forces the interleaving deterministically (delaying the first index read just long enough for a second concurrent update to finish first, instead of hoping the event loop schedules it unluckily), and one that backdates an index entry while leaving the lease file untouched, to confirm the reaper no longer trusts a stale index on its own. Both fail against the code on main and pass with the fix.
Ran the full
tests/server/concurrency/andtests/server/services/test_repossessor.pysuites locally (61 tests, all passing), plus ruff and mypy on the changed files with nothing new flagged.