Implement configurable message body storage by johnsimons · Pull Request #5646 · Particular/ServiceControl

johnsimons · 2026-07-24T03:58:05Z

This change introduces a pluggable message body storage mechanism, replacing the previous fake implementation. It enables configuring the storage type (e.g., FileSystem, Azure Blob, S3) via settings.

This change adds support for storing message bodies in Azure Blob Storage.

This completes the S3 body storage option introduced in the configurable message body storage feature.

Refactors Azure Blob and S3 body storage to stream large message bodies directly from cloud providers. This change avoids buffering entire messages into memory, improving performance and reducing memory consumption for large payloads.

warwickschroeder · 2026-07-27T01:50:30Z

+// existing one is left untouched. 
+public class FileSystemBodyStoragePersistence(EFPersisterSettings settings) : IBodyStoragePersistence
+{
+    const int FormatVersion = 1;


Its not clear to me how this is meant to be used. So if we change the body format, we increase this version number. Does that then mean users with error messages in the queue and body in external storage, retrieving these messages will just throw?

What's does that experience look like for the user?

Maybe add some comments to explain it? Making it clear what the impact of updating this actually is.

We should probably create a gate of some sort, maybe a test, to ensure this version is incremented.

Applies to the other seams as well

The version marker is insurance rather than something we plan to use. It exists so that if the layout ever does change, a body written under the old one fails loudly instead of being silently misread.

To your first question: if we bumped it today with no other change, every body already in storage would throw out of ReadBody and the message body panel would return a 500 until those messages aged past ErrorRetentionPeriod. That is exactly why the rule has to be that we never bump it on its own. Any future format change has to ship with read support for every version still within the retention window, so the bump becomes additive and no existing body breaks.

On the gate: agreed in principle, but there is nothing to gate yet while there is only one version. I would rather add the pinned-layout test at the point we actually introduce a v2, when we will know what shape the dispatch needs to take. Happy to add a comment stating the contract now if that is useful.

warwickschroeder · 2026-07-27T02:42:37Z

+            {
+                var metadata = content.Details.Metadata;
+
+                if (metadata.TryGetValue("FormatVersion", out var version) && version != FormatVersion)


Seems a missing FormatVersion is tolerated in Azure and AWS, but not file storage. Is there a reason for this?

Not a deliberate difference. The file header is positional so the field is present by construction and absence is unrepresentable, whereas the cloud stores do a dictionary lookup that can miss, and the miss path just falls through. In practice it is not load-bearing: we always write the metadata, and this ships new so there are no pre-existing objects without it.

Happy to tighten Azure and S3 to require it if you would prefer them consistent, it is a two-line change.

warwickschroeder · 2026-07-27T02:54:26Z

+            Stream bodyStream = isCompressed
+                ? new BrotliStream(fileStream, CompressionMode.Decompress, leaveOpen: false)
+                : fileStream;


AI found the below:

When we save a body to disk we write a small header first and then the body. On the way back out we read the header and hand the rest of the file to ASP.NET to send to the browser.

The problem is that we hand over the file handle itself. ASP.NET asks it how big it is, and it answers with the size of the whole file, header included. But we've already read past the header, so only the body bytes actually get sent. We promise 38 bytes and send 4. Kestrel spots the mismatch and kills the response, so the download fails.

Suggested change

Stream bodyStream = isCompressed

? new BrotliStream(fileStream, CompressionMode.Decompress, leaveOpen: false)

: fileStream;

Stream bodyStream = isCompressed

? new ExpectedLengthStream(new BrotliStream(fileStream, CompressionMode.Decompress, leaveOpen: false), bodySize)

: new ExpectedLengthStream(fileStream, bodySize);

Would it be better to store the metadata beside the body which I believe we do for Learning Transport, or will that bloat the disk space and operations too much?

fixed and added test

@rbev that would mean 2 reads instead of just one, not sure if it is worth it.
It also means compression needs to be done 2x.

Extracts body storage-related configuration from `EFPersisterSettings` into dedicated `BodyStorageSettings`, `AzureBlobStorageSettings`, and `S3StorageSettings`. This improves the organization and clarity of the persistence settings by grouping related properties and reducing the surface area of `EFPersisterSettings`.

rbev · 2026-07-27T04:23:48Z

+    {
+        var raw = SettingsReader.Read<string>(settingsRootNamespace, BodyStorageTypeKey);
+
+        if (string.IsNullOrWhiteSpace(raw))


should this error or default to filesystem in some predictable location instead of failing?

There's no defensible default path that holds across SCMU installs and containers, so any default we picked would be wrong somewhere. More importantly, silently writing bodies to local disk on an instance the operator intended to point at blob storage is a worse failure than refusing to start.
So yes it is a deliberate choice.

rbev · 2026-07-27T04:26:38Z

+        settings.AzureBlobServiceUri = hasServiceUri ? serviceUri : null;
+        settings.AzureBlobManagedIdentityClientId = SettingsReader.Read<string>(settingsRootNamespace, AzureManagedIdentityClientIdKey);
+        settings.AzureBlobAuthorityHost = ReadAzureAuthorityHost(settingsRootNamespace);
+        settings.AzureBlobContainerName = SettingsReader.Read(settingsRootNamespace, AzureContainerNameKey, settings.AzureBlobContainerName);


this value should probably be mandatory if you are using Azure Blob

For this setting we actually have a default of "error-bodies", so that is why we left it as not required

rbev · 2026-07-27T04:39:10Z

+        var buffer = ArrayPool<byte>.Shared.Rent(BrotliEncoder.GetMaxCompressedLength(body.Length));
+        try
+        {
+            return BrotliEncoder.TryCompress(body.Span, buffer, out var written, quality: 1, window: 22)


Should there be documentation around the decision to use Brotli over something else?

Is there something else that we should use instead?
Does it matter?

Anyway, I have added more docs about it.

rbev · 2026-07-27T04:40:20Z

+using ServiceControl.Persistence.EFCore.Infrastructure;

-public class BodyStorage : IBodyStorage
+// A body is stored inline in BodyText (small text) or in external storage (binary, or large text


should this be /// doc comments?

rbev · 2026-07-27T04:53:22Z

+            Stream bodyStream = isCompressed
+                ? new BrotliStream(fileStream, CompressionMode.Decompress, leaveOpen: false)
+                : fileStream;


Would it be better to store the metadata beside the body which I believe we do for Learning Transport, or will that bloat the disk space and operations too much?

rbev · 2026-07-27T04:54:01Z

+
+                if (shouldCompress)
+                {
+                    var brotliStream = new BrotliStream(fileStream, CompressionLevel.Fastest, leaveOpen: true);


Should the Brotli part of this be using the BodyCompression class?

No, I tried that, and trying to reuse that part of the code got too complex and therefore opted this way.
The complexity is becasue the filesystem has the file has the cache, so there is no in memory data.

rbev · 2026-07-27T04:54:06Z

+            var isCompressed = reader.ReadBoolean();
+
+            // The returned stream owns fileStream and is disposed by the caller.
+            Stream bodyStream = isCompressed


Should the Brotli part of this be using the BodyCompression class?

rbev · 2026-07-27T04:55:29Z

+                var isCompressed = bool.TryParse(metadata["is-compressed"], out var compressed) && compressed;
+                var contentType = response.Headers.ContentType ?? "application/octet-stream";
+                Stream stream = isCompressed
+                    ? new ExpectedLengthStream(new BrotliStream(response.ResponseStream, CompressionMode.Decompress), bodySize)


Should the Brotli part of this be using the BodyCompression class?

rbev · 2026-07-27T04:56:14Z

+        if (!string.IsNullOrEmpty(serviceUrl))
+        {
+            config.ServiceURL = serviceUrl;
+            config.ForcePathStyle = true; // Required for S3-compatible endpoints (MinIO, LocalStack).


What are the implications of this option when not on the non-AWS stacks?

It's only set when a custom ServiceUrl is configured, so it never applies against real AWS, where the SDK uses virtual-host-style addressing. The flag is there for S3-compatible endpoints like LocalStack and MinIO, which can't do virtual-host addressing without wildcard DNS.

This change organizes the message body storage implementation by: * Moving related files into a dedicated `Implementation.BodyStorage` namespace. * Extracting default container and key prefix names into `const` fields. * Adding XML documentation to key body storage components.

johnsimons added 4 commits July 24, 2026 13:44

Implement configurable message body storage

a526858

This change introduces a pluggable message body storage mechanism, replacing the previous fake implementation. It enables configuring the storage type (e.g., FileSystem, Azure Blob, S3) via settings.

Implement Azure Blob storage for message bodies

b593eeb

This change adds support for storing message bodies in Azure Blob Storage.

Implement S3 storage for message bodies

20611a9

This completes the S3 body storage option introduced in the configurable message body storage feature.

Improve message body streaming from cloud storage

f75acd8

Refactors Azure Blob and S3 body storage to stream large message bodies directly from cloud providers. This change avoids buffering entire messages into memory, improving performance and reducing memory consumption for large payloads.

johnsimons self-assigned this Jul 24, 2026

johnsimons marked this pull request as ready for review July 24, 2026 05:38

johnsimons requested review from abparticular, rbev and warwickschroeder July 24, 2026 05:38

warwickschroeder reviewed Jul 27, 2026

View reviewed changes

johnsimons requested a review from warwickschroeder July 27, 2026 05:02

rbev approved these changes Jul 27, 2026

View reviewed changes

warwickschroeder approved these changes Jul 27, 2026

View reviewed changes

johnsimons enabled auto-merge July 27, 2026 23:07

johnsimons merged commit 6f33fbf into master Jul 27, 2026
37 checks passed

johnsimons deleted the john/body_storage branch July 27, 2026 23:10

Uh oh!

Conversation

johnsimons commented Jul 24, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnsimons Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnsimons Jul 27, 2026 •

edited

Loading