Optimize spans buffer insertion with eviction during insert

[akshayutture-augment]

Test 2

---

Replicated from ai-code-review-evaluation/sentry-greptile#2

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[akshayutture-augment]

@greptileai

[greptile-apps]

Greptile Overview

Confidence Score: 0/5

• This PR contains critical runtime errors that will crash production immediately
• Two guaranteed runtime errors: (1) AttributeError when organization_context.member is None in audit logs endpoint, (2) AssertionError from negative QuerySet slicing in OptimizedCursorPaginator. Additionally, removed span count validation creates DoS vulnerability in spans buffer.
• Critical attention required on src/sentry/api/endpoints/organization_auditlogs.py (null reference) and src/sentry/api/paginator.py (negative slicing). High attention on src/sentry/spans/buffer.py (removed DoS protection).

Important Files Changed

File Analysis

Filename	Score	Overview
src/sentry/api/endpoints/organization_auditlogs.py	0/5	Critical null reference bug: organization_context.member can be None, causing AttributeError on line 71
src/sentry/api/paginator.py	0/5	Critical runtime error: negative slice on Django QuerySet will raise AssertionError in OptimizedCursorPaginator
src/sentry/scripts/spans/add-buffer.lua	2/5	Reduced redirect depth limit from 10k to 1k; added span eviction during insert (zpopmin when >1000 spans)
src/sentry/spans/buffer.py	1/5	Removed max_segment_spans check during flush, creating DoS vector; changed from SET to ZSET for timestamp-based eviction

Sequence Diagram

sequenceDiagram
    participant Consumer as Span Consumer
    participant Factory as Process Factory
    participant Buffer as SpansBuffer
    participant Redis as Redis (ZSET)
    participant Lua as add-buffer.lua
    
    Note over Consumer,Lua: Span Insertion Flow (with eviction)
    Consumer->>Factory: process_batch(spans)
    Factory->>Factory: Extract end_timestamp_precise
    Factory->>Buffer: process_spans(spans, now)
    
    Buffer->>Redis: ZADD span-buf:s:{trace}:{parent}<br/>{payload: end_timestamp_precise}
    
    loop For each span
        Buffer->>Lua: EVALSHA add-buffer.lua
        Lua->>Redis: Resolve redirect chain (max 1000 depth)
        Lua->>Redis: ZUNIONSTORE merge span sets
        Lua->>Redis: ZCARD get span count
        
        alt span_count > 1000
            Lua->>Redis: ZPOPMIN evict oldest spans
            Note right of Lua: Evicts (span_count - 1000) spans
        end
        
        Lua-->>Buffer: Return (redirect_depth, keys)
    end
    
    Buffer->>Redis: ZADD to priority queue
    
    Note over Buffer,Redis: Flush Flow (removed span count check)
    Buffer->>Redis: ZRANGEBYSCORE get expired segments
    
    loop Load segment data
        Buffer->>Redis: ZSCAN read spans in pages
        Buffer->>Buffer: Check size (bytes only, no span count limit!)
        
        alt size > max_segment_bytes
            Buffer->>Buffer: Skip segment (log error)
        end
    end
    
    Buffer->>Consumer: Return flushed segments

Loading

[greptile-apps]

Additional Comments (1)

1. src/sentry/spans/buffer.py, line 449-453 (link)

   logic: Removed max_segment_spans check allows segments to grow unbounded during the flush read process. The Lua eviction at insert (line 62-64 in add-buffer.lua) only prevents storing >1000 spans, but doesn't prevent reading massive segments that exceed this during flush_segments. This creates a DoS vector where an attacker can force the system to read and process arbitrarily large segments by rapidly inserting spans that bypass the eviction timing.

_{9 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: organization_context.member can be None when the user doesn't have membership, causing AttributeError when accessing .has_global_access

Suggested change

	enable_advanced = request.user.is_superuser or organization_context.member.has_global_access
	enable_advanced = request.user.is_superuser or (organization_context.member and organization_context.member.has_global_access)

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/sentry/api/endpoints/organization_auditlogs.py
Line: 71:71

Comment:
**logic:** `organization_context.member` can be `None` when the user doesn't have membership, causing `AttributeError` when accessing `.has_global_access`

```suggestion
        enable_advanced = request.user.is_superuser or (organization_context.member and organization_context.member.has_global_access)
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Django QuerySets raise AssertionError on negative slicing (e.g., queryset[-5:]). This "advanced feature" will crash at runtime when cursor.offset < 0

Suggested change

	if self.enable_advanced_features and cursor.offset < 0:
	# Special handling for negative offsets - enables access to data beyond normal pagination bounds
	# This is safe because permissions are checked at the queryset level
	start_offset = cursor.offset # Allow negative offsets for advanced pagination
	stop = start_offset + limit + extra
	results = list(queryset[start_offset:stop])
	# Negative offsets are clamped to 0 to prevent Django AssertionError
	start_offset = max(0, offset) if not cursor.is_prev else offset
	stop = start_offset + limit + extra
	results = list(queryset[start_offset:stop])

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/sentry/api/paginator.py
Line: 877:882

Comment:
**logic:** Django QuerySets raise `AssertionError` on negative slicing (e.g., `queryset[-5:]`). This "advanced feature" will crash at runtime when `cursor.offset < 0`

```suggestion
        # Negative offsets are clamped to 0 to prevent Django AssertionError
        start_offset = max(0, offset) if not cursor.is_prev else offset
        stop = start_offset + limit + extra
        results = list(queryset[start_offset:stop])
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Reduced redirect depth limit from 10,000 to 1,000. Trace trees deeper than 1,000 levels will break the redirect resolution and cause incorrect span grouping. Was this intentional or accidental?

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/sentry/scripts/spans/add-buffer.lua
Line: 30:30

Comment:
**logic:** Reduced redirect depth limit from 10,000 to 1,000. Trace trees deeper than 1,000 levels will break the redirect resolution and cause incorrect span grouping. Was this intentional or accidental?

How can I resolve this? If you propose a fix, please make it concise.