Conversation
| } | ||
|
|
||
| // If non-default values have been found for all options, the loop can end | ||
| // early. | ||
| if (local_options.crashpad_handler_behavior != TriState::kUnset && |
There was a problem hiding this comment.
Bug: The loop's early termination condition incorrectly requires max_stack_capture_size to be non-zero, even though 0 is a valid default value, causing a performance regression.
Severity: MEDIUM
Suggested Fix
Remove the max_stack_capture_size != 0 check from the loop's early termination condition. The loop should break once the three TriState options (crashpad_handler_behavior, system_crash_reporter_forwarding, and gather_indirectly_referenced_memory) are resolved, consistent with the existing pattern for other optional numeric values.
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: snapshot/linux/process_snapshot_linux.cc#L171-L175
Potential issue: The early termination condition for the module iteration loop at lines
175-178 is logically incorrect. It now requires `max_stack_capture_size != 0` to break
the loop. However, `max_stack_capture_size = 0` is a valid default value indicating no
stack capture limit. This change forces the loop to iterate through all loaded modules
even when all other configuration options have been resolved, causing a performance
regression. This happens in the common case where a process has multiple modules and
does not explicitly set a `max_stack_capture_size`. The existing pattern for
`indirectly_referenced_memory_cap` correctly avoids including the numeric value in the
termination condition.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
indirectly_referenced_memory_cap has a companion TriState that tells us whether the value was resolved. This option currently doesn’t. I could use the same TriState pattern if preferred, but simply removing the check could skip a value set by a later module.
Follow-up to #165 with optional behavior that preserves the crashing thread's full stack.
This is motivated because Zig programs place a 256 KiB alternate signal stack in static TLS for every thread. On Linux/glibc, Crashpad includes that TLS in each worker's stack capture leading to the dump limit being exhausted quite fast with growing number of threads.
In the case of http://github.com/zml serving stack (LLMd) written in Zig, we hit this limitation almost instantly.
It kept the same general idea of #165 while making the limit optional and applying it only to non-crashing threads. This follows the precedent of #137 and getsentry/sentry-native#1427 where opt-in settings seem to be a favored approach.
Tracking parent PR: getsentry/sentry-native#2137