Add guard rails for DataStreams that differ in-size from their reported size. by LukeButters · Pull Request #704 · OctopusDeploy/Halibut

LukeButters · 2026-03-25T09:55:00Z

Background

ref CLOUDPT-11160
ref EFT-3141

DataStreams are sent using the following format:

<DataStream ID><DataStream Length><DataStream Data><DataStream Length>

In the code it is possible to declare the length of the DataStream to be different from the number of bytes sent in the DataStream. 😨
This is especially bad when the DataSent in the DataStream is less then the reported size, e.g. if the DataStream claims to be 100bytes but only 10 bytes is sent THEN we end up in a situation in which the BOTH ends sit waiting for each other to send data.

To ensure this is indeed NOT happening we will now:

Always log if the amount of data sent in the DataStream differs from the expected amount of data actually sent.
Optionally throw an exception when the wrong amount of data has been sent.

Throwing an exception is useful, because that will result in the TCP stream being killed which will result in both end reconnecting in a short amount of time. This will allow for another RPC to be made, useful if RPCs are retried.

Additionally:

Admit that sometimes the read message is null e.g. in the case of null requests for keeping a polling tentacle connection alive.
Fix up a bunch of tests to pass on mac.

Results

How to review this PR

Quality ✔️

Pre-requisites

I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
I have considered informing or consulting the right people, according to the ownership map.
I have considered appropriate testing for my change.

rhysparry

There are a few things that are a little trickier to understand than the could be. Assuming the ByteCountingStream works properly this looks to at least do all the same things, just clearer errors and a new exception if we haven't written the expected number of bytes.

Let me know when you've addressed the comments and I'll ✅

rhysparry · 2026-03-26T00:16:48Z

source/Halibut.Tests/Transport/Protocol/MessageSerializerFixture.cs

            {
                var result = await ReadMessage<ResponseMessage>(sut, stream);
-                result.Error.Should().NotBeNull();
+                result!.Error.Should().NotBeNull();


I'd prefer an assertion rather than the null-forgiving operator. It "should" give a clearer error too.

i.e.

result.Should().NotBeNull();

I'm pretty sure this properly marks result as non-null for the analyzers (at least, I've seen it do that).

It seems I still need the bang so changing to:

result.Should().NotBeNull(); result!.Error.Should().NotBeNull();

rhysparry · 2026-03-26T00:17:50Z

source/Halibut.Tests/Transport/Protocol/MessageSerializerFixture.cs

                var result = await ReadMessage<ResponseMessage>(sut, stream);
-                result.Error.Should().NotBeNull();
+                result!.Error.Should().NotBeNull();
                result.Error!.Message = "foo";


In theory you might even be able to drop this null-forgiving operator too.

source/Halibut/Transport/Protocol/MessageExchangeStream.cs

rhysparry · 2026-03-26T00:49:53Z

source/Halibut/Transport/Protocol/MessageExchangeStream.cs

        }

-        async Task<TemporaryFileStream> CopyStreamToFileAsync(Guid id, long length, Stream stream, CancellationToken cancellationToken)
+        async Task<TemporaryFileStream> CopyStreamToFileAsync(string messageId, Guid id, long length, Stream stream, IReadOnlyList<DataStream> deserializedStreams, CancellationToken cancellationToken)


Including the deserializedStreams here was a little confusing. We seem to use this to determine the total data stream length so that it can be included in the error message.

Some options:

Provide the total length instead of the data streams (we can then use it in ReadStreamAsync when we are logging/throwing exceptions rather than recalculating it)

Catch and provide the extra details in ReadStreamAsync.

Passing in the total size, and re-orderd the params, and renamed some prams

rhysparry · 2026-03-26T00:51:22Z

source/Halibut/Transport/Protocol/MessageExchangeStream.cs

+                                                            "Expected length: {2}, Actual bytes read: {3}. " +
+                                                            "Total length of all DataStreams to be sent is {4}. Exception: {5}",
+                                                    ex,
+                                                   messageId, id, length, length - bytesLeftToRead, totalDataStreamLength, ex.Message);


nit: extract a variable for length - bytesLeftToRead to improve clarity.

nit: Do implementations of WriteException actually not write the exception message? If not, does the duplication make things less clear?

I forced it to throw and it seems we can ditch printing the exception message.

rhysparry · 2026-03-26T01:06:11Z

source/Halibut/Transport/Protocol/MessageExchangeStream.cs

                await stream.FlushAsync(cancellationToken);

-                await ((IDataStreamInternal)dataStream).TransmitAsync(stream, cancellationToken);
+                var byteCountingStream = new ByteCountingStream(stream, OnDispose.LeaveInputStreamOpen);


Should we dispose the byteCountingStream anyway (via await using) so we get reliable behaviour rather than it being run by the garbage collector?

(I know we won't dispose the wrapped stream)

Disposing anyway.

rhysparry · 2026-03-26T01:15:34Z

source/Halibut/Transport/Protocol/IMessageSerializer.cs

    {
        Task<IReadOnlyList<DataStream>> WriteMessageAsync<T>(Stream stream, T message, CancellationToken cancellationToken);
-        Task<(T Message, IReadOnlyList<DataStream> DataStreams)> ReadMessageAsync<T>(RewindableBufferStream stream, CancellationToken cancellationToken);
+        Task<(T? Message, IReadOnlyList<DataStream> DataStreams)> ReadMessageAsync<T>(RewindableBufferStream stream, CancellationToken cancellationToken);


I think this is only used internally (within Halibut), so this change should be safe. I couldn't find usages in Octopus or Tentacle.

It should be, lol.

Specifically, it should be used internally only.

rhysparry

💚 LGTM

LukeButters added 10 commits March 25, 2026 20:48

.

aa7534e

.

75d0694

.

dbba0e2

.

55f408c

.

e13d9ab

.

07cd30b

.

f70ce15

Message can be null

72d0106

.

3bc86c6

Allow more tests to pass on mac

83b56c1

LukeButters changed the title . Add guard rails for DataStreams that differ in-size from their reported size. Mar 25, 2026

LukeButters added 2 commits March 26, 2026 11:02

.

232778a

.

f19a338

LukeButters marked this pull request as ready for review March 26, 2026 00:08

LukeButters requested a review from a team as a code owner March 26, 2026 00:08

rhysparry reviewed Mar 26, 2026

View reviewed changes

.

964bfac

rhysparry approved these changes Mar 26, 2026

View reviewed changes

LukeButters enabled auto-merge (squash) March 26, 2026 02:34

LukeButters merged commit 4cbfd93 into main Mar 26, 2026
17 checks passed

LukeButters deleted the luke/data-stream-sizes branch March 26, 2026 02:48

LukeButters mentioned this pull request Mar 29, 2026

Improving logging around receiving data streams. OctopusDeploy/OctopusTentacle#1202

Merged

3 tasks

Conversation

LukeButters commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Results

How to review this PR

Pre-requisites

Uh oh!

rhysparry left a comment

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!

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!

LukeButters Mar 26, 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!

rhysparry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LukeButters commented Mar 25, 2026 •

edited

Loading

LukeButters Mar 26, 2026 •

edited

Loading