Skip to content

feat(serialization): add owned Arc buffer codec and copier - #11463

Open
ReubenBond wants to merge 1 commit into
dotnet:mainfrom
ReubenBond:rb-arc-serialization-foundations
Open

ReubenBond wants to merge 1 commit into
dotnet:mainfrom
ReubenBond:rb-arc-serialization-foundations

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

ArcBuffer gains a built-in codec that preserves source ownership and contents across repeated serialization, and a copier that acquires independent pins over the referenced bytes. Reader<TInput>.ReadOwnedBuffer(int) centralizes owned payload reads: nonempty Arc input acquires slice pins, other inputs copy into available pooled-page capacity, and a zero-length read returns owner-free ArcBuffer.Empty without advancing. The codec delegates to this reader API.

This PR contains only the six-file serialization delta: codec/copier, owned-read method and Arc slicing helper, focused serialization/ownership tests, ownership-aware built-in codec test discovery, and generated API additions. General buffer and reader correctness fixes and raw lifetime tests were split into #11474, which has merged. This branch is rebased onto its main merge commit 42682376d80e1176a952ba13aa2de25a307b576e; those fixes are no longer part of this PR's diff.

Originally extracted from #10693, this layer serves independently owned pooled-byte serialization. Durable Messaging's GC-owned byte[] envelope payloads require no dependency on it. Main's page-pool policy is preserved; RPC lifetimes and retention policy remain separate concerns.

Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings October 10, 2026 15:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Arc readers can fail on valid buffers containing consecutive zero-length pages.

1 open finding
What changed in this PR

Adds ownership-aware serialization and copying for ArcBuffer, with lifetime fixes and focused tests.

Changes:

  • Adds an ArcBuffer codec and deep copier.
  • Improves empty-buffer, disposal, pinning, and reader-fork behavior.
  • Adds ownership/lifetime tests, documentation, and generated API updates.
File Description
test/​Orleans.Serialization.UnitTests/​BuiltInCodecTests.cs Recognizes ownership-specific codec tests.
test/​Orleans.Serialization.UnitTests/​ArcBufferLifetimeTests.cs Tests buffer and writer lifetimes.
test/​Orleans.Serialization.UnitTests/​ArcBufferCodecTests.cs Tests codec ownership and failure cases.
src/​Orleans.Serialization/​README.md Documents reference-counted buffer usage.
src/​Orleans.Serialization/​Codecs/​ArcBufferCodec.cs Implements serialization and deep copying.
src/​Orleans.Serialization/​Buffers/​Reader.cs Adds owned Arc slice reads and fixes fork offsets.
src/​Orleans.Serialization/​Buffers/​ArcBufferWriter.cs Improves empty-buffer and validity handling.
src/​Orleans.Serialization/​Buffers/​Adaptors/​BufferSliceReaderInput.cs Validates Arc input and exposes slicing.
src/​api/​Orleans.Serialization/​Orleans.Serialization.cs Updates the generated public API surface.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Orleans.Serialization/Buffers/Adaptors/BufferSliceReaderInput.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 15:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new valid owner-free buffer state conflicts with the non-nullable public First contract.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Medium severity Update ArcBuffer.First nullability contract for empty buffers

src/​Orleans.Serialization/​Buffers/​ArcBufferWriter.cs:1068

Making default/Empty a supported value means First is legitimately null for a valid ArcBuffer (as the new tests assert), but the public First field and constructor parameter still advertise non-nullable ArcBufferPage. Nullable-aware consumers can therefore dereference ArcBuffer.Empty.First without a warning. Please update the public nullability contract (and regenerate the API surface) so owner-free buffers expose ArcBufferPage?.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 15:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Consecutive empty pages can break valid reads, and the new empty state has an incorrect nullability contract.

1 open finding
Previously missed (2)

In code that hasn't changed since last review

Medium severity Expose nullable First and constructor page contracts

src/​Orleans.Serialization/​Buffers/​ArcBufferWriter.cs:1068

Empty makes First == null a valid public state, but First and the constructor parameter still expose non-nullable ArcBufferPage. Nullable-aware callers therefore receive no warning before dereferencing ArcBuffer.Empty.First. Change both contracts to ArcBufferPage? and regenerate the API surface.

Medium severity Consume exact remaining page capacity before allocating

src/​Orleans.Serialization/​Buffers/​Reader.cs:536

Requesting exactly 4096 bytes causes ArcBufferWriter.GetSpan to allocate whenever the current page has exactly 4096 bytes left (sizeHint >= WriteCapacity). As a result, this copy path leaves one quarter of each 16 KiB pooled page unused and allocates roughly 33% more pages for large payloads. Obtain the current span first and cap the read size afterward so exact remaining capacity is consumed.

🧠 Review effort: Balanced

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request Current main Variance
Lines 83.38% (118,470 / 142,089) 83.40% (118,474 / 142,050) -0.0257 pp
Branches 72.94% (35,227 / 48,294) 72.99% (35,240 / 48,282) -0.0451 pp

Report-only conclusion: regressed.

The current-main baseline is commit 42682376d8 and uses the same reviewed coverage matrix.

Coverage combines every CI test matrix job, including providers, CodeGen, .NET 8/10, Linux, Windows, and macOS, using canonical physical source and branch identities.

The comparison remains report-only while normal line and branch variance is calibrated.

Coverage details

Copilot AI balanced review requested due to automatic review settings October 10, 2026 16:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new valid empty state exposes a null page through a public member declared non-nullable.

1 open finding
Previously missed (1)

In code that hasn't changed since last review

Medium severity Annotate ArcBuffer.First as nullable

src/​Orleans.Serialization/​Buffers/​ArcBufferWriter.cs:1068

Empty makes a null First part of the valid public contract, but ArcBuffer.First and the constructor's first parameter are still declared non-nullable (and the generated API surface preserves that annotation). Nullable-aware consumers can therefore dereference ArcBuffer.Empty.First without a warning and get a NullReferenceException. Please annotate the page as ArcBufferPage? throughout the public and dependent internal API, then regenerate the API surface.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 16:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Structurally invalid public ArcBuffer values can be serialized into malformed length-prefixed payloads.

2 open findings

🧠 Review effort: Balanced

Comment thread src/Orleans.Serialization/Codecs/ArcBufferCodec.cs
Copilot AI balanced review requested due to automatic review settings October 10, 2026 18:09
@ReubenBond
ReubenBond force-pushed the rb-arc-serialization-foundations branch from ef5a482 to 6b2e783 Compare October 10, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Empty-page reads, malformed public buffer shapes, and the nullable First contract remain unresolved.

3 open findings

🧠 Review effort: Balanced

Comment thread src/Orleans.Serialization/Buffers/ArcBufferWriter.cs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Several public writer operations still bypass the new disposed-state guard and throw the wrong exception.

0 open findings

3 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add disposed checks to slice and zero-length reader methods

src/​Orleans.Serialization/​Buffers/​ArcBufferWriter.cs:395

The disposed-state guard is only effective for methods which call ThrowIfPinnedPages. After Dispose() nulls the page fields, AdvanceReader(0), PeekSlice(0), and ConsumeSlice(0) still bypass this guard and dereference _readPage, producing NullReferenceException instead of the newly established ObjectDisposedException behavior. Add a shared disposed check to these public entry points as well.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 10, 2026 23:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The reference-counting and pooled-page lifetime changes warrant final human validation despite strong focused coverage.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 00:32
@ReubenBond
ReubenBond force-pushed the rb-arc-serialization-foundations branch from 9dd1a83 to 6ef3bcc Compare October 11, 2026 00:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Low-level reference-counted ownership and pooled-page lifetime semantics warrant final human review despite broad targeted tests.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 01:02
@ReubenBond
ReubenBond force-pushed the rb-arc-serialization-foundations branch from 6ef3bcc to e6830c3 Compare October 11, 2026 01:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Pooled-page ownership and lifetime semantics are high-risk and depend on the still-unmerged prerequisite PR #11474.

0 open findings

🧠 Review effort: Balanced

@ReubenBond
ReubenBond force-pushed the rb-arc-serialization-foundations branch from e6830c3 to 9b8cba9 Compare October 11, 2026 01:21

This branch has not been deployed

No deployments
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