Guard blocks handed to the transaction#1544
Conversation
A transaction runs blocks supplied by user code in a few places: the error block passed to `set_error`, `send_error`, and `report_error`, and the `after_create` and `before_complete` hooks. Any of them can raise. These blocks can run far from where they were defined. In agent mode an error block runs at completion, not when the error is added. So a raise propagated out of whatever drove creation or completion, surfacing the error in unrelated code. In collector mode it also skipped the backend's completion step, which left the transaction's OpenTelemetry context attached to the fiber. That leaked context then became the parent of the next request's root span on the reused thread, merging unrelated requests into one trace. Agent mode never showed the leak because its extension handle is not a per-fiber stack. Run each of these blocks defensively. On failure, log the error and the block's definition site, then carry on so creation and completion always finish. The error is swallowed rather than re-raised, because the block can run far from its caller and re-raising would surface it in code that has nothing to do with the block.
|
I've made this fix target |
There was a problem hiding this comment.
Pull request overview
This PR ensures that user-supplied blocks executed as part of the transaction lifecycle (error blocks and transaction hooks) cannot break transaction creation/completion when they raise, preventing incomplete completion paths that can leave OpenTelemetry context attached (notably in collector mode) and leak into subsequent requests.
Changes:
- Add a defensive
call_transaction_blockwrapper that logs exceptions (including definition site) and swallows them. - Route error blocks (
set_error/send_error/report_error) andafter_create/before_completehooks through the wrapper. - Add specs asserting blocks are swallowed/logged and that OpenTelemetry context is detached; add a patch changeset entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| spec/lib/appsignal/transaction_spec.rb | Adds coverage for raised user blocks (error blocks + hooks), verifying completion continues and OpenTelemetry context doesn’t leak. |
| lib/appsignal/transaction.rb | Introduces call_transaction_block and uses it for error blocks and lifecycle hooks to avoid breaking the transaction lifecycle. |
| .changesets/complete-transaction-when-a-block-raises.md | Documents the behavioral fix as a patch release changeset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.class.with_transaction(self) { block.call(self) } if block | ||
| if block | ||
| self.class.with_transaction(self) do | ||
| call_transaction_block(block, self, :source => "error") |
There was a problem hiding this comment.
Would you want to differentiate between the three different handlers that now have the source set to 'error'?
Like so for example:
| call_transaction_block(block, self, :source => "error") | |
| call_transaction_block(block, self, :source => "internal_set_error") |
There was a problem hiding this comment.
I'd like for it to be meaningful to the customer -- so send_error/set_error/report_error, but that information is lost at this point in the call tree. I could try and pass it down, or I could try to show all three names for readability.
As far as we're concerned, in terms of our ability to debug it, we should get which one of internal_set_error or report_errors_as_duplicates it was that caused it from the backtrace itself, so that information is never lost. The method name in the error is for the customer, for them to be able to debug what it is they're doing in a block that caused it to error.
What does concern me, though, is that you mentioned it's called three different times. I think some of those are redundant with each other -- if internal_set_error calls its block with call_transaction_block, then report_errors_as_duplicates does not need to call its block with call_transaction_block from inside a block passed to internal_set_error. Thank you for flagging that.
There was a problem hiding this comment.
Following up on this: I went with wrapping at the entry point.
Each error block is now wrapped once, where user code hands it in, instead of at every place it later runs. That fixed the redundancy I mentioned. The duplicate-transaction path no longer wraps a block from inside a block passed to internal_set_error. It calls the already-wrapped block directly.
It also recovered the customer-facing name. send_error and report_error pass their own name when they wrap the block, so the log names the call the customer made rather than an internal method. set_error is left out on purpose, because its block runs inline at the call site, so a failure there already surfaces to the caller.
The after_create and before_complete hooks are the one exception. They can be registered either with a block or by pushing a method onto the set, so there is no single entry point to wrap them at. They keep their guard where they run.
The first version guarded each block where it runs. That wrapped the error block in more than one place, including once from inside a block passed to `internal_set_error`, which was redundant. Wrap each error block once instead, where user code hands it in. It then stays protected wherever it later runs. `send_error` and `report_error` name themselves when they wrap the block, so the log points the customer at the call they made, rather than at an internal method name. The `after_create` and `before_complete` hooks keep their guard where they run. They can be registered with a block or by pushing a method onto the set, so there is no single entry point to wrap them at.
Related to #1542 -- the issue this PR fixes (triggered by a bogus method call in a test setup, now removed in appsignal/test-setups#380) is what allowed the OpenTelemetry context to leak in the first place. They each address one half of the issue: #1542 fixes accidentally making the current context the parent of what should always be a root span, this fixes a previous current context leaking into newer requests.
Guard blocks handed to the transaction
A transaction runs blocks supplied by user code in a few places: the
error block passed to
set_error,send_error, andreport_error, andthe
after_createandbefore_completehooks. Any of them can raise.These blocks can run far from where they were defined. In agent mode an
error block runs at completion, not when the error is added. So a raise
propagated out of whatever drove creation or completion, surfacing the
error in unrelated code. In collector mode it also skipped the backend's
completion step, which left the transaction's OpenTelemetry context
attached to the fiber. That leaked context then became the parent of the
next request's root span on the reused thread, merging unrelated
requests into one trace. Agent mode never showed the leak because its
extension handle is not a per-fiber stack.
Run each of these blocks defensively. On failure, log the error and the
block's definition site, then carry on so creation and completion always
finish. The error is swallowed rather than re-raised, because the block
can run far from its caller and re-raising would surface it in code that
has nothing to do with the block.
Wrap transaction blocks at their entry points
The first version guarded each block where it runs. That wrapped the
error block in more than one place, including once from inside a block
passed to
internal_set_error, which was redundant.Wrap each error block once instead, where user code hands it in. It then
stays protected wherever it later runs.
send_errorandreport_errorname themselves when they wrap the block, so the log points the customer
at the call they made, rather than at an internal method name.
The
after_createandbefore_completehooks keep their guard wherethey run. They can be registered with a block or by pushing a method
onto the set, so there is no single entry point to wrap them at.