Skip to content

perf: cache Frame objects in StacktraceBuilder by Line identity #2906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HazAT wants to merge 1 commit into
base: master
Choose a base branch
from
Open

perf: cache Frame objects in StacktraceBuilder by Line identity #2906

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions sentry-ruby/lib/sentry/interfaces/stacktrace_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,16 @@ def build(backtrace:, &frame_callback)
private

def convert_parsed_line_into_frame(line)
# Cache frames by Line object identity — same Line produces same Frame
cache_key = line.object_id
Copy link

@sentry sentry bot Mar 18, 2026

Choose a reason for hiding this comment

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

Bug: The frame cache uses line.object_id as a key, which is unsafe as object_ids can be reused after garbage collection, leading to incorrect cache hits.
Severity: HIGH

Suggested Fix

Use a cache key that uniquely and reliably identifies the source line, such as a string combining file, lineno, and method. This avoids the problem of object_id reuse. Additionally, consider cloning Frame objects when returning them from the cache to prevent mutations in callbacks from affecting the cached instance.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: sentry-ruby/lib/sentry/interfaces/stacktrace_builder.rb#L82

Potential issue: The frame caching mechanism uses `line.object_id` as a cache key. Since
Ruby can reuse `object_id`s for new objects after the original object has been garbage
collected, this can lead to cache key collisions. A `Frame` object for one source code
location could be incorrectly returned for a completely different location if a new
`Line` object happens to be assigned a recycled `object_id`. This would result in
corrupted stack trace data, where frames from one exception's backtrace appear in
another's.

Did we get this right? 👍 / 👎 to inform future reviews.

cached_frame = @frame_cache&.[](cache_key)
return cached_frame if cached_frame
Comment on lines +83 to +84
Copy link

@sentry sentry bot Mar 18, 2026

Choose a reason for hiding this comment

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

Bug: Cached Frame objects are mutable and can be modified by callbacks. These mutations persist in the cache, causing data to leak between different events.
Severity: MEDIUM

Suggested Fix

Clone the Frame object before returning it from the cache. This ensures that any mutations made by callbacks affect only a copy of the object, leaving the cached version pristine for subsequent use. For example, change return cached_frame to return cached_frame.dup.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: sentry-ruby/lib/sentry/interfaces/stacktrace_builder.rb#L83-L84

Potential issue: The `StacktraceBuilder` caches and reuses `Frame` object instances.
These objects are passed to a user-configurable `frame_callback`, which can mutate them
(e.g., by adding `vars`). Because the cache stores and returns a reference to the same
mutable object, any modifications made by the callback persist in the cache. Subsequent
exceptions that hit the same cache entry will receive the already-mutated `Frame`
object, leading to data from one event leaking into another.

Did we get this right? 👍 / 👎 to inform future reviews.


frame = StacktraceInterface::Frame.new(project_root, line, strip_backtrace_load_path)
frame.set_context(linecache, context_lines) if context_lines

@frame_cache ||= {}
@frame_cache[cache_key] = frame if @frame_cache.size < 2048
Copy link

@cursor cursor bot Mar 18, 2026

Choose a reason for hiding this comment

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

Cached frames mutated after caching cause data leakage

High Severity

StacktraceBuilder is a memoized singleton on Configuration, so @frame_cache persists across all events for the application lifetime. Cached Frame objects are mutated after caching — SingleExceptionInterface.build_with_stacktrace sets stacktrace.frames.last&.vars = locals, and frame_callback can also modify frames in-place (as demonstrated in the existing test). These mutations corrupt the cached Frame, causing exception locals or callback modifications from one error event to bleed into future events that reuse the same cached frame.

Fix in Cursor Fix in Web

Copy link

@cursor cursor bot Mar 18, 2026

Choose a reason for hiding this comment

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

Cache keyed by object_id breaks after GC

Medium Severity

The cache keys by line.object_id but stores only the integer key and Frame value — it holds no reference to the Line object itself. Since StacktraceBuilder is a long-lived singleton, original Line objects can be garbage collected and their object_id reassigned to completely different Line objects. This produces false cache hits, returning an incorrect Frame for a different backtrace line.

Fix in Cursor Fix in Web

frame
end

Expand Down
Loading