Skip to content

Don't log delivery_info from the default error reporter - #48

Closed
ganey wants to merge 1 commit into
ruby-amqp:mainfrom
ganey:dont-log-delivery-info
Closed

ganey wants to merge 1 commit into
ruby-amqp:mainfrom
ganey:dont-log-delivery-info

Conversation

@ganey

@ganey ganey commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Sneakers::ErrorReporter::DefaultLogger#call does
Sneakers.logger.warn(context_hash), and Worker#process_work puts
delivery_info in that hash. delivery_info references the Bunny
channel, which references the connection, which references the channel
again.

With the default Logger + ProductionFormatter this is harmless: the
hash is interpolated into a string, and Hash#to_s handles cycles. With
a structured logger that serializes the payload, such as
SemanticLogger's JSON formatter calling as_json, it recurses until
SystemStackError. We hit this in production on a worker whose handler
raised; the consumer died and the queue sat at 0 consumers.

This drops :delivery_info from what DefaultLogger logs. The hash
passed to custom reporters is unchanged, so reporters that use
delivery_info keep working.

Added a spec with a cyclic delivery_info; it fails without the change.

DefaultLogger logs the whole context hash with Sneakers.logger.warn.
That hash includes delivery_info, which references the Bunny channel
and connection. With a stock Logger the hash is only interpolated into
a string, which is cycle-safe. A structured logger that serializes the
payload (e.g. as_json, as SemanticLogger's JSON formatter does)
recurses through the cyclic graph until it raises SystemStackError,
which takes down the consumer thread.

Drop :delivery_info from what DefaultLogger logs. The context hash
passed to custom error reporters is unchanged.
@ganey
ganey force-pushed the dont-log-delivery-info branch from 08ea8d2 to 427742e Compare October 17, 2026 20:11
@michaelklishin

Copy link
Copy Markdown
Member

@ganey @simi what do you think about dropping the logging line instead?

Or we can adjust Bunny so that it does not include the channel into the string representation of the delivery.

@michaelklishin

Copy link
Copy Markdown
Member

I suggest changing Bunny like so ruby-amqp/bunny#755 instead.

@michaelklishin

Copy link
Copy Markdown
Member

Let's continue in the Bunny PR for now. I will look into the one configuration CI failure soon, it's not mergeable yet. Sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants