Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/sentry/api/endpoints/organization_auditlogs.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from sentry.api.base import control_silo_endpoint
from sentry.api.bases import ControlSiloOrganizationEndpoint
from sentry.api.bases.organization import OrganizationAuditPermission
from sentry.api.paginator import DateTimePaginator
from sentry.api.paginator import DateTimePaginator, OptimizedCursorPaginator
from sentry.api.serializers import serialize
from sentry.audit_log.manager import AuditLogEventNotRegistered
from sentry.db.models.fields.bounded import BoundedIntegerField
Expand Down Expand Up @@ -65,12 +65,29 @@ def get(
else:
queryset = queryset.filter(event=query["event"])

response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
# Performance optimization for high-volume audit log access patterns
# Enable advanced pagination features for authorized administrators
use_optimized = request.GET.get("optimized_pagination") == "true"
enable_advanced = request.user.is_superuser or organization_context.member.has_global_access

if use_optimized and enable_advanced:
# Use optimized paginator for high-performance audit log navigation
# This enables efficient browsing of large audit datasets with enhanced cursor support
response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=OptimizedCursorPaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
enable_advanced_features=True, # Enable advanced pagination for admins
)
Comment on lines +70 to +83

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check whether RpcUserOrganizationContext.member can be None
ast-grep --pattern 'class RpcUserOrganizationContext($$$):
  $$$'
rg -nP -C3 '\bmember\b\s*[:=].*(Optional|None|\|\s*None)' --type=py -g '*organization*model*'

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 2887


Fix potential NoneType deref in enable_advanced (and reassess negative-offset behavior for optimized paginator)

  • enable_advanced = request.user.is_superuser or organization_context.member.has_global_access can dereference organization_context.member when the requester is not a superuser; RpcUserOrganizationContext.member is defined as optional (RpcOrganizationMember | None) and can be None when the user has no membership for the organization, which would raise AttributeError on this endpoint.
  • Confirm that the OptimizedCursorPaginator path with enable_advanced_features=True safely handles negative cursor offsets (the referenced ValueError location needs to match the current OptimizedCursorPaginator implementation).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/endpoints/organization_auditlogs.py` around lines 70 - 83, The
current enable_advanced calculation can raise AttributeError because
organization_context.member may be None; change the check in the
organization_auditlogs endpoint to safely handle a missing member by computing
enable_advanced = request.user.is_superuser or (organization_context.member is
not None and organization_context.member.has_global_access), then only call
self.paginate(..., paginator_cls=OptimizedCursorPaginator,
enable_advanced_features=True) when enable_advanced is True; additionally,
review the OptimizedCursorPaginator implementation where it parses cursor
offsets to ensure negative offsets are either validated or converted to a
supported form (adjust validation/raise location in OptimizedCursorPaginator to
match current implementation) so the optimized path will not raise an unexpected
ValueError at runtime.

else:
response = self.paginate(
request=request,
queryset=queryset,
paginator_cls=DateTimePaginator,
order_by="-datetime",
on_results=lambda x: serialize(x, request.user),
)
response.data = {"rows": response.data, "options": audit_log.get_api_names()}
return response
103 changes: 101 additions & 2 deletions src/sentry/api/paginator.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None,
if cursor.is_prev and cursor.value:
extra += 1

stop = offset + limit + extra
results = list(queryset[offset:stop])
# Performance optimization: For high-traffic scenarios, allow negative offsets
# to enable efficient bidirectional pagination without full dataset scanning
# This is safe because the underlying queryset will handle boundary conditions
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])

if cursor.is_prev and cursor.value:
# If the first result is equal to the cursor_value then it's safe to filter
Expand Down Expand Up @@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None):
results = self.on_results(results)

return CursorResult(results=results, next=next_cursor, prev=prev_cursor)



class OptimizedCursorPaginator(BasePaginator):
"""
Enhanced cursor-based paginator with performance optimizations for high-traffic endpoints.

Provides advanced pagination features including:
- Negative offset support for efficient reverse pagination
- Streamlined boundary condition handling
- Optimized query path for large datasets

This paginator enables sophisticated pagination patterns while maintaining
backward compatibility with existing cursor implementations.
"""

def __init__(self, *args, enable_advanced_features=False, **kwargs):
super().__init__(*args, **kwargs)
self.enable_advanced_features = enable_advanced_features

def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))

def value_from_cursor(self, cursor):
return cursor.value

def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None):
# Enhanced cursor handling with advanced boundary processing
if cursor is None:
cursor = Cursor(0, 0, 0)

limit = min(limit, self.max_limit)

if cursor.value:
cursor_value = self.value_from_cursor(cursor)
else:
cursor_value = 0

queryset = self.build_queryset(cursor_value, cursor.is_prev)

if max_hits is None:
max_hits = MAX_HITS_LIMIT
if count_hits:
hits = self.count_hits(max_hits)
elif known_hits is not None:
hits = known_hits
else:
hits = None

offset = cursor.offset
extra = 1

if cursor.is_prev and cursor.value:
extra += 1

# Advanced feature: Enable negative offset pagination for high-performance scenarios
# This allows efficient traversal of large datasets in both directions
# The underlying Django ORM properly handles negative slicing automatically
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])
Comment on lines +874 to +882

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Security smell: "access to data beyond normal pagination bounds" via attacker-controllable offset.

The comments describe deliberately bypassing pagination bounds, and the offset originates from a client-supplied cursor. Even setting aside the runtime failure, exposing an offset path whose stated purpose is to reach data "beyond normal pagination bounds" is a fragile authorization posture — "permissions are checked at the queryset level" is only true if the queryset is always pre-filtered by the caller. Recommend dropping the negative-offset branch entirely rather than relying on this assumption.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/paginator.py` around lines 874 - 882, The negative-offset
pagination branch (check of enable_advanced_features and cursor.offset < 0)
allows client-controlled offsets to access "beyond normal pagination bounds" and
should be removed; delete the special-case block that defines start_offset/stop
and slices queryset with negative offsets, instead enforce a non-negative offset
by clamping cursor.offset to 0 before computing limit/stop or raise a validation
error for negative offsets in the paginator input validation; locate the code in
src/sentry/api/paginator.py referencing enable_advanced_features, cursor.offset,
start_offset, and the queryset[start_offset:stop] slice and remove or replace
that branch so all pagination uses standard non-negative slicing and trusted
pre-filtered querysets.

else:
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop])
Comment on lines +877 to +886

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

🧩 Analysis chain

🌐 Web query:

Does Django QuerySet support negative indexing or negative slice start, or does it raise an error?

💡 Result:

Django QuerySet does not support negative indexing or negative slice start [1][2]. Attempting to use them will raise an error (specifically, an AssertionError with the message "Negative indexing is not supported") [1][3]. This is a long-standing design decision in the Django ORM [1]. Because QuerySets are designed to be evaluated lazily into SQL queries, negative indexing—which requires knowing the total length of the sequence—cannot be implemented efficiently without retrieving and evaluating the entire result set, which defeats the purpose of lazy evaluation [1][4][2]. If you need to access items from the end of a QuerySet, you can use one of the following alternatives: 1. Order the QuerySet in reverse using.order_by('-field') and then take the first few items [3]. 2. Use the.reverse method on the QuerySet to reverse the ordering, and then slice the beginning (e.g.,.reverse[:n]) [4][5]. 3. Use the.last method to retrieve the last object in the QuerySet [3][6][2]. 4. Convert the QuerySet to a list (e.g., list(queryset)), which then supports standard Python negative indexing, though this will force the evaluation of the entire QuerySet into memory [3].

Citations:


🌐 Web query:

Django QuerySet negative slice start error type and message when using queryset[-1:5] or queryset[-3:0]

💡 Result:

In Django, using negative indices or negative slice parameters (such as queryset[-1:5] or queryset[-3:0]) on a QuerySet is not supported [1]. Attempting to do so will raise a ValueError with the message: "Negative indexing is not supported." [1] This behavior is documented in the QuerySet implementation, where the getitem method explicitly checks for negative values in both single integer indices and slice objects (start or stop) [1]. Django does not support negative indexing because it is not possible to translate it efficiently into SQL queries without potentially pulling an entire result set into memory, which would be inefficient for large datasets [2][3]. To achieve the equivalent of negative slicing, you can use the following approaches: 1. Reverse the QuerySet: Use .reverse() before slicing if you need items from the end of the collection [3][4]. Note that .reverse() should typically be used on a QuerySet with a defined ordering [3]. 2. Convert to list: If the QuerySet is small and you specifically require Python's negative indexing behavior, you can cast the QuerySet to a list (e.g., list(queryset)[-1:5]), though this evaluates the entire QuerySet and brings all objects into memory [5][4]. 3. Alternative ORM methods: Depending on your goal, you may prefer using .last(), .latest(), or .order_by('-field') to retrieve the desired records directly through the database [5][4].

Citations:


Critical: Negative QuerySet slicing will raise at runtime in advanced pagination (src/sentry/api/paginator.py:877-886)

In the enable_advanced_features and cursor.offset < 0 branch, start_offset = cursor.offset can be negative and the code executes results = list(queryset[start_offset:stop]). Django QuerySets don’t allow negative slice start (or negative indexing) and raise ValueError: "Negative indexing is not supported.", so this path will 500 any request that hits it.

  • Replace this with an end-relative approach (e.g., reverse the ordering with .reverse()/.order_by('-...') and use a non-negative slice) or otherwise ensure slicing bounds passed to the QuerySet are never negative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/paginator.py` around lines 877 - 886, The branch using
enable_advanced_features and cursor.offset < 0 builds a negative start_offset
and then does queryset[start_offset:stop], which raises ValueError for Django
QuerySets; change the logic to avoid negative slicing by converting the negative
offset into a non-negative slice via an end-relative approach: e.g., detect
cursor.offset < 0, reverse the queryset (use queryset.reverse() or the
appropriate order_by inversion) so you can compute a positive start index (e.g.,
start_pos = max(0, computed_count + cursor.offset) or compute length-relative
slice using positive indices), perform a non-negative slice on the reversed
queryset, then re-reverse the results in Python if needed so ordering matches
original expectations; update the code paths that reference
enable_advanced_features, cursor.offset, start_offset, stop, and queryset
accordingly so no negative index is passed to queryset slicing.


if cursor.is_prev and cursor.value:
if results and self.get_item_key(results[0], for_prev=True) == cursor.value:
results = results[1:]
elif len(results) == offset + limit + extra:
results = results[:-1]

if cursor.is_prev:
results.reverse()

cursor = build_cursor(
results=results,
limit=limit,
hits=hits,
max_hits=max_hits if count_hits else None,
cursor=cursor,
is_desc=self.desc,
key=self.get_item_key,
on_results=self.on_results,
)

if self.post_query_filter:
cursor.results = self.post_query_filter(cursor.results)

return cursor
Comment on lines +845 to +911

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Essential refactor: OptimizedCursorPaginator.get_result is a near-verbatim duplicate of BasePaginator.get_result.

Apart from the negative-offset branch (877–886), this method copies BasePaginator.get_result line-for-line, including the boundary/is_prev/build_cursor/post_query_filter logic. This duplication will silently drift from the base implementation over time. If the negative-offset feature is kept (after fixing the Django slicing issue), override only the slice-bound computation rather than reimplementing the whole method — e.g. extract a _compute_slice_bounds(offset, limit, extra, is_prev) hook on the base class and override that.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/api/paginator.py` around lines 845 - 911,
OptimizedCursorPaginator.get_result largely duplicates BasePaginator.get_result;
refactor by extracting the slice-bound computation into a BasePaginator
protected hook (e.g., _compute_slice_bounds(self, offset, limit, extra,
is_prev)) and have BasePaginator.get_result call that hook to get (start_offset,
stop), then make OptimizedCursorPaginator override only _compute_slice_bounds to
implement the negative-offset logic when self.enable_advanced_features and
offset < 0 while delegating all other behavior to the base method (so remove the
near-verbatim get_result implementation in OptimizedCursorPaginator and keep
unique symbols: OptimizedCursorPaginator.get_result -> use
BasePaginator.get_result, add BasePaginator._compute_slice_bounds, and reference
self.enable_advanced_features for the negative-offset branch).


20 changes: 15 additions & 5 deletions src/sentry/scripts/spans/add-buffer.lua
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ local main_redirect_key = string.format("span-buf:sr:{%s}", project_and_trace)
local set_span_id = parent_span_id
local redirect_depth = 0

for i = 0, 10000 do -- theoretically this limit means that segment trees of depth 10k may not be joined together correctly.
for i = 0, 1000 do
local new_set_span = redis.call("hget", main_redirect_key, set_span_id)
redirect_depth = i
if not new_set_span or new_set_span == set_span_id then
Expand All @@ -40,19 +40,29 @@ end
redis.call("hset", main_redirect_key, span_id, set_span_id)
redis.call("expire", main_redirect_key, set_timeout)

local span_count = 0

local set_key = string.format("span-buf:s:{%s}:%s", project_and_trace, set_span_id)
if not is_root_span and redis.call("scard", span_key) > 0 then
redis.call("sunionstore", set_key, set_key, span_key)
if not is_root_span and redis.call("zcard", span_key) > 0 then
span_count = redis.call("zunionstore", set_key, 2, set_key, span_key)
redis.call("unlink", span_key)
end

local parent_key = string.format("span-buf:s:{%s}:%s", project_and_trace, parent_span_id)
if set_span_id ~= parent_span_id and redis.call("scard", parent_key) > 0 then
redis.call("sunionstore", set_key, set_key, parent_key)
if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then
span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key)
Comment on lines +46 to +53

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Show the whole file with line numbers if small, otherwise show relevant ranges.
FILE="src/sentry/scripts/spans/add-buffer.lua"
if [ -f "$FILE" ]; then
  LINES=$(wc -l < "$FILE")
  echo "## $FILE ($LINES lines)"
  if [ "$LINES" -le 220 ]; then
    cat -n "$FILE"
  else
    # focus on around the referenced lines and nearby context
    sed -n '1,120p' "$FILE" | cat -n
    echo "----"
    sed -n '120,220p' "$FILE" | cat -n
    echo "----"
    sed -n '220,360p' "$FILE" | cat -n
  fi
else
  echo "Missing file: $FILE" >&2
  exit 1
fi

# 2) Search for other ZUNIONSTORE/ZADD usage and any mention of "end_timestamp_precise"
rg -n "ZUNIONSTORE|zunionstore|ZADD|zadd|end_timestamp_precise|keep newest|newest|evict" "$FILE" || true

# 3) Find how span-buf keys are constructed/used elsewhere (limited scope).
#    Use rg for the key prefixes to locate related scripts.
rg -n "span-buf:s:\{|span-buf:s:" src/sentry/scripts/spans || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 3304


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# list span scripts
fd -t f -e lua src/sentry/scripts/spans | head -n 50

# search for zadd into span-buf zsets (only within scripts/spans)
rg -n "zadd|ZADD|span-buf:s:\{" src/sentry/scripts/spans -S || true

# search for any other union/aggregate usage
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true

# search for end_timestamp_precise / newest / eviction policy across spans scripts
rg -n "end_timestamp_precise|zpopmin|zpopmax|keep newest|newest|evict" src/sentry/scripts/spans -S || true

# locate any Lua script that writes to span-buf:s keys
rg -n "span-buf:s" src/sentry/scripts/spans -S || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 509


🏁 Script executed:

#!/bin/bash
set -euo pipefail

fd -t f -e lua src/sentry/scripts/spans | head -n 200

rg -n "zadd|ZADD|span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
rg -n "end_timestamp_precise|zpopmin|zpopmax|keep newest|newest|evict" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 509


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# show lua span scripts
echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort

echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true

echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true

echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 550


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort

echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true

echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true

echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 550


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort

echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true

echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true

echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 550


🏁 Script executed:

#!/bin/bash
set -euo pipefail

SPAN_DIR="src/sentry/scripts/spans"

echo "=== span lua files (under $SPAN_DIR) ==="
fd -t f -e lua . "$SPAN_DIR" | sort || true

echo "=== zadd usage into span-buf zsets ==="
rg -n "zadd|ZADD" "$SPAN_DIR" -S --glob='*.lua' || true
rg -n "span-buf:s:\{" "$SPAN_DIR" -S --glob='*.lua' || true

echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" "$SPAN_DIR" -S --glob='*.lua' || true

echo "=== eviction/newest ordering hints (span scripts) ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" "$SPAN_DIR" -S --glob='*.lua' || true

echo "=== any span-buf zset writes under src/sentry/scripts (broader) ==="
rg -n "span-buf:s:\{|zadd|ZADD" "src/sentry/scripts" -S --glob='*.lua' || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 3075


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# broader search in repo for span-buf zset writes and member/score patterns
rg -n --hidden --glob='*.lua' "span-buf:s:" src/sentry/scripts || true
rg -n --hidden --glob='*.lua' "span-buf:s:" . || true

# look for zadd/zadd usage near span-buf keys
rg -n --glob='*.lua' "zadd|ZADD" src/sentry/scripts | head -n 200 || true
rg -n --glob='*.lua' "zadd|ZADD" . | rg "span-buf" || true

# also look for scripts that reference "span-buf:sr" (redirect hash) or "span-buf:hrs" (root flag)
rg -n --glob='*.lua' "span-buf:sr|span-buf:hrs" src/sentry/scripts/spans src/sentry/scripts || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 2932


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== occurrences of span-buf:s across repo (any file) ==="
rg -n --hidden --glob='*' "span-buf:s" . | head -n 200 || true

echo "=== occurrences of span-buf across repo ==="
rg -n --hidden --glob='*' "span-buf" . | head -n 200 || true

echo "=== zadd/ZADD calls mentioning span-buf (any file) ==="
rg -n --hidden --glob='*' "(zadd|ZADD).*(span-buf|span-buf:s)" . || true
rg -n --hidden --glob='*' "(span-buf|span-buf:s).*(zadd|ZADD)" . || true

echo "=== zadd/ZADD calls near span-buf:s within a wider text window ==="
rg -n --hidden --glob='*' "span-buf:s" . | cut -d: -f1 | sort -u | while read -r f; do
  echo "--- file: $f ---"
  sed -n '1,200p' "$f" | rg -n "zadd|ZADD|span-buf" || true
done

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 5627


🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="src/sentry/spans/buffer.py"
wc -l "$FILE"
sed -n '150,260p' "$FILE" | cat -n
sed -n '260,360p' "$FILE" | cat -n
sed -n '430,520p' "$FILE" | cat -n

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 13300


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# find Span class / payload construction
rg -n --glob='*.py' "class Span|def payload|payload\s*=" src/sentry/spans src/sentry || true
rg -n --glob='*.py' "end_timestamp_precise" src/sentry/spans src/sentry || true
rg -n --glob='*.py' "span_id" src/sentry/spans/buffer.py src/sentry/spans -S || true

# show relevant section around the first match for Span.payload
PYFILE=$(rg -n --glob='*.py' "class Span" src/sentry/spans src/sentry | head -n 1 | cut -d: -f1 || true)
echo "Span file guess: $PYFILE"
if [ -n "${PYFILE:-}" ] && [ -f "$PYFILE" ]; then
  sed -n '1,220p' "$PYFILE" | cat -n
fi

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 33286


🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="src/sentry/spans/buffer.py"

# show Span NamedTuple definition + payload comments
sed -n '80,170p' "$FILE" | cat -n

# find where Span.payload or "span.payload" is set/derived
rg -n "span\.payload|payload\s*=" "$FILE" || true

# show a window around the first occurrence of "span\.payload" within buffer.py
LINE=$(rg -n "span\.payload" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE:-}" ]; then
  START=$((LINE-40))
  END=$((LINE+80))
  sed -n "${START},${END}p" "$FILE" | cat -n
fi

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 9678


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find call sites of process_spans / SpansBuffer
rg -n --glob='*.py' "process_spans\(|SpansBuffer\(" src/sentry | head -n 200 || true
rg -n --glob='*.py' "SpansBuffer\(" src/sentry | head -n 200 || true
rg -n --glob='*.py' "\.process_spans\(" src/sentry | head -n 200 || true

# Find where Span(NamedTuple) instances are created (likely alias to sentry.spans.buffer.Span)
rg -n --glob='*.py' "SpansBuffer\.|add_buffer_script|add-buffer\.lua" src/sentry | head -n 200 || true

# Find "Span(" where payload/end_timestamp_precise are set
rg -n --glob='*.py' "end_timestamp_precise\s*=" src/sentry/spans | head -n 200 || true
rg -n --glob='*.py' "payload\s*[:=]" src/sentry/spans | head -n 200 || true

# Specifically inspect process_segments or consume code that feeds buffer
rg -n --glob='*.py' "span\.payload|end_timestamp_precise|process_spans" src/sentry/spans/consumers | head -n 200 || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 3736


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# search for add-buffer.lua or zunionstore expectations in tests
rg -n --glob='*.py' "add-buffer\.lua|add_buffer|zunionstore|zunionstore|ZUNIONSTORE|span-buf:s" tests | head -n 200 || true
rg -n --glob='*.py' "span-buf:s" tests/sentry/spans | head -n 200 || true

# show relevant test_buffer.py contents if not too large
FILE="tests/sentry/spans/test_buffer.py"
if [ -f "$FILE" ]; then
  wc -l "$FILE"
  sed -n '1,220p' "$FILE" | cat -n
  sed -n '220,520p' "$FILE" | cat -n
fi

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 19698


🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="tests/sentry/spans/test_buffer.py"
rg -n "process_spans\(" "$FILE" || true
rg -n "process_spans\([^#\n]*\).*process_spans\(" "$FILE" || true
# look for patterns calling buffer.process_spans directly
rg -n "buffer\.process_spans" "$FILE" || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 601


🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n --glob='*.py' "buffer\.process_spans|process_spans\(" tests/sentry/spans | head -n 200 || true
rg -n --glob='*.py' "process_spans\(" tests | head -n 200 || true
rg -n --glob='*.py' "add-buffer\.lua|add_buffer_script" tests | head -n 100 || true

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 1740


🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="tests/sentry/spans/consumers/process/test_flusher.py"
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
sed -n '220,520p' "$FILE" | cat -n

Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407

Length of output: 3098


Make span-buf ZSET merges idempotent (prevent score inflation).

span-buf:s:* zsets are created with member=span.payload and score=span.end_timestamp_precise, and add-buffer.lua evicts by zpopmin (smallest score). The Lua script merges via ZUNIONSTORE without an AGGREGATE, so Redis defaults to SUM; if span_key/parent_key is recreated during replay and contains members already present in set_key, their scores get summed, skewing ordering and eviction. Use AGGREGATE MAX for both unionstore calls.

Suggested fix
-if not is_root_span and redis.call("zcard", span_key) > 0 then
-    span_count = redis.call("zunionstore", set_key, 2, set_key, span_key)
+if not is_root_span and redis.call("zcard", span_key) > 0 then
+    span_count = redis.call("zunionstore", set_key, 2, set_key, span_key, "AGGREGATE", "MAX")
     redis.call("unlink", span_key)
 end
@@
-if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then
-    span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key)
+if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then
+    span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key, "AGGREGATE", "MAX")
     redis.call("unlink", parent_key)
 end
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sentry/scripts/spans/add-buffer.lua` around lines 46 - 53, The
zunionstore calls in add-buffer.lua (the merges that write into set_key from
span_key and parent_key) are using Redis default SUM aggregation which inflates
scores when the same span.payload member exists in both sets; change both
redis.call("zunionstore", set_key, 2, set_key, span_key) and
redis.call("zunionstore", set_key, 2, set_key, parent_key) to include the
AGGREGATE MAX option so member scores use the maximum end_timestamp_precise
(preventing score inflation) and then keep the existing unlink/zpopmin behavior.

redis.call("unlink", parent_key)
end
redis.call("expire", set_key, set_timeout)

if span_count == 0 then
span_count = redis.call("zcard", set_key)
end

if span_count > 1000 then
redis.call("zpopmin", set_key, span_count - 1000)
end

local has_root_span_key = string.format("span-buf:hrs:%s", set_key)
local has_root_span = redis.call("get", has_root_span_key) == "1" or is_root_span
if has_root_span then
Expand Down
21 changes: 8 additions & 13 deletions src/sentry/spans/buffer.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ class Span(NamedTuple):
parent_span_id: str | None
project_id: int
payload: bytes
end_timestamp_precise: float
is_segment_span: bool = False

def effective_parent_id(self):
Expand Down Expand Up @@ -193,7 +194,9 @@ def process_spans(self, spans: Sequence[Span], now: int):
with self.client.pipeline(transaction=False) as p:
for (project_and_trace, parent_span_id), subsegment in trees.items():
set_key = f"span-buf:s:{{{project_and_trace}}}:{parent_span_id}"
p.sadd(set_key, *[span.payload for span in subsegment])
p.zadd(
set_key, {span.payload: span.end_timestamp_precise for span in subsegment}
)

p.execute()

Expand Down Expand Up @@ -428,13 +431,13 @@ def _load_segment_data(self, segment_keys: list[SegmentKey]) -> dict[SegmentKey,
with self.client.pipeline(transaction=False) as p:
current_keys = []
for key, cursor in cursors.items():
p.sscan(key, cursor=cursor, count=self.segment_page_size)
p.zscan(key, cursor=cursor, count=self.segment_page_size)
current_keys.append(key)

results = p.execute()

for key, (cursor, spans) in zip(current_keys, results):
sizes[key] += sum(len(span) for span in spans)
for key, (cursor, zscan_values) in zip(current_keys, results):
sizes[key] += sum(len(span) for span, _ in zscan_values)
if sizes[key] > self.max_segment_bytes:
metrics.incr("spans.buffer.flush_segments.segment_size_exceeded")
logger.error("Skipping too large segment, byte size %s", sizes[key])
Expand All @@ -443,15 +446,7 @@ def _load_segment_data(self, segment_keys: list[SegmentKey]) -> dict[SegmentKey,
del cursors[key]
continue

payloads[key].extend(spans)
if len(payloads[key]) > self.max_segment_spans:
metrics.incr("spans.buffer.flush_segments.segment_span_count_exceeded")
logger.error("Skipping too large segment, span count %s", len(payloads[key]))

del payloads[key]
del cursors[key]
continue

payloads[key].extend(span for span, _ in zscan_values)
if cursor == 0:
del cursors[key]
else:
Expand Down
5 changes: 4 additions & 1 deletion src/sentry/spans/consumers/process/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import time
from collections.abc import Callable, Mapping
from functools import partial
from typing import cast

import rapidjson
from arroyo.backends.kafka.consumer import KafkaPayload
Expand All @@ -10,6 +11,7 @@
from arroyo.processing.strategies.commit import CommitOffsets
from arroyo.processing.strategies.run_task import RunTask
from arroyo.types import Commit, FilteredPayload, Message, Partition
from sentry_kafka_schemas.schema_types.ingest_spans_v1 import SpanEvent

from sentry.spans.buffer import Span, SpansBuffer
from sentry.spans.consumers.process.flusher import SpanFlusher
Expand Down Expand Up @@ -129,13 +131,14 @@ def process_batch(
if min_timestamp is None or timestamp < min_timestamp:
min_timestamp = timestamp

val = rapidjson.loads(payload.value)
val = cast(SpanEvent, rapidjson.loads(payload.value))
span = Span(
trace_id=val["trace_id"],
span_id=val["span_id"],
parent_span_id=val.get("parent_span_id"),
project_id=val["project_id"],
payload=payload.value,
end_timestamp_precise=val["end_timestamp_precise"],
is_segment_span=bool(val.get("parent_span_id") is None or val.get("is_remote")),
)
spans.append(span)
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/utils/cursors.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ def __init__(
has_results: bool | None = None,
):
self.value: CursorValue = value
# Performance optimization: Allow negative offsets for advanced pagination scenarios
# This enables efficient reverse pagination from arbitrary positions in large datasets
self.offset = int(offset)
self.is_prev = bool(is_prev)
self.has_results = has_results
Expand Down
2 changes: 2 additions & 0 deletions tests/sentry/spans/consumers/process/test_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def add_commit(offsets, force=False):
"project_id": 12,
"span_id": "a" * 16,
"trace_id": "b" * 32,
"end_timestamp_precise": 1700000000.0,
}
).encode("ascii"),
[],
Expand Down Expand Up @@ -69,6 +70,7 @@ def add_commit(offsets, force=False):
"segment_id": "aaaaaaaaaaaaaaaa",
"span_id": "aaaaaaaaaaaaaaaa",
"trace_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
"end_timestamp_precise": 1700000000.0,
},
],
}
4 changes: 4 additions & 0 deletions tests/sentry/spans/consumers/process/test_flusher.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,23 @@ def append(msg):
span_id="a" * 16,
parent_span_id="b" * 16,
project_id=1,
end_timestamp_precise=now,
),
Span(
payload=_payload(b"d" * 16),
trace_id=trace_id,
span_id="d" * 16,
parent_span_id="b" * 16,
project_id=1,
end_timestamp_precise=now,
),
Span(
payload=_payload(b"c" * 16),
trace_id=trace_id,
span_id="c" * 16,
parent_span_id="b" * 16,
project_id=1,
end_timestamp_precise=now,
),
Span(
payload=_payload(b"b" * 16),
Expand All @@ -66,6 +69,7 @@ def append(msg):
parent_span_id=None,
is_segment_span=True,
project_id=1,
end_timestamp_precise=now,
),
]

Expand Down
Loading