Fix audited messages retaining mutated headers#7860
Conversation
|
Hi @w3lld1, Thank you for your contribution! We’ve routed the PR to the appropriate team for evaluation. If only small adjustments are needed, we’ll take care of them. If larger changes are required, we’ll either handle them ourselves (when we have capacity) or let you know what’s needed so you can choose whether to continue, or wait until we’re able to pick it up. Kind regards, |
|
Hi @w3lld1, We need to dedicate some time to investigate the impact of this PR as it will create a new dictionary for every single message that runs through the pipeline. Can you tell us what prompted you to raise this PR? Are you encountering an issue with the current behavior of the audit feature? |
|
I picked this up from #7739 rather than from a production incident of my own. The reported behavior is that pipeline mutations can leak into the audited copy: added headers remain, removed headers disappear, and original values can be replaced. That makes the audit record differ from the incoming transport message, which is the behavior this change is intended to correct. I understand the allocation concern. The current implementation favors a straightforward immutable snapshot, but I'm happy to adjust the approach if there is a lower-allocation point in the pipeline where the original headers can be retained or reconstructed safely. |
|
@w3lld1 We have reviewed this PR and we are accepting it. However we do not plan to merge it straight away. We had a good discussion about it and we need to consider the requirements more carefully before implementing a change. This may lead to a new feature, rather than addressing the bug as it is described. Our process for doing this is to create a new branch in our repo and merge these changes into it. Thank you for taking the time to put this together for us. |
b2ee208
into
Particular:7739-revert-audit-message-headers
Summary
Validation
dotnet build src --configuration Release --no-restoreNServiceBus.Core.Tests,NServiceBus.AcceptanceTests,NServiceBus.Core.Analyzer.Tests.Roslyn5,NServiceBus.TransportTests,NServiceBus.PersistenceTests, andNServiceBus.Learning.AcceptanceTests)Fixes #7739