Repository navigation
fix(tracing): report spans that outlive their sent parent - #1163
Conversation
b822b9b to
203b3c5
Compare
203b3c5 to
b9a2229
Compare
b9a2229 to
c15942f
Compare
c15942f to
6863a94
Compare
| defp start_report_task(nil), do: :ok | ||
|
|
||
| defp start_report_task(test_process) do | ||
| notify = String.to_existing_atom(test_process) |
There was a problem hiding this comment.
Bug: The call to String.to_existing_atom/1 with an unvalidated URL parameter test_process can cause the LiveView to crash if the atom does not exist.
Severity: LOW
Suggested Fix
Although this is in a test application, consider using String.to_atom/1 within a try/rescue block to handle cases where the atom does not exist, or switch to String.to_atom/1 if atom creation is acceptable in this context. Alternatively, add a check to validate the input before conversion.
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:
test_integrations/phoenix_app/lib/phoenix_app_web/live/async_report_live.ex#L22
Potential issue: The function `start_report_task` receives a `test_process` string from
a URL query parameter and passes it directly to `String.to_existing_atom/1`. There is no
validation to ensure the corresponding atom exists in the atom table. If a request is
made to the `/async-report` endpoint with a `test_process` parameter that does not
correspond to a pre-existing atom, the call will raise an `ArgumentError`, causing the
LiveView process to crash during the `mount` phase.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6863a94. Configure here.
| :ets.delete(table_name, key) | ||
| end | ||
|
|
||
| remove_child_spans(span_data.span_id, table_name: table_name) |
There was a problem hiding this comment.
Finished spans dropped during send
High Severity
Cleanup after a send deletes every descendant that now has an end_time, not only spans that were in the payload and marked sent. Work that finishes after collection—especially nested spans whose in-progress parent is not marked—can be removed after on_end already decided to wait, so they are never reported.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6863a94. Configure here.
| # records are removed below, later spans can never be attached to this | ||
| # transaction either way. | ||
| sent_span_ids = [span_record.span_id | Enum.map(child_span_records, & &1.span_id)] | ||
| :ok = SpanStorage.mark_spans_sent(sent_span_ids) |
There was a problem hiding this comment.
Follow-ups can duplicate sent spans
Medium Severity
Follow-up transactions include every stored descendant with an end_time, without skipping IDs already passed to mark_spans_sent. If an in-progress parent ends while the ancestor send is still in flight, cleanup has not run yet, so spans already in the original payload can be sent again on the follow-up.
Reviewed by Cursor Bugbot for commit 6863a94. Configure here.
A span that outlives its transaction root (ie async work via Tasks/Broadway/Oban continuing a trace after the root was reported) was either corrupted or lost:
Unfinished children are now excluded from the transaction payload, and sent transaction roots leave a short-lived marker in span storage. A span ending with a marked parent is promoted to a follow-up transaction in the same trace - same
trace_id,parent_span_idpointing at the sent root, taggedsentry.parent_span_already_sent: true.Before
After
Fixes #1011