Skip to content

fix(runtime): coalesce grain timer ticks and preserve delayed wakeups - #11464

Merged
ReubenBond merged 9 commits into
dotnet:mainfrom
ReubenBond:rb-grain-timer-scheduling
Oct 11, 2026
Merged

ReubenBond merged 9 commits into
dotnet:mainfrom
ReubenBond:rb-grain-timer-scheduling

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Queue immediately due grain timer ticks directly as local-only activation requests instead of routing them through the physical timer. Preserve normal interleaving and keep-alive rules and start a fresh call chain. Delayed timing and resolution remain owned by the configured TimeProvider. Timer messages use the factory's explicit context-free path so ambient request data is neither copied nor mutated; ordinary requests retain their existing context export behavior.

Keep the physical timer eagerly created and readonly. Use one Idle/Busy/Disposed lifecycle, where Busy covers both the queued turn and its running callback. Reserving a turn defaults its following delay to the period; Change replaces that following schedule. A queued callback keeps its turn, and changes while queued or running take effect after it completes, with the latest due time and period winning. This avoids separate queued/running bookkeeping and replacement-arm invalidation.

A small state lock coordinates provider callbacks with grain-side scheduling and runtime disposal. Enqueue local activation messages and run cancellation registrations outside that lock to preserve shutdown ordering. Disposal is terminal before cancellation registrations execute, so queued messages drain and admitted callbacks receive cancellation. Activation enqueueing is a local operation: the timer relies on that guarantee rather than modeling recoverable delivery failure or resetting and re-arming its schedule. The existing boundary logger reports unexpected runtime exceptions; application callback and cancellation-registration exceptions retain their established handling.

The focused tests cover distinct contracts: immediate activation admission, normal versus context-free message construction, queued/running changes, successive zero-period turns, provider timing, in-flight tick races, disposal and cancellation, callback exceptions, and legacy registration. Shared setup consolidates registration, diagnostics and logging assertions. The complete shared timer fixture also arms readiness observers before triggering work.

This carries the timer repair investigated in #11451 and proposed in #11454 onto the narrowed provider-owned implementation. The configured provider's tick notifications are admitted through the lifecycle state machine, preserving queue-pump progress when timer resolution produces a pre-deadline notification. The investigations retain their individually recorded historical stream failures.

This standalone timer extraction is based on main b3b76e2ab207f00e664e0167cd733800cd60379d. It originated from the timer-only source of #10693 at 9c1ef0f2feb258c1764de5f408d89c53e7e63f9f, extracted using original base 48f3aaa47fc18123db4109063af591f315e6f9cb, and has been narrowed through user-directed state, timing, lifecycle, and review-surface simplification.

Microsoft Reviewers: Open in CodeFlow

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.

🟡 Changes recommended

Disposal permits a queued interleavable callback to observe an uncanceled token before cancellation completes.

1 open finding
What changed in this PR

Refactors grain timers to coalesce immediate ticks while preserving delayed wakeups and activation scheduling semantics.

Changes:

  • Queues immediate ticks directly and lazily creates physical timers.
  • Adds deterministic coverage for scheduling, coalescing, cancellation, and stale callbacks.
  • Documents updated timer behavior.
File Description
src/​Orleans.Runtime/​Timers/​GrainTimer.cs Implements the new scheduling state machine.
src/​Orleans.Runtime/​Timers/​TimerRegistry.cs Starts timers through the new initialization path.
test/​Orleans.Core.Tests/​Runtime/​GrainTimerTests.cs Adds focused timer behavior tests.
docs/​site/​src/​content/​docs/​grains/​timers.md Documents scheduling and lifecycle semantics.

🧠 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.Runtime/Timers/GrainTimer.cs Outdated
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request Current main Variance
Lines 83.45% (118,576 / 142,096) 83.43% (118,525 / 142,062) +0.0159 pp
Branches 73.04% (35,284 / 48,308) 73.02% (35,265 / 48,292) +0.0151 pp

Report-only conclusion: improved.

The current-main baseline is commit bfe0e81e37 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 18:33
@ReubenBond
ReubenBond force-pushed the rb-grain-timer-scheduling branch from a4fe254 to 2c3459d Compare October 10, 2026 18:33

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

Concurrent admission failure can discard a newer immediate schedule, and additional performance and documentation issues remain.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Comment thread docs/site/src/content/docs/grains/timers.md Outdated
Comment thread src/Orleans.Runtime/Timers/GrainTimer.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 21:04

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

Admission failure can discard a newer immediate schedule, and immediate ticks unnecessarily copy request-context data.

3 open findings

🧠 Review effort: Balanced

Comment thread src/Orleans.Runtime/Timers/GrainTimer.cs Outdated

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

A failed admission can discard a newer immediate schedule, alongside unresolved performance and documentation issues.

4 open findings

🧠 Review effort: Balanced

Comment thread src/Orleans.Runtime/Timers/GrainTimer.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 10, 2026 23: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

Timer message creation unnecessarily deep-copies and then discards request-context data on every tick.

4 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 00: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.

🔵 Needs a closer look

The test diagnostic observer writes concurrently to a non-thread-safe list, risking flaky lifecycle assertions.

0 open findings

4 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Concurrent fixture event writes cause flaky lifecycle assertions

test/​Orleans.Core.Tests/​Runtime/​GrainTimerTests.cs:578

This observer is called synchronously on whichever thread emits the diagnostic event, but DisposeDuringCallback_ReentrantFailingCancellationCompletesDisposalOnce can emit Disposed from the disposing thread while the asynchronously resumed callback emits TickStop. Concurrent List<T>.Add calls can corrupt or lose fixture events, making the lifecycle assertions flaky. Serialize writes here (or use a concurrent collection).

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 14:22

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

Public API XML documentation contradicts the newly documented queued-tick behavior.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 15:31

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

Core timer scheduling and disposal behavior is concurrency-sensitive and warrants final human review despite strong focused coverage.

0 open findings

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 11, 2026 16: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.

🔵 Needs a closer look

The concurrency-sensitive runtime lifecycle changes warrant final human validation despite comprehensive focused tests.

0 open findings

🧠 Review effort: Balanced

@ReubenBond
ReubenBond merged commit 79873eb into dotnet:main Oct 11, 2026
94 checks passed
@ReubenBond
ReubenBond deleted the rb-grain-timer-scheduling branch October 11, 2026 20:24
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