Skip to content

Commit f0feb91

Browse files
committed
fix: prevent head-of-line blocking in RedisWorker fetch_task() (#7900)
When many waiting tasks need the same blocked exclusive resource, fetch_task() now tracks blocked resources and excludes them from subsequent DB queries using reserved_resources_record__overlap, leveraging the partial GIN index. This allows workers to skip past thousands of blocked tasks and find free-resource tasks efficiently. The taken_exclusive/taken_shared sets accumulate across loop iterations to preserve FIFO ordering when blocked_resources exclusion changes the DB result set between iterations. Includes a reproduction test and changelog entry. Closes #7900
1 parent 4148977 commit f0feb91

3 files changed

Lines changed: 186 additions & 5 deletions

File tree

CHANGES/7900.bugfix

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixed head-of-line blocking in RedisWorker fetch_task() when thousands of tasks need the same blocked resource by excluding known-blocked resources from subsequent DB queries.

pulpcore/tasking/redis_worker.py

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -599,27 +599,41 @@ def fetch_task(self):
599599
3. If resource locks acquired, attempts to claim the task
600600
with a Redis task lock (24h expiration)
601601
4. Returns the first task for which both locks can be acquired
602-
5. If no task found in the batch, doubles the fetch limit and retries from the oldest
602+
5. Tracks blocked resources across iterations and excludes them at the DB level
603+
using the ``reserved_resources_record__overlap`` filter, leveraging the partial
604+
GIN index ``pulp_task_resources_index``
605+
606+
The ``blocked_resources`` set is local to each ``fetch_task()`` call — it resets
607+
between calls so that resources freed while the worker was busy are retried.
608+
FIFO ordering within a resource is preserved because all tasks needing a blocked
609+
resource are excluded together.
603610
604611
Returns:
605612
Task: A task object if one was successfully locked, None otherwise
606613
"""
607614
fetch_limit = FETCH_TASK_LIMIT
615+
blocked_resources = set()
616+
taken_exclusive = set()
617+
taken_shared = set()
608618

609619
while True:
610-
taken_exclusive = set()
611-
taken_shared = set()
612620

613-
waiting_tasks = list(
621+
qs = (
614622
Task.objects.filter(state=TASK_STATES.WAITING, app_lock=None)
615623
.exclude(pk__in=self.ignored_task_ids)
616624
.order_by("pulp_created")
617-
.select_related("pulp_domain")[:fetch_limit]
625+
.select_related("pulp_domain")
618626
)
627+
if blocked_resources:
628+
qs = qs.exclude(reserved_resources_record__overlap=list(blocked_resources))
629+
630+
waiting_tasks = list(qs[:fetch_limit])
619631

620632
if not waiting_tasks:
621633
break
622634

635+
prev_blocked_count = len(blocked_resources)
636+
623637
for task in waiting_tasks:
624638
try:
625639
exclusive_resources, shared_resources = extract_task_resources(task)
@@ -650,6 +664,18 @@ def fetch_task(self):
650664
shared_resources,
651665
)
652666
if blocked_resource_list:
667+
for resource_name in blocked_resource_list:
668+
# Redis returns bytes; decode for ORM compatibility.
669+
if isinstance(resource_name, bytes):
670+
resource_name = resource_name.decode()
671+
if resource_name == "__task_lock__":
672+
continue
673+
# Add the raw resource name (matches exclusive entries
674+
# in reserved_resources_record).
675+
blocked_resources.add(resource_name)
676+
# Also add the shared: variant so tasks referencing
677+
# this resource as shared are excluded too.
678+
blocked_resources.add(f"shared:{resource_name}")
653679
continue
654680

655681
rows = Task.objects.filter(
@@ -674,6 +700,11 @@ def fetch_task(self):
674700
pass
675701
continue
676702

703+
# If we learned new blocked resources, re-query with updated exclusions
704+
# (same fetch_limit — the exclusion yields different tasks).
705+
if len(blocked_resources) > prev_blocked_count:
706+
continue
707+
677708
if len(waiting_tasks) < fetch_limit:
678709
break
679710

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
"""Reproduction test for RedisWorker fetch_task() head-of-line blocking (issue #7900).
2+
3+
When many waiting tasks need the same blocked exclusive resource, fetch_task()
4+
should skip them at the DB level and find tasks for free resources without
5+
excessive acquire_locks calls.
6+
"""
7+
8+
from datetime import timedelta
9+
from unittest.mock import patch as mock_patch
10+
from uuid import uuid4
11+
12+
import pytest
13+
from django.conf import settings
14+
15+
from pulpcore.app.models import AppStatus, Domain, Task
16+
from pulpcore.app.redis_connection import get_redis_connection
17+
from pulpcore.constants import TASK_STATES
18+
from pulpcore.tasking.redis_locks import (
19+
acquire_locks as real_acquire,
20+
)
21+
from pulpcore.tasking.redis_locks import (
22+
resource_to_lock_key,
23+
safe_release_task_locks,
24+
)
25+
from pulpcore.tasking.redis_worker import RedisWorker
26+
27+
pytestmark = pytest.mark.skipif(
28+
settings.WORKER_TYPE != "redis",
29+
reason="Only runs with WORKER_TYPE=redis",
30+
)
31+
32+
33+
@pytest.mark.django_db
34+
def test_fetch_task_skips_blocked_resources():
35+
"""fetch_task() must skip tasks for blocked resources and find free ones.
36+
37+
Reproduces issue #7900: when many tasks need a blocked exclusive resource,
38+
fetch_task() should use DB-level exclusion to skip them and find tasks
39+
for free resources, without excessive acquire_locks calls.
40+
41+
With the bug (no DB-level exclusion):
42+
- The doubling algorithm (20->40->80->160->320) re-scans from position 0
43+
each iteration, calling acquire_locks once per iteration for the first
44+
blocked task. Total: ~6 acquire_locks calls for 200 blocked tasks.
45+
46+
With the fix (DB-level exclusion via __overlap):
47+
- After the first acquire_locks failure, the blocked resource is excluded
48+
from subsequent DB queries. The free-resource task is found directly.
49+
Total: 2 acquire_locks calls.
50+
"""
51+
redis_conn = get_redis_connection()
52+
domain = Domain.objects.get(name="default")
53+
domain_shared = f"shared:prn:core.domain:{domain.pk}"
54+
test_id = uuid4().hex[:8]
55+
redis_keys = []
56+
57+
AppStatus.objects._current_app_status = None
58+
app_status = AppStatus.objects.create(
59+
name=f"test-hol-{test_id}",
60+
app_type="worker",
61+
versions={},
62+
ttl=timedelta(seconds=30),
63+
)
64+
65+
worker = object.__new__(RedisWorker)
66+
worker.ignored_task_ids = list(
67+
Task.objects.filter(state=TASK_STATES.WAITING, app_lock=None).values_list("pk", flat=True)
68+
)
69+
worker.redis_conn = redis_conn
70+
worker.name = app_status.name
71+
worker.app_status = app_status
72+
73+
# Block one resource in Redis (simulates another worker holding it)
74+
blocked_resource = f"prn:test.hol-{test_id}:blocked"
75+
blocked_key = resource_to_lock_key(blocked_resource)
76+
redis_conn.set(blocked_key, "other-worker")
77+
redis_keys.append(blocked_key)
78+
79+
free_resource = f"prn:test.hol-{test_id}:free"
80+
81+
result = None
82+
try:
83+
# Create 200 tasks needing the blocked resource (fill the queue head)
84+
Task.objects.bulk_create(
85+
[
86+
Task(
87+
state=TASK_STATES.WAITING,
88+
name="pulpcore.app.tasks.test.sleep",
89+
logging_cid=f"hol-{test_id}-blocked-{i}",
90+
reserved_resources_record=[blocked_resource, domain_shared],
91+
pulp_domain=domain,
92+
)
93+
for i in range(200)
94+
]
95+
)
96+
97+
# Create 1 task needing a free resource (last in FIFO order)
98+
Task.objects.bulk_create(
99+
[
100+
Task(
101+
state=TASK_STATES.WAITING,
102+
name="pulpcore.app.tasks.test.sleep",
103+
logging_cid=f"hol-{test_id}-free",
104+
reserved_resources_record=[free_resource, domain_shared],
105+
pulp_domain=domain,
106+
)
107+
]
108+
)
109+
110+
# Count acquire_locks calls during fetch_task
111+
acquire_count = 0
112+
113+
def counting_acquire(*args, **kwargs):
114+
nonlocal acquire_count
115+
acquire_count += 1
116+
return real_acquire(*args, **kwargs)
117+
118+
with mock_patch(
119+
"pulpcore.tasking.redis_worker.acquire_locks",
120+
side_effect=counting_acquire,
121+
):
122+
result = worker.fetch_task()
123+
124+
# The free-resource task must be found
125+
assert result is not None, (
126+
"fetch_task() returned None -- failed to find the free-resource task "
127+
"behind 200 blocked tasks"
128+
)
129+
assert f"hol-{test_id}-free" in result.logging_cid, (
130+
f"fetch_task() returned wrong task: {result.logging_cid}"
131+
)
132+
133+
# With DB-level exclusion, acquire_locks should be called at most 3 times
134+
# (1 for the blocked resource + 1 for the free resource + margin).
135+
# Without the fix, the doubling algorithm calls it ~6 times.
136+
assert acquire_count <= 3, (
137+
f"acquire_locks called {acquire_count} times -- fetch_task() is "
138+
f"re-scanning blocked resources instead of excluding them at the DB level"
139+
)
140+
141+
finally:
142+
for key in redis_keys:
143+
redis_conn.delete(key)
144+
if result:
145+
safe_release_task_locks(result, lock_owner=worker.name)
146+
Task.objects.filter(pk=result.pk).update(app_lock=None, state=TASK_STATES.COMPLETED)
147+
Task.objects.filter(logging_cid__startswith=f"hol-{test_id}").delete()
148+
AppStatus.objects._current_app_status = None
149+
app_status.delete()

0 commit comments

Comments
 (0)