[devtools-agent]

Description of the change

Fixes #1030: the [Rollbar] Reporting internal error encountered while sending data to Rollbar. log line didn't say what the internal error was.

Notifier#report_internal_error logged only that fixed text. The exception that caused it reached the log only if reporting the internal error also failed (the rescue branch logs [Rollbar] Exception: …). Otherwise the details were only in the internal item sent to Rollbar. The reporter couldn't find that item, so the log gave them nothing to go on.

The line now includes the internal error's class, message and nearest frame. When the internal error happened while reporting another exception, it also includes that original exception:

[Rollbar] Reporting internal error encountered while sending data to Rollbar: RuntimeError: "database unavailable" in app/models/foo.rb:12:in 'rollbar_context' (while reporting ActiveRecord::ConnectionNotEstablished: "..." in ...)

• The formatting already existed in failsafe_exception_reason. It's extracted into exception_info and shared, so failsafe messages are unchanged.
• Each exception is described by describe_exception, which rescues StandardError and SystemStackError and falls back to that exception's class name. SystemStackError is included because report_with_rescue catches it while building the payload, and calling the same #message again here would raise it again. The fallback applies to each exception separately, so a broken #message on the original exception doesn't drop the internal error's details. That's the common case: Item::Backtrace calls the user exception's #message, so the internal error's frame points straight at the broken method. A broken #message also can't push report_internal_error into its failsafe path.

Repro steps (before this change)

1. Configure Rollbar with a logger, and report an exception whose payload can't be built. For example, its rollbar_context raises, standing in for a DB failover during payload building:

   e = StandardError.new('connection lost')
   def e.rollbar_context
     raise 'database unavailable'
   end
   Rollbar.error(e) # => "error"

2. The log shows only [Rollbar] Reporting internal error encountered while sending data to Rollbar. and an instance link. Neither RuntimeError: database unavailable nor the original connection lost error appears anywhere in the log.

Out of scope (noted during triage)

• The original error is never reported: only the internal error is. This PR doesn't change that.
• The issue's "thousands per request" storm wasn't reproduced. A 429 response doesn't raise an internal error: handle_response only logs a warning. So rate limiting alone doesn't explain it.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

• Linear: SDK-811
• Fix [Rollbar] Reporting internal error encountered while sending data to Rollbar #1030

How this was validated

Ruby 3.4 with gemfiles/rails81.gemfile, in a ruby:3.4 container.

• New specs in spec/rollbar_spec.rb (report_internal_error → logging the internal error):
  • Repro above via Rollbar.error: the log line names the internal and the original exception.
  • Internal error with no original error: class, message and nearest frame are logged.
  • Internal error whose #message raises: the class name is still logged.
  • Original exception whose #message raises, via Rollbar.error: the internal error's class, message and frame are still logged, followed by (while reporting StandardError).
  • Original exception whose #message overflows the stack, via Rollbar.error: Rollbar.error still returns 'error' instead of raising SystemStackError into the caller, and logs SystemStackError: "stack level too deep" in … (while reporting StandardError).
• The first 4 fail on master, which logs only the fixed text. The #message-on-the-original spec also fails on this PR's first commit, which logged only RuntimeError (the case raised in review). The stack-overflow spec fails with SystemStackError on f590f8d, before describe_exception rescued it. That was a regression from this PR, also raised in review: master returns 'error' there.
• Full suite: 684 examples, 0 failures (checkout mounted at a path containing rollbar-gem, which project_gems should handle regex gem patterns expects).
• RuboCop 1.15.0 (+ rubocop-performance) on the changed files reports no new offenses. The pre-existing Metrics/ClassLength on Notifier grows (640 → 643 lines on this branch, limit 100).
• Not run on the Ruby 2.5–2.7 CI matrix locally. The change uses no newer syntax.

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

🤖 Generated with Claude Code

[linear-code]

SDK-811

CR-669

[devtools-agent]

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

LGTM, no findings. I read every changed line against the code in the checkout.

lib/rollbar/notifier.rb

• report_internal_error (lib/rollbar/notifier.rb:541-561) now adds internal_error_details(exception, original_error) to its first log line. That line still runs after skip_reporting_internal_error, so internal errors listed in ignore_internal_errors are still never logged.
• internal_error_details (lib/rollbar/notifier.rb:565-575) reads the original exception from original_error[:exception]. That matches the hash built in report_with_rescue (lib/rollbar/notifier.rb:166-172). The other caller, process_from_async_handler (lib/rollbar/notifier.rb:263), passes no original error, which the original_error && ... guard handles. If #message raises, the StandardError rescue keeps the error out of the outer rescue in report_internal_error, so it can't force the failsafe path.
• Moving the formatting into exception_info (lib/rollbar/notifier.rb:853-864) doesn't change failsafe_exception_reason (lib/rollbar/notifier.rb:846-851). It still produces "<info>: <message>" and keeps its own rescue. The existing send_failsafe specs pin that format (spec/rollbar_spec.rb:2098, 2113, 2129).
• Ruby compatibility: the gemspec requires Ruby >= 2.0.0 (rollbar.gemspec:21). The new code uses only syntax available there (rescue directly in a def body, %[] literals).

spec/rollbar_spec.rb

• The repro spec works through Item#error_context: it calls exception.rollbar_context while building the item (lib/rollbar/item.rb:229-231, called from build_extra at lib/rollbar/item.rb:219). A raising rollbar_context therefore makes report fail, and report_with_rescue passes the original exception along.
• The specs swap in a logger mock the same way the existing report_internal_error spec does (spec/rollbar_spec.rb:2165-2181). This works because reconfigure_notifier builds a fresh notifier (spec/support/notifier_helpers.rb:2-14) and Notifier#logger caches its proxy only on first use (lib/rollbar/notifier.rb:379-381).
• # rubocop:disable Layout/LineLength is the right cop name for the pinned RuboCop 1.15.0 (Gemfile:77, .rubocop.yml:19).

Considered, not flagged

• The new log line includes the original exception's message without truncation. The failsafe diagnostic truncates it to FAILSAFE_STRING_LENGTH (lib/rollbar/notifier.rb:341-348). Existing log lines don't truncate either (lib/rollbar/notifier.rb:221-222, 297, 560), so this follows current practice. Maintainers may still want to decide whether original exception messages belong in app logs.
• If reporting the internal error also fails, the internal exception now shows up twice: on the new line and again in [Rollbar] Exception: ... (lib/rollbar/notifier.rb:560). That's cosmetic.

Out-of-scope items from the description
The original error still isn't reported, and the 429 storm wasn't reproduced. Neither is linked to a PR or ticket, so I'm only noting them, not confirming them. I didn't check the description's claims about test results and lint output, because I had no shell.

[waltjones]

The fallback drops the internal error's details when the original exception is the broken one. lib/rollbar/notifier.rb:565-575

internal_error_details has one rescue around both exception_info calls. If describing the original exception fails, the internal error's description is discarded too, even though it was already built, and only exception.class.name is logged.

This isn't an edge case. A broken #message is far more likely on a user's exception than on Rollbar's own internal error. Item::Backtrace calls current_exception.message (lib/rollbar/item/backtrace.rb:70), so when a user exception's #message raises:

• The resulting RuntimeError becomes the internal error.
• internal_error_details calls the original's #message again, which raises again.
• The log line ends with just RuntimeError.

I confirmed this against the PR's code with ruby -Ilib:
broken original#message => "RuntimeError"
The internal error's message and frame are lost, and the frame would have pointed straight at the broken #message method. The fix is to make the fallback apply to each exception separately:

def internal_error_details(exception, original_error)
  details = describe_exception(exception)
  original_exception = original_error && original_error[:exception]
  if original_exception
    details += " (while reporting #{describe_exception(original_exception)})"
  end

  details
end

def describe_exception(exception)
  exception_info(exception)
rescue StandardError
  exception.class.name.to_s
end

Add a spec for this case: an original exception whose #message raises, reported through Rollbar.error.

[devtools-agent]

Addressed the changes-requested review in f590f8d.

• The class-name fallback now lives in a new describe_exception, applied to each exception separately (lib/rollbar/notifier.rb, internal_error_details / describe_exception). This follows the suggested shape, plus .to_s so an anonymous class can't produce nil. If the original exception's #message raises, the internal error's class, message and frame are still logged, followed by (while reporting StandardError).
• Added the requested spec: an original exception whose #message raises, reported through Rollbar.error (report_internal_error → logging the internal error → when the original exception has a #message that raises). On the previous commit it fails with got: "...Rollbar: RuntimeError", the case you reproduced. It passes now.
• Ruby 3.4 / gemfiles/rails81.gemfile: full suite 683 examples, 0 failures. RuboCop 1.15.0 reports no new offenses on the changed files.

The review had no inline threads, so there was nothing to resolve. The PR description is updated to match.

[devtools-agent]

AI Agent Review (openai, openai-astra)

Review of #1223: log the internal error's class and message (#1030)

The log line from report_internal_error now says what the internal error was, and what was being reported when it happened. I checked the changed code paths in the checkout:

• Failsafe messages are unchanged. The new exception_info (lib/rollbar/notifier.rb:859-870) produces the same string the old inline code built. failsafe_exception_reason still rescues StandardError and returns '', as before.
• Where the original exception comes from. internal_error_details reads it from original_error[:exception]. The only caller that passes original_error is report_with_rescue (notifier.rb:166-172). process_from_async_handler (notifier.rb:263) passes none, so its log line describes only the internal error.
• The specs match the code paths. Item::Backtrace#trace_data calls the user exception's #message (lib/rollbar/item/backtrace.rb:70), so the "original #message raises" spec really does produce a RuntimeError: "broken message" internal error. In the "describing the internal error fails" spec, the exception has no backtrace, so describe_exception falls back to "StandardError".
• One gap: describe_exception rescues only StandardError. The surrounding code expects SystemStackError while building the payload: report_with_rescue rescues it explicitly (notifier.rb:165). The new log line calls the original exception's #message again, so an exception whose #message overflows the stack can now escape Rollbar.error into application code. Details are in the inline finding.

Note, not a defect: the original exception's message now goes to the application logger. Payload scrubbing does not apply to log lines. Whether that is acceptable is a product decision.

The description's test, suite and RuboCop results are the author's claims. The diff does not prove them, and I could not run anything here.

[devtools-agent]

AI Agent Review (openai, openai-astra)

Review: [SDK-811] Log the internal error's class and message

No defects found in the changed lines. I read the whole diff against the checkout: lib/rollbar/notifier.rb, its callers, Item/Item::Backtrace, LoggerProxy, the spec helpers and the specs that touch this log line.

What I checked

• Refactor keeps behaviour. failsafe_exception_reason (lib/rollbar/notifier.rb:854-859) now calls the extracted exception_info (lib/rollbar/notifier.rb:861-872). The logic and the rescue StandardError → '' fallback are the same as the removed code, including the case where exception_info returns nil. Failsafe bodies and logs don't change.
• The new log line can't push report_internal_error into its failsafe path. describe_exception (lib/rollbar/notifier.rb:579-583) rescues StandardError, SystemStackError for each exception separately. SystemStackError matches what report_with_rescue already catches (lib/rollbar/notifier.rb:163-175). This is relevant because Item::Backtrace#trace_data calls the user exception's #message (lib/rollbar/item/backtrace.rb:65-72). The internal errors that reach this code come from callers that rescued a raised exception (lib/rollbar/notifier.rb:165-172, lib/rollbar/notifier.rb:262-263), so they always have a backtrace frame.
• Placement. The new helpers are below private (lib/rollbar/notifier.rb:408). exception_info is defined only once in lib/ (grep).
• Existing specs still apply. The negative checks at spec/rollbar_spec.rb:1406-1408 and spec/rollbar_bc_spec.rb:58-60 use /...Rollbar./. The . matches the new :, so they still check for this line.
• New specs. reconfigure_notifier (spec/support/notifier_helpers.rb:2-15, via spec/support/shared_contexts.rb:1-2) clears the notifier before the nested Rollbar.configure. The memoized LoggerProxy (lib/rollbar/notifier.rb:379-381) is therefore built with the mock logger. The paths the new specs exercise (rollbar_context at lib/rollbar/item.rb:229-231, #message at lib/rollbar/item/backtrace.rb:70) are the ones they claim to cover. I didn't run the suite, so I'm not asserting that it passes.

Minor points (not findings)

• Anonymous exception classes. For those, exception.class.name is nil, and the describe_exception fallback exception.class.name.to_s (lib/rollbar/notifier.rb:582) gives "". The log line then reads ...Rollbar: or (while reporting ) with no description. exception_info already behaved this way in the failsafe path. If you care, a fallback of exception.class.name || exception.class.to_s would cover it.
• No truncation in the new log line. It puts both exceptions' messages in full. The same original message is truncated to FAILSAFE_STRING_LENGTH when it goes into the failsafe diagnostic (lib/rollbar/notifier.rb:332-352). That may be worth capping if large messages (for example SQL) are common. It's a design choice, not a defect.

Out of scope (stated in the description, not checked further): the original error still isn't reported, and the 429 storm wasn't reproduced. handle_response only logs a warning and runs the on_error_response hook on non-200 responses (lib/rollbar/notifier.rb:781-791), which matches the description.

No pinned dependency versions could be resolved for this checkout. None of the conclusions above depend on a library version.