feat(uptime): Add ability to use queues to manage parallelism#2
feat(uptime): Add ability to use queues to manage parallelism#2Frawatson wants to merge 1 commit into
Conversation
One potential problem we have with batch processing is that any one slow item will clog up the whole batch. This pr implements a queueing method instead, where we keep N queues that each have their own workers. There's still a chance of individual items backlogging a queue, but we can try increased concurrency here to reduce the chances of that happening <!-- Describe your PR here. -->
🤖 Grapple PR ReviewSummaryThis PR adds a queue-based parallelism mechanism for managing Kafka consumer workloads in the uptime/remote subscriptions system, introducing a QueueConsumer abstraction that allows controlled parallel processing of messages. The implementation wraps existing result consumers to distribute work across configurable queues. Changes: 7 files (+1276 / -6 lines)
Resolves: #9 (Optimize spans buffer insertion with eviction during insert) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 __init__.py +2/-2"]
pr_0 --> f1
f2["🟢 queue_consumer.py +345/-0"]
pr_0 --> f2
f3["🟡 result_consumer.py +41/-3"]
pr_0 --> f3
f4["🟢 __init__.py +0/-0"]
pr_0 --> f4
f5["🟢 __init__.py +0/-0"]
pr_0 --> f5
f6["🟢 test_queue_consumer.py +421/-0"]
pr_0 --> f6
f7["🟡 test_results_consumer.py +467/-1"]
pr_0 --> f7
f2 -.- f3
f5 -.- f6
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
|
CRITICAL — The |
|
MAJOR — The internal queues appear to have no enforced maximum size (or use unbounded |
|
MAJOR — The |
|
MAJOR — There is no validation that |
|
MAJOR — Moving |
|
MINOR — If a queue worker thread raises an unhandled exception, the behavior depends on whether exceptions are caught in the worker loop. The PR intent states 'Workers processing queued messages should handle exceptions without crashing the entire consumer process', but without seeing the full worker loop, swallowed exceptions would cause silent message loss — the offset would eventually be committed without the message being processed. |
|
MINOR — The |
|
MINOR — The intent specification asks about backpressure behavior and memory pressure from unbounded queue growth. Based on the visible code using |
|
MINOR — In the |
|
MINOR — The static scanner flagged |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a queue-based parallelism management system for uptime/remote subscription consumers, introducing a QueueConsumer abstraction that allows Kafka consumers to process messages concurrently using internal queues rather than sequentially. The implementation wraps existing result consumers to enable configurable parallel processing of subscription check results.
Changes: 7 files (+1276 / -6 lines)
- 🟢 4 files added:
src/sentry/remote_subscriptions/consumers/queue_consumer.py,tests/sentry/remote_subscriptions/__init__.py,tests/sentry/remote_subscriptions/consumers/__init__.py,tests/sentry/remote_subscriptions/consumers/test_queue_consumer.py - 🟡 3 files modified:
src/sentry/consumers/__init__.py,src/sentry/remote_subscriptions/consumers/result_consumer.py,tests/sentry/uptime/consumers/test_results_consumer.py
Resolves: #9 (Optimize spans buffer insertion with eviction during insert)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 __init__.py +2/-2"]
pr_0 --> f1
f2["🟢 queue_consumer.py +345/-0"]
pr_0 --> f2
f3["🟡 result_consumer.py +41/-3"]
pr_0 --> f3
f4["🟢 __init__.py +0/-0"]
pr_0 --> f4
f5["🟢 __init__.py +0/-0"]
pr_0 --> f5
f6["🟢 test_queue_consumer.py +421/-0"]
pr_0 --> f6
f7["🟡 test_results_consumer.py +467/-1"]
pr_0 --> f7
f2 -.- f3
f5 -.- f6
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
4 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 4 |
| 🟠 Major | 10 |
| 🟡 Minor | 10 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-07T21:08:56.417Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 132.2s
- Low-confidence findings suppressed: 6
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): |
There was a problem hiding this comment.
🔴 CRITICAL — Offset Commit Correctness (confidence: 100%)
The _get_partition_lock method has a race condition: two threads can simultaneously find lock is None, both call setdefault, but the first thread may proceed with a different lock object than what was actually stored. While dict.setdefault is atomic in CPython due to the GIL, the initial self.partition_locks.get(partition) check followed by setdefault creates a TOCTOU pattern. More critically, self.partition_locks itself is a plain dict being accessed from multiple threads without synchronization — if a new partition is encountered concurrently from two threads, dict mutation during iteration in get_committable_offsets (which iterates self.all_offsets.keys()) could cause issues.
Evidence:
- Line 56:
lock = self.partition_locks.get(partition)— no global lock protects this - Line 58:
return self.partition_locks.setdefault(partition, threading.Lock())— while setdefault is atomic in CPython, the pattern is not thread-safe in general - The OffsetTracker is accessed from both the main consumer thread (add_offset) and worker threads (complete_offset) concurrently
- get_committable_offsets iterates
self.all_offsets.keys()while worker threads may be modifying the dict via complete_offset
Agent: logic
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): | ||
| self.all_offsets[partition].add(offset) |
There was a problem hiding this comment.
🔴 CRITICAL — Thread Safety / Race Condition (confidence: 100%)
The _get_partition_lock method has a TOCTOU race condition. Two threads could both check self.partition_locks.get(partition) simultaneously, find it missing, and both call setdefault. While dict.setdefault is atomic in CPython due to the GIL, this is an implementation detail and not guaranteed. More critically, self.partition_locks itself is a plain dict that is being mutated from multiple threads without synchronization. The _get_partition_lock method is called from add_offset and complete_offset which can be called from different threads (main thread and worker threads respectively).
Evidence:
- add_offset is called from the main consumer thread in SimpleQueueProcessingStrategy.submit()
- complete_offset is called from worker threads in FixedQueuePool._worker()
- Both call _get_partition_lock which reads and writes to self.partition_locks without a global lock
- defaultdict mutation for self.all_offsets and self.outstanding also happens inside per-partition locks, but new partitions could cause dict resize from multiple threads
Agent: architecture
| return self.partition_locks.setdefault(partition, threading.Lock()) | ||
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" |
There was a problem hiding this comment.
🔴 CRITICAL — Concurrency issues (confidence: 100%)
Race condition in _get_partition_lock: the check-then-set pattern for creating partition locks is not atomic, potentially creating two locks for the same partition under concurrent access.
Evidence:
- The method first checks
self.partition_locks.get(partition), then callssetdefault— but between the get and setdefect, another thread could have inserted a different lock object. - While
dict.setdefaultis atomic in CPython due to the GIL, the initialgetcall creates a TOCTOU window: the code pathif lock: return lockmay return a stale reference if the dict is being concurrently modified. - More critically,
self.all_offsetsandself.outstandinginadd_offset/complete_offsetaredefaultdict(set), which are NOT thread-safe. Multiple threads callingadd_offsetconcurrently on a new partition could race ondefaultdict.__missing__, corrupting the set. - The partition lock is only acquired AFTER accessing
self.all_offsets[partition]andself.outstanding[partition]inadd_offset— no, wait, looking again:add_offsetacquires the lock first. Butget_committable_offsetsiteratesself.all_offsets.keys()without a global lock while other threads may be adding new partitions.
Agent: performance
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🔴 CRITICAL — Concurrency issues (confidence: 100%)
get_committable_offsets calls min(all_offsets) and max(all_offsets) while holding per-partition locks, but the outer loop iterates self.all_offsets.keys() without any lock, creating a race with add_offset adding new partitions.
Evidence:
for partition in list(self.all_offsets.keys())— thelist()snapshot is taken without a lock, so a concurrentadd_offsetcall on a new partition may add a key toall_offsetsbut not yet have acquired the per-partition lock, leading to incomplete state being read.- The inner lock only protects per-partition operations, not the cross-partition key enumeration.
- This is called from the main Kafka poll thread while worker threads call
complete_offsetconcurrently.
Agent: performance
|
|
||
| worker = OrderedQueueWorker[T]( | ||
| worker_id=i, | ||
| work_queue=work_queue, |
There was a problem hiding this comment.
🟠 MAJOR — Error Handling / Silent Message Loss (confidence: 100%)
In the _worker method, when process_func raises an exception (line ~182), the exception is caught and logged, but the offset is still marked as completed via offset_tracker.complete_offset() in the finally block (line ~189). This means a failed message is silently skipped — its offset is committed as if it succeeded. The intent specification asks 'What happens when a worker processing a queue raises an unhandled exception — does the consumer halt, retry, or skip the message?' The answer here is 'skip', which could cause silent data loss.
Evidence:
- Lines 182-186: except block logs the error but continues
- Lines 188-189: finally block always calls offset_tracker.complete_offset()
- This means failed messages are treated as successfully processed for offset commit purposes
- The intent specification flags this as a risk: 'unhandled exceptions in worker threads may silently drop messages'
Agent: logic
| self.partition_locks: dict[Partition, threading.Lock] = {} | ||
|
|
||
| def _get_partition_lock(self, partition: Partition) -> threading.Lock: | ||
| """Get or create a lock for a partition.""" |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 87%)
Missing module-level docstring explaining the purpose, architecture, and key design decisions of the QueueConsumer system. This is important for understanding offset commit semantics, concurrency model, and when to use this mode.
Evidence:
- Module starts directly with imports without explaining the queue-based parallelism strategy
- Complex offset tracking and queue management logic would benefit from high-level architecture documentation
- Risk areas identified mention critical offset commit and deadlock concerns that should be documented at module level
Agent: style
| self, | ||
| result_processor: Callable[[str, T], None], | ||
| identifier: str, | ||
| num_queues: int = 20, |
There was a problem hiding this comment.
🟡 MINOR — Naming conventions (confidence: 80%)
Inconsistent variable naming in get_committable_offsets(): uses both min_offset/max_offset and min_outstanding_offset within the same method logic without clear distinction of their purposes.
Evidence:
- Lines 168-169 compute
min_offsetandmax_offsetfrom all_offsets - This appears unused in the subsequent loop logic for finding contiguous ranges
- Variable naming should clarify whether we're looking at outstanding vs all offsets
Agent: style
| def __init__( | ||
| self, | ||
| mode: Literal["batched-parallel", "parallel", "serial"] = "serial", | ||
| mode: Literal["batched-parallel", "parallel", "serial", "thread-queue-parallel"] = "serial", |
There was a problem hiding this comment.
🟡 MINOR — Code organization (confidence: 75%)
The result_processor is instantiated early in __init__ (line 108) before mode-specific initialization, but this could be problematic if subclasses rely on instance attributes set during mode initialization. The instantiation order may cause subtle bugs if result_processor_cls() expects certain attributes to already be configured.
Evidence:
- Line 108 instantiates
self.result_processor = self.result_processor_cls()before mode-specific setup (batched-parallel, multiprocessing, thread-queue-parallel) - Original code (before diff) instantiated it after all mode setup at the end of
__init__ - Moving instantiation earlier could cause initialization order issues if the processor depends on instance state
Agent: style
| """ | ||
| queue_index = self.get_queue_for_group(group_key) | ||
| work_queue = self.queues[queue_index] | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Thread Safety (confidence: 72%)
The SimpleQueueProcessingStrategy accesses self._closed and self._shutdown flags from potentially different threads without synchronization. While in practice these are likely only set from the main thread and read from the main thread, the _shutdown flag on FixedQueuePool is set from the main thread but read from worker threads without a memory barrier (no threading.Event or lock). In CPython the GIL makes boolean reads/writes atomic, but this is an implementation detail.
Evidence:
- FixedQueuePool._shutdown is a plain bool accessed from main thread (setter) and worker threads (reader in _worker loop)
- No threading.Event or volatile equivalent is used
- This works in CPython due to GIL but is technically a data race
Agent: logic
| Does the consumer use thread-queue-parallel processing? | ||
| """ | ||
|
|
||
| multiprocessing_pool: MultiprocessingPool | None = None |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 77%)
New attributes thread_queue_parallel and queue_pool are added to ResultsStrategyFactory without a docstring explaining when they are used or their lifecycle.
Evidence:
- Line 94-95:
thread_queue_parallel = Falseandqueue_pool: FixedQueuePool | None = Noneare added as class attributes - No docstring explaining that these are conditionally initialized based on mode='thread-queue-parallel'
- Readers may not understand the initialization pattern
Agent: style
| self.result_processor = result_processor | ||
| self.identifier = identifier | ||
| self.num_queues = num_queues | ||
| self.offset_tracker = OffsetTracker() |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (7 lines, 1 file))
Worker queues have no maximum depth configured, allowing unbounded memory growth if workers fall behind processing speed.
| self.offset_tracker = OffsetTracker() | |
| max_queue_depth: int = 1000, | |
| self.max_queue_depth = max_queue_depth | |
| # Bounded queue: blocks the producer (Kafka consumer thread) when full, | |
| # applying back-pressure instead of growing without bound and causing OOM. | |
| # Total max in-flight items = num_queues * max_queue_depth. | |
| work_queue: queue.Queue[WorkItem[T]] = queue.Queue(maxsize=max_queue_depth) |
🤖 Grapple PR auto-fix • major • confidence: 100%
| ) | ||
| finally: | ||
| self.offset_tracker.complete_offset(work_item.partition, work_item.offset) | ||
| metrics.gauge( |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
The FixedQueuePool constructor accepts
num_queuesbut does not validate that it is a positive integer. The intent specification explicitly calls out: 'Behavior when queue count is 0 or negative — should fail fast with a clear error.' With num_queues=0,self.queueswould be empty,self.workerswould be empty, andenqueuewould fail with a ZeroDivisionError onhash(item.result) % len(self.queues)(or similar). With negative values, the range() would produce no items.
| metrics.gauge( | |
| if not isinstance(num_queues, int) or num_queues <= 0: | |
| raise ValueError( | |
| f"num_queues must be a positive integer, got {num_queues!r}. " | |
| "Pass a value >= 1 to ensure at least one worker queue is created." | |
| ) |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| click.Option( | ||
| ["--mode", "mode"], | ||
| type=click.Choice(["serial", "parallel", "batched-parallel"]), | ||
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The new mode
thread-queue-parallelis added to the CLI choices, but there's no validation downstream thatmax_workersis provided or has a sensible value when this mode is selected. The default falls back tomax_workers or 20inresult_consumer.py, but if someone passes--max-workers 0or a negative number, the behavior is undefined. TheFixedQueuePool.__init__doesn't validatenum_queues.
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), | |
| help="The maximum number of workers/queues to use when in a parallel mode. Must be a positive integer when using thread-queue-parallel mode.", |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| self, | ||
| result_processor: Callable[[str, T], None], | ||
| identifier: str, | ||
| num_queues: int = 20, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (10 lines, 1 file))
Inconsistent variable naming in
get_committable_offsets(): uses bothmin_offset/max_offsetandmin_outstanding_offsetwithin the same method logic without clear distinction of their purposes.
| num_queues: int = 20, | |
| # Bounds of all offsets seen for this partition (not just outstanding ones) | |
| all_offsets_min = min(all_offsets) | |
| all_offsets_max = max(all_offsets) | |
| # Start from whichever is higher: the offset after last commit, or the earliest seen offset | |
| start = max(last_committed + 1, all_offsets_min) | |
| for offset in range(start, all_offsets_max + 1): |
🤖 Grapple PR auto-fix • minor • confidence: 80%
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (25 lines, 1 file) — review recommended)
The
_get_partition_lockmethod has a race condition: two threads can simultaneously findlockis None, both callsetdefault, but the first thread may proceed with a different lock object than what was actually stored. Whiledict.setdefaultis atomic in CPython due to the GIL, the initialself.partition_locks.get(partition)check followed bysetdefaultcreates a TOCTOU pattern. More critically,self.partition_locksitself is a plain dict being accessed from multiple threads without synchronization — if a new partition is encountered concurrently from two threads, dict mutation during iteration inget_committable_offsets(which iteratesself.all_offsets.keys()) could cause issues.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -38,9 +38,15 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Global lock guards creation of per-partition locks only.
+ # Once a partition lock exists, this lock is not needed for
+ # add_offset / complete_offset / mark_committed (fast path).
+ self._global_lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: partition lock already exists, no global lock needed.
+ # dict reads are safe under CPython GIL for existing keys, and we
+ # only mutate partition_locks under _global_lock below.
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ # Slow path: create the partition lock under the global lock to
+ # eliminate the TOCTOU between get() and setdefault().
+ with self._global_lock:
+ # Re-check inside the lock: another thread may have created
+ # the lock between our get() above and acquiring _global_lock.
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ lock = threading.Lock()
+ self.partition_locks[partition] = lock
+ return lock🤖 Grapple PR auto-fix • critical • Review this diff before applying
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): | ||
| self.all_offsets[partition].add(offset) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (19 lines, 1 file) — review recommended)
The
_get_partition_lockmethod has a TOCTOU race condition. Two threads could both checkself.partition_locks.get(partition)simultaneously, find it missing, and both callsetdefault. Whiledict.setdefaultis atomic in CPython due to the GIL, this is an implementation detail and not guaranteed. More critically,self.partition_locksitself is a plain dict that is being mutated from multiple threads without synchronization. The_get_partition_lockmethod is called fromadd_offsetandcomplete_offsetwhich can be called from different threads (main thread and worker threads respectively).
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -43,12 +43,22 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Protects partition_locks dict itself against concurrent reads/writes
+ # from multiple threads (main consumer thread via add_offset, worker
+ # threads via complete_offset). Per-partition locks are acquired
+ # separately *after* releasing this meta-lock so that unrelated
+ # partitions never contend on a single global lock.
+ self._meta_lock: threading.Lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: partition already registered — read under meta-lock to
+ # avoid a TOCTOU race on the underlying dict (which is not thread-safe
+ # for concurrent reads during a resize triggered by another writer).
+ with self._meta_lock:
+ lock = self.partition_locks.get(partition)
+ if lock is None:
+ lock = threading.Lock()
+ self.partition_locks[partition] = lock
+ return lock🤖 Grapple PR auto-fix • critical • Review this diff before applying
| return self.partition_locks.setdefault(partition, threading.Lock()) | ||
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (42 lines, 1 file) — review recommended)
Race condition in
_get_partition_lock: the check-then-set pattern for creating partition locks is not atomic, potentially creating two locks for the same partition under concurrent access.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -43,16 +43,33 @@ class OffsetTracker:
def __init__(self) -> None:
- self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
- self.outstanding: dict[Partition, set[int]] = defaultdict(set)
+ # Use plain dicts (not defaultdict) so that accesses outside a lock
+ # never silently create entries via __missing__, which is not thread-safe.
+ self.all_offsets: dict[Partition, set[int]] = {}
+ self.outstanding: dict[Partition, set[int]] = {}
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Guards creation of new partition entries in all four dicts above.
+ # Per-partition locks guard mutations to already-registered partitions.
+ self._meta_lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
- """Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ """
+ Get or create a lock for a partition.
+
+ Uses _meta_lock to make partition registration fully atomic: the lock
+ object and the corresponding set entries in all_offsets/outstanding are
+ created together so no caller can ever see a partition lock without its
+ backing data structures already in place.
+ """
+ # Fast path: partition already registered (no write, safe to read under GIL
+ # after the initial write was done while holding _meta_lock).
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ # Slow path: register the partition atomically.
+ with self._meta_lock:
+ # Re-check inside the lock to handle the race between two threads
+ # both reaching the slow path simultaneously.
+ if partition not in self.partition_locks:
+ self.partition_locks[partition] = threading.Lock()
+ self.all_offsets[partition] = set()
+ self.outstanding[partition] = set()
+ return self.partition_locks[partition]
def add_offset(self, partition: Partition, offset: int) -> None:
"""Record that we've started processing an offset."""
@@ -67,7 +84,12 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
"""
committable = {}
- for partition in list(self.all_offsets.keys()):
+ # Snapshot the key list under _meta_lock so we don't iterate a dict
+ # that another thread is concurrently expanding with new partitions.
+ with self._meta_lock:
+ partitions = list(self.partition_locks.keys())
+ for partition in partitions:
with self._get_partition_lock(partition):🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (25 lines, 1 file) — review recommended)
get_committable_offsetscallsmin(all_offsets)andmax(all_offsets)while holding per-partition locks, but the outer loop iteratesself.all_offsets.keys()without any lock, creating a race withadd_offsetadding new partitions.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -38,6 +38,12 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Global lock guards cross-partition operations (key enumeration, new partition
+ # registration). Per-partition locks guard per-partition data mutations.
+ # Use RLock in case future refactors call these methods re-entrantly.
+ self._global_lock = threading.RLock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: partition lock already exists, no global lock needed.
+ lock = self.partition_locks.get(partition)
+ if lock:
+ return lock
+ # Slow path: new partition — hold global lock to prevent two threads
+ # from racing on setdefault for the same partition.
+ with self._global_lock:
+ return self.partition_locks.setdefault(partition, threading.Lock())
def add_offset(self, partition: Partition, offset: int) -> None:
"""Record that we've started processing an offset."""
with self._get_partition_lock(partition):
self.all_offsets[partition].add(offset)
self.outstanding[partition].add(offset)
def complete_offset(self, partition: Partition, offset: int) -> None:
"""Mark an offset as completed."""
with self._get_partition_lock(partition):
self.outstanding[partition].discard(offset)
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
"""
committable = {}
- for partition in list(self.all_offsets.keys()):
+ # Hold the global lock only long enough to take a consistent snapshot of
+ # the partition keys. This prevents add_offset from inserting a brand-new
+ # partition (and its incomplete defaultdict entry) between our snapshot and
+ # the per-partition lock acquisition below.
+ with self._global_lock:
+ partitions = list(self.all_offsets.keys())
+
+ for partition in partitions:
with self._get_partition_lock(partition):
all_offsets = self.all_offsets[partition]
if not all_offsets:
continue
outstanding = self.outstanding[partition]
last_committed = self.last_committed.get(partition, -1)
min_offset = min(all_offsets)
max_offset = max(all_offsets)
start = max(last_committed + 1, min_offset)
highest_committable = last_committed
for offset in range(start, max_offset + 1):
if offset in all_offsets and offset not in outstanding:
highest_committable = offset
else:
break
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
# Remove all offsets <= committed offset
self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| worker = OrderedQueueWorker[T]( | ||
| worker_id=i, | ||
| work_queue=work_queue, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (26 lines, 1 file) — review recommended)
In the
_workermethod, whenprocess_funcraises an exception (line ~182), the exception is caught and logged, but the offset is still marked as completed viaoffset_tracker.complete_offset()in the finally block (line ~189). This means a failed message is silently skipped — its offset is committed as if it succeeded. The intent specification asks 'What happens when a worker processing a queue raises an unhandled exception — does the consumer halt, retry, or skip the message?' The answer here is 'skip', which could cause silent data loss.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -147,13 +147,30 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
try:
with sentry_sdk.start_transaction(
op="queue_worker.process",
name=f"monitors.{self.identifier}.worker_{self.worker_id}",
):
self.result_processor(self.identifier, work_item.result)
except queue.ShutDown:
break
except Exception:
+ # NOTE: We intentionally still complete the offset below in the
+ # finally block even on failure. The OffsetTracker requires
+ # contiguous offset completion — leaving this offset incomplete
+ # would permanently stall commits for all later offsets on this
+ # partition. There is no retry infrastructure in this consumer.
+ #
+ # Failures here mean the message is skipped. This is an explicit
+ # architectural decision: observability (Sentry + metric) is used
+ # to detect and alert on drops rather than blocking the pipeline.
logger.exception(
- "Unexpected error in queue worker", extra={"worker_id": self.worker_id}
+ "Unexpected error in queue worker — message will be skipped",
+ extra={
+ "worker_id": self.worker_id,
+ "partition": work_item.partition,
+ "offset": work_item.offset,
+ "identifier": self.identifier,
+ },
+ )
+ sentry_sdk.capture_exception()
+ metrics.incr(
+ "remote_subscriptions.queue_worker.message_dropped",
+ tags={
+ "identifier": self.identifier,
+ "reason": "processing_exception",
+ },
)
finally:
+ # Always complete the offset — see note in except block above.
self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
metrics.gauge(
"remote_subscriptions.queue_worker.queue_depth",🤖 Grapple PR auto-fix • major • Review this diff before applying
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (27 lines, 1 file) — review recommended)
The
result_processoris now instantiated in__init__(line 115:self.result_processor = self.result_processor_cls()) before the mode-specific initialization. Previously it was created after the mode selection block. This moves the instantiation earlier, which is fine, but nowself.result_processoris created unconditionally AND also passed toFixedQueuePool(line 131). If the factory'screate_with_partitionsis called multiple times (e.g., after rebalance), a new processing strategy is created but theresult_processorandqueue_poolare shared across invocations since they live on the factory. This could cause issues if the result processor has state.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -128,11 +128,7 @@ def __init__(
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
- if mode == "thread-queue-parallel":
- self.thread_queue_parallel = True
- self.queue_pool = FixedQueuePool(
- result_processor=self.result_processor,
- identifier=self.identifier,
- num_queues=max_workers or 20, # Number of parallel queues
- )
+ if mode == "thread-queue-parallel":
+ self.thread_queue_parallel = True
+ self.num_queues = max_workers or 20
metrics.incr(
@@ -151,7 +147,6 @@ def __init__(
if output_block_size is not None:
self.output_block_size = output_block_size
- self.result_processor = self.result_processor_cls()
-
@property
@abc.abstractmethod
def topic_for_codec(self) -> Topic:
@@ -180,8 +175,6 @@ def shutdown(self) -> None:
if self.parallel_executor:
self.parallel_executor.shutdown()
- if self.queue_pool:
- self.queue_pool.shutdown()
- self.queue_pool = None
def create_thread_queue_parallel_worker(
self, commit: Commit
) -> ProcessingStrategy[KafkaPayload]:
- assert self.queue_pool is not None
+ # Create a fresh FixedQueuePool per strategy lifecycle (i.e., per rebalance).
+ # Sharing a single pool across create_with_partitions calls is unsafe because
+ # the pool's worker threads are drained/closed when the previous strategy is
+ # joined, making the old pool unusable for the next assignment.
+ queue_pool = FixedQueuePool(
+ result_processor=self.result_processor,
+ identifier=self.identifier,
+ num_queues=self.num_queues,
+ )
def commit_offsets(offsets: dict[Partition, int]):
# We add + 1 here because the committed offset should represent the next offset to read from
commit_data = {partition: offset + 1 for partition, offset in offsets.items()}
commit(commit_data)
return SimpleQueueProcessingStrategy(
- queue_pool=self.queue_pool,
+ queue_pool=queue_pool,
decoder=partial(self.decode_payload, self.topic_for_codec),
grouping_fn=self.build_payload_grouping_key,
commit_function=commit_offsets,
)🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| self.offset_tracker.add_offset(work_item.partition, work_item.offset) | ||
| work_queue.put(work_item) | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (30 lines, 1 file) — review recommended)
In
SimpleQueueProcessingStrategy.submit(), when the queue is full, the code enters a blocking retry loop (while Truewithtime.sleep(0.01)) that could block the Arroyo processing strategy'ssubmitmethod indefinitely. Since Arroyo'sStreamProcessorcallspoll()and thensubmit()on the strategy, blocking insubmit()prevents the Kafka consumer from callingpoll(), which means heartbeats won't be sent. This will cause the consumer to be evicted from the consumer group aftersession.timeout.ms(typically 10-30 seconds), triggering a rebalance. The_check_queue_healthmethod logs warnings but doesn't actually resolve the back-pressure situation.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -204,10 +204,26 @@ class FixedQueuePool(Generic[T]):
def submit(self, group_key: str, work_item: WorkItem[T]) -> None:
"""
Submit a work item to the appropriate queue.
+
+ Uses put_nowait() to avoid blocking the Arroyo main thread. If the queue
+ is full (only possible if queues are created with a maxsize), the item is
+ dropped and its offset is immediately marked complete so commits can
+ proceed. This prevents Kafka heartbeat starvation and consumer group
+ rebalancing caused by blocking in submit().
"""
queue_index = self.get_queue_for_group(group_key)
work_queue = self.queues[queue_index]
- self.offset_tracker.add_offset(work_item.partition, work_item.offset)
- work_queue.put(work_item)
+ try:
+ # Register the offset before enqueuing so the tracker is always consistent.
+ self.offset_tracker.add_offset(work_item.partition, work_item.offset)
+ work_queue.put_nowait(work_item)
+ except queue.Full:
+ # Queue is full — drop the item rather than blocking the Kafka consumer
+ # thread. Mark the offset complete immediately so commits can advance.
+ # This is a last-resort safety valve; callers should monitor the
+ # "remote_subscriptions.queue_pool.queue_full_drops" metric and tune
+ # queue capacity or worker count before drops become frequent.
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ logger.warning(
+ "Queue %d full, dropping work item for group %r (partition=%s offset=%d)",
+ queue_index,
+ group_key,
+ work_item.partition,
+ work_item.offset,
+ )
+ metrics.incr(
+ "remote_subscriptions.queue_pool.queue_full_drops",
+ tags={"identifier": self.identifier, "queue_index": str(queue_index)},
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
| - No dynamic reassignment that could break ordering | ||
| - Tracks offset completion for safe commits | ||
| """ | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (28 lines, 1 file) — review recommended)
In
FixedQueuePool._worker(), whenself.process_result()raises an exception, the code catches it (line ~163), logs the error and captures to Sentry, but then continues to mark the offset as complete viaoffset_tracker.complete_offset(). This means failed messages are silently dropped — they won't be retried since the offset is committed. While this may be intentional for poison messages, it effectively means any transient error (database timeout, network blip) causes permanent message loss rather than retry.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -128,18 +128,35 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
def run(self) -> None:
"""Process items from the queue in order."""
while not self.shutdown:
try:
work_item = self.work_queue.get()
except queue.ShutDown:
break
+ processing_failed = False
try:
with sentry_sdk.start_transaction(
op="queue_worker.process",
name=f"monitors.{self.identifier}.worker_{self.worker_id}",
):
self.result_processor(self.identifier, work_item.result)
except queue.ShutDown:
break
except Exception:
+ processing_failed = True
logger.exception(
- "Unexpected error in queue worker", extra={"worker_id": self.worker_id}
+ "Unexpected error in queue worker — offset NOT committed, partition will stall",
+ extra={
+ "worker_id": self.worker_id,
+ "partition": work_item.partition,
+ "offset": work_item.offset,
+ },
)
+ metrics.incr(
+ "remote_subscriptions.queue_worker.processing_error",
+ tags={
+ "identifier": self.identifier,
+ },
+ )
finally:
- self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ # Only mark the offset complete on success. On failure we intentionally
+ # leave the offset in `outstanding` so that `get_committable_offsets()`
+ # cannot advance past this point, preventing silent message loss for
+ # transient errors (DB timeouts, network blips, etc.).
+ #
+ # NOTE: This will stall the partition at this offset until the consumer
+ # is restarted (triggering a re-delivery from the last committed offset).
+ # If poison-pill / permanently-failing messages are a concern, the caller's
+ # `result_processor` should handle its own dead-letter logic and only raise
+ # for genuinely retryable errors.
+ if not processing_failed:
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
metrics.gauge(
"remote_subscriptions.queue_worker.queue_depth",
self.work_queue.qsize(),
tags={
"identifier": self.identifier,
},
)🤖 Grapple PR auto-fix • major • Review this diff before applying
| metric_tags = {"identifier": self.identifier, "mode": self.mode} | ||
| self.result_processor = self.result_processor_cls() | ||
| if mode == "batched-parallel": | ||
| self.batched_parallel = True |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (11 lines, 1 file) — review recommended)
The
result_processorinstance (self.result_processor = self.result_processor_cls()) was moved from after mode-specific initialization to before it (line 117). This changes the initialization order for ALL modes, not justthread-queue-parallel. Ifresult_processor_cls()constructor has side effects or depends on state that was previously set up after the mode initialization, this could cause subtle regressions in existing modes (serial, parallel, batched-parallel).
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -115,7 +115,6 @@
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -128,6 +127,8 @@
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ if mode == "thread-queue-parallel":
+ # result_processor must be instantiated before FixedQueuePool, which takes it as an argument.
+ # We instantiate it here (before the shared assignment below) so that all other modes
+ # continue to receive it at the end of __init__, preserving the original initialization order.
+ self.result_processor = self.result_processor_cls()
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(
result_processor=self.result_processor,
@@ -151,6 +152,10 @@
if output_block_size is not None:
self.output_block_size = output_block_size
+ # For all modes except thread-queue-parallel, result_processor is instantiated here,
+ # after all mode-specific setup, preserving the original initialization order.
+ if not self.thread_queue_parallel:
+ self.result_processor = self.result_processor_cls()
+
@property
@abc.abstractmethod
def topic_for_codec(self) -> Topic:🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (50 lines, 1 file) — review recommended)
get_committable_offsetsis O(n) in the number of outstanding offsets per partition for each call tomin()andmax(), and this is called on every Kafka poll cycle.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -1,6 +1,7 @@
from __future__ import annotations
import logging
+import queue
import threading
import time
from collections import defaultdict
@@ -8,6 +9,8 @@ from collections.abc import Callable
from dataclasses import dataclass
from typing import Any, Generic, TypeVar
+from sortedcontainers import SortedList
+
import sentry_sdk
from arroyo.backends.kafka.consumer import KafkaPayload
from arroyo.processing.strategies import ProcessingStrategy
@@ -33,7 +36,9 @@ class OffsetTracker:
- Thread-safe for concurrent access with per-partition locks
"""
+ # Uses SortedList so min() is O(1) via index 0, and iteration is already
+ # sorted — eliminating O(k) min()/max() scans and O(gap) range walks.
def __init__(self) -> None:
- self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
+ self.all_offsets: dict[Partition, SortedList[int]] = defaultdict(SortedList)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
@@ -52,7 +57,7 @@ class OffsetTracker:
def add_offset(self, partition: Partition, offset: int) -> None:
"""Record that we've started processing an offset."""
with self._get_partition_lock(partition):
- self.all_offsets[partition].add(offset)
+ self.all_offsets[partition].add(offset) # O(log k) insert into SortedList
self.outstanding[partition].add(offset)
def complete_offset(self, partition: Partition, offset: int) -> None:
@@ -67,32 +72,46 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
+
+ Complexity: O(c * log k) per partition where c is the number of newly
+ committable offsets this cycle and k is the total tracked offsets.
+ Previously O(k) for min/max + O(gap) for the range scan.
"""
committable = {}
for partition in list(self.all_offsets.keys()):
with self._get_partition_lock(partition):
all_offsets = self.all_offsets[partition]
if not all_offsets:
continue
outstanding = self.outstanding[partition]
last_committed = self.last_committed.get(partition, -1)
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
-
+ # SortedList[0] is O(1) — no full iteration needed.
+ # We start from the smallest tracked offset that is beyond
+ # last_committed, so we never revisit already-committed offsets.
highest_committable = last_committed
- for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
- highest_committable = offset
- else:
+
+ # bisect_right returns the insertion point after last_committed,
+ # giving us the index of the first candidate offset — O(log k).
+ start_idx = all_offsets.bisect_right(last_committed)
+
+ # Walk the sorted offsets sequentially. We stop as soon as we
+ # find a gap (next offset != previous + 1) or an outstanding
+ # (not-yet-completed) offset. This is O(c) where c is the
+ # number of contiguous completed offsets, NOT O(max - min).
+ expected = last_committed + 1
+ for offset in all_offsets.islice(start_idx):
+ if offset != expected:
+ # Gap in offsets — cannot commit past here.
+ break
+ if offset in outstanding:
+ # This offset is still being processed — stop.
break
+ highest_committable = offset
+ expected += 1
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable
@@ -101,9 +120,11 @@ class OffsetTracker:
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
- # Remove all offsets <= committed offset
- self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+ # Remove all offsets <= committed offset from the SortedList.
+ # irange gives us only the values to remove — O(c * log k).
+ to_remove = list(self.all_offsets[partition].irange(None, offset))
+ for o in to_remove:
+ self.all_offsets[partition].remove(o)🤖 Grapple PR auto-fix • major • Review this diff before applying
| """ | ||
| queue_index = self.get_queue_for_group(group_key) | ||
| work_queue = self.queues[queue_index] | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (110 lines, 1 file) — review recommended)
Worker threads in
FixedQueuePoolthat raise unhandled exceptions silently die, causing messages queued to that worker to be permanently stuck (queue never drained), which will eventually block the main Kafka poll thread.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -108,6 +108,7 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
self.identifier = identifier
self.offset_tracker = offset_tracker
self.shutdown = False
+ self._alive = True
def run(self) -> None:
"""Process items from the queue in order."""
- while not self.shutdown:
- try:
- work_item = self.work_queue.get()
- except queue.ShutDown:
- break
-
+ work_item = None
+ while not self.shutdown:
try:
+ try:
+ work_item = self.work_queue.get()
+ except queue.ShutDown:
+ break
+
with sentry_sdk.start_transaction(
op="queue_worker.process",
name=f"monitors.{self.identifier}.worker_{self.worker_id}",
):
self.result_processor(self.identifier, work_item.result)
- except queue.ShutDown:
- break
except Exception:
logger.exception(
"Unexpected error in queue worker", extra={"worker_id": self.worker_id}
)
finally:
- self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ if work_item is not None:
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ work_item.done_event.set() if hasattr(work_item, "done_event") else None
+ work_item = None
metrics.gauge(
"remote_subscriptions.queue_worker.queue_depth",
self.work_queue.qsize(),
tags={
"identifier": self.identifier,
},
)
+ self._alive = False
+ # Drain remaining items so callers blocked on queue.put() are never stuck.
+ # complete_offset is called for each drained item so shutdown() doesn't hang.
+ self._drain_on_exit()
+
+ def _drain_on_exit(self) -> None:
+ """
+ Drain all remaining items from the queue when the worker exits unexpectedly.
+ This prevents the main Kafka poll thread from blocking on queue.put() and
+ ensures offset_tracker.complete_offset() is called for every queued item
+ so that shutdown() / queue.join() do not hang.
+ """
+ while True:
+ try:
+ work_item = self.work_queue.get_nowait()
+ except (queue.Empty, queue.ShutDown):
+ break
+ except Exception:
+ logger.exception(
+ "Unexpected error draining queue on worker exit",
+ extra={"worker_id": self.worker_id},
+ )
+ break
+ try:
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ except Exception:
+ logger.exception(
+ "Error completing offset during drain",
+ extra={"worker_id": self.worker_id},
+ )
class FixedQueuePool(Generic[T]):
@@ -180,10 +221,38 @@ class FixedQueuePool(Generic[T]):
def submit(self, group_key: str, work_item: WorkItem[T]) -> None:
"""
Submit a work item to the appropriate queue.
+
+ If the target worker has died, attempt to restart it so the queue does
+ not fill up and block the Kafka poll thread indefinitely.
"""
queue_index = self.get_queue_for_group(group_key)
work_queue = self.queues[queue_index]
+ worker = self.workers[queue_index]
+
+ # Detect dead workers before submitting to avoid blocking indefinitely.
+ if not worker._alive and not worker.is_alive():
+ logger.error(
+ "Queue worker %d has died; restarting to prevent message loss",
+ queue_index,
+ extra={"identifier": self.identifier},
+ )
+ metrics.incr(
+ "remote_subscriptions.queue_pool.worker_restart",
+ tags={"identifier": self.identifier, "worker_id": str(queue_index)},
+ )
+ new_worker = OrderedQueueWorker[T](
+ worker_id=queue_index,
+ work_queue=work_queue,
+ result_processor=self.result_processor,
+ identifier=self.identifier,
+ offset_tracker=self.offset_tracker,
+ )
+ new_worker.start()
+ self.workers[queue_index] = new_worker
+ worker = new_worker
self.offset_tracker.add_offset(work_item.partition, work_item.offset)
- work_queue.put(work_item)
+ # Use a timeout so the main thread never blocks permanently if the
+ # newly restarted worker also fails before accepting the item.
+ try:
+ work_queue.put(work_item, timeout=30.0)
+ except queue.Full:
+ # Mark the offset complete so offset tracking doesn't stall, then
+ # log loudly — this is a last-resort safety valve.
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ logger.error(
+ "Queue %d is full after 30s timeout; dropping work item to prevent deadlock. "
+ "partition=%s offset=%d",
+ queue_index,
+ work_item.partition,
+ work_item.offset,
+ extra={"identifier": self.identifier},
+ )
+ metrics.incr(
+ "remote_subscriptions.queue_pool.item_dropped",
+ tags={"identifier": self.identifier, "worker_id": str(queue_index)},
+ )
def get_stats(self) -> dict[str, Any]:
"""Get statistics about queue depths."""
@@ -195,6 +264,17 @@ class FixedQueuePool(Generic[T]):
+ def all_workers_alive(self) -> bool:
+ """Returns True if all worker threads are currently alive."""
+ return all(w.is_alive() for w in self.workers)
+
def wait_until_empty(self, timeout: float = 5.0) -> bool:
"""Wait until all queues are empty. Returns True if successful, False if timeout."""
start_time = time.time()
while time.time() - start_time < timeout:
if self.get_stats()["total_items"] == 0:
return True
time.sleep(0.01)
return False
def shutdown(self) -> None:
"""Gracefully shutdown all workers."""
for worker in self.workers:
worker.shutdown = True
for q in self.queues:
try:
q.shutdown(immediate=False)
except Exception:
logger.exception("Error shutting down queue")
for worker in self.workers:
- worker.join(timeout=5.0)
+ worker.join(timeout=10.0)
+ if worker.is_alive():
+ logger.warning(
+ "Worker %d did not exit within timeout during shutdown",
+ worker.worker_id,
+ extra={"identifier": self.identifier},
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a queue-based parallelism management system for uptime/remote subscription consumers, introducing a QueueConsumer abstraction that allows Kafka consumers to dispatch work to parallel queues while controlling concurrency. The implementation wraps existing result consumer logic to enable parallel processing of uptime check results.
Changes: 7 files (+1276 / -6 lines)
- 🟢 4 files added:
src/sentry/remote_subscriptions/consumers/queue_consumer.py,tests/sentry/remote_subscriptions/__init__.py,tests/sentry/remote_subscriptions/consumers/__init__.py,tests/sentry/remote_subscriptions/consumers/test_queue_consumer.py - 🟡 3 files modified:
src/sentry/consumers/__init__.py,src/sentry/remote_subscriptions/consumers/result_consumer.py,tests/sentry/uptime/consumers/test_results_consumer.py
Resolves: #9 (Optimize spans buffer insertion with eviction during insert)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 __init__.py +2/-2"]
pr_0 --> f1
f2["🟢 queue_consumer.py +345/-0"]
pr_0 --> f2
f3["🟡 result_consumer.py +41/-3"]
pr_0 --> f3
f4["🟢 __init__.py +0/-0"]
pr_0 --> f4
f5["🟢 __init__.py +0/-0"]
pr_0 --> f5
f6["🟢 test_queue_consumer.py +421/-0"]
pr_0 --> f6
f7["🟡 test_results_consumer.py +467/-1"]
pr_0 --> f7
f2 -.- f3
f5 -.- f6
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
5 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 5 |
| 🟠 Major | 14 |
| 🟡 Minor | 9 |
| 🔵 Info | 4 |
Review details
- Reviewed at: 2026-04-08T00:49:25.659Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 191.5s
- Low-confidence findings suppressed: 7
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| return lock | ||
| return self.partition_locks.setdefault(partition, threading.Lock()) | ||
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🔴 CRITICAL — Thread Safety - Race Condition (confidence: 100%)
The _get_partition_lock method has a TOCTOU race condition. Two threads could simultaneously check self.partition_locks.get(partition) and both get None, then both call setdefault. While dict.setdefault is atomic in CPython due to the GIL, the partition_locks dict itself is a regular dict being accessed from multiple threads without synchronization. The initial get call and the subsequent setdefault are two separate operations, and the partition_locks dictionary is shared mutable state accessed from worker threads (via complete_offset) and the main thread (via add_offset and get_committable_offsets).
Evidence:
- The PR description explicitly lists 'Exception propagation from worker threads: unhandled exceptions in threads that don’t propagate to the main thread can cause silent failures' as a risk area
- Worker threads are daemon threads or background threads consuming from internal queues; if one dies, items put into that queue are never consumed
- The OffsetTracker will have outstanding offsets that never get completed, so get_committable_offsets() will never advance past the dead worker's last offset
- No heartbeat, health check, or thread resurrection mechanism is visible in the truncated code
partition_locksis a plaindict, not a thread-safe structure- Two threads can concurrently call
_get_partition_lockfor a new partition - Python 3.13 introduces experimental no-GIL mode (PEP 703); Sentry may eventually adopt it
setdefaultatomicity is a CPython implementation detail, not a language guarantee- Worker threads call
complete_offsetwhich calls_get_partition_lock - Main thread calls
add_offsetandget_committable_offsetswhich also call_get_partition_lock - The
partition_locksdict is a plain dict with no synchronization protecting its own access - While CPython's GIL makes dict.setdefault effectively atomic, this is an implementation detail, not a language guarantee
- Intent: 'On shutdown/close, the QueueConsumer should drain in-flight messages from all queues before stopping'
- Edge case: 'Shutdown signal received while queues have a large backlog — verify drain timeout or maximum wait behavior'
- The
timemodule is imported but no timeout constants or drain timeout logic is visible in the non-truncated portion - queue_consumer.py ~line 170-180:
if self._shutdown.is_set() and self._queue.empty(): break - queue_consumer.py ~line 200-210: shutdown() sets _shutdown then puts None sentinel
- Between is_set() and empty(), the main thread could still be putting items via submit()
Agent: logic
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🔴 CRITICAL — Offset Commit Correctness (confidence: 100%)
The get_committable_offsets method iterates from min_offset to max_offset looking for the highest contiguous completed offset. However, after calling mark_committed, the committed offsets are removed from all_offsets via cleanup. This means after the first batch is committed, the next call to get_committable_offsets starts from the new min(all_offsets). If offset 100 was committed and removed, and offset 101 is still outstanding but 102 is complete, the method would start scanning from 101, find it outstanding, and correctly return nothing. This logic appears correct on closer inspection, BUT the mark_committed method's cleanup of all_offsets removes offsets <= committed value. If offsets are not perfectly contiguous (e.g., filtered messages skip offsets), the scan from min_offset to max_offset will encounter gaps and treat missing offsets as completed (since they're not in outstanding), potentially committing past unprocessed messages.
Evidence:
- The scan loop
for offset in range(min_offset, max_offset + 1)checksif offset in outstandingbut does NOT checkif offset in all_offsets - If a Kafka offset exists that was never added to
all_offsets(e.g., filtered messages, rebalancing), the scan would skip over it as if it were completed - FilteredPayload messages may not go through
add_offsetbut their offsets still need to be tracked for correct commit behavior
Agent: logic
|
|
||
| min_offset = min(all_offsets) | ||
| max_offset = max(all_offsets) | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — Memory leaks (confidence: 100%)
OffsetTracker.all_offsets grows unboundedly — completed offsets are never removed from all_offsets, only from outstanding. Over time this set accumulates every offset ever seen for a partition, causing unbounded memory growth in long-running consumers.
Evidence:
add_offsetadds toself.all_offsets[partition]but neithercomplete_offsetnormark_committedremove entries fromall_offsetsget_committable_offsetscallsmin(all_offsets)andmax(all_offsets)on the full historical set, not just the window since last commit- In
mark_committed, onlylast_committedis updated;all_offsetsis untouched - A consumer processing millions of messages per day will accumulate millions of integers in this set per partition
- add_offset() adds to both all_offsets and outstanding
- complete_offset() only calls outstanding[partition].discard(offset) — all_offsets is never pruned
- mark_committed() (referenced in tests) presumably removes from all_offsets, but get_committable_offsets() iterates all_offsets which will grow indefinitely if mark_committed is not called correctly or lags behind
- In a high-throughput uptime consumer processing millions of messages per day, this set will consume gigabytes of memory
- No
revoke_partitionorclear_partitionmethod exists on OffsetTracker - The
SimpleQueueProcessingStrategyhas no code path that clears partition state on rebalance last_committedpersists across rebalances, so if partition 0 was at offset 500 before revoke, and comes back at offset 100 after reassignment,get_committable_offsetswill never return a value for it since all new offsets will be < last_committed
Agent: performance
| """ | ||
| committable = {} | ||
| for partition in list(self.all_offsets.keys()): | ||
| with self._get_partition_lock(partition): |
There was a problem hiding this comment.
🔴 CRITICAL — Concurrency issues (confidence: 100%)
_get_partition_lock() has a race condition: checking existence and then calling setdefault are not atomic relative to concurrent reads of partition_locks itself, and partition_locks is a plain dict with no lock protecting it. Two threads could simultaneously pass the if lock: return lock check, both see None, and both attempt setdefault — while a third thread could be iterating partition_locks in get_committable_offsets().
Evidence:
self.partition_locksis a plaindictwith no protecting lockget_committable_offsets()callslist(self.all_offsets.keys())and thenself._get_partition_lock(partition)— dict is being read while worker threads could be writing new partition entries- Python dict is not thread-safe for concurrent structural modifications (adding new keys while iterating)
- The per-partition lock creation itself is not atomic:
lock = self.partition_locks.get(partition)followed bysetdefaultis a TOCTOU pattern
Agent: performance
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🔴 CRITICAL — Cross-file Bug / Incorrect Attribute Access (confidence: 100%)
Moving self.result_processor = self.result_processor_cls() before the mode checks means it's now instantiated for ALL modes, but more critically, the FixedQueuePool constructor receives result_processor=self.result_processor which is a ResultProcessor instance. However, looking at queue_consumer.py, the FixedQueuePool.__init__ stores this as self.result_processor and passes it to QueueWorker which calls self.result_processor(self.identifier, item). This means ResultProcessor.__call__ must accept (identifier, item) arguments. If result_processor_cls produces an object whose __call__ signature doesn't match (identifier: str, item: T), this will fail at runtime. The FixedQueuePool type hint says result_processor: Callable[[str, T], Any] but it's receiving a class instance - the type compatibility depends entirely on the __call__ signature of the concrete ResultProcessor subclass.
Evidence:
- queue_consumer.py line ~160:
self.result_processor(self.identifier, item)calls with (str, T) - result_consumer.py line 115:
self.result_processor = self.result_processor_cls()creates the instance - result_consumer.py line 131:
result_processor=self.result_processorpasses it to FixedQueuePool - The concrete UptimeResultProcessor's call signature must match (str, T) for this to work correctly
Agent: logic
| click.Option( | ||
| ["--mode", "mode"], | ||
| type=click.Choice(["serial", "parallel", "batched-parallel"]), | ||
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), |
There was a problem hiding this comment.
🟡 MINOR — Intent Mismatch / PR-Issue Linkage (confidence: 82%)
The linked issue #9 is titled 'Optimize spans buffer insertion with eviction during insert' which is about spans buffer optimization, not about queue-based parallelism for uptime consumers. This PR does not appear to address issue #9's actual content.
Evidence:
- Issue Optimize spans buffer insertion with eviction during insert #9: 'Optimize spans buffer insertion with eviction during insert'
- PR implements: queue-based parallelism for uptime/remote subscription consumers
- These are completely different features
Agent: logic
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
| self.mode = mode | |||
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 93%)
The result_processor is instantiated early in init before mode validation, but is only used conditionally based on mode. The instantiation was moved before the mode handling logic, which could cause issues if result_processor_cls() has side effects or expensive initialization that only applies to certain modes.
Evidence:
- Line 117: self.result_processor = self.result_processor_cls() is instantiated unconditionally
- Lines 119-128: The result_processor is only used when mode == 'thread-queue-parallel'
- Original code (line 150 in diff context) showed this was instantiated after mode setup
- If result_processor_cls has expensive initialization or mode-specific setup, this could be wasteful or incorrect
Agent: style
| from sentry.utils import metrics | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🔵 INFO — Code Organization (confidence: 85%)
The module lacks a docstring explaining the purpose of the QueueConsumer abstraction and its role in the system. This is important for maintainability given the complexity of offset tracking and thread synchronization.
Evidence:
- No module-level docstring explaining the queue-consumer pattern, why it exists, or how it relates to the broader result consumer system
- The code introduces complex thread-safety mechanisms (OffsetTracker with locks, WorkItem queues, worker threads) that need documentation
- New developers won't understand the design intent without a high-level explanation
Agent: style
|
|
||
| with mock.patch.object(type(factory.result_processor), "__call__") as mock_processor_call: | ||
| subscription_2 = self.create_uptime_subscription( | ||
| subscription_id=uuid.uuid4().hex, interval_seconds=300, url="http://santry.io" |
There was a problem hiding this comment.
🔵 INFO — Test Quality / Integration Test Uses Real HTTP URLs Without HTTPS (confidence: 82%)
Integration tests use http:// URLs (flagged by static scanner). While these are test fixtures and not real connections, using HTTP URLs in uptime monitoring tests could mask issues where the production code should enforce HTTPS-only monitoring. The test should use HTTPS URLs to match production behavior and avoid testing against a weaker security posture.
Evidence:
- Static scanner flagged lines 1701 and 1794 for insecure HTTP URLs
- Uptime monitoring is specifically designed to check external URLs — test fixtures should mirror production URL patterns
Agent: security
| except queue.ShutDown: | ||
| break | ||
| except Exception: | ||
| logger.exception( |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 75%)
The get_committable_offsets method logic lacks inline documentation explaining the contiguous offset algorithm. The method is critical for preventing message loss but the logic (lines 131-144 are truncated) is not clearly explained.
Evidence:
- The docstring only says 'For each partition, finds the highest contiguous offset that has been processed' but doesn't explain WHY contiguity matters (to prevent gaps in offset commits)
- The algorithm behavior when there are gaps (e.g., offsets 100, 101, 103 where 102 is outstanding) is not documented
- This is a core safety mechanism for Kafka offset management and needs clearer intent documentation
Agent: style
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
Moving
self.result_processor = self.result_processor_cls()before the mode checks means it's now instantiated for ALL modes, but more critically, theFixedQueuePoolconstructor receivesresult_processor=self.result_processorwhich is aResultProcessorinstance. However, looking atqueue_consumer.py, theFixedQueuePool.__init__stores this asself.result_processorand passes it toQueueWorkerwhich callsself.result_processor(self.identifier, item). This meansResultProcessor.__call__must accept(identifier, item)arguments. Ifresult_processor_clsproduces an object whose__call__signature doesn't match(identifier: str, item: T), this will fail at runtime. TheFixedQueuePooltype hint saysresult_processor: Callable[[str, T], Any]but it's receiving a class instance - the type compatibility depends entirely on the__call__signature of the concreteResultProcessorsubclass.
| ) -> None: | |
| # Explicitly wrap result_processor as a Callable[[str, T], Any] to satisfy | |
| # FixedQueuePool's type contract and QueueWorker's call site: | |
| # self.result_processor(self.identifier, item) | |
| # ResultProcessor.__call__ must accept (identifier: str, item: T). | |
| # Using a lambda makes this contract explicit and catches signature mismatches early. | |
| _processor_callable: Callable[[str, Any], Any] = lambda identifier, item: self.result_processor(identifier, item) | |
| result_processor=_processor_callable, |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| def __init__( | ||
| self, | ||
| queue_pool: FixedQueuePool[T], | ||
| decoder: Callable[[KafkaPayload | FilteredPayload], T | None], |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (4 lines, 1 file))
The
submitmethod inSimpleQueueProcessingStrategyonly callsself.offset_tracker.add_offset()and enqueues work when the message payload is not aFilteredPayload. However,flushcallscommit_offsetswhich only commits offsets tracked by the offset_tracker. SinceFilteredPayloadmessages are never added to the tracker, their offsets are never committed. If a stream has a mix of filtered and non-filtered messages, the committed offset will stall at the last non-filtered message offset, causing filtered-message offsets to be redelivered on restart.
| decoder: Callable[[KafkaPayload | FilteredPayload], T | None], | |
| # FilteredPayload and messages that decode to None must still be | |
| # tracked so the contiguous-offset chain is not broken. | |
| # add_offset + immediate complete_offset = "this offset is done, | |
| # advance past it" without enqueuing any real work. |
🤖 Grapple PR auto-fix • major • confidence: 100%
| if num_processes is None: | ||
| num_processes = multiprocessing.cpu_count() | ||
| self.multiprocessing_pool = MultiprocessingPool(num_processes) | ||
| if mode == "thread-queue-parallel": |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
The default number of queues when max_workers is not specified is hardcoded to 20 (
max_workers or 20). This is a high default that could create 20 worker threads. Themax_workersCLI option hasdefault=None, so if a user specifies--mode thread-queue-parallelwithout--max-workers, they silently get 20 threads. This should either require explicit configuration or use a more conservative default.
| if mode == "thread-queue-parallel": | |
| if max_workers is None: | |
| num_queues = multiprocessing.cpu_count() | |
| metric_tags["workers"] = "default" | |
| else: | |
| num_queues = max_workers | |
| metric_tags["workers"] = str(max_workers) | |
| num_queues=num_queues, |
🤖 Grapple PR auto-fix • minor • confidence: 97%
| click.Option( | ||
| ["--mode", "mode"], | ||
| type=click.Choice(["serial", "parallel", "batched-parallel"]), | ||
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (7 lines, 1 file))
The linked issue #9 is titled 'Optimize spans buffer insertion with eviction during insert' which is about spans buffer optimization, not about queue-based parallelism for uptime consumers. This PR does not appear to address issue #9's actual content.
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), | |
| """Return a list of uptime-results consumer options. | |
| These options control parallelism for the uptime/remote-subscription | |
| consumer, including the queue-based 'thread-queue-parallel' mode which | |
| dispatches work to parallel queues via QueueConsumer. | |
| """ |
🤖 Grapple PR auto-fix • minor • confidence: 82%
| return lock | ||
| return self.partition_locks.setdefault(partition, threading.Lock()) | ||
|
|
||
| def add_offset(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (23 lines, 1 file) — review recommended)
The
_get_partition_lockmethod has a TOCTOU race condition. Two threads could simultaneously checkself.partition_locks.get(partition)and both getNone, then both callsetdefault. Whiledict.setdefaultis atomic in CPython due to the GIL, thepartition_locksdict itself is a regular dict being accessed from multiple threads without synchronization. The initialgetcall and the subsequentsetdefaultare two separate operations, and thepartition_locksdictionary is shared mutable state accessed from worker threads (viacomplete_offset) and the main thread (viaadd_offsetandget_committable_offsets).
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -43,10 +43,17 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Protects partition_locks dict itself during check-and-create.
+ # Per-partition locks are still used for per-partition data access.
+ # This is necessary for correctness under no-GIL Python (PEP 703)
+ # and for clarity: dict.setdefault atomicity is a CPython detail,
+ # not a language guarantee.
+ self._registry_lock: threading.Lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: lock already exists (no registry lock needed for read
+ # once the partition is registered, since we never delete entries).
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ # Slow path: partition not yet registered — acquire registry lock
+ # to eliminate the TOCTOU race between get() and insertion.
+ with self._registry_lock:
+ # Re-check inside the lock: another thread may have inserted
+ # the lock between our get() above and acquiring _registry_lock.
+ if partition not in self.partition_locks:
+ self.partition_locks[partition] = threading.Lock()
+ return self.partition_locks[partition]🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (29 lines, 1 file) — review recommended)
The
get_committable_offsetsmethod iterates frommin_offsettomax_offsetlooking for the highest contiguous completed offset. However, after callingmark_committed, the committed offsets are removed fromall_offsetsvia cleanup. This means after the first batch is committed, the next call toget_committable_offsetsstarts from the newmin(all_offsets). If offset 100 was committed and removed, and offset 101 is still outstanding but 102 is complete, the method would start scanning from 101, find it outstanding, and correctly return nothing. This logic appears correct on closer inspection, BUT themark_committedmethod's cleanup ofall_offsetsremoves offsets <= committed value. If offsets are not perfectly contiguous (e.g., filtered messages skip offsets), the scan frommin_offsettomax_offsetwill encounter gaps and treat missing offsets as completed (since they're not inoutstanding), potentially committing past unprocessed messages.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -67,6 +67,15 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
+
+ Note: Every Kafka offset that this consumer receives must be registered via
+ add_offset() (and subsequently complete_offset()) — including filtered/skipped
+ messages — so that the contiguity scan below has a complete picture. If an
+ offset is never added to all_offsets, a gap between last_committed and
+ min(all_offsets) will block commits for that partition, which is the safe
+ behavior. See submit() in SimpleQueueProcessingStrategy for how FilteredPayload
+ and decoder-None results are handled.
"""
committable = {}
for partition in list(self.all_offsets.keys()):
@@ -83,8 +92,21 @@ class OffsetTracker:
min_offset = min(all_offsets)
max_offset = max(all_offsets)
+ # If there is a gap between the last committed offset and the lowest
+ # tracked offset, it means one or more Kafka offsets were never
+ # registered with add_offset(). We cannot safely commit past
+ # last_committed because we have no record of those intermediate
+ # offsets being processed.
+ #
+ # Example: last_committed=100, all_offsets={102} means offset 101
+ # was never tracked. We must not advance the commit position until
+ # offset 101 is accounted for (or we know it cannot appear).
+ if min_offset > last_committed + 1:
+ # There is an untracked gap — cannot commit anything new.
+ continue
+
start = max(last_committed + 1, min_offset)
highest_committable = last_committed
for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
+ if offset in all_offsets:
+ if offset in outstanding:
+ # This offset is still being processed — stop here.
+ break
highest_committable = offset
else:
+ # This offset is not in all_offsets — it is a gap between
+ # two tracked offsets (e.g., offset was never registered).
+ # This is unsafe to skip; stop scanning.
break
if highest_committable > last_committed:🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| min_offset = min(all_offsets) | ||
| max_offset = max(all_offsets) | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (32 lines, 1 file) — review recommended)
OffsetTracker.all_offsets grows unboundedly — completed offsets are never removed from all_offsets, only from outstanding. Over time this set accumulates every offset ever seen for a partition, causing unbounded memory growth in long-running consumers.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -93,6 +93,18 @@ class OffsetTracker:
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
# Remove all offsets <= committed offset to prevent unbounded growth
+ # all_offsets now only holds the in-flight window, not the full history
self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+
+ def revoke_partition(self, partition: Partition) -> None:
+ """
+ Clear all state for a partition on rebalance.
+
+ Must be called when a partition is revoked to prevent stale last_committed
+ from suppressing valid offsets after reassignment at a lower offset.
+ """
+ with self._get_partition_lock(partition):
+ self.all_offsets.pop(partition, None)
+ self.outstanding.pop(partition, None)
+ self.last_committed.pop(partition, None)
+ # Note: keep the lock entry — it's cheap and avoids a TOCTOU race
+ # if a worker thread completes an offset for this partition concurrently.
@@ -240,6 +252,20 @@ class SimpleQueueProcessingStrategy(ProcessingStrategy[KafkaPayload], Generic[T
self.commit_thread = threading.Thread(target=self._commit_loop, daemon=True)
self.commit_thread.start()
+ def revoke_partitions(self, partitions: list[Partition]) -> None:
+ """
+ Called by Arroyo on partition rebalance.
+
+ Clears OffsetTracker state for revoked partitions so that stale
+ last_committed values don't suppress offsets after reassignment.
+ Without this, a partition reassigned at a lower offset (e.g. after
+ consumer group rebalance) would never produce a committable offset
+ because all new offsets would be < last_committed.
+ """
+ for partition in partitions:
+ logger.info(
+ "Revoking partition state from offset tracker",
+ extra={"partition": partition},
+ )
+ self.queue_pool.offset_tracker.revoke_partition(partition)
+
def _commit_loop(self) -> None:
while not self.shutdown_event.is_set():
try:🤖 Grapple PR auto-fix • critical • Review this diff before applying
| """ | ||
| committable = {} | ||
| for partition in list(self.all_offsets.keys()): | ||
| with self._get_partition_lock(partition): |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (21 lines, 1 file) — review recommended)
_get_partition_lock() has a race condition: checking existence and then calling setdefault are not atomic relative to concurrent reads of partition_locks itself, and partition_locks is a plain dict with no lock protecting it. Two threads could simultaneously pass the
if lock: return lockcheck, both see None, and both attempt setdefault — while a third thread could be iterating partition_locks in get_committable_offsets().
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -43,10 +43,16 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Protects structural modifications to partition_locks (i.e. adding new
+ # partition entries). Per-partition locks are still used for the actual
+ # offset data so contention on this registry lock is minimal — it is
+ # only held long enough to do a dict lookup + possible setdefault.
+ self._registry_lock: threading.Lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: partition already registered. Read under registry lock so
+ # we never observe a partially-constructed partition_locks dict.
+ with self._registry_lock:
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ # Slow path: first time we see this partition — create and register
+ # atomically under the same registry lock so no other thread can
+ # race us to setdefault and end up with a different lock object.
+ new_lock = threading.Lock()
+ self.partition_locks[partition] = new_lock
+ return new_lock🤖 Grapple PR auto-fix • critical • Review this diff before applying
| ) -> None: | ||
| self.mode = mode | ||
| metric_tags = {"identifier": self.identifier, "mode": self.mode} | ||
| self.result_processor = self.result_processor_cls() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Small fix (4 lines, 1 file))
The
self.result_processor = self.result_processor_cls()was moved from the end of__init__to before the mode-specific initialization (line 117). This means the result_processor is now always instantiated regardless of mode, which changes behavior from the original code. More critically, the FixedQueuePool at line 130 receivesself.result_processorwhich is now initialized. However, ifresult_processor_cls()has side effects or is expensive, this is now always invoked even for modes that don't need it early. The original code initialized it at the end of init.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -115,7 +115,6 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -128,6 +127,8 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ # Initialize result_processor before thread-queue-parallel block (which needs it),
+ # and after other mode-specific setup to avoid side effects for modes that don't need it early.
+ self.result_processor = self.result_processor_cls()
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(🤖 Grapple PR auto-fix • major • Review this diff before applying
| self, | ||
| worker_id: int, | ||
| work_queue: queue.Queue[WorkItem[T]], | ||
| result_processor: Callable[[str, T], None], |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (38 lines, 1 file) — review recommended)
get_committable_offsets() acquires each partition lock individually in sequence, which means the set of partitions seen between the outer list() call and individual lock acquisitions can be inconsistent. More critically, the contiguous-offset scan is O(n) per partition per call to get_committable_offsets(), and this method is called from the main thread's submit() or flush() path while worker threads are concurrently modifying outstanding. Holding the lock for a linear scan blocks workers from completing offsets.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -68,25 +68,28 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
+ Uses a sorted scan over only the tracked offsets (O(k log k) where k is the
+ number of tracked offsets) rather than iterating over the full offset range
+ (O(range_size)), so sparse partitions or large gaps don't stall worker threads.
"""
committable = {}
for partition in list(self.all_offsets.keys()):
with self._get_partition_lock(partition):
- all_offsets = self.all_offsets[partition]
- if not all_offsets:
+ # Take a consistent snapshot under the lock so the scan
+ # sees a stable view even as workers call complete_offset().
+ all_offsets_snapshot = set(self.all_offsets[partition])
+ if not all_offsets_snapshot:
continue
- outstanding = self.outstanding[partition]
+ outstanding_snapshot = set(self.outstanding[partition])
last_committed = self.last_committed.get(partition, -1)
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
-
+ # Sort only the offsets we actually track — O(k log k) where k is
+ # the number of outstanding/tracked offsets, not the range size.
+ # This prevents long lock holds on partitions with sparse offsets.
highest_committable = last_committed
- for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
- highest_committable = offset
- else:
+ prev = None
+ for offset in sorted(all_offsets_snapshot):
+ if offset <= last_committed:
+ # Already committed; skip but don't break the contiguity check.
+ prev = offset
+ continue
+ # Check for a gap: if this offset is not contiguous with the
+ # previous committable offset (or last_committed), stop.
+ expected = (prev if prev is not None else last_committed) + 1
+ if offset != expected:
+ break
+ if offset in outstanding_snapshot:
+ # This offset is not yet done; contiguous run ends here.
break
+ highest_committable = offset
+ prev = offset
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable🤖 Grapple PR auto-fix • major • Review this diff before applying
| self.thread_queue_parallel = True | ||
| self.queue_pool = FixedQueuePool( | ||
| result_processor=self.result_processor, | ||
| identifier=self.identifier, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (25 lines, 1 file) — review recommended)
result_processor is instantiated before the mode branching logic, meaning it is created even for modes that don't use it (serial mode creates a result_processor that is then also used by thread-queue-parallel). More critically, result_processor_cls() is called once and the single instance is shared across all worker threads in FixedQueuePool without any documented thread-safety guarantees.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -115,7 +115,6 @@
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -128,9 +127,13 @@
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ if mode == "thread-queue-parallel":
+ self.thread_queue_parallel = True
+ # Pass the class (factory) rather than a single instance so that
+ # FixedQueuePool can instantiate one result_processor per worker
+ # thread, avoiding data races on any stateful ResultProcessor
+ # subclasses (DB connections, caches, instance variables).
+ self.queue_pool = FixedQueuePool(
+ result_processor_cls=self.result_processor_cls,
+ identifier=self.identifier,
+ num_queues=max_workers or 20, # Number of parallel queues
+ )
- if mode == "thread-queue-parallel":
- self.thread_queue_parallel = True
- self.queue_pool = FixedQueuePool(
- result_processor=self.result_processor,
- identifier=self.identifier,
- num_queues=max_workers or 20, # Number of parallel queues
- )
metrics.incr(
"remote_subscriptions.result_consumer.start",
@@ -151,6 +154,11 @@
if output_block_size is not None:
self.output_block_size = output_block_size
+ # Instantiate result_processor for non-thread-queue-parallel modes only,
+ # at the end of __init__ (matching original initialization order) so that
+ # subclasses which set up state before calling super().__init__() are unaffected.
+ if mode != "thread-queue-parallel":
+ self.result_processor = self.result_processor_cls()
+
@property
@abc.abstractmethod
def topic_for_codec(self) -> Topic:🤖 Grapple PR auto-fix • major • Review this diff before applying
| def __init__( | ||
| self, | ||
| queue_pool: FixedQueuePool[T], | ||
| decoder: Callable[[KafkaPayload | FilteredPayload], T | None], |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (34 lines, 1 file) — review recommended)
shutdown() joins worker threads with a fixed timeout but does not verify all queues are actually drained. If queues contain more items than workers can process within the timeout, messages are abandoned without any warning, violating the 'drain in-flight messages before stopping' requirement.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -231,12 +231,34 @@ class FixedQueuePool(Generic[T]):
def shutdown(self) -> None:
"""Gracefully shutdown all workers."""
+ # Step 1: Drain all queues before stopping workers.
+ # wait_until_empty() polls qsize(); give it a generous window so that
+ # a large-but-finite backlog doesn't cause silent message loss.
+ DRAIN_TIMEOUT = 30.0
+ drained = self.wait_until_empty(timeout=DRAIN_TIMEOUT)
+ if not drained:
+ remaining = self.get_stats()["total_items"]
+ logger.warning(
+ "Queue pool shutdown timed out before draining all items; "
+ "%d item(s) will be abandoned",
+ remaining,
+ extra={"identifier": self.identifier},
+ )
+ metrics.gauge(
+ "remote_subscriptions.queue_pool.abandoned_on_shutdown",
+ remaining,
+ tags={"identifier": self.identifier},
+ )
+
+ # Step 2: Signal workers to stop accepting new work.
for worker in self.workers:
worker.shutdown = True
+ # Step 3: Shut down the underlying queue objects so blocked get()
+ # calls in workers unblock immediately (queue.ShutDown is raised).
for q in self.queues:
try:
q.shutdown(immediate=False)
except Exception:
logger.exception("Error shutting down queue")
+ # Step 4: Reap worker threads. Use a per-worker timeout that is long
+ # enough for a worker currently mid-item to finish, but don't block
+ # forever. By this point the queues are drained (or we've logged the
+ # abandonment), so workers should exit promptly.
for worker in self.workers:
- worker.join(timeout=5.0)
+ worker.join(timeout=10.0)
+ if worker.is_alive():
+ logger.warning(
+ "Worker %d did not exit within join timeout",
+ worker.worker_id,
+ extra={"identifier": self.identifier},
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (44 lines, 1 file) — review recommended)
The
get_committable_offsetsmethod computes committable offsets by iterating overall_offsets[partition]frommin_offsettomax_offset. However,all_offsetsis never pruned — it accumulates every offset ever seen. Over time,min(all_offsets)will always return a very old, already-committed offset. The logic checksif offset <= last_committed: continuebut then themin_offsetscan starts from an ancient baseline, causing O(N) iteration over all historical offsets on every call. This is a correctness issue: aftermark_committedis called,all_offsetsstill retains those committed entries, so themin_offsetcomputed fromall_offsetsis a long-past offset, and the loop scan frommin_offsetwill always include stale offsets that are neither inoutstandingnor abovelast_committed, causing the logic to never find gaps correctly as the consumer runs longer.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -62,7 +62,7 @@ class OffsetTracker:
def complete_offset(self, partition: Partition, offset: int) -> None:
"""Mark an offset as completed."""
with self._get_partition_lock(partition):
self.outstanding[partition].discard(offset)
+ self.all_offsets[partition].discard(offset)
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
- For each partition, finds the highest contiguous offset that has been processed.
+ For each partition, finds the highest contiguous completed offset that has been processed.
+ An offset is "completed" when it has been removed from both outstanding and all_offsets
+ (via complete_offset). We track the committed frontier via last_committed and only
+ advance it over a contiguous run of completed offsets above that frontier.
"""
committable = {}
for partition in list(self.all_offsets.keys()):
with self._get_partition_lock(partition):
- all_offsets = self.all_offsets[partition]
- if not all_offsets:
+ outstanding = self.outstanding[partition]
+ if not outstanding:
+ # No in-flight offsets; nothing new to commit
continue
- outstanding = self.outstanding[partition]
last_committed = self.last_committed.get(partition, -1)
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
+ # The next offset to commit is just past what we last committed.
+ # The frontier of outstanding tells us how far we can scan.
+ start = last_committed + 1
+ # We can commit up to (but not through) the smallest in-flight offset.
+ # Everything below min(outstanding) has been completed.
+ committable_up_to = min(outstanding) - 1
highest_committable = last_committed
- for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
- highest_committable = offset
- else:
- break
+ # Every offset in [start, committable_up_to] has been completed
+ # (no longer outstanding, no longer in all_offsets) so we can
+ # advance the committed frontier to committable_up_to.
+ if committable_up_to >= start:
+ highest_committable = committable_up_to
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
- # Remove all offsets <= committed offset
- self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+ # Prune any stale entries <= committed offset from both tracking sets.
+ # Under normal operation these should already be empty (complete_offset
+ # removes from all_offsets; outstanding entries are in-flight and should
+ # not be below the committed frontier), but we prune defensively.
+ self.all_offsets[partition] = {
+ o for o in self.all_offsets[partition] if o > offset
+ }
+ self.outstanding[partition] = {
+ o for o in self.outstanding[partition] if o > offset
+ }🤖 Grapple PR auto-fix • major • Review this diff before applying
| self.result_processor = result_processor | ||
| self.identifier = identifier | ||
| self.num_queues = num_queues | ||
| self.offset_tracker = OffsetTracker() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (6 lines, 1 file))
The
FixedQueuePoolis constructed with aresult_processorobject instance. However, theresult_processorpassed toFixedQueuePoolisself.result_processorfromResultsStrategyFactory, which is an instance ofResultProcessor. TheFixedQueuePoolworker threads then callself.result_processor(identifier, item)treating it as a two-argument callable. But looking atResultProcessor, it is typically called asself.result_processor(message)with a single decoded message argument. The worker thread calling convention likely mismatches the actualResultProcessor.__call__signature, causing runtimeTypeErroron every message processed inthread-queue-parallelmode.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -100,7 +100,7 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
def __init__(
self,
worker_id: int,
work_queue: queue.Queue[WorkItem[T]],
- result_processor: Callable[[str, T], None],
+ result_processor: Callable[[T], None],
identifier: str,
offset_tracker: OffsetTracker,
) -> None:
@@ -136,7 +136,7 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
with sentry_sdk.start_transaction(
op="queue_worker.process",
name=f"monitors.{self.identifier}.worker_{self.worker_id}",
):
- self.result_processor(self.identifier, work_item.result)
+ self.result_processor(work_item.result)
except queue.ShutDown:
break
@@ -171,7 +171,7 @@ class FixedQueuePool(Generic[T]):
def __init__(
self,
- result_processor: Callable[[str, T], None],
+ result_processor: Callable[[T], None],
identifier: str,
num_queues: int = 20,
) -> None:🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (44 lines, 1 file) — review recommended)
OffsetTracker has no method to remove/reset state for a partition when Kafka partitions are revoked during a rebalance. If a partition is revoked and later reassigned, stale offset data in
all_offsets,outstanding, andlast_committedwill corrupt the commit logic. TheSimpleQueueProcessingStrategyhas noon_partitions_revokedhandler, and arroyo's strategy interface doesn't directly support it, but the factory'screate_with_partitionsis called on rebalance. Since the factory creates a new strategy on rebalance but theFixedQueuePool(and itsOffsetTracker) lives on the factory and persists across rebalances, stale partition state will accumulate.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -100,6 +100,30 @@ class OffsetTracker:
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
# Remove all offsets <= committed offset
self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+
+ def remove_partition(self, partition: Partition) -> None:
+ """
+ Remove all tracking state for a partition.
+
+ Must be called when a partition is revoked during a Kafka rebalance.
+ Since OffsetTracker persists on FixedQueuePool (which lives on the factory
+ across rebalances), failing to clean up here will cause stale offsets from
+ a previous assignment to corrupt get_committable_offsets() and mark_committed()
+ on reassignment.
+
+ Acquires the partition lock before removal to guard against concurrent
+ worker threads that may still be calling complete_offset() for in-flight
+ work items on this partition.
+ """
+ lock = self.partition_locks.get(partition)
+ if lock is None:
+ # Partition was never tracked; nothing to clean up.
+ return
+ with lock:
+ self.all_offsets.pop(partition, None)
+ self.outstanding.pop(partition, None)
+ self.last_committed.pop(partition, None)
+ # Remove the lock entry last, outside the lock itself, so any thread
+ # that acquired it just before us will finish its critical section
+ # before the entry disappears.
+ self.partition_locks.pop(partition, None)
class OrderedQueueWorker(threading.Thread, Generic[T]):
@@ -196,6 +220,19 @@ class FixedQueuePool(Generic[T]):
def wait_until_empty(self, timeout: float = 5.0) -> bool:
"""Wait until all queues are empty. Returns True if successful, False if timeout."""
start_time = time.time()
while time.time() - start_time < timeout:
if self.get_stats()["total_items"] == 0:
return True
time.sleep(0.01)
return False
+ def on_partitions_revoked(self, revoked: list[Partition]) -> None:
+ """
+ Clean up OffsetTracker state for revoked partitions.
+
+ Called by the consumer factory's create_with_partitions on rebalance.
+ Because FixedQueuePool (and its OffsetTracker) is shared across strategy
+ instances, we must explicitly clear per-partition state here; otherwise
+ stale all_offsets / outstanding / last_committed entries from the previous
+ assignment will corrupt commit logic when the partition is reassigned.
+ """
+ for partition in revoked:
+ logger.info(
+ "Clearing OffsetTracker state for revoked partition",
+ extra={"partition": partition},
+ )
+ self.offset_tracker.remove_partition(partition)
+
def shutdown(self) -> None:
"""Gracefully shutdown all workers."""
for worker in self.workers:🤖 Grapple PR auto-fix • major • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a queue-based parallelism management system for uptime/remote subscription consumers, introducing a new queue_consumer.py module that enables parallel processing of Kafka messages while controlling concurrency via queues. It also extends the existing result_consumer.py to support this queue-based processing model.
Changes: 7 files (+1276 / -6 lines)
- 🟢 4 files added:
src/sentry/remote_subscriptions/consumers/queue_consumer.py,tests/sentry/remote_subscriptions/__init__.py,tests/sentry/remote_subscriptions/consumers/__init__.py,tests/sentry/remote_subscriptions/consumers/test_queue_consumer.py - 🟡 3 files modified:
src/sentry/consumers/__init__.py,src/sentry/remote_subscriptions/consumers/result_consumer.py,tests/sentry/uptime/consumers/test_results_consumer.py
Resolves: #9 (Optimize spans buffer insertion with eviction during insert)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 7 files"/]
f1["🟡 __init__.py +2/-2"]
pr_0 --> f1
f2["🟢 queue_consumer.py +345/-0"]
pr_0 --> f2
f3["🟡 result_consumer.py +41/-3"]
pr_0 --> f3
f4["🟢 __init__.py +0/-0"]
pr_0 --> f4
f5["🟢 __init__.py +0/-0"]
pr_0 --> f5
f6["🟢 test_queue_consumer.py +421/-0"]
pr_0 --> f6
f7["🟡 test_results_consumer.py +467/-1"]
pr_0 --> f7
f2 -.- f3
f5 -.- f6
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 13 |
| 🟡 Minor | 7 |
| 🔵 Info | 1 |
Review details
- Reviewed at: 2026-04-12T03:17:27.099Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 165.8s
- Low-confidence findings suppressed: 10
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): | ||
| self.all_offsets[partition].add(offset) |
There was a problem hiding this comment.
🔴 CRITICAL — Thread Safety - Race Condition (confidence: 100%)
The _get_partition_lock method has a TOCTOU race condition. Two threads could simultaneously check self.partition_locks.get(partition), both get None, and then both call setdefault. While dict.setdefault is atomic in CPython due to the GIL, the initial get followed by setdefault is not an atomic operation, and this pattern is fragile. More critically, self.partition_locks is a plain dict that is accessed from multiple threads (worker threads calling complete_offset and the main thread calling add_offset and get_committable_offsets). In non-CPython implementations or future Python versions without a GIL (free-threaded Python 3.13+), this is a data race.
Evidence:
- The method first checks
self.partition_locks.get(partition)then conditionally callsself.partition_locks.setdefault(partition, threading.Lock()) - Multiple worker threads can call
add_offsetandcomplete_offsetconcurrently, which both call_get_partition_lock - dict.setdefault is only safe due to CPython GIL - this is not guaranteed by the Python language spec
- Multiple worker threads call
complete_offsetwhich calls_get_partition_lock - The main thread calls
add_offsetandget_committable_offsetswhich also call_get_partition_lock - dict.setdefault is only safe under CPython GIL; the two-step get-then-setdefault pattern is not atomic
- Python 3.13 introduces free-threaded mode (no GIL) where this would be a real data race
lock = self.partition_locks.get(partition)— unlocked readif lock: return lock— returns early without any synchronization guarantee outside CPython's GIL- The pattern is fragile: if partition_locks itself were replaced with a non-dict structure, this becomes a real race
Agent: architecture
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🔴 CRITICAL — Offset Commit Correctness (confidence: 100%)
The get_committable_offsets method iterates over all_offsets to find the highest contiguous completed offset, but the logic has a subtle correctness issue: it iterates from min_offset to max_offset and finds the contiguous completed range. However, after mark_committed cleans up offsets from all_offsets, there is no synchronization between the cleanup in mark_committed and a concurrent call to get_committable_offsets. If a worker thread completes an offset while get_committable_offsets is iterating, the all_offsets and outstanding sets could be in an inconsistent state within the lock scope, since get_committable_offsets acquires and releases the per-partition lock, but mark_committed also modifies all_offsets under the same lock. The real issue is that list(self.all_offsets.keys()) is taken without any lock, so a partition could be added or removed concurrently.
Evidence:
- The algorithm walks from min_offset to max_offset checking
if offset in outstanding: breakandif offset not in all_offsets: break - Kafka offsets can have gaps due to compaction, transactions, or filtered messages
- The
breakonoffset not in all_offsetsmeans any gap in tracked offsets stops contiguity scanning - Line 82:
for partition in list(self.all_offsets.keys())iterates without holding any lock - A worker thread could call
complete_offseton a new partition, modifyingself.all_offsetsandself.partition_locksconcurrently - mark_committed removes offsets from all_offsets, which could interact with concurrent get_committable_offsets calls
Agent: architecture
| if highest_committable > last_committed: | ||
| committable[partition] = highest_committable | ||
|
|
||
| return committable |
There was a problem hiding this comment.
🔴 CRITICAL — Logic Error - Offset Commit Correctness (confidence: 100%)
The get_committable_offsets method iterates offsets from min_offset to max_offset checking if each is NOT in outstanding, but it does NOT check if each offset is actually in all_offsets. If there are gaps in the tracked offsets (e.g., offsets 100, 102 are tracked but 101 was never seen), the method will walk through offset 101, see it's not outstanding, and consider it committed — advancing past an offset that was never processed. This violates the acceptance criterion that 'offsets should only be committed after all in-flight work at or before that offset is complete'.
Evidence:
- Lines 86-93: the loop does
for offset in range(min_offset, max_offset + 1)and checksif offset in outstandingbut never checksif offset in all_offsets - In Kafka, it's possible for the consumer to receive non-contiguous offsets (e.g., due to compaction, filtered payloads, or partition reassignment)
- The intent spec explicitly states: 'Offset commits are not advanced past unprocessed messages'
Agent: logic
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🟠 MAJOR — Resource Management - Shutdown (confidence: 100%)
The result_processor is now instantiated unconditionally in __init__ (moved from after the mode-specific setup), but the FixedQueuePool also receives a reference to self.result_processor. If result_processor_cls() is expensive or has side effects, this is now always triggered regardless of mode. More importantly, the result_processor is shared between the factory and the queue pool - if the pool's worker threads call it concurrently, the processor must be thread-safe, but there's no documentation or enforcement of this requirement.
Evidence:
- Line 117:
self.result_processor = self.result_processor_cls()moved before mode-specific initialization - Line 131:
result_processor=self.result_processorpasses the same instance to FixedQueuePool - Multiple queue worker threads will call this processor concurrently
Agent: logic
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🟠 MAJOR — Resource Lifecycle / Shutdown Safety (confidence: 100%)
The result_processor is now instantiated in __init__ (moved earlier from later in the constructor) for all modes, not just thread-queue-parallel. This is a behavioral change: previously self.result_processor was set after all mode-specific initialization. While this works for thread-queue-parallel (which passes it to FixedQueuePool), this reordering means that if result_processor_cls() raises an exception, none of the mode-specific resources (parallel_executor, multiprocessing_pool, queue_pool) will have been initialized, but the shutdown() method checks for self.parallel_executor and self.queue_pool which would not exist as attributes yet, causing an AttributeError during cleanup.
Evidence:
- Line 115:
self.result_processor = self.result_processor_cls()moved before mode-specific init - If result_processor_cls() succeeds but later init fails, or if it's called from a context that expects cleanup, attributes like self.parallel_executor may not exist
- shutdown() references self.parallel_executor and self.queue_pool without hasattr checks
Agent: architecture
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
| self.mode = mode | |||
There was a problem hiding this comment.
🟡 MINOR — Logic Error - result_processor instantiated before mode validation (confidence: 97%)
self.result_processor = self.result_processor_cls() is now called unconditionally at the top of __init__, before the mode-specific branches. If result_processor_cls() has side effects (e.g., opens DB connections, registers signal handlers, starts background threads), these will fire even for modes that don't use result_processor directly (e.g., 'parallel' with multiprocessing, where the processor runs in a subprocess). This could cause resource leaks or double-initialization in the parent process.
Evidence:
- Line 115:
self.result_processor = self.result_processor_cls()runs unconditionally. - Previously (before this PR),
self.result_processor = self.result_processor_cls()was at the bottom of__init__(line ~151 in old code), after mode setup. - For the 'parallel' multiprocessing mode, the parent process now holds a live
result_processorinstance that may hold resources unnecessarily. - This was flagged as a style issue in the first pass but has a real resource-leak security/correctness implication if
result_processor_clsopens connections.
Agent: security
| def __init__( | ||
| self, | ||
| mode: Literal["batched-parallel", "parallel", "serial"] = "serial", | ||
| mode: Literal["batched-parallel", "parallel", "serial", "thread-queue-parallel"] = "serial", |
There was a problem hiding this comment.
🟡 MINOR — Code organization (confidence: 90%)
The result_processor is instantiated before mode-specific initialization, but only used conditionally based on mode. This ordering could cause confusion about when the processor is available.
Evidence:
- Line 108 instantiates
self.result_processor = self.result_processor_cls()before the mode-specific branches (lines 112-135) - The processor is used in the thread-queue-parallel branch (line 132) but instantiation happens before mode checking
- In the original code, instantiation happened after all mode-specific setup, which was clearer about initialization order
Agent: style
| @@ -117,6 +128,13 @@ def __init__( | |||
| if num_processes is None: | |||
| num_processes = multiprocessing.cpu_count() | |||
| self.multiprocessing_pool = MultiprocessingPool(num_processes) | |||
There was a problem hiding this comment.
🟡 MINOR — Unnecessary work (confidence: 89%)
self.result_processor is instantiated eagerly at the top of init before mode is determined. In multiprocessing mode, the result_processor instance created in the parent process is unused (child processes instantiate their own), wasting initialization work and potentially holding resources (DB connections, etc.) unnecessarily.
Evidence:
self.result_processor = self.result_processor_cls()is now at line ~115, before the mode branching- Previously it was at the end of init (the diff shows it was moved from the bottom to the top)
- In multiprocessing mode, the parent process's result_processor is never used for actual processing
Agent: performance
| """ | ||
|
|
||
| multiprocessing_pool: MultiprocessingPool | None = None | ||
| queue_pool: FixedQueuePool | None = None |
There was a problem hiding this comment.
🟡 MINOR — Pattern Violation - Instance vs Class Attributes (confidence: 87%)
The attributes thread_queue_parallel, multiprocessing_pool, queue_pool, input_block_size, and output_block_size are defined as class-level attributes with default values, then overridden in __init__. This is an existing pattern in the file (for parallel_executor, etc.), but the addition of queue_pool as a mutable class attribute (None) shared across all instances is a subtle anti-pattern. If multiple factory instances were created, any bugs related to class-level mutable state could cause cross-instance pollution.
Evidence:
- Line 96:
thread_queue_parallel = Falseis a class attribute - Line 101:
queue_pool: FixedQueuePool | None = Noneis a class attribute - These are overridden in init but the class-level defaults remain accessible
Agent: architecture
| committable[partition] = highest_committable | ||
|
|
||
| return committable | ||
|
|
There was a problem hiding this comment.
🔵 INFO — Naming conventions (confidence: 83%)
The method _get_partition_lock() initializes a lock that may already exist but doesn't use the dict's setdefault pattern consistently with all_offsets and outstanding initialization.
Evidence:
- Line 49:
defaultdict(set)is used for auto-initialization of partition collections - Lines 78-82: Lock initialization uses manual lookup and setdefault instead of a defaultdict
- This inconsistency makes the code harder to scan for initialization patterns
Agent: style
| if highest_committable > last_committed: | ||
| committable[partition] = highest_committable | ||
|
|
||
| return committable |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The
get_committable_offsetsmethod iterates offsets frommin_offsettomax_offsetchecking if each is NOT inoutstanding, but it does NOT check if each offset is actually inall_offsets. If there are gaps in the tracked offsets (e.g., offsets 100, 102 are tracked but 101 was never seen), the method will walk through offset 101, see it's not outstanding, and consider it committed — advancing past an offset that was never processed. This violates the acceptance criterion that 'offsets should only be committed after all in-flight work at or before that offset is complete'.
| return committable | |
| if offset in all_offsets and offset not in outstanding: |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| """ | ||
| committable = {} | ||
| for partition in list(self.all_offsets.keys()): | ||
| with self._get_partition_lock(partition): |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (5 lines, 1 file))
all_offsets[partition] is never cleaned up after mark_committed is called. Offsets are added to all_offsets in add_offset() but the mark_committed() method only updates last_committed and removes from outstanding — it never removes the offset from all_offsets. Over time this set grows without bound, consuming increasing memory.
| with self._get_partition_lock(partition): | |
| # Prune all_offsets to prevent unbounded memory growth. | |
| # Only retain offsets strictly greater than the committed watermark, | |
| # since anything <= offset has already been committed and will never | |
| # need to be considered by get_committable_offsets again. | |
| self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset} |
🤖 Grapple PR auto-fix • major • confidence: 100%
| return { | ||
| "queue_depths": queue_depths, | ||
| "total_items": sum(queue_depths), | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (8 lines, 1 file))
Worker threads perform blocking queue.get() without a timeout inside the main processing loop. If the shutdown flag is set while a worker is blocked on queue.get(), the worker will hang indefinitely until a new item arrives or the queue is closed. This can cause the consumer to hang on shutdown when queues are empty.
| } | |
| try: | |
| work_item = self.work_queue.get(timeout=0.2) | |
| except queue.Empty: | |
| # No item available; loop back to re-check self.shutdown. | |
| # This prevents blocking indefinitely when shutdown is signaled | |
| # while the queue is empty. | |
| continue |
🤖 Grapple PR auto-fix • major • confidence: 100%
| # Remove all offsets <= committed offset | ||
| self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset} | ||
|
|
||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))
The
mark_committedmethod modifiesself.all_offsets[partition]andself.last_committed[partition]without acquiring the partition lock, whileadd_offset,complete_offset, andget_committable_offsetsall acquire the lock. This creates a data race when a worker thread callscomplete_offset(which holds the lock and modifiesoutstanding) concurrently with the main thread callingmark_committed(which modifiesall_offsetswithout the lock). Set mutation during concurrent iteration/mutation is not thread-safe in CPython beyond the GIL's basic guarantees.
| # NOTE: Lock is held here — safe for concurrent access with add_offset/complete_offset/get_committable_offsets |
🤖 Grapple PR auto-fix • major • confidence: 100%
| """ | ||
| Does the consumer use thread-queue-parallel processing? | ||
| """ | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The
thread_queue_parallelflag andqueue_poolare defined as class-level attributes (lines 96, 101), but the__init__method sets them as instance attributes only in specific mode branches. Increate_with_partitions(line 195-212), the code checksself.thread_queue_parallel— if mode is NOT 'thread-queue-parallel', this reads the class-levelFalsewhich is fine. Butqueue_poolis class-levelNone, andshutdown()checksself.queue_pooland sets it toNone. Sincequeue_poolis a class attribute, if two factory instances existed, the class attribute could be unintentionally shared. More practically, theself.queue_pool = Noneinshutdown()creates an instance attribute that shadows the class attribute, which is confusing but functionally correct for single-instance usage.
| self.thread_queue_parallel = False | |
| self.queue_pool: FixedQueuePool | None = None |
🤖 Grapple PR auto-fix • major • confidence: 100%
| click.Option( | ||
| ["--mode", "mode"], | ||
| type=click.Choice(["serial", "parallel", "batched-parallel"]), | ||
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (4 lines, 1 file))
The new mode 'thread-queue-parallel' is added to the CLI choices, but the diff only shows 2 lines changed in this file. If the consumer factory wiring (the function that maps mode to strategy factory) does not also handle 'thread-queue-parallel', selecting this mode at runtime would pass validation but could fail silently or raise an error downstream. The
ResultsStrategyFactory.__init__does handle it, but verify that any dispatch logic in__init__.pythat conditionally constructs strategy factories based on mode also recognizes this new value.
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), | |
| type=click.Choice(["serial", "parallel", "batched-parallel", "thread-queue-parallel"]), | |
| help="The mode to process results in. 'serial' processes one at a time, 'parallel' uses multithreading, 'batched-parallel' batches results before parallel processing, 'thread-queue-parallel' uses a queue-based concurrency model.", |
🤖 Grapple PR auto-fix • major • confidence: 82%
| """Mark an offset as completed.""" | ||
| with self._get_partition_lock(partition): | ||
| self.outstanding[partition].discard(offset) | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (9 lines, 1 file))
The
all_offsetsset grows unboundedly. Aftermark_committedis called (visible in tests), offsets should be cleaned fromall_offsetsto prevent memory growth. For long-running consumers processing millions of messages, this set will consume significant memory.
| # Prune all_offsets of any entries already below the last committed | |
| # offset to prevent unbounded memory growth. Offsets <= last_committed | |
| # are already safe to discard since mark_committed handles bulk cleanup, | |
| # but complete_offset may be called many times between commit cycles. | |
| last_committed = self.last_committed.get(partition, -1) | |
| if last_committed >= 0: | |
| self.all_offsets[partition] = { | |
| o for o in self.all_offsets[partition] if o > last_committed | |
| } |
🤖 Grapple PR auto-fix • minor • confidence: 97%
| def add_offset(self, partition: Partition, offset: int) -> None: | ||
| """Record that we've started processing an offset.""" | ||
| with self._get_partition_lock(partition): | ||
| self.all_offsets[partition].add(offset) |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (21 lines, 1 file) — review recommended)
The
_get_partition_lockmethod has a TOCTOU race condition. Two threads could simultaneously checkself.partition_locks.get(partition), both getNone, and then both callsetdefault. Whiledict.setdefaultis atomic in CPython due to the GIL, the initialgetfollowed bysetdefaultis not an atomic operation, and this pattern is fragile. More critically,self.partition_locksis a plaindictthat is accessed from multiple threads (worker threads callingcomplete_offsetand the main thread callingadd_offsetandget_committable_offsets). In non-CPython implementations or future Python versions without a GIL (free-threaded Python 3.13+), this is a data race.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -40,12 +40,21 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # Guards mutations to partition_locks dict itself, ensuring safety
+ # under free-threaded Python 3.13+ (no-GIL) where dict operations
+ # are no longer implicitly serialized.
+ self._partition_locks_lock: threading.Lock = threading.Lock()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
- lock = self.partition_locks.get(partition)
- if lock:
- return lock
- return self.partition_locks.setdefault(partition, threading.Lock())
+ # Fast path: read under the meta-lock to avoid TOCTOU between the
+ # initial get() and setdefault(). Under CPython with the GIL this
+ # is equivalent to the old pattern but is also safe under
+ # free-threaded Python 3.13+ (PEP 703) where dict operations are
+ # no longer serialized by the GIL.
+ lock = self.partition_locks.get(partition)
+ if lock is not None:
+ return lock
+ with self._partition_locks_lock:
+ # Re-check inside the lock (double-checked locking) so we
+ # don't allocate an extra Lock() object unnecessarily when two
+ # threads race here simultaneously.
+ return self.partition_locks.setdefault(partition, threading.Lock())🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| return committable | ||
|
|
||
| def mark_committed(self, partition: Partition, offset: int) -> None: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (84 lines, 1 file) — review recommended)
The
get_committable_offsetsmethod iterates overall_offsetsto find the highest contiguous completed offset, but the logic has a subtle correctness issue: it iterates frommin_offsettomax_offsetand finds the contiguous completed range. However, aftermark_committedcleans up offsets fromall_offsets, there is no synchronization between the cleanup inmark_committedand a concurrent call toget_committable_offsets. If a worker thread completes an offset whileget_committable_offsetsis iterating, theall_offsetsandoutstandingsets could be in an inconsistent state within the lock scope, sinceget_committable_offsetsacquires and releases the per-partition lock, butmark_committedalso modifiesall_offsetsunder the same lock. The real issue is thatlist(self.all_offsets.keys())is taken without any lock, so a partition could be added or removed concurrently.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -38,6 +38,7 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ self._global_lock = threading.RLock()
@@ -45,7 +46,10 @@ class OffsetTracker:
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
lock = self.partition_locks.get(partition)
if lock:
return lock
return self.partition_locks.setdefault(partition, threading.Lock())
+ # NOTE: callers must hold self._global_lock before calling this method
+ # to avoid a race between the .get() check and .setdefault() on
+ # self.partition_locks from concurrent threads introducing a new partition.
@@ -53,10 +57,14 @@ class OffsetTracker:
def add_offset(self, partition: Partition, offset: int) -> None:
"""Record that we've started processing an offset."""
- with self._get_partition_lock(partition):
+ with self._global_lock:
+ lock = self._get_partition_lock(partition)
+ with lock:
self.all_offsets[partition].add(offset)
self.outstanding[partition].add(offset)
@@ -63,8 +71,11 @@ class OffsetTracker:
def complete_offset(self, partition: Partition, offset: int) -> None:
"""Mark an offset as completed."""
- with self._get_partition_lock(partition):
+ with self._global_lock:
+ lock = self._get_partition_lock(partition)
+ with lock:
self.outstanding[partition].discard(offset)
@@ -73,26 +84,44 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
For each partition, finds the highest contiguous offset that has been processed.
+ Contiguity is evaluated over the set of *tracked* offsets only — gaps from
+ Kafka compaction, transactions, or filtered messages (offsets never added via
+ add_offset) are intentionally ignored.
"""
committable = {}
- for partition in list(self.all_offsets.keys()):
- with self._get_partition_lock(partition):
- all_offsets = self.all_offsets[partition]
- if not all_offsets:
- continue
-
- outstanding = self.outstanding[partition]
- last_committed = self.last_committed.get(partition, -1)
-
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
-
- highest_committable = last_committed
- for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
- highest_committable = offset
- else:
- break
-
- if highest_committable > last_committed:
- committable[partition] = highest_committable
+ # Take a snapshot of partitions under the global lock so that concurrent
+ # add_offset / complete_offset calls on new partitions cannot mutate
+ # self.all_offsets or self.partition_locks while we iterate.
+ with self._global_lock:
+ partitions = list(self.all_offsets.keys())
+ for partition in partitions:
+ with self._get_partition_lock(partition):
+ all_offsets = self.all_offsets[partition]
+ if not all_offsets:
+ continue
+
+ outstanding = self.outstanding[partition]
+ last_committed = self.last_committed.get(partition, -1)
+
+ # Iterate over sorted tracked offsets rather than a dense integer
+ # range. A dense range would break on any gap (compacted /
+ # transactional / filtered offset) that was never registered with
+ # add_offset, producing an artificially low committable offset.
+ sorted_offsets = sorted(all_offsets)
+
+ highest_committable = last_committed
+ prev = None
+ for offset in sorted_offsets:
+ if offset <= last_committed:
+ # Already committed; skip but update prev so the next
+ # offset's contiguity check is against the right base.
+ prev = offset
+ continue
+ # Break contiguity only when there is a gap between two
+ # *consecutive tracked* offsets (i.e. both were registered
+ # via add_offset). Untracked gaps are invisible here by design.
+ if prev is not None and offset != prev + 1:
+ break
+ if offset in outstanding:
+ # This offset is still being processed — cannot advance.
+ break
+ highest_committable = offset
+ prev = offset
+
+ if highest_committable > last_committed:
+ committable[partition] = highest_committable
+
return committable
@@ -100,7 +129,9 @@ class OffsetTracker:
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
- with self._get_partition_lock(partition):
+ with self._global_lock:
+ lock = self._get_partition_lock(partition)
+ with lock:
self.last_committed[partition] = offset
# Remove all offsets <= committed offset
self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}🤖 Grapple PR auto-fix • critical • Review this diff before applying
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
The
result_processoris now instantiated unconditionally in__init__(moved from after the mode-specific setup), but theFixedQueuePoolalso receives a reference toself.result_processor. Ifresult_processor_cls()is expensive or has side effects, this is now always triggered regardless of mode. More importantly, theresult_processoris shared between the factory and the queue pool - if the pool's worker threads call it concurrently, the processor must be thread-safe, but there's no documentation or enforcement of this requirement.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -89,6 +89,12 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
Does the consumer process all messages in parallel.
"""
+ # result_processor_cls must produce instances that are thread-safe when
+ # mode="thread-queue-parallel" is used, because the FixedQueuePool will call
+ # the processor concurrently from multiple worker threads. Subclasses that
+ # override result_processor_cls must ensure their processor implementation
+ # is either stateless or uses appropriate locking/synchronization.
+ result_processor_cls: type # abstract; must be set by subclasses
+
thread_queue_parallel = False
"""
Does the consumer use thread-queue-parallel processing?
@@ -115,7 +121,15 @@ def __init__(
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
+ # Instantiate the result_processor once here so it can be passed to
+ # mode-specific executors (e.g. FixedQueuePool). When mode is
+ # "thread-queue-parallel", this single instance WILL be called
+ # concurrently by multiple worker threads — the implementation returned
+ # by result_processor_cls() MUST be thread-safe (stateless or
+ # internally synchronized). See class-level comment above.
+ self.result_processor = self.result_processor_cls()
+
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -132,6 +146,13 @@ def __init__(
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ if mode == "thread-queue-parallel":
+ # Passes self.result_processor to the pool — see thread-safety
+ # requirement documented at the top of this class.
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(
result_processor=self.result_processor,🤖 Grapple PR auto-fix • major • Review this diff before applying
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Small fix (6 lines, 1 file))
The
result_processoris now instantiated in__init__(moved earlier from later in the constructor) for all modes, not just thread-queue-parallel. This is a behavioral change: previouslyself.result_processorwas set after all mode-specific initialization. While this works for thread-queue-parallel (which passes it toFixedQueuePool), this reordering means that ifresult_processor_cls()raises an exception, none of the mode-specific resources (parallel_executor, multiprocessing_pool, queue_pool) will have been initialized, but theshutdown()method checks forself.parallel_executorandself.queue_poolwhich would not exist as attributes yet, causing anAttributeErrorduring cleanup.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -93,6 +93,7 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
multiprocessing_pool: MultiprocessingPool | None = None
queue_pool: FixedQueuePool | None = None
+ parallel_executor: ThreadPoolExecutor | None = None
input_block_size: int | None = None
output_block_size: int | None = None
@@ -180,7 +180,9 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
def shutdown(self) -> None:
- if self.parallel_executor:
+ if self.parallel_executor is not None:
self.parallel_executor.shutdown()
- if self.queue_pool:
+ self.parallel_executor = None
+ if self.queue_pool is not None:
self.queue_pool.shutdown()
self.queue_pool = None🤖 Grapple PR auto-fix • major • Review this diff before applying
| self.queue_pool.shutdown() | ||
|
|
||
| def join(self, timeout: float | None = None) -> None: | ||
| self.close() |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (48 lines, 1 file) — review recommended)
The queue consumer uses Python's
queue.Queuewith aputoperation. Based on the intent specification, backpressure handling is critical when the queue is full. The truncated code makes it difficult to verify the exactputbehavior (blocking with timeout vs. non-blocking), but if the queue uses blockingput()without a timeout, the main consumer poll thread will block, preventing offset commits and Kafka heartbeats. This could cause the consumer to be kicked out of the consumer group due to missed heartbeats, triggering a rebalance.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -14,7 +14,8 @@
from arroyo.backends.kafka.consumer import KafkaPayload
-from arroyo.processing.strategies import ProcessingStrategy
+from arroyo.processing.strategies import MessageRejected, ProcessingStrategy
from arroyo.types import BrokerValue, FilteredPayload, Message, Partition
from sentry.utils import metrics
@@ -159,6 +160,16 @@ class FixedQueuePool(Generic[T]):
"""
Fixed pool of queues that guarantees order within groups.
Key properties:
- Each group is consistently assigned to the same queue
- Each queue has exactly one worker thread
- Items within a queue are processed in FIFO order
- No dynamic reassignment that could break ordering
- Tracks offset completion for safe commits
+
+ Backpressure:
+ - Each queue has a bounded maxsize to prevent unbounded memory growth
+ - submit() raises MessageRejected when a queue is full, signaling Arroyo
+ to pause fetching while still calling poll() for Kafka heartbeats
+ - The poll loop is never blocked indefinitely, preventing consumer group
+ eviction due to missed heartbeats
"""
+ # Maximum items per worker queue. At 20 queues this caps total in-flight
+ # work items at 20 * MAX_QUEUE_SIZE. Tune based on message size and memory.
+ MAX_QUEUE_SIZE = 1000
+
+ # How long submit() waits for queue space before raising MessageRejected.
+ # Must be short enough that the poll loop isn't stalled past
+ # session.timeout.ms / max.poll.interval.ms. 100 ms is a safe default.
+ SUBMIT_TIMEOUT_SECS = 0.1
+
def __init__(
self,
result_processor: Callable[[str, T], None],
identifier: str,
num_queues: int = 20,
+ max_queue_size: int | None = None,
) -> None:
self.result_processor = result_processor
self.identifier = identifier
self.num_queues = num_queues
+ self.max_queue_size = max_queue_size if max_queue_size is not None else self.MAX_QUEUE_SIZE
self.offset_tracker = OffsetTracker()
self.queues: list[queue.Queue[WorkItem[T]]] = []
self.workers: list[OrderedQueueWorker[T]] = []
for i in range(num_queues):
- work_queue: queue.Queue[WorkItem[T]] = queue.Queue()
+ work_queue: queue.Queue[WorkItem[T]] = queue.Queue(maxsize=self.max_queue_size)
self.queues.append(work_queue)
worker = OrderedQueueWorker[T](
@@ -204,12 +219,30 @@ class FixedQueuePool(Generic[T]):
def submit(self, group_key: str, work_item: WorkItem[T]) -> None:
"""
Submit a work item to the appropriate queue.
+
+ Raises MessageRejected if the target queue is full, which signals Arroyo
+ to apply backpressure (pause fetching, keep calling poll() for heartbeats).
+ The offset is NOT registered with the tracker until we know the item was
+ accepted, so no phantom offsets are left outstanding on rejection.
"""
queue_index = self.get_queue_for_group(group_key)
work_queue = self.queues[queue_index]
- self.offset_tracker.add_offset(work_item.partition, work_item.offset)
- work_queue.put(work_item)
+ try:
+ # Use a short timeout so the poll loop is never blocked indefinitely.
+ # add_offset is called only after successful enqueue so that a
+ # rejected item doesn't leave a dangling outstanding offset entry
+ # that would stall future commits.
+ work_queue.put(work_item, block=True, timeout=self.SUBMIT_TIMEOUT_SECS)
+ self.offset_tracker.add_offset(work_item.partition, work_item.offset)
+ except queue.Full:
+ metrics.incr(
+ "remote_subscriptions.queue_pool.backpressure",
+ tags={
+ "identifier": self.identifier,
+ "queue_index": str(queue_index),
+ },
+ )
+ # Raise MessageRejected to signal Arroyo's consumer runner to pause
+ # polling new messages while continuing to call poll() so Kafka
+ # heartbeats are maintained.
+ raise MessageRejected()
def get_stats(self) -> dict[str, Any]:🤖 Grapple PR auto-fix • major • Review this diff before applying
| self, | ||
| worker_id: int, | ||
| work_queue: queue.Queue[WorkItem[T]], | ||
| result_processor: Callable[[str, T], None], |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (49 lines, 1 file) — review recommended)
get_committable_offsets() iterates over all tracked offsets for each partition to find the highest contiguous committed offset. Because all_offsets and outstanding are plain sets (not sorted structures), min() and max() are O(n) each call, and the inner contiguous-scan loop is also O(n). This method is called on every commit cycle, and since offsets are never evicted from all_offsets until mark_committed is called, the sets grow unboundedly over the consumer's lifetime.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -1,7 +1,8 @@
from __future__ import annotations
import logging
import queue
import threading
import time
from collections import defaultdict
+from sortedcontainers import SortedList
from collections.abc import Callable
from dataclasses import dataclass
from typing import Any, Generic, TypeVar
@@ -38,8 +39,13 @@ class OffsetTracker:
def __init__(self) -> None:
- self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
+ # SortedList gives O(log n) add/remove and O(1) min/max via index,
+ # replacing the plain set whose min()/max() were O(n) each call.
+ self.all_offsets: dict[Partition, SortedList] = defaultdict(SortedList)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ # next_expected[partition] is the lowest offset we still need to scan
+ # from when computing the contiguous frontier. Avoids re-scanning from
+ # min(all_offsets) on every commit cycle.
+ self.next_expected: dict[Partition, int] = {}
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
@@ -52,6 +58,10 @@ class OffsetTracker:
def add_offset(self, partition: Partition, offset: int) -> None:
"""Record that we've started processing an offset."""
with self._get_partition_lock(partition):
self.all_offsets[partition].add(offset)
self.outstanding[partition].add(offset)
+ # Initialise the frontier pointer the first time we see an offset
+ # for this partition.
+ if partition not in self.next_expected:
+ self.next_expected[partition] = offset
def complete_offset(self, partition: Partition, offset: int) -> None:
"""Mark an offset as completed."""
@@ -67,28 +77,37 @@ class OffsetTracker:
committable = {}
for partition in list(self.all_offsets.keys()):
with self._get_partition_lock(partition):
all_offsets = self.all_offsets[partition]
if not all_offsets:
continue
outstanding = self.outstanding[partition]
last_committed = self.last_committed.get(partition, -1)
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
+ # O(1): SortedList index access instead of O(n) min()/max().
+ max_offset = all_offsets[-1]
+ # Resume from the known frontier rather than re-scanning from
+ # the beginning of all_offsets every cycle.
+ start = max(
+ last_committed + 1,
+ self.next_expected.get(partition, all_offsets[0]),
+ )
+
highest_committable = last_committed
for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
+ # Membership check is O(log n) for SortedList and O(1) for set.
+ if offset in all_offsets and offset not in outstanding:
highest_committable = offset
else:
break
+ # Advance the frontier pointer so the next commit cycle starts
+ # here instead of at last_committed + 1.
+ if highest_committable > last_committed:
+ self.next_expected[partition] = highest_committable + 1
+
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
- # Remove all offsets <= committed offset
- self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+ # irange gives us an O(log n) view of values <= offset; removing
+ # them one-by-one is O(k log n) where k is the number removed —
+ # far better than rebuilding the whole set via comprehension O(n).
+ sl = self.all_offsets[partition]
+ to_remove = list(sl.irange(None, offset))
+ for o in to_remove:
+ sl.remove(o)
+ # Ensure the frontier pointer is always at least last_committed + 1
+ # so we never re-scan already-committed history.
+ self.next_expected[partition] = max(
+ self.next_expected.get(partition, offset + 1),
+ offset + 1,
+ )🤖 Grapple PR auto-fix • major • Review this diff before applying
| offset_tracker: OffsetTracker, | ||
| ) -> None: | ||
| super().__init__(daemon=True) | ||
| self.worker_id = worker_id |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (39 lines, 1 file) — review recommended)
The
get_committable_offsetsalgorithm is logically flawed. It usesmin(all_offsets)as the starting point, but after some offsets are committed andlast_committedis updated, the minimum ofall_offsetsstill includes old (already-committed) offsets unless they're pruned. This means the contiguous scan frommin_offsetwill repeatedly find the same already-committed offsets as committable, potentially re-committing stale offsets or, if combined with the unbounded growth bug, producing wrong commit points.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -67,28 +67,38 @@ class OffsetTracker:
def get_committable_offsets(self) -> dict[Partition, int]:
"""
Get the highest offset per partition that can be safely committed.
- For each partition, finds the highest contiguous offset that has been processed.
+ For each partition, finds the highest contiguous offset that has been
+ processed, starting from the lowest tracked offset.
+
+ Correctness invariant: mark_committed() prunes all_offsets to only
+ contain offsets > last_committed, so min(all_offsets) is always the
+ first un-committed offset we know about. We can only advance if that
+ offset is contiguous with last_committed (i.e., min == last_committed + 1)
+ AND it is no longer outstanding. Any gap between last_committed and
+ min(all_offsets) means we cannot safely commit further.
"""
committable = {}
for partition in list(self.all_offsets.keys()):
with self._get_partition_lock(partition):
all_offsets = self.all_offsets[partition]
if not all_offsets:
continue
outstanding = self.outstanding[partition]
last_committed = self.last_committed.get(partition, -1)
- min_offset = min(all_offsets)
- max_offset = max(all_offsets)
-
- start = max(last_committed + 1, min_offset)
-
+ # Sort so we scan in ascending order and detect gaps immediately.
+ sorted_offsets = sorted(all_offsets)
+ min_offset = sorted_offsets[0]
+
+ # If there is a gap between the last committed offset and the
+ # lowest tracked offset, we cannot advance — some offsets in
+ # that gap were never added to all_offsets (or were from a
+ # prior assignment). Stop here.
+ if min_offset != last_committed + 1:
+ continue
+
highest_committable = last_committed
- for offset in range(start, max_offset + 1):
- if offset in all_offsets and offset not in outstanding:
- highest_committable = offset
- else:
+ prev = last_committed
+ for offset in sorted_offsets:
+ if offset != prev + 1:
+ # Gap in tracked offsets — cannot commit past prev.
break
+ if offset in outstanding:
+ # This offset is still being processed — stop here.
+ break
+ highest_committable = offset
+ prev = offset
if highest_committable > last_committed:
committable[partition] = highest_committable
return committable🤖 Grapple PR auto-fix • major • Review this diff before applying
| """ | ||
| Does the consumer use thread-queue-parallel processing? | ||
| """ | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (33 lines, 1 file) — review recommended)
The class-level attributes
multiprocessing_pool,queue_pool,input_block_size, andoutput_block_sizeare defined as class-level annotations with default values (None). SinceResultsStrategyFactoryis a class that may be instantiated multiple times in tests or in different modes, mutations to these attributes on one instance do not affect others — UNLESS Python's attribute lookup falls through to the class. The specific risk here:thread_queue_parallel = Falseandparallel = Falseare boolean class attributes. If a subclass or test accidentally reads these as class attributes rather than instance attributes after__init__sets them, state from a previous mode could bleed through.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -89,15 +89,9 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
Does the consumer process all messages in parallel.
"""
- thread_queue_parallel = False
- """
- Does the consumer use thread-queue-parallel processing?
- """
-
- multiprocessing_pool: MultiprocessingPool | None = None
- queue_pool: FixedQueuePool | None = None
- input_block_size: int | None = None
- output_block_size: int | None = None
-
+ # NOTE: Do not define mutable instance state here as class-level attributes.
+ # All mutable attributes (parallel, thread_queue_parallel, multiprocessing_pool,
+ # queue_pool, input_block_size, output_block_size) are initialized explicitly in
+ # __init__ to prevent class-level state bleed across instances or subclasses.
def __init__(
self,
mode: Literal["batched-parallel", "parallel", "serial", "thread-queue-parallel"] = "serial",
@@ -111,6 +105,15 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
+
+ # Initialize all mutable instance state explicitly, regardless of mode.
+ # This prevents class-level attribute lookup fallthrough and ensures each
+ # instance is fully isolated even if instantiated multiple times in the
+ # same process (e.g., during tests or mode switches).
+ self.parallel: bool = False
+ self.thread_queue_parallel: bool = False
+ self.multiprocessing_pool: MultiprocessingPool | None = None
+ self.queue_pool: FixedQueuePool | None = None
+ self.input_block_size: int | None = None
+ self.output_block_size: int | None = None
+
self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
@@ -124,6 +127,7 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ self.parallel = True
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(
@@ -148,10 +152,8 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
tags=metric_tags,
)
- if input_block_size is not None:
- self.input_block_size = input_block_size
- if output_block_size is not None:
- self.output_block_size = output_block_size
+ self.input_block_size = input_block_size if input_block_size is not None else None
+ self.output_block_size = output_block_size if output_block_size is not None else None🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| committable = self.queue_pool.offset_tracker.get_committable_offsets() | ||
|
|
||
| if committable: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (24 lines, 1 file) — review recommended)
If a worker thread in
FixedQueuePoolraises an unhandled exception while processing a work item, the exception will be caught by the thread's exception handler (if any) or silently terminate the thread. The work item's offset will never be marked as complete viacomplete_offset, causing that offset to remain inoutstandingforever. This meansget_committable_offsetswill never advance past that offset, effectively stalling all future Kafka offset commits for that partition — a silent consumer stall that appears healthy but stops making progress.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -149,20 +149,33 @@ class OrderedQueueWorker(threading.Thread, Generic[T]):
def run(self) -> None:
"""Process items from the queue in order."""
while not self.shutdown:
+ work_item = None
try:
work_item = self.work_queue.get()
except queue.ShutDown:
break
+ # NOTE: complete_offset MUST be called in the finally block below,
+ # even when result_processor raises. Failing to do so will permanently
+ # stall get_committable_offsets() for this partition, since that method
+ # requires contiguous completion. The trade-off is that a failed work
+ # item's offset is committed anyway — callers should treat result_processor
+ # failures as non-fatal and handle errors internally (e.g., via dead-letter
+ # queue or metrics) rather than relying on Kafka redelivery.
try:
with sentry_sdk.start_transaction(
op="queue_worker.process",
name=f"monitors.{self.identifier}.worker_{self.worker_id}",
):
self.result_processor(self.identifier, work_item.result)
except queue.ShutDown:
- break
+ # ShutDown raised during processing — still complete the offset
+ # before exiting so we don't stall the commit pipeline.
+ if work_item is not None:
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ break
except Exception:
logger.exception(
"Unexpected error in queue worker", extra={"worker_id": self.worker_id}
)
+ metrics.incr(
+ "remote_subscriptions.queue_worker.processing_error",
+ tags={"identifier": self.identifier},
+ )
finally:
- self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
+ # Guard against the (theoretically impossible) case where work_item
+ # was never assigned — e.g., if queue.get() raised something other
+ # than ShutDown before we broke out of the loop.
+ if work_item is not None:
+ self.offset_tracker.complete_offset(work_item.partition, work_item.offset)
metrics.gauge(
"remote_subscriptions.queue_worker.queue_depth",
self.work_queue.qsize(),
tags={
"identifier": self.identifier,
},
)🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| committable = self.queue_pool.offset_tracker.get_committable_offsets() | ||
|
|
||
| if committable: |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (9 lines, 1 file))
The commit loop in SimpleQueueProcessingStrategy calls get_committable_offsets() on every poll/submit cycle regardless of whether any offsets have completed since the last call. For high-throughput consumers, this adds unnecessary overhead computing the committable frontier on every message submission.
--- a/src/sentry/remote_subscriptions/consumers/queue_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/queue_consumer.py
@@ -38,6 +38,7 @@ class OffsetTracker:
def __init__(self) -> None:
self.all_offsets: dict[Partition, set[int]] = defaultdict(set)
self.outstanding: dict[Partition, set[int]] = defaultdict(set)
self.last_committed: dict[Partition, int] = {}
self.partition_locks: dict[Partition, threading.Lock] = {}
+ self._dirty = threading.Event()
def _get_partition_lock(self, partition: Partition) -> threading.Lock:
"""Get or create a lock for a partition."""
@@ -55,6 +56,7 @@ class OffsetTracker:
def complete_offset(self, partition: Partition, offset: int) -> None:
"""Mark an offset as completed."""
with self._get_partition_lock(partition):
self.outstanding[partition].discard(offset)
+ self._dirty.set()
def get_committable_offsets(self) -> dict[Partition, int]:
@@ -91,6 +93,7 @@ class OffsetTracker:
def mark_committed(self, partition: Partition, offset: int) -> None:
"""Update the last committed offset for a partition."""
with self._get_partition_lock(partition):
self.last_committed[partition] = offset
# Remove all offsets <= committed offset
self.all_offsets[partition] = {o for o in self.all_offsets[partition] if o > offset}
+ self._dirty.clear()
class OrderedQueueWorker(threading.Thread, Generic[T]):
@@ -273,6 +275,11 @@ class SimpleQueueProcessingStrategy(ProcessingStrategy[KafkaPayload], Generic[T
def _commit_loop(self) -> None:
while not self.shutdown_event.is_set():
try:
self.shutdown_event.wait(1.0)
+ # Skip the lock-heavy offset scan if no offsets have completed
+ # since the last commit cycle. This avoids unnecessary work when
+ # the consumer is idle or when all workers are still in-flight.
+ if not self.queue_pool.offset_tracker._dirty.is_set():
+ continue
+
committable = self.queue_pool.offset_tracker.get_committable_offsets()
if committable:🤖 Grapple PR auto-fix • minor • Review this diff before applying
| @@ -105,6 +115,7 @@ def __init__( | |||
| ) -> None: | |||
| self.mode = mode | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Small fix (6 lines, 1 file))
self.result_processor = self.result_processor_cls()is now called unconditionally at the top of__init__, before the mode-specific branches. Ifresult_processor_cls()has side effects (e.g., opens DB connections, registers signal handlers, starts background threads), these will fire even for modes that don't useresult_processordirectly (e.g., 'parallel' with multiprocessing, where the processor runs in a subprocess). This could cause resource leaks or double-initialization in the parent process.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -113,7 +113,6 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -126,6 +125,8 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ # Instantiate result_processor after mode setup so that modes which fork
+ # subprocesses (e.g. 'parallel') do not leak live processor resources in
+ # the parent process. 'thread-queue-parallel' needs it below, so it must
+ # be created before that branch.
+ self.result_processor = self.result_processor_cls()
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(🤖 Grapple PR auto-fix • minor • Review this diff before applying
| def __init__( | ||
| self, | ||
| mode: Literal["batched-parallel", "parallel", "serial"] = "serial", | ||
| mode: Literal["batched-parallel", "parallel", "serial", "thread-queue-parallel"] = "serial", |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (2 lines, 1 file))
The
result_processoris instantiated before mode-specific initialization, but only used conditionally based on mode. This ordering could cause confusion about when the processor is available.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -113,7 +113,6 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -126,6 +125,7 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ self.result_processor = self.result_processor_cls()
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(🤖 Grapple PR auto-fix • minor • Review this diff before applying
| @@ -117,6 +128,13 @@ def __init__( | |||
| if num_processes is None: | |||
| num_processes = multiprocessing.cpu_count() | |||
| self.multiprocessing_pool = MultiprocessingPool(num_processes) | |||
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Small fix (5 lines, 1 file))
self.result_processor is instantiated eagerly at the top of init before mode is determined. In multiprocessing mode, the result_processor instance created in the parent process is unused (child processes instantiate their own), wasting initialization work and potentially holding resources (DB connections, etc.) unnecessarily.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -115,7 +115,6 @@ def __init__(
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
- self.result_processor = self.result_processor_cls()
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -127,6 +126,8 @@ def __init__(
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+ else:
+ # Only instantiate in the parent process for non-multiprocessing modes.
+ # In "parallel" mode, child processes create their own result_processor instances.
+ self.result_processor = self.result_processor_cls()
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(🤖 Grapple PR auto-fix • minor • Review this diff before applying
| """ | ||
|
|
||
| multiprocessing_pool: MultiprocessingPool | None = None | ||
| queue_pool: FixedQueuePool | None = None |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)
The attributes
thread_queue_parallel,multiprocessing_pool,queue_pool,input_block_size, andoutput_block_sizeare defined as class-level attributes with default values, then overridden in__init__. This is an existing pattern in the file (forparallel_executor, etc.), but the addition ofqueue_poolas a mutable class attribute (None) shared across all instances is a subtle anti-pattern. If multiple factory instances were created, any bugs related to class-level mutable state could cause cross-instance pollution.
--- a/src/sentry/remote_subscriptions/consumers/result_consumer.py
+++ b/src/sentry/remote_subscriptions/consumers/result_consumer.py
@@ -19,6 +19,7 @@
from __future__ import annotations
import abc
+import logging
import multiprocessing
from collections import defaultdict
from concurrent.futures import ThreadPoolExecutor
@@ -27,6 +28,7 @@ from functools import partial
from typing import Any, Generic, Literal, TypeVar
from arroyo.types import BrokerValue, Commit, FilteredPayload, Message, Partition
+from typing import ClassVar
from sentry.conf.types.kafka_definition import Topic, get_topic_codec
from sentry.remote_subscriptions.consumers.queue_consumer import (
@@ -89,13 +90,6 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
Does the consumer process all messages in parallel.
"""
- thread_queue_parallel = False
- """
- Does the consumer use thread-queue-parallel processing?
- """
-
- multiprocessing_pool: MultiprocessingPool | None = None
- queue_pool: FixedQueuePool | None = None
- input_block_size: int | None = None
- output_block_size: int | None = None
+ thread_queue_parallel: ClassVar[bool] = False
+ """
+ Does the consumer use thread-queue-parallel processing?
+ This is a class-level sentinel default; instances override it in __init__.
+ """
def __init__(
self,
@@ -110,6 +104,13 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
) -> None:
self.mode = mode
metric_tags = {"identifier": self.identifier, "mode": self.mode}
+
+ # Declare all mutable instance attributes here to avoid class-level shared state
+ self.multiprocessing_pool: MultiprocessingPool | None = None
+ self.queue_pool: FixedQueuePool | None = None
+ self.input_block_size: int | None = None
+ self.output_block_size: int | None = None
+
self.result_processor = self.result_processor_cls()
+
if mode == "batched-parallel":
self.batched_parallel = True
self.parallel_executor = ThreadPoolExecutor(max_workers=max_workers)
@@ -128,6 +129,13 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if num_processes is None:
num_processes = multiprocessing.cpu_count()
self.multiprocessing_pool = MultiprocessingPool(num_processes)
+
if mode == "thread-queue-parallel":
self.thread_queue_parallel = True
self.queue_pool = FixedQueuePool(
@@ -148,10 +156,8 @@ class ResultsStrategyFactory(ProcessingStrategyFactory[KafkaPayload], Generic[T,
if input_block_size is not None:
self.input_block_size = input_block_size
-
if output_block_size is not None:
self.output_block_size = output_block_size
-🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #9 - Replicating Greptile benchmark for Grapple PR evaluation.