Skip to content

fix(streaming): reapply checkpoint advancement with safe recovery - #11269

Open
ReubenBond wants to merge 13 commits into
dotnet:mainfrom
ReubenBond:rb-checkpoint-recovery-progress
Open

ReubenBond wants to merge 13 commits into
dotnet:mainfrom
ReubenBond:rb-checkpoint-recovery-progress

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 16, 2026

Copy link
Copy Markdown
Member

Purpose and release sequencing

This PR re-attempts the checkpoint advancement introduced by #11249, which was reverted by #11330. The original optimization could remain disabled for an agent's lifetime after a recoverable failure. The re-attempt establishes explicit recovery, ownership and retention guarantees before advancing checkpoints.

Release gate: cut a release from the reverted state before considering this PR for a later release. The reverted tree is a standalone release state. It restores subscription-based checkpoints while preserving #11268's shutdown-draining and handshake-ownership guarantees. Idle subscriptions can still pin conservative checkpoint progress, with the associated replay tradeoff accepted for that release.

#11330 is merged as 39ea49999e. This branch has now been rebased onto that actual main commit, retaining the explicit reapplication and recovery rework. All 13 reapplication/fix commits are patch-identical to the prepared series. The ordinary PR diff now includes the complete reapplication against the reverted baseline. When the intervening release is published, renew compatibility validation against that released baseline before landing this PR.

Solution

The built-in Event Hubs components cooperate through an internal certified-prefix protocol. Cursors scan unrelated records and retain selected matching records until acknowledged, filtered, or explicitly skipped by delivery-failure policy. Recovery reconciles receiver-owned source positions and staged notifications; registration and handshake ownership remain independent barriers. The minimum subscription prefix, bounded by the last fully accounted read, governs checkpoint publication and Event Hubs eviction.

Unresolved work retries on pump ticks. Certified progress is published before every read-loop capacity check, so sustained nonempty reads refresh checkpoint and purge authority. Same-cursor re-handshakes retain and rewind unsettled selections when older deliveries finish; replacement cursors retain their requested replay positions. Removing the last unresolved subscriber releases a settled registration pin. Sticky rewind misses preserve the unresolved range while the consumer and failure handler receive the error. The shutdown-draining and handshake-ownership guarantees from #11268 are preserved.

Capacity controls new reception. Already-accepted read accounting, read recovery, and registration retries continue at zero capacity, including when a receiver notification handoff or the agent's admission/discovery accounting filled the cache.

Failure policy and bounded retention

StreamPullingAgentOptions.RetryFailedDeliveries defaults to false. After the existing delivery retry budget is exhausted, the built-in Event Hubs path notifies the consumer and failure handler, skips that selected batch, and continues. Those events can be absent from the subscription's output, and checkpointing can advance past them.

Setting the option to true retains the batch for later pump retries and holds checkpointing and reclamation until it is resolved or the subscription is removed. Subscription faulting/removal follows the configured failure handler. Both settings preserve failed-read, incomplete-selection, cache-miss, and handshake replay obligations. Ownership changes defer skip decisions and preserve replay.

Native certified admission reserves one possible new pool buffer per requested record, independently of averaged consumer pressure. The existing cache constructor's defaultMaxAddCount supplies both maximum read size and buffer budget. The native factory permits 1,000 buffers of 1 MiB per partition. Deployment sizing also includes metadata, SDK prefetch, free pooled buffers and partition count. A factory returning the native cache can choose a smaller constructor budget. Age-eligible, certified records release capacity; staged handoffs can finish while new reception is paused. Packing rollback and exception-safe purge cleanup preserve actual buffer ownership, including when observers throw.

Public surface and compatibility

Recovery, cursor progress, filtering and checkpoint capabilities are internal. Publication reuses the released IQueueCache.UpdateDeliveryProgress callback. Public additions remain the opt-in delivery-policy boolean and IPurgeObservable.TryRemoveOldestMessage, whose default calls the existing removal method and returns true. The Event Hubs API diff records an explicit implementation of the existing ICacheDataAdapter.Compare member.

The Orleans 10.3.1 nullable callback/default and existing custom or derived cache, transport, adapter and eviction behavior are preserved. Ordinary receipt providers keep their notification/skip policy. Certified processing is selected for the native Event Hubs composition and stays fixed for the provider run. Normal Release package validation and unchanged 10.3.1 binary implementations/callers pass without compatibility suppressions.

On the actual post-revert baseline 39ea49999e, 314 shared/Functional and 78 Event Hubs/monitor cases pass on each of net8.0 and net10.0. Coverage includes handshake ownership, both failure policies, error notification, registration cleanup, continuous publication, partial records, atomic admission, full-cache recovery, capacity reopening and guarded reclamation. The rebase preserves the streaming implementation, tests and documentation unchanged.

Runtime cost

Already-accounted cursors stay idle while the read boundary is unchanged. Mutation-safe traversals reuse pooled snapshots. Certified bookkeeping is enabled only for checkpoint-capable owners; ordinary grouped delivery tracks node ranges with explicit mappings only for sliced records.

A historical 22-case actual-build BenchmarkDotNet comparison measured the per-read publication/agent repairs against 660fba8e3d. Warm idle allocation remained 208 B/tick at 1, 32 and 256 subscriptions, and every active allocation result was unchanged. Elapsed differences ranged from approximately 8% lower to 7% higher; the one-subscription certified case increased from 22.10 to 23.71 microseconds per 64-input operation. The repaired fixture verified certification through the current completed read, whereas the prior implementation reported the preceding pump's prefix.

Earlier original-base comparisons showed certified processing approximately 6–15% above the successful-delivery baseline, and ordinary receipt cases from 3% faster to 11% slower. These measurements predate the final buffer-cleanup/full-cache-gate changes and subsequent rebases; they are not fresh-head throughput claims.

Measurements use .NET 10.0.12, BenchmarkDotNet 0.15.8, Windows Hyper-V and real agent/SimpleQueueCache code with synchronous local consumers. They exclude Event Hubs pooled-cache admission, SDK/network, remote RPC, serialization and checkpoint-store I/O. VM variation applies. The earlier additional approximately 214 KB/s of warm idle allocation was eliminated.

Shared boundaries remain separate from #10588's positioning work, #10316's adaptive pressure policy and #7464's persisted checkpoint reset. This update does not approve, merge, release, or enable auto-merge on this PR.

Copilot AI lite review requested due to automatic review settings September 16, 2026 04:44

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.

Copilot review overview

🔵 Needs a closer look

The recovery and checkpointing changes span runtime behavior and integration coverage, requiring final human review.

Review tier: Lite
Findings: None

What changed in this PR

Restores checkpoint progress after verified retained-cursor recovery while preserving conservative fallback for uncertain history.

Changes:

  • Implements recovery proof and fallback tracking.
  • Adds unit and Event Hubs regression coverage.
  • Updates internals access and recovery documentation.
File Description
test/​Orleans.Streaming.Tests/​StreamingTests/​PersistentStreamPullingAgentTests.cs Adds recovery and fallback regression tests.
test/​Extensions/​Orleans.Streaming.EventHubs.Tests/​CheckpointerTests/​EventHubCheckpointRecoveryTests.cs Adds Event Hubs checkpoint/resume coverage.
src/​Orleans.Streaming/​PersistentStreams/​PersistentStreamPullingAgent.cs Implements verified recovery and progress tracking.
src/​Orleans.Streaming/​Orleans.Streaming.csproj Grants test access to internals.
src/​Orleans.Runtime/​Orleans.Runtime.csproj Grants test access to runtime internals.
docs/​site/​src/​content/​docs/​implementation/​streams-implementation/​index.md Documents checkpoint recovery behavior.

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

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request
Lines 82.83% (115,677 / 139,653)
Branches 72.27% (33,841 / 46,824)

Report-only conclusion: current-main baseline stale.

The newest successful coverage run tested 39ea499, not current main 598c0d2.

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 review requested due to automatic review settings September 16, 2026 19:17
@ReubenBond
ReubenBond force-pushed the rb-checkpoint-recovery-progress branch from c50794e to 5cb17c1 Compare September 16, 2026 19:17
@ReubenBond ReubenBond changed the title fix(streaming): restore checkpoint progress after retained cursor recovery feat(streaming)!: use certified checkpoint prefixes Sep 16, 2026

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.

Copilot review overview

🟡 Changes recommended

Six unresolved findings remain, including three critical checkpoint and recovery issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 High severity · 1 Medium severity

Open (4)

Comment thread src/Orleans.Streaming/PersistentStreams/PersistentStreamPullingAgent.cs Outdated
Copilot AI review requested due to automatic review settings September 16, 2026 19: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.

Copilot review overview

🟡 Changes recommended

Unresolved Event Hubs cleanup, contract validation, eviction performance, and read-recovery issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity · 1 Medium severity

Open (5)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid a linear scan for every buffer allocation

src/​Orleans.Streaming/​Common/​PooledCache/​ChronologicalEvictionStrategy.cs:63

Queue<T>.Contains makes every normal buffer allocation scan all currently owned buffers, turning repeated allocation into O(n²) work as a cache retains more blocks. Keep a hash set alongside the purge queue (removing entries when buffers are freed) so the idempotence check remains O(1).

Copilot AI review requested due to automatic review settings September 16, 2026 20:24

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.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in cache validation, cursor failure handling, receipt-cache retries, read recovery, and Event Hubs buffer cleanup.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 7 High severity · 1 Medium severity

Open (8)

Comment thread src/Orleans.Streaming/Common/SimpleCache/SimpleQueueCacheCursor.cs
Copilot AI review requested due to automatic review settings September 16, 2026 22:47
@ReubenBond
ReubenBond force-pushed the rb-checkpoint-recovery-progress branch from eab9f7d to c96b091 Compare September 16, 2026 22:47
@ReubenBond ReubenBond changed the title feat(streaming)!: use certified checkpoint prefixes feat(streaming): add compatible certified checkpoint progress Sep 16, 2026

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.

Copilot review overview

🟡 Changes recommended

A nullability mismatch in EventHubAdapterReceiver.UpdateDeliveryProgress vs ICheckpointingQueueCache.UpdateDeliveryProgress will likely surface as warnings-as-errors and should be corrected before merge.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 7 High severity · 1 Medium severity

Open (8)
Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 17, 2026 20:19

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.

Copilot review overview

🔵 Needs a closer look

It introduces broad, behavior-changing streaming runtime contracts (caches, cursors, receiver recovery, Event Hubs eviction/checkpointing) that warrant final human review despite strong test coverage.

Review effort: Lite
Findings: 7 High severity · 1 Medium severity

Open (8)
Previously missed (1)

In code that hasn't changed since last review

Medium severity GetEvents<T>() slices after casting, which can throw on skipped events

src/​Azure/​Orleans.Streaming.EventHubs/​Providers/​Streams/​EventHub/​EventHubBatchContainer.cs:121

GetEvents() casts every payload event to T before applying the slice (Skip(firstEventIndex)). Since the payload is a List, this can throw if any skipped event is not of type T, even though it should be excluded by the slice. It also does unnecessary work for skipped events.

Consider iterating from firstEventIndex and casting only the events which will actually be returned.

Copilot AI review requested due to automatic review settings September 17, 2026 20:59
@ReubenBond ReubenBond changed the title feat(streaming): add compatible certified checkpoint progress fix(streaming): restore checkpoint progress after recoverable failures Sep 17, 2026

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.

Copilot review overview

🟡 Changes recommended

PersistentStreamPullingAgent.ReadFromQueue clears _pendingRead on an empty batch without resetting _pendingReadAdded, which can leave inconsistent state and skip cache admission on the next read.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 7 High severity · 2 Medium severity

Open (9)

Copilot AI review requested due to automatic review settings September 17, 2026 22:08

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.

Copilot review overview

🟡 Changes recommended

There are unhandled exception paths in the new certified-progress rewind logic (RecordDeliveryFailure) and a likely hot-path performance regression in ChronologicalEvictionStrategy which should be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 8 High severity · 2 Medium severity

Open (10)
Previously missed (1)

In code that hasn't changed since last review

Medium severity O(n) Queue.Contains in OnBlockAllocated may regress hot-path

src/​Orleans.Streaming/​Common/​PooledCache/​ChronologicalEvictionStrategy.cs:64

ChronologicalEvictionStrategy.OnBlockAllocated now calls inUseBuffers.Contains(newBlock) on every allocation. Since inUseBuffers is a Queue, this is O(n) per buffer allocation and can become a significant hot-path cost as the number of in-use buffers grows. Preventing duplicate notifications likely needs a separate O(1) membership structure (eg, a HashSet of buffer Ids) which is updated when buffers are dequeued/disposed in FreePurgedBuffers.

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.

Comment on lines 207 to +216
public async Task CloseAsync(CancellationToken cancellationToken)
{
await client.CloseAsync(cancellationToken);
try
{
await client.CloseAsync(cancellationToken);
}
finally
{
if (connection is not null) await connection.CloseAsync(cancellationToken);
}
Comment on lines +953 to +957
var consumer = consumers[j];
if (consumer.PendingHandshakes == 0 && consumer.HasUnresolvedHandshake)
{
AddSubscriber_Impl(consumer.SubscriptionId, consumer.StreamId, default, consumer.FilterData, consumer.PendingStartToken).Ignore();
}

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.

Copilot review overview

🔵 Needs a closer look

A confirmed InvalidCastException risk exists in certified Event Hubs cache capacity calculation when certified mode is forced on with non-chronological eviction strategies.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity GetMaxAddCount can throw InvalidCastException when certifiedDeliveryProgress is…

src/​Azure/​Orleans.Streaming.EventHubs/​Providers/​Streams/​EventHub/​EventHubQueueCache.cs:198

GetMaxAddCount unconditionally casts evictionStrategy to ChronologicalEvictionStrategy when certifiedDeliveryProgress is true. Since EnableCertifiedDeliveryProgress() can be called even with non-chronological strategies (eg, fault-injection tests), this can crash the receiver with an InvalidCastException. Guard the cast (or validate the strategy when enabling certified mode) so certified mode cannot be enabled into an incompatible eviction strategy silently.

Medium severity Linear Contains() in OnBlockAllocated makes buffer tracking O(n^2)

src/​Orleans.Streaming/​Common/​PooledCache/​ChronologicalEvictionStrategy.cs:66

OnBlockAllocated uses inUseBuffers.Contains(newBlock) to make allocation notifications idempotent. Since Queue.Contains is O(n) and this runs for every allocated block, the total work becomes O(n^2) as buffer counts grow (up to 1,000 buffers/partition per the new certified budget). Consider tracking allocated buffers with a HashSet (removing entries when buffers are freed) or otherwise making the idempotence check O(1).

ReubenBond added a commit to ReubenBond/orleans that referenced this pull request Sep 18, 2026
Reapply the checkpoint-advancement portion of dotnet#11249 after its dedicated revert (4bf635d). The following commits in dotnet#11269 replace lifetime fallback with certified recovery and preserve the reviewed final implementation. Land the revert first, then rebase this re-attempt onto its actual main merge commit.
Copilot AI review requested due to automatic review settings September 18, 2026 22:26
@ReubenBond
ReubenBond force-pushed the rb-checkpoint-recovery-progress branch from 69faa36 to 2357d92 Compare September 18, 2026 22:26
@ReubenBond ReubenBond changed the title fix(streaming): restore checkpoint progress after recoverable failures fix(streaming): reapply checkpoint advancement with safe recovery Sep 18, 2026

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.

Copilot review overview

🟡 Changes recommended

There are confirmed runtime-exception/race risks in the new retry-handshake path and in certified Event Hubs max-add-count computation which need to be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 Medium severity

Open (4)

Comment on lines +195 to +198
return certifiedDeliveryProgress
? Math.Max(0, defaultMaxAddCount - ((ChronologicalEvictionStrategy)evictionStrategy).BufferCount
- (pendingBuffers.Count - pendingBufferNotification))
: defaultMaxAddCount;
Comment on lines +955 to +958
if (consumer.PendingHandshakes == 0 && consumer.HasUnresolvedHandshake)
{
AddSubscriber_Impl(consumer.SubscriptionId, consumer.StreamId, default, consumer.FilterData, consumer.PendingStartToken).Ignore();
}
Reapply the checkpoint-advancement portion of dotnet#11249 after its dedicated revert (4bf635d). The following commits in dotnet#11269 replace lifetime fallback with certified recovery and preserve the reviewed final implementation. Land the revert first, then rebase this re-attempt onto its actual main merge commit.
BREAKING CHANGE: checkpointing providers implement ICheckpointingQueueCache, certified cursor progress and receiver-owned read recovery. The legacy IQueueCache.UpdateDeliveryProgress callback is removed. Custom Event Hubs caches and eviction strategies must enforce certified purge boundaries and idempotent buffer ownership.
Copilot AI review requested due to automatic review settings September 18, 2026 23:56
@ReubenBond
ReubenBond force-pushed the rb-checkpoint-recovery-progress branch from 2357d92 to a3aed48 Compare September 18, 2026 23:56

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.

Copilot review overview

🔵 Needs a closer look

The change set is large and touches core streaming correctness (checkpointing, eviction, and recovery), and at least one concrete correctness issue was found which should be addressed before approval.

Review effort: Lite
Findings: 5 Medium severity

Open (5)

Comment on lines 62 to 66
public void OnBlockAllocated(FixedSizeBuffer newBlock)
{
if (inUseBuffers.Contains(newBlock)) return;
this.inUseBuffers.Enqueue(newBlock);
//report metrics
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